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

Refactoring around MiningParameters #7798

Merged
merged 10 commits into from
Oct 31, 2024
Merged

Conversation

jflo
Copy link
Contributor

@jflo jflo commented Oct 22, 2024

  • optionalized some fields of ProtocolSchedule constructor, which will allow us to reduce number of constructors later.
  • Refactored JsonBlockImporterTest so it can provide a BesuComponent
  • ProtocolScheduleBuilder optionalizes the default chainid
  • Blocks SubCommand is now Dagger aware
  • Creates but rarely uses ProtocolSpec and ProtocolSchedule modules. Deeper adoption pending.
  • Adds a Coinbase module with fixed values for use in testing.
  • Introduces an EthereumCoreComponent, to be used as a subcomponent of BesuComponent in the future.
  • Introduces a MiningParameters module with common static values used by tests. Removes MiningParameters static constructor.

tested by syncing to the head of ephemery testnet

Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

Looks good. Some comments, dagger questions, etc.

@@ -132,7 +132,6 @@ protected ProtocolSchedule createProtocolSchedule() {
genesisConfigOptions,
forksSchedule,
nodeKey,
privacyParameters,
Copy link
Contributor

Choose a reason for hiding this comment

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

it isn't obvious to me why we can use the default privacy parameters here.

Copy link
Contributor

@garyschulte garyschulte Oct 31, 2024

Choose a reason for hiding this comment

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

why is it safe to use this constructor which uses default privacy parameters rather than the superclass' privacy parameters which may (or may not) have been set on the CLI. This is strictly an enterprise concern, but it seems like we are breaking clique networks that have privacy enabled here.

@jflo jflo enabled auto-merge (squash) October 31, 2024 16:01
Signed-off-by: Justin Florentine <justin+github@florentine.us>
Signed-off-by: Justin Florentine <justin+github@florentine.us>
Signed-off-by: Justin Florentine <justin+github@florentine.us>
Signed-off-by: Justin Florentine <justin+github@florentine.us>
Signed-off-by: Justin Florentine <justin+github@florentine.us>
Signed-off-by: Justin Florentine <justin+github@florentine.us>
Signed-off-by: Justin Florentine <justin+github@florentine.us>
Signed-off-by: Justin Florentine <justin+github@florentine.us>
Signed-off-by: jflo <justin+github@florentine.us>
Signed-off-by: jflo <justin+github@florentine.us>
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

LGTM, but I still have that question about why we are dropping privacy params for clique protocol scheduels

@jflo jflo merged commit 7eb7b87 into hyperledger:main Oct 31, 2024
43 checks passed
@jflo
Copy link
Contributor Author

jflo commented Nov 1, 2024

LGTM, but I still have that question about why we are dropping privacy params for clique protocol scheduels

Sorry, my reply must've gotten dropped. The answer is that it was only ever passed in as a hardcoded default.

@garyschulte
Copy link
Contributor

LGTM, but I still have that question about why we are dropping privacy params for clique protocol scheduels

Sorry, my reply must've gotten dropped. The answer is that it was only ever passed in as a hardcoded default.

This still looks wrong imo. The link you provides is just the alternate create() that doesn't take privacy parameters. ❓

As I see it, BesuCommand is going to set privacy parameters on the controller builder, derived from CLI params. For a clique network, that will be a CliqueBesuControllerBuilder, with the privacy params set by BesuCommand.

In CliqueBesuControllerBuilder when we use the ProtocolSchedule.create method (the one with privacy params on 115) the privacy params from BesuCommand are used. But if we use the other create (155), we are discarding the BesuCommand configured privacy params.

It looks like the ProtocolScheduleBuilder that is return is indeed creating ProtocolSpec instances with the privacy params - so it isn't like they are being ignored. What am I missing?

JanetMo pushed a commit to JanetMo/besu that referenced this pull request Nov 17, 2024
decoupled parent block header from block creators
optionalized some fields of ProtocolSchedule constructor, which will allow us to reduce number of constructors later.
Refactored JsonBlockImporterTest so it can provide a BesuComponent
ProtocolScheduleBuilder optionalizes the default chainid
Blocks SubCommand is now Dagger aware
Creates but rarely uses ProtocolSpec and ProtocolSchedule modules. Deeper adoption pending.
Adds a Coinbase module with fixed values for use in testing.
Introduces an EthereumCoreComponent, to be used as a subcomponent of BesuComponent in the future.
Introduces a MiningParameters module with common static values used by tests. Removes MiningParameters static constructor.

---------

Signed-off-by: Justin Florentine <justin+github@florentine.us>
Signed-off-by: jflo <justin+github@florentine.us>
Signed-off-by: Marlene Marz <m.marz@kabelmail.de>
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