refactor: move `CDSTXManager` to `NodeContext`, abstract wallet-only CoinJoin managers behind interface, `s/CJContext/CJWalletManager/g` #6840

kwvg · 2025-09-08T23:04:10Z

Additional Information

Depends on refactor: drop fMasternodeMode global, create new context for masternode mode-only logic (ActiveContext), move spun-off signers and EHF signals handler #6828
Depends on chore: consolidate oft-repeated type definitions to {evo,llmq}/types.h #6839
As part of efforts in reducing ENABLE_WALLET proliferation in non-wallet code, this pull request isolates wallet-specific logic by abstracting implementation details of CoinJoin wallet managers in a manner similar to PeerManager (source).
- Additionally, as it is now conditionally initialized (as opposed to current develop behavior of unconditional initialization and bailout), we get to reduce the footprint of CActiveMasternodeManager, which is part of a larger initiative to isolate masternode-mode logic.
MisbehavingError and MessageProcessingResult were extracted to msg_processing.h as support for relaying DSQ messages requires including coinjoin/coinjoin.h which creates a circular dependency issue if left as-is in protocol.h (as it is in develop), see below.
```
A new circular dependency in the form of "chainparams -> protocol -> coinjoin/coinjoin -> chainparams" appears to have been introduced.
```
Moving the constructor definitions to a source file would not have resolved this as circular dependencies treat the source file and header as one unit.

How Has This Been Tested?

A full CoinJoin session run on 6a20464

Breaking Changes

None expected.

Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation (note: N/A)
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

github-actions · 2025-09-08T23:05:04Z

⚠️ Potential Merge Conflicts Detected

This PR has potential conflicts with the following open PRs:

feat: share vecMasternodesUsed across all wallets, improve its handling #6875 - feat: share vecMasternodesUsed across all wallets, improve its handling

Please coordinate with the authors of these PRs to avoid merge conflicts.

github-actions · 2025-09-09T16:38:23Z

This pull request has conflicts, please rebase.

coderabbitai · 2025-09-30T06:42:18Z

Walkthrough

The patch removes CJContext and its header/implementation, introduces CJWalletManager (wallet/queue orchestration) and CDSTXManager (DSTX tracking), and adds a new msg_result.h defining MisbehavingError and MessageProcessingResult (migrated from protocol.h). Many interfaces and constructors were rewired to accept dstxman and optional cj_walletman instead of CJContext (NodeContext, PeerManager::make, DSNotificationInterface, net_processing, init, tests). CoinJoin client/server code drops masternode-specific guards and defers DSQUEUE relay via MessageProcessingResult. Build sources and Makefile exports switch from coinjoin/context.* to coinjoin/walletman.*. Numerous headers now include msg_result.h.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 15.66% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The pull request title "refactor: move `CDSTXManager` to `NodeContext`, abstract wallet-only CoinJoin managers behind interface, `s/CJContext/CJWalletManager/g`" accurately and specifically describes the three main architectural changes in the changeset. The title identifies the movement of CDSTXManager, the abstraction of wallet managers behind an interface, and the systematic replacement of CJContext with CJWalletManager, which directly correspond to the file changes throughout the PR (src/node/context.h, src/coinjoin/walletman.h, src/coinjoin/client.h, and numerous other refactored files). The title is clear, concise, and provides sufficient specificity that a developer scanning the history would understand the primary refactoring effort.
Description Check	✅ Passed	The pull request description is clearly related to the changeset and provides meaningful context about the refactoring goals. It explains the effort to reduce ENABLE_WALLET proliferation in non-wallet code by abstracting wallet-specific logic similar to PeerManager, addresses how the PR conditionally initializes components to reduce CActiveMasternodeManager footprint, and importantly documents the technical decision to extract MisbehavingError and MessageProcessingResult to msg_result.h to resolve a circular dependency issue. The description is supported by testing evidence (CoinJoin session run), breaking changes assessment, and a completion checklist, making it substantive and directly relevant to understanding the changeset's purpose and implementation decisions.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (7)

src/msg_processing.h (3)

19-32: Consider defaulting the message parameter or providing a factory method.

The template constructor with perfect forwarding (lines 27-31) is good for efficiency, but having two constructors where one takes only score and another takes score + message could be confusing at call sites. Consider whether a default parameter or a named factory method (MisbehavingError::WithMessage(score, msg)) would improve clarity.

47-66: Document the lifecycle and ownership semantics of these fields.

The struct aggregates several different types of results (errors, inventories, transactions, etc.), but the documentation doesn't clarify:

Can multiple fields be set simultaneously, or are they mutually exclusive?

