From 0042a1ffeb8812417f467dbb32b14c61117ba062 Mon Sep 17 00:00:00 2001
From: Joost Jager <joost.jager@gmail.com>
Date: Tue, 4 Feb 2020 10:07:56 +0100
Subject: [PATCH] invoices: fix htlc timer deadlock

---
 invoices/invoiceregistry.go | 86 ++++++++++++++++++++++++++++---------
 1 file changed, 65 insertions(+), 21 deletions(-)

diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go
index e83a21b61..d7ac13f7e 100644
--- a/invoices/invoiceregistry.go
+++ b/invoices/invoiceregistry.go
@@ -807,8 +807,53 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash,
 		}
 	}
 
+	// Execute locked notify exit hop logic.
 	i.Lock()
-	defer i.Unlock()
+	resolution, err := i.notifyExitHopHtlcLocked(&updateCtx, hodlChan)
+	i.Unlock()
+	if err != nil {
+		return nil, err
+	}
+
+	switch r := resolution.(type) {
+
+	// A direct resolution was received for this htlc.
+	case *HtlcResolution:
+		return r, nil
+
+	// The htlc is held. Start a timer outside the lock if the htlc should
+	// be auto-released, because otherwise a deadlock may happen with the
+	// main event loop.
+	case *acceptResolution:
+		if r.autoRelease {
+			err := i.startHtlcTimer(rHash, circuitKey, r.acceptTime)
+			if err != nil {
+				return nil, err
+			}
+		}
+
+		return nil, nil
+
+	default:
+		return nil, errors.New("invalid resolution type")
+	}
+}
+
+// acceptResolution is returned when the htlc should be held.
+type acceptResolution struct {
+	// autoRelease signals that the htlc should be automatically released
+	// after a timeout.
+	autoRelease bool
+
+	// acceptTime is the time at which this htlc was accepted.
+	acceptTime time.Time
+}
+
+// notifyExitHopHtlcLocked is the internal implementation of NotifyExitHopHtlc
+// that should be executed inside the registry lock.
+func (i *InvoiceRegistry) notifyExitHopHtlcLocked(
+	ctx *invoiceUpdateCtx, hodlChan chan<- interface{}) (
+	interface{}, error) {
 
 	// We'll attempt to settle an invoice matching this rHash on disk (if
 	// one exists). The callback will update the invoice state and/or htlcs.
@@ -817,11 +862,11 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash,
 		updateSubscribers bool
 	)
 	invoice, err := i.cdb.UpdateInvoice(
-		rHash,
+		ctx.hash,
 		func(inv *channeldb.Invoice) (
 			*channeldb.InvoiceUpdateDesc, error) {
 
-			updateDesc, res, err := updateInvoice(&updateCtx, inv)
+			updateDesc, res, err := updateInvoice(ctx, inv)
 			if err != nil {
 				return nil, err
 			}
@@ -841,29 +886,30 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash,
 		// If the invoice was not found, return a failure resolution
 		// with an invoice not found result.
 		return NewFailureResolution(
-			circuitKey, currentHeight, ResultInvoiceNotFound,
+			ctx.circuitKey, ctx.currentHeight,
+			ResultInvoiceNotFound,
 		), nil
 
 	case nil:
 
 	default:
-		updateCtx.log(err.Error())
+		ctx.log(err.Error())
 		return nil, err
 	}
 
-	updateCtx.log(result.String())
+	ctx.log(result.String())
 
 	if updateSubscribers {
-		i.notifyClients(rHash, invoice, invoice.State)
+		i.notifyClients(ctx.hash, invoice, invoice.State)
 	}
 
 	// Inspect latest htlc state on the invoice.
-	invoiceHtlc, ok := invoice.Htlcs[circuitKey]
+	invoiceHtlc, ok := invoice.Htlcs[ctx.circuitKey]
 
 	// If it isn't recorded, cancel htlc.
 	if !ok {
 		return NewFailureResolution(
-			circuitKey, currentHeight, result,
+			ctx.circuitKey, ctx.currentHeight, result,
 		), nil
 	}
 
@@ -876,7 +922,7 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash,
 	switch invoiceHtlc.State {
 	case channeldb.HtlcStateCanceled:
 		return NewFailureResolution(
-			circuitKey, acceptHeight, result,
+			ctx.circuitKey, acceptHeight, result,
 		), nil
 
 	case channeldb.HtlcStateSettled:
@@ -902,27 +948,25 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash,
 		}
 
 		resolution := NewSettleResolution(
-			invoice.Terms.PaymentPreimage, circuitKey,
+			invoice.Terms.PaymentPreimage, ctx.circuitKey,
 			acceptHeight, result,
 		)
 		return resolution, nil
 
 	case channeldb.HtlcStateAccepted:
-		// (Re)start the htlc timer if the invoice is still open. It can
+		var resolution acceptResolution
+
+		// Auto-release the htlc if the invoice is still open. It can
 		// only happen for mpp payments that there are htlcs in state
 		// Accepted while the invoice is Open.
 		if invoice.State == channeldb.ContractOpen {
-			err := i.startHtlcTimer(
-				rHash, circuitKey,
-				invoiceHtlc.AcceptTime,
-			)
-			if err != nil {
-				return nil, err
-			}
+			resolution.acceptTime = invoiceHtlc.AcceptTime
+			resolution.autoRelease = true
+
 		}
 
-		i.hodlSubscribe(hodlChan, circuitKey)
-		return nil, nil
+		i.hodlSubscribe(hodlChan, ctx.circuitKey)
+		return &resolution, nil
 
 	default:
 		panic("unknown action")