fix(state-transition): bound distinct attestation data in the transition

[MegaRedHand]

What

Enforce the per-block cap on distinct AttestationData inside process_attestations, where leanSpec has it, in addition to the existing check at the import boundary in on_block.

Why

leanSpec puts the bound in the transition (state_transition.process_attestations) and fork_choice.on_block defers to it explicitly:

The transition itself bounds the distinct-data count. Only the wire-level duplicate prohibition lives here.

We only had it in on_block (store.rs), so state_transition() accepted an over-cap block and then failed on the state root instead. Two callers reach the transition without passing through on_block:

caller	before	after
build_block -> process_block	unbounded (the proposer-side clamp is the only guard)	fails loudly instead of publishing an unimportable block
spec-fixture replay / Hive state_transition/run	over-cap block accepted, then STATE_ROOT_MISMATCH	rejected with the cap error

The check goes first in process_attestations, ahead of the justification-bookkeeping guards, matching the spec's order when a block violates two rules at once.

The duplicate check in on_block stays

Deliberately, for now: it runs before verify_block_signatures, so an over-cap block is still rejected without paying for proof verification. Whether we collapse the two sites into one is a follow-up decision.

Testing

• cargo test --workspace --profile release-fast: green, no new tests added here.
• The behavior is covered by the existing fixture test_block_exceeding_distinct_attestation_data_cap_rejects_block, which asserts failure but not yet the reason. Cross-checked against test(spec): assert the rejection reason on expected-failure fixtures #547, which adds the reason assertion to the fixture runners: with both branches applied, that fixture passes for the right reason (TOO_MANY_ATTESTATION_DATA, previously STATE_ROOT_MISMATCH).

Whichever of the two lands first, the other's exhaustive From<&Error> for RejectionReason match makes the missing mapping arm a compile error, so the reason cannot be silently dropped.

(leanSpec #536)

[github-actions]

🤖 Kimi Code Review

This PR implements a DoS protection mechanism (leanSpec #536) to limit distinct AttestationData entries per block. The implementation is correct and well-tested.

Security & Correctness

• Line 262-268: Correctly bounds distinct attestation data before expensive per-validator tally allocation. The early return prevents memory exhaustion attacks.
• Line 265: Uses HashSet<&AttestationData> (references) rather than cloning data, which is memory-efficient.
• Line 267: Inclusive check (> MAX_ATTESTATIONS_DATA) matches the test assertion that "the bound is inclusive."

Code Quality

• Line 248-261: Excellent comment explaining why the check belongs in state transition rather than just import boundaries.
• Line 993, 1018: Test helpers are well-isolated and use descriptive names.
• Line 1037-1043: The ordering test (attestation_data_cap_precedes_the_justification_guards) correctly verifies fail-fast behavior for DoS prevention.

Minor Suggestions

• Line 262: Consider pre-allocating HashSet capacity to avoid reallocations:

  let distinct_attestation_data: HashSet<&AttestationData> =
      HashSet::with_capacity(MAX_ATTESTATIONS_DATA.min(attestations.len()));
  attestations.iter().map(|att| &att.data).collect_into(&mut distinct_attestation_data);

  (Or simply HashSet::with_capacity(attestations.len()) if the stdlib version supports it.)

• Line 973: The distinct_attestations helper could validate that count doesn't exceed the SSZ list limit before calling try_into() to provide a clearer panic message, though this is test-only code.

Consensus Safety
The validation logic correctly identifies that the workload is driven by distinct data count (which drives tally allocation) rather than total attestation count. The check precedes justification bookkeeping validation, matching the specified ordering requirements.

Approved with minor style nits.

---

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

[github-actions]

🤖 Claude Code Review

This is a clean, well-scoped fix. Review complete.

Review: PR 555 — bound distinct attestation data in the transition

Overall: Correct, minimal, and well-tested fix that closes a real gap: process_attestations (reachable from build_block and fixture replay, not just on_block) previously had no cap, so an over-cap block would fail later with a confusing STATE_ROOT_MISMATCH instead of a clear rejection reason.

Correctness

• The cap check (crates/blockchain/state_transition/src/lib.rs:257-264) correctly counts distinct AttestationData via HashSet<&AttestationData>, matching the spec's semantics (split aggregates over the same data count once). AttestationData already derives Eq/Hash (crates/common/types/src/attestation.rs:22), so this is sound.
• Placement ahead of the justification-bookkeeping guards mirrors the spec's check order and is also the cheaper thing to do first, since the later loop allocates a validator-sized vote vector per newly-seen target root — a legitimate reason to fail fast here, not just spec fidelity.
• Boundary is inclusive (> MAX_ATTESTATIONS_DATA), consistent with the existing on_block check in store.rs:653. Good consistency between the two enforcement points.
• Confirmed the proposer path can't regress: block_builder.rs:210 already clamps max_rounds to MAX_ATTESTATIONS_DATA, so this is purely a defense-in-depth addition for build_block/fixture-replay callers, not a new constraint on normal proposing.

Security / DoS

• AggregatedAttestations is SSZ-bounded to 4096 entries (crates/common/types/src/block.rs:257), so building the HashSet here is cheap even for a maximally-sized untrusted block — no new DoS surface.
• The on_block duplicate-vs-cap checks in store.rs still run pre-signature-verification, so a crafted over-cap block continues to be rejected without paying for proof verification. The PR description's note that on_block's exact-duplicate prohibition is intentionally distinct from the transition's distinct-count cap is accurate and consistent with the diff — no unintended duplication of semantics.

Tests

• The two new unit tests are good: one exercises both sides of the boundary (cap, cap+1) and the other confirms error-ordering relative to NoValidators. Test fixtures (state_for_cap_tests, distinct_attestations) are appropriately minimal for what they exercise.

Minor/nit

• Not a blocker, but the doc comment at lib.rs:248-256 is a bit long for what's a straightforward bounds check — though given the amount of cross-cutting context (on_block relationship, perf rationale, spec reference) it earns its length here.

No changes requested.

---

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

[greptile-apps]

Greptile Summary

Adds transition-level enforcement of the distinct attestation-data cap.

• Introduces a dedicated TooManyAttestationData transition error.
• Rejects over-cap blocks before justification bookkeeping and vote processing.
• Adds tests for the inclusive boundary and validation ordering.

Confidence Score: 5/5

The PR appears safe to merge, with no actionable correctness or security issues identified.

The transition now enforces the same distinct-attestation-data limit and inclusive boundary as block import, while existing callers isolate failed mutations in disposable cloned state.

Important Files Changed

Filename	Overview
crates/blockchain/state_transition/src/lib.rs	Correctly mirrors the existing import-boundary cap inside process_attestations, using identical distinctness semantics and boundary behavior, with focused regression tests.

_{Reviews (1): Last reviewed commit: "fix(state-transition): bound distinct at..." | Re-trigger Greptile}

[github-actions]

🤖 Codex Code Review

1. The STF still does not enforce the “at most one entry per AttestationData” invariant, so this only partially aligns state_transition with the import path. The new comment in lib.rs says the transition is the shared entry point, but duplicate rejection still exists only in store.rs. Direct STF callers such as the RPC test driver at test_driver.rs can still accept blocks that real import would reject if they contain repeated identical AttestationData but stay under the distinct-data cap. For consensus/test-fixture parity, I’d either move the duplicate check into process_attestations too, or narrow the comment to say this PR only ports the cap.

2. The new guard in lib.rs builds a full HashSet for the whole attestation list before checking a limit of 8. That weakens the “rejected cheaply” claim on adversarial input: STF-only callers still pay to hash and allocate for all entries up to the SSZ list bound. This should short-circuit after the 9th unique item, ideally with HashSet::with_capacity(MAX_ATTESTATIONS_DATA + 1) and an early return during iteration.

No other correctness or security problems stood out in this diff.

I could not run the targeted tests here because the toolchain wrapper tries to write under /home/runner/.rustup, which is read-only in this environment.

---

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