Skip to content

governance: add threading annotations, add default values, adjust constructors #4465

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
PastaPastaPasta wants to merge 10 commits into from
Closed

governance: add threading annotations, add default values, adjust constructors #4465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
751077d
governance: use more atomic, adjust mutexes
PastaPastaPasta Sep 26, 2021
a5b6114
fix
PastaPastaPasta Sep 28, 2021
4b51df0
std atomic
PastaPastaPasta Oct 1, 2021
3592b2d
return by value instead of by reference, as they are protected
PastaPastaPasta Oct 1, 2021
cd2275b
Lock cs as needed, adjust cs_main locking to hopefully avoid deadlock
PastaPastaPasta Oct 1, 2021
72967c9
use .load in constructor initializer list
PastaPastaPasta Oct 1, 2021
cc35ad2
make nTimeLastDiff, nCachedBlockHeight atomic, lock cs for cmapInvali…
PastaPastaPasta Oct 5, 2021
d843109
fixes
PastaPastaPasta Oct 11, 2021
0c31b32
fixes
PastaPastaPasta Oct 20, 2021
9325e6d
fix
PastaPastaPasta Oct 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/governance/classes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#define BITCOIN_GOVERNANCE_CLASSES_H

#include <amount.h>
#include <governance/governance.h>
#include <governance/object.h>
#include <script/script.h>
#include <script/standard.h>
Expand Down Expand Up @@ -37,9 +38,9 @@ class CGovernanceTriggerManager
private:
std::map<uint256, CSuperblock_sptr> mapTrigger;

std::vector<CSuperblock_sptr> GetActiveTriggers();
bool AddNewTrigger(uint256 nHash);
void CleanAndRemove();
std::vector<CSuperblock_sptr> GetActiveTriggers() EXCLUSIVE_LOCKS_REQUIRED(governance.cs);
bool AddNewTrigger(uint256 nHash) EXCLUSIVE_LOCKS_REQUIRED(governance.cs);
void CleanAndRemove() EXCLUSIVE_LOCKS_REQUIRED(governance.cs);

public:
CGovernanceTriggerManager() :
Expand All @@ -55,7 +56,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);
Expand Down Expand Up @@ -135,7 +136,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
{
Expand Down
35 changes: 12 additions & 23 deletions src/governance/governance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<CDeterministicMNList>()),
cs()
{
}

// Accessors for thread-safe access to maps
bool CGovernanceManager::HaveObjectForHash(const uint256& nHash) const
{
Expand Down Expand Up @@ -180,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);
Expand Down Expand Up @@ -260,6 +244,7 @@ void CGovernanceManager::CheckOrphanVotes(CGovernanceObject& govobj, CConnman& c
{
uint256 nHash = govobj.GetHash();
std::vector<vote_time_pair_t> vecVotePairs;
LOCK(cs);
cmmapOrphanVotes.GetAll(nHash, vecVotePairs);

ScopedLockBool guard(cs, fRateChecksEnabled, false);
Expand Down Expand Up @@ -295,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;
}
Expand All @@ -316,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))) {
Copy link

@UdjinM6 UdjinM6 Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for all governance.cs locks for triggerman here and below in this file - we hold cs already, no need to lock twice (I know you did this to silence werror probably but it feels like a wrong way to fix it)

LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- undo adding invalid trigger object: hash = %s\n", nHash.ToString());
objpair.first->second.PrepareDeletion(GetAdjustedTime());
return;
Expand Down Expand Up @@ -362,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();

Expand Down Expand Up @@ -703,6 +690,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()) {
Expand Down Expand Up @@ -859,6 +847,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);
Expand Down Expand Up @@ -1102,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());
}
}
Expand Down Expand Up @@ -1191,7 +1180,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();
Expand Down
36 changes: 20 additions & 16 deletions src/governance/governance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <cachemap.h>
#include <cachemultimap.h>
#include <evo/deterministicmns.h>
#include <governance/object.h>

class CBloomFilter;
Expand All @@ -18,12 +19,14 @@ class CGovernanceTriggerManager;
class CGovernanceObject;
class CGovernanceVote;

using CDeterministicMNListPtr = std::shared_ptr<CDeterministicMNList>;

extern CGovernanceManager governance;

static const int RATE_BUFFER_SIZE = 5;

class CDeterministicMNList;
using CDeterministicMNListPtr = std::shared_ptr<CDeterministicMNList>;
typedef std::shared_ptr<CDeterministicMNList> CDeterministicMNListPtr;

class CRateCheckBuffer
{
Expand Down Expand Up @@ -161,38 +164,38 @@ class CGovernanceManager
static const int MAX_TIME_FUTURE_DEVIATION;
static const int RELIABLE_PROPAGATION_TIME;

int64_t nTimeLastDiff;
std::atomic<int64_t> nTimeLastDiff;

// keep track of current block height
int nCachedBlockHeight;
std::atomic<int> nCachedBlockHeight;

// keep track of the scanning errors
std::map<uint256, CGovernanceObject> mapObjects;
std::map<uint256, CGovernanceObject> mapObjects GUARDED_BY(cs);

// mapErasedGovernanceObjects contains key-value pairs, where
// key - governance object's hash
// value - expiration time for deleted objects
std::map<uint256, int64_t> mapErasedGovernanceObjects;
std::map<uint256, int64_t> mapErasedGovernanceObjects GUARDED_BY(cs);

std::map<uint256, CGovernanceObject> mapPostponedObjects;
hash_s_t setAdditionalRelayObjects;
std::map<uint256, CGovernanceObject> 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<uint256, CGovernanceVote> cmapInvalidVotes;
CacheMap<uint256, CGovernanceVote> 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{true};

// used to check for changed voting keys
CDeterministicMNListPtr lastMNListForVotingKeys;
CDeterministicMNListPtr lastMNListForVotingKeys GUARDED_BY(cs) {std::make_shared<CDeterministicMNList>()};

class ScopedLockBool
{
Expand All @@ -218,7 +221,7 @@ class CGovernanceManager
// critical section to protect the inner data structures
mutable CCriticalSection cs;

CGovernanceManager();
CGovernanceManager() = default;

virtual ~CGovernanceManager() = default;

Expand Down Expand Up @@ -353,6 +356,7 @@ class CGovernanceManager

void AddInvalidVote(const CGovernanceVote& vote)
{
LOCK(cs);
cmapInvalidVotes.Insert(vote.GetHash(), vote);
}

Expand Down
Loading