Merge pull request #7177 from morehouse/fix_chantype_negotiation

funding: fix channel type negotiation bug

docs/release-notes/release-notes-0.17.0.md

@@ -148,6 +148,9 @@
 * [Cancel rebroadcasting of a transaction when abandoning
   a channel](https://github.com/lightningnetwork/lnd/pull/7819).
 
+* [Fixed a validation bug](https://github.com/lightningnetwork/lnd/pull/7177) in
+  `channel_type` negotiation.
+
 ## Code Health
 
 * Updated [our fork for serializing protobuf as JSON to be based on the

funding/commitment_type_negotiation.go

@@ -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 " +
-		"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.
@@ -22,40 +16,63 @@ 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,
-		) {
+// 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,
+	lnwallet.CommitmentType, error) {
 
-			chanType, err := explicitNegotiateCommitmentType(
-				*channelType, local, remote,
-			)
-			return true, channelType, chanType, err
-		}
+	// BOLT#2 specifies we MUST use explicit negotiation if both peers
+	// signal for it.
+	explicitNegotiation := hasFeatures(
+		local, remote, lnwire.ExplicitChannelTypeOptional,
+	)
+
+	chanTypeRequested := desiredChanType != nil
+
+	switch {
+	case explicitNegotiation && chanTypeRequested:
+		commitType, err := explicitNegotiateCommitmentType(
+			*desiredChanType, local, remote,
+		)
+
+		return desiredChanType, commitType, err
+
+	// We don't have a specific channel type requested, so we select a
+	// default type as if implicit negotiation were used, and then we
+	// explicitly signal that default type.
+	case explicitNegotiation && !chanTypeRequested:
+		defaultChanType, commitType := implicitNegotiateCommitmentType(
+			local, remote,
+		)
 
-		// 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 defaultChanType, commitType, nil
+
+	// A specific channel type was requested, but we can't explicitly signal
+	// it. So if implicit negotiation wouldn't select the desired channel
+	// type, we must return an error.
+	case !explicitNegotiation && chanTypeRequested:
+		implicitChanType, commitType := implicitNegotiateCommitmentType(
+			local, remote,
+		)
+
+		expected := lnwire.RawFeatureVector(*desiredChanType)
+		actual := lnwire.RawFeatureVector(*implicitChanType)
+		if !expected.Equals(&actual) {
+			return nil, 0, errUnsupportedChannelType
 		}
-	}
 
-	chanType, commitType := implicitNegotiateCommitmentType(local, remote)
-	return false, chanType, commitType, nil
+		return nil, commitType, nil
+
+	default: // !explicitNegotiation && !chanTypeRequested
+		_, commitType := implicitNegotiateCommitmentType(local, remote)
+
+		return nil, commitType, nil
+	}
 }
 
 // explicitNegotiateCommitmentType attempts to explicitly negotiate for a

funding/commitment_type_negotiation_test.go

@@ -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
@@ -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",
@@ -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,
@@ -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,
@@ -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,
@@ -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,
@@ -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,
 		},
 		{
@@ -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,
 		},
 		{
@@ -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,
@@ -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,
 		},
 		{
@@ -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",
@@ -320,7 +303,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 				lnwire.AnchorsZeroFeeHtlcTxOptional,
 			),
 			expectsCommitType: lnwallet.CommitmentTypeLegacy,
-			expectsChanType:   lnwire.ChannelType(*lnwire.NewRawFeatureVector()),
+			expectsChanType:   nil,
 			expectsErr:        nil,
 		},
 	}
@@ -343,9 +326,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 				)
 			}
 
-			_, lChan, lCommit, err := negotiateCommitmentType(
+			lChan, lCommit, err := negotiateCommitmentType(
 				channelType, localFeatures, remoteFeatures,
-				testCase.mustBeExplicit,
 			)
 
 			var (
@@ -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 {
@@ -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,
 			)
 		})