refactor: protect some CGovernanceObject variables

Currently, the LOCKs aren't guarding anything. Variables protected were
based on existing LOCK'ing patterns (i.e. (de)ser'ed variables except
for m_obj).

Author: kwvg

src/governance/governance.cpp

@@ -187,7 +187,7 @@ bool CGovernanceManager::HaveVoteForHash(const uint256& nHash) const
     LOCK(cs_store);
 
     CGovernanceObject* pGovobj = nullptr;
-    return cmapVoteToObject.Get(nHash, pGovobj) && pGovobj->GetVoteFile().HasVote(nHash);
+    return cmapVoteToObject.Get(nHash, pGovobj) && WITH_LOCK(pGovobj->cs, return pGovobj->GetVoteFile().HasVote(nHash));
 }
 
 int CGovernanceManager::GetVoteCount() const
@@ -201,7 +201,7 @@ bool CGovernanceManager::SerializeVoteForHash(const uint256& nHash, CDataStream&
     LOCK(cs_store);
 
     CGovernanceObject* pGovobj = nullptr;
-    return cmapVoteToObject.Get(nHash, pGovobj) && pGovobj->GetVoteFile().SerializeVoteToStream(nHash, ss);
+    return cmapVoteToObject.Get(nHash, pGovobj) && WITH_LOCK(pGovobj->cs, return pGovobj->GetVoteFile().SerializeVoteToStream(nHash, ss));
 }
 
 void CGovernanceManager::AddPostponedObject(const CGovernanceObject& govobj)
@@ -744,10 +744,12 @@ MessageProcessingResult CGovernanceManager::SyncSingleObjVotes(CNode& peer, cons
         return {};
     }
 
-    const auto& fileVotes = govobj.GetVoteFile();
+    MessageProcessingResult ret{};
     const auto tip_mn_list = Assert(m_dmnman)->GetListAtChainTip();
 
