fix(fork-choice): make the equal-slot equivocation tie deterministic (leanSpec #1181)

[MegaRedHand]

What

Ports leanSpec #1181: make the equal-slot equivocation tie in fork choice deterministic.

Why

An equivocating validator can sign two distinct votes A and B for the same slot. Nothing rejects the second (the pool is keyed by attestation data, and the two data differ), so both are admitted. extract_latest_attestations then resolved the equal-slot tie with a strict existing.slot < entry.data.slot while iterating in insertion (arrival) order, so the winner was simply whichever was seen first. Two honest nodes with the same blocks and votes but different arrival order could land the equivocator's weight on different branches and pick different heads permanently — a determinism/safety bug that unit tests miss because it only appears across nodes.

Changes

crates/storage/src/store.rs — both extract_latest_attestations implementations (the aggregated PayloadBuffer and the raw GossipSignatureBuffer):

• Process votes newest-first, breaking the equal-slot tie toward the larger canonical attestation-data root — the same rule the block-level fork-choice tiebreak applies to block roots. The extracted head becomes a pure function of pool contents, independent of arrival/insertion order. An equivocator is counted once, on one branch every node agrees on.
• The pool key is already hash_tree_root(data), so the tie needs no extra hashing.

Block production is not changed: ethlambda's block builder already uses data_root as its deterministic final tiebreak (EntryScore::ordering_key, from leanSpec #1149), so it is already order-independent.

The drain doc comment is updated (vote-extraction determinism no longer depends on drain order), and the unit test that asserted first-seen-wins is rewritten to assert order-independence (larger canonical root wins in both arrival orders).

Tests

• extract_latest_attestations_canonical_root_wins_on_slot_tie (rewritten from ..._first_inserted_wins_on_slot_tie)

cargo fmt, clippy -D warnings, and the storage lib tests (44) pass.

Since #1181 is merged in leanSpec, the released fork-choice fixtures encode the new expected head for the equivocation vectors; this change aligns ethlambda with them. Recommend a forkchoice_spectests run against fresh fixtures to confirm.

[github-actions]

🤖 Kimi Code Review

Overall Assessment: Correct and necessary consensus fix. The change eliminates insertion-order nondeterminism in fork-choice vote extraction by using canonical (slot, data_root) ordering, preventing potential safety/liveness issues from network-level reordering.

Issues & Suggestions:

1. Performance Regression (O(n) → O(n log n))

• File: crates/storage/src/store.rs
• Lines: 306-307, 475-476
• Issue: Both extract_latest_attestations methods now collect into a Vec and sort_unstable_by on every call. For large attestation pools, this introduces significant overhead compared to the previous O(n) iteration over self.order.
• Suggestion: Add a // PERF: comment noting this trade-off. If this becomes a bottleneck, consider maintaining a BTreeMap keyed by (Reverse<Slot>, Reverse<H256>) to keep entries sorted incrementally, or benchmark typical pool sizes to ensure the sort is acceptable.

2. Documentation Consistency

• File: crates/storage/src/store.rs
• Line: 231
• Issue: The comment on drain() still implies insertion order matters for downstream consumers, but the justification for determinism is slightly stale. Since extract_latest_attestations no longer uses this order, clarify that insertion order in drain is only for deterministic re-insertion into known_payloads, not for fork-choice correctness.
• Suggested edit:

  /// deterministically; `self.data` iteration alone would be RandomState-seeded.
  /// (Note: fork-choice correctness no longer depends on this drain order, 
  /// but deterministic re-insertion avoids unnecessary state divergence.)
  

3. Unnecessary self.order Usage Check

• File: crates/storage/src/store.rs
• Line: 229 (method drain)
• Observation: The self.order field is now only used by drain(). Verify that promote_new_aggregated_payloads (mentioned in comments) still requires this determinism. If not, self.order could be removed in a future cleanup PR to reduce memory overhead.

4. Test Robustness

• File: crates/storage/src/store.rs
• Lines: 2358-2360 (test assertions)
• Observation: The test correctly verifies that validators 0 and 1 (equivocating) get the canonical winner, while 2, 3, 4 (exclusive) keep their respective votes. This is the correct property to test for consensus.
• Minor suggestion: Add an assertion that extracted.len() == 5 to ensure no duplicate entries are created if a validator appears in multiple proofs for the same attestation data (defensive check).

Security/Consensus Verification:

• Correct: The descending sort by (slot, data_root) with or_insert_with ensures that for any validator, the first processed vote is the highest slot, and for slot ties, the highest canonical root wins. This matches the leanSpec #1181 tiebreak rule.
• Correct: Using sort_unstable_by is safe here because the sort key (slot, data_root) is unique (data_root is a hash map key), so stability is irrelevant.

Code Quality:

• The use of result.entry(vid).or_insert_with(...) at lines 322 and 485 is idiomatic and efficient.
• Proper use of references in Vec<(&H256, &PayloadEntry)> avoids unnecessary cloning during the sort phase.

**

---

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

[github-actions]

🤖 Codex Code Review

Findings

1. crates/storage/src/store.rs:301 and crates/storage/src/store.rs:468: the new tie-break is described as matching block-level canonical precedence, but the code actually orders by hash_tree_root(AttestationData) via the map key, not by entry.data.head.root. Those are not equivalent because slot, source, and target also affect the SSZ root. In a same-slot equivocation between two sibling heads, this can assign the validator’s single counted vote to the lexicographically lower head if that full attestation-data root sorts higher, which can change LMD-GHOST on equal-weight forks. If the intended rule is “same canonical tiebreak as block roots”, the comparator should use (slot, entry.data.head.root) rather than (slot, data_root).

2. crates/storage/src/store.rs:474 and crates/storage/src/store.rs:2335: the PR updates both extraction paths, but only adds a same-slot regression test for PayloadBuffer. GossipSignatureBuffer::extract_latest_attestations() now has the same consensus-sensitive ordering logic and is what the "signatures" spectest checks exercise, so it needs a mirrored order-independence test as well. Right now a future drift between the two implementations would be easy to miss.

No obvious memory-safety or error-handling issues stood out in this diff.

I couldn’t run the Rust tests in this environment because cargo needs to write/fetch dependencies under a read-only home directory (~/.cargo / ~/.rustup).

---

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

[greptile-apps]

Greptile Summary

This PR fixes a determinism bug in fork-choice vote extraction where an equivocating validator's weight could land on different branches across nodes depending on message arrival order. It replaces insertion-order-based tiebreaking with a sort-then-first-wins approach that is a pure function of pool contents.

• PayloadBuffer::extract_latest_attestations and GossipSignatureBuffer::extract_latest_attestations now sort votes descending by (slot, hash_tree_root(attestation_data)) before iterating, then use or_insert_with so the first entry seen (highest canonical root) wins for each validator — matching leanSpec #1181 and the existing block-root tiebreak in fork choice.
• The drain doc comment is updated to note that fork-choice extraction no longer relies on drain order; the test for the equal-slot tiebreak is rewritten to assert order-independence rather than first-seen-wins.

Confidence Score: 4/5

The determinism fix is logically sound and the core sorting+or_insert_with pattern correctly implements the canonical-root tiebreak in both buffer types. The only gaps are a stale test doc comment and a missing parallel test for the gossip path.

The sorting approach and or_insert_with semantics correctly resolve the non-determinism for both PayloadBuffer and GossipSignatureBuffer. GossipSignatureBuffer::extract_latest_attestations receives the same logic change but no new test covering the equal-slot equivocation case, leaving a coverage gap that could hide future regressions in that path.

crates/storage/src/store.rs — specifically the GossipSignatureBuffer::extract_latest_attestations function, which lacks an equivocation tiebreak test comparable to the one added for PayloadBuffer.

Important Files Changed

Filename	Overview
crates/storage/src/store.rs	Core change: both extract_latest_attestations implementations now sort descending by (slot, data_root) and use or_insert_with for first-wins semantics. Logic is correct and symmetric. Minor: drain's test doc comment retains stale equivocation reasoning, and GossipSignatureBuffer lacks a parallel equivocation tiebreak test.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[extract_latest_attestations called] --> B[Collect self.data.iter into Vec]
    B --> C["sort_unstable_by: descending (slot, data_root)"]
    C --> D[Iterate entries in sorted order]
    D --> E[For each validator ID in entry]
    E --> F{result already has this validator?}
    F -- No --> G[or_insert_with: assign current entry.data]
    F -- Yes --> H[Skip — canonical winner already recorded]
    G --> E
    H --> E
    E -- done --> D
    D -- done --> I[Return result HashMap]

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[extract_latest_attestations called] --> B[Collect self.data.iter into Vec]
    B --> C["sort_unstable_by: descending (slot, data_root)"]
    C --> D[Iterate entries in sorted order]
    D --> E[For each validator ID in entry]
    E --> F{result already has this validator?}
    F -- No --> G[or_insert_with: assign current entry.data]
    F -- Yes --> H[Skip — canonical winner already recorded]
    G --> E
    H --> E
    E -- done --> D
    D -- done --> I[Return result HashMap]

Loading

Comments Outside Diff (2)

1. crates/storage/src/store.rs, line 2382-2385 (link)

   The drain_preserves_insertion_order test doc comment still attributes drain order to "preserving same-slot equivocation semantics through the new → known migration," but that rationale is no longer accurate after this PR — extract_latest_attestations now sorts independently of insertion order. The drain function comment itself (fn drain) was correctly updated; this test-level doc comment was missed. The test itself is still valid (drain order matters for other downstream consumers), but the stated reason is stale.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: crates/storage/src/store.rs
   Line: 2382-2385
   
   Comment:
   The `drain_preserves_insertion_order` test doc comment still attributes drain order to "preserving same-slot equivocation semantics through the new → known migration," but that rationale is no longer accurate after this PR — `extract_latest_attestations` now sorts independently of insertion order. The drain function comment itself (`fn drain`) was correctly updated; this test-level doc comment was missed. The test itself is still valid (drain order matters for other downstream consumers), but the stated reason is stale.
   
   
   
   How can I resolve this? If you propose a fix, please make it concise.

   Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

2. crates/storage/src/store.rs, line 474-487 (link)

   Missing equivocation-tiebreak test for GossipSignatureBuffer

   PayloadBuffer::extract_latest_attestations now has extract_latest_attestations_canonical_root_wins_on_slot_tie validating both insertion orders, but GossipSignatureBuffer::extract_latest_attestations has no equivalent. Since both implementations share the same sorting logic and are described as mirrors of each other, a gap here means a silent regression in the gossip path would not be caught by the test suite. Adding a parallel test that inserts two equal-slot equivocating signatures in both orders and asserts the canonical-root winner is consistent would complete the coverage.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: crates/storage/src/store.rs
   Line: 474-487
   
   Comment:
   **Missing equivocation-tiebreak test for `GossipSignatureBuffer`**
   
   `PayloadBuffer::extract_latest_attestations` now has `extract_latest_attestations_canonical_root_wins_on_slot_tie` validating both insertion orders, but `GossipSignatureBuffer::extract_latest_attestations` has no equivalent. Since both implementations share the same sorting logic and are described as mirrors of each other, a gap here means a silent regression in the gossip path would not be caught by the test suite. Adding a parallel test that inserts two equal-slot equivocating signatures in both orders and asserts the canonical-root winner is consistent would complete the coverage.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
crates/storage/src/store.rs:2382-2385
The `drain_preserves_insertion_order` test doc comment still attributes drain order to "preserving same-slot equivocation semantics through the new → known migration," but that rationale is no longer accurate after this PR — `extract_latest_attestations` now sorts independently of insertion order. The drain function comment itself (`fn drain`) was correctly updated; this test-level doc comment was missed. The test itself is still valid (drain order matters for other downstream consumers), but the stated reason is stale.

```suggestion
    /// `drain` must hand back entries in insertion order so that
    /// `promote_new_aggregated_payloads` lands them in known_payloads in the
    /// same order (deterministic re-insertion, independent of HashMap iteration).
```

### Issue 2 of 2
crates/storage/src/store.rs:474-487
**Missing equivocation-tiebreak test for `GossipSignatureBuffer`**

`PayloadBuffer::extract_latest_attestations` now has `extract_latest_attestations_canonical_root_wins_on_slot_tie` validating both insertion orders, but `GossipSignatureBuffer::extract_latest_attestations` has no equivalent. Since both implementations share the same sorting logic and are described as mirrors of each other, a gap here means a silent regression in the gossip path would not be caught by the test suite. Adding a parallel test that inserts two equal-slot equivocating signatures in both orders and asserts the canonical-root winner is consistent would complete the coverage.

_{Reviews (1): Last reviewed commit: "fix(fork-choice): make the equal-slot eq..." | Re-trigger Greptile}