Merge pull request #3893 from cfromknecht/pathfind-ignore-required-features

routing: ignore unknown required features in pathfinding

feature/required.go

@@ -0,0 +1,37 @@
+package feature
+
+import (
+	"fmt"
+
+	"github.com/lightningnetwork/lnd/lnwire"
+)
+
+// ErrUnknownRequired signals that a feature vector requires certain features
+// that our node is unaware of or does not implement.
+type ErrUnknownRequired struct {
+	unknown []lnwire.FeatureBit
+}
+
+// NewErrUnknownRequired initializes an ErrUnknownRequired with the unknown
+// feature bits.
+func NewErrUnknownRequired(unknown []lnwire.FeatureBit) ErrUnknownRequired {
+	return ErrUnknownRequired{
+		unknown: unknown,
+	}
+}
+
+// Error returns a human-readable description of the error.
+func (e ErrUnknownRequired) Error() string {
+	return fmt.Sprintf("feature vector contains unknown required "+
+		"features: %v", e.unknown)
+}
+
+// ValidateRequired returns an error if the feature vector contains a non-zero
+// number of unknown, required feature bits.
+func ValidateRequired(fv *lnwire.FeatureVector) error {
+	unknown := fv.UnknownRequiredFeatures()
+	if len(unknown) > 0 {
+		return NewErrUnknownRequired(unknown)
+	}
+	return nil
+}

peer.go

