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) {
+// 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) {
 
-	if channelType != nil {
-		// If the peer does know explicit negotiation, let's attempt
-		// that now.
-		if hasFeatures(
-			local, remote, lnwire.ExplicitChannelTypeOptional,
-		) {
+	// BOLT#2 specifies we MUST use explicit negotiation if both peers
+	// signal for it.
+	explicitNegotiation := hasFeatures(
+		local, remote, lnwire.ExplicitChannelTypeOptional,
+	)
 
-			chanType, err := explicitNegotiateCommitmentType(
-				*channelType, local, remote,
-			)
-			return true, channelType, chanType, err
+	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,
+		)
+
+		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
 		}
 
-		// 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 nil, commitType, nil
+
+	default: // !explicitNegotiation && !chanTypeRequested
+		_, commitType := implicitNegotiateCommitmentType(local, remote)
+
+		return nil, commitType, nil
 	}
-
-	chanType, commitType := implicitNegotiateCommitmentType(local, remote)
-	return false, chanType, 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,
 			)
 		})

funding/manager.go

@@ -1504,9 +1504,8 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer,
 	// the remote peer are signaling the proper feature bit if we're using
 	// implicit negotiation, and simply the channel type sent over if we're
 	// using explicit negotiation.
-	wasExplicit, _, commitType, err := negotiateCommitmentType(
+	chanType, commitType, err := negotiateCommitmentType(
 		msg.ChannelType, peer.LocalFeatures(), peer.RemoteFeatures(),
-		false,
 	)
 	if err != nil {
 		// TODO(roasbeef): should be using soft errors
@@ -1525,19 +1524,16 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer,
 	}
 
 	var (
-		chanTypeFeatureBits *lnwire.ChannelType
-		zeroConf            bool
-		scid                bool
+		zeroConf bool
+		scid     bool
 	)
 
-	if wasExplicit {
-		// Only echo back a channel type in AcceptChannel if we actually
-		// used explicit negotiation above.
-		chanTypeFeatureBits = msg.ChannelType
-
+	// Only echo back a channel type in AcceptChannel if we actually used
+	// explicit negotiation above.
+	if chanType != nil {
 		// Check if the channel type includes the zero-conf or
 		// scid-alias bits.
-		featureVec := lnwire.RawFeatureVector(*chanTypeFeatureBits)
+		featureVec := lnwire.RawFeatureVector(*chanType)
 		zeroConf = featureVec.IsSet(lnwire.ZeroConfRequired)
 		scid = featureVec.IsSet(lnwire.ScidAliasRequired)
 
@@ -1773,7 +1769,7 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer,
 		remoteMaxHtlcs:    maxHtlcs,
 		remoteChanReserve: chanReserve,
 		maxLocalCsv:       f.cfg.MaxLocalCSVDelay,
-		channelType:       msg.ChannelType,
+		channelType:       chanType,
 		err:               make(chan error, 1),
 		peer:              peer,
 	}
@@ -1846,7 +1842,7 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer,
 		HtlcPoint:             ourContribution.HtlcBasePoint.PubKey,
 		FirstCommitmentPoint:  ourContribution.FirstCommitmentPoint,
 		UpfrontShutdownScript: ourContribution.UpfrontShutdown,
-		ChannelType:           chanTypeFeatureBits,
+		ChannelType:           chanType,
 		LeaseExpiry:           msg.LeaseExpiry,
 	}
 
@@ -1926,18 +1922,16 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer,
 	} else if msg.ChannelType != nil {
 		// The spec isn't too clear about whether it's okay to set the
 		// channel type in the accept_channel response if we didn't
-		// explicitly set it in the open_channel message. For now, let's
-		// just log the problem instead of failing the funding flow.
-		_, implicitChannelType := implicitNegotiateCommitmentType(
+		// explicitly set it in the open_channel message. For now, we
+		// check that it's the same type we'd have arrived through
+		// implicit negotiation. If it's another type, we fail the flow.
+		_, implicitCommitType := implicitNegotiateCommitmentType(
 			peer.LocalFeatures(), peer.RemoteFeatures(),
 		)
 
-		// We pass in false here as the funder since at this point, we
-		// didn't set a chan type ourselves, so falling back to
-		// implicit funding is acceptable.
-		_, _, negotiatedChannelType, err := negotiateCommitmentType(
+		_, negotiatedCommitType, err := negotiateCommitmentType(
 			msg.ChannelType, peer.LocalFeatures(),
-			peer.RemoteFeatures(), false,
+			peer.RemoteFeatures(),
 		)
 		if err != nil {
 			err := errors.New("received unexpected channel type")
@@ -1945,11 +1939,7 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer,
 			return
 		}
 
-		// Even though we don't expect a channel type to be set when we
-		// didn't send one in the first place, we check that it's the
-		// same type we'd have arrived through implicit negotiation. If
-		// it's another type, we fail the flow.
-		if implicitChannelType != negotiatedChannelType {
+		if implicitCommitType != negotiatedCommitType {
 			err := errors.New("negotiated unexpected channel type")
 			f.failFundingFlow(peer, cid, err)
 			return
@@ -4152,9 +4142,9 @@ func (f *Manager) handleInitFundingMsg(msg *InitFundingMsg) {
 	// Before we init the channel, we'll also check to see what commitment
 	// format we can use with this peer. This is dependent on *both* us and
 	// the remote peer are signaling the proper feature bit.
-	_, chanType, commitType, err := negotiateCommitmentType(
+	chanType, commitType, err := negotiateCommitmentType(
 		msg.ChannelType, msg.Peer.LocalFeatures(),
-		msg.Peer.RemoteFeatures(), true,
+		msg.Peer.RemoteFeatures(),
 	)
 	if err != nil {
 		log.Errorf("channel type negotiation failed: %v", err)
@@ -4167,20 +4157,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)
-	zeroConf = featureVec.IsSet(lnwire.ZeroConfRequired)
-	scid = featureVec.IsSet(lnwire.ScidAliasRequired)
+	if chanType != nil {
+		// Check if the returned chanType includes either the zero-conf
+		// or scid-alias bits.
+		featureVec := lnwire.RawFeatureVector(*chanType)
+		zeroConf = featureVec.IsSet(lnwire.ZeroConfRequired)
+		scid = featureVec.IsSet(lnwire.ScidAliasRequired)
 
-	// The option-scid-alias channel type for a public channel is
-	// disallowed.
-	if scid && !msg.Private {
-		err = fmt.Errorf("option-scid-alias chantype for public " +
-			"channel")
-		log.Error(err)
-		msg.Err <- err
-		return
+		// The option-scid-alias channel type for a public channel is
+		// disallowed.
+		if scid && !msg.Private {
+			err = fmt.Errorf("option-scid-alias chantype for " +
+				"public channel")
+			log.Error(err)
+			msg.Err <- err
+
+			return
+		}
 	}
 
 	// First, we'll query the fee estimator for a fee that should get the
@@ -4338,7 +4331,7 @@ func (f *Manager) handleInitFundingMsg(msg *InitFundingMsg) {
 		remoteMaxHtlcs:    maxHtlcs,
 		remoteChanReserve: chanReserve,
 		maxLocalCsv:       maxCSV,
-		channelType:       msg.ChannelType,
+		channelType:       chanType,
 		reservation:       reservation,
 		peer:              msg.Peer,
 		updates:           msg.Updates,

funding/manager_test.go

@@ -4507,3 +4507,62 @@ func TestCommitmentTypeFundmaxSanityCheck(t *testing.T) {
 		}
 	}
 }
+
+// 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) {
+	t.Parallel()
+
+	alice, bob := setupFundingManagers(t)
+	t.Cleanup(func() {
+		tearDownFundingManagers(t, alice, bob)
+	})
+
+	// Alice and Bob will have the same set of feature bits in our test.
+	featureBits := []lnwire.FeatureBit{
+		lnwire.ExplicitChannelTypeOptional,
+		lnwire.StaticRemoteKeyOptional,
+		lnwire.AnchorsZeroFeeHtlcTxOptional,
+	}
+	alice.localFeatures = featureBits
+	alice.remoteFeatures = featureBits
+	bob.localFeatures = featureBits
+	bob.remoteFeatures = featureBits
+
+	expectedChanType := (*lnwire.ChannelType)(lnwire.NewRawFeatureVector(
+		lnwire.StaticRemoteKeyRequired,
+		lnwire.AnchorsZeroFeeHtlcTxRequired,
+	))
+
+	// Create a funding request and start the workflow.
+	updateChan := make(chan *lnrpc.OpenStatusUpdate)
+	errChan := make(chan error, 1)
+	initReq := &InitFundingMsg{
+		Peer:            bob,
+		TargetPubkey:    bob.privKey.PubKey(),
+		ChainHash:       *fundingNetParams.GenesisHash,
+		LocalFundingAmt: 500000,
+		Updates:         updateChan,
+		Err:             errChan,
+	}
+
+	alice.fundingMgr.InitFundingWorkflow(initReq)
+
+	// Alice should have sent the OpenChannel message to Bob.
+	openChanMsg := expectOpenChannelMsg(t, alice.msgChan)
+
+	require.Equal(t, expectedChanType, openChanMsg.ChannelType)
+
+	// Let Bob handle the OpenChannel message.
+	bob.fundingMgr.ProcessFundingMsg(openChanMsg, alice)
+
+	acceptChanMsg, _ := assertFundingMsgSent(t, bob.msgChan,
+		"AcceptChannel").(*lnwire.AcceptChannel)
+
+	require.Equal(t, expectedChanType, acceptChanMsg.ChannelType)
+
+	// Drop the channel type and ensure Alice responds with an error.
+	acceptChanMsg.ChannelType = nil
+	alice.fundingMgr.ProcessFundingMsg(acceptChanMsg, bob)
+	assertFundingMsgSent(t, alice.msgChan, "Error")
+}