Merge pull request #6027 from Roasbeef/funding-api-consistency

funding: ensure a local funding w/ explicit type can't be downgraded

Author: Roasbeef

docs/release-notes/release-notes-0.14.1.md

@@ -22,6 +22,10 @@
   incompatibilities when opening channels with the latest versions of
   c-lightning and eclair](https://github.com/lightningnetwork/lnd/pull/6026).
 
+* [Ensure that if a user specifies explicit channel funding on the API level,
+  then it can't be
+  downgraded](https://github.com/lightningnetwork/lnd/pull/6027).
+
 # Contributors (Alphabetical Order)
 
 * Jamie Turley

funding/commitment_type_negotiation.go

@@ -8,6 +8,12 @@ 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.
@@ -20,19 +26,30 @@ var (
 // will be attempted if the set of both local and remote features support it.
 // Otherwise, implicit negotiation will be attempted.
 func negotiateCommitmentType(channelType *lnwire.ChannelType,
-	local, remote *lnwire.FeatureVector) (lnwallet.CommitmentType, error) {
+	local, remote *lnwire.FeatureVector,
+	mustBeExplicit bool) (bool, lnwallet.CommitmentType, error) {
 
 	if channelType != nil {
 		// If the peer does know explicit negotiation, let's attempt
 		// that now.
 		if hasFeatures(local, remote, lnwire.ExplicitChannelTypeOptional) {
-			return explicitNegotiateCommitmentType(
+			chanType, err := explicitNegotiateCommitmentType(
 				*channelType, local, remote,
 			)
+			return true, 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, 0, errUnsupportedExplicitNegotiation
 		}
 	}
 
-	return implicitNegotiateCommitmentType(local, remote), nil
+	return false, implicitNegotiateCommitmentType(local, remote), nil
 }
 
 // explicitNegotiateCommitmentType attempts to explicitly negotiate for a

funding/commitment_type_negotiation_test.go

@@ -15,6 +15,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 
 	testCases := []struct {
 		name            string
+		mustBeExplicit  bool
 		channelFeatures *lnwire.RawFeatureVector
 		localFeatures   *lnwire.RawFeatureVector
 		remoteFeatures  *lnwire.RawFeatureVector
@@ -39,6 +40,25 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 			expectsRes: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx,
 			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,
+		},
 		{
 			name: "explicit missing remote commitment feature",
 			channelFeatures: lnwire.NewRawFeatureVector(
@@ -168,13 +188,15 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 					*testCase.channelFeatures,
 				)
 			}
-			localType, err := negotiateCommitmentType(
+			_, localType, err := negotiateCommitmentType(
 				channelType, localFeatures, remoteFeatures,
+				testCase.mustBeExplicit,
 			)
 			require.Equal(t, testCase.expectsErr, err)
 
-			remoteType, err := negotiateCommitmentType(
+			_, remoteType, err := negotiateCommitmentType(
 				channelType, remoteFeatures, localFeatures,
+				testCase.mustBeExplicit,
 			)
 			require.Equal(t, testCase.expectsErr, err)
 

funding/manager.go

@@ -1274,8 +1274,9 @@ 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.
-	commitType, err := negotiateCommitmentType(
+	wasExplicit, commitType, err := negotiateCommitmentType(
 		msg.ChannelType, peer.LocalFeatures(), peer.RemoteFeatures(),
+		false,
 	)
 	if err != nil {
 		// TODO(roasbeef): should be using soft errors
@@ -1284,6 +1285,13 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer,
 		return
 	}
 
+	// Only echo back a channel type in AcceptChannel if we actually used
+	// explicit negotiation above.
+	var chanTypeFeatureBits *lnwire.ChannelType
+	if wasExplicit {
+		chanTypeFeatureBits = msg.ChannelType
+	}
+
 	chainHash := chainhash.Hash(msg.ChainHash)
 	req := &lnwallet.InitFundingReserveMsg{
 		ChainHash:        &chainHash,
@@ -1520,7 +1528,7 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer,
 		HtlcPoint:             ourContribution.HtlcBasePoint.PubKey,
 		FirstCommitmentPoint:  ourContribution.FirstCommitmentPoint,
 		UpfrontShutdownScript: ourContribution.UpfrontShutdown,
-		ChannelType:           msg.ChannelType,
+		ChannelType:           chanTypeFeatureBits,
 		LeaseExpiry:           msg.LeaseExpiry,
 	}
 
@@ -1594,9 +1602,13 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer,
 		implicitChannelType := implicitNegotiateCommitmentType(
 			peer.LocalFeatures(), peer.RemoteFeatures(),
 		)
-		negotiatedChannelType, err := negotiateCommitmentType(
+
+		// 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(
 			msg.ChannelType, peer.LocalFeatures(),
-			peer.RemoteFeatures(),
+			peer.RemoteFeatures(), false,
 		)
 		if err != nil {
 			err := errors.New("received unexpected channel type")
@@ -3246,9 +3258,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.
-	commitType, err := negotiateCommitmentType(
+	_, commitType, err := negotiateCommitmentType(
 		msg.ChannelType, msg.Peer.LocalFeatures(),
-		msg.Peer.RemoteFeatures(),
+		msg.Peer.RemoteFeatures(), true,
 	)
 	if err != nil {
 		log.Errorf("channel type negotiation failed: %v", err)