diff --git a/docs/release-notes/release-notes-0.19.0.md b/docs/release-notes/release-notes-0.19.0.md index 45a880253..f4d055d2e 100644 --- a/docs/release-notes/release-notes-0.19.0.md +++ b/docs/release-notes/release-notes-0.19.0.md @@ -94,6 +94,11 @@ * [Pass through](https://github.com/lightningnetwork/lnd/pull/9601) the unused `MaxPeers` configuration variable for neutrino mode. + +* [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/9543) where + the payment might have been failed more than once and therefore overwriting + the failure reason and notifying the `SubscribeAllPayments` subscribers more + than once. * [Fixed](https://github.com/lightningnetwork/lnd/pull/9609) a bug that may cause `listunspent` to give inaccurate wallet UTXOs. diff --git a/itest/list_on_test.go b/itest/list_on_test.go index 07f2da997..795e0e3f9 100644 --- a/itest/list_on_test.go +++ b/itest/list_on_test.go @@ -378,6 +378,10 @@ var allTestCases = []*lntest.TestCase{ Name: "sendtoroute multi path payment", TestFunc: testSendToRouteMultiPath, }, + { + Name: "send to route fail payment notification", + TestFunc: testSendToRouteFailPaymentNotification, + }, { Name: "send multi path payment", TestFunc: testSendMultiPathPayment, diff --git a/itest/lnd_trackpayments_test.go b/itest/lnd_trackpayments_test.go index b3a2eedb1..dc8655a26 100644 --- a/itest/lnd_trackpayments_test.go +++ b/itest/lnd_trackpayments_test.go @@ -2,8 +2,10 @@ package itest import ( "encoding/hex" + "time" "github.com/btcsuite/btcd/btcutil" + "github.com/lightningnetwork/lnd/chainreg" "github.com/lightningnetwork/lnd/lnrpc" "github.com/lightningnetwork/lnd/lnrpc/routerrpc" "github.com/lightningnetwork/lnd/lntest" @@ -148,3 +150,96 @@ func testTrackPaymentsCompatible(ht *lntest.HarnessTest) { require.NoError(ht, err, "unable to get payment update") require.Equal(ht, lnrpc.Payment_SUCCEEDED, payment3.Status) } + +// testSendToRouteFailPaymentNotification tests that when we are failing a +// payment via SendToRouteV2 we only receive one failure notification, also +// making sure the failure reason is not overwritten in our db. +func testSendToRouteFailPaymentNotification(ht *lntest.HarnessTest) { + // Create a simple network with Alice->Bob. + _, nodes := ht.CreateSimpleNetwork( + [][]string{nil, nil}, + lntest.OpenChannelParams{Amt: 100_000}, + ) + alice, bob := nodes[0], nodes[1] + + // Make Bob create an invoice for Alice to pay. + _, rHashes, _ := ht.CreatePayReqs(bob, paymentAmt, 1) + + rHash := rHashes[0] + + // We create a dummy payment address so that the payment fails at + // the first hop. + payAddr := make([]byte, 32) + + // Subscribe to all the payments because otherwise we would not be able + // to verify if two failure notifications are received. The single + // subscriber is deleted after receiving the first failure notification. + tracker := alice.RPC.TrackPayments(&routerrpc.TrackPaymentsRequest{ + NoInflightUpdates: true, + }) + + bobPubKey, err := hex.DecodeString(bob.PubKeyStr) + require.NoError(ht, err, "unable to decode bob's pubkey") + + // Build a route for the specified hops. + r := alice.RPC.BuildRoute(&routerrpc.BuildRouteRequest{ + AmtMsat: int64(paymentAmt * 1000), + FinalCltvDelta: chainreg.DefaultBitcoinTimeLockDelta, + HopPubkeys: [][]byte{bobPubKey}, + }).Route + + // Set the MPP records to indicate this is a payment shard. + hop := r.Hops[len(r.Hops)-1] + hop.MppRecord = &lnrpc.MPPRecord{ + PaymentAddr: payAddr, + TotalAmtMsat: int64(paymentAmt * 1000), + } + + // Send the only shard. + sendReq := &routerrpc.SendToRouteRequest{ + PaymentHash: rHash, + Route: r, + } + + // We launch the payment and check the payment stream to verify that + // we are receiving exactly one failure notification. + respChan := make(chan *lnrpc.HTLCAttempt, 1) + go func() { + attempt := alice.RPC.SendToRouteV2(sendReq) + respChan <- attempt + }() + + // We excpect the attempt and the payment to fail. + select { + case attempt := <-respChan: + require.Equal(ht, attempt.Status, lnrpc.HTLCAttempt_FAILED) + + case <-time.After(defaultTimeout): + ht.Fatal("timeout waiting for SendToRouteV2 response") + } + + // Assert the first track payment update should be a failed update. + update, err := tracker.Recv() + require.NoError(ht, err, "unable to receive payment update") + require.Equal(ht, lnrpc.Payment_FAILED, update.Status) + + // Now check no other notifications come in. + notifChan := make(chan *lnrpc.Payment, 1) + go func() { + notif, err := tracker.Recv() + if err != nil { + return + } + + notifChan <- notif + }() + + // We do not expect any other notifications. + select { + case unexpectedNotif := <-notifChan: + ht.Fatalf("received unexpected notification: %v", + unexpectedNotif) + + case <-time.After(100 * time.Millisecond): + } +} diff --git a/routing/control_tower.go b/routing/control_tower.go index 8968ca312..174ff6a7f 100644 --- a/routing/control_tower.go +++ b/routing/control_tower.go @@ -282,6 +282,9 @@ func (p *controlTower) FetchPayment(paymentHash lntypes.Hash) ( // reason the payment failed. After invoking this method, InitPayment should // return nil on its next call for this payment hash, allowing the switch to // make a subsequent payment. +// +// NOTE: This method will overwrite the failure reason if the payment is already +// failed. func (p *controlTower) FailPayment(paymentHash lntypes.Hash, reason channeldb.FailureReason) error { diff --git a/routing/router.go b/routing/router.go index 323fe7616..3fa259ed6 100644 --- a/routing/router.go +++ b/routing/router.go @@ -1048,6 +1048,29 @@ func (r *ChannelRouter) sendToRoute(htlcHash lntypes.Hash, rt *route.Route, firstHopCustomRecords lnwire.CustomRecords) (*channeldb.HTLCAttempt, error) { + // Helper function to fail a payment. It makes sure the payment is only + // failed once so that the failure reason is not overwritten. + failPayment := func(paymentIdentifier lntypes.Hash, + reason channeldb.FailureReason) error { + + payment, fetchErr := r.cfg.Control.FetchPayment( + paymentIdentifier, + ) + if fetchErr != nil { + return fetchErr + } + + // NOTE: We cannot rely on the payment status to be failed here + // because it can still be in-flight although the payment is + // already failed. + _, failedReason := payment.TerminalInfo() + if failedReason != nil { + return nil + } + + return r.cfg.Control.FailPayment(paymentIdentifier, reason) + } + log.Debugf("SendToRoute for payment %v with skipTempErr=%v", htlcHash, skipTempErr) @@ -1148,20 +1171,6 @@ func (r *ChannelRouter) sendToRoute(htlcHash lntypes.Hash, rt *route.Route, return nil, err } - // We now look up the payment to see if it's already failed. - payment, err := p.router.cfg.Control.FetchPayment(p.identifier) - if err != nil { - return result.attempt, err - } - - // Exit if the above error has caused the payment to be failed, we also - // return the error from sending attempt to mimic the old behavior of - // this method. - _, failedReason := payment.TerminalInfo() - if failedReason != nil { - return result.attempt, result.err - } - // Since for SendToRoute we won't retry in case the shard fails, we'll // mark the payment failed with the control tower immediately if the // skipTempErr is false. @@ -1175,8 +1184,7 @@ func (r *ChannelRouter) sendToRoute(htlcHash lntypes.Hash, rt *route.Route, return result.attempt, result.err } - // Otherwise we need to fail the payment. - err := r.cfg.Control.FailPayment(paymentIdentifier, reason) + err := failPayment(paymentIdentifier, reason) if err != nil { return nil, err } @@ -1199,7 +1207,7 @@ func (r *ChannelRouter) sendToRoute(htlcHash lntypes.Hash, rt *route.Route, // An error returned from collecting the result, we'll mark the payment // as failed if we don't skip temp error. if !skipTempErr { - err := r.cfg.Control.FailPayment(paymentIdentifier, reason) + err := failPayment(paymentIdentifier, reason) if err != nil { return nil, err } diff --git a/routing/router_test.go b/routing/router_test.go index 6efc75df2..62a68b213 100644 --- a/routing/router_test.go +++ b/routing/router_test.go @@ -2215,13 +2215,6 @@ func TestSendToRouteSkipTempErrSuccess(t *testing.T) { mock.Anything, rt, ).Return(nil) - // Mock the control tower to return the mocked payment. - payment := &mockMPPayment{} - controlTower.On("FetchPayment", payHash).Return(payment, nil).Once() - - // Mock the payment to return nil failure reason. - payment.On("TerminalInfo").Return(nil, nil) - // Expect a successful send to route. attempt, err := router.SendToRouteSkipTempErr(payHash, rt, nil) require.NoError(t, err) @@ -2231,7 +2224,6 @@ func TestSendToRouteSkipTempErrSuccess(t *testing.T) { controlTower.AssertExpectations(t) payer.AssertExpectations(t) missionControl.AssertExpectations(t) - payment.AssertExpectations(t) } // TestSendToRouteSkipTempErrNonMPP checks that an error is return when @@ -2352,19 +2344,12 @@ func TestSendToRouteSkipTempErrTempFailure(t *testing.T) { mock.Anything, mock.Anything, mock.Anything, ).Return(tempErr) - // Mock the control tower to return the mocked payment. - payment := &mockMPPayment{} - controlTower.On("FetchPayment", payHash).Return(payment, nil).Once() - // Mock the mission control to return a nil reason from reporting the // attempt failure. missionControl.On("ReportPaymentFail", mock.Anything, rt, mock.Anything, mock.Anything, ).Return(nil, nil) - // Mock the payment to return nil failure reason. - payment.On("TerminalInfo").Return(nil, nil) - // Expect a failed send to route. attempt, err := router.SendToRouteSkipTempErr(payHash, rt, nil) require.Equal(t, tempErr, err) @@ -2374,7 +2359,6 @@ func TestSendToRouteSkipTempErrTempFailure(t *testing.T) { controlTower.AssertExpectations(t) payer.AssertExpectations(t) missionControl.AssertExpectations(t) - payment.AssertExpectations(t) } // TestSendToRouteSkipTempErrPermanentFailure validates a permanent failure @@ -2436,7 +2420,9 @@ func TestSendToRouteSkipTempErrPermanentFailure(t *testing.T) { ).Return(testAttempt, nil) // Expect the payment to be failed. - controlTower.On("FailPayment", payHash, mock.Anything).Return(nil) + controlTower.On( + "FailPayment", payHash, mock.Anything, + ).Return(nil).Once() // Mock an error to be returned from sending the htlc. payer.On("SendHTLC", @@ -2448,13 +2434,6 @@ func TestSendToRouteSkipTempErrPermanentFailure(t *testing.T) { mock.Anything, rt, mock.Anything, mock.Anything, ).Return(&failureReason, nil) - // Mock the control tower to return the mocked payment. - payment := &mockMPPayment{} - controlTower.On("FetchPayment", payHash).Return(payment, nil).Once() - - // Mock the payment to return a failure reason. - payment.On("TerminalInfo").Return(nil, &failureReason) - // Expect a failed send to route. attempt, err := router.SendToRouteSkipTempErr(payHash, rt, nil) require.Equal(t, permErr, err) @@ -2464,7 +2443,6 @@ func TestSendToRouteSkipTempErrPermanentFailure(t *testing.T) { controlTower.AssertExpectations(t) payer.AssertExpectations(t) missionControl.AssertExpectations(t) - payment.AssertExpectations(t) } // TestSendToRouteTempFailure validates a temporary failure will cause the @@ -2525,7 +2503,9 @@ func TestSendToRouteTempFailure(t *testing.T) { ).Return(testAttempt, nil) // Expect the payment to be failed. - controlTower.On("FailPayment", payHash, mock.Anything).Return(nil) + controlTower.On( + "FailPayment", payHash, mock.Anything, + ).Return(nil).Once() payer.On("SendHTLC", mock.Anything, mock.Anything, mock.Anything, @@ -2536,7 +2516,7 @@ func TestSendToRouteTempFailure(t *testing.T) { controlTower.On("FetchPayment", payHash).Return(payment, nil).Once() // Mock the payment to return nil failure reason. - payment.On("TerminalInfo").Return(nil, nil) + payment.On("TerminalInfo").Return(nil, nil).Once() // Return a nil reason to mock a temporary failure. missionControl.On("ReportPaymentFail",