From 4141773e90e7f9310cde4521f1dcee68561431ed Mon Sep 17 00:00:00 2001
From: Joost Jager <joost.jager@gmail.com>
Date: Mon, 18 Feb 2019 21:27:54 +0100
Subject: [PATCH] htlcswitch: resolve invoice cancelation race condition

Previously it could happen that an invoice was open at the time of the
LookupInvoice call, the htlc was settled because of that, but when the
SettleInvoice call was made eventually, it would fail because the
invoice was canceled in the mean time. The htlc would then be settled,
but the invoice not marked as such.
---
 htlcswitch/link.go | 52 ++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 20 deletions(-)

diff --git a/htlcswitch/link.go b/htlcswitch/link.go
index 68fac720..b432eec0 100644
--- a/htlcswitch/link.go
+++ b/htlcswitch/link.go
@@ -2555,6 +2555,10 @@ func (l *channelLink) processExitHop(pd *lnwallet.PaymentDescriptor,
 	// We're the designated payment destination.  Therefore we attempt to
 	// see if we have an invoice locally which'll allow us to settle this
 	// htlc.
+	//
+	// Only the immutable data from LookupInvoice is used, because otherwise
+	// a race condition may be created with concurrent writes to the invoice
+	// registry. For example: cancelation of an invoice.
 	invoiceHash := lntypes.Hash(pd.RHash)
 	invoice, minCltvDelta, err := l.cfg.Registry.LookupInvoice(invoiceHash)
 	if err != nil {
@@ -2565,16 +2569,6 @@ func (l *channelLink) processExitHop(pd *lnwallet.PaymentDescriptor,
 		return true, nil
 	}
 
-	// Reject htlcs for canceled invoices.
-	if invoice.Terms.State == channeldb.ContractCanceled {
-		l.errorf("Rejecting htlc due to canceled invoice")
-
-		failure := lnwire.NewFailUnknownPaymentHash(pd.Amount)
-		l.sendHTLCError(pd.HtlcIndex, failure, obfuscator, pd.SourceRef)
-
-		return true, nil
-	}
-
 	// If the invoice is already settled, we choose to accept the payment to
 	// simplify failure recovery.
 	//
@@ -2588,7 +2582,13 @@ func (l *channelLink) processExitHop(pd *lnwallet.PaymentDescriptor,
 	//
 	// TODO(conner): track ownership of settlements to properly recover from
 	// failures? or add batch invoice settlement
-	if invoice.Terms.State != channeldb.ContractOpen {
+	//
+	// TODO(joostjager): The log statement below is not always accurate, as
+	// the invoice may have been canceled after the LookupInvoice call.
+	// Leaving it as is for now, because fixing this would involve changing
+	// the signature of InvoiceRegistry.SettleInvoice just because of this
+	// log statement.
+	if invoice.Terms.State == channeldb.ContractSettled {
 		log.Warnf("Accepting duplicate payment for hash=%x",
 			pd.RHash[:])
 	}
@@ -2661,6 +2661,27 @@ func (l *channelLink) processExitHop(pd *lnwallet.PaymentDescriptor,
 		return true, nil
 	}
 
+	// Notify the invoiceRegistry of the invoices we just settled (with the
+	// amount accepted at settle time) with this latest commitment update.
+	err = l.cfg.Registry.SettleInvoice(invoiceHash, pd.Amount)
+
+	// Reject htlcs for canceled invoices.
+	if err == channeldb.ErrInvoiceAlreadyCanceled {
+		l.errorf("Rejecting htlc due to canceled invoice")
+
+		failure := lnwire.NewFailUnknownPaymentHash(pd.Amount)
+		l.sendHTLCError(pd.HtlcIndex, failure, obfuscator, pd.SourceRef)
+
+		return true, nil
+	}
+
+	// Handle unexpected errors.
+	if err != nil {
+		return false, fmt.Errorf("unable to settle invoice: %v", err)
+	}
+
+	l.infof("settling %x as exit hop", pd.RHash)
+
 	preimage := invoice.Terms.PaymentPreimage
 	err = l.channel.SettleHTLC(
 		preimage, pd.HtlcIndex, pd.SourceRef, nil, nil,
@@ -2669,15 +2690,6 @@ func (l *channelLink) processExitHop(pd *lnwallet.PaymentDescriptor,
 		return false, fmt.Errorf("unable to settle htlc: %v", err)
 	}
 
-	// Notify the invoiceRegistry of the invoices we just settled (with the
-	// amount accepted at settle time) with this latest commitment update.
-	err = l.cfg.Registry.SettleInvoice(invoiceHash, pd.Amount)
-	if err != nil {
-		return false, fmt.Errorf("unable to settle invoice: %v", err)
-	}
-
-	l.infof("settling %x as exit hop", pd.RHash)
-
 	// If the link is in hodl.BogusSettle mode, replace the preimage with a
 	// fake one before sending it to the peer.
 	if l.cfg.DebugHTLC && l.cfg.HodlMask.Active(hodl.BogusSettle) {