From 7f61d31a5cec8fc61328bee43f90d3f1dcb0a035 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Thu, 29 Feb 2024 10:53:44 +0100 Subject: [PATCH 1/4] [refactor]: update coin selection algorithms input parameter `max_weight` name - This commit renames the coin selection algorithms input parameter `max_weight` to `max_selection_weight` for clarity. The parameter represent the maximum weight of the UTXOs the coin selection algorithm should select, not the transaction maximum weight. - The commit updates the parameter docstring to provide correct description. - Also updates coin selection unit and fuzzing test variables to match the new name. --- src/wallet/coinselection.cpp | 26 +++++++------- src/wallet/coinselection.h | 10 +++--- src/wallet/spend.cpp | 14 ++++---- src/wallet/test/coinselector_tests.cpp | 50 +++++++++++++------------- src/wallet/test/fuzz/coinselection.cpp | 10 +++--- 5 files changed, 55 insertions(+), 55 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index f1706b6800ca8..c37d42d22a4fb 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -84,14 +84,14 @@ struct { * bound of the range. * @param const CAmount& cost_of_change This is the cost of creating and spending a change output. * This plus selection_target is the upper bound of the range. - * @param int max_weight The maximum weight available for the input set. + * @param int max_selection_weight The maximum allowed weight for a selection result to be valid. * @returns The result of this coin selection algorithm, or std::nullopt */ static const size_t TOTAL_TRIES = 100000; util::Result SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, - int max_weight) + int max_selection_weight) { SelectionResult result(selection_target, SelectionAlgorithm::BNB); CAmount curr_value = 0; @@ -128,7 +128,7 @@ util::Result SelectCoinsBnB(std::vector& utxo_pool 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 + } else if (curr_selection_weight > max_selection_weight) { // Selected UTXOs weight exceeds the maximum weight allowed, 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 @@ -319,10 +319,10 @@ util::Result SelectCoinsBnB(std::vector& utxo_pool * group with multiple as a heavier UTXO with the combined amount here.) * @param const CAmount& selection_target This is the minimum amount that we need for the transaction without considering change. * @param const CAmount& change_target The minimum budget for creating a change output, by which we increase the selection_target. - * @param int max_weight The maximum permitted weight for the input set. + * @param int max_selection_weight The maximum allowed weight for a selection result to be valid. * @returns The result of this coin selection algorithm, or std::nullopt */ -util::Result CoinGrinder(std::vector& utxo_pool, const CAmount& selection_target, CAmount change_target, int max_weight) +util::Result CoinGrinder(std::vector& utxo_pool, const CAmount& selection_target, CAmount change_target, int max_selection_weight) { std::sort(utxo_pool.begin(), utxo_pool.end(), descending_effval_weight); // The sum of UTXO amounts after this UTXO index, e.g. lookahead[5] = Σ(UTXO[6+].amount) @@ -359,7 +359,7 @@ util::Result CoinGrinder(std::vector& utxo_pool, c // The weight of the currently selected input set, and the weight of the best selection int curr_weight = 0; - int best_selection_weight = max_weight; // Tie is fine, because we prefer lower selection amount + int best_selection_weight = max_selection_weight; // Tie is fine, because we prefer lower selection amount // Whether the input sets generated during this search have exceeded the maximum transaction weight at any point bool max_tx_weight_exceeded = false; @@ -436,8 +436,8 @@ util::Result CoinGrinder(std::vector& utxo_pool, c // Insufficient funds with lookahead: CUT should_cut = true; } else if (curr_weight > best_selection_weight) { - // best_selection_weight is initialized to max_weight - if (curr_weight > max_weight) max_tx_weight_exceeded = true; + // best_selection_weight is initialized to max_selection_weight + if (curr_weight > max_selection_weight) max_tx_weight_exceeded = true; // Worse weight than best solution. More UTXOs only increase weight: // CUT if last selected group had minimal weight, else SHIFT if (utxo_pool[curr_tail].m_weight <= min_tail_weight[curr_tail]) { @@ -535,7 +535,7 @@ class MinOutputGroupComparator }; util::Result SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, CAmount change_fee, FastRandomContext& rng, - int max_weight) + int max_selection_weight) { SelectionResult result(target_value, SelectionAlgorithm::SRD); std::priority_queue, MinOutputGroupComparator> heap; @@ -565,14 +565,14 @@ util::Result SelectCoinsSRD(const std::vector& utx // If the selection weight exceeds the maximum allowed size, remove the least valuable inputs until we // are below max weight. - if (weight > max_weight) { + if (weight > max_selection_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); + } while (!heap.empty() && weight > max_selection_weight); } // Now check if we are above the target @@ -648,7 +648,7 @@ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::v } util::Result KnapsackSolver(std::vector& groups, const CAmount& nTargetValue, - CAmount change_target, FastRandomContext& rng, int max_weight) + CAmount change_target, FastRandomContext& rng, int max_selection_weight) { SelectionResult result(nTargetValue, SelectionAlgorithm::KNAPSACK); @@ -709,7 +709,7 @@ util::Result KnapsackSolver(std::vector& groups, c } // If the result exceeds the maximum allowed size, return closest UTXO above the target - if (result.GetWeight() > max_weight) { + if (result.GetWeight() > max_selection_weight) { // No coin above target, nothing to do. if (!lowest_larger) return ErrorMaxWeightExceeded(); diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 9fb000422c7bc..6b0d1ab2a472c 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -440,9 +440,9 @@ struct SelectionResult }; util::Result SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, - int max_weight); + int max_selection_weight); -util::Result CoinGrinder(std::vector& utxo_pool, const CAmount& selection_target, CAmount change_target, int max_weight); +util::Result CoinGrinder(std::vector& utxo_pool, const CAmount& selection_target, CAmount change_target, int max_selection_weight); /** Select coins by Single Random Draw. OutputGroups are selected randomly from the eligible * outputs until the target is satisfied @@ -450,15 +450,15 @@ util::Result CoinGrinder(std::vector& utxo_pool, c * @param[in] utxo_pool The positive effective value OutputGroups eligible for selection * @param[in] target_value The target value to select for * @param[in] rng The randomness source to shuffle coins - * @param[in] max_weight The maximum allowed weight for a selection result to be valid + * @param[in] max_selection_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, CAmount change_fee, FastRandomContext& rng, - int max_weight); + int max_selection_weight); // Original coin selection algorithm as a fallback util::Result KnapsackSolver(std::vector& groups, const CAmount& nTargetValue, - CAmount change_target, FastRandomContext& rng, int max_weight); + CAmount change_target, FastRandomContext& rng, int max_selection_weight); } // namespace wallet #endif // BITCOIN_WALLET_COINSELECTION_H diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index c16b8a9d4f140..a39a4ca0d1ec0 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -695,26 +695,26 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co } }; - // Maximum allowed weight - int max_inputs_weight = MAX_STANDARD_TX_WEIGHT - (coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR); + // Maximum allowed weight for selected coins. + int max_selection_weight = MAX_STANDARD_TX_WEIGHT - (coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR); // SFFO frequently causes issues in the context of changeless input sets: skip BnB when SFFO is active if (!coin_selection_params.m_subtract_fee_outputs) { - if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_cost_of_change, max_inputs_weight)}) { + if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_cost_of_change, max_selection_weight)}) { results.push_back(*bnb_result); } else append_error(std::move(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); + max_selection_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, max_inputs_weight)}) { + if (auto knapsack_result{KnapsackSolver(groups.mixed_group, nTargetValue, coin_selection_params.m_min_change_target, coin_selection_params.rng_fast, max_selection_weight)}) { results.push_back(*knapsack_result); } else append_error(std::move(knapsack_result)); if (coin_selection_params.m_effective_feerate > CFeeRate{3 * coin_selection_params.m_long_term_feerate}) { // Minimize input set for feerates of at least 3×LTFRE (default: 30 ṩ/vB+) - if (auto cg_result{CoinGrinder(groups.positive_group, nTargetValue, coin_selection_params.m_min_change_target, max_inputs_weight)}) { + if (auto cg_result{CoinGrinder(groups.positive_group, nTargetValue, coin_selection_params.m_min_change_target, max_selection_weight)}) { cg_result->RecalculateWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); results.push_back(*cg_result); } else { @@ -722,7 +722,7 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co } } - if (auto srd_result{SelectCoinsSRD(groups.positive_group, nTargetValue, coin_selection_params.m_change_fee, 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_selection_weight)}) { results.push_back(*srd_result); } else append_error(std::move(srd_result)); diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 7bd92b471cd97..eb9c349c22ebb 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -1097,13 +1097,13 @@ BOOST_AUTO_TEST_CASE(effective_value_test) static util::Result CoinGrinder(const CAmount& target, const CoinSelectionParams& cs_params, const node::NodeContext& m_node, - int max_weight, + int max_selection_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 CoinGrinder(group.positive_group, target, cs_params.m_min_change_target, max_weight); + return CoinGrinder(group.positive_group, target, cs_params.m_min_change_target, max_selection_weight); } BOOST_AUTO_TEST_CASE(coin_grinder_tests) @@ -1135,8 +1135,8 @@ BOOST_AUTO_TEST_CASE(coin_grinder_tests) // 1) Insufficient funds, select all provided coins and fail // ######################################################### CAmount target = 49.5L * COIN; - int max_weight = 10'000; // high enough to not fail for this reason. - const auto& res = CoinGrinder(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + int max_selection_weight = 10'000; // high enough to not fail for this reason. + const auto& res = CoinGrinder(target, dummy_params, m_node, max_selection_weight, [&](CWallet& wallet) { CoinsResult available_coins; for (int j = 0; j < 10; ++j) { add_coin(available_coins, wallet, CAmount(1 * COIN)); @@ -1153,8 +1153,8 @@ BOOST_AUTO_TEST_CASE(coin_grinder_tests) // 2) Test max weight exceeded // ########################### CAmount target = 29.5L * COIN; - int max_weight = 3000; - const auto& res = CoinGrinder(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + int max_selection_weight = 3000; + const auto& res = CoinGrinder(target, dummy_params, m_node, max_selection_weight, [&](CWallet& wallet) { CoinsResult available_coins; for (int j = 0; j < 10; ++j) { add_coin(available_coins, wallet, CAmount(1 * COIN), CFeeRate(5000), 144, false, 0, true); @@ -1171,8 +1171,8 @@ BOOST_AUTO_TEST_CASE(coin_grinder_tests) // 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 = 10'000; // WU - const auto& res = CoinGrinder(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + int max_selection_weight = 10'000; // WU + const auto& res = CoinGrinder(target, dummy_params, m_node, max_selection_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(5000), 144, false, 0, true); @@ -1193,8 +1193,8 @@ BOOST_AUTO_TEST_CASE(coin_grinder_tests) // 4) Test that two less valuable UTXOs with a combined lower weight are preferred over a more valuable heavier UTXO // ################################################################################################################# CAmount target = 1.9L * COIN; - int max_weight = 400'000; // WU - const auto& res = CoinGrinder(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + int max_selection_weight = 400'000; // WU + const auto& res = CoinGrinder(target, dummy_params, m_node, max_selection_weight, [&](CWallet& wallet) { CoinsResult available_coins; add_coin(available_coins, wallet, CAmount(2 * COIN), CFeeRate(5000), 144, false, 0, true, 148); add_coin(available_coins, wallet, CAmount(1 * COIN), CFeeRate(5000), 144, false, 0, true, 68); @@ -1215,8 +1215,8 @@ BOOST_AUTO_TEST_CASE(coin_grinder_tests) // 5) Test finding a solution in a UTXO pool with mixed weights // ################################################################################################################ CAmount target = 30L * COIN; - int max_weight = 400'000; // WU - const auto& res = CoinGrinder(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + int max_selection_weight = 400'000; // WU + const auto& res = CoinGrinder(target, dummy_params, m_node, max_selection_weight, [&](CWallet& wallet) { CoinsResult available_coins; for (int j = 0; j < 5; ++j) { // Add heavy coins {3, 6, 9, 12, 15} @@ -1244,8 +1244,8 @@ BOOST_AUTO_TEST_CASE(coin_grinder_tests) // 6) Test that the lightest solution among many clones is found // ################################################################################################################# CAmount target = 9.9L * COIN; - int max_weight = 400'000; // WU - const auto& res = CoinGrinder(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + int max_selection_weight = 400'000; // WU + const auto& res = CoinGrinder(target, dummy_params, m_node, max_selection_weight, [&](CWallet& wallet) { CoinsResult available_coins; // Expected Result: 4 + 3 + 2 + 1 = 10 BTC at 400 vB add_coin(available_coins, wallet, CAmount(4 * COIN), CFeeRate(5000), 144, false, 0, true, 100); @@ -1283,8 +1283,8 @@ BOOST_AUTO_TEST_CASE(coin_grinder_tests) // 7) Test that lots of tiny UTXOs can be skipped if they are too heavy while there are enough funds in lookahead // ################################################################################################################# CAmount target = 1.9L * COIN; - int max_weight = 40000; // WU - const auto& res = CoinGrinder(target, dummy_params, m_node, max_weight, [&](CWallet& wallet) { + int max_selection_weight = 40000; // WU + const auto& res = CoinGrinder(target, dummy_params, m_node, max_selection_weight, [&](CWallet& wallet) { CoinsResult available_coins; add_coin(available_coins, wallet, CAmount(1.8 * COIN), CFeeRate(5000), 144, false, 0, true, 2500); add_coin(available_coins, wallet, CAmount(1 * COIN), CFeeRate(5000), 144, false, 0, true, 1000); @@ -1308,13 +1308,13 @@ BOOST_AUTO_TEST_CASE(coin_grinder_tests) static util::Result SelectCoinsSRD(const CAmount& target, const CoinSelectionParams& cs_params, const node::NodeContext& m_node, - int max_weight, + int max_selection_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.m_change_fee, cs_params.rng_fast, max_weight); + return SelectCoinsSRD(group.positive_group, target, cs_params.m_change_fee, cs_params.rng_fast, max_selection_weight); } BOOST_AUTO_TEST_CASE(srd_tests) @@ -1342,8 +1342,8 @@ BOOST_AUTO_TEST_CASE(srd_tests) // 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) { + int max_selection_weight = 10000; // high enough to not fail for this reason. + const auto& res = SelectCoinsSRD(target, dummy_params, m_node, max_selection_weight, [&](CWallet& wallet) { CoinsResult available_coins; for (int j = 0; j < 10; ++j) { add_coin(available_coins, wallet, CAmount(1 * COIN)); @@ -1360,8 +1360,8 @@ BOOST_AUTO_TEST_CASE(srd_tests) // 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) { + int max_selection_weight = 3000; + const auto& res = SelectCoinsSRD(target, dummy_params, m_node, max_selection_weight, [&](CWallet& wallet) { CoinsResult available_coins; for (int j = 0; j < 10; ++j) { /* 10 × 1 BTC + 10 × 2 BTC = 30 BTC. 20 × 272 WU = 5440 WU */ @@ -1379,8 +1379,8 @@ BOOST_AUTO_TEST_CASE(srd_tests) // 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) { + int max_selection_weight = 10000; // WU + const auto& res = SelectCoinsSRD(target, dummy_params, m_node, max_selection_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); @@ -1415,7 +1415,7 @@ static bool has_coin(const CoinSet& set, CAmount amount) return std::any_of(set.begin(), set.end(), [&](const auto& coin) { return coin->GetEffectiveValue() == amount; }); } -BOOST_AUTO_TEST_CASE(check_max_weight) +BOOST_AUTO_TEST_CASE(check_max_selection_weight) { const CAmount target = 49.5L * COIN; CCoinControl cc; diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 644a8dd7adf5d..72975c3d681e3 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -195,11 +195,11 @@ FUZZ_TARGET(coin_grinder_is_optimal) if (best_weight < std::numeric_limits::max()) { // Sufficient funds and acceptable weight: CoinGrinder should find at least one solution - int high_max_weight = fuzzed_data_provider.ConsumeIntegralInRange(best_weight, std::numeric_limits::max()); + int high_max_selection_weight = fuzzed_data_provider.ConsumeIntegralInRange(best_weight, std::numeric_limits::max()); - auto result_cg = CoinGrinder(group_pos, target, coin_params.m_min_change_target, high_max_weight); + auto result_cg = CoinGrinder(group_pos, target, coin_params.m_min_change_target, high_max_selection_weight); assert(result_cg); - assert(result_cg->GetWeight() <= high_max_weight); + assert(result_cg->GetWeight() <= high_max_selection_weight); assert(result_cg->GetSelectedEffectiveValue() >= target + coin_params.m_min_change_target); assert(best_weight < result_cg->GetWeight() || (best_weight == result_cg->GetWeight() && best_amount <= result_cg->GetSelectedEffectiveValue())); if (result_cg->GetAlgoCompleted()) { @@ -210,8 +210,8 @@ FUZZ_TARGET(coin_grinder_is_optimal) } // CoinGrinder cannot ever find a better solution than the brute-forced best, or there is none in the first place - int low_max_weight = fuzzed_data_provider.ConsumeIntegralInRange(0, best_weight - 1); - auto result_cg = CoinGrinder(group_pos, target, coin_params.m_min_change_target, low_max_weight); + int low_max_selection_weight = fuzzed_data_provider.ConsumeIntegralInRange(0, best_weight - 1); + auto result_cg = CoinGrinder(group_pos, target, coin_params.m_min_change_target, low_max_selection_weight); // Max_weight should have been exceeded, or there were insufficient funds assert(!result_cg); } From baab0d2d43049a71dc90176bc4d72062f7b2ce19 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Thu, 29 Feb 2024 10:59:50 +0100 Subject: [PATCH 2/4] [doc]: update reason for deducting change output weight `CoinGrinder` will also produce change output, listing all the Coin selection algorithms that produces change output is not maintainable, just infer that remaining algorithms all might produce change. --- src/wallet/spend.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index a39a4ca0d1ec0..4759f1eb956fd 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -705,7 +705,7 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co } else append_error(std::move(bnb_result)); } - // As Knapsack and SRD can create change, also deduce change weight. + // Deduct change weight because remaining Coin Selection algorithms can create change output max_selection_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. From b6fc5043c16c2467a2a6768a6ca9b18035fc400f Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Thu, 21 Mar 2024 17:05:51 +0100 Subject: [PATCH 3/4] [wallet]: update the data type of `change_output_size`, `change_spend_size` and `tx_noinputs_size` to `int` - This change ensures consistency in transaction size and weight calculation within the wallet and prevents conversion overflow when calculating `max_selection_weight`. --- src/wallet/coinselection.h | 10 +++++----- src/wallet/spend.cpp | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 6b0d1ab2a472c..1f234427d2c6c 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -139,9 +139,9 @@ struct CoinSelectionParams { /** Randomness to use in the context of coin selection. */ FastRandomContext& rng_fast; /** Size of a change output in bytes, determined by the output type. */ - size_t change_output_size = 0; + int change_output_size = 0; /** Size of the input to spend a change output in virtual bytes. */ - size_t change_spend_size = 0; + int change_spend_size = 0; /** Mininmum change to target in Knapsack solver and CoinGrinder: * select coins to cover the payment and at least this value of change. */ CAmount m_min_change_target{0}; @@ -162,7 +162,7 @@ struct CoinSelectionParams { CFeeRate m_discard_feerate; /** Size of the transaction before coin selection, consisting of the header and recipient * output(s), excluding the inputs and change output(s). */ - size_t tx_noinputs_size = 0; + int tx_noinputs_size = 0; /** Indicate that we are subtracting the fee from outputs */ bool m_subtract_fee_outputs = false; /** When true, always spend all (up to OUTPUT_GROUP_MAX_ENTRIES) or none of the outputs @@ -175,9 +175,9 @@ struct CoinSelectionParams { */ bool m_include_unsafe_inputs = false; - CoinSelectionParams(FastRandomContext& rng_fast, size_t change_output_size, size_t change_spend_size, + CoinSelectionParams(FastRandomContext& rng_fast, int change_output_size, int change_spend_size, CAmount min_change_target, CFeeRate effective_feerate, - CFeeRate long_term_feerate, CFeeRate discard_feerate, size_t tx_noinputs_size, bool avoid_partial) + CFeeRate long_term_feerate, CFeeRate discard_feerate, int tx_noinputs_size, bool avoid_partial) : rng_fast{rng_fast}, change_output_size(change_output_size), change_spend_size(change_spend_size), diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 4759f1eb956fd..e0546c6210a8f 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1059,7 +1059,7 @@ static util::Result CreateTransactionInternal( if (change_spend_size == -1) { coin_selection_params.change_spend_size = DUMMY_NESTED_P2WPKH_INPUT_SIZE; } else { - coin_selection_params.change_spend_size = (size_t)change_spend_size; + coin_selection_params.change_spend_size = change_spend_size; } // Set discard feerate From 734076c6de1781f957c8bc3bf7ed6951920cfcf6 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Thu, 29 Feb 2024 12:43:52 +0100 Subject: [PATCH 4/4] [wallet, rpc]: add `max_tx_weight` to tx funding options This allows a transaction's weight to be bound under a certain weight if possible and desired. This can be beneficial for future RBF attempts, or whenever a more restricted spend topology is desired. Co-authored-by: Greg Sanders --- src/rpc/client.cpp | 3 ++ src/wallet/coincontrol.h | 2 + src/wallet/coinselection.cpp | 31 ++++++++++---- src/wallet/coinselection.h | 8 +++- src/wallet/rpc/spend.cpp | 14 ++++++ src/wallet/spend.cpp | 21 +++++++-- src/wallet/test/fuzz/coinselection.cpp | 11 +++-- test/functional/rpc_psbt.py | 45 ++++++++++++++++++++ test/functional/test_framework/blocktools.py | 1 + 9 files changed, 119 insertions(+), 17 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 7dfe69a6c5021..b866fa484bf69 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -146,6 +146,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "fundrawtransaction", 1, "conf_target"}, { "fundrawtransaction", 1, "replaceable"}, { "fundrawtransaction", 1, "solving_data"}, + { "fundrawtransaction", 1, "max_tx_weight"}, { "fundrawtransaction", 2, "iswitness" }, { "walletcreatefundedpsbt", 0, "inputs" }, { "walletcreatefundedpsbt", 1, "outputs" }, @@ -164,6 +165,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "walletcreatefundedpsbt", 3, "conf_target"}, { "walletcreatefundedpsbt", 3, "replaceable"}, { "walletcreatefundedpsbt", 3, "solving_data"}, + { "walletcreatefundedpsbt", 3, "max_tx_weight"}, { "walletcreatefundedpsbt", 4, "bip32derivs" }, { "walletprocesspsbt", 1, "sign" }, { "walletprocesspsbt", 3, "bip32derivs" }, @@ -208,6 +210,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "send", 4, "conf_target"}, { "send", 4, "replaceable"}, { "send", 4, "solving_data"}, + { "send", 4, "max_tx_weight"}, { "sendall", 0, "recipients" }, { "sendall", 1, "conf_target" }, { "sendall", 3, "fee_rate"}, diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index b2f813383dc5a..d36314312ad1f 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -115,6 +115,8 @@ class CCoinControl std::optional m_locktime; //! Version std::optional m_version; + //! Caps weight of resulting tx + std::optional m_max_tx_weight{std::nullopt}; CCoinControl(); diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index c37d42d22a4fb..a54d1a46685f1 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -597,11 +597,12 @@ util::Result SelectCoinsSRD(const std::vector& utx * nTargetValue, with indices corresponding to groups. If the ith * entry is true, that means the ith group in groups was selected. * param@[out] nBest Total amount of subset chosen that is closest to nTargetValue. + * paramp[in] max_selection_weight The maximum allowed weight for a selection result to be valid. * param@[in] iterations Maximum number of tries. */ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::vector& groups, const CAmount& nTotalLower, const CAmount& nTargetValue, - std::vector& vfBest, CAmount& nBest, int iterations = 1000) + std::vector& vfBest, CAmount& nBest, int max_selection_weight, int iterations = 1000) { std::vector vfIncluded; @@ -613,6 +614,7 @@ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::v { vfIncluded.assign(groups.size(), false); CAmount nTotal = 0; + int selected_coins_weight{0}; bool fReachedTarget = false; for (int nPass = 0; nPass < 2 && !fReachedTarget; nPass++) { @@ -627,9 +629,9 @@ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::v if (nPass == 0 ? insecure_rand.randbool() : !vfIncluded[i]) { nTotal += groups[i].GetSelectionAmount(); + selected_coins_weight += groups[i].m_weight; vfIncluded[i] = true; - if (nTotal >= nTargetValue) - { + if (nTotal >= nTargetValue && selected_coins_weight <= max_selection_weight) { fReachedTarget = true; // If the total is between nTargetValue and nBest, it's our new best // approximation. @@ -639,6 +641,7 @@ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::v vfBest = vfIncluded; } nTotal -= groups[i].GetSelectionAmount(); + selected_coins_weight -= groups[i].m_weight; vfIncluded[i] = false; } } @@ -652,6 +655,7 @@ util::Result KnapsackSolver(std::vector& groups, c { SelectionResult result(nTargetValue, SelectionAlgorithm::KNAPSACK); + bool max_weight_exceeded{false}; // List of values less than target std::optional lowest_larger; // Groups with selection amount smaller than the target and any change we might produce. @@ -662,6 +666,10 @@ util::Result KnapsackSolver(std::vector& groups, c Shuffle(groups.begin(), groups.end(), rng); for (const OutputGroup& group : groups) { + if (group.m_weight > max_selection_weight) { + max_weight_exceeded = true; + continue; + } if (group.GetSelectionAmount() == nTargetValue) { result.AddInput(group); return result; @@ -677,11 +685,18 @@ util::Result KnapsackSolver(std::vector& groups, c for (const auto& group : applicable_groups) { result.AddInput(group); } - return result; + if (result.GetWeight() <= max_selection_weight) return result; + else max_weight_exceeded = true; + + // Try something else + result.Clear(); } if (nTotalLower < nTargetValue) { - if (!lowest_larger) return util::Error(); + if (!lowest_larger) { + if (max_weight_exceeded) return ErrorMaxWeightExceeded(); + return util::Error(); + } result.AddInput(*lowest_larger); return result; } @@ -691,9 +706,9 @@ util::Result KnapsackSolver(std::vector& groups, c std::vector vfBest; CAmount nBest; - ApproximateBestSubset(rng, applicable_groups, nTotalLower, nTargetValue, vfBest, nBest); + ApproximateBestSubset(rng, applicable_groups, nTotalLower, nTargetValue, vfBest, nBest, max_selection_weight); if (nBest != nTargetValue && nTotalLower >= nTargetValue + change_target) { - ApproximateBestSubset(rng, applicable_groups, nTotalLower, nTargetValue + change_target, vfBest, nBest); + ApproximateBestSubset(rng, applicable_groups, nTotalLower, nTargetValue + change_target, vfBest, nBest, max_selection_weight); } // If we have a bigger coin and (either the stochastic approximation didn't find a good solution, @@ -728,7 +743,7 @@ util::Result KnapsackSolver(std::vector& groups, c LogPrint(BCLog::SELECTCOINS, "%stotal %s\n", log_message, FormatMoney(nBest)); } } - + Assume(result.GetWeight() <= max_selection_weight); return result; } diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 1f234427d2c6c..8a81cfc268213 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -174,10 +174,13 @@ struct CoinSelectionParams { * 1) Received from other wallets, 2) replacing other txs, 3) that have been replaced. */ bool m_include_unsafe_inputs = false; + /** The maximum weight for this transaction. */ + std::optional m_max_tx_weight{std::nullopt}; CoinSelectionParams(FastRandomContext& rng_fast, int change_output_size, int change_spend_size, CAmount min_change_target, CFeeRate effective_feerate, - CFeeRate long_term_feerate, CFeeRate discard_feerate, int tx_noinputs_size, bool avoid_partial) + CFeeRate long_term_feerate, CFeeRate discard_feerate, int tx_noinputs_size, bool avoid_partial, + std::optional max_tx_weight = std::nullopt) : rng_fast{rng_fast}, change_output_size(change_output_size), change_spend_size(change_spend_size), @@ -186,7 +189,8 @@ struct CoinSelectionParams { m_long_term_feerate(long_term_feerate), m_discard_feerate(discard_feerate), tx_noinputs_size(tx_noinputs_size), - m_avoid_partial_spends(avoid_partial) + m_avoid_partial_spends(avoid_partial), + m_max_tx_weight(max_tx_weight) { } CoinSelectionParams(FastRandomContext& rng_fast) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index ac2a4826f05d9..8ea51e65c7897 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -542,6 +542,7 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact {"minconf", UniValueType(UniValue::VNUM)}, {"maxconf", UniValueType(UniValue::VNUM)}, {"input_weights", UniValueType(UniValue::VARR)}, + {"max_tx_weight", UniValueType(UniValue::VNUM)}, }, true, true); @@ -701,6 +702,10 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact } } + if (options.exists("max_tx_weight")) { + coinControl.m_max_tx_weight = options["max_tx_weight"].getInt(); + } + if (recipients.empty()) throw JSONRPCError(RPC_INVALID_PARAMETER, "TX must have at least one output"); @@ -786,6 +791,8 @@ RPCHelpMan fundrawtransaction() }, }, }, + {"max_tx_weight", RPCArg::Type::NUM, RPCArg::Default{MAX_STANDARD_TX_WEIGHT}, "The maximum acceptable transaction weight.\n" + "Transaction building will fail if this can not be satisfied."}, }, FundTxDoc()), RPCArgOptions{ @@ -1240,6 +1247,8 @@ RPCHelpMan send() {"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."}, }, }, + {"max_tx_weight", RPCArg::Type::NUM, RPCArg::Default{MAX_STANDARD_TX_WEIGHT}, "The maximum acceptable transaction weight.\n" + "Transaction building will fail if this can not be satisfied."}, }, FundTxDoc()), RPCArgOptions{.oneline_description="options"}}, @@ -1287,6 +1296,9 @@ RPCHelpMan send() // Automatically select coins, unless at least one is manually selected. Can // be overridden by options.add_inputs. coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; + if (options.exists("max_tx_weight")) { + coin_control.m_max_tx_weight = options["max_tx_weight"].getInt(); + } SetOptionsInputWeights(options["inputs"], options); // Clear tx.vout since it is not meant to be used now that we are passing outputs directly. // This sets us up for a future PR to completely remove tx from the function signature in favor of passing inputs directly @@ -1697,6 +1709,8 @@ RPCHelpMan walletcreatefundedpsbt() {"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."}, }, }, + {"max_tx_weight", RPCArg::Type::NUM, RPCArg::Default{MAX_STANDARD_TX_WEIGHT}, "The maximum acceptable transaction weight.\n" + "Transaction building will fail if this can not be satisfied."}, }, FundTxDoc()), RPCArgOptions{.oneline_description="options"}}, diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index e0546c6210a8f..1ad570442e0f5 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -696,7 +696,12 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co }; // Maximum allowed weight for selected coins. - int max_selection_weight = MAX_STANDARD_TX_WEIGHT - (coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR); + int max_transaction_weight = coin_selection_params.m_max_tx_weight.value_or(MAX_STANDARD_TX_WEIGHT); + int tx_weight_no_input = coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR; + int max_selection_weight = max_transaction_weight - tx_weight_no_input; + if (max_selection_weight <= 0) { + return util::Error{_("Maximum transaction weight is less than transaction weight without inputs")}; + } // SFFO frequently causes issues in the context of changeless input sets: skip BnB when SFFO is active if (!coin_selection_params.m_subtract_fee_outputs) { @@ -706,7 +711,11 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co } // Deduct change weight because remaining Coin Selection algorithms can create change output - max_selection_weight -= (coin_selection_params.change_output_size * WITNESS_SCALE_FACTOR); + int change_outputs_weight = coin_selection_params.change_output_size * WITNESS_SCALE_FACTOR; + max_selection_weight -= change_outputs_weight; + if (max_selection_weight < 0 && results.empty()) { + return util::Error{_("Maximum transaction weight is too low, can not accommodate change output")}; + } // 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, max_selection_weight)}) { @@ -801,7 +810,7 @@ util::Result SelectCoins(const CWallet& wallet, CoinsResult& av coin_selection_params.m_change_fee); // Verify we haven't exceeded the maximum allowed weight - int max_inputs_weight = MAX_STANDARD_TX_WEIGHT - (coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR); + int max_inputs_weight = coin_selection_params.m_max_tx_weight.value_or(MAX_STANDARD_TX_WEIGHT) - (coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR); if (op_selection_result->GetWeight() > max_inputs_weight) { return util::Error{_("The combination of the pre-selected inputs and the wallet automatic inputs selection exceeds the transaction maximum weight. " "Please try sending a smaller amount or manually consolidating your wallet's UTXOs")}; @@ -1002,7 +1011,11 @@ static util::Result CreateTransactionInternal( CoinSelectionParams coin_selection_params{rng_fast}; // Parameters for coin selection, init with dummy coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends; coin_selection_params.m_include_unsafe_inputs = coin_control.m_include_unsafe_inputs; - + coin_selection_params.m_max_tx_weight = coin_control.m_max_tx_weight.value_or(MAX_STANDARD_TX_WEIGHT); + int minimum_tx_weight = MIN_STANDARD_TX_NONWITNESS_SIZE * WITNESS_SCALE_FACTOR; + if (coin_selection_params.m_max_tx_weight.value() < minimum_tx_weight || coin_selection_params.m_max_tx_weight.value() > MAX_STANDARD_TX_WEIGHT) { + return util::Error{strprintf(_("Maximum transaction weight must be between %d and %d"), minimum_tx_weight, MAX_STANDARD_TX_WEIGHT)}; + } // Set the long term feerate estimate to the wallet's consolidate feerate coin_selection_params.m_long_term_feerate = wallet.m_consolidate_feerate; diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 72975c3d681e3..209c87fd422bd 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -256,29 +256,34 @@ FUZZ_TARGET(coinselection) (void)group.EligibleForSpending(filter); } + int max_selection_weight = fuzzed_data_provider.ConsumeIntegralInRange(0, std::numeric_limits::max()); + // Run coinselection algorithms auto result_bnb = coin_params.m_subtract_fee_outputs ? util::Error{Untranslated("BnB disabled when SFFO is enabled")} : - SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, MAX_STANDARD_TX_WEIGHT); + SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, max_selection_weight); if (result_bnb) { assert(result_bnb->GetChange(coin_params.min_viable_change, coin_params.m_change_fee) == 0); assert(result_bnb->GetSelectedValue() >= target); + assert(result_bnb->GetWeight() <= max_selection_weight); (void)result_bnb->GetShuffledInputVector(); (void)result_bnb->GetInputSet(); } - auto result_srd = SelectCoinsSRD(group_pos, target, coin_params.m_change_fee, fast_random_context, MAX_STANDARD_TX_WEIGHT); + auto result_srd = SelectCoinsSRD(group_pos, target, coin_params.m_change_fee, fast_random_context, max_selection_weight); if (result_srd) { assert(result_srd->GetSelectedValue() >= target); assert(result_srd->GetChange(CHANGE_LOWER, coin_params.m_change_fee) > 0); // Demonstrate that SRD creates change of at least CHANGE_LOWER + assert(result_srd->GetWeight() <= max_selection_weight); result_srd->RecalculateWaste(coin_params.min_viable_change, coin_params.m_cost_of_change, coin_params.m_change_fee); (void)result_srd->GetShuffledInputVector(); (void)result_srd->GetInputSet(); } 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); + auto result_knapsack = KnapsackSolver(group_all, target, change_target, fast_random_context, max_selection_weight); if (result_knapsack) { assert(result_knapsack->GetSelectedValue() >= target); + assert(result_knapsack->GetWeight() <= max_selection_weight); result_knapsack->RecalculateWaste(coin_params.min_viable_change, coin_params.m_cost_of_change, coin_params.m_change_fee); (void)result_knapsack->GetShuffledInputVector(); (void)result_knapsack->GetInputSet(); diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 6ee7e56886816..111ca63618256 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -8,6 +8,9 @@ from itertools import product from random import randbytes +from test_framework.blocktools import ( + MAX_STANDARD_TX_WEIGHT, +) from test_framework.descriptors import descsum_create from test_framework.key import H_POINT from test_framework.messages import ( @@ -16,6 +19,7 @@ CTxIn, CTxOut, MAX_BIP125_RBF_SEQUENCE, + WITNESS_SCALE_FACTOR, ) from test_framework.psbt import ( PSBT, @@ -30,6 +34,7 @@ PSBT_OUT_TAP_TREE, ) from test_framework.script import CScript, OP_TRUE +from test_framework.script_util import MIN_STANDARD_TX_NONWITNESS_SIZE from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_approx, @@ -186,6 +191,46 @@ def run_test(self): # Create and fund a raw tx for sending 10 BTC psbtx1 = self.nodes[0].walletcreatefundedpsbt([], {self.nodes[2].getnewaddress():10})['psbt'] + self.log.info("Test for invalid maximum transaction weights") + dest_arg = [{self.nodes[0].getnewaddress(): 1}] + min_tx_weight = MIN_STANDARD_TX_NONWITNESS_SIZE * WITNESS_SCALE_FACTOR + assert_raises_rpc_error(-4, f"Maximum transaction weight must be between {min_tx_weight} and {MAX_STANDARD_TX_WEIGHT}", self.nodes[0].walletcreatefundedpsbt, [], dest_arg, 0, {"max_tx_weight": -1}) + assert_raises_rpc_error(-4, f"Maximum transaction weight must be between {min_tx_weight} and {MAX_STANDARD_TX_WEIGHT}", self.nodes[0].walletcreatefundedpsbt, [], dest_arg, 0, {"max_tx_weight": 0}) + assert_raises_rpc_error(-4, f"Maximum transaction weight must be between {min_tx_weight} and {MAX_STANDARD_TX_WEIGHT}", self.nodes[0].walletcreatefundedpsbt, [], dest_arg, 0, {"max_tx_weight": MAX_STANDARD_TX_WEIGHT + 1}) + + # Base transaction vsize: version (4) + locktime (4) + input count (1) + witness overhead (1) = 10 vbytes + base_tx_vsize = 10 + # One P2WPKH output vsize: outpoint (31 vbytes) + p2wpkh_output_vsize = 31 + # 1 vbyte for output count + output_count = 1 + tx_weight_without_inputs = (base_tx_vsize + output_count + p2wpkh_output_vsize) * WITNESS_SCALE_FACTOR + # min_tx_weight is greater than transaction weight without inputs + assert_greater_than(min_tx_weight, tx_weight_without_inputs) + + # In order to test for when the passed max weight is less than the transaction weight without inputs + # Define destination with two outputs. + dest_arg_large = [{self.nodes[0].getnewaddress(): 1}, {self.nodes[0].getnewaddress(): 1}] + large_tx_vsize_without_inputs = base_tx_vsize + output_count + (p2wpkh_output_vsize * 2) + large_tx_weight_without_inputs = large_tx_vsize_without_inputs * WITNESS_SCALE_FACTOR + assert_greater_than(large_tx_weight_without_inputs, min_tx_weight) + # Test for max_tx_weight less than Transaction weight without inputs + assert_raises_rpc_error(-4, "Maximum transaction weight is less than transaction weight without inputs", self.nodes[0].walletcreatefundedpsbt, [], dest_arg_large, 0, {"max_tx_weight": min_tx_weight}) + assert_raises_rpc_error(-4, "Maximum transaction weight is less than transaction weight without inputs", self.nodes[0].walletcreatefundedpsbt, [], dest_arg_large, 0, {"max_tx_weight": large_tx_weight_without_inputs}) + + # Test for max_tx_weight just enough to include inputs but not change output + assert_raises_rpc_error(-4, "Maximum transaction weight is too low, can not accommodate change output", self.nodes[0].walletcreatefundedpsbt, [], dest_arg_large, 0, {"max_tx_weight": (large_tx_vsize_without_inputs + 1) * WITNESS_SCALE_FACTOR}) + self.log.info("Test that a funded PSBT is always faithful to max_tx_weight option") + large_tx_vsize_with_change = large_tx_vsize_without_inputs + p2wpkh_output_vsize + # It's enough but won't accommodate selected input size + assert_raises_rpc_error(-4, "The inputs size exceeds the maximum weight", self.nodes[0].walletcreatefundedpsbt, [], dest_arg_large, 0, {"max_tx_weight": (large_tx_vsize_with_change) * WITNESS_SCALE_FACTOR}) + + max_tx_weight_sufficient = 1000 # 1k vbytes is enough + psbt = self.nodes[0].walletcreatefundedpsbt(outputs=dest_arg,locktime=0, options={"max_tx_weight": max_tx_weight_sufficient})["psbt"] + weight = self.nodes[0].decodepsbt(psbt)["tx"]["weight"] + # ensure the transaction's weight is below the specified max_tx_weight. + assert_greater_than_or_equal(max_tx_weight_sufficient, weight) + # If inputs are specified, do not automatically add more: utxo1 = self.nodes[0].listunspent()[0] assert_raises_rpc_error(-4, "The preselected coins total amount does not cover the transaction target. " diff --git a/test/functional/test_framework/blocktools.py b/test/functional/test_framework/blocktools.py index f0dc866f692a8..338b7fa3b00c9 100644 --- a/test/functional/test_framework/blocktools.py +++ b/test/functional/test_framework/blocktools.py @@ -48,6 +48,7 @@ MAX_BLOCK_SIGOPS = 20000 MAX_BLOCK_SIGOPS_WEIGHT = MAX_BLOCK_SIGOPS * WITNESS_SCALE_FACTOR +MAX_STANDARD_TX_WEIGHT = 400000 # Genesis block time (regtest) TIME_GENESIS_BLOCK = 1296688602