From bd9f1b597ef5f74be85c489fc50a7c75487b6ccd Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Mon, 23 Apr 2018 17:50:56 -0700 Subject: [PATCH 1/2] routing: prune edges instead of vertexes in response to an FailUnknownNextPeer error In this commit we fix an lingering bug in the Mission Control logic we execute in response to the FailUnknownNextPeer error. Historically, we would treat this as the _next_ node not being online. As a result, we would then prune away the vertex from the current reachable graph all together. It was recently realized, that this would at times be a bit _tooo_ aggressive if the channel we attempt to route over was faulty, down, or the incoming node had connectivity issues with the outgoing node. In light of this realization, we'll now instead only prune the _edge_ that we attempted to route over. This ensures that we'll continue to explore the possible edges. Additionally, this guards us against failure modes where nodes report FailUnknownNextPeer to other nodes in an attempt to more closely control our retry logic. This change is a stop gap on the path to a more intelligent set of autopilot heuristics. Fixes #1114. --- routing/router.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/routing/router.go b/routing/router.go index bb1e4b62..833989b8 100644 --- a/routing/router.go +++ b/routing/router.go @@ -1819,12 +1819,14 @@ func (r *ChannelRouter) SendPayment(payment *LightningPayment) ([32]byte, *Route continue // If the next hop in the route wasn't known or - // offline, we'll prune the _next_ hop from the set of - // routes and retry. + // offline, we'll only the channel which we attempted + // to route over. This is conservative, and it can + // handle faulty channels between nodes properly. + // Additionally, this guards against routing nodes + // returning errors in order to attempt to black list + // another node. case *lnwire.FailUnknownNextPeer: - pruneVertexFailure( - paySession, route, errSource, true, - ) + pruneEdgeFailure(paySession, route, errSource) continue // If the node wasn't able to forward for which ever From 76b9501e048617be9a7ce8a20bdb8555e76ad842 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Mon, 23 Apr 2018 17:53:41 -0700 Subject: [PATCH 2/2] routing: update TestSendPaymentErrorPathPruning due to recent UnknownPeer change In this commit, we update the TestSendPaymentErrorPathPruning test to reflect the new behavior w.r.t how we respond to UnknownPeer errors. In this new test, we expect that we'll find alternative route in light of us getting an UnknownPeer error "pointing" to our destination node. --- routing/router_test.go | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/routing/router_test.go b/routing/router_test.go index b7029e67..1330ed54 100644 --- a/routing/router_test.go +++ b/routing/router_test.go @@ -607,20 +607,34 @@ func TestSendPaymentErrorPathPruning(t *testing.T) { return preImage, nil } - // The final error returned should also indicate that the peer wasn't - // online (the last error we returned). - _, _, err = ctx.router.SendPayment(&payment) - if err == nil { - t.Fatalf("payment didn't return error") + // This shouldn't return an error, as we'll make a payment attempt via + // the satoshi channel based on the assumption that there might be an + // intermittent issue with the roasbeef <-> lioji channel. + paymentPreImage, route, err := ctx.router.SendPayment(&payment) + if err != nil { + t.Fatalf("unable send payment: %v", err) } - if !strings.Contains(err.Error(), "UnknownNextPeer") { - t.Fatalf("expected UnknownNextPeer instead got: %v", err) + + // This path should go: roasbeef -> satoshi -> luoji + if len(route.Hops) != 2 { + t.Fatalf("incorrect route length: expected %v got %v", 2, + len(route.Hops)) + } + if !bytes.Equal(paymentPreImage[:], preImage[:]) { + t.Fatalf("incorrect preimage used: expected %x got %x", + preImage[:], paymentPreImage[:]) + } + if route.Hops[0].Channel.Node.Alias != "satoshi" { + t.Fatalf("route should go through satoshi as first hop, "+ + "instead passes through: %v", + route.Hops[0].Channel.Node.Alias) } ctx.router.missionControl.ResetHistory() // Finally, we'll modify the SendToSwitch function to indicate that the - // roasbeef -> luoji channel has insufficient capacity. + // roasbeef -> luoji channel has insufficient capacity. This should + // again cause us to instead go via the satoshi route. ctx.router.cfg.SendToSwitch = func(n [33]byte, _ *lnwire.UpdateAddHTLC, _ *sphinx.Circuit) ([32]byte, error) { if bytes.Equal(ctx.aliases["luoji"].SerializeCompressed(), n[:]) { @@ -635,7 +649,7 @@ func TestSendPaymentErrorPathPruning(t *testing.T) { return preImage, nil } - paymentPreImage, route, err := ctx.router.SendPayment(&payment) + paymentPreImage, route, err = ctx.router.SendPayment(&payment) if err != nil { t.Fatalf("unable to send payment: %v", err) }