Merge 7df63761eb into a50af6e4c4

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();

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"},

src/wallet/coincontrol.h

@@ -117,6 +117,10 @@ public:
     std::optional<uint32_t> m_version;
     //! Caps weight of resulting tx
     std::optional<int> 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<uint32_t> 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<CAmount> m_max_excess;
 
     CCoinControl();
 

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<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change,
+util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& max_excess,
-                                             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<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool
     CAmount curr_waste = 0;
     std::vector<size_t> 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<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& 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) {
+            CAmount curr_excess = (curr_value - selection_target);
-                best_selection = curr_selection;
+            if (add_excess_to_target) {
-                best_waste = curr_waste;
+                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<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& 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;

src/wallet/coinselection.h

@@ -176,6 +176,16 @@ struct CoinSelectionParams {
     bool m_include_unsafe_inputs = false;
     /** The maximum weight for this transaction. */
     std::optional<int> 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<uint32_t> 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<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change,
+util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& max_excess,
-                                             int max_selection_weight);
+                                             int max_selection_weight, const bool add_excess_to_target);
 
 util::Result<SelectionResult> CoinGrinder(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, CAmount change_target, int max_selection_weight);
 

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<uint32_t>();
+            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"}},

src/wallet/spend.cpp

@@ -710,7 +710,7 @@ util::Result<SelectionResult> 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<CreatedTransactionResult> 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<CreatedTransactionResult> 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);

src/wallet/test/coinselector_tests.cpp

@@ -98,9 +98,9 @@ std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups,
     return res ? std::optional<SelectionResult>(*res) : std::nullopt;
 }
 
-std::optional<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change)
+std::optional<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& 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<SelectionResult>(*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),
+        const auto& no_res = SelectCoinsBnB(GroupCoins(available_coins.All(), /*subtract_fee_outputs=*/true),
-                                            selection_target, /*cost_of_change=*/0, MAX_STANDARD_TX_WEIGHT);
+                                            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<CWallet> 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<CWallet> 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)

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);