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 1b711e3c5b1..0849615dcbb 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -150,6 +150,8 @@ static const CRPCConvertParam vRPCConvertParams[] = { "fundrawtransaction", 1, "replaceable"}, { "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" }, @@ -169,6 +171,8 @@ static const CRPCConvertParam vRPCConvertParams[] = { "walletcreatefundedpsbt", 3, "replaceable"}, { "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" }, @@ -216,6 +220,8 @@ static const CRPCConvertParam vRPCConvertParams[] = { "send", 4, "replaceable"}, { "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 d36314312ad..8b3e557c6ee 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -117,6 +117,10 @@ 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; + //! 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 6e6d7e053bd..b2fcd1b497c 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,17 @@ 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 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, - int max_selection_weight) +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); 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; @@ -125,23 +127,32 @@ 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 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,7 +205,11 @@ util::Result SelectCoinsBnB(std::vector& utxo_pool for (const size_t& i : best_selection) { result.AddInput(utxo_pool.at(i)); } - result.RecalculateWaste(cost_of_change, cost_of_change, CAmount{0}); + if (add_excess_to_target) { + auto excess = result.ResetTargetToSelectedValue(); + assert(best_excess == excess); + } + result.RecalculateWaste(max_excess, max_excess, CAmount{0}); assert(best_waste == result.GetWaste()); return result; @@ -852,6 +867,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..8235bb80416 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -176,6 +176,16 @@ 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; + /*** + * 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, CAmount min_change_target, CFeeRate effective_feerate, @@ -375,6 +385,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 @@ -443,8 +456,8 @@ public: int GetWeight() const { return m_weight; } }; -util::Result SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, - int max_selection_weight); +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 0cba830e2ab..f86a649654d 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -543,6 +543,8 @@ 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}, + {"max_excess", UniValueType()}, // will be checked by AmountFromValue() below }, true, true); @@ -625,6 +627,18 @@ 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())); + } + } + + if (options.exists("max_excess")) { + coinControl.m_max_excess = CAmount(AmountFromValue(options["max_excess"])); + } + } } else { // if options is null and not a bool @@ -795,6 +809,9 @@ 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"}, + {"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{ @@ -1253,6 +1270,9 @@ 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."}, + {"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"}}, @@ -1715,6 +1735,9 @@ 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."}, + {"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 7b26cf15bd3..76076e32e37 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -710,7 +710,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_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)); } @@ -1131,6 +1131,11 @@ 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; + // 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; @@ -1177,6 +1182,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..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)}; + 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; } @@ -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, /*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); @@ -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 max_excess. + // Inputs set [11, 5, 2, 10], Selection Target = 6 and max_excess = 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 max_excess = 4 * CENT; + const auto& res = SelectCoinsBnB(GroupCoins(available_coins.All(), /*subtract_fee_outputs=*/false), + 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 < max_excess); + } + + { + // Test bnb add_excess_to_target + // 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); + + 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 max_excess = 4 * CENT; + const auto& res = SelectCoinsBnB(GroupCoins(available_coins.All(), /*subtract_fee_outputs=*/false), + selection_target, /*max_excess=*/max_excess, 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 < max_excess); + } } BOOST_AUTO_TEST_CASE(bnb_sffo_restriction) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index ed77befac2b..1884069ac9a 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -261,7 +261,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);