Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue (#2177) #2276

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Fix issue (#2177) #2276

wants to merge 11 commits into from

Conversation

VolodymyrBg
Copy link

@VolodymyrBg VolodymyrBg commented Dec 16, 2024

Issue being referred to : #2177

What I changed in this file:

  1. Special configuration for Nethermind

  2. Methods for configuration management

  3. Automatic setup of Nethermind parameters

  4. Configuration validation capability

I think now when using Nethermind, optimized settings will be applied which should help resolve the block finalization issue.

Signed-off-by: VolodymyrBg <[email protected]>
@@ -13,7 +13,7 @@
// LICENSOR AS EXPRESSLY REQUIRED BY THIS LICENSE).
//
// TO THE EXTENT PERMITTED BY APPLICABLE LAW, THE LICENSED WORK IS PROVIDED ON
// AN AS IS BASIS. LICENSOR HEREBY DISCLAIMS ALL WARRANTIES AND CONDITIONS,
// AN "AS IS" BASIS. LICENSOR HEREBY DISCLAIMS ALL WARRANTIES AND CONDITIONS,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert this change. We should fix all headings altogether if they are not right

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected

return jsonBytes
}

// ValidateNethermindConfig validates the Nethermind configuration
func (c *E2ETestConfig) ValidateNethermindConfig() error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't know for sure if this solves Nethermind issues unless we make use of it.
In order to do that consider re-introducing nethermind replicas in testing/e2e/config/defaults.go in stead of geth or reth ones

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, pls test the scenario mentioned in #2177 and share the results, that will help us in knowing if this fix really works. Thanks for your work!

Copy link
Collaborator

@abi87 abi87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting work, left some comments to complete the PR

@nidhi-singh02
Copy link
Contributor

Issue referred to #2177

Signed-off-by: VolodymyrBg <[email protected]>
Signed-off-by: VolodymyrBg <[email protected]>
@@ -83,13 +83,14 @@ type NodeSettings struct {
ExecutionSettings ExecutionSettings `json:"execution_settings"`
}

// ExecutionSettings holds the configuration for the execution layer
// clients.
// ExecutionSettings holds the configuration for the execution layer clients.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: not really required

Comment on lines 170 to 171
ConsensusConfig struct {
ForceSealing bool `json:"ForceSealing"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ForceSealing is part of Aura namespace, pls check the correct usage. Refer https://docs.nethermind.io/fundamentals/configuration/#aura-forcesealing

I am curious why do we need it ?
Another point to note is it is true by default.

Signed-off-by: VolodymyrBg <[email protected]>
Signed-off-by: VolodymyrBg <[email protected]>
Signed-off-by: VolodymyrBg <[email protected]>
Signed-off-by: VolodymyrBg <[email protected]>
@nidhi-singh02 nidhi-singh02 marked this pull request as draft December 17, 2024 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants