funding: fix channel type negotiation bug #7177

morehouse · 2022-11-18T22:02:02Z

The bug manifests when a nil ChannelType is passed to the funding manager in InitFundingMsg. A default value for ChannelType is selected and sent in the OpenChannel message. However, a nil ChannelType is stored in the reservation context. This causes our ChannelType checks in handleFundingAccept to be bypassed.

Usually this makes us end up in the "peer unexpectedly sent explicit ChannelType" case, where we can still recover by reselecting a default ChannelType and verifying it matches the one the peer sent. But if the peer sends a nil ChannelType, we miss it.

While fixing the bug, I also tried to simplify the negotiation logic, as the complexity is likely what hid the bug in the first place.

Now negotiateCommitmentType only returns the ChannelType to be used in OpenChannel/AcceptChannel and the CommitmentType to pass to the wallet. It will even return a nil ChannelType when we're supposed to use implicit negotiation, so we don't need to manually set it to nil for OpenChannel and AcceptChannel.

yyforyongyu

Thanks for the PR and nice catch! I think the first commit can be split into two commits - one for refactoring and one for the actual fix, that way it'd be easier for the reviewers to follow the changes. Besides fixing the bug, the refactoring isn't so "pure" as it changes the behavior and we need to take a deeper look.

yyforyongyu · 2023-01-16T01:11:44Z

funding/manager.go

