fix(wallet): retry coin selection on NoChange to avoid dust/zero drai…

[alienx5499]

Description

Fixes #376.

Wallet::create_tx could return CreateTxError::CoinSelection for drain-only transactions (drain_to, no explicit recipients) when spending a foreign input (e.g P2A) with fee_absolute.

In that situation, the initial coin_select pass may select only required inputs and yield Excess::NoChange (i.e. zero or dust remaining for the drain output). This results in a coin selection failure despite sufficient total funds, because the drain-only path cannot produce a non-dust output under Excess::NoChange.

This change adds a retry for that scenario : optional wallet UTXOs are promoted to required inputs one at a time and coin_select is run again until Excess is no longer NoChange or there are no optional UTXOs left. That yields a valid (non-dust) drain output when possible.

Scope: General coin selection behavior is unchanged. This only adds a retry for the NoChange + drain-only case. Optional UTXOs are taken in existing order (pop()), so no new selection policy is introduced.

Notes to the reviewers

• Guards: Retry is gated on:
  • no explicit recipients (params.recipients.is_empty())
  • drain_to is set
  • (params.drain_wallet || !params.utxos.is_empty()) (preserves NoRecipients behavior vs the empty-output path in mod.rs)
  • not manually_selected_only
• Tests: tests/drain_to_dust_pull_utxo.rs
  • Foreign P2A-style UTXOs via insert_txout, fee_absolute, drain_to
  • Covers dust remainder (500 sat value, 400 sat fee) and zero remainder (200 sat value, 200 sat fee)
  • Asserts at least 2 inputs (foreign + local) non-empty outputs, foreign outpoint present in inputs

Changelog notice

fix(wallet): retry coin selection on NoChange to avoid dust/zero drain outputs (#376)

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran just p before pushing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[codecov]

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.23%. Comparing base (fb7681a) to head (f083d6f).
⚠️ Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
src/wallet/mod.rs	91.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #448      +/-   ##
==========================================
+ Coverage   80.04%   80.23%   +0.19%     
==========================================
  Files          24       24              
  Lines        5336     5362      +26     
  Branches      242      249       +7     
==========================================
+ Hits         4271     4302      +31     
+ Misses        987      981       -6     
- Partials       78       79       +1     

Flag	Coverage Δ
rust	80.23% <91.66%> (+0.19%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[nikunjkumar05]

The loop has both an outer || optional_remaining.is_empty() break condition and an inner let Some(w) = optional_remaining.pop() else { break result } guard. Since the outer check already breaks when the Vec is empty, the else { break result } branch of pop() is dead code. This is confusing — pick one:

if !should_retry_for_dust_drain || !matches!(&result.excess, Excess::NoChange { .. }) { break result; }

[alienx5499]

Good catch!
Thanks!

[Copilot]

Pull request overview

This PR fixes a Wallet::create_tx failure mode for drain-only transactions that include a foreign UTXO and use fee_absolute, where initial coin selection can yield Excess::NoChange (dust/zero remainder) and ultimately surface as CreateTxError::CoinSelection despite sufficient total funds.

Changes:

• Add a retry loop in Wallet::create_tx that promotes optional wallet UTXOs to required inputs when coin selection returns Excess::NoChange for the drain-only + drain_to case.
• Add integration tests covering both dust remainder and zero remainder scenarios when spending a foreign P2A-style input with fee_absolute and drain_to.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/wallet/mod.rs	Adds a guarded coin-selection retry loop for the drain-only + Excess::NoChange path to avoid dust/zero drain outputs.
tests/drain_to_dust_pull_utxo.rs	Adds regression tests that reproduce the error and assert the builder pulls in an additional local UTXO to produce a valid drain output.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This introduces a loop that clones required_for_attempt and optional_remaining on every create_tx call, even when should_retry_for_dust_drain is false (i.e., the common case). That can be a noticeable performance/memory regression for wallets with many UTXOs. Consider keeping the original single coin_select call 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 returns Excess::NoChange.

Suggested change

	// 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);
	// initial selection yields NoChange, promote an optional UTXO to required and retry;
	// 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 first_coin_selection = coin_selection
	.coin_select(
	required_utxos,
	optional_utxos,
	fee_rate,
	outgoing + fee_amount,
	&drain_script,
	rng,
	)
	.map_err(CreateTxError::CoinSelection)?;
	let coin_selection = if should_retry_for_dust_drain
	&& matches!(&first_coin_selection.excess, Excess::NoChange { .. })
	{
	let mut required_for_attempt = first_coin_selection.selected.clone();
	let mut optional_remaining = first_coin_selection.local_selected.clone();
	loop {
	let Some(w) = optional_remaining.pop() else {
	break first_coin_selection;
	};
	required_for_attempt.push(w);
	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;
	}
	}
	} else {
	first_coin_selection

[Copilot]

let coin_selection = loop { ... } shadows the coin_selection: Cs algorithm parameter, so the same identifier refers to two different concepts (algorithm vs selection result) within a small scope. This is easy to misread and makes future edits riskier; consider renaming one of them (e.g., keep the algorithm as coin_selector and the loop result as selection_result).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In this retry loop, each coin_select attempt is fallible; returning immediately on Err(InsufficientFunds) (via map_err later in this chain) can change behavior vs the previous successful Ok(… Excess::NoChange …) attempt and can also prevent trying other optionals. Consider keeping the last successful result and breaking/falling back to it if a later retry errors, instead of propagating the error from the retry attempt.

Suggested change

	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));
	}
	}

[Copilot]

Potential performance issue: the retry loop clones required_for_attempt and optional_remaining on 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).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In the retry loop, if coin_select errors after promoting an optional UTXO, the code breaks out by returning last_successful_result (which is necessarily an Excess::NoChange result) rather than continuing to try other optional UTXOs. This can cause false failures when the last optional popped has negative effective value (BnB explicitly filters these from optional_utxos), since promoting it to required can trigger InsufficientFunds even though another optional UTXO could have resolved the NoChange dust case. Consider treating a failed promoted UTXO as “skipped” (remove it from required_for_attempt and continue with remaining optionals), and only return an error once all candidates are exhausted.

Suggested change

	if let Some(result) = last_successful_result {
	break result;
	if let Some(result) = last_successful_result.take() {
	required_for_attempt.pop();
	if optional_remaining.is_empty() {
	break result;
	}
	last_successful_result = Some(result);
	continue;