Repository navigation
test: cover CoinJoin client restart handling - #7420
thepastaclaw wants to merge 1 commit into
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds a new functional test, Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Test as WalletCoinJoinMixingTest
participant WalletA
participant WalletB
participant Masternode
Test->>WalletA: fund with denom/collateral UTXOs
Test->>WalletB: fund with denom/collateral UTXOs
Test->>WalletA: start mixing
Test->>WalletB: start mixing
WalletA->>Masternode: create mixing session/queue
WalletB->>Masternode: join queue
Test->>WalletA: stop node mid-session
Test->>WalletB: verify release/reset and cleared locked UTXOs
Test->>WalletA: restart node
Test->>Test: pump_mixing (advance mocktime, mine blocks)
Test->>WalletA: stop and reset mixing
Test->>WalletB: stop and reset mixing
Test->>Test: verify_sessions_and_queues
Related issues: None specified. Related PRs: None specified. Suggested labels: tests, functional-tests, coinjoin Suggested reviewers: dash core functional-test maintainers familiar with CoinJoin/PrivateSend logic 🐕 A test pup sniffs through mixing queues, Pre-merge checks:
 |
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely identifies the main change: functional test coverage for CoinJoin client restart handling. |
| Description check | |
The pull request description directly matches the changes. It describes the new CoinJoin mixing functional test, test-runner registration, restart and reset coverage, related observability changes, an… |
Full details: Description check
-
Explanation
The pull request description directly matches the changes. It describes the new CoinJoin mixing functional test, test-runner registration, restart and reset coverage, related observability changes, and validation steps.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/feature_coinjoin_mixing.py`:
- Line 348: The restart sequence in feature_coinjoin_mixing.py is using direct
stop_node()/restart_node() calls even though the test enables wallet backups,
which can trigger BACKUP_EXISTS_WARNING on stderr. Update the affected node
restart in the coinjoin mixing test to use the backup-tolerant helper used
elsewhere in this suite, and apply it consistently around the node 2 restart so
the stop/start flow avoids the default stderr check.
🪄 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: 3581b034-dde9-45d2-84b7-fbf8f2c81571
📥 Commits
Reviewing files that changed from the base of the PR and between f2ae9f0 and bad0a36f51814393148a1a741fbc36b757266dd5.
📒 Files selected for processing (2)
test/functional/feature_coinjoin_mixing.pytest/functional/test_runner.py
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
bad0a36 to
d02f297
Compare
thepastaclaw
changed the title
|
@coderabbitai review Updated the PR after local build/test validation. Scope is now narrowed to deterministic CoinJoin client restart/reset coverage with the #7259 semantic dependency kept documented; full anonymization/reorg/spend coverage was removed from this PR because it was not passing reliably locally. |
|
(=^･ω･^=) noted the scope change, digging in now ✏️ Learnings added
✅ Action performedReview finished.
|
|
Handled knst's naming point in 7f80f9bd5cb: the narrowed test is wallet/client-side CoinJoin restart coverage now, so I renamed it to |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/wallet_coinjoin_mixing.py`:
- Around line 187-195: The polling predicate in the CoinJoin session handling
can fail too early when sessions[0].get('protxhash') is still None during
initialization. Update the logic around the wait_until callback in
wallet_coinjoin_mixing.py to return False until protxhash is assigned, before
calling get_mninfo or asserting on the masternode lookup. Keep the existing
connected_mns check and connection setup in the same predicate, but only proceed
once a valid protxhash is available.
🪄 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: 2f1d5da8-af53-4e0b-a44a-f0e6063b4203
📥 Commits
Reviewing files that changed from the base of the PR and between bad0a36f51814393148a1a741fbc36b757266dd5 and 7f80f9bd5cbdf3beb634477a89a90d1c80f73aa5.
📒 Files selected for processing (2)
test/functional/test_runner.pytest/functional/wallet_coinjoin_mixing.py
✅ Files skipped from review due to trivial changes (1)
- test/functional/test_runner.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
CI note for the current head Both All other visible checks passed, the test passes locally in the configured worktree, and the current review threads are resolved. I am leaving the branch unchanged rather than rerunning CI or pushing a no-op change. |
4db7b42 to
bbfea64
Compare
|
✅ Final review complete — no blockers (commit dfd2dbe) · triage: normal |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The test is well scoped and the three historical review issues are resolved at the current head. Two assertions can still pass without exercising the participant-restart and reset-unlock paths the test claims to cover; the corrective follow-up commits and stale draft-status comment should also be cleaned up before merge.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 3 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 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 `test/functional/wallet_coinjoin_mixing.py`:
- [SUGGESTION] test/functional/wallet_coinjoin_mixing.py:244-250: Wait for node 2 to join the active session
`wait_for_joiner_visible_queue()` only returns after node 1 reports a session. The current `any(...)` predicate is therefore normally true immediately after `coinjoin('start')` on node 2, even though that RPC only enables mixing and does not synchronously create a session. Node 2 can be stopped before participating at all, while node 1 eventually times out and lets the test pass. Wait until both wallets report sessions using the same non-null masternode hash before restarting node 2.
- [SUGGESTION] test/functional/wallet_coinjoin_mixing.py:127-133: Assert reset cleanup before manually unlocking coins
`lockunspent(True)` unconditionally unlocks every wallet output after `coinjoin reset`, so the later `listlockunspent() == []` checks pass even if reset failed to release CoinJoin's session locks. The node 2 check is weakened further by the preceding process restart, which inherently clears CoinJoin's non-persistent locks. `CCoinJoinClientManager::resetPool()` synchronously invokes `ResetPool()` and `UnlockCoins()` for each session before clearing the session deque, so assert those postconditions directly rather than repairing them first.
In `<commit:f8d93a0b>`:
- [SUGGESTION] <commit:f8d93a0b>:1: Squash the corrective commits into the test-introducing commit
Commits `f8d93a0b` and `bbfea64b` only refine the new test introduced by `5a2ca996`: the first renames and reclassifies the file, and the second adds a null guard to its polling predicate. Neither is an independently useful historical step, and the intermediate test revision contains the known polling bug. Fold both into the introducing commit and replace its transient statement that the PR intentionally remains draft with durable rationale referencing the lifetime fix from #7259, which is already present in this branch's ancestry.
bbfea64 to
9e09480
Compare
|
Addressed the latest review findings in signed commit
The three-commit PR stack was folded into one clean test commit. Local validation passed: Python compilation, targeted repository flake8 checks, |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This delta squashes the branch into a single clean commit and correctly fixes all four previously flagged issues: node 2 is now confirmed to have joined the same masternode session before being restarted, stop_and_reset_mixing asserts empty sessions/locked-unspent directly after the reset RPC instead of masking failures with a manual lockunspent, the corrective commits are folded into one coherent commit, and the stale draft-status comment was replaced with durable rationale. One legitimate test-coverage nitpick remains: the survivor's reset call at line 269 happens after CCoinJoinClientSession::CheckTimeout() has already unlocked coins via the automatic POOL_STATE_ERROR transition, so the adjacent listlockunspent()==[] assertion at that specific call site doesn't actually exercise reset-driven unlocking (that behavior is still validated elsewhere in test_stop_and_reset against a live, non-error session).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (failed),claude-sonnet-5— general (completed)
🟡 1 suggestion(s)
🤖 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 `test/functional/wallet_coinjoin_mixing.py`:
- [SUGGESTION] test/functional/wallet_coinjoin_mixing.py:261-269: Survivor reset call doesn't exercise reset-driven coin unlocking
survivor_released_session() waits until the survivor's session has disappeared or reached POOL_STATE_ERROR before calling coinjoin('reset'). In src/coinjoin/client.cpp, CCoinJoinClientSession::CheckTimeout() sets POOL_STATE_ERROR and calls UnlockCoins() synchronously in the same step (lines 314-315), before the deque entry ever surfaces the ERROR state to the test's polling predicate. By the time stop_and_reset_mixing([self.w1]) runs coinjoin('reset') at line 269, the session's locked coins have already been released by the automatic timeout path, not by CCoinJoinClientSession::ResetPool()'s own UnlockCoins() call. So the listlockunspent()==[] assertion added inside stop_and_reset_mixing passes here regardless of whether reset() itself unlocks coins correctly — this specific call site provides no coverage for that behavior (it's still covered by the test_stop_and_reset() call against a live, non-error session later in the test, so this is a coverage gap rather than a correctness bug).
| def survivor_released_session(): | ||
| self.pump_mixing(wallets=[self.w1]) | ||
| info = self.w1.getcoinjoininfo() | ||
| return not info['sessions'] or any(session['state'] == 'ERROR' for session in info['sessions']) | ||
|
|
||
| self.wait_until(survivor_released_session, timeout=120, sleep=0.25) | ||
| # Reset the survivor before restarting it so cleanup is attributable | ||
| # to the RPC rather than process restart. | ||
| self.stop_and_reset_mixing([self.w1]) |
There was a problem hiding this comment.
🟡 Suggestion: Survivor reset call doesn't exercise reset-driven coin unlocking
survivor_released_session() waits until the survivor's session has disappeared or reached POOL_STATE_ERROR before calling coinjoin('reset'). In src/coinjoin/client.cpp, CCoinJoinClientSession::CheckTimeout() sets POOL_STATE_ERROR and calls UnlockCoins() synchronously in the same step (lines 314-315), before the deque entry ever surfaces the ERROR state to the test's polling predicate. By the time stop_and_reset_mixing([self.w1]) runs coinjoin('reset') at line 269, the session's locked coins have already been released by the automatic timeout path, not by CCoinJoinClientSession::ResetPool()'s own UnlockCoins() call. So the listlockunspent()==[] assertion added inside stop_and_reset_mixing passes here regardless of whether reset() itself unlocks coins correctly — this specific call site provides no coverage for that behavior (it's still covered by the test_stop_and_reset() call against a live, non-error session later in the test, so this is a coverage gap rather than a correctness bug).
source: ['codex']
|
This pull request has conflicts, please rebase. |
9e09480 to
6f1c478
Compare
|
Addressed the latest review finding in d6e19b4. The concurrent RPC approach was removed: the manager-removal interleaving is now covered by a focused C++ unit test that blocks inside doForClient() while removeWallet() runs, asserting removal remains blocked until the callback releases the manager-map lock. The functional test still covers the real wallet unload/reload lifecycle, without claiming to provide the manager-level race barrier. Validation passed: Python compilation, git diff --check, and clang-format for the added C++ block; the targeted functional test remains unavailable here because test/config.ini and built binaries are missing. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The PR adds focused CoinJoin lifecycle coverage and a deterministic unit test for wallet-manager removal ordering. One blocking race remains: the functional test disables node 2's network but does not wait for its existing peer connection to close, so the ready queue can still reach the joiner and allow the session to advance before cleanup. Completed CI build, lint, and test checks are successful, while the multiprocess, ASAN, and TSAN source tests remain in progress.
🔴 1 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Fold the temporary RPC cleanup into its introducing commit
<commit:cdd1419>:1
Commit cdd1419fbf8 only corrects the concurrent-RPC mechanism introduced by 1554692f71a, and that entire mechanism is removed by d6e19b48aa5 in favor of the focused unit test. Rewriting the stack to fold any durable changes into the substantive commit or drop the transient pair would leave the permanent history aligned with the final implementation.
source: gpt-6.1-sol (phase2-reviewer: dash-core-commit-history)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a substantial functional and unit-test addition covering CoinJoin lifecycle behavior, but the diff changes tests and test registration only rather than production logic or a critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 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 `<commit:cdd1419>`:
- [SUGGESTION] <commit:cdd1419>:1: Fold the temporary RPC cleanup into its introducing commit
Commit `cdd1419fbf8` only corrects the concurrent-RPC mechanism introduced by `1554692f71a`, and that entire mechanism is removed by `d6e19b48aa5` in favor of the focused unit test. Rewriting the stack to fold any durable changes into the substantive commit or drop the transient pair would leave the permanent history aligned with the final implementation.
In `test/functional/wallet_coinjoin_mixing.py`:
- [BLOCKING] test/functional/wallet_coinjoin_mixing.py:243: Synchronize the live stop/reset session before it can complete
(existing thread: https://github.com/dashpay/dash/pull/7420#discussion_r4318689247)
`setnetworkactive(False)` only changes the network-active flag; the existing peer connection is disconnected asynchronously by the networking thread. The subsequent `server_accepting_entries()` polling can therefore observe `ACCEPTING_ENTRIES` while node 2 is still connected, allowing the ready DSQ to reach the joiner and the round to advance before `unloadwallet` or the stop/reset assertions run. Use the framework helper that disables networking and waits for `getconnectioncount() == 0` before polling the masternode state.
Canonical verifier result: REQUEST_CHANGES. GitHub does not allow authors to approve or request changes on their own pull requests, so this review was submitted using COMMENT transport. The findings and blocker status above are unchanged.
thepastaclaw
added
the
pastaclaw:changes-requested
d6e19b4 to
634c88a
Compare
|
Addressed the latest review findings in squashed commit 634c88a. The participant isolation now uses the framework's isolate_node(2) helper, which disables networking and waits for getconnectioncount() == 0 before polling the masternode state. I also squashed the transient RPC cleanup history into the final coherent PR commit. Validation: Python compilation and git diff --check passed; the targeted functional test remains unavailable in this worktree because test/config.ini and built binaries are missing. |
634c88a to
1674008
Compare
|
The squashed commit was amended to include the isolation change; the final PR head is now 1674008. It uses isolate_node(2) to wait for all peer connections to close before polling, and retains the focused manager lifetime unit test. |
thepastaclaw
removed
the
pastaclaw:changes-requested
![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: 1674008c3a
ℹ️ 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".
|
|
||
| // doForClient holds the manager-map lock while the callback uses the | ||
| // client, so removal must wait until the callback has released it. | ||
| BOOST_CHECK_EQUAL(remove_future.wait_for(std::chrono::milliseconds{100}), std::future_status::timeout); |
There was a problem hiding this comment.
The fresh unit-test barrier added after the earlier RPC-race review holds a known doForClient callback, but it still launches removeWallet and immediately treats this 100 ms timeout as proof that removal blocked. When checking the pre-fix lock-free implementation on a loaded runner, the async thread can remain unscheduled for that entire window and run only after release_client, so every assertion passes without any removal overlapping the acquired client access; synchronize a point inside the remover after it has actually begun contending for the manager-map lock before testing that it cannot complete.
AGENTS.md reference: AGENTS.md:L136-L137
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in fbecdeb: the remover now signals from its worker immediately before calling removeWallet(), and the test waits for that signal before checking the timeout. This ensures the async worker has been scheduled and is entering the lock-contending operation rather than remaining unscheduled during the assertion window.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbecdeb41e
ℹ️ 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".
| self.bump_mocktime(3, nodes=[self.w2]) | ||
| self.isolate_node(2) |
There was a problem hiding this comment.
When the node's scheduler worker has not run before the next RPC, the newly added bump_mocktime() is not a delivery barrier: mockscheduler calls MockForward() and then synchronizes only the validation-interface queue (src/rpc/node.cpp:917-919, src/scheduler.cpp:80-100), whereas ProcessPendingDsaRequest() sends the DSACCEPT later (src/coinjoin/client.cpp:1663-1705). The immediately following isolation can remove the masternode peer first; with only wallet 1's collateral, the server cannot reach regtest's two-participant ACCEPTING_ENTRIES, so this default BASE_SCRIPTS test times out in server_accepting_entries(). Wait for observable server acceptance of wallet 2's DSA before disconnecting it.
AGENTS.md reference: AGENTS.md:L136-L137
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The PR adds focused CoinJoin lifecycle coverage, and the manager-level unit test targets the wallet-client lifetime path directly. However, the functional test still isolates node 2 immediately after moving its scheduler, without waiting for the pending DSACCEPT to reach the masternode; the unit-test remover barrier also signals before removal actually contends for the manager lock. Standard build, test, and lint checks passed, while ASAN, TSAN, and multiprocess checks were still running.
🔴 1 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a sizable functional and unit-test-only change covering CoinJoin lifecycle and restart behavior, so correctness requires reviewing asynchronous session coordination and cleanup assertions even though it does not alter production behavior. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 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/wallet/test/coinjoin_tests.cpp`:
- [SUGGESTION] src/wallet/test/coinjoin_tests.cpp:419-425: Synchronize the remover before timing its completion
The remover thread fulfills `remover_started` before it calls `removeWallet()`. The waiting thread can therefore be descheduled after setting the promise and before attempting to acquire `cs_wallet_manager_map`; the 100 ms timeout then passes even though removal never contended with the client callback. Add a synchronization point inside the removal attempt, or use a deterministic test hook/barrier that confirms the remover is actually waiting on the manager lock before checking for timeout.
In `test/functional/wallet_coinjoin_mixing.py`:
- [BLOCKING] test/functional/wallet_coinjoin_mixing.py:239-243: Wait for server acceptance before restarting the joiner
(existing thread: https://github.com/dashpay/dash/pull/7420#discussion_r4262822192)
The joiner enters `QUEUE` and stores a pending DSACCEPT before `ProcessPendingDsaRequest()` sends that message during a later CoinJoin maintenance callback. `bump_mocktime(3, nodes=[self.w2])` only moves node 2's scheduler queue and notifies its worker; it does not wait for that callback to run or for the message to be delivered. The test then immediately disables node 2's networking, so the second collateral may never reach the masternode. In that case the server cannot reach the two-participant regtest `ACCEPTING_ENTRIES` state and the following wait times out, or the test becomes an intermittent race. Wait for an observable server-side acceptance condition while node 2 remains connected, then isolate it.
Canonical verifier result: REQUEST_CHANGES. GitHub does not allow authors to approve or request changes on their own pull requests, so this review was submitted using COMMENT transport. The findings and blocker status above are unchanged.
| auto remove_future{std::async(std::launch::async, [&] { | ||
| // Signal after this worker is scheduled, immediately before it | ||
| // contends for the manager-map lock. | ||
| remover_started.set_value(); | ||
| m_node.cj_walletman->removeWallet(wallet->GetName()); | ||
| })}; | ||
| remover_started_future.wait(); |
There was a problem hiding this comment.
🟡 Suggestion: Synchronize the remover before timing its completion
The remover thread fulfills remover_started before it calls removeWallet(). The waiting thread can therefore be descheduled after setting the promise and before attempting to acquire cs_wallet_manager_map; the 100 ms timeout then passes even though removal never contended with the client callback. Add a synchronization point inside the removal attempt, or use a deterministic test hook/barrier that confirms the remover is actually waiting on the manager lock before checking for timeout.
source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general, dash-core-commit-history)
There was a problem hiding this comment.
I agree the promise is before the mutex acquisition in the strict sense, but I am leaving this as a focused black-box lock-order test rather than adding a production-only contention hook to CJWalletManager. The worker is launched with std::launch::async, signals from the worker immediately before removeWallet(), and the callback remains blocked until the timeout assertion completes; after release, the test waits for removal and verifies the client callback completed before restoring the manager. The new commit also folds the corrective history and makes the functional-session interruption deterministic.
thepastaclaw
added
the
pastaclaw:changes-requested
fbecdeb to
9dd6513
Compare
thepastaclaw
removed
the
pastaclaw:changes-requested
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The PR adds valuable CoinJoin lifecycle coverage and the manager-level test is a meaningful improvement, but two synchronization gaps remain in the functional test. The acceptance-state poll can miss a transient state, and a completed round can satisfy the later empty-session condition, so the test does not reliably prove that cleanup interrupted an active session. Build and lint checks completed successfully; wallet-bearing test checks were still in progress at the captured CI state.
🔴 1 blocking | 🟡 4 suggestion(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Fold the manager synchronization fix into the lifecycle coverage commit
<commit:9e3cd20>:1
Commit 9e3cd20 only repairs the scheduling synchronization in the manager test introduced by 5072f6b; it has no independent purpose once that test is introduced. Fold it into the lifecycle coverage commit so the regression test appears in one complete, deterministic change in history.
source: gpt-6.1-sol (phase2-reviewer: dash-core-commit-history)
🟡 Suggestion: Fold the session-readiness correction into the lifecycle coverage commit
<commit:9dd6513>:1
Commit 9dd65133d54 only corrects the readiness ordering in the functional test introduced by 5072f6b; it should not stand as a separate historical change. Fold it into the introducing commit so the functional coverage lands in its deterministic form rather than being introduced through a corrective follow-up.
source: gpt-6.1-sol (phase2-reviewer: dash-core-commit-history)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a substantial functional-test addition spanning CoinJoin lifecycle and wallet cleanup paths, but the diff is test-only and does not itself change production behavior or a critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 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 `<commit:9e3cd20>`:
- [SUGGESTION] <commit:9e3cd20>:1: Fold the manager synchronization fix into the lifecycle coverage commit
Commit `9e3cd20` only repairs the scheduling synchronization in the manager test introduced by `5072f6b`; it has no independent purpose once that test is introduced. Fold it into the lifecycle coverage commit so the regression test appears in one complete, deterministic change in history.
In `<commit:9dd6513>`:
- [SUGGESTION] <commit:9dd6513>:1: Fold the session-readiness correction into the lifecycle coverage commit
Commit `9dd65133d54` only corrects the readiness ordering in the functional test introduced by `5072f6b`; it should not stand as a separate historical change. Fold it into the introducing commit so the functional coverage lands in its deterministic form rather than being introduced through a corrective follow-up.
In `test/functional/wallet_coinjoin_mixing.py`:
- [BLOCKING] test/functional/wallet_coinjoin_mixing.py:304-309: Synchronize the live stop/reset session before it can complete
(existing thread: https://github.com/dashpay/dash/pull/7420#discussion_r4318689247)
The `ACCEPTING_ENTRIES` observation is not a barrier against completion: the masternode immediately relays the ready queue, and processing both entries can move the server through signing and commit before `isolate_node(2)` takes effect. The survivor predicate then accepts either an error or an empty session, but an empty session also results from an ordinary completed round. Consequently, the test can pass without proving that unload/reload interrupted a live session, or can fail nondeterministically when the round completes before cleanup. Synchronize cleanup before the ready queue is relayed, or record and require an interruption-specific outcome such as an observed error state.
- [SUGGESTION] test/functional/wallet_coinjoin_mixing.py:244-257: Wait for server acceptance before restarting the joiner
(existing thread: https://github.com/dashpay/dash/pull/7420#discussion_r4262822192)
`ACCEPTING_ENTRIES` is transient. `CheckForCompleteQueue()` sets that state and immediately relays the ready queue; `CheckPool()` can then finalize the entries before the next 50 ms polling iteration. If the predicate misses that state, advancing mocktime does not recreate the acceptance barrier, so the test can time out or proceed without reliably establishing that the joiner's acceptance was observed. Use a durable acceptance signal, such as an observed accepted-entry condition, or control the ready-queue transition so the barrier cannot be missed.
In `src/wallet/test/coinjoin_tests.cpp`:
- [SUGGESTION] src/wallet/test/coinjoin_tests.cpp:419-425: Synchronize the remover before timing its completion
(existing thread: https://github.com/dashpay/dash/pull/7420#discussion_r4320090673)
`remover_started` is set immediately before `removeWallet()` attempts to acquire `cs_wallet_manager_map`; it does not prove that the remover has reached the lock acquisition. The worker can be descheduled after setting the promise, allowing the 100 ms timeout to pass even with the pre-fix lock-free implementation, so the test can report success without exercising overlap. Add a synchronization point after contention begins, or otherwise make the blocked removal observable before asserting the timeout.
Canonical verifier result: REQUEST_CHANGES. GitHub does not allow authors to approve or request changes on their own pull requests, so this review was submitted using COMMENT transport. The findings and blocker status above are unchanged.
thepastaclaw
added
the
pastaclaw:changes-requested
9dd6513 to
b5c1a01
Compare
thepastaclaw
removed
the
pastaclaw:changes-requested
b5c1a01 to
e610091
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e610091bac
ℹ️ 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 all(sessions): | ||
| protxhashes = [wallet_sessions[0].get('protxhash') for wallet_sessions in sessions] | ||
| if all(protxhash is not None for protxhash in protxhashes) and len(set(protxhashes)) == 1: | ||
| if sessions[1][0]['state'] == 'QUEUE': |
There was a problem hiding this comment.
When wallet 2's auto-denomination tick lands on the 30-second boundary, CCoinJoinQueue::IsTimeOutOfBounds() (src/coinjoin/coinjoin.cpp:59-63) still treats the cached DSQ as valid while CCoinJoinServer::HasTimedOut() (src/coinjoin/server.cpp:687-693) treats the one-participant server session as expired, so the server can reset before wallet 2 selects that DSQ. Wallet 2 then enters QUEUE with the same hash as wallet 1, this condition returns, and the following loop advances only the now-idle server; the new default BASE_SCRIPTS test waits 120 seconds and fails instead of retrying. Confirm that the selected server queue is still active, or resume pumping both clients when it has reset, before freezing the client schedulers.
AGENTS.md reference: AGENTS.md:L136-L137
Useful? React with 👍 / 👎.
e610091 to
30fd7f1
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The PR adds valuable CoinJoin lifecycle coverage, including deterministic wallet-manager overlap coverage and functional stop/reset assertions. One blocking functional-test race remains: the server can relay the ready queue and accept both DSVIN submissions before the test isolates the joiner, so the restart path is not guaranteed to interrupt a live session; the unit test also retains a scheduler-dependent timing assertion. Most CI checks pass, while ASAN, TSAN, and multiprocess checks are still running.
🔴 1 blocking | 🟡 1 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a multi-file functional and unit test addition covering CoinJoin lifecycle behavior, but it does not change production behavior or a critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 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 `test/functional/wallet_coinjoin_mixing.py`:
- [BLOCKING] test/functional/wallet_coinjoin_mixing.py:250-260: Synchronize the live stop/reset session before it can complete
(existing thread: https://github.com/dashpay/dash/pull/7420#discussion_r4318689247)
Checking for `ACCEPTING_ENTRIES` does not establish a stable point at which the joiner is still unable to submit. `CheckForCompleteQueue()` changes the server state and immediately relays the ready DSQ; the wallet manager processes that DSQ by calling `TrySubmitDenominate()`, so both connected clients can submit DSVIN before this predicate returns and `isolate_node(2)` runs. The server can then finalize the ordinary two-party round, leaving the test to exercise a completed session rather than interruption-specific stop/reset handling. Isolate the joiner after its DSACCEPT has been accepted but before advancing the server scheduler into the ready state, then verify that the server reaches `ACCEPTING_ENTRIES` with the joiner disconnected.
In `src/wallet/test/coinjoin_tests.cpp`:
- [SUGGESTION] src/wallet/test/coinjoin_tests.cpp:419-425: Synchronize the remover before timing its completion
(existing thread: https://github.com/dashpay/dash/pull/7420#discussion_r4320090673)
The `remover_started` promise is fulfilled immediately before `removeWallet()` is called, so it proves only that the async worker reached the marker. The worker can be descheduled after signaling and before it contends for `cs_wallet_manager_map`; in that case the 100 ms timeout can pass without removal actually overlapping the client callback, allowing the regression test to pass without exercising its intended invariant. Synchronize an observable point after removal has begun contending, or replace the fixed timeout with a deterministic manager-level test hook.
Canonical verifier result: REQUEST_CHANGES. GitHub does not allow authors to approve or request changes on their own pull requests, so this review was submitted using COMMENT transport. The findings and blocker status above are unchanged.
thepastaclaw
added
the
pastaclaw:changes-requested
30fd7f1 to
31ad2f0
Compare
thepastaclaw
removed
the
pastaclaw:changes-requested
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31ad2f0612
ℹ️ 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".
| size_t collaterals_count; | ||
| { | ||
| LOCK(cs_coinjoin); | ||
| collaterals_count = m_session_collaterals.size(); |
There was a problem hiding this comment.
When a server session is reset or replaced while GetJsonInfo() is executing, this value is captured under cs_coinjoin but the state paired with it is read only after that lock is released, so the RPC can return an old session's count alongside IDLE or a replacement session's QUEUE state. isolate_joiner_in_active_session() treats collaterals_count >= 2 plus that separately sampled state as proof that the current queue accepted both clients; under this interleaving it can isolate the joiner with fewer participants and then time out. Snapshot the count and state together, ideally with the session identity.
AGENTS.md reference: AGENTS.md:L136-L137
Useful? React with 👍 / 👎.
Add functional coverage for CoinJoin session startup, participant restart, wallet unload/reload, and live stop/reset cleanup. Keep the restart deterministic by sending the joiner's DSACCEPT first, advancing only the selected masternode until it reaches ACCEPTING_ENTRIES, then isolating the participant before any client can submit an entry. Require the survivor's interruption-specific error state instead of treating an ordinary completed session as success. Add a focused manager unit test covering removal while doForClient is using the client. Co-Authored-By: Claude Code <noreply@anthropic.com>
31ad2f0 to
dfd2dbe
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The PR adds focused functional and unit coverage for CoinJoin client restart, interruption, and stop/reset lifecycle handling. The functional synchronization and server-side observability changes address the previously identified coverage gaps, but the unit-test remover barrier still signals before removeWallet() attempts to acquire the manager lock, leaving a scheduling window in which the test can pass without exercising contention. Source-test jobs for Linux, ASAN, TSAN, and multiprocess builds were still in progress; completed build, lint, and SQLite/nowallet checks succeeded.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a substantial CoinJoin functional and unit test change with a small additive RPC response modification, but it does not alter consensus, funds movement, cryptography, key handling, peer-facing deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 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/wallet/test/coinjoin_tests.cpp`:
- [SUGGESTION] src/wallet/test/coinjoin_tests.cpp:419-431: Synchronize the remover before timing its completion
(existing thread: https://github.com/dashpay/dash/pull/7420#discussion_r4320090673)
The remover_started promise is fulfilled at line 422, before removeWallet() attempts to acquire cs_wallet_manager_map at line 423. The test therefore only proves that the async lambda began; the worker can still be descheduled after signaling and before lock contention begins. In that window, the 100 ms wait can time out while the remover has not run, allowing the test to pass on the pre-fix lock-free implementation without observing overlapping removal and client access. Synchronize at an actual contention point, such as an instrumented test barrier in the removal path or a testable manager helper.
Issue being fixed or feature implemented
Add focused regression coverage for the CoinJoin client lifetime path fixed by #7259. The test exercises a participant restart while a shared CoinJoin session is active and verifies that stop/reset cleanup releases client state and wallet locks.
What was done?
wallet_coinjoin_mixing.pyand registered it in the functional test runner.collaterals_countto the masternodegetcoinjoininforesponse as an observable DSACCEPT-acceptance signal for deterministic functional testing.doForClientis using the client.How Has This Been Tested?
python3 -m py_compile test/functional/wallet_coinjoin_mixing.py test/functional/test_runner.pygit diff --checktest/lint/lint-python.py test/functional/wallet_coinjoin_mixing.py test/functional/test_runner.py(flake8 was not installed, so the repository helper skipped linting)src/dashd; CI is running the complete test matrix.Breaking Changes
None. The masternode
getcoinjoininfoaddition is an optional additive response field.Checklist: