feat(blockchain): score head votes when selecting attestations to pack

[MegaRedHand]

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:

• Two entries bringing the same justification voters were treated as interchangeable, even when one moved far more validators' latest head.
• An entry whose target was already fully covered was dropped outright (score_entry returned None), despite carrying the freshest head votes anyone had.

This came out of a devnet-5 investigation. On a chain where justified - finalized sits at 6, slot_is_justifiable_after only 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

ProjectedState optionally carries 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 new head voters alongside new justification voters. 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:

Build:            (tier, more_new_voters, MORE_NEW_HEAD_VOTERS, newer_target, newer_att, data_root)
Justify/Finalize: (tier, newer_target, newer_att, more_new_voters, MORE_NEW_HEAD_VOTERS, data_root)

An entry that adds only head votes is now kept at Build tier rather than dropped. It stays at Build regardless of the prior vote count, since an entry adding no justification voter cannot push its target past the threshold whatever prior_count already 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 it None. 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 and score_entry would stop returning None — disabling the worker's zero-value skip. head_vote_scoring_is_off_when_the_map_is_not_seeded guards this.

Store::should_replace_vote moved to AttestationData::supersedes. The scorer needs the same latest-message rule fork choice applies, and a second copy would drift. It lands in ethlambda-types rather than beside fork choice because the vote map is maintained in the storage layer, and ethlambda-storage does not depend on the fork choice crate (only on ethlambda-crypto and ethlambda-types); hoisting it there would force a new storage → fork_choice edge and invert the layering.

build_block and select_attestations now take a ProposalInputs struct rather than growing another loose parameter each (clippy's too_many_arguments fires 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 test all 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_antisymmetric
• head_vote_scoring_is_off_when_the_map_is_not_seeded
• score_entry_keeps_an_entry_that_only_adds_head_votes
• score_entry_drops_an_entry_that_adds_neither_voters_nor_head_votes
• advance_head_votes_prevents_double_counting_across_rounds
• head_votes_break_a_tie_on_justification_voters

[github-actions]

🤖 Kimi Code Review

I'll review this PR which adds head-vote scoring to block building, integrating LMD-GHOST latest-message awareness into attestation selection.

Overall Assessment

This 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 Issues

1. supersedes tie-breaker is non-deterministic across implementations (Consensus Risk)

File: crates/common/types/src/attestation.rs, lines 52-53

pub fn supersedes(&self, other: &AttestationData) -> bool {
    self.slot > other.slot
        || (self.slot == other.slot && self.hash_tree_root() > other.hash_tree_root())
}

Problem: Using hash_tree_root() as a tie-breaker creates a consensus risk. The hash_tree_root() of AttestationData depends on SSZ serialization, but the AttestationData struct contains Checkpoint fields with H256 roots. If two different AttestationData values have the same slot but different contents that hash to different roots, nodes will agree on ordering. However, the comment claims this ensures two nodes "that saw the same votes in different orders still agree"—but this only holds if the SSZ implementation is perfectly consistent across all nodes.

More critically: hash_tree_root() computes a Merkle root. This is expensive (see Point 5) and unnecessary. The spec typically uses hash_tree_root() for signing roots, but for internal tie-breaking, a cheaper deterministic comparison would suffice. Worse, if there's any version skew in SSZ encoding between nodes, this tie-breaker diverges.

Recommendation: Consider whether a simpler canonical byte comparison or struct field comparison would suffice, or document why hash_tree_root() is specifically required for consensus compatibility.

---

2. advance_head_votes clones AttestationData on every insertion

File: crates/blockchain/src/block_builder.rs, lines 450-456

pub(crate) fn advance_head_votes(
    &mut self,
    att_data: &AttestationData,
    new_head_voters: impl IntoIterator<Item = u64>,
) {
    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());  // ← clones every time
    }
}

Problem: att_data.clone() is called for every head voter. In a block with 128 attestations and ~500 validators each, this is 64,000 clones of a moderately large struct.

Recommendation: Since advance_head_votes is called once per selected entry with the same att_data, and new_head_voters is already computed, consider whether head_votes can store H256 (the data root) instead of full AttestationData, or use Arc<AttestationData>. The supersedes check only needs slot and hash_tree_root, not the full struct.

---

3. new_head_voters allocates a HashSet even when head votes are disabled

File: crates/blockchain/src/block_builder.rs, lines 462-475

fn new_head_voters(&self, att_data: &AttestationData, coverage: &HashSet<u64>) -> HashSet<u64> {
    let Some(head_votes) = self.head_votes.as_ref() else {
        return HashSet::new();  // ← allocates empty HashSet on every call
    };
    // ...
}

Problem: When head_votes is None (aggregation worker path), this still allocates an empty HashSet for every candidate entry evaluated. In a large pool with thousands of entries, this is significant overhead.

Recommendation: Return a Cow<'_, HashSet<u64>> or use a static empty set. Better: restructure so the caller checks head_votes.is_some() before calling, or return Option<HashSet<u64>> where None means "no head voters."

---

4. score_entry return type is becoming unwieldy

File: crates/blockchain/src/block_builder.rs, line 503

-> Option<(EntryScore, HashSet<u64>, HashSet<u64>)>

Problem: Triples with same-typed HashSets are error-prone. The caller at line 293-295 destructures these:

let Some((score, new_voters, new_head_voters)) =
    projected.score_entry(att_data, &coverage, chain.validator_count)

It's easy to swap new_voters and new_head_voters by mistake.

