Merge pull request #6075 from Roasbeef/chan-type-satisfice

funding: always send a channel type in explicit mode

docs/release-notes/release-notes-0.14.2.md

@@ -5,6 +5,9 @@
 * [Return the nearest known fee rate when a given conf target cannot be found
   from Web API fee estimator.](https://github.com/lightningnetwork/lnd/pull/6062)
 
+* [We now _always_ set a channel type if the other party signals the feature
+  bit](https://github.com/lightningnetwork/lnd/pull/6075).
+
 ## Wallet
 
 * A bug that prevented opening anchor-based channels from external wallets when

funding/commitment_type_negotiation.go

@@ -26,8 +26,8 @@ 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,
-	mustBeExplicit bool) (bool, lnwallet.CommitmentType, error) {
+	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
@@ -36,7 +36,7 @@ func negotiateCommitmentType(channelType *lnwire.ChannelType,
 			chanType, err := explicitNegotiateCommitmentType(
 				*channelType, local, remote,
 			)
-			return true, chanType, err
+			return true, channelType, chanType, err
 		}
 
 		// If we're the funder, and we are attempting to use an
@@ -45,11 +45,12 @@ func negotiateCommitmentType(channelType *lnwire.ChannelType,
 		// user doesn't end up with an unexpected channel type via
 		// implicit negotiation.
 		if mustBeExplicit {
-			return false, 0, errUnsupportedExplicitNegotiation
+			return false, nil, 0, errUnsupportedExplicitNegotiation
 		}
 	}
 
-	return false, implicitNegotiateCommitmentType(local, remote), nil
+	chanType, commitType := implicitNegotiateCommitmentType(local, remote)
+	return false, chanType, commitType, nil
 }
 
 // explicitNegotiateCommitmentType attempts to explicitly negotiate for a
@@ -113,12 +114,17 @@ func explicitNegotiateCommitmentType(channelType lnwire.ChannelType,
 // implicitly by choosing the latest type supported by the local and remote
 // fetures.
 func implicitNegotiateCommitmentType(local,
-	remote *lnwire.FeatureVector) lnwallet.CommitmentType {
+	remote *lnwire.FeatureVector) (*lnwire.ChannelType, lnwallet.CommitmentType) {
 
 	// If both peers are signalling support for anchor commitments with
 	// zero-fee HTLC transactions, we'll use this type.
 	if hasFeatures(local, remote, lnwire.AnchorsZeroFeeHtlcTxOptional) {
-		return lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx
+		chanType := lnwire.ChannelType(*lnwire.NewRawFeatureVector(
+			lnwire.AnchorsZeroFeeHtlcTxRequired,
+			lnwire.StaticRemoteKeyRequired,
+		))
+
+		return &chanType, lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx
 	}
 
 	// Since we don't want to support the "legacy" anchor type, we will fall
@@ -128,11 +134,16 @@ func implicitNegotiateCommitmentType(local,
 	// If both nodes are signaling the proper feature bit for tweakless
 	// commitments, we'll use that.
 	if hasFeatures(local, remote, lnwire.StaticRemoteKeyOptional) {
-		return lnwallet.CommitmentTypeTweakless
+		chanType := lnwire.ChannelType(*lnwire.NewRawFeatureVector(
+			lnwire.StaticRemoteKeyRequired,
+		))
+
+		return &chanType, lnwallet.CommitmentTypeTweakless
 	}
 
 	// Otherwise we'll fall back to the legacy type.
-	return lnwallet.CommitmentTypeLegacy
+	chanType := lnwire.ChannelType(*lnwire.NewRawFeatureVector())
+	return &chanType, lnwallet.CommitmentTypeLegacy
 }
 
 // hasFeatures determines whether a set of features is supported by both the set

funding/commitment_type_negotiation_test.go

@@ -14,13 +14,14 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 	t.Parallel()
 
 	testCases := []struct {
-		name            string
-		mustBeExplicit  bool
-		channelFeatures *lnwire.RawFeatureVector
-		localFeatures   *lnwire.RawFeatureVector
-		remoteFeatures  *lnwire.RawFeatureVector
-		expectsRes      lnwallet.CommitmentType
-		expectsErr      error
+		name              string
+		mustBeExplicit    bool
+		channelFeatures   *lnwire.RawFeatureVector
+		localFeatures     *lnwire.RawFeatureVector
+		remoteFeatures    *lnwire.RawFeatureVector
+		expectsCommitType lnwallet.CommitmentType
+		expectsChanType   lnwire.ChannelType
+		expectsErr        error
 	}{
 		{
 			name: "explicit missing remote negotiation feature",
@@ -37,7 +38,11 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 				lnwire.StaticRemoteKeyOptional,
 				lnwire.AnchorsZeroFeeHtlcTxOptional,
 			),
-			expectsRes: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx,
+			expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx,
+			expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector(
+				lnwire.StaticRemoteKeyRequired,
+				lnwire.AnchorsZeroFeeHtlcTxRequired,
+			)),
 			expectsErr: nil,
 		},
 		{
@@ -92,7 +97,11 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 				lnwire.AnchorsZeroFeeHtlcTxOptional,
 				lnwire.ExplicitChannelTypeOptional,
 			),
-			expectsRes: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx,
+			expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx,
+			expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector(
+				lnwire.StaticRemoteKeyRequired,
+				lnwire.AnchorsZeroFeeHtlcTxRequired,
+			)),
 			expectsErr: nil,
 		},
 		{
@@ -110,7 +119,10 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 				lnwire.AnchorsZeroFeeHtlcTxOptional,
 				lnwire.ExplicitChannelTypeOptional,
 			),
