Skip to content
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.

Sign up for GitHub

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

Jump to bottom

funding: fix channel type negotiation bug #7177

Merged
merged 7 commits into from
Aug 15, 2023
Merged

funding: fix channel type negotiation bug #7177

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ec85b5b
funding: rename channelType parameter
morehouse Jan 16, 2023
e59cd7f
funding: stop naming commit types `chanType`
morehouse Jan 16, 2023
34186de
funding: remove incorrect and repetitive comments
morehouse Jan 16, 2023
0ae9c63
funding: make expectsChanType a pointer
morehouse Jan 16, 2023
a8a50f3
funding: fix channel type negotiation bug
morehouse Jan 16, 2023
c50d704
docs: release note for #7177
morehouse May 17, 2023
e6a2167
funding: refactor negotiateCommitmentType
morehouse Aug 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
69 changes: 36 additions & 33 deletions funding/commitment_type_negotiation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,6 @@ import (
)

var (
// 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 " +
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

"type negotiation not supported")

// errUnsupportedCommitmentType is an error returned when a specific
// channel commitment type is being explicitly negotiated but either
// peer of the channel does not support it.
Expand All @@ -22,40 +16,49 @@ var (
)

// negotiateCommitmentType negotiates the commitment type of a newly opened
// channel. If a channelType is provided, explicit negotiation for said type
// channel. If a desiredChanType is provided, explicit negotiation for said type
// will be attempted if the set of both local and remote features support it.
// Otherwise, implicit negotiation will be attempted. Two booleans are
// returned letting the caller know if the option-scid-alias or zero-conf
// channel types were negotiated.
func negotiateCommitmentType(channelType *lnwire.ChannelType, local,
remote *lnwire.FeatureVector, mustBeExplicit bool) (bool,
*lnwire.ChannelType, lnwallet.CommitmentType, error) {

if channelType != nil {
// If the peer does know explicit negotiation, let's attempt
// that now.
if hasFeatures(
local, remote, lnwire.ExplicitChannelTypeOptional,
) {

chanType, err := explicitNegotiateCommitmentType(
*channelType, local, remote,
// Otherwise, implicit negotiation will be attempted.
//
// The returned ChannelType is nil when implicit negotiation is used. An error
// is only returned if desiredChanType is not supported.
func negotiateCommitmentType(desiredChanType *lnwire.ChannelType, local,
remote *lnwire.FeatureVector) (*lnwire.ChannelType,
morehouse marked this conversation as resolved.
Show resolved Hide resolved
lnwallet.CommitmentType, error) {

// BOLT#2 specifies we MUST use explicit negotiation if both peers
// signal for it.
if hasFeatures(local, remote, lnwire.ExplicitChannelTypeOptional) {
if desiredChanType != nil {
commitType, err := explicitNegotiateCommitmentType(
Copy link
Member

Choose a reason for hiding this comment

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

👍

*desiredChanType, local, remote,
)
return true, channelType, chanType, err
}

// If we're the funder, and we are attempting to use an
// explicit channel type, but the remote party doesn't signal
// the bit, then we actually want to exit here, to ensure the
// user doesn't end up with an unexpected channel type via
// implicit negotiation.
if mustBeExplicit {
return false, nil, 0, errUnsupportedExplicitNegotiation
return desiredChanType, commitType, err
}

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

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, 
)

Copy link
Member

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.

remote)

return chanType, commitType, nil
}

// Otherwise, we'll use implicit negotiation. In this case, we are
carlaKC marked this conversation as resolved.
Show resolved Hide resolved
// restricted to the newest channel type advertised. If the passed-in
// channelType doesn't match what was advertised, we fail.
chanType, commitType := implicitNegotiateCommitmentType(local, remote)
return false, chanType, commitType, nil

if desiredChanType != nil {
expected := lnwire.RawFeatureVector(*desiredChanType)
actual := lnwire.RawFeatureVector(*chanType)
if !expected.Equals(&actual) {
return nil, 0, errUnsupportedChannelType
}
}

return nil, commitType, nil
}

// explicitNegotiateCommitmentType attempts to explicitly negotiate for a
Expand Down
101 changes: 41 additions & 60 deletions funding/commitment_type_negotiation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,11 @@ func TestCommitmentTypeNegotiation(t *testing.T) {

testCases := []struct {
name string
mustBeExplicit bool
channelFeatures *lnwire.RawFeatureVector
localFeatures *lnwire.RawFeatureVector
remoteFeatures *lnwire.RawFeatureVector
expectsCommitType lnwallet.CommitmentType
expectsChanType lnwire.ChannelType
expectsChanType *lnwire.ChannelType
zeroConf bool
scidAlias bool
expectsErr error
Expand All @@ -41,30 +40,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
lnwire.AnchorsZeroFeeHtlcTxOptional,
),
expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx,
expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxRequired,
)),
expectsErr: nil,
},
{
name: "local funder wants explicit, remote doesn't " +
"support so fall back",
mustBeExplicit: true,
channelFeatures: lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxRequired,
),
localFeatures: lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyOptional,
lnwire.AnchorsZeroFeeHtlcTxOptional,
lnwire.ExplicitChannelTypeOptional,
),
remoteFeatures: lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyOptional,
lnwire.AnchorsZeroFeeHtlcTxOptional,
),
expectsErr: errUnsupportedExplicitNegotiation,
expectsChanType: nil,
expectsErr: nil,
},
{
name: "explicit missing remote commitment feature",
Expand Down Expand Up @@ -106,8 +83,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
lnwire.ExplicitChannelTypeOptional,
),
expectsCommitType: lnwallet.CommitmentTypeScriptEnforcedLease,
expectsChanType: lnwire.ChannelType(
*lnwire.NewRawFeatureVector(
expectsChanType: (*lnwire.ChannelType)(
lnwire.NewRawFeatureVector(
lnwire.ZeroConfRequired,
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxRequired,
Expand Down Expand Up @@ -137,8 +114,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
lnwire.ExplicitChannelTypeOptional,
),
expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx,
expectsChanType: lnwire.ChannelType(
*lnwire.NewRawFeatureVector(
expectsChanType: (*lnwire.ChannelType)(
lnwire.NewRawFeatureVector(
lnwire.ZeroConfRequired,
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxRequired,
Expand Down Expand Up @@ -170,8 +147,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
lnwire.ExplicitChannelTypeOptional,
),
expectsCommitType: lnwallet.CommitmentTypeScriptEnforcedLease,
expectsChanType: lnwire.ChannelType(
*lnwire.NewRawFeatureVector(
expectsChanType: (*lnwire.ChannelType)(
lnwire.NewRawFeatureVector(
lnwire.ScidAliasRequired,
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxRequired,
Expand Down Expand Up @@ -201,8 +178,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
lnwire.ExplicitChannelTypeOptional,
),
expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx,
expectsChanType: lnwire.ChannelType(
*lnwire.NewRawFeatureVector(
expectsChanType: (*lnwire.ChannelType)(
lnwire.NewRawFeatureVector(
lnwire.ScidAliasRequired,
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxRequired,
Expand All @@ -228,10 +205,12 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
lnwire.ExplicitChannelTypeOptional,
),
expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx,
expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxRequired,
)),
expectsChanType: (*lnwire.ChannelType)(
lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxRequired,
),
),
expectsErr: nil,
},
{
Expand All @@ -250,9 +229,11 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
lnwire.ExplicitChannelTypeOptional,
),
expectsCommitType: lnwallet.CommitmentTypeTweakless,
expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyRequired,
)),
expectsChanType: (*lnwire.ChannelType)(
lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyRequired,
),
),
expectsErr: nil,
},
{
Expand All @@ -269,14 +250,16 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
lnwire.ExplicitChannelTypeOptional,
),
expectsCommitType: lnwallet.CommitmentTypeLegacy,
expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector()),
expectsErr: nil,
expectsChanType: (*lnwire.ChannelType)(
lnwire.NewRawFeatureVector(),
),
expectsErr: nil,
},
// Both sides signal the explicit chan type bit, so we expect
// that we return the corresponding chan type feature bits,
// even though we didn't set an explicit channel type.
// even though we didn't set a desired channel type.
{
name: "implicit anchors",
name: "default explicit anchors",
channelFeatures: nil,
localFeatures: lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyOptional,
Expand All @@ -289,10 +272,12 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
lnwire.ExplicitChannelTypeOptional,
),
expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx,
expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxRequired,
)),
expectsChanType: (*lnwire.ChannelType)(
lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxRequired,
),
),
expectsErr: nil,
},
{
Expand All @@ -306,10 +291,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
lnwire.StaticRemoteKeyOptional,
),
expectsCommitType: lnwallet.CommitmentTypeTweakless,
expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyRequired,
)),
expectsErr: nil,
expectsChanType: nil,
expectsErr: nil,
},
{
name: "implicit legacy",
Expand All @@ -320,7 +303,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
lnwire.AnchorsZeroFeeHtlcTxOptional,
),
expectsCommitType: lnwallet.CommitmentTypeLegacy,
expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector()),
expectsChanType: nil,
expectsErr: nil,
},
}
Expand All @@ -343,9 +326,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
)
}

_, lChan, lCommit, err := negotiateCommitmentType(
lChan, lCommit, err := negotiateCommitmentType(
channelType, localFeatures, remoteFeatures,
testCase.mustBeExplicit,
)

var (
Expand All @@ -369,9 +351,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
require.Equal(t, testCase.scidAlias, localScid)
require.Equal(t, testCase.expectsErr, err)

_, rChan, rCommit, err := negotiateCommitmentType(
rChan, rCommit, err := negotiateCommitmentType(
channelType, remoteFeatures, localFeatures,
testCase.mustBeExplicit,
)

if rChan != nil {
Expand Down Expand Up @@ -402,11 +383,11 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
)

require.Equal(
t, testCase.expectsChanType, *lChan,
t, testCase.expectsChanType, lChan,
testCase.name,
)
require.Equal(
t, testCase.expectsChanType, *rChan,
t, testCase.expectsChanType, rChan,
testCase.name,
)
})
Expand Down