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

Clique block period transition #6596

Merged
merged 19 commits into from
Feb 28, 2024
Merged

Conversation

siladu
Copy link
Contributor

@siladu siladu commented Feb 20, 2024

Add BFT-style transitions to Clique, modelled with ForksSchedule<CliqueConfigOptions>

CliqueMiningAcceptanceTest heavily inspired by BftBlockRewardPaymentAcceptanceTest

Preparatory story to #6290 - a createemptyblocks transition will be added in the next PR.

Create genesis config transitions:

  "config": {
    ...
    "clique": {
      "blockperiodseconds": 3,
      "epochlength": 30,
      "requesttimeoutseconds": 6,
      "createemptyblocks": true
    },
    "transitions": {
      "clique": [
        {
          "block": 3,
          "blockperiodseconds": 1
        },
        {
          "block": 6,
          "blockperiodseconds": 2
        },
      ]
    }
  },

jframe and others added 5 commits October 20, 2023 11:38
Signed-off-by: Jason Frame <jason.frame@consensys.net>
Signed-off-by: Jason Frame <jason.frame@consensys.net>
Signed-off-by: Jason Frame <jason.frame@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Copy link

github-actions bot commented Feb 20, 2024

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests
  • I thought about running CI.
  • If I did not run CI, I ran as much locally as possible before pushing.

…locks

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@siladu siladu added the doc-change-required Indicates an issue or PR that requires doc to be updated label Feb 22, 2024
@siladu siladu marked this pull request as ready for review February 22, 2024 05:26
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
…locks

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@siladu siladu added the TeamGroot GH issues worked on by Groot Team label Feb 22, 2024
…locks

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@siladu siladu requested review from jframe and fab-10 February 26, 2024 02:49
@siladu siladu mentioned this pull request Feb 26, 2024
8 tasks
Copy link
Contributor

@fab-10 fab-10 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, just would avoid having the mutable config class, and minor stuffs

Comment on lines 43 to 48
final MutableCliqueConfigOptions cliqueConfigOptions =
new MutableCliqueConfigOptions(lastSpec.getValue());

fork.getBlockPeriodSeconds().ifPresent(cliqueConfigOptions::setBlockPeriodSeconds);

return cliqueConfigOptions;
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I see, there should not be a need for this mutable object, since if you use immutables annotation for CliqueConfigOptions then you can do something like this

var next = ImmutableCliqueConfigOptions.builder().from(lastSpec.getValue());
fork.getBlockPeriodSeconds().ifPresent(next::blockPeriodSeconds);
return next.build();

and avoid having mutable objects around, since the conf should newer change once it is created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This Mutable*ConfigOptions approach is pretty ingrained in the transition functionality used throughout the codebase, ie for BFT too. Can spike this out unless @jframe already knows there would be an issue with this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be fine to change it using a builder. It's only mutable for building the CliqueConfigOptions from the transitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done this for Clique, but not BFT

Copy link
Contributor

Choose a reason for hiding this comment

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

fine, the other part could be done later

…locks

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
…locks

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@siladu siladu requested a review from fab-10 February 28, 2024 02:55
@siladu siladu enabled auto-merge (squash) February 28, 2024 05:28
@siladu siladu merged commit 0e3d2f4 into hyperledger:main Feb 28, 2024
53 of 55 checks passed
@siladu siladu deleted the clique_transition_blocks branch March 5, 2024 03:22
@joaniefromtheblock joaniefromtheblock removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Mar 6, 2024
matthew1001 pushed a commit to kaleido-io/besu that referenced this pull request Jun 7, 2024
Add BFT-style transitions to Clique, modelled with ForksSchedule<CliqueConfigOptions>
Add ability to transition the blockperiodseconds config.

---------

Signed-off-by: Jason Frame <jason.frame@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Co-authored-by: Jason Frame <jason.frame@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TeamGroot GH issues worked on by Groot Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants