Enforce single AttestationData per block (leanSpec PR #510)

[pablodeymo]

Motivation

Currently, build_block can produce multiple attestation entries sharing the same AttestationData -- each backed by a different aggregated signature proof. This happens when extend_proofs_greedily selects multiple proofs for the same vote data (e.g., from different aggregation intervals covering non-overlapping validator sets).

leanSpec PR #510 introduces a new protocol invariant: each unique AttestationData must appear at most once per block. Multiple proofs for the same vote are compacted into a single entry, reducing block size and simplifying downstream processing. The invariant is enforced on both the building and validation sides.

Description

Block validation (on_block_core)

A cheap O(n) uniqueness check is inserted before signature verification and state transition (the two most expensive steps). If a block contains duplicate AttestationData entries, it is rejected with a new StoreError::DuplicateAttestationData error. This uses HashSet<&AttestationData> -- possible because AttestationData already derives Hash + Eq.

Block building (build_block)

After the existing greedy proof selection loop, a new compact_attestations step groups all (attestation, proof) pairs by their AttestationData and merges duplicates:

• Single entry per data: kept as-is (fast path).
• Multiple entries with empty proofs (skip-sig / devnet mode): participant bitfields are unioned via union_aggregation_bits, producing a single AggregatedSignatureProof::empty(merged_bits).
• Multiple entries with real proofs (production with XMSS aggregation): the proof covering the most participants is kept. Full merge would require recursive proof aggregation, which lean-multisig does not yet support (the spec itself notes "The API supports recursive aggregation but the bindings currently do not").

The intermediate block built inside the justification-check loop is not compacted -- it only tests whether justification advances, and vote counting is identical regardless of entry layout.

New helpers

Function	Purpose
union_aggregation_bits(a, b)	Bitwise OR of two AggregationBits bitfields
compact_attestations(atts, proofs)	Groups by AttestationData, merges duplicates, preserves first-occurrence order

Future work

When lean-multisig adds recursive aggregation support, the else branch in compact_attestations (real proofs) can be upgraded to cryptographically merge all proofs instead of keeping only the best one. This will recover the small amount of validator coverage currently lost when multiple real proofs exist for the same AttestationData.

Test plan

• compact_attestations_no_duplicates -- distinct data passes through unchanged
• compact_attestations_merges_empty_proofs -- two entries with same data + empty proofs merge into one with unioned participants covering all validators
• compact_attestations_real_proofs_keeps_best -- two entries with same data + real proofs keeps the one with more participants
• compact_attestations_preserves_order -- multiple data entries (some duplicated) output in first-occurrence order
• on_block_rejects_duplicate_attestation_data -- block with duplicate entries returns DuplicateAttestationData error via on_block_without_verification
• All 18 existing blockchain lib tests still pass
• cargo fmt --all -- --check clean
• cargo clippy -p ethlambda-blockchain -- -D warnings clean
• Spec test fixtures update (deferred until leanSpec submodule includes PR #510)

[github-actions]

🤖 Codex Code Review

Findings

1. High: the new compaction step changes consensus semantics by dropping valid votes when the same AttestationData has multiple real proofs with disjoint participants. extend_proofs_greedily was explicitly selecting multiple proofs for one attestation message to maximize validator coverage, and process_attestations unions those bits when tallying votes toward justification/finalization. The new compact_attestations path keeps only the single “best” real proof, so the final block can have less voting weight than the candidate block used to decide current_justified. That can change justification/finalization outcomes and produce objectively different chain behavior. See store.rs#L1023, store.rs#L1169, and lib.rs#L269.

2. High: block import now rejects any repeated AttestationData, but repeated entries are part of the current wire/storage model rather than obviously invalid input. The payload buffer stores multiple proofs per data_root, only deduplicating exact participant sets, and the old block builder emitted one aggregated attestation per selected proof. The state transition also accepts repeated entries and merges their validator votes. Rejecting them in on_block_core therefore risks rejecting blocks produced by older versions or other peers using the existing semantics, which is a consensus-split risk. See store.rs#L522, storage.rs#L126, and lib.rs#L280.

I did not run tests locally: cargo test is blocked here because the sandboxed toolchain tries to write under read-only /home/runner/.rustup.

---

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

[github-actions]

🤖 Kimi Code Review

Review of PR #258 for ethlambda. Overall, this is a solid implementation of the "unique AttestationData per block" consensus rule with proper compaction logic for block production.

Critical Issues

None found. The validation logic correctly enforces consensus rules before state transition.

Correctness & Logic

crates/blockchain/src/store.rs:1014-1023
In compact_attestations, when multiple attestations share the same AttestationData but have non-empty proofs, the code keeps only the largest one and discards others. This creates valid blocks but loses attestations (reducing inclusion rewards and chain security weight).

Consider adding a comment explaining this trade-off, or implementing subset merging (if one bitfield is a subset of another, keep the superset):

// TODO: If bitfields are disjoint, we could keep both by concatenating proof_data
// when lean-multisig supports proof aggregation.

crates/blockchain/src/store.rs:935
The union_aggregation_bits function performs bit-by-bit iteration. For large validator sets (e.g., 64K validators), this is O(n) per union. Since this is only used in block production (not hot path validation),

---

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

[github-actions]

🤖 Claude Code Review

Now I have enough context. Here's my review:

---

Review: PR #258 — Enforce single AttestationData per block

The overall approach is sound and well-structured. The validation/building split is correct, and the tests cover the happy paths well. A few issues worth addressing:

---

Bug: Mixed empty/real proofs selects wrong proof

In compact_attestations (line ~1004–1012), when a group has some empty proofs and some real proofs, all_empty is false and the code falls through to the "keep best by participant count" path. But participant_indices().count() counts set bits in aggregation_bits regardless of whether the proof is real or empty — so an empty proof covering 5 validators beats a real proof covering 3 validators, discarding the only cryptographically valid proof in the group.

// Current: may select empty proof over real proof
let best_idx = indices
    .iter()
    .copied()
    .max_by_key(|&i| proofs[i].participant_indices().count())
    .expect("group is non-empty");

Fix: prefer real proofs, tiebreak by participant count:

.max_by_key(|&i| (!proofs[i].proof_data.is_empty(), proofs[i].participant_indices().count()))

There's no test for the mixed case. This scenario may not arise in practice today (all proofs in a given run are either all-real or all-empty), but the logic is silently wrong and becomes a latent consensus bug.

---

Silent coverage loss with real proofs

When multiple real proofs share the same AttestationData, all but the "best" are discarded. This silently loses valid attestation weight from the block — every dropped proof represents validators whose votes won't count toward justification/finalization for this slot.

The comment acknowledges "Full merge deferred until lean-multisig supports recursive aggregation", which is a fair constraint. But the behavior is only documented in the PR description, not in the code comment. The inline comment at line ~1004 should explicitly state the consequence:

// Cannot merge real proofs yet (lean-multisig doesn't support recursive aggregation).
// Keeping only the best-coverage proof; the other proofs' validator coverage is lost
// for this block. TODO: merge when lean-multisig supports recursive aggregation.

---

expect panics in union_aggregation_bits

Lines ~932–941: AggregationBits::with_length(max_len).expect("union exceeds bitlist capacity") will panic if max_len exceeds the bitlist's capacity limit. For blockchain code, unhandled panics in consensus-critical paths are high-severity. The error message also reads backwards (it describes the error condition, not what the .expect guards against).

compact_attestations should propagate this as a Result, or the function should validate that max_len is within bounds before calling.

---

Double clone per new unique entry in grouping logic

Lines ~964–970:

groups
    .entry(att.data.clone())   // clone for HashMap key
    .or_insert_with(|| {
        order.push(att.data.clone());  // second clone for `order`
        Vec::new()
    })

att.data is cloned twice for each new unique AttestationData. This can be avoided by using entry_ref (nightly) or by cloning into a local variable first. Minor, but AttestationData may not be trivially cheap to clone. Not a blocking issue.

---

Validation check placement and cost

The HashSet<&AttestationData> dedup check (line ~522) allocates a hash set and hashes every AttestationData in the block. This is O(n) and cheap, and its placement — after parent state check, before sig verification — is appropriate.

One edge case: for a block with zero attestations, unique_count == attestations.len() == 0, so the check passes trivially. Correct behavior.

---

Test: on_block_rejects_duplicate_attestation_data uses state_root: H256::ZERO

The test block at slot 1 has state_root: H256::ZERO (line ~1668), which won't match the real computed state root. The test relies on rejection happening before state root validation — which is true given the check is placed before the state transition. This dependency on check ordering is implicit; a comment noting this would help future-proof the test.

---

Minor: debug_assert_eq! vs assert_eq! for length invariant

compact_attestations uses debug_assert_eq!(attestations.len(), proofs.len()) (line ~956). Since mismatched lengths would cause incorrect behavior (not just performance issues), this invariant arguably deserves a runtime check with a proper error return, especially since this is called from build_block after complex multi-step logic.

---

Summary

Severity	Item
Medium bug	Mixed empty/real proofs: empty proof may win over real proof
High design gap	Silent validator coverage loss when discarding real proofs (needs better inline docs)
Medium	expect panics in union_aggregation_bits — no graceful error path
Low	Double clone per new unique entry
Low	Missing test for mixed empty/real proof case
Nit	debug_assert_eq for a load-bearing invariant

---

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

[greptile-apps]

Greptile Summary

This PR enforces the new leanSpec protocol invariant that each unique AttestationData must appear at most once per block (leanSpec PR #510). It adds a cheap O(n) duplicate check in on_block_core (before expensive signature verification), and introduces a compact_attestations step in build_block that merges duplicate entries produced by extend_proofs_greedily.

The implementation is logically sound and well-tested for the expected cases, with one subtle edge noted below.

Confidence Score: 4/5

Safe to merge with one latent correctness edge case worth addressing before recursive aggregation lands.

Both findings are P2 in practice (skip-sig and real-proof modes are mutually exclusive today; spec-transition enforcement is explicitly deferred), but the mixed-proof participant_indices issue is a future correctness trap that could silently produce blocks with cryptographically invalid proofs once the modes interact.

crates/blockchain/src/store.rs — specifically the else branch of compact_attestations (lines 1004-1013) and the intermediate process_block call in the justification loop (lines 1144-1166).

Important Files Changed

Filename	Overview
crates/blockchain/src/store.rs	Adds O(n) duplicate-AttestationData check in on_block_core and compact_attestations helper in build_block; five new unit tests cover the main paths; one latent mixed-proof edge case in the real-proofs branch.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[build_block called] --> B[extend_proofs_greedily loop]
    B --> C{justification advanced?}
    C -- yes --> B
    C -- no --> D[compact_attestations]
    D --> E{any duplicate AttestationData?}
    E -- no --> F[fast path: return as-is]
    E -- yes --> G{all proofs empty?}
    G -- yes --> H[union aggregation_bits → single empty proof]
    G -- no --> I[keep proof with most participant_indices]
    H --> J[final build_block + process_block]
    I --> J
    F --> J
    J --> K[on_block_core receives block]
    K --> L[HashSet uniqueness check]
    L --> M{duplicates?}
    M -- yes --> N[Err: DuplicateAttestationData]
    M -- no --> O[sig verification + state transition]
    O --> P[block accepted]

Loading

Comments Outside Diff (1)

1. crates/blockchain/src/store.rs, line 1144-1166 (link)

   Uncompacted intermediate block may conflict with future state-transition enforcement

   The aggregated_attestations passed to the intermediate process_block call inside the justification-check loop can still contain duplicate AttestationData entries (produced by extend_proofs_greedily). Currently only on_block_core enforces the uniqueness invariant, so this is harmless. But when the leanSpec PR #510 state-transition fixtures land and process_block itself begins rejecting duplicates, these intermediate calls will start returning an error and the loop will break unexpectedly. The PR description acknowledges the deferred fixture update, but neither the comment on line 1169 nor the loop body flags this forward-compatibility gap.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: crates/blockchain/src/store.rs
   Line: 1144-1166
   
   Comment:
   **Uncompacted intermediate block may conflict with future state-transition enforcement**
   
   The `aggregated_attestations` passed to the intermediate `process_block` call inside the justification-check loop can still contain duplicate `AttestationData` entries (produced by `extend_proofs_greedily`). Currently only `on_block_core` enforces the uniqueness invariant, so this is harmless. But when the leanSpec PR #510 state-transition fixtures land and `process_block` itself begins rejecting duplicates, these intermediate calls will start returning an error and the loop will break unexpectedly. The PR description acknowledges the deferred fixture update, but neither the comment on line 1169 nor the loop body flags this forward-compatibility gap.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 1144-1166

Comment:
**Uncompacted intermediate block may conflict with future state-transition enforcement**

The `aggregated_attestations` passed to the intermediate `process_block` call inside the justification-check loop can still contain duplicate `AttestationData` entries (produced by `extend_proofs_greedily`). Currently only `on_block_core` enforces the uniqueness invariant, so this is harmless. But when the leanSpec PR #510 state-transition fixtures land and `process_block` itself begins rejecting duplicates, these intermediate calls will start returning an error and the loop will break unexpectedly. The PR description acknowledges the deferred fixture update, but neither the comment on line 1169 nor the loop body flags this forward-compatibility gap.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 1004-1013

Comment:
**`participant_indices()` called on empty proofs in mixed-proof groups**

In the `else` branch (real-proofs path), `max_by_key(|&i| proofs[i].participant_indices().count())` iterates over *all* indices in the group, including any that have `proof_data.is_empty()`. `participant_indices()` counts bits in the `participants` bitfield regardless of whether `proof_data` is present, so an empty proof with a large bitfield could win the comparison and be selected as "best". In a fully mixed group this would embed a proof with no cryptographic backing into the final block, which would then fail signature verification at the receiving node. In practice skip-sig mode and real-proof mode are mutually exclusive deployments, so this path won't trigger today — but it is a latent correctness trap worth guarding explicitly (e.g. by restricting the `max_by_key` iteration to indices where `!proofs[i].proof_data.is_empty()`).

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "Enforce single AttestationData per block..." | Re-trigger Greptile}

[greptile-apps]

participant_indices() called on empty proofs in mixed-proof groups

In the else branch (real-proofs path), max_by_key(|&i| proofs[i].participant_indices().count()) iterates over all indices in the group, including any that have proof_data.is_empty(). participant_indices() counts bits in the participants bitfield regardless of whether proof_data is present, so an empty proof with a large bitfield could win the comparison and be selected as "best". In a fully mixed group this would embed a proof with no cryptographic backing into the final block, which would then fail signature verification at the receiving node. In practice skip-sig mode and real-proof mode are mutually exclusive deployments, so this path won't trigger today — but it is a latent correctness trap worth guarding explicitly (e.g. by restricting the max_by_key iteration to indices where !proofs[i].proof_data.is_empty()).

Prompt To Fix With AI

This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 1004-1013

Comment:
**`participant_indices()` called on empty proofs in mixed-proof groups**

In the `else` branch (real-proofs path), `max_by_key(|&i| proofs[i].participant_indices().count())` iterates over *all* indices in the group, including any that have `proof_data.is_empty()`. `participant_indices()` counts bits in the `participants` bitfield regardless of whether `proof_data` is present, so an empty proof with a large bitfield could win the comparison and be selected as "best". In a fully mixed group this would embed a proof with no cryptographic backing into the final block, which would then fail signature verification at the receiving node. In practice skip-sig mode and real-proof mode are mutually exclusive deployments, so this path won't trigger today — but it is a latent correctness trap worth guarding explicitly (e.g. by restricting the `max_by_key` iteration to indices where `!proofs[i].proof_data.is_empty()`).

How can I resolve this? If you propose a fix, please make it concise.

[MegaRedHand]

This isn't in the spec from what I see. It seems unnecessary

[pablodeymo]

You're right, removed it in the latest commit along with its test. The merge path (union of participant bitfields) is now the only path for duplicate entries.