diff --git a/lntest/itest/lnd_test.go b/lntest/itest/lnd_test.go index 1a113399..70febd9f 100644 --- a/lntest/itest/lnd_test.go +++ b/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 !strings.Contains(resp.PaymentError, replayErr) { - t.Fatalf("received payment error: %v, expected %v", - resp.PaymentError, replayErr) + if resp.PaymentError == "" { + t.Fatalf("expected payment error") } + 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) { - // TODO(roasbeef): make into proper gRPC error code - t.Fatalf("payment should have failed due to unknown payment hash, "+ - "instead failed due to: %v", resp.PaymentError) - } + + assertLastHTLCError( + t, net.Alice, + lnrpc.Failure_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS, + ) // 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 - // amount. - if !strings.Contains(resp.PaymentError, htlcAmt.String()) { - t.Fatalf("error didn't include expected payment amt of %v: "+ - "%v", htlcAmt, resp.PaymentError) - } + assertLastHTLCError( + t, net.Alice, + lnrpc.Failure_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS, + ) // 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) { - t.Fatalf("payment should fail due to unknown hop, instead: %v", - resp.PaymentError) - } + + assertLastHTLCError(t, net.Alice, lnrpc.Failure_UNKNOWN_NEXT_PEER) // 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) diff --git a/routing/errors.go b/routing/errors.go index 3166f739..3beac229 100644 --- a/routing/errors.go +++ b/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, diff --git a/routing/payment_lifecycle.go b/routing/payment_lifecycle.go index dacd4fc4..6e98eb1b 100644 --- a/routing/payment_lifecycle.go +++ b/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 " + - "before timeout") - - return [32]byte{}, nil, newErr(ErrPaymentAttemptTimeout, errStr) + continue // 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. - return [32]byte{}, nil, err + continue } // 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. - // - // TODO(halseth): make payment codes for the actual reason we don't - // continue path finding. - err := p.router.cfg.Control.Fail( - p.paymentHash, *reason, - ) + err := p.router.cfg.Control.Fail(p.paymentHash, *reason) if err != nil { return err } - // Terminal state, return the error we encountered. - return sendErr + return nil } // failAttempt calls control tower to fail the current payment attempt. diff --git a/routing/router_test.go b/routing/router_test.go index d3c866ae..08499c5a 100644 --- a/routing/router_test.go +++ b/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 !strings.Contains(err.Error(), "unable to find") { - t.Fatalf("expected UnknownNextPeer instead got: %v", err) + if err != channeldb.FailureReasonNoRoute { + t.Fatalf("expected no route instead got: %v", err) } // Inspect the two attempts that were made before the payment failed.