Conversation
… of object initializtion in CJ
…unique_ptr inside ForEachCJClientMan
…tManagerImpl CJWalletManagerImpl is private implementation of CJWalletManager It helps to hide internal details of CJWalletManagerImpl which used public CCoinJoinClientManager It reduce amount of abstractions and complexity of implementation.
…Impl It reduce amount of abstractions and levels of inheritance While CJWalletManagerImpl is already private
…erver and make it a member
…injoin_basemanager_tests.cpp
/usr/bin/ld: libbitcoin_node.a(libbitcoin_node_a-walletman.o): in function `CJWalletManagerImpl::UpdatedBlockTip(CBlockIndex const*, CBlockIndex const*, bool)::{lambda(CCoinJoinClientManager&)#1}::operator()(CCoinJoinClientManager&) const':
DASH/src/coinjoin/walletman.cpp:132:(.text+0x51c): undefined reference to `CCoinJoinClientManager::UpdatedBlockTip(CBlockIndex const*)'
…o interfaces::CoinJoin::Client
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acb27a0efa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| LOCK(cs_wallet_manager_map); | ||
| auto it = m_wallet_manager_map.find(name); | ||
| if (it == m_wallet_manager_map.end()) return false; | ||
| func(*it->second); |
There was a problem hiding this comment.
doForClient() now keeps cs_wallet_manager_map locked while running arbitrary callbacks, and several new callbacks call client.resetPool() (for example coinjoin reset in src/rpc/coinjoin.cpp and flushWallet() in this file), which takes wallet locks via CCoinJoinClientSession::ResetPool(). At the same time, wallet code can take the opposite order (cs_wallet then CoinJoin manager lock) through CWallet::NewKeyPoolCallback() calling WithClient() in src/wallet/wallet.cpp, creating a lock-order inversion that can deadlock under concurrent RPC/UI keypool operations and CoinJoin reset/flush paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
not, the whole point of PR is doing it under cs_wallet_manager_map mutex, otherwhise wallet maybe destroyed
WalkthroughThis PR reorganizes CoinJoin queue and client management: it replaces CCoinJoinBaseManager/CCoinJoinClientQueueManager with a standalone CoinJoinQueueManager, moves queue ownership into server/wallet managers, and shifts client lookup from pointer-returning GetClient() to callback-based WithClient()/doForClient() across UI, RPC, and wallet code. CCoinJoinClientManager public methods are renamed to lowerCamelCase and now implement interfaces::CoinJoin::Client. Wallet manager storage moves to a map of client managers with a separate (nullable) CoinJoinQueueManager. Build/test adjustments include moving walletman.cpp between libraries, removing a test, and adding new queue tests. Sequence Diagram(s)sequenceDiagram
participant Caller
participant Loader as interfaces::CoinJoin::Loader
participant WalletMan as CJWalletManagerImpl
participant Client as CCoinJoinClientManager
participant QueueMan as CoinJoinQueueManager
Note over Caller,Client: NEW callback-based flow for client ops
Caller->>Loader: WithClient(wallet_name, callback)
Loader->>WalletMan: doForClient(wallet_name, callback)
WalletMan->>Client: (invoke callback with Client&)
Client->>Client: isMixing()/getJsonInfo()/startMixing()/stopMixing()
WalletMan-->>Loader: returns bool
Loader-->>Caller: returns bool
Note over QueueMan,Server: Queue operations delegated to QueueMan
Caller->>Server: ProcessDSQUEUE / ProcessDSACCEPT
Server->>QueueMan: TryCheckDuplicate(dsq) / TryHasQueueFromMasternode(outpoint)
alt enqueue accepted
Server->>QueueMan: TryAddQueue(dsq) or AddQueue(dsq)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Makefile.am (1)
656-660:⚠️ Potential issue | 🔴 CriticalThe library split still leaves a no-wallet link hole.
The new
#ifdef ENABLE_WALLETinsrc/init.cppremoves the directCJWalletManager::make()call, butsrc/net_processing.cppis still built intolibbitcoin_node.aand still usesCJWalletManagerthroughPeerManagerplus thehasQueue()/processMessage()call sites. Withcoinjoin/walletman.cppmoved only intolibbitcoin_wallet.a, a--disable-walletbuild no longer has an implementation owner for that surface. Either keepcoinjoin/walletman.cppinlibbitcoin_node.afor now, or finish moving the remainingCJWalletManagerreferences behind wallet-only code first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Makefile.am` around lines 656 - 660, The build splits wallet sources into libbitcoin_wallet.a but leaves references to CJWalletManager in code still built into libbitcoin_node.a (notably net_processing.cpp via PeerManager and the hasQueue()/processMessage() call sites), causing unresolved links when --disable-wallet is used; either move coinjoin/walletman.cpp back into the node archive or gate all usages of CJWalletManager (and any calls to PeerManager::hasQueue()/processMessage() that forward to it) behind the same ENABLE_WALLET conditional as used in init.cpp so that net_processing.cpp no longer references CJWalletManager when wallets are disabled; update the Makefile.am entry for libbitcoin_node_a_SOURCES or wrap the CJWalletManager-using code paths in net_processing.cpp/PeerManager with `#ifdef` ENABLE_WALLET to restore a consistent implementation owner.
🧹 Nitpick comments (7)
src/coinjoin/walletman.h (1)
51-53: Document the callback as non-reentrant.
funcruns while the wallet-manager lock is held. A short note that callbacks must not call back intoCJWalletManagerwould make that deadlock boundary explicit for future call sites.📝 Suggested doc tweak
- //! Execute func under the wallet manager lock for the client identified by name. + //! Execute func under the wallet manager lock for the client identified by name. + //! The callback must not re-enter CJWalletManager methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/walletman.h` around lines 51 - 53, The comment for doForClient should explicitly document that the callback func is non-reentrant: add a short sentence to the function comment (for doForClient) stating that func is invoked while the wallet-manager lock is held and must not call back into CJWalletManager or invoke any methods that may acquire the same wallet-manager lock (to avoid deadlocks); reference CCoinJoinClientManager as the callback parameter type and make the non-reentrancy constraint explicit so future callers know not to call CJWalletManager methods from within func.src/qt/overviewpage.cpp (2)
599-607: Potential incorrect state if client unavailable.When
withCoinJoin()fails to find the client,notMixingremainsfalse(its initialization), which means the code path after line 607 treats this as if mixing is active. This could display incorrect UI state.Consider initializing
notMixingtotrueas a safer default:♻️ Suggested safer default
- bool notMixing{false}; + bool notMixing{true}; // Default to not mixing if client unavailable walletModel->withCoinJoin([&](auto& client) { notMixing = !client.isMixing();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/overviewpage.cpp` around lines 599 - 607, The current logic leaves notMixing=false when withCoinJoin() can't find a client, causing the UI to incorrectly show "mixing" active; change the default to true (bool notMixing{true};) so absence of a client is treated as not mixing, and keep the existing update inside walletModel->withCoinJoin(...) that sets notMixing = !client.isMixing(), while preserving the cached-blocks check (client.getCachedBlocks(), client.setCachedBlocks(...), updateCoinJoinProgress()) so state stays correct when a client is present.
581-585: Unchecked return value could mask client availability issues.The
withCoinJoin()return value is ignored. If the client is not found,tooFastremainsfalseand the UI update proceeds. While this is likely benign (the UI updates when it shouldn't skip), consider whether silent failure is the desired behavior across all these call sites, or if a missing client should be logged/handled differently.This pattern is repeated throughout this file (lines 599-607, 665-666, 690-697, 714-715, 734, 745-755, 794).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/overviewpage.cpp` around lines 581 - 585, withCoinJoin's boolean return value is ignored so a missing client can be silently skipped; change the call sites (e.g. the block using walletModel->withCoinJoin, the tooFast assignment that references client.getCachedBlocks()) to capture the return value and handle the false case explicitly: if withCoinJoin(...) returns false, log a warning (or otherwise handle client-unavailable) and avoid treating tooFast as valid, otherwise run the lambda to set tooFast using nBestHeight, client.getCachedBlocks(), GetTime() and nLastDSProgressBlockTime; apply the same pattern to all repeated sites in this file.src/coinjoin/server.cpp (2)
193-193: Inconsistent synchronization strategy: TryAddQueue vs AddQueue.
ProcessDSQUEUEuses the non-blockingTryAddQueue()which silently fails if the lock is contended, whileCreateNewSession(line 728) andCheckForCompleteQueue(line 520) use the blockingAddQueue().This inconsistency means:
- Incoming DSQ messages from peers may be silently dropped under load
- Server-initiated queues (session creation) always succeed
This appears intentional (peer messages are less critical to drop vs server-initiated actions), but the behavioral difference should be documented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/server.cpp` at line 193, ProcessDSQUEUE currently uses the non-blocking m_queueman.TryAddQueue(dsq) while CreateNewSession and CheckForCompleteQueue use the blocking AddQueue(), causing peer DSQ messages to be silently dropped under contention; update the codebase to either (a) unify behavior by replacing TryAddQueue with AddQueue in ProcessDSQUEUE or (b) explicitly document the intended difference—add a clear comment at the ProcessDSQUEUE call site referencing TryAddQueue, explaining that peer DSQ drops are intentional and why, and update any developer docs; ensure you reference the methods TryAddQueue, AddQueue and the functions ProcessDSQUEUE, CreateNewSession, CheckForCompleteQueue so reviewers can locate and verify the change.
90-97: Silent return on lock contention may drop legitimate requests.When
TryHasQueueFromMasternode()returnsstd::nulloptdue to lock contention, the DSACCEPT message is silently dropped without any response to the peer. This differs from the previous blocking behavior where the message would always be processed.While this prevents potential deadlocks, peers may experience intermittent failures to join mixing sessions under load. Consider logging when this occurs for debugging purposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/server.cpp` around lines 90 - 97, When TryHasQueueFromMasternode (called on m_queueman with m_mn_activeman.GetOutPoint()) returns std::nullopt in the DSACCEPT handling, do not silently return: add a LogPrint(BCLog::COINJOIN, ...) that records the lock-contention case including the masternode outpoint and peer info for debugging, and reply to the peer so the request is not dropped (e.g. call PushStatus(peer, STATUS_REJECTED, ERR_BUSY) or another appropriate temporary-failure code) so callers get a clear transient-failure response instead of no response.src/coinjoin/coinjoin.h (1)
352-376: Fix formatting to pass CI pipeline.The pipeline reports clang-format differences on lines 360-364. The
[[nodiscard]]attribute declarations forTryHasQueueFromMasternodeandTryCheckDuplicateneed formatting adjustments.🔧 Suggested formatting fix
- //! TRY_LOCK variant: returns nullopt if lock can't be acquired; true if any queue entry has this - //! outpoint (any readiness). - [[nodiscard]] std::optional<bool> TryHasQueueFromMasternode(const COutPoint& outpoint) const EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue); - //! TRY_LOCK combined duplicate check: returns nullopt if lock can't be acquired; true if dsq is - //! an exact duplicate or the masternode is sending too many dsqs with the same readiness. - [[nodiscard]] std::optional<bool> TryCheckDuplicate(const CCoinJoinQueue& dsq) const EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue); + //! TRY_LOCK variant: returns nullopt if lock can't be acquired; true if any queue entry has this + //! outpoint (any readiness). + [[nodiscard]] std::optional<bool> TryHasQueueFromMasternode(const COutPoint& outpoint) const + EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue); + //! TRY_LOCK combined duplicate check: returns nullopt if lock can't be acquired; true if dsq is + //! an exact duplicate or the masternode is sending too many dsqs with the same readiness. + [[nodiscard]] std::optional<bool> TryCheckDuplicate(const CCoinJoinQueue& dsq) const + EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/coinjoin.h` around lines 352 - 376, Reformat the two function declarations so clang-format is happy: put the [[nodiscard]] attribute directly adjacent to the function declaration rather than on its own line and ensure spacing matches the surrounding declarations; specifically update the declarations for TryHasQueueFromMasternode and TryCheckDuplicate so the [[nodiscard]] attribute precedes the function signature on the same line and keep the EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue) annotation and ending semicolon intact.src/coinjoin/client.cpp (1)
140-152: Fix formatting to pass CI pipeline.The pipeline reports clang-format differences on lines 137-155. The method signatures for
startMixing,stopMixing, andisMixingneed formatting adjustments.🔧 Suggested formatting fix
-bool CCoinJoinClientManager::startMixing() { +bool CCoinJoinClientManager::startMixing() +{ bool expected{false}; return fMixing.compare_exchange_strong(expected, true); } -void CCoinJoinClientManager::stopMixing() { +void CCoinJoinClientManager::stopMixing() +{ fMixing = false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/client.cpp` around lines 140 - 152, Reformat the three methods to match project clang-format style: place the opening brace on the same line as the function signature, use consistent spacing for initializers (e.g., bool expected = false or bool expected{false} per project style), and ensure return/statements and braces follow the same indentation and spacing conventions; update CCoinJoinClientManager::startMixing, CCoinJoinClientManager::stopMixing, and CCoinJoinClientManager::isMixing accordingly so clang-format no longer reports differences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/coinjoin/server.cpp`:
- Line 904: Clang-format flagged inconsistent spacing on the obj.pushKV line;
reformat the line that calls obj.pushKV("queue_size", m_queueman.GetQueueSize())
so spacing and alignment match project clang-format rules (run clang-format or
your editor's format command), ensuring the call to obj.pushKV and the
m_queueman.GetQueueSize() argument use the same spacing style as surrounding
pushKV calls.
- Around line 159-165: Run clang-format on the affected file and fix the
formatting of the LogPrint line in the block where you call
m_queueman.TryCheckDuplicate(dsq); specifically reflow/wrap the long LogPrint
invocation (the line referencing BCLog::COINJOIN, the format string "DSQUEUE --
Peer %d is sending WAY too many dsq messages for a masternode with collateral
%s\n", and the arguments from and dsq.masternodeOutpoint.ToStringShort()) so it
matches the project's clang-format style (proper indentation and line breaks)
and then commit the formatted change.
In `@src/rpc/coinjoin.cpp`:
- Around line 281-286: The coinjoin salt generation path (coinjoinsalt_generate)
currently calls node.coinjoin_loader->WithClient(...) and silently proceeds if
WithClient returns false, skipping the mixing check; update the call to assert
the client lookup succeeded using CHECK_NONFATAL so the RPC fails early when the
client isn't found (i.e., wrap the WithClient invocation result in
CHECK_NONFATAL and preserve the existing mixing check that throws JSONRPCError
if client.isMixing()), referencing coinjoin_loader->WithClient,
coinjoinsalt_generate, CHECK_NONFATAL, and client.isMixing to locate and fix the
code.
---
Outside diff comments:
In `@src/Makefile.am`:
- Around line 656-660: The build splits wallet sources into libbitcoin_wallet.a
but leaves references to CJWalletManager in code still built into
libbitcoin_node.a (notably net_processing.cpp via PeerManager and the
hasQueue()/processMessage() call sites), causing unresolved links when
--disable-wallet is used; either move coinjoin/walletman.cpp back into the node
archive or gate all usages of CJWalletManager (and any calls to
PeerManager::hasQueue()/processMessage() that forward to it) behind the same
ENABLE_WALLET conditional as used in init.cpp so that net_processing.cpp no
longer references CJWalletManager when wallets are disabled; update the
Makefile.am entry for libbitcoin_node_a_SOURCES or wrap the
CJWalletManager-using code paths in net_processing.cpp/PeerManager with `#ifdef`
ENABLE_WALLET to restore a consistent implementation owner.
---
Nitpick comments:
In `@src/coinjoin/client.cpp`:
- Around line 140-152: Reformat the three methods to match project clang-format
style: place the opening brace on the same line as the function signature, use
consistent spacing for initializers (e.g., bool expected = false or bool
expected{false} per project style), and ensure return/statements and braces
follow the same indentation and spacing conventions; update
CCoinJoinClientManager::startMixing, CCoinJoinClientManager::stopMixing, and
CCoinJoinClientManager::isMixing accordingly so clang-format no longer reports
differences.
In `@src/coinjoin/coinjoin.h`:
- Around line 352-376: Reformat the two function declarations so clang-format is
happy: put the [[nodiscard]] attribute directly adjacent to the function
declaration rather than on its own line and ensure spacing matches the
surrounding declarations; specifically update the declarations for
TryHasQueueFromMasternode and TryCheckDuplicate so the [[nodiscard]] attribute
precedes the function signature on the same line and keep the
EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue) annotation and ending semicolon intact.
In `@src/coinjoin/server.cpp`:
- Line 193: ProcessDSQUEUE currently uses the non-blocking
m_queueman.TryAddQueue(dsq) while CreateNewSession and CheckForCompleteQueue use
the blocking AddQueue(), causing peer DSQ messages to be silently dropped under
contention; update the codebase to either (a) unify behavior by replacing
TryAddQueue with AddQueue in ProcessDSQUEUE or (b) explicitly document the
intended difference—add a clear comment at the ProcessDSQUEUE call site
referencing TryAddQueue, explaining that peer DSQ drops are intentional and why,
and update any developer docs; ensure you reference the methods TryAddQueue,
AddQueue and the functions ProcessDSQUEUE, CreateNewSession,
CheckForCompleteQueue so reviewers can locate and verify the change.
- Around line 90-97: When TryHasQueueFromMasternode (called on m_queueman with
m_mn_activeman.GetOutPoint()) returns std::nullopt in the DSACCEPT handling, do
not silently return: add a LogPrint(BCLog::COINJOIN, ...) that records the
lock-contention case including the masternode outpoint and peer info for
debugging, and reply to the peer so the request is not dropped (e.g. call
PushStatus(peer, STATUS_REJECTED, ERR_BUSY) or another appropriate
temporary-failure code) so callers get a clear transient-failure response
instead of no response.
In `@src/coinjoin/walletman.h`:
- Around line 51-53: The comment for doForClient should explicitly document that
the callback func is non-reentrant: add a short sentence to the function comment
(for doForClient) stating that func is invoked while the wallet-manager lock is
held and must not call back into CJWalletManager or invoke any methods that may
acquire the same wallet-manager lock (to avoid deadlocks); reference
CCoinJoinClientManager as the callback parameter type and make the
non-reentrancy constraint explicit so future callers know not to call
CJWalletManager methods from within func.
In `@src/qt/overviewpage.cpp`:
- Around line 599-607: The current logic leaves notMixing=false when
withCoinJoin() can't find a client, causing the UI to incorrectly show "mixing"
active; change the default to true (bool notMixing{true};) so absence of a
client is treated as not mixing, and keep the existing update inside
walletModel->withCoinJoin(...) that sets notMixing = !client.isMixing(), while
preserving the cached-blocks check (client.getCachedBlocks(),
client.setCachedBlocks(...), updateCoinJoinProgress()) so state stays correct
when a client is present.
- Around line 581-585: withCoinJoin's boolean return value is ignored so a
missing client can be silently skipped; change the call sites (e.g. the block
using walletModel->withCoinJoin, the tooFast assignment that references
client.getCachedBlocks()) to capture the return value and handle the false case
explicitly: if withCoinJoin(...) returns false, log a warning (or otherwise
handle client-unavailable) and avoid treating tooFast as valid, otherwise run
the lambda to set tooFast using nBestHeight, client.getCachedBlocks(), GetTime()
and nLastDSProgressBlockTime; apply the same pattern to all repeated sites in
this file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 252bc396-6bb7-4302-80d6-7814b1bb0bef
📒 Files selected for processing (25)
contrib/auto_gdb/simple_class_obj.pysrc/Makefile.amsrc/Makefile.test.includesrc/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/interfaces.cppsrc/coinjoin/server.cppsrc/coinjoin/server.hsrc/coinjoin/walletman.cppsrc/coinjoin/walletman.hsrc/init.cppsrc/interfaces/coinjoin.hsrc/qt/bitcoingui.cppsrc/qt/optionsdialog.cppsrc/qt/overviewpage.cppsrc/qt/walletmodel.cppsrc/qt/walletmodel.hsrc/rpc/coinjoin.cppsrc/test/coinjoin_basemanager_tests.cppsrc/test/coinjoin_queue_tests.cppsrc/wallet/init.cppsrc/wallet/test/coinjoin_tests.cppsrc/wallet/wallet.cpp
💤 Files with no reviewable changes (2)
- src/Makefile.test.include
- src/test/coinjoin_basemanager_tests.cpp
| const auto isDup = m_queueman.TryCheckDuplicate(dsq); | ||
| if (!isDup.has_value()) return; | ||
| if (*isDup) { | ||
| LogPrint(BCLog::COINJOIN, "DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with collateral %s\n", from, dsq.masternodeOutpoint.ToStringShort()); | ||
| return; | ||
| } | ||
| } // cs_vecqueue | ||
| } |
There was a problem hiding this comment.
Pipeline failure: Clang-format issue.
The CI pipeline reports a formatting difference on these lines. Please run clang-format to fix the indentation/wrapping of the LogPrint statement.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 159-161: Clang-format-diff.py reported formatting differences (LogPrint wrapped/indented). This step failed due to detected diff; exit code 1.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/server.cpp` around lines 159 - 165, Run clang-format on the
affected file and fix the formatting of the LogPrint line in the block where you
call m_queueman.TryCheckDuplicate(dsq); specifically reflow/wrap the long
LogPrint invocation (the line referencing BCLog::COINJOIN, the format string
"DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with
collateral %s\n", and the arguments from and
dsq.masternodeOutpoint.ToStringShort()) so it matches the project's clang-format
style (proper indentation and line breaks) and then commit the formatted change.
| obj.clear(); | ||
| obj.setObject(); | ||
| obj.pushKV("queue_size", GetQueueSize()); | ||
| obj.pushKV("queue_size", m_queueman.GetQueueSize()); |
There was a problem hiding this comment.
Pipeline failure: Clang-format issue.
The CI pipeline reports a formatting difference on this line (spacing/alignment of pushKV). Please run clang-format to fix.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 901-904: Clang-format-diff.py reported formatting differences (obj.pushKV queue_size spacing alignment). This step failed due to detected diff; exit code 1.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/server.cpp` at line 904, Clang-format flagged inconsistent
spacing on the obj.pushKV line; reformat the line that calls
obj.pushKV("queue_size", m_queueman.GetQueueSize()) so spacing and alignment
match project clang-format rules (run clang-format or your editor's format
command), ensuring the call to obj.pushKV and the m_queueman.GetQueueSize()
argument use the same spacing style as surrounding pushKV calls.
| node.coinjoin_loader->WithClient(wallet->GetName(), [&](auto& client) { | ||
| if (client.isMixing()) { | ||
| throw JSONRPCError(RPC_WALLET_ERROR, | ||
| strprintf("Wallet \"%s\" is currently mixing, cannot change salt!", str_wallet)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Silent no-op when client not found during salt generation.
Unlike other RPC handlers that use CHECK_NONFATAL, coinjoinsalt_generate silently continues if WithClient returns false (client not found). This means the mixing check is skipped, which could allow salt generation while mixing if the client lookup fails for some reason.
Consider adding CHECK_NONFATAL for consistency:
♻️ Suggested fix for consistency
if (node.coinjoin_loader != nullptr) {
- node.coinjoin_loader->WithClient(wallet->GetName(), [&](auto& client) {
+ CHECK_NONFATAL(node.coinjoin_loader->WithClient(wallet->GetName(), [&](auto& client) {
if (client.isMixing()) {
throw JSONRPCError(RPC_WALLET_ERROR,
strprintf("Wallet \"%s\" is currently mixing, cannot change salt!", str_wallet));
}
- });
+ }));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| node.coinjoin_loader->WithClient(wallet->GetName(), [&](auto& client) { | |
| if (client.isMixing()) { | |
| throw JSONRPCError(RPC_WALLET_ERROR, | |
| strprintf("Wallet \"%s\" is currently mixing, cannot change salt!", str_wallet)); | |
| } | |
| }); | |
| CHECK_NONFATAL(node.coinjoin_loader->WithClient(wallet->GetName(), [&](auto& client) { | |
| if (client.isMixing()) { | |
| throw JSONRPCError(RPC_WALLET_ERROR, | |
| strprintf("Wallet \"%s\" is currently mixing, cannot change salt!", str_wallet)); | |
| } | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rpc/coinjoin.cpp` around lines 281 - 286, The coinjoin salt generation
path (coinjoinsalt_generate) currently calls
node.coinjoin_loader->WithClient(...) and silently proceeds if WithClient
returns false, skipping the mixing check; update the call to assert the client
lookup succeeded using CHECK_NONFATAL so the RPC fails early when the client
isn't found (i.e., wrap the WithClient invocation result in CHECK_NONFATAL and
preserve the existing mixing check that throws JSONRPCError if
client.isMixing()), referencing coinjoin_loader->WithClient,
coinjoinsalt_generate, CHECK_NONFATAL, and client.isMixing to locate and fix the
code.
There was a problem hiding this comment.
logic is matched between versions; no new bugs introduced or any logic changed.
There was a problem hiding this comment.
@knst, you're right — I missed the equivalence. The original code explicitly guarded with cj_clientman != nullptr, meaning the mixing check was intentionally skipped when no client was found. The new WithClient call preserves exactly that behavior: when the client doesn't exist, WithClient returns false without invoking the callback, so the salt generation proceeds unimpeded — just as before. Adding CHECK_NONFATAL would have been a behavioral regression, not a fix. Thanks for the clarification!
knst
force-pushed
the
fix-cj-dangling-ptr
branch
from
acb27a0 to
b017394
Compare
Review GateCommit:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b017394a9c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (fAutoStart) { | ||
| mgr.startMixing(); | ||
| } |
There was a problem hiding this comment.
InitCoinJoinSettings() now calls mgr.startMixing() unconditionally when -coinjoinautostart=1, but this function no longer has any wallet-lock check. For encrypted wallets that start locked, this sets isMixing() to true even though DoAutomaticDenominating() exits early on lock, so the client reports a running mix session that cannot proceed; this also blocks operations like coinjoinsalt_generate/coinjoinsalt_set that reject while mixing is active. The previous logic explicitly stopped mixing for locked wallets, so this is a behavioral regression for locked-at-startup nodes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/coinjoin/client.cpp`:
- Around line 140-152: Reformat the three CCoinJoinClientManager methods
(CCoinJoinClientManager::startMixing, ::stopMixing, and ::isMixing) to match
project clang-format style: place the opening brace on the same line as the
function signature, ensure spacing around return types and parameters is
consistent, and keep single-line bodies compact (e.g., return fMixing; or
fMixing = false; as appropriate); run clang-format or apply the repo's style so
these method definitions and use of the atomic fMixing variable conform to CI
formatting rules.
In `@src/coinjoin/interfaces.cpp`:
- Around line 44-50: The lambda passed to manager().doForClient in AddWallet()
is mis-indented and fails clang-format; reformat the call so the lambda's
opening bracket and body align with the call (ensure the capture/signature and
body are indented consistently), e.g. place the lambda on the next line with its
body indented and the closing "});" aligned with the start of the
manager().doForClient call; target the AddWallet function and the
manager().doForClient(..., [&](CCoinJoinClientManager& mgr) { ... }) lambda for
the fix.
In `@src/coinjoin/walletman.cpp`:
- Around line 39-79: The function declarations with EXCLUSIVE_LOCKS_REQUIRED
annotations (doForClient, processMessage, getMixingMasternodes, addWallet,
removeWallet, flushWallet, UpdatedBlockTip, DoMaintenance, and ProcessDSQueue)
fail clang-format; reflow each declaration so the EXCLUSIVE_LOCKS_REQUIRED
attribute is placed on its own trailing line (not inline with parameters) per
project style — keep the return type, function name and parameter list together,
then place the EXCLUSIVE_LOCKS_REQUIRED(...) on the next line for each of the
listed symbols to satisfy formatting checks.
In `@src/wallet/init.cpp`:
- Around line 204-213: The default for -enablecoinjoin in
WalletInit::InitCoinJoinSettings is currently true
(gArgs.GetBoolArg("-enablecoinjoin", true)) which contradicts the documented
default of 0; change the fallback value to false (or to whatever project-wide
DEFAULT_COINJOIN_ENABLED constant you use) in the GetBoolArg call and keep
CCoinJoinClientOptions::SetEnabled(...) as is — alternatively, update the help
text to reflect that coinjoin is enabled by default; see
WalletInit::InitCoinJoinSettings, CCoinJoinClientOptions::SetEnabled, and
gArgs.GetBoolArg("-enablecoinjoin", ...) to make the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c6feb52a-bb86-4e9a-88f9-6dc0853dd1b8
📒 Files selected for processing (18)
src/Makefile.amsrc/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/interfaces.cppsrc/coinjoin/walletman.cppsrc/coinjoin/walletman.hsrc/init.cppsrc/interfaces/coinjoin.hsrc/qt/bitcoingui.cppsrc/qt/optionsdialog.cppsrc/qt/overviewpage.cppsrc/qt/walletmodel.cppsrc/qt/walletmodel.hsrc/rpc/coinjoin.cppsrc/wallet/init.cppsrc/wallet/test/coinjoin_tests.cppsrc/wallet/wallet.cppsrc/walletinitinterface.h
✅ Files skipped from review due to trivial changes (4)
- src/Makefile.am
- src/wallet/wallet.cpp
- src/init.cpp
- src/interfaces/coinjoin.h
🚧 Files skipped from review as they are similar to previous changes (7)
- src/qt/optionsdialog.cpp
- src/wallet/test/coinjoin_tests.cpp
- src/qt/bitcoingui.cpp
- src/coinjoin/walletman.h
- src/qt/walletmodel.cpp
- src/qt/overviewpage.cpp
- src/rpc/coinjoin.cpp
| bool CCoinJoinClientManager::startMixing() { | ||
| bool expected{false}; | ||
| return fMixing.compare_exchange_strong(expected, true); | ||
| } | ||
|
|
||
| void CCoinJoinClientManager::StopMixing() { | ||
| void CCoinJoinClientManager::stopMixing() { | ||
| fMixing = false; | ||
| } | ||
|
|
||
| bool CCoinJoinClientManager::IsMixing() const | ||
| bool CCoinJoinClientManager::isMixing() const | ||
| { | ||
| return fMixing; | ||
| } |
There was a problem hiding this comment.
Fix formatting to pass CI checks.
The CI pipeline reports clang-format errors for the method declarations/definitions at lines 140-152. The brace placement and method signatures need reformatting.
🔧 Suggested formatting fix
-bool CCoinJoinClientManager::startMixing() {
+bool CCoinJoinClientManager::startMixing()
+{
bool expected{false};
return fMixing.compare_exchange_strong(expected, true);
}
-void CCoinJoinClientManager::stopMixing() {
+void CCoinJoinClientManager::stopMixing()
+{
fMixing = false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool CCoinJoinClientManager::startMixing() { | |
| bool expected{false}; | |
| return fMixing.compare_exchange_strong(expected, true); | |
| } | |
| void CCoinJoinClientManager::StopMixing() { | |
| void CCoinJoinClientManager::stopMixing() { | |
| fMixing = false; | |
| } | |
| bool CCoinJoinClientManager::IsMixing() const | |
| bool CCoinJoinClientManager::isMixing() const | |
| { | |
| return fMixing; | |
| } | |
| bool CCoinJoinClientManager::startMixing() | |
| { | |
| bool expected{false}; | |
| return fMixing.compare_exchange_strong(expected, true); | |
| } | |
| void CCoinJoinClientManager::stopMixing() | |
| { | |
| fMixing = false; | |
| } | |
| bool CCoinJoinClientManager::isMixing() const | |
| { | |
| return fMixing; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/client.cpp` around lines 140 - 152, Reformat the three
CCoinJoinClientManager methods (CCoinJoinClientManager::startMixing,
::stopMixing, and ::isMixing) to match project clang-format style: place the
opening brace on the same line as the function signature, ensure spacing around
return types and parameters is consistent, and keep single-line bodies compact
(e.g., return fMixing; or fMixing = false; as appropriate); run clang-format or
apply the repo's style so these method definitions and use of the atomic fMixing
variable conform to CI formatting rules.
| void AddWallet(const std::shared_ptr<CWallet>& wallet) override | ||
| { | ||
| manager().addWallet(wallet); | ||
| g_wallet_init_interface.InitCoinJoinSettings(*this, wallet_loader()); | ||
| manager().doForClient(wallet->GetName(), [&](CCoinJoinClientManager& mgr) { | ||
| g_wallet_init_interface.InitCoinJoinSettings(mgr); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fix lambda indentation to pass CI checks.
The CI pipeline reports clang-format errors for the lambda call indentation in AddWallet().
🔧 Suggested formatting fix
void AddWallet(const std::shared_ptr<CWallet>& wallet) override
{
manager().addWallet(wallet);
- manager().doForClient(wallet->GetName(), [&](CCoinJoinClientManager& mgr) {
- g_wallet_init_interface.InitCoinJoinSettings(mgr);
- });
+ manager().doForClient(wallet->GetName(),
+ [&](CCoinJoinClientManager& mgr) { g_wallet_init_interface.InitCoinJoinSettings(mgr); });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void AddWallet(const std::shared_ptr<CWallet>& wallet) override | |
| { | |
| manager().addWallet(wallet); | |
| g_wallet_init_interface.InitCoinJoinSettings(*this, wallet_loader()); | |
| manager().doForClient(wallet->GetName(), [&](CCoinJoinClientManager& mgr) { | |
| g_wallet_init_interface.InitCoinJoinSettings(mgr); | |
| }); | |
| } | |
| void AddWallet(const std::shared_ptr<CWallet>& wallet) override | |
| { | |
| manager().addWallet(wallet); | |
| manager().doForClient(wallet->GetName(), | |
| [&](CCoinJoinClientManager& mgr) { g_wallet_init_interface.InitCoinJoinSettings(mgr); }); | |
| } |
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 44-44: Clang-format would reformat lambda call indentation in AddWallet().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/interfaces.cpp` around lines 44 - 50, The lambda passed to
manager().doForClient in AddWallet() is mis-indented and fails clang-format;
reformat the call so the lambda's opening bracket and body align with the call
(ensure the capture/signature and body are indented consistently), e.g. place
the lambda on the next line with its body indented and the closing "});" aligned
with the start of the manager().doForClient call; target the AddWallet function
and the manager().doForClient(..., [&](CCoinJoinClientManager& mgr) { ... })
lambda for the fix.
|
|
||
| public: | ||
| bool hasQueue(const uint256& hash) const override; | ||
| CCoinJoinClientManager* getClient(const std::string& name) override; | ||
| bool doForClient(const std::string& name, const std::function<void(CCoinJoinClientManager&)>& func) override EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map); | ||
| MessageProcessingResult processMessage(CNode& peer, CChainState& chainstate, CConnman& connman, CTxMemPool& mempool, | ||
| std::string_view msg_type, CDataStream& vRecv) override; | ||
| std::string_view msg_type, CDataStream& vRecv) override EXCLUSIVE_LOCKS_REQUIRED(!cs_ProcessDSQueue, !cs_wallet_manager_map); | ||
| std::optional<CCoinJoinQueue> getQueueFromHash(const uint256& hash) const override; | ||
| std::optional<int> getQueueSize() const override; | ||
| std::vector<CDeterministicMNCPtr> getMixingMasternodes() override; | ||
| void addWallet(const std::shared_ptr<wallet::CWallet>& wallet) override; | ||
| void removeWallet(const std::string& name) override; | ||
| void flushWallet(const std::string& name) override; | ||
| std::vector<CDeterministicMNCPtr> getMixingMasternodes() override EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map); | ||
| void addWallet(const std::shared_ptr<wallet::CWallet>& wallet) override EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map); | ||
| void removeWallet(const std::string& name) override EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map); | ||
| void flushWallet(const std::string& name) override EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map); | ||
|
|
||
| protected: | ||
| // CValidationInterface | ||
| void UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload) override; | ||
| void UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload) override EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map); | ||
|
|
||
| private: | ||
| const bool m_relay_txes; | ||
| ChainstateManager& m_chainman; | ||
| CDeterministicMNManager& m_dmnman; | ||
| CMasternodeMetaMan& m_mn_metaman; | ||
| CTxMemPool& m_mempool; | ||
| const CMasternodeSync& m_mn_sync; | ||
| const llmq::CInstantSendManager& m_isman; | ||
|
|
||
| // m_queueman is declared before the wallet map so that it is destroyed | ||
| // after all CCoinJoinClientManager instances (which hold a raw pointer to it). | ||
| // Null when relay_txes is false (no queue processing). | ||
| const std::unique_ptr<CoinJoinQueueManager> m_queueman; | ||
|
|
||
| mutable Mutex cs_ProcessDSQueue; | ||
|
|
||
| mutable Mutex cs_wallet_manager_map; | ||
| std::map<const std::string, std::unique_ptr<CCoinJoinClientManager>> m_wallet_manager_map GUARDED_BY(cs_wallet_manager_map); | ||
|
|
||
| void DoMaintenance(CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map); | ||
|
|
||
| [[nodiscard]] MessageProcessingResult ProcessDSQueue(NodeId from, CConnman& connman, std::string_view msg_type, | ||
| CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_ProcessDSQueue, !cs_wallet_manager_map); | ||
|
|
There was a problem hiding this comment.
Fix formatting to pass CI checks.
The CI pipeline reports clang-format errors for the function declarations with EXCLUSIVE_LOCKS_REQUIRED annotations. These need to be reflowed onto multiple lines per the project's formatting rules.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 39-79: Clang-format would reflow function declarations/override lines with EXCLUSIVE_LOCKS_REQUIRED onto multiple lines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/walletman.cpp` around lines 39 - 79, The function declarations
with EXCLUSIVE_LOCKS_REQUIRED annotations (doForClient, processMessage,
getMixingMasternodes, addWallet, removeWallet, flushWallet, UpdatedBlockTip,
DoMaintenance, and ProcessDSQueue) fail clang-format; reflow each declaration so
the EXCLUSIVE_LOCKS_REQUIRED attribute is placed on its own trailing line (not
inline with parameters) per project style — keep the return type, function name
and parameter list together, then place the EXCLUSIVE_LOCKS_REQUIRED(...) on the
next line for each of the listed symbols to satisfy formatting checks.
| void WalletInit::InitCoinJoinSettings(CCoinJoinClientManager& mgr) const | ||
| { | ||
| const auto& wallets{wallet_loader.getWallets()}; | ||
| CCoinJoinClientOptions::SetEnabled(!wallets.empty() ? gArgs.GetBoolArg("-enablecoinjoin", true) : false); | ||
| CCoinJoinClientOptions::SetEnabled(gArgs.GetBoolArg("-enablecoinjoin", true)); | ||
| if (!CCoinJoinClientOptions::IsEnabled()) { | ||
| return; | ||
| } | ||
| bool fAutoStart = gArgs.GetBoolArg("-coinjoinautostart", DEFAULT_COINJOIN_AUTOSTART); | ||
| for (auto& wallet : wallets) { | ||
| auto manager = Assert(coinjoin_loader.GetClient(wallet->getWalletName())); | ||
| if (wallet->isLocked(/*fForMixing=*/false)) { | ||
| manager->stopMixing(); | ||
| LogPrintf("CoinJoin: Mixing stopped for locked wallet \"%s\"\n", wallet->getWalletName()); | ||
| } else if (fAutoStart) { | ||
| manager->startMixing(); | ||
| LogPrintf("CoinJoin: Automatic mixing started for wallet \"%s\"\n", wallet->getWalletName()); | ||
| } | ||
| if (fAutoStart) { | ||
| mgr.startMixing(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the documented default value vs actual behavior
echo "=== Checking -enablecoinjoin arg registration ==="
rg -n "enablecoinjoin" --type=cpp -C2
echo ""
echo "=== Checking DEFAULT_COINJOIN values ==="
rg -n "DEFAULT_COINJOIN" --type=cpp src/coinjoin/Repository: dashpay/dash
Length of output: 5546
Fix the -enablecoinjoin default value mismatch.
At line 206, GetBoolArg("-enablecoinjoin", true) defaults to true, but the help text at line 103 documents the default as 0 (false). Either update the fallback value to match the documented default or update the documentation to reflect that CoinJoin is enabled by default during wallet initialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/init.cpp` around lines 204 - 213, The default for -enablecoinjoin
in WalletInit::InitCoinJoinSettings is currently true
(gArgs.GetBoolArg("-enablecoinjoin", true)) which contradicts the documented
default of 0; change the fallback value to false (or to whatever project-wide
DEFAULT_COINJOIN_ENABLED constant you use) in the GetBoolArg call and keep
CCoinJoinClientOptions::SetEnabled(...) as is — alternatively, update the help
text to reflect that coinjoin is enabled by default; see
WalletInit::InitCoinJoinSettings, CCoinJoinClientOptions::SetEnabled, and
gArgs.GetBoolArg("-enablecoinjoin", ...) to make the fix.
b017394 to
5c2f753
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c2f753b78
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Dash Specific WalletInitInterface | ||
| virtual void AutoLockMasternodeCollaterals(interfaces::WalletLoader& wallet_loader) const = 0; | ||
| virtual void InitCoinJoinSettings(interfaces::CoinJoin::Loader& coinjoin_loader, interfaces::WalletLoader& wallet_loader) const = 0; | ||
| virtual void InitCoinJoinSettings(CCoinJoinClientManager& mgr) const = 0; |
There was a problem hiding this comment.
Changing WalletInitInterface::InitCoinJoinSettings to take CCoinJoinClientManager& here leaves the non-wallet implementation out of sync: DummyWalletInit still declares InitCoinJoinSettings(interfaces::CoinJoin::Loader&, interfaces::WalletLoader&) in src/dummywallet.cpp. In --disable-wallet builds that signature no longer overrides the pure virtual method, so the dummy wallet target fails to compile (or remains abstract).
Useful? React with 👍 / 👎.
knst
force-pushed
the
fix-cj-dangling-ptr
branch
from
5c2f753 to
92d0f34
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92d0f348f8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!wallet->IsLocked()) { | ||
| g_wallet_init_interface.InitCoinJoinSettings(mgr); | ||
| } |
There was a problem hiding this comment.
CoinJoinLoaderImpl::AddWallet now calls InitCoinJoinSettings only when !wallet->IsLocked(), but CoinJoinLoaderImpl sets CCoinJoinClientOptions::SetEnabled(false) in its constructor and this is the only place settings are re-initialized. For encrypted wallets that start locked, CoinJoin stays globally disabled forever (even after later unlock), so coinjoin start/status/stop/reset fail in ValidateCoinJoinArguments() with the internal-disabled error. This is a functional regression for the default locked-at-startup wallet flow.
Useful? React with 👍 / 👎.
2 participants
Depends on #7248
Issue being fixed or feature implemented
Hold lock during flush operations to prevent TOCTOU race condition.
getClient() releases cs_wallet_manager_map before returning the raw pointer. If another thread calls removeWallet() concurrently, the unique_ptr is destroyed while flushWallet() still holds a reference, leading to use-after-free. This is inconsistent with removeWallet() which safely erases under the lock.
Discovered here #7248 (comment)
What was done?
Use
doWithClient(name, [](){...})instead ofgetClient(name)->...How Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Checklist: