diff --git a/docs/release-notes/release-notes-0.19.0.md b/docs/release-notes/release-notes-0.19.0.md index 31d1800cf..0228d234d 100644 --- a/docs/release-notes/release-notes-0.19.0.md +++ b/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) diff --git a/routing/payment_lifecycle.go b/routing/payment_lifecycle.go index 9a7414079..a64c8c87e 100644 --- a/routing/payment_lifecycle.go +++ b/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) } diff --git a/routing/payment_lifecycle_test.go b/routing/payment_lifecycle_test.go index b04b04d4b..9df02ec32 100644 --- a/routing/payment_lifecycle_test.go +++ b/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) +}