Remove CreateTransaction while loop and some related variables

Remove the CreateTransaction while loop. Removes variables that were
only needed because of that loop. Also renames a few variables and
moves their declarations to where they are used.

Some subtractFeeFromOutputs handling is moved to after coin selection
in order to reduce their amounts once the fee is known.

If subtracting the fee reduces the change to dust, we will also now
remove the change output

1 files changed, 104 insertions, 117 deletions

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index c1e58a949f..303823695e 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2804,7 +2804,6 @@ bool CWallet::CreateTransactionInternal(
     CAmount nValue = 0;
     const OutputType change_type = TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : m_default_change_type, vecSend);
     ReserveDestination reservedest(this, change_type);
-    int nChangePosRequest = nChangePosInOut;
     unsigned int nSubtractFeeFromAmount = 0;
     for (const auto& recipient : vecSend)
     {
@@ -2909,151 +2908,139 @@ bool CWallet::CreateTransactionInternal(
             coin_selection_params.m_change_fee = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size);
             coin_selection_params.m_cost_of_change = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size) + coin_selection_params.m_change_fee;
 
-            nFeeRet = 0;
-            CAmount nValueIn = 0;
-
             coin_selection_params.m_subtract_fee_outputs = nSubtractFeeFromAmount != 0; // If we are doing subtract fee from recipient, don't use effective values
-            // Start with no fee and loop until there is enough fee
-            while (true)
+
+            // 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)
+            }
+            for (const auto& recipient : vecSend)
             {
-                nChangePosInOut = nChangePosRequest;
-                txNew.vin.clear();
-                txNew.vout.clear();
+                CTxOut txout(recipient.nAmount, recipient.scriptPubKey);
 
-                // vouts to the payees
+                // Include the fee cost for outputs.
                 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)
+                    coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION);
                 }
-                for (const auto& recipient : vecSend)
+
+                if (IsDust(txout, chain().relayDustFee()))
                 {
-                    CTxOut txout(recipient.nAmount, recipient.scriptPubKey);
+                    error = _("Transaction amount too small");
+                    return false;
+                }
+                txNew.vout.push_back(txout);
+            }
 
-                    // Include the fee cost for outputs.
-                    if (!coin_selection_params.m_subtract_fee_outputs) {
-                        coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION);
-                    }
+            // 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;
 
-                    if (IsDust(txout, chain().relayDustFee()))
-                    {
-                        error = _("Transaction amount too small");
-                        return false;
-                    }
-                    txNew.vout.push_back(txout);
-                }
+            // Choose coins to use
+            CAmount inputs_sum = 0;
+            setCoins.clear();
+            if (!SelectCoins(vAvailableCoins, /* nTargetValue */ nValueToSelect, setCoins, inputs_sum, coin_control, coin_selection_params))
+            {
+                error = _("Insufficient funds");
+                return false;
+            }
 
-                // 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;
+            // Always make a change output
+            // We will reduce the fee from this change output later, and remove the output if it is too small.
+            const CAmount change_and_fee = inputs_sum - nValue;
+            assert(change_and_fee >= 0);
+            CTxOut newTxOut(change_and_fee, scriptChange);
 
-                // Choose coins to use
-                nValueIn = 0;
-                setCoins.clear();
-                if (!SelectCoins(vAvailableCoins, /* nTargetValue */ nValueToSelect, setCoins, nValueIn, coin_control, coin_selection_params))
-                {
-                    error = _("Insufficient funds");
-                    return false;
-                }
+            if (nChangePosInOut == -1)
+            {
+                // Insert change txn at random position:
+                nChangePosInOut = GetRandInt(txNew.vout.size()+1);
+            }
+            else if ((unsigned int)nChangePosInOut > txNew.vout.size())
+            {
+                error = _("Change index out of range");
+                return false;
+            }
 
-                // Always make a change output
-                // We will reduce the fee from this change output later, and remove the output if it is too small.
-                const CAmount change_and_fee = nValueIn - nValue;
-                assert(change_and_fee >= 0);
-                CTxOut newTxOut(change_and_fee, scriptChange);
+            assert(nChangePosInOut != -1);
+            auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut);
 
-                if (nChangePosInOut == -1)
-                {
-                    // Insert change txn at random position:
-                    nChangePosInOut = GetRandInt(txNew.vout.size()+1);
-                }
-                else if ((unsigned int)nChangePosInOut > txNew.vout.size())
-                {
-                    error = _("Change index out of range");
-                    return false;
-                }
+            // Dummy fill vin for maximum size estimation
+            //
+            for (const auto& coin : setCoins) {
+                txNew.vin.push_back(CTxIn(coin.outpoint,CScript()));
+            }
 
