Merge bitcoin/bitcoin#27846: [coinselection] Increase SRD target by change_fee

1771daa [fuzz] Show that SRD budgets for non-dust change (Murch)
941b8c6 [bug] Increase SRD target by change_fee (Murch)

Pull request description:

  I discovered via fuzzing of another coin selection approach that at extremely high feerates SRD may find input sets that lead to transactions without change outputs. This is an unintended outcome since SRD is meant to always produce a transaction with a change output—we use other algorithms to specifically search for changeless solutions.

  The issue occurs when the flat allowance of 50,000 ṩ for change is insufficient to pay for the creation of a change output with a non-dust amount, at and above 1,613 ṩ/vB. Increasing the change budget by `change_fee` makes SRD behave as expected at any feerates.

  Note: The intermittent failures of `test/functional/interface_usdt_mempool.py` are a known issue: bitcoin/bitcoin#27380

ACKs for top commit:
  achow101:
    ACK 1771daa
  S3RK:
    ACK 1771daa

Tree-SHA512: 3f36a3e317ef0a711d0e409069c05032bff1d45403023f3728bf73dfd55ddd9e0dc2a9969d4d69fe0a426807ebb0bed1f54abfc05581409bfe42c327acf766d4

Author: achow101

src/wallet/coinselection.cpp

@@ -191,7 +191,7 @@ class MinOutputGroupComparator
     }
 };
 
-util::Result<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value, FastRandomContext& rng,
+util::Result<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value, CAmount change_fee, FastRandomContext& rng,
                                              int max_weight)
 {
     SelectionResult result(target_value, SelectionAlgorithm::SRD);
@@ -201,7 +201,7 @@ util::Result<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utx
     // barely meets the target. Just use the lower bound change target instead of the randomly
     // generated one, since SRD will result in a random change amount anyway; avoid making the
     // target needlessly large.
-    target_value += CHANGE_LOWER;
+    target_value += CHANGE_LOWER + change_fee;
 
     std::vector<size_t> indexes;
     indexes.resize(utxo_pool.size());

src/wallet/coinselection.h

@@ -421,7 +421,7 @@ util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool
  * @param[in]  max_weight The maximum allowed weight for a selection result to be valid
  * @returns If successful, a valid SelectionResult, otherwise, util::Error
  */
-util::Result<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value, FastRandomContext& rng,
+util::Result<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value, CAmount change_fee, FastRandomContext& rng,
                                              int max_weight);
 
 // Original coin selection algorithm as a fallback

src/wallet/spend.cpp

@@ -583,7 +583,7 @@ util::Result<SelectionResult> ChooseSelectionResult(const CAmount& nTargetValue,
         results.push_back(*knapsack_result);
     } else append_error(knapsack_result);
 
-    if (auto srd_result{SelectCoinsSRD(groups.positive_group, nTargetValue, 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_inputs_weight)}) {
         srd_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
         results.push_back(*srd_result);
     } else append_error(srd_result);

src/wallet/test/coinselector_tests.cpp

@@ -968,7 +968,7 @@ static util::Result<SelectionResult> SelectCoinsSRD(const CAmount& target,
     std::unique_ptr<CWallet> 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.rng_fast, max_weight);
+    return SelectCoinsSRD(group.positive_group, target, cs_params.m_change_fee, cs_params.rng_fast, max_weight);
 }
 
 BOOST_AUTO_TEST_CASE(srd_tests)

src/wallet/test/fuzz/coinselection.cpp

@@ -90,8 +90,11 @@ FUZZ_TARGET(coinselection)
     // Run coinselection algorithms
     const auto result_bnb = SelectCoinsBnB(group_pos, target, cost_of_change, MAX_STANDARD_TX_WEIGHT);
 
-    auto result_srd = SelectCoinsSRD(group_pos, target, fast_random_context, MAX_STANDARD_TX_WEIGHT);
-    if (result_srd) result_srd->ComputeAndSetWaste(cost_of_change, cost_of_change, 0);
+    auto result_srd = SelectCoinsSRD(group_pos, target, coin_params.m_change_fee, fast_random_context, MAX_STANDARD_TX_WEIGHT);
+    if (result_srd) {
+        assert(result_srd->GetChange(CHANGE_LOWER, coin_params.m_change_fee) > 0); // Demonstrate that SRD creates change of at least CHANGE_LOWER
+        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, MAX_STANDARD_TX_WEIGHT);