Masternode: locking internal cs for lastPing set.

furszy · furszy · commit af01054eef0d · 2020-11-05T10:55:13.000-03:00
diff --git a/src/activemasternode.cpp b/src/activemasternode.cpp
@@ -209,13 +209,18 @@ bool CActiveMasternode::SendMasternodePing(std::string& errorMessage)
             return false;
         }
 
-        pmn->lastPing = mnp;
+        // SetLastPing locks the masternode cs, be careful with the lock order.
+        pmn->SetLastPing(mnp);
         mnodeman.mapSeenMasternodePing.emplace(mnp.GetHash(), mnp);
 
         //mnodeman.mapSeenMasternodeBroadcast.lastPing is probably outdated, so we'll update it
         CMasternodeBroadcast mnb(*pmn);
         uint256 hash = mnb.GetHash();
-        if (mnodeman.mapSeenMasternodeBroadcast.count(hash)) mnodeman.mapSeenMasternodeBroadcast[hash].lastPing = mnp;
+        if (mnodeman.mapSeenMasternodeBroadcast.count(hash)) {
+            // SetLastPing locks the masternode cs, be careful with the lock order.
+            // TODO: check why are we double setting the last ping here..
+            mnodeman.mapSeenMasternodeBroadcast[hash].SetLastPing(mnp);
+        }
 
         mnp.Relay();
         return true;
diff --git a/src/masternode.cpp b/src/masternode.cpp
@@ -727,7 +727,8 @@ bool CMasternodePing::CheckAndUpdate(int& nDos, bool fRequireEnabled, bool fChec
                 return false;
             }
 
-            pmn->lastPing = *this;
+            // SetLastPing locks masternode cs. Be careful with the lock ordering.
+            pmn->SetLastPing(*this);
 
             //mnodeman.mapSeenMasternodeBroadcast.lastPing is probably outdated, so we'll update it
             CMasternodeBroadcast mnb(*pmn);
diff --git a/src/masternode.h b/src/masternode.h
@@ -147,6 +147,8 @@ class CMasternode : public CSignedMessage
     const CTxIn GetVin() const override { return vin; };
     const CPubKey GetPublicKey(std::string& strErrorRet) const override { return pubKeyCollateralAddress; }
 
+    void SetLastPing(const CMasternodePing& _lastPing) { WITH_LOCK(cs, lastPing = _lastPing;); }
+
     void swap(CMasternode& first, CMasternode& second) // nothrow
     {
         CSignedMessage::swap(first, second);

Original file line number	Diff line number	Diff line change
`@@ -727,7 +727,8 @@ bool CMasternodePing::CheckAndUpdate(int& nDos, bool fRequireEnabled, bool fChec`
`727`	`727`	`return false;`
`728`	`728`	`}`
`729`	`729`
`730`		`- pmn->lastPing = *this;`
	`730`	`+ // SetLastPing locks masternode cs. Be careful with the lock ordering.`
	`731`	`+ pmn->SetLastPing(*this);`
`731`	`732`
`732`	`733`	`//mnodeman.mapSeenMasternodeBroadcast.lastPing is probably outdated, so we'll update it`
`733`	`734`	`CMasternodeBroadcast mnb(*pmn);`
Original file line number	Diff line number	Diff line change
`@@ -147,6 +147,8 @@ class CMasternode : public CSignedMessage`
`147`	`147`	`const CTxIn GetVin() const override { return vin; };`
`148`	`148`	`const CPubKey GetPublicKey(std::string& strErrorRet) const override { return pubKeyCollateralAddress; }`
`149`	`149`
	`150`	`+ void SetLastPing(const CMasternodePing& _lastPing) { WITH_LOCK(cs, lastPing = _lastPing;); }`
	`151`	`+`
`150`	`152`	`void swap(CMasternode& first, CMasternode& second) // nothrow`
`151`	`153`	`{`
`152`	`154`	`CSignedMessage::swap(first, second);`