Merge bitcoindevkit#1586: Change methods of IndexedTxGraph/`TxGraph…

…`/`Wallet` that insert txs to be more generic

87e6121 feat(chain,wallet)!: change methods to take in generic instead of `&Transaction` (志宇)
c39284d feat(wallet): make `Wallet::insert_tx` generic (志宇)
67d5fa6 feat(chain): make various insert tx methods more generic (志宇)

Pull request description:

  ### Description

  We want to reuse the `Arc` pointer whenever possible. However, some methods on `TxGraph` and `IndexedTxGraph` that insert transactions take in `&Transaction` or `Transaction` (thus forcing us to create a new `Arc<Transaction>` internally by cloning, even if the input tx is under a `Arc`).

  This PR changes these methods to take in a generic parameter, `T: Into<Arc<Transaction>>`, allowing us to reuse the `Arc` pointer whenever possible.

  ### Notes to the reviewers

  Methods that previously took in `Transaction` can be changed to take in `T: Into<Arc<Transaction>>` and be non-breaking (since `Arc<T>` impls `From<T>` automatically). These changes are contained in the first two commits.

  Methods that previously took in `&Transaction` will break. However, I think these api changes are small and the improvements are substantial enough to be worth it. These changes are contained in the last commit.

  ### Changelog notice

  * Changed `IndexedTxGraph` methods `insert_tx`, `batch_insert_relevant`, `batch_insert_relevant_unconfirmed`, `batch_insert_unconfirmed` to take in `T: Into<Arc<Transaction>>` instead of `Transaction` or `&Transaction`.
  * Changed `TxGraph` method `batch_insert_unconfirmed` to take in `T: Into<Arc<Transaction>>` instead of `Transaction`.
  * Changed `Wallet` methods `insert_tx`, `apply_unconfirmed_txs` to take in `T: Into<Arc<Transaction>>` instead of `Transaction` or `&Transaction`.

  ### 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

ACKs for top commit:
  oleonardolima:
    utACK 87e6121
  notmandatory:
    ACK 87e6121

Tree-SHA512: 6be8cde80984caaaf544fa5738bc4de39428cd4cd352d242ecb73d7cf2e69794c869249e76653a32a47bb5d19e4b3a47036e063cdd32ad47642b44c99cb7ee7c

crates/chain/src/indexed_tx_graph.rs

@@ -2,7 +2,7 @@
 //! [`IndexedTxGraph`] documentation for more.
 use core::fmt::Debug;
 
-use alloc::vec::Vec;
+use alloc::{sync::Arc, vec::Vec};
 use bitcoin::{Block, OutPoint, Transaction, TxOut, Txid};
 
 use crate::{
@@ -133,13 +133,10 @@ where
     }
 
     /// Insert and index a transaction into the graph.
-    pub fn insert_tx(&mut self, tx: Transaction) -> ChangeSet<A, I::ChangeSet> {
-        let graph = self.graph.insert_tx(tx);
-        let indexer = self.index_tx_graph_changeset(&graph);
-        ChangeSet {
-            tx_graph: graph,
-            indexer,
-        }
+    pub fn insert_tx<T: Into<Arc<Transaction>>>(&mut self, tx: T) -> ChangeSet<A, I::ChangeSet> {
+        let tx_graph = self.graph.insert_tx(tx);
+        let indexer = self.index_tx_graph_changeset(&tx_graph);
+        ChangeSet { tx_graph, indexer }
     }
 
     /// Insert an `anchor` for a given transaction.
@@ -159,38 +156,38 @@ where
     ///
     /// Relevancy is determined by the [`Indexer::is_tx_relevant`] implementation of `I`. Irrelevant
     /// transactions in `txs` will be ignored. `txs` do not need to be in topological order.
-    pub fn batch_insert_relevant<'t>(
+    pub fn batch_insert_relevant<T: Into<Arc<Transaction>>>(
         &mut self,
-        txs: impl IntoIterator<Item = (&'t Transaction, impl IntoIterator<Item = A>)>,
+        txs: impl IntoIterator<Item = (T, impl IntoIterator<Item = A>)>,
     ) -> ChangeSet<A, I::ChangeSet> {
         // The algorithm below allows for non-topologically ordered transactions by using two loops.
         // This is achieved by:
         // 1. insert all txs into the index. If they are irrelevant then that's fine it will just
         //    not store anything about them.
         // 2. decide whether to insert them into the graph depending on whether `is_tx_relevant`
         //    returns true or not. (in a second loop).
-        let txs = txs.into_iter().collect::<Vec<_>>();
+        let txs = txs
+            .into_iter()
+            .map(|(tx, anchors)| (<T as Into<Arc<Transaction>>>::into(tx), anchors))
+            .collect::<Vec<_>>();
 
         let mut indexer = I::ChangeSet::default();
         for (tx, _) in &txs {
             indexer.merge(self.index.index_tx(tx));
         }
 
-        let mut graph = tx_graph::ChangeSet::default();
+        let mut tx_graph = tx_graph::ChangeSet::default();
         for (tx, anchors) in txs {
-            if self.index.is_tx_relevant(tx) {
+            if self.index.is_tx_relevant(&tx) {
                 let txid = tx.compute_txid();
-                graph.merge(self.graph.insert_tx(tx.clone()));
+                tx_graph.merge(self.graph.insert_tx(tx.clone()));
                 for anchor in anchors {
-                    graph.merge(self.graph.insert_anchor(txid, anchor));
+                    tx_graph.merge(self.graph.insert_anchor(txid, anchor));
                 }
             }
         }
 
-        ChangeSet {
-            tx_graph: graph,
-            indexer,
-        }
+        ChangeSet { tx_graph, indexer }
     }
 
     /// Batch insert unconfirmed transactions, filtering out those that are irrelevant.
@@ -201,17 +198,20 @@ where
     /// Items of `txs` are tuples containing the transaction and a *last seen* timestamp. The
     /// *last seen* communicates when the transaction is last seen in the mempool which is used for
     /// conflict-resolution in [`TxGraph`] (refer to [`TxGraph::insert_seen_at`] for details).
-    pub fn batch_insert_relevant_unconfirmed<'t>(
+    pub fn batch_insert_relevant_unconfirmed<T: Into<Arc<Transaction>>>(
         &mut self,
-        unconfirmed_txs: impl IntoIterator<Item = (&'t Transaction, u64)>,
+        unconfirmed_txs: impl IntoIterator<Item = (T, u64)>,
     ) -> ChangeSet<A, I::ChangeSet> {
         // The algorithm below allows for non-topologically ordered transactions by using two loops.
         // This is achieved by:
         // 1. insert all txs into the index. If they are irrelevant then that's fine it will just
         //    not store anything about them.
         // 2. decide whether to insert them into the graph depending on whether `is_tx_relevant`
         //    returns true or not. (in a second loop).
-        let txs = unconfirmed_txs.into_iter().collect::<Vec<_>>();
+        let txs = unconfirmed_txs
+            .into_iter()
+            .map(|(tx, last_seen)| (<T as Into<Arc<Transaction>>>::into(tx), last_seen))
+            .collect::<Vec<_>>();
 
         let mut indexer = I::ChangeSet::default();
         for (tx, _) in &txs {
@@ -239,9 +239,9 @@ where
     /// To filter out irrelevant transactions, use [`batch_insert_relevant_unconfirmed`] instead.
     ///
     /// [`batch_insert_relevant_unconfirmed`]: IndexedTxGraph::batch_insert_relevant_unconfirmed
-    pub fn batch_insert_unconfirmed(
+    pub fn batch_insert_unconfirmed<T: Into<Arc<Transaction>>>(
         &mut self,
-        txs: impl IntoIterator<Item = (Transaction, u64)>,
+        txs: impl IntoIterator<Item = (T, u64)>,
     ) -> ChangeSet<A, I::ChangeSet> {
         let graph = self.graph.batch_insert_unconfirmed(txs);
         let indexer = self.index_tx_graph_changeset(&graph);

crates/chain/src/tx_graph.rs

@@ -606,12 +606,13 @@ impl<A: Clone + Ord> TxGraph<A> {
     /// Items of `txs` are tuples containing the transaction and a *last seen* timestamp. The
     /// *last seen* communicates when the transaction is last seen in mempool which is used for
     /// conflict-resolution (refer to [`TxGraph::insert_seen_at`] for details).
-    pub fn batch_insert_unconfirmed(
+    pub fn batch_insert_unconfirmed<T: Into<Arc<Transaction>>>(
         &mut self,
-        txs: impl IntoIterator<Item = (Transaction, u64)>,
+        txs: impl IntoIterator<Item = (T, u64)>,
     ) -> ChangeSet<A> {
         let mut changeset = ChangeSet::<A>::default();
         for (tx, seen_at) in txs {
+            let tx: Arc<Transaction> = tx.into();
             changeset.merge(self.insert_seen_at(tx.compute_txid(), seen_at));
             changeset.merge(self.insert_tx(tx));
         }

crates/chain/tests/test_indexed_tx_graph.rs

@@ -81,7 +81,7 @@ fn insert_relevant_txs() {
     };
 
     assert_eq!(
-        graph.batch_insert_relevant(txs.iter().map(|tx| (tx, None))),
+        graph.batch_insert_relevant(txs.iter().cloned().map(|tx| (tx, None))),
         changeset,
     );
 
@@ -237,10 +237,10 @@ fn test_list_owned_txouts() {
     // Insert unconfirmed txs with a last_seen timestamp
 
     let _ =
-        graph.batch_insert_relevant([&tx1, &tx2, &tx3, &tx6].iter().enumerate().map(|(i, tx)| {
+        graph.batch_insert_relevant([&tx1, &tx2, &tx3, &tx6].iter().enumerate().map(|(i, &tx)| {
             let height = i as u32;
             (
-                *tx,
+                tx.clone(),
                 local_chain
                     .get(height)
                     .map(|cp| cp.block_id())
@@ -251,7 +251,8 @@ fn test_list_owned_txouts() {
             )
         }));
 
-    let _ = graph.batch_insert_relevant_unconfirmed([&tx4, &tx5].iter().map(|tx| (*tx, 100)));
+    let _ =
+        graph.batch_insert_relevant_unconfirmed([&tx4, &tx5].iter().map(|&tx| (tx.clone(), 100)));
 
     // A helper lambda to extract and filter data from the graph.
     let fetch =

crates/wallet/src/wallet/mod.rs

@@ -1094,7 +1094,7 @@ impl Wallet {
     /// By default the inserted `tx` won't be considered "canonical" because it's not known
     /// whether the transaction exists in the best chain. To know whether it exists, the tx
     /// must be broadcast to the network and the wallet synced via a chain source.
-    pub fn insert_tx(&mut self, tx: Transaction) -> bool {
+    pub fn insert_tx<T: Into<Arc<Transaction>>>(&mut self, tx: T) -> bool {
         let mut changeset = ChangeSet::default();
         changeset.merge(self.indexed_graph.insert_tx(tx).into());
         let ret = !changeset.is_empty();
@@ -2484,9 +2484,9 @@ impl Wallet {
     /// **WARNING**: You must persist the changes resulting from one or more calls to this method
     /// if you need the applied unconfirmed transactions to be reloaded after closing the wallet.
     /// See [`Wallet::reveal_next_address`].
-    pub fn apply_unconfirmed_txs<'t>(
+    pub fn apply_unconfirmed_txs<T: Into<Arc<Transaction>>>(
         &mut self,
-        unconfirmed_txs: impl IntoIterator<Item = (&'t Transaction, u64)>,
+        unconfirmed_txs: impl IntoIterator<Item = (T, u64)>,
     ) {
         let indexed_graph_changeset = self
             .indexed_graph

example-crates/example_bitcoind_rpc_polling/src/main.rs

@@ -201,9 +201,10 @@ fn main() -> anyhow::Result<()> {
             }
 
             let mempool_txs = emitter.mempool()?;
-            let graph_changeset = graph.lock().unwrap().batch_insert_relevant_unconfirmed(
-                mempool_txs.iter().map(|(tx, time)| (tx, *time)),
-            );
+            let graph_changeset = graph
+                .lock()
+                .unwrap()
+                .batch_insert_relevant_unconfirmed(mempool_txs);
             {
                 let db = &mut *db.lock().unwrap();
                 db_stage.merge(ChangeSet {
@@ -286,9 +287,7 @@ fn main() -> anyhow::Result<()> {
                         (chain_changeset, graph_changeset)
                     }
                     Emission::Mempool(mempool_txs) => {
-                        let graph_changeset = graph.batch_insert_relevant_unconfirmed(
-                            mempool_txs.iter().map(|(tx, time)| (tx, *time)),
-                        );
+                        let graph_changeset = graph.batch_insert_relevant_unconfirmed(mempool_txs);
                         (local_chain::ChangeSet::default(), graph_changeset)
                     }
                     Emission::Tip(h) => {

example-crates/wallet_rpc/src/main.rs

@@ -157,7 +157,7 @@ fn main() -> anyhow::Result<()> {
             }
             Emission::Mempool(mempool_emission) => {
                 let start_apply_mempool = Instant::now();
-                wallet.apply_unconfirmed_txs(mempool_emission.iter().map(|(tx, time)| (tx, *time)));
+                wallet.apply_unconfirmed_txs(mempool_emission);
                 wallet.persist(&mut db)?;
                 println!(
                     "Applied unconfirmed transactions in {}s",