fix(beacon): check vote signatures under the fork schedule's domain

[MegaRedHand]

Problem

Gossip and the Beacon API verify vote signatures against a state that was never advanced to the vote's epoch:

Path	State used
beacon_attestation_{subnet} gossip (gossip::attestation::stateful_checks)	voted block's post-state
beacon_aggregate_and_proof gossip and POST /eth/v2/validator/aggregate_and_proofs (gossip::aggregate::stateful_checks)	voted block's post-state
POST /eth/v2/beacon/pool/attestations (pool::validate)	head's post-state

get_domain takes the fork version from state.fork. If a fork's first slots are empty, that state is still the previous fork's, so a correctly signed vote of the new fork fails its signature check. Gossip REJECTs it, which also penalizes the peer that relayed it, and a validator client gets "invalid signature". This lasts until a block of the new fork is imported.

Seen on a fulu-to-gloas transition devnet:

• With slots 160 and 161 (the first two gloas slots) empty, every ethlambda node refused every client's attestations for them.
• A node whose head stayed pre-fork through an execution-client outage refused all of the fork's first epoch.

Fix

• get_domain_from_schedule(config, state, domain_type, epoch) reads the fork version from the config's schedule at epoch. It is used for:
  • the subnet attestation signature;
  • the aggregate's selection proof, its aggregator signature and the aggregate signature itself;
  • the pool endpoint.
• is_valid_indexed_attestation_with_domain (phase0 and electra) is the existing check with the domain passed in. is_valid_indexed_attestation is now a thin wrapper over it, so the state transition is unchanged.
• The state transition keeps get_domain, since it advances the state to the slot first and there the two agree. Operation gossip (slashings) keeps it too, since the spec checks those against the head state as-is.

Tests

• the_schedule_names_the_new_fork_before_the_state_reaches_it: the two readings agree on the old fork's epochs and differ at the new fork's first.
• a_vote_across_a_fork_boundary_verifies_under_the_new_forks_version (gossip), an_attestation_in_a_forks_first_empty_slot_is_published and an_aggregate_in_a_forks_first_empty_slot_is_published (API): an electra head, a fulu vote. Each fails without the fix: BadSignature, or a 400.
• The rpc tests now sign votes under the schedule, as a validator client does, rather than under the test states' all-zero fork.

cargo test --workspace --profile release-fast --lib --bins passes, as do the 96 gossip spec vectors (--features beacon-spec-tests -- gossip/) and make lint.

[github-actions]

🤖 Claude Code Review

Review: PR 673, check vote signatures under the fork schedule's domain

Overall: The fix is correct and well scoped. I read the diff only and did not build or run the tests.

Why the approach is sound

• get_domain_from_schedule (accessors.rs) takes the fork version from config.fork_version(config.fork_at_epoch(epoch)). It still uses state.genesis_validators_root(), which is fixed across forks.
• The old code read state.fork against a state that had not been advanced to the target epoch. That state still carries the previous fork's version, so every correctly signed vote in a new fork's empty first slots was rejected.
• The old behaviour also produced a gossip REJECT, which penalises the relaying peer. That is a real peer-scoring and liveness problem, and the PR removes it.
• The state transition keeps get_domain, and the doc comment explains why. That is right, because process_slots advances the state first, so the two readings agree there.

Details that look right

• The refactor of is_valid_indexed_attestation into a wrapper plus _with_domain leaves existing callers unchanged. The wrapper builds the domain exactly as before, so the state-transition path should behave the same.
• In aggregate.rs, attester_domain is computed once and used for both the Phase0 and Electra arms.
• In attestation.rs, store.config() moves up above the signature check, and the later duplicate is removed.
• The tests are good. They cover the helper at the boundary, with before == get_domain and at-fork != get_domain. They also cover gossip subnet attestations, the pool endpoint, and the aggregate's three signatures. The tests sign under the new fork's version while the state is the previous fork's, so they exercise the bug.

Minor, non-blocking notes

1. The security trade-off is worth a sentence in the PR description. For an attestation whose target epoch is in the future relative to the state, the committee and shuffling still come from the stale state. The epoch_committees lookup is outside this diff, and I did not check that it is safe across a fork boundary. It is unaffected by this change, so this is not a regression.
2. get_domain_from_schedule takes &Config and &BeaconState only to read genesis_validators_root. Passing the root directly would avoid needing a state. This is purely stylistic and the current signature mirrors get_domain, which is fine.
3. In pool.rs, config is used in validate. I could not confirm in the truncated diff that it is bound before the signature check, but it looks like it already was.

No bugs or security issues found. LGTM.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Kimi Code Review

I'll review this PR which fixes fork-boundary attestation signature verification by using the config's fork schedule instead of the state's current fork when determining signing domains.

Overall Assessment

