Post Merge cleanup: remove latestValidAncestorDescendsFromTerminal by fab-10 · Pull Request #4703 · besu-eth/besu

fab-10 · 2022-11-18T10:21:50Z

Signed-off-by: Fabio Di Fabio fabio.difabio@consensys.net

PR description

Fixed Issue(s)

Documentation

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

Changelog

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

daniellehrner · 2022-11-18T10:46:43Z

AFAIK some devnets for Shanghai are still planned to start with PoW and transition to PoS. @gezero Should be able to confirm that. But if that is the case I think we still have to wait until we can remove it.

fab-10 · 2022-11-18T11:04:57Z

Indeed this was one of the thing that I was recalling, not sure if these testnets are a blocker for the cleanup, since their bootstrap is quite controlled and limited to a small number of cooperating nodes.

garyschulte

In general I agree we should remove these checks, since they are only there to ensure a correct merge transition. I'd like to see besu successfully work in a multi-client merge network with these removed.

Last thought - we will most likely fail/regress hive merge tests after this PR. Not sure if we care though

garyschulte · 2022-11-18T16:25:46Z

-    // TODO: post-merge cleanup
-    if (!mergeCoordinator.latestValidAncestorDescendsFromTerminal(newBlockHeader)) {
-      mergeCoordinator.addBadBlock(block);
-      return respondWithInvalid(
-          reqId,
-          blockParam,
-          Hash.ZERO,
-          INVALID,
-          newBlockHeader.getHash() + " did not descend from terminal block");
-    }


just a comment - this reflects the perspective that we do whatever the CL tells us to do and do not second guess it. I think doing this is consistent with that view

garyschulte · 2022-11-18T16:26:22Z

-    // TODO: post-merge cleanup, this should be unnecessary after merge
-    if (!mergeCoordinator.latestValidAncestorDescendsFromTerminal(newHead.get())) {
-      logForkchoiceUpdatedCall(INVALID, forkChoice);
-      return new JsonRpcSuccessResponse(
-          requestId,
-          new EngineUpdateForkchoiceResult(
-              INVALID,
-              Hash.ZERO,
-              null,
-              Optional.of(newHead.get() + " did not descend from terminal block")));
-    }


same comment as on newPayload regarding second guessing the CL.

siladu · 2022-11-21T00:27:59Z

AFAIK some devnets for Shanghai are still planned to start with PoW and transition to PoS. @gezero Should be able to confirm that. But if that is the case I think we still have to wait until we can remove it.

~~This is true, at least teku and prysm devnets require the merge to happen~~

Update
Teku has had to implement a workaround for this to be able to interop with geth, not sure if it's permanent though.
Prysm still requires it.

siladu · 2022-12-05T04:00:14Z

Update Teku has had to implement a workaround for this to be able to interop with geth, not sure if it's permanent though. Prysm still requires it.

Another update. The goal for the next withdrawals testnet is to have all clients starting post-merge, so that blocker will hopefully go away soon.
I hear Prysm should have support soon.
~~Besu + Teku still seems to require running through the merge, so that requires investigation.~~ This was a config issue: teku's genesis needs to know about besu's genesis block hash

ajsutton · 2022-12-05T04:06:04Z

For the record in https://github.com/ConsenSys/teku/blob/a49f9a8a279a2260c57308fbb313162dfa7e2127/acceptance-tests/src/acceptance-test/java/tech/pegasys/teku/test/acceptance/MergedGenesisAcceptanceTest.java#L28 we have an AT that starts a Teku+Besu node with an already merged genesis state.

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> # Conflicts: # consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java # consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeMiningCoordinator.java # consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java # ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdated.java # ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayload.java # ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java # ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayloadTest.java

# Conflicts: # consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

# Conflicts: # ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineForkchoiceUpdatedTest.java # ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayloadTest.java # ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedV1Test.java # ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV1Test.java

jflo

buh bye TTD!

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

…eanup

…esu-eth#4703) * Post Merge cleanup: remove latestValidAncestorDescendsFromTerminal Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> * fixed tests Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> --------- Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>

