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

eth/catalyst: fix canon behind CurrentBlock #24988

Conversation

MariusVanDerWijden
Copy link
Member

This fixes all hive tests Invalid Ancestor Chain Re-Org, Invalid StateRoot, Invalid P9', Reveal using newPayload (go-ethereum)
where we reveal using newPayload.
The issue is that the head block is marked as canonical but the current block is behind the head block.
I am not really confident in this as I suspect it might not fix the root cause (a block being marked canonical that is not really in our canonical chain).

Maybe @rjl493456442 could take a look at this

@@ -144,7 +144,7 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update beacon.ForkchoiceStateV1, pa
}
}

if rawdb.ReadCanonicalHash(api.eth.ChainDb(), block.NumberU64()) != update.HeadBlockHash {
if rawdb.ReadCanonicalHash(api.eth.ChainDb(), block.NumberU64()) != update.HeadBlockHash || block.NumberU64() > api.eth.BlockChain().CurrentBlock().NumberU64() {
Copy link
Member

Choose a reason for hiding this comment

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

The scenario is: the local canonical hash is matched with the block, but local chain head is lower than the block.

I am wondering why it can happen in the first place. Whenever reorg happens, all the canonical hash above the new head will be deleted.

@rjl493456442
Copy link
Member

Can you share the concrete test scenario description?

@MariusVanDerWijden
Copy link
Member Author

MariusVanDerWijden commented May 30, 2022

Invalid Ancestor Re-Org Tests Attempt to re-org to an unknown side chain which at some point contains an invalid payload. The side chain is constructed in parallel while the CL Mock builds the canonical chain, but changing the extraData to simply produce a different hash. At a given point, the side chain invalidates one of the payloads by modifying one of the payload fields. Once the side chain reaches a certain deviation height (N) from the commonAncestor, the CL switches to it by either of the following methods: a) newPayload each of the payloads in the side chain, and the invalid payload shall return INVALID

commonAncestor◄─▲── P1 ◄─ P2 ◄─ P3 ◄─ ... ◄─ Pn
		    │
             └── P1' ◄─ P2' ◄─ ... ◄─ INV_P ◄─ ... ◄─ Pn'
		          ```

@rjl493456442
Copy link
Member

I think the root cause is: after reorg, the newchain length is 1. The new head won't be processed in reorg function.

So here https://github.com/ethereum/go-ethereum/blob/master/core/blockchain.go#L2075 HeadBlock is not updated and no canonical hash marker get deleted

I will submit a PR to fix it

@holiman
Copy link
Contributor

holiman commented Jun 2, 2022

I will submit a PR to fix it

So, I think @rjl493456442 's PR #24996 was it?
Question then: is this PR still needed?

@MariusVanDerWijden
Copy link
Member Author

I don't thinks so, I'll wait for hive to run and close this PR then

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.

3 participants