-
Notifications
You must be signed in to change notification settings - Fork 417
Description
Description
Wallet::create_tx is removing duplicated utxos twice.
Wallet::preselect_utxos is used in Wallet::create_tx to get the required and optional utxos. Notice that Wallet::preselect_utxos already filters required utxos from the list of optional ones. However, a second filtering is performed anyways with coin_selection::filter_duplicates a few lines later in the same Wallet::create_tx. I think one of the two is redundant.
I created a test to assert what I'm referring here. It changes some parts of the API visibility to allow the isolation of the referred function calls.
diff --git a/crates/wallet/src/wallet/coin_selection.rs b/crates/wallet/src/wallet/coin_selection.rs
index 4429fae7..bbad2d84 100644
--- a/crates/wallet/src/wallet/coin_selection.rs
+++ b/crates/wallet/src/wallet/coin_selection.rs
@@ -723,7 +723,7 @@ 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<I>(required: I, optional: I) -> (I, I)
+pub fn filter_duplicates<I>(required: I, optional: I) -> (I, I)
where
I: IntoIterator<Item = WeightedUtxo> + FromIterator<WeightedUtxo>,
{
diff --git a/crates/wallet/src/wallet/mod.rs b/crates/wallet/src/wallet/mod.rs
index 2e068a95..fea34bd8 100644
--- a/crates/wallet/src/wallet/mod.rs
+++ b/crates/wallet/src/wallet/mod.rs
@@ -1989,7 +1989,7 @@ impl Wallet {
/// 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(
+ pub fn preselect_utxos(
&self,
params: &TxParams,
current_height: Option<u32>,
diff --git a/crates/wallet/src/wallet/tx_builder.rs b/crates/wallet/src/wallet/tx_builder.rs
index 868d51df..aeb76d3b 100644
--- a/crates/wallet/src/wallet/tx_builder.rs
+++ b/crates/wallet/src/wallet/tx_builder.rs
@@ -111,21 +111,21 @@ use crate::{KeychainKind, LocalOutput, Utxo, WeightedUtxo};
#[derive(Debug)]
pub struct TxBuilder<'a, Cs> {
pub(crate) wallet: &'a mut Wallet,
- pub(crate) params: TxParams,
+ pub params: TxParams,
pub(crate) coin_selection: Cs,
}
/// The parameters for transaction creation sans coin selection algorithm.
//TODO: TxParams should eventually be exposed publicly.
#[derive(Default, Debug, Clone)]
-pub(crate) struct TxParams {
+pub struct TxParams {
pub(crate) recipients: Vec<(ScriptBuf, Amount)>,
pub(crate) drain_wallet: bool,
pub(crate) drain_to: Option<ScriptBuf>,
pub(crate) fee_policy: Option<FeePolicy>,
pub(crate) internal_policy_path: Option<BTreeMap<String, Vec<usize>>>,
pub(crate) external_policy_path: Option<BTreeMap<String, Vec<usize>>>,
- pub(crate) utxos: Vec<WeightedUtxo>,
+ pub utxos: Vec<WeightedUtxo>,
pub(crate) unspendable: HashSet<OutPoint>,
pub(crate) manually_selected_only: bool,
pub(crate) sighash: Option<psbt::PsbtSighashType>,
diff --git a/crates/wallet/tests/wallet.rs b/crates/wallet/tests/wallet.rs
index dcc8030b..b26c711a 100644
--- a/crates/wallet/tests/wallet.rs
+++ b/crates/wallet/tests/wallet.rs
@@ -819,6 +819,44 @@ fn test_create_tx_drain_wallet_and_drain_to_and_with_recipient() {
);
}
+#[test]
+fn filter_candidate_utxos_twice() {
+ use bdk_wallet::TxParams;
+ use bdk_wallet::{Utxo, WeightedUtxo};
+ let (wallet, _) = get_funded_wallet_wpkh();
+
+ let ((required_v1, optional_v1), last_utxo) = {
+ let utxos: Vec<_> = wallet.list_unspent().map(|u| u.outpoint).collect();
+ let mut params = TxParams::default();
+ let last_utxo = wallet.get_utxo(*utxos.last().unwrap()).unwrap();
+ let descriptor = wallet.public_descriptor(last_utxo.keychain);
+ let satisfaction_weight = descriptor.max_weight_to_satisfy().unwrap();
+ params.utxos.push(WeightedUtxo {
+ satisfaction_weight,
+ utxo: Utxo::Local(last_utxo.clone()),
+ });
+ (wallet.preselect_utxos(¶ms, None), last_utxo)
+ };
+
+ let ((required_v3, optional_v3), optional_v2) = {
+ use bdk_wallet::coin_selection;
+
+ let descriptor = wallet.public_descriptor(last_utxo.keychain);
+ let satisfaction_weight = descriptor.max_weight_to_satisfy().unwrap();
+ let mut optional_v2 = optional_v1.clone();
+ optional_v2.push(WeightedUtxo {
+ satisfaction_weight,
+ utxo: Utxo::Local(last_utxo),
+ });
+ (coin_selection::filter_duplicates(required_v1.clone(), optional_v2.clone()), optional_v2)
+ };
+
+ assert_eq!(required_v1, required_v3);
+ assert!(optional_v1 != optional_v2);
+ assert_eq!(optional_v1, optional_v3);
+}
+
+
#[test]
fn test_create_tx_drain_to_and_utxos() {
let (mut wallet, _) = get_funded_wallet_wpkh();
Probably the only case where the second coin_selection::filter_duplicate is triggered is when drain_wallet is true. I think this can be refactored to avoid the burden for all the other cases.
Metadata
Metadata
Assignees
Labels
Type
Projects
Status