From fff6f83f23b44e406f5a20427e8dd38d1e493b08 Mon Sep 17 00:00:00 2001
From: carla <kirkcohenc@gmail.com>
Date: Fri, 23 Apr 2021 08:19:47 +0200
Subject: [PATCH 01/10] itest/test: update remote force close timeout to use
 hodl inv

This commit updates our multi-hop force close test to use a hodl
invoice so that we can reproduce some bugs which will require
the preimage for the invoice that is timed out on chain.
---
 ..._force_close_on_chain_htlc_timeout_test.go | 27 ++++++++++++-------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/lntest/itest/lnd_multi-hop_remote_force_close_on_chain_htlc_timeout_test.go b/lntest/itest/lnd_multi-hop_remote_force_close_on_chain_htlc_timeout_test.go
index e79cf7c34..f6f40490d 100644
--- a/lntest/itest/lnd_multi-hop_remote_force_close_on_chain_htlc_timeout_test.go
+++ b/lntest/itest/lnd_multi-hop_remote_force_close_on_chain_htlc_timeout_test.go
@@ -5,9 +5,11 @@ import (
 	"fmt"
 
 	"github.com/btcsuite/btcutil"
+	"github.com/lightningnetwork/lnd/lnrpc/invoicesrpc"
 	"github.com/lightningnetwork/lnd/lnrpc/routerrpc"
 	"github.com/lightningnetwork/lnd/lntest"
 	"github.com/lightningnetwork/lnd/lntest/wait"
+	"github.com/lightningnetwork/lnd/lntypes"
 	"github.com/stretchr/testify/require"
 )
 
@@ -43,14 +45,21 @@ func testMultiHopRemoteForceCloseOnChainHtlcTimeout(net *lntest.NetworkHarness,
 	defer cancel()
 
 	// We'll now send a single HTLC across our multi-hop network.
-	carolPubKey := carol.PubKey[:]
-	payHash := makeFakePayHash(t)
-	_, err := alice.RouterClient.SendPaymentV2(
+	preimage := lntypes.Preimage{1, 2, 3}
+	payHash := preimage.Hash()
+	invoiceReq := &invoicesrpc.AddHoldInvoiceRequest{
+		Value:      int64(htlcAmt),
+		CltvExpiry: 40,
+		Hash:       payHash[:],
+	}
+
+	ctxt, _ := context.WithTimeout(ctxb, defaultTimeout)
+	carolInvoice, err := carol.AddHoldInvoice(ctxt, invoiceReq)
+	require.NoError(t.t, err)
+
+	_, err = alice.RouterClient.SendPaymentV2(
 		ctx, &routerrpc.SendPaymentRequest{
-			Dest:           carolPubKey,
-			Amt:            int64(htlcAmt),
-			PaymentHash:    payHash,
-			FinalCltvDelta: finalCltvDelta,
+			PaymentRequest: carolInvoice.PaymentRequest,
 			TimeoutSeconds: 60,
 			FeeLimitMsat:   noFeeLimitMsat,
 		},
@@ -61,7 +70,7 @@ func testMultiHopRemoteForceCloseOnChainHtlcTimeout(net *lntest.NetworkHarness,
 	// show that the HTLC has been locked in.
 	nodes := []*lntest.HarnessNode{alice, bob, carol}
 	err = wait.NoError(func() error {
-		return assertActiveHtlcs(nodes, payHash)
+		return assertActiveHtlcs(nodes, payHash[:])
 	}, defaultTimeout)
 	require.NoError(t.t, err)
 
@@ -73,7 +82,7 @@ func testMultiHopRemoteForceCloseOnChainHtlcTimeout(net *lntest.NetworkHarness,
 	// transaction. This will let us exercise that Bob is able to sweep the
 	// expired HTLC on Carol's version of the commitment transaction. If
 	// Carol has an anchor, it will be swept too.
-	ctxt, _ := context.WithTimeout(ctxb, channelCloseTimeout)
+	ctxt, _ = context.WithTimeout(ctxb, channelCloseTimeout)
 	closeChannelAndAssertType(
 		ctxt, t, net, carol, bobChanPoint, c == commitTypeAnchors,
 		true,

From 16373d387984b7725db0c1031bcfeb935b57b2c4 Mon Sep 17 00:00:00 2001
From: carla <kirkcohenc@gmail.com>
Date: Fri, 23 Apr 2021 08:19:50 +0200
Subject: [PATCH 02/10] itest/test: add test to reproduce settling timed out
 invoice

Reproduce the case where we allow settling of invoices that have
htlcs that have actually timed out on chain. This bug can rarely
occur if a hodl invoice goes to chain and is manually settled
after it has timed out. Funds are SAFU, but this could be a
headache because the invoice says it's settled when no funds
were claimed.
---
 ..._force_close_on_chain_htlc_timeout_test.go | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/lntest/itest/lnd_multi-hop_remote_force_close_on_chain_htlc_timeout_test.go b/lntest/itest/lnd_multi-hop_remote_force_close_on_chain_htlc_timeout_test.go
index f6f40490d..b8dee8221 100644
--- a/lntest/itest/lnd_multi-hop_remote_force_close_on_chain_htlc_timeout_test.go
+++ b/lntest/itest/lnd_multi-hop_remote_force_close_on_chain_htlc_timeout_test.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 
 	"github.com/btcsuite/btcutil"
+	"github.com/lightningnetwork/lnd/lnrpc"
 	"github.com/lightningnetwork/lnd/lnrpc/invoicesrpc"
 	"github.com/lightningnetwork/lnd/lnrpc/routerrpc"
 	"github.com/lightningnetwork/lnd/lntest"
@@ -177,6 +178,10 @@ func testMultiHopRemoteForceCloseOnChainHtlcTimeout(net *lntest.NetworkHarness,
 	err = waitForNumChannelPendingForceClose(ctxt, bob, 0, nil)
 	require.NoError(t.t, err)
 
+	// While we're here, we demonstrate some bugs in our handling of
+	// invoices that timeout on chain.
+	assertOnChainInvoiceState(ctxb, t, carol, preimage)
+
 	// We'll close out the test by closing the channel from Alice to Bob,
 	// and then shutting down the new node we created as its no longer
 	// needed. Coop close, no anchors.
@@ -185,3 +190,39 @@ func testMultiHopRemoteForceCloseOnChainHtlcTimeout(net *lntest.NetworkHarness,
 		ctxt, t, net, alice, aliceChanPoint, false, false,
 	)
 }
+
+// assertOnChainInvoiceState asserts that we have some bugs with how we handle
+// hold invoices that are expired on-chain.
+// - htlcs accepted: despite being timed out, our htlcs are still in accepted
+//   state
+// - can settle: our invoice that has expired on-chain can still be settled
+//   even though we don't claim any htlcs.
+func assertOnChainInvoiceState(ctx context.Context, t *harnessTest,
+	node *lntest.HarnessNode, preimage lntypes.Preimage) {
+
+	hash := preimage.Hash()
+	inv, err := node.LookupInvoice(ctx, &lnrpc.PaymentHash{
+		RHash: hash[:],
+	})
+	require.NoError(t.t, err)
+
+	for _, htlc := range inv.Htlcs {
+		require.Equal(t.t, lnrpc.InvoiceHTLCState_ACCEPTED, htlc.State)
+	}
+
+	_, err = node.SettleInvoice(ctx, &invoicesrpc.SettleInvoiceMsg{
+		Preimage: preimage[:],
+	})
+	require.NoError(t.t, err, "expected erroneous invoice settle")
+
+	inv, err = node.LookupInvoice(ctx, &lnrpc.PaymentHash{
+		RHash: hash[:],
+	})
+	require.NoError(t.t, err)
+
+	require.True(t.t, inv.Settled, "expected erroneously settled invoice")
+	for _, htlc := range inv.Htlcs {
+		require.Equal(t.t, lnrpc.InvoiceHTLCState_SETTLED, htlc.State,
+			"expected htlcs to be erroneously settled")
+	}
+}

From 2e39edd6bd4307c878b0ae7c9c79b0f95ca2a154 Mon Sep 17 00:00:00 2001
From: carla <kirkcohenc@gmail.com>
Date: Fri, 23 Apr 2021 08:19:51 +0200
Subject: [PATCH 03/10] itest/test: add test for expired hold invoices

This commit adds a test for a hold invoice which is accepted
off-chain, and held by the recipient until it expired and
the payer force-closes the channel. With this test we
demonstrate two bugs in our handling of hold invoice state
in the invoice registry when we expire on chain:
- Htlcs not updated: even when we've timed out, we don't
  update the htlc state accordingly.
- Invoice can be settled: the invoice can be settled even
  though it's expired on chain.
---
 lntest/itest/lnd_hold_invoice_force_test.go | 131 ++++++++++++++++++++
 lntest/itest/lnd_test.go                    |   5 +-
 lntest/itest/lnd_test_list_on_test.go       |   4 +
 3 files changed, 138 insertions(+), 2 deletions(-)
 create mode 100644 lntest/itest/lnd_hold_invoice_force_test.go

diff --git a/lntest/itest/lnd_hold_invoice_force_test.go b/lntest/itest/lnd_hold_invoice_force_test.go
new file mode 100644
index 000000000..008310004
--- /dev/null
+++ b/lntest/itest/lnd_hold_invoice_force_test.go
@@ -0,0 +1,131 @@
+package itest
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/lightningnetwork/lnd/lncfg"
+	"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/wait"
+	"github.com/lightningnetwork/lnd/lntypes"
+	"github.com/stretchr/testify/require"
+)
+
+// testHoldInvoiceForceClose demonstrates that recipients of hold invoices
+// will not release active htlcs for their own invoices when they expire,
+// resulting in a force close of their channel.
+func testHoldInvoiceForceClose(net *lntest.NetworkHarness, t *harnessTest) {
+	ctxb, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	// Open a channel between alice and bob.
+	chanReq := lntest.OpenChannelParams{
+		Amt: 300000,
+	}
+
+	ctxt, _ := context.WithTimeout(ctxb, channelOpenTimeout)
+	chanPoint := openChannelAndAssert(ctxt, t, net, net.Alice, net.Bob, chanReq)
+
+	// Create a non-dust hold invoice for bob.
+	var (
+		preimage = lntypes.Preimage{1, 2, 3}
+		payHash  = preimage.Hash()
+	)
+	invoiceReq := &invoicesrpc.AddHoldInvoiceRequest{
+		Value:      30000,
+		CltvExpiry: 40,
+		Hash:       payHash[:],
+	}
+
+	ctxt, _ = context.WithTimeout(ctxb, defaultTimeout)
+	bobInvoice, err := net.Bob.AddHoldInvoice(ctxt, invoiceReq)
+	require.NoError(t.t, err)
+
+	// Pay this invoice from Alice -> Bob, we should achieve this with a
+	// single htlc.
+	_, err = net.Alice.RouterClient.SendPaymentV2(
+		ctxb, &routerrpc.SendPaymentRequest{
+			PaymentRequest: bobInvoice.PaymentRequest,
+			TimeoutSeconds: 60,
+			FeeLimitMsat:   noFeeLimitMsat,
+		},
+	)
+	require.NoError(t.t, err)
+
+	waitForInvoiceAccepted(t, net.Bob, payHash)
+
+	// Once the HTLC has cleared, alice and bob should both have a single
+	// htlc locked in.
+	nodes := []*lntest.HarnessNode{net.Alice, net.Bob}
+	err = wait.NoError(func() error {
+		return assertActiveHtlcs(nodes, payHash[:])
+	}, defaultTimeout)
+	require.NoError(t.t, err)
+
+	// Get our htlc expiry height and current block height so that we
+	// can mine the exact number of blocks required to expire the htlc.
+	chans, err := net.Alice.ListChannels(ctxb, &lnrpc.ListChannelsRequest{})
+	require.NoError(t.t, err)
+	require.Len(t.t, chans.Channels, 1)
+	require.Len(t.t, chans.Channels[0].PendingHtlcs, 1)
+	activeHtlc := chans.Channels[0].PendingHtlcs[0]
+
+	info, err := net.Alice.GetInfo(ctxb, &lnrpc.GetInfoRequest{})
+	require.NoError(t.t, err)
+
+	// Now we will mine blocks until the htlc expires, and wait for each
+	// node to sync to our latest height. Sanity check that we won't
+	// underflow.
+	require.Greater(t.t, activeHtlc.ExpirationHeight, info.BlockHeight,
+		"expected expiry after current height")
+	blocksTillExpiry := activeHtlc.ExpirationHeight - info.BlockHeight
+
+	// Alice will go to chain with some delta, sanity check that we won't
+	// underflow and subtract this from our mined blocks.
+	require.Greater(t.t, blocksTillExpiry,
+		uint32(lncfg.DefaultOutgoingBroadcastDelta))
+	blocksTillForce := blocksTillExpiry - lncfg.DefaultOutgoingBroadcastDelta
+
+	mineBlocks(t, net, blocksTillForce, 0)
+
+	require.NoError(t.t, net.Alice.WaitForBlockchainSync(ctxb))
+	require.NoError(t.t, net.Bob.WaitForBlockchainSync(ctxb))
+
+	// Alice should have a waiting-close channel because she has force
+	// closed to time out the htlc.
+	assertNumPendingChannels(t, net.Alice, 1, 0)
+
+	// We should have our force close tx in the mempool.
+	mineBlocks(t, net, 1, 1)
+
+	// Ensure alice and bob are synced to chain after we've mined our force
+	// close.
+	require.NoError(t.t, net.Alice.WaitForBlockchainSync(ctxb))
+	require.NoError(t.t, net.Bob.WaitForBlockchainSync(ctxb))
+
+	// At this point, Bob's channel should be resolved because his htlc is
+	// expired, so no further action is required. Alice will still have a
+	// pending force close channel because she needs to resolve the htlc.
+	assertNumPendingChannels(t, net.Alice, 0, 1)
+	assertNumPendingChannels(t, net.Bob, 0, 0)
+
+	ctxt, _ = context.WithTimeout(ctxb, defaultTimeout)
+	err = waitForNumChannelPendingForceClose(ctxt, net.Alice, 1,
+		func(channel *lnrpcForceCloseChannel) error {
+			numHtlcs := len(channel.PendingHtlcs)
+			if numHtlcs != 1 {
+				return fmt.Errorf("expected 1 htlc, got: "+
+					"%v", numHtlcs)
+			}
+
+			return nil
+		},
+	)
+	require.NoError(t.t, err)
+
+	// Cleanup Alice's force close.
+	cleanupForceClose(t, net, net.Alice, chanPoint)
+}
diff --git a/lntest/itest/lnd_test.go b/lntest/itest/lnd_test.go
index 666b66415..bacc4bea3 100644
--- a/lntest/itest/lnd_test.go
+++ b/lntest/itest/lnd_test.go
@@ -440,8 +440,9 @@ func waitForNumChannelPendingForceClose(ctx context.Context,
 
 		forceCloseChans := resp.PendingForceClosingChannels
 		if len(forceCloseChans) != expectedNum {
-			return fmt.Errorf("bob should have %d pending "+
-				"force close channels but has %d", expectedNum,
+			return fmt.Errorf("%v should have %d pending "+
+				"force close channels but has %d",
+				node.Cfg.Name, expectedNum,
 				len(forceCloseChans))
 		}
 
diff --git a/lntest/itest/lnd_test_list_on_test.go b/lntest/itest/lnd_test_list_on_test.go
index 7f630c0df..679c572ff 100644
--- a/lntest/itest/lnd_test_list_on_test.go
+++ b/lntest/itest/lnd_test_list_on_test.go
@@ -230,6 +230,10 @@ var allTestCases = []*testCase{
 		name: "hold invoice sender persistence",
 		test: testHoldInvoicePersistence,
 	},
+	{
+		name: "hold invoice force close",
+		test: testHoldInvoiceForceClose,
+	},
 	{
 		name: "cpfp",
 		test: testCPFP,

From f5f1e9e6c7da0bafb11662f80366cb77e70bd894 Mon Sep 17 00:00:00 2001
From: carla <kirkcohenc@gmail.com>
Date: Fri, 23 Apr 2021 08:19:52 +0200
Subject: [PATCH 04/10] invoices: refactor - rename invoiceExpiry to
 invoiceExpiryTs

We're going to add block based expiry, so we clarify now.
---
 invoices/invoice_expiry_watcher.go      | 26 ++++++++++++-------------
 invoices/invoice_expiry_watcher_test.go |  2 +-
 invoices/invoiceregistry.go             |  2 +-
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/invoices/invoice_expiry_watcher.go b/invoices/invoice_expiry_watcher.go
index 4c7f8f6e2..2c22c9161 100644
--- a/invoices/invoice_expiry_watcher.go
+++ b/invoices/invoice_expiry_watcher.go
@@ -12,9 +12,9 @@ import (
 	"github.com/lightningnetwork/lnd/zpay32"
 )
 
-// invoiceExpiry holds and invoice's payment hash and its expiry. This
-// is used to order invoices by their expiry for cancellation.
-type invoiceExpiry struct {
+// invoiceExpiryTs holds and invoice's payment hash and its expiry. This
+// is used to order invoices by their expiry time for cancellation.
+type invoiceExpiryTs struct {
 	PaymentHash lntypes.Hash
 	Expiry      time.Time
 	Keysend     bool
@@ -22,8 +22,8 @@ type invoiceExpiry struct {
 
 // Less implements PriorityQueueItem.Less such that the top item in the
 // priorty queue will be the one that expires next.
-func (e invoiceExpiry) Less(other queue.PriorityQueueItem) bool {
-	return e.Expiry.Before(other.(*invoiceExpiry).Expiry)
+func (e invoiceExpiryTs) Less(other queue.PriorityQueueItem) bool {
+	return e.Expiry.Before(other.(*invoiceExpiryTs).Expiry)
 }
 
 // InvoiceExpiryWatcher handles automatic invoice cancellation of expried
@@ -50,7 +50,7 @@ type InvoiceExpiryWatcher struct {
 
 	// newInvoices channel is used to wake up the main loop when a new
 	// invoices is added.
-	newInvoices chan []*invoiceExpiry
+	newInvoices chan []*invoiceExpiryTs
 
 	wg sync.WaitGroup
 
@@ -62,7 +62,7 @@ type InvoiceExpiryWatcher struct {
 func NewInvoiceExpiryWatcher(clock clock.Clock) *InvoiceExpiryWatcher {
 	return &InvoiceExpiryWatcher{
 		clock:       clock,
-		newInvoices: make(chan []*invoiceExpiry),
+		newInvoices: make(chan []*invoiceExpiryTs),
 		quit:        make(chan struct{}),
 	}
 }
@@ -107,7 +107,7 @@ func (ew *InvoiceExpiryWatcher) Stop() {
 // the expiry time and creates a slimmer invoiceExpiry object with the hash and
 // expiry time.
 func makeInvoiceExpiry(paymentHash lntypes.Hash,
-	invoice *channeldb.Invoice) *invoiceExpiry {
+	invoice *channeldb.Invoice) *invoiceExpiryTs {
 
 	if invoice.State != channeldb.ContractOpen {
 		log.Debugf("Invoice not added to expiry watcher: %v",
@@ -121,7 +121,7 @@ func makeInvoiceExpiry(paymentHash lntypes.Hash,
 	}
 
 	expiry := invoice.CreationDate.Add(realExpiry)
-	return &invoiceExpiry{
+	return &invoiceExpiryTs{
 		PaymentHash: paymentHash,
 		Expiry:      expiry,
 		Keysend:     len(invoice.PaymentRequest) == 0,
@@ -129,7 +129,7 @@ func makeInvoiceExpiry(paymentHash lntypes.Hash,
 }
 
 // AddInvoices adds invoices to the InvoiceExpiryWatcher.
-func (ew *InvoiceExpiryWatcher) AddInvoices(invoices ...*invoiceExpiry) {
+func (ew *InvoiceExpiryWatcher) AddInvoices(invoices ...*invoiceExpiryTs) {
 	if len(invoices) > 0 {
 		select {
 		case ew.newInvoices <- invoices:
@@ -147,7 +147,7 @@ func (ew *InvoiceExpiryWatcher) AddInvoices(invoices ...*invoiceExpiry) {
 // If there are no active invoices, then it'll simply wait indefinitely.
 func (ew *InvoiceExpiryWatcher) nextExpiry() <-chan time.Time {
 	if !ew.expiryQueue.Empty() {
-		top := ew.expiryQueue.Top().(*invoiceExpiry)
+		top := ew.expiryQueue.Top().(*invoiceExpiryTs)
 		return ew.clock.TickAfter(top.Expiry.Sub(ew.clock.Now()))
 	}
 
@@ -158,7 +158,7 @@ func (ew *InvoiceExpiryWatcher) nextExpiry() <-chan time.Time {
 // it from the expiry queue.
 func (ew *InvoiceExpiryWatcher) cancelNextExpiredInvoice() {
 	if !ew.expiryQueue.Empty() {
-		top := ew.expiryQueue.Top().(*invoiceExpiry)
+		top := ew.expiryQueue.Top().(*invoiceExpiryTs)
 		if !top.Expiry.Before(ew.clock.Now()) {
 			return
 		}
@@ -190,7 +190,7 @@ func (ew *InvoiceExpiryWatcher) mainLoop() {
 		// Cancel any invoices that may have expired.
 		ew.cancelNextExpiredInvoice()
 
-		pushInvoices := func(invoicesWithExpiry []*invoiceExpiry) {
+		pushInvoices := func(invoicesWithExpiry []*invoiceExpiryTs) {
 			for _, invoiceWithExpiry := range invoicesWithExpiry {
 				// Avoid pushing nil object to the heap.
 				if invoiceWithExpiry != nil {
diff --git a/invoices/invoice_expiry_watcher_test.go b/invoices/invoice_expiry_watcher_test.go
index a06bde53d..c5b5e518e 100644
--- a/invoices/invoice_expiry_watcher_test.go
+++ b/invoices/invoice_expiry_watcher_test.go
@@ -157,7 +157,7 @@ func TestInvoiceExpiryWhenAddingMultipleInvoices(t *testing.T) {
 	t.Parallel()
 
 	test := newInvoiceExpiryWatcherTest(t, testTime, 5, 5)
-	var invoices []*invoiceExpiry
+	var invoices []*invoiceExpiryTs
 
 	for hash, invoice := range test.testData.expiredInvoices {
 		invoices = append(invoices, makeInvoiceExpiry(hash, invoice))
diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go
index bb336be7c..24eb6ef9b 100644
--- a/invoices/invoiceregistry.go
+++ b/invoices/invoiceregistry.go
@@ -160,7 +160,7 @@ func NewRegistry(cdb *channeldb.DB, expiryWatcher *InvoiceExpiryWatcher,
 // invoices.
 func (i *InvoiceRegistry) scanInvoicesOnStart() error {
 	var (
-		pending   []*invoiceExpiry
+		pending   []*invoiceExpiryTs
 		removable []channeldb.InvoiceDeleteRef
 	)
 

From 9c6e83b15f731ac6168d7e0f51113c144d3369e7 Mon Sep 17 00:00:00 2001
From: carla <kirkcohenc@gmail.com>
Date: Fri, 23 Apr 2021 08:19:53 +0200
Subject: [PATCH 05/10] invoices: refactor - rename expiry queue to be
 timestamp specific

---
 invoices/invoice_expiry_watcher.go | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/invoices/invoice_expiry_watcher.go b/invoices/invoice_expiry_watcher.go
index 2c22c9161..21bb5d08e 100644
--- a/invoices/invoice_expiry_watcher.go
+++ b/invoices/invoice_expiry_watcher.go
@@ -44,9 +44,9 @@ type InvoiceExpiryWatcher struct {
 	// cancelInvoice is a template method that cancels an expired invoice.
 	cancelInvoice func(lntypes.Hash, bool) error
 
-	// expiryQueue holds invoiceExpiry items and is used to find the next
-	// invoice to expire.
-	expiryQueue queue.PriorityQueue
+	// timestampExpiryQueue holds invoiceExpiry items and is used to find
+	// the next invoice to expire.
+	timestampExpiryQueue queue.PriorityQueue
 
 	// newInvoices channel is used to wake up the main loop when a new
 	// invoices is added.
@@ -146,8 +146,8 @@ func (ew *InvoiceExpiryWatcher) AddInvoices(invoices ...*invoiceExpiryTs) {
 // nextExpiry returns a Time chan to wait on until the next invoice expires.
 // If there are no active invoices, then it'll simply wait indefinitely.
 func (ew *InvoiceExpiryWatcher) nextExpiry() <-chan time.Time {
-	if !ew.expiryQueue.Empty() {
-		top := ew.expiryQueue.Top().(*invoiceExpiryTs)
+	if !ew.timestampExpiryQueue.Empty() {
+		top := ew.timestampExpiryQueue.Top().(*invoiceExpiryTs)
 		return ew.clock.TickAfter(top.Expiry.Sub(ew.clock.Now()))
 	}
 
@@ -157,8 +157,8 @@ func (ew *InvoiceExpiryWatcher) nextExpiry() <-chan time.Time {
 // cancelNextExpiredInvoice will cancel the next expired invoice and removes
 // it from the expiry queue.
 func (ew *InvoiceExpiryWatcher) cancelNextExpiredInvoice() {
-	if !ew.expiryQueue.Empty() {
-		top := ew.expiryQueue.Top().(*invoiceExpiryTs)
+	if !ew.timestampExpiryQueue.Empty() {
+		top := ew.timestampExpiryQueue.Top().(*invoiceExpiryTs)
 		if !top.Expiry.Before(ew.clock.Now()) {
 			return
 		}
@@ -177,7 +177,7 @@ func (ew *InvoiceExpiryWatcher) cancelNextExpiredInvoice() {
 				top.PaymentHash)
 		}
 
-		ew.expiryQueue.Pop()
+		ew.timestampExpiryQueue.Pop()
 	}
 }
 
@@ -194,7 +194,9 @@ func (ew *InvoiceExpiryWatcher) mainLoop() {
 			for _, invoiceWithExpiry := range invoicesWithExpiry {
 				// Avoid pushing nil object to the heap.
 				if invoiceWithExpiry != nil {
-					ew.expiryQueue.Push(invoiceWithExpiry)
+					ew.timestampExpiryQueue.Push(
+						invoiceWithExpiry,
+					)
 				}
 			}
 		}

From 4cd48c52ea059fd5d69b91031160ff52d54f924b Mon Sep 17 00:00:00 2001
From: carla <kirkcohenc@gmail.com>
Date: Fri, 23 Apr 2021 08:19:54 +0200
Subject: [PATCH 06/10] invoices: refactor - add interface for expiry items

In preparation for having more than one expiry type, we
alias the queue.PrioirtyQueueItem interface for readability.
---
 invoices/invoice_expiry_watcher.go      | 52 ++++++++++++++++---------
 invoices/invoice_expiry_watcher_test.go |  2 +-
 invoices/invoiceregistry.go             |  2 +-
 3 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/invoices/invoice_expiry_watcher.go b/invoices/invoice_expiry_watcher.go
index 21bb5d08e..daac4aedb 100644
--- a/invoices/invoice_expiry_watcher.go
+++ b/invoices/invoice_expiry_watcher.go
@@ -12,6 +12,14 @@ import (
 	"github.com/lightningnetwork/lnd/zpay32"
 )
 
+// invoiceExpiry is a vanity interface for different invoice expiry types
+// which implement the priority queue item interface, used to improve code
+// readability.
+type invoiceExpiry queue.PriorityQueueItem
+
+// Compile time assertion that invoiceExpiryTs implements invoiceExpiry.
+var _ invoiceExpiry = (*invoiceExpiryTs)(nil)
+
 // invoiceExpiryTs holds and invoice's payment hash and its expiry. This
 // is used to order invoices by their expiry time for cancellation.
 type invoiceExpiryTs struct {
@@ -50,7 +58,7 @@ type InvoiceExpiryWatcher struct {
 
 	// newInvoices channel is used to wake up the main loop when a new
 	// invoices is added.
-	newInvoices chan []*invoiceExpiryTs
+	newInvoices chan []invoiceExpiry
 
 	wg sync.WaitGroup
 
@@ -62,7 +70,7 @@ type InvoiceExpiryWatcher struct {
 func NewInvoiceExpiryWatcher(clock clock.Clock) *InvoiceExpiryWatcher {
 	return &InvoiceExpiryWatcher{
 		clock:       clock,
-		newInvoices: make(chan []*invoiceExpiryTs),
+		newInvoices: make(chan []invoiceExpiry),
 		quit:        make(chan struct{}),
 	}
 }
@@ -104,10 +112,9 @@ func (ew *InvoiceExpiryWatcher) Stop() {
 }
 
 // makeInvoiceExpiry checks if the passed invoice may be canceled and calculates
-// the expiry time and creates a slimmer invoiceExpiry object with the hash and
-// expiry time.
+// the expiry time and creates a slimmer invoiceExpiry implementation.
 func makeInvoiceExpiry(paymentHash lntypes.Hash,
-	invoice *channeldb.Invoice) *invoiceExpiryTs {
+	invoice *channeldb.Invoice) invoiceExpiry {
 
 	if invoice.State != channeldb.ContractOpen {
 		log.Debugf("Invoice not added to expiry watcher: %v",
@@ -129,7 +136,7 @@ func makeInvoiceExpiry(paymentHash lntypes.Hash,
 }
 
 // AddInvoices adds invoices to the InvoiceExpiryWatcher.
-func (ew *InvoiceExpiryWatcher) AddInvoices(invoices ...*invoiceExpiryTs) {
+func (ew *InvoiceExpiryWatcher) AddInvoices(invoices ...invoiceExpiry) {
 	if len(invoices) > 0 {
 		select {
 		case ew.newInvoices <- invoices:
@@ -181,6 +188,24 @@ func (ew *InvoiceExpiryWatcher) cancelNextExpiredInvoice() {
 	}
 }
 
+// pushInvoices adds invoices to be expired to their relevant queue.
+func (ew *InvoiceExpiryWatcher) pushInvoices(invoices []invoiceExpiry) {
+	for _, inv := range invoices {
+		// Switch on the type of entry we have. We need to check nil
+		// on the implementation of the interface because the interface
+		// itself is non-nil.
+		switch expiry := inv.(type) {
+		case *invoiceExpiryTs:
+			if expiry != nil {
+				ew.timestampExpiryQueue.Push(expiry)
+			}
+
+		default:
+			log.Errorf("unexpected queue item: %T", inv)
+		}
+	}
+}
+
 // mainLoop is a goroutine that receives new invoices and handles cancellation
 // of expired invoices.
 func (ew *InvoiceExpiryWatcher) mainLoop() {
@@ -190,23 +215,12 @@ func (ew *InvoiceExpiryWatcher) mainLoop() {
 		// Cancel any invoices that may have expired.
 		ew.cancelNextExpiredInvoice()
 
-		pushInvoices := func(invoicesWithExpiry []*invoiceExpiryTs) {
-			for _, invoiceWithExpiry := range invoicesWithExpiry {
-				// Avoid pushing nil object to the heap.
-				if invoiceWithExpiry != nil {
-					ew.timestampExpiryQueue.Push(
-						invoiceWithExpiry,
-					)
-				}
-			}
-		}
-
 		select {
 
 		case invoicesWithExpiry := <-ew.newInvoices:
 			// Take newly forwarded invoices with higher priority
 			// in order to not block the newInvoices channel.
-			pushInvoices(invoicesWithExpiry)
+			ew.pushInvoices(invoicesWithExpiry)
 			continue
 
 		default:
@@ -217,7 +231,7 @@ func (ew *InvoiceExpiryWatcher) mainLoop() {
 				continue
 
 			case invoicesWithExpiry := <-ew.newInvoices:
-				pushInvoices(invoicesWithExpiry)
+				ew.pushInvoices(invoicesWithExpiry)
 
 			case <-ew.quit:
 				return
diff --git a/invoices/invoice_expiry_watcher_test.go b/invoices/invoice_expiry_watcher_test.go
index c5b5e518e..e2c7ea82a 100644
--- a/invoices/invoice_expiry_watcher_test.go
+++ b/invoices/invoice_expiry_watcher_test.go
@@ -157,7 +157,7 @@ func TestInvoiceExpiryWhenAddingMultipleInvoices(t *testing.T) {
 	t.Parallel()
 
 	test := newInvoiceExpiryWatcherTest(t, testTime, 5, 5)
-	var invoices []*invoiceExpiryTs
+	var invoices []invoiceExpiry
 
 	for hash, invoice := range test.testData.expiredInvoices {
 		invoices = append(invoices, makeInvoiceExpiry(hash, invoice))
diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go
index 24eb6ef9b..3ef83a984 100644
--- a/invoices/invoiceregistry.go
+++ b/invoices/invoiceregistry.go
@@ -160,7 +160,7 @@ func NewRegistry(cdb *channeldb.DB, expiryWatcher *InvoiceExpiryWatcher,
 // invoices.
 func (i *InvoiceRegistry) scanInvoicesOnStart() error {
 	var (
-		pending   []*invoiceExpiryTs
+		pending   []invoiceExpiry
 		removable []channeldb.InvoiceDeleteRef
 	)
 

From d29f2fe4f9ddadf43a7fb447fa214cad13b4b0f8 Mon Sep 17 00:00:00 2001
From: carla <kirkcohenc@gmail.com>
Date: Fri, 23 Apr 2021 08:19:55 +0200
Subject: [PATCH 07/10] invoices: refactor - move cancel check into separate
 function

Useful for tests which need to mock cancelInvoiceImpl
---
 invoices/invoiceregistry.go | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go
index 3ef83a984..c457acd54 100644
--- a/invoices/invoiceregistry.go
+++ b/invoices/invoiceregistry.go
@@ -1176,6 +1176,20 @@ func (i *InvoiceRegistry) CancelInvoice(payHash lntypes.Hash) error {
 	return i.cancelInvoiceImpl(payHash, true)
 }
 
+// shouldCancel examines the state of an invoice and whether we want to
+// cancel already accepted invoices, taking our force cancel boolean into
+// account. This is pulled out into its own function so that tests that mock
+// cancelInvoiceImpl can reuse this logic.
+func shouldCancel(state channeldb.ContractState, cancelAccepted bool) bool {
+	if state != channeldb.ContractAccepted {
+		return true
+	}
+
+	// If the invoice is accepted, we should only cancel if we want to
+	// force cancelation of accepted invoices.
+	return cancelAccepted
+}
+
 // cancelInvoice attempts to cancel the invoice corresponding to the passed
 // payment hash. Accepted invoices will only be canceled if explicitly
 // requested to do so. It notifies subscribing links and resolvers that
@@ -1192,9 +1206,7 @@ func (i *InvoiceRegistry) cancelInvoiceImpl(payHash lntypes.Hash,
 	updateInvoice := func(invoice *channeldb.Invoice) (
 		*channeldb.InvoiceUpdateDesc, error) {
 
-		// Only cancel the invoice in ContractAccepted state if explicitly
-		// requested to do so.
-		if invoice.State == channeldb.ContractAccepted && !cancelAccepted {
+		if !shouldCancel(invoice.State, cancelAccepted) {
 			return nil, nil
 		}
 

From 039e9f439c55f37702ede9ddc3117c4a05f404a0 Mon Sep 17 00:00:00 2001
From: carla <kirkcohenc@gmail.com>
Date: Fri, 23 Apr 2021 08:19:56 +0200
Subject: [PATCH 08/10] invoices: refactor - update timestamp expiry to use
 specific names

---
 invoices/invoice_expiry_watcher.go | 35 ++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/invoices/invoice_expiry_watcher.go b/invoices/invoice_expiry_watcher.go
index daac4aedb..8cf2b4cb9 100644
--- a/invoices/invoice_expiry_watcher.go
+++ b/invoices/invoice_expiry_watcher.go
@@ -116,9 +116,25 @@ func (ew *InvoiceExpiryWatcher) Stop() {
 func makeInvoiceExpiry(paymentHash lntypes.Hash,
 	invoice *channeldb.Invoice) invoiceExpiry {
 
-	if invoice.State != channeldb.ContractOpen {
+	switch invoice.State {
+	// If we have an open invoice with no htlcs, we want to expire the
+	// invoice based on timestamp
+	case channeldb.ContractOpen:
+		return makeTimestampExpiry(paymentHash, invoice)
+
+	default:
 		log.Debugf("Invoice not added to expiry watcher: %v",
 			paymentHash)
+
+		return nil
+	}
+}
+
+// makeTimestampExpiry creates a timestamp-based expiry entry.
+func makeTimestampExpiry(paymentHash lntypes.Hash,
+	invoice *channeldb.Invoice) *invoiceExpiryTs {
+
+	if invoice.State != channeldb.ContractOpen {
 		return nil
 	}
 
@@ -150,9 +166,10 @@ func (ew *InvoiceExpiryWatcher) AddInvoices(invoices ...invoiceExpiry) {
 	}
 }
 
-// nextExpiry returns a Time chan to wait on until the next invoice expires.
-// If there are no active invoices, then it'll simply wait indefinitely.
-func (ew *InvoiceExpiryWatcher) nextExpiry() <-chan time.Time {
+// nextTimestampExpiry returns a Time chan to wait on until the next invoice
+// expires. If there are no active invoices, then it'll simply wait
+// indefinitely.
+func (ew *InvoiceExpiryWatcher) nextTimestampExpiry() <-chan time.Time {
 	if !ew.timestampExpiryQueue.Empty() {
 		top := ew.timestampExpiryQueue.Top().(*invoiceExpiryTs)
 		return ew.clock.TickAfter(top.Expiry.Sub(ew.clock.Now()))
@@ -217,21 +234,21 @@ func (ew *InvoiceExpiryWatcher) mainLoop() {
 
 		select {
 
-		case invoicesWithExpiry := <-ew.newInvoices:
+		case newInvoices := <-ew.newInvoices:
 			// Take newly forwarded invoices with higher priority
 			// in order to not block the newInvoices channel.
-			ew.pushInvoices(invoicesWithExpiry)
+			ew.pushInvoices(newInvoices)
 			continue
 
 		default:
 			select {
 
-			case <-ew.nextExpiry():
+			case <-ew.nextTimestampExpiry():
 				// Wait until the next invoice expires.
 				continue
 
-			case invoicesWithExpiry := <-ew.newInvoices:
-				ew.pushInvoices(invoicesWithExpiry)
+			case newInvoices := <-ew.newInvoices:
+				ew.pushInvoices(newInvoices)
 
 			case <-ew.quit:
 				return

From 7536dd81790a1db628e490e2b9685398a2716ea0 Mon Sep 17 00:00:00 2001
From: carla <kirkcohenc@gmail.com>
Date: Fri, 23 Apr 2021 08:19:57 +0200
Subject: [PATCH 09/10] invoices: refactor - add method to handle expected
 cancelation errors

---
 invoices/invoice_expiry_watcher.go | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/invoices/invoice_expiry_watcher.go b/invoices/invoice_expiry_watcher.go
index 8cf2b4cb9..14257581a 100644
--- a/invoices/invoice_expiry_watcher.go
+++ b/invoices/invoice_expiry_watcher.go
@@ -193,18 +193,27 @@ func (ew *InvoiceExpiryWatcher) cancelNextExpiredInvoice() {
 		// field would never be used. Enabling cancellation for accepted
 		// keysend invoices creates a safety mechanism that can prevents
 		// channel force-closes.
-		err := ew.cancelInvoice(top.PaymentHash, top.Keysend)
-		if err != nil && err != channeldb.ErrInvoiceAlreadySettled &&
-			err != channeldb.ErrInvoiceAlreadyCanceled {
-
-			log.Errorf("Unable to cancel invoice: %v",
-				top.PaymentHash)
-		}
-
+		ew.expireInvoice(top.PaymentHash, top.Keysend)
 		ew.timestampExpiryQueue.Pop()
 	}
 }
 
+// expireInvoice attempts to expire an invoice and logs an error if we get an
+// unexpected error.
+func (ew *InvoiceExpiryWatcher) expireInvoice(hash lntypes.Hash, force bool) {
+	err := ew.cancelInvoice(hash, force)
+	switch err {
+	case nil:
+
+	case channeldb.ErrInvoiceAlreadyCanceled:
+
+	case channeldb.ErrInvoiceAlreadySettled:
+
+	default:
+		log.Errorf("Unable to cancel invoice: %v: %v", hash, err)
+	}
+}
+
 // pushInvoices adds invoices to be expired to their relevant queue.
 func (ew *InvoiceExpiryWatcher) pushInvoices(invoices []invoiceExpiry) {
 	for _, inv := range invoices {

From d0d0d21573dbaebd9f0389fee88e0ce8d73ab2de Mon Sep 17 00:00:00 2001
From: carla <kirkcohenc@gmail.com>
Date: Fri, 23 Apr 2021 08:19:58 +0200
Subject: [PATCH 10/10] itest/test: remove hold force close dependency

Our aggregate htlc test depends on our previous behavior
where recipients would allow channels with pending hold
invoice htlcs to force close. Now that we have an expiry
watcher to prevent these force closes, we can't rely on
this for tests because the recipient will cancel the htlcs
back before they expire.
---
 lntest/itest/lnd_multi-hop_htlc_aggregation_test.go | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lntest/itest/lnd_multi-hop_htlc_aggregation_test.go b/lntest/itest/lnd_multi-hop_htlc_aggregation_test.go
index b1261c15c..3c91eed1f 100644
--- a/lntest/itest/lnd_multi-hop_htlc_aggregation_test.go
+++ b/lntest/itest/lnd_multi-hop_htlc_aggregation_test.go
@@ -169,6 +169,12 @@ func testMultiHopHtlcAggregation(net *lntest.NetworkHarness, t *harnessTest,
 	// be cpfp'ed.
 	net.SetFeeEstimate(30000)
 
+	// We want Carol's htlcs to expire off-chain to demonstrate bob's force
+	// close. However, Carol will cancel her invoices to prevent force
+	// closes, so we shut her down for now.
+	restartCarol, err := net.SuspendNode(carol)
+	require.NoError(t.t, err)
+
 	// We'll now mine enough blocks to trigger Bob's broadcast of his
 	// commitment transaction due to the fact that the Carol's HTLCs are
 	// about to timeout. With the default outgoing broadcast delta of zero,
@@ -225,6 +231,9 @@ func testMultiHopHtlcAggregation(net *lntest.NetworkHarness, t *harnessTest,
 		}
 	}
 
+	// Once bob has force closed, we can restart carol.
+	require.NoError(t.t, restartCarol())
+
 	// Mine a block to confirm the closing transaction.
 	mineBlocks(t, net, 1, expectedTxes)