refactor: simplify CoinJoin class hierarchy#7248
refactor: simplify CoinJoin class hierarchy#7248knst wants to merge 7 commits intodashpay:developfrom
Conversation
… of object initializtion in CJ
…unique_ptr inside ForEachCJClientMan
…tManagerImpl CJWalletManagerImpl is private implementation of CJWalletManager It helps to hide internal details of CJWalletManagerImpl which used public CCoinJoinClientManager It reduce amount of abstractions and complexity of implementation.
…Impl It reduce amount of abstractions and levels of inheritance While CJWalletManagerImpl is already private
…erver and make it a member
…injoin_basemanager_tests.cpp
|
![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.
WalkthroughThis pull request refactors the CoinJoin queue management architecture by replacing the inherited base manager pattern with a composition-based approach. The changes replace 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.
| 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.
Lock contention allows duplicate DSQUEUE messages through.
When TryCheckDuplicate cannot acquire the lock (Line 160), returning early silently allows the message to proceed without anti-spam validation. A malicious peer could potentially flood DSQUEUE messages during periods of lock contention.
The log message at Line 162 is misleading—it says "sending WAY too many dsq messages" but this path is only reached when *isDup is true, not on lock contention (where we return at Line 160).
🧰 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 - 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.
| 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.
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. |
2 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: