#36344 wallet: reject duplicate inputs in sendall
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36344 wallet: reject duplicate inputs in sendall | Ready | 2 (+1) | Strong | 4 + 19 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
Rejects duplicate inputs in the `sendall` RPC by tracking seen outpoints and throwing an `RPC_INVALID_PARAMETER` error if a duplicate is found. This prevents constructing and committing invalid transactions when an outpoint is specified more than once in the `inputs` option.
Problem: `sendall` adds the value of each specified input to the total input amount without verifying uniqueness. Supplying duplicate UTXOs creates a consensus-invalid transaction spending the same prevout multiple times; while the RPC reports success, the transaction fails mempool acceptance (`bad-txns-inputs-duplicate`) and locks the UTXOs in the wallet with negative balance accounting until abandoned.
Evidence of importance:
- The PR description demonstrates on regtest that passing the same UTXO twice creates a tx with duplicate inputs and double the payout, returns complete: true, is rejected by testmempoolaccept with bad-txns-inputs-duplicate, and causes getbalances to show nonmempool: -1.0 until abandoned.
- A functional test in test/functional/wallet_sendall.py confirms the RPC now throws RPC_INVALID_PARAMETER and leaves wallet balance and transaction counts untouched.
Risk of a mistake: medium (wallet, rpc_api). A flaw in input validation during transaction construction could reject valid UTXO sets or fail to properly construct spend transactions in the wallet RPC.
Changed since the previous assessment: No functional or structural change; appears to be a rebase or commit metadata update.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- nothing: all reviews addressed and PR has ACKs
Agreement
Strong
substantive support, no open objection (l0rinc, molnard, l0rinc)
Thread notes: Added Tested ACK from molnard and full ACK from l0rinc. Recorded l0rinc's container nit as an accepted suggestion since they approved and stated they are fine with it as-is.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| l0rinc (member) | suggestion, approach | assert the affected wallet state instead of an assertion that passes before and after, demonstrating the full state transition | accepted · fix pushed · l0rinc verified the state transition test and ACKed the push | answered | yes | 2026-10-18: 'The previous line correctly fails without the fix, but this assertion would pass before and after because the invalid transaction never enters the mempool - we should assert the affected wallet state ' Status from: 2026-10-30: l0rinc: 'I like 1 beef, ACK 1beefce3519348662d833734cadf01a23d60d83e' |
| l0rinc (member) | suggestion, approach | prefer std::unordered_set<COutPoint, SaltedOutpointHasher> with reserve for linear work and lower overhead on large sweeps | accepted · reviewer explicitly noted they are fine with the implementation as-is | answered | no | 2026-10-30: 'Nit: I’d prefer `std::unordered_set<COutPoint, SaltedOutpointHasher>` with `reserve(rawTx.vin.size())` here.' Status from: 2026-10-30: l0rinc: 'Left a nit, also fine with it as-is.' |
Support:
- achow101 (member) (ACK, 2026-09-29): (no reason given) [not substantive]
- l0rinc (member) (Concept ACK, 2026-10-18) [read]: The RPC-error assertion catches the missing rejection, but we should cover the reported wallet-state correction also - see inline suggestion.
- molnard (none) (Tested ACK, 2026-10-30) [tested: wallet_sendall.py]: Ran wallet_sendall.py on win; passes with the fix and fails with No exception raised when check is removed
- l0rinc (member) (ACK, 2026-10-30) [both: test/functional/wallet_sendall.py]: Verified state transition using characterization test in wallet_sendall.py
Participants:
- achow101 (member, support, 1 statements 2026-09-29 to 2026-09-29): ACKed the commit hash
- l0rinc (member, support, 4 statements 2026-10-18 to 2026-10-30): ACKed 1beefce after verifying state transition in tests; left an optional nit
- molnard (none, support, 1 statements 2026-10-30 to 2026-10-30): Tested ACK on Windows
Files
19 lines under test/bench/ci.
- test/functional/wallet_sendall.py +19/-0
- src/wallet/rpc/spend.cpp +4/-0
Card
The short text the ranking pass compares across PRs.
Reject duplicate inputs in the sendall RPC with an RPC_INVALID_PARAMETER error. Previously, passing duplicate UTXOs to sendall caused the wallet to sum input values multiple times and generate consensus-invalid transactions (bad-txns-inputs-duplicate). These transactions failed mempool acceptance but were committed to the wallet, locking coins and displaying negative balances until abandoned. The fix mirrors duplicate checks in other RPCs and adds functional test coverage.
Data
thread read through 2026-10-30 (7 statements), head 1beefce351
labels: Wallet
Categories
Wallet
P3 · bug fix
Fixes a bug where `sendall` creates consensus-invalid transactions and leaves the wallet with negative balance accounting when duplicate inputs are passed. While not a permanent loss of funds, it resolves confusing failures and bad wallet state requiring manual abandonment.
Membership: Has the Wallet label and modifies `src/wallet/rpc/spend.cpp` and `test/functional/wallet_sendall.py`.
Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 0