From 39a916a7098210e27216a1bb732cdf68494a4bd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Thu, 17 Sep 2026 18:07:59 -0300 Subject: [PATCH 1/5] feat(blockchain): score head votes when selecting attestations to pack Selection valued an entry only by the justification voters it added for its target. That misses half of what an attestation carries: its head vote moves LMD-GHOST whether or not its target still needs voters. Two entries bringing the same justification voters were treated as interchangeable even when one moved far more validators' latest head, and an entry whose target was already fully covered was dropped outright despite carrying the freshest head votes anyone had. `ProjectedState` now optionally holds the per-validator latest votes fork choice weighs, seeded from `Store::extract_latest_known_attestations` and advanced as entries are selected so a validator is not credited twice across rounds. `score_entry` reports the new head voters alongside the new justification voters, and `EntryScore::ordering_key` places them immediately after `new_voters` in both tier arms, so head votes break a tie on justification value and never outrank it. An entry that adds only head votes is now kept, at `Build` tier, rather than returned as `None`. It stays at `Build` regardless of the prior vote count, since an entry adding no justification voter cannot push its target past the threshold. The head-vote map is `Option`, not a possibly-empty map. The aggregation worker shares this scorer to pick which group to prove next and leaves it `None`: with no recorded vote every validator in an entry's coverage reads as newly covered, so an empty map would score every entry as maximally valuable and silently disable the worker's zero-value skip. `Store::should_replace_vote` moves to `AttestationData::supersedes` so the scorer applies the same latest-message rule fork choice does rather than a second copy of it. It lands in `ethlambda-types` rather than beside fork choice because the vote map is maintained in the storage layer, which does not depend on the fork choice crate. `build_block` and `select_attestations` take a `ProposalInputs` struct instead of growing another loose parameter each. --- crates/blockchain/src/aggregation.rs | 7 +- crates/blockchain/src/block_builder.rs | 395 ++++++++++++++++++++++--- crates/blockchain/src/store.rs | 16 +- crates/common/types/src/attestation.rs | 66 +++++ crates/storage/src/store.rs | 8 +- 5 files changed, 441 insertions(+), 51 deletions(-) diff --git a/crates/blockchain/src/aggregation.rs b/crates/blockchain/src/aggregation.rs index b9c4e762..118e4b4a 100644 --- a/crates/blockchain/src/aggregation.rs +++ b/crates/blockchain/src/aggregation.rs @@ -523,7 +523,10 @@ fn pick_best_candidate( continue; } - let Some((score, _new_voters)) = + // Head votes are not scored here: the worker's projection leaves + // `head_votes` at `None`, so `new_head_voters` is always empty and the + // zero-new-voters skip below keeps its original meaning. + let Some((score, _new_voters, _new_head_voters)) = projected.score_entry(att_data, &candidate.coverage(), validator_count) else { trace_skipped_candidate("zero_new_voters", att_data, data_root); @@ -2016,6 +2019,7 @@ mod tests { justified_slots: JustifiedSlots::new(), finalized_slot: 0, current_votes: HashMap::new(), + head_votes: None, }; let (picked_root, score) = pick_best_candidate( @@ -2108,6 +2112,7 @@ mod tests { justified_slots: JustifiedSlots::new(), finalized_slot: 0, current_votes: HashMap::new(), + head_votes: None, }; // Round 1: A (6 new voters) outranks B (2 new voters); both Build tier. diff --git a/crates/blockchain/src/block_builder.rs b/crates/blockchain/src/block_builder.rs index f6255964..08dc9511 100644 --- a/crates/blockchain/src/block_builder.rs +++ b/crates/blockchain/src/block_builder.rs @@ -98,8 +98,7 @@ pub(crate) fn build_block( slot: u64, proposer_index: u64, parent_root: H256, - known_block_roots: &HashSet, - aggregated_payloads: &HashMap)>, + inputs: ProposalInputs<'_>, config: ProposerConfig, ) -> Result<(Block, Vec, PostBlockCheckpoints), StoreError> { info!(slot, proposer_index, "Building block"); @@ -109,8 +108,7 @@ pub(crate) fn build_block( head_state, slot, parent_root, - known_block_roots, - aggregated_payloads, + inputs, config.max_attestations_per_block, ); metrics::observe_block_proposal_phase("select_payloads", select_start.elapsed()); @@ -183,10 +181,15 @@ fn select_attestations( head_state: &State, slot: u64, parent_root: H256, - known_block_roots: &HashSet, - aggregated_payloads: &HashMap)>, + inputs: ProposalInputs<'_>, max_attestations_per_block: usize, ) -> Vec<(AggregatedAttestation, SingleMessageAggregate)> { + let ProposalInputs { + known_block_roots, + aggregated_payloads, + latest_head_votes, + } = inputs; + let mut selected: Vec<(AggregatedAttestation, SingleMessageAggregate)> = Vec::new(); if aggregated_payloads.is_empty() { return selected; @@ -213,14 +216,15 @@ fn select_attestations( // Running per-target-root voter set, seeded from state and updated // incrementally as entries are selected. Mirrors the role of Eth2 // participation flags in Prysm/Lighthouse-style packing. - let mut projected = ProjectedState::from_head_state(head_state); + let mut projected = + ProjectedState::from_head_state(head_state).with_head_votes(latest_head_votes); let mut processed_data_roots: HashSet = HashSet::new(); // A block may carry at most `MAX_ATTESTATIONS_DATA` distinct entries // (`on_block` rejects more), so the proposer-side limit never exceeds it. let max_rounds = max_attestations_per_block.min(MAX_ATTESTATIONS_DATA); for _round in 0..max_rounds { - let Some((data_root, score, new_voters)) = + let Some((data_root, score, new_voters, new_head_voters)) = pick_best_candidate(&chain, &processed_data_roots, &projected) else { trace!( @@ -241,6 +245,7 @@ fn select_attestations( trace!( tier = ?score.tier, new_voters = score.new_voters, + new_head_voters = score.new_head_voters, target_slot = score.target_slot, target_root = %ShortRoot(&target_root.0), data_root = %ShortRoot(&data_root.0), @@ -249,6 +254,7 @@ fn select_attestations( ); projected.advance(score.tier, att_data, new_voters); + projected.advance_head_votes(att_data, new_head_voters); } selected @@ -257,16 +263,17 @@ fn select_attestations( /// Scan candidate attestation entries and pick the highest-scoring one. /// /// Skips entries already processed, those failing `entry_passes_filters` -/// (logging the reason), and those with zero new voters. Among remaining -/// entries, returns `(data_root, score, new_voters)` for the entry with the +/// (logging the reason), and those adding neither a justification voter nor a +/// head vote. Among remaining entries, returns +/// `(data_root, score, new_voters, new_head_voters)` for the entry with the /// best `EntryScore::ordering_key` (lower is better). Caller re-indexes /// `chain.aggregated_payloads[&data_root]` for `att_data` and `proofs`. fn pick_best_candidate( chain: &ChainContext<'_>, processed_data_roots: &HashSet, projected: &ProjectedState, -) -> Option<(H256, EntryScore, HashSet)> { - let mut best: Option<(H256, EntryScore, HashSet)> = None; +) -> Option<(H256, EntryScore, HashSet, HashSet)> { + let mut best: Option<(H256, EntryScore, HashSet, HashSet)> = None; let mut best_key: Option = None; for (data_root, (att_data, proofs)) in chain.aggregated_payloads { @@ -286,7 +293,7 @@ fn pick_best_candidate( .iter() .flat_map(|proof| proof.participant_indices()) .collect(); - let Some((score, new_voters)) = + let Some((score, new_voters, new_head_voters)) = projected.score_entry(att_data, &coverage, chain.validator_count) else { trace_skipped_attestation("zero_new_voters", att_data, data_root); @@ -295,7 +302,7 @@ fn pick_best_candidate( let candidate_key = score.ordering_key(*data_root); if best_key.as_ref().is_none_or(|k| candidate_key < *k) { - best = Some((*data_root, score, new_voters)); + best = Some((*data_root, score, new_voters, new_head_voters)); best_key = Some(candidate_key); } } @@ -303,6 +310,26 @@ fn pick_best_candidate( best } +/// What a proposer builds a block out of: the attestation pool plus the two +/// pieces of node-local state used to filter and score it. +/// +/// Grouped rather than passed as loose parameters because they travel together +/// through every entry point here and are all sourced from the same `Store` +/// read in `produce_block_with_signatures`. +pub(crate) struct ProposalInputs<'a> { + /// Roots this node holds a block for. A vote naming an unknown head is not + /// packable, since the state transition could not resolve it. + pub(crate) known_block_roots: &'a HashSet, + /// The attestation pool: `data_root -> (data, proofs)`. + pub(crate) aggregated_payloads: + &'a HashMap)>, + /// Per-validator latest head votes, as fork choice currently holds them. + /// + /// Owned because `Store::extract_latest_known_attestations` already returns + /// a clone, and the projection mutates it as entries are selected. + pub(crate) latest_head_votes: HashMap, +} + /// Static inputs to the attestation selection scan: the candidate pool and /// the chain-level facts used to filter and score entries. Built once before /// the round loop in `select_attestations`. @@ -327,6 +354,17 @@ pub(crate) struct ProjectedState { pub(crate) justified_slots: JustifiedSlots, pub(crate) finalized_slot: u64, pub(crate) current_votes: HashMap>, + /// Each validator's latest head vote as fork choice currently holds it, + /// advanced as entries are selected so a validator is not credited twice + /// across rounds. + /// + /// `None` turns head-vote scoring off entirely, which is not the same as + /// seeding an empty map: with no recorded vote every validator in an + /// entry's coverage reads as newly covered, so an empty map would score + /// every entry as maximally valuable and defeat the zero-value skip. The + /// aggregation worker leaves this `None` — it picks which group to prove, + /// not what a block carries, so head-vote value is not its question. + pub(crate) head_votes: Option>, } impl ProjectedState { @@ -338,9 +376,22 @@ impl ProjectedState { justified_slots: head_state.justified_slots.clone(), finalized_slot: head_state.latest_finalized.slot, current_votes: build_running_votes(head_state), + head_votes: None, } } + /// Seed the per-validator latest head votes, so scoring can value an + /// entry for the fork-choice weight it adds and not only for the + /// justification voters it brings. + /// + /// Takes the map by value: `Store::extract_latest_known_attestations` + /// already hands out an owned clone, so there is nothing to gain by + /// borrowing it and the projection then owns what it mutates. + pub(crate) fn with_head_votes(mut self, head_votes: HashMap) -> Self { + self.head_votes = Some(head_votes); + self + } + /// Fold a selected entry into the projection: record its voters under the /// entry's `target.root`, then advance justification/finalization per /// `tier` (Finalize implies Justify). `new_voters` is the entry's marginal @@ -383,15 +434,61 @@ impl ProjectedState { } } + /// Fold a selected entry's head votes into the projection, so the next + /// round does not credit the same validator for the same head again. + /// + /// Separate from [`ProjectedState::advance`] because that one is handed the + /// entry's *marginal justification* voters, which is a strictly smaller set + /// than the coverage whose head votes this entry carries. Folding head + /// votes there would silently under-credit them. + pub(crate) fn advance_head_votes( + &mut self, + att_data: &AttestationData, + new_head_voters: impl IntoIterator, + ) { + let Some(head_votes) = self.head_votes.as_mut() else { + return; + }; + for validator_id in new_head_voters { + head_votes.insert(validator_id, att_data.clone()); + } + } + + /// The subset of `coverage` whose latest head vote this entry would + /// replace, per the LMD-GHOST latest-message rule + /// ([`AttestationData::supersedes`]). + /// + /// A validator with no recorded vote counts as new: fork choice holds + /// nothing for it, so this entry is the first weight it contributes. + fn new_head_voters(&self, att_data: &AttestationData, coverage: &HashSet) -> HashSet { + let Some(head_votes) = self.head_votes.as_ref() else { + return HashSet::new(); + }; + coverage + .iter() + .copied() + .filter(|vid| { + head_votes + .get(vid) + .is_none_or(|existing| att_data.supersedes(existing)) + }) + .collect() + } + /// Score a candidate entry from its realized validator `coverage` against /// this projection. /// - /// Returns `None` if `coverage` contributes zero validators relative to the - /// running voter set for `att_data.target.root` (no marginal value, drop). - /// On `Some`, the returned `HashSet` is the subset of `coverage` that is new - /// (caller uses it to `advance` the projection without re-scanning - /// `coverage`). A genesis self-vote cannot justify or finalize and is always - /// scored as tier 3. + /// Returns `None` only if the entry is worthless on *both* axes: it adds no + /// justification voter for `att_data.target.root` and no validator's head + /// vote either. An entry that adds head votes alone is kept, at + /// [`Tier::Build`], because its fork-choice weight is real even when its + /// target is already carried: dropping it is how a slot whose votes all + /// name a settled target ends up proposing nothing at all. + /// + /// On `Some`, the returned sets are the subsets of `coverage` that are new + /// on each axis, so the caller can `advance` and `advance_head_votes` the + /// projection without re-scanning `coverage`. A genesis self-vote cannot + /// justify or finalize and is always scored as tier 3. /// /// The caller resolves `coverage` and passes it in: block building unions a /// data's proof participants (see `pick_best_candidate`); committee-signature @@ -403,7 +500,7 @@ impl ProjectedState { att_data: &AttestationData, coverage: &HashSet, validator_count: usize, - ) -> Option<(EntryScore, HashSet)> { + ) -> Option<(EntryScore, HashSet, HashSet)> { let prior_voters = self.current_votes.get(&att_data.target.root); let prior_count = prior_voters.map_or(0, HashSet::len); @@ -412,7 +509,8 @@ impl ProjectedState { .copied() .filter(|vid| prior_voters.is_none_or(|prior| !prior.contains(vid))) .collect(); - if new_voters.is_empty() { + let new_head_voters = self.new_head_voters(att_data, coverage); + if new_voters.is_empty() && new_head_voters.is_empty() { return None; } @@ -430,7 +528,10 @@ impl ProjectedState { && (att_data.source.slot + 1..att_data.target.slot) .all(|s| !slot_is_justifiable_after(s, self.finalized_slot)); - let tier = if is_genesis_self_vote(att_data) || !crosses_2_3 { + // An entry that adds no justification voter cannot move the target past + // the threshold, whatever `prior_count` already sits at, so it stays at + // `Build` regardless of `crosses_2_3` — it is here for its head votes. + let tier = if is_genesis_self_vote(att_data) || !crosses_2_3 || new_voters.is_empty() { Tier::Build } else if finalizes { Tier::Finalize @@ -441,10 +542,11 @@ impl ProjectedState { let score = EntryScore { tier, new_voters: new_voters.len(), + new_head_voters: new_head_voters.len(), target_slot: att_data.target.slot, att_slot: att_data.slot, }; - Some((score, new_voters)) + Some((score, new_voters, new_head_voters)) } /// Validate a candidate entry against the projection and the given chain @@ -531,8 +633,8 @@ pub(crate) enum Tier { /// Tiered score for a candidate `AttestationData` entry during block building. /// -/// Lower `tier` wins. Entries with zero new voters relative to the running -/// per-target-root voter set are dropped (returned as `None`). +/// Lower `tier` wins. Entries that add neither a justification voter nor a +/// head vote are dropped (returned as `None`). /// /// The within-tier ordering is tier-dependent (leanSpec PR #1149): /// @@ -544,11 +646,17 @@ pub(crate) enum Tier { /// coverage leads: more `new_voters`, then larger `target_slot`, then larger /// `att_slot`. /// +/// `new_head_voters` sits immediately after `new_voters` in both tiers, so it +/// breaks a tie on justification value and never outranks it. Two entries that +/// bring the same justification voters are not equivalent: the one whose votes +/// also move more validators' latest head is worth more to fork choice. +/// /// In both tiers `data_root` (ascending) is the final deterministic tiebreak. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) struct EntryScore { pub(crate) tier: Tier, pub(crate) new_voters: usize, + pub(crate) new_head_voters: usize, /// Read only inside [`EntryScore::ordering_key`]; kept private. target_slot: u64, /// Read only inside [`EntryScore::ordering_key`]; kept private. @@ -556,22 +664,31 @@ pub(crate) struct EntryScore { } /// Total order over candidate entries; the smallest value is the best pick. -/// `tier` leads, then three tier-dependent `Reverse`-encoded priorities, then +/// `tier` leads, then four tier-dependent `Reverse`-encoded priorities, then /// `data_root` as the deterministic tiebreak. See [`EntryScore::ordering_key`]. -pub(crate) type OrderingKey = (Tier, Reverse, Reverse, Reverse, H256); +pub(crate) type OrderingKey = ( + Tier, + Reverse, + Reverse, + Reverse, + Reverse, + H256, +); impl EntryScore { /// Sort key where the smallest tuple is the best candidate. `tier` always - /// leads; the remaining three slots carry tier-dependent priorities (see + /// leads; the remaining four slots carry tier-dependent priorities (see /// the type-level docs), all encoded as `Reverse` so "larger is better". pub(crate) fn ordering_key(&self, data_root: H256) -> OrderingKey { let more_new_voters = Reverse(self.new_voters as u64); + let more_new_head_voters = Reverse(self.new_head_voters as u64); let newer_target = Reverse(self.target_slot); let newer_att = Reverse(self.att_slot); match self.tier { Tier::Build => ( self.tier, more_new_voters, + more_new_head_voters, newer_target, newer_att, data_root, @@ -581,6 +698,7 @@ impl EntryScore { newer_target, newer_att, more_new_voters, + more_new_head_voters, data_root, ), } @@ -1056,9 +1174,10 @@ mod tests { justified_slots: JustifiedSlots::new(), finalized_slot: FINALIZED_SLOT, current_votes: HashMap::new(), + head_votes: None, }; - let (score, _) = projected + let (score, _, _) = projected .score_entry(&att_data, &coverage, NUM_VALIDATORS) .expect("entry contributes new voters"); @@ -1069,6 +1188,187 @@ mod tests { ); } + /// An entry scored against a projection whose head votes were never seeded + /// must report zero new head voters. + /// + /// This is the guard for the aggregation worker, which shares this scorer + /// but leaves `head_votes` at `None`. Seeding an empty map instead would + /// make every validator in coverage read as newly covered, so every entry + /// would score as valuable and `score_entry` would stop returning `None` — + /// silently disabling the worker's zero-value skip. + #[test] + fn head_vote_scoring_is_off_when_the_map_is_not_seeded() { + let projected = ProjectedState { + justified_slots: JustifiedSlots::new(), + finalized_slot: 0, + current_votes: HashMap::from([(H256::ZERO, HashSet::from([0, 1, 2]))]), + head_votes: None, + }; + let coverage: HashSet = HashSet::from([0, 1, 2]); + + assert!( + projected + .score_entry(&make_att_data(5), &coverage, 4) + .is_none(), + "an unseeded projection must fall back to justification-only scoring" + ); + } + + /// A vote whose target is fully covered still carries fork-choice weight, + /// so it is kept at `Build` rather than dropped. + #[test] + fn score_entry_keeps_an_entry_that_only_adds_head_votes() { + let att_data = AttestationData { + slot: 9, + head: Checkpoint { + slot: 8, + root: H256([8u8; 32]), + }, + target: Checkpoint { + slot: 6, + root: H256([6u8; 32]), + }, + source: Checkpoint { + slot: 3, + root: H256([3u8; 32]), + }, + }; + let coverage: HashSet = HashSet::from([0, 1, 2]); + + // Every voter already counted toward this target, so there is no + // justification value left; their recorded head vote is older. + let projected = ProjectedState { + justified_slots: JustifiedSlots::new(), + finalized_slot: 0, + current_votes: HashMap::from([(att_data.target.root, coverage.clone())]), + head_votes: Some(HashMap::from([ + (0, make_att_data(4)), + (1, make_att_data(4)), + (2, make_att_data(4)), + ])), + }; + + let (score, new_voters, new_head_voters) = projected + .score_entry(&att_data, &coverage, 4) + .expect("an entry that only moves heads is still worth carrying"); + + assert!( + new_voters.is_empty(), + "the target was already fully covered" + ); + assert_eq!(new_head_voters.len(), 3); + assert_eq!(score.new_head_voters, 3); + assert_eq!( + score.tier, + Tier::Build, + "an entry adding no justification voter cannot justify, whatever \ + the prior count" + ); + } + + /// Worthless on both axes: already counted for the target, and every voter + /// already holds a newer head vote. + #[test] + fn score_entry_drops_an_entry_that_adds_neither_voters_nor_head_votes() { + let coverage: HashSet = HashSet::from([0, 1, 2]); + let projected = ProjectedState { + justified_slots: JustifiedSlots::new(), + finalized_slot: 0, + current_votes: HashMap::from([(H256::ZERO, coverage.clone())]), + head_votes: Some(HashMap::from([ + (0, make_att_data(9)), + (1, make_att_data(9)), + (2, make_att_data(9)), + ])), + }; + + assert!( + projected + .score_entry(&make_att_data(5), &coverage, 4) + .is_none(), + "a vote older than what fork choice already holds adds nothing" + ); + } + + /// Credited head votes do not count twice across selection rounds, and a + /// genuinely newer vote still does. + #[test] + fn advance_head_votes_prevents_double_counting_across_rounds() { + let mut projected = ProjectedState { + justified_slots: JustifiedSlots::new(), + finalized_slot: 0, + current_votes: HashMap::new(), + head_votes: Some(HashMap::new()), + }; + let coverage: HashSet = HashSet::from([0, 1]); + let first = make_att_data(5); + + let credited = projected.new_head_voters(&first, &coverage); + assert_eq!( + credited.len(), + 2, + "no recorded vote means every voter is new" + ); + + projected.advance_head_votes(&first, credited); + assert!( + projected.new_head_voters(&first, &coverage).is_empty(), + "the same entry must not be credited a second time" + ); + assert_eq!( + projected + .new_head_voters(&make_att_data(6), &coverage) + .len(), + 2, + "a later slot still supersedes what this block already credited" + ); + } + + /// Head votes break a tie on justification voters, and never outrank them. + #[test] + fn head_votes_break_a_tie_on_justification_voters() { + let root = H256::ZERO; + let base = EntryScore { + tier: Tier::Build, + new_voters: 3, + new_head_voters: 1, + target_slot: 5, + att_slot: 7, + }; + let more_head = EntryScore { + new_head_voters: 2, + ..base + }; + let more_voters = EntryScore { + new_voters: 4, + new_head_voters: 0, + ..base + }; + + assert!( + more_head.ordering_key(root) < base.ordering_key(root), + "with justification voters tied, more head votes must win" + ); + assert!( + more_voters.ordering_key(root) < more_head.ordering_key(root), + "head votes must not outrank justification voters" + ); + + // Same rule in the Justify arm, where new_voters sits later in the key. + let justify = EntryScore { + tier: Tier::Justify, + ..base + }; + let justify_more_head = EntryScore { + new_head_voters: 2, + ..justify + }; + assert!( + justify_more_head.ordering_key(root) < justify.ordering_key(root), + "the Justify arm must break its new_voters tie on head votes too" + ); + } + /// Regression test for https://github.com/lambdaclass/ethlambda/issues/259 /// /// Simulates a stall scenario by populating the payload pool with 50 @@ -1191,8 +1491,11 @@ mod tests { slot, proposer_index, parent_root, - &known_block_roots, - &aggregated_payloads, + ProposalInputs { + known_block_roots: &known_block_roots, + aggregated_payloads: &aggregated_payloads, + latest_head_votes: HashMap::new(), + }, ProposerConfig { enable_proposer_aggregation: true, max_attestations_per_block: MAX_ATTESTATIONS_DATA, @@ -1337,8 +1640,11 @@ mod tests { slot, proposer_index, parent_root, - &known_block_roots, - &aggregated_payloads, + ProposalInputs { + known_block_roots: &known_block_roots, + aggregated_payloads: &aggregated_payloads, + latest_head_votes: HashMap::new(), + }, ProposerConfig { enable_proposer_aggregation: false, max_attestations_per_block: limit, @@ -1463,8 +1769,11 @@ mod tests { slot, proposer_index, parent_root, - &known_block_roots, - &aggregated_payloads, + ProposalInputs { + known_block_roots: &known_block_roots, + aggregated_payloads: &aggregated_payloads, + latest_head_votes: HashMap::new(), + }, ProposerConfig { enable_proposer_aggregation: false, max_attestations_per_block: MAX_ATTESTATIONS_DATA, @@ -1769,8 +2078,11 @@ mod tests { slot, proposer_index, parent_root, - &known_block_roots, - &aggregated_payloads, + ProposalInputs { + known_block_roots: &known_block_roots, + aggregated_payloads: &aggregated_payloads, + latest_head_votes: HashMap::new(), + }, ProposerConfig { enable_proposer_aggregation: true, max_attestations_per_block: MAX_ATTESTATIONS_DATA, @@ -1905,8 +2217,11 @@ mod tests { slot, proposer_index, parent_root, - &known_block_roots, - &aggregated_payloads, + ProposalInputs { + known_block_roots: &known_block_roots, + aggregated_payloads: &aggregated_payloads, + latest_head_votes: HashMap::new(), + }, ProposerConfig { enable_proposer_aggregation: true, max_attestations_per_block: MAX_ATTESTATIONS_DATA, diff --git a/crates/blockchain/src/store.rs b/crates/blockchain/src/store.rs index 60711f8b..c5f1c8a6 100644 --- a/crates/blockchain/src/store.rs +++ b/crates/blockchain/src/store.rs @@ -18,7 +18,7 @@ use tracing::{info, trace, warn}; use crate::{ GOSSIP_DISPARITY_INTERVALS, INTERVALS_PER_SLOT, MAX_ATTESTATIONS_DATA, SlotInterval, - block_builder::{PostBlockCheckpoints, ProposerConfig, build_block}, + block_builder::{PostBlockCheckpoints, ProposalInputs, ProposerConfig, build_block}, metrics, }; @@ -968,6 +968,17 @@ pub fn produce_block_with_signatures( let known_block_roots = store.get_block_roots().unwrap(); + // The per-validator latest votes fork choice weighs, so selection can value + // an entry for the head weight it adds and not only for the justification + // voters it brings. + let latest_head_votes = store.extract_latest_known_attestations(); + + let inputs = ProposalInputs { + known_block_roots: &known_block_roots, + aggregated_payloads: &aggregated_payloads, + latest_head_votes, + }; + let (block, signatures, post_checkpoints) = { let _timing = metrics::time_block_building_payload_aggregation(); build_block( @@ -975,8 +986,7 @@ pub fn produce_block_with_signatures( slot, validator_index, head_root, - &known_block_roots, - &aggregated_payloads, + inputs, config, )? }; diff --git a/crates/common/types/src/attestation.rs b/crates/common/types/src/attestation.rs index 91d00105..3b06e1b6 100644 --- a/crates/common/types/src/attestation.rs +++ b/crates/common/types/src/attestation.rs @@ -35,6 +35,26 @@ pub struct AttestationData { pub source: Checkpoint, } +impl AttestationData { + /// Whether this vote supersedes `other` as a validator's latest message. + /// + /// The LMD-GHOST latest-message rule: the later slot wins, and a tie is + /// broken by data root. Breaking the tie on a total order rather than on + /// arrival matters because the latest-vote map is written from more than + /// one place (block import, gossip payload insertion, the aggregation + /// worker), so an order-dependent rule would let two nodes that saw the + /// same votes in different orders disagree about the head. + /// + /// Lives here rather than beside fork choice because the vote map is + /// maintained in the storage layer, which does not depend on the fork + /// choice crate; `ethlambda-types` is what storage, blockchain and fork + /// choice all share. + pub fn supersedes(&self, other: &AttestationData) -> bool { + self.slot > other.slot + || (self.slot == other.slot && self.hash_tree_root() > other.hash_tree_root()) + } +} + /// Validator attestation bundled with its signature. /// ///
@@ -205,6 +225,52 @@ impl From for HashedAttestationData { mod tests { use super::*; + fn att_data(slot: u64, head_root: u8) -> AttestationData { + AttestationData { + slot, + head: Checkpoint { + slot, + root: H256([head_root; 32]), + }, + target: Checkpoint::default(), + source: Checkpoint::default(), + } + } + + #[test] + fn supersedes_prefers_the_later_slot() { + let earlier = att_data(4, 1); + let later = att_data(5, 1); + + assert!(later.supersedes(&earlier)); + assert!(!earlier.supersedes(&later)); + } + + #[test] + fn supersedes_is_irreflexive() { + let vote = att_data(4, 1); + + assert!( + !vote.supersedes(&vote), + "a vote does not replace an identical one, or record_vote would clone every duplicate" + ); + } + + /// Same slot: the data root decides, so two nodes that saw the same votes + /// in different orders still agree on which one is a validator's latest. + #[test] + fn supersedes_breaks_a_slot_tie_on_data_root_and_is_antisymmetric() { + let a = att_data(4, 1); + let b = att_data(4, 2); + + assert_ne!(a.hash_tree_root(), b.hash_tree_root()); + assert_eq!( + a.supersedes(&b), + !b.supersedes(&a), + "exactly one of the two must win the tie" + ); + } + /// Build an `AggregationBits` of `len` bits with the indices in `set` flipped on. fn bits(len: usize, set: &[usize]) -> AggregationBits { let mut b = AggregationBits::with_length(len).unwrap(); diff --git a/crates/storage/src/store.rs b/crates/storage/src/store.rs index b92aaad2..d43b57cc 100644 --- a/crates/storage/src/store.rs +++ b/crates/storage/src/store.rs @@ -1512,12 +1512,6 @@ impl Store { // ============ Attestation Extraction ============ - fn should_replace_vote(existing: &AttestationData, candidate: &AttestationData) -> bool { - candidate.slot > existing.slot - || (candidate.slot == existing.slot - && candidate.hash_tree_root() > existing.hash_tree_root()) - } - fn record_vote( votes: &mut HashMap, validator_id: u64, @@ -1525,7 +1519,7 @@ impl Store { ) { let should_replace = votes .get(&validator_id) - .is_none_or(|existing| Self::should_replace_vote(existing, data)); + .is_none_or(|existing| data.supersedes(existing)); if should_replace { votes.insert(validator_id, data.clone()); } From 8d90f97d2742e5a1139b785c191e477fe008ed51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Thu, 17 Sep 2026 19:03:42 -0300 Subject: [PATCH 2/5] feat(blockchain): pack votes for an already-justified target for their head votes Selection dropped every entry whose target was already justified, mirroring `is_valid_vote`. But the two disagree about what that verdict means. The state transition SKIPS such a vote (`is_valid_vote` returns `Ok(false)` and `process_attestations` does `continue`) without rejecting the block, while `insert_signed_block` records every attestation a block carries as a fork-choice vote regardless of that verdict. So the vote is worthless for justification and still moves LMD-GHOST. That makes it a question of value, not validity, and it is now answered by scoring: `entry_passes_filters` admits the entry, and `score_entry` zeroes its justification axis while keeping its head-vote value, so it can only ever win at `Build` tier. Zeroing that axis is what keeps the earlier fix intact: the transition drops a justified target's `justifications` entry, so `current_votes` holds no prior voters for it and a naive score would credit the entire aggregation bitfield as new. Why this matters: on a chain whose `justified - finalized` sits at 6, the justifiable rungs are 3 slots apart (above delta 5 only squares and pronics qualify), so three consecutive slots of validators all vote for the same rung. Once it is justified, every pooled entry hit this filter, `select_attestations` returned an empty list on round 0, and every aggregator built no candidate body at all. Measured on devnet-5: 48% of slots had zero candidates built fleet-wide, and ~50% of blocks were empty, in a clean 3-on/3-off cycle. The aggregation worker now seeds head votes into its projection too. It shares this scorer to choose which group to prove, and without the seed it would score every settled-target group at zero on both axes and prove none of them, leaving the pool empty on exactly the slots this is meant to cover. Adds an STF test pinning the property the packing side depends on: a block carrying a vote for an already-justified target applies cleanly, moves no justification, and opens no tally. If the transition ever started erroring there instead, every proposer packing a settled target would build blocks the network rejects. `snapshot_skips_group_whose_target_is_already_justified` is renamed to `..._is_at_or_behind_finalized`, which is what it actually pins: its target sits below the finalized slot, so `target_not_justifiable` rejects it, and that is still correct. The justified-but-above-finalized case it appeared to cover now has its own test asserting the opposite. --- crates/blockchain/src/aggregation.rs | 90 ++++++- crates/blockchain/src/block_builder.rs | 227 +++++++++++++++--- crates/blockchain/state_transition/src/lib.rs | 72 ++++++ 3 files changed, 346 insertions(+), 43 deletions(-) diff --git a/crates/blockchain/src/aggregation.rs b/crates/blockchain/src/aggregation.rs index 118e4b4a..a23b3756 100644 --- a/crates/blockchain/src/aggregation.rs +++ b/crates/blockchain/src/aggregation.rs @@ -405,7 +405,14 @@ pub fn snapshot_aggregation_inputs( head_state.historical_block_hashes.iter().copied().collect(); extended_historical_block_hashes.push(store.head().expect("head read works")); - let mut projected = block_builder::ProjectedState::from_head_state(&head_state); + // Seed the head votes too, not just the justification projection. Since a + // vote for an already-justified target is no longer filtered out, its only + // remaining value is the fork-choice weight it carries; without this the + // worker would score every such group at zero on both axes and prove none + // of them, leaving the pool empty exactly on the slots where every pooled + // vote names a settled target — which is the case this is meant to cover. + let mut projected = block_builder::ProjectedState::from_head_state(&head_state) + .with_head_votes(store.extract_latest_known_attestations()); let mut jobs: Vec = Vec::with_capacity(max_jobs.min(groups_considered)); for _round in 0..max_jobs { @@ -2202,12 +2209,20 @@ mod tests { ); } - /// A group whose target is already justified (here: at or behind the - /// finalized boundary) can never justify or finalize anything further and - /// must never become a job, even with enough raw sigs to otherwise be - /// viable. + /// A group whose target sits at or behind the finalized boundary must + /// never become a job, even with enough raw sigs to otherwise be viable: + /// it can neither justify nor finalize anything, and a finalized target is + /// settled for good, so its votes carry no fork-choice signal worth proving + /// either. + /// + /// The rejection comes from `target_not_justifiable` + /// (`slot_is_justifiable_after` is false below the finalized slot), not + /// from the target being justified: a justified target *above* the + /// finalized boundary is deliberately still eligible, scored on its head + /// votes alone. See + /// `snapshot_aggregates_a_justified_target_for_its_head_votes`. #[test] - fn snapshot_skips_group_whose_target_is_already_justified() { + fn snapshot_skips_group_whose_target_is_at_or_behind_finalized() { const NUM_VALIDATORS: usize = 10; const HEAD_SLOT: u64 = 20; const FINALIZED_SLOT: u64 = 10; @@ -2244,7 +2259,68 @@ mod tests { assert!( snapshot_aggregation_inputs(&store, 999, MAX_AGGREGATION_JOBS, vacuous_window_config()) .is_none(), - "a group targeting an already-justified slot must never become a job" + "a group targeting a finalized slot must never become a job" + ); + } + + /// The counterpart: a target that is justified but still above the + /// finalized boundary DOES become a job, on the strength of its head votes + /// alone. + /// + /// This is the case that used to be filtered out wholesale. On a chain + /// whose justifiable rungs sit several slots apart, every pooled vote names + /// a settled target for slots at a time; dropping them all left the + /// aggregators with nothing to prove and the next proposer with no + /// candidate body to adopt. + #[test] + fn snapshot_aggregates_a_justified_target_for_its_head_votes() { + const NUM_VALIDATORS: usize = 10; + const HEAD_SLOT: u64 = 20; + const FINALIZED_SLOT: u64 = 10; + const TARGET_SLOT: u64 = 12; // above finalized, and marked justified + + let hashes: Vec = (0..HEAD_SLOT).map(|i| H256([(i + 1) as u8; 32])).collect(); + let mut head_state = make_head_state(HEAD_SLOT, NUM_VALIDATORS, &hashes); + head_state.latest_finalized = Checkpoint { + root: hashes[FINALIZED_SLOT as usize], + slot: FINALIZED_SLOT, + }; + ethlambda_state_transition::justified_slots_ops::extend_to_slot( + &mut head_state.justified_slots, + FINALIZED_SLOT, + TARGET_SLOT, + ); + ethlambda_state_transition::justified_slots_ops::set_justified( + &mut head_state.justified_slots, + FINALIZED_SLOT, + TARGET_SLOT, + ); + let mut store = new_test_store(head_state); + insert_test_block(&mut store, hashes[0], 0, H256::ZERO); + + let att_data = AttestationData { + slot: TARGET_SLOT, + head: Checkpoint { + root: hashes[0], + slot: 0, + }, + target: Checkpoint { + root: hashes[TARGET_SLOT as usize], + slot: TARGET_SLOT, + }, + source: Checkpoint { + root: hashes[0], + slot: 0, + }, + }; + let hashed = HashedAttestationData::new(att_data); + store.insert_gossip_signature(hashed.clone(), 0, dummy_sig()); + store.insert_gossip_signature(hashed, 1, dummy_sig()); + + assert!( + snapshot_aggregation_inputs(&store, 999, MAX_AGGREGATION_JOBS, vacuous_window_config()) + .is_some(), + "a justified target above the finalized boundary must still be proved for its head votes" ); } diff --git a/crates/blockchain/src/block_builder.rs b/crates/blockchain/src/block_builder.rs index 08dc9511..e99d8cd0 100644 --- a/crates/blockchain/src/block_builder.rs +++ b/crates/blockchain/src/block_builder.rs @@ -360,10 +360,16 @@ pub(crate) struct ProjectedState { /// /// `None` turns head-vote scoring off entirely, which is not the same as /// seeding an empty map: with no recorded vote every validator in an - /// entry's coverage reads as newly covered, so an empty map would score - /// every entry as maximally valuable and defeat the zero-value skip. The - /// aggregation worker leaves this `None` — it picks which group to prove, - /// not what a block carries, so head-vote value is not its question. + /// entry's coverage reads as newly covered, so an empty map scores every + /// entry as maximally valuable. That is the right answer when the map is + /// genuinely empty (fork choice holds nothing, so every vote is the first + /// weight its validator contributes) and the wrong one as a stand-in for + /// "not scoring head votes here", which is what `None` is for. + /// + /// Both production callers seed it. It stays optional because the scoring + /// tests construct projections directly, and because a caller that only + /// wants justification scoring should have to say so rather than pass an + /// empty map and get the opposite. pub(crate) head_votes: Option>, } @@ -460,6 +466,24 @@ impl ProjectedState { /// /// A validator with no recorded vote counts as new: fork choice holds /// nothing for it, so this entry is the first weight it contributes. + /// Whether `att_data`'s target is already justified in this projection. + /// + /// An untracked target slot (commonly the head block's own slot, or any + /// slot past the head-seeded window's edge) is not yet justified as far as + /// this projection knows, so `is_slot_justified` returning an error reads + /// as "not justified", not "unknown". Exempt: the genesis self-vote + /// (source == target == slot 0), which fork-choice bootstrapping needs even + /// though its target is trivially "justified". + pub(crate) fn target_already_justified(&self, att_data: &AttestationData) -> bool { + !is_genesis_self_vote(att_data) + && justified_slots_ops::is_slot_justified( + &self.justified_slots, + self.finalized_slot, + att_data.target.slot, + ) + .unwrap_or(false) + } + fn new_head_voters(&self, att_data: &AttestationData, coverage: &HashSet) -> HashSet { let Some(head_votes) = self.head_votes.as_ref() else { return HashSet::new(); @@ -501,14 +525,30 @@ impl ProjectedState { coverage: &HashSet, validator_count: usize, ) -> Option<(EntryScore, HashSet, HashSet)> { + // A settled target contributes no justification voters, whatever its + // coverage. Scoring it normally would credit the whole bitfield as new: + // the state transition drops a justified target's `justifications` + // entry, so `current_votes` holds no prior voters for it and every + // participant would read as marginal. The entry survives on its head + // votes alone, at `Build` tier. + let target_settled = self.target_already_justified(att_data); + let prior_voters = self.current_votes.get(&att_data.target.root); - let prior_count = prior_voters.map_or(0, HashSet::len); + let prior_count = if target_settled { + 0 + } else { + prior_voters.map_or(0, HashSet::len) + }; - let new_voters: HashSet = coverage - .iter() - .copied() - .filter(|vid| prior_voters.is_none_or(|prior| !prior.contains(vid))) - .collect(); + let new_voters: HashSet = if target_settled { + HashSet::new() + } else { + coverage + .iter() + .copied() + .filter(|vid| prior_voters.is_none_or(|prior| !prior.contains(vid))) + .collect() + }; let new_head_voters = self.new_head_voters(att_data, coverage); if new_voters.is_empty() && new_head_voters.is_empty() { return None; @@ -552,15 +592,19 @@ impl ProjectedState { /// Validate a candidate entry against the projection and the given chain /// view. /// - /// Mirrors `state_transition::is_valid_vote`: the entry's head must be - /// known, its source must be justified, its (source, target) must match - /// the candidate-block chain view, `target.slot > source.slot`, target - /// must not already be justified, and target must be a justifiable slot - /// relative to the projected finalized slot. The genesis self-vote - /// (source == target == slot 0) is exempt from the `target.slot > - /// source.slot` and `target_already_justified` checks since fork-choice - /// bootstrapping needs it; STF will silently drop it, but it carries - /// fork-choice signal. + /// Narrower than `state_transition::is_valid_vote`: the entry's head must + /// be known, its source must be justified, its (source, target) must match + /// the candidate-block chain view, `target.slot > source.slot`, and target + /// must be a justifiable slot relative to the projected finalized slot. + /// + /// Deliberately does NOT reject an already-justified target, though + /// `is_valid_vote` skips one: that vote still carries fork-choice weight, + /// so it is scored rather than filtered (see the note at that check, and + /// [`ProjectedState::score_entry`]). + /// + /// The genesis self-vote (source == target == slot 0) is exempt from the + /// `target.slot > source.slot` check since fork-choice bootstrapping needs + /// it; STF will silently drop it, but it carries fork-choice signal. pub(crate) fn entry_passes_filters( &self, att_data: &AttestationData, @@ -592,18 +636,21 @@ impl ProjectedState { if !is_genesis_self_vote && att_data.target.slot <= att_data.source.slot { return Err("target_not_after_source"); } - // An untracked target slot (commonly the head block's own slot) is not yet - // justified, so it stays eligible. Same reasoning as the source check above. - if !is_genesis_self_vote - && justified_slots_ops::is_slot_justified( - &self.justified_slots, - self.finalized_slot, - att_data.target.slot, - ) - .unwrap_or(false) - { - return Err("target_already_justified"); - } + // An already-justified target is deliberately NOT rejected here. + // + // The state transition skips such a vote without rejecting the block + // (`is_valid_vote` returns `Ok(false)` and `process_attestations` does + // `continue`), while `insert_signed_block` records every attestation a + // block carries as a fork-choice vote regardless of that verdict. So + // the vote is worthless for justification and still valuable for + // LMD-GHOST. That is a question of value, not validity, and it is + // answered in `score_entry`, which zeroes the justification axis for a + // settled target and keeps its head-vote value. + // + // Rejecting it here is what left a slot whose votes all name a settled + // target with nothing to propose: on devnet-5 the justifiable rungs sit + // 3 slots apart, so two slots in every three had every pooled entry + // dropped at this line and built no candidate body at all. if !is_genesis_self_vote && !slot_is_justifiable_after(att_data.target.slot, self.finalized_slot) { @@ -1191,11 +1238,11 @@ mod tests { /// An entry scored against a projection whose head votes were never seeded /// must report zero new head voters. /// - /// This is the guard for the aggregation worker, which shares this scorer - /// but leaves `head_votes` at `None`. Seeding an empty map instead would - /// make every validator in coverage read as newly covered, so every entry - /// would score as valuable and `score_entry` would stop returning `None` — - /// silently disabling the worker's zero-value skip. + /// `None` must mean "do not score head votes", not "an empty map": with no + /// recorded vote every validator in coverage reads as newly covered, so an + /// empty map scores every entry as maximally valuable and `score_entry` + /// stops returning `None`. A caller wanting justification-only scoring has + /// to be able to say so without accidentally getting the opposite. #[test] fn head_vote_scoring_is_off_when_the_map_is_not_seeded() { let projected = ProjectedState { @@ -1324,6 +1371,114 @@ mod tests { ); } + /// A settled target is no longer filtered out, and is scored with its + /// justification axis zeroed: its coverage must NOT be credited as new + /// voters just because the state transition dropped its `justifications` + /// entry on justification. + #[test] + fn score_entry_zeroes_the_justification_axis_for_a_settled_target() { + const FINALIZED_SLOT: u64 = 0; + const TARGET_SLOT: u64 = 3; + + let mut justified_slots = JustifiedSlots::new(); + justified_slots_ops::extend_to_slot(&mut justified_slots, FINALIZED_SLOT, TARGET_SLOT); + justified_slots_ops::set_justified(&mut justified_slots, FINALIZED_SLOT, TARGET_SLOT); + + let att_data = AttestationData { + slot: 5, + head: Checkpoint { + slot: 4, + root: H256([4u8; 32]), + }, + target: Checkpoint { + slot: TARGET_SLOT, + root: H256([3u8; 32]), + }, + source: Checkpoint { + slot: 1, + root: H256([1u8; 32]), + }, + }; + let coverage: HashSet = HashSet::from([0, 1, 2, 3]); + + let projected = ProjectedState { + justified_slots, + finalized_slot: FINALIZED_SLOT, + // Empty, exactly as it is after the transition drops a justified + // target's tally. Without the settled-target guard the whole + // coverage would read as new. + current_votes: HashMap::new(), + head_votes: Some(HashMap::new()), + }; + + let (score, new_voters, new_head_voters) = projected + .score_entry(&att_data, &coverage, 4) + .expect("head votes keep the entry alive"); + + assert!( + new_voters.is_empty(), + "a settled target must credit no justification voters" + ); + assert_eq!(score.new_voters, 0); + assert_eq!(new_head_voters.len(), 4, "its head votes are still new"); + assert_eq!( + score.tier, + Tier::Build, + "it cannot justify, so it must not be tiered as if it could" + ); + } + + /// The filter must let a settled target through, since the state transition + /// skips such a vote without rejecting the block while still recording it + /// as a fork-choice vote. Rejecting it here is what left slots whose votes + /// all named a settled target with no candidate body at all. + #[test] + fn entry_passes_filters_admits_an_already_justified_target() { + const FINALIZED_SLOT: u64 = 0; + const TARGET_SLOT: u64 = 2; + + let mut justified_slots = JustifiedSlots::new(); + justified_slots_ops::extend_to_slot(&mut justified_slots, FINALIZED_SLOT, TARGET_SLOT); + justified_slots_ops::set_justified(&mut justified_slots, FINALIZED_SLOT, TARGET_SLOT); + // Source at slot 1 must read as justified for the filter to get past it. + justified_slots_ops::set_justified(&mut justified_slots, FINALIZED_SLOT, 1); + + let roots: Vec = (0..4u8).map(|i| H256([i + 1; 32])).collect(); + let att_data = AttestationData { + slot: 3, + head: Checkpoint { + slot: TARGET_SLOT, + root: roots[TARGET_SLOT as usize], + }, + target: Checkpoint { + slot: TARGET_SLOT, + root: roots[TARGET_SLOT as usize], + }, + source: Checkpoint { + slot: 1, + root: roots[1], + }, + }; + + let projected = ProjectedState { + justified_slots, + finalized_slot: FINALIZED_SLOT, + current_votes: HashMap::new(), + head_votes: None, + }; + let known: HashSet = roots.iter().copied().collect(); + + assert!( + projected.target_already_justified(&att_data), + "fixture must actually have a settled target" + ); + assert_eq!( + projected.entry_passes_filters(&att_data, &known, &roots), + Ok(()), + "a settled target is a scoring question, not a validity one" + ); + } + /// Head votes break a tie on justification voters, and never outrank them. #[test] fn head_votes_break_a_tie_on_justification_voters() { diff --git a/crates/blockchain/state_transition/src/lib.rs b/crates/blockchain/state_transition/src/lib.rs index d03f74b2..4302eb32 100644 --- a/crates/blockchain/state_transition/src/lib.rs +++ b/crates/blockchain/state_transition/src/lib.rs @@ -970,6 +970,78 @@ mod tests { ); } + /// A vote whose target is already justified must be SKIPPED, not rejected: + /// `is_valid_vote` returns `Ok(false)` and the loop does `continue`, so the + /// block carrying it still applies cleanly. + /// + /// The block builder relies on this. It deliberately packs such votes, + /// because they carry no justification value but still move LMD-GHOST + /// (`insert_signed_block` records every attestation a block carries as a + /// fork-choice vote, whatever this function decides). If the transition + /// ever started erroring here instead, every proposer packing a settled + /// target would produce blocks the network rejects. + #[test] + fn process_attestations_skips_an_already_justified_target_without_rejecting_the_block() { + const NUM_VALIDATORS: usize = 4; + let r1 = H256([1u8; 32]); + let r2 = H256([2u8; 32]); + + let mut justified_slots = JustifiedSlots::new(); + justified_slots_ops::extend_to_slot(&mut justified_slots, 0, 1); + justified_slots_ops::set_justified(&mut justified_slots, 0, 1); + + let mut state = State { + config: StateConfig { genesis_time: 0 }, + slot: 3, + latest_block_header: BlockHeader { + slot: 2, + proposer_index: 0, + parent_root: r1, + state_root: H256::ZERO, + body_root: BlockBody::default().hash_tree_root(), + }, + latest_justified: Checkpoint { slot: 1, root: r1 }, + latest_finalized: Checkpoint { + slot: 0, + root: H256::ZERO, + }, + historical_block_hashes: SszList::try_from(vec![H256::ZERO, r1, r2]).unwrap(), + justified_slots, + validators: SszList::try_from(make_validators(NUM_VALIDATORS)).unwrap(), + justifications_roots: SszList::try_from(vec![]).unwrap(), + justifications_validators: JustificationValidators::new(), + }; + + // Target slot 1 is already justified above; source is genesis. + let vote = AggregatedAttestation { + aggregation_bits: make_bits(&[0, 1, 2], NUM_VALIDATORS), + data: AttestationData { + slot: 2, + head: Checkpoint { slot: 1, root: r1 }, + target: Checkpoint { slot: 1, root: r1 }, + source: Checkpoint { + slot: 0, + root: H256::ZERO, + }, + }, + }; + + let before = state.latest_justified; + let atts: AggregatedAttestations = vec![vote].try_into().unwrap(); + + process_attestations(&mut state, &atts) + .expect("an already-justified target is skipped, not an error"); + + assert_eq!( + state.latest_justified, before, + "the skipped vote must not move justification" + ); + assert!( + state.justifications_roots.is_empty(), + "the skipped vote must not open a tally for a settled target" + ); + } + /// leanSpec #1178: `process_attestations` on a state with no validators is /// rejected with a typed error. Belt-and-suspenders: the header stage already /// rejects an empty registry first in the normal flow, but the flat-vote From 280bd16e36ea627eaefc395249c5752bc8831dc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Fri, 18 Sep 2026 11:37:15 -0300 Subject: [PATCH 3/5] fix(blockchain): score head votes against the chain, not against votes we have seen Head-vote scoring measured the wrong thing, and measured it as exactly zero every time. The baseline was `extract_latest_known_attestations`, the map of every vote this node has seen. But that map and the aggregated-payload pool advance in lockstep from the same data, at both stages: `insert_new_aggregated_payload` writes `new_votes` and `new_payloads` in one call, and `promote_new_aggregated_payloads` then drains `new_votes` into `known_votes` and `new_payloads` into `known_payloads`, also in one call. A candidate body is built out of that pool, so it can never carry a vote that supersedes the map it is scored against. `supersedes` is irreflexive, so the answer was always zero. Measured on devnet-5 before this change: `new_head_voters=0` on 54 of 54 adopted candidates, including ones carrying 864 new justification voters. The axis was dead, so the tie-breaker never broke a tie, `score_entry` never kept a head-only entry, and relaxing `entry_passes_filters` to admit already-justified targets admitted entries that scored zero and were dropped one step later. "Have I seen this vote?" is the right question for fork choice and the wrong one for deciding what to PACK. The question that matters there is whether the CHAIN already carries the vote. `ForkChoiceState` gains `on_chain_votes` to answer it, written only by `record_known_attestation_votes`, which is reached only from `insert_signed_block`. Nothing on a gossip, pool or attestation-processing path touches it, which is the entire property that makes it a usable baseline. It is bounded by the validator set (one entry per validator, replaced in place) and needs no pruning. `update_head` and the fork-choice API keep the seen-votes map: fork choice must weigh every vote it knows, not only the ones a block happened to carry. Also fixes two defects the review surfaced in the aggregation worker, both of which this change would otherwise have amplified: - The worker credited head voters again on every selection round, because `pick_best_candidate` discarded them and only `advance` was called. Harmless while the axis was dead; now that it decides ordering, it made later candidates over-score. `pick_best_candidate` carries the voters out and the round loop calls `advance_head_votes`. - The comment claiming the worker leaves `head_votes` at `None` has been false since the projection was seeded, and told a reader the zero-new-voters skip still meant "no justification voters" when it now means "nothing on either axis". Tests pin the property rather than the implementation. At the storage layer, `aggregated_payloads_move_known_votes_but_never_on_chain_votes` fails if the new map ever starts tracking the pool, and `a_pooled_vote_newer_than_the_chain_supersedes_the_on_chain_baseline` asserts both directions: new against the chain, NOT new against the seen-votes map, which is the bug itself written down. At the call site, `snapshot_scores_head_votes_against_the_chain_not_against_seen_votes` promotes a payload so the seen-votes map holds the very vote under test, then requires a job to still be selected; swapping that call site back makes it fail. The accessor-level tests alone would not have caught a reverted call site. --- crates/blockchain/src/aggregation.rs | 122 +++++++++++++++++--- crates/blockchain/src/block_builder.rs | 24 ++-- crates/blockchain/src/store.rs | 12 +- crates/storage/src/store.rs | 153 +++++++++++++++++++++++++ 4 files changed, 287 insertions(+), 24 deletions(-) diff --git a/crates/blockchain/src/aggregation.rs b/crates/blockchain/src/aggregation.rs index a23b3756..6e8cf9c5 100644 --- a/crates/blockchain/src/aggregation.rs +++ b/crates/blockchain/src/aggregation.rs @@ -410,13 +410,19 @@ pub fn snapshot_aggregation_inputs( // remaining value is the fork-choice weight it carries; without this the // worker would score every such group at zero on both axes and prove none // of them, leaving the pool empty exactly on the slots where every pooled - // vote names a settled target — which is the case this is meant to cover. + // vote names a settled target, which is the case this is meant to cover. + // + // The baseline is what the CHAIN carries, not what this node has seen. + // Every aggregate this worker produces is applied back into the pool and + // the fork-choice vote map together (`apply_aggregated_group` on the actor + // thread, then the next promote moves both new->known), so scoring against + // the seen-votes map would report zero for the very groups just proved. let mut projected = block_builder::ProjectedState::from_head_state(&head_state) - .with_head_votes(store.extract_latest_known_attestations()); + .with_head_votes(store.extract_on_chain_votes()); let mut jobs: Vec = Vec::with_capacity(max_jobs.min(groups_considered)); for _round in 0..max_jobs { - let Some((data_root, score)) = pick_best_candidate( + let Some((data_root, score, new_head_voters)) = pick_best_candidate( &candidates, &projected, &known_block_roots, @@ -452,6 +458,10 @@ pub fn snapshot_aggregation_inputs( // same-target candidates re-tier across rounds exactly as the block // builder's post-state would. projected.advance(score.tier, att_data, coverage.iter().copied()); + // Credit its head voters too. Without this a validator counts as newly + // covered again on every round, so later candidates over-score on an + // axis that now decides ordering, and the worker picks the wrong group. + projected.advance_head_votes(att_data, new_head_voters); jobs.push(job); } @@ -515,8 +525,8 @@ fn pick_best_candidate( extended_historical_block_hashes: &[H256], current_slot: u64, validator_count: usize, -) -> Option<(H256, EntryScore)> { - let mut best: Option<(H256, EntryScore)> = None; +) -> Option<(H256, EntryScore, HashSet)> { + let mut best: Option<(H256, EntryScore, HashSet)> = None; let mut best_key: Option<(u8, block_builder::OrderingKey)> = None; for (data_root, candidate) in candidates { @@ -530,10 +540,11 @@ fn pick_best_candidate( continue; } - // Head votes are not scored here: the worker's projection leaves - // `head_votes` at `None`, so `new_head_voters` is always empty and the - // zero-new-voters skip below keeps its original meaning. - let Some((score, _new_voters, _new_head_voters)) = + // Head votes ARE scored here: the projection above is seeded from + // `extract_on_chain_votes`. So this skip now means "adds nothing on + // EITHER axis" rather than "adds no justification voters", and a group + // whose target is already settled survives on its head votes alone. + let Some((score, _new_voters, new_head_voters)) = projected.score_entry(att_data, &candidate.coverage(), validator_count) else { trace_skipped_candidate("zero_new_voters", att_data, data_root); @@ -546,7 +557,7 @@ fn pick_best_candidate( let slot_bucket: u8 = if att_data.slot == current_slot { 0 } else { 1 }; let candidate_key = candidate_ordering_key(slot_bucket, &score, *data_root); if best_key.as_ref().is_none_or(|k| candidate_key < *k) { - best = Some((*data_root, score)); + best = Some((*data_root, score, new_head_voters)); best_key = Some(candidate_key); } } @@ -2029,7 +2040,7 @@ mod tests { head_votes: None, }; - let (picked_root, score) = pick_best_candidate( + let (picked_root, score, _head_voters) = pick_best_candidate( &candidates, &projected, &known_block_roots, @@ -2123,7 +2134,7 @@ mod tests { }; // Round 1: A (6 new voters) outranks B (2 new voters); both Build tier. - let (picked_root, score) = pick_best_candidate( + let (picked_root, score, _head_voters) = pick_best_candidate( &candidates, &projected, &known_block_roots, @@ -2146,7 +2157,7 @@ mod tests { // Round 2: only B remains. Combined with A's now-recorded 6 voters, // B's 2 new voters cross 2/3 of 10 — B is re-tiered from what would // have been Build in isolation to Justify. - let (picked_root, score) = pick_best_candidate( + let (picked_root, score, _head_voters) = pick_best_candidate( &candidates, &projected, &known_block_roots, @@ -2263,6 +2274,91 @@ mod tests { ); } + /// Regression guard for the CALL SITE, not the accessor. + /// + /// `snapshot_aggregation_inputs` must score head votes against the votes + /// the CHAIN carries (`extract_on_chain_votes`), never against the votes + /// this node has merely seen (`extract_latest_known_attestations`). The two + /// look interchangeable and both compile, but the seen-votes map advances + /// in lockstep with the very pool these jobs are selected from + /// (`insert_new_aggregated_payload` writes `new_votes` + `new_payloads`, + /// then `promote_new_aggregated_payloads` drains both into their `known` + /// counterparts), so scoring against it reports zero for every group and + /// silently kills the whole head-vote axis. That shipped twice. + /// + /// So: promote a payload for this exact attestation, which populates + /// `known_votes` while leaving `on_chain_votes` empty. A job must still be + /// selected. Swapping the call site back to the seen-votes map makes this + /// assertion fail, which is the entire point of the test. + #[test] + fn snapshot_scores_head_votes_against_the_chain_not_against_seen_votes() { + const NUM_VALIDATORS: usize = 10; + const HEAD_SLOT: u64 = 20; + const FINALIZED_SLOT: u64 = 10; + const TARGET_SLOT: u64 = 12; + + let hashes: Vec = (0..HEAD_SLOT).map(|i| H256([(i + 1) as u8; 32])).collect(); + let mut head_state = make_head_state(HEAD_SLOT, NUM_VALIDATORS, &hashes); + head_state.latest_finalized = Checkpoint { + root: hashes[FINALIZED_SLOT as usize], + slot: FINALIZED_SLOT, + }; + ethlambda_state_transition::justified_slots_ops::extend_to_slot( + &mut head_state.justified_slots, + FINALIZED_SLOT, + TARGET_SLOT, + ); + ethlambda_state_transition::justified_slots_ops::set_justified( + &mut head_state.justified_slots, + FINALIZED_SLOT, + TARGET_SLOT, + ); + let mut store = new_test_store(head_state); + insert_test_block(&mut store, hashes[0], 0, H256::ZERO); + + let att_data = AttestationData { + slot: TARGET_SLOT, + head: Checkpoint { + root: hashes[0], + slot: 0, + }, + target: Checkpoint { + root: hashes[TARGET_SLOT as usize], + slot: TARGET_SLOT, + }, + source: Checkpoint { + root: hashes[0], + slot: 0, + }, + }; + let hashed = HashedAttestationData::new(att_data.clone()); + store.insert_gossip_signature(hashed.clone(), 0, dummy_sig()); + store.insert_gossip_signature(hashed.clone(), 1, dummy_sig()); + + // Put this very vote into the SEEN map, the way the worker's own output + // lands there, while leaving the on-chain map untouched. + let mut bits = AggregationBits::with_length(NUM_VALIDATORS).unwrap(); + bits.set(0, true).unwrap(); + bits.set(1, true).unwrap(); + store.insert_new_aggregated_payload(hashed, SingleMessageAggregate::empty(bits)); + store.promote_new_aggregated_payloads(); + assert!( + !store.extract_latest_known_attestations().is_empty(), + "fixture must actually populate the seen-votes map" + ); + assert!( + store.extract_on_chain_votes().is_empty(), + "fixture must leave the on-chain map empty: no block carried this" + ); + + assert!( + snapshot_aggregation_inputs(&store, 999, MAX_AGGREGATION_JOBS, vacuous_window_config()) + .is_some(), + "a vote the chain does not carry is still worth proving, however \ + many times this node has already seen it" + ); + } + /// The counterpart: a target that is justified but still above the /// finalized boundary DOES become a job, on the strength of its head votes /// alone. diff --git a/crates/blockchain/src/block_builder.rs b/crates/blockchain/src/block_builder.rs index e99d8cd0..8714bf65 100644 --- a/crates/blockchain/src/block_builder.rs +++ b/crates/blockchain/src/block_builder.rs @@ -323,9 +323,13 @@ pub(crate) struct ProposalInputs<'a> { /// The attestation pool: `data_root -> (data, proofs)`. pub(crate) aggregated_payloads: &'a HashMap)>, - /// Per-validator latest head votes, as fork choice currently holds them. + /// Per-validator latest head votes that the CHAIN already carries. /// - /// Owned because `Store::extract_latest_known_attestations` already returns + /// Deliberately not the votes fork choice holds: that map advances in + /// lockstep with the pool these entries come from, so every entry would + /// score zero new head voters. See `ForkChoiceState::on_chain_votes`. + /// + /// Owned because `Store::extract_on_chain_votes` already returns /// a clone, and the projection mutates it as entries are selected. pub(crate) latest_head_votes: HashMap, } @@ -354,16 +358,22 @@ pub(crate) struct ProjectedState { pub(crate) justified_slots: JustifiedSlots, pub(crate) finalized_slot: u64, pub(crate) current_votes: HashMap>, - /// Each validator's latest head vote as fork choice currently holds it, + /// Each validator's latest head vote that the CHAIN already carries, /// advanced as entries are selected so a validator is not credited twice /// across rounds. /// /// `None` turns head-vote scoring off entirely, which is not the same as /// seeding an empty map: with no recorded vote every validator in an /// entry's coverage reads as newly covered, so an empty map scores every - /// entry as maximally valuable. That is the right answer when the map is - /// genuinely empty (fork choice holds nothing, so every vote is the first - /// weight its validator contributes) and the wrong one as a stand-in for + /// entry as maximally valuable. + /// + /// An empty map is a real state here, and it no longer means what it meant + /// when this was seeded from fork choice: a node that has just resumed + /// holds a full set of gossip-learned votes within a slot while it has + /// still seen no block, so `on_chain_votes` is empty and every entry scores + /// its whole coverage. That errs toward packing more rather than less, it + /// is capped by `max_attestations_per_block`, and it resolves on the first + /// import that carries attestations. What it must NOT be is a stand-in for /// "not scoring head votes here", which is what `None` is for. /// /// Both production callers seed it. It stays optional because the scoring @@ -390,7 +400,7 @@ impl ProjectedState { /// entry for the fork-choice weight it adds and not only for the /// justification voters it brings. /// - /// Takes the map by value: `Store::extract_latest_known_attestations` + /// Takes the map by value: `Store::extract_on_chain_votes` /// already hands out an owned clone, so there is nothing to gain by /// borrowing it and the projection then owns what it mutates. pub(crate) fn with_head_votes(mut self, head_votes: HashMap) -> Self { diff --git a/crates/blockchain/src/store.rs b/crates/blockchain/src/store.rs index c5f1c8a6..049a5d43 100644 --- a/crates/blockchain/src/store.rs +++ b/crates/blockchain/src/store.rs @@ -968,10 +968,14 @@ pub fn produce_block_with_signatures( let known_block_roots = store.get_block_roots().unwrap(); - // The per-validator latest votes fork choice weighs, so selection can value - // an entry for the head weight it adds and not only for the justification - // voters it brings. - let latest_head_votes = store.extract_latest_known_attestations(); + // The latest vote per validator the CHAIN already carries, so selection can + // value an entry for the head weight it would ADD and not only for the + // justification voters it brings. + // + // Deliberately not `extract_latest_known_attestations`: that map is written + // in lockstep with the aggregated-payload pool this block is built from, so + // every entry would score zero new head voters and the axis would be dead. + let latest_head_votes = store.extract_on_chain_votes(); let inputs = ProposalInputs { known_block_roots: &known_block_roots, diff --git a/crates/storage/src/store.rs b/crates/storage/src/store.rs index d43b57cc..c3f61958 100644 --- a/crates/storage/src/store.rs +++ b/crates/storage/src/store.rs @@ -333,6 +333,32 @@ type BlockRootIndexChanges = (Vec, Vec); struct ForkChoiceState { known_votes: HashMap, new_votes: HashMap, + /// Latest vote per validator that a BLOCK has carried, as opposed to one + /// this node merely learned about. + /// + /// Maintained only from `insert_signed_block`, never from gossip or the + /// aggregation pool, which is the whole point: the vote maps and the + /// payload buffers advance in lockstep from the same data, at both stages. + /// [`Store::insert_new_aggregated_payload`] writes `new_votes` and + /// `new_payloads` in one call, and [`Store::promote_new_aggregated_payloads`] + /// then drains `new_votes` into `known_votes` and `new_payloads` into + /// `known_payloads`, also in one call. So `known_votes` and the pool that + /// `known_aggregated_payloads` serves are two views of the same set of + /// votes, and a candidate body built out of that pool can never carry a + /// vote newer than `known_votes`. Scoring a body's head-vote value against + /// `known_votes` therefore always yields zero and is the wrong question. + /// The one that matters when deciding what to PACK is whether the chain + /// already carries the vote, which is exactly what this map answers. + /// + /// Not fork-aware: a vote carried by a block on an abandoned branch still + /// counts as on-chain here. That only ever makes a vote look less novel + /// than it is, so the failure mode is packing slightly less rather than + /// double-counting, and keeping it fork-aware would cost an ancestor walk + /// per scoring call to save nothing at the scale we run at. + /// + /// Bounded by the validator-set size: one entry per validator, replaced in + /// place, so it needs no pruning as the chain advances. + on_chain_votes: HashMap, } /// Bounded buffer for gossip signatures with FIFO eviction. @@ -1525,6 +1551,14 @@ impl Store { } } + /// Record a block's attestations as fork-choice votes. + /// + /// Called from `insert_signed_block` only, so it doubles as the one place + /// that learns a vote is now ON CHAIN. Both maps are updated: `known_votes` + /// is what fork choice weighs, `on_chain_votes` is the baseline block + /// production scores a candidate body's head-vote value against. See + /// [`ForkChoiceState::on_chain_votes`] for why the two cannot be the same + /// map. fn record_known_attestation_votes(&self, attestations: &[AggregatedAttestation]) { let mut fork_choice = self.fork_choice.lock().unwrap(); for attestation in attestations { @@ -1534,6 +1568,11 @@ impl Store { validator_id, &attestation.data, ); + Self::record_vote( + &mut fork_choice.on_chain_votes, + validator_id, + &attestation.data, + ); } } } @@ -1543,6 +1582,18 @@ impl Store { self.fork_choice.lock().unwrap().known_votes.clone() } + /// Extract the latest vote per validator that a block has already carried. + /// + /// The baseline for scoring how much fork-choice weight a candidate body + /// would ADD to the chain. Deliberately not + /// [`Self::extract_latest_known_attestations`]: that map is written in + /// lockstep with the aggregated-payload pool bodies are built from, so + /// scoring against it reports zero for every candidate. See + /// [`ForkChoiceState::on_chain_votes`]. + pub fn extract_on_chain_votes(&self) -> HashMap { + self.fork_choice.lock().unwrap().on_chain_votes.clone() + } + /// Extract per-validator latest attestations from new (pending) payloads. pub fn extract_latest_new_attestations(&self) -> HashMap { self.fork_choice.lock().unwrap().new_votes.clone() @@ -2115,6 +2166,108 @@ mod tests { assert_eq!(votes[&3], data); } + /// The pool and `known_votes` are written in lockstep, so a candidate body + /// built from the pool can never carry a vote newer than `known_votes`. + /// `on_chain_votes` must NOT move with them, or head-vote scoring reports + /// zero for every candidate and the whole axis is dead. + /// + /// This is the regression guard for exactly that: scoring a body's + /// head-vote value against `known_votes` looks reasonable and silently + /// always returns nothing. + #[test] + fn aggregated_payloads_move_known_votes_but_never_on_chain_votes() { + let mut store = Store::test_store(); + let data = make_att_data_for_target(8, root(8)); + + store.insert_new_aggregated_payload( + HashedAttestationData::new(data.clone()), + make_proof_for_validator(0), + ); + store.promote_new_aggregated_payloads(); + + assert_eq!( + store.extract_latest_known_attestations()[&0], + data, + "the pool write must reach the fork-choice map" + ); + assert!( + store.extract_on_chain_votes().is_empty(), + "no block has carried this vote, so it is not on chain" + ); + } + + /// The other half: a block import is what makes a vote on-chain, and it + /// must move BOTH maps. + #[test] + fn insert_signed_block_records_on_chain_votes() { + let mut store = Store::test_store(); + let data = make_att_data_for_target(8, root(8)); + let block = signed_block_with_attestations( + 1, + H256::ZERO, + vec![AggregatedAttestation { + aggregation_bits: make_proof_for_validators(&[1, 3]).participants, + data: data.clone(), + }], + ); + let block_root = block.message.hash_tree_root(); + + store + .insert_signed_block(block_root, block) + .expect("insert signed block"); + + let on_chain = store.extract_on_chain_votes(); + assert_eq!(on_chain[&1], data); + assert_eq!(on_chain[&3], data); + assert_eq!( + store.extract_latest_known_attestations()[&1], + data, + "a block import still feeds fork choice as before" + ); + } + + /// A pooled vote strictly newer than what the chain carries must read as + /// new against the on-chain baseline. This is the property the whole fix + /// turns on: if it fails, candidates score zero again. + #[test] + fn a_pooled_vote_newer_than_the_chain_supersedes_the_on_chain_baseline() { + let mut store = Store::test_store(); + let on_chain_data = make_att_data_for_target(8, root(8)); + let block = signed_block_with_attestations( + 1, + H256::ZERO, + vec![AggregatedAttestation { + aggregation_bits: make_proof_for_validator(0).participants, + data: on_chain_data.clone(), + }], + ); + store + .insert_signed_block(block.message.hash_tree_root(), block) + .expect("insert signed block"); + + // A later attestation from the same validator, still only in the pool. + let fresher = make_att_data_for_target(9, root(9)); + store.insert_new_aggregated_payload( + HashedAttestationData::new(fresher.clone()), + make_proof_for_validator(0), + ); + store.promote_new_aggregated_payloads(); + + let on_chain = store.extract_on_chain_votes(); + assert_eq!( + on_chain[&0], on_chain_data, + "the pool must not advance the on-chain baseline" + ); + assert!( + fresher.supersedes(&on_chain[&0]), + "the pooled vote must read as new against what the chain carries" + ); + assert!( + !fresher.supersedes(&store.extract_latest_known_attestations()[&0]), + "and must read as NOT new against known_votes, which is the bug this fixes" + ); + } + #[test] fn prune_old_block_proofs_within_retention() { let backend = Arc::new(InMemoryBackend::new()); From 371099bdf79b0db0e8e27a3203f4f9bf16ebfc79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Fri, 18 Sep 2026 11:42:36 -0300 Subject: [PATCH 4/5] docs(blockchain): correct comments the on-chain vote baseline invalidated Follow-up from review of the previous commit. No behaviour change except the added test. - `new_head_voters`' doc comment had been concatenated onto `target_already_justified` by an earlier conflict resolution, leaving `new_head_voters` undocumented and attributing head-vote reasoning to a justification predicate. Split back apart, and the surviving text now says the count is measured against what the chain carries. - `supersedes` justified its total-order tiebreak by the vote map having several writers. Still true of the seen-votes map, not of the on-chain one, which has exactly one; it relies on the same total order for a different reason, namely independence from import interleaving. - `reaggregate` skips attestations whose target is at or behind the justified checkpoint, and said it does so because such votes "carry no fork-choice value". Selection now packs exactly those votes for their fork-choice value, so that reason is the opposite of what the code elsewhere relies on. The skip is correct and stays: the vote is already on chain in the block being imported, so splitting it back into the pool would let it be repacked indefinitely, paying a SNARK per round. Only the stated reason changes. Adds `select_skips_a_group_whose_vote_the_chain_already_carries`, covering the suppression direction. Every other test in this area asserts that a group IS selected, and with an empty on-chain baseline everything scores its full coverage, so "always selects" and "correctly selects" were indistinguishable. Note this test passes under either baseline, since a block import writes both maps; the guard against a reverted call site is `snapshot_scores_head_votes_against_the_chain_not_against_seen_votes`. --- crates/blockchain/src/aggregation.rs | 94 ++++++++++++++++++++++++++ crates/blockchain/src/block_builder.rs | 13 ++-- crates/blockchain/src/reaggregate.rs | 6 +- crates/common/types/src/attestation.rs | 6 +- 4 files changed, 110 insertions(+), 9 deletions(-) diff --git a/crates/blockchain/src/aggregation.rs b/crates/blockchain/src/aggregation.rs index 6e8cf9c5..990d870d 100644 --- a/crates/blockchain/src/aggregation.rs +++ b/crates/blockchain/src/aggregation.rs @@ -2359,6 +2359,100 @@ mod tests { ); } + /// The suppression direction, which the other tests never exercise: once a + /// block HAS carried the vote, the group is worth nothing on either axis + /// and must not become a job. + /// + /// Without this, "always selects" and "correctly selects" look identical: + /// an empty on-chain baseline makes every group score its full coverage, so + /// a test that only ever asserts `is_some()` passes even if the baseline is + /// ignored outright. + #[test] + fn select_skips_a_group_whose_vote_the_chain_already_carries() { + const NUM_VALIDATORS: usize = 10; + const HEAD_SLOT: u64 = 20; + const FINALIZED_SLOT: u64 = 10; + const TARGET_SLOT: u64 = 12; + + let hashes: Vec = (0..HEAD_SLOT).map(|i| H256([(i + 1) as u8; 32])).collect(); + let mut head_state = make_head_state(HEAD_SLOT, NUM_VALIDATORS, &hashes); + head_state.latest_finalized = Checkpoint { + root: hashes[FINALIZED_SLOT as usize], + slot: FINALIZED_SLOT, + }; + ethlambda_state_transition::justified_slots_ops::extend_to_slot( + &mut head_state.justified_slots, + FINALIZED_SLOT, + TARGET_SLOT, + ); + ethlambda_state_transition::justified_slots_ops::set_justified( + &mut head_state.justified_slots, + FINALIZED_SLOT, + TARGET_SLOT, + ); + let mut store = new_test_store(head_state); + insert_test_block(&mut store, hashes[0], 0, H256::ZERO); + + let att_data = AttestationData { + slot: TARGET_SLOT, + head: Checkpoint { + root: hashes[0], + slot: 0, + }, + target: Checkpoint { + root: hashes[TARGET_SLOT as usize], + slot: TARGET_SLOT, + }, + source: Checkpoint { + root: hashes[0], + slot: 0, + }, + }; + let hashed = HashedAttestationData::new(att_data.clone()); + store.insert_gossip_signature(hashed.clone(), 0, dummy_sig()); + store.insert_gossip_signature(hashed, 1, dummy_sig()); + + // Now put this exact vote ON CHAIN for both participants. + let mut bits = AggregationBits::with_length(NUM_VALIDATORS).unwrap(); + bits.set(0, true).unwrap(); + bits.set(1, true).unwrap(); + let block = SignedBlock { + message: Block { + slot: 1, + proposer_index: 0, + parent_root: hashes[0], + state_root: H256::ZERO, + body: BlockBody { + attestations: vec![ethlambda_types::attestation::AggregatedAttestation { + aggregation_bits: bits, + data: att_data, + }] + .try_into() + .unwrap(), + }, + }, + proof: MultiMessageAggregate::default(), + }; + let block_root = { + use ethlambda_types::primitives::HashTreeRoot as _; + block.message.hash_tree_root() + }; + store + .insert_signed_block(block_root, block) + .expect("insert block carrying the vote"); + assert_eq!( + store.extract_on_chain_votes().len(), + 2, + "fixture must actually put the vote on chain" + ); + + assert!( + snapshot_aggregation_inputs(&store, 999, MAX_AGGREGATION_JOBS, vacuous_window_config()) + .is_none(), + "the chain already carries this vote, so it adds nothing on either axis" + ); + } + /// The counterpart: a target that is justified but still above the /// finalized boundary DOES become a job, on the strength of its head votes /// alone. diff --git a/crates/blockchain/src/block_builder.rs b/crates/blockchain/src/block_builder.rs index 8714bf65..6dc82205 100644 --- a/crates/blockchain/src/block_builder.rs +++ b/crates/blockchain/src/block_builder.rs @@ -470,12 +470,6 @@ impl ProjectedState { } } - /// The subset of `coverage` whose latest head vote this entry would - /// replace, per the LMD-GHOST latest-message rule - /// ([`AttestationData::supersedes`]). - /// - /// A validator with no recorded vote counts as new: fork choice holds - /// nothing for it, so this entry is the first weight it contributes. /// Whether `att_data`'s target is already justified in this projection. /// /// An untracked target slot (commonly the head block's own slot, or any @@ -494,6 +488,13 @@ impl ProjectedState { .unwrap_or(false) } + /// The subset of `coverage` whose latest head vote this entry would + /// replace, per the LMD-GHOST latest-message rule + /// ([`AttestationData::supersedes`]). + /// + /// Measured against the votes the CHAIN already carries, so a validator + /// with no entry counts as new: no block has carried a vote for it, so this + /// entry is the first weight it would contribute on chain. fn new_head_voters(&self, att_data: &AttestationData, coverage: &HashSet) -> HashSet { let Some(head_votes) = self.head_votes.as_ref() else { return HashSet::new(); diff --git a/crates/blockchain/src/reaggregate.rs b/crates/blockchain/src/reaggregate.rs index 13b433c5..43330ae6 100644 --- a/crates/blockchain/src/reaggregate.rs +++ b/crates/blockchain/src/reaggregate.rs @@ -15,7 +15,11 @@ //! 1. Only deconstructing when the chain is in sync — backfilling nodes //! must not flood gossip with rederived aggregates. //! 2. Skipping attestations whose target is at or behind the store's -//! justified checkpoint — they carry no fork-choice value. +//! justified checkpoint. Note this is NOT because such a vote is worthless: +//! selection deliberately packs one for the LMD-GHOST weight it carries. +//! It is because the vote is already ON CHAIN, in the very block being +//! imported, so splitting it back into the pool would let it be repacked +//! indefinitely while paying a SNARK for each round. //! 3. Skipping attestations whose participants are already a subset of the //! local union for that data — nothing to recover. //! 4. Capping the number of splits per imported block at diff --git a/crates/common/types/src/attestation.rs b/crates/common/types/src/attestation.rs index 3b06e1b6..dae73376 100644 --- a/crates/common/types/src/attestation.rs +++ b/crates/common/types/src/attestation.rs @@ -40,10 +40,12 @@ impl AttestationData { /// /// The LMD-GHOST latest-message rule: the later slot wins, and a tie is /// broken by data root. Breaking the tie on a total order rather than on - /// arrival matters because the latest-vote map is written from more than + /// arrival matters because the seen-votes map is written from more than /// one place (block import, gossip payload insertion, the aggregation /// worker), so an order-dependent rule would let two nodes that saw the - /// same votes in different orders disagree about the head. + /// same votes in different orders disagree about the head. The on-chain + /// vote map is the exception, written only on block import, and relies on + /// the same total order to stay independent of import interleaving. /// /// Lives here rather than beside fork choice because the vote map is /// maintained in the storage layer, which does not depend on the fork From f4f0b9dd2574a698bb7937826e1f645cbda45d29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Fri, 18 Sep 2026 12:29:29 -0300 Subject: [PATCH 5/5] feat(blockchain): add a TargetAdvance tier and rank head votes above voters Head weight had no tier of its own. An entry could bring 2/3 of the validator set's latest head votes onto a root and still be scored `Build`, indistinguishable from one adding a single marginal vote below every threshold. `Justify` exists because crossing 2/3 on a target is categorically different from approaching it; the same is true on the head axis, and nothing expressed it. `TargetAdvance` sits between `Justify` and `Build`: below `Justify` because finality beats head weight, above `Build` because crossing a threshold beats approaching one. It is claimed only when the entry itself moves head votes AND the projected post-state puts 2/3 of validators on this entry's head root. The "itself moves" half matters: without it an entry that shifts nobody could claim a threshold that was already met, which is exactly the miscount the justification axis had, where a settled target's whole coverage read as new. `head_crosses_2_3` counts over the post-state, the same way `crosses_2_3` does. Ordering, per tier: Finalize/Justify newer_target > newer_att > head_votes > voters > root TargetAdvance newer_att > head_votes > root Build voters > head_votes > newer_target > newer_att > root Two changes from before. In the justify arm head votes now outrank justification voters: past the 2/3 target threshold the marginal justification voter buys little, while the head weight riding along with it still moves fork choice. `TargetAdvance` ranks on recency first and does not consult `newer_target` at all, since the target is by definition not moving at that tier, nor `more_new_voters`, since an entry there was chosen for head weight. The two `OrderingKey` slots it leaves unranked take a constant, which cannot discriminate. `Build` is unchanged. `advance` keys on `tier <= Tier::Justify`, so inserting a variant below `Justify` leaves justification bookkeeping untouched. Two existing tests asserted `Tier::Build` for entries that now legitimately reach `TargetAdvance`: both move a supermajority of heads. The invariant each was written to guard is that an entry adding no justification voter must not be tiered as if it justified, which still holds and is now asserted directly (`tier > Tier::Justify`) rather than implied by a `Build` literal. Neither was weakened. --- crates/blockchain/src/block_builder.rs | 313 +++++++++++++++++++++++-- 1 file changed, 291 insertions(+), 22 deletions(-) diff --git a/crates/blockchain/src/block_builder.rs b/crates/blockchain/src/block_builder.rs index 6dc82205..8eb84dbc 100644 --- a/crates/blockchain/src/block_builder.rs +++ b/crates/blockchain/src/block_builder.rs @@ -495,6 +495,35 @@ impl ProjectedState { /// Measured against the votes the CHAIN already carries, so a validator /// with no entry counts as new: no block has carried a vote for it, so this /// entry is the first weight it would contribute on chain. + /// Whether applying this entry puts 2/3 of the validator set's latest head + /// votes on `att_data.head.root`. + /// + /// The head-side analogue of `crosses_2_3` for justification, and counted + /// the same way: over the projected POST-state, not the delta. A validator + /// counts when the entry moves it onto this head, or when it already names + /// this head and the entry does not move it elsewhere. + /// + /// `None` head votes means the axis is switched off, so no supermajority + /// can be claimed. + fn head_crosses_2_3( + &self, + att_data: &AttestationData, + new_head_voters: &HashSet, + validator_count: usize, + ) -> bool { + let Some(head_votes) = self.head_votes.as_ref() else { + return false; + }; + let head_root = att_data.head.root; + // Everyone this entry moves lands on `head_root` by construction. + let retained = head_votes + .iter() + .filter(|(vid, vote)| vote.head.root == head_root && !new_head_voters.contains(vid)) + .count(); + let total = retained + new_head_voters.len(); + 3 * total >= 2 * validator_count + } + fn new_head_voters(&self, att_data: &AttestationData, coverage: &HashSet) -> HashSet { let Some(head_votes) = self.head_votes.as_ref() else { return HashSet::new(); @@ -516,14 +545,16 @@ impl ProjectedState { /// Returns `None` only if the entry is worthless on *both* axes: it adds no /// justification voter for `att_data.target.root` and no validator's head /// vote either. An entry that adds head votes alone is kept, at - /// [`Tier::Build`], because its fork-choice weight is real even when its - /// target is already carried: dropping it is how a slot whose votes all - /// name a settled target ends up proposing nothing at all. + /// [`Tier::TargetAdvance`] if those votes carry the head past 2/3 and + /// [`Tier::Build`] otherwise, because its fork-choice weight is real even + /// when its target is already carried: dropping it is how a slot whose + /// votes all name a settled target ends up proposing nothing at all. /// /// On `Some`, the returned sets are the subsets of `coverage` that are new /// on each axis, so the caller can `advance` and `advance_head_votes` the /// projection without re-scanning `coverage`. A genesis self-vote cannot - /// justify or finalize and is always scored as tier 3. + /// justify or finalize, so it never reaches `Justify`/`Finalize`; it is + /// still eligible for `TargetAdvance` on its head weight. /// /// The caller resolves `coverage` and passes it in: block building unions a /// data's proof participants (see `pick_best_candidate`); committee-signature @@ -580,14 +611,25 @@ impl ProjectedState { .all(|s| !slot_is_justifiable_after(s, self.finalized_slot)); // An entry that adds no justification voter cannot move the target past - // the threshold, whatever `prior_count` already sits at, so it stays at - // `Build` regardless of `crosses_2_3` — it is here for its head votes. - let tier = if is_genesis_self_vote(att_data) || !crosses_2_3 || new_voters.is_empty() { - Tier::Build - } else if finalizes { + // the threshold, whatever `prior_count` already sits at, so it cannot + // justify regardless of `crosses_2_3` — it is here for its head votes. + let justifies = !is_genesis_self_vote(att_data) && crosses_2_3 && !new_voters.is_empty(); + + // Same rule on the head axis, for the same reason: an entry that moves + // nobody's head vote did not bring the head anywhere, however much + // weight already sits there. Requiring a non-empty contribution is what + // keeps a settled entry from claiming a threshold it did not cross. + let advances_head = !new_head_voters.is_empty() + && self.head_crosses_2_3(att_data, &new_head_voters, validator_count); + + let tier = if justifies && finalizes { Tier::Finalize - } else { + } else if justifies { Tier::Justify + } else if advances_head { + Tier::TargetAdvance + } else { + Tier::Build }; let score = EntryScore { @@ -685,8 +727,18 @@ pub(crate) enum Tier { Finalize = 1, /// Applying the entry crosses 2/3 on target but does not finalize. Justify = 2, - /// Adds marginal new voters toward target's 2/3 supermajority. - Build = 3, + /// Applying the entry brings 2/3 of validators' latest head votes onto the + /// entry's head root, without justifying anything. + /// + /// The LMD-GHOST analogue of `Justify`: it does not move the justification + /// checkpoint, but it settles the head, which is what a later target is + /// eventually chosen against. Ranks below `Justify` because finality beats + /// head weight, and above `Build` because crossing the threshold is worth + /// more than adding marginal weight below it. + TargetAdvance = 3, + /// Adds marginal new voters toward target's 2/3 supermajority, or head + /// weight below the head threshold. + Build = 4, } /// Tiered score for a candidate `AttestationData` entry during block building. @@ -738,25 +790,49 @@ impl EntryScore { /// leads; the remaining four slots carry tier-dependent priorities (see /// the type-level docs), all encoded as `Reverse` so "larger is better". pub(crate) fn ordering_key(&self, data_root: H256) -> OrderingKey { + /// Filler for the `OrderingKey` slots a tier does not rank on. Any + /// constant works: identical across every entry in that tier, it can + /// never decide a comparison. + const ORDERING_UNUSED: Reverse = Reverse(0); + let more_new_voters = Reverse(self.new_voters as u64); let more_new_head_voters = Reverse(self.new_head_voters as u64); let newer_target = Reverse(self.target_slot); let newer_att = Reverse(self.att_slot); match self.tier { - Tier::Build => ( + // Finality first: which checkpoint this moves, then how recent the + // vote is, and only then how much weight it carries. Head votes + // outrank justification voters here because by this point the + // target is already crossing 2/3, so the marginal justification + // voter is worth less than the head weight riding along with it. + Tier::Finalize | Tier::Justify => ( self.tier, - more_new_voters, - more_new_head_voters, newer_target, newer_att, + more_new_head_voters, + more_new_voters, data_root, ), - Tier::Finalize | Tier::Justify => ( + // The head is what this tier settles, and the target is not moving, + // so `newer_target` would be noise. Justification voters are not + // ranked at all: an entry here is chosen for head weight. + // `ORDERING_UNUSED` holds the two slots this tier does not rank on; + // being constant, it never discriminates. + Tier::TargetAdvance => ( self.tier, - newer_target, newer_att, + more_new_head_voters, + ORDERING_UNUSED, + ORDERING_UNUSED, + data_root, + ), + // Below every threshold, so raw progress toward one leads. + Tier::Build => ( + self.tier, more_new_voters, more_new_head_voters, + newer_target, + newer_att, data_root, ), } @@ -1316,12 +1392,16 @@ mod tests { ); assert_eq!(new_head_voters.len(), 3); assert_eq!(score.new_head_voters, 3); - assert_eq!( - score.tier, - Tier::Build, + assert!( + score.tier > Tier::Justify, "an entry adding no justification voter cannot justify, whatever \ the prior count" ); + assert_eq!( + score.tier, + Tier::TargetAdvance, + "3 of 4 validators moved onto this head crosses 2/3" + ); } /// Worthless on both axes: already counted for the target, and every voter @@ -1432,10 +1512,14 @@ mod tests { ); assert_eq!(score.new_voters, 0); assert_eq!(new_head_voters.len(), 4, "its head votes are still new"); + assert!( + score.tier > Tier::Justify, + "it cannot justify, so it must not be tiered as if it could" + ); assert_eq!( score.tier, - Tier::Build, - "it cannot justify, so it must not be tiered as if it could" + Tier::TargetAdvance, + "its head votes still carry the head past 2/3" ); } @@ -1490,6 +1574,191 @@ mod tests { ); } + /// Below the head threshold there is no `TargetAdvance`: the entry is + /// carrying weight, not settling anything. + #[test] + fn head_votes_below_two_thirds_stay_at_build() { + let att_data = make_att_data(5); + // 1 of 10 validators is nowhere near 2/3. + let coverage: HashSet = HashSet::from([0]); + + let projected = ProjectedState { + justified_slots: JustifiedSlots::new(), + finalized_slot: 0, + current_votes: HashMap::from([(att_data.target.root, coverage.clone())]), + head_votes: Some(HashMap::from([(0, make_att_data(4))])), + }; + + let (score, _, new_head_voters) = projected + .score_entry(&att_data, &coverage, 10) + .expect("it still moves a head vote"); + + assert_eq!(new_head_voters.len(), 1); + assert_eq!(score.tier, Tier::Build); + } + + /// An entry that moves nobody's head vote must not claim `TargetAdvance` + /// off weight that was already there. Same rule the justification axis + /// applies, and for the same reason: the threshold has to be crossed BY + /// this entry. + #[test] + fn an_entry_that_moves_no_head_vote_cannot_claim_target_advance() { + // A real target, not `make_att_data`'s genesis self-vote, so the entry + // can actually reach `Justify`. + let att_data = AttestationData { + slot: 5, + head: Checkpoint { + slot: 4, + root: H256([4u8; 32]), + }, + target: Checkpoint { + slot: 3, + root: H256([3u8; 32]), + }, + source: Checkpoint { + slot: 1, + root: H256([1u8; 32]), + }, + }; + let coverage: HashSet = HashSet::from([0, 1, 2]); + + // Everyone already voted for this exact data, so nothing moves, but the + // head is already at a supermajority. + let projected = ProjectedState { + justified_slots: JustifiedSlots::new(), + finalized_slot: 0, + current_votes: HashMap::new(), + head_votes: Some(HashMap::from([ + (0, att_data.clone()), + (1, att_data.clone()), + (2, att_data.clone()), + ])), + }; + + let (score, _, new_head_voters) = projected + .score_entry(&att_data, &coverage, 4) + .expect("it still adds justification voters"); + + assert!(new_head_voters.is_empty(), "no head vote moves"); + assert_eq!( + score.tier, + Tier::Justify, + "it justifies on its own axis, and must not be credited for a head \ + threshold it did not cross" + ); + } + + /// `TargetAdvance` ranks on recency first, then head weight. The target is + /// not moving at this tier, so `newer_target` is deliberately not consulted. + #[test] + fn target_advance_ranks_newer_attestation_over_more_head_votes() { + let newer_but_lighter = EntryScore { + tier: Tier::TargetAdvance, + new_voters: 0, + new_head_voters: 1, + target_slot: 2, + att_slot: 9, + }; + let older_but_heavier = EntryScore { + tier: Tier::TargetAdvance, + new_voters: 0, + new_head_voters: 500, + target_slot: 7, + att_slot: 8, + }; + + assert!( + newer_but_lighter.ordering_key(H256([1u8; 32])) + < older_but_heavier.ordering_key(H256([2u8; 32])), + "a fresher attestation wins even against far more head weight" + ); + } + + /// Within `TargetAdvance`, equal attestation slots fall through to head + /// weight, and justification voters never enter the comparison. + #[test] + fn target_advance_breaks_an_attestation_slot_tie_on_head_votes_only() { + let heavier = EntryScore { + tier: Tier::TargetAdvance, + new_voters: 0, + new_head_voters: 9, + target_slot: 1, + att_slot: 8, + }; + let lighter_but_more_justification_voters = EntryScore { + tier: Tier::TargetAdvance, + new_voters: 900, + new_head_voters: 8, + target_slot: 1, + att_slot: 8, + }; + + assert!( + heavier.ordering_key(H256([1u8; 32])) + < lighter_but_more_justification_voters.ordering_key(H256([2u8; 32])), + "head weight decides; justification voters are not ranked at this tier" + ); + } + + /// In the justify arm head votes now outrank justification voters, once + /// target and attestation slots tie. + #[test] + fn justify_ranks_head_votes_above_justification_voters() { + let more_head_votes = EntryScore { + tier: Tier::Justify, + new_voters: 1, + new_head_voters: 50, + target_slot: 4, + att_slot: 6, + }; + let more_justification_voters = EntryScore { + tier: Tier::Justify, + new_voters: 900, + new_head_voters: 49, + target_slot: 4, + att_slot: 6, + }; + + assert!( + more_head_votes.ordering_key(H256([1u8; 32])) + < more_justification_voters.ordering_key(H256([2u8; 32])), + "past the 2/3 target threshold the marginal justification voter is \ + worth less than head weight" + ); + } + + /// Tier still dominates every other term: a `Justify` entry with nothing + /// else going for it beats the best possible `TargetAdvance` entry, which + /// in turn beats the best possible `Build` entry. + #[test] + fn tier_dominates_every_other_ordering_term() { + let justify = EntryScore { + tier: Tier::Justify, + new_voters: 0, + new_head_voters: 0, + target_slot: 0, + att_slot: 0, + }; + let target_advance = EntryScore { + tier: Tier::TargetAdvance, + new_voters: u32::MAX as usize, + new_head_voters: u32::MAX as usize, + target_slot: u64::MAX, + att_slot: u64::MAX, + }; + let build = EntryScore { + tier: Tier::Build, + new_voters: u32::MAX as usize, + new_head_voters: u32::MAX as usize, + target_slot: u64::MAX, + att_slot: u64::MAX, + }; + let root = H256([1u8; 32]); + + assert!(justify.ordering_key(root) < target_advance.ordering_key(root)); + assert!(target_advance.ordering_key(root) < build.ordering_key(root)); + } + /// Head votes break a tie on justification voters, and never outrank them. #[test] fn head_votes_break_a_tie_on_justification_voters() {