What the developer wrote
Authored by Hodlinator
wallet: Remove CoinsResult::Clear()
Instead we fully reset CoinsResult-instances without forgetting any fields.
Fixes: https://github.com/bitcoin/bitcoin/issues/35449
Variant resetting the other fields in Clear():
₿ hyperfine --warmup 2 "./build/bin/test_bitcoin -t coinselector_tests/knapsack_solver_test"
Benchmark 1: ./build/bin/test_bitcoin -t coinselector_tests/knapsack_solver_test
Time (mean ± σ): 4.289 s ± 0.008 s [User: 4.252 s, System: 0.027 s]
Range (min … max): 4.279 s … 4.301 s 10 runs
This variant:
₿ hyperfine --warmup 2 "./build/bin/test_bitcoin -t coinselector_tests/knapsack_solver_test"
Benchmark 1: ./build/bin/test_bitcoin -t coinselector_tests/knapsack_solver_test
Time (mean ± σ): 4.279 s ± 0.005 s [User: 4.244 s, System: 0.026 s]
Range (min … max): 4.271 s … 4.287 s 10 runs
What changed, and why it matters
This Bitcoin Core commit removes a custom `Clear()` method from a wallet coin-selection helper struct and replaces its uses with a full default reset (`available_coins = {}`). The stated reason is to avoid accidentally leaving fields partially reset in the future. The change is defensive code cleanup; there is no direct evidence in the commit or supplied references that it fixes an active security vulnerability or exploitable bug.
Treat as routine code-quality/maintenance cleanup. Review whether any live code paths (not just tests) relied on `CoinsResult::Clear()` semantics, and verify that default construction is behaviorally equivalent for all current fields. No urgent security action is indicated by the supplied materials.
Security signals we found
Defensive removal of partial-reset helper
No explicit security claim in commit message
No CVE or advisory referenced
Change limited to wallet coin-selection internals and unit tests
Evidence from the diff
The CoinsResult struct in src/wallet/spend.h previously exposed a Clear() member that only cleared the inner coins map. The patch removes that method and updates all call sites in src/wallet/test/coinselector_tests.cpp to assign a fresh default-constructed object instead. This ensures any future fields added to CoinsResult are also reset. The commit message frames this as fixing issue #35449 and includes a micro-benchmark showing no performance regression. No CVE, advisory, or vendor security disclosure is supplied or referenced.
Changed components
src/wallet/spend.hsrc/wallet/spend.cppsrc/wallet/test/coinselector_tests.cppInspect captured patch +11 / −16
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 9384299d..dba7b882 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -210,10 +210,6 @@ std::vector<COutput> CoinsResult::All() const
return all;
}
-void CoinsResult::Clear() {
- coins.clear();
-}
-
void CoinsResult::Erase(const std::unordered_set<COutPoint, SaltedOutpointHasher>& coins_to_remove)
{
for (auto& [type, vec] : coins) {
diff --git a/src/wallet/spend.h b/src/wallet/spend.h
index 925111dc..b013a3fa 100644
--- a/src/wallet/spend.h
+++ b/src/wallet/spend.h
@@ -40,7 +40,7 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* walle
* This struct is really just a wrapper around OutputType vectors with a convenient
* method for concatenating and returning all COutputs as one vector.
*
- * Size(), Clear(), Erase(), Shuffle(), and Add() methods are implemented to
+ * Size(), Erase(), Shuffle(), and Add() methods are implemented to
* allow easy interaction with the struct.
*/
struct CoinsResult {
@@ -54,7 +54,6 @@ struct CoinsResult {
size_t Size() const;
/** Return how many different output types this struct stores */
size_t TypesCount() const { return coins.size(); }
- void Clear();
void Erase(const std::unordered_set<COutPoint, SaltedOutpointHasher>& coins_to_remove);
void Shuffle(FastRandomContext& rng_fast);
void Add(OutputType type, const COutput& out);
diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
index ee6a639a..5713466e 100644
--- a/src/wallet/test/coinselector_tests.cpp
+++ b/src/wallet/test/coinselector_tests.cpp
@@ -202,7 +202,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
BOOST_CHECK(!SelectCoinsBnB(GroupCoins(available_coins.All()), 1 * CENT, coin_selection_params_bnb.m_cost_of_change));
// Test fees subtracted from output:
- available_coins.Clear();
+ available_coins = {};
add_coin(available_coins, *wallet, 1 * CENT, coin_selection_params_bnb.m_effective_feerate);
available_coins.All().at(0).input_bytes = 40;
const auto result9 = SelectCoinsBnB(GroupCoins(available_coins.All()), 1 * CENT, coin_selection_params_bnb.m_cost_of_change);
@@ -343,7 +343,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
// test multiple times to allow for differences in the shuffle order
for (int i = 0; i < RUN_TESTS; i++)
{
- available_coins.Clear();
+ available_coins = {};
// with an empty wallet we can't even pay one cent
BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 1 * CENT, CENT));
@@ -412,7 +412,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
BOOST_CHECK_EQUAL(result8->GetInputSet().size(), 1U);
// now clear out the wallet and start again to test choosing between subsets of smaller coins and the next biggest coin
- available_coins.Clear();
+ available_coins = {};
add_coin(available_coins, *wallet, 6*CENT);
add_coin(available_coins, *wallet, 7*CENT);
@@ -470,7 +470,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
// empty the wallet and start again, now with fractions of a cent, to test small change avoidance
- available_coins.Clear();
+ available_coins = {};
add_coin(available_coins, *wallet, CENT * 1 / 10);
add_coin(available_coins, *wallet, CENT * 2 / 10);
add_coin(available_coins, *wallet, CENT * 3 / 10);
@@ -502,7 +502,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
// run the 'mtgox' test (see https://blockexplorer.com/tx/29a3efd3ef04f9153d47a990bd7b048a4b2d213daaa5fb8ed670fb85f13bdbcf)
// they tried to consolidate 10 50k coins into one 500k coin, and ended up with 50k in change
- available_coins.Clear();
+ available_coins = {};
for (int j = 0; j < 20; j++)
add_coin(available_coins, *wallet, 50000 * COIN);
@@ -515,7 +515,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
// we need to try finding an exact subset anyway
// sometimes it will fail, and so we use the next biggest coin:
- available_coins.Clear();
+ available_coins = {};
add_coin(available_coins, *wallet, CENT * 5 / 10);
add_coin(available_coins, *wallet, CENT * 6 / 10);
add_coin(available_coins, *wallet, CENT * 7 / 10);
@@ -526,7 +526,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
BOOST_CHECK_EQUAL(result20->GetInputSet().size(), 1U);
// but sometimes it's possible, and we use an exact subset (0.4 + 0.6 = 1.0)
- available_coins.Clear();
+ available_coins = {};
add_coin(available_coins, *wallet, CENT * 4 / 10);
add_coin(available_coins, *wallet, CENT * 6 / 10);
add_coin(available_coins, *wallet, CENT * 8 / 10);
@@ -537,7 +537,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
BOOST_CHECK_EQUAL(result21->GetInputSet().size(), 2U); // in two coins 0.4+0.6
// test avoiding small change
- available_coins.Clear();
+ available_coins = {};
add_coin(available_coins, *wallet, CENT * 5 / 100);
add_coin(available_coins, *wallet, CENT * 1);
add_coin(available_coins, *wallet, CENT * 100);
@@ -557,7 +557,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
// test with many inputs
for (CAmount amt=1500; amt < COIN; amt*=10) {
- available_coins.Clear();
+ available_coins = {};
// Create 676 inputs (= (old MAX_STANDARD_TX_SIZE == 100000) / 148 bytes per input)
for (uint16_t j = 0; j < 676; j++)
add_coin(available_coins, *wallet, amt);
@@ -583,7 +583,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
// test randomness
{
- available_coins.Clear();
+ available_coins = {};
for (int i2 = 0; i2 < 100; i2++)
add_coin(available_coins, *wallet, COIN);
Why this scored 29/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.