feat: report parent_block_hash for safe/finalized post-Gloas#9393
feat: report parent_block_hash for safe/finalized post-Gloas#9393wemeetagain wants to merge 1 commit into
Conversation
Implements consensus-specs PR-5197. Post-Gloas, engine_forkchoiceUpdated must report the bid's parent_block_hash as both safeBlockHash and finalizedBlockHash rather than the block's own execution payload hash: under ePBS the safe/finalized block's own payload may not yet be confirmed canonical, but its parent EL block has been. Adds getFinalizedExecutionBlockHash to mirror getSafeExecutionBlockHash and switches all five FCU callsites to the new fork-aware helpers.
There was a problem hiding this comment.
Code Review
This pull request introduces logic to correctly handle execution block hashes for the engine_forkchoiceUpdated call in preparation for the Gloas fork. It adds a new utility function getFinalizedExecutionBlockHash and refactors getSafeExecutionBlockHash to return the parent_block_hash for Gloas blocks, ensuring compliance with the updated consensus specifications. These utilities are integrated across the beacon node's block import and production flows, and new unit tests have been added to verify the logic for both pre-Gloas and post-Gloas scenarios. I have no feedback to provide.
Performance Report✔️ no performance regression detected Full benchmark results
|
| if (isGloasBlock(block)) { | ||
| return block.parentBlockHash as RootHex; | ||
| } | ||
| return block.executionPayloadBlockHash ?? HEX_ZERO_HASH; |
There was a problem hiding this comment.
I think doing ?? HEX_ZERO_HASH silences an issue in the event that something is wrong in our fork choice.
We probably need to add a log if we fall back to HEX_ZERO_HASH. I think prysm is having an issue like this https://discord.com/channels/595666850260713488/892088344438255616/1505929876136530021
|
|
||
| function getExecutionBlockHashForFCU(block: ProtoBlock): RootHex { | ||
| if (isGloasBlock(block)) { | ||
| return block.parentBlockHash as RootHex; |
There was a problem hiding this comment.
Don't really like this cast
Can we do export function isGloasBlock(block: ProtoBlock): block is ProtoBlock & {parentBlockHash: RootHex}?
| return getExecutionBlockHashForFCU(forkChoice.getFinalizedBlock()); | ||
| } | ||
|
|
||
| function getExecutionBlockHashForFCU(block: ProtoBlock): RootHex { |
There was a problem hiding this comment.
why is the "for fcu" part relevant?
Motivation
Description
Implements consensus-specs PR-5197. Post-Gloas, engine_forkchoiceUpdated must report the bid's parent_block_hash as both safeBlockHash and finalizedBlockHash rather than the block's own execution payload hash: under ePBS the safe/finalized block's own payload may not yet be confirmed canonical, but its parent EL block has been.
Adds getFinalizedExecutionBlockHash to mirror getSafeExecutionBlockHash and switches all five FCU callsites to the new fork-aware helpers.
AI Assistance Disclosure