refactor: simplify CoinJoin class hierarchy#7248
refactor: simplify CoinJoin class hierarchy#7248knst wants to merge 8 commits intodashpay:developfrom
Conversation
|
![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: d4cfc61d20
ℹ️ 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_ProcessDSQueue); | ||
|
|
||
| if (m_queueman->HasQueue(dsq.GetHash())) return ret; |
There was a problem hiding this comment.
This replaces the old q == dsq duplicate gate with HasQueue(dsq.GetHash()), but CCoinJoinQueue::GetHash() is based on serialized fields that exclude masternodeOutpoint (coinjoin.h serialization), while the previous equality check included the outpoint. In the specific case where incoming DSQUEUE messages have m_protxHash unset (which this function still accepts), two different masternodes broadcasting in the same second with the same denom/readiness can hash-collide and the second queue is dropped as a duplicate, reducing queue propagation and join opportunities.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@chatgpt-codex-connector, is it a new issue or is it moved code?
There was a problem hiding this comment.
To use Codex here, create a Codex account and connect to github.
WalkthroughThe PR replaces CCoinJoinBaseManager with a new CoinJoinQueueManager and converts queue management from inheritance to composition. Queue operations (add/check/try-lock APIs) are moved into CoinJoinQueueManager; server and client code now delegate queue checks and mutations to that member. DSQUEUE processing and queue routing were centralized in CJWalletManagerImpl, which validates DSQUEUE messages and either routes ready queues to client managers or enqueues them in CoinJoinQueueManager. Tests were removed for the old base manager and new unit tests added for CoinJoinQueueManager. sequenceDiagram
participant Peer as "Peer (network)"
participant WalletMgr as "CJWalletManagerImpl"
participant QueueMgr as "CoinJoinQueueManager"
participant ClientMgr as "CCoinJoinClientManager(s)"
participant Server as "CCoinJoinServer"
Peer->>WalletMgr: Send DSQUEUE
WalletMgr->>WalletMgr: Validate DSQUEUE (MN, sig, thresholds)
alt Ready
WalletMgr->>ClientMgr: TrySubmitDenominate (iterate clients)
ClientMgr-->>WalletMgr: Accept / Reject
else Not ready or rejected
WalletMgr->>QueueMgr: AddQueue(dsq) / TryAddQueue(dsq)
QueueMgr-->>WalletMgr: Enqueued / Busy
end
Note right of QueueMgr: Periodic maintenance
Server->>QueueMgr: CheckQueue() / GetQueueItemAndTry()
QueueMgr-->>Server: Provide eligible queue item / purge expired
Server->>ClientMgr: Promote queue item to session
ClientMgr-->>Server: Session actions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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/coinjoin/server.cpp (1)
90-97:⚠️ Potential issue | 🟡 MinorSilent bypass on lock contention may weaken anti-spam protection.
When
TryHasQueueFromMasternodereturnsstd::nulloptdue to lock contention (Line 91), the function returns early without rejecting the request. This allows a DSACCEPT through without verifying whether this masternode already has a queue, potentially bypassing the duplicate-queue protection.Consider whether this is the intended behavior. If the check is critical, a blocking
LOCKor retry mechanism may be more appropriate.🤖 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, The current early return when m_queueman.TryHasQueueFromMasternode(m_mn_activeman.GetOutPoint()) returns std::nullopt silently bypasses the duplicate-queue check; instead, handle lock contention explicitly: either acquire the appropriate lock around the call or implement a short retry loop (e.g., 3 attempts with small backoff) calling TryHasQueueFromMasternode again, and if it still returns std::nullopt log the contention and reject the request by calling PushStatus(peer, STATUS_REJECTED, ERR_RECENT) (and keep the existing log message path when *hasQueue is true). Ensure references to m_queueman, TryHasQueueFromMasternode, m_mn_activeman.GetOutPoint, PushStatus, STATUS_REJECTED, and ERR_RECENT are used so reviewers can locate the change.
🧹 Nitpick comments (3)
src/coinjoin/coinjoin.h (1)
341-346: Lambda captures by value unnecessarily.The lambda at Line 345 captures
qby value (auto q), which copies eachCCoinJoinQueueduring iteration. Use a const reference for efficiency:Suggested fix
bool HasQueue(const uint256& queueHash) EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue) { LOCK(cs_vecqueue); return std::any_of(vecCoinJoinQueue.begin(), vecCoinJoinQueue.end(), - [&queueHash](auto q) { return q.GetHash() == queueHash; }); + [&queueHash](const auto& q) { return q.GetHash() == queueHash; }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/coinjoin.h` around lines 341 - 346, The HasQueue method iterates vecCoinJoinQueue using a lambda that copies each CCoinJoinQueue (auto q), causing unnecessary copies; change the lambda parameter to take each element by const reference (e.g., const auto& q) so the comparison q.GetHash() == queueHash uses a reference and avoids copying. Update the lambda in std::any_of inside HasQueue accordingly.src/coinjoin/server.cpp (1)
193-194: Queue addition silently skipped on lock contention.When
TryAddQueuefails to acquire the lock, the queue is not added and the function returns without any logging. ThePeerRelayDSQat Line 194 is never reached, but there's no indication of why the queue wasn't added.Consider adding debug logging when the lock cannot be acquired:
Suggested logging
- if (!m_queueman.TryAddQueue(dsq)) return; + if (!m_queueman.TryAddQueue(dsq)) { + LogPrint(BCLog::COINJOIN, "DSQUEUE -- failed to add queue (lock contention), masternode=%s\n", dsq.masternodeOutpoint.ToStringShort()); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/server.cpp` around lines 193 - 194, Queue addition can silently fail when m_queueman.TryAddQueue(dsq) can't acquire the lock; add a debug/error log when TryAddQueue returns false so the failure is visible and easier to diagnose, then return early as before. Locate the call site where TryAddQueue(dsq) is invoked (near m_queueman.TryAddQueue and m_peer_manager->PeerRelayDSQ) and insert a logging statement that includes context (e.g., dsq identifier or peer info) before returning, ensuring you do not call PeerRelayDSQ when TryAddQueue fails.src/coinjoin/walletman.cpp (1)
264-269: Consider adding a log message for time-out-of-bounds rejection.Line 266 silently returns when
dsq.IsTimeOutOfBounds(), while other rejection paths either log or return misbehaving errors. A debug log would help operators diagnose queue processing issues.📝 Optional: Add logging for time bounds rejection
LogPrint(BCLog::COINJOIN, "DSQUEUE -- %s new\n", dsq.ToString()); - if (dsq.IsTimeOutOfBounds()) return ret; + if (dsq.IsTimeOutOfBounds()) { + LogPrint(BCLog::COINJOIN, "DSQUEUE -- time out of bounds, ignoring: %s\n", dsq.ToString()); + return ret; + } auto dmn = tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/walletman.cpp` around lines 264 - 269, The dsq.IsTimeOutOfBounds() branch silently returns without logging, making queue rejections hard to diagnose; update the block around the call to dsq.IsTimeOutOfBounds() (in the same scope where LogPrint(BCLog::COINJOIN, "DSQUEUE -- %s new\n", dsq.ToString()) and before tip_mn_list.GetValidMNByCollateral(...) is called) to emit a debug/log message (e.g., using LogPrint or the existing logging facility) indicating the DSQueue entry was rejected due to timeout bounds and include dsq.ToString() or relevant time details, then return ret as before so behavior is unchanged but now observable.
🤖 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`:
- Around line 159-160: Adjust formatting to satisfy clang-format: reformat the
block using m_queueman.TryCheckDuplicate(dsq) and the subsequent conditional so
LogPrint alignment matches project style (ensure consistent spacing and
indentation around the if (!isDup.has_value()) return;), then re-run
clang-format across src/coinjoin/server.cpp and fix spacing in the object where
pushKV is used (ensure spacing around commas/parentheses matches project
formatting rules) so both the LogPrint alignment and pushKV spacing conform to
CI; you can simply run the repository's clang-format or apply the project's
formatting rules to these occurrences.
- Around line 159-165: The early return when m_queueman.TryCheckDuplicate(dsq)
returns no value lets messages bypass anti-spam under lock contention and the
LogPrint message is misleading; update the DSQUEUE handling to treat a missing
optional (no lock acquired) as a rejection: log a clear warning that the
duplicate check failed due to lock contention (include
dsq.masternodeOutpoint.ToStringShort() and peer id `from`) and drop the message
instead of returning silently, and change the existing LogPrint that fires when
*isDup is true to explicitly state "duplicate dsq message" (referencing
m_queueman.TryCheckDuplicate, dsq, and LogPrint) so the two paths are
distinguishable.
In `@src/coinjoin/walletman.cpp`:
- Around line 181-186: flushWallet currently calls getClient() which releases
cs_wallet_manager_map before returning a raw pointer, creating a
TOCTOU/use-after-free if removeWallet() runs concurrently; modify
CJWalletManagerImpl::flushWallet to acquire the cs_wallet_manager_map mutex and
either (a) perform ResetPool() and StopMixing() while holding that lock, or (b)
copy/move the wallet's unique_ptr (from the map entry obtained under the lock)
into a local smart pointer so the wallet cannot be destroyed after the lock is
released, then call clientman->ResetPool() and clientman->StopMixing();
reference symbols: CJWalletManagerImpl::flushWallet, getClient, removeWallet,
cs_wallet_manager_map, and the wallet unique_ptr stored in the manager map.
---
Outside diff comments:
In `@src/coinjoin/server.cpp`:
- Around line 90-97: The current early return when
m_queueman.TryHasQueueFromMasternode(m_mn_activeman.GetOutPoint()) returns
std::nullopt silently bypasses the duplicate-queue check; instead, handle lock
contention explicitly: either acquire the appropriate lock around the call or
implement a short retry loop (e.g., 3 attempts with small backoff) calling
TryHasQueueFromMasternode again, and if it still returns std::nullopt log the
contention and reject the request by calling PushStatus(peer, STATUS_REJECTED,
ERR_RECENT) (and keep the existing log message path when *hasQueue is true).
Ensure references to m_queueman, TryHasQueueFromMasternode,
m_mn_activeman.GetOutPoint, PushStatus, STATUS_REJECTED, and ERR_RECENT are used
so reviewers can locate the change.
---
Nitpick comments:
In `@src/coinjoin/coinjoin.h`:
- Around line 341-346: The HasQueue method iterates vecCoinJoinQueue using a
lambda that copies each CCoinJoinQueue (auto q), causing unnecessary copies;
change the lambda parameter to take each element by const reference (e.g., const
auto& q) so the comparison q.GetHash() == queueHash uses a reference and avoids
copying. Update the lambda in std::any_of inside HasQueue accordingly.
In `@src/coinjoin/server.cpp`:
- Around line 193-194: Queue addition can silently fail when
m_queueman.TryAddQueue(dsq) can't acquire the lock; add a debug/error log when
TryAddQueue returns false so the failure is visible and easier to diagnose, then
return early as before. Locate the call site where TryAddQueue(dsq) is invoked
(near m_queueman.TryAddQueue and m_peer_manager->PeerRelayDSQ) and insert a
logging statement that includes context (e.g., dsq identifier or peer info)
before returning, ensuring you do not call PeerRelayDSQ when TryAddQueue fails.
In `@src/coinjoin/walletman.cpp`:
- Around line 264-269: The dsq.IsTimeOutOfBounds() branch silently returns
without logging, making queue rejections hard to diagnose; update the block
around the call to dsq.IsTimeOutOfBounds() (in the same scope where
LogPrint(BCLog::COINJOIN, "DSQUEUE -- %s new\n", dsq.ToString()) and before
tip_mn_list.GetValidMNByCollateral(...) is called) to emit a debug/log message
(e.g., using LogPrint or the existing logging facility) indicating the DSQueue
entry was rejected due to timeout bounds and include dsq.ToString() or relevant
time details, then return ret as before so behavior is unchanged but now
observable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eb6ca1b7-3151-4797-966b-a6a0cea6e276
📒 Files selected for processing (11)
contrib/auto_gdb/simple_class_obj.pysrc/Makefile.test.includesrc/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/server.cppsrc/coinjoin/server.hsrc/coinjoin/walletman.cppsrc/test/coinjoin_basemanager_tests.cppsrc/test/coinjoin_queue_tests.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; |
There was a problem hiding this comment.
Fix formatting to pass CI.
The pipeline reports clang-format failures at lines 159-160 (LogPrint alignment) and line 904 (pushKV spacing). Run the formatter to resolve these.
Also applies to: 904-904
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 159-160: Clang format check failed (clang-format-diff.py). Formatting differences found in server.cpp: LogPrint arguments were rewrapped/aligned.
🤖 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 - 160, Adjust formatting to satisfy
clang-format: reformat the block using m_queueman.TryCheckDuplicate(dsq) and the
subsequent conditional so LogPrint alignment matches project style (ensure
consistent spacing and indentation around the if (!isDup.has_value()) return;),
then re-run clang-format across src/coinjoin/server.cpp and fix spacing in the
object where pushKV is used (ensure spacing around commas/parentheses matches
project formatting rules) so both the LogPrint alignment and pushKV spacing
conform to CI; you can simply run the repository's clang-format or apply the
project's formatting rules to these occurrences.
| void CJWalletManagerImpl::flushWallet(const std::string& name) | ||
| { | ||
| walletman.Flush(name); | ||
| auto* clientman = Assert(getClient(name)); | ||
| clientman->ResetPool(); | ||
| clientman->StopMixing(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if flushWallet callers could be concurrent with removeWallet
rg -n -C3 'flushWallet|removeWallet' --type cppRepository: dashpay/dash
Length of output: 6249
🏁 Script executed:
# First, find and view the getClient() implementation
rg -n 'getClient\(' src/coinjoin/walletman.cpp -A 10 | head -50Repository: dashpay/dash
Length of output: 1549
🏁 Script executed:
# Check the context in interfaces.cpp where flushWallet is called
sed -n '100,115p' src/coinjoin/interfaces.cppRepository: dashpay/dash
Length of output: 703
🏁 Script executed:
# Find all callers of flushWallet to understand threading context
rg -n 'flushWallet' --type cpp -B 3 -A 3Repository: dashpay/dash
Length of output: 1985
🏁 Script executed:
# Find callers of FlushWallet interface method
rg -n 'FlushWallet\(' --type cpp -B 3 -A 3Repository: dashpay/dash
Length of output: 1482
🏁 Script executed:
# Check wallet loading/unloading patterns to see if concurrent calls are possible
rg -n 'wallet.*Flush|wallet.*Remove' --type cpp | head -30Repository: dashpay/dash
Length of output: 1110
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.
Proposed fix
void CJWalletManagerImpl::flushWallet(const std::string& name)
{
- auto* clientman = Assert(getClient(name));
- clientman->ResetPool();
- clientman->StopMixing();
+ LOCK(cs_wallet_manager_map);
+ auto it = m_wallet_manager_map.find(name);
+ if (it != m_wallet_manager_map.end()) {
+ it->second->ResetPool();
+ it->second->StopMixing();
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/walletman.cpp` around lines 181 - 186, flushWallet currently
calls getClient() which releases cs_wallet_manager_map before returning a raw
pointer, creating a TOCTOU/use-after-free if removeWallet() runs concurrently;
modify CJWalletManagerImpl::flushWallet to acquire the cs_wallet_manager_map
mutex and either (a) perform ResetPool() and StopMixing() while holding that
lock, or (b) copy/move the wallet's unique_ptr (from the map entry obtained
under the lock) into a local smart pointer so the wallet cannot be destroyed
after the lock is released, then call clientman->ResetPool() and
clientman->StopMixing(); reference symbols: CJWalletManagerImpl::flushWallet,
getClient, removeWallet, cs_wallet_manager_map, and the wallet unique_ptr stored
in the manager map.
There was a problem hiding this comment.
it's existing issue; shared_ptr should be used. I will prepare new PR with this fix.
There was a problem hiding this comment.
@knst, understood — thanks for the confirmation! Using shared_ptr (or an RAII scope-guard over cs_wallet_manager_map) would be a clean fix for this.
Would you like me to open a GitHub issue to track this follow-up so it doesn't get lost?
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean mechanical refactoring. No blocking issues found. There is one genuine performance bug (copy-by-value in a hot-path lambda) and a const-correctness inconsistency. Two nitpicks about log quality and test coverage round out the review.
Reviewed commit: d4cfc61
🟡 2 suggestion(s) | 💬 2 nitpick(s)
1 additional finding
🟡 suggestion: HasQueue lambda copies each CCoinJoinQueue by value
src/coinjoin/coinjoin.h (lines 344-345)
[&queueHash](auto q) copies every CCoinJoinQueue per iteration, including its heap-allocated std::vector<unsigned char> vchSig. This is called on every incoming DSQUEUE message (via ProcessDSQueue) and for inventory checks (AlreadyHave). The adjacent GetQueueFromHash (line 350) and HasQueueFromMasternode (line 358) both correctly use const auto& q—this one should too.
💡 Suggested change
return std::any_of(vecCoinJoinQueue.begin(), vecCoinJoinQueue.end(),
[&queueHash](const auto& q) { return q.GetHash() == queueHash; });
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/coinjoin.h`:
- [SUGGESTION] lines 344-345: HasQueue lambda copies each CCoinJoinQueue by value
`[&queueHash](auto q)` copies every `CCoinJoinQueue` per iteration, including its heap-allocated `std::vector<unsigned char> vchSig`. This is called on every incoming `DSQUEUE` message (via `ProcessDSQueue`) and for inventory checks (`AlreadyHave`). The adjacent `GetQueueFromHash` (line 350) and `HasQueueFromMasternode` (line 358) both correctly use `const auto& q`—this one should too.
- [SUGGESTION] lines 341-351: HasQueue and GetQueueFromHash should be const
Both methods only read `vecCoinJoinQueue` under `LOCK(cs_vecqueue)`, and `cs_vecqueue` is already `mutable Mutex` (line 327). `GetQueueSize()` (line 338) and `HasQueueFromMasternode()` (line 355) are already `const`. The inconsistency matters because `CJWalletManagerImpl::hasQueue` is a `const` method that calls `m_queueman->HasQueue()`—this only compiles because `std::unique_ptr::operator->()` doesn't propagate const, silently bypassing const-correctness.
| LOCK(cs_vecqueue); | ||
| return util::find_if_opt(vecCoinJoinQueue, [&queueHash](const auto& q) { return q.GetHash() == queueHash; }); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: HasQueue and GetQueueFromHash should be const
Both methods only read vecCoinJoinQueue under LOCK(cs_vecqueue), and cs_vecqueue is already mutable Mutex (line 327). GetQueueSize() (line 338) and HasQueueFromMasternode() (line 355) are already const. The inconsistency matters because CJWalletManagerImpl::hasQueue is a const method that calls m_queueman->HasQueue()—this only compiles because std::unique_ptr::operator->() doesn't propagate const, silently bypassing const-correctness.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/coinjoin.h`:
- [SUGGESTION] lines 341-351: HasQueue and GetQueueFromHash should be const
Both methods only read `vecCoinJoinQueue` under `LOCK(cs_vecqueue)`, and `cs_vecqueue` is already `mutable Mutex` (line 327). `GetQueueSize()` (line 338) and `HasQueueFromMasternode()` (line 355) are already `const`. The inconsistency matters because `CJWalletManagerImpl::hasQueue` is a `const` method that calls `m_queueman->HasQueue()`—this only compiles because `std::unique_ptr::operator->()` doesn't propagate const, silently bypassing const-correctness.
| 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; |
There was a problem hiding this comment.
💬 Nitpick: TryCheckDuplicate conflates exact duplicates with rate abuse in the log
TryCheckDuplicate returns true for both exact duplicates (q == dsq) and same-MN-same-readiness matches. The caller logs "WAY too many dsq messages" for both cases. Exact duplicates are benign repeated messages (P2P retransmission), not rate abuse. The log is misleading for that case. Consider either splitting the return value or accepting the minor log noise with a comment.
source: ['claude']
| BOOST_AUTO_TEST_CASE(queuemanager_checkqueue_removes_timeouts) | ||
| { | ||
| CoinJoinQueueManager man; | ||
| const int denom = CoinJoin::AmountToDenomination(CoinJoin::GetSmallestDenomination()); | ||
| const int64_t now = GetAdjustedTime(); | ||
| // Non-expired | ||
| man.AddQueue(MakeQueue(denom, now, false, COutPoint(uint256S("11"), 0))); | ||
| // Expired (too old) | ||
| man.AddQueue(MakeQueue(denom, now - COINJOIN_QUEUE_TIMEOUT - 1, false, COutPoint(uint256S("12"), 0))); | ||
|
|
||
| BOOST_CHECK_EQUAL(man.GetQueueSize(), 2); | ||
| man.CheckQueue(); | ||
| // One should be removed | ||
| BOOST_CHECK_EQUAL(man.GetQueueSize(), 1); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(queuemanager_getqueueitem_marks_tried_once) | ||
| { | ||
| CoinJoinQueueManager man; | ||
| const int denom = CoinJoin::AmountToDenomination(CoinJoin::GetSmallestDenomination()); | ||
| const int64_t now = GetAdjustedTime(); | ||
| CCoinJoinQueue dsq = MakeQueue(denom, now, false, COutPoint(uint256S("21"), 0)); | ||
| man.AddQueue(dsq); | ||
|
|
||
| CCoinJoinQueue picked; | ||
| // First retrieval should succeed | ||
| BOOST_CHECK(man.GetQueueItemAndTry(picked)); | ||
| // No other items left to try (picked is marked tried inside) | ||
| CCoinJoinQueue picked2; | ||
| BOOST_CHECK(!man.GetQueueItemAndTry(picked2)); | ||
| } | ||
|
|
There was a problem hiding this comment.
💬 Nitpick: TryCheckDuplicate lacks a dedicated test
The migrated tests cover AddQueue, CheckQueue, and GetQueueItemAndTry. TryCheckDuplicate encodes non-trivial duplicate detection logic (exact match vs. same-MN-same-readiness) that would benefit from a targeted test. The other Try* methods are thin TRY_LOCK wrappers and lower priority.
source: ['claude']
|
This pull request has conflicts, please rebase. |
… 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
knst
force-pushed
the
refactor-simplify-cj-hierarchy
branch
from
d4cfc61 to
267e340
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/coinjoin/coinjoin.h (1)
341-351: 🛠️ Refactor suggestion | 🟠 Major
HasQueueandGetQueueFromHashshould beconst.Both methods only read
vecCoinJoinQueueunderLOCK(cs_vecqueue), andcs_vecqueueis alreadymutable Mutex.GetQueueSize()(line 338) andHasQueueFromMasternode()(line 355) are alreadyconst. The inconsistency matters becauseCJWalletManagerImpl::hasQueueis aconstmethod that callsm_queueman->HasQueue()—this compiles only becausestd::unique_ptr::operator->()doesn't propagate const.Suggested fix
- bool HasQueue(const uint256& queueHash) EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue) + bool HasQueue(const uint256& queueHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue) { LOCK(cs_vecqueue); return std::any_of(vecCoinJoinQueue.begin(), vecCoinJoinQueue.end(), [&queueHash](auto q) { return q.GetHash() == queueHash; }); } - std::optional<CCoinJoinQueue> GetQueueFromHash(const uint256& queueHash) EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue) + std::optional<CCoinJoinQueue> GetQueueFromHash(const uint256& queueHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue) { LOCK(cs_vecqueue); return util::find_if_opt(vecCoinJoinQueue, [&queueHash](const auto& q) { return q.GetHash() == queueHash; }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/coinjoin.h` around lines 341 - 351, Change the signatures of HasQueue and GetQueueFromHash to be const (append "const" after the EXCLUSIVE_LOCKS_REQUIRED annotation) so they can be called from const callers; in HasQueue also avoid copying queue entries by changing the lambda to take const auto& q (instead of auto q) so the comparison q.GetHash() works on const references. Update the declarations/definitions of HasQueue and GetQueueFromHash accordingly (functions: HasQueue, GetQueueFromHash).src/coinjoin/walletman.cpp (1)
181-186:⚠️ Potential issue | 🟠 MajorHold lock during flush operations to prevent TOCTOU race condition.
getClient()acquirescs_wallet_manager_mapbut releases it before returning the raw pointer. If another thread callsremoveWallet()concurrently, theunique_ptris destroyed whileflushWallet()still holds a reference, leading to use-after-free.Proposed fix
void CJWalletManagerImpl::flushWallet(const std::string& name) { - auto* clientman = Assert(getClient(name)); - clientman->ResetPool(); - clientman->StopMixing(); + LOCK(cs_wallet_manager_map); + auto it = m_wallet_manager_map.find(name); + if (it != m_wallet_manager_map.end()) { + it->second->ResetPool(); + it->second->StopMixing(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/walletman.cpp` around lines 181 - 186, flushWallet currently calls getClient() which drops cs_wallet_manager_map before using the returned raw pointer, risking use-after-free if removeWallet() runs concurrently; modify CJWalletManagerImpl::flushWallet to acquire and hold the cs_wallet_manager_map lock (e.g., std::unique_lock or LOCK(cs_wallet_manager_map)) for the duration of the operation, then call Assert(getClient(name)) and invoke clientman->ResetPool() and clientman->StopMixing() while the lock is held so the underlying unique_ptr cannot be destroyed by removeWallet(); keep references to the same symbols (CJWalletManagerImpl::flushWallet, getClient, removeWallet, cs_wallet_manager_map, ResetPool, StopMixing).
🧹 Nitpick comments (1)
src/coinjoin/walletman.cpp (1)
112-118: Consider simplifying the destructor.The destructor explicitly resets each
unique_ptrunder lock, but simply clearing the map would achieve the same effect more idiomatically:Suggested simplification
CJWalletManagerImpl::~CJWalletManagerImpl() { LOCK(cs_wallet_manager_map); - for (auto& [_, clientman] : m_wallet_manager_map) { - clientman.reset(); - } + m_wallet_manager_map.clear(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/walletman.cpp` around lines 112 - 118, The destructor CJWalletManagerImpl::~CJWalletManagerImpl currently locks cs_wallet_manager_map and manually resets each unique_ptr in m_wallet_manager_map; simplify it by keeping the LOCK(cs_wallet_manager_map) but replace the for-loop with m_wallet_manager_map.clear() to idiomatically destroy all entries (unique_ptrs) and release resources in one call, preserving thread-safety.
🤖 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/coinjoin.h`:
- Around line 360-366: Run clang-format (or adjust formatting) on the
declarations of TryHasQueueFromMasternode and TryCheckDuplicate so the trailing
attribute EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue) is on its own line / matches
the project's clang-format rules; locate the declarations for the functions
TryHasQueueFromMasternode(const COutPoint& outpoint) and TryCheckDuplicate(const
CCoinJoinQueue& dsq) in coinjoin.h and reflow the line breaks/spacing to conform
to the project's style (or run the project's clang-format command to auto-fix).
In `@src/coinjoin/walletman.cpp`:
- Around line 41-54: Several function declarations (e.g., getClient,
processMessage, getMixingMasternodes, addWallet, removeWallet, and
UpdatedBlockTip) have incorrect line breaks before the EXCLUSIVE_LOCKS_REQUIRED
annotations; run clang-format on src/coinjoin/walletman.cpp to reflow these
declarations so the EXCLUSIVE_LOCKS_REQUIRED(...) annotation is on the same line
as the function declaration (also fix the similar mismatches around the
declarations at the block noted as lines 75-78).
---
Duplicate comments:
In `@src/coinjoin/coinjoin.h`:
- Around line 341-351: Change the signatures of HasQueue and GetQueueFromHash to
be const (append "const" after the EXCLUSIVE_LOCKS_REQUIRED annotation) so they
can be called from const callers; in HasQueue also avoid copying queue entries
by changing the lambda to take const auto& q (instead of auto q) so the
comparison q.GetHash() works on const references. Update the
declarations/definitions of HasQueue and GetQueueFromHash accordingly
(functions: HasQueue, GetQueueFromHash).
In `@src/coinjoin/walletman.cpp`:
- Around line 181-186: flushWallet currently calls getClient() which drops
cs_wallet_manager_map before using the returned raw pointer, risking
use-after-free if removeWallet() runs concurrently; modify
CJWalletManagerImpl::flushWallet to acquire and hold the cs_wallet_manager_map
lock (e.g., std::unique_lock or LOCK(cs_wallet_manager_map)) for the duration of
the operation, then call Assert(getClient(name)) and invoke
clientman->ResetPool() and clientman->StopMixing() while the lock is held so the
underlying unique_ptr cannot be destroyed by removeWallet(); keep references to
the same symbols (CJWalletManagerImpl::flushWallet, getClient, removeWallet,
cs_wallet_manager_map, ResetPool, StopMixing).
---
Nitpick comments:
In `@src/coinjoin/walletman.cpp`:
- Around line 112-118: The destructor CJWalletManagerImpl::~CJWalletManagerImpl
currently locks cs_wallet_manager_map and manually resets each unique_ptr in
m_wallet_manager_map; simplify it by keeping the LOCK(cs_wallet_manager_map) but
replace the for-loop with m_wallet_manager_map.clear() to idiomatically destroy
all entries (unique_ptrs) and release resources in one call, preserving
thread-safety.
🪄 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: 57057b69-cf5e-4597-b88c-8b009c337f25
📒 Files selected for processing (11)
contrib/auto_gdb/simple_class_obj.pysrc/Makefile.test.includesrc/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/server.cppsrc/coinjoin/server.hsrc/coinjoin/walletman.cppsrc/test/coinjoin_basemanager_tests.cppsrc/test/coinjoin_queue_tests.cpp
💤 Files with no reviewable changes (2)
- src/Makefile.test.include
- src/test/coinjoin_basemanager_tests.cpp
✅ Files skipped from review due to trivial changes (1)
- src/coinjoin/server.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- contrib/auto_gdb/simple_class_obj.py
- src/coinjoin/server.h
- src/coinjoin/client.cpp
- src/coinjoin/coinjoin.cpp
| } | ||
| //! 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); |
There was a problem hiding this comment.
Address clang-format pipeline failures.
The pipeline reports formatting mismatches on the TryHasQueueFromMasternode and TryCheckDuplicate declarations. Run clang-format to fix line breaks before EXCLUSIVE_LOCKS_REQUIRED.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 360-362: Clang-format check mismatch: TryHasQueueFromMasternode declaration formatting (line break added before EXCLUSIVE_LOCKS_REQUIRED).
[error] 364-366: Clang-format check mismatch: TryCheckDuplicate declaration formatting (line break added before EXCLUSIVE_LOCKS_REQUIRED).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/coinjoin.h` around lines 360 - 366, Run clang-format (or adjust
formatting) on the declarations of TryHasQueueFromMasternode and
TryCheckDuplicate so the trailing attribute
EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue) is on its own line / matches the
project's clang-format rules; locate the declarations for the functions
TryHasQueueFromMasternode(const COutPoint& outpoint) and TryCheckDuplicate(const
CCoinJoinQueue& dsq) in coinjoin.h and reflow the line breaks/spacing to conform
to the project's style (or run the project's clang-format command to auto-fix).
267e340 to
c363fff
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/coinjoin/walletman.cpp (2)
42-50:⚠️ Potential issue | 🟡 MinorRe-run
clang-formaton the updated declarations.The diff-format job is still failing on the
EXCLUSIVE_LOCKS_REQUIRED(...)wrapping in this block and on theProcessDSQueuesignature, so CI will stay red as-is.Also applies to: 54-54, 75-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/walletman.cpp` around lines 42 - 50, Re-run clang-format on src/coinjoin/walletman.cpp to fix the wrapping/spacing of the thread-safety annotations and the ProcessDSQueue signature: ensure EXCLUSIVE_LOCKS_REQUIRED(...) stays attached to the function declaration (not split awkwardly onto its own line) for getClient, processMessage, getMixingMasternodes, addWallet, removeWallet, flushWallet and that the ProcessDSQueue/processMessage signature formatting matches project style; run the repository clang-format configuration (or the same .clang-format used by CI) and re-commit the formatted changes so the diff-format job passes.
143-147:⚠️ Potential issue | 🔴 Critical
flushWallet()still has a use-after-free race.
getClient()releasescs_wallet_manager_mapbefore returning the raw pointer, andflushWallet()dereferences it later. A concurrentremoveWallet()can erase thatunique_ptrin between, soResetPool()/StopMixing()can run on freed memory.Suggested fix
void CJWalletManagerImpl::flushWallet(const std::string& name) { - auto* clientman = Assert(getClient(name)); - clientman->ResetPool(); - clientman->StopMixing(); + LOCK(cs_wallet_manager_map); + if (auto it = m_wallet_manager_map.find(name); it != m_wallet_manager_map.end()) { + it->second->ResetPool(); + it->second->StopMixing(); + } }If either callee can re-enter
CJWalletManagerImpl, use an ownership-bearing handle instead of a borrowed raw pointer.Also applies to: 181-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/walletman.cpp` around lines 143 - 147, getClient currently returns a raw pointer to a CCoinJoinClientManager after releasing cs_wallet_manager_map, which allows removeWallet to erase the unique_ptr and cause use-after-free when flushWallet calls ResetPool/StopMixing; change the ownership model so callers get an ownership-bearing handle: replace stored unique_ptrs in m_wallet_manager_map with shared_ptrs (or return a std::shared_ptr created from the existing unique_ptr) and change getClient to return std::shared_ptr<CCoinJoinClientManager> (or an explicit owner handle) so flushWallet (and any callers of ResetPool/StopMixing) hold a reference that prevents removal races, and update removeWallet/CJWalletManagerImpl methods to use/erase the shared_ptr accordingly while keeping cs_wallet_manager_map locked only for map access.
🤖 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 254-273: Resolve the masternode identity before hashing the
DSQueue: call tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint) first,
reject if no dmn, then if dsq.m_protxHash is non-null and does not match
dmn->proTxHash return ret, otherwise set dsq.m_protxHash = dmn->proTxHash when
null; only after that run HasQueue(dsq.GetHash()) and
HasQueueFromMasternode(dsq.masternodeOutpoint, dsq.fReady) (and later
AddQueue(dsq)), so the hash-based duplicate fast-path uses the
normalized/validated proTxHash.
- Around line 120-124: The Schedule method currently skips scheduling
maintenance when m_relay_txes is false, which prevents
CJWalletManagerImpl::DoMaintenance from running for wallets in that mode; remove
the early return that checks m_relay_txes so scheduler.scheduleEvery is always
called, relying on DoMaintenance's internal check (and
CCoinJoinClientManager::DoMaintenance behavior) to no-op when m_queueman ==
nullptr; update CJWalletManagerImpl::Schedule to always bind and schedule
DoMaintenance with the connman reference.
---
Duplicate comments:
In `@src/coinjoin/walletman.cpp`:
- Around line 42-50: Re-run clang-format on src/coinjoin/walletman.cpp to fix
the wrapping/spacing of the thread-safety annotations and the ProcessDSQueue
signature: ensure EXCLUSIVE_LOCKS_REQUIRED(...) stays attached to the function
declaration (not split awkwardly onto its own line) for getClient,
processMessage, getMixingMasternodes, addWallet, removeWallet, flushWallet and
that the ProcessDSQueue/processMessage signature formatting matches project
style; run the repository clang-format configuration (or the same .clang-format
used by CI) and re-commit the formatted changes so the diff-format job passes.
- Around line 143-147: getClient currently returns a raw pointer to a
CCoinJoinClientManager after releasing cs_wallet_manager_map, which allows
removeWallet to erase the unique_ptr and cause use-after-free when flushWallet
calls ResetPool/StopMixing; change the ownership model so callers get an
ownership-bearing handle: replace stored unique_ptrs in m_wallet_manager_map
with shared_ptrs (or return a std::shared_ptr created from the existing
unique_ptr) and change getClient to return
std::shared_ptr<CCoinJoinClientManager> (or an explicit owner handle) so
flushWallet (and any callers of ResetPool/StopMixing) hold a reference that
prevents removal races, and update removeWallet/CJWalletManagerImpl methods to
use/erase the shared_ptr accordingly while keeping cs_wallet_manager_map locked
only for map access.
🪄 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: 381c0ef8-1cbb-45fa-b92e-aed055d1909a
📒 Files selected for processing (1)
src/coinjoin/walletman.cpp
| void CJWalletManagerImpl::Schedule(CConnman& connman, CScheduler& scheduler) | ||
| { | ||
| if (!m_relay_txes) return; | ||
| scheduler.scheduleEvery(std::bind(&CCoinJoinClientQueueManager::DoMaintenance, std::ref(*queueman)), | ||
| std::chrono::seconds{1}); | ||
| scheduler.scheduleEvery(std::bind(&CoinJoinWalletManager::DoMaintenance, std::ref(walletman), std::ref(connman)), | ||
| scheduler.scheduleEvery(std::bind(&CJWalletManagerImpl::DoMaintenance, this, std::ref(connman)), | ||
| std::chrono::seconds{1}); |
There was a problem hiding this comment.
Don't gate wallet maintenance on relay_txes.
DoMaintenance() already skips queue work when m_queueman == nullptr. This early return only disables CCoinJoinClientManager::DoMaintenance(...) for the relay_txes == false path, so wallets added in that mode never get periodic maintenance.
Suggested fix
void CJWalletManagerImpl::Schedule(CConnman& connman, CScheduler& scheduler)
{
- if (!m_relay_txes) return;
scheduler.scheduleEvery(std::bind(&CJWalletManagerImpl::DoMaintenance, this, std::ref(connman)),
std::chrono::seconds{1});
}📝 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 CJWalletManagerImpl::Schedule(CConnman& connman, CScheduler& scheduler) | |
| { | |
| if (!m_relay_txes) return; | |
| scheduler.scheduleEvery(std::bind(&CCoinJoinClientQueueManager::DoMaintenance, std::ref(*queueman)), | |
| std::chrono::seconds{1}); | |
| scheduler.scheduleEvery(std::bind(&CoinJoinWalletManager::DoMaintenance, std::ref(walletman), std::ref(connman)), | |
| scheduler.scheduleEvery(std::bind(&CJWalletManagerImpl::DoMaintenance, this, std::ref(connman)), | |
| std::chrono::seconds{1}); | |
| void CJWalletManagerImpl::Schedule(CConnman& connman, CScheduler& scheduler) | |
| { | |
| scheduler.scheduleEvery(std::bind(&CJWalletManagerImpl::DoMaintenance, this, std::ref(connman)), | |
| std::chrono::seconds{1}); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/walletman.cpp` around lines 120 - 124, The Schedule method
currently skips scheduling maintenance when m_relay_txes is false, which
prevents CJWalletManagerImpl::DoMaintenance from running for wallets in that
mode; remove the early return that checks m_relay_txes so
scheduler.scheduleEvery is always called, relying on DoMaintenance's internal
check (and CCoinJoinClientManager::DoMaintenance behavior) to no-op when
m_queueman == nullptr; update CJWalletManagerImpl::Schedule to always bind and
schedule DoMaintenance with the connman reference.
| if (m_queueman->HasQueue(dsq.GetHash())) return ret; | ||
|
|
||
| if (m_queueman->HasQueueFromMasternode(dsq.masternodeOutpoint, dsq.fReady)) { | ||
| // no way the same mn can send another dsq with the same readiness this soon | ||
| LogPrint(BCLog::COINJOIN, /* Continued */ | ||
| "DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with collateral %s\n", | ||
| from, dsq.masternodeOutpoint.ToStringShort()); | ||
| return ret; | ||
| } | ||
|
|
||
| LogPrint(BCLog::COINJOIN, "DSQUEUE -- %s new\n", dsq.ToString()); | ||
|
|
||
| if (dsq.IsTimeOutOfBounds()) return ret; | ||
|
|
||
| auto dmn = tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint); | ||
| if (!dmn) return ret; | ||
|
|
||
| if (dsq.m_protxHash.IsNull()) { | ||
| dsq.m_protxHash = dmn->proTxHash; | ||
| } |
There was a problem hiding this comment.
Normalize and validate the masternode identity before HasQueue().
Here HasQueue(dsq.GetHash()) hashes the pre-normalized object, but Line 271 can still mutate dsq.m_protxHash before AddQueue(dsq). Repeated outpoint-only DSQUEUEs then miss the duplicate fast-path and fall into the "WAY too many dsq messages" branch instead; a non-null but mismatched m_protxHash is also accepted today.
Resolve dmn first, reject conflicting non-null m_protxHash, fill the null case, and only then run the hash-based duplicate check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/walletman.cpp` around lines 254 - 273, Resolve the masternode
identity before hashing the DSQueue: call
tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint) first, reject if no
dmn, then if dsq.m_protxHash is non-null and does not match dmn->proTxHash
return ret, otherwise set dsq.m_protxHash = dmn->proTxHash when null; only after
that run HasQueue(dsq.GetHash()) and
HasQueueFromMasternode(dsq.masternodeOutpoint, dsq.fReady) (and later
AddQueue(dsq)), so the hash-based duplicate fast-path uses the
normalized/validated proTxHash.
c363fff to
eb2142d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/coinjoin/walletman.cpp (2)
251-273:⚠️ Potential issue | 🟠 MajorNormalize
m_protxHashbefore duplicate detection to prevent bypass.
HasQueue(dsq.GetHash())at Line 254 computes the hash beforedsq.m_protxHashis filled at Line 271-273. If a DSQUEUE arrives with a nullm_protxHash, the hash used for duplicate detection differs from the hash of the normalized object added viaAddQueue(). This allows bypassing the duplicate check.Resolve the masternode (
dmn) and normalizem_protxHashbefore the hash-based duplicate checks:Proposed fix
{ LOCK(cs_ProcessDSQueue); + auto dmn = tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint); + if (!dmn) return ret; + + if (dsq.m_protxHash.IsNull()) { + dsq.m_protxHash = dmn->proTxHash; + } else if (dsq.m_protxHash != dmn->proTxHash) { + // Reject mismatched proTxHash + ret.m_error = MisbehavingError{10}; + return ret; + } + if (m_queueman->HasQueue(dsq.GetHash())) return ret; if (m_queueman->HasQueueFromMasternode(dsq.masternodeOutpoint, dsq.fReady)) { // no way the same mn can send another dsq with the same readiness this soon LogPrint(BCLog::COINJOIN, /* Continued */ "DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with collateral %s\n", from, dsq.masternodeOutpoint.ToStringShort()); return ret; } LogPrint(BCLog::COINJOIN, "DSQUEUE -- %s new\n", dsq.ToString()); if (dsq.IsTimeOutOfBounds()) return ret; - auto dmn = tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint); - if (!dmn) return ret; - - if (dsq.m_protxHash.IsNull()) { - dsq.m_protxHash = dmn->proTxHash; - } - if (!dsq.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/walletman.cpp` around lines 251 - 273, Move masternode resolution and normalization of dsq.m_protxHash before the duplicate/hash checks: call tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint) early, and if it returns a valid dmn and dsq.m_protxHash.IsNull() set dsq.m_protxHash = dmn->proTxHash, then perform m_queueman->HasQueue(dsq.GetHash()) and m_queueman->HasQueueFromMasternode(...) as before; this ensures the hash used by HasQueue/HasQueueFromMasternode matches the normalized object later added via AddQueue().
181-186:⚠️ Potential issue | 🔴 CriticalTOCTOU race condition still present in
flushWallet().
getClient()releasescs_wallet_manager_mapbefore returning. IfremoveWallet()is called concurrently, theunique_ptris destroyed whileflushWallet()still holds a dangling pointer, causing use-after-free. This issue was flagged previously and remains unaddressed.Proposed fix
void CJWalletManagerImpl::flushWallet(const std::string& name) { - auto* clientman = Assert(getClient(name)); - clientman->ResetPool(); - clientman->StopMixing(); + LOCK(cs_wallet_manager_map); + auto it = m_wallet_manager_map.find(name); + if (it != m_wallet_manager_map.end()) { + it->second->ResetPool(); + it->second->StopMixing(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/walletman.cpp` around lines 181 - 186, flushWallet() currently calls getClient() which releases cs_wallet_manager_map before returning, leaving clientman as a raw pointer and allowing removeWallet() to destroy the unique_ptr concurrently; change the code so flushWallet() obtains a safe reference while holding the wallet-manager map lock or uses a shared ownership type: either (A) modify getClient()/CJWalletManagerImpl to return a std::shared_ptr to the client manager (instead of a raw pointer) and use that shared_ptr in flushWallet(), or (B) lock cs_wallet_manager_map inside flushWallet(), copy or move a std::shared_ptr/borrowed handle to clientman while locked, then release the lock and call ResetPool()/StopMixing() on the stable handle; ensure removeWallet() still erases only after dropping its shared_ptr so no use-after-free occurs (refer to symbols: flushWallet, getClient, removeWallet, cs_wallet_manager_map, CJWalletManagerImpl, clientman).
🤖 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 43-44: The EXCLUSIVE_LOCKS_REQUIRED attribute is misformatted on
multi-line declarations (e.g., the processMessage method) causing clang-format
pipeline failures; run clang-format and adjust the declaration formatting so the
EXCLUSIVE_LOCKS_REQUIRED annotation is placed per project style (move it to the
correct position/line for multi-line function declarations or combine it with
the closing parenthesis as clang-format expects) for processMessage and the
other similar multi-line declarations (the other EXCLUSIVE_LOCKS_REQUIRED
occurrence around the same area) so the linting passes.
- Around line 120-125: Remove the early return in CJWalletManagerImpl::Schedule
that checks m_relay_txes so the scheduler always registers DoMaintenance;
specifically, delete the guard "if (!m_relay_txes) return;" in
CJWalletManagerImpl::Schedule and always call
scheduler.scheduleEvery(std::bind(&CJWalletManagerImpl::DoMaintenance, this,
std::ref(connman)), std::chrono::seconds{1}); Do not change DoMaintenance
itself—it already checks m_queueman before calling CheckQueue and
clientman->DoMaintenance can safely run regardless of m_relay_txes.
---
Duplicate comments:
In `@src/coinjoin/walletman.cpp`:
- Around line 251-273: Move masternode resolution and normalization of
dsq.m_protxHash before the duplicate/hash checks: call
tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint) early, and if it
returns a valid dmn and dsq.m_protxHash.IsNull() set dsq.m_protxHash =
dmn->proTxHash, then perform m_queueman->HasQueue(dsq.GetHash()) and
m_queueman->HasQueueFromMasternode(...) as before; this ensures the hash used by
HasQueue/HasQueueFromMasternode matches the normalized object later added via
AddQueue().
- Around line 181-186: flushWallet() currently calls getClient() which releases
cs_wallet_manager_map before returning, leaving clientman as a raw pointer and
allowing removeWallet() to destroy the unique_ptr concurrently; change the code
so flushWallet() obtains a safe reference while holding the wallet-manager map
lock or uses a shared ownership type: either (A) modify
getClient()/CJWalletManagerImpl to return a std::shared_ptr to the client
manager (instead of a raw pointer) and use that shared_ptr in flushWallet(), or
(B) lock cs_wallet_manager_map inside flushWallet(), copy or move a
std::shared_ptr/borrowed handle to clientman while locked, then release the lock
and call ResetPool()/StopMixing() on the stable handle; ensure removeWallet()
still erases only after dropping its shared_ptr so no use-after-free occurs
(refer to symbols: flushWallet, getClient, removeWallet, cs_wallet_manager_map,
CJWalletManagerImpl, clientman).
🪄 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: 0131a9db-68c7-4cf9-bb31-ca09df52862e
📒 Files selected for processing (1)
src/coinjoin/walletman.cpp
| 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); |
There was a problem hiding this comment.
Address remaining clang-format pipeline failures.
The pipeline is still reporting formatting mismatches for EXCLUSIVE_LOCKS_REQUIRED placement on multi-line declarations. Run clang-format to fix these before merging.
Also applies to: 77-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/walletman.cpp` around lines 43 - 44, The
EXCLUSIVE_LOCKS_REQUIRED attribute is misformatted on multi-line declarations
(e.g., the processMessage method) causing clang-format pipeline failures; run
clang-format and adjust the declaration formatting so the
EXCLUSIVE_LOCKS_REQUIRED annotation is placed per project style (move it to the
correct position/line for multi-line function declarations or combine it with
the closing parenthesis as clang-format expects) for processMessage and the
other similar multi-line declarations (the other EXCLUSIVE_LOCKS_REQUIRED
occurrence around the same area) so the linting passes.
| void CJWalletManagerImpl::Schedule(CConnman& connman, CScheduler& scheduler) | ||
| { | ||
| if (!m_relay_txes) return; | ||
| scheduler.scheduleEvery(std::bind(&CCoinJoinClientQueueManager::DoMaintenance, std::ref(*queueman)), | ||
| std::chrono::seconds{1}); | ||
| scheduler.scheduleEvery(std::bind(&CoinJoinWalletManager::DoMaintenance, std::ref(walletman), std::ref(connman)), | ||
| scheduler.scheduleEvery(std::bind(&CJWalletManagerImpl::DoMaintenance, this, std::ref(connman)), | ||
| std::chrono::seconds{1}); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how relay_txes=false is used and whether wallet maintenance is expected
rg -n -B3 -A3 'relay_txes.*false|relay_txes == false' --type cppRepository: dashpay/dash
Length of output: 584
🏁 Script executed:
#!/bin/bash
# Look at DoMaintenance implementation to see what it does
rg -n -A15 'void CJWalletManagerImpl::DoMaintenance' src/coinjoin/walletman.cppRepository: dashpay/dash
Length of output: 836
🏁 Script executed:
#!/bin/bash
# Check if wallets can be added/used when relay_txes=false
rg -n -B5 -A5 'relay_txes.*true|relay_txes.*=' src/coinjoin/walletman.cpp | head -50Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check if there are tests showing expected behavior when relay_txes=false
fd -e cpp -e h test | xargs rg -l 'relay_txes' 2>/dev/null || echo "No test files found with relay_txes"Repository: dashpay/dash
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check initialization and usage of m_relay_txes in the class
rg -n 'm_relay_txes' src/coinjoin/walletman.cpp | head -20Repository: dashpay/dash
Length of output: 242
🏁 Script executed:
#!/bin/bash
# Check if AddWallet/wallets can be added when relay_txes is false
rg -n -B5 -A5 'AddWallet|AddClient' src/coinjoin/walletman.cpp | head -40Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check the header file for m_relay_txes declaration and comments
cat src/coinjoin/walletman.h | grep -n -B3 -A3 'relay_txes'Repository: dashpay/dash
Length of output: 528
🏁 Script executed:
#!/bin/bash
# Find where CJWalletManager::make() is called
rg -n 'CJWalletManager::make|CJWalletManager\(' --type cpp -B2 -A2Repository: dashpay/dash
Length of output: 1906
🏁 Script executed:
#!/bin/bash
# Check if there are any comments about relay_txes meaning or usage
rg -n -B2 -A2 'relay.*tx|relay_txes' src/coinjoin/ --type cpp | head -60Repository: dashpay/dash
Length of output: 4048
Remove the early return that gates wallet maintenance on relay_txes.
When m_relay_txes is false, Schedule() early returns and prevents all maintenance from running. However, the intent is to disable only queue processing (as documented: "Null when relay_txes is false (no queue processing)"), not wallet maintenance. The DoMaintenance() method already properly handles a null m_queueman with a null-check before calling CheckQueue() (line 196), so wallet-level maintenance via clientman->DoMaintenance() can safely run regardless. Remove the if (!m_relay_txes) return; guard and let the scheduler always invoke DoMaintenance().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/walletman.cpp` around lines 120 - 125, Remove the early return
in CJWalletManagerImpl::Schedule that checks m_relay_txes so the scheduler
always registers DoMaintenance; specifically, delete the guard "if
(!m_relay_txes) return;" in CJWalletManagerImpl::Schedule and always call
scheduler.scheduleEvery(std::bind(&CJWalletManagerImpl::DoMaintenance, this,
std::ref(connman)), std::chrono::seconds{1}); Do not change DoMaintenance
itself—it already checks m_queueman before calling CheckQueue and
clientman->DoMaintenance can safely run regardless of m_relay_txes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb2142db5b
ℹ️ 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_ProcessDSQueue); | ||
|
|
||
| if (m_queueman->HasQueue(dsq.GetHash())) return ret; |
There was a problem hiding this comment.
Replace this hash-only duplicate check with the previous outpoint-aware logic. CCoinJoinQueue::GetHash() is derived from serialized network fields and does not include masternodeOutpoint (SERIALIZE_METHODS in coinjoin.h), whereas the old q == dsq path compared outpoints. In this function, DSQUEUE messages with null m_protxHash are still accepted, so two different masternodes broadcasting the same denom/readiness/timestamp can collide on hash and cause the second valid queue to be dropped as a duplicate, reducing queue relay and join opportunities.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Legacy dsq-s are no longer a thing, m_protxHash can't be null.
3 participants
Issue being fixed or feature implemented
Current coinjoin subsystem have multiple classes that inherits from each other but some of them doesn't provide any real abstraction level yet complicate architecture.
CJWalletManagerImpl, CCoinJoinClientManager, and CCoinJoinClientQueueManager are 2 separate pieces of implementation of CJWalletManager while CJWalletManagerImpl is already private and could keep all implementation inside.
What was done?
This PR simplify CJ architecture by removing boiler code and unneeded abstraction levels to make code easier to support, read and improve.
The changes for this PR has been spotted and half-done during #7234 but it's not a direct dependency of final solution, so far as it was possible to fully cut CJ from dash-chainstate implementation.
CoinJoin Class Diagram — Before vs After [by claude]
How Has This Been Tested?
Run unit & functional tests.
Breaking Changes
N/A
Checklist: