lib: add taproot support to libconsensus

brunoerg commented at 9:02 pm on September 26, 2023: contributor

Grabbed from #21158. Closes #21133.

DrahtBot commented at 9:02 pm on September 26, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, achow101, darosior

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28438 (Use serialization parameters for CTransaction by ajtowns)

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.

brunoerg referenced this in commit a5edeca85d on Sep 26, 2023

brunoerg force-pushed on Sep 26, 2023

DrahtBot added the label CI failed on Sep 26, 2023

DrahtBot removed the label CI failed on Sep 27, 2023

fanquake commented at 8:16 am on September 27, 2023: member

cc @darosior

darosior commented at 9:53 am on September 27, 2023: member

Yay, concept ACK. Thanks for picking this up. Do you plan to address #21158 (review)? Is this why you opened this as a draft?

brunoerg commented at 10:42 am on September 27, 2023: contributor

Yay, concept ACK. Thanks for picking this up. Do you plan to address #21158 (review)? Is this why you opened this as a draft?

Yes!

brunoerg referenced this in commit 8b3ca44230 on Sep 28, 2023

brunoerg force-pushed on Sep 28, 2023

brunoerg referenced this in commit 47f96cc0b4 on Sep 28, 2023

brunoerg force-pushed on Sep 28, 2023

DrahtBot added the label CI failed on Sep 28, 2023

brunoerg referenced this in commit 83ac8c70e2 on Sep 29, 2023

brunoerg force-pushed on Sep 29, 2023

sipa requested review from sipa on Sep 29, 2023

sipa requested review from theStack on Sep 29, 2023

sipa removed review request from theStack on Sep 29, 2023

brunoerg marked this as ready for review on Sep 29, 2023

brunoerg commented at 12:31 pm on September 29, 2023: contributor

Ready for review!

in src/test/script_tests.cpp:1666 in 83ac8c70e2 outdated

1661+
1662+    result = bitcoinconsensus_verify_script_with_amount(scriptPubKey.data(), scriptPubKey.size(), creditTx.vout[0].nValue, (const unsigned char*)&stream[0], stream.size(), nIn, libconsensus_flags, &err);
1663+    BOOST_CHECK_EQUAL(result, 0);
1664+    BOOST_CHECK_EQUAL(err, bitcoinconsensus_ERR_SPENT_OUTPUTS_REQUIRED);
1665+
1666+    result = bitcoinconsensus_verify_script(scriptPubKey.data(), scriptPubKey.size(), (const unsigned char*)&stream[0], stream.size(), nIn, libconsensus_flags, &err);

maflcko commented at 12:41 pm on September 29, 2023:

I think you can use UCharCast, which is safer than the c-style pointer cast?

brunoerg commented at 1:08 pm on September 29, 2023:

Done

in src/test/script_tests.cpp:1629 in e5d89cc639 outdated

1625@@ -1626,7 +1626,7 @@ BOOST_AUTO_TEST_CASE(bitcoinconsensus_verify_script_invalid_flags)
1626 
1627     scriptPubKey << OP_EQUAL;
1628     CTransaction creditTx{BuildCreditingTransaction(scriptPubKey, 1)};
1629-    CTransaction spendTx{BuildSpendingTransaction(scriptSig, wit, creditTx)};
1630+    CTransaction spendTx(BuildSpendingTransaction(scriptSig, wit, creditTx));

maflcko commented at 12:42 pm on September 29, 2023:

Is this change needed?

brunoerg commented at 12:55 pm on September 29, 2023:

sorry, no! gonna change it

brunoerg referenced this in commit 61cf25914d on Sep 29, 2023

brunoerg force-pushed on Sep 29, 2023

brunoerg commented at 1:09 pm on September 29, 2023: contributor

Force-pushed addressing #28539 (review) and changed the type of value in UTXO struct to int64_t.

in src/test/script_tests.cpp:1536 in 96d8b24955 outdated

1532@@ -1533,7 +1533,7 @@ BOOST_AUTO_TEST_CASE(bitcoinconsensus_verify_script_tx_index_err)
1533     CScriptWitness wit;
1534 
1535     scriptPubKey << OP_EQUAL;
1536-    CTransaction creditTx{BuildCreditingTransaction(scriptPubKey, 1)};
1537+    CTransaction creditTx(BuildCreditingTransaction(scriptPubKey, 1));

darosior commented at 1:12 pm on September 29, 2023:

Seems like those are unneeded too.

brunoerg referenced this in commit f5cd559846 on Sep 29, 2023

brunoerg force-pushed on Sep 29, 2023

brunoerg commented at 1:26 pm on September 29, 2023: contributor

Addressed #28539 (review)

darosior commented at 1:48 pm on September 29, 2023: member

Let’s include the new function to the libbitcoinconsensus fuzz target? (I think we could actually improve this target but that’s probably better done separately.)

 0diff --git a/src/test/fuzz/script_bitcoin_consensus.cpp b/src/test/fuzz/script_bitcoin_consensus.cpp
 1index fcd66b234e..b4b7988e77 100644
 2--- a/src/test/fuzz/script_bitcoin_consensus.cpp
 3+++ b/src/test/fuzz/script_bitcoin_consensus.cpp
 4@@ -28,4 +28,19 @@ FUZZ_TARGET(script_bitcoin_consensus)
 5     }
 6     (void)bitcoinconsensus_verify_script(random_bytes_1.data(), random_bytes_1.size(), random_bytes_2.data(), random_bytes_2.size(), n_in, flags, err_p);
 7     (void)bitcoinconsensus_verify_script_with_amount(random_bytes_1.data(), random_bytes_1.size(), money, random_bytes_2.data(), random_bytes_2.size(), n_in, flags, err_p);
 8+
 9+    std::vector<UTXO> spent_outputs;
10+    spent_outputs.reserve(n_in);
11+    std::vector<std::vector<unsigned char>> spent_spks;
12+    spent_spks.reserve(n_in);
13+    for (unsigned i = 0; i < n_in; ++i) {
14+        spent_spks.push_back(ConsumeRandomLengthByteVector(fuzzed_data_provider));
15+        const CAmount value{ConsumeMoney(fuzzed_data_provider)};
16+        const auto spk_size{static_cast<unsigned>(spent_spks.back().size())};
17+        spent_outputs.push_back({.scriptPubKey = spent_spks.back().data(), .scriptPubKeySize = spk_size, .value = value});
18+    }
19+    const auto spent_outs_size{static_cast<unsigned>(spent_outputs.size())};
20+    (void)bitcoinconsensus_verify_script_with_spent_outputs(
21+            random_bytes_1.data(), random_bytes_1.size(), money, random_bytes_2.data(), random_bytes_2.size(),
22+            spent_outputs.data(), spent_outs_size, n_in, flags, err_p);
23 }

darosior commented at 2:15 pm on September 29, 2023: member

You can also drop the first commit, it was only needed because the former approach was deserializing the outputs.

Other than this, code LGTM.

fanquake added this to the milestone 26.0 on Sep 29, 2023

DrahtBot removed the label CI failed on Sep 29, 2023

brunoerg commented at 3:03 pm on September 29, 2023: contributor

Let’s include the new function to the libbitcoinconsensus fuzz target? (I think we could actually improve this target but that’s probably better done separately.)

Sounds good, but not sure about the effectiveness, it really seems to need an improvement. I tried your suggestion and got OOM.

brunoerg referenced this in commit 11990d421a on Sep 29, 2023

brunoerg force-pushed on Sep 29, 2023

brunoerg commented at 5:32 pm on September 29, 2023: contributor

You can also drop the first commit, it was only needed because the former approach was deserializing the outputs.

Force-pushed dropping it.

darosior commented at 2:15 pm on September 30, 2023: member

EDIT: sorry about this comment’s previous formatting issues, my mobile email client seems to only have sent part of the mail… Here is the initial content of my reply.

Sounds good, but not sure about the effectiveness, it really seems to need an improvement. I tried your suggestion and got OOM.

Yeah it can be simply fixed by only allocating the spent_outputs vector if n_in is less than or equal to the maximum number of inputs possible in a transaction, 24386. A smaller limit, like 10k could also do imo. Just let’s keep the call to the newly introduce function even when spent_outputs is empty?

brunoerg commented at 3:04 pm on September 30, 2023: contributor

Force-pushed adding fuzz coverage for bitcoinconsensus_verify_script_with_spent_outputs.

in src/test/fuzz/script_bitcoin_consensus.cpp:32 in de4be03ac4 outdated

27@@ -28,4 +28,20 @@ FUZZ_TARGET(script_bitcoin_consensus)
28     }
29     (void)bitcoinconsensus_verify_script(random_bytes_1.data(), random_bytes_1.size(), random_bytes_2.data(), random_bytes_2.size(), n_in, flags, err_p);
30     (void)bitcoinconsensus_verify_script_with_amount(random_bytes_1.data(), random_bytes_1.size(), money, random_bytes_2.data(), random_bytes_2.size(), n_in, flags, err_p);
31+
32+    if (n_in > 24386) return;

fanquake commented at 1:03 pm on October 2, 2023:

cc @dergoegge

darosior commented at 1:19 pm on October 2, 2023:

I think we should still call bitcoinconsensus_verify_script_with_spent_outputs when n_in is too large. Just only allocate when it’s not.

