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 finalized block hash on ForkchoiceUpdated #1803

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 finalized 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#L154-L167

Also, this is bullet number 6 from fork choice update specs

@cbrzn cbrzn changed the title Engine: Validate finalized block hash on ForkchoiceUpdated Client: Validate finalized block hash on ForkchoiceUpdated Mar 21, 2022
@codecov
Copy link

codecov bot commented Mar 21, 2022

Codecov Report

Merging #1803 (7c7f94d) into master (3ae51b7) will increase coverage by 16.46%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
block 85.57% <ø> (ø)
blockchain 83.82% <ø> (ø)
client 75.87% <100.00%> (+57.64%) ⬆️
common 94.19% <ø> (ø)
devp2p 82.56% <ø> (ø)
ethash 90.76% <ø> (ø)
trie 80.02% <ø> (ø)
tx 88.20% <ø> (ø)
util 92.62% <ø> (ø)
vm 81.22% <ø> (ø)

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

ryanio
ryanio previously approved these changes Mar 21, 2022
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 :)

@cbrzn cbrzn force-pushed the engine/validate-finalized-block-hash branch from afb0a10 to 7c7f94d Compare March 22, 2022 19:33
@cbrzn
Copy link
Contributor Author

cbrzn commented Mar 22, 2022

conflicts fixed :)

@cbrzn cbrzn closed this Mar 22, 2022
@cbrzn cbrzn reopened this Mar 22, 2022
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, nice, thanks!

@ryanio ryanio merged commit fe45ddc into master Mar 22, 2022
@ryanio ryanio deleted the engine/validate-finalized-block-hash branch March 22, 2022 23:16
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.

2 participants