From 941b8c6539d72890fd4e36fc900be9c300e1d737 Mon Sep 17 00:00:00 2001 From: Murch Date: Fri, 2 Jun 2023 16:27:36 -0400 Subject: [PATCH 1/2] [bug] Increase SRD target by change_fee MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I discovered via fuzzing of another coin selection approach that at extremely high feerates SRD may find input sets that lead to transactions without change outputs. This is an unintended outcome since SRD is meant to always produce a transaction with a change output—we use other algorithms to specifically search for changeless solutions. The issue occures when the flat allowance of 50,000 ṩ for change is insufficient to pay for the creation of a change output with a non-dust amount, at and above 1,613 ṩ/vB. Increasing the change budget by change_fees makes SRD behave as expected at any feerates. --- src/wallet/coinselection.cpp | 4 ++-- src/wallet/coinselection.h | 2 +- src/wallet/spend.cpp | 2 +- src/wallet/test/coinselector_tests.cpp | 2 +- src/wallet/test/fuzz/coinselection.cpp | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index bd74025f096..d6b9b68e1f4 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -191,7 +191,7 @@ public: } }; -util::Result SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, FastRandomContext& rng, +util::Result SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, CAmount change_fee, FastRandomContext& rng, int max_weight) { SelectionResult result(target_value, SelectionAlgorithm::SRD); @@ -201,7 +201,7 @@ util::Result SelectCoinsSRD(const std::vector& utx // barely meets the target. Just use the lower bound change target instead of the randomly // generated one, since SRD will result in a random change amount anyway; avoid making the // target needlessly large. - target_value += CHANGE_LOWER; + target_value += CHANGE_LOWER + change_fee; std::vector indexes; indexes.resize(utxo_pool.size()); diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 432d7d14314..afd868fc89e 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -421,7 +421,7 @@ util::Result SelectCoinsBnB(std::vector& utxo_pool * @param[in] max_weight The maximum allowed weight for a selection result to be valid * @returns If successful, a valid SelectionResult, otherwise, util::Error */ -util::Result SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, FastRandomContext& rng, +util::Result SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, CAmount change_fee, FastRandomContext& rng, int max_weight); // Original coin selection algorithm as a fallback diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 99c6582f9c7..a3728223fbe 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -583,7 +583,7 @@ util::Result ChooseSelectionResult(const CAmount& nTargetValue, results.push_back(*knapsack_result); } else append_error(knapsack_result); - if (auto srd_result{SelectCoinsSRD(groups.positive_group, nTargetValue, coin_selection_params.rng_fast, max_inputs_weight)}) { + if (auto srd_result{SelectCoinsSRD(groups.positive_group, nTargetValue, coin_selection_params.m_change_fee, coin_selection_params.rng_fast, max_inputs_weight)}) { 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); } else append_error(srd_result); diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index a5394d88164..5c3a5339401 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -968,7 +968,7 @@ static util::Result SelectCoinsSRD(const CAmount& target, std::unique_ptr wallet = NewWallet(m_node); CoinEligibilityFilter filter(0, 0, 0); // accept all coins without ancestors Groups group = GroupOutputs(*wallet, coin_setup(*wallet), cs_params, {{filter}})[filter].all_groups; - return SelectCoinsSRD(group.positive_group, target, cs_params.rng_fast, max_weight); + return SelectCoinsSRD(group.positive_group, target, cs_params.m_change_fee, cs_params.rng_fast, max_weight); } BOOST_AUTO_TEST_CASE(srd_tests) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 9be8efab62f..bf457073e48 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -90,7 +90,7 @@ FUZZ_TARGET(coinselection) // Run coinselection algorithms const auto result_bnb = SelectCoinsBnB(group_pos, target, cost_of_change, MAX_STANDARD_TX_WEIGHT); - auto result_srd = SelectCoinsSRD(group_pos, target, fast_random_context, MAX_STANDARD_TX_WEIGHT); + auto result_srd = SelectCoinsSRD(group_pos, target, coin_params.m_change_fee, fast_random_context, MAX_STANDARD_TX_WEIGHT); 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)}; From 1771daa815ec014276cfcb30c934b0eaff4d72bf Mon Sep 17 00:00:00 2001 From: Murch Date: Wed, 14 Jun 2023 16:52:08 -0400 Subject: [PATCH 2/2] [fuzz] Show that SRD budgets for non-dust change Adding this assert to the fuzz test without increasing the change target by the change_fee resulted in a crash within a few seconds. --- src/wallet/test/fuzz/coinselection.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index bf457073e48..bc935157b1b 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -91,7 +91,10 @@ FUZZ_TARGET(coinselection) const auto result_bnb = SelectCoinsBnB(group_pos, target, cost_of_change, MAX_STANDARD_TX_WEIGHT); auto result_srd = SelectCoinsSRD(group_pos, target, coin_params.m_change_fee, fast_random_context, MAX_STANDARD_TX_WEIGHT); - if (result_srd) result_srd->ComputeAndSetWaste(cost_of_change, cost_of_change, 0); + if (result_srd) { + assert(result_srd->GetChange(CHANGE_LOWER, coin_params.m_change_fee) > 0); // Demonstrate that SRD creates change of at least CHANGE_LOWER + 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, MAX_STANDARD_TX_WEIGHT);