consensus: handle a txid repeated inside one confirm batch like Core

changelog.d/repeated-txid-in-batch.md

@@ -0,0 +1,9 @@
+Fixed
+
+- **A txid that repeats inside one confirm batch is handled like Core.**
+  A coinbase can repeat once its first copy is spent: below BIP34, or after
+  BIP34 when an old coinbase already pushed the later height. A spend now uses
+  the copy that exists at its height, the outputs of a recreated transaction
+  can be spent again, and BIP30 counts spends made earlier in the same batch.
+  A retry after those bodies were already archived uses the same order.
+  Before, IBD could halt on such a chain that Core accepts.

crates/rbitcoin-query/src/archive.rs

@@ -570,27 +570,66 @@ struct PlanRow {
     tx_index: u32,
 }
 
+/// Newest in-batch create of this txid strictly before `spender`.
+///
+/// `repeats` holds every fk when the txid occurs more than once. Otherwise
+/// `latest` is the only copy. A copy created later in the wave is not a coin yet.
+/// The fresh plan and the archived stamp share this.
+pub(crate) fn batch_copy_before(
+    latest: Option<Fk>,
+    repeats: Option<&[Fk]>,
+    spender: Fk,
+) -> Option<Fk> {
+    match repeats {
+        Some(fks) => fks.iter().rev().find(|f| f.0 < spender.0).copied(),
+        None => latest.filter(|f| f.0 < spender.0),
+    }
+}
+
+/// Parents to resolve outside this batch. A txid this batch only creates after
+/// the spend is asked for when `outside` can have it (an earlier copy); otherwise
+/// it stays a later in-batch create, which structural rejects as not yet a coin.
 fn collect_plan_need_external(
     work: &[PlanRow],
     batch_map: &crate::TxidMap<Fk>,
+    batch_repeats: &crate::TxidMap<Vec<Fk>>,
     carried_need: Option<&[[u8; 32]]>,
+    outside: impl Fn(&[u8; 32]) -> bool,
 ) -> Vec<[u8; 32]> {
     let mut need_external: crate::TxidSet = crate::TxidSet::with_hasher(Default::default());
     if let Some(keys) = carried_need {
+        let mut created_here = false;
         for &prev in keys {
-            if prev == [0u8; 32] || batch_map.contains_key(&prev) {
+            if prev == [0u8; 32] {
+                continue;
+            }
+            if batch_map.contains_key(&prev) {
+                created_here = true;
                 continue;
             }
             need_external.insert(prev);
         }
-        return need_external.into_iter().collect();
+        // The wave asked about a txid this batch creates (below BIP34, or a
+        // spend before its create): only a per-input walk knows which copy.
+        if !created_here {
+            return need_external.into_iter().collect();
+        }
     }
     for row in work {
         for inp in row.ins.iter() {
-            if inp.is_coinbase {
+            if inp.is_coinbase || inp.prev_txid == [0u8; 32] {
                 continue;
             }
-            if batch_map.contains_key(&inp.prev_txid) || inp.prev_txid == [0u8; 32] {
+            if batch_copy_before(
+                batch_map.get(&inp.prev_txid).copied(),
+                batch_repeats.get(&inp.prev_txid).map(Vec::as_slice),
+                row.tx_fk,
+            )
+            .is_some()
+            {
+                continue;
+            }
+            if batch_map.contains_key(&inp.prev_txid) && !outside(&inp.prev_txid) {
                 continue;
             }
             need_external.insert(inp.prev_txid);
@@ -1069,8 +1108,8 @@ impl Query {
     ///
     /// Does not build [`TxApply`]. `body_est` uses packed encoded lengths.
     /// Same txid in one block is Corrupt. Same txid across headers in the wave
-    /// is BIP30 (91842/91880): both rows are planned; `batch_map` keeps the
-    /// later fk.
+    /// is BIP30 (91842/91880, or regtest below BIP34): both rows are planned,
+    /// and a spend uses the newest copy created before it.
     pub fn archive_plan_batch_from_wire(
         &self,
         need: &[WirePlanNeed<'_>],
@@ -1091,6 +1130,7 @@ impl Query {
 
         let t_assign = Instant::now();
         let mut batch_map: crate::TxidMap<Fk> = crate::TxidMap::default();
+        let mut batch_repeats: crate::TxidMap<Vec<Fk>> = crate::TxidMap::default();
         let mut work: Vec<PlanRow> = Vec::new();
         let mut per_header_ranges: Vec<(Fk, Fk, u32)> = Vec::with_capacity(need.len());
 
@@ -1113,7 +1153,12 @@ impl Query {
                 }
                 let tx_fk = Fk(next_tx);
                 next_tx += 1;
-                batch_map.insert(*txid, tx_fk);
+                if let Some(prev) = batch_map.insert(*txid, tx_fk) {
+                    batch_repeats
+                        .entry(*txid)
+                        .or_insert_with(|| vec![prev])
+                        .push(tx_fk);
+                }
                 let rec = tx_record_from_wire(tx, *txid);
                 let ins: Vec<PlanIn> = tx.input.iter().map(plan_in_from_txin).collect();
                 work.push(PlanRow {
@@ -1130,6 +1175,7 @@ impl Query {
         let plan = self.finish_archive_plan(
             work,
             batch_map,
+            batch_repeats,
             per_header_ranges,
             n_headers,
             assign_ns,
@@ -1145,6 +1191,7 @@ impl Query {
         &self,
         work: Vec<PlanRow>,
         batch_map: crate::TxidMap<Fk>,
+        batch_repeats: crate::TxidMap<Vec<Fk>>,
         per_header_ranges: Vec<(Fk, Fk, u32)>,
         n_headers: u64,
         assign_ns: u64,
@@ -1157,7 +1204,12 @@ impl Query {
         let index_tx = self.tx_index_enabled();
 
         let t_collect = Instant::now();
-        let need_vec = collect_plan_need_external(&work, &batch_map, carried_need);
+        // With a skeleton, a key the wave did not resolve is Corrupt, so ask
+        // only for what in-flight or the skeleton holds.
+        let need_vec =
+            collect_plan_need_external(&work, &batch_map, &batch_repeats, carried_need, |t| {
+                skeleton.is_none_or(|s| s.get(t).is_some()) || in_flight.get_create_fk(t).is_some()
+            });
         let collect_ns = t_collect.elapsed().as_nanos() as u64;
 
         let ext = crate::stamp_external_parents(
@@ -1209,12 +1261,20 @@ impl Query {
                     });
                     continue;
                 }
-                let create_fk = if let Some(&cfk) = batch_map.get(&inp.prev_txid) {
+                let create_fk = if let Some(cfk) = batch_copy_before(
+                    batch_map.get(&inp.prev_txid).copied(),
+                    batch_repeats.get(&inp.prev_txid).map(Vec::as_slice),
+                    tx_fk,
+                ) {
                     batch_stamp = batch_stamp.saturating_add(1);
                     cfk
                 } else if let Some(&cfk) = resolved.get(&inp.prev_txid) {
                     resolved_stamp = resolved_stamp.saturating_add(1);
                     cfk
+                } else if let Some(&cfk) = batch_map.get(&inp.prev_txid) {
+                    // Only a later copy here: not a coin yet; structural rejects it.
+                    batch_stamp = batch_stamp.saturating_add(1);
+                    cfk
                 } else {
                     return Err(StoreError::Corrupt(
                         "archive: parent create_fk unresolved (contiguous batch required)",
@@ -2729,6 +2789,30 @@ mod tests {
         let _ = std::fs::remove_dir_all(&dir);
     }
 
+    /// A spend sitting between two copies of one txid must bind the copy that
+    /// already exists, not the copy this wave creates later.
+    #[test]
+    fn plan_batch_spend_between_repeated_txid_binds_the_earlier_copy() {
+        let (dir, q) = temp_query("bip30-between");
+        let txid = coinbase_apply(1).tx.txid;
+        let child = child_spend(txid, 0x99);
+        let need = vec![
+            (Fk(1), vec![coinbase_apply(1)]),
+            (Fk(2), vec![child]),
+            (Fk(3), vec![coinbase_apply(1)]),
+        ];
+        let plan = plan_applies(&q, &need, 1, &crate::InFlight::new(), None)
+            .expect("spend between two copies of one txid");
+        let spend = plan.edges.get(&2).expect("child");
+        assert_eq!(
+            spend[0].create_fk,
+            Fk(1),
+            "spend between two copies binds the earlier one, got {:?}",
+            spend[0].create_fk
+        );
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
     #[test]
     fn plan_batch_from_wire_duplicate_txid_in_one_block_is_corrupt() {
         let (dir, q) = temp_query("dup-txid-in-block");

crates/rbitcoin-query/src/block_pipeline.rs

@@ -231,6 +231,7 @@ pub fn assemble_block_prevouts(
         if !create_fk.is_null() {
             pending_creates.insert(txid, create_fk);
         }
+        release_repeated_outputs(pending_spent, txid, tx.output.len());
     }
 
     let clk_prev = t_loop.elapsed().as_nanos() as u64;
@@ -617,7 +618,7 @@ pub fn structural_validate_spends(
         u32,
     )],
     fees: i64,
-    pending_spent: &mut rbitcoin_consensus::OutPointSet,
+    pending_spent: &mut BatchSpent,
     batch_parents: &crate::BatchParents,
     mtp_cache: &mut U32Map<u32>,
     run_create_height: &RunCreateHeight,
@@ -631,7 +632,7 @@ pub fn structural_validate_spends(
     let maturity = ctx.params.coinbase_maturity();
     // BIP30's txid batch is inside `spent_ns` (signet runs it on every block).
     let t_spent = Instant::now();
-    reject_bip30_unspent_overwrite(query, block, ctx, pending_spent, scratch)?;
+    reject_bip30_unspent_overwrite(query, block, ctx, pending_spent, run_create_height, scratch)?;
     let t_abs = Instant::now();
     structural_abs_heights(query, run_create_height, scratch, abs_jobs)?;
     let tip = query.tip_height().map(|h| h.0);
@@ -646,7 +647,12 @@ pub fn structural_validate_spends(
     let spent_abs_ns = (t_abs.elapsed().as_nanos() as u64).saturating_sub(spent_strong_ns);
     let spent_cold_ns = multi_list_ns;
     let t_pending = Instant::now();
-    structural_mark_pending(spends, pending_spent, &scratch.durable_spent)?;
+    structural_mark_pending(
+        spends,
+        pending_spent,
+        run_create_height,
+        &scratch.durable_spent,
+    )?;
     let spent_pending_ns = t_pending.elapsed().as_nanos() as u64;
     let spent_ns = t_spent.elapsed().as_nanos() as u64;
     let t_create = Instant::now();
@@ -1008,10 +1014,11 @@ fn structural_mark_pending(
         rbitcoin_primitives::Fk,
         u32,
     )],
-    pending_spent: &mut rbitcoin_consensus::OutPointSet,
+    pending_spent: &mut BatchSpent,
+    run_create_height: &RunCreateHeight,
     durable_spent: &DurableSpentSet,
 ) -> Result<(), ConfirmError> {
-    for &(prev_txid, vout, _spend_fk, create_fk, _vin) in spends {
+    for &(prev_txid, vout, spend_fk, create_fk, _vin) in spends {
         let key = (prev_txid, vout);
         let spent = if create_fk.is_null() {
             false
@@ -1020,7 +1027,17 @@ fn structural_mark_pending(
         } else {
             false
         };
-        rbitcoin_consensus::block::reject_prevout_spent(spent || !pending_spent.insert(key))?;
+        // A same-block create has no fk here. Its spender's fk has the same
+        // height in this batch, which is what identifies the copy.
+        let copy = if create_fk.is_null() {
+            spend_fk
+        } else {
+            create_fk
+        };
+        let again = pending_spent
+            .insert(key, copy)
+            .is_some_and(|prev| same_copy(run_create_height, prev, copy));
+        rbitcoin_consensus::block::reject_prevout_spent(spent || again)?;
     }
     Ok(())
 }
@@ -1172,7 +1189,8 @@ fn reject_bip30_unspent_overwrite(
     query: &Query,
     block: &Block,
     ctx: &rbitcoin_consensus::ValidationContext<'_>,
-    pending_spent: &rbitcoin_consensus::OutPointSet,
+    pending_spent: &BatchSpent,
+    run_create_height: &RunCreateHeight,
     scratch: &mut StructuralScratch,
 ) -> Result<(), ConfirmError> {
     if ctx.params.is_bip30_repeat(ctx.height.0, block.block_hash())
@@ -1193,14 +1211,19 @@ fn reject_bip30_unspent_overwrite(
         };
         let unspent = scratch.batch_vouts[lo as usize..hi as usize]
             .iter()
-            .any(|&v| !pending_spent.contains(&(*txid, v)));
+            .any(|&v| {
+                pending_spent
+                    .get(&(*txid, v))
+                    .and_then(|&copy| run_create_height.get(copy))
+                    != Some(prior_height)
+            });
         rbitcoin_consensus::block::reject_bip30_unspent_prior(ctx.height.0, prior_height, unspent)?;
     }
     let hits = query
         .store()
         .get_fk_by_txid_batch(&create_txids)
         .map_err(ConfirmError::from)?;
-    for (_txid, row) in hits {
+    for (txid, row) in hits {
         let Some((old_fk, _)) = row else {
             continue;
         };
@@ -1224,10 +1247,13 @@ fn reject_bip30_unspent_overwrite(
             if rbitcoin_consensus::policy::is_unspendable(&out.script) {
                 continue;
             }
-            let spent = query
-                .store()
-                .has_confirmed_strong_spender_create(old_fk, v, None)
-                .map_err(ConfirmError::from)?;
+            // A spend earlier in this batch is not connected yet either. It
+            // spent this copy, or a later one that needed this copy spent.
+            let spent = pending_spent.contains_key(&(txid, v))
+                || query
+                    .store()
+                    .has_confirmed_strong_spender_create(old_fk, v, None)
+                    .map_err(ConfirmError::from)?;
             if !spent {
                 unspent = true;
                 break;
@@ -1257,6 +1283,43 @@ fn reject_bip30_unspent_overwrite(
     Ok(())
 }
 
+/// Outpoints spent so far in one write batch, each with the copy the spend used:
+/// its create fk, or for a same-block create the spender's fk. A txid can be
+/// created again once its first copy is spent (below BIP34, or a pre-BIP34
+/// coinbase whose scriptSig already pushes a later height), so a spend of the
+/// new copy is not a second spend of the old one.
+pub(crate) type BatchSpent = std::collections::HashMap<
+    ([u8; 32], u32),
+    rbitcoin_primitives::Fk,
+    BuildHasherDefault<rbitcoin_consensus::OutPointHasher>,
+>;
+
+/// One block holds one copy of a txid, so two copies created in this batch are
+/// the same copy when they have the same height.
+fn same_copy(
+    run_create_height: &RunCreateHeight,
+    a: rbitcoin_primitives::Fk,
+    b: rbitcoin_primitives::Fk,
+) -> bool {
+    a == b
+        || run_create_height
+            .get(a)
+            .is_some_and(|h| run_create_height.get(b) == Some(h))
+}
+
+/// A txid can be created again once its first copy is spent (see [`BatchSpent`]).
+/// Its outputs are coins again (Core overwrites them in the view), so this
+/// load batch's earlier spends of the old copy no longer block the new one.
+fn release_repeated_outputs(
+    pending_spent: &mut rbitcoin_consensus::OutPointSet,
+    txid: [u8; 32],
+    n_out: usize,
+) {
+    for v in 0..n_out as u32 {
+        pending_spent.remove(&(txid, v));
+    }
+}
+
 fn bip34_ancestry_skips_bip30(
     query: &Query,
     ctx: &rbitcoin_consensus::ValidationContext<'_>,