Merge #1917: Canonicalization should handle transactions that spend from two conflicting transactions

370497c fix(chain): Correctly handle txs that double spend themselves (志宇)

Pull request description:

  Fixes #1898

  ### Description

  When we initially created the canonicalization algorithm, we didn't expect callers to insert invalid transactions into the graph. However, user error happens and we should handle it correctly.

  Before this PR, when inserting transactions that double-spend themselves (where two or more different inputs of the transactions conflict), the canonicalization result will have inconsistencies.

  ### Notes to the reviewers

  Logic changes are all contained in `CanonicalIter::mark_canonical`. `mark_canonical` will detect whether the `tx` being passed in double spends itself. If so, we abort and undo all changes made so far.

  There is a slight <2% degradation in performance with this change (except in two cases where there is a performance improvement of ~10%).

  [bench.txt](https://github.com/user-attachments/files/19788730/bench.txt)

  ### Changelog notice

  * Fix canonicalization mess-up when transactions that conflict with itself are inserted.

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### Bugfixes:

  ~~* [ ] This pull request breaks the existing API~~
  * [x] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  ValuedMammal:
    ACK 370497c
  LagginTimes:
    ACK 370497c

Tree-SHA512: 41b08208b3ca7119ff10898c22f91b1fd41bb83876e4ca0655ece13b43db8cf3963a529286282c5fcd0e94d6ae073ffb30b19e91d28d8459957d5592bbacd3c9

Author: evanlinjin

crates/chain/src/canonical_iter.rs

@@ -1,12 +1,16 @@
-use crate::collections::{hash_map, HashMap, HashSet, VecDeque};
+use crate::collections::{HashMap, HashSet, VecDeque};
 use crate::tx_graph::{TxAncestors, TxDescendants};
 use crate::{Anchor, ChainOracle, TxGraph};
 use alloc::boxed::Box;
 use alloc::collections::BTreeSet;
 use alloc::sync::Arc;
+use alloc::vec::Vec;
 use bdk_core::BlockId;
 use bitcoin::{Transaction, Txid};
 
+type CanonicalMap<A> = HashMap<Txid, (Arc<Transaction>, CanonicalReason<A>)>;
+type NotCanonicalSet = HashSet<Txid>;
+
 /// Iterates over canonical txs.
 pub struct CanonicalIter<'g, A, C> {
     tx_graph: &'g TxGraph<A>,
@@ -18,8 +22,8 @@ pub struct CanonicalIter<'g, A, C> {
     unprocessed_txs_with_last_seens: Box<dyn Iterator<Item = (Txid, Arc<Transaction>, u64)> + 'g>,
     unprocessed_txs_left_over: VecDeque<(Txid, Arc<Transaction>, u32)>,
 
-    canonical: HashMap<Txid, (Arc<Transaction>, CanonicalReason<A>)>,
-    not_canonical: HashSet<Txid>,
+    canonical: CanonicalMap<A>,
+    not_canonical: NotCanonicalSet,
 
     queue: VecDeque<Txid>,
 }
@@ -87,27 +91,49 @@ impl<'g, A: Anchor, C: ChainOracle> CanonicalIter<'g, A, C> {
         Ok(())
     }
 
-    /// Marks a transaction and it's ancestors as canonical. Mark all conflicts of these as
+    /// Marks `tx` and it's ancestors as canonical and mark all conflicts of these as
     /// `not_canonical`.