Who owns the referenced objects (e.g., in m_inv_filter's std::variant<CTransactionRef, uint256>)?

What's the expected flow when multiple inventories or DSQs are present?

Consider expanding the struct-level documentation (lines 34-44) to cover these points, which would help maintainers understand the intended usage patterns.

8-8: Use a forward declaration for CCoinJoinQueue instead of including coinjoin/coinjoin.h
In src/msg_processing.h (line 8), you only need CCoinJoinQueue for m_dsq, but <coinjoin/coinjoin.h> pulls in a dozen headers. Replace it with class CCoinJoinQueue; and only include a minimal header if required.
src/coinjoin/walletman.cpp (1)
136-149: Consider removing unnecessary return statements.

The return statements on lines 138, 143, and 148 are unnecessary for void functions. While not harmful, removing them would improve consistency with C++ conventions.

Apply this diff:
 void CJWalletManagerImpl::addWallet(const std::shared_ptr<wallet::CWallet>& wallet)
 {
-    return walletman->Add(wallet);
+    walletman->Add(wallet);
 }
 
 void CJWalletManagerImpl::flushWallet(const std::string& name)
 {
-    return walletman->Flush(name);
+    walletman->Flush(name);
 }
 
 void CJWalletManagerImpl::removeWallet(const std::string& name)
 {
-    return walletman->Remove(name);
+    walletman->Remove(name);
 }
src/net_processing.h (1)
13-14: Reduce header coupling: forward-declare MessageProcessingResult instead of including msg_processing.h

The header only needs the type name in a method signature. Prefer a forward declaration to avoid pulling transitive deps into a widely included header.

Apply:
-#include <msg_processing.h>
+// fwd decl to avoid heavy include here
+struct MessageProcessingResult;
src/net_processing.cpp (2)
2236-2239: Avoid relying on implicit bool conversion of CCoinJoinBroadcastTx

static_cast(m_dstxman.GetDSTX(inv.hash)) assumes CCoinJoinBroadcastTx has a safe boolean conversion. Prefer an explicit presence API to make intent clear and avoid accidental costly copies.

Example (manager API change, if acceptable):
- (inv.IsMsgDstx() && static_cast<bool>(m_dstxman.GetDSTX(inv.hash))) ||
+ (inv.IsMsgDstx() && m_dstxman.HasDSTX(inv.hash)) ||
If changing the API isn’t feasible now, consider returning a lightweight optional-like type to avoid copying.

2447-2447: Hot path: multiple GetDSTX lookups per tx broadcast

m_dstxman.GetDSTX(hash) is called in tight broadcast loops. If GetDSTX locks and copies, this adds overhead. Cache the result per-tx per-cycle or expose a HasDSTX(hash) that avoids constructing/copying.

Minimal local caching in SendMessages:
- int nInvType = m_dstxman.GetDSTX(hash) ? MSG_DSTX : MSG_TX;
+ const bool is_dstx = /* cheap presence check */ m_dstxman.HasDSTX ? m_dstxman.HasDSTX(hash) : static_cast<bool>(m_dstxman.GetDSTX(hash));
+ int nInvType = is_dstx ? MSG_DSTX : MSG_TX;
Also applies to: 6099-6102, 6176-6178

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cae0bff and f786cb246fb358bc0eaeadac9c25ced3d56511a4.

📒 Files selected for processing (39)

src/Makefile.am (3 hunks)
src/coinjoin/client.cpp (5 hunks)
src/coinjoin/client.h (5 hunks)
src/coinjoin/context.cpp (0 hunks)
src/coinjoin/context.h (0 hunks)
src/coinjoin/interfaces.cpp (3 hunks)
src/coinjoin/server.cpp (1 hunks)
src/coinjoin/server.h (1 hunks)
src/coinjoin/walletman.cpp (1 hunks)
src/coinjoin/walletman.h (1 hunks)
src/dsnotificationinterface.cpp (6 hunks)
src/dsnotificationinterface.h (2 hunks)
src/evo/mnauth.h (1 hunks)
src/governance/governance.h (1 hunks)
src/init.cpp (6 hunks)
src/llmq/blockprocessor.h (1 hunks)
src/llmq/dkgsessionhandler.h (1 hunks)
src/llmq/dkgsessionmgr.h (1 hunks)
src/llmq/ehf_signals.h (1 hunks)
src/llmq/quorums.h (1 hunks)
src/llmq/signing.h (1 hunks)
src/llmq/signing_shares.h (1 hunks)
src/masternode/utils.cpp (3 hunks)
src/masternode/utils.h (1 hunks)
src/msg_processing.h (1 hunks)
src/net_processing.cpp (17 hunks)
src/net_processing.h (2 hunks)
src/node/context.cpp (1 hunks)
src/node/context.h (2 hunks)
src/protocol.h (0 hunks)
src/rpc/coinjoin.cpp (2 hunks)
src/spork.h (1 hunks)
src/test/coinjoin_dstxmanager_tests.cpp (2 hunks)
src/test/denialofservice_tests.cpp (4 hunks)
src/test/net_peer_connection_tests.cpp (1 hunks)
src/test/util/setup_common.cpp (4 hunks)
src/wallet/test/coinjoin_tests.cpp (2 hunks)
src/wallet/test/wallet_tests.cpp (0 hunks)
test/lint/lint-circular-dependencies.py (1 hunks)

💤 Files with no reviewable changes (4)

src/wallet/test/wallet_tests.cpp
src/coinjoin/context.h
src/coinjoin/context.cpp
src/protocol.h

🧰 Additional context used

📓 Path-based instructions (3)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/spork.h
src/llmq/ehf_signals.h
src/llmq/dkgsessionhandler.h
src/llmq/signing_shares.h
src/node/context.h
src/llmq/quorums.h
src/test/denialofservice_tests.cpp
src/governance/governance.h
src/node/context.cpp
src/coinjoin/interfaces.cpp
src/test/coinjoin_dstxmanager_tests.cpp
src/coinjoin/server.cpp
src/llmq/signing.h
src/coinjoin/client.h
src/llmq/dkgsessionmgr.h
src/masternode/utils.cpp
src/rpc/coinjoin.cpp
src/net_processing.h
src/evo/mnauth.h
src/net_processing.cpp
src/coinjoin/walletman.cpp
src/dsnotificationinterface.cpp
src/dsnotificationinterface.h
src/coinjoin/server.h
src/msg_processing.h
src/test/net_peer_connection_tests.cpp
src/masternode/utils.h
src/coinjoin/walletman.h
src/wallet/test/coinjoin_tests.cpp
src/llmq/blockprocessor.h
src/test/util/setup_common.cpp
src/init.cpp
src/coinjoin/client.cpp

src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Files:

src/test/denialofservice_tests.cpp
src/test/coinjoin_dstxmanager_tests.cpp
src/test/net_peer_connection_tests.cpp
src/wallet/test/coinjoin_tests.cpp
src/test/util/setup_common.cpp

src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Masternode lists must use immutable data structures (Immer library) for thread safety

Files:

src/masternode/utils.cpp
src/evo/mnauth.h
src/masternode/utils.h

🧬 Code graph analysis (20)

src/node/context.h (2)

src/coinjoin/coinjoin.h (1)

CDSTXManager (378-406)

src/coinjoin/walletman.h (1)

CJWalletManager (36-64)

src/test/denialofservice_tests.cpp (2)

src/coinjoin/walletman.cpp (3)

make (152-163)

make (152-155)

connman (33-33)

src/net_processing.cpp (2)

make (1954-1966)

make (1954-1963)

src/coinjoin/interfaces.cpp (2)

src/coinjoin/walletman.cpp (4)

wallet (43-43)

name (37-37)

name (44-44)

name (45-45)

src/coinjoin/walletman.h (1)

wallet (32-34)

src/test/coinjoin_dstxmanager_tests.cpp (1)

src/coinjoin/interfaces.cpp (2)

Assert (80-83)

Assert (85-88)

src/coinjoin/server.cpp (1)

src/net_processing.cpp (1)

ret (659-659)

src/coinjoin/client.h (2)

src/net_processing.cpp (3)

ProcessMessage (3519-5302)

ProcessMessage (3519-3524)

from (1031-1031)

src/coinjoin/client.cpp (6)

ProcessMessage (42-141)

ProcessMessage (42-43)

ProcessMessage (143-164)

ProcessMessage (143-143)

ProcessMessage (179-225)

ProcessMessage (179-179)

src/masternode/utils.cpp (1)

src/coinjoin/client.cpp (6)

DoMaintenance (1819-1824)

DoMaintenance (1819-1819)

DoMaintenance (1826-1842)

DoMaintenance (1826-1826)

DoMaintenance (1885-1891)

DoMaintenance (1885-1885)

src/net_processing.h (3)

src/coinjoin/coinjoin.h (1)

CDSTXManager (378-406)

src/coinjoin/walletman.h (1)

CJWalletManager (36-64)

src/masternode/active/context.cpp (2)

ActiveContext (16-33)

ActiveContext (35-39)

src/net_processing.cpp (1)

src/coinjoin/walletman.cpp (5)

make (152-163)

make (152-155)

connman (33-33)

hash (36-36)

hash (40-40)

src/coinjoin/walletman.cpp (4)

src/net_processing.cpp (3)

ret (659-659)

make (1954-1966)

make (1954-1963)

src/coinjoin/interfaces.cpp (8)

name (103-107)

name (103-103)

name (108-111)

name (108-108)

name (112-116)

name (112-112)

wallet (98-102)

wallet (98-98)

src/coinjoin/walletman.h (1)

wallet (32-34)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

src/dsnotificationinterface.cpp (2)

src/dsnotificationinterface.h (1)

CDSNotificationInterface (20-57)

src/coinjoin/coinjoin.cpp (4)

BlockConnected (486-494)

BlockConnected (486-486)

BlockDisconnected (496-503)

BlockDisconnected (496-496)

src/dsnotificationinterface.h (2)

src/coinjoin/coinjoin.h (1)

CDSTXManager (378-406)

src/dsnotificationinterface.cpp (1)

CDSNotificationInterface (23-39)

src/msg_processing.h (2)

src/protocol.h (1)

CInv (529-566)

src/net_processing.cpp (6)

inv (625-625)

inv (626-626)

inv (645-645)

inv (651-651)

inv (936-936)

inv (2311-2311)

src/test/net_peer_connection_tests.cpp (2)

src/coinjoin/walletman.cpp (3)

make (152-163)

make (152-155)

connman (33-33)

src/net_processing.cpp (2)

make (1954-1966)

make (1954-1963)

src/masternode/utils.h (2)

src/coinjoin/walletman.h (1)

CJWalletManager (36-64)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

src/coinjoin/walletman.h (3)

src/coinjoin/interfaces.cpp (8)

wallet (98-102)

wallet (98-98)

name (103-107)

name (103-103)

name (108-111)

name (108-108)

name (112-116)

name (112-112)

src/coinjoin/walletman.cpp (32)

wallet (43-43)

make (152-163)

make (152-155)

Schedule (68-75)

Schedule (68-68)

connman (33-33)

hasQueue (86-92)

hasQueue (86-86)

hash (36-36)

hash (40-40)

getClient (94-97)

getClient (94-94)

name (37-37)

name (44-44)

name (45-45)

processMessage (99-110)

processMessage (99-101)

getQueueFromHash (112-118)

getQueueFromHash (112-112)

getQueueSize (120-126)

getQueueSize (120-120)

getMixingMasternodes (128-134)

getMixingMasternodes (128-128)

addWallet (136-139)

addWallet (136-136)

removeWallet (146-149)

removeWallet (146-146)

flushWallet (141-144)

flushWallet (141-141)

UpdatedBlockTip (77-84)

UpdatedBlockTip (77-77)

pindexNew (49-49)

src/net_processing.cpp (9)

make (1954-1966)

make (1954-1963)

scheduler (617-617)

scheduler (668-668)

hash (999-999)

hash (1005-1005)

UpdatedBlockTip (2117-2152)

UpdatedBlockTip (2117-2117)

pindexNew (602-602)

src/wallet/test/coinjoin_tests.cpp (1)

src/coinjoin/interfaces.cpp (2)

Assert (80-83)

Assert (85-88)

src/test/util/setup_common.cpp (2)

src/coinjoin/walletman.cpp (2)

make (152-163)

make (152-155)

src/net_processing.cpp (2)

make (1954-1966)

make (1954-1963)

src/init.cpp (3)

src/net_processing.cpp (11)

node (609-609)

node (610-610)

node (635-635)

node (763-763)

node (863-867)

node (878-878)

node (889-889)

node (900-900)

node (908-908)

make (1954-1966)

make (1954-1963)

src/coinjoin/walletman.cpp (2)

make (152-163)

make (152-155)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

src/coinjoin/client.cpp (2)

src/net_processing.cpp (4)

ProcessMessage (3519-5302)

ProcessMessage (3519-3524)

from (1031-1031)

ret (659-659)

src/coinjoin/server.cpp (2)

ProcessMessage (26-40)

ProcessMessage (26-26)

🔇 Additional comments (73)

test/lint/lint-circular-dependencies.py (2)

30-30: LGTM: New circular dependency properly documented.

The new dependency chain chainlock/chainlock -> llmq/quorums -> msg_processing -> coinjoin/coinjoin -> chainlock/chainlock correctly reflects the architectural change where message processing results (including DSQ relay) now flow through the new msg_processing.h abstraction.

34-35: LGTM: Circular dependencies updated to reflect manager-based architecture.

The dependency updates correctly capture the refactoring from CJContext to the new manager-based approach:

Line 34 now reflects the path through msg_processing for CoinJoin message handling

Line 35 updates from coinjoin/context to coinjoin/walletman consistent with the s/CJContext/CJWalletManager/g rename

src/msg_processing.h (1)

69-75: LGTM: Convenience constructors are well-designed.

The default, CInv, and MisbehavingError constructors provide clear, type-safe initialization paths for common use cases.

src/evo/mnauth.h (1)

9-10: LGTM: Include addition supports MessageProcessingResult usage.

The include is necessary for the ProcessMessage method at line 63 which returns MessageProcessingResult. The formatting with a blank line after follows standard conventions.

src/spork.h (1)

8-9: LGTM: Include addition supports MessageProcessingResult return types.

The include is necessary for the ProcessMessage (line 262) and ProcessSpork (line 270) methods which return MessageProcessingResult. The formatting is consistent with project conventions.

src/llmq/signing_shares.h (1)

10-11: Direct include of msg_processing.h is appropriate; no circular dependency detected. The function HandleNewRecoveredSig returns MessageProcessingResult, so including msg_processing.h is required; although llmq/signing.h also includes it, there’s no back-include to signing_shares.h.

src/governance/governance.h (1)

10-10: LGTM! Include is necessary for MessageProcessingResult.

The addition of <msg_processing.h> is required because this header declares methods that return MessageProcessingResult (lines 274, 275, 277). This aligns with the PR's objective to extract message processing types to a dedicated header.

src/llmq/quorums.h (1)

14-14: LGTM! Include correctly supports MessageProcessingResult usage.

The <msg_processing.h> include is necessary for the MessageProcessingResult return type used in the ProcessMessage method (line 273). This change aligns with the PR's refactoring to centralize message processing types.

src/coinjoin/server.h (1)

9-9: LGTM! Include required for MessageProcessingResult return types.

The <msg_processing.h> include is necessary because this header declares methods returning MessageProcessingResult (lines 90, 115). This is consistent with the PR's refactoring to use centralized message processing types.

src/test/coinjoin_dstxmanager_tests.cpp (2)

41-41: LGTM! Test correctly updated for new NodeContext structure.

The test correctly adapts to the architectural change where CDSTXManager is now a direct member of NodeContext (m_node.dstxman) instead of being accessed through cj_ctx. The use of Assert() ensures the manager is present.

55-55: LGTM! Consistent access pattern for dstxman.

This line correctly uses the same direct access pattern to m_node.dstxman, maintaining consistency throughout the test.

src/node/context.cpp (1)

9-10: Includes are necessary.

The context.cpp file requires <coinjoin/coinjoin.h> for CDSTXManager and <coinjoin/walletman.h> for CJWalletManager; no changes needed.

src/llmq/ehf_signals.h (1)

9-9: LGTM! Necessary include for MessageProcessingResult.

The include is required for the MessageProcessingResult return type used by HandleNewRecoveredSig at line 47.

src/llmq/signing.h (1)

12-12: LGTM! Necessary include for MessageProcessingResult.

The include is required for the MessageProcessingResult return type used by CRecoveredSigsListener::HandleNewRecoveredSig (line 154) and CSigningManager::ProcessMessage (line 185).

src/llmq/dkgsessionhandler.h (1)

8-9: LGTM! Necessary include for MessageProcessingResult.

The include is required for the MessageProcessingResult return type used by CDKGPendingMessages::PushPendingMessage (line 81) and CDKGSessionHandler::ProcessMessage (line 171).

src/node/context.h (3)

21-21: LGTM! Forward declaration for CDSTXManager.

This forward declaration is necessary for the dstxman member added at line 85, aligning with the PR objective to move CDSTXManager into NodeContext.

26-26: LGTM! Forward declaration for CJWalletManager.

This forward declaration is necessary for the cj_walletman member added at line 84, aligning with the PR objective to rename CJContext to CJWalletManager and abstract wallet-only CoinJoin managers.

84-85: LGTM! NodeContext members align with PR architectural changes.

The addition of cj_walletman and dstxman members correctly reflects the refactoring objectives:

CDSTXManager is now part of NodeContext for managing DSTX broadcasts

CJWalletManager replaces CJContext as the wallet-specific CoinJoin manager interface

Both follow the existing pattern of using unique_ptr for manager ownership.

src/llmq/blockprocessor.h (1)

11-14: LGTM! Include additions and reordering for msg_processing.h.

The changes introduce msg_processing.h (required for MessageProcessingResult at line 62) and reorder existing includes. The reordering likely ensures proper dependency resolution after the refactoring.

src/llmq/dkgsessionmgr.h (1)

11-11: LGTM! Include is necessary for the ProcessMessage return type.

The include of msg_processing.h is required because ProcessMessage (line 93) returns MessageProcessingResult, which is defined in that header. The placement is appropriate.

src/rpc/coinjoin.cpp (2)

6-6: LGTM! Include updated to reflect refactored wallet manager.

The include change from coinjoin/context.h to coinjoin/walletman.h aligns with the PR's goal of replacing CJContext with CJWalletManager.

475-479: LGTM! Proper optional chaining for queue size retrieval.

The implementation correctly:

Checks if cj_walletman exists before accessing it

Uses the optional return from getQueueSize()

Only adds queue_size to the JSON response when both conditions are met

This follows modern C++ optional patterns and prevents null pointer dereference.

src/masternode/utils.h (2)

11-11: LGTM! Forward declaration updated for new wallet manager.

The forward declaration correctly changes from CJContext to CJWalletManager, consistent with the PR's renaming strategy.

16-17: LGTM! Signature change correctly makes wallet manager optional.

The parameter change from CJContext& cj_ctx (required reference) to const CJWalletManager* const cj_walletman (optional const pointer) aligns with the PR's goal of making wallet-related initialization conditional. The implementation in src/masternode/utils.cpp (lines 14-81) properly handles the null case, checking cj_walletman before calling getMixingMasternodes().

src/coinjoin/server.cpp (1)

184-184: LGTM! Correctly implements caller-driven DSQ relay.

The change from immediate relay (m_peerman.RelayDSQ(dsq)) to deferred relay via ret.m_dsq.push_back(dsq) aligns with the new message processing architecture introduced in this PR. The caller (PostProcessMessage in net_processing.cpp) will handle the relay based on the returned MessageProcessingResult.

src/coinjoin/client.h (6)

11-11: LGTM! Required include for MessageProcessingResult.

The inclusion of msg_processing.h is necessary for the MessageProcessingResult return type used in CCoinJoinClientQueueManager::ProcessMessage (line 248).

79-89: LGTM! Consistent constructor parameter update.

The queueman parameter type change from reference to const std::unique_ptr<CCoinJoinClientQueueManager>& is consistent with the updated architecture and matches the same pattern used in other constructors (lines 298, 304).

198-200: LGTM! Removes is_masternode parameter.

The removal of the is_masternode boolean parameter from CCoinJoinClientSession constructor aligns with the PR objective to reduce the footprint of masternode-mode logic and eliminate masternode-specific short-circuits in client code.

239-246: LGTM! Streamlined constructor signature.

The removal of the PeerManager parameter and the is_masternode boolean aligns with the new architecture where message processing results are returned rather than immediately acted upon, and masternode-specific logic is isolated.

248-250: LGTM! Updated to new message processing pattern.

The signature change from accepting a PeerManager& reference to accepting a NodeId from and returning MessageProcessingResult correctly implements the new caller-driven relay pattern described in the PR objectives. This allows DSQ messages to be relayed via the returned result structure rather than immediately through PeerManager.

298-306: LGTM! Consistent queueman parameter type.

The queueman parameter type matches the pattern established in CoinJoinWalletManager constructor (lines 81, 88) and is consistent across the codebase.

src/wallet/test/coinjoin_tests.cpp (2)

9-9: LGTM! Required include for new wallet manager interface.

The inclusion of coinjoin/walletman.h is necessary to access the CJWalletManager interface and its getClient() method used in the updated test (line 224).

224-230: LGTM! Correctly updated to new wallet manager access pattern.

The test correctly migrates from the old CJContext-based access (m_node.cj_ctx->walletman->Get("")) to the new CJWalletManager-based access (m_node.cj_walletman->getClient("")). The test logic remains unchanged and properly validates the IsMixing(), StartMixing(), and StopMixing() behavior. The use of Assert() to dereference the pointer is appropriate for test code.

src/coinjoin/interfaces.cpp (6)

9-9: LGTM!

The include change from coinjoin/context.h to coinjoin/walletman.h correctly reflects the refactoring to use the new CJWalletManager interface.

80-83: LGTM!

The refactoring of the helper method is well-executed:

Renamed from walletman() to manager() for better clarity

Updated return type to CJWalletManager& to use the new interface

Switched from m_node.cj_ctx->walletman to direct access via m_node.cj_walletman

This aligns with the PR's goal of abstracting wallet-only CoinJoin managers behind an interface.

98-102: LGTM!

The AddWallet method correctly uses the renamed manager() helper and calls the interface method addWallet().

103-107: LGTM!

The RemoveWallet method correctly uses the renamed manager() helper and calls the interface method removeWallet().

108-111: LGTM!

The FlushWallet method correctly uses the renamed manager() helper and calls the interface method flushWallet().

112-116: LGTM!

The GetClient method correctly uses the renamed manager() helper and calls the interface method getClient().

src/Makefile.am (3)

185-185: LGTM!

The addition of coinjoin/walletman.h to BITCOIN_CORE_H correctly registers the new public header in the build system.

294-294: LGTM!

The addition of msg_processing.h to BITCOIN_CORE_H correctly registers the new header that contains MessageProcessingResult and MisbehavingError types, helping avoid circular dependencies as noted in the PR objectives.

489-489: LGTM!

The addition of coinjoin/walletman.cpp to libbitcoin_node_a_SOURCES correctly includes the new implementation file in the build.

src/test/net_peer_connection_tests.cpp (1)

88-92: LGTM!

The PeerManager::make call has been correctly updated to match the new signature:

Added *m_node.dstxman parameter for the new CDSTXManager reference

Replaced cj_ctx with cj_walletman (as nullptr for this test)

Added explicit active_ctx parameter

Maintained llmq_ctx parameter in the correct position

The changes align with the PR's refactoring goals of abstracting wallet-only CoinJoin managers behind an interface.

src/coinjoin/walletman.cpp (1)

99-110: No aggregation needed for client manager ProcessMessage
CCoinJoinClientManager::ProcessMessage returns void, so there’s no result to collect or merge with the queue manager’s return value.

src/net_processing.cpp (3)

586-596: CJWalletManager and CDSTXManager wiring: LGTM

Constructor and member wiring for m_dstxman and m_cj_walletman are consistent with the updated factory signature.

Also applies to: 1968-1993

3501-3504: DSQ relaying via PostProcessMessage: LGTM

Relaying DSQ items from MessageProcessingResult centralizes propagation. No issues spotted.

2867-2884: DSQ GetData fallback order makes sense

Querying server first, then wallet manager for DSQ content avoids duplication where both may track queues. Looks correct.

src/dsnotificationinterface.h (1)

23-28: No external constructor usages—signature change safe
No additional call site updates required.

src/net_processing.h (1)

61-70: Signature changes look correct—all PeerManager::make calls updated

src/coinjoin/walletman.h (5)

1-35: LGTM! Clean header structure.

The includes and forward declarations are appropriate for an interface header. The new msg_processing.h include properly supports the MessageProcessingResult return type used in the interface.

36-43: LGTM! Factory pattern is well-designed.

The static factory method appropriately handles conditional wallet compilation (returns nullptr when ENABLE_WALLET is not defined, as seen in the implementation). The parameter set covers all necessary dependencies for CoinJoin wallet management.

45-50: LGTM! Core interface methods are well-defined.

The Schedule, hasQueue, and getClient methods provide the essential interface for CoinJoin wallet scheduling and client access. Signatures match the implementation patterns seen in walletman.cpp.

51-58: LGTM! Message processing and query methods are properly designed.

The interface methods appropriately use:

MessageProcessingResult for the new message processing architecture

std::optional return types for queries that may not have values (when queue manager doesn't exist)

Vector return for mixing masternodes collection

The wallet lifecycle methods (addWallet, removeWallet, flushWallet) support dynamic wallet management.

60-64: LGTM! CValidationInterface override is correctly specified.

The UpdatedBlockTip override properly integrates the CoinJoin wallet manager with the validation interface. The pure virtual specification ensures derived classes implement block tip updates for their client managers.

src/dsnotificationinterface.cpp (4)

23-39: LGTM! Constructor refactored to use DSTX manager.

The constructor signature correctly transitions from CJContext-based initialization to CDSTXManager-based routing. The member initialization properly reflects the new architecture with m_dstxman replacing the previous CJ context handling.

47-64: LGTM! Assert guards added for context access.

The additions of Assert(m_llmq_ctx) and Assert(m_dmnman) provide runtime safety checks while maintaining the same functional behavior. This is consistent with defensive programming practices.

66-94: LGTM! Block tip and mempool routing updated to DSTX manager.

The changes correctly route block tip updates and mempool transaction notifications through the new m_dstxman instead of the previous CJ context. The Assert guards ensure m_llmq_ctx is valid before dereferencing.

96-130: LGTM! Validation interface callbacks consistently routed through DSTX manager.

All block and chainlock notifications (TransactionRemovedFromMempool, BlockConnected, BlockDisconnected, NotifyChainLock) now properly route through m_dstxman, completing the architectural shift from CJ context to the new manager-based approach. Assert guards are consistently applied.

src/init.cpp (5)

82-82: LGTM! Include added for CJWalletManager.

The include is properly placed in the coinjoin section and necessary for the new manager initialization below.

2165-2175: LGTM! New managers properly wired into PeerManager and notification interface.

The initialization correctly passes:

*node.dstxman (dereferencing unique_ptr to get reference) to PeerManager::make and CDSNotificationInterface

node.cj_walletman.get() (raw pointer from unique_ptr) to PeerManager::make

This matches the expected signatures from net_processing.cpp and dsnotificationinterface.h.

2177-2303: LGTM! Scheduling and maintenance properly configured for new managers.

The changes correctly:

Pass *node.dstxman to ActiveContext for masternode operations (Line 2181)

Pass node.cj_walletman.get() to CMasternodeUtils::DoMaintenance (Line 2289), which safely handles null wallet managers

Conditionally schedule CJWalletManager maintenance only when it exists (Lines 2299-2303)

The separation of masternode-specific scheduling (Line 2296-2299) and wallet manager scheduling (Lines 2301-2303) is appropriate.

329-396: LGTM! Shutdown sequence properly handles new managers.

The shutdown logic correctly:

Unregisters cj_walletman from validation interface with null check (Lines 329-331)

Repositions g_ds_notification_interface cleanup for proper shutdown order (Lines 333-336)

Resets managers after wallet removal in the appropriate order: cj_walletman then dstxman (Lines 395-396)

The conditional checks ensure safe cleanup when managers are null (e.g., non-wallet builds or masternode mode).

2153-2163: Confirm CJWalletManager behavior in masternode mode

CDSTXManager is unconditionally instantiated (init.cpp:2153–2154).

CJWalletManager is only instantiated when !node.mn_activeman (init.cpp:2157–2160), leaving node.cj_walletman as nullptr in masternode mode.

PeerManager and CMasternodeUtils receive this nullptr without additional runtime guards.

Should masternodes support CoinJoin wallet functionality, or is excluding CJWalletManager here intentional? Please verify and document this behavior.

src/test/util/setup_common.cpp (5)

50-51: LGTM! Include updates align with the refactor.

The new includes for coinjoin/coinjoin.h and coinjoin/walletman.h correctly replace the old CJContext-based headers, consistent with the broader refactor introducing CJWalletManager.

337-337: LGTM! CDSTXManager initialization is correct.

The unconditional initialization of CDSTXManager in the node context aligns with the architectural change of moving it out of CJContext and into NodeContext.

338-343: LGTM! CJWalletManager initialization is correct.

The factory call to CJWalletManager::make with all required parameters is correct and matches the signature in src/coinjoin/walletman.cpp. The assertion ensures the manager was successfully created, which is appropriate given the #ifdef ENABLE_WALLET guard in the test environment.

354-366: LGTM! PeerManager construction is correct.

The updated PeerManager::make call correctly passes the new dstxman and cj_walletman parameters. The use of *m_node.dstxman (dereferenced) and m_node.cj_walletman.get() (raw pointer) matches the expected signature from src/net_processing.cpp. The nullptr values for mn_activeman and active_ctx are appropriate for the test environment.

373-386: LGTM! Destructor cleanup order is correct.

The reset order properly accounts for dependencies: peerman (which depends on dstxman and cj_walletman) is reset first, followed by banman, wallet-related components, cj_walletman, and finally dstxman. This ensures proper teardown without use-after-free issues.

src/coinjoin/client.cpp (5)

42-48: LGTM! ProcessMessage signature correctly updated.

The removal of the PeerManager& parameter aligns with the architectural change where DSQ messages are now returned via MessageProcessingResult instead of being relayed directly within this method.

139-139: LGTM! DSQ relaying correctly deferred to caller.

Appending dsq to ret.m_dsq instead of relaying directly via PeerManager is the correct pattern. This allows the caller (likely PeerManagerImpl) to handle relaying, consistent with the new message processing architecture.

166-177: LGTM! Constructor signature correctly simplified.

The removal of the is_masternode parameter streamlines the constructor, eliminating masternode-specific branching. The queueman parameter is correctly retained and initialized.

982-982: LGTM! Session construction correctly updated.

The emplace_back call correctly omits the is_masternode parameter, matching the updated CCoinJoinClientSession constructor signature at lines 166-177.

1877-1883: LGTM! Client manager construction correctly updated.

The CCoinJoinClientManager construction in CoinJoinWalletManager::Add correctly omits the is_masternode parameter, aligning with the updated constructor signature and the removal of masternode-specific branching.

src/coinjoin/walletman.cpp

src/net_processing.cpp

knst

LGTM overall except refactorings related to MessageProcessingResult and msg_processing.h

consider comments

src/coinjoin/context.h

knst · 2025-09-30T07:05:57Z

src/dsnotificationinterface.cpp

nit: seems unrelated (swap of order qdkgsman and qman)

knst · 2025-09-30T07:15:08Z

src/protocol.h

@@ -13,11 +13,15 @@
 #include <uint256.h>
 #include <util/time.h>

+#include <coinjoin/coinjoin.h>


coinjoin/coinjoin.h is a heavy header.

consider a way to avoid including it:

move CCoinJoinQueue to the own include

drop commit 5bb371f65c914bbcdc518937ce45ad1468e78ba7 completely

instead pushing CCoinJoinQueue object and calling after that PeerManagerImpl::RelayDSQ consider do some pre-processed data

Option 1 seems most attractive but would be out of scope for this PR.

I think I agree that coinjoin.h is too heavy to have in this header. I think this may be blocker

src/llmq/blockprocessor.h

knst · 2025-09-30T07:16:49Z

src/llmq/blockprocessor.h

 #include <bls/bls.h>
-#include <checkqueue.h>
 #include <llmq/params.h>
 #include <llmq/utils.h>
+#include <msg_processing.h>
+#include <unordered_lru_cache.h>
+
+#include <checkqueue.h>
 #include <protocol.h>
 #include <saltedhasher.h>
 #include <sync.h>


nit: strange order of headers, I can not understand grouping logic now

#include <bls/bls.h> #include <checkqueue.h> #include <llmq/params.h> #include <llmq/utils.h> #include <msg_processing.h> #include <protocol.h> #include <saltedhasher.h> #include <sync.h> #include <unordered_lru_cache.h> ```

checkqueue.h is not Dash-specific code (introduced in bitcoin#2060), unordered_lru_cache.h is (introduced in dash#2756).

checkqueue.h - util cmponent
unordered_lru_cache.h - also util component.

The whole file llmq/blockprocessor.h - our own, so, we don't have backports here. I see no reason to split it

knst · 2025-09-30T07:39:54Z

src/coinjoin/interfaces.cpp

@@ -77,9 +77,9 @@ class CoinJoinClientImpl : public interfaces::CoinJoin::Client
 class CoinJoinLoaderImpl : public interfaces::CoinJoin::Loader
 {
 private:
-    CoinJoinWalletManager& walletman()
+    CJContext& manager()


probably
-#include <coinjoin/walletman.h>
?

src/net_processing.cpp

knst · 2025-09-30T08:00:01Z

src/test/denialofservice_tests.cpp

@@ -151,7 +151,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
    auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, /*banman=*/nullptr, *m_node.dstxman,
                                       *m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync,
                                       *m_node.govman, *m_node.sporkman, /*mn_activeman=*/ nullptr, m_node.dmnman,
-                                       /*active_ctx=*/nullptr, m_node.cj_ctx, m_node.llmq_ctx, /*ignore_incoming_txs=*/false);
+                                       /*active_ctx=*/nullptr, /*cj_ctx=*/nullptr, m_node.llmq_ctx, /*ignore_incoming_txs=*/false);


nit: consider to create a helper here, which is used for this regression test only to simplify further refactorings and backports:

static std::unique_ptr<PeerManager> make(const CChainParams& chainparams, CConnman& connman, AddrMan& addrman, CDSTXManager& dstxman, ChainstateManager& chainman, CTxMemPool& pool, CMasternodeMetaMan& mn_metaman, CMasternodeSync& mn_sync, CGovernanceManager& govman, CSporkManager& sporkman, const std::unique_ptr<CDeterministicMNManager>& dmnman, const std::unique_ptr<ActiveContext>& active_ctx, const std::unique_ptr<CJContext>& cj_ctx, const std::unique_ptr<LLMQContext>& llmq_ctx, bool ignore_incoming_txs) { return PeerManager::make(chainparams, connman, addrman, /*banman=*/nullptr, dstxman, chainman, pool, mn_metaman, mn_sync, govman, sporkman, /*mn_activeman=*/nullptr, dmnman, /*active_ctx=*/nullptr, /*cj_ctx=*/nullptr, llmq_ctx, /*ignore_incoming_txs=*/false); }

so, you don't need to edit src/test/denialofservice_tests.cpp every time when any masternode's stuff implementation is changed

knst · 2025-09-30T08:04:12Z

src/coinjoin/client.cpp

@@ -167,21 +166,18 @@ void CCoinJoinClientManager::ProcessMessage(CNode& peer, CChainState& active_cha
 CCoinJoinClientSession::CCoinJoinClientSession(const std::shared_ptr<CWallet>& wallet, CCoinJoinClientManager& clientman,
                                               CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman,
                                               const CMasternodeSync& mn_sync, const llmq::CInstantSendManager& isman,
-                                               const std::unique_ptr<CCoinJoinClientQueueManager>& queueman,
-                                               bool is_masternode) :


removing is_masternode is a good catch! we don't have wallets on masternodes

src/coinjoin/context.cpp

knst · 2025-09-30T08:39:56Z

one more improvement,

$ git diff
diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp
index 9a1e1c4fa3..88d5ee4f81 100644
--- a/src/coinjoin/client.cpp
+++ b/src/coinjoin/client.cpp
@@ -12,7 +12,6 @@
 #include <masternode/meta.h>
 #include <masternode/sync.h>
 #include <net.h>
-#include <net_processing.h>
 #include <netmessagemaker.h>
 #include <rpc/evo_util.h>
 #include <shutdown.h>
diff --git a/src/coinjoin/client.h b/src/coinjoin/client.h
index 6b4f6a8456..550ebbf9bc 100644
--- a/src/coinjoin/client.h
+++ b/src/coinjoin/client.h
@@ -30,7 +30,6 @@ class CMasternodeSync;
 class CNode;
 class CoinJoinWalletManager;
 class CTxMemPool;
-class PeerManager;
 
 class UniValue;

$ test/lint/lint-circular-dependencies.py 
A new circular dependency in the form of "coinjoin/client -> coinjoin/coinjoin -> instantsend/instantsend -> net_processing -> coinjoin/walletman -> coinjoin/client" appears to have been introduced.

Good job! The circular dependency "coinjoin/client -> net_processing -> coinjoin/walletman -> coinjoin/client" is no longer present.
Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in /home/knst/projects/dash-reviews/test/lint/lint-circular-dependencies.py
to make sure this circular dependency is not accidentally reintroduced.

coderabbitai

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/net_processing.cpp (1)

1-1: Fix circular-dependencies lint script and update EXPECTED_CIRCULAR_DEPENDENCIES

Add the multiprocess package (or change the import to Python’s built-in multiprocessing) so that test/lint/lint-circular-dependencies.py runs without ModuleNotFoundError.

Remove all entries reported as “no longer present” by the script from the EXPECTED_CIRCULAR_DEPENDENCIES tuple in test/lint/lint-circular-dependencies.py.

♻️ Duplicate comments (3)

src/coinjoin/walletman.cpp (1)

58-66: Fix member initialization order in constructor.

The constructor passes queueman to walletman's constructor on line 63, but queueman is declared after walletman in the class definition (lines 54-55). This means queueman is uninitialized when passed to walletman, causing undefined behavior if walletman dereferences it during construction.

Swap the member declarations in the class definition so queueman is declared before walletman:
 private:
     const bool m_relay_txes;
 
+    const std::unique_ptr<CCoinJoinClientQueueManager> queueman;
     CoinJoinWalletManager walletman;
-    const std::unique_ptr<CCoinJoinClientQueueManager> queueman;
Then update the initializer list to match the declaration order:
     m_relay_txes{relay_txes},
-    walletman{chainman, dmnman, mn_metaman, mempool, mn_sync, isman, queueman},
-    queueman{m_relay_txes ? std::make_unique<CCoinJoinClientQueueManager>(walletman, dmnman, mn_metaman, mn_sync) : nullptr}
+    queueman{m_relay_txes ? std::make_unique<CCoinJoinClientQueueManager>(walletman, dmnman, mn_metaman, mn_sync) : nullptr},
+    walletman{chainman, dmnman, mn_metaman, mempool, mn_sync, isman, queueman}

src/net_processing.cpp (2)

787-789: Outdated comment (CJContext → CJWalletManager).

Update the doc to reflect the new type.

-    /** Pointer to this node's CJContext. May be nullptr - check existence before dereferencing. */
+    /** Pointer to this node's CJWalletManager. May be nullptr - check existence before dereferencing. */

5267-5272: DSQUEUE can be parsed twice from the same CDataStream (risk of stream misread and duplicate side effects).

Both m_cj_walletman->processMessage(...) and m_active_ctx->cj_server->ProcessMessage(...) receive the same vRecv for all msg_types; if they both handle DSQUEUE, the second sees an advanced stream.

Route DSQUEUE to exactly one consumer; e.g.:

-        if (m_cj_walletman) {
-            PostProcessMessage(m_cj_walletman->processMessage(pfrom, m_chainman.ActiveChainstate(), m_connman, m_mempool, msg_type, vRecv), pfrom.GetId());
-        }
-        if (m_active_ctx) {
-            PostProcessMessage(m_active_ctx->cj_server->ProcessMessage(pfrom, msg_type, vRecv), pfrom.GetId());
-        }
+        if (msg_type == NetMsgType::DSQUEUE) {
+            if (m_active_ctx) {
+                PostProcessMessage(m_active_ctx->cj_server->ProcessMessage(pfrom, msg_type, vRecv), pfrom.GetId());
+            } else if (m_cj_walletman) {
+                PostProcessMessage(m_cj_walletman->processMessage(pfrom, m_chainman.ActiveChainstate(), m_connman, m_mempool, msg_type, vRecv), pfrom.GetId());
+            }
+        } else {
+            if (m_cj_walletman) {
+                PostProcessMessage(m_cj_walletman->processMessage(pfrom, m_chainman.ActiveChainstate(), m_connman, m_mempool, msg_type, vRecv), pfrom.GetId());
+            }
+            if (m_active_ctx) {
+                PostProcessMessage(m_active_ctx->cj_server->ProcessMessage(pfrom, msg_type, vRecv), pfrom.GetId());
+            }
+        }

If overlap exists for other CJ messages, extend the routing set similarly. Alternatively add a “WantsMessage(msg_type)” probe to choose a single handler.

🧹 Nitpick comments (5)

src/init.cpp (1)
2153-2164: Add a defensive assert before CJWalletManager::make

We dereference node.llmq_ctx to pass isman; add an assert to make failures obvious if llmq_ctx isn’t initialized yet.
 assert(!node.dstxman);
 node.dstxman = std::make_unique<CDSTXManager>();

 assert(!node.cj_walletman);
- if (!node.mn_activeman) {
+ if (!node.mn_activeman) {
+     assert(node.llmq_ctx && node.llmq_ctx->isman);
      node.cj_walletman = CJWalletManager::make(chainman, *node.dmnman, *node.mn_metaman, *node.mempool, *node.mn_sync,
                                                *node.llmq_ctx->isman, !ignores_incoming_txs);
 }
 if (node.cj_walletman) {
     RegisterValidationInterface(node.cj_walletman.get());
 }
src/coinjoin/client.h (1)

77-88: Keep client.h light, but avoid risky enum fwd-decls

Given PoolState/PoolMessage are used in declarations, keeping coinjoin/coinjoin.h included here is acceptable. Prefer breaking the cycle at walletman.h (see prior comment) rather than aggressive forward-declarations that may fail if underlying enum types aren’t explicitly declared.

src/msg_processing.h (2)

39-43: Doc nit: grammar.

“These handlers do not supposed to know...” → “are not supposed to know...”.

8-8: Reduce public header coupling to coinjoin types. msg_processing.h still directly includes <coinjoin/coinjoin.h> and exposes CCoinJoinQueue in its public API. Extract a minimal forward declaration (e.g. coinjoin/coinjoin_fwd.h) or decouple the DSQ payload into a lightweight struct to shrink the dependency surface.
src/net_processing.cpp (1)
623-623: Stray double semicolon.

Minor cleanup.
-    void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);;
+    void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f786cb246fb358bc0eaeadac9c25ced3d56511a4 and 3dc919c9205ce18804460a505ce9e01a260b1f92.

📒 Files selected for processing (37)

src/Makefile.am (3 hunks)
src/coinjoin/client.cpp (5 hunks)
src/coinjoin/client.h (5 hunks)
src/coinjoin/context.cpp (0 hunks)
src/coinjoin/context.h (0 hunks)
src/coinjoin/interfaces.cpp (3 hunks)
src/coinjoin/server.cpp (1 hunks)
src/coinjoin/server.h (1 hunks)
src/coinjoin/walletman.cpp (1 hunks)
src/coinjoin/walletman.h (1 hunks)
src/evo/mnauth.h (1 hunks)
src/governance/governance.h (1 hunks)
src/init.cpp (6 hunks)
src/llmq/blockprocessor.h (1 hunks)
src/llmq/dkgsessionhandler.h (1 hunks)
src/llmq/dkgsessionmgr.h (1 hunks)
src/llmq/ehf_signals.h (1 hunks)
src/llmq/quorums.h (1 hunks)
src/llmq/signing.h (1 hunks)
src/llmq/signing_shares.h (1 hunks)
src/masternode/utils.cpp (3 hunks)
src/masternode/utils.h (1 hunks)
src/msg_processing.h (1 hunks)
src/net_processing.cpp (17 hunks)
src/net_processing.h (2 hunks)
src/node/context.cpp (1 hunks)
src/node/context.h (2 hunks)
src/protocol.h (0 hunks)
src/rpc/coinjoin.cpp (2 hunks)
src/spork.h (1 hunks)
src/test/coinjoin_dstxmanager_tests.cpp (2 hunks)
src/test/denialofservice_tests.cpp (4 hunks)
src/test/net_peer_connection_tests.cpp (1 hunks)
src/test/util/setup_common.cpp (4 hunks)
src/wallet/test/coinjoin_tests.cpp (2 hunks)
src/wallet/test/wallet_tests.cpp (0 hunks)
test/lint/lint-circular-dependencies.py (1 hunks)

💤 Files with no reviewable changes (4)

src/wallet/test/wallet_tests.cpp
src/coinjoin/context.cpp
src/protocol.h
src/coinjoin/context.h

✅ Files skipped from review due to trivial changes (1)

src/llmq/dkgsessionhandler.h

🚧 Files skipped from review as they are similar to previous changes (10)

src/llmq/blockprocessor.h
src/wallet/test/coinjoin_tests.cpp
src/rpc/coinjoin.cpp
test/lint/lint-circular-dependencies.py
src/coinjoin/server.h
src/coinjoin/server.cpp
src/governance/governance.h
src/spork.h
src/test/coinjoin_dstxmanager_tests.cpp
src/llmq/dkgsessionmgr.h

🧰 Additional context used

📓 Path-based instructions (3)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/test/denialofservice_tests.cpp
src/node/context.cpp
src/llmq/ehf_signals.h
src/test/net_peer_connection_tests.cpp
src/llmq/quorums.h
src/msg_processing.h
src/evo/mnauth.h
src/net_processing.h
src/masternode/utils.cpp
src/net_processing.cpp
src/coinjoin/walletman.h
src/coinjoin/walletman.cpp
src/coinjoin/client.h
src/llmq/signing.h
src/masternode/utils.h
src/coinjoin/client.cpp
src/llmq/signing_shares.h
src/node/context.h
src/test/util/setup_common.cpp
src/coinjoin/interfaces.cpp
src/init.cpp

src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Files:

src/test/denialofservice_tests.cpp
src/test/net_peer_connection_tests.cpp
src/test/util/setup_common.cpp

src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Masternode lists must use immutable data structures (Immer library) for thread safety

Files:

src/evo/mnauth.h
src/masternode/utils.cpp
src/masternode/utils.h

🧬 Code graph analysis (15)

src/test/denialofservice_tests.cpp (2)

src/coinjoin/walletman.cpp (3)

make (152-163)

make (152-155)

connman (33-33)

src/net_processing.cpp (2)

make (1954-1966)

make (1954-1963)

src/test/net_peer_connection_tests.cpp (2)

src/coinjoin/walletman.cpp (3)

make (152-163)

make (152-155)

connman (33-33)

src/net_processing.cpp (2)

make (1954-1966)

make (1954-1963)

src/msg_processing.h (2)

src/protocol.h (1)

CInv (529-566)

src/net_processing.cpp (6)

inv (625-625)

inv (626-626)

inv (645-645)

inv (651-651)

inv (936-936)

inv (2311-2311)

src/net_processing.h (3)

src/coinjoin/coinjoin.h (1)

CDSTXManager (378-406)

src/coinjoin/walletman.h (1)

CJWalletManager (36-64)

src/masternode/active/context.cpp (2)

ActiveContext (16-33)

ActiveContext (35-39)

src/masternode/utils.cpp (2)

src/coinjoin/client.cpp (6)

DoMaintenance (1818-1823)

DoMaintenance (1818-1818)

DoMaintenance (1825-1841)

DoMaintenance (1825-1825)

DoMaintenance (1884-1890)

DoMaintenance (1884-1884)

src/coinjoin/walletman.cpp (1)

connman (33-33)

src/net_processing.cpp (1)

src/coinjoin/walletman.cpp (5)

make (152-163)

make (152-155)

connman (33-33)

hash (36-36)

hash (40-40)

src/coinjoin/walletman.h (3)

src/coinjoin/interfaces.cpp (8)

wallet (98-102)

wallet (98-98)

name (103-107)

name (103-103)

name (108-111)

name (108-108)

name (112-116)

name (112-112)

src/coinjoin/walletman.cpp (32)

wallet (43-43)

make (152-163)

make (152-155)

Schedule (68-75)

Schedule (68-68)

connman (33-33)

hasQueue (86-92)

hasQueue (86-86)

hash (36-36)

hash (40-40)

getClient (94-97)

getClient (94-94)

name (37-37)

name (44-44)

name (45-45)

processMessage (99-110)

processMessage (99-101)

getQueueFromHash (112-118)

getQueueFromHash (112-112)

getQueueSize (120-126)

getQueueSize (120-120)

getMixingMasternodes (128-134)

getMixingMasternodes (128-128)

addWallet (136-139)

addWallet (136-136)

removeWallet (146-149)

removeWallet (146-146)

flushWallet (141-144)

flushWallet (141-141)

UpdatedBlockTip (77-84)

UpdatedBlockTip (77-77)

pindexNew (49-49)

src/net_processing.cpp (9)

make (1954-1966)

make (1954-1963)

scheduler (617-617)

scheduler (668-668)

hash (999-999)

hash (1005-1005)

UpdatedBlockTip (2117-2152)

UpdatedBlockTip (2117-2117)

pindexNew (602-602)

src/coinjoin/walletman.cpp (4)

src/net_processing.cpp (9)

peer (673-673)

peer (805-805)

peer (910-910)

peer (911-911)

peer (1018-1018)

peer (1026-1026)

ret (659-659)

make (1954-1966)

make (1954-1963)

src/coinjoin/interfaces.cpp (8)

name (103-107)

name (103-103)

name (108-111)

name (108-108)

name (112-116)

name (112-112)

wallet (98-102)

wallet (98-98)

src/coinjoin/walletman.h (1)

wallet (32-34)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

src/coinjoin/client.h (2)

src/net_processing.cpp (3)

ProcessMessage (3519-5302)

ProcessMessage (3519-3524)

from (1031-1031)

src/coinjoin/client.cpp (6)

ProcessMessage (41-140)

ProcessMessage (41-42)

ProcessMessage (142-163)

ProcessMessage (142-142)

ProcessMessage (178-224)

ProcessMessage (178-178)

src/masternode/utils.h (2)

src/coinjoin/walletman.h (1)

CJWalletManager (36-64)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

src/coinjoin/client.cpp (2)

src/net_processing.cpp (4)

ProcessMessage (3519-5302)

ProcessMessage (3519-3524)

from (1031-1031)

ret (659-659)

src/coinjoin/server.cpp (2)

ProcessMessage (26-40)

ProcessMessage (26-26)

src/node/context.h (2)

src/coinjoin/coinjoin.h (1)

CDSTXManager (378-406)

src/coinjoin/walletman.h (1)

CJWalletManager (36-64)

src/test/util/setup_common.cpp (2)

src/coinjoin/walletman.cpp (2)

make (152-163)

make (152-155)

src/net_processing.cpp (2)

make (1954-1966)

make (1954-1963)

src/coinjoin/interfaces.cpp (1)

src/coinjoin/walletman.cpp (4)

wallet (43-43)

name (37-37)

name (44-44)

name (45-45)

src/init.cpp (3)

src/net_processing.cpp (11)

node (609-609)

node (610-610)

node (635-635)

node (763-763)

node (863-867)

node (878-878)

node (889-889)

node (900-900)

node (908-908)

make (1954-1966)

make (1954-1963)

src/coinjoin/walletman.cpp (2)

make (152-163)

make (152-155)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

🪛 GitHub Actions: Clang Diff Format Check

src/coinjoin/walletman.cpp

[error] 91-91: Clang format differences detected in this file. Apply formatting changes to match the after-format version.

[error] 133-133: Clang format differences detected in this file. Apply formatting changes to match the after-format version.

src/coinjoin/client.h

[error] 245-245: Clang format differences detected in this file. Apply formatting changes to match the after-format version.

src/coinjoin/interfaces.cpp

[error] 77-77: Clang format differences detected in this file. Apply formatting changes to match the after-format version.

[error] 105-105: Clang format differences detected in this file. Apply formatting changes to match the after-format version.

[error] 133-133: Clang format differences detected in this file. Apply formatting changes to match the after-format version.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

GitHub Check: linux64_multiprocess-build / Build source
GitHub Check: mac-build / Build source
GitHub Check: linux64_sqlite-build / Build source
GitHub Check: linux64_fuzz-build / Build source
GitHub Check: linux64_ubsan-build / Build source
GitHub Check: linux64-build / Build source
GitHub Check: linux64_nowallet-build / Build source
GitHub Check: arm-linux-build / Build source
GitHub Check: linux64_tsan-build / Build source
GitHub Check: win64-build / Build source

🔇 Additional comments (49)

src/masternode/utils.h (1)

11-11: LGTM! Proper forward declaration and signature update.

The forward declaration correctly uses class for CJWalletManager, and the signature change from reference to pointer properly reflects the optional nature of the wallet manager (can be nullptr in non-wallet builds).

Also applies to: 16-17

src/Makefile.am (1)

185-185: LGTM! Build configuration properly updated for the refactor.

The addition of coinjoin/walletman.h, msg_processing.h, and coinjoin/walletman.cpp correctly reflects the architectural changes. Note that walletman.cpp is added to libbitcoin_node_a_SOURCES (node library) rather than the wallet library, which is appropriate for an interface/manager abstraction.

Also applies to: 294-294, 489-489

src/masternode/utils.cpp (4)

11-13: LGTM! Proper includes for the refactored dependencies.

All new includes are used in the implementation and necessary for the refactored manager-based architecture.

15-16: LGTM! Signature correctly updated to match header.

The parameter change from reference to pointer properly supports optional wallet manager (can be nullptr in non-wallet builds), with appropriate null-checking at line 39.

39-39: LGTM! Proper null-safe conditional logic.

The ternary correctly guards against dereferencing a null cj_walletman pointer, falling back to an empty vector when the wallet manager is unavailable. This allows the function to operate correctly in both wallet and non-wallet builds.

70-72: LGTM! Correct usage of conditionally-derived mixing masternodes.

The logic correctly uses the mixing_masternodes vector derived at line 39, maintaining the same disconnection behavior while adapting to the new manager-based architecture.

src/coinjoin/walletman.h (1)

1-67: LGTM! Well-designed interface.

The CJWalletManager interface is cleanly designed:

Proper separation between public API and protected validation interface overrides

Factory method pattern appropriately returns std::unique_ptr and handles ENABLE_WALLET guard in the implementation

Virtual destructor is correctly defaulted

All methods are appropriately pure virtual

Forward declarations minimize header dependencies

The interface successfully abstracts wallet-specific CoinJoin logic from non-wallet code.

src/coinjoin/walletman.cpp (9)

68-75: Schedule method correctly guards against null queueman.

The method properly checks m_relay_txes before dereferencing queueman, ensuring safe access.

77-84: Block tip update logic is correct.

The early return conditions properly handle IBD and disconnected blocks, and the iteration over client managers is appropriate.

94-97: Client retrieval is straightforward.

Direct delegation to walletman.Get() is appropriate.

99-110: Message processing correctly delegates to components.

The method appropriately iterates over client managers and conditionally processes queue messages.

112-118: Queue lookup is properly guarded.

Returns std::nullopt when queueman is null, which is correct.

120-126: Queue size retrieval is properly guarded.

Returns std::nullopt when queueman is null, matching the pattern used elsewhere.

136-139: Wallet addition is straightforward.

Direct delegation to walletman.Add() is appropriate.

141-149: Wallet lifecycle methods are correctly implemented.

Both flushWallet and removeWallet properly delegate to the underlying walletman.

152-163: Factory method correctly handles ENABLE_WALLET guard.

Returns nullptr when wallet support isn't built, allowing calling code to handle the absence gracefully.

src/evo/mnauth.h (1)

9-9: Include is necessary for MessageProcessingResult return type.

The #include <msg_processing.h> is required because the ProcessMessage method on line 63 returns MessageProcessingResult. This aligns with the PR's objective to extract message processing types to avoid circular dependencies.

src/node/context.cpp (1)

9-10: LGTM!

The replacement of coinjoin/context.h with coinjoin/coinjoin.h and coinjoin/walletman.h aligns with the PR's objective to refactor CJContext into separate wallet and DSTX managers.

src/llmq/quorums.h (1)

14-14: LGTM!

Adding msg_processing.h is necessary for the MessageProcessingResult return type used by ProcessMessage (line 273). This change aligns with the PR's goal of centralizing message processing types to break circular dependencies.

src/llmq/ehf_signals.h (1)

9-9: LGTM!

Adding msg_processing.h is necessary for the MessageProcessingResult return type used by HandleNewRecoveredSig (line 47). This change aligns with the PR's refactor to centralize message processing types.

src/llmq/signing.h (1)

12-12: LGTM!

Adding msg_processing.h is necessary for the MessageProcessingResult return type used by ProcessMessage (line 185) and CRecoveredSigsListener::HandleNewRecoveredSig (line 154). This change aligns with the PR's refactor to centralize message processing types and break circular dependencies.

src/llmq/signing_shares.h (1)

10-11: LGTM!

Reordering the llmq/signing.h include and adding msg_processing.h aligns with the PR's refactor to centralize message processing types. The MessageProcessingResult type is used by HandleNewRecoveredSig (line 440).
src/test/denialofservice_tests.cpp (1)
151-155: LGTM! Parameter wiring updated correctly.

All four PeerManager::make call sites have been updated to include the new dstxman parameter and replace cj_ctx with cj_walletman (passed as nullptr in tests). The parameter ordering aligns with the updated signature in src/net_processing.cpp.

Note: A previous reviewer suggested creating a test helper function to wrap PeerManager::make and simplify future refactorings/backports. While not blocking, consider implementing this helper to reduce maintenance burden:
static std::unique_ptr<PeerManager> MakeTestPeerManager(
    const CChainParams& chainparams, CConnman& connman, AddrMan& addrman,
    BanMan* banman, TestingSetup& setup)
{
    return PeerManager::make(
        chainparams, connman, addrman, banman, *setup.m_node.dstxman,
        *setup.m_node.chainman, *setup.m_node.mempool, *setup.m_node.mn_metaman,
        *setup.m_node.mn_sync, *setup.m_node.govman, *setup.m_node.sporkman,
        /*mn_activeman=*/nullptr, setup.m_node.dmnman,
        /*active_ctx=*/nullptr, /*cj_walletman=*/nullptr,
        setup.m_node.llmq_ctx, /*ignore_incoming_txs=*/false);
}
This would isolate test code from future PeerManager::make signature changes.

Based on learnings.

Also applies to: 256-260, 324-328, 430-434
src/node/context.h (1)

21-21: LGTM! Manager-based wiring introduced correctly.

The addition of CDSTXManager and CJWalletManager forward declarations and the corresponding std::unique_ptr members in NodeContext align with the refactoring goals. This replaces the previous CJContext-based wiring with a cleaner separation between DSTX management and wallet-specific CoinJoin logic.

Also applies to: 26-26, 84-85

src/test/net_peer_connection_tests.cpp (1)

88-92: LGTM! Parameter wiring updated correctly.

The PeerManager::make call has been updated to include the new dstxman parameter and replace cj_ctx with cj_walletman (passed as nullptr in this test context). The parameter ordering aligns with the updated signature.

src/coinjoin/interfaces.cpp (1)

9-9: Clarification: Include is necessary.

A previous reviewer suggested removing #include <coinjoin/walletman.h>, but this include is required. The manager() method (lines 80-83) returns CJWalletManager& and dereferences m_node.cj_walletman, which requires the complete type definition. Without this include, the code would fail to compile with an incomplete type error.

src/net_processing.h (2)

13-13: LGTM! Header dependencies updated correctly.

The addition of #include <msg_processing.h> and forward declarations for CDSTXManager and CJWalletManager are appropriate for the new manager-based architecture. These changes support the interface updates in PeerManager::make without exposing implementation details.

Also applies to: 22-22, 26-26

62-62: LGTM! Signature updated consistently.

The PeerManager::make signature has been updated to include CDSTXManager& dstxman and CJWalletManager* const cj_walletman, replacing the previous CJContext-based parameter. The use of a raw pointer for cj_walletman (vs. reference for dstxman) appropriately allows nullptr in non-wallet builds, aligning with the conditional initialization pattern.

Also applies to: 68-68

src/init.cpp (5)

329-337: Shutdown: correct unregistration order for CJ/DS interfaces

Unregistering cj_walletman before DS notifications is the right order; avoids callbacks into torn-down objects. LGTM.

395-397: Manager teardown is explicit

Resetting cj_walletman and dstxman here is good; guarantees no dangling pointers post-wallet removal. LGTM.

2166-2171: PeerManager wiring looks correct

Passing dstxman and (possibly null) cj_walletman aligns with the new factory signature. LGTM.

Please run basic P2P startup to confirm no null-derefs when wallet is disabled.

2172-2175: DS notifications correctly depend on dstxman

CDSNotificationInterface now uses dstxman; creation point and lifetime look good. LGTM.

2289-2304: Scheduling guards are sound

CMasternodeUtils::DoMaintenance receives cj_walletman pointer (may be null).

cj_walletman->Schedule called only when present.

This keeps wallet concerns out of masternode-only nodes. LGTM.

src/test/util/setup_common.cpp (3)

337-353: Test wiring: construct managers early and consistently

Creating dstxman and cj_walletman before peerman is correct. Consider registering cj_walletman as a validation interface in tests that rely on UpdatedBlockTip-style callbacks; otherwise current setup is fine.

354-366: PeerManager init OK

peerman is built with dstxman and cj_walletman and wired to connman via Options.m_msgproc. LGTM.

375-386: Destructor order is safe

peerman → banman → wallet clients → cj_walletman → dstxman ensures no dangling references. LGTM.

src/coinjoin/client.cpp (5)

41-46: ProcessMessage signature reduction is coherent

Dropping PeerManager& from CCoinJoinClientQueueManager::ProcessMessage matches the new relay-via-return contract. LGTM.

138-139: Return DSQ for centralized relay

ret.m_dsq.push_back(dsq) is the right way to hand DSQ back to net_processing for relay. LGTM.

978-983: Session emplace with new ctor is correct

deqSessions.emplace_back(...) with queueman added looks good. LGTM.

1878-1882: Wallet manager Add: pass queueman

Passing m_queueman into CCoinJoinClientManager aligns with new design. LGTM.

546-551: Remove redundant include
coinjoin/coinjoin.h is already brought in via #include <coinjoin/client.h>, so no additional include is needed.

Likely an incorrect or invalid review comment.
src/coinjoin/client.h (3)
238-251: Locking contract annotations

ProcessMessage annotated EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue) matches implementation (internal lock acquisition). LGTM.

245-251: Apply clang-format to src/coinjoin/client.h
Install clang-format (e.g. via your package manager) and run:
clang-format -i src/coinjoin/client.h
to clear the CI formatting diff.

8-16: Could not locate circular-deps lint tool—please manually verify the cycle is broken

I didn’t find a dedicated circular-deps lint configuration or script in the repo. After applying the suggested forward-declaration in coinjoin/walletman.h and moving the #include <coinjoin/client.h> into walletman.cpp, please run your circular-dependency checker and confirm that the cycle between coinjoin/client, coinjoin/coinjoin, instantsend/instantsend, net_processing, and coinjoin/walletman has been eliminated.
src/net_processing.cpp (5)

3499-3503: Good: DSQ relaying centralized via PostProcessMessage.

Relaying queues through RelayDSQ from MessageProcessingResult looks clean and keeps net_processing authoritative for gossip.

2447-2448: Correct inv typing for DSTX.

Using m_dstxman to select MSG_DSTX vs MSG_TX is consistent across relay paths.

Please ensure GetDSTX(txid) is O(1) (likely a map lookup) to avoid hot-path regressions.

Also applies to: 6099-6102, 6175-6178

2236-2239: DSQ presence checks are coherent.

AlreadyHave and GETDATA DSQ retrieval consult both walletman and server; nice symmetry.

Also applies to: 2876-2883

1954-1966: Factory/ctor wiring LGTM.

New PeerManager::make and PeerManagerImpl constructor properly thread dstxman and cj_walletman through.

Also applies to: 1968-1999

16-22: Includes: sanity.

Added headers align with new uses (txindex, netmessagemaker, validation, coinjoin/*, msg_processing.h). No issues spotted.

Also applies to: 38-41, 43-51, 61-66, 68-77

src/coinjoin/walletman.cpp

src/msg_result.h

github-actions · 2025-09-30T13:54:33Z

This pull request has conflicts, please rebase.

coderabbitai

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/coinjoin/interfaces.cpp (1)
1-127: Fix clang-format violations before merge.

The pipeline reports clang-format differences in this file. These formatting issues must be resolved.

Run the following script to apply formatting:
#!/bin/bash
# Apply clang-format to fix formatting issues in coinjoin/interfaces.cpp
clang-format -i src/coinjoin/interfaces.cpp
src/coinjoin/client.cpp (1)
1295-1305: Guard against empty vecInputsByRounds before dereference (possible UB).

If no entry succeeded in the loop above, vecInputsByRounds is empty and begin()->first is undefined.

Fix:
-    int nRounds = vecInputsByRounds.begin()->first;
-    if (PrepareDenominate(nRounds, nRounds, strError, vecTxDSIn, vecPSInOutPairsTmp)) {
+    if (!vecInputsByRounds.empty()) {
+        const int nRounds = vecInputsByRounds.begin()->first;
+        if (PrepareDenominate(nRounds, nRounds, strError, vecTxDSIn, vecPSInOutPairsTmp)) {
+            WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- Running CoinJoin denominate for %d rounds, success\n", nRounds);
+            return SendDenominate(vecPSInOutPairsTmp, connman);
+        }
+    }
-        WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- Running CoinJoin denominate for %d rounds, success\n", nRounds);
-        return SendDenominate(vecPSInOutPairsTmp, connman);
-    }
The existing fallback below (PrepareDenominate(0, ...)) will cover the empty case.

♻️ Duplicate comments (3)

src/msg_processing.h (1)
14-17: Missing include for std::forward (compile blocker).

The template constructor on line 30 uses std::forward but <utility> is not included, which will cause a compilation error.

Apply this diff:
 #include <optional>
 #include <string>
+#include <utility>
 #include <variant>
 #include <vector>
src/net_processing.cpp (1)
5356-5361: Recheck DSQUEUE double-consumption risk (wallet vs server)

Both m_cj_walletman->processMessage(...) and m_active_ctx->cj_server->ProcessMessage(...) receive the same vRecv. If their handled msg_type sets overlap (e.g., DSQUEUE), the second consumer reads an advanced stream.

If overlap remains, route DSQUEUE to exactly one handler (preferred), or pass a copy of vRecv to the second (less ideal).

Quick overlap check:
#!/bin/bash
# Identify DSQUEUE handling in wallet manager and server
rg -nP --type=cpp -C3 'processMessage\s*$.*msg_type.*$' src/coinjoin
rg -nP --type=cpp -C3 'NetMsgType::DSQUEUE|NetMsgType::DSQ|DSQUEUE' src/coinjoin
If overlap is confirmed, a minimal router in net_processing for DSQUEUE avoids double reads.
src/coinjoin/walletman.cpp (1)

1-1: Formatting diffs detected — run clang-format on this file.

CI flagged clang-format differences; please format the file to project style.

Also applies to: 86-92, 128-134

🧹 Nitpick comments (3)

src/test/denialofservice_tests.cpp (1)

151-155: Consider introducing a test helper for PeerManager::make.

As noted in past review comments, this test file requires updates whenever the PeerManager::make signature changes. A dedicated test helper that wraps PeerManager::make with commonly-used test defaults would simplify future refactorings and backports.

Example helper:
namespace {
std::unique_ptr<PeerManager> MakeTestPeerManager(
    const CChainParams& chainparams,
    ConnmanTestMsg& connman,
    TestingSetup& setup)
{
    return PeerManager::make(
        chainparams, connman, *setup.m_node.addrman,
        /*banman=*/nullptr, *setup.m_node.dstxman,
        *setup.m_node.chainman, *setup.m_node.mempool,
        *setup.m_node.mn_metaman, *setup.m_node.mn_sync,
        *setup.m_node.govman, *setup.m_node.sporkman,
        /*mn_activeman=*/nullptr, setup.m_node.dmnman,
        /*active_ctx=*/nullptr, /*cj_walletman=*/nullptr,
        setup.m_node.llmq_ctx, /*ignore_incoming_txs=*/false);
}
} // anonymous namespace
Then call sites would simplify to:
auto peerLogic = MakeTestPeerManager(chainparams, *connman, *this);

