fix(coinjoin): push finalized DSTX directly to participants before DSCOMPLETE#7478
fix(coinjoin): push finalized DSTX directly to participants before DSCOMPLETE#7478PastaPastaPasta wants to merge 1 commit into
Conversation
Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant CCoinJoinServer
participant Connman
participant ParticipantNode
CCoinJoinServer->>CCoinJoinServer: create fully signed DSTX
CCoinJoinServer->>Connman: send DSTX to participant address
Connman->>ParticipantNode: deliver signed DSTX
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 1
🧹 Nitpick comments (1)
src/coinjoin/server.cpp (1)
391-397: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAdd a functional test for DSTX-before-DSCOMPLETE ordering.
The new behavior depends on per-participant message ordering; a delayed-DSTX scenario should assert that the participant receives
DSTXbeforeDSCOMPLETE.🤖 Prompt for 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. In `@src/coinjoin/server.cpp` around lines 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.
🤖 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 `@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.
🪄 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: 70343333-a0d7-4cb9-8ae5-e4d22853ddbd
📒 Files selected for processing (6)
src/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/server.cppsrc/coinjoin/server.hsrc/rpc/coinjoin.cppsrc/wallet/test/coinjoin_tests.cpp
|
⛔ Blockers found — Sonnet deferred (commit 6b178c5) |
…COMPLETE The coordinator announced the finalized mixing transaction only via PeerRelayInv, which queues an inventory item subject to trickle delays, while DSCOMPLETE is pushed directly. A participant could therefore process the success notification long before receiving the transaction that spends its inputs. Push the fully signed DSTX directly to every mixing participant before sending DSCOMPLETE so that, with in-order message processing, participants observe the spend of their inputs before the completion message. The inv-based relay to the rest of the network is unchanged, and DSTX is an existing message so older clients handle the direct push as-is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
e18b458 to
6b178c5
Compare
PastaPastaPasta
changed the title
![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: 6b178c5098
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (const auto dstx = m_dstxman.GetDSTX(hashTx); dstx) { | ||
| WITH_LOCK(cs_coinjoin, RelayDSTXToParticipants(dstx)); |
There was a problem hiding this comment.
When a participant receives this direct DSTX, ProcessTransaction only enqueues the wallet's TransactionAddedToMempool callback; it does not synchronously mark the wallet input spent. The following directly queued DSCOMPLETE is handled immediately and CompletedTransaction() calls UnlockCoins(), so if the validation callback queue has not drained (a normal scheduling race, or a backlog), a -coinjoinmultisession=1 session can still select and sign the same input. Thus sending the messages in socket order does not establish the claimed wallet-observation ordering; retain the lock until the wallet-side callback/observed spend is processed.
AGENTS.md reference: AGENTS.md:L164-L164
Useful? React with 👍 / 👎.
PastaPastaPasta
removed
the
RPC
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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
🤖 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/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.
| // Push the fully signed transaction directly to every mixing participant first: | ||
| // the inv-based relay below is subject to delayed trickling, so without this a | ||
| // participant could process DSCOMPLETE (which is sent directly) and release its | ||
| // inputs for reuse before ever seeing the transaction that spends them. | ||
| if (const auto dstx = m_dstxman.GetDSTX(hashTx); dstx) { | ||
| WITH_LOCK(cs_coinjoin, RelayDSTXToParticipants(dstx)); |
There was a problem hiding this comment.
🔴 Blocking: 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.
source: ['codex']
There was a problem hiding this comment.
I agree; I'll work on another PR. I want this one just doing the send ordering fix
There was a problem hiding this comment.
Blocking
-
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. -
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. -
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
-
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. -
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); } -
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. -
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.
-
Add a test for the invariant. coinjoin_tests passing is a regression check, not evidence for the fix.
4 participants
Issue being fixed or feature implemented
A CoinJoin participant releases its selected inputs as soon as it processes
DSCOMPLETE(MSG_SUCCESS). However, the coordinator announces the finalized mixing transaction only viaPeerRelayInv(delayed inventory trickling) whileDSCOMPLETEis pushed directly, so the completion message can overtake theINV/GETDATA/DSTXexchange. During that window the session inputs are neither wallet-locked nor marked spent, so with-coinjoinmultisession=1(where the one-block cooldown is bypassed) another session can select and sign the very same inputs, producing two valid CoinJoin transactions spending the same outpoint. One will later be rejected.The broken invariant: after a successful session, a participant must observe the finalized transaction spending its inputs before it releases them for reuse. The old ordering allowed the release to happen first.
What was done?
CommitFinalTransactionnow pushes the fully signed DSTX directly to every mixing participant before sendingDSCOMPLETE, so with in-order message processing participants observe the spend of their inputs (via mempool acceptance) before the success notification unlocks them. The inv-based relay to the rest of the network is unchanged.DSTXis an existing message, so no protocol change: old clients accept the direct push as-is (unsolicitedTX/DSTXis explicitly non-punishable innet_processing), and the push is best-effort — a participant that misses it still gets the transaction via the regular inv relay, i.e. the previous behavior.Note: since the fix is coordinator-side, the race is closed as masternodes upgrade; a client mixing through an un-upgraded coordinator retains the previous behavior.
How Has This Been Tested?
coinjoin_testssuite passes locally.Breaking Changes
None. No wire-protocol changes (
DSTXpush uses an existing message).Checklist:
🤖 Generated with Claude Code