[3/?] - lnwire: add new TLV record types + messages for taproot chans by Roasbeef · Pull Request #7331 · lightningnetwork/lnd

Roasbeef · 2023-01-17T03:52:55Z

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 an lnwire.Sig and an input.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

Your PR passes all CI checks.
Tests covering the positive and negative (error paths) are included.
Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
Commits follow the Ideal Git Commit Structure.
Any new logging statements use an appropriate subsystem and logging level.
There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

ellemouton

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

Roasbeef · 2023-01-25T03:21:37Z

@ellemouton I merged the PR to my BOLT fork that this was baed on. Things should be synced now.

Roasbeef · 2023-01-25T03:42:58Z

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.

Roasbeef · 2023-01-26T03:06:33Z

Fixed some more API test/compilation issues in the latest diff.

ellemouton

LGTM 🚰 🥕

ellemouton · 2023-01-26T12:04:00Z

Ah - actually - the random_seed itest seems to consistently fail. LGTM after that is fixed :)

Roasbeef · 2023-01-27T02:38:22Z

@ellemouton fixed! Was using the wrong sig parsing for the RPCKeyRing.SignMessage method.

Roasbeef · 2023-01-27T02:38:40Z

Review ping @yyforyongyu.

joostjager · 2023-01-31T11:36:03Z

Should there be a reference to 'unadvertised' in the feature bit name?

Nope, good call out, removed.

joostjager · 2023-01-31T11:38:05Z

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.

If we want the entire commit to compile, then we need all the other API changes coupled in as well.

joostjager · 2023-01-31T11:43:42Z

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.

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.

joostjager · 2023-01-31T11:44:48Z

In other PRs, I found it better readable to handle the !ok case in the if block.

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

joostjager · 2023-01-31T11:47:08Z

Can the PartialSig type encoder be called here? Maybe over-engineering.

Not sure if we're looking at the same diff, but partialSig.Encode ends up calling this.

joostjager · 2023-01-31T11:52:54Z

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.

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.

joostjager · 2023-01-31T11:54:09Z

Why actually this change and not just always store the tlv data as is as was done previously?

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.

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.

joostjager · 2023-01-31T11:55:18Z

Can't you add &tlvRecords to the ReadElements arg list above?

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

yyforyongyu

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!

yyforyongyu · 2023-02-17T11:28:12Z

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.

yyforyongyu · 2023-02-17T11:47:10Z

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.

The LDK reviewers wanted to use distinct types here (compared to that sent in the chan open) messages citing a diff context/semantics.

yyforyongyu · 2023-02-17T11:59:42Z

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.

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.

litbot-9000 · 2023-03-10T12:52:17Z

@Roasbeef, remember to re-request review from reviewers when ready

Roasbeef · 2023-03-14T01:51:01Z

Note that I've changed the base branch to the new taproot staging branch as mentioned in: #6691 (comment)

Roasbeef · 2023-03-14T02:30:40Z

New version pushed, PTAL.

Roasbeef · 2023-03-15T00:37:25Z

Fixed a linter issue. Forces us to have a new line in the fuzz pacakge.

yyforyongyu

The last commit needs to be squashed, otherwise this is good to go⚡️

yyforyongyu · 2023-03-14T17:01:03Z

nit: seems it could be fit into one line

yyforyongyu · 2023-03-15T13:37:23Z

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.

yyforyongyu self-requested a review January 17, 2023 04:25

saubyk added this to the v0.16.0 milestone Jan 17, 2023

saubyk requested a review from ellemouton January 17, 2023 18:06

saubyk assigned Roasbeef Jan 17, 2023

saubyk added taproot tlv wire protocol Encoding of messages for the communication between nodes labels Jan 19, 2023

Roasbeef changed the title ~~lnwire: add new TLV record types + messages for taproot chans~~ [3/?] - lnwire: add new TLV record types + messages for taproot chans Jan 19, 2023

Roasbeef force-pushed the simple-taproot-wire-msgs branch from eec8e5f to 3bc1128 Compare January 19, 2023 04:03

