lnwire: add wire types for dynamic commitments by ProofOfKeags · Pull Request #8026 · lightningnetwork/lnd

ProofOfKeags · 2023-09-23T02:37:07Z

Change Description

This PR introduces new datatypes to LND as described in the Dynamic Commitments spec.

Steps to Test

make unit

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.

Roasbeef · 2023-09-27T18:25:55Z

We should still read in the ExtraOpaqueData here. Otherwise we may end up with just a partially consumed buffer.

I guess we also still need to decide which portions of the message are TLV vs otherwise.

Crypt-iQ · 2023-10-09T16:31:55Z

Think this should come with fuzz tests for the new message types. Can just do round-trip equality w/ decode + encode

ProofOfKeags · 2023-10-11T16:51:03Z

Think this should come with fuzz tests for the new message types. Can just do round-trip equality w/ decode + encode

It does.

Crypt-iQ · 2023-10-12T11:40:33Z

The more modern fuzz tests are in lnwire/fuzz_test.go. The go fuzzing engine is a bit better than testing/quick mutation. You can probably use the existing harness function for each of the new messages here:

lnd/lnwire/fuzz_test.go

Lines 26 to 54 in ca9cdac

    
           func harness(t *testing.T, data []byte) { 
        
           	t.Helper() 
        
           	// Create a reader with the byte array. 
        
           	r := bytes.NewReader(data) 
        
           	// Check that the created message is not greater than the maximum 
        
           	// message size. 
        
           	if len(data) > MaxSliceLength { 
        
           		return 
        
           	} 
        
           	msg, err := ReadMessage(r, 0) 
        
           	if err != nil { 
        
           		return 
        
           	} 
        
           	// We will serialize the message into a new bytes buffer. 
        
           	var b bytes.Buffer 
        
           	_, err = WriteMessage(&b, msg, 0) 
        
           	require.NoError(t, err) 
        
           	// Deserialize the message from the serialized bytes buffer, and then 
        
           	// assert that the original message is equal to the newly deserialized 
        
           	// message. 
        
           	newMsg, err := ReadMessage(&b, 0) 
        
           	require.NoError(t, err) 
        
           	require.Equal(t, msg, newMsg) 
        
           }

Roasbeef

Message structure looks pretty good.

Open to trying out Option[T] in place of the pointer usage as an initial pilot on the cobdebase. Here's one I prototyped in the past: https://go.dev/play/p/l1R_ueg2NoJ. I think siimlar to tapd, we'd add an fn package, drop this in, then go from there.

Other thing we need to nail down is the initial version of the reject bitvector/message we want go along with.

Roasbeef · 2023-10-20T00:06:30Z

Isn't this information already apparent to both sides contextually?

Eg: for channel funding we know who initiator is from the very start, so we don't need messages like closing_signed to signal if it's sent by the initiator not (for that msg behavior differs based on if you're the initiator or not).

What do we do in the situation where both sides send a dyn_propose "at the same time"? It's unlikely in practice but possible. I suppose we could just fail at some point when the conflict becomes unignorable. This was put in to express intent to the receiver that we believe we are initiating the negotiation. Such an intent isn't clear otherwise.

I think this can be fixed if you abort the flow if you send a dyn_propose with initiator=true and receive a dyn_propose with initiator=true

Agreed. But absent the initiator=true label I don't think this can be accomplished.

Roasbeef · 2023-10-20T00:09:54Z

If the target isn't a non-primitive type, then you can decode directly into it. Should be able to save ~20 lines or so here w/ that.

Roasbeef · 2023-10-20T00:11:44Z

Similar here re if not primitive type, can pass the attribute directly in.

lightninglabs-deploy · 2023-10-27T00:54:25Z

@Crypt-iQ: review reminder
@ProofOfKeags, remember to re-request review from reviewers when ready

ProofOfKeags · 2023-10-28T20:51:21Z

Is this a sin?

Crypt-iQ · 2023-11-01T15:21:29Z

nit: should have a line break before tlvRecords similar to how ChannelType is below

Crypt-iQ · 2023-11-01T15:29:55Z

Note for later implementation:
kickoffFeerate is SatPerKWeight which is int64 under the hood. Casting to uint32 can lead to integer truncation if the feerate is very high or low. To avoid headaches and possible bugs, we should ensure that whatever kickoff feerate we send/receive is <= math.MaxUint32

Crypt-iQ · 2023-11-01T16:55:07Z

nit: newline for chainfee.SatPerKWeight to avoid double parenthesis

Crypt-iQ · 2023-11-01T17:14:14Z

nit: id unnecessary?

Crypt-iQ · 2023-11-01T17:14:27Z

nit: id unnecessary?

this commit introduces many of the most common functions you will want to use with the Option type. Not all of them are used immediately in this PR.

Roasbeef

LGTM ✏️

Roasbeef · 2023-11-07T02:18:22Z

+
+// Option[A] represents a value which may or may not be there. This is very
+// often preferable to nil-able pointers.
+type Option[A any] struct {


Roasbeef · 2023-11-08T23:28:19Z

Could be an optional?

Fixed. Should I also do the same thing to the LocalNonce field even though it's unrelated to dyncomms or leave it heterogeneous for now?

lnwire: add tests for dynamic commitment wire serialization

ProofOfKeags changed the base branch from master to 0-18-staging September 23, 2023 02:37

ProofOfKeags force-pushed the feature/dyncomms branch from 38589cd to 1938b2b Compare September 25, 2023 22:48

ProofOfKeags marked this pull request as ready for review September 25, 2023 23:02

ProofOfKeags marked this pull request as draft September 25, 2023 23:02

ProofOfKeags force-pushed the feature/dyncomms branch 2 times, most recently from 7ec0860 to a48aa9a Compare September 26, 2023 03:30

Roasbeef reviewed Sep 27, 2023

View reviewed changes

Crypt-iQ reviewed Oct 3, 2023

View reviewed changes

Comment thread lnwire/dyn_begin_propose.go Outdated

Comment thread lnwire/dyn_propose.go Outdated

Comment thread lnwire/dyn_propose.go Outdated

Comment thread lnwire/dyn_propose.go Outdated

Comment thread lnwire/dyn_propose.go Outdated

Comment thread lnwire/dyn_propose.go Outdated

saubyk added this to the v0.18.0 milestone Oct 3, 2023

saubyk assigned ProofOfKeags and unassigned ProofOfKeags Oct 5, 2023

ProofOfKeags force-pushed the feature/dyncomms branch from a48aa9a to 3b39bc8 Compare October 10, 2023 22:07

Crypt-iQ reviewed Oct 11, 2023

View reviewed changes

Comment thread lnwire/dyn_ack.go Outdated

Comment thread lnwire/dyn_ack.go Outdated

Comment thread lnwire/dyn_propose.go Outdated

Comment thread lnwire/dyn_reject.go Outdated

ProofOfKeags force-pushed the feature/dyncomms branch from 3b39bc8 to 6ada649 Compare October 11, 2023 17:44

saubyk assigned ProofOfKeags Oct 12, 2023

saubyk linked an issue Oct 12, 2023 that may be closed by this pull request

[epic]: Dynamic Commitments (Dynamic Channels) #7878

Closed

18 tasks

ProofOfKeags force-pushed the feature/dyncomms branch 3 times, most recently from 4dc6cba to 32601d4 Compare October 14, 2023 18:42

ProofOfKeags changed the title ~~lnwire: add wire types for dynamic commitment negotation messages~~ lnwire: add wire types for dynamic commitments Oct 19, 2023

ProofOfKeags force-pushed the feature/dyncomms branch 2 times, most recently from 889dd1f to 81e3e83 Compare October 19, 2023 03:51

ProofOfKeags marked this pull request as ready for review October 19, 2023 03:51

saubyk requested review from Crypt-iQ and Roasbeef October 19, 2023 15:24

Roasbeef requested changes Oct 20, 2023

View reviewed changes

ProofOfKeags force-pushed the feature/dyncomms branch from 81e3e83 to 7e00fe0 Compare October 28, 2023 00:28

ProofOfKeags commented Oct 28, 2023

View reviewed changes

Comment thread lnwire/lnwire.go Outdated

Comment on lines 594 to 600

Copy link
Copy Markdown

Collaborator Author

ProofOfKeags Oct 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a sin?

ProofOfKeags force-pushed the feature/dyncomms branch 3 times, most recently from 1ec5cce to ecf9483 Compare October 31, 2023 19:33

ProofOfKeags requested a review from Roasbeef October 31, 2023 19:35

Crypt-iQ reviewed Nov 1, 2023

View reviewed changes

fn: introduce option type

3d8c57d

this commit introduces many of the most common functions you will want to use with the Option type. Not all of them are used immediately in this PR.

ProofOfKeags force-pushed the feature/dyncomms branch 3 times, most recently from 00fc24c to 134fa78 Compare November 8, 2023 02:06

ProofOfKeags requested a review from Crypt-iQ November 8, 2023 02:09

ProofOfKeags force-pushed the feature/dyncomms branch from 134fa78 to 80c3368 Compare November 8, 2023 18:06

Roasbeef approved these changes Nov 8, 2023

View reviewed changes

ProofOfKeags force-pushed the feature/dyncomms branch from 80c3368 to b8ea14b Compare November 9, 2023 04:51

Crypt-iQ reviewed Nov 9, 2023

View reviewed changes

Comment thread lnwire/dyn_reject.go Outdated

Comment thread lnwire/dyn_reject.go Outdated

Comment thread lnwire/dyn_propose.go Outdated

Comment thread lnwire/lnwire_test.go Outdated

ProofOfKeags added 5 commits November 9, 2023 09:10

lnwire: introduce message types for dynamic commitment negotiation

26b995c

lnwire: add tests for dynamic commitment wire serialization

lnwire: add TLV extension to channel_reestablish for dynamic commitments

c28934a

lnwire: add fuzz tests for new dynamic commitment message types

fc9a965

docs: update release notes

f394401

lnwire: add musig2 taproot execution messages for dynamic commitments

134e9a7

ProofOfKeags force-pushed the feature/dyncomms branch from b8ea14b to 134e9a7 Compare November 9, 2023 18:19

Crypt-iQ approved these changes Nov 9, 2023

View reviewed changes

Roasbeef merged this pull request into lightningnetwork:0-18-staging Nov 10, 2023

ProofOfKeags mentioned this pull request Nov 11, 2023

Bugfix/htlc flush shutdown #8145

Closed

7 tasks

morehouse mentioned this pull request Nov 17, 2023

lnwire: initial dynamic commitment seeds lightninglabs/lnd-fuzz#20

Merged

ProofOfKeags mentioned this pull request Feb 5, 2024

[epic]: Dynamic Commitments (Dynamic Channels) #7878

Closed

18 tasks

Conversation

ProofOfKeags commented Sep 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Description

Steps to Test

Pull Request Checklist

Testing

Code Style and Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Crypt-iQ commented Oct 9, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ProofOfKeags commented Oct 11, 2023

Uh oh!

Crypt-iQ commented Oct 12, 2023

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lightninglabs-deploy commented Oct 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Roasbeef left a comment

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!

Uh oh!

ProofOfKeags commented Sep 23, 2023 •

edited

Loading