refactor: Remove CreateTransactionInternal fee_needed variable

ryanofsky · ryanofsky · commit 0cc10f837749 · 2021-06-12T11:22:41.000-04:00
This commit does not change behavior in any way. It just removes a variable and adds a comment to explain nFeeRet return value better. The fee_needed variable became redundant after commit 9d3bd74 removed looping in CreateTransactionInternal. This change was originally posted #22008 (comment) and discussed in the thread of that PR and in IRC. It might be followed up with more changes later to alter subtract from recipient behavior, but this commit just cleans up and documents the existing code.
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
@@ -756,9 +756,8 @@ bool CWallet::CreateTransactionInternal(
     nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes);
 
     // Subtract fee from the change output if not subtracting it from recipient outputs
-    CAmount fee_needed = nFeeRet;
     if (!coin_selection_params.m_subtract_fee_outputs) {
-        change_position->nValue -= fee_needed;
+        change_position->nValue -= nFeeRet;
     }
 
     // We want to drop the change to fees if:
@@ -774,17 +773,12 @@ bool CWallet::CreateTransactionInternal(
         // Because we have dropped this change, the tx size and required fee will be different, so let's recalculate those
         tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly);
         nBytes = tx_sizes.vsize;
-        fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes);
-    }
-
-    // Update nFeeRet in case fee_needed changed due to dropping the change output
-    if (fee_needed <= change_and_fee - change_amount) {
-        nFeeRet = change_and_fee - change_amount;
+        nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes);
     }
 
     // Reduce output values for subtractFeeFromAmount
     if (coin_selection_params.m_subtract_fee_outputs) {
-        CAmount to_reduce = fee_needed + change_amount - change_and_fee;
+        CAmount to_reduce = nFeeRet + change_amount - change_and_fee;
         int i = 0;
         bool fFirst = true;
         for (const auto& recipient : vecSend)
@@ -816,7 +810,15 @@ bool CWallet::CreateTransactionInternal(
             }
             ++i;
         }
-        nFeeRet = fee_needed;
+    } else if (nFeeRet <= change_and_fee - change_amount) {
+        // If dropping the change output covered the fee, update the returned
+        // fee amount. Note that that in subtract-fee-from-recipients case
+        // above, if the change output is dropped, the change dust value will
+        // be paid to recipients, rather than to the miner (`to_reduce` above
+        // will be negative). In that case, the dust amount sent is an
+        // additional cost of the transaction, but it's not considered part of
+        // the fee since it isn't paid to the miner.
+        nFeeRet = change_and_fee - change_amount;
     }
 
     // Give up if change keypool ran out and change is required
