Merge #11864: Make CWallet::FundTransaction atomic

03a5dc9 [wallet] Make CWallet::FundTransaction atomic (João Barbosa)
95d4450 [wallet] Tidy up CWallet::FundTransaction (João Barbosa)

Pull request description:

  This PR fixes a race for `setLockedCoins` when `lockUnspents` is true. For instance, it should not be possible to use the same unspent in concurrent `fundrawtransaction` calls.

  Now the `cs_main` and `cs_wallet` locks are held during `CreateTransaction` and `LockCoin`(s). Also added some style nits around the change.

Tree-SHA512: ccf383c0c5f6db775655a3e9ccd200c3bd831a83afae2b7c389564c74f7227f5bea86a4775727de2c3603b188f383f8a12d3f9d6d94f7887865c31c94ce95ef6

src/wallet/wallet.cpp

@@ -2595,9 +2595,8 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
 {
     std::vector<CRecipient> vecSend;
 
-    // Turn the txout set into a CRecipient vector
+    // Turn the txout set into a CRecipient vector.
-    for (size_t idx = 0; idx < tx.vout.size(); idx++)
+    for (size_t idx = 0; idx < tx.vout.size(); idx++) {
-    {
         const CTxOut& txOut = tx.vout[idx];
         CRecipient recipient = {txOut.scriptPubKey, txOut.nValue, setSubtractFeeFromOutputs.count(idx) == 1};
         vecSend.push_back(recipient);
@@ -2605,8 +2604,13 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
 
     coinControl.fAllowOtherInputs = true;
 
-    for (const CTxIn& txin : tx.vin)
+    for (const CTxIn& txin : tx.vin) {
         coinControl.Select(txin.prevout);
+    }
+
+    // Acquire the locks to prevent races to the new locked unspents between the
+    // CreateTransaction call and LockCoin calls (when lockUnspents is true).
+    LOCK2(cs_main, cs_wallet);
 
     CReserveKey reservekey(this);
     CWalletTx wtx;
@@ -2616,31 +2620,28 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
 
     if (nChangePosInOut != -1) {
         tx.vout.insert(tx.vout.begin() + nChangePosInOut, wtx.tx->vout[nChangePosInOut]);
-        // we don't have the normal Create/Commit cycle, and don't want to risk reusing change,
+        // We don't have the normal Create/Commit cycle, and don't want to risk
-        // so just remove the key from the keypool here.
+        // reusing change, so just remove the key from the keypool here.
         reservekey.KeepKey();
     }
 
-    // Copy output sizes from new transaction; they may have had the fee subtracted from them
+    // Copy output sizes from new transaction; they may have had the fee
-    for (unsigned int idx = 0; idx < tx.vout.size(); idx++)
+    // subtracted from them.
+    for (unsigned int idx = 0; idx < tx.vout.size(); idx++) {
         tx.vout[idx].nValue = wtx.tx->vout[idx].nValue;
+    }
 
-    // Add new txins (keeping original txin scriptSig/order)
+    // Add new txins while keeping original txin scriptSig/order.
-    for (const CTxIn& txin : wtx.tx->vin)
+    for (const CTxIn& txin : wtx.tx->vin) {
-    {
+        if (!coinControl.IsSelected(txin.prevout)) {
-        if (!coinControl.IsSelected(txin.prevout))
-        {
             tx.vin.push_back(txin);
 
-            if (lockUnspents)
+            if (lockUnspents) {
-            {
-              LOCK2(cs_main, cs_wallet);
                 LockCoin(txin.prevout);
             }
         }
     }
 
-
     return true;
 }