Enforce that amount is provided for signrawtransaction prevtxs #12458

pull Empact wants to merge 2 commits into bitcoin:master from Empact:rawtransaction-amount-required changing 5 files +74 −4
  1. Empact commented at 8:11 PM on February 16, 2018: member

    Fixes #12429

    This value is listed as required in the docs, but not enforced as such. We should update the docs if it's not required.

  2. Empact force-pushed on Feb 16, 2018
  3. Empact force-pushed on Feb 16, 2018
  4. instagibbs commented at 8:29 PM on February 16, 2018: member

    utACK

    I wouldn't mind argument checks for the other prevtx arguments, if none exist, but not a blocker.

  5. Empact commented at 8:52 PM on February 16, 2018: member

    @instagibbs Did you have any specific sort of testing in mind? Everything except redeemScript is tested, but redeemScript is not required in all cases.

  6. Empact commented at 8:55 PM on February 16, 2018: member

    Scratch that - redeemScript is tested here: https://github.com/bitcoin/bitcoin/pull/12458/files#diff-01aa7d1d32f1b9e5a836c9c411978918L839

  7. instagibbs commented at 8:56 PM on February 16, 2018: member

    I mean tested inside functional tests, sorry.

  8. Empact force-pushed on Feb 16, 2018
  9. Empact force-pushed on Feb 16, 2018
  10. fanquake added the label RPC/REST/ZMQ on Feb 17, 2018
  11. sipa commented at 3:26 AM on February 17, 2018: member

    It should only be required when signing a segwit input, no?

  12. jonasschnelli commented at 11:31 AM on February 17, 2018: contributor

    Agree with sipa. There is no need to enforce amounts for non SW inputs...

  13. Empact commented at 1:45 AM on February 19, 2018: member

    Gotcha - I'll revise.

  14. Empact force-pushed on Mar 1, 2018
  15. Empact renamed this:
    Enforce that amount is provided for signrawtransaction prevtxs
    WIP: Enforce that amount is provided for signrawtransaction prevtxs
    on Mar 1, 2018
  16. Empact force-pushed on Apr 11, 2018
  17. Empact commented at 6:48 PM on May 18, 2018: member

    This is up for grabs, fyi.

  18. MarcoFalke added the label Up for grabs on May 18, 2018
  19. ajtowns commented at 4:19 PM on June 27, 2018: member

    Grabbed in #13547

  20. Empact force-pushed on Jun 29, 2018
  21. Empact force-pushed on Jun 29, 2018
  22. Empact commented at 8:12 PM on June 29, 2018: member

    Thanks @ajtowns for picking this up! Heavily inspired by your work, I updated this PR. Relatively minor differences:

    • Use 0 rather than MAX_MONEY as the signifier. Seems that existing coins with 0 value are not any more valid than new ones.
    • throw before updating the input. No point in updating with the invalid signature data.
  23. Empact renamed this:
    WIP: Enforce that amount is provided for signrawtransaction prevtxs
    Enforce that amount is provided for signrawtransaction prevtxs
    on Jun 29, 2018
  24. sipa commented at 8:32 PM on June 29, 2018: member

    It is perfectly valid to spend UTXOs with value 0.

  25. Empact commented at 8:58 PM on June 29, 2018: member

    @sipa Maybe I'm confused or misspeaking then - if amount is required for segwit transactions to be signed properly, then amount of 0 would be invalid as not meeting that requirement when checking a segwit input, right?

  26. instagibbs commented at 9:01 PM on June 29, 2018: member

    @Empact 0-value outputs are completely legal and spendable.

  27. sipa commented at 9:32 PM on June 29, 2018: member

    @Empact I don't understand. The segwit sighashing scheme in BIP143 requires including the amount into the message being hashing. If the amount is zero, then it simply means the signature must be valid for a signature on a message that includes 0 in that position.

  28. Empact force-pushed on Jun 29, 2018
  29. Empact commented at 10:37 PM on June 29, 2018: member

    Thanks guys, TIL. Updated to use MAX_MONEY as in #13547. The differences are now down to a few incidental tests and an earlier throw in this PR.

  30. laanwj added this to the "Blockers" column in a project
  31. sipa commented at 3:32 AM on July 6, 2018: member

    utACK. Tested that it still works after merge with #13425.

    We should probably do the same for bitcoin-tx, but that can be done elsewhere.

  32. Empact force-pushed on Jul 7, 2018
  33. Empact commented at 4:48 PM on July 7, 2018: member

    Updated tests as per @MarcoFalke's feedback #13547 (review)

  34. in src/rpc/rawtransaction.cpp:814 in f7a4d0f9c8 outdated 
     810 | @@ -811,7 +811,7 @@ UniValue SignTransaction(CMutableTransaction& mtx, const UniValue& prevTxsUnival
 811 |                  }
 812 |                  Coin newcoin;
 813 |                  newcoin.out.scriptPubKey = scriptPubKey;
 814 | -                newcoin.out.nValue = 0;
 815 | +                newcoin.out.nValue = MAX_MONEY;
    l2a5b1 commented at 12:10 PM on July 8, 2018:

    I was wondering if MAX_MONEY should be used, because MAX_MONEY (like the value 0 that it replaces) is a valid amount isn't it?

    l2a5b1 commented at 12:28 PM on July 8, 2018:

    Please feel free to ignore this, but alternatively to express intent more directly,

    newcoin.out.nValue = MAX_MONEY;
