[BUG] Fix lock order issue with CMMan::CheckAndRemove and CMPayments::CleanPaymentList

src/masternode-payments.cpp

@@ -66,7 +66,7 @@ bool CMasternodePaymentDB::Write(const CMasternodePayments& objToSave)
     return true;
 }
 
-CMasternodePaymentDB::ReadResult CMasternodePaymentDB::Read(CMasternodePayments& objToLoad, bool fDryRun)
+CMasternodePaymentDB::ReadResult CMasternodePaymentDB::Read(CMasternodePayments& objToLoad)
 {
     int64_t nStart = GetTimeMillis();
     // open input file, and associate with CAutoFile
@@ -138,12 +138,6 @@ CMasternodePaymentDB::ReadResult CMasternodePaymentDB::Read(CMasternodePayments&
 
     LogPrint(BCLog::MASTERNODE,"Loaded info from mnpayments.dat  %dms\n", GetTimeMillis() - nStart);
     LogPrint(BCLog::MASTERNODE,"  %s\n", objToLoad.ToString());
-    if (!fDryRun) {
-        LogPrint(BCLog::MASTERNODE,"Masternode payments manager - cleaning....\n");
-        objToLoad.CleanPaymentList();
-        LogPrint(BCLog::MASTERNODE,"Masternode payments manager - result:\n");
-        LogPrint(BCLog::MASTERNODE,"  %s\n", objToLoad.ToString());
-    }
 
     return Ok;
 }
@@ -209,7 +203,7 @@ void DumpMasternodePayments()
     CMasternodePayments tempPayments;
 
     LogPrint(BCLog::MASTERNODE,"Verifying mnpayments.dat format...\n");
-    CMasternodePaymentDB::ReadResult readResult = paymentdb.Read(tempPayments, true);
+    CMasternodePaymentDB::ReadResult readResult = paymentdb.Read(tempPayments);
     // there was an error and it was not an error on file opening => do not proceed
     if (readResult == CMasternodePaymentDB::FileError)
         LogPrint(BCLog::MASTERNODE,"Missing budgets file - mnpayments.dat, will try to recreate\n");
@@ -614,14 +608,12 @@ bool CMasternodePayments::IsTransactionValid(const CTransaction& txNew, int nBlo
     return true;
 }
 
-void CMasternodePayments::CleanPaymentList()
+void CMasternodePayments::CleanPaymentList(int mnCount, int nHeight)
 {
     LOCK2(cs_mapMasternodePayeeVotes, cs_mapMasternodeBlocks);
 
-    int nHeight = mnodeman.GetBestHeight();
-
     //keep up to five cycles for historical sake
-    int nLimit = std::max(int(mnodeman.size() * 1.25), 1000);
+    int nLimit = std::max(int(mnCount * 1.25), 1000);
 
     std::map<uint256, CMasternodePaymentWinner>::iterator it = mapMasternodePayeeVotes.begin();
     while (it != mapMasternodePayeeVotes.end()) {

src/masternode-payments.h

@@ -52,7 +52,7 @@ class CMasternodePaymentDB
 
     CMasternodePaymentDB();
     bool Write(const CMasternodePayments& objToSave);
-    ReadResult Read(CMasternodePayments& objToLoad, bool fDryRun = false);
+    ReadResult Read(CMasternodePayments& objToLoad);
 };
 
 class CMasternodePayee
@@ -251,7 +251,7 @@ class CMasternodePayments
     bool ProcessBlock(int nBlockHeight);
 
     void Sync(CNode* node, int nCountNeeded);
-    void CleanPaymentList();
+    void CleanPaymentList(int mnCount, int nHeight);
 
     bool GetBlockPayee(int nBlockHeight, CScript& payee);
     bool IsTransactionValid(const CTransaction& txNew, int nBlockHeight);

src/masternodeman.cpp

@@ -93,7 +93,7 @@ bool CMasternodeDB::Write(const CMasternodeMan& mnodemanToSave)
     return true;
 }
 
-CMasternodeDB::ReadResult CMasternodeDB::Read(CMasternodeMan& mnodemanToLoad, bool fDryRun)
+CMasternodeDB::ReadResult CMasternodeDB::Read(CMasternodeMan& mnodemanToLoad)
 {
     int64_t nStart = GetTimeMillis();
     // open input file, and associate with CAutoFile
@@ -164,12 +164,6 @@ CMasternodeDB::ReadResult CMasternodeDB::Read(CMasternodeMan& mnodemanToLoad, bo
 
     LogPrint(BCLog::MASTERNODE,"Loaded info from mncache.dat  %dms\n", GetTimeMillis() - nStart);
     LogPrint(BCLog::MASTERNODE,"  %s\n", mnodemanToLoad.ToString());
-    if (!fDryRun) {
-        LogPrint(BCLog::MASTERNODE,"Masternode manager - cleaning....\n");
-        mnodemanToLoad.CheckAndRemove(true);
-        LogPrint(BCLog::MASTERNODE,"Masternode manager - result:\n");
-        LogPrint(BCLog::MASTERNODE,"  %s\n", mnodemanToLoad.ToString());
-    }
 
     return Ok;
 }
@@ -182,7 +176,7 @@ void DumpMasternodes()
     CMasternodeMan tempMnodeman;
 
     LogPrint(BCLog::MASTERNODE,"Verifying mncache.dat format...\n");
-    CMasternodeDB::ReadResult readResult = mndb.Read(tempMnodeman, true);
+    CMasternodeDB::ReadResult readResult = mndb.Read(tempMnodeman);
     // there was an error and it was not an error on file opening => do not proceed
     if (readResult == CMasternodeDB::FileError)
         LogPrint(BCLog::MASTERNODE,"Missing masternode cache file - mncache.dat, will try to recreate\n");
@@ -215,8 +209,9 @@ bool CMasternodeMan::Add(CMasternode& mn)
 
     const auto& it = mapMasternodes.find(mn.vin.prevout);
     if (it == mapMasternodes.end()) {
-        LogPrint(BCLog::MASTERNODE, "CMasternodeMan: Adding new Masternode %s - %i now\n", mn.vin.prevout.hash.ToString(), size() + 1);
+        LogPrint(BCLog::MASTERNODE, "Adding new Masternode %s\n", mn.vin.prevout.ToString());
         mapMasternodes.emplace(mn.vin.prevout, std::make_shared<CMasternode>(mn));
+        LogPrint(BCLog::MASTERNODE, "Masternode added. New total count: %d\n", mapMasternodes.size());
         return true;
     }
 
@@ -239,7 +234,7 @@ void CMasternodeMan::AskForMN(CNode* pnode, const CTxIn& vin)
     mWeAskedForMasternodeListEntry[vin.prevout] = askAgain;
 }
 
-void CMasternodeMan::CheckAndRemove(bool forceExpiredRemoval)
+int CMasternodeMan::CheckAndRemove(bool forceExpiredRemoval)
 {
     LOCK(cs);
 
@@ -252,7 +247,7 @@ void CMasternodeMan::CheckAndRemove(bool forceExpiredRemoval)
             activeState == CMasternode::MASTERNODE_VIN_SPENT ||
             (forceExpiredRemoval && activeState == CMasternode::MASTERNODE_EXPIRED) ||
             mn->protocolVersion < ActiveProtocol()) {
-            LogPrint(BCLog::MASTERNODE, "CMasternodeMan: Removing inactive Masternode %s - %i now\n", it->first.hash.ToString(), size() - 1);
+            LogPrint(BCLog::MASTERNODE, "Removing inactive Masternode %s\n", it->first.ToString());
 
             //erase all of the broadcasts we've seen from this vin
             // -- if we missed a few pings and the node was removed, this will allow is to get it back without them
@@ -278,10 +273,12 @@ void CMasternodeMan::CheckAndRemove(bool forceExpiredRemoval)
             }
 
             it = mapMasternodes.erase(it);
+            LogPrint(BCLog::MASTERNODE, "Masternode removed.\n");
         } else {
             ++it;
         }
     }
