From e3dadd528b6267093c83facc5a5bf4a641b7e5c9 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Sun, 10 Sep 2023 09:22:30 +0800 Subject: [PATCH] routing: mark payment as failed when no route can be found --- routing/payment_lifecycle.go | 51 ++++++++++------------ routing/payment_lifecycle_test.go | 71 +++++-------------------------- 2 files changed, 32 insertions(+), 90 deletions(-) diff --git a/routing/payment_lifecycle.go b/routing/payment_lifecycle.go index 20c9c9cde..fdaa68042 100644 --- a/routing/payment_lifecycle.go +++ b/routing/payment_lifecycle.go @@ -266,9 +266,14 @@ lifecycle: return exitWithErr(err) } - // NOTE: might cause an infinite loop, see notes in - // `requestRoute` for details. + // We may not be able to find a route for current attempt. In + // that case, we continue the loop and move straight to the + // next iteration in case there are results for inflight HTLCs + // that still need to be collected. if rt == nil { + log.Errorf("No route found for payment %v", + p.identifier) + continue lifecycle } @@ -363,42 +368,30 @@ func (p *paymentLifecycle) requestRoute( log.Warnf("Failed to find route for payment %v: %v", p.identifier, err) // If the error belongs to `noRouteError` set, it means a non-critical - // error has happened during path finding and we might be able to find - // another route during next HTLC attempt. Otherwise, we'll return the - // critical error found. + // error has happened during path finding and we will mark the payment + // failed with this reason. Otherwise, we'll return the critical error + // found to abort the lifecycle. var routeErr noRouteError if !errors.As(err, &routeErr) { return nil, fmt.Errorf("requestRoute got: %w", err) } - // There is no route to try, and we have no active shards. This means - // that there is no way for us to send the payment, so mark it failed - // with no route. - // - // NOTE: if we have zero `numShardsInFlight`, it means all the HTLC - // attempts have failed. Otherwise, if there are still inflight - // attempts, we might enter an infinite loop in our lifecycle if - // there's still remaining amount since we will keep adding new HTLC - // attempts and they all fail with `noRouteError`. - // - // TODO(yy): further check the error returned here. It's the - // `paymentSession`'s responsibility to find a route for us with best - // effort. When it cannot find a path, we need to treat it as a - // terminal condition and fail the payment no matter it has inflight + // It's the `paymentSession`'s responsibility to find a route for us + // with best effort. When it cannot find a path, we need to treat it as + // a terminal condition and fail the payment no matter it has inflight // HTLCs or not. - if ps.NumAttemptsInFlight == 0 { - failureCode := routeErr.FailureReason() - log.Debugf("Marking payment %v permanently failed with no "+ - "route: %v", p.identifier, failureCode) + failureCode := routeErr.FailureReason() + log.Warnf("Marking payment %v permanently failed with no route: %v", + p.identifier, failureCode) - err := p.router.cfg.Control.FailPayment( - p.identifier, failureCode, - ) - if err != nil { - return nil, fmt.Errorf("FailPayment got: %w", err) - } + err = p.router.cfg.Control.FailPayment(p.identifier, failureCode) + if err != nil { + return nil, fmt.Errorf("FailPayment got: %w", err) } + // NOTE: we decide to not return the non-critical noRouteError here to + // avoid terminating the payment lifecycle as there might be other + // inflight HTLCs which we must wait for their results. return nil, nil } diff --git a/routing/payment_lifecycle_test.go b/routing/payment_lifecycle_test.go index f4b637ee4..f0a18165e 100644 --- a/routing/payment_lifecycle_test.go +++ b/routing/payment_lifecycle_test.go @@ -419,13 +419,8 @@ func TestRequestRouteHandleCriticalErr(t *testing.T) { func TestRequestRouteHandleNoRouteErr(t *testing.T) { t.Parallel() - p := createTestPaymentLifecycle() - - // Create a mock payment session. - paySession := &mockPaymentSession{} - - // Mount the mocked payment session. - p.paySession = paySession + // Create a paymentLifecycle with mockers. + p, m := newTestPaymentLifecycle(t) // Create a dummy payment state. ps := &channeldb.MPPaymentState{ @@ -437,68 +432,22 @@ func TestRequestRouteHandleNoRouteErr(t *testing.T) { // Mock remainingFees to be 1. p.feeLimit = ps.FeesPaid + 1 - // Mock the paySession's `RequestRoute` method to return an error. - paySession.On("RequestRoute", + // Mock the paySession's `RequestRoute` method to return a NoRouteErr + // type. + m.paySession.On("RequestRoute", mock.Anything, mock.Anything, mock.Anything, mock.Anything, ).Return(nil, errNoTlvPayload) + // The payment should be failed with reason no route. + m.control.On("FailPayment", + p.identifier, channeldb.FailureReasonNoRoute, + ).Return(nil).Once() + result, err := p.requestRoute(ps) // Expect no error is returned since it's not critical. require.NoError(t, err, "expected no error") require.Nil(t, result, "expected no route returned") - - // Assert that `RequestRoute` is called as expected. - paySession.AssertExpectations(t) -} - -// TestRequestRouteFailPaymentSucceed checks that `requestRoute` fails the -// payment when received an `noRouteError` returned from payment session while -// it has no inflight attempts. -func TestRequestRouteFailPaymentSucceed(t *testing.T) { - t.Parallel() - - p := createTestPaymentLifecycle() - - // Create a mock payment session. - paySession := &mockPaymentSession{} - - // Mock the control tower's `FailPayment` method. - ct := &mockControlTower{} - ct.On("FailPayment", - p.identifier, errNoTlvPayload.FailureReason(), - ).Return(nil) - - // Mount the mocked control tower and payment session. - p.router.cfg.Control = ct - p.paySession = paySession - - // Create a dummy payment state with zero inflight attempts. - ps := &channeldb.MPPaymentState{ - NumAttemptsInFlight: 0, - RemainingAmt: 1, - FeesPaid: 100, - } - - // Mock remainingFees to be 1. - p.feeLimit = ps.FeesPaid + 1 - - // Mock the paySession's `RequestRoute` method to return an error. - paySession.On("RequestRoute", - mock.Anything, mock.Anything, mock.Anything, mock.Anything, - ).Return(nil, errNoTlvPayload) - - result, err := p.requestRoute(ps) - - // Expect no error is returned since it's not critical. - require.NoError(t, err, "expected no error") - require.Nil(t, result, "expected no route returned") - - // Assert that `RequestRoute` is called as expected. - paySession.AssertExpectations(t) - - // Assert that `FailPayment` is called as expected. - ct.AssertExpectations(t) } // TestRequestRouteFailPaymentError checks that `requestRoute` returns the