funding: fix channel type negotiation bug

The bug manifests when a nil ChannelType is passed to the funding
manager in InitFundingMsg. A default value for ChannelType is selected
and sent in the OpenChannel message. However, a nil ChannelType is
stored in the reservation context. This causes our ChannelType checks in
handleFundingAccept to be bypassed.

Usually this makes us end up in the "peer unexpectedly sent explicit
ChannelType" case, where we can still recover by reselecting a default
ChannelType and verifying it matches the one the peer sent. But if the
peer sends a nil ChannelType, we miss it.

While fixing the bug, I also tried to simplify the negotiation logic, as
the complexity is likely what hid the bug in the first place.

Now negotiateCommitmentType only returns the ChannelType to be used in
OpenChannel/AcceptChannel and the CommitmentType to pass to the wallet.
It will even return a nil ChannelType when we're supposed to use implicit
negotiation, so we don't need to manually set it to nil for OpenChannel
and AcceptChannel.

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.
@@ -24,38 +18,47 @@ var (
 // negotiateCommitmentType negotiates the commitment type of a newly opened
 // 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.
+// 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, mustBeExplicit bool) (bool,
-	*lnwire.ChannelType, lnwallet.CommitmentType, error) {
+	remote *lnwire.FeatureVector) (*lnwire.ChannelType,
+	lnwallet.CommitmentType, error) {
 
+	// BOLT#2 specifies we MUST use explicit negotiation if both peers
+	// signal for it.
+	if hasFeatures(local, remote, lnwire.ExplicitChannelTypeOptional) {
 		if desiredChanType != nil {
-		// If the peer does know explicit negotiation, let's attempt
-		// that now.
-		if hasFeatures(
-			local, remote, lnwire.ExplicitChannelTypeOptional,
-		) {
-
 			commitType, err := explicitNegotiateCommitmentType(
 				*desiredChanType, local, remote,
 			)
-			return true, desiredChanType, commitType, err
+
+			return desiredChanType, commitType, 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, nil, 0, errUnsupportedExplicitNegotiation
-		}
+		// Explicitly signal the "implicit" negotiation commitment type
+		// as default when a desired channel type isn't specified.
+		chanType, commitType := implicitNegotiateCommitmentType(local,
+			remote)
+
+		return chanType, commitType, nil
 	}
 
+	// Otherwise, we'll use implicit negotiation. In this case, we are
+	// restricted to the newest channel type advertised. If the passed-in
+	// channelType doesn't match what was advertised, we fail.
 	chanType, commitType := implicitNegotiateCommitmentType(local, remote)
-	return false, chanType, commitType, nil
+
+	if desiredChanType != nil {
+		expected := lnwire.RawFeatureVector(*desiredChanType)
+		actual := lnwire.RawFeatureVector(*chanType)
+		if !expected.Equals(&actual) {
+			return nil, 0, errUnsupportedChannelType
+		}
+	}
+
+	return nil, commitType, nil
 }
 
 // explicitNegotiateCommitmentType attempts to explicitly negotiate for a

funding/commitment_type_negotiation_test.go

@@ -15,7 +15,6 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 
 	testCases := []struct {
 		name              string
-		mustBeExplicit    bool
 		channelFeatures   *lnwire.RawFeatureVector
 		localFeatures     *lnwire.RawFeatureVector
 		remoteFeatures    *lnwire.RawFeatureVector
@@ -41,33 +40,9 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 				lnwire.AnchorsZeroFeeHtlcTxOptional,
 			),
 			expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx,
-			expectsChanType: (*lnwire.ChannelType)(
-				lnwire.NewRawFeatureVector(
-					lnwire.StaticRemoteKeyRequired,
-					lnwire.AnchorsZeroFeeHtlcTxRequired,
-				),
-			),
+			expectsChanType:   nil,
 			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(
@@ -282,9 +257,9 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 		},
 		// 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,
@@ -316,11 +291,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 				lnwire.StaticRemoteKeyOptional,
 			),
 			expectsCommitType: lnwallet.CommitmentTypeTweakless,
-			expectsChanType: (*lnwire.ChannelType)(
-				lnwire.NewRawFeatureVector(
-					lnwire.StaticRemoteKeyRequired,
-				),
-			),
+			expectsChanType:   nil,
 			expectsErr:        nil,
 		},
 		{
@@ -332,9 +303,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 				lnwire.AnchorsZeroFeeHtlcTxOptional,
 			),
 			expectsCommitType: lnwallet.CommitmentTypeLegacy,
-			expectsChanType: (*lnwire.ChannelType)(
-				lnwire.NewRawFeatureVector(),
-			),
+			expectsChanType:   nil,
 			expectsErr:        nil,
 		},
 	}
@@ -357,9 +326,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 				)
 			}
 
-			_, lChan, lCommit, err := negotiateCommitmentType(
+			lChan, lCommit, err := negotiateCommitmentType(
 				channelType, localFeatures, remoteFeatures,
-				testCase.mustBeExplicit,
 			)
 
 			var (
@@ -383,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 {

funding/manager.go

@@ -1452,9 +1452,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
@@ -1473,19 +1472,16 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer,
 	}
 
 	var (
-		chanTypeFeatureBits *lnwire.ChannelType
 		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)
 
@@ -1727,7 +1723,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,
 	}
@@ -1800,7 +1796,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,
 	}
 
@@ -1888,12 +1884,9 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer,
 			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.
-		_, _, negotiatedCommitType, err := negotiateCommitmentType(
+		_, negotiatedCommitType, err := negotiateCommitmentType(
 			msg.ChannelType, peer.LocalFeatures(),
-			peer.RemoteFeatures(), false,
+			peer.RemoteFeatures(),
 		)
 		if err != nil {
 			err := errors.New("received unexpected channel type")
@@ -4055,9 +4048,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)
@@ -4070,8 +4063,9 @@ func (f *Manager) handleInitFundingMsg(msg *InitFundingMsg) {
 		scid     bool
 	)
 
-	// Check if the returned chanType includes either the zero-conf or
-	// scid-alias bits.
+	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)
@@ -4079,12 +4073,14 @@ func (f *Manager) handleInitFundingMsg(msg *InitFundingMsg) {
 		// 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")
+			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
 	// commitment transaction confirmed by the next few blocks (conf target
@@ -4239,7 +4235,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

@@ -4493,3 +4493,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")
+}