Merge pull request #5205 from carlaKC/4987-refactorprep

invoices: add test to demonstrate hodl invoice bug and refactor

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)
+}

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)
 

lntest/itest/lnd_multi-hop_remote_force_close_on_chain_htlc_timeout_test.go

@@ -5,9 +5,12 @@ 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"
 	"github.com/lightningnetwork/lnd/lntest/wait"
+	"github.com/lightningnetwork/lnd/lntypes"
 	"github.com/stretchr/testify/require"
 )
 
@@ -43,14 +46,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 +71,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 +83,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,
@@ -168,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.
@@ -176,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")
+	}
+}

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))
 		}
 

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,