-
Notifications
You must be signed in to change notification settings - Fork 12
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
Taproot support #195
Taproot support #195
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test vectors for changes in address.go?
if payload.Program == nil { | ||
return nil, errors.New("unable to compute taproot's tweaked key from payment data") | ||
} | ||
|
||
if len(payload.Program) != 32 { | ||
return nil, errors.New("taproot's tweaked key has wrong length") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need these post checks? In general, this lib's methods make sure that everything's ok before doing any operation "by design", instead of performing post checks.
Is there a way to make these checks over the taproot data before doing the magic in order to make sure that at this point payload.Program
is exactly a 32-bytes buffer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need the first one if for any reasons the Program has not been computed from data, better to return a error instead of nil isn't it ?
about the length, it's almost the same reason, it's a kind of sanity check before returning the program (it ensures that if len is not 32, it won't fail during address computation)
But I agree, theorically the parse functions on taproot data should "validate" before taproot magic so maybe not useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job so far!
Let's increase the number of (failing) tests to discover and fix "easy" bugs before merging.
Don't think this is in the scope of the PR. We should open a dedicated ticket and discuss it there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if we can add test cases examples with more complex scripts(multipath) for learning/doc purpose.
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we verify signature? Is VerifyTaprootKeySpend/VerifyTaprootLeafCommitment? same in other test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessery in my opinion. VerifyTaprootLeafCommitment is tested in taproot pkg and broadcast is already a "sig check"
This PR adds Taproot/Tapscript support for elements, greatly inspired by the work already done on btcd: btcsuite/btcd#1787
1. bump btcd to latest, drop btcutils
btcsuite/btcd
(including taproot PRs + schnorr support), go mod is pointing the latest commit hash. It seems to me that taproot changes has not been released yet (Am I wrong ?).2. Elements taproot
taproot
package extends thebtcd/txscript/taproot
file and reuses the same unit test filetaproot_test.go
. Lot of copy-paste there, the logic is the same on Elements. The major change is about BIP-341 hash tags (suffixed by "/elements" on Liquid).payment
pkg implements several new functions in order to support P2TR payments (unconfidential/confidential).blech32
supports segwit v1 encoding (bech32m) + BONUS: add unit tests from liquidjs-libtransaction
pkg implementsHashForWitnessV1
.address
pkg (getScriptType, toOutputScript etc...).it closes #172
@altafan @tiero please review