Skip to content

fix(wallet): don't cache CoinJoin rounds for txes unknown to the wallet - #7636

Open
PastaPastaPasta wants to merge 1 commit into
dashpay:developfrom
PastaPastaPasta:fix-cj-rounds-cache-unknown-tx
Open

fix(wallet): don't cache CoinJoin rounds for txes unknown to the wallet#7636
PastaPastaPasta wants to merge 1 commit into
dashpay:developfrom
PastaPastaPasta:fix-cj-rounds-cache-unknown-tx

Conversation

@PastaPastaPasta

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

Follow-up to #7261. CWallet::GetRealOutpointCoinJoinRounds() memoizes its result in mapOutpointRoundsCache, including the -1 it returns for an outpoint whose transaction the wallet has no record of. Nothing invalidates that entry when the wallet later learns about the transaction — the only invalidation is ClearCoinJoinRoundsCache(), called from the coinjoinsalt RPCs.

This used to be unreachable in practice because every caller passed wallet-owned outpoints. Since #7261, fundrawtransaction, send and walletcreatefundedpsbt feed arbitrary user-supplied preset inputs into IsFullyMixed(), so querying an outpoint before the wallet knows its transaction (e.g. while a rescan is still running) permanently poisons the cache: once the wallet does add that transaction, a coin that is in fact a fully mixed denomination keeps reporting -1 rounds for the lifetime of the wallet in memory. It is then excluded from use_cj spends, missing from the anonymized balance in getbalances, and re-qualifies for mixing, so the client would re-mix (and pay fees on) an already-mixed coin.

What was done?

Don't memoize the unknown-transaction result: drop the just-inserted cache entry on that path and return -1 directly. The recursion in GetRealOutpointCoinJoinRounds() only descends into inputs for which InputIsMine() is true, which requires the previous transaction to be in mapWallet, so the directly queried outpoint is the only case that can be unknown — the emplace-based cycle protection for the recursive path is unaffected.

Added a unit regression test that queries an outpoint before the wallet knows its transaction (expects -1) and again after AddToWallet() (expects the real rounds). The test fails with -1 != 1 without the fix.

How Has This Been Tested?

  • New regression test in src/wallet/test/coinjoin_tests.cpp: ./src/test/test_dash --run_test=coinjoin_tests passes with the fix and fails without it.
  • test/functional/test_runner.py rpc_coinjoin.py wallet_send.py wallet_fundrawtransaction.py all pass locally (macOS arm64, --enable-debug).

Breaking Changes

None. The only behavior change is that an unknown outpoint is re-evaluated on subsequent queries instead of being permanently cached as having no rounds.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • 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)

GetRealOutpointCoinJoinRounds() memoizes its result in mapOutpointRoundsCache, including the -1 it returns for an outpoint whose transaction the wallet has no record of. Nothing invalidates that entry when the wallet later learns about the transaction (the only invalidation is ClearCoinJoinRoundsCache(), called from the coinjoinsalt RPCs), so the -1 sticks for the lifetime of the wallet in memory.

This used to be unreachable in practice because every caller passed wallet-owned outpoints. Since dash#7261, fundrawtransaction, send and walletcreatefundedpsbt feed arbitrary user-supplied preset inputs into IsFullyMixed(), so querying an outpoint before the wallet knows its transaction (e.g. while a rescan is still running) permanently marks it as having no rounds: a coin that is in fact a fully mixed denomination is then excluded from use_cj spends, missing from the anonymized balance and re-qualifies for mixing. Drop the cache entry on that path instead of memoizing it; the recursion only descends into IsMine inputs whose transactions are always present, so the directly queried outpoint is the only case that can be unknown.
@thepastaclaw

thepastaclaw commented Aug 23, 2026

Copy link
Copy Markdown

✅ Final review complete — no blockers (commit 5fa6b4a)

@coderabbitai

coderabbitai Bot commented Aug 23, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: aa93784f-114e-4b11-95b1-a5f742e98527

📥 Commits

Reviewing files that changed from the base of the PR and between d45a8be and 5fa6b4a.

📒 Files selected for processing (2)
  • src/wallet/coinjoin.cpp
  • src/wallet/test/coinjoin_tests.cpp

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


Walkthrough

GetRealOutpointCoinJoinRounds now removes the cache entry when the referenced transaction is unavailable before returning -1. A new test confirms that the same outpoint returns 1 after the transaction is added to the wallet.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 5fa6b

This change prevents unknown wallet transactions from being permanently treated as having no CoinJoin rounds and adds regression coverage for re-evaluation after the transaction is learned. No actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: udjinm6

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main wallet fix: avoiding cached CoinJoin rounds for transactions unknown to the wallet.
Description check ✅ Passed The description directly explains the cache defect, implemented fix, regression test, and validation results.
Docstring Coverage ✅ Passed Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Codex/Sol only (Phase 2 disabled)

The exact-head diff safely removes only the provisional cache entry for an unknown transaction, while preserving recursion protection for known wallet transactions. The regression test covers the stale-cache sequence, and ./src/test/test_dash --run_test=coinjoin_tests passed locally; no in-scope issue was confirmed.
Source: reviewer backends gpt-5.6-sol (general and dash-core-commit-history); final verifier backend gpt-5.6-sol; orchestration-only openclaw-agent/cliproxy/gpt-5.6-sol (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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants