From 525ef594c79973c35cbc965aa062399c9bc59566 Mon Sep 17 00:00:00 2001
From: Olaoluwa Osuntokun <laolu32@gmail.com>
Date: Tue, 6 Jul 2021 18:41:22 -0700
Subject: [PATCH] routing: don't return an error from failPayment within
 handleSendError

In this commit, we fix a regression introduced by a recent bug fix in
this area. Before this change, we'd inspect the error returned by
`processSendError`, and then fail the payment from the PoV of mission
control using the returned error.

A recent refactoring removed `processSendError` and combined the logic
with `tryApplyChannelUpdate` in order to introduce a new
`handleSendError` method that consolidates the logic within the
`shardHandler`. Along the way, the behavior of the prior check was
replicated in the form of a new internal `failPayment` closure. However,
the new function closure ends up returning a `channeldb.FailureReason`
instance, which is actually an `error`.

In the wild, when `SendToRoute` fails due to an error at the
destination, then this new logic caused the `handleSendErorr` method to
fail with an error, returning an unstructured error back to the caller,
instead of the usual payment failure details.

We fix this by no longer checking the `handleSendErorr` for an error as
normal. The `handleSendErorr` function as is will always return an error
of type `*channeldb.FailureReason`, therefore we don't need to treat it
as a normal error. Instead, we check for the type of error returned, and
update the control tower state accordingly.

With this commit, the test added in the prior commit now passes.

Fixes #5477.
---
 routing/payment_lifecycle.go |  7 +++++--
 routing/router.go            | 23 +++++++++++++++++------
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/routing/payment_lifecycle.go b/routing/payment_lifecycle.go
index 96f58b45..aa856e7b 100644
--- a/routing/payment_lifecycle.go
+++ b/routing/payment_lifecycle.go
@@ -782,10 +782,13 @@ func (p *shardHandler) handleSendError(attempt *channeldb.HTLCAttemptInfo,
 		if err := p.router.cfg.Control.Fail(
 			p.identifier, *reason); err != nil {
 
-			return err
+			log.Errorf("unable to report failure to control "+
+				"tower: %v", err)
+
+			return &internalErrorReason
 		}
 
-		return *reason
+		return reason
 	}
 
 	// reportFail is a helper closure that reports the failure to the
diff --git a/routing/router.go b/routing/router.go
index 1f88b02a..e6f4c321 100644
--- a/routing/router.go
+++ b/routing/router.go
@@ -2,6 +2,7 @@ package routing
 
 import (
 	"bytes"
+	goErrors "errors"
 	"fmt"
 	"runtime"
 	"strings"
@@ -2163,13 +2164,23 @@ func (r *ChannelRouter) SendToRoute(htlcHash lntypes.Hash, rt *route.Route) (
 	// mark the payment failed with the control tower immediately. Process
 	// the error to check if it maps into a terminal error code, if not use
 	// a generic NO_ROUTE error.
-	if err := sh.handleSendError(attempt, shardError); err != nil {
-		return nil, err
-	}
+	var failureReason *channeldb.FailureReason
+	err = sh.handleSendError(attempt, shardError)
 
-	err = r.cfg.Control.Fail(
-		paymentIdentifier, channeldb.FailureReasonNoRoute,
-	)
+	switch {
+	// If we weren't able to extract a proper failure reason (which can
+	// happen if the second chance logic is triggered), then we'll use the
+	// normal no route error.
+	case err == nil:
+		err = r.cfg.Control.Fail(
+			paymentIdentifier, channeldb.FailureReasonNoRoute,
+		)
+
+	// If this is a failure reason, then we'll apply the failure directly
+	// to the control tower, and return the normal response to the caller.
+	case goErrors.As(err, &failureReason):
+		err = r.cfg.Control.Fail(paymentIdentifier, *failureReason)
+	}
 	if err != nil {
 		return nil, err
 	}