feat(aggregation): score recursive aggregation through a per-aggregator subnet window by MegaRedHand · Pull Request #613 · lambdaclass/ethlambda

MegaRedHand · 2026-09-10T13:30:38Z

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. The node refuses to start on an --aggregate-subnet-ids value at or above the committee count: subscriptions use the raw id while the window reduces it, so an out-of-range value would subscribe to a topic no validator publishes on and aggregate as a subnet the node does not listen to.
--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.
- Deployment precondition: every subnet below the committee count needs an aggregator holding it as its duty subnet. Ownership rotates, but a width can have no owner in a slot on a sparser placement even with every node healthy: at duty subnets {0, 2} and committee count 4, nothing owns width 4 in an odd slot, and the fallback is off, so that merge level is dropped for the slot. Leave the flag unset on a placement that does not cover every subnet.

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.

That emptiness is a timing expectation, not an invariant. A peer's aggregate for the current slot landing before this node's snapshot, under clock skew or a session started early via EarlyAggregationCheck, gives the current-slot candidate an anchor and a width of 2; under --skip-redundant-aggregation the duty subnets that do not own width 2 then sit that slot out and their raw signatures miss the next block. On the intended topology (distinct duty subnets, distinct subscriptions) a round-one peer proof never touches this node's subnet, so it holds anyway. Overlapping subscriptions are where it fails.

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: merges the window would have dropped, recovered by retrying selection at the full committee set. Counts recoveries, not attempts: a candidate no window could have made viable, a lone raw signature or a single-proof group, never reaches the retry. A persistently rising value therefore means the aggregator placement is too sparse for the committee count. Stays flat entirely under --skip-redundant-aggregation.

Test Plan

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.

Groundwork for giving each aggregator a distinct slice of the shared proof pool. Pure functions with no call sites yet.

Sizes subnet_reach's working set by participants rather than by the uncapped committee count, drops a provably dead branch in contains_subnet, canonicalizes the stored width so derived equality matches containment, and covers the zero-committee guards.

Values a candidate child by the in-window validators it newly covers rather than by total coverage, so aggregators on different duty subnets pick different children. Every caller still passes the vacuous single-committee window, so behavior is unchanged until the window is derived for real.

The rule was justified by a raw-signature trim that reads a value resolve_job discards. It actually keeps the marginal-coverage score honest across greedy rounds. Also folds the zero-score guard into the search, dropping a panic path, and covers the known-proof fallback.

Narrows an aggregator to the widest level it owns in the slot, rotating the owner so no duty subnet is permanently the one sitting out. Not wired to a caller yet.

Restores the width floor as a property of the function rather than a precondition on a caller that does not exist yet, matching how the sibling primitives answer degenerate inputs. Documents that the ladder truncates at odd committee counts and that a ragged wrap can hand two duty subnets overlapping windows.

Width comes from the best reach in the candidate's own proof pool, so every aggregator derives the same width for a data root and the merge tree stays in step. The duty subnet is still a placeholder pending the CLI wiring.

A hardcoded duty subnet gave every aggregator the same window, which both kept the duplicated work this change exists to remove and made proofs outside that window unmergeable as children. Also registers the window metrics at startup and observes the width per candidate, so the counter reads 0 rather than absent while the rotation is off.

Moves the duty subnet and the redundancy-skipping flag onto BlockChainConfig so the binary can supply them, with the previous inline fallback relocated to main.rs unchanged.

Nothing validates the upper bound of --aggregate-subnet-ids, and at a width that does not divide the committee count an out-of-range duty subnet rotates on different slots from its reduced twin. Also marks the two main.rs placeholders that the CLI wiring will replace.

The duty subnet is the first --aggregate-subnet-ids value, so operators place co-located aggregators on different subnets deliberately rather than having every node derive the same one from its subscriptions.

The first --aggregate-subnet-ids entry became load-bearing without its help text saying so, and a node that silently fell back to the lowest subscribed subnet was indistinguishable from one an operator placed deliberately.

Round 1 produces four proofs with distinct coverage, round 2 merges them to the full validator set. Also pins that a single committee ignores the duty subnet, so the window is a no-op there.

The width pins once the best pool proof reaches half the committees, not all of them, so a pinned sample means the window no longer restricts selection rather than that the pool is saturated.

The test guarding the pre-rotation reduction passed without it, since SubnetWindow::new's own fold masked the difference at a committee count the width divides. Also covers the redundancy-skipping path end to end.

… a merge A window is a contiguous run of subnets 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. The window can now only improve on that selection, never regress below it.

A derived duty subnet is identical on every node whose validators span all subnets, so the rotation narrows every aggregator in lockstep rather than taking turns, and the widest level gets no producer at all in most slots.

One session per slot, and the current slot's pool is empty at snapshot time, so a data root gets about one windowed merge rather than the multi-round climb the comments implied. The widening still matters across slots for a data root that stays live.

github-actions · 2026-09-10T13:32:18Z

