refactor: separate network and consensus logic 3/N [governance] #7008
Conversation
knst
changed the title
|
WalkthroughThe PR refactors governance networking into a NetGovernance NetHandler and centralizes handler scheduling via PeerManager::ScheduleHandlers. CGovernanceManager's network-facing API is simplified: ProcessMessage is replaced by ProcessObject and several new data-accessors (FetchRelayInventory, FetchGovernanceObjectVotes, GetMNManager, ProcessVote, etc.) are added with updated locking annotations. NetHandler gains Schedule and ProcessMessage now accepts a CConnman& parameter. CMasternodeSync governance tick/maintenance code is removed. LoadChainstate signatures drop governance/txindex/network-id parameters. Test expectations for circular dependencies are updated. Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant PeerManager
participant NetGovernance
participant CGovernanceManager
participant Connman
Note over Scheduler,PeerManager: New handler registration flow
Scheduler->>PeerManager: ScheduleHandlers()
activate PeerManager
PeerManager->>NetGovernance: Schedule(scheduler, connman)
deactivate PeerManager
Note over Scheduler,NetGovernance: Periodic tick + message handling
Scheduler->>NetGovernance: ProcessTick(connman)
activate NetGovernance
alt Need votes/objects
NetGovernance->>NetGovernance: RequestGovernanceObjectVotes(...)
NetGovernance->>Connman: send requests
else Sync progress / housekeeping
NetGovernance->>CGovernanceManager: FetchRelayInventory / FetchGovernanceObjectVotes
end
deactivate NetGovernance
Note over PeerManager,NetGovernance: Inbound message processing
PeerManager->>NetGovernance: ProcessMessage(peer, connman, msg_type, vRecv)
activate NetGovernance
alt MNGOVERNANCEOBJECT
NetGovernance->>CGovernanceManager: ProcessObject(peer, hash, govobj)
else MNGOVERNANCEOBJECTVOTE
NetGovernance->>CGovernanceManager: ProcessVote(peer, vote, exception, connman)
end
NetGovernance->>PeerManager: PeerPostProcessMessage(result)
deactivate NetGovernance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
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)
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: 2
🧹 Nitpick comments (9)
src/net_processing.cpp (1)
662-662: ClarifyPeerPostProcessMessagesemantics for Node-scoped side effects
PeerPostProcessMessagealways callsPostProcessMessage(std::move(ret), -1), so anym_error/m_to_eraseinMessageProcessingResultwill not be attributed to a real peer (bothMisbehavingandEraseObjectRequesteffectively no-op for node-1).If NetHandlers are expected to:
- Drive misbehavior scoring or
- Erase per-peer object requests
via
MessageProcessingResult, they will currently not get those behaviors when going throughPeerPostProcessMessage. In that case, either:
- Thread the actual
NodeIdthroughPeerPostProcessMessage, or- Document that handlers must call
PeerMisbehaving/PeerEraseObjectRequestdirectly and only usePeerPostProcessMessagefor node-agnostic actions (m_transactions,m_inventory,m_dsq).Please confirm which contract you intend and adjust either the API or callers accordingly.
Also applies to: 3587-3604, 6579-6582
src/governance/net_governance.cpp (7)
20-20: Unnecessary forward declaration.
CConnmanis already declared innet.hwhich is included on line 12. This forward declaration is redundant.-class CConnman; - void NetGovernance::Schedule(CScheduler& scheduler, CConnman& connman)
131-131: Minor typo: "to early" → "too early".- // to early to ask the same node + // too early to ask the same node
161-162: Minor style: preferstatic constoverconst static.Both are valid C++, but
static constis the more conventional ordering.- const static int64_t nSyncStart = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()); - const static std::string strAllow = strprintf("allow-sync-%lld", nSyncStart); + static const int64_t nSyncStart = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()); + static const std::string strAllow = strprintf("allow-sync-%lld", nSyncStart);
310-310: Minor typo: "to early" → "too early".- continue; // to early for this node + continue; // too early for this node
379-382: RedundantIsBlockchainSynced()check.This check is redundant because line 348 already returns early if
!m_node_sync.IsBlockchainSynced(). Consider removing for clarity.- if (!m_node_sync.IsBlockchainSynced()) { - LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECT -- masternode list not synced\n"); - return; - }
409-412: RedundantIsBlockchainSynced()check.Same as above - this is already guarded by the check on line 348.
- if (!m_node_sync.IsBlockchainSynced()) { - LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECTVOTE -- masternode list not synced\n"); - return; - }
440-441: Existing TODO comment about relay issue.This TODO indicates a known issue with immediate inventory relay. Consider tracking this in an issue if not already done.
Would you like me to help document this issue or investigate why immediate sending doesn't work here?
src/governance/governance.h (1)
389-389: Minor: extra blank line.bool ProcessVote(CNode* pfrom, const CGovernanceVote& vote, CGovernanceException& exception, CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(!cs_store); - private:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/Makefile.am(2 hunks)src/governance/governance.cpp(9 hunks)src/governance/governance.h(1 hunks)src/governance/net_governance.cpp(1 hunks)src/governance/net_governance.h(1 hunks)src/init.cpp(3 hunks)src/instantsend/net_instantsend.cpp(1 hunks)src/instantsend/net_instantsend.h(1 hunks)src/masternode/sync.cpp(1 hunks)src/masternode/sync.h(2 hunks)src/net_processing.cpp(5 hunks)src/net_processing.h(3 hunks)src/node/chainstate.cpp(0 hunks)src/node/chainstate.h(0 hunks)src/test/util/setup_common.cpp(0 hunks)src/validation.h(0 hunks)test/lint/lint-circular-dependencies.py(2 hunks)
💤 Files with no reviewable changes (4)
- src/test/util/setup_common.cpp
- src/validation.h
- src/node/chainstate.h
- src/node/chainstate.cpp
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/masternode/sync.cppsrc/governance/net_governance.hsrc/instantsend/net_instantsend.cppsrc/governance/governance.cppsrc/masternode/sync.hsrc/net_processing.hsrc/instantsend/net_instantsend.hsrc/init.cppsrc/net_processing.cppsrc/governance/net_governance.cppsrc/governance/governance.h
src/{masternode,evo}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/sync.cppsrc/masternode/sync.h
src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Files:
src/masternode/sync.cppsrc/governance/net_governance.hsrc/governance/governance.cppsrc/masternode/sync.hsrc/governance/net_governance.cppsrc/governance/governance.h
src/{masternode,llmq}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
BLS integration must be used for cryptographic foundation of advanced masternode features
Files:
src/masternode/sync.cppsrc/masternode/sync.h
src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Files:
src/masternode/sync.cppsrc/governance/net_governance.hsrc/governance/governance.cppsrc/masternode/sync.hsrc/governance/net_governance.cppsrc/governance/governance.h
src/governance/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Files:
src/governance/net_governance.hsrc/governance/governance.cppsrc/governance/net_governance.cppsrc/governance/governance.h
🧠 Learnings (21)
📓 Common learnings
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: 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.
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: 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: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Applied to files:
src/Makefile.amsrc/masternode/sync.cppsrc/governance/net_governance.hsrc/governance/governance.cppsrc/masternode/sync.hsrc/init.cppsrc/governance/net_governance.cppsrc/governance/governance.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/Makefile.amsrc/governance/governance.cppsrc/masternode/sync.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/Makefile.amsrc/governance/governance.cppsrc/net_processing.hsrc/governance/governance.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Applied to files:
src/masternode/sync.cppsrc/governance/governance.cppsrc/masternode/sync.hsrc/init.cppsrc/governance/governance.h
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket operations are properly synchronized using the cs_net mutex. The m_sock and m_server members are GUARDED_BY(cs_net), and methods like Connect(), SendDirectly(), and ReconnectThread() use appropriate locking with cs_net to prevent race conditions on socket access.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access requires synchronization using the cs_net mutex. The race condition exists between SendDirectly() (callable from any thread) and Connect() (called by ReconnectThread), where both read and write m_sock concurrently. The cs_net mutex properly coordinates these network operations.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/instantsend/net_instantsend.cppsrc/governance/governance.h
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access is protected by the cs_net mutex. This mutex coordinates between the TCP send path in SendDirectly() and the reconnection operations in Connect()/Reconnect() methods, ensuring proper synchronization of socket state.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket access operations (m_sock and m_server) should be protected by the cs_net mutex, not avoiding synchronization entirely. While lock overhead concerns are valid in general, socket operations require proper synchronization via cs_net.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 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 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
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
src/governance/governance.cppsrc/masternode/sync.hsrc/init.cpp
📚 Learning: 2025-10-28T08:54:00.392Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6926
File: test/functional/feature_governance_cl.py:0-0
Timestamp: 2025-10-28T08:54:00.392Z
Learning: In Dash tests, the scheduler (mockscheduler) operates independently of network state. Isolated nodes should still run scheduler-based cleanup processes like governance cleanup, even if they have different state due to network isolation.
Applied to files:
src/governance/governance.cppsrc/init.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/masternode/sync.h
📚 Learning: 2025-11-04T18:23:28.175Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/governance/classes.cpp:147-154
Timestamp: 2025-11-04T18:23:28.175Z
Learning: In src/governance/classes.cpp, CSuperblock::GetPaymentsLimit intentionally uses synthetic difficulty constants (nBits = 1 for mainnet, powLimit for networks with min difficulty) and simple height-based V20 activation checks instead of actual chain block data. This is by design because superblocks themselves are "synthetic" governance payment blocks, not regular mined blocks.
Applied to files:
src/masternode/sync.hsrc/governance/governance.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
src/masternode/sync.hsrc/init.cppsrc/governance/governance.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/init.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/**/*.{cpp,h} : Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Applied to files:
src/init.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Applied to files:
src/init.cpp
📚 Learning: 2025-11-13T20:02:55.480Z
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.).
Applied to files:
src/governance/governance.h
🧬 Code graph analysis (7)
src/governance/net_governance.h (3)
src/governance/governance.cpp (2)
CGovernanceManager(66-79)CGovernanceManager(81-85)src/net_processing.h (3)
NetHandler(71-87)NetHandler(75-78)PeerManagerInternal(58-69)src/governance/net_governance.cpp (10)
Schedule(22-54)Schedule(22-22)ProcessMessage(345-449)ProcessMessage(345-345)SendGovernanceSyncRequest(56-61)SendGovernanceSyncRequest(56-56)RequestGovernanceObjectVotes(63-152)RequestGovernanceObjectVotes(63-63)ProcessTick(154-343)ProcessTick(154-154)
src/instantsend/net_instantsend.cpp (1)
src/net_processing.cpp (12)
ProcessMessage(3681-5471)ProcessMessage(3681-3686)pfrom(618-618)pfrom(639-640)pfrom(753-755)pfrom(764-764)pfrom(772-772)pfrom(775-775)pfrom(777-777)pfrom(779-779)pfrom(875-875)pfrom(1057-1057)
src/governance/governance.cpp (2)
src/governance/governance.h (1)
nCachedBlockHeight(261-261)src/net.h (1)
FullyConnectedOnly(1296-1296)
src/net_processing.h (1)
src/net_processing.cpp (26)
PeerGetRequestedObjectCount(6574-6577)PeerGetRequestedObjectCount(6574-6574)nodeid(628-628)nodeid(631-631)nodeid(644-644)nodeid(656-656)nodeid(661-661)nodeid(679-679)nodeid(681-681)nodeid(719-720)nodeid(730-730)nodeid(862-862)nodeid(864-864)nodeid(1040-1040)nodeid(1075-1075)PeerPostProcessMessage(6579-6582)PeerPostProcessMessage(6579-6579)ret(662-662)ret(685-685)scheduler(624-624)scheduler(652-652)scheduler(694-694)ProcessMessage(3681-5471)ProcessMessage(3681-3686)ScheduleHandlers(1701-1706)ScheduleHandlers(1701-1701)
src/instantsend/net_instantsend.h (2)
src/net_processing.cpp (12)
ProcessMessage(3681-5471)ProcessMessage(3681-3686)pfrom(618-618)pfrom(639-640)pfrom(753-755)pfrom(764-764)pfrom(772-772)pfrom(775-775)pfrom(777-777)pfrom(779-779)pfrom(875-875)pfrom(1057-1057)src/instantsend/net_instantsend.cpp (2)
ProcessMessage(17-65)ProcessMessage(17-17)
src/init.cpp (3)
src/interfaces/node.h (2)
node(42-44)node(404-404)src/rpc/blockchain.h (1)
node(33-36)src/init.h (1)
node(23-25)
src/governance/net_governance.cpp (3)
src/util/time.h (1)
TicksSinceEpoch(86-89)src/net.h (1)
FullyConnectedOnly(1296-1296)src/masternode/sync.cpp (2)
ProcessMessage(99-112)ProcessMessage(99-99)
⏰ 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_tsan-build / Build source
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (29)
src/Makefile.am (1)
222-223: Net governance unit correctly wired into build systemAdding
governance/net_governance.htoBITCOIN_CORE_Handgovernance/net_governance.cpptolibbitcoin_node_a_SOURCESis consistent with how other governance components are exposed and linked; no issues from a build/ordering perspective.Also applies to: 511-511
src/instantsend/net_instantsend.h (1)
34-38: NetInstantSend interface aligned with NetHandler and Stop/Interrupt contractUpdating
ProcessMessageto acceptCConnman&and adding anInterrupt()override that forwards toworkInterrupt()cleanly integrates NetInstantSend with the new NetHandler interface and the existingStop()assertion that expectsInterrupt()to be called first. No functional regressions apparent.src/instantsend/net_instantsend.cpp (1)
17-17: ProcessMessage signature update is consistent and behavior‑preservingThe implementation now takes
CConnman&(unnamed) to match the updated NetHandler interface and net_processing call sites, while leaving the message handling logic unchanged. This maintains existing InstantSend behavior.src/net_processing.cpp (3)
1701-1706: Handler scheduling loop is straightforward and thread-safe
ScheduleHandlersjust iteratesm_handlersand forwardsschedulerandm_connman; there are no new locking or lifecycle hazards here given handlers are already managed viaStart/Stop/Interrupt.
5462-5463: EnsureNetHandler::ProcessMessage’s contract matches ignoring its return value
handler->ProcessMessage(pfrom, m_connman, msg_type, vRecv);is now called for eachm_handlersentry, and its return value (if any) is ignored, unlike the built-in handlers above which still funnel theirMessageProcessingResultthroughPostProcessMessage.Please verify that:
NetHandler::ProcessMessagehas been changed tovoid(handlers do their own post-processing viaPeer*APIs), or- If it still returns a
MessageProcessingResult, that all relevant logic was migrated into the handlers and no behavior previously driven by the result is now lost.If the latter, consider restoring a
PostProcessMessagecall on the result to keep the behavior consistent.
652-652: Scheduling API surface is cohesive with existing handler lifecycleAdding
ScheduleHandlers(CScheduler&)alongsideStartHandlers/StopHandlers/InterruptHandlersmaintains a clear responsibility split: construction, lifecycle, and periodic scheduling are all delegated throughPeerManagerImpl. The current design keeps handler ownership and scheduling concerns centralized.Also applies to: 1670-1699
src/masternode/sync.cpp (1)
7-8: Includes updated to match used typesAdding
<chain.h>and<net.h>is appropriate here given this implementation usesCBlockIndex,CConnman, andCNode. This tightens dependencies without changing behavior.src/init.cpp (2)
2229-2229: LGTM! NetGovernance handler correctly wired.The handler is properly constructed with required dependencies and added after loading the governance cache. This follows the same pattern as
NetInstantSendon line 2198.Note: The
NetGovernance::ProcessMessageandNetGovernance::Scheduleimplementations already checkm_gov_manager.IsValid()internally (persrc/governance/net_governance.cpplines 23 and 344), so the handler can be safely added regardless ofis_governance_enabled.
2301-2301: Centralized scheduling viaScheduleHandlersis a clean refactor.This replaces governance-specific scheduling with a generalized mechanism that iterates over all registered
NetHandlerinstances and calls theirSchedulemethod. This improves separation of concerns.src/net_processing.h (3)
67-68: Appropriate extension ofPeerManagerInternalinterface.These methods expose necessary peer-level operations to
NetHandlerimplementations:
PeerGetRequestedObjectCount: Allows handlers to check request queue size to preventsetAskForoverflow (used inRequestGovernanceObjectVotes)PeerPostProcessMessage: Enables handlers to trigger post-processing for misbehavior trackingBoth are properly implemented in
PeerManagerImplper the relevant snippets.
83-84:NetHandlerinterface extended with scheduling and message processing hooks.Adding
CConnman¶meter to both methods is necessary for handlers to send network messages. The default empty implementations allow handlers to opt-in to only the functionality they need.
174-174:ScheduleHandlersprovides centralized scheduling orchestration.This completes the
PeerManagerinterface to support the new handler scheduling pattern used ininit.cpp.src/governance/net_governance.h (1)
1-39: Well-structured newNetGovernancehandler class.The design cleanly separates governance networking concerns:
- Proper inheritance from
NetHandlerwithfinalspecifier- Constructor dependency injection pattern matches the codebase style
- References (not pointers) for required dependencies ensure non-null invariants
- Private helpers encapsulate implementation details
Minor observation: The include guard uses
BITCOIN_GOVERNANCE_NET_GOVERNANCE_Hwhich follows the existing convention in the codebase.test/lint/lint-circular-dependencies.py (2)
42-43: Circular dependency expectations correctly updated.The paths now route through
governance/governancedirectly instead of viamasternode/active/context, reflecting the architectural change where governance networking is decoupled from the active masternode context.
53-53: The new circular dependency is a necessary consequence of the design, not unintended.The cycle exists because
CMasternodeSynchas aCConnman&member variable (sync.h line 52) and calls methods on it in the implementation, requiringsync.cppto include the fullnet.hdefinition. Conversely, multiple functions inCConnmantakeCMasternodeSync¶meters and appear to require the full definition, necessitating the include innet.cpp. This tight coupling is the minimal expected dependency given the architectural choice to embedCConnman&inCMasternodeSync. No refactoring is needed unless the underlying architecture changes.src/masternode/sync.h (2)
22-28: Sync state constants properly exposed.Moving these constants to the header allows
NetGovernanceand other components to reference sync states and timing values without duplicating magic numbers. The values are well-documented with comments.
67-73: New accessors enable external governance sync management.These additions expose the minimum necessary internal state:
BumpAttempt(): AllowsNetGovernanceto track peer attemptsGetLastBump(): Enables timeout checking in external schedulingIsReachedBestHeader(): Required for sync state decisionsThis is a good pattern - exposing controlled accessors rather than making members public or passing the entire object.
src/governance/net_governance.cpp (3)
22-54: LGTM!The scheduling logic correctly gates governance-specific tasks behind
IsValid()and uses appropriate timing for test vs. production environments.
94-96: Static local state requires careful consideration.Using
staticlocals forcs_recentlyandmapAskedRecentlymeans this state persists across allNetGovernanceinstances and is shared globally. While this appears intentional (to track recently-asked peers across sync sessions), ensure this is the desired behavior, especially if multipleNetGovernanceinstances could exist.
140-147: Verify loop termination when no nodes can be asked.The
i--on line 146 compensates whenfAskedis false, but since lines 141-145 unconditionally pop from the vectors regardless offAsked, the loop will eventually terminate when vectors empty (line 116 breaks). This logic appears correct but is subtle.src/governance/governance.cpp (8)
27-27: LGTM!Include change from
validation.htovalidationinterface.haligns with the PR's goal of separating concerns. The file usesGetMainSignals()fromvalidationinterface.hfor notifications.
183-231: LGTM with optional efficiency note.The new
ProcessObjectmethod correctly consolidates governance object processing. The lock annotations properly requirecs_mainto be held by the caller (as shown innet_governance.cppline 393 withWITH_LOCK(::cs_main, ...)).For a future optimization, consider taking
CGovernanceObjectby rvalue reference since it's ultimately moved into storage, but this is fine for a refactoring PR.
449-455: LGTM!Clean swap-and-return pattern for fetching relay inventory with proper locking.
599-599: LGTM!Adding explicit
LOCK(cs_store)at function start correctly implements the lock requirement for accessingmapObjects.
652-652: LGTM!Same as above - proper lock acquisition for
SyncObjects.
960-960: LGTM!
GetMNManager()correctly usesAssertto ensurem_dmnmanis valid before dereferencing.
962-991: LGTM!The updated
FetchGovernanceObjectVotesreturns a structured pair of vectors, providing a cleaner API than modifying parameters in-place.
1174-1198: LGTM!The orphan vote cleanup logic is correctly inlined, and the
NodesSnapshotpattern withFullyConnectedOnlymatches usage inProcessTick()(seenet_governance.cppline 179).src/governance/governance.h (1)
367-388: LGTM! Clear API section for NetGovernance integration.The new public methods are well-organized under a clear comment and have proper lock annotations. The API surface exposed to
NetGovernanceis appropriately scoped:
FetchRelayInventory()- relay coordinationCheckAndRemove()- cleanup operationsRequestOrphanObjects()- orphan managementFetchGovernanceObjectVotes()- vote request preparationProcessObject()/ProcessVote()- object/vote processingSyncObjects()/SyncSingleObjVotes()- sync responsesAcceptMessage()- request trackingGetMNManager()- MN list access
| /** Default for -syncmempool */ | ||
| static const bool DEFAULT_SYNC_MEMPOOL = true; |
There was a problem hiding this comment.
🧩 Analysis chain
DEFAULT_SYNC_MEMPOOL constant placement.
This constant is now defined in the masternode/sync header. Verify this is the intended location, as it appears to be a general node configuration option rather than masternode-specific.
🏁 Script executed:
#!/bin/bash
# Check where DEFAULT_SYNC_MEMPOOL was previously defined and where it's used
rg -n "DEFAULT_SYNC_MEMPOOL" --type cpp --type hLength of output: 383
I'll verify whether this location is appropriate by checking the pattern for similar configuration constants.
Actually, let me correct that - I need to search the actual codebase:
🏁 Script executed:
#!/bin/bash
# Check pattern of DEFAULT_* constants and their locations
echo "=== Finding DEFAULT_* constants ==="
rg -n "static const.*DEFAULT_" --type cpp --type h | head -20
echo ""
echo "=== Checking what init.cpp includes for constants ==="
rg -n "^#include" src/init.cpp | head -20
echo ""
echo "=== Checking where other mempool constants are defined ==="
rg -n "mempool" --type h src/ | grep -i "const\|define" | head -15Length of output: 4544
🏁 Script executed:
This will be run on the codebase to check patterns.Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check pattern of DEFAULT_* constants and their locations
echo "=== Finding DEFAULT_* constants ==="
rg -n "static const.*DEFAULT_" --type cpp --type h | head -20
echo ""
echo "=== Checking what init.cpp includes ==="
head -50 src/init.cpp | rg "^#include"
echo ""
echo "=== Check other DEFAULT_ constants in masternode/sync.h and nearby files ==="
rg -n "DEFAULT_" src/masternode/ --type cpp --type h | head -15Length of output: 2768
Let me check if this constant was recently added or moved to this location.
🏁 Script executed:
#!/bin/bash
# Check if validation.h is included by masternode/sync.h or other related files
echo "=== Checking masternode/sync.h includes ==="
head -30 src/masternode/sync.h
echo ""
echo "=== Check validation.h for mempool-related constants ==="
rg -n "DEFAULT_.*MEMPOOL|DEFAULT_.*SYNC" src/validation.h
echo ""
echo "=== Look for any other instances of DEFAULT_SYNC in codebase ==="
rg -n "DEFAULT_SYNC" --type cpp --type hLength of output: 1813
Move DEFAULT_SYNC_MEMPOOL to src/validation.h with other mempool-related defaults.
The constant is placed in src/masternode/sync.h but should follow the established pattern: similar mempool constants like DEFAULT_MEMPOOL_EXPIRY and DEFAULT_PERSIST_MEMPOOL are defined in src/validation.h. The -syncmempool option is a general node configuration (used in src/init.cpp) and governance (src/governance/net_governance.cpp), not masternode-specific functionality. This placement should be moved to maintain proper separation of concerns and code organization.
🤖 Prompt for AI Agents
In src/masternode/sync.h around lines 17-18, the DEFAULT_SYNC_MEMPOOL constant
belongs in src/validation.h with the other mempool defaults; remove the static
const bool DEFAULT_SYNC_MEMPOOL = true; from sync.h, add the same definition
into src/validation.h next to DEFAULT_MEMPOOL_EXPIRY/DEFAULT_PERSIST_MEMPOOL,
and ensure any files that previously relied on including sync.h for this symbol
(e.g., src/init.cpp, src/governance/net_governance.cpp) include src/validation.h
instead; rebuild to verify no missing symbol or include errors.
There was a problem hiding this comment.
That's correct; the file masternode/sync.h which have CMasternodeSync is incorrectly named and incorrectly placed. It has nothing to do with "masternode".
It should be called NodeSyncing or NodeSyncStatus or something similar and be located in node/sync.h.
DEFAULT_SYNC_MEMPOOL is correctly located now. it has nothing to do with "validation" in validation.h
Renaming it is out-of-scope of this PR; could be done someday later
There was a problem hiding this comment.
@knst, thank you for the clarification! You're absolutely right that DEFAULT_SYNC_MEMPOOL belongs with the synchronization logic conceptually, not with validation. I understand now that while the file naming and location could be improved (as a future refactoring to node/sync.h with a class rename), the constant is correctly co-located with the sync functionality it relates to.
I appreciate you explaining the architectural reasoning—this helps me better understand the separation between sync-related configuration and validation-related configuration.
(\__/)
(='.'=) *noted for future reviews*
(")_(")
✏️ Learnings added
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.501Z
Learning: In Dash Core, `DEFAULT_SYNC_MEMPOOL` belongs with node synchronization logic (currently in src/masternode/sync.h) rather than with validation logic in src/validation.h, even though other mempool-related constants are in validation.h. The constant is conceptually tied to sync functionality, not validation.
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.501Z
Learning: The file src/masternode/sync.h containing `CMasternodeSync` is misnamed and misplaced—it has nothing to do with "masternode" functionality. It should eventually be renamed to `NodeSyncing` or `NodeSyncStatus` and moved to src/node/sync.h as a future refactoring.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
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: 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: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
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.
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.
src/net_processing.cpp
Outdated
| void PeerRelayInvFiltered(const CInv& inv, const CTransaction& relatedTx) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); | ||
| void PeerRelayInvFiltered(const CInv& inv, const uint256& relatedTxHash) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); | ||
| void PeerAskPeersForTransaction(const uint256& txid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); | ||
| size_t PeerGetRequestedObjectCount(NodeId nodeid) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); |
There was a problem hiding this comment.
Fix missing cs_main lock in PeerGetRequestedObjectCount wrapper
GetRequestedObjectCount asserts cs_main and reads m_node_states (GUARDED_BY(cs_main)), but PeerGetRequestedObjectCount calls it without taking cs_main, which will:
- Trip
AssertLockHeld(cs_main)in debug builds. - Cause a data race on
m_node_statesin release builds.
Wrap the call in a cs_main lock (and keep the !m_peer_mutex precondition to preserve lock order), e.g.:
size_t PeerManagerImpl::PeerGetRequestedObjectCount(NodeId nodeid) const
{
- return GetRequestedObjectCount(nodeid);
+ WITH_LOCK(cs_main, return GetRequestedObjectCount(nodeid));
}This aligns the wrapper with the callee’s locking contract.
Also applies to: 1650-1658, 6574-6577
🤖 Prompt for AI Agents
In src/net_processing.cpp around line 661 (and similarly at 1650-1658 and
6574-6577), the PeerGetRequestedObjectCount wrapper calls
GetRequestedObjectCount without holding cs_main even though
GetRequestedObjectCount asserts cs_main and reads m_node_states
(GUARDED_BY(cs_main)); update each wrapper to acquire cs_main while preserving
the existing !m_peer_mutex precondition/lock ordering, call
GetRequestedObjectCount while cs_main is held, then release cs_main before
returning so the wrapper satisfies the callee’s locking contract and avoids
assert/data-race.
There was a problem hiding this comment.
cs_main is hold correctly, see:
WITH_LOCK(::cs_main, m_peer_manager->PeerEraseObjectRequest(peer.GetId(), CInv{MSG_GOVERNANCE_OBJECT, nHash}));
Annotation exists also:
void PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
There was a problem hiding this comment.
@knst Thanks for the clarification — agreed on PeerEraseObjectRequest: it’s annotated EXCLUSIVE_LOCKS_REQUIRED(::cs_main) and the call site holds cs_main.
The concern I raised is specifically about PeerGetRequestedObjectCount. Its interface is only EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex), but it calls GetRequestedObjectCount(), which asserts cs_main and reads m_node_states (GUARDED_BY(cs_main)). Since callers of PeerGetRequestedObjectCount are not required to hold cs_main, this can trip the AssertLockHeld(cs_main) in debug or race in release.
Minimal, safe fix (preserves the existing precondition and lock ordering):
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@
size_t PeerManagerImpl::PeerGetRequestedObjectCount(NodeId nodeid) const
{
- return GetRequestedObjectCount(nodeid);
+ // Wrapper does not require cs_main; the callee asserts cs_main.
+ // Take cs_main here to satisfy the callee’s locking contract while keeping
+ // the !m_peer_mutex precondition and lock order intact.
+ WITH_LOCK(::cs_main, return GetRequestedObjectCount(nodeid));
}If you prefer the alternative of making cs_main an explicit requirement for the wrapper, we can also tighten the annotation in the interface to EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !m_peer_mutex) and adjust call sites accordingly. Let me know which direction you’d like; I can update the patch.
🧠 Learnings used
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket access operations (m_sock and m_server) should be protected by the cs_net mutex, not avoiding synchronization entirely. While lock overhead concerns are valid in general, socket operations require proper synchronization via cs_net.
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
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: 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: 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.
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.
dc1d437 to
094a3f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/governance/net_governance.cpp (3)
20-20: Redundant forward declaration.
CConnmanis already included transitively through the headers. This forward declaration is unnecessary.-class CConnman; - void NetGovernance::Schedule(CScheduler& scheduler, CConnman& connman)
131-132: Typo: "to early" should be "too early".- // to early to ask the same node + // too early to ask the same node if (mapAskedRecently[nHashGovobj].count(pnode->addr)) continue;
154-171: Static local variables limit testability.The static variables (
nTick,nSyncStart,strAllow,nTimeLastProcess) persist across calls and would be shared if multipleNetGovernanceinstances existed. This limits unit test isolation.Consider promoting these to member variables in a follow-up PR to improve testability, though this is acceptable for a refactoring PR focused on code movement.
src/governance/governance.cpp (1)
183-231: Consider adding lock assertion for clarity.The method is declared with
EXCLUSIVE_LOCKS_REQUIRED(::cs_main, ...)and is correctly called withcs_mainheld inNetGovernance::ProcessMessage. However, adding an explicit assertion at the method entry would make the requirement self-documenting and catch misuse earlier (before the assertion inAddGovernanceObjectInternal).bool CGovernanceManager::ProcessObject(const CNode& peer, const uint256& nHash, CGovernanceObject govobj) { + AssertLockHeld(::cs_main); std::string strHash = nHash.ToString(); LOCK(cs_store);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/governance/governance.cpp(9 hunks)src/governance/governance.h(1 hunks)src/governance/net_governance.cpp(1 hunks)src/governance/net_governance.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/governance/net_governance.cppsrc/governance/net_governance.hsrc/governance/governance.cppsrc/governance/governance.h
src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Files:
src/governance/net_governance.cppsrc/governance/net_governance.hsrc/governance/governance.cppsrc/governance/governance.h
src/governance/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Files:
src/governance/net_governance.cppsrc/governance/net_governance.hsrc/governance/governance.cppsrc/governance/governance.h
src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Files:
src/governance/net_governance.cppsrc/governance/net_governance.hsrc/governance/governance.cppsrc/governance/governance.h
🧠 Learnings (12)
📓 Common learnings
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: 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.
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: 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: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Applied to files:
src/governance/net_governance.cppsrc/governance/net_governance.hsrc/governance/governance.cppsrc/governance/governance.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/governance/net_governance.cppsrc/governance/governance.cppsrc/governance/governance.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Applied to files:
src/governance/governance.cppsrc/governance/governance.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
src/governance/governance.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/governance/governance.cpp
📚 Learning: 2025-10-28T08:54:00.392Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6926
File: test/functional/feature_governance_cl.py:0-0
Timestamp: 2025-10-28T08:54:00.392Z
Learning: In Dash tests, the scheduler (mockscheduler) operates independently of network state. Isolated nodes should still run scheduler-based cleanup processes like governance cleanup, even if they have different state due to network isolation.
Applied to files:
src/governance/governance.cpp
📚 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 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
📚 Learning: 2025-11-13T20:02:55.480Z
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.).
Applied to files:
src/governance/governance.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
src/governance/governance.h
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/governance/governance.h
📚 Learning: 2025-11-04T18:23:28.175Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/governance/classes.cpp:147-154
Timestamp: 2025-11-04T18:23:28.175Z
Learning: In src/governance/classes.cpp, CSuperblock::GetPaymentsLimit intentionally uses synthetic difficulty constants (nBits = 1 for mainnet, powLimit for networks with min difficulty) and simple height-based V20 activation checks instead of actual chain block data. This is by design because superblocks themselves are "synthetic" governance payment blocks, not regular mined blocks.
Applied to files:
src/governance/governance.h
🧬 Code graph analysis (3)
src/governance/net_governance.h (4)
src/governance/governance.cpp (2)
CGovernanceManager(66-79)CGovernanceManager(81-85)src/masternode/sync.cpp (4)
CMasternodeSync(14-20)CMasternodeSync(22-22)ProcessMessage(99-112)ProcessMessage(99-99)src/net_processing.h (3)
NetHandler(71-87)NetHandler(75-78)PeerManagerInternal(58-69)src/governance/net_governance.cpp (10)
Schedule(22-54)Schedule(22-22)ProcessMessage(345-449)ProcessMessage(345-345)SendGovernanceSyncRequest(56-61)SendGovernanceSyncRequest(56-56)RequestGovernanceObjectVotes(63-152)RequestGovernanceObjectVotes(63-63)ProcessTick(154-343)ProcessTick(154-154)
src/governance/governance.cpp (2)
src/governance/governance.h (1)
nCachedBlockHeight(261-261)src/coinjoin/client.h (1)
nCachedBlockHeight(264-264)
src/governance/governance.h (2)
src/governance/governance.cpp (10)
FetchRelayInventory(449-455)FetchRelayInventory(449-449)AcceptMessage(993-1005)AcceptMessage(993-993)ProcessObject(183-231)ProcessObject(183-183)GetMNManager(960-960)GetMNManager(960-960)ProcessVote(800-858)ProcessVote(800-800)src/governance/object.cpp (2)
ProcessVote(58-159)ProcessVote(58-59)
⏰ 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_tsan-build / Build source
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (6)
src/governance/net_governance.h (1)
14-37: Clean NetHandler implementation for governance networking.The
NetGovernanceclass properly derives fromNetHandlerand follows the established pattern for network handlers. The dependency injection via constructor references is appropriate, and the interface is well-defined.src/governance/net_governance.cpp (2)
22-54: Well-structured scheduling with appropriate guards.The scheduling logic properly:
- Checks
ShutdownRequested()before processing ticks- Conditionally schedules housekeeping only when governance is valid
- Uses different timing for test vs. production networks
345-448: Governance message handling properly migrated.The
ProcessMessagemethod correctly handles all three governance message types with appropriate validation and peer misbehavior scoring. TheProcessObjectcall at line 393 properly acquirescs_mainviaWITH_LOCKas required by the method's lock annotations.src/governance/governance.cpp (2)
449-455: Efficient swap-based inventory transfer.Using
swapto transfer the relay inventory avoids unnecessary copying while atomically clearing the source vector under the lock.
1174-1188: Good addition of orphan vote cleanup.The orphan vote cleanup properly removes expired entries before requesting objects, preventing accumulation of stale data. The iterator handling correctly saves the current position before advancing.
src/governance/governance.h (1)
367-388: Well-organized public API expansion for NetGovernance.The new public methods are properly grouped with a clarifying comment and have consistent lock annotations that match their implementations. The separation of governance networking concerns into
NetGovernancewhile exposing necessary accessors maintains good encapsulation.
It is used to show this warning:
Transaction index can't be disabled with governance validation enabled. Either start with -disablegovernance command line switch or enable transaction index.
New message is shown in this situation now; behavior is changed by forcing disabling governance:
Warning: You are starting with governance validation disabled.
…de/sync and governance/governance
…d code from governance
…uestOrphanObjects It helps to manage amount of public methods of CGovernanceManager
…relevant code to NetGovernance Please note that circular dependency masternode/sync -> net -> masternode/sync existed for a long time, it's not newly introduced
…dependency governance on net_processing
knst
force-pushed
the
refactor-peermanager-handlers-gov
branch
from
094a3f6 to
a5ef0da
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/governance/governance.cpp (1)
183-231: Clarify locking expectations forProcessObject(cs_main precondition).
ProcessObjectassumes::cs_mainis held by the caller (as done inNetGovernance::ProcessMessageviaWITH_LOCK(::cs_main, ...)) but only lockscs_storelocally. This works with current call sites and keepsAddGovernanceObjectInternal’s lock assertions satisfied, but nothing in this method enforces thecs_mainprecondition.To future‑proof against accidental misuse, consider adding an
AssertLockHeld(::cs_main);at the top ofProcessObjectand documenting that it must be called withcs_mainheld. Behavior stays identical, but incorrect future call sites will fail fast in debug builds.src/instantsend/net_instantsend.cpp (1)
17-65: ProcessMessage signature alignment looks good.Updating
NetInstantSend::ProcessMessageto takeCConnman&matches the newNetHandlerinterface andPeerManagerdispatch. The unused parameter is fine; if compiler warnings become noisy, you can mark it[[maybe_unused]]or name it and(void)connman;, but that’s optional.src/governance/governance.h (1)
389-390: Minor: Extra blank line.There's an extra blank line before the
private:section that could be removed for consistency.bool ProcessVote(CNode* pfrom, const CGovernanceVote& vote, CGovernanceException& exception, CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(!cs_store); - private:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/Makefile.am(2 hunks)src/governance/governance.cpp(9 hunks)src/governance/governance.h(1 hunks)src/governance/net_governance.cpp(1 hunks)src/governance/net_governance.h(1 hunks)src/init.cpp(3 hunks)src/instantsend/net_instantsend.cpp(1 hunks)src/instantsend/net_instantsend.h(1 hunks)src/masternode/sync.cpp(1 hunks)src/masternode/sync.h(2 hunks)src/net_processing.cpp(5 hunks)src/net_processing.h(3 hunks)src/node/chainstate.cpp(0 hunks)src/node/chainstate.h(0 hunks)src/test/util/setup_common.cpp(0 hunks)src/validation.h(0 hunks)test/lint/lint-circular-dependencies.py(2 hunks)
💤 Files with no reviewable changes (4)
- src/node/chainstate.cpp
- src/node/chainstate.h
- src/test/util/setup_common.cpp
- src/validation.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/instantsend/net_instantsend.h
- src/Makefile.am
- src/masternode/sync.cpp
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/net_processing.hsrc/instantsend/net_instantsend.cppsrc/governance/net_governance.hsrc/governance/net_governance.cppsrc/init.cppsrc/governance/governance.cppsrc/governance/governance.hsrc/masternode/sync.hsrc/net_processing.cpp
src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Files:
src/governance/net_governance.hsrc/governance/net_governance.cppsrc/governance/governance.cppsrc/governance/governance.hsrc/masternode/sync.h
src/governance/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Files:
src/governance/net_governance.hsrc/governance/net_governance.cppsrc/governance/governance.cppsrc/governance/governance.h
src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Files:
src/governance/net_governance.hsrc/governance/net_governance.cppsrc/governance/governance.cppsrc/governance/governance.hsrc/masternode/sync.h
src/{masternode,evo}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/sync.h
src/{masternode,llmq}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
BLS integration must be used for cryptographic foundation of advanced masternode features
Files:
src/masternode/sync.h
🧠 Learnings (30)
📓 Common learnings
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: 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.
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: 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: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/net_processing.hsrc/governance/net_governance.cppsrc/governance/governance.cppsrc/governance/governance.h
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket operations are properly synchronized using the cs_net mutex. The m_sock and m_server members are GUARDED_BY(cs_net), and methods like Connect(), SendDirectly(), and ReconnectThread() use appropriate locking with cs_net to prevent race conditions on socket access.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access requires synchronization using the cs_net mutex. The race condition exists between SendDirectly() (callable from any thread) and Connect() (called by ReconnectThread), where both read and write m_sock concurrently. The cs_net mutex properly coordinates these network operations.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/instantsend/net_instantsend.cppsrc/governance/governance.hsrc/net_processing.cpp
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access is protected by the cs_net mutex. This mutex coordinates between the TCP send path in SendDirectly() and the reconnection operations in Connect()/Reconnect() methods, ensuring proper synchronization of socket state.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket access operations (m_sock and m_server) should be protected by the cs_net mutex, not avoiding synchronization entirely. While lock overhead concerns are valid in general, socket operations require proper synchronization via cs_net.
Applied to files:
src/instantsend/net_instantsend.cppsrc/net_processing.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Applied to files:
src/governance/net_governance.hsrc/governance/net_governance.cppsrc/init.cppsrc/governance/governance.cppsrc/governance/governance.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/init.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Applied to files:
src/init.cppsrc/governance/governance.cppsrc/governance/governance.hsrc/masternode/sync.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
src/init.cppsrc/governance/governance.hsrc/masternode/sync.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/**/*.{cpp,h} : Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Applied to files:
src/init.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
src/init.cppsrc/governance/governance.cppsrc/masternode/sync.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Applied to files:
src/init.cpp
📚 Learning: 2025-10-28T08:54:00.392Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6926
File: test/functional/feature_governance_cl.py:0-0
Timestamp: 2025-10-28T08:54:00.392Z
Learning: In Dash tests, the scheduler (mockscheduler) operates independently of network state. Isolated nodes should still run scheduler-based cleanup processes like governance cleanup, even if they have different state due to network isolation.
Applied to files:
src/init.cppsrc/governance/governance.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/governance/governance.cppsrc/masternode/sync.h
📚 Learning: 2025-11-25T10:53:37.523Z
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.523Z
Learning: The file src/masternode/sync.h containing `CMasternodeSync` is misnamed and misplaced—it has nothing to do with "masternode" functionality. It should eventually be renamed to `NodeSyncing` or `NodeSyncStatus` and moved to src/node/sync.h as a future refactoring.
Applied to files:
src/governance/governance.cppsrc/masternode/sync.h
📚 Learning: 2025-11-25T10:53:37.523Z
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.523Z
Learning: In Dash Core, `DEFAULT_SYNC_MEMPOOL` belongs with node synchronization logic (currently in src/masternode/sync.h) rather than with validation logic in src/validation.h, even though other mempool-related constants are in validation.h. The constant is conceptually tied to sync functionality, not validation.
Applied to files:
src/governance/governance.cppsrc/masternode/sync.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 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
📚 Learning: 2025-11-13T20:02:55.480Z
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.).
Applied to files:
src/governance/governance.h
📚 Learning: 2025-11-04T18:23:28.175Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/governance/classes.cpp:147-154
Timestamp: 2025-11-04T18:23:28.175Z
Learning: In src/governance/classes.cpp, CSuperblock::GetPaymentsLimit intentionally uses synthetic difficulty constants (nBits = 1 for mainnet, powLimit for networks with min difficulty) and simple height-based V20 activation checks instead of actual chain block data. This is by design because superblocks themselves are "synthetic" governance payment blocks, not regular mined blocks.
Applied to files:
src/governance/governance.hsrc/masternode/sync.h
📚 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/sync.hsrc/net_processing.cpp
📚 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/sync.hsrc/net_processing.cpp
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
src/masternode/sync.h
📚 Learning: 2025-07-29T14:32:48.369Z
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.
Applied to files:
src/masternode/sync.hsrc/net_processing.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h} : Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Applied to files:
src/masternode/sync.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/masternode/sync.h
📚 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/net_processing.cpp
📚 Learning: 2025-07-29T14:33:01.040Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.
Applied to files:
src/net_processing.cpp
🧬 Code graph analysis (7)
src/net_processing.h (1)
src/net_processing.cpp (36)
PeerGetRequestedObjectCount(6574-6577)PeerGetRequestedObjectCount(6574-6574)nodeid(628-628)nodeid(631-631)nodeid(644-644)nodeid(656-656)nodeid(661-661)nodeid(679-679)nodeid(681-681)nodeid(719-720)nodeid(730-730)nodeid(862-862)nodeid(864-864)nodeid(1040-1040)nodeid(1075-1075)PeerPostProcessMessage(6579-6582)PeerPostProcessMessage(6579-6579)ret(662-662)ret(685-685)scheduler(624-624)scheduler(652-652)scheduler(694-694)ProcessMessage(3681-5471)ProcessMessage(3681-3686)pfrom(618-618)pfrom(639-640)pfrom(753-755)pfrom(764-764)pfrom(772-772)pfrom(775-775)pfrom(777-777)pfrom(779-779)pfrom(875-875)pfrom(1057-1057)ScheduleHandlers(1701-1706)ScheduleHandlers(1701-1701)
src/instantsend/net_instantsend.cpp (1)
src/net_processing.cpp (12)
ProcessMessage(3681-5471)ProcessMessage(3681-3686)pfrom(618-618)pfrom(639-640)pfrom(753-755)pfrom(764-764)pfrom(772-772)pfrom(775-775)pfrom(777-777)pfrom(779-779)pfrom(875-875)pfrom(1057-1057)
src/governance/net_governance.h (2)
src/net_processing.h (3)
NetHandler(71-87)NetHandler(75-78)PeerManagerInternal(58-69)src/governance/net_governance.cpp (10)
Schedule(22-54)Schedule(22-22)ProcessMessage(345-449)ProcessMessage(345-345)SendGovernanceSyncRequest(56-61)SendGovernanceSyncRequest(56-56)RequestGovernanceObjectVotes(63-152)RequestGovernanceObjectVotes(63-63)ProcessTick(154-343)ProcessTick(154-154)
src/governance/net_governance.cpp (1)
src/net.h (1)
FullyConnectedOnly(1296-1296)
src/init.cpp (5)
src/net_processing.cpp (10)
node(616-616)node(617-617)node(642-642)node(757-757)node(792-792)node(892-896)node(907-907)node(918-918)node(929-929)node(937-937)src/interfaces/node.h (2)
node(42-44)node(404-404)src/kernel/coinstats.h (1)
node(19-21)src/rpc/blockchain.h (1)
node(33-36)src/init.h (1)
node(23-25)
src/governance/governance.cpp (1)
src/governance/governance.h (1)
nCachedBlockHeight(261-261)
src/masternode/sync.h (1)
src/masternode/sync.cpp (6)
BumpAssetLastTime(43-48)BumpAssetLastTime(43-43)GetAssetName(50-59)GetAssetName(50-50)GetSyncStatus(89-97)GetSyncStatus(89-89)
🔇 Additional comments (15)
src/net_processing.h (1)
58-69: Handler and peer APIs are consistent with the new scheduling/message flow.The additions to
PeerManagerInternal(PeerGetRequestedObjectCount,PeerPostProcessMessage),NetHandler(Schedule,ProcessMessagewithCConnman&), andPeerManager::ScheduleHandlersmatch the pattern implemented inPeerManagerImpland used byNetGovernance/NetInstantSend. The abstractions keep network plumbing insidePeerManagerwhile letting individual handlers own their scheduling and message handling.Also applies to: 71-87, 155-175
src/governance/net_governance.h (1)
14-37: NetGovernance handler design matches the refactoring goal.
NetGovernanceencapsulates governance P2P logic behind the genericNetHandlerinterface and is wired via constructor injection into the existing managers, which is exactly what’s needed to decouple governance consensus state fromPeerManager. TheSchedule/ProcessMessageoverrides match the base class and the implementation innet_governance.cpp.src/init.cpp (2)
88-92: NetGovernance attachment and gating look correct.
NetGovernanceis always added as an extra handler, butis_governance_enabledis still respected by only loading the governance cache when enabled, and the handler’s implementation guards work onm_gov_manager.IsValid()/ sync state. That keeps the wiring simple ininit.cppwhile avoiding governance network activity (and DB usage) when validation is disabled.Also applies to: 1690-1691, 2212-2229
2292-2302: Scheduler integration viaScheduleHandlersis a clean generalization.Calling
node.peerman->ScheduleHandlers(*node.scheduler);after registering all handlers (InstantSend, NetGovernance, etc.) centralizes their periodic work (governance cleanup, orphan requests, relay batching, etc.) behind the NetHandler abstraction instead of hard-coding per-subsystem schedule blocks ininit.cpp. The order relative to LLQM/masternode scheduling and the shutdown sequence is consistent with existing patterns.test/lint/lint-circular-dependencies.py (1)
41-53: Updated circular-dependency expectations are consistent with the new handler wiring.The added cycles capturing
core_io → evo/mnhftx → llmq/signing → net_processing → governance/* → core_ioandmasternode/sync → net → masternode/syncreflect the new NetGovernance/handler paths and sync wiring without changing the linter logic. The whitelist stays aligned with the current architecture.src/masternode/sync.h (1)
17-18: Sync constants and CMasternodeSync accessors are consistent with current design.Defining
DEFAULT_SYNC_MEMPOOLalongside other sync-related constants and exposingBumpAttempt(),GetLastBump(), andIsReachedBestHeader()mirrors the existing implementation insync.cppand keeps node-sync status queryable without changing behavior. Placement of the constant here matches the intended association with node synchronization rather than validation.Also applies to: 65-74
src/governance/governance.cpp (1)
449-455: LGTM on new relay/vote fetch helpers and orphan‑vote cleanup.
FetchRelayInventory()correctly batches and drainsm_relay_invsundercs_relay, called within scheduler-driven relay path inNetGovernance.FetchGovernanceObjectVotes()safely co-operates with externalNetGovernancecode by cleaning expiredmapAskedRecentlyentries using the advance-then-erase pattern while holdingcs_store, and honoringnPeersPerHashMaxbefore processing objects.RequestOrphanObjects()'s pruning loop overcmmapOrphanVotes.GetItemList()correctly removes expired orphan votes using the advance-then-erase pattern to avoid iterator invalidation before requesting.These changes improve separation between network handler code and governance state while maintaining scheduler-driven, network-decoupled voting architecture, without altering consensus behavior.
src/governance/net_governance.cpp (4)
22-54: Scheduler setup looks good; consider lifetime assumptions of captured referencesThe three scheduled tasks mirror the expected 1s tick plus slower housekeeping and relay pacing, and the
IsValid()gate on the heavier governance maintenance/relay work makes sense.One thing to double‑check:
thisand&connmanare captured by reference in lambdas that may outliveNetGovernanceif shutdown/teardown ordering ever changes. IfScheduleis called once at init andNetGovernance/CConnmanboth live until scheduler shutdown, this is fine; if not, you may want a stop hook or a weak/indirection pattern.
56-61: Governance sync request wiring is straightforward
SendGovernanceSyncRequest’s construction ofMNGOVERNANCESYNCwith an empty Bloom filter and a zero hash matches the expectations inProcessMessageand keeps the wire encoding simple. No issues here.
63-152: Vote request throttling and per‑hash/per‑peer limits look consistentThe logic in
RequestGovernanceObjectVotes(per‑hash peer caps, projected vote count limit per peer,MAX_INV_SZguard, and staticmapAskedRecentlywith a mutex) is coherent and appears to preserve the intended behavior of avoiding bandwidth explosions while still stressing non‑main networks more heavily.Assuming
FetchGovernanceObjectVotesis responsible for aging outmapAskedRecentlyentries based onnNowandnTimeout, this design should remain bounded over long uptimes. If that assumption changed in the refactor, it’s worth confirming to avoid unbounded growth ofmapAskedRecently.
345-449: ProcessMessage refactor preserves structure; minor duplication onlyThe three branches for
MNGOVERNANCESYNC,MNGOVERNANCEOBJECT, andMNGOVERNANCEOBJECTVOTElook like a direct lift from the old governance message handler, with the right hand‑offs toCGovernanceManagerandPeerManagerand appropriate sync gating and misbehavior scoring.There is some duplicated
IsBlockchainSynced()checking (top of function plus inside the object/vote branches), but that’s harmless and may even be desirable given the concurrency between sync and net threads. No changes needed here.src/net_processing.cpp (3)
652-652: NetHandler scheduling hook looks sound
ScheduleHandlerscleanly iteratesm_handlersand delegates tohandler->Schedule(scheduler, m_connman), matching the new interface and keeping scheduling centralized inPeerManagerImpl. No locking is needed as long asm_handlersis configured only during init/shutdown, which matches existingAddExtraHandler/RemoveHandlersusage.Also applies to: 1701-1706
661-661: Locking contract forPeerGetRequestedObjectCountnow matches internal state accessRequiring both
!m_peer_mutexand::cs_mainonPeerGetRequestedObjectCountaligns the wrapper withGetRequestedObjectCountand them_node_states(GUARDED_BY(cs_main)) access. The wrapper’s implementation is a straight passthrough, which is correct given the updated precondition; callers just need to ensure they now holdcs_mainwhen using the peer-facing variant.Also applies to: 1650-1659, 6574-6577
5461-5463: PassingCConnman&into NetHandlers is appropriateUpdating the NetHandler dispatch to
handler->ProcessMessage(pfrom, m_connman, msg_type, vRecv);matches the expanded handler API and keeps all P2P sends/connection operations going through the sharedCConnmaninstance. This is consistent with the refactoring goal of separating network orchestration from handler logic.src/governance/governance.h (1)
367-388: Well-structured API boundary for the network/consensus separation.The new "Used by NetGovernance" section clearly delineates the methods needed by the new
NetGovernancehandler. Lock annotations are consistently applied:
cs_relayfor relay inventory operationscs_storefor governance object/vote storage operationscs_mainappropriately added toProcessObjectfor chain-state accessThe design correctly exposes data-fetching methods (
FetchRelayInventory,FetchGovernanceObjectVotes) and processing entry points (ProcessObject,ProcessVote) that allowNetGovernanceto handle network messaging whileCGovernanceManagerfocuses on consensus logic.
GetMNManager()(line 385) returning a reference without lock annotations is correct sincem_dmnmanis a const member initialized at construction.
| if (gArgs.GetBoolArg("-syncmempool", DEFAULT_SYNC_MEMPOOL)) { | ||
| // Now that the blockchain is synced request the mempool from the connected outbound nodes if possible | ||
| for (auto pNodeTmp : snap.Nodes()) { | ||
| bool fRequestedEarlier = m_netfulfilledman.HasFulfilledRequest(pNodeTmp->addr, | ||
| "mempool-sync"); | ||
| if (!pNodeTmp->IsInboundConn() && !fRequestedEarlier && !pNodeTmp->IsBlockRelayOnly()) { | ||
| m_netfulfilledman.AddFulfilledRequest(pNodeTmp->addr, "mempool-sync"); | ||
| connman.PushMessage(pNodeTmp, msgMaker.Make(NetMsgType::MEMPOOL)); | ||
| LogPrint(BCLog::MNSYNC, /* Continued */ | ||
| "Sync Tick -- nTick %d asset_id %d -- syncing mempool from peer=%d\n", nTick, | ||
| asset_id, pNodeTmp->GetId()); | ||
| } |
There was a problem hiding this comment.
BUG: MEMPOOL sync uses CNetMsgMaker from the wrong peer
In the mempool sync block you send MEMPOOL to pNodeTmp using a CNetMsgMaker constructed from pnode->GetCommonVersion():
CNetMsgMaker msgMaker(pnode->GetCommonVersion());
// ...
for (auto pNodeTmp : snap.Nodes()) {
// ...
connman.PushMessage(pNodeTmp, msgMaker.Make(NetMsgType::MEMPOOL));
}CNetMsgMaker bakes in the per‑peer protocol version; reusing a maker built for one peer when sending to another is incorrect and can lead to mismatched serialization if peers negotiate different versions.
You should construct the message maker using pNodeTmp’s negotiated version instead:
- for (auto pNodeTmp : snap.Nodes()) {
+ for (auto pNodeTmp : snap.Nodes()) {
bool fRequestedEarlier = m_netfulfilledman.HasFulfilledRequest(pNodeTmp->addr,
"mempool-sync");
if (!pNodeTmp->IsInboundConn() && !fRequestedEarlier && !pNodeTmp->IsBlockRelayOnly()) {
m_netfulfilledman.AddFulfilledRequest(pNodeTmp->addr, "mempool-sync");
- connman.PushMessage(pNodeTmp, msgMaker.Make(NetMsgType::MEMPOOL));
+ CNetMsgMaker mempoolMsgMaker(pNodeTmp->GetCommonVersion());
+ connman.PushMessage(pNodeTmp, mempoolMsgMaker.Make(NetMsgType::MEMPOOL));
LogPrint(BCLog::MNSYNC, /* Continued */
"Sync Tick -- nTick %d asset_id %d -- syncing mempool from peer=%d\n", nTick,
asset_id, pNodeTmp->GetId());
}
}This keeps behavior aligned with the rest of the codebase, where messages are always serialized with the destination peer’s own common version.
🤖 Prompt for AI Agents
In src/governance/net_governance.cpp around lines 246-257, the MEMPOOL message
is being created with a CNetMsgMaker built from the wrong peer (pnode) which
bakes in an incorrect protocol version; fix it by constructing the CNetMsgMaker
with the destination peer's negotiated version (pNodeTmp->GetCommonVersion())
when sending inside the loop (i.e. create a msgMaker per pNodeTmp or call
CNetMsgMaker(pNodeTmp->GetCommonVersion()) before connman.PushMessage), so each
MEMPOOL message is serialized using the target peer's common version.
| void PeerRelayInvFiltered(const CInv& inv, const uint256& relatedTxHash) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); | ||
| void PeerAskPeersForTransaction(const uint256& txid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); | ||
| size_t PeerGetRequestedObjectCount(NodeId nodeid) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, ::cs_main); | ||
| void PeerPostProcessMessage(MessageProcessingResult&& ret) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); |
There was a problem hiding this comment.
PeerPostProcessMessage drops peer context, disabling misbehavior and request cleanup
PeerPostProcessMessage currently forwards to:
void PeerManagerImpl::PeerPostProcessMessage(MessageProcessingResult&& ret)
{
PostProcessMessage(std::move(ret), -1);
}PostProcessMessage uses the node argument to:
- Attribute
m_errorto a specific peer viaMisbehaving(node, ...). - Call
EraseObjectRequest(node, ...)undercs_main.
Passing -1 means any m_error or m_to_erase set by callers of PeerPostProcessMessage will be silently ignored: Misbehaving(-1, ...) and EraseObjectRequest(-1, ...) won’t find a peer/state entry. Only m_transactions, m_inventory, and m_dsq are effective.
If NetHandlers are expected to use PeerPostProcessMessage with a real per-peer MessageProcessingResult (which the name strongly suggests), this will undercut DoS accounting and object-request bookkeeping for those subsystems.
Consider tightening this by threading the actual NodeId through the interface, e.g.:
-void PeerPostProcessMessage(MessageProcessingResult&& ret) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
+void PeerPostProcessMessage(MessageProcessingResult&& ret, NodeId node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);and
-void PeerManagerImpl::PeerPostProcessMessage(MessageProcessingResult&& ret)
-{
- PostProcessMessage(std::move(ret), -1);
-}
+void PeerManagerImpl::PeerPostProcessMessage(MessageProcessingResult&& ret, NodeId node)
+{
+ PostProcessMessage(std::move(ret), node);
+}with corresponding updates to PeerManagerInternal and NetGovernance (or other) call sites so they pass pfrom.GetId() when there is a peer context, and only use -1 explicitly in the rare cases where there truly is none.
Also applies to: 3587-3604, 6579-6582
🤖 Prompt for AI Agents
In src/net_processing.cpp around line 662 (and similarly at 3587-3604 and
6579-6582), PeerPostProcessMessage currently calls PostProcessMessage with -1
which drops peer context and prevents Misbehaving(...) and
EraseObjectRequest(...) from attributing errors/cleanup to the correct peer;
change the PeerPostProcessMessage signature to accept a NodeId (or int node) and
forward that node to PostProcessMessage instead of -1, update the
PeerManagerInternal and NetGovernance (and any other callers) to pass
pfrom.GetId() when a peer exists (or -1 only when there truly is no peer), and
update the override/declaration sites accordingly so m_error and m_to_erase are
handled against the correct peer state under cs_main.
|
it conflicts with #6992 but conflicts are pretty trivial. Whatever which one will get merged first; 2nd will be rebased on the top. |
1 participant
Issue being fixed or feature implemented
Separation of consensus & chain code and network & node in Dash Core is blocked by tight connection of network and consensus code; in components such as
llmq::CSigningManager,llmq::CSigSharesManager,coinjoin::client,coinjoin::server,governance/,llmq::CInstantSendManager, etc.It blocks backport's of bitcoin related to 'kernel' project.
What was done?
This PR addresses a dependency of
CGovernanceManageronPeerManagerIt is a split from proof-of-concept PR #6934
Prior work: #6959 #6992
How Has This Been Tested?
Run unit & functional tests.
Run a linter
test/lint/lint-circular-dependencies.pyOverall conception is tested in #6934 as potentially successful to finish separation of network & consensus code.
Breaking Changes
N/A
Checklist: