[wallet] Stop implicitly enforcing manaul selection by .add_utxo

This makes it possible to choose a UTXO manually without having to
choose them *all* manually. I introduced the `manually_selected_only`
option to enforce that only manually selected utxos can be used.

To stop the cli semantics changing I made the `utxos` keep the old
behaviour by calling `manually_selected_only`.

diff --git a/src/cli.rs b/src/cli.rs
index bf50d961263c01f260ee2d7dd8b3d791af45d180..06b74fcb4196a786b9f601ee79c58e6c3d403187 100644 (file)
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -406,7 +406,7 @@ where
                 .map(|i| parse_outpoint(i))
                 .collect::<Result<Vec<_>, _>>()
                 .map_err(|s| Error::Generic(s.to_string()))?;
-            tx_builder = tx_builder.utxos(utxos);
+            tx_builder = tx_builder.utxos(utxos).manually_selected_only();
         }
 
         if let Some(unspendable) = sub_matches.values_of("unspendable") {

diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs
index ac350d45d1fd532bd938ee199d26b87afe57de1c..25482eadfc76ff08bed6bf11ccfdd613eb7d6a70 100644 (file)
--- a/src/wallet/mod.rs
+++ b/src/wallet/mod.rs
@@ -349,17 +349,14 @@ where
             ));
         }
 
-        let (available_utxos, use_all_utxos) = self.get_available_utxos(
+        let (must_use_utxos, may_use_utxos) = self.get_must_may_use_utxos(
             builder.change_policy,
-            &builder.utxos,
             &builder.unspendable,
+            &builder.utxos,
             builder.send_all,
+            builder.manually_selected_only,
         )?;
 
-        let (must_use_utxos, may_use_utxos) = match use_all_utxos {
-            true => (available_utxos, vec![]),
-            false => (vec![], available_utxos),
-        };
         let coin_selection::CoinSelectionResult {
             txin,
             selected_amount,
@@ -596,30 +593,29 @@ where
             })
             .collect::<Result<Vec<_>, _>>()?;
 
-        let builder_extra_utxos = builder.utxos.as_ref().map(|utxos| {
-            utxos
-                .iter()
-                .filter(|utxo| {
-                    !original_txin
-                        .iter()
-                        .any(|txin| &&txin.previous_output == utxo)
-                })
-                .cloned()
-                .collect()
-        });
-        let (available_utxos, use_all_utxos) = self.get_available_utxos(
+        let builder_extra_utxos = builder
+            .utxos
+            .iter()
+            .filter(|utxo| {
+                !original_txin
+                    .iter()
+                    .any(|txin| &&txin.previous_output == utxo)
+            })
+            .cloned()
+            .collect::<Vec<_>>();
+
+        let (must_use_utxos, may_use_utxos) = self.get_must_may_use_utxos(
             builder.change_policy,
-            &builder_extra_utxos,
             &builder.unspendable,
-            false,
+            &builder_extra_utxos[..],
+            false, // when doing bump_fee `send_all` does not mean use all available utxos
+            builder.manually_selected_only,
         )?;
-        let available_utxos =
-            rbf::filter_available(self.database.borrow().deref(), available_utxos.into_iter())?;
 
-        let (mut must_use_utxos, may_use_utxos) = match use_all_utxos {
-            true => (available_utxos, vec![]),
-            false => (vec![], available_utxos),
-        };
+        let mut must_use_utxos =
+            rbf::filter_available(self.database.borrow().deref(), must_use_utxos.into_iter())?;
+        let may_use_utxos =
+            rbf::filter_available(self.database.borrow().deref(), may_use_utxos.into_iter())?;
         must_use_utxos.append(&mut original_utxos);
 
         let amount_needed = tx.output.iter().fold(0, |acc, out| acc + out.value);
@@ -965,13 +961,7 @@ where
         Ok(())
     }
 