🤖 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

Excellent test coverage: The 400+ lines of new tests cover normal operation, edge cases (zero committees, wrap-around, empty pools), and the redundancy rotation.
Good documentation: Architecture doc explains the feature clearly; metrics doc explains how to interpret the new histogram.
Defensive programming: committee_count == 0 guards throughout, width.max(1) floor, explicit modulo reduction of duty subnet.
Clean separation: record_window_metrics kept pure; SubnetWindow is a well-encapsulated primitive.
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 · 2026-09-10T13:32:18Z

🤖 Codex Code Review

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 · 2026-09-10T13:34:52Z

🤖 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):

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."
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.
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

… proof The width was derived from the widest proof anywhere in the candidate's pool. That made a single sparse proof, one validator in each of many subnets, set the width to the full committee count for every aggregator and switch the window off network-wide. It also said nothing about whether the aggregator had any stake in the level it was being sized for. Derive it from the *anchor* instead: the largest-coverage proof in the pool that touches the aggregator's own duty subnet. Coverage rather than reach picks it, so a sparse proof no longer dominates. Requiring it 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, and the narrowest window then leaves it aggregating those rather than merging proofs it cannot add to. The width is no longer uniform across the network, so two aggregators reading one lopsided pool can end up with nesting windows instead of tiling ones. That costs a round of climbing, not correctness. Also change what --skip-redundant-aggregation does with a level it does not own. It used to halve the width until it owned one, which at any committee count above 4 lands on a level nested inside an owner's window and duplicates the owner's merge anyway. It now sits the candidate out entirely, so the job budget goes to the next-best AttestationData. The full-width fallback is disabled under the flag for the same reason: every width below the committee count has several owners, so retrying there would rebuild the duplication the flag exists to remove. lean_aggregation_narrowed_total becomes lean_aggregation_skipped_redundant_total, counting candidates handed to another duty subnet rather than windows narrowed.

github-actions · 2026-09-15T20:11:39Z

🤖 Kimi Code Review

I'll review this PR for the subnet-windowed aggregation feature, focusing on correctness, security, performance, and Rust best practices.

High-Level Assessment

This is a well-structured feature adding subnet-windowed aggregation to prevent duplicate proof generation across co-located aggregators. The code is generally clean with good test coverage and clear documentation. I found several issues ranging from minor to moderate severity.

Issues Found

1. Potential Division by Zero in `owns_width` — Not Fully Protected (Critical)

File: crates/blockchain/src/aggregation.rs, lines 1004–1010

pub(crate) fn owns_width(duty_subnet: u64, slot: u64, width: u64) -> bool {
    let width = width.max(1);
    duty_subnet % width == slot % width
}

The width.max(1) protection is good, but duty_subnet and slot can be any u64. When width == 1, this always returns true (correct per design). However, consider: what if width is u64::MAX? Then duty_subnet % width and slot % width are well-defined in Rust (no panic), but the function's behavior with width > committee_count is mathematically inconsistent with the tiling description.

More importantly, at line 1004, the function is pub(crate) but the docstring says "Floor at the narrowest window rather than trusting the caller" — yet width here is the window width, not raw committee count. If window_width returns a value > committee_count due to a bug, owns_width would use that oversized width, breaking the tiling invariant.

Suggestion: Add a debug assertion or clamp width to committee_count in window_for_candidate before passing to owns_width, since the ownership semantics only make sense within the committee count.

2. Inconsistent Redundancy of `committee_count == 0` Checks (Moderate)

File: crates/blockchain/src/aggregation.rs, multiple locations

The committee_count == 0 guard appears in:

SubnetWindow::new (line ~780)
SubnetWindow::contains_subnet (line ~798)
subnet_reach (line ~830)
window_width (line ~870)
window_for_candidate (line ~715)

This is defensive but scattered. Worse, at line 715, window_for_candidate does:

let duty_subnet = if config.committee_count == 0 {
    0
} else {
    config.duty_subnet % config.committee_count
};

But SubnetWindow::new also reduces start modulo committee_count. This double-reduction is harmless but redundant. However, if config.committee_count == 0, duty_subnet becomes 0, then SubnetWindow::new(0, width, 0) will set start = 0 (correct), but the width path in new does width.min(committee_count) = width.min(0) = 0, creating a zero-width window.

Wait — let me re-check. At line ~785:

let width = if committee_count == 0 {
    width
} else {
    width.min(committee_count)
};

So with committee_count == 0, width stays as passed. But window_width(0, 0) returns 1 (line ~872), so width would be 1. Then SubnetWindow::new(0, 1, 0) has start=0, width=1, committee_count=0.

Then contains_subnet at line ~798: committee_count == 0 returns true immediately. So the window "contains everything" despite width 1. This is consistent with the "no subnet structure" semantics, but the width field is misleading.

Suggestion: Centralize the committee_count == 0 semantics in one place or document the invariant more clearly. The current behavior is correct but fragile to future changes.

