From 9ee12ee02934d5bf522e1df6692483b689f2bb13 Mon Sep 17 00:00:00 2001 From: ziggie Date: Tue, 28 Jan 2025 17:34:37 +0100 Subject: [PATCH 1/8] 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 | 67 +++++++++++++++++------------ itest/lnd_invoice_acceptor_test.go | 22 +++++++++- 4 files changed, 124 insertions(+), 37 deletions(-) diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go index cc76d5aefa5..97e6085efd0 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 32164cbe179..d6d59b4a05e 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 3137bbbfa71..6f7a34f4c46 100644 --- a/invoices/update.go +++ b/invoices/update.go @@ -109,40 +109,51 @@ func (i invoiceUpdateCtx) acceptRes( return newAcceptResolution(i.circuitKey, outcome) } -// 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. -func updateInvoice(ctx *invoiceUpdateCtx, inv *Invoice) ( - *InvoiceUpdateDesc, HtlcResolution, error) { +// 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, 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 - } + htlc, replayedHTLC := inv.Htlcs[ctx.circuitKey] + if !replayedHTLC { + return false, nil, nil + } + + switch htlc.State { + case HtlcStateCanceled: + return true, ctx.failRes(ResultReplayToCanceled), nil - return nil, ctx.settleRes( - *pre, - ResultReplayToSettled, - ), nil + case HtlcStateAccepted: + return true, ctx.acceptRes(resultReplayToAccepted), nil - default: - return nil, nil, errors.New("unknown htlc state") + 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) { // 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 diff --git a/itest/lnd_invoice_acceptor_test.go b/itest/lnd_invoice_acceptor_test.go index e94d8155142..9195b102e3c 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 { From 17e37bd7c20fb3aca993378d178e08c54e0b7836 Mon Sep 17 00:00:00 2001 From: ziggie Date: Tue, 28 Jan 2025 19:05:46 +0100 Subject: [PATCH 2/8] multi: introduce new traffic shaper method. We introduce a new specific fail resolution error when the external HTLC interceptor denies the incoming HTLC. Moreover we introduce a new traffic shaper method which moves the implementation of asset HTLC to the external layers. Moreover itests are adopted to reflect this new change. --- htlcswitch/interfaces.go | 5 + htlcswitch/link.go | 25 ++- invoices/invoiceregistry.go | 12 +- invoices/modification_interceptor.go | 3 - invoices/resolution_result.go | 10 +- itest/list_on_test.go | 8 - itest/lnd_forward_interceptor_test.go | 218 -------------------------- routing/bandwidth_test.go | 4 + 8 files changed, 37 insertions(+), 248 deletions(-) diff --git a/htlcswitch/interfaces.go b/htlcswitch/interfaces.go index 78cb0198928..1ea1318ff1d 100644 --- a/htlcswitch/interfaces.go +++ b/htlcswitch/interfaces.go @@ -508,4 +508,9 @@ type AuxTrafficShaper interface { PaymentBandwidth(htlcBlob, commitmentBlob fn.Option[tlv.Blob], linkBandwidth, htlcAmt lnwire.MilliSatoshi) (lnwire.MilliSatoshi, error) + + // IsCustomHTLC returns true if the HTLC carries the set of relevant + // custom records to put it under the purview of the traffic shaper, + // meaning that it's from a custom channel. + IsCustomHTLC(htlcRecords lnwire.CustomRecords) bool } diff --git a/htlcswitch/link.go b/htlcswitch/link.go index f0e95899240..77c8f67e313 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -4164,21 +4164,20 @@ func (l *channelLink) processExitHop(add lnwire.UpdateAddHTLC, return nil } + // In case the traffic shaper is active, we'll check if the HTLC has + // custom records and skip the amount check in the onion payload below. + isCustomHTLC := fn.MapOptionZ( + l.cfg.AuxTrafficShaper, + func(ts AuxTrafficShaper) bool { + return ts.IsCustomHTLC(add.CustomRecords) + }, + ) + // As we're the exit hop, we'll double check the hop-payload included in // the HTLC to ensure that it was crafted correctly by the sender and - // is compatible with the HTLC we were extended. - // - // For a special case, if the fwdInfo doesn't have any blinded path - // information, and the incoming HTLC had special extra data, then - // we'll skip this amount check. The invoice acceptor will make sure we - // reject the HTLC if it's not containing the correct amount after - // examining the custom data. - hasBlindedPath := fwdInfo.NextBlinding.IsSome() - customHTLC := len(add.CustomRecords) > 0 && !hasBlindedPath - log.Tracef("Exit hop has_blinded_path=%v custom_htlc_bypass=%v", - hasBlindedPath, customHTLC) - - if !customHTLC && add.Amount < fwdInfo.AmountToForward { + // is compatible with the HTLC we were extended. If an external + // validator is active we might bypass the amount check. + if !isCustomHTLC && add.Amount < fwdInfo.AmountToForward { l.log.Errorf("onion payload of incoming htlc(%x) has "+ "incompatible value: expected <=%v, got %v", add.PaymentHash, add.Amount, fwdInfo.AmountToForward) diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go index 97e6085efd0..49f0cb0f0c1 100644 --- a/invoices/invoiceregistry.go +++ b/invoices/invoiceregistry.go @@ -1117,13 +1117,15 @@ func (i *InvoiceRegistry) notifyExitHopHtlcLocked( 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 - // we probably can't settle the invoice at all. + // The error `ExternalValidationFailed` error + // information will be packed in the + // `FailIncorrectDetails` msg when sending the msg to + // the peer. Error codes are defined by the BOLT 04 + // specification. The error text can be arbitrary + // therefore we return a custom error msg. resolution = NewFailResolution( ctx.circuitKey, ctx.currentHeight, - ResultAmountTooLow, + ExternalValidationFailed, ) // We cancel all HTLCs which are in the accepted state. diff --git a/invoices/modification_interceptor.go b/invoices/modification_interceptor.go index 58f5b63d074..c8d6783f5d5 100644 --- a/invoices/modification_interceptor.go +++ b/invoices/modification_interceptor.go @@ -137,9 +137,6 @@ func (s *HtlcModificationInterceptor) Intercept(clientRequest HtlcModifyRequest, // Wait for the client to respond or an error to occur. select { case response := <-responseChan: - log.Debugf("Received invoice HTLC interceptor response: %v", - response) - responseCallback(*response) return nil diff --git a/invoices/resolution_result.go b/invoices/resolution_result.go index f66198f0524..5a7d9984dc2 100644 --- a/invoices/resolution_result.go +++ b/invoices/resolution_result.go @@ -120,6 +120,10 @@ const ( // ResultAmpReconstruction is returned when the derived child // hash/preimage pairs were invalid for at least one HTLC in the set. ResultAmpReconstruction + + // ExternalValidationFailed is returned when the external validation + // failed. + ExternalValidationFailed ) // String returns a string representation of the result. @@ -189,6 +193,9 @@ func (f FailResolutionResult) FailureString() string { case ResultAmpReconstruction: return "amp reconstruction failed" + case ExternalValidationFailed: + return "external validation failed" + default: return "unknown failure resolution result" } @@ -202,7 +209,8 @@ func (f FailResolutionResult) IsSetFailure() bool { ResultAmpReconstruction, ResultHtlcSetTotalTooLow, ResultHtlcSetTotalMismatch, - ResultHtlcSetOverpayment: + ResultHtlcSetOverpayment, + ExternalValidationFailed: return true diff --git a/itest/list_on_test.go b/itest/list_on_test.go index 6de56605bb4..2cf71feec90 100644 --- a/itest/list_on_test.go +++ b/itest/list_on_test.go @@ -406,14 +406,6 @@ var allTestCases = []*lntest.TestCase{ Name: "forward interceptor", TestFunc: testForwardInterceptorBasic, }, - { - Name: "forward interceptor modified htlc", - TestFunc: testForwardInterceptorModifiedHtlc, - }, - { - Name: "forward interceptor wire records", - TestFunc: testForwardInterceptorWireRecords, - }, { Name: "forward interceptor restart", TestFunc: testForwardInterceptorRestart, diff --git a/itest/lnd_forward_interceptor_test.go b/itest/lnd_forward_interceptor_test.go index 93012c98f3b..7a6888a57d0 100644 --- a/itest/lnd_forward_interceptor_test.go +++ b/itest/lnd_forward_interceptor_test.go @@ -10,7 +10,6 @@ import ( "github.com/btcsuite/btcd/btcutil" "github.com/lightningnetwork/lnd/chainreg" "github.com/lightningnetwork/lnd/lnrpc" - "github.com/lightningnetwork/lnd/lnrpc/invoicesrpc" "github.com/lightningnetwork/lnd/lnrpc/routerrpc" "github.com/lightningnetwork/lnd/lntest" "github.com/lightningnetwork/lnd/lntest/node" @@ -346,223 +345,6 @@ func testForwardInterceptorBasic(ht *lntest.HarnessTest) { } } -// testForwardInterceptorModifiedHtlc tests that the interceptor can modify the -// amount and custom records of an intercepted HTLC and resume it. -func testForwardInterceptorModifiedHtlc(ht *lntest.HarnessTest) { - const chanAmt = btcutil.Amount(300000) - p := lntest.OpenChannelParams{Amt: chanAmt} - - // Initialize the test context with 3 connected nodes. - cfgs := [][]string{nil, nil, nil} - - // Open and wait for channels. - _, nodes := ht.CreateSimpleNetwork(cfgs, p) - alice, bob, carol := nodes[0], nodes[1], nodes[2] - - // Init the scenario. - ts := &interceptorTestScenario{ - ht: ht, - alice: alice, - bob: bob, - carol: carol, - } - - // Connect an interceptor to Bob's node. - bobInterceptor, cancelBobInterceptor := bob.RPC.HtlcInterceptor() - - // We're going to modify the payment amount and want Carol to accept the - // payment, so we set up an invoice acceptor on Dave. - carolAcceptor, carolCancel := carol.RPC.InvoiceHtlcModifier() - defer carolCancel() - - // Prepare the test cases. - invoiceValueAmtMsat := int64(20_000_000) - req := &lnrpc.Invoice{ValueMsat: invoiceValueAmtMsat} - addResponse := carol.RPC.AddInvoice(req) - invoice := carol.RPC.LookupInvoice(addResponse.RHash) - tc := &interceptorTestCase{ - amountMsat: invoiceValueAmtMsat, - invoice: invoice, - payAddr: invoice.PaymentAddr, - } - - // We initiate a payment from Alice. - done := make(chan struct{}) - go func() { - // Signal that all the payments have been sent. - defer close(done) - - ts.sendPaymentAndAssertAction(tc) - }() - - // We start the htlc interceptor with a simple implementation that saves - // all intercepted packets. These packets are held to simulate a - // pending payment. - packet := ht.ReceiveHtlcInterceptor(bobInterceptor) - - // Resume the intercepted HTLC with a modified amount and custom - // records. - customRecords := make(map[uint64][]byte) - - // Add custom records entry. - crKey := uint64(65537) - crValue := []byte("custom-records-test-value") - customRecords[crKey] = crValue - - // Modify the amount of the HTLC, so we send out less than the original - // amount. - const modifyAmount = 5_000_000 - newOutAmountMsat := packet.OutgoingAmountMsat - modifyAmount - err := bobInterceptor.Send(&routerrpc.ForwardHtlcInterceptResponse{ - IncomingCircuitKey: packet.IncomingCircuitKey, - OutAmountMsat: newOutAmountMsat, - OutWireCustomRecords: customRecords, - Action: actionResumeModify, - }) - require.NoError(ht, err, "failed to send request") - - invoicePacket := ht.ReceiveInvoiceHtlcModification(carolAcceptor) - require.EqualValues( - ht, newOutAmountMsat, invoicePacket.ExitHtlcAmt, - ) - amtPaid := newOutAmountMsat + modifyAmount - err = carolAcceptor.Send(&invoicesrpc.HtlcModifyResponse{ - CircuitKey: invoicePacket.ExitHtlcCircuitKey, - AmtPaid: &amtPaid, - }) - require.NoError(ht, err, "carol acceptor response") - - // Cancel the context, which will disconnect Bob's interceptor. - cancelBobInterceptor() - - // Make sure all goroutines are finished. - select { - case <-done: - case <-time.After(defaultTimeout): - require.Fail(ht, "timeout waiting for sending payment") - } - - // Assert that the payment was successful. - var preimage lntypes.Preimage - copy(preimage[:], invoice.RPreimage) - ht.AssertPaymentStatus(alice, preimage, lnrpc.Payment_SUCCEEDED) -} - -// testForwardInterceptorWireRecords tests that the interceptor can read any -// wire custom records provided by the sender of a payment as part of the -// update_add_htlc message. -func testForwardInterceptorWireRecords(ht *lntest.HarnessTest) { - const chanAmt = btcutil.Amount(300000) - p := lntest.OpenChannelParams{Amt: chanAmt} - - // Initialize the test context with 4 connected nodes. - cfgs := [][]string{nil, nil, nil, nil} - - // Open and wait for channels. - _, nodes := ht.CreateSimpleNetwork(cfgs, p) - alice, bob, carol, dave := nodes[0], nodes[1], nodes[2], nodes[3] - - // Connect an interceptor to Bob's node. - bobInterceptor, cancelBobInterceptor := bob.RPC.HtlcInterceptor() - defer cancelBobInterceptor() - - // Also connect an interceptor on Carol's node to check whether we're - // relaying the TLVs send in update_add_htlc over Alice -> Bob on the - // Bob -> Carol link. - carolInterceptor, cancelCarolInterceptor := carol.RPC.HtlcInterceptor() - defer cancelCarolInterceptor() - - // We're going to modify the payment amount and want Dave to accept the - // payment, so we set up an invoice acceptor on Dave. - daveAcceptor, daveCancel := dave.RPC.InvoiceHtlcModifier() - defer daveCancel() - - req := &lnrpc.Invoice{ValueMsat: 20_000_000} - addResponse := dave.RPC.AddInvoice(req) - invoice := dave.RPC.LookupInvoice(addResponse.RHash) - - customRecords := map[uint64][]byte{ - 65537: []byte("test"), - } - sendReq := &routerrpc.SendPaymentRequest{ - PaymentRequest: invoice.PaymentRequest, - TimeoutSeconds: int32(wait.PaymentTimeout.Seconds()), - FeeLimitMsat: noFeeLimitMsat, - FirstHopCustomRecords: customRecords, - } - ht.SendPaymentAssertInflight(alice, sendReq) - - // We start the htlc interceptor with a simple implementation that saves - // all intercepted packets. These packets are held to simulate a - // pending payment. - packet := ht.ReceiveHtlcInterceptor(bobInterceptor) - require.Equal(ht, lntest.CustomRecordsWithUnendorsed( - customRecords, - ), packet.InWireCustomRecords) - - // Just resume the payment on Bob. - err := bobInterceptor.Send(&routerrpc.ForwardHtlcInterceptResponse{ - IncomingCircuitKey: packet.IncomingCircuitKey, - Action: actionResume, - }) - require.NoError(ht, err, "failed to send request") - - // Assert that the Alice -> Bob custom records in update_add_htlc are - // not propagated on the Bob -> Carol link, just an endorsement signal. - packet = ht.ReceiveHtlcInterceptor(carolInterceptor) - require.Equal(ht, lntest.CustomRecordsWithUnendorsed(nil), - packet.InWireCustomRecords) - - // We're going to tell Carol to forward 5k sats less to Dave. We need to - // set custom records on the HTLC as well, to make sure the HTLC isn't - // rejected outright and actually gets to the invoice acceptor. - const modifyAmount = 5_000_000 - newOutAmountMsat := packet.OutgoingAmountMsat - modifyAmount - err = carolInterceptor.Send(&routerrpc.ForwardHtlcInterceptResponse{ - IncomingCircuitKey: packet.IncomingCircuitKey, - OutAmountMsat: newOutAmountMsat, - OutWireCustomRecords: customRecords, - Action: actionResumeModify, - }) - require.NoError(ht, err, "carol interceptor response") - - // The payment should get to Dave, and we should be able to intercept - // and modify it, telling Dave to accept it. - invoicePacket := ht.ReceiveInvoiceHtlcModification(daveAcceptor) - require.EqualValues( - ht, newOutAmountMsat, invoicePacket.ExitHtlcAmt, - ) - amtPaid := newOutAmountMsat + modifyAmount - err = daveAcceptor.Send(&invoicesrpc.HtlcModifyResponse{ - CircuitKey: invoicePacket.ExitHtlcCircuitKey, - AmtPaid: &amtPaid, - }) - require.NoError(ht, err, "dave acceptor response") - - // Assert that the payment was successful. - var preimage lntypes.Preimage - copy(preimage[:], invoice.RPreimage) - ht.AssertPaymentStatus( - alice, preimage, lnrpc.Payment_SUCCEEDED, - func(p *lnrpc.Payment) error { - recordsEqual := reflect.DeepEqual( - p.FirstHopCustomRecords, - lntest.CustomRecordsWithUnendorsed( - customRecords, - ), - ) - if !recordsEqual { - return fmt.Errorf("expected custom records to "+ - "be equal, got %v expected %v", - p.FirstHopCustomRecords, - sendReq.FirstHopCustomRecords) - } - - return nil - }, - ) -} - // testForwardInterceptorRestart tests that the interceptor can read any wire // custom records provided by the sender of a payment as part of the // update_add_htlc message and that those records are persisted correctly and diff --git a/routing/bandwidth_test.go b/routing/bandwidth_test.go index b31d0095ac1..259f347bae5 100644 --- a/routing/bandwidth_test.go +++ b/routing/bandwidth_test.go @@ -164,3 +164,7 @@ func (*mockTrafficShaper) ProduceHtlcExtraData(totalAmount lnwire.MilliSatoshi, return totalAmount, nil, nil } + +func (*mockTrafficShaper) IsCustomHTLC(_ lnwire.CustomRecords) bool { + return false +} From 0532990a04ede63a2d198dc5a0dcfa8a28e8c0a5 Mon Sep 17 00:00:00 2001 From: ziggie Date: Tue, 28 Jan 2025 18:03:12 +0100 Subject: [PATCH 3/8] invoices: enhance the unit test suite. The invoiceregistry test suite also includes unit tests for multi part payment especially also including payments to AMP invoices. --- invoices/invoiceregistry_test.go | 348 ++++++++++++++++++++++++++++++- invoices/mock.go | 19 +- invoices/test_utils_test.go | 19 +- 3 files changed, 375 insertions(+), 11 deletions(-) diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go index 3deb6405c02..b926260f846 100644 --- a/invoices/invoiceregistry_test.go +++ b/invoices/invoiceregistry_test.go @@ -20,6 +20,7 @@ import ( "github.com/lightningnetwork/lnd/lnwire" "github.com/lightningnetwork/lnd/record" "github.com/lightningnetwork/lnd/sqldb" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -108,6 +109,14 @@ func TestInvoiceRegistry(t *testing.T) { name: "SpontaneousAmpPayment", test: testSpontaneousAmpPayment, }, + { + name: "FailPartialMPPPaymentExternal", + test: testFailPartialMPPPaymentExternal, + }, + { + name: "FailPartialAMPPayment", + test: testFailPartialAMPPayment, + }, } makeKeyValueDB := func(t *testing.T) (invpkg.InvoiceDB, @@ -204,7 +213,7 @@ func testSettleInvoice(t *testing.T, require.Equal(t, subscription.PayHash(), &testInvoicePaymentHash) // Add the invoice. - testInvoice := newInvoice(t, false) + testInvoice := newInvoice(t, false, false) addIdx, err := ctx.registry.AddInvoice( ctxb, testInvoice, testInvoicePaymentHash, ) @@ -395,7 +404,7 @@ func testCancelInvoiceImpl(t *testing.T, gc bool, require.Equal(t, subscription.PayHash(), &testInvoicePaymentHash) // Add the invoice. - testInvoice := newInvoice(t, false) + testInvoice := newInvoice(t, false, false) _, err = ctx.registry.AddInvoice( ctxb, testInvoice, testInvoicePaymentHash, ) @@ -555,7 +564,7 @@ func testSettleHoldInvoice(t *testing.T, require.Equal(t, subscription.PayHash(), &testInvoicePaymentHash) // Add the invoice. - invoice := newInvoice(t, true) + invoice := newInvoice(t, true, false) _, err = registry.AddInvoice(ctxb, invoice, testInvoicePaymentHash) require.NoError(t, err) @@ -716,7 +725,7 @@ func testCancelHoldInvoice(t *testing.T, ctxb := context.Background() // Add the invoice. - invoice := newInvoice(t, true) + invoice := newInvoice(t, true, false) _, err = registry.AddInvoice(ctxb, invoice, testInvoicePaymentHash) require.NoError(t, err) @@ -1043,7 +1052,7 @@ func testMppPayment(t *testing.T, ctxb := context.Background() // Add the invoice. - testInvoice := newInvoice(t, false) + testInvoice := newInvoice(t, false, false) _, err := ctx.registry.AddInvoice( ctxb, testInvoice, testInvoicePaymentHash, ) @@ -1141,7 +1150,7 @@ func testMppPaymentWithOverpayment(t *testing.T, ctx := newTestContext(t, nil, makeDB) // Add the invoice. - testInvoice := newInvoice(t, false) + testInvoice := newInvoice(t, false, false) _, err := ctx.registry.AddInvoice( ctxb, testInvoice, testInvoicePaymentHash, ) @@ -1432,7 +1441,7 @@ func testHeightExpiryWithRegistryImpl(t *testing.T, numParts int, settle bool, // Add a hold invoice, we set a non-nil payment request so that this // invoice is not considered a keysend by the expiry watcher. - testInvoice := newInvoice(t, false) + testInvoice := newInvoice(t, false, false) testInvoice.HodlInvoice = true testInvoice.PaymentRequest = []byte{1, 2, 3} @@ -1545,7 +1554,7 @@ func testMultipleSetHeightExpiry(t *testing.T, ctx := newTestContext(t, nil, makeDB) // Add a hold invoice. - testInvoice := newInvoice(t, true) + testInvoice := newInvoice(t, true, false) ctxb := context.Background() _, err := ctx.registry.AddInvoice( @@ -2109,3 +2118,326 @@ func testSpontaneousAmpPaymentImpl( } } } + +// testFailPartialMPPPaymentExternal tests that the HTLC set is cancelled back +// as soon as the HTLC interceptor denies one of the HTLCs. +func testFailPartialMPPPaymentExternal(t *testing.T, + makeDB func(t *testing.T) (invpkg.InvoiceDB, *clock.TestClock)) { + + t.Parallel() + + mockHtlcInterceptor := &invpkg.MockHtlcModifier{} + cfg := defaultRegistryConfig() + cfg.HtlcInterceptor = mockHtlcInterceptor + ctx := newTestContext(t, &cfg, makeDB) + + // Add an invoice which we are going to pay via a MPP set. + testInvoice := newInvoice(t, false, false) + + ctxb := context.Background() + _, err := ctx.registry.AddInvoice( + ctxb, testInvoice, testInvoicePaymentHash, + ) + require.NoError(t, err) + + mppPayload := &mockPayload{ + mpp: record.NewMPP(testInvoiceAmount, [32]byte{}), + } + + // Send first HTLC which pays part of the invoice but keeps the invoice + // in an open state because the amount is less than the invoice amount. + hodlChan1 := make(chan interface{}, 1) + resolution, err := ctx.registry.NotifyExitHopHtlc( + testInvoicePaymentHash, testInvoice.Terms.Value/3, + testHtlcExpiry, testCurrentHeight, getCircuitKey(1), + hodlChan1, nil, mppPayload, + ) + require.NoError(t, err) + require.Nil(t, resolution, "did not expect direct resolution") + + // Register the expected response from the interceptor so that the + // whole HTLC set is cancelled. + expectedResponse := invpkg.HtlcModifyResponse{ + CancelSet: true, + } + mockHtlcInterceptor.On("Intercept", mock.Anything, mock.Anything). + Return(nil, expectedResponse) + + // Send htlc 2. We expect the HTLC to be cancelled because the + // interceptor will deny it. + resolution, err = ctx.registry.NotifyExitHopHtlc( + testInvoicePaymentHash, testInvoice.Terms.Value/2, + testHtlcExpiry, testCurrentHeight, getCircuitKey(2), nil, + nil, mppPayload, + ) + require.NoError(t, err) + failResolution, ok := resolution.(*invpkg.HtlcFailResolution) + require.True(t, ok, "expected fail resolution, got: %T", resolution) + + // Make sure the resolution includes the custom error msg. + require.Equal(t, invpkg.ExternalValidationFailed, + failResolution.Outcome, "expected ExternalValidationFailed, "+ + "got: %v", failResolution.Outcome) + + // Expect HLTC 1 also to be cancelled because it is part of the cancel + // set and the interceptor cancelled the whole set after receiving the + // second HTLC. + select { + case resolution := <-hodlChan1: + htlcResolution, _ := resolution.(invpkg.HtlcResolution) + failResolution, ok = htlcResolution.(*invpkg.HtlcFailResolution) + require.True( + t, ok, "expected fail resolution, got: %T", + htlcResolution, + ) + require.Equal( + t, invpkg.ExternalValidationFailed, + failResolution.Outcome, "expected "+ + "ExternalValidationFailed, got: %v", + failResolution.Outcome, + ) + + case <-time.After(testTimeout): + t.Fatal("timeout waiting for HTLC resolution") + } + + // Assert that the invoice is still open. + inv, err := ctx.registry.LookupInvoice(ctxb, testInvoicePaymentHash) + require.NoError(t, err) + require.Equal(t, invpkg.ContractOpen, inv.State, "expected "+ + "OPEN invoice") + + // Now let the invoice expire. + currentTime := ctx.clock.Now() + ctx.clock.SetTime(currentTime.Add(61 * time.Minute)) + + // Make sure the invoices changes to the canceled state. + require.Eventuallyf(t, func() bool { + inv, err := ctx.registry.LookupInvoice( + ctxb, testInvoicePaymentHash, + ) + require.NoError(t, err) + + return inv.State == invpkg.ContractCanceled + }, testTimeout, time.Millisecond*100, "invoice not canceled") + + // Fetch the invoice again and compare the number of cancelled HTLCs. + inv, err = ctx.registry.LookupInvoice( + ctxb, testInvoicePaymentHash, + ) + require.NoError(t, err) + + // Make sure all HTLCs are in the canceled state which in our case is + // only the first one because the second HTLC was never added to the + // invoice registry in the first place. + require.Len(t, inv.Htlcs, 1) + require.Equal( + t, invpkg.HtlcStateCanceled, inv.Htlcs[getCircuitKey(1)].State, + ) +} + +// testFailPartialAMPPayment tests the MPP timeout logic for AMP invoices. It +// makes sure that all HTLCs are cancelled if the full invoice amount is not +// received. Moreover it points out some TODOs to make AMP invoices more robust. +func testFailPartialAMPPayment(t *testing.T, + makeDB func(t *testing.T) (invpkg.InvoiceDB, *clock.TestClock)) { + + t.Parallel() + + ctx := newTestContext(t, nil, makeDB) + ctxb := context.Background() + + const ( + expiry = uint32(testCurrentHeight + 20) + numShards = 4 + ) + + var ( + shardAmt = testInvoiceAmount / lnwire.MilliSatoshi(numShards) + setID [32]byte + payAddr [32]byte + ) + _, err := rand.Read(payAddr[:]) + require.NoError(t, err) + + // Create an AMP invoice we are going to pay via a multi-part payment. + ampInvoice := newInvoice(t, false, true) + + // An AMP invoice is referenced by the payment address. + ampInvoice.Terms.PaymentAddr = payAddr + + _, err = ctx.registry.AddInvoice( + ctxb, ampInvoice, testInvoicePaymentHash, + ) + require.NoError(t, err) + + // Generate a random setID for the HTLCs. + _, err = rand.Read(setID[:]) + require.NoError(t, err) + + htlcPayload1 := &mockPayload{ + mpp: record.NewMPP(testInvoiceAmount, payAddr), + // We are not interested in settling the AMP HTLC so we don't + // use valid shares. + amp: record.NewAMP([32]byte{1}, setID, 1), + } + + // Send first HTLC which pays part of the invoice. + hodlChan1 := make(chan interface{}, 1) + resolution, err := ctx.registry.NotifyExitHopHtlc( + lntypes.Hash{1}, shardAmt, expiry, testCurrentHeight, + getCircuitKey(1), hodlChan1, nil, htlcPayload1, + ) + require.NoError(t, err) + require.Nil(t, resolution, "did not expect direct resolution") + + htlcPayload2 := &mockPayload{ + mpp: record.NewMPP(testInvoiceAmount, payAddr), + // We are not interested in settling the AMP HTLC so we don't + // use valid shares. + amp: record.NewAMP([32]byte{2}, setID, 2), + } + + // Send htlc 2 which should be added to the invoice as expected. + hodlChan2 := make(chan interface{}, 1) + resolution, err = ctx.registry.NotifyExitHopHtlc( + lntypes.Hash{2}, shardAmt, expiry, testCurrentHeight, + getCircuitKey(2), hodlChan2, nil, htlcPayload2, + ) + require.NoError(t, err) + require.Nil(t, resolution, "did not expect direct resolution") + + // Now time-out the HTLCs. The HoldDuration is 30 seconds after the + // HTLC will be cancelled. + currentTime := ctx.clock.Now() + ctx.clock.SetTime(currentTime.Add(35 * time.Second)) + + // Expect HLTC 1 to be canceled via the MPPTimeout fail resolution. + select { + case resolution := <-hodlChan1: + htlcResolution, _ := resolution.(invpkg.HtlcResolution) + failRes, ok := htlcResolution.(*invpkg.HtlcFailResolution) + require.True( + t, ok, "expected fail resolution, got: %T", resolution, + ) + require.Equal( + t, invpkg.ResultMppTimeout, failRes.Outcome, + "expected MPPTimeout, got: %v", failRes.Outcome, + ) + + case <-time.After(testTimeout): + t.Fatal("timeout waiting for HTLC resolution") + } + + // Expect HLTC 2 to be canceled via the MPPTimeout fail resolution. + select { + case resolution := <-hodlChan2: + htlcResolution, _ := resolution.(invpkg.HtlcResolution) + failRes, ok := htlcResolution.(*invpkg.HtlcFailResolution) + require.True( + t, ok, "expected fail resolution, got: %T", resolution, + ) + require.Equal( + t, invpkg.ResultMppTimeout, failRes.Outcome, + "expected MPPTimeout, got: %v", failRes.Outcome, + ) + + case <-time.After(testTimeout): + t.Fatal("timeout waiting for HTLC resolution") + } + + // The AMP invoice should still be open. + inv, err := ctx.registry.LookupInvoice(ctxb, testInvoicePaymentHash) + require.NoError(t, err) + require.Equal(t, invpkg.ContractOpen, inv.State, "expected "+ + "OPEN invoice") + + // Because one HTLC of the set was cancelled we expect the AMPState to + // be set to canceled. + ampState, ok := inv.AMPState[setID] + require.True(t, ok, "expected AMPState to be set") + require.Equal(t, invpkg.HtlcStateCanceled, ampState.State, "expected "+ + "AMPState CANCELED") + + // The following is a bug and should not be allowed because the sub + // AMP invoice is already marked as canceled. However LND will accept + // other HTLCs to the AMP sub-invoice. + // + // TODO(ziggie): Fix this bug. + htlcPayload3 := &mockPayload{ + mpp: record.NewMPP(testInvoiceAmount, payAddr), + // We are not interested in settling the AMP HTLC so we don't + // use valid shares. + amp: record.NewAMP([32]byte{3}, setID, 3), + } + + // Send htlc 3 which should be added to the invoice as expected. + hodlChan3 := make(chan interface{}, 1) + resolution, err = ctx.registry.NotifyExitHopHtlc( + lntypes.Hash{3}, shardAmt, expiry, testCurrentHeight, + getCircuitKey(3), hodlChan3, nil, htlcPayload3, + ) + require.NoError(t, err) + require.Nil(t, resolution, "did not expect direct resolution") + + // TODO(ziggie): This is a race condition between the invoice being + // cancelled and the htlc being added to the invoice. If we do not wait + // here until the HTLC is added to the invoice, the test might fail + // because the HTLC will not be resolved. + require.Eventuallyf(t, func() bool { + inv, err := ctx.registry.LookupInvoice( + ctxb, testInvoicePaymentHash, + ) + require.NoError(t, err) + + return len(inv.Htlcs) == 3 + }, testTimeout, time.Millisecond*100, "HTLC 3 not added to invoice") + + // Now also let the invoice expire the invoice expiry is 1 hour. + currentTime = ctx.clock.Now() + ctx.clock.SetTime(currentTime.Add(1 * time.Minute)) + + // Expect HLTC 3 to be canceled either via the cancelation of the + // invoice or because the MPP timeout kicks in. + select { + case resolution := <-hodlChan3: + htlcResolution, _ := resolution.(invpkg.HtlcResolution) + failRes, ok := htlcResolution.(*invpkg.HtlcFailResolution) + require.True( + t, ok, "expected fail resolution, got: %T", resolution, + ) + require.Equal( + t, invpkg.ResultMppTimeout, failRes.Outcome, + "expected MPPTimeout, got: %v", failRes.Outcome, + ) + + case <-time.After(testTimeout): + t.Fatal("timeout waiting for HTLC resolution") + } + + // expire the invoice here. + currentTime = ctx.clock.Now() + ctx.clock.SetTime(currentTime.Add(61 * time.Minute)) + + require.Eventuallyf(t, func() bool { + inv, err := ctx.registry.LookupInvoice( + ctxb, testInvoicePaymentHash, + ) + require.NoError(t, err) + + return inv.State == invpkg.ContractCanceled + }, testTimeout, time.Millisecond*100, "invoice not canceled") + + // Fetch the invoice again and compare the number of cancelled HTLCs. + inv, err = ctx.registry.LookupInvoice( + ctxb, testInvoicePaymentHash, + ) + require.NoError(t, err) + + // Make sure all HTLCs are in the cancelled state. + require.Len(t, inv.Htlcs, 3) + for _, htlc := range inv.Htlcs { + require.Equal(t, invpkg.HtlcStateCanceled, htlc.State, + "expected HTLC to be canceled") + } +} diff --git a/invoices/mock.go b/invoices/mock.go index 5d929c2274f..68f5f66dcef 100644 --- a/invoices/mock.go +++ b/invoices/mock.go @@ -86,6 +86,7 @@ func (m *MockInvoiceDB) DeleteCanceledInvoices(ctx context.Context) error { // MockHtlcModifier is a mock implementation of the HtlcModifier interface. type MockHtlcModifier struct { + mock.Mock } // Intercept generates a new intercept session for the given invoice. @@ -94,9 +95,23 @@ type MockHtlcModifier struct { // created in the first place, which is only the case if a client is // registered. func (m *MockHtlcModifier) Intercept( - _ HtlcModifyRequest, _ func(HtlcModifyResponse)) error { + req HtlcModifyRequest, callback func(HtlcModifyResponse)) error { + + // If no expectations are set, return nil by default. + if len(m.ExpectedCalls) == 0 { + return nil + } + + args := m.Called(req, callback) - return nil + // If a response was provided to the mock, execute the callback with it. + if response, ok := args.Get(1).(HtlcModifyResponse); ok && + callback != nil { + + callback(response) + } + + return args.Error(0) } // RegisterInterceptor sets the client callback function that will be diff --git a/invoices/test_utils_test.go b/invoices/test_utils_test.go index 509b92d85cf..6062b3b8080 100644 --- a/invoices/test_utils_test.go +++ b/invoices/test_utils_test.go @@ -207,7 +207,7 @@ func getCircuitKey(htlcID uint64) invpkg.CircuitKey { // Note that this invoice *does not* have a payment address set. It will // create a regular invoice with a preimage is hodl is false, and a hodl // invoice with no preimage otherwise. -func newInvoice(t *testing.T, hodl bool) *invpkg.Invoice { +func newInvoice(t *testing.T, hodl bool, ampInvoice bool) *invpkg.Invoice { invoice := &invpkg.Invoice{ Terms: invpkg.ContractTerm{ Value: testInvoiceAmount, @@ -217,6 +217,23 @@ func newInvoice(t *testing.T, hodl bool) *invpkg.Invoice { CreationDate: testInvoiceCreationDate, } + // This makes the invoice an AMP invoice. We do not support AMP hodl + // invoices. + if ampInvoice { + ampFeature := lnwire.NewRawFeatureVector( + lnwire.TLVOnionPayloadOptional, + lnwire.PaymentAddrOptional, + lnwire.AMPRequired, + ) + + ampFeatures := lnwire.NewFeatureVector( + ampFeature, lnwire.Features, + ) + invoice.Terms.Features = ampFeatures + + return invoice + } + // If creating a hodl invoice, we don't include a preimage. if hodl { invoice.HodlInvoice = true From c95d73c8988f5dc072a252f16bb6f72c36e390c1 Mon Sep 17 00:00:00 2001 From: ziggie Date: Tue, 28 Jan 2025 18:09:40 +0100 Subject: [PATCH 4/8] invoices: remove obsolete code for AMP invoices. We always fetch the HTLCs for the specific setID, so there is no need to keep this code. In earlier versions we would call the UpdateInvoice method with `nil` for the setID therefore we had to lookup the AMPState. However this was error prune because in case one partial payment times-out the AMPState would change to cancelled and that could lead to not resolve HTLCs. --- invoices/invoiceregistry.go | 43 ++++++++++++------------------------- 1 file changed, 14 insertions(+), 29 deletions(-) diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go index 49f0cb0f0c1..90ffa4e263b 100644 --- a/invoices/invoiceregistry.go +++ b/invoices/invoiceregistry.go @@ -654,7 +654,9 @@ func (i *InvoiceRegistry) startHtlcTimer(invoiceRef InvoiceRef, func (i *InvoiceRegistry) cancelSingleHtlc(invoiceRef InvoiceRef, key CircuitKey, result FailResolutionResult) error { - updateInvoice := func(invoice *Invoice) (*InvoiceUpdateDesc, error) { + updateInvoice := func(invoice *Invoice, setID *SetID) ( + *InvoiceUpdateDesc, error) { + // Only allow individual htlc cancellation on open invoices. if invoice.State != ContractOpen { log.Debugf("cancelSingleHtlc: invoice %v no longer "+ @@ -663,37 +665,16 @@ func (i *InvoiceRegistry) cancelSingleHtlc(invoiceRef InvoiceRef, return nil, nil } - // Lookup the current status of the htlc in the database. - var ( - htlcState HtlcState - setID *SetID - ) + // Also for AMP invoices we fetch the relevant HTLCs, so + // the HTLC should be found, otherwise we return an error. htlc, ok := invoice.Htlcs[key] if !ok { - // If this is an AMP invoice, then all the HTLCs won't - // be read out, so we'll consult the other mapping to - // try to find the HTLC state in question here. - var found bool - for ampSetID, htlcSet := range invoice.AMPState { - ampSetID := ampSetID - for htlcKey := range htlcSet.InvoiceKeys { - if htlcKey == key { - htlcState = htlcSet.State - setID = &SetID - - found = true - break - } - } - } - - if !found { - return nil, fmt.Errorf("htlc %v not found", key) - } - } else { - htlcState = htlc.State + return nil, fmt.Errorf("htlc %v not found on "+ + "invoice %v", key, invoiceRef) } + htlcState := htlc.State + // Cancellation is only possible if the htlc wasn't already // resolved. if htlcState != HtlcStateAccepted { @@ -729,7 +710,7 @@ func (i *InvoiceRegistry) cancelSingleHtlc(invoiceRef InvoiceRef, func(invoice *Invoice) ( *InvoiceUpdateDesc, error) { - updateDesc, err := updateInvoice(invoice) + updateDesc, err := updateInvoice(invoice, setID) if err != nil { return nil, err } @@ -756,8 +737,12 @@ func (i *InvoiceRegistry) cancelSingleHtlc(invoiceRef InvoiceRef, key, int32(htlc.AcceptHeight), result, ) + log.Debugf("Cancelling htlc (%v) of invoice(%v) with "+ + "resolution: %v", key, invoiceRef, result) + i.notifyHodlSubscribers(resolution) } + return nil } From 34e56b69e92aa005c311044efbd6cd50c93ecbd6 Mon Sep 17 00:00:00 2001 From: ziggie Date: Tue, 28 Jan 2025 18:18:59 +0100 Subject: [PATCH 5/8] invoicerpc: add clarifying comment. --- lnrpc/invoicesrpc/utils.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lnrpc/invoicesrpc/utils.go b/lnrpc/invoicesrpc/utils.go index 19ade28fd81..096c8305ddc 100644 --- a/lnrpc/invoicesrpc/utils.go +++ b/lnrpc/invoicesrpc/utils.go @@ -187,13 +187,15 @@ func CreateRPCInvoice(invoice *invoices.Invoice, AmtPaidSat: int64(satAmtPaid), AmtPaidMsat: int64(invoice.AmtPaid), AmtPaid: int64(invoice.AmtPaid), - State: state, - Htlcs: rpcHtlcs, - Features: CreateRPCFeatures(invoice.Terms.Features), - IsKeysend: invoice.IsKeysend(), - PaymentAddr: invoice.Terms.PaymentAddr[:], - IsAmp: invoice.IsAMP(), - IsBlinded: invoice.IsBlinded(), + // This will be set to SETTLED if at least one of the AMP Sets + // is settled (see below). + State: state, + Htlcs: rpcHtlcs, + Features: CreateRPCFeatures(invoice.Terms.Features), + IsKeysend: invoice.IsKeysend(), + PaymentAddr: invoice.Terms.PaymentAddr[:], + IsAmp: invoice.IsAMP(), + IsBlinded: invoice.IsBlinded(), } rpcInvoice.AmpInvoiceState = make(map[string]*lnrpc.AMPInvoiceState) From 46f32609242b1f6d550a4747ee296b83e98d6e3a Mon Sep 17 00:00:00 2001 From: ziggie Date: Wed, 29 Jan 2025 01:24:12 +0100 Subject: [PATCH 6/8] invoices: make sure the db uses the same testTime. --- invoices/invoiceregistry_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go index b926260f846..ba110791b67 100644 --- a/invoices/invoiceregistry_test.go +++ b/invoices/invoiceregistry_test.go @@ -122,7 +122,7 @@ func TestInvoiceRegistry(t *testing.T) { makeKeyValueDB := func(t *testing.T) (invpkg.InvoiceDB, *clock.TestClock) { - testClock := clock.NewTestClock(testNow) + testClock := clock.NewTestClock(testTime) db, err := channeldb.MakeTestInvoiceDB( t, channeldb.OptionClock(testClock), ) @@ -156,7 +156,7 @@ func TestInvoiceRegistry(t *testing.T) { }, ) - testClock := clock.NewTestClock(testNow) + testClock := clock.NewTestClock(testTime) return invpkg.NewSQLStore(executor, testClock), testClock } From 15e6e35cdb0c2ce97887eec9fe16820c3237f841 Mon Sep 17 00:00:00 2001 From: ziggie Date: Wed, 29 Jan 2025 18:18:28 +0100 Subject: [PATCH 7/8] invoices: fix log entries and add a TODO. We need to make sure if we cancel an AMP invoice we also cancel all remaining HTLCs back. --- invoices/invoiceregistry.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go index 90ffa4e263b..d9497bf9194 100644 --- a/invoices/invoiceregistry.go +++ b/invoices/invoiceregistry.go @@ -659,8 +659,9 @@ func (i *InvoiceRegistry) cancelSingleHtlc(invoiceRef InvoiceRef, // Only allow individual htlc cancellation on open invoices. if invoice.State != ContractOpen { - log.Debugf("cancelSingleHtlc: invoice %v no longer "+ - "open", invoiceRef) + log.Debugf("CancelSingleHtlc: cannot cancel htlc %v "+ + "on invoice %v, invoice is no longer open", key, + invoiceRef) return nil, nil } @@ -678,13 +679,13 @@ func (i *InvoiceRegistry) cancelSingleHtlc(invoiceRef InvoiceRef, // Cancellation is only possible if the htlc wasn't already // resolved. if htlcState != HtlcStateAccepted { - log.Debugf("cancelSingleHtlc: htlc %v on invoice %v "+ + log.Debugf("CancelSingleHtlc: htlc %v on invoice %v "+ "is already resolved", key, invoiceRef) return nil, nil } - log.Debugf("cancelSingleHtlc: cancelling htlc %v on invoice %v", + log.Debugf("CancelSingleHtlc: cancelling htlc %v on invoice %v", key, invoiceRef) // Return an update descriptor that cancels htlc and keeps @@ -737,8 +738,9 @@ func (i *InvoiceRegistry) cancelSingleHtlc(invoiceRef InvoiceRef, key, int32(htlc.AcceptHeight), result, ) - log.Debugf("Cancelling htlc (%v) of invoice(%v) with "+ - "resolution: %v", key, invoiceRef, result) + log.Debugf("Signaling htlc(%v) cancellation of invoice(%v) "+ + "with resolution(%v) to the link subsystem", key, + invoiceRef, result) i.notifyHodlSubscribers(resolution) } @@ -1457,6 +1459,8 @@ func (i *InvoiceRegistry) cancelInvoiceImpl(ctx context.Context, } invoiceRef := InvoiceRefByHash(payHash) + + // We pass a nil setID which means no HTLCs will be read out. invoice, err := i.idb.UpdateInvoice(ctx, invoiceRef, nil, updateInvoice) // Implement idempotency by returning success if the invoice was already @@ -1483,6 +1487,8 @@ func (i *InvoiceRegistry) cancelInvoiceImpl(ctx context.Context, // that are waiting for resolution. Any htlcs that were already canceled // before, will be notified again. This isn't necessary but doesn't hurt // either. + // + // TODO(ziggie): Also consider AMP HTLCs here. for key, htlc := range invoice.Htlcs { if htlc.State != HtlcStateCanceled { continue From f4e2f2a3963b77053a7e7a8f5daf3987a44d8352 Mon Sep 17 00:00:00 2001 From: ziggie Date: Tue, 28 Jan 2025 18:58:47 +0100 Subject: [PATCH 8/8] docs: add release-notes. --- docs/release-notes/release-notes-0.18.5.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 docs/release-notes/release-notes-0.18.5.md diff --git a/docs/release-notes/release-notes-0.18.5.md b/docs/release-notes/release-notes-0.18.5.md new file mode 100644 index 00000000000..c8e24294bd1 --- /dev/null +++ b/docs/release-notes/release-notes-0.18.5.md @@ -0,0 +1,8 @@ +## Bug Fixes + +* [Improved user experience](https://github.com/lightningnetwork/lnd/pull/9454) + by returning a custom error code when HTLC carries incorrect custom records. + +# Contributors (Alphabetical Order) + +* Ziggie