fix(blockchain): make proof selection tie-breaking deterministic

[pablodeymo]

🗒️ Description / Motivation

extend_proofs_greedily kept its remaining candidate proofs in a HashSet<usize> and
picked the best-coverage proof with max_by_key over the set's randomized iteration
order, so equal-coverage ties were broken arbitrarily per process: the same store state
could produce blocks with different aggregation bits from one run to the next.

Found while building the offline block-building benchmark (#497), whose same-seed
determinism check reported differing block roots across runs of an identical workload.
Split out of that PR because it is a standalone node-behavior fix, unrelated to the
harness.

What Changed

File	Change
crates/blockchain/src/block_builder.rs	remaining_indices is a Vec<usize> iterated in index order; coverage ties break toward the lowest index (pool insertion order) via max_by_key((count, Reverse(idx)))

Correctness / Behavior Guarantees

• Selection quality is unchanged: the greedy still picks maximum marginal coverage
  every round. Only the choice among equal-coverage candidates changes, and that
  choice was previously random.
• Block building is now reproducible for a given pool, which is what makes a
  baseline-vs-optimized benchmark comparison meaningful.

Tests Added / Run

• extend_proofs_greedily_breaks_coverage_ties_by_pool_order: six disjoint proofs of
  identical coverage, so every round is again a six-way tie and selection order is
  decided purely by the tie-break. An arbitrary order cannot match pool order by luck
  (1 in 720); against the previous code the test fails on most runs.
• make fmt, make lint, make test — all clean.

Related Issues / PRs

• Split out of feat(cli): add offline block-building benchmark subcommand #497
• Related to Block building optimizations #465

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• Ran make test (cargo test --workspace --profile release-fast) — all passing

[github-actions]

🤖 Kimi Code Review

Overall Assessment: This PR correctly fixes a consensus-critical determinism bug. The change ensures that block construction is deterministic across all nodes by replacing hash-iteration-order dependency with explicit index-based tie-breaking.

Critical Consensus Fix (Positive)

• Line 809, 823: Changing from HashSet<usize> to Vec<usize> and adding Reverse(idx) tie-breaking eliminates non-determinism in block construction. HashSet iteration order varies by process/runtime, causing different validators to produce different blocks (and thus disagree on the canonical chain) when given identical attestation pools.
• Line 812-816: Excellent comment explaining the consensus rationale. This should prevent future "optimizations" that reintroduce non-determinism.

Code Correctness

• Line 823: The tuple (count, Reverse(idx)) correctly prioritizes higher coverage, then lower index (pool insertion order). This is the correct semantics for deterministic greedy selection.
• Import Check: Ensure std::cmp::Reverse (or core::cmp::Reverse if no_std) is imported. The diff doesn't show imports—if missing, this will fail to compile.

Performance

• Line 850: Vec::retain is O(n) vs HashSet::remove O(1). Since the outer loop already scans all remaining indices (O(n) per iteration), the overall complexity remains O(n²) where n is the number of proofs. Given Ethereum's MAX_ATTESTATIONS bound (typically 128), this is negligible.
• Alternative: If concerned about micro-optimizations, swap_remove (O(1)) followed by manual iteration could be used, but the current code is clearer and sufficiently fast.

Testing

• Lines 1965-1990: The test is thorough. Using 6 items (1/720 chance of false positive) instead of 2 (1/2 chance) properly validates the determinism property. The disjoint coverage design correctly isolates the tie-breaking logic.

Nitpick

• Line 815-816: Minor formatting—the comment line break mid-sentence is slightly awkward but acceptable.

Verdict: Approve after verifying Reverse is imported. This is a high-quality fix for a serious consensus issue.

---

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

[github-actions]

🤖 Codex Code Review

1. crates/blockchain/src/block_builder.rs:1973 The new regression test is still probabilistic against the buggy implementation. With the old HashSet-based code, a 6-way tie can still iterate in pool order by chance, so this test passes about 1/720 of the time and won’t deterministically catch the bug in a single CI run. For a determinism fix, the test should be made deterministic instead of just low-probability.

I did not find a correctness, security, or consensus-layer bug in the production change itself. The Vec + max_by_key(|&(idx, count)| (count, Reverse(idx))) tie-break at crates/blockchain/src/block_builder.rs:809 and crates/blockchain/src/block_builder.rs:826 does make proof selection deterministic in pool order, which is the right behavior for block reproducibility. The extra retain scan is a small constant-factor cost only; I don’t see a meaningful performance or safety concern there.

Validation note: I could not run cargo test in this environment because the pinned Rust toolchain could not be fetched under the sandbox/network restrictions.

---

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

[github-actions]

🤖 Claude Code Review

I now have enough to write the review.

Review: fix(blockchain): make proof selection tie-breaking deterministic

Summary

Small, well-targeted fix: extend_proofs_greedily (crates/blockchain/src/block_builder.rs) switches its remaining-candidate set from HashSet<usize> to Vec<usize>, and breaks equal-coverage ties deterministically toward the lowest pool index via max_by_key((count, Reverse(idx))). This matches the exact pattern already used a few dozen lines above in keep_best_proof_per_data (line 766), so the fix is consistent with an established convention in this file rather than introducing a new one.

Correctness

• The tie-break is sound: since idx values in remaining_indices are unique, (count, Reverse(idx)) tuples are all distinct, so there's no residual ambiguity from max_by_key's "last element wins on ties" semantics — the comparison never actually hits a tie at the tuple level. Good catch avoiding a subtly wrong fix.
• Vec::retain preserves relative order, so remaining_indices stays in ascending index order across iterations, which is required for the tie-break to consistently mean "pool insertion order."
• Selection quality is unaffected — greedy max-marginal-coverage logic is unchanged, only tie resolution.

Performance

remaining_indices.remove(&idx) (average O(1) on HashSet) becomes retain (O(n)) per round. This doesn't change the algorithm's asymptotic complexity since the per-round scan for best coverage was already O(n) (iterating all remaining candidates and calling participant_indices() per candidate), so the extra O(n) retain is a constant-factor cost, not a new complexity class. Proof-pool sizes here are bounded by validator/aggregator counts per slot, so this is a non-issue in practice.

Test coverage

extend_proofs_greedily_breaks_coverage_ties_by_pool_order is a good regression test — six disjoint, equal-coverage proofs force a tie every round, so the assertion on selection order is a strong signal (1/720 chance of passing by luck against the old code, as the PR description notes). It correctly exercises the exact bug (HashSet iteration-order nondeterminism) rather than just checking final coverage.

Minor observations (non-blocking)

• The updated comment at block_builder.rs:812-816 clearly explains why a Vec is needed instead of a HashSet (documenting the reproducibility requirement) — good practice per this repo's comment guidelines (explains non-obvious "why", not "what").
• No other HashSet-iteration-order-dependent max_by_key/min_by_key sites remain in this file — confirmed by search, so this isn't a partial fix leaving a sibling bug elsewhere in block_builder.rs.

Verdict

Looks correct, minimal, well-tested, and consistent with existing code conventions in the same file. No blocking issues found.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt