Refactor OutputGroups to handle effective values, fees, and filtering

Summary:
Instead of having callers set the fees, effective values, and filtering
of outputs, do these within OutputGroups themselves as member functions.

m_fee and m_long_term_fee is added to OutputGroup to track the fees of
the OutputGroup.

This is a backport of [[bitcoin/bitcoin#17458 | core#17458]] [2/2]
bitcoin/bitcoin@9adc2f8

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Subscribers: Detetivepro1

Differential Revision: https://reviews.bitcoinabc.org/D10089

src/wallet/coinselection.cpp

@@ -4,6 +4,7 @@
 
 #include <wallet/coinselection.h>
 
+#include <feerate.h>
 #include <util/moneystr.h>
 #include <util/system.h>
 
@@ -360,7 +361,9 @@ void OutputGroup::Insert(const CInputCoin &output, int depth, bool from_me,
     // 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 = m_value;
+    effective_value += output.effective_value;
+    fee += output.m_fee;
+    long_term_fee += output.m_long_term_fee;
 }
 
 std::vector<CInputCoin>::iterator
@@ -374,6 +377,8 @@ OutputGroup::Discard(const CInputCoin &output) {
     }
     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);
 }
 
@@ -384,3 +389,40 @@ bool OutputGroup::EligibleForSpending(
            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 = Amount::zero();
+    long_term_fee = Amount::zero();
+    effective_value = Amount::zero();
+    for (CInputCoin &coin : m_outputs) {
+        coin.m_fee = coin.m_input_bytes < 0
+                         ? Amount::zero()
+                         : effective_feerate.GetFee(coin.m_input_bytes);
+        fee += coin.m_fee;
+
+        coin.m_long_term_fee =
+            coin.m_input_bytes < 0
+                ? Amount::zero()
+                : 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 <= Amount::zero()) {
+            it = group.Discard(coin);
+        } else {
+            ++it;
+        }
+    }
+    return group;
+}

src/wallet/coinselection.h

@@ -9,6 +9,8 @@
 #include <primitives/transaction.h>
 #include <random.h>
 
+class CFeeRate;
+
 //! target minimum change amount
 static constexpr Amount MIN_CHANGE{COIN / 100};
 //! final minimum change amount after paying for fees
@@ -37,6 +39,8 @@ class CInputCoin {
     COutPoint outpoint;
     CTxOut txout;
     Amount effective_value;
+    Amount m_fee{Amount::zero()};
+    Amount m_long_term_fee{Amount::zero()};
 
     /**
      * Pre-computed estimated size of this output as a fully-signed input in a
@@ -99,6 +103,12 @@ struct OutputGroup {
     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();
 };
 
 bool SelectCoinsBnB(std::vector<OutputGroup> &utxo_pool,

src/wallet/wallet.cpp

@@ -2597,42 +2597,20 @@ bool CWallet::SelectCoinsMinConf(
                 continue;
             }
 
-            group.fee = Amount::zero();
-            group.long_term_fee = Amount::zero();
-            group.effective_value = Amount::zero();
-            for (auto it = group.m_outputs.begin();
-                 it != group.m_outputs.end();) {
-                const CInputCoin &coin = *it;
-                Amount effective_value =
-                    coin.txout.nValue -
-                    (coin.m_input_bytes < 0
-                         ? Amount::zero()
-                         : coin_selection_params.effective_fee.GetFee(
-                               coin.m_input_bytes));
-                // Only include outputs that are positive effective value (i.e.
-                // not dust)
-                if (effective_value > Amount::zero()) {
-                    group.fee +=
-                        coin.m_input_bytes < 0
-                            ? Amount::zero()
-                            : coin_selection_params.effective_fee.GetFee(
-                                  coin.m_input_bytes);
-                    group.long_term_fee +=
-                        coin.m_input_bytes < 0
-                            ? Amount::zero()
-                            : long_term_feerate.GetFee(coin.m_input_bytes);
-                    if (coin_selection_params.m_subtract_fee_outputs) {
-                        group.effective_value += coin.txout.nValue;
-                    } else {
-                        group.effective_value += effective_value;
-                    }
-                    ++it;
-                } else {
-                    it = group.Discard(coin);
-                }
+            if (coin_selection_params.m_subtract_fee_outputs) {
+                // Set the effective feerate to 0 as we don't want to use the
+                // effective value since the fees will be deducted from the
+                // output
+                group.SetFees(CFeeRate(Amount::zero()) /* effective_feerate */,
+                              long_term_feerate);
+            } else {
+                group.SetFees(coin_selection_params.effective_fee,
+                              long_term_feerate);
             }
-            if (group.effective_value > Amount::zero()) {
-                utxo_pool.push_back(group);
+
+            OutputGroup pos_group = group.GetPositiveOnlyGroup();
+            if (pos_group.effective_value > Amount::zero()) {
+                utxo_pool.push_back(pos_group);
             }
         }
         // Calculate the fees for things that aren't inputs