routing: add AllowMoreAttempts to decide whether more attempts are allowed

channeldb/mp_payment.go

@@ -469,6 +469,51 @@ 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.
 func serializeHTLCSettleInfo(w io.Writer, s *HTLCSettleInfo) error {
 	if _, err := w.Write(s.Preimage[:]); err != nil {

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

routing/control_tower.go

@@ -33,6 +33,11 @@ type dbMPPayment interface {
 
 	// 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)
 }
 
 // ControlTower tracks all outgoing payments made, whose primary purpose is to

routing/mock_test.go

@@ -839,6 +839,11 @@ func (m *mockMPPayment) GetFailureReason() *channeldb.FailureReason {
 	return reason.(*channeldb.FailureReason)
 }
 
+func (m *mockMPPayment) AllowMoreAttempts() (bool, error) {
+	args := m.Called()
+	return args.Bool(0), args.Error(1)
+}
+
 type mockLink struct {
 	htlcswitch.ChannelLink
 	bandwidth         lnwire.MilliSatoshi