-
Notifications
You must be signed in to change notification settings - Fork 82
fix(wallet): retry coin selection on NoChange to avoid dust/zero drai… #448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
55d050d
98f32a8
235d336
11ab2d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1467,16 +1467,37 @@ impl Wallet { | |
| } | ||
| }; | ||
|
|
||
| let coin_selection = coin_selection | ||
| .coin_select( | ||
| required_utxos, | ||
| optional_utxos, | ||
| fee_rate, | ||
| outgoing + fee_amount, | ||
| &drain_script, | ||
| rng, | ||
| ) | ||
| .map_err(CreateTxError::CoinSelection)?; | ||
| // Retry coin selection to avoid dust/zero drain outputs (see issue #376). If the | ||
| // selection yields NoChange the loop promotes an optional UTXO to required and retries; | ||
| // it exits when optional_remaining is exhausted or a viable drain output is found. | ||
| let should_retry_for_dust_drain = params.recipients.is_empty() | ||
| && params.drain_to.is_some() | ||
| && (params.drain_wallet || !params.utxos.is_empty()) | ||
| && !params.manually_selected_only; | ||
|
|
||
| let mut required_for_attempt = required_utxos; | ||
| let mut optional_remaining = optional_utxos; | ||
| let coin_selection = loop { | ||
| let result = coin_selection | ||
| .coin_select( | ||
| required_for_attempt.clone(), | ||
| optional_remaining.clone(), | ||
| fee_rate, | ||
| outgoing + fee_amount, | ||
| &drain_script, | ||
| rng, | ||
| ) | ||
| .map_err(CreateTxError::CoinSelection)?; | ||
|
|
||
| if !should_retry_for_dust_drain || !matches!(&result.excess, Excess::NoChange { .. }) { | ||
| break result; | ||
| } | ||
|
|
||
| let Some(w) = optional_remaining.pop() else { | ||
| break result; | ||
| }; | ||
| required_for_attempt.push(w); | ||
| }; | ||
|
|
||
| let excess = &coin_selection.excess; | ||
| tx.input = coin_selection | ||
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| use bdk_wallet::test_utils::*; | ||
| use bdk_wallet::KeychainKind; | ||
| use bitcoin::{hashes::Hash, psbt, Amount, OutPoint, ScriptBuf, TxOut, Weight}; | ||
|
|
||
| // Ensures coin selection pulls a local UTXO when drain-only selection would produce dust. | ||
| #[test] | ||
| fn test_drain_to_pulls_local_utxo_when_foreign_only_dust() { | ||
| let (mut wallet, _) = get_funded_wallet_wpkh(); | ||
| let drain_spk = wallet | ||
| .next_unused_address(KeychainKind::External) | ||
| .script_pubkey(); | ||
|
|
||
| let witness_utxo = TxOut { | ||
| value: Amount::from_sat(500), | ||
| script_pubkey: ScriptBuf::new_p2a(), | ||
| }; | ||
| // Remember to include this as a "floating" txout in the wallet. | ||
| let outpoint = OutPoint::new(Hash::hash(b"foreign-p2a-prev"), 1); | ||
| wallet.insert_txout(outpoint, witness_utxo.clone()); | ||
| let satisfaction_weight = Weight::from_wu(71); | ||
| let psbt_input = psbt::Input { | ||
| witness_utxo: Some(witness_utxo), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let mut tx_builder = wallet.build_tx(); | ||
| tx_builder | ||
| .add_foreign_utxo(outpoint, psbt_input, satisfaction_weight) | ||
| .unwrap() | ||
| .only_witness_utxo() | ||
| .fee_absolute(Amount::from_sat(400)) | ||
| .drain_to(drain_spk); | ||
|
|
||
| let psbt = tx_builder.finish().unwrap(); | ||
| let tx = psbt.unsigned_tx; | ||
| assert!(tx.input.len() >= 2); | ||
| assert!(!tx.output.is_empty()); | ||
| assert!( | ||
| tx.input.iter().any(|txin| txin.previous_output == outpoint), | ||
| "foreign_utxo should be in there" | ||
| ); | ||
| } | ||
|
|
||
| // Foreign value equals fee: no satoshis left for a drain output until a wallet UTXO is included. | ||
| #[test] | ||
| fn test_drain_to_pulls_local_utxo_when_foreign_value_equals_fee() { | ||
| let (mut wallet, _) = get_funded_wallet_wpkh(); | ||
| let drain_spk = wallet | ||
| .next_unused_address(KeychainKind::External) | ||
| .script_pubkey(); | ||
|
|
||
| let witness_utxo = TxOut { | ||
| value: Amount::from_sat(200), | ||
| script_pubkey: ScriptBuf::new_p2a(), | ||
| }; | ||
| let outpoint = OutPoint::new(Hash::hash(b"foreign-p2a-prev-200"), 1); | ||
| wallet.insert_txout(outpoint, witness_utxo.clone()); | ||
| let satisfaction_weight = Weight::from_wu(71); | ||
| let psbt_input = psbt::Input { | ||
| witness_utxo: Some(witness_utxo), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let mut tx_builder = wallet.build_tx(); | ||
| tx_builder | ||
| .add_foreign_utxo(outpoint, psbt_input, satisfaction_weight) | ||
| .unwrap() | ||
| .only_witness_utxo() | ||
| .fee_absolute(Amount::from_sat(200)) | ||
| .drain_to(drain_spk); | ||
|
|
||
| let psbt = tx_builder.finish().unwrap(); | ||
| let tx = psbt.unsigned_tx; | ||
| assert!(tx.input.len() >= 2); | ||
| assert!(!tx.output.is_empty()); | ||
| assert!(tx.input.iter().any(|txin| txin.previous_output == outpoint)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This introduces a
loopthat clonesrequired_for_attemptandoptional_remainingon everycreate_txcall, even whenshould_retry_for_dust_drainis false (i.e., the common case). That can be a noticeable performance/memory regression for wallets with many UTXOs. Consider keeping the original singlecoin_selectcall for the non-retry path (move the vectors instead of cloning), and only entering the retry loop (with cloning) when the guard is true and the first attempt returnsExcess::NoChange.