Replace getByBlockNumber by getByBlockHeader #5127

gfukushima · 2023-02-23T02:14:18Z

PR description

This PR brings the changes introduced in #5020 + the amend to the issue spotted on #5064.
Replacing getByBlockNumber by getByBlockHeader brought issue to sync because TransitionProtocolSchedule getByBlockHeader does not have the same behaviour as getByBlockNumber.
This PR Introduced a private TransitionProtocolSchedule getByBlockHeaderForBws to restore the original behaviour of getByBlockHeader.
getByBlockHeader follows the same approach getByBlockNumber used to have which uses the dispatchFunctionAccordingToMergeState to return the pre or post merge protocolSchedule.

Syncing test done:

- Checkpoint Sepolia
- Sepolia Shanghai Upgrade
- Checkpoint Goerli
- Checkpoint Mainnet

Fixed Issue(s)

Part of #4789

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

I thought about the changelog and included a changelog update if required.

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

Change default getByBlockHeader behaviour to use dispatchFunctionAccordingToMergeState Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

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

siladu · 2023-02-27T03:28:15Z

...sus/merge/src/main/java/org/hyperledger/besu/consensus/merge/TransitionProtocolSchedule.java

+   * @param blockHeader the block header
+   * @return the ProtocolSpec to be used by the provided block
+   */
+  public ProtocolSpec getByBlockHeaderForBws(final ProcessableBlockHeader blockHeader) {


Not sure what a better name would be but I don't think it should encode the caller since this might change. The difference is that it calls getByBlockHeaderFromTransitionUtils but that name doesn't really capture the difference in behaviour either.

Think I would need to understand why backwards sync needs this different behaviour and whether or not it's relevant for other code paths that call getByBlockHeader...maybe a question for @gezero?

After reading around a bit, wonder if getByBlockHeaderAndHandleReorgBelowTTD makes more sense.

This appears to be the PR that introduced the extra logic: #3696
Seems super-specific and edge-casey...wonder if syncing goerli always hits this edge case though 🤔

Why was using this causing the Goerli sync to fail?

siladu · 2023-02-27T04:06:46Z

...merge/src/test/java/org/hyperledger/besu/consensus/merge/TransitionProtocolScheduleTest.java

@@ -159,52 +155,45 @@ public void getByBlockHeader_returnsTimestampScheduleIfPresent() {
  }

  @Test
-  public void getByBlockNumber_returnsTimestampScheduleIfPresent() {


Why are the getByBlockNumber tests being removed and migrated to cover getByBlockHeaderForBws?

This PR isn't removing getByBlockNumber so I think the tests should remain, we may need to duplicate some for the new method.

Added back getByBlockNumber tests which are almost duplicates of the getByBlockHeader tests since that method still exists and is being used.

siladu · 2023-02-27T04:08:54Z

...merge/src/test/java/org/hyperledger/besu/consensus/merge/TransitionProtocolScheduleTest.java


    verifyPostMergeProtocolScheduleReturned();
  }

  @Test
-  public void getByBlockNumber_delegatesToPostMergeScheduleWhenTimestampScheduleDoesNotExist() {
+  public void getByBlockHeader_delegatesToPostMergeScheduleWhenTimestampScheduleDoesNotExist() {


Test names should be updated from getByBlockHeader_... -> getByBlockHeaderForBws_...

Changed these tests to use getByBlockHeader so we have test coverage for that method

siladu · 2023-02-27T04:10:01Z

...merge/src/test/java/org/hyperledger/besu/consensus/merge/TransitionProtocolScheduleTest.java


    verifyPreMergeProtocolScheduleReturned();
  }

  @Test
-  public void getByBlockNumber_delegatesToPostMergeScheduleWhenBlockNotFound() {
-    when(blockchain.getBlockHeader(BLOCK_NUMBER)).thenReturn(Optional.empty());
+  public void getByBlockHeader_delegatesToPostMergeScheduleWhenBlockNotFound() {


Not shown in the diff, but I think getByBlockHeader() is now only being covered by one unit test, for when the timestamp schedule is found. It needs at least the other branch of logic tested, which is similar to this unit test.

Changed the getByBlockHeader tests to use getByBlockHeader instead of getByBlockHeaderForBws. The other tests cover the backward sync conditions.

siladu · 2023-02-27T06:32:48Z

I think there's still some cases where getByBlockNumber has not been migrated across (even though the header is available) so would like to understand those a bit more and why they weren't included in this PR (if intentional).

besu/src/test/java/org/hyperledger/besu/controller/TransitionControllerBuilderTest.java

...h/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContext.java

jframe · 2023-03-01T04:02:08Z

...sus/merge/src/main/java/org/hyperledger/besu/consensus/merge/TransitionProtocolSchedule.java

+   * @param blockHeader the block header
+   * @return the ProtocolSpec to be used by the provided block
+   */
+  public ProtocolSpec getByBlockHeaderForBws(final ProcessableBlockHeader blockHeader) {


Why was using this causing the Goerli sync to fail?

…d use getByBlockHeader for blockHeader tests instead of getByBlockHeaderForBws Signed-off-by: Jason Frame <jason.frame@consensys.net>

…umber-by-getByBlockHeader

Signed-off-by: Jason Frame <jason.frame@consensys.net>

…andling Signed-off-by: Jason Frame <jason.frame@consensys.net>

siladu · 2023-03-02T05:55:55Z

...merge/src/test/java/org/hyperledger/besu/consensus/merge/TransitionProtocolScheduleTest.java

+  }
+
+  @Test
+  public void getByBlockHeader_delegatesToPostMergeScheduleWhenBlockNotFound() {


How is this distinguished from the test below, getByBlockHeader_delegatesToPostMergeScheduleWhenTimestampScheduleDoesNotExist?

Before, it was testing 'block not found' with this...
when(blockchain.getBlockHeader(BLOCK_NUMBER)).thenReturn(Optional.empty());

Fixed the tests for the block not found cases for getByBlockNumber. And have removed tests for getByBlockHeader for the block not found cases. For getByBlockHeader we don't need to do a blockchain.getBlockHeader() call so we can never get the case where the block or block header doesn't exist.

Signed-off-by: Jason Frame <jason.frame@consensys.net>

siladu

LGTM

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com> Signed-off-by: Jason Frame <jason.frame@consensys.net>

gfukushima and others added 24 commits January 25, 2023 17:39

Replace getByBlockNumber by getByBlockHeader where trivial

9181f3b

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

Fix tests

d0d8c3e

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

Fix tests

47794d8

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

Rolledback change since tests will need non trivial changes

508885a

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

Rollback replace

01bac49

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

Merge branch 'main' into replace-getByBlockNumber-by-getByBlockHeader

e74025c

Change other 2 trivial tests

eca7596

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

spotless

8d9c0d8

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

Pass ProcessableBlockHeader to the mocks

b5559b9

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

Replace getByBlockNumber by getByBlockHeader

0239537

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

Merge branch 'main' into replace-getByBlockNumber-by-getByBlockHeader

d223aae

Add mock of blockHeader

be677b1

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

Use any() for the tests

7cfd093

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

Merge branch 'main' into replace-getByBlockNumber-by-getByBlockHeader

479e51a

Add unit test for headBlockHeader()

4bba346

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

Merge branch 'main' into replace-getByBlockNumber-by-getByBlockHeader

be34ecd

Merge branch 'main' into replace-getByBlockNumber-by-getByBlockHeader

87d1633

Merge branch 'main' into replace-getByBlockNumber-by-getByBlockHeader

7f823d6

Merge branch 'main' into replace-getByBlockNumber-by-getByBlockHeader

75b5ec9

Merge branch 'main' into replace-getByBlockNumber-by-getByBlockHeader

0e418b5

Add getByBlockHeaderForBws which handles re-org TTD blocks

44e94d4

Change default getByBlockHeader behaviour to use dispatchFunctionAccordingToMergeState Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

Fix tests to use getByBlockHeaderForBws

05b874e

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

Fix javadoc

492e6f5

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

Merge branch 'main' into replace-getByBlockNumber-by-getByBlockHeader

b736665

gfukushima added TeamGroot GH issues worked on by Groot Team mainnet labels Feb 23, 2023

siladu marked this pull request as ready for review February 27, 2023 01:13

siladu reviewed Feb 27, 2023

View reviewed changes

jframe reviewed Mar 1, 2023

View reviewed changes

jframe added 4 commits March 2, 2023 14:13

Add back getByBlockNumber tests for TransitionProtocolScheduleTest an…

06dc593

…d use getByBlockHeader for blockHeader tests instead of getByBlockHeaderForBws Signed-off-by: Jason Frame <jason.frame@consensys.net>

Merge remote-tracking branch 'upstream/main' into replace-getByBlockN…

7f26167

…umber-by-getByBlockHeader

Spotless

42dd671

Signed-off-by: Jason Frame <jason.frame@consensys.net>

rename getByBlockHeaderForBws to getByBlockHeaderWithTransitionReorgH…

dae715a

…andling Signed-off-by: Jason Frame <jason.frame@consensys.net>

siladu reviewed Mar 2, 2023

View reviewed changes

Fix TransitionProtocolScheduleTest for block not found cases

22052c6

Signed-off-by: Jason Frame <jason.frame@consensys.net>

siladu approved these changes Mar 3, 2023

View reviewed changes

Merge branch 'main' into replace-getByBlockNumber-by-getByBlockHeader

6311aa1

jframe enabled auto-merge (squash) March 3, 2023 05:02

jframe merged commit bf01981 into hyperledger:main Mar 3, 2023

gfukushima deleted the replace-getByBlockNumber-by-getByBlockHeader branch March 23, 2023 22:59

elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023

Replace getByBlockNumber by getByBlockHeader (hyperledger#5127)

f1a0dce

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com> Signed-off-by: Jason Frame <jason.frame@consensys.net>

eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023

Replace getByBlockNumber by getByBlockHeader (hyperledger#5127)

d807be6

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com> Signed-off-by: Jason Frame <jason.frame@consensys.net>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Replace getByBlockNumber by getByBlockHeader #5127

Replace getByBlockNumber by getByBlockHeader #5127

Uh oh!

gfukushima commented Feb 23, 2023 •

edited

Loading

Uh oh!

siladu Feb 27, 2023

Uh oh!

siladu Feb 27, 2023

Uh oh!

jframe Mar 1, 2023

Uh oh!

siladu Feb 27, 2023

Uh oh!

jframe Mar 2, 2023

Uh oh!

siladu Feb 27, 2023

Uh oh!

jframe Mar 2, 2023

Uh oh!

siladu Feb 27, 2023

Uh oh!

jframe Mar 2, 2023

Uh oh!

siladu commented Feb 27, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

jframe Mar 1, 2023

Uh oh!

siladu Mar 2, 2023

Uh oh!

jframe Mar 3, 2023

Uh oh!

siladu left a comment

Uh oh!

Uh oh!

Replace getByBlockNumber by getByBlockHeader #5127

Replace getByBlockNumber by getByBlockHeader #5127

Uh oh!

Conversation

gfukushima commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR description

Fixed Issue(s)

Documentation

Acceptance Tests (Non Mainnet)

Changelog

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siladu commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siladu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gfukushima commented Feb 23, 2023 •

edited

Loading

siladu commented Feb 27, 2023 •

edited

Loading