-    fn get_available_utxos(
-        &self,
-        change_policy: tx_builder::ChangeSpendPolicy,
-        utxo: &Option<Vec<OutPoint>>,
-        unspendable: &HashSet<OutPoint>,
-        send_all: bool,
-    ) -> Result<(Vec<(UTXO, usize)>, bool), Error> {
+    fn get_available_utxos(&self) -> Result<Vec<(UTXO, usize)>, Error> {
         let external_weight = self
             .get_descriptor_for_script_type(ScriptType::External)
             .0
@@ -990,33 +980,57 @@ where
             (utxo, weight)
         };
 
-        match utxo {
-            // with manual coin selection we always want to spend all the selected utxos, no matter
-            // what (even if they are marked as unspendable)
-            Some(raw_utxos) => {
-                let full_utxos = raw_utxos
-                    .iter()
-                    .map(|u| {
-                        Ok(add_weight(
-                            self.database
-                                .borrow()
-                                .get_utxo(&u)?
-                                .ok_or(Error::UnknownUTXO)?,
-                        ))
-                    })
-                    .collect::<Result<Vec<_>, Error>>()?;
+        let utxos = self.list_unspent()?.into_iter().map(add_weight).collect();
 
-                Ok((full_utxos, true))
-            }
-            // otherwise limit ourselves to the spendable utxos for the selected policy, and the `send_all` setting
-            None => {
-                let utxos = self.list_unspent()?.into_iter().filter(|u| {
-                    change_policy.is_satisfied_by(u) && !unspendable.contains(&u.outpoint)
-                });
+        Ok(utxos)
+    }
 
-                Ok((utxos.map(add_weight).collect(), send_all))
-            }
+    /// Given the options returns the list of utxos that must be used to form the
+    /// transaction and any further that may be used if needed.
+    #[allow(clippy::type_complexity)]
+    fn get_must_may_use_utxos(
+        &self,
+        change_policy: tx_builder::ChangeSpendPolicy,
+        unspendable: &HashSet<OutPoint>,
+        manually_selected: &[OutPoint],
+        must_use_all_available: bool,
+        manual_only: bool,
+    ) -> Result<(Vec<(UTXO, usize)>, Vec<(UTXO, usize)>), Error> {
+        //    must_spend <- manually selected utxos
+        //    may_spend  <- all other available utxos
+        let mut may_spend = self.get_available_utxos()?;
+        let mut must_spend = {
+            let must_spend_idx = manually_selected
+                .iter()
+                .map(|manually_selected| {
+                    may_spend
+                        .iter()
+                        .position(|available| available.0.outpoint == *manually_selected)
+                        .ok_or(Error::UnknownUTXO)
+                })
+                .collect::<Result<Vec<_>, _>>()?;
+
+            must_spend_idx
+                .into_iter()
+                .map(|i| may_spend.remove(i))
+                .collect()
+        };
+
+        // NOTE: we are intentionally ignoring `unspendable` here. i.e manual
+        // selection overrides unspendable.
+        if manual_only {
+            return Ok((must_spend, vec![]));
         }
+
+        may_spend.retain(|u| {
+            change_policy.is_satisfied_by(&u.0) && !unspendable.contains(&u.0.outpoint)
+        });
+
+        if must_use_all_available {
+            must_spend.append(&mut may_spend);
+        }
+
+        Ok((must_spend, may_spend))
     }
 
     fn complete_transaction<Cs: coin_selection::CoinSelectionAlgorithm<D>>(
@@ -1988,6 +2002,56 @@ mod test {
         assert!(psbt.inputs[0].witness_utxo.is_some());
     }
 
+    #[test]
+    fn test_create_tx_add_utxo() {
+        let (wallet, descriptors, _) = get_funded_wallet(get_test_wpkh());
+        let small_output_txid = wallet.database.borrow_mut().received_tx(
+            testutils! (@tx ( (@external descriptors, 0) => 25_000 ) (@confirmations 1)),
+            Some(100),
+        );
+
+        let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX").unwrap();
+        let (psbt, details) = wallet
+            .create_tx(
+                TxBuilder::with_recipients(vec![(addr.script_pubkey(), 30_000)]).add_utxo(
+                    OutPoint {
+                        txid: small_output_txid,
+                        vout: 0,
+                    },
+                ),
+            )
+            .unwrap();
+
+        assert_eq!(
+            psbt.global.unsigned_tx.input.len(),
+            2,
+            "should add an additional input since 25_000 < 30_000"
+        );
+        assert_eq!(details.sent, 75_000, "total should be sum of both inputs");
+    }
+
+    #[test]
+    #[should_panic(expected = "InsufficientFunds")]
+    fn test_create_tx_manually_selected_insufficient() {
+        let (wallet, descriptors, _) = get_funded_wallet(get_test_wpkh());
+        let small_output_txid = wallet.database.borrow_mut().received_tx(
+            testutils! (@tx ( (@external descriptors, 0) => 25_000 ) (@confirmations 1)),
+            Some(100),
+        );
+
+        let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX").unwrap();
+        wallet
+            .create_tx(
+                TxBuilder::with_recipients(vec![(addr.script_pubkey(), 30_000)])
+                    .add_utxo(OutPoint {
+                        txid: small_output_txid,
+                        vout: 0,
+                    })
+                    .manually_selected_only(),
+            )
+            .unwrap();
+    }
+
     #[test]
     #[should_panic(expected = "IrreplaceableTransaction")]
     fn test_bump_fee_irreplaceable_tx() {
@@ -2330,6 +2394,7 @@ mod test {
                         txid: incoming_txid,
                         vout: 0,
                     }])
+                    .manually_selected_only()
                     .send_all()
                     .enable_rbf(),
             )
