multi: implement BIP 341 and 342 a.k.a complete taproot and tapscript consensus verification logic #1787
Conversation
|
In script.go, calls to |
There was a problem hiding this comment.
Great work putting all of this together! I've completed an initial pass in which I mostly focused on spec compat.
The PR will become a bit unwieldy once tests have been added, I think it would be useful to break it up into smaller parts to help reviewers focus on some of the independent changes being added. An example breakdown could be (each point represents its own PR):
- Prep work for Taproot integration
- Taproot key spend path
- Control block construction/verification
- Taproot script spend path (including sig opcode changes)
Roasbeef
force-pushed
the
taproot-impl
branch
from
da170c1
to
e0d13b7
Compare
There was a problem hiding this comment.
Started some end-to-end integrations (see https://github.com/guggero/btcwallet/tree/schnorr-taproot and https://github.com/guggero/lnd/tree/schnorr-taproot-signet) and found a few small-ish issues. Going to do a more in-depth review of this PR again tomorrow, just leaving those comments here.
Roasbeef
force-pushed
the
taproot-impl
branch
from
52006f8
to
c7a457a
Compare
Roasbeef
force-pushed
the
taproot-impl
branch
from
c7a457a
to
dd86580
Compare
Roasbeef
force-pushed
the
taproot-impl
branch
from
dd86580
to
e95aafc
Compare
Pull Request Test Coverage Report for Build 1962253109
💛 - Coveralls |
|
Just pushed a revamped version which addresses all existing review comments, and also commits the I have some smaller TODO's to clean up still, like better error messages, but I'm pretty confident/happy in the current diff. Still entertaining the option of splitting this up into smaller PRs as with the latest diff (ignore all the files committed as those are just test vectors) the PR size has swelled a bit. |
|
The little I've been looking through, this looks great! Splitting up into smaller PRs would help a lot, if you do have the inclination ^^ |
There was a problem hiding this comment.
Did a full review (third round) and was able to end-to-end test key spend, script (single leaf and multiple leaf) execution, including OP_CHECKSIG with schnorr signatures.
Found a few small issues but all in all I'd say this is very close to be ready! Given we have superb coverage of the validation code thanks to the test cases from bitcoind I'd say this can be merged very soon.
Really great work on this, the code is super clean and easy to read/understand!
|
I fixed a few things I mentioned in the comments here: https://github.com/guggero/btcd/tree/1787-fixes feel free to incorporate directly into this PR. |
Roasbeef
force-pushed
the
taproot-impl
branch
2 times, most recently
from
417425c
to
f5186a5
Compare
|
@guggero thanks! Added those in, and also working on more tests to ensure we accept sigs/witnesses we generate. |
There was a problem hiding this comment.
End-to-end tested this latest iteration in lnd and all test cases were successful. Of course more unit tests are always great to get more coverage on the signing part.
Since the verification part is covered very well, I'm giving my LGTM so we can move forward with the depending PRs.
| opts..., | ||
| ) | ||
| if err != nil { | ||
| // TODO(roasbeef): propagate the error here? |
There was a problem hiding this comment.
Perhaps worth cross-referencing with what bitcoind does in this case?
There was a problem hiding this comment.
calcTaprootSignatureHashRaw only returns en error if:
- An invalid sighash is used, bitcoind does the same here: https://github.com/bitcoin/bitcoin/blob/623745ca74cf3f54b474dac106f5802b7929503f/src/script/interpreter.cpp#L1716
- An index is passed in that's greater than the total number of actual inputs
- For this, earlier in their pipeline there's an assertion to fail early for this case. We have a return clause here as we expose this function to consumers of the library, which may pass in an invalid index.
The other error paths there are just error returns from the io.Writer when creating the sig message, which in practice don't return an error.
txscript/taproot.go
Outdated
| // remaining bytes to be a multiple of the node size, which is 32 | ||
| // bytes. | ||
| case (len(ctrlBlock)-ControlBlockBaseSize)%ControlBlockNodeSize != 0: | ||
| return nil, fmt.Errorf("invalid max block size") |
There was a problem hiding this comment.
Nit: this is the same error text as above.
There was a problem hiding this comment.
Will resolve these in a final pass to insert proper errors.
Roasbeef
force-pushed
the
taproot-impl
branch
from
f5186a5
to
4629cbd
Compare
|
Responded to the latest round of comments! Also added a new On the |
In this commit, we use the recently added checksig verifiers to validate signatures for pre-segwit, and segwit v0 scripts.
In this commit, we add two new functions: one for signing a raw top-level taproot keyspend, and another for generating a valid witness for a keyspend.
In this commit, we add the initial verification logic for top-level taproot keyspends. Keyspends use the base BIP 341 sighash digest and don't require any tapscript level functionality for validation.
We'll use this to examine if a script has any OP_SUCCESS op codes during pre-processing before we attempt full tapscript execution.
In this commit, we add a new struct to represent the ControlBlock structure used to feed in the tapscript leaf inclusion proof into the witness tack. The `ParseControlBlock` parses a would-be control block and returns an error if it's incorrectly formatted.
In this commit, we add a new function to verify the taproot merkle commitment of a given tapscript leaf. Along the way we add some helper functions which can be used to construct a taproot output given the raw script root.
In this commit, we add a new function `RawTxInTapscriptSignature` that will be used to generate signatures in the _tapscript_ context. Note that this differs from top-level taproot as a distinct sighash is used, and we _always_ accept a root hash to perform the proper tweak.
…sses In this commit, we add a new AssembleTaprootScriptTree function that given a list of tapscript leaves, generates a valid tapscript root, along with the auxiliary proof data needed to spend each output.
In this commit, we use the recently added control block and script tree verification+generation routines to implement full script path verification within the VM. This includes verifying the script reveal commitment, and recursing one layer deeper to execute the revealed witness script as specified by BIP 342.
… position We'll need this to properly generate the sighash during tapscript validation later
In this commit, we implement the new checksig semantics as part of
tapscript validation. Namely:
* OP_CHECKSIGVERIFY no longer pops the OP_TRUE off the stack (TODO(roasbeef): verify))
* the new sig ops semantics are added where each sig deducts 50 from a
starting budget of 50+the weight of the witness
* NULLFAIL is always enforced, meaning invalid sigs MUST be an empty sig array
In this commit, we implement OP_CHECKSIGADD which replaces OP_CHECKMULTISIG* in the tapscript execution environment.
In this commit, we add a total of 2760 taproot reference tests generated by the bitcoind functional tests located at: https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_taproot.py. The tests aren't deterministic (fresh private keys are generated), so we time we go to update the set of tests, we'll end up with fresh hashes (the file name is the sha1 of the raw json test) and tests.
In this commit, we add the deployment parameters of taproot as specified in the deployment section of BIp 341: https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#deployment. Take note of the custom activation threshold, as well as the specified min activation heights for mainnet only.
This PR includes some changes to them, so we'll need to use a temporary replace directives to ensure the build passes.
🚨 Just received word from Merge Control: this PR is a go!! 🚨 |
In this PR, we implement full validation logic for BIP 341 and BIP 342. Along the way, we've created a series of utility functions to be used in wallet libraries to handle things like parsing the control block, generating merkle tapscript inclusion proofs, and computing the two sighash variants introduced in the BIPs.
See each commit message for a detailed description w.r.t the scope of the incremental changes, as well as notes for reviewers w.r.t improvements to the logic/routines or things to doubly verify.
This PR builds on a series of PRs including:
Steps to Graduate from a Draft PR
v2module forbtcec.bitcoindimplementation (the BIPs contain no test vectors, but they have some good coverage saved from their fuzz testing) to ensure the logic implemented is compliant with consensus rules.Fixes everything in #1735, other than the
btcwalletspecific components.