Merge bitcoin/bitcoin#25806: wallet: group outputs only once, decouple it from Coin Selection

6a302d4 wallet: single output groups filtering and grouping process (furszy)
bd91ed1 wallet: unify outputs grouping process (furszy)
5596200 test: coinselector_tests refactor, use CoinsResult instead of plain std::vector (furszy)
34f54a0 wallet: decouple outputs grouping process from each ChooseSelectionResult (furszy)
461f082 refactor: make OutputGroup::m_outputs field a vector of shared_ptr (furszy)
d8e749b test: wallet, add coverage for outputs grouping process (furszy)
06ec8f9 wallet: make OutputGroup "positive_only" filter explicit (furszy)

Pull request description:

  The idea originates from bitcoin/bitcoin#24845 (comment).

  Note:
  For clarity, it's recommended to start reviewing from the end result to understand the structure of the flow.

  #### GroupOutputs function rationale:
  If "Avoid Partial Spends" is enabled, the function gathers outputs with the same script together inside a container. So Coin Selection can treats them as if them were just one possible input and either select them all or not select them.

  #### How the Inputs Fetch + Selection process roughly works:

  ```
  1. Fetch user’s manually selected inputs.
  2. Fetch wallet available coins (walks through the entire wallet txes map) and insert them into a set of vectors (each vector store outputs from a single type).
  3. Coin Selection Process:
     Call `AttemptSelection` 8 times. Each of them expands the coin eligibility filter (accepting a larger subset of coins in the calculation) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds.

     Each `AttemptSelection` call performs the following actions:
       - For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and a combination of all of them):
         Call ‘ChooseSelectionResult’ providing the respective, filtered by type, coins vector. Which:
             I. Groups the outputs vector twice (one for positive only and a second one who includes the negative ones as well).
                - GroupOutputs walks-through the entire inputted coins vector one time at least, + more if we are avoiding partial spends, to generate a vector of OutputGroups.
             II. Then performs every coin selection algorithm using the recently created vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD.
             III. Then returns the best solution out of them.
  ```

  We perform the general operation of gathering outputs, with the same script, into a single container inside:
  Each coins selection attempt (8 times —> each coin eligibility filter), for each of the outputs vector who were filtered by type (plus another one joining all the outputs as well if needed), twice (one for the positive only outputs effective value and a second one for all of them).

  So, in the worst case scenario where no solution is found after the 8 Coin Selection attempts, the `GroupOutputs` function is called 80 times (8 * 5 * 2).

  #### Improvements:

  This proposal streamlines the process so that the output groups, filtered by coin eligibility and type, are created in a single loop outside of the Coin Selection Process.

  The new process is as follows:

  ```
  1. Fetch user’s manually selected inputs.
  2. Fetch wallet available coins.
  3. Group outputs by each coin eligibility filter and each different output type found.
  4. Coin Selection Process:
     Call AttemptSelection 8 times. Each of them expands the coin eligibility filter (accepting different output groups) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds.

     Each ‘AttemptSelection’ call performs the following actions:
        - For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and all of them):
            A. Call ‘ChooseSelectionResult’ providing the respective, filtered by type, output group. Which:
               I. Performs every coin selection algorithm using the provided vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD.
               II. Then returns the best solution out of them.
  ```

  Extra Note:
  The next steps after this PR will be to:
  1) Merge `AvailableCoins` and `GroupOutputs` processes.
  2) Skip entire coin selection rounds if no new coins are added into the subsequent round.
  3) Remove global feerates from the OutputGroup class.
  4) Remove secondary "grouped" tx creation from `CreateTransactionInternal` by running Coin Selection results over the aps grouped outputs vs non-aps ones.

ACKs for top commit:
  S3RK:
    ReACK 6a302d4
  achow101:
    ACK 6a302d4
  theStack:
    re-ACK 6a302d4 🥥

Tree-SHA512: dff849063be328e7d9c358ec80239a6db2cd6131963b511b83699b95b337d3106263507eaba0119eaac63e6ac21c6c42d187ae23d79d9220b90c323d44b01d24

