Merge bitcoin/bitcoin#36284: wallet: don't double discard output groups with avoidpartialspends
What changed, and why it matters
This is a wallet bug, not a theft or remote-code bug. When a Bitcoin Core user turns on the optional 'avoidpartialspends' or 'avoid_reuse' setting, an output group rejected during coin selection could be counted twice as 'discarded.' That double-counting could make the wallet wrongly believe there are not enough spendable coins and fail to create a transaction, even though the user actually has enough confirmed funds. The fix makes sure each rejected group is recorded only once, and adds tests to prove it.
No urgent security response required. Users relying on avoidpartialspends or avoid_reuse should upgrade to a release containing this fix to avoid spurious transaction-creation failures. Reviewers should verify the new tests reproduce the failure before the patch and pass after it.
Security signals we found
Logic error causing double-counting of discarded UTXO groups
Can trigger false 'insufficient funds' failure in coin selection
Affects avoidpartialspends / avoid_reuse wallets only
No memory corruption, remote execution, or key leakage
Fix includes both unit and functional regression tests
Evidence from the diff
In GroupOutputs(), with avoidpartialspends/avoid_reuse enabled, every positive-value output group is pushed into both the mixed and positive-only filtered maps. If a group fails all eligibility filters, it was previously appended to ret_discarded_groups in both passes, so AutomaticCoinSelection subtracted its value twice from the available total. The patch records discards only on the mixed pass, since the positive-only set is a subset of the mixed set. A unit test checks discarded_groups.size() == 2 instead of 4, and a functional test verifies that a wallet with avoid_reuse can still send 0.65 BTC using a confirmed 0.7 BTC coin even when a long unconfirmed chain makes another coin ineligible.
Changed components
src/wallet/spend.cppsrc/wallet/spend.hsrc/wallet/test/group_outputs_tests.cpptest/functional/wallet_create_tx.pyInspect captured patch +43 / −1
### src/wallet/spend.cpp
@@ -672,7 +672,8 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet,
filtered_groups[filter].Push(group, type, positive_only, /*insert_mixed=*/!positive_only);
accepted = true;
}
- if (!accepted) ret_discarded_groups.emplace_back(group);
+ // The positive-only groups are a subset of the mixed ones, don't record them twice
+ if (!accepted && !positive_only) ret_discarded_groups.emplace_back(group);
}
}
};
### src/wallet/spend.h
@@ -120,6 +120,15 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet,
const CoinSelectionParams& coin_sel_params,
const std::vector<SelectionFilter>& filters);
+/**
+ * Group coins by the provided filters, groups that pass no filter are appended to `ret_discarded_groups`.
+ */
+FilteredOutputGroups GroupOutputs(const CWallet& wallet,
+ const CoinsResult& coins,
+ const CoinSelectionParams& coin_sel_params,
+ const std::vector<SelectionFilter>& filters,
+ std::vector<OutputGroup>& ret_discarded_groups);
+
/**
* Attempt to find a valid input set that preserves privacy by not mixing OutputTypes.
* `ChooseSelectionResult()` will be called on each OutputType individually and the best
### src/wallet/test/group_outputs_tests.cpp
@@ -202,6 +202,11 @@ BOOST_AUTO_TEST_CASE(outputs_grouping_tests)
/*expected_without_partial_spends_size=*/ 3,
/*positive_only=*/ false);
+ // The two ineligible UTXOs must be discarded exactly once each
+ std::vector<OutputGroup> discarded_groups;
+ GroupOutputs(*wallet, group_verifier.coins_pool, makeSelectionParams(group_verifier.rand, /*avoid_partial_spends=*/true), {{BASIC_FILTER}}, discarded_groups);
+ BOOST_CHECK_EQUAL(discarded_groups.size(), 2U);
+
// ###########################################################################################
// 7) Surpass the OUTPUT_GROUP_MAX_ENTRIES and verify that a second partial group gets created
// ###########################################################################################
### test/functional/wallet_create_tx.py
@@ -35,6 +35,7 @@ def run_test(self):
self.test_anti_fee_sniping()
self.test_tx_size_too_large()
self.test_create_too_long_mempool_chain()
+ self.test_too_long_mempool_chain_avoid_partial_spends()
self.test_version3()
def test_anti_fee_sniping(self):
@@ -110,6 +111,32 @@ def test_create_too_long_mempool_chain(self):
test_wallet.unloadwallet()
+ def test_too_long_mempool_chain_avoid_partial_spends(self):
+ self.log.info('Check that a discarded too-long-chain coin is not counted twice with avoidpartialspends')
+ df_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
+
+ # avoid_reuse implies avoidpartialspends
+ self.nodes[0].createwallet(wallet_name="aps", avoid_reuse=True)
+ aps_wallet = self.nodes[0].get_wallet_rpc("aps")
+
+ df_wallet.sendtoaddress(aps_wallet.getnewaddress(), 0.3)
+ self.generate(self.nodes[0], 1)
+
+ # Spend the 0.3 coin to ourselves until it hits the ancestor limit. The chain is from us, so
+ # it is trusted and available for selection, but too long to be eligible, so it gets discarded.
+ for _ in range(25):
+ txid = aps_wallet.sendall([aps_wallet.getnewaddress()])['txid']
+ assert_equal(self.nodes[0].getmempoolentry(txid)['ancestorcount'], 25)
+
+ # Add a confirmed 0.7 coin at another address, leaving the chain in the mempool
+ txid = df_wallet.sendtoaddress(aps_wallet.getnewaddress(), 0.7)
+ self.generateblock(self.nodes[0], output=df_wallet.getnewaddress(), transactions=[txid])
+
+ # The confirmed 0.7 coin alone covers this payment
+ aps_wallet.sendtoaddress(df_wallet.getnewaddress(), 0.65)
+
+ aps_wallet.unloadwallet()
+
def test_version3(self):
self.log.info('Check wallet does not create transactions with version=3 yet')
wallet_rpc = self.nodes[0].get_wallet_rpc(self.default_wallet_name)Why this scored 44/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.