Merge #20040: wallet: Refactor OutputGroups to handle fees and spending eligibility on grouping

5d45976 Rewrite OutputGroups to be clearer and to use scriptPubKeys (Andrew Chow)
f6b3052 Explicitly filter out partial groups when we don't want them (Andrew Chow)
416d74f Move OutputGroup positive only filtering into Insert (Andrew Chow)
d895e98 Move EligibleForSpending into GroupOutputs (Andrew Chow)
99b399a Move fee setting of OutputGroup to Insert (Andrew Chow)
6148a8a Move GroupOutputs into SelectCoinsMinConf (Andrew Chow)
2acad03 Remove OutputGroup non-default constructors (Andrew Chow)

Pull request description:

  Even after #17458, we still deal with setting fees of an `OutputGroup` and filtering the `OutputGroup` outside of the struct. We currently make all of the `OutputGroup`s in `SelectCoins` and then copy and modify them within each `SelectCoinsMinConf` scenario. This PR changes this to constructing the `OutputGroup`s within the `SelectCoinsMinConf` so that the scenario can be taken into account during the group construction. Furthermore, setting of fees and filtering for effective value is moved into `OutputGroup::Insert` itself so that we don't add undesirable outputs to an `OutputGroup` rather than deleting them afterwards.

  To facilitate fee calculation and effective value filtering during `OutputGroup::Insert`, `OutputGroup` now takes the feerates in its constructor and computes the fees and effective value for each output during `Insert`.

  While removing `OutputGroup`s in accordance with the `CoinEligibilityFilter` still requires creating the `OutputGroup`s first, we can do that within the function that makes them - `GroupOutput`s.

ACKs for top commit:
  Xekyo:
    Code review ACK: bitcoin/bitcoin@5d45976
  fjahr:
    Code review ACK 5d45976
  meshcollider:
    Light utACK 5d45976

Tree-SHA512: 35965b6d49a87f4ebb366ec4f00aafaaf78e9282481ae2c9682b515a3a9f2cbcd3cd6e202fee29489d48fe7f3a7cede4270796f5e72bbaff76da647138fb3059

Author: meshcollider

src/bench/coin_selection.cpp

@@ -42,20 +42,19 @@ static void CoinSelection(benchmark::Bench& bench)
     }
     addCoin(3 * COIN, wallet, wtxs);
 
-    // Create groups
-    std::vector<OutputGroup> groups;
+    // Create coins
+    std::vector<COutput> coins;
     for (const auto& wtx : wtxs) {
-        COutput output(wtx.get(), 0 /* iIn */, 6 * 24 /* nDepthIn */, true /* spendable */, true /* solvable */, true /* safe */);
-        groups.emplace_back(output.GetInputCoin(), 6, false, 0, 0);
+        coins.emplace_back(wtx.get(), 0 /* iIn */, 6 * 24 /* nDepthIn */, true /* spendable */, true /* solvable */, true /* safe */);
     }
 
     const CoinEligibilityFilter filter_standard(1, 6, 0);
-    const CoinSelectionParams coin_selection_params(true, 34, 148, CFeeRate(0), 0);
+    const CoinSelectionParams coin_selection_params(true, 34, 148, CFeeRate(0), 0, false);
     bench.run([&] {
         std::set<CInputCoin> setCoinsRet;
         CAmount nValueRet;
         bool bnb_used;
-        bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used);
+        bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used);
         assert(success);
         assert(nValueRet == 1003 * COIN);
         assert(setCoinsRet.size() == 2);
@@ -75,7 +74,8 @@ static void add_coin(const CAmount& nValue, int nInput, std::vector<OutputGroup>
     tx.vout.resize(nInput + 1);
     tx.vout[nInput].nValue = nValue;
     std::unique_ptr<CWalletTx> wtx = MakeUnique<CWalletTx>(&testWallet, MakeTransactionRef(std::move(tx)));
-    set.emplace_back(COutput(wtx.get(), nInput, 0, true, true, true).GetInputCoin(), 0, true, 0, 0);
+    set.emplace_back();
+    set.back().Insert(COutput(wtx.get(), nInput, 0, true, true, true).GetInputCoin(), 0, true, 0, 0, false);
     wtxn.emplace_back(std::move(wtx));
 }
 // Copied from src/wallet/test/coinselector_tests.cpp

src/wallet/coinselection.cpp

@@ -300,8 +300,26 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& group
 
  ******************************************************************************/
 
