From 331623656c88c7c2a0346c275a2a6f3525744a07 Mon Sep 17 00:00:00 2001 From: nymius <155548262+nymius@users.noreply.github.com> Date: Tue, 10 Jun 2025 12:10:43 -0300 Subject: [PATCH 1/3] fix(tx_builder): preserve insertion order with TxOrdering::Untouched When TxBuilder::ordering is TxOrdering::Untouched, the insertion order of recipients and manually selected UTxOs should be preserved in transaction's output and input vectors respectively. Fixes #244 --- wallet/src/wallet/mod.rs | 95 +++++---- wallet/src/wallet/tx_builder.rs | 332 +++++++++++++++++++------------- wallet/tests/wallet.rs | 59 ++++++ 3 files changed, 304 insertions(+), 182 deletions(-) diff --git a/wallet/src/wallet/mod.rs b/wallet/src/wallet/mod.rs index be3773d34..ed6cb3f9e 100644 --- a/wallet/src/wallet/mod.rs +++ b/wallet/src/wallet/mod.rs @@ -1511,7 +1511,7 @@ impl Wallet { let (required_utxos, optional_utxos) = { // NOTE: manual selection overrides unspendable - let mut required: Vec = params.utxos.values().cloned().collect(); + let mut required: Vec = params.utxos.clone(); let optional = self.filter_utxos(¶ms, current_height.to_consensus_u32()); // if drain_wallet is true, all UTxOs are required @@ -1740,52 +1740,45 @@ impl Wallet { }) .map(|(prev_tx, prev_txout, chain_position)| { match txout_index.index_of_spk(prev_txout.script_pubkey.clone()) { - Some(&(keychain, derivation_index)) => ( - txin.previous_output, - WeightedUtxo { - satisfaction_weight: self - .public_descriptor(keychain) - .max_weight_to_satisfy() - .unwrap(), - utxo: Utxo::Local(LocalOutput { - outpoint: txin.previous_output, - txout: prev_txout.clone(), - keychain, - is_spent: true, - derivation_index, - chain_position, - }), - }, - ), + Some(&(keychain, derivation_index)) => WeightedUtxo { + satisfaction_weight: self + .public_descriptor(keychain) + .max_weight_to_satisfy() + .unwrap(), + utxo: Utxo::Local(LocalOutput { + outpoint: txin.previous_output, + txout: prev_txout.clone(), + keychain, + is_spent: true, + derivation_index, + chain_position, + }), + }, None => { let satisfaction_weight = Weight::from_wu_usize( serialize(&txin.script_sig).len() * 4 + serialize(&txin.witness).len(), ); - - ( - txin.previous_output, - WeightedUtxo { - utxo: Utxo::Foreign { - outpoint: txin.previous_output, - sequence: txin.sequence, - psbt_input: Box::new(psbt::Input { - witness_utxo: prev_txout - .script_pubkey - .witness_version() - .map(|_| prev_txout.clone()), - non_witness_utxo: Some(prev_tx.as_ref().clone()), - ..Default::default() - }), - }, - satisfaction_weight, + WeightedUtxo { + utxo: Utxo::Foreign { + outpoint: txin.previous_output, + sequence: txin.sequence, + psbt_input: Box::new(psbt::Input { + witness_utxo: prev_txout + .script_pubkey + .witness_version() + .map(|_| prev_txout.clone()), + non_witness_utxo: Some(prev_tx.as_ref().clone()), + ..Default::default() + }), }, - ) + satisfaction_weight, + } } } }) }) - .collect::, BuildFeeBumpError>>()?; + .collect::, BuildFeeBumpError>>()?; if tx.output.len() > 1 { let mut change_index = None; @@ -2099,6 +2092,11 @@ impl Wallet { vec![] // only process optional UTxOs if manually_selected_only is false } else { + let manually_selected_outpoints = params + .utxos + .iter() + .map(|wutxo| wutxo.utxo.outpoint()) + .collect::>(); self.indexed_graph .graph() // get all unspent UTxOs from wallet @@ -2116,9 +2114,10 @@ impl Wallet { .is_mature(current_height) .then(|| new_local_utxo(k, i, full_txo)) }) - // only process UTxOs not selected manually, they will be considered later in the - // chain NOTE: this avoid UTxOs in both required and optional list - .filter(|may_spend| !params.utxos.contains_key(&may_spend.outpoint)) + // only process UTXOs not selected manually, they will be considered later in the + // chain + // NOTE: this avoid UTXOs in both required and optional list + .filter(|may_spend| !manually_selected_outpoints.contains(&may_spend.outpoint)) // only add to optional UTxOs those which satisfy the change policy if we reuse // change .filter(|local_output| { @@ -2745,18 +2744,10 @@ mod test { let txid = two_output_tx.compute_txid(); insert_tx(&mut wallet, two_output_tx); - let mut params = TxParams::default(); - let output = wallet.get_utxo(OutPoint { txid, vout: 0 }).unwrap(); - params.utxos.insert( - output.outpoint, - WeightedUtxo { - satisfaction_weight: wallet - .public_descriptor(output.keychain) - .max_weight_to_satisfy() - .unwrap(), - utxo: Utxo::Local(output), - }, - ); + let outpoint = OutPoint { txid, vout: 0 }; + let mut builder = wallet.build_tx(); + builder.add_utxo(outpoint).expect("should add local utxo"); + let params = builder.params.clone(); // enforce selection of first output in transaction let received = wallet.filter_utxos(¶ms, wallet.latest_checkpoint().block_id().height); // notice expected doesn't include the first output from two_output_tx as it should be diff --git a/wallet/src/wallet/tx_builder.rs b/wallet/src/wallet/tx_builder.rs index 949cb7921..0ab1b9c0b 100644 --- a/wallet/src/wallet/tx_builder.rs +++ b/wallet/src/wallet/tx_builder.rs @@ -126,7 +126,7 @@ pub(crate) struct TxParams { pub(crate) fee_policy: Option, pub(crate) internal_policy_path: Option>>, pub(crate) external_policy_path: Option>>, - pub(crate) utxos: HashMap, + pub(crate) utxos: Vec, pub(crate) unspendable: HashSet, pub(crate) manually_selected_only: bool, pub(crate) sighash: Option, @@ -274,51 +274,65 @@ impl<'a, Cs> TxBuilder<'a, Cs> { /// If an error occurs while adding any of the UTXOs then none of them are added and the error /// is returned. /// - /// 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. + /// 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_utxos(&mut self, outpoints: &[OutPoint]) -> Result<&mut Self, AddUtxoError> { - let utxo_batch = outpoints + // Canonicalize once, instead of once for every call to `get_utxo`. + let unspent: HashMap = self + .wallet + .list_unspent() + .map(|output| (output.outpoint, output)) + .collect(); + + // Ensure that only unique outpoints are added, but keep insertion order. + let mut visited = >::new(); + + let utxos: Vec = outpoints .iter() - .map(|outpoint| { - self.wallet - .get_utxo(*outpoint) - .ok_or(AddUtxoError::UnknownUtxo(*outpoint)) - .map(|output| { - ( - *outpoint, - WeightedUtxo { - satisfaction_weight: self - .wallet - .public_descriptor(output.keychain) - .max_weight_to_satisfy() - .unwrap(), - utxo: Utxo::Local(output), - }, - ) - }) + .filter(|&&op| visited.insert(op)) + .map(|&op| -> Result<_, AddUtxoError> { + let output = unspent + .get(&op) + .cloned() + .ok_or(AddUtxoError::UnknownUtxo(op))?; + Ok(WeightedUtxo { + satisfaction_weight: self + .wallet + .public_descriptor(output.keychain) + .max_weight_to_satisfy() + .expect("descriptor should be satisfiable"), + utxo: Utxo::Local(output), + }) }) - .collect::, AddUtxoError>>()?; - self.params.utxos.extend(utxo_batch); + .collect::>()?; + + // Remove already inserted UTXOs first. + self.params + .utxos + .retain(|wutxo| !visited.contains(&wutxo.utxo.outpoint())); + + // Update the removed UTXOs in their new positions. + self.params.utxos.extend_from_slice(&utxos); Ok(self) } - /// Add a utxo to the internal list of utxos that **must** be spent + /// Add a UTXO to the internal list of UTXOs that **must** be spent /// - /// 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. + /// 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, AddUtxoError> { self.add_utxos(&[outpoint]) } /// Add a foreign UTXO i.e. a UTXO not known by this wallet. /// - /// There might be cases where the UTxO belongs to the wallet but it doesn't have knowledge of + /// There might be cases where the UTXO belongs to the wallet but it doesn't have knowledge of /// it. This is possible if the wallet is not synced or its not being use to track /// transactions. In those cases is the responsibility of the user to add any possible local - /// UTxOs through the [`TxBuilder::add_utxo`] method. - /// A manually added local UTxO will always have greater precedence than a foreign utxo. No - /// matter if it was added before or after the foreign UTxO. + /// UTXOs through the [`TxBuilder::add_utxo`] method. + /// A manually added local UTXO will always have greater precedence than a foreign UTXO. No + /// matter if it was added before or after the foreign UTXO. /// /// At a minimum to add a foreign UTXO we need: /// @@ -407,25 +421,32 @@ impl<'a, Cs> TxBuilder<'a, Cs> { } } - if let Some(WeightedUtxo { - utxo: Utxo::Local { .. }, - .. - }) = self.params.utxos.get(&outpoint) - { - None - } else { - self.params.utxos.insert( + let mut existing_index: Option = None; + + for (idx, wutxo) in self.params.utxos.iter().enumerate() { + if wutxo.utxo.outpoint() == outpoint { + match wutxo.utxo { + Utxo::Local(..) => return Ok(self), + Utxo::Foreign { .. } => { + existing_index = Some(idx); + break; + } + } + } + } + + if let Some(idx) = existing_index { + self.params.utxos.remove(idx); + } + + self.params.utxos.push(WeightedUtxo { + satisfaction_weight, + utxo: Utxo::Foreign { outpoint, - WeightedUtxo { - satisfaction_weight, - utxo: Utxo::Foreign { - outpoint, - sequence, - psbt_input: Box::new(psbt_input), - }, - }, - ) - }; + sequence, + psbt_input: Box::new(psbt_input), + }, + }); Ok(self) } @@ -468,6 +489,11 @@ impl<'a, Cs> TxBuilder<'a, Cs> { } /// Choose the ordering for inputs and outputs of the transaction + /// + /// When [TxBuilder::ordering] is set to [TxOrdering::Untouched], the insertion order of + /// recipients and manually selected UTXOs is preserved and reflected exactly in transaction's + /// output and input vectors respectively. If algorithmically selected UTXOs are included, they + /// will be placed after all the manually selected ones in the transaction's input vector. pub fn ordering(&mut self, ordering: TxOrdering) -> &mut Self { self.params.ordering = ordering; self @@ -778,6 +804,12 @@ pub enum TxOrdering { #[default] Shuffle, /// Unchanged + /// + /// Unchanged insertion order for recipients and for manually added UTXOs. This guarantees all + /// recipients preserve insertion order in the transaction's output vector and manually added + /// UTXOs preserve insertion order in the transaction's input vector, but does not make any + /// guarantees about algorithmically selected UTXOs. However, by design they will always be + /// placed after the manually selected ones. Untouched, /// Provide custom comparison functions for sorting Custom { @@ -1134,30 +1166,42 @@ mod test { let _ = builder.finish().unwrap(); } + #[test] + fn duplicated_utxos_in_add_utxos_are_only_added_once() { + use crate::test_utils::get_funded_wallet_wpkh; + + let (mut wallet, _) = get_funded_wallet_wpkh(); + let utxo = wallet.list_unspent().next().unwrap(); + let op = utxo.outpoint; + + let mut tx_builder = wallet.build_tx(); + tx_builder.add_utxos(&[op, op, op]).unwrap(); + + assert_eq!(tx_builder.params.utxos.len(), 1); + } + #[test] fn not_duplicated_utxos_in_required_list() { - let mut params = TxParams::default(); - let test_utxos = get_test_utxos(); + use crate::test_utils::get_funded_wallet_wpkh; + let (mut wallet1, _) = get_funded_wallet_wpkh(); + let utxo1 @ LocalOutput { outpoint, .. } = wallet1.list_unspent().next().unwrap(); + let mut builder = wallet1.build_tx(); let fake_weighted_utxo = WeightedUtxo { - satisfaction_weight: Weight::from_wu(0), - utxo: Utxo::Local(test_utxos[0].clone()), + satisfaction_weight: Weight::from_wu(107), + utxo: Utxo::Local(utxo1.clone()), }; + for _ in 0..3 { - params - .utxos - .insert(test_utxos[0].outpoint, fake_weighted_utxo.clone()); + builder.add_utxo(outpoint).expect("should add"); } - assert_eq!( - vec![(test_utxos[0].outpoint, fake_weighted_utxo)], - params.utxos.into_iter().collect::>() - ); + assert_eq!(vec![fake_weighted_utxo], builder.params.utxos); } #[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 + // 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()); @@ -1174,7 +1218,7 @@ mod test { let mut builder = wallet2.build_tx(); - // add foreign utxo with satisfaction weight x + // add foreign UTXO with satisfaction weight x assert!(builder .add_foreign_utxo( utxo1.outpoint, @@ -1190,7 +1234,7 @@ mod test { assert_ne!(satisfaction_weight, modified_satisfaction_weight); - // add foreign utxo with same outpoint but satisfaction weight x - 6wu + // add foreign UTXO with same outpoint but satisfaction weight x - 6wu assert!(builder .add_foreign_utxo( utxo1.outpoint, @@ -1202,12 +1246,9 @@ mod test { ) .is_ok()); - let foreign_utxo_with_modified_weight = - builder.params.utxos.values().collect::>()[0]; - assert_eq!(builder.params.utxos.len(), 1); assert_eq!( - foreign_utxo_with_modified_weight.satisfaction_weight, + builder.params.utxos[0].satisfaction_weight, modified_satisfaction_weight ); } @@ -1217,18 +1258,20 @@ mod test { // In this test we are assuming a setup where there are two wallets using the same // descriptor, but only one is tracking transactions, while the other is not. // Within this conditions we want the second wallet to be able to consider the unknown - // LocalOutputs provided by the first wallet with greater precedence than any foreign utxo, - // even if the foreign utxo shares the same outpoint Remember the second wallet does not - // know about any UTxOs, so in principle, an unknown local utxo could be added as foreign. + // LocalOutputs provided by the first wallet with greater precedence than any foreign UTXO, + // even if the foreign UTXO shares the same outpoint. + // + // Remember the second wallet does not know about any UTXOs, so in principle, an unknown + // local UTXO could be added as foreign. // - // In this case, somehow the wallet has knowledge of one local utxo and it tries to add the - // same utxo as a foreign one, but the API ignores this, because local utxos have higher + // In this case, somehow the wallet has knowledge of one local UTXO and it tries to add the + // same UTXO as a foreign one, but the API ignores this, because local UTXOs have higher // precedence. use crate::test_utils::{get_funded_wallet_wpkh, get_test_wpkh_and_change_desc}; use bitcoin::Network; // Use the same wallet twice - let (wallet1, txid1) = get_funded_wallet_wpkh(); + let (mut wallet1, txid1) = get_funded_wallet_wpkh(); // But the second one has no knowledge of tx associated with txid1 let (main_descriptor, change_descriptor) = get_test_wpkh_and_change_desc(); let mut wallet2 = Wallet::create(main_descriptor, change_descriptor) @@ -1237,47 +1280,57 @@ mod test { .expect("descriptors must be valid"); let utxo1 = wallet1.list_unspent().next().unwrap(); - let tx1 = wallet1.get_tx(txid1).unwrap().tx_node.tx.clone(); + let tx1 = wallet1.get_tx(txid1).unwrap().tx_node.as_ref().clone(); let satisfaction_weight = wallet1 .public_descriptor(KeychainKind::External) .max_weight_to_satisfy() .unwrap(); - let mut builder = wallet2.build_tx(); + // Here we are copying old_params to simulate, the very unlikely behavior where a wallet + // becomes aware of a local UTXO while it is creating a transaction using the local UTXO + // it just became aware of as a foreign UTXO + let old_params = { + let mut builder = wallet1.build_tx(); + + builder + .add_utxo(utxo1.outpoint) + .expect("should add local utxo"); + + builder.params.clone() + }; - // Add local UTxO manually, through tx_builder private parameters and not through - // add_utxo method because we are assuming wallet2 has not knowledge of utxo1 yet - builder.params.utxos.insert( - utxo1.outpoint, + // Build a transaction as wallet2 was aware of utxo1 but not tracking transactions + let mut new_builder = wallet2.build_tx(); + new_builder.params = old_params; + + // Check the local UTXO is still there + // UTXO should still be LocalOutput + assert!(matches!( + new_builder.params.utxos[0], WeightedUtxo { - satisfaction_weight: wallet1 - .public_descriptor(utxo1.keychain) - .max_weight_to_satisfy() - .unwrap(), - utxo: Utxo::Local(utxo1.clone()), - }, - ); + utxo: Utxo::Local { .. }, + .. + } + )); - // add foreign utxo - assert!(builder + // add foreign UTXO + assert!(new_builder .add_foreign_utxo( utxo1.outpoint, psbt::Input { - non_witness_utxo: Some(tx1.as_ref().clone()), + non_witness_utxo: Some(tx1), ..Default::default() }, satisfaction_weight, ) .is_ok()); - let utxo_should_still_be_local = builder.params.utxos.values().collect::>()[0]; - - assert_eq!(builder.params.utxos.len(), 1); - assert_eq!(utxo_should_still_be_local.utxo.outpoint(), utxo1.outpoint); - // UTxO should still be LocalOutput + assert_eq!(new_builder.params.utxos.len(), 1); + assert_eq!(new_builder.params.utxos[0].utxo.outpoint(), utxo1.outpoint); + // UTXO should still be LocalOutput assert!(matches!( - utxo_should_still_be_local, + new_builder.params.utxos[0], WeightedUtxo { utxo: Utxo::Local(..), .. @@ -1290,14 +1343,16 @@ mod test { // In this test we are assuming a setup where there are two wallets using the same // descriptor, but only one is tracking transactions, while the other is not. // Within this conditions we want the second wallet to be able to consider the unknown - // LocalOutputs provided by the first wallet with greater precedence than any foreign utxo, - // even if the foreign utxo shares the same outpoint Remember the second wallet does not - // know about any UTxOs, so in principle, an unknown local utxo could be added as foreign. + // LocalOutputs provided by the first wallet with greater precedence than any foreign UTXO, + // even if the foreign UTXO shares the same outpoint. // - // In this case, the wallet adds a local utxo as if it were foreign and after this it adds - // it as local utxo. In this case the local utxo should still have precedence over the - // foreign utxo. - use crate::test_utils::{get_funded_wallet_wpkh, get_test_wpkh_and_change_desc}; + // Remember the second wallet does not know about any UTXOs, so in principle, an unknown + // local UTXO could be added as foreign. + // + // In this case, the wallet adds a local UTXO as if it were foreign and after this it adds + // it as local UTXO. In this case the local UTXO should still have precedence over the + // foreign UTXO. + use crate::test_utils::{get_funded_wallet_wpkh, get_test_wpkh_and_change_desc, insert_tx}; use bitcoin::Network; // Use the same wallet twice @@ -1310,47 +1365,64 @@ mod test { .expect("descriptors must be valid"); let utxo1 = wallet1.list_unspent().next().unwrap(); - let tx1 = wallet1.get_tx(txid1).unwrap().tx_node.tx.clone(); + let tx1 = wallet1.get_tx(txid1).unwrap().tx_node.as_ref().clone(); let satisfaction_weight = wallet1 .public_descriptor(KeychainKind::External) .max_weight_to_satisfy() .unwrap(); - let mut builder = wallet2.build_tx(); + // Here we are copying old_params to simulate, the very unlikely behavior where a wallet + // becomes aware of a local UTXO while it is creating a transaction using the local UTXO + // it just became aware of as a foreign UTXO + let old_params = { + let mut builder = wallet2.build_tx(); + + // add foreign UTXO + assert!(builder + .add_foreign_utxo( + utxo1.outpoint, + psbt::Input { + non_witness_utxo: Some(tx1.clone()), + ..Default::default() + }, + satisfaction_weight, + ) + .is_ok()); - // add foreign utxo - assert!(builder - .add_foreign_utxo( - utxo1.outpoint, - psbt::Input { - non_witness_utxo: Some(tx1.as_ref().clone()), - ..Default::default() - }, - satisfaction_weight, - ) - .is_ok()); + builder.params.clone() + }; + + // The wallet becomes aware of the new UTXO + insert_tx(&mut wallet2, tx1.clone()); - // Add local UTxO manually, through tx_builder private parameters and not through - // add_utxo method because we are assuming wallet2 has not knowledge of utxo1 yet - builder.params.utxos.insert( - utxo1.outpoint, + // Keep building the old transaction as we wouldn't have stopped of doing so + let mut new_builder = wallet2.build_tx(); + new_builder.params = old_params; + + // Check the foreign UTXO is still there + // UTXO should still be LocalOutput + assert!(matches!( + new_builder.params.utxos[0], WeightedUtxo { - satisfaction_weight: wallet1 - .public_descriptor(utxo1.keychain) - .max_weight_to_satisfy() - .unwrap(), - utxo: Utxo::Local(utxo1.clone()), - }, - ); + utxo: Utxo::Foreign { .. }, + .. + } + )); - let utxo_should_still_be_local = builder.params.utxos.values().collect::>()[0]; + // This method is the correct way of adding UTXOs to the builder. + // It checks the local availability of the UTXO, so a precondition for this method to work + // is that the transaction creating this UTXO must be already known by the wallet. + new_builder + .add_utxo(utxo1.outpoint) + .expect("should add local utxo"); - assert_eq!(builder.params.utxos.len(), 1); - assert_eq!(utxo_should_still_be_local.utxo.outpoint(), utxo1.outpoint); - // UTxO should still be LocalOutput + assert_eq!(new_builder.params.utxos.len(), 1); + assert_eq!(new_builder.params.utxos[0].utxo.outpoint(), utxo1.outpoint); + + // UTXO should still be LocalOutput assert!(matches!( - utxo_should_still_be_local, + new_builder.params.utxos[0], WeightedUtxo { utxo: Utxo::Local(..), .. diff --git a/wallet/tests/wallet.rs b/wallet/tests/wallet.rs index 8ed28159e..cba56980e 100644 --- a/wallet/tests/wallet.rs +++ b/wallet/tests/wallet.rs @@ -4699,3 +4699,62 @@ fn test_tx_details_method() { let tx_details_2_option = test_wallet.tx_details(txid_2); assert!(tx_details_2_option.is_none()); } + +#[test] +fn test_tx_ordering_untouched_preserves_insertion_ordering() { + let (mut wallet, txid) = get_funded_wallet_wpkh(); + let script_pubkey = wallet + .next_unused_address(KeychainKind::External) + .address + .script_pubkey(); + let tx1 = Transaction { + input: vec![TxIn { + previous_output: OutPoint { txid, vout: 0 }, + ..Default::default() + }], + output: vec![ + TxOut { + value: Amount::from_sat(500), + script_pubkey: script_pubkey.clone(), + }; + 4 + ], + ..new_tx(0) + }; + + insert_tx(&mut wallet, tx1); + let utxos = wallet + .list_unspent() + .map(|o| o.outpoint) + .take(2) + .collect::>(); + + let mut builder = wallet.build_tx(); + builder + .ordering(bdk_wallet::TxOrdering::Untouched) + .add_utxos(&utxos) + .unwrap() + .add_recipient(script_pubkey.clone(), Amount::from_sat(400)) + .add_recipient(script_pubkey.clone(), Amount::from_sat(300)) + .add_recipient(script_pubkey.clone(), Amount::from_sat(500)); + + let tx = builder.finish().unwrap().unsigned_tx; + let txins = tx + .input + .iter() + .take(2) // First two UTxOs should be manually selected and sorted by insertion + .map(|txin| txin.previous_output) + .collect::>(); + + assert!(txins == utxos); + + let txouts = tx + .output + .iter() + .take(3) // Exclude possible change output + .map(|txout| txout.value.to_sat()) + .collect::>(); + + // Check vout is sorted by recipient insertion order + assert!(txouts == vec![400, 300, 500]); +} From 73bef28489b0113e06acde7896b111b402692334 Mon Sep 17 00:00:00 2001 From: nymius <155548262+nymius@users.noreply.github.com> Date: Mon, 9 Jun 2025 11:03:35 -0300 Subject: [PATCH 2/3] doc(tx_builder): add info about manually selected UTxOs priority --- wallet/src/wallet/tx_builder.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/wallet/src/wallet/tx_builder.rs b/wallet/src/wallet/tx_builder.rs index 0ab1b9c0b..faa295805 100644 --- a/wallet/src/wallet/tx_builder.rs +++ b/wallet/src/wallet/tx_builder.rs @@ -276,6 +276,8 @@ impl<'a, Cs> TxBuilder<'a, 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. + /// + /// If a UTXO is inserted multiple times, only the final insertion will take effect. pub fn add_utxos(&mut self, outpoints: &[OutPoint]) -> Result<&mut Self, AddUtxoError> { // Canonicalize once, instead of once for every call to `get_utxo`. let unspent: HashMap = self @@ -327,6 +329,10 @@ impl<'a, Cs> TxBuilder<'a, Cs> { /// Add a foreign UTXO i.e. a UTXO not known by this wallet. /// + /// Foreign UTXOs are not prioritized over local UTXOs. If a local UTXO is added to the + /// manually selected list, it will replace any conflicting foreign UTXOs. However, a foreign + /// UTXO cannot replace a conflicting local UTXO. + /// /// There might be cases where the UTXO belongs to the wallet but it doesn't have knowledge of /// it. This is possible if the wallet is not synced or its not being use to track /// transactions. In those cases is the responsibility of the user to add any possible local From 0522114aa17c788d952ac60db8b26cb08ad1d871 Mon Sep 17 00:00:00 2001 From: valued mammal Date: Tue, 24 Jun 2025 12:01:03 -0400 Subject: [PATCH 3/3] test(tx_builder): update precedence check of local UTXOs over add_foreign_utxos --- wallet/src/wallet/tx_builder.rs | 191 ++++++-------------------------- 1 file changed, 35 insertions(+), 156 deletions(-) diff --git a/wallet/src/wallet/tx_builder.rs b/wallet/src/wallet/tx_builder.rs index faa295805..ff20fb39e 100644 --- a/wallet/src/wallet/tx_builder.rs +++ b/wallet/src/wallet/tx_builder.rs @@ -1259,180 +1259,59 @@ mod test { ); } + // Test that local outputs have precedence over utxos added via `add_foreign_utxo` #[test] - fn test_prexisting_local_utxo_have_precedence_over_foreign_utxo_with_same_outpoint() { - // In this test we are assuming a setup where there are two wallets using the same - // descriptor, but only one is tracking transactions, while the other is not. - // Within this conditions we want the second wallet to be able to consider the unknown - // LocalOutputs provided by the first wallet with greater precedence than any foreign UTXO, - // even if the foreign UTXO shares the same outpoint. - // - // Remember the second wallet does not know about any UTXOs, so in principle, an unknown - // local UTXO could be added as foreign. - // - // In this case, somehow the wallet has knowledge of one local UTXO and it tries to add the - // same UTXO as a foreign one, but the API ignores this, because local UTXOs have higher - // precedence. - use crate::test_utils::{get_funded_wallet_wpkh, get_test_wpkh_and_change_desc}; - use bitcoin::Network; - - // Use the same wallet twice - let (mut wallet1, txid1) = get_funded_wallet_wpkh(); - // But the second one has no knowledge of tx associated with txid1 - let (main_descriptor, change_descriptor) = get_test_wpkh_and_change_desc(); - let mut wallet2 = Wallet::create(main_descriptor, change_descriptor) - .network(Network::Regtest) - .create_wallet_no_persist() - .expect("descriptors must be valid"); - - let utxo1 = wallet1.list_unspent().next().unwrap(); - let tx1 = wallet1.get_tx(txid1).unwrap().tx_node.as_ref().clone(); - - let satisfaction_weight = wallet1 - .public_descriptor(KeychainKind::External) - .max_weight_to_satisfy() - .unwrap(); - - // Here we are copying old_params to simulate, the very unlikely behavior where a wallet - // becomes aware of a local UTXO while it is creating a transaction using the local UTXO - // it just became aware of as a foreign UTXO - let old_params = { - let mut builder = wallet1.build_tx(); - - builder - .add_utxo(utxo1.outpoint) - .expect("should add local utxo"); + fn test_local_utxos_have_precedence_over_foreign_utxos() { + use crate::test_utils::get_funded_wallet_wpkh; + let (mut wallet, _) = get_funded_wallet_wpkh(); - builder.params.clone() - }; + let utxo = wallet.list_unspent().next().unwrap(); + let outpoint = utxo.outpoint; - // Build a transaction as wallet2 was aware of utxo1 but not tracking transactions - let mut new_builder = wallet2.build_tx(); - new_builder.params = old_params; + // case 1: add foreign after local, expect local is kept + let mut builder = wallet.build_tx(); + builder.add_utxo(outpoint).unwrap(); - // Check the local UTXO is still there - // UTXO should still be LocalOutput - assert!(matches!( - new_builder.params.utxos[0], - WeightedUtxo { - utxo: Utxo::Local { .. }, - .. - } - )); + assert_eq!(builder.params.utxos[0].utxo.outpoint(), outpoint); - // add foreign UTXO - assert!(new_builder + builder .add_foreign_utxo( - utxo1.outpoint, + outpoint, psbt::Input { - non_witness_utxo: Some(tx1), + witness_utxo: Some(utxo.txout.clone()), ..Default::default() }, - satisfaction_weight, + Weight::from_wu(107), ) - .is_ok()); + .unwrap(); - assert_eq!(new_builder.params.utxos.len(), 1); - assert_eq!(new_builder.params.utxos[0].utxo.outpoint(), utxo1.outpoint); - // UTXO should still be LocalOutput + assert_eq!(builder.params.utxos.len(), 1); assert!(matches!( - new_builder.params.utxos[0], - WeightedUtxo { - utxo: Utxo::Local(..), - .. - } + &builder.params.utxos[0].utxo, + Utxo::Local(output) if output.outpoint == outpoint, )); - } - - #[test] - fn test_prexisting_foreign_utxo_have_no_precedence_over_local_utxo_with_same_outpoint() { - // In this test we are assuming a setup where there are two wallets using the same - // descriptor, but only one is tracking transactions, while the other is not. - // Within this conditions we want the second wallet to be able to consider the unknown - // LocalOutputs provided by the first wallet with greater precedence than any foreign UTXO, - // even if the foreign UTXO shares the same outpoint. - // - // Remember the second wallet does not know about any UTXOs, so in principle, an unknown - // local UTXO could be added as foreign. - // - // In this case, the wallet adds a local UTXO as if it were foreign and after this it adds - // it as local UTXO. In this case the local UTXO should still have precedence over the - // foreign UTXO. - use crate::test_utils::{get_funded_wallet_wpkh, get_test_wpkh_and_change_desc, insert_tx}; - use bitcoin::Network; - - // Use the same wallet twice - let (wallet1, txid1) = get_funded_wallet_wpkh(); - // But the second one has no knowledge of tx associated with txid1 - let (main_descriptor, change_descriptor) = get_test_wpkh_and_change_desc(); - let mut wallet2 = Wallet::create(main_descriptor, change_descriptor) - .network(Network::Regtest) - .create_wallet_no_persist() - .expect("descriptors must be valid"); - let utxo1 = wallet1.list_unspent().next().unwrap(); - let tx1 = wallet1.get_tx(txid1).unwrap().tx_node.as_ref().clone(); + // case 2: add local after foreign, expect foreign is removed + builder.params = TxParams::default(); - let satisfaction_weight = wallet1 - .public_descriptor(KeychainKind::External) - .max_weight_to_satisfy() + builder + .add_foreign_utxo( + outpoint, + psbt::Input { + witness_utxo: Some(utxo.txout), + ..Default::default() + }, + Weight::from_wu(107), + ) .unwrap(); - // Here we are copying old_params to simulate, the very unlikely behavior where a wallet - // becomes aware of a local UTXO while it is creating a transaction using the local UTXO - // it just became aware of as a foreign UTXO - let old_params = { - let mut builder = wallet2.build_tx(); - - // add foreign UTXO - assert!(builder - .add_foreign_utxo( - utxo1.outpoint, - psbt::Input { - non_witness_utxo: Some(tx1.clone()), - ..Default::default() - }, - satisfaction_weight, - ) - .is_ok()); + assert_eq!(builder.params.utxos[0].utxo.outpoint(), outpoint); - builder.params.clone() - }; - - // The wallet becomes aware of the new UTXO - insert_tx(&mut wallet2, tx1.clone()); - - // Keep building the old transaction as we wouldn't have stopped of doing so - let mut new_builder = wallet2.build_tx(); - new_builder.params = old_params; - - // Check the foreign UTXO is still there - // UTXO should still be LocalOutput - assert!(matches!( - new_builder.params.utxos[0], - WeightedUtxo { - utxo: Utxo::Foreign { .. }, - .. - } - )); + builder.add_utxo(outpoint).unwrap(); - // This method is the correct way of adding UTXOs to the builder. - // It checks the local availability of the UTXO, so a precondition for this method to work - // is that the transaction creating this UTXO must be already known by the wallet. - new_builder - .add_utxo(utxo1.outpoint) - .expect("should add local utxo"); - - assert_eq!(new_builder.params.utxos.len(), 1); - assert_eq!(new_builder.params.utxos[0].utxo.outpoint(), utxo1.outpoint); - - // UTXO should still be LocalOutput - assert!(matches!( - new_builder.params.utxos[0], - WeightedUtxo { - utxo: Utxo::Local(..), - .. - } - )); + assert_eq!(builder.params.utxos.len(), 1); + assert!( + matches!(&builder.params.utxos[0].utxo, Utxo::Local(output) if output.outpoint == outpoint) + ); } }