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: reset to current header if chain is rewound (in dev mode) #27992

Merged
merged 8 commits into from
Aug 25, 2023

Conversation

jsvisa
Copy link
Contributor

@jsvisa jsvisa commented Aug 23, 2023

fix #27990

@jsvisa jsvisa requested a review from gballet as a code owner August 23, 2023 15:18
Copy link
Contributor

@jwasinger jwasinger left a comment

Choose a reason for hiding this comment

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

An edge case is if SetHead is invoked after the check you have added, and before the end of sealBlock. One change that needs to be made is adding a check that returns from sealBlock if the ForkChoiceStatus returned from ForkChoiceUpdatedV2 (on line 147) is engine.STATUS_SYNCING.

I'm looking to see if anything else needs to be done.

@jwasinger
Copy link
Contributor

We will also want to check if GetBlockByNumber returns nil at https://github.com/ethereum/go-ethereum/blob/master/eth/catalyst/simulated_beacon.go#L166

@jsvisa jsvisa force-pushed the eth-catalyst-reset branch from b9864b8 to dfd8a79 Compare August 24, 2023 01:13
@jsvisa jsvisa requested a review from jwasinger August 24, 2023 01:14
@jsvisa
Copy link
Contributor Author

jsvisa commented Aug 24, 2023

@jwasinger thanks for the advice, PTAL :)

@holiman holiman changed the title eth/catalyst: reset to current header if chain is rewinded eth/catalyst: reset to current header if chain is rewound Aug 24, 2023
@holiman holiman changed the title eth/catalyst: reset to current header if chain is rewound eth/catalyst: reset to current header if chain is rewound (in dev mode) Aug 24, 2023
eth/catalyst/simulated_beacon.go Outdated Show resolved Hide resolved
eth/catalyst/simulated_beacon.go Outdated Show resolved Hide resolved
jsvisa and others added 3 commits August 24, 2023 18:15
Signed-off-by: jsvisa <[email protected]>
@holiman holiman added this to the 1.13.0 milestone Aug 25, 2023
@holiman holiman merged commit cde462c into ethereum:master Aug 25, 2023
Comment on lines +148 to +149
finalizedHash := c.finalizedBlockHash(header.Number.Uint64())
c.setCurrentState(header.Hash(), *finalizedHash)
Copy link
Contributor

Choose a reason for hiding this comment

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

finalizedHash can be nil, should have nil check here.

Copy link
Contributor

@jwasinger jwasinger Oct 11, 2023

Choose a reason for hiding this comment

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

Yes, it could happen in theory but should be extremely unlikely: a setHead which rolls back to before the latest finalized block would have to occur between the execution of line 147 and 148.

devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
sduchesneau pushed a commit to streamingfast/go-ethereum that referenced this pull request Jan 4, 2024
…e) (ethereum#27992)

Signed-off-by: jsvisa <[email protected]>
Co-authored-by: Jared Wasinger <[email protected]>
(cherry picked from commit cde462c)
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.

Calling debug_setHead crashes geth with SIGSEGV in dev mode.
4 participants