Recommendation: Introduce a named struct:

struct ScoreResult {
    score: EntryScore,
    new_voters: HashSet<u64>,
    new_head_voters: HashSet<u64>,
}

This is already partially addressed by EntryScore containing new_head_voters for display, but the returned sets are still separate.

---

Medium Issues

5. hash_tree_root() in hot path (Performance)

File: crates/common/types/src/attestation.rs, line 53

supersedes calls self.hash_tree_root() and other.hash_tree_root(). This is called in:

• new_head_voters (line 472): for every validator in coverage, for every candidate entry, every round
• record_vote in storage (line 1519): on every attestation insertion

Problem: hash_tree_root() computes a full SSZ Merkleization. In the block builder's inner loop, this could be called tens of thousands of times.

Recommendation: Cache the hash tree root in AttestationData or use a cheaper comparison. Since AttestationData is immutable after construction, this is a good candidate for lazy initialization or interning.

---

6. new_head_voters iterates entire coverage even when most are not new

File: crates/blockchain/src/block_builder.rs, lines 468-475

coverage
    .iter()
    .copied()
    .filter(|vid| {
        head_votes
            .get(vid)
            .is_none_or(|existing| att_data.supersedes(existing))
    })
    .collect()

Problem: This iterates all validators in coverage. If head_votes contains most of them with newer votes, we still check every one.

Recommendation: If typical case is "most votes are old," consider early exit or maintaining an index. However, given typical committee sizes (128-2048), this may be acceptable.

---

7. Test head_vote_scoring_is_off_when_the_map_is_not_seeded has incorrect validator count

File: crates/blockchain/src/block_builder.rs, lines 1197-1212

let projected = ProjectedState {
    // ...
    current_votes: HashMap::from([(H256::ZERO, HashSet::from([0, 1, 2]))]),
    head_votes: None,
};
let coverage: HashSet<u64> = HashSet::from([0, 1, 2]);

assert!(
    projected
        .score_entry(&make_att_data(5), &coverage, 4)  // ← validator_count = 4
        .is_none(),

Problem: validator_count = 4 but coverage contains validators 0,1,2. The score_entry function uses validator_count for threshold calculations (line 513-514). With 4 validators, 2/3 threshold is 3 validators. The test happens to pass because new_voters is empty and new_head_voters is empty (head_votes is None), but this validator_count mismatch is confusing and fragile.

Recommendation: Use validator_count: 3 to match the actual coverage, or add a comment explaining why 4 is used.

---

Minor Issues

8. make_att_data in tests doesn't set meaningful head/target/source

File: crates/blockchain/src/block_builder.rs (various test locations)

Tests use make_att_data(5) which likely creates default Checkpoint values. The supersedes comparison depends on slot and hash_tree_root(). With default Checkpoint fields, hash_tree_root() may collide or be predictable in ways that don't test real scenarios.

Recommendation: Ensure make_att_data sets distinct head/target/source values, or use the explicit construction shown in score_entry_keeps_an_entry_that_only_adds_head_votes.

---

9. OrderingKey type alias has grown to 6 elements

File: crates/blockchain/src/block_builder.rs, lines 667-673

pub(crate) type OrderingKey = (
    Tier,
    Reverse<u64>,
    Reverse<u64>,
    Reverse<u64>,
    Reverse<u64>,
    H256,
);

Problem: This is at the limit of readability. The Reverse<u64> elements are position-dependent and easy to confuse.

Recommendation: Consider a small struct with named fields, or at minimum document which field is which at the type alias level.

---

Positive Observations

1. Good test coverage: The PR includes 5 new tests specifically for head-vote behavior, covering edge cases like tie-breaking, double-counting prevention, and the None map guard.

2. Clear documentation: Comments explain why head_votes: None is different from Some(empty_map), and why advance_head_votes is separate from advance.

3. No consensus logic changes in state transition: This only affects block packing, not on_block validation, so it cannot cause a consensus split by itself. The risk is limited to proposer profitability and network liveness.

4. ProposalInputs struct improves API: Grouping related parameters is cleaner than the previous 5-parameter build_block signature.

---

Summary Table

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

[github-actions]

🤖 Codex Code Review

Findings

1. new_head_voters is overcounting votes that do not actually change fork-choice weight. In block_builder.rs:463, proposer scoring treats an attestation as a new “head vote” whenever att_data.supersedes(existing), but attestation.rs:52 compares the full AttestationData hash on same-slot ties. LMD-GHOST weight only depends on the validator’s latest slot and head.root; a same-slot attestation with the same head.root but different target/source does not move fork choice at all. This can make the proposer spend block space on attestations that score as adding new_head_voters even though they add zero head weight. I’d split this into a dedicated head-weight comparison, e.g. compare (slot, head.root) instead of the full data root here.

2. The new head-vote objective is inconsistent with the enable_proposer_aggregation = false path. Selection scores each data_root on the union of all proof participants in block_builder.rs:292 and then advances projected head votes for that whole union in block_builder.rs:256. But when proposer aggregation is disabled, compaction later keeps only one proof per AttestationData, chosen solely by marginal justification coverage in block_builder.rs:861. So the selector can prefer a candidate because it supposedly moves many latest-head votes, then emit a block carrying only one of those proofs. That mismatch was pre-existing for justification coverage, but this PR makes it materially worse because the new scoring axis is not preserved at all in the no-aggregation path.

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 cargo needs a Rust toolchain download and network access is blocked in this environment.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt