From b24edb28330d8e6e49a166b68a2ac520707a17d0 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 6 Jul 2021 18:35:20 -0700 Subject: [PATCH 1/3] routing: modify TestSendToRouteStructuredError to return non-second chance error In this commit, we modify the existing `TestSendToRouteStructuredError` test to return an error that doesn't trigger the second chance logic. Otherwise, we'll get a nil failure result from the mission control interpretation, meaning we won't exercise the full code path. Instead, we use a terminal error to ensure that the expected code path is hit. As is, this test will fail as a recent refactoring causes us to return a `channeldb.FailureReason` error, since the newly added `handleSendError` code path in the `SendToRoute` method will return the raw error, rather than the `shardError`, which is of the expected type. --- routing/router_test.go | 92 +++++++++++++++++++++++++----------------- 1 file changed, 56 insertions(+), 36 deletions(-) diff --git a/routing/router_test.go b/routing/router_test.go index 29ad342d..558a54c8 100644 --- a/routing/router_test.go +++ b/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 - // payment with an error originating from the first hop of the route. - // The unsigned channel update is attached to the failure message. - ctx.router.cfg.Payer.(*mockPaymentAttemptDispatcherOld).setPaymentResult( - func(firstHop lnwire.ShortChannelID) ([32]byte, error) { - return [32]byte{}, htlcswitch.NewForwardingError( - &lnwire.FailFeeInsufficient{ - Update: lnwire.ChannelUpdate{}, - }, 1, + finalHopIndex := len(hops) + testCases := map[int]lnwire.FailureMessage{ + finalHopIndex: lnwire.NewFailIncorrectDetails(payAmt, 100), + 1: &lnwire.FailFeeInsufficient{ + Update: lnwire.ChannelUpdate{}, + }, + } + + 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") } } From 525ef594c79973c35cbc965aa062399c9bc59566 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 6 Jul 2021 18:41:22 -0700 Subject: [PATCH 2/3] 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 } From 079ab1c2bf2ba69b871380af0b9ea01397cb6e1a Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 6 Jul 2021 18:41:38 -0700 Subject: [PATCH 3/3] channeldb: fix typo in PaymentControl.Fail --- channeldb/payment_control.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/channeldb/payment_control.go b/channeldb/payment_control.go index 0bbb344b..c11bc5c9 100644 --- a/channeldb/payment_control.go +++ b/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.