From c7a241fc5974df3ec9f17a78b280bf98645dbaf8 Mon Sep 17 00:00:00 2001
From: Conner Fromknecht <conner@lightning.engineering>
Date: Wed, 8 Jan 2020 12:25:00 -0800
Subject: [PATCH] routing/pathfind: ignore unknown required features

This commit brings us inline with recent modifications to the spec, that
say we shouldn't pay nodes whose feature vectors signal unknown required
features, and also that we shouldn't route through nodes signaling
unknown required features.

Currently we assert that invoices don't have such features during
decoding, but now that users can specify feature vectors via the rpc
interface, it makes sense to perform this check deeper in call stack.
This will also allow us to remove the check from decoding entirely,
making decodepayreq more useful for debugging.
---
 routing/pathfind.go      | 29 +++++++++++++---
 routing/pathfind_test.go | 71 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+), 5 deletions(-)

diff --git a/routing/pathfind.go b/routing/pathfind.go
index a6bd5878..23bc1f9e 100644
--- a/routing/pathfind.go
+++ b/routing/pathfind.go
@@ -456,8 +456,14 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig,
 		}
 	}
 
-	// With the destination's feature vector selected, ensure that all
-	// transitive depdencies are set.
+	// Ensure that the destination's features don't include unknown
+	// required features.
+	err = feature.ValidateRequired(features)
+	if err != nil {
+		return nil, err
+	}
+
+	// Ensure that all transitive dependencies are set.
 	err = feature.ValidateDeps(features)
 	if err != nil {
 		return nil, err
@@ -752,11 +758,24 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig,
 
 			// If the node exists and has valid features, use them.
 			case err == nil:
-				err := feature.ValidateDeps(targetNode.Features)
-				if err == nil {
-					fromFeatures = targetNode.Features
+				nodeFeatures := targetNode.Features
+
+				// Don't route through nodes that contain
+				// unknown required features.
+				err = feature.ValidateRequired(nodeFeatures)
+				if err != nil {
+					break
 				}
 
+				// Don't route through nodes that don't properly
+				// set all transitive feature dependencies.
+				err = feature.ValidateDeps(nodeFeatures)
+				if err != nil {
+					break
+				}
+
+				fromFeatures = nodeFeatures
+
 			// If an error other than the node not existing is hit,
 			// abort.
 			case err != channeldb.ErrGraphNodeNotFound:
diff --git a/routing/pathfind_test.go b/routing/pathfind_test.go
index c638d851..03b3160a 100644
--- a/routing/pathfind_test.go
+++ b/routing/pathfind_test.go
@@ -80,6 +80,10 @@ var (
 			lnwire.PaymentAddrOptional,
 		), lnwire.Features,
 	)
+
+	unknownRequiredFeatures = lnwire.NewFeatureVector(
+		lnwire.NewRawFeatureVector(100), lnwire.Features,
+	)
 )
 
 var (
@@ -1645,6 +1649,73 @@ func TestMissingFeatureDep(t *testing.T) {
 	}
 }
 
+// TestUnknownRequiredFeatures asserts that we fail path finding when the
+// destination requires an unknown required feature, and that we skip
+// intermediaries that signal unknown required features.
+func TestUnknownRequiredFeatures(t *testing.T) {
+	t.Parallel()
+
+	testChannels := []*testChannel{
+		asymmetricTestChannel("roasbeef", "conner", 100000,
+			&testChannelPolicy{
+				Expiry:  144,
+				FeeRate: 400,
+				MinHTLC: 1,
+				MaxHTLC: 100000000,
+			},
+			&testChannelPolicy{
+				Expiry:   144,
+				FeeRate:  400,
+				MinHTLC:  1,
+				MaxHTLC:  100000000,
+				Features: unknownRequiredFeatures,
+			}, 0,
+		),
+		asymmetricTestChannel("conner", "joost", 100000,
+			&testChannelPolicy{
+				Expiry:   144,
+				FeeRate:  400,
+				MinHTLC:  1,
+				MaxHTLC:  100000000,
+				Features: unknownRequiredFeatures,
+			},
+			&testChannelPolicy{
+				Expiry:  144,
+				FeeRate: 400,
+				MinHTLC: 1,
+				MaxHTLC: 100000000,
+			}, 0,
+		),
+	}
+
+	ctx := newPathFindingTestContext(t, testChannels, "roasbeef")
+	defer ctx.cleanup()
+
+	conner := ctx.keyFromAlias("conner")
+	joost := ctx.keyFromAlias("joost")
+
+	// Conner's node in the graph has an unknown required feature (100).
+	// Pathfinding should fail since we check the destination's features for
+	// unknown required features before beginning pathfinding.
+	expErr := feature.NewErrUnknownRequired([]lnwire.FeatureBit{100})
+	_, err := ctx.findPath(conner, 100)
+	if !reflect.DeepEqual(err, expErr) {
+		t.Fatalf("path shouldn't have been found: %v", err)
+	}
+
+	// Now, try to find a route to joost through conner. The destination
+	// features are valid, but conner's feature vector in the graph still
+	// requires feature 100. We expect errNoPathFound and not the error
+	// above since intermediate hops are simply skipped if they have invalid
+	// feature vectors, leaving no possible route to joost. This asserts
+	// that we don't try to route _through_ nodes with unknown required
+	// features.
+	_, err = ctx.findPath(joost, 100)
+	if err != errNoPathFound {
+		t.Fatalf("path shouldn't have been found: %v", err)
+	}
+}
+
 // TestDestPaymentAddr asserts that we properly detect when we can send a
 // payment address to a receiver, and also that we fallback to the receiver's
 // node announcement if we don't have an invoice features.