fix: use fork choice for parent payload status in block production#9209
fix: use fork choice for parent payload status in block production#9209
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the block production logic to determine if a parent block is full by querying the fork choice instead of using a state-transition utility. This change involves passing the forkChoice object through the prepareExecutionPayload and preparePayloadAttributes functions. A review comment suggests optimizing the check for a full parent block by moving the lookup inside the conditional statement to leverage short-circuit evaluation, ensuring the lookup only occurs for post-Gloas states.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e578ab475
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Performance Report✔️ no performance regression detected Full benchmark results
|
|
converted to draft, let's wait for the spec to be finalized |
|
@twoeths thanks for reviewing, keep as draft, still debating how to define this on the spec side |
twoeths
left a comment
twoeths
left a comment
There was a problem hiding this comment.
just want to point out that the call of prepareExecutionPayload in prepareNextSlot will not work in PROD because payload is available at the time, so chain.forkChoice.shouldExtendPayload will always false
did you mean not available? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae611fa3d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
I think twoeths' statement is correct as written ("payload is available"), but the conclusion is inverted. Tracing the
This means The gap is: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #9209 +/- ##
============================================
- Coverage 52.53% 52.53% -0.01%
============================================
Files 848 848
Lines 61408 61402 -6
Branches 4525 4525
============================================
- Hits 32262 32256 -6
Misses 29081 29081
Partials 65 65 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52866ebbff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isEpochTransition) { | ||
| this.metrics?.precomputeNextEpochTransition.count.inc({result: "success"}, 1); | ||
| const previousHits = this.chain.regen.updatePreComputedCheckpoint(headRoot, nextEpoch); | ||
| const previousHits = this.chain.regen.updatePreComputedCheckpoint(updatedHeadRoot, nextEpoch); |
There was a problem hiding this comment.
this seems like a bug we have on unstable, I don't see a reason why we would pre-compute for the head root which we predict to be re-orged in certain cases due to proposer boost reorg
There was a problem hiding this comment.
Yes, agreed — this looks like a latent bug on unstable, not a gloas-specific regression.
History check: the updatePreComputedCheckpoint(headRoot, nextEpoch) call was introduced in 5fe87f8e44 (#6924 fix: prepareNextEpoch metric) after the proposer-boost-reorg logic had already been added in f6d3bce802 (#6652). At that point an inner updatedHeadRoot already existed inside the if (feeRecipient) block, but the new call used the outer headRoot. That reads like an oversight rather than an intentional choice — there's no load-bearing reason I can find for pre-computing the checkpoint on the root we ourselves predict will be reorged away; the next block is going to build on updatedHeadRoot, so that's also the root whose next-epoch state the rest of the system will actually request.
The fix here is also strictly safe:
- When no proposer-boost-reorg is predicted,
updatedHeadRoot === headRoot(it's initialized toheadRootat the top of the loop and only overwritten on a predicted reorg), so behavior is unchanged. - When a proposer-boost-reorg is predicted and happens, we now precompute the right checkpoint instead of wasting it.
- When a proposer-boost-reorg is predicted but doesn't happen, we lose the precompute — but we would have lost it anyway under the old code in any case where the real head ends up somewhere other than
headRoot, so net no worse.
Happy either way on scope: keep it bundled here (it's a 2-line change and thematically adjacent to the parent-payload-status work) or split into a tiny standalone fix against unstable if you'd rather have it land independently of the gloas changes. Marginal preference for splitting, but no strong view.
There was a problem hiding this comment.
the proposer boost reorg will not happen at epoch boundary
so headerRoot == updatedHeadRoot in that case
leaving updatedHeaderRoot is misleading
I prefer leaving headerRoot with this comment for later reference
There was a problem hiding this comment.
You're right, and my earlier reply was wrong on this — I missed the epoch-boundary suppression inside predictProposerHead.
Traced it: predictProposerHead → shouldOverrideForkChoiceUpdate → getPreliminaryProposerHead, which at forkChoice.ts:1852-1857 short-circuits with NotReorgedReason.NotShufflingStable whenever slot % SLOTS_PER_EPOCH === 0:
// No reorg if we are at epoch boundary where proposer shuffling could change
const isShufflingStable = slot % SLOTS_PER_EPOCH !== 0;
if (!isShufflingStable) {
return {prelimProposerHead, prelimNotReorgedReason: NotReorgedReason.NotShufflingStable};
}So predictProposerHead() returns the original headBlock at epoch boundaries → updatedHeadRoot === headRoot at the point updatePreComputedCheckpoint runs (since that branch is gated on isEpochTransition). The change in ddf56e2d76 is functionally a no-op, and using updatedHeadRoot there reads as if the two could differ when they can't.
+1 on reverting that one line back to headRoot and keeping @nflaig's "this seems like a bug we have on unstable…" note as a doc-only comment for future reference. Sorry for the noisy earlier confirmation.
| prepareState: updatedPrepareState, | ||
| prepareSlot, | ||
| parentBlockRoot: fromHex(headRoot), | ||
| parentBlockRoot: fromHex(updatedHeadRoot), |
There was a problem hiding this comment.
I believe this was wrong previously @twoeths, can you double check before we merge just to be sure
There was a problem hiding this comment.
Traced this too — I do think this one is a real latent bug on unstable, not a cosmetic tweak, and it's a different story from the updatePreComputedCheckpoint line above:
On unstable the SSE block currently does:
const data = getPayloadAttributesForSSE(fork as ForkPostBellatrix, this.chain, {
prepareState: updatedPrepareState, // may be the reorged-parent state
prepareSlot,
parentBlockRoot: fromHex(headRoot), // original head
...
});When a proposer-boost-reorg is predicted and we take the branch that re-regens updatedPrepareState from the parent of the weak head, the state being emitted is built on updatedHeadRoot, but the event reports parentBlockRoot = headRoot. So the SSE {prepareState, parentBlockRoot} pair is internally inconsistent for a proposer-boost-reorg prediction.
For symmetry, the same-block prepareExecutionPayload(...) call in the feeRecipient branch already uses fromHex(updatedHeadRoot) as its parentBeaconBlockRoot, so that path is self-consistent — only the SSE emit diverged.
And unlike the updatePreComputedCheckpoint path, this block is not gated on isEpochTransition, so predictProposerHead can genuinely return a different root here (the shuffling-stable short-circuit at forkChoice.ts:1852 only kicks in at epoch boundary). So updatedHeadRoot !== headRoot is a reachable case here.
Change to parentBlockRoot: fromHex(updatedHeadRoot) aligns the SSE payload with both prepareState and the prepareExecutionPayload parent root, and is strictly safe when no reorg is predicted because updatedHeadRoot still initializes to headRoot. Reads correct to me pending @twoeths's double-check.
There was a problem hiding this comment.
I'm not so sure about this one
why do we have to emit routes.events.EventType.payloadAttributes for every slot? the value we emit may not be the correct one
I think we should only do it if we're the next proposer, ie do it inside if (feeRecipient) {
and use the value of the head root we're supposed to propose
There was a problem hiding this comment.
why do we have to emit routes.events.EventType.payloadAttributes for every slot?
yes for builders
There was a problem hiding this comment.
+1 to @nflaig on keeping the per-slot emit. The payload_attributes topic on /eth/v1/events is specified as fired on every head-advance / slot transition precisely so external block builders (MEV relays, local builder clients) can observe attributes they'll be asked to build against, regardless of whether this node is the next proposer. Restricting to if (feeRecipient) would make the event fire only on the subset of slots we happen to know we're proposing, which wouldn't serve the relay use case.
On the value we emit in the non-proposer case: updatedHeadRoot === headRoot there (we only rebind updatedHeadRoot inside the if (feeRecipient) / proposer-boost-reorg branch), and updatedPrepareState === prepareState, so the emit degenerates to "here's the current head's payload attributes" — which is exactly what builders want in that case.
The parentBlockRoot → updatedHeadRoot change here is orthogonal to the larger emit-gating question: it only aligns parentBlockRoot with the prepareState the SSE already emits (and with the parentBeaconBlockRoot the sibling prepareExecutionPayload(..., fromHex(updatedHeadRoot), ...) call uses). Without this change, when we are the proposer and a proposer-boost-reorg is predicted, the SSE event pairs prepareState: updatedPrepareState (regened from the reorged-to parent) with parentBlockRoot: headRoot (the weak head), which is an internal inconsistency a relay would struggle to make sense of.
So my read is the same as before: keep this PR's one-line change on the SSE emit, leave the restructure-or-keep-emitting-every-slot decision out of scope.
Per bot review feedback on PR: removing the `onExecutionPayload(anchor, ...)` call alongside the PTC vote override broke block production after restart from a post-Gloas anchor. `shouldExtendPayload(anchorRoot)` (used in ChainSafe#9209) short- circuits to false via `!hasPayload()`, so `produceBlockBody`/`prepareNextSlot` pick `latestExecutionPayloadBid.parentBlockHash` instead of `.blockHash`, producing bids that fail `processExecutionPayloadBid`'s parent-hash check. Upstream consensus-specs `c7a0a8527` removes only the PTC vote seeding (`payload_timeliness_vote={}`, `payload_data_availability_vote={}`) — it does not remove the FULL-payload semantics of the anchor. The anchor state's `latestBlockHash` is the executed payload hash, so `onExecutionPayload(anchor)` still matches the spec's intent of `payload_states = {anchor_root: ...}`. Keep the PTC vote override removal (the actual subject of this PR); restore the FULL-variant seeding with a focused comment. 🤖 Generated with AI assistance
3 participants
related to changes in ethereum/consensus-specs#5094, we wanna avoid using
is_parent_block_full