diff --git a/wallet/src/wallet/mod.rs b/wallet/src/wallet/mod.rs index be3773d3..6c45675a 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 @@ -2117,8 +2115,9 @@ impl Wallet { .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)) + // 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 949cb792..fe11a308 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, @@ -276,29 +276,45 @@ 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> { - 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), - }, - ) - }) + .map(|&op| -> Result<_, AddUtxoError> { + visited.insert(op); + 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) } @@ -313,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 @@ -407,25 +427,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 collisioning_foreign_utxo_idx: 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 { .. } => { + collisioning_foreign_utxo_idx = Some(idx); + break; + } + } + } + } + + if let Some(idx) = collisioning_foreign_utxo_idx { + 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 +495,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 +810,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 { @@ -1136,21 +1174,19 @@ mod test { #[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] @@ -1202,12 +1238,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 ); } @@ -1218,8 +1251,10 @@ mod test { // 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. + // 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 @@ -1228,7 +1263,7 @@ mod test { 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 +1272,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 + 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); + 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(..), .. @@ -1291,13 +1336,15 @@ mod test { // 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. + // 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}; + 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 +1357,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()); + + // 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; - // 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, + // 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!(new_builder.params.utxos.len(), 1); + assert_eq!(new_builder.params.utxos[0].utxo.outpoint(), utxo1.outpoint); - assert_eq!(builder.params.utxos.len(), 1); - assert_eq!(utxo_should_still_be_local.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 8ed28159..70bc5fc1 100644 --- a/wallet/tests/wallet.rs +++ b/wallet/tests/wallet.rs @@ -4699,3 +4699,73 @@ 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) + }; + + let txid1 = tx1.compute_txid(); + insert_tx(&mut wallet, tx1); + insert_anchor( + &mut wallet, + txid1, + ConfirmationBlockTime { + block_id: BlockId { + height: 2_100, + hash: BlockHash::all_zeros(), + }, + confirmation_time: 300, + }, + ); + 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]); +}