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

Taproot support #195

Closed
wants to merge 30 commits into from
Closed

Taproot support #195

wants to merge 30 commits into from

Conversation

louisinger
Copy link
Contributor

@louisinger louisinger commented May 31, 2022

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

  • In order to use the latest changes of 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 ?).
  • btcutils packages are now included in btcd, that's why some imports have been modified

2. Elements taproot

  • taproot package extends the btcd/txscript/taproot file and reuses the same unit test file taproot_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-lib
  • transaction pkg implements HashForWitnessV1.
  • some changes in address pkg (getScriptType, toOutputScript etc...).
  • e2e nigiri testing for taproot (keypath & tapscript).
  • add the new elements tapscript opcodes constants.

About new opcodes: It is quite possible to use the new opcodes now. However, for better introspection support, I think we should extend the scriptBuilder from txscript with some elements-specific functions like AddAssetHash or AddIndex etc.

it closes #172

@altafan @tiero please review

@tiero tiero requested a review from altafan May 31, 2022 11:49
Copy link
Collaborator

@altafan altafan left a 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?

Comment on lines +122 to +128
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")
}
Copy link
Collaborator

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?

Copy link
Contributor Author

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

payment/p2tr.go Outdated Show resolved Hide resolved
transaction/transaction.go Outdated Show resolved Hide resolved
transaction/transaction.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@altafan altafan left a 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.

@altafan
Copy link
Collaborator

altafan commented May 31, 2022

About new opcodes: It is quite possible to use the new opcodes now. However, for better introspection support, I think we should extend the scriptBuilder from txscript with some elements-specific functions like AddAssetHash or AddIndex etc.

Don't think this is in the scope of the PR. We should open a dedicated ticket and discuss it there.

@tiero tiero requested a review from sekulicd May 31, 2022 21:23
Copy link
Contributor

@sekulicd sekulicd left a 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.

payment/p2tr.go Show resolved Hide resolved
payment/p2tr.go Outdated Show resolved Hide resolved
taproot/taproot.go Outdated Show resolved Hide resolved
taproot/taproot.go Outdated Show resolved Hide resolved
taproot/taproot.go Outdated Show resolved Hide resolved
taproot/taproot.go Outdated Show resolved Hide resolved
taproot/taproot_e2e_test.go Show resolved Hide resolved
if err != nil {
t.Fatal(err)
}

Copy link
Contributor

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

Copy link
Contributor Author

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"

@louisinger louisinger closed this Jun 10, 2022
This was referenced Jun 10, 2022
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.

Support Taproot & Tapscript
4 participants