Merge bitcoin/bitcoin#22019: wallet: Introduce SelectionResult for encapsulating a coin selection solution

05300c1 Use SelectionResult in SelectCoins (Andrew Chow)
9d9b101 Use SelectionResult in AttemptSelection (Andrew Chow)
bb50850 Use SelectionResult for waste calculation (Andrew Chow)
e8f7ae5 Make an OutputGroup for preset inputs (Andrew Chow)
51a9c00 Return SelectionResult from SelectCoinsSRD (Andrew Chow)
0ef6184 Return SelectionResult from KnapsackSolver (Andrew Chow)
60d2ca7 Return SelectionResult from SelectCoinsBnB (Andrew Chow)
a339add Make member variables of SelectionResult private (Andrew Chow)
cbf0b9f scripted-diff: Use SelectionResult in coin selector tests (Andrew Chow)
9d1d86d Introduce SelectionResult struct (Andrew Chow)
94d851d Fix bnb_search_test to use set equivalence for (Andrew Chow)

Pull request description:

  Instead of returning a set of selected coins and their total value as separate items, encapsulate both of these, and other variables, into a new `SelectionResult` struct. This allows us to have all of the things relevant to a coin selection solution be in a single object. `SelectionResult` enables us to implement the waste calculation in a cleaner way.

  All of the coin selection functions (`SelectCoinsBnB`, `KnapsackSolver`, `AttemptSelection`, and `SelectCoins`) are changed to use a `SelectionResult` as the output parameter.

  Based on #22009

ACKs for top commit:
  laanwj:
    Code review ACK 05300c1

Tree-SHA512: e4dbb4d78a6cda9c237d230b19e7265591efac5a101a64e6970f0654e2c4f93d13bb5d07b98e8c7b8d37321753dbfc94c28c3a7810cb1c59b5bc29b08a8493ef

Author: laanwj

src/bench/coin_selection.cpp

@@ -54,12 +54,10 @@ static void CoinSelection(benchmark::Bench& bench)
                                                     /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0),
                                                     /* tx_noinputs_size= */ 0, /* avoid_partial= */ false);
     bench.run([&] {
-        std::set<CInputCoin> setCoinsRet;
-        CAmount nValueRet;
-        bool success = AttemptSelection(wallet, 1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params);
-        assert(success);
-        assert(nValueRet == 1003 * COIN);
-        assert(setCoinsRet.size() == 2);
+        auto result = AttemptSelection(wallet, 1003 * COIN, filter_standard, coins, coin_selection_params);
+        assert(result);
+        assert(result->GetSelectedValue() == 1003 * COIN);
+        assert(result->GetInputSet().size() == 2);
     });
 }
 
@@ -92,17 +90,14 @@ static void BnBExhaustion(benchmark::Bench& bench)
 {
     // Setup
     std::vector<OutputGroup> utxo_pool;
-    CoinSet selection;
-    CAmount value_ret = 0;
 
     bench.run([&] {
         // Benchmark
         CAmount target = make_hard_case(17, utxo_pool);
-        SelectCoinsBnB(utxo_pool, target, 0, selection, value_ret); // Should exhaust
+        SelectCoinsBnB(utxo_pool, target, 0); // Should exhaust
 
         // Cleanup
         utxo_pool.clear();
-        selection.clear();
     });
 }
 

src/wallet/coinselection.cpp

@@ -56,17 +56,14 @@ struct {
  *        bound of the range.
  * @param const CAmount& cost_of_change This is the cost of creating and spending a change output.
  *        This plus selection_target is the upper bound of the range.
- * @param std::set<CInputCoin>& out_set -> This is an output parameter for the set of CInputCoins
- *        that have been selected.
- * @param CAmount& value_ret -> This is an output parameter for the total value of the CInputCoins
- *        that were selected.
+ * @returns The result of this coin selection algorithm, or std::nullopt
  */
 
 static const size_t TOTAL_TRIES = 100000;
 
-bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, std::set<CInputCoin>& out_set, CAmount& value_ret)
+std::optional<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change)
 {
-    out_set.clear();
+    SelectionResult result(selection_target);
     CAmount curr_value = 0;
 
     std::vector<bool> curr_selection; // select the utxo at this index
@@ -80,7 +77,7 @@ bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selectio
         curr_available_value += utxo.GetSelectionAmount();
     }
     if (curr_available_value < selection_target) {
-        return false;
+        return std::nullopt;
     }
 
     // Sort the utxo_pool