@@ -2506,6 +2571,7 @@ mod test {
                         txid: incoming_txid,
                         vout: 0,
                     })
+                    .manually_selected_only()
                     .enable_rbf(),
             )
             .unwrap();

diff --git a/src/wallet/tx_builder.rs b/src/wallet/tx_builder.rs
index f72bf48d8e09d3a9053cfd8d21713547c430ad43..17f97dd9c31a6c732ba6af578e398c0e49449082 100644 (file)
--- a/src/wallet/tx_builder.rs
+++ b/src/wallet/tx_builder.rs
@@ -63,8 +63,9 @@ pub struct TxBuilder<D: Database, Cs: CoinSelectionAlgorithm<D>> {
     pub(crate) send_all: bool,
     pub(crate) fee_policy: Option<FeePolicy>,
     pub(crate) policy_path: Option<BTreeMap<String, Vec<usize>>>,
-    pub(crate) utxos: Option<Vec<OutPoint>>,
+    pub(crate) utxos: Vec<OutPoint>,
     pub(crate) unspendable: HashSet<OutPoint>,
+    pub(crate) manually_selected_only: bool,
     pub(crate) sighash: Option<SigHashType>,
     pub(crate) ordering: TxOrdering,
     pub(crate) locktime: Option<u32>,
@@ -102,6 +103,7 @@ where
             policy_path: Default::default(),
             utxos: Default::default(),
             unspendable: Default::default(),
+            manually_selected_only: Default::default(),
             sighash: Default::default(),
             ordering: Default::default(),
             locktime: Default::default(),
@@ -141,11 +143,19 @@ impl<D: Database, Cs: CoinSelectionAlgorithm<D>> TxBuilder<D, Cs> {
         self
     }
 
-    /// Send all the selected utxos to a single output
+    /// Send all inputs to a single output.
+    ///
+    /// The semantics of `send_all` depend on whether you are using [`create_tx`] or [`bump_fee`].
+    /// In `create_tx` it (by default) **selects all the wallets inputs** and sends them to a single
+    /// output. In `bump_fee` it means to send the original inputs and any additional manually
+    /// selected intputs to a single output.
     ///
     /// Adding more than one recipients with this option enabled will result in an error.
     ///
     /// The value associated with the only recipient is irrelevant and will be replaced by the wallet.
+    ///
+    /// [`bump_fee`]: crate::wallet::Wallet::bump_fee
+    /// [`create_tx`]: crate::wallet::Wallet::create_tx
     pub fn send_all(mut self) -> Self {
         self.send_all = true;
         self
@@ -179,7 +189,7 @@ impl<D: Database, Cs: CoinSelectionAlgorithm<D>> TxBuilder<D, Cs> {
     /// These have priority over the "unspendable" utxos, meaning that if a utxo is present both in
     /// the "utxos" and the "unspendable" list, it will be spent.
     pub fn utxos(mut self, utxos: Vec<OutPoint>) -> Self {
-        self.utxos = Some(utxos);
+        self.utxos = utxos;
         self
     }
 
@@ -188,7 +198,19 @@ impl<D: Database, Cs: CoinSelectionAlgorithm<D>> TxBuilder<D, Cs> {
     /// These have priority over the "unspendable" utxos, meaning that if a utxo is present both in
     /// the "utxos" and the "unspendable" list, it will be spent.
     pub fn add_utxo(mut self, utxo: OutPoint) -> Self {
-        self.utxos.get_or_insert(vec![]).push(utxo);
+        self.utxos.push(utxo);
+        self
+    }
+
+    /// Only spend utxos added by [`add_utxo`] and [`utxos`].
+    ///
+    /// The wallet will **not** add additional utxos to the transaction even if they are needed to
+    /// make the transaction valid.
+    ///
+    /// [`add_utxo`]: Self::add_utxo
+    /// [`utxos`]: Self::utxos
+    pub fn manually_selected_only(mut self) -> Self {
+        self.manually_selected_only = true;
         self
     }
 
@@ -310,6 +332,7 @@ impl<D: Database, Cs: CoinSelectionAlgorithm<D>> TxBuilder<D, Cs> {
             policy_path: self.policy_path,
             utxos: self.utxos,
             unspendable: self.unspendable,
+            manually_selected_only: self.manually_selected_only,
             sighash: self.sighash,
             ordering: self.ordering,
             locktime: self.locktime,