From 9ee12ee02934d5bf522e1df6692483b689f2bb13 Mon Sep 17 00:00:00 2001 From: ziggie Date: Tue, 28 Jan 2025 17:34:37 +0100 Subject: [PATCH] invoices: treat replayed HTLCs beforehand. We make sure that HTLCs which have already been decided upon are resolved before before allowing the external interceptor to potentially cancel them back. This makes the implementation for the external HTLC interceptor more streamlined. --- invoices/invoiceregistry.go | 69 ++++++++++++++++++++++++++---- invoices/invoices.go | 3 ++ invoices/update.go | 69 +++++++++++++++++------------- itest/lnd_invoice_acceptor_test.go | 22 +++++++++- 4 files changed, 125 insertions(+), 38 deletions(-) diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go index cc76d5aef..97e6085ef 100644 --- a/invoices/invoiceregistry.go +++ b/invoices/invoiceregistry.go @@ -9,6 +9,7 @@ import ( "time" "github.com/lightningnetwork/lnd/clock" + "github.com/lightningnetwork/lnd/fn/v2" "github.com/lightningnetwork/lnd/lntypes" "github.com/lightningnetwork/lnd/lnwire" "github.com/lightningnetwork/lnd/queue" @@ -1086,17 +1087,36 @@ func (i *InvoiceRegistry) notifyExitHopHtlcLocked( updateSubscribers bool ) callback := func(inv *Invoice) (*InvoiceUpdateDesc, error) { - updateDesc, res, err := updateInvoice(ctx, inv) + // First check if this is a replayed htlc and resolve it + // according to its current state. We cannot decide differently + // once the HTLC has already been processed before. + isReplayed, res, err := resolveReplayedHtlc(ctx, inv) if err != nil { return nil, err } + if isReplayed { + resolution = res + return nil, nil + } - // Only send an update if the invoice state was changed. - updateSubscribers = updateDesc != nil && - updateDesc.State != nil - - // Assign resolution to outer scope variable. + // In case the HTLC interceptor cancels the HTLC set, we do NOT + // cancel the invoice however we cancel the complete HTLC set. if cancelSet { + // If the invoice is not open, something is wrong, we + // fail just the HTLC with the specific error. + if inv.State != ContractOpen { + log.Errorf("Invoice state (%v) is not OPEN, "+ + "cancelling HTLC set not allowed by "+ + "external source", inv.State) + + resolution = NewFailResolution( + ctx.circuitKey, ctx.currentHeight, + ResultInvoiceNotOpen, + ) + + return nil, nil + } + // If a cancel signal was set for the htlc set, we set // the resolution as a failure with an underpayment // indication. Something was wrong with this htlc, so @@ -1105,10 +1125,43 @@ func (i *InvoiceRegistry) notifyExitHopHtlcLocked( ctx.circuitKey, ctx.currentHeight, ResultAmountTooLow, ) - } else { - resolution = res + + // We cancel all HTLCs which are in the accepted state. + // + // NOTE: The current HTLC is not included because it + // was never accepted in the first place. + htlcs := inv.HTLCSet(ctx.setID(), HtlcStateAccepted) + htlcKeys := fn.KeySet[CircuitKey](htlcs) + + // The external source did cancel the htlc set, so we + // cancel all HTLCs in the set. We however keep the + // invoice in the open state. + // + // NOTE: The invoice event loop will still call the + // `cancelSingleHTLC` method for MPP payments, however + // because the HTLCs are already cancled back it will be + // a NOOP. + update := &InvoiceUpdateDesc{ + UpdateType: CancelHTLCsUpdate, + CancelHtlcs: htlcKeys, + SetID: setID, + } + + return update, nil } + updateDesc, res, err := updateInvoice(ctx, inv) + if err != nil { + return nil, err + } + + // Set resolution in outer scope only after successful update. + resolution = res + + // Only send an update if the invoice state was changed. + updateSubscribers = updateDesc != nil && + updateDesc.State != nil + return updateDesc, nil } diff --git a/invoices/invoices.go b/invoices/invoices.go index 32164cbe1..d6d59b4a0 100644 --- a/invoices/invoices.go +++ b/invoices/invoices.go @@ -765,6 +765,9 @@ type InvoiceStateUpdateDesc struct { // InvoiceUpdateCallback is a callback used in the db transaction to update the // invoice. +// TODO(ziggie): Add the option of additional return values to the callback +// for example the resolution which is currently assigned via an outer scope +// variable. type InvoiceUpdateCallback = func(invoice *Invoice) (*InvoiceUpdateDesc, error) // ValidateInvoice assures the invoice passes the checks for all the relevant diff --git a/invoices/update.go b/invoices/update.go index 3137bbbfa..6f7a34f4c 100644 --- a/invoices/update.go +++ b/invoices/update.go @@ -109,41 +109,52 @@ func (i invoiceUpdateCtx) acceptRes( return newAcceptResolution(i.circuitKey, outcome) } +// resolveReplayedHtlc returns the HTLC resolution for a replayed HTLC. The +// returned boolean indicates whether the HTLC was replayed or not. +func resolveReplayedHtlc(ctx *invoiceUpdateCtx, inv *Invoice) (bool, + HtlcResolution, error) { + + // Don't update the invoice when this is a replayed htlc. + htlc, replayedHTLC := inv.Htlcs[ctx.circuitKey] + if !replayedHTLC { + return false, nil, nil + } + + switch htlc.State { + case HtlcStateCanceled: + return true, ctx.failRes(ResultReplayToCanceled), nil + + case HtlcStateAccepted: + return true, ctx.acceptRes(resultReplayToAccepted), nil + + case HtlcStateSettled: + pre := inv.Terms.PaymentPreimage + + // Terms.PaymentPreimage will be nil for AMP invoices. + // Set it to the HTLCs AMP Preimage instead. + if pre == nil { + pre = htlc.AMP.Preimage + } + + return true, ctx.settleRes( + *pre, + ResultReplayToSettled, + ), nil + + default: + return true, nil, errors.New("unknown htlc state") + } +} + // updateInvoice is a callback for DB.UpdateInvoice that contains the invoice // settlement logic. It returns a HTLC resolution that indicates what the // outcome of the update was. +// +// NOTE: Make sure replayed HTLCs are always considered before calling this +// function. func updateInvoice(ctx *invoiceUpdateCtx, inv *Invoice) ( *InvoiceUpdateDesc, HtlcResolution, error) { - // Don't update the invoice when this is a replayed htlc. - htlc, ok := inv.Htlcs[ctx.circuitKey] - if ok { - switch htlc.State { - case HtlcStateCanceled: - return nil, ctx.failRes(ResultReplayToCanceled), nil - - case HtlcStateAccepted: - return nil, ctx.acceptRes(resultReplayToAccepted), nil - - case HtlcStateSettled: - pre := inv.Terms.PaymentPreimage - - // Terms.PaymentPreimage will be nil for AMP invoices. - // Set it to the HTLCs AMP Preimage instead. - if pre == nil { - pre = htlc.AMP.Preimage - } - - return nil, ctx.settleRes( - *pre, - ResultReplayToSettled, - ), nil - - default: - return nil, nil, errors.New("unknown htlc state") - } - } - // If no MPP payload was provided, then we expect this to be a keysend, // or a payment to an invoice created before we started to require the // MPP payload. diff --git a/itest/lnd_invoice_acceptor_test.go b/itest/lnd_invoice_acceptor_test.go index e94d81551..9195b102e 100644 --- a/itest/lnd_invoice_acceptor_test.go +++ b/itest/lnd_invoice_acceptor_test.go @@ -116,6 +116,7 @@ func testInvoiceHtlcModifierBasic(ht *lntest.HarnessTest) { &invoicesrpc.HtlcModifyResponse{ CircuitKey: modifierRequest.ExitHtlcCircuitKey, AmtPaid: &amtPaid, + CancelSet: tc.cancelSet, }, ) require.NoError(ht, err, "failed to send request") @@ -128,7 +129,9 @@ func testInvoiceHtlcModifierBasic(ht *lntest.HarnessTest) { require.Fail(ht, "timeout waiting for payment send") } - ht.Log("Ensure invoice status is settled") + ht.Logf("Ensure invoice status is expected state %v", + tc.finalInvoiceState) + require.Eventually(ht, func() bool { updatedInvoice := carol.RPC.LookupInvoice( tc.invoice.RHash, @@ -141,6 +144,13 @@ func testInvoiceHtlcModifierBasic(ht *lntest.HarnessTest) { tc.invoice.RHash, ) + // If the HTLC modifier canceled the incoming HTLC set, we don't + // expect any HTLCs in the invoice. + if tc.cancelSet { + require.Len(ht, updatedInvoice.Htlcs, 0) + return + } + require.Len(ht, updatedInvoice.Htlcs, 1) require.Equal( ht, lntest.CustomRecordsWithUnendorsed( @@ -231,6 +241,10 @@ type acceptorTestCase struct { // invoice is the invoice that will be paid. invoice *lnrpc.Invoice + + // cancelSet is a boolean which indicates whether the HTLC modifier + // canceled the incoming HTLC set. + cancelSet bool } // acceptorTestScenario is a helper struct to hold the test context and provides @@ -282,6 +296,12 @@ func (c *acceptorTestScenario) prepareTestCases() []*acceptorTestCase { lnwire.MinCustomRecordsTlvType: {1, 2, 3}, }, }, + { + invoiceAmountMsat: 9000, + sendAmountMsat: 1000, + finalInvoiceState: lnrpc.Invoice_OPEN, + cancelSet: true, + }, } for _, t := range cases {