-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
funding: fix channel type negotiation bug #7177
funding: fix channel type negotiation bug #7177
Conversation
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.
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.
@@ -4131,7 +4124,7 @@ func (f *Manager) handleInitFundingMsg(msg *InitFundingMsg) { | |||
remoteMaxHtlcs: maxHtlcs, | |||
remoteChanReserve: chanReserve, | |||
maxLocalCsv: maxCSV, | |||
channelType: msg.ChannelType, | |||
channelType: chanType, |
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.
Looks like this is the only line needed for the fix? Since after implicitNegotiateCommitmentType
, chanType
won't be 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.
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.
// 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 " + |
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 about the removal of this error. In this case the sender will get less explicit error?
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 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.
I'll see what I can do to split up the change. But |
36bdfd7
to
451c7b8
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.
Sorry about the late review. Wanna bump this as it involves some nice refactors, and left some questions.
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.
451c7b8
to
c50d704
Compare
Rebased and updated. |
@yyforyongyu: review reminder |
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🙏
@@ -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) |
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.
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.
|
||
// 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) { |
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.
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.
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.
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.
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.
Nice fix, I think it would be nice to switch to switch but that would need another round from @yyforyongyu. Happy with either iteration.
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.
ACK 81d8d31d82fcee8051ef3e0de1f531e1116ca8b0
- use a switch instead of nested ifs to improve readability - improve some variable names - reword comments
81d8d31
to
e6a2167
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.
Concept Ack. Thanks for identifying this and the fix. |
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 🌋
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.
@@ -38,10 +38,10 @@ func negotiateCommitmentType(desiredChanType *lnwire.ChannelType, local, | |||
local, remote, lnwire.ExplicitChannelTypeOptional, | |||
) { | |||
|
|||
chanType, err := explicitNegotiateCommitmentType( | |||
commitType, err := explicitNegotiateCommitmentType( |
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.
👍
|
||
// Explicitly signal the "implicit" negotiation commitment type | ||
// as default when a desired channel type isn't specified. | ||
chanType, commitType := implicitNegotiateCommitmentType(local, |
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.
Weird line folding, should be:
chanType, commitType := implicitNegotiateCommitmentType(
local, remote,
)
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.
Fixed in the refactor commit.
The bug manifests when a nil
ChannelType
is passed to the funding manager inInitFundingMsg
. A default value forChannelType
is selected and sent in theOpenChannel
message. However, a nilChannelType
is stored in the reservation context. This causes ourChannelType
checks inhandleFundingAccept
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 defaultChannelType
and verifying it matches the one the peer sent. But if the peer sends a nilChannelType
, 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 theChannelType
to be used inOpenChannel
/AcceptChannel
and theCommitmentType
to pass to the wallet. It will even return a nilChannelType
when we're supposed to use implicit negotiation, so we don't need to manually set it to nil forOpenChannel
andAcceptChannel
.