lib: Add Taproot support to libconsensus #21158

pull jrawsthorne wants to merge 4 commits into bitcoin:master from jrawsthorne:libconsensus_taproot changing 5 files +141 −10
  1. jrawsthorne commented at 10:59 PM on February 11, 2021: none

    This change exposes a new function in libconsensus that allows passing spent outputs for Taproot verification. It also adds a VERIFY_TAPROOT flag.

    If spent outputs are not provided when the VERIFY_TAPROOT flag is enabled, there is a new error returned SPENT_OUTPUTS_REQUIRED. If the VERIFY_TAPROOT flag is enabled when verify or verify_with_amount then the same error is returned.

    I also included testing the consensus lib in the qa asset test.

    Closes #21133

  2. DrahtBot added the label Consensus on Feb 11, 2021
  3. jrawsthorne force-pushed on Feb 12, 2021
  4. jrawsthorne force-pushed on Feb 12, 2021
  5. jrawsthorne force-pushed on Feb 12, 2021
  6. jrawsthorne marked this as ready for review on Feb 12, 2021
  7. in src/script/bitcoinconsensus.cpp:16 in 883cac2efe outdated
      12 | @@ -13,13 +13,13 @@
      13 |  namespace {
      14 |  
      15 |  /** A class that deserializes a single CTransaction one time. */
      16 | -class TxInputStream
      17 | +class InputStream
    


    ariard commented at 1:09 PM on February 13, 2021:

    I would advice introducing the code style changes in their own commit. Also, I think it's clearer to keep the Tx prefix, you have other class of inputs in the codebase beyond transaction like hash elements.

  8. in src/script/bitcoinconsensus.cpp:115 in 883cac2efe outdated
     107 |      } catch (const std::exception&) {
     108 |          return set_error(err, bitcoinconsensus_ERR_TX_DESERIALIZE); // Error deserializing
     109 |      }
     110 |  }
     111 |  
     112 | +int bitcoinconsensus_verify_script_with_spent_outputs(const unsigned char *scriptPubKey, unsigned int scriptPubKeyLen, int64_t amount,
    


    ariard commented at 1:37 PM on February 13, 2021:

    Have a look on b7dbeb2, I think you should bump the API version, at least it was done for segwit support.

    I think you should also add a commit to document this change in doc/shared-libraries.md.

  9. in src/script/bitcoinconsensus.cpp:94 in 883cac2efe outdated
      87 |          CTransaction tx(deserialize, stream);
      88 | +        std::vector<CTxOut> spent_outputs;
      89 | +        if (spentOutputs != nullptr) {
      90 | +            InputStream spent_outputs_stream(PROTOCOL_VERSION, spentOutputs, spentOutputsLen);
      91 | +            spent_outputs_stream >> spent_outputs;
      92 | +        }
    


    ariard commented at 1:40 PM on February 13, 2021:

    Check the equality between inputs/outputs against tx.vin.size() (a bitcoinconsensus_ERR_OUTP_INDEX ?)

  10. ariard commented at 1:42 PM on February 13, 2021: member

    Thanks for adding this. If the API is bumped, take care to notify library users through mentioning the change in 0.22 release note (doc/release-noted.md)

  11. jrawsthorne commented at 1:46 AM on February 14, 2021: none

    Thanks for adding this. If the API is bumped, take care to notify library users through mentioning the change in 0.22 release note (doc/release-noted.md) @ariard Thanks for the review. Do you have a suggestion about which section the change should be mentioned in?

  12. ariard commented at 10:06 PM on February 14, 2021: member

    @jrawsthorne "Tools and Utilities" sounds good to me.

  13. jrawsthorne force-pushed on Feb 14, 2021
  14. in doc/shared-libraries.md:18 in 01086293a7 outdated
      15 | +`bitcoinconsensus_version` returns an `unsigned int` with the API version *(currently `2`)*.
      16 |  
      17 |  #### Script Validation
      18 |  
      19 | -`bitcoinconsensus_verify_script` returns an `int` with the status of the verification. It will be `1` if the input script correctly spends the previous output `scriptPubKey`.
      20 | +`bitcoinconsensus_verify_script`, `bitcoinconsensus_verify_script_with_amounts` and `bitcoinconsensus_verify_script_with_spent_outputs` return an `int` with the status of the verification. It will be `1` if the input script correctly spends the previous output `scriptPubKey`.
    


    SomberNight commented at 5:56 AM on February 17, 2021:

    there is a typo in the function name

    `bitcoinconsensus_verify_script`, `bitcoinconsensus_verify_script_with_amount` and `bitcoinconsensus_verify_script_with_spent_outputs` return an `int` with the status of the verification. It will be `1` if the input script correctly spends the previous output `scriptPubKey`.
    
  15. in doc/shared-libraries.md:30 in 01086293a7 outdated
      26 | @@ -26,6 +27,28 @@ The interface is defined in the C header `bitcoinconsensus.h` located in `src/sc
      27 |  - `unsigned int flags` - The script validation flags *(see below)*.
      28 |  - `bitcoinconsensus_error* err` - Will have the error/success code for the operation *(see below)*.
      29 |  
      30 | +###### bitcoinconsensus_verify_script_with_amounts
    


    SomberNight commented at 5:59 AM on February 17, 2021:

    same as above, typo in function name

  16. jrawsthorne force-pushed on Feb 17, 2021
  17. jrawsthorne requested review from ariard on Feb 18, 2021
  18. in doc/shared-libraries.md:72 in 28245a8641 outdated
      65 | @@ -42,6 +66,8 @@ The interface is defined in the C header `bitcoinconsensus.h` located in `src/sc
      66 |  - `bitcoinconsensus_ERR_DESERIALIZE` - An error deserializing `txTo`
      67 |  - `bitcoinconsensus_ERR_AMOUNT_REQUIRED` - Input amount is required if WITNESS is used
      68 |  - `bitcoinconsensus_ERR_INVALID_FLAGS` - Script verification `flags` are invalid (i.e. not part of the libconsensus interface)
      69 | +- `bitcoinconsensus_ERR_SPENT_OUTPUTS_REQUIRED` - Spent outputs are required if TAPROOT is used
      70 | +- `bitcoinconsensus_ERR_SPENT_OUTPUTS_MISMATCH` - Spent outputs size doesn't match tx inputs size
      71 |  
      72 |  ### Example Implementations
    


    ariard commented at 3:39 PM on February 23, 2021:

    Can you update this to point to rust-bitcoinconsensus :) ?

  19. ariard commented at 3:40 PM on February 23, 2021: member

    Code Review ACK 28245a8, thanks for the changes

    Up to you to take the nit.

  20. DrahtBot cross-referenced this on Mar 2, 2021 from issue Deal with missing data in signature hashes more consistently by sipa
  21. DrahtBot commented at 11:39 AM on March 2, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    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.

  22. DrahtBot cross-referenced this on Mar 4, 2021 from issue Basic Taproot signing support for descriptor wallets by sipa
  23. DrahtBot commented at 4:50 PM on March 15, 2021: contributor

    <!--4a62be1de6b64f3ed646cdc7932c8cf5-->

    🕵️ @harding has been requested to review this pull request as specified in the REVIEWERS file.

  24. DrahtBot cross-referenced this on Apr 4, 2021 from issue cli: create -addrinfo by jonatack
  25. DrahtBot added the label Needs rebase on Apr 13, 2021
  26. jrawsthorne force-pushed on Apr 15, 2021
  27. DrahtBot removed the label Needs rebase on Apr 15, 2021
  28. ariard commented at 4:50 PM on April 16, 2021: member

    Code Review ACK 7c85ec9. Changes from last ACK is complying VerifyScript call to new MissingDataBehavior introduced by b77b0cc and mention of rust-bitcoin lib.

  29. DrahtBot added the label Needs rebase on Apr 20, 2021
  30. ariard commented at 5:33 PM on April 26, 2021: member

    @jrawsthorne I think this needs rebase, but that would be cool to land this for downstream projects.

  31. jrawsthorne force-pushed on Apr 26, 2021
  32. jrawsthorne commented at 5:44 PM on April 26, 2021: none

    Thanks @ariard, rebased

  33. DrahtBot removed the label Needs rebase on Apr 26, 2021
  34. ariard commented at 1:59 PM on April 30, 2021: member

    Code Review ACK 096ab5c.

    Diff since last ack is swallowing release-note changes.

  35. ariard commented at 2:00 PM on April 30, 2021: member

    @SomberNight I guess you're also interested to for downstream support, if you have time to review again, that would be cool :)

  36. jrawsthorne cross-referenced this on May 16, 2021 from issue libbitcoinconsensus.dylib in MacOS releases is broken by piotrnar
  37. DrahtBot cross-referenced this on May 20, 2021 from issue cli: Improve -getinfo return format by klementtan
  38. DrahtBot cross-referenced this on Jun 6, 2021 from issue doc: update tor.md, release notes with removal of tor v2 support by jonatack
  39. DrahtBot added the label Needs rebase on Jun 12, 2021
  40. refactor: Rename variables in preparation for generic deserialization 78447f3d11
  41. jrawsthorne force-pushed on Jun 12, 2021
  42. DrahtBot removed the label Needs rebase on Jun 12, 2021
  43. in src/test/script_tests.cpp:1763 in d0c6b1a9b6 outdated
    1758 |          for (const auto flags : ALL_CONSENSUS_FLAGS) {
    1759 |              // If a test is supposed to fail with test_flags, it should also fail with any superset thereof.
    1760 |              if ((flags & test_flags) == test_flags) {
    1761 |                  bool ret = VerifyScript(tx.vin[idx].scriptSig, prevouts[idx].scriptPubKey, &tx.vin[idx].scriptWitness, flags, txcheck, nullptr);
    1762 |                  BOOST_CHECK(!ret);
    1763 | +                #if defined(HAVE_CONSENSUS_LIB)
    


    sipa commented at 3:48 AM on June 13, 2021:

    Generally macro statements aren't indented like the code they're included in.


    jrawsthorne commented at 1:28 PM on June 13, 2021:

    Thanks, didn't know that. Fixed in d82e5de6a41794cc8fd1e3afaa12af26bb7be926

  44. in src/script/bitcoinconsensus.h:58 in d0c6b1a9b6 outdated
      54 | @@ -53,9 +55,11 @@ enum
      55 |      bitcoinconsensus_SCRIPT_FLAGS_VERIFY_CHECKLOCKTIMEVERIFY = (1U << 9), // enable CHECKLOCKTIMEVERIFY (BIP65)
      56 |      bitcoinconsensus_SCRIPT_FLAGS_VERIFY_CHECKSEQUENCEVERIFY = (1U << 10), // enable CHECKSEQUENCEVERIFY (BIP112)
      57 |      bitcoinconsensus_SCRIPT_FLAGS_VERIFY_WITNESS             = (1U << 11), // enable WITNESS (BIP141)
      58 | +    bitcoinconsensus_SCRIPT_FLAGS_VERIFY_TAPROOT             = (1U << 17), // enable TAPROOT (BIP341-343)
    


    sipa commented at 3:56 AM on June 13, 2021:

    BIP341-342 (343 isn't a thing).


    jrawsthorne commented at 1:28 PM on June 13, 2021:

    Oops, fixed in d82e5de6a41794cc8fd1e3afaa12af26bb7be926

  45. sipa commented at 3:57 AM on June 13, 2021: member

    utACK 698391966afd0004d6d31fc590ffb8e4222408b8

  46. lib: Add Taproot support to libconsensus d82e5de6a4
  47. docs: Add docs for additional libconsensus functions 3dd932ef22
  48. docs: Link to rust-bitcoinconsensus c23996ad35
  49. jrawsthorne force-pushed on Jun 13, 2021
  50. jrawsthorne commented at 1:29 PM on June 13, 2021: none

    Thanks for your review @sipa, I've addressed your comments. Also pinging @ariard

  51. in src/script/bitcoinconsensus.cpp:90 in c23996ad35
      83 | @@ -83,6 +84,14 @@ static int verify_script(const unsigned char *scriptPubKey, unsigned int scriptP
      84 |      try {
      85 |          TxInputStream stream(PROTOCOL_VERSION, txTo, txToLen);
      86 |          CTransaction tx(deserialize, stream);
      87 | +        std::vector<CTxOut> spent_outputs;
      88 | +        if (spentOutputs != nullptr) {
      89 | +            TxInputStream spent_outputs_stream(PROTOCOL_VERSION, spentOutputs, spentOutputsLen);
      90 | +            spent_outputs_stream >> spent_outputs;
    


    luke-jr commented at 1:56 AM on June 23, 2021:

    Might be nicer to let the caller pass in an array rather than have to serialise the vector?


    jrawsthorne commented at 11:17 AM on July 4, 2021:

    Correct me if I'm wrong but I thought it would have to be a vector and that can't be exposed as part of the C API


    sipa commented at 6:18 PM on August 20, 2021:

    It could take in a pointer to an array of {pointer to utxo, length} structs or so, as well as the number of inputs. That's not particularly convenient to use, but neither is the current interface (as it probably requires serialization code on the caller side).

  52. in doc/shared-libraries.md:46 in c23996ad35
      41 | +- `const unsigned char *scriptPubKey` - The previous output script that encumbers spending.
      42 | +- `unsigned int scriptPubKeyLen` - The number of bytes for the `scriptPubKey`.
      43 | +- `int64_t amount` - The amount spent in the input
      44 | +- `const unsigned char *txTo` - The transaction with the input that is spending the previous output.
      45 | +- `unsigned int txToLen` - The number of bytes for the `txTo`.
      46 | +- `const unsigned char *spentOutputs` - Previous outputs spent in the transaction
    


    luke-jr commented at 1:56 AM on June 23, 2021:

    This leaves me guessing how to call this function.


    jrawsthorne commented at 11:17 AM on July 4, 2021:

    Do you want to add something like "serialized as a vector of TxOut" or something more substantial?

  53. luke-jr changes_requested
  54. DrahtBot cross-referenced this on Jul 14, 2021 from issue Eliminate Signature Checker/Creator Ambiguity w/ LIFETIMEBOUND References by JeremyRubin
  55. DrahtBot added the label Needs rebase on Jul 21, 2021
  56. DrahtBot commented at 10:00 AM on July 21, 2021: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

    🐙 This pull request conflicts with the target branch and needs rebase.

    <sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

  57. DrahtBot commented at 12:28 PM on December 22, 2021: contributor

    <!--13523179cfe9479db18ec6c5d236f789-->There hasn't been much activity lately and the patch still needs rebase. What is the status here?

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
  58. MarcoFalke added the label Up for grabs on Apr 29, 2022
  59. MarcoFalke commented at 2:17 PM on April 29, 2022: member

    Marked "up for grabs"

  60. fanquake closed this on May 12, 2022

  61. theStack cross-referenced this on Apr 22, 2023 from issue Consider Removing Message Signing by TheBlueMatt
  62. bitcoin locked this on May 12, 2023
  63. fanquake removed the label Up for grabs on Sep 27, 2023
  64. fanquake removed the label Needs rebase on Sep 27, 2023

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