routing/payment_lifecycle: return recorded errors

In preparation for MPP we return the terminal errors recorded with the
control tower. The reason is that we cannot return immediately when a
shard fails for MPP, since there might be more shards in flight that we
must wait for. For that reason we instead mark the payment failed in the
control tower, then return this error when we inspect the payment,
seeing it has been failed and there are no shards in flight.

lntest/itest/lnd_test.go

@@ -1915,8 +1915,9 @@ func testUpdateChannelPolicy(net *lntest.NetworkHarness, t *harnessTest) {
 
 	// Alice knows about the channel policy of Carol and should therefore
 	// not be able to find a path during routing.
+	expErr := channeldb.FailureReasonNoRoute.Error()
 	if err == nil ||
-		!strings.Contains(err.Error(), "unable to find a path") {
+		!strings.Contains(err.Error(), expErr) {
 		t.Fatalf("expected payment to fail, instead got %v", err)
 	}
 
@@ -3995,6 +3996,41 @@ func assertAmountSent(amt btcutil.Amount, sndr, rcvr *lntest.HarnessNode) func()
 	}
 }
 
+// assertLastHTLCError checks that the last sent HTLC of the last payment sent
+// by the given node failed with the expected failure code.
+func assertLastHTLCError(t *harnessTest, node *lntest.HarnessNode,
+	code lnrpc.Failure_FailureCode) {
+
+	req := &lnrpc.ListPaymentsRequest{
+		IncludeIncomplete: true,
+	}
+	ctxt, _ := context.WithTimeout(context.Background(), defaultTimeout)
+	paymentsResp, err := node.ListPayments(ctxt, req)
+	if err != nil {
+		t.Fatalf("error when obtaining payments: %v", err)
+	}
+
+	payments := paymentsResp.Payments
+	if len(payments) == 0 {
+		t.Fatalf("no payments found")
+	}
+
+	payment := payments[len(payments)-1]
+	htlcs := payment.Htlcs
+	if len(htlcs) == 0 {
+		t.Fatalf("no htlcs")
+	}
+
+	htlc := htlcs[len(htlcs)-1]
+	if htlc.Failure == nil {
+		t.Fatalf("expected failure")
+	}
+
+	if htlc.Failure.Code != code {
+		t.Fatalf("expected failure %v, got %v", code, htlc.Failure.Code)
+	}
+}
+
 // testSphinxReplayPersistence verifies that replayed onion packets are rejected
 // by a remote peer after a restart. We use a combination of unsafe
 // configuration arguments to force Carol to replay the same sphinx packet after
@@ -4134,11 +4170,10 @@ func testSphinxReplayPersistence(net *lntest.NetworkHarness, t *harnessTest) {
 
 	// Construct the response we expect after sending a duplicate packet
 	// that fails due to sphinx replay detection.
-	replayErr := "InvalidOnionKey"
+	if resp.PaymentError == "" {
-	if !strings.Contains(resp.PaymentError, replayErr) {
+		t.Fatalf("expected payment error")
-		t.Fatalf("received payment error: %v, expected %v",
-			resp.PaymentError, replayErr)
 	}
+	assertLastHTLCError(t, carol, lnrpc.Failure_INVALID_ONION_KEY)
 
 	// Since the payment failed, the balance should still be left
 	// unaltered.
@@ -9452,12 +9487,11 @@ out:
 		t.Fatalf("payment should have been rejected due to invalid " +
 			"payment hash")
 	}
-	expectedErrorCode := lnwire.CodeIncorrectOrUnknownPaymentDetails.String()
+
-	if !strings.Contains(resp.PaymentError, expectedErrorCode) {
+	assertLastHTLCError(
-		// TODO(roasbeef): make into proper gRPC error code
+		t, net.Alice,
-		t.Fatalf("payment should have failed due to unknown payment hash, "+
+		lnrpc.Failure_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS,
-			"instead failed due to: %v", resp.PaymentError)
+	)
-	}
 
 	// The balances of all parties should be the same as initially since
 	// the HTLC was canceled.
@@ -9484,18 +9518,11 @@ out:
 		t.Fatalf("payment should have been rejected due to wrong " +
 			"HTLC amount")
 	}
-	expectedErrorCode = lnwire.CodeIncorrectOrUnknownPaymentDetails.String()
-	if !strings.Contains(resp.PaymentError, expectedErrorCode) {
-		t.Fatalf("payment should have failed due to wrong amount, "+
-			"instead failed due to: %v", resp.PaymentError)
-	}
 
-	// We'll also ensure that the encoded error includes the invlaid HTLC
+	assertLastHTLCError(
-	// amount.
+		t, net.Alice,
-	if !strings.Contains(resp.PaymentError, htlcAmt.String()) {
+		lnrpc.Failure_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS,
-		t.Fatalf("error didn't include expected payment amt of %v: "+
+	)
-			"%v", htlcAmt, resp.PaymentError)
-	}
 
 	// The balances of all parties should be the same as initially since
 	// the HTLC was canceled.
@@ -9574,12 +9601,12 @@ out:
 	if resp.PaymentError == "" {
 		t.Fatalf("payment should fail due to insufficient "+
 			"capacity: %v", err)
-	} else if !strings.Contains(resp.PaymentError,
-		lnwire.CodeTemporaryChannelFailure.String()) {
-		t.Fatalf("payment should fail due to insufficient capacity, "+
-			"instead: %v", resp.PaymentError)
 	}
 
