-
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 2 commits
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,19 +1467,53 @@ 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 selection_result = if should_retry_for_dust_drain { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut required_for_attempt = required_utxos; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut optional_remaining = optional_utxos; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 !matches!(&result.excess, Excess::NoChange { .. }) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| break result; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let Some(w) = optional_remaining.pop() else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| break result; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| required_for_attempt.push(w); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 !matches!(&result.excess, Excess::NoChange { .. }) { | |
| break result; | |
| } | |
| let Some(w) = optional_remaining.pop() else { | |
| break result; | |
| }; | |
| required_for_attempt.push(w); | |
| let mut last_successful_result = None; | |
| loop { | |
| match coin_selection.coin_select( | |
| required_for_attempt.clone(), | |
| optional_remaining.clone(), | |
| fee_rate, | |
| outgoing + fee_amount, | |
| &drain_script, | |
| rng, | |
| ) { | |
| Ok(result) => { | |
| if !matches!(&result.excess, Excess::NoChange { .. }) { | |
| break result; | |
| } | |
| let Some(w) = optional_remaining.pop() else { | |
| break result; | |
| }; | |
| last_successful_result = Some(result); | |
| required_for_attempt.push(w); | |
| } | |
| Err(err) => { | |
| if let Some(result) = last_successful_result { | |
| break result; | |
| } | |
| return Err(CreateTxError::CoinSelection(err)); | |
| } | |
| } |
| 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.
Potential performance issue: the retry loop clones
required_for_attemptandoptional_remainingon each iteration, and may rerun coin selection many times when the wallet has a large UTXO set. Consider adding a cheap guard like!optional_utxos.is_empty()(or an upper bound on retries) to avoid extra allocations/work in cases where no retry can succeed (e.g., no optional UTXOs).