coin selection: knapsack, select closest UTXO above target if result exceeds max tx size

The simplest scenario where this is useful is on the 'check_max_weight' unit test
already:

We create 1515 UTXOs with 0.033 BTC each, and 1 UTXO with 50 BTC. Then perform
Coin Selection.

As the selection of the 1515 small UTXOs exceeds the max allowed tx size, the
expectation here is to receive a selection result that only contain the big
UTXO (which is not happening for the reasons stated below).

As knapsack returns a result that exceeds the max allowed transaction size, we
fallback to SRD, which selects coins randomly up until the target is met. So
we end up with a selection result with lot more coins than what is needed.

src/wallet/coinselection.cpp

@@ -15,6 +15,13 @@
 #include <optional>
 
 namespace wallet {
+// Common selection error across the algorithms
+static util::Result<SelectionResult> ErrorMaxWeightExceeded()
+{
+    return util::Error{_("The inputs size exceeds the maximum weight. "
+                         "Please try sending a smaller amount or manually consolidating your wallet's UTXOs")};
+}
+
 // Descending order comparator
 struct {
     bool operator()(const OutputGroup& a, const OutputGroup& b) const
@@ -253,7 +260,7 @@ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::v
 }
 
 util::Result<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue,
-                                              CAmount change_target, FastRandomContext& rng)
+                                             CAmount change_target, FastRandomContext& rng, int max_weight)
 {
     SelectionResult result(nTargetValue, SelectionAlgorithm::KNAPSACK);
 
@@ -313,6 +320,16 @@ util::Result<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, c
             }
         }
 
+        // If the result exceeds the maximum allowed size, return closest UTXO above the target
+        if (result.GetWeight() > max_weight) {
+            // No coin above target, nothing to do.
+            if (!lowest_larger) return ErrorMaxWeightExceeded();
+
+            // Return closest UTXO above target
+            result.Clear();
+            result.AddInput(*lowest_larger);
+        }
+
         if (LogAcceptCategory(BCLog::SELECTCOINS, BCLog::Level::Debug)) {
             std::string log_message{"Coin selection best subset: "};
             for (unsigned int i = 0; i < applicable_groups.size(); i++) {

src/wallet/coinselection.h

@@ -422,7 +422,7 @@ util::Result<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utx
 
 // Original coin selection algorithm as a fallback
 util::Result<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue,
-                                              CAmount change_target, FastRandomContext& rng);
+                                             CAmount change_target, FastRandomContext& rng, int max_weight);
 } // namespace wallet
 
 #endif // BITCOIN_WALLET_COINSELECTION_H

src/wallet/spend.cpp

@@ -563,12 +563,18 @@ util::Result<SelectionResult> ChooseSelectionResult(const CAmount& nTargetValue,
         }
     };
 
+    // Maximum allowed weight
+    int max_inputs_weight = MAX_STANDARD_TX_WEIGHT - (coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR);
+
     if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_cost_of_change)}) {
         results.push_back(*bnb_result);
     } else append_error(bnb_result);
 
+    // As Knapsack and SRD can create change, also deduce change weight.
+    max_inputs_weight -= (coin_selection_params.change_output_size * WITNESS_SCALE_FACTOR);
+
     // The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here.
-    if (auto knapsack_result{KnapsackSolver(groups.mixed_group, nTargetValue, coin_selection_params.m_min_change_target, coin_selection_params.rng_fast)}) {
+    if (auto knapsack_result{KnapsackSolver(groups.mixed_group, nTargetValue, coin_selection_params.m_min_change_target, coin_selection_params.rng_fast, max_inputs_weight)}) {
         knapsack_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
         results.push_back(*knapsack_result);
     } else append_error(knapsack_result);

src/wallet/test/coinselector_tests.cpp

@@ -87,6 +87,14 @@ static void add_coin(CoinsResult& available_coins, CWallet& wallet, const CAmoun
     available_coins.Add(OutputType::BECH32, {COutPoint(wtx.GetHash(), nInput), txout, nAge, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe, feerate});
 }
 
+// Helper
+std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue,
+                                              CAmount change_target, FastRandomContext& rng)
+{
+    auto res{KnapsackSolver(groups, nTargetValue, change_target, rng, MAX_STANDARD_TX_WEIGHT)};
+    return res ? std::optional<SelectionResult>(*res) : std::nullopt;
+}
+
 /** Check if SelectionResult a is equivalent to SelectionResult b.
  * Equivalent means same input values, but maybe different inputs (i.e. same value, different prevout) */
 static bool EquivalentResult(const SelectionResult& a, const SelectionResult& b)
@@ -987,7 +995,10 @@ BOOST_AUTO_TEST_CASE(check_max_weight)
             chain);
 
         BOOST_CHECK(result);
-        BOOST_CHECK(has_coin(result->GetInputSet(), CAmount(50 * COIN)));
+        // Verify that only the 50 BTC UTXO was selected
+        const auto& selection_res = result->GetInputSet();
+        BOOST_CHECK(selection_res.size() == 1);
+        BOOST_CHECK((*selection_res.begin())->GetEffectiveValue() == 50 * COIN);
     }
 
     {

src/wallet/test/fuzz/coinselection.cpp

@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <policy/feerate.h>
+#include <policy/policy.h>
 #include <primitives/transaction.h>
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
@@ -44,6 +45,9 @@ static void GroupCoins(FuzzedDataProvider& fuzzed_data_provider, const std::vect
     if (valid_outputgroup) output_groups.push_back(output_group);
 }
 
+// Returns true if the result contains an error and the message is not empty
+static bool HasErrorMsg(const util::Result<SelectionResult>& res) { return !util::ErrorString(res).empty(); }
+
 FUZZ_TARGET(coinselection)
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
@@ -90,12 +94,12 @@ FUZZ_TARGET(coinselection)
     if (result_srd) result_srd->ComputeAndSetWaste(cost_of_change, cost_of_change, 0);
 
     CAmount change_target{GenerateChangeTarget(target, coin_params.m_change_fee, fast_random_context)};
-    auto result_knapsack = KnapsackSolver(group_all, target, change_target, fast_random_context);
+    auto result_knapsack = KnapsackSolver(group_all, target, change_target, fast_random_context, MAX_STANDARD_TX_WEIGHT);
     if (result_knapsack) result_knapsack->ComputeAndSetWaste(cost_of_change, cost_of_change, 0);
 
     // If the total balance is sufficient for the target and we are not using
-    // effective values, Knapsack should always find a solution.
+    // effective values, Knapsack should always find a solution (unless the selection exceeded the max tx weight).
-    if (total_balance >= target && subtract_fee_outputs) {
+    if (total_balance >= target && subtract_fee_outputs && !HasErrorMsg(result_knapsack)) {
         assert(result_knapsack);
     }
 }