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

Ensure empty withdrawal lists are set in BFT blocks when the protocol spec is shanghai or higher #6765

Merged
merged 5 commits into from
Mar 27, 2024

Conversation

matthew1001
Copy link
Contributor

@matthew1001 matthew1001 commented Mar 19, 2024

PR description

If the protocol spec is as shanghai or higher, a valid (if empty) withdrawals list must be present in the blocks proposed.

Fixed Issue(s)

Fixes #6760

… schedule is shanghai or higher

Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matthew Whitehead <[email protected]>
final Block block = blockCreator.createBlock(headerTimeStampSeconds).getBlock();

// Determine if we're at a protocol spec that requires withdrawals (even if empty) to be present
ProtocolSpec protocolSpec =
Copy link
Contributor

Choose a reason for hiding this comment

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

This is duplicated several times. It feels like we can do this in BftBlockCreator by overriding createBlock, checking the withdrawal validator, and switching on that. If the sequence number is truly needed then making a new createBlock method with both args. May need to also do it for PkuQbftBlockCreator since there is no shared parent classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good thought. I've done some refactoring as BftBlockCreator already has access to the parent header required to get the next block number. So it was a little easier than I thought. Now QbftRound and IbftRound can just continue to call createBlock(...) as they did before, and BftBlockCreator.createBlock(...) can decide which type of block to actually create. There's a little repetition in PkiQbftBlockCreator like you say, but it's definitely tidier now.

@matthew1001 matthew1001 force-pushed the shanghai-qbft-fix branch 5 times, most recently from 036b7e2 to 41bf3e0 Compare March 21, 2024 15:13
Signed-off-by: Matthew Whitehead <[email protected]>
@matthew1001 matthew1001 enabled auto-merge (squash) March 27, 2024 16:18
@matthew1001 matthew1001 merged commit 5bc81ae into hyperledger:main Mar 27, 2024
42 checks passed
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
… spec is shanghai or higher (hyperledger#6765)

* Ensure empty withdrawal lists are set in BFT blocks when the protocol schedule is shanghai or higher

Signed-off-by: Matthew Whitehead <[email protected]>

* Unit tests missing mock

Signed-off-by: Matthew Whitehead <[email protected]>

* Unit tests missing mock

Signed-off-by: Matthew Whitehead <[email protected]>

* Refactor and reduce code duplication

Signed-off-by: Matthew Whitehead <[email protected]>

---------

Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
Signed-off-by: amsmota <[email protected]>
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
… spec is shanghai or higher (hyperledger#6765)

* Ensure empty withdrawal lists are set in BFT blocks when the protocol schedule is shanghai or higher

Signed-off-by: Matthew Whitehead <[email protected]>

* Unit tests missing mock

Signed-off-by: Matthew Whitehead <[email protected]>

* Unit tests missing mock

Signed-off-by: Matthew Whitehead <[email protected]>

* Refactor and reduce code duplication

Signed-off-by: Matthew Whitehead <[email protected]>

---------

Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
Signed-off-by: amsmota <[email protected]>
@cryptohazard
Copy link

hello,
could you confirm that this update only work for network that will now go through a Shanghai update and not for previous networks that already activated Shanghai with an older version of Besu.
I'm having an issue related to that #7036 (comment)

@ex-shubham-koli
Copy link

+following

This was referenced May 16, 2024
matthew1001 added a commit to kaleido-io/besu that referenced this pull request Jun 7, 2024
… spec is shanghai or higher (hyperledger#6765)

* Ensure empty withdrawal lists are set in BFT blocks when the protocol schedule is shanghai or higher

Signed-off-by: Matthew Whitehead <[email protected]>

* Unit tests missing mock

Signed-off-by: Matthew Whitehead <[email protected]>

* Unit tests missing mock

Signed-off-by: Matthew Whitehead <[email protected]>

* Refactor and reduce code duplication

Signed-off-by: Matthew Whitehead <[email protected]>

---------

Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matt Whitehead <[email protected]>
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.

QBFT/Shanghai blocks failing withdrawal validation on import
4 participants