Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
![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 refactors the CoinJoin subsystem's client access pattern from direct pointer return to callback-based invocation. It renames CoinJoin client methods to lowerCamelCase ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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.
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
|
|
||
| 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.
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 👍 / 👎.
/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
knst
force-pushed
the
fix-cj-dangling-ptr
branch
from
92d0f34 to
af2d8ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/coinjoin/interfaces.cpp (1)
42-48:⚠️ Potential issue | 🟡 MinorAlign the
-enablecoinjoinfallback with the advertised default.
src/wallet/init.cppstill documents-enablecoinjoinas defaulting to0, but this path enables CoinJoin when the flag is omitted. Please make this fallback consistent with the help text, and keepsrc/rpc/coinjoin.cpp’sValidateCoinJoinArguments()on the same shared default as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/interfaces.cpp` around lines 42 - 48, The code currently enables CoinJoin by default via gArgs.GetBoolArg("-enablecoinjoin", true) in CCoinJoinClientOptions::SetEnabled which conflicts with the help text in src/wallet/init.cpp and the shared default used by ValidateCoinJoinArguments(); change the fallback to false (use gArgs.GetBoolArg("-enablecoinjoin", false)) so the absent flag defaults to disabled, and ensure ValidateCoinJoinArguments() and any other consumers reference the same shared default/constant rather than hardcoding true/false so the behavior and help text remain consistent (update any shared default variable or call site to use the unified default).
🧹 Nitpick comments (1)
src/interfaces/coinjoin.h (1)
50-52: Document the callback lifetime contract explicitly.Please spell out that the
Client&passed toWithClient(...)is only valid for the duration offuncand must not be stored. That is the safety property replacing the old raw-pointer API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interfaces/coinjoin.h` around lines 50 - 52, The comment for WithClient(...) must explicitly state the lifetime contract: update the docstring for virtual bool WithClient(const std::string& name, std::function<void(Client&)> func) to clearly say the Client& passed into func is only valid for the duration of the callback invocation and must not be stored, copied, or used after func returns (this is the safety guarantee replacing the old raw-pointer API); mention that callers should perform any needed work inside func or copy safe data out, and that implementations must ensure the reference becomes invalid immediately after func returns.
🤖 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/walletman.cpp`:
- Around line 318-320: The non-wallet factory branch in
src/coinjoin/walletman.cpp currently does assert(false) and then falls off the
end of a non-void function in release builds; update the non-wallet path (the
factory function in walletman.cpp that constructs/returns a wallet object when
wallet support is disabled) to return an explicit safe default (e.g., return
nullptr) after the assert(false) so the function has a defined return value in
non-debug builds; ensure the returned value matches the function's return type
(use nullptr for pointer returns or an appropriate sentinel value) and keep the
assert for debug-time diagnostics.
---
Duplicate comments:
In `@src/coinjoin/interfaces.cpp`:
- Around line 42-48: The code currently enables CoinJoin by default via
gArgs.GetBoolArg("-enablecoinjoin", true) in CCoinJoinClientOptions::SetEnabled
which conflicts with the help text in src/wallet/init.cpp and the shared default
used by ValidateCoinJoinArguments(); change the fallback to false (use
gArgs.GetBoolArg("-enablecoinjoin", false)) so the absent flag defaults to
disabled, and ensure ValidateCoinJoinArguments() and any other consumers
reference the same shared default/constant rather than hardcoding true/false so
the behavior and help text remain consistent (update any shared default variable
or call site to use the unified default).
---
Nitpick comments:
In `@src/interfaces/coinjoin.h`:
- Around line 50-52: The comment for WithClient(...) must explicitly state the
lifetime contract: update the docstring for virtual bool WithClient(const
std::string& name, std::function<void(Client&)> func) to clearly say the Client&
passed into func is only valid for the duration of the callback invocation and
must not be stored, copied, or used after func returns (this is the safety
guarantee replacing the old raw-pointer API); mention that callers should
perform any needed work inside func or copy safe data out, and that
implementations must ensure the reference becomes invalid immediately after func
returns.
🪄 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: 2911219b-5305-430e-b53b-aaba4ec97f6c
📒 Files selected for processing (19)
src/Makefile.amsrc/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/interfaces.cppsrc/coinjoin/walletman.cppsrc/coinjoin/walletman.hsrc/dummywallet.cppsrc/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 (3)
- src/Makefile.am
- src/qt/optionsdialog.cpp
- src/init.cpp
🚧 Files skipped from review as they are similar to previous changes (7)
- src/qt/bitcoingui.cpp
- src/coinjoin/walletman.h
- src/qt/walletmodel.cpp
- src/walletinitinterface.h
- src/qt/walletmodel.h
- src/qt/overviewpage.cpp
- src/coinjoin/client.cpp
| #else | ||
| // Cannot be constructed if wallet support isn't built | ||
| return nullptr; | ||
| assert(false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba src/coinjoin/walletman.cpp | sed -n '311,322p'Repository: dashpay/dash
Length of output: 98
🏁 Script executed:
sed -n '311,322p' src/coinjoin/walletman.cppRepository: dashpay/dash
Length of output: 729
Return after assert(false) in the non-wallet factory path.
In release builds, assert(false) compiles out, so this branch falls off the end of a non-void function. This breaks -nowallet builds and is undefined behavior.
🔧 Minimal fix
`#else`
// Cannot be constructed if wallet support isn't built
assert(false);
+ return nullptr;
`#endif` // ENABLE_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.
| #else | |
| // Cannot be constructed if wallet support isn't built | |
| return nullptr; | |
| assert(false); | |
| `#else` | |
| // Cannot be constructed if wallet support isn't built | |
| assert(false); | |
| return nullptr; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/walletman.cpp` around lines 318 - 320, The non-wallet factory
branch in src/coinjoin/walletman.cpp currently does assert(false) and then falls
off the end of a non-void function in release builds; update the non-wallet path
(the factory function in walletman.cpp that constructs/returns a wallet object
when wallet support is disabled) to return an explicit safe default (e.g.,
return nullptr) after the assert(false) so the function has a defined return
value in non-debug builds; ensure the returned value matches the function's
return type (use nullptr for pointer returns or an appropriate sentinel value)
and keep the assert for debug-time diagnostics.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b79f5a512e
ℹ️ 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 (std::unique_ptr<interfaces::CoinJoin::Client> coinjoin_client = coinjoin_available() ? coinjoin_loader().GetClient(GetName()) : nullptr) { | ||
| coinjoin_client->stopMixing(); | ||
| if (coinjoin_available()) { | ||
| coinjoin_loader().WithClient(GetName(), [](auto& client) { client.stopMixing(); }); |
There was a problem hiding this comment.
LegacyScriptPubKeyMan::NewKeyPool() invokes m_storage.NewKeyPoolCallback() while holding cs_KeyStore (see src/wallet/scriptpubkeyman.cpp around lines 1428-1439), and this callback now calls coinjoin_loader().WithClient(...), which acquires cs_wallet_manager_map. At the same time, coinjoin reset/flushWallet execute resetPool() under cs_wallet_manager_map, and resetPool() calls keyHolderStorage.ReturnAll() which reaches ReserveDestination::ReturnDestination() and takes cs_KeyStore (via src/coinjoin/client.cpp and src/wallet/scriptpubkeyman.cpp). This introduces a lock-order inversion (cs_KeyStore -> cs_wallet_manager_map vs cs_wallet_manager_map -> cs_KeyStore) that can deadlock during concurrent keypool refresh and CoinJoin reset/flush operations.
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: