Fix some (potential dead)locks (#1977)

* Avoid locking cs_main in CMasternode/Ping/Broadcast

Deligate this to CMasternodeMan and use AssertLockHeld instead

* Ensure consistent locking order (cs_main, cs_wallet, mempool.cs, cs_instantsend) in CInstantSend while avoiding potential deadlocks at the same time

* Add missing locks in wallet

* Add missing locks in governance

* Fix cs_vPendingMasternodes vs cs_main potential deadlock

SendVerifyRequest no longer opens connection directly, so no cs_main is needed here

src/governance-object.cpp

@@ -102,6 +102,7 @@ bool CGovernanceObject::ProcessVote(CNode* pfrom,
                                     CConnman& connman)
 {
     if(!mnodeman.Has(vote.GetMasternodeOutpoint())) {
+        LOCK(cs);
         std::ostringstream ostr;
         ostr << "CGovernanceObject::ProcessVote -- Masternode " << vote.GetMasternodeOutpoint().ToStringShort() << " not found";
         exception = CGovernanceException(ostr.str(), GOVERNANCE_EXCEPTION_WARNING);
@@ -117,6 +118,9 @@ bool CGovernanceObject::ProcessVote(CNode* pfrom,
         return false;
     }
 
+    vote_instance_t voteInstance;
+    {
+    LOCK(cs);
     vote_m_it it = mapCurrentMNVotes.find(vote.GetMasternodeOutpoint());
     if(it == mapCurrentMNVotes.end()) {
         it = mapCurrentMNVotes.insert(vote_m_t::value_type(vote.GetMasternodeOutpoint(), vote_rec_t())).first;
@@ -141,7 +145,7 @@ bool CGovernanceObject::ProcessVote(CNode* pfrom,
     if(it2 == recVote.mapInstances.end()) {
         it2 = recVote.mapInstances.insert(vote_instance_m_t::value_type(int(eSignal), vote_instance_t())).first;
     }
-    vote_instance_t& voteInstance = it2->second;
+    voteInstance = it2->second;
 
     // Reject obsolete votes
     if(vote.GetTimestamp() < voteInstance.nCreationTime) {
@@ -151,6 +155,7 @@ bool CGovernanceObject::ProcessVote(CNode* pfrom,
         exception = CGovernanceException(ostr.str(), GOVERNANCE_EXCEPTION_NONE);
         return false;
     }
+    } // LOCK(cs)
 
     int64_t nNow = GetAdjustedTime();
     int64_t nVoteTimeUpdate = voteInstance.nTime;
@@ -189,6 +194,7 @@ bool CGovernanceObject::ProcessVote(CNode* pfrom,
         exception = CGovernanceException(ostr.str(), GOVERNANCE_EXCEPTION_PERMANENT_ERROR);
         return false;
     }
+    LOCK(cs);
     voteInstance = vote_instance_t(vote.GetOutcome(), nVoteTimeUpdate, vote.GetTimestamp());
     if(!fileVotes.HasVote(vote.GetHash())) {
         fileVotes.AddVote(vote);
@@ -199,6 +205,8 @@ bool CGovernanceObject::ProcessVote(CNode* pfrom,
 
 void CGovernanceObject::ClearMasternodeVotes()
 {
+    LOCK(cs);
+
     vote_m_it it = mapCurrentMNVotes.begin();
     while(it != mapCurrentMNVotes.end()) {
         if(!mnodeman.Has(it->first)) {
@@ -628,6 +636,8 @@ bool CGovernanceObject::IsCollateralValid(std::string& strError, bool& fMissingC
 
 int CGovernanceObject::CountMatchingVotes(vote_signal_enum_t eVoteSignalIn, vote_outcome_enum_t eVoteOutcomeIn) const
 {
+    LOCK(cs);
+
     int nCount = 0;
     for(vote_m_cit it = mapCurrentMNVotes.begin(); it != mapCurrentMNVotes.end(); ++it) {
         const vote_rec_t& recVote = it->second;
@@ -674,6 +684,8 @@ int CGovernanceObject::GetAbstainCount(vote_signal_enum_t eVoteSignalIn) const
 
 bool CGovernanceObject::GetCurrentMNVotes(const COutPoint& mnCollateralOutpoint, vote_rec_t& voteRecord)
 {
+    LOCK(cs);
+
     vote_m_it it = mapCurrentMNVotes.find(mnCollateralOutpoint);
     if (it == mapCurrentMNVotes.end()) {
         return false;

src/governance.cpp

@@ -1164,6 +1164,8 @@ bool CGovernanceManager::AcceptMessage(const uint256& nHash, hash_s_t& setHash)
 
 void CGovernanceManager::RebuildIndexes()
 {
+    LOCK(cs);
+
     cmapVoteToObject.Clear();
     for(object_m_it it = mapObjects.begin(); it != mapObjects.end(); ++it) {
         CGovernanceObject& govobj = it->second;

src/instantx.cpp

@@ -69,15 +69,11 @@ void CInstantSend::ProcessMessage(CNode* pfrom, const std::string& strCommand, C
         // Ignore any InstantSend messages until masternode list is synced
         if(!masternodeSync.IsMasternodeListSynced()) return;
 
-        LOCK(cs_main);
+        {
-#ifdef ENABLE_WALLET
+            LOCK(cs_instantsend);
-        if (pwalletMain)
+            auto ret = mapTxLockVotes.emplace(nVoteHash, vote);
-            LOCK(pwalletMain->cs_wallet);
+            if (!ret.second) return;
-#endif
+        }
-        LOCK(cs_instantsend);
-
-        if(mapTxLockVotes.count(nVoteHash)) return;
-        mapTxLockVotes.insert(std::make_pair(nVoteHash, vote));
 
         ProcessNewTxLockVote(pfrom, vote, connman);
 
@@ -87,7 +83,11 @@ void CInstantSend::ProcessMessage(CNode* pfrom, const std::string& strCommand, C
 
 bool CInstantSend::ProcessTxLockRequest(const CTxLockRequest& txLockRequest, CConnman& connman)
 {
-    LOCK2(cs_main, cs_instantsend);
+    LOCK(cs_main);
+#ifdef ENABLE_WALLET
+    LOCK(pwalletMain ? &pwalletMain->cs_wallet : NULL);
+#endif
+    LOCK2(mempool.cs, cs_instantsend);
 
     uint256 txHash = txLockRequest.GetHash();
 
@@ -183,13 +183,23 @@ void CInstantSend::CreateEmptyTxLockCandidate(const uint256& txHash)
 void CInstantSend::Vote(const uint256& txHash, CConnman& connman)
 {
     AssertLockHeld(cs_main);
-    LOCK(cs_instantsend);
+#ifdef ENABLE_WALLET
+    LOCK(pwalletMain ? &pwalletMain->cs_wallet : NULL);
+#endif
+
+    CTxLockRequest dummyRequest;
+    CTxLockCandidate txLockCandidate(dummyRequest);
+    {
+        LOCK(cs_instantsend);
+        auto itLockCandidate = mapTxLockCandidates.find(txHash);
+        if (itLockCandidate == mapTxLockCandidates.end()) return;
+        txLockCandidate = itLockCandidate->second;
+        Vote(txLockCandidate, connman);
+    }
 
-    std::map<uint256, CTxLockCandidate>::iterator itLockCandidate = mapTxLockCandidates.find(txHash);
-    if (itLockCandidate == mapTxLockCandidates.end()) return;
-    Vote(itLockCandidate->second, connman);
     // Let's see if our vote changed smth
-    TryToFinalizeLockCandidate(itLockCandidate->second);
+    LOCK2(mempool.cs, cs_instantsend);
+    TryToFinalizeLockCandidate(txLockCandidate);
 }
 
 void CInstantSend::Vote(CTxLockCandidate& txLockCandidate, CConnman& connman)
@@ -197,7 +207,8 @@ void CInstantSend::Vote(CTxLockCandidate& txLockCandidate, CConnman& connman)
     if(!fMasternodeMode) return;
     if(!sporkManager.IsSporkActive(SPORK_2_INSTANTSEND_ENABLED)) return;
 
-    LOCK2(cs_main, cs_instantsend);
+    AssertLockHeld(cs_main);
+    AssertLockHeld(cs_instantsend);
 
     uint256 txHash = txLockCandidate.GetHash();
     // We should never vote on a Transaction Lock Request that was not (yet) accepted by the mempool
@@ -295,14 +306,6 @@ void CInstantSend::Vote(CTxLockCandidate& txLockCandidate, CConnman& connman)
 
 bool CInstantSend::ProcessNewTxLockVote(CNode* pfrom, const CTxLockVote& vote, CConnman& connman)
 {
-    // cs_main, cs_wallet and cs_instantsend should be already locked
-    AssertLockHeld(cs_main);
-#ifdef ENABLE_WALLET
-    if (pwalletMain)
-        AssertLockHeld(pwalletMain->cs_wallet);
-#endif
-    AssertLockHeld(cs_instantsend);
-
     uint256 txHash = vote.GetTxHash();
     uint256 nVoteHash = vote.GetHash();
 
@@ -315,6 +318,12 @@ bool CInstantSend::ProcessNewTxLockVote(CNode* pfrom, const CTxLockVote& vote, C
     // relay valid vote asap
     vote.Relay(connman);
 
+    LOCK(cs_main);
+#ifdef ENABLE_WALLET
+    LOCK(pwalletMain ? &pwalletMain->cs_wallet : NULL);
+#endif
+    LOCK2(mempool.cs, cs_instantsend);
+
     // Masternodes will sometimes propagate votes before the transaction is known to the client,
     // will actually process only after the lock request itself has arrived
 
@@ -457,12 +466,8 @@ void CInstantSend::UpdateVotedOutpoints(const CTxLockVote& vote, CTxLockCandidat
 
 void CInstantSend::ProcessOrphanTxLockVotes()
 {
-    LOCK(cs_main);
+    AssertLockHeld(cs_main);
-#ifdef ENABLE_WALLET
+    AssertLockHeld(cs_instantsend);
-    if (pwalletMain)
-        LOCK(pwalletMain->cs_wallet);
-#endif
-    LOCK(cs_instantsend);
 
     std::map<uint256, CTxLockVote>::iterator it = mapTxLockVotesOrphan.begin();
     while(it != mapTxLockVotesOrphan.end()) {
@@ -496,12 +501,8 @@ void CInstantSend::TryToFinalizeLockCandidate(const CTxLockCandidate& txLockCand
 {
     if(!sporkManager.IsSporkActive(SPORK_2_INSTANTSEND_ENABLED)) return;
 
-    LOCK(cs_main);
+    AssertLockHeld(cs_main);
-#ifdef ENABLE_WALLET
+    AssertLockHeld(cs_instantsend);
-    if (pwalletMain)
-        LOCK(pwalletMain->cs_wallet);
-#endif
-    LOCK(cs_instantsend);
 
     uint256 txHash = txLockCandidate.txLockRequest.tx->GetHash();
     if(txLockCandidate.IsAllOutPointsReady() && !IsLockedInstantSendTransaction(txHash)) {
@@ -516,7 +517,8 @@ void CInstantSend::TryToFinalizeLockCandidate(const CTxLockCandidate& txLockCand
 
 void CInstantSend::UpdateLockedTransaction(const CTxLockCandidate& txLockCandidate)
 {
-    // cs_wallet and cs_instantsend should be already locked
+    // cs_main, cs_wallet and cs_instantsend should be already locked
+    AssertLockHeld(cs_main);
 #ifdef ENABLE_WALLET
     if (pwalletMain)
         AssertLockHeld(pwalletMain->cs_wallet);
@@ -575,14 +577,15 @@ bool CInstantSend::GetLockedOutPointTxHash(const COutPoint& outpoint, uint256& h
 
 bool CInstantSend::ResolveConflicts(const CTxLockCandidate& txLockCandidate)
 {
-    LOCK2(cs_main, cs_instantsend);
+    AssertLockHeld(cs_main);
+    AssertLockHeld(cs_instantsend);
 
     uint256 txHash = txLockCandidate.GetHash();
 
     // make sure the lock is ready
     if(!txLockCandidate.IsAllOutPointsReady()) return false;
 
-    LOCK(mempool.cs); // protect mempool.mapNextTx
+    AssertLockHeld(mempool.cs); // protect mempool.mapNextTx
 
     for (const auto& txin : txLockCandidate.txLockRequest.tx->vin) {
         uint256 hashConflicting;
@@ -953,7 +956,7 @@ bool CTxLockRequest::IsValid() const
         LogPrint("instantsend", "CTxLockRequest::IsValid -- WARNING: Too many inputs: tx=%s", ToString());
     }
 
-    LOCK(cs_main);
+    AssertLockHeld(cs_main);
     if(!CheckFinalTx(*tx)) {
         LogPrint("instantsend", "CTxLockRequest::IsValid -- Transaction is not final: tx=%s", ToString());
         return false;

src/masternode.cpp

@@ -130,6 +130,7 @@ CMasternode::CollateralStatus CMasternode::CheckCollateral(const COutPoint& outp
 
 void CMasternode::Check(bool fForce)
 {
+    AssertLockHeld(cs_main);
     LOCK(cs);
 
     if(ShutdownRequested()) return;
@@ -144,9 +145,6 @@ void CMasternode::Check(bool fForce)
 
     int nHeight = 0;
     if(!fUnitTest) {
-        TRY_LOCK(cs_main, lockMain);
-        if(!lockMain) return;
-
         Coin coin;
         if(!GetUTXOCoin(outpoint, coin)) {
             nActiveState = MASTERNODE_OUTPOINT_SPENT;
@@ -415,6 +413,8 @@ bool CMasternodeBroadcast::SimpleCheck(int& nDos)
 {
     nDos = 0;
 
+    AssertLockHeld(cs_main);
+
     // make sure addr is valid
     if(!IsValidNetAddr()) {
         LogPrintf("CMasternodeBroadcast::SimpleCheck -- Invalid addr, rejected: masternode=%s  addr=%s\n",
@@ -470,6 +470,8 @@ bool CMasternodeBroadcast::Update(CMasternode* pmn, int& nDos, CConnman& connman
 {
     nDos = 0;
 
+    AssertLockHeld(cs_main);
+
     if(pmn->sigTime == sigTime && !fRecovery) {
         // mapSeenMasternodeBroadcast in CMasternodeMan::CheckMnbAndUpdateMasternodeList should filter legit duplicates
         // but this still can happen if we just started, which is ok, just do nothing here.
@@ -820,6 +822,8 @@ bool CMasternodePing::SimpleCheck(int& nDos)
 
 bool CMasternodePing::CheckAndUpdate(CMasternode* pmn, bool fFromNewBroadcast, int& nDos, CConnman& connman)
 {
+    AssertLockHeld(cs_main);
+
     // don't ban by default
     nDos = 0;
 
@@ -845,7 +849,6 @@ bool CMasternodePing::CheckAndUpdate(CMasternode* pmn, bool fFromNewBroadcast, i
     }
 
     {
-        LOCK(cs_main);
         BlockMap::iterator mi = mapBlockIndex.find(blockHash);
         if ((*mi).second && (*mi).second->nHeight < chainActive.Height() - 24) {
             LogPrintf("CMasternodePing::CheckAndUpdate -- Masternode ping is invalid, block hash is too old: masternode=%s  blockHash=%s\n", masternodeOutpoint.ToStringShort(), blockHash.ToString());

src/masternodeman.cpp

@@ -156,7 +156,7 @@ bool CMasternodeMan::PoSeBan(const COutPoint &outpoint)
 
 void CMasternodeMan::Check()
 {
-    LOCK(cs);
+    LOCK2(cs_main, cs);
 
     LogPrint("masternode", "CMasternodeMan::Check -- nLastSentinelPingTime=%d, IsSentinelPingActive()=%d\n", nLastSentinelPingTime, IsSentinelPingActive());
 
@@ -999,9 +999,7 @@ void CMasternodeMan::DoFullVerificationStep(CConnman& connman)
     rank_pair_vec_t vecMasternodeRanks;
     GetMasternodeRanks(vecMasternodeRanks, nCachedBlockHeight - 1, MIN_POSE_PROTO_VERSION);
 
-    // Need LOCK2 here to ensure consistent locking order because the SendVerifyRequest call below locks cs_main
+    LOCK(cs);
-    // through InitializeNode signal in OpenNetworkConnection
-    LOCK2(cs_main, cs);
 
     int nCount = 0;
 
@@ -1643,7 +1641,7 @@ void CMasternodeMan::RemoveGovernanceObject(uint256 nGovernanceObjectHash)
 
 void CMasternodeMan::CheckMasternode(const CPubKey& pubKeyMasternode, bool fForce)
 {
-    LOCK(cs);
+    LOCK2(cs_main, cs);
     for (auto& mnpair : mapMasternodes) {
         if (mnpair.second.pubKeyMasternode == pubKeyMasternode) {
             mnpair.second.Check(fForce);

src/wallet/wallet.cpp

@@ -3193,6 +3193,8 @@ bool CWallet::SelectCoinsDark(CAmount nValueMin, CAmount nValueMax, std::vector<
 
 bool CWallet::GetCollateralTxDSIn(CTxDSIn& txdsinRet, CAmount& nValueRet) const
 {
+    LOCK2(cs_main, cs_wallet);
+
     std::vector<COutput> vCoins;
 
     AvailableCoins(vCoins);
@@ -3305,6 +3307,8 @@ bool CWallet::HasCollateralInputs(bool fOnlyConfirmed) const
 
 bool CWallet::CreateCollateralTransaction(CMutableTransaction& txCollateral, std::string& strReason)
 {
+    LOCK2(cs_main, cs_wallet);
+
     txCollateral.vin.clear();
     txCollateral.vout.clear();
 
@@ -3375,6 +3379,8 @@ bool CWallet::GetBudgetSystemCollateralTX(CWalletTx& tx, uint256 hash, CAmount a
 
 bool CWallet::ConvertList(std::vector<CTxIn> vecTxIn, std::vector<CAmount>& vecAmounts)
 {
+    LOCK2(cs_main, cs_wallet);
+
     for (const auto& txin : vecTxIn) {
         if (mapWallet.count(txin.prevout.hash)) {
             CWalletTx& wtx = mapWallet[txin.prevout.hash];