(I initially pointed this out in #28539 (comment) but my email client messed up my message, sorry about that.)

brunoerg commented at 2:37 pm on October 2, 2023:

Done, thanks!

brunoerg force-pushed on Oct 2, 2023

brunoerg commented at 2:37 pm on October 2, 2023: contributor

Force-pushed addressing #28539 (review)

darosior commented at 5:35 pm on October 2, 2023: member

https://github.com/bitcoin/bitcoin/actions/runs/6381585890/job/17318337355?pr=28539

Run script_bitcoin_consensus with args [’/Users/runner/work/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-apple-darwin/src/test/fuzz/fuzz’, PosixPath(’/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_seed_corpus/script_bitcoin_consensus’)]Error processing input “/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_seed_corpus/script_bitcoin_consensus/f8cf3d09c66c9cfb4a22baea47b7a2eb697f6b40”

Error processing input “/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_seed_corpus/script_bitcoin_consensus/f8cf3d09c66c9cfb4a22baea47b7a2eb697f6b40”

Target [’/Users/runner/work/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-apple-darwin/src/test/fuzz/fuzz’, PosixPath(’/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_seed_corpus/script_bitcoin_consensus’)] failed with exit code 1

brunoerg force-pushed on Oct 2, 2023

brunoerg commented at 7:40 pm on October 2, 2023: contributor

Force-pushed fixing fuzz testing.

darosior commented at 9:11 am on October 3, 2023: member

ACK 40cab2b663299ddfe669391648584b16b7492d35 (for code changes, haven’t checked the doc carefully)

in src/script/bitcoinconsensus.cpp:136 in 35bcc70c1b outdated

131                                     const unsigned char *txTo        , unsigned int txToLen,
132                                     unsigned int nIn, unsigned int flags, bitcoinconsensus_error* err)
133 {
134+    if (flags & bitcoinconsensus_SCRIPT_FLAGS_VERIFY_TAPROOT) {
135+        return set_error(err, bitcoinconsensus_ERR_SPENT_OUTPUTS_REQUIRED);
136+    }

achow101 commented at 6:53 pm on October 9, 2023:

In 35bcc70c1b44921d7da0207ba4d70a2c114e8ba1 “lib: add Taproot support to libconsensus”

nit: Seems like this check could be pushed into verify_script to avoid having to duplicate it across all of the verify_script_* variations.

brunoerg commented at 7:17 pm on October 9, 2023:

Thanks, if I touch the code again, I address it.

achow101 commented at 6:54 pm on October 9, 2023: member

ACK 40cab2b663299ddfe669391648584b16b7492d35

in src/script/bitcoinconsensus.h:85 in 35bcc70c1b outdated

79@@ -70,6 +80,11 @@ EXPORT_SYMBOL int bitcoinconsensus_verify_script_with_amount(const unsigned char
80                                     const unsigned char *txTo        , unsigned int txToLen,
81                                     unsigned int nIn, unsigned int flags, bitcoinconsensus_error* err);
82 
83+EXPORT_SYMBOL int bitcoinconsensus_verify_script_with_spent_outputs(const unsigned char *scriptPubKey, unsigned int scriptPubKeyLen, int64_t amount,
84+                                    const unsigned char *txTo        , unsigned int txToLen,
85+                                    UTXO *spentOutputs, unsigned int spentOutputsLen,

theStack commented at 11:27 pm on October 11, 2023:

Same as for the other two pointer parameters (scriptPubKey and txTo), I think spentOutputs should also follow const-correctness for consistency and type-safety reasons (otherwise this suggests to the caller that the passed UTXO instance could be modified inside):

0                                    const UTXO *spentOutputs, unsigned int spentOutputsLen,

brunoerg commented at 2:44 pm on October 12, 2023:

done

in src/script/bitcoinconsensus.cpp:95 in 35bcc70c1b outdated

90+                CTxOut tx_out = CTxOut(value, spk);
91+                spent_outputs.push_back(tx_out);
92+            }
93+            if (spent_outputs.size() != tx.vin.size()) {
94+                return set_error(err, bitcoinconsensus_ERR_SPENT_OUTPUTS_MISMATCH);
95+            }

theStack commented at 11:30 pm on October 11, 2023:

more of a nit, but could compare with the input parameter spentOutputsLen instead and do the error checking before the for loop (as early as possible)

brunoerg commented at 2:07 pm on October 12, 2023:

Makes sense, I’m gonna address it.

brunoerg commented at 2:44 pm on October 12, 2023:

done

brunoerg referenced this in commit f939fc74d9 on Oct 12, 2023

brunoerg force-pushed on Oct 12, 2023

brunoerg commented at 2:45 pm on October 12, 2023: contributor

Force-pushed addressing:

brunoerg force-pushed on Oct 12, 2023

brunoerg referenced this in commit 78a7014ef1 on Oct 12, 2023

DrahtBot added the label CI failed on Oct 12, 2023

brunoerg commented at 5:45 pm on October 12, 2023: contributor

CI failure seems unrelated:

0Reading package lists...
1Building dependency tree...
2Reading state information...
3E: Unable to locate package linux-headers-6.2.0-32-generic
4E: Couldn't find any package by glob 'linux-headers-6.2.0-32-generic'
5E: Couldn't find any package by regex 'linux-headers-6.2.0-32-generic'
6Retries exhausted
7Error: building at STEP "RUN bash -c cd /ci_container_base/ && set -o errexit && source ./ci/test/00_setup_env.sh && ./ci/test/01_base_install.sh": while running runtime: exit status 100
8
9Exit status: 100

maflcko commented at 8:04 pm on October 12, 2023: member

Jup, this particular CI failure can be ignored. It should also fix itself on the next push.

fanquake requested review from achow101 on Oct 13, 2023

fanquake requested review from theStack on Oct 13, 2023

fanquake requested review from darosior on Oct 13, 2023

lib: add Taproot support to libconsensus

Co-authored-by: Bruno Garcia <brunoely.gc@gmail.com>

fb0db07e41

docs: link to rust-bitcoinconsensus 70106e0689

docs: add docs for additional libconsensus functions

Co-authored-by: Bruno Garcia <brunoely.gc@gmail.com>

de54882348

docs: add release notes for #28539 c5f2a757d7

fuzz: add coverage for `bitcoinconsensus_verify_script_with_spent_outputs`

Co-authored-by: Antonie Poinsot <darosior@protonmail.com>

ff8e2fc2e2

in src/script/bitcoinconsensus.h:69 in 2665892e2a outdated

65+
66+struct UTXO {
67+    const unsigned char *scriptPubKey;
68+    unsigned int scriptPubKeySize;
69+    int64_t value;
70 };

theStack commented at 11:46 am on October 13, 2023:

0typedef struct {
1    const unsigned char *scriptPubKey;
2    unsigned int scriptPubKeySize;
3    int64_t value;
4} UTXO;

as the current API header leads to an error if compiled with C:

 0$ cat libconsensus_test.c
 1#include "./src/script/bitcoinconsensus.h"
 2
 3int main()
 4{
 5}
 6$ gcc -o libconsensus_test libconsensus_test.c
 7In file included from libconsensus_test.c:1:
 8./src/script/bitcoinconsensus.h:85:43: error: unknown type name 'UTXO'
 9   85 |                                     const UTXO *spentOutputs, unsigned int spentOutputsLen,
10      |                                           ^~~~

brunoerg commented at 11:56 am on October 13, 2023:

Nice find, gonna change it.

brunoerg commented at 12:01 pm on October 13, 2023:

done

brunoerg force-pushed on Oct 13, 2023

brunoerg commented at 12:03 pm on October 13, 2023: contributor

Force-pushed: rebased and addressed #28539 (review).

theStack approved

theStack commented at 12:20 pm on October 13, 2023: contributor

ACK ff8e2fc2e2416f6f3b84cdb40db8ac168596b579

Some follow-up thoughts: on the long-term, I think it would be very nice to have a test that interacts with the libbitcoinconsensus shared library (i.e. .so/.dylib/.dll file, depending on the OS used). This should be doable in form of a functional test without any extra dependencies using Python’s ctypes module, IIUC: https://docs.python.org/3/library/ctypes.html

However, I didn’t manage to even build a shared library so far. Do we still support this in the build system? ./configure --enable-shared doesn’t do anything on my side (tested on Ubuntu 22.04), I keep getting checking whether to build shared libraries... no from the configure script.

DrahtBot removed the label CI failed on Oct 13, 2023

achow101 commented at 4:09 pm on October 13, 2023: member

ACK ff8e2fc2e2416f6f3b84cdb40db8ac168596b579

DrahtBot removed review request from achow101 on Oct 13, 2023

darosior approved

darosior commented at 11:28 am on October 15, 2023: member

re-ACK ff8e2fc2e2416f6f3b84cdb40db8ac168596b579

achow101 merged this on Oct 16, 2023

achow101 closed this on Oct 16, 2023

in doc/release-notes-123.md:1 in ff8e2fc2e2

0@@ -0,0 +1,5 @@
1+Tools and Utilities

fanquake commented at 8:46 am on October 17, 2023:

Probably better to use the actual PR number next time?

Frank-GER referenced this in commit 115b783a05 on Oct 21, 2023

panicfarm commented at 11:24 pm on December 21, 2023: none

A naive question (I am a Rust developer, not a C++ Core dev): taproot soft fork has been in the bitcoin core code since 0.22. But even at the 0.25 release, libconsensus still does not have taproot support. Isn’t libconsensus compiled from the same source as the core binary itself, albeit as a shared library? If this is so, I am confused why it has not had taproot support since 0.22?

sipa commented at 4:26 pm on December 22, 2023: member

@panicfarm libconsensus had certainly contained all the logic for taproot validation ever since Bitcoin Core had it, but it was not exposed through the library interface, for two reasons:

The API requires passing in flags to select which softfork rules to apply to validation. No such flag was exposed for taproot.
Taproot validation requires access to more information that wasn’t present in the libconsensus API (specifically, the UTXO data of all spent UTXOs by a transaction). New functions were needed that allow passing in this information.

lib: add taproot support to libconsensus #28539

Code Coverage

Reviews

Conflicts