Move cs_wallet lock in CreateTransactionInternal to top of function

It isn't necessary to not lock parts of this function. Just lock the
whole thing and get rid of an indent.

1 files changed, 185 insertions, 184 deletions

diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 97fc7acca5..00edc94403 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -578,6 +578,8 @@ bool CWallet::CreateTransactionInternal(
         FeeCalculation& fee_calc_out,
         bool sign)
 {
+    AssertLockHeld(cs_wallet);
+
     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);
@@ -606,224 +608,221 @@ bool CWallet::CreateTransactionInternal(
     int nBytes;
     {
         std::set<CInputCoin> setCoins;
-        LOCK(cs_wallet);
         txNew.nLockTime = GetLocktimeForNewTransaction(chain(), GetLastBlockHash(), GetLastBlockHeight());
-        {
-            std::vector<COutput> vAvailableCoins;
-            AvailableCoins(vAvailableCoins, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0);
-            CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy
-            coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;
-
-            // Create change script that will be used if we need change
-            // TODO: pass in scriptChange instead of reservedest so
-            // change transaction isn't always pay-to-bitcoin-address
-            CScript scriptChange;
-
-            // coin control: send change to custom address
-            if (!std::get_if<CNoDestination>(&coin_control.destChange)) {
-                scriptChange = GetScriptForDestination(coin_control.destChange);
-            } else { // no coin control: send change to newly generated address
-                // Note: We use a new key here to keep it from being obvious which side is the change.
-                //  The drawback is that by not reusing a previous key, the change may be lost if a
-                //  backup is restored, if the backup doesn't have the new private key for the change.
-                //  If we reused the old key, it would be possible to add code to look for and
-                //  rediscover unknown transactions that were written with keys of ours to recover
-                //  post-backup change.
-
-                // Reserve a new key pair from key pool. If it fails, provide a dummy
-                // destination in case we don't need change.
-                CTxDestination dest;
-                if (!reservedest.GetReservedDestination(dest, true)) {
-                    error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.");
-                }
-                scriptChange = GetScriptForDestination(dest);
-                // A valid destination implies a change script (and
-                // vice-versa). An empty change script will abort later, if the
-                // change keypool ran out, but change is required.
-                CHECK_NONFATAL(IsValidDestination(dest) != scriptChange.empty());
-            }
-            CTxOut change_prototype_txout(0, scriptChange);
-            coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout);
-
-            // Get size of spending the change output
-            int change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this);
-            // If the wallet doesn't know how to sign change output, assume p2sh-p2wpkh
-            // as lower-bound to allow BnB to do it's thing
-            if (change_spend_size == -1) {
-                coin_selection_params.change_spend_size = DUMMY_NESTED_P2WPKH_INPUT_SIZE;
-            } else {
-                coin_selection_params.change_spend_size = (size_t)change_spend_size;
-            }
+        std::vector<COutput> vAvailableCoins;
+        AvailableCoins(vAvailableCoins, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0);
+        CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy
+        coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;
+
+        // Create change script that will be used if we need change
+        // TODO: pass in scriptChange instead of reservedest so
+        // change transaction isn't always pay-to-bitcoin-address
+        CScript scriptChange;
+
+        // coin control: send change to custom address
+        if (!std::get_if<CNoDestination>(&coin_control.destChange)) {
+            scriptChange = GetScriptForDestination(coin_control.destChange);
+        } else { // no coin control: send change to newly generated address
+            // Note: We use a new key here to keep it from being obvious which side is the change.
+            //  The drawback is that by not reusing a previous key, the change may be lost if a
+            //  backup is restored, if the backup doesn't have the new private key for the change.
+            //  If we reused the old key, it would be possible to add code to look for and
+            //  rediscover unknown transactions that were written with keys of ours to recover
+            //  post-backup change.
+
+            // Reserve a new key pair from key pool. If it fails, provide a dummy
+            // destination in case we don't need change.
+            CTxDestination dest;
+            if (!reservedest.GetReservedDestination(dest, true)) {
+                error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.");
+            }
+            scriptChange = GetScriptForDestination(dest);
+            // A valid destination implies a change script (and
+            // vice-versa). An empty change script will abort later, if the
+            // change keypool ran out, but change is required.
+            CHECK_NONFATAL(IsValidDestination(dest) != scriptChange.empty());
+        }
+        CTxOut change_prototype_txout(0, scriptChange);
+        coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout);
+
+        // Get size of spending the change output
+        int change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this);
+        // If the wallet doesn't know how to sign change output, assume p2sh-p2wpkh
+        // as lower-bound to allow BnB to do it's thing
+        if (change_spend_size == -1) {
+            coin_selection_params.change_spend_size = DUMMY_NESTED_P2WPKH_INPUT_SIZE;
+        } else {
+            coin_selection_params.change_spend_size = (size_t)change_spend_size;
+        }
 