This is a well-structured bugfix for a legitimate consensus issue: when a fork's first slots are empty, attestations are verified against a state that hasn't been advanced to the new fork, causing signature verification failures. The fix introduces get_domain_from_schedule to use the config's fork schedule at the target epoch rather than state.fork.

---

Issues Found

1. Critical: get_domain_from_schedule ignores state.fork entirely — potential spec deviation in edge case

File: crates/blockchain/state_transition/src/beacon/helpers/accessors.rs, lines 427-444

pub fn get_domain_from_schedule(
    config: &Config,
    state: &BeaconState,
    domain_type: DomainType,
    epoch: Epoch,
) -> Domain {
    let fork_version = config.fork_version(config.fork_at_epoch(epoch));
    compute_domain(domain_type, fork_version, state.genesis_validators_root())
}

Problem: This always uses config.fork_at_epoch(epoch), but the consensus spec's get_domain uses state.fork.current_version when epoch >= state.fork.epoch, and previous_version otherwise. The config schedule and state fork should agree, but if they don't (e.g., state from a different chain, corrupted state, or config mismatch), this silently uses the config.

Question: Is there any case where state.fork and config.fork_at_epoch(epoch) could diverge legitimately? The comment says "the state transition keeps get_domain" — but what about block processing where the state has been advanced? The function is only used in gossip/API paths per the PR, but this is a sharp edge.

Suggestion: Add a debug assertion or comment clarifying this invariant. Consider whether state.fork should be checked for consistency with config in debug builds.

---

2. Medium: get_domain still used in is_valid_indexed_attestation — state transition paths may still be vulnerable

File: crates/blockchain/state_transition/src/beacon/helpers/attestation.rs, lines 78-86 and electra.rs, lines 275-283

pub fn is_valid_indexed_attestation(
    state: &BeaconState,
    indexed_attestation: &IndexedAttestation,
) -> bool {
    let domain = get_domain(
        state,
        constants::DOMAIN_BEACON_ATTESTER,
        Some(indexed_attestation.data.target.epoch),
    );
    is_valid_indexed_attestation_with_domain(state, indexed_attestation, domain)
}

Problem: The old is_valid_indexed_attestation is preserved for state transition use. However, if this is ever called from gossip/API paths (or if there's a code path where the state hasn't been advanced), the bug persists.

Verification needed: Audit all call sites. The PR changes two call sites in aggregate.rs to use with_domain, but are there others? A quick grep suggests process_attestation in state transition might use these.

Suggestion: Add #[doc(alias = "state_transition_only")] or rename to is_valid_indexed_attestation_for_processed_state to make the intended use explicit and prevent misuse.

---

3. Minor: Test in attestation.rs uses hardcoded Root::repeat_byte(7) without documenting significance

File: crates/blockchain/state_transition/src/beacon/gossip/attestation.rs, lines 453-454

let block_root = Root::repeat_byte(7);
// ...
root: block_root,  // used for both beacon_block_root and target.root

Problem: The target checkpoint root equals the beacon block root, which is unusual — normally target root is the block root at the epoch boundary. For this test's purposes (first slot of epoch, no block yet), this is correct since the head is its own checkpoint, but it's subtle.