-			expectsRes: lnwallet.CommitmentTypeTweakless,
+			expectsCommitType: lnwallet.CommitmentTypeTweakless,
+			expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector(
+				lnwire.StaticRemoteKeyRequired,
+			)),
 			expectsErr: nil,
 		},
 		{
@@ -126,9 +138,13 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 				lnwire.AnchorsZeroFeeHtlcTxOptional,
 				lnwire.ExplicitChannelTypeOptional,
 			),
-			expectsRes: lnwallet.CommitmentTypeLegacy,
-			expectsErr: nil,
+			expectsCommitType: lnwallet.CommitmentTypeLegacy,
+			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.
 		{
 			name:            "implicit anchors",
 			channelFeatures: nil,
@@ -142,7 +158,11 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 				lnwire.AnchorsZeroFeeHtlcTxOptional,
 				lnwire.ExplicitChannelTypeOptional,
 			),
-			expectsRes: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx,
+			expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx,
+			expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector(
+				lnwire.StaticRemoteKeyRequired,
+				lnwire.AnchorsZeroFeeHtlcTxRequired,
+			)),
 			expectsErr: nil,
 		},
 		{
@@ -155,7 +175,10 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 			remoteFeatures: lnwire.NewRawFeatureVector(
 				lnwire.StaticRemoteKeyOptional,
 			),
-			expectsRes: lnwallet.CommitmentTypeTweakless,
+			expectsCommitType: lnwallet.CommitmentTypeTweakless,
+			expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector(
+				lnwire.StaticRemoteKeyRequired,
+			)),
 			expectsErr: nil,
 		},
 		{
@@ -166,8 +189,9 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 				lnwire.StaticRemoteKeyOptional,
 				lnwire.AnchorsZeroFeeHtlcTxOptional,
 			),
-			expectsRes: lnwallet.CommitmentTypeLegacy,
-			expectsErr: nil,
+			expectsCommitType: lnwallet.CommitmentTypeLegacy,
+			expectsChanType:   lnwire.ChannelType(*lnwire.NewRawFeatureVector()),
+			expectsErr:        nil,
 		},
 	}
 
@@ -188,13 +212,13 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 					*testCase.channelFeatures,
 				)
 			}
-			_, localType, err := negotiateCommitmentType(
+			_, localChanType, localCommitType, err := negotiateCommitmentType(
 				channelType, localFeatures, remoteFeatures,
 				testCase.mustBeExplicit,
 			)
 			require.Equal(t, testCase.expectsErr, err)
 
-			_, remoteType, err := negotiateCommitmentType(
+			_, remoteChanType, remoteCommitType, err := negotiateCommitmentType(
 				channelType, remoteFeatures, localFeatures,
 				testCase.mustBeExplicit,
 			)
@@ -205,11 +229,20 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 			}
 
 			require.Equal(
-				t, testCase.expectsRes, localType,
+				t, testCase.expectsCommitType, localCommitType,
 				testCase.name,
 			)
 			require.Equal(
-				t, testCase.expectsRes, remoteType,
+				t, testCase.expectsCommitType, remoteCommitType,
+				testCase.name,
+			)
+
+			require.Equal(
+				t, testCase.expectsChanType, *localChanType,
+				testCase.name,
+			)
+			require.Equal(
+				t, testCase.expectsChanType, *remoteChanType,
 				testCase.name,
 			)
 		})

funding/manager.go

@@ -1274,7 +1274,7 @@ 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(
+	wasExplicit, _, commitType, err := negotiateCommitmentType(
 		msg.ChannelType, peer.LocalFeatures(), peer.RemoteFeatures(),
 		false,
 	)
@@ -1599,14 +1599,14 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer,
 		// 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(
+		_, implicitChannelType := 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(
+		_, _, negotiatedChannelType, err := negotiateCommitmentType(
 			msg.ChannelType, peer.LocalFeatures(),
 			peer.RemoteFeatures(), false,
 		)
@@ -3258,7 +3258,7 @@ 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(
+	_, chanType, commitType, err := negotiateCommitmentType(
 		msg.ChannelType, msg.Peer.LocalFeatures(),
 		msg.Peer.RemoteFeatures(), true,
 	)
@@ -3415,7 +3415,7 @@ func (f *Manager) handleInitFundingMsg(msg *InitFundingMsg) {
 		FirstCommitmentPoint:  ourContribution.FirstCommitmentPoint,
 		ChannelFlags:          channelFlags,
 		UpfrontShutdownScript: shutdown,
-		ChannelType:           msg.ChannelType,
+		ChannelType:           chanType,
 		LeaseExpiry:           leaseExpiry,
 	}
 	if err := msg.Peer.SendMessage(true, &fundingOpen); err != nil {