From 06ec8f992890cac69cd0fd20224aa51fa311a181 Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 31 Jul 2022 17:22:04 -0300 Subject: 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(-) (limited to 'src') 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()) { -- cgit v1.2.3