…esu-eth#4703) * Post Merge cleanup: remove latestValidAncestorDescendsFromTerminal Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> * fixed tests Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> --------- Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: garyschulte <garyschulte@gmail.com>

…esu-eth#4703) * Post Merge cleanup: remove latestValidAncestorDescendsFromTerminal Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> * fixed tests Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> --------- Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>

garyschulte reviewed Nov 18, 2022

View reviewed changes

wcgcyx mentioned this pull request Nov 22, 2022

Add chain data pruning experimental feature #4686

Merged

2 tasks

gfukushima mentioned this pull request Nov 25, 2022

Post Merge checkpoint sync #4735

Closed

2 tasks

fab-10 force-pushed the post-merge-cleanup branch from d6fe16c to 59e9f79 Compare July 12, 2023 16:36

fab-10 requested a review from garyschulte July 12, 2023 16:39

fab-10 force-pushed the post-merge-cleanup branch from 59e9f79 to 885d3a3 Compare July 12, 2023 16:46

fab-10 force-pushed the post-merge-cleanup branch from 885d3a3 to a51515d Compare July 12, 2023 16:55

fab-10 added 2 commits July 20, 2023 17:24

Merge branch 'main' into post-merge-cleanup

6961a85

# Conflicts: # consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java

Fix javadoc

0b46e19

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

fab-10 force-pushed the post-merge-cleanup branch from 9e688bc to 0b46e19 Compare July 20, 2023 15:42

fab-10 marked this pull request as ready for review July 20, 2023 15:45

jflo approved these changes Aug 23, 2023

View reviewed changes

macfarla added 2 commits August 24, 2023 10:36

fixed tests

e047430

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

Merge branch 'main' of github.com:hyperledger/besu into post-merge-cl…

9e7f174

…eanup

macfarla enabled auto-merge (squash) August 24, 2023 00:40

macfarla merged commit 6bd3135 into besu-eth:main Aug 24, 2023

fab-10 deleted the post-merge-cleanup branch August 24, 2023 08:32

gfukushima mentioned this pull request Feb 23, 2024

Remove unnecessary PoS checkpoint methods and variables #6603

Merged

11 tasks

gfukushima mentioned this pull request Mar 6, 2024

clean-up: Remove isChainPruneEnabled from MergeContext interface #6672

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Post Merge cleanup: remove latestValidAncestorDescendsFromTerminal#4703

Post Merge cleanup: remove latestValidAncestorDescendsFromTerminal#4703
macfarla merged 6 commits into
besu-eth:mainfrom
fab-10:post-merge-cleanup

fab-10 commented Nov 18, 2022 •

edited

Loading

Uh oh!

daniellehrner commented Nov 18, 2022

Uh oh!

fab-10 commented Nov 18, 2022

Uh oh!

garyschulte left a comment •

edited

Loading

Uh oh!

Uh oh!

garyschulte Nov 18, 2022

Uh oh!

garyschulte Nov 18, 2022

Uh oh!

siladu commented Nov 21, 2022 •

edited

Loading

Uh oh!

siladu commented Dec 5, 2022 •

edited

Loading

Uh oh!

ajsutton commented Dec 5, 2022

Uh oh!

jflo left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

fab-10 commented Nov 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR description

Fixed Issue(s)

Documentation

Changelog

Uh oh!

daniellehrner commented Nov 18, 2022

Uh oh!

fab-10 commented Nov 18, 2022

Uh oh!

garyschulte left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

garyschulte Nov 18, 2022

Choose a reason for hiding this comment

Uh oh!

garyschulte Nov 18, 2022

Choose a reason for hiding this comment

Uh oh!

siladu commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

siladu commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajsutton commented Dec 5, 2022

Uh oh!

jflo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fab-10 commented Nov 18, 2022 •

edited

Loading

garyschulte left a comment •

edited

Loading

siladu commented Nov 21, 2022 •

edited

Loading

siladu commented Dec 5, 2022 •

edited

Loading