Author: achow101

src/Makefile.test.include

@@ -179,7 +179,8 @@ BITCOIN_TESTS += \
   wallet/test/ismine_tests.cpp \
   wallet/test/rpc_util_tests.cpp \
   wallet/test/scriptpubkeyman_tests.cpp \
-  wallet/test/walletload_tests.cpp
+  wallet/test/walletload_tests.cpp \
+  wallet/test/group_outputs_tests.cpp
 
 FUZZ_SUITE_LD_COMMON +=\
  $(SQLITE_LIBS) \

src/bench/coin_selection.cpp

@@ -75,8 +75,9 @@ static void CoinSelection(benchmark::Bench& bench)
         /*tx_noinputs_size=*/ 0,
         /*avoid_partial=*/ false,
     };
+    auto group = wallet::GroupOutputs(wallet, available_coins, coin_selection_params, {{filter_standard}})[filter_standard];
     bench.run([&] {
-        auto result = AttemptSelection(wallet, 1003 * COIN, filter_standard, available_coins, coin_selection_params, /*allow_mixed_output_types=*/true);
+        auto result = AttemptSelection(1003 * COIN, group, coin_selection_params, /*allow_mixed_output_types=*/true);
         assert(result);
         assert(result->GetSelectedValue() == 1003 * COIN);
         assert(result->GetInputSet().size() == 2);
@@ -91,7 +92,7 @@ static void add_coin(const CAmount& nValue, int nInput, std::vector<OutputGroup>
     tx.vout[nInput].nValue = nValue;
     COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 0, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ true, /*fees=*/ 0);
     set.emplace_back();
-    set.back().Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false);
+    set.back().Insert(std::make_shared<COutput>(output), /*ancestors=*/ 0, /*descendants=*/ 0);
 }
 // Copied from src/wallet/test/coinselector_tests.cpp
 static CAmount make_hard_case(int utxos, std::vector<OutputGroup>& utxo_pool)

src/wallet/coinselection.cpp

@@ -333,12 +333,9 @@ std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups,
 
  ******************************************************************************/
 
-void OutputGroup::Insert(const COutput& output, size_t ancestors, size_t descendants, bool positive_only) {
-    // Filter for positive only here before adding the coin
-    if (positive_only && output.GetEffectiveValue() <= 0) return;
-
+void OutputGroup::Insert(const std::shared_ptr<COutput>& output, size_t ancestors, size_t descendants) {
     m_outputs.push_back(output);
-    COutput& coin = m_outputs.back();
+    auto& coin = *m_outputs.back();
 
     fee += coin.GetFee();
 
@@ -358,8 +355,8 @@ void OutputGroup::Insert(const COutput& output, size_t ancestors, size_t descend
     // coin itself; thus, this value is counted as the max, not the sum
     m_descendants = std::max(m_descendants, descendants);
 
-    if (output.input_bytes > 0) {
-        m_weight += output.input_bytes * WITNESS_SCALE_FACTOR;
+    if (output->input_bytes > 0) {
+        m_weight += output->input_bytes * WITNESS_SCALE_FACTOR;
     }
 }
 
@@ -375,15 +372,38 @@ CAmount OutputGroup::GetSelectionAmount() const
     return m_subtract_fee_outputs ? m_value : effective_value;
 }
 
-CAmount GetSelectionWaste(const std::set<COutput>& inputs, CAmount change_cost, CAmount target, bool use_effective_value)
+void OutputGroupTypeMap::Push(const OutputGroup& group, OutputType type, bool insert_positive, bool insert_mixed)
+{
+    if (group.m_outputs.empty()) return;
+
+    Groups& groups = groups_by_type[type];
+    if (insert_positive && group.GetSelectionAmount() > 0) {
+        groups.positive_group.emplace_back(group);
+        all_groups.positive_group.emplace_back(group);
+    }
+    if (insert_mixed) {
+        groups.mixed_group.emplace_back(group);
+        all_groups.mixed_group.emplace_back(group);
+    }
+}
+
+std::optional<Groups> OutputGroupTypeMap::Find(OutputType type)
+{
+    auto it_by_type = groups_by_type.find(type);
+    if (it_by_type == groups_by_type.end()) return std::nullopt;
+    return it_by_type->second;
+}
+
+CAmount GetSelectionWaste(const std::set<std::shared_ptr<COutput>>& inputs, CAmount change_cost, CAmount target, bool use_effective_value)
 {
     // This function should not be called with empty inputs as that would mean the selection failed
     assert(!inputs.empty());
 
     // Always consider the cost of spending an input now vs in the future.
     CAmount waste = 0;
     CAmount selected_effective_value = 0;
-    for (const COutput& coin : inputs) {
+    for (const auto& coin_ptr : inputs) {
+        const COutput& coin = *coin_ptr;
         waste += coin.GetFee() - coin.long_term_fee;
         selected_effective_value += use_effective_value ? coin.GetEffectiveValue() : coin.txout.nValue;
     }
@@ -431,12 +451,12 @@ CAmount SelectionResult::GetWaste() const
 
 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; });
+    return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin->txout.nValue; });
 }
 
 CAmount SelectionResult::GetSelectedEffectiveValue() const
 {
-    return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin.GetEffectiveValue(); });
+    return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin->GetEffectiveValue(); });
 }
 
 void SelectionResult::Clear()
