refactor: drop mutex and atomic from CMasternodeMetaInfo, access to object directly without shared_ptr #6894
Conversation
knst
changed the title
WalkthroughRefactors masternode metadata to value-based CMasternodeMetaInfo with expanded CMasternodeMetaMan manager-level APIs. Call sites that previously accessed per-entry pointers (GetMetaInfo(...)->...) now call CMasternodeMetaMan methods (e.g., IsMixingThresholdExceeded, AllowMixing/DisallowMixing, IsValidForMixingTxes, Set/GetLastOutboundAttempt/Success, OutboundFailedTooManyTimes, AddGovernanceVote, Set/Reset/GetPlatformBan). DSQ/mixing guards, outbound tracking, governance vote handling, platform-ban forwarding, and related logging were updated across coinjoin, evo, llmq, governance, net, net_processing, rpc, and masternode meta modules. New constants and serialization changes added in the meta implementation. Sequence Diagram(s)sequenceDiagram
autonumber
participant Peer as Peer
participant CJServer as CoinJoin Server
participant MNMeta as CMasternodeMetaMan
Note right of Peer: DSACCEPT / DSQUEUE request
Peer->>CJServer: Send DSQ request (proTxHash, mnCount)
CJServer->>MNMeta: IsMixingThresholdExceeded(proTxHash, mnCount)?
alt exceeded == true
MNMeta-->>CJServer: true
CJServer->>Peer: Reject (ERR_RECENT) / concise log
else exceeded == false
MNMeta-->>CJServer: false
CJServer->>MNMeta: AllowMixing(proTxHash)
CJServer->>CJServer: Initialize queue/session and proceed
CJServer->>Peer: Continue DSQ handling
end
sequenceDiagram
autonumber
participant Node as NetProcessing
participant MNMeta as CMasternodeMetaMan
participant Prop as BanPropagator
Note right of Node: Platform ban detected/received
Node->>MNMeta: SetPlatformBan(inv_hash, ban_msg)
MNMeta-->>Node: applied? (bool)
alt applied == true
Node->>Prop: Enqueue ban for forwarding
Prop->>Network: Forward ban to peers
else applied == false
Node->>Logs: Already known / no-op
end
sequenceDiagram
autonumber
participant Auth as CMNAuth
participant MNMeta as CMasternodeMetaMan
Note right of Auth: Outbound success update
Auth->>MNMeta: SetLastOutboundSuccess(proRegTxHash, timestamp)
MNMeta-->>Auth: (void)
Auth->>Logs: Updated last outbound success timestamp
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Files/areas needing extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used🧠 Learnings (13)📓 Common learnings📚 Learning: 2025-07-09T15:02:26.899ZApplied to files:
📚 Learning: 2025-06-09T16:43:20.996ZApplied to files:
📚 Learning: 2025-11-04T18:24:27.241ZApplied to files:
📚 Learning: 2025-08-19T14:57:31.801ZApplied to files:
📚 Learning: 2025-09-09T21:36:11.833ZApplied to files:
📚 Learning: 2025-09-09T21:36:58.969ZApplied to files:
📚 Learning: 2025-10-21T11:09:34.688ZApplied to files:
📚 Learning: 2025-06-06T11:53:09.094ZApplied to files:
📚 Learning: 2025-10-05T20:38:28.457ZApplied to files:
📚 Learning: 2025-07-29T14:32:48.369ZApplied to files:
📚 Learning: 2025-11-13T20:02:55.480ZApplied to files:
📚 Learning: 2025-10-21T11:09:34.688ZApplied to files:
🧬 Code graph analysis (2)src/masternode/meta.cpp (1)
src/masternode/meta.h (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (6)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/net_processing.cpp (1)
3480-3485: Still need the mixing meta lock
IsValidForMixingTxesassumesCMasternodeMetaMan::csis held; the previous code performed the meta check inside the manager’s critical section. We call it here without locking, so the method’s internalAssertLockHeldwill trip (or worse, we race). AcquireWITH_LOCK(mn_metaman.GetCs(), return mn_metaman.IsValidForMixingTxes(...))(or similar helper) before invoking the new API.
🧹 Nitpick comments (5)
src/masternode/meta.h (4)
56-61: Serialization format change: confirm backward compatibility and handle duplicatesYou now serialize a vector of CMasternnodeMetaInfo by value. Ensure this matches prior on-disk format for the same SERIALIZATION_VERSION_STRING. Also, if the vector contains duplicate m_protx_hash entries, later ones are silently ignored by emplace.
Consider using insert_or_assign to make last-wins explicit.
Apply this small change to make behavior explicit:
- for (auto& mm : tmpMetaInfo) { - metaInfos.emplace(mm.m_protx_hash, CMasternodeMetaInfo{std::move(mm)}); - } + for (auto& mm : tmpMetaInfo) { + metaInfos.insert_or_assign(mm.m_protx_hash, std::move(mm)); + }
71-73: Add thread-safety annotation to outbound settersSetLastOutboundAttempt/Success mutate shared state. For consistency with the new locking model, add the same EXCLUSIVE_LOCKS_REQUIRED(!cs) (or appropriate annotation) used on other mutators.
39-41: Consider guarding against attempt counter overflowoutboundAttemptCount is int and is unbounded; in pathological cases it could overflow. Either use a wider unsigned type or saturate at a reasonable max in SetLastOutboundAttempt.
Example:
- void SetLastOutboundAttempt(int64_t t) { lastOutboundAttempt = t; ++outboundAttemptCount; } + void SetLastOutboundAttempt(int64_t t) { + lastOutboundAttempt = t; + if (outboundAttemptCount < std::numeric_limits<int>::max()) ++outboundAttemptCount; + }
92-106: Serialize/Unserialize: minor efficiency/readability nits
- Reserve tmpMetaInfo to avoid reallocations (metaInfos.size()).
- Use insert_or_assign during Unserialize as suggested above.
- std::vector<CMasternodeMetaInfo> tmpMetaInfo; - for (const auto& p : metaInfos) { - tmpMetaInfo.emplace_back(p.second); - } + std::vector<CMasternnodeMetaInfo> tmpMetaInfo; + tmpMetaInfo.reserve(metaInfos.size()); + for (const auto& [_, mi] : metaInfos) { + tmpMetaInfo.emplace_back(mi); + }Also applies to: 114-125
src/governance/object.cpp (1)
151-152: Behavioral change: dropped failure handling from AddGovernanceVotePreviously, failure from AddGovernanceVote aborted processing. The new API is void; processing now proceeds unconditionally. Confirm this is intentional and that rate/duplicate checks are fully covered earlier (fileVotes/rate checks).
If you still need a guard, consider having AddGovernanceVote return a bool (no-op vs. insert) and log on false.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/coinjoin/client.cpp(2 hunks)src/coinjoin/server.cpp(2 hunks)src/evo/deterministicmns.cpp(1 hunks)src/evo/mnauth.cpp(1 hunks)src/governance/object.cpp(1 hunks)src/llmq/dkgsession.cpp(1 hunks)src/llmq/utils.cpp(1 hunks)src/masternode/meta.cpp(4 hunks)src/masternode/meta.h(5 hunks)src/net.cpp(4 hunks)src/net_processing.cpp(2 hunks)src/rpc/evo.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/llmq/dkgsession.cppsrc/llmq/utils.cppsrc/net_processing.cppsrc/net.cppsrc/governance/object.cppsrc/coinjoin/server.cppsrc/coinjoin/client.cppsrc/rpc/evo.cppsrc/evo/deterministicmns.cppsrc/evo/mnauth.cppsrc/masternode/meta.hsrc/masternode/meta.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/deterministicmns.cppsrc/evo/mnauth.cppsrc/masternode/meta.hsrc/masternode/meta.cpp
🔇 Additional comments (13)
src/llmq/utils.cpp (1)
915-916: Good API migrationSwitched to mn_metaman.GetLastOutboundSuccess(...) keeps logic unchanged and centralizes access.
src/rpc/evo.cpp (1)
1382-1383: Avoids shared_ptr/meta indirectionUsing GetInfo(...).ToJson() is simpler and matches value-based storage. Minor perf note: this copies the meta before JSON, which is fine here.
src/llmq/dkgsession.cpp (1)
495-502: Correctly moved to manager-level checksOutboundFailedTooManyTimes/IsPlatformBanned calls on m_mn_metaman are consistent with the refactor and preserve behavior.
src/coinjoin/server.cpp (2)
81-91: Centralized DSQ gating via IsDsqOverGood consolidation; logs and ERR_RECENT behavior preserved.
173-179: Consistent DSQ abuse preventionUsing IsDsqOver(...) with GetValidMNsCount() matches the new API and intent to avoid domination.
src/masternode/meta.h (1)
200-206: Public API surface looks coherentThe value-returning GetInfo and centralized IsDsqOver/readers align with the new locking model and reduce unsafe exposure.
Optionally, verify no old pointer-style usages remain:
src/coinjoin/client.cpp (2)
119-124: Centralized DSQ rate-limit — looks good; confirm lockingUsing m_mn_metaman.IsDsqOver(...) simplifies and unifies the guard. Please confirm these new manager-level APIs perform internal locking of CMasternodeMetaMan::cs (since CMasternodeMetaInfo dropped its own mutex/atomic) so call sites don’t need to hold the lock explicitly.
1166-1169: Unified check in StartNewQueue — good; verify count semanticsThe IsDsqOver(dmn->proTxHash, nMnCount) gate is consistent with ProcessMessage. Please double-check IsDsqOver expects non-weighted MN count (GetValidMNsCount), not weighted, to avoid threshold drift.
src/net.cpp (5)
3445-3449: Swap to GetLastOutboundAttempt — verify time units and lockingnANow is seconds (GetTimestd::chrono::seconds().count()). Please confirm GetLastOutboundAttempt returns the same unit and that internal locking of CMasternodeMetaMan::cs is handled inside the method.
3469-3471: SetLastOutboundSuccess on existing outbound — ensure backoff resetThe success mark here is appropriate. Confirm it also clears/decays failure/backoff state for this MN so future attempts aren’t unnecessarily delayed.
3476-3482: Second use of GetLastOutboundAttempt — same unit/lock checkSame ask as above: confirm return unit matches nANow and that manager-level methods encapsulate required locking.
3527-3528: SetLastOutboundAttempt before dialing — semantics checkMarking the attempt pre-connection is fine for backoff. Please verify failure paths always advance failure counters (or similar) so OutboundFailedTooManyTimes reflects this attempt if connect fails immediately.
3541-3543: OutboundFailedTooManyTimes usage — confirm increment pathEnsure the code path that led here increments the failure count used by OutboundFailedTooManyTimes() within CMasternodeMetaMan, and that concurrency with list updates is internally synchronized.
src/evo/deterministicmns.cpp
Outdated
| if (m_mn_metaman.ResetPlatformBan(opt_proTx->proTxHash, nHeight)) { | ||
| LogPrint(BCLog::LLMQ, "%s -- MN %s is failed to Platform revived at height %d\n", __func__, | ||
| opt_proTx->proTxHash.ToString(), nHeight); | ||
| } |
There was a problem hiding this comment.
Clarify the log message for platform ban revival.
The log message "is failed to Platform revived" is grammatically incorrect and confusing. Since this block now executes when the revival succeeds (not when it fails, as in the previous implementation), consider revising the message to something clearer like:
LogPrint(BCLog::LLMQ, "%s -- MN %s platform ban revived at height %d\n", __func__,
opt_proTx->proTxHash.ToString(), nHeight);🤖 Prompt for AI Agents
In src/evo/deterministicmns.cpp around lines 671-674, the LogPrint message is
grammatically incorrect and misleading ("is failed to Platform revived") and
should reflect that ResetPlatformBan succeeded; update the log string to a
clear, concise message such as "MN %s platform ban revived at height %d"
(preserve __func__ and the proTxHash.ToString() and nHeight parameters) so the
log accurately represents a successful revival.
| if (!peer.IsInboundConn()) { | ||
| mn_metaman.GetMetaInfo(mnauth.proRegTxHash)->SetLastOutboundSuccess(GetTime<std::chrono::seconds>().count()); | ||
| mn_metaman.SetLastOutboundSuccess(mnauth.proRegTxHash, GetTime<std::chrono::seconds>().count()); | ||
| if (peer.m_masternode_probe_connection) { | ||
| LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- Masternode probe successful for %s, disconnecting. peer=%d\n", |
There was a problem hiding this comment.
Guard SetLastOutboundSuccess with mn meta lock
SetLastOutboundSuccess requires the manager mutex to be held (the old code locked via GetMetaInfo). 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
In src/evo/mnauth.cpp around lines 105 to 108, SetLastOutboundSuccess is called
without holding mn_metaman's mutex which violates the manager's locking contract
and will assert; wrap the call with the manager lock, e.g. use
WITH_LOCK(mn_metaman.GetCs(),
mn_metaman.SetLastOutboundSuccess(mnauth.proRegTxHash,
GetTime<std::chrono::seconds>().count())); and keep the rest of the logic
unchanged so the call is protected by the manager's mutex.
| // At this point, the outgoing message serialization version can't change. | ||
| const auto meta_info = m_mn_metaman.GetMetaInfo(ban_msg.m_protx_hash); | ||
| if (meta_info->SetPlatformBan(true, ban_msg.m_requested_height)) { | ||
| LogPrintf("PLATFORMBAN -- forward message to other nodes\n"); | ||
| m_mn_metaman.RememberPlatformBan(hash, std::move(ban_msg)); | ||
| if (m_mn_metaman.SetPlatformBan(hash, std::move(ban_msg))) { | ||
| LogPrintf("PLATFORMBAN -- hash: %s forward message to other nodes\n", hash.ToString()); | ||
| ret.m_inventory.emplace_back(MSG_PLATFORM_BAN, hash); | ||
| } |
There was a problem hiding this comment.
Missing ban propagation after failed SetPlatformBan
SetPlatformBan now returns false when the entry was already present. Previously we always forwarded by calling RememberPlatformBan regardless. Dropping the relay in this branch breaks propagation of matching quorum signatures (peers stop seeing refreshed bans). Keep forwarding when the manager reports “already had it”—only skip if the call throws/invalidates.
🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 3607 to 3611, the current logic only
forwards the platform-ban message when m_mn_metaman.SetPlatformBan returns true;
change it so that a false return that indicates “already present” still results
in forwarding the ban inventory (i.e., still emplace MSG_PLATFORM_BAN) so
matching quorum signature refreshes propagate — only avoid forwarding if
SetPlatformBan signals an error/invalidation (exception or explicit invalid
status); in practice, treat a normal false (already-existed) the same as true
for relay, and only suppress relay on thrown/invalid cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/masternode/meta.h (1)
54-54: Copy constructor may be expensive for large governance maps.The defaulted copy constructor will perform a deep copy of
mapGovernanceObjectsVotedOn, which could be expensive if a masternode has voted on many governance objects. SinceCMasternodeMetaInfois now returned by value fromGetInfo(), this copy cost will be incurred on every call.Consider whether the call sites need a full copy, or if they could work with const references where appropriate. If copies are necessary, document the performance implications or consider optimizing the storage structure if governance maps become large.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/masternode/meta.h(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/masternode/meta.h
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/meta.h
🔇 Additional comments (6)
src/masternode/meta.h (6)
98-106: LGTM!The serialization correctly creates a vector from the value-based map entries. The use of
p.secondat line 103 properly extracts theCMasternodeMetaInfovalue from the map.
109-124: LGTM!The unserialization correctly reconstructs the map using
mm.m_protx_hashas the key and move semantics for the value. This aligns well with the value-based storage model.
188-190: LGTM!The private helpers correctly return references (
GetMetaInfo) and const references (GetMetaInfoOrDefault) while requiring the lock to be held by the caller. This is an efficient pattern for internal use within the class.
199-199: LGTM!Returning
CMasternodeMetaInfoby value ensures thread safety by providing a copy to the caller, eliminating the need for the caller to hold the lock after the method returns. This is the correct approach for the public API.
205-227: LGTM!The public API methods are consistently annotated with
EXCLUSIVE_LOCKS_REQUIRED(!cs), indicating they acquire the lock internally. This provides a clean, thread-safe interface for callers who don't need to manage locking manually.
72-83: Verify platform ban idempotency at the same height.The logic at lines 77-78 prevents unsetting a ban at the same height, but allows setting a ban multiple times at the same height (when
is_banned == true). This might allow duplicate processing of the same ban message.Verify whether callers deduplicate ban messages before calling
SetPlatformBan, or if this method should enforce stricter idempotency:
knst
force-pushed
the
fix-drop-meta-cs
branch
from
6834d68 to
cdc9825
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/masternode/meta.cpp (1)
93-108: Critical logic error in IsDsqOver already flagged in past review.This issue was previously identified: Line 97 checks
if (it != metaInfos.end())(meaning "found"), but then returns false and does not use the iterator. Subsequently, line 101 attempts to accessit->second, which is undefined behavior when the iterator is atend()(not found).The condition must be inverted to check for "not found" before returning false.
Apply this diff to fix the logic error:
bool CMasternodeMetaMan::IsDsqOver(const uint256& protx_hash, int mn_count) const { LOCK(cs); auto it = metaInfos.find(protx_hash); - if (it != metaInfos.end()) { - LogPrint(BCLog::COINJOIN, "DSQUEUE -- node %s is logged\n", protx_hash.ToString()); + if (it == metaInfos.end()) { + LogPrint(BCLog::COINJOIN, "DSQUEUE -- node %s not found in metaInfos\n", protx_hash.ToString()); return false; } 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; }
🧹 Nitpick comments (1)
src/masternode/meta.cpp (1)
16-18: Minor: Consider renaming the default instance for clarity.The variable name
default_meta_info_meta_infocontains redundant repetition. Consider simplifying todefault_meta_infoordefault_metainfo.Apply this diff to improve naming:
namespace { -static const CMasternodeMetaInfo default_meta_info_meta_info{}; +static const CMasternodeMetaInfo default_meta_info{}; } // anonymous namespaceThen update line 73:
- if (it == metaInfos.end()) return default_meta_info_meta_info; + if (it == metaInfos.end()) return default_meta_info;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/masternode/meta.cpp(4 hunks)src/masternode/meta.h(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/masternode/meta.cppsrc/masternode/meta.h
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/meta.cppsrc/masternode/meta.h
🔇 Additional comments (13)
src/masternode/meta.cpp (4)
110-118: Verify atomic increment ordering is correct.
nDsqCount++(line 112) increments the atomic counter before acquiring the lock. WhilenDsqCountis atomic and this is safe for the increment itself, confirm that readingnDsqCount.load()on line 116 after incrementing but potentially from a different thread's perspective won't cause any DSQ sequencing issues.The current pattern appears intentional—incrementing the global counter first, then recording that value under lock—but please verify this ordering satisfies the DSQ queue semantics across concurrent AllowMixing calls.
126-130: Confirm mixing transaction threshold semantics.Line 129 uses
m_mixing_tx_count <= MASTERNODE_MAX_MIXING_TXES, allowing up to 5 mixing transactions inclusive. Ensure this matches the intended policy (i.e., 5 is the maximum allowed, not 4).
189-197: LGTM: ResetPlatformBan correctly handles missing entries.Returning
falsewhen the masternode entry does not exist (line 194) is appropriate—you cannot reset a ban on a nonexistent entry.
199-210: LGTM: SetPlatformBan creates entry if needed and caches ban message.The method correctly uses
GetMetaInfo(which creates an entry if absent), attempts the ban update, and on success caches the ban message. The move semantics forban_msgare efficient.src/masternode/meta.h (9)
29-46: LGTM: Public member variables align with value-based refactor.Exposing
CMasternodeMetaInfofields publicly is appropriate given the refactor removes per-entry locking and centralizes synchronization at the manager level (CMasternodeMetaMan::cs).
56-61: LGTM: Serialization includes all necessary fields.All members—including
m_protx_hash, DSQ/mixing state, governance votes, outbound tracking, and platform ban state—are serialized. This preserves state across restarts.
69-70: LGTM: Outbound attempt/success setters correctly update counters.
SetLastOutboundAttemptincrements the attempt counter, whileSetLastOutboundSuccessresets it. This logic supports the "too many failed attempts" check.
72-83: Verify platform ban state transition semantics.The
SetPlatformBanlogic rejects older heights (line 74-76) and rejects unbanning at the same height as a ban (line 77-79). Confirm this aligns with the intended platform ban state machine—specifically, that unbanning at the same height as banning should indeed fail.If the semantics are correct, consider adding a brief comment explaining why same-height unbans are rejected, as this is subtle.
92-92: LGTM: Value-based map storage aligns with refactor objectives.Switching from
std::map<uint256, std::shared_ptr<CMasternodeMetaInfo>>tostd::map<uint256, CMasternodeMetaInfo>removes pointer indirection and simplifies memory management, as intended by this refactor.
102-103: LGTM: Serialization correctly copies value-based entries.Iterating
metaInfosand emplacingp.secondinto the temporary vector is correct for the value-based map.
122-122: LGTM: Unserialization efficiently moves entries into map.Using
CMasternodeMetaInfo{std::move(mm)}avoids copying the unserialized object. The keymm.m_protx_hashis extracted before the move, which is correct.
188-189: LGTM: Private accessor lock annotations match implementations.The
EXCLUSIVE_LOCKS_REQUIRED(cs)annotations correctly document that callers must holdcswhen invoking these private helpers.
199-227: LGTM: Public API lock annotations correctly specify external requirements.All public methods use
EXCLUSIVE_LOCKS_REQUIRED(!cs), documenting that callers must not hold the lock (the methods acquire it internally). This prevents deadlocks and centralizes locking at the manager level.
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
fix-drop-meta-cs
branch
from
cdc9825 to
d72d404
Compare
knst
force-pushed
the
fix-drop-meta-cs
branch
from
d72d404 to
9d84956
Compare
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
fix-drop-meta-cs
branch
from
9d84956 to
a657aff
Compare
knst
force-pushed
the
fix-drop-meta-cs
branch
from
a657aff to
fe3966d
Compare
Co-Authored-By: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-Authored-By: UdjinM6 <UdjinM6@users.noreply.github.com>
knst
force-pushed
the
fix-drop-meta-cs
branch
from
5da1a99 to
982b68e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/masternode/meta.cpp (1)
93-108: Correct the misleading log message.The iterator check on line 97 is now correct (checks for
end()), but the log message on line 98 still says "node %s is logged" when the node is not found. This was flagged in previous reviews as needing correction.Apply this diff to clarify the message:
if (it == metaInfos.end()) { - LogPrint(BCLog::COINJOIN, "DSQUEUE -- node %s is logged\n", protx_hash.ToString()); + LogPrint(BCLog::COINJOIN, "DSQUEUE -- node %s not found in metaInfos\n", protx_hash.ToString()); return false; }
🧹 Nitpick comments (1)
src/masternode/meta.h (1)
55-55: Explicitly default the move constructor.Line 55 defaults only the copy constructor. For a value-based type, explicitly defaulting the move constructor improves clarity and ensures optimal move semantics.
Apply this diff:
CMasternodeMetaInfo(const CMasternodeMetaInfo& ref) = default; +CMasternodeMetaInfo(CMasternodeMetaInfo&& ref) = default;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/evo/deterministicmns.cpp(1 hunks)src/masternode/meta.cpp(4 hunks)src/masternode/meta.h(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/evo/deterministicmns.cpp
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/masternode/meta.hsrc/masternode/meta.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/meta.hsrc/masternode/meta.cpp
🧠 Learnings (7)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/masternode/meta.hsrc/masternode/meta.cpp
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/masternode/meta.h
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
Applied to files:
src/masternode/meta.h
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
src/masternode/meta.cpp
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
src/masternode/meta.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/masternode/meta.cpp
🧬 Code graph analysis (2)
src/masternode/meta.h (1)
src/masternode/meta.cpp (42)
ToJson(38-54)ToJson(38-38)AddGovernanceVote(56-62)AddGovernanceVote(56-56)AddGovernanceVote(130-134)AddGovernanceVote(130-130)RemoveGovernanceObject(64-68)RemoveGovernanceObject(64-64)RemoveGovernanceObject(136-142)RemoveGovernanceObject(136-136)SetLastOutboundAttempt(151-155)SetLastOutboundAttempt(151-151)SetLastOutboundSuccess(157-161)SetLastOutboundSuccess(157-157)SetPlatformBan(197-208)SetPlatformBan(197-197)GetMetaInfo(83-91)GetMetaInfo(83-83)GetMetaInfoOrDefault(70-75)GetMetaInfoOrDefault(70-70)GetInfo(77-81)GetInfo(77-77)IsDsqOver(93-108)IsDsqOver(93-93)AllowMixing(110-116)AllowMixing(110-110)DisallowMixing(118-122)DisallowMixing(118-118)IsValidForMixingTxes(124-128)IsValidForMixingTxes(124-124)GetAndClearDirtyGovernanceObjectHashes(144-149)GetAndClearDirtyGovernanceObjectHashes(144-144)GetLastOutboundAttempt(163-167)GetLastOutboundAttempt(163-163)GetLastOutboundSuccess(169-173)GetLastOutboundSuccess(169-169)OutboundFailedTooManyTimes(175-179)OutboundFailedTooManyTimes(175-175)IsPlatformBanned(181-185)IsPlatformBanned(181-181)ResetPlatformBan(187-195)ResetPlatformBan(187-187)
src/masternode/meta.cpp (1)
src/masternode/meta.h (10)
m_last_dsq(34-34)m_mixing_tx_count(35-35)outboundAttemptCount(40-40)lastOutboundAttempt(41-41)lastOutboundSuccess(42-42)m_platform_ban(45-85)m_platform_ban_updated(47-47)nDsqCount(95-95)SetLastOutboundSuccess(71-71)SetPlatformBan(73-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
🔇 Additional comments (6)
src/masternode/meta.h (2)
30-85: Value-based design correctly removes internal locking.The refactoring to expose fields directly and rely on external locking at the manager level is sound. All access patterns are now guarded by
CMasternodeMetaMan::cs, eliminating the overhead of per-entry atomics and mutexes.
203-204: Private accessor methods correctly require caller to hold lock.
GetMetaInfoandGetMetaInfoOrDefaultare annotated withEXCLUSIVE_LOCKS_REQUIRED(cs), ensuring they're only called from within locked contexts. This aligns with the value-based refactoring goal.src/masternode/meta.cpp (4)
13-18: Constants and default instance are well-placed.Using
constexprfor the thresholds and an anonymous-namespace default instance forCMasternodeMetaInfois clean and efficient.
70-81: GetMetaInfoOrDefault and GetInfo correctly handle missing entries.
GetMetaInfoOrDefaultreturns a const reference to a default instance when the entry is not found, avoiding unnecessary allocations.GetInforeturns a copy for safe cross-lock-boundary access.
110-122: AllowMixing and DisallowMixing correctly update metadata.Both methods properly acquire the lock and update the relevant fields. The pre-increment of
nDsqCountin line 114 is clean.
151-208: Outbound and platform ban methods are well-structured.All methods correctly acquire
cs, delegate to per-entry methods or access fields directly, and return the appropriate values. The platform ban logic inSetPlatformBanproperly integrates with the LRU cache.
knst
force-pushed
the
fix-drop-meta-cs
branch
from
0386115 to
16915ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/masternode/meta.h (1)
31-32: Keep the stored proTx hash immutable.
m_protx_hashis the identity that ties a meta entry to themetaInfosmap key. Making it mutable invites accidental reassignments (now that the field is public), leaving the stored value out of sync with the key and breaking lookups. Please keep this memberconst(as it effectively was before) and initialize it in the constructors; if copy assignment ever becomes necessary we can provide a custom implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/masternode/meta.h(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/masternode/meta.h
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/meta.h
🧠 Learnings (4)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/masternode/meta.h
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/masternode/meta.h
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
Applied to files:
src/masternode/meta.h
🧬 Code graph analysis (1)
src/masternode/meta.h (1)
src/masternode/meta.cpp (42)
ToJson(38-54)ToJson(38-38)AddGovernanceVote(56-62)AddGovernanceVote(56-56)AddGovernanceVote(130-134)AddGovernanceVote(130-130)RemoveGovernanceObject(64-68)RemoveGovernanceObject(64-64)RemoveGovernanceObject(136-142)RemoveGovernanceObject(136-136)SetLastOutboundAttempt(151-155)SetLastOutboundAttempt(151-151)SetLastOutboundSuccess(157-161)SetLastOutboundSuccess(157-157)SetPlatformBan(197-208)SetPlatformBan(197-197)GetMetaInfo(83-91)GetMetaInfo(83-83)GetMetaInfoOrDefault(70-75)GetMetaInfoOrDefault(70-70)GetInfo(77-81)GetInfo(77-77)IsDsqOver(93-108)IsDsqOver(93-93)AllowMixing(110-116)AllowMixing(110-110)DisallowMixing(118-122)DisallowMixing(118-118)IsValidForMixingTxes(124-128)IsValidForMixingTxes(124-124)GetAndClearDirtyGovernanceObjectHashes(144-149)GetAndClearDirtyGovernanceObjectHashes(144-144)GetLastOutboundAttempt(163-167)GetLastOutboundAttempt(163-163)GetLastOutboundSuccess(169-173)GetLastOutboundSuccess(169-169)OutboundFailedTooManyTimes(175-179)OutboundFailedTooManyTimes(175-175)IsPlatformBanned(181-185)IsPlatformBanned(181-181)ResetPlatformBan(187-195)ResetPlatformBan(187-187)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/masternode/meta.h (2)
129-131: Clarify move semantics by capturing the key before move.While the current code is safe (C++17 guarantees left-to-right evaluation and
uint256is likely trivially copyable), accessingmm.m_protx_hashin the same expression asstd::move(mm)is potentially confusing.Consider this refactor for improved clarity:
for (auto& mm : tmpMetaInfo) { - metaInfos.emplace(mm.m_protx_hash, CMasternodeMetaInfo{std::move(mm)}); + const uint256 protx_hash = mm.m_protx_hash; + metaInfos.emplace(protx_hash, std::move(mm)); }
106-109: Consider reserving vector capacity for serialization.Pre-allocating space for the temporary vector would avoid potential reallocations during the copy loop.
std::vector<CMasternodeMetaInfo> tmpMetaInfo; +tmpMetaInfo.reserve(metaInfos.size()); for (const auto& p : metaInfos) { tmpMetaInfo.emplace_back(p.second); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/masternode/meta.h(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/masternode/meta.h
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/meta.h
🧠 Learnings (4)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/masternode/meta.h
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/masternode/meta.h
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
Applied to files:
src/masternode/meta.h
🧬 Code graph analysis (1)
src/masternode/meta.h (1)
src/masternode/meta.cpp (42)
ToJson(38-54)ToJson(38-38)AddGovernanceVote(56-62)AddGovernanceVote(56-56)AddGovernanceVote(130-134)AddGovernanceVote(130-130)RemoveGovernanceObject(64-68)RemoveGovernanceObject(64-64)RemoveGovernanceObject(136-142)RemoveGovernanceObject(136-136)SetLastOutboundAttempt(151-155)SetLastOutboundAttempt(151-151)SetLastOutboundSuccess(157-161)SetLastOutboundSuccess(157-157)SetPlatformBan(197-208)SetPlatformBan(197-197)GetMetaInfo(83-91)GetMetaInfo(83-83)GetMetaInfoOrDefault(70-75)GetMetaInfoOrDefault(70-70)GetInfo(77-81)GetInfo(77-77)IsDsqOver(93-108)IsDsqOver(93-93)AllowMixing(110-116)AllowMixing(110-110)DisallowMixing(118-122)DisallowMixing(118-118)IsValidForMixingTxes(124-128)IsValidForMixingTxes(124-124)GetAndClearDirtyGovernanceObjectHashes(144-149)GetAndClearDirtyGovernanceObjectHashes(144-144)GetLastOutboundAttempt(163-167)GetLastOutboundAttempt(163-163)GetLastOutboundSuccess(169-173)GetLastOutboundSuccess(169-169)OutboundFailedTooManyTimes(175-179)OutboundFailedTooManyTimes(175-175)IsPlatformBanned(181-185)IsPlatformBanned(181-181)ResetPlatformBan(187-195)ResetPlatformBan(187-187)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (2)
src/masternode/meta.h (2)
28-85: LGTM! Clean refactor to value-based semantics.The transition from mutex-guarded, pointer-backed entries to a public value structure is well-executed. All fields are appropriately public since access is controlled by
CMasternodeMetaMan::cs. TheSetPlatformBanlogic correctly prevents stale or redundant updates.
203-241: LGTM! Well-structured manager API with proper locking.The expanded public API follows a clean pattern:
- Private helpers (
GetMetaInfo,GetMetaInfoOrDefault) requirecsheld- Public methods acquire
csand delegate to private helpers- Locking annotations are consistent and correct
The move from per-object to manager-level locking simplifies the design and is well-suited to the brief access patterns across the codebase.
src/masternode/meta.h
Outdated
| // MN's 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. | ||
| bool IsDsqOver(const uint256& protx_hash, int mn_count) const EXCLUSIVE_LOCKS_REQUIRED(!cs); |
There was a problem hiding this comment.
IsDsqOver as name doesn't make a lot of sense to me. Should it be, DidMasternodeMixRecently or CanMasternodeMix or something like that @coderabbitai, please propose some better names if you have better ideas than those.
There was a problem hiding this comment.
@PastaPastaPasta, I agree that IsDsqOver could be clearer. Here are some naming alternatives based on the method's behavior (returns true when a masternode has been used too recently for mixing and should be skipped):
Option 1: HasMasternodeExceededMixingThreshold
- Most explicit about both what's being checked and the threshold concept
- Clear boolean semantics:
true= exceeded,false= hasn't exceeded - Longer but unambiguous
Option 2: IsMasternodeRecentlyUsedForMixing
- More natural language, immediately understandable
- Shorter while still being clear
- "Recently" captures the time-based threshold aspect
Option 3: ShouldSkipMasternodeForMixing
- Action-oriented name that matches how it's used in the code
- Very clear intent:
true= skip it,false= use it - Aligns well with usage patterns in the codebase
Option 4: WasMasternodeUsedTooRecently (slight variation)
- Past tense emphasizes the check is about historical usage
- "Too recently" incorporates the threshold notion
- Natural phrasing
My recommendation would be Option 2 (IsMasternodeRecentlyUsedForMixing) or Option 3 (ShouldSkipMasternodeForMixing), as they strike the best balance between clarity and conciseness. Option 3 might be slightly better since it directly expresses the decision outcome rather than requiring the caller to interpret what "recently used" means for their use case.
What do you think?
🧠 Learnings used
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6969
File: src/evo/deterministicmns.h:441-479
Timestamp: 2025-11-13T20:02:55.480Z
Learning: In `src/evo/deterministicmns.h`, the `internalId` field in `CDeterministicMN` and the `mnInternalIdMap` in `CDeterministicMNList` are non-deterministic and used only for internal bookkeeping and efficient lookups. Different nodes can assign different internalIds to the same masternode depending on their sync history. Methods like `IsEqual()` intentionally ignore internalId mappings and only compare consensus-critical deterministic fields (proTxHash, collateral, state, etc.).
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
There was a problem hiding this comment.
@knst pls apply something like this; otherwise, I'm happy with it
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I used IsMixingThresholdExceeded because it's a member of CMasternodeMetaMan and it has an argument protx_hash so it's already tell that it is masternode's related method (along all other members that has protx_hash as an argument)
| exception = CGovernanceException(msg, GOVERNANCE_EXCEPTION_PERMANENT_ERROR); | ||
| return false; | ||
| } | ||
| mn_metaman.AddGovernanceVote(dmn->proTxHash, vote.GetParentHash()); |
There was a problem hiding this comment.
can this no longer fail?
| 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); |
There was a problem hiding this comment.
does mapGovernanceObjectsVotedOnnot need to be guarded?
| private: | ||
| mutable Mutex cs; | ||
|
|
||
| uint256 proTxHash GUARDED_BY(cs); | ||
| public: | ||
| uint256 m_protx_hash; | ||
|
|
||
| //the dsq count from the last dsq broadcast of this node | ||
| std::atomic<int64_t> nLastDsq{0}; | ||
| std::atomic<int> nMixingTxCount{0}; | ||
| //! the dsq count from the last dsq broadcast of this node | ||
| int64_t m_last_dsq{0}; | ||
| int m_mixing_tx_count{0}; | ||
|
|
||
| // KEEP TRACK OF GOVERNANCE ITEMS EACH MASTERNODE HAS VOTE UPON FOR RECALCULATION | ||
| std::map<uint256, int> mapGovernanceObjectsVotedOn GUARDED_BY(cs); | ||
| std::map<uint256, int> mapGovernanceObjectsVotedOn; | ||
|
|
||
| std::atomic<int> outboundAttemptCount{0}; | ||
| std::atomic<int64_t> lastOutboundAttempt{0}; | ||
| std::atomic<int64_t> lastOutboundSuccess{0}; | ||
| int outboundAttemptCount{0}; | ||
| int64_t lastOutboundAttempt{0}; | ||
| int64_t lastOutboundSuccess{0}; |
There was a problem hiding this comment.
please explain how these are thread safe
There was a problem hiding this comment.
They are protected by cs murex of manager, check PR description
3 participants
Issue being fixed or feature implemented
Current implementation:
CMasternodeMetaInfois done by accessingGetMetaInfounder mutexCMasternodeMetaMan::csGetMetaInforeturns shared_ptr to objectGetMetaInfois protected by atomic, second mutexCMasternodeMetaInfo::cs; access to object is done indirectly by one more memory hop (because shared_ptr)It is not efficient, because
GetMetaInfois spread all over code base and not a single case need shared_ptr here; all accesses are brief and short (single read or single write; except RPC where all structure is serialized).What was done?
This PR is follow-up for PR #6868 with further improvements and simplification of code.
Instead returning shared_ptr with
CMasternodeMetaInfodirectly access to members ofCMasternodeMetaManunder its mutex; all mutexes, atomic and shared_ptr itself are removed fromCMasternodeMetaInfoaccesses.It simplified implementation significantly, removed big amount of thread-safety annotations.
Performance is probably improved, but improvement is unmeasurable, because
CMasternodeMetaInfoandCMasternodeMetaManhas not been spotted as hot spot in profiler neither hot spot in lock contentions logs.Also this PR removes duplicated code between coinjoin/client and coinjoin/server by creating a helper in
CMasternodeMetaManHow Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Checklist: