From 461f0821a2dff0a27f755464b0eb92314fba1acd Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 1 Aug 2022 16:15:36 -0300 Subject: [PATCH] refactor: make OutputGroup::m_outputs field a vector of shared_ptr Initial steps towards sharing COutput instances across all possible OutputGroups (instead of copying them time after time). --- src/bench/coin_selection.cpp | 2 +- src/wallet/coinselection.cpp | 27 +++++++++++++------------- src/wallet/coinselection.h | 14 ++++++------- src/wallet/spend.cpp | 8 ++++---- src/wallet/spend.h | 4 ++-- src/wallet/test/coinselector_tests.cpp | 18 ++++++++--------- src/wallet/test/fuzz/coinselection.cpp | 2 +- 7 files changed, 38 insertions(+), 37 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index f77ac24df2..8a78783517 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -91,7 +91,7 @@ static void add_coin(const CAmount& nValue, int nInput, std::vector tx.vout[nInput].nValue = nValue; COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 0, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ true, /*fees=*/ 0); set.emplace_back(); - set.back().Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0); + set.back().Insert(std::make_shared(output), /*ancestors=*/ 0, /*descendants=*/ 0); } // Copied from src/wallet/test/coinselector_tests.cpp static CAmount make_hard_case(int utxos, std::vector& utxo_pool) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index cebe2cd7ed..f99beabc70 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -333,9 +333,9 @@ std::optional KnapsackSolver(std::vector& groups, ******************************************************************************/ -void OutputGroup::Insert(const COutput& output, size_t ancestors, size_t descendants) { +void OutputGroup::Insert(const std::shared_ptr& output, size_t ancestors, size_t descendants) { m_outputs.push_back(output); - COutput& coin = m_outputs.back(); + auto& coin = *m_outputs.back(); fee += coin.GetFee(); @@ -355,8 +355,8 @@ void OutputGroup::Insert(const COutput& output, size_t ancestors, size_t descend // coin itself; thus, this value is counted as the max, not the sum m_descendants = std::max(m_descendants, descendants); - if (output.input_bytes > 0) { - m_weight += output.input_bytes * WITNESS_SCALE_FACTOR; + if (output->input_bytes > 0) { + m_weight += output->input_bytes * WITNESS_SCALE_FACTOR; } } @@ -372,7 +372,7 @@ CAmount OutputGroup::GetSelectionAmount() const return m_subtract_fee_outputs ? m_value : effective_value; } -CAmount GetSelectionWaste(const std::set& inputs, CAmount change_cost, CAmount target, bool use_effective_value) +CAmount GetSelectionWaste(const std::set>& inputs, CAmount change_cost, CAmount target, bool use_effective_value) { // This function should not be called with empty inputs as that would mean the selection failed assert(!inputs.empty()); @@ -380,7 +380,8 @@ CAmount GetSelectionWaste(const std::set& inputs, CAmount change_cost, // Always consider the cost of spending an input now vs in the future. CAmount waste = 0; CAmount selected_effective_value = 0; - for (const COutput& coin : inputs) { + for (const auto& coin_ptr : inputs) { + const COutput& coin = *coin_ptr; waste += coin.GetFee() - coin.long_term_fee; selected_effective_value += use_effective_value ? coin.GetEffectiveValue() : coin.txout.nValue; } @@ -428,12 +429,12 @@ CAmount SelectionResult::GetWaste() const CAmount SelectionResult::GetSelectedValue() const { - return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin.txout.nValue; }); + return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin->txout.nValue; }); } CAmount SelectionResult::GetSelectedEffectiveValue() const { - return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin.GetEffectiveValue(); }); + return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin->GetEffectiveValue(); }); } void SelectionResult::Clear() @@ -452,14 +453,14 @@ 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 std::set>& inputs, bool subtract_fee_outputs) { // As it can fail, combine inputs first InsertInputs(inputs); m_use_effective = !subtract_fee_outputs; m_weight += std::accumulate(inputs.cbegin(), inputs.cend(), 0, [](int sum, const auto& coin) { - return sum + std::max(coin.input_bytes, 0) * WITNESS_SCALE_FACTOR; + return sum + std::max(coin->input_bytes, 0) * WITNESS_SCALE_FACTOR; }); } @@ -477,14 +478,14 @@ void SelectionResult::Merge(const SelectionResult& other) m_weight += other.m_weight; } -const std::set& SelectionResult::GetInputSet() const +const std::set>& SelectionResult::GetInputSet() const { return m_selected_inputs; } -std::vector SelectionResult::GetShuffledInputVector() const +std::vector> SelectionResult::GetShuffledInputVector() const { - std::vector coins(m_selected_inputs.begin(), m_selected_inputs.end()); + std::vector> coins(m_selected_inputs.begin(), m_selected_inputs.end()); Shuffle(coins.begin(), coins.end(), FastRandomContext()); return coins; } diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 0e5e86f652..3ca6520242 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -198,7 +198,7 @@ struct CoinEligibilityFilter struct OutputGroup { /** The list of UTXOs contained in this output group. */ - std::vector m_outputs; + std::vector> m_outputs; /** Whether the UTXOs were sent by the wallet to itself. This is relevant because we may want at * least a certain number of confirmations on UTXOs received from outside wallets while trusting * our own UTXOs more. */ @@ -237,7 +237,7 @@ struct OutputGroup m_subtract_fee_outputs(params.m_subtract_fee_outputs) {} - void Insert(const COutput& output, size_t ancestors, size_t descendants); + void Insert(const std::shared_ptr& output, size_t ancestors, size_t descendants); bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const; CAmount GetSelectionAmount() const; }; @@ -259,7 +259,7 @@ struct OutputGroup * @param[in] use_effective_value Whether to use the input's effective value (when true) or the real value (when false). * @return The waste */ -[[nodiscard]] CAmount GetSelectionWaste(const std::set& inputs, CAmount change_cost, CAmount target, bool use_effective_value = true); +[[nodiscard]] CAmount GetSelectionWaste(const std::set>& inputs, CAmount change_cost, CAmount target, bool use_effective_value = true); /** Choose a random change target for each transaction to make it harder to fingerprint the Core @@ -292,7 +292,7 @@ struct SelectionResult { private: /** Set of inputs selected by the algorithm to use in the transaction */ - std::set m_selected_inputs; + std::set> 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 */ @@ -329,7 +329,7 @@ public: void Clear(); void AddInput(const OutputGroup& group); - void AddInputs(const std::set& inputs, bool subtract_fee_outputs); + void AddInputs(const std::set>& inputs, bool subtract_fee_outputs); /** Calculates and stores the waste for this selection via GetSelectionWaste */ void ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee); @@ -344,9 +344,9 @@ public: void Merge(const SelectionResult& other); /** Get m_selected_inputs */ - const std::set& GetInputSet() const; + const std::set>& GetInputSet() const; /** Get the vector of COutputs that will be used to fill in a CTransaction's vin */ - std::vector GetShuffledInputVector() const; + std::vector> GetShuffledInputVector() const; bool operator<(SelectionResult other) const; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 25af6f49bd..a9c777dd70 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -421,7 +421,7 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vector 0) { // Make an OutputGroup containing just this output OutputGroup group{coin_sel_params}; - group.Insert(output, ancestors, descendants); + group.Insert(std::make_shared(output), ancestors, descendants); if (group.EligibleForSpending(filter)) groups_out.push_back(group); } @@ -465,7 +465,7 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vector 0) { - group->Insert(output, ancestors, descendants); + group->Insert(std::make_shared(output), ancestors, descendants); } } @@ -936,7 +936,7 @@ static util::Result CreateTransactionInternal( } // Shuffle selected coins and fill in final vin - std::vector selected_coins = result.GetShuffledInputVector(); + std::vector> selected_coins = result.GetShuffledInputVector(); // The sequence number is set to non-maxint so that DiscourageFeeSniping // works. @@ -948,7 +948,7 @@ static util::Result CreateTransactionInternal( // behavior." const uint32_t nSequence{coin_control.m_signal_bip125_rbf.value_or(wallet.m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : CTxIn::MAX_SEQUENCE_NONFINAL}; for (const auto& coin : selected_coins) { - txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence)); + txNew.vin.push_back(CTxIn(coin->outpoint, CScript(), nSequence)); } DiscourageFeeSniping(txNew, rng_fast, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight()); diff --git a/src/wallet/spend.h b/src/wallet/spend.h index ad122b7ef3..105a1a2549 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -148,7 +148,7 @@ util::Result ChooseSelectionResult(const CWallet& wallet, const // User manually selected inputs that must be part of the transaction struct PreSelectedInputs { - std::set coins; + std::set> 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 @@ -161,7 +161,7 @@ struct PreSelectedInputs } else { total_amount += output.GetEffectiveValue(); } - coins.insert(output); + coins.insert(std::make_shared(output)); } }; diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 1c2a561bef..1bb2e965d8 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -29,7 +29,7 @@ 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; +typedef std::set> CoinSet; static const CoinEligibilityFilter filter_standard(1, 6, 0); static const CoinEligibilityFilter filter_confirmed(1, 1, 0); @@ -53,7 +53,7 @@ static void add_coin(const CAmount& nValue, int nInput, SelectionResult& result) tx.nLockTime = nextLockTime++; // so all transactions get different hashes COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, /*fees=*/ 0); OutputGroup group; - group.Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0); + group.Insert(std::make_shared(output), /*ancestors=*/ 0, /*descendants=*/ 0); result.AddInput(group); } @@ -65,7 +65,7 @@ static void add_coin(const CAmount& nValue, int nInput, CoinSet& set, CAmount fe tx.nLockTime = nextLockTime++; // so all transactions get different hashes COutput coin(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ 148, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, fee); coin.long_term_fee = long_term_fee; - set.insert(coin); + set.insert(std::make_shared(coin)); } static void add_coin(CoinsResult& available_coins, CWallet& wallet, const CAmount& nValue, CFeeRate feerate = CFeeRate(0), int nAge = 6*24, bool fIsFromMe = false, int nInput =0, bool spendable = false) @@ -94,10 +94,10 @@ static bool EquivalentResult(const SelectionResult& a, const SelectionResult& b) std::vector a_amts; std::vector b_amts; for (const auto& coin : a.GetInputSet()) { - a_amts.push_back(coin.txout.nValue); + a_amts.push_back(coin->txout.nValue); } for (const auto& coin : b.GetInputSet()) { - b_amts.push_back(coin.txout.nValue); + b_amts.push_back(coin->txout.nValue); } std::sort(a_amts.begin(), a_amts.end()); std::sort(b_amts.begin(), b_amts.end()); @@ -110,8 +110,8 @@ static bool EquivalentResult(const SelectionResult& a, const SelectionResult& b) static bool EqualResult(const SelectionResult& a, const SelectionResult& b) { std::pair ret = std::mismatch(a.GetInputSet().begin(), a.GetInputSet().end(), b.GetInputSet().begin(), - [](const COutput& a, const COutput& b) { - return a.outpoint == b.outpoint; + [](const std::shared_ptr& a, const std::shared_ptr& b) { + return a->outpoint == b->outpoint; }); return ret.first == a.GetInputSet().end() && ret.second == b.GetInputSet().end(); } @@ -134,7 +134,7 @@ inline std::vector& GroupCoins(const std::vector& availabl static_groups.clear(); for (auto& coin : available_coins) { static_groups.emplace_back(); - static_groups.back().Insert(coin, /*ancestors=*/ 0, /*descendants=*/ 0); + static_groups.back().Insert(std::make_shared(coin), /*ancestors=*/ 0, /*descendants=*/ 0); } return static_groups; } @@ -943,7 +943,7 @@ static util::Result select_coins(const CAmount& target, const C static bool has_coin(const CoinSet& set, CAmount amount) { - return std::any_of(set.begin(), set.end(), [&](const auto& coin) { return coin.GetEffectiveValue() == amount; }); + return std::any_of(set.begin(), set.end(), [&](const auto& coin) { return coin->GetEffectiveValue() == amount; }); } BOOST_AUTO_TEST_CASE(check_max_weight) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 95060c748f..304190eec1 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -30,7 +30,7 @@ static void GroupCoins(FuzzedDataProvider& fuzzed_data_provider, const std::vect bool valid_outputgroup{false}; for (auto& coin : coins) { if (!positive_only || (positive_only && coin.GetEffectiveValue() > 0)) { - output_group.Insert(coin, /*ancestors=*/0, /*descendants=*/0); + output_group.Insert(std::make_shared(coin), /*ancestors=*/0, /*descendants=*/0); } // If positive_only was specified, nothing was inserted, leading to an empty output group // that would be invalid for the BnB algorithm