feat: add use_cj feature to RPCs send, sendall and fundrawtransaction - #7261
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1160ac9529
ℹ️ 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".
| if (options.exists("use_cj")) { | ||
| coin_control.UseCoinJoin(options["use_cj"].get_bool()); | ||
| } |
There was a problem hiding this comment.
use_cj for manually specified sendall inputs
When sendall is called with both options.use_cj=true and options.inputs, this new flag does not actually constrain the provided inputs to fully mixed coins: the inputs branch later accepts any wallet UTXO and skips the AvailableCoins(..., &coin_control, ...) filtering path. In this scenario, callers can unintentionally spend non-CoinJoin funds despite opting into CoinJoin-only behavior, so this should either validate each specified input as fully mixed or reject the combination as incompatible.
Useful? React with 👍 / 👎.
|
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 (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThis change adds the Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change adds an optional CoinJoin funding flag to selected RPCs and has no actionable merge-blocking risk remaining beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant WalletRPC
participant FundTransaction
participant CoinControl
participant TransactionCommit
WalletRPC->>FundTransaction: pass use_cj
FundTransaction->>CoinControl: enable CoinJoin-only selection
CoinControl-->>FundTransaction: select fully mixed inputs
FundTransaction->>TransactionCommit: commit transaction with DS=1 metadata
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/rpc/spend.cpp`:
- Line 525: The shared options schema for FundTransaction() currently includes
the undocumented "use_cj" flag which unintentionally exposes it on
walletcreatefundedpsbt; either remove "use_cj" from the shared schema and add it
only to the RPCs that should accept it (e.g. include the {"use_cj", ...} entry
in the specific RPC handler's options instead of the common list used by
FundTransaction()), or keep it in the schema but add the corresponding
documentation for walletcreatefundedpsbt; locate the {"use_cj",
UniValueType(UniValue::VBOOL)} entry, the FundTransaction() call sites, and the
walletcreatefundedpsbt RPC handler and apply one of these changes so the option
is either documented or only accepted by the intended RPCs.
- Around line 1149-1151: In sendall, options.inputs bypass CoinJoin
restrictions: when coin_control.UseCoinJoin is true and options.inputs is
provided, validate and enforce CoinJoin eligibility on those explicit inputs
instead of accepting them unconditionally; specifically, in the branch handling
options.inputs check coin_control.UseCoinJoin() (or the internal flag set by
options["use_cj"]) and filter the provided inputs to only include UTXOs that
meet the wallet's CoinJoin/mixing eligibility (or return an error if any
provided input is not eligible), updating the inputs used for constructing the
transaction so CoinJoin rules are enforced.
🪄 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: 6c7262d7-30a0-4177-87ea-ca0b6144b1f2
📒 Files selected for processing (1)
src/wallet/rpc/spend.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1160ac9529
ℹ️ 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".
| if (options.exists("use_cj")) { | ||
| coinControl.UseCoinJoin(options["use_cj"].get_bool()); | ||
| } |
There was a problem hiding this comment.
Setting options.use_cj=true here is unsafe for fundrawtransaction (and send when add_inputs=true) if callers provide preselected non-mixed inputs: SelectCoins skips those preset inputs under ONLY_FULLY_MIXED (in src/wallet/spend.cpp), but the RPC funding flow later keeps the original tx.vin entries when merging the funded transaction, so those non-CoinJoin inputs are still spent while fee/change were computed as if they were absent. In that path users can unintentionally spend non-CoinJoin funds and overpay fees, so this option should reject incompatible preset inputs instead of silently proceeding.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I validated this PR directly from the checked-out diff because both ACP reviewer lanes stalled before producing any findings. The new use_cj plumbing is wired into the automatic coin-selection paths, but sendall has one behavioral hole: when callers also pass explicit inputs, the RPC accepts and spends them without checking whether they are actually mixed, so the newly documented privacy constraint is silently bypassed.
Reviewed commit: 1160ac9
🔴 1 blocking
1 additional finding
🔴 blocking: `sendall` ignores `use_cj=true` when the caller provides explicit inputs
src/wallet/rpc/spend.cpp (lines 1172-1182)
This new option only affects the automatic branch at line 1184, where AvailableCoins(*pwallet, &coin_control, fee_rate, ...) filters the wallet UTXO set using coin_control.nCoinType. In the explicit-input branch above, the code just validates ownership/spentness and sums the values, so sendall(..., {"inputs": [...], "use_cj": true}) will still spend non-mixed wallet UTXOs. That makes the newly advertised "Use CoinJoin funds only" flag a no-op for one of the supported call patterns and can leak unmixed coins in exactly the RPC the PR is extending. The branch needs to reject non-mixed selected inputs (or declare use_cj incompatible with inputs) instead of silently bypassing the mixed-only requirement.
💡 Suggested change
} else if (options.exists("inputs")) {
for (const CTxIn& input : rawTx.vin) {
if (pwallet->IsSpent(input.prevout)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Input not available. UTXO (%s:%d) was already spent.", input.prevout.hash.ToString(), input.prevout.n));
}
const CWalletTx* tx{pwallet->GetWalletTx(input.prevout.hash)};
if (!tx || input.prevout.n >= tx->tx->vout.size() || !(pwallet->IsMine(tx->tx->vout[input.prevout.n]) & (coin_control.fAllowWatchOnly ? ISMINE_ALL : ISMINE_SPENDABLE))) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Input not found. UTXO (%s:%d) is not part of wallet.", input.prevout.hash.ToString(), input.prevout.n));
}
if (coin_control.IsUsingCoinJoin() && !pwallet->IsFullyMixed(input.prevout)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Input not available. UTXO (%s:%d) is not fully mixed.", input.prevout.hash.ToString(), input.prevout.n));
}
total_input_value += tx->tx->vout[input.prevout.n].nValue;
}
🤖 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/rpc/spend.cpp`:
- [BLOCKING] lines 1172-1182: `sendall` ignores `use_cj=true` when the caller provides explicit inputs
This new option only affects the automatic branch at line 1184, where `AvailableCoins(*pwallet, &coin_control, fee_rate, ...)` filters the wallet UTXO set using `coin_control.nCoinType`. In the explicit-input branch above, the code just validates ownership/spentness and sums the values, so `sendall(..., {"inputs": [...], "use_cj": true})` will still spend non-mixed wallet UTXOs. That makes the newly advertised "Use CoinJoin funds only" flag a no-op for one of the supported call patterns and can leak unmixed coins in exactly the RPC the PR is extending. The branch needs to reject non-mixed selected inputs (or declare `use_cj` incompatible with `inputs`) instead of silently bypassing the mixed-only requirement.
|
✅ Final review complete — no blockers (commit 99a818c) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Three convergent issues confirmed against the head commit. The blocking bug — sendall's explicit-input branch never enforces CoinJoin-only selection — is real: the loop at lines 1172-1182 only checks IsSpent/IsMine and never consults coin_control.IsUsingCoinJoin() or IsFullyMixed, while the auto-selection branch correctly funnels through AvailableCoins/SelectCoins. The DS=1 wallet marker is also genuinely missing from the new send/sendall commit path (FinishTransaction calls CommitTransaction with an empty mapValue, while the legacy SendMoney path sets it). And the PR ships with no automated coverage at all.
Reviewed commit: 1160ac9
Fresh dispatcher run for this queue item. A same-SHA review already existed, so this records the fresh verification without duplicating inline threads.
Code Review
Reviewed commit: 1160ac9
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
🤖 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/rpc/spend.cpp`:
- [BLOCKING] lines 1172-1182: `sendall` does not enforce CoinJoin-only spending when explicit `inputs` are supplied
`sendall` enables CoinJoin-only selection by calling `coin_control.UseCoinJoin(options["use_cj"].get_bool())` at lines 1149-1151, which sets `nCoinType = CoinType::ONLY_FULLY_MIXED`. The auto-selection branch at line 1184 then funnels through `AvailableCoins(*pwallet, &coin_control, ...)`, and `SelectCoins` drops non-mixed preset inputs at `src/wallet/spend.cpp:548-552`. However, the explicit-input branch at lines 1172-1182 hand-builds the input set using only `IsSpent`/`IsMine`/`fAllowWatchOnly` checks — it never consults `coin_control.IsUsingCoinJoin()` or `wallet.IsFullyMixed(input.prevout)`. A caller that passes non-mixed UTXOs together with `use_cj=true` will have those non-mixed coins spent silently, defeating the privacy guarantee documented as "Use CoinJoin funds only". Reject `use_cj=true` combined with non-mixed explicit `inputs` by validating each input via `wallet.IsFullyMixed`.
- [SUGGESTION] lines 106-115: `send`/`sendall` CoinJoin spends are committed without the existing `DS=1` wallet marker
`SendMoney()` (used by `sendtoaddress`/`sendmany`) sets `map_value["DS"] = "1"` at line 154 whenever `coin_control.IsUsingCoinJoin()` is true, so `CommitTransaction` records the outgoing tx in wallet history as a CoinJoin send. The new `use_cj` paths in `send` (line 1022) and `sendall` (line 1263) both go through `FinishTransaction()`, which commits with an empty `mapValue` at line 111: `pwallet->CommitTransaction(tx, {}, /*orderForm*/ {})`. Downstream classification in `src/wallet/rpc/transactions.cpp:355` and `src/qt/transactionrecord.cpp:153,271` keys on the `DS` marker to distinguish CoinJoin sends from ordinary sends, so spends made via these new paths will be misclassified in `listtransactions` and the Qt UI. `fundrawtransaction` is unaffected since it does not commit. Plumb a `mapValue_t` with `DS=1` into `FinishTransaction` when `coin_control.IsUsingCoinJoin()` is set.
- [SUGGESTION] lines 522-1151: No automated tests added for `use_cj` on `send`/`sendall`/`fundrawtransaction`
The PR adds a new user-visible option that alters coin selection semantics on three RPCs but touches no unit or functional tests. The author-supplied manual test covers only `send`'s auto-selection happy path. Cases worth covering: (1) `sendall` with explicit `inputs` + `use_cj=true` (the blocking finding above), (2) `send`/`fundrawtransaction` with non-mixed explicit `inputs` + `use_cj=true` — `SelectCoins` silently drops them at `src/wallet/spend.cpp:548-552`, (3) `use_cj=true` when the wallet has zero fully-mixed denominations (clear error expected), (4) interactions with `add_inputs`, `send_max`, `include_watching`, and the PSBT path (`psbt=true`, `add_to_wallet=false`). A functional test modeled on `wallet_sendall.py` / `feature_coinjoin.py` would have caught the explicit-input regression immediately. Per CLAUDE.md, new Dash-specific features should have functional tests where applicable.
1160ac9 to
04bb8b0
Compare
04bb8b0 to
99a818c
Compare
src/wallet/rpc/spend.cpp (lines 1172-1182) This new option only affects the automatic branch at line 1184, where AvailableCoins(*pwallet, &coin_control, fee_rate, ...) filters the wallet UTXO set using coin_control.nCoinType. In the explicit-input branch above, the code just validates ownership/spentness and sums the values, so sendall(..., {"inputs": [...], "use_cj": true}) will still spend non-mixed wallet UTXOs. That makes the newly advertised "Use CoinJoin funds only" flag a no-op for one of the supported call patterns and can leak unmixed coins in exactly the RPC the PR is extending. The branch needs to reject non-mixed selected inputs (or declare use_cj incompatible with inputs) instead of silently bypassing the mixed-only requirement. @thepastaclaw |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact head correctly enforces mixed-only selection for automatic and preset inputs, documents the shared walletcreatefundedpsbt support, preserves the DS=1 wallet marker, and adds functional coverage; the local rpc_coinjoin.py run passed. The only remaining in-scope issue is a commit-history nitpick: the subject enumerates three RPCs while the commit deliberately exposes, documents, and tests a fourth.
Source: Codex reviewer (exact backend model ID was not supplied in the evidence); Claude Agent SDK verifier (exact backend model ID was not exposed in the runtime context); 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)
💬 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 `<commit:99a818c>`:
- [NITPICK] <commit:99a818c>:1: Commit subject omits `walletcreatefundedpsbt`
The subject explicitly lists `send`, `sendall`, and `fundrawtransaction`, but this commit also exposes, documents, and tests `use_cj` for `walletcreatefundedpsbt`. Reword the subject to include all four RPCs or use a complete summary such as `feat: add use_cj to transaction funding RPCs` so the permanent history accurately describes the change.
2821f84 test: extract CoinJoin mixing helper (pasta) 1cb2b49 test: cover use_cj success path in rpc_coinjoin.py (pasta) Pull request description: ## Issue being fixed or feature implemented Follow-up to #7261. The `use_cj` tests added there only exercise failure paths: the test wallet never holds a fully mixed coin, so nothing verifies that a successful `use_cj` spend actually selects mixed inputs, suppresses change, or records the `DS="1"` CoinJoin marker that `FinishTransaction()` now attaches via `CommitTransaction()`. A regression in any of those would pass the suite unnoticed. ## What was done? Added a success-path subtest to `test/functional/rpc_coinjoin.py`. It builds two fully mixed coins by simulating mixing: same-denomination, fee-free self-spends advance a denominated output by one round each, and they are mined directly via `generateblock` since zero-fee transactions are not relayed. Chaining `COINJOIN_ROUNDS_MIN + COINJOIN_RANDOM_ROUNDS` rounds makes `IsFullyMixed()` deterministic regardless of the wallet's salt (the salt-based coin flip only applies strictly below that threshold). It then asserts: - both coins report full `coinjoin_rounds` in `listunspent` and count towards the `getbalances` anonymized balance; - `send` accepts a fully mixed preset input (the positive counterpart of the rejection tests from #7261), spends exactly that input, creates no change output (the remainder is paid as fee, as fully mixed coins are spent in whole denominations) and records `DS="1"`; - `sendall` with automatic selection sweeps exactly the remaining fully mixed coin and records `DS="1"`. `setcoinjoinrounds` is a node-global setting; the subtest pins it to `COINJOIN_ROUNDS_MIN` and restores the default at the end so later subtests are unaffected. ## How Has This Been Tested? `test/functional/test_runner.py rpc_coinjoin.py` passes locally (macOS arm64, `--enable-debug` build, both descriptor and legacy wallet paths of the runner's default). `test/lint/lint-python.py` clean. ## Breaking Changes None, test-only change. ## 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)_ Top commit has no ACKs. Tree-SHA512: dc6a730ae9bac1a8b69abc3b051e12f3d4b289ceacda7c4048b440611fd9c90acc0e3dffce94db8cf4611b63ea3000a48e1da6751274634572f8ee606a32a274
…n to the wallet 5fa6b4a fix(wallet): don't cache CoinJoin rounds for txes unknown to the wallet (pasta) Pull request description: ## 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: - [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)_ Top commit has no ACKs. Tree-SHA512: 297e018459b6b129e8c59df1e85f2b6d7f6d7d07df8744658b1f3a50906a6af901c2b05a22c6f860bc3aacc906b6eabb3336f3d39a2caf914d1fc97708527ba3
Issue being fixed or feature implemented
The RPCs
send,sendall,fundrawtransactiondoesn't have a functionality to use cj to fund transaction.What was done?
Added flag use_cj to the options and use it coin-select.
How Has This Been Tested?
Call RPC
send '[{"yRgziYdxyApbFsvph89JAVMzjsv8en16bA":0.01}]' null "unset" null '{"use_cj":true}'It produced tx
getrawtransaction 8df1998514388b9c19705077e743c5bc2fd6dd378a6287cd865f0a1b91e8afaf 1which looks as expected.Breaking Changes
N/A
Checklist: