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/9] 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/9] 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/9] 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/9] 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/9] 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() { From d1ef34365c28706bc3519a8d95168a0730323efa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Mon, 21 Sep 2026 13:03:46 -0300 Subject: [PATCH 6/9] fix(blockchain): score head votes against a window of this branch's blocks The head-vote baseline was `ForkChoiceState::on_chain_votes`, a per-validator map fed by `record_known_attestation_votes`. That runs from `insert_signed_block`, which `on_block` calls for EVERY valid block, before `update_head` decides which branch is canonical. So the map was a union over every branch this node ever imported, while every other input to the scorer (`current_votes` from the head state, `justified_slots`, the chain view in `entry_passes_filters`) is branch-relative. Forks broke the axis in both directions through that one asymmetry. Under-credit: a sibling block that packed a vote naming a COMMON ANCESTOR as its head marked that vote carried, for every branch. The branch we kept, which never carried it, then scored it zero on the head axis; with an already justified target zeroing the other axis, `score_entry` returned `None` and the entry was dropped. That is the empty-block failure this branch exists to fix, re-armed by any fork. Over-credit, which the old field's doc argued could not happen: it reasoned the fork-blindness "only ever makes a vote look less novel than it is". True of `new_head_voters`, since the map is a per-validator max and `supersedes(map)` implies genuinely new. Not true of the `retained` term `head_crosses_2_3` added later, which counts validators whose latest vote names this head root. Fed off-branch, it let an entry moving ONE head claim `TargetAdvance`, a tier that outranks every `Build` entry, over entries bringing 19 justification voters. With `--max-attestations-per-block` defaulting to 3, that ordering decides the whole body. `Store::extract_head_vote_window(head_root, blocks)` replaces the running map. It walks parent links from the head being extended and returns a `HeadVoteWindow`. Both halves come from that one walk, since a root in `roots` whose votes are missing from `votes` would read as a contested head nobody has voted on: - `votes` is the old question asked branch-relatively: does this branch already carry the vote. - `roots` is new: is this entry's head still in play. An entry naming a head behind the window scores zero on the axis whatever `votes` says. That is the stretch of chain a proposer can still influence; a head with a window's worth of blocks built past it is not being contested. It also closes a cycle the bounded `votes` would otherwise open: once the block that carried a vote falls out of the window the vote reads as new again, so without the gate a settled-target entry would be packable once per window, indefinitely. Three blocks covers the case the axis exists for. On a chain whose `justified - finalized` sits at 6 the justifiable rungs are 3 slots apart, so several consecutive slots vote for the same settled target and have nothing but their head votes to offer, and those heads are the last few blocks. The walk follows headers and reads the body only for votes, so a block whose body the store does not hold still bounds the window. That is the checkpoint-sync anchor, since `from_anchor_state` writes no body row: stopping there would leave `roots` empty, which under the gate switches the axis off entirely rather than merely leaving the baseline empty. It resolves on the first import. Tests pin the properties rather than the implementation. At the storage layer `head_vote_window_ignores_a_block_on_an_abandoned_branch` inserts a sibling carrying a LATER vote from the same validator and requires neither its vote nor its root to reach the window, which is the regression itself written down, and `head_vote_window_covers_the_head_and_its_recent_ancestors` pins both halves at the window edge. At the scorer, `score_entry_ignores_a_head_that_has_aged_out_of_the_window` and `an_aged_out_head_cannot_claim_target_advance` score one entry twice with only `roots` differing, so removing the gate fails them without touching the votes path. Every scoring test now states which heads are in play, which is the new semantic made explicit. Two aggregation fixtures voted for a head outside their own window and failed until the fixture put that block on the head's branch. `ProjectedState::head_votes` becomes `head_window`, and `with_head_votes` becomes `with_head_window`. --- crates/blockchain/src/aggregation.rs | 152 ++++++---- crates/blockchain/src/block_builder.rs | 340 ++++++++++++++++------ crates/blockchain/src/store.rs | 17 +- crates/storage/src/lib.rs | 3 +- crates/storage/src/store.rs | 371 ++++++++++++++++++------- 5 files changed, 644 insertions(+), 239 deletions(-) diff --git a/crates/blockchain/src/aggregation.rs b/crates/blockchain/src/aggregation.rs index 990d870d..8b27c3db 100644 --- a/crates/blockchain/src/aggregation.rs +++ b/crates/blockchain/src/aggregation.rs @@ -401,9 +401,10 @@ pub fn snapshot_aggregation_inputs( // existing blocks (head.slot / target.slot <= head_slot), so no // empty-slot padding beyond the tip is needed. let known_block_roots = store.get_block_roots().expect("block roots read works"); + let head_root = store.head().expect("head read works"); let mut extended_historical_block_hashes: Vec = head_state.historical_block_hashes.iter().copied().collect(); - extended_historical_block_hashes.push(store.head().expect("head read works")); + extended_historical_block_hashes.push(head_root); // 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 @@ -412,13 +413,18 @@ pub fn snapshot_aggregation_inputs( // 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. // - // 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. + // The baseline is what the head's own branch 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. Reading from `head_root` rather than from a running + // import-fed map is what keeps a branch this node abandoned from + // suppressing work on the one it kept. let mut projected = block_builder::ProjectedState::from_head_state(&head_state) - .with_head_votes(store.extract_on_chain_votes()); + .with_head_window( + store.extract_head_vote_window(head_root, block_builder::HEAD_VOTE_WINDOW_BLOCKS), + ); let mut jobs: Vec = Vec::with_capacity(max_jobs.min(groups_considered)); for _round in 0..max_jobs { @@ -541,7 +547,7 @@ fn pick_best_candidate( } // Head votes ARE scored here: the projection above is seeded from - // `extract_on_chain_votes`. So this skip now means "adds nothing on + // `extract_head_vote_window`. 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)) = @@ -1820,6 +1826,15 @@ mod tests { } } + /// The branch baseline `snapshot_aggregation_inputs` scores against, read + /// the same way the call site reads it. + fn head_window(store: &Store) -> ethlambda_storage::HeadVoteWindow { + store.extract_head_vote_window( + store.head().expect("head root"), + block_builder::HEAD_VOTE_WINDOW_BLOCKS, + ) + } + fn new_test_store(head_state: State) -> Store { let backend: Arc = Arc::new(InMemoryBackend::new()); Store::from_anchor_state(backend, head_state, DEFAULT_MILLISECONDS_PER_SLOT) @@ -2037,7 +2052,7 @@ mod tests { justified_slots: JustifiedSlots::new(), finalized_slot: 0, current_votes: HashMap::new(), - head_votes: None, + head_window: None, }; let (picked_root, score, _head_voters) = pick_best_candidate( @@ -2130,7 +2145,7 @@ mod tests { justified_slots: JustifiedSlots::new(), finalized_slot: 0, current_votes: HashMap::new(), - head_votes: None, + head_window: None, }; // Round 1: A (6 new voters) outranks B (2 new voters); both Build tier. @@ -2277,19 +2292,20 @@ 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` + /// the head's own branch carries (`extract_head_vote_window`), 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. + /// `known_votes` while leaving the branch baseline 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; @@ -2313,6 +2329,10 @@ mod tests { FINALIZED_SLOT, TARGET_SLOT, ); + // The votes below name `hashes[0]` as their head, so that block has to + // sit inside the head-vote window: an entry whose head has aged out of + // it scores zero on this axis by design. + head_state.latest_block_header.parent_root = hashes[0]; let mut store = new_test_store(head_state); insert_test_block(&mut store, hashes[0], 0, H256::ZERO); @@ -2336,7 +2356,7 @@ mod tests { 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. + // lands there, while leaving the branch baseline untouched. let mut bits = AggregationBits::with_length(NUM_VALIDATORS).unwrap(); bits.set(0, true).unwrap(); bits.set(1, true).unwrap(); @@ -2347,8 +2367,9 @@ mod tests { "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" + head_window(&store).votes.is_empty(), + "fixture must leave the branch baseline empty: no block on this \ + branch carried the vote" ); assert!( @@ -2360,13 +2381,18 @@ 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. + /// block ON THIS BRANCH 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 + /// an empty 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. + /// + /// The carrier is the head block's parent rather than a loose sibling, + /// which is the whole point of a branch-relative baseline: a block the head + /// does not descend from must NOT suppress anything (covered at the + /// accessor by `head_window_votes_ignore_a_block_on_an_abandoned_branch`). #[test] fn select_skips_a_group_whose_vote_the_chain_already_carries() { const NUM_VALIDATORS: usize = 10; @@ -2375,23 +2401,6 @@ mod tests { 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, @@ -2408,15 +2417,12 @@ mod tests { 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. + // The block that puts 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 { + let carrier = SignedBlock { message: Block { slot: 1, proposer_index: 0, @@ -2425,7 +2431,7 @@ mod tests { body: BlockBody { attestations: vec![ethlambda_types::attestation::AggregatedAttestation { aggregation_bits: bits, - data: att_data, + data: att_data.clone(), }] .try_into() .unwrap(), @@ -2433,23 +2439,55 @@ mod tests { }, proof: MultiMessageAggregate::default(), }; - let block_root = { + let carrier_root = { use ethlambda_types::primitives::HashTreeRoot as _; - block.message.hash_tree_root() + carrier.message.hash_tree_root() }; + + 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, + ); + // The head descends from the carrier, and reads back as an empty-bodied + // block so the window walk can step through it to reach the carrier. + head_state.latest_block_header.parent_root = carrier_root; + head_state.latest_block_header.body_root = { + use ethlambda_types::primitives::HashTreeRoot as _; + BlockBody::default().hash_tree_root() + }; + + let mut store = new_test_store(head_state); + insert_test_block(&mut store, hashes[0], 0, H256::ZERO); store - .insert_signed_block(block_root, block) + .insert_signed_block(carrier_root, carrier) .expect("insert block carrying the vote"); + + let hashed = HashedAttestationData::new(att_data); + store.insert_gossip_signature(hashed.clone(), 0, dummy_sig()); + store.insert_gossip_signature(hashed, 1, dummy_sig()); + assert_eq!( - store.extract_on_chain_votes().len(), + head_window(&store).votes.len(), 2, - "fixture must actually put the vote on chain" + "fixture must actually put the vote on the head's branch" ); 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" + "this branch already carries the vote, so it adds nothing on either \ + axis" ); } @@ -2485,6 +2523,10 @@ mod tests { FINALIZED_SLOT, TARGET_SLOT, ); + // The votes below name `hashes[0]` as their head, so that block has to + // sit inside the head-vote window: an entry whose head has aged out of + // it scores zero on this axis by design. + head_state.latest_block_header.parent_root = hashes[0]; let mut store = new_test_store(head_state); insert_test_block(&mut store, hashes[0], 0, H256::ZERO); diff --git a/crates/blockchain/src/block_builder.rs b/crates/blockchain/src/block_builder.rs index 8eb84dbc..c9a365f4 100644 --- a/crates/blockchain/src/block_builder.rs +++ b/crates/blockchain/src/block_builder.rs @@ -23,6 +23,7 @@ use ethlambda_state_transition::{ attestation_data_matches_chain, justified_slots_ops, process_block, process_slots, slot_is_justifiable_after, }; +use ethlambda_storage::HeadVoteWindow; use ethlambda_types::{ ShortRoot, attestation::{AggregatedAttestation, AggregationBits, AttestationData}, @@ -43,6 +44,26 @@ use crate::{ store::StoreError, }; +/// How far back the head-vote baseline is read: the head block and the two +/// blocks it descends from. +/// +/// This is the stretch of chain a proposer can still influence, so it bounds +/// the head axis at both ends. An entry naming a head inside it is scored for +/// the weight it would add ([`ethlambda_storage::HeadVoteWindow::roots`]); +/// an entry naming a head behind it scores zero, whatever its votes look like +/// against what the window carries. +/// +/// Three blocks is chosen to cover the case the axis exists for. On a chain +/// whose `justified - finalized` sits at 6 the justifiable rungs are 3 slots +/// apart, so several consecutive slots all vote for the same settled target and +/// have nothing but their head votes to offer. Their heads are the last few +/// blocks, so the window keeps them packable; anything older has had a +/// window's worth of blocks built past it and is no longer being contested. +/// +/// It also bounds the walk to three block reads per selection, on the proposal +/// hot path. +pub(crate) const HEAD_VOTE_WINDOW_BLOCKS: usize = 3; + /// Post-block checkpoints extracted from the state transition in `build_block`. /// /// When building a block, the state transition processes attestations that may @@ -187,7 +208,7 @@ fn select_attestations( let ProposalInputs { known_block_roots, aggregated_payloads, - latest_head_votes, + head_window, } = inputs; let mut selected: Vec<(AggregatedAttestation, SingleMessageAggregate)> = Vec::new(); @@ -216,8 +237,7 @@ 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).with_head_votes(latest_head_votes); + let mut projected = ProjectedState::from_head_state(head_state).with_head_window(head_window); let mut processed_data_roots: HashSet = HashSet::new(); // A block may carry at most `MAX_ATTESTATIONS_DATA` distinct entries @@ -323,15 +343,19 @@ pub(crate) struct ProposalInputs<'a> { /// The attestation pool: `data_root -> (data, proofs)`. pub(crate) aggregated_payloads: &'a HashMap)>, - /// Per-validator latest head votes that the CHAIN already carries. + /// The last [`HEAD_VOTE_WINDOW_BLOCKS`] blocks of the branch this block + /// extends, and the votes they carry. /// /// 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`. + /// score zero new head voters. And deliberately branch-relative: a + /// per-validator map fed by every block import cannot tell a vote our + /// branch carries from one only an abandoned sibling carried. See + /// [`ethlambda_storage::HeadVoteWindow`]. /// - /// 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, + /// Owned because `Store::extract_head_vote_window` already returns a fresh + /// value, and the projection mutates it as entries are selected. + pub(crate) head_window: HeadVoteWindow, } /// Static inputs to the attestation selection scan: the candidate pool and @@ -358,29 +382,25 @@ 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 that the CHAIN already carries, - /// advanced as entries are selected so a validator is not credited twice - /// across rounds. + /// The window of recent blocks on the branch being extended, with its + /// `votes` 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. + /// `None` turns head-vote scoring off entirely. It is not the same as a + /// window with an empty `votes`, which says "these blocks are in play and + /// carried nothing", and under which every validator in an entry's + /// coverage reads as newly covered. /// - /// 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. + /// A window with empty `roots` is the other real state, and it also scores + /// every entry at zero on this axis: nothing is in play, so nothing can be + /// advanced. That is what a node resumed from an anchor whose header the + /// store does not hold sees, and it resolves on the first import. /// /// 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>, + /// empty window and rely on it meaning the same thing. + pub(crate) head_window: Option, } impl ProjectedState { @@ -392,19 +412,19 @@ 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, + head_window: 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. + /// Seed the head-vote window, 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_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 { - self.head_votes = Some(head_votes); + /// Takes the window by value: `Store::extract_head_vote_window` already + /// hands out an owned value, so there is nothing to gain by borrowing it + /// and the projection then owns what it mutates. + pub(crate) fn with_head_window(mut self, head_window: HeadVoteWindow) -> Self { + self.head_window = Some(head_window); self } @@ -462,11 +482,11 @@ impl ProjectedState { att_data: &AttestationData, new_head_voters: impl IntoIterator, ) { - let Some(head_votes) = self.head_votes.as_mut() else { + let Some(head_window) = self.head_window.as_mut() else { return; }; for validator_id in new_head_voters { - head_votes.insert(validator_id, att_data.clone()); + head_window.votes.insert(validator_id, att_data.clone()); } } @@ -488,13 +508,6 @@ 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. /// Whether applying this entry puts 2/3 of the validator set's latest head /// votes on `att_data.head.root`. /// @@ -503,6 +516,15 @@ impl ProjectedState { /// counts when the entry moves it onto this head, or when it already names /// this head and the entry does not move it elsewhere. /// + /// Both halves of that count come from the same branch-relative window, so + /// the post-state this measures is the one the block being built would + /// actually produce. A per-validator map fed by every block import would + /// break the second half in the direction that matters: it would retain + /// validators whose vote only a sibling branch carried, letting an entry + /// that moves one head claim a threshold this branch is nowhere near and + /// outrank, at [`Tier::TargetAdvance`], entries bringing real justification + /// voters. + /// /// `None` head votes means the axis is switched off, so no supermajority /// can be claimed. fn head_crosses_2_3( @@ -511,12 +533,13 @@ impl ProjectedState { new_head_voters: &HashSet, validator_count: usize, ) -> bool { - let Some(head_votes) = self.head_votes.as_ref() else { + let Some(head_window) = self.head_window.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 + let retained = head_window + .votes .iter() .filter(|(vid, vote)| vote.head.root == head_root && !new_head_voters.contains(vid)) .count(); @@ -524,15 +547,34 @@ impl ProjectedState { 3 * total >= 2 * validator_count } + /// The subset of `coverage` whose latest head vote this entry would + /// replace, per the LMD-GHOST latest-message rule + /// ([`AttestationData::supersedes`]). + /// + /// Empty unless this entry names a head still inside the window. A head + /// older than that is not in play: the block it names already sits under a + /// window's worth of descendants on this branch, so weight added there + /// moves no decision a proposer can influence, and crediting it would let + /// a vote be packed again every time the block that carried it aged out of + /// `HeadVoteWindow::votes`. + /// + /// Within the window, measured against the votes those blocks carry, so a + /// validator with no entry counts as new: no block in the window has + /// carried a vote for it, so this entry is the first weight it would + /// contribute here. fn new_head_voters(&self, att_data: &AttestationData, coverage: &HashSet) -> HashSet { - let Some(head_votes) = self.head_votes.as_ref() else { + let Some(head_window) = self.head_window.as_ref() else { return HashSet::new(); }; + if !head_window.roots.contains(&att_data.head.root) { + return HashSet::new(); + } coverage .iter() .copied() .filter(|vid| { - head_votes + head_window + .votes .get(vid) .is_none_or(|existing| att_data.supersedes(existing)) }) @@ -1266,6 +1308,21 @@ mod tests { } } + /// A head-vote window over `roots` carrying `votes`. + /// + /// Scoring credits an entry only when its head is one of `roots`, so a + /// test that wants the votes to decide has to put the entry's head in + /// play. Tests of the in-play gate itself leave it out on purpose. + fn window(roots: &[H256], votes: &[(u64, AttestationData)]) -> HeadVoteWindow { + HeadVoteWindow { + roots: roots.iter().copied().collect(), + votes: votes.iter().cloned().collect(), + } + } + + /// The head root every `make_att_data` entry votes for. + const DEFAULT_HEAD: H256 = H256::ZERO; + fn make_bits(indices: &[usize]) -> AggregationBits { let max = indices.iter().copied().max().unwrap_or(0); let mut bits = AggregationBits::with_length(max + 1).unwrap(); @@ -1308,7 +1365,7 @@ mod tests { justified_slots: JustifiedSlots::new(), finalized_slot: FINALIZED_SLOT, current_votes: HashMap::new(), - head_votes: None, + head_window: None, }; let (score, _, _) = projected @@ -1325,18 +1382,19 @@ mod tests { /// An entry scored against a projection whose head votes were never seeded /// must report zero new head voters. /// - /// `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. + /// `None` must mean "do not score head votes", not "a window carrying no + /// votes": in a window with no recorded vote every validator in coverage + /// reads as newly covered, so such a window 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() { + fn head_vote_scoring_is_off_when_the_window_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, + head_window: None, }; let coverage: HashSet = HashSet::from([0, 1, 2]); @@ -1375,11 +1433,14 @@ mod tests { 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)), - ])), + head_window: Some(window( + &[att_data.head.root], + &[ + (0, make_att_data(4)), + (1, make_att_data(4)), + (2, make_att_data(4)), + ], + )), }; let (score, new_voters, new_head_voters) = projected @@ -1404,6 +1465,117 @@ mod tests { ); } + /// A head that has aged out of the window is not in play: this branch has + /// built a window's worth of blocks past it, so weight added there moves no + /// decision the proposer can influence. + /// + /// This is also what closes the re-pack cycle. `HeadVoteWindow::votes` + /// reaches back only as far as `roots` does, so once the block that carried + /// a vote falls out of the window the vote reads as new again; without the + /// `roots` gate a settled-target entry would be packable once per window, + /// indefinitely, for weight nobody is contesting. + #[test] + fn score_entry_ignores_a_head_that_has_aged_out_of_the_window() { + 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]); + // Target fully covered, so the entry lives or dies on its head votes. + let current_votes = HashMap::from([(att_data.target.root, coverage.clone())]); + + let in_play = ProjectedState { + justified_slots: JustifiedSlots::new(), + finalized_slot: 0, + current_votes: current_votes.clone(), + head_window: Some(window(&[att_data.head.root], &[])), + }; + let (_, _, new_head_voters) = in_play + .score_entry(&att_data, &coverage, 4) + .expect("a head still in play carries value"); + assert_eq!(new_head_voters.len(), 3); + + // The same entry against the same (empty) votes. Only `roots` moved on. + let aged_out = ProjectedState { + justified_slots: JustifiedSlots::new(), + finalized_slot: 0, + current_votes, + head_window: Some(window(&[H256([9u8; 32]), H256([10u8; 32])], &[])), + }; + assert!( + aged_out.new_head_voters(&att_data, &coverage).is_empty(), + "a head behind the window scores nothing, however novel its votes \ + look against what the window carries" + ); + assert!( + aged_out.score_entry(&att_data, &coverage, 4).is_none(), + "and with its target settled too, the entry is worth nothing at all" + ); + } + + /// Nor can an aged-out head reach `TargetAdvance`, however much weight the + /// window already shows sitting on it. `advances_head` requires the entry + /// to move somebody, and behind the window it moves nobody. + /// + /// The tier matters more than the count: `TargetAdvance` outranks every + /// `Build` entry, so a stale head claiming it would displace entries + /// bringing real justification voters from a block that carries three. + #[test] + fn an_aged_out_head_cannot_claim_target_advance() { + const NUM_VALIDATORS: usize = 10; + + 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]), + }, + }; + // 8 of 10 validators already name this head, which would be a + // supermajority if the head were still in play. + let settled: Vec<(u64, AttestationData)> = + (0..8).map(|vid| (vid, att_data.clone())).collect(); + let projected = ProjectedState { + justified_slots: JustifiedSlots::new(), + finalized_slot: 0, + current_votes: HashMap::new(), + head_window: Some(window(&[H256([9u8; 32])], &settled)), + }; + let coverage: HashSet = HashSet::from([8]); + + let (score, new_voters, new_head_voters) = projected + .score_entry(&att_data, &coverage, NUM_VALIDATORS) + .expect("it still brings a justification voter"); + + assert_eq!(new_voters.len(), 1); + assert!(new_head_voters.is_empty()); + assert_eq!(score.new_head_voters, 0); + assert_eq!( + score.tier, + Tier::Build, + "a head nobody is contesting cannot be advanced" + ); + } + /// Worthless on both axes: already counted for the target, and every voter /// already holds a newer head vote. #[test] @@ -1413,18 +1585,23 @@ mod tests { 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)), - ])), + // The head IS in play, so the entry is dropped for its votes + // rather than for naming a head nobody is contesting. + head_window: Some(window( + &[DEFAULT_HEAD], + &[ + (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" + "a vote older than what this branch already carries adds nothing" ); } @@ -1436,7 +1613,7 @@ mod tests { justified_slots: JustifiedSlots::new(), finalized_slot: 0, current_votes: HashMap::new(), - head_votes: Some(HashMap::new()), + head_window: Some(window(&[DEFAULT_HEAD], &[])), }; let coverage: HashSet = HashSet::from([0, 1]); let first = make_att_data(5); @@ -1499,7 +1676,7 @@ mod tests { // target's tally. Without the settled-target guard the whole // coverage would read as new. current_votes: HashMap::new(), - head_votes: Some(HashMap::new()), + head_window: Some(window(&[att_data.head.root], &[])), }; let (score, new_voters, new_head_voters) = projected @@ -1559,7 +1736,7 @@ mod tests { justified_slots, finalized_slot: FINALIZED_SLOT, current_votes: HashMap::new(), - head_votes: None, + head_window: None, }; let known: HashSet = roots.iter().copied().collect(); @@ -1586,7 +1763,7 @@ mod tests { 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))])), + head_window: Some(window(&[att_data.head.root], &[(0, make_att_data(4))])), }; let (score, _, new_head_voters) = projected @@ -1628,11 +1805,14 @@ mod tests { 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()), - ])), + head_window: Some(window( + &[att_data.head.root], + &[ + (0, att_data.clone()), + (1, att_data.clone()), + (2, att_data.clone()), + ], + )), }; let (score, _, new_head_voters) = projected @@ -1929,7 +2109,7 @@ mod tests { ProposalInputs { known_block_roots: &known_block_roots, aggregated_payloads: &aggregated_payloads, - latest_head_votes: HashMap::new(), + head_window: HeadVoteWindow::default(), }, ProposerConfig { enable_proposer_aggregation: true, @@ -2078,7 +2258,7 @@ mod tests { ProposalInputs { known_block_roots: &known_block_roots, aggregated_payloads: &aggregated_payloads, - latest_head_votes: HashMap::new(), + head_window: HeadVoteWindow::default(), }, ProposerConfig { enable_proposer_aggregation: false, @@ -2207,7 +2387,7 @@ mod tests { ProposalInputs { known_block_roots: &known_block_roots, aggregated_payloads: &aggregated_payloads, - latest_head_votes: HashMap::new(), + head_window: HeadVoteWindow::default(), }, ProposerConfig { enable_proposer_aggregation: false, @@ -2516,7 +2696,7 @@ mod tests { ProposalInputs { known_block_roots: &known_block_roots, aggregated_payloads: &aggregated_payloads, - latest_head_votes: HashMap::new(), + head_window: HeadVoteWindow::default(), }, ProposerConfig { enable_proposer_aggregation: true, @@ -2655,7 +2835,7 @@ mod tests { ProposalInputs { known_block_roots: &known_block_roots, aggregated_payloads: &aggregated_payloads, - latest_head_votes: HashMap::new(), + head_window: HeadVoteWindow::default(), }, ProposerConfig { enable_proposer_aggregation: true, diff --git a/crates/blockchain/src/store.rs b/crates/blockchain/src/store.rs index 049a5d43..8eeab381 100644 --- a/crates/blockchain/src/store.rs +++ b/crates/blockchain/src/store.rs @@ -18,7 +18,9 @@ use tracing::{info, trace, warn}; use crate::{ GOSSIP_DISPARITY_INTERVALS, INTERVALS_PER_SLOT, MAX_ATTESTATIONS_DATA, SlotInterval, - block_builder::{PostBlockCheckpoints, ProposalInputs, ProposerConfig, build_block}, + block_builder::{ + HEAD_VOTE_WINDOW_BLOCKS, PostBlockCheckpoints, ProposalInputs, ProposerConfig, build_block, + }, metrics, }; @@ -968,19 +970,22 @@ pub fn produce_block_with_signatures( let known_block_roots = store.get_block_roots().unwrap(); - // 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. + // The recent blocks of the branch we are extending and the votes they + // carry, 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(); + // And read from `head_root` rather than kept as a running map, because a + // map fed by every block import cannot tell a vote this branch carries from + // one a sibling we abandoned carried. + let head_window = store.extract_head_vote_window(head_root, HEAD_VOTE_WINDOW_BLOCKS); let inputs = ProposalInputs { known_block_roots: &known_block_roots, aggregated_payloads: &aggregated_payloads, - latest_head_votes, + head_window, }; let (block, signatures, post_checkpoints) = { diff --git a/crates/storage/src/lib.rs b/crates/storage/src/lib.rs index 95da4df6..ec5dcfbf 100644 --- a/crates/storage/src/lib.rs +++ b/crates/storage/src/lib.rs @@ -9,5 +9,6 @@ pub use api::{ALL_TABLES, StorageBackend, StorageReadView, StorageWriteBatch, Ta /// callers can match on it (e.g. to distinguish [`Error::GenesisMismatch`]). pub use error::Error; pub use store::{ - ForkCheckpoints, GetForkchoiceStoreError, MAX_RESUMABLE_DB_STATE_AGE, NEW_PAYLOAD_CAP, Store, + ForkCheckpoints, GetForkchoiceStoreError, HeadVoteWindow, MAX_RESUMABLE_DB_STATE_AGE, + NEW_PAYLOAD_CAP, Store, }; diff --git a/crates/storage/src/store.rs b/crates/storage/src/store.rs index c3f61958..02574d5e 100644 --- a/crates/storage/src/store.rs +++ b/crates/storage/src/store.rs @@ -329,36 +329,38 @@ type StorageKey = Vec; type StorageEntry = (StorageKey, Vec); type BlockRootIndexChanges = (Vec, Vec); +/// The head-vote baseline a proposer scores against: the last +/// `HEAD_VOTE_WINDOW_BLOCKS` blocks of the branch it is extending. +/// +/// The two halves answer different questions and are both needed. +/// +/// `roots` answers "is this entry's head still in play". A vote naming a head +/// older than the window moves nothing a proposer can influence: the block it +/// names is already buried under the window's worth of descendants, and fork +/// choice settled that stretch of chain before the window opened. Crediting it +/// would also let a vote be packed again once the block that carried it ages +/// out of `votes`, paying a block entry per cycle for weight nobody is +/// contesting. +/// +/// `votes` answers "does this branch already carry the vote", so an entry is +/// valued for the weight it would ADD rather than for its whole coverage. +/// +/// They come from one walk because they must agree: a root in `roots` whose +/// votes are missing from `votes` reads as a head still in play that nobody +/// has voted on, which is exactly backwards. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct HeadVoteWindow { + /// Roots of the blocks in the window, the head itself included. + pub roots: HashSet, + /// Latest vote per validator those blocks carry, by the same + /// latest-message rule fork choice uses. + pub votes: HashMap, +} + #[derive(Clone, Default)] 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. @@ -1553,12 +1555,10 @@ 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. + /// Feeds `known_votes`, which is what fork choice weighs: every vote this + /// node knows, however it arrived. Block production does NOT score against + /// this map; it reads the branch it is building on through + /// [`Self::extract_head_window_votes`]. fn record_known_attestation_votes(&self, attestations: &[AggregatedAttestation]) { let mut fork_choice = self.fork_choice.lock().unwrap(); for attestation in attestations { @@ -1568,11 +1568,6 @@ impl Store { validator_id, &attestation.data, ); - Self::record_vote( - &mut fork_choice.on_chain_votes, - validator_id, - &attestation.data, - ); } } } @@ -1582,16 +1577,53 @@ impl Store { self.fork_choice.lock().unwrap().known_votes.clone() } - /// Extract the latest vote per validator that a block has already carried. + /// The last `blocks` blocks of the branch ending at `head_root`: which + /// blocks they are, and the latest vote per validator they carry. /// /// 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() + /// would ADD to the branch it is built on. See [`HeadVoteWindow`] for what + /// each half answers and why the two must come from one walk. + /// + /// Deliberately not [`Self::extract_latest_known_attestations`]. That map + /// advances in lockstep with the aggregated-payload pool bodies are built + /// from: [`Self::insert_new_aggregated_payload`] writes `new_votes` and + /// `new_payloads` in one call, and [`Self::promote_new_aggregated_payloads`] + /// drains both into their `known` counterparts in one call. A body built + /// out of that pool can therefore never carry a vote newer than + /// `known_votes`, so scoring against it reports zero for every candidate + /// and the head-vote axis is dead. + /// + /// Read from the block store on demand rather than kept as a running map + /// because the answer is branch-relative. `insert_signed_block` runs for + /// every valid block, including ones on branches this node never adopts, + /// so a running map would report a vote as carried by the branch we are + /// building on when only an abandoned sibling carried it. The proposer + /// would then score the vote at zero on both axes and drop it, leaving the + /// branch it kept with nothing to pack. + /// + /// The walk follows headers, so a block whose body the store does not hold + /// still bounds the window; only the votes it carried are unreadable. That + /// is the checkpoint-sync anchor, and it resolves on the first import. The + /// walk stops at a root with no header at all, which is the normal + /// terminator: the anchor's parent names no block. + pub fn extract_head_vote_window(&self, head_root: H256, blocks: usize) -> HeadVoteWindow { + let mut window = HeadVoteWindow::default(); + let mut root = head_root; + for _ in 0..blocks { + let Ok(Some(header)) = self.get_block_header(&root) else { + break; + }; + window.roots.insert(root); + if let Ok(Some(block)) = self.get_block(&root) { + for attestation in block.body.attestations.iter() { + for validator_id in validator_indices(&attestation.aggregation_bits) { + Self::record_vote(&mut window.votes, validator_id, &attestation.data); + } + } + } + root = header.parent_root; + } + window } /// Extract per-validator latest attestations from new (pending) payloads. @@ -2166,17 +2198,189 @@ mod tests { assert_eq!(votes[&3], data); } + /// Build a chain of `count` blocks on top of the anchor, block `i` + /// carrying one attestation from validator `i` for target slot `i + 1`. + /// Returns the roots and datas, oldest first. + fn chain_of_attesting_blocks( + store: &mut Store, + count: u64, + ) -> (Vec, Vec) { + let mut parent = store.head().expect("head root"); + let mut roots = Vec::new(); + let mut datas = Vec::new(); + for i in 0..count { + let slot = i + 1; + let data = make_att_data_for_target(slot, root(slot)); + let block = signed_block_with_attestations( + slot, + parent, + vec![AggregatedAttestation { + aggregation_bits: make_proof_for_validator(i as usize).participants, + data: data.clone(), + }], + ); + let block_root = block.message.hash_tree_root(); + store + .insert_signed_block(block_root, block) + .expect("insert block"); + parent = block_root; + roots.push(block_root); + datas.push(data); + } + (roots, datas) + } + + fn anchored_store() -> Store { + let backend = Arc::new(InMemoryBackend::new()); + Store::from_anchor_state( + backend, + State::from_genesis(0, vec![]), + DEFAULT_MILLISECONDS_PER_SLOT, + ) + } + + /// The window is exactly the head block plus the `blocks - 1` blocks it + /// descends from; anything older is outside it and reads as not carried. + #[test] + fn head_vote_window_covers_the_head_and_its_recent_ancestors() { + let mut store = anchored_store(); + let (roots, datas) = chain_of_attesting_blocks(&mut store, 4); + let head = *roots.last().expect("four blocks"); + + let window = store.extract_head_vote_window(head, 3); + + assert_eq!( + window.roots, + HashSet::from([roots[3], roots[2], roots[1]]), + "the head and the two blocks it descends from are in play" + ); + assert!( + !window.roots.contains(&roots[0]), + "the fourth block back is behind the window, so an entry naming it \ + as head scores nothing" + ); + + assert_eq!(window.votes.len(), 3, "three blocks, one validator each"); + assert_eq!(window.votes[&3], datas[3], "the head block"); + assert_eq!(window.votes[&2], datas[2]); + assert_eq!(window.votes[&1], datas[1]); + assert!( + !window.votes.contains_key(&0), + "a vote carried only behind the window is not part of the baseline" + ); + } + + /// The regression this window exists for. `insert_signed_block` runs for + /// every valid block, so a sibling this node never adopts would enter a + /// running on-chain map and suppress packing on the branch it kept. The + /// window is anchored at a head, so it cannot see the sibling at all. + #[test] + fn head_vote_window_ignores_a_block_on_an_abandoned_branch() { + let mut store = anchored_store(); + let anchor = store.head().expect("head root"); + + // The branch we keep: validator 0's slot-1 vote. + let kept_data = make_att_data_for_target(1, root(1)); + let kept = signed_block_with_attestations( + 1, + anchor, + vec![AggregatedAttestation { + aggregation_bits: make_proof_for_validator(0).participants, + data: kept_data.clone(), + }], + ); + let kept_root = kept.message.hash_tree_root(); + store + .insert_signed_block(kept_root, kept) + .expect("insert kept block"); + + // A sibling off the same anchor, carrying a LATER vote from the same + // validator. A running per-validator map would let this win. + let orphan_data = make_att_data_for_target(2, root(2)); + let orphan = signed_block_with_attestations( + 2, + anchor, + vec![AggregatedAttestation { + aggregation_bits: make_proof_for_validator(0).participants, + data: orphan_data.clone(), + }], + ); + let orphan_root = orphan.message.hash_tree_root(); + store + .insert_signed_block(orphan_root, orphan) + .expect("insert orphan block"); + + let window = store.extract_head_vote_window(kept_root, 3); + + assert_eq!( + window.votes[&0], kept_data, + "the baseline must name what the branch we build on carries" + ); + assert!( + !window.votes.values().any(|vote| *vote == orphan_data), + "a branch we abandoned must not suppress packing on the branch we \ + kept" + ); + assert!( + !window.roots.contains(&orphan_root), + "nor may it put its own block in play as a head worth voting for" + ); + } + + /// Within the window the latest-message rule still decides, so a validator + /// that voted in two of the last three blocks reads as carrying the newer + /// vote whichever order the walk visits them in. + #[test] + fn head_vote_window_keeps_the_latest_vote_per_validator() { + let mut store = anchored_store(); + let anchor = store.head().expect("head root"); + + let older = make_att_data_for_target(1, root(1)); + let first = signed_block_with_attestations( + 1, + anchor, + vec![AggregatedAttestation { + aggregation_bits: make_proof_for_validator(0).participants, + data: older.clone(), + }], + ); + let first_root = first.message.hash_tree_root(); + store + .insert_signed_block(first_root, first) + .expect("insert first block"); + + let newer = make_att_data_for_target(2, root(2)); + let second = signed_block_with_attestations( + 2, + first_root, + vec![AggregatedAttestation { + aggregation_bits: make_proof_for_validator(0).participants, + data: newer.clone(), + }], + ); + let second_root = second.message.hash_tree_root(); + store + .insert_signed_block(second_root, second) + .expect("insert second block"); + + assert_eq!( + store.extract_head_vote_window(second_root, 3).votes[&0], + newer + ); + } + /// 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. + /// The window baseline 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(); + fn head_vote_window_does_not_move_with_the_aggregated_payload_pool() { + let mut store = anchored_store(); + let head = store.head().expect("head root"); let data = make_att_data_for_target(8, root(8)); store.insert_new_aggregated_payload( @@ -2191,58 +2395,30 @@ mod tests { "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" + store.extract_head_vote_window(head, 3).votes.is_empty(), + "no block on this branch has carried this vote" ); } - /// The other half: a block import is what makes a vote on-chain, and it - /// must move BOTH maps. + /// A pooled vote strictly newer than what the window carries must read as + /// new against it. This is the property the whole axis turns on: if it + /// fails, candidates score zero again. #[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)); + fn a_pooled_vote_newer_than_the_window_supersedes_the_baseline() { + let mut store = anchored_store(); + let anchor = store.head().expect("head root"); + let carried = 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, + 8, + anchor, vec![AggregatedAttestation { aggregation_bits: make_proof_for_validator(0).participants, - data: on_chain_data.clone(), + data: carried.clone(), }], ); + let head = block.message.hash_tree_root(); store - .insert_signed_block(block.message.hash_tree_root(), block) + .insert_signed_block(head, block) .expect("insert signed block"); // A later attestation from the same validator, still only in the pool. @@ -2253,18 +2429,19 @@ mod tests { ); store.promote_new_aggregated_payloads(); - let on_chain = store.extract_on_chain_votes(); + let window = store.extract_head_vote_window(head, 3); assert_eq!( - on_chain[&0], on_chain_data, - "the pool must not advance the on-chain baseline" + window.votes[&0], carried, + "the pool must not advance the branch-relative baseline" ); assert!( - fresher.supersedes(&on_chain[&0]), - "the pooled vote must read as new against what the chain carries" + fresher.supersedes(&window.votes[&0]), + "the pooled vote must read as new against what the branch 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" + "and must read as NOT new against known_votes, which is the bug \ + this baseline exists to avoid" ); } From 4b7a291c10e36aca5dd1e66232aba479b942e9b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Wed, 23 Sep 2026 17:34:00 -0300 Subject: [PATCH 7/9] fix(blockchain): stop filtering votes whose target is their source Once the rung the head can reach is the one already justified, every honest vote names it as both source and target. entry_passes_filters rejected that shape, and the aggregation worker shares the filter, so aggregators proved nothing for those slots. The "new" pool was then empty when the safe target was computed, the safe target fell back to the justified root, and the next slot's attestation target walked back onto the same rung. On devnet-5 that loop held the safe target 4-6 slots behind the head for 3-4 slots out of every 6, until the target lookback cap forced the head onto the next rung. The state transition skips such a vote without rejecting the block, and block import still records it as a fork-choice vote: the same trade as the already-justified target this filter already admits. score_entry zeroes its justification axis (the source, and so the target, is justified), so it competes on head-vote value alone. --- crates/blockchain/src/block_builder.rs | 91 ++++++++++++++++++++++---- 1 file changed, 77 insertions(+), 14 deletions(-) diff --git a/crates/blockchain/src/block_builder.rs b/crates/blockchain/src/block_builder.rs index c9a365f4..ff9436bd 100644 --- a/crates/blockchain/src/block_builder.rs +++ b/crates/blockchain/src/block_builder.rs @@ -689,17 +689,17 @@ impl ProjectedState { /// /// 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. + /// the candidate-block chain view, 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`]). + /// Deliberately does NOT reject an already-justified target, nor a target + /// equal to its source, though `is_valid_vote` skips both: those votes + /// still carry fork-choice weight, so they are scored rather than filtered + /// (see the note at the end, 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. + /// justifiability 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, @@ -727,11 +727,9 @@ impl ProjectedState { if !attestation_data_matches_chain(extended_historical_block_hashes, att_data) { return Err("chain_mismatch"); } - let is_genesis_self_vote = is_genesis_self_vote(att_data); - if !is_genesis_self_vote && att_data.target.slot <= att_data.source.slot { - return Err("target_not_after_source"); - } - // An already-justified target is deliberately NOT rejected here. + // An already-justified target is deliberately NOT rejected here, and + // neither is a target equal to its source (the source is justified by + // the check above, so that target is settled too). // // The state transition skips such a vote without rejecting the block // (`is_valid_vote` returns `Ok(false)` and `process_attestations` does @@ -746,7 +744,17 @@ impl ProjectedState { // 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 + // + // The target == source case is the same trade on the aggregation + // worker, which shares this filter. Once the rung the head can reach is + // the one already justified, every honest vote names it as both source + // and target. Rejecting them left the aggregators with nothing to prove + // for those slots, so the "new" pool was empty when the safe target was + // computed, the safe target fell back to the justified root, and the + // next slot's target walked back onto that same rung: a loop that held + // the safe target several slots behind the head until the attestation + // target's lookback cap forced the head onto the next rung. + if !is_genesis_self_vote(att_data) && !slot_is_justifiable_after(att_data.target.slot, self.finalized_slot) { return Err("target_not_justifiable"); @@ -1751,6 +1759,61 @@ mod tests { ); } + /// A vote whose target IS its source passes too, on its head votes alone. + /// Once the rung the head can reach is the one already justified, every + /// honest vote has this shape: filtering it left the aggregators nothing to + /// prove for those slots and held the safe target on the justified root. + #[test] + fn entry_passes_filters_admits_a_target_equal_to_its_source() { + const FINALIZED_SLOT: u64 = 0; + const JUSTIFIED_SLOT: u64 = 2; + const HEAD_SLOT: u64 = 3; + + let mut justified_slots = JustifiedSlots::new(); + justified_slots_ops::extend_to_slot(&mut justified_slots, FINALIZED_SLOT, JUSTIFIED_SLOT); + justified_slots_ops::set_justified(&mut justified_slots, FINALIZED_SLOT, JUSTIFIED_SLOT); + + let roots: Vec = (0..5u8).map(|i| H256([i + 1; 32])).collect(); + let justified = Checkpoint { + slot: JUSTIFIED_SLOT, + root: roots[JUSTIFIED_SLOT as usize], + }; + let att_data = AttestationData { + slot: 4, + head: Checkpoint { + slot: HEAD_SLOT, + root: roots[HEAD_SLOT as usize], + }, + target: justified, + source: justified, + }; + + let projected = ProjectedState { + justified_slots, + finalized_slot: FINALIZED_SLOT, + current_votes: HashMap::new(), + head_window: Some(window(&[att_data.head.root], &[])), + }; + let known: HashSet = roots.iter().copied().collect(); + + assert_eq!( + projected.entry_passes_filters(&att_data, &known, &roots), + Ok(()), + "target == source is a scoring question, not a validity one" + ); + + let coverage: HashSet = HashSet::from([0, 1]); + let (score, new_voters, new_head_voters) = projected + .score_entry(&att_data, &coverage, 10) + .expect("its head votes are new"); + assert!( + new_voters.is_empty(), + "a target equal to its justified source adds no justification voter" + ); + assert_eq!(new_head_voters, coverage); + assert_eq!(score.tier, Tier::Build); + } + /// Below the head threshold there is no `TargetAdvance`: the entry is /// carrying weight, not settling anything. #[test] From 2462b5bdfcc9dfc28f0eb35cd895e87a2b41c2cd 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, 25 Sep 2026 16:52:04 -0300 Subject: [PATCH 8/9] fix(blockchain): require TargetAdvance to cross the head threshold TargetAdvance measured only the post-state, so once a head sat at 2/3 every entry adding a single vote for it claimed the tier and outranked Build entries bringing real justification voters. The justification axis never has this problem because a crossed target is marked justified and later entries read as settled; the head axis has no such settling. In steady state the block before the head carries nearly everyone's vote for the block two back, so a few late validators still naming it made exactly that entry, and with one aggregation job per slot the worker could spend it on that sliver. The head count must now be below 2/3 before the entry and at or above it after. Also from review: warn when the head-vote window read hits a store error instead of silently shortening it, rename the skip tag that now covers both axes, and correct the EntryScore docs, which claimed head votes never outrank justification voters in any tier. --- crates/blockchain/src/aggregation.rs | 2 +- crates/blockchain/src/block_builder.rs | 195 +++++++++++++++++++------ crates/storage/src/store.rs | 40 ++++- 3 files changed, 185 insertions(+), 52 deletions(-) diff --git a/crates/blockchain/src/aggregation.rs b/crates/blockchain/src/aggregation.rs index 324cba47..ed2e1ea1 100644 --- a/crates/blockchain/src/aggregation.rs +++ b/crates/blockchain/src/aggregation.rs @@ -553,7 +553,7 @@ fn pick_best_candidate( 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); + trace_skipped_candidate("no_new_voters_or_head_votes", att_data, data_root); continue; }; diff --git a/crates/blockchain/src/block_builder.rs b/crates/blockchain/src/block_builder.rs index 00a7fc11..848687be 100644 --- a/crates/blockchain/src/block_builder.rs +++ b/crates/blockchain/src/block_builder.rs @@ -190,8 +190,9 @@ pub(crate) fn build_block( /// Tiered greedy attestation selection for block proposal. /// /// Each round scores remaining candidates against a projected post-state and -/// picks the best per `EntryScore`: tier 1 (finalizes source) beats tier 2 -/// (justifies target) beats tier 3 (adds new voters). Justification and +/// picks the best per `EntryScore`: `Finalize` (finalizes source) beats +/// `Justify` (justifies target) beats `TargetAdvance` (brings the head to 2/3) +/// beats `Build` (adds new voters or head votes). Justification and /// finalization are projected incrementally so dependent attestations become /// eligible on the next round without re-running the STF. /// @@ -316,7 +317,7 @@ fn pick_best_candidate( 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); + trace_skipped_attestation("no_new_voters_or_head_votes", att_data, data_root); continue; }; @@ -508,22 +509,25 @@ impl ProjectedState { .unwrap_or(false) } - /// Whether applying this entry puts 2/3 of the validator set's latest head - /// votes on `att_data.head.root`. + /// Whether applying this entry takes `att_data.head.root` from below 2/3 of + /// the validator set's latest head votes to at least 2/3. /// - /// 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. + /// Both ends are measured, unlike `crosses_2_3` for justification, which + /// only checks the post-state. That axis gets the "below before" half for + /// free: once a target crosses, `advance` marks it justified and every + /// later entry for it reads as settled, adding no voters. The head axis has + /// no such settling, so without the pre-state check every entry adding a + /// single vote to a head already at 2/3 would claim [`Tier::TargetAdvance`] + /// and outrank every `Build` entry, including ones bringing real + /// justification voters. In steady state that head is common: the block + /// before the head carries nearly everyone's vote for the block two back, + /// and a few late validators still naming it make exactly that entry. /// - /// Both halves of that count come from the same branch-relative window, so - /// the post-state this measures is the one the block being built would - /// actually produce. A per-validator map fed by every block import would - /// break the second half in the direction that matters: it would retain - /// validators whose vote only a sibling branch carried, letting an entry - /// that moves one head claim a threshold this branch is nowhere near and - /// outrank, at [`Tier::TargetAdvance`], entries bringing real justification - /// voters. + /// Both counts come from the same branch-relative window, so they describe + /// the branch the block being built extends. A per-validator map fed by + /// every block import would retain validators whose vote only a sibling + /// branch carried, letting an entry claim a threshold this branch is + /// nowhere near. /// /// `None` head votes means the axis is switched off, so no supermajority /// can be claimed. @@ -537,14 +541,25 @@ impl ProjectedState { return false; }; let head_root = att_data.head.root; - // Everyone this entry moves lands on `head_root` by construction. - let retained = head_window + let meets_threshold = |count: usize| 3 * count >= 2 * validator_count; + + let before = head_window .votes + .values() + .filter(|vote| vote.head.root == head_root) + .count(); + // Everyone this entry moves lands on `head_root`; those already naming + // it are counted in `before`. + let moved_on = new_head_voters .iter() - .filter(|(vid, vote)| vote.head.root == head_root && !new_head_voters.contains(vid)) + .filter(|vid| { + head_window + .votes + .get(vid) + .is_none_or(|vote| vote.head.root != head_root) + }) .count(); - let total = retained + new_head_voters.len(); - 3 * total >= 2 * validator_count + !meets_threshold(before) && meets_threshold(before + moved_on) } /// The subset of `coverage` whose latest head vote this entry would @@ -587,7 +602,7 @@ 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::TargetAdvance`] if those votes carry the head past 2/3 and + /// [`Tier::TargetAdvance`] if those votes carry the head across 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. @@ -657,12 +672,9 @@ impl ProjectedState { // 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); + // Same rule on the head axis: the threshold has to be crossed BY this + // entry, however much weight already sits on its head. + let advances_head = self.head_crosses_2_3(att_data, &new_head_voters, validator_count); let tier = if justifies && finalizes { Tier::Finalize @@ -777,8 +789,9 @@ pub(crate) enum Tier { Finalize = 1, /// Applying the entry crosses 2/3 on target but does not finalize. Justify = 2, - /// Applying the entry brings 2/3 of validators' latest head votes onto the - /// entry's head root, without justifying anything. + /// Applying the entry takes the entry's head root from below 2/3 of + /// validators' latest head votes to at least 2/3, without justifying + /// anything. A head already at 2/3 before the entry does not qualify. /// /// 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 @@ -800,18 +813,22 @@ pub(crate) enum Tier { /// /// - **Finalize / Justify**: the entry already crosses 2/3 on its target, so /// newer chain progress leads: larger `target_slot`, then larger `att_slot`, -/// then more `new_voters`. Pushing the justified slot as far forward as -/// possible shortens recovery from a justification or finalization stall. -/// - **Build**: the entry only adds marginal voters toward the threshold, so -/// 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. +/// then more `new_head_voters`, then more `new_voters`. Pushing the justified +/// slot as far forward as possible shortens recovery from a justification or +/// finalization stall. Head votes come before justification voters here +/// because the target is crossing either way, so the marginal justification +/// voter is worth less than the head weight riding along with it. +/// - **TargetAdvance**: the entry settles a head, not a target, so larger +/// `att_slot` leads, then more `new_head_voters`. Neither `target_slot` nor +/// `new_voters` is ranked. +/// - **Build**: the entry only adds marginal weight below every threshold, so +/// coverage leads: more `new_voters`, then more `new_head_voters`, then +/// larger `target_slot`, then larger `att_slot`. Head votes break a tie on +/// justification value and never outrank it: two entries bringing the same +/// justification voters are not equivalent, since the one also moving more +/// validators' latest head is worth more to fork choice. /// -/// In both tiers `data_root` (ascending) is the final deterministic tiebreak. +/// In every tier `data_root` (ascending) is the final deterministic tiebreak. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) struct EntryScore { pub(crate) tier: Tier, @@ -1919,6 +1936,97 @@ mod tests { ); } + /// `TargetAdvance` needs the entry to cross 2/3, not merely to land on a + /// head already above it. Otherwise one late vote for a head that already + /// carries a supermajority would outrank a `Build` entry bringing real + /// justification voters. + #[test] + fn target_advance_requires_crossing_not_already_above() { + const VALIDATOR_COUNT: usize = 10; + // Validators 0..=6 (7 of 10, already >= 2/3) name DEFAULT_HEAD. + let carried: Vec<(u64, AttestationData)> = (0..=6).map(|v| (v, make_att_data(4))).collect(); + + // A: one more validator onto the already-supermajority head. + let a = make_att_data(5); + let a_coverage: HashSet = HashSet::from([7]); + + // B: 3 justification voters on an open target, head outside the window. + let b = AttestationData { + slot: 5, + head: Checkpoint { + slot: 4, + root: H256([9u8; 32]), + }, + target: Checkpoint { + slot: 3, + root: H256([3u8; 32]), + }, + source: Checkpoint { + slot: 0, + root: H256::ZERO, + }, + }; + let b_coverage: HashSet = HashSet::from([0, 1, 2]); + + let projected = ProjectedState { + justified_slots: JustifiedSlots::new(), + finalized_slot: 0, + current_votes: HashMap::new(), + head_window: Some(window(&[DEFAULT_HEAD], &carried)), + }; + + let (score_a, _, new_head_voters_a) = projected + .score_entry(&a, &a_coverage, VALIDATOR_COUNT) + .expect("A moves a head vote"); + let (score_b, _, _) = projected + .score_entry(&b, &b_coverage, VALIDATOR_COUNT) + .expect("B adds justification voters"); + + assert_eq!(new_head_voters_a.len(), 1); + assert_eq!( + score_a.tier, + Tier::Build, + "A did not cross 2/3, the head was already there" + ); + assert!( + score_b.ordering_key(H256([2u8; 32])) < score_a.ordering_key(H256([1u8; 32])), + "B's justification voters must outrank A's single head vote" + ); + } + + /// The pre-state check also applies across rounds: once a selected entry + /// carries the head across 2/3, a later entry for the same head is no + /// longer `TargetAdvance`, just as a crossed target stops justifying. + #[test] + fn target_advance_is_claimed_once_per_head() { + const VALIDATOR_COUNT: usize = 10; + // Validators 0..=5 (6 of 10, just below 2/3) name DEFAULT_HEAD. + let carried: Vec<(u64, AttestationData)> = (0..=5).map(|v| (v, make_att_data(4))).collect(); + let mut projected = ProjectedState { + justified_slots: JustifiedSlots::new(), + finalized_slot: 0, + current_votes: HashMap::new(), + head_window: Some(window(&[DEFAULT_HEAD], &carried)), + }; + + let first = make_att_data(5); + let (score, _, new_head_voters) = projected + .score_entry(&first, &HashSet::from([6]), VALIDATOR_COUNT) + .expect("it moves a head vote"); + assert_eq!(score.tier, Tier::TargetAdvance, "6 -> 7 of 10 crosses 2/3"); + projected.advance_head_votes(&first, new_head_voters); + + let second = make_att_data(6); + let (score, _, _) = projected + .score_entry(&second, &HashSet::from([7]), VALIDATOR_COUNT) + .expect("it still moves a head vote"); + assert_eq!( + score.tier, + Tier::Build, + "the head crossed in the previous round, so 7 -> 8 crosses nothing" + ); + } + /// `TargetAdvance` ranks on recency first, then head weight. The target is /// not moving at this tier, so `newer_target` is deliberately not consulted. #[test] @@ -2030,7 +2138,8 @@ mod tests { assert!(target_advance.ordering_key(root) < build.ordering_key(root)); } - /// Head votes break a tie on justification voters, and never outrank them. + /// At `Build` tier, 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; diff --git a/crates/storage/src/store.rs b/crates/storage/src/store.rs index b429eb71..9d12dea8 100644 --- a/crates/storage/src/store.rs +++ b/crates/storage/src/store.rs @@ -9,6 +9,7 @@ use crate::error::Error; use ethlambda_crypto::signature::ValidatorSignature; use ethlambda_types::{ + ShortRoot, attestation::{ AggregatedAttestation, AggregationBits, AttestationData, HashedAttestationData, bits_is_subset, validator_indices, @@ -27,7 +28,7 @@ use libssz::{SszDecode, SszEncode}; use crate::state_diff::StateDiff; use thiserror::Error; -use tracing::{error, info}; +use tracing::{error, info, warn}; /// Errors returned by [`Store::get_forkchoice_store`]. #[derive(Debug, Error)] @@ -1558,7 +1559,7 @@ impl Store { /// Feeds `known_votes`, which is what fork choice weighs: every vote this /// node knows, however it arrived. Block production does NOT score against /// this map; it reads the branch it is building on through - /// [`Self::extract_head_window_votes`]. + /// [`Self::extract_head_vote_window`]. fn record_known_attestation_votes(&self, attestations: &[AggregatedAttestation]) { let mut fork_choice = self.fork_choice.lock().unwrap(); for attestation in attestations { @@ -1606,20 +1607,43 @@ impl Store { /// is the checkpoint-sync anchor, and it resolves on the first import. The /// walk stops at a root with no header at all, which is the normal /// terminator: the anchor's parent names no block. + /// + /// A read error is treated like a missing record, so it shortens the window + /// rather than failing the proposal, but it is logged: unlike a missing + /// record it is never expected. pub fn extract_head_vote_window(&self, head_root: H256, blocks: usize) -> HeadVoteWindow { let mut window = HeadVoteWindow::default(); let mut root = head_root; for _ in 0..blocks { - let Ok(Some(header)) = self.get_block_header(&root) else { - break; + let header = match self.get_block_header(&root) { + Ok(Some(header)) => header, + Ok(None) => break, + Err(err) => { + warn!( + %err, + block_root = %ShortRoot(&root.0), + "Head-vote window cut short: failed to read block header" + ); + break; + } }; window.roots.insert(root); - if let Ok(Some(block)) = self.get_block(&root) { - for attestation in block.body.attestations.iter() { - for validator_id in validator_indices(&attestation.aggregation_bits) { - Self::record_vote(&mut window.votes, validator_id, &attestation.data); + match self.get_block(&root) { + Ok(Some(block)) => { + for attestation in block.body.attestations.iter() { + for validator_id in validator_indices(&attestation.aggregation_bits) { + Self::record_vote(&mut window.votes, validator_id, &attestation.data); + } } } + Ok(None) => {} + Err(err) => { + warn!( + %err, + block_root = %ShortRoot(&root.0), + "Head-vote window missing a block's votes: failed to read block" + ); + } } root = header.parent_root; } From 66eba0eb42107f15b01255535aa77b61bed9dd17 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, 25 Sep 2026 16:56:42 -0300 Subject: [PATCH 9/9] fix(blockchain): don't credit a re-vote for the same head as head weight new_head_voters counted every validator whose window vote the entry supersedes, including one whose newer vote names the same head root. LMD-GHOST weighs a vote only by the head it names, so that re-vote moves no weight, yet it scored as a new head voter: it kept otherwise worthless entries alive at Build and spent block space on them. A validator now counts only when the window carries no vote for it, or its window vote names a different head that this entry supersedes. Checking the head root first also keeps the common case, a vote the window already carries, off the data-root hashing supersedes does on a slot tie. With every new head voter moving from another head, head_crosses_2_3 adds the set's size directly. Three tests used a same-head newer vote to stand for "a vote that moves weight"; they now name a different head so they keep testing what their names say. --- crates/blockchain/src/block_builder.rs | 106 ++++++++++++++++++------- 1 file changed, 77 insertions(+), 29 deletions(-) diff --git a/crates/blockchain/src/block_builder.rs b/crates/blockchain/src/block_builder.rs index 848687be..d14a1f90 100644 --- a/crates/blockchain/src/block_builder.rs +++ b/crates/blockchain/src/block_builder.rs @@ -548,23 +548,24 @@ impl ProjectedState { .values() .filter(|vote| vote.head.root == head_root) .count(); - // Everyone this entry moves lands on `head_root`; those already naming - // it are counted in `before`. - let moved_on = new_head_voters - .iter() - .filter(|vid| { - head_window - .votes - .get(vid) - .is_none_or(|vote| vote.head.root != head_root) - }) - .count(); - !meets_threshold(before) && meets_threshold(before + moved_on) + // Disjoint from `before` by construction: `new_head_voters` excludes a + // validator whose window vote already names `head_root`, so everyone in + // it moves onto this head from elsewhere or from nothing. + let after = before + new_head_voters.len(); + !meets_threshold(before) && meets_threshold(after) } - /// The subset of `coverage` whose latest head vote this entry would - /// replace, per the LMD-GHOST latest-message rule - /// ([`AttestationData::supersedes`]). + /// The subset of `coverage` whose head weight this entry would move: a + /// validator counts when the window carries no vote for it, or when the + /// window's vote names a different head and this entry replaces it per the + /// LMD-GHOST latest-message rule ([`AttestationData::supersedes`]). + /// + /// A newer vote naming the same head is not counted. It becomes the + /// validator's latest message, but LMD-GHOST weighs a vote only by the head + /// it names, so no weight moves and there is nothing for this axis to + /// credit. The head-root check also runs before `supersedes`, which keeps + /// the common case (a vote the window already carries) off the data-root + /// hashing `supersedes` does on a slot tie. /// /// Empty unless this entry names a head still inside the window. A head /// older than that is not in play: the block it names already sits under a @@ -588,10 +589,9 @@ impl ProjectedState { .iter() .copied() .filter(|vid| { - head_window - .votes - .get(vid) - .is_none_or(|existing| att_data.supersedes(existing)) + head_window.votes.get(vid).is_none_or(|existing| { + existing.head.root != att_data.head.root && att_data.supersedes(existing) + }) }) .collect() } @@ -1634,6 +1634,15 @@ mod tests { #[test] fn score_entry_drops_an_entry_that_adds_neither_voters_nor_head_votes() { let coverage: HashSet = HashSet::from([0, 1, 2]); + // Newer votes for a different head, so the entry loses on recency + // rather than on naming the head those votes already name. + let newer_vote = AttestationData { + head: Checkpoint { + slot: 8, + root: H256([8u8; 32]), + }, + ..make_att_data(9) + }; let projected = ProjectedState { justified_slots: JustifiedSlots::new(), finalized_slot: 0, @@ -1643,9 +1652,9 @@ mod tests { head_window: Some(window( &[DEFAULT_HEAD], &[ - (0, make_att_data(9)), - (1, make_att_data(9)), - (2, make_att_data(9)), + (0, newer_vote.clone()), + (1, newer_vote.clone()), + (2, newer_vote), ], )), }; @@ -1659,17 +1668,25 @@ mod tests { } /// Credited head votes do not count twice across selection rounds, and a - /// genuinely newer vote still does. + /// genuinely newer vote for a different head still does. #[test] fn advance_head_votes_prevents_double_counting_across_rounds() { + let next_head = H256([6u8; 32]); let mut projected = ProjectedState { justified_slots: JustifiedSlots::new(), finalized_slot: 0, current_votes: HashMap::new(), - head_window: Some(window(&[DEFAULT_HEAD], &[])), + head_window: Some(window(&[DEFAULT_HEAD, next_head], &[])), }; let coverage: HashSet = HashSet::from([0, 1]); let first = make_att_data(5); + let later = AttestationData { + head: Checkpoint { + slot: 6, + root: next_head, + }, + ..make_att_data(6) + }; let credited = projected.new_head_voters(&first, &coverage); assert_eq!( @@ -1684,11 +1701,34 @@ mod tests { "the same entry must not be credited a second time" ); assert_eq!( - projected - .new_head_voters(&make_att_data(6), &coverage) - .len(), + projected.new_head_voters(&later, &coverage).len(), 2, - "a later slot still supersedes what this block already credited" + "a later vote for another head still supersedes what this block \ + already credited" + ); + } + + /// A newer vote naming the head a validator's window vote already names + /// replaces its latest message but moves no LMD-GHOST weight, so it is not + /// a new head voter. Only the validator with no window vote counts. + #[test] + fn a_newer_vote_for_the_same_head_moves_no_head_weight() { + let entry = make_att_data(5); + let coverage: HashSet = HashSet::from([0, 1, 2]); + let projected = ProjectedState { + justified_slots: JustifiedSlots::new(), + finalized_slot: 0, + current_votes: HashMap::new(), + head_window: Some(window( + &[DEFAULT_HEAD], + &[(0, make_att_data(4)), (1, make_att_data(4))], + )), + }; + + assert_eq!( + projected.new_head_voters(&entry, &coverage), + HashSet::from([2]), + "validators 0 and 1 already name this head, so re-voting it moves nothing" ); } @@ -1866,12 +1906,20 @@ mod tests { let att_data = make_att_data(5); // 1 of 10 validators is nowhere near 2/3. let coverage: HashSet = HashSet::from([0]); + // Validator 0's window vote names an older head, so this entry moves it. + let older_head_vote = AttestationData { + head: Checkpoint { + slot: 3, + root: H256([3u8; 32]), + }, + ..make_att_data(4) + }; let projected = ProjectedState { justified_slots: JustifiedSlots::new(), finalized_slot: 0, current_votes: HashMap::from([(att_data.target.root, coverage.clone())]), - head_window: Some(window(&[att_data.head.root], &[(0, make_att_data(4))])), + head_window: Some(window(&[att_data.head.root], &[(0, older_head_vote)])), }; let (score, _, new_head_voters) = projected