fix(coinjoin): keep mixed inputs locked until the finalized tx is observed - #7480
fix(coinjoin): keep mixed inputs locked until the finalized tx is observed#7480PastaPastaPasta wants to merge 5 commits into
Conversation
|
⛔ Blockers found — Sonnet deferred (commit 22608a9) |
![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: 68b7a62cff
ℹ️ 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 (m_wallet->IsSpent(outpoint)) { | ||
| vecToUnlock.push_back(outpoint); | ||
| } else { | ||
| m_pending_obs.emplace(outpoint, 0); |
There was a problem hiding this comment.
If the wallet is unloaded or the node restarts before the finalized transaction arrives, every reconstructed pending input is assigned timestamp 0; the timeout branch explicitly excludes such entries, so when the transaction never propagates the protective lock remains forever instead of being released after one hour. Persist the pending-observation purpose and start time separately so a reload can retain the terminal fallback without conflating these locks with user-created persistent locks.
AGENTS.md reference: AGENTS.md:L169-L171
Useful? React with 👍 / 👎.
| m_wallet->LockCoin(outpoint, &batch); | ||
| m_pending_obs.emplace(outpoint, nNow); |
There was a problem hiding this comment.
If WriteLockedUTXO fails because of a wallet database or I/O error, LockCoin returns false, but completion still removes the outpoint from the session's lock list and records it only in memory. A subsequent restart then makes an already-signed input selectable again, recreating the double-spend window this change is meant to close; check the return value and avoid treating the observation as durably registered on failure.
Useful? React with 👍 / 👎.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCoinJoin sessions retain mixing-input locks after successful completion and register those inputs for pending spend observation. The manager persists and restores pending state, releases locks when spends are observed or observations expire, and reconciles manual unlocks during maintenance. Scheduled checks also run when transaction relay is disabled, and CoinJoin information reports Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CoinJoinSession
participant CoinJoinManager
participant Wallet
participant Blockchain
participant Mempool
CoinJoinSession->>CoinJoinManager: Register mixed input outpoints
CoinJoinManager->>Wallet: Persist protective locks
CoinJoinManager->>Blockchain: Check whether outpoints are spent
CoinJoinManager->>Mempool: Check unconfirmed spend state
Blockchain-->>CoinJoinManager: Return spend state
Mempool-->>CoinJoinManager: Return spend state
CoinJoinManager->>Wallet: Release observed or expired locks
Possibly related PRs
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
🤖 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 643-723: Update ReconcilePersistedLocks and the
pending-observation bookkeeping so reconstructed locks receive the same finite
grace period and chain/mempool safety check as freshly tracked locks, including
periodic warning and release behavior for unspent inputs. Ensure user-created
locks are not misclassified as CoinJoin pending locks by using a persisted
CoinJoin-specific marker or equivalent ownership distinction, while preserving
manually released locks.
🪄 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 Plus
Run ID: 0c5eab18-9b49-4b43-b288-f7c57fb96463
📒 Files selected for processing (4)
src/coinjoin/client.cppsrc/coinjoin/client.hsrc/rpc/coinjoin.cppsrc/wallet/test/coinjoin_tests.cpp
68b7a62 to
bb38442
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb3844227c
ℹ️ 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".
| std::map<COutPoint, Coin> coins{{outpoint, Coin{}}}; | ||
| m_wallet->chain().findCoins(coins); | ||
| if (!coins.at(outpoint).IsSpent()) { |
There was a problem hiding this comment.
When the finalized transaction remains in the mempool but its wallet notification has been delayed past the one-hour timeout, this check can incorrectly release the input: chain().findCoins() delegates to CCoinsViewMemPool::GetCoin, which returns mempool-created outputs or falls through to the chain UTXO but does not check whether mapNextTx spends that chain output. Consequently the original coin appears unspent here, allowing CoinJoin to select it again before the wallet processes the transaction—the double-spend window this change is intended to close. Query mempool spentness explicitly, and add a timeout test with a mempool spender that has not yet been added to the wallet.
AGENTS.md reference: AGENTS.md:L169-L171
Useful? React with 👍 / 👎.
bb38442 to
7e69fd8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e69fd8c90
ℹ️ 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".
| // Run this before the mixing gates below: inputs of already completed sessions | ||
| // must be able to unlock even when mixing or CoinJoin itself is disabled, | ||
| // otherwise their persisted locks could linger forever | ||
| CheckPendingObservations(mempool); |
There was a problem hiding this comment.
When a wallet with pending observations is restarted with -blocksonly, this cleanup never executes: init.cpp constructs CJWalletManagerImpl with relay_txes=false, and CJWalletManagerImpl::Schedule returns without scheduling any maintenance in that mode. Consequently, even after the wallet observes the finalized transaction, its persisted pending locks and metadata remain indefinitely unless the user manually unlocks them. Schedule the wallet cleanup independently of relay/queue maintenance.
AGENTS.md reference: AGENTS.md:L169-L171
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/wallet/test/coinjoin_tests.cpp (1)
253-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock time can leak into sibling tests if an assertion throws.
SetMockTime(0)only runs on the happy path; a failingBOOST_REQUIREinside the lambda aborts the test case with mock time still set, which can destabilize subsequent cases in this suite. Consider resetting via a scope-guard (orSetMockTime(0)outside the lambda at the end of the test body).Also applies to: 342-342
🤖 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/wallet/test/coinjoin_tests.cpp` around lines 253 - 254, Ensure the test’s mock time is reset even when an assertion inside the lambda fails: update the setup around SetMockTime in the affected coinjoin tests to use a scope guard or equivalent cleanup that always calls SetMockTime(0), including the second occurrence. Preserve the existing mocked-time behavior during each test.
🤖 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.
Nitpick comments:
In `@src/wallet/test/coinjoin_tests.cpp`:
- Around line 253-254: Ensure the test’s mock time is reset even when an
assertion inside the lambda fails: update the setup around SetMockTime in the
affected coinjoin tests to use a scope guard or equivalent cleanup that always
calls SetMockTime(0), including the second occurrence. Preserve the existing
mocked-time behavior during each test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0234d8a8-30d7-4574-9a0b-a034db52ad50
📒 Files selected for processing (6)
src/coinjoin/client.cppsrc/coinjoin/client.hsrc/rpc/coinjoin.cppsrc/wallet/test/coinjoin_tests.cppsrc/wallet/walletdb.cppsrc/wallet/walletdb.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/rpc/coinjoin.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The pending-observation mechanism closes the CoinJoin input-reuse window and provides focused lifecycle coverage. Two blocking issues remain: the timeout path reverses the required chain/wallet lock order, and block-only mode never schedules the fallback cleanup for persisted pending inputs.
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)
🔴 2 blocking
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 `src/coinjoin/client.cpp`:
- [BLOCKING] src/coinjoin/client.cpp:713-715: Acquire chain locks before the wallet lock
`CheckPendingObservations()` holds `m_wallet->cs_wallet` from line 676 while calling `chain().findCoins()`. The node implementation delegates to `node::FindCoins()`, which acquires `cs_main` and the mempool lock at `src/node/coin.cpp:16`, establishing the prohibited order `cs_wallet -> cs_main`. Once an entry reaches the timeout, the periodic maintenance task can deadlock with a path using the required `cs_main -> cs_wallet` order. Inspect chain and mempool spentness without holding `cs_wallet`, then acquire the wallet lock and revalidate the pending entry before changing its lock state.
In `src/coinjoin/walletman.cpp`:
- [BLOCKING] src/coinjoin/walletman.cpp:120-124: Schedule pending-observation cleanup in block-only mode
`Schedule()` returns without scheduling `DoMaintenance()` when `m_relay_txes` is false. Initialization passes `!ignores_incoming_txs` as `m_relay_txes`, so this occurs under `-blocksonly=1`, and `DoMaintenance()` is the only production caller of `CheckPendingObservations()`. After a restart, the wallet loader restores each persistent `lockedutxo`, but the CoinJoin pending record remains unloaded and its one-hour fallback is never evaluated. If the finalized transaction never confirms, the input therefore stays locked indefinitely. Schedule pending-observation cleanup independently of transaction relay and normal queue/mixing maintenance while preserving the existing prohibition on regular CoinJoin activity in block-only mode.
| std::map<COutPoint, Coin> coins{{outpoint, Coin{}}}; | ||
| m_wallet->chain().findCoins(coins); | ||
| if (!coins.at(outpoint).IsSpent() && !mempool.isSpent(outpoint)) { |
There was a problem hiding this comment.
🔴 Blocking: Acquire chain locks before the wallet lock
CheckPendingObservations() holds m_wallet->cs_wallet from line 676 while calling chain().findCoins(). The node implementation delegates to node::FindCoins(), which acquires cs_main and the mempool lock at src/node/coin.cpp:16, establishing the prohibited order cs_wallet -> cs_main. Once an entry reaches the timeout, the periodic maintenance task can deadlock with a path using the required cs_main -> cs_wallet order. Inspect chain and mempool spentness without holding cs_wallet, then acquire the wallet lock and revalidate the pending entry before changing its lock state.
source: ['codex']
There was a problem hiding this comment.
Lock ordering (cs_wallet → cs_main) — FALSE POSITIVE
The finding's premise is that cs_wallet → cs_main is "the prohibited order." It isn't — it's the established one in this codebase:
wallet/spend.cpp:1170 — FundTransaction takes LOCK(wallet.cs_wallet) eight lines before wallet.chain().findCoins(coins). That's upstream Bitcoin code.
coinjoin/client.cpp:453 — the pre-existing SignFinalTransaction takes cs_wallet, then calls findCoins at line 536.
For the deadlock to be reachable there must be a cs_main → cs_wallet path. I looked for one:
No wallet function asserts or requires cs_main (grep AssertLockHeld(cs_main) / EXCLUSIVE_LOCKS_REQUIRED(cs_main across src/wallet/ returns nothing).
Every validation-interface callback the wallet consumes — TransactionAddedToMempool, BlockConnected, BlockDisconnected, UpdatedBlockTip, ChainStateFlushed, NotifyChainLock, NotifyTransactionLock — goes through ENQUEUE_AND_LOG_EVENT onto the scheduler queue. They run on the scheduler thread with no cs_main held. The wallet's proxy notably uses UpdatedBlockTip, not the synchronous variant.
Empirically: these builds are -DDEBUG_LOCKORDER, which aborts on an observed inversion. wallet_fundrawtransaction.py exercises cs_wallet → cs_main heavily alongside block connection and mempool activity, and passes with no inconsistency reported.
The suggested remedy would also introduce the bug it warns about: releasing cs_wallet mid-loop while still holding cs_pending_obs establishes cs_pending_obs → cs_wallet, inverting against AddPendingObservation, which takes them the other way. Plus a TOCTOU gap between the spentness query and the unlock decision. No change made.
The doc reference the finding leans on (developer-notes.md:598) is a generic illustration of what deadlock is — it doesn't declare a required order between these two.
There was a problem hiding this comment.
Resolved in f9c4beb — Acquire chain locks before the wallet lock no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
A client releases its session inputs as soon as it processes DSCOMPLETE(MSG_SUCCESS), but the finalized DSTX is relayed via delayed inventory trickling and can arrive much later. During that window the inputs are neither wallet-locked nor marked spent, so another session (especially with -coinjoinmultisession=1) can select and sign them again, producing two valid CoinJoin transactions spending the same outpoint. Conflicting InstantSend votes then leave both transactions without an ISLock, which can stall ChainLocks. Instead of unlocking on success, move the session's mixing inputs into a per-wallet pending-observation set: they stay wallet-locked and are released only once the wallet observes a mempool or confirmed transaction spending them. The set is mirrored to the wallet database under a CoinJoin-specific record, so both the locks and their grace period survive a restart and are never confused with locks the user set themselves via lockunspent. If no spend is ever observed the input is released after a conservative timeout, but only on a synced chain and only if it is unspent per the chain UTXO set and the mempool. Both checks are needed: findCoins() reports outputs which exist in the UTXO set or are created by a mempool transaction, but CCoinsViewMemPool::GetCoin never consults mapNextTx, so a finalized mixing transaction sitting unprocessed in our own mempool would otherwise be misread as "never propagated". The release check runs before the mixing gates in DoMaintenance so pending inputs can still unlock while mixing or CoinJoin itself is disabled. In block-only mode no CoinJoin maintenance is scheduled at all, so the check is scheduled on its own there - locks restored from a previous run would never be released otherwise. A manual user unlock purges the pending entry, and collateral inputs and all failure paths keep the old immediate-unlock behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cover the pending-observation lifecycle: a user's own lock on a denominated coin is never adopted as pending, pending inputs stay locked and excluded from coin selection, the set is mirrored to the wallet database, an observed spend releases the lock, the terminal timeout only fires on a synced chain, 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 user unlock purges the pending entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7e69fd8 to
bced3b7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/wallet/test/coinjoin_tests.cpp (1)
253-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock time leaks into later test cases if an assertion throws.
SetMockTime(0)at Line 342 is the last statement of the lambda, so anyBOOST_REQUIREfailure above it leaves global mock time set forcoinjoin_manager_start_stop_testsand the rest of the suite. A small RAII guard (orSetMockTimereset in a scope-exit) keeps failures isolated.Also applies to: 342-342
🤖 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/wallet/test/coinjoin_tests.cpp` around lines 253 - 254, Update the test scope around nStart and SetMockTime in the coinjoin test lambda to install an RAII or scope-exit cleanup that always calls SetMockTime(0), including when an assertion throws. Keep the existing mock-time setup and test behavior unchanged while ensuring cleanup runs before coinjoin_manager_start_stop_tests and subsequent cases.
🤖 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.
Nitpick comments:
In `@src/wallet/test/coinjoin_tests.cpp`:
- Around line 253-254: Update the test scope around nStart and SetMockTime in
the coinjoin test lambda to install an RAII or scope-exit cleanup that always
calls SetMockTime(0), including when an assertion throws. Keep the existing
mock-time setup and test behavior unchanged while ensuring cleanup runs before
coinjoin_manager_start_stop_tests and subsequent cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 95a67d4f-e247-478a-83cf-1eba94e37a2a
📒 Files selected for processing (7)
src/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/walletman.cppsrc/rpc/coinjoin.cppsrc/wallet/test/coinjoin_tests.cppsrc/wallet/walletdb.cppsrc/wallet/walletdb.h
🚧 Files skipped from review as they are similar to previous changes (5)
- src/rpc/coinjoin.cpp
- src/wallet/walletdb.h
- src/coinjoin/client.h
- src/wallet/walletdb.cpp
- src/coinjoin/client.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bced3b75a7
ℹ️ 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".
| fPersisted &= m_wallet->LockCoin(outpoint, &batch); | ||
| m_pending_obs.emplace(outpoint, nNow); |
There was a problem hiding this comment.
When a user makes an already session-locked input persistent with lockunspent false ... true, the RPC explicitly allows the operation and writes a lockedutxo record, but this call cannot distinguish that user-owned record and adopts the outpoint as a CoinJoin pending lock. If the finalized transaction never propagates, the timeout path later calls UnlockCoin and erases the user's persistent lock, making the coin selectable contrary to the user's request. Track whether persistence already existed before adopting the input, and add a timeout test for that case.
AGENTS.md reference: AGENTS.md:L169-L171
Useful? React with 👍 / 👎.
… poll Review follow-ups to the pending-observation mechanism: - AddPendingObservation() persisted the wallet locks before the cj_pending_obs record which owns them. Those are two separate writes, so a crash in between left persistently locked coins behind with nothing tracking them and no timeout to release them. Write the record first and only persist the locks if that succeeded; the resulting order is self-healing, CheckPendingObservations() drops a pending entry as soon as it sees its coin is not locked. - CheckPendingObservations() constructed a WalletBatch on every pass, and every WalletBatch checkpoints the wallet database when it goes out of scope. Open it lazily, on the first actual write, and read the record with fFlushOnClose=false. - Do not persist the timer refresh applied to an input which is spent in chain/mempool but not according to the wallet. Such an entry can be terminal, and rewriting the record for it buys nothing: the worst a restart can do is re-run the check once. Log it so it is diagnosable. - Schedule the check on its own rather than running it from DoMaintenance ahead of the mixing gates. It keeps the same property - pending inputs unlock even while mixing or CoinJoin itself is disabled - with one cadence for both relay and block-only mode instead of once per second in one and once per minute in the other. Releasing these locks is pure bookkeeping: the outpoints are the ones the finalized mixing transaction spends, so by the time the wallet observes the spend they could not be selected again anyway. - Move the timeout next to the other CoinJoin timeouts in coinjoin.h and restore the alphabetical include order. - Mark the wrapped log format strings with /* Continued */ so lint-logs.py is satisfied, matching init.cpp and net_processing.cpp. It only inspects the first line of a log call and requires it to end with "\n". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ndence coinjoin_pending_observation_reload_tests drops the client manager and creates a fresh one for the same wallet, the way a restart would, and checks that the pending entry is read back from the wallet database along with its grace period: the terminal timeout has to be measured from when the session completed, not from when the manager was recreated. Without the record surviving, the persisted lock would be left with nothing to release it. Also pin minRelayTxFee in the fixture, which fixes the win64 CI failure on coinjoin_pending_observation_tests: coinjoin_tests.cpp(208): fatal error: critical check res has failed minRelayTxFee is a global. transaction_tests raises it and then restores DUST_RELAY_TX_FEE rather than DEFAULT_MIN_RELAY_TX_FEE, so it stays at 3 duff/B for the rest of the process. GetTallyItem() asks for CFeeRate(1000), which is below that, and every CreateTransaction() call in the fixture then fails with Fee rate (1.000 duff/B) is lower than the minimum fee rate setting (3.000 duff/B) Boost shuffles test order in CI, which is why this only shows up in some runs and never in declaration order locally. It reproduces reliably with test_dash --run_test=transaction_tests,coinjoin_tests CTransactionBuilderTest and wallet_tests already defend against this by resetting the global themselves; do it in the fixture constructor instead so every test using it is covered rather than each one having to remember. GetTallyItem() also asserted CreateTransaction() succeeded with a bare BOOST_REQUIRE, reporting only "critical check res has failed" with no indication of why - which is what made the above take a while to pin down. Report the error string and the amount instead. rpc_coinjoin.py asserts the new getcoinjoininfo pending_inputs field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Covers the new getcoinjoininfo pending_inputs field, the behavioral change to when mixed inputs are released, and the new cj_pending_obs wallet database record. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22608a9ff4
ℹ️ 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".
| // persistently locked coins behind with nothing tracking them: nothing would ever | ||
| // release them again. The opposite order is self-healing, CheckPendingObservations() | ||
| // drops a pending entry as soon as it sees its coin is not locked. | ||
| bool fPersisted{batch.WriteCoinJoinPendingObs(m_pending_obs)}; |
There was a problem hiding this comment.
With -coinjoinautostart=1, a crash after WriteCoinJoinPendingObs commits but before all WriteLockedUTXO calls complete leaves the affected inputs unlocked after restart; DoMaintenance first runs after one second and can select them, while the cleanup that notices the missing lock first runs after one minute. The fresh evidence in this revision is that these remain separate autocommitted database writes, so writing the owner record first only changes which half survives rather than making the operation crash-safe; use a wallet database transaction for the record and all locks.
AGENTS.md reference: AGENTS.md:L169-L171
Useful? React with 👍 / 👎.
| m_wallet->chain().findCoins(coins); | ||
| if (!coins.at(outpoint).IsSpent() && !mempool.isSpent(outpoint)) { |
There was a problem hiding this comment.
When the timeout check races with the finalized transaction being mined, findCoins() can return the chain output while the transaction is still in the mempool, then block connection can spend the chain output and remove the transaction before the separate mempool.isSpent() call; both operands consequently report "unspent" and the lock is released before the queued wallet notification records the spend. The fresh evidence in the current revision is that the added mempool check uses a second lock acquisition rather than the same cs_main/mempool snapshot as findCoins(); query both states atomically.
AGENTS.md reference: AGENTS.md:L169-L171
Useful? React with 👍 / 👎.
| m_wallet->UnlockCoin(outpoint, &get_batch()); | ||
| it = m_pending_obs.erase(it); |
There was a problem hiding this comment.
If the wallet database returns an I/O error while erasing lockedutxo, CWallet::UnlockCoin has already removed the in-memory lock but returns false; this result is ignored, the pending entry is erased, and the reduced pending map can still be persisted. After restart the surviving lockedutxo record recreates a permanent lock with no pending owner to release it; only erase the observation after the persistent unlock succeeds, including in the timeout branch.
Useful? React with 👍 / 👎.
| { | ||
| UniValue obj(UniValue::VOBJ); | ||
| obj.pushKV("running", isMixing()); | ||
| obj.pushKV("pending_inputs", static_cast<int64_t>(GetPendingObservationCount())); |
There was a problem hiding this comment.
After loading a wallet that has persisted pending observations, m_pending_obs remains empty until CheckPendingObservations() lazily reads the database; because scheduleEvery performs its first cleanup run only after the one-minute interval, getcoinjoininfo incorrectly reports pending_inputs: 0 during that window even though the corresponding coins are already locked. Load the record before answering this RPC or initialize it when the client manager is created.
AGENTS.md reference: AGENTS.md:L169-L171
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings: the non-atomic chain/mempool snapshot and missing DSCOMPLETE migration-path coverage both remain valid at the current head. New latest-delta findings: pending records and persistent locks are still not committed atomically, commit f9c4beb is not independently test-buildable, and the release notes overstate the timeout as a one-hour maximum. Three blocking correctness or build-history issues remain, so changes are required.
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)
🔴 3 blocking | 🟡 1 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 `src/coinjoin/client.cpp`:
- [BLOCKING] src/coinjoin/client.cpp:646-653: Commit the pending record and persistent locks atomically
`AddPendingObservation()` writes `cj_pending_obs` first and then writes each `lockedutxo` separately. `WalletBatch` explicitly documents that every write is its own transaction unless `TxnBegin()` is used, so a crash after the pending record is committed but before every lock is written can leave an input durable in the pending set without its protective persistent lock. On restart, the wallet exposes that input as selectable, and `CheckPendingObservations()` interprets the missing lock as a manual unlock and deletes the recovery record. Commit the pending record and all associated lock records in one explicit wallet-database transaction, aborting the transaction on any write failure.
- [BLOCKING] src/coinjoin/client.cpp:736-738: Take an atomic chain-and-mempool spentness snapshot
`findCoins()` and `mempool.isSpent()` observe state under separate lock scopes. After `findCoins()` reports the chain output as unspent, block connection can spend that output and remove its spending transaction from the mempool before `isSpent()` runs, causing both checks to indicate that it is unspent. The wallet's queued `BlockConnected` callback cannot update `CWallet::IsSpent()` while this function holds `cs_wallet`, so the code can remove the pending entry and protective lock during the pre-wallet-observation window this PR is intended to close. Query chain UTXO existence and mempool `mapNextTx` spentness through one operation holding `cs_main` and the mempool lock for a coherent snapshot.
In `<commit:f9c4bebea16>`:
- [BLOCKING] <commit:f9c4bebea16>:1: Move the timeout-reference update into f9c4bebea16
Commit f9c4bebea16 removes `CCoinJoinClientManager::PENDING_OBSERVATION_TIMEOUT_SECONDS` from `src/coinjoin/client.h`, but its version of `src/wallet/test/coinjoin_tests.cpp:313` still references that removed member. The replacement with `COINJOIN_PENDING_OBSERVATION_TIMEOUT` appears only in the following commit, 490f54cb96f, so checking out f9c4bebea16 fails to compile the wallet test target and creates a broken bisect revision. Fold the one-line reference update into f9c4bebea16 while leaving the substantive reload and test-order coverage in 490f54cb96f.
In `src/wallet/test/coinjoin_tests.cpp`:
- [SUGGESTION] src/wallet/test/coinjoin_tests.cpp:263: Exercise the DSCOMPLETE input migration path
Both pending-observation tests enter through `CCoinJoinClientManager::AddPendingObservation()` directly, bypassing `CCoinJoinClientSession::CompletedTransaction()`. That production path handles successful DSCOMPLETE by separating mixing inputs, which remain pending, from collateral inputs, which unlock immediately. Removing the production call or regressing this classification would leave the current tests passing. Add coverage that completes a populated session with `MSG_SUCCESS` and verifies both input classes, together with the immediate-unlock behavior for failed completion.
| bool fPersisted{batch.WriteCoinJoinPendingObs(m_pending_obs)}; | ||
| for (const auto& outpoint : outpoints) { | ||
| // The coins are locked in memory already (see PrepareDenominate), this only | ||
| // persists the lock so that a restart before the finalized transaction is | ||
| // observed cannot make the input available for selection again. Skip persisting | ||
| // it if the record above could not be written though, a persistent lock with | ||
| // nothing left to release it strands the input. | ||
| if (!m_wallet->LockCoin(outpoint, fPersisted ? &batch : nullptr)) fPersisted = false; |
There was a problem hiding this comment.
🔴 Blocking: Commit the pending record and persistent locks atomically
AddPendingObservation() writes cj_pending_obs first and then writes each lockedutxo separately. WalletBatch explicitly documents that every write is its own transaction unless TxnBegin() is used, so a crash after the pending record is committed but before every lock is written can leave an input durable in the pending set without its protective persistent lock. On restart, the wallet exposes that input as selectable, and CheckPendingObservations() interprets the missing lock as a manual unlock and deletes the recovery record. Commit the pending record and all associated lock records in one explicit wallet-database transaction, aborting the transaction on any write failure.
source: ['codex']
| std::map<COutPoint, Coin> coins{{outpoint, Coin{}}}; | ||
| m_wallet->chain().findCoins(coins); | ||
| if (!coins.at(outpoint).IsSpent() && !mempool.isSpent(outpoint)) { |
There was a problem hiding this comment.
🔴 Blocking: Take an atomic chain-and-mempool spentness snapshot
findCoins() and mempool.isSpent() observe state under separate lock scopes. After findCoins() reports the chain output as unspent, block connection can spend that output and remove its spending transaction from the mempool before isSpent() runs, causing both checks to indicate that it is unspent. The wallet's queued BlockConnected callback cannot update CWallet::IsSpent() while this function holds cs_wallet, so the code can remove the pending entry and protective lock during the pre-wallet-observation window this PR is intended to close. Query chain UTXO existence and mempool mapNextTx spentness through one operation holding cs_main and the mempool lock for a coherent snapshot.
source: ['codex']
|
|
||
| const int64_t nStart{GetTime()}; | ||
| SetMockTime(nStart); | ||
| cj_man.AddPendingObservation({outpointPending}); |
There was a problem hiding this comment.
🟡 Suggestion: Exercise the DSCOMPLETE input migration path
Both pending-observation tests enter through CCoinJoinClientManager::AddPendingObservation() directly, bypassing CCoinJoinClientSession::CompletedTransaction(). That production path handles successful DSCOMPLETE by separating mixing inputs, which remain pending, from collateral inputs, which unlock immediately. Removing the production call or regressing this classification would leave the current tests passing. Add coverage that completes a populated session with MSG_SUCCESS and verifies both input classes, together with the immediate-unlock behavior for failed completion.
source: ['codex']
| is processed. They stay locked until the wallet observes a transaction | ||
| spending them, or for at most an hour if that transaction never propagates. |
There was a problem hiding this comment.
💬 Nitpick: Avoid promising a one-hour maximum lock duration
The one-hour threshold is evaluated only while blockchain synchronization is complete and only on the scheduler's one-minute cadence, so it is not an upper bound. A delayed scheduler pass or an unsynchronized node can retain an otherwise releasable lock for longer than an hour, while an input reported spent by chain or mempool remains locked until the wallet observes the spend. Describe the timeout as a spentness check performed after one hour once synchronized, rather than saying inputs remain locked for "at most an hour."
source: ['codex']
3 participants
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 viaPeerRelayInv(delayed inventory trickling) whileDSCOMPLETEis pushed directly, so the completion message routinely overtakes 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. 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:
DSTXbeforeDSCOMPLETEonly guarantees the transaction reaches the mempool first.CMainSignals::TransactionAddedToMempoolenqueues to the scheduler, somapTxSpendsis updated asynchronously — the participant can still processDSCOMPLETEand 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.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.What was done?
On
MSG_SUCCESS,CCoinJoinClientSession::CompletedTransactionno longer unlocks the session's mixing inputs. They are handed toCCoinJoinClientManager::AddPendingObservation, which:WalletBatch, so a restart before the finalized transaction is observed cannot make the input selectable again, andcj_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 vialockunspentare never touched.CCoinJoinClientManager::CheckPendingObservations(run fromDoMaintenance, deliberately before the mixing/enabled gates so pending inputs can still unlock while mixing or CoinJoin itself is disabled) releases a lock when:CWallet::IsSpent) — the normal path; orPENDING_OBSERVATION_TIMEOUT_SECONDS(1h) — the fallback, so a mixing transaction that never propagated cannot strand funds. Two guards apply here: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;findCoins()reports outputs that exist in the UTXO set or are created by a mempool transaction, butCCoinsViewMemPool::GetCoinnever looks atmapNextTx, 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::isSpentis queried explicitly;lockunspent), which purges the pending entry.In block-only mode (
-blocksonly=1) no CoinJoin maintenance is scheduled at all, since mixing needs transaction relay. The pending-observation check is therefore scheduled on its own there: the locks are persistent and are restored with the wallet, so without it 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.
getcoinjoininfogains apending_inputsfield for observability.How Has This Been Tested?
coinjoin_pending_observation_testscovers 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.!mempool.isSpent(outpoint)guard makes it fail (the input gets released), and restoring it makes it pass.coinjoin_tests,wallet_tests,walletdb_testspass.test/functional/rpc_coinjoin.py,wallet_basic.py,wallet_fundrawtransaction.py,p2p_blocksonly.pypass.On lock ordering:
CheckPendingObservationsholdscs_walletacrosschain().findCoins(), which takescs_main. That is the established direction here —FundTransaction(wallet/spend.cpp:1170, straight from upstream) and the pre-existing CoinJoin signing path (coinjoin/client.cpp:453) both do the same. There is nocs_main → cs_walletpath to invert against: no wallet function assertscs_main, and every validation-interface callback the wallet consumes is dispatched through the scheduler queue rather than synchronously undercs_main. Builds here are-DDEBUG_LOCKORDERandwallet_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.
getcoinjoininfogains 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 persistedlockedutxoentries it accompanies are understood by older clients regardless, so the inputs stay locked either way.Checklist:
🤖 Generated with Claude Code