-
-
Notifications
You must be signed in to change notification settings - Fork 479
feat: report parent_block_hash for safe/finalized post-Gloas #9393
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
Changes from all commits
48c6ced
2437c3c
00dbe3b
eb3cbca
b8a35c6
ce6d9bb
ad1d81f
4bb2e4b
fd431ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import {ZERO_HASH_HEX} from "@lodestar/params"; | ||
| import {GENESIS_SLOT, ZERO_HASH_HEX} from "@lodestar/params"; | ||
| import {Root, RootHex} from "@lodestar/types"; | ||
| import {fromHex} from "@lodestar/utils"; | ||
| import {LogLevel, Logger, fromHex} from "@lodestar/utils"; | ||
| import {HEX_ZERO_HASH, ProtoBlock, isGloasBlock} from "../protoArray/interface.js"; | ||
| import {IForkChoice} from "./interface.js"; | ||
|
|
||
| /** | ||
|
|
@@ -19,17 +20,56 @@ export function getSafeBeaconBlockRoot(fc: IForkChoice): Root { | |
| } | ||
|
|
||
| /** | ||
| * Get execution payload hash for the safe block | ||
| * Get execution payload hash to report as `safeBlockHash` in `engine_forkchoiceUpdated`. | ||
| * | ||
| * https://github.com/ethereum/consensus-specs/blob/master/fork_choice/safe-block.md#get_safe_execution_block_hash | ||
| * Pre-Gloas: the confirmed block's own payload hash. | ||
| * Post-Gloas: the confirmed block's bid `parent_block_hash` — under ePBS the block's own | ||
| * payload may not yet be confirmed canonical, so we report the parent EL block which has | ||
| * been (the bid commits to extending it). | ||
| * | ||
| * https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.13/specs/bellatrix/fast-confirmation.md#new-get_safe_execution_block_hash | ||
| */ | ||
| export function getSafeExecutionBlockHash(forkChoice: IForkChoice): RootHex { | ||
| const confirmedRoot = forkChoice.getConfirmedRoot(); | ||
| if (confirmedRoot) { | ||
| const confirmedBlock = forkChoice.getBlockHexDefaultStatus(confirmedRoot); | ||
| if (confirmedBlock?.executionPayloadBlockHash) { | ||
| return confirmedBlock.executionPayloadBlockHash; | ||
| } | ||
| export function getSafeExecutionBlockHash(forkChoice: IForkChoice, logger?: Pick<Logger, LogLevel.warn>): RootHex { | ||
| const confirmedBlock = forkChoice.getConfirmedBlock(); | ||
| if (confirmedBlock === null) { | ||
| logger?.warn("Confirmed block not found, using zero safe execution block hash", { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should throw error instead
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agree with this, had same thought when reviewing this pr, did another quick audit of code and |
||
| confirmedRoot: forkChoice.getConfirmedRoot(), | ||
| }); | ||
| return ZERO_HASH_HEX; | ||
| } | ||
| return ZERO_HASH_HEX; | ||
|
|
||
| return getExecutionBlockHash(confirmedBlock, logger); | ||
| } | ||
|
|
||
| /** | ||
| * Get execution payload hash to report as `finalizedBlockHash` in `engine_forkchoiceUpdated`. | ||
| * Mirrors `getSafeExecutionBlockHash`: post-Gloas returns the bid `parent_block_hash`. | ||
| * | ||
| * https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.13/specs/gloas/fork-choice.md#notify_forkchoice_updated | ||
| */ | ||
| export function getFinalizedExecutionBlockHash(forkChoice: IForkChoice, logger?: Pick<Logger, LogLevel.warn>): RootHex { | ||
| return getExecutionBlockHash(forkChoice.getFinalizedBlock(), logger); | ||
| } | ||
|
|
||
| function getExecutionBlockHash(block: ProtoBlock, logger?: Pick<Logger, LogLevel.warn>): RootHex { | ||
| if (isGloasBlock(block)) { | ||
| return block.parentBlockHash; | ||
| } | ||
|
|
||
| // The genesis block body carries a default execution payload, so it is not an execution block | ||
| // per the spec (`is_execution_block`); its proto-array payload hash comes from the anchor | ||
| // state header instead of a block body and must not be reported as safe. | ||
| if (block.slot === GENESIS_SLOT) { | ||
| return HEX_ZERO_HASH; | ||
| } | ||
|
|
||
| if (block.executionPayloadBlockHash === null) { | ||
| logger?.warn("Execution payload block hash not found, using zero hash", { | ||
| blockRoot: block.blockRoot, | ||
| slot: block.slot, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same to above, I think we should throw error instead
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also updated in #9794 |
||
| }); | ||
| return HEX_ZERO_HASH; | ||
| } | ||
|
|
||
| return block.executionPayloadBlockHash; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to log when confirmed block is the finalized block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added in #9794