feat: implement should_apply_proposer_boost for gloas - #9233
Open
ensi321 wants to merge 12 commits into
Open
Conversation
Implement the `should_apply_proposer_boost` logic from consensus-specs commit 71d1151 (PR #4807). This addresses the builder reveal safety concern where a colluding next-slot proposer could use proposer boost to override a legitimately revealed block. Changes: - Add `ptcTimeliness` and `proposerIndex` fields to ProtoBlock - Add `isBlockPtcTimely` to track PTC deadline timeliness - Add `shouldApplyProposerBoost` which withholds boost when the parent is a weak, equivocating block from the previous slot - Add `findEquivocatingBlocks` in ProtoArray to detect proposer equivocations by scanning for PTC-timely blocks at the same slot from the same proposer - Gate proposer boost in `getWeight` on `shouldApplyProposerBoost()` - Pre-gloas blocks retain unconditional boost (backward compatible) Co-Authored-By: Claude Opus 4.6 (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 implements the Gloas fork-choice logic for proposer boost and PTC (Payload Timeliness Committee) timeliness. It adds ptcTimeliness and proposerIndex to block metadata and introduces the shouldApplyProposerBoost logic, which considers parent block weight and proposer equivocations. Feedback focuses on preventing a crash during the fork transition by using dynamic payload status for parent nodes and optimizing the performance of the equivocation check to avoid O(N) map iterations during head updates.
ensi321
marked this pull request as draft
April 18, 2026 02:25
![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: 9bc6eb6ce5
ℹ️ 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".
Contributor
Performance Report🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
4 tasks
GrapeBaBa
added a commit
to GrapeBaBa/lodestar
that referenced
this pull request
May 1, 2026
Wire the consensus-specs Fork Choice Compliance suite (ChainSafe#3831) into the existing `forkChoiceTest` runner. The on-disk layout matches the standard spec-test layout (`tests/<preset>/<fork>/fork_choice_compliance/<handler>/<suite>/<case>/`), so it slots in alongside `fork_choice` and `sync` runners. Three test-only accommodations the compliance fixtures require: 1. `bls_setting: 2` — every compliance fixture uses placeholder signatures. Pass `validSignatures: testcase.meta?.bls_setting !== BigInt(1)` to `chain.processBlock` so verification short-circuits. Standard `fork_choice` fixtures use `bls_setting: 1` so behavior there is unchanged. 2. `BLOCK_ERROR_ALREADY_KNOWN` — compliance fixtures intentionally re-import the same block (`dup_shift` mutations in their `meta.yaml`). Spec semantics for `on_block(store, known_block)` is a no-op success. Production block import correctly rejects with ALREADY_KNOWN; this runner treats that case as success only when the step is `valid: true`. 3. Cross-epoch attestation shuffling — `on_attestation` decodes aggregation_bits using the state at the attestation's target checkpoint, not the head state. The runner now resolves the right shuffling via ShufflingCache + regen (mirroring the production validation path) instead of `headState.epochCtx.getIndexedAttestation`, which only worked when the attestation's epoch happened to be in the head's epoch cache (±1 epoch) and broke on cross-epoch fork attestations surfaced by the compliance suite. Adds support for two compliance-only check fields: - `viable_for_head_roots_and_weights` (consensus-specs#3831): compared via `getViableHeads()`. Both sides are sorted by root before comparison since the spec doesn't fix order. - `head_payload_status` (gloas): mapped between our internal enum ordering (PENDING=0, EMPTY=1, FULL=2) and spec ordering (EMPTY=0, FULL=1, PENDING=2). Pass rate against the latest comptests workflow `small.tar.gz` artifact: fulu/fork_choice_compliance: 253/1472 cases pass (17.2%) Top remaining failures: - ~80% `Invalid proposer boost root` — consensus-specs#4807 introduced a `block.proposer_index == get_beacon_proposer_index(head_state)` guard in `update_proposer_boost_root` that we do not yet implement; affects all forks (not just gloas equivocation handling). Tracked for follow-up alongside ChainSafe#9233. - ~1% `Invalid viable heads` — proposer-boost rounding on minimal preset (see `getViableHeads()` weight note).
GrapeBaBa
added a commit
to GrapeBaBa/lodestar
that referenced
this pull request
May 1, 2026
Wire the consensus-specs Fork Choice Compliance suite (ChainSafe#3831) into the existing `forkChoiceTest` runner. The on-disk layout matches the standard spec-test layout (`tests/<preset>/<fork>/fork_choice_compliance/<handler>/<suite>/<case>/`), so it slots in alongside `fork_choice` and `sync` runners. Three test-only accommodations the compliance fixtures require: 1. `bls_setting: 2` — every compliance fixture uses placeholder signatures. Pass `validSignatures: testcase.meta?.bls_setting !== BigInt(1)` to `chain.processBlock` so verification short-circuits. Standard `fork_choice` fixtures use `bls_setting: 1` so behavior there is unchanged. 2. `BLOCK_ERROR_ALREADY_KNOWN` — compliance fixtures intentionally re-import the same block (`dup_shift` mutations in their `meta.yaml`). Spec semantics for `on_block(store, known_block)` is a no-op success. Production block import correctly rejects with ALREADY_KNOWN; this runner treats that case as success only when the step is `valid: true`. 3. Cross-epoch attestation shuffling — `on_attestation` decodes aggregation_bits using the state at the attestation's target checkpoint, not the head state. The runner now resolves the right shuffling via ShufflingCache + regen (mirroring the production validation path) instead of `headState.epochCtx.getIndexedAttestation`, which only worked when the attestation's epoch happened to be in the head's epoch cache (±1 epoch) and broke on cross-epoch fork attestations surfaced by the compliance suite. Adds support for two compliance-only check fields: - `viable_for_head_roots_and_weights` (consensus-specs#3831): compared via `getViableHeads()`. Both sides are sorted by root before comparison since the spec doesn't fix order. - `head_payload_status` (gloas): mapped between our internal enum ordering (PENDING=0, EMPTY=1, FULL=2) and spec ordering (EMPTY=0, FULL=1, PENDING=2). Pass rate against the latest comptests workflow `small.tar.gz` artifact: fulu/fork_choice_compliance: 253/1472 cases pass (17.2%) Top remaining failures: - ~80% `Invalid proposer boost root` — consensus-specs#4807 introduced a `block.proposer_index == get_beacon_proposer_index(head_state)` guard in `update_proposer_boost_root` that we do not yet implement; affects all forks (not just gloas equivocation handling). Tracked for follow-up alongside ChainSafe#9233. - ~1% `Invalid viable heads` — proposer-boost rounding on minimal preset (see `getViableHeads()` weight note).
GrapeBaBa
added a commit
to GrapeBaBa/lodestar
that referenced
this pull request
May 1, 2026
Wire the consensus-specs Fork Choice Compliance suite (ChainSafe#3831) into the existing `forkChoiceTest` runner. The on-disk layout matches the standard spec-test layout (`tests/<preset>/<fork>/fork_choice_compliance/<handler>/<suite>/<case>/`), so it slots in alongside `fork_choice` and `sync` runners. Three test-only accommodations the compliance fixtures require: 1. `bls_setting: 2` — every compliance fixture uses placeholder signatures. Pass `validSignatures: testcase.meta?.bls_setting !== BigInt(1)` to `chain.processBlock` so verification short-circuits. Standard `fork_choice` fixtures use `bls_setting: 1` so behavior there is unchanged. 2. `BLOCK_ERROR_ALREADY_KNOWN` — compliance fixtures intentionally re-import the same block (`dup_shift` mutations in their `meta.yaml`). Spec semantics for `on_block(store, known_block)` is a no-op success. Production block import correctly rejects with ALREADY_KNOWN; this runner treats that case as success only when the step is `valid: true`. 3. Cross-epoch attestation shuffling — `on_attestation` decodes aggregation_bits using the state at the attestation's target checkpoint, not the head state. The runner now resolves the right shuffling via ShufflingCache + regen (mirroring the production validation path) instead of `headState.epochCtx.getIndexedAttestation`, which only worked when the attestation's epoch happened to be in the head's epoch cache (±1 epoch) and broke on cross-epoch fork attestations surfaced by the compliance suite. Adds support for two compliance-only check fields: - `viable_for_head_roots_and_weights` (consensus-specs#3831): compared via `getViableHeads()`. Both sides are sorted by root before comparison since the spec doesn't fix order. - `head_payload_status` (gloas): mapped between our internal enum ordering (PENDING=0, EMPTY=1, FULL=2) and spec ordering (EMPTY=0, FULL=1, PENDING=2). Pass rate against the latest comptests workflow `small.tar.gz` artifact: fulu/fork_choice_compliance: 253/1472 cases pass (17.2%) Top remaining failures: - ~80% `Invalid proposer boost root` — consensus-specs#4807 introduced a `block.proposer_index == get_beacon_proposer_index(head_state)` guard in `update_proposer_boost_root` that we do not yet implement; affects all forks (not just gloas equivocation handling). Tracked for follow-up alongside ChainSafe#9233. - ~1% `Invalid viable heads` — proposer-boost rounding on minimal preset (see `getViableHeads()` weight note).
GrapeBaBa
added a commit
to GrapeBaBa/lodestar
that referenced
this pull request
Jul 7, 2026
Wire the consensus-specs Fork Choice Compliance suite (ChainSafe#3831) into the existing `forkChoiceTest` runner. The on-disk layout matches the standard spec-test layout (`tests/<preset>/<fork>/fork_choice_compliance/<handler>/<suite>/<case>/`), so it slots in alongside `fork_choice` and `sync` runners. Three test-only accommodations the compliance fixtures require: 1. `bls_setting: 2` — every compliance fixture uses placeholder signatures. Pass `validSignatures: testcase.meta?.bls_setting !== BigInt(1)` to `chain.processBlock` so verification short-circuits. Standard `fork_choice` fixtures use `bls_setting: 1` so behavior there is unchanged. 2. `BLOCK_ERROR_ALREADY_KNOWN` — compliance fixtures intentionally re-import the same block (`dup_shift` mutations in their `meta.yaml`). Spec semantics for `on_block(store, known_block)` is a no-op success. Production block import correctly rejects with ALREADY_KNOWN; this runner treats that case as success only when the step is `valid: true`. 3. Cross-epoch attestation shuffling — `on_attestation` decodes aggregation_bits using the state at the attestation's target checkpoint, not the head state. The runner now resolves the right shuffling via ShufflingCache + regen (mirroring the production validation path) instead of `headState.epochCtx.getIndexedAttestation`, which only worked when the attestation's epoch happened to be in the head's epoch cache (±1 epoch) and broke on cross-epoch fork attestations surfaced by the compliance suite. Adds support for two compliance-only check fields: - `viable_for_head_roots_and_weights` (consensus-specs#3831): compared via `getViableHeads()`. Both sides are sorted by root before comparison since the spec doesn't fix order. - `head_payload_status` (gloas): mapped between our internal enum ordering (PENDING=0, EMPTY=1, FULL=2) and spec ordering (EMPTY=0, FULL=1, PENDING=2). Pass rate against the latest comptests workflow `small.tar.gz` artifact: fulu/fork_choice_compliance: 253/1472 cases pass (17.2%) Top remaining failures: - ~80% `Invalid proposer boost root` — consensus-specs#4807 introduced a `block.proposer_index == get_beacon_proposer_index(head_state)` guard in `update_proposer_boost_root` that we do not yet implement; affects all forks (not just gloas equivocation handling). Tracked for follow-up alongside ChainSafe#9233. - ~1% `Invalid viable heads` — proposer-boost rounding on minimal preset (see `getViableHeads()` weight note).
Member
|
@ensi321 is this still relevant? |
Hardcoding PayloadStatus.PENDING in the weak-parent lookup crashes at the gloas fork transition: the first gloas block's parent is pre-gloas (FULL-only), and getNodeIndexByRootAndStatus throws INVALID_NODE_INDEX when PENDING is requested for a pre-gloas root. The adjacent-parent guard does not short-circuit this path at the boundary. Use parentBlock.payloadStatus instead, matching the pattern already used for the strong-variant lookup elsewhere in this file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
For some reason, our unstable is now passing these tests even without this PR and we failed them previously lodestar/packages/beacon-node/test/spec/presets/fork_choice.test.ts Lines 738 to 742 in e6d38ec But we still need to implement Added some new tests ethereum/consensus-specs#5441 to make sure this is covered. |
ensi321
marked this pull request as ready for review
July 9, 2026 14:36
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #9233 +/- ##
=========================================
Coverage 52.49% 52.49%
=========================================
Files 848 848
Lines 60487 60486 -1
Branches 4466 4465 -1
=========================================
Hits 31755 31755
Misses 28670 28670
+ Partials 62 61 -1 🚀 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: d2bb9684f2
ℹ️ 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".
twoeths
reviewed
Jul 14, 2026
|
|
||
| // Indicate whether block arrives in a timely manner ie. before the 4 second mark | ||
| // Spec: Store.block_timeliness[ATTESTATION_TIMELINESS_INDEX] | ||
| timeliness: boolean; |
Member
There was a problem hiding this comment.
we may want to use a single block_timeliness: number to represent these 2 flags, cc @wemeetagain to confirm
Member
Author
There was a problem hiding this comment.
Is there a real benefit to this? Using bits impact readability, and we are only saving one boolean field per ProtoNode
This was referenced Jul 16, 2026
wemeetagain
pushed a commit
that referenced
this pull request
Jul 24, 2026
**Motivation** - is_head_weak() is needed for should_apply_proposer_boost() #9233 - we also need it for `getProposerHead()` **Description** - implement cross-fork [is_head_weak()](https://github.com/ethereum/consensus-specs/blob/6408b990c0b4e58d4f50ae49b59384e82e5e06d8/specs/gloas/fork-choice.md?plain=1#L739) spec api and use it in `getProposerHead()` - track attestation weight in ProtoNode separately, weight still means total weight - back propagate boost deltas and attestation deltas separately - also implement is_parent_strong() **AI Assistance Disclosure** - created with the help of Claude --------- Co-authored-by: Tuyen Nguyen <twoeths@users.noreply.github.com> Co-authored-by: bing <spiralladder@fastmail.com>
…roposer-boost # Conflicts: # packages/beacon-node/test/spec/presets/fork_choice.test.ts
…ProposerBoost is_head_weak() landed via #9654 with the exact gloas semantics (boost-excluded attestation score + committee-scoped equivocator balance add-back), so the weak-parent check can call it directly instead of approximating with raw node.weight against the reorg threshold. Also adapt findEquivocatingBlocks to the current VariantIndices layout via getDefaultNodeIndex, and add the new ProtoBlock fields to test fixtures that were added on unstable since the last merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
should_apply_proposer_boost judges the parent via is_head_weak against the attestations known to the store, but the decision ran before applyScoreChanges() landed this round's deltas, so it read the previous round's attestation scores. A parent crossing the weak threshold on the pending batch kept the boost withheld, and a newly detected equivocator kept its discounted vote counted. For a gloas boosted block, split the score update in two passes: attestation deltas first, then the boost decision, then the boost deltas. Pre-gloas keeps the single pass since the boost is unconditional there. Also replace findEquivocatingBlocks() with hasEquivocatingBlock(): the only caller tests emptiness, so return on first match instead of allocating a list. The two ordering tests fail on the previous commit and pass on this one; the spec vectors cannot catch this because the check step recomputes the head, self-healing the transient decision before assertions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Measures the two-pass applyScoreChanges overhead against the single-pass pre-gloas row: 2.96 ms/op vs 2.72 ms/op at vc 600k bc 64 (~9%), paid only while a gloas block holds proposer boost. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
nflaig
reviewed
Aug 4, 2026
| * | ||
| * Child class can overwrite this for testing purpose. | ||
| */ | ||
| protected isBlockPtcTimely(block: BeaconBlock, blockDelaySec: number): boolean { |
Member
There was a problem hiding this comment.
so this should only be relevant in case of proposer equivocation to make sure the next proposer can see it and can be aware of the equivocation in time?
so using the ptc timing seems pretty arbitrary to me but the purpose if I understand this correctly is to figure out if the it was an "early equivocation"
should this be renamed?
Suggested change
| protected isBlockPtcTimely(block: BeaconBlock, blockDelaySec: number): boolean { | |
| protected isBlockEquivocationTimely(block: BeaconBlock, blockDelaySec: number): boolean { |
Edit: after thinking more about it, not sure the rename is better because it could suggest that the block is an equivocation
| continue; | ||
| } | ||
| const node = this.nodes[nodeIndex]; | ||
| if (node !== undefined && node.slot === slot && node.proposerIndex === proposerIndex && node.ptcTimeliness) { |
Member
There was a problem hiding this comment.
I am not sure this can happen right now, if we receive a equivocating block over gossip, we would just ignore it
lodestar/packages/beacon-node/src/chain/validation/block.ts
Lines 86 to 90 in 148a1c5
this is kinda related to #9757, need to see if we wanna use seenBlockProposers or fork choice for this, but generally, the equivocation check on the api shouldn't care about timings, if there was an equivocation when submitting the payload, then it's safe to withhold as you don't need to pay the bid.value to the proposer
Member
There was a problem hiding this comment.
the IGNORE is more about the gossipsub spec, we should still process it and add to forkchoice #9805
some peers may still vote for the 2nd block, we'll use UnknownBlockInput sync to download, process and add to forkchoice soon anyway
nflaig
reviewed
Aug 4, 2026
Comment on lines
-748
to
+758
| // TODO GLOAS: boost attribution across payload-status variants of the boosted | ||
| // root should be handled when we set up gloas compliance test. | ||
| // Pre-gloas each root has exactly one entry. | ||
| const isBoosted = exp.root === expectedBoostRoot; | ||
| // Pre-gloas each root has exactly one entry. Gloas applies the boost to the | ||
| // PENDING variant only, keeping it neutral between EMPTY and FULL. | ||
| const isBoosted = | ||
| exp.root === expectedBoostRoot && | ||
| (!isGloas || exp.payloadStatus === payloadStatusToSpec[PayloadStatus.PENDING]); |
nflaig
reviewed
Aug 4, 2026
Comment on lines
+593
to
+595
| this.protoArray.applyScoreChanges({attestationDeltas, proposerBoost: null, ...checkpoints}); | ||
| const proposerBoost = this.shouldApplyProposerBoost() ? this.getProposerBoost() : null; | ||
| this.protoArray.applyScoreChanges({ |
Member
There was a problem hiding this comment.
is calling applyScoreChanges twice fine? cc @twoeths
Member
There was a problem hiding this comment.
right now applyScoreChanges() does 2 things:
- (1) update node's weights
- (2) update best child/descendants
we can improve by calling 1) twice and call 2) in the end
it's more like a refactor and does not affect fulu so we can just leave a TODO and address in another PR I guess
twoeths
reviewed
Aug 4, 2026
| // https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.12/specs/gloas/fork-choice.md#new-should_apply_proposer_boost | ||
| this.protoArray.applyScoreChanges({attestationDeltas, proposerBoost: null, ...checkpoints}); | ||
| const proposerBoost = this.shouldApplyProposerBoost() ? this.getProposerBoost() : null; | ||
| this.protoArray.applyScoreChanges({ |
Member
There was a problem hiding this comment.
no need to call applyScoreChanges() again if proposerBoost is null?
Member
Author
|
Going to reflect the changes from ethereum/consensus-specs#5515 also |
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
6 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.
Summary
Implements the
should_apply_proposer_boostlogic ethereum/consensus-specs/pull/4807 (gloas/fork-choice.md#new-should_apply_proposer_boost).Was blocked on
is_head_weak(); now rebased on top of #9654 (merged) and reuses itsisHeadWeak()— boost-excluded attestation score + equivocator balance add-back — for the weak-parent check, exactly matching the spec'sis_head_weak(store, parent_root)call.Changes
ptcTimelinessandproposerIndexfields toProtoBlockisBlockPtcTimelyto track PTC deadline timelinessshouldApplyProposerBoostwhich withholds boost when the parent is a weak, equivocating block from the previous slotisHeadWeak()from feat: implement forkchoice is_head_weak() and is_parent_strong() #9654 for the weak-parent checkfindEquivocatingBlocksinProtoArrayto detect proposer equivocationsgetWeightonshouldApplyProposerBoost()Note on test coverage
The existing gloas fork_choice vectors pass with or without this gate (the withhold branch executes but never decides the asserted head), so CI alone doesn't discriminate it. Discriminating vectors are proposed upstream in consensus-specs (
test_should_apply_proposer_boost.py, MC/DC over the three apply conditions); the withheld case fails onunstableand passes on this branch.AI Assistance Disclosure
Created with the help of Claude.
🤖 Generated with Claude Code