node, types: skip recursive aggregation for 1-sig + 0-child trivial inputs (#907) - #908
Merged
Merged
Conversation
…nputs (#907) When the aggregator's input for an AttestationData is exactly one gossip signature and zero child payloads, recursive aggregation is a costly no-op: the lone sig is already on the attestation_signatures gossip topic so peers can fold it into their own aggregations directly, and the STARK prover cost is constant in input size — issuing a recursive proof for a single signature spends the full prover budget on a 1-validator "aggregate" that cannot move any quorum. The existing fast path in `computeAggregatedSignatures` only covered the opposite shape (`0 gossip + exactly 1 child`, cloned as-is). Add a symmetric fast path for `1 gossip + 0 children` that skips the att_data and leaves the sig in `signatures_map` so the next pass — once a peer payload arrives or another local sig is added — can fold it in as a non-trivial input. Predicate is extracted as `isTrivialAggregationInput(num_children, num_gossip_sigs)` so it can be unit-tested without XMSS key/sig setup. Two existing tests in `forkchoice.zig` (`aggregate prunes attestation signatures` and `aggregate (#890): does not acquire forkchoice main mutex`) seeded a single sig and asserted exactly one aggregation was produced. Both now seed two distinct validators on the same AttestationData — the minimum non-trivial shape — so the FFI-bearing path under test is still exercised. Operator-visible effect on devnet aggregators that currently see only their own validator on a duty subnet (issue #907 finding 3): - `zeam_aggregate_worker_duration_seconds` p50 drops from ~10.8s to the snapshot/commit overhead (~10ms) - `zeam_aggregate_skip_total{reason="in_flight"}` rate falls toward 0 because the worker no longer holds the slot interval - `zeam_aggregator_publish_aggregations_total{subnet}` no longer ticks for slots where the only input is the local sig (peers already see it via the regular gossip channel)
g11tech
reviewed
May 21, 2026
anshalshukla
previously approved these changes
May 21, 2026
…tion-inputs Address review on #908: rather than hard-coding the trivial-input check at exactly `1 gossip + 0 children`, surface the threshold as an operator-configurable knob. Add `--min-aggregation-inputs` (default `2`) on `zeam node`. Threaded through `NodeOptions` → `BeamNode.NodeOpts` → `ChainOpts` → `ForkChoiceParams` → `ForkChoice.min_aggregation_inputs`, and read on every `aggregate*()` call when invoking `computeAggregatedSignatures`. Predicate generalisation in `pkgs/types/src/block.zig`: fn isTrivialAggregationInput(num_children, num_gossip_sigs, min_inputs) bool { if (num_children > 0) return false; return num_gossip_sigs < min_inputs; } The threshold is intentionally only enforced in the no-children branch: once any child is selected the prover provides genuine consensus value regardless of gossip-sig count, and the lone-child clone fast path (`0 sigs + 1 child`) does not invoke the prover at all and so is unaffected. Threshold semantics: - `1` reverts to pre-#908 behaviour: always aggregate ≥1 sig. - `2` (default) is post-#908: skip the no-children + single-sig case where the prover would produce a 1-validator aggregate of zero consensus value. - `3+` trades slot latency for fewer sub-threshold aggregates on chatty subnets (operator can choose to wait for more local sigs before spending the full prover budget). Logged on startup so operators can see whether the threshold came from the default or a `--min-aggregation-inputs` override, mirroring the existing `--rayon-threads` startup line. Test changes: - `pkgs/types/src/block.zig`: existing `isTrivialAggregationInput` unit tests are extended to cover all three threshold regimes (1, 2, 3+). All four `computeAggregatedSignatures` test call sites now pass `default_min_aggregation_inputs`. - `pkgs/node/src/forkchoice.zig`: `ForkChoice.min_aggregation_inputs` is defaulted at the field level to keep test struct literals unchanged. The two existing aggregator tests already seed two validators (from the original PR), so they continue to exercise the FFI-bearing path. Pre-commit: zig fmt --check . cargo fmt --check && cargo clippy -- -D warnings zig build test --summary all zig build simtest --summary all
Move the post-#908 trivial-input skip (no children + sub-threshold gossip sigs) from inside the spec-pure FFI function into a small aggregator-only pre-filter (`pruneTrivialFromAggregateSnapshot`) that runs against the owned snapshot before `computeAggregatedSignatures` is invoked. The FFI core is now spec-pure: it aggregates whatever it is given. The threshold is an aggregator-role policy, not a property of the aggregation primitive. A future block-proposer caller (which must aggregate every `att_data` it chose to include in the block, even one with a single gossip sig) gets correct behavior without having to remember a knob. Behaviorally identical to abc090d for the aggregator: same predicate (`isAggregatorTrivialInput`), same inputs, same dropped att_data — just relocated from inside the loop to the snapshot. Addresses review feedback on PR #908.
anshalshukla
approved these changes
May 21, 2026
This was referenced May 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
If an aggregator only has its own validator's sig and no peer payloads to fold in, today it still spends ~10s running the recursive STARK prover to produce a 1-validator "aggregate". That aggregate cannot move quorum and the sig is already on the gossip topic, so the work is wasted.
There was already a fast path in
computeAggregatedSignaturesfor0 gossip + 1 child(clone the child, no FFI). This adds the symmetric case:1 gossip + 0 childrenskips the att_data and leaves the sig insignatures_mapso a future pass can fold it in once there's something to fold it with.Code changes
pkgs/types/src/block.zig: new fast path plus a small helperisTrivialAggregationInput(num_children, num_gossip_sigs)so the predicate is unit-testable without real XMSS keys.pkgs/node/src/forkchoice.zig: two existing tests seeded a single sig and expected one aggregation. They now seed two distinct validators so they still exercise the FFI path they were written to test.Why it's safe
attestation_signaturestopic at sign time, so peers don't lose visibility.signatures_mapare only pruned when an aggregation consumes them; the skipped sig stays in the map for the next pass.Expected metric impact on devnet
For aggregators currently in the steady state described in #907 (one local sig, no peer payloads):
zeam_aggregate_worker_duration_secondsp50: ~10.8s → ~10mszeam_aggregate_skip_total{reason="in_flight"}: rate falls to ~0zeam_aggregator_publish_aggregations_total{subnet}: stops ticking while there's only a local sig; resumes once anything else arrivesTests
zig fmt --check .cargo fmt --check,cargo clippy -- -D warningszig build test --summary all,zig build simtest --summary allOut of scope
The Rust-side
rec_xmss_aggregatehas no degenerate-case shortcut either (#907 finding 4b). That's a separate change in leanMultisig and not part of this PR.Related