multi: stop opening legacy channels

docs/release-notes/release-notes-0.20.5.md

@@ -56,10 +56,37 @@
 
 ## Breaking Changes
 
+* lnd [no longer opens or
+  accepts](https://github.com/lightningnetwork/lnd/pull/11212) new channels
+  using the legacy commitment type, which was
+  [removed](https://github.com/lightning/bolts/commit/91f4bd2383cc2fc7a0a43b697e209f9eb9f5183c)
+  from the spec in 2024. Its tweaked `to_remote` output is why funds in such a
+  channel cannot be recovered unilaterally after data loss: recovery needs the
+  peer to supply the relevant commitment point.
+
+  Note that an empty `channel_type` in `open_channel` asks for exactly this
+  type, and used to be accepted without any feature check at all, so a peer
+  could obtain a legacy channel from us no matter what either side signalled.
+  New channels now fall back to the static remote key commitment type instead.
+
+  Channels that already use the legacy type are **not** affected. They keep
+  working and can be operated, force closed and cooperatively closed as before.
+  Only opening new ones is refused.
+
+  `OpenChannel` now rejects `commitment_type` `LEGACY`. The enum value itself
+  remains, since it is also how existing channels are reported by
+  `ListChannels`, `ClosedChannels`, `PendingChannels` and the channel acceptor.
+
 ## Performance Improvements
 
 ## Deprecations
 
+* The dev build only `protocol.legacy.committweak` option is
+  [deprecated](https://github.com/lightningnetwork/lnd/pull/11212) and no longer
+  has any effect. It stopped the node from signalling
+  `option_static_remotekey`, which now leaves no commitment type left to
+  negotiate at all.
+
 # Technical and Architectural Updates
 
 ## BOLT Spec Updates
@@ -87,3 +114,4 @@
 * elsirion
 * Gijs van Dam
 * Yong Yu
+* Ziggie

docs/release-notes/release-notes-0.21.4.md

@@ -68,10 +68,37 @@
 
 ## Breaking Changes
 
+* lnd [no longer opens or
+  accepts](https://github.com/lightningnetwork/lnd/pull/11212) new channels
+  using the legacy commitment type, which was
+  [removed](https://github.com/lightning/bolts/commit/91f4bd2383cc2fc7a0a43b697e209f9eb9f5183c)
+  from the spec in 2024. Its tweaked `to_remote` output is why funds in such a
+  channel cannot be recovered unilaterally after data loss: recovery needs the
+  peer to supply the relevant commitment point.
+
+  Note that an empty `channel_type` in `open_channel` asks for exactly this
+  type, and used to be accepted without any feature check at all, so a peer
+  could obtain a legacy channel from us no matter what either side signalled.
+  New channels now fall back to the static remote key commitment type instead.
+
+  Channels that already use the legacy type are **not** affected. They keep
+  working and can be operated, force closed and cooperatively closed as before.
+  Only opening new ones is refused.
+
+  `OpenChannel` now rejects `commitment_type` `LEGACY`. The enum value itself
+  remains, since it is also how existing channels are reported by
+  `ListChannels`, `ClosedChannels`, `PendingChannels` and the channel acceptor.
+
 ## Performance Improvements
 
 ## Deprecations
 
+* The dev build only `protocol.legacy.committweak` option is
+  [deprecated](https://github.com/lightningnetwork/lnd/pull/11212) and no longer
+  has any effect. It stopped the node from signalling
+  `option_static_remotekey`, which now leaves no commitment type left to
+  negotiate at all.
+
 ### ⚠️ **Warning:** Deprecated fields in `lnrpc.Hop` will be removed in release version **0.22**
 
 ### ⚠️ **Warning:** The deprecated fee rate option `--sat_per_byte` will be removed in release version **0.22**

funding/commitment_type_negotiation.go

@@ -13,15 +13,33 @@ var (
 	// peer of the channel does not support it.
 	errUnsupportedChannelType = errors.New("requested channel type " +
 		"not supported")
+
+	// ErrDeprecatedChanType is returned when settling on the legacy
+	// commitment type is the only option left, either because the caller of
+	// our own RPC asked for it or because automatic selection would have
+	// fallen back to it. We keep operating the legacy channels we already
+	// have, but no longer open new ones.
+	//
+	// Unlike lnwire.ErrChanTypeDeprecated, which we send to a peer whose
+	// proposal we reject, this never goes on the wire. The audience is our
+	// own operator, who can act on the answer, so it spells out what to use
+	// instead.
+	ErrDeprecatedChanType = errors.New("the legacy commitment type is " +
+		"deprecated, new channels must use the static remote key " +
+		"commitment type or later")
 )
 
 // negotiateCommitmentType determines the commitment type of a newly opened
 // channel. If desiredChanType is provided, it is validated against the
 // commitment features supported by both peers. Otherwise, a default type is
 // selected from those features.
 //
-// The returned ChannelType is always non-nil and is always signaled on the
-// wire. An error is only returned if desiredChanType is not supported.
+// The legacy commitment type is never selected, whether it was requested
+// explicitly or would only have been reached by falling back.
+//
+// On success, the returned ChannelType is non-nil and is signaled on the wire.
+// An error is returned if the requested type is unsupported or deprecated, or
+// if no supported default type can be selected.
 func negotiateCommitmentType(desiredChanType *lnwire.ChannelType, local,
 	remote *lnwire.FeatureVector) (*lnwire.ChannelType,
 	lnwallet.CommitmentType, error) {
@@ -38,7 +56,12 @@ func negotiateCommitmentType(desiredChanType *lnwire.ChannelType, local,
 	// No specific channel type was requested. Select a default type based
 	// on locally-known feature compatibility. This default is then sent
 	// explicitly over the wire.
-	defaultChanType, commitType := selectDefaultChannelType(local, remote)
+	defaultChanType, commitType, err := selectDefaultChannelType(
+		local, remote,
+	)
+	if err != nil {
+		return nil, 0, err
+	}
 
 	return defaultChanType, commitType, nil
 }
@@ -413,9 +436,14 @@ func explicitNegotiateCommitmentType(channelType lnwire.ChannelType, local,
 
 		return lnwallet.CommitmentTypeSimpleTaprootOverlay, nil
 
-	// No features, use legacy commitment type.
+	// An empty channel type asks for the legacy commitment type, which was
+	// removed from the spec in 2024 and which we refuse outright, not by
+	// configuration. Note that this branch performs no feature check of its
+	// own, since the legacy type predates feature bits entirely: any peer
+	// sending an empty channel_type TLV used to get a legacy channel out of
+	// us no matter what either side signalled.
 	case channelFeatures.IsEmpty():
-		return lnwallet.CommitmentTypeLegacy, nil
+		return 0, lnwire.ErrChanTypeDeprecated
 
 	default:
 		return 0, errUnsupportedChannelType
@@ -427,11 +455,14 @@ func explicitNegotiateCommitmentType(channelType lnwire.ChannelType, local,
 // Taproot channels must be requested explicitly, so that defaults stay on
 // channel types usable for both public and private channels.
 //
+// An error is returned if there is no mutually supported type above the legacy
+// one, which we no longer open.
+//
 // TODO(yy): Revisit taproot channel selection once public taproot channel
 // announcements are supported.
 func selectDefaultChannelType(local,
 	remote *lnwire.FeatureVector) (*lnwire.ChannelType,
-	lnwallet.CommitmentType) {
+	lnwallet.CommitmentType, error) {
 
 	// If both peers are signalling support for anchor commitments with
 	// zero-fee HTLC transactions, we'll use this type.
@@ -441,7 +472,8 @@ func selectDefaultChannelType(local,
 			lnwire.StaticRemoteKeyRequired,
 		))
 
-		return &chanType, lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx
+		return &chanType, lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx,
+			nil
 	}
 
 	// Since we don't want to support the "legacy" anchor type, we will fall
@@ -455,12 +487,13 @@ func selectDefaultChannelType(local,
 			lnwire.StaticRemoteKeyRequired,
 		))
 
-		return &chanType, lnwallet.CommitmentTypeTweakless
+		return &chanType, lnwallet.CommitmentTypeTweakless, nil
 	}
 
-	// Otherwise we'll fall back to the legacy type.
-	chanType := lnwire.ChannelType(*lnwire.NewRawFeatureVector())
-	return &chanType, lnwallet.CommitmentTypeLegacy
+	// Without a mutually supported type above it, the only one left to fall
+	// back on is the legacy type, which we never open. Either side failing
+	// to signal static remote key is enough to end up here.
+	return nil, 0, ErrDeprecatedChanType
 }
 
 // hasFeatures determines whether a set of features is supported by both the set

funding/commitment_type_negotiation_test.go

@@ -227,7 +227,10 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 			expectsErr: nil,
 		},
 		{
-			name:            "explicit legacy",
+			// An empty channel type asks for the legacy commitment
+			// type, which we no longer open. Note that this used to
+			// be accepted with no feature check at all.
+			name:            "explicit legacy rejected",
 			channelFeatures: lnwire.NewRawFeatureVector(),
 			localFeatures: lnwire.NewRawFeatureVector(
 				lnwire.StaticRemoteKeyRequired,
@@ -237,11 +240,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 				lnwire.StaticRemoteKeyOptional,
 				lnwire.AnchorsZeroFeeHtlcTxOptional,
 			),
-			expectsCommitType: lnwallet.CommitmentTypeLegacy,
-			expectsChanType: (*lnwire.ChannelType)(
-				lnwire.NewRawFeatureVector(),
-			),
-			expectsErr: nil,
+			expectsErr: lnwire.ErrChanTypeDeprecated,
 		},
 		// No desired channel type is set, so we expect the default
 		// selection to return the corresponding chan type feature bits,
@@ -285,18 +284,18 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 			expectsErr: nil,
 		},
 		{
-			name:            "default legacy",
+			// Without mutual support for static remote key there is
+			// nothing left to fall back on but the legacy type.
+			// Note that the test runs both peer orderings, so it
+			// covers either side being the one that lacks it.
+			name:            "default legacy rejected",
 			channelFeatures: nil,
 			localFeatures:   lnwire.NewRawFeatureVector(),
 			remoteFeatures: lnwire.NewRawFeatureVector(
 				lnwire.StaticRemoteKeyOptional,
 				lnwire.AnchorsZeroFeeHtlcTxOptional,
 			),
-			expectsCommitType: lnwallet.CommitmentTypeLegacy,
-			expectsChanType: (*lnwire.ChannelType)(
-				lnwire.NewRawFeatureVector(),
-			),
-			expectsErr: nil,
+			expectsErr: ErrDeprecatedChanType,
 		},
 
 		// Test cases for final taproot channels with explicit
@@ -443,15 +442,19 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 			name:            "default ignores staging taproot without anchors",
 			channelFeatures: nil,
 			localFeatures: lnwire.NewRawFeatureVector(
+				lnwire.StaticRemoteKeyOptional,
 				lnwire.SimpleTaprootChannelsOptionalFinal,
 				lnwire.SimpleTaprootChannelsOptionalStaging,
 			),
 			remoteFeatures: lnwire.NewRawFeatureVector(
+				lnwire.StaticRemoteKeyOptional,
 				lnwire.SimpleTaprootChannelsOptionalStaging,
 			),
-			expectsCommitType: lnwallet.CommitmentTypeLegacy,
+			expectsCommitType: lnwallet.CommitmentTypeTweakless,
 			expectsChanType: (*lnwire.ChannelType)(
-				lnwire.NewRawFeatureVector(),
+				lnwire.NewRawFeatureVector(
+					lnwire.StaticRemoteKeyRequired,
+				),
 			),
 			expectsErr: nil,
 		},
@@ -460,14 +463,18 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
 			name:            "default ignores final taproot without anchors",
 			channelFeatures: nil,
 			localFeatures: lnwire.NewRawFeatureVector(
+				lnwire.StaticRemoteKeyOptional,
 				lnwire.SimpleTaprootChannelsOptionalFinal,
 			),
 			remoteFeatures: lnwire.NewRawFeatureVector(
+				lnwire.StaticRemoteKeyOptional,
 				lnwire.SimpleTaprootChannelsOptionalFinal,
 			),
-			expectsCommitType: lnwallet.CommitmentTypeLegacy,
+			expectsCommitType: lnwallet.CommitmentTypeTweakless,
 			expectsChanType: (*lnwire.ChannelType)(
-				lnwire.NewRawFeatureVector(),
+				lnwire.NewRawFeatureVector(
+					lnwire.StaticRemoteKeyRequired,
+				),
 			),
 			expectsErr: nil,
 		},

funding/manager_test.go

@@ -674,6 +674,17 @@ func createTestFundingManager(t *testing.T, privKey *btcec.PrivateKey,
 		shutdownChannel: shutdownChan,
 		reportScidChan:  reportScidChan,
 		addr:            addr,
+
+		// Default both sides to static remote key. Without any
+		// features at all these nodes used to negotiate the legacy
+		// commitment type, which we no longer open. Tests that care
+		// about a specific type overwrite these.
+		localFeatures: []lnwire.FeatureBit{
+			lnwire.StaticRemoteKeyOptional,
+		},
+		remoteFeatures: []lnwire.FeatureBit{
+			lnwire.StaticRemoteKeyOptional,
+		},
 	}
 
 	f.cfg.NotifyWhenOnline = func(peer [33]byte,
@@ -5324,6 +5335,65 @@ func TestFundingManagerRejectMissingChanType(t *testing.T) {
 	assertNumPendingReservations(t, bob, alicePubKey, 0)
 }
 
+// TestFundingManagerRejectLegacyChanType verifies that the fundee rejects an
+// OpenChannel message carrying an empty ChannelType, which asks for the legacy
+// commitment type.
+//
+// This branch used to perform no feature check whatsoever, since the legacy
+// type predates feature bits entirely, so any peer could obtain a legacy
+// channel from us no matter what either side signalled.
+func TestFundingManagerRejectLegacyChanType(t *testing.T) {
+	t.Parallel()
+
+	alice, bob := setupFundingManagers(t)
+	t.Cleanup(func() {
+		tearDownFundingManagers(t, alice, bob)
+	})
+
+	// Both peers support better than the legacy type, which is exactly the
+	// case that used to slip through.
+	featureBits := []lnwire.FeatureBit{
+		lnwire.StaticRemoteKeyOptional,
+		lnwire.AnchorsZeroFeeHtlcTxOptional,
+	}
+	alice.localFeatures = featureBits
+	alice.remoteFeatures = featureBits
+	bob.localFeatures = featureBits
+	bob.remoteFeatures = featureBits
+
+	emptyChanType := (*lnwire.ChannelType)(lnwire.NewRawFeatureVector())
+
+	openChannelReq := &lnwire.OpenChannel{
+		ChainHash:            *fundingNetParams.GenesisHash,
+		PendingChannelID:     [32]byte{0x01},
+		FundingAmount:        btcutil.Amount(10000000),
+		PushAmount:           0,
+		DustLimit:            btcutil.Amount(546),
+		MaxValueInFlight:     lnwire.MilliSatoshi(100000000),
+		ChannelReserve:       btcutil.Amount(10000),
+		HtlcMinimum:          lnwire.MilliSatoshi(1000),
+		FeePerKiloWeight:     15000,
+		CsvDelay:             144,
+		MaxAcceptedHTLCs:     483,
+		FundingKey:           alice.privKey.PubKey(),
+		RevocationPoint:      alice.privKey.PubKey(),
+		PaymentPoint:         alice.privKey.PubKey(),
+		DelayedPaymentPoint:  alice.privKey.PubKey(),
+		HtlcPoint:            alice.privKey.PubKey(),
+		FirstCommitmentPoint: alice.privKey.PubKey(),
+		ChannelType:          emptyChanType,
+	}
+	bob.fundingMgr.ProcessFundingMsg(openChannelReq, alice)
+
+	// Bob should reject the OpenChannel message instead of echoing the
+	// empty channel type back in an AcceptChannel.
+	errMsg := assertFundingMsgSent(t, bob.msgChan, "Error")
+	err, ok := errMsg.(*lnwire.Error)
+	require.True(t, ok)
+	require.Equal(t, lnwire.ErrChanTypeDeprecated.Error(), string(err.Data))
+	assertNumPendingReservations(t, bob, alicePubKey, 0)
+}
+
 // TestFundingManagerAcceptChanType verifies that the fundee accepts an
 // OpenChannel message that includes the ChannelType field and echoes it back
 // in AcceptChannel, even when neither peer advertises the explicit channel