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

Post Merge cleanup: remove latestValidAncestorDescendsFromTerminal #4703

Merged
merged 6 commits into from
Aug 24, 2023

Conversation

fab-10
Copy link
Contributor

@fab-10 fab-10 commented Nov 18, 2022

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

@daniellehrner
Copy link
Contributor

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
Copy link
Contributor Author

fab-10 commented Nov 18, 2022

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.

Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 204 to 213
// 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");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Comment on lines 118 to 128
// 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")));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as on newPayload regarding second guessing the CL.

@siladu
Copy link
Contributor

siladu commented Nov 21, 2022

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
Copy link
Contributor

siladu commented Dec 5, 2022

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
Copy link
Contributor

ajsutton commented Dec 5, 2022

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.

@fab-10 fab-10 force-pushed the post-merge-cleanup branch from d6fe16c to 59e9f79 Compare July 12, 2023 16:36
@fab-10 fab-10 requested a review from garyschulte July 12, 2023 16:39
@fab-10 fab-10 force-pushed the post-merge-cleanup branch from 59e9f79 to 885d3a3 Compare July 12, 2023 16:46
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
@fab-10 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
# Conflicts:
#	consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10 fab-10 force-pushed the post-merge-cleanup branch from 9e688bc to 0b46e19 Compare July 20, 2023 15:42
@fab-10 fab-10 marked this pull request as ready for review July 20, 2023 15:45
# 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
Copy link
Contributor

@jflo jflo left a comment

Choose a reason for hiding this comment

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

buh bye TTD!

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla macfarla enabled auto-merge (squash) August 24, 2023 00:40
@macfarla macfarla merged commit 6bd3135 into hyperledger:main Aug 24, 2023
@fab-10 fab-10 deleted the post-merge-cleanup branch August 24, 2023 08:32
garyschulte pushed a commit to garyschulte/besu that referenced this pull request Aug 28, 2023
…yperledger#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 pushed a commit to garyschulte/besu that referenced this pull request Aug 28, 2023
…yperledger#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 pushed a commit to garyschulte/besu that referenced this pull request Aug 28, 2023
…yperledger#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>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
…yperledger#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>
NickSneo pushed a commit to NickSneo/besu that referenced this pull request Nov 12, 2023
…yperledger#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>
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.

7 participants