@@ -2460,7 +2460,7 @@ func (p *peer) handleInitMsg(msg *lnwire.Init) error {
 	// those presented in the local features fields.
 	err := msg.Features.Merge(msg.GlobalFeatures)
 	if err != nil {
-		return fmt.Errorf("unable to merge legacy global featues: %v",
+		return fmt.Errorf("unable to merge legacy global features: %v",
 			err)
 	}
 
@@ -2472,11 +2472,9 @@ func (p *peer) handleInitMsg(msg *lnwire.Init) error {
 
 	// Now that we have their features loaded, we'll ensure that they
 	// didn't set any required bits that we don't know of.
-	unknownFeatures := p.remoteFeatures.UnknownRequiredFeatures()
+	err = feature.ValidateRequired(p.remoteFeatures)
-	if len(unknownFeatures) > 0 {
+	if err != nil {
-		err := fmt.Errorf("peer set unknown feature bits: %v",
+		return fmt.Errorf("invalid remote features: %v", err)
-			unknownFeatures)
-		return err
 	}
 
 	// Ensure the remote party's feature vector contains all transistive
@@ -2484,7 +2482,7 @@ func (p *peer) handleInitMsg(msg *lnwire.Init) error {
 	// during the feature manager's instantiation.
 	err = feature.ValidateDeps(p.remoteFeatures)
 	if err != nil {
-		return fmt.Errorf("peer set invalid feature vector: %v", err)
+		return fmt.Errorf("invalid remote features: %v", err)
 	}
 
 	// Now that we know we understand their requirements, we'll check to

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
+	// Ensure that the destination's features don't include unknown
-	// transitive depdencies are set.
+	// 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)
+				nodeFeatures := targetNode.Features
-				if err == nil {
+
-					fromFeatures = 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:

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.

watchtower/wtwire/init.go

@@ -5,6 +5,7 @@ import (
 	"io"
 
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
+	"github.com/lightningnetwork/lnd/feature"
 	"github.com/lightningnetwork/lnd/lnwire"
 )
 
@@ -92,12 +93,7 @@ func (msg *Init) CheckRemoteInit(remoteInit *Init,
 
 	// Check that the remote peer doesn't have any required connection
 	// feature bits that we ourselves are unaware of.
-	unknownConnFeatures := remoteConnFeatures.UnknownRequiredFeatures()
+	return feature.ValidateRequired(remoteConnFeatures)
-	if len(unknownConnFeatures) > 0 {
-		return NewErrUnknownRequiredFeatures(unknownConnFeatures...)
-	}
-
-	return nil
 }
 
 // ErrUnknownChainHash signals that the remote Init has a different chain hash
@@ -116,24 +112,3 @@ func NewErrUnknownChainHash(hash chainhash.Hash) *ErrUnknownChainHash {
 func (e *ErrUnknownChainHash) Error() string {
 	return fmt.Sprintf("remote init has unknown chain hash: %s", e.hash)
 }
-
-// ErrUnknownRequiredFeatures signals that the remote Init has required feature
-// bits that were unknown to us.
-type ErrUnknownRequiredFeatures struct {
-	unknownFeatures []lnwire.FeatureBit
-}
-
-// NewErrUnknownRequiredFeatures creates an ErrUnknownRequiredFeatures using the
-// remote Init's required features that were unknown to us.
-func NewErrUnknownRequiredFeatures(
-	unknownFeatures ...lnwire.FeatureBit) *ErrUnknownRequiredFeatures {
-
-	return &ErrUnknownRequiredFeatures{unknownFeatures}
-}
-
-// Error returns a human-readable error displaying the unknown required feature
-// bits.
-func (e *ErrUnknownRequiredFeatures) Error() string {
-	return fmt.Sprintf("remote init has unknown required features: %v",
-		e.unknownFeatures)
-}

watchtower/wtwire/init_test.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/btcsuite/btcd/chaincfg"
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
+	"github.com/lightningnetwork/lnd/feature"
 	"github.com/lightningnetwork/lnd/lnwire"
 	"github.com/lightningnetwork/lnd/watchtower/wtwire"
 )
@@ -60,8 +61,8 @@ var checkRemoteInitTests = []checkRemoteInitTest{
 		lHash:     testnetChainHash,
 		rFeatures: lnwire.NewRawFeatureVector(lnwire.GossipQueriesRequired),
 		rHash:     testnetChainHash,
-		expErr: wtwire.NewErrUnknownRequiredFeatures(
+		expErr: feature.NewErrUnknownRequired(
-			lnwire.GossipQueriesRequired,
+			[]lnwire.FeatureBit{lnwire.GossipQueriesRequired},
 		),
 	},
 }

zpay32/invoice.go

@@ -956,14 +956,7 @@ func parseFeatures(data []byte) (*lnwire.FeatureVector, error) {
 		return nil, err
 	}
 
-	fv := lnwire.NewFeatureVector(rawFeatures, lnwire.Features)
+	return lnwire.NewFeatureVector(rawFeatures, lnwire.Features), nil
-	unknownFeatures := fv.UnknownRequiredFeatures()
-	if len(unknownFeatures) > 0 {
-		return nil, fmt.Errorf("invoice contains unknown required "+
-			"features: %v", unknownFeatures)
-	}
-
-	return fv, nil
 }
 
 // writeTaggedFields writes the non-nil tagged fields of the Invoice to the

zpay32/invoice_test.go

@@ -534,9 +534,8 @@ func TestDecodeEncode(t *testing.T) {
 		{
 			// On mainnet, please send $30 coffee beans supporting
 			// features 9, 15, 99, and 100, using secret 0x11...
-			encodedInvoice: "lnbc25m1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdq5vdhkven9v5sxyetpdeessp5zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zygs9q4psqqqqqqqqqqqqqqqpqqqqu7fz6pjqczdm3jp3qps7xntj2w2mm70e0ckhw3c5xk9p36pvk3sewn7ncaex6uzfq0vtqzy28se6pcwn790vxex7xystzumhg55p6qq9wq7td",
+			encodedInvoice: "lnbc25m1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdq5vdhkven9v5sxyetpdeessp5zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zygs9q4psqqqqqqqqqqqqqqqpqsqq40wa3khl49yue3zsgm26jrepqr2eghqlx86rttutve3ugd05em86nsefzh4pfurpd9ek9w2vp95zxqnfe2u7ckudyahsa52q66tgzcp6t2dyk",
-			valid:          false,
+			valid:          true,
-			skipEncoding:   true,
 			decodedInvoice: func() *Invoice {
 				return &Invoice{
 					Net:         &chaincfg.MainNetParams,
@@ -710,7 +709,7 @@ func TestDecodeEncode(t *testing.T) {
 		}
 
 		if test.valid {
-			if err := compareInvoices(test.decodedInvoice(), invoice); err != nil {
+			if err := compareInvoices(decodedInvoice, invoice); err != nil {
 				t.Errorf("Invoice decoding result %d not as expected: %v", i, err)
 				return
 			}