-            // Set discard feerate
-            coin_selection_params.m_discard_feerate = GetDiscardRate(*this);
+        // Set discard feerate
+        coin_selection_params.m_discard_feerate = GetDiscardRate(*this);
 
-            // Get the fee rate to use effective values in coin selection
-            coin_selection_params.m_effective_feerate = GetMinimumFeeRate(*this, coin_control, &feeCalc);
-            // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly
-            // provided one
-            if (coin_control.m_feerate && coin_selection_params.m_effective_feerate > *coin_control.m_feerate) {
-                error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::SAT_VB));
-                return false;
-            }
-            if (feeCalc.reason == FeeReason::FALLBACK && !m_allow_fallback_fee) {
-                // eventually allow a fallback fee
-                error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee.");
-                return false;
-            }
+        // Get the fee rate to use effective values in coin selection
+        coin_selection_params.m_effective_feerate = GetMinimumFeeRate(*this, coin_control, &feeCalc);
+        // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly
+        // provided one
+        if (coin_control.m_feerate && coin_selection_params.m_effective_feerate > *coin_control.m_feerate) {
+            error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::SAT_VB));
+            return false;
+        }
+        if (feeCalc.reason == FeeReason::FALLBACK && !m_allow_fallback_fee) {
+            // eventually allow a fallback fee
+            error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee.");
+            return false;
+        }
 
-            // Get long term estimate
-            CCoinControl cc_temp;
-            cc_temp.m_confirm_target = chain().estimateMaxBlocks();
-            coin_selection_params.m_long_term_feerate = GetMinimumFeeRate(*this, cc_temp, nullptr);
+        // Get long term estimate
+        CCoinControl cc_temp;
+        cc_temp.m_confirm_target = chain().estimateMaxBlocks();
+        coin_selection_params.m_long_term_feerate = GetMinimumFeeRate(*this, cc_temp, nullptr);
 
-            // Calculate the cost of change
-            // Cost of change is the cost of creating the change output + cost of spending the change output in the future.
-            // For creating the change output now, we use the effective feerate.
-            // For spending the change output in the future, we use the discard feerate for now.
-            // So cost of change = (change output size * effective feerate) + (size of spending change output * discard feerate)
-            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;
+        // Calculate the cost of change
+        // Cost of change is the cost of creating the change output + cost of spending the change output in the future.
+        // For creating the change output now, we use the effective feerate.
+        // For spending the change output in the future, we use the discard feerate for now.
+        // So cost of change = (change output size * effective feerate) + (size of spending change output * discard feerate)
+        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;
 
-            coin_selection_params.m_subtract_fee_outputs = nSubtractFeeFromAmount != 0; // If we are doing subtract fee from recipient, don't use effective values
+        coin_selection_params.m_subtract_fee_outputs = nSubtractFeeFromAmount != 0; // If we are doing subtract fee from recipient, don't use effective values
 
-            // vouts to the payees
+        // 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)
+        {
+            CTxOut txout(recipient.nAmount, recipient.scriptPubKey);
+
+            // 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
-            CAmount inputs_sum = 0;
-            setCoins.clear();
-            if (!SelectCoins(vAvailableCoins, /* nTargetValue */ nValueToSelect, setCoins, inputs_sum, 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 = inputs_sum - 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.vsize;
+        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.vsize;
-            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.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;
-            }
+                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;
+                ++i;
             }
+            nFeeRet = fee_needed;
+        }
 
-            // Give up if change keypool ran out and change is required
-            if (scriptChange.empty() && nChangePosInOut != -1) {
-                return false;
-            }
+        // Give up if change keypool ran out and change is required
+        if (scriptChange.empty() && nChangePosInOut != -1) {
+            return false;
         }
 
         // Shuffle selected coins and fill in final vin
@@ -900,6 +899,8 @@ bool CWallet::CreateTransaction(
         FeeCalculation& fee_calc_out,
         bool sign)
 {
+    LOCK(cs_wallet);
+
     int nChangePosIn = nChangePosInOut;
     Assert(!tx); // tx is an out-param. TODO change the return type from bool to tx (or nullptr)
     bool res = CreateTransactionInternal(vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, fee_calc_out, sign);