feat(aggregation): score interval-2 jobs like the block builder

[MegaRedHand]

What

Rework snapshot_aggregation_inputs into a tiered greedy selector modeled on
the block builder's select_attestations: an up-front store pass resolves each
candidate's aggregation material once (raw-first + trim), then a loop scores
candidates by (current-slot-first, Finalize > Justify > Build) against an
optimistically-projected state, emitting at most MAX_AGGREGATION_JOBS jobs.

Why

The interval-2 session aggregated current-slot gossip groups in arbitrary order
and ignored existing proofs, so it could not prioritize the groups whose
aggregation most advances consensus.

How

• Candidates are filtered by the block builder's entry_passes_filters against a
  chain view covering [0, head_slot] (the head root is pushed onto the state's
  historical_block_hashes, which omits the head's own root), so prover time is
  spent only on aggregations a block could actually pack.
• Reuses the shared ProjectedState (from_head_state + advance) and
  Tier/EntryScore/entry_passes_filters via a coverage-based
  score_from_coverage core, so proposer and aggregator can never drift on
  either the justify/finalize projection or the tiering.
• Deletes snapshot_current_slot_aggregation_inputs and
  build_raw_signature_job (subsumed).

Stacking

Stacked on #525 (the block builder refactor). Review/merge that first; GitHub
retargets this PR to main once it lands.

Checks

• cargo fmt clean
• cargo clippy --all-targets -- -D warnings clean
• cargo test -p ethlambda-blockchain --lib green (incl. resolve_job / pick_best_candidate / snapshot tests)

[github-actions]

🤖 Kimi Code Review

Overall Assessment: High-quality PR. The refactoring correctly unifies scoring logic between block building and aggregation, fixes a critical chain-view bug for current-head attestations, and properly prevents XMSS double-inclusion. Good test coverage.

Critical Consensus Correctness

• Item 1: Chain view extension for current-head votes (aggregation.rs:254-256)
  Correctly extends historical_block_hashes with store.head() to cover the tip at index head_slot. Without this, attestation_data_matches_chain would reject valid attestations targeting the current head (since process_block_header only pushes the parent root). The regression test snapshot_aggregates_vote_for_current_head_on_non_genesis_chain validates this fix.

• Item 2: Double-inclusion prevention in XMSS aggregation (aggregation.rs:481-486)
  The resolve_job function correctly trims raw signatures whose validators are already covered by selected child proofs. The comment explicitly notes that aggregate_mixed must never receive a validator both as a raw participant and inside a child. This is security-critical for XMSS signature validity.

• Item 3: Justified target filtering (aggregation.rs:400-403 via entry_passes_filters)
  Correctly filters out attestations targeting already-justified/finalized slots. This prevents wasting prover cycles on attestations that cannot contribute to fork choice.

Code Correctness & Edge Cases

• Item 4: Duplicate validator ID handling in resolve_job (aggregation.rs:459-467)
  If validator_sigs contains duplicate validator IDs (same vid appearing twice), raw_by_id.insert(*vid, ...) will silently overwrite the first signature with the second. While keys_to_delete correctly captures both for cleanup, the aggregation loses the first signature. Consider using HashMap::entry or asserting uniqueness if the protocol guarantees no duplicates.

• Item 5: Viability threshold consistency (aggregation.rs:496-501)
  The logic rejecting lone raw signatures (raw_ids.len() <= 1 && children.is_empty()) and insufficient children (raw_ids.is_empty() && children.len() < 2) is correct and consistent with the PR description. This prevents spawning useless aggregation jobs.

Performance & Resource Management

• Item 6: Historical hashes cloning (aggregation.rs:254)
  extended_historical_block_hashes clones the entire historical_block_hashes vector. For long-running chains, this is O(n) in chain length. Consider whether an Arc<[H256]> or slice view could be used, though likely acceptable given Ethereum's current historical root window.

• Item 7: MAX_AGGREGATION_JOBS cap (aggregation.rs:164)
  Capping at 3 jobs is a sensible DoS protection against unbounded prover work per slot.

Rust Best Practices

• Item 8: Error handling with expect (aggregation.rs:254-255, 263)
  The uses of expect for store.get_block_roots() and store.head() are acceptable only if these are truly invariant violations (store must be initialized). Ensure these cannot be triggered by adversarial input or corrupted state.

• Item 9: Test signature caching (aggregation.rs:806-817)
  The dummy_sig() helper correctly uses std::sync::LazyLock to cache the XMSS signature across tests, avoiding expensive re-generation. Good pattern.

Code Structure

• Item 10: Shared scoring logic (block_builder.rs:449-478)
  Extracting score_from_coverage and making it pub(crate) is a good refactor that ensures aggregation and block building can never drift on tiering logic (Finalize > Justify > Build).

Minor Suggestions

• Line 462 (aggregation.rs): The data_root parameter in resolve_job is unused except for keys_to_delete construction. This is fine, but could be documented.

• Line 624 (aggregation.rs): select_proofs_greedily now takes seed_covered: HashSet<u64> by value (moved). This is correct since the set is consumed, but document that the caller loses ownership.

Security Summary: No critical vulnerabilities introduced. The XMSS double-inclusion guard (Item 2) and chain-view fix (Item 1) are correctly implemented. The MAX_AGGREGATION_JOBS cap provides DoS resistance.

Approval: Approve with minor suggestion to handle duplicate validator IDs explicitly in resolve_job if the input isn't guaranteed unique.

---

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

[github-actions]

🤖 Codex Code Review

1. High: raw XMSS inputs lose the required validator ordering in aggregation.rs. resolve_job() first copies validator_sigs into a HashMap, then rebuilds raw_pubkeys / raw_sigs / raw_ids by iterating that map at aggregation.rs. The storage layer explicitly documents that gossip signatures are kept in ascending validator order because XMSS aggregation requires it at store.rs, and the snapshot currently preserves that order via BTreeMap::iter() at store.rs. Replacing that with HashMap iteration makes the raw component order nondeterministic, which can yield invalid proofs or flaky behavior. Preserve the original validator_sigs order and use a side set/map only for trimming.

2. Medium: the early-aggregation path now snapshots the full pool, including stale groups and payload-only merges, at lib.rs and aggregation.rs, but an early-started session still suppresses the normal T2 session for that slot at lib.rs and is only started once per slot at lib.rs. Combined with the 3-job cap at aggregation.rs, this means signatures that arrive later in the same slot can no longer be picked up by a fresh T2 snapshot, while stale/payload-only work can consume the slot’s only aggregation session. That is a liveness regression for current-slot attestation coverage. I’d keep the early path current-slot-only, or allow a second refresh at T2.

3. Low: leansig and rand were added as normal dependencies in crates/blockchain/Cargo.toml, but they are only used by the test helper dummy_sig() under #[cfg(test)] at aggregation.rs. These should be dev-dependencies to avoid expanding the production dependency graph and compile surface.

I did not run tests locally: cargo is blocked here because the pinned Rust toolchain tries to write under a read-only ~/.rustup, and the environment has no network fallback.

---

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

[greptile-apps]

Greptile Summary

This PR replaces interval-2 aggregation with a tiered greedy job selector. The main changes are:

• Resolves raw signatures and existing proofs before scoring candidates.
• Filters candidates against the canonical chain view.
• Prioritizes current-slot and consensus-advancing jobs.
• Projects selected coverage across up to three rounds.
• Adds tests for resolution, ordering, projection, filtering, and job limits.

Confidence Score: 5/5

The latest changes look safe to merge.

• No additional blocking issue was found in the updated code.
• The existing storage-read failure is already covered by the earlier review comment.

Important Files Changed

Filename	Overview
crates/blockchain/src/aggregation.rs	Adds candidate resolution, chain filtering, projected-state scoring, capped greedy selection, and unit tests.
crates/blockchain/src/lib.rs	Routes interval-2 aggregation through the new slot-aware snapshot selector.
crates/blockchain/Cargo.toml	Adds test dependencies used to generate aggregation test signatures.
Cargo.lock	Records the new direct test dependencies for the blockchain crate.

_{Reviews (2): Last reviewed commit: "Merge branch 'main' into feat/scored-agg..." | Re-trigger Greptile}

[github-actions]

🤖 Kimi Code Review

The PR introduces a tiered greedy selector for attestation aggregation that mirrors the block builder's logic, with proper handling of raw signatures vs. existing proofs to prevent double inclusion. Overall this is a solid refactor with good test coverage, but there are a few issues to address:

Critical Issues

1. Panic risk on store operations (lines 232, 247)
store.get_block_roots() and store.head() use .expect() which will crash the node if the store returns an error. While these might be considered invariant violations, consensus clients should degrade gracefully or retry rather than panic on storage hiccups.

// Line 232
let known_block_roots = store.get_block_roots().expect("block roots read works");
// Line 247  
extended_historical_block_hashes.push(store.head().expect("head read works"));

Suggestion: Convert to proper error handling or ? operator propagation, allowing the aggregation worker to skip the round rather than crash the process.

Security & Correctness

2. Double inclusion prevention (lines 367-375)
The trimming logic in resolve_job correctly prevents validators from appearing both as raw signatures and inside child proofs. The comment at lines 441-444 accurately describes why this matters: aggregate_mixed must never receive overlapping validator sets. This is a critical security fix—acknowledged.

3. Seed coverage in proof selection (line 622)
select_proofs_greedily now accepts seed_covered to prime the coverage set. This ensures child proofs are only selected for coverage beyond what raw signatures already provide, preventing redundant aggregation work and ensuring accurate scoring. Correct.

Performance & Code Quality

4. Unnecessary clone in candidate removal (line 283)

let Candidate { hashed, resolved } = candidates
    .remove(&data_root)
    .expect("picked candidate exists in pool");

Since data_root is an H256 (Copy), this is fine, but consider using if let Some(candidate) = candidates.remove(&data_root) to avoid the expect (though the expect is technically safe here since you just checked existence).

5. Test-only dependencies properly gated
The addition of leansig and rand to Cargo.toml is correctly scoped (available for tests via #[cfg(test)]). The dummy_sig() implementation using LazyLock is efficient for tests.

Consensus Logic

6. Current head extension (lines 243-250)
The fix for extending historical_block_hashes with store.head() to cover the current tip is correct. Without this, votes for the current head would fail attestation_data_matches_chain because process_block_header only pushes parent roots, not the block's own root. This matches the Ethereum consensus spec behavior.

7. Slot bucket prioritization (line 308)

let slot_bucket: u8 = if att_data.slot == current_slot { 0 } else { 1 };

Current-slot attestations correctly precede stale ones. This aligns with the block builder's priority and prevents interval-2 aggregation from wasting time on old slots when current-slot work is pending.

Testing

8. Comprehensive test coverage
The tests cover:

• Raw-first + trim logic (resolve_job_trims_raw_covered_by_chosen_child)
• Slot prioritization (pick_best_candidate_prefers_current_slot_over_higher_stale_score)
• Projection and re-tiering (pick_best_candidate_re_tiers_same_target_after_first_selection)
• Current head aggregation (snapshot_aggregates_vote_for_current_head_on_non_genesis_chain)
• Caps (snapshot_caps_jobs_at_max_aggregation_jobs)

Minor nit: In snapshot_caps_jobs_at_max_aggregation_jobs, the assertion checks for slots [3,4,5] when NUM_GROUPS=5 and slots are 1..=5. Verify that Build-tier ordering indeed prefers larger target_slot on ties—if the ordering is reversed, this test would fail incorrectly.

Summary

The PR is technically sound and fixes a real consensus bug (the current head chain view issue). The main blocker is the use of .expect() on store operations which introduces crash vectors. Once those are hardened, this is ready to merge.

---

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

[github-actions]

🤖 Codex Code Review

1. crates/blockchain/src/aggregation.rs reuses ProjectedState::entry_passes_filters, which in turn enforces attestation_data_matches_chain against the local head-chain view (crates/blockchain/src/block_builder.rs, crates/blockchain/state_transition/src/lib.rs). That is correct for block packing, but wrong for gossip aggregation: on_gossip_attestation accepts votes on any known branch that descends from finalized (crates/blockchain/src/store.rs), while this selector now drops off-head-but-valid votes as chain_mismatch. In a fork, those are exactly the votes LMD-GHOST needs to learn about the competing branch, so this can suppress honest aggregates and delay or prevent head changes.

2. crates/blockchain/src/aggregation.rs, crates/blockchain/src/aggregation.rs, crates/blockchain/src/lib.rs hard-cap each interval-2 session at 3 jobs. That is a behavioral regression from the previous snapshot_current_slot_aggregation_inputs path, which aggregated every current-slot gossip group. If a slot has more than 3 attestation-data groups, the rest stay as raw gossip signatures; because selection always prioritizes att_data.slot == current_slot first (crates/blockchain/src/aggregation.rs), sustained load can starve older groups indefinitely. This is a liveness/fork-choice issue, not just a throughput tradeoff.

I couldn’t run cargo test here because cargo needs to write under a read-only home and fetch the new leansig git dependency. The head-chain extension for current-head votes looks good, but the two points above should block the PR as written.

---

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