Have KnapsackSolver actually use effective values

Although the CreateTransaction loop currently remains, it should be
largely unused. KnapsackSolver will now account for transaction fees
when doing its selection.

In the previous commit, SelectCoinsMinConf was refactored to have some
calculations become shared for KnapsackSolver and SelectCoinsBnB. In
this commit, KnapsackSolver will now use the not_input_fees and
effective_feerate so that it include the fee for non-input things
(excluding a change output) so that the algorithm will select enough to
cover those fees. This is necessary for selecting on effective values.

Additionally, the OutputGroups
created for KnapsackSolver will actually have their effective values
calculated and set, and KnapsackSolver will do its selection on those
effective values.

Lastly, SelectCoins is modified to use the same value for preselected
inputs for BnB and KnapsackSolver. While it will still use the real
value when subtracting the fee from outputs, this behavior will be
the same regardless of the algo used for selecting additional inputs.

2 files changed, 10 insertions, 18 deletions

diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp
index db770f6a45..af88bf85dd 100644
--- a/src/wallet/coinselection.cpp
+++ b/src/wallet/coinselection.cpp
@@ -227,14 +227,14 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& group
     Shuffle(groups.begin(), groups.end(), FastRandomContext());
 
     for (const OutputGroup& group : groups) {
-        if (group.m_value == nTargetValue) {
+        if (group.effective_value == nTargetValue) {
             util::insert(setCoinsRet, group.m_outputs);
             nValueRet += group.m_value;
             return true;
-        } else if (group.m_value < nTargetValue + MIN_CHANGE) {
+        } else if (group.effective_value < nTargetValue + MIN_CHANGE) {
             applicable_groups.push_back(group);
-            nTotalLower += group.m_value;
-        } else if (!lowest_larger || group.m_value < lowest_larger->m_value) {
+            nTotalLower += group.effective_value;
+        } else if (!lowest_larger || group.effective_value < lowest_larger->effective_value) {
             lowest_larger = group;
         }
     }
@@ -267,7 +267,7 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& group
     // If we have a bigger coin and (either the stochastic approximation didn't find a good solution,
     //                                   or the next bigger coin is closer), return the bigger coin
     if (lowest_larger &&
-        ((nBest != nTargetValue && nBest < nTargetValue + MIN_CHANGE) || lowest_larger->m_value <= nBest)) {
+        ((nBest != nTargetValue && nBest < nTargetValue + MIN_CHANGE) || lowest_larger->effective_value <= nBest)) {
         util::insert(setCoinsRet, lowest_larger->m_outputs);
         nValueRet += lowest_larger->m_value;
     } else {
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 3383b3e96b..b229d8ad63 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2413,13 +2413,11 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil
         return SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change, setCoinsRet, nValueRet);
     } else {
         // The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here.
-        // The knapsack solver currently does not use effective values, so we give GroupOutputs feerates of 0 so it sets the effective values to be the same as the real value.
-        std::vector<OutputGroup> groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, CFeeRate(0), CFeeRate(0), eligibility_filter, false /* positive_only */);
-
+        std::vector<OutputGroup> all_groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, effective_feerate, coin_selection_params.m_long_term_feerate, eligibility_filter, false /* positive_only */);
         bnb_used = false;
         // While nTargetValue includes the transaction fees for non-input things, it does not include the fee for creating a change output.
         // So we need to include that for KnapsackSolver as well, as we are expecting to create a change output.
-        return KnapsackSolver(nTargetValue + coin_selection_params.m_change_fee, groups, setCoinsRet, nValueRet);
+        return KnapsackSolver(nTargetValue + coin_selection_params.m_change_fee, all_groups, setCoinsRet, nValueRet);
     }
 }
 
@@ -2467,10 +2465,10 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
                 return false; // Not solvable, can't estimate size for fee
             }
             coin.effective_value = coin.txout.nValue - coin_selection_params.m_effective_feerate.GetFee(coin.m_input_bytes);
-            if (coin_selection_params.use_bnb) {
-                value_to_select -= coin.effective_value;
-            } else {
+            if (coin_selection_params.m_subtract_fee_outputs) {
                 value_to_select -= coin.txout.nValue;
+            } else {
+                value_to_select -= coin.effective_value;
             }
             setPresetCoins.insert(coin);
         } else {
@@ -2956,12 +2954,6 @@ bool CWallet::CreateTransactionInternal(
                 const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size);
                 CAmount nValueToSelect = nValue + not_input_fees;
 
-                // For KnapsackSolver, when we are not subtracting the fee from the recipients, we also want to include the fees for the
-                // inputs that we found in the previous iteration.
-                if (!coin_selection_params.use_bnb && nSubtractFeeFromAmount == 0) {
-                    nValueToSelect += std::max(CAmount(0), nFeeRet - not_input_fees);
-                }
-
                 // Choose coins to use
                 bool bnb_used = false;
                 nValueIn = 0;