Roasbeef mentioned this pull request Jan 19, 2023

[4/?] - input+lnwallet: prepare input package for funding logic, add new MusigSession abstraction #7340

Merged

7 tasks

ellemouton reviewed Jan 20, 2023

View reviewed changes

saubyk added the taproot chans label Jan 23, 2023

Roasbeef force-pushed the simple-taproot-wire-msgs branch from 3bc1128 to 4874fc4 Compare January 25, 2023 03:41

Roasbeef force-pushed the simple-taproot-wire-msgs branch from 4874fc4 to 3006325 Compare January 26, 2023 03:06

Roasbeef force-pushed the simple-taproot-wire-msgs branch 3 times, most recently from 296569c to ece355a Compare January 26, 2023 04:08

ellemouton approved these changes Jan 26, 2023

View reviewed changes

Comment thread lnwire/features.go Outdated

Comment thread lnwire/signature.go Outdated

Comment thread lnwire/funding_created.go Outdated

Comment thread lnwire/funding_locked.go Outdated

Comment thread lnwire/fuzz_test.go Outdated

Roasbeef force-pushed the simple-taproot-wire-msgs branch from ece355a to 2df0b47 Compare January 27, 2023 02:37

Roasbeef force-pushed the simple-taproot-wire-msgs branch from 2df0b47 to 6852b0a Compare January 27, 2023 03:23

joostjager reviewed Jan 31, 2023

View reviewed changes

saubyk modified the milestones: v0.16.0, v0.16.1 Feb 14, 2023

yyforyongyu requested changes Feb 17, 2023

View reviewed changes

saubyk modified the milestones: v0.16.1, v0.17.0 Mar 13, 2023

Roasbeef changed the base branch from master to simple-taproot-chans-staging March 14, 2023 01:50

Roasbeef force-pushed the simple-taproot-wire-msgs branch from 6852b0a to 3a6f8e9 Compare March 14, 2023 02:30

Roasbeef requested review from joostjager and yyforyongyu March 14, 2023 02:30

yyforyongyu reviewed Mar 15, 2023

View reviewed changes

Roasbeef added 15 commits March 15, 2023 14:45

lnwire: add feature bits for simple taproot chans

df453ac

lnwire: add new Musig2Nonce TLV record

ef87a3a

lnwire: add new musig2 partial signature type

f043f17

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.

lnwire: add LocalNonce to OpenChannel

8a5b7cf

lnwire: add LocalNonce to AcceptChannel

86dd6a2

lnwire: add PartialSig to FundingCreated

5c494ed

lnwire: add PartialSig to FundingSigned

5d2f4d6

lnwire: add LocalNonce to FundingLocked

e086d42

lnwire: add LocalNonce to ChannelReestablish

5b57964

lnwire: add PartialSig to CommitSig

70bbd6f

lnwire: add LocalNonce to RevokeAndAck

21d0bba

lnwire: add ShutdownNonce to Shutdown

4e8952d

lnwire: sort records in ExtractRecords

5210891

This ensures that the caller doesn't need to worry about the TLV type ordering of the records the pass into the function.

lnwire: TestLightningWireProtocol quick check tests for taproot fields

c9b962e

Roasbeef force-pushed the simple-taproot-wire-msgs branch from 8aefe80 to c9b962e Compare March 15, 2023 21:46

Roasbeef merged commit ffe6383 into lightningnetwork:simple-taproot-chans-staging Mar 15, 2023

Conversation

Roasbeef commented Jan 17, 2023

Change Description

Pull Request Checklist

Testing

Code Style and Documentation

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Roasbeef commented Jan 25, 2023

Uh oh!

Roasbeef commented Jan 25, 2023

Uh oh!

Roasbeef commented Jan 26, 2023

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ellemouton commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Roasbeef commented Jan 27, 2023

Uh oh!

Roasbeef commented Jan 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ellemouton commented Jan 26, 2023 •

edited

Loading