Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/wallet/rpc/spend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,7 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
{"conf_target", UniValueType(UniValue::VNUM)},
{"estimate_mode", UniValueType(UniValue::VSTR)},
{"input_sizes", UniValueType(UniValue::VARR)},
{"use_cj", UniValueType(UniValue::VBOOL)},
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

use_cj becomes accepted in walletcreatefundedpsbt without being documented there.

Because FundTransaction() is shared, adding "use_cj" to this strict options schema also enables it for walletcreatefundedpsbt (which still doesn’t advertise it in its options docs). Please either document it there too or explicitly gate it per-RPC to avoid undocumented API behavior.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/wallet/rpc/spend.cpp` at 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.

},
true, true);

Expand Down Expand Up @@ -572,6 +573,10 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
if (options.exists("subtractFeeFromOutputs") || options.exists("subtract_fee_from_outputs") )
subtractFeeFromOutputs = (options.exists("subtract_fee_from_outputs") ? options["subtract_fee_from_outputs"] : options["subtractFeeFromOutputs"]).get_array();

if (options.exists("use_cj")) {
coinControl.UseCoinJoin(options["use_cj"].get_bool());
}
Comment on lines +576 to +578
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Reject non-mixed preset inputs when enabling use_cj

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 👍 / 👎.


SetFeeEstimateMode(wallet, coinControl, options["conf_target"], options["estimate_mode"], options["fee_rate"], override_min_fee);
}
} else {
Expand Down Expand Up @@ -753,6 +758,7 @@ RPCHelpMan fundrawtransaction()
},
},
},
{"use_cj", RPCArg::Type::BOOL, RPCArg::Default{false}, "Use CoinJoin funds only"},
},
FundTxDoc()),
"options"},
Expand Down Expand Up @@ -958,6 +964,7 @@ RPCHelpMan send()
{"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."},
},
},
{"use_cj", RPCArg::Type::BOOL, RPCArg::Default{false}, "Use CoinJoin funds only"},
},
FundTxDoc()),
"options"},
Expand Down Expand Up @@ -1064,6 +1071,7 @@ RPCHelpMan sendall()
{"lock_unspents", RPCArg::Type::BOOL, RPCArg::Default{false}, "Lock selected unspent outputs"},
{"psbt", RPCArg::Type::BOOL, RPCArg::DefaultHint{"automatic"}, "Always return a PSBT, implies add_to_wallet=false."},
{"send_max", RPCArg::Type::BOOL, RPCArg::Default{false}, "When true, only use UTXOs that can pay for their own fees to maximize the output amount. When 'false' (default), no UTXO is left behind. send_max is incompatible with providing specific inputs."},
{"use_cj", RPCArg::Type::BOOL, RPCArg::Default{false}, "Use CoinJoin funds only"},
},
FundTxDoc()
),
Expand Down Expand Up @@ -1138,6 +1146,10 @@ RPCHelpMan sendall()

coin_control.fAllowWatchOnly = ParseIncludeWatchonly(options["include_watching"], *pwallet);

if (options.exists("use_cj")) {
coin_control.UseCoinJoin(options["use_cj"].get_bool());
}
Comment on lines +1149 to +1151
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Enforce 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 👍 / 👎.

Comment on lines +1149 to +1151
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 30, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

sendall does not enforce use_cj when options.inputs is provided.

use_cj is set on coin_control, but explicit inputs bypass CoinJoin filtering in the options.inputs branch. With use_cj=true, non-mixed UTXOs can still be spent there.

Proposed fix
@@
             } else if (options.exists("inputs")) {
                 for (const CTxIn& input : rawTx.vin) {
+                    if (coin_control.IsUsingCoinJoin() && !pwallet->IsFullyMixed(input.prevout)) {
+                        throw JSONRPCError(RPC_INVALID_PARAMETER,
+                            strprintf("Input not available. UTXO (%s:%d) is not fully mixed while use_cj=true.",
+                                      input.prevout.hash.ToString(), input.prevout.n));
+                    }
                     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));
                     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/wallet/rpc/spend.cpp` around lines 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

no, it works as expected:

$ sendall '["y...]' null "unset" null '{"use_cj":true}'
Total value of UTXO pool too low to pay for transaction. Try using lower feerate or excluding uneconomic UTXOs with 'send_max' option. (code -6)
sendall '["y..."]'

{
  "txid": "XXX",
  "complete": true
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@knst, thanks for the clarification. Your test demonstrates that the non-explicit-inputs path correctly enforces use_cj by filtering through AvailableCoins with the coin control (returning an error when no mixed UTXOs are available).

My original concern was specifically about the narrower case where options.inputs is explicitly provided at the same time as use_cj=true — in that branch the inputs are iterated directly without going through AvailableCoins, so CoinJoin eligibility isn't validated. That said, you know the intended UX better than I do, and if you consider that edge case acceptable (e.g. the caller takes responsibility when specifying inputs manually), I'm happy to defer to your judgment.


🧠 Learnings used
Learnt from: d0wn3d
Repo: dashpay/dash PR: 7238
File: src/wallet/rpc/wallet.cpp:0-0
Timestamp: 2026-03-20T19:03:15.360Z
Learning: In Dash Core/DashPay code, RPCHelpMan does not provide the Bitcoin Core-style `Arg<T>(...)` template. When handling optional boolean RPC parameters, avoid `self.Arg<bool>(X)`. Instead, use the dashp2p/UniValue null-check pattern on the request param, e.g. `request.params[X].isNull() || request.params[X].get_bool()` (or `!request.params[X].isNull() && request.params[X].get_bool()` if the default semantics require it), so you correctly distinguish between an omitted/null value and an explicit boolean.

Learnt from: UdjinM6
Repo: dashpay/dash PR: 7265
File: src/wallet/wallet.cpp:3381-3382
Timestamp: 2026-04-03T14:17:17.656Z
Learning: In Dash Core, `strprintf()` uses `tinyformat` (type-safe printf). Do not flag format-specifier mismatches for integer types when using `strprintf()`: `%d` is correct for any integer type, including `CAmount`/`int64_t` arguments. Only treat `%d`/integer combinations as a formatting issue if they occur in a non-`strprintf()` (e.g., libc `printf`) context or there is evidence the format string is not being handled by `tinyformat`.


FeeCalculation fee_calc_out;
CFeeRate fee_rate{GetMinimumFeeRate(*pwallet, coin_control, &fee_calc_out)};
// Do not, ever, assume that it's fine to change the fee rate if the user has explicitly
Expand Down
Loading