Merge #2027: feat(chain): add new list_ordered_canonical_txs method

12c1076 feat(chain): introduce new `list_ordered_canonical_txs` (Leonardo Lima)
f34117b test(chain): add `test_list_ordered_canonical_txs` (Leonardo Lima)
a201be0 chore(test): rename `txid_to_name` to proper `tx_name_to_txid` (Leonardo Lima)
6cf720e chore(docs): fix typo in docs (Leonardo Lima)

Pull request description:

  ### Description

  It introduces a new method for topological ordering canonical transactions, `list_ordered_canonical_txs`. It now ensures the dependency-based transaction processing, guaranteeing that parent transactions always appears before their children transaction.

  The existing `list_canonical_txs` and `try_list_canonical_txs` methods have been deprecated in favor of the new ordered version.

  ### Notes to the reviewers

  "[...] For those reviewing, and wondering why we don't have a fallible try version of this method, it's because we don't have a fallible ChainOracle implementation - we will get rid of ChainOracle trait soon anyway."

  This PR is intended for a point release so that bdk_wallet 2.x users can get a topologically sorted list of transactions (we need a point release on bdk_wallet 2.x as well).

  It might be useful to take a look at the new test scenarios that I've added, it shows some specific scenarios where the current implementation and output of `canonical_txs` didn't output the transactions in topological order.

  Let me know if you think the TopologicalIter algorithm and/or API could be improved.

  ### Changelog notice
  ```
  #### Added
  - New `list_ordered_canonical_txs` method to `TxGraph` that returns canonical transactions in topological order, ensuring parent transactions always appear before their children

  #### Deprecated
  - `list_canonical_txs` method - use `list_ordered_canonical_txs` instead for guaranteed topological ordering
  - `try_list_canonical_txs` method - use `list_ordered_canonical_txs` instead for guaranteed topological ordering
  ```

  ### Checklists

  #### All Submissions:

  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)

  #### New Features:

  * [x] I've added tests for the new feature
  * [x] I've added docs for the new feature

ACKs for top commit:
  ValuedMammal:
    ACK 12c1076

Tree-SHA512: 73c8cc40e6b77844af52ddc1a788cf1190357e08f487f443ab492881e50ed4343598e2607f542e7d9b69615cd102f29ec3bda5942753186860d919433b1d62dd

Author: ValuedMammal

crates/chain/benches/canonicalization.rs

@@ -99,6 +99,15 @@ fn run_list_canonical_txs(tx_graph: &KeychainTxGraph, chain: &LocalChain, exp_tx
     assert_eq!(txs.count(), exp_txs);
 }
 
