From 9b8a861dc44c335a2ae39296dcbaaf5de1759ea5 Mon Sep 17 00:00:00 2001
From: Pablo Deymonnaz <pdeymon@fi.uba.ar>
Date: Wed, 26 Aug 2026 14:34:06 -0300
Subject: [PATCH] refactor(blockchain): collapse the greedy selection loop
 guards
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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

`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. One `loop` with one exit — no
candidate, or none that adds coverage — says the same thing, and
`best_count` no longer needs a binding just to be compared against zero.
A test pins the empty pool that the early return used to shortcut.

Also record in the comment 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.
---
 crates/blockchain/src/block_builder.rs | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/crates/blockchain/src/block_builder.rs b/crates/blockchain/src/block_builder.rs
index 6df8cda1..0b04eea5 100644
--- a/crates/blockchain/src/block_builder.rs
+++ b/crates/blockchain/src/block_builder.rs
@@ -801,19 +801,17 @@ fn extend_proofs_greedily(
     selected: &mut Vec<(AggregatedAttestation, SingleMessageAggregate)>,
     att_data: &AttestationData,
 ) {
-    if proofs.is_empty() {
-        return;
-    }
-
     let mut covered: HashSet<u64> = HashSet::new();
     let mut remaining_indices: Vec<usize> = (0..proofs.len()).collect();
 
-    while !remaining_indices.is_empty() {
+    loop {
         // Pick proof covering the most uncovered validators (count only, no
         // allocation). Coverage ties break to the lowest index (pool insertion
         // order): a HashSet here would let hash-iteration order pick an
         // arbitrary equal-coverage winner, making the built block's
-        // aggregation bits differ from run to run.
+        // aggregation bits differ from run to run. The candidates must
+        // therefore stay in ascending order, which is why the winner leaves
+        // via `retain` below and not `swap_remove`.
         let best = remaining_indices
             .iter()
             .map(|&idx| {
@@ -825,12 +823,10 @@ fn extend_proofs_greedily(
             })
             .max_by_key(|&(idx, count)| (count, Reverse(idx)));
 
-        let Some((best_idx, best_count)) = best else {
+        // Stops on an empty pool, and once no candidate adds coverage.
+        let Some((best_idx, _)) = best.filter(|&(_, count)| count > 0) else {
             break;
         };
-        if best_count == 0 {
-            break;
-        }
 
         let proof = &proofs[best_idx];
 
@@ -1989,4 +1985,14 @@ mod tests {
         let pool_order: Vec<Vec<u64>> = (0..6).map(|g| vec![g * 2, g * 2 + 1]).collect();
         assert_eq!(order, pool_order);
     }
+
+    /// An empty pool selects nothing: the candidate loop finds no best proof
+    /// and stops on its first round, so no `proofs.is_empty()` guard is needed
+    /// ahead of it.
+    #[test]
+    fn extend_proofs_greedily_selects_nothing_from_an_empty_pool() {
+        let mut selected = Vec::new();
+        extend_proofs_greedily(&[], &mut selected, &make_att_data(1));
+        assert!(selected.is_empty());
+    }
 }