loopin: cancel the swap invoice before the timeout refund

docs/release-notes/release-notes-next.md

@@ -48,6 +48,15 @@
   `loopd` failed with `exec format error` on ARM hosts.
   [Issue #1211](https://github.com/lightninglabs/loop/issues/1211)
 
+* A MuSig2 Loop In no longer reveals the internal key of its HTLC to the
+  server when the swap invoice is canceled. The key is only shared once the
+  invoice is paid.
+
+* A Loop In now cancels its swap invoice before it refunds an expired HTLC,
+  and refunds only once lnd confirmed that the invoice can no longer be
+  paid. Previously the invoice stayed payable until the refund confirmed, so
+  a late payment could settle while the HTLC was being refunded.
+
 #### Maintenance
 
 * Align the standalone `looprpc` module's OpenTelemetry SDK and OTLP trace

loopin.go

@@ -79,6 +79,23 @@ func isInvoiceAlreadySettledError(err error) bool {
 		rpcStatus.Message() == invpkg.ErrInvoiceAlreadySettled.Error()
 }
 
+// isInvoiceNotFoundError returns true if the error reports that lnd does not
+// know the invoice, either as the sentinel itself or as its gRPC form.
+func isInvoiceNotFoundError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	if errors.Is(err, invpkg.ErrInvoiceNotFound) {
+		return true
+	}
+
+	rpcStatus, ok := status.FromError(err)
+	return ok &&
+		rpcStatus.Code() == codes.Unknown &&
+		rpcStatus.Message() == invpkg.ErrInvoiceNotFound.Error()
+}
+
 // loopInSwap contains all the in-memory state related to a pending loop in
 // swap.
 type loopInSwap struct {
@@ -99,6 +116,15 @@ type loopInSwap struct {
 
 	timeoutAddr btcutil.Address
 
+	// invoiceSettled is set once the swap invoice is known to be settled.
+	// Settlement is final, so the flag is never cleared.
+	invoiceSettled bool
+
+	// invoiceCanceled is set once lnd acknowledged the cancellation of the
+	// swap invoice, or no longer knows the invoice. Either way, the server
+	// can no longer pay it.
+	invoiceCanceled bool
+
 	abandonChan chan struct{}
 
 	wg sync.WaitGroup
@@ -904,24 +930,33 @@ func (s *loopInSwap) waitForSwapComplete(ctx context.Context,
 		return fmt.Errorf("subscribe to swap invoice: %v", err)
 	}
 
+	if s.state == loopdb.StateInvoiceSettled ||
+		s.state == loopdb.StateSuccess {
+
+		s.invoiceSettled = true
+	}
+
 	// publishTxOnTimeout publishes the timeout tx if the contract has
-	// expired and invoice has not been settled.
+	// expired and the invoice can no longer be settled.
 	publishTxOnTimeout := func() (btcutil.Amount, error) {
-		// Don't publish the timeout tx if the invoice was settled.
-		if s.state == loopdb.StateInvoiceSettled {
+		// Don't publish the timeout tx if the invoice was settled or
+		// the swap succeeded.
+		if s.invoiceSettled || s.state == loopdb.StateInvoiceSettled ||
+			s.state == loopdb.StateSuccess {
+
 			return 0, nil
 		}
 
-		// Don't publish the timeout tx if the swap succeeded.
-		if s.state == loopdb.StateSuccess {
+		if s.height < s.LoopInContract.CltvExpiry {
 			return 0, nil
 		}
 
-		if s.height >= s.LoopInContract.CltvExpiry {
-			return s.publishTimeoutTx(ctx, htlcOutpoint, htlcValue)
+		refund, err := s.authorizeRefund(ctx)
+		if err != nil || !refund {
+			return 0, err
 		}
 
-		return 0, nil
+		return s.publishTimeoutTx(ctx, htlcOutpoint, htlcValue)
 	}
 
 	// Check timeout at current height. After a restart we may want to
@@ -936,8 +971,9 @@ func (s *loopInSwap) waitForSwapComplete(ctx context.Context,
 	invoiceFinalized := false
 	htlcKeyRevealed := false
 	for {
-		// Check stop conditions.
-		if htlcSpend && invoiceFinalized {
+		// Check stop conditions. A canceled invoice is final even if
+		// lnd deleted it and never reports the cancellation.
+		if htlcSpend && (invoiceFinalized || s.invoiceCanceled) {
 			break
 		}
 		if s.state == loopdb.StateInvoiceSettled {
@@ -967,7 +1003,7 @@ func (s *loopInSwap) waitForSwapComplete(ctx context.Context,
 				return err
 			}
 
-			if invoiceFinalized && !htlcKeyRevealed {
+			if s.invoiceSettled && !htlcKeyRevealed {
 				htlcKeyRevealed = s.tryPushHtlcKey(ctx)
 			}
 
@@ -1030,6 +1066,7 @@ func (s *loopInSwap) waitForSwapComplete(ctx context.Context,
 				}
 
 				invoiceFinalized = true
+				s.invoiceSettled = true
 				htlcKeyRevealed = s.tryPushHtlcKey(ctx)
 				s.cost.Server = s.AmountRequested -
 					update.AmtPaid
@@ -1048,14 +1085,79 @@ func (s *loopInSwap) waitForSwapComplete(ctx context.Context,
 	return nil
 }
 
+// authorizeRefund makes sure that the swap invoice can no longer be settled
+// before the expired htlc is refunded, and reports whether the refund may
+// proceed.
+//
+// Once the htlc expired, the server must not be able to pay the invoice
+// anymore: a late payment would give the server the preimage while the client
+// takes back the htlc. lnd's CancelInvoice resolves the race with settlement
+// atomically. It succeeds for an open or already canceled invoice and fails
+// for a settled one, which then blocks the refund. lnd only deletes canceled
+// invoices, so an invoice that it no longer knows can't be settled either,
+// for example one that its garbage collection removed after an earlier
+// cancellation. Any other error leaves the outcome unresolved, so the refund
+// waits for a later attempt.
+func (s *loopInSwap) authorizeRefund(ctx context.Context) (bool, error) {
+	if s.invoiceCanceled {
+		return true, nil
+	}
+
+	err := s.lnd.Invoices.CancelInvoice(ctx, s.hash)
+	switch {
+	case err == nil, isInvoiceNotFoundError(err):
+
+	case isInvoiceAlreadySettledError(err):
+		s.log.Infof("Swap invoice settled before the refund, not " +
+			"refunding the htlc")
+
+		// The swap can complete before the invoice update that
+		// reports the paid amount arrives, so take the amount from the
+		// invoice itself.
+		invoice, lookupErr := s.lnd.Client.LookupInvoice(ctx, s.hash)
+		if lookupErr != nil {
+			s.log.Warnf("Unable to look up the paid amount of the "+
+				"settled swap invoice: %v", lookupErr)
+		} else {
+			s.cost.Server = s.AmountRequested -
+				invoice.AmountPaid.ToSatoshis()
+		}
+
+		s.invoiceSettled = true
+		if s.state == loopdb.StateHtlcPublished {
+			s.setState(loopdb.StateInvoiceSettled)
+			return false, s.persistAndAnnounceState(ctx)
+		}
+
+		return false, nil
+
+	default:
+		s.log.Warnf("Unable to cancel the swap invoice before the "+
+			"refund, retrying at the next block: %v", err)
+
+		return false, nil
+	}
+
+	s.invoiceCanceled = true
+
+	return true, nil
+}
+
 // tryPushHtlcKey attempts to push the htlc key to the server. If the server
 // returns an error of any kind we'll log it as a warning but won't act as the
 // swap execution can just go on without the server gaining knowledge of our
 // internal key.
+//
+// The internal key lets the server spend the htlc through its key path, so it
+// is only revealed after the swap invoice was settled. For a canceled invoice
+// the key must stay secret while the htlc can still be spent.
 func (s *loopInSwap) tryPushHtlcKey(ctx context.Context) bool {
 	if s.ProtocolVersion < loopdb.ProtocolVersionMuSig2 {
 		return false
 	}
+	if !s.invoiceSettled {
+		return false
+	}
 
 	log.Infof("Attempting to reveal internal HTLC key to the server")
 
@@ -1106,9 +1208,14 @@ func (s *loopInSwap) processHtlcSpend(ctx context.Context,
 		// swap invoice. We still need to query the final invoice state.
 		// This is not a hodl invoice, so it may be that the invoice was
 		// already settled. This means that the server didn't succeed in
-		// sweeping the htlc after paying the invoice.
+		// sweeping the htlc after paying the invoice. An invoice that
+		// lnd no longer knows was canceled before the refund.
 		err := s.lnd.Invoices.CancelInvoice(ctx, s.hash)
-		if err != nil && !isInvoiceAlreadySettledError(err) {
+		switch {
+		case err == nil, isInvoiceNotFoundError(err):
+			s.invoiceCanceled = true
+
+		case !isInvoiceAlreadySettledError(err):
 			return err
 		}
 	}