feat(aggregation): score recursive aggregation through a per-aggregator subnet window

[MegaRedHand]

Summary

Every aggregator on the network currently does the same aggregation work. In a healthy slot all validators vote for the same head, so there is one hot AttestationData; candidates are scored deterministically from head state, and select_proofs_greedily picks the two highest-coverage children from a pool every aggregator sees, since aggregates gossip on one global topic. So all aggregators select the same two children and produce the same merged proof, and every copy of that leanVM work past the first is wasted.

If every aggregator anchors on the pool's best proof r0, then aggregator i publishes out_i = r0 ∪ x_i, and out_i ∪ out_j = out_i ∪ x_j. Merging two published proofs gains nothing over merging one of them with a raw pool proof.

This gives each aggregator a duty subnet and a window of subnets starting there, used as a scoring lens on child selection, so different aggregators merge different children.

Design

• Window: the contiguous cyclic run of subnets {s, s+1, ...} an aggregator is responsible for, starting at its duty subnet.
• Scoring lens, not a filter: a child is valued by the validators it newly covers whose subnet is inside the window. A proof straddling the boundary stays usable for its in-window part; one lying wholly outside scores zero. A selected child still contributes all of its participants to covered, which keeps the marginal-coverage score honest across greedy rounds.
• Width from the anchor: min(2 * reach(anchor), C), where the anchor is the largest-coverage proof in the candidate's pool that touches the aggregator's own duty subnet, and a proof's reach is how many distinct subnets it touches. Coverage rather than reach picks the anchor, so a sparse proof holding one validator in each of many subnets no longer sets the width for everybody. Requiring the anchor to touch the duty subnet makes "no anchor" mean "no peer has covered my subnet", which is exactly when this node's raw signatures are irreplaceable: the window then sits at its narrowest and the aggregator works on those instead of merging proofs it cannot add to. Deriving the width rather than choosing it is essential either way, since windows nest and "widest viable window" would collapse to the full committee set for everyone.
• Duty subnet: the first --aggregate-subnet-ids value, else the lowest subscribed subnet, else 0. Logged at startup so a collision is diagnosable.
• --skip-redundant-aggregation (opt-in): an aggregator sits out any candidate whose derived width it does not own this slot (duty_subnet % w == slot % w), and the freed job goes to the next-best AttestationData rather than to a narrower merge of the same one. Ownership rotates with the slot, so no duty subnet is permanently the one sitting out, and width 1 is owned by everyone, so a candidate with no anchor on this node's subnet is never skipped.

Safety

• No consensus impact. Nothing in attestation processing, block building, or fork choice is touched. A windowed aggregate binds exactly raw_ids ∪ accepted_child_ids and its bits derive from that same set, so it is valid with fewer participants: less fork-choice weight, never wrong weight.
• No coverage regression. A window is contiguous but the pool need not be contiguous in subnet space, so a strided aggregator placement could leave a window holding one proof and drop a merge the unwindowed selection would have made. When a windowed selection is not viable it falls back to the full committee set, so the window can only improve on the old selection, never regress below it. The fallback is disabled under --skip-redundant-aggregation: every width below the committee count has several owners, so retrying there would rebuild exactly the duplication the flag buys away.
• attestation_committee_count = 1 is a provable no-op: the window contains every validator, so in-window coverage equals total coverage and the break condition is identical to before, tie-break order included.
• Mixed-network safe. No wire-format, topic, or fork-digest change. A main node's wide proof can raise a branch node's anchor reach where it touches that node's duty subnet, which opens its window, so a partial rollout makes the feature weaker rather than inconsistent.

Known trade-off

Because the anchor must touch the duty subnet, the derived width is no longer uniform across the network. Two aggregators reading one lopsided pool can derive different widths, so their windows nest rather than tile. That costs a round of climbing, not correctness, and it is the direct price of tying the window to work the aggregator can actually contribute to.

Cadence, in practice

There is one aggregation session per slot, and the current slot's pool is empty at snapshot time since produced aggregates are held until the interval-2 boundary. So the window bites on the stale candidate, and a data root gets about one windowed merge rather than a multi-round climb. The widening matters across slots for a data root that stays live.

Metrics

• lean_aggregation_window_width (Histogram): width derived per candidate. Climbs from 1 as the anchor climbs; pinned at the committee count means the window no longer restricts selection. Stuck at 1 while the network is aggregating means the pool holds nothing on this node's duty subnet, so it is only aggregating its own raw signatures.
• lean_aggregation_skipped_redundant_total: candidates handed to another duty subnet by the redundancy-skipping rotation. Only increments with the flag on.
• lean_aggregation_window_fallback_total: candidates whose windowed selection was not viable and fell back to the full committee set. A persistently rising value means the aggregator placement is too sparse for the committee count. Stays flat entirely under --skip-redundant-aggregation.

Test Plan

• cargo test --workspace --profile release-fast: 695 tests pass
• make lint clean, make fmt clean, make docs builds
• Four-aggregator reduction pinned end to end: round 1 produces four distinct proofs, round 2 reaches the full validator set
• Mid-climb widening covered at committee count 8, where the window grows but stays a proper subset
• The strided-placement regression has a test that fails without the fallback
• Single-committee no-op pinned
• Anchor selection pinned: a duty subnet the pool does not reach gets width 1, a sparse wide proof loses to a denser narrow one, a coverage tie falls to reach so pool order does not matter
• A skipped candidate hands its job budget to the next-best AttestationData
• Devnet: run all-ethlambda with attestation_committee_count = 4 and aggregators on distinct duty subnets, confirm lean_aggregation_window_width climbs rather than pinning at 4, fleet-wide aggregation CPU drops against a control, and finality is unaffected
• Devnet: confirm lean_aggregation_window_fallback_total stays flat on the intended placement

Notes for review

• The default proposer path (keep_best_proof_per_data) keeps one proof per AttestationData and drops the rest. Aggregators now emit proofs with distinct coverage rather than near-identical ones, so that path may drop more useful coverage than before. Bounded, since the window is sized to fit two children of the anchor's current reach, so a windowed proof is the same size as before; --enable-proposer-aggregation removes the exposure entirely. Declared out of scope here but worth measuring.
• With --skip-redundant-aggregation set and no explicit --aggregate-subnet-ids, every aggregator on a subnet-spanning topology derives duty subnet 0 and sits out in lockstep instead of taking turns. The node warns at startup when that combination is configured.
• The raw-signature guarantee is structural rather than a hard floor: "no anchor" means no peer covered our subnet. It is not airtight when two aggregators share a subnet and hold different slices of it, which no supported topology does today.

[github-actions]

🤖 Kimi Code Review

I'll review this PR focusing on the aggregation window feature for reducing redundant proof work across co-located aggregators.

Overall Assessment

This is a well-designed feature with good documentation, comprehensive tests, and careful attention to edge cases. The core logic for subnet-windowed aggregation and the optional redundancy-skipping rotation is sound. I found a few issues to address.

---

Issues Found

1. Potential Division by Zero in effective_width — crates/blockchain/src/aggregation.rs:912

while w > 1 && duty_subnet % w != slot % w {

Problem: When w becomes 0 from the halving loop (possible with width: u64::MAX or similar edge case, though window_width caps reasonably), the modulo % w would panic. The w > 1 guard prevents this in normal operation, but w is halved via w /= 2. Starting from width = 0 (floored to 1) or width = 1, this is safe. However, if width were u64::MAX, w /= 2 eventually reaches 1. The width.max(1) on line 910 handles the 0 case.

Verification: The floor on line 910 (width.max(1)) and w > 1 guard make this safe for all u64 inputs. No change needed, but worth noting the defense in depth is correct.

---

2. HashSet Iteration Order Dependency in resolve_aggregation_duty_subnet — bin/ethlambda/src/main.rs:855

.or_else(|| subscribed_subnets.iter().copied().min())

Problem: The comment correctly notes HashSet iteration order is unstable, and uses .min() which is deterministic. However, the min() call iterates all elements — this is O(n) where n is subscribed subnets. For typical subnet counts this is negligible, but the comment claims stability as the reason for min vs "arbitrary pick."

Actual issue: The .min() is indeed stable and deterministic, but there's a subtle concern: if HashSet implementation changes (e.g., different hash algorithm in future Rust version), min() still returns the same result since it does a full linear scan comparing elements. This is fine.

Suggested improvement: The code is correct but the comment slightly overstates the concern. No change required.

---

3. Missing Metrics Initialization Guard — crates/blockchain/src/metrics.rs:865-866

std::sync::LazyLock::force(&LEAN_AGGREGATION_NARROWED_TOTAL);
std::sync::LazyLock::force(&LEAN_AGGREGATION_WINDOW_FALLBACK_TOTAL);

Problem: These are correctly added to init(). However, LEAN_AGGREGATION_WINDOW_WIDTH on line 879 is initialized but not added to the init() function's force list. Wait — checking again... Line 879 has it. All three new metrics are properly initialized. ✓

---

4. CandidateWindow narrowed Field Logic — crates/blockchain/src/aggregation.rs:256

if primary.is_some() || derived.narrowed {
    return primary;
}

Problem: This is correct per the design doc: when narrowed is true, the empty result is intentional (skip redundant work), so no fallback. But consider: what if primary is Some but the job is low-quality? The fallback only triggers when primary is None. This matches the documented behavior.

However, there's a subtle issue: derived.narrowed being true means effective_width reduced below base_width. But primary being Some means resolve_job found a viable job at the narrowed width. The early return is correct — we don't fallback when we already have a job.

---

5. SubnetWindow::contains_subnet Wrap-Around Arithmetic — crates/blockchain/src/aggregation.rs:780-786

let offset = if subnet >= self.start {
    subnet - self.start
} else {
    self.committee_count - self.start + subnet
};

Problem: This is correct cyclic distance. But consider: committee_count = 0 is handled above. For committee_count > 0, start is reduced mod committee_count in new(). So start < committee_count and subnet < committee_count. The arithmetic is safe from overflow.

Edge case: self.committee_count - self.start + subnet when start = 0 gives committee_count + subnet, but subnet < committee_count so this is < 2*committee_count. The offset < self.width check still works, but offset can exceed committee_count. That's fine since width <= committee_count, so offset >= committee_count will always fail offset < width.

Actually, when start = 0 and subnet < start is false (since subnet >= 0), we don't take the else branch. Correct.

---

6. Test window_fallback_recovers_a_merge_a_strided_placement_would_drop — crates/blockchain/src/aggregation.rs:2326

assert_eq!(
    snapshot.jobs[0].coverage(),
    HashSet::from([5, 7, 13, 15]),
    "coverage matches the unwindowed selection's last-two-by-pool-order tie-break"
);

Problem: The comment explains the tie-breaking behavior of max_by_key (last element wins on ties). However, select_proofs_greedily uses:

.max_by_key(|&(_, score)| score)

Per Rust docs, max_by_key returns the first maximum on ties for stable iterators, but remaining.iter().enumerate() is a stable iterator over a Vec. Actually, Vec::iter is stable, so the first element with max score wins. But the comment says "last" and the test expects [5, 13, 7, 15] which corresponds to pool order indices 2 and 3 (the last two).

Wait — let me re-read. The pool is [0,8], [3,11], [5,13], [7,15]. All have score 2 (2 validators each) in a full window. max_by_key on a stable iterator returns the first maximum. So it should pick index 0 ([0,8]) first, then index 1 ([3,11]), giving coverage {0, 8, 3, 11}.

But the test expects {5, 7, 13, 15} which is indices 2 and 3. This contradicts standard Iterator::max_by_key behavior!

Critical bug or documentation error: Either the comment is wrong about "last" and there's a different tie-break, or the test is incorrect. Let me re-check...

Actually, looking more carefully: the select_proofs_greedily function processes new_proofs before known_proofs. In the fallback path, all proofs are in new_proofs. The remaining Vec is iterated with .iter().enumerate().

Wait — I need to re-verify Rust's max_by_key behavior. From the standard library docs: "If several elements are equally maximum, the last element is returned." Let me check... Actually no, for Iterator::max_by_key it's implementation-defined for unstable iterators, but for Slice::iter() which is stable, it's the first element.

Hmm, but Vec::iter() yields &T and max_by_key on Iterator — let me be more careful. The Rust docs for Iterator::max_by_key say: "Returns the element that gives the maximum value from the specified function." For equal keys, it returns the first such element (for stable orderings).

But the test expects and documents "last" behavior. Let me check if std changed this or if itertools differs...

Actually, I believe the test author may be using itertools::Itertools::max_by_key which does return the last maximum, or there may be a custom implementation. Or perhaps I'm wrong about std.

Action needed: Verify which max_by_key is in scope. The file imports don't show itertools. If this is std::iter::Iterator::max_by_key, the test expectation may be wrong, or the behavior may differ from what I recall.

Given this is consensus-critical (affects which proofs get aggregated), this needs explicit verification. The test passes in CI presumably, so either std behavior matches "last" or there's something else at play.

---

7. resolve_job_with_window_fallback Metrics Double-Counting — crates/blockchain/src/aggregation.rs:636

metrics::inc_aggregation_window_fallback();

Problem: This increments before attempting the fallback. If the fallback also returns None, we still count it as a fallback. This seems intentional (we attempted a fallback), but the metric name says "fell back to the full committee set" which implies success. The docs say "Candidates whose windowed selection was not viable and fell back" — "fell back" could mean attempted or succeeded.

Minor: consider whether to count only successful fallbacks. Current behavior is defensible but should match docs.

---

8. record_window_metrics Called Per-Candidate — crates/blockchain/src/aggregation.rs:326

record_window_metrics(&derived);

Problem: This is called for every candidate, including payload-only candidates in the second loop. The metrics will reflect all candidates considered, not just those that become jobs. This matches the histogram description ("derived for one aggregation candidate") but means the count of width observations exceeds the count of jobs. Documented behavior is correct.

---

9. AggregationWindowConfig Derives Copy — crates/blockchain/src/aggregation.rs:198

#[derive(Clone, Copy, Debug)]
pub struct AggregationWindowConfig {

Problem: Contains only u64 and bool, so Copy is fine. No issue.

---

10. CLI Flag Default Value — bin/ethlambda/src/cli.rs:112

#[arg(long, default_value = "false", requires = "is_aggregator")]
pub(crate) skip_redundant_aggregation: bool,

Problem: default_value = "false" for bool is redundant (bool defaults to false), but explicit is fine. The requires = "is_aggregator" is correct.

---

Summary

Item	Severity	File:Line	Description
1	Info	aggregation.rs:912	Division by zero properly guarded; no change needed
2	Info	main.rs:855	HashSet + min() is correct; comment slightly overstates
3	None	metrics.rs	All metrics properly initialized ✓
4	None	aggregation.rs:256	narrowed logic correct per design
5	None	aggregation.rs:780	Wrap-around arithmetic correct
6	High	aggregation.rs:2326	Verify max_by_key tie-breaking behavior — test expects "last" but std may return "first"
7	Low	aggregation.rs:636	Fallback metric counts attempts not successes; clarify if intended
8	None	aggregation.rs:326	Per-candidate metrics correct per docs
9	None	aggregation.rs:198	Copy derive correct
10	None	cli.rs:112	Explicit false default acceptable

Recommended Changes

Item 6 — Verify tie-breaking behavior

Add an explicit comment or use a deterministic tie-breaker:

// In select_proofs_greedily, aggregation.rs:935
// If scores tie, prefer the proof with more total participants, then
// lower pool index for determinism:
.max_by_key(|(i, p)| {
    let score = in_window_new_coverage(p, &covered, window);
    (score, p.participant_count(), !(*i == 0)) // or similar total order
})

Or if the current behavior is verified correct, document which max_by_key implementation provides the "last wins" semantics.

Item 7 — Clarify fallback metric (optional)

// aggregation.rs:636 — consider:
if resolve_job(...).is_some() {
    metrics::inc_aggregation_window_fallback();
    // return it
}

Or update metric description to "attempted fallback" rather than "fell back."

---

Positive Notes

1. Excellent test coverage: The 400+ lines of new tests cover normal operation, edge cases (zero committees, wrap-around, empty pools), and the redundancy rotation.
2. Good documentation: Architecture doc explains the feature clearly; metrics doc explains how to interpret the new histogram.
3. Defensive programming: committee_count == 0 guards throughout, width.max(1) floor, explicit modulo reduction of duty subnet.
4. Clean separation: record_window_metrics kept pure; SubnetWindow is a well-encapsulated primitive.
5. Backward compatibility: vacuous_window_config() ensures existing tests continue to validate the same behavior with committee_count = 1.

---

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

[github-actions]

🤖 Codex Code Review

1. crates/blockchain/src/aggregation.rs: resolve_job_with_window_fallback() suppresses the full-width fallback for every derived.narrowed case, but narrowed does not mean “this duty subnet is sitting out.” It also covers cases where the node legitimately owns a narrower width after rotation (base_width=4, effective_width=2, for example). In a sparse/strided proof pool, that owned narrower window can still leave only one scoring child, primary becomes None, and the code drops the merge entirely instead of recovering via the fallback. That is a liveness regression in aggregation coverage, and it weakens the exact placement pattern this PR says it protects against. The fallback gate needs to distinguish “unowned wider level, deliberate sit-out” from “owned narrowed level, but contiguous window was too sparse.”

I did not find other obvious correctness or security issues in the touched paths. I could not run the Rust tests here because cargo/rustup attempted to write under /home/runner/.rustup, which is read-only in this environment.

---

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

[github-actions]

🤖 Claude Code Review

Review: Subnet-windowed aggregation (PR #613)

Went through the diff in detail (aggregation.rs, lib.rs, cli.rs, main.rs, metrics.rs, docs) and hand-verified the trickiest arithmetic (effective_width's rotation, SubnetWindow wraparound, window_width/subnet_reach edge cases at committee_count == 0) against the accompanying unit tests. This is a clean, well-reasoned change. Highlights of what I checked and why it holds up:

Consensus safety — confirmed the core safety claim: a windowed aggregate still binds exactly raw_ids ∪ accepted_child_ids, and the window is used purely as a scoring lens in select_proofs_greedily/in_window_new_coverage, never as an admission filter. select_proofs_greedily still adds a selected proof's entire participant set to covered (in-window or not), which keeps marginal-coverage scoring honest across rounds — this is exactly right and well tested (select_proofs_greedily_covers_out_of_window_participants_of_a_chosen_proof).

Coverage regression guard — resolve_job_with_window_fallback correctly falls back to a full-committee window only when the primary windowed attempt fails and the narrowing wasn't a deliberate --skip-redundant-aggregation sit-out (derived.narrowed). This distinction is important and correctly implemented: it prevents the fallback from defeating the whole point of --skip-redundant-aggregation while still protecting against coverage loss from a sparse/strided proof pool (window_fallback_recovers_a_merge_a_strided_placement_would_drop is a good regression test for this).

Rotation correctness — hand-verified effective_width's halving loop (duty_subnet % w == slot % w) against effective_width_rotates_which_aggregator_works_widest's expected matrix for width=4, 8 duty subnets, slot 0; it matches exactly. The loop always terminates at w == 1 (universally owned), so raw-signature aggregation can never be starved even with --skip-redundant-aggregation on and a non-power-of-two committee count.

Zero-division guards — committee_count == 0 is unreachable in production (enforced via eyre::ensure!(attestation_committee_count >= 1, ...) in main.rs:213), but every primitive (SubnetWindow::new, subnet_reach, window_width) still degrades gracefully to "no subnet structure" rather than panicking, and this is tested (zero_committee_count_disables_the_subnet_scheme). Good defensive design even though currently dead code from the CLI's perspective.

Out-of-range duty subnet — --aggregate-subnet-ids has no upper-bound validation, so window_for_candidate correctly reduces duty_subnet % committee_count before the rotation (not just inside SubnetWindow::new), and the test (window_for_candidate_reduces_an_out_of_range_duty_subnet) specifically catches the case where reducing after would desync the rotation from its reduced twin — a subtle bug class that's actually tested against.

Minor observations (non-blocking):

1. bin/ethlambda/src/main.rs:293 — the startup warning for --skip-redundant-aggregation without an explicit duty subnet only fires when aggregate_subnet_ids.is_none(). If two co-located aggregators each pass a single, identical --aggregate-subnet-ids value (e.g. both --aggregate-subnet-ids 0), they'll silently collide on the same duty subnet with no warning, same failure mode as the unset case. Worth widening the check or documenting that operators must ensure distinct first values, not just "any assignment."
2. crates/blockchain/src/aggregation.rs (subnet_reach) allocates a fresh HashSet per candidate per session; fine at current scale (bounded proof pools, MAX_AGGREGATION_JOBS = 2), but if committee counts or candidate counts grow significantly this is a spot to revisit — not a concern for the current PR.
3. Formatting nit: lines like let derived = window_for_candidate(&new_proofs, &known_proofs, current_slot, window_config); in snapshot_aggregation_inputs (around the two call sites) look close to the rustfmt width limit — make fmt in CI will catch/fix this if it hasn't already, not worth a manual look.

Overall: correctness reasoning is sound, the test suite (especially four_aggregators_climb_from_per_subnet_proofs_to_full_coverage and window_fallback_recovers_a_merge_a_strided_placement_would_drop) exercises exactly the scenarios that would reveal a subtle bug in this kind of scheme, and the docs (architecture.md, metrics.md) accurately describe the implemented behavior, including the deliberate trade-offs. No blocking issues found.

---

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