@@ -4131,7 +4124,7 @@ func (f *Manager) handleInitFundingMsg(msg *InitFundingMsg) {
 		remoteMaxHtlcs:    maxHtlcs,
 		remoteChanReserve: chanReserve,
 		maxLocalCsv:       maxCSV,
-		channelType:       msg.ChannelType,
+		channelType:       chanType,


Looks like this is the only line needed for the fix? Since after implicitNegotiateCommitmentType, chanType won't be nil.

Unfortunately, it's not that simple.

Changing this value to chanType causes many other test cases to fail. It makes us expect the peer to use explicit negotiation in handleFundingAccept, even if the peer never signaled the necessary feature bit. And so we incorrectly fail the channel.

Looking at previous changes to the negotiation function, it seems to me that no one has really understood what the function is doing, and has instead applied bandaids to fix urgent bugs, while introducing new more subtle bugs.

I hope this PR helps fix the root problem once and for all.

yyforyongyu · 2023-01-16T01:13:12Z

funding/commitment_type_negotiation.go

-	// errUnsupportedExplicitNegotiation is an error returned when explicit
-	// channel commitment negotiation is attempted but either peer of the
-	// channel does not support it.
-	errUnsupportedExplicitNegotiation = errors.New("explicit channel " +


Not sure about the removal of this error. In this case the sender will get less explicit error?

The error only exists due to a previous bandaid-type fix: #6027.

After the fix in this PR, there seems to be little need for the error message. The only way to get non-default channel types is through explicit negotiation. If the peer doesn't support explicit negotiation it also won't support the desired channel type, so it seems just as accurate to return errUnsupportedChannelType.

The one case where it would potentially make sense to return errUnsupportedExplicitNegotiation is when the default channel type matches the desired channel type and the peer doesn't support explicit negotiation. But this PR makes handles that case by using implicit negotiation instead of failing like we did previously.

morehouse · 2023-01-16T19:30:28Z

Thanks for the PR and nice catch! I think the first commit can be split into two commits - one for refactoring and one for the actual fix, that way it'd be easier for the reviewers to follow the changes. Besides fixing the bug, the refactoring isn't so "pure" as it changes the behavior and we need to take a deeper look.

I'll see what I can do to split up the change. But negotiateCommitmentType really does need the complete rewrite in this PR. The current code is bug-prone and seems to have confused multiple people in the past.

yyforyongyu

Sorry about the late review. Wanna bump this as it involves some nice refactors, and left some questions.

funding/manager.go

docs/release-notes/release-notes-0.16.0.md

funding/commitment_type_negotiation.go

This prevents us from confusing the channelType parameter with the chanType local.

negotiateCommitmentType returns both a chanType and a commitType, so it is really confusing when the commitType is called chanType instead.

We don't actually do the logging described in the first comment, and the second comment block repeats much of the first comment block.

This allows us to expect chanType to be nil, which will be possible after the next commit.

The bug manifests when a nil ChannelType is passed to the funding manager in InitFundingMsg. A default value for ChannelType is selected and sent in the OpenChannel message. However, a nil ChannelType is stored in the reservation context. This causes our ChannelType checks in handleFundingAccept to be bypassed. Usually this makes us end up in the "peer unexpectedly sent explicit ChannelType" case, where we can still recover by reselecting a default ChannelType and verifying it matches the one the peer sent. But if the peer sends a nil ChannelType, we miss it. While fixing the bug, I also tried to simplify the negotiation logic, as the complexity is likely what hid the bug in the first place. Now negotiateCommitmentType only returns the ChannelType to be used in OpenChannel/AcceptChannel and the CommitmentType to pass to the wallet. It will even return a nil ChannelType when we're supposed to use implicit negotiation, so we don't need to manually set it to nil for OpenChannel and AcceptChannel.

morehouse · 2023-05-17T16:36:11Z

Rebased and updated.

lightninglabs-deploy · 2023-05-24T16:46:39Z

@yyforyongyu: review reminder

yyforyongyu

LGTM🙏

funding/commitment_type_negotiation.go

yyforyongyu · 2023-05-22T17:20:34Z

funding/manager.go

@@ -4070,20 +4063,23 @@ func (f *Manager) handleInitFundingMsg(msg *InitFundingMsg) {
 		scid     bool
 	)

-	// Check if the returned chanType includes either the zero-conf or
-	// scid-alias bits.
-	featureVec := lnwire.RawFeatureVector(*chanType)


Some notes for other reviewers: previously if the msg.ChannelType is nil, we'd implicitly negotiate a chanType. And when it's not nil, and ExplicitChannelTypeOptional is not supported, then we'd return an error so we won't get here.

funding/commitment_type_negotiation.go

funding/manager.go

carlaKC · 2023-08-04T19:45:00Z

funding/manager_test.go

+
+// TestFundingManagerNoEchoChanType tests that the funding flow is aborted if
+// the peer fails to echo back the channel type in AcceptChannel.
+func TestFundingManagerNoEchoChanType(t *testing.T) {


structure nittynit: nice to have the test in a commit before the bug fix triggering the condition and then updated in the fix commit to show it's addressed.

carlaKC

Nice fix to a gnarly piece of code. Only question here (so far) is whether we could go even simpler to save ourselves future headaches.

Not blocking, as I know 17 needs to get out of the door soon. Will take another pass on Monday.

funding/commitment_type_negotiation.go

carlaKC

Nice fix, I think it would be nice to switch to switch but that would need another round from @yyforyongyu. Happy with either iteration.

yyforyongyu

ACK 81d8d31

funding/commitment_type_negotiation.go

- use a switch instead of nested ifs to improve readability - improve some variable names - reword comments

carlaKC

Shouldn't e6a2167 be squashed with a8a50f3?

saubyk · 2023-08-14T01:57:28Z

Concept Ack. Thanks for identifying this and the fix.

Roasbeef

LGTM 🌋

The clean ups here are much appreciated! While working in the final set of changes for the taproot channels, I realized that once we merge in that feature, implicit negotiation starts to break down as otherwise if we had that in place still, one would never be able to open a public anchors channel after upgrading. I brought this up in the past spec meeting, and ppl generally agreed that we'd start to set the chan type feature bit to required, then only use that going forward.

Roasbeef · 2023-08-15T00:03:29Z

funding/commitment_type_negotiation.go

@@ -38,10 +38,10 @@ func negotiateCommitmentType(desiredChanType *lnwire.ChannelType, local,
 			local, remote, lnwire.ExplicitChannelTypeOptional,
 		) {

-			chanType, err := explicitNegotiateCommitmentType(
+			commitType, err := explicitNegotiateCommitmentType(


Roasbeef · 2023-08-15T00:08:41Z

funding/commitment_type_negotiation.go

+
+		// Explicitly signal the "implicit" negotiation commitment type
+		// as default when a desired channel type isn't specified.
+		chanType, commitType := implicitNegotiateCommitmentType(local,


Weird line folding, should be:

chanType, commitType := implicitNegotiateCommitmentType( local, remote, )

Fixed in the refactor commit.

morehouse mentioned this pull request Nov 29, 2022

lnwallet,funding: simplify initial funding flow #7210

Open

yyforyongyu requested changes Jan 16, 2023

View reviewed changes

morehouse force-pushed the fix_chantype_negotiation branch from 36bdfd7 to 451c7b8 Compare January 16, 2023 21:40

morehouse requested a review from yyforyongyu January 16, 2023 23:02

yyforyongyu reviewed May 17, 2023

View reviewed changes

funding/manager.go Show resolved Hide resolved

funding/manager.go Outdated Show resolved Hide resolved

docs/release-notes/release-notes-0.16.0.md Outdated Show resolved Hide resolved

funding/commitment_type_negotiation.go Outdated Show resolved Hide resolved

morehouse added 6 commits May 17, 2023 11:27

funding: rename channelType parameter

ec85b5b

This prevents us from confusing the channelType parameter with the chanType local.

funding: stop naming commit types chanType

e59cd7f

negotiateCommitmentType returns both a chanType and a commitType, so it is really confusing when the commitType is called chanType instead.

funding: remove incorrect and repetitive comments

34186de

We don't actually do the logging described in the first comment, and the second comment block repeats much of the first comment block.

funding: make expectsChanType a pointer

0ae9c63

This allows us to expect chanType to be nil, which will be possible after the next commit.

docs: release note for lightningnetwork#7177

c50d704

morehouse force-pushed the fix_chantype_negotiation branch from 451c7b8 to c50d704 Compare May 17, 2023 16:32

morehouse requested a review from yyforyongyu May 17, 2023 16:35

yyforyongyu approved these changes May 25, 2023

View reviewed changes

yyforyongyu added this to the v0.17.0 milestone May 25, 2023

Roasbeef mentioned this pull request May 31, 2023

[bug]: Unhandled commitment type in channel acceptor request with zero conf channels from CLN #7642

Closed

saubyk assigned morehouse Jun 14, 2023

Roasbeef added funding Related to the opening of new channels with funding transactions on the blockchain bug fix labels Aug 2, 2023

carlaKC self-requested a review August 4, 2023 17:27

carlaKC reviewed Aug 4, 2023

View reviewed changes

funding/commitment_type_negotiation.go Show resolved Hide resolved

carlaKC reviewed Aug 4, 2023

View reviewed changes

funding/manager.go Show resolved Hide resolved

carlaKC reviewed Aug 4, 2023

View reviewed changes

morehouse requested a review from carlaKC August 10, 2023 15:46

carlaKC reviewed Aug 10, 2023

View reviewed changes

funding/commitment_type_negotiation.go Show resolved Hide resolved

carlaKC approved these changes Aug 10, 2023

View reviewed changes

morehouse requested a review from yyforyongyu August 10, 2023 22:43

yyforyongyu approved these changes Aug 11, 2023

View reviewed changes

funding/commitment_type_negotiation.go Show resolved Hide resolved

funding/commitment_type_negotiation.go Show resolved Hide resolved

funding: refactor negotiateCommitmentType

e6a2167

- use a switch instead of nested ifs to improve readability - improve some variable names - reword comments

morehouse force-pushed the fix_chantype_negotiation branch from 81d8d31 to e6a2167 Compare August 11, 2023 16:20

carlaKC reviewed Aug 11, 2023

View reviewed changes

Roasbeef approved these changes Aug 15, 2023

View reviewed changes

Roasbeef merged commit c6a68d1 into lightningnetwork:master Aug 15, 2023
21 of 24 checks passed

morehouse deleted the fix_chantype_negotiation branch August 15, 2023 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

funding: fix channel type negotiation bug #7177

funding: fix channel type negotiation bug #7177

morehouse commented Nov 18, 2022

yyforyongyu left a comment

yyforyongyu Jan 16, 2023

morehouse Jan 16, 2023

yyforyongyu Jan 16, 2023

morehouse Jan 16, 2023

morehouse commented Jan 16, 2023

yyforyongyu left a comment

morehouse commented May 17, 2023

lightninglabs-deploy commented May 24, 2023

yyforyongyu left a comment

yyforyongyu May 22, 2023

carlaKC Aug 4, 2023

carlaKC left a comment

carlaKC left a comment

yyforyongyu left a comment

carlaKC left a comment

saubyk commented Aug 14, 2023

Roasbeef left a comment

Roasbeef Aug 15, 2023

Roasbeef Aug 15, 2023

Roasbeef Aug 15, 2023

funding: fix channel type negotiation bug #7177

funding: fix channel type negotiation bug #7177

Conversation

morehouse commented Nov 18, 2022

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morehouse commented Jan 16, 2023

yyforyongyu left a comment

Choose a reason for hiding this comment

morehouse commented May 17, 2023

lightninglabs-deploy commented May 24, 2023

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

saubyk commented Aug 14, 2023

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment