Skip to content

feat(blockchain): cap aggregation job children to 2 (binary merge) #482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MegaRedHand wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat(blockchain): cap aggregation job children to 2 (binary merge) #482

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
02601cb
feat(blockchain): cap aggregation job children to 2 (binary merge)
MegaRedHand Jun 30, 2026
dcba9da
test: remove unit tests
MegaRedHand Jun 30, 2026
a93dc42
docs: improve documentation
MegaRedHand Jun 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions crates/blockchain/src/aggregation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,12 +321,18 @@ pub fn finalize_aggregation_session(store: &Store) {
metrics::update_gossip_signatures(store.gossip_signatures_count());
}

/// Maximum number of existing proofs reused as children in a single
/// aggregation job. Recursive aggregation is costly, so we limit the
/// number of children to avoid unbounded aggregation times.
const MAX_AGGREGATION_CHILDREN: usize = 2;

/// Greedy set-cover selection of proofs to maximize validator coverage.
///
/// Processes proof sets in priority order (new before known). Within each set,
/// repeatedly picks the proof covering the most uncovered validators until
/// no proof adds new coverage. This keeps the number of children minimal
/// while maximizing the validators we can skip re-aggregating from scratch.
/// repeatedly picks the proof covering the most uncovered validators until no
/// proof adds new coverage.
///
/// Caps the number of proofs selected at [`MAX_AGGREGATION_CHILDREN`].
fn select_proofs_greedily(
new_proofs: &[TypeOneMultiSignature],
known_proofs: &[TypeOneMultiSignature],
Expand All @@ -337,7 +343,7 @@ fn select_proofs_greedily(
for proof_set in [new_proofs, known_proofs] {

@greptile-apps greptile-apps Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Priority Bucket Hides Better Proofs

When new_proofs contains two small disjoint proofs, this loop reaches the child cap before known_proofs is examined. A known proof for the same root that covers many more validators is skipped, so build_job receives a smaller covered set and can produce a heavier, lower-coverage aggregate even though the same two-child limit could have reused the better proof.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/blockchain/src/aggregation.rs
Line: 347

Comment:
**Priority Bucket Hides Better Proofs**

When `new_proofs` contains two small disjoint proofs, this loop reaches the child cap before `known_proofs` is examined. A known proof for the same root that covers many more validators is skipped, so `build_job` receives a smaller `covered` set and can produce a heavier, lower-coverage aggregate even though the same two-child limit could have reused the better proof.

How can I resolve this? If you propose a fix, please make it concise.

let mut remaining: Vec<&TypeOneMultiSignature> = proof_set.iter().collect();

while !remaining.is_empty() {
while selected.len() < MAX_AGGREGATION_CHILDREN && !remaining.is_empty() {
let best_idx = remaining
.iter()
.enumerate()
Expand All @@ -361,6 +367,10 @@ fn select_proofs_greedily(
selected.push(remaining.swap_remove(best_idx).clone());
covered.extend(new_coverage);
}

if selected.len() >= MAX_AGGREGATION_CHILDREN {
break;
}
}

(selected, covered)
Expand Down