#36494 wallet: reject unknown options in sendall
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36494 wallet: reject unknown options in sendall | Ready | 2 -1 | Mild | 26 + 11 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Validates the options object in the sendall RPC using strict type checking, rejecting unknown keys and verifying expected types. It runs the check before fee arguments are merged into options, superseding PreventOutdatedOptions.
Problem: sendall silently ignored unrecognized keys in its options parameter. Callers making typos could trigger unintended behavior, such as spending every wallet UTXO instead of a specified subset if inputs is misspelled as input, or immediately broadcasting a transaction if add_to_wallet or psbt is misspelled.
Evidence of importance:
- PR description: '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.'
- Commit message: '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.'
Risk of a mistake: medium (rpc_api, wallet). Any valid sendall option mistakenly omitted from the schema would cause RPC calls using that option to unexpectedly fail.
Changed since the previous assessment: Retained the solving_data option in the allowed options list instead of removing it.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- nothing: nothing pending
Agreement
Mild
NACK from polespinasa not withdrawn (Removing solving_data is an undocumented breaking change that violates the deprecation process)
Thread notes: Added Tested ACK from Zippo76 with functional testing on macOS/regtest and code review.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| kevkevinpal (contributor) | suggestion, maintenance | Add a release note for rejecting unknown options in sendall | open | answered | no | 2026-10-10: 'This should have a release note if `sendall` now rejects unknown options' |
| vicjuma (contributor) | suggestion, style | Simplify release note phrasing to unknown or unsupported options | open | answered | no | 2026-10-14: 'Minor nit: `unknown` (or `unsupported`) options seem enough, probably?' |
| polespinasa (member) | objection, interface | Removing solving_data is an undocumented breaking change that violates the deprecation process | fixed · fix pushed · dropped solving_data removal in a push | changed | yes | 2026-10-15: 'NACK on removing `solving_data`. Although it is not used it is a breaking change that should be documented and follow the deprecation process.' Status from: 2026-10-16: thomasbuilds: 'dropped the `solving_data` removal.' |
| maflcko (member) | question, maintenance | Whether PreventOutdatedOptions call can be removed now, and what about send | accepted · author removed the call from sendall and explained send; reviewer accepted leaving send as-is | answered | yes | 2026-10-16: 'This call can be removed now? Also, what about `send`?' Status from: 2026-10-16: maflcko: 'seems fine to leave as-is for now.' |
| maflcko (member) | suggestion, maintenance | Remove dummy tests that do not check if options are honoured to avoid false sense of coverage | fixed · fix pushed · author slimmed down the test suite and reviewer approved | answered | yes | 2026-10-16: 'It would be better to remove them to avoid a false sense of coverage.' Status from: 2026-10-16: maflcko: 'only change is slimming down the test' |
| maflcko (member) | suggestion, maintenance | Make the invalid options test smaller | fixed · fix pushed · author applied suggested diff and reviewer confirmed | answered | yes | 2026-10-16: 'not sure about such a large test. This can be made smaller:' Status from: 2026-10-16: maflcko: 'only change is slimming down the test' |
Support:
- vicjuma (contributor) (Concept ACK, 2026-10-14): (no reason given) [not substantive]
- maflcko (member) (ACK, 2026-10-16) [read]: only change is slimming down the test [not substantive]
- maflcko (member) (ACK, 2026-10-16) [read]: makes sense to reject unknown fields explicitly instead of silently ignoring them
- Zippo76 (none) (Tested ACK, 2026-10-29) [both: wallet_sendall.py]: The new case in wallet_sendall.py fails on master, where the unknown option is ignored and the call reaches coin selection; it passes with this PR. Manually checked options={"add_to_wllt": false} in regtest: on master, the transaction was accepted into the mempool. With this PR, the same call return
Participants:
- kevkevinpal (contributor, neutral, 1 statements 2026-10-10 to 2026-10-10): suggested adding a release note
- Conradgabe (none, support, 1 statements 2026-10-13 to 2026-10-13): Concept ACK without rationale
- vicjuma (contributor, support, 3 statements 2026-10-14 to 2026-10-14): Concept ACK, tested the change, and suggested a nit on the release note wording
- polespinasa (member, objection, 1 statements 2026-10-15 to 2026-10-15): Concept ACK on options check, but NACK on removing solving_data without deprecation
- maflcko (member, support, 8 statements 2026-10-16 to 2026-10-16): ACKed rejecting unknown options; suggested removing redundant check and slimming test
- Zippo76 (none, support, 1 statements 2026-10-29 to 2026-10-29): Tested ACK; verified test and manual regtest behavior, reviewed diff logic
Files
11 lines under test/bench/ci.
- src/wallet/rpc/spend.cpp +19/-1
- test/functional/wallet_sendall.py +11/-0
- doc/release-notes-36494.md +7/-0
Card
The short text the ranking pass compares across PRs.
Reject unrecognized or mistyped keys in the options dictionary of the sendall RPC rather than silently ignoring them. Today, a typo such as input instead of inputs causes the wallet to spend all available UTXOs rather than the intended selection, while typos in psbt or add_to_wallet can lead to unintended transaction broadcasts. This change brings sendall in line with strict option parsing already used in RPCs like send and fundrawtransaction.
Data
thread read through 2026-10-29 (19 statements), head 702ec9d40f
labels: Wallet
Categories
Wallet
P2 · fund safety (ranking pass: Moved up: an unnoticed typo spending every wallet UTXO is a fund-safety defect, ranked with the other fund-safety P2s.)
Prevents silent execution of unintended spends and broadcasts when option keys are misspelled in sendall. As noted in the PR description, a typo like 'input' instead of 'inputs' causes the wallet to spend all available UTXOs rather than the intended selection, while misspelling 'add_to_wallet' or 'psbt' can cause unexpected transaction broadcast.
Membership: Has Wallet label and modifies src/wallet/rpc/spend.cpp.
Factors: security/stability 1, bug 2, performance 0, user value 2, leverage 0