Suggestion: Add a comment explaining why target.root == beacon_block_root is correct here (first slot of epoch, head hasn't changed).

---

4. Minor: fork_boundary_fixture in pool.rs has confusing epoch arithmetic

File: crates/net/rpc/src/beacon/pool.rs, lines 433-449

fn fork_boundary_fixture() -> Fixture {
    let mut state = with_signing_validators_at(ForkName::Electra, 64);
    let (probe, _) = beacon_store_at(state.clone());
    let wall_epoch = compute_epoch_at_slot(crate::beacon::node::wall_slot(&probe));
    let config = Config::mainnet().with_fork_epoch(ForkName::Fulu, wall_epoch);
    *state.slot_mut() = compute_start_slot_at_epoch(wall_epoch - 1);

Problem: wall_epoch is derived from wall_slot(&probe) which uses beacon_store_at(state.clone()) — but state was initialized at slot 0 (Electra from genesis). So wall_slot returns something based on current time, making this test non-deterministic!

Wait — let me re-check. beacon_store_at uses state.slot() which is 0 initially, but wall_slot computes from genesis time + current time. This means wall_epoch varies based on when the test runs!

Actually, looking more carefully: with_signing_validators_at(ForkName::Electra, 64) — what slot does this create? If it's slot 0, then wall_slot is based on real time, making wall_epoch non-deterministic. But the test seems to expect a specific structure.

Verification needed: Does with_signing_validators_at set a specific slot? Does wall_slot use a fixed genesis time in tests?

Looking at test_utils.rs: GENESIS_TIME is fixed at 1_000, and beacon_store_at uses 1_606_824_023. The wall slot calculation: genesis_time + SECONDS_PER_SLOT * slot = current_time. With genesis time in 2020, wall slot would be huge. But beacon_store_at in pool.rs uses 1_606_824_023 not 1_000.

Wait — beacon_store_at (in test_utils.rs) uses 1_606_824_023 as genesis time. store_with_config in gossip uses 1_000. These are inconsistent.

Actually re-reading: pool.rs test uses beacon_store_at which uses Config::mainnet() and genesis time 1_606_824_023. The wall_slot will be based on current system time minus that genesis time. This is definitely non-deterministic.

This is a pre-existing issue, not introduced by this PR, but the new test fork_boundary_fixture inherits it.

Suggestion: The test should use a deterministic time source or mock wall_slot. At minimum, document this dependency. Consider if the test could fail in CI due to timing.

---

5. Style: Inconsistent import of get_domain_from_schedule

File: crates/net/rpc/src/beacon/validator_client_tests.rs, line 17

use ethlambda_state_transition::beacon::helpers::{
    accessors::{get_domain, get_domain_from_schedule},

Problem: get_domain is imported but only get_domain_from_schedule is used (via scheduled_domain). get_domain appears unused in this file now.

Suggestion: Remove unused import get_domain.

---

6. Minor: scheduled_domain helper duplicates config construction

File: crates/net/rpc/src/beacon/validator_client_tests.rs, lines 248-255

fn scheduled_domain(
    state: &BeaconState,
    domain_type: ethlambda_types::beacon::primitives::DomainType,
    epoch: ethlambda_types::beacon::primitives::Epoch,
) -> ethlambda_types::beacon::primitives::Domain {
    let config = ethlambda_types::beacon::config::Config::mainnet();
    get_domain_from_schedule(&config, state, domain_type, epoch)
}

Problem: Constructs Config::mainnet() on every call. This is cheap but unnecessary. More importantly, it hardcodes mainnet config — if tests ever use a different config, this will silently be wrong.

Suggestion: Accept config: &Config parameter, or document why mainnet is always correct here.

---

Positive Observations

1. Good test coverage: The PR adds targeted tests for the fork boundary case in both gossip attestation and RPC pool paths.

2. Clear documentation: Comments in get_domain_from_schedule and call sites explain why the schedule is used, not just what.

3. Minimal API surface: The with_domain variants preserve existing functions for state transition use, avoiding a flag day migration.

4. Correct domain types used: Verified that DOMAIN_BEACON_ATTESTER, DOMAIN_SELECTION_PROOF, and DOMAIN_AGGREGATE_AND_PROOF all use get_domain_from_schedule in gossip paths.

---

Summary

Priority	Item	File	Line
Medium	Verify no other is_valid_indexed_attestation call sites need fixing	attestation.rs, electra.rs	78, 275
Medium	Address non-deterministic wall_epoch in test	pool.rs	438
Low	Remove unused get_domain import	validator_client_tests.rs	17
Low	Pass config to scheduled_domain	validator_client_tests.rs	248
Low	Add assertion/comment on config/state fork consistency	accessors.rs	427
Low	Document target.root == beacon_block_root in test	attestation.rs	453

The core fix is correct and well-justified. The main risk is ensuring all call sites are updated and that the state transition paths don't accidentally use the wrong variant.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

[github-actions]

🤖 Codex Code Review

Looks good overall. The core change fixes a real consensus bug: gossip signature checks now use the fork schedule for target_epoch instead of the voted block state’s possibly stale fork, which is the right behavior around empty fork-boundary slots.

• No correctness or security issues stood out in the consensus-critical path. In particular, crates/blockchain/state_transition/src/beacon/gossip/aggregate.rs:357, crates/blockchain/state_transition/src/beacon/gossip/aggregate.rs:365, crates/blockchain/state_transition/src/beacon/gossip/aggregate.rs:410, and crates/blockchain/state_transition/src/beacon/gossip/attestation.rs:182 all consistently switch to schedule-derived domains, and the new helper in crates/blockchain/state_transition/src/beacon/helpers/accessors.rs:425 is narrowly scoped and correct.
• I also like that aggregate signature verification now threads the explicit domain into indexed-attestation validation rather than duplicating logic. That keeps state-transition behavior unchanged while fixing gossip-only semantics.
• Performance is fine: this adds only cheap domain computation and does not increase committee derivation or signature work.
• Error handling remains conservative and safe; the paths still reject/ignore without introducing panics in adversarial flows.

Small maintainability nit:

• crates/net/rpc/src/beacon/validator_client_tests.rs:14 still imports get_domain, but the test now uses scheduled_domain/get_domain_from_schedule. If get_domain is now unused in this file, drop the import to keep warnings clean.

If you want, I can also do a second pass specifically on the new tests around fork-boundary gossip behavior.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt