lib: Add Taproot support to libconsensus by jrawsthorne · Pull Request #21158 · bitcoin/bitcoin

jrawsthorne · 2021-02-11T22:59:06Z

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

ariard

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)

src/script/bitcoinconsensus.cpp

jrawsthorne · 2021-02-14T01:46:03Z

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?

ariard · 2021-02-14T22:06:25Z

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

doc/shared-libraries.md

ariard

Code Review ACK 28245a8, thanks for the changes

Up to you to take the nit.

doc/shared-libraries.md

DrahtBot · 2021-03-02T11:39:28Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Eliminate Signature Checker/Creator Ambiguity w/ LIFETIMEBOUND References #22440 by JeremyRubin

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.

DrahtBot · 2021-03-15T16:50:05Z

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

ariard · 2021-04-16T16:50:55Z

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

ariard · 2021-04-26T17:33:19Z

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

jrawsthorne · 2021-04-26T17:44:14Z

Thanks @ariard, rebased

ariard · 2021-04-30T13:59:49Z

Code Review ACK 096ab5c.

Diff since last ack is swallowing release-note changes.

ariard · 2021-04-30T14:00:53Z

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

sipa

utACK 698391966afd0004d6d31fc590ffb8e4222408b8

src/test/script_tests.cpp

src/script/bitcoinconsensus.h

jrawsthorne · 2021-06-13T13:29:22Z

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

luke-jr · 2021-06-23T01:56:04Z

src/script/bitcoinconsensus.cpp

+        std::vector<CTxOut> spent_outputs;
+        if (spentOutputs != nullptr) {
+            TxInputStream spent_outputs_stream(PROTOCOL_VERSION, spentOutputs, spentOutputsLen);
+            spent_outputs_stream >> spent_outputs;


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

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

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

luke-jr · 2021-06-23T01:56:52Z

doc/shared-libraries.md

+- `int64_t amount` - The amount spent in the input
+- `const unsigned char *txTo` - The transaction with the input that is spending the previous output.
+- `unsigned int txToLen` - The number of bytes for the `txTo`.
+- `const unsigned char *spentOutputs` - Previous outputs spent in the transaction


This leaves me guessing how to call this function.

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

DrahtBot · 2021-07-21T10:00:55Z

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

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

DrahtBot · 2021-12-22T12:28:55Z

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.

maflcko · 2022-04-29T14:17:52Z

Marked "up for grabs"

DrahtBot added the Consensus label Feb 11, 2021

jrawsthorne force-pushed the libconsensus_taproot branch 3 times, most recently from 512c9ca to 883cac2 Compare February 12, 2021 23:38

jrawsthorne marked this pull request as ready for review February 12, 2021 23:39

ariard reviewed Feb 13, 2021

View reviewed changes

src/script/bitcoinconsensus.cpp Outdated Show resolved Hide resolved

src/script/bitcoinconsensus.cpp Outdated Show resolved Hide resolved

src/script/bitcoinconsensus.cpp Outdated Show resolved Hide resolved

jrawsthorne force-pushed the libconsensus_taproot branch from 883cac2 to 0108629 Compare February 14, 2021 23:16

SomberNight reviewed Feb 17, 2021

View reviewed changes

doc/shared-libraries.md Outdated Show resolved Hide resolved

doc/shared-libraries.md Outdated Show resolved Hide resolved

jrawsthorne force-pushed the libconsensus_taproot branch from 0108629 to 28245a8 Compare February 17, 2021 13:25

jrawsthorne requested a review from ariard February 18, 2021 15:10

ariard reviewed Feb 23, 2021

View reviewed changes

doc/shared-libraries.md Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Mar 2, 2021

Deal with missing data in signature hashes more consistently #21330

Merged

DrahtBot mentioned this pull request Mar 4, 2021

Basic Taproot signing support for descriptor wallets #21365

Merged

DrahtBot mentioned this pull request Apr 4, 2021

cli: create -addrinfo #21595

Merged

DrahtBot added the Needs rebase label Apr 13, 2021

jrawsthorne force-pushed the libconsensus_taproot branch from 071d933 to 7c85ec9 Compare April 15, 2021 18:17

DrahtBot removed the Needs rebase label Apr 15, 2021

DrahtBot added the Needs rebase label Apr 20, 2021

jrawsthorne force-pushed the libconsensus_taproot branch from 7c85ec9 to 096ab5c Compare April 26, 2021 17:43

DrahtBot removed the Needs rebase label Apr 26, 2021

jrawsthorne mentioned this pull request May 16, 2021

libbitcoinconsensus.dylib in MacOS releases is broken #21794

Closed

DrahtBot mentioned this pull request May 20, 2021

cli: Improve -getinfo return format #21832

Merged

DrahtBot mentioned this pull request Jun 6, 2021

doc: update tor.md, release notes with removal of tor v2 support #22172

Merged

DrahtBot added the Needs rebase label Jun 12, 2021

refactor: Rename variables in preparation for generic deserialization

78447f3

jrawsthorne force-pushed the libconsensus_taproot branch from 096ab5c to 6983919 Compare June 12, 2021 20:45

DrahtBot removed the Needs rebase label Jun 12, 2021

sipa reviewed Jun 13, 2021

View reviewed changes

src/test/script_tests.cpp Outdated Show resolved Hide resolved

src/script/bitcoinconsensus.h Outdated Show resolved Hide resolved

jrawsthorne added 3 commits June 13, 2021 14:07

lib: Add Taproot support to libconsensus

d82e5de

docs: Add docs for additional libconsensus functions

3dd932e

docs: Link to rust-bitcoinconsensus

c23996a

jrawsthorne force-pushed the libconsensus_taproot branch from 6983919 to c23996a Compare June 13, 2021 13:08

luke-jr suggested changes Jun 23, 2021

View reviewed changes

DrahtBot mentioned this pull request Jul 14, 2021

Eliminate Signature Checker/Creator Ambiguity w/ LIFETIMEBOUND References #22440

Closed

DrahtBot added the Needs rebase label Jul 21, 2021

maflcko added the Up for grabs label Apr 29, 2022

fanquake closed this May 12, 2022

theStack mentioned this pull request Apr 22, 2023

Consider Removing Message Signing #27515

Closed

bitcoin locked and limited conversation to collaborators May 12, 2023

fanquake removed Up for grabs Needs rebase labels Sep 27, 2023

Conversation

jrawsthorne commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrawsthorne commented Feb 14, 2021

Uh oh!

ariard commented Feb 14, 2021

Uh oh!

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

DrahtBot commented Mar 15, 2021

Uh oh!

ariard commented Apr 16, 2021

Uh oh!

ariard commented Apr 26, 2021

Uh oh!

jrawsthorne commented Apr 26, 2021

Uh oh!

ariard commented Apr 30, 2021

Uh oh!

ariard commented Apr 30, 2021

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jrawsthorne commented Jun 13, 2021

Uh oh!

luke-jr Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

jrawsthorne Jul 4, 2021

Choose a reason for hiding this comment

Uh oh!

sipa Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

luke-jr Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

jrawsthorne Jul 4, 2021

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 21, 2021

Uh oh!

DrahtBot commented Dec 22, 2021

Uh oh!

maflcko commented Apr 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jrawsthorne commented Feb 11, 2021 •

edited

Loading

DrahtBot commented Mar 2, 2021 •

edited

Loading