wallet: ensure COutput added in set are unique
What changed, and why it matters
This Bitcoin Core change fixes a subtle bug in how the wallet keeps track of selected coins. Previously, the wallet used a set of coin objects directly, which automatically prevented duplicates. After a recent refactor, it started using a set of pointers to coin objects, where the default behavior only prevents duplicate pointer addresses—not duplicate coins. This meant two different pointers representing the same coin could both end up in the selection, potentially causing the wallet to try to spend the same coin twice or miscalculate fees and change. The fix makes the set compare the actual coin data, not just pointer addresses, restoring the old duplicate-prevention behavior.
Treat as a wallet correctness fix worth backporting to affected releases. Review whether any released code path could actually produce duplicate shared_ptr<COutput> values in practice, and add regression tests that explicitly insert duplicate-value pointers into OutputSet to verify deduplication.
Security signals we found
Duplicate coin selection could lead to double-spend attempts or invalid transactions
Set uniqueness semantics changed by pointer indirection refactor
Custom comparator restores value-based deduplication
Wallet coin-selection correctness bug, not a remote code execution vulnerability
Evidence from the diff
The commit introduces OutputPtrComparator and an OutputSet alias so that std::set
Changed components
src/wallet/coinselection.cppsrc/wallet/coinselection.hsrc/wallet/spend.cppsrc/wallet/spend.hsrc/util/insert.hsrc/wallet/test/coinselector_tests.cppsrc/wallet/test/fuzz/coinselection.cppInspect captured patch +24 / −14
diff --git a/src/util/insert.h b/src/util/insert.h
index 995b382a..96dc51b5 100644
--- a/src/util/insert.h
+++ b/src/util/insert.h
@@ -19,6 +19,11 @@ inline void insert(std::set<TsetT>& dst, const Tsrc& src) {
dst.insert(src.begin(), src.end());
}
+template <typename TsetT, typename Compare, typename Tsrc>
+inline void insert(std::set<TsetT, Compare>& dst, const Tsrc& src) {
+ dst.insert(src.begin(), src.end());
+}
+
} // namespace util
#endif // BITCOIN_UTIL_INSERT_H
diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp
index 192e4936..8977d999 100644
--- a/src/wallet/coinselection.cpp
+++ b/src/wallet/coinselection.cpp
@@ -908,7 +908,7 @@ void SelectionResult::AddInput(const OutputGroup& group)
m_weight += group.m_weight;
}
-void SelectionResult::AddInputs(const std::set<std::shared_ptr<COutput>>& inputs, bool subtract_fee_outputs)
+void SelectionResult::AddInputs(const OutputSet& inputs, bool subtract_fee_outputs)
{
// As it can fail, combine inputs first
InsertInputs(inputs);
@@ -933,7 +933,7 @@ void SelectionResult::Merge(const SelectionResult& other)
m_weight += other.m_weight;
}
-const std::set<std::shared_ptr<COutput>>& SelectionResult::GetInputSet() const
+const OutputSet& SelectionResult::GetInputSet() const
{
return m_selected_inputs;
}
diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h
index 79fee40d..72e398a3 100644
--- a/src/wallet/coinselection.h
+++ b/src/wallet/coinselection.h
@@ -319,11 +319,18 @@ enum class SelectionAlgorithm : uint8_t
std::string GetAlgorithmName(const SelectionAlgorithm algo);
+struct OutputPtrComparator {
+ bool operator()(const std::shared_ptr<COutput>& a, const std::shared_ptr<COutput>& b) const {
+ return *a < *b;
+ }
+};
+using OutputSet = std::set<std::shared_ptr<COutput>, OutputPtrComparator>;
+
struct SelectionResult
{
private:
/** Set of inputs selected by the algorithm to use in the transaction */
- std::set<std::shared_ptr<COutput>> m_selected_inputs;
+ OutputSet m_selected_inputs;
/** The target the algorithm selected for. Equal to the recipient amount plus non-input fees */
CAmount m_target;
/** The algorithm used to produce this result */
@@ -368,7 +375,7 @@ public:
void Clear();
void AddInput(const OutputGroup& group);
- void AddInputs(const std::set<std::shared_ptr<COutput>>& inputs, bool subtract_fee_outputs);
+ void AddInputs(const OutputSet& inputs, bool subtract_fee_outputs);
/** How much individual inputs overestimated the bump fees for shared ancestries */
void SetBumpFeeDiscount(const CAmount discount);
@@ -409,7 +416,7 @@ public:
void Merge(const SelectionResult& other);
/** Get m_selected_inputs */
- const std::set<std::shared_ptr<COutput>>& GetInputSet() const;
+ const OutputSet& GetInputSet() const;
/** Get the vector of COutputs that will be used to fill in a CTransaction's vin */
std::vector<std::shared_ptr<COutput>> GetShuffledInputVector() const;
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 1e0ac8f0..a2ac727d 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -789,7 +789,7 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co
// If the chosen input set has unconfirmed inputs, check for synergies from overlapping ancestry
for (auto& result : results) {
std::vector<COutPoint> outpoints;
- std::set<std::shared_ptr<COutput>> coins = result.GetInputSet();
+ OutputSet coins = result.GetInputSet();
CAmount summed_bump_fees = 0;
for (auto& coin : coins) {
if (coin->depth > 0) continue; // Bump fees only exist for unconfirmed inputs
diff --git a/src/wallet/spend.h b/src/wallet/spend.h
index 5a1a879a..debe7d29 100644
--- a/src/wallet/spend.h
+++ b/src/wallet/spend.h
@@ -155,7 +155,7 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co
// User manually selected inputs that must be part of the transaction
struct PreSelectedInputs
{
- std::set<std::shared_ptr<COutput>> coins;
+ OutputSet coins;
// If subtract fee from outputs is disabled, the 'total_amount'
// will be the sum of each output effective value
// instead of the sum of the outputs amount
diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
index 5c047bad..b609ce82 100644
--- a/src/wallet/test/coinselector_tests.cpp
+++ b/src/wallet/test/coinselector_tests.cpp
@@ -30,8 +30,6 @@ BOOST_FIXTURE_TEST_SUITE(coinselector_tests, WalletTestingSetup)
// we repeat those tests this many times and only complain if all iterations of the test fail
#define RANDOM_REPEATS 5
-typedef std::set<std::shared_ptr<COutput>> CoinSet;
-
static const CoinEligibilityFilter filter_standard(1, 6, 0);
static const CoinEligibilityFilter filter_confirmed(1, 1, 0);
static const CoinEligibilityFilter filter_standard_extra(6, 6, 0);
@@ -117,7 +115,7 @@ static bool EquivalentResult(const SelectionResult& a, const SelectionResult& b)
/** Check if this selection is equal to another one. Equal means same inputs (i.e same value and prevout) */
static bool EqualResult(const SelectionResult& a, const SelectionResult& b)
{
- std::pair<CoinSet::iterator, CoinSet::iterator> ret = std::mismatch(a.GetInputSet().begin(), a.GetInputSet().end(), b.GetInputSet().begin(),
+ std::pair<OutputSet::iterator, OutputSet::iterator> ret = std::mismatch(a.GetInputSet().begin(), a.GetInputSet().end(), b.GetInputSet().begin(),
[](const std::shared_ptr<COutput>& a, const std::shared_ptr<COutput>& b) {
return a->outpoint == b->outpoint;
});
@@ -1257,7 +1255,7 @@ static util::Result<SelectionResult> select_coins(const CAmount& target, const C
return result;
}
-static bool has_coin(const CoinSet& set, CAmount amount)
+static bool has_coin(const OutputSet& set, CAmount amount)
{
return std::any_of(set.begin(), set.end(), [&](const auto& coin) { return coin->GetEffectiveValue() == amount; });
}
diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp
index e85a49f5..2f13d96d 100644
--- a/src/wallet/test/fuzz/coinselection.cpp
+++ b/src/wallet/test/fuzz/coinselection.cpp
@@ -68,7 +68,7 @@ static CAmount CreateCoins(FuzzedDataProvider& fuzzed_data_provider, std::vector
static SelectionResult ManualSelection(std::vector<COutput>& utxos, const CAmount& total_amount, const bool& subtract_fee_outputs)
{
SelectionResult result(total_amount, SelectionAlgorithm::MANUAL);
- std::set<std::shared_ptr<COutput>> utxo_pool;
+ OutputSet utxo_pool;
for (const auto& utxo : utxos) {
utxo_pool.insert(std::make_shared<COutput>(utxo));
}
@@ -319,7 +319,7 @@ void FuzzCoinSelectionAlgorithm(std::span<const uint8_t> buffer) {
std::vector<COutput> utxos;
CAmount new_total_balance{CreateCoins(fuzzed_data_provider, utxos, coin_params, next_locktime)};
if (new_total_balance > 0) {
- std::set<std::shared_ptr<COutput>> new_utxo_pool;
+ OutputSet new_utxo_pool;
for (const auto& utxo : utxos) {
new_utxo_pool.insert(std::make_shared<COutput>(utxo));
}
@@ -336,7 +336,7 @@ void FuzzCoinSelectionAlgorithm(std::span<const uint8_t> buffer) {
auto manual_selection{ManualSelection(manual_inputs, manual_balance, coin_params.m_subtract_fee_outputs)};
if (result) {
const CAmount old_target{result->GetTarget()};
- const std::set<std::shared_ptr<COutput>> input_set{result->GetInputSet()};
+ const OutputSet input_set{result->GetInputSet()};
const int old_weight{result->GetWeight()};
result->Merge(manual_selection);
assert(result->GetInputSet().size() == input_set.size() + manual_inputs.size());
Why this scored 32/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.