fix: gate builder bid lookup on shouldBuildOnFull in produceBlockV4 - #9442
Merged
Conversation
When the PTC majority votes the parent's payload was unavailable or untimely, shouldBuildOnFull returns false and the self-build path in produceBlockBody correctly reorgs to EMPTY. The builder bid path bypassed this check: produceBlockV4 unconditionally queried the bid pool with parentBlock.executionPayloadBlockHash (the FULL hash when FULL remains canonical head by weight), then preferred any fulfilled builder result over the engine result. Net effect: the validator would publish a builder block extending the late/unavailable FULL payload instead of executing the spec-mandated reorg. Compute shouldBuildOnFull before the pool lookup and, when reorging, query the pool with the EMPTY variant's executionPayloadBlockHash (grandparent EL hash) so only EMPTY-reorg-targeting bids can match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the validator API to conditionally select the parent block hash for builder bids based on whether the node is building on a full block. However, querying the fork choice for the empty variant of the parent block is likely to return null during block production. It is recommended to directly use the parent block's parent block hash instead, which is more efficient and avoids this potential bug.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…ckHash Use parentBlock.parentBlockHash directly instead of looking up the EMPTY variant in the protoArray. For any variant of a gloas parent block, both fields hold the same value (the bid's parent_block_hash), because forkChoice.onBlock sets executionPayloadBlockHash = parentBlockHash = bid.parentBlockHash for the PENDING/EMPTY variants, and only the FULL variant overrides executionPayloadBlockHash to the delivered payload hash. Pre-gloas parents (at the fork transition) have parentBlockHash = null, which getBestBid already handles by returning null. The explicit null guard is therefore unnecessary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Performance Report🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
nflaig
approved these changes
Jun 1, 2026
nflaig
left a comment
nflaig
left a comment
Member
There was a problem hiding this comment.
lgtm, I am fine with merging that as is, functionally, it should achieve what we want
| parentBlock.executionPayloadBlockHash, | ||
| parentBlockRootHex | ||
| ); | ||
| const isBuildingOnFull = chain.forkChoice.shouldBuildOnFull(parentBlock, slot); |
Member
There was a problem hiding this comment.
have you looked into deduplicating the shouldBuildOnFull call? now we call it twice, this might require more refactoring though, but ideally we just wanna call it once
| parentBlockRootHex | ||
| ); | ||
| const isBuildingOnFull = chain.forkChoice.shouldBuildOnFull(parentBlock, slot); | ||
| const bidParentBlockHash = isBuildingOnFull ? parentBlock.executionPayloadBlockHash : parentBlock.parentBlockHash; |
Member
There was a problem hiding this comment.
I find our naming here confusing, why do we use parentBlockHash per spec, but also executionPayloadBlockHash instead of blockHash?
we can choose either, but would be good if we can be consistent, or is there a good reason for this?
Member
|
going to merge this, we can look into further refactoring separately |
Member
|
🎉 This PR is included in v1.44.0 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See discussion from https://github.com/ChainSafe/lodestar/pull/9431/changes#r3330285562