+	assertLastHTLCError(
+		t, net.Alice, lnrpc.Failure_TEMPORARY_CHANNEL_FAILURE,
+	)
+
 	// Generate new invoice to not pay same invoice twice.
 	ctxt, _ = context.WithTimeout(ctxb, defaultTimeout)
 	carolInvoice, err = carol.AddInvoice(ctxt, invoiceReq)
@@ -9616,11 +9643,8 @@ out:
 	if resp.PaymentError == "" {
 		t.Fatalf("payment should have failed")
 	}
-	expectedErrorCode = lnwire.CodeUnknownNextPeer.String()
+
-	if !strings.Contains(resp.PaymentError, expectedErrorCode) {
+	assertLastHTLCError(t, net.Alice, lnrpc.Failure_UNKNOWN_NEXT_PEER)
-		t.Fatalf("payment should fail due to unknown hop, instead: %v",
-			resp.PaymentError)
-	}
 
 	// Finally, immediately close the channel. This function will also
 	// block until the channel is closed and will additionally assert the
@@ -9787,9 +9811,8 @@ func testRejectHTLC(net *lntest.NetworkHarness, t *harnessTest) {
 			"should have been rejected, carol will not accept forwarded htlcs",
 		)
 	}
-	if !strings.Contains(err.Error(), lnwire.CodeChannelDisabled.String()) {
+
-		t.Fatalf("error returned should have been Channel Disabled")
+	assertLastHTLCError(t, net.Alice, lnrpc.Failure_CHANNEL_DISABLED)
-	}
 
 	// Close all channels.
 	ctxt, _ = context.WithTimeout(ctxb, channelCloseTimeout)

routing/errors.go

@@ -15,10 +15,6 @@ const (
 	// this update can't bring us something new, or because a node
 	// announcement was given for node not found in any channel.
 	ErrIgnored
-
-	// ErrPaymentAttemptTimeout is an error that indicates that a payment
-	// attempt timed out before we were able to successfully route an HTLC.
-	ErrPaymentAttemptTimeout
 )
 
 // routerError is a structure that represent the error inside the routing package,

routing/payment_lifecycle.go

@@ -1,7 +1,6 @@
 package routing
 
 import (
-	"fmt"
 	"time"
 
 	"github.com/davecgh/go-spew/spew"
@@ -95,6 +94,12 @@ func (p *paymentLifecycle) resumePayment() ([32]byte, *route.Route, error) {
 			return settle.Settle.Preimage, &settle.Route, nil
 		}
 
+		// If the payment already is failed, and there is no in-flight
+		// HTLC, return immediately.
+		if attempt == nil && payment.FailureReason != nil {
+			return [32]byte{}, nil, *payment.FailureReason
+		}
+
 		// If this payment had no existing payment attempt, we create
 		// and send one now.
 		if attempt == nil {
@@ -113,10 +118,7 @@ func (p *paymentLifecycle) resumePayment() ([32]byte, *route.Route, error) {
 					return [32]byte{}, nil, err
 				}
 
-				errStr := fmt.Sprintf("payment attempt not completed " +
+				continue
-					"before timeout")
-
-				return [32]byte{}, nil, newErr(ErrPaymentAttemptTimeout, errStr)
 
 			// The payment will be resumed from the current state
 			// after restart.
@@ -149,8 +151,7 @@ func (p *paymentLifecycle) resumePayment() ([32]byte, *route.Route, error) {
 					return [32]byte{}, nil, saveErr
 				}
 
-				// Terminal state, return.
+				continue
-				return [32]byte{}, nil, err
 			}
 
 			// With the route in hand, launch a new shard.
@@ -511,7 +512,9 @@ func (p *shardHandler) sendPaymentAttempt(
 }
 
 // handleSendError inspects the given error from the Switch and determines
-// whether we should make another payment attempt.
+// whether we should make another payment attempt, or if it should be
+// considered a terminal error. Terminal errors will be recorded with the
+// control tower.
 func (p *shardHandler) handleSendError(attempt *channeldb.HTLCAttemptInfo,
 	sendErr error) error {
 
@@ -525,19 +528,12 @@ func (p *shardHandler) handleSendError(attempt *channeldb.HTLCAttemptInfo,
 	log.Debugf("Payment %x failed: final_outcome=%v, raw_err=%v",
 		p.paymentHash, *reason, sendErr)
 
-	// Mark the payment failed with no route.
+	err := p.router.cfg.Control.Fail(p.paymentHash, *reason)
-	//
-	// TODO(halseth): make payment codes for the actual reason we don't
-	// continue path finding.
-	err := p.router.cfg.Control.Fail(
-		p.paymentHash, *reason,
-	)
 	if err != nil {
 		return err
 	}
 
-	// Terminal state, return the error we encountered.
+	return nil
-	return sendErr
 }
 
 // failAttempt calls control tower to fail the current payment attempt.

routing/router_test.go

@@ -7,7 +7,6 @@ import (
 	"image/color"
 	"math"
 	"math/rand"
-	"strings"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -792,9 +791,8 @@ func TestSendPaymentErrorPathPruning(t *testing.T) {
 
 	// The final error returned should also indicate that the peer wasn't
 	// online (the last error we returned).
-	// TODO: proper err code
+	if err != channeldb.FailureReasonNoRoute {
-	if !strings.Contains(err.Error(), "unable to find") {
+		t.Fatalf("expected no route instead got: %v", err)
-		t.Fatalf("expected UnknownNextPeer instead got: %v", err)
 	}
 
 	// Inspect the two attempts that were made before the payment failed.