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

5d4597666d589e39354e0d8dd5b2afbe1a5d7d8e Rewrite OutputGroups to be clearer and to use scriptPubKeys (Andrew Chow)
f6b305273910db0e46798d361413a7e878cb45f7 Explicitly filter out partial groups when we don't want them (Andrew Chow)
416d74fb1687ae1d47a58c153d09d9afe0b6dc60 Move OutputGroup positive only filtering into Insert (Andrew Chow)
d895e98b594b873f3d34c8ba63e9b55125d51b5a Move EligibleForSpending into GroupOutputs (Andrew Chow)
99b399aba5d27476b61b4865cc39553d03965d57 Move fee setting of OutputGroup to Insert (Andrew Chow)
6148a8acda5e594bb9b3b2d989056f9e03ddbdbd Move GroupOutputs into SelectCoinsMinConf (Andrew Chow)
2acad036575ec998f8bbe4f10f6206b1c8ad3d23 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: https://github.com/bitcoin/bitcoin/pull/20040/commits/5d4597666d589e39354e0d8dd5b2afbe1a5d7d8e
  fjahr:
    Code review ACK 5d4597666d589e39354e0d8dd5b2afbe1a5d7d8e
  meshcollider:
    Light utACK 5d4597666d589e39354e0d8dd5b2afbe1a5d7d8e

Tree-SHA512: 35965b6d49a87f4ebb366ec4f00aafaaf78e9282481ae2c9682b515a3a9f2cbcd3cd6e202fee29489d48fe7f3a7cede4270796f5e72bbaff76da647138fb3059

1 files changed, 19 insertions, 47 deletions

diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp
index a2dea84d17..10f89e3a6f 100644
--- a/src/wallet/coinselection.cpp
+++ b/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;
-}