Skip to content

Comments

Fix empty body concept after shanghai#5174

Merged
gfukushima merged 7 commits intohyperledger:mainfrom
gfukushima:fix-empty-block-on-CompleteBlocksTask
Mar 7, 2023
Merged

Fix empty body concept after shanghai#5174
gfukushima merged 7 commits intohyperledger:mainfrom
gfukushima:fix-empty-block-on-CompleteBlocksTask

Conversation

@gfukushima
Copy link
Contributor

@gfukushima gfukushima commented Mar 6, 2023 •
edited by siladu
Loading

PR description

This PR adds withdrawals to the method that checks for empty block bodies in CompleteBlocksTask.java
Adding withdrawals allows Besu to be able to distinguish empty blocks after shanghai skiping the block body download when body is empty.
Prior to this PR Besu would understand blocks like (0txs, 0 ommers, x ws) as an empty block, during the sync process. (x > 0)

Fixed Issue(s)

Fixes #4976

This PR has performed the following tests:

  • Recreated and tested fix on zhejiang-testnet using FAST sync
  • Successfully synced Sepolia (starting from checkpoint and transitioning to Shanghai)
  • Successfully synced Goerli
  • Successfully synced Mainnet

Documentation

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

Acceptance Tests (Non Mainnet)

  • I have considered running ./gradlew acceptanceTestNonMainnet locally if my PR affects non-mainnet modules.

Changelog

@gfukushima
Fix empty body concept after shanghai
0c39601 
Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
@gfukushima gfukushima added bug Something isn't working TeamGroot GH issues worked on by Groot Team mainnet labels Mar 6, 2023
@gfukushima
Check protocolSchedule to create empty block and add withdrawals to e…
d4a6251 
…mpty block check

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
@gfukushima
Add unit tests
4f7c43d 
Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
jframe
jframe reviewed Mar 7, 2023
View reviewed changes
...m/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/tasks/CompleteBlocksTaskTest.java Outdated
}

@Test
public void shouldCompleteWhenBlocksAreEmptyExceptForWithdrawals() {
Copy link
Contributor

@jframe jframe Mar 7, 2023

Choose a reason for hiding this comment

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

Is this testing that when blocks are empty but have withdrawals they are still downloaded? i.e. that they are considered empty blocks. Was a bit confused from the test name

Copy link
Contributor

@siladu siladu Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is this testing that when blocks are empty but have withdrawals they are still downloaded?

yes, i.e. that they are not considered empty blocks like they were being before.

Any name suggestion? This was an modification of the existing test's name: shouldCompleteWithoutPeersWhenAllBlocksAreEmpty. Agree the new one could be better

@gfukushima
Extract block creation into a private method
f782406 
Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
@gfukushima gfukushima marked this pull request as ready for review March 7, 2023 09:25
@gfukushima
Nit changes
b9dcdb4 
Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
@gfukushima gfukushima requested review from garyschulte and jframe March 7, 2023 13:00
macfarla
macfarla approved these changes Mar 7, 2023
View reviewed changes
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

LGTM

@siladu
Refactor test names
74ba4cf 
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@gfukushima gfukushima enabled auto-merge (squash) March 7, 2023 21:33
@gfukushima gfukushima merged commit 3a3cca9 into hyperledger:main Mar 7, 2023
@gfukushima gfukushima deleted the fix-empty-block-on-CompleteBlocksTask branch March 23, 2023 22:58
@garyschulte garyschulte mentioned this pull request Apr 4, 2023
Merged
garyschulte added a commit to garyschulte/besu that referenced this pull request Apr 10, 2023
@gfukushima @garyschulte @siladu
Fix empty body concept after shanghai (hyperledger#5174)
591662d 
* Fix empty body concept after shanghai

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Check protocolSchedule to create empty block and add withdrawals to empty block check

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Add unit tests

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Extract block creation into a private method

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Nit changes

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Refactor test names
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

---------

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
Co-authored-by: garyschulte <garyschulte@gmail.com>
Co-authored-by: Simon Dudley <simon.dudley@consensys.net>
elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023
@gfukushima @garyschulte
@siladu @elenduuche
Fix empty body concept after shanghai (hyperledger#5174)
b26b6ec 
* Fix empty body concept after shanghai

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Check protocolSchedule to create empty block and add withdrawals to empty block check

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Add unit tests

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Extract block creation into a private method

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Nit changes

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Refactor test names
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

---------

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
Co-authored-by: garyschulte <garyschulte@gmail.com>
Co-authored-by: Simon Dudley <simon.dudley@consensys.net>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
@gfukushima @garyschulte
@siladu @eum602
Fix empty body concept after shanghai (hyperledger#5174)
c8aaa61 
* Fix empty body concept after shanghai

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Check protocolSchedule to create empty block and add withdrawals to empty block check

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Add unit tests

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Extract block creation into a private method

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Nit changes

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Refactor test names
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

---------

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
Co-authored-by: garyschulte <garyschulte@gmail.com>
Co-authored-by: Simon Dudley <simon.dudley@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@siladu siladu siladu left review comments

@jframe jframe jframe approved these changes

@macfarla macfarla macfarla approved these changes

@garyschulte garyschulte Awaiting requested review from garyschulte

Assignees

No one assigned

Labels

bug Something isn't working mainnet TeamGroot GH issues worked on by Groot Team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Withdrawals to handle filling empty blocks when syncing

5 participants

@gfukushima @jframe @garyschulte @macfarla @siladu