more 16426: governance

Author: UdjinM6

src/governance/governance.cpp

@@ -48,13 +48,13 @@ CGovernanceManager::CGovernanceManager() :
 // Accessors for thread-safe access to maps
 bool CGovernanceManager::HaveObjectForHash(const uint256& nHash) const
 {
-    LOCK(cs);
+    LOCK2(cs, cs_postponed);
     return (mapObjects.count(nHash) == 1 || mapPostponedObjects.count(nHash) == 1);
 }
 
 bool CGovernanceManager::SerializeObjectForHash(const uint256& nHash, CDataStream& ss) const
 {
-    LOCK(cs);
+    LOCK2(cs, cs_postponed);
     auto it = mapObjects.find(nHash);
     if (it == mapObjects.end()) {
         it = mapPostponedObjects.find(nHash);
@@ -143,15 +143,17 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, const std::string& msg_typ
             return;
         }
 
-        LOCK2(cs_main, cs);
+        bool fRateCheckBypassed{false};
+        {
+            LOCK2(cs, cs_postponed);
 
-        if (mapObjects.count(nHash) || mapPostponedObjects.count(nHash) || mapErasedGovernanceObjects.count(nHash)) {
-            // TODO - print error code? what if it's GOVOBJ_ERROR_IMMATURE?
-            LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECT -- Received already seen object: %s\n", strHash);
-            return;
+            if (mapObjects.count(nHash) || mapPostponedObjects.count(nHash) || mapErasedGovernanceObjects.count(nHash)) {
+                // TODO - print error code? what if it's GOVOBJ_ERROR_IMMATURE?
+                LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECT -- Received already seen object: %s\n", strHash);
+                return;
+            }
         }
 
-        bool fRateCheckBypassed = false;
         if (!MasternodeRateCheck(govobj, true, false, fRateCheckBypassed)) {
             LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECT -- masternode rate check failed - %s - (current block height %d) \n", strHash, nCachedBlockHeight);
             return;
@@ -175,6 +177,7 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, const std::string& msg_typ
             } else {
                 LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECT -- Governance object is invalid - %s\n", strError);
                 // apply node's ban score
+                LOCK(cs_main);
                 Misbehaving(pfrom->GetId(), 20);
             }
 
@@ -230,6 +233,8 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, const std::string& msg_typ
 
 void CGovernanceManager::CheckOrphanVotes(CGovernanceObject& govobj, CConnman& connman)
 {
+    LOCK(cs);
+
     uint256 nHash = govobj.GetHash();
     std::vector<vote_time_pair_t> vecVotePairs;
     cmmapOrphanVotes.GetAll(nHash, vecVotePairs);
@@ -262,11 +267,9 @@ void CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj, CConnman
 
     govobj.UpdateSentinelVariables(); //this sets local vars in object
 
-    LOCK2(cs_main, cs);
-    std::string strError;
-
     // MAKE SURE THIS OBJECT IS OK
 
+    std::string strError;
     if (!govobj.IsValidLocally(strError, true)) {
         LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- invalid governance object - %s - (nCachedBlockHeight %d) \n", strError, nCachedBlockHeight);
         return;
@@ -276,7 +279,7 @@ void CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj, CConnman
 
     // INSERT INTO OUR GOVERNANCE OBJECT MEMORY
     // IF WE HAVE THIS OBJECT ALREADY, WE DON'T WANT ANOTHER COPY
-    auto objpair = mapObjects.emplace(nHash, govobj);
+    auto objpair = WITH_LOCK(cs, return mapObjects.emplace(nHash, govobj));
 
     if (!objpair.second) {
         LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- already have governance object %s\n", nHash.ToString());
@@ -321,7 +324,7 @@ void CGovernanceManager::UpdateCachesAndClean()
 
     std::vector<uint256> vecDirtyHashes = mmetaman.GetAndClearDirtyGovernanceObjectHashes();
 
-    LOCK2(cs_main, cs);
+    LOCK(cs);
 
     for (const uint256& nHash : vecDirtyHashes) {
         auto it = mapObjects.find(nHash);
@@ -534,6 +537,7 @@ bool CGovernanceManager::ConfirmInventoryRequest(const CInv& inv)
     // First check if we've already recorded this object
     switch (inv.type) {
     case MSG_GOVERNANCE_OBJECT: {
+        LOCK(cs_postponed);
         if (mapObjects.count(inv.hash) == 1 || mapPostponedObjects.count(inv.hash) == 1) {
             LogPrint(BCLog::GOBJECT, "CGovernanceManager::ConfirmInventoryRequest already have governance object, returning false\n");
             return false;
@@ -836,8 +840,8 @@ void CGovernanceManager::CheckPostponedObjects(CConnman& connman)
 {
     if (!masternodeSync.IsSynced()) return;
 
-    LOCK2(cs_main, cs);
-
+    {
+    LOCK(cs_postponed);
     // Check postponed proposals
     for (auto it = mapPostponedObjects.begin(); it != mapPostponedObjects.end();) {
         const uint256& nHash = it->first;
@@ -863,7 +867,9 @@ void CGovernanceManager::CheckPostponedObjects(CConnman& connman)
         // remove processed or invalid object from the queue
         mapPostponedObjects.erase(it++);
     }
+    } // LOCK(cs_postponed)
 
+    LOCK(cs);
 
     // Perform additional relays for triggers
     int64_t nNow = GetAdjustedTime();

src/governance/governance.h

@@ -174,7 +174,9 @@ class CGovernanceManager
     //   value - expiration time for deleted objects
     std::map<uint256, int64_t> mapErasedGovernanceObjects;
 
-    std::map<uint256, CGovernanceObject> mapPostponedObjects;
+    mutable CCriticalSection cs_postponed;
+    std::map<uint256, CGovernanceObject> mapPostponedObjects GUARDED_BY(cs_postponed);
+
     hash_s_t setAdditionalRelayObjects;
 
     object_ref_cm_t cmapVoteToObject;
@@ -316,7 +318,7 @@ class CGovernanceManager
 
     void AddPostponedObject(const CGovernanceObject& govobj)
     {
-        LOCK(cs);
+        LOCK(cs_postponed);
         mapPostponedObjects.insert(std::make_pair(govobj.GetHash(), govobj));
     }
 

src/governance/object.cpp

@@ -438,7 +438,6 @@ UniValue CGovernanceObject::ToJson() const
 
 void CGovernanceObject::UpdateLocalValidity()
 {
-    LOCK(cs_main);
     // THIS DOES NOT CHECK COLLATERAL, THIS IS CHECKED UPON ORIGINAL ARRIVAL
     fCachedLocalValidity = IsValidLocally(strLocalValidityError, false);
 }
@@ -453,8 +452,6 @@ bool CGovernanceObject::IsValidLocally(std::string& strError, bool fCheckCollate
 
 bool CGovernanceObject::IsValidLocally(std::string& strError, bool& fMissingConfirmations, bool fCheckCollateral) const
 {
-    AssertLockHeld(cs_main);
-
     fMissingConfirmations = false;
 
     if (fUnparsable) {
@@ -464,7 +461,7 @@ bool CGovernanceObject::IsValidLocally(std::string& strError, bool& fMissingConf
 
     switch (nObjectType) {
     case GOVERNANCE_OBJECT_PROPOSAL: {
-        bool fAllowScript = (VersionBitsTipState(Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0024) == ThresholdState::ACTIVE);
+        bool fAllowScript = WITH_LOCK(cs_main, return VersionBitsTipState(Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0024) == ThresholdState::ACTIVE);
         bool fAllowLegacyFormat = !fAllowScript; // reusing the same bit to stop accepting proposals in legacy format
         CProposalValidator validator(GetDataAsHexString(), fAllowLegacyFormat, fAllowScript);
         // Note: It's ok to have expired proposals
@@ -558,8 +555,8 @@ bool CGovernanceObject::IsCollateralValid(std::string& strError, bool& fMissingC
     CScript findScript;
     findScript << OP_RETURN << ToByteVector(nExpectedHash);
 
-    AssertLockHeld(cs_main);
-    bool fork_active = VersionBitsState(LookupBlockIndex(nBlockHash), Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0024, versionbitscache) == ThresholdState::ACTIVE;
+    const CBlockIndex* pindex = WITH_LOCK(cs_main, return LookupBlockIndex(nBlockHash));
+    bool fork_active = WITH_LOCK(cs_main, return VersionBitsState(pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0024, versionbitscache) == ThresholdState::ACTIVE);
     CAmount nMinFee = GetMinCollateralFee(fork_active);
 
     LogPrint(BCLog::GOBJECT, "CGovernanceObject::IsCollateralValid -- txCollateral->vout.size() = %s, findScript = %s, nMinFee = %lld\n",
@@ -587,13 +584,9 @@ bool CGovernanceObject::IsCollateralValid(std::string& strError, bool& fMissingC
 
     // GET CONFIRMATIONS FOR TRANSACTION
 
-    AssertLockHeld(cs_main);
     int nConfirmationsIn = 0;
-    if (nBlockHash != uint256()) {
-        const CBlockIndex* pindex = LookupBlockIndex(nBlockHash);
-        if (pindex && ::ChainActive().Contains(pindex)) {
-            nConfirmationsIn += ::ChainActive().Height() - pindex->nHeight + 1;
-        }
+    if (::ChainActive().Contains(pindex)) {
+        nConfirmationsIn += ::ChainActive().Height() - pindex->nHeight + 1;
     }
 
     if (nConfirmationsIn < GOVERNANCE_FEE_CONFIRMATIONS) {

src/governance/object.h

@@ -240,12 +240,12 @@ class CGovernanceObject
 
     // CORE OBJECT FUNCTIONS
 
-    bool IsValidLocally(std::string& strError, bool fCheckCollateral) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool IsValidLocally(std::string& strError, bool fCheckCollateral) const;
 
-    bool IsValidLocally(std::string& strError, bool& fMissingConfirmations, bool fCheckCollateral) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool IsValidLocally(std::string& strError, bool& fMissingConfirmations, bool fCheckCollateral) const;
 
     /// Check the collateral transaction for the budget proposal/finalized budget
-    bool IsCollateralValid(std::string& strError, bool& fMissingConfirmations) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool IsCollateralValid(std::string& strError, bool& fMissingConfirmations) const;
 
     void UpdateLocalValidity();
 

src/qt/governancelist.cpp

@@ -64,7 +64,6 @@ QString Proposal::url() const { return m_url; }
 
 bool Proposal::isActive() const
 {
-    LOCK(cs_main);
     std::string strError;
     return govObj.IsValidLocally(strError, false);
 }

src/rpc/governance.cpp

@@ -188,11 +188,10 @@ static UniValue gobject_prepare(const JSONRPCRequest& request)
                 request.params[3].getValStr(), request.params[4].getValStr(),
                 govobj.GetDataAsPlainString(), govobj.GetHash().ToString());
 
+    bool fDIP0024IsActive = WITH_LOCK(cs_main, return VersionBitsTipState(Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0024) == ThresholdState::ACTIVE);
     if (govobj.GetObjectType() == GOVERNANCE_OBJECT_PROPOSAL) {
-        LOCK(cs_main);
-        bool fAllowScript = (VersionBitsTipState(Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0024) == ThresholdState::ACTIVE);
         // Note: we do not allow legacy format in RPC already, no need to reuse DEPLOYMENT_DIP0024
-        CProposalValidator validator(strDataHex, false, fAllowScript);
+        CProposalValidator validator(strDataHex, false, fDIP0024IsActive);
         if (!validator.Validate()) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid proposal data, error messages:" + validator.GetErrorMessages());
         }
@@ -208,11 +207,9 @@ static UniValue gobject_prepare(const JSONRPCRequest& request)
 
     LOCK(pwallet->cs_wallet);
 
-    {
-        LOCK(cs_main);
-        std::string strError = "";
-        if (!govobj.IsValidLocally(strError, false))
-            throw JSONRPCError(RPC_INTERNAL_ERROR, "Governance object is not valid - " + govobj.GetHash().ToString() + " - " + strError);
+    std::string strError = "";
+    if (!govobj.IsValidLocally(strError, false)) {
+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Governance object is not valid - " + govobj.GetHash().ToString() + " - " + strError);
     }
 
     // If specified, spend this outpoint as the proposal fee
@@ -229,9 +226,7 @@ static UniValue gobject_prepare(const JSONRPCRequest& request)
 
     CTransactionRef tx;
 
-    bool fork_active = VersionBitsTipState(Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0024) == ThresholdState::ACTIVE;
-
-    if (!pwallet->GetBudgetSystemCollateralTX(tx, govobj.GetHash(), govobj.GetMinCollateralFee(fork_active), outpoint)) {
+    if (!pwallet->GetBudgetSystemCollateralTX(tx, govobj.GetHash(), govobj.GetMinCollateralFee(fDIP0024IsActive), outpoint)) {
         std::string err = "Error making collateral transaction for governance object. Please check your wallet balance and make sure your wallet is unlocked.";
         if (!request.params[6].isNull() && !request.params[7].isNull()) {
             err += "Please verify your specified output is valid and is enough for the combined proposal fee and transaction fee.";
@@ -363,8 +358,7 @@ static UniValue gobject_submit(const JSONRPCRequest& request)
                 govobj.GetDataAsPlainString(), govobj.GetHash().ToString(), txidFee.ToString());
 
     if (govobj.GetObjectType() == GOVERNANCE_OBJECT_PROPOSAL) {
-        LOCK(cs_main);
-        bool fAllowScript = (VersionBitsTipState(Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0024) == ThresholdState::ACTIVE);
+        bool fAllowScript = WITH_LOCK(cs_main, return VersionBitsTipState(Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0024) == ThresholdState::ACTIVE);
         // Note: we do not allow legacy format in RPC already, no need to reuse DEPLOYMENT_DIP0024
         CProposalValidator validator(strDataHex, false, fAllowScript);
         if (!validator.Validate()) {
@@ -389,18 +383,15 @@ static UniValue gobject_submit(const JSONRPCRequest& request)
 
     std::string strHash = govobj.GetHash().ToString();
 
+    if (g_txindex) {
+        g_txindex->BlockUntilSyncedToCurrentChain();
+    }
+
     std::string strError = "";
     bool fMissingConfirmations;
-    {
-        if (g_txindex) {
-            g_txindex->BlockUntilSyncedToCurrentChain();
-        }
-
-        LOCK(cs_main);
-        if (!govobj.IsValidLocally(strError, fMissingConfirmations, true) && !fMissingConfirmations) {
-            LogPrintf("gobject(submit) -- Object submission rejected because object is not valid - hash = %s, strError = %s\n", strHash, strError);
-            throw JSONRPCError(RPC_INTERNAL_ERROR, "Governance object is not valid - " + strHash + " - " + strError);
-        }
+    if (!govobj.IsValidLocally(strError, fMissingConfirmations, true) && !fMissingConfirmations) {
+        LogPrintf("gobject(submit) -- Object submission rejected because object is not valid - hash = %s, strError = %s\n", strHash, strError);
+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Governance object is not valid - " + strHash + " - " + strError);
     }
 
     // RELAY THIS OBJECT
@@ -724,7 +715,7 @@ static UniValue ListObjects(const std::string& strCachedSignal, const std::strin
         g_txindex->BlockUntilSyncedToCurrentChain();
     }
 
-    LOCK2(cs_main, governance.cs);
+    LOCK(governance.cs);
 
     std::vector<CGovernanceObject> objs = governance.GetAllNewerThan(nStartTime);
     governance.UpdateLastDiffTime(GetTime());
@@ -867,7 +858,7 @@ static UniValue gobject_get(const JSONRPCRequest& request)
     }
 
     // FIND THE GOVERNANCE OBJECT THE USER IS LOOKING FOR
-    LOCK2(cs_main, governance.cs);
+    LOCK(governance.cs);
     CGovernanceObject* pGovObj = governance.FindGovernanceObject(hash);
 
     if (pGovObj == nullptr) {
@@ -1182,14 +1173,13 @@ static UniValue getgovernanceinfo(const JSONRPCRequest& request)
     }
 
     int nLastSuperblock = 0, nNextSuperblock = 0;
-    const auto* pindex = WITH_LOCK(cs_main, return ::ChainActive().Tip());
-    int nBlockHeight = pindex->nHeight;
+    const auto* pindex = ::ChainActive().Tip();
+    bool fork_active = WITH_LOCK(cs_main, return VersionBitsState(pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0024, versionbitscache) == ThresholdState::ACTIVE);
 
-    CSuperblock::GetNearestSuperblocksHeights(nBlockHeight, nLastSuperblock, nNextSuperblock);
+    CSuperblock::GetNearestSuperblocksHeights(pindex->nHeight, nLastSuperblock, nNextSuperblock);
 
     UniValue obj(UniValue::VOBJ);
     obj.pushKV("governanceminquorum", Params().GetConsensus().nGovernanceMinQuorum);
-    bool fork_active = VersionBitsState(pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0024, versionbitscache) == ThresholdState::ACTIVE;
     obj.pushKV("proposalfee", ValueFromAmount(fork_active ? GOVERNANCE_PROPOSAL_FEE_TX : GOVERNANCE_PROPOSAL_FEE_TX_OLD));
     obj.pushKV("superblockcycle", Params().GetConsensus().nSuperblockCycle);
     obj.pushKV("lastsuperblock", nLastSuperblock);