Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: add taproot support to libconsensus #28539

Merged
merged 5 commits into from Oct 16, 2023

Conversation

brunoerg
Copy link
Contributor

Grabbed from #21158. Closes #21133.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 26, 2023

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 added a commit to brunoerg/bitcoin that referenced this pull request Sep 26, 2023
@fanquake
Copy link
Member

cc @darosior

@darosior
Copy link
Member

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

@brunoerg
Copy link
Contributor Author

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

Yes!

brunoerg added a commit to brunoerg/bitcoin that referenced this pull request Sep 28, 2023
brunoerg added a commit to brunoerg/bitcoin that referenced this pull request Sep 28, 2023
brunoerg added a commit to brunoerg/bitcoin that referenced this pull request Sep 29, 2023
@sipa sipa requested review from sipa and theStack and removed request for theStack September 29, 2023 12:16
@brunoerg brunoerg marked this pull request as ready for review September 29, 2023 12:31
@brunoerg
Copy link
Contributor Author

Ready for review!

src/test/script_tests.cpp Outdated Show resolved Hide resolved
src/test/script_tests.cpp Outdated Show resolved Hide resolved
brunoerg added a commit to brunoerg/bitcoin that referenced this pull request Sep 29, 2023
@brunoerg
Copy link
Contributor Author

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

src/test/script_tests.cpp Outdated Show resolved Hide resolved
brunoerg added a commit to brunoerg/bitcoin that referenced this pull request Sep 29, 2023
@brunoerg
Copy link
Contributor Author

Addressed #28539 (comment)

brunoerg added a commit to brunoerg/bitcoin that referenced this pull request Oct 12, 2023
@brunoerg
Copy link
Contributor Author

CI failure seems unrelated:

Reading package lists...
Building dependency tree...
Reading state information...
E: Unable to locate package linux-headers-6.2.0-32-generic
E: Couldn't find any package by glob 'linux-headers-6.2.0-32-generic'
E: Couldn't find any package by regex 'linux-headers-6.2.0-32-generic'
Retries exhausted
Error: 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

Exit status: 100

@maflcko
Copy link
Member

maflcko commented Oct 12, 2023

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

jrawsthorne and others added 5 commits October 13, 2023 08:55
@brunoerg
Copy link
Contributor Author

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

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK ff8e2fc

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.

@achow101
Copy link
Member

ACK ff8e2fc

@DrahtBot DrahtBot removed the request for review from achow101 October 13, 2023 16:09
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK ff8e2fc

@achow101 achow101 merged commit 90f7d8a into bitcoin:master Oct 16, 2023
16 checks passed
@@ -0,0 +1,5 @@
Tools and Utilities
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better to use the actual PR number next time?

@panicfarm
Copy link

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
Copy link
Member

sipa commented Dec 22, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Taproot support to libconsensus
10 participants