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

Don't cache protocol spec in block creator #4982

Merged

Conversation

shemnon
Copy link
Contributor

@shemnon shemnon commented Jan 23, 2023

PR description

Don't cache the protocol spec in the block creator. With the new shanghaiTimestamp the correct spec may be a function of the timestamp not just the block number. So every time we are asked to build a block re-query the spec.

Signed-off-by: Danno Ferrin danno.ferrin@swirldslabs.com

Fixed Issue(s)

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

Don't cache the protocol spec in the block creator.  With the new
shanghaiTimestamp the correct spec may be a function of the timestamp
not just the block number.  So every time we are asked to build a block
re-query the spec.

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
@shemnon shemnon requested a review from jframe January 23, 2023 10:39
@shemnon shemnon changed the title Don't cacne protocol spec in block creator Don't cache protocol spec in block creator Jan 23, 2023
Copy link
Contributor

@jframe jframe left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Was thinking we should have a test case (maybe in MergeBlockCreatorTest?) that proves Shanghai spec gets picked up, what do you think?

@@ -154,8 +153,16 @@ protected BlockCreationResult createBlock(
boolean rewardCoinbase) {

try (final MutableWorldState disposableWorldState = duplicateWorldStateAtParent()) {
final ProtocolSpec newProtocolSpec =
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: nextProtocolSpec? nextBlockProtocolSpec?

Don't cache the protocol spec in the block creator.  With the new
shanghaiTimestamp the correct spec may be a function of the timestamp
not just the block number.  So every time we are asked to build a block
re-query the spec.

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
@shemnon shemnon merged commit 9034d31 into hyperledger:main Jan 23, 2023
elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023
* Don't cache protocol spec in block creator

Don't cache the protocol spec in the block creator.  With the new
shanghaiTimestamp the correct spec may be a function of the timestamp
not just the block number.  So every time we are asked to build a block
re-query the spec.

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
* Don't cache protocol spec in block creator

Don't cache the protocol spec in the block creator.  With the new
shanghaiTimestamp the correct spec may be a function of the timestamp
not just the block number.  So every time we are asked to build a block
re-query the spec.

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
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