#36342 wallet: reject recipient amounts above MAX_MONEY
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36342 wallet: reject recipient amounts above MAX_MONEY | Ready | 0 (+1) | Strong | 3 + 22 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 that each recipient amount and the running sum of recipient amounts satisfy MoneyRange in CreateTransactionInternal before coin selection starts. Removes the separate non-negative check in CreateTransaction, which is now subsumed by the MoneyRange check.
Problem: CreateTransaction only checked that recipient amounts were not negative, omitting upper-bound checks against MAX_MONEY for both individual amounts and their sum. When funding transactions with large amounts (via fundrawtransaction, walletcreatefundedpsbt, or sendmany), recipients_sum could overflow int64, triggering UBSan errors, an assertion failure crash in RecalculateWaste, or wrapping around to a small positive sum and incorrectly funding outputs exceeding 21 million BTC.
Evidence of importance:
- Author description demonstrates a reproducible node crash on master with fundrawtransaction: 'Assertion failed: (!m_selected_inputs.empty()), function RecalculateWaste, file coinselection.cpp, line 904.'
- Author notes that with other wrapped sums, 'fundrawtransaction returns success for a tx with outputs of 46116860184 BTC each.'
Risk of a mistake: medium (wallet). A flaw in recipient amount validation during transaction creation could reject legitimate payments or disrupt wallet funding operations.
Changed since the previous assessment: No functional changes; the commit was rebased or re-formatted.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: re-review after author incorporated loop refactoring suggestions and squashed 2026-09-29
- author: reply to pinheadmz's suggestion to close other PRs 2026-09-28
Agreement
Strong
substantive support, no open objection (maflcko)
Thread notes: Added furszy's suggestion to move the MoneyRange check into the existing recipients loop, which the author implemented and squashed in c:5892125804, also resolving maflcko's suggestion rc:4131313711. maflcko's squash request rc:4134252001 is a process comment.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| pinheadmz (member) | question, approach | Asks whether the submission conforms to the AI policy and whether the author understands the choices made and is solving real problems. | answered · author confirmed reading the policy, clarified AI was only used for description and tests, and explained submitting multiple PRs at once | answered | yes | 2026-09-26: 'Do all three of these conform to our AI Policy? Before we spend any precious human resources reading through your submissions can you please assure us that you understand all the choices you've made' Status from: 2026-09-26: Bruce039: 'Yes, I've read the AI policy and I accept it. And yes, I used AI, but only for the PR description and the tests.' |
| pinheadmz (member) | suggestion, approach | Suggests closing 6 of the author's 7 open PRs to focus on one and avoid flooding the repository with AI-assisted submissions. | open | answered | no | 2026-09-28: 'If you really want to support the project I suggest closing 6 of them. Pick one that really means a lot to you, that you think is the most important for the most bitcoin users, and really commit to it' |
| furszy (member) | suggestion, approach | Move the check into the existing recipients loop and merge the negative amount check there too. | fixed · fix pushed · author moved the check into the existing recipients loop and removed the separate check | answered | yes | 2026-09-29: 'Can move the check below the existing loop: ... and could also merge the negative amount checks there too. So it is just one loop.' Status from: 2026-09-29: Bruce039: 'Following furszy, the MoneyRange check now sits in the existing recipients loop in `CreateTransactionInternal`, and it also covers the negative-amount case' |
| maflcko (member) | suggestion, approach | Combine the check into a single loop and simplify the error message to 'Transaction output amount must be valid' to avoid two loops. | fixed · fix pushed · author folded negative check into MoneyRange loop and squashed commits | answered | yes | 2026-09-29: 'I think you can combine this check/loop and have the error message just be "Transaction output amount must be valid". this avoids two loops and overly strict hand holding' Status from: 2026-09-29: Bruce039: 'Following furszy, the MoneyRange check now sits in the existing recipients loop in `CreateTransactionInternal`, and it also covers the negative-amount case' |
Support:
- maflcko (member) (ACK, 2026-09-28) [read]: Seems fine as a low-prio sanity check and for nicer sanitizer use
Participants:
- pinheadmz (member, question, 2 statements 2026-09-26 to 2026-09-28): asked about AI policy compliance and suggested closing other PRs to focus on one
- maflcko (member, support, 3 statements 2026-09-28 to 2026-09-29): ACKed as a sanity check and suggested combining the validation loop
- furszy (member, neutral, 1 statements 2026-09-29 to 2026-09-29): suggested merging the check into the existing recipients loop
Files
22 lines under test/bench/ci.
- test/functional/wallet_fundrawtransaction.py +22/-0
- src/wallet/spend.cpp +3/-4
Card
The short text the ranking pass compares across PRs.
Validates that each recipient amount and their running sum fall within MoneyRange in CreateTransactionInternal before coin selection. Without this check, transactions with large outputs passed through RPCs like fundrawtransaction can overflow the int64 recipients sum, causing node crashes via assertion failures in coin selection or wrapping to fund transactions exceeding MAX_MONEY. Functional tests are added to verify rejection of oversized outputs, cumulative overflow, and negative amounts.
Data
thread read through 2026-09-29 (9 statements), head bab579e56a
labels: Wallet
Categories
Wallet
P3 · bug fix (ranking pass: Moved up: it prevents an assertion crash and an int64 overflow, the strongest of the P3 bug fixes.)
Prevents integer overflow and an assertion crash in RecalculateWaste when wallet transaction creation RPCs receive amounts exceeding MAX_MONEY. The problem is a real correctness and crash bug, but requires authenticated RPC calls with out-of-range amounts and is reasonably deferrable.
Membership: Matches Wallet label and modifies wallet spend logic in src/wallet/spend.cpp to validate recipient amounts.
Factors: security/stability 1, bug 2, performance 0, user value 1, leverage 0