fix: revert anchor block PTC vote override#9244
fix: revert anchor block PTC vote override#9244lodekeeper wants to merge 6 commits intoChainSafe:unstablefrom
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 removes the anchor block PTC vote override and the associated onExecutionPayload call in ProtoArray. While the removal of the PTC vote override aligns with the updated specification, the feedback indicates that removing the onExecutionPayload call for the anchor block introduces a regression. Specifically, without this call, the anchor block lacks a 'FULL' payload variant, which causes subsequent blocks extending it to be incorrectly treated as orphans. A code suggestion was provided to restore the necessary initialization for Gloas blocks.
| } | ||
| } | ||
|
|
||
| return protoArray; |
There was a problem hiding this comment.
While reverting the PTC vote override is correct per the updated spec (commit c7a0a8527), removing the onExecutionPayload call for the anchor block introduces a regression.
According to the Gloas spec for get_forkchoice_store, payload_states must be initialized with {anchor_root: PAYLOAD_STATUS_FULL}. In the context of ProtoArray, this means the anchor block must have a FULL variant. If it only has PENDING and EMPTY variants (the default from onBlock), any subsequent block that attempts to extend the anchor's FULL variant will fail to find its parent in getBlockHexAndBlockHash and will be treated as an orphan.
The onExecutionPayload call should be preserved to ensure the anchor block is correctly initialized with its payload state.
if (isGloasBlock(block as ProtoBlock) && block.executionPayloadBlockHash !== null) {
protoArray.onExecutionPayload(
block.blockRoot,
currentSlot,
block.executionPayloadBlockHash,
(block as {executionPayloadNumber?: number}).executionPayloadNumber ?? 0,
block.stateRoot,
null,
ExecutionStatus.Valid
);
}
return protoArray;There was a problem hiding this comment.
Good catch — you're right, pushed 6e59a7b restoring the onExecutionPayload(anchor, …) seeding. Upstream c7a0a8527 only removes the PTC vote seeding (payload_timeliness_vote={} / payload_data_availability_vote={}); it doesn't change the semantics of the anchor's payload being considered executed. The anchor state's latestBlockHash is the hash of the executed payload, so marking the anchor's FULL variant here still matches the spec's intent of payload_states = {anchor_root: anchor_state.copy()}.
Scope of this PR is now narrowed back to exactly what the upstream spec fix requires: dropping the 8-line PTC vote override only.
There was a problem hiding this comment.
Walking this back — pushed 1681b0d reverting the seeding. Per nflaig's follow-up and re-reading consensus-specs master: get_forkchoice_store now has payloads={}, is_payload_verified is literally root in store.payloads, and should_extend_payload short-circuits on not is_payload_verified. So the anchor is deliberately unseeded by spec; our !hasPayload() guard already matches that. The block-production consequence is expected to be resolved by sync building up FULL variants as envelopes arrive for post-anchor blocks. Sorry for the initial direction flip.
![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: b86b6b22e4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| null | ||
| ); | ||
|
|
||
| // Anchor block PTC votes must be all-true per spec get_forkchoice_store: | ||
| // payload_timeliness_vote={anchor_root: Vector[boolean, PTC_SIZE](True for _ in range(PTC_SIZE))} | ||
| // Spec: https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.4/specs/gloas/fork-choice.md#modified-get_forkchoice_store | ||
| if (protoArray.ptcVotes.has(block.blockRoot)) { | ||
| protoArray.ptcVotes.set(block.blockRoot, BitArray.fromBoolArray(Array.from({length: PTC_SIZE}, () => true))); | ||
|
|
||
| // In the spec, we have payload_states = {anchor_root: anchor_state.copy()} | ||
| // which means the anchor's "payload" is considered received | ||
| // Without FULL, blocks extending FULL from the anchor would be orphaned. | ||
| // TODO GLOAS: This is a bug in the spec. Keep this to pass the current spec test | ||
| // for now. Need to remove this when we work on v1.7.0-alpha.5 | ||
| if (block.executionPayloadBlockHash !== null) { | ||
| protoArray.onExecutionPayload( | ||
| block.blockRoot, | ||
| currentSlot, | ||
| block.executionPayloadBlockHash, | ||
| (block as {executionPayloadNumber?: number}).executionPayloadNumber ?? 0, | ||
| block.stateRoot, | ||
| null, | ||
| ExecutionStatus.Valid | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return protoArray; |
There was a problem hiding this comment.
By returning immediately after onBlock() in initialize(), post-Gloas anchors no longer get a FULL variant, so shouldExtendPayload() now always fails its !hasPayload() guard for the anchor path. That value is consumed by block production (produceBlockBody/prepareNextSlot) to choose latestExecutionPayloadBid.blockHash vs latestExecutionPayloadBid.parentBlockHash; when the anchor payload is actually available (latestBlockHash == latestExecutionPayloadBid.blockHash), this forces the wrong parent hash and can produce bids that violate processExecutionPayloadBid's parent-hash check. In practice, this can make the first block(s) after restart from a post-Gloas anchor invalid or rejected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — traced the call chain and this is a real regression. #9209 made block production (both produceBlockBody and prepareNextSlot) use forkChoice.shouldExtendPayload(headRoot) to pick between latestExecutionPayloadBid.blockHash and latestExecutionPayloadBid.parentBlockHash. With the anchor lacking a FULL variant, shouldExtendPayload(anchor) short-circuits to false via the !hasPayload() guard, so the first bid after restart from a post-Gloas anchor uses bid.parentBlockHash and fails processExecutionPayloadBid's parent-hash check.
Pushed 6e59a7b restoring the onExecutionPayload(anchor, …) seeding while keeping the PTC vote override removal (which is the only piece the upstream c7a0a8527 spec fix actually requires).
There was a problem hiding this comment.
this needs to be handled by sync
There was a problem hiding this comment.
Agreed — reverted in 1681b0d. Post-restart proto-array picks up FULL variants via onExecutionPayload as sync processes envelopes for post-anchor blocks; no init-time seeding needed.
There was a problem hiding this comment.
Walking this back — pushed 1681b0d reverting the seeding. Per nflaig's follow-up and re-reading consensus-specs master: the spec deliberately leaves the anchor unseeded (payloads={}, and should_extend_payload returns False via the is_payload_verified guard). Our !hasPayload() short-circuit is the spec-correct behavior, not a regression. The first-bid-after-restart edge case you described is expected to be handled by sync — proto-array picks up FULL variants as envelopes are processed for post-anchor blocks during catch-up, before the validator is eligible to propose. Sorry for the initial direction flip.
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
| // Gloas: seed the anchor's FULL variant so `shouldExtendPayload(anchor)` is true | ||
| // for block production; otherwise bids after restart use `bid.parentBlockHash` | ||
| // and fail `processExecutionPayloadBid`'s parent-hash check. | ||
| if (isGloasBlock(block as ProtoBlock) && block.executionPayloadBlockHash !== null) { |
There was a problem hiding this comment.
this is wrong, please see latest consensus-specs on master branch, it's not correct to seed the payload of the anchor block
There was a problem hiding this comment.
You're right, pushed 1681b0d reverting the seeding. Re-checking consensus-specs master specs/gloas/fork-choice.md:
get_forkchoice_storeinitializespayloads={}— anchor is deliberately not payload-verified.is_payload_verified(root)is literallyreturn root in store.payloads, sois_payload_verified(anchor_root) == False.should_extend_payloadshort-circuits withif not is_payload_verified(store, root): return False.
So our existing !hasPayload() guard in shouldExtendPayload already matches spec behavior on the unseeded anchor — returning false is correct, not a regression. My earlier reply to the bots was wrong; sorry for the noise. PR is now back to just the 8-line PTC-vote-override revert, matching c7a0a8527 exactly.
…alize" This reverts commit 6e59a7b. Per @nflaig review feedback (discussion_r3116954612, discussion_r3116958318) and re-reading consensus-specs `master` `specs/gloas/fork-choice.md`: - `get_forkchoice_store` initializes `payloads={}` — the anchor is deliberately not seeded as payload-verified. - `is_payload_verified(root)` is literally `return root in store.payloads`, so `is_payload_verified(anchor_root) == False` after restart. - `should_extend_payload` on the anchor returns `False` via the `if not is_payload_verified: return False` short-circuit. Lodestar's `shouldExtendPayload`'s `!hasPayload()` guard therefore matches spec behavior — returning `false` on the unseeded anchor is correct, not a bug. The block-production bid-selection consequence is an edge case that sync is expected to resolve (proto-array gets FULL variants for post-anchor blocks as their envelopes are processed during catch-up). 🤖 Generated with AI assistance
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #9244 +/- ##
=========================================
Coverage 52.53% 52.53%
=========================================
Files 848 848
Lines 61405 61405
Branches 4525 4525
=========================================
Hits 32259 32259
Misses 29081 29081
Partials 65 65 🚀 New features to boost your workflow:
|
|
this might need |
Required to pass spec tests that rely on the alpha.5 anchor-block payload-parent semantics (consensus-specs ChainSafe#5067 "Treat genesis block as empty payload parent"), which aligns with reverting the anchor block PTC vote override. 🤖 Generated with AI assistance
This reverts commit 47f27f4.
|
Reverted the alpha.5 spec bump ( Why spec tests fail on this PR (alpha.3 baseline, commit Why bumping to alpha.5 wasn't a drop-in (
Scope summary: Making this PR pass spec tests requires a genuine alpha.5 integration — new runner implementation + specrefs/source-mapping updates + rename cascade. That's substantially out of scope for this focused revert. Options:
Which direction would you prefer? Happy to pick up any of them. |
|
@lodekeeper we have merged alpha.5 into unstable, please update your branch, also double check if the changes are still needed and make sure you are passing spec tests |
|
Thanks @nflaig — double-checked against alpha.5 and the anchor PTC override should stay, so this PR is no longer needed.
payload_timeliness_vote={
anchor_root: Vector[boolean, PTC_SIZE](True for _ in range(PTC_SIZE))
},
payload_data_availability_vote={
anchor_root: Vector[boolean, PTC_SIZE](True for _ in range(PTC_SIZE))
},The spec fix Closing this PR. Will revisit when a spec tag picks up Side note: alpha.5 also seeds |
|
Closing: alpha.5 spec still requires anchor PTC all-true seeding (see previous comment). Revisit when spec tag picks up consensus-specs#5135. |
@lodekeeper please check the |
|
Checked
I'll reopen a PR of the same shape once a spec tag (alpha.6 or later) includes #5135. Until then no branch action needed, and #9244 stays closed. Related: I also noticed alpha.5 seeds |
|
@lodekeeper please make sure we follow latest |
|
Re-opened as #9264 on top of current Change is identical — removes the 8-line anchor-PTC-all-true override in Noted in the new PR that alpha.5 still carries the old seed, so fork-choice spec tests covering the anchor boundary may flag until |
## Summary Revert the anchor-block PTC vote all-true override added in #9188. Upstream `consensus-specs` `master` fixed the underlying spec bug in: - [`c7a0a8527`](ethereum/consensus-specs@c7a0a8527) — **Remove incorrect anchor seed for payload votes (#5135)** [`specs/gloas/fork-choice.md#get_forkchoice_store`](https://github.com/ethereum/consensus-specs/blob/master/specs/gloas/fork-choice.md#modified-get_forkchoice_store) on `master` now initializes: ```python payload_timeliness_vote={}, payload_data_availability_vote={}, ``` So Lodestar should stop force-seeding the anchor block's PTC votes to all-true in `ProtoArray.initialize()` — anchor initialization returns to the default `onBlock()` behavior. This PR removes the temporary 8-line override. ## Tag vs. master \`v1.7.0-alpha.5\` (current spec-test target) still carries the old anchor all-true seed — #5135 landed on spec master **after** the alpha.5 cut. Aligning Lodestar with `master` now means: - the fix is in place for the next spec tag (alpha.6+) picking up #5135 - fork-choice spec tests generated from alpha.5 may flag the anchor-seed boundary until `spec-tests-version.json` bumps to that next tag Per direction from @nflaig to follow latest `master` of the spec. ## Context - Supersedes closed PR #9244 (closed when alpha.5 was still the current tag and kept the old seed). - Fixes #9239. ## AI assistance Drafted and validated with AI assistance. Co-authored-by: lodekeeper <lodekeeper@users.noreply.github.com>
|
Done in #9264 — same-shape single-commit revert of the anchor PTC all-true override, aligned to consensus-specs CI is green on #9264 including the Keeping #9244 closed as the superseded draft. |
3 participants
Summary
Revert the anchor-block PTC vote override added in #9188.
Upstream
consensus-specsmasterfixed the underlying spec bug in:c7a0a8527—Remove incorrect anchor seed for payload votes (#5135)specs/gloas/fork-choice.md#get_forkchoice_storeonmasternow initializes:payload_timeliness_vote={}So Lodestar should stop force-seeding the anchor block to all-true in
ProtoArray.initialize().This PR removes the temporary 8-line override and returns anchor initialization to the default
onBlock()behavior.Verification
pnpm lint -- packages/fork-choice/src/protoArray/protoArray.tsAI assistance
Drafted and validated with AI assistance.
Fixes #9239