bitcoin-tx: Require that input amount is provided for witness transactions #23784

pull theStack wants to merge 2 commits into bitcoin:master from theStack:bitcoin-tx_check_witness_input_amount changing 4 files +30 −1
  1. theStack commented at 3:25 PM on December 15, 2021: contributor

    This PR picks up the obviously abandoned PR #13608 (last activity was three and a half years ago) by rebasing it on master and adding missing tests. Original PR description: "Applies fix from #12458 / #13547 to bitcoin-tx."

    The private key is the compressed version of the one used in most other util tests (5HpHagT65TZzG1PH3CSu63k8DbpvD8s5ip4nEB3kEsreAnchuDf, corresponds to the scalar value k=1 in big endian), since segwit signing refuses uncompressed keys.

    The error message from the picked up PR is changed to not include the amount, as showing any value would be just confusing.

  2. MarcoFalke cross-referenced this on Dec 15, 2021 from issue WIP: bitcoin-tx: Require that input amount is provided for witness transactions by Empact
  3. theStack force-pushed on Dec 15, 2021
  4. DrahtBot added the label Build system on Dec 15, 2021
  5. DrahtBot added the label Utils/log/libs on Dec 15, 2021
  6. laanwj removed the label Build system on Dec 15, 2021
  7. in src/bitcoin-tx.cpp:673 in 8dc5fa1ee5 outdated 
     668 | @@ -669,6 +669,10 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
 669 |          if (!fHashSingle || (i < mergedTx.vout.size()))
 670 |              ProduceSignature(keystore, MutableTransactionSignatureCreator(&mergedTx, i, amount, nHashType), prevPubKey, sigdata);
 671 |  
 672 | +        if (amount == MAX_MONEY && !sigdata.scriptWitness.IsNull()) {
 673 | +            throw std::runtime_error(strprintf("Missing amount for %s", coin.out.ToString()));
    josibake commented at 2:57 PM on December 29, 2021:

    I tend to agree that seeing CTxOut(nValue=21000000.00000000... when the error is for a missing amount is confusing.

    Using MAX_AMOUNT for the check is already a nuanced concept (I had to read through all the linked PRs/issues before I understood why it was being used here). I don't think displaying 0 is correct, either, since a 0 amount UTXO is a valid spend.

    Perhaps just print the scriptPubKey? Something like Missing amount for CTxOut with scriptPubKey=0014751e76e8199196d454941c45d1

    theStack commented at 8:29 PM on December 29, 2021:

    Thanks, I agree that the best would be to not display the amount at all if it is missing and picked up your suggestion.

  8. josibake commented at 2:59 PM on December 29, 2021: contributor

    Concept ACK

    +1 for adding the tests! I don't think printing nValue=21000000.00000000 is a great idea, so I left a suggestion inline

  9. josibake commented at 3:00 PM on December 29, 2021: contributor

    One other suggestion: add a test with a 0 amount as that is a good (edge?) case to have tested.

  10. Require that input amount is provided for bitcoin-tx witness transactions
    Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
    c337b27d7c
  11. test: check that bitcoin-tx detects missing input amount for segwit transactions 8bd34dc774
  12. theStack force-pushed on Dec 29, 2021
  13. theStack commented at 8:33 PM on December 29, 2021: contributor

    Thanks for the valuable review @josibake! Rebased on master and force-pushed with the following adaptions:

    (git range-diff 8dc5fa1...8bd34dc)

  15. MarcoFalke commented at 11:40 AM on January 3, 2022: member

    Needs OP adjusted before merge?

  16. theStack commented at 1:57 PM on January 3, 2022: contributor

    Needs OP adjusted before merge?

    Thanks, done.

  17. MarcoFalke merged this on Jan 5, 2022
  18. MarcoFalke closed this on Jan 5, 2022
  19. theStack deleted the branch on Jan 5, 2022
  20. sidhujag referenced this in commit aefcb9b11f on Jan 6, 2022
  21. bitcoin locked this on Jan 5, 2023
Contributors
theStackjosibakeMarcoFalke
Labels
Utils/log/libs
Linked (view graph)
#12458 Enforce that amount is provided for signrawtransaction prevtxs#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
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-05-19 06:53 UTC