fix: skip one signature aggregation jobs

[MegaRedHand]

🗒️ Description / Motivation

build_job (crates/blockchain/src/aggregation.rs) already skipped aggregation when there was nothing at all to do (no raw signatures and fewer than 2 children), but it missed one case: exactly one raw gossip signature with no children to merge it with.

That job still went to the worker and ran the full aggregate_mixed leanVM/XMSS proving pipeline, just to wrap a single signature into a 1-participant TypeOneMultiSignature. That proof is no smaller or cheaper to verify than the raw signature it wraps, so the proving cost was paid for zero compression benefit, every interval-2 tick, for every attestation-data root that happened to have only one signer so far (e.g. early in a slot, before other votes arrive). aggregate_mixed's own doc comment already calls this out for the "lone child" case ("A lone child is already a valid Type-1; further aggregation is wasted work") but the equivalent "lone raw signature" case wasn't covered.

Aggregation runs on a tight AGGREGATION_DEADLINE budget shared across all groups in a session, so every wasted proof is time taken away from groups that actually need it.

What Changed

• crates/blockchain/src/aggregation.rs: build_job now also returns None when children.is_empty() && raw_ids.len() <= 1, skipping job creation for a lone, companion-less raw signature.

Correctness / Behavior Guarantees

• No votes are lost: a skipped signature simply stays in the in-memory gossip_signatures buffer (it's never added to keys_to_delete), so it's reconsidered on the next interval-2 tick once a companion signature or mergeable proof shows up for the same attestation data. It remains subject to the buffer's existing FIFO eviction and finalization pruning, same as before.
• Purely a CPU-cost optimization on the aggregation hot path; it does not change which attestations eventually reach a block, only avoids a wasted leanVM call when one would have produced a no-op (1-in, 1-out) proof.

Tests Added / Run

• No new tests added; this is a one-line guard mirroring the existing pattern just above it.
• Ran cargo test -p ethlambda-blockchain --release (incl. forkchoice_spectests signature-aggregation cases) — all passing.

Related Issues / PRs

• N/A

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• Ran cargo test --workspace --release — all passing

[github-actions]

🤖 Codex Code Review

Findings

1. High: the new early return in aggregation.rs drops lone valid gossip attestations on the floor. Aggregators persist unaggregated validator signatures only in the gossip buffer in store.rs, and those signatures become fork-choice-visible only after apply_aggregated_group inserts a Type-1 proof into new_payloads in aggregation.rs. With children.is_empty() && raw_ids.len() <= 1, a message signed by exactly one validator never gets materialized into a proof, so it contributes neither to safe-target computation nor to LMD GHOST until another signer happens to arrive. That is a consensus regression, not just a perf optimization. It also conflicts with aggregate_mixed, which explicitly allows “at least one raw signature” in crypto/src/lib.rs. The fix is to keep the old behavior for the 1 raw / 0 children case, or route that case through aggregate_signatures if you want a cheaper fast path.

No other issues stood out in this small diff, but I would add a regression test for “single gossip attestation becomes a pending proof and affects fork-choice-visible state.”

---

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

[github-actions]

🤖 Kimi Code Review

Overall Assessment: The change correctly optimizes aggregation by skipping redundant operations on single signatures. The logic is sound and safe.

Specific Feedback:

1. Logic Correctness (Lines 208-215): The new condition correctly handles the case where there is only a single raw signature and no child proofs to aggregate with. Returning None here is appropriate—aggregating a single signature is a no-op, and the caller should use the raw signature directly.

2. Code Clarity: The two separate if statements are functionally equivalent to checking if raw_ids.len() + children.len() < 2, but the separated form is more explicit about the distinct cases. Consider consolidating if you prefer terseness, but the current approach is readable and maintainable.

3. Edge Case Handling:

   • The first condition (raw_ids.is_empty() && children.len() < 2) handles cases with 0 raw signatures and 0-1 children.
   • The second condition (children.is_empty() && raw_ids.len() <= 1) handles cases with 0 children and 0-1 raw signatures.
   • The overlap (0 raw + 0 children) is handled by the first check, so the second check's <= 1 (which includes 0) is harmless but logically correct.

4. Consensus Safety: Ensure the calling code in build_job or its consumers properly handles the None case by propagating the single unaggregated signature rather than treating None as a failure. This appears to be the intended behavior given the comment.

Suggestion (Optional):
If raw_ids and children are both Vec or slice types, you could simplify to:

if raw_ids.len() + children.len() < 2 {
    return None;
}

However, the explicit separation may be intentional for future extensibility (e.g., different thresholds for raw vs. aggregated signatures).

Verdict: LGTM. The optimization is correct and prevents unnecessary cryptographic operations.

---

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

[greptile-apps]

Greptile Summary

This PR changes when aggregation jobs are built for small input groups. The main change is:

• Skip job creation when there is one raw signature and no child aggregate.

Confidence Score: 4/5

The single-signature aggregation path can leave gossip signatures unconsumed.

• The changed branch returns before cleanup metadata is created.
• The caller does not delete raw gossip signatures when build_job returns None.
• Single-signer groups can remain in storage instead of being promoted or removed.

crates/blockchain/src/aggregation.rs

Important Files Changed

Filename	Overview
crates/blockchain/src/aggregation.rs	Adds an early return in build_job that suppresses job creation for a single raw signature with no child aggregate.

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/aggregation.rs:213-215
**Single Signature Stays Unconsumed**

When a gossip group has exactly one raw validator signature and no child aggregate, this branch returns before `keys_to_delete` is built. The caller treats `None` as no aggregation job, so the previous path that produced an aggregate and deleted the consumed gossip signature is skipped, leaving that single-signer attestation unpromoted and still present in the gossip store.

_{Reviews (1): Last reviewed commit: "fix: skip aggregation when there's only ..." | Re-trigger Greptile}

[github-actions]

🤖 Claude Code Review

Review: fix: skip aggregation when there's only a single signature (PR #483)

Diff: crates/blockchain/src/aggregation.rs — adds a second early-return in build_job so a job with zero "children" proofs and at most one raw gossip signature is skipped.

1. Possible vote-loss / liveness concern — please confirm against intended semantics

build_job is the only producer of entries for new_payloads / known_payloads (see apply_aggregated_group → insert_new_aggregated_payload, crates/blockchain/src/aggregation.rs:313-315). Those two buffers are in turn the only source fork choice reads from (store.extract_latest_known_attestations / extract_latest_new_attestations, used in crates/blockchain/src/store.rs:42,117) and the only source block-building packs into the body (store.known_aggregated_payloads() in crates/blockchain/src/store.rs:813).

With this change, an attestation-data group that ends up with exactly one raw gossip signature and zero reusable child proofs is dropped from build_job (returns None) instead of being trivially wrapped into a 1-participant TypeOneMultiSignature. The signature stays in the gossip_signatures buffer and is re-evaluated every interval-2 tick, but if it never gains a second signature for the same data_root (e.g. an isolated/minority-fork attester, or unlucky gossip timing that never recurs), it will never be promoted, never count toward fork-choice weight, and never be included in a block — until it's eventually pruned at finalization (prune_gossip_signatures, slot-based, not content-based).

Before this PR, that lone signature would still go through aggregate_mixed (with children=[], one raw sig) and produce a valid 1-of-N proof that did count. So this is a real behavior change, not just a perf optimization: it trades "spend the leanVM budget proving a useless 1-of-N aggregate" for "permanently strand a vote that never finds a partner." For small validator sets (e.g. devnets) a single missing vote can matter for the 3SF-mini justification thresholds.

Please confirm this matches leanSpec's reference behavior (i.e., that the spec also discards genuinely-unaggregatable singleton votes rather than admitting them as trivial 1-of-1 proofs) rather than this being an unintended side effect of trying to avoid wasted aggregation work.

2. The two skip conditions can be merged into one, simpler check

The new logic at aggregation.rs:208-215:

if raw_ids.is_empty() && children.len() < 2 {
    return None;
}
if children.is_empty() && raw_ids.len() <= 1 {
    return None;
}

is exactly equivalent to:

if children.len() + raw_ids.len() < 2 {
    return None;
}

(verified by case enumeration over {0,1,2+} × {0,1,2+}). A single condition reading "need at least two total signatures (raw + already-aggregated children) to make aggregation worthwhile" is clearer than two overlapping special cases and removes the need to reason about why there are two separate ifs.

3. aggregate_job's defensive guard is now inconsistent with build_job

aggregate_job (line 271-274) still only guards raw_ids.is_empty() && children.len() < 2 — it doesn't know about the new children.is_empty() && raw_ids.len() <= 1 case. This is currently harmless since build_job is the sole caller and already filters those jobs out, but the two thresholds have now drifted apart with no comment explaining why. Worth either:

• extracting a shared fn is_aggregatable(children: usize, raw: usize) -> bool used by both, or
• adding a comment in aggregate_job noting its guard intentionally only mirrors the lower-level aggregate_mixed precondition and is not meant to be a complete duplicate of build_job's filter.

4. No test coverage added

aggregation.rs has no unit tests at all today, and this PR doesn't add any either, despite fixing a boundary condition in consensus-relevant code. Given Point 1 above, a focused unit test on build_job (or at least aggregate_job/the simplified predicate) enumerating (children_count, raw_ids_count) → Some/None would both lock in the intended behavior and make the consensus implication in Point 1 explicit and reviewable.

Nit

• The comment "Skip aggregation when there's only a single raw signature to aggregate" pairs with raw_ids.len() <= 1, but by the time this line is reached raw_ids.len() == 0 is already excluded by the first check, so the only reachable case is raw_ids.len() == 1. Using == 1 instead of <= 1 would match the comment exactly (purely cosmetic, no behavior difference) — moot if Point 2's simplification is adopted.

---

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