peer: ensure consistent local+global feature bits when downgrading

In this commit, we fix an issue that would cause peers running lnd 0.12
to not be able to connect to existing peers due to a feature bit
compatibility issue. In a recent PR we started to downgrade our required
feature bit for static key from required to optional, if we had a legacy
(non-tweakless) open with the peer then we would unset the required bit
and set the optional bit to ensure we could still connect to them.

The change implementing this new version of downgrade failed _also_
unset the bit (the required bit) in the "legacy global" feature bit
section. This caused the `RawFeatureVector.Merge` method to fail as we
would have the required bit set in the `GlobalFeatures` section, but the
optional bit set in the `Features` section. The `Merge` method ensures
that a required and optional bit can't be set in two different locations
for the same feature.

This PR fixes this issue by also unsetting the bit in the
`GlobalFeatures` field in the init message.

Fixes #4871

peer/brontide.go

@@ -2775,6 +2775,7 @@ func (p *Brontide) RemoteFeatures() *lnwire.FeatureVector {
 // our currently supported local and global features.
 func (p *Brontide) sendInitMsg(legacyChan bool) error {
 	features := p.cfg.Features.Clone()
+	legacyFeatures := p.cfg.LegacyFeatures.Clone()
 
 	// If we have a legacy channel open with a peer, we downgrade static
 	// remote required to optional in case the peer does not understand the
@@ -2786,12 +2787,18 @@ func (p *Brontide) sendInitMsg(legacyChan bool) error {
 			"downgrading static remote required feature bit to "+
 			"optional", p.PubKey())
 
+		// Unset and set in both the local and global features to
+		// ensure both sets are consistent and merge able by old and
+		// new nodes.
 		features.Unset(lnwire.StaticRemoteKeyRequired)
+		legacyFeatures.Unset(lnwire.StaticRemoteKeyRequired)
+
 		features.Set(lnwire.StaticRemoteKeyOptional)
+		legacyFeatures.Set(lnwire.StaticRemoteKeyOptional)
 	}
 
 	msg := lnwire.NewInitMessage(
-		p.cfg.LegacyFeatures.RawFeatureVector,
+		legacyFeatures.RawFeatureVector,
 		features.RawFeatureVector,
 	)
 

peer/brontide_test.go

@@ -875,6 +875,11 @@ func TestStaticRemoteDowngrade(t *testing.T) {
 		)
 		legacy = lnwire.NewFeatureVector(rawLegacy, nil)
 
+		legacyCombinedOptional = lnwire.NewRawFeatureVector(
+			lnwire.UpfrontShutdownScriptOptional,
+			lnwire.StaticRemoteKeyOptional,
+		)
+
 		rawFeatureOptional = lnwire.NewRawFeatureVector(
 			lnwire.StaticRemoteKeyOptional,
 		)
@@ -925,12 +930,17 @@ func TestStaticRemoteDowngrade(t *testing.T) {
 				Features:       rawFeatureRequired,
 			},
 		},
+
+		// In this case we need to flip our required bit to optional,
+		// this should also propagate to the legacy set of feature bits
+		// so we have proper consistency: a bit isn't set to optional
+		// in one field and required in the other.
 		{
 			name:     "legacy channel, static required",
 			legacy:   true,
 			features: featureRequired,
 			expectedInit: &lnwire.Init{
-				GlobalFeatures: rawLegacy,
+				GlobalFeatures: legacyCombinedOptional,
 				Features:       rawFeatureOptional,
 			},
 		},