Rework govobject/trigger cleanup a bit (#3070)

* No friends for `CGovernanceObject`

* Simplify CGovernanceTriggerManager::CleanAndRemove() logic and make sure CSuperblock::IsExpired() does not modify the state

Author: UdjinM6

src/governance/governance-classes.cpp

@@ -143,7 +143,7 @@ void CGovernanceTriggerManager::CleanAndRemove()
         CGovernanceObject* pObj = nullptr;
         CSuperblock_sptr& pSuperblock = it->second;
         if (!pSuperblock) {
-            LogPrint(BCLog::GOBJECT, "CGovernanceTriggerManager::CleanAndRemove -- nullptr superblock marked for removal\n");
+            LogPrint(BCLog::GOBJECT, "CGovernanceTriggerManager::CleanAndRemove -- nullptr superblock\n");
             remove = true;
         } else {
             pObj = governance.FindGovernanceObject(it->first);
@@ -160,9 +160,15 @@ void CGovernanceTriggerManager::CleanAndRemove()
                 remove = true;
                 break;
             case SEEN_OBJECT_IS_VALID:
-            case SEEN_OBJECT_EXECUTED:
-                remove = pSuperblock->IsExpired();
+            case SEEN_OBJECT_EXECUTED: {
+                LogPrint(BCLog::GOBJECT, "CGovernanceTriggerManager::CleanAndRemove -- Valid trigger found\n");
+                if (pSuperblock->IsExpired()) {
+                    // update corresponding object
+                    pObj->SetExpired();
+                    remove = true;
+                }
                 break;
+            }
             default:
                 break;
             }
@@ -173,15 +179,10 @@ void CGovernanceTriggerManager::CleanAndRemove()
             std::string strDataAsPlainString = "nullptr";
             if (pObj) {
                 strDataAsPlainString = pObj->GetDataAsPlainString();
+                // mark corresponding object for deletion
+                pObj->PrepareDeletion(GetAdjustedTime());
             }
             LogPrint(BCLog::GOBJECT, "CGovernanceTriggerManager::CleanAndRemove -- Removing trigger object %s\n", strDataAsPlainString);
-            // mark corresponding object for deletion
-            if (pObj) {
-                pObj->fCachedDelete = true;
-                if (pObj->nDeletionTime == 0) {
-                    pObj->nDeletionTime = GetAdjustedTime();
-                }
-            }
             // delete the trigger
             mapTrigger.erase(it++);
         } else {
@@ -662,9 +663,8 @@ bool CSuperblock::IsValid(const CTransaction& txNew, int nBlockHeight, CAmount b
     return true;
 }
 
-bool CSuperblock::IsExpired()
+bool CSuperblock::IsExpired() const
 {
-    bool fExpired{false};
     int nExpirationBlocks{0};
     // Executed triggers are kept for another superblock cycle (approximately 1 month),
     // other valid triggers are kept for ~1 day only, everything else is pruned after ~1h.
@@ -686,16 +686,10 @@ bool CSuperblock::IsExpired()
 
     if (governance.GetCachedBlockHeight() > nExpirationBlock) {
         LogPrint(BCLog::GOBJECT, "CSuperblock::IsExpired -- Outdated trigger found\n");
-        fExpired = true;
-        CGovernanceObject* pgovobj = GetGovernanceObject();
-        if (pgovobj) {
-            LogPrint(BCLog::GOBJECT, "CSuperblock::IsExpired -- Expiring outdated object: %s\n", pgovobj->GetHash().ToString());
-            pgovobj->fExpired = true;
-            pgovobj->nDeletionTime = GetAdjustedTime();
-        }
+        return true;
     }
 
-    return fExpired;
+    return false;
 }
 
 /**

src/governance/governance-classes.h

@@ -172,7 +172,7 @@ class CSuperblock : public CGovernanceObject
     CAmount GetPaymentsTotalAmount();
 
     bool IsValid(const CTransaction& txNew, int nBlockHeight, CAmount blockReward);
-    bool IsExpired();
+    bool IsExpired() const;
 };
 
 #endif

src/governance/governance-object.cpp

@@ -227,6 +227,7 @@ void CGovernanceObject::ClearMasternodeVotes()
         if (!mnList.HasMNByCollateral(it->first)) {
             fileVotes.RemoveVotesFromMasternode(it->first);
             mapCurrentMNVotes.erase(it++);
+            fDirtyCache = true;
         } else {
             ++it;
         }

src/governance/governance-object.h

@@ -107,10 +107,6 @@ struct vote_rec_t {
 
 class CGovernanceObject
 {
-    friend class CGovernanceManager;
-    friend class CGovernanceTriggerManager;
-    friend class CSuperblock;
-
 public: // Types
     typedef std::map<COutPoint, vote_rec_t> vote_m_t;
 
@@ -246,6 +242,11 @@ class CGovernanceObject
         return fExpired;
     }
 
+    void SetExpired()
+    {
+        fExpired = true;
+    }
+
     const CGovernanceObjectVoteFile& GetVoteFile() const
     {
         return fileVotes;
@@ -273,6 +274,14 @@ class CGovernanceObject
 
     void UpdateSentinelVariables();
 
+    void PrepareDeletion(int64_t nDeletionTime_)
+    {
+        fCachedDelete = true;
+        if (nDeletionTime == 0) {
+            nDeletionTime = nDeletionTime_;
+        }
+    }
+
     CAmount GetMinCollateralFee() const;
 
     UniValue GetJSONObject();
@@ -329,7 +338,6 @@ class CGovernanceObject
         // AFTER DESERIALIZATION OCCURS, CACHED VARIABLES MUST BE CALCULATED MANUALLY
     }
 
-private:
     // FUNCTIONS FOR DEALING WITH DATA STRING
     void LoadData();
     void GetData(UniValue& objResult);

src/governance/governance.cpp

@@ -310,16 +310,12 @@ void CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj, CConnman
     // SHOULD WE ADD THIS OBJECT TO ANY OTHER MANANGERS?
 
     LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- Before trigger block, GetDataAsPlainString = %s, nObjectType = %d\n",
-                govobj.GetDataAsPlainString(), govobj.nObjectType);
+                govobj.GetDataAsPlainString(), govobj.GetObjectType());
 
-    if (govobj.nObjectType == GOVERNANCE_OBJECT_TRIGGER) {
+    if (govobj.GetObjectType() == GOVERNANCE_OBJECT_TRIGGER) {
         if (!triggerman.AddNewTrigger(nHash)) {
             LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- undo adding invalid trigger object: hash = %s\n", nHash.ToString());
-            CGovernanceObject& objref = objpair.first->second;
-            objref.fCachedDelete = true;
-            if (objref.nDeletionTime == 0) {
-                objref.nDeletionTime = GetAdjustedTime();
-            }
+            objpair.first->second.PrepareDeletion(GetAdjustedTime());
             return;
         }
     }
@@ -355,7 +351,6 @@ void CGovernanceManager::UpdateCachesAndClean()
             continue;
         }
         it->second.ClearMasternodeVotes();
-        it->second.fDirtyCache = true;
     }
 
     ScopedLockBool guard(cs, fRateChecksEnabled, false);
@@ -429,10 +424,7 @@ void CGovernanceManager::UpdateCachesAndClean()
                 CProposalValidator validator(pObj->GetDataAsHexString(), true);
                 if (!validator.Validate()) {
                     LogPrintf("CGovernanceManager::UpdateCachesAndClean -- set for deletion expired obj %s\n", strHash);
-                    pObj->fCachedDelete = true;
-                    if (pObj->nDeletionTime == 0) {
-                        pObj->nDeletionTime = nNow;
-                    }
+                    pObj->PrepareDeletion(nNow);
                 }
             }
             ++it;
@@ -999,7 +991,7 @@ int CGovernanceManager::RequestGovernanceObjectVotes(const std::vector<CNode*>&
                 if (mapAskedRecently[nHash].size() >= nPeersPerHashMax) continue;
             }
 
-            if (objPair.second.nObjectType == GOVERNANCE_OBJECT_TRIGGER) {
+            if (objPair.second.GetObjectType() == GOVERNANCE_OBJECT_TRIGGER) {
                 vTriggerObjHashes.push_back(nHash);
             } else {
                 vOtherObjHashes.push_back(nHash);
@@ -1107,15 +1099,12 @@ void CGovernanceManager::AddCachedTriggers()
     for (auto& objpair : mapObjects) {
         CGovernanceObject& govobj = objpair.second;
 
-        if (govobj.nObjectType != GOVERNANCE_OBJECT_TRIGGER) {
+        if (govobj.GetObjectType() != GOVERNANCE_OBJECT_TRIGGER) {
             continue;
         }
 
         if (!triggerman.AddNewTrigger(govobj.GetHash())) {
-            govobj.fCachedDelete = true;
-            if (govobj.nDeletionTime == 0) {
-                govobj.nDeletionTime = GetAdjustedTime();
-            }
+            govobj.PrepareDeletion(GetAdjustedTime());
         }
     }
 }