From fe7cad2695e5da99d73343a0e53a2550503ed141 Mon Sep 17 00:00:00 2001
From: Carla Kirk-Cohen <kirkcohenc@gmail.com>
Date: Mon, 3 Apr 2023 13:14:10 +0200
Subject: [PATCH] features: disallow unsetting of config-set features

---
 feature/manager.go               | 42 +++++++++++++++++++++++++---
 feature/manager_internal_test.go | 48 ++++++++++++++++++++++++++++++--
 2 files changed, 84 insertions(+), 6 deletions(-)

diff --git a/feature/manager.go b/feature/manager.go
index c1f036fd9..e7db9899a 100644
--- a/feature/manager.go
+++ b/feature/manager.go
@@ -7,9 +7,15 @@ import (
 	"github.com/lightningnetwork/lnd/lnwire"
 )
 
-// ErrUnknownSet is returned if a proposed feature vector contains a set that
-// is unknown to LND.
-var ErrUnknownSet = errors.New("unknown feature bit set")
+var (
+	// ErrUnknownSet is returned if a proposed feature vector contains a
+	// set that is unknown to LND.
+	ErrUnknownSet = errors.New("unknown feature bit set")
+
+	// ErrFeatureConfigured is returned if an attempt is made to unset a
+	// feature that was configured at startup.
+	ErrFeatureConfigured = errors.New("can't unset configured feature")
+)
 
 // Config houses any runtime modifications to the default set descriptors. For
 // our purposes, this typically means disabling certain features to test legacy
@@ -61,6 +67,11 @@ type Manager struct {
 	// fsets is a static map of feature set to raw feature vectors. Requests
 	// are fulfilled by cloning these internal feature vectors.
 	fsets map[Set]*lnwire.RawFeatureVector
+
+	// configFeatures is a set of custom features that were "hard set" in
+	// lnd's config that cannot be updated at runtime (as is the case with
+	// our "standard" features that are defined in LND).
+	configFeatures map[Set]*lnwire.FeatureVector
 }
 
 // NewManager creates a new feature Manager, applying any custom modifications
@@ -99,6 +110,7 @@ func newManager(cfg Config, desc setDesc) (*Manager, error) {
 	}
 
 	// Now, remove any features as directed by the config.
+	configFeatures := make(map[Set]*lnwire.FeatureVector)
 	for set, raw := range fsets {
 		if cfg.NoTLVOnion {
 			raw.Unset(lnwire.TLVOnionPayloadOptional)
@@ -177,6 +189,15 @@ func newManager(cfg Config, desc setDesc) (*Manager, error) {
 					"feature: %d", err, custom)
 			}
 		}
+
+		// Track custom features separately so that we can check that
+		// they aren't unset in subsequent updates. If there is no
+		// entry for the set, the vector will just be empty.
+		configFeatures[set] = lnwire.NewFeatureVector(
+			lnwire.NewRawFeatureVector(cfg.CustomFeatures[set]...),
+			lnwire.Features,
+		)
+
 		// Ensure that all of our feature sets properly set any
 		// dependent features.
 		fv := lnwire.NewFeatureVector(raw, lnwire.Features)
@@ -188,7 +209,8 @@ func newManager(cfg Config, desc setDesc) (*Manager, error) {
 	}
 
 	return &Manager{
-		fsets: fsets,
+		fsets:          fsets,
+		configFeatures: configFeatures,
 	}, nil
 }
 
@@ -248,6 +270,18 @@ func (m *Manager) UpdateFeatureSets(
 			return err
 		}
 
+		// If any features were configured for this set, ensure that
+		// they are still set in the new feature vector.
+		if cfgFeat, haveCfgFeat := m.configFeatures[set]; haveCfgFeat {
+			for feature := range cfgFeat.Features() {
+				if !newFeatures.IsSet(feature) {
+					return fmt.Errorf("%w: can't unset: "+
+						"%d", ErrFeatureConfigured,
+						feature)
+				}
+			}
+		}
+
 		fv := lnwire.NewFeatureVector(newFeatures, lnwire.Features)
 		if err := ValidateDeps(fv); err != nil {
 			return err
diff --git a/feature/manager_internal_test.go b/feature/manager_internal_test.go
index 04422cd9b..ee716fba0 100644
--- a/feature/manager_internal_test.go
+++ b/feature/manager_internal_test.go
@@ -157,6 +157,7 @@ func TestUpdateFeatureSets(t *testing.T) {
 	testCases := []struct {
 		name     string
 		features map[Set]*lnwire.RawFeatureVector
+		config   Config
 		err      error
 	}{
 		{
@@ -211,6 +212,49 @@ func TestUpdateFeatureSets(t *testing.T) {
 				),
 			},
 		},
+		{
+			name: "missing configured feature",
+			features: map[Set]*lnwire.RawFeatureVector{
+				SetInit: lnwire.NewRawFeatureVector(
+					lnwire.DataLossProtectRequired,
+				),
+				SetNodeAnn: lnwire.NewRawFeatureVector(
+					lnwire.DataLossProtectRequired,
+					lnwire.GossipQueriesOptional,
+				),
+			},
+			config: Config{
+				CustomFeatures: map[Set][]lnwire.FeatureBit{
+					SetInit: {
+						lnwire.FeatureBit(333),
+					},
+				},
+			},
+			err: ErrFeatureConfigured,
+		},
+		{
+			name: "valid",
+			features: map[Set]*lnwire.RawFeatureVector{
+				SetInit: lnwire.NewRawFeatureVector(
+					lnwire.DataLossProtectRequired,
+				),
+				SetNodeAnn: lnwire.NewRawFeatureVector(
+					lnwire.DataLossProtectRequired,
+					lnwire.GossipQueriesOptional,
+					lnwire.FeatureBit(500),
+				),
+				SetInvoice: lnwire.NewRawFeatureVector(
+					lnwire.FeatureBit(333),
+				),
+			},
+			config: Config{
+				CustomFeatures: map[Set][]lnwire.FeatureBit{
+					SetInvoice: {
+						lnwire.FeatureBit(333),
+					},
+				},
+			},
+		},
 	}
 
 	for _, testCase := range testCases {
@@ -218,7 +262,7 @@ func TestUpdateFeatureSets(t *testing.T) {
 		t.Run(testCase.name, func(t *testing.T) {
 			t.Parallel()
 
-			featureMgr, err := newManager(Config{}, setDesc)
+			featureMgr, err := newManager(testCase.config, setDesc)
 			require.NoError(t, err)
 
 			err = featureMgr.UpdateFeatureSets(testCase.features)
@@ -231,7 +275,7 @@ func TestUpdateFeatureSets(t *testing.T) {
 			actual := featureMgr
 			if err != nil {
 				originalMgr, err := newManager(
-					Config{}, setDesc,
+					testCase.config, setDesc,
 				)
 				require.NoError(t, err)
 				expected = originalMgr.fsets