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

Make block txs selection max time aware of PoA transitions #6676

Conversation

fab-10
Copy link
Contributor

@fab-10 fab-10 commented Mar 4, 2024

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Most advanced CI tests are deferred until PR approval, but you could:

  • locally run all unit tests via: ./gradlew build
  • locally run all acceptance tests via: ./gradlew acceptanceTest
  • locally run all integration tests via: ./gradlew integrationTest
  • locally run all reference tests via: ./gradlew ethereum:referenceTests:referenceTests

PR description

As a follow up of #6596 this PR makes the block txs selection max time, correctly aware of the PoA transitions, while before only the block period from the genesis was used, while that value can change according to the transitions defined in the genesis file. To support that the genesisBlockPeriodSeconds has been renamed to blockPeriodSeconds and the configuration is now modifiable at runtime, according to the transitions schedule.

Fixed Issue(s)

@fab-10 fab-10 force-pushed the poa-block-txs-selection-max-time-transaction-aware branch from a53c184 to b6cd237 Compare March 4, 2024 16:22
@fab-10 fab-10 changed the title Make block txs selection time PoA transaction aware Make block txs selection max time aware of PoA transactions Mar 4, 2024
@fab-10 fab-10 marked this pull request as ready for review March 4, 2024 16:51
@fab-10 fab-10 force-pushed the poa-block-txs-selection-max-time-transaction-aware branch from b6cd237 to df163ff Compare March 4, 2024 17:35
Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

Wiring runtime values into MiningParameters is making me think twice, but seems there is a precedent for it already.

Also the way the block period is set on MiningParameters but I don't feel too strongly about it.

Other than that, PR looks good :)

CHANGELOG.md Outdated Show resolved Hide resolved
@fab-10 fab-10 changed the title Make block txs selection max time aware of PoA transactions Make block txs selection max time aware of PoA transitions Mar 6, 2024
@fab-10 fab-10 requested a review from jframe March 6, 2024 17:07
@fab-10 fab-10 force-pushed the poa-block-txs-selection-max-time-transaction-aware branch from 8da2a03 to a7b5b2f Compare March 6, 2024 17:20
@@ -83,6 +84,14 @@ private static MiningBeneficiaryCalculator miningBeneficiaryCalculator(
forksSchedule.getFork(blockNum).getValue().getMiningBeneficiary().orElse(localAddress);
}

private void setBlockPeriodSeconds(
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving this into the controller same way you did with Clique would be nicer

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, please re-review

@fab-10 fab-10 requested a review from jframe March 7, 2024 11:14
@fab-10 fab-10 force-pushed the poa-block-txs-selection-max-time-transaction-aware branch from 66c1576 to 216c405 Compare March 7, 2024 11:31
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10 fab-10 force-pushed the poa-block-txs-selection-max-time-transaction-aware branch from 216c405 to 48af465 Compare March 12, 2024 09:32
@fab-10 fab-10 enabled auto-merge (squash) March 12, 2024 09:33
@fab-10 fab-10 merged commit 65f8880 into hyperledger:main Mar 12, 2024
45 checks passed
MASDXI pushed a commit to MASDXI/besu that referenced this pull request Mar 13, 2024
…er#6676)

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: MASDXI <sirawitt42@gmail.com>
matthew1001 pushed a commit to kaleido-io/besu that referenced this pull request Jun 7, 2024
…er#6676)


Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
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