-                assert(nChangePosInOut != -1);
-                auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut);
+            // Calculate the transaction fee
+            tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly);
+            nBytes = tx_sizes.first;
+            if (nBytes < 0) {
+                error = _("Signing transaction failed");
+                return false;
+            }
+            nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes);
 
-                // Dummy fill vin for maximum size estimation
-                //
-                for (const auto& coin : setCoins) {
-                    txNew.vin.push_back(CTxIn(coin.outpoint,CScript()));
-                }
+            // Subtract fee from the change output if not subtrating it from recipient outputs
+            CAmount fee_needed = nFeeRet;
+            if (nSubtractFeeFromAmount == 0) {
+                change_position->nValue -= fee_needed;
+            }
 
-                // Calculate the transaction fee
+            // We want to drop the change to fees if:
+            // 1. The change output would be dust
+            // 2. The change is within the (almost) exact match window, i.e. it is less than or equal to the cost of the change output (cost_of_change)
+            CAmount change_amount = change_position->nValue;
+            if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change)
+            {
+                nChangePosInOut = -1;
+                change_amount = 0;
+                txNew.vout.erase(change_position);
+
+                // 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.first;
-                if (nBytes < 0) {
-                    error = _("Signing transaction failed");
-                    return false;
-                }
-                nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes);
+                fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes);
+            }
 
-                // Subtract fee from the change output if not subtrating it from recipient outputs
-                CAmount fee_needed = nFeeRet;
-                if (nSubtractFeeFromAmount == 0) {
-                    change_position->nValue -= fee_needed;
-                }
+            // 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;
+            }
 
-                // We want to drop the change to fees if:
-                // 1. The change output would be dust
-                // 2. The change is within the (almost) exact match window, i.e. it is less than or equal to the cost of the change output (cost_of_change)
-                CAmount change_amount = change_position->nValue;
-                if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change)
+            // Reduce output values for subtractFeeFromAmount
+            if (nSubtractFeeFromAmount != 0) {
+                CAmount to_reduce = fee_needed + change_amount - change_and_fee;
+                int i = 0;
+                bool fFirst = true;
+                for (const auto& recipient : vecSend)
                 {
-                    nChangePosInOut = -1;
-                    change_amount = 0;
-                    txNew.vout.erase(change_position);
-
-                    // 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.first;
-                    fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes);
-                }
-
-                // If the fee is covered, there's no need to loop or subtract from recipients
-                if (fee_needed <= change_and_fee - change_amount) {
-                    nFeeRet = change_and_fee - change_amount;
-                    break;
-                }
+                    if (i == nChangePosInOut) {
+                        ++i;
+                    }
+                    CTxOut& txout = txNew.vout[i];
 
-                // Reduce output values for subtractFeeFromAmount
-                if (nSubtractFeeFromAmount != 0) {
-                    CAmount to_reduce = fee_needed + change_amount - change_and_fee;
-                    int i = 0;
-                    bool fFirst = true;
-                    for (const auto& recipient : vecSend)
+                    if (recipient.fSubtractFeeFromAmount)
                     {
-                        if (i == nChangePosInOut) {
-                            ++i;
-                        }
-                        CTxOut& txout = txNew.vout[i];
+                        txout.nValue -= to_reduce / nSubtractFeeFromAmount; // Subtract fee equally from each selected recipient
 
-                        if (recipient.fSubtractFeeFromAmount)
+                        if (fFirst) // first receiver pays the remainder not divisible by output count
                         {
-                            txout.nValue -= to_reduce / nSubtractFeeFromAmount; // Subtract fee equally from each selected recipient
-
-                            if (fFirst) // first receiver pays the remainder not divisible by output count
-                            {
-                                fFirst = false;
-                                txout.nValue -= to_reduce % nSubtractFeeFromAmount;
-                            }
+                            fFirst = false;
+                            txout.nValue -= to_reduce % nSubtractFeeFromAmount;
+                        }
 
-                            // Error if this output is reduced to be below dust
-                            if (IsDust(txout, chain().relayDustFee())) {
-                                if (txout.nValue < 0) {
-                                    error = _("The transaction amount is too small to pay the fee");
-                                } else {
-                                    error = _("The transaction amount is too small to send after the fee has been deducted");
-                                }
-                                return false;
+                        // Error if this output is reduced to be below dust
+                        if (IsDust(txout, chain().relayDustFee())) {
+                            if (txout.nValue < 0) {
+                                error = _("The transaction amount is too small to pay the fee");
+                            } else {
+                                error = _("The transaction amount is too small to send after the fee has been deducted");
                             }
+                            return false;
                         }
-                        ++i;
                     }
-                    nFeeRet = fee_needed;
-                    break; // The fee has been deducted from the recipients, nothing left to do here
+                    ++i;
                 }
+                nFeeRet = fee_needed;
             }
 
             // Give up if change keypool ran out and change is required