multi: changes to the taproot HTLC required for both client and server #477
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bhandras
changed the title
bhandras
force-pushed
the
taproot-htlc
branch
7 times, most recently
from
274ec4c to
36da078
Compare
bhandras
force-pushed
the
taproot-htlc
branch
3 times, most recently
from
b332b53 to
61f729c
Compare
bhandras
force-pushed
the
taproot-htlc
branch
7 times, most recently
from
b947b1c to
fe94c3b
Compare
bhandras
changed the title
bhandras
force-pushed
the
taproot-htlc
branch
3 times, most recently
from
3583191 to
f4b2395
Compare
1 task
bhandras
changed the title
guggero
reviewed
Jun 2, 2022
Contributor
guggero
left a comment
guggero
left a comment
There was a problem hiding this comment.
Great cleanup and refactors! Only a few minor comments, nothing major.
1 task
bhandras
force-pushed
the
taproot-htlc
branch
2 times, most recently
from
872f647 to
b406ff9
Compare
Member
Author
|
@arshbot ping |
carlaKC
approved these changes
Jun 9, 2022
| var schnorrSenderKey, schnorrReceiverKey [32]byte | ||
| copy(schnorrSenderKey[:], schnorr.SerializePubKey(senderPubKey)) | ||
| copy(schnorrReceiverKey[:], schnorr.SerializePubKey(receiverPubKey)) | ||
| aggregateKey, _, _, err := musig2.AggregateKeys( |
Contributor
There was a problem hiding this comment.
nice, much cleaner than passing it all the way through everywhere 🤦♀️
Member
Author
There was a problem hiding this comment.
Keys.. Keys everywhere...
sweep/sweeper.go
Outdated
| } | ||
|
|
||
| // We need our previous outputs for taproot spends, and there's no | ||
| // harm including them for segwit v0, so we always inlucde our prevOut. |
Contributor
There was a problem hiding this comment.
s/inlucde/include
This is definitely my typo, I can feel it :')
Use of the Script() function is problematic when we introduce taproot because our script will vary depending whether we use keyspend or a tapleaf spend path (and on the tapleaf spent). This has not previously been a problem for segwitv0 scripts, because they contain all of the logical branches for each of our spend conditions in a single script. This commit prepares for removal of the Script() function by moving our address/pkScript/sigScript generation (which need Script()) into each script's implementation of the HtlcScript interface so that they have access to the script directly.
Taproot spends require a different sighash, so we update our HtlcScript interface to provide the appropriate sighash when sweeping. We also add distinct Timeout/Success Script functions to allow for tapleaf spends which have different locking scripts for different paths. Note that the timeout and success paths will be the same for segwit v0 htlcs, because it has a single branched script containing all spend paths. In future iterations, this differentiation of claim scripts can also be used to use musig2 to collaboratively keyspend P2TR htlcs with the server. This script can be expressed as PriorityScript (because we'll try to keyspend as a priority, and then fall back to a tap leaf spend). As we've done here, segwit v0 spends would just return their single script for PriorityScript, and the claim would be no different from our other claims.
Member
Author
Thanks for the review, welcome back :) |
Contributor
Came for the nits, stayed for the musig2 😜 |
arshbot
approved these changes
Jun 9, 2022
Contributor
arshbot
left a comment
arshbot
left a comment
There was a problem hiding this comment.
LGTM! I really like this approach of abstracting away the output type logic so each type can figure out how to come up with the needed assets. Gives the right amount of flexibility because each htlc type conforms to lockingConditions, but doesn't commit to a path
| } | ||
|
|
||
| // Script is not implemented, but necessary to conform to interface. | ||
| func (h *HtlcScriptV3) Script() []byte { |
Contributor
There was a problem hiding this comment.
This is certainly a much cleaner way of going about it
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a few changes to the already existing taproot HTLC code to make it simpler to integrate.