3. `resolve_job_with_window_fallback` Double-Clones `hashed` (Minor/Performance)

File: crates/blockchain/src/aggregation.rs, lines 1050–1075

let primary = resolve_job(
    hashed.clone(),  // clone 1
    validator_sigs,
    new_proofs,
    known_proofs,
    validators,
    window,
);
// ...
resolve_job(
    hashed,  // move (but already cloned above, so this is the second HashedAttestationData)
    // ...
)

HashedAttestationData likely contains an AttestationData and a cached hash. Cloning it twice is wasteful. Since resolve_job takes hashed by value, and the fallback path only runs when primary.is_none(), you could avoid the first clone:

let primary = resolve_job(
    hashed.clone(),  // only needed because we might use it again
    // ...
);

Actually, looking more carefully: hashed is moved into the first resolve_job only if we don't enter the fallback. In the fallback path, hashed was already moved. So the .clone() is necessary for the fallback path to work.

Suggestion: Restructure to avoid clone on the common path:

let primary = resolve_job(
    hashed,  // move
    validator_sigs,
    new_proofs,
    known_proofs,
    validators,
    window,
);
if primary.is_some() || config.skip_redundant {
    return primary;
}
// Need hashed again — but we moved it. So we need to either:
// 1. Clone only when fallback is needed (rare), or
// 2. Restructure resolve_job to take &HashedAttestationData

Since resolve_job only uses hashed to call .root() and .clone() into the result, it could take &HashedAttestationData instead. This would eliminate both clones.

4. `resolve_job` Signature Change Not Reflected in All Call Sites Documentation (Minor)

File: crates/blockchain/src/aggregation.rs, line ~560

The docstring for resolve_job says:

/// 2. Runs [`select_proofs_greedily`] seeded with that `covered` set...

But doesn't mention the new window: &SubnetWindow parameter or how it affects selection. The select_proofs_greedily docstring is updated, but resolve_job's isn't fully consistent.

5. Metrics Registration Panic on Duplicate (Minor — Pre-existing Pattern)

File: crates/blockchain/src/metrics.rs, lines 316–330

The LazyLock::new(|| register_int_counter!(...).unwrap()) pattern panics if the metric is already registered. This is pre-existing code, but the new metrics follow the same pattern. In a library context or test scenarios with multiple initializations, this could panic.

Suggestion: Not a new issue, but consider register_int_counter!(..., opts! { ... }).unwrap_or_else(|_| ...) or using try_register in future refactors.

6. Test `window_fallback_recovers_a_merge_a_strided_placement_would_drop` Has Brittle Assertion (Moderate)

File: crates/blockchain/src/aggregation.rs, lines ~2680–2700

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"
);

This asserts specific validator IDs based on tie-breaking behavior of max_by_key. The comment acknowledges this depends on "std's documented tie-breaking" for max_by_key — but Rust's max_by_key stability is not actually documented to break ties toward the last element. The standard library docs say:

If several elements are equally maximum, the last element is returned.

Actually, checking: Iterator::max_by_key docs say "Returns the element that gives the maximum value from the specified function." The stability note for max is: "If several elements are equally maximum, the last element is returned." This is documented.

However, this makes the test fragile to changes in MAX_AGGREGATION_CHILDREN or the selection algorithm. If someone changes MAX_AGGREGATION_CHILDREN to 3, this test breaks.

Suggestion: Add a comment explicitly linking this test to MAX_AGGREGATION_CHILDREN = 2, or better, parameterize the test or assert properties rather than exact sets.

7. `insert_test_block` Helper Missing in Diff (Potential Compilation Issue)

File: crates/blockchain/src/aggregation.rs, line ~2560

The test a_skipped_candidate_hands_its_budget_to_the_next_best calls:

insert_test_block(
    &mut store,
    hashes[STALE_SLOT as usize],
    STALE_SLOT,
    hashes[STALE_SLOT as usize - 1],
);

This helper is not visible in the diff. If it's a pre-existing test helper, fine. If it was supposed to be added, this is a bug. Please verify this compiles.

8. `HashSet` Iteration Order in `resolve_aggregation_duty_subnet` (Already Handled, Good)

File: bin/ethlambda/src/main.rs, lines ~855–860

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

Good: explicitly uses .min() instead of .next() to avoid HashSet iteration order non-determinism. The comment at line ~849 documents this correctly.

9. CLI Warning for `skip_redundant_aggregation` with Derived Duty Subnet (Good Practice)

File: bin/ethlambda/src/main.rs, lines ~290–300

The warning when --skip-redundant-aggregation is set without explicit --aggregate-subnet-ids is excellent operational hygiene. This prevents the "lockstep sit-out" problem described in the docstring.

10. `AggregationWindowConfig` Derives `Copy` but Contains `u64` Fields (Fine, but Note)

File: crates/blockchain/src/aggregation.rs, line ~190

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

All fields are u64 and bool, so Copy is appropriate and efficient. No issue.

11. Potential Integer Overflow in `anchor_reach.saturating_mul(2)` (Already Safe)

File: crates/blockchain/src/aggregation.rs, line ~872

anchor_reach.saturating_mul(2).min(committee_count)

Good use of saturating_mul to prevent overflow. Since anchor_reach <= committee_count (from subnet_reach which caps at committee_count), and committee_count is a CLI parameter, overflow is theoretically possible with malicious input but handled safely.

12. Docstring Inconsistency: "width below the committee count has several owners" (Minor)

File: crates/blockchain/src/aggregation.rs, line ~1040

/// Every width below the committee count has several owners

This is not strictly true: if committee_count = 3 and width = 2, there are ceil(3/2) = 2 owners per slot (for slot 0: subnets 0, 2; actually let me check: owns_width(d, s, w) is d % w == s % w. For w=2, s=0: d ∈ {0, 2}. That's 2 owners out of 3 subnets. For w=2, s=1: d ∈ {1}. That's 1 owner. So "several owners" is not guaranteed for all widths.

The docstring at line ~1040 is in the context of explaining why fallback is disabled under --skip-redundant-aggregation. The point is that most widths have multiple owners, not all. The phrasing is slightly imprecise but acceptable in context.

13. Missing `#[cfg(test)]` on Test Helpers (Minor)

File: crates/blockchain/src/aggregation.rs, lines ~2480+

Helpers like store_with_payload_only_proofs, window_test_store, window_test_att_data, insert_payload_only_candidate are defined in the tests module (good), but vacuous_window_config at line ~1230 is also in tests. All good.

14. `subnet_reach` Uses `HashSet` with Early Exit — Good Optimization

File: crates/blockchain/src/aggregation.rs, lines ~830–845

The early exit when seen.len() == committee_count is a good optimization. However, for large committee_count values, the HashSet allocation could be expensive. Consider using a bitvec or FixedBitSet if this becomes hot.

Actually, with the early exit and typical committee counts (4–64), this is fine.

Summary Table

Item	Severity	File	Line	Description
1	Moderate	`aggregation.rs`	~1004	`owns_width` semantics with unclamped width
2	Minor	`aggregation.rs`	Multiple	Scattered `committee_count == 0` checks
3	Minor	`aggregation.rs`	~1050	Double `HashedAttestationData` clone in fallback
4	Minor	`aggregation.rs`	~560	`resolve_job` docstring missing `window` param docs
5	Minor (pre-existing)	`metrics.rs`	~316	`unwrap()` on metric registration
6	Moderate	`aggregation.rs`	~2680	Brittle test assertion on tie-breaking
7	Verify	`aggregation.rs`	~2560	`insert_test_block` helper existence

Positive Acknowledgments

Excellent test coverage with property-based reasoning (tiling, rotation, fallback)
Good documentation in architecture.md explaining the feature's trade-offs
The metered_window_for_candidate separation keeps pure logic testable
Warning log for dangerous CLI flag combination is operator-friendly
subnet_reach early-exit optimization is well-commented
AggregationWindowConfig grouping is cleaner than passing loose parameters

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

github-actions · 2026-09-15T20:12:35Z

🤖 Codex Code Review

Findings:

aggregate_subnet_ids can now put networking and aggregation on different subnets. resolve_aggregation_duty_subnet keeps the raw first configured ID (bin/ethlambda/src/main.rs:282), and window_for_candidate later reduces it modulo committee_count before ownership/selection (crates/blockchain/src/aggregation.rs:248). But subscriptions still use the raw IDs unchanged (crates/net/p2p/src/lib.rs:196, crates/net/p2p/src/lib.rs:413). Example: with --attestation-committee-count 4 --aggregate-subnet-ids 5, this node aggregates as duty subnet 1 but only subscribes to topic 5, so it misses the subnet it thinks it owns. This needs validation or normalization at startup, before the value is shared with both P2P and aggregation.
--skip-redundant-aggregation can legitimately leave a width with zero producers even when every configured node is healthy. The skip is unconditional once owns_width fails (crates/blockchain/src/aggregation.rs:260, crates/blockchain/src/aggregation.rs:953), but the CLI/help text only frames this as “few producers” / “a node that is down or late” (bin/ethlambda/src/cli.rs:95, bin/ethlambda/src/main.rs:291). With duty subnets {0,2} and a candidate at width 4, slots 1 and 3 have no owner at all, so that merge level is dropped for the slot. If that tradeoff is intended, it should be documented as a hard deployment precondition; otherwise the flag needs stronger guardrails.

The aggregation tests added here are otherwise thoughtful and cover the windowing logic well. I could not run the test suite in this environment because cargo/rustup need writable home directories and the sandbox exposes them read-only.

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

github-actions · 2026-09-15T20:13:50Z

🤖 Claude Code Review

All call sites are consistently updated. Comprehensive review complete — this PR is well-tested and defensively coded (all division/modulo-by-zero paths guarded). Findings below are minor.

Review: Subnet-windowed aggregation (PR 613)

Overall

This is a well-designed, well-documented change. The core algorithm (contiguous cyclic window as a scoring lens rather than a filter, anchor-derived width, full-window fallback, redundancy-skip rotation) is sound, and the "no consensus impact" claim holds: a windowed aggregate is still raw_ids ∪ accepted_child_ids with bits matching that set, so validity is untouched — only which children get merged changes. Zero/div-by-zero edge cases (committee_count == 0, width == 0) are all explicitly guarded rather than left to panic. Test coverage is thorough, including the strided-placement regression and the redundancy-skip rotation.

Minor observations

bin/ethlambda/src/main.rs:282-297 — logged/stored duty subnet is unreduced. resolve_aggregation_duty_subnet and the startup info!/warn! log the raw first --aggregate-subnet-ids value, but window_for_candidate (crates/blockchain/src/aggregation.rs:235-239) reduces it % committee_count before use. If an operator sets a duty subnet ≥ committee_count, the startup log and BlockChainConfig/BlockChainServer field will show the raw (out-of-range) value while the actual window/rotation logic silently uses the reduced one. Not a correctness bug (well-tested: window_for_candidate_reduces_an_out_of_range_duty_subnet), but it could confuse an operator cross-referencing logs against lean_aggregation_window_width. Consider logging the reduced value too, or validating the CLI input against attestation_committee_count at parse time.
crates/blockchain/src/aggregation.rs:394-424 (resolve_job_with_window_fallback) — doubled work on the common miss path. Every time the windowed selection is not viable (documented as expected under a strided placement), resolve_job reruns from scratch (rebuilding raw_by_id, re-running select_proofs_greedily, re-resolving child pubkeys). This is explicitly called out as an accepted cost ("store-free, so trying it twice is cheap"), which is reasonable given MAX_AGGREGATION_JOBS = 2 bounds the blast radius — just flagging it as something to watch if resolve_child_pubkeys/pubkey resolution ever becomes non-trivial.
crates/blockchain/src/aggregation.rs:510-527 (subnet_reach) — pool-proof cost scales with participant count, uncapped by config. committee_count has no CLI upper bound (noted in the code comment itself), so a very large configured value defeats the early-exit optimization for wide proofs, degrading to a full scan per proof per candidate per session. This is operator-controlled configuration, not attacker input, so severity is low, but worth a sanity bound on attestation_committee_count if one doesn't already exist elsewhere.
Anchor-manipulation surface is closed correctly. Worth confirming explicitly (not a finding, just a check I wanted to make visible in review): widening the anchor requires a proof with genuinely high coverage touching the duty subnet, and proofs in the pool are already-verified aggregates (store.existing_proofs_for_data), so an adversary can't cheaply inflate anchor_reach/window_width with forged bits — this closes the "sparse wide proof" gaming vector the commit history shows was already found and fixed once (e58eb206).
Nit — docs/architecture.md and docs/metrics.md updates are accurate and match the implementation (width formula, fallback conditions, counter semantics all cross-checked against the code). No corrections needed there.

No blocking issues found. The fallback-to-full-window safety net and the disabling of that fallback under --skip-redundant-aggregation are both correctly implemented and match their documented rationale.

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

…w path Cleanup pass over the subnet-window code, no behaviour change. `anchor_key` walked a proof's bits twice: once for coverage, then again inside `subnet_reach` for the reach. `validator_indices` visits every index up to the bitfield's length rather than just the set ones, so the second pass doubled the per-proof cost for every anchor candidate. Both numbers now fall out of one pass, which also removes the hand-rolled modulo membership test in favour of the subnet set the pass already builds. `subnet_reach` had no other caller, so it goes; its tests move onto `anchor_key`. Greedy child selection cloned each chosen `SingleMessageAggregate`, proof bytes included, only for `resolve_child_pubkeys` to clone those bytes again. Selection now hands back borrows and the accepted children clone once. This matters more than it used to: when a window declines a merge, `resolve_job_with_window_fallback` runs the whole selection a second time. The two candidate loops in `snapshot_aggregation_inputs` carried an identical copy of the store-read, window-derive, resolve sequence; `build_candidate` now holds it once. The fallback's full-width window gets a named `SubnetWindow::full` constructor, dropping a `.max(1)` that could never fire: at a committee count of 0 both containment methods return before reading the width.

pablodeymo · 2026-09-16T15:49:25Z

+    if primary.is_some() || config.skip_redundant {
+        return primary;
+    }
+    metrics::inc_aggregation_window_fallback();


This retry runs, and the counter increments, whenever the first resolve_job returns None, but that also happens for reasons the window had nothing to do with: a gossip group holding a single raw signature and no children (every minority or late vote), or a candidate whose derived width already equals the committee count, where SubnetWindow::full scores identically and cannot recover anything.

So lean_aggregation_window_fallback_total will climb on any healthy deployment, which contradicts the metrics doc ("should stay at or near zero on a well-tiled deployment"), and the second selection pass runs on every non-viable candidate.

Suggestion:

return early when window.width() == config.committee_count (the retry is a no-op there), and

increment the counter only when the full-width retry actually yields a job, since that is the event the metric claims to count.

Right on both counts, fixed in f33b308.

The retry now returns early in the two cases where it is provably a no-op, and the counter only increments when the retry yields a job:

window.width() >= committee_count: a window as wide as the committee admits every subnet, so SubnetWindow::full is the same lens. >= rather than == so a committee_count of 0 folds in too.

empty proof pool: no child to admit at any width. This is the single-raw-signature case you named, and it is the ordinary shape of a minority or late vote.

What is left, a group whose pool is non-empty but whose windowed selection came up with one child, is exactly what the fallback exists for, so the retry there is genuine work. resolve_job can still decline it, hence counting recoveries rather than attempts.

Pulled the predicate out as fallback_can_recover so the three cases are unit-testable without a metrics registry. The metric's description and docs/metrics.md now say "recoveries, not attempts", which makes the "at or near zero on a well-tiled deployment" reading true.

pablodeymo · 2026-09-16T15:49:25Z

    // receiver-count guard in `emit` makes every emission a no-op.
    let events = EventBus::default();

+    let aggregation_duty_subnet = resolve_aggregation_duty_subnet(


The duty subnet is later reduced modulo committee_count in window_for_candidate, but the subscription set built at line 263 keeps the raw --aggregate-subnet-ids values and the swarm subscribes to those raw topics.

With --attestation-committee-count 4 --aggregate-subnet-ids 5 this node subscribes to topic 5, which no validator publishes on, and aggregates as duty subnet 1, which it does not listen to. The startup log prints 5, so the mismatch is invisible to the operator.

Rather than reducing silently, reject any --aggregate-subnet-ids value at or above the committee count here, next to the existing attestation_committee_count >= 1 check. That fixes the pre-existing dead subscription too, and the modulo in window_for_candidate plus window_for_candidate_reduces_an_out_of_range_duty_subnet can then go.

Agreed, the silent reduction was the wrong call. 61e21ab adds validate_aggregate_subnet_ids, called from run_node right after the committee count resolves (it cannot live in clap, since the count comes from the flag or validator-config.yaml). Every id is checked, not just the first: only the first becomes the duty subnet, but all of them become subscriptions. That closes the pre-existing dead subscription too.

One deviation from your suggestion: I kept the modulo in window_for_candidate and its test. AggregationWindowConfig and snapshot_aggregation_inputs are pub, so the blockchain crate cannot lean on the binary's validation, and SubnetWindow::new reduces start regardless. Dropping the reduction only in window_for_candidate would leave the ownership test running on the raw value while the window ran on the reduced one, which is precisely the desync that test pins. So it is all-or-nothing, and three lines guarding a public API seemed the better side to land on. The comment there now says the binary is what actually enforces it.

pablodeymo · 2026-09-16T15:51:58Z

+    ///
+    /// Worth enabling when leanVM prover CPU is the bottleneck on co-located
+    /// aggregators. The cost is that a level in a given slot has few
+    /// producers, so a node that is down or late forfeits that slot's merge at


The cost is larger than "a node that is down or late". Ownership is duty_subnet % width == slot % width, so a level can have zero owners while every configured node is healthy: with duty subnets {0, 2} at committee count 4, width 4 has no owner in odd slots, and since the full-width fallback is off under this flag, that merge level is dropped for the slot.

The rotation only guarantees an owner at every level when every subnet has an aggregator with that duty subnet. Worth stating that as a deployment precondition here and in the architecture doc, so an operator does not read the flag as safe on a sparse placement.

Confirmed your example: duty subnets {0, 2}, committee count 4, slot 1 gives 1 % 4 == 1, which neither 0 nor 2 matches, so width 4 has no owner in odd slots and the fallback is off. The old wording framed that as a node being down or late, which is wrong.

8ac6dfc rewrites the flag's help and adds a paragraph to docs/architecture.md, stating the precondition explicitly: give every subnet below --attestation-committee-count an aggregator holding it as its duty subnet, otherwise leave the flag off. The loss is called structural rather than a consequence of an unhealthy node.

pablodeymo · 2026-09-16T15:51:58Z

+/// aggregator's window to the full committee set.
+///
+/// Only one session runs per slot (see [`snapshot_aggregation_inputs`]), and
+/// this candidate's own pool is still empty at that point: a produced


This is an assumption about timing, not an invariant the code enforces. If a peer's aggregate for the current slot lands before this node's snapshot (clock skew, or a session that started early via EarlyAggregationCheck), the current-slot candidate gains an anchor and a width of 2. Under --skip-redundant-aggregation that means half the aggregators sit the current slot out, and their raw signatures miss the next block.

In the intended topology, distinct duty subnets with distinct subscriptions, a round-one peer proof never touches this node's subnet, so it does not bite there. It would with overlapping subscriptions. A sentence here (or in the PR description's cadence section) noting the assumption and when it fails would save the next reader the derivation.

Fair, "always derives the narrowest width" claimed more than the code guarantees. 8ac6dfc softens it to "in practice" and adds the derivation you asked for: it is a timing expectation, a peer aggregate for the current slot landing before this node's snapshot (clock skew, or a session started early via EarlyAggregationCheck) gives the candidate an anchor and width 2, under --skip-redundant-aggregation the duty subnets that do not own width 2 sit the slot out and their raw signatures miss the next block, the intended topology keeps it true because a round-one peer proof never touches this node's subnet, and overlapping subscriptions are where it fails.

… subnet for The flag feeds two consumers that read an out-of-range value differently. The P2P swarm subscribes to the raw id, while the aggregation window reduces it modulo the committee count, so `--attestation-committee-count 4 --aggregate-subnet-ids 5` subscribes to a topic no validator publishes on and aggregates as duty subnet 1, which the node does not listen to. The startup log prints 5 for both, so the split is invisible to the operator. Refusing to start is the only reading of that configuration that cannot silently mean something else, and it closes the dead subscription that predates the duty subnet. The check lives in `run_node` rather than clap because the committee count is only known once the CLI flag and the validator config have both been consulted. The reduction inside `window_for_candidate` stays: `AggregationWindowConfig` is public, and `SubnetWindow::new` reduces `start` regardless, so dropping it would leave the ownership test running on an unreduced value.

…ery miss `lean_aggregation_window_fallback_total` is documented as the signal that the aggregator placement is too sparse for the committee count, so it has to count merges the window dropped. It was incrementing on every `resolve_job` miss instead, and `resolve_job` declines for reasons the window has no part in: a gossip group holding a single raw signature and no children, which is every minority or late vote. The counter therefore climbed on any healthy deployment, contradicting its own reading, and the second selection pass ran on candidates no width could have saved. Return before the retry in the two cases where it is provably a no-op, a window already as wide as the committee and an empty proof pool, and increment only when the full-width retry actually yields a job.

Both came out of review, and both are assumptions a reader has to re-derive from the arithmetic today. `--skip-redundant-aggregation` costs more than "a node that is down or late". Ownership is `duty_subnet % width == slot % width`, so a width can have no owner at all while every configured aggregator is healthy: with duty subnets {0, 2} at committee count 4, nothing owns width 4 in an odd slot, and the flag disables the full-width fallback, so that merge level is dropped for the slot. The rotation only covers every level when every subnet has an aggregator holding it as its duty subnet, which is a deployment precondition rather than a property of the flag. "A current-slot candidate always derives the narrowest width" is a timing expectation, not an invariant. A peer's aggregate for the current slot landing before this node's snapshot, under clock skew or an early session, gives that candidate an anchor and a width of 2. The intended topology keeps it true (a round-one peer proof never touches this node's subnet), overlapping subscriptions are where it fails.

MegaRedHand added 20 commits September 9, 2026 16:49

feat(aggregation): add subnet reach and window primitives

bd76661

Groundwork for giving each aggregator a distinct slice of the shared proof pool. Pure functions with no call sites yet.

feat(aggregation): add the redundancy-skipping width rotation

234a549

Narrows an aggregator to the widest level it owns in the slot, rotating the owner so no duty subnet is permanently the one sitting out. Not wired to a caller yet.

feat(aggregation): derive a per-candidate subnet window

0018e49

Width comes from the best reach in the candidate's own proof pool, so every aggregator derives the same width for a data root and the merge tree stays in step. The duty subnet is still a placeholder pending the CLI wiring.

feat(blockchain): carry the aggregation duty subnet on the actor

68b8f4b

Moves the duty subnet and the redundancy-skipping flag onto BlockChainConfig so the binary can supply them, with the previous inline fallback relocated to main.rs unchanged.

feat(cli): add --skip-redundant-aggregation and duty-subnet resolution

05ab79b

The duty subnet is the first --aggregate-subnet-ids value, so operators place co-located aggregators on different subnets deliberately rather than having every node derive the same one from its subscriptions.

docs(cli): document the duty subnet and log how it resolved

0641ddb

The first --aggregate-subnet-ids entry became load-bearing without its help text saying so, and a node that silently fell back to the lowest subscribed subnet was indistinguishable from one an operator placed deliberately.

docs: document the aggregation window metrics

57b3233

test(aggregation): pin the four-aggregator reduction tree

6041332

Round 1 produces four proofs with distinct coverage, round 2 merges them to the full validator set. Also pins that a single committee ignores the duty subnet, so the window is a no-op there.

docs: shorten the aggregation metric rows and correct the pinned reading

e5825f0

The width pins once the best pool proof reaches half the committees, not all of them, so a pinned sample means the window no longer restricts selection rather than that the pool is saturated.

test(aggregation): make the duty-subnet reduction test discriminate

5653ae7

The test guarding the pre-rotation reduction passed without it, since SubnetWindow::new's own fold masked the difference at a committee count the width divides. Also covers the redundancy-skipping path end to end.

docs: describe subnet-windowed aggregation in the architecture guide

1782908

MegaRedHand added 2 commits September 10, 2026 13:53

Merge branch 'main' into feat/subnet-windowed-aggregation

aab43b1

MegaRedHand marked this pull request as draft September 10, 2026 20:35

dimka90 mentioned this pull request Sep 15, 2026

perf(proposal): final block-proof generation can outlast the proposal’s useful lifetime geanlabs/gean#438

Open

6 tasks

MegaRedHand marked this pull request as ready for review September 15, 2026 20:10

Merge branch 'main' into feat/subnet-windowed-aggregation

255d75f

pablodeymo reviewed Sep 16, 2026

View reviewed changes

MegaRedHand added 3 commits September 16, 2026 13:21

pablodeymo approved these changes Sep 16, 2026

View reviewed changes

MegaRedHand added this pull request to the merge queue Sep 16, 2026

Merged via the queue into main with commit 1be9995 Sep 16, 2026
6 checks passed

MegaRedHand deleted the feat/subnet-windowed-aggregation branch September 16, 2026 18:10

Uh oh!

Conversation

MegaRedHand commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Design

Safety

Known trade-off

Cadence, in practice

Metrics

Test Plan

Notes for review

Uh oh!

github-actions Bot commented Sep 10, 2026

🤖 Kimi Code Review

Overall Assessment

Issues Found

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

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

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

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

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

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

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

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

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

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

Summary

Recommended Changes

Item 6 — Verify tie-breaking behavior

Item 7 — Clarify fallback metric (optional)

Positive Notes

Uh oh!

github-actions Bot commented Sep 10, 2026

🤖 Codex Code Review

Uh oh!

github-actions Bot commented Sep 10, 2026

🤖 Claude Code Review

Review: Subnet-windowed aggregation (PR #613)

Uh oh!

github-actions Bot commented Sep 15, 2026

🤖 Kimi Code Review

High-Level Assessment

Issues Found

1. Potential Division by Zero in owns_width — Not Fully Protected (Critical)

2. Inconsistent Redundancy of committee_count == 0 Checks (Moderate)

3. resolve_job_with_window_fallback Double-Clones hashed (Minor/Performance)

4. resolve_job Signature Change Not Reflected in All Call Sites Documentation (Minor)

5. Metrics Registration Panic on Duplicate (Minor — Pre-existing Pattern)

6. Test window_fallback_recovers_a_merge_a_strided_placement_would_drop Has Brittle Assertion (Moderate)

7. insert_test_block Helper Missing in Diff (Potential Compilation Issue)

8. HashSet Iteration Order in resolve_aggregation_duty_subnet (Already Handled, Good)

9. CLI Warning for skip_redundant_aggregation with Derived Duty Subnet (Good Practice)

10. AggregationWindowConfig Derives Copy but Contains u64 Fields (Fine, but Note)

11. Potential Integer Overflow in anchor_reach.saturating_mul(2) (Already Safe)

12. Docstring Inconsistency: "width below the committee count has several owners" (Minor)

13. Missing #[cfg(test)] on Test Helpers (Minor)

14. subnet_reach Uses HashSet with Early Exit — Good Optimization

Summary Table

Positive Acknowledgments

Uh oh!

github-actions Bot commented Sep 15, 2026

🤖 Codex Code Review

Uh oh!

github-actions Bot commented Sep 15, 2026

🤖 Claude Code Review

Review: Subnet-windowed aggregation (PR 613)

Overall

Minor observations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MegaRedHand commented Sep 10, 2026 •

edited

Loading

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

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

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

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

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

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

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

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

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

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

1. Potential Division by Zero in `owns_width` — Not Fully Protected (Critical)

2. Inconsistent Redundancy of `committee_count == 0` Checks (Moderate)

3. `resolve_job_with_window_fallback` Double-Clones `hashed` (Minor/Performance)

4. `resolve_job` Signature Change Not Reflected in All Call Sites Documentation (Minor)

6. Test `window_fallback_recovers_a_merge_a_strided_placement_would_drop` Has Brittle Assertion (Moderate)

7. `insert_test_block` Helper Missing in Diff (Potential Compilation Issue)

8. `HashSet` Iteration Order in `resolve_aggregation_duty_subnet` (Already Handled, Good)

9. CLI Warning for `skip_redundant_aggregation` with Derived Duty Subnet (Good Practice)

10. `AggregationWindowConfig` Derives `Copy` but Contains `u64` Fields (Fine, but Note)

11. Potential Integer Overflow in `anchor_reach.saturating_mul(2)` (Already Safe)

13. Missing `#[cfg(test)]` on Test Helpers (Minor)

14. `subnet_reach` Uses `HashSet` with Early Exit — Good Optimization