-
Notifications
You must be signed in to change notification settings - Fork 576
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: Add taproot address and signing capabilities #792
Conversation
Dependent PR has been merged. |
|
||
replace github.com/btcsuite/btcd/btcec/v2 => github.com/guggero/btcd/btcec/v2 v2.0.0-20220214155024-51fd177d32c0 | ||
|
||
replace github.com/btcsuite/btcwallet/wallet/txauthor => ./wallet/txauthor |
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.
FWIW this was removed in a prior PR, but I can see how it's helpful to fully integrate things in one swoop. Otherwise, we can make a new PR that adds the new txscript
stuff, then does a tag, then to have it integrated here ultimately.
4ec92b0
to
203df47
Compare
6e96598
to
aca9822
Compare
da559a8
to
6a78190
Compare
6a78190
to
ff7eeb1
Compare
The PR is now ready for review! I'll take a look at the linter and unit test failures in a bit. |
c253f14
to
6d1c60c
Compare
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.
Excellent PR!
Completed an initial review pass through the latest set of commits. I like how we don't bind the caller/user into a particular way and allow them to either use a single script path or just import the entire tree for tracking/spending purposes. This'll really be useful once we start to implement more advanced multi-sig and other script operations using the wallet.
Ultimately, we may want to start (from an lnd
perspective) also importing our scripts into the wallet, but possibly with a non-default account type. In any case, that's beyond the scope of this PR.
Only lingering design question IMO is if we really need to distinguish between pure-keyspend and a mixed variant for addresses. I guess as is, we assume usage of BIP 86, so anything that doesn't use that should use this new import API instead.
6d1c60c
to
b42468a
Compare
f656436
to
2b6e334
Compare
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.
Amazing PR!
The diff is looking really good from my PoV now. I think the only two things we'll want to do as follow up are:
- Ensure that on the lnd side, we have proper recovery working, which may need some additional modifications to the recovery/rescan logic. I think since it works at mostly a higher level and lets the waddrmgr derive the addresses it should be ok. It can also make optimizations like not scan for taproot addrs if an address was created before a certain height, but that may generate some false positives.
- (this one is a bonus imo) implement the PSBT parsing and assembly functionality.
75369b4
to
cb37aec
Compare
As a preparation for when the identifier of a script isn't the hash of a script in every case anymore, we extract that part out of the importScriptAddress method and rename it. Pay-to-Taproot script addresses use the x-only coordinates of a tweaked public key as the address identifier.
To make integration of the upcoming taproot script address type a bit easier, we allow returning a ManagedScriptAddress interface type in newWitnessScriptAddress so we can re-use that function for the taproot script address too.
cb37aec
to
89167fe
Compare
Rebased after the dependent PR has been merged. |
89167fe
to
85b35fa
Compare
I added that integration test to lightningnetwork/lnd#6263, no issues found. I also added a commit that exposes the I think with those two things tested end-to-end, this PR is ready to be merged. |
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.
Great job on this one @guggero, really excited to see this ready to be merged 🎉
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.
Great PR! Did a high level santiy pass, looks great.
I think that the distinction between TaprootAddress
and TaprootScript
discussed here will be useful, the two different use cases make sense to me.
) | ||
} | ||
|
||
// importScriptAddress imports a new pay-to-script or pay-to-witness-script | ||
// address. | ||
func (s *ScopedKeyManager) importScriptAddress(ns walletdb.ReadWriteBucket, | ||
script []byte, bs *BlockStamp, addrType AddressType, | ||
identity Identity, script []byte, bs *BlockStamp, addrType AddressType, |
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.
Why do we need to pass the identity closure in here vs just []byte
?
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.
See comment here: #792 (comment)
Gives us the ability to use nicely named, pre-defined identity functions.
8de604d
to
15b5f8a
Compare
Taproot: Add taproot address and signing capabilities
Taproot: Add taproot address and signing capabilities
Depends on lightningnetwork/lnd#6285 (for the TLV module).Adds taproot key spend and script spend addresses.
Merge order for this saga of PRs is: