Merge pull request #5479 from Roasbeef/fix-send-to-route-error-propagation

lnrpc/routerrrpc+routing: fix send to route error propagation

channeldb/payment_control.go

@@ -503,7 +503,7 @@ func (p *PaymentControl) Fail(paymentHash lntypes.Hash,
 			return err
 		}
 
-		// We mark the payent as failed as long as it is known. This
+		// We mark the payment as failed as long as it is known. This
 		// lets the last attempt to fail with a terminal write its
 		// failure to the PaymentControl without synchronizing with
 		// other attempts.

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

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 {
+	var failureReason *channeldb.FailureReason
-		return nil, err
+	err = sh.handleSendError(attempt, shardError)
-	}
 
-	err = r.cfg.Control.Fail(
+	switch {
-		paymentIdentifier, channeldb.FailureReasonNoRoute,
+	// 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
 	}

routing/router_test.go

@@ -2921,44 +2921,64 @@ func TestSendToRouteStructuredError(t *testing.T) {
 		t.Fatalf("unable to create route: %v", err)
 	}
 
-	// We'll modify the SendToSwitch method so that it simulates a failed
+	finalHopIndex := len(hops)
-	// payment with an error originating from the first hop of the route.
+	testCases := map[int]lnwire.FailureMessage{
-	// The unsigned channel update is attached to the failure message.
+		finalHopIndex: lnwire.NewFailIncorrectDetails(payAmt, 100),
-	ctx.router.cfg.Payer.(*mockPaymentAttemptDispatcherOld).setPaymentResult(
+		1: &lnwire.FailFeeInsufficient{
-		func(firstHop lnwire.ShortChannelID) ([32]byte, error) {
+			Update: lnwire.ChannelUpdate{},
-			return [32]byte{}, htlcswitch.NewForwardingError(
+		},
-				&lnwire.FailFeeInsufficient{
+	}
-					Update: lnwire.ChannelUpdate{},
+
-				}, 1,
+	for failIndex, errorType := range testCases {
+		failIndex := failIndex
+		errorType := errorType
+
+		t.Run(fmt.Sprintf("%T", errorType), func(t *testing.T) {
+			// We'll modify the SendToSwitch method so that it
+			// simulates a failed payment with an error originating
+			// from the final hop in the route.
+			ctx.router.cfg.Payer.(*mockPaymentAttemptDispatcherOld).setPaymentResult(
+				func(firstHop lnwire.ShortChannelID) ([32]byte, error) {
+					return [32]byte{}, htlcswitch.NewForwardingError(
+						errorType, failIndex,
+					)
+				},
 			)
+
+			// The payment parameter is mostly redundant in
+			// SendToRoute.  Can be left empty for this test.
+			var payment lntypes.Hash
+
+			// Send off the payment request to the router. The
+			// specified route should be attempted and the channel
+			// update should be received by router and ignored
+			// because it is missing a valid
+			// signature.
+			_, err = ctx.router.SendToRoute(payment, rt)
+
+			fErr, ok := err.(*htlcswitch.ForwardingError)
+			require.True(
+				t, ok, "expected forwarding error, got: %T", err,
+			)
+
+			require.IsType(
+				t, errorType, fErr.WireMessage(),
+				"expected type %T got %T", errorType,
+				fErr.WireMessage(),
+			)
+
+			// Check that the correct values were used when
+			// initiating the payment.
+			select {
+			case initVal := <-init:
+				if initVal.c.Value != payAmt {
+					t.Fatalf("expected %v, got %v", payAmt,
+						initVal.c.Value)
+				}
+			case <-time.After(100 * time.Millisecond):
+				t.Fatalf("initPayment not called")
+			}
 		})
-
-	// The payment parameter is mostly redundant in SendToRoute. Can be left
-	// empty for this test.
-	var payment lntypes.Hash
-
-	// Send off the payment request to the router. The specified route
-	// should be attempted and the channel update should be received by
-	// router and ignored because it is missing a valid signature.
-	_, err = ctx.router.SendToRoute(payment, rt)
-
-	fErr, ok := err.(*htlcswitch.ForwardingError)
-	if !ok {
-		t.Fatalf("expected forwarding error")
-	}
-
-	if _, ok := fErr.WireMessage().(*lnwire.FailFeeInsufficient); !ok {
-		t.Fatalf("expected fee insufficient error")
-	}
-
-	// Check that the correct values were used when initiating the payment.
-	select {
-	case initVal := <-init:
-		if initVal.c.Value != payAmt {
-			t.Fatalf("expected %v, got %v", payAmt, initVal.c.Value)
-		}
-	case <-time.After(100 * time.Millisecond):
-		t.Fatalf("initPayment not called")
 	}
 }