@@ -455,14 +475,14 @@ void SelectionResult::AddInput(const OutputGroup& group)
     m_weight += group.m_weight;
 }
 
-void SelectionResult::AddInputs(const std::set<COutput>& inputs, bool subtract_fee_outputs)
+void SelectionResult::AddInputs(const std::set<std::shared_ptr<COutput>>& inputs, bool subtract_fee_outputs)
 {
     // As it can fail, combine inputs first
     InsertInputs(inputs);
     m_use_effective = !subtract_fee_outputs;
 
     m_weight += std::accumulate(inputs.cbegin(), inputs.cend(), 0, [](int sum, const auto& coin) {
-        return sum + std::max(coin.input_bytes, 0) * WITNESS_SCALE_FACTOR;
+        return sum + std::max(coin->input_bytes, 0) * WITNESS_SCALE_FACTOR;
     });
 }
 
@@ -480,14 +500,14 @@ void SelectionResult::Merge(const SelectionResult& other)
     m_weight += other.m_weight;
 }
 
-const std::set<COutput>& SelectionResult::GetInputSet() const
+const std::set<std::shared_ptr<COutput>>& SelectionResult::GetInputSet() const
 {
     return m_selected_inputs;
 }
 
-std::vector<COutput> SelectionResult::GetShuffledInputVector() const
+std::vector<std::shared_ptr<COutput>> SelectionResult::GetShuffledInputVector() const
 {
-    std::vector<COutput> coins(m_selected_inputs.begin(), m_selected_inputs.end());
+    std::vector<std::shared_ptr<COutput>> coins(m_selected_inputs.begin(), m_selected_inputs.end());
     Shuffle(coins.begin(), coins.end(), FastRandomContext());
     return coins;
 }

src/wallet/coinselection.h

@@ -7,6 +7,7 @@
 
 #include <consensus/amount.h>
 #include <consensus/consensus.h>
+#include <outputtype.h>
 #include <policy/feerate.h>
 #include <primitives/transaction.h>
 #include <random.h>
@@ -192,13 +193,18 @@ struct CoinEligibilityFilter
     CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_ancestors) {}
     CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants) {}
     CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants, bool include_partial) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants), m_include_partial_groups(include_partial) {}
