diff --git a/crates/blockchain/src/aggregation.rs b/crates/blockchain/src/aggregation.rs index b9c4e762..990d870d 100644 --- a/crates/blockchain/src/aggregation.rs +++ b/crates/blockchain/src/aggregation.rs @@ -405,11 +405,24 @@ 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. + // + // 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_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, @@ -445,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); } @@ -508,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 { @@ -523,7 +540,11 @@ fn pick_best_candidate( continue; } - let Some((score, _new_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); @@ -536,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); } } @@ -2016,9 +2037,10 @@ mod tests { justified_slots: JustifiedSlots::new(), finalized_slot: 0, current_votes: HashMap::new(), + head_votes: None, }; - let (picked_root, score) = pick_best_candidate( + let (picked_root, score, _head_voters) = pick_best_candidate( &candidates, &projected, &known_block_roots, @@ -2108,10 +2130,11 @@ 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. - let (picked_root, score) = pick_best_candidate( + let (picked_root, score, _head_voters) = pick_best_candidate( &candidates, &projected, &known_block_roots, @@ -2134,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, @@ -2197,12 +2220,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; @@ -2239,7 +2270,247 @@ 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" + ); + } + + /// 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 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. + /// + /// 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 f6255964..8eb84dbc 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,30 @@ 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 that the CHAIN already carries. + /// + /// 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, +} + /// 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 +358,29 @@ 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. + /// + /// `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. + /// + /// 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 + /// 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>, } impl ProjectedState { @@ -338,9 +392,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_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); + 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 +450,111 @@ 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()); + } + } + + /// 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) + } + + /// 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`. + /// + /// 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(); + }; + 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::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, 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 @@ -403,16 +566,33 @@ impl ProjectedState { att_data: &AttestationData, coverage: &HashSet, validator_count: usize, - ) -> Option<(EntryScore, HashSet)> { + ) -> 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(); - if new_voters.is_empty() { + 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; } @@ -430,35 +610,54 @@ 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 { - Tier::Build - } else if finalizes { + // An entry that adds no justification voter cannot move the target past + // 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 { 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 /// 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, @@ -490,18 +689,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) { @@ -525,14 +727,24 @@ 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. /// -/// 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 +756,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,31 +774,65 @@ 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 { + /// 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, 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, ), } @@ -1056,9 +1308,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 +1322,488 @@ 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. + #[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!( + 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 + /// 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" + ); + } + + /// 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!( + score.tier > Tier::Justify, + "it cannot justify, so it must not be tiered as if it could" + ); + assert_eq!( + score.tier, + Tier::TargetAdvance, + "its head votes still carry the head past 2/3" + ); + } + + /// 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" + ); + } + + /// 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() { + 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 +1926,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 +2075,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 +2204,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 +2513,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 +2652,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/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/blockchain/src/store.rs b/crates/blockchain/src/store.rs index 60711f8b..049a5d43 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,21 @@ 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. + // + // 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, + aggregated_payloads: &aggregated_payloads, + latest_head_votes, + }; + let (block, signatures, post_checkpoints) = { let _timing = metrics::time_block_building_payload_aggregation(); build_block( @@ -975,8 +990,7 @@ pub fn produce_block_with_signatures( slot, validator_index, head_root, - &known_block_roots, - &aggregated_payloads, + inputs, config, )? }; 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 diff --git a/crates/common/types/src/attestation.rs b/crates/common/types/src/attestation.rs index 91d00105..dae73376 100644 --- a/crates/common/types/src/attestation.rs +++ b/crates/common/types/src/attestation.rs @@ -35,6 +35,28 @@ 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 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. 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 + /// 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 +227,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..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. @@ -1512,12 +1538,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,12 +1545,20 @@ 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()); } } + /// 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 { @@ -1540,6 +1568,11 @@ impl Store { validator_id, &attestation.data, ); + Self::record_vote( + &mut fork_choice.on_chain_votes, + validator_id, + &attestation.data, + ); } } } @@ -1549,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() @@ -2121,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());