wallet: reject unknown options in sendall #36494

thomasbuilds commented at 2:01 PM on October 10, 2026: contributor

sendall silently ignores unknown keys in its options object, including misspelled ones: input instead of inputs spends all available UTXOs rather than the given ones, and a misspelled add_to_wallet or psbt can broadcast a transaction the caller asked not to broadcast.

Reject unknown options, like send, fundrawtransaction, walletcreatefundedpsbt and bumpfee already do.

DrahtBot added the label Wallet on Oct 10, 2026

DrahtBot commented at 2:01 PM on October 10, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36494
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36494.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36494#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	maflcko
Concept NACK	polespinasa
Concept ACK	Conradgabe, vicjuma

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35302 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35302.svg"></sub> (Silent Payments: Sending (take 2) by Eunovo)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

kevkevinpal commented at 5:11 PM on October 10, 2026: contributor

This should have a release note if sendall now rejects unknown options

thomasbuilds referenced this in commit 4a65a755b0 on Oct 10, 2026

thomasbuilds force-pushed on Oct 10, 2026

Conradgabe commented at 10:57 AM on October 13, 2026: none

Concept ACK

vicjuma commented at 2:17 PM on October 14, 2026: contributor

Concept ACK

in src/wallet/rpc/spend.cpp:1388 in 4a65a755b0 outdated

1383 | @@ -1384,8 +1384,26 @@ RPCMethod sendall()
1384 |              pwallet->BlockUntilSyncedToCurrentChain();
1385 |  
1386 |              UniValue options{request.params[4].isNull() ? UniValue::VOBJ : request.params[4]};
1387 | -            InterpretFeeEstimationInstructions(/*conf_target=*/request.params[1], /*estimate_mode=*/request.params[2], /*fee_rate=*/request.params[3], options);
1388 |              PreventOutdatedOptions(options);
1389 | +            RPCTypeCheckObj(options,

vicjuma commented at 2:18 PM on October 14, 2026:

Works as expected

Before:

After:

in doc/release-notes-36494.md:5 in 4a65a755b0

   0 | @@ -0,0 +1,11 @@
   1 | +Wallet
   2 | +------
   3 | +
   4 | +- `sendall` now rejects unknown keys in its `options` object, like `send`,
   5 | +  `fundrawtransaction` and `walletcreatefundedpsbt` already do. Misspelled

vicjuma commented at 2:20 PM on October 14, 2026:

Misspelled options and options of send that sendall does not support, such as subtract_fee_from_outputs

Minor nit: unknown (or unsupported) options seem enough, probably?

polespinasa commented at 6:33 AM on October 15, 2026: member

concept ACK on adding the options check.

NACK on removing solving_data. Although it is not used it is a breaking change that should be documented and follow the deprecation process. See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#feature-deprecation-and-removal-process

thomasbuilds referenced this in commit cb00b6e670 on Oct 16, 2026

thomasbuilds force-pushed on Oct 16, 2026

thomasbuilds commented at 9:15 AM on October 16, 2026: contributor

@polespinasa dropped the solving_data removal.

in src/wallet/rpc/spend.cpp:1387 in cb00b6e670

1383 | @@ -1384,8 +1384,27 @@ RPCMethod sendall()
1384 |              pwallet->BlockUntilSyncedToCurrentChain();
1385 |  
1386 |              UniValue options{request.params[4].isNull() ? UniValue::VOBJ : request.params[4]};
1387 | -            InterpretFeeEstimationInstructions(/*conf_target=*/request.params[1], /*estimate_mode=*/request.params[2], /*fee_rate=*/request.params[3], options);
1388 |              PreventOutdatedOptions(options);

maflcko commented at 9:35 AM on October 16, 2026:

This call can be removed now? Also, what about send?

maflcko commented at 9:35 AM on October 16, 2026:

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

thomasbuilds commented at 12:21 PM on October 16, 2026:

Removed and squashed - send still needs it since FundTransaction accepts the camelCase names for fundrawtransaction

maflcko commented at 1:26 PM on October 16, 2026:

Removed and squashed - send still needs it since FundTransaction accepts the camelCase names for fundrawtransaction

huh, PreventOutdatedOptions is called in send and it literally rejects all camelCase?

Using LLMs is fine, but you'll have to read and understand all code yourself as well.

thomasbuilds commented at 1:39 PM on October 16, 2026:

I meant the call can't be dropped from send like it was from sendall. send's options go through the RPCTypeCheckObj in FundTransaction, which allows the camelCase names PreventOutdatedOptions checks for, since they are fundrawtransaction options. So in send that call is what rejects them. The new list in sendall doesn't have them so the call was redundant there.

maflcko commented at 1:49 PM on October 16, 2026:

ok, hmm. I was thinking to add the type and field checks right in the RPC that needs them, not through some deeper calls, which accept camelCase, and then some more extra calls which reject it again.

But maybe this would be too much of a refactor. So seems fine to leave as-is for now.

I had a branch to remove all of this code anyway, but it was in tmpfs and I poured tea over my machine and lost it. Maybe I'll try to re-create it after #36489 is merged.

thomasbuilds force-pushed on Oct 16, 2026

DrahtBot added the label CI failed on Oct 16, 2026

DrahtBot removed the label CI failed on Oct 16, 2026

in test/functional/wallet_sendall.py:306 in b688b6dcdc

 301 | +    def sendall_accepts_documented_options(self):
 302 | +        self.log.info("Test sendall accepts documented options")
 303 | +        self.add_utxos([15, 6])
 304 | +        self.test_sendall_success(sendall_args=[self.remainder_target], options={"include_watching": False, "fee_rate": 10, "solving_data": {}})
 305 | +        self.add_utxos([15, 6])
 306 | +        self.test_sendall_success(sendall_args=[self.remainder_target], options={"conf_target": 6, "estimate_mode": "economical"})

maflcko commented at 2:01 PM on October 16, 2026:

I am not sure how much value there is in adding dummy tests that just check success without also checking that the conf target, estimate mode, etc are really honoured.

It would be better to remove them to avoid a false sense of coverage.

in test/functional/wallet_sendall.py:298 in b688b6dcdc outdated

 293 | +        assert_raises_rpc_error(-3, "Unexpected key feeRate",
 294 | +                self.wallet.sendall, recipients=[self.remainder_target], options={"feeRate": 0.0001})
 295 | +        assert_raises_rpc_error(-3, "Unexpected key estimate_mde",
 296 | +                self.wallet.sendall, recipients=[self.remainder_target], options={"conf_target": 6, "estimate_mde": "economical"})
 297 | +        assert_raises_rpc_error(-3, "JSON value of type string for field add_to_wallet is not of expected type bool",
 298 | +                self.wallet.sendall, recipients=[self.remainder_target], options={"add_to_wallet": "false"})

maflcko commented at 2:04 PM on October 16, 2026:

not sure about such a large test. This can be made smaller:

diff --git a/test/functional/wallet_sendall.py b/test/functional/wallet_sendall.py
index f0faa9a85d..3724237513 100755
--- a/test/functional/wallet_sendall.py
+++ b/test/functional/wallet_sendall.py
@@ -285,15 +285,8 @@ class SendallTest(BitcoinTestFramework): [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)
     def sendall_fails_on_invalid_options(self):
         self.log.info("Test sendall fails on unknown options or options of the wrong type")
-        self.add_utxos([15, 6])
-        utxo = self.wallet.listunspent()[0]
-
-        assert_raises_rpc_error(-3, "Unexpected key input",
-                self.wallet.sendall, recipients=[self.remainder_target], options={"input": [utxo]})
-        assert_raises_rpc_error(-3, "Unexpected key feeRate",
-                self.wallet.sendall, recipients=[self.remainder_target], options={"feeRate": 0.0001})
-        assert_raises_rpc_error(-3, "Unexpected key estimate_mde",
-                self.wallet.sendall, recipients=[self.remainder_target], options={"conf_target": 6, "estimate_mde": "economical"})
+        assert_raises_rpc_error(-3, "Unexpected key add_to_wllt",
+                self.wallet.sendall, recipients=[self.remainder_target], options={"add_to_wllt": "false"})
         assert_raises_rpc_error(-3, "JSON value of type string for field add_to_wallet is not of expected type bool",
                 self.wallet.sendall, recipients=[self.remainder_target], options={"add_to_wallet": "false"})

thomasbuilds commented at 2:18 PM on October 16, 2026:

done ty

maflcko approved

maflcko commented at 2:05 PM on October 16, 2026: member

lgtm, makes sense to reject unknown fields explicitly instead of silently ignoring them

review ACK b688b6dcdc22222d9208b1e28adfe0beab61bb09 🖇

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK b688b6dcdc22222d9208b1e28adfe0beab61bb09 🖇
MUNdQAsCiZLs9ChYoHySAuwH9oKHch6PED2ZdR87D4HxI6uMdgg/JWKDI2t709O8n+CWerUTBWq5FLqjEe5aBw==

</details>

DrahtBot requested review from vicjuma on Oct 16, 2026

wallet: reject unknown options in sendall

Unknown keys in the options object were silently ignored, so a typo
such as "input" for "inputs" spent all available UTXOs rather than the
given ones. The options are checked before the fee arguments are merged
into them, so that a misspelled estimate_mode is reported as an
unexpected key. The check also rejects the outdated camelCase names, so
sendall no longer needs PreventOutdatedOptions.

702ec9d40f

thomasbuilds force-pushed on Oct 16, 2026

maflcko commented at 2:19 PM on October 16, 2026: member

only change is slimming down the test

review ACK 702ec9d40f84dfd0ca9bf73e37052c36f3802702 🐀

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 702ec9d40f84dfd0ca9bf73e37052c36f3802702 🐀
ZIsQXc3hPnteVdmhWFAzzktVar1SyUDVNj8lE0Qq3N6nbWGFfqfsstktLDuMykhI4c1yMR+c5EZdy6EOkA/qBQ==

</details>