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

In this commit, we fix an API inconsistency introduced by a recent fix.
Without this commit, it would be possible for a local user to attempt an
explicit funding type, but then _fallback_ to implicit negotiation if
the remote party didn't have the bit set.

We resolve this by adding a new bit of information to ensure that if
we're the funder and want explicit chan negotiation, we don't settle
for anything less. A new test case has been added to exercise this
behavior.

To recap for posterity, the following issues were found and fixed in the
negotiation logic:

  1. If the remote party sent a channel type (in accept channel), but we
     didn't, then we would error out. We no longer error out and instead
     ensure the channel type they sent matches what we derived via
     implicit funding.

  2. If the remote party _did not_ have the bit set, but they sent a
     chan type (in open channel), we would error out. We no longer error
     out, but instead will fall back to implicit negotiation.

Ultimately we want to eventually flip the explicit funding bit to
_required_ to eliminate any other future ambiguities and also ensure
that it isn't possible to inadvertently fall back to implicit funding,
when the _user_ expects explicit funding.

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)