feat(blockchain): cap attestation packing at 2/3 supermajority#472
feat(blockchain): cap attestation packing at 2/3 supermajority#472MegaRedHand wants to merge 3 commits into
Conversation
Block building stitched partial proofs together until ~100% voter coverage per AttestationData, so any entry with multiple partial proofs paid for a recursive proof merge (aggregate_proofs) in compact_attestations: the dominant cost of building blocks that carry attestations. Consensus only needs a 2/3 supermajority; once a target crosses it the target is justified and further votes are rejected. extend_proofs_greedily now stops packing proofs as soon as prior + covered voters cross 2/3, so an entry whose largest proof alone exceeds 2/3 keeps a single proof and compaction skips aggregate_proofs entirely. Sub-supermajority (Build-tier) entries never trip the cutoff and keep full coverage. This is safe because fork-choice weight is gossip-sourced (not block-sourced) and there are no inclusion rewards, so dropping voters above 2/3 changes neither head selection nor finalization. extend_proofs_greedily returns the voters it actually packed so the selection projection stays consistent with the block; skipped proofs are traced (supermajority_reached / no_new_coverage).
🤖 Kimi Code ReviewThe PR correctly moves the "new voters" calculation from Correctness & Logic
Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
No other correctness or security regressions stood out in the diff. The projection change in I could not run the targeted tests here because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR optimises attestation packing in block building by stopping proof accumulation for an
Confidence Score: 4/5The change is safe to merge; it reduces unnecessary recursive proof merges without affecting consensus correctness, and the new unit tests cover the key cutoff scenarios thoroughly. The algorithm is internally consistent: score_entry's tier classification (which requires full coverage) and extend_proofs_greedily's early stopping (which stops at 2/3) are correctly decoupled, and the projection update uses the actually-packed voter set. The two observations flagged are trace accuracy and a bounded greedy suboptimality — both pre-existing or purely diagnostic in nature. crates/blockchain/src/block_builder.rs — the trace
|
| Filename | Overview |
|---|---|
| crates/blockchain/src/block_builder.rs | Implements 2/3 supermajority cutoff in extend_proofs_greedily; refactors score_entry/pick_best_candidate to drop the returned voter set; adds is_supermajority helper; updates projection to use actual packed voters; adds 4 new unit tests covering the key cutoff scenarios. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[select_attestations loop] --> B[pick_best_candidate]
B --> C{Any candidate\nwith new voters?}
C -- No --> Z[Done - converged]
C -- Yes --> D[score_entry\nfor each candidate\nfull-proof scan]
D --> E[Returns EntryScore\ntier + new_voters count]
E --> F[Best score picked\nby ordering_key]
F --> G[get prior_voters\nfrom projected state]
G --> H[extend_proofs_greedily\nwith prior_voters + validator_count]
H --> I{After each proof:\nprior_count + covered_new\n>= 2/3?}
I -- No --> J[Pick next best proof\nby uncovered count]
J --> I
I -- Yes --> K[cutoff reached,\nstop packing]
I -- Proofs exhausted --> L[All coverage packed\nBuild-tier path]
K --> M[Return covered_new]
L --> M
M --> N[Update projected.current_votes\nwith covered_new]
N --> O{Tier Justify\nor Finalize?}
O -- Yes --> P[Project justification,\nremove target from votes]
O -- No --> A
P --> A
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[select_attestations loop] --> B[pick_best_candidate]
B --> C{Any candidate\nwith new voters?}
C -- No --> Z[Done - converged]
C -- Yes --> D[score_entry\nfor each candidate\nfull-proof scan]
D --> E[Returns EntryScore\ntier + new_voters count]
E --> F[Best score picked\nby ordering_key]
F --> G[get prior_voters\nfrom projected state]
G --> H[extend_proofs_greedily\nwith prior_voters + validator_count]
H --> I{After each proof:\nprior_count + covered_new\n>= 2/3?}
I -- No --> J[Pick next best proof\nby uncovered count]
J --> I
I -- Yes --> K[cutoff reached,\nstop packing]
I -- Proofs exhausted --> L[All coverage packed\nBuild-tier path]
K --> M[Return covered_new]
L --> M
M --> N[Update projected.current_votes\nwith covered_new]
N --> O{Tier Justify\nor Finalize?}
O -- Yes --> P[Project justification,\nremove target from votes]
O -- No --> A
P --> A
Comments Outside Diff (1)
-
crates/blockchain/src/block_builder.rs, line 199-207 (link)new_voterstrace field overstates actual packed coverage for Justify/Finalize entriesscore.new_voterscomes fromscore_entry, which unions ALL proof participants minusprior_voters. For a Justify or Finalize entry,extend_proofs_greedilymay stop short at the 2/3 cutoff, so the count of validators actually written to the block (i.e.covered_new.len()) can be substantially less. Anyone reading this trace to diagnose convergence or voter-accumulation behaviour will see an inflatednew_votersfigure whileselected_proofswill correctly show fewer proofs were packed. Consider loggingcovered.len()(the return value ofextend_proofs_greedily) instead of, or alongside,score.new_votersso the two fields remain consistent.Prompt To Fix With AI
This is a comment left during a code review. Path: crates/blockchain/src/block_builder.rs Line: 199-207 Comment: **`new_voters` trace field overstates actual packed coverage for Justify/Finalize entries** `score.new_voters` comes from `score_entry`, which unions ALL proof participants minus `prior_voters`. For a Justify or Finalize entry, `extend_proofs_greedily` may stop short at the 2/3 cutoff, so the count of validators actually written to the block (i.e. `covered_new.len()`) can be substantially less. Anyone reading this trace to diagnose convergence or voter-accumulation behaviour will see an inflated `new_voters` figure while `selected_proofs` will correctly show fewer proofs were packed. Consider logging `covered.len()` (the return value of `extend_proofs_greedily`) instead of, or alongside, `score.new_voters` so the two fields remain consistent. How can I resolve this? If you propose a fix, please make it concise.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/blockchain/src/block_builder.rs:199-207
**`new_voters` trace field overstates actual packed coverage for Justify/Finalize entries**
`score.new_voters` comes from `score_entry`, which unions ALL proof participants minus `prior_voters`. For a Justify or Finalize entry, `extend_proofs_greedily` may stop short at the 2/3 cutoff, so the count of validators actually written to the block (i.e. `covered_new.len()`) can be substantially less. Anyone reading this trace to diagnose convergence or voter-accumulation behaviour will see an inflated `new_voters` figure while `selected_proofs` will correctly show fewer proofs were packed. Consider logging `covered.len()` (the return value of `extend_proofs_greedily`) instead of, or alongside, `score.new_voters` so the two fields remain consistent.
### Issue 2 of 2
crates/blockchain/src/block_builder.rs:668-715
**Greedy ranks proofs by "uncovered by selected proofs," not by "net new to the target voter union"**
`best_count` is computed by filtering against `covered`, which includes validators that are already in `prior_voters` if they appear in a selected proof. This means a proof that is, say, 80% prior voters and 20% fresh validators can appear "larger" to the greedy than a proof that is 100% fresh validators. Concretely: if `prior_voters = {0,1,2,3}`, proof A = `{0,1,2,3,4}` (5 uncovered, 1 truly new) beats proof B = `{5,6,7,8}` (4 uncovered, 4 truly new) in the first pick. As a result the greedy may pack one extra proof before reaching the cutoff — exactly the scenario the PR is trying to minimise. The issue is bounded (at most one unnecessary extra merge per entry) and the previous code had the same property, but it is worth documenting as a known residual cost.
Reviews (1): Last reviewed commit: "feat(blockchain): cap attestation packin..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -664,10 +700,48 @@ fn extend_proofs_greedily( | |||
| metrics::inc_pq_sig_aggregated_signatures(); | |||
| metrics::inc_pq_sig_attestations_in_aggregated_signatures(new_covered.len() as u64); | |||
|
|
|||
| for &vid in &new_covered { | |||
| if prior_voters.is_none_or(|prior| !prior.contains(&vid)) { | |||
| covered_new.insert(vid); | |||
| } | |||
| } | |||
| covered.extend(new_covered); | |||
| selected.push((att, proof.clone())); | |||
| remaining_indices.remove(&best_idx); | |||
|
|
|||
| if is_supermajority(prior_count + covered_new.len(), validator_count) { | |||
| cutoff_reached = true; | |||
| break; | |||
| } | |||
There was a problem hiding this comment.
best_count is computed by filtering against covered, which includes validators that are already in prior_voters if they appear in a selected proof. This means a proof that is, say, 80% prior voters and 20% fresh validators can appear "larger" to the greedy than a proof that is 100% fresh validators. Concretely: if prior_voters = {0,1,2,3}, proof A = {0,1,2,3,4} (5 uncovered, 1 truly new) beats proof B = {5,6,7,8} (4 uncovered, 4 truly new) in the first pick. As a result the greedy may pack one extra proof before reaching the cutoff — exactly the scenario the PR is trying to minimise. The issue is bounded (at most one unnecessary extra merge per entry) and the previous code had the same property, but it is worth documenting as a known residual cost.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/block_builder.rs
Line: 668-715
Comment:
**Greedy ranks proofs by "uncovered by selected proofs," not by "net new to the target voter union"**
`best_count` is computed by filtering against `covered`, which includes validators that are already in `prior_voters` if they appear in a selected proof. This means a proof that is, say, 80% prior voters and 20% fresh validators can appear "larger" to the greedy than a proof that is 100% fresh validators. Concretely: if `prior_voters = {0,1,2,3}`, proof A = `{0,1,2,3,4}` (5 uncovered, 1 truly new) beats proof B = `{5,6,7,8}` (4 uncovered, 4 truly new) in the first pick. As a result the greedy may pack one extra proof before reaching the cutoff — exactly the scenario the PR is trying to minimise. The issue is bounded (at most one unnecessary extra merge per entry) and the previous code had the same property, but it is worth documenting as a known residual cost.
How can I resolve this? If you propose a fix, please make it concise.
🤖 Claude Code ReviewAll verifications complete. Here is the review. Code Review:
|
Address automated and agent review feedback on the supermajority cutoff: - extend_proofs_greedily ranked candidate proofs by participants not in the previously-packed set, ignoring prior_voters. With the new cutoff that let the greedy pack a redundant prior-heavy proof before crossing 2/3, weakening the optimization. Rank by additions to the full target union (prior_voters + packed) instead, so an all-new proof beats a prior-heavy one and a proof whose voters are already counted is never packed for a wasted merge child. - Gate the skipped-proof trace block on trace level being enabled so the per-proof recount and hash_tree_root are skipped on the hot path. - "selected" trace now reports packed_voters (what actually landed) alongside candidate_new_voters (the full-coverage score), which alone was misleading. - Note the registry-bounded validator_count on is_supermajority. Tests: a net-new ranking regression test, and a build_block end-to-end test proving a cutoff-capped entry still justifies its target through the real STF while staying on compaction's single-proof fast path.
1 participant
What
Block building stops packing attestation proofs for an
AttestationDataentry as soon as its voters cross the 2/3 supermajority, instead of stitching partial proofs together until ~100% coverage.Why
select_attestations→extend_proofs_greedilygreedily covered ~all available voters per entry. Any entry left with multiple partial proofs then paid for a recursive proof merge (aggregate_proofs, the leanVM SNARK) incompact_attestations. Per profiling, that compaction is the dominant cost of building blocks that carry attestations.Consensus only needs a 2/3 supermajority: once a target reaches it, the target is justified and
is_valid_voterejects further votes for it. So every proof packed beyond 2/3 was added to an expensive merge for zero consensus gain.How
One uniform cutoff in
extend_proofs_greedily, no tier branching:prior_voters + coveredcross 2/3 (is_supermajorityhelper, shared withscore_entry).prior_voters= validators that already voted for this target in the projected state or earlier rounds.compact_attestationstakes its single-proof fast path and skipsaggregate_proofsentirely.extend_proofs_greedilynow returns the voters it actually packed;select_attestationsupdates its vote projection from that set (notscore_entry's full union) so the projection stays consistent with the real block.tracelevel (reason = supermajority_reached | no_new_coverage).Safety
extract_latest_known_attestations), not block-included attestations. Head weight is unchanged.Tradeoff: a non-justifying target accumulates one proposer's selection per block rather than the full gossip union, converging over slots. No safety impact.
Tests
extend_proofs_greedily_*tests for the new signature (highvalidator_countso they keep testing overlap/exhaustion).build_block_*integration tests unchanged.make fmt,make lint, andcargo test -p ethlambda-blockchain --lib(42 pass) all green.