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.

Author: ziggie1984

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
 	}
 

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

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

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 {