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

Client: Validate block safe hash on ForkchoiceUpdated #1804

Merged
merged 2 commits into from
Mar 22, 2022

Conversation

cbrzn
Copy link
Contributor

@cbrzn cbrzn commented Mar 21, 2022

This PR aims to validate that the safe head block exists in the chain. Fixing the test case from the Hive: Unknown SafeBlockHash

Implementation inspired from geth: https://github.com/ethereum/go-ethereum/blob/master/eth/catalyst/api.go#L168-L179

Also, this is the validation mentioned on Forkchoice state specs

edit: this PR will have conflicts with #1803 - I will take care of that once one of these two PRs got merged

@codecov
Copy link

codecov bot commented Mar 21, 2022

Codecov Report

Merging #1804 (867ed62) into master (fe71b04) will decrease coverage by 16.42%.
The diff coverage is 80.00%.

Impacted file tree graph

Flag Coverage Δ
block 85.57% <ø> (ø)
blockchain 83.82% <ø> (ø)
client 18.24% <80.00%> (-57.47%) ⬇️
common 94.19% <ø> (ø)
devp2p 82.69% <ø> (ø)
ethash 90.76% <ø> (ø)
trie 80.02% <ø> (ø)
tx 88.20% <ø> (ø)
util 92.62% <ø> (ø)
vm 81.23% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for adding :)

@holgerd77 holgerd77 force-pushed the engine/validate-safe-head branch from e2c8f7e to 867ed62 Compare March 22, 2022 08:02
@holgerd77 holgerd77 merged commit fa286c3 into master Mar 22, 2022
@holgerd77 holgerd77 deleted the engine/validate-safe-head branch March 22, 2022 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants