feat(blockchain): aggregate only current-slot raw signatures at interval 2

[MegaRedHand]

What

Adds snapshot_current_slot_aggregation_inputs and wires it into start_aggregation_session, so the interval-2 aggregation worker aggregates only the current slot's raw gossip signatures.

Why

The old snapshot_aggregation_inputs built jobs in HashMap-iteration order: Pass 1 gossip groups (in random HashMap::values() order) followed by Pass 2 payload-only recursive merges. There was no slot ordering.

Since the worker stops at the AGGREGATION_DEADLINE (750ms) and drops any remaining jobs, under load the deadline could sacrifice current-slot aggregation in favor of stale-slot or recursive-merge work, purely by random iteration order. Only the current slot's aggregates get promoted to known_payloads and packed into the next block, so that deadline budget was misspent.

How

New function restricts aggregation to gossip groups whose data.slot == current_slot, building pure raw-signature jobs:

	Old (snapshot_aggregation_inputs)	New (snapshot_current_slot_aggregation_inputs)
Slot filter	none (all slots)	data.slot == current_slot
Proof reuse (children)	greedy set-cover over existing proofs	none
Pass 2 (payload-only merges)	yes	no
Job order under deadline	random HashMap order	all current-slot, so ordering is moot

snapshot_aggregation_inputs and build_job are left untouched.

Consequences

• Interval-2 aggregation no longer does recursive proof merge / compaction. The block-build path still compacts when packing, so this is mostly redundant work removed.
• Stale-slot gossip signatures are no longer consumed at interval 2; they linger until FIFO eviction or prune-below-finalized (bounded).

Testing

make fmt, make lint (workspace clippy -D warnings) clean. Per request, no unit tests added in this PR.

[github-actions]

🤖 Kimi Code Review

The PR introduces a targeted optimization to aggregate only current-slot gossip signatures, avoiding wasteful re-aggregation of stale attestations. The logic is sound, but I have concerns about signature deletion behavior and state validity.

Critical: Potential Loss of Unresolved Signatures

• File: crates/blockchain/src/aggregation.rs
• Lines: 303-306

In build_raw_signature_job, keys_to_delete includes all signatures in the group, even those that failed pubkey resolution (lines 287-293 skip invalid validators but still delete their entries):

let keys_to_delete: Vec<(u64, H256)> = validator_sigs
    .iter()
    .map(|(vid, _)| (*vid, data_root))
    .collect();

If resolution fails due to transient state issues (e.g., head state not yet updated for a new validator set), these signatures are permanently lost from gossip rather than retried later. Recommendation: Only delete successfully resolved signatures (those in raw_ids) unless this is intentional for DoS prevention, in which case document this behavior explicitly.

State Validity Concern

• File: crates/blockchain/src/aggregation.rs
• Line: 184

Using store.head_state() to resolve validator pubkeys assumes attestations are valid against the head state. Per consensus spec, attestations should be validated against the state corresponding to their target epoch, not the head state. If the head state diverges from the attestation's target state (e.g., during reorgs or epoch transitions), valid attestations may be incorrectly dropped. Verify this aligns with your validation pipeline.

Minor Issues

1. Integer Truncation (32-bit platforms)

   • Line: 288
   • *vid as usize truncates validator indices > 2^32 on 32-bit architectures. While unlikely given current validator set sizes, use usize::try_from(*vid).ok()? for safety.

2. Capacity Pre-allocation Mismatch

   • Lines: 283-285
   • Vectors pre-allocate capacity based on validator_sigs.len() but may end up smaller due to resolution failures. This is fine for performance, but consider shrinking or using filter_map collect if memory is tight.

3. Documentation Consistency

   • Line: 301-302
   • The comment says "mirroring build_job" but verify that build_job also deletes unresolved signatures. If not, this introduces inconsistent behavior between aggregation passes.

Positive Observations

• Good use of let-else for error handling (lines 186-187, 289-291)
• Early return on empty gossip groups avoids unnecessary head state lookups (lines 182-184)
• Clear separation between slot-filtered and full aggregation logic preserves backward compatibility

