Merge pull request #7503 from yyforyongyu/new-payment-lifecycle

channeldb+routing: decouple payment lifecycle flow and fix stuck payments

channeldb/mp_payment.go

@@ -219,10 +219,10 @@ func (m *MPPayment) Terminated() bool {
 // TerminalInfo returns any HTLC settle info recorded. If no settle info is
 // recorded, any payment level failure will be returned. If neither a settle
 // nor a failure is recorded, both return values will be nil.
-func (m *MPPayment) TerminalInfo() (*HTLCSettleInfo, *FailureReason) {
+func (m *MPPayment) TerminalInfo() (*HTLCAttempt, *FailureReason) {
 	for _, h := range m.HTLCs {
 		if h.Settle != nil {
-			return h.Settle, nil
+			return &h, nil
 		}
 	}
 
@@ -264,6 +264,7 @@ func (m *MPPayment) InFlightHTLCs() []HTLCAttempt {
 
 // GetAttempt returns the specified htlc attempt on the payment.
 func (m *MPPayment) GetAttempt(id uint64) (*HTLCAttempt, error) {
+	// TODO(yy): iteration can be slow, make it into a tree or use BS.
 	for _, htlc := range m.HTLCs {
 		htlc := htlc
 		if htlc.AttemptID == id {
@@ -463,9 +464,49 @@ func (m *MPPayment) GetHTLCs() []HTLCAttempt {
 	return m.HTLCs
 }
 
-// GetFailureReason returns the failure reason.
-func (m *MPPayment) GetFailureReason() *FailureReason {
-	return m.FailureReason
+// AllowMoreAttempts is used to decide whether we can safely attempt more HTLCs
+// for a given payment state. Return an error if the payment is in an
+// unexpected state.
+func (m *MPPayment) AllowMoreAttempts() (bool, error) {
+	// Now check whether the remainingAmt is zero or not. If we don't have
+	// any remainingAmt, no more HTLCs should be made.
+	if m.State.RemainingAmt == 0 {
+		// If the payment is newly created, yet we don't have any
+		// remainingAmt, return an error.
+		if m.Status == StatusInitiated {
+			return false, fmt.Errorf("%w: initiated payment has "+
+				"zero remainingAmt", ErrPaymentInternal)
+		}
+
+		// Otherwise, exit early since all other statuses with zero
+		// remainingAmt indicate no more HTLCs can be made.
+		return false, nil
+	}
+
+	// Otherwise, the remaining amount is not zero, we now decide whether
+	// to make more attempts based on the payment's current status.
+	//
+	// If at least one of the payment's attempts is settled, yet we haven't
+	// sent all the amount, it indicates something is wrong with the peer
+	// as the preimage is received. In this case, return an error state.
+	if m.Status == StatusSucceeded {
+		return false, fmt.Errorf("%w: payment already succeeded but "+
+			"still have remaining amount %v", ErrPaymentInternal,
+			m.State.RemainingAmt)
+	}
+
+	// Now check if we can register a new HTLC.
+	err := m.Registrable()
+	if err != nil {
+		log.Warnf("Payment(%v): cannot register HTLC attempt: %v, "+
+			"current status: %s", m.Info.PaymentIdentifier,
+			err, m.Status)
+
+		return false, nil
+	}
+
+	// Now we know we can register new HTLCs.
+	return true, nil
 }
 
 // serializeHTLCSettleInfo serializes the details of a settled htlc.

channeldb/mp_payment_test.go

@@ -368,6 +368,183 @@ func TestNeedWaitAttempts(t *testing.T) {
 	}
 }
 
+// TestAllowMoreAttempts checks whether more attempts can be created against
+// ALL possible payment statuses.
+func TestAllowMoreAttempts(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		status         PaymentStatus
+		remainingAmt   lnwire.MilliSatoshi
+		hasSettledHTLC bool
+		paymentFailed  bool
+		allowMore      bool
+		expectedErr    error
+	}{
+		{
+			// A newly created payment with zero remainingAmt
+			// indicates an error.
+			status:       StatusInitiated,
+			remainingAmt: 0,
+			allowMore:    false,
+			expectedErr:  ErrPaymentInternal,
+		},
+		{
+			// With zero remainingAmt we don't allow more HTLC
+			// attempts.
+			status:       StatusInFlight,
+			remainingAmt: 0,
+			allowMore:    false,
+			expectedErr:  nil,
+		},
+		{
+			// With zero remainingAmt we don't allow more HTLC
+			// attempts.
+			status:       StatusSucceeded,
+			remainingAmt: 0,
+			allowMore:    false,
+			expectedErr:  nil,
+		},
+		{
+			// With zero remainingAmt we don't allow more HTLC
+			// attempts.
+			status:       StatusFailed,
+			remainingAmt: 0,
+			allowMore:    false,
+			expectedErr:  nil,
+		},
+		{
+			// With zero remainingAmt and settled HTLCs we don't
+			// allow more HTLC attempts.
+			status:         StatusInFlight,
+			remainingAmt:   0,
+			hasSettledHTLC: true,
+			allowMore:      false,
+			expectedErr:    nil,
+		},
+		{
+			// With zero remainingAmt and failed payment we don't
+			// allow more HTLC attempts.
+			status:        StatusInFlight,
+			remainingAmt:  0,
+			paymentFailed: true,
+			allowMore:     false,
+			expectedErr:   nil,
+		},
+		{
+			// With zero remainingAmt and both settled HTLCs and
+			// failed payment, we don't allow more HTLC attempts.
+			status:         StatusInFlight,
+			remainingAmt:   0,
+			hasSettledHTLC: true,
+			paymentFailed:  true,
+			allowMore:      false,
+			expectedErr:    nil,
+		},
+		{
+			// A newly created payment can have more attempts.
+			status:       StatusInitiated,
+			remainingAmt: 1000,
+			allowMore:    true,
+			expectedErr:  nil,
+		},
+		{
+			// With HTLCs inflight we can have more attempts when
+			// the remainingAmt is not zero and we have neither
+			// failed payment or settled HTLCs.
+			status:       StatusInFlight,
+			remainingAmt: 1000,
+			allowMore:    true,
+			expectedErr:  nil,
+		},
+		{
+			// With HTLCs inflight we cannot have more attempts
+			// though the remainingAmt is not zero but we have
+			// settled HTLCs.
+			status:         StatusInFlight,
+			remainingAmt:   1000,
+			hasSettledHTLC: true,
+			allowMore:      false,
+			expectedErr:    nil,
+		},
+		{
+			// With HTLCs inflight we cannot have more attempts
+			// though the remainingAmt is not zero but we have
+			// failed payment.
+			status:        StatusInFlight,
+			remainingAmt:  1000,
+			paymentFailed: true,
+			allowMore:     false,
+			expectedErr:   nil,
+		},
+		{
+			// With HTLCs inflight we cannot have more attempts
+			// though the remainingAmt is not zero but we have
+			// settled HTLCs and failed payment.
+			status:         StatusInFlight,
+			remainingAmt:   1000,
+			hasSettledHTLC: true,
+			paymentFailed:  true,
+			allowMore:      false,
+			expectedErr:    nil,
+		},
+		{
+			// With the payment settled, but the remainingAmt is
+			// not zero, we have an error state.
+			status:         StatusSucceeded,
+			remainingAmt:   1000,
+			hasSettledHTLC: true,
+			allowMore:      false,
+			expectedErr:    ErrPaymentInternal,
+		},
+		{
+			// With the payment failed with no inflight HTLCs, we
+			// don't allow more attempts to be made.
+			status:        StatusFailed,
+			remainingAmt:  1000,
+			paymentFailed: true,
+			allowMore:     false,
+			expectedErr:   nil,
+		},
+		{
+			// With the payment in an unknown state, we don't allow
+			// more attempts to be made.
+			status:       0,
+			remainingAmt: 1000,
+			allowMore:    false,
+			expectedErr:  nil,
+		},
+	}
+
+	for i, tc := range testCases {
+		tc := tc
+
+		p := &MPPayment{
+			Info: &PaymentCreationInfo{
+				PaymentIdentifier: [32]byte{1, 2, 3},
+			},
+			Status: tc.status,
+			State: &MPPaymentState{
+				RemainingAmt:   tc.remainingAmt,
+				HasSettledHTLC: tc.hasSettledHTLC,
+				PaymentFailed:  tc.paymentFailed,
+			},
+		}
+
+		name := fmt.Sprintf("test_%d|status=%s|remainingAmt=%v", i,
+			tc.status, tc.remainingAmt)
+
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			result, err := p.AllowMoreAttempts()
+			require.ErrorIs(t, err, tc.expectedErr)
+			require.Equalf(t, tc.allowMore, result, "status=%v, "+
+				"remainingAmt=%v", tc.status, tc.remainingAmt)
+		})
+	}
+}
+
 func makeActiveAttempt(total, fee int) HTLCAttempt {
 	return HTLCAttempt{
 		HTLCAttemptInfo: makeAttemptInfo(total, total-fee),

docs/release-notes/release-notes-0.18.0.md

@@ -19,6 +19,16 @@
 
 # Bug Fixes
 
+* [Fixed a potential case](https://github.com/lightningnetwork/lnd/pull/7824)
+  that when sweeping inputs with locktime, an unexpected lower fee rate is
+  applied.
+
+* LND will now [enforce pong responses
+  ](https://github.com/lightningnetwork/lnd/pull/7828) from its peers
+
+* [Fixed a case](https://github.com/lightningnetwork/lnd/pull/7503) where it's
+  possible a failed payment might be stuck in pending.
+
 # New Features
 ## Functional Enhancements
 
@@ -38,15 +48,24 @@
   where the required flags are tagged with `(blinded paths)`.
 
 ## RPC Additions
+
+* [Deprecated](https://github.com/lightningnetwork/lnd/pull/7175)
+  `StatusUnknown` from the payment's rpc response in its status and added a new
+  status, `StatusInitiated`, to explicitly report its current state. Before
+  running this new version, please make sure to upgrade your client application
+  to include this new status so it can understand the RPC response properly.
+
 ## lncli Additions
 
 # Improvements
 ## Functional Updates
 ## RPC Updates
 
-* [Deprecated](https://github.com/lightningnetwork/lnd/pull/7175)
-  `StatusUnknown` from the payment's rpc response in its status and replaced it
-  with `StatusInitiated` to explicitly report its current state.
+* `sendtoroute` will return an error when it's called using the flag
+  `--skip_temp_err` on a payment that's not a MPP. This is needed as a temp
+  error is defined as a routing error found in one of a MPP's HTLC attempts.
+  If, however, there's only one HTLC attempt, when it's failed, this payment is
+  considered failed, thus there's no such thing as temp error for a non-MPP.
 
 ## lncli Updates
 ## Code Health
@@ -57,6 +76,11 @@
   `lnrpc.GetInfoResponse` message along with the `chain` field in the
   `lnrpc.Chain` message have also been deprecated for the same reason.
 
+* The payment lifecycle code has been refactored to improve its maintainablity.
+  In particular, the complexity involved in the lifecycle loop has been
+  decoupled into logical steps, with each step having its own responsibility,
+  making it easier to reason about the payment flow.
+
 ## Breaking Changes
 ## Performance Improvements
 

htlcswitch/switch.go

@@ -1000,6 +1000,8 @@ func (s *Switch) extractResult(deobfuscator ErrorDecrypter, n *networkResult,
 	// We've received a fail update which means we can finalize the
 	// user payment and return fail response.
 	case *lnwire.UpdateFailHTLC:
+		// TODO(yy): construct deobfuscator here to avoid creating it
+		// in paymentLifecycle even for settled HTLCs.
 		paymentErr := s.parseFailedPayment(
 			deobfuscator, attemptID, paymentHash, n.unencrypted,
 			n.isResolution, htlc,

itest/lnd_max_htlcs_test.go

@@ -122,8 +122,7 @@ func acceptHoldInvoice(ht *lntest.HarnessTest, idx int, sender,
 	invoice := receiver.RPC.AddHoldInvoice(req)
 
 	invStream := receiver.RPC.SubscribeSingleInvoice(hash[:])
-	inv := ht.ReceiveSingleInvoice(invStream)
-	require.Equal(ht, lnrpc.Invoice_OPEN, inv.State, "expect open")
+	ht.AssertInvoiceState(invStream, lnrpc.Invoice_OPEN)
 
 	sendReq := &routerrpc.SendPaymentRequest{
 		PaymentRequest: invoice.PaymentRequest,
@@ -145,9 +144,7 @@ func acceptHoldInvoice(ht *lntest.HarnessTest, idx int, sender,
 	)
 	require.Len(ht, payment.Htlcs, 1)
 
-	inv = ht.ReceiveSingleInvoice(invStream)
-	require.Equal(ht, lnrpc.Invoice_ACCEPTED, inv.State,
-		"expected accepted")
+	ht.AssertInvoiceState(invStream, lnrpc.Invoice_ACCEPTED)
 
 	return &holdSubscription{
 		recipient:           receiver,

routing/control_tower.go

@@ -31,8 +31,14 @@ type dbMPPayment interface {
 	// InFlightHTLCs returns all HTLCs that are in flight.
 	InFlightHTLCs() []channeldb.HTLCAttempt
 
-	// GetFailureReason returns the reason the payment failed.
-	GetFailureReason() *channeldb.FailureReason
+	// AllowMoreAttempts is used to decide whether we can safely attempt
+	// more HTLCs for a given payment state. Return an error if the payment
+	// is in an unexpected state.
+	AllowMoreAttempts() (bool, error)
+
+	// TerminalInfo returns the settled HTLC attempt or the payment's
+	// failure reason.
+	TerminalInfo() (*channeldb.HTLCAttempt, *channeldb.FailureReason)
 }
 
 // ControlTower tracks all outgoing payments made, whose primary purpose is to

routing/control_tower_test.go

@@ -134,8 +134,8 @@ func TestControlTowerSubscribeSuccess(t *testing.T) {
 			"subscriber %v failed, want %s, got %s", i,
 			channeldb.StatusSucceeded, result.GetStatus())
 
-		settle, _ := result.TerminalInfo()
-		if settle.Preimage != preimg {
+		attempt, _ := result.TerminalInfo()
+		if attempt.Settle.Preimage != preimg {
 			t.Fatal("unexpected preimage")
 		}
 		if len(result.HTLCs) != 1 {
@@ -264,9 +264,8 @@ func TestPaymentControlSubscribeAllSuccess(t *testing.T) {
 	)
 
 	settle1, _ := result1.TerminalInfo()
-	require.Equal(
-		t, preimg1, settle1.Preimage, "unexpected preimage payment 1",
-	)
+	require.Equal(t, preimg1, settle1.Settle.Preimage,
+		"unexpected preimage payment 1")
 
 	require.Len(
 		t, result1.HTLCs, 1, "expect 1 htlc for payment 1, got %d",
@@ -283,9 +282,8 @@ func TestPaymentControlSubscribeAllSuccess(t *testing.T) {
 	)
 
 	settle2, _ := result2.TerminalInfo()
-	require.Equal(
-		t, preimg2, settle2.Preimage, "unexpected preimage payment 2",
-	)
+	require.Equal(t, preimg2, settle2.Settle.Preimage,
+		"unexpected preimage payment 2")
 	require.Len(
 		t, result2.HTLCs, 1, "expect 1 htlc for payment 2, got %d",
 		len(result2.HTLCs),