[BUG] Fix potential deadlock

random-zebra · random-zebra · commit 5da28cc7d149 · 2021-09-23T17:17:51.000+02:00
CountEnabled is being called while holding CMasternodeMan::cs, by
SecondsSincePayment, and it locks CDeterministicMNManager::cs to get the
deterministic manager's tip height.
Pass the enabled masternode count to SecondsSincePayment, directly from
the caller, so the two locks are not held at the same time.
diff --git a/src/masternodeman.cpp b/src/masternodeman.cpp
@@ -420,10 +420,13 @@ int CMasternodeMan::CountEnabled(bool only_legacy) const
     int i = 0;
     int protocolVersion = ActiveProtocol();
 
-    for (const auto& it : mapMasternodes) {
-        const MasternodeRef& mn = it.second;
-        if (mn->protocolVersion < protocolVersion || !mn->IsEnabled()) continue;
-        i++;
+    {
+        LOCK(cs);
+        for (const auto& it : mapMasternodes) {
+            const MasternodeRef& mn = it.second;
+            if (mn->protocolVersion < protocolVersion || !mn->IsEnabled()) continue;
+            i++;
+        }
     }
 
     if (!only_legacy && deterministicMNManager->IsDIP3Enforced()) {
@@ -543,13 +546,13 @@ MasternodeRef CMasternodeMan::GetNextMasternodeInQueueForPayment(int nBlockHeigh
         Make a vector with all of the last paid times
     */
     int minProtocol = ActiveProtocol();
-    int nMnCount = CountEnabled();
+    int count_enabled = CountEnabled();
     {
         LOCK(cs);
         for (const auto& it : mapMasternodes) {
             if (!it.second->IsEnabled()) continue;
-            if (canScheduleMN(fFilterSigTime, it.second, minProtocol, nMnCount, nBlockHeight)) {
-                vecMasternodeLastPaid.emplace_back(SecondsSincePayment(it.second, BlockReading), it.second);
+            if (canScheduleMN(fFilterSigTime, it.second, minProtocol, count_enabled, nBlockHeight)) {
+                vecMasternodeLastPaid.emplace_back(SecondsSincePayment(it.second, count_enabled, BlockReading), it.second);
             }
         }
     }
@@ -558,16 +561,16 @@ MasternodeRef CMasternodeMan::GetNextMasternodeInQueueForPayment(int nBlockHeigh
         CDeterministicMNList mnList = deterministicMNManager->GetListAtChainTip();
         mnList.ForEachMN(true, [&](const CDeterministicMNCPtr& dmn) {
             const MasternodeRef mn = MakeMasternodeRefForDMN(dmn);
-            if (canScheduleMN(fFilterSigTime, mn, minProtocol, nMnCount, nBlockHeight)) {
-                vecMasternodeLastPaid.emplace_back(SecondsSincePayment(mn, BlockReading), mn);
+            if (canScheduleMN(fFilterSigTime, mn, minProtocol, count_enabled, nBlockHeight)) {
+                vecMasternodeLastPaid.emplace_back(SecondsSincePayment(mn, count_enabled, BlockReading), mn);
             }
         });
     }
 
     nCount = (int)vecMasternodeLastPaid.size();
 
     //when the network is in the process of upgrading, don't penalize nodes that recently restarted
-    if (fFilterSigTime && nCount < nMnCount / 3) return GetNextMasternodeInQueueForPayment(nBlockHeight, false, nCount, BlockReading);
+    if (fFilterSigTime && nCount < count_enabled / 3) return GetNextMasternodeInQueueForPayment(nBlockHeight, false, nCount, BlockReading);
 
     // Sort them high to low
     sort(vecMasternodeLastPaid.rbegin(), vecMasternodeLastPaid.rend(), CompareScoreMN());
@@ -576,7 +579,7 @@ MasternodeRef CMasternodeMan::GetNextMasternodeInQueueForPayment(int nBlockHeigh
     //  -- This doesn't look at who is being paid in the +8-10 blocks, allowing for double payments very rarely
     //  -- 1/100 payments should be a double payment on mainnet - (1/(3000/10))*2
     //  -- (chance per block * chances before IsScheduled will fire)
-    int nTenthNetwork = nMnCount / 10;
+    int nTenthNetwork = count_enabled / 10;
     int nCountTenth = 0;
     arith_uint256 nHigh = ARITH_UINT256_ZERO;
     const uint256& hash = GetHashAtHeight(nBlockHeight - 101);
@@ -943,9 +946,9 @@ void CMasternodeMan::UpdateMasternodeList(CMasternodeBroadcast& mnb)
     }
 }
 
-int64_t CMasternodeMan::SecondsSincePayment(const MasternodeRef& mn, const CBlockIndex* BlockReading) const
+int64_t CMasternodeMan::SecondsSincePayment(const MasternodeRef& mn, int count_enabled, const CBlockIndex* BlockReading) const
 {
-    int64_t sec = (GetAdjustedTime() - GetLastPaid(mn, BlockReading));
+    int64_t sec = (GetAdjustedTime() - GetLastPaid(mn, count_enabled, BlockReading));
     int64_t month = 60 * 60 * 24 * 30;
     if (sec < month) return sec; //if it's less than 30 days, give seconds
 
@@ -958,7 +961,7 @@ int64_t CMasternodeMan::SecondsSincePayment(const MasternodeRef& mn, const CBloc
     return month + hash.GetCompact(false);
 }
 
-int64_t CMasternodeMan::GetLastPaid(const MasternodeRef& mn, const CBlockIndex* BlockReading) const
+int64_t CMasternodeMan::GetLastPaid(const MasternodeRef& mn, int count_enabled, const CBlockIndex* BlockReading) const
 {
     if (BlockReading == nullptr) return false;
 
@@ -972,8 +975,8 @@ int64_t CMasternodeMan::GetLastPaid(const MasternodeRef& mn, const CBlockIndex*
     // use a deterministic offset to break a tie -- 2.5 minutes
     int64_t nOffset = UintToArith256(hash).GetCompact(false) % 150;
 
-    int nMnCount = CountEnabled() * 1.25;
-    for (int n = 0; n < nMnCount; n++) {
+    int max_depth = count_enabled * 1.25;
+    for (int n = 0; n < max_depth; n++) {
         const auto& it = masternodePayments.mapMasternodeBlocks.find(BlockReading->nHeight);
         if (it != masternodePayments.mapMasternodeBlocks.end()) {
             // Search for this payee, with at least 2 votes. This will aid in consensus
diff --git a/src/masternodeman.h b/src/masternodeman.h
@@ -181,8 +181,8 @@ class CMasternodeMan
     void UpdateMasternodeList(CMasternodeBroadcast& mnb);
 
     /// Get the time a masternode was last paid
-    int64_t GetLastPaid(const MasternodeRef& mn, const CBlockIndex* BlockReading) const;
-    int64_t SecondsSincePayment(const MasternodeRef& mn, const CBlockIndex* BlockReading) const;
+    int64_t GetLastPaid(const MasternodeRef& mn, int count_enabled, const CBlockIndex* BlockReading) const;
+    int64_t SecondsSincePayment(const MasternodeRef& mn, int count_enabled, const CBlockIndex* BlockReading) const;
 
     // Block hashes cycling vector management
     void CacheBlockHash(const CBlockIndex* pindex);
diff --git a/src/rpc/masternode.cpp b/src/rpc/masternode.cpp
@@ -191,6 +191,7 @@ UniValue listmasternodes(const JSONRPCRequest& request)
     int nHeight = chainTip->nHeight;
     auto mnList = deterministicMNManager->GetListAtChainTip();
 
+    int count_enabled = mnodeman.CountEnabled();
     std::vector<std::pair<int64_t, MasternodeRef>> vMasternodeRanks = mnodeman.GetMasternodeRanks(nHeight);
     for (int pos=0; pos < (int) vMasternodeRanks.size(); pos++) {
         const auto& s = vMasternodeRanks[pos];
@@ -244,7 +245,7 @@ UniValue listmasternodes(const JSONRPCRequest& request)
         obj.pushKV("version", mn.protocolVersion);
         obj.pushKV("lastseen", (int64_t)mn.lastPing.sigTime);
         obj.pushKV("activetime", (int64_t)(mn.lastPing.sigTime - mn.sigTime));
-        obj.pushKV("lastpaid", (int64_t)mnodeman.GetLastPaid(s.second, chainTip));
+        obj.pushKV("lastpaid", (int64_t)mnodeman.GetLastPaid(s.second, count_enabled, chainTip));
 
         ret.push_back(obj);
     }
