fix(wallet): ensure there are not duplicated foreign utxos

Author: nymius

crates/wallet/src/wallet/mod.rs

@@ -1432,7 +1432,7 @@ impl Wallet {
                     utxo: Utxo::Local(utxo),
                 })
                 // include foreign UTxOs as required
-                .chain(params.foreign_utxos.clone())
+                .chain(params.foreign_utxos.clone().into_values())
                 .collect();
             let optional = self.filter_utxos(&params, current_height.to_consensus_u32());
 
@@ -1640,9 +1640,12 @@ impl Wallet {
             .map_err(|_| BuildFeeBumpError::FeeRateUnavailable)?;
 
         // remove the inputs from the tx and process them
-        let (local, foreign): (HashSet<LocalOutput>, Vec<WeightedUtxo>) =
+        let (local, foreign): (HashSet<LocalOutput>, HashMap<OutPoint, WeightedUtxo>) =
             tx.input.drain(..).try_fold(
-                (HashSet::<LocalOutput>::new(), Vec::<WeightedUtxo>::new()),
+                (
+                    HashSet::<LocalOutput>::new(),
+                    HashMap::<OutPoint, WeightedUtxo>::new(),
+                ),
                 |(mut local, mut foreign), txin| -> Result<_, BuildFeeBumpError> {
                     let prev_tx = graph
                         .get_tx(txin.previous_output.txid)
@@ -1670,27 +1673,24 @@ impl Wallet {
                                     + serialize(&txin.witness).len(),
                             );
 
-                            let psbt_input_data = if txout.script_pubkey.witness_version().is_some()
-                            {
-                                Box::new(psbt::Input {
-                                    witness_utxo: Some(txout.clone()),
-                                    non_witness_utxo: Some(prev_tx.as_ref().clone()),
-                                    ..Default::default()
-                                })
-                            } else {
-                                Box::new(psbt::Input {
-                                    non_witness_utxo: Some(prev_tx.as_ref().clone()),
-                                    ..Default::default()
-                                })
-                            };
-                            foreign.push(WeightedUtxo {
-                                utxo: Utxo::Foreign {
-                                    outpoint: txin.previous_output,
-                                    sequence: txin.sequence,
-                                    psbt_input: psbt_input_data,
+                            foreign.insert(
+                                txin.previous_output,
+                                WeightedUtxo {
+                                    utxo: Utxo::Foreign {
+                                        outpoint: txin.previous_output,
+                                        sequence: txin.sequence,
+                                        psbt_input: Box::new(psbt::Input {
+                                            witness_utxo: txout
+                                                .script_pubkey
+                                                .witness_version()
+                                                .map(|_| txout.clone()),
+                                            non_witness_utxo: Some(prev_tx.as_ref().clone()),
+                                            ..Default::default()
+                                        }),
+                                    },
+                                    satisfaction_weight,
                                 },
-                                satisfaction_weight,
-                            });
+                            );
                             true
                         }
                     };

crates/wallet/src/wallet/tx_builder.rs

@@ -52,7 +52,7 @@ use rand_core::RngCore;
 use super::coin_selection::CoinSelectionAlgorithm;
 use super::utils::shuffle_slice;
 use super::{CreateTxError, Wallet};
-use crate::collections::{BTreeMap, HashSet};
+use crate::collections::{BTreeMap, HashMap, HashSet};
 use crate::{KeychainKind, LocalOutput, Utxo, WeightedUtxo};
 
 /// A transaction builder
@@ -126,7 +126,7 @@ pub(crate) struct TxParams {
     pub(crate) internal_policy_path: Option<BTreeMap<String, Vec<usize>>>,
     pub(crate) external_policy_path: Option<BTreeMap<String, Vec<usize>>>,
     pub(crate) utxos: HashSet<LocalOutput>,
-    pub(crate) foreign_utxos: Vec<WeightedUtxo>,
+    pub(crate) foreign_utxos: HashMap<OutPoint, WeightedUtxo>,
     pub(crate) unspendable: HashSet<OutPoint>,
     pub(crate) manually_selected_only: bool,
     pub(crate) sighash: Option<psbt::PsbtSighashType>,
@@ -382,14 +382,17 @@ impl<'a, Cs> TxBuilder<'a, Cs> {
             }
         }
 
-        self.params.foreign_utxos.push(WeightedUtxo {
-            satisfaction_weight,
-            utxo: Utxo::Foreign {
-                outpoint,
-                sequence,
-                psbt_input: Box::new(psbt_input),
+        self.params.foreign_utxos.insert(
+            outpoint,
+            WeightedUtxo {
+                satisfaction_weight,
+                utxo: Utxo::Foreign {
+                    outpoint,
+                    sequence,
+                    psbt_input: Box::new(psbt_input),
+                },
             },
-        });
+        );
 
         Ok(self)
     }
@@ -1100,4 +1103,56 @@ mod test {
             params.utxos.into_iter().collect::<Vec<_>>()
         );
     }
+
+    #[test]
+    fn not_duplicated_foreign_utxos_with_same_outpoint_but_different_weight() {
+        use crate::test_utils::{get_funded_wallet_single, get_funded_wallet_wpkh, get_test_wpkh};
+
+        // Use two different wallets to avoid adding local utxos
+        let (wallet1, txid1) = get_funded_wallet_wpkh();
+        let (mut wallet2, txid2) = get_funded_wallet_single(get_test_wpkh());
+
+        // if the transactions were produced by the same wallet the following assert should fail
+        assert_ne!(txid1, txid2);
+
+        let utxo1 = wallet1.list_unspent().next().unwrap();
+        let tx1 = wallet1.get_tx(txid1).unwrap().tx_node.tx.clone();
+
+        let satisfaction_weight = wallet1
+            .public_descriptor(KeychainKind::External)
+            .max_weight_to_satisfy()
+            .unwrap();
+
+        let mut builder = wallet2.build_tx();
+
+        // add foreign utxo with satisfaction weight x
+        assert!(builder
+            .add_foreign_utxo(
+                utxo1.outpoint,
+                psbt::Input {
+                    non_witness_utxo: Some(tx1.as_ref().clone()),
+                    ..Default::default()
+                },
+                satisfaction_weight,
+            )
+            .is_ok());
+
+        let modified_satisfaction_weight = satisfaction_weight - Weight::from_wu(6);
+
+        assert_ne!(satisfaction_weight, modified_satisfaction_weight);
+
+        // add foreign utxo with same outpoint but satisfaction weight x - 6wu
+        assert!(builder
+            .add_foreign_utxo(
+                utxo1.outpoint,
+                psbt::Input {
+                    non_witness_utxo: Some(tx1.as_ref().clone()),
+                    ..Default::default()
+                },
+                modified_satisfaction_weight,
+            )
+            .is_ok());
+
+        assert_eq!(builder.params.foreign_utxos.len(), 1);
+    }
 }