+    LogPrint(BCLog::MASTERNODE, "New total masternode count: %d\n", mapMasternodes.size());
 
     // check who's asked for the Masternode list
     std::map<CNetAddr, int64_t>::iterator it1 = mAskedUsForMasternodeList.begin();
@@ -333,6 +330,8 @@ void CMasternodeMan::CheckAndRemove(bool forceExpiredRemoval)
             ++it4;
         }
     }
+
+    return mapMasternodes.size();
 }
 
 void CMasternodeMan::Clear()
@@ -389,8 +388,9 @@ int CMasternodeMan::CountEnabled(int protocolVersion) const
     return i;
 }
 
-void CMasternodeMan::CountNetworks(int protocolVersion, int& ipv4, int& ipv6, int& onion) const
+int CMasternodeMan::CountNetworks(int& ipv4, int& ipv6, int& onion) const
 {
+    LOCK(cs);
     for (const auto& it : mapMasternodes) {
         const MasternodeRef& mn = it.second;
         std::string strHost;
@@ -411,6 +411,7 @@ void CMasternodeMan::CountNetworks(int protocolVersion, int& ipv4, int& ipv6, in
                 break;
         }
     }
+    return mapMasternodes.size();
 }
 
 void CMasternodeMan::DsegUpdate(CNode* pnode)
@@ -843,9 +844,10 @@ void CMasternodeMan::UpdateMasternodeList(CMasternodeBroadcast mnb)
 std::string CMasternodeMan::ToString() const
 {
     std::ostringstream info;
-
-    info << "Masternodes: " << (int)size() << ", peers who asked us for Masternode list: " << (int)mAskedUsForMasternodeList.size() << ", peers we asked for Masternode list: " << (int)mWeAskedForMasternodeList.size() << ", entries in Masternode list we asked for: " << (int)mWeAskedForMasternodeListEntry.size();
-
+    info << "Masternodes: " << (int)mapMasternodes.size()
+         << ", peers who asked us for Masternode list: " << (int)mAskedUsForMasternodeList.size()
+         << ", peers we asked for Masternode list: " << (int)mWeAskedForMasternodeList.size()
+         << ", entries in Masternode list we asked for: " << (int)mWeAskedForMasternodeListEntry.size();
     return info.str();
 }
 
