From 79bd7da87603c8dfe65bee4bd7061c56e39d0953 Mon Sep 17 00:00:00 2001 From: nymius <155548262+nymius@users.noreply.github.com> Date: Mon, 13 Jan 2025 12:40:13 -0300 Subject: [PATCH 1/3] refactor(wallet): use iterators and adaptors in preselect_utxos There were multiple calls for de-duplication of selected UTxOs. As the test `test_filter_duplicates` shows, there are four possible cases for duplication of UTxOs while feeding the coin selection algorithms. 1. no duplication: out of concern 2. duplication in the required utxos only: covered by the source of `required_utxos`, `Wallet::list_unspent`, which roots back the provided `UTxOs` to a `HashMap` which should avoid any duplication by definition 3. duplication in the optional utxos only: is the only one possible as optional `UTxOs` are stored in a `Vec` and no checks are performed about the duplicity of their members. 4. duplication across the required and optional utxos: is already covered by `Wallet::preselect_utxos`, which avoid the processing of required UTxOs while listing the unspent available UTxOs in the wallet. This refactor changes: - `TxParams::utxos` type to be `HashSet` avoiding the duplication case 3, and allowing a query closer to O(1) on avg. to cover duplication case 4 (before was O(n) where n is the size of required utxos). - Moves the computation of the `WeightedUtxos` to the last part of UTxO filtering, allowing the unification of the computation for local outputs. - Removes some extra allocations done for helpers structures or intermediate results while filtering UTxOs. - Allows for future integration of UTxO filtering methods for other utilities. - Adds more comments for each filtering step. With these changes all four cases would be covered, and `coin_selection::filter_duplicates` would be no longer needed. --- crates/wallet/src/wallet/mod.rs | 284 ++++++++++------------- crates/wallet/src/wallet/tx_builder.rs | 304 ++++++++++++++++++++++--- 2 files changed, 393 insertions(+), 195 deletions(-) diff --git a/crates/wallet/src/wallet/mod.rs b/crates/wallet/src/wallet/mod.rs index 6e00b1094..2626a583c 100644 --- a/crates/wallet/src/wallet/mod.rs +++ b/crates/wallet/src/wallet/mod.rs @@ -36,7 +36,7 @@ use bdk_chain::{ use bitcoin::{ absolute, consensus::encode::serialize, - constants::{genesis_block, COINBASE_MATURITY}, + constants::genesis_block, psbt, secp256k1::Secp256k1, sighash::{EcdsaSighashType, TapSighashType}, @@ -1417,8 +1417,19 @@ impl Wallet { fee_amount += fee_rate * tx.weight(); - let (required_utxos, optional_utxos) = - self.preselect_utxos(¶ms, Some(current_height.to_consensus_u32())); + let (required_utxos, optional_utxos) = { + // NOTE: manual selection overrides unspendable + let mut required: Vec = params.utxos.values().cloned().collect(); + let optional = self.filter_utxos(¶ms, current_height.to_consensus_u32()); + + // if drain_wallet is true, all UTxOs are required + if params.drain_wallet { + required.extend(optional); + (required, vec![]) + } else { + (required, optional) + } + }; // get drain script let mut drain_index = Option::<(KeychainKind, u32)>::None; @@ -1447,9 +1458,6 @@ impl Wallet { } }; - let (required_utxos, optional_utxos) = - coin_selection::filter_duplicates(required_utxos, optional_utxos); - let coin_selection = coin_selection .coin_select( required_utxos, @@ -1618,60 +1626,71 @@ impl Wallet { .map_err(|_| BuildFeeBumpError::FeeRateUnavailable)?; // remove the inputs from the tx and process them - let original_txin = tx.input.drain(..).collect::>(); - let original_utxos = original_txin - .iter() + let utxos = tx + .input + .drain(..) .map(|txin| -> Result<_, BuildFeeBumpError> { - let prev_tx = graph + graph + // Get previous transaction .get_tx(txin.previous_output.txid) - .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output))?; - let txout = &prev_tx.output[txin.previous_output.vout as usize]; - - let chain_position = chain_positions - .get(&txin.previous_output.txid) - .cloned() - .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output))?; - - let weighted_utxo = match txout_index.index_of_spk(txout.script_pubkey.clone()) { - Some(&(keychain, derivation_index)) => { - let satisfaction_weight = self - .public_descriptor(keychain) - .max_weight_to_satisfy() - .unwrap(); - WeightedUtxo { - utxo: Utxo::Local(LocalOutput { - outpoint: txin.previous_output, - txout: txout.clone(), - keychain, - is_spent: true, - derivation_index, - chain_position, - }), - satisfaction_weight, - } - } - None => { - let satisfaction_weight = Weight::from_wu_usize( - serialize(&txin.script_sig).len() * 4 + serialize(&txin.witness).len(), - ); - WeightedUtxo { - utxo: Utxo::Foreign { - outpoint: txin.previous_output, - sequence: txin.sequence, - psbt_input: Box::new(psbt::Input { - witness_utxo: Some(txout.clone()), - non_witness_utxo: Some(prev_tx.as_ref().clone()), - ..Default::default() - }), - }, - satisfaction_weight, + .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output)) + // Get chain position + .and_then(|prev_tx| { + chain_positions + .get(&txin.previous_output.txid) + .cloned() + .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output)) + .map(|chain_position| (prev_tx, chain_position)) + }) + .map(|(prev_tx, chain_position)| { + let txout = prev_tx.output[txin.previous_output.vout as usize].clone(); + match txout_index.index_of_spk(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: 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: txout + .script_pubkey + .witness_version() + .map(|_| txout.clone()), + non_witness_utxo: Some(prev_tx.as_ref().clone()), + ..Default::default() + }), + }, + satisfaction_weight, + }, + ) + } } - } - }; - - Ok(weighted_utxo) + }) }) - .collect::, _>>()?; + .collect::, BuildFeeBumpError>>()?; if tx.output.len() > 1 { let mut change_index = None; @@ -1698,7 +1717,7 @@ impl Wallet { .into_iter() .map(|txout| (txout.script_pubkey, txout.value)) .collect(), - utxos: original_utxos, + utxos, bumping_fee: Some(tx_builder::PreviousFee { absolute: fee, rate: fee_rate, @@ -1976,117 +1995,52 @@ impl Wallet { descriptor.at_derivation_index(child).ok() } - fn get_available_utxos(&self) -> Vec<(LocalOutput, Weight)> { - self.list_unspent() - .map(|utxo| { - let keychain = utxo.keychain; - (utxo, { - self.public_descriptor(keychain) - .max_weight_to_satisfy() - .unwrap() - }) - }) - .collect() - } - /// Given the options returns the list of utxos that must be used to form the /// transaction and any further that may be used if needed. - fn preselect_utxos( - &self, - params: &TxParams, - current_height: Option, - ) -> (Vec, Vec) { - let TxParams { - change_policy, - unspendable, - utxos, - drain_wallet, - manually_selected_only, - bumping_fee, - .. - } = params; - - let manually_selected = utxos.clone(); - // we mandate confirmed transactions if we're bumping the fee - let must_only_use_confirmed_tx = bumping_fee.is_some(); - let must_use_all_available = *drain_wallet; - - // must_spend <- manually selected utxos - // may_spend <- all other available utxos - let mut may_spend = self.get_available_utxos(); - - may_spend.retain(|may_spend| { - !manually_selected - .iter() - .any(|manually_selected| manually_selected.utxo.outpoint() == may_spend.0.outpoint) - }); - let mut must_spend = manually_selected; - - // NOTE: we are intentionally ignoring `unspendable` here. i.e manual - // selection overrides unspendable. - if *manually_selected_only { - return (must_spend, vec![]); - } - - let satisfies_confirmed = may_spend - .iter() - .map(|u| -> bool { - let txid = u.0.outpoint.txid; - let tx = match self.indexed_graph.graph().get_tx(txid) { - Some(tx) => tx, - None => return false, - }; - - // Whether the UTXO is mature and, if needed, confirmed - let mut spendable = true; - let chain_position = u.0.chain_position; - if must_only_use_confirmed_tx && !chain_position.is_confirmed() { - return false; - } - if tx.is_coinbase() { - debug_assert!( - chain_position.is_confirmed(), - "coinbase must always be confirmed" - ); - if let Some(current_height) = current_height { - match chain_position { - ChainPosition::Confirmed { anchor, .. } => { - // https://github.com/bitcoin/bitcoin/blob/c5e67be03bb06a5d7885c55db1f016fbf2333fe3/src/validation.cpp#L373-L375 - let spend_height = current_height + 1; - let coin_age_at_spend_height = - spend_height.saturating_sub(anchor.block_id.height); - spendable &= coin_age_at_spend_height >= COINBASE_MATURITY; - } - ChainPosition::Unconfirmed { .. } => spendable = false, - } - } - } - spendable - }) - .collect::>(); - - let mut i = 0; - may_spend.retain(|u| { - let retain = (self.keychains().count() == 1 || change_policy.is_satisfied_by(&u.0)) - && !unspendable.contains(&u.0.outpoint) - && satisfies_confirmed[i]; - i += 1; - retain - }); - - let mut may_spend = may_spend - .into_iter() - .map(|(local_utxo, satisfaction_weight)| WeightedUtxo { - satisfaction_weight, - utxo: Utxo::Local(local_utxo), - }) - .collect(); - - if must_use_all_available { - must_spend.append(&mut may_spend); + fn filter_utxos(&self, params: &TxParams, current_height: u32) -> Vec { + if params.manually_selected_only { + vec![] + // only process optional UTxOs if manually_selected_only is false + } else { + self.indexed_graph + .graph() + // get all unspent UTxOs from wallet + // NOTE: the UTxOs returned by the following method already belong to wallet as the + // call chain uses get_tx_node infallibly + .filter_chain_unspents( + &self.chain, + self.chain.tip().block_id(), + self.indexed_graph.index.outpoints().iter().cloned(), + ) + // only create LocalOutput if UTxO is mature + .filter_map(move |((k, i), full_txo)| { + full_txo + .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 add to optional UTxOs those which satisfy the change policy if we reuse change + .filter(|local_output| { + self.keychains().count() == 1 + || params.change_policy.is_satisfied_by(local_output) + }) + // only add to optional UTxOs those marked as spendable + .filter(|local_output| !params.unspendable.contains(&local_output.outpoint)) + // if bumping fees only add to optional UTxOs those confirmed + .filter(|local_output| { + params.bumping_fee.is_none() || local_output.chain_position.is_confirmed() + }) + .map(|utxo| WeightedUtxo { + satisfaction_weight: self + .public_descriptor(utxo.keychain) + .max_weight_to_satisfy() + .unwrap(), + utxo: Utxo::Local(utxo), + }) + .collect() } - - (must_spend, may_spend) } fn complete_transaction( diff --git a/crates/wallet/src/wallet/tx_builder.rs b/crates/wallet/src/wallet/tx_builder.rs index da41c6b00..235987fc0 100644 --- a/crates/wallet/src/wallet/tx_builder.rs +++ b/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 @@ -125,7 +125,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: Vec, + pub(crate) utxos: HashMap, pub(crate) unspendable: HashSet, pub(crate) manually_selected_only: bool, pub(crate) sighash: Option, @@ -274,26 +274,28 @@ 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. pub fn add_utxos(&mut self, outpoints: &[OutPoint]) -> Result<&mut Self, AddUtxoError> { - { - let wallet = &mut self.wallet; - let utxos = outpoints - .iter() - .map(|outpoint| { - wallet - .get_utxo(*outpoint) - .ok_or(AddUtxoError::UnknownUtxo(*outpoint)) - }) - .collect::, _>>()?; - - for utxo in utxos { - let descriptor = wallet.public_descriptor(utxo.keychain); - let satisfaction_weight = descriptor.max_weight_to_satisfy().unwrap(); - self.params.utxos.push(WeightedUtxo { - satisfaction_weight, - utxo: Utxo::Local(utxo), - }); - } - } + let utxo_batch = 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), + }, + ) + }) + }) + .collect::, AddUtxoError>>()?; + self.params.utxos.extend(utxo_batch); Ok(self) } @@ -306,7 +308,14 @@ impl<'a, Cs> TxBuilder<'a, Cs> { self.add_utxos(&[outpoint]) } - /// Add a foreign UTXO i.e. a UTXO not owned by this wallet. + /// 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 + /// 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. /// /// At a minimum to add a foreign UTXO we need: /// @@ -393,14 +402,25 @@ impl<'a, Cs> TxBuilder<'a, Cs> { } } - self.params.utxos.push(WeightedUtxo { - satisfaction_weight, - utxo: Utxo::Foreign { + if let Some(WeightedUtxo { + utxo: Utxo::Local { .. }, + .. + }) = self.params.utxos.get(&outpoint) + { + None + } else { + self.params.utxos.insert( outpoint, - sequence, - psbt_input: Box::new(psbt_input), - }, - }); + WeightedUtxo { + satisfaction_weight, + utxo: Utxo::Foreign { + outpoint, + sequence, + psbt_input: Box::new(psbt_input), + }, + }, + ) + }; Ok(self) } @@ -1098,4 +1118,228 @@ mod test { builder.fee_rate(FeeRate::from_sat_per_kwu(feerate + 250)); let _ = builder.finish().unwrap(); } + + #[test] + fn not_duplicated_utxos_in_required_list() { + let mut params = TxParams::default(); + let test_utxos = get_test_utxos(); + let fake_weighted_utxo = WeightedUtxo { + satisfaction_weight: Weight::from_wu(0), + utxo: Utxo::Local(test_utxos[0].clone()), + }; + for _ in 0..3 { + params + .utxos + .insert(test_utxos[0].outpoint, fake_weighted_utxo.clone()); + } + assert_eq!( + vec![(test_utxos[0].outpoint, fake_weighted_utxo)], + params.utxos.into_iter().collect::>() + ); + } + + #[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()); + + 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, + modified_satisfaction_weight + ); + } + + #[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 (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.tx.clone(); + + let satisfaction_weight = wallet1 + .public_descriptor(KeychainKind::External) + .max_weight_to_satisfy() + .unwrap(); + + let mut builder = wallet2.build_tx(); + + // 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, + WeightedUtxo { + satisfaction_weight: wallet1 + .public_descriptor(utxo1.keychain) + .max_weight_to_satisfy() + .unwrap(), + utxo: Utxo::Local(utxo1.clone()), + }, + ); + + // 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()); + + 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!(matches!( + utxo_should_still_be_local, + WeightedUtxo { + utxo: Utxo::Local(..), + .. + } + )); + } + + #[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}; + 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.tx.clone(); + + let satisfaction_weight = wallet1 + .public_descriptor(KeychainKind::External) + .max_weight_to_satisfy() + .unwrap(); + + let mut builder = wallet2.build_tx(); + + // 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()); + + // 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, + 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]; + + 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, + WeightedUtxo { + utxo: Utxo::Local(..), + .. + } + )); + } } From 39df2b940a161d6b5468e6a6c36f888c14a3e24e Mon Sep 17 00:00:00 2001 From: nymius <155548262+nymius@users.noreply.github.com> Date: Mon, 13 Jan 2025 16:52:29 -0300 Subject: [PATCH 2/3] refactor(wallet): remove coin_selection::filter_duplicates --- crates/wallet/src/wallet/coin_selection.rs | 121 +-------------------- 1 file changed, 2 insertions(+), 119 deletions(-) diff --git a/crates/wallet/src/wallet/coin_selection.rs b/crates/wallet/src/wallet/coin_selection.rs index 4429fae73..e5654fb4d 100644 --- a/crates/wallet/src/wallet/coin_selection.rs +++ b/crates/wallet/src/wallet/coin_selection.rs @@ -101,7 +101,6 @@ //! # Ok::<(), anyhow::Error>(()) //! ``` -use crate::chain::collections::HashSet; use crate::wallet::utils::IsDust; use crate::Utxo; use crate::WeightedUtxo; @@ -109,7 +108,6 @@ use bitcoin::{Amount, FeeRate, SignedAmount}; use alloc::vec::Vec; use bitcoin::consensus::encode::serialize; -use bitcoin::OutPoint; use bitcoin::TxIn; use bitcoin::{Script, Weight}; @@ -720,30 +718,12 @@ fn calculate_cs_result( } } -/// Remove duplicate UTXOs. -/// -/// If a UTXO appears in both `required` and `optional`, the appearance in `required` is kept. -pub(crate) fn filter_duplicates(required: I, optional: I) -> (I, I) -where - I: IntoIterator + FromIterator, -{ - let mut visited = HashSet::::new(); - let required = required - .into_iter() - .filter(|utxo| visited.insert(utxo.utxo.outpoint())) - .collect::(); - let optional = optional - .into_iter() - .filter(|utxo| visited.insert(utxo.utxo.outpoint())) - .collect::(); - (required, optional) -} - #[cfg(test)] mod test { use assert_matches::assert_matches; use bitcoin::hashes::Hash; - use chain::{BlockId, ChainPosition, ConfirmationBlockTime}; + use bitcoin::OutPoint; + use chain::{ChainPosition, ConfirmationBlockTime}; use core::str::FromStr; use rand::rngs::StdRng; @@ -751,7 +731,6 @@ mod test { use super::*; use crate::types::*; - use crate::wallet::coin_selection::filter_duplicates; use rand::prelude::SliceRandom; use rand::{thread_rng, Rng, RngCore, SeedableRng}; @@ -1618,102 +1597,6 @@ mod test { assert_eq!(res.selected_amount(), Amount::from_sat(200_000)); } - #[test] - fn test_filter_duplicates() { - fn utxo(txid: &str, value: u64) -> WeightedUtxo { - WeightedUtxo { - satisfaction_weight: Weight::ZERO, - utxo: Utxo::Local(LocalOutput { - outpoint: OutPoint::new(bitcoin::hashes::Hash::hash(txid.as_bytes()), 0), - txout: TxOut { - value: Amount::from_sat(value), - script_pubkey: ScriptBuf::new(), - }, - keychain: KeychainKind::External, - is_spent: false, - derivation_index: 0, - chain_position: ChainPosition::Confirmed { - anchor: ConfirmationBlockTime { - block_id: BlockId { - height: 12345, - hash: BlockHash::all_zeros(), - }, - confirmation_time: 12345, - }, - transitively: None, - }, - }), - } - } - - fn to_utxo_vec(utxos: &[(&str, u64)]) -> Vec { - let mut v = utxos - .iter() - .map(|&(txid, value)| utxo(txid, value)) - .collect::>(); - v.sort_by_key(|u| u.utxo.outpoint()); - v - } - - struct TestCase<'a> { - name: &'a str, - required: &'a [(&'a str, u64)], - optional: &'a [(&'a str, u64)], - exp_required: &'a [(&'a str, u64)], - exp_optional: &'a [(&'a str, u64)], - } - - let test_cases = [ - TestCase { - name: "no_duplicates", - required: &[("A", 1000), ("B", 2100)], - optional: &[("C", 1000)], - exp_required: &[("A", 1000), ("B", 2100)], - exp_optional: &[("C", 1000)], - }, - TestCase { - name: "duplicate_required_utxos", - required: &[("A", 3000), ("B", 1200), ("C", 1234), ("A", 3000)], - optional: &[("D", 2100)], - exp_required: &[("A", 3000), ("B", 1200), ("C", 1234)], - exp_optional: &[("D", 2100)], - }, - TestCase { - name: "duplicate_optional_utxos", - required: &[("A", 3000), ("B", 1200)], - optional: &[("C", 5000), ("D", 1300), ("C", 5000)], - exp_required: &[("A", 3000), ("B", 1200)], - exp_optional: &[("C", 5000), ("D", 1300)], - }, - TestCase { - name: "duplicate_across_required_and_optional_utxos", - required: &[("A", 3000), ("B", 1200), ("C", 2100)], - optional: &[("A", 3000), ("D", 1200), ("E", 5000)], - exp_required: &[("A", 3000), ("B", 1200), ("C", 2100)], - exp_optional: &[("D", 1200), ("E", 5000)], - }, - ]; - - for (i, t) in test_cases.into_iter().enumerate() { - let (required, optional) = - filter_duplicates(to_utxo_vec(t.required), to_utxo_vec(t.optional)); - assert_eq!( - required, - to_utxo_vec(t.exp_required), - "[{}:{}] unexpected `required` result", - i, - t.name - ); - assert_eq!( - optional, - to_utxo_vec(t.exp_optional), - "[{}:{}] unexpected `optional` result", - i, - t.name - ); - } - } - #[test] fn test_deterministic_coin_selection_picks_same_utxos() { enum CoinSelectionAlgo { From 2f83b4508fe2369c6336c9579517f3a4eb25d923 Mon Sep 17 00:00:00 2001 From: nymius <155548262+nymius@users.noreply.github.com> Date: Thu, 23 Jan 2025 13:59:38 -0300 Subject: [PATCH 3/3] test(wallet): check there are no duplicates across required and optional utxos This test replaces the one used to test `coin_selection::filter_duplicates` introduced in 5299db34cb9117ad1b66a6afcb51f6ca7e1f0d95. As the code changed and there is not a single point to verificate the following properties: - there are no duplicates in required utxos - there are no duplicates in optional utxos - there are no duplicates across optional and required utxos anymore, test have been prefixed with `not_duplicated_utxos*` to allow its joint execution by using the following command: cargo test -- not_duplicated_utxos --- crates/wallet/src/wallet/mod.rs | 70 +++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/crates/wallet/src/wallet/mod.rs b/crates/wallet/src/wallet/mod.rs index 2626a583c..e4dc6d056 100644 --- a/crates/wallet/src/wallet/mod.rs +++ b/crates/wallet/src/wallet/mod.rs @@ -2559,3 +2559,73 @@ macro_rules! doctest_wallet { wallet }} } + +#[cfg(test)] +mod test { + use super::*; + use crate::test_utils::get_test_tr_single_sig_xprv_and_change_desc; + use crate::test_utils::insert_tx; + + #[test] + fn not_duplicated_utxos_across_optional_and_required() { + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + + // create new wallet + let mut wallet = Wallet::create(external_desc, internal_desc) + .network(Network::Testnet) + .create_wallet_no_persist() + .unwrap(); + + let two_output_tx = Transaction { + input: vec![], + output: vec![ + TxOut { + script_pubkey: wallet + .next_unused_address(KeychainKind::External) + .script_pubkey(), + value: Amount::from_sat(25_000), + }, + TxOut { + script_pubkey: wallet + .next_unused_address(KeychainKind::External) + .script_pubkey(), + value: Amount::from_sat(75_000), + }, + ], + version: transaction::Version::non_standard(0), + lock_time: absolute::LockTime::ZERO, + }; + + 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), + }, + ); + // 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 + // filtered out + let expected = vec![wallet + .get_utxo(OutPoint { txid, vout: 1 }) + .map(|utxo| WeightedUtxo { + satisfaction_weight: wallet + .public_descriptor(utxo.keychain) + .max_weight_to_satisfy() + .unwrap(), + utxo: Utxo::Local(utxo), + }) + .unwrap()]; + + assert_eq!(expected, received); + } +}