From bf26e018de33216d6f0ed0d6ff822b93536f7cc1 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 30 Oct 2019 18:41:13 -0400 Subject: Roll static tx fees into nValueToSelect instead of having it be separate The fees for transaction overhead and recipient outputs are now included in nTargetValue instead of being a separate parameter. For the coin selection algorithms, it doesn't matter that these are separate as in either case, the algorithm needs to select enough to cover these fees. Note that setting nValueToSelect is changed as it now includes not_input_fees. Without the change to how nValueToSelect is increased for KnapsackSolver, this would result in overpaying fees. The change to increase by the difference between nFeeRet and not_input_fees allows this to have the same behavior as previously. Additionally, because we assume that KnapsackSolver will always find a solution that requires change (we assume that BnB always finds a non-change solution), we also include the fee for the change output in KnapsackSolver's target. As part of this, we also use the changeless nFeeRet when iterating for KnapsackSolver. This is because we include the change fee when doing KnapsackSolver, so nFeeRet on further iterations won't include the change fee. --- src/wallet/wallet.cpp | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) (limited to 'src/wallet/wallet.cpp') diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 51ff683c2e..3383b3e96b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2399,9 +2399,6 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil setCoinsRet.clear(); nValueRet = 0; - // Calculate the fees for things that aren't inputs, excluding the change output - const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); - // Get the feerate for effective value. // When subtracting the fee from the outputs, we want the effective feerate to be 0 CFeeRate effective_feerate{0}; @@ -2412,14 +2409,17 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil if (coin_selection_params.use_bnb) { std::vector positive_groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, effective_feerate, coin_selection_params.m_long_term_feerate, eligibility_filter, true /* positive_only */); bnb_used = true; - return SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change, setCoinsRet, nValueRet, not_input_fees); + // Note that unlike KnapsackSolver, we do not include the fee for creating a change output as BnB will not create a change output. + 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 groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, CFeeRate(0), CFeeRate(0), eligibility_filter, false /* positive_only */); bnb_used = false; - return KnapsackSolver(nTargetValue, groups, setCoinsRet, nValueRet); + // 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); } } @@ -2931,10 +2931,6 @@ bool CWallet::CreateTransactionInternal( txNew.vin.clear(); txNew.vout.clear(); - CAmount nValueToSelect = nValue; - if (nSubtractFeeFromAmount == 0) - nValueToSelect += nFeeRet; - // vouts to the payees if (!coin_selection_params.m_subtract_fee_outputs) { coin_selection_params.tx_noinputs_size = 11; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 output count, 1 witness overhead (dummy, flag, stack size) @@ -2956,11 +2952,21 @@ bool CWallet::CreateTransactionInternal( txNew.vout.push_back(txout); } + // Include the fees for things that aren't inputs, excluding the change output + 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; setCoins.clear(); - if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coin_control, coin_selection_params, bnb_used)) + if (!SelectCoins(vAvailableCoins, /* nTargetValue */ nValueToSelect, setCoins, nValueIn, coin_control, coin_selection_params, bnb_used)) { // If BnB was used, it was the first pass. No longer the first pass and continue loop with knapsack. if (bnb_used) { -- cgit v1.2.3