@@ -913,6 +915,9 @@ void ThreadCheckMasternodes()
     unsigned int c = 0;
 
     try {
+        // first clean up stale masternode payments data
+        masternodePayments.CleanPaymentList(mnodeman.CheckAndRemove(), mnodeman.GetBestHeight());
+
         while (true) {
 
             if (ShutdownRequested()) {
@@ -921,6 +926,7 @@ void ThreadCheckMasternodes()
 
             MilliSleep(1000);
             boost::this_thread::interruption_point();
+
             // try to sync from all available nodes, one step at a time
             masternodeSync.Process();
 
@@ -933,8 +939,7 @@ void ThreadCheckMasternodes()
                     activeMasternode.ManageStatus();
 
                 if (c % (MasternodePingSeconds()/5) == 0) {
-                    mnodeman.CheckAndRemove();
-                    masternodePayments.CleanPaymentList();
+                    masternodePayments.CleanPaymentList(mnodeman.CheckAndRemove(), mnodeman.GetBestHeight());
                 }
             }
         }

src/masternodeman.h

@@ -51,7 +51,7 @@ class CMasternodeDB
 
     CMasternodeDB();
     bool Write(const CMasternodeMan& mnodemanToSave);
-    ReadResult Read(CMasternodeMan& mnodemanToLoad, bool fDryRun = false);
+    ReadResult Read(CMasternodeMan& mnodemanToLoad);
 };
 
 //
@@ -120,8 +120,8 @@ class CMasternodeMan
     /// Ask (source) node for mnb
     void AskForMN(CNode* pnode, const CTxIn& vin);
 
-    /// Check all Masternodes and remove inactive
-    void CheckAndRemove(bool forceExpiredRemoval = false);
+    /// Check all Masternodes and remove inactive. Return the total masternode count.
+    int CheckAndRemove(bool forceExpiredRemoval = false);
 
     /// Clear Masternode vector
     void Clear();
@@ -131,7 +131,8 @@ class CMasternodeMan
 
     int CountEnabled(int protocolVersion = -1) const;
 
-    void CountNetworks(int protocolVersion, int& ipv4, int& ipv6, int& onion) const;
+    /// Count the number of nodes with a specific proto version for each network. Return the total.
+    int CountNetworks(int& ipv4, int& ipv6, int& onion) const;
 
     void DsegUpdate(CNode* pnode);
 
@@ -161,9 +162,6 @@ class CMasternodeMan
     // Process GETMNLIST message, returning the banning score (if 0, no ban score increase is needed)
     int ProcessGetMNList(CNode* pfrom, CTxIn& vin);
 
-    /// Return the number of (unique) Masternodes
-    int size() const { LOCK(cs); return mapMasternodes.size(); }
-
     /// Return the number of Masternodes older than (default) 8000 seconds
     int stable_size() const;
 

src/qt/clientmodel.cpp

@@ -76,10 +76,10 @@ int ClientModel::getNumConnections(unsigned int flags) const
 QString ClientModel::getMasternodeCountString() const
 {
     int ipv4 = 0, ipv6 = 0, onion = 0;
-    mnodeman.CountNetworks(ActiveProtocol(), ipv4, ipv6, onion);
-    int nUnknown = mnodeman.size() - ipv4 - ipv6 - onion;
+    int total = mnodeman.CountNetworks(ipv4, ipv6, onion);
+    int nUnknown = total - ipv4 - ipv6 - onion;
     if(nUnknown < 0) nUnknown = 0;
-    return tr("Total: %1 (IPv4: %2 / IPv6: %3 / Tor: %4 / Unknown: %5)").arg(QString::number((int)mnodeman.size())).arg(QString::number((int)ipv4)).arg(QString::number((int)ipv6)).arg(QString::number((int)onion)).arg(QString::number((int)nUnknown));
+    return tr("Total: %1 (IPv4: %2 / IPv6: %3 / Tor: %4 / Unknown: %5)").arg(QString::number(total)).arg(QString::number((int)ipv4)).arg(QString::number((int)ipv6)).arg(QString::number((int)onion)).arg(QString::number((int)nUnknown));
 }
 
 int ClientModel::getNumBlocks()

src/rpc/masternode.cpp

@@ -203,9 +203,9 @@ UniValue getmasternodecount (const JSONRPCRequest& request)
     if (nChainHeight < 0) return "unknown";
 
     mnodeman.GetNextMasternodeInQueueForPayment(nChainHeight, true, nCount);
-    mnodeman.CountNetworks(ActiveProtocol(), ipv4, ipv6, onion);
+    int total = mnodeman.CountNetworks(ipv4, ipv6, onion);
 
-    obj.pushKV("total", mnodeman.size());
+    obj.pushKV("total", total);
     obj.pushKV("stable", mnodeman.stable_size());
     obj.pushKV("enabled", mnodeman.CountEnabled());
     obj.pushKV("inqueue", nCount);