From f9a1acfbe42c1f06d31983760bd654495d12c82e Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Wed, 8 Jan 2020 12:24:37 -0800 Subject: [PATCH 1/5] feature/required: add ValidateRequired method This wraps the raw UnknownRequiredFeatures method and returns a proper error type to enable more exact testing. --- feature/required.go | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 feature/required.go diff --git a/feature/required.go b/feature/required.go new file mode 100644 index 000000000..d141604df --- /dev/null +++ b/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 +} From c7a241fc5974df3ec9f17a78b280bf98645dbaf8 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Wed, 8 Jan 2020 12:25:00 -0800 Subject: [PATCH 2/5] 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 a6bd58785..23bc1f9e9 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 c638d8512..03b3160a4 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. From b9b66419ff9453e357ca887614e29cbc64e1df55 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Wed, 8 Jan 2020 12:25:21 -0800 Subject: [PATCH 3/5] zpay32/invoice: remove unknown required fbit check from decode This commit removes the unknown required feature bit check from the invoice decoding logic. This allows greater utility to users of the decodepayreq rpc since it can provide inspection of otherwise invalid invoices. In the prior commit, this check moved into our path finding logic, so invalid features taken from an invoice will instead cause a failure when attempting to pay. --- zpay32/invoice.go | 9 +-------- zpay32/invoice_test.go | 7 +++---- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/zpay32/invoice.go b/zpay32/invoice.go index 3f8dcc2d5..691c01697 100644 --- a/zpay32/invoice.go +++ b/zpay32/invoice.go @@ -956,14 +956,7 @@ func parseFeatures(data []byte) (*lnwire.FeatureVector, error) { return nil, err } - fv := lnwire.NewFeatureVector(rawFeatures, lnwire.Features) - unknownFeatures := fv.UnknownRequiredFeatures() - if len(unknownFeatures) > 0 { - return nil, fmt.Errorf("invoice contains unknown required "+ - "features: %v", unknownFeatures) - } - - return fv, nil + return lnwire.NewFeatureVector(rawFeatures, lnwire.Features), nil } // writeTaggedFields writes the non-nil tagged fields of the Invoice to the diff --git a/zpay32/invoice_test.go b/zpay32/invoice_test.go index 923e692e6..41687a690 100644 --- a/zpay32/invoice_test.go +++ b/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", - valid: false, - skipEncoding: true, + encodedInvoice: "lnbc25m1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdq5vdhkven9v5sxyetpdeessp5zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zygs9q4psqqqqqqqqqqqqqqqpqsqq40wa3khl49yue3zsgm26jrepqr2eghqlx86rttutve3ugd05em86nsefzh4pfurpd9ek9w2vp95zxqnfe2u7ckudyahsa52q66tgzcp6t2dyk", + valid: 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 } From 0f9023256e5a0c1e5709d8864d612b4ec68361c3 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Wed, 8 Jan 2020 12:25:41 -0800 Subject: [PATCH 4/5] peer: use feature.ValidateRequired, cleanup error msgs --- peer.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/peer.go b/peer.go index 7a9014135..a95689043 100644 --- a/peer.go +++ b/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() - if len(unknownFeatures) > 0 { - err := fmt.Errorf("peer set unknown feature bits: %v", - unknownFeatures) - return err + err = feature.ValidateRequired(p.remoteFeatures) + if err != nil { + return fmt.Errorf("invalid remote features: %v", 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 From 2510ec00f5abc648248562d1a876f876a77b91ff Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Wed, 8 Jan 2020 12:26:00 -0800 Subject: [PATCH 5/5] watchtower/wtwire/init: use feature.ValidateRequired This allows us to remove the custom error type originally implemented for this purpose. --- watchtower/wtwire/init.go | 29 ++--------------------------- watchtower/wtwire/init_test.go | 5 +++-- 2 files changed, 5 insertions(+), 29 deletions(-) diff --git a/watchtower/wtwire/init.go b/watchtower/wtwire/init.go index 79a5fbf8b..4d5ec34bd 100644 --- a/watchtower/wtwire/init.go +++ b/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() - if len(unknownConnFeatures) > 0 { - return NewErrUnknownRequiredFeatures(unknownConnFeatures...) - } - - return nil + return feature.ValidateRequired(remoteConnFeatures) } // 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) -} diff --git a/watchtower/wtwire/init_test.go b/watchtower/wtwire/init_test.go index 1aee5530b..c0b0fa751 100644 --- a/watchtower/wtwire/init_test.go +++ b/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( - lnwire.GossipQueriesRequired, + expErr: feature.NewErrUnknownRequired( + []lnwire.FeatureBit{lnwire.GossipQueriesRequired}, ), }, }