From 2f0691a99aef21ed8ddd805136eff136696c5428 Mon Sep 17 00:00:00 2001 From: Tim Flynn Date: Sat, 6 Aug 2016 11:28:09 -0400 Subject: [PATCH 1/5] - Added trigger manager cs locking - Fixed LogPrint bug in CSuperblock::IsValid --- src/governance-classes.cpp | 12 +++++++++--- src/governance-classes.h | 17 +++++++++++++++-- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/governance-classes.cpp b/src/governance-classes.cpp index 285e0df7c844..c6ca13819453 100644 --- a/src/governance-classes.cpp +++ b/src/governance-classes.cpp @@ -62,6 +62,7 @@ std::vector SplitBy(std::string strCommand, std::string strDelimit) bool CGovernanceTriggerManager::AddNewTrigger(uint256 nHash) { DBG( cout << "CGovernanceTriggerManager::AddNewTrigger: Start" << endl; ); + LOCK(cs); // IF WE ALREADY HAVE THIS HASH, RETURN if(mapTrigger.count(nHash)) { @@ -215,6 +216,7 @@ bool CSuperblockManager::IsValidSuperblockHeight(int nBlockHeight) bool CSuperblockManager::IsSuperblockTriggered(int nBlockHeight) { + LOCK(triggerman.cs); // GET ALL ACTIVE TRIGGERS printf("IsSuperblockTriggered\n"); std::vector vecTriggers = triggerman.GetActiveTriggers(); @@ -268,6 +270,8 @@ bool CSuperblockManager::GetBestSuperblock(CSuperblock_sptr& pBlock, int nBlockH printf("GetBestSuperblock\n"); + AssertLockHeld(triggerman.cs); + BOOST_FOREACH(CSuperblock_sptr superblock, vecTriggers) { if(!superblock) { @@ -310,6 +314,7 @@ bool CSuperblockManager::GetBestSuperblock(CSuperblock_sptr& pBlock, int nBlockH void CSuperblockManager::CreateSuperblock(CMutableTransaction& txNew, CAmount nFees, int nBlockHeight) { printf("CSuperblockManager::CreateSuperblock Start\n"); + LOCK(triggerman.cs); AssertLockHeld(cs_main); if(!chainActive.Tip()) { @@ -364,6 +369,7 @@ void CSuperblockManager::CreateSuperblock(CMutableTransaction& txNew, CAmount nF bool CSuperblockManager::IsValid(const CTransaction& txNew, int nBlockHeight) { // GET BEST SUPERBLOCK, SHOULD MATCH + LOCK(triggerman.cs); CSuperblock_sptr pBlock; if(CSuperblockManager::GetBestSuperblock(pBlock, nBlockHeight)) @@ -384,6 +390,7 @@ bool CSuperblock::IsValid(const CTransaction& txNew) { printf("IsValid"); // TODO : LOCK(cs); + std::string strPayeesPossible = ""; printf("1"); @@ -412,7 +419,7 @@ bool CSuperblock::IsValid(const CTransaction& txNew) // TODO: PRINT NICE N.N DASH OUTPUT - LogPrintf("SUPERBLOCK: output n %d payment %d to %s\n", payment.nAmount, address2.ToString()); + LogPrintf("SUPERBLOCK: output n %d payment %d to %s\n", i, payment.nAmount, address2.ToString()); printf("4\n"); @@ -434,8 +441,7 @@ bool CSuperblock::IsValid(const CTransaction& txNew) std::string CSuperblockManager::GetRequiredPaymentsString(int nBlockHeight) { - // TODO : LOCK(cs); - + LOCK(triggerman.cs); std::string ret = "Unknown"; // GET BEST SUPERBLOCK diff --git a/src/governance-classes.h b/src/governance-classes.h index a1593b291593..28240b7ef7a6 100755 --- a/src/governance-classes.h +++ b/src/governance-classes.h @@ -27,6 +27,7 @@ using namespace std; class CSuperblock; class CGovernanceTrigger; class CGovernanceTriggerManager; +class CSuperblockManager; typedef boost::shared_ptr CSuperblock_sptr; @@ -56,6 +57,10 @@ struct trigger_man_rec_t { class CGovernanceTriggerManager { + friend class CSuperblock; + friend class CSuperblockManager; + +public: // Typedefs typedef std::map trigger_m_t; typedef trigger_m_t::iterator trigger_m_it; @@ -65,9 +70,17 @@ class CGovernanceTriggerManager private: trigger_m_t mapTrigger; + CCriticalSection cs; + public: - std::vector GetActiveTriggers(); + CGovernanceTriggerManager() + : mapTrigger() + {} + bool AddNewTrigger(uint256 nHash); + +private: + std::vector GetActiveTriggers(); void CleanAndRemove(); bool UpdateStatus(uint256 nHash, int nNewStatus); @@ -75,7 +88,7 @@ class CGovernanceTriggerManager }; /** -* Superblock Mananger +* Superblock Manager * * Class for querying superblock information */ From 2be55bc44d4dd86447d2c27c76ee787c97d32867 Mon Sep 17 00:00:00 2001 From: Tim Flynn Date: Mon, 8 Aug 2016 02:39:19 -0400 Subject: [PATCH 2/5] - Simplified trigger manager locking to avoid a deadlock trigger manager access is now protected only by the governance.cs mutex - Removed unsafe caching of govobj pointer in CSuperblock - Moved status from map to CSuperblock - Reduced width of public interfaces to CTriggerManager and CSuperblockManager - Implemented removal of executed and invalid (but not expired) triggers - Reordered CSporkMessage members to avoid structure padding (note serialization format is unchanged) - Replaced X11 hash with CHashWriter for CSporkMessage --- src/governance-classes.cpp | 140 ++++++++++++++++++------------------- src/governance-classes.h | 84 ++++++++++++++-------- src/governance.cpp | 5 ++ src/spork.h | 9 ++- 4 files changed, 134 insertions(+), 104 deletions(-) diff --git a/src/governance-classes.cpp b/src/governance-classes.cpp index c6ca13819453..3ef107e5d384 100644 --- a/src/governance-classes.cpp +++ b/src/governance-classes.cpp @@ -62,7 +62,7 @@ std::vector SplitBy(std::string strCommand, std::string strDelimit) bool CGovernanceTriggerManager::AddNewTrigger(uint256 nHash) { DBG( cout << "CGovernanceTriggerManager::AddNewTrigger: Start" << endl; ); - LOCK(cs); + AssertLockHeld(governance.cs); // IF WE ALREADY HAVE THIS HASH, RETURN if(mapTrigger.count(nHash)) { @@ -75,34 +75,18 @@ bool CGovernanceTriggerManager::AddNewTrigger(uint256 nHash) return false; } - CGovernanceObject* pObj = governance.FindGovernanceObject(nHash); + CSuperblock_sptr superblock(new CSuperblock(nHash)); - // Failed to find corresponding govobj - // Note this function should only be called after the govobj has been added to - // the main map so this case should not occur but we need to protect against - // a NULL ptr access. - if(!pObj) { - DBG( cout << "CGovernanceTriggerManager::AddNewTrigger: Couldn't find govobj, returning false" << endl; ); + if(superblock->GetErrorState()) { + DBG( cout << "AddNewTrigger: error in superblock: " << superblock->GetErrorMessage() << endl; ); + LogPrint("superblock", "CGovernanceTriggerManager::AddNewTrigger: Error creating superblock: %s\n", superblock->GetErrorMessage()); return false; } - DBG( cout << "CGovernanceTriggerManager::AddGovernanceObject Before trigger block, strData = " - << pObj->GetDataAsString() - << ", nObjectType = " << pObj->nObjectType - << endl; ); - - // IF THIS ISN'T A TRIGGER, WHY ARE WE HERE? - if(pObj->GetObjectType() != GOVERNANCE_OBJECT_TRIGGER) { - DBG( cout << "AddNewTrigger: govobj not a trigger, returning false" << endl; ); - return false; - } - - DBG( cout << "CGovernanceTriggerManager::AddNewTrigger: Creating superblock" << endl; ); - - CSuperblock_sptr superblock(new CSuperblock(pObj)); + superblock->SetStatus(SEEN_OBJECT_IS_VALID); DBG( cout << "CGovernanceTriggerManager::AddNewTrigger: Inserting trigger" << endl; ); - mapTrigger.insert(make_pair(nHash, trigger_man_rec_t(SEEN_OBJECT_IS_VALID,superblock))); + mapTrigger.insert(make_pair(nHash, superblock)); DBG( cout << "CGovernanceTriggerManager::AddNewTrigger: End" << endl; ); @@ -117,52 +101,68 @@ bool CGovernanceTriggerManager::AddNewTrigger(uint256 nHash) void CGovernanceTriggerManager::CleanAndRemove() { + DBG( cout << "CGovernanceTriggerManager::CleanAndRemove: Start" << endl; ); + AssertLockHeld(governance.cs); + // LOOK AT THESE OBJECTS AND COMPILE A VALID LIST OF TRIGGERS - trigger_m_it it = mapTrigger.begin(); - while(it != mapTrigger.end()) { + for(trigger_m_it it = mapTrigger.begin(); it != mapTrigger.end(); ++it) { //int nNewStatus = -1; CGovernanceObject* pObj = governance.FindGovernanceObject((*it).first); + if(!pObj) { + continue; + } + CSuperblock_sptr& superblock = it->second; + if(!superblock) { + continue; + } // IF THIS ISN'T A TRIGGER, WHY ARE WE HERE? if(pObj->GetObjectType() != GOVERNANCE_OBJECT_TRIGGER) { - UpdateStatus((*it).first, SEEN_OBJECT_ERROR_INVALID); + superblock->SetStatus(SEEN_OBJECT_ERROR_INVALID); } - it++; } -} -/** -* -* Update Status -* -*/ - -bool CGovernanceTriggerManager::UpdateStatus(uint256 nHash, int nNewStatus) -{ - if(!mapTrigger.count(nHash)) { - return false; - } - - mapTrigger[nHash].status = nNewStatus; - - LogPrintf("CGovernanceTriggerManager::UpdateStatus - nHash %s nNewStatus %d\n", nHash.ToString(), nNewStatus); - - return true; -} - - -/** -* -* Get Status -* -*/ - -int CGovernanceTriggerManager::GetStatus(uint256 nHash) -{ - if(!mapTrigger.count(nHash)) { - return SEEN_OBJECT_UNKNOWN; + // Remove triggers that are invalid or already executed + DBG( cout << "CGovernanceTriggerManager::CleanAndRemove: mapTrigger.size() = " << mapTrigger.size() << endl; ); + trigger_m_it it = mapTrigger.begin(); + while(it != mapTrigger.end()) { + bool remove = false; + CSuperblock_sptr& superblock = it->second; + if(!superblock) { + DBG( cout << "CGovernanceTriggerManager::CleanAndRemove: NULL superblock marked for removal " << endl; ); + remove = true; + } + else { + DBG( cout << "CGovernanceTriggerManager::CleanAndRemove: superblock status = " << superblock->GetStatus() << endl; ); + switch(superblock->GetStatus()) { + case SEEN_OBJECT_ERROR_INVALID: + case SEEN_OBJECT_EXECUTED: + case SEEN_OBJECT_UNKNOWN: + remove = true; + break; + default: + break; + } + } + + if(remove) { + DBG( + CGovernanceObject* govobj = superblock->GetGovernanceObject(); + string strdata = "NULL"; + if(govobj) { + strdata = govobj->GetDataAsString(); + } + cout << "CGovernanceTriggerManager::CleanAndRemove: Removing object: " + << strdata + << endl; + ); + mapTrigger.erase(it++); + } + else { + ++it; + } } - return mapTrigger[nHash].status; + DBG( cout << "CGovernanceTriggerManager::CleanAndRemove: End" << endl; ); } /** @@ -174,19 +174,20 @@ int CGovernanceTriggerManager::GetStatus(uint256 nHash) std::vector CGovernanceTriggerManager::GetActiveTriggers() { + AssertLockHeld(governance.cs); std::vector vecResults; DBG( cout << "GetActiveTriggers: mapTrigger.size() = " << mapTrigger.size() << endl; ); // LOOK AT THESE OBJECTS AND COMPILE A VALID LIST OF TRIGGERS trigger_m_it it = mapTrigger.begin(); - while(it != mapTrigger.end()){ + while(it != mapTrigger.end()) { CGovernanceObject* pObj = governance.FindGovernanceObject((*it).first); if(pObj) { DBG( cout << "GetActiveTriggers: pObj->GetDataAsString() = " << pObj->GetDataAsString() << endl; ); - vecResults.push_back(it->second.superblock); + vecResults.push_back(it->second); } ++it; } @@ -216,7 +217,7 @@ bool CSuperblockManager::IsValidSuperblockHeight(int nBlockHeight) bool CSuperblockManager::IsSuperblockTriggered(int nBlockHeight) { - LOCK(triggerman.cs); + LOCK(governance.cs); // GET ALL ACTIVE TRIGGERS printf("IsSuperblockTriggered\n"); std::vector vecTriggers = triggerman.GetActiveTriggers(); @@ -265,13 +266,11 @@ bool CSuperblockManager::IsSuperblockTriggered(int nBlockHeight) bool CSuperblockManager::GetBestSuperblock(CSuperblock_sptr& pBlock, int nBlockHeight) { + printf("GetBestSuperblock\n"); + AssertLockHeld(governance.cs); std::vector vecTriggers = triggerman.GetActiveTriggers(); int nYesCount = 0; - printf("GetBestSuperblock\n"); - - AssertLockHeld(triggerman.cs); - BOOST_FOREACH(CSuperblock_sptr superblock, vecTriggers) { if(!superblock) { @@ -314,9 +313,9 @@ bool CSuperblockManager::GetBestSuperblock(CSuperblock_sptr& pBlock, int nBlockH void CSuperblockManager::CreateSuperblock(CMutableTransaction& txNew, CAmount nFees, int nBlockHeight) { printf("CSuperblockManager::CreateSuperblock Start\n"); - LOCK(triggerman.cs); - + LOCK(governance.cs); AssertLockHeld(cs_main); + if(!chainActive.Tip()) { DBG( cout << "CSuperblockManager::CreateSuperblock No active tip, returning" << endl; ); return; @@ -357,6 +356,7 @@ void CSuperblockManager::CreateSuperblock(CMutableTransaction& txNew, CAmount nF DBG( cout << "CSuperblockManager::CreateSuperblock Before LogPrintf call " << endl; ); LogPrintf("NEW Superblock : output %d (addr %s, amount %d)\n", i, address2.ToString(), payment.nAmount); DBG( cout << "CSuperblockManager::CreateSuperblock After LogPrintf call " << endl; ); + pBlock->SetExecuted(); } else { DBG( cout << "CSuperblockManager::CreateSuperblock Payment not found " << endl; ); @@ -369,7 +369,7 @@ void CSuperblockManager::CreateSuperblock(CMutableTransaction& txNew, CAmount nF bool CSuperblockManager::IsValid(const CTransaction& txNew, int nBlockHeight) { // GET BEST SUPERBLOCK, SHOULD MATCH - LOCK(triggerman.cs); + LOCK(governance.cs); CSuperblock_sptr pBlock; if(CSuperblockManager::GetBestSuperblock(pBlock, nBlockHeight)) @@ -441,7 +441,7 @@ bool CSuperblock::IsValid(const CTransaction& txNew) std::string CSuperblockManager::GetRequiredPaymentsString(int nBlockHeight) { - LOCK(triggerman.cs); + LOCK(governance.cs); std::string ret = "Unknown"; // GET BEST SUPERBLOCK diff --git a/src/governance-classes.h b/src/governance-classes.h index 28240b7ef7a6..da72726765f7 100755 --- a/src/governance-classes.h +++ b/src/governance-classes.h @@ -18,6 +18,7 @@ #include #include #include "init.h" +#include "governance.h" using namespace std; @@ -37,17 +38,6 @@ extern CGovernanceTriggerManager triggerman; // SPLIT A STRING UP - USED FOR SUPERBLOCK PAYMENTS std::vector SplitBy(std::string strCommand, std::string strDelimit); -struct trigger_man_rec_t { - - trigger_man_rec_t(int status_= SEEN_OBJECT_UNKNOWN,CSuperblock_sptr superblock_=CSuperblock_sptr()) - : status(status_), - superblock(superblock_) - {} - - int status; - CSuperblock_sptr superblock; -}; - /** * Trigger Mananger * @@ -57,11 +47,11 @@ struct trigger_man_rec_t { class CGovernanceTriggerManager { - friend class CSuperblock; friend class CSuperblockManager; + friend class CGovernanceManager; public: // Typedefs - typedef std::map trigger_m_t; + typedef std::map trigger_m_t; typedef trigger_m_t::iterator trigger_m_it; @@ -70,21 +60,18 @@ class CGovernanceTriggerManager private: trigger_m_t mapTrigger; - CCriticalSection cs; - public: CGovernanceTriggerManager() : mapTrigger() {} - bool AddNewTrigger(uint256 nHash); - private: std::vector GetActiveTriggers(); + + bool AddNewTrigger(uint256 nHash); + void CleanAndRemove(); - bool UpdateStatus(uint256 nHash, int nNewStatus); - int GetStatus(uint256 nHash); }; /** @@ -118,7 +105,10 @@ class CSuperblockManager static std::string GetRequiredPaymentsString(int nBlockHeight); static bool IsValid(const CTransaction& txNew, int nBlockHeight); + +private: static bool GetBestSuperblock(CSuperblock_sptr& pBlock, int nBlockHeight); + }; /** @@ -188,37 +178,52 @@ class CSuperblock : public CGovernanceObject */ private: - CGovernanceObject* pGovObj; + uint256 nGovObjHash; bool fError; std::string strError; int nEpochStart; - bool nExecuted; + int status; std::vector vecPayments; public: CSuperblock() - : pGovObj(NULL), + : nGovObjHash(), fError(true), strError(), nEpochStart(0), - nExecuted(false), + status(SEEN_OBJECT_UNKNOWN), vecPayments() {} - CSuperblock(CGovernanceObject* pGovObjIn) - : pGovObj(pGovObjIn), + CSuperblock(uint256& nHash) + : nGovObjHash(nHash), fError(true), strError(), nEpochStart(0), - nExecuted(false), + status(SEEN_OBJECT_UNKNOWN), vecPayments() { DBG( cout << "CSuperblock Constructor Start" << endl; ); + + CGovernanceObject* pGovObj = GetGovernanceObject(); + if(!pGovObj) { - DBG( cout << "CSuperblock Constructor pGovObj is NULL, returning" << endl; ); + DBG( cout << "CSuperblock Constructor pGovObjIn is NULL, returning" << endl; ); + strError = "Failed to find Governance Object"; + return; + } + + DBG( cout << "CSuperblock Constructor pGovObj : " + << pGovObj->GetDataAsString() + << ", nObjectType = " << pGovObj->nObjectType + << endl; ); + + if(pGovObj->GetObjectType() != GOVERNANCE_OBJECT_TRIGGER) { + DBG( cout << "CSuperblock Constructor pHoObj not a trigger, returning" << endl; ); + strError = "Governance Object not a trigger"; return; } @@ -238,7 +243,6 @@ class CSuperblock : public CGovernanceObject std::string strAmounts = obj["payment_amounts"].get_str(); ParsePaymentSchedule(strAddresses, strAmounts); - nExecuted = false; fError = false; strError = ""; } @@ -253,8 +257,26 @@ class CSuperblock : public CGovernanceObject DBG( cout << "CSuperblock Constructor End" << endl; ); } + int GetStatus() { + return status; + } + + void SetStatus(int status_) { + status = status_; + } + + int GetErrorState() { + return fError; + } + + std::string GetErrorMessage() { + return strError; + } + CGovernanceObject* GetGovernanceObject() { - return pGovObj; + AssertLockHeld(governance.cs); + CGovernanceObject* pObj = governance.FindGovernanceObject(nGovObjHash); + return pObj; } int GetBlockStart() @@ -325,13 +347,13 @@ class CSuperblock : public CGovernanceObject // IS THIS TRIGGER ALREADY EXECUTED? bool IsExecuted() { - return triggerman.GetStatus(pGovObj->GetHash()) == SEEN_OBJECT_EXECUTED; + return (status == SEEN_OBJECT_EXECUTED); }; // TELL THE ENGINE WE EXECUTED THIS EVENT void SetExecuted() { - triggerman.UpdateStatus(pGovObj->GetHash(), SEEN_OBJECT_EXECUTED); + status = SEEN_OBJECT_EXECUTED; }; int CountPayments() diff --git a/src/governance.cpp b/src/governance.cpp index b3c7bbac0f1f..712da920e1f5 100644 --- a/src/governance.cpp +++ b/src/governance.cpp @@ -315,6 +315,8 @@ void CGovernanceManager::UpdateCachesAndClean() { LogPrintf("CGovernanceManager::UpdateCachesAndClean \n"); + AssertLockHeld(cs); + // DOUBLE CHECK THAT WE HAVE A VALID POINTER TO TIP if(!pCurrentBlockIndex) return; @@ -358,6 +360,9 @@ void CGovernanceManager::UpdateCachesAndClean() } } + // Clean up any expired or invalid triggers + triggerman.CleanAndRemove(); + // CHECK EACH GOVERNANCE OBJECTS VALIDITY (CPU HEAVY) // 12.1 todo - compile issues diff --git a/src/spork.h b/src/spork.h index ed500c83c900..b1c7894c3c05 100644 --- a/src/spork.h +++ b/src/spork.h @@ -65,13 +65,16 @@ class CSporkMessage { public: std::vector vchSig; - int nSporkID; int64_t nValue; int64_t nTimeSigned; + int nSporkID; uint256 GetHash(){ - uint256 n = HashX11(BEGIN(nSporkID), END(nTimeSigned)); - return n; + CHashWriter ss(SER_GETHASH, PROTOCOL_VERSION); + ss << nSporkID; + ss << nValue; + ss << nTimeSigned; + return ss.GetHash(); } ADD_SERIALIZE_METHODS; From dbb3e97d85b857e37e9625278451d83ed6cbb63d Mon Sep 17 00:00:00 2001 From: Tim Flynn Date: Mon, 8 Aug 2016 04:14:11 -0400 Subject: [PATCH 3/5] - Added fExpired flag to CGovernanceObject so that trigger removal now causes the removal of the underlying govobj - Added many LOCK(governance.cs) calls to hopefully make access to CGovernanceManager safe. This needs heavy testing or better yet static analysis to guarantee correctness. Deadlock conditions cannot yet be excluded. --- src/governance-classes.cpp | 10 ++++++++- src/governance.cpp | 42 +++++++++++++++++++++----------------- src/governance.h | 1 + src/main.cpp | 13 ++++++++++-- src/rpcgovernance.cpp | 12 ++++++++--- 5 files changed, 53 insertions(+), 25 deletions(-) diff --git a/src/governance-classes.cpp b/src/governance-classes.cpp index 3ef107e5d384..91067261b0c7 100644 --- a/src/governance-classes.cpp +++ b/src/governance-classes.cpp @@ -135,8 +135,16 @@ void CGovernanceTriggerManager::CleanAndRemove() DBG( cout << "CGovernanceTriggerManager::CleanAndRemove: superblock status = " << superblock->GetStatus() << endl; ); switch(superblock->GetStatus()) { case SEEN_OBJECT_ERROR_INVALID: + remove = true; + break; case SEEN_OBJECT_EXECUTED: case SEEN_OBJECT_UNKNOWN: + { + CGovernanceObject* govobj = superblock->GetGovernanceObject(); + if(govobj) { + govobj->fExpired = true; + } + } remove = true; break; default: @@ -146,8 +154,8 @@ void CGovernanceTriggerManager::CleanAndRemove() if(remove) { DBG( - CGovernanceObject* govobj = superblock->GetGovernanceObject(); string strdata = "NULL"; + CGovernanceObject* govobj = superblock->GetGovernanceObject(); if(govobj) { strdata = govobj->GetDataAsString(); } diff --git a/src/governance.cpp b/src/governance.cpp index 712da920e1f5..0f2a45dfeb80 100644 --- a/src/governance.cpp +++ b/src/governance.cpp @@ -146,6 +146,7 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, std::string& strCommand, C else if (strCommand == NetMsgType::MNGOVERNANCEOBJECT) { + LOCK(cs); // MAKE SURE WE HAVE A VALID REFERENCE TO THE TIP BEFORE CONTINUING if(!pCurrentBlockIndex) return; @@ -188,7 +189,6 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, std::string& strCommand, C } // UPDATE THAT WE'VE SEEN THIS OBJECT - mapSeenGovernanceObjects.insert(make_pair(govobj.GetHash(), SEEN_OBJECT_IS_VALID)); masternodeSync.AddedBudgetItem(govobj.GetHash()); @@ -327,31 +327,34 @@ void CGovernanceManager::UpdateCachesAndClean() std::map mapDirtyObjects; + // Clean up any expired or invalid triggers + triggerman.CleanAndRemove(); + while(it != mapObjects.end()) { CGovernanceObject* pObj = &((*it).second); - // IF CACHE IS NOT DIRTY, WHY DO THIS? - - if(!pObj || !pObj->fDirtyCache) - { + if(!pObj) { ++it; continue; } - mapDirtyObjects.insert(make_pair((*it).first, 1)); - - // UPDATE LOCAL VALIDITY AGAINST CRYPTO DATA - - pObj->UpdateLocalValidity(pCurrentBlockIndex); - - // UPDATE SENTINEL SIGNALING VARIABLES - - pObj->UpdateSentinelVariables(pCurrentBlockIndex); + // IF CACHE IS NOT DIRTY, WHY DO THIS? + if(pObj->fDirtyCache) { + mapDirtyObjects.insert(make_pair((*it).first, 1)); + + // UPDATE LOCAL VALIDITY AGAINST CRYPTO DATA + + pObj->UpdateLocalValidity(pCurrentBlockIndex); + + // UPDATE SENTINEL SIGNALING VARIABLES + + pObj->UpdateSentinelVariables(pCurrentBlockIndex); + } // IF DELETE=TRUE, THEN CLEAN THE MESS UP! - if(pObj->fCachedDelete) + if(pObj->fCachedDelete || pObj->fExpired) { LogPrintf("UpdateCachesAndClean --- erase obj %s\n", (*it).first.ToString()); mapObjects.erase(it++); @@ -360,9 +363,6 @@ void CGovernanceManager::UpdateCachesAndClean() } } - // Clean up any expired or invalid triggers - triggerman.CleanAndRemove(); - // CHECK EACH GOVERNANCE OBJECTS VALIDITY (CPU HEAVY) // 12.1 todo - compile issues @@ -629,6 +629,7 @@ CGovernanceObject::CGovernanceObject() fCachedEndorsed = false; fDirtyCache = true; fUnparsable = false; + fExpired = false; // PARSE JSON DATA STORAGE (STRDATA) LoadData(); @@ -654,6 +655,7 @@ CGovernanceObject::CGovernanceObject(uint256 nHashParentIn, int nRevisionIn, std fCachedEndorsed = false; fDirtyCache = true; fUnparsable = false; + fExpired = false; // PARSE JSON DATA STORAGE (STRDATA) LoadData(); @@ -678,6 +680,7 @@ CGovernanceObject::CGovernanceObject(const CGovernanceObject& other) fCachedDelete = other.fCachedDelete; fCachedEndorsed = other.fCachedEndorsed; fDirtyCache = other.fDirtyCache; + fExpired = other.fExpired; } int CGovernanceObject::GetObjectType() @@ -1011,7 +1014,7 @@ void CGovernanceObject::UpdateSentinelVariables(const CBlockIndex *pCurrentBlock fCachedDelete = false; fCachedEndorsed = false; fDirtyCache = false; - + // SET SENTINEL FLAGS TO TRUE IF MIMIMUM SUPPORT LEVELS ARE REACHED // ARE ANY OF THESE FLAGS CURRENTLY ACTIVATED? @@ -1050,4 +1053,5 @@ void CGovernanceObject::swap(CGovernanceObject& first, CGovernanceObject& second swap(first.fCachedDelete, second.fCachedDelete); swap(first.fCachedEndorsed, second.fCachedEndorsed); swap(first.fDirtyCache, second.fDirtyCache); + swap(first.fExpired, second.fExpired); } diff --git a/src/governance.h b/src/governance.h index c2158efad800..5d7805814204 100644 --- a/src/governance.h +++ b/src/governance.h @@ -191,6 +191,7 @@ class CGovernanceObject bool fCachedEndorsed; // true == minimum network support has been reached flagging this object as endorsed by an elected representative body (e.g. business review board / technecial review board /etc) bool fDirtyCache; // object was updated and cached values should be updated soon bool fUnparsable; // data field was unparsible, object will be rejected + bool fExpired; // Object is no longer of interest CGovernanceObject(); CGovernanceObject(uint256 nHashParentIn, int nRevisionIn, std::string strNameIn, int64_t nTime, uint256 nCollateralHashIn, std::string strDataIn); diff --git a/src/main.cpp b/src/main.cpp index 021723717836..8cf5f891e6ff 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4355,10 +4355,17 @@ bool static AlreadyHave(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main) return mnpayments.mapMasternodePayeeVotes.count(inv.hash); case MSG_GOVERNANCE_VOTE: - return governance.mapVotesByHash.count(inv.hash); + { + LOCK(governance.cs); + return governance.mapVotesByHash.count(inv.hash); + } + case MSG_GOVERNANCE_OBJECT: - return governance.mapObjects.count(inv.hash); + { + LOCK(governance.cs); + return governance.mapObjects.count(inv.hash); + } case MSG_MASTERNODE_ANNOUNCE: return mnodeman.mapSeenMasternodeBroadcast.count(inv.hash); @@ -4531,6 +4538,7 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam } if (!pushed && inv.type == MSG_GOVERNANCE_VOTE) { + LOCK(governance.cs); if(governance.mapVotesByHash.count(inv.hash)) { CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); ss.reserve(1000); @@ -4541,6 +4549,7 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam } if (!pushed && inv.type == MSG_GOVERNANCE_OBJECT) { + LOCK(governance.cs); if(governance.mapObjects.count(inv.hash)) { CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); ss.reserve(1000); diff --git a/src/rpcgovernance.cpp b/src/rpcgovernance.cpp index 417843600528..65a542a4aff9 100644 --- a/src/rpcgovernance.cpp +++ b/src/rpcgovernance.cpp @@ -176,7 +176,7 @@ UniValue gobject(const UniValue& params, bool fHelp) // ASSEMBLE NEW GOVERNANCE OBJECT FROM USER PARAMETERS - LOCK(cs_main); + LOCK2(cs_main, governance.cs); CBlockIndex* pindex = chainActive.Tip(); std::vector mnEntries; @@ -212,7 +212,6 @@ UniValue gobject(const UniValue& params, bool fHelp) } // RELAY THIS OBJECT - governance.mapSeenGovernanceObjects.insert(make_pair(govobj.GetHash(), SEEN_OBJECT_IS_VALID)); govobj.Relay(); governance.AddGovernanceObject(govobj); @@ -283,6 +282,7 @@ UniValue gobject(const UniValue& params, bool fHelp) return "Failure to sign."; } + LOCK(governance.cs); std::string strError = ""; if(governance.AddOrUpdateVote(vote, NULL, strError)) { governance.mapSeenVotes.insert(make_pair(vote.GetHash(), SEEN_OBJECT_IS_VALID)); @@ -391,6 +391,7 @@ UniValue gobject(const UniValue& params, bool fHelp) // UPDATE LOCAL DATABASE WITH NEW OBJECT SETTINGS + LOCK(governance.cs); std::string strError = ""; if(governance.AddOrUpdateVote(vote, NULL, strError)) { governance.mapSeenVotes.insert(make_pair(vote.GetHash(), SEEN_OBJECT_IS_VALID)); @@ -443,6 +444,8 @@ UniValue gobject(const UniValue& params, bool fHelp) // GET MATCHING GOVERNANCE OBJECTS + LOCK(governance.cs); + std::vector objs = governance.GetAllNewerThan(nStartTime); governance.UpdateLastDiffTime(GetTime()); @@ -504,7 +507,8 @@ UniValue gobject(const UniValue& params, bool fHelp) // REPORT BASIC OBJECT STATS - LOCK(cs_main); + LOCK2(cs_main, governance.cs); + UniValue objResult(UniValue::VOBJ); objResult.push_back(Pair("Name", pGovObj->GetName())); objResult.push_back(Pair("Hash", pGovObj->GetHash().ToString())); @@ -572,6 +576,8 @@ UniValue gobject(const UniValue& params, bool fHelp) // FIND OBJECT USER IS LOOKING FOR + LOCK(governance.cs); + CGovernanceObject* pGovObj = governance.FindGovernanceObject(hash); if(pGovObj == NULL) From 7785b3d40217b7e4c897c2c7fb083bd0d0aca521 Mon Sep 17 00:00:00 2001 From: Tim Flynn Date: Mon, 8 Aug 2016 08:38:57 -0400 Subject: [PATCH 4/5] Changes to allow removal of time expired triggers - Implemented caching of block height in CGovernanceManager to avoid possible deadlock between cs_main and governance.cs mutexes - Implemented expiration of unexecuted triggers approximately one month after their scheduled trigger time (per Evan's request) - Removed unused chain tip pointer in rpcgovernance.cpp. Fixes compiler warning and avoids an unnecessary lock on cs_main. --- src/governance-classes.cpp | 16 +++++++++++++++- src/governance.cpp | 6 ++++++ src/governance.h | 21 ++++++++++++++++++--- src/rpcgovernance.cpp | 6 ------ 4 files changed, 39 insertions(+), 10 deletions(-) diff --git a/src/governance-classes.cpp b/src/governance-classes.cpp index 91067261b0c7..29f180b02363 100644 --- a/src/governance-classes.cpp +++ b/src/governance-classes.cpp @@ -135,10 +135,10 @@ void CGovernanceTriggerManager::CleanAndRemove() DBG( cout << "CGovernanceTriggerManager::CleanAndRemove: superblock status = " << superblock->GetStatus() << endl; ); switch(superblock->GetStatus()) { case SEEN_OBJECT_ERROR_INVALID: + case SEEN_OBJECT_UNKNOWN: remove = true; break; case SEEN_OBJECT_EXECUTED: - case SEEN_OBJECT_UNKNOWN: { CGovernanceObject* govobj = superblock->GetGovernanceObject(); if(govobj) { @@ -147,6 +147,20 @@ void CGovernanceTriggerManager::CleanAndRemove() } remove = true; break; + case SEEN_OBJECT_IS_VALID: + { + // Rough approximation: 30 days per month * 576 blocks per day + static const int nMonthlyBlocks = 30*576; + int nTriggerBlock = superblock->GetBlockStart(); + int nExpirationBlock = nTriggerBlock + nMonthlyBlocks; + if(governance.GetCachedBlockHeight() > nExpirationBlock) { + remove = true; + CGovernanceObject* govobj = superblock->GetGovernanceObject(); + if(govobj) { + govobj->fExpired = true; + } + } + } default: break; } diff --git a/src/governance.cpp b/src/governance.cpp index 0f2a45dfeb80..643c19aa92a8 100644 --- a/src/governance.cpp +++ b/src/governance.cpp @@ -146,6 +146,7 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, std::string& strCommand, C else if (strCommand == NetMsgType::MNGOVERNANCEOBJECT) { + UpdateCachedBlockHeight(); LOCK(cs); // MAKE SURE WE HAVE A VALID REFERENCE TO THE TIP BEFORE CONTINUING @@ -252,6 +253,7 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, std::string& strCommand, C void CGovernanceManager::CheckOrphanVotes() { + UpdateCachedBlockHeight(); LOCK(cs); std::string strError = ""; @@ -268,6 +270,7 @@ void CGovernanceManager::CheckOrphanVotes() bool CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj) { + UpdateCachedBlockHeight(); LOCK(cs); std::string strError = ""; @@ -473,6 +476,7 @@ struct sortProposalsByVotes { void CGovernanceManager::NewBlock() { + UpdateCachedBlockHeight(); TRY_LOCK(cs, fBudgetNewBlock); if(!fBudgetNewBlock || !pCurrentBlockIndex) return; @@ -498,6 +502,7 @@ void CGovernanceManager::NewBlock() void CGovernanceManager::Sync(CNode* pfrom, uint256 nProp) { + UpdateCachedBlockHeight(); LOCK(cs); /* @@ -544,6 +549,7 @@ void CGovernanceManager::SyncParentObjectByVote(CNode* pfrom, const CGovernanceV bool CGovernanceManager::AddOrUpdateVote(const CGovernanceVote& vote, CNode* pfrom, std::string& strError) { + UpdateCachedBlockHeight(); LOCK(cs); // MAKE SURE WE HAVE THE PARENT OBJECT THE VOTE IS FOR diff --git a/src/governance.h b/src/governance.h index 5d7805814204..070f33acf423 100644 --- a/src/governance.h +++ b/src/governance.h @@ -69,6 +69,7 @@ class CGovernanceManager const CBlockIndex *pCurrentBlockIndex; int64_t nTimeLastDiff; + int nCachedBlockHeight; public: // critical section to protect the inner data structures @@ -87,9 +88,14 @@ class CGovernanceManager std::map mapVotesByHash; std::map mapVotesByType; - CGovernanceManager() { - mapObjects.clear(); - } + CGovernanceManager() + : mapCollateral(), + pCurrentBlockIndex(NULL), + nTimeLastDiff(0), + nCachedBlockHeight(0), + cs(), + mapObjects() + {} void ClearSeen() { mapSeenGovernanceObjects.clear(); @@ -157,6 +163,15 @@ class CGovernanceManager void UpdatedBlockTip(const CBlockIndex *pindex); int64_t GetLastDiffTime() {return nTimeLastDiff;} void UpdateLastDiffTime(int64_t nTimeIn) {nTimeLastDiff=nTimeIn;} + + int GetCachedBlockHeight() { return nCachedBlockHeight; } + +private: + void UpdateCachedBlockHeight() { + LOCK(cs_main); + nCachedBlockHeight = chainActive.Height(); + } + }; /** diff --git a/src/rpcgovernance.cpp b/src/rpcgovernance.cpp index c88ec3f56641..b5c6458f8a7c 100644 --- a/src/rpcgovernance.cpp +++ b/src/rpcgovernance.cpp @@ -487,12 +487,6 @@ UniValue gobject(const UniValue& params, bool fHelp) if(pGovObj == NULL) throw JSONRPCError(RPC_INVALID_PARAMETER, "Unknown govobj"); - CBlockIndex* pindex; - { - LOCK(cs_main); - pindex = chainActive.Tip(); - } - // REPORT BASIC OBJECT STATS LOCK2(cs_main, governance.cs); From e93023cdce7bbf41631bc6ad376091d9ba453860 Mon Sep 17 00:00:00 2001 From: Tim Flynn Date: Mon, 8 Aug 2016 08:52:58 -0400 Subject: [PATCH 5/5] Wrapped GetHash printf with DBG macro --- src/governance.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/governance.cpp b/src/governance.cpp index 643c19aa92a8..ad11061e1081 100644 --- a/src/governance.cpp +++ b/src/governance.cpp @@ -716,7 +716,7 @@ uint256 CGovernanceObject::GetHash() // fee_tx is left out on purpose uint256 h1 = ss.GetHash(); - printf("CGovernanceObject::GetHash %i %s %li %s\n", nRevision, strName.c_str(), nTime, strData.c_str()); + DBG( printf("CGovernanceObject::GetHash %i %s %li %s\n", nRevision, strName.c_str(), nTime, strData.c_str()); ); return h1; }