fix(fork-choice): reject attestation slot before head block slot

[MegaRedHand]

What

Ports the head-consistency guard from leanSpec PR #1020 into validate_attestation_data.

A gossip attestation's slot records when the head was observed, so it cannot precede the slot of the head block it claims to have seen. We did not enforce this lower bound: the wire slot field was untethered from any real block, free to be set arbitrarily relative to the observed head.

Why

Property	Risk without the guard
Soundness	An unbounded slot violates 3SF attestation semantics: the slot is meant to anchor when the head was seen.
Liveness (spec context)	In leanSpec a crafted near-2**64 slot overflowed the slot→interval constructor and crashed the node (remote DoS).

Note on the overflow half: ethlambda's time check already uses saturating_mul (data.slot.saturating_mul(INTERVALS_PER_SLOT)), so a near-u64::MAX slot saturates to u64::MAX and is rejected cleanly as too-far-in-future rather than panicking. The Rust port was never crashable the way the unchecked Python constructor was, so no change to the time-check arithmetic is needed; the spec's switch to slot-unit comparison is mathematically equivalent to the existing interval comparison. This PR adds the genuinely missing piece: the semantic lower bound.

Change

• New StoreError::AttestationSlotBeforeHead { attestation_slot, head_slot } rejection.
• Check data.slot < data.head.slot after the ancestry checks, before the time check (mirrors the spec's ordering).
• Two unit tests: a vote whose slot precedes its head (rejected as AttestationSlotBeforeHead), and a u64::MAX ceiling slot (rejected as AttestationTooFarInFuture, confirming no overflow).

Applies to both gossip paths via the shared validate_attestation_data (on_gossip_attestation and on_gossip_aggregated_attestation).

Testing

cargo test -p ethlambda-blockchain --lib validate_attestation   # 5 passed
cargo fmt --all && cargo clippy -p ethlambda-blockchain --lib    # clean

https://claude.ai/code/session_018pvnQJkom3DobK9oSbK42T

[github-actions]

🤖 Kimi Code Review

This is a well-crafted security fix. The change adds a critical consensus validation rule with proper defense-in-depth and test coverage.

Security & Correctness

1. Line 226-233 (store.rs): The new head-consistency check is correct. A validator cannot have observed a head block that was produced after their attestation slot (slot >= head.slot). This prevents attestations claiming to see future blocks.

2. Defense-in-depth: The placement of this check before the time calculation (line 235+) prevents arithmetic overflow when converting a maliciously crafted u64::MAX slot to time intervals. This is a smart hardening measure.

3. Spec compliance: This aligns with Ethereum consensus rules where the head vote must refer to a block seen by the attester at the time of attestation.

Code Quality

4. Error variant (store.rs:873-878): The new AttestationSlotBeforeHead error is well-structured with named fields for observability.

5. Documentation: The doc comment update correctly renumbers the validation rules (items 6 and 7) and explains the rationale clearly.

Testing

6. Line 1297-1330: The validate_attestation_rejects_slot_before_head test correctly exercises the boundary case where slot < head.slot.

7. Line 1333-1362: The validate_attestation_rejects_ceiling_slot_without_overflow test is excellent—it verifies that u64::MAX slots don't panic and are instead rejected as AttestationTooFarInFuture via saturating_mul (implied by the test comment).

Minor Note

8. Validation ordering: The check is correctly placed after ancestry validation (ensuring the head block actually exists in the chain) and before time-based validation. This ordering is sound.

Verdict: No changes required. This is a correct, secure implementation of leanSpec #1020 with comprehensive test coverage.

---

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

[github-actions]

🤖 Claude Code Review

PR #442 Review: fix(fork-choice): reject attestation slot before head block slot

Diff: 1 file changed, +94/-1 in crates/blockchain/src/store.rs

---

Overview

Ports the head-consistency guard from leanSpec PR #1020. Adds a semantic lower-bound check in validate_attestation_data: an attestation's slot must not precede the slot of the head block it references, since a validator cannot have observed a head that didn't yet exist when the vote was cast.

---

Correctness

The check is semantically correct.

if data.slot < data.head.slot {
    return Err(StoreError::AttestationSlotBeforeHead { ... });
}

An attestation at slot S represents what the attester observed at the start of slot S. A head at slot H > S could not have existed at that point. The boundary data.slot == data.head.slot is correctly permitted — a block published early in slot H can legitimately be the head for an attestation also at slot H.

Placement in the validation sequence is correct. The check comes after ancestry checks (which need head to be a known block and target to be an ancestor of head) and before the time check. This mirrors the spec's ordering and fails fast on the simpler structural violation before computing the interval arithmetic.

The new error variant is well-structured:

#[error("Attestation slot {attestation_slot} precedes head block slot {head_slot}")]
AttestationSlotBeforeHead { attestation_slot: u64, head_slot: u64 }

Field naming is consistent with AttestationTooFarInFuture.

---

Tests

Both tests are correct and meaningful.

validate_attestation_rejects_slot_before_head: Properly exercises the new path. The ancestry chain (genesis → b1 → b2 → b3) ensures all prior checks pass, so the test cleanly isolates the new guard. The matches! assertion with explicit field values is thorough.

validate_attestation_rejects_ceiling_slot_without_overflow: A good regression test for the existing saturating_mul protection. Worth having explicitly. The test correctly shows the new head-consistency check is transparent for slot = u64::MAX with a small head slot (the check passes since u64::MAX >= 2), and the time check handles rejection.

---

Minor Issues

1. Inline comment overstates the overflow protection benefit (line ~226):

// This lower bound also keeps the wire slot clear of the overflow edge that a
// crafted near-`u64::MAX` slot would otherwise probe in the time check below.

This is inaccurate. As the second test demonstrates, slot = u64::MAX with a small head slot passes the new check entirely (u64::MAX >= head.slot for any realistic head slot). The saturating_mul in the time check is the actual overflow defense. The comment implies this new check provides overflow protection that it does not, in practice, provide.

Suggest either removing this sentence or replacing it with something like: "(The time check's saturating_mul already prevents overflow on the interval computation; this guard provides the semantic bound.)"

2. Doc comment renumbering is correct — items 6 and 7 in the function-level doc block accurately reflect the new ordering.

---

Security Assessment

This is a genuine protocol-level fix. Without this guard, a peer could send an attestation whose slot is arbitrarily small relative to the claimed head — a structurally invalid vote that would pass all existing checks. While the Rust implementation was not vulnerable to the Python crash (due to saturating_mul), the semantic violation was real: an attacker could inject votes with slots detached from any plausible observation window, potentially influencing fork-choice state with logically incoherent data. The fix correctly closes this.

---

Summary

The logic is correct, the placement is right, tests are solid, and this is a meaningful security improvement. The only actionable finding is the misleading comment in Item 1 — the sentence attributing overflow protection to the new check should be removed or corrected.

---

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

[github-actions]

🤖 Codex Code Review

1. crates/blockchain/src/store.rs:226 adds the new data.slot >= data.head.slot invariant only on the gossip ingress path, but block-imported attestations still bypass it and can affect justification/finalization. on_block_core goes straight into state_transition, and state_transition::is_valid_vote never checks data.slot against data.head.slot at all; it only validates source/target plus chain membership (crates/blockchain/state_transition/src/lib.rs:365). That means a block can still carry an attestation whose vote slot predates its claimed head, and this client will count it toward latest_justified even though the PR now rejects the same message on gossip. For consensus code, that split is risky: either mirror the check in state transition / attestation_data_matches_chain, or explicitly scope this as a non-consensus mempool filter.

I did not find another obvious problem in the diff itself. I wasn’t able to run the targeted Rust tests because cargo/rustup tried to write under a read-only home path in this environment.

---

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

[greptile-apps]

Greptile Summary

This PR adds a semantic lower-bound guard to validate_attestation_data: an attestation's slot cannot precede the slot of the head block it claims to have observed. This closes the gap identified in leanSpec #1020 where the wire slot field was entirely unconstrained relative to the claimed head.

• Adds StoreError::AttestationSlotBeforeHead { attestation_slot, head_slot } and the check data.slot < data.head.slot, placed after ancestry validation and before the time check, matching the spec's ordering.
• Two new unit tests cover the rejection case (slot 2 attesting to a slot-3 head) and confirm that a u64::MAX slot still reaches and is rejected by the existing saturating_mul-guarded time check without overflow.

Confidence Score: 4/5

Safe to merge; the new check is a strict addition that only rejects previously-unchecked invalid attestations and does not affect any existing valid code path.

The guard logic, error variant, and both unit tests are correct. The one issue is an inline comment that incorrectly attributes overflow protection to the new check when saturating_mul is the actual safeguard — a reader relying on that comment could misunderstand the real defense-in-depth structure.

crates/blockchain/src/store.rs — specifically the inline comment on lines 228–229 of the head consistency check block.

Important Files Changed

Filename	Overview
crates/blockchain/src/store.rs	Adds a head-consistency lower-bound check (data.slot < data.head.slot) to validate_attestation_data, a new StoreError::AttestationSlotBeforeHead variant, and two unit tests. Logic and tests are correct; one inline comment overstates the overflow-protection benefit of the new check.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[validate_attestation_data] --> B[Availability Check\nblocks exist in store]
    B --> C[Topology Check\nsource ≤ target ≤ head slots]
    C --> D[Consistency Check\ncheckpoint slots match block slots]
    D --> E[Ancestry Check\nsource → target → head on one chain]
    E --> F{data.slot < data.head.slot?}
    F -- Yes --> G[❌ AttestationSlotBeforeHead]
    F -- No --> H{attestation_start_interval >\nstore.time + GOSSIP_DISPARITY?}
    H -- Yes --> I[❌ AttestationTooFarInFuture\nsaturating_mul prevents overflow]
    H -- No --> J[✅ Ok]

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[validate_attestation_data] --> B[Availability Check\nblocks exist in store]
    B --> C[Topology Check\nsource ≤ target ≤ head slots]
    C --> D[Consistency Check\ncheckpoint slots match block slots]
    D --> E[Ancestry Check\nsource → target → head on one chain]
    E --> F{data.slot < data.head.slot?}
    F -- Yes --> G[❌ AttestationSlotBeforeHead]
    F -- No --> H{attestation_start_interval >\nstore.time + GOSSIP_DISPARITY?}
    H -- Yes --> I[❌ AttestationTooFarInFuture\nsaturating_mul prevents overflow]
    H -- No --> J[✅ Ok]

Loading

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
crates/blockchain/src/store.rs:226-229
**Misleading overflow-protection rationale in comment**

The third sentence claims this check "keeps the wire slot clear of the overflow edge," implying it prevents a crafted near-`u64::MAX` slot from reaching the time check. That's not accurate: a gossip message can set `data.slot = u64::MAX` with a head block at any realistic slot (e.g. slot 2), which satisfies `data.slot >= data.head.slot` and passes straight through to `saturating_mul`. The second test in this PR (`validate_attestation_rejects_ceiling_slot_without_overflow`) explicitly demonstrates this path. The actual overflow protection comes entirely from the existing `saturating_mul`, not from this bound. The first two sentences are correct and self-contained; the third should be removed or rewritten to avoid misleading future readers into thinking this check provides overflow safety.

_{Reviews (1): Last reviewed commit: "fix(fork-choice): reject attestation slo..." | Re-trigger Greptile}