From 8689eeede4c89db52bc8abd6d6de1e975005cd1b Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 10 Apr 2025 18:35:19 +0800 Subject: [PATCH 1/3] routing: add regression test to catch the panic --- routing/payment_lifecycle_test.go | 58 +++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) 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) +} From 106e1e91f0a6f276f2293061d26f47024080c4c6 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 10 Apr 2025 19:01:52 +0800 Subject: [PATCH 2/3] routing: patch the hash field for legacy payments For legacy payments, the hash field will be nil, and we need to use the payment identifier instead. We have multiple ways to fix this: A trivial solution is we can simply call `sharder.GetHash` in `collectResult`, and pass this hash to `attempt.Circuit()`, which ends up multiple methods taking the hash. This is bad as it's confusing why the methods of `HTLCAttempt` need to take another hash value, while itself already has the info via `HTLCAttempt.Hash`. We don't want an exceptional case to influence our main flow. We can then patch the field `HTLCAttempt.Hash`, and set it to the payment hash if it's nil, which can be done in `collectResult`. This is also less optimal as it means every htlc attempts, either legacy or not, now need to bear this context. The best way to do this is to patch the field in `reloadInflightAttempts`. As we are sure any new payments made won't be legacy, and the only source of legacy payments comes from reloading existing payments. --- routing/payment_lifecycle.go | 40 +++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) 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) } From 79ab2b2dafdb4da9a40e68288c214e521302887b Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 11 Apr 2025 15:42:29 +0800 Subject: [PATCH 3/3] docs: update release notes --- docs/release-notes/release-notes-0.19.0.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/release-notes/release-notes-0.19.0.md b/docs/release-notes/release-notes-0.19.0.md index 7ebc54037..7eb5150f0 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)