From 4fef5344288e454460b80db0316294e1ec1ad8ad Mon Sep 17 00:00:00 2001 From: S3RK <1466284+S3RK@users.noreply.github.com> Date: Wed, 6 Jul 2022 09:03:01 +0200 Subject: wallet: use GetChange() when computing waste --- src/wallet/coinselection.cpp | 12 +++++++++--- src/wallet/coinselection.h | 2 +- src/wallet/spend.cpp | 8 ++++---- src/wallet/test/coinselector_tests.cpp | 13 +++++++++---- src/wallet/test/fuzz/coinselection.cpp | 4 ++-- 5 files changed, 25 insertions(+), 14 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 58d297724d..b568e90998 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -156,7 +156,7 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo for (const size_t& i : best_selection) { result.AddInput(utxo_pool.at(i)); } - result.ComputeAndSetWaste(CAmount{0}); + result.ComputeAndSetWaste(cost_of_change, cost_of_change, CAmount{0}); assert(best_waste == result.GetWaste()); return result; @@ -406,9 +406,15 @@ CAmount GenerateChangeTarget(const CAmount payment_value, const CAmount change_f } } -void SelectionResult::ComputeAndSetWaste(CAmount change_cost) +void SelectionResult::ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee) { - m_waste = GetSelectionWaste(m_selected_inputs, change_cost, m_target, m_use_effective); + const CAmount change = GetChange(min_viable_change, change_fee); + + if (change > 0) { + m_waste = GetSelectionWaste(m_selected_inputs, change_cost, m_target, m_use_effective); + } else { + m_waste = GetSelectionWaste(m_selected_inputs, 0, m_target, m_use_effective); + } } CAmount SelectionResult::GetWaste() const diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index dd52241e2a..761c2be0b3 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -310,7 +310,7 @@ public: void AddInput(const OutputGroup& group); /** Calculates and stores the waste for this selection via GetSelectionWaste */ - void ComputeAndSetWaste(CAmount change_cost); + void ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee); [[nodiscard]] CAmount GetWaste() const; void Merge(const SelectionResult& other); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index f9429907cd..718c499e7b 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -497,12 +497,12 @@ std::optional ChooseSelectionResult(const CWallet& wallet, cons // 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)}) { - knapsack_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change); + 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)}) { - srd_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change); + 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); } @@ -574,7 +574,7 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL); result.AddInput(preset_inputs); if (result.GetSelectedValue() < nTargetValue) return std::nullopt; - result.ComputeAndSetWaste(coin_selection_params.m_cost_of_change); + result.ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); return result; } @@ -671,7 +671,7 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a // Add preset inputs to result res->Merge(preselected); if (res->GetAlgo() == SelectionAlgorithm::MANUAL) { - res->ComputeAndSetWaste(coin_selection_params.m_cost_of_change); + res->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); } return res; diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index a4a7216436..3c21dae712 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -248,9 +248,9 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) // Iteration exhaustion test CAmount target = make_hard_case(17, utxo_pool); - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), target, 0)); // Should exhaust + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), target, 1)); // Should exhaust target = make_hard_case(14, utxo_pool); - const auto result7 = SelectCoinsBnB(GroupCoins(utxo_pool), target, 0); // Should not exhaust + const auto result7 = SelectCoinsBnB(GroupCoins(utxo_pool), target, 1); // Should not exhaust BOOST_CHECK(result7); // Test same value early bailout optimization @@ -289,8 +289,8 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) // Make sure that effective value is working in AttemptSelection when BnB is used CoinSelectionParams coin_selection_params_bnb{ rand, - /*change_output_size=*/ 0, - /*change_spend_size=*/ 0, + /*change_output_size=*/ 31, + /*change_spend_size=*/ 68, /*min_change_target=*/ 0, /*effective_feerate=*/ CFeeRate(3000), /*long_term_feerate=*/ CFeeRate(1000), @@ -298,6 +298,9 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) /*tx_noinputs_size=*/ 0, /*avoid_partial=*/ false, }; + coin_selection_params_bnb.m_change_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_output_size); + coin_selection_params_bnb.m_cost_of_change = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_spend_size) + coin_selection_params_bnb.m_change_fee; + coin_selection_params_bnb.min_viable_change = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_spend_size); { std::unique_ptr wallet = std::make_unique(m_node.chain.get(), "", m_args, CreateMockWalletDatabase()); wallet->LoadWallet(); @@ -777,6 +780,8 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test) /*tx_noinputs_size=*/ 0, /*avoid_partial=*/ false, }; + cs_params.m_cost_of_change = 1; + cs_params.min_viable_change = 1; CCoinControl cc; const auto result = SelectCoins(*wallet, available_coins, target, cc, cs_params); BOOST_CHECK(result); diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 52bd74126d..90a328179e 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -85,11 +85,11 @@ FUZZ_TARGET(coinselection) const auto result_bnb = SelectCoinsBnB(group_pos, target, cost_of_change); auto result_srd = SelectCoinsSRD(group_pos, target, fast_random_context); - if (result_srd) result_srd->ComputeAndSetWaste(cost_of_change); + if (result_srd) result_srd->ComputeAndSetWaste(cost_of_change, cost_of_change, 0); CAmount change_target{GenerateChangeTarget(target, coin_params.m_change_fee, fast_random_context)}; auto result_knapsack = KnapsackSolver(group_all, target, change_target, fast_random_context); - if (result_knapsack) result_knapsack->ComputeAndSetWaste(cost_of_change); + if (result_knapsack) result_knapsack->ComputeAndSetWaste(cost_of_change, cost_of_change, 0); // If the total balance is sufficient for the target and we are not using // effective values, Knapsack should always find a solution. -- cgit v1.2.3