@@ -156,25 +153,22 @@ bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selectio
 
     // Check for solution
     if (best_selection.empty()) {
-        return false;
+        return std::nullopt;
     }
 
     // Set output set
-    value_ret = 0;
     for (size_t i = 0; i < best_selection.size(); ++i) {
         if (best_selection.at(i)) {
-            util::insert(out_set, utxo_pool.at(i).m_outputs);
-            value_ret += utxo_pool.at(i).m_value;
+            result.AddInput(utxo_pool.at(i));
         }
     }
 
-    return true;
+    return result;
 }
 
-std::optional<std::pair<std::set<CInputCoin>, CAmount>> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value)
+std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value)
 {
-    std::set<CInputCoin> out_set;
-    CAmount value_ret = 0;
+    SelectionResult result(target_value);
 
     std::vector<size_t> indexes;
     indexes.resize(utxo_pool.size());
@@ -186,10 +180,9 @@ std::optional<std::pair<std::set<CInputCoin>, CAmount>> SelectCoinsSRD(const std
         const OutputGroup& group = utxo_pool.at(i);
         Assume(group.GetSelectionAmount() > 0);
         selected_eff_value += group.GetSelectionAmount();
-        value_ret += group.m_value;
-        util::insert(out_set, group.m_outputs);
+        result.AddInput(group);
         if (selected_eff_value >= target_value) {
-            return std::make_pair(out_set, value_ret);
+            return result;
         }
     }
     return std::nullopt;
@@ -241,10 +234,9 @@ static void ApproximateBestSubset(const std::vector<OutputGroup>& groups, const
     }
 }
 
-bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& groups, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet)
+std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue)
 {
-    setCoinsRet.clear();
-    nValueRet = 0;
+    SelectionResult result(nTargetValue);
 
     // List of values less than target
     std::optional<OutputGroup> lowest_larger;
@@ -255,9 +247,8 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& group
 
     for (const OutputGroup& group : groups) {
         if (group.GetSelectionAmount() == nTargetValue) {
-            util::insert(setCoinsRet, group.m_outputs);
-            nValueRet += group.m_value;
-            return true;
+            result.AddInput(group);
+            return result;
         } else if (group.GetSelectionAmount() < nTargetValue + MIN_CHANGE) {
             applicable_groups.push_back(group);
             nTotalLower += group.GetSelectionAmount();
@@ -268,17 +259,15 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& group
 
     if (nTotalLower == nTargetValue) {
         for (const auto& group : applicable_groups) {
-            util::insert(setCoinsRet, group.m_outputs);
-            nValueRet += group.m_value;
+            result.AddInput(group);
         }
-        return true;
+        return result;
     }
 
     if (nTotalLower < nTargetValue) {
-        if (!lowest_larger) return false;
-        util::insert(setCoinsRet, lowest_larger->m_outputs);
-        nValueRet += lowest_larger->m_value;
-        return true;
+        if (!lowest_larger) return std::nullopt;
+        result.AddInput(*lowest_larger);
+        return result;
     }
 
     // Solve subset sum by stochastic approximation
@@ -295,13 +284,11 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& group
     //                                   or the next bigger coin is closer), return the bigger coin
     if (lowest_larger &&
         ((nBest != nTargetValue && nBest < nTargetValue + MIN_CHANGE) || lowest_larger->GetSelectionAmount() <= nBest)) {
-        util::insert(setCoinsRet, lowest_larger->m_outputs);
-        nValueRet += lowest_larger->m_value;
+        result.AddInput(*lowest_larger);
     } else {
         for (unsigned int i = 0; i < applicable_groups.size(); i++) {
             if (vfBest[i]) {
-                util::insert(setCoinsRet, applicable_groups[i].m_outputs);
-                nValueRet += applicable_groups[i].m_value;
+                result.AddInput(applicable_groups[i]);
             }
         }
 
@@ -316,7 +303,7 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& group
         }
     }
 
-    return true;
+    return result;
 }
 
 /******************************************************************************
@@ -395,3 +382,51 @@ CAmount GetSelectionWaste(const std::set<CInputCoin>& inputs, CAmount change_cos
 
     return waste;
 }
+
+void SelectionResult::ComputeAndSetWaste(CAmount change_cost)
+{
+    m_waste = GetSelectionWaste(m_selected_inputs, change_cost, m_target, m_use_effective);
+}
+
+CAmount SelectionResult::GetWaste() const
+{
+    Assume(m_waste != std::nullopt);
+    return *m_waste;
+}
+
+CAmount SelectionResult::GetSelectedValue() const
+{
+    return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin.txout.nValue; });
+}
+
+void SelectionResult::Clear()
+{
+    m_selected_inputs.clear();
+    m_waste.reset();
+}
+
+void SelectionResult::AddInput(const OutputGroup& group)
+{
+    util::insert(m_selected_inputs, group.m_outputs);
+    m_use_effective = !group.m_subtract_fee_outputs;
+}
+
+const std::set<CInputCoin>& SelectionResult::GetInputSet() const
+{
+    return m_selected_inputs;
+}
+
+std::vector<CInputCoin> SelectionResult::GetShuffledInputVector() const
+{
+    std::vector<CInputCoin> coins(m_selected_inputs.begin(), m_selected_inputs.end());
+    Shuffle(coins.begin(), coins.end(), FastRandomContext());
+    return coins;
+}
+
+bool SelectionResult::operator<(SelectionResult other) const
+{
+    Assume(m_waste != std::nullopt);
+    Assume(other.m_waste != std::nullopt);
+    // As this operator is only used in std::min_element, we want the result that has more inputs when waste are equal.
+    return *m_waste < *other.m_waste || (*m_waste == *other.m_waste && m_selected_inputs.size() > other.m_selected_inputs.size());
+}

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.
@@ -197,18 +199,55 @@ struct OutputGroup
  */
 [[nodiscard]] CAmount GetSelectionWaste(const std::set<CInputCoin>& inputs, CAmount change_cost, CAmount target, bool use_effective_value = true);
 
-bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, std::set<CInputCoin>& out_set, CAmount& value_ret);
+struct SelectionResult
+{
+private:
+    /** Set of inputs selected by the algorithm to use in the transaction */
+    std::set<CInputCoin> m_selected_inputs;
+    /** The target the algorithm selected for. Note that this may not be equal to the recipient amount as it can include non-input fees */
+    const CAmount m_target;
+    /** Whether the input values for calculations should be the effective value (true) or normal value (false) */
+    bool m_use_effective{false};
+    /** The computed waste */
+    std::optional<CAmount> m_waste;
+
+public:
+    explicit SelectionResult(const CAmount target)
+        : m_target(target) {}
+
+    SelectionResult() = delete;
+
+    /** Get the sum of the input values */
+    [[nodiscard]] CAmount GetSelectedValue() const;
+
+    void Clear();
+
+    void AddInput(const OutputGroup& group);
+
+    /** Calculates and stores the waste for this selection via GetSelectionWaste */
+    void ComputeAndSetWaste(CAmount change_cost);
+    [[nodiscard]] CAmount GetWaste() const;
+
+    /** Get m_selected_inputs */
+    const std::set<CInputCoin>& GetInputSet() const;
+    /** Get the vector of CInputCoins that will be used to fill in a CTransaction's vin */
+    std::vector<CInputCoin> GetShuffledInputVector() const;
+
+    bool operator<(SelectionResult other) const;
+};
+
+std::optional<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change);
 
 /** Select coins by Single Random Draw. OutputGroups are selected randomly from the eligible
  * outputs until the target is satisfied
  *
  * @param[in]  utxo_pool    The positive effective value OutputGroups eligible for selection
  * @param[in]  target_value The target value to select for
- * @returns If successful, a pair of set of outputs and total selected value, otherwise, std::nullopt
+ * @returns If successful, a SelectionResult, otherwise, std::nullopt
  */
-std::optional<std::pair<std::set<CInputCoin>, CAmount>> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value);
+std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value);
 
 // Original coin selection algorithm as a fallback
-bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& groups, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet);
+std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue);
 
 #endif // BITCOIN_WALLET_COINSELECTION_H