diff options
Diffstat (limited to 'src/wallet')
| -rw-r--r-- | src/wallet/coinselection.cpp | 19 | ||||
| -rw-r--r-- | src/wallet/coinselection.h | 2 | ||||
| -rw-r--r-- | src/wallet/spend.cpp | 8 | ||||
| -rw-r--r-- | src/wallet/test/coinselector_tests.cpp | 13 | ||||
| -rw-r--r-- | src/wallet/test/fuzz/coinselection.cpp | 10 |
5 files changed, 45 insertions, 7 deletions
diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 5ea9b7fad3..75027520dd 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -15,6 +15,13 @@ #include <optional> namespace wallet { +// Common selection error across the algorithms +static util::Result<SelectionResult> ErrorMaxWeightExceeded() +{ + return util::Error{_("The inputs size exceeds the maximum weight. " + "Please try sending a smaller amount or manually consolidating your wallet's UTXOs")}; +} + // Descending order comparator struct { bool operator()(const OutputGroup& a, const OutputGroup& b) const @@ -253,7 +260,7 @@ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::v } util::Result<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue, - CAmount change_target, FastRandomContext& rng) + CAmount change_target, FastRandomContext& rng, int max_weight) { SelectionResult result(nTargetValue, SelectionAlgorithm::KNAPSACK); @@ -313,6 +320,16 @@ util::Result<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, c } } + // If the result exceeds the maximum allowed size, return closest UTXO above the target + if (result.GetWeight() > max_weight) { + // No coin above target, nothing to do. + if (!lowest_larger) return ErrorMaxWeightExceeded(); + + // Return closest UTXO above target + result.Clear(); + result.AddInput(*lowest_larger); + } + if (LogAcceptCategory(BCLog::SELECTCOINS, BCLog::Level::Debug)) { std::string log_message{"Coin selection best subset: "}; for (unsigned int i = 0; i < applicable_groups.size(); i++) { diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 95a3bb2bc8..807e75ee30 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -422,7 +422,7 @@ util::Result<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utx // Original coin selection algorithm as a fallback util::Result<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue, - CAmount change_target, FastRandomContext& rng); + CAmount change_target, FastRandomContext& rng, int max_weight); } // namespace wallet #endif // BITCOIN_WALLET_COINSELECTION_H diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index a55e4c45a8..b28da6ec42 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -563,12 +563,18 @@ util::Result<SelectionResult> ChooseSelectionResult(const CAmount& nTargetValue, } }; + // Maximum allowed weight + int max_inputs_weight = MAX_STANDARD_TX_WEIGHT - (coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR); + if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_cost_of_change)}) { results.push_back(*bnb_result); } else append_error(bnb_result); + // As Knapsack and SRD can create change, also deduce change weight. + max_inputs_weight -= (coin_selection_params.change_output_size * WITNESS_SCALE_FACTOR); + // 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. - if (auto knapsack_result{KnapsackSolver(groups.mixed_group, 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, max_inputs_weight)}) { 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); } else append_error(knapsack_result); diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 8f626addde..db8322e3cb 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -87,6 +87,14 @@ static void add_coin(CoinsResult& available_coins, CWallet& wallet, const CAmoun available_coins.Add(OutputType::BECH32, {COutPoint(wtx.GetHash(), nInput), txout, nAge, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe, feerate}); } +// Helper +std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue, + CAmount change_target, FastRandomContext& rng) +{ + auto res{KnapsackSolver(groups, nTargetValue, change_target, rng, MAX_STANDARD_TX_WEIGHT)}; + return res ? std::optional<SelectionResult>(*res) : std::nullopt; +} + /** Check if SelectionResult a is equivalent to SelectionResult b. * Equivalent means same input values, but maybe different inputs (i.e. same value, different prevout) */ static bool EquivalentResult(const SelectionResult& a, const SelectionResult& b) @@ -987,7 +995,10 @@ BOOST_AUTO_TEST_CASE(check_max_weight) chain); BOOST_CHECK(result); - BOOST_CHECK(has_coin(result->GetInputSet(), CAmount(50 * COIN))); + // Verify that only the 50 BTC UTXO was selected + const auto& selection_res = result->GetInputSet(); + BOOST_CHECK(selection_res.size() == 1); + BOOST_CHECK((*selection_res.begin())->GetEffectiveValue() == 50 * COIN); } { diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 304190eec1..4878b24c30 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include <policy/feerate.h> +#include <policy/policy.h> #include <primitives/transaction.h> #include <test/fuzz/FuzzedDataProvider.h> #include <test/fuzz/fuzz.h> @@ -44,6 +45,9 @@ static void GroupCoins(FuzzedDataProvider& fuzzed_data_provider, const std::vect if (valid_outputgroup) output_groups.push_back(output_group); } +// Returns true if the result contains an error and the message is not empty +static bool HasErrorMsg(const util::Result<SelectionResult>& res) { return !util::ErrorString(res).empty(); } + FUZZ_TARGET(coinselection) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; @@ -90,12 +94,12 @@ FUZZ_TARGET(coinselection) 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); + auto result_knapsack = KnapsackSolver(group_all, target, change_target, fast_random_context, MAX_STANDARD_TX_WEIGHT); 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. - if (total_balance >= target && subtract_fee_outputs) { + // effective values, Knapsack should always find a solution (unless the selection exceeded the max tx weight). + if (total_balance >= target && subtract_fee_outputs && !HasErrorMsg(result_knapsack)) { assert(result_knapsack); } } |