channeldb: expand PaymentStatus to explicitly represent payment status

This commit introduces more granular statuses to better determine a
payment's current state. Based on whether there are inflight HTLCs, the
state of each of the HTLCs, and whether the payment is failed, a total
of 5 states are derived, which can give a finer control over what action
to take based on a given state.

Also, `fetchPayment` now uses `decidePaymentStatus`. After applying the
new function, the returned payment status would be different,

```
| inflight | settled | failed | reason |       previous  ->   now          |
|:--------:|:-------:|:------:|:------:|:---------------------------------:|
|   true   |   true  |  true  |   yes  |    StatusInFlight(unchanged)      |
|   true   |   true  |  true  |   no   |    StatusInFlight(unchanged)      |
|   true   |   true  |  false |   yes  |    StatusInFlight(unchanged)      |
|   true   |   true  |  false |   no   |    StatusInFlight(unchanged)      |
|   true   |   false |  true  |   yes  |    StatusInFlight(unchanged)      |
|   true   |   false |  true  |   no   |    StatusInFlight(unchanged)      |
|   true   |   false |  false |   yes  |    StatusInFlight(unchanged)      |
|   true   |   false |  false |   no   |    StatusInFlight(unchanged)      |
|   false  |   true  |  true  |   yes  |    StatusSucceeded(unchanged)     |
|   false  |   true  |  true  |   no   |    StatusSucceeded(unchanged)     |
|   false  |   true  |  false |   yes  |    StatusSucceeded(unchanged)     |
|   false  |   true  |  false |   no   |    StatusSucceeded(unchanged)     |
|   false  |   false |  true  |   yes  |     StatusFailed(unchanged)       |
|   false  |   false |  true  |   no   |     StatusInFlight(unchanged)     |
|   false  |   false |  false |   yes  |     StatusFailed(unchanged)       |
|   false  |   false |  false |   no   |  StatusInFlight -> StatusInitiated|
```

channeldb/payment_control_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"crypto/rand"
 	"crypto/sha256"
+	"errors"
 	"fmt"
 	"io"
 	"reflect"
@@ -67,7 +68,9 @@ func TestPaymentControlSwitchFail(t *testing.T) {
 	require.NoError(t, err, "unable to send htlc message")
 
 	assertPaymentIndex(t, pControl, info.PaymentIdentifier)
-	assertPaymentStatus(t, pControl, info.PaymentIdentifier, StatusInFlight)
+	assertPaymentStatus(
+		t, pControl, info.PaymentIdentifier, StatusInitiated,
+	)
 	assertPaymentInfo(
 		t, pControl, info.PaymentIdentifier, info, nil, nil,
 	)
@@ -98,7 +101,9 @@ func TestPaymentControlSwitchFail(t *testing.T) {
 	assertPaymentIndex(t, pControl, info.PaymentIdentifier)
 	assertNoIndex(t, pControl, payment.SequenceNum)
 
-	assertPaymentStatus(t, pControl, info.PaymentIdentifier, StatusInFlight)
+	assertPaymentStatus(
+		t, pControl, info.PaymentIdentifier, StatusInitiated,
+	)
 	assertPaymentInfo(
 		t, pControl, info.PaymentIdentifier, info, nil, nil,
 	)
@@ -119,7 +124,9 @@ func TestPaymentControlSwitchFail(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	assertPaymentStatus(t, pControl, info.PaymentIdentifier, StatusInFlight)
+	assertPaymentStatus(
+		t, pControl, info.PaymentIdentifier, StatusInFlight,
+	)
 
 	htlc := &htlcStatus{
 		HTLCAttemptInfo: attempt,
@@ -198,7 +205,9 @@ func TestPaymentControlSwitchDoubleSend(t *testing.T) {
 	require.NoError(t, err, "unable to send htlc message")
 
 	assertPaymentIndex(t, pControl, info.PaymentIdentifier)
-	assertPaymentStatus(t, pControl, info.PaymentIdentifier, StatusInFlight)
+	assertPaymentStatus(
+		t, pControl, info.PaymentIdentifier, StatusInitiated,
+	)
 	assertPaymentInfo(
 		t, pControl, info.PaymentIdentifier, info, nil, nil,
 	)
@@ -207,10 +216,9 @@ func TestPaymentControlSwitchDoubleSend(t *testing.T) {
 	// payment hash, should result in error indicating that payment has
 	// already been sent.
 	err = pControl.InitPayment(info.PaymentIdentifier, info)
-	if err != ErrPaymentInFlight {
+	require.Equal(t, ErrPaymentExists, err, "payment control wrong "+
-		t.Fatalf("payment control wrong behaviour: " +
+		"behaviour: init payment again must trigger ErrPaymentExists "+
-			"double sending must trigger ErrPaymentInFlight error")
+		"error")
-	}
 
 	// Record an attempt.
 	_, err = pControl.RegisterAttempt(info.PaymentIdentifier, attempt)
@@ -682,7 +690,9 @@ func TestPaymentControlMultiShard(t *testing.T) {
 		}
 
 		assertPaymentIndex(t, pControl, info.PaymentIdentifier)
-		assertPaymentStatus(t, pControl, info.PaymentIdentifier, StatusInFlight)
+		assertPaymentStatus(
+			t, pControl, info.PaymentIdentifier, StatusInitiated,
+		)
 		assertPaymentInfo(
 			t, pControl, info.PaymentIdentifier, info, nil, nil,
 		)
@@ -708,7 +718,8 @@ func TestPaymentControlMultiShard(t *testing.T) {
 				t.Fatalf("unable to send htlc message: %v", err)
 			}
 			assertPaymentStatus(
-				t, pControl, info.PaymentIdentifier, StatusInFlight,
+				t, pControl, info.PaymentIdentifier,
+				StatusInFlight,
 			)
 
 			htlc := &htlcStatus{
@@ -807,11 +818,14 @@ func TestPaymentControlMultiShard(t *testing.T) {
 			// Record the reason we failed the payment, such that
 			// we can assert this later in the test.
 			firstFailReason = &failReason
-		}
 
-		// The payment should still be considered in-flight, since there
+			// The payment is now considered pending fail, since
-		// is still an active HTLC.
+			// there is still an active HTLC.
-		assertPaymentStatus(t, pControl, info.PaymentIdentifier, StatusInFlight)
+			assertPaymentStatus(
+				t, pControl, info.PaymentIdentifier,
+				StatusInFlight,
+			)
+		}
 
 		// Try to register yet another attempt. This should fail now
 		// that the payment has reached a terminal condition.
@@ -839,15 +853,12 @@ func TestPaymentControlMultiShard(t *testing.T) {
 					Preimage: preimg,
 				},
 			)
-			if err != nil {
+			require.NoError(t, err, "unable to settle")
-				t.Fatalf("error shouldn't have been "+
-					"received, got: %v", err)
-			}
 
 			htlc.settle = &preimg
 			assertPaymentInfo(
-				t, pControl, info.PaymentIdentifier, info,
+				t, pControl, info.PaymentIdentifier,
-				firstFailReason, htlc,
+				info, firstFailReason, htlc,
 			)
 		} else {
 			// Fail the attempt.
@@ -875,9 +886,7 @@ func TestPaymentControlMultiShard(t *testing.T) {
 			// syncing.
 			failReason := FailureReasonPaymentDetails
 			_, err = pControl.Fail(info.PaymentIdentifier, failReason)
-			if err != nil {
+			require.NoError(t, err, "unable to fail")
-				t.Fatalf("unable to fail payment hash: %v", err)
-			}
 		}
 
 		var (
@@ -1105,7 +1114,7 @@ func assertPaymentStatus(t *testing.T, p *PaymentControl,
 	t.Helper()
 
 	payment, err := p.FetchPayment(hash)
-	if err == ErrPaymentNotInitiated {
+	if errors.Is(err, ErrPaymentNotInitiated) {
 		return
 	}
 	if err != nil {

channeldb/payment_status.go

@@ -26,6 +26,9 @@ const (
 	StatusFailed PaymentStatus = 4
 )
 
+// errPaymentStatusUnknown is returned when a payment has an unknown status.
+var errPaymentStatusUnknown = fmt.Errorf("unknown payment status")
+
 // String returns readable representation of payment status.
 func (ps PaymentStatus) String() string {
 	switch ps {
@@ -127,3 +130,123 @@ func (ps PaymentStatus) updatable() error {
 		return fmt.Errorf("%w: %v", ErrUnknownPaymentStatus, ps)
 	}
 }
+
+// decidePaymentStatus uses the payment's DB state to determine a memory status
+// that's used by the payment router to decide following actions.
+// Together, we use four variables to determine the payment's status,
+//   - inflight: whether there are any pending HTLCs.
+//   - settled: whether any of the HTLCs has been settled.
+//   - htlc failed: whether any of the HTLCs has been failed.
+//   - payment failed: whether the payment has been marked as failed.
+//
+// Based on the above variables, we derive the status using the following
+// table,
+// | inflight | settled | htlc failed | payment failed |         status       |
+// |:--------:|:-------:|:-----------:|:--------------:|:--------------------:|
+// |   true   |   true  |     true    |      true      |    StatusInFlight    |
+// |   true   |   true  |     true    |      false     |    StatusInFlight    |
+// |   true   |   true  |     false   |      true      |    StatusInFlight    |
+// |   true   |   true  |     false   |      false     |    StatusInFlight    |
+// |   true   |   false |     true    |      true      |    StatusInFlight    |
+// |   true   |   false |     true    |      false     |    StatusInFlight    |
+// |   true   |   false |     false   |      true      |    StatusInFlight    |
+// |   true   |   false |     false   |      false     |    StatusInFlight    |
+// |   false  |   true  |     true    |      true      |    StatusSucceeded   |
+// |   false  |   true  |     true    |      false     |    StatusSucceeded   |
+// |   false  |   true  |     false   |      true      |    StatusSucceeded   |
+// |   false  |   true  |     false   |      false     |    StatusSucceeded   |
+// |   false  |   false |     true    |      true      |      StatusFailed    |
+// |   false  |   false |     true    |      false     |    StatusInFlight    |
+// |   false  |   false |     false   |      true      |      StatusFailed    |
+// |   false  |   false |     false   |      false     |    StatusInitiated   |
+//
+// When `inflight`, `settled`, `htlc failed`, and `payment failed` are false,
+// this indicates the payment is newly created and hasn't made any HTLCs yet.
+// When `inflight` and `settled` are false, `htlc failed` is true yet `payment
+// failed` is false, this indicates all the payment's HTLCs have occurred a
+// temporarily failure and the payment is still in-flight.
+func decidePaymentStatus(htlcs []HTLCAttempt,
+	reason *FailureReason) (PaymentStatus, error) {
+
+	var (
+		inflight      bool
+		htlcSettled   bool
+		htlcFailed    bool
+		paymentFailed bool
+	)
+
+	// If we have a failure reason, the payment is failed.
+	if reason != nil {
+		paymentFailed = true
+	}
+
+	// Go through all HTLCs for this payment, check whether we have any
+	// settled HTLC, and any still in-flight.
+	for _, h := range htlcs {
+		if h.Failure != nil {
+			htlcFailed = true
+			continue
+		}
+
+		if h.Settle != nil {
+			htlcSettled = true
+			continue
+		}
+
+		// If any of the HTLCs are not failed nor settled, we
+		// still have inflight HTLCs.
+		inflight = true
+	}
+
+	// Use the DB state to determine the status of the payment.
+	switch {
+	// If we have inflight HTLCs, no matter we have settled or failed
+	// HTLCs, or the payment failed, we still consider it inflight so we
+	// inform upper systems to wait for the results.
+	case inflight:
+		return StatusInFlight, nil
+
+	// If we have no in-flight HTLCs, and at least one of the HTLCs is
+	// settled, the payment succeeded.
+	//
+	// NOTE: when reaching this case, paymentFailed could be true, which
+	// means we have a conflicting state for this payment. We choose to
+	// mark the payment as succeeded because it's the receiver's
+	// responsibility to only settle the payment iff all HTLCs are
+	// received.
+	case htlcSettled:
+		return StatusSucceeded, nil
+
+	// If we have no in-flight HTLCs, and the payment failure is set, the
+	// payment is considered failed.
+	//
+	// NOTE: when reaching this case, settled must be false.
+	case paymentFailed:
+		return StatusFailed, nil
+
+	// If we have no in-flight HTLCs, yet the payment is NOT failed, it
+	// means all the HTLCs are failed. In this case we can attempt more
+	// HTLCs.
+	//
+	// NOTE: when reaching this case, both settled and paymentFailed must
+	// be false.
+	case htlcFailed:
+		return StatusInFlight, nil
+
+	// If none of the HTLCs is either settled or failed, and we have no
+	// inflight HTLCs, this means the payment has no HTLCs created yet.
+	//
+	// NOTE: when reaching this case, both settled and paymentFailed must
+	// be false.
+	case !htlcFailed:
+		return StatusInitiated, nil
+
+	// Otherwise an impossible state is reached.
+	//
+	// NOTE: we should never end up here.
+	default:
+		log.Error("Impossible payment state reached")
+		return 0, fmt.Errorf("%w: payment is corrupted",
+			errPaymentStatusUnknown)
+	}
+}

channeldb/payments.go

@@ -298,41 +298,10 @@ func fetchPayment(bucket kvdb.RBucket) (*MPPayment, error) {
 		failureReason = &reason
 	}
 
-	// Go through all HTLCs for this payment, noting whether we have any
+	// Now determine the payment's status.
-	// settled HTLC, and any still in-flight.
+	paymentStatus, err := decidePaymentStatus(htlcs, failureReason)
-	var inflight, settled bool
+	if err != nil {
-	for _, h := range htlcs {
+		return nil, err
-		if h.Failure != nil {
-			continue
-		}
-
-		if h.Settle != nil {
-			settled = true
-			continue
-		}
-
-		// If any of the HTLCs are not failed nor settled, we
-		// still have inflight HTLCs.
-		inflight = true
-	}
-
-	// Use the DB state to determine the status of the payment.
-	var paymentStatus PaymentStatus
-
-	switch {
-	// If any of the the HTLCs did succeed and there are no HTLCs in
-	// flight, the payment succeeded.
-	case !inflight && settled:
-		paymentStatus = StatusSucceeded
-
-	// If we have no in-flight HTLCs, and the payment failure is set, the
-	// payment is considered failed.
-	case !inflight && failureReason != nil:
-		paymentStatus = StatusFailed
-
-	// Otherwise it is still in flight.
-	default:
-		paymentStatus = StatusInFlight
 	}
 
 	return &MPPayment{