+fn run_list_ordered_canonical_txs(tx_graph: &KeychainTxGraph, chain: &LocalChain, exp_txs: usize) {
+    let txs = tx_graph.graph().list_ordered_canonical_txs(
+        chain,
+        chain.tip().block_id(),
+        CanonicalizationParams::default(),
+    );
+    assert_eq!(txs.count(), exp_txs);
+}
+
 fn run_filter_chain_txouts(tx_graph: &KeychainTxGraph, chain: &LocalChain, exp_txos: usize) {
     let utxos = tx_graph.graph().filter_chain_txouts(
         chain,
@@ -147,6 +156,13 @@ pub fn many_conflicting_unconfirmed(c: &mut Criterion) {
         let (tx_graph, chain) = (tx_graph.clone(), chain.clone());
         move |b| b.iter(|| run_list_canonical_txs(&tx_graph, &chain, 2))
     });
+    c.bench_function(
+        "many_conflicting_unconfirmed::list_ordered_canonical_txs",
+        {
+            let (tx_graph, chain) = (tx_graph.clone(), chain.clone());
+            move |b| b.iter(|| run_list_ordered_canonical_txs(&tx_graph, &chain, 2))
+        },
+    );
     c.bench_function("many_conflicting_unconfirmed::filter_chain_txouts", {
         let (tx_graph, chain) = (tx_graph.clone(), chain.clone());
         move |b| b.iter(|| run_filter_chain_txouts(&tx_graph, &chain, 2))
@@ -185,6 +201,10 @@ pub fn many_chained_unconfirmed(c: &mut Criterion) {
         let (tx_graph, chain) = (tx_graph.clone(), chain.clone());
         move |b| b.iter(|| run_list_canonical_txs(&tx_graph, &chain, 2101))
     });
+    c.bench_function("many_chained_unconfirmed::list_ordered_canonical_txs", {
+        let (tx_graph, chain) = (tx_graph.clone(), chain.clone());
+        move |b| b.iter(|| run_list_ordered_canonical_txs(&tx_graph, &chain, 2101))
+    });
     c.bench_function("many_chained_unconfirmed::filter_chain_txouts", {
         let (tx_graph, chain) = (tx_graph.clone(), chain.clone());
         move |b| b.iter(|| run_filter_chain_txouts(&tx_graph, &chain, 1))
@@ -234,6 +254,13 @@ pub fn nested_conflicts(c: &mut Criterion) {
         let (tx_graph, chain) = (tx_graph.clone(), chain.clone());
         move |b| b.iter(|| run_list_canonical_txs(&tx_graph, &chain, GRAPH_DEPTH))
     });
+    c.bench_function(
+        "nested_conflicts_unconfirmed::list_ordered_canonical_txs",
+        {
+            let (tx_graph, chain) = (tx_graph.clone(), chain.clone());
+            move |b| b.iter(|| run_list_ordered_canonical_txs(&tx_graph, &chain, GRAPH_DEPTH))
+        },
+    );
     c.bench_function("nested_conflicts_unconfirmed::filter_chain_txouts", {
         let (tx_graph, chain) = (tx_graph.clone(), chain.clone());
         move |b| b.iter(|| run_filter_chain_txouts(&tx_graph, &chain, GRAPH_DEPTH))

crates/chain/src/canonical_iter.rs

@@ -1,5 +1,5 @@
 use crate::collections::{HashMap, HashSet, VecDeque};
-use crate::tx_graph::{TxAncestors, TxDescendants};
+use crate::tx_graph::{CanonicalTx, TxAncestors, TxDescendants};
 use crate::{Anchor, ChainOracle, TxGraph};
 use alloc::boxed::Box;
 use alloc::collections::BTreeSet;
@@ -14,7 +14,7 @@ type NotCanonicalSet = HashSet<Txid>;
 /// Modifies the canonicalization algorithm.
 #[derive(Debug, Default, Clone)]
 pub struct CanonicalizationParams {
-    /// Transactions that will supercede all other transactions.
+    /// Transactions that will supersede all other transactions.
     ///
     /// In case of conflicting transactions within `assume_canonical`, transactions that appear
     /// later in the list (have higher index) have precedence.
@@ -108,7 +108,7 @@ impl<'g, A: Anchor, C: ChainOracle> CanonicalIter<'g, A, C> {
                 .iter()
                 .last()
                 .expect(
-                    "tx taken from `unprocessed_txs_with_anchors` so it must atleast have an anchor",
+                    "tx taken from `unprocessed_txs_with_anchors` so it must at least have an anchor",
                 )
                 .confirmation_height_upper_bound(),
         ));
@@ -266,7 +266,7 @@ pub enum ObservedIn {
 /// The reason why a transaction is canonical.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum CanonicalReason<A> {
-    /// This transaction is explicitly assumed to be canonical by the caller, superceding all other
+    /// This transaction is explicitly assumed to be canonical by the caller, superseding all other
     /// canonicalization rules.
     Assumed {
         /// Whether it is a descendant that is assumed to be canonical.
@@ -290,7 +290,7 @@ pub enum CanonicalReason<A> {
 }
 
 impl<A: Clone> CanonicalReason<A> {
-    /// Constructs a [`CanonicalReason`] for a transaction that is assumed to supercede all other
+    /// Constructs a [`CanonicalReason`] for a transaction that is assumed to supersede all other
     /// transactions.
     pub fn assumed() -> Self {
         Self::Assumed { descendant: None }
@@ -312,7 +312,7 @@ impl<A: Clone> CanonicalReason<A> {
         }
     }
 
-    /// Contruct a new [`CanonicalReason`] from the original which is transitive to `descendant`.
+    /// Construct a new [`CanonicalReason`] from the original which is transitive to `descendant`.
     ///
     /// This signals that either the [`ObservedIn`] or [`Anchor`] value belongs to the transaction's
     /// descendant, but is transitively relevant.
@@ -342,3 +342,157 @@ impl<A: Clone> CanonicalReason<A> {
         }
     }
 }
+
+/// Iterator based on the Kahn's Algorithm, that yields transactions in topological spending order
+/// in depth, and properly sorted with level.
+///
+/// NOTE: Please refer to the Kahn's Algorithm reference: https://dl.acm.org/doi/pdf/10.1145/368996.369025
+pub(crate) struct TopologicalIterator<'a, A> {
+    /// Map of txid to its canonical transaction
+    canonical_txs: HashMap<Txid, CanonicalTx<'a, Arc<Transaction>, A>>,
+
+    /// Current level of transactions to process
+    current_level: Vec<Txid>,
+    /// Next level of transactions to process
+    next_level: Vec<Txid>,
+
+    /// Adjacency list: parent txid -> list of children txids
+    children_map: HashMap<Txid, Vec<Txid>>,
+    /// Number of unprocessed parents for each transaction
+    parent_count: HashMap<Txid, usize>,
+
+    /// Current index in the current level
+    current_index: usize,
+}
+
+impl<'a, A: Clone + Anchor> TopologicalIterator<'a, A> {
+    /// Constructs [`TopologicalIterator`] from a list of `canonical_txs` (e.g [`CanonicalIter`]),
+    /// in order to handle all the graph building internally.
+    pub(crate) fn new(
+        canonical_txs: impl Iterator<Item = CanonicalTx<'a, Arc<Transaction>, A>>,
+    ) -> Self {
+        // Build a map from txid to canonical tx for quick lookup
+        let mut tx_map: HashMap<Txid, CanonicalTx<'a, Arc<Transaction>, A>> = HashMap::new();
+        let mut canonical_set: HashSet<Txid> = HashSet::new();
+
+        for canonical_tx in canonical_txs {
+            let txid = canonical_tx.tx_node.txid;
+            canonical_set.insert(txid);
+            tx_map.insert(txid, canonical_tx);
+        }
+
+        // Build the dependency graph (txid -> parents it depends on)
+        let mut dependencies: HashMap<Txid, Vec<Txid>> = HashMap::new();
+        let mut has_parents: HashSet<Txid> = HashSet::new();
+
+        for &txid in canonical_set.iter() {
+            let canonical_tx = tx_map.get(&txid).expect("txid must exist in map");
+            let tx = &canonical_tx.tx_node.tx;
+
+            // Find all parents (transactions this one depends on)
+            let mut parents = Vec::new();
+            if !tx.is_coinbase() {
+                for txin in &tx.input {
+                    let parent_txid = txin.previous_output.txid;
+                    // Only include if the parent is also canonical
+                    if canonical_set.contains(&parent_txid) {
+                        parents.push(parent_txid);
+                        has_parents.insert(txid);
+                    }
+                }
+            }
+
+            if !parents.is_empty() {
+                dependencies.insert(txid, parents);
+            }
+        }
+
+        // Build adjacency list and parent counts for traversal
+        let mut parent_count = HashMap::new();
+        let mut children_map: HashMap<Txid, Vec<Txid>> = HashMap::new();
+
+        for (txid, parents) in &dependencies {
+            for parent_txid in parents {
+                children_map.entry(*parent_txid).or_default().push(*txid);
+                *parent_count.entry(*txid).or_insert(0) += 1;
+            }
+        }
+
+        // Find root transactions (those with no parents in the canonical set)
+        let roots: Vec<Txid> = canonical_set
+            .iter()
+            .filter(|&&txid| !has_parents.contains(&txid))
+            .copied()
+            .collect();
+
+        // Sort the initial level
+        let mut current_level = roots;
+        Self::sort_level_by_chain_position(&mut current_level, &tx_map);
+
+        Self {
+            canonical_txs: tx_map,
+            current_level,
+            next_level: Vec::new(),
+            children_map,
+            parent_count,
+            current_index: 0,
+        }
+    }
+
+    /// Sort transactions within a level by their chain position
+    /// Confirmed transactions come first (sorted by height), then unconfirmed (sorted by last_seen)
+    fn sort_level_by_chain_position(
+        level: &mut [Txid],
+        canonical_txs: &HashMap<Txid, CanonicalTx<'a, Arc<Transaction>, A>>,
+    ) {
+        level.sort_by(|&a_txid, &b_txid| {
+            let a_tx = canonical_txs.get(&a_txid).expect("txid must exist");
+            let b_tx = canonical_txs.get(&b_txid).expect("txid must exist");
+
+            a_tx.cmp(b_tx)
+        });
+    }
+
+    fn advance_to_next_level(&mut self) {
+        self.current_level = core::mem::take(&mut self.next_level);
+        Self::sort_level_by_chain_position(&mut self.current_level, &self.canonical_txs);
+        self.current_index = 0;
+    }
+}
+
+impl<'a, A: Clone + Anchor> Iterator for TopologicalIterator<'a, A> {
+    type Item = CanonicalTx<'a, Arc<Transaction>, A>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        // If we've exhausted the current level, move to next
+        if self.current_index >= self.current_level.len() {
+            if self.next_level.is_empty() {
+                return None;
+            }
+            self.advance_to_next_level();
+        }
+
+        let current_txid = self.current_level[self.current_index];
+        self.current_index += 1;
+
+        // If this is the last item in current level, prepare dependents for next level
+        if self.current_index == self.current_level.len() {
+            // Process all dependents of all transactions in current level
+            for &tx in &self.current_level {
+                if let Some(children) = self.children_map.get(&tx) {
+                    for &child in children {
+                        if let Some(count) = self.parent_count.get_mut(&child) {
+                            *count -= 1;
+                            if *count == 0 {
+                                self.next_level.push(child);
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
+        // Return the CanonicalTx for the current txid
+        self.canonical_txs.get(&current_txid).cloned()
+    }
+}

crates/chain/src/tx_graph.rs

@@ -988,6 +988,9 @@ impl<A: Anchor> TxGraph<A> {
     /// Each transaction is represented as a [`CanonicalTx`] that contains where the transaction is
     /// observed in-chain, and the [`TxNode`].
     ///
+    /// NOTE: It does not guarantee the topological order of yielded transactions, the
+    /// [`list_ordered_canonical_txs`] can be used instead.
+    ///
     /// # Error
     ///
     /// If the [`ChainOracle`] implementation (`chain`) fails, an error will be returned with the
@@ -996,6 +999,7 @@ impl<A: Anchor> TxGraph<A> {
     /// If the [`ChainOracle`] is infallible, [`list_canonical_txs`] can be used instead.
     ///
     /// [`list_canonical_txs`]: Self::list_canonical_txs
+    /// [`list_ordered_canonical_txs`]: Self::list_ordered_canonical_txs
     pub fn try_list_canonical_txs<'a, C: ChainOracle + 'a>(
         &'a self,
         chain: &'a C,
@@ -1079,7 +1083,11 @@ impl<A: Anchor> TxGraph<A> {
     ///
     /// This is the infallible version of [`try_list_canonical_txs`].
     ///
+    /// NOTE: It does not guarantee the topological order of yielded transactions, the
+    /// [`list_ordered_canonical_txs`] can be used instead.
+    ///
     /// [`try_list_canonical_txs`]: Self::try_list_canonical_txs
+    /// [`list_ordered_canonical_txs`]: Self::list_ordered_canonical_txs
     pub fn list_canonical_txs<'a, C: ChainOracle<Error = Infallible> + 'a>(
         &'a self,
         chain: &'a C,
@@ -1090,6 +1098,28 @@ impl<A: Anchor> TxGraph<A> {
             .map(|res| res.expect("infallible"))
     }
 
+    /// List graph transactions that are in `chain` with `chain_tip` in topological order.
+    ///
+    /// Each transaction is represented as a [`CanonicalTx`] that contains where the transaction is
+    /// observed in-chain, and the [`TxNode`].
+    ///
+    /// Transactions are returned in topological spending order, meaning that if transaction B
+    /// spends from transaction A, then A will always appear before B in the resulting list.
+    ///
+    /// This is the infallible version which uses [`list_canonical_txs`] internally and then
+    /// reorders the transactions based on their spending relationships.
+    ///
+    /// [`list_canonical_txs`]: Self::list_canonical_txs
+    pub fn list_ordered_canonical_txs<'a, C: ChainOracle<Error = Infallible>>(
+        &'a self,
+        chain: &'a C,
+        chain_tip: BlockId,
+        params: CanonicalizationParams,
+    ) -> impl Iterator<Item = CanonicalTx<'a, Arc<Transaction>, A>> {
+        use crate::canonical_iter::TopologicalIterator;
+        TopologicalIterator::new(self.list_canonical_txs(chain, chain_tip, params))
+    }
+
     /// Get a filtered list of outputs from the given `outpoints` that are in `chain` with
     /// `chain_tip`.
     ///
@@ -1118,6 +1148,7 @@ impl<A: Anchor> TxGraph<A> {
     ) -> Result<impl Iterator<Item = (OI, FullTxOut<A>)> + 'a, C::Error> {
         let mut canon_txs = HashMap::<Txid, CanonicalTx<Arc<Transaction>, A>>::new();
         let mut canon_spends = HashMap::<OutPoint, Txid>::new();
+
         for r in self.try_list_canonical_txs(chain, chain_tip, params) {
             let canonical_tx = r?;
             let txid = canonical_tx.tx_node.txid;
@@ -1418,6 +1449,7 @@ impl<A: Anchor> TxGraph<A> {
         I: fmt::Debug + Clone + Ord + 'a,
     {
         let indexer = indexer.as_ref();
+
         self.try_list_canonical_txs(chain, chain_tip, CanonicalizationParams::default())
             .flat_map(move |res| -> Vec<Result<(ScriptBuf, Txid), C::Error>> {
                 let range = &spk_index_range;

crates/chain/tests/common/tx_template.rs

@@ -56,7 +56,7 @@ impl TxOutTemplate {
 pub struct TxTemplateEnv<'a, A> {
     pub tx_graph: TxGraph<A>,
     pub indexer: SpkTxOutIndex<u32>,
-    pub txid_to_name: HashMap<&'a str, Txid>,
+    pub tx_name_to_txid: HashMap<&'a str, Txid>,
     pub canonicalization_params: CanonicalizationParams,
 }
 
@@ -77,7 +77,7 @@ pub fn init_graph<'a, A: Anchor + Clone + 'a>(
                 .script_pubkey(),
         );
     });
-    let mut txid_to_name = HashMap::<&'a str, Txid>::new();
+    let mut tx_name_to_txid = HashMap::<&'a str, Txid>::new();
 
     let mut canonicalization_params = CanonicalizationParams::default();
     for (bogus_txin_vout, tx_tmp) in tx_templates.into_iter().enumerate() {
@@ -108,7 +108,7 @@ pub fn init_graph<'a, A: Anchor + Clone + 'a>(
                         witness: Witness::new(),
                     },
                     TxInTemplate::PrevTx(prev_name, prev_vout) => {
-                        let prev_txid = txid_to_name.get(prev_name).expect(
+                        let prev_txid = tx_name_to_txid.get(prev_name).expect(
                             "txin template must spend from tx of template that comes before",
                         );
                         TxIn {
@@ -140,7 +140,7 @@ pub fn init_graph<'a, A: Anchor + Clone + 'a>(
         if tx_tmp.assume_canonical {
             canonicalization_params.assume_canonical.push(txid);
         }
-        txid_to_name.insert(tx_tmp.tx_name, txid);
+        tx_name_to_txid.insert(tx_tmp.tx_name, txid);
         indexer.scan(&tx);
         let _ = tx_graph.insert_tx(tx.clone());
         for anchor in tx_tmp.anchors.iter() {
@@ -153,7 +153,7 @@ pub fn init_graph<'a, A: Anchor + Clone + 'a>(
     TxTemplateEnv {
         tx_graph,
         indexer,
-        txid_to_name,
+        tx_name_to_txid,
         canonicalization_params,
     }
 }

crates/chain/tests/test_indexed_tx_graph.rs

@@ -782,6 +782,7 @@ fn test_get_chain_position() {
         }
 
         // check chain position
+
         let chain_pos = graph
             .graph()
             .list_canonical_txs(