From: nymius <155548262+nymius@users.noreply.github.com> Date: Mon, 13 Jan 2025 15:40:13 +0000 (-0300) Subject: refactor(wallet): use iterators and adaptors in preselect_utxos X-Git-Tag: wallet-1.2.0~19^2~2 X-Git-Url: http://internal-gitweb-vhost/script/%22https:/struct.EncoderStringWriter.html?a=commitdiff_plain;h=79bd7da87603c8dfe65bee4bd7061c56e39d0953;p=bdk 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. --- diff --git a/crates/wallet/src/wallet/mod.rs b/crates/wallet/src/wallet/mod.rs index 6e00b109..2626a583 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 da41c6b0..235987fc 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(..), + .. + } + )); + } }