-void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants) {
+void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants, bool positive_only) {
+    // Compute the effective value first
+    const CAmount coin_fee = output.m_input_bytes < 0 ? 0 : m_effective_feerate.GetFee(output.m_input_bytes);
+    const CAmount ev = output.txout.nValue - coin_fee;
+
+    // Filter for positive only here before adding the coin
+    if (positive_only && ev <= 0) return;
+
     m_outputs.push_back(output);
+    CInputCoin& coin = m_outputs.back();
+
+    coin.m_fee = coin_fee;
+    fee += coin.m_fee;
+
+    coin.m_long_term_fee = coin.m_input_bytes < 0 ? 0 : m_long_term_feerate.GetFee(coin.m_input_bytes);
+    long_term_fee += coin.m_long_term_fee;
+
+    coin.effective_value = ev;
+    effective_value += coin.effective_value;
+
     m_from_me &= from_me;
     m_value += output.txout.nValue;
     m_depth = std::min(m_depth, depth);
@@ -312,20 +330,6 @@ void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size
     // descendants is the count as seen from the top ancestor, not the descendants as seen from the
     // coin itself; thus, this value is counted as the max, not the sum
     m_descendants = std::max(m_descendants, descendants);
-    effective_value += output.effective_value;
-    fee += output.m_fee;
-    long_term_fee += output.m_long_term_fee;
-}
-
-std::vector<CInputCoin>::iterator OutputGroup::Discard(const CInputCoin& output) {
-    auto it = m_outputs.begin();
-    while (it != m_outputs.end() && it->outpoint != output.outpoint) ++it;
-    if (it == m_outputs.end()) return it;
-    m_value -= output.txout.nValue;
-    effective_value -= output.effective_value;
-    fee -= output.m_fee;
-    long_term_fee -= output.m_long_term_fee;
-    return m_outputs.erase(it);
 }
 
 bool OutputGroup::EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const
@@ -334,35 +338,3 @@ bool OutputGroup::EligibleForSpending(const CoinEligibilityFilter& eligibility_f
         && m_ancestors <= eligibility_filter.max_ancestors
         && m_descendants <= eligibility_filter.max_descendants;
 }
-
-void OutputGroup::SetFees(const CFeeRate effective_feerate, const CFeeRate long_term_feerate)
-{
-    fee = 0;
-    long_term_fee = 0;
-    effective_value = 0;
-    for (CInputCoin& coin : m_outputs) {
-        coin.m_fee = coin.m_input_bytes < 0 ? 0 : effective_feerate.GetFee(coin.m_input_bytes);
-        fee += coin.m_fee;
-
-        coin.m_long_term_fee = coin.m_input_bytes < 0 ? 0 : long_term_feerate.GetFee(coin.m_input_bytes);
-        long_term_fee += coin.m_long_term_fee;
-
-        coin.effective_value = coin.txout.nValue - coin.m_fee;
-        effective_value += coin.effective_value;
-    }
-}
-
-OutputGroup OutputGroup::GetPositiveOnlyGroup()
-{
-    OutputGroup group(*this);
-    for (auto it = group.m_outputs.begin(); it != group.m_outputs.end(); ) {
-        const CInputCoin& coin = *it;
-        // Only include outputs that are positive effective value (i.e. not dust)
-        if (coin.effective_value <= 0) {
-            it = group.Discard(coin);
-        } else {
-            ++it;
-        }
-    }
-    return group;
-}

src/wallet/coinselection.h

@@ -6,11 +6,10 @@
 #define BITCOIN_WALLET_COINSELECTION_H
 
 #include <amount.h>
+#include <policy/feerate.h>
 #include <primitives/transaction.h>
 #include <random.h>
 
-class CFeeRate;
-
 //! target minimum change amount
 static constexpr CAmount MIN_CHANGE{COIN / 100};
 //! final minimum change amount after paying for fees
@@ -63,9 +62,11 @@ struct CoinEligibilityFilter
     const int conf_theirs;
     const uint64_t max_ancestors;
     const uint64_t max_descendants;
+    const bool m_include_partial_groups{false}; //! Include partial destination groups when avoid_reuse and there are full groups
 
     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) {}
 };
 
 struct OutputGroup
@@ -78,27 +79,18 @@ struct OutputGroup
     size_t m_descendants{0};
     CAmount effective_value{0};
     CAmount fee{0};
+    CFeeRate m_effective_feerate{0};
     CAmount long_term_fee{0};
+    CFeeRate m_long_term_feerate{0};
 
     OutputGroup() {}
-    OutputGroup(std::vector<CInputCoin>&& outputs, bool from_me, CAmount value, int depth, size_t ancestors, size_t descendants)
-    : m_outputs(std::move(outputs))
-    , m_from_me(from_me)
-    , m_value(value)
-    , m_depth(depth)
-    , m_ancestors(ancestors)
-    , m_descendants(descendants)
+    OutputGroup(const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate) :
+        m_effective_feerate(effective_feerate),
+        m_long_term_feerate(long_term_feerate)
     {}
-    OutputGroup(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants) : OutputGroup() {
-        Insert(output, depth, from_me, ancestors, descendants);
-    }
-    void Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants);
-    std::vector<CInputCoin>::iterator Discard(const CInputCoin& output);
-    bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const;
 
-    //! Update the OutputGroup's fee, long_term_fee, and effective_value based on the given feerates
-    void SetFees(const CFeeRate effective_feerate, const CFeeRate long_term_feerate);
-    OutputGroup GetPositiveOnlyGroup();
+    void Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants, bool positive_only);
+    bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const;
 };
 
 bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& target_value, const CAmount& cost_of_change, std::set<CInputCoin>& out_set, CAmount& value_ret, CAmount not_input_fees);