Actionable comments posted: 1

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 `@src/coinjoin/client.cpp`:
- Around line 702-705: Update CheckPendingObservations so it releases
m_wallet->cs_wallet before calling m_wallet->chain().findCoins(coins), then
reacquires cs_wallet before evaluating or updating the pending observation
result. Preserve the existing timeout loop and IsSpent handling while ensuring
all wallet state access after the query occurs under the re-acquired lock.

---

Nitpick comments:
In `@src/coinjoin/server.cpp`:
- Around line 391-397: Add a functional test covering the delayed-DSTX path
around RelayDSTXToParticipants and DSCOMPLETE handling. Configure the scenario
so DSTX delivery is delayed, then assert each mixing participant receives DSTX
before DSCOMPLETE, preserving the intended per-participant message ordering.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6b178c5098

Preliminary review — Codex only

The change guarantees DSTX-before-DSCOMPLETE ordering on the wire, but it does not guarantee that the wallet records the spend before completion unlocks the inputs. Because mempool notifications are asynchronous, the input-reuse race remains reachable when multi-session CoinJoin is enabled.

Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 1 blocking

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:391-396: Direct DSTX ordering does not prevent wallet input reuse
  Processing DSTX first only adds the transaction to the mempool and enqueues `TransactionAddedToMempool`; `validationinterface.cpp` dispatches that notification asynchronously, so `CWallet::transactionAddedToMempool()` may not yet have called `SyncTransaction()` and `AddToSpends()` when the next DSCOMPLETE is processed. DSCOMPLETE then calls `CompletedTransaction()` and unlocks the session inputs, while `AvailableCoins()` still sees them as unspent because it relies on the wallet's lock and `mapTxSpends` state. This is reachable with `-coinjoinmultisession=1`: if the scheduler dequeues a due CoinJoin maintenance task before the newly queued validation callback and blocks on `cs_wallet_manager_map` while DSCOMPLETE holds it, the maintenance task resumes immediately after completion unlocks the inputs and can select them before the same scheduler thread processes the wallet callback. Synchronize preceding validation callbacks before dispatching a validated matching DSCOMPLETE without holding CoinJoin locks, or retain the input locks until wallet observation is confirmed. Add a regression test that delays the validation callback and verifies that successful completion cannot make the spent inputs selectable.

Blocking

1. Don't land RelayDSTXToParticipants as-is — it leaks the participant set.
   The participant relays the DSTX onward unconditionally on ATMP success (net_processing.cpp:4844), with no suppression anywhere. Today it learns of the tx via the coordinator's inbound trickle (NextInvToInbounds, mean 5s) plus a getdata round trip, so it announces well behind the crowd. The direct push moves it to t0 + RTT, announcing concurrently with the coordinator's own outbound trickle (mean 2s). A sybil observer then sees the coordinator (self-identifying — it signs the DSTX) plus its N participants as the earliest announcers, resolving that round's anonymity set to IPs. Coordinator-side deployment means un-upgraded clients get this with no opt-out.

2. Fix it client-side instead.
   In CCoinJoinClientSession::CompletedTransaction (client.cpp:575), on MSG_SUCCESS hold vecOutPointLocked until the wallet observes the spend — the client already has finalMutableTransaction, it signed it — with a timeout fallback so locks can't leak if the tx never lands. Deterministic, no propagation change, and the node bearing the risk deploys the fix.

3. Document the MN-only-publication rationale in the code. It isn't written down anywhere, which is plausibly how this approach got proposed.

If the coordinator-side approach is pursued anyway

1. Correct the ordering claim.
   "Participants observe the spend before DSCOMPLETE unlocks" is true at the mempool level only. CMainSignals::TransactionAddedToMempool enqueues to the scheduler (validationinterface.cpp:215), so the wallet's mapTxSpends updates asynchronously. It works in practice because coin selection runs on that same single-threaded scheduler via DoMaintenance, but the invariant rests on scheduler FIFO ordering, not P2P message ordering — say so in the comment, or a future refactor reopens the race silently.

2. Keep the CCoinJoinBroadcastTx instead of re-querying.
   server.cpp:381-394 looks the map up twice; if the second GetDSTX misses, the fix is skipped with no log: CCoinJoinBroadcastTx dstx = m_dstxman.GetDSTX(hashTx);
   if (!dstx) { dstx = CCoinJoinBroadcastTx(...); dstx.vchSig = ...; m_dstxman.AddDSTX(dstx); }

3. Consider folding the push into RelayCompletedTransaction's existing loop (server.cpp:906) — one ForNode per peer instead of two, drops a near-copy of RelayFinalTransaction, and makes the DSTX→DSCOMPLETE adjacency structural rather than a property of
   two distant call sites.

4. Note the redundant inv. PeerRelayInv still announces to participants who just got the push; they answer AlreadyHave and never getdata. Harmless, but comment it so it doesn't read as an oversight.

5. Add a test for the invariant. coinjoin_tests passing is a regression check, not evidence for the fix.