Merge pull request #9703 from yyforyongyu/fix-attempt-hash

Patch htlc attempt hash for legacy payments

docs/release-notes/release-notes-0.19.0.md

@@ -111,6 +111,9 @@ keysend payment validation is stricter.
   process of the node won't be interrupted if a non-fatal error is returned from
   the subsystems.
 
+* [Fixed](https://github.com/lightningnetwork/lnd/pull/9703) a possible panic
+  when reloading legacy inflight payments which don't have the MPP feature.
+
 # New Features
 
 * Add support for [archiving channel backup](https://github.com/lightningnetwork/lnd/pull/9232)

routing/payment_lifecycle.go

@@ -14,6 +14,7 @@ import (
 	"github.com/lightningnetwork/lnd/graph/db/models"
 	"github.com/lightningnetwork/lnd/htlcswitch"
 	"github.com/lightningnetwork/lnd/lntypes"
+	"github.com/lightningnetwork/lnd/lnutils"
 	"github.com/lightningnetwork/lnd/lnwire"
 	"github.com/lightningnetwork/lnd/routing/route"
 	"github.com/lightningnetwork/lnd/routing/shards"
@@ -500,7 +501,8 @@ func (p *paymentLifecycle) collectResultAsync(attempt *channeldb.HTLCAttempt) {
 func (p *paymentLifecycle) collectResult(
 	attempt *channeldb.HTLCAttempt) (*htlcswitch.PaymentResult, error) {
 
-	log.Tracef("Collecting result for attempt %v", spew.Sdump(attempt))
+	log.Tracef("Collecting result for attempt %v",
+		lnutils.SpewLogClosure(attempt))
 
 	result := &htlcswitch.PaymentResult{}
 
@@ -1060,6 +1062,38 @@ func marshallError(sendError error, time time.Time) *channeldb.HTLCFailInfo {
 	return response
 }
 
+// patchLegacyPaymentHash will make a copy of the passed attempt and sets its
+// Hash field to be the payment hash if it's nil.
+//
+// NOTE: For legacy payments, which were created before the AMP feature was
+// enabled, the `Hash` field in their HTLC attempts is nil. In that case, we use
+// the payment hash as the `attempt.Hash` as they are identical.
+func (p *paymentLifecycle) patchLegacyPaymentHash(
+	a channeldb.HTLCAttempt) channeldb.HTLCAttempt {
+
+	// Exit early if this is not a legacy attempt.
+	if a.Hash != nil {
+		return a
+	}
+
+	// Log a warning if the user is still using legacy payments, which has
+	// weaker support.
+	log.Warnf("Found legacy htlc attempt %v in payment %v", a.AttemptID,
+		p.identifier)
+
+	// Set the attempt's hash to be the payment hash, which is the payment's
+	// `PaymentHash`` in the `PaymentCreationInfo`. For legacy payments
+	// before AMP feature, the `Hash` field was not set so we use the
+	// payment hash instead.
+	//
+	// NOTE: During the router's startup, we have a similar logic in
+	// `resumePayments`, in which we will use the payment hash instead if
+	// the attempt's hash is nil.
+	a.Hash = &p.identifier
+
+	return a
+}
+
 // reloadInflightAttempts is called when the payment lifecycle is resumed after
 // a restart. It reloads all inflight attempts from the control tower and
 // collects the results of the attempts that have been sent before.
@@ -1075,6 +1109,10 @@ func (p *paymentLifecycle) reloadInflightAttempts() (DBMPPayment, error) {
 		log.Infof("Resuming HTLC attempt %v for payment %v",
 			a.AttemptID, p.identifier)
 
+		// Potentially attach the payment hash to the `Hash` field if
+		// it's a legacy payment.
+		a = p.patchLegacyPaymentHash(a)
+
 		p.resultCollector(&a)
 	}
 

routing/payment_lifecycle_test.go

@@ -1809,3 +1809,61 @@ func TestHandleAttemptResultSuccess(t *testing.T) {
 	require.NoError(t, err, "expected no error")
 	require.Equal(t, attempt, attemptResult.attempt)
 }
+
+// TestReloadInflightAttemptsLegacy checks that when handling a legacy HTLC
+// attempt, `collectResult` behaves as expected.
+func TestReloadInflightAttemptsLegacy(t *testing.T) {
+	t.Parallel()
+
+	// Create a test paymentLifecycle with the initial two calls mocked.
+	p, m := newTestPaymentLifecycle(t)
+
+	// Mount the resultCollector to check the full call path.
+	p.resultCollector = p.collectResultAsync
+
+	// Create testing params.
+	paymentAmt := 10_000
+	preimage := lntypes.Preimage{1}
+	attempt := makeSettledAttempt(t, paymentAmt, preimage)
+
+	// Make the attempt.Hash to be nil to mock a legacy payment.
+	attempt.Hash = nil
+
+	// Create a mock result returned from the switch.
+	result := &htlcswitch.PaymentResult{
+		Preimage: preimage,
+	}
+
+	// 1. calls `FetchPayment` and return the payment.
+	m.control.On("FetchPayment", p.identifier).Return(m.payment, nil).Once()
+
+	// 2. calls `InFlightHTLCs` and return the attempt.
+	attempts := []channeldb.HTLCAttempt{*attempt}
+	m.payment.On("InFlightHTLCs").Return(attempts).Once()
+
+	// 3. Mock the htlcswitch to return a the result chan.
+	resultChan := make(chan *htlcswitch.PaymentResult, 1)
+	m.payer.On("GetAttemptResult",
+		attempt.AttemptID, p.identifier, mock.Anything,
+	).Return(resultChan, nil).Once().Run(func(args mock.Arguments) {
+		// Send the preimage to the result chan.
+		resultChan <- result
+	})
+
+	// Now call the method under test.
+	payment, err := p.reloadInflightAttempts()
+	require.NoError(t, err)
+	require.Equal(t, m.payment, payment)
+
+	var r *switchResult
+
+	// Assert the result is returned within testTimeout.
+	waitErr := wait.NoError(func() error {
+		r = <-p.resultCollected
+		return nil
+	}, testTimeout)
+	require.NoError(t, waitErr, "timeout waiting for result")
+
+	// Assert the result is received as expected.
+	require.Equal(t, result, r.result)
+}