feat(blockchain): score head votes when selecting attestations to pack - #615
MegaRedHand wants to merge 5 commits into
Conversation
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.
🤖 Kimi Code ReviewI'll review this PR which adds head-vote scoring to block building, integrating LMD-GHOST latest-message awareness into attestation selection. Overall AssessmentThis is a well-structured PR with good test coverage and clear documentation. The core idea—valuing attestations for fork-choice weight in addition to justification value—is correct and important for consensus health. However, I found several issues ranging from correctness concerns to performance implications. Critical Issues1.
|
| Priority | Item | File | Line |
|---|---|---|---|
| Critical | supersedes hash_tree_root consensus risk |
attestation.rs |
52-53 |
| Critical | advance_head_votes clones AttestationData per voter |
block_builder.rs |
454 |
| High | new_head_voters allocates empty HashSet when disabled |
block_builder.rs |
465 |
| High | Triple return type is error-prone | block_builder.rs |
503 |
| Medium | hash_tree_root in hot path |
attestation.rs |
53 |
| Medium | new_head_voters full coverage scan |
block_builder.rs |
468-475 |
| Low | Test validator count mismatch | block_builder.rs |
1206 |
| Low | OrderingKey readability |
block_builder.rs |
667-673 |
Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt
🤖 Codex Code ReviewFindings
I did not see a consensus-invalidating STF / justification / finalization bug in the diff beyond those selection issues. I couldn’t run the targeted tests here because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
…r 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.
…s 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.
…ated 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`.
…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.
1 participant
Motivation
Attestation 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 concrete consequences:
score_entryreturnedNone), despite carrying the freshest head votes anyone had.This came out of a devnet-5 investigation. On a chain where
justified - finalizedsits at 6,slot_is_justifiable_afteronly admits squares and pronics above delta 5, so attestation targets land on rungs 3 slots apart. Three consecutive slots of validators all vote for the same rung, and once it is justified every later vote for it scores as worthless even though its head vote is current.What changed
ProjectedStateoptionally carries the per-validator latest votes fork choice weighs, seeded fromStore::extract_latest_known_attestationsand advanced as entries are selected so a validator is not credited twice across rounds.score_entryreports new head voters alongside new justification voters.EntryScore::ordering_keyplaces them immediately afternew_votersin 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
Buildtier rather than dropped. It stays atBuildregardless of the prior vote count, since an entry adding no justification voter cannot push its target past the threshold whateverprior_countalready is.Two details worth review attention
The head-vote map is
Option, not a possibly-empty map. The aggregation worker shares this scorer to decide which group to prove next and leaves itNone. Seeding an empty map instead would be a silent regression: with no recorded vote, every validator in an entry's coverage reads as newly covered, so every entry would score as maximally valuable andscore_entrywould stop returningNone— disabling the worker's zero-value skip.head_vote_scoring_is_off_when_the_map_is_not_seededguards this.Store::should_replace_votemoved toAttestationData::supersedes. The scorer needs the same latest-message rule fork choice applies, and a second copy would drift. It lands inethlambda-typesrather than beside fork choice because the vote map is maintained in the storage layer, andethlambda-storagedoes not depend on the fork choice crate (only onethlambda-cryptoandethlambda-types); hoisting it there would force a new storage → fork_choice edge and invert the layering.build_blockandselect_attestationsnow take aProposalInputsstruct rather than growing another loose parameter each (clippy'stoo_many_argumentsfires otherwise).Scope
This changes which attestations get packed and ranked. It does not touch
entry_passes_filters, which still drops an entry whose target is already justified before any score is computed. Letting those through is a separate follow-up.Testing
make fmt,make lint,make testall clean; full workspace suite green, zero failures.New tests:
supersedes_prefers_the_later_slot,supersedes_is_irreflexive,supersedes_breaks_a_slot_tie_on_data_root_and_is_antisymmetrichead_vote_scoring_is_off_when_the_map_is_not_seededscore_entry_keeps_an_entry_that_only_adds_head_votesscore_entry_drops_an_entry_that_adds_neither_voters_nor_head_votesadvance_head_votes_prevents_double_counting_across_roundshead_votes_break_a_tie_on_justification_voters