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
[3/?] - lnwire: add new TLV record types + messages for taproot chans #7331
[3/?] - lnwire: add new TLV record types + messages for taproot chans #7331
Conversation
eec8e5f
to
3bc1128
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.
First pass done! Looking slick 🔥
My main initial confusion was due to this being a bit out of sync with the current spec proposal doc.
But other than that, looks good & was easy to follow
@ellemouton I merged the PR to my BOLT fork that this was baed on. Things should be synced now. |
3bc1128
to
4874fc4
Compare
Pushed up a new version. Things should be building now, and you can use my spec PR as a base for the review. PTAL @ellemouton @yyforyongyu. |
4874fc4
to
3006325
Compare
Fixed some more API test/compilation issues in the latest diff. |
296569c
to
ece355a
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.
LGTM 🚰 🥕
Ah - actually - the |
ece355a
to
2df0b47
Compare
@ellemouton fixed! Was using the wrong sig parsing for the |
Review ping @yyforyongyu. |
2df0b47
to
6852b0a
Compare
@@ -221,6 +221,14 @@ const ( | |||
// TODO: Decide on actual feature bit value. | |||
ScriptEnforcedLeaseOptional FeatureBit = 2023 | |||
|
|||
// SimpleTaprootChannelsRequred is an required bit that indicates the | |||
// node is able to create unadvertised taproot-native channels. | |||
SimpleTaprootChannelsRequired = 80 |
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 there be a reference to 'unadvertised' in the feature bit name?
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.
Nope, good call out, removed.
sigTypeECDSA sigType = iota | ||
|
||
// sigTypeSchnorr represents a schnorr signature. | ||
sigTypeSchnorr |
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.
If you add schnorr in a separate commit, it is clear what part of the change set is just rename/refactor and what is actually new. I think that would help with review.
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.
If we want the entire commit to compile, then we need all the other API changes coupled in as well.
type Sig struct { | ||
bytes [64]byte | ||
|
||
sigType sigType |
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.
How does the chosen approach compare to creating a new SchnorrSig
type?
I suppose it will become clear in later prs in this series why it is good to keep a single Sig
struct.
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.
If we did that, then for all the instances where either sig type can be used (closing signed, commit_sig, etc) we'd need to add this other type along side things, then have other methods to check which was set, then extract the sig from that.
There's another option where we make those messages generic (type params), but then this bleeds into all the APIs a bit, as any area we're storing one of those fields needs to mind the type param (ChanCloser[schnorr.signature]
).
This approach takes into account that they're actually the same set of bytes and are serialized identically on the wire (just the raw 64 bytes). The main quirk is the ForceSchnorr
method. We need this as in certain situations, we need to be able to extract a schnorr signature from an opaque lnwire.Sig
type. This comes up when we're going to validate a second level HTLC, and we know it's a taproot channel, so we need a schnorr sig.
Alternatively, we can add a new lnwire.Sig
interface, then have an implementation for ECDSA and schnorr. I started doing this initially, but realized that later up the stack (when you need to actually use the sigs), we still need methods to obtain the concrete types (we go indirectly thru input.Signature
to do that atm).
So to summarize, this approach is closer to how things are in the spec (we overload the [64]byte
on the wire, and context is needed to obtain the proper concrete type). We can try generics (type params), but that complicates the API for seemingly minimal gain. We can also try to make a new interface for lnwire.Sig
here similar to input.Signature
-- my gut tells me YAGNI tho, as this is only the second new signature type in over a decade of Bitcoin's existence.
|
||
// nonceTypeEncoder is a custom TLV encoder for the Musig2Nonce type. | ||
func nonceTypeEncoder(w io.Writer, val interface{}, buf *[8]byte) error { | ||
if v, ok := val.(*Musig2Nonce); ok { |
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.
In other PRs, I found it better readable to handle the !ok case in the if block.
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.
Seems like minor preference? Everywhere else I've used TLV, I used this pattern. See the channel type (one of the first instances we used it): https://github.com/lightningnetwork/lnd/blob/master/lnwire/channel_type.go#L34-L44
|
||
if v, ok := val.(*PartialSigWithNonce); ok { | ||
sigBytes := v.Sig.Bytes() | ||
if _, err := w.Write(sigBytes[:]); err != nil { |
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.
Can the PartialSig
type encoder be called here? Maybe over-engineering.
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 sure if we're looking at the same diff, but partialSig.Encode
ends up calling this.
} | ||
|
||
if len(tlvRecords) != 0 { | ||
f.ExtraData = tlvRecords |
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.
Interesting. For the inbound fee channel update message extension, I left parsing of the tlv records to the user of these wire types. But perhaps it's better to do it here despite the duplication in memory.
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.
So this is a pattern left over from when we needed to start storing the opaque TLV for gossip messages: if we needed to encode/decode TLV, then we also set the ExtraData
field as it might contain TLV keys we don't actually know about.
c.LocalNonce = &localNonce | ||
} | ||
|
||
if len(tlvRecords) != 0 { |
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 actually this change and not just always store the tlv data as is as was done previously?
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.
Have the same question. tho I prefer the current approach as it separates tlv data from "normal" data, which enables us to maybe add a dedicated tlv reader/writer for these messages.
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 above, this ensures that if we don't understand certain types then we still end up writing them to disk for future broadcast.
IIUC, we also need to do this as otherwise the quick check tests start failing (raw bytes don't match the struct and other way around).
Just to sync up on mental models here: this sets the ExtraData
to what it would look like right after the call to Encode
. Otherwise, the struct that had Encode
called on it would have ExtraData
populated to raw TLV, while the struct that had Decode
on it wouldn't. This means a strict equality check fails.
} | ||
|
||
var tlvRecords ExtraOpaqueData | ||
if err := ReadElements(r, &tlvRecords); err != nil { |
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.
Can't you add &tlvRecords
to the ReadElements
arg list above?
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 question...🤔
Mostly just following the pattern of other areas we use TLV like OpenChannel
(for the channel type field): https://github.com/lightningnetwork/lnd/blob/master/lnwire/open_channel.go#L282-L288
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.
Smooth! And sorry that missed the ping🤦🏻 My main question is the addition of the shutdown nonce, not sure about I understand it. Other than that a few typos and nits, then this is good to go!
c.LocalNonce = &localNonce | ||
} | ||
|
||
if len(tlvRecords) != 0 { |
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.
Have the same question. tho I prefer the current approach as it separates tlv data from "normal" data, which enables us to maybe add a dedicated tlv reader/writer for these messages.
|
||
const ( | ||
// ShutdownNonceRecordType is the type of the shutdown nonce TLV record. | ||
ShutdownNonceRecordType = 8 |
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.
A bit confused about this nonce. Isn't it the same as NonceRecordType
since they all contain 66 bytes of public nonces? My understanding is the next_local_nonce
can just be named to musig2_nonce
with type 4 and it can be used inside shutdown
message.
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.
The LDK reviewers wanted to use distinct types here (compared to that sent in the chan open) messages citing a diff context/semantics.
@@ -544,6 +586,16 @@ func TestLightningWireProtocol(t *testing.T) { | |||
return | |||
} | |||
|
|||
// 1/2 chance to attach a partial sig. | |||
if r.Intn(2) == 0 { |
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.
hmmm don't enjoy too much with this random approach as it may give us flakes. I think the test is better to fail deterministically than pass randomly. Anyway non-blocking tho as I see it's an addition over the original tests. Maybe we will fix it someday.
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.
If we want to go in this direction, then I think we'd overhaul all the tests to do the same. I originally added both everywhere, as it showed some bugs that would come up when only some of the TLV fields were present.
@Roasbeef, remember to re-request review from reviewers when ready |
Note that I've changed the base branch to the new taproot staging branch as mentioned in: #6691 (comment) |
6852b0a
to
3a6f8e9
Compare
New version pushed, PTAL. |
Fixed a linter issue. Forces us to have a new line in the fuzz pacakge. |
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.
The last commit needs to be squashed, otherwise this is good to go⚡️
signBase32, err := bech32.ConvertBits(append(sig[:], recoveryID), 8, 5, true) | ||
signBase32, err := bech32.ConvertBits( | ||
append(sig.RawBytes(), recoveryID), | ||
8, 5, true, |
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.
nit: seems it could be fit into one line
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.
this commit needs to be squashed
In this commit, we update the Sig type to support ECDSA and schnorr signatures. We need to do this as the HTLC signatures will become schnorr sigs for taproot channels. The current spec draft opts to overload this field since both the sigs are actually 64 bytes in length. The only consideration with this move is that callers need to "coerce" a sig to the proper type if they need schnorr signatures.
In this commit, we add the new types that'll house musig signatures with and without their nonces. We send the nonce along with the sig everywhere but the co-op close flow.
This ensures that the caller doesn't need to worry about the TLV type ordering of the records the pass into the function.
8aefe80
to
c9b962e
Compare
ffe6383
into
lightningnetwork:simple-taproot-chans-staging
Change Description
This PR was split off of #6877.
This PR adds the set of new messages and TLV types for the current spec draft of simple taproot channels. None of the messages in this PR are used yet, as the goal is to split up the current mega PR into a series of digestible PRs.
The main change to the existing fields is the overloading of the
lnwire.Sig
type. This message is now updated to support encoding/decoding both ECDSA and schnorr signatures. We do this via the addition of a new type enum, which is consulted when converting between anlnwire.Sig
and aninput.Signature
.The other changes are adding the new partial sig, and nonce types to the relevant set of messages. Local/verification nonces are exchange up front by both sides. When a signature needs to be generated, a signing/remote nonce is generated prior, which is then included along with the nonce.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.