-
Notifications
You must be signed in to change notification settings - Fork 1.2k
refactor: drop mutex and atomic from CMasternodeMetaInfo, access to object directly without shared_ptr #6894
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.
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
Changes from all commits
6cb2344
16abbed
7ec239b
7ba51ec
a40c418
b1a03e6
c0e146f
d5e693f
1bfd6ff
69ed5f5
b79dd90
ed27d90
04ab976
969b841
fe3966d
f860031
982b68e
16915ff
572bafd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -147,14 +147,7 @@ bool CGovernanceObject::ProcessVote(CMasternodeMetaMan& mn_metaman, CGovernanceM | |
| return false; | ||
| } | ||
|
|
||
| if (!mn_metaman.AddGovernanceVote(dmn->proTxHash, vote.GetParentHash())) { | ||
| std::string msg{strprintf("CGovernanceObject::%s -- Unable to add governance vote, MN outpoint = %s, " | ||
| "governance object hash = %s", | ||
| __func__, vote.GetMasternodeOutpoint().ToStringShort(), GetHash().ToString())}; | ||
| LogPrint(BCLog::GOBJECT, "%s\n", msg); | ||
| exception = CGovernanceException(msg, GOVERNANCE_EXCEPTION_PERMANENT_ERROR); | ||
| return false; | ||
| } | ||
| mn_metaman.AddGovernanceVote(dmn->proTxHash, vote.GetParentHash()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can this no longer fail?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See 969b841 |
||
|
|
||
| voteInstanceRef = vote_instance_t(vote.GetOutcome(), nVoteTimeUpdate, vote.GetTimestamp()); | ||
| fileVotes.AddVote(vote); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,13 @@ | |
|
|
||
| const std::string MasternodeMetaStore::SERIALIZATION_VERSION_STRING = "CMasternodeMetaMan-Version-5"; | ||
|
|
||
| static constexpr int MASTERNODE_MAX_FAILED_OUTBOUND_ATTEMPTS{5}; | ||
| static constexpr int MASTERNODE_MAX_MIXING_TXES{5}; | ||
|
|
||
| namespace { | ||
| static const CMasternodeMetaInfo default_meta_info{}; | ||
| } // anonymous namespace | ||
|
|
||
| CMasternodeMetaMan::CMasternodeMetaMan() : | ||
| m_db{std::make_unique<db_type>("mncache.dat", "magicMasternodeCache")} | ||
| { | ||
|
|
@@ -30,29 +37,24 @@ bool CMasternodeMetaMan::LoadCache(bool load_cache) | |
|
|
||
| UniValue CMasternodeMetaInfo::ToJson() const | ||
| { | ||
| UniValue ret(UniValue::VOBJ); | ||
|
|
||
| int64_t now = GetTime<std::chrono::seconds>().count(); | ||
|
|
||
| ret.pushKV("lastDSQ", nLastDsq.load()); | ||
| ret.pushKV("mixingTxCount", nMixingTxCount.load()); | ||
| ret.pushKV("outboundAttemptCount", outboundAttemptCount.load()); | ||
| ret.pushKV("lastOutboundAttempt", lastOutboundAttempt.load()); | ||
| ret.pushKV("lastOutboundAttemptElapsed", now - lastOutboundAttempt.load()); | ||
| ret.pushKV("lastOutboundSuccess", lastOutboundSuccess.load()); | ||
| ret.pushKV("lastOutboundSuccessElapsed", now - lastOutboundSuccess.load()); | ||
| { | ||
| LOCK(cs); | ||
| ret.pushKV("is_platform_banned", m_platform_ban); | ||
| ret.pushKV("platform_ban_height_updated", m_platform_ban_updated); | ||
| } | ||
| UniValue ret(UniValue::VOBJ); | ||
| ret.pushKV("lastDSQ", m_last_dsq); | ||
| ret.pushKV("mixingTxCount", m_mixing_tx_count); | ||
| ret.pushKV("outboundAttemptCount", outboundAttemptCount); | ||
| ret.pushKV("lastOutboundAttempt", lastOutboundAttempt); | ||
| ret.pushKV("lastOutboundAttemptElapsed", now - lastOutboundAttempt); | ||
| ret.pushKV("lastOutboundSuccess", lastOutboundSuccess); | ||
| ret.pushKV("lastOutboundSuccessElapsed", now - lastOutboundSuccess); | ||
| ret.pushKV("is_platform_banned", m_platform_ban); | ||
| ret.pushKV("platform_ban_height_updated", m_platform_ban_updated); | ||
|
|
||
| return ret; | ||
| } | ||
|
|
||
| void CMasternodeMetaInfo::AddGovernanceVote(const uint256& nGovernanceObjectHash) | ||
| { | ||
| LOCK(cs); | ||
| // Insert a zero value, or not. Then increment the value regardless. This | ||
| // ensures the value is in the map. | ||
| const auto& pair = mapGovernanceObjectsVotedOn.emplace(nGovernanceObjectHash, 0); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does mapGovernanceObjectsVotedOnnot need to be guarded? |
||
|
|
@@ -61,65 +63,81 @@ void CMasternodeMetaInfo::AddGovernanceVote(const uint256& nGovernanceObjectHash | |
|
|
||
| void CMasternodeMetaInfo::RemoveGovernanceObject(const uint256& nGovernanceObjectHash) | ||
| { | ||
| LOCK(cs); | ||
| // Whether or not the govobj hash exists in the map first is irrelevant. | ||
| mapGovernanceObjectsVotedOn.erase(nGovernanceObjectHash); | ||
| } | ||
|
|
||
| CMasternodeMetaInfoPtr CMasternodeMetaMan::GetMetaInfo(const uint256& proTxHash, bool fCreate) | ||
| const CMasternodeMetaInfo& CMasternodeMetaMan::GetMetaInfoOrDefault(const uint256& protx_hash) const | ||
| { | ||
| const auto it = metaInfos.find(protx_hash); | ||
| if (it == metaInfos.end()) return default_meta_info; | ||
| return it->second; | ||
| } | ||
|
|
||
| CMasternodeMetaInfo CMasternodeMetaMan::GetInfo(const uint256& proTxHash) const | ||
| { | ||
| LOCK(cs); | ||
| return GetMetaInfoOrDefault(proTxHash); | ||
| } | ||
|
|
||
| CMasternodeMetaInfo& CMasternodeMetaMan::GetMetaInfo(const uint256& proTxHash) | ||
| { | ||
| auto it = metaInfos.find(proTxHash); | ||
| if (it != metaInfos.end()) { | ||
| return it->second; | ||
| } | ||
| if (!fCreate) { | ||
| return nullptr; | ||
| } | ||
| it = metaInfos.emplace(proTxHash, std::make_shared<CMasternodeMetaInfo>(proTxHash)).first; | ||
| it = metaInfos.emplace(proTxHash, CMasternodeMetaInfo{proTxHash}).first; | ||
| return it->second; | ||
| } | ||
|
|
||
| // We keep track of dsq (mixing queues) count to avoid using same masternodes for mixing too often. | ||
| // This threshold is calculated as the last dsq count this specific masternode was used in a mixing | ||
| // session plus a margin of 20% of masternode count. In other words we expect at least 20% of unique | ||
| // masternodes before we ever see a masternode that we know already mixed someone's funds earlier. | ||
| int64_t CMasternodeMetaMan::GetDsqThreshold(const uint256& proTxHash, int nMnCount) | ||
| bool CMasternodeMetaMan::IsMixingThresholdExceeded(const uint256& protx_hash, int mn_count) const | ||
| { | ||
| auto metaInfo = GetMetaInfo(proTxHash); | ||
| if (metaInfo == nullptr) { | ||
| // return a threshold which is slightly above nDsqCount i.e. a no-go | ||
| return nDsqCount + 1; | ||
| LOCK(cs); | ||
| auto it = metaInfos.find(protx_hash); | ||
| if (it == metaInfos.end()) { | ||
| LogPrint(BCLog::COINJOIN, "DSQUEUE -- node %s is logged\n", protx_hash.ToString()); | ||
| return false; | ||
| } | ||
| return metaInfo->GetLastDsq() + nMnCount / 5; | ||
| const auto& meta_info = it->second; | ||
| int64_t last_dsq = meta_info.m_last_dsq; | ||
| int64_t threshold = last_dsq + mn_count / 5; | ||
|
|
||
| LogPrint(BCLog::COINJOIN, "DSQUEUE -- mn: %s last_dsq: %d dsq_threshold: %d nDsqCount: %d\n", | ||
| protx_hash.ToString(), last_dsq, threshold, nDsqCount); | ||
| return last_dsq != 0 && threshold > nDsqCount; | ||
| } | ||
|
|
||
| void CMasternodeMetaMan::AllowMixing(const uint256& proTxHash) | ||
| { | ||
| auto mm = GetMetaInfo(proTxHash); | ||
| nDsqCount++; | ||
| mm->nLastDsq = nDsqCount.load(); | ||
| mm->nMixingTxCount = 0; | ||
| LOCK(cs); | ||
| auto& mm = GetMetaInfo(proTxHash); | ||
| mm.m_last_dsq = ++nDsqCount; | ||
| mm.m_mixing_tx_count = 0; | ||
| } | ||
|
|
||
| void CMasternodeMetaMan::DisallowMixing(const uint256& proTxHash) | ||
| { | ||
| auto mm = GetMetaInfo(proTxHash); | ||
| mm->nMixingTxCount++; | ||
| LOCK(cs); | ||
| GetMetaInfo(proTxHash).m_mixing_tx_count++; | ||
| } | ||
|
|
||
| bool CMasternodeMetaMan::AddGovernanceVote(const uint256& proTxHash, const uint256& nGovernanceObjectHash) | ||
| bool CMasternodeMetaMan::IsValidForMixingTxes(const uint256& protx_hash) const | ||
| { | ||
| auto mm = GetMetaInfo(proTxHash); | ||
| mm->AddGovernanceVote(nGovernanceObjectHash); | ||
| return true; | ||
| LOCK(cs); | ||
| return GetMetaInfoOrDefault(protx_hash).m_mixing_tx_count <= MASTERNODE_MAX_MIXING_TXES; | ||
| } | ||
|
|
||
| void CMasternodeMetaMan::AddGovernanceVote(const uint256& proTxHash, const uint256& nGovernanceObjectHash) | ||
| { | ||
| LOCK(cs); | ||
| GetMetaInfo(proTxHash).AddGovernanceVote(nGovernanceObjectHash); | ||
| } | ||
|
|
||
| void CMasternodeMetaMan::RemoveGovernanceObject(const uint256& nGovernanceObjectHash) | ||
| { | ||
| LOCK(cs); | ||
| for(const auto& p : metaInfos) { | ||
| p.second->RemoveGovernanceObject(nGovernanceObjectHash); | ||
| for (auto& [_, meta_info] : metaInfos) { | ||
| meta_info.RemoveGovernanceObject(nGovernanceObjectHash); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -130,6 +148,65 @@ std::vector<uint256> CMasternodeMetaMan::GetAndClearDirtyGovernanceObjectHashes( | |
| return vecTmp; | ||
| } | ||
|
|
||
| void CMasternodeMetaMan::SetLastOutboundAttempt(const uint256& protx_hash, int64_t t) | ||
| { | ||
| LOCK(cs); | ||
| GetMetaInfo(protx_hash).SetLastOutboundAttempt(t); | ||
| } | ||
|
|
||
| void CMasternodeMetaMan::SetLastOutboundSuccess(const uint256& protx_hash, int64_t t) | ||
| { | ||
| LOCK(cs); | ||
| GetMetaInfo(protx_hash).SetLastOutboundSuccess(t); | ||
| } | ||
|
|
||
| int64_t CMasternodeMetaMan::GetLastOutboundAttempt(const uint256& protx_hash) const | ||
| { | ||
| LOCK(cs); | ||
| return GetMetaInfoOrDefault(protx_hash).lastOutboundAttempt; | ||
| } | ||
|
|
||
| int64_t CMasternodeMetaMan::GetLastOutboundSuccess(const uint256& protx_hash) const | ||
| { | ||
| LOCK(cs); | ||
| return GetMetaInfoOrDefault(protx_hash).lastOutboundSuccess; | ||
| } | ||
|
|
||
| bool CMasternodeMetaMan::OutboundFailedTooManyTimes(const uint256& protx_hash) const | ||
| { | ||
| LOCK(cs); | ||
| return GetMetaInfoOrDefault(protx_hash).outboundAttemptCount > MASTERNODE_MAX_FAILED_OUTBOUND_ATTEMPTS; | ||
| } | ||
|
|
||
| bool CMasternodeMetaMan::IsPlatformBanned(const uint256& protx_hash) const | ||
| { | ||
| LOCK(cs); | ||
| return GetMetaInfoOrDefault(protx_hash).m_platform_ban; | ||
| } | ||
|
|
||
| bool CMasternodeMetaMan::ResetPlatformBan(const uint256& protx_hash, int height) | ||
| { | ||
| LOCK(cs); | ||
|
|
||
| auto it = metaInfos.find(protx_hash); | ||
| if (it == metaInfos.end()) return false; | ||
|
|
||
| return it->second.SetPlatformBan(false, height); | ||
| } | ||
|
|
||
| bool CMasternodeMetaMan::SetPlatformBan(const uint256& inv_hash, PlatformBanMessage&& ban_msg) | ||
| { | ||
| LOCK(cs); | ||
|
|
||
| const uint256& protx_hash = ban_msg.m_protx_hash; | ||
|
|
||
| bool ret = GetMetaInfo(protx_hash).SetPlatformBan(true, ban_msg.m_requested_height); | ||
| if (ret) { | ||
| m_seen_platform_bans.insert(inv_hash, std::move(ban_msg)); | ||
| } | ||
| return ret; | ||
| } | ||
|
|
||
| bool CMasternodeMetaMan::AlreadyHavePlatformBan(const uint256& inv_hash) const | ||
| { | ||
| LOCK(cs); | ||
|
|
@@ -147,12 +224,6 @@ std::optional<PlatformBanMessage> CMasternodeMetaMan::GetPlatformBan(const uint2 | |
| return ret; | ||
| } | ||
|
|
||
| void CMasternodeMetaMan::RememberPlatformBan(const uint256& inv_hash, PlatformBanMessage&& msg) | ||
| { | ||
| LOCK(cs); | ||
| m_seen_platform_bans.insert(inv_hash, std::move(msg)); | ||
| } | ||
|
|
||
| void CMasternodeMetaMan::AddUsedMasternode(const uint256& proTxHash) | ||
| { | ||
| LOCK(cs); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard SetLastOutboundSuccess with mn meta lock
SetLastOutboundSuccessrequires the manager mutex to be held (the old code locked viaGetMetaInfo). We’re now calling it bare, which violates the contract and will assert. Wrap the call in the manager’s lock (e.g.WITH_LOCK(mn_metaman.GetCs(), mn_metaman.SetLastOutboundSuccess(...));).🤖 Prompt for AI Agents