+
+    bool operator<(const CoinEligibilityFilter& other) const {
+        return std::tie(conf_mine, conf_theirs, max_ancestors, max_descendants, m_include_partial_groups)
+               < std::tie(other.conf_mine, other.conf_theirs, other.max_ancestors, other.max_descendants, other.m_include_partial_groups);
+    }
 };
 
 /** A group of UTXOs paid to the same output script. */
 struct OutputGroup
 {
     /** The list of UTXOs contained in this output group. */
-    std::vector<COutput> m_outputs;
+    std::vector<std::shared_ptr<COutput>> m_outputs;
     /** Whether the UTXOs were sent by the wallet to itself. This is relevant because we may want at
      * least a certain number of confirmations on UTXOs received from outside wallets while trusting
      * our own UTXOs more. */
@@ -237,11 +243,37 @@ struct OutputGroup
         m_subtract_fee_outputs(params.m_subtract_fee_outputs)
     {}
 
-    void Insert(const COutput& output, size_t ancestors, size_t descendants, bool positive_only);
+    void Insert(const std::shared_ptr<COutput>& output, size_t ancestors, size_t descendants);
     bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const;
     CAmount GetSelectionAmount() const;
 };
 
+struct Groups {
+    // Stores 'OutputGroup' containing only positive UTXOs (value > 0).
+    std::vector<OutputGroup> positive_group;
+    // Stores 'OutputGroup' which may contain both positive and negative UTXOs.
+    std::vector<OutputGroup> mixed_group;
+};
+
+/** Stores several 'Groups' whose were mapped by output type. */
+struct OutputGroupTypeMap
+{
+    // Maps output type to output groups.
+    std::map<OutputType, Groups> groups_by_type;
+    // All inserted groups, no type distinction.
+    Groups all_groups;
+
+    // Based on the insert flag; appends group to the 'mixed_group' and, if value > 0, to the 'positive_group'.
+    // This affects both; the groups filtered by type and the overall groups container.
+    void Push(const OutputGroup& group, OutputType type, bool insert_positive, bool insert_mixed);
+    // Retrieves 'Groups' filtered by type
+    std::optional<Groups> Find(OutputType type);
+    // Different output types count
+    size_t TypesCount() { return groups_by_type.size(); }
+};
+
+typedef std::map<CoinEligibilityFilter, OutputGroupTypeMap> FilteredOutputGroups;
+
 /** Compute 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)
@@ -259,7 +291,7 @@ struct OutputGroup
  * @param[in] use_effective_value Whether to use the input's effective value (when true) or the real value (when false).
  * @return The waste
  */
-[[nodiscard]] CAmount GetSelectionWaste(const std::set<COutput>& inputs, CAmount change_cost, CAmount target, bool use_effective_value = true);
+[[nodiscard]] CAmount GetSelectionWaste(const std::set<std::shared_ptr<COutput>>& inputs, CAmount change_cost, CAmount target, bool use_effective_value = true);
 
 
 /** Choose a random change target for each transaction to make it harder to fingerprint the Core
@@ -292,7 +324,7 @@ struct SelectionResult
 {
 private:
     /** Set of inputs selected by the algorithm to use in the transaction */
-    std::set<COutput> m_selected_inputs;
+    std::set<std::shared_ptr<COutput>> m_selected_inputs;
     /** The target the algorithm selected for. Equal to the recipient amount plus non-input fees */
     CAmount m_target;
     /** The algorithm used to produce this result */
@@ -329,7 +361,7 @@ struct SelectionResult
     void Clear();
 
     void AddInput(const OutputGroup& group);
-    void AddInputs(const std::set<COutput>& inputs, bool subtract_fee_outputs);
+    void AddInputs(const std::set<std::shared_ptr<COutput>>& inputs, bool subtract_fee_outputs);
 
     /** Calculates and stores the waste for this selection via GetSelectionWaste */
     void ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee);
@@ -344,9 +376,9 @@ struct SelectionResult
     void Merge(const SelectionResult& other);
 
     /** Get m_selected_inputs */
-    const std::set<COutput>& GetInputSet() const;
+    const std::set<std::shared_ptr<COutput>>& GetInputSet() const;
     /** Get the vector of COutputs that will be used to fill in a CTransaction's vin */
-    std::vector<COutput> GetShuffledInputVector() const;
+    std::vector<std::shared_ptr<COutput>> GetShuffledInputVector() const;
 
     bool operator<(SelectionResult other) const;