From 324281f686e52f41a6bf9dbf9230bed72526312c Mon Sep 17 00:00:00 2001 From: Richard Myers Date: Fri, 26 Apr 2024 23:23:45 +0200 Subject: [PATCH 1/3] wallet: Add coin control option "add_excess_to_recipient_position" If this coin selection RPC option is set, then the excess value for a changeless BnB tx will be added to the selected recipient output position instead of added to fees. The BnB algorithm with this option set will primarily select the input set with the least fee waste and excess value less than the cost of adding change. When comparing input sets with the same fee waste, the input set with the least excess value will be selected. --- src/bench/coin_selection.cpp | 2 +- src/rpc/client.cpp | 3 ++ src/wallet/coincontrol.h | 2 + src/wallet/coinselection.cpp | 34 +++++++++++--- src/wallet/coinselection.h | 11 ++++- src/wallet/rpc/spend.cpp | 14 ++++++ src/wallet/spend.cpp | 13 +++++- src/wallet/test/coinselector_tests.cpp | 61 ++++++++++++++++++++++++-- src/wallet/test/fuzz/coinselection.cpp | 2 +- 9 files changed, 128 insertions(+), 14 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index a752f73610b..6fdb99d8470 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -130,7 +130,7 @@ static void BnBExhaustion(benchmark::Bench& bench) bench.run([&] { // Benchmark CAmount target = make_hard_case(17, utxo_pool); - SelectCoinsBnB(utxo_pool, target, 0, MAX_STANDARD_TX_WEIGHT); // Should exhaust + SelectCoinsBnB(utxo_pool, target, 0, MAX_STANDARD_TX_WEIGHT, /*add_excess_to_target=*/false); // Should exhaust // Cleanup utxo_pool.clear(); diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index b866fa484bf..8ef996e93ea 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -147,6 +147,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "fundrawtransaction", 1, "replaceable"}, { "fundrawtransaction", 1, "solving_data"}, { "fundrawtransaction", 1, "max_tx_weight"}, + { "fundrawtransaction", 1, "add_excess_to_recipient_position"}, { "fundrawtransaction", 2, "iswitness" }, { "walletcreatefundedpsbt", 0, "inputs" }, { "walletcreatefundedpsbt", 1, "outputs" }, @@ -166,6 +167,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "walletcreatefundedpsbt", 3, "replaceable"}, { "walletcreatefundedpsbt", 3, "solving_data"}, { "walletcreatefundedpsbt", 3, "max_tx_weight"}, + { "walletcreatefundedpsbt", 3, "add_excess_to_recipient_position"}, { "walletcreatefundedpsbt", 4, "bip32derivs" }, { "walletprocesspsbt", 1, "sign" }, { "walletprocesspsbt", 3, "bip32derivs" }, @@ -211,6 +213,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "send", 4, "replaceable"}, { "send", 4, "solving_data"}, { "send", 4, "max_tx_weight"}, + { "send", 4, "add_excess_to_recipient_position"}, { "sendall", 0, "recipients" }, { "sendall", 1, "conf_target" }, { "sendall", 3, "fee_rate"}, diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index d36314312ad..39cf5f797a9 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -117,6 +117,8 @@ public: std::optional m_version; //! Caps weight of resulting tx std::optional m_max_tx_weight{std::nullopt}; + //! If set, add any excess from changeless spends to the specified recipient output index instead of to fees and do not count it as waste. + std::optional m_add_excess_to_recipient_position; CCoinControl(); diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index cee558088fb..4f242dfe3cb 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -85,13 +85,14 @@ struct { * @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_selection_weight The maximum allowed weight for a selection result to be valid. + * @param bool add_excess_to_target When true do not count excess as waste and add to the result target * @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_selection_weight) + int max_selection_weight, const bool add_excess_to_target) { SelectionResult result(selection_target, SelectionAlgorithm::BNB); CAmount curr_value = 0; @@ -116,6 +117,7 @@ util::Result SelectCoinsBnB(std::vector& utxo_pool CAmount curr_waste = 0; std::vector best_selection; CAmount best_waste = MAX_MONEY; + CAmount best_excess = MAX_MONEY; bool is_feerate_high = utxo_pool.at(0).fee > utxo_pool.at(0).long_term_fee; bool max_tx_weight_exceeded = false; @@ -132,16 +134,25 @@ util::Result SelectCoinsBnB(std::vector& utxo_pool 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. // However we are not going to explore that because this optimization for the waste is only done when we have hit our target // value. Adding any more UTXOs will be just burning the UTXO; it will go entirely to fees. Thus we aren't going to // explore any more UTXOs to avoid burning money like that. - if (curr_waste <= best_waste) { - best_selection = curr_selection; - best_waste = curr_waste; + CAmount curr_excess = (curr_value - selection_target); + if (add_excess_to_target) { + if (curr_waste < best_waste || (curr_waste == best_waste && curr_excess <= best_excess)) { + best_selection = curr_selection; + best_waste = curr_waste; + best_excess = curr_excess; + } } - curr_waste -= (curr_value - selection_target); // Remove the excess value as we will be selecting different coins now + else { + if (curr_waste + curr_excess <= best_waste) { + best_selection = curr_selection; + best_waste = curr_waste + curr_excess; + } + } + backtrack = true; } @@ -194,6 +205,10 @@ util::Result SelectCoinsBnB(std::vector& utxo_pool for (const size_t& i : best_selection) { result.AddInput(utxo_pool.at(i)); } + if (add_excess_to_target) { + auto excess = result.ResetTargetToSelectedValue(); + assert(best_excess == excess); + } result.RecalculateWaste(cost_of_change, cost_of_change, CAmount{0}); assert(best_waste == result.GetWaste()); @@ -853,6 +868,13 @@ void SelectionResult::RecalculateWaste(const CAmount min_viable_change, const CA m_waste = waste; } +CAmount SelectionResult::ResetTargetToSelectedValue() +{ + CAmount excess = (m_use_effective ? GetSelectedEffectiveValue(): GetSelectedValue()) - m_target; + m_target += excess; + return excess; +} + void SelectionResult::SetAlgoCompleted(bool algo_completed) { m_algo_completed = algo_completed; diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 08889c8e060..b7a5b6f2ab3 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -176,6 +176,12 @@ struct CoinSelectionParams { bool m_include_unsafe_inputs = false; /** The maximum weight for this transaction. */ std::optional m_max_tx_weight{std::nullopt}; + /** + * When set, excess value for changeless results will be added to the target amount at the given position + * and not counted as waste. Otherwise excess value will be be applied to fees and counted as waste. + */ + std::optional m_add_excess_to_recipient_position; + CoinSelectionParams(FastRandomContext& rng_fast, int change_output_size, int change_spend_size, CAmount min_change_target, CFeeRate effective_feerate, @@ -375,6 +381,9 @@ public: /** How much individual inputs overestimated the bump fees for shared ancestries */ void SetBumpFeeDiscount(const CAmount discount); + /** Reset target to the current selected amount */ + CAmount ResetTargetToSelectedValue(); + /** Calculates and stores the waste for this result given the cost of change * and the opportunity cost of spending these inputs now vs in the future. * If change exists, waste = change_cost + inputs * (effective_feerate - long_term_feerate) - bump_fee_group_discount @@ -444,7 +453,7 @@ public: }; util::Result SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, - int max_selection_weight); + int max_selection_weight, const bool add_excess_to_target); util::Result CoinGrinder(std::vector& utxo_pool, const CAmount& selection_target, CAmount change_target, int max_selection_weight); diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index bea9b2eec18..e9252313ec1 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -543,6 +543,7 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact {"maxconf", UniValueType(UniValue::VNUM)}, {"input_weights", UniValueType(UniValue::VARR)}, {"max_tx_weight", UniValueType(UniValue::VNUM)}, + {"add_excess_to_recipient_position", UniValue::VNUM}, }, true, true); @@ -625,6 +626,13 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact } } SetFeeEstimateMode(wallet, coinControl, options["conf_target"], options["estimate_mode"], options["fee_rate"], override_min_fee); + + if (options.exists("add_excess_to_recipient_position")) { + coinControl.m_add_excess_to_recipient_position = options["add_excess_to_recipient_position"].getInt(); + if (coinControl.m_add_excess_to_recipient_position.value() >= recipients.size()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Cannot add excess to the recipient output at index %d; the output does not exist.", coinControl.m_add_excess_to_recipient_position.value())); + } + } } } else { // if options is null and not a bool @@ -795,6 +803,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."}, + {"add_excess_to_recipient_position", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index where excess" + "fees are added. If not set, excess value from changeless transactions is added to fees"}, }, FundTxDoc()), RPCArgOptions{ @@ -1251,6 +1261,8 @@ RPCHelpMan send() }, {"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."}, + {"add_excess_to_recipient_position", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index where excess" + "fees are added. If not set, excess value from changeless transactions is added to fees."}, }, FundTxDoc()), RPCArgOptions{.oneline_description="options"}}, @@ -1713,6 +1725,8 @@ RPCHelpMan walletcreatefundedpsbt() }, {"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."}, + {"add_excess_to_recipient_position", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index where excess" + "fees are added. If not set, excess value from changeless transactions is added to fees."}, }, FundTxDoc()), RPCArgOptions{.oneline_description="options"}}, diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 7abf7f59c08..4aa53117706 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -705,7 +705,7 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co // 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_selection_weight)}) { + if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_cost_of_change, max_selection_weight, coin_selection_params.m_add_excess_to_recipient_position.has_value())}) { results.push_back(*bnb_result); } else append_error(std::move(bnb_result)); } @@ -1126,6 +1126,9 @@ static util::Result CreateTransactionInternal( const auto change_spend_fee = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size); coin_selection_params.min_viable_change = std::max(change_spend_fee + 1, dust); + // If set, do not add any excess from a changeless transaction to fees + coin_selection_params.m_add_excess_to_recipient_position = coin_control.m_add_excess_to_recipient_position; + // Include the fees for things that aren't inputs, excluding the change output const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.m_subtract_fee_outputs ? 0 : coin_selection_params.tx_noinputs_size); CAmount selection_target = recipients_sum + not_input_fees; @@ -1171,6 +1174,14 @@ static util::Result CreateTransactionInternal( { txNew.vout.emplace_back(recipient.nAmount, GetScriptForDestination(recipient.dest)); } + + // if set, add excess to selected recipient + if (result.GetTarget() != selection_target && coin_selection_params.m_add_excess_to_recipient_position) { + auto excess = result.GetTarget() - selection_target; + txNew.vout[coin_selection_params.m_add_excess_to_recipient_position.value()].nValue += excess; + recipients_sum += excess; + } + const CAmount change_amount = result.GetChange(coin_selection_params.min_viable_change, coin_selection_params.m_change_fee); if (change_amount > 0) { CTxOut newTxOut(change_amount, scriptChange); diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index eb9c349c22e..72520d34e33 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -100,7 +100,7 @@ std::optional KnapsackSolver(std::vector& groups, 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)}; + auto res{SelectCoinsBnB(utxo_pool, selection_target, cost_of_change, MAX_STANDARD_TX_WEIGHT, /*add_excess_to_target=*/false)}; return res ? std::optional(*res) : std::nullopt; } @@ -438,14 +438,14 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) 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); + const auto& no_res = SelectCoinsBnB(GroupCoins(available_coins.All(), /*subtract_fee_outputs=*/true), + selection_target, /*cost_of_change=*/0, MAX_STANDARD_TX_WEIGHT, /*add_excess_to_target=*/false); BOOST_REQUIRE(!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); + 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); @@ -453,6 +453,59 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(3 * CENT, 2, expected_result); BOOST_CHECK(EquivalentResult(expected_result, *res)); } + + { + // Test bnb add_excess_to_target + // Select best single input result with least fee waste and excess less than cost_of_change. + // Inputs set [11, 5, 2, 10], Selection Target = 6 and cost_of_change = 4. + + std::unique_ptr wallet = NewWallet(m_node); + + CoinsResult available_coins; + add_coin(available_coins, *wallet, 11 * 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); + add_coin(available_coins, *wallet, 2 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 10 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + + CAmount selection_target = 6 * CENT; + CAmount cost_of_change = 4 * CENT; + const auto& res = SelectCoinsBnB(GroupCoins(available_coins.All(), /*subtract_fee_outputs=*/false), + selection_target, /*cost_of_change=*/cost_of_change, MAX_STANDARD_TX_WEIGHT, /*add_excess_to_target=*/true); + BOOST_REQUIRE(res); + + expected_result.Clear(); + add_coin(10 * CENT, 2, expected_result); + BOOST_CHECK(EquivalentResult(expected_result, *res)); + CAmount excess = res->GetTarget() - selection_target; + BOOST_CHECK(excess < cost_of_change); + } + + { + // Test bnb add_excess_to_target + // Select best two input result with least fee waste and excess less than cost_of_change. + // Inputs set [3, 11, 2, 5], Selection Target = 6 and cost_of_change = 4. + + std::unique_ptr wallet = NewWallet(m_node); + + CoinsResult available_coins; + add_coin(available_coins, *wallet, 3 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 11 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 2 * 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); + + CAmount selection_target = 6 * CENT; + CAmount cost_of_change = 4 * CENT; + const auto& res = SelectCoinsBnB(GroupCoins(available_coins.All(), /*subtract_fee_outputs=*/false), + selection_target, /*cost_of_change=*/cost_of_change, MAX_STANDARD_TX_WEIGHT, /*add_excess_to_target=*/true); + BOOST_REQUIRE(res); + + expected_result.Clear(); + add_coin(5 * CENT, 2, expected_result); + add_coin(2 * CENT, 2, expected_result); + BOOST_CHECK(EquivalentResult(expected_result, *res)); + CAmount excess = res->GetTarget() - selection_target; + BOOST_CHECK(excess < cost_of_change); + } } BOOST_AUTO_TEST_CASE(bnb_sffo_restriction) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 209c87fd422..63c54376b32 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -260,7 +260,7 @@ FUZZ_TARGET(coinselection) // 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_selection_weight); + SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, max_selection_weight, /*add_excess_to_target=*/false); if (result_bnb) { assert(result_bnb->GetChange(coin_params.min_viable_change, coin_params.m_change_fee) == 0); assert(result_bnb->GetSelectedValue() >= target); From 17bc70f80a39d88d3e743d045bc57f70f550b066 Mon Sep 17 00:00:00 2001 From: Richard Myers Date: Fri, 17 May 2024 14:07:49 +0200 Subject: [PATCH 2/3] wallet: Add coin control option "max_excess" If set, excess from changeless spends can not exceed the lesser of this amount and the current cost_of_change, otherwise just use cost_of_change by default --- src/rpc/client.cpp | 3 +++ src/wallet/coincontrol.h | 2 ++ src/wallet/coinselection.cpp | 14 ++++++------- src/wallet/coinselection.h | 6 +++++- src/wallet/rpc/spend.cpp | 9 +++++++++ src/wallet/spend.cpp | 8 +++++++- src/wallet/test/coinselector_tests.cpp | 28 +++++++++++++------------- 7 files changed, 47 insertions(+), 23 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 8ef996e93ea..a2ac68fdf86 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -148,6 +148,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "fundrawtransaction", 1, "solving_data"}, { "fundrawtransaction", 1, "max_tx_weight"}, { "fundrawtransaction", 1, "add_excess_to_recipient_position"}, + { "fundrawtransaction", 1, "max_excess"}, { "fundrawtransaction", 2, "iswitness" }, { "walletcreatefundedpsbt", 0, "inputs" }, { "walletcreatefundedpsbt", 1, "outputs" }, @@ -168,6 +169,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "walletcreatefundedpsbt", 3, "solving_data"}, { "walletcreatefundedpsbt", 3, "max_tx_weight"}, { "walletcreatefundedpsbt", 3, "add_excess_to_recipient_position"}, + { "walletcreatefundedpsbt", 3, "max_excess"}, { "walletcreatefundedpsbt", 4, "bip32derivs" }, { "walletprocesspsbt", 1, "sign" }, { "walletprocesspsbt", 3, "bip32derivs" }, @@ -214,6 +216,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "send", 4, "solving_data"}, { "send", 4, "max_tx_weight"}, { "send", 4, "add_excess_to_recipient_position"}, + { "send", 4, "max_excess"}, { "sendall", 0, "recipients" }, { "sendall", 1, "conf_target" }, { "sendall", 3, "fee_rate"}, diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index 39cf5f797a9..8b3e557c6ee 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -119,6 +119,8 @@ public: std::optional m_max_tx_weight{std::nullopt}; //! If set, add any excess from changeless spends to the specified recipient output index instead of to fees and do not count it as waste. std::optional m_add_excess_to_recipient_position; + //! If set, excess from changeless spends can not exceed this amount, otherwise use cost_of_change by default + std::optional m_max_excess; CCoinControl(); diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 4f242dfe3cb..9fa27d1e1b7 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -52,7 +52,7 @@ struct { /* * This is the Branch and Bound Coin Selection algorithm designed by Murch. It searches for an input * set that can pay for the spending target and does not exceed the spending target by more than the - * cost of creating and spending a change output. The algorithm uses a depth-first search on a binary + * specified maximum excess value. The algorithm uses a depth-first search on a binary * tree. In the binary tree, each node corresponds to the inclusion or the omission of a UTXO. UTXOs * are sorted by their effective values and the tree is explored deterministically per the inclusion * branch first. At each node, the algorithm checks whether the selection is within the target range. @@ -82,16 +82,16 @@ struct { * values are their effective values. * @param const CAmount& selection_target This is the value that we want to select. It is the lower * bound of the range. - * @param const CAmount& cost_of_change This is the cost of creating and spending a change output. + * @param const CAmount& max_excess This is the amount of value over the selection target we can select. * This plus selection_target is the upper bound of the range. * @param int max_selection_weight The maximum allowed weight for a selection result to be valid. - * @param bool add_excess_to_target When true do not count excess as waste and add to the result target + * @param bool add_excess_to_target When true do not count excess as waste and add it to the result target * @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, +util::Result SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& max_excess, int max_selection_weight, const bool add_excess_to_target) { SelectionResult result(selection_target, SelectionAlgorithm::BNB); @@ -127,7 +127,7 @@ util::Result SelectCoinsBnB(std::vector& utxo_pool // Conditions for starting a backtrack bool backtrack = false; if (curr_value + curr_available_value < selection_target || // Cannot possibly reach target with the amount remaining in the curr_available_value. - curr_value > selection_target + cost_of_change || // Selected value is out of range, go back and try other branch + curr_value > selection_target + max_excess || // 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_selection_weight) { // Selected UTXOs weight exceeds the maximum weight allowed, cannot find more solutions by adding more inputs @@ -152,7 +152,7 @@ util::Result SelectCoinsBnB(std::vector& utxo_pool best_waste = curr_waste + curr_excess; } } - + backtrack = true; } @@ -209,7 +209,7 @@ util::Result SelectCoinsBnB(std::vector& utxo_pool auto excess = result.ResetTargetToSelectedValue(); assert(best_excess == excess); } - result.RecalculateWaste(cost_of_change, cost_of_change, CAmount{0}); + result.RecalculateWaste(max_excess, max_excess, CAmount{0}); assert(best_waste == result.GetWaste()); return result; diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index b7a5b6f2ab3..8235bb80416 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -181,6 +181,10 @@ struct CoinSelectionParams { * and not counted as waste. Otherwise excess value will be be applied to fees and counted as waste. */ std::optional m_add_excess_to_recipient_position; + /*** + * amount that changeless spends can exceed the target amount. + */ + CAmount m_max_excess{0}; CoinSelectionParams(FastRandomContext& rng_fast, int change_output_size, int change_spend_size, @@ -452,7 +456,7 @@ public: int GetWeight() const { return m_weight; } }; -util::Result 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& max_excess, int max_selection_weight, const bool add_excess_to_target); util::Result CoinGrinder(std::vector& utxo_pool, const CAmount& selection_target, CAmount change_target, int max_selection_weight); diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index e9252313ec1..fb9c45a1953 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -544,6 +544,7 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact {"input_weights", UniValueType(UniValue::VARR)}, {"max_tx_weight", UniValueType(UniValue::VNUM)}, {"add_excess_to_recipient_position", UniValue::VNUM}, + {"max_excess", UniValueType()}, // will be checked by AmountFromValue() below }, true, true); @@ -633,6 +634,11 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Cannot add excess to the recipient output at index %d; the output does not exist.", coinControl.m_add_excess_to_recipient_position.value())); } } + + if (options.exists("max_excess")) { + coinControl.m_max_excess = CAmount(AmountFromValue(options["max_excess"])); + } + } } else { // if options is null and not a bool @@ -805,6 +811,7 @@ RPCHelpMan fundrawtransaction() "Transaction building will fail if this can not be satisfied."}, {"add_excess_to_recipient_position", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index where excess" "fees are added. If not set, excess value from changeless transactions is added to fees"}, + {"max_excess", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to default wallet behavior"}, "Specify the maximum excess amount for changeless transactions in " + CURRENCY_UNIT + "."}, }, FundTxDoc()), RPCArgOptions{ @@ -1263,6 +1270,7 @@ RPCHelpMan send() "Transaction building will fail if this can not be satisfied."}, {"add_excess_to_recipient_position", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index where excess" "fees are added. If not set, excess value from changeless transactions is added to fees."}, + {"max_excess", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to default wallet behavior"}, "Specify the maximum excess amount for changeless transactions in " + CURRENCY_UNIT + "."}, }, FundTxDoc()), RPCArgOptions{.oneline_description="options"}}, @@ -1727,6 +1735,7 @@ RPCHelpMan walletcreatefundedpsbt() "Transaction building will fail if this can not be satisfied."}, {"add_excess_to_recipient_position", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index where excess" "fees are added. If not set, excess value from changeless transactions is added to fees."}, + {"max_excess", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to default wallet behavior"}, "Specify the maximum excess amount for changeless transactions in " + CURRENCY_UNIT + "."}, }, FundTxDoc()), RPCArgOptions{.oneline_description="options"}}, diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 4aa53117706..240641612a2 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -705,7 +705,7 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co // 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_selection_weight, coin_selection_params.m_add_excess_to_recipient_position.has_value())}) { + if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_max_excess, max_selection_weight, coin_selection_params.m_add_excess_to_recipient_position.has_value())}) { results.push_back(*bnb_result); } else append_error(std::move(bnb_result)); } @@ -1119,6 +1119,12 @@ static util::Result CreateTransactionInternal( coin_selection_params.m_min_change_target = GenerateChangeTarget(std::floor(recipients_sum / vecSend.size()), coin_selection_params.m_change_fee, rng_fast); + if (coin_control.m_max_excess) { + coin_selection_params.m_max_excess = std::max(coin_control.m_max_excess.value(), coin_selection_params.m_cost_of_change); + } else { + coin_selection_params.m_max_excess = coin_selection_params.m_cost_of_change; + } + // The smallest change amount should be: // 1. at least equal to dust threshold // 2. at least 1 sat greater than fees to spend it at m_discard_feerate diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 72520d34e33..2e5c26b6d06 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -98,9 +98,9 @@ std::optional KnapsackSolver(std::vector& groups, return res ? std::optional(*res) : std::nullopt; } -std::optional SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change) +std::optional SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& max_excess) { - auto res{SelectCoinsBnB(utxo_pool, selection_target, cost_of_change, MAX_STANDARD_TX_WEIGHT, /*add_excess_to_target=*/false)}; + auto res{SelectCoinsBnB(utxo_pool, selection_target, max_excess, MAX_STANDARD_TX_WEIGHT, /*add_excess_to_target=*/false)}; return res ? std::optional(*res) : std::nullopt; } @@ -439,13 +439,13 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) 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, /*add_excess_to_target=*/false); + selection_target, /*max_excess=*/0, MAX_STANDARD_TX_WEIGHT, /*add_excess_to_target=*/false); BOOST_REQUIRE(!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); + const auto& res = SelectCoinsBnB(GroupCoins(available_coins.All(), /*subtract_fee_outputs=*/true), selection_target, /*max_excess=*/0); expected_result.Clear(); add_coin(8 * CENT, 2, expected_result); @@ -456,8 +456,8 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) { // Test bnb add_excess_to_target - // Select best single input result with least fee waste and excess less than cost_of_change. - // Inputs set [11, 5, 2, 10], Selection Target = 6 and cost_of_change = 4. + // Select best single input result with least fee waste and excess less than max_excess. + // Inputs set [11, 5, 2, 10], Selection Target = 6 and max_excess = 4. std::unique_ptr wallet = NewWallet(m_node); @@ -468,22 +468,22 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(available_coins, *wallet, 10 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); CAmount selection_target = 6 * CENT; - CAmount cost_of_change = 4 * CENT; + CAmount max_excess = 4 * CENT; const auto& res = SelectCoinsBnB(GroupCoins(available_coins.All(), /*subtract_fee_outputs=*/false), - selection_target, /*cost_of_change=*/cost_of_change, MAX_STANDARD_TX_WEIGHT, /*add_excess_to_target=*/true); + selection_target, /*max_excess=*/max_excess, MAX_STANDARD_TX_WEIGHT, /*add_excess_to_target=*/true); BOOST_REQUIRE(res); expected_result.Clear(); add_coin(10 * CENT, 2, expected_result); BOOST_CHECK(EquivalentResult(expected_result, *res)); CAmount excess = res->GetTarget() - selection_target; - BOOST_CHECK(excess < cost_of_change); + BOOST_CHECK(excess < max_excess); } { // Test bnb add_excess_to_target - // Select best two input result with least fee waste and excess less than cost_of_change. - // Inputs set [3, 11, 2, 5], Selection Target = 6 and cost_of_change = 4. + // Select best two input result with least fee waste and excess less than max_excess. + // Inputs set [3, 11, 2, 5], Selection Target = 6 and max_excess = 4. std::unique_ptr wallet = NewWallet(m_node); @@ -494,9 +494,9 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(available_coins, *wallet, 5 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); CAmount selection_target = 6 * CENT; - CAmount cost_of_change = 4 * CENT; + CAmount max_excess = 4 * CENT; const auto& res = SelectCoinsBnB(GroupCoins(available_coins.All(), /*subtract_fee_outputs=*/false), - selection_target, /*cost_of_change=*/cost_of_change, MAX_STANDARD_TX_WEIGHT, /*add_excess_to_target=*/true); + selection_target, /*max_excess=*/max_excess, MAX_STANDARD_TX_WEIGHT, /*add_excess_to_target=*/true); BOOST_REQUIRE(res); expected_result.Clear(); @@ -504,7 +504,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(2 * CENT, 2, expected_result); BOOST_CHECK(EquivalentResult(expected_result, *res)); CAmount excess = res->GetTarget() - selection_target; - BOOST_CHECK(excess < cost_of_change); + BOOST_CHECK(excess < max_excess); } } From 7df63761ebbc9405af0aa7589a149a6feb8d342a Mon Sep 17 00:00:00 2001 From: Richard Myers Date: Mon, 16 Sep 2024 12:09:40 +0200 Subject: [PATCH 3/3] wallet: Update BnB upper bound to be consistent with tx building Use min_viable_change instead of cost_of_change as the upper limit of excess allowed when computing a changeless BnB solution. This prevents a corner case where dust threshold > change_fee results in some changeless BnB solutions not being considered, despite change being dropped during tx building. h/t @S3RK for identifying this issue in #26466 . --- src/wallet/spend.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 240641612a2..efb3be4a9a7 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1119,12 +1119,6 @@ static util::Result CreateTransactionInternal( coin_selection_params.m_min_change_target = GenerateChangeTarget(std::floor(recipients_sum / vecSend.size()), coin_selection_params.m_change_fee, rng_fast); - if (coin_control.m_max_excess) { - coin_selection_params.m_max_excess = std::max(coin_control.m_max_excess.value(), coin_selection_params.m_cost_of_change); - } else { - coin_selection_params.m_max_excess = coin_selection_params.m_cost_of_change; - } - // The smallest change amount should be: // 1. at least equal to dust threshold // 2. at least 1 sat greater than fees to spend it at m_discard_feerate @@ -1132,6 +1126,8 @@ static util::Result CreateTransactionInternal( const auto change_spend_fee = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size); coin_selection_params.min_viable_change = std::max(change_spend_fee + 1, dust); + coin_selection_params.m_max_excess = std::max(coin_control.m_max_excess.value_or(0), coin_selection_params.min_viable_change); + // If set, do not add any excess from a changeless transaction to fees coin_selection_params.m_add_excess_to_recipient_position = coin_control.m_add_excess_to_recipient_position;