Merge pull request #9543 from ziggie1984/fix-payment-inconsitency

multi: fix payment failure overwrite

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.

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,

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):
+	}
+}

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 {
 

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
 		}

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",