diff --git a/wallet/src/wallet/mod.rs b/wallet/src/wallet/mod.rs index be3773d34..330c9af7d 100644 --- a/wallet/src/wallet/mod.rs +++ b/wallet/src/wallet/mod.rs @@ -1511,7 +1511,11 @@ impl Wallet { let (required_utxos, optional_utxos) = { // NOTE: manual selection overrides unspendable - let mut required: Vec = params.utxos.values().cloned().collect(); + let mut required_to_sort: Vec<(u32, WeightedUtxo)> = + params.utxos.values().cloned().collect(); + required_to_sort.sort_by_key(|(k, _v)| *k); + let mut required: Vec = + required_to_sort.into_iter().map(|(_k, v)| v).collect(); let optional = self.filter_utxos(¶ms, current_height.to_consensus_u32()); // if drain_wallet is true, all UTxOs are required @@ -1720,7 +1724,8 @@ impl Wallet { let utxos = tx .input .drain(..) - .map(|txin| -> Result<_, BuildFeeBumpError> { + .zip(0u32..) + .map(|(txin, order)| -> Result<_, BuildFeeBumpError> { graph // Get previous transaction .get_tx(txin.previous_output.txid) @@ -1736,26 +1741,29 @@ impl Wallet { .get(txin.previous_output.vout as usize) .ok_or(BuildFeeBumpError::InvalidOutputIndex(txin.previous_output)) .cloned()?; - Ok((prev_tx, prev_txout, chain_position)) + Ok((prev_tx, prev_txout, chain_position, order)) }) - .map(|(prev_tx, prev_txout, chain_position)| { + .map(|(prev_tx, prev_txout, chain_position, order)| { 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, - }), - }, + ( + order, + 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( @@ -1765,27 +1773,32 @@ impl Wallet { ( 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() - }), + ( + order, + 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, }, - satisfaction_weight, - }, + ), ) } } }) }) - .collect::, BuildFeeBumpError>>()?; + .collect::, BuildFeeBumpError>>()?; if tx.output.len() > 1 { let mut change_index = None; @@ -2749,13 +2762,16 @@ mod test { 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), - }, + ( + 0, + WeightedUtxo { + satisfaction_weight: wallet + .public_descriptor(output.keychain) + .max_weight_to_satisfy() + .unwrap(), + utxo: Utxo::Local(output), + }, + ), ); // enforce selection of first output in transaction let received = wallet.filter_utxos(¶ms, wallet.latest_checkpoint().block_id().height); diff --git a/wallet/src/wallet/tx_builder.rs b/wallet/src/wallet/tx_builder.rs index 949cb7921..84f1b2363 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: HashMap, pub(crate) unspendable: HashSet, pub(crate) manually_selected_only: bool, pub(crate) sighash: Option, @@ -276,28 +276,41 @@ 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 order_max = self + .params + .utxos + .values() + .map(|(order, _)| order) + .max() + .unwrap_or(&0); let utxo_batch = outpoints .iter() - .map(|outpoint| { + .zip(1u32..) + .map(|(outpoint, counter)| { 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), - }, + ( + order_max + counter, + WeightedUtxo { + satisfaction_weight: self + .wallet + .public_descriptor(output.keychain) + .max_weight_to_satisfy() + .unwrap(), + utxo: Utxo::Local(output), + }, + ), ) }) }) - .collect::, AddUtxoError>>()?; + .collect::, AddUtxoError>>()?; self.params.utxos.extend(utxo_batch); Ok(self) @@ -313,6 +326,10 @@ impl<'a, Cs> TxBuilder<'a, Cs> { /// Add a foreign UTXO i.e. a UTXO not known by this wallet. /// + /// Foreign UTxOs do not take priority 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,23 +424,36 @@ impl<'a, Cs> TxBuilder<'a, Cs> { } } - if let Some(WeightedUtxo { - utxo: Utxo::Local { .. }, - .. - }) = self.params.utxos.get(&outpoint) + if let Some(( + _order, + WeightedUtxo { + utxo: Utxo::Local { .. }, + .. + }, + )) = self.params.utxos.get(&outpoint) { None } else { + let order_max = self + .params + .utxos + .values() + .map(|(order, _)| order) + .max() + .unwrap_or(&0); self.params.utxos.insert( outpoint, - WeightedUtxo { - satisfaction_weight, - utxo: Utxo::Foreign { - outpoint, - sequence, - psbt_input: Box::new(psbt_input), + ( + order_max + 1, + WeightedUtxo { + satisfaction_weight, + utxo: Utxo::Foreign { + outpoint, + sequence, + psbt_input: Box::new(psbt_input), + }, }, - }, + ), ) }; @@ -468,6 +498,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 @@ -777,7 +812,11 @@ pub enum TxOrdering { /// Randomized (default) #[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 { @@ -1142,14 +1181,18 @@ mod test { satisfaction_weight: Weight::from_wu(0), utxo: Utxo::Local(test_utxos[0].clone()), }; - for _ in 0..3 { + for order in 0..3 { params .utxos - .insert(test_utxos[0].outpoint, fake_weighted_utxo.clone()); + .insert(test_utxos[0].outpoint, (order, fake_weighted_utxo.clone())); } assert_eq!( vec![(test_utxos[0].outpoint, fake_weighted_utxo)], - params.utxos.into_iter().collect::>() + params + .utxos + .into_iter() + .map(|(k, (_order, v))| (k, v)) + .collect::>() ); } @@ -1202,7 +1245,7 @@ mod test { ) .is_ok()); - let foreign_utxo_with_modified_weight = + let (_order, foreign_utxo_with_modified_weight) = builder.params.utxos.values().collect::>()[0]; assert_eq!(builder.params.utxos.len(), 1); @@ -1250,13 +1293,16 @@ mod test { // add_utxo method because we are assuming wallet2 has not knowledge of utxo1 yet builder.params.utxos.insert( utxo1.outpoint, - WeightedUtxo { - satisfaction_weight: wallet1 - .public_descriptor(utxo1.keychain) - .max_weight_to_satisfy() - .unwrap(), - utxo: Utxo::Local(utxo1.clone()), - }, + ( + 0, + WeightedUtxo { + satisfaction_weight: wallet1 + .public_descriptor(utxo1.keychain) + .max_weight_to_satisfy() + .unwrap(), + utxo: Utxo::Local(utxo1.clone()), + }, + ), ); // add foreign utxo @@ -1271,7 +1317,8 @@ mod test { ) .is_ok()); - let utxo_should_still_be_local = builder.params.utxos.values().collect::>()[0]; + let (_order, 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); @@ -1335,16 +1382,20 @@ mod test { // add_utxo method because we are assuming wallet2 has not knowledge of utxo1 yet builder.params.utxos.insert( utxo1.outpoint, - WeightedUtxo { - satisfaction_weight: wallet1 - .public_descriptor(utxo1.keychain) - .max_weight_to_satisfy() - .unwrap(), - utxo: Utxo::Local(utxo1.clone()), - }, + ( + 0, + WeightedUtxo { + satisfaction_weight: wallet1 + .public_descriptor(utxo1.keychain) + .max_weight_to_satisfy() + .unwrap(), + utxo: Utxo::Local(utxo1.clone()), + }, + ), ); - let utxo_should_still_be_local = builder.params.utxos.values().collect::>()[0]; + let (_order, 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); diff --git a/wallet/tests/wallet.rs b/wallet/tests/wallet.rs index 8ed28159e..70bc5fc1b 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]); +}