wallet: fix amount computed as boolean in coin selection
What the developer wrote
Authored by furszy
wallet: fix amount computed as boolean in coin selection
The comparison is evaluated before the assignment, so total_amount
ends up holding a boolean instead of the actual amount:
total_amount = (a - b < c)
which is not what we want here. This has been working by accident.
What changed, and why it matters
A single missing semicolon in Bitcoin Core's wallet coin-selection code caused a comparison to run before an assignment. As a result, the variable that was supposed to hold the total available amount instead held just true or false. The code still mostly worked by accident, but the wrong value could mislead the wallet's decision about whether it has enough funds, especially when unconfirmed transactions with long chains are involved. This could lead to confusing error messages or, in edge cases, incorrect transaction creation.
Apply the patch. After patching, review whether the buggy code path could have produced incorrect SelectionResult or fee/change calculations in edge cases involving long unconfirmed chains, and consider adding regression tests for this branch.
Security signals we found
Operator precedence bug causing unintended boolean assignment
Downstream arithmetic on a variable that held a boolean value
Coin-selection logic affecting fund availability checks
Potential incorrect handling of long unconfirmed transaction chains
Evidence from the diff
In src/wallet/spend.cpp, AutomaticCoinSelection contained the statement if (CAmount total_amount = available_coins.GetTotalAmount() - total_discarded < value_to_select). Because < binds more tightly than =, this is parsed as CAmount total_amount = (available_coins.GetTotalAmount() - total_discarded < value_to_select), assigning a bool to a CAmount. The fix splits the declaration and comparison with a comma/semicolon: CAmount total_amount = available_coins.GetTotalAmount() - total_discarded; total_amount < value_to_select. The downstream use total_amount + total_unconf_long_chain > value_to_select then operates on the real amount rather than 0/1. The commit message says this ‘has been working by accident.’
Changed components
src/wallet/spend.cppAutomaticCoinSelectionWallet coin selection / fund availability checkInspect captured patch +1 / −1
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 3db42d1b..3d150426 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -939,7 +939,7 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin
if (group.m_ancestors >= max_ancestors || group.m_max_cluster_count >= max_cluster_count) total_unconf_long_chain += group.GetSelectionAmount();
}
- if (CAmount total_amount = available_coins.GetTotalAmount() - total_discarded < value_to_select) {
+ if (CAmount total_amount = available_coins.GetTotalAmount() - total_discarded; total_amount < value_to_select) {
// Special case, too-long-mempool cluster.
if (total_amount + total_unconf_long_chain > value_to_select) {
return util::Error{_("Unconfirmed UTXOs are available, but spending them creates a chain of transactions that will be rejected by the mempool")};
Why this scored 46/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.