feat(blockchain): disable attestation compaction, keep single best proof

[MegaRedHand]

What

An alternative to PR #472 (the 2/3 supermajority cutoff). This PR disables compaction entirely: block building packs exactly one proof per AttestationData (the best one) and never merges proofs.

The two PRs are independent (both branch from main) so they can be compared/benchmarked; only one should be merged.

	#472 (cutoff)	This PR (best-proof)
Proofs per data	minimal to cross 2/3 (may merge a few)	exactly 1, never merge
compact_attestations	kept, fires less	removed
Per-block coverage	≥ 2/3 when reachable	one proof's worth
Justification of a fragmented target	this block if proofs cross 2/3	may take several slots

Why

The recursive XMSS merge (compact_attestations → aggregate_proofs) is the dominant cost of building blocks that carry attestations. Removing it makes the build path much cheaper, at the cost of per-block vote coverage: a target whose 2/3 is split across multiple gossip proofs may take more slots to justify, but each block is built with no recursive proof merge.

How

• select_best_proof replaces extend_proofs_greedily: picks the single proof covering the most validators not already counted for the target (prior_voters), so a proof that is mostly prior voters loses to an all-new one. Ranking iterates the proof slice in order, so ties are deterministic.
• It returns the voters it actually packed; select_attestations updates its projection from that set, keeping the projection consistent with the block.
• build_block drops the compact step. score_entry / pick_best_candidate no longer return the per-entry voter set.
• Removed: compact_attestations, union_aggregation_bits, and the aggregate_proofs import. (StoreError::SignatureAggregationFailed is retained; it is part of the public error enum.)
• The cross-data Type-2 block proof built in propose_block is unchanged — that is the block signature, not compaction.

Safety

Same basis as #472: fork-choice weight is gossip-sourced (not block-sourced), justification is binary at 2/3, and there are no inclusion rewards. Dropping voters from a block changes neither head selection nor finalization.

Tests

• select_best_proof unit tests: packs the single largest proof; ranks by net-new over prior_voters; packs nothing when no proof adds a new voter.
• build_block_packs_single_best_proof_and_justifies: end-to-end — a data entry with multiple proofs packs only the best one and still justifies the target through the real STF.
• make fmt, workspace clippy -D warnings, and cargo test -p ethlambda-blockchain --lib (38 pass) all green.

[github-actions]

🤖 Kimi Code Review

Overall Assessment: The PR correctly disables attestation compaction by selecting a single best proof per AttestationData instead of recursively aggregating multiple proofs. This is a valid performance optimization (avoiding expensive XMSS aggregation during block building) at the potential cost of including fewer total attestations. The code is safe, idiomatic, and well-tested.

Specific Comments:

1. Minor Performance: In select_best_proof (line ~529-540), participant_indices() is iterated twice for the winning proof—once during max_by_key and once when collecting covered_new. For large validator sets, consider collecting into a Vec during the max search or using a smallvec optimization, though this is not critical for consensus safety.

2. Determinism: The reliance on Iterator::max_by_key returning the last element in case of ties (line ~531) for deterministic selection is correct and well-documented in the comment ("ties resolve deterministically (last max wins)"). This ensures predictable block construction across nodes.

3. Heuristic Accuracy: In score_entry (lines ~375-385), the function calculates the union of new voters across all proofs to compute the score tier, but the block will only include one proof. This may overestimate the justification value of entries with fragmented proofs across many small attestations. However, as a greedy heuristic for prioritization, this is acceptable.

4. Memory Efficiency: Good cleanup of the compact_attestations function and union_aggregation_bits helper (previously lines 514-695), removing complex stateful grouping logic and the aggregate_proofs cryptographic dependency.

5. Test Coverage: The new tests (lines ~1082-1200) correctly verify:

   • Single proof selection (no merging)
   • Net-new voter prioritization over raw participant count
   • Defensive empty return when no voters are added
   • End-to-end justification still works with the best single proof

6. Metrics Consistency: The calls to metrics::inc_pq_sig_aggregated_signatures() (line ~553) now track individual proof selection rather than true aggregation. Consider renaming the metric or adding a comment that semantics changed from "number of aggregated signatures" to "number of selected signature batches," though this is minor.

Security/Liveness Note: Disabling compaction means blocks cannot combine multiple partial proofs for the same AttestationData to reach the 2/3 threshold. If the network produces many fragmented attestations, this could theoretically delay justification compared to the compacting version. The PR accepts this trade-off for performance; ensure this aligns with the protocol's liveness assumptions.

---

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

[github-actions]

🤖 Codex Code Review

1. High: the scorer still assumes union-of-all-proofs semantics, but the builder now packs only one proof. In score_entry it unions voters across every proof for a data_root and uses that union to decide crosses_2_3 / tier at block_builder.rs:389. But selection now emits a single proof via select_best_proof, and the projected justification/finalization state is still advanced from the inflated score at block_builder.rs:208. With fragmented support, this can mark a target justified/finalized in the projection even though the actual block body only carries a sub-threshold proof. Example: two disjoint 20/50 proofs score as 40 votes, but only one 20-vote proof is packed. That can make post_checkpoints wrong, admit descendants whose source is not actually justified, and skip votes the block still needed.

2. Medium: even when Item 1 does not cross the threshold, candidate ordering is now materially mispriced. EntryScore.new_voters is still computed from the union of all proofs for the entry at block_builder.rs:377, while the actual packed coverage comes from only the max single proof at block_builder.rs:533. Under the MAX_ATTESTATIONS_DATA cap, a fragmented candidate can outrank a denser one even though it contributes fewer real voters once packed. That is a liveness/performance regression in attestation packing and should be fixed by scoring against the exact proof that will be included, not the union of proofs that are no longer merged.

I did not find a memory-safety issue in the diff itself, but the consensus-layer scoring/projection mismatch above is a blocker. I did not run tests.

---

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

[greptile-apps]

Greptile Summary

This PR changes block building to keep one attestation proof per data entry. The main changes are:

• Removed the compaction and recursive proof aggregation step.
• Replaced greedy proof extension with select_best_proof.
• Updated projected voter tracking from the packed proof set.
• Added tests for single-proof selection and block justification.

Confidence Score: 4/5

The changed attestation selection path needs a fix before merging.

• Candidate tiering can use voters from proofs that are not packed.
• The projected justification state can get ahead of the real block contents.
• Removed private helpers and imports do not appear to leave build references behind.

crates/blockchain/src/block_builder.rs

Important Files Changed

Filename	Overview
crates/blockchain/src/block_builder.rs	Disables compaction and adds single-best-proof selection, but candidate tiering still uses union coverage from all proofs.

Comments Outside Diff (1)

1. crates/blockchain/src/block_builder.rs, line 403-409 (link)

   Union Score Projects Justification

   When one AttestationData has several fragmented proofs whose combined voters cross 2/3 but no single proof does, this tier is marked as Justify from the union even though select_best_proof only packs one proof. select_attestations then records the target as justified from score.tier, can skip a competing single-proof supermajority entry or admit dependent attestations, and the final STF can fail or produce a block that misses a justification it could have included.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: crates/blockchain/src/block_builder.rs
   Line: 403-409
   
   Comment:
   **Union Score Projects Justification**
   
   When one `AttestationData` has several fragmented proofs whose combined voters cross 2/3 but no single proof does, this tier is marked as `Justify` from the union even though `select_best_proof` only packs one proof. `select_attestations` then records the target as justified from `score.tier`, can skip a competing single-proof supermajority entry or admit dependent attestations, and the final STF can fail or produce a block that misses a justification it could have included.
   
   How can I resolve this? If you propose a fix, please make it concise.

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/block_builder.rs:403-409
**Union Score Projects Justification**

When one `AttestationData` has several fragmented proofs whose combined voters cross 2/3 but no single proof does, this tier is marked as `Justify` from the union even though `select_best_proof` only packs one proof. `select_attestations` then records the target as justified from `score.tier`, can skip a competing single-proof supermajority entry or admit dependent attestations, and the final STF can fail or produce a block that misses a justification it could have included.

_{Reviews (1): Last reviewed commit: "feat(blockchain): disable attestation co..." | Re-trigger Greptile}

[github-actions]

🤖 Claude Code Review

I have everything I need. Here is the review:

---

PR #473 — feat(blockchain): disable attestation compaction, keep single best proof

This PR removes compact_attestations and extend_proofs_greedily and replaces them with select_best_proof, which picks the single proof with the most net-new validators for each AttestationData entry. The block builder is simplified significantly (−321/+259 lines) with no change to build_block's public signature. Test coverage is updated to match the new one-proof-per-entry guarantee.

---

Finding 1 — Tier mismatch: score_entry scores from union-of-all-proofs, block carries only one (correctness bug)

crates/blockchain/src/block_builder.rs:377–390 and 208–228

score_entry (lines 377–384) unions every proof's net-new participants into new_voters and uses total = prior_count + new_voters.len() to compute crosses_2_3 and assign a tier (Justify or Finalize). But select_best_proof (line 533) picks only one proof. When no individual proof crosses 2/3 but their union does, score_entry returns Tier::Justify, and lines 208–221 advance the projected state: set_justified marks the target slot as justified in projected.justified_slots and projected.current_votes.remove(&target_root) discards the vote bucket.

The actual STF at the end of build_block receives only the single packed proof. If that proof's voter count is below 2/3, the target is not justified in the real post-state.

The cascade: a subsequent candidate entry whose source.slot equals this falsely-projected-justified slot will pass the source_not_justified filter in entry_passes_filters (line 328), get included in the block, and then fail (or be silently dropped) when process_block runs the STF — because the source is not actually justified in the state at that point. Depending on how the STF handles invalid sources, this can cause build_block to return an error or build a structurally valid but peer-rejected block.

Repro scenario: 50 validators; entry has two proofs, proof_A covering validators 0–19 (20 voters) and proof_B covering 20–39 (20 voters); prior_count = 0. Union = 40 ≥ 34 (2/3 of 50) → Tier::Justify. Best single proof = 20 voters < 34 → STF does not justify. A second entry with source.slot = this target.slot is in aggregated_payloads and gets selected (projected source check passes). STF rejects that second attestation or the whole block.

---

Finding 2 — False Justify projection discards vote bucket, blocks in-block recovery (consequence of Finding 1)

crates/blockchain/src/block_builder.rs:221

Once the false Justify projection fires and projected.current_votes.remove(&target_root) executes (line 221), the running vote bucket for that target is gone. In subsequent rounds of the for _round in 0..MAX_ATTESTATIONS_DATA loop, entry_passes_filters rejects any entry targeting the same slot as target_already_justified (line 344-346). Even if aggregated_payloads contains a single proof that would genuinely cross 2/3, it cannot be selected within the same build_block call. Justification for that slot is deferred to a future block — and future blocks would need to start fresh without the accumulated prior voters that were just discarded. This is a direct liveness regression under the failure scenario from Finding 1.

---

Finding 3 — StoreError::SignatureAggregationFailed is a dead enum variant

crates/blockchain/src/store.rs:927

The PR removed the aggregate_proofs import from block_builder.rs — the only code path that previously produced StoreError::SignatureAggregationFailed. The variant still exists in the public enum. reaggregate.rs:181 does call ethlambda_crypto::aggregate_proofs, but handles errors locally with debug! + continue and never surfaces them as StoreError::SignatureAggregationFailed. The variant is now unreachable. Downstream exhaustive match arms on StoreError will have a dead branch; and if ethlambda_crypto::AggregationError's import is ever cleaned up transitively, a compile error will surface. The variant should either be removed or a comment added explaining intentional retention.

---

Minor — selected_proofs trace field is always 0 or 1

crates/blockchain/src/block_builder.rs:202

selected_proofs = selected.len() - before,

before and selected.len() are still read (not dead code), but since select_best_proof appends at most one proof per call, selected_proofs will always be 0 or 1. The field name suggests a variable multi-proof count that was meaningful under the old greedy path. Renaming to proof_packed (bool-like) or removing the field would reduce confusion when reading traces.

---

Note on candidate_new_voters vs packed_voters in traces

crates/blockchain/src/block_builder.rs:197

candidate_new_voters = score.new_voters logs the union-of-all-proofs count; packed_voters logs the single-proof count. When these diverge, the trace makes the tier look misleading (tier=Justify candidate_new_voters=34 packed_voters=20). This is a direct symptom of Finding 1 and would be resolved if score_entry is fixed to score against the single best proof's count rather than the full union.

---

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