From 10fcba94393fdfa762959e2fdc053a1f83f980cd Mon Sep 17 00:00:00 2001
From: LLFourn <lloyd.fourn@gmail.com>
Date: Fri, 22 Jan 2021 14:04:06 +1100
Subject: [PATCH] Revert back to Vec to hold utxos in builder

Due to brain malfunction I made utxos into a BTree. This made a test
pass but is incorrect. The test itself was incorrect as per comment in

https://github.com/bitcoindevkit/bdk/pull/258#issuecomment-758370380

So I (1) reverted utxos back to a Vec, (2) fixed the test and expanded
the comment in the test.
---
 src/wallet/mod.rs        | 17 ++++++++---------
 src/wallet/tx_builder.rs | 16 ++++++----------
 2 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs
index ccebe4f7..7921a189 100644
--- a/src/wallet/mod.rs
+++ b/src/wallet/mod.rs
@@ -507,7 +507,7 @@ where
         let (required_utxos, optional_utxos) = self.preselect_utxos(
             params.change_policy,
             &params.unspendable,
-            params.utxos.values().cloned().collect(),
+            params.utxos.clone(),
             params.drain_wallet,
             params.manually_selected_only,
             params.bumping_fee.is_some(), // we mandate confirmed transactions if we're bumping the fee
@@ -672,7 +672,7 @@ where
         let original_txin = tx.input.drain(..).collect::<Vec<_>>();
         let original_utxos = original_txin
             .iter()
-            .map(|txin| -> Result<(OutPoint, (UTXO, usize)), Error> {
+            .map(|txin| -> Result<_, Error> {
                 let txout = self
                     .database
                     .borrow()
@@ -706,9 +706,9 @@ where
                     keychain,
                 };
 
-                Ok((utxo.outpoint, (utxo, weight)))
+                Ok((utxo, weight))
             })
-            .collect::<Result<BTreeMap<OutPoint, (UTXO, usize)>, _>>()?;
+            .collect::<Result<Vec<_>, _>>()?;
 
         if tx.output.len() > 1 {
             let mut change_index = None;
@@ -2653,9 +2653,10 @@ mod test {
     fn test_bump_fee_remove_output_manually_selected_only() {
         let (wallet, descriptors, _) = get_funded_wallet(get_test_wpkh());
         // receive an extra tx so that our wallet has two utxos. then we manually pick only one of
-        // them, and make sure that `bump_fee` doesn't try to add more. eventually, it should fail
-        // because the fee rate is too high and the single utxo isn't enough to create a non-dust
-        // output
+        // them, and make sure that `bump_fee` doesn't try to add more. This fails because we've
+        // told the wallet it's not allowed to add more inputs AND it can't reduce the value of the
+        // existing output. In other words, bump_fee + manually_selected_only is always an error
+        // unless you've also set "maintain_single_recipient".
         let incoming_txid = crate::populate_test_db!(
             wallet.database.borrow_mut(),
             testutils! (@tx ( (@external descriptors, 0) => 25_000 ) (@confirmations 1)),
@@ -2694,8 +2695,6 @@ mod test {
 
         let mut builder = wallet.build_fee_bump(txid).unwrap();
         builder
-            .add_utxo(outpoint)
-            .unwrap()
             .manually_selected_only()
             .fee_rate(FeeRate::from_sat_per_vb(255.0));
         builder.finish().unwrap();
diff --git a/src/wallet/tx_builder.rs b/src/wallet/tx_builder.rs
index fce15827..92ea37c1 100644
--- a/src/wallet/tx_builder.rs
+++ b/src/wallet/tx_builder.rs
@@ -147,7 +147,7 @@ pub(crate) struct TxParams {
     pub(crate) fee_policy: Option<FeePolicy>,
     pub(crate) internal_policy_path: Option<BTreeMap<String, Vec<usize>>>,
     pub(crate) external_policy_path: Option<BTreeMap<String, Vec<usize>>>,
-    pub(crate) utxos: BTreeMap<OutPoint, (UTXO, usize)>,
+    pub(crate) utxos: Vec<(UTXO, usize)>,
     pub(crate) unspendable: HashSet<OutPoint>,
     pub(crate) manually_selected_only: bool,
     pub(crate) sighash: Option<SigHashType>,
@@ -273,15 +273,11 @@ impl<'a, B, D: BatchDatabase, Cs: CoinSelectionAlgorithm<D>, Ctx: TxBuilderConte
     /// 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, outpoint: OutPoint) -> Result<&mut Self, Error> {
-        if self.params.utxos.get(&outpoint).is_none() {
-            let deriv_ctx = crate::wallet::descriptor_to_pk_ctx(self.wallet.secp_ctx());
-            let utxo = self.wallet.get_utxo(outpoint)?.ok_or(Error::UnknownUTXO)?;
-            let descriptor = self.wallet.get_descriptor_for_keychain(utxo.keychain);
-            let satisfaction_weight = descriptor.max_satisfaction_weight(deriv_ctx).unwrap();
-            self.params
-                .utxos
-                .insert(outpoint, (utxo, satisfaction_weight));
-        }
+        let deriv_ctx = crate::wallet::descriptor_to_pk_ctx(self.wallet.secp_ctx());
+        let utxo = self.wallet.get_utxo(outpoint)?.ok_or(Error::UnknownUTXO)?;
+        let descriptor = self.wallet.get_descriptor_for_keychain(utxo.keychain);
+        let satisfaction_weight = descriptor.max_satisfaction_weight(deriv_ctx).unwrap();
+        self.params.utxos.push((utxo, satisfaction_weight));
         Ok(self)
     }