From 2202ab597596c84fc49f8784e823372b7a9efcbe Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Tue, 27 Jan 2026 12:47:06 +0530 Subject: [PATCH] wallet: ensure COutput added in set are unique before #25806, set was used and would not contain same COutputs in the set. now we use set> and it might be possible for 2 distinct shared_ptr (different pointer address but same COutputs) to be added into the set. so preserve previous behaviour by making sure values in the set are also distinct --- src/util/insert.h | 5 +++++ src/wallet/coinselection.cpp | 4 ++-- src/wallet/coinselection.h | 13 ++++++++++--- src/wallet/spend.cpp | 2 +- src/wallet/spend.h | 2 +- src/wallet/test/coinselector_tests.cpp | 6 ++---- src/wallet/test/fuzz/coinselection.cpp | 6 +++--- 7 files changed, 24 insertions(+), 14 deletions(-) diff --git a/src/util/insert.h b/src/util/insert.h index 995b382a033..96dc51b5006 100644 --- a/src/util/insert.h +++ b/src/util/insert.h @@ -19,6 +19,11 @@ inline void insert(std::set& dst, const Tsrc& src) { dst.insert(src.begin(), src.end()); } +template +inline void insert(std::set& 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 192e49361a1..58cb427432f 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>& inputs, bool subtract_fee_outputs) +void SelectionResult::AddInputs(const COutputSet& 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>& SelectionResult::GetInputSet() const +const COutputSet& SelectionResult::GetInputSet() const { return m_selected_inputs; } diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 79fee40dc1d..4e21c8e3104 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 COutputPtrComparator { + bool operator()(const std::shared_ptr& a, const std::shared_ptr& b) const { + return *a < *b; + } +}; +using COutputSet = std::set, COutputPtrComparator>; + struct SelectionResult { private: /** Set of inputs selected by the algorithm to use in the transaction */ - std::set> m_selected_inputs; + COutputSet 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>& inputs, bool subtract_fee_outputs); + void AddInputs(const COutputSet& 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>& GetInputSet() const; + const COutputSet& GetInputSet() const; /** Get the vector of COutputs that will be used to fill in a CTransaction's vin */ std::vector> GetShuffledInputVector() const; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 1e0ac8f0428..440356834da 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -789,7 +789,7 @@ util::Result 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 outpoints; - std::set> coins = result.GetInputSet(); + COutputSet 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 5a1a879a281..4cf4ef78eba 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -155,7 +155,7 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co // User manually selected inputs that must be part of the transaction struct PreSelectedInputs { - std::set> coins; + COutputSet 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 5c047bad02a..df07245b1b7 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> 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 ret = std::mismatch(a.GetInputSet().begin(), a.GetInputSet().end(), b.GetInputSet().begin(), + std::pair ret = std::mismatch(a.GetInputSet().begin(), a.GetInputSet().end(), b.GetInputSet().begin(), [](const std::shared_ptr& a, const std::shared_ptr& b) { return a->outpoint == b->outpoint; }); @@ -1257,7 +1255,7 @@ static util::Result select_coins(const CAmount& target, const C return result; } -static bool has_coin(const CoinSet& set, CAmount amount) +static bool has_coin(const COutputSet& 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 e85a49f5898..c249fe61c10 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& utxos, const CAmount& total_amount, const bool& subtract_fee_outputs) { SelectionResult result(total_amount, SelectionAlgorithm::MANUAL); - std::set> utxo_pool; + COutputSet utxo_pool; for (const auto& utxo : utxos) { utxo_pool.insert(std::make_shared(utxo)); } @@ -319,7 +319,7 @@ void FuzzCoinSelectionAlgorithm(std::span buffer) { std::vector utxos; CAmount new_total_balance{CreateCoins(fuzzed_data_provider, utxos, coin_params, next_locktime)}; if (new_total_balance > 0) { - std::set> new_utxo_pool; + COutputSet new_utxo_pool; for (const auto& utxo : utxos) { new_utxo_pool.insert(std::make_shared(utxo)); } @@ -336,7 +336,7 @@ void FuzzCoinSelectionAlgorithm(std::span 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> input_set{result->GetInputSet()}; + const COutputSet input_set{result->GetInputSet()}; const int old_weight{result->GetWeight()}; result->Merge(manual_selection); assert(result->GetInputSet().size() == input_set.size() + manual_inputs.size());