Use SelectionResult for waste calculation

Author: achow101

src/wallet/coinselection.h

@@ -187,6 +187,8 @@ struct OutputGroup
  * where excess = selected_effective_value - target
  * change_cost = effective_feerate * change_output_size + long_term_feerate * change_spend_size
  *
+ * Note this function is separate from SelectionResult for the tests.
+ *
  * @param[in] inputs The selected inputs
  * @param[in] change_cost The cost of creating change and spending it in the future.
  *                        Only used if there is change, in which case it must be positive.

src/wallet/spend.cpp

@@ -378,16 +378,14 @@ bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const
 {
     setCoinsRet.clear();
     nValueRet = 0;
-    // Vector of results for use with waste calculation
-    // In order: calculated waste, selected inputs, selected input value (sum of input values)
-    // TODO: Use a struct representing the selection result
-    std::vector<std::tuple<CAmount, std::set<CInputCoin>, CAmount>> results;
+    // Vector of results. We will choose the best one based on waste.
+    std::vector<SelectionResult> results;
 
     // Note that unlike KnapsackSolver, we do not include the fee for creating a change output as BnB will not create a change output.
     std::vector<OutputGroup> positive_groups = GroupOutputs(wallet, coins, coin_selection_params, eligibility_filter, true /* positive_only */);
     if (auto bnb_result{SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change)}) {
         bnb_result->ComputeAndSetWaste(CAmount(0));
-        results.emplace_back(std::make_tuple(bnb_result->GetWaste(), bnb_result->GetInputSet(), bnb_result->GetSelectedValue()));
+        results.push_back(*bnb_result);
     }
 
     // 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.
@@ -396,15 +394,15 @@ bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const
     // So we need to include that for KnapsackSolver as well, as we are expecting to create a change output.
     if (auto knapsack_result{KnapsackSolver(all_groups, nTargetValue + coin_selection_params.m_change_fee)}) {
         knapsack_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
-        results.emplace_back(std::make_tuple(knapsack_result->GetWaste(), knapsack_result->GetInputSet(), knapsack_result->GetSelectedValue()));
+        results.push_back(*knapsack_result);
     }
 
     // We include the minimum final change for SRD as we do want to avoid making really small change.
     // KnapsackSolver does not need this because it includes MIN_CHANGE internally.
     const CAmount srd_target = nTargetValue + coin_selection_params.m_change_fee + MIN_FINAL_CHANGE;
     if (auto srd_result{SelectCoinsSRD(positive_groups, srd_target)}) {
         srd_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
-        results.emplace_back(std::make_tuple(srd_result->GetWaste(), srd_result->GetInputSet(), srd_result->GetSelectedValue()));
+        results.push_back(*srd_result);
     }
 
     if (results.size() == 0) {
@@ -414,11 +412,9 @@ bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const
 
     // Choose the result with the least waste
     // If the waste is the same, choose the one which spends more inputs.
-    const auto& best_result = std::min_element(results.begin(), results.end(), [](const auto& a, const auto& b) {
-        return std::get<0>(a) < std::get<0>(b) || (std::get<0>(a) == std::get<0>(b) && std::get<1>(a).size() > std::get<1>(b).size());
-    });
-    setCoinsRet = std::get<1>(*best_result);
-    nValueRet = std::get<2>(*best_result);
+    auto& best_result = *std::min_element(results.begin(), results.end());
+    setCoinsRet = best_result.GetInputSet();
+    nValueRet = best_result.GetSelectedValue();
     return true;
 }