Skip to content

Commit

Permalink
Fix issues with mapSeenGovernanceObjects (#1511)
Browse files Browse the repository at this point in the history 
* fix issues with mapSeenGovernanceObjects

Removed seen-governance-objects optimization except for deleted objects. Otherwise some nodes can permanently lost proposals if they received them too early.
Beside of that there is a vulnerability with seen-governance-objects mechanism if malicious node send us a lot of invalid governance objects.

* mapSeenGovernanceObjects renamed to mapErasedGovernanceObjects

* current fixes

* use int64_t for expiration timestamp
  • Loading branch information
@UdjinM6
Ilya Savinov authored and UdjinM6 committed Jul 12, 2017
1 parent 7b5556a commit f7aa815
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 70 deletions.
99 changes: 46 additions & 53 deletions src/governance.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ CGovernanceManager governance;

int nSubmittedFinalBudget;

const std::string CGovernanceManager::SERIALIZATION_VERSION_STRING = "CGovernanceManager-Version-11";
const std::string CGovernanceManager::SERIALIZATION_VERSION_STRING = "CGovernanceManager-Version-12";
const int CGovernanceManager::MAX_TIME_FUTURE_DEVIATION = 60*60;
const int CGovernanceManager::RELIABLE_PROPAGATION_TIME = 60;

Expand All @@ -27,7 +27,7 @@ CGovernanceManager::CGovernanceManager()
nTimeLastDiff(0),
nCachedBlockHeight(0),
mapObjects(),
mapSeenGovernanceObjects(),
mapErasedGovernanceObjects(),
mapMasternodeOrphanObjects(),
mapWatchdogObjects(),
nHashWatchdogCurrent(),
Expand Down Expand Up @@ -99,12 +99,6 @@ bool CGovernanceManager::SerializeVoteForHash(uint256 nHash, CDataStream& ss)
return true;
}

void CGovernanceManager::AddSeenGovernanceObject(uint256 nHash, int status)
{
LOCK(cs);
mapSeenGovernanceObjects[nHash] = status;
}

void CGovernanceManager::ProcessMessage(CNode* pfrom, std::string& strCommand, CDataStream& vRecv)
{
// lite mode is not supported
Expand Down Expand Up @@ -183,7 +177,8 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, std::string& strCommand, C

LOCK2(cs_main, cs);

if(mapSeenGovernanceObjects.count(nHash)) {
if(mapObjects.count(nHash) || mapPostponedObjects.count(nHash) ||
mapErasedGovernanceObjects.count(nHash) || mapMasternodeOrphanObjects.count(nHash)) {
// TODO - print error code? what if it's GOVOBJ_ERROR_IMMATURE?
LogPrint("gobject", "MNGOVERNANCEOBJECT -- Received already seen object: %s\n", strHash);
return;
Expand Down Expand Up @@ -218,9 +213,9 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, std::string& strCommand, C
LogPrintf("MNGOVERNANCEOBJECT -- Not enough fee confirmations for: %s, strError = %s\n", strHash, strError);
} else {
LogPrintf("MNGOVERNANCEOBJECT -- Governance object is invalid - %s\n", strError);
// TODO: apply node's ban score if object is invalid
}

mapSeenGovernanceObjects.insert(std::make_pair(nHash, SEEN_OBJECT_ERROR_INVALID));
return;
}

Expand Down Expand Up @@ -298,60 +293,30 @@ void CGovernanceManager::CheckOrphanVotes(CGovernanceObject& govobj, CGovernance

void CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj, CNode* pfrom)
{
DBG( cout << "CGovernanceManager::AddGovernanceObject START" << endl; );

uint256 nHash = govobj.GetHash();
std::string strHash = nHash.ToString();

// UPDATE CACHED VARIABLES FOR THIS OBJECT AND ADD IT TO OUR MANANGED DATA

govobj.UpdateSentinelVariables(); //this sets local vars in object

bool fAddToSeen = true;
if(AddGovernanceObject(govobj, fAddToSeen, pfrom))
{
LogPrintf("AddGovernanceObject -- %s new, received form %s\n", strHash, pfrom? pfrom->addrName : "NULL");
govobj.Relay();
}

// PROCESS OBJECT EXACTLY THE SAME WAY AS USUAL

if(fAddToSeen) {
// UPDATE THAT WE'VE SEEN THIS OBJECT
mapSeenGovernanceObjects.insert(std::make_pair(nHash, SEEN_OBJECT_IS_VALID));
// Update the rate buffer
MasternodeRateCheck(govobj, UPDATE_TRUE);
}

masternodeSync.AddedGovernanceItem();

// WE MIGHT HAVE PENDING/ORPHAN VOTES FOR THIS OBJECT

CGovernanceException exception;
CheckOrphanVotes(govobj, exception);
}

bool CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj, bool& fAddToSeen, CNode* pfrom)
{
LOCK2(cs_main, cs);
std::string strError = "";

DBG( cout << "CGovernanceManager::AddGovernanceObject START" << endl; );

fAddToSeen = true;

uint256 nHash = govobj.GetHash();

// MAKE SURE THIS OBJECT IS OK

if(!govobj.IsValidLocally(strError, true)) {
LogPrintf("CGovernanceManager::AddGovernanceObject -- invalid governance object - %s - (nCachedBlockHeight %d) \n", strError, nCachedBlockHeight);
return false;
return;
}

// IF WE HAVE THIS OBJECT ALREADY, WE DON'T WANT ANOTHER COPY

if(mapObjects.count(nHash)) {
LogPrintf("CGovernanceManager::AddGovernanceObject -- already have governance object %s\n", nHash.ToString());
return false;
return;
}

LogPrint("gobject", "CGovernanceManager::AddGovernanceObject -- Adding object: hash = %s, type = %d\n", nHash.ToString(), govobj.GetObjectType());
Expand All @@ -363,17 +328,16 @@ bool CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj, bool& fA
) {
// drop it
LogPrint("gobject", "CGovernanceManager::AddGovernanceObject -- CreationTime is out of bounds: hash = %s\n", nHash.ToString());
return false;
return;
}

if(!UpdateCurrentWatchdog(govobj)) {
// Allow wd's which are not current to be reprocessed
fAddToSeen = false;
if(pfrom && (nHashWatchdogCurrent != uint256())) {
pfrom->PushInventory(CInv(MSG_GOVERNANCE_OBJECT, nHashWatchdogCurrent));
}
LogPrint("gobject", "CGovernanceManager::AddGovernanceObject -- Watchdog not better than current: hash = %s\n", nHash.ToString());
return false;
return;
}
}

Expand Down Expand Up @@ -401,9 +365,20 @@ bool CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj, bool& fA
break;
}

DBG( cout << "CGovernanceManager::AddGovernanceObject END" << endl; );
LogPrintf("AddGovernanceObject -- %s new, received form %s\n", strHash, pfrom? pfrom->addrName : "NULL");
govobj.Relay();

return true;
// Update the rate buffer
MasternodeRateCheck(govobj, UPDATE_TRUE);

masternodeSync.AddedGovernanceItem();

// WE MIGHT HAVE PENDING/ORPHAN VOTES FOR THIS OBJECT

CGovernanceException exception;
CheckOrphanVotes(govobj, exception);

DBG( cout << "CGovernanceManager::AddGovernanceObject END" << endl; );
}

bool CGovernanceManager::UpdateCurrentWatchdog(CGovernanceObject& watchdogNew)
Expand Down Expand Up @@ -550,15 +525,33 @@ void CGovernanceManager::UpdateCachesAndClean()
++lit;
}
}
if(pObj->nObjectType == GOVERNANCE_OBJECT_WATCHDOG) {
mapWatchdogObjects.erase(it->first);

int64_t nSuperblockCycleSeconds = Params().GetConsensus().nSuperblockCycle * Params().GetConsensus().nPowTargetSpacing;
int64_t nTimeExpired = pObj->GetCreationTime() + 2 * nSuperblockCycleSeconds + GOVERNANCE_DELETION_DELAY;

if(pObj->GetObjectType() == GOVERNANCE_OBJECT_WATCHDOG) {
mapWatchdogObjects.erase(nHash);
} else if(pObj->GetObjectType() != GOVERNANCE_OBJECT_TRIGGER) {
// keep hashes of deleted proposals forever
nTimeExpired = std::numeric_limits<int64_t>::max();
}

mapErasedGovernanceObjects.insert(std::make_pair(nHash, nTimeExpired));
mapObjects.erase(it++);
} else {
++it;
}
}

// forget about expired deleted objects
hash_time_m_it s_it = mapErasedGovernanceObjects.begin();
while(s_it != mapErasedGovernanceObjects.end()) {
if(s_it->second < GetTime())
mapErasedGovernanceObjects.erase(s_it++);
else
++s_it;
}

LogPrintf("CGovernanceManager::UpdateCachesAndClean -- %s\n", ToString());
}

Expand Down Expand Up @@ -1385,9 +1378,9 @@ std::string CGovernanceManager::ToString() const
++it;
}

return strprintf("Governance Objects: %d (Proposals: %d, Triggers: %d, Watchdogs: %d/%d, Other: %d; Seen: %d), Votes: %d",
return strprintf("Governance Objects: %d (Proposals: %d, Triggers: %d, Watchdogs: %d/%d, Other: %d; Erased: %d), Votes: %d",
(int)mapObjects.size(),
nProposalCount, nTriggerCount, nWatchdogCount, mapWatchdogObjects.size(), nOtherCount, (int)mapSeenGovernanceObjects.size(),
nProposalCount, nTriggerCount, nWatchdogCount, mapWatchdogObjects.size(), nOtherCount, (int)mapErasedGovernanceObjects.size(),
(int)mapVoteToObject.GetSize());
}

Expand Down
24 changes: 7 additions & 17 deletions src/governance.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,12 +179,6 @@ class CGovernanceManager

typedef CacheMap<uint256, CGovernanceObject*> object_ref_cache_t;

typedef std::map<uint256, int> count_m_t;

typedef count_m_t::iterator count_m_it;

typedef count_m_t::const_iterator count_m_cit;

typedef std::map<uint256, CGovernanceVote> vote_m_t;

typedef vote_m_t::iterator vote_m_it;
Expand Down Expand Up @@ -238,7 +232,10 @@ class CGovernanceManager
// keep track of the scanning errors
object_m_t mapObjects;

count_m_t mapSeenGovernanceObjects;
// mapErasedGovernanceObjects contains key-value pairs, where
// key - governance object's hash
// value - expiration time for deleted objects
hash_time_m_t mapErasedGovernanceObjects;

object_time_m_t mapMasternodeOrphanObjects;

Expand Down Expand Up @@ -293,13 +290,6 @@ class CGovernanceManager

virtual ~CGovernanceManager() {}

int CountProposalInventoryItems()
{
// TODO What is this for ?
return mapSeenGovernanceObjects.size();
//return mapSeenGovernanceObjects.size() + mapSeenVotes.size();
}

/**
* This is called by AlreadyHave in main.cpp as part of the inventory
* retrieval process. Returns true if we want to retrieve the object, otherwise
Expand All @@ -321,7 +311,6 @@ class CGovernanceManager

bool IsBudgetPaymentBlock(int nBlockHeight);
void AddGovernanceObject(CGovernanceObject& govobj, CNode* pfrom = NULL);
bool AddGovernanceObject(CGovernanceObject& govobj, bool& fAddToSeen, CNode* pfrom = NULL);

std::string GetRequiredPaymentsString(int nBlockHeight);

Expand All @@ -335,7 +324,7 @@ class CGovernanceManager

LogPrint("gobject", "Governance object manager was cleared\n");
mapObjects.clear();
mapSeenGovernanceObjects.clear();
mapErasedGovernanceObjects.clear();
mapWatchdogObjects.clear();
nHashWatchdogCurrent = uint256();
nTimeWatchdogCurrent = 0;
Expand All @@ -360,7 +349,8 @@ class CGovernanceManager
strVersion = SERIALIZATION_VERSION_STRING;
READWRITE(strVersion);
}
READWRITE(mapSeenGovernanceObjects);

READWRITE(mapErasedGovernanceObjects);
READWRITE(mapInvalidVotes);
READWRITE(mapOrphanVotes);
READWRITE(mapObjects);
Expand Down

0 comments on commit f7aa815

Please sign in to comment.