Fix locking of funds for mixing (#3194)

* Fix locking of funds for mixing

Lock funds earlier and actually lock mixing collaterals

* Streamline the locking logic in PrepareDenominate

Author: UdjinM6

src/privatesend/privatesend-client.cpp

@@ -450,15 +450,6 @@ bool CPrivateSendClientSession::SendDenominate(const std::vector<std::pair<CTxDS
         return false;
     }
 
-    // lock the funds we're going to use
-    for (const auto& txin : txMyCollateral.vin) {
-        vecOutPointLocked.push_back(txin.prevout);
-    }
-
-    for (const auto& pair : vecPSInOutPairsIn) {
-        vecOutPointLocked.push_back(pair.first.prevout);
-    }
-
     // we should already be connected to a Masternode
     if (!nSessionID) {
         LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::SendDenominate -- No Masternode has been selected yet.\n");
@@ -922,6 +913,11 @@ bool CPrivateSendClientSession::DoAutomaticDenominating(CConnman& connman, bool
                 }
             }
         }
+        // lock the funds we're going to use for our collateral
+        for (const auto& txin : txMyCollateral.vin) {
+            vpwallets[0]->LockCoin(txin.prevout);
+            vecOutPointLocked.push_back(txin.prevout);
+        }
     } // LOCK2(cs_main, vpwallets[0]->cs_wallet);
 
     // Always attempt to join an existing queue
@@ -1291,16 +1287,15 @@ bool CPrivateSendClientSession::SelectDenominate(std::string& strErrorRet, std::
 
 bool CPrivateSendClientSession::PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector<std::pair<CTxDSIn, CTxOut> >& vecPSInOutPairsIn, std::vector<std::pair<CTxDSIn, CTxOut> >& vecPSInOutPairsRet, bool fDryRun)
 {
+    AssertLockHeld(cs_main);
+    AssertLockHeld(vpwallets[0]->cs_wallet);
+
     std::vector<int> vecBits;
     if (!CPrivateSend::GetDenominationsBits(nSessionDenom, vecBits)) {
         strErrorRet = "Incorrect session denom";
         return false;
     }
 
-    for (const auto& pair : vecPSInOutPairsIn) {
-        vpwallets[0]->LockCoin(pair.first.prevout);
-    }
-
     // NOTE: No need to randomize order of inputs because they were
     // initially shuffled in CWallet::SelectPSInOutPairsByDenominations already.
     int nDenomResult{0};
@@ -1312,8 +1307,6 @@ bool CPrivateSendClientSession::PrepareDenominate(int nMinRounds, int nMaxRounds
     // Try to add up to PRIVATESEND_ENTRY_MAX_SIZE of every needed denomination
     for (const auto& pair : vecPSInOutPairsIn) {
         if (pair.second.nRounds < nMinRounds || pair.second.nRounds > nMaxRounds) {
-            // unlock unused coins
-            vpwallets[0]->UnlockCoin(pair.first.prevout);
             continue;
         }
         bool fFound = false;
@@ -1343,22 +1336,23 @@ bool CPrivateSendClientSession::PrepareDenominate(int nMinRounds, int nMaxRounds
                 break;
             }
         }
-        if (!fFound || fDryRun) {
-            // unlock unused coins and if we are not going to mix right away
-            vpwallets[0]->UnlockCoin(pair.first.prevout);
-        }
     }
 
     if (nDenomResult != nSessionDenom) {
-        // unlock used coins on failure
-        for (const auto& pair : vecPSInOutPairsRet) {
-            vpwallets[0]->UnlockCoin(pair.first.prevout);
-        }
         keyHolderStorage.ReturnAll();
         strErrorRet = "Can't prepare current denominated outputs";
         return false;
     }
 
+    if (fDryRun) {
+        return true;
+    }
+
+    for (const auto& pair : vecPSInOutPairsRet) {
+        vpwallets[0]->LockCoin(pair.first.prevout);
+        vecOutPointLocked.push_back(pair.first.prevout);
+    }
+
     return true;
 }