wallet: reject recipient amounts above MAX_MONEY #36342

pull Bruce039 wants to merge 1 commits into bitcoin:master from Bruce039:fix-recipients-sum-overflow changing 2 files +25 −4
  1. Bruce039 commented at 9:53 AM on September 26, 2026: none

    CreateTransaction checks that recipient amounts aren't negative, but not that they're within MAX_MONEY. CreateTransactionInternal then sums them into recipients_sum (spend.cpp:1100) with no range check, and that sum can overflow.

    Easiest way to hit it is fundrawtransaction with a raw tx that has two outputs of 2^62 sat. It can also be reached with only valid amounts through walletcreatefundedpsbt/send/sendmany and a few thousand 21M BTC outputs. The "Transaction too large" check only runs after coin selection, so it doesn't stop this. On master the node aborts:

    Assertion failed: (!m_selected_inputs.empty()), function RecalculateWaste, file coinselection.cpp, line 904.
    

    UBSan reports the overflow at spend.cpp:1100 first. With other values the sum wraps to a small positive number instead, and fundrawtransaction returns success for a tx with outputs of 46116860184 BTC each.

    This checks every amount and the running sum against MoneyRange in the existing recipients loop of CreateTransactionInternal, before the amount is added to recipients_sum, and returns "Transaction output amount must be valid" before coin selection starts. The same check rejects negative amounts, so the separate negative check in CreateTransaction is removed. The new test_outputs_above_max_money in wallet_fundrawtransaction.py covers a single output above MAX_MONEY, two outputs whose sum is above it, a sum that overflows int64, and a negative output. On master the first case returns "Insufficient funds" and the overflow case crashes the node; with the fix all of them get the new error. wallet_fundrawtransaction.py, wallet_send.py, wallet_basic.py and rpc_psbt.py pass.

  2. DrahtBot added the label Wallet on Sep 26, 2026
  3. DrahtBot commented at 9:53 AM on September 26, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36342.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

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

    Type Reviewers
    Stale ACK maflcko

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #35511 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35511.svg"></sub> (RFC: consensus: Make CAmount a class by hodlinator)

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. Bruce039 force-pushed on Sep 26, 2026
  5. pinheadmz commented at 11:40 AM on September 26, 2026: member

    You just opened three PRs in ten minutes. 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 and that you are motivated by solving real problems real users have with the software?

  6. Bruce039 commented at 12:05 PM on September 26, 2026: none

    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. Finding the bug and the rest of the underlying work was done by me. About the 3 PRs: I've been preparing for PRs for the last few days, and it made more sense to me to collect all the bugs I found and send them at the same time.

  7. DrahtBot added the label CI failed on Sep 26, 2026
  8. DrahtBot removed the label CI failed on Sep 26, 2026
  9. pinheadmz commented at 2:49 PM on September 28, 2026: member

    You now have 7 PRs open in two days, and you are a brand new contributor. 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. Show us that you the human cares about the project and is willing to put in the time and the hard work that the rest of us put in.

    Earn your reputation here by starting slow.

    We do not need another "AI operator" pasting claude output, that will result in a ban.

  10. maflcko commented at 3:50 PM on September 28, 2026: member

    lgtm ACK f2915243d616e9e579663cbea41e12f2ba1e93fa

    Seems fine as a low-prio sanity check and for nicer sanitizer use

  11. in src/wallet/spend.cpp:1460 in f2915243d6
    1459 | @@ -1460,6 +1460,15 @@ util::Result<CreatedTransactionResult> CreateTransaction(
    1460 |          return util::Error{_("Transaction amounts must not be negative")};
    


    maflcko commented at 8:34 AM on September 29, 2026:

    nit: 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 for errors that should never happen anyway.


    Bruce039 commented at 1:46 PM on September 29, 2026:

    Done: the negative-amount check is folded into the MoneyRange loop, with the single "Transaction output amount must be valid" error. (Changes prepared with AI tools.)


    maflcko commented at 1:51 PM on September 29, 2026:
  12. furszy commented at 2:03 PM on September 29, 2026: member

    Can move the check below the existing loop:

    https://github.com/bitcoin/bitcoin/blob/dc2a9987aab711abdc1a77aa6d1462c6a46deabd/src/wallet/spend.cpp#L1092-L1097

    and could also merge the negative amount checks there too. So it is just one loop.

  13. wallet: reject recipient amounts outside MoneyRange
    CreateTransactionInternal summed the recipient amounts without checking
    them, so an output above MAX_MONEY, or outputs whose sum exceeds it or
    overflows CAmount, reached coin selection. Check each amount and the
    running sum against MoneyRange in the existing recipients loop. This also
    covers negative amounts, so the separate check in CreateTransaction is
    removed.
    bab579e56a
  14. Bruce039 force-pushed on Sep 29, 2026
  15. Bruce039 commented at 2:17 PM on September 29, 2026: none

    Squashed into one commit (bab579e56a). Following furszy, the MoneyRange check now sits in the existing recipients loop in CreateTransactionInternal, and it also covers the negative-amount case, so the separate check in CreateTransaction is gone. Added a negative output to the test. (Changes prepared with AI tools.)


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-10-18 10:51 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me