refactor: update lock annotations for CGovernanceObject and GovernanceStore, move away from RecursiveMutexes
#6849
Conversation
|
|
This pull request has conflicts, please rebase. |
|
This pull request has conflicts, please rebase. |
|
This pull request has conflicts, please rebase. |
…`GovernanceSigner`, drop `Relay()`s and use periodic relay instead, minor cleanup b51cd1d lint: apply most `clang-format` suggestions, update circular allowlist (Kittywhiskers Van Gogh) 700e069 refactor: cleanup headers and forward decls (Kittywhiskers Van Gogh) c516fd3 refactor: apply review suggestions (Kittywhiskers Van Gogh) d19e1f3 fix: add `nullptr` check before using `FindGovernanceObject()` retval (Kittywhiskers Van Gogh) 1777919 refactor: use `std::chrono` for governance time constants (Kittywhiskers Van Gogh) 4d96f5f refactor: move `DoMaintenance()` inside `Schedule()` (Kittywhiskers Van Gogh) e224991 governance: add lock annotations for `cs_relay` (Kittywhiskers Van Gogh) df589c7 governance: introduce task for relaying governance objects (Kittywhiskers Van Gogh) 3811924 governance: drop `Relay()` from `CGovernance{Object,Vote}` (Kittywhiskers Van Gogh) 63448ff refactor: abstract away parent implementation from signer (Kittywhiskers Van Gogh) 757ded3 refactor: remove need for access to private members (Kittywhiskers Van Gogh) bd6af83 refactor: separate masternode mode logic into dedicated signer class (Kittywhiskers Van Gogh) 2b74e15 refactor: move masternode mode logic to `governance/signing.cpp` (Kittywhiskers Van Gogh) fefbe27 refactor: replace `Sign` functions with direct activeman call (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6828 * Depends on #6842 * Depends on #6877 * Dependency for #6849 * To reduce the proliferation of `PeerManager` in governance logic, relaying has been centralized to `CGovernanceManager` and now done through a scheduled task that processes a queue of requests. This reduces the amount of code that needs to fetch a reference to `PeerManager` in order to queue a message for relay and brings us closer to dropping it as a member in other objects. * On test chains, the queue of requests is processed every second but on mainnet, it is processed every 5 seconds. This is because functional tests assume instantaneous relay and fail if it takes too long to relay messages (as cascading delays cause test timeout). * Likewise, to reduce the references to `CActiveMasternodeManager`, signing logic is consolidated to `SignBasic()` with the objects using simple getter/setters (or direct assignment) where applicable. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK b51cd1d knst: utACK b51cd1d Tree-SHA512: cd4bba1e7314695a215a8a83e2d5319be8d2eb2e88eb42f641f6c88ca6d74ee68ced9e95475c6b1d93f5b061993efa6a195d23f61a8775f920867d9836a0dcf8
kwvg
changed the title
GovernanceStore, CGovernanceManager and CGovernanceStore, move away from RecursiveMutexesGovernanceStore and CGovernanceManager, move away from RecursiveMutexes
kwvg
changed the title
GovernanceStore and CGovernanceManager, move away from RecursiveMutexesGovernanceStore and CGovernanceObject, move away from RecursiveMutexes
kwvg
changed the title
GovernanceStore and CGovernanceObject, move away from RecursiveMutexesCGovernanceObject and GovernanceStore, move away from RecursiveMutexes
kwvg
requested review from
PastaPastaPasta,
UdjinM6,
knst and
thephez
and removed request for
thephez
WalkthroughThis PR centralizes governance synchronization on a new Mutex Sequence Diagram(s)sequenceDiagram
autonumber
participant RPC as RPC / External Caller
participant Node as Node
participant GM as CGovernanceManager
participant Store as GovernanceStore\n(cs_store)
participant Obj as CGovernanceObject\n(obj->cs)
Note over RPC,Node: Request governance data or votes
RPC->>Node: request(hash)
Node->>GM: FindConstGovernanceObject(hash)
GM->>Store: LOCK(cs_store)
Store-->>GM: return std::shared_ptr<CGovernanceObject>
GM->>Store: UNLOCK(cs_store)
alt object found
GM->>Obj: WITH_LOCK(obj->cs)
Obj-->>GM: read protected fields/votes
end
GM-->>Node: return shared_ptr / null
Node-->>RPC: response
sequenceDiagram
autonumber
participant Caller as Internal Caller
participant GM as CGovernanceManager
participant Store as GovernanceStore\n(cs_store)
participant Obj as CGovernanceObject\n(obj->cs)
Note over Caller,GM: Ingest or postpone a governance object
Caller->>GM: AddPostponedObject(govobj)
GM->>GM: AddPostponedObjectInternal(govobj)
GM->>Store: LOCK(cs_store)
Store-->>GM: insert std::shared_ptr into maps
GM->>Store: UNLOCK(cs_store)
GM->>Obj: WITH_LOCK(obj->cs)
Obj-->>GM: update internal object state
GM-->>Caller: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus review on:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/governance/object.h (1)
95-132: Now searching for where these unguarded flags are read to understand the full extent of data races:Critical data race detected: unguarded cache flags modified in lock-free context.
GUARDED_BY declares data members protected by a capability, with read operations requiring shared access and write operations requiring exclusive access. The analysis warns whenever code reads or writes guarded data without first locking the mutex.
Severity: This is a data race vulnerability, not a style issue.
UpdateSentinelVariables()at line 603:
- Line 605:
AssertLockNotHeld(cs)— explicitly designed to run without the lock- Lines 619–637: Modifies all five unguarded cache flags (
fCachedFunding,fCachedValid,fCachedDelete,fCachedEndorsed,fDirtyCache) without synchronization- Line 628: TOCTOU race — reads
!fCachedDeleteunguarded, then conditionally modifies it (line 629), only acquiring lock afterwards (line 630) fornDeletionTimeRoot cause: Cache flags are plain
bool, notstd::atomic, and accessed without locks in a method explicitly markedAssertLockNotHeld(cs). This violates thread-safety discipline.Fix required:
- Add
GUARDED_BY(cs)to all cache flags- Update
UpdateSentinelVariables()to acquireLOCK(cs)before modifying cache state, or- Convert cache flags to
std::atomic<bool>if intentional lock-free semantics
🧹 Nitpick comments (1)
src/governance/governance.cpp (1)
304-308: Consider renamingunused_rcbfor clarity.The variable
unused_rcbindicates the rate-check-bypassed output parameter is intentionally unused. While the naming makes the intent clear, consider using[[maybe_unused]]attribute or a more descriptive name likefRateCheckBypassedIgnoredto be more explicit about why it's acceptable to ignore this output.- bool unused_rcb; - if (fRateCheckBypassed && fIsValid && !MasternodeRateCheck(govobj, true, true, unused_rcb)) { + bool fRateCheckBypassedIgnored; + if (fRateCheckBypassed && fIsValid && !MasternodeRateCheck(govobj, true, true, fRateCheckBypassedIgnored)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/governance/governance.cpp(57 hunks)src/governance/governance.h(6 hunks)src/governance/object.cpp(7 hunks)src/governance/object.h(5 hunks)src/rpc/governance.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/governance/object.cppsrc/rpc/governance.cppsrc/governance/governance.cppsrc/governance/object.hsrc/governance/governance.h
🧬 Code graph analysis (5)
src/governance/object.cpp (1)
src/governance/object.h (1)
cs(99-99)
src/rpc/governance.cpp (1)
src/node/interfaces.cpp (6)
LOCK(541-545)LOCK(551-558)LOCK(559-566)LOCK(828-837)LOCK(871-875)LOCK(1055-1059)
src/governance/governance.cpp (5)
src/node/interfaces.cpp (17)
LOCK(541-545)LOCK(551-558)LOCK(559-566)LOCK(828-837)LOCK(871-875)LOCK(1055-1059)Assert(93-93)Assert(94-94)Assert(118-118)Assert(259-259)Assert(282-282)Assert(404-404)Assert(613-616)Assert(825-825)Assert(1131-1134)vote(149-161)vote(149-149)src/net_processing.cpp (17)
LOCK(327-333)LOCK(340-344)LOCK(345-349)WITH_LOCK(335-338)WITH_LOCK(3172-3172)WITH_LOCK(3194-3194)ProcessMessage(3617-5411)ProcessMessage(3617-3622)peer(681-681)peer(814-814)peer(919-919)peer(920-920)peer(1027-1027)peer(1035-1035)ret(667-667)now(673-673)now(994-995)src/governance/object.cpp (3)
ss(235-235)ToJson(356-359)ToJson(356-356)src/governance/vote.cpp (3)
ss(109-109)ToString(95-103)ToString(95-95)src/coinjoin/server.cpp (2)
ProcessMessage(26-40)ProcessMessage(26-26)
src/governance/object.h (3)
src/governance/object.cpp (37)
CGovernanceObject(25-29)CGovernanceObject(31-38)CGovernanceObject(40-56)SetMasternodeOutpoint(246-249)SetMasternodeOutpoint(246-246)SetSignature(251-254)SetSignature(251-251)CheckSignature(256-265)CheckSignature(256-256)GetSignatureHash(241-244)GetSignatureHash(241-241)IsValidLocally(369-374)IsValidLocally(369-369)IsValidLocally(376-429)IsValidLocally(376-376)IsCollateralValid(447-535)IsCollateralValid(447-447)UpdateLocalValidity(361-366)UpdateLocalValidity(361-361)CountMatchingVotes(537-554)CountMatchingVotes(537-537)GetAbsoluteYesCount(560-564)GetAbsoluteYesCount(560-560)GetAbsoluteNoCount(566-570)GetAbsoluteNoCount(566-566)GetYesCount(572-576)GetYesCount(572-572)GetNoCount(578-582)GetNoCount(578-578)GetAbstainCount(584-588)GetAbstainCount(584-584)GetCurrentMNVotes(590-601)GetCurrentMNVotes(590-590)ClearMasternodeVotes(171-186)ClearMasternodeVotes(171-171)RemoveInvalidVotes(188-226)RemoveInvalidVotes(188-188)src/governance/common.cpp (1)
Object(14-23)src/governance/governance.cpp (2)
RemoveInvalidVotes(1451-1499)RemoveInvalidVotes(1451-1451)
src/governance/governance.h (1)
src/governance/governance.cpp (71)
GovernanceStore(56-65)Clear(1292-1301)Clear(1292-1292)Clear(1303-1309)Clear(1303-1303)ToString(1311-1337)ToString(1311-1311)ToString(1339-1343)ToString(1339-1339)LoadCache(124-134)LoadCache(124-124)ToJson(1345-1376)ToJson(1345-1345)CheckAndRemove(474-596)CheckAndRemove(474-474)Schedule(92-122)Schedule(92-92)AddInvalidVote(1103-1108)AddInvalidVote(1103-1103)GetCurrentVotes(639-678)GetCurrentVotes(639-639)GetAllNewerThan(680-694)GetAllNewerThan(680-680)ConfirmInventoryRequest(696-739)ConfirmInventoryRequest(696-696)ProcessVoteAndRelay(937-946)ProcessVoteAndRelay(937-937)RequestGovernanceObjectVotes(1110-1115)RequestGovernanceObjectVotes(1110-1110)RequestGovernanceObjectVotes(1117-1228)RequestGovernanceObjectVotes(1117-1118)ProcessMessage(226-371)ProcessMessage(226-227)RelayObject(136-146)RelayObject(136-136)RelayVote(148-164)RelayVote(148-148)UpdatedBlockTip(1378-1402)UpdatedBlockTip(1378-1378)AddGovernanceObject(465-472)AddGovernanceObject(465-465)InternalAddGovernanceObject(402-463)InternalAddGovernanceObject(402-402)MasternodeRateUpdate(841-863)MasternodeRateUpdate(841-841)CheckPostponedObjects(1003-1069)CheckPostponedObjects(1003-1003)InitOnLoad(1277-1290)InitOnLoad(1277-1277)AddNewTrigger(1505-1536)AddNewTrigger(1505-1505)CleanAndRemoveTriggers(1544-1605)CleanAndRemoveTriggers(1544-1544)ExecuteBestSuperblock(1793-1803)ExecuteBestSuperblock(1793-1793)RequestGovernanceObject(1071-1101)RequestGovernanceObject(1071-1071)ProcessVote(948-1001)ProcessVote(948-948)CheckOrphanVotes(373-400)CheckOrphanVotes(373-373)RebuildIndexes(1244-1256)RebuildIndexes(1244-1244)AddCachedTriggers(1258-1275)AddCachedTriggers(1258-1258)RequestOrphanObjects(1404-1430)RequestOrphanObjects(1404-1404)CleanOrphanObjects(1432-1449)CleanOrphanObjects(1432-1432)RemoveInvalidVotes(1451-1499)RemoveInvalidVotes(1451-1451)
🔇 Additional comments (14)
src/rpc/governance.cpp (2)
606-606: LGTM: Correct removal of governance manager lock.The change from
LOCK2(cs_main, govman.cs)toLOCK(cs_main)is correct. TheGetAllNewerThanmethod now handlescs_storelocking internally per itsEXCLUSIVE_LOCKS_REQUIRED(!cs_store)annotation (governance.h line 308-309).
731-731: LGTM: Correct removal of governance manager lock.The change from
LOCK2(cs_main, node.govman->cs)toLOCK(cs_main)is correct.FindConstGovernanceObjectand the vote counting methods now handle their own locking internally per their annotations in governance.h and object.h.src/governance/governance.h (3)
185-197: LGTM: Proper use of GUARDED_BY annotations.The conversion from
RecursiveMutextoMutexand the addition ofGUARDED_BY(cs_store)annotations on all data members is well-structured. Themutablequalifier oncs_storecorrectly allows locking in const methods.
204-234: LGTM: Correct serialization locking pattern.The
SerializeandUnserializemethods correctly require the lock NOT to be held (EXCLUSIVE_LOCKS_REQUIRED(!cs_store)) and then acquire it internally. This prevents recursive locking issues with the newMutex.
383-407: LGTM: Clean internal/public method split pattern.The introduction of
Internalmethod variants that requirecs_storeto be held (e.g.,InternalFindConstGovernanceObject,InternalAddGovernanceObject) paired with public wrappers that require the lock NOT to be held is a clean pattern that prevents recursive locking with the newMutex.src/governance/governance.cpp (4)
213-224: LGTM: Correct internal/external method split.The split of
AddPostponedObjectinto a public wrapper (lines 213-218) that lockscs_storeand an internal version (lines 220-224) that asserts the lock is held follows the correct pattern for avoiding recursive locking.
465-472: LGTM: Correct public wrapper implementation.The new public
AddGovernanceObjectmethod correctly asserts locks are not held, acquires them in the proper order (cs_mainbeforecs_store), and delegates toInternalAddGovernanceObject. This follows the established pattern for avoiding recursive locking.
598-613: LGTM: Correct const accessor split.The split of
FindConstGovernanceObjectinto a public wrapper that lockscs_storeand an internal const version that asserts the lock is held is correct. This maintains const-correctness while following the locking pattern.
771-784: LGTM: Correct nested locking for vote file access.The nested
LOCK(govobj.cs)(lines 771-784) whilecs_storeis already held maintains proper lock ordering (store-level lock before object-level lock) and correctly satisfiesGetVoteFile()'s requirement forEXCLUSIVE_LOCKS_REQUIRED(cs).src/governance/object.h (4)
145-156: LGTM: Correct accessor pattern for guarded members.
IsSetExpired()andGetDeletionTime()correctly useWITH_LOCK(cs, ...)to safely read guarded members, and properly annotate withEXCLUSIVE_LOCKS_REQUIRED(!cs)to prevent recursive locking.
158-162: LGTM: Correct accessor for guarded vote file.
GetVoteFile()correctly requires the caller to holdcs(viaEXCLUSIVE_LOCKS_REQUIRED(cs)) before returning a const reference to the guardedfileVotesmember. This prevents data races while allowing efficient access.
242-263: LGTM: Correct conditional locking in serialization.The
SerializeandUnserializemethods correctly:
- Require the lock NOT to be held initially (
EXCLUSIVE_LOCKS_REQUIRED(!cs))- Only lock
cswhen accessing guarded members for disk serialization (lines 248-249, 259-260)- Don't lock for network-only serialization of
m_objwhich is not guardedThis minimizes lock contention while maintaining thread-safety.
91-93: No changes needed—public mutex exposure is intentional and follows Dash Core architectural patterns.This design is correct. Dash Core classes like CTxMemPool document their public mutex as needing to be locked when accessing guarded members, with documented locking protocols for consistent snapshots. The
cspublic mutex pattern is standard across multiple core classes (CAddrMan, CGovernanceManager, CTxMemPool, etc.).The external locking at governance.cpp:772 and 1251 is necessary for atomic operations coordinating vote file access. The
mutablequalifier correctly allows locking within const methods. No refactoring is required.src/governance/object.cpp (1)
605-606: No action required - fCachedDelete is intentionally lock-free.The cached flags (
fCachedDelete,fCachedFunding,fCachedValid,fCachedEndorsed) are all plain bools withoutGUARDED_BYannotations, in contrast tonDeletionTimewhich explicitly hasGUARDED_BY(cs). This is a deliberate design where cached voting results are set lock-free, while only timing-critical values likenDeletionTimerequire synchronization. The pattern is consistent across all cached flags throughout the codebase, indicating this is intentional to avoid lock contention on frequently-read cached state.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/governance/object.h (1)
197-204: Header uses GetTime in an inline without including its declarationPrepareDeletion() calls GetTimestd::chrono::seconds() in this header, but <util/time.h> isn’t included here. Make the header self-contained to avoid ODR/compile surprises in TUs that include object.h but not util/time.h.
Apply this diff near the existing includes:
#include <governance/votedb.h> #include <sync.h> #include <span.h> #include <univalue.h> +#include <util/time.h>Also applies to: 8-17
src/governance/signing.cpp (1)
20-22: Fix compile issues: chrono literal and ranges alias
- 2h requires chrono literals; prefer hours to avoid a global using.
- ranges::any_of needs <util/ranges.h> (project alias) or std::ranges.
Apply:
#include <util/time.h> #include <validation.h> +#include <util/ranges.h> namespace { -constexpr std::chrono::seconds GOVERNANCE_FUDGE_WINDOW{2h}; +constexpr auto GOVERNANCE_FUDGE_WINDOW = std::chrono::hours{2}; } // anonymous namespaceNo behavior change; resolves build portability.
Also applies to: 185-197, 214-221
🧹 Nitpick comments (4)
src/governance/object.h (1)
1-3: Fix clang-format diff failures reported by CIRun the project’s clang-format diff script and commit the resulting changes for this file.
src/governance/governance.cpp (1)
1-10: Fix clang-format diff failures reported by CIRun the project’s clang-format diff script and commit the resulting changes for this file.
src/governance/governance.h (2)
1-10: Fix clang-format diff failures reported by CIRun the project’s clang-format diff script and commit the resulting changes for this file.
266-273: Remove unusedvotedFundingYesTriggerHashmember from CGovernanceManagerThe member at src/governance/governance.h:271 is only initialized and reset to
std::nulloptinClear()but is never read or used elsewhere in the class. This is dead code and should be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/governance/governance.cpp(59 hunks)src/governance/governance.h(7 hunks)src/governance/object.cpp(4 hunks)src/governance/object.h(5 hunks)src/governance/signing.cpp(1 hunks)src/governance/signing.h(1 hunks)src/rpc/governance.cpp(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/governance/object.cppsrc/governance/signing.cppsrc/governance/signing.hsrc/rpc/governance.cppsrc/governance/governance.hsrc/governance/governance.cppsrc/governance/object.h
🧠 Learnings (2)
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
PR: dashpay/dash#6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In PR #6849, `cmapVoteToObject` was intentionally moved from `GovernanceStore` to `CGovernanceManager` as a memory-only variable and is NOT guarded by `cs_store`. It relies on `CacheMap`'s internal thread-safety instead.
Applied to files:
src/governance/governance.hsrc/governance/governance.cpp
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
PR: dashpay/dash#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/governance/governance.cpp
🧬 Code graph analysis (5)
src/governance/object.cpp (2)
src/governance/object.h (1)
cs(99-99)src/masternode/node.h (1)
cs(45-80)
src/governance/signing.h (1)
src/governance/governance.cpp (4)
FindGovernanceObject(595-600)FindGovernanceObject(595-595)FindGovernanceObjectByDataHash(609-617)FindGovernanceObjectByDataHash(609-609)
src/governance/governance.h (1)
src/governance/governance.cpp (89)
GovernanceStore(56-65)Clear(1260-1269)Clear(1260-1260)Clear(1271-1285)Clear(1271-1271)ToString(1287-1312)ToString(1287-1287)ToString(1314-1318)ToString(1314-1314)LoadCache(120-130)LoadCache(120-120)ToJson(1320-1350)ToJson(1320-1320)CheckAndRemove(462-577)CheckAndRemove(462-462)Schedule(88-118)Schedule(88-88)AddInvalidVote(1077-1081)AddInvalidVote(1077-1077)GetCurrentVotes(619-657)GetCurrentVotes(619-619)GetAllNewerThan(659-672)GetAllNewerThan(659-659)ConfirmInventoryRequest(674-717)ConfirmInventoryRequest(674-674)ProcessVoteAndRelay(911-920)ProcessVoteAndRelay(911-911)ProcessMessage(216-361)ProcessMessage(216-217)RelayObject(132-142)RelayObject(132-132)RelayVote(144-160)RelayVote(144-144)UpdatedBlockTip(1352-1376)UpdatedBlockTip(1352-1352)MasternodeRateCheck(840-845)MasternodeRateCheck(840-840)MasternodeRateCheck(847-909)MasternodeRateCheck(847-847)FindGovernanceObjectByDataHash(609-617)FindGovernanceObjectByDataHash(609-609)GetApprovedProposals(1774-1805)GetApprovedProposals(1774-1775)AddGovernanceObject(456-460)AddGovernanceObject(456-456)FindGovernanceObject(595-600)FindGovernanceObject(595-595)AddPostponedObject(204-208)AddPostponedObject(204-204)FindConstGovernanceObject(579-583)FindConstGovernanceObject(579-579)SyncObjects(772-814)SyncObjects(772-772)SyncSingleObjVotes(719-770)SyncSingleObjVotes(719-719)AddPostponedObjectInternal(210-214)AddPostponedObjectInternal(210-210)FindConstGovernanceObjectInternal(585-593)FindConstGovernanceObjectInternal(585-585)AddGovernanceObjectInternal(392-454)AddGovernanceObjectInternal(392-392)MasternodeRateUpdate(816-838)MasternodeRateUpdate(816-816)CheckPostponedObjects(977-1043)CheckPostponedObjects(977-977)InitOnLoad(1246-1258)InitOnLoad(1246-1246)AddNewTrigger(1477-1508)AddNewTrigger(1477-1477)CleanAndRemoveTriggers(1516-1576)CleanAndRemoveTriggers(1516-1516)ExecuteBestSuperblock(1762-1772)ExecuteBestSuperblock(1762-1762)RequestGovernanceObject(1045-1075)RequestGovernanceObject(1045-1045)ProcessVote(922-975)ProcessVote(922-922)AcceptMessage(1201-1213)AcceptMessage(1201-1201)CheckOrphanVotes(363-390)CheckOrphanVotes(363-363)RebuildIndexes(1215-1227)RebuildIndexes(1215-1215)RequestOrphanObjects(1378-1404)RequestOrphanObjects(1378-1378)CleanOrphanObjects(1406-1421)CleanOrphanObjects(1406-1406)RemoveInvalidVotes(1423-1471)RemoveInvalidVotes(1423-1423)
src/governance/governance.cpp (2)
src/governance/object.cpp (3)
ss(231-231)RemoveInvalidVotes(185-222)RemoveInvalidVotes(185-185)src/governance/governance.h (2)
nCachedBlockHeight(265-265)nTimeLastDiff(263-263)
src/governance/object.h (4)
src/governance/object.cpp (37)
CGovernanceObject(25-29)CGovernanceObject(31-38)CGovernanceObject(40-56)SetMasternodeOutpoint(242-245)SetMasternodeOutpoint(242-242)SetSignature(247-250)SetSignature(247-247)CheckSignature(252-261)CheckSignature(252-252)GetSignatureHash(237-240)GetSignatureHash(237-237)IsValidLocally(365-370)IsValidLocally(365-365)IsValidLocally(372-425)IsValidLocally(372-372)IsCollateralValid(443-531)IsCollateralValid(443-443)UpdateSentinelVariables(596-631)UpdateSentinelVariables(596-596)CountMatchingVotes(533-548)CountMatchingVotes(533-533)GetAbsoluteYesCount(554-558)GetAbsoluteYesCount(554-554)GetAbsoluteNoCount(560-564)GetAbsoluteNoCount(560-560)GetYesCount(566-570)GetYesCount(566-566)GetNoCount(572-576)GetNoCount(572-572)GetAbstainCount(578-582)GetAbstainCount(578-578)GetCurrentMNVotes(584-594)GetCurrentMNVotes(584-584)ClearMasternodeVotes(169-183)ClearMasternodeVotes(169-169)RemoveInvalidVotes(185-222)RemoveInvalidVotes(185-185)src/governance/common.cpp (1)
Object(14-23)src/governance/vote.cpp (4)
CheckSignature(124-142)CheckSignature(124-124)CheckSignature(144-153)CheckSignature(144-144)src/governance/governance.cpp (2)
RemoveInvalidVotes(1423-1471)RemoveInvalidVotes(1423-1423)
🪛 GitHub Actions: Clang Diff Format Check
src/governance/governance.h
[error] 1-1: Clang format differences detected in governance.h compared to origin/develop. Run the project's clang-format diff script or apply formatting changes.
src/governance/governance.cpp
[error] 1-1: Clang format differences detected in governance.cpp compared to origin/develop. Run the project's clang-format diff script or apply formatting changes.
src/governance/object.h
[error] 1-1: Clang format differences detected in object.h compared to origin/develop. Run the project's clang-format diff script or apply formatting changes.
⏰ 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_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (6)
src/governance/object.cpp (2)
533-548: LGTM: internal locking and structured bindingsCountMatchingVotes() locks object.cs internally and uses structured bindings cleanly. Lock order remains consistent with manager code (cs_store → object.cs).
596-631: LGTM: negative-capability + scoped lock for deletion timeAssertLockNotHeld(cs) plus a scoped LOCK(cs) for nDeletionTime is correct and prevents reentrancy issues.
src/rpc/governance.cpp (1)
603-641: LGTM: narrower lock scope and shared_ptr retrievalSwitching to FindConstGovernanceObject() with internal cs_store locking and holding only cs_main here reduces contention without sacrificing safety.
If you want a quick audit of remaining raw-pointer callsites, I can script a scan for FindGovernanceObject/FindConstGovernanceObject usage across RPC.
Also applies to: 723-731, 827-839
src/governance/signing.h (1)
40-46: LGTM: return std::shared_ptr from find APIsReturning shared_ptr eliminates UAF risks and matches manager’s storage type. Callers in signing.cpp were updated accordingly.
src/governance/governance.cpp (2)
392-458: LGTM: shared_ptr ownership + consistent lock order
- Using shared_ptr in mapObjects/cmapVoteToObject prevents dangling references.
- cs_store → object.cs acquisition order is consistent throughout (WITH_LOCK where needed).
- Clear() now resets postponed/relay/requested state as expected.
Also applies to: 476-577, 1271-1285
1314-1318: LGTM: ToString() compositionComposing GovernanceStore::ToString() (internally locks cs_store) with CacheMap::GetSize() (internally synchronized) is safe and cheap. Matches the intended thread-safety model.
`votedFundingYesTriggerHash` was moved to GovernanceSigner but the old definition still lingered around, get rid of it.
`cmapVoteToObject` isn't (de)ser'ed so we move it to the manager and don't consider it for the mutex
Review with `git log -p -n1 --color-moved=dimmed_zebra`.
This will simplify our work in subsequent commits. Review with `git log -p -n1 --color-moved=dimmed_zebra`.
Needed to make the next commit easier to follow.
Currently, the LOCKs aren't guarding anything. Variables protected were based on existing LOCK'ing patterns (i.e. (de)ser'ed variables except for m_obj).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/governance/object.cpp (1)
141-149: Critical: self-deadlock on cs_store via AddInvalidVote call pathCGovernanceObject::ProcessVote() calls govman.AddInvalidVote(vote) inside the “invalid vote” branch while CGovernanceManager::ProcessVote() holds cs_store. Since cs_store is now a non-recursive Mutex, this re-locks the same mutex in the same thread and can deadlock at runtime.
Fix by recording invalid votes in CGovernanceManager::ProcessVote() after ProcessVote() returns false (under cs_store), and remove the AddInvalidVote() call here.
Apply this change (part 1/2):
- if (!vote.IsValid(tip_mn_list, onlyVotingKeyAllowed)) { + if (!vote.IsValid(tip_mn_list, onlyVotingKeyAllowed)) { std::string msg{strprintf("CGovernanceObject::%s -- Invalid vote, MN outpoint = %s, governance object hash = %s, " "vote hash = %s", __func__, vote.GetMasternodeOutpoint().ToStringShort(), GetHash().ToString(), vote.GetHash().ToString())}; LogPrintf("%s\n", msg); exception = CGovernanceException(msg, GOVERNANCE_EXCEPTION_PERMANENT_ERROR, 20); - govman.AddInvalidVote(vote); return false; }src/governance/governance.cpp (1)
922-975: Critical: avoid re-locking cs_store inside CGovernanceObject::ProcessVote pathcs_store is held across govobj.ProcessVote(...). On invalid votes, the object-level code currently attempts govman.AddInvalidVote(vote), which locks cs_store again and deadlocks (Mutex is non-recursive). Move invalid-vote recording to the manager, immediately after ProcessVote returns.
Apply this change (part 2/2):
- bool fOk = govobj.ProcessVote(m_mn_metaman, *this, Assert(m_dmnman)->GetListAtChainTip(), vote, exception) && cmapVoteToObject.Insert(nHashVote, it->second); - LEAVE_CRITICAL_SECTION(cs_store); - return fOk; + bool fValid = govobj.ProcessVote(m_mn_metaman, *this, Assert(m_dmnman)->GetListAtChainTip(), vote, exception); + if (fValid) { + (void)cmapVoteToObject.Insert(nHashVote, it->second); + } else if (exception.GetNodePenalty() == 20) { + // Permanent invalid vote — record under cs_store + cmapInvalidVotes.Insert(nHashVote, vote); + } + LEAVE_CRITICAL_SECTION(cs_store); + return fValid;
🧹 Nitpick comments (3)
src/governance/object.cpp (1)
596-627: Guard cached flags to avoid data races (optional)fCachedFunding/fCachedValid/fCachedDelete/fCachedEndorsed/fDirtyCache are read/written across threads without a guard. Consider guarding them with cs (and annotating with GUARDED_BY(cs)) or switching them to std::atomic to avoid data races. As a minimal tweak, set fCachedDelete under cs together with nDeletionTime for coherence.
Example tweak:
- if ((GetAbsoluteYesCount(tip_mn_list, VOTE_SIGNAL_DELETE) >= nAbsDeleteReq) && !fCachedDelete) { - fCachedDelete = true; - LOCK(cs); + if ((GetAbsoluteYesCount(tip_mn_list, VOTE_SIGNAL_DELETE) >= nAbsDeleteReq)) { + LOCK(cs); + fCachedDelete = true; if (nDeletionTime == 0) { nDeletionTime = GetTime<std::chrono::seconds>().count(); } }src/governance/object.h (2)
91-100: Consider guarding cached flags with cs to avoid data racesnDeletionTime and fExpired are GUARDED_BY(cs), but fCachedFunding/fCachedValid/fCachedDelete/fCachedEndorsed/fDirtyCache remain unguarded and are mutated from multiple threads. Recommend either:
- Guard them under cs and add GUARDED_BY(cs), or
- Make them std::atomic with comments explaining relaxed semantics.
This will also satisfy thread-safety analyzers and future TSAN runs.
Also applies to: 124-133
167-175: PrepareDeletion: set fCachedDelete under cs for coherenceCurrently fCachedDelete is written outside the lock while nDeletionTime is guarded. Move fCachedDelete under the same cs critical section and annotate it accordingly to avoid torn reads.
Also applies to: 194-205
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/governance/governance.cpp(59 hunks)src/governance/governance.h(7 hunks)src/governance/object.cpp(4 hunks)src/governance/object.h(5 hunks)src/governance/signing.cpp(1 hunks)src/governance/signing.h(1 hunks)src/rpc/governance.cpp(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/governance/signing.h
- src/governance/signing.cpp
- src/rpc/governance.cpp
🧰 Additional context used
📓 Path-based instructions (1)
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/governance/object.cppsrc/governance/governance.cppsrc/governance/governance.hsrc/governance/object.h
🧠 Learnings (1)
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
PR: dashpay/dash#6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In PR #6849, `cmapVoteToObject` was intentionally moved from `GovernanceStore` to `CGovernanceManager` as a memory-only variable and is NOT guarded by `cs_store`. It relies on `CacheMap`'s internal thread-safety instead.
Applied to files:
src/governance/governance.cppsrc/governance/governance.h
🧬 Code graph analysis (4)
src/governance/object.cpp (1)
src/governance/object.h (1)
cs(99-99)
src/governance/governance.cpp (2)
src/governance/object.cpp (2)
RemoveInvalidVotes(185-222)RemoveInvalidVotes(185-185)src/governance/governance.h (2)
nCachedBlockHeight(265-265)nTimeLastDiff(263-263)
src/governance/governance.h (1)
src/governance/governance.cpp (71)
GovernanceStore(56-65)Clear(1260-1269)Clear(1260-1260)Clear(1271-1284)Clear(1271-1271)ToString(1286-1311)ToString(1286-1286)ToString(1313-1317)ToString(1313-1313)LoadCache(120-130)LoadCache(120-120)ToJson(1319-1349)ToJson(1319-1319)CheckAndRemove(462-577)CheckAndRemove(462-462)Schedule(88-118)Schedule(88-88)AddInvalidVote(1077-1081)AddInvalidVote(1077-1077)ProcessVoteAndRelay(911-920)ProcessVoteAndRelay(911-911)RequestGovernanceObjectVotes(1083-1087)RequestGovernanceObjectVotes(1083-1083)RequestGovernanceObjectVotes(1089-1199)RequestGovernanceObjectVotes(1089-1090)ProcessMessage(216-361)ProcessMessage(216-217)RelayObject(132-142)RelayObject(132-132)RelayVote(144-160)RelayVote(144-144)MasternodeRateCheck(840-845)MasternodeRateCheck(840-840)MasternodeRateCheck(847-909)MasternodeRateCheck(847-847)AddGovernanceObject(456-460)AddGovernanceObject(456-456)FindGovernanceObject(595-600)FindGovernanceObject(595-595)AddPostponedObject(204-208)AddPostponedObject(204-204)FindConstGovernanceObject(579-583)FindConstGovernanceObject(579-579)SyncObjects(772-814)SyncObjects(772-772)SyncSingleObjVotes(719-770)SyncSingleObjVotes(719-719)AddPostponedObjectInternal(210-214)AddPostponedObjectInternal(210-210)FindGovernanceObjectInternal(602-607)FindGovernanceObjectInternal(602-602)FindConstGovernanceObjectInternal(585-593)FindConstGovernanceObjectInternal(585-585)AddGovernanceObjectInternal(392-454)AddGovernanceObjectInternal(392-392)InitOnLoad(1246-1258)InitOnLoad(1246-1246)CleanAndRemoveTriggers(1515-1575)CleanAndRemoveTriggers(1515-1515)RequestGovernanceObject(1045-1075)RequestGovernanceObject(1045-1045)ProcessVote(922-975)ProcessVote(922-922)AcceptMessage(1201-1213)AcceptMessage(1201-1201)RebuildIndexes(1215-1227)RebuildIndexes(1215-1215)RequestOrphanObjects(1377-1403)RequestOrphanObjects(1377-1377)CleanOrphanObjects(1405-1420)CleanOrphanObjects(1405-1405)
src/governance/object.h (3)
src/governance/object.cpp (39)
CGovernanceObject(25-29)CGovernanceObject(31-38)CGovernanceObject(40-56)SetMasternodeOutpoint(242-245)SetMasternodeOutpoint(242-242)SetSignature(247-250)SetSignature(247-247)CheckSignature(252-261)CheckSignature(252-252)GetSignatureHash(237-240)GetSignatureHash(237-237)IsValidLocally(365-370)IsValidLocally(365-365)IsValidLocally(372-425)IsValidLocally(372-372)IsCollateralValid(443-531)IsCollateralValid(443-443)UpdateLocalValidity(357-362)UpdateLocalValidity(357-357)UpdateSentinelVariables(596-631)UpdateSentinelVariables(596-596)CountMatchingVotes(533-548)CountMatchingVotes(533-533)GetAbsoluteYesCount(554-558)GetAbsoluteYesCount(554-554)GetAbsoluteNoCount(560-564)GetAbsoluteNoCount(560-560)GetYesCount(566-570)GetYesCount(566-566)GetNoCount(572-576)GetNoCount(572-572)GetAbstainCount(578-582)GetAbstainCount(578-578)GetCurrentMNVotes(584-594)GetCurrentMNVotes(584-584)ClearMasternodeVotes(169-183)ClearMasternodeVotes(169-169)RemoveInvalidVotes(185-222)RemoveInvalidVotes(185-185)src/governance/common.cpp (1)
Object(14-23)src/governance/governance.cpp (2)
RemoveInvalidVotes(1422-1470)RemoveInvalidVotes(1422-1422)
🪛 GitHub Actions: Clang Diff Format Check
src/governance/governance.cpp
[error] 1-1: Clang format differences detected in governance.cpp. Run clang-format-diff.py -p1 to fix formatting.
src/governance/governance.h
[error] 1-1: Clang format differences detected in governance.h. Run clang-format-diff.py -p1 to fix formatting.
⏰ 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_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (11)
src/governance/object.cpp (2)
538-545: Structured bindings + outpoint use LGTMThe iteration and collateral lookup are clearer and correct; vote weight calculation remains unchanged.
554-582: Good: negative-capability asserts on read-only countersAssertLockNotHeld(cs) before calling into CountMatchingVotes (which locks cs internally) enforces the intended locking protocol. Please keep call sites free of cs to avoid TSAN false-positives.
src/governance/governance.cpp (3)
44-53: ScopedLockBool guard pattern LGTMAsserts and rollback semantics are correct; maintains cs_store invariant while temporarily toggling flags.
1288-1311: Clang-format: fix diff formatting in this region (and file-wide)CI reports clang-format differences for governance.cpp. Please run clang-format-diff.py -p1 (or git clang-format) on this file.
1776-1804: Approved: shared_ptr + stable sort criteriaRead-only ownership model and tie-breaker by hash look solid; aligns with new locking contracts.
src/governance/object.h (1)
158-163: Accessor contract is clearGetVoteFile() requiring cs and asserting it is held looks correct and matches call sites.
src/governance/governance.h (5)
182-205: Serialize/Unserialize negative capability pattern LGTMEXCLUSIVE_LOCKS_REQUIRED(!cs_store) with internal LOCK(cs_store) is consistent and prevents accidental re-entrance.
Also applies to: 235-240
263-271: Lock annotations and shared_ptr-based maps look consistentcmapVoteToObject as CacheMap<..., shared_ptr<...>> plus mapObjects/mapPostponedObjects guarded by cs_store align with lifetime safety goals.
286-296: Scheduler annotations are correctExplicit negative capabilities for cs_store and cs_relay help avoid accidental deadlocks in scheduled lambdas.
372-381: Const find returns shared_ptr — goodPrevents UAF while keeping call sites simple.
1-1: Clang-format: governance.h also flagged by CIPlease run clang-format-diff.py -p1 on this header as reported by CI.
| bool CGovernanceManager::HaveVoteForHash(const uint256& nHash) const | ||
| { | ||
| LOCK(cs); | ||
| LOCK(cs_store); | ||
|
|
||
| CGovernanceObject* pGovobj = nullptr; | ||
| return cmapVoteToObject.Get(nHash, pGovobj) && pGovobj->GetVoteFile().HasVote(nHash); | ||
| std::shared_ptr<CGovernanceObject> pGovobj{nullptr}; | ||
| return cmapVoteToObject.Get(nHash, pGovobj) && WITH_LOCK(pGovobj->cs, return pGovobj->GetVoteFile().HasVote(nHash)); | ||
| } |
There was a problem hiding this comment.
Null-check/Assert on shared_ptr from cache (minor)
If cmapVoteToObject.Get returns true, pGovobj should be non-null. Add Assert(pGovobj) before WITH_LOCK to catch anomalies:
- return cmapVoteToObject.Get(nHash, pGovobj) && WITH_LOCK(pGovobj->cs, return pGovobj->GetVoteFile().HasVote(nHash));
+ return cmapVoteToObject.Get(nHash, pGovobj) &&
+ Assert(pGovobj) && WITH_LOCK(pGovobj->cs, return pGovobj->GetVoteFile().HasVote(nHash));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/governance/governance.cpp around lines 182 to 188, the code assumes
pGovobj is non-null after cmapVoteToObject.Get returns true; add an explicit
Assert(pGovobj) immediately before the WITH_LOCK call to catch anomalies and
document the invariant, so the function becomes: after obtaining pGovobj from
the cache and before entering WITH_LOCK, call Assert(pGovobj) (or the project's
assert macro) to ensure pGovobj is valid; no other behavior changes are
required.
|
@kwvg pls check |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/governance/signing.cpp (1)
217-217: Keep const qualifier on local binding for clarity.The local
govobjbinding is only used for read-only operations (null-check,GetHash(),GetBlockHeight()). Retainingconst autodocuments the intent and prevents accidental modification.Apply this diff:
- auto govobj = m_govman.FindGovernanceObject(trigger->GetGovernanceObjHash()); + const auto govobj = m_govman.FindGovernanceObject(trigger->GetGovernanceObjHash());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/governance/governance.cpp(58 hunks)src/governance/governance.h(7 hunks)src/governance/object.cpp(4 hunks)src/governance/object.h(5 hunks)src/governance/signing.cpp(1 hunks)src/governance/signing.h(1 hunks)src/rpc/governance.cpp(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/governance/object.cpp
- src/governance/signing.h
🧰 Additional context used
📓 Path-based instructions (1)
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/governance/signing.cppsrc/rpc/governance.cppsrc/governance/governance.hsrc/governance/governance.cppsrc/governance/object.h
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In PR #6849, `cmapVoteToObject` was intentionally moved from `GovernanceStore` to `CGovernanceManager` as a memory-only variable and is NOT guarded by `cs_store`. It relies on `CacheMap`'s internal thread-safety instead.
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
PR: dashpay/dash#6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In PR #6849, `cmapVoteToObject` was intentionally moved from `GovernanceStore` to `CGovernanceManager` as a memory-only variable and is NOT guarded by `cs_store`. It relies on `CacheMap`'s internal thread-safety instead.
Applied to files:
src/governance/governance.hsrc/governance/governance.cpp
🧬 Code graph analysis (3)
src/governance/governance.h (1)
src/governance/governance.cpp (103)
GovernanceStore(56-65)Clear(1259-1268)Clear(1259-1259)Clear(1270-1283)Clear(1270-1270)ToString(1285-1310)ToString(1285-1285)ToString(1312-1316)ToString(1312-1312)LoadCache(120-130)LoadCache(120-120)ToJson(1318-1348)ToJson(1318-1318)CheckAndRemove(462-577)CheckAndRemove(462-462)Schedule(88-118)Schedule(88-88)GetCurrentVotes(619-657)GetCurrentVotes(619-619)GetAllNewerThan(659-672)GetAllNewerThan(659-659)ConfirmInventoryRequest(674-717)ConfirmInventoryRequest(674-674)ProcessVoteAndRelay(911-920)ProcessVoteAndRelay(911-911)RequestGovernanceObjectVotes(1082-1086)RequestGovernanceObjectVotes(1082-1082)RequestGovernanceObjectVotes(1088-1198)RequestGovernanceObjectVotes(1088-1089)ProcessMessage(216-361)ProcessMessage(216-217)RelayObject(132-142)RelayObject(132-132)RelayVote(144-160)RelayVote(144-144)UpdatedBlockTip(1350-1374)UpdatedBlockTip(1350-1350)MasternodeRateCheck(840-845)MasternodeRateCheck(840-840)MasternodeRateCheck(847-909)MasternodeRateCheck(847-847)AddGovernanceObject(456-460)AddGovernanceObject(456-456)GetSuperblockPayments(1699-1744)GetSuperblockPayments(1699-1700)IsSuperblockTriggered(1605-1656)IsSuperblockTriggered(1605-1605)IsValidSuperblock(1746-1758)IsValidSuperblock(1746-1747)GetBestSuperblock(1658-1664)GetBestSuperblock(1658-1659)FindGovernanceObject(595-600)FindGovernanceObject(595-595)GetActiveTriggers(1582-1587)GetActiveTriggers(1582-1582)AddPostponedObject(204-208)AddPostponedObject(204-204)FindConstGovernanceObject(579-583)FindConstGovernanceObject(579-579)SyncObjects(772-814)SyncObjects(772-772)SyncSingleObjVotes(719-770)SyncSingleObjVotes(719-719)AddPostponedObjectInternal(210-214)AddPostponedObjectInternal(210-210)GetBestSuperblockInternal(1666-1697)GetBestSuperblockInternal(1666-1667)FindGovernanceObjectInternal(602-607)FindGovernanceObjectInternal(602-602)GetActiveTriggersInternal(1589-1603)GetActiveTriggersInternal(1589-1589)FindConstGovernanceObjectInternal(585-593)FindConstGovernanceObjectInternal(585-585)AddGovernanceObjectInternal(392-454)AddGovernanceObjectInternal(392-392)MasternodeRateUpdate(816-838)MasternodeRateUpdate(816-816)CheckPostponedObjects(982-1048)CheckPostponedObjects(982-982)InitOnLoad(1245-1257)InitOnLoad(1245-1245)AddNewTrigger(1475-1506)AddNewTrigger(1475-1475)CleanAndRemoveTriggers(1514-1574)CleanAndRemoveTriggers(1514-1514)ExecuteBestSuperblock(1760-1770)ExecuteBestSuperblock(1760-1760)RequestGovernanceObject(1050-1080)RequestGovernanceObject(1050-1050)ProcessVote(922-980)ProcessVote(922-922)CheckOrphanVotes(363-390)CheckOrphanVotes(363-363)RebuildIndexes(1214-1226)RebuildIndexes(1214-1214)AddCachedTriggers(1228-1243)AddCachedTriggers(1228-1228)RequestOrphanObjects(1376-1402)RequestOrphanObjects(1376-1376)CleanOrphanObjects(1404-1419)CleanOrphanObjects(1404-1404)RemoveInvalidVotes(1421-1469)RemoveInvalidVotes(1421-1421)
src/governance/governance.cpp (2)
src/governance/object.cpp (2)
RemoveInvalidVotes(184-221)RemoveInvalidVotes(184-184)src/governance/governance.h (2)
nCachedBlockHeight(265-265)nTimeLastDiff(263-263)
src/governance/object.h (2)
src/governance/object.cpp (39)
CGovernanceObject(25-29)CGovernanceObject(31-38)CGovernanceObject(40-56)SetMasternodeOutpoint(241-244)SetMasternodeOutpoint(241-241)SetSignature(246-249)SetSignature(246-246)CheckSignature(251-260)CheckSignature(251-251)GetSignatureHash(236-239)GetSignatureHash(236-236)IsValidLocally(364-369)IsValidLocally(364-364)IsValidLocally(371-424)IsValidLocally(371-371)IsCollateralValid(442-530)IsCollateralValid(442-442)UpdateLocalValidity(356-361)UpdateLocalValidity(356-356)UpdateSentinelVariables(595-630)UpdateSentinelVariables(595-595)CountMatchingVotes(532-547)CountMatchingVotes(532-532)GetAbsoluteYesCount(553-557)GetAbsoluteYesCount(553-553)GetAbsoluteNoCount(559-563)GetAbsoluteNoCount(559-559)GetYesCount(565-569)GetYesCount(565-565)GetNoCount(571-575)GetNoCount(571-571)GetAbstainCount(577-581)GetAbstainCount(577-577)GetCurrentMNVotes(583-593)GetCurrentMNVotes(583-583)ClearMasternodeVotes(168-182)ClearMasternodeVotes(168-168)RemoveInvalidVotes(184-221)RemoveInvalidVotes(184-184)src/governance/common.cpp (1)
Object(14-23)
🪛 GitHub Actions: Clang Diff Format Check
src/governance/governance.h
[error] 1-1: Clang format differences found between before and after formatting. See diff_output.txt.
src/governance/governance.cpp
[error] 1-1: Clang format differences found between before and after formatting. See diff_output.txt.
src/governance/object.h
[error] 1-1: Clang format differences found between before and after formatting. See diff_output.txt.
⏰ 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_tsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (46)
src/rpc/governance.cpp (6)
417-421: LGTM! Per-object locking correctly removed.The per-object lock is no longer needed because
FindConstGovernanceObjectnow returns astd::shared_ptr<const CGovernanceObject>, which provides safe lifetime management without requiring external locking during the lookup.
430-430: LGTM! Structured binding modernizes the code.The structured binding
[proTxHash, keyID]improves readability and is idiomatic C++17.
728-729: LGTM! Per-object locking correctly removed.The transition to
FindConstGovernanceObjectreturningshared_ptreliminates the need for holdinggovman.csduring the lookup. The shared pointer ensures the object remains valid for the duration of use.
827-827: LGTM! Consistent with the refactoring pattern.The removal of per-object locking and direct use of
FindConstGovernanceObjectaligns with the broader shared_ptr-based ownership model introduced in this PR.
922-922: LGTM! Lambda captures govobj safely via shared_ptr.The lambda captures the
shared_ptrreturned byFindConstGovernanceObject, ensuring the object remains valid throughout the lambda's execution even aftercs_storeis released.
603-603: Locking contract verified and correct.The
GetAllNewerThanfunction properly acquirescs_storeinternally viaLOCK(cs_store);at function entry and guards themapObjectsiteration. The change fromLOCK2(cs_main, govman.cs)toLOCK(cs_main)at line 603 is safe and correct because the locking responsibility has been properly delegated to the function itself.src/governance/governance.cpp (16)
44-50: LGTM! ScopedLockBool correctly updated for Mutex.The change from
RecursiveMutex&toMutex&aligns with the broader migration fromRecursiveMutextoMutexforcs_store. TheAssertLockHeld(_cs)at line 47 ensures the lock is held when the guard is constructed.
56-65: LGTM! GovernanceStore initialization follows new locking model.The initialization of
cs_store()(line 57) and the guarded members (mapObjects,mapErasedGovernanceObjects, etc.) is correct. All members that areGUARDED_BY(cs_store)in the header are properly initialized here.
76-78: LGTM! cmapVoteToObject correctly NOT guarded by cs_store.As documented in the retrieved learnings,
cmapVoteToObjectwas intentionally moved fromGovernanceStoretoCGovernanceManageras a memory-only variable and relies onCacheMap's internal thread-safety. This is correct per the PR objectives.Based on learnings.
90-92: LGTM! Proper lock assertions for Schedule.The
AssertLockNotHeld(cs_store)andAssertLockNotHeld(cs_relay)annotations match the header declaration (line 294-295 in governance.h) and prevent potential deadlocks when scheduling periodic tasks.
182-188: LGTM! HaveVoteForHash uses shared_ptr safely.The function locks
cs_store, retrieves ashared_ptr<CGovernanceObject>viacmapVoteToObject.Get, and then accesses the vote file underpGovobj->cs. The shared_ptr ensures the object remains valid during theWITH_LOCKcall.
196-202: LGTM! SerializeVoteForHash follows the same safe pattern.The locking discipline is consistent:
LOCK(cs_store)followed byshared_ptrretrieval and per-object locking viaWITH_LOCK(pGovobj->cs, ...).
204-214: LGTM! Clean separation of public and internal APIs.The public
AddPostponedObject(lines 204-208) acquirescs_storeand delegates toAddPostponedObjectInternal(lines 210-214), which asserts the lock is held. This pattern is used consistently throughout the refactor and prevents accidental lock-ordering issues.
294-298: LGTM! Unused variable is necessary for API contract.The
unused_rcbvariable (line 294-295) is required by theMasternodeRateChecksignature (line 295) even though the returned value isn't used. This is acceptable and clear from the naming.
363-390: LGTM! CheckOrphanVotes correctly locks and uses shared_ptr.The function properly:
- Asserts
cs_storeis held (line 365)- Uses
ScopedLockBoolto temporarily disable rate checks (line 372)- Employs structured binding for readability (line 377)
- Passes votes to
govobj.ProcessVoteunder proper locking
392-454: LGTM! AddGovernanceObjectInternal correctly uses shared_ptr and locking.The implementation properly:
- Asserts both
::cs_mainandcs_storeare held (lines 394-395)- Uses
std::make_shared<CGovernanceObject>to create shared ownership (line 421)- Uses structured binding to decompose the emplace result (line 430)
- Relays the object via the shared_ptr (line 441)
- Notifies external systems with a const shared_ptr wrapper (line 453)
476-573: LGTM! CheckAndRemove correctly manages object lifecycle with shared_ptr.The function properly:
- Acquires
LOCK2(::cs_main, cs_store)at line 477- Uses structured bindings for iteration (line 494)
- Iterates over
cmapVoteToObject.GetItemList()to remove dangling vote references (lines 519-528)- Compares shared_ptr values correctly at line 521
- Erases objects from
mapObjectsafter processing (line 543)
579-617: LGTM! Find accessor patterns correctly separate public and internal APIs.*The accessor implementations follow a consistent pattern:
- Public
FindConstGovernanceObject(lines 579-583): acquirescs_storeand delegates- Internal
FindConstGovernanceObjectInternal(lines 585-593): asserts lock held, performs lookup- Similar pattern for
FindGovernanceObject(lines 595-607) andFindGovernanceObjectByDataHash(lines 609-617)This separation ensures proper locking discipline and prevents lock-ordering issues.
1259-1268: LGTM! GovernanceStore::Clear correctly resets all guarded state.The function properly locks
cs_storeand clears all guarded members, including the reset oflastMNListForVotingKeysto a freshCDeterministicMNListinstance (line 1267).
1270-1283: LGTM! CGovernanceManager::Clear extends base class correctly.The function calls
GovernanceStore::Clear()(which lockscs_storeinternally) and then resets allCGovernanceManager-specific state includingcmapVoteToObject,mapPostponedObjects,setAdditionalRelayObjects, andm_requested_hash_time. This addresses the concern raised in the past review comments about leaving postponed/orphan state behind.
1285-1316: LGTM! ToString implementations correctly use cs_store.Both
GovernanceStore::ToString()(lines 1285-1310) andCGovernanceManager::ToString()(lines 1312-1316) properly lockcs_storewhen accessing guarded members. The concern aboutcmapVoteToObject.GetSize()being called without a lock (line 1315) is not an issue becauseCacheMapprovides internal thread-safety via its own mutex (as documented in the retrieved learnings).Based on learnings.
1318-1348: LGTM! ToJson correctly locks and iterates over mapObjects.The function acquires
cs_store(line 1320) and safely iterates overmapObjectsusing structured bindings (line 1326). The call tocmapVoteToObject.GetSize()at line 1346 is safe due toCacheMap's internal locking.Based on learnings.
src/governance/object.h (15)
91-93: LGTM! Migration from RecursiveMutex to Mutex is correct.The change from
mutable RecursiveMutex cstomutable Mutex csaligns with the PR objectives. The compile-time annotations and runtime assertions mentioned in the PR description ensure that the simplifiedMutexis sufficient (no recursive locking was actually needed).
99-99: LGTM! nDeletionTime properly guarded.The
GUARDED_BY(cs)annotation documents thatnDeletionTimemust be accessed undercs. This is enforced by the locking inPrepareDeletion(lines 197-204) andGetDeletionTime(lines 153-156).
125-125: LGTM! fExpired properly guarded.The
GUARDED_BY(cs)annotation is consistent withSetExpired(lines 168-171) andIsSetExpired(lines 147-150), which both useWITH_LOCK(cs, ...)to access the flag.
130-132: LGTM! Vote-related members properly guarded.Both
mapCurrentMNVotesandfileVotesare correctly markedGUARDED_BY(cs)and are consistently accessed under lock in the .cpp implementations (e.g.,ClearMasternodeVotes,RemoveInvalidVotes,GetCurrentMNVotes).
138-140: LGTM! Deserialization constructor enables shared_ptr creation.The template constructor
CGovernanceObject(deserialize_type, Stream&)allows direct deserialization into ashared_ptr, which is necessary for the new storage model inmapObjectsandmapPostponedObjects.
147-156: LGTM! Getters correctly use EXCLUSIVE_LOCKS_REQUIRED(!cs) and WITH_LOCK.Both
IsSetExpired()(lines 147-150) andGetDeletionTime()(lines 153-156) follow the pattern:
- Annotated with
EXCLUSIVE_LOCKS_REQUIRED(!cs)to prevent callers from holding the lock- Use
WITH_LOCK(cs, ...)internally to acquire the lock and return the guarded valueThis prevents deadlocks and ensures atomic reads.
158-162: LGTM! GetVoteFile requires caller to hold cs.The
EXCLUSIVE_LOCKS_REQUIRED(cs)annotation (line 158) andAssertLockHeld(cs)(line 160) correctly enforce that callers must already holdcswhen accessing the vote file. This is appropriate for internal use where the caller manages locking scope.
168-171: LGTM! SetExpired correctly locks internally.The
EXCLUSIVE_LOCKS_REQUIRED(!cs)annotation prevents callers from holding the lock, andWITH_LOCK(cs, fExpired = true)ensures the write is atomic.
181-189: LGTM! IsValidLocally correctly requires cs_main.The
EXCLUSIVE_LOCKS_REQUIRED(::cs_main)annotations (lines 182, 185) are appropriate because the validation logic needs to check collateral transactions and masternode state, which requirecs_mainto be held.
194-195: LGTM! UpdateSentinelVariables correctly annotated.The
EXCLUSIVE_LOCKS_REQUIRED(!cs)annotation allows the function to lockcsinternally when it needs to updatenDeletionTime(see implementation in object.cpp lines 594-629, specifically line 623 where it locks cs).
197-204: LGTM! PrepareDeletion correctly locks internally.The function is annotated
EXCLUSIVE_LOCKS_REQUIRED(!cs)and usesLOCK(cs)at line 200 to protect the write tonDeletionTime. This ensures atomic updates while preventing deadlocks.
215-227: LGTM! Vote counting methods correctly annotated.All vote counting methods (
GetAbsoluteYesCount,GetAbsoluteNoCount,GetYesCount,GetNoCount,GetAbstainCount) are annotatedEXCLUSIVE_LOCKS_REQUIRED(!cs)because they callCountMatchingVotes, which internally lockscs(see object.cpp line 533).
229-230: LGTM! GetCurrentMNVotes correctly annotated.The
EXCLUSIVE_LOCKS_REQUIRED(!cs)annotation is correct because the implementation (object.cpp lines 582-592) usesLOCK(cs)to protect access tomapCurrentMNVotes.
240-261: LGTM! Serialization correctly locks guarded members.Both
SerializeandUnserializetemplates are annotatedEXCLUSIVE_LOCKS_REQUIRED(!cs)and useLOCK(cs)(lines 246, 257) before accessing guarded members (nDeletionTime,fExpired,mapCurrentMNVotes,fileVotes) on disk. Network serialization doesn't include these fields, which is correct.
270-282: LGTM! Vote processing methods correctly annotated.
ProcessVote(line 270),ClearMasternodeVotes(line 274), andRemoveInvalidVotes(line 281) are all annotatedEXCLUSIVE_LOCKS_REQUIRED(!cs)because they lockcsinternally in their implementations (object.cpp lines 58, 167, 183 respectively).src/governance/governance.h (9)
59-60: LGTM! cs_main declaration is necessary for annotations.The
extern RecursiveMutex cs_maindeclaration (line 59) is required for theEXCLUSIVE_LOCKS_REQUIRED(::cs_main)annotations used throughout the governance code (e.g.,AddGovernanceObjectInternalat line 404).
182-196: LGTM! cs_store and guarded storage members are correctly declared.The changes properly:
- Rename
cstocs_storeand change type toMutex(line 184)- Change
mapObjectsto storestd::shared_ptr<CGovernanceObject>and markGUARDED_BY(cs_store)(line 187)- Mark all other storage members (
mapErasedGovernanceObjects,cmapInvalidVotes,cmmapOrphanVotes,mapLastMasternodeObject,lastMNListForVotingKeys) asGUARDED_BY(cs_store)(lines 191-196)This establishes the foundation for the new locking model.
203-233: LGTM! Serialize/Unserialize correctly lock cs_store.Both
Serialize(lines 203-213) andUnserialize(lines 216-233) are annotatedEXCLUSIVE_LOCKS_REQUIRED(!cs_store)and useLOCK(cs_store)(lines 205, 220) before accessing guarded members. The version string check at lines 223-225 correctly bails out on mismatch.
235-239: LGTM! Clear and ToString correctly annotated.Both methods are declared
EXCLUSIVE_LOCKS_REQUIRED(!cs_store), which allows them to acquirecs_storeinternally. The implementations in governance.cpp (lines 1259-1283 and 1285-1316) correctly useLOCK(cs_store).
251-251: LGTM! object_ref_cm_t type alias supports the new storage model.The
using object_ref_cm_t = CacheMap<uint256, std::shared_ptr<CGovernanceObject>>type alias correctly defines the storage type forcmapVoteToObject(line 266), which maps vote hashes to governance objects via shared_ptr.
263-270: LGTM! Member variables correctly initialized with defaults.The explicit default initialization (e.g.,
nTimeLastDiff{0},nCachedBlockHeight{0},fRateChecksEnabled{true}) makes the intended initial state clear and is consistent with modern C++ best practices.
284-295: LGTM! Basic API methods correctly annotated.The public methods (
LoadCache,ToString,ToJson,Clear,CheckAndRemove,Schedule) are consistently annotated withEXCLUSIVE_LOCKS_REQUIRED(!cs_store)and/orEXCLUSIVE_LOCKS_REQUIRED(!cs_relay), which allows them to acquire these locks internally and prevents deadlocks.
303-376: LGTM! Public accessor and interface methods properly annotated.The extensive locking contracts across:
- Getters/setters (lines 303-307)
- Networking methods (lines 315-330)
- Notification interface (lines 333-334)
- Signer interface (lines 337-344)
- Superblock methods (lines 347-375)
All consistently use
EXCLUSIVE_LOCKS_REQUIRED(!cs_store)to allow internal locking. The separation of concerns betweencs_store(governance state) andcs_relay(relay queue) is clean and prevents lock-ordering issues.
380-458: LGTM! Internal methods correctly require locks to be held.The private/internal methods (lines 382-458) consistently use
EXCLUSIVE_LOCKS_REQUIRED(cs_store)to assert the lock is already held by the caller. This pattern:
- Prevents unnecessary lock/unlock cycles
- Documents lock invariants clearly
- Enables compile-time verification via thread-safety analysis
The split between public
Add*/Find*/Get*methods and internal*Internalcounterparts is clean and maintainable.
4 participants
Additional Information
Dependent on refactor: split off governance masternode-only logic to
GovernanceSigner, dropRelay()s and use periodic relay instead, minor cleanup #6838The
RecursiveMutexinCGovernanceObjectwas first used in b02d526 (source) but as the structure evolved fromCBudgetProposalto its current form, at no point has the mutex guarded any members (using theGUARDED_BYmacro).This has been resolved by guarding variables that are (de)serialised and updating annotations to reflect this change. As the usage of
CGovernanceObject::csdoes not warrant recursive locking, it has been updated to be a simplerMutexand has been made private.cmapVoteToObjectwas incorrectly moved toGovernanceStorein dash#5555 despite it being memory-only. This has been resolved and it has been moved toCGovernanceManager, where memory-only variables reside.GovernanceStorageare now protected by the mutexcs_store(formerlycs) and annotations inCGovernanceManagerhas been updated to reflect this explicitly. This has not been extended to memory-only variables.With the compile-time annotations, runtime assertions and internal functions that assume the lock is already held, we can reasonably detect double-locking scenarios and can safely move the
GovernanceStore::cs_storeRecursiveMutexto aMutexThe loss of recursive locking resulted in detection of inconsistent lock ordering (see below), this has been resolved in a separate commit.
Error:
Breaking Changes
None expected.
Checklist