refactor: break some circular dependencies over coinjoin/server #7070
Conversation
|
WalkthroughThe CCoinJoinServer was converted into a NetHandler-backed component: its constructor now accepts PeerManagerInternal* and replaces PeerManager usage with m_peer_manager. ProcessMessage and ProcessDSQUEUE now return void and use m_peer_manager for relays and misbehavior/erase calls. DoMaintenance was renamed to Schedule(CScheduler&) and periodic execution moved to a scheduler. New NetHandler overrides AlreadyHave and ProcessGetData were added. ActiveContext and init wiring were updated to construct and bind a CCoinJoinServer instance; PeerManagerInternal and net_processing gained DSQ/transaction relay hooks and external-handler invocation points. Sequence Diagram(s)sequenceDiagram
autonumber
participant Init as init.cpp
participant PMI as PeerManagerInternal / PeerManagerImpl
participant CJS as CCoinJoinServer (NetHandler)
participant AC as ActiveContext
participant NP as net_processing (handler registry)
participant Conn as CConnman
Init->>PMI: provide PeerManagerInternal*
Init->>CJS: construct CCoinJoinServer(peer_manager, ...)
Init->>AC: construct ActiveContext(..., CCoinJoinServer&)
AC->>CJS: bind m_cj_server reference
Init->>NP: AddExtraHandler(CJS)
NP->>CJS: incoming message -> ProcessMessage(msg)
CJS->>PMI: RelayDSQ / RelayInv / PeerMisbehaving / PeerEraseObjectRequest
PMI->>Conn: send relayed network messages
NP->>CJS: query AlreadyHave / ProcessGetData when handling getdata
CJS->>NP: trigger PeerRelayDSQ / PeerRelayTransaction via PeerManagerInternal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/coinjoin/server.cpp (1)
48-61: Remove redundant return statement.Line 55 uses
return ProcessDSQUEUE(...)butProcessDSQUEUEnow returnsvoid(line 129). Thereturnkeyword is redundant and should be removed for clarity.Apply this diff:
} else if (msg_type == NetMsgType::DSQUEUE) { - return ProcessDSQUEUE(peer.GetId(), vRecv); + ProcessDSQUEUE(peer.GetId(), vRecv); } else if (msg_type == NetMsgType::DSVIN) {
🧹 Nitpick comments (1)
src/net_processing.cpp (1)
2353-2372: DSQAlreadyHavedelegation to CJWalletManager and NetHandlers looks consistentThe DSQ branch now:
- Checks
m_cj_walletman->hasQueue()first, and- Falls through to
m_handlersviahandler->AlreadyHave(inv)returningstd::optional<bool>.This preserves the “first handler that knows, decides” semantics and cleanly routes DSQ knowledge to CoinJoin-related components without re-introducing direct coinjoin/server dependencies. Returning
falsewhen no handler claims the object is appropriate so DSQ can be requested when relevant. Just ensure NetHandler implementations returnstd::nullopt(notfalse) for unrelatedinv.types so they don’t accidentally suppress other handlers.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 123ee5e and 7d821b43070a22e8c69fa921ab0264841384361a.
📒 Files selected for processing (9)
src/coinjoin/server.cpp(30 hunks)src/coinjoin/server.h(4 hunks)src/init.cpp(1 hunks)src/masternode/active/context.cpp(1 hunks)src/masternode/active/context.h(2 hunks)src/net_processing.cpp(6 hunks)src/net_processing.h(3 hunks)src/rpc/coinjoin.cpp(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
🧰 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/rpc/coinjoin.cppsrc/masternode/active/context.hsrc/init.cppsrc/net_processing.hsrc/coinjoin/server.cppsrc/net_processing.cppsrc/coinjoin/server.hsrc/masternode/active/context.cpp
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/active/context.hsrc/masternode/active/context.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/masternode/active/context.hsrc/coinjoin/server.cppsrc/coinjoin/server.hsrc/masternode/active/context.cpp
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/active/context.hsrc/masternode/active/context.cpp
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/active/context.hsrc/coinjoin/server.cppsrc/coinjoin/server.hsrc/masternode/active/context.cpp
src/coinjoin/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Files:
src/coinjoin/server.cppsrc/coinjoin/server.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: 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: 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.
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
📚 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/rpc/coinjoin.cpptest/lint/lint-circular-dependencies.pysrc/masternode/active/context.hsrc/init.cppsrc/net_processing.hsrc/coinjoin/server.cppsrc/net_processing.cppsrc/coinjoin/server.hsrc/masternode/active/context.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: In src/net_processing.cpp, if ActiveContext (m_active_ctx) is non-null, its members (including cj_server) are guaranteed to be initialized; call sites can safely dereference m_active_ctx->cj_server without an additional null-check.
Applied to files:
src/rpc/coinjoin.cppsrc/masternode/active/context.hsrc/init.cppsrc/masternode/active/context.cpp
📚 Learning: 2025-10-03T11:20:37.475Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/active/context.cpp:29-33
Timestamp: 2025-10-03T11:20:37.475Z
Learning: In Dash codebase, NodeContext (src/node/context.h) serves only as a container with trivial c/d-tors; member lifetime is explicitly managed via reset() calls in the shutdown sequence (src/init.cpp), not by declaration order. For example, active_ctx.reset() is called before DashChainstateSetupClose handles llmq_ctx teardown, ensuring proper destruction order regardless of declaration order in the struct.
Applied to files:
src/masternode/active/context.hsrc/init.cppsrc/masternode/active/context.cpp
📚 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/masternode/active/context.hsrc/coinjoin/server.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/masternode/active/context.hsrc/net_processing.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/active/context.hsrc/init.cppsrc/net_processing.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}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
src/masternode/active/context.hsrc/init.cppsrc/net_processing.hsrc/coinjoin/server.cppsrc/net_processing.cppsrc/coinjoin/server.hsrc/masternode/active/context.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/masternode/active/context.hsrc/masternode/active/context.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/masternode/active/context.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}/**/*.{cpp,h} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/masternode/active/context.h
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/masternode/active/context.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/masternode/active/context.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/**/*.{cpp,h} : NodeContext must be extended with Dash-specific managers for each subsystem during initialization
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/{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/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/{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/net_processing.hsrc/net_processing.cppsrc/masternode/active/context.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} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Applied to files:
src/net_processing.cppsrc/masternode/active/context.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/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/net_processing.cppsrc/masternode/active/context.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/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/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/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/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/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Applied to files:
src/net_processing.cppsrc/masternode/active/context.cpp
🧬 Code graph analysis (1)
src/coinjoin/server.h (3)
src/coinjoin/server.cpp (9)
CCoinJoinServer(27-44)CCoinJoinServer(46-46)ProcessGetData(953-962)ProcessGetData(953-953)inv(372-372)AlreadyHave(947-951)AlreadyHave(947-947)Schedule(923-935)Schedule(923-923)src/coinjoin/coinjoin.h (1)
CCoinJoinBaseManager(322-351)src/net_processing.h (3)
NetHandler(73-97)NetHandler(77-80)std(91-91)
🪛 Cppcheck (2.18.0)
src/masternode/active/context.cpp
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (22)
src/net_processing.h (3)
28-28: LGTM!Forward declaration is appropriately added for the
ProcessGetDatamethod signature.
66-67: Interface extension properly supports dependency breaking.The pure virtual methods
PeerRelayTransaction()andPeerRelayDSQ()appropriately extendPeerManagerInternalto provide relay functionality for CoinJoin components without requiring the fullPeerManagerinterface. Both methods are correctly implemented inPeerManagerImplwith proper lock annotations, allowingCCoinJoinServerto depend on the narrowerPeerManagerInternal*interface instead of a fullPeerManager&dependency.
86-94: [rewritten review comment text]
[classification tag]src/net_processing.cpp (4)
44-46: CoinJoin include switch to shared header looks fineSwitching to
<coinjoin/coinjoin.h>cleanly decouples net_processing from the server-specific header and aligns with the new NetHandler-based integration. No issues spotted.
2986-3003: DSQ getdata handling and NetHandler fallback are correctly orderedFor
MSG_DSQ:
- You first consult
m_cj_walletman->getQueueFromHash()and, if present, respond withDSQUEUE.- Only if
pushis still false do you iteratem_handlersand let external NetHandlers attemptProcessGetData.This avoids double responses, keeps wallet-driven DSQ serving as a fast path, and lets CoinJoin server (or other handlers) take over when the wallet has no queue entry. The generic
m_handlersloop for anyinvtype is also guarded viapushso only one handler responds. No locking or flow-control issues seen.
649-656: NewPeerRelayDSQ/PeerRelayTransactionhooks match existing relay contractsAdding
PeerRelayDSQandPeerRelayTransactionto the PeerManagerInternal interface, with the same lock annotations as the existing relay methods, gives NetHandlers a clear, symmetric way to trigger DSQ and tx relay without coupling to concretePeerManagerImpl. The signatures and annotations are consistent withRelayDSQ/RelayTransaction.
6584-6592: Thin forwarding implementations for PeerRelay keep behaviour centralized*
PeerRelayDSQandPeerRelayTransactionsimply forward toRelayDSQ(queue)andRelayTransaction(txid), so all relay behaviour (including filters, DSQ preference handling, etc.) remains in the existing methods. This avoids duplication and ensures external NetHandlers use the same relay path as core code.src/coinjoin/server.h (2)
28-28: LGTM: NetHandler inheritance enables dependency decoupling.The addition of
NetHandleras a base class is the key change that breaks the circular dependency by allowingCCoinJoinServerto be managed through a polymorphic interface in net_processing without direct coupling.
97-106: Constructor and interface changes are consistent with NetHandler.The constructor signature taking
PeerManagerInternal*and the virtual method overrides (ProcessMessage,ProcessGetData,AlreadyHave,Schedule) correctly implement theNetHandlerinterface as shown in the relevant code snippets fromsrc/net_processing.h.src/rpc/coinjoin.cpp (1)
468-468: LGTM: Access pattern updated for reference member.The change from pointer dereference (
cj_server->) to reference access (m_cj_server.) correctly reflects the updatedActiveContextmember type. Based on learnings, whennode.active_ctxis non-null, its members are guaranteed to be initialized, so this access is safe.test/lint/lint-circular-dependencies.py (1)
35-35: Circular dependency changes align with PR objectives.The newly expected cycle
coinjoin/coinjoin -> ... -> net_processing -> coinjoin/coinjoinreflects the architectural change where CoinJoin server functionality is now integrated via theNetHandlerinterface. This successfully removes thecoinjoin/server -> net_processingcycles mentioned in the PR objectives.src/masternode/active/context.h (2)
43-46: Constructor signature updated to accept CCoinJoinServer reference.The removal of
dstxmanandmn_metamanparameters and addition ofCCoinJoinServer& cj_serveraligns with the refactoring whereCCoinJoinServernow internally manages its own dependencies rather than having them passed throughActiveContext.
58-58: Changed from pointer to reference member for type safety.The change from
cj_serverpointer toCCoinJoinServer& m_cj_serverreference provides stronger guarantees: the member must be initialized at construction time and cannot be null. This aligns with the retrieved learning that "if ActiveContext is non-null, its members (including cj_server) are guaranteed to be initialized."src/init.cpp (2)
2202-2203: NetInstantSend and NetSigning handlers are correctly registered unconditionally.These handlers process network-wide messages (ISDLOCK and QSIGREC) that all nodes must receive and validate, not just masternodes. The isdlock message produced by the locking process is relayed to the entire Dash network. Regular nodes need these handlers to track locked transactions and maintain consensus state. Masternode-specific functionality (like lock creation) is properly isolated within the
if (node.mn_activeman)block via ActiveContext and CCoinJoinServer.
2205-2210: Correct the shutdown order concern: peerman (owning cj_server) is destroyed before active_ctx.The review correctly identifies a lifetime management issue. In
PrepareShutdown()at line 96,node.peerman.reset()destroys theCCoinJoinServerbefore line 140 wherenode.active_ctx.reset()destroys theActiveContextthat holds a reference to it.However, this is mitigated by the shutdown sequence: the RPC server is stopped at lines 63-66 (before
peerman.reset()), preventing any RPC calls from accessingm_cj_server. Additionally,ActiveContext::~ActiveContext()(lines 41-45) does not dereferencem_cj_server—it only disconnects signers viallmq_ctx.While the dangling reference is fragile design, it poses no practical safety issue given the current shutdown ordering and cleanup code. Consider reordering
peerman.reset()to occur afteractive_ctx.reset()(line 140) for defensive robustness.src/masternode/active/context.cpp (1)
19-25: LGTM! Clean dependency injection refactor.The constructor changes successfully break the circular dependency by accepting
CCoinJoinServer&as an external dependency rather than constructing it internally. The initialization is correct.src/coinjoin/server.cpp (6)
27-31: LGTM! Constructor aligns with NetHandler integration.The updated constructor correctly integrates
CCoinJoinServerinto theNetHandlerframework by acceptingPeerManagerInternal*and initializing the base class.
137-137: LGTM! Consistent peer manager integration.All usages of
m_peer_managermethods (PeerEraseObjectRequest,PeerMisbehaving,PeerRelayDSQ,PeerRelayInv,PeerRelayTransaction) are correctly migrated from the previousm_peermanmember, aligning with thePeerManagerInternalinterface.Also applies to: 142-142, 147-147, 156-156, 190-190, 206-206, 373-373, 485-485, 532-532, 753-753
923-935: LGTM! Clean scheduler integration.The
Schedulemethod correctly integrates withCSchedulerfor recurring maintenance tasks. The lambda capturesthissafely, includes appropriate sync and shutdown guards, and calls the expected maintenance methods on a one-second interval.
947-951: LGTM! Correct NetHandler integration.The
AlreadyHavemethod correctly implements theNetHandlerinterface for DSQ inventory handling, returningstd::nulloptfor non-DSQ types and delegating toHasQueuefor MSG_DSQ.
953-962: LGTM! Correct GetData handling.The
ProcessGetDatamethod correctly implements theNetHandlerinterface for DSQ GetData requests, returningfalsefor non-DSQ types or missing entries, and successfully responding with the DSQUEUE message when found.
276-277: Modern C++ pattern: inline variable declaration.The use of
if (int entries = GetEntriesCount(); entries != 0)is a clean C++17 init-statement pattern that limits the scope of theentriesvariable.
knst
force-pushed
the
refactor-peermanager-handlers-cj
branch
from
7d821b4 to
d88585d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/net_processing.cpp (1)
2353-2372: DSQAlreadyHaveNetHandler integration is sound; consider documenting “first handler wins”The new
MSG_DSQcase correctly:
- Short‑circuits via
m_cj_walletman->hasQueue()when available, and- Falls back to iterating
m_handlersand using the first handler that returns a non‑emptystd::optional<bool>.This achieves the goal of removing the direct
coinjoin/serverdependency while keepingAlreadyHavefast and side‑effect free.One thing to consider: right now the semantics are “first handler that knows this inv type decides”. If you ever introduce multiple handlers for the same inv type, later handlers will never be consulted. A short comment or assertion around handler registration (e.g., “at most one handler per inv type”) would make this contract explicit and avoid subtle future bugs.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 7d821b43070a22e8c69fa921ab0264841384361a and d88585d5b438817a04b146cce3034f117c7df50b.
📒 Files selected for processing (9)
src/coinjoin/server.cpp(18 hunks)src/coinjoin/server.h(4 hunks)src/init.cpp(1 hunks)src/masternode/active/context.cpp(1 hunks)src/masternode/active/context.h(2 hunks)src/net_processing.cpp(6 hunks)src/net_processing.h(3 hunks)src/rpc/coinjoin.cpp(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/net_processing.h
- src/rpc/coinjoin.cpp
- test/lint/lint-circular-dependencies.py
🧰 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/active/context.cppsrc/net_processing.cppsrc/coinjoin/server.hsrc/masternode/active/context.hsrc/init.cppsrc/coinjoin/server.cpp
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/active/context.cppsrc/masternode/active/context.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/active/context.cppsrc/coinjoin/server.hsrc/masternode/active/context.hsrc/coinjoin/server.cpp
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/active/context.cppsrc/masternode/active/context.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/active/context.cppsrc/coinjoin/server.hsrc/masternode/active/context.hsrc/coinjoin/server.cpp
src/coinjoin/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Files:
src/coinjoin/server.hsrc/coinjoin/server.cpp
🧠 Learnings (22)
📓 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: 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: 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.
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: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: 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
📚 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/active/context.cppsrc/net_processing.cppsrc/coinjoin/server.hsrc/masternode/active/context.hsrc/init.cppsrc/coinjoin/server.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: In src/net_processing.cpp, if ActiveContext (m_active_ctx) is non-null, its members (including cj_server) are guaranteed to be initialized; call sites can safely dereference m_active_ctx->cj_server without an additional null-check.
Applied to files:
src/masternode/active/context.cppsrc/masternode/active/context.hsrc/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/masternode/active/context.cppsrc/net_processing.cppsrc/coinjoin/server.hsrc/masternode/active/context.hsrc/init.cppsrc/coinjoin/server.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} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Applied to files:
src/masternode/active/context.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/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Applied to files:
src/masternode/active/context.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/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/masternode/active/context.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/masternode/active/context.cppsrc/masternode/active/context.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/masternode/active/context.cppsrc/net_processing.cppsrc/masternode/active/context.h
📚 Learning: 2025-10-03T11:20:37.475Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/active/context.cpp:29-33
Timestamp: 2025-10-03T11:20:37.475Z
Learning: In Dash codebase, NodeContext (src/node/context.h) serves only as a container with trivial c/d-tors; member lifetime is explicitly managed via reset() calls in the shutdown sequence (src/init.cpp), not by declaration order. For example, active_ctx.reset() is called before DashChainstateSetupClose handles llmq_ctx teardown, ensuring proper destruction order regardless of declaration order in the struct.
Applied to files:
src/masternode/active/context.cppsrc/masternode/active/context.hsrc/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/{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.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,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/net_processing.cppsrc/masternode/active/context.hsrc/init.cppsrc/coinjoin/server.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/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/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/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/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/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/active/context.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/masternode/active/context.hsrc/coinjoin/server.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/masternode/active/context.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}/**/*.{cpp,h} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/masternode/active/context.h
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/masternode/active/context.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/masternode/active/context.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/**/*.{cpp,h} : NodeContext must be extended with Dash-specific managers for each subsystem during initialization
Applied to files:
src/init.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/coinjoin/server.cpp
🪛 Cppcheck (2.18.0)
src/masternode/active/context.cpp
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
🔇 Additional comments (18)
src/masternode/active/context.cpp (1)
19-39: LGTM! Clean refactor to external CCoinJoinServer ownership.The constructor correctly accepts CCoinJoinServer by reference and initializes m_cj_server in the member initializer list. This cleanly breaks the circular dependency by externalizing server ownership.
src/masternode/active/context.h (1)
43-46: LGTM! Reference member correctly replaces unique_ptr ownership.The change from
const std::unique_ptr<CCoinJoinServer> cj_servertoCCoinJoinServer& m_cj_servercorrectly implements external ownership while maintaining access. Reference members must be initialized in the constructor (verified in context.cpp line 25) and can't be reassigned, which enforces the single-binding contract.The public reference allows external access (e.g., rpc/coinjoin.cpp) while clearly indicating ActiveContext doesn't own the server.
Also applies to: 58-58
src/init.cpp (1)
2202-2210: Verify lifetime safety: m_cj_server reference becomes dangling between peerman and active_ctx destruction.The CCoinJoinServer is created with a reference passed to ActiveContext, then ownership is moved to peerman via AddExtraHandler. This creates a subtle lifetime dependency:
- Line 2210: Ownership transferred to peerman's handler list
- PrepareShutdown line 325:
node.peerman.reset()destroys the CCoinJoinServer- PrepareShutdown line 369:
node.active_ctx.reset()destroys ActiveContext, which holds a now-dangling reference to m_cj_serverCurrently safe because ActiveContext destructor (context.cpp lines 41-45) doesn't access m_cj_server. However, this design is brittle—future modifications to the destructor could introduce use-after-free bugs.
Consider one of these solutions:
- Reset active_ctx before peerman in PrepareShutdown to make the dependency explicit
- Document this lifetime constraint clearly in ActiveContext
- Use a weak_ptr or optional reference to make the dangling reference explicit
⛔ Skipped due to learnings
Learnt from: kwvg Repo: dashpay/dash PR: 6840 File: src/net_processing.cpp:2882-2886 Timestamp: 2025-10-02T18:29:54.756Z Learning: In src/net_processing.cpp, if ActiveContext (m_active_ctx) is non-null, its members (including cj_server) are guaranteed to be initialized; call sites can safely dereference m_active_ctx->cj_server without an additional null-check.Learnt from: kwvg Repo: dashpay/dash PR: 6838 File: src/active/context.cpp:29-33 Timestamp: 2025-10-03T11:20:37.475Z Learning: In Dash codebase, NodeContext (src/node/context.h) serves only as a container with trivial c/d-tors; member lifetime is explicitly managed via reset() calls in the shutdown sequence (src/init.cpp), not by declaration order. For example, active_ctx.reset() is called before DashChainstateSetupClose handles llmq_ctx teardown, ensuring proper destruction order regardless of declaration order in the struct.Learnt from: kwvg Repo: dashpay/dash PR: 6840 File: src/net_processing.cpp:2882-2886 Timestamp: 2025-10-02T18:29:54.756Z Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.src/coinjoin/server.h (4)
10-10: LGTM: Include updated to support NetHandler integration.The switch from
msg_result.htonet_processing.haligns with the new NetHandler base class and the removal of MessageProcessingResult return types.
28-28: LGTM: NetHandler inheritance breaks circular dependencies.This inheritance is central to the PR's goal of breaking circular dependencies between coinjoin/server and net_processing by providing a well-defined interface for network message handling.
97-100: LGTM: Constructor signature updated for NetHandler integration.Adding
PeerManagerInternal* peer_manageras the first parameter enables NetHandler initialization and provides access to network processing functionality without creating circular dependencies.
103-106: LGTM: Public API updated to match NetHandler interface.The changes reflect the net-centric design:
- ProcessMessage now returns void with error handling via PeerMisbehaving
- ProcessGetData and AlreadyHave expose DSQ inventory handling
- Schedule replaces DoMaintenance for recurring tasks
src/coinjoin/server.cpp (7)
14-15: LGTM: Includes added for new functionality.The new includes support ProcessGetData (netmessagemaker.h) and Schedule (scheduler.h) methods.
27-31: LGTM: Constructor properly initializes NetHandler.The constructor correctly accepts
PeerManagerInternal* peer_managerand passes it to the NetHandler base class initializer, enabling network message handling without circular dependencies.
48-61: LGTM: ProcessMessage refactored to void return type.The method now handles errors inline via peer misbehaving calls instead of returning status codes. The early return pattern (line 50, 55) is appropriate for the new design.
129-143: LGTM: ProcessDSQUEUE refactored with early returns.The conversion to early returns for validation failures is consistent and improves readability. The use of
m_peer_managerfor misbehaving reports and relay operations is correct (note: line 188-206 has a separate critical issue).Also applies to: 144-187, 192-207
372-372: LGTM: Relay calls updated to use m_peer_manager.All relay operations (PeerRelayInv, PeerRelayTransaction, PeerRelayDSQ) now use
m_peer_managerinstead of the removedm_peerman, consistent with the NetHandler integration.Also applies to: 485-485, 530-530, 740-740
900-912: LGTM: Schedule method implements recurring maintenance tasks.The method correctly schedules CheckForCompleteQueue, CheckPool, and CheckTimeout on a 1-second interval with appropriate guards for blockchain sync status and shutdown requests.
924-939: LGTM: AlreadyHave and ProcessGetData expose DSQ inventory handling.These NetHandler overrides correctly:
- Filter for MSG_DSQ inventory type
- Delegate to existing queue management methods (HasQueue, GetQueueFromHash)
- Integrate DSQ handling into the network layer
src/net_processing.cpp (4)
44-46: Include swap tocoinjoin/coinjoin.hlooks appropriateSwitching to
#include <coinjoin/coinjoin.h>here is consistent with the new NetHandler‑based integration (CCoinJoin types, DSTX validation) and helps decouplenet_processingfrom the concrete server implementation. No issues from a dependency or layering perspective.
2986-3003: DSQGETDATAhandling refactor matches new abstraction; verify handler expectationsThe DSQ branch in
ProcessGetDatanow:
- Serves DSQ from
m_cj_walletman->getQueueFromHash()when available, and- Delegates to each
NetHandler::ProcessGetDatauntil one returnstrue, otherwise falls back to NOTFOUND.This is consistent with the new
AlreadyHavebehavior and keeps wallet/server responsibilities nicely separated. It also ensures peers don’t hang waiting for DSQ, since unhandled requests still land invNotFound.Just ensure that:
- NetHandler implementations of
ProcessGetDataare safe to call under this function’s locking preconditions (nocs_mainheld,peer.m_getdata_requests_mutexheld by caller), and- They return
trueiff they fully handled the request (including any responses or NOTFOUND), sopushreflects “request was served”.Functionally this looks correct and aligned with the PR’s intent.
655-657: New PeerManagerInternal relay hooks are minimal and consistentAdding
PeerRelayDSQandPeerRelayTransactiontoPeerManagerInternalgives NetHandlers an explicit way to trigger network relay without depending on concretePeerManagerImplinternals. The lock annotations mirror the existing relay APIs and fit cleanly into the interface surface.
6584-6592:PeerRelayDSQ/PeerRelayTransactionwrappers are correctThese implementations just forward to
RelayDSQandRelayTransaction, preserving existing relay semantics while exposing them through the internal interface. Lock annotations line up with the underlying methods, and there’s no additional state or complexity introduced.
knst
force-pushed
the
refactor-peermanager-handlers-cj
branch
from
deffce0 to
75a9ea2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/net_processing.cpp (1)
2353-2372: DSQ handling inAlreadyHavelooks correct; consider clarifying NetHandler contractThe new
MSG_DSQbranch:
- Checks
m_cj_walletman->hasQueue(inv.hash)first, which is a cheap local “have-it” check.- Then iterates
m_handlers, usingstd::optional<bool>fromhandler->AlreadyHave(inv)and treatingstd::nulloptas “handler not interested”, while honoring the first concretetrue/false.- Falls back to
false(we do not already have the DSQ), which is appropriate for a known type that we want to request if no component owns it.Behavior-wise this keeps DSQ out of the generic “unknown → already have” path and lets external handlers (e.g. CoinJoin server NetHandler) participate without creating new dependencies.
One small improvement: it might be worth adding a brief comment to the NetHandler interface (or here) explicitly stating that
AlreadyHave()must returnstd::nulloptwhen a handler does not own a givenCInvtype, otherwise a handler that always returnsfalsewould short‑circuit others.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between deffce0595a7a9e7f1cbb584271830f0edac7280 and 75a9ea23d5116b8490fbe2ee7484988495a96e7e.
📒 Files selected for processing (9)
src/coinjoin/server.cpp(18 hunks)src/coinjoin/server.h(4 hunks)src/init.cpp(1 hunks)src/masternode/active/context.cpp(1 hunks)src/masternode/active/context.h(2 hunks)src/net_processing.cpp(6 hunks)src/net_processing.h(3 hunks)src/rpc/coinjoin.cpp(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/lint/lint-circular-dependencies.py
🧰 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/init.cppsrc/rpc/coinjoin.cppsrc/net_processing.hsrc/coinjoin/server.hsrc/masternode/active/context.cppsrc/coinjoin/server.cppsrc/net_processing.cppsrc/masternode/active/context.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/coinjoin/server.hsrc/masternode/active/context.cppsrc/coinjoin/server.cppsrc/masternode/active/context.h
src/coinjoin/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Files:
src/coinjoin/server.hsrc/coinjoin/server.cpp
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/coinjoin/server.hsrc/masternode/active/context.cppsrc/coinjoin/server.cppsrc/masternode/active/context.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/active/context.cppsrc/masternode/active/context.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/active/context.cppsrc/masternode/active/context.h
🧠 Learnings (23)
📓 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: 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: 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.
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
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: In src/net_processing.cpp, if ActiveContext (m_active_ctx) is non-null, its members (including cj_server) are guaranteed to be initialized; call sites can safely dereference m_active_ctx->cj_server without an additional null-check.
Applied to files:
src/init.cppsrc/rpc/coinjoin.cppsrc/masternode/active/context.cppsrc/masternode/active/context.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/init.cppsrc/rpc/coinjoin.cppsrc/net_processing.hsrc/coinjoin/server.hsrc/masternode/active/context.cppsrc/coinjoin/server.cppsrc/net_processing.cppsrc/masternode/active/context.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/net_processing.hsrc/net_processing.cppsrc/masternode/active/context.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/**/*.{cpp,h} : NodeContext must be extended with Dash-specific managers for each subsystem during initialization
Applied to files:
src/init.cpp
📚 Learning: 2025-10-03T11:20:37.475Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/active/context.cpp:29-33
Timestamp: 2025-10-03T11:20:37.475Z
Learning: In Dash codebase, NodeContext (src/node/context.h) serves only as a container with trivial c/d-tors; member lifetime is explicitly managed via reset() calls in the shutdown sequence (src/init.cpp), not by declaration order. For example, active_ctx.reset() is called before DashChainstateSetupClose handles llmq_ctx teardown, ensuring proper destruction order regardless of declaration order in the struct.
Applied to files:
src/init.cppsrc/masternode/active/context.cppsrc/masternode/active/context.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/init.cppsrc/net_processing.hsrc/coinjoin/server.hsrc/masternode/active/context.cppsrc/coinjoin/server.cppsrc/net_processing.cppsrc/masternode/active/context.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/net_processing.hsrc/masternode/active/context.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/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/net_processing.hsrc/masternode/active/context.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} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Applied to files:
src/masternode/active/context.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/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Applied to files:
src/masternode/active/context.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/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/masternode/active/context.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/masternode/active/context.cppsrc/masternode/active/context.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/coinjoin/server.cpp
📚 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/coinjoin/server.cpp
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
src/coinjoin/server.cpp
📚 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/coinjoin/server.cppsrc/masternode/active/context.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/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/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/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/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/net_processing.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/masternode/active/context.h
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/masternode/active/context.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/masternode/active/context.h
🧬 Code graph analysis (3)
src/init.cpp (2)
src/interfaces/node.h (2)
node(42-44)node(417-417)src/init.h (1)
node(23-25)
src/coinjoin/server.h (1)
src/net_processing.h (3)
NetHandler(73-97)NetHandler(77-80)std(91-91)
src/net_processing.cpp (5)
src/node/interfaces.cpp (6)
txid(1063-1068)txid(1063-1063)txid(1069-1075)txid(1069-1069)txid(1084-1089)txid(1084-1084)src/coinjoin/server.cpp (1)
inv(372-372)src/instantsend/net_instantsend.cpp (1)
inv(191-191)src/llmq/dkgsession.cpp (4)
inv(301-301)inv(608-608)inv(819-819)inv(1174-1174)src/init.cpp (1)
handler(496-496)
🪛 Cppcheck (2.18.0)
src/masternode/active/context.cpp
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (21)
src/coinjoin/server.h (5)
10-10: LGTM: Include change aligns with NetHandler usage.The switch from
msg_result.htonet_processing.his appropriate since the class now inherits fromNetHandlerand usesPeerManagerInternal.
28-28: LGTM: NetHandler inheritance breaks circular dependency.The addition of
NetHandleras a base class follows the pattern introduced to break circular dependencies between coinjoin/server and net_processing.
97-100: LGTM: Constructor updated for NetHandler initialization.The addition of
PeerManagerInternal* peer_manageras the first parameter correctly initializes theNetHandlerbase class, which provides access to peer management functionality.
103-106: LGTM: NetHandler overrides correctly implement the interface.All four method overrides properly match the
NetHandlerinterface:
ProcessMessage: Changed to void return for internal error handlingProcessGetData: Handles CoinJoin-specific inventory requestsAlreadyHave: Checks for MSG_DSQ inventorySchedule: ReplacesDoMaintenancefor periodic tasks
87-87: Verify error handling after void return change.The change from
[[nodiscard]] MessageProcessingResulttovoidcorrectly shifts error handling to internal calls. Confirmed:
- Call site at line 55 properly handles void return without capturing a result
- Error handling is properly internalized via
m_peer_manager->PeerMisbehaving()calls for invalid denominations (score 10), null identifiers (score 100), invalid masternode (score 10), and signature failures (score 10)src/net_processing.h (3)
28-28: LGTM: Forward declaration supports ProcessGetData signature.The
CNetMsgMakerforward declaration is properly added to support the newProcessGetDatamethod signature.
66-67: LGTM: New relay methods support handler callbacks.The addition of
PeerRelayTransactionandPeerRelayDSQtoPeerManagerInternalallowsNetHandlerimplementations (likeCCoinJoinServer) to trigger relay operations, properly breaking the circular dependency.
86-94: LGTM: Well-designed handler delegation pattern.The new
NetHandlermethods use appropriate patterns:
AlreadyHavereturnsstd::optional<bool>to distinguish "not my responsibility" (nullopt) from "I handle this" (true/false)ProcessGetDatareturnsboolto indicate whether data was pushed- Clear comments explain the delegation semantics
This design allows multiple handlers to coexist and properly delegate responsibility.
src/net_processing.cpp (3)
44-46: Include switch tocoinjoin/coinjoin.his appropriateUsing the aggregate CoinJoin header here is consistent with the NetHandler-based refactor and helps avoid the previous circular dependency on
coinjoin/server.h. No issues spotted.
655-656: PeerRelayDSQ/PeerRelayTransaction wrappers are correctly wiredThe new
PeerRelayDSQandPeerRelayTransactionoverrides cleanly delegate toRelayDSQandRelayTransaction, and their lock annotations match the underlying methods (no extra locking requirements introduced). This gives NetHandler implementations a narrow, well-scoped way to trigger relays without exposing more ofPeerManagerImpl.Also applies to: 6584-6592
2986-3003: DSQGETDATApath and NetHandler delegation preserve semanticsFor
MSG_DSQgetdata requests you now:
- Try
m_cj_walletman->getQueueFromHash(inv.hash)and, if present, sendDSQUEUEimmediately and markpush = true.- Then iterate
m_handlersand let the first handler that returnstruefromProcessGetDataclaim the request, for any supported inv type.- Only append to
vNotFoundwhenpushremainedfalse, so objects handled by either wallet or handlers won’t triggerNOTFOUND.This keeps DSQ responses local when the wallet has the queue, and moves CoinJoin server / other NetHandler responses behind the generic extension hook without changing NOTFOUND behavior. Locking is also consistent:
ProcessGetDatais called withpeer.m_getdata_requests_mutexheld andcs_mainexcluded, and the new calls respect that.No issues from a net-processing perspective.
src/rpc/coinjoin.cpp (1)
468-468: LGTM!The member access correctly reflects the transition from
cj_serverunique_ptr tom_cj_serverreference. Based on learnings, whenactive_ctxis non-null, its members are guaranteed to be initialized, so no additional null-check is needed.src/init.cpp (2)
2202-2203: LGTM!Registration of
NetInstantSendandNetSigninghandlers before ActiveContext creation is appropriate and follows the established pattern.
2205-2210: Shutdown order is safe; no use-after-free risk.The code safely handles the
CCoinJoinServerlifetime. RPC is stopped at line 55 beforepeerman.reset()at line 86, eliminating access tom_cj_serverthrough the RPC handler. Additionally,ActiveContext::Stop()(line 58) and~ActiveContext()do not accessm_cj_server, so the dangling reference afterpeermandestruction poses no risk beforeactive_ctxis destroyed at line 130.Likely an incorrect or invalid review comment.
src/masternode/active/context.cpp (1)
19-39: LGTM!The constructor correctly accepts
CCoinJoinServer&and binds it tom_cj_server. The destructor andStop()method only interact with LLMQ-related members, notm_cj_server, which addresses potential lifetime concerns during shutdown.src/masternode/active/context.h (1)
43-58: LGTM!The transition from
std::unique_ptr<CCoinJoinServer>toCCoinJoinServer&is consistent with the dependency injection refactoring goal. The deleted copy operations appropriately prevent copying a class with reference members.src/coinjoin/server.cpp (5)
27-44: LGTM!Constructor correctly initializes the
NetHandlerbase class withpeer_managerand removes the oldm_peermanreference in favor of the inheritedm_peer_managerpointer.
48-61: LGTM!The conversion to
voidreturn type is correct. All message handling paths either return early or delegate to specific handlers.
129-207: LGTM!
ProcessDSQUEUEcorrectly handles all error paths with early returns after callingPeerMisbehaving. The previously flagged issues (unusedretvariable and missing return after signature validation failure) have been addressed.
925-939: LGTM!The
AlreadyHaveandProcessGetDataimplementations follow theNetHandlerinterface pattern correctly.AlreadyHavereturnsstd::nulloptfor non-DSQ inventory types, allowing other handlers to respond.
901-913: TheSchedulemethod is properly invoked viaPeerManager::ScheduleHandlers. The CCoinJoinServer is registered as an extra handler at initialization, stored in them_handlersvector, andScheduleHandlerscorrectly iterates through all registered handlers calling theirSchedulemethods. The implementation at lines 901-913 correctly encapsulates the periodic maintenance logic with proper guard checks for blockchain sync and shutdown state.
knst
force-pushed
the
refactor-peermanager-handlers-cj
branch
from
75a9ea2 to
a56c106
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/net_processing.h (1)
89-93: Solid extension points for NetHandler implementations.These virtual methods elegantly allow NetHandler implementations (like CCoinJoinServer) to participate in inventory management and GETDATA processing without tight coupling to net_processing internals. The default implementations returning
falseprovide safe fallback behavior.Optional consideration:
AlreadyHavecould potentially be markedconstif implementations only perform read-only checks without modifying internal state. However, this may be intentionally non-const to allow for cache updates or other state management.- virtual bool AlreadyHave(const CInv& inv) { return false; } + virtual bool AlreadyHave(const CInv& inv) const { return false; }Only apply this change if all derived implementations can be const-qualified.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 75a9ea23d5116b8490fbe2ee7484988495a96e7e and a56c106.
📒 Files selected for processing (5)
src/coinjoin/server.cpp(18 hunks)src/coinjoin/server.h(4 hunks)src/net_processing.cpp(6 hunks)src/net_processing.h(3 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/lint/lint-circular-dependencies.py
🧰 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/net_processing.hsrc/coinjoin/server.cppsrc/net_processing.cppsrc/coinjoin/server.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/coinjoin/server.cppsrc/coinjoin/server.h
src/coinjoin/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Files:
src/coinjoin/server.cppsrc/coinjoin/server.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/coinjoin/server.cppsrc/coinjoin/server.h
🧠 Learnings (16)
📓 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: 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: 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.
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
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: In src/net_processing.cpp, if ActiveContext (m_active_ctx) is non-null, its members (including cj_server) are guaranteed to be initialized; call sites can safely dereference m_active_ctx->cj_server without an additional null-check.
📚 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/net_processing.hsrc/coinjoin/server.cppsrc/net_processing.cppsrc/coinjoin/server.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/net_processing.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/{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/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,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/net_processing.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/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/net_processing.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/net_processing.hsrc/coinjoin/server.cppsrc/net_processing.cppsrc/coinjoin/server.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/coinjoin/server.cpp
📚 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/coinjoin/server.cpp
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
src/coinjoin/server.cpp
📚 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/coinjoin/server.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} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Applied to files:
src/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/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/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/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/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/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/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/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Applied to files:
src/net_processing.cpp
🧬 Code graph analysis (2)
src/net_processing.h (1)
src/net_processing.cpp (33)
PeerRelayTransaction(6588-6591)PeerRelayTransaction(6588-6588)txid(629-629)txid(656-656)txid(657-657)txid(662-662)txid(665-665)PeerRelayDSQ(6583-6586)PeerRelayDSQ(6583-6583)queue(631-631)queue(655-655)pfrom(613-613)pfrom(634-635)pfrom(750-752)pfrom(761-761)pfrom(769-769)pfrom(772-772)pfrom(774-774)pfrom(776-776)pfrom(872-872)pfrom(1054-1054)inv(627-627)inv(628-628)inv(652-652)inv(653-653)inv(654-654)inv(668-668)inv(674-674)inv(962-962)inv(2406-2406)ProcessGetData(2801-3039)ProcessGetData(2801-2801)msgMaker(2159-2159)
src/coinjoin/server.h (5)
src/coinjoin/server.cpp (9)
CCoinJoinServer(27-44)CCoinJoinServer(46-46)ProcessGetData(930-939)ProcessGetData(930-930)inv(372-372)AlreadyHave(925-928)AlreadyHave(925-925)Schedule(901-913)Schedule(901-901)src/coinjoin/coinjoin.cpp (2)
CCoinJoinBaseManager(125-125)CCoinJoinBaseManager(127-127)src/coinjoin/coinjoin.h (1)
CCoinJoinBaseManager(322-351)src/net_processing.h (2)
NetHandler(73-96)NetHandler(77-80)src/coinjoin/walletman.cpp (1)
connman(33-33)
🪛 GitHub Actions: Clang Diff Format Check
src/coinjoin/server.cpp
[error] 922-922: Clang-format differences detected. Formatting changed in function AlreadyHave. Process completed with exit code 1.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (18)
src/net_processing.h (2)
28-28: LGTM! Appropriate forward declaration.The forward declaration of
CNetMsgMakeris correctly added to support the newProcessGetDatamethod signature on line 93, avoiding unnecessary header inclusion and supporting the dependency reduction objectives of this PR.
66-67: Abstraction correctly implements interface for breaking circular dependencies.The
PeerRelayTransactionandPeerRelayDSQmethods provide a clean interface allowing CoinJoin components to relay transactions and DSQ messages through the abstractPeerManagerInternalinterface. Implementation is verified: both methods inPeerManagerImplcorrectly delegate to their respectiveRelayTransactionandRelayDSQmethods. Usage is confirmed across three call sites inCCoinJoinServer::ConsumeCollateraland queue processing methods, all properly usingm_peer_managerinitialized through theNetHandlerbase class. The signatures, parameter passing by const reference, and method patterns align with existing interface conventions.src/net_processing.cpp (3)
44-45: Include swap tocoinjoin/coinjoin.his appropriateThe file now only needs CoinJoin core types (e.g.
CCoinJoinQueue), not the full server header; this narrows the dependency surface and is consistent with moving server logic behindNetHandler.
2353-2371: DSQAlreadyHave/GETDATAdelegation to handlers looks consistentThe new
MSG_DSQhandling inAlreadyHave(wallet check thenm_handlers) and inProcessGetData(walletgetQueueFromHash, thenNetHandler::ProcessGetDatafallback, elseNOTFOUND) cleanly routes DSQ inventory through the new handler abstraction without changing semantics for callers ofAlreadyHave/GETDATA. The booleanpushgate also ensures only a single producer responds per request.Also applies to: 2985-3003
655-656: PeerManagerInternal relay hooks correctly forward to existing relay logic
PeerRelayDSQandPeerRelayTransactionare thin wrappers that just callRelayDSQ/RelayTransaction, preserving existing locking and relay behavior while exposing stable hooks for externalNetHandlers. The override signatures and implementations are consistent with the rest of thePeerManagerInternalAPI.Also applies to: 6583-6591
src/coinjoin/server.h (5)
10-10: LGTM! Include change aligns with NetHandler integration.The change from
msg_result.htonet_processing.his correct as CCoinJoinServer now inherits from NetHandler (defined in net_processing.h) and no longer returns MessageProcessingResult.
28-28: LGTM! NetHandler inheritance correctly implemented.The addition of NetHandler inheritance is consistent with the PR objective to break circular dependencies. The inheritance order (CCoinJoinBaseSession, CCoinJoinBaseManager, NetHandler) is appropriate.
87-87: LGTM! ProcessDSQUEUE signature change is correct.The signature change to void return and NodeId parameter (instead of CNode&) aligns with the net-centric refactoring. This allows the method to use m_peer_manager for network operations without directly depending on the peer object.
97-100: LGTM! Constructor signature correctly updated.The constructor now takes PeerManagerInternal* as the first parameter, which is properly passed to the NetHandler base class. This is consistent with the NetHandler pattern shown in the relevant code snippets.
103-106: LGTM! NetHandler override signatures are correct.All four override methods (ProcessMessage, ProcessGetData, AlreadyHave, Schedule) match the NetHandler base class interface shown in the relevant code snippets. The void return types and parameter lists are appropriate for the net-centric design.
src/coinjoin/server.cpp (8)
27-44: LGTM! Constructor correctly initializes NetHandler.The constructor properly accepts PeerManagerInternal* and initializes the NetHandler base class. The base class constructor asserts the pointer is non-null, so initialization is safe.
48-61: LGTM! ProcessMessage correctly implements void return flow.The void return signature matches the NetHandler interface. The early return on Line 50 and the ProcessDSQUEUE call on Line 55 (without capturing a return value) are both correct for the new signature.
129-208: LGTM! ProcessDSQUEUE correctly refactored to void return.The method correctly handles error cases with m_peer_manager->PeerMisbehaving followed by early returns (Lines 141-142, 146-147, 155-156, 189-190). The success case (Line 206) properly uses m_peer_manager->PeerRelayDSQ. Past review comments about missing returns and unused variables have been addressed.
373-373: LGTM! Peer manager relay calls correctly updated.All relay operations (PeerRelayInv, PeerRelayTransaction, PeerRelayDSQ) correctly use m_peer_manager from the NetHandler base class. This is consistent with the refactoring to break circular dependencies.
Also applies to: 486-486, 531-531, 741-741
901-913: LGTM! Schedule correctly replaces DoMaintenance.The Schedule method properly implements the NetHandler interface. The lambda function includes appropriate guards (blockchain sync check and shutdown check) and executes the same maintenance tasks (CheckForCompleteQueue, CheckPool, CheckTimeout) on a 1-second interval. The conversion from DoMaintenance to scheduled execution is clean.
925-928: LGTM! AlreadyHave correctly implements queue lookup.The implementation correctly returns true if the inventory type is MSG_DSQ and we have the queue, false otherwise. The bool return type (rather than optional) is appropriate as confirmed in past review discussions.
930-939: LGTM! ProcessGetData correctly handles DSQ inventory requests.The implementation properly handles GETDATA requests for MSG_DSQ inventory. It returns false for non-DSQ types, retrieves the queue, and sends it to the requesting peer using the appropriate network message. The return value correctly indicates whether the request was handled.
924-924: No formatting issues detected in the current code state. The spacing between GetJsonInfo() and AlreadyHave() functions follows standard C++ conventions with a single blank line between function definitions.
…r dependencies Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
3 participants
Issue being fixed or feature implemented
There are several circular dependencies that may be resolved by using
NetHandlerinterface in CJThis PR is not a part of separation of Network and Consensus code; it's a side quest to break couple circular dependencies. It doesn't block me currently and any conflicting PRs may be merged before this one.
ToDo the same refactoring for
coinjoin/clientandcoinjoin/walletman(out of scope this PR, could be done anytime in the future, but probably a bit more complex than coinjoin/server because need to splitCJWalletManagerandCJWalletManagerImpl::queueman)What was done?
Two circular dependencies are resolved.
This PR use
NetHandlerinterface to aggregate network code.How Has This Been Tested?
Run unit & functional tests.
Breaking Changes
N/A
Checklist: