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

Added a Block Config separate from Mining Config to reduce PoS confusion (issue #4836) #4847

Merged
merged 8 commits into from
Nov 22, 2022

Conversation

OlegJakushkin
Copy link
Contributor

Resolves #4836
The issue was that Mining.Enabled (that is extremely rarely used) made a user confused if it is needed to have the ability to set other Block creation parameters. With this PR, we enable Block parameters configuration separate from Mining parameter(s).
Backward compatibility and synchronisation are enabled on the MigrateConfigs step.

In other words, one might set, for example, Mining.Enabled, Mining.RandomizedBlocks, Blocks.RandomizedBlocks Blocks.ExtraData at the same time and:

  1. If RandomizedBlocks is the same on Mining and Blocks continue, otherwise get an exception
  2. Have properties in the Mining and Blocks configurations synchronised

Changes:

  • Added separate Block creation configuration settings
  • Deprecated them in the Mining configuration by adding relevant descriptions
  • Made config synchronisation step (based on an existing implementation)

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Testing

Added unit tests to test functionality as it is mostly cosmetic

  • Yes
  • No

In case you checked yes, did you write tests?

  • Yes
  • No

Kept compatibility with old configurations
Restoring configuration
To comply with All_default_values_are_correct test
Copy link
Contributor

@smartprogrammer93 smartprogrammer93 left a comment

Choose a reason for hiding this comment

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

Minor comments. Otherwise looks good.

src/Nethermind/Nethermind.Consensus/IMiningConfig.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Init/Steps/MigrateConfigs.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Consensus/MiningConfig.cs Outdated Show resolved Hide resolved
@OlegJakushkin OlegJakushkin merged commit ecabb23 into master Nov 22, 2022
@OlegJakushkin OlegJakushkin deleted the fix/issue-4836 branch November 22, 2022 20:10
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.

Mining module is confusing in the post merge world
2 participants