+    ///
+    /// The exception is when it is discovered that `tx` double spends itself (i.e. two of it's
+    /// inputs conflict with each other), then no changes will be made.
+    ///
+    /// The logic works by having two loops where one is nested in another.
+    /// * The outer loop iterates through ancestors of `tx` (including `tx`). We can transitively
+    ///   assume that all ancestors of `tx` are also canonical.
+    /// * The inner loop loops through conflicts of ancestors of `tx`. Any descendants of conflicts
+    ///   are also conflicts and are transitively considered non-canonical.
+    ///
+    /// If the inner loop ends up marking `tx` as non-canonical, then we know that it double spends
+    /// itself.
     fn mark_canonical(&mut self, txid: Txid, tx: Arc<Transaction>, reason: CanonicalReason<A>) {
         let starting_txid = txid;
-        let mut is_root = true;
-        TxAncestors::new_include_root(
+        let mut is_starting_tx = true;
+
+        // We keep track of changes made so far so that we can undo it later in case we detect that
+        // `tx` double spends itself.
+        let mut detected_self_double_spend = false;
+        let mut undo_not_canonical = Vec::<Txid>::new();
+
+        // `staged_queue` doubles as the `undo_canonical` data.
+        let staged_queue = TxAncestors::new_include_root(
             self.tx_graph,
             tx,
-            |_: usize, tx: Arc<Transaction>| -> Option<()> {
+            |_: usize, tx: Arc<Transaction>| -> Option<Txid> {
                 let this_txid = tx.compute_txid();
-                let this_reason = if is_root {
-                    is_root = false;
+                let this_reason = if is_starting_tx {
+                    is_starting_tx = false;
                     reason.clone()
                 } else {
                     reason.to_transitive(starting_txid)
                 };
+
+                use crate::collections::hash_map::Entry;
                 let canonical_entry = match self.canonical.entry(this_txid) {
                     // Already visited tx before, exit early.
-                    hash_map::Entry::Occupied(_) => return None,
-                    hash_map::Entry::Vacant(entry) => entry,
+                    Entry::Occupied(_) => return None,
+                    Entry::Vacant(entry) => entry,
                 };
+
                 // Any conflicts with a canonical tx can be added to `not_canonical`. Descendants
                 // of `not_canonical` txs can also be added to `not_canonical`.
                 for (_, conflict_txid) in self.tx_graph.direct_conflicts(&tx) {
@@ -116,6 +142,7 @@ impl<'g, A: Anchor, C: ChainOracle> CanonicalIter<'g, A, C> {
                         conflict_txid,
                         |_: usize, txid: Txid| -> Option<()> {
                             if self.not_canonical.insert(txid) {
+                                undo_not_canonical.push(txid);
                                 Some(())
                             } else {
                                 None
@@ -124,12 +151,28 @@ impl<'g, A: Anchor, C: ChainOracle> CanonicalIter<'g, A, C> {
                     )
                     .run_until_finished()
                 }
+
+                if self.not_canonical.contains(&this_txid) {
+                    // Early exit if self-double-spend is detected.
+                    detected_self_double_spend = true;
+                    return None;
+                }
                 canonical_entry.insert((tx, this_reason));
-                self.queue.push_back(this_txid);
-                Some(())
+                Some(this_txid)
             },
         )
-        .run_until_finished()
+        .collect::<Vec<Txid>>();
+
+        if detected_self_double_spend {
+            for txid in staged_queue {
+                self.canonical.remove(&txid);
+            }
+            for txid in undo_not_canonical {
+                self.not_canonical.remove(&txid);
+            }
+        } else {
+            self.queue.extend(staged_queue);
+        }
     }
 }
 

crates/chain/tests/test_tx_graph_conflicts.rs

@@ -686,7 +686,111 @@ fn test_tx_conflict_handling() {
             exp_chain_txouts: HashSet::from([("tx", 0)]),
             exp_unspents: HashSet::from([("tx", 0)]),
             exp_balance: Balance { trusted_pending: Amount::from_sat(9000), ..Default::default() }
-        }
+        },
+        Scenario {
+            name: "tx spends from 2 conflicting transactions where a conflict spends another",
+            tx_templates: &[
+                TxTemplate {
+                    tx_name: "A",
+                    inputs: &[TxInTemplate::Bogus],
+                    outputs: &[TxOutTemplate::new(10_000, None)],
+                    last_seen: Some(1),
+                    ..Default::default()
+                },
+                TxTemplate {
+                    tx_name: "S1",
+                    inputs: &[TxInTemplate::PrevTx("A", 0)],
+                    outputs: &[TxOutTemplate::new(9_000, None)],
+                    last_seen: Some(2),
+                    ..Default::default()
+                },
+                TxTemplate {
+                    tx_name: "S2",
+                    inputs: &[TxInTemplate::PrevTx("A", 0), TxInTemplate::PrevTx("S1", 0)],
+                    outputs: &[TxOutTemplate::new(17_000, None)],
+                    last_seen: Some(3),
+                    ..Default::default()
+                },
+            ],
+            exp_chain_txs: HashSet::from(["A", "S1"]),
+            exp_chain_txouts: HashSet::from([]),
+            exp_unspents: HashSet::from([]),
+            exp_balance: Balance::default(),
+        },
+        Scenario {
+            name: "tx spends from 2 conflicting transactions where the conflict is nested",
+            tx_templates: &[
+                TxTemplate {
+                    tx_name: "A",
+                    inputs: &[TxInTemplate::Bogus],
+                    outputs: &[TxOutTemplate::new(10_000, Some(0))],
+                    last_seen: Some(1),
+                    ..Default::default()
+                },
+                TxTemplate {
+                    tx_name: "S1",
+                    inputs: &[TxInTemplate::PrevTx("A", 0)],
+                    outputs: &[TxOutTemplate::new(9_000, Some(0))],
+                    last_seen: Some(3),
+                    ..Default::default()
+                },
+                TxTemplate {
+                    tx_name: "B",
+                    inputs: &[TxInTemplate::PrevTx("S1", 0)],
+                    outputs: &[TxOutTemplate::new(8_000, Some(0))],
+                    last_seen: Some(2),
+                    ..Default::default()
+                },
+                TxTemplate {
+                    tx_name: "S2",
+                    inputs: &[TxInTemplate::PrevTx("B", 0), TxInTemplate::PrevTx("A", 0)],
+                    outputs: &[TxOutTemplate::new(17_000, Some(0))],
+                    last_seen: Some(4),
+                    ..Default::default()
+                },
+            ],
+            exp_chain_txs: HashSet::from(["A", "S1", "B"]),
+            exp_chain_txouts: HashSet::from([("A", 0), ("B", 0), ("S1", 0)]),
+            exp_unspents: HashSet::from([("B", 0)]),
+            exp_balance: Balance { trusted_pending: Amount::from_sat(8_000), ..Default::default() },
+        },
+        Scenario {
+            name: "tx spends from 2 conflicting transactions where the conflict is nested (different last_seens)",
+            tx_templates: &[
+                TxTemplate {
+                    tx_name: "A",
+                    inputs: &[TxInTemplate::Bogus],
+                    outputs: &[TxOutTemplate::new(10_000, Some(0))],
+                    last_seen: Some(1),
+                    ..Default::default()
+                },
+                TxTemplate {
+                    tx_name: "S1",
+                    inputs: &[TxInTemplate::PrevTx("A", 0)],
+                    outputs: &[TxOutTemplate::new(9_000, Some(0))],
+                    last_seen: Some(4),
+                    ..Default::default()
+                },
+                TxTemplate {
+                    tx_name: "B",
+                    inputs: &[TxInTemplate::PrevTx("S1", 0)],
+                    outputs: &[TxOutTemplate::new(8_000, Some(0))],
+                    last_seen: Some(2),
+                    ..Default::default()
+                },
+                TxTemplate {
+                    tx_name: "S2",
+                    inputs: &[TxInTemplate::PrevTx("A", 0), TxInTemplate::PrevTx("B", 0)],
+                    outputs: &[TxOutTemplate::new(17_000, Some(0))],
+                    last_seen: Some(3),
+                    ..Default::default()
+                },
+            ],
+            exp_chain_txs: HashSet::from(["A", "S1", "B"]),
+            exp_chain_txouts: HashSet::from([("A", 0), ("B", 0), ("S1", 0)]),
+            exp_unspents: HashSet::from([("B", 0)]),
+            exp_balance: Balance { trusted_pending: Amount::from_sat(8_000), ..Default::default() },
+        },
     ];
 
     for scenario in scenarios {