src/net_processing.cpp (2)

2453-2453: De-duplicate MSG_DSTX vs MSG_TX selection

The ternary m_dstxman.GetDSTX(txid) ? MSG_DSTX : MSG_TX appears multiple times. Extract a small helper to centralize policy and avoid repeated lookups.

@@ class PeerManagerImpl final : public PeerManager
 private:
+    inline int InvTypeForTx(const uint256& txid) const {
+        return m_dstxman.GetDSTX(txid) ? MSG_DSTX : MSG_TX;
+    }
@@ void PeerManagerImpl::_RelayTransaction(const uint256& txid)
-    const CInv inv{m_dstxman.GetDSTX(txid) ? MSG_DSTX : MSG_TX, txid};
+    const CInv inv{InvTypeForTx(txid), txid};
@@ if (fSendTrickle && tx_relay->m_send_mempool) {
-                    int nInvType = m_dstxman.GetDSTX(hash) ? MSG_DSTX : MSG_TX;
+                    int nInvType = InvTypeForTx(hash);
@@ while (!vInvTx.empty() && nRelayedTransactions < broadcast_max) {
-                    int nInvType = m_dstxman.GetDSTX(hash) ? MSG_DSTX : MSG_TX;
+                    int nInvType = InvTypeForTx(hash);

Also applies to: 6190-6192, 6266-6268

3586-3591: Tiny nit: make PLATFORM_BAN_REQUESTID_PREFIX static constexpr

Avoid re-allocating the prefix string on each call.

-    const std::string PLATFORM_BAN_REQUESTID_PREFIX = "PlatformPoSeBan";
+    static constexpr char PLATFORM_BAN_REQUESTID_PREFIX[] = "PlatformPoSeBan";

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3dc919c9205ce18804460a505ce9e01a260b1f92 and 00a8d0c9c88de5f334ee75b98088a45a85625d41.

📒 Files selected for processing (39)

src/Makefile.am (3 hunks)
src/coinjoin/client.cpp (5 hunks)
src/coinjoin/client.h (5 hunks)
src/coinjoin/context.cpp (0 hunks)
src/coinjoin/context.h (0 hunks)
src/coinjoin/interfaces.cpp (3 hunks)
src/coinjoin/server.cpp (1 hunks)
src/coinjoin/server.h (1 hunks)
src/coinjoin/walletman.cpp (1 hunks)
src/coinjoin/walletman.h (1 hunks)
src/dsnotificationinterface.cpp (6 hunks)
src/dsnotificationinterface.h (2 hunks)
src/evo/mnauth.h (1 hunks)
src/governance/governance.h (1 hunks)
src/init.cpp (6 hunks)
src/llmq/blockprocessor.h (1 hunks)
src/llmq/dkgsessionhandler.h (1 hunks)
src/llmq/dkgsessionmgr.h (1 hunks)
src/llmq/ehf_signals.h (1 hunks)
src/llmq/quorums.h (1 hunks)
src/llmq/signing.h (1 hunks)
src/llmq/signing_shares.h (1 hunks)
src/masternode/utils.cpp (3 hunks)
src/masternode/utils.h (1 hunks)
src/msg_processing.h (1 hunks)
src/net_processing.cpp (17 hunks)
src/net_processing.h (2 hunks)
src/node/context.cpp (1 hunks)
src/node/context.h (2 hunks)
src/protocol.h (0 hunks)
src/rpc/coinjoin.cpp (2 hunks)
src/spork.h (1 hunks)
src/test/coinjoin_dstxmanager_tests.cpp (2 hunks)
src/test/denialofservice_tests.cpp (4 hunks)
src/test/net_peer_connection_tests.cpp (1 hunks)
src/test/util/setup_common.cpp (4 hunks)
src/wallet/test/coinjoin_tests.cpp (2 hunks)
src/wallet/test/wallet_tests.cpp (0 hunks)
test/lint/lint-circular-dependencies.py (1 hunks)

💤 Files with no reviewable changes (4)

src/wallet/test/wallet_tests.cpp
src/protocol.h
src/coinjoin/context.cpp
src/coinjoin/context.h

✅ Files skipped from review due to trivial changes (2)

src/llmq/dkgsessionmgr.h
src/llmq/dkgsessionhandler.h

🚧 Files skipped from review as they are similar to previous changes (11)

src/coinjoin/server.h
src/llmq/ehf_signals.h
src/node/context.h
src/coinjoin/server.cpp
src/masternode/utils.h
src/test/net_peer_connection_tests.cpp
src/spork.h
src/coinjoin/client.h
src/node/context.cpp
src/Makefile.am
src/llmq/signing_shares.h

🧰 Additional context used

📓 Path-based instructions (3)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/governance/governance.h
src/test/denialofservice_tests.cpp
src/test/coinjoin_dstxmanager_tests.cpp
src/coinjoin/interfaces.cpp
src/llmq/quorums.h
src/dsnotificationinterface.h
src/rpc/coinjoin.cpp
src/coinjoin/walletman.cpp
src/dsnotificationinterface.cpp
src/coinjoin/client.cpp
src/llmq/blockprocessor.h
src/wallet/test/coinjoin_tests.cpp
src/net_processing.h
src/msg_processing.h
src/evo/mnauth.h
src/masternode/utils.cpp
src/net_processing.cpp
src/test/util/setup_common.cpp
src/init.cpp
src/coinjoin/walletman.h
src/llmq/signing.h

src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Files:

src/test/denialofservice_tests.cpp
src/test/coinjoin_dstxmanager_tests.cpp
src/wallet/test/coinjoin_tests.cpp
src/test/util/setup_common.cpp

src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Masternode lists must use immutable data structures (Immer library) for thread safety

Files:

src/evo/mnauth.h
src/masternode/utils.cpp

🧠 Learnings (1)

📚 Learning: 2025-07-23T09:30:34.631Z

Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.

Applied to files:

src/llmq/blockprocessor.h

🧬 Code graph analysis (16)

src/test/denialofservice_tests.cpp (2)

src/coinjoin/walletman.cpp (3)

make (152-163)

make (152-155)

connman (33-33)

src/net_processing.cpp (2)

make (1958-1970)

make (1958-1967)

src/test/coinjoin_dstxmanager_tests.cpp (1)

src/coinjoin/interfaces.cpp (2)

Assert (80-83)

Assert (85-88)

src/coinjoin/interfaces.cpp (3)

src/node/interfaces.cpp (18)

Assert (92-92)

Assert (93-93)

Assert (117-117)

Assert (259-259)

Assert (282-282)

Assert (404-404)

Assert (613-616)

Assert (825-825)

Assert (1131-1134)

name (1090-1093)

name (1090-1090)

name (1094-1097)

name (1094-1094)

name (1098-1101)

name (1098-1098)

name (1102-1111)

name (1102-1102)

name (1112-1122)

src/coinjoin/walletman.cpp (4)

wallet (43-43)

name (37-37)

name (44-44)

name (45-45)

src/coinjoin/walletman.h (1)

wallet (32-34)

src/dsnotificationinterface.h (2)

src/coinjoin/coinjoin.h (1)

CDSTXManager (378-406)

src/dsnotificationinterface.cpp (1)

CDSNotificationInterface (23-39)

src/rpc/coinjoin.cpp (1)

src/interfaces/node.h (1)

node (41-43)

src/coinjoin/walletman.cpp (3)

src/net_processing.cpp (9)

peer (674-674)

peer (809-809)

peer (914-914)

peer (915-915)

peer (1022-1022)

peer (1030-1030)

ret (660-660)

make (1958-1970)

make (1958-1967)

src/coinjoin/interfaces.cpp (8)

name (103-107)

name (103-103)

name (108-111)

name (108-108)

name (112-116)

name (112-112)

wallet (98-102)

wallet (98-98)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

src/dsnotificationinterface.cpp (3)

src/dsnotificationinterface.h (1)

CDSNotificationInterface (20-57)

src/node/interfaces.cpp (11)

chainman (576-578)

chainman (1061-1063)

Assert (92-92)

Assert (93-93)

Assert (117-117)

Assert (259-259)

Assert (282-282)

Assert (404-404)

Assert (613-616)

Assert (825-825)

Assert (1131-1134)

src/coinjoin/coinjoin.cpp (4)

BlockConnected (486-494)

BlockConnected (486-486)

BlockDisconnected (496-503)

BlockDisconnected (496-496)

src/coinjoin/client.cpp (1)

src/net_processing.cpp (4)

ProcessMessage (3609-5392)

ProcessMessage (3609-3614)

from (1035-1035)

ret (660-660)

src/wallet/test/coinjoin_tests.cpp (1)

src/coinjoin/interfaces.cpp (2)

Assert (80-83)

Assert (85-88)

src/net_processing.h (3)

src/coinjoin/coinjoin.h (1)

CDSTXManager (378-406)

src/coinjoin/walletman.h (1)

CJWalletManager (36-64)

src/masternode/active/context.cpp (2)

ActiveContext (16-33)

ActiveContext (35-39)

src/msg_processing.h (1)

src/protocol.h (1)

CInv (531-568)

src/masternode/utils.cpp (3)

src/coinjoin/client.cpp (6)

DoMaintenance (1818-1823)

DoMaintenance (1818-1818)

DoMaintenance (1825-1841)

DoMaintenance (1825-1825)

DoMaintenance (1884-1890)

DoMaintenance (1884-1884)

src/evo/deterministicmns.cpp (2)

DoMaintenance (1364-1371)

DoMaintenance (1364-1364)

src/masternode/sync.cpp (2)

DoMaintenance (367-372)

DoMaintenance (367-367)

src/net_processing.cpp (1)

src/coinjoin/walletman.cpp (5)

make (152-163)

make (152-155)

connman (33-33)

hash (36-36)

hash (40-40)

src/test/util/setup_common.cpp (2)

src/coinjoin/walletman.cpp (2)

make (152-163)

make (152-155)

src/net_processing.cpp (2)

make (1958-1970)

make (1958-1967)

src/init.cpp (3)

src/net_processing.cpp (12)

node (610-610)

node (611-611)

node (636-636)

node (732-732)

node (767-767)

node (867-871)

node (882-882)

node (893-893)

node (904-904)

node (912-912)

make (1958-1970)

make (1958-1967)

src/coinjoin/walletman.cpp (2)

make (152-163)

make (152-155)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

src/coinjoin/walletman.h (3)

src/coinjoin/interfaces.cpp (8)

wallet (98-102)

wallet (98-98)

name (103-107)

name (103-103)

name (108-111)

name (108-108)

name (112-116)

name (112-112)

src/coinjoin/walletman.cpp (32)

wallet (43-43)

make (152-163)

make (152-155)

Schedule (68-75)

Schedule (68-68)

connman (33-33)

hasQueue (86-92)

hasQueue (86-86)

hash (36-36)

hash (40-40)

getClient (94-97)

getClient (94-94)

name (37-37)

name (44-44)

name (45-45)

processMessage (99-110)

processMessage (99-101)

getQueueFromHash (112-118)

getQueueFromHash (112-112)

getQueueSize (120-126)

getQueueSize (120-120)

getMixingMasternodes (128-134)

getMixingMasternodes (128-128)

addWallet (136-139)

addWallet (136-136)

removeWallet (146-149)

removeWallet (146-146)

flushWallet (141-144)

flushWallet (141-141)

UpdatedBlockTip (77-84)

UpdatedBlockTip (77-77)

pindexNew (49-49)

src/net_processing.cpp (9)

make (1958-1970)

make (1958-1967)

scheduler (618-618)

scheduler (669-669)

hash (1003-1003)

hash (1009-1009)

UpdatedBlockTip (2121-2156)

UpdatedBlockTip (2121-2121)

pindexNew (603-603)

🪛 GitHub Actions: Clang Diff Format Check

src/coinjoin/interfaces.cpp

[error] 1-1: Clang format differences found in src/coinjoin/interfaces.cpp. Run the clang-format-diff.py script or clang-format to update formatting.

src/coinjoin/walletman.cpp

[error] 1-1: Clang format differences found in src/coinjoin/walletman.cpp. Run the clang-format-diff.py script or clang-format to update formatting.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

GitHub Check: linux64_multiprocess-build / Build source
GitHub Check: mac-build / Build source
GitHub Check: linux64_tsan-build / Build source
GitHub Check: win64-build / Build source
GitHub Check: linux64_sqlite-build / Build source
GitHub Check: linux64_ubsan-build / Build source
GitHub Check: linux64-build / Build source
GitHub Check: linux64_fuzz-build / Build source
GitHub Check: linux64_nowallet-build / Build source
GitHub Check: arm-linux-build / Build source

🔇 Additional comments (44)

src/msg_processing.h (1)

34-76: LGTM!

The MessageProcessingResult struct is well-designed with clear documentation explaining its purpose as a return type for message handlers outside net_processing.cpp. The multiple fields support various processing outcomes, and the constructors provide convenient initialization paths.

src/llmq/quorums.h (1)

14-14: LGTM!

The include change from protocol.h to msg_processing.h correctly aligns with the introduction of MessageProcessingResult used in the ProcessMessage method at line 273. This is part of the broader refactoring to standardize message processing results.

src/llmq/signing.h (1)

12-12: LGTM!

The include change from protocol.h to msg_processing.h correctly supports the MessageProcessingResult return type used in methods like ProcessMessage (line 185) and HandleNewRecoveredSig (line 154). This aligns with the project-wide message processing refactoring.

src/evo/mnauth.h (1)

9-10: LGTM!

The addition of msg_processing.h correctly supports the MessageProcessingResult return type in the ProcessMessage method (line 63). The blank line at line 10 improves readability by separating the new include from the existing ones.

src/governance/governance.h (1)

10-10: LGTM!

The inclusion of msg_processing.h correctly enables the use of MessageProcessingResult in public method signatures (SyncSingleObjVotes at line 274, SyncObjects at line 275, and ProcessMessage at line 277). This is part of the standardized message processing result pattern being adopted project-wide.

src/llmq/blockprocessor.h (1)

8-17: LGTM: Include reordering supports new message processing types.

The addition of msg_processing.h and reorganization of includes correctly supports the MessageProcessingResult return type used by ProcessMessage at line 62. The reordering is consistent with the PR's extraction of message processing types to avoid circular dependencies.

src/test/coinjoin_dstxmanager_tests.cpp (2)

41-41: LGTM: Direct dstxman access aligns with new node context architecture.

Replacing m_node.cj_ctx->dstxman with m_node.dstxman correctly reflects the PR's move of CDSTXManager into NodeContext.

55-55: LGTM: Consistent dstxman access pattern.

The updated access pattern is consistent with the first test case and the new node context structure.

src/rpc/coinjoin.cpp (2)

6-6: LGTM: Updated include aligns with new wallet manager interface.

Replacing coinjoin/context.h with coinjoin/walletman.h correctly reflects the shift to the CJWalletManager interface introduced in this PR.

475-479: LGTM: Defensive queue size retrieval through wallet manager.

The conditional retrieval via node.cj_walletman->getQueueSize() is more defensive than the previous unconditional access, correctly handling cases where the wallet manager or queue size may not be available.

src/wallet/test/coinjoin_tests.cpp (2)

9-9: LGTM: Include added for new wallet manager interface.

Adding coinjoin/walletman.h provides the CJWalletManager interface required by the updated test code.

224-230: LGTM: Client access updated to use new wallet manager API.

The refactored access via m_node.cj_walletman->getClient("") correctly uses the new CJWalletManager interface. Test assertions remain unchanged, confirming functional equivalence.

src/coinjoin/interfaces.cpp (6)

9-9: LGTM: Include updated for new wallet manager interface.

Replacing coinjoin/context.h with coinjoin/walletman.h correctly reflects the transition to the CJWalletManager interface.

80-82: LGTM: Manager accessor correctly wired to new node context.

The renamed manager() method correctly accesses m_node.cj_walletman and returns the CJWalletManager& interface. The logic is sound.

100-100: LGTM: Wallet addition delegates to manager.

The call to manager().addWallet(wallet) correctly uses the new wallet manager interface.

105-105: LGTM: Wallet removal delegates to manager.

The call to manager().removeWallet(name) correctly uses the new wallet manager interface.

110-110: LGTM: Wallet flush delegates to manager.

The call to manager().flushWallet(name) correctly uses the new wallet manager interface.

114-114: LGTM: Client retrieval delegates to manager.

The call to manager().getClient(name) correctly uses the new wallet manager interface to retrieve the client manager.
src/masternode/utils.cpp (4)
11-13: LGTM! Clean dependency management.

The added includes are appropriate for the new function signature and implementation that now references CJWalletManager, deterministic masternode management, and sync state.

15-16: Function signature updated correctly.

The signature change from CJContext to CJWalletManager* const cj_walletman aligns with the PR's architectural shift to abstract wallet-only CoinJoin managers behind an interface. The const pointer parameter is appropriate here.

70-72: LGTM! Cleaner range-based check.

The switch from a loop over vecDmns to ranges::any_of over mixing_masternodes is more idiomatic and maintains the same logic: protect nodes that are mixing masternodes from disconnection.

39-39: Confirmed getMixingMasternodes() in CJWalletManager. It’s declared as
virtual std::vector<CDeterministicMNCPtr> getMixingMasternodes() = 0;
in src/coinjoin/walletman.h and implemented in CJWalletManagerImpl, so the nullptr-safe call is valid.
src/net_processing.h (3)

13-13: LGTM! Required include for MessageProcessingResult.

The msg_processing.h include is necessary for the MessageProcessingResult type used in PostProcessMessage (line 128).

18-32: LGTM! Forward declarations updated correctly.

The forward declarations have been properly updated to include CDSTXManager and CJWalletManager while removing CJContext. The ordering and completeness of declarations are appropriate.

61-69: LGTM! PeerManager::make signature updated consistently.

The signature changes correctly reflect the architectural shift:

CDSTXManager& dstxman inserted after BanMan* banman (required, passed by reference)

CJWalletManager* const cj_walletman replaces the previous CJContext parameter (optional, passed as const pointer)

This aligns with the PR objectives to move CDSTXManager into NodeContext and abstract wallet-only managers behind an interface.

src/dsnotificationinterface.h (3)

12-12: LGTM! Forward declaration added.

The CDSTXManager forward declaration is appropriate for the class member reference declared on line 49.

23-27: LGTM! Constructor signature updated correctly.

The constructor signature has been updated to accept CDSTXManager& dstxman after CConnman& connman, aligning with the PR's objective to move CDSTXManager into NodeContext and wire it through notification interfaces.

49-49: LGTM! Member variable properly declared.

The m_dstxman member is correctly declared as a reference to CDSTXManager, consistent with the constructor parameter and the class's role in handling blockchain events that affect DSTX state.
test/lint/lint-circular-dependencies.py (1)
34-35: Unable to verify automatically—script dependency missing
The circular-dependencies.py run failed with ModuleNotFoundError: No module named 'multiprocess'. Please install the Python multiprocess package (or update the script to use multiprocessing), re-run the lint tool, and confirm that the simple path
coinjoin/client -> net_processing -> coinjoin/walletman -> coinjoin/client
no longer appears in EXPECTED_CIRCULAR_DEPENDENCIES.
src/dsnotificationinterface.cpp (7)

5-21: LGTM!

The addition of #include <util/check.h> is necessary for the Assert() calls used throughout the file.

23-39: LGTM!

The constructor correctly wires CDSTXManager via the new dstxman parameter, aligning with the PR's refactoring objectives. Member initialization order is consistent with the header declaration.

49-49: LGTM!

The Assert(m_llmq_ctx) usage is consistent with the codebase pattern for dereferencing unique_ptr references in critical paths.

63-63: LGTM!

The Assert(m_dmnman) usage is consistent with the codebase pattern for dereferencing unique_ptr references in critical paths.

91-91: LGTM!

The Assert(m_llmq_ctx) calls are consistent with the codebase pattern for safely dereferencing unique_ptr references in critical paths.

Also applies to: 99-99, 104-104, 111-111, 127-127

76-76: LGTM!

The CDSTXManager integration is correct. All method calls use the new m_dstxman member, and the signatures are consistent with the implementation in src/coinjoin/coinjoin.cpp.

Also applies to: 93-93, 106-106, 113-113, 128-128

81-81: LGTM!

The qman->UpdatedBlockTip call correctly includes the m_connman parameter as part of the refactoring.

src/coinjoin/walletman.h (5)

1-15: LGTM!

The include guards follow project conventions, and all includes are necessary for the interface definition. The inclusion of msg_processing.h aligns with the PR's extraction of MessageProcessingResult.

16-34: LGTM!

All forward declarations are used in the interface and appropriately minimize header dependencies.

36-43: LGTM!

The class design is sound: inheriting from CValidationInterface for blockchain notifications, using a factory pattern for conditional compilation, and providing a virtual destructor for proper polymorphic cleanup. The factory signature is consistent with the implementation in walletman.cpp.

45-59: LGTM!

The public API is well-designed with appropriate const-correctness, use of std::optional for potentially absent values, and clear separation of concerns. The processMessage method correctly returns MessageProcessingResult, aligning with the PR's refactoring objectives.

60-67: LGTM!

The protected UpdatedBlockTip override is correctly declared as part of the CValidationInterface contract. File structure and include guards are properly formed.

src/net_processing.cpp (1)

2278-2278: Ignore null-check for cj_server – ActiveContext’s constructor always initializes cj_server as a non-null unique_ptr; guarding it is redundant when m_active_ctx is non-null. No change needed.

Likely an incorrect or invalid review comment.

src/init.cpp (1)

2134-2151: Manager wiring and lifecycle look consistent.

Creation, registration, scheduling, and shutdown for CDSTXManager and CJWalletManager are correctly sequenced and gated. LGTM.

Please sanity-check on:

Masternode mode: node.cj_walletman should remain null and no Schedule() call executed.

Blocksonly: CJWalletManager constructed with relay_txes=false; ensure Schedule() early-returns as intended and no CoinJoin maintenance runs.

Also applies to: 2153-2157, 2269-2285

src/test/util/setup_common.cpp (1)

338-367: Test setup updated correctly for new manager interface.

PeerManager construction with dstxman and cj_walletman and connman->Init wiring look good. Teardown resets peerman, cj_walletman, and dstxman in a safe order.

Run the unit tests that previously touched CoinJoin paths to confirm no regressions in fixture startup/shutdown.

Also applies to: 374-387

src/coinjoin/walletman.cpp

src/net_processing.cpp

coderabbitai · 2025-09-30T18:35:28Z

test/lint/lint-circular-dependencies.py

@@ -27,12 +27,12 @@
    "chainlock/chainlock -> instantsend/instantsend -> instantsend/signing -> chainlock/chainlock",
    "chainlock/chainlock -> instantsend/instantsend -> net_processing -> chainlock/chainlock",
    "chainlock/chainlock -> instantsend/instantsend -> net_processing -> masternode/active/context -> chainlock/chainlock",
+    "chainlock/chainlock -> llmq/quorums -> msg_processing -> coinjoin/coinjoin -> chainlock/chainlock",


⚠️ Potential issue | 🟠 Major

New circular dependency introduced.

A new circular dependency has been added:
chainlock/chainlock -> llmq/quorums -> msg_processing -> coinjoin/coinjoin -> chainlock/chainlock

This is acknowledged in the past review comments where you clarified that the total number of circular dependencies remains unchanged. However, ensure that the introduction of msg_processing.h and the extraction of MessageProcessingResult and MisbehavingError to avoid one circular dependency hasn't inadvertently created this new one without addressing the root cause.

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (3)

src/coinjoin/walletman.cpp (1)
52-66: Initialization order pattern is safe but deserves documentation.

The constructor passes queueman (a reference to std::unique_ptr<CCoinJoinClientQueueManager>) to walletman's constructor before queueman is initialized. This pattern is safe because:

queueman as a member has its storage allocated before walletman's constructor runs (members initialize in declaration order)

The reference binds to the unique_ptr storage itself, not the pointed-to object

Usage sites properly null-check before dereferencing (e.g., lines 88-89, 106-107)

However, this subtle pattern warrants a comment explaining why the initialization order is safe, as it's non-obvious to reviewers.

Consider adding a comment above the constructor explaining the initialization pattern:
+    // Note: queueman is passed by reference to walletman's constructor before being initialized.
+    // This is safe because the reference binds to the unique_ptr's storage (allocated in declaration
+    // order), and walletman only dereferences it after null-checking.
     CJWalletManagerImpl(ChainstateManager& chainman, CDeterministicMNManager& dmnman,
Based on past review discussion with kwvg.
src/test/denialofservice_tests.cpp (1)
151-155: Consider extracting a test helper for PeerManager creation.

All four call sites mechanically add dstxman and cj_walletman parameters to PeerManager::make. As suggested in past review comments, a local helper function would simplify future refactorings and backports:
namespace {
std::unique_ptr<PeerManager> MakePeerManagerForTest(
    const CChainParams& chainparams, CConnman& connman, AddrMan& addrman,
    BanMan* banman, TestingSetup& setup)
{
    return PeerManager::make(chainparams, connman, addrman, banman, *setup.m_node.dstxman,
                            *setup.m_node.chainman, *setup.m_node.mempool, *setup.m_node.mn_metaman,
                            *setup.m_node.mn_sync, *setup.m_node.govman, *setup.m_node.sporkman,
                            /*mn_activeman=*/nullptr, setup.m_node.dmnman, /*active_ctx=*/nullptr,
                            /*cj_walletman=*/nullptr, setup.m_node.llmq_ctx, /*ignore_incoming_txs=*/false);
}
} // anonymous namespace
Then each call site becomes:
auto peerLogic = MakePeerManagerForTest(chainparams, *connman, *m_node.addrman, /*banman=*/nullptr, *this);
Based on past review comments.

Also applies to: 256-260, 324-328, 430-434
src/init.cpp (1)
2134-2144: Consider adding logging if CJWalletManager creation fails.

Lines 2138-2141 create cj_walletman only in non-masternode mode. However, CJWalletManager::make() can return nullptr even when ENABLE_WALLET is defined and we're not in masternode mode.

Consider adding a log entry if creation fails unexpectedly:
     assert(!node.cj_walletman);
     if (!node.mn_activeman) {
         node.cj_walletman = CJWalletManager::make(chainman, *node.dmnman, *node.mn_metaman, *node.mempool, *node.mn_sync,
                                                   *node.llmq_ctx->isman, !ignores_incoming_txs);
+        if (!node.cj_walletman) {
+            LogPrintf("Warning: Failed to create CJWalletManager (wallet support may be unavailable)\n");
+        }
     }
     if (node.cj_walletman) {
         RegisterValidationInterface(node.cj_walletman.get());
This would help diagnose unexpected failures in non-masternode configurations while keeping the nullptr case valid for masternode mode.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 00a8d0c9c88de5f334ee75b98088a45a85625d41 and eb666d9.

📒 Files selected for processing (19)

src/Makefile.am (3 hunks)
src/coinjoin/interfaces.cpp (3 hunks)
src/coinjoin/walletman.cpp (1 hunks)
src/coinjoin/walletman.h (1 hunks)
src/init.cpp (6 hunks)
src/masternode/utils.cpp (3 hunks)
src/masternode/utils.h (1 hunks)
src/net_processing.cpp (17 hunks)
src/net_processing.h (2 hunks)
src/node/context.cpp (1 hunks)
src/node/context.h (2 hunks)
src/rpc/coinjoin.cpp (2 hunks)
src/test/coinjoin_dstxmanager_tests.cpp (2 hunks)
src/test/denialofservice_tests.cpp (4 hunks)
src/test/net_peer_connection_tests.cpp (1 hunks)
src/test/util/setup_common.cpp (4 hunks)
src/wallet/test/coinjoin_tests.cpp (2 hunks)
src/wallet/test/wallet_tests.cpp (0 hunks)
test/lint/lint-circular-dependencies.py (1 hunks)

💤 Files with no reviewable changes (1)

src/wallet/test/wallet_tests.cpp

🚧 Files skipped from review as they are similar to previous changes (4)

src/test/coinjoin_dstxmanager_tests.cpp
src/coinjoin/interfaces.cpp
src/rpc/coinjoin.cpp
src/test/net_peer_connection_tests.cpp

🧰 Additional context used

📓 Path-based instructions (3)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/node/context.h
src/test/denialofservice_tests.cpp
src/wallet/test/coinjoin_tests.cpp
src/masternode/utils.cpp
src/test/util/setup_common.cpp
src/node/context.cpp
src/init.cpp
src/net_processing.h
src/coinjoin/walletman.cpp
src/masternode/utils.h
src/coinjoin/walletman.h
src/net_processing.cpp

src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Files:

src/test/denialofservice_tests.cpp
src/wallet/test/coinjoin_tests.cpp
src/test/util/setup_common.cpp

src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Masternode lists must use immutable data structures (Immer library) for thread safety

Files:

src/masternode/utils.cpp
src/masternode/utils.h

🧠 Learnings (2)

📚 Learning: 2025-10-02T18:29:54.717Z

Learnt from: kwvg
PR: dashpay/dash#6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.717Z
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/net_processing.cpp

📚 Learning: 2025-10-02T18:29:54.716Z

Learnt from: kwvg
PR: dashpay/dash#6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.716Z
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/net_processing.cpp

🧬 Code graph analysis (10)

src/node/context.h (2)

src/coinjoin/coinjoin.h (1)

CDSTXManager (378-406)

src/coinjoin/walletman.h (1)

CJWalletManager (36-64)

src/test/denialofservice_tests.cpp (2)

src/coinjoin/walletman.cpp (3)

make (152-163)

make (152-155)

connman (33-33)

src/net_processing.cpp (2)

make (1958-1970)

make (1958-1967)

src/wallet/test/coinjoin_tests.cpp (1)

src/coinjoin/interfaces.cpp (2)

Assert (80-83)

Assert (85-88)

src/test/util/setup_common.cpp (2)

src/coinjoin/walletman.cpp (2)

make (152-163)

make (152-155)

src/net_processing.cpp (2)

make (1958-1970)

make (1958-1967)

src/init.cpp (3)

src/net_processing.cpp (12)

node (610-610)

node (611-611)

node (636-636)

node (732-732)

node (767-767)

node (867-871)

node (882-882)

node (893-893)

node (904-904)

node (912-912)

make (1958-1970)

make (1958-1967)

src/coinjoin/walletman.cpp (2)

make (152-163)

make (152-155)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

src/net_processing.h (3)

src/coinjoin/coinjoin.h (1)

CDSTXManager (378-406)

src/coinjoin/walletman.h (1)

CJWalletManager (36-64)

src/masternode/active/context.cpp (2)

ActiveContext (16-33)

ActiveContext (35-39)

src/coinjoin/walletman.cpp (3)

src/net_processing.cpp (9)

peer (674-674)

peer (809-809)

peer (914-914)

peer (915-915)

peer (1022-1022)

peer (1030-1030)

ret (660-660)

make (1958-1970)

make (1958-1967)

src/coinjoin/interfaces.cpp (8)

name (103-107)

name (103-103)

name (108-111)

name (108-108)

name (112-116)

name (112-112)

wallet (98-102)

wallet (98-98)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

src/masternode/utils.h (2)

src/coinjoin/walletman.h (1)

CJWalletManager (36-64)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

src/coinjoin/walletman.h (3)

src/coinjoin/interfaces.cpp (8)

wallet (98-102)

wallet (98-98)

name (103-107)

name (103-103)

name (108-111)

name (108-108)

name (112-116)

name (112-112)

src/coinjoin/walletman.cpp (32)

wallet (43-43)

make (152-163)

make (152-155)

Schedule (68-75)

Schedule (68-68)

connman (33-33)

hasQueue (86-92)

hasQueue (86-86)

hash (36-36)

hash (40-40)

getClient (94-97)

getClient (94-94)

name (37-37)

name (44-44)

name (45-45)

processMessage (99-110)

processMessage (99-101)

getQueueFromHash (112-118)

getQueueFromHash (112-112)

getQueueSize (120-126)

getQueueSize (120-120)

getMixingMasternodes (128-134)

getMixingMasternodes (128-128)

addWallet (136-139)

addWallet (136-136)

removeWallet (146-149)

removeWallet (146-146)

flushWallet (141-144)

flushWallet (141-141)

UpdatedBlockTip (77-84)

UpdatedBlockTip (77-77)

pindexNew (49-49)

src/net_processing.cpp (9)

make (1958-1970)

make (1958-1967)

scheduler (618-618)

scheduler (669-669)

hash (1003-1003)

hash (1009-1009)

UpdatedBlockTip (2121-2156)

UpdatedBlockTip (2121-2121)

pindexNew (603-603)

src/net_processing.cpp (1)

src/coinjoin/walletman.cpp (5)

make (152-163)

make (152-155)

connman (33-33)

hash (36-36)

hash (40-40)

🪛 GitHub Actions: Clang Diff Format Check

src/coinjoin/walletman.cpp

[error] 1-1: Clang format differences found in 'src/coinjoin/walletman.cpp'. Run 'clang-format -i src/coinjoin/walletman.cpp' to fix formatting.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

GitHub Check: linux64_multiprocess-build / Build source
GitHub Check: linux64_tsan-build / Build source
GitHub Check: mac-build / Build source
GitHub Check: arm-linux-build / Build source
GitHub Check: win64-build / Build source
GitHub Check: linux64_nowallet-build / Build source
GitHub Check: linux64_sqlite-build / Build source
GitHub Check: linux64_ubsan-build / Build source
GitHub Check: linux64_fuzz-build / Build source
GitHub Check: linux64-build / Build source

🔇 Additional comments (31)

src/coinjoin/walletman.cpp (1)
99-109: Verify stream reuse behavior is intentional.

The code passes the same vRecv stream to multiple client managers via ForEachCJClientMan. If ProcessMessage consumes the stream (advances its read position), subsequent clients will receive a partially consumed or empty stream. This may cause the first client to process the message while others skip it.

Based on the past review discussion where kwvg asked if this is intended behavior, please confirm:

Is passing the same stream reference intentional?

Does each client's ProcessMessage reset the stream position, or is only the first client expected to process it?

If stream consumption is the issue, the fix would be:
     walletman.ForEachCJClientMan([&](std::unique_ptr<CCoinJoinClientManager>& clientman) {
+        CDataStream vRecvCopy = vRecv; // fresh copy per client
-        clientman->ProcessMessage(pfrom, chainstate, connman, mempool, msg_type, vRecv);
+        clientman->ProcessMessage(pfrom, chainstate, connman, mempool, msg_type, vRecvCopy);
     });
Based on past review comments.
src/node/context.cpp (1)

9-10: LGTM: Include updates align with refactoring.

The replacement of coinjoin/context.h with coinjoin/coinjoin.h and coinjoin/walletman.h correctly reflects the architectural shift from CJContext to the new manager-based design.

src/Makefile.am (1)

185-185: LGTM: Build configuration updated correctly.

The additions of coinjoin/walletman.h, msg_processing.h to headers and coinjoin/walletman.cpp to sources align with the refactoring that replaces coinjoin/context.{h,cpp} with the new manager-based architecture.

Also applies to: 294-294, 489-489

src/node/context.h (1)

21-21: LGTM: Manager types properly integrated into NodeContext.

The additions of CDSTXManager and CJWalletManager with appropriate forward declarations cleanly separate:

Node-level DSTX management (dstxman)

Wallet-specific CoinJoin management (cj_walletman, optional behind ENABLE_WALLET)

This replaces the previous CJContext with a more modular design.

Also applies to: 26-26, 84-85

src/wallet/test/coinjoin_tests.cpp (1)

9-9: LGTM: Test updated to use new wallet manager interface.

The test correctly transitions from cj_ctx->walletman->Get("") to cj_walletman->getClient(""), using Assert to safely dereference the returned pointer. The test logic remains unchanged, validating IsMixing() state transitions.

Also applies to: 224-230

src/masternode/utils.h (1)

11-11: LGTM: Signature correctly updated to use optional wallet manager.

The change from const CJContext& to CJWalletManager* const properly reflects that the wallet manager is optional (only available when ENABLE_WALLET is defined). The implementation in utils.cpp null-checks before dereferencing (line 24 in relevant snippet).

Also applies to: 16-17

test/lint/lint-circular-dependencies.py (1)

30-30: Old resolved circular dependencies already removed. No further updates to EXPECTED_CIRCULAR_DEPENDENCIES are needed.

src/masternode/utils.cpp (4)

11-13: LGTM!

The new includes are correctly added and align with the refactored interface design.

15-16: LGTM!

The signature change from CJContext to CJWalletManager* const cj_walletman correctly reflects the new manager-based architecture and allows for nullable wallet managers.

39-39: LGTM!

The nullable check and fallback to an empty vector when cj_walletman is null is safe and correct.

70-72: LGTM!

The use of ranges::any_of with mixing_masternodes is safe even when the vector is empty, and correctly checks for mixing masternodes before disconnection.

src/net_processing.h (3)

13-13: LGTM!

The inclusion of msg_processing.h is necessary for MessageProcessingResult used in PostProcessMessage.

18-32: LGTM!

The forward declarations for CDSTXManager and CJWalletManager are correctly added and align with the new manager-based architecture.

62-68: Constructor parameters match factory signature
The PeerManagerImpl constructor accepts CDSTXManager& dstxman and CJWalletManager* const cj_walletman, matching the PeerManager::make signature.

src/coinjoin/walletman.h (6)

1-7: LGTM!

The file header and include guards are correctly formatted.

8-34: LGTM!

The includes and forward declarations are appropriate and minimal, reducing compilation dependencies.

36-43: LGTM!

The interface design is sound:

Inheriting from CValidationInterface allows the manager to receive blockchain notifications.

The static factory method make follows the factory pattern and returns a unique_ptr, ensuring proper ownership semantics.

The virtual destructor is correctly declared as default.

46-46: LGTM!

The Schedule method provides a clean way to register scheduled tasks with the scheduler.

62-63: LGTM!

The protected UpdatedBlockTip override is correctly declared as pure virtual, ensuring implementations handle blockchain updates.

49-58: Manual verification required for circular dependency
Lint check failed with ModuleNotFoundError: No module named 'multiprocess', preventing conclusive results. Please manually confirm the cycle
coinjoin/client -> coinjoin/coinjoin -> instantsend/instantsend -> net_processing -> coinjoin/walletman -> coinjoin/client
is not present before merging.

src/net_processing.cpp (1)

5354-5362: Ensure CJ wallet/server handlers don't consume the same stream

Both m_cj_walletman->processMessage(...) and m_active_ctx->cj_server->ProcessMessage(...) still receive the identical CDataStream& vRecv. If a wallet-enabled masternode is running, both handlers will touch CoinJoin messages like DSQUEUE. The first consumer advances vRecv, so the second sees an already‑consumed payload and fails to parse, recreating the double-read bug we were trying to eliminate. Please gate dispatch so only one handler runs per message, or hand one of them a copy/reset stream before parsing.
src/test/util/setup_common.cpp (4)
51-52: LGTM! Include updates align with the new manager architecture.

The replacement of coinjoin/context.h with coinjoin/coinjoin.h and coinjoin/walletman.h correctly reflects the shift from CJContext to the modular CDSTXManager and CJWalletManager abstractions.

339-343: Verify the assertion on Line 343 is appropriate.

CJWalletManager::make() can return nullptr even under ENABLE_WALLET if construction fails internally. The assertion at Line 343 will terminate the test process if cj_walletman is null, which may be too aggressive for test setup.

Consider replacing the assertion with a conditional check that logs an error or skips wallet-dependent tests:
 #ifdef ENABLE_WALLET
     // The test suite doesn't use masternode mode, so we may initialize it
     m_node.cj_walletman = CJWalletManager::make(*m_node.chainman, *m_node.dmnman, *m_node.mn_metaman, *m_node.mempool,
                                                 *m_node.mn_sync, *m_node.llmq_ctx->isman, /*relay_txes=*/true);
-    assert(m_node.cj_walletman);
+    if (!m_node.cj_walletman) {
+        LogPrintf("Warning: Failed to create CJWalletManager in test setup\n");
+    }
 
     // WalletInit::Construct()-like logic needed for wallet tests that run on
Alternatively, verify whether CJWalletManager::make() is guaranteed to succeed in the test environment and document this assumption.

Based on learnings: A previous comment noted similar concerns about assertions on manager creation.

355-366: LGTM! PeerManager wiring correctly updated.

The PeerManager creation properly wires the new managers:

dstxman is correctly dereferenced (guaranteed non-null from Line 338)

cj_walletman.get() correctly passes a raw pointer that may be nullptr if wallet support is disabled or creation failed, which the PeerManager signature accepts

Initialization order is correct: managers → PeerManager → CConnman initialization.

374-386: LGTM! Proper RAII teardown order.

The destructor correctly tears down resources in reverse initialization order:

peerman (depends on managers)

banman

Wallet chain clients (under ENABLE_WALLET)

cj_walletman (wallet-specific manager)

dstxman (foundational node-level manager)

This ordering prevents use-after-free errors and ensures clean shutdown.
src/init.cpp (6)
82-82: LGTM! Include update aligns with manager architecture.

The addition of coinjoin/walletman.h is consistent with the architectural shift from CJContext to the modular CJWalletManager abstraction.

329-336: Verify the validation interface unregistration order.

The current unregistration order is:

cj_walletman (Lines 329-331)

g_ds_notification_interface (Lines 333-336)

However, the registration order in AppInitMain is:

cj_walletman (Line 2143)

g_ds_notification_interface (Line 2156)

Best practice for validation interfaces typically suggests unregistering in reverse registration order (LIFO) to ensure dependent interfaces are cleaned up first.

Consider reordering to unregister g_ds_notification_interface before cj_walletman:
+    if (g_ds_notification_interface) {
+        UnregisterValidationInterface(g_ds_notification_interface.get());
+        g_ds_notification_interface.reset();
+    }
+
     if (node.cj_walletman) {
         UnregisterValidationInterface(node.cj_walletman.get());
     }

-    if (g_ds_notification_interface) {
-        UnregisterValidationInterface(g_ds_notification_interface.get());
-        g_ds_notification_interface.reset();
-    }
Verify whether the current order could cause issues if g_ds_notification_interface depends on cj_walletman being registered.

393-396: LGTM! Proper manager cleanup order.

The manager resets occur after:

Wallet chain clients are cleared (Line 391)

Validation interfaces are unregistered (Lines 329-336)

PeerManager is destroyed (Line 296)

The order (cj_walletman → dstxman) correctly reflects reverse initialization order and ensures dependent components are cleaned up before foundational managers.

2146-2156: LGTM! PeerManager and DSNotificationInterface wiring is correct.

The initialization properly wires the new managers:

dstxman is dereferenced safely (guaranteed non-null from Line 2135)

cj_walletman.get() correctly passes a nullable pointer as expected by PeerManager's signature

Validation interface registration order is correct

DSNotificationInterface correctly receives dstxman by reference

2158-2167: LGTM! ActiveContext properly wired with dstxman.

In masternode mode, the ActiveContext constructor correctly receives dstxman by reference (Line 2162). The dereference is safe as dstxman is guaranteed non-null from Line 2135.

This change integrates DSTX management into masternode-specific context as intended by the refactoring.

2270-2270: LGTM! Maintenance scheduling correctly handles both modes.

The scheduling logic properly separates concerns:

Line 2270: CMasternodeUtils::DoMaintenance now receives cj_walletman as a nullable pointer, which is appropriate since it may not exist in masternode mode

Lines 2277-2280: Masternode-specific maintenance (CoinJoinServer, DKG cleanup) scheduled only in masternode mode

Lines 2282-2284: cj_walletman scheduling occurs outside the masternode block, correctly applying only when the manager exists (non-masternode mode)

This separation ensures wallet-related CoinJoin maintenance runs only when appropriate.

Also applies to: 2277-2284

coderabbitai · 2025-10-02T18:39:37Z

src/coinjoin/walletman.cpp

@@ -0,0 +1,163 @@
+// Copyright (c) 2023-2025 The Dash Core developers


⚠️ Potential issue | 🟡 Minor

Apply clang-format to fix formatting.

The pipeline detected formatting differences in this file. Run clang-format -i src/coinjoin/walletman.cpp to ensure the file matches the project's formatting standards.

Based on pipeline failures.

🧰 Tools

🪛 GitHub Actions: Clang Diff Format Check

[error] 1-1: Clang format differences found in 'src/coinjoin/walletman.cpp'. Run 'clang-format -i src/coinjoin/walletman.cpp' to fix formatting.

🤖 Prompt for AI Agents

In src/coinjoin/walletman.cpp around lines 1 to 1, the file formatting does not match the project's clang-format rules; run clang-format -i src/coinjoin/walletman.cpp to reformat the file, then review the diff and commit the updated file so the pipeline formatting check passes.

src/coinjoin/context.cpp

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (3)

src/test/coinjoin_queue_tests.cpp (1)

8-8: Include is correct; fix formatting and consider trimming dependencies.

coinjoin/queue.h include is expected after moving CCoinJoinQueue.

Apply clang-format to resolve the CI diff failure.

Optional: if only queue + denomination helpers are needed, prefer including coinjoin/common.h + coinjoin/queue.h instead of coinjoin/coinjoin.h to reduce compile deps.

src/coinjoin/params.h (1)

1-13: Centralized timeouts look good; format and consider stronger typing.

Apply clang-format (CI flagged).

Optional: use std::chrono::seconds to strongly type timeouts.

src/coinjoin/queue.h (1)

28-35: Consider encapsulating public data members.

All data members are public, which exposes internal state and reduces encapsulation. For a simple data structure like CCoinJoinQueue, this is acceptable, but consider making data members private with getter/setter methods if this class evolves to include more complex invariants or validation logic.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eb666d9 and b1c28ce.

📒 Files selected for processing (10)

src/Makefile.am (3 hunks)
src/coinjoin/coinjoin.cpp (2 hunks)
src/coinjoin/coinjoin.h (2 hunks)
src/coinjoin/params.h (1 hunks)
src/coinjoin/queue.cpp (1 hunks)
src/coinjoin/queue.h (1 hunks)
src/msg_processing.h (1 hunks)
src/test/coinjoin_basemanager_tests.cpp (1 hunks)
src/test/coinjoin_inouts_tests.cpp (1 hunks)
src/test/coinjoin_queue_tests.cpp (1 hunks)

✅ Files skipped from review due to trivial changes (1)

src/test/coinjoin_inouts_tests.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

src/msg_processing.h

🧰 Additional context used

📓 Path-based instructions (2)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/coinjoin/params.h
src/test/coinjoin_queue_tests.cpp
src/coinjoin/coinjoin.cpp
src/coinjoin/queue.h
src/coinjoin/queue.cpp
src/coinjoin/coinjoin.h
src/test/coinjoin_basemanager_tests.cpp

src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Files:

src/test/coinjoin_queue_tests.cpp
src/test/coinjoin_basemanager_tests.cpp

🧬 Code graph analysis (2)

src/coinjoin/queue.h (3)

src/masternode/node.cpp (1)

CActiveMasternodeManager (52-61)

src/coinjoin/queue.cpp (14)

GetHash (21-24)

GetHash (21-21)

GetSignatureHash (16-19)

GetSignatureHash (16-16)

Sign (26-35)

Sign (26-26)

CheckSignature (37-44)

CheckSignature (37-37)

IsTimeOutOfBounds (46-50)

IsTimeOutOfBounds (46-46)

IsTimeOutOfBounds (52-55)

IsTimeOutOfBounds (52-52)

ToString (57-61)

ToString (57-57)

src/coinjoin/coinjoin.cpp (6)

GetSignatureHash (45-48)

GetSignatureHash (45-45)

Sign (50-60)

Sign (50-50)

CheckSignature (62-70)

CheckSignature (62-62)

src/coinjoin/queue.cpp (3)

src/coinjoin/coinjoin.cpp (6)

GetSignatureHash (45-48)

GetSignatureHash (45-45)

Sign (50-60)

Sign (50-50)

CheckSignature (62-70)

CheckSignature (62-62)

src/bls/bls.h (1)

CBLSSignature (346-349)

src/coinjoin/queue.h (3)

nTime (31-31)

fReady (32-70)

fTried (35-54)

🪛 GitHub Actions: Clang Diff Format Check

src/coinjoin/params.h

[error] 1-1: Clang format differences found. Apply clang-format to fix formatting in this file.

src/test/coinjoin_queue_tests.cpp

[error] 1-1: Clang format differences found. Apply clang-format to fix formatting in this file.

src/coinjoin/queue.h

[error] 1-1: Clang format differences found. Apply clang-format to fix formatting in this file.

src/coinjoin/queue.cpp

[error] 1-1: Clang format differences found. Apply clang-format to fix formatting in this file.

src/coinjoin/coinjoin.h

[error] 1-1: Clang format differences found. Apply clang-format to fix formatting in this file.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

GitHub Check: linux64_sqlite-build / Build source
GitHub Check: linux64_ubsan-build / Build source
GitHub Check: linux64-build / Build source
GitHub Check: linux64_fuzz-build / Build source
GitHub Check: arm-linux-build / Build source
GitHub Check: mac-build / Build source
GitHub Check: win64-build / Build source
GitHub Check: linux64_tsan-build / Build source
GitHub Check: linux64_nowallet-build / Build source
GitHub Check: Lint / Run linters

🔇 Additional comments (9)

src/test/coinjoin_basemanager_tests.cpp (1)

8-8: LGTM: queue header inclusion added.

This aligns tests with the CCoinJoinQueue move.

src/coinjoin/coinjoin.h (2)

9-10: New includes are reasonable; verify include layering and run clang-format.

Including queue.h here is necessary due to std::vector.

Please verify coinjoin/queue.h does not include coinjoin/coinjoin.h to avoid header include cycles.

Apply clang-format (CI flagged).

Per PR notes, remove the resolved “coinjoin/client -> net_processing -> coinjoin/walletman -> coinjoin/client” cycle from EXPECTED_CIRCULAR_DEPENDENCIES (outside this file).

176-176: Note-only change.

Acknowledges CCoinJoinQueue relocation; no action needed.

src/coinjoin/coinjoin.cpp (2)

6-6: Correct: include queue.h after moving CCoinJoinQueue impl.

Ensure coinjoin/queue.cpp is added to the build (Makefile/CMake) to satisfy linkage.

43-43: Note-only change.

Implementation moved to coinjoin/queue.cpp; no action needed here.

src/Makefile.am (1)

180-187: LGTM!

The Makefile changes correctly reflect the new modular structure by adding coinjoin/queue.h, coinjoin/params.h, coinjoin/walletman.h, msg_processing.h, coinjoin/queue.cpp, and coinjoin/walletman.cpp, while removing the obsolete coinjoin/context.h and coinjoin/context.cpp. The additions are properly placed in both BITCOIN_CORE_H and libbitcoin_node_a_SOURCES.

Also applies to: 296-296, 490-492

src/coinjoin/queue.h (2)

48-54: SERIALIZE_METHODS correctly handles vchSig.

The conditional serialization of vchSig (excluded when SER_GETHASH is set) is correct and consistent with the signature hash calculation in GetSignatureHash(). This ensures the signature is not included in the hash being signed.

1-75: Fix formatting and verify serialization

Run clang-format -i src/coinjoin/queue.h and remove the two trailing blank lines.

Confirm excluding masternodeOutpoint from SERIALIZE_METHODS is intentional, since m_protxHash alone identifies the masternode in serialized data.
src/coinjoin/queue.cpp (1)
46-50: ```bash
#!/bin/bash

Verify that queue.h includes params.h

rg -nP '#include\s*["<]coinjoin/params.h[">]' src/coinjoin/queue.h -n -C2
</blockquote></details>

</blockquote></details>

</details>

coderabbitai · 2025-10-03T16:43:24Z

src/coinjoin/queue.cpp

+// Copyright (c) 2014-2025 The Dash Core developers
+// Distributed under the MIT/X11 software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <coinjoin/queue.h>
+
+#include <bls/bls.h>
+#include <logging.h>
+#include <masternode/node.h>
+#include <span.h>
+#include <tinyformat.h>
+#include <timedata.h>
+
+#include <version.h>
+
+uint256 CCoinJoinQueue::GetSignatureHash() const
+{
+    return SerializeHash(*this, SER_GETHASH, PROTOCOL_VERSION);
+}
+
+uint256 CCoinJoinQueue::GetHash() const
+{
+    return SerializeHash(*this, SER_NETWORK, PROTOCOL_VERSION);
+}
+
+bool CCoinJoinQueue::Sign(const CActiveMasternodeManager& mn_activeman)
+{
+    uint256 hash = GetSignatureHash();
+    CBLSSignature sig = mn_activeman.Sign(hash, /*is_legacy=*/ false);
+    if (!sig.IsValid()) {
+        return false;
+    }
+    vchSig = sig.ToByteVector(false);
+    return true;
+}
+
+bool CCoinJoinQueue::CheckSignature(const CBLSPublicKey& blsPubKey) const
+{
+    if (!CBLSSignature(Span{vchSig}, false).VerifyInsecure(blsPubKey, GetSignatureHash(), false)) {
+        LogPrint(BCLog::COINJOIN, "CCoinJoinQueue::CheckSignature -- VerifyInsecure() failed\n");
+        return false;
+    }
+    return true;
+}
+
+bool CCoinJoinQueue::IsTimeOutOfBounds(int64_t current_time) const
+{
+    return current_time - nTime > COINJOIN_QUEUE_TIMEOUT ||
+           nTime - current_time > COINJOIN_QUEUE_TIMEOUT;
+}
+
+bool CCoinJoinQueue::IsTimeOutOfBounds() const
+{
+    return IsTimeOutOfBounds(GetAdjustedTime());
+}
+
+std::string CCoinJoinQueue::ToString() const
+{
+    return strprintf("nDenom=%d, nTime=%lld, fReady=%s, fTried=%s, masternode=%s",
+        nDenom, nTime, fReady ? "true" : "false", fTried ? "true" : "false", masternodeOutpoint.ToStringShort());
+}
+
+


⚠️ Potential issue | 🟡 Minor

Fix clang-format issue and consider refactoring duplicate code.

The implementation looks correct and follows the same patterns as CCoinJoinBroadcastTx in coinjoin.cpp. However:

Clang-format issue: The pipeline reports formatting differences. Run clang-format on this file to fix.

Potential code duplication: The GetSignatureHash(), Sign(), and CheckSignature() methods are nearly identical to those in CCoinJoinBroadcastTx (src/coinjoin/coinjoin.cpp lines 44-69). Consider extracting common BLS signing/verification logic into a shared utility function to reduce duplication and improve maintainability.

Extra blank line: Line 63 has an extra blank line at the end of the file.

Apply clang-format:

#!/bin/bash # Run clang-format on the file clang-format -i src/coinjoin/queue.cpp

🧰 Tools

🪛 GitHub Actions: Clang Diff Format Check

[error] 1-1: Clang format differences found. Apply clang-format to fix formatting in this file.

🤖 Prompt for AI Agents

src/coinjoin/queue.cpp lines 1-63: Fix formatting and minor cleanup by running clang-format on this file (e.g., clang-format -i src/coinjoin/queue.cpp) and remove the extra blank line at EOF; then reduce duplication by extracting shared BLS signing/verification code used by CCoinJoinQueue and CCoinJoinBroadcastTx into a small utility (e.g., coinjoin/bls_util.{h,cpp}) that provides functions like ComputeSignatureHash(const T&), SignWithActiveMasternode(hash, mn_activeman) and VerifySignatureWithPubKey(vchSig, blsPubKey, hash), and update CCoinJoinQueue::GetSignatureHash/Sign/CheckSignature to call those helpers.

kwvg · 2025-10-05T17:10:06Z

Marking as draft as it'll conflict with dash#6838 and the former is a blocker

coderabbitai

Actionable comments posted: 3

🧹 Nitpick comments (2)

src/test/util/setup_common.cpp (1)

340-340: Clarify the comment for better readability.

The pronoun "it" in the comment is ambiguous. Consider making the comment more explicit.

Apply this diff to improve clarity:
-    // The test suite doesn't use masternode mode, so we may initialize it
+    // The test suite doesn't use masternode mode, so we may initialize the wallet manager

src/init.cpp (1)

2134-2145: Gate CJWalletManager creation by -disablewallet

Avoid constructing wallet manager when wallets are disabled. It also narrows surface in non‑wallet runs.

-    assert(!node.cj_walletman);
-    if (!node.mn_activeman) {
-        node.cj_walletman = CJWalletManager::make(chainman, *node.dmnman, *node.mn_metaman, *node.mempool, *node.mn_sync,
-                                                  *node.llmq_ctx->isman, !ignores_incoming_txs);
-    }
+    assert(!node.cj_walletman);
+#ifdef ENABLE_WALLET
+    if (!args.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET) && !node.mn_activeman) {
+        node.cj_walletman = CJWalletManager::make(chainman, *node.dmnman, *node.mn_metaman, *node.mempool, *node.mn_sync,
+                                                  *node.llmq_ctx->isman, !ignores_incoming_txs);
+    }
+#endif

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b1c28ce and 5be7da7.

📒 Files selected for processing (36)

src/Makefile.am (3 hunks)
src/coinjoin/client.cpp (5 hunks)
src/coinjoin/client.h (5 hunks)
src/coinjoin/context.cpp (0 hunks)
src/coinjoin/context.h (0 hunks)
src/coinjoin/interfaces.cpp (3 hunks)
src/coinjoin/server.h (1 hunks)
src/coinjoin/walletman.cpp (1 hunks)
src/coinjoin/walletman.h (1 hunks)
src/evo/mnauth.h (1 hunks)
src/governance/governance.h (1 hunks)
src/init.cpp (6 hunks)
src/llmq/blockprocessor.h (1 hunks)
src/llmq/dkgsessionhandler.h (1 hunks)
src/llmq/dkgsessionmgr.h (1 hunks)
src/llmq/ehf_signals.h (1 hunks)
src/llmq/quorums.h (1 hunks)
src/llmq/signing.h (1 hunks)
src/llmq/signing_shares.h (1 hunks)
src/masternode/utils.cpp (3 hunks)
src/masternode/utils.h (1 hunks)
src/msg_result.h (1 hunks)
src/net_processing.cpp (17 hunks)
src/net_processing.h (2 hunks)
src/node/context.cpp (1 hunks)
src/node/context.h (2 hunks)
src/protocol.h (0 hunks)
src/rpc/coinjoin.cpp (2 hunks)
src/spork.h (1 hunks)
src/test/coinjoin_dstxmanager_tests.cpp (2 hunks)
src/test/denialofservice_tests.cpp (4 hunks)
src/test/net_peer_connection_tests.cpp (1 hunks)
src/test/util/setup_common.cpp (4 hunks)
src/wallet/test/coinjoin_tests.cpp (2 hunks)
src/wallet/test/wallet_tests.cpp (0 hunks)
test/lint/lint-circular-dependencies.py (1 hunks)

💤 Files with no reviewable changes (4)

src/coinjoin/context.cpp
src/coinjoin/context.h
src/wallet/test/wallet_tests.cpp
src/protocol.h

✅ Files skipped from review due to trivial changes (1)

src/llmq/ehf_signals.h

🚧 Files skipped from review as they are similar to previous changes (10)

src/llmq/dkgsessionmgr.h
src/llmq/signing_shares.h
src/rpc/coinjoin.cpp
src/node/context.cpp
src/coinjoin/server.h
src/llmq/signing.h
src/coinjoin/interfaces.cpp
src/spork.h
src/Makefile.am
src/evo/mnauth.h

🧰 Additional context used

📓 Path-based instructions (3)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/governance/governance.h
src/masternode/utils.h
src/test/denialofservice_tests.cpp
src/llmq/dkgsessionhandler.h
src/masternode/utils.cpp
src/node/context.h
src/coinjoin/client.cpp
src/net_processing.h
src/msg_result.h
src/test/coinjoin_dstxmanager_tests.cpp
src/coinjoin/walletman.h
src/coinjoin/client.h
src/init.cpp
src/test/util/setup_common.cpp
src/test/net_peer_connection_tests.cpp
src/wallet/test/coinjoin_tests.cpp
src/llmq/blockprocessor.h
src/llmq/quorums.h
src/net_processing.cpp
src/coinjoin/walletman.cpp

src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Masternode lists must use immutable data structures (Immer library) for thread safety

Files:

src/masternode/utils.h
src/masternode/utils.cpp

src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Files:

src/test/denialofservice_tests.cpp
src/test/coinjoin_dstxmanager_tests.cpp
src/test/util/setup_common.cpp
src/test/net_peer_connection_tests.cpp
src/wallet/test/coinjoin_tests.cpp

🧠 Learnings (4)

📚 Learning: 2025-07-23T09:30:34.631Z

Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.

Applied to files:

src/llmq/blockprocessor.h

📚 Learning: 2025-10-03T11:20:37.475Z

Learnt from: kwvg
PR: dashpay/dash#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/net_processing.cpp

📚 Learning: 2025-10-02T18:29:54.756Z

Learnt from: kwvg
PR: dashpay/dash#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/net_processing.cpp

📚 Learning: 2025-10-02T18:29:54.756Z

Learnt from: kwvg
PR: dashpay/dash#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/net_processing.cpp

🧬 Code graph analysis (16)

src/masternode/utils.h (4)

src/coinjoin/walletman.h (1)

CJWalletManager (36-64)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

src/coinjoin/client.cpp (6)

DoMaintenance (1818-1823)

DoMaintenance (1818-1818)

DoMaintenance (1825-1841)

DoMaintenance (1825-1825)

DoMaintenance (1884-1890)

DoMaintenance (1884-1884)

src/coinjoin/walletman.cpp (1)

connman (33-33)

src/test/denialofservice_tests.cpp (2)

src/coinjoin/walletman.cpp (3)

make (152-163)

make (152-155)

connman (33-33)

src/net_processing.cpp (2)

make (1958-1970)

make (1958-1967)

src/masternode/utils.cpp (4)

src/coinjoin/client.cpp (6)

DoMaintenance (1818-1823)

DoMaintenance (1818-1818)

DoMaintenance (1825-1841)

DoMaintenance (1825-1825)

DoMaintenance (1884-1890)

DoMaintenance (1884-1884)

src/evo/deterministicmns.cpp (2)

DoMaintenance (1364-1371)

DoMaintenance (1364-1364)

src/masternode/sync.cpp (2)

DoMaintenance (367-372)

DoMaintenance (367-367)

src/coinjoin/walletman.cpp (1)

connman (33-33)

src/node/context.h (2)

src/coinjoin/coinjoin.h (1)

CDSTXManager (378-406)

src/coinjoin/walletman.h (1)

CJWalletManager (36-64)

src/coinjoin/client.cpp (2)

src/net_processing.cpp (4)

ProcessMessage (3609-5392)

ProcessMessage (3609-3614)

from (1035-1035)

ret (660-660)

src/coinjoin/server.cpp (2)

ProcessMessage (26-40)

ProcessMessage (26-26)

src/net_processing.h (3)

src/coinjoin/coinjoin.h (1)

CDSTXManager (378-406)

src/coinjoin/walletman.h (1)

CJWalletManager (36-64)

src/masternode/active/context.cpp (2)

ActiveContext (16-33)

ActiveContext (35-39)

src/msg_result.h (2)

src/protocol.h (1)

CInv (531-568)

src/net_processing.cpp (6)

inv (626-626)

inv (627-627)

inv (646-646)

inv (652-652)

inv (940-940)

inv (2317-2317)

src/test/coinjoin_dstxmanager_tests.cpp (1)

src/coinjoin/interfaces.cpp (2)

Assert (80-83)

Assert (85-88)

src/coinjoin/walletman.h (3)

src/coinjoin/interfaces.cpp (8)

wallet (98-102)

wallet (98-98)

name (103-107)

name (103-103)

name (108-111)

name (108-108)

name (112-116)

name (112-112)

src/coinjoin/walletman.cpp (32)

wallet (43-43)

make (152-163)

make (152-155)

Schedule (68-75)

Schedule (68-68)

connman (33-33)

hasQueue (86-92)

hasQueue (86-86)

hash (36-36)

hash (40-40)

getClient (94-97)

getClient (94-94)

name (37-37)

name (44-44)

name (45-45)

processMessage (99-110)

processMessage (99-101)

getQueueFromHash (112-118)

getQueueFromHash (112-112)

getQueueSize (120-126)

getQueueSize (120-120)

getMixingMasternodes (128-134)

getMixingMasternodes (128-128)

addWallet (136-139)

addWallet (136-136)

removeWallet (146-149)

removeWallet (146-146)

flushWallet (141-144)

flushWallet (141-141)

UpdatedBlockTip (77-84)

UpdatedBlockTip (77-77)

pindexNew (49-49)

src/net_processing.cpp (9)

make (1958-1970)

make (1958-1967)

scheduler (618-618)

scheduler (669-669)

hash (1003-1003)

hash (1009-1009)

UpdatedBlockTip (2121-2156)

UpdatedBlockTip (2121-2121)

pindexNew (603-603)

src/coinjoin/client.h (2)

src/net_processing.cpp (3)

ProcessMessage (3609-5392)

ProcessMessage (3609-3614)

from (1035-1035)

src/coinjoin/client.cpp (6)

ProcessMessage (41-140)

ProcessMessage (41-42)

ProcessMessage (142-163)

ProcessMessage (142-142)

ProcessMessage (178-224)

ProcessMessage (178-178)

src/init.cpp (3)

src/net_processing.cpp (12)

node (610-610)

node (611-611)

node (636-636)

node (732-732)

node (767-767)

node (867-871)

node (882-882)

node (893-893)

node (904-904)

node (912-912)

make (1958-1970)

make (1958-1967)

src/coinjoin/walletman.cpp (2)

make (152-163)

make (152-155)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

src/test/util/setup_common.cpp (2)

src/coinjoin/walletman.cpp (2)

make (152-163)

make (152-155)

src/net_processing.cpp (2)

make (1958-1970)

make (1958-1967)

src/test/net_peer_connection_tests.cpp (2)

src/coinjoin/walletman.cpp (3)

make (152-163)

make (152-155)

connman (33-33)

src/net_processing.cpp (2)

make (1958-1970)

make (1958-1967)

src/wallet/test/coinjoin_tests.cpp (1)

src/coinjoin/interfaces.cpp (2)

Assert (80-83)

Assert (85-88)

src/net_processing.cpp (1)

src/coinjoin/walletman.cpp (5)

make (152-163)

make (152-155)

connman (33-33)

hash (36-36)

hash (40-40)

src/coinjoin/walletman.cpp (4)

src/net_processing.cpp (9)

peer (674-674)

peer (809-809)

peer (914-914)

peer (915-915)

peer (1022-1022)

peer (1030-1030)

ret (660-660)

make (1958-1970)

make (1958-1967)

src/coinjoin/interfaces.cpp (8)

name (103-107)

name (103-103)

name (108-111)

name (108-108)

name (112-116)

name (112-112)

wallet (98-102)

wallet (98-98)

src/coinjoin/walletman.h (1)

wallet (32-34)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

🪛 GitHub Actions: Clang Diff Format Check

src/coinjoin/client.h

[error] 1-1: Clang format differences found in src/coinjoin/client.h. Run the formatting script to fix: git diff ... | ./contrib/devtools/clang-format-diff.py -p1.

src/coinjoin/walletman.cpp

[error] 1-1: Clang format differences found in src/coinjoin/walletman.cpp. Run the formatting script to fix: git diff ... | ./contrib/devtools/clang-format-diff.py -p1.

⏰ 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: arm-linux-build / Build source
GitHub Check: linux64_tsan-build / Build source
GitHub Check: win64-build / Build source
GitHub Check: mac-build / Build source
GitHub Check: linux64_ubsan-build / Build source
GitHub Check: linux64_sqlite-build / Build source
GitHub Check: linux64-build / Build source
GitHub Check: linux64_fuzz-build / Build source
GitHub Check: linux64_nowallet-build / Build source

🔇 Additional comments (8)

src/test/coinjoin_dstxmanager_tests.cpp (1)

41-42: DSTX manager now sourced from NodeContext.

Thanks for switching to m_node.dstxman; this keeps the test in lockstep with the new manager ownership.

src/masternode/utils.cpp (2)

11-16: LGTM! Clean interface abstraction.

The refactor correctly transitions from CJContext to CJWalletManager*, properly handling the case when wallet support is not compiled by accepting a nullable pointer parameter.

39-39: Good defensive programming with the conditional initialization.

The ternary operator correctly handles the case when cj_walletman is nullptr (when wallet support is disabled), returning an empty vector that causes no mixing masternodes to be preserved during disconnection checks.

src/test/util/setup_common.cpp (3)

338-343: LGTM! Correct initialization order and proper assertion.

The initialization creates dstxman before cj_walletman, and both before peerman, which is correct since peerman depends on these components. The assertion ensures wallet manager creation succeeds when ENABLE_WALLET is defined.

355-366: LGTM! Correct PeerManager wiring.

The PeerManager::make call correctly passes *m_node.dstxman (by reference) and m_node.cj_walletman.get() (raw pointer, which can be nullptr when ENABLE_WALLET is not defined), matching the updated signature from the relevant code snippets.

376-386: LGTM! Correct destruction order.

The destructor properly tears down components in reverse dependency order:

peerman (depends on dstxman, cj_walletman, etc.)

banman

Wallet clients

cj_walletman

dstxman

This ensures dependent components are destroyed before their dependencies.

src/coinjoin/client.cpp (1)

108-115: Confirm DSQUEUE relay behavior when fReady is true

In the fReady branch, you return early without adding dsq to ret.m_dsq, while in the non‑ready path you add it (Line 138). Is skipping relay in the ready case intentional? If not, push dsq in both cases for consistent relaying.

Also applies to: 138-140
src/coinjoin/walletman.cpp (1)
1-1: Confirm src/coinjoin/walletman.cpp formatting clang-format-diff.py produced no output; manually rerun
git diff -U0 | ./contrib/devtools/clang-format-diff.py -p1
and apply any resulting fixes to clear CI errors.

coderabbitai · 2025-10-05T17:19:22Z

src/msg_result.h

+#ifndef BITCOIN_MSG_RESULT_H
+#define BITCOIN_MSG_RESULT_H
+
+#include <coinjoin/coinjoin.h>


⚠️ Potential issue | 🟠 Major

Break the new circular dependency introduced by msg_result.h.

Pulling in <coinjoin/coinjoin.h> from this new common header creates a fresh cycle (coinjoin/coinjoin -> instantsend/instantsend -> llmq/quorums -> msg_result -> coinjoin/coinjoin) plus the longer chainlock loop now added to EXPECTED_CIRCULAR_DEPENDENCIES. The whole point of extracting MessageProcessingResult was to untangle includes, but adding this dependency just shifts the problem elsewhere and forces us to whitelist more cycles.

Please restructure so msg_result.h does not depend on the full CoinJoin surface—e.g. by lightweighting the dependency (dedicated header for CCoinJoinQueue, indirect handle, or alternative payload) and keeping the bulky include in the translation units instead of the shared result type. That breaks the circle without bloating the lint expectations again.

🤖 Prompt for AI Agents

In src/msg_result.h around line 8, the new inclusion of <coinjoin/coinjoin.h> creates a circular dependency; remove that heavy include and replace the direct CoinJoin type dependence with a lightweight alternative such as a forward declaration or an opaque handle: introduce a tiny dedicated header (e.g., coinjoin_fwd.h) that forward-declares CCoinJoinQueue or defines an enum/struct identifier used by MessageProcessingResult, update msg_result.h to use that lightweight type (or an index/ID/unique_ptr-to-void) instead of the full CoinJoin class, and move the full <coinjoin/coinjoin.h> include back into translation units (.cpp) that need to access CoinJoin internals so the shared header no longer pulls in the full CoinJoin surface and breaks the cycle.

src/test/net_peer_connection_tests.cpp

coderabbitai · 2025-10-05T17:19:23Z

test/lint/lint-circular-dependencies.py

+    "chainlock/chainlock -> llmq/quorums -> msg_result -> coinjoin/coinjoin -> chainlock/chainlock",
    "chainlock/chainlock -> validation -> chainlock/chainlock",
    "chainlock/chainlock -> validation -> evo/chainhelper -> chainlock/chainlock",
    "chainlock/signing -> instantsend/instantsend -> net_processing -> masternode/active/context -> chainlock/signing",
-    "coinjoin/client -> net_processing -> coinjoin/client",
-    "coinjoin/client -> net_processing -> coinjoin/context -> coinjoin/client",
-    "coinjoin/coinjoin -> instantsend/instantsend -> net_processing -> coinjoin/context -> coinjoin/coinjoin",
+    "coinjoin/client -> coinjoin/coinjoin -> instantsend/instantsend -> net_processing -> coinjoin/walletman -> coinjoin/client",
+    "coinjoin/coinjoin -> instantsend/instantsend -> llmq/quorums -> msg_result -> coinjoin/coinjoin",
    "coinjoin/server -> net_processing -> coinjoin/server",


⚠️ Potential issue | 🟠 Major

Don’t whitelist the newly created circular dependencies.

These new EXPECTED entries correspond exactly to the cycles introduced by msg_result.h pulling in coinjoin/coinjoin.h. Rather than institutionalizing them in the lint allowlist, we should eliminate the root include problem so the cycles vanish and the previous expectation set stays smaller. Once msg_result.h stops depending on the CoinJoin monolith, these additions can be dropped.

🤖 Prompt for AI Agents

In test/lint/lint-circular-dependencies.py around lines 30 to 36, the reviewer warns against adding the new EXPECTED cycle whitelist entries; instead remove the underlying include that caused the cycle. Locate the header (msg_result.h) that currently includes coinjoin/coinjoin.h and refactor it to remove the direct include: replace it with forward declarations and move any coinjoin-specific usage into implementation files (or introduce a minimal header with the small API needed) so msg_result.h no longer depends on the CoinJoin monolith; after that, delete the newly added EXPECTED entries from this file so the lint allowlist remains unchanged.

github-actions · 2025-10-14T13:54:14Z

This pull request has conflicts, please rebase.

We moved CCoinJoinServer to ActiveContext and 2/3 members of CJContext are conditionally initialized, let's move the unconditionally init'ed member out so we can conditionally initialize the whole object.

Currently a lot of non-wallet code is peppered with `ENABLE_WALLET` due to wallet-only CoinJoin managers, we need to contain them.

Placing them in `protocol.h` limits what headers we can include as circular dependencies can balloon unexpectedly, a separate header will reduce how often this happens.

This should prevent ENABLE_WALLET from spilling out to files that include the header.

Now that we no longer assume that cj_ctx is always initialized, we can skip the whole process if we are in masternode mode.

As it's now a fixed value, we can drop it, yay!

CJContext is no longer just a context class, it's a lot more involved now and serves triple duty as also a notification interface and a layer of abstraction. Name reflects that it's the wallet-specific components of CoinJoin.

coderabbitai

Actionable comments posted: 2

♻️ Duplicate comments (4)

src/msg_result.h (1)
8-9: Break circular deps: remove coinjoin include; use fwd‑decl + pointer types.

Including <coinjoin/coinjoin.h> here reintroduces cycles. Store DSQs as shared_ptrs and forward‑declare types so this header is lightweight. This addresses the earlier circular‑deps concern.

Apply:
-#include <coinjoin/coinjoin.h>
-#include <primitives/transaction.h>
+#include <protocol.h>
+#include <uint256.h>
+#include <memory>
@@
-    //! @m_dsq will relay DSQs to connected peers
-    std::vector<CCoinJoinQueue> m_dsq;
+    //! @m_dsq will relay DSQs to connected peers
+    // Use shared_ptr to allow forward declaration and avoid heavy includes here
+    std::vector<std::shared_ptr<const CCoinJoinQueue>> m_dsq;
@@
-    //! @m_inv_filter will relay this inventory if filter matches to connected peers if not nullopt
-    std::optional<std::pair<CInv, std::variant<CTransactionRef, uint256>>> m_inv_filter;
+    //! @m_inv_filter will relay this inventory if filter matches to connected peers if not nullopt
+    // Avoid pulling primitives/transaction.h by using a forward-declared CTransaction
+    std::optional<std::pair<CInv, std::variant<std::shared_ptr<const CTransaction>, uint256>>> m_inv_filter;
And add these forward declarations near the top:
+#include <optional>
+#include <string>
+#include <utility>
+#include <variant>
+#include <vector>
+
+class CCoinJoinQueue;
+class CTransaction;
Note: Include coinjoin/coinjoin.h and primitives/transaction.h in .cpps that construct/use concrete objects.
Also consider removing <protocol.h> from headers that don’t need it and forward‑declaring CInv where possible, but that’s optional.

</blockquote></details>
<details>
<summary>src/net_processing.cpp (1)</summary><blockquote>

`5363-5364`: **Avoid accidental double consumption of vRecv across wallet/server.**

Follow-up to previous DSQUEUE fix: please verify the wallet and server handlers’ handled message sets are disjoint to prevent advancing the shared CDataStream twice.


```shell
#!/bin/bash
# Show msg types handled by CJWalletManager and CCoinJoinServer to spot overlaps
rg -nP --type=cpp 'NetMsgType::\w+' src/coinjoin/walletman.cpp -C2
rg -nP --type=cpp 'NetMsgType::\w+' src/coinjoin/server.cpp -C2

# Quick overlap diff (names only)
echo "Walletman:" && rg -nP 'NetMsgType::(\w+)' -o --no-filename src/coinjoin/walletman.cpp | awk -F'::' '{print $2}' | sort -u > /tmp/w && cat /tmp/w
echo "Server:" && rg -nP 'NetMsgType::(\w+)' -o --no-filename src/coinjoin/server.cpp | awk -F'::' '{print $2}' | sort -u > /tmp/s && cat /tmp/s
echo "Overlap:" && comm -12 /tmp/w /tmp/s
src/coinjoin/walletman.cpp (2)

52-65: Ctor init order: keep as-is but ensure no early deref.

walletman takes a reference to queueman while queueman is initialized afterward. This pattern is acceptable here as long as walletman does not dereference queueman during construction; it should only store the reference and check for null at use sites. Please confirm this invariant holds. Based on learnings.

99-110: Reusing vRecv across clients is intentional — confirm invariant.

Forwarding the same CDataStream to all clients is fine if only the targeted session reads it (others must bail before consuming). Please confirm no non-target path advances vRecv. Based on prior discussion.

🧹 Nitpick comments (3)

src/masternode/utils.cpp (1)

70-74: Defensive: guard potential null netInfo to avoid crash.

dmn->pdmnState->netInfo may be null; guard before deref to be safe.

-        bool fFound = ranges::any_of(mixing_masternodes, [&pnode](const auto& dmn) {
-            return pnode->addr == dmn->pdmnState->netInfo->GetPrimary();
-        });
+        bool fFound = ranges::any_of(mixing_masternodes, [&pnode](const auto& dmn) {
+            const auto net_info = dmn->pdmnState->netInfo;
+            return net_info && (pnode->addr == net_info->GetPrimary());
+        });

src/net_processing.cpp (2)

2453-2454: DRY: factor inv-type choice (TX vs DSTX) into a helper.

Same logic appears in three places; centralize to avoid drift.

@@ class PeerManagerImpl final : public PeerManager
 private:
+    // Helper: choose inv type for a txid (regular vs DSTX)
+    int InvTypeForTx(const uint256& txid) const {
+        return m_dstxman.GetDSTX(txid) ? MSG_DSTX : MSG_TX;
+    }
@@ void PeerManagerImpl::_RelayTransaction(const uint256& txid)
-    const CInv inv{m_dstxman.GetDSTX(txid) ? MSG_DSTX : MSG_TX, txid};
+    const CInv inv{InvTypeForTx(txid), txid};
@@ if (fSendTrickle && tx_relay->m_send_mempool) {
-                    int nInvType = m_dstxman.GetDSTX(hash) ? MSG_DSTX : MSG_TX;
+                    int nInvType = InvTypeForTx(hash);
@@ while (!vInvTx.empty() && nRelayedTransactions < broadcast_max) {
-                    int nInvType = m_dstxman.GetDSTX(hash) ? MSG_DSTX : MSG_TX;
+                    int nInvType = InvTypeForTx(hash);

Also applies to: 6195-6198, 6271-6273

2883-2886: Unify DSQ source order with AlreadyHave (wallet → server).

AlreadyHave(MSG_DSQ) checks wallet first; GETDATA checks server first. Flip here for consistency and easier reasoning.

-            auto opt_dsq = m_active_ctx ? m_active_ctx->cj_server->GetQueueFromHash(inv.hash) : std::nullopt;
-            if (m_cj_walletman && !opt_dsq.has_value()) {
-                opt_dsq = m_cj_walletman->getQueueFromHash(inv.hash);
-            }
+            auto opt_dsq = m_cj_walletman ? m_cj_walletman->getQueueFromHash(inv.hash) : std::nullopt;
+            if (!opt_dsq.has_value() && m_active_ctx) {
+                opt_dsq = m_active_ctx->cj_server->GetQueueFromHash(inv.hash);
+            }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5be7da7 and 6a20464.

📒 Files selected for processing (39)

src/Makefile.am (3 hunks)
src/coinjoin/client.cpp (5 hunks)
src/coinjoin/client.h (5 hunks)
src/coinjoin/context.cpp (0 hunks)
src/coinjoin/context.h (0 hunks)
src/coinjoin/interfaces.cpp (3 hunks)
src/coinjoin/server.cpp (1 hunks)
src/coinjoin/server.h (1 hunks)
src/coinjoin/walletman.cpp (1 hunks)
src/coinjoin/walletman.h (1 hunks)
src/dsnotificationinterface.cpp (6 hunks)
src/dsnotificationinterface.h (2 hunks)
src/evo/mnauth.h (1 hunks)
src/governance/governance.h (1 hunks)
src/init.cpp (6 hunks)
src/llmq/blockprocessor.h (1 hunks)
src/llmq/dkgsessionhandler.h (1 hunks)
src/llmq/dkgsessionmgr.h (1 hunks)
src/llmq/ehf_signals.h (1 hunks)
src/llmq/quorums.h (1 hunks)
src/llmq/signing.h (1 hunks)
src/llmq/signing_shares.h (1 hunks)
src/masternode/utils.cpp (3 hunks)
src/masternode/utils.h (1 hunks)
src/msg_result.h (1 hunks)
src/net_processing.cpp (17 hunks)
src/net_processing.h (2 hunks)
src/node/context.cpp (1 hunks)
src/node/context.h (2 hunks)
src/protocol.h (0 hunks)
src/rpc/coinjoin.cpp (2 hunks)
src/spork.h (1 hunks)
src/test/coinjoin_dstxmanager_tests.cpp (2 hunks)
src/test/denialofservice_tests.cpp (4 hunks)
src/test/net_peer_connection_tests.cpp (1 hunks)
src/test/util/setup_common.cpp (4 hunks)
src/wallet/test/coinjoin_tests.cpp (2 hunks)
src/wallet/test/wallet_tests.cpp (0 hunks)
test/lint/lint-circular-dependencies.py (1 hunks)

💤 Files with no reviewable changes (4)

src/protocol.h
src/wallet/test/wallet_tests.cpp
src/coinjoin/context.cpp
src/coinjoin/context.h

✅ Files skipped from review due to trivial changes (1)

src/llmq/ehf_signals.h

🚧 Files skipped from review as they are similar to previous changes (15)

src/llmq/signing_shares.h
src/masternode/utils.h
src/rpc/coinjoin.cpp
src/coinjoin/server.h
src/llmq/dkgsessionhandler.h
src/node/context.h
src/llmq/dkgsessionmgr.h
test/lint/lint-circular-dependencies.py
src/llmq/blockprocessor.h
src/coinjoin/walletman.h
src/spork.h
src/test/net_peer_connection_tests.cpp
src/Makefile.am
src/node/context.cpp
src/coinjoin/client.h

🧰 Additional context used

📓 Path-based instructions (3)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/coinjoin/server.cpp
src/llmq/signing.h
src/coinjoin/interfaces.cpp
src/msg_result.h
src/llmq/quorums.h
src/masternode/utils.cpp
src/evo/mnauth.h
src/test/coinjoin_dstxmanager_tests.cpp
src/net_processing.h
src/wallet/test/coinjoin_tests.cpp
src/dsnotificationinterface.cpp
src/test/denialofservice_tests.cpp
src/governance/governance.h
src/coinjoin/walletman.cpp
src/test/util/setup_common.cpp
src/init.cpp
src/dsnotificationinterface.h
src/coinjoin/client.cpp
src/net_processing.cpp

src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Masternode lists must use immutable data structures (Immer library) for thread safety

Files:

src/masternode/utils.cpp
src/evo/mnauth.h

src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Files:

src/test/coinjoin_dstxmanager_tests.cpp
src/wallet/test/coinjoin_tests.cpp
src/test/denialofservice_tests.cpp
src/test/util/setup_common.cpp

🧠 Learnings (4)

📚 Learning: 2025-07-29T14:32:48.369Z

Learnt from: kwvg
PR: dashpay/dash#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/walletman.cpp

📚 Learning: 2025-10-03T11:20:37.475Z

Learnt from: kwvg
PR: dashpay/dash#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/net_processing.cpp

📚 Learning: 2025-10-02T18:29:54.756Z

Learnt from: kwvg
PR: dashpay/dash#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/net_processing.cpp

📚 Learning: 2025-10-02T18:29:54.756Z

Learnt from: kwvg
PR: dashpay/dash#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/net_processing.cpp

🧬 Code graph analysis (15)

src/coinjoin/server.cpp (1)

src/net_processing.cpp (1)

ret (660-660)

src/coinjoin/interfaces.cpp (2)

src/coinjoin/walletman.cpp (4)

wallet (43-43)

name (37-37)

name (44-44)

name (45-45)

src/coinjoin/walletman.h (1)

wallet (32-34)

src/msg_result.h (2)

src/protocol.h (1)

CInv (531-568)

src/net_processing.cpp (6)

inv (626-626)

inv (627-627)

inv (646-646)

inv (652-652)

inv (940-940)

inv (2317-2317)

src/masternode/utils.cpp (2)

src/coinjoin/client.cpp (6)

DoMaintenance (1818-1823)

DoMaintenance (1818-1818)

DoMaintenance (1825-1841)

DoMaintenance (1825-1825)

DoMaintenance (1884-1890)

DoMaintenance (1884-1884)

src/masternode/sync.cpp (2)

DoMaintenance (367-372)

DoMaintenance (367-367)

src/test/coinjoin_dstxmanager_tests.cpp (1)

src/coinjoin/interfaces.cpp (2)

Assert (80-83)

Assert (85-88)

src/net_processing.h (3)

src/coinjoin/coinjoin.h (1)

CDSTXManager (368-396)

src/coinjoin/walletman.h (1)

CJWalletManager (36-64)

src/masternode/active/context.cpp (2)

ActiveContext (18-37)

ActiveContext (39-43)

src/wallet/test/coinjoin_tests.cpp (1)

src/coinjoin/interfaces.cpp (2)

Assert (80-83)

Assert (85-88)

src/dsnotificationinterface.cpp (2)

src/dsnotificationinterface.h (1)

CDSNotificationInterface (18-55)

src/coinjoin/coinjoin.cpp (4)

BlockConnected (461-469)

BlockConnected (461-461)

BlockDisconnected (471-478)

BlockDisconnected (471-471)

src/test/denialofservice_tests.cpp (2)

src/coinjoin/walletman.cpp (3)

make (152-163)

make (152-155)

connman (33-33)

src/net_processing.cpp (2)

make (1958-1970)

make (1958-1967)

src/coinjoin/walletman.cpp (4)

src/net_processing.cpp (9)

peer (674-674)

peer (809-809)

peer (914-914)

peer (915-915)

peer (1022-1022)

peer (1030-1030)

ret (660-660)

make (1958-1970)

make (1958-1967)

src/coinjoin/interfaces.cpp (8)

name (103-107)

name (103-103)

name (108-111)

name (108-108)

name (112-116)

name (112-112)

wallet (98-102)

wallet (98-98)

src/coinjoin/walletman.h (1)

wallet (32-34)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

src/test/util/setup_common.cpp (2)

src/coinjoin/walletman.cpp (2)

make (152-163)

make (152-155)

src/net_processing.cpp (2)

make (1958-1970)

make (1958-1967)

src/init.cpp (5)

src/net_processing.cpp (12)

node (610-610)

node (611-611)

node (636-636)

node (732-732)

node (767-767)

node (867-871)

node (882-882)

node (893-893)

node (904-904)

node (912-912)

make (1958-1970)

make (1958-1967)

src/validationinterface.cpp (4)

UnregisterValidationInterface (143-148)

UnregisterValidationInterface (143-143)

RegisterValidationInterface (131-136)

RegisterValidationInterface (131-131)

src/coinjoin/walletman.cpp (2)

make (152-163)

make (152-155)

src/masternode/utils.cpp (2)

DoMaintenance (15-82)

DoMaintenance (15-16)

src/coinjoin/client.cpp (6)

DoMaintenance (1818-1823)

DoMaintenance (1818-1818)

DoMaintenance (1825-1841)

DoMaintenance (1825-1825)

DoMaintenance (1884-1890)

DoMaintenance (1884-1884)

src/dsnotificationinterface.h (2)

src/coinjoin/coinjoin.h (1)

CDSTXManager (368-396)

src/dsnotificationinterface.cpp (1)

CDSNotificationInterface (23-38)

src/coinjoin/client.cpp (1)

src/net_processing.cpp (3)

ProcessMessage (3609-5398)

ProcessMessage (3609-3614)

from (1035-1035)

src/net_processing.cpp (1)

src/coinjoin/walletman.cpp (5)

make (152-163)

make (152-155)

connman (33-33)

hash (36-36)

hash (40-40)

🪛 GitHub Actions: Clang Diff Format Check

src/coinjoin/interfaces.cpp

[error] 1-1: Clang format differences found in src/coinjoin/interfaces.cpp. Run the clang-format-diff check and format the file to match style guidelines. Command: git diff -U0 origin/develop -- $(git ls-files -- $(cat test/util/data/non-backported.txt)) | ./contrib/devtools/clang-format-diff.py -p1 > diff_output.txt

src/dsnotificationinterface.cpp

[error] 1-1: Clang format differences found in src/dsnotificationinterface.cpp. Run the clang-format-diff check and format the file to match style guidelines. Command: git diff -U0 origin/develop -- $(git ls-files -- $(cat test/util/data/non-backported.txt)) | ./contrib/devtools/clang-format-diff.py -p1 > diff_output.txt

src/coinjoin/walletman.cpp

[error] 1-1: Clang format differences found in src/coinjoin/walletman.cpp. Run the clang-format-diff check and format the file to match style guidelines. Command: git diff -U0 origin/develop -- $(git ls-files -- $(cat test/util/data/non-backported.txt)) | ./contrib/devtools/clang-format-diff.py -p1 > diff_output.txt

src/dsnotificationinterface.h

[error] 1-1: Clang format differences found in src/dsnotificationinterface.h. Run the clang-format-diff check and format the file to match style guidelines. Command: git diff -U0 origin/develop -- $(git ls-files -- $(cat test/util/data/non-backported.txt)) | ./contrib/devtools/clang-format-diff.py -p1 > diff_output.txt

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: linux64_sqlite-test / Test source

🔇 Additional comments (40)

src/evo/mnauth.h (1)

9-10: LGTM!

The addition of #include <msg_result.h> is necessary to support the MessageProcessingResult return type used in the ProcessMessage method declaration at line 63.

src/llmq/quorums.h (1)

14-14: LGTM!

Replacing #include <protocol.h> with #include <msg_result.h> is appropriate as this header now depends on MessageProcessingResult (used at line 277) which is defined in msg_result.h.

src/governance/governance.h (1)

11-11: LGTM!

The addition of #include <msg_result.h> is necessary to support the MessageProcessingResult return type used in multiple method declarations (lines 297, 298, 300-301).

src/coinjoin/server.cpp (1)

191-191: LGTM!

The change correctly defers DSQ relay to the caller by appending the validated dsq to ret.m_dsq. This aligns with the broader refactor to use MessageProcessingResult for conveying relay data rather than performing immediate relays within the processing flow.

src/test/coinjoin_dstxmanager_tests.cpp (1)

41-41: LGTM!

The updated access path from m_node.cj_ctx->dstxman to m_node.dstxman correctly reflects that CDSTXManager is now a direct member of NodeContext rather than being accessed through CJContext. The test logic remains unchanged.

Also applies to: 55-55

src/llmq/signing.h (1)

12-12: LGTM!

Replacing #include <protocol.h> with #include <msg_result.h> is appropriate as this header depends on MessageProcessingResult (used at lines 158 and 189-190) which is now defined in msg_result.h.

src/coinjoin/interfaces.cpp (1)

9-9: LGTM!

The include directive change to #include <coinjoin/walletman.h> is necessary to support the CJWalletManager type used in the implementation.

src/test/denialofservice_tests.cpp (1)

151-155: LGTM!

The PeerManager::make call sites are correctly updated to pass the new dstxman parameter (after banman) and cj_walletman parameter (before llmq_ctx), while removing the old cj_ctx argument. The changes are consistent across all four call sites.

Note: A past review comment suggested creating a test-specific helper function to wrap PeerManager::make to reduce churn in future refactorings. While optional, this could improve maintainability for regression tests.

Also applies to: 256-260, 324-328, 430-434

src/wallet/test/coinjoin_tests.cpp (1)

224-231: LGTM — test updated to new CJWalletManager API.

Using Assert(m_node.cj_walletman->getClient("")) is correct and keeps semantics.
src/dsnotificationinterface.h (2)
21-28: Constructor wiring looks good.

CDSTXManager dependency threaded correctly; CJContext removed. No functional concerns.

1-59: The review comment references a style check failure detected by the CI workflow. The CI runs clang-format-diff.py on file changes to detect formatting violations. However, the sandbox environment lacks the clang-format binary required to verify this claim directly.

Please verify locally:
Ensure clang-format is installed (required version: compatible with C++20)
Run the CI check command locally:
git diff -U0 origin/develop -- $(git ls-files -- $(cat test/util/data/non-backported.txt)) | ./contrib/devtools/clang-format-diff.py -p1
If formatting violations are found, apply the formatter as suggested in the review comment
The current file code appears visually consistent with the .clang-format configuration (4-space indentation, left pointer alignment, reasonable line lengths), but definitive verification requires the actual clang-format tool.
src/dsnotificationinterface.cpp (3)

23-38: LGTM — CDSTXManager integration.

Constructor and member wiring to m_dstxman are correct; CJContext removal is clean.

70-81: Verify LLMQ update order change is intentional.

The calls are now:

qdkgsman->UpdatedBlockTip(...)

qman->UpdatedBlockTip(...)

If the previous order was different, confirm no subtle dependency exists between managers.

1-130: Unable to verify formatting compliance without clang-format tool.

The file src/dsnotificationinterface.cpp is included in the non-backported list and would be subject to CI style checks. However, manual inspection of the code reveals consistent formatting with no obvious violations (proper pointer alignment, indentation, brace placement, and line lengths).

Since clang-format is unavailable in this environment, definitively confirming or denying formatting issues is not possible. You should verify formatting compliance locally by running clang-format on this file or checking the CI results in your actual development environment.

src/net_processing.h (1)

61-70: Signature change verified—all call sites already updated correctly.

All 7 call sites to PeerManager::make() across the codebase have been checked and confirmed to match the new signature. The new parameters (CDSTXManager& dstxman and CJWalletManager* cj_walletman) are correctly positioned and passed in all locations:

src/init.cpp

src/test/util/setup_common.cpp

src/test/net_peer_connection_tests.cpp

src/test/denialofservice_tests.cpp (4 call sites)

src/masternode/utils.cpp (1)

15-17: All callers already updated—no issues found.

Verification confirms that CMasternodeUtils::DoMaintenance has exactly one call site at src/init.cpp:2271, which already passes all four parameters matching the new signature, including the newly added CJWalletManager* const cj_walletman. No updates are required.

src/test/util/setup_common.cpp (4)

51-52: LGTM—necessary includes for the refactored CoinJoin managers.

These headers provide CDSTXManager and CJWalletManager types used in the test setup.

344-349: LGTM—correct initialization of DSTX and wallet managers.

The initialization correctly reflects the new architecture:

CDSTXManager is unconditional (node-level DSTX tracking).

CJWalletManager is conditional on ENABLE_WALLET (wallet-specific CoinJoin logic).

The assertion ensures the wallet manager is valid when wallet support is compiled.

Both managers are created before PeerManager (line 362), satisfying the dependency order.

361-372: LGTM—correct initialization order for BanMan and PeerManager.

The initialization order properly satisfies dependencies:

BanMan is created first (line 361).

PeerManager is created with references to dstxman (guaranteed non-null from line 344) and cj_walletman (nullable pointer, correct when ENABLE_WALLET is undefined).

ConnMan is initialized after PeerManager creation.

The updated PeerManager::make signature correctly receives the new manager parameters.

382-392: LGTM—correct destruction order.

The destruction order properly unwinds dependencies:

PeerManager is destroyed first (depends on other managers).

BanMan is destroyed next.

Wallet components are destroyed (when ENABLE_WALLET is defined).

CJWalletManager is destroyed.

CDSTXManager is destroyed last.

This ensures dependent objects are destroyed before the objects they depend on.

src/net_processing.cpp (7)

791-793: Doc comment fix (CJContext → CJWalletManager) is correct.

1958-1970: Factory/ctor signature update consistent with member initialization.

Parameter threading and member init order look consistent and safe.

Also applies to: 1973-1980, 1984-1989

2240-2243: Efficient DSTX presence check via CDSTXManager — LGTM.

2278-2279: DSQ presence check — LGTM.

Wallet-first check here; see follow-up suggestion to mirror this order in GETDATA for consistency.

2745-2746: DSTX GETDATA response via CDSTXManager — LGTM.

3514-3516: Good integration: PostProcessMessage now relays DSQ via RelayDSQ.

590-597: All PeerManager::make call sites confirmed updated; LLMQContext is nullable, not guaranteed non-null.

All 7 call sites across test files, setup_common.cpp, and init.cpp have been verified with the updated signature including active_ctx, cj_walletman, and llmq_ctx parameters. The factory pattern is properly enforced (no direct PeerManagerImpl constructor calls exist outside net_processing.cpp).

However, the original assertion that "LLMQContext is always non-null per invariant" is incorrect—test harnesses explicitly pass nullptr for llmq_ctx, and production code (init.cpp) passes node.llmq_ctx which may also be null. This is not necessarily a problem if nullable context is intentional, but it's not a guaranteed non-null invariant.

src/init.cpp (6)

328-336: Good teardown order for validation interfaces.

Unregistering cj_walletman and g_ds_notification_interface before UnregisterAllValidationInterfaces prevents callbacks during destruction. LGTM.

2139-2150: Manager instantiation sequence looks correct; verify nullptr cases.

Creating CDSTXManager unconditionally and CJWalletManager only when not active MN is sensible. Please confirm CJWalletManager::make returns nullptr when wallet is disabled so RegisterValidationInterface(node.cj_walletman.get()) is a no-op.

2152-2156: PeerManager wiring updated correctly.

Passing dstxman and optional cj_walletman to PeerManager::make matches the new API and keeps wallet logic out of non-wallet code. LGTM.

2159-2162: DS notification interface now depends on CDSTXManager — OK.

Constructing CDSNotificationInterface with dstxman looks consistent with the refactor. Ensure it’s registered before any DS-related callbacks fire (it is).

2271-2271: Masternode maintenance excludes mixing MNs via cj_walletman — nice.

Passing cj_walletman into CMasternodeUtils::DoMaintenance enables skipping mixing masternodes when present, without ENABLE_WALLET sprawl. LGTM.

2283-2285: Scheduling wallet-only CoinJoin tasks is properly conditional.

Guarding Schedule() on node.cj_walletman avoids wallet tasks on MNs or walletless builds. LGTM.

src/coinjoin/walletman.cpp (3)

1-1: Run clang-format on this file.

CI reports formatting diffs; please apply clang-format to align with project style.

68-75: Conditional scheduling is correct.

Early-return on m_relay_txes and scheduling queue and wallet maintenance at 1s cadence looks good.

152-163: Factory method behavior is appropriate.

Returning nullptr when !ENABLE_WALLET avoids wallet objects in walletless builds. LGTM.

src/coinjoin/client.cpp (4)

41-140: DSQUEUE handling refactor looks solid; verify ban scores.

Returning m_to_erase and m_dsq for PeerManager post-processing matches the new flow. Please confirm MisbehavingError scores (100/10) align with current ban policy.

165-176: Session ctor update (drop is_masternode) is coherent.

Constructor now wires wallet, managers, and optional queueman. LGTM.

178-224: Per-session message gating prevents vRecv over-consumption.

Checks on mixingMasternode and peer before deserialization are in place; this ensures only the matching session reads vRecv. LGTM.

981-982: Multi-session creation passes queueman as intended.

Emplacing sessions with m_queueman enables queue joining where available. LGTM.

coderabbitai · 2025-10-21T18:37:14Z

src/coinjoin/interfaces.cpp

+    CJWalletManager& manager()
    {
-        return *Assert(Assert(m_node.cj_ctx)->walletman);
+        return *Assert(m_node.cj_walletman);
    }


⚠️ Potential issue | 🔴 Critical

Fix clang-format violations before merge.

The pipeline reports clang-format style violations in this file. Please run the project's formatting command to resolve these issues.

Run the following script to apply formatting:

#!/bin/bash # Apply clang-format to fix formatting issues git diff -U0 HEAD -- src/coinjoin/interfaces.cpp | ./contrib/devtools/clang-format-diff.py -p1 -i

Also applies to: 100-100, 105-105, 110-110, 114-114

🤖 Prompt for AI Agents

In src/coinjoin/interfaces.cpp around lines 80-83 (also apply to lines 100, 105, 110, 114), there are clang-format style violations; run the project's formatting command to fix them by executing the provided script: generate the diff for src/coinjoin/interfaces.cpp and pipe it through contrib/devtools/clang-format-diff.py (e.g. run the shell snippet from the review), review the resulting formatting changes, stage and commit the updated file so the pipeline no longer reports clang-format violations.

coderabbitai · 2025-10-21T18:37:14Z

src/net_processing.h

+#include <msg_result.h>
+


🛠️ Refactor suggestion | 🟠 Major

Avoid heavy include to reduce header coupling and cycles.

You only need a forward declaration for MessageProcessingResult here. Drop the include and fwd‑declare the type to shrink dependencies and help the circular‑deps linter.

Apply:

-#include <msg_result.h> +// Forward declare to avoid pulling heavy deps; include msg_result.h in the .cpp +struct MessageProcessingResult;

Ensure net_processing.cpp includes msg_result.h.

🤖 Prompt for AI Agents

In src/net_processing.h around lines 13-14, remove the heavy #include <msg_result.h> and replace it with a forward declaration like "class MessageProcessingResult;" (or the correct namespace-qualified forward declaration) to avoid header coupling and potential circular dependencies; then add #include "msg_result.h" to src/net_processing.cpp so the full definition is available where needed.

PastaPastaPasta

utACK 6a20464

UdjinM6

Seems to be mixing with no issues in local wallet.

light ACK 6a20464

kwvg added this to the 23 milestone Sep 8, 2025

github-actions bot added the needs rebase label Sep 9, 2025

kwvg force-pushed the cj_walletman branch from b72b407 to f6d8204 Compare September 29, 2025 15:40

github-actions bot removed the needs rebase label Sep 29, 2025

kwvg force-pushed the cj_walletman branch 2 times, most recently from eb4e6bf to f786cb2 Compare September 29, 2025 18:22

kwvg marked this pull request as ready for review September 30, 2025 06:31

kwvg requested review from PastaPastaPasta, UdjinM6 and knst September 30, 2025 06:31

coderabbitai bot reviewed Sep 30, 2025

View reviewed changes

src/coinjoin/walletman.cpp Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

knst reviewed Sep 30, 2025

View reviewed changes

kwvg force-pushed the cj_walletman branch from f786cb2 to 3dc919c Compare September 30, 2025 12:06

kwvg requested a review from knst September 30, 2025 12:14

coderabbitai bot reviewed Sep 30, 2025

View reviewed changes

src/coinjoin/walletman.cpp Show resolved Hide resolved

src/coinjoin/walletman.cpp Show resolved Hide resolved

src/msg_result.h Show resolved Hide resolved

github-actions bot mentioned this pull request Sep 30, 2025

refactor: update lock annotations for CGovernanceObject and GovernanceStore, move away from RecursiveMutexes #6849

Merged

5 tasks

github-actions bot added the needs rebase label Sep 30, 2025

kwvg force-pushed the cj_walletman branch from 3dc919c to 00a8d0c Compare September 30, 2025 18:27

github-actions bot removed the needs rebase label Sep 30, 2025

coderabbitai bot reviewed Sep 30, 2025

View reviewed changes

kwvg force-pushed the cj_walletman branch from 00a8d0c to eb666d9 Compare October 2, 2025 18:29

coderabbitai bot reviewed Oct 2, 2025

View reviewed changes

PastaPastaPasta reviewed Oct 2, 2025

View reviewed changes

src/coinjoin/context.cpp Outdated Show resolved Hide resolved

github-actions bot mentioned this pull request Oct 3, 2025

refactor: split off governance masternode-only logic to GovernanceSigner, drop Relay()s and use periodic relay instead, minor cleanup #6838

Merged

5 tasks

coderabbitai bot reviewed Oct 3, 2025

View reviewed changes

kwvg force-pushed the cj_walletman branch 2 times, most recently from eb666d9 to 5be7da7 Compare October 5, 2025 17:07

kwvg marked this pull request as draft October 5, 2025 17:10

coderabbitai bot reviewed Oct 5, 2025

View reviewed changes

github-actions bot added the needs rebase label Oct 14, 2025

kwvg added 3 commits October 21, 2025 16:33

refactor: move CDSTXManager out of CJContext

e786b4c

We moved CCoinJoinServer to ActiveContext and 2/3 members of CJContext are conditionally initialized, let's move the unconditionally init'ed member out so we can conditionally initialize the whole object.

refactor: obscure scheduled CoinJoin tasks by moving into context

2606d8c

refactor: remove ENABLE_WALLET from dsnotificationinterface.cpp

cebe18b

Currently a lot of non-wallet code is peppered with `ENABLE_WALLET` due to wallet-only CoinJoin managers, we need to contain them.

kwvg force-pushed the cj_walletman branch from 5be7da7 to a9622da Compare October 21, 2025 11:49

github-actions bot removed the needs rebase label Oct 21, 2025

kwvg added 8 commits October 21, 2025 17:22

refactor: allow MessageProcessingResult to submit DSQ messages

9d06491

refactor: separate MisbehavingError and MessageProcessingResult

cd77127

Placing them in `protocol.h` limits what headers we can include as circular dependencies can balloon unexpectedly, a separate header will reduce how often this happens.

refactor: restrict access to CoinJoin wallet managers

d56879f

refactor: make CJContext an interface, move implementation to source

80c201b

This should prevent ENABLE_WALLET from spilling out to files that include the header.

refactor: drop mn_activeman from CJContext

1a35a65

Now that we no longer assume that cj_ctx is always initialized, we can skip the whole process if we are in masternode mode.

refactor: drop m_is_masternode from CCoinJoinClientQueueManager

5bd1a6b

As it's now a fixed value, we can drop it, yay!

refactor: drop m_is_masternode from CoinJoinWalletManager

4f3da68

refactor: s/CJContext/CJWalletManager/g

6a20464

CJContext is no longer just a context class, it's a lot more involved now and serves triple duty as also a notification interface and a layer of abstraction. Name reflects that it's the wallet-specific components of CoinJoin.

kwvg force-pushed the cj_walletman branch from a9622da to 6a20464 Compare October 21, 2025 11:52

kwvg modified the milestones: 23, 23.1 Oct 21, 2025

kwvg marked this pull request as ready for review October 21, 2025 18:17

kwvg requested a review from PastaPastaPasta October 21, 2025 18:20

coderabbitai bot reviewed Oct 21, 2025

View reviewed changes

github-actions bot mentioned this pull request Oct 21, 2025

perf: make m_peer_mutex a SharedMutex #6468

Merged

5 tasks

PastaPastaPasta approved these changes Oct 22, 2025

View reviewed changes

UdjinM6 approved these changes Oct 22, 2025

View reviewed changes

PastaPastaPasta merged commit 9a5644a into dashpay:develop Oct 24, 2025
83 of 90 checks passed

		@@ -0,0 +1,163 @@
		// Copyright (c) 2023-2025 The Dash Core developers

refactor: move CDSTXManager to NodeContext, abstract wallet-only CoinJoin managers behind interface, s/CJContext/CJWalletManager/g #6840

refactor: move CDSTXManager to NodeContext, abstract wallet-only CoinJoin managers behind interface, s/CJContext/CJWalletManager/g #6840

Uh oh!

Conversation

kwvg commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Additional Information

How Has This Been Tested?

Breaking Changes

Checklist

Uh oh!

github-actions bot commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ Potential Merge Conflicts Detected

Uh oh!

github-actions bot commented Sep 9, 2025

Uh oh!

coderabbitai bot commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

knst commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Sep 30, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Oct 2, 2025

refactor: move `CDSTXManager` to `NodeContext`, abstract wallet-only CoinJoin managers behind interface, `s/CJContext/CJWalletManager/g` #6840

refactor: move `CDSTXManager` to `NodeContext`, abstract wallet-only CoinJoin managers behind interface, `s/CJContext/CJWalletManager/g` #6840

kwvg commented Sep 8, 2025 •

edited

Loading

github-actions bot commented Sep 8, 2025 •

edited

Loading

coderabbitai bot commented Sep 30, 2025 •

edited

Loading

knst commented Sep 30, 2025 •

edited

Loading