diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 265d4bf655..11ce14728d 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -115,7 +116,7 @@ static void BnBExhaustion(benchmark::Bench& bench) bench.run([&] { // Benchmark CAmount target = make_hard_case(17, utxo_pool); - SelectCoinsBnB(utxo_pool, target, 0); // Should exhaust + SelectCoinsBnB(utxo_pool, target, 0, MAX_STANDARD_TX_WEIGHT); // Should exhaust // Cleanup utxo_pool.clear(); diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 9cf61dbe51..f704d03b5f 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -13,8 +13,16 @@ #include #include +#include namespace wallet { +// Common selection error across the algorithms +static util::Result 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 @@ -63,11 +71,13 @@ struct { static const size_t TOTAL_TRIES = 100000; -std::optional SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change) +util::Result SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, + int max_weight) { SelectionResult result(selection_target, SelectionAlgorithm::BNB); CAmount curr_value = 0; std::vector curr_selection; // selected utxo indexes + int curr_selection_weight = 0; // sum of selected utxo weight // Calculate curr_available_value CAmount curr_available_value = 0; @@ -78,7 +88,7 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo curr_available_value += utxo.GetSelectionAmount(); } if (curr_available_value < selection_target) { - return std::nullopt; + return util::Error(); } // Sort the utxo_pool @@ -89,6 +99,7 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo CAmount best_waste = MAX_MONEY; bool is_feerate_high = utxo_pool.at(0).fee > utxo_pool.at(0).long_term_fee; + bool max_tx_weight_exceeded = false; // Depth First search loop for choosing the UTXOs for (size_t curr_try = 0, utxo_pool_index = 0; curr_try < TOTAL_TRIES; ++curr_try, ++utxo_pool_index) { @@ -98,6 +109,9 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo curr_value > selection_target + cost_of_change || // Selected value is out of range, go back and try other branch (curr_waste > best_waste && is_feerate_high)) { // Don't select things which we know will be more wasteful if the waste is increasing backtrack = true; + } else if (curr_selection_weight > max_weight) { // Exceeding weight for standard tx, cannot find more solutions by adding more inputs + max_tx_weight_exceeded = true; // at least one selection attempt exceeded the max weight + backtrack = true; } else if (curr_value >= selection_target) { // Selected value is within range curr_waste += (curr_value - selection_target); // This is the excess value which is added to the waste for the below comparison // Adding another UTXO after this check could bring the waste down if the long term fee is higher than the current fee. @@ -127,6 +141,7 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo OutputGroup& utxo = utxo_pool.at(utxo_pool_index); curr_value -= utxo.GetSelectionAmount(); curr_waste -= utxo.fee - utxo.long_term_fee; + curr_selection_weight -= utxo.m_weight; curr_selection.pop_back(); } else { // Moving forwards, continuing down this branch OutputGroup& utxo = utxo_pool.at(utxo_pool_index); @@ -146,13 +161,14 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo curr_selection.push_back(utxo_pool_index); curr_value += utxo.GetSelectionAmount(); curr_waste += utxo.fee - utxo.long_term_fee; + curr_selection_weight += utxo.m_weight; } } } // Check for solution if (best_selection.empty()) { - return std::nullopt; + return max_tx_weight_exceeded ? ErrorMaxWeightExceeded() : util::Error(); } // Set output set @@ -165,9 +181,20 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo return result; } -std::optional SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, FastRandomContext& rng) +class MinOutputGroupComparator +{ +public: + int operator() (const OutputGroup& group1, const OutputGroup& group2) const + { + return group1.GetSelectionAmount() > group2.GetSelectionAmount(); + } +}; + +util::Result SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, FastRandomContext& rng, + int max_weight) { SelectionResult result(target_value, SelectionAlgorithm::SRD); + std::priority_queue, MinOutputGroupComparator> heap; // Include change for SRD as we want to avoid making really small change if the selection just // barely meets the target. Just use the lower bound change target instead of the randomly @@ -181,16 +208,40 @@ std::optional SelectCoinsSRD(const std::vector& ut Shuffle(indexes.begin(), indexes.end(), rng); CAmount selected_eff_value = 0; + int weight = 0; + bool max_tx_weight_exceeded = false; for (const size_t i : indexes) { const OutputGroup& group = utxo_pool.at(i); Assume(group.GetSelectionAmount() > 0); + + // Add group to selection + heap.push(group); selected_eff_value += group.GetSelectionAmount(); - result.AddInput(group); + weight += group.m_weight; + + // If the selection weight exceeds the maximum allowed size, remove the least valuable inputs until we + // are below max weight. + if (weight > max_weight) { + max_tx_weight_exceeded = true; // mark it in case we don't find any useful result. + do { + const OutputGroup& to_remove_group = heap.top(); + selected_eff_value -= to_remove_group.GetSelectionAmount(); + weight -= to_remove_group.m_weight; + heap.pop(); + } while (!heap.empty() && weight > max_weight); + } + + // Now check if we are above the target if (selected_eff_value >= target_value) { + // Result found, add it. + while (!heap.empty()) { + result.AddInput(heap.top()); + heap.pop(); + } return result; } } - return std::nullopt; + return max_tx_weight_exceeded ? ErrorMaxWeightExceeded() : util::Error(); } /** Find a subset of the OutputGroups that is at least as large as, but as close as possible to, the @@ -252,8 +303,8 @@ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::v } } -std::optional KnapsackSolver(std::vector& groups, const CAmount& nTargetValue, - CAmount change_target, FastRandomContext& rng) +util::Result KnapsackSolver(std::vector& groups, const CAmount& nTargetValue, + CAmount change_target, FastRandomContext& rng, int max_weight) { SelectionResult result(nTargetValue, SelectionAlgorithm::KNAPSACK); @@ -286,7 +337,7 @@ std::optional KnapsackSolver(std::vector& groups, } if (nTotalLower < nTargetValue) { - if (!lowest_larger) return std::nullopt; + if (!lowest_larger) return util::Error(); result.AddInput(*lowest_larger); return result; } @@ -313,6 +364,16 @@ std::optional KnapsackSolver(std::vector& groups, } } + // 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 5a7b748be1..723f5bbfb3 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -13,6 +13,7 @@ #include #include #include +#include #include @@ -408,20 +409,24 @@ public: int GetWeight() const { return m_weight; } }; -std::optional SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change); +util::Result SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, + int max_weight); /** Select coins by Single Random Draw. OutputGroups are selected randomly from the eligible * outputs until the target is satisfied * * @param[in] utxo_pool The positive effective value OutputGroups eligible for selection * @param[in] target_value The target value to select for - * @returns If successful, a SelectionResult, otherwise, std::nullopt + * @param[in] rng The randomness source to shuffle coins + * @param[in] max_weight The maximum allowed weight for a selection result to be valid + * @returns If successful, a valid SelectionResult, otherwise, util::Error */ -std::optional SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, FastRandomContext& rng); +util::Result SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, FastRandomContext& rng, + int max_weight); // Original coin selection algorithm as a fallback -std::optional KnapsackSolver(std::vector& groups, const CAmount& nTargetValue, - CAmount change_target, FastRandomContext& rng); +util::Result KnapsackSolver(std::vector& groups, const CAmount& nTargetValue, + 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 57f3785a3a..c3303419d4 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -559,42 +559,45 @@ util::Result ChooseSelectionResult(const CAmount& nTargetValue, { // Vector of results. We will choose the best one based on waste. std::vector results; + std::vector> errors; + auto append_error = [&] (const util::Result& result) { + // If any specific error message appears here, then something different from a simple "no selection found" happened. + // Let's save it, so it can be retrieved to the user if no other selection algorithm succeeded. + if (HasErrorMsg(result)) { + errors.emplace_back(result); + } + }; - if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_cost_of_change)}) { + // 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, max_inputs_weight)}) { 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); - if (auto srd_result{SelectCoinsSRD(groups.positive_group, nTargetValue, coin_selection_params.rng_fast)}) { + if (auto srd_result{SelectCoinsSRD(groups.positive_group, nTargetValue, 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); if (results.empty()) { - // No solution found - return util::Error(); - } - - std::vector eligible_results; - std::copy_if(results.begin(), results.end(), std::back_inserter(eligible_results), [coin_selection_params](const SelectionResult& result) { - const auto initWeight{coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR}; - return initWeight + result.GetWeight() <= static_cast(MAX_STANDARD_TX_WEIGHT); - }); - - if (eligible_results.empty()) { - return util::Error{_("The inputs size exceeds the maximum weight. " - "Please try sending a smaller amount or manually consolidating your wallet's UTXOs")}; + // No solution found, retrieve the first explicit error (if any). + // future: add 'severity level' to errors so the worst one can be retrieved instead of the first one. + return errors.empty() ? util::Error() : errors.front(); } // Choose the result with the least waste // If the waste is the same, choose the one which spends more inputs. - auto& best_result = *std::min_element(eligible_results.begin(), eligible_results.end()); - return best_result; + return *std::min_element(results.begin(), results.end()); } util::Result SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const PreSelectedInputs& pre_set_inputs, diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 8f626addde..7f66179517 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -68,7 +68,7 @@ static void add_coin(const CAmount& nValue, int nInput, CoinSet& set, CAmount fe 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) +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, int custom_size = 0) { CMutableTransaction tx; tx.nLockTime = nextLockTime++; // so all transactions get different hashes @@ -84,7 +84,21 @@ static void add_coin(CoinsResult& available_coins, CWallet& wallet, const CAmoun assert(ret.second); CWalletTx& wtx = (*ret.first).second; const auto& txout = wtx.tx->vout.at(nInput); - 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}); + available_coins.Add(OutputType::BECH32, {COutPoint(wtx.GetHash(), nInput), txout, nAge, custom_size == 0 ? CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr) : custom_size, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe, feerate}); +} + +// Helpers +std::optional KnapsackSolver(std::vector& 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(*res) : std::nullopt; +} + +std::optional SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change) +{ + auto res{SelectCoinsBnB(utxo_pool, selection_target, cost_of_change, MAX_STANDARD_TX_WEIGHT)}; + return res ? std::optional(*res) : std::nullopt; } /** Check if SelectionResult a is equivalent to SelectionResult b. @@ -128,13 +142,15 @@ static CAmount make_hard_case(int utxos, std::vector& utxo_pool) return target; } -inline std::vector& GroupCoins(const std::vector& available_coins) +inline std::vector& GroupCoins(const std::vector& available_coins, bool subtract_fee_outputs = false) { static std::vector static_groups; static_groups.clear(); for (auto& coin : available_coins) { static_groups.emplace_back(); - static_groups.back().Insert(std::make_shared(coin), /*ancestors=*/ 0, /*descendants=*/ 0); + OutputGroup& group = static_groups.back(); + group.Insert(std::make_shared(coin), /*ancestors=*/ 0, /*descendants=*/ 0); + group.m_subtract_fee_outputs = subtract_fee_outputs; } return static_groups; } @@ -158,6 +174,16 @@ inline std::vector& KnapsackGroupOutputs(const CoinsResult& availab return static_groups.all_groups.mixed_group; } +static std::unique_ptr NewWallet(const node::NodeContext& m_node, const std::string& wallet_name = "") +{ + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), wallet_name, CreateMockWalletDatabase()); + BOOST_CHECK(wallet->LoadWallet() == DBErrors::LOAD_OK); + LOCK(wallet->cs_wallet); + wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); + wallet->SetupDescriptorScriptPubKeyMans(); + return wallet; +} + // Branch and bound coin selection tests BOOST_AUTO_TEST_CASE(bnb_search_test) { @@ -294,11 +320,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) coin_selection_params_bnb.m_subtract_fee_outputs = true; { - 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(); + std::unique_ptr wallet = NewWallet(m_node); CoinsResult available_coins; @@ -316,11 +338,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) } { - 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(); + std::unique_ptr wallet = NewWallet(m_node); CoinsResult available_coins; @@ -335,15 +353,13 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) selected_input.Insert(select_coin, coin_selection_params_bnb.m_subtract_fee_outputs); available_coins.Erase({available_coins.coins[OutputType::BECH32].begin()->outpoint}); coin_selection_params_bnb.m_effective_feerate = CFeeRate(0); + LOCK(wallet->cs_wallet); const auto result10 = SelectCoins(*wallet, available_coins, selected_input, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(result10); } { - 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(); + std::unique_ptr wallet = NewWallet(m_node); + LOCK(wallet->cs_wallet); // Every 'SelectCoins' call requires it CoinsResult available_coins; @@ -397,6 +413,37 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) const auto result13 = SelectCoins(*wallet, available_coins, selected_input, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(EquivalentResult(expected_result, *result13)); } + + { + // Test bnb max weight exceeded + // Inputs set [10, 9, 8, 5, 3, 1], Selection Target = 16 and coin 5 exceeding the max weight. + + std::unique_ptr wallet = NewWallet(m_node); + + CoinsResult available_coins; + add_coin(available_coins, *wallet, 10 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 9 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 8 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 5 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true, /*custom_size=*/MAX_STANDARD_TX_WEIGHT); + add_coin(available_coins, *wallet, 3 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 1 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + + CAmount selection_target = 16 * CENT; + const auto& no_res = SelectCoinsBnB(GroupCoins(available_coins.All(), /*subtract_fee_outputs*/true), + selection_target, /*cost_of_change=*/0, MAX_STANDARD_TX_WEIGHT); + BOOST_ASSERT(!no_res); + BOOST_CHECK(util::ErrorString(no_res).original.find("The inputs size exceeds the maximum weight") != std::string::npos); + + // Now add same coin value with a good size and check that it gets selected + add_coin(available_coins, *wallet, 5 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + const auto& res = SelectCoinsBnB(GroupCoins(available_coins.All(), /*subtract_fee_outputs*/true), selection_target, /*cost_of_change=*/0); + + expected_result.Clear(); + add_coin(8 * CENT, 2, expected_result); + add_coin(5 * CENT, 2, expected_result); + add_coin(3 * CENT, 2, expected_result); + BOOST_CHECK(EquivalentResult(expected_result, *res)); + } } BOOST_AUTO_TEST_CASE(knapsack_solver_test) @@ -404,11 +451,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) FastRandomContext rand{}; const auto temp1{[&rand](std::vector& g, const CAmount& v, CAmount c) { return KnapsackSolver(g, v, c, rand); }}; const auto KnapsackSolver{temp1}; - 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(); + std::unique_ptr wallet = NewWallet(m_node); CoinsResult available_coins; @@ -714,11 +757,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) BOOST_AUTO_TEST_CASE(ApproximateBestSubset) { FastRandomContext rand{}; - 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(); + std::unique_ptr wallet = NewWallet(m_node); CoinsResult available_coins; @@ -736,11 +775,8 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) // Tests that with the ideal conditions, the coin selector will always be able to find a solution that can pay the target value BOOST_AUTO_TEST_CASE(SelectCoins_test) { - 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(); + std::unique_ptr wallet = NewWallet(m_node); + LOCK(wallet->cs_wallet); // Every 'SelectCoins' call requires it // Random generator stuff std::default_random_engine generator; @@ -921,16 +957,101 @@ BOOST_AUTO_TEST_CASE(effective_value_test) BOOST_CHECK_EQUAL(output5.GetEffectiveValue(), nValue); // The effective value should be equal to the absolute value if input_bytes is -1 } -static util::Result select_coins(const CAmount& target, const CoinSelectionParams& cs_params, const CCoinControl& cc, std::function coin_setup, interfaces::Chain* chain) -{ - std::unique_ptr wallet = std::make_unique(chain, "", CreateMockWalletDatabase()); - wallet->LoadWallet(); - LOCK(wallet->cs_wallet); - wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); - wallet->SetupDescriptorScriptPubKeyMans(); +static util::Result SelectCoinsSRD(const CAmount& target, + const CoinSelectionParams& cs_params, + const node::NodeContext& m_node, + int max_weight, + std::function coin_setup) +{ + 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); +} + +BOOST_AUTO_TEST_CASE(srd_tests) +{ + // Test SRD: + // 1) Insufficient funds, select all provided coins and fail. + // 2) Exceeded max weight, coin selection always surpasses the max allowed weight. + // 3) Select coins without surpassing the max weight (some coins surpasses the max allowed weight, some others not) + + FastRandomContext rand; + CoinSelectionParams dummy_params{ // Only used to provide the 'avoid_partial' flag. + rand, + /*change_output_size=*/34, + /*change_spend_size=*/68, + /*min_change_target=*/CENT, + /*effective_feerate=*/CFeeRate(0), + /*long_term_feerate=*/CFeeRate(0), + /*discard_feerate=*/CFeeRate(0), + /*tx_noinputs_size=*/10 + 34, // static header size + output size + /*avoid_partial=*/false, + }; + + { + // ######################################################### + // 1) Insufficient funds, select all provided coins and fail + // ######################################################### + CAmount target = 49.5L * COIN; + int max_weight = 10000; // high enough to not fail for this reason. + const auto& res = SelectCoinsSRD(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + CoinsResult available_coins; + for (int j = 0; j < 10; ++j) { + add_coin(available_coins, wallet, CAmount(1 * COIN)); + add_coin(available_coins, wallet, CAmount(2 * COIN)); + } + return available_coins; + }); + BOOST_CHECK(!res); + BOOST_CHECK(util::ErrorString(res).empty()); // empty means "insufficient funds" + } + + { + // ########################### + // 2) Test max weight exceeded + // ########################### + CAmount target = 49.5L * COIN; + int max_weight = 3000; + const auto& res = SelectCoinsSRD(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + CoinsResult available_coins; + for (int j = 0; j < 10; ++j) { + add_coin(available_coins, wallet, CAmount(1 * COIN), CFeeRate(0), 144, false, 0, true); + add_coin(available_coins, wallet, CAmount(2 * COIN), CFeeRate(0), 144, false, 0, true); + } + return available_coins; + }); + BOOST_CHECK(!res); + BOOST_CHECK(util::ErrorString(res).original.find("The inputs size exceeds the maximum weight") != std::string::npos); + } + + { + // ################################################################################################################ + // 3) Test selection when some coins surpass the max allowed weight while others not. --> must find a good solution + // ################################################################################################################ + CAmount target = 25.33L * COIN; + int max_weight = 10000; // WU + const auto& res = SelectCoinsSRD(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + CoinsResult available_coins; + for (int j = 0; j < 60; ++j) { // 60 UTXO --> 19,8 BTC total --> 60 × 272 WU = 16320 WU + add_coin(available_coins, wallet, CAmount(0.33 * COIN), CFeeRate(0), 144, false, 0, true); + } + for (int i = 0; i < 10; i++) { // 10 UTXO --> 20 BTC total --> 10 × 272 WU = 2720 WU + add_coin(available_coins, wallet, CAmount(2 * COIN), CFeeRate(0), 144, false, 0, true); + } + return available_coins; + }); + BOOST_CHECK(res); + } +} + +static util::Result select_coins(const CAmount& target, const CoinSelectionParams& cs_params, const CCoinControl& cc, std::function coin_setup, const node::NodeContext& m_node) +{ + std::unique_ptr wallet = NewWallet(m_node); auto available_coins = coin_setup(*wallet); + LOCK(wallet->cs_wallet); auto result = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/ {}, target, cc, cs_params); if (result) { const auto signedTxSize = 10 + 34 + 68 * result->GetInputSet().size(); // static header size + output size + inputs size (P2WPKH) @@ -964,8 +1085,6 @@ BOOST_AUTO_TEST_CASE(check_max_weight) /*avoid_partial=*/false, }; - auto chain{m_node.chain.get()}; - { // Scenario 1: // The actor starts with 1x 50.0 BTC and 1515x 0.033 BTC (~100.0 BTC total) unspent outputs @@ -984,10 +1103,13 @@ BOOST_AUTO_TEST_CASE(check_max_weight) add_coin(available_coins, wallet, CAmount(50 * COIN), CFeeRate(0), 144, false, 0, true); return available_coins; }, - chain); + m_node); 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); } { @@ -1009,7 +1131,7 @@ BOOST_AUTO_TEST_CASE(check_max_weight) } return available_coins; }, - chain); + m_node); BOOST_CHECK(has_coin(result->GetInputSet(), CAmount(0.0625 * COIN))); BOOST_CHECK(has_coin(result->GetInputSet(), CAmount(0.025 * COIN))); @@ -1030,7 +1152,7 @@ BOOST_AUTO_TEST_CASE(check_max_weight) } return available_coins; }, - chain); + m_node); // No results // 1515 inputs * 68 bytes = 103,020 bytes @@ -1045,20 +1167,11 @@ BOOST_AUTO_TEST_CASE(SelectCoins_effective_value_test) // This test creates a coin whose value is higher than the target but whose effective value is lower than the target. // The coin is selected using coin control, with m_allow_other_inputs = false. SelectCoins should fail due to insufficient funds. - 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(); + std::unique_ptr wallet = NewWallet(m_node); CoinsResult available_coins; { - std::unique_ptr dummyWallet = std::make_unique(m_node.chain.get(), "dummy", CreateMockWalletDatabase()); - dummyWallet->LoadWallet(); - LOCK(dummyWallet->cs_wallet); - dummyWallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); - dummyWallet->SetupDescriptorScriptPubKeyMans(); - + std::unique_ptr dummyWallet = NewWallet(m_node, /*wallet_name=*/"dummy"); add_coin(available_coins, *dummyWallet, 100000); // 0.001 BTC } @@ -1082,6 +1195,7 @@ BOOST_AUTO_TEST_CASE(SelectCoins_effective_value_test) cc.SetInputWeight(output.outpoint, 148); cc.SelectExternal(output.outpoint, output.txout); + LOCK(wallet->cs_wallet); const auto preset_inputs = *Assert(FetchSelectedInputs(*wallet, cc, cs_params)); available_coins.Erase({available_coins.coins[OutputType::BECH32].begin()->outpoint}); @@ -1094,11 +1208,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_coinsresult_test, BasicTestingSetup) // Test case to verify CoinsResult object sanity. CoinsResult available_coins; { - std::unique_ptr dummyWallet = std::make_unique(m_node.chain.get(), "dummy", CreateMockWalletDatabase()); - BOOST_CHECK_EQUAL(dummyWallet->LoadWallet(), DBErrors::LOAD_OK); - LOCK(dummyWallet->cs_wallet); - dummyWallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); - dummyWallet->SetupDescriptorScriptPubKeyMans(); + std::unique_ptr dummyWallet = NewWallet(m_node, /*wallet_name=*/"dummy"); // Add some coins to 'available_coins' for (int i=0; i<10; i++) { diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 304190eec1..9be8efab62 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 +#include #include #include #include @@ -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& res) { return !util::ErrorString(res).empty(); } + FUZZ_TARGET(coinselection) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; @@ -84,18 +88,18 @@ FUZZ_TARGET(coinselection) GroupCoins(fuzzed_data_provider, utxo_pool, coin_params, /*positive_only=*/false, group_all); // Run coinselection algorithms - const auto result_bnb = SelectCoinsBnB(group_pos, target, cost_of_change); + 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); + auto result_srd = SelectCoinsSRD(group_pos, target, 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)}; - 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); } }