-    MessageProcessingResult ret{};
+    {
+    LOCK(govobj.cs);
+    const auto& fileVotes = govobj.GetVoteFile();
     for (const auto& vote : fileVotes.GetVotes()) {
         uint256 nVoteHash = vote.GetHash();
 
@@ -758,6 +760,7 @@ MessageProcessingResult CGovernanceManager::SyncSingleObjVotes(CNode& peer, cons
         }
         ret.m_inventory.emplace_back(MSG_GOVERNANCE_OBJECT_VOTE, nVoteHash);
     }
+    }
 
     CNetMsgMaker msgMaker(peer.GetCommonVersion());
     connman.PushMessage(&peer, msgMaker.Make(NetMsgType::SYNCSTATUSCOUNT, MASTERNODE_SYNC_GOVOBJ_VOTE,
@@ -1064,7 +1067,7 @@ void CGovernanceManager::RequestGovernanceObject(CNode* pfrom, const uint256& nH
 
         if (pObj) {
             filter = CBloomFilter(Params().GetConsensus().nGovernanceFilterElements, GOVERNANCE_FILTER_FP_RATE, GetRand<int>(/*nMax=*/999999), BLOOM_UPDATE_ALL);
-            std::vector<CGovernanceVote> vecVotes = pObj->GetVoteFile().GetVotes();
+            std::vector<CGovernanceVote> vecVotes = WITH_LOCK(pObj->cs, return pObj->GetVoteFile().GetVotes());
             nVoteCount = vecVotes.size();
             for (const auto& vote : vecVotes) {
                 filter.insert(vote.GetHash());
@@ -1222,7 +1225,7 @@ void CGovernanceManager::RebuildIndexes()
     cmapVoteToObject.Clear();
     for (auto& objPair : mapObjects) {
         CGovernanceObject& govobj = objPair.second;
-        std::vector<CGovernanceVote> vecVotes = govobj.GetVoteFile().GetVotes();
+        std::vector<CGovernanceVote> vecVotes = WITH_LOCK(govobj.cs, return govobj.GetVoteFile().GetVotes());
         for (const auto& vecVote : vecVotes) {
             cmapVoteToObject.Insert(vecVote.GetHash(), &govobj);
         }

src/governance/object.cpp

@@ -614,6 +614,7 @@ void CGovernanceObject::UpdateSentinelVariables(const CDeterministicMNList& tip_
     if (GetAbsoluteYesCount(tip_mn_list, VOTE_SIGNAL_FUNDING) >= nAbsVoteReq) fCachedFunding = true;
     if ((GetAbsoluteYesCount(tip_mn_list, VOTE_SIGNAL_DELETE) >= nAbsDeleteReq) && !fCachedDelete) {
         fCachedDelete = true;
+        LOCK(cs);
         if (nDeletionTime == 0) {
             nDeletionTime = GetTime<std::chrono::seconds>().count();
         }

src/governance/object.h

@@ -88,14 +88,15 @@ class CGovernanceObject
 public: // Types
     using vote_m_t = std::map<COutPoint, vote_rec_t>;
 
-private:
+public:
     /// critical section to protect the inner data structures
     mutable RecursiveMutex cs;
 
+private:
     Governance::Object m_obj;
 
     /// time this object was marked for deletion
-    int64_t nDeletionTime{0};
+    int64_t nDeletionTime GUARDED_BY(cs){0};
 
     /// is valid by blockchain
     bool fCachedLocalValidity{false};
@@ -121,14 +122,14 @@ class CGovernanceObject
     bool fDirtyCache{true};
 
     /// Object is no longer of interest
-    bool fExpired{false};
+    bool fExpired GUARDED_BY(cs){false};
 
     /// Failed to parse object data
     bool fUnparsable{false};
 
-    vote_m_t mapCurrentMNVotes;
+    vote_m_t mapCurrentMNVotes GUARDED_BY(cs);
 
-    CGovernanceObjectVoteFile fileVotes;
+    CGovernanceObjectVoteFile fileVotes GUARDED_BY(cs);
 
 public:
     CGovernanceObject();
@@ -141,18 +142,31 @@ class CGovernanceObject
     bool IsSetCachedDelete() const { return fCachedDelete; }
     bool IsSetCachedEndorsed() const { return fCachedEndorsed; }
     bool IsSetDirtyCache() const { return fDirtyCache; }
-    bool IsSetExpired() const { return fExpired; }
+    bool IsSetExpired() const EXCLUSIVE_LOCKS_REQUIRED(!cs)
+    {
+        return WITH_LOCK(cs, return fExpired);
+    }
     GovernanceObject GetObjectType() const { return m_obj.type; }
     int64_t GetCreationTime() const { return m_obj.time; }
-    int64_t GetDeletionTime() const { return nDeletionTime; }
+    int64_t GetDeletionTime() const EXCLUSIVE_LOCKS_REQUIRED(!cs)
+    {
+        return WITH_LOCK(cs, return nDeletionTime);
+    }
 
-    const CGovernanceObjectVoteFile& GetVoteFile() const { return fileVotes; }
+    const CGovernanceObjectVoteFile& GetVoteFile() const EXCLUSIVE_LOCKS_REQUIRED(cs)
+    {
+        AssertLockHeld(cs);
+        return fileVotes;
+    }
     const COutPoint& GetMasternodeOutpoint() const { return m_obj.masternodeOutpoint; }
     const Governance::Object& Object() const { return m_obj; }
     const uint256& GetCollateralHash() const { return m_obj.collateralHash; }
 
     // Setters
-    void SetExpired() { fExpired = true; }
+    void SetExpired() EXCLUSIVE_LOCKS_REQUIRED(!cs)
+    {
+        WITH_LOCK(cs, fExpired = true);
+    }
     void SetMasternodeOutpoint(const COutPoint& outpoint);
     void SetSignature(Span<const uint8_t> sig);
 
@@ -177,9 +191,10 @@ class CGovernanceObject
 
     void UpdateSentinelVariables(const CDeterministicMNList& tip_mn_list);
 
-    void PrepareDeletion(int64_t nDeletionTime_)
+    void PrepareDeletion(int64_t nDeletionTime_) EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         fCachedDelete = true;
+        LOCK(cs);
         if (nDeletionTime == 0) {
             nDeletionTime = nDeletionTime_;
         }
@@ -211,15 +226,27 @@ class CGovernanceObject
 
     // SERIALIZER
 
-    SERIALIZE_METHODS(CGovernanceObject, obj)
+    template<typename Stream>
+    void Serialize(Stream& s) const EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         // SERIALIZE DATA FOR SAVING/LOADING OR NETWORK FUNCTIONS
-        READWRITE(obj.m_obj);
+        s << m_obj;
         if (s.GetType() & SER_DISK) {
             // Only include these for the disk file format
-            READWRITE(obj.nDeletionTime, obj.fExpired, obj.mapCurrentMNVotes, obj.fileVotes);
+            LOCK(cs);
+            s << nDeletionTime << fExpired << mapCurrentMNVotes << fileVotes;
         }
+    }
 
+    template<typename Stream>
+    void Unserialize(Stream& s) EXCLUSIVE_LOCKS_REQUIRED(!cs)
+    {
+        s >> m_obj;
+        if (s.GetType() & SER_DISK) {
+            // Only include these for the disk file format
+            LOCK(cs);
+            s >> nDeletionTime >> fExpired >> mapCurrentMNVotes >> fileVotes;
+        }
         // AFTER DESERIALIZATION OCCURS, CACHED VARIABLES MUST BE CALCULATED MANUALLY
     }