refactor(blockchain): collapse the greedy selection loop guards

[pablodeymo]

🗒️ Description / Motivation

Follow-up to #590, which made the coverage tie-break in extend_proofs_greedily
deterministic and left the loop around it with three exits for two conditions.

What Changed

File

Change

crates/blockchain/src/block_builder.rs

while !remaining_indices.is_empty() made the None arm of the best-candidate lookup unreachable, and that arm in turn made the proofs.is_empty() early return redundant. Now one loop with one exit — no candidate, or none that adds coverage — and best_count no longer needs a binding just to be compared against zero. The comment records that candidates must stay in ascending order, so the retain is not later "optimized" into a swap_remove that would reintroduce the arbitrary tie-break #590 removed

Correctness / Behavior Guarantees

Pure refactor — same selection, same order, same stopping point. The removed guards were
unreachable or redundant, not load-bearing.

Tests Added / Run

• extend_proofs_greedily_selects_nothing_from_an_empty_pool pins the path the early
  return used to shortcut.
• make fmt, make lint, and the 68 ethlambda-blockchain lib tests — all clean on
  current main.

Related Issues / PRs

• Follow-up to fix(blockchain): make proof selection tie-breaking deterministic #590

✅ 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

The changes to extend_proofs_greedily in crates/blockchain/src/block_builder.rs are correct and improve code quality. A few specific observations:

Consensus Safety (Determinism)

• Line 813-814: The updated comment correctly documents the tie-breaking requirement. Using retain (O(n)) instead of swap_remove (O(1)) to remove selected indices from remaining_indices is essential for consensus—swap_remove would destroy the ascending order, causing non-deterministic selection when coverage counts are equal, leading to divergent block hashes across nodes. This is a critical correctness fix.

Logic Correctness

• Lines 804-806: Removing the proofs.is_empty() guard is safe. The loop terminates immediately when best is None (empty pool) or when count == 0 (no additional coverage), as verified by the new test at line 1988.
• Line 826: The combined best.filter(|&(_, count)| count > 0) in the let-else binding is idiomatic and correctly replaces the previous two-step check.

Performance

• The use of retain instead of swap_remove introduces O(n²) worst-case complexity for the selection loop (each iteration scans the remaining candidates). Given that:

  1. Deterministic tie-breaking is mandatory for consensus
  2. Attestation pools are bounded by the validator set size
  3. This runs only during block production (not attestation processing)

  The trade-off is acceptable. However, if profiling shows this becomes a bottleneck, consider using a BTreeMap or maintaining a separate index structure while preserving determinism.

Code Quality

• Line 826: The Reverse(idx) tie-breaker ensures stable, deterministic selection favoring earlier pool entries when coverage is equal. This is correct.
• Lines 1988-1996: The new test effectively documents the termination behavior for empty inputs and prevents regression of the removed early-return optimization.

Verdict: Approve. The refactoring is sound, the determinism fix is consensus-critical, and the test coverage is appropriate.

---

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

[github-actions]

🤖 Codex Code Review

No blocking findings.

The change in crates/blockchain/src/block_builder.rs:804 through :829 is behaviorally equivalent for correctness: an empty proofs slice now exits via best.filter(|&(_, count)| count > 0) instead of the removed front-door guard, and the zero-coverage termination condition is still preserved. Deterministic tie-breaking is unchanged, which is the consensus-relevant property here.

The added unit test at crates/blockchain/src/block_builder.rs:1992 is appropriate and directly covers the refactor’s empty-input case.

Residual risk: this was a static review only. I couldn’t run cargo test in this environment because Cargo attempted to write to a read-only home/Cargo cache while resolving dependencies.

---

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

[github-actions]

🤖 Claude Code Review

Review: PR 592 — collapse the greedy selection loop guards

This is a small, well-scoped refactor of extend_proofs_greedily in crates/blockchain/src/block_builder.rs. I traced through the control-flow equivalence carefully since this touches attestation-packing logic.

Correctness of the refactor itself

The collapse is behaviorally equivalent to the original:

• When proofs is empty, remaining_indices starts empty, so remaining_indices.iter().map(...).max_by_key(...) returns None on the very first iteration → the Some((best_idx, _)) pattern in the let-else fails → immediate break. This reproduces the old if proofs.is_empty() { return; } guard exactly, so dropping it is safe.
• best.filter(|&(_, count)| count > 0) reproduces the old if best_count == 0 { break; } check — Option::filter maps Some(x) to None when the predicate is false, which is exactly the old two-step check folded into one.
• Each iteration still strictly shrinks remaining_indices via retain, so termination is preserved.

Good catch pinning the empty-pool path with extend_proofs_greedily_selects_nothing_from_an_empty_pool — that's exactly the branch the old early return used to shortcut, and it wasn't covered before.

Finding: the new comment's justification for keeping retain over swap_remove is inaccurate

crates/blockchain/src/block_builder.rs:812-814, restated in the PR description:

"The candidates must therefore stay in ascending order, which is why the winner leaves via retain below and not swap_remove."

This isn't actually true given the current selection code. The winner is chosen by:

.max_by_key(|&(idx, count)| (count, Reverse(idx)))

Since every element of remaining_indices holds a distinct idx (they're drawn from 0..proofs.len() and only ever removed, never duplicated), the key tuple (count, Reverse(idx)) is unique per candidate. max_by_key therefore returns a result that depends only on the values being compared, not on the vector's iteration order — swapping to swap_remove (which reorders remaining_indices via a swap from the tail) would not change which (idx, count) pair wins, because the tie-break is already explicit and value-based (Reverse(idx)), not an artifact of "last element wins" iteration order.

The arbitrary tie-break that #590 actually fixed was the HashSet iteration order feeding coverage computation, not the order of remaining_indices. So the comment attributes the fix to the wrong mechanism and could mislead a future contributor into preserving an O(n) retain for a correctness reason that doesn't hold, or into distrusting a legitimate swap_remove optimization later.

This is documentation-only — it doesn't cause a behavioral bug in this PR — but since the PR's whole stated purpose is to make invariants explicit in comments to guard against future regressions, an inaccurate guard comment is worth fixing before merge. Suggest either removing the ascending-order claim or replacing it with the accurate reasoning (the Reverse(idx) key already makes selection order-independent; retain vs swap_remove is purely a performance choice, not a correctness one).

Everything else

• The loop { ... let-else break } idiom is idiomatic Rust and clippy-clean for this pattern.
• No change to selection semantics, metrics calls, or the AggregatedAttestation/SingleMessageAggregate construction — matches the "pure refactor" framing.
• Test addition is appropriately minimal and targets exactly the removed guard.

No correctness, security, or performance issues in the actual runtime behavior — the only actionable item is the misleading comment above.

---

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