if (prevOut.exists("amount")) {
    newcoin.out.nValue = AmountFromValue(find_value(prevOut, "amount"));
}

    can be replaced with a conditional expression:

    newcoin.out.nValue = prevOut.exists("amount") ? AmountFromValue(find_value(prevOut, "amount")) : MAX_MONEY;
  35. in src/rpc/rawtransaction.cpp:886 in f7a4d0f9c8 outdated 
     881 | @@ -882,6 +882,11 @@ UniValue SignTransaction(CMutableTransaction& mtx, const UniValue& prevTxsUnival
 882 |          }
 883 |          sigdata = CombineSignatures(prevPubKey, TransactionSignatureChecker(&txConst, i, amount), sigdata, DataFromTransaction(mtx, i));
 884 |  
 885 | +        // amount required for valid segwit signature
 886 | +        if (amount == MAX_MONEY && !sigdata.scriptWitness.IsNull()) {
    l2a5b1 commented at 12:44 PM on July 8, 2018:

    Please do ignore this if comparing against MAX_MONEY is valid, but if it isn't and you will be using an amount outside of the range [0,MAX_MONEY] then the boolean expression needs to be addressed. I suspect something along these lines would work:

    if (!MoneyRange(amount) && !sigdata.scriptWitness.IsNull()) {
  36. l2a5b1 commented at 1:03 PM on July 8, 2018: contributor

    Hey @Empact,

    I was wondering about the use of MAX_MONEY in this pull request and pull requests #13547 and #13608.

    Should we not use an amount outside of the range [0, MAX_MONEY], because MAX_MONEY is a valid amount?

  37. Empact force-pushed on Jul 9, 2018
  38. Empact force-pushed on Jul 9, 2018
  39. rpc: Enforce signrawtransaction prevtxs include an amount
    Fixes #12429
    a1636737e6
  40. [tests] Check signrawtransaction* errors on missing prevtx info a00adee186
  41. Empact force-pushed on Jul 9, 2018
  42. Empact commented at 4:27 AM on July 9, 2018: member

    Fair enough, added INVALID_MONEY as an alternative magic value.

  43. l2a5b1 commented at 8:27 AM on July 10, 2018: contributor

    @Empact, please see #13547#pullrequestreview-135472612. I hope my review comments haven't sent you in the wrong direction. :/

  44. sipa commented at 8:31 AM on July 10, 2018: member

    @251Labs 21000000.00000000 BTC (the MAX_MONEY constant) would be accepted as an amount if it were to occur inside a transaction. However, this can never happen for a valid transaction because there will never be that much in circulation.

  45. l2a5b1 commented at 9:58 AM on July 10, 2018: contributor

    Thanks @sipa, this confirmation is very valuable.

    I tried to set out in #13547 (review) why it was confusing to me. Perhaps, for the avoidance of doubt, it would be good to add your clarification it to the MAX_MONEY documentation.

    Apologies for the noise, I hope there was at least some value.

  46. laanwj referenced this in commit fad42e8c4a on Jul 10, 2018
  47. MarcoFalke removed the label Up for grabs on Jul 10, 2018
  48. DrahtBot added the label Needs rebase on Jul 10, 2018
  49. DrahtBot commented at 4:17 PM on July 10, 2018: member

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

  50. Empact commented at 5:47 AM on July 11, 2018: member

    Closing since #13547 was merged

  51. Empact closed this on Jul 11, 2018
  52. Empact deleted the branch on Jul 11, 2018
  53. laanwj removed this from the "Blockers" column in a project
  54. laanwj removed the label Needs rebase on Oct 24, 2019
  55. PastaPastaPasta referenced this in commit f5459232f7 on Jul 17, 2020
  56. PastaPastaPasta referenced this in commit 8bacc96f07 on Jun 27, 2021
  57. PastaPastaPasta referenced this in commit 5ecee84163 on Jun 28, 2021
  58. PastaPastaPasta referenced this in commit d1ad6b157f on Jun 29, 2021
  59. DrahtBot locked this on Dec 16, 2021
Contributors
Empactinstagibbssipajonasschnelliajtownsl2a5b1DrahtBot
Labels
RPC/REST/ZMQ
Linked (view graph)
#12429 signrawtransaction without "amount" creates invalid signature#13547 Make signrawtransaction* give an error when amount is needed but missing#13608 WIP: bitcoin-tx: Require that input amount is provided for witness transactions#23784 bitcoin-tx: Require that input amount is provided for witness transactions
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-04-13 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me