fix(wallet): exclude unconfirmable outputs from CoinJoin accounting - #7634
fix(wallet): exclude unconfirmable outputs from CoinJoin accounting#7634UdjinM6 wants to merge 4 commits into
Conversation
setWalletUTXO holds every unspent output the wallet owns, including outputs of transactions that cannot confirm as they stand: conflicted ones, and ones that were abandoned, never broadcast, or rejected from the mempool. AvailableCoins() filters those out, but the CoinJoin functions that read setWalletUTXO directly checked at most `depth < 0`, so such outputs were counted as spendable wallet funds. CountInputsWithAmount() feeds CCoinJoinClientSession::CreateDenominated(), which then under-creates denominations, and GetAverageAnonymizedRounds() and GetNormalizedAnonymizedBalance() surface the same skew in the GUI. Give all four direct readers of setWalletUTXO the liveness filter AvailableCoins() uses, via IsWalletUTXOSpendable(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe wallet adds Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change excludes unconfirmable outputs from CoinJoin accounting, keeping wallet mixing counts aligned with spendable funds. No actionable merge-blocking risk remains after normal checks and review. 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 |
|
✅ Final review complete — no blockers (commit 6b8b0b8) |
![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: f44af84fe9
ℹ️ 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".
| CAmount nValue = it->second.tx->vout[outpoint.n].nValue; | ||
| if (!CoinJoin::IsDenominatedAmount(nValue)) continue; | ||
| if (GetTxDepthInMainChain(it->second) < 0) continue; | ||
| if (!IsWalletUTXOSpendable(it->second)) continue; |
There was a problem hiding this comment.
Filtering only the normalized/rounds paths leaves the primary CoinJoin balances unchanged: GetBalance() still calls CachedTxGetAvailableCoinJoinCredits(), whose nDepth < 0 guard accepts an inactive depth-zero transaction and then classifies its denominations as denominated_trusted; CachedTxGetAnonymizedCredit() has the same gap for CoinJoin-only spending. Thus the transaction reproduced by the new test still inflates getbalances.mine.coinjoin, coinjoin_balance, the Qt denomination/anonymized figures, and—if its output has enough rounds—the CoinJoin send balance, while this newly filtered normalized value excludes it. Apply the same inactive/non-mempool rejection in both aggregate credit functions.
Useful? React with 👍 / 👎.
| if (wtx.IsCoinBase() && GetTxBlocksToMaturity(wtx) > 0) continue; | ||
| if (fSkipUnconfirmed && !CachedTxIsTrusted(*this, wtx)) continue; | ||
| if (GetTxDepthInMainChain(wtx) < 0) continue; | ||
| if (!IsWalletUTXOSpendable(wtx)) continue; |
There was a problem hiding this comment.
When a trusted wallet transaction is in the mempool, the default GetAnonymizableBalance() path can cache it before reaching this check on later calls. For non-conflict mempool removals, transactionRemovedFromMempool() only calls RefreshMempoolStatus() and does not clear fAnonymizableTallyCached or fAnonymizableTallyCachedNonDenom, so after eviction/expiry this function returns the cached tally at lines 137–147 without evaluating the new liveness condition. Automatic CoinJoin accounting can therefore continue using the unconfirmable outputs until an unrelated cache reset; invalidate these caches on the in-mempool-to-inactive transition or avoid caching zero-depth entries.
Useful? React with 👍 / 👎.
CachedTxGetAvailableCoinJoinCredits() and CachedTxGetAnonymizedCredit() value a transaction's outputs for GetBalance() and the Qt figures, and both rejected only conflicted transactions. An inactive transaction that never reached the mempool therefore still inflated getbalances.mine.coinjoin and the denominated and anonymized balances, while the counts and the normalized balance no longer counted it — the two disagreed about the same output. Reuse IsWalletUTXOSpendable() there as well, so everything that values wallet outputs applies the same rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The liveness predicate is correctly applied to the direct CoinJoin UTXO readers and aggregate credit functions, but the anonymizable tally caches can retain a trusted transaction after it leaves the mempool, temporarily restoring the accounting error this PR intends to fix. This leaves one in-scope blocking correctness issue.
Source: reviewers gpt-5.6-sol (general and dash-core-commit-history); final verifier gpt-5.6-sol. Orchestration only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Validated blockers were found in the Codex precheck. Opus 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/wallet/coinjoin.cpp`:
- [BLOCKING] src/wallet/coinjoin.cpp:165: Invalidate cached tallies when a transaction leaves the mempool
`SelectCoinsGroupedByAddresses()` returns `vecAnonymizableTallyCached` or `vecAnonymizableTallyCachedNonDenom` at lines 137-147 before reaching this new liveness predicate. A wallet-created zero-depth transaction can pass `CachedTxIsTrusted()` while it is in the mempool and populate either cache at lines 214-220. On non-conflict removals such as `EXPIRY`, `SIZELIMIT`, or `MANUAL`, `CWallet::transactionRemovedFromMempool()` changes that transaction from `TxStateInMempool` to `TxStateInactive` through `RefreshMempoolStatus()`, but does not clear either cache flag. Subsequent `GetAnonymizableBalance()` calls therefore continue counting the now-unconfirmable outputs until a block or unrelated wallet event invalidates the cache. Clear both tally caches on this mempool-state transition and add a regression test that primes the cache before removing the transaction from the mempool.
| if (wtx.IsCoinBase() && GetTxBlocksToMaturity(wtx) > 0) continue; | ||
| if (fSkipUnconfirmed && !CachedTxIsTrusted(*this, wtx)) continue; | ||
| if (GetTxDepthInMainChain(wtx) < 0) continue; | ||
| if (!IsWalletUTXOSpendable(wtx)) continue; |
There was a problem hiding this comment.
🔴 Blocking: Invalidate cached tallies when a transaction leaves the mempool
SelectCoinsGroupedByAddresses() returns vecAnonymizableTallyCached or vecAnonymizableTallyCachedNonDenom at lines 137-147 before reaching this new liveness predicate. A wallet-created zero-depth transaction can pass CachedTxIsTrusted() while it is in the mempool and populate either cache at lines 214-220. On non-conflict removals such as EXPIRY, SIZELIMIT, or MANUAL, CWallet::transactionRemovedFromMempool() changes that transaction from TxStateInMempool to TxStateInactive through RefreshMempoolStatus(), but does not clear either cache flag. Subsequent GetAnonymizableBalance() calls therefore continue counting the now-unconfirmable outputs until a block or unrelated wallet event invalidates the cache. Clear both tally caches on this mempool-state transition and add a regression test that primes the cache before removing the transaction from the mempool.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Invalidate cached tallies when a transaction leaves the mempool 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.
SelectCoinsGroupedByAddresses() serves its result from vecAnonymizableTallyCached before it looks at any transaction, so the liveness filter never runs on a cached call. A wallet transaction is trusted at depth zero while it sits in the mempool, so its outputs enter that cache; when it later leaves the mempool for any reason other than a conflict, transactionRemovedFromMempool() refreshes its state but left both cache flags set. The tally then kept handing out outputs of a transaction that can no longer confirm as it stands, until some unrelated event cleared the cache. Clear both flags on that transition, as the conflict path already does through SyncTransaction(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At the exact head, the prior cache-invalidation blocker is fixed: non-conflict mempool removal clears both anonymizable tally caches, and the regression test primes the cache before verifying removal. The liveness rule now covers both direct UTXO readers and aggregate CoinJoin balance paths, with no in-scope findings remaining; the proposed squash is not required because each implementation commit is coherent and independently explains its change.
Source: reviewers gpt-5.6-sol (general and dash-core-commit-history); final verifier claude-opus-4-6; openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
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/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
2 participants
Issue being fixed or feature implemented
setWalletUTXOholds every unspent output the wallet owns, including outputs of transactions that cannot confirm as they stand: conflicted ones, and ones that were abandoned, never broadcast, or rejected from the mempool.AvailableCoins()filters those out — it skipsdepth < 0, anddepth == 0 && !wtx.InMempool()— but the CoinJoin functions that readsetWalletUTXOdirectly checked at mostdepth < 0, andGetAverageAnonymizedRounds()checked nothing at all. Outputs that will never exist were therefore counted as spendable wallet funds.The consequences are all in CoinJoin:
CountInputsWithAmount()is whatCCoinJoinClientSession::CreateDenominated()uses to cap how many outputs of each denomination to create. Counting phantom denominations makes the wallet create fewer real ones than the user asked for.GetAverageAnonymizedRounds()andGetNormalizedAnonymizedBalance()feed the mixing figures shown on the Qt overview page, so the reported progress does not match the coins the wallet can actually use.SelectCoinsGroupedByAddresses()tallies candidates for mixing from the same set.To reproduce: create a wallet transaction paying yourself a denomination and never broadcast it (or let the mempool reject it). Its outputs are counted immediately, before it can possibly confirm, and stay counted.
What was done?
Added
CWallet::IsWalletUTXOSpendable(), which expresses the same liveness ruleAvailableCoins()applies — reject conflicted transactions, and reject zero-depth transactions that are not in the mempool — and applied it to all four direct readers ofsetWalletUTXOinsrc/wallet/coinjoin.cpp.Applying it to all four rather than only the miscounting one is deliberate: they read the same set for the same purpose, and filtering some but not others would be arbitrary.
No consensus, network or serialization code is touched.
How Has This Been Tested?
New unit test
availablecoins_tests/UnconfirmableOutputsAreNotWalletFunds: a transaction the wallet knows about but that never reached the mempool has its outputs excluded, and the same transaction inTxStateInMempoolhas them counted. The test was confirmed to be a genuine regression test by reverting the filter back toGetTxDepthInMainChain(...) < 0and observing it fail with1 != 0.Ran
availablecoins_tests,coinjoin_tests,coinjoin_inouts_tests,wallet_testsandspend_tests— all pass. Built with the full tree including Qt on macOS (aarch64-apple-darwin), no new warnings.Breaking Changes
None to any API, RPC or wallet format.
There is an intentional behaviour change inside CoinJoin: outputs of transactions that are conflicted, or that sit at depth 0 outside the mempool, no longer contribute to denomination counts, average rounds or the normalized anonymized balance. Newly created denominations still count as soon as they reach the mempool, so mixing progresses as before; only outputs that cannot be spent stop being counted.
Checklist: