From 06ec8f992890cac69cd0fd20224aa51fa311a181 Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 31 Jul 2022 17:22:04 -0300 Subject: [PATCH 1/7] wallet: make OutputGroup "positive_only" filter explicit And not hide it inside the `OutputGroup::Insert` method. This method does not return anything if insertion fails. We can know before calling `Insert` whether the coin will be accepted or not. --- src/bench/coin_selection.cpp | 2 +- src/wallet/coinselection.cpp | 5 +---- src/wallet/coinselection.h | 2 +- src/wallet/spend.cpp | 19 +++++++++++-------- src/wallet/test/coinselector_tests.cpp | 4 ++-- src/wallet/test/fuzz/coinselection.cpp | 6 ++++-- 6 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 11b0e0dee2..f77ac24df2 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, /*positive_only=*/ false); + set.back().Insert(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 e6ba89627c..cebe2cd7ed 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -333,10 +333,7 @@ std::optional KnapsackSolver(std::vector& groups, ******************************************************************************/ -void OutputGroup::Insert(const COutput& output, size_t ancestors, size_t descendants, bool positive_only) { - // Filter for positive only here before adding the coin - if (positive_only && output.GetEffectiveValue() <= 0) return; - +void OutputGroup::Insert(const COutput& output, size_t ancestors, size_t descendants) { m_outputs.push_back(output); COutput& coin = m_outputs.back(); diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 9ff2011ce3..0e5e86f652 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -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, bool positive_only); + void Insert(const COutput& output, size_t ancestors, size_t descendants); bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const; CAmount GetSelectionAmount() const; }; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 865f6e41a6..25af6f49bd 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -417,13 +417,14 @@ 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); - // Check the OutputGroup's eligibility. Only add the eligible ones. - if (positive_only && group.GetSelectionAmount() <= 0) continue; - if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter)) groups_out.push_back(group); + if (group.EligibleForSpending(filter)) groups_out.push_back(group); + } } return groups_out; } @@ -462,8 +463,10 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vectorInsert(output, ancestors, descendants, positive_only); + // Filter for positive only before adding the output to group + if (!positive_only || output.GetEffectiveValue() > 0) { + group->Insert(output, ancestors, descendants); + } } // Now we go through the entire map and pull out the OutputGroups diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 1c731b95e5..1c2a561bef 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -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, /*positive_only=*/ true); + group.Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0); result.AddInput(group); } @@ -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, /*positive_only=*/ false); + static_groups.back().Insert(coin, /*ancestors=*/ 0, /*descendants=*/ 0); } return static_groups; } diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 1a0708c43a..95060c748f 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -29,8 +29,10 @@ static void GroupCoins(FuzzedDataProvider& fuzzed_data_provider, const std::vect auto output_group = OutputGroup(coin_params); bool valid_outputgroup{false}; for (auto& coin : coins) { - output_group.Insert(coin, /*ancestors=*/0, /*descendants=*/0, positive_only); - // If positive_only was specified, nothing may have been inserted, leading to an empty output group + if (!positive_only || (positive_only && coin.GetEffectiveValue() > 0)) { + output_group.Insert(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 valid_outputgroup = !positive_only || output_group.GetSelectionAmount() > 0; if (valid_outputgroup && fuzzed_data_provider.ConsumeBool()) { From d8e749bb840cf65065ed00561998255156126278 Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 31 Jul 2022 17:25:04 -0300 Subject: [PATCH 2/7] test: wallet, add coverage for outputs grouping process The following scenarios are covered: 1) 10 UTXO with the same script: partial spends is enabled --> outputs must not be grouped. 2) 10 UTXO with the same script: partial spends disabled --> outputs must be grouped. 3) 20 UTXO, 10 one from scriptA + 10 from scriptB: a) if partial spends is enabled --> outputs must not be grouped. b) if partial spends is not enabled --> 2 output groups expected (one per script). 3) Try to add a negative output (value - fee < 0): a) if "positive_only" is enabled --> negative output must be skipped. b) if "positive_only" is disabled --> negative output must be added. 4) Try to add a non-eligible UTXO (due not fulfilling the min depth target for "not mine" UTXOs) --> it must not be added to any group 5) Try to add a non-eligible UTXO (due not fulfilling the min depth target for "mine" UTXOs) --> it must not be added to any group 6) Surpass the 'OUTPUT_GROUP_MAX_ENTRIES' size and verify that a second partial group gets created. --- src/Makefile.test.include | 3 +- src/wallet/test/group_outputs_tests.cpp | 226 ++++++++++++++++++++++++ 2 files changed, 228 insertions(+), 1 deletion(-) create mode 100644 src/wallet/test/group_outputs_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index fa77e28736..83b721d8bf 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -179,7 +179,8 @@ BITCOIN_TESTS += \ wallet/test/ismine_tests.cpp \ wallet/test/rpc_util_tests.cpp \ wallet/test/scriptpubkeyman_tests.cpp \ - wallet/test/walletload_tests.cpp + wallet/test/walletload_tests.cpp \ + wallet/test/group_outputs_tests.cpp FUZZ_SUITE_LD_COMMON +=\ $(SQLITE_LIBS) \ diff --git a/src/wallet/test/group_outputs_tests.cpp b/src/wallet/test/group_outputs_tests.cpp new file mode 100644 index 0000000000..ad48661833 --- /dev/null +++ b/src/wallet/test/group_outputs_tests.cpp @@ -0,0 +1,226 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://www.opensource.org/licenses/mit-license.php. + +#include + +#include +#include +#include + +#include + +namespace wallet { +BOOST_FIXTURE_TEST_SUITE(group_outputs_tests, TestingSetup) + +static int nextLockTime = 0; + +static std::shared_ptr NewWallet(const node::NodeContext& m_node) +{ + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", CreateMockWalletDatabase()); + wallet->LoadWallet(); + LOCK(wallet->cs_wallet); + wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); + wallet->SetupDescriptorScriptPubKeyMans(); + return wallet; +} + +static void addCoin(CoinsResult& coins, + CWallet& wallet, + const CTxDestination& dest, + const CAmount& nValue, + bool is_from_me, + CFeeRate fee_rate = CFeeRate(0), + int depth = 6) +{ + CMutableTransaction tx; + tx.nLockTime = nextLockTime++; // so all transactions get different hashes + tx.vout.resize(1); + tx.vout[0].nValue = nValue; + tx.vout[0].scriptPubKey = GetScriptForDestination(dest); + + const uint256& txid = tx.GetHash(); + LOCK(wallet.cs_wallet); + auto ret = wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid), std::forward_as_tuple(MakeTransactionRef(std::move(tx)), TxStateInactive{})); + assert(ret.second); + CWalletTx& wtx = (*ret.first).second; + const auto& txout = wtx.tx->vout.at(0); + coins.Add(*Assert(OutputTypeFromDestination(dest)), + {COutPoint(wtx.GetHash(), 0), + txout, + depth, + CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), + /*spendable=*/ true, + /*solvable=*/ true, + /*safe=*/ true, + wtx.GetTxTime(), + is_from_me, + fee_rate}); +} + + CoinSelectionParams makeSelectionParams(FastRandomContext& rand, bool avoid_partial_spends) +{ + return CoinSelectionParams{ + rand, + /*change_output_size=*/ 0, + /*change_spend_size=*/ 0, + /*min_change_target=*/ CENT, + /*effective_feerate=*/ CFeeRate(0), + /*long_term_feerate=*/ CFeeRate(0), + /*discard_feerate=*/ CFeeRate(0), + /*tx_noinputs_size=*/ 0, + /*avoid_partial=*/ avoid_partial_spends, + }; +} + +class GroupVerifier +{ +public: + std::shared_ptr wallet{nullptr}; + CoinsResult coins_pool; + FastRandomContext rand; + + void GroupVerify(const CoinEligibilityFilter& filter, + bool avoid_partial_spends, + bool positive_only, + int expected_size) + { + std::vector groups = GroupOutputs(*wallet, + coins_pool.All(), + makeSelectionParams(rand, avoid_partial_spends), + filter, + positive_only); + BOOST_CHECK_EQUAL(groups.size(), expected_size); + } + + void GroupAndVerify(const CoinEligibilityFilter& filter, + int expected_with_partial_spends_size, + int expected_without_partial_spends_size, + bool positive_only) + { + // First avoid partial spends + GroupVerify(filter, /*avoid_partial_spends=*/false, positive_only, expected_with_partial_spends_size); + // Second don't avoid partial spends + GroupVerify(filter, /*avoid_partial_spends=*/true, positive_only, expected_without_partial_spends_size); + } +}; + +BOOST_AUTO_TEST_CASE(outputs_grouping_tests) +{ + const auto& wallet = NewWallet(m_node); + GroupVerifier group_verifier; + group_verifier.wallet = wallet; + + const CoinEligibilityFilter& BASIC_FILTER{1, 6, 0}; + + // ################################################################################# + // 10 outputs from different txs going to the same script + // 1) if partial spends is enabled --> must not be grouped + // 2) if partial spends is not enabled --> must be grouped into a single OutputGroup + // ################################################################################# + + unsigned long GROUP_SIZE = 10; + const CTxDestination dest = *Assert(wallet->GetNewDestination(OutputType::BECH32, "")); + for (unsigned long i = 0; i < GROUP_SIZE; i++) { + addCoin(group_verifier.coins_pool, *wallet, dest, 10 * COIN, /*is_from_me=*/true); + } + + group_verifier.GroupAndVerify(BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE, + /*expected_without_partial_spends_size=*/ 1, + /*positive_only=*/ true); + + // #################################################################################### + // 3) 10 more UTXO are added with a different script --> must be grouped into a single + // group for avoid partial spends and 10 different output groups for partial spends + // #################################################################################### + + const CTxDestination dest2 = *Assert(wallet->GetNewDestination(OutputType::BECH32, "")); + for (unsigned long i = 0; i < GROUP_SIZE; i++) { + addCoin(group_verifier.coins_pool, *wallet, dest2, 5 * COIN, /*is_from_me=*/true); + } + + group_verifier.GroupAndVerify(BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2, + /*expected_without_partial_spends_size=*/ 2, + /*positive_only=*/ true); + + // ################################################################################ + // 4) Now add a negative output --> which will be skipped if "positive_only" is set + // ################################################################################ + + const CTxDestination dest3 = *Assert(wallet->GetNewDestination(OutputType::BECH32, "")); + addCoin(group_verifier.coins_pool, *wallet, dest3, 1, true, CFeeRate(100)); + BOOST_CHECK(group_verifier.coins_pool.coins[OutputType::BECH32].back().GetEffectiveValue() <= 0); + + // First expect no changes with "positive_only" enabled + group_verifier.GroupAndVerify(BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2, + /*expected_without_partial_spends_size=*/ 2, + /*positive_only=*/ true); + + // Then expect changes with "positive_only" disabled + group_verifier.GroupAndVerify(BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2 + 1, + /*expected_without_partial_spends_size=*/ 3, + /*positive_only=*/ false); + + + // ############################################################################## + // 5) Try to add a non-eligible UTXO (due not fulfilling the min depth target for + // "not mine" UTXOs) --> it must not be added to any group + // ############################################################################## + + const CTxDestination dest4 = *Assert(wallet->GetNewDestination(OutputType::BECH32, "")); + addCoin(group_verifier.coins_pool, *wallet, dest4, 6 * COIN, + /*is_from_me=*/false, CFeeRate(0), /*depth=*/5); + + // Expect no changes from this round and the previous one (point 4) + group_verifier.GroupAndVerify(BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2 + 1, + /*expected_without_partial_spends_size=*/ 3, + /*positive_only=*/ false); + + + // ############################################################################## + // 6) Try to add a non-eligible UTXO (due not fulfilling the min depth target for + // "mine" UTXOs) --> it must not be added to any group + // ############################################################################## + + const CTxDestination dest5 = *Assert(wallet->GetNewDestination(OutputType::BECH32, "")); + addCoin(group_verifier.coins_pool, *wallet, dest5, 6 * COIN, + /*is_from_me=*/true, CFeeRate(0), /*depth=*/0); + + // Expect no changes from this round and the previous one (point 5) + group_verifier.GroupAndVerify(BASIC_FILTER, + /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2 + 1, + /*expected_without_partial_spends_size=*/ 3, + /*positive_only=*/ false); + + // ########################################################################################### + // 7) Surpass the OUTPUT_GROUP_MAX_ENTRIES and verify that a second partial group gets created + // ########################################################################################### + + const CTxDestination dest7 = *Assert(wallet->GetNewDestination(OutputType::BECH32, "")); + uint16_t NUM_SINGLE_ENTRIES = 101; + for (unsigned long i = 0; i < NUM_SINGLE_ENTRIES; i++) { // OUTPUT_GROUP_MAX_ENTRIES{100} + addCoin(group_verifier.coins_pool, *wallet, dest7, 9 * COIN, /*is_from_me=*/true); + } + + // Exclude partial groups only adds one more group to the previous test case (point 6) + int PREVIOUS_ROUND_COUNT = GROUP_SIZE * 2 + 1; + group_verifier.GroupAndVerify(BASIC_FILTER, + /*expected_with_partial_spends_size=*/ PREVIOUS_ROUND_COUNT + NUM_SINGLE_ENTRIES, + /*expected_without_partial_spends_size=*/ 4, + /*positive_only=*/ false); + + // Include partial groups should add one more group inside the "avoid partial spends" count + const CoinEligibilityFilter& avoid_partial_groups_filter{1, 6, 0, 0, /*include_partial=*/ true}; + group_verifier.GroupAndVerify(avoid_partial_groups_filter, + /*expected_with_partial_spends_size=*/ PREVIOUS_ROUND_COUNT + NUM_SINGLE_ENTRIES, + /*expected_without_partial_spends_size=*/ 5, + /*positive_only=*/ false); +} + +BOOST_AUTO_TEST_SUITE_END() +} // end namespace wallet From 461f0821a2dff0a27f755464b0eb92314fba1acd Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 1 Aug 2022 16:15:36 -0300 Subject: [PATCH 3/7] 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 From 34f54a0a3a54c59d3f062de69550ab3a3b077ea0 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 1 Aug 2022 16:59:50 -0300 Subject: [PATCH 4/7] wallet: decouple outputs grouping process from each ChooseSelectionResult Another step towards the single OutputGroups calculation goal --- src/wallet/coinselection.h | 7 +++++++ src/wallet/spend.cpp | 23 ++++++++++++++--------- src/wallet/spend.h | 7 ++----- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 3ca6520242..6443bc3087 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -242,6 +242,13 @@ struct OutputGroup CAmount GetSelectionAmount() const; }; +struct Groups { + // Stores 'OutputGroup' containing only positive UTXOs (value > 0). + std::vector positive_group; + // Stores 'OutputGroup' which may contain both positive and negative UTXOs. + std::vector mixed_group; +}; + /** Compute the waste for this result given the cost of change * and the opportunity cost of spending these inputs now vs in the future. * If change exists, waste = change_cost + inputs * (effective_feerate - long_term_feerate) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index a9c777dd70..d43ce004bb 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -499,8 +499,11 @@ util::Result AttemptSelection(const CWallet& wallet, const CAmo { // Run coin selection on each OutputType and compute the Waste Metric std::vector results; - for (const auto& it : available_coins.coins) { - auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, it.second, coin_selection_params)}; + for (const auto& [type, coins] : available_coins.coins) { + Groups groups; + groups.positive_group = GroupOutputs(wallet, coins, coin_selection_params, eligibility_filter, true /* positive_only */); + groups.mixed_group = GroupOutputs(wallet, coins, coin_selection_params, eligibility_filter, false /* positive_only */); + auto result{ChooseSelectionResult(nTargetValue, groups, coin_selection_params)}; // If any specific error message appears here, then something particularly wrong happened. if (HasErrorMsg(result)) return result; // So let's return the specific error. // Append the favorable result. @@ -514,31 +517,33 @@ util::Result AttemptSelection(const CWallet& wallet, const CAmo // over all available coins, which would allow mixing. // If TypesCount() <= 1, there is nothing to mix. if (allow_mixed_output_types && available_coins.TypesCount() > 1) { - return ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.All(), coin_selection_params); + const auto& all = available_coins.All(); + Groups groups; + groups.positive_group = GroupOutputs(wallet, all, coin_selection_params, eligibility_filter, true /* positive_only */); + groups.mixed_group = GroupOutputs(wallet, all, coin_selection_params, eligibility_filter, false /* positive_only */); + return ChooseSelectionResult(nTargetValue, groups, coin_selection_params); } // Either mixing is not allowed and we couldn't find a solution from any single OutputType, or mixing was allowed and we still couldn't // find a solution using all available coins return util::Error(); }; -util::Result ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector& available_coins, const CoinSelectionParams& coin_selection_params) +util::Result ChooseSelectionResult(const CAmount& nTargetValue, Groups& groups, const CoinSelectionParams& coin_selection_params) { // Vector of results. We will choose the best one based on waste. std::vector results; - std::vector positive_groups = GroupOutputs(wallet, available_coins, coin_selection_params, eligibility_filter, /*positive_only=*/true); - if (auto bnb_result{SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change)}) { + if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_cost_of_change)}) { results.push_back(*bnb_result); } // The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here. - std::vector all_groups = GroupOutputs(wallet, available_coins, coin_selection_params, eligibility_filter, /*positive_only=*/false); - if (auto knapsack_result{KnapsackSolver(all_groups, nTargetValue, coin_selection_params.m_min_change_target, coin_selection_params.rng_fast)}) { + if (auto knapsack_result{KnapsackSolver(groups.mixed_group, nTargetValue, coin_selection_params.m_min_change_target, coin_selection_params.rng_fast)}) { knapsack_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); results.push_back(*knapsack_result); } - if (auto srd_result{SelectCoinsSRD(positive_groups, nTargetValue, coin_selection_params.rng_fast)}) { + if (auto srd_result{SelectCoinsSRD(groups.positive_group, nTargetValue, coin_selection_params.rng_fast)}) { srd_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); results.push_back(*srd_result); } diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 105a1a2549..4bfb53a2e2 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -132,18 +132,15 @@ util::Result AttemptSelection(const CWallet& wallet, const CAmo * Multiple coin selection algorithms will be run and the input set that produces the least waste * (according to the waste metric) will be chosen. * - * param@[in] wallet The wallet which provides solving data for the coins * param@[in] nTargetValue The target value - * param@[in] eligilibity_filter A filter containing rules for which coins are allowed to be included in this selection - * param@[in] available_coins The struct of coins, organized by OutputType, available for selection prior to filtering + * param@[in] groups The struct containing the outputs grouped by script and divided by (1) positive only outputs and (2) all outputs (positive + negative). * param@[in] coin_selection_params Parameters for the coin selection * returns If successful, a SelectionResult containing the input set * If failed, returns (1) an empty error message if the target was not reached (general "Insufficient funds") * or (2) an specific error message if there was something particularly wrong (e.g. a selection * result that surpassed the tx max weight size). */ -util::Result ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector& available_coins, - const CoinSelectionParams& coin_selection_params); +util::Result ChooseSelectionResult(const CAmount& nTargetValue, Groups& groups, const CoinSelectionParams& coin_selection_params); // User manually selected inputs that must be part of the transaction struct PreSelectedInputs From 55962001da4f8467e52da502b05f5c0a85128fb0 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 23 Dec 2022 17:35:03 -0300 Subject: [PATCH 5/7] test: coinselector_tests refactor, use CoinsResult instead of plain std::vector No functional changes. Only cosmetic changes to simplify the follow-up commit. --- src/wallet/test/coinselector_tests.cpp | 66 +++++++++++++------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 1bb2e965d8..4f6b3a5e4f 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -139,7 +139,7 @@ inline std::vector& GroupCoins(const std::vector& availabl return static_groups; } -inline std::vector& KnapsackGroupOutputs(const std::vector& available_coins, CWallet& wallet, const CoinEligibilityFilter& filter) +inline std::vector& KnapsackGroupOutputs(const CoinsResult& available_coins, CWallet& wallet, const CoinEligibilityFilter& filter) { FastRandomContext rand{}; CoinSelectionParams coin_selection_params{ @@ -154,7 +154,7 @@ inline std::vector& KnapsackGroupOutputs(const std::vector /*avoid_partial=*/ false, }; static std::vector static_groups; - static_groups = GroupOutputs(wallet, available_coins, coin_selection_params, filter, /*positive_only=*/false); + static_groups = GroupOutputs(wallet, available_coins.All(), coin_selection_params, filter, /*positive_only=*/false); return static_groups; } @@ -418,25 +418,25 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) available_coins.Clear(); // with an empty wallet we can't even pay one cent - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 1 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 1 * CENT, CENT)); add_coin(available_coins, *wallet, 1*CENT, CFeeRate(0), 4); // add a new 1 cent coin // with a new 1 cent coin, we still can't find a mature 1 cent - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 1 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 1 * CENT, CENT)); // but we can find a new 1 cent - const auto result1 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result1 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result1); BOOST_CHECK_EQUAL(result1->GetSelectedValue(), 1 * CENT); add_coin(available_coins, *wallet, 2*CENT); // add a mature 2 cent coin // we can't make 3 cents of mature coins - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 3 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 3 * CENT, CENT)); // we can make 3 cents of new coins - const auto result2 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 3 * CENT, CENT); + const auto result2 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 3 * CENT, CENT); BOOST_CHECK(result2); BOOST_CHECK_EQUAL(result2->GetSelectedValue(), 3 * CENT); @@ -447,38 +447,38 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // now we have new: 1+10=11 (of which 10 was self-sent), and mature: 2+5+20=27. total = 38 // we can't make 38 cents only if we disallow new coins: - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 38 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 38 * CENT, CENT)); // we can't even make 37 cents if we don't allow new coins even if they're from us - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard_extra), 38 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard_extra), 38 * CENT, CENT)); // but we can make 37 cents if we accept new coins from ourself - const auto result3 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 37 * CENT, CENT); + const auto result3 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 37 * CENT, CENT); BOOST_CHECK(result3); BOOST_CHECK_EQUAL(result3->GetSelectedValue(), 37 * CENT); // and we can make 38 cents if we accept all new coins - const auto result4 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 38 * CENT, CENT); + const auto result4 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 38 * CENT, CENT); BOOST_CHECK(result4); BOOST_CHECK_EQUAL(result4->GetSelectedValue(), 38 * CENT); // try making 34 cents from 1,2,5,10,20 - we can't do it exactly - const auto result5 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 34 * CENT, CENT); + const auto result5 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 34 * CENT, CENT); BOOST_CHECK(result5); BOOST_CHECK_EQUAL(result5->GetSelectedValue(), 35 * CENT); // but 35 cents is closest BOOST_CHECK_EQUAL(result5->GetInputSet().size(), 3U); // the best should be 20+10+5. it's incredibly unlikely the 1 or 2 got included (but possible) // when we try making 7 cents, the smaller coins (1,2,5) are enough. We should see just 2+5 - const auto result6 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 7 * CENT, CENT); + const auto result6 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 7 * CENT, CENT); BOOST_CHECK(result6); BOOST_CHECK_EQUAL(result6->GetSelectedValue(), 7 * CENT); BOOST_CHECK_EQUAL(result6->GetInputSet().size(), 2U); // when we try making 8 cents, the smaller coins (1,2,5) are exactly enough. - const auto result7 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 8 * CENT, CENT); + const auto result7 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 8 * CENT, CENT); BOOST_CHECK(result7); BOOST_CHECK(result7->GetSelectedValue() == 8 * CENT); BOOST_CHECK_EQUAL(result7->GetInputSet().size(), 3U); // when we try making 9 cents, no subset of smaller coins is enough, and we get the next bigger coin (10) - const auto result8 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 9 * CENT, CENT); + const auto result8 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 9 * CENT, CENT); BOOST_CHECK(result8); BOOST_CHECK_EQUAL(result8->GetSelectedValue(), 10 * CENT); BOOST_CHECK_EQUAL(result8->GetInputSet().size(), 1U); @@ -493,12 +493,12 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 30*CENT); // now we have 6+7+8+20+30 = 71 cents total // check that we have 71 and not 72 - const auto result9 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 71 * CENT, CENT); + const auto result9 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 71 * CENT, CENT); BOOST_CHECK(result9); - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 72 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 72 * CENT, CENT)); // now try making 16 cents. the best smaller coins can do is 6+7+8 = 21; not as good at the next biggest coin, 20 - const auto result10 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 16 * CENT, CENT); + const auto result10 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 16 * CENT, CENT); BOOST_CHECK(result10); BOOST_CHECK_EQUAL(result10->GetSelectedValue(), 20 * CENT); // we should get 20 in one coin BOOST_CHECK_EQUAL(result10->GetInputSet().size(), 1U); @@ -506,7 +506,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 5*CENT); // now we have 5+6+7+8+20+30 = 75 cents total // now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, better than the next biggest coin, 20 - const auto result11 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 16 * CENT, CENT); + const auto result11 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 16 * CENT, CENT); BOOST_CHECK(result11); BOOST_CHECK_EQUAL(result11->GetSelectedValue(), 18 * CENT); // we should get 18 in 3 coins BOOST_CHECK_EQUAL(result11->GetInputSet().size(), 3U); @@ -514,13 +514,13 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 18*CENT); // now we have 5+6+7+8+18+20+30 // and now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, the same as the next biggest coin, 18 - const auto result12 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 16 * CENT, CENT); + const auto result12 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 16 * CENT, CENT); BOOST_CHECK(result12); BOOST_CHECK_EQUAL(result12->GetSelectedValue(), 18 * CENT); // we should get 18 in 1 coin BOOST_CHECK_EQUAL(result12->GetInputSet().size(), 1U); // because in the event of a tie, the biggest coin wins // now try making 11 cents. we should get 5+6 - const auto result13 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 11 * CENT, CENT); + const auto result13 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 11 * CENT, CENT); BOOST_CHECK(result13); BOOST_CHECK_EQUAL(result13->GetSelectedValue(), 11 * CENT); BOOST_CHECK_EQUAL(result13->GetInputSet().size(), 2U); @@ -530,12 +530,12 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 2*COIN); add_coin(available_coins, *wallet, 3*COIN); add_coin(available_coins, *wallet, 4*COIN); // now we have 5+6+7+8+18+20+30+100+200+300+400 = 1094 cents - const auto result14 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 95 * CENT, CENT); + const auto result14 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 95 * CENT, CENT); BOOST_CHECK(result14); BOOST_CHECK_EQUAL(result14->GetSelectedValue(), 1 * COIN); // we should get 1 BTC in 1 coin BOOST_CHECK_EQUAL(result14->GetInputSet().size(), 1U); - const auto result15 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 195 * CENT, CENT); + const auto result15 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 195 * CENT, CENT); BOOST_CHECK(result15); BOOST_CHECK_EQUAL(result15->GetSelectedValue(), 2 * COIN); // we should get 2 BTC in 1 coin BOOST_CHECK_EQUAL(result15->GetInputSet().size(), 1U); @@ -551,7 +551,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // try making 1 * CENT from the 1.5 * CENT // we'll get change smaller than CENT whatever happens, so can expect CENT exactly - const auto result16 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT, CENT); + const auto result16 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), CENT, CENT); BOOST_CHECK(result16); BOOST_CHECK_EQUAL(result16->GetSelectedValue(), CENT); @@ -559,7 +559,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 1111*CENT); // try making 1 from 0.1 + 0.2 + 0.3 + 0.4 + 0.5 + 1111 = 1112.5 - const auto result17 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result17 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result17); BOOST_CHECK_EQUAL(result17->GetSelectedValue(), 1 * CENT); // we should get the exact amount @@ -568,7 +568,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, CENT * 7 / 10); // and try again to make 1.0 * CENT - const auto result18 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result18 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result18); BOOST_CHECK_EQUAL(result18->GetSelectedValue(), 1 * CENT); // we should get the exact amount @@ -578,7 +578,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) for (int j = 0; j < 20; j++) add_coin(available_coins, *wallet, 50000 * COIN); - const auto result19 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 500000 * COIN, CENT); + const auto result19 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 500000 * COIN, CENT); BOOST_CHECK(result19); BOOST_CHECK_EQUAL(result19->GetSelectedValue(), 500000 * COIN); // we should get the exact amount BOOST_CHECK_EQUAL(result19->GetInputSet().size(), 10U); // in ten coins @@ -592,7 +592,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, CENT * 6 / 10); add_coin(available_coins, *wallet, CENT * 7 / 10); add_coin(available_coins, *wallet, 1111 * CENT); - const auto result20 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result20 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result20); BOOST_CHECK_EQUAL(result20->GetSelectedValue(), 1111 * CENT); // we get the bigger coin BOOST_CHECK_EQUAL(result20->GetInputSet().size(), 1U); @@ -603,7 +603,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, CENT * 6 / 10); add_coin(available_coins, *wallet, CENT * 8 / 10); add_coin(available_coins, *wallet, 1111 * CENT); - const auto result21 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT, CENT); + const auto result21 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), CENT, CENT); BOOST_CHECK(result21); BOOST_CHECK_EQUAL(result21->GetSelectedValue(), CENT); // we should get the exact amount BOOST_CHECK_EQUAL(result21->GetInputSet().size(), 2U); // in two coins 0.4+0.6 @@ -615,13 +615,13 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, CENT * 100); // trying to make 100.01 from these three coins - const auto result22 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT * 10001 / 100, CENT); + const auto result22 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), CENT * 10001 / 100, CENT); BOOST_CHECK(result22); BOOST_CHECK_EQUAL(result22->GetSelectedValue(), CENT * 10105 / 100); // we should get all coins BOOST_CHECK_EQUAL(result22->GetInputSet().size(), 3U); // but if we try to make 99.9, we should take the bigger of the two small coins to avoid small change - const auto result23 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT * 9990 / 100, CENT); + const auto result23 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), CENT * 9990 / 100, CENT); BOOST_CHECK(result23); BOOST_CHECK_EQUAL(result23->GetSelectedValue(), 101 * CENT); BOOST_CHECK_EQUAL(result23->GetInputSet().size(), 2U); @@ -636,7 +636,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // We only create the wallet once to save time, but we still run the coin selection RUN_TESTS times. for (int i = 0; i < RUN_TESTS; i++) { - const auto result24 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 2000, CENT); + const auto result24 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 2000, CENT); BOOST_CHECK(result24); if (amt - 2000 < CENT) { @@ -727,7 +727,7 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) add_coin(available_coins, *wallet, 1000 * COIN); add_coin(available_coins, *wallet, 3 * COIN); - const auto result = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 1003 * COIN, CENT, rand); + const auto result = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_standard), 1003 * COIN, CENT, rand); BOOST_CHECK(result); BOOST_CHECK_EQUAL(result->GetSelectedValue(), 1003 * COIN); BOOST_CHECK_EQUAL(result->GetInputSet().size(), 2U); From bd91ed1cb2cc804f824d1b204513ac2afb7d5e28 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 2 Aug 2022 11:54:20 -0300 Subject: [PATCH 6/7] wallet: unify outputs grouping process The 'GroupOutputs()' function performs the same calculations for only-positive and mixed groups, the only difference is that when we look for only-positive groups, we discard negative utxos. So, instead of wasting resources calling GroupOutputs() for positive-only first, then call it again to include the negative ones in the result, we can execute GroupOutputs() only once, including in the response both group types (positive-only and mixed). --- src/wallet/coinselection.cpp | 22 +++++ src/wallet/coinselection.h | 16 ++++ src/wallet/spend.cpp | 117 ++++++++++++++---------- src/wallet/spend.h | 9 +- src/wallet/test/coinselector_tests.cpp | 6 +- src/wallet/test/group_outputs_tests.cpp | 44 +++++---- 6 files changed, 143 insertions(+), 71 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index f99beabc70..3fd0280b8b 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -372,6 +372,28 @@ CAmount OutputGroup::GetSelectionAmount() const return m_subtract_fee_outputs ? m_value : effective_value; } +void OutputGroupTypeMap::Push(const OutputGroup& group, OutputType type, bool insert_positive, bool insert_mixed) +{ + if (group.m_outputs.empty()) return; + + Groups& groups = groups_by_type[type]; + if (insert_positive && group.GetSelectionAmount() > 0) { + groups.positive_group.emplace_back(group); + all_groups.positive_group.emplace_back(group); + } + if (insert_mixed) { + groups.mixed_group.emplace_back(group); + all_groups.mixed_group.emplace_back(group); + } +} + +std::optional OutputGroupTypeMap::Find(OutputType type) +{ + auto it_by_type = groups_by_type.find(type); + if (it_by_type == groups_by_type.end()) return std::nullopt; + return it_by_type->second; +} + 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 diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 6443bc3087..420892e06e 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -249,6 +250,21 @@ struct Groups { std::vector mixed_group; }; +/** Stores several 'Groups' whose were mapped by output type. */ +struct OutputGroupTypeMap +{ + // Maps output type to output groups. + std::map groups_by_type; + // All inserted groups, no type distinction. + Groups all_groups; + + // Based on the insert flag; appends group to the 'mixed_group' and, if value > 0, to the 'positive_group'. + // This affects both; the groups filtered by type and the overall groups container. + void Push(const OutputGroup& group, OutputType type, bool insert_positive, bool insert_mixed); + // Retrieves 'Groups' filtered by type + std::optional Find(OutputType type); +}; + /** Compute the waste for this result given the cost of change * and the opportunity cost of spending these inputs now vs in the future. * If change exists, waste = change_cost + inputs * (effective_feerate - long_term_feerate) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index d43ce004bb..9e1c9a6f6f 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -404,29 +404,33 @@ std::map> ListCoins(const CWallet& wallet) return result; } -std::vector GroupOutputs(const CWallet& wallet, const std::vector& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only) +OutputGroupTypeMap GroupOutputs(const CWallet& wallet, + const CoinsResult& coins, + const CoinSelectionParams& coin_sel_params, + const CoinEligibilityFilter& filter) { - std::vector groups_out; + OutputGroupTypeMap output_groups; if (!coin_sel_params.m_avoid_partial_spends) { - // Allowing partial spends means no grouping. Each COutput gets its own OutputGroup. - for (const COutput& output : outputs) { - // Skip outputs we cannot spend - if (!output.spendable) continue; + // Allowing partial spends means no grouping. Each COutput gets its own OutputGroup + for (const auto& [type, outputs] : coins.coins) { + for (const COutput& output : outputs) { + // Skip outputs we cannot spend + if (!output.spendable) continue; - size_t ancestors, descendants; - wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); + // Get mempool info + size_t ancestors, descendants; + wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); - // If 'positive_only' is set, filter for positive only before adding the coin - if (!positive_only || output.GetEffectiveValue() > 0) { - // Make an OutputGroup containing just this output - OutputGroup group{coin_sel_params}; + // Create a new group per output and add it to the all groups vector + OutputGroup group(coin_sel_params); group.Insert(std::make_shared(output), ancestors, descendants); - if (group.EligibleForSpending(filter)) groups_out.push_back(group); + if (!group.EligibleForSpending(filter)) continue; + output_groups.Push(group, type, /*insert_positive=*/true, /*insert_mixed=*/true); } } - return groups_out; + return output_groups; } // We want to combine COutputs that have the same scriptPubKey into single OutputGroups @@ -435,16 +439,12 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vector> spk_to_groups_map; - for (const auto& output : outputs) { - // Skip outputs we cannot spend - if (!output.spendable) continue; - - size_t ancestors, descendants; - wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); - CScript spk = output.txout.scriptPubKey; - - std::vector& groups = spk_to_groups_map[spk]; + typedef std::map, std::vector> ScriptPubKeyToOutgroup; + const auto& group_outputs = []( + const COutput& output, OutputType type, size_t ancestors, size_t descendants, + ScriptPubKeyToOutgroup& groups_map, const CoinSelectionParams& coin_sel_params, + bool positive_only) { + std::vector& groups = groups_map[std::make_pair(output.txout.scriptPubKey,type)]; if (groups.size() == 0) { // No OutputGroups for this scriptPubKey yet, add one @@ -467,28 +467,49 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vector 0) { group->Insert(std::make_shared(output), ancestors, descendants); } - } + }; - // Now we go through the entire map and pull out the OutputGroups - for (const auto& spk_and_groups_pair: spk_to_groups_map) { - const std::vector& groups_per_spk= spk_and_groups_pair.second; + ScriptPubKeyToOutgroup spk_to_groups_map; + ScriptPubKeyToOutgroup spk_to_positive_groups_map; + for (const auto& [type, outs] : coins.coins) { + for (const COutput& output : outs) { + // Skip outputs we cannot spend + if (!output.spendable) continue; - // Go through the vector backwards. This allows for the first item we deal with being the partial group. - for (auto group_it = groups_per_spk.rbegin(); group_it != groups_per_spk.rend(); group_it++) { - const OutputGroup& group = *group_it; + size_t ancestors, descendants; + wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); - // Don't include partial groups if there are full groups too and we don't want partial groups - if (group_it == groups_per_spk.rbegin() && groups_per_spk.size() > 1 && !filter.m_include_partial_groups) { - continue; - } - - // Check the OutputGroup's eligibility. Only add the eligible ones. - if (positive_only && group.GetSelectionAmount() <= 0) continue; - if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter)) groups_out.push_back(group); + group_outputs(output, type, ancestors, descendants, spk_to_groups_map, coin_sel_params, /*positive_only=*/ false); + group_outputs(output, type, ancestors, descendants, spk_to_positive_groups_map, + coin_sel_params, /*positive_only=*/ true); } } - return groups_out; + // Now we go through the entire maps and pull out the OutputGroups + const auto& push_output_groups = [&](const ScriptPubKeyToOutgroup& groups_map, bool positive_only) { + for (const auto& [script, groups] : groups_map) { + // Go through the vector backwards. This allows for the first item we deal with being the partial group. + for (auto group_it = groups.rbegin(); group_it != groups.rend(); group_it++) { + const OutputGroup& group = *group_it; + + if (!group.EligibleForSpending(filter)) continue; + + // Don't include partial groups if there are full groups too and we don't want partial groups + if (group_it == groups.rbegin() && groups.size() > 1 && !filter.m_include_partial_groups) { + continue; + } + + OutputType type = script.second; + // Either insert the group into the positive-only groups or the mixed ones. + output_groups.Push(group, type, positive_only, /*insert_mixed=*/!positive_only); + } + } + }; + + push_output_groups(spk_to_groups_map, /*positive_only=*/ false); + push_output_groups(spk_to_positive_groups_map, /*positive_only=*/ true); + + return output_groups; } // Returns true if the result contains an error and the message is not empty @@ -497,13 +518,15 @@ static bool HasErrorMsg(const util::Result& res) { return !util util::Result AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins, const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types) { + // Calculate all the output groups filtered by type at once + OutputGroupTypeMap groups = GroupOutputs(wallet, available_coins, coin_selection_params, {eligibility_filter}); + // Run coin selection on each OutputType and compute the Waste Metric std::vector results; for (const auto& [type, coins] : available_coins.coins) { - Groups groups; - groups.positive_group = GroupOutputs(wallet, coins, coin_selection_params, eligibility_filter, true /* positive_only */); - groups.mixed_group = GroupOutputs(wallet, coins, coin_selection_params, eligibility_filter, false /* positive_only */); - auto result{ChooseSelectionResult(nTargetValue, groups, coin_selection_params)}; + auto group_for_type = groups.Find(type); + if (!group_for_type) continue; + auto result{ChooseSelectionResult(nTargetValue, *group_for_type, coin_selection_params)}; // If any specific error message appears here, then something particularly wrong happened. if (HasErrorMsg(result)) return result; // So let's return the specific error. // Append the favorable result. @@ -517,11 +540,7 @@ util::Result AttemptSelection(const CWallet& wallet, const CAmo // over all available coins, which would allow mixing. // If TypesCount() <= 1, there is nothing to mix. if (allow_mixed_output_types && available_coins.TypesCount() > 1) { - const auto& all = available_coins.All(); - Groups groups; - groups.positive_group = GroupOutputs(wallet, all, coin_selection_params, eligibility_filter, true /* positive_only */); - groups.mixed_group = GroupOutputs(wallet, all, coin_selection_params, eligibility_filter, false /* positive_only */); - return ChooseSelectionResult(nTargetValue, groups, coin_selection_params); + return ChooseSelectionResult(nTargetValue, groups.all_groups, coin_selection_params); } // Either mixing is not allowed and we couldn't find a solution from any single OutputType, or mixing was allowed and we still couldn't // find a solution using all available coins diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 4bfb53a2e2..315f8a7fe9 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -106,7 +106,14 @@ const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const COutPoint& */ std::map> ListCoins(const CWallet& wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); -std::vector GroupOutputs(const CWallet& wallet, const std::vector& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only); +/** +* Group coins by the provided filters. +*/ +OutputGroupTypeMap GroupOutputs(const CWallet& wallet, + const CoinsResult& coins, + const CoinSelectionParams& coin_sel_params, + const CoinEligibilityFilter& filter); + /** * 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 diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 4f6b3a5e4f..841d762932 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -153,9 +153,9 @@ inline std::vector& KnapsackGroupOutputs(const CoinsResult& availab /*tx_noinputs_size=*/ 0, /*avoid_partial=*/ false, }; - static std::vector static_groups; - static_groups = GroupOutputs(wallet, available_coins.All(), coin_selection_params, filter, /*positive_only=*/false); - return static_groups; + static OutputGroupTypeMap static_groups; + static_groups = GroupOutputs(wallet, available_coins, coin_selection_params, {filter}); + return static_groups.all_groups.mixed_group; } // Branch and bound coin selection tests diff --git a/src/wallet/test/group_outputs_tests.cpp b/src/wallet/test/group_outputs_tests.cpp index ad48661833..352ec44d5d 100644 --- a/src/wallet/test/group_outputs_tests.cpp +++ b/src/wallet/test/group_outputs_tests.cpp @@ -80,28 +80,28 @@ public: CoinsResult coins_pool; FastRandomContext rand; - void GroupVerify(const CoinEligibilityFilter& filter, + void GroupVerify(const OutputType type, + const CoinEligibilityFilter& filter, bool avoid_partial_spends, bool positive_only, int expected_size) { - std::vector groups = GroupOutputs(*wallet, - coins_pool.All(), - makeSelectionParams(rand, avoid_partial_spends), - filter, - positive_only); - BOOST_CHECK_EQUAL(groups.size(), expected_size); + OutputGroupTypeMap groups = GroupOutputs(*wallet, coins_pool, makeSelectionParams(rand, avoid_partial_spends), filter); + std::vector& groups_out = positive_only ? groups.groups_by_type[type].positive_group : + groups.groups_by_type[type].mixed_group; + BOOST_CHECK_EQUAL(groups_out.size(), expected_size); } - void GroupAndVerify(const CoinEligibilityFilter& filter, + void GroupAndVerify(const OutputType type, + const CoinEligibilityFilter& filter, int expected_with_partial_spends_size, int expected_without_partial_spends_size, bool positive_only) { // First avoid partial spends - GroupVerify(filter, /*avoid_partial_spends=*/false, positive_only, expected_with_partial_spends_size); + GroupVerify(type, filter, /*avoid_partial_spends=*/false, positive_only, expected_with_partial_spends_size); // Second don't avoid partial spends - GroupVerify(filter, /*avoid_partial_spends=*/true, positive_only, expected_without_partial_spends_size); + GroupVerify(type, filter, /*avoid_partial_spends=*/true, positive_only, expected_without_partial_spends_size); } }; @@ -125,7 +125,8 @@ BOOST_AUTO_TEST_CASE(outputs_grouping_tests) addCoin(group_verifier.coins_pool, *wallet, dest, 10 * COIN, /*is_from_me=*/true); } - group_verifier.GroupAndVerify(BASIC_FILTER, + group_verifier.GroupAndVerify(OutputType::BECH32, + BASIC_FILTER, /*expected_with_partial_spends_size=*/ GROUP_SIZE, /*expected_without_partial_spends_size=*/ 1, /*positive_only=*/ true); @@ -140,7 +141,8 @@ BOOST_AUTO_TEST_CASE(outputs_grouping_tests) addCoin(group_verifier.coins_pool, *wallet, dest2, 5 * COIN, /*is_from_me=*/true); } - group_verifier.GroupAndVerify(BASIC_FILTER, + group_verifier.GroupAndVerify(OutputType::BECH32, + BASIC_FILTER, /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2, /*expected_without_partial_spends_size=*/ 2, /*positive_only=*/ true); @@ -154,13 +156,15 @@ BOOST_AUTO_TEST_CASE(outputs_grouping_tests) BOOST_CHECK(group_verifier.coins_pool.coins[OutputType::BECH32].back().GetEffectiveValue() <= 0); // First expect no changes with "positive_only" enabled - group_verifier.GroupAndVerify(BASIC_FILTER, + group_verifier.GroupAndVerify(OutputType::BECH32, + BASIC_FILTER, /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2, /*expected_without_partial_spends_size=*/ 2, /*positive_only=*/ true); // Then expect changes with "positive_only" disabled - group_verifier.GroupAndVerify(BASIC_FILTER, + group_verifier.GroupAndVerify(OutputType::BECH32, + BASIC_FILTER, /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2 + 1, /*expected_without_partial_spends_size=*/ 3, /*positive_only=*/ false); @@ -176,7 +180,8 @@ BOOST_AUTO_TEST_CASE(outputs_grouping_tests) /*is_from_me=*/false, CFeeRate(0), /*depth=*/5); // Expect no changes from this round and the previous one (point 4) - group_verifier.GroupAndVerify(BASIC_FILTER, + group_verifier.GroupAndVerify(OutputType::BECH32, + BASIC_FILTER, /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2 + 1, /*expected_without_partial_spends_size=*/ 3, /*positive_only=*/ false); @@ -192,7 +197,8 @@ BOOST_AUTO_TEST_CASE(outputs_grouping_tests) /*is_from_me=*/true, CFeeRate(0), /*depth=*/0); // Expect no changes from this round and the previous one (point 5) - group_verifier.GroupAndVerify(BASIC_FILTER, + group_verifier.GroupAndVerify(OutputType::BECH32, + BASIC_FILTER, /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2 + 1, /*expected_without_partial_spends_size=*/ 3, /*positive_only=*/ false); @@ -209,14 +215,16 @@ BOOST_AUTO_TEST_CASE(outputs_grouping_tests) // Exclude partial groups only adds one more group to the previous test case (point 6) int PREVIOUS_ROUND_COUNT = GROUP_SIZE * 2 + 1; - group_verifier.GroupAndVerify(BASIC_FILTER, + group_verifier.GroupAndVerify(OutputType::BECH32, + BASIC_FILTER, /*expected_with_partial_spends_size=*/ PREVIOUS_ROUND_COUNT + NUM_SINGLE_ENTRIES, /*expected_without_partial_spends_size=*/ 4, /*positive_only=*/ false); // Include partial groups should add one more group inside the "avoid partial spends" count const CoinEligibilityFilter& avoid_partial_groups_filter{1, 6, 0, 0, /*include_partial=*/ true}; - group_verifier.GroupAndVerify(avoid_partial_groups_filter, + group_verifier.GroupAndVerify(OutputType::BECH32, + avoid_partial_groups_filter, /*expected_with_partial_spends_size=*/ PREVIOUS_ROUND_COUNT + NUM_SINGLE_ENTRIES, /*expected_without_partial_spends_size=*/ 5, /*positive_only=*/ false); From 6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 3 Aug 2022 19:04:36 -0300 Subject: [PATCH 7/7] wallet: single output groups filtering and grouping process Optimizes coin selection by performing the "group outputs" procedure only once, outside the "attempt selection" process. Avoiding the repeated execution of the 'GroupOutputs' operation that occurs on each coin eligibility filters (up to 8 of them); then for every coin vector type plus one for all the coins together. This also let us not perform coin selection over coin eligibility filtered groups that don't add new elements. (because, if the previous round failed, and the subsequent one has the same coins, then this new round will fail again). --- src/bench/coin_selection.cpp | 3 +- src/wallet/coinselection.h | 9 ++++ src/wallet/spend.cpp | 63 +++++++++++++------------ src/wallet/spend.h | 15 +++--- src/wallet/test/coinselector_tests.cpp | 2 +- src/wallet/test/group_outputs_tests.cpp | 2 +- 6 files changed, 55 insertions(+), 39 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 8a78783517..265d4bf655 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -75,8 +75,9 @@ static void CoinSelection(benchmark::Bench& bench) /*tx_noinputs_size=*/ 0, /*avoid_partial=*/ false, }; + auto group = wallet::GroupOutputs(wallet, available_coins, coin_selection_params, {{filter_standard}})[filter_standard]; bench.run([&] { - auto result = AttemptSelection(wallet, 1003 * COIN, filter_standard, available_coins, coin_selection_params, /*allow_mixed_output_types=*/true); + auto result = AttemptSelection(1003 * COIN, group, coin_selection_params, /*allow_mixed_output_types=*/true); assert(result); assert(result->GetSelectedValue() == 1003 * COIN); assert(result->GetInputSet().size() == 2); diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 420892e06e..3abd22c207 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -193,6 +193,11 @@ struct CoinEligibilityFilter CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_ancestors) {} CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants) {} CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants, bool include_partial) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants), m_include_partial_groups(include_partial) {} + + bool operator<(const CoinEligibilityFilter& other) const { + return std::tie(conf_mine, conf_theirs, max_ancestors, max_descendants, m_include_partial_groups) + < std::tie(other.conf_mine, other.conf_theirs, other.max_ancestors, other.max_descendants, other.m_include_partial_groups); + } }; /** A group of UTXOs paid to the same output script. */ @@ -263,8 +268,12 @@ struct OutputGroupTypeMap void Push(const OutputGroup& group, OutputType type, bool insert_positive, bool insert_mixed); // Retrieves 'Groups' filtered by type std::optional Find(OutputType type); + // Different output types count + size_t TypesCount() { return groups_by_type.size(); } }; +typedef std::map FilteredOutputGroups; + /** Compute the waste for this result given the cost of change * and the opportunity cost of spending these inputs now vs in the future. * If change exists, waste = change_cost + inputs * (effective_feerate - long_term_feerate) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 9e1c9a6f6f..a8ecce119a 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -404,12 +404,12 @@ std::map> ListCoins(const CWallet& wallet) return result; } -OutputGroupTypeMap GroupOutputs(const CWallet& wallet, +FilteredOutputGroups GroupOutputs(const CWallet& wallet, const CoinsResult& coins, const CoinSelectionParams& coin_sel_params, - const CoinEligibilityFilter& filter) + const std::vector& filters) { - OutputGroupTypeMap output_groups; + FilteredOutputGroups filtered_groups; if (!coin_sel_params.m_avoid_partial_spends) { // Allowing partial spends means no grouping. Each COutput gets its own OutputGroup @@ -426,11 +426,15 @@ OutputGroupTypeMap GroupOutputs(const CWallet& wallet, OutputGroup group(coin_sel_params); group.Insert(std::make_shared(output), ancestors, descendants); - if (!group.EligibleForSpending(filter)) continue; - output_groups.Push(group, type, /*insert_positive=*/true, /*insert_mixed=*/true); + // Each filter maps to a different set of groups + for (const auto& sel_filter : filters) { + const auto& filter = sel_filter.filter; + if (!group.EligibleForSpending(filter)) continue; + filtered_groups[filter].Push(group, type, /*insert_positive=*/true, /*insert_mixed=*/true); + } } } - return output_groups; + return filtered_groups; } // We want to combine COutputs that have the same scriptPubKey into single OutputGroups @@ -492,16 +496,20 @@ OutputGroupTypeMap GroupOutputs(const CWallet& wallet, for (auto group_it = groups.rbegin(); group_it != groups.rend(); group_it++) { const OutputGroup& group = *group_it; - if (!group.EligibleForSpending(filter)) continue; + // Each filter maps to a different set of groups + for (const auto& sel_filter : filters) { + const auto& filter = sel_filter.filter; + if (!group.EligibleForSpending(filter)) continue; - // Don't include partial groups if there are full groups too and we don't want partial groups - if (group_it == groups.rbegin() && groups.size() > 1 && !filter.m_include_partial_groups) { - continue; + // Don't include partial groups if there are full groups too and we don't want partial groups + if (group_it == groups.rbegin() && groups.size() > 1 && !filter.m_include_partial_groups) { + continue; + } + + OutputType type = script.second; + // Either insert the group into the positive-only groups or the mixed ones. + filtered_groups[filter].Push(group, type, positive_only, /*insert_mixed=*/!positive_only); } - - OutputType type = script.second; - // Either insert the group into the positive-only groups or the mixed ones. - output_groups.Push(group, type, positive_only, /*insert_mixed=*/!positive_only); } } }; @@ -509,24 +517,19 @@ OutputGroupTypeMap GroupOutputs(const CWallet& wallet, push_output_groups(spk_to_groups_map, /*positive_only=*/ false); push_output_groups(spk_to_positive_groups_map, /*positive_only=*/ true); - return output_groups; + return filtered_groups; } // Returns true if the result contains an error and the message is not empty static bool HasErrorMsg(const util::Result& res) { return !util::ErrorString(res).empty(); } -util::Result AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins, +util::Result AttemptSelection(const CAmount& nTargetValue, OutputGroupTypeMap& groups, const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types) { - // Calculate all the output groups filtered by type at once - OutputGroupTypeMap groups = GroupOutputs(wallet, available_coins, coin_selection_params, {eligibility_filter}); - // Run coin selection on each OutputType and compute the Waste Metric std::vector results; - for (const auto& [type, coins] : available_coins.coins) { - auto group_for_type = groups.Find(type); - if (!group_for_type) continue; - auto result{ChooseSelectionResult(nTargetValue, *group_for_type, coin_selection_params)}; + for (auto& [type, group] : groups.groups_by_type) { + auto result{ChooseSelectionResult(nTargetValue, group, coin_selection_params)}; // If any specific error message appears here, then something particularly wrong happened. if (HasErrorMsg(result)) return result; // So let's return the specific error. // Append the favorable result. @@ -539,7 +542,7 @@ util::Result AttemptSelection(const CWallet& wallet, const CAmo // If we can't fund the transaction from any individual OutputType, run coin selection one last time // over all available coins, which would allow mixing. // If TypesCount() <= 1, there is nothing to mix. - if (allow_mixed_output_types && available_coins.TypesCount() > 1) { + if (allow_mixed_output_types && groups.TypesCount() > 1) { return ChooseSelectionResult(nTargetValue, groups.all_groups, coin_selection_params); } // Either mixing is not allowed and we couldn't find a solution from any single OutputType, or mixing was allowed and we still couldn't @@ -634,11 +637,6 @@ util::Result SelectCoins(const CWallet& wallet, CoinsResult& av return op_selection_result; } -struct SelectionFilter { - CoinEligibilityFilter filter; - bool allow_mixed_output_types{true}; -}; - util::Result AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& value_to_select, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) { unsigned int limit_ancestor_count = 0; @@ -693,12 +691,17 @@ util::Result AutomaticCoinSelection(const CWallet& wallet, Coin } } + // Group outputs and map them by coin eligibility filter + FilteredOutputGroups filtered_groups = GroupOutputs(wallet, available_coins, coin_selection_params, ordered_filters); + // Walk-through the filters until the solution gets found. // If no solution is found, return the first detailed error (if any). // future: add "error level" so the worst one can be picked instead. std::vector> res_detailed_errors; for (const auto& select_filter : ordered_filters) { - if (auto res{AttemptSelection(wallet, value_to_select, select_filter.filter, available_coins, + auto it = filtered_groups.find(select_filter.filter); + if (it == filtered_groups.end()) continue; + if (auto res{AttemptSelection(value_to_select, it->second, coin_selection_params, select_filter.allow_mixed_output_types)}) { return res; // result found } else { diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 315f8a7fe9..b8bc82db7a 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -106,13 +106,18 @@ const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const COutPoint& */ std::map> ListCoins(const CWallet& wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); +struct SelectionFilter { + CoinEligibilityFilter filter; + bool allow_mixed_output_types{true}; +}; + /** * Group coins by the provided filters. */ -OutputGroupTypeMap GroupOutputs(const CWallet& wallet, +FilteredOutputGroups GroupOutputs(const CWallet& wallet, const CoinsResult& coins, const CoinSelectionParams& coin_sel_params, - const CoinEligibilityFilter& filter); + const std::vector& filters); /** * Attempt to find a valid input set that preserves privacy by not mixing OutputTypes. @@ -120,10 +125,8 @@ OutputGroupTypeMap GroupOutputs(const CWallet& wallet, * the solution (according to the waste metric) will be chosen. If a valid input cannot be found from any * single OutputType, fallback to running `ChooseSelectionResult()` over all available coins. * - * param@[in] wallet The wallet which provides solving data for the coins * param@[in] nTargetValue The target value - * param@[in] eligilibity_filter A filter containing rules for which coins are allowed to be included in this selection - * param@[in] available_coins The struct of coins, organized by OutputType, available for selection prior to filtering + * param@[in] groups The grouped outputs mapped by coin eligibility filters * param@[in] coin_selection_params Parameters for the coin selection * param@[in] allow_mixed_output_types Relax restriction that SelectionResults must be of the same OutputType * returns If successful, a SelectionResult containing the input set @@ -131,7 +134,7 @@ OutputGroupTypeMap GroupOutputs(const CWallet& wallet, * or (2) an specific error message if there was something particularly wrong (e.g. a selection * result that surpassed the tx max weight size). */ -util::Result AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins, +util::Result AttemptSelection(const CAmount& nTargetValue, OutputGroupTypeMap& groups, const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types); /** diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 841d762932..8f626addde 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -154,7 +154,7 @@ inline std::vector& KnapsackGroupOutputs(const CoinsResult& availab /*avoid_partial=*/ false, }; static OutputGroupTypeMap static_groups; - static_groups = GroupOutputs(wallet, available_coins, coin_selection_params, {filter}); + static_groups = GroupOutputs(wallet, available_coins, coin_selection_params, {{filter}})[filter]; return static_groups.all_groups.mixed_group; } diff --git a/src/wallet/test/group_outputs_tests.cpp b/src/wallet/test/group_outputs_tests.cpp index 352ec44d5d..283e87989c 100644 --- a/src/wallet/test/group_outputs_tests.cpp +++ b/src/wallet/test/group_outputs_tests.cpp @@ -86,7 +86,7 @@ public: bool positive_only, int expected_size) { - OutputGroupTypeMap groups = GroupOutputs(*wallet, coins_pool, makeSelectionParams(rand, avoid_partial_spends), filter); + OutputGroupTypeMap groups = GroupOutputs(*wallet, coins_pool, makeSelectionParams(rand, avoid_partial_spends), {{filter}})[filter]; std::vector& groups_out = positive_only ? groups.groups_by_type[type].positive_group : groups.groups_by_type[type].mixed_group; BOOST_CHECK_EQUAL(groups_out.size(), expected_size);