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.
this info is already "leaked" by the DSCOMPLETE message no? |
How so? |
|
DSCOMPLETE is a point-to-point session message that triggers no relay, while the DSTX push makes participants early announcers to the whole network which where the leak is happening |
… finalized tx is observed 82794fe test(coinjoin): cover an unreadable pending-observation record (UdjinM6) f13e0e0 fix(coinjoin): report an unreadable pending-observation record only once (UdjinM6) 6695726 fix(coinjoin): don't treat an unreadable pending-observation record as empty (pasta) 71c8dde fix(coinjoin): commit pending-observation record and locks atomically (pasta) 22608a9 doc: add release notes for the CoinJoin pending-observation locks (UdjinM6) 490f54c test(coinjoin): cover pending-observation reload, fix test-order dependence (UdjinM6) f9c4beb fix(coinjoin): make pending-observation locks crash-safe and cheap to poll (UdjinM6) bced3b7 test(coinjoin): cover pending-observation input locking (pasta) 51ecf82 fix(coinjoin): keep mixed inputs locked until finalized tx is observed (pasta) Pull request description: ## Issue being fixed or feature implemented Client-side alternative to #7478, per #7478 (review). #7478 fixes the wire ordering coordinator-side; this fixes the underlying invariant on the node that actually bears the risk. A CoinJoin participant releases its selected inputs as soon as it processes `DSCOMPLETE(MSG_SUCCESS)`. The coordinator announces the finalized mixing transaction only via `PeerRelayInv` (delayed inventory trickling) while `DSCOMPLETE` is pushed directly, so the completion message routinely overtakes the `INV`/`GETDATA`/`DSTX` exchange. 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. Conflicting InstantSend votes then leave both transactions without an ISLock, which can stall ChainLocks. 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. Why client-side rather than coordinator-side: - **Message ordering is not wallet ordering.** Pushing `DSTX` before `DSCOMPLETE` only guarantees the transaction reaches the mempool first. `CMainSignals::TransactionAddedToMempool` enqueues to the scheduler, so `mapTxSpends` is updated asynchronously — the participant can still process `DSCOMPLETE` and unlock while the wallet has not recorded the spend. This fix keys off actual wallet state (`CWallet::IsSpent`), so it does not depend on scheduler FIFO ordering. - **No propagation change, so no anonymity-set leak.** A direct push moves the participants' own onward relay of the DSTX from "coordinator inbound trickle + getdata round trip" to `t0 + RTT`, making the coordinator and its N participants the earliest announcers of that round's transaction — a sybil observer could resolve the anonymity set to IPs. - **The node bearing the risk deploys the fix**, instead of depending on masternode operators upgrading, with no opt-out for un-upgraded clients. ## What was done? On `MSG_SUCCESS`, `CCoinJoinClientSession::CompletedTransaction` no longer unlocks the session's *mixing* inputs. They are handed to `CCoinJoinClientManager::AddPendingObservation`, which: - persists the wallet lock via `WalletBatch`, so a restart before the finalized transaction is observed cannot make the input selectable again, and - records the outpoint, with the time it was added, in a per-wallet pending-observation set mirrored to the wallet database under a new CoinJoin-specific record (`cj_pending_obs`). Because ownership is recorded explicitly rather than inferred, both the locks *and* their grace period survive a restart, and locks the user set themselves via `lockunspent` are never touched. `CCoinJoinClientManager::CheckPendingObservations` runs as its own scheduler task (`CJWalletManagerImpl::Schedule`, once a minute), deliberately independent of `DoMaintenance` and its gates so pending inputs can still unlock while mixing is stopped or CoinJoin itself is disabled. It releases a lock when: - the wallet observes a transaction spending the input (`CWallet::IsSpent`) — the normal path; or - the input is still unspent after `PENDING_OBSERVATION_TIMEOUT_SECONDS` (1h) — the fallback, so a mixing transaction that never propagated cannot strand funds. Two guards apply here: - **gated on `IsBlockchainSynced()`** — while catching up, the spending transaction may sit in a block we have not downloaded yet, and releasing on the strength of that would defeat the point; - **chain UTXO set *and* mempool are both consulted** — `findCoins()` reports outputs that exist in the UTXO set or are *created* by a mempool transaction, but `CCoinsViewMemPool::GetCoin` never looks at `mapNextTx`, so it cannot tell that a mempool transaction *spends* a chain output. Relying on it alone would misread the finalized mixing transaction sitting unprocessed in our own mempool as "never propagated" and release the input — reopening exactly the window this PR closes. `CTxMemPool::isSpent` is queried explicitly; - the user released the lock manually (e.g. `lockunspent`), which purges the pending entry. This standalone scheduling also matters in block-only mode (`-blocksonly=1`), where no other CoinJoin maintenance is scheduled at all since mixing needs transaction relay: the locks are persistent and are restored with the wallet, so without the check inputs from a session that completed before an earlier shutdown would stay locked forever. No other CoinJoin activity is scheduled in that mode, as before. Collateral inputs and every failure path keep the old immediate-unlock behavior. `getcoinjoininfo` gains a `pending_inputs` field for observability. ## How Has This Been Tested? - New unit test `coinjoin_pending_observation_tests` covers the full lifecycle: a user's own lock on a denominated coin is never adopted as pending, pending inputs stay locked and excluded from coin selection while unrelated inputs remain selectable, the set is mirrored to the wallet database, an observed spend releases the lock, the terminal timeout does not fire while the chain is unsynced, an input spent by a mempool transaction the wallet has not recorded yet is **kept** locked while a genuinely unspent one is released, and a manual unlock purges the pending entry. - The mempool-spender case was verified as a real regression test: removing the `!mempool.isSpent(outpoint)` guard makes it fail (the input gets released), and restoring it makes it pass. - `coinjoin_tests`, `wallet_tests`, `walletdb_tests` pass. - `test/functional/rpc_coinjoin.py`, `wallet_basic.py`, `wallet_fundrawtransaction.py`, `p2p_blocksonly.py` pass. On lock ordering: `CheckPendingObservations` holds `cs_wallet` across `chain().findCoins()`, which takes `cs_main`. That is the established direction here — `FundTransaction` ([wallet/spend.cpp:1170](src/wallet/spend.cpp:1170), straight from upstream) and the pre-existing CoinJoin signing path ([coinjoin/client.cpp:453](src/coinjoin/client.cpp:453)) both do the same. There is no `cs_main → cs_wallet` path to invert against: no wallet function asserts `cs_main`, and every validation-interface callback the wallet consumes is dispatched through the scheduler queue rather than synchronously under `cs_main`. Builds here are `-DDEBUG_LOCKORDER` and `wallet_fundrawtransaction.py` (which exercises that exact order alongside block connection and mempool activity) reports no inconsistency. ## Breaking Changes None on the wire. Behavioral: after a successful mixing session a client's mixed inputs stay locked (and are now persistently locked) until it sees the transaction spending them, instead of becoming immediately selectable. `getcoinjoininfo` gains one field. Wallet database: adds one new record, `cj_pending_obs`. It is written only when a mixing session completes successfully, and an older client loading such a wallet ignores it (counted as an unknown record) — the persisted `lockedutxo` entries it accompanies are understood by older clients regardless, so the inputs stay locked either way. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: UdjinM6: utACK 82794fe Tree-SHA512: 90684e17527657ffb24fee9d4be752eee9eb6d60a3de90750edd8cab12de62ef9790740d70cdb815aa5dd7c766e2899203e9ef3f645a26e62760e6f487e9616d
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