fix: spec-faithful aggregation_bits bounds handling in process_attestations

[MegaRedHand]

Problem (audit finding C1)

process_attestations incremented attestations_processed and inserted an empty votes entry into justifications before the oversized aggregation_bits length check. A skipped attestation therefore still added an all-false justification root that got serialized into the post-state: a cross-client state-divergence vector.

Changes

Commit 1 moves the bounds check above the entry() insert, so a skipped attestation leaves no trace in the post-state.

Commit 2 resolves the skip-vs-reject question by checking leanSpec at the pinned commit (f12000b, src/lean_spec/forks/lstar/spec.py::process_attestations). The spec has no bitlist length check at all. Its actual failure modes:

Case	Spec behavior	ethlambda before	ethlambda after
Set bit at index >= validator count	IndexError -> STF aborts -> block rejected	attestation skipped, block accepted	block rejected (Error::AggregationBitsOutOfBounds)
No bits set	AssertionError in to_validator_indices -> block rejected	processed as no-op + spurious all-false entry in post-state	block rejected (Error::EmptyAggregationBits)
Oversized bitlist, all set bits in range	processed normally	attestation skipped (post-state divergence)	processed normally

Zeam and Lantern also reject such blocks (per the pre-existing in-code comment), so the previous skip-while-accepting behavior could split ethlambda off from the rest of the network on a crafted block.

The checks sit after is_valid_vote, matching the spec's filter order: an attestation that fails the vote-validity filters is skipped without touching its bits, exactly as the spec continues before any bits access.

Testing

• cargo clippy --workspace --all-targets -- -D warnings clean.
• STF spectests currently fail to deserialize fixtures locally (missing field attestationPubkey) because the local leanSpec checkout drifted off the pin; pre-existing and unrelated. CI regenerates fixtures from the pin.

[github-actions]

🤖 Kimi Code Review

Overall Assessment: Correct and necessary consensus-layer fix. The change from skipping invalid attestations to rejecting the block prevents consensus splits with other clients (Zeam, Lantern) and aligns with the spec's crash behavior (IndexError/AssertionError).

Specific Feedback:

1. Performance (Minor)

• File: crates/blockchain/state_transition/src/lib.rs
• Lines: 289, 297-298
• The implementation makes two passes over the bitlist (count_ones() + find()). While attestations per block are bounded, consider fusing these into a single pass if this becomes a hotspot:

// Single-pass alternative (optional optimization)
let mut participant_count = 0;
let oob_bit = attestation.aggregation_bits.iter().enumerate().find(|&(i, b)| {
    if b {
        participant_count += 1;
        i >= validator_count
    } else {
        false
    }
});
if participant_count == 0 { return Err(Error::EmptyAggregationBits); }
if let Some((index, _)) = oob_bit { /* ... */ }

2. Unused Import Warning

• File: crates/blockchain/state_transition/src/lib.rs
• The removal of the warn! macro usage (old lines 288-292) may leave an unused import if warn was only used here. Verify and remove unused use tracing::warn; (or equivalent) to resolve compiler warnings.

3. Test Coverage

• File: crates/blockchain/state_transition/src/lib.rs
• Lines: 857-938
• The test aggregation_bits_bounds_follow_spec correctly covers:
  1. Oversized bitlist with in-range participants (acceptance)
  2. Out-of-bounds set bit (rejection)
  3. Empty aggregation bits (rejection)
• Consider adding a test case where validator_count > aggregation_bits.len() (valid but shorter bitlist) to ensure the range (validator_count..len) handles empty ranges correctly, though find on an empty range naturally returns None.

4. Code Clarity

• Lines: 296-304
• The OOB check is clear and correctly implements the spec's behavior: only set bits beyond the validator index trigger rejection, not merely an oversized bitlist structure.

Security/Consensus: Item 1 (two-pass iteration) is a performance note, not a security issue. The consensus logic is correct—returning Err immediately on invalid attestations matches the spec's failure mode and prevents the client from accepting blocks that other implementations reject.

---

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

[github-actions]

🤖 Codex Code Review

No findings.

The change in crates/blockchain/state_transition/src/lib.rs looks like a consensus-safety improvement: malformed aggregated attestations now hard-fail the block instead of being silently skipped, while still allowing oversized bitlists when every set bit is within the validator set, which matches the intended spec behavior described in the comments. The added coverage in crates/blockchain/state_transition/src/lib.rs exercises the important edge cases.

Residual risk: I could not run the targeted tests in this environment. cargo test is blocked here because Cargo/Rustup needs writable cache paths and network access for the leanVM dependency, both of which are unavailable.

---

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

[greptile-apps]

Greptile Summary

This PR fixes audit finding C1 by moving the aggregation_bits bounds check above the entry() insert and changing the semantics from skip to reject (matching leanSpec behavior). It also adds an EmptyAggregationBits guard that rejects blocks whose attestation has zero participants, and includes a targeted unit test covering all three spec-aligned cases.

• Audit fix: The spurious all-false justification entry that a skipped oversized attestation left in the post-state is eliminated; invalid attestations now return an error before touching justifications.
• Skip → reject: Blocks with a set bit at an out-of-range index now return Error::AggregationBitsOutOfBounds, and blocks with no set bits return Error::EmptyAggregationBits, matching leanSpec's IndexError/AssertionError crash-and-reject behavior (Zeam and Lantern agree).
• State safety: The store's on_block caller operates on a post_state copy of parent_state; a STF error causes post_state to be dropped, so any partial attestation mutations before the failing attestation are safely discarded.

Confidence Score: 5/5

Safe to merge; the fix correctly eliminates the spurious justification entry and aligns block-rejection semantics with leanSpec and peer implementations.

The two new checks fire before any state is touched, the store wraps the STF in a copy-on-write pattern so partial mutations on error are safely discarded, the spec table in the PR description is accurately reflected in both the implementation and the new unit test, and no unintended regressions were identified.

No files require special attention.

Important Files Changed

Filename	Overview
crates/blockchain/state_transition/src/lib.rs	Moves OOB/empty checks before entry() insert, changes skip→reject semantics with two new error variants, and adds a comprehensive unit test covering all three spec-aligned cases; logic is correct and state handling on error is safe.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[for attestation in attestations] --> B{is_valid_vote?}
    B -- no --> A
    B -- yes --> C{count_ones == 0?}
    C -- yes --> ERR1[Err: EmptyAggregationBits\nblock rejected]
    C -- no --> D{any set bit at index\n>= validator_count?}
    D -- yes --> ERR2[Err: AggregationBitsOutOfBounds\nblock rejected]
    D -- no --> E[attestations_processed += 1\njustifications.entry insert\nvote bits marked]
    E --> F{supermajority?}
    F -- yes --> G[update latest_justified\nset justified_slots\ntry_finalize]
    G --> A
    F -- no --> A
    A -- done --> H[serialize_justifications\nmetrics update\nOk]

Loading

_{Reviews (1): Last reviewed commit: "fix: reject blocks with out-of-bounds or..." | Re-trigger Greptile}

[github-actions]

🤖 Claude Code Review

I have enough context to write a thorough review.

---

Review: fix: spec-faithful aggregation_bits bounds handling in process_attestations

Summary

This PR fixes a genuine spec-divergence bug (audit finding C1) and deserves approval. The core logic is correct, the test coverage is solid, and the reasoning matches both the spec and peer implementations (Zeam, Lantern). A few observations below.

---

Correctness

The main fix is correct. Moving the bounds checks above the justifications.entry() insert (store.rs:308) ensures a skipped/rejected attestation leaves no trace in the post-state. The old code polluted the justifications map with an all-false entry even for rejected attestations, which was the state-divergence vector.

Behavioral change for "oversized, all in-range" case is spec-faithful. The old len > validator_count check would skip the attestation entirely (drop it from the state), causing post-state divergence versus any client that processes it. The new code passes these through, matching spec behavior where the list length is never explicitly checked.

OOB detection is precise:

let oob_bit = (validator_count..attestation.aggregation_bits.len())
    .find(|&i| attestation.aggregation_bits.get(i) == Some(true));

The range (validator_count..len) is empty when validator_count >= len, so exact-size and smaller bitlists take zero iterations. Only set bits in the tail are flagged — not the presence of a tail itself.

Vote-loop safety after OOB check passes (lib.rs:313-321): votes.len() == validator_count caps the iter_mut() independent of the .take(attestation.aggregation_bits.len()). The .take() is now effectively unreachable for its original purpose (the OOB guard makes it redundant) — a minor cleanup opportunity, not a bug.

---

Potential Issues

Minor: .take(aggregation_bits.len()) is misleading after the OOB guard

lib.rs:316:

.take(attestation.aggregation_bits.len())

After the OOB check guarantees all set bits are in [0, validator_count), iterating votes.iter_mut() (length validator_count) already bounds this. The .take() no longer serves a defensive role. Consider changing to .take(validator_count) (or removing it entirely) to make the invariant explicit at the loop site. Not a correctness issue since the iterator never exceeds validator_count anyway.

Minor: Empty-bits check fires before OOB check for count_ones()==0 + oversized bitlist

An attestation with zero set bits and an oversized bitlist returns EmptyAggregationBits, not AggregationBitsOutOfBounds. This is fine — both reject the block — but it's worth noting the precedence is set by check ordering, not spec spec order (which would crash on to_validator_indices before any index check). The behavior is equivalent.

---

Test Coverage

The three-case test aggregation_bits_bounds_follow_spec (lib.rs:866) covers all rows in the PR table precisely:

• Oversized with in-range participants → processed, target justified ✓
• Set bit at exactly NUM_VALIDATORS → AggregationBitsOutOfBounds { index: 4, validator_count: 4 } ✓
• All-zero bits → EmptyAggregationBits ✓

One gap worth considering: no test covers the happy path where aggregation_bits.len() == validator_count exactly within this new test function. The existing tests elsewhere cover this, but an assertion like make_atts(&[0, 1, 2], NUM_VALIDATORS) succeeding here would make the boundary explicit in the spec-conformance test. Low priority.

---

Error Messages

Both error variants have clear, informative messages:

"aggregated attestation has no participants"
"aggregation bit set at index {index} beyond validator count {validator_count}"

The StoreError::StateTransitionFailed(#[from]) wrapper at store.rs:823 ensures these propagate with full context and are logged as warn! in process_or_pend_block (lib.rs:625-632). No silent swallowing.

---

Overall

The fix is correct, spec-faithful, and well-documented. The behavioral change (skip → reject for OOB bits) is the right call and is aligned with Zeam and Lantern. The .take() cleanup noted above is a cosmetic improvement that could be bundled in or left for a follow-up — it does not affect correctness.

---

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