From 751077d6a92a8b112bb4d75037850ca9eeac89ae Mon Sep 17 00:00:00 2001 From: Pasta Date: Sun, 26 Sep 2021 17:22:31 -0400 Subject: [PATCH 01/10] governance: use more atomic, adjust mutexes --- src/governance/classes.h | 10 ++++----- src/governance/governance.cpp | 16 ------------- src/governance/governance.h | 30 ++++++++++++------------- src/governance/object.cpp | 42 ++--------------------------------- src/governance/object.h | 40 ++++++++++++++++----------------- 5 files changed, 42 insertions(+), 96 deletions(-) diff --git a/src/governance/classes.h b/src/governance/classes.h index f106f1d8b8ee..3ae0761ffc14 100644 --- a/src/governance/classes.h +++ b/src/governance/classes.h @@ -37,9 +37,9 @@ class CGovernanceTriggerManager private: std::map mapTrigger; - std::vector GetActiveTriggers(); - bool AddNewTrigger(uint256 nHash); - void CleanAndRemove(); + std::vector GetActiveTriggers() EXCLUSIVE_LOCKS_REQUIRED(governance.cs); + bool AddNewTrigger(uint256 nHash) EXCLUSIVE_LOCKS_REQUIRED(governance.cs); + void CleanAndRemove() EXCLUSIVE_LOCKS_REQUIRED(governance.cs); public: CGovernanceTriggerManager() : @@ -55,7 +55,7 @@ class CGovernanceTriggerManager class CSuperblockManager { private: - static bool GetBestSuperblock(CSuperblock_sptr& pSuperblockRet, int nBlockHeight); + static bool GetBestSuperblock(CSuperblock_sptr& pSuperblockRet, int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(governance.cs); public: static bool IsSuperblockTriggered(int nBlockHeight); @@ -135,7 +135,7 @@ class CSuperblock : public CGovernanceObject // TELL THE ENGINE WE EXECUTED THIS EVENT void SetExecuted() { nStatus = SEEN_OBJECT_EXECUTED; } - CGovernanceObject* GetGovernanceObject(); + CGovernanceObject* GetGovernanceObject() EXCLUSIVE_LOCKS_REQUIRED(governance.cs); int GetBlockHeight() const { diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index b3996b20aba4..305ad505fb76 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -28,22 +28,6 @@ const std::string CGovernanceManager::SERIALIZATION_VERSION_STRING = "CGovernanc const int CGovernanceManager::MAX_TIME_FUTURE_DEVIATION = 60 * 60; const int CGovernanceManager::RELIABLE_PROPAGATION_TIME = 60; -CGovernanceManager::CGovernanceManager() : - nTimeLastDiff(0), - nCachedBlockHeight(0), - mapObjects(), - mapErasedGovernanceObjects(), - cmapVoteToObject(MAX_CACHE_SIZE), - cmapInvalidVotes(MAX_CACHE_SIZE), - cmmapOrphanVotes(MAX_CACHE_SIZE), - mapLastMasternodeObject(), - setRequestedObjects(), - fRateChecksEnabled(true), - lastMNListForVotingKeys(std::make_shared()), - cs() -{ -} - // Accessors for thread-safe access to maps bool CGovernanceManager::HaveObjectForHash(const uint256& nHash) const { diff --git a/src/governance/governance.h b/src/governance/governance.h index 8d30df7cc686..d4ece4894dee 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -161,38 +161,38 @@ class CGovernanceManager static const int MAX_TIME_FUTURE_DEVIATION; static const int RELIABLE_PROPAGATION_TIME; - int64_t nTimeLastDiff; + int64_t nTimeLastDiff GUARDED_BY(cs); // keep track of current block height - int nCachedBlockHeight; + int nCachedBlockHeight GUARDED_BY(cs); // keep track of the scanning errors - std::map mapObjects; + std::map mapObjects GUARDED_BY(cs); // mapErasedGovernanceObjects contains key-value pairs, where // key - governance object's hash // value - expiration time for deleted objects - std::map mapErasedGovernanceObjects; + std::map mapErasedGovernanceObjects GUARDED_BY(cs); - std::map mapPostponedObjects; - hash_s_t setAdditionalRelayObjects; + std::map mapPostponedObjects GUARDED_BY(cs); + hash_s_t setAdditionalRelayObjects GUARDED_BY(cs); - object_ref_cm_t cmapVoteToObject; + object_ref_cm_t cmapVoteToObject GUARDED_BY(cs) {MAX_CACHE_SIZE}; - CacheMap cmapInvalidVotes; + CacheMap cmapInvalidVotes GUARDED_BY(cs) {MAX_CACHE_SIZE}; - vote_cmm_t cmmapOrphanVotes; + vote_cmm_t cmmapOrphanVotes GUARDED_BY(cs) {MAX_CACHE_SIZE}; - txout_m_t mapLastMasternodeObject; + txout_m_t mapLastMasternodeObject GUARDED_BY(cs); - hash_s_t setRequestedObjects; + hash_s_t setRequestedObjects GUARDED_BY(cs); - hash_s_t setRequestedVotes; + hash_s_t setRequestedVotes GUARDED_BY(cs); - bool fRateChecksEnabled; + bool fRateChecksEnabled GUARDED_BY(cs) {true}; // used to check for changed voting keys - CDeterministicMNListPtr lastMNListForVotingKeys; + CDeterministicMNListPtr lastMNListForVotingKeys GUARDED_BY(cs); class ScopedLockBool { @@ -218,7 +218,7 @@ class CGovernanceManager // critical section to protect the inner data structures mutable CCriticalSection cs; - CGovernanceManager(); + CGovernanceManager() = default; virtual ~CGovernanceManager() = default; diff --git a/src/governance/object.cpp b/src/governance/object.cpp index b482a0348964..f5f36a7155a8 100644 --- a/src/governance/object.cpp +++ b/src/governance/object.cpp @@ -17,62 +17,24 @@ #include -CGovernanceObject::CGovernanceObject() : - cs(), - nObjectType(GOVERNANCE_OBJECT_UNKNOWN), - nHashParent(), - nRevision(0), - nTime(0), - nDeletionTime(0), - nCollateralHash(), - vchData(), - masternodeOutpoint(), - vchSig(), - fCachedLocalValidity(false), - strLocalValidityError(), - fCachedFunding(false), - fCachedValid(true), - fCachedDelete(false), - fCachedEndorsed(false), - fDirtyCache(true), - fExpired(false), - fUnparsable(false), - mapCurrentMNVotes(), - fileVotes() +CGovernanceObject::CGovernanceObject() { // PARSE JSON DATA STORAGE (VCHDATA) LoadData(); } CGovernanceObject::CGovernanceObject(const uint256& nHashParentIn, int nRevisionIn, int64_t nTimeIn, const uint256& nCollateralHashIn, const std::string& strDataHexIn) : - cs(), - nObjectType(GOVERNANCE_OBJECT_UNKNOWN), nHashParent(nHashParentIn), nRevision(nRevisionIn), nTime(nTimeIn), - nDeletionTime(0), nCollateralHash(nCollateralHashIn), - vchData(ParseHex(strDataHexIn)), - masternodeOutpoint(), - vchSig(), - fCachedLocalValidity(false), - strLocalValidityError(), - fCachedFunding(false), - fCachedValid(true), - fCachedDelete(false), - fCachedEndorsed(false), - fDirtyCache(true), - fExpired(false), - fUnparsable(false), - mapCurrentMNVotes(), - fileVotes() + vchData(ParseHex(strDataHexIn)) { // PARSE JSON DATA STORAGE (VCHDATA) LoadData(); } CGovernanceObject::CGovernanceObject(const CGovernanceObject& other) : - cs(), nObjectType(other.nObjectType), nHashParent(other.nHashParent), nRevision(other.nRevision), diff --git a/src/governance/object.h b/src/governance/object.h index 196399688068..0aff1784519c 100644 --- a/src/governance/object.h +++ b/src/governance/object.h @@ -98,62 +98,62 @@ class CGovernanceObject mutable CCriticalSection cs; /// Object typecode - int nObjectType; + int nObjectType GUARDED_BY(cs) {GOVERNANCE_OBJECT_UNKNOWN}; /// parent object, 0 is root - uint256 nHashParent; + uint256 nHashParent GUARDED_BY(cs); /// object revision in the system - int nRevision; + int nRevision GUARDED_BY(cs) {0}; /// time this object was created - int64_t nTime; + int64_t nTime GUARDED_BY(cs) {0}; /// time this object was marked for deletion - int64_t nDeletionTime; + int64_t nDeletionTime GUARDED_BY(cs) {0}; /// fee-tx - uint256 nCollateralHash; + uint256 nCollateralHash GUARDED_BY(cs); /// Data field - can be used for anything - std::vector vchData; + std::vector vchData GUARDED_BY(cs); /// Masternode info for signed objects - COutPoint masternodeOutpoint; - std::vector vchSig; + COutPoint masternodeOutpoint GUARDED_BY(cs); + std::vector vchSig GUARDED_BY(cs); /// is valid by blockchain - bool fCachedLocalValidity; - std::string strLocalValidityError; + bool fCachedLocalValidity GUARDED_BY(cs) {false}; + std::string strLocalValidityError GUARDED_BY(cs); // VARIOUS FLAGS FOR OBJECT / SET VIA MASTERNODE VOTING /// true == minimum network support has been reached for this object to be funded (doesn't mean it will for sure though) - bool fCachedFunding; + bool fCachedFunding GUARDED_BY(cs) {false}; /// true == minimum network has been reached flagging this object as a valid and understood governance object (e.g, the serialized data is correct format, etc) - bool fCachedValid; + bool fCachedValid GUARDED_BY(cs) {true}; /// true == minimum network support has been reached saying this object should be deleted from the system entirely - bool fCachedDelete; + bool fCachedDelete GUARDED_BY(cs) {false}; /** true == minimum network support has been reached flagging this object as endorsed by an elected representative body * (e.g. business review board / technical review board /etc) */ - bool fCachedEndorsed; + bool fCachedEndorsed GUARDED_BY(cs) {false}; /// object was updated and cached values should be updated soon - bool fDirtyCache; + bool fDirtyCache GUARDED_BY(cs) {false}; /// Object is no longer of interest - bool fExpired; + bool fExpired GUARDED_BY(cs) {false}; /// Failed to parse object data - bool fUnparsable; + bool fUnparsable GUARDED_BY(cs) {false}; - vote_m_t mapCurrentMNVotes; + vote_m_t mapCurrentMNVotes GUARDED_BY(cs); - CGovernanceObjectVoteFile fileVotes; + CGovernanceObjectVoteFile fileVotes GUARDED_BY(cs); public: CGovernanceObject(); From a5b6114a8b1eac79f9bb43f2d1fbc3fd2ebbd047 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 28 Sep 2021 15:15:46 -0400 Subject: [PATCH 02/10] fix Signed-off-by: pasta --- src/governance/governance.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/governance/governance.h b/src/governance/governance.h index d4ece4894dee..9b8d109ec071 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -7,6 +7,7 @@ #include #include +#include #include class CBloomFilter; @@ -18,12 +19,14 @@ class CGovernanceTriggerManager; class CGovernanceObject; class CGovernanceVote; +using CDeterministicMNListPtr = std::shared_ptr; + extern CGovernanceManager governance; static const int RATE_BUFFER_SIZE = 5; class CDeterministicMNList; -using CDeterministicMNListPtr = std::shared_ptr; +typedef std::shared_ptr CDeterministicMNListPtr; class CRateCheckBuffer { @@ -192,7 +195,7 @@ class CGovernanceManager bool fRateChecksEnabled GUARDED_BY(cs) {true}; // used to check for changed voting keys - CDeterministicMNListPtr lastMNListForVotingKeys GUARDED_BY(cs); + CDeterministicMNListPtr lastMNListForVotingKeys GUARDED_BY(cs) {std::make_shared()}; class ScopedLockBool { From 4b51df09224c0331b2dc8f699d39cd60227e8a26 Mon Sep 17 00:00:00 2001 From: Pasta Date: Fri, 1 Oct 2021 13:47:22 -0400 Subject: [PATCH 03/10] std atomic --- src/governance/object.h | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/governance/object.h b/src/governance/object.h index 0aff1784519c..ec84fa8e917e 100644 --- a/src/governance/object.h +++ b/src/governance/object.h @@ -98,19 +98,19 @@ class CGovernanceObject mutable CCriticalSection cs; /// Object typecode - int nObjectType GUARDED_BY(cs) {GOVERNANCE_OBJECT_UNKNOWN}; + std::atomic nObjectType{GOVERNANCE_OBJECT_UNKNOWN}; /// parent object, 0 is root uint256 nHashParent GUARDED_BY(cs); /// object revision in the system - int nRevision GUARDED_BY(cs) {0}; + std::atomic nRevision{0}; /// time this object was created - int64_t nTime GUARDED_BY(cs) {0}; + std::atomic nTime{0}; /// time this object was marked for deletion - int64_t nDeletionTime GUARDED_BY(cs) {0}; + std::atomic nDeletionTime{0}; /// fee-tx uint256 nCollateralHash GUARDED_BY(cs); @@ -123,33 +123,33 @@ class CGovernanceObject std::vector vchSig GUARDED_BY(cs); /// is valid by blockchain - bool fCachedLocalValidity GUARDED_BY(cs) {false}; + std::atomic fCachedLocalValidity{false}; std::string strLocalValidityError GUARDED_BY(cs); // VARIOUS FLAGS FOR OBJECT / SET VIA MASTERNODE VOTING /// true == minimum network support has been reached for this object to be funded (doesn't mean it will for sure though) - bool fCachedFunding GUARDED_BY(cs) {false}; + std::atomic fCachedFunding{false}; /// true == minimum network has been reached flagging this object as a valid and understood governance object (e.g, the serialized data is correct format, etc) - bool fCachedValid GUARDED_BY(cs) {true}; + std::atomic fCachedValid{true}; /// true == minimum network support has been reached saying this object should be deleted from the system entirely - bool fCachedDelete GUARDED_BY(cs) {false}; + std::atomic fCachedDelete{false}; /** true == minimum network support has been reached flagging this object as endorsed by an elected representative body * (e.g. business review board / technical review board /etc) */ - bool fCachedEndorsed GUARDED_BY(cs) {false}; + std::atomic fCachedEndorsed{false}; /// object was updated and cached values should be updated soon - bool fDirtyCache GUARDED_BY(cs) {false}; + std::atomic fDirtyCache{false}; /// Object is no longer of interest - bool fExpired GUARDED_BY(cs) {false}; + std::atomic fExpired{false}; /// Failed to parse object data - bool fUnparsable GUARDED_BY(cs) {false}; + std::atomic fUnparsable{false}; vote_m_t mapCurrentMNVotes GUARDED_BY(cs); From 3592b2ddfcb05b1588652bdf474c17719bdf883f Mon Sep 17 00:00:00 2001 From: Pasta Date: Fri, 1 Oct 2021 14:42:02 -0400 Subject: [PATCH 04/10] return by value instead of by reference, as they are protected --- src/governance/object.h | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/governance/object.h b/src/governance/object.h index ec84fa8e917e..ffcccab6b917 100644 --- a/src/governance/object.h +++ b/src/governance/object.h @@ -179,13 +179,15 @@ class CGovernanceObject return nObjectType; } - const uint256& GetCollateralHash() const + uint256 GetCollateralHash() const { + LOCK(cs); return nCollateralHash; } - const COutPoint& GetMasternodeOutpoint() const + COutPoint GetMasternodeOutpoint() const { + LOCK(cs); return masternodeOutpoint; } @@ -224,8 +226,12 @@ class CGovernanceObject fExpired = true; } - const CGovernanceObjectVoteFile& GetVoteFile() const + /* TODO we are returning an expensive copy here. Refactor this code to avoid copy, while retaining saftey. + * Likely this will include making CGovernanceObjectVoteFile internally safe and returning a ptr or ref + */ + CGovernanceObjectVoteFile GetVoteFile() const { + LOCK(cs); return fileVotes; } From cd2275ba97a45ad9aee56af383fe475ff9a9f731 Mon Sep 17 00:00:00 2001 From: Pasta Date: Fri, 1 Oct 2021 14:43:02 -0400 Subject: [PATCH 05/10] Lock cs as needed, adjust cs_main locking to hopefully avoid deadlock --- src/governance/object.cpp | 19 ++++++++++++++----- src/governance/object.h | 7 ++++--- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/governance/object.cpp b/src/governance/object.cpp index f5f36a7155a8..896364182e6c 100644 --- a/src/governance/object.cpp +++ b/src/governance/object.cpp @@ -241,6 +241,8 @@ uint256 CGovernanceObject::GetHash() const // CREATE HASH OF ALL IMPORTANT PIECES OF DATA + LOCK(cs); + CHashWriter ss(SER_GETHASH, PROTOCOL_VERSION); ss << nHashParent; ss << nRevision; @@ -260,6 +262,7 @@ uint256 CGovernanceObject::GetSignatureHash() const void CGovernanceObject::SetMasternodeOutpoint(const COutPoint& outpoint) { + LOCK(cs); masternodeOutpoint = outpoint; } @@ -269,12 +272,14 @@ bool CGovernanceObject::Sign(const CBLSSecretKey& key) if (!sig.IsValid()) { return false; } + LOCK(cs); vchSig = sig.ToByteVector(); return true; } bool CGovernanceObject::CheckSignature(const CBLSPublicKey& pubKey) const { + LOCK(cs); if (!CBLSSignature(vchSig).VerifyInsecure(pubKey, GetSignatureHash())) { LogPrintf("CGovernanceObject::CheckSignature -- VerifyInsecure() failed\n"); return false; @@ -290,7 +295,7 @@ bool CGovernanceObject::CheckSignature(const CBLSPublicKey& pubKey) const UniValue CGovernanceObject::GetJSONObject() const { UniValue obj(UniValue::VOBJ); - if (vchData.empty()) { + if (LOCK(cs); vchData.empty()) { return obj; } @@ -318,7 +323,7 @@ UniValue CGovernanceObject::GetJSONObject() const void CGovernanceObject::LoadData() { - if (vchData.empty()) { + if (LOCK(cs); vchData.empty()) { return; } @@ -369,16 +374,19 @@ void CGovernanceObject::GetData(UniValue& objResult) const std::string CGovernanceObject::GetDataAsHexString() const { + LOCK(cs); return HexStr(vchData); } std::string CGovernanceObject::GetDataAsPlainString() const { + LOCK(cs); return std::string(vchData.begin(), vchData.end()); } UniValue CGovernanceObject::ToJson() const { + LOCK(cs); UniValue obj(UniValue::VOBJ); obj.pushKV("objectHash", GetHash().ToString()); obj.pushKV("parentHash", nHashParent.ToString()); @@ -400,7 +408,7 @@ UniValue CGovernanceObject::ToJson() const void CGovernanceObject::UpdateLocalValidity() { - LOCK(cs_main); + LOCK(cs); // THIS DOES NOT CHECK COLLATERAL, THIS IS CHECKED UPON ORIGINAL ARRIVAL fCachedLocalValidity = IsValidLocally(strLocalValidityError, false); } @@ -415,6 +423,7 @@ bool CGovernanceObject::IsValidLocally(std::string& strError, bool fCheckCollate bool CGovernanceObject::IsValidLocally(std::string& strError, bool& fMissingConfirmations, bool fCheckCollateral) const { + AssertLockHeld(cs); fMissingConfirmations = false; if (fUnparsable) { @@ -432,7 +441,7 @@ bool CGovernanceObject::IsValidLocally(std::string& strError, bool& fMissingConf strError = strprintf("Invalid proposal data, error messages: %s", validator.GetErrorMessages()); return false; } - if (fCheckCollateral && !IsCollateralValid(strError, fMissingConfirmations)) { + if (fCheckCollateral && !WITH_LOCK(cs_main, return IsCollateralValid(strError, fMissingConfirmations))) { strError = "Invalid proposal collateral"; return false; } @@ -493,7 +502,7 @@ bool CGovernanceObject::IsCollateralValid(std::string& strError, bool& fMissingC // RETRIEVE TRANSACTION IN QUESTION - if (!GetTransaction(nCollateralHash, txCollateral, Params().GetConsensus(), nBlockHash)) { + if (LOCK(cs); !GetTransaction(nCollateralHash, txCollateral, Params().GetConsensus(), nBlockHash)) { strError = strprintf("Can't find collateral tx %s", nCollateralHash.ToString()); LogPrintf("CGovernanceObject::IsCollateralValid -- %s\n", strError); return false; diff --git a/src/governance/object.h b/src/governance/object.h index ffcccab6b917..dadcb01a224a 100644 --- a/src/governance/object.h +++ b/src/governance/object.h @@ -245,12 +245,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 EXCLUSIVE_LOCKS_REQUIRED(cs); - 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 EXCLUSIVE_LOCKS_REQUIRED(cs); /// 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 EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); void UpdateLocalValidity(); @@ -294,6 +294,7 @@ class CGovernanceObject SERIALIZE_METHODS(CGovernanceObject, obj) { // SERIALIZE DATA FOR SAVING/LOADING OR NETWORK FUNCTIONS + LOCK(obj.cs); READWRITE( obj.nHashParent, obj.nRevision, From 72967c96639627c626e609ffc0801c6daeb8b7f3 Mon Sep 17 00:00:00 2001 From: Pasta Date: Fri, 1 Oct 2021 18:02:33 -0400 Subject: [PATCH 06/10] use .load in constructor initializer list --- src/governance/object.cpp | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/governance/object.cpp b/src/governance/object.cpp index 896364182e6c..c1b518b5fb4b 100644 --- a/src/governance/object.cpp +++ b/src/governance/object.cpp @@ -35,24 +35,24 @@ CGovernanceObject::CGovernanceObject(const uint256& nHashParentIn, int nRevision } CGovernanceObject::CGovernanceObject(const CGovernanceObject& other) : - nObjectType(other.nObjectType), + nObjectType(other.nObjectType.load()), nHashParent(other.nHashParent), - nRevision(other.nRevision), - nTime(other.nTime), - nDeletionTime(other.nDeletionTime), + nRevision(other.nRevision.load()), + nTime(other.nTime.load()), + nDeletionTime(other.nDeletionTime.load()), nCollateralHash(other.nCollateralHash), vchData(other.vchData), masternodeOutpoint(other.masternodeOutpoint), vchSig(other.vchSig), - fCachedLocalValidity(other.fCachedLocalValidity), + fCachedLocalValidity(other.fCachedLocalValidity.load()), strLocalValidityError(other.strLocalValidityError), - fCachedFunding(other.fCachedFunding), - fCachedValid(other.fCachedValid), - fCachedDelete(other.fCachedDelete), - fCachedEndorsed(other.fCachedEndorsed), - fDirtyCache(other.fDirtyCache), - fExpired(other.fExpired), - fUnparsable(other.fUnparsable), + fCachedFunding(other.fCachedFunding.load()), + fCachedValid(other.fCachedValid.load()), + fCachedDelete(other.fCachedDelete.load()), + fCachedEndorsed(other.fCachedEndorsed.load()), + fDirtyCache(other.fDirtyCache.load()), + fExpired(other.fExpired.load()), + fUnparsable(other.fUnparsable.load()), mapCurrentMNVotes(other.mapCurrentMNVotes), fileVotes(other.fileVotes) { From cc35ad2af89fd624c063e3635cb61e0d5c576a29 Mon Sep 17 00:00:00 2001 From: Pasta Date: Tue, 5 Oct 2021 14:01:52 -0400 Subject: [PATCH 07/10] make nTimeLastDiff, nCachedBlockHeight atomic, lock cs for cmapInvalidVotes --- src/governance/governance.cpp | 2 +- src/governance/governance.h | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index 305ad505fb76..192b69c79f20 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -1175,7 +1175,7 @@ void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex, CConnman& co } nCachedBlockHeight = pindex->nHeight; - LogPrint(BCLog::GOBJECT, "CGovernanceManager::UpdatedBlockTip -- nCachedBlockHeight: %d\n", nCachedBlockHeight); + LogPrint(BCLog::GOBJECT, "CGovernanceManager::UpdatedBlockTip -- nCachedBlockHeight: %d\n", pindex->nHeight); if (deterministicMNManager->IsDIP3Enforced(pindex->nHeight)) { RemoveInvalidVotes(); diff --git a/src/governance/governance.h b/src/governance/governance.h index 9b8d109ec071..e60cbf834288 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -164,10 +164,10 @@ class CGovernanceManager static const int MAX_TIME_FUTURE_DEVIATION; static const int RELIABLE_PROPAGATION_TIME; - int64_t nTimeLastDiff GUARDED_BY(cs); + std::atomic nTimeLastDiff; // keep track of current block height - int nCachedBlockHeight GUARDED_BY(cs); + std::atomic nCachedBlockHeight; // keep track of the scanning errors std::map mapObjects GUARDED_BY(cs); @@ -356,6 +356,7 @@ class CGovernanceManager void AddInvalidVote(const CGovernanceVote& vote) { + LOCK(cs); cmapInvalidVotes.Insert(vote.GetHash(), vote); } From d8431093c667f90df40340c49a30ba67f2759433 Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 11 Oct 2021 13:16:21 -0400 Subject: [PATCH 08/10] fixes Signed-off-by: pasta --- src/governance/governance.cpp | 9 ++++++--- src/governance/governance.h | 2 +- src/governance/object.h | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index 192b69c79f20..f6540046b21a 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -164,7 +164,7 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, const std::string& strComm // CHECK OBJECT AGAINST LOCAL BLOCKCHAIN bool fMissingConfirmations = false; - bool fIsValid = govobj.IsValidLocally(strError, fMissingConfirmations, true); + bool fIsValid = WITH_LOCK(govobj.cs, return govobj.IsValidLocally(strError, fMissingConfirmations, true)); if (fRateCheckBypassed && fIsValid && !MasternodeRateCheck(govobj, true)) { LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECT -- masternode rate check failed (after signature verification) - %s - (current block height %d)\n", strHash, nCachedBlockHeight); @@ -244,6 +244,7 @@ void CGovernanceManager::CheckOrphanVotes(CGovernanceObject& govobj, CConnman& c { uint256 nHash = govobj.GetHash(); std::vector vecVotePairs; + LOCK(cs); cmmapOrphanVotes.GetAll(nHash, vecVotePairs); ScopedLockBool guard(cs, fRateChecksEnabled, false); @@ -279,7 +280,7 @@ void CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj, CConnman // MAKE SURE THIS OBJECT IS OK - if (!govobj.IsValidLocally(strError, true)) { + if (!WITH_LOCK(govobj.cs, return govobj.IsValidLocally(strError, true))) { LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- invalid governance object - %s - (nCachedBlockHeight %d) \n", strError, nCachedBlockHeight); return; } @@ -300,7 +301,7 @@ void CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj, CConnman LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- Before trigger block, GetDataAsPlainString = %s, nObjectType = %d\n", govobj.GetDataAsPlainString(), govobj.GetObjectType()); - if (govobj.GetObjectType() == GOVERNANCE_OBJECT_TRIGGER && !triggerman.AddNewTrigger(nHash)) { + if (govobj.GetObjectType() == GOVERNANCE_OBJECT_TRIGGER && !WITH_LOCK(governance.cs, return triggerman.AddNewTrigger(nHash))) { LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- undo adding invalid trigger object: hash = %s\n", nHash.ToString()); objpair.first->second.PrepareDeletion(GetAdjustedTime()); return; @@ -687,6 +688,7 @@ void CGovernanceManager::MasternodeRateUpdate(const CGovernanceObject& govobj) if (govobj.GetObjectType() != GOVERNANCE_OBJECT_TRIGGER) return; const COutPoint& masternodeOutpoint = govobj.GetMasternodeOutpoint(); + LOCK(cs); auto it = mapLastMasternodeObject.find(masternodeOutpoint); if (it == mapLastMasternodeObject.end()) { @@ -843,6 +845,7 @@ void CGovernanceManager::CheckPostponedObjects(CConnman& connman) std::string strError; bool fMissingConfirmations; + LOCK(govobj.cs); if (govobj.IsCollateralValid(strError, fMissingConfirmations)) { if (govobj.IsValidLocally(strError, false)) { AddGovernanceObject(govobj, connman); diff --git a/src/governance/governance.h b/src/governance/governance.h index e60cbf834288..e90f9b141668 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -192,7 +192,7 @@ class CGovernanceManager hash_s_t setRequestedVotes GUARDED_BY(cs); - bool fRateChecksEnabled GUARDED_BY(cs) {true}; + bool fRateChecksEnabled{true}; // used to check for changed voting keys CDeterministicMNListPtr lastMNListForVotingKeys GUARDED_BY(cs) {std::make_shared()}; diff --git a/src/governance/object.h b/src/governance/object.h index dadcb01a224a..b752636fc129 100644 --- a/src/governance/object.h +++ b/src/governance/object.h @@ -93,9 +93,9 @@ class CGovernanceObject public: // Types using vote_m_t = std::map; -private: /// critical section to protect the inner data structures mutable CCriticalSection cs; +private: /// Object typecode std::atomic nObjectType{GOVERNANCE_OBJECT_UNKNOWN}; From 0c31b32e1d795ea34b502693630b36483e17f42b Mon Sep 17 00:00:00 2001 From: Pasta Date: Wed, 20 Oct 2021 15:05:07 -0400 Subject: [PATCH 09/10] fixes --- src/governance/governance.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index f6540046b21a..abc3e7f66b3a 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -347,8 +347,10 @@ void CGovernanceManager::UpdateCachesAndClean() ScopedLockBool guard(cs, fRateChecksEnabled, false); // Clean up any expired or invalid triggers - triggerman.CleanAndRemove(); - + { + LOCK(governance.cs); + triggerman.CleanAndRemove(); + } auto it = mapObjects.begin(); int64_t nNow = GetAdjustedTime(); @@ -1089,7 +1091,7 @@ void CGovernanceManager::AddCachedTriggers() continue; } - if (!triggerman.AddNewTrigger(govobj.GetHash())) { + if (!WITH_LOCK(governance.cs, return triggerman.AddNewTrigger(govobj.GetHash()))) { govobj.PrepareDeletion(GetAdjustedTime()); } } From 9325e6ddff80cdae01dfcddf06648f9bb2fa50eb Mon Sep 17 00:00:00 2001 From: Pasta Date: Sat, 23 Oct 2021 23:54:11 -0400 Subject: [PATCH 10/10] fix --- src/governance/classes.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/governance/classes.h b/src/governance/classes.h index 3ae0761ffc14..6f300f9d8cba 100644 --- a/src/governance/classes.h +++ b/src/governance/classes.h @@ -5,6 +5,7 @@ #define BITCOIN_GOVERNANCE_CLASSES_H #include +#include #include #include