From 78f7629a776e6b1839090a9f2bf10c3640367629 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Mon, 6 Jul 2026 17:55:52 -0300 Subject: [PATCH 1/4] fix(blockchain): select proof by marginal coverage when proposer aggregation is off `keep_best_proof_per_data` collapses same-`AttestationData` proofs to one (a block carries at most one entry per data) when proposer aggregation is disabled. It picked the proof with the most participants by *absolute* count. That starves a small subnet: once the largest subnet's voters are already counted in-state for a target, that subnet keeps winning the collapse yet adds zero new coverage, while the smaller subnet holding the only still-missing votes is dropped every block. The target's in-state coverage caps below the largest single subnet's union and never reaches 2/3, so justification stalls. The aggregation-on path (`compact_attestations`) sidesteps this by merging all same-data proofs into one union-coverage proof. Without leanVM we cannot merge, but we can pick the single proof that adds the most NEW voters over the target's in-state voter set instead of the largest one. Ties fall back to absolute participant count, then first occurrence, so a fresh target keeps the densest proof exactly as before. This also makes the proof-level collapse consistent with `select_attestations`, which already scores the entry on the same marginal coverage. --- crates/blockchain/src/block_builder.rs | 177 ++++++++++++++++++++++--- 1 file changed, 156 insertions(+), 21 deletions(-) diff --git a/crates/blockchain/src/block_builder.rs b/crates/blockchain/src/block_builder.rs index 919a7719..a6c32520 100644 --- a/crates/blockchain/src/block_builder.rs +++ b/crates/blockchain/src/block_builder.rs @@ -117,7 +117,8 @@ pub(crate) fn build_block( let compacted = if config.enable_proposer_aggregation { compact_attestations(selected, head_state, slot)? } else { - keep_best_proof_per_data(selected, slot) + let running_votes = build_running_votes(head_state); + keep_best_proof_per_data(selected, &running_votes, slot) }; metrics::observe_block_proposal_phase("compact", compact_start.elapsed()); @@ -661,30 +662,40 @@ fn compact_attestations( /// /// The block format permits at most one entry per `AttestationData`: `on_block` /// rejects duplicates (`StoreError::DuplicateAttestationData`). When proposer -/// aggregation is disabled we therefore cannot keep every selected proof, nor -/// can we merge them. For each group sharing an `AttestationData` we keep the -/// single proof covering the most validators (ties broken by first occurrence) -/// and drop the rest. No leanVM aggregation runs; coverage is whatever the best -/// individual proof already had, which is the cost of skipping aggregation. +/// aggregation is disabled we can neither keep every selected proof nor merge +/// them, so for each group sharing an `AttestationData` exactly one proof +/// survives and the rest are dropped. No leanVM aggregation runs. +/// +/// The surviving proof is the one adding the most NEW voters over the target's +/// in-state voter set (`running_votes`, keyed by `target.root`), with ties +/// broken by larger absolute participant count, then first occurrence. For a +/// fresh target (no in-state voters) marginal coverage equals absolute count, +/// so the densest proof wins, matching the previous behavior. +/// +/// Scoring by marginal (rather than absolute) coverage keeps this collapse +/// consistent with the union coverage `compact_attestations` reaches when +/// aggregation is enabled, and with `select_attestations`, which already scored +/// the entry on the same marginal coverage. Selecting by absolute participant +/// count instead lets a larger subnet already fully counted in-state keep +/// winning while adding nothing, starving a smaller subnet whose votes are the +/// only ones still missing; the target's coverage then caps below 2/3 and never +/// justifies. fn keep_best_proof_per_data( entries: Vec<(AggregatedAttestation, SingleMessageAggregate)>, + running_votes: &HashMap>, block_slot: u64, ) -> Vec<(AggregatedAttestation, SingleMessageAggregate)> { - // Preserve first-occurrence order of distinct AttestationData; for each, - // track the index of the best (most participants) entry seen so far. + // Group entry indices by AttestationData, preserving first-occurrence order. let mut order: Vec = Vec::new(); - let mut best_index: HashMap = HashMap::new(); + let mut groups: HashMap> = HashMap::new(); for (i, (att, _)) in entries.iter().enumerate() { - match best_index.entry(att.data.clone()) { + match groups.entry(att.data.clone()) { std::collections::hash_map::Entry::Vacant(e) => { order.push(e.key().clone()); - e.insert(i); + e.insert(vec![i]); } std::collections::hash_map::Entry::Occupied(mut e) => { - let current_best = entries[*e.get()].0.aggregation_bits.count_ones(); - if att.aggregation_bits.count_ones() > current_best { - e.insert(i); - } + e.get_mut().push(i); } } } @@ -702,15 +713,33 @@ fn keep_best_proof_per_data( "Skipping attestation compaction" ); - let mut items: Vec> = - entries.into_iter().map(Some).collect(); - order + // Pick the surviving index per group: most new coverage over the target's + // in-state voters, then densest proof, then earliest occurrence. Computed + // over `entries` before it is moved into `items` for extraction. + let best_per_data: Vec = order .iter() .map(|data| { - items[best_index[data]] - .take() - .expect("best index taken once") + let indices = &groups[data]; + let prior = running_votes.get(&data.target.root); + *indices + .iter() + .max_by_key(|&&idx| { + let (att, proof) = &entries[idx]; + let marginal = proof + .participant_indices() + .filter(|vid| prior.is_none_or(|voted| !voted.contains(vid))) + .count(); + (marginal, att.aggregation_bits.count_ones(), Reverse(idx)) + }) + .expect("group is non-empty") }) + .collect(); + + let mut items: Vec> = + entries.into_iter().map(Some).collect(); + best_per_data + .into_iter() + .map(|idx| items[idx].take().expect("best index taken once")) .collect() } @@ -1320,6 +1349,112 @@ mod tests { ); } + /// When several proofs share one `AttestationData` but the proposer cannot + /// aggregate, only one may be kept. Selecting by absolute participant count + /// starves a small subnet whose votes are the only ones still missing from + /// the target's in-state coverage: a larger subnet already counted in-state + /// keeps winning yet adds nothing, so the target never reaches 2/3. + /// + /// This mirrors the union coverage `compact_attestations` achieves when + /// aggregation is enabled: the kept proof must be the one adding the most + /// NEW voters over the target's in-state voter set, not the largest one. + #[test] + fn keep_best_proof_per_data_prefers_marginal_coverage_over_absolute_size() { + let target_root = H256([9u8; 32]); + let att_data = AttestationData { + slot: 5, + head: Checkpoint::default(), + target: Checkpoint { + slot: 5, + root: target_root, + }, + source: Checkpoint::default(), + }; + + // Two candidate proofs for the same data: a 3-validator subnet already + // fully counted in-state, and a 2-validator subnet {3, 7} that is the + // only remaining new coverage. + let large = ( + AggregatedAttestation { + aggregation_bits: make_bits(&[0, 1, 2]), + data: att_data.clone(), + }, + SingleMessageAggregate::empty(make_bits(&[0, 1, 2])), + ); + let small = ( + AggregatedAttestation { + aggregation_bits: make_bits(&[3, 7]), + data: att_data.clone(), + }, + SingleMessageAggregate::empty(make_bits(&[3, 7])), + ); + let entries = vec![large, small]; + + // In-state, validators {0, 1, 2} already voted for this target. + let mut running_votes: HashMap> = HashMap::new(); + running_votes.insert(target_root, HashSet::from([0, 1, 2])); + + let out = keep_best_proof_per_data(entries, &running_votes, att_data.slot); + + assert_eq!( + out.len(), + 1, + "a block carries one entry per AttestationData" + ); + let kept = &out[0].0; + assert_eq!( + kept.aggregation_bits.count_ones(), + 2, + "marginal coverage, not absolute participant count, drives the choice" + ); + assert!( + kept.aggregation_bits.get(3).unwrap_or(false) + && kept.aggregation_bits.get(7).unwrap_or(false), + "the small subnet {{3, 7}} adds the only new coverage and must be kept over the \ + larger {{0, 1, 2}} subnet already counted in-state" + ); + } + + /// With no in-state votes for the target (a fresh target), marginal + /// coverage equals absolute count, so `keep_best_proof_per_data` keeps the + /// larger proof, preserving the pre-existing behavior. + #[test] + fn keep_best_proof_per_data_keeps_largest_when_target_is_fresh() { + let att_data = AttestationData { + slot: 5, + head: Checkpoint::default(), + target: Checkpoint { + slot: 5, + root: H256([9u8; 32]), + }, + source: Checkpoint::default(), + }; + let small = ( + AggregatedAttestation { + aggregation_bits: make_bits(&[0]), + data: att_data.clone(), + }, + SingleMessageAggregate::empty(make_bits(&[0])), + ); + let large = ( + AggregatedAttestation { + aggregation_bits: make_bits(&[1, 2]), + data: att_data.clone(), + }, + SingleMessageAggregate::empty(make_bits(&[1, 2])), + ); + let entries = vec![small, large]; + + let out = keep_best_proof_per_data(entries, &HashMap::new(), att_data.slot); + + assert_eq!(out.len(), 1); + assert_eq!( + out[0].0.aggregation_bits.count_ones(), + 2, + "with an empty in-state voter set the larger proof wins on absolute count" + ); + } + /// Regression test for leanSpec PR #716: build_block must absorb /// gap-closing attestations whose source is justified on the head /// chain but older than `latest_justified` (e.g., a sibling fork From e6ae645691665ab2db8bd89af95b5c51b181050a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Mon, 6 Jul 2026 18:07:04 -0300 Subject: [PATCH 2/4] docs: clean up function documentation --- crates/blockchain/src/block_builder.rs | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/crates/blockchain/src/block_builder.rs b/crates/blockchain/src/block_builder.rs index a6c32520..2a692dc3 100644 --- a/crates/blockchain/src/block_builder.rs +++ b/crates/blockchain/src/block_builder.rs @@ -21,16 +21,16 @@ use ethlambda_state_transition::{ slot_is_justifiable_after, }; use ethlambda_types::{ - ShortRoot, attestation::{AggregatedAttestation, AggregationBits, AttestationData}, block::{AggregatedAttestations, Block, BlockBody, SingleMessageAggregate}, checkpoint::Checkpoint, - primitives::{H256, HashTreeRoot as _}, + primitives::{HashTreeRoot as _, H256}, state::{JustifiedSlots, State}, + ShortRoot, }; use tracing::{info, trace}; -use crate::{MAX_ATTESTATIONS_DATA, metrics, store::StoreError}; +use crate::{metrics, store::StoreError, MAX_ATTESTATIONS_DATA}; /// Post-block checkpoints extracted from the state transition in `build_block`. /// @@ -666,20 +666,8 @@ fn compact_attestations( /// them, so for each group sharing an `AttestationData` exactly one proof /// survives and the rest are dropped. No leanVM aggregation runs. /// -/// The surviving proof is the one adding the most NEW voters over the target's -/// in-state voter set (`running_votes`, keyed by `target.root`), with ties -/// broken by larger absolute participant count, then first occurrence. For a -/// fresh target (no in-state voters) marginal coverage equals absolute count, -/// so the densest proof wins, matching the previous behavior. -/// -/// Scoring by marginal (rather than absolute) coverage keeps this collapse -/// consistent with the union coverage `compact_attestations` reaches when -/// aggregation is enabled, and with `select_attestations`, which already scored -/// the entry on the same marginal coverage. Selecting by absolute participant -/// count instead lets a larger subnet already fully counted in-state keep -/// winning while adding nothing, starving a smaller subnet whose votes are the -/// only ones still missing; the target's coverage then caps below 2/3 and never -/// justifies. +/// The surviving proof is the one adding the most NEW voters, with ties +/// broken by larger absolute participant count, then first occurrence. fn keep_best_proof_per_data( entries: Vec<(AggregatedAttestation, SingleMessageAggregate)>, running_votes: &HashMap>, From d5fa2f316aa94ebc14cf67b8eb35ac97dd6423c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Mon, 6 Jul 2026 18:15:32 -0300 Subject: [PATCH 3/4] style: restore canonical rustfmt import order The docs-cleanup commit reordered imports into an order the pinned rustfmt (1.92.0) does not produce, so `cargo fmt --all -- --check` (run in CI) failed. Re-run `cargo fmt --all` to restore canonical order. --- crates/blockchain/src/block_builder.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/blockchain/src/block_builder.rs b/crates/blockchain/src/block_builder.rs index 2a692dc3..bf55e844 100644 --- a/crates/blockchain/src/block_builder.rs +++ b/crates/blockchain/src/block_builder.rs @@ -21,16 +21,16 @@ use ethlambda_state_transition::{ slot_is_justifiable_after, }; use ethlambda_types::{ + ShortRoot, attestation::{AggregatedAttestation, AggregationBits, AttestationData}, block::{AggregatedAttestations, Block, BlockBody, SingleMessageAggregate}, checkpoint::Checkpoint, - primitives::{HashTreeRoot as _, H256}, + primitives::{H256, HashTreeRoot as _}, state::{JustifiedSlots, State}, - ShortRoot, }; use tracing::{info, trace}; -use crate::{metrics, store::StoreError, MAX_ATTESTATIONS_DATA}; +use crate::{MAX_ATTESTATIONS_DATA, metrics, store::StoreError}; /// Post-block checkpoints extracted from the state transition in `build_block`. /// From 39a056ee95723996de319a1830e05fa4facae372 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Mon, 6 Jul 2026 18:32:52 -0300 Subject: [PATCH 4/4] fix(blockchain): account for cross-group voters when collapsing proofs Two distinct AttestationData entries can share one target.root (differing only in slot, head, or source), and the STF unions all their voters onto that target. keep_best_proof_per_data scored each group's candidates only against the frozen in-state snapshot, so a proof whose voters an earlier group already supplied still looked like new coverage. Both groups could then keep the same subnet, leaving the target under-covered and a projected justify/finalize threshold potentially missed. Visit groups in first-occurrence (block) order and accumulate each winner's voters into a running claimed set; marginal coverage is now measured against in-state voters plus that set, mirroring the block's own vote accumulation. --- crates/blockchain/src/block_builder.rs | 114 ++++++++++++++++++++----- 1 file changed, 93 insertions(+), 21 deletions(-) diff --git a/crates/blockchain/src/block_builder.rs b/crates/blockchain/src/block_builder.rs index bf55e844..9af5ff83 100644 --- a/crates/blockchain/src/block_builder.rs +++ b/crates/blockchain/src/block_builder.rs @@ -701,27 +701,38 @@ fn keep_best_proof_per_data( "Skipping attestation compaction" ); - // Pick the surviving index per group: most new coverage over the target's - // in-state voters, then densest proof, then earliest occurrence. Computed - // over `entries` before it is moved into `items` for extraction. - let best_per_data: Vec = order - .iter() - .map(|data| { - let indices = &groups[data]; - let prior = running_votes.get(&data.target.root); - *indices - .iter() - .max_by_key(|&&idx| { - let (att, proof) = &entries[idx]; - let marginal = proof - .participant_indices() - .filter(|vid| prior.is_none_or(|voted| !voted.contains(vid))) - .count(); - (marginal, att.aggregation_bits.count_ones(), Reverse(idx)) - }) - .expect("group is non-empty") - }) - .collect(); + // Pick the surviving index per group: most new coverage (over in-state + // voters plus those already claimed by earlier groups this block), then + // densest proof, then earliest occurrence. Computed over `entries` before + // it is moved into `items` for extraction. + let mut claimed: HashMap> = HashMap::new(); + let mut best_per_data: Vec = Vec::with_capacity(order.len()); + for data in &order { + let target_root = data.target.root; + let prior = running_votes.get(&target_root); + let block_claimed = claimed.get(&target_root); + let best = *groups[data] + .iter() + .max_by_key(|&&idx| { + let (att, proof) = &entries[idx]; + let marginal = proof + .participant_indices() + .filter(|vid| { + prior.is_none_or(|voted| !voted.contains(vid)) + && block_claimed.is_none_or(|voted| !voted.contains(vid)) + }) + .count(); + (marginal, att.aggregation_bits.count_ones(), Reverse(idx)) + }) + .expect("group is non-empty"); + // Record the winner's voters so a later group sharing this target root + // does not treat them as new coverage. + claimed + .entry(target_root) + .or_default() + .extend(entries[best].1.participant_indices()); + best_per_data.push(best); + } let mut items: Vec> = entries.into_iter().map(Some).collect(); @@ -1443,6 +1454,67 @@ mod tests { ); } + /// Two distinct `AttestationData` entries can share one `target.root` + /// (differing only in `slot`, `head`, or `source`). The STF unions all + /// their voters onto that target, so the collapse must score each group's + /// candidates against voters already claimed by earlier groups in this + /// block, not just the frozen in-state snapshot. Otherwise both groups keep + /// the same subnet and the block under-covers the target. + #[test] + fn keep_best_proof_per_data_accounts_for_voters_claimed_by_earlier_groups() { + let target_root = H256([9u8; 32]); + let target = Checkpoint { + slot: 5, + root: target_root, + }; + // Same target root, different attestation slots -> distinct data roots. + let data_a = AttestationData { + slot: 5, + head: Checkpoint::default(), + target, + source: Checkpoint::default(), + }; + let data_b = AttestationData { + slot: 6, + head: Checkpoint::default(), + target, + source: Checkpoint::default(), + }; + + // A has a single proof {3, 7}. B offers {3, 7} (already claimed by A) + // and {8, 9} (the only new coverage). B's {3, 7} is listed first, so + // absent claim-tracking the first-occurrence tiebreak would keep it. + let entry = |bits: &[usize], data: &AttestationData| { + ( + AggregatedAttestation { + aggregation_bits: make_bits(bits), + data: data.clone(), + }, + SingleMessageAggregate::empty(make_bits(bits)), + ) + }; + let entries = vec![ + entry(&[3, 7], &data_a), + entry(&[3, 7], &data_b), + entry(&[8, 9], &data_b), + ]; + + let out = keep_best_proof_per_data(entries, &HashMap::new(), data_a.slot); + + assert_eq!(out.len(), 2, "one entry per distinct AttestationData"); + assert!( + out[0].0.aggregation_bits.get(3).unwrap_or(false) + && out[0].0.aggregation_bits.get(7).unwrap_or(false), + "group A keeps its only proof {{3, 7}}" + ); + assert!( + out[1].0.aggregation_bits.get(8).unwrap_or(false) + && out[1].0.aggregation_bits.get(9).unwrap_or(false), + "group B must keep {{8, 9}}: {{3, 7}} was already claimed by group A for \ + the same target root, so it adds no new coverage" + ); + } + /// Regression test for leanSpec PR #716: build_block must absorb /// gap-closing attestations whose source is justified on the head /// chain but older than `latest_justified` (e.g., a sibling fork