Merge bitcoindevkit#1737: fix(tx_builder)!: make TxBuilder Send safe,…

… remove Clone trait

663fb13 fix(tx_builder)!: make TxBuilder Send safe, remove Clone trait (Steve Myers)

Pull request description:

  ### Description

  Inspired by discord chat with @stevenroose as a way to make the `TxBuilder` Send safe.

  See his original patch on 1.0.0-beta.5: https://gist.github.com/stevenroose/f7736dfedfaa64bbdbb0da5875df28fc

  ### Notes to the reviewers

  I had to remove the `Clone` trait on `TxBuilder` but it was only being used in tests.

  ### Changelog notice

  - TxBuilder is now Send safe and does not implement the Clone trait

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

  #### New Features:

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

ACKs for top commit:
  evanlinjin:
    ACK 663fb13

Tree-SHA512: 026c0f5f227b5613bbab069b2c5238266aea6f6c2ae184cf77d37777fee2ddd52a99c9e305c107a2edd855dbd182d1b9194de361703995732061649f155cb65f

crates/wallet/src/wallet/mod.rs

@@ -1205,7 +1205,7 @@ impl Wallet {
     /// [`TxBuilder`]: crate::TxBuilder
     pub fn build_tx(&mut self) -> TxBuilder<'_, DefaultCoinSelectionAlgorithm> {
         TxBuilder {
-            wallet: alloc::rc::Rc::new(core::cell::RefCell::new(self)),
+            wallet: self,
             params: TxParams::default(),
             coin_selection: DefaultCoinSelectionAlgorithm::default(),
         }
@@ -1711,7 +1711,7 @@ impl Wallet {
         };
 
         Ok(TxBuilder {
-            wallet: alloc::rc::Rc::new(core::cell::RefCell::new(self)),
+            wallet: self,
             params,
             coin_selection: DefaultCoinSelectionAlgorithm::default(),
         })

crates/wallet/src/wallet/tx_builder.rs

@@ -36,8 +36,7 @@
 //! # Ok::<(), anyhow::Error>(())
 //! ```
 
-use alloc::{boxed::Box, rc::Rc, string::String, vec::Vec};
-use core::cell::RefCell;
+use alloc::{boxed::Box, string::String, vec::Vec};
 use core::fmt;
 
 use alloc::sync::Arc;
@@ -111,7 +110,7 @@ use crate::{KeychainKind, LocalOutput, Utxo, WeightedUtxo};
 /// [`coin_selection`]: Self::coin_selection
 #[derive(Debug)]
 pub struct TxBuilder<'a, Cs> {
-    pub(crate) wallet: Rc<RefCell<&'a mut Wallet>>,
+    pub(crate) wallet: &'a mut Wallet,
     pub(crate) params: TxParams,
     pub(crate) coin_selection: Cs,
 }
@@ -161,16 +160,6 @@ impl Default for FeePolicy {
     }
 }
 
-impl<'a, Cs: Clone> Clone for TxBuilder<'a, Cs> {
-    fn clone(&self) -> Self {
-        TxBuilder {
-            wallet: self.wallet.clone(),
-            params: self.params.clone(),
-            coin_selection: self.coin_selection.clone(),
-        }
-    }
-}
-
 // Methods supported for any CoinSelectionAlgorithm.
 impl<'a, Cs> TxBuilder<'a, Cs> {
     /// Set a custom fee rate.
@@ -286,7 +275,7 @@ impl<'a, Cs> TxBuilder<'a, Cs> {
     /// the "utxos" and the "unspendable" list, it will be spent.
     pub fn add_utxos(&mut self, outpoints: &[OutPoint]) -> Result<&mut Self, AddUtxoError> {
         {
-            let wallet = self.wallet.borrow();
+            let wallet = &mut self.wallet;
             let utxos = outpoints
                 .iter()
                 .map(|outpoint| {
@@ -682,9 +671,7 @@ impl<'a, Cs: CoinSelectionAlgorithm> TxBuilder<'a, Cs> {
     /// **WARNING**: To avoid change address reuse you must persist the changes resulting from one
     /// or more calls to this method before closing the wallet. See [`Wallet::reveal_next_address`].
     pub fn finish_with_aux_rand(self, rng: &mut impl RngCore) -> Result<Psbt, CreateTxError> {
-        self.wallet
-            .borrow_mut()
-            .create_tx(self.coin_selection, self.params, rng)
+        self.wallet.create_tx(self.coin_selection, self.params, rng)
     }
 }
 
@@ -750,7 +737,7 @@ impl fmt::Display for AddForeignUtxoError {
 #[cfg(feature = "std")]
 impl std::error::Error for AddForeignUtxoError {}
 
-type TxSort<T> = dyn Fn(&T, &T) -> core::cmp::Ordering;
+type TxSort<T> = dyn (Fn(&T, &T) -> core::cmp::Ordering) + Send + Sync;
 
 /// Ordering of the transaction's inputs and outputs
 #[derive(Clone, Default)]

crates/wallet/tests/wallet.rs

@@ -1649,11 +1649,10 @@ fn test_add_foreign_utxo_only_witness_utxo() {
         .max_weight_to_satisfy()
         .unwrap();
 
-    let mut builder = wallet1.build_tx();
-    builder.add_recipient(addr.script_pubkey(), Amount::from_sat(60_000));
-
     {
-        let mut builder = builder.clone();
+        let mut builder = wallet1.build_tx();
+        builder.add_recipient(addr.script_pubkey(), Amount::from_sat(60_000));
+
         let psbt_input = psbt::Input {
             witness_utxo: Some(utxo2.txout.clone()),
             ..Default::default()
@@ -1668,7 +1667,9 @@ fn test_add_foreign_utxo_only_witness_utxo() {
     }
 
     {
-        let mut builder = builder.clone();
+        let mut builder = wallet1.build_tx();
+        builder.add_recipient(addr.script_pubkey(), Amount::from_sat(60_000));
+
         let psbt_input = psbt::Input {
             witness_utxo: Some(utxo2.txout.clone()),
             ..Default::default()
@@ -1684,7 +1685,9 @@ fn test_add_foreign_utxo_only_witness_utxo() {
     }
 
     {
-        let mut builder = builder.clone();
+        let mut builder = wallet1.build_tx();
+        builder.add_recipient(addr.script_pubkey(), Amount::from_sat(60_000));
+
         let tx2 = wallet2.get_tx(txid2).unwrap().tx_node.tx;
         let psbt_input = psbt::Input {
             non_witness_utxo: Some(tx2.as_ref().clone()),
@@ -4192,3 +4195,9 @@ fn test_transactions_sort_by() {
         .collect();
     assert_eq!([None, Some(2000), Some(1000)], conf_heights.as_slice());
 }
+
+#[test]
+fn test_tx_builder_is_send_safe() {
+    let (mut wallet, _txid) = get_funded_wallet_wpkh();
+    let _box: Box<dyn Send + Sync> = Box::new(wallet.build_tx());
+}