Skip to content

fix: revert anchor block PTC vote override #9244

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.

Sign up for GitHub

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

Closed
lodekeeper wants to merge 6 commits into from
Closed

fix: revert anchor block PTC vote override #9244

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b86b6b2
fix: revert anchor block PTC vote override
lodekeeper Apr 21, 2026
6e59a7b
fix: preserve anchor FULL variant seeding in ProtoArray.initialize
lodekeeper Apr 21, 2026
1681b0d
Revert "fix: preserve anchor FULL variant seeding in ProtoArray.initi…
lodekeeper Apr 21, 2026
0d92ce8
Merge remote-tracking branch 'origin/unstable' into fix/revert-anchor…
lodekeeper Apr 21, 2026
47f27f4
chore: bump spec tests to v1.7.0-alpha.5
lodekeeper Apr 21, 2026
7477fb6
Revert "chore: bump spec tests to v1.7.0-alpha.5"
lodekeeper Apr 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 0 additions & 24 deletions packages/fork-choice/src/protoArray/protoArray.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,30 +109,6 @@ export class ProtoArray {
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;
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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;

Copy link
Copy Markdown
Contributor Author

@lodekeeper lodekeeper Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@lodekeeper lodekeeper Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 109 to 112
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve anchor payload availability in ProtoArray.initialize

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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

@lodekeeper lodekeeper Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Member

@nflaig nflaig Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be handled by sync

Copy link
Copy Markdown
Contributor Author

@lodekeeper lodekeeper Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@lodekeeper lodekeeper Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

Expand Down