Consensus Safety
The slot-filtering logic (line 192) correctly implements the optimization described in the PR description. However, ensure this doesn't prevent aggregation of attestations for the previous slot that arrive late (within the attestation propagation window). If current_slot is strictly the store's current slot, late attestations from slot N-1 processed at slot N boundary will be ignored, potentially affecting fork choice weights. Verify your network layer separates "current slot duties" from "fork choice inclusion".

---

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

[github-actions]

🤖 Codex Code Review

1. current_slot filtering drops valid late attestations on the floor. In crates/blockchain/src/aggregation.rs, snapshot_current_slot_aggregation_inputs skips every gossip group whose data.slot != current_slot, and crates/blockchain/src/lib.rs now uses that path exclusively. But raw gossip signatures are only stored “for later aggregation at interval 2” in crates/blockchain/src/store.rs, and block production consumes only known_aggregated_payloads in crates/blockchain/src/store.rs. So an attestation that arrives after its own slot’s interval-2 window remains in gossip_signatures until pruning and never reaches fork choice or block inclusion. That is a consensus liveness regression compared with the previous “aggregate any pending gossip” behavior.

2. The new raw-only job path removes proof reuse and participant de-duplication, which will inflate proof count and can evict unrelated attestations under load. The previous path in crates/blockchain/src/aggregation.rs reused existing proofs and skipped raw signatures already covered by selected children (covered.contains(vid)). The new build_raw_signature_job in crates/blockchain/src/aggregation.rs always emits a fresh proof from all current-slot raw sigs and the caller no longer runs the payload-only merge pass. Since the payload buffers are capped by total proofs, not by attestation data roots, in crates/storage/src/store.rs and crates/storage/src/store.rs, overlapping proofs will accumulate longer, increase proposer-side aggregation work, and can FIFO-evict other useful payloads earlier.

No security or memory-safety issue stands out in this diff beyond those behavioral regressions. I would also want a regression test for “attestation arrives after slot N interval 2, gets aggregated at slot N+1” before merging.

---

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

[greptile-apps]

Greptile Summary

This PR narrows interval-2 aggregation to current-slot raw gossip signatures. The main changes are:

• Adds a current-slot-only aggregation snapshot helper.
• Adds raw-signature-only aggregation job construction.
• Switches start_aggregation_session to use the new snapshot path.

Confidence Score: 4/5

The changed flow looks mergeable after checking the payload-only aggregate path.

• Current-slot raw gossip aggregation follows the intended narrower path.
• Payload-only current-slot proofs no longer get interval-2 recursive merge work.
• No security issue was identified in the changed code.

crates/blockchain/src/lib.rs

Important Files Changed

Filename	Overview
crates/blockchain/src/aggregation.rs	Adds current-slot raw-signature snapshotting and raw-only job construction.
crates/blockchain/src/lib.rs	Routes interval-2 aggregation through the new current-slot-only snapshot path.

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/lib.rs:392-394
**Payload-Only Aggregates Stop Merging**

When the node receives current-slot aggregated payloads without matching raw gossip signatures, this path now returns early because `snapshot_current_slot_aggregation_inputs` never scans `new_payload_keys`. The old snapshot built payload-only merge jobs for those proofs, so interval-2 aggregation can now skip recursive merging for valid current-slot payloads and leave the block builder with less compact aggregate coverage.

_{Reviews (1): Last reviewed commit: "feat(blockchain): aggregate only current..." | Re-trigger Greptile}

[github-actions]

🤖 Claude Code Review

Review: feat(blockchain): aggregate only current-slot raw signatures at interval 2

Summary

The change is small, well-scoped, and the goal (stop letting deadline pressure spend interval-2 budget on stale-slot/recursive-merge work instead of current-slot votes) is reasonable. build_raw_signature_job correctly mirrors the pubkey-resolution and keys_to_delete logic of build_job. Fmt/lint clean per the PR description. Two points below are worth deliberate sign-off before merge since they affect attestation inclusion, not just performance.

Findings

1. Late-arriving current-slot signatures are now silently dropped forever, not just deprioritized (consensus-relevant)

crates/blockchain/src/aggregation.rs:186-218 / crates/blockchain/src/lib.rs:391-396

The snapshot for slot N is taken once, at the start of interval 2. Any gossip signature for slot N that lands after that snapshot but before the 750ms deadline is not in the job list (this was already true before this PR — same architecture). The difference is what happens next: with the old snapshot_aggregation_inputs, that leftover slot-N group would still be picked up on the next interval-2 tick (slot N+1), because Pass 1 iterated over gossip_groups with no slot filter. With snapshot_current_slot_aggregation_inputs, the slot-N+1 tick only looks at data.slot == N+1, so the slot-N group is never revisited — it just sits in the FIFO buffer until eviction or finalization pruning, and those validators' votes for round N never get aggregated or packed into any block.

The PR description acknowledges this ("Stale-slot gossip signatures are no longer consumed at interval 2; they linger…") but frames it as a bounded/benign side effect. It's worth confirming this is the intended trade-off: under normal clock sync this only affects the tail of late attesters, but under network jitter/partial partition it could measurably reduce the aggregate participation rate carried into blocks, with knock-on effects on justification thresholds. If that's acceptable (and matches the "only current slot's votes get promoted anyway" reasoning in the PR body), fine — but this is a behavior change in attestation inclusion, not merely a scheduling optimization, and should be called out as such in review/changelog rather than purely as a performance fix.

2. snapshot_aggregation_inputs (and its private helpers build_job, select_proofs_greedily, resolve_child_pubkeys) are now dead in production

crates/blockchain/src/aggregation.rs:122-173, 223-274, 328-354, 424-461

Grep confirms snapshot_aggregation_inputs has no callers anywhere in the workspace after this PR rewires start_aggregation_session to the new function (crates/blockchain/src/lib.rs:391-392). Because it's pub fn in a pub mod, it won't trigger dead_code lint, so make lint stays clean even though ~90 lines of proof-reuse/greedy-set-cover/Pass-2 logic are now unreachable from the actor. If there's no near-term plan to call it (e.g., from an API endpoint, a test, or a future recovery path), consider removing it along with its now-orphaned helpers — keeping a full parallel aggregation strategy around as unused "just in case" code invites drift (e.g., bugs fixed in one path and not the other) for a reader who doesn't realize it's dead.

3. Minor: duplicated pubkey-resolution logic between build_job and build_raw_signature_job

crates/blockchain/src/aggregation.rs:236-249 vs crates/blockchain/src/aggregation.rs:289-299

The validator-lookup / pubkey-resolution / keys_to_delete construction loops are copy-pasted. Since build_raw_signature_job is effectively build_job with proof-reuse disabled and no store access, this could be expressed as a shared helper (e.g., extract the raw-sig resolution loop into a free function taking validators and validator_sigs) to avoid the two copies drifting apart if one is patched later (e.g., a future dedup/validation fix applied to only one). Not blocking, and the current PR is small enough that this is a nit rather than an actual bug.

4. Performance note (secondary to Finding 1): iter_gossip_signatures() now clones a fuller buffer more often

crates/storage/src/store.rs:453-465, GOSSIP_SIGNATURE_CAP = 2048 (crates/storage/src/store.rs:128)

snapshot() clones every entry (attestation data + all validator signatures) in the FIFO buffer regardless of slot, then snapshot_current_slot_aggregation_inputs filters down to one slot. Previously, interval-2 actively drained essentially the whole buffer each tick (all slots processed), keeping it small; now stale-slot groups persist until FIFO eviction/finalization pruning, so the buffer will more often sit near its 2048-signature cap, making the per-tick full clone consistently costlier than before. Bounded (not unbounded growth), but worth confirming the ~4s cadence and clone size are acceptable — a Store method that iterates without cloning entries outside the target slot would avoid this if it becomes a hot spot.

Nice-to-have

No unit test was added for snapshot_current_slot_aggregation_inputs (stated as intentional per PR body). Given Finding 1 above, at minimum a test asserting that a stale-slot gossip group is excluded from the snapshot (vs. included by the old function) would make the intended semantic change explicit and regression-proof.

---

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