feat: implement EIP-8045 exclude slashed validators from proposing#9422
Merged
Conversation
![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 processProposerLookahead function to exclude slashed validators from proposing under EIP-8045, specifically when the fork is greater than or equal to ForkSeq.gloas. It filters shuffling.activeIndices using the FLAG_UNSLASHED flag before passing them to computeProposerIndices. There are no review comments, so I have no feedback to provide.
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
added a commit
that referenced
this pull request
Jun 1, 2026
lodekeeper
approved these changes
Jun 1, 2026
Contributor
There was a problem hiding this comment.
LGTM — implementation matches consensus-specs#5115 and all 4 new spec test cases pass locally.
Test results
Built gloas+minimal reftests from consensus-specs#5115 (head 52a38c40a), overlaid onto packages/beacon-node/spec-tests/tests/minimal/gloas/, ran against this branch (head 689969e423):
| Suite | Test | Result |
|---|---|---|
epoch_processing/proposer_lookahead |
proposer_lookahead_does_not_contain_slashed_validators |
✅ pass |
epoch_processing/proposer_lookahead |
proposer_lookahead_full_with_many_slashed_validators |
✅ pass |
sanity/blocks |
proposer_lookahead_excludes_slashed_validators |
✅ pass |
fork/fork |
fork_proposer_lookahead_slashed_validators_carried_over |
✅ pass |
Implementation review
fork >= ForkSeq.gloasfilter inprocessProposerLookahead.tscorrectly mirrors the spec's[index for index in get_active_validator_indices(state, epoch) if not state.validators[index].slashed].FLAG_UNSLASHEDis set inbeforeProcessEpoch(epochTransitionCache.ts:283) directly fromstate.validators[i].slashed, and is consistent at the timeprocessProposerLookaheadruns.- The Fulu→Gloas fork transition is covered by the existing
upgradeStateToGloas.ts:67which carriesproposerLookaheadover unchanged, matching the spec test's expectation that pre-fork (Fulu-computed) entries persist with their slashed validators through the boundary. util/fulu.ts::initializeProposerLookaheadis only called fromupgradeStateToFulu.ts(Electra→Fulu boundary), not on any Gloas path — no EIP-8045 gap there.
Edit: A previous "Side note" about 2 pre-existing failures in the pinned v1.7.0-alpha.8 tarball was incorrect — my local spec-tests dir was stuck at v1.7.0-alpha.2 (6 versions stale). Those tests pass on CI with fresh alpha.8 data. Apologies for the noise.
wemeetagain
previously approved these changes
Jun 2, 2026
nflaig
commented
Jun 2, 2026
| // (consensus-specs #5275) are implemented. New gloas on_attestation vectors in v1.7.0-alpha.9. | ||
| name.endsWith("validate_on_attestation_beacon_root_payload_check") || | ||
| name.endsWith("validate_on_attestation_payload_invalid_index") || | ||
| name.endsWith("validate_on_attestation_same_slot_full_vote_rejected"), |
Member
Author
There was a problem hiding this comment.
a bit confused about these but can be addressed in a follow up
Contributor
There was a problem hiding this comment.
Yeah, these 3 are the new gloas validate_on_attestation payload-status vectors from consensus-specs #5275 (merged 2026-05-29):
validate_on_attestation_same_slot_full_vote_rejected— at the attested block's slot, a full-node vote (payload_index=1) must be rejected even if a verified envelope exists.validate_on_attestation_payload_invalid_index—payload_index=2(or any non-{0,1}) is rejected.validate_on_attestation_beacon_root_payload_check— across an epoch boundary, a full-node vote requires the beacon block root's payload to be verified.
Same on_attestation handler we skipped under the on_payload_attestation_message umbrella for the PTC fork-choice work — these explicitly cover the attestation-side payload validation. Will unskip together with the broader Gloas fork-choice PTC work in a follow-up.
nflaig
commented
Jun 2, 2026
| // unconditionally. Only the altair vectors exercise the changed condition (other forks pass). | ||
| name.endsWith("altair/fork_choice/on_block/pyspec_tests/justified_update_always_if_better") || | ||
| name.endsWith("altair/fork_choice/on_block/pyspec_tests/justified_update_not_realized_finality") || | ||
| name.endsWith("altair/fork_choice/get_head/pyspec_tests/voting_source_beyond_two_epoch") || |
Member
Author
There was a problem hiding this comment.
- we need Apply proposer boost if dependent roots match ethereum/consensus-specs#5306 to pass these
Contributor
There was a problem hiding this comment.
Confirmed — these 3 altair vectors were added in consensus-specs #5306 (merged 2026-06-01) to switch proposer boost from "same proposer index" to "same dependent root".
Spec diff in specs/phase0/fork-choice.md:
# Before
if block.proposer_index == get_beacon_proposer_index(head_state):
store.proposer_boost_root = root
# After (#5306)
is_same_dependent_root = get_dependent_root(store, root) == get_dependent_root(store, head)
if is_timely and is_first_block and is_same_dependent_root:
store.proposer_boost_root = rootLodestar's check is at packages/fork-choice/src/forkChoice/forkChoice.ts:677-685 — still using block.proposerIndex === expectedProposerIndex. Will unskip once we add get_dependent_root and swap the predicate in a follow-up.
nflaig
commented
Jun 2, 2026
| // implemented. v1.7.0-alpha.9 regenerated these transition vectors so eth1-bridge deposits are | ||
| // no longer processed across the Electra to Fulu boundary; Lodestar still applies the Electra | ||
| // deposit flow, diverging the post-fork state root. | ||
| /^fulu\/transition\/.*/, |
Member
Author
There was a problem hiding this comment.
there's more code we might be able to remove due to ethereum/consensus-specs#4704, can be done in separate PR
Contributor
There was a problem hiding this comment.
Confirmed. #4704 drops the legacy eth1-bridge deposit path in Fulu+:
process_operationsin Fulu+ assertslen(body.deposits) == 0and no longer callsprocess_depositprocess_pending_depositsloses the "Eth1 bridge deposits not yet applied" guard- Gloas spec page just reuses the Fulu version
Lodestar mirror:
state-transition/src/block/processOperations.ts:42-61—getEth1DepositCount+for (const deposit of body.deposits) processDeposit(...)should be guardedfork < ForkSeq.fulu(and the loop dropped for ≥ Fulu).state-transition/src/epoch/processPendingDeposits.ts— drop theeth1_deposit_index < deposit_requests_start_indexearly-break in Fulu+.
Will unskip fulu/transition/* and clean these up in a separate PR.
nflaig
commented
Jun 2, 2026
| - get_base_reward#phase0 | ||
| - get_checkpoint_block#phase0 | ||
| - get_current_store_epoch#phase0 | ||
| - get_dependent_root#phase0 |
Member
Author
There was a problem hiding this comment.
worth to revisit this, we should be able to point this to the proper function in our code
Contributor
There was a problem hiding this comment.
Good catch — followed the same pattern as is_eligible_for_activation / is_eligible_for_activation_queue (both already point to epochTransitionCache.ts via an inline Python signature comment).
ethspecify check passes (1102 valid, was 1101).
Diff (apply with git am < patch or copy directly — I can't push to your branch on ChainSafe):
diff --git a/packages/state-transition/src/cache/epochTransitionCache.ts b/packages/state-transition/src/cache/epochTransitionCache.ts
--- a/packages/state-transition/src/cache/epochTransitionCache.ts
+++ b/packages/state-transition/src/cache/epochTransitionCache.ts
@@ -296,6 +296,16 @@ export function beforeProcessEpoch(
// Both active validators and slashed-but-not-yet-withdrawn validators are eligible to receive penalties.
// This is done to prevent self-slashing from being a way to escape inactivity leaks.
// TODO: Consider using an array of `eligibleValidatorIndices: number[]`
+ // ```python
+ // def get_eligible_validator_indices(state: BeaconState) -> Sequence[ValidatorIndex]:
+ // previous_epoch = get_previous_epoch(state)
+ // return [
+ // ValidatorIndex(index)
+ // for index, v in enumerate(state.validators)
+ // if is_active_validator(v, previous_epoch)
+ // or (v.slashed and previous_epoch + 1 < v.withdrawable_epoch)
+ // ]
+ // ```
if (isActivePrev || (validator.slashed && prevEpoch + 1 < validator.withdrawableEpoch)) {
flag |= FLAG_ELIGIBLE_ATTESTER;
}
diff --git a/specrefs/.ethspecify.yml b/specrefs/.ethspecify.yml
--- a/specrefs/.ethspecify.yml
+++ b/specrefs/.ethspecify.yml
@@ -153,7 +153,6 @@ exceptions:
- get_checkpoint_block#phase0
- get_current_store_epoch#phase0
- get_dependent_root#phase0
- - get_eligible_validator_indices#phase0
- get_eth1_vote#phase0
- get_filtered_block_tree#phase0
- get_forkchoice_store#phase0
diff --git a/specrefs/functions.yml b/specrefs/functions.yml
--- a/specrefs/functions.yml
+++ b/specrefs/functions.yml
@@ -3630,7 +3630,9 @@
</spec>
- name: get_eligible_validator_indices#phase0
- sources: []
+ sources:
+ - file: packages/state-transition/src/cache/epochTransitionCache.ts
+ search: "// def get_eligible_validator_indices(state: BeaconState) -> Sequence[ValidatorIndex]:"
spec: |
<spec fn="get_eligible_validator_indices" fork="phase0" hash="22a24ce4">
def get_eligible_validator_indices(state: BeaconState) -> Sequence[ValidatorIndex]:
wemeetagain
previously approved these changes
Jun 2, 2026
nflaig
commented
Jun 2, 2026
Comment on lines
-29
to
+36
| const lastEpochProposerLookahead = computeProposerIndices(fork, state, shuffling, epoch); | ||
| const activeIndices = | ||
| fork >= ForkSeq.gloas | ||
| ? // Exclude slashed validators from proposing (EIP-8045) | ||
| shuffling.activeIndices.filter((index) => (cache.flags[index] & FLAG_UNSLASHED) !== 0) | ||
| : shuffling.activeIndices; | ||
|
|
||
| const lastEpochProposerLookahead = computeProposerIndices(fork, state, {activeIndices}, epoch); |
Member
Author
There was a problem hiding this comment.
for reference, that's the only real change in this PR
Contributor
There was a problem hiding this comment.
👍 Yep, this 7-line filter is the entire EIP-8045 surface — FLAG_UNSLASHED is set in epochTransitionCache.ts:283 from state.validators[i].slashed, so the gated shuffling.activeIndices.filter is the exact mirror of consensus-specs#5115's pre-compute_proposer_indices slashed filter.
nflaig
commented
Jun 2, 2026
| // TODO: decide whether to expose EMPTY_BLOCK_HASH in the config API. New bellatrix constant in | ||
| // v1.7.0-alpha.9; a zero Hash32 only used in the terminal-block fork-choice check, so skipped for | ||
| // now like the PAYLOAD_STATUS_* constants above. | ||
| "EMPTY_BLOCK_HASH", |
Member
Author
There was a problem hiding this comment.
another follow up we need to handle
Contributor
There was a problem hiding this comment.
Investigated. Recommendation: keep skipped, tighten the rationale comment so it's no longer a TODO.
What it is: EMPTY_BLOCK_HASH: Hash32 = Hash32() — a 32-byte zero sentinel introduced in v1.7.0-alpha.9. Only spec use is the if TERMINAL_BLOCK_HASH != EMPTY_BLOCK_HASH: check (validate_merge_block, is_valid_terminal_pow_block, notify_new_payload).
Why skip is the right call:
- Other CL clients don't expose it —
EMPTY_BLOCK_HASHreturns 0 hits on Lighthouse, Prysm, and Teku via GitHub code search. The constant is purely a spec-readability rename of the literal zero hash; nobody imports it. - It's a fixed sentinel, not configurable — always
0x0000…0000. Clients comparingchainConfig.TERMINAL_BLOCK_HASHagainst the literal cover the same check. - Matches existing precedent —
PAYLOAD_STATUS_VALID/INVALIDATED/NOT_VALIDATED(also bellatrix-introduced named sentinels) are skipped on the same grounds.
If we want to permanently bless the skip, suggested wording (replacing the current TODO):
- // TODO: decide whether to expose EMPTY_BLOCK_HASH in the config API. New bellatrix constant in
- // v1.7.0-alpha.9; a zero Hash32 only used in the terminal-block fork-choice check, so skipped for
- // now like the PAYLOAD_STATUS_* constants above.
+ // EMPTY_BLOCK_HASH (bellatrix, v1.7.0-alpha.9) is a 32-byte zero sentinel used only for
+ // `TERMINAL_BLOCK_HASH != EMPTY_BLOCK_HASH` comparisons. Not exposed via the config API:
+ // it's a fixed value, not configurable, and Lighthouse/Prysm/Teku don't expose it either.
+ // Same treatment as the PAYLOAD_STATUS_* sentinels above.
"EMPTY_BLOCK_HASH",Happy to either:
- Push that comment-only tightening to the branch (if I had access — diff above is ready), or
- Reverse the call and actually expose it (one line: add
EMPTY_BLOCK_HASHtospecConstantsinpackages/beacon-node/src/api/impl/config/constants.tsastoHexBytes32(EMPTY_BLOCK_HASH)).
Your call. Tracking as a 🟢 follow-up in the meantime.
wemeetagain
approved these changes
Jun 2, 2026
This was referenced Jun 2, 2026
3 tasks
1 task
nflaig
pushed a commit
that referenced
this pull request
Jun 6, 2026
**Motivation** Three Gloas `on_attestation` spec test vectors added in ethereum/consensus-specs#5275 were skipped in #9422 with a TODO pending investigation. This PR resolves that investigation and re-enables the tests. **Description** The validation logic in `validateAttestationData` was already correct — all three payload-status checks were implemented and firing as expected. The root cause was that the attestation step handler in `fork_choice.test.ts` had no `valid` flag handling: it called `onAttestation` unconditionally and let any thrown `ForkChoiceError` propagate as a test failure, even when the spec vector marks the attestation as `valid: false`. Fix: wrap `onAttestation` in the same `try/catch` + `isValid` pattern already used by the `execution_payload` step handler, then unskip the three tests. Closes #9447
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 https://eips.ethereum.org/EIPS/eip-8045 and ethereum/consensus-specs#5115