From 403d37f426989027304b7d358f616e7617e4f948 Mon Sep 17 00:00:00 2001
From: Olaoluwa Osuntokun <laolu32@gmail.com>
Date: Thu, 17 Dec 2020 15:49:55 -0800
Subject: [PATCH] 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      |  9 ++++++++-
 peer/brontide_test.go | 12 +++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/peer/brontide.go b/peer/brontide.go
index 3dc92e11..fda50811 100644
--- a/peer/brontide.go
+++ b/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,
 	)
 
diff --git a/peer/brontide_test.go b/peer/brontide_test.go
index 77734d66..2141a945 100644
--- a/peer/brontide_test.go
+++ b/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,
 			},
 		},