Skip to content
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

Jump to bottom

refactor: minimize cs_main locking for governance #5623

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

refactor: minimize cs_main locking for governance #5623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 12 additions & 15 deletions src/governance/governance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,9 +167,8 @@ void CGovernanceManager::ProcessMessage(CNode& peer, PeerManager& peerman, CConn
return;
}

LOCK2(cs_main, cs);

if (mapObjects.count(nHash) || mapPostponedObjects.count(nHash) || mapErasedGovernanceObjects.count(nHash)) {
if (LOCK(cs); 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;
Expand Down Expand Up @@ -285,7 +284,6 @@ 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
Expand All @@ -297,6 +295,7 @@ void CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj, CConnman

LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- Adding object: hash = %s, type = %d\n", nHash.ToString(),
ToUnderlying(govobj.GetObjectType()));
LOCK(cs);

// INSERT INTO OUR GOVERNANCE OBJECT MEMORY
// IF WE HAVE THIS OBJECT ALREADY, WE DON'T WANT ANOTHER COPY
Expand Down Expand Up @@ -343,7 +342,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);
Expand Down Expand Up @@ -664,8 +663,11 @@ std::optional<CSuperblock> CGovernanceManager::CreateSuperblockCandidate(int nHe

void CGovernanceManager::CreateGovernanceTrigger(const CSuperblock& sb, CConnman& connman)
{
auto mnList = deterministicMNManager->GetListAtChainTip();
auto mn_payees = mnList.GetProjectedMNPayeesAtChainTip(); // Locks cs_main internally

//TODO: Check if nHashParentIn, nRevision and nCollateralHashIn are correct
LOCK2(cs_main, cs);
LOCK(cs);

// Check if identical trigger (equal DataHash()) is already created (signed by other masternode)
CGovernanceObject* gov_sb_voting{nullptr};
Expand All @@ -675,8 +677,6 @@ void CGovernanceManager::CreateGovernanceTrigger(const CSuperblock& sb, CConnman
if (identical_sb == nullptr) {
// Nobody submitted a trigger we'd like to see,
// so let's do it but only if we are the payee
auto mnList = deterministicMNManager->GetListAtChainTip();
auto mn_payees = mnList.GetProjectedMNPayeesAtChainTip();
if (mn_payees.empty()) return;
{
LOCK(activeMasternodeInfoCs);
Expand Down Expand Up @@ -1075,7 +1075,7 @@ void CGovernanceManager::CheckPostponedObjects(CConnman& connman)
{
if (!::masternodeSync->IsSynced()) return;

LOCK2(cs_main, cs);
LOCK(cs);

// Check postponed proposals
for (auto it = mapPostponedObjects.begin(); it != mapPostponedObjects.end();) {
Expand Down Expand Up @@ -1247,13 +1247,10 @@ int CGovernanceManager::RequestGovernanceObjectVotes(Span<CNode*> vNodesCopy, CC
// initiated from another node, so skip it too.
if (!pnode->CanRelay() || (fMasternodeMode && pnode->fInbound)) continue;
// stop early to prevent setAskFor overflow
{
LOCK(cs_main);
size_t nProjectedSize = GetRequestedObjectCount(pnode->GetId()) + nProjectedVotes;
if (nProjectedSize > MAX_INV_SZ) continue;
// to early to ask the same node
if (mapAskedRecently[nHashGovobj].count(pnode->addr)) continue;
}
size_t nProjectedSize = WITH_LOCK(cs_main, return GetRequestedObjectCount(pnode->GetId())) + nProjectedVotes;
if (nProjectedSize > MAX_INV_SZ) continue;
// to early to ask the same node
if (mapAskedRecently[nHashGovobj].count(pnode->addr)) continue;

RequestGovernanceObject(pnode, nHashGovobj, connman, true);
mapAskedRecently[nHashGovobj][pnode->addr] = nNow + nTimeout;
Expand Down
2 changes: 1 addition & 1 deletion src/governance/governance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ class CGovernanceManager : public GovernanceStore
void ProcessMessage(CNode& peer, PeerManager& peerman, CConnman& connman, std::string_view msg_type, CDataStream& vRecv);

std::optional<CSuperblock> CreateSuperblockCandidate(int nHeight) const;
void CreateGovernanceTrigger(const CSuperblock& sb, CConnman& connman);
void CreateGovernanceTrigger(const CSuperblock& sb, CConnman& connman) LOCKS_EXCLUDED(cs);
bool VoteFundingTrigger(const uint256& nHash, const vote_outcome_enum_t outcome, CConnman& connman);
bool HasAlreadyVotedFundingTrigger() const;
void ResetVotedFundingTrigger();
Expand Down
8 changes: 2 additions & 6 deletions src/governance/object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,6 @@ UniValue CGovernanceObject::ToJson() const

void CGovernanceObject::UpdateLocalValidity()
{
AssertLockHeld(cs_main);
// THIS DOES NOT CHECK COLLATERAL, THIS IS CHECKED UPON ORIGINAL ARRIVAL
fCachedLocalValidity = IsValidLocally(strLocalValidityError, false);
}
Expand All @@ -464,8 +463,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) {
Expand Down Expand Up @@ -537,14 +534,13 @@ CAmount CGovernanceObject::GetMinCollateralFee() const

bool CGovernanceObject::IsCollateralValid(std::string& strError, bool& fMissingConfirmations) const
{
AssertLockHeld(cs_main);

strError = "";
fMissingConfirmations = false;
uint256 nExpectedHash = GetHash();

// RETRIEVE TRANSACTION IN QUESTION
uint256 nBlockHash;
// Locks cs_main internally
CTransactionRef txCollateral = GetTransaction(/* block_index */ nullptr, /* mempool */ nullptr, nCollateralHash, Params().GetConsensus(), nBlockHash);
if (!txCollateral) {
strError = strprintf("Can't find collateral tx %s", nCollateralHash.ToString());
Expand Down Expand Up @@ -596,9 +592,9 @@ bool CGovernanceObject::IsCollateralValid(std::string& strError, bool& fMissingC

// GET CONFIRMATIONS FOR TRANSACTION

AssertLockHeld(cs_main);
int nConfirmationsIn = 0;
if (nBlockHash != uint256()) {
LOCK(cs_main);
const CBlockIndex* pindex = g_chainman.m_blockman.LookupBlockIndex(nBlockHash);
if (pindex && ::ChainActive().Contains(pindex)) {
nConfirmationsIn += ::ChainActive().Height() - pindex->nHeight + 1;
Expand Down
6 changes: 3 additions & 3 deletions src/governance/object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,12 +247,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 LOCKS_EXCLUDED(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 LOCKS_EXCLUDED(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 LOCKS_EXCLUDED(cs);

void UpdateLocalValidity();

Expand Down