From 835c73632bae59340f7e1f23baf549faafeb8f34 Mon Sep 17 00:00:00 2001 From: Matheus Degiovani Date: Mon, 9 Mar 2020 14:43:47 -0300 Subject: [PATCH] itest: fix off-by-one mining tests This fixes tests that were surfaced as flaky. Usually these tests had an off-by-one error when mining blocks while waiting for a CSV-encumbered output. --- .../lnd_multi-hop_htlc_local_timeout_test.go | 18 +++++++--- ..._multi-hop_htlc_remote_chain_claim_test.go | 36 +++++++++++++------ ..._force_close_on_chain_htlc_timeout_test.go | 8 +++-- lntest/itest/lnd_test.go | 15 +++++--- 4 files changed, 55 insertions(+), 22 deletions(-) diff --git a/lntest/itest/lnd_multi-hop_htlc_local_timeout_test.go b/lntest/itest/lnd_multi-hop_htlc_local_timeout_test.go index a86dcf0e..e9ef4643 100644 --- a/lntest/itest/lnd_multi-hop_htlc_local_timeout_test.go +++ b/lntest/itest/lnd_multi-hop_htlc_local_timeout_test.go @@ -160,8 +160,11 @@ func testMultiHopHtlcLocalTimeout(net *lntest.NetworkHarness, t *harnessTest, require.NotNil(t.t, htlcTimeout) // We'll mine the remaining blocks in order to generate the sweep - // transaction of Bob's commitment output. - mineBlocks(t, net, defaultCSV, expectedTxes) + // transaction of Bob's commitment output. The commitment was just + // mined at the current tip and the sweep will be broadcast so it can + // be mined at the tip+defaultCSV'th block, so mine one less to be able + // to make mempool assertions. + mineBlocks(t, net, defaultCSV-1, expectedTxes) // Check that the sweep spends from the mined commitment. txes, err = getNTxsFromMempool(net.Miner.Node, 1, minerMempoolTimeout) @@ -181,10 +184,15 @@ func testMultiHopHtlcLocalTimeout(net *lntest.NetworkHarness, t *harnessTest, require.NotZero(t.t, forceCloseChan.LimboBalance) require.NotZero(t.t, len(forceCloseChan.PendingHtlcs)) - // Now we'll mine an additional block, which should confirm Bob's commit - // sweep. This block should also prompt Bob to broadcast their second + // Mine a block to confirm Bob's commit sweep tx and assert it was in + // fact mined. + block := mineBlocks(t, net, 1, 1)[0] + commitSweepTxid := txes[0].TxHash() + assertTxInBlock(t, block, &commitSweepTxid) + + // Mine an additional block to prompt Bob to broadcast their second // layer sweep due to the CSV on the HTLC timeout output. - mineBlocks(t, net, 1, 1) + mineBlocks(t, net, 1, 0) assertSpendingTxInMempool( t, net.Miner.Node, minerMempoolTimeout, wire.OutPoint{ Hash: *htlcTimeout, diff --git a/lntest/itest/lnd_multi-hop_htlc_remote_chain_claim_test.go b/lntest/itest/lnd_multi-hop_htlc_remote_chain_claim_test.go index 29ffd1ce..1aeff01e 100644 --- a/lntest/itest/lnd_multi-hop_htlc_remote_chain_claim_test.go +++ b/lntest/itest/lnd_multi-hop_htlc_remote_chain_claim_test.go @@ -97,19 +97,28 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest err = waitForChannelPendingForceClose(ctxt, alice, aliceChanPoint) require.NoError(t.t, err) + // After closeChannelAndAssertType returns, it has mined a block so now + // bob will attempt to redeem his anchor commitment (if the channel + // type is of that type). + if c == commitTypeAnchors { + _, err = waitForNTxsInMempool(net.Miner.Node, 1, minerMempoolTimeout) + if err != nil { + t.Fatalf("unable to find bob's anchor commit sweep: %v", err) + + } + } + // Mine enough blocks for Alice to sweep her funds from the force - // closed channel. - _, err = net.Miner.Node.Generate(defaultCSV) + // closed channel. closeCHannelAndAssertType() already mined a block + // containing the commitment tx and the commit sweep tx will be + // broadcast immediately before it can be included in a block, so mine + // one less than defaultCSV in order to perform mempool assertions. + _, err = net.Miner.Node.Generate(defaultCSV - 1) require.NoError(t.t, err) - // Alice should now sweep her funds. If there are anchors, Alice should - // also sweep hers. - expectedTxes := 1 - if c == commitTypeAnchors { - expectedTxes = 2 - } + // Alice should now sweep her funds. _, err = waitForNTxsInMempool( - net.Miner.Node, expectedTxes, minerMempoolTimeout, + net.Miner.Node, 1, minerMempoolTimeout, ) require.NoError(t.t, err) @@ -138,8 +147,13 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest _, err = net.Miner.Node.Generate(numBlocks) require.NoError(t.t, err) - // Carol's commitment transaction should now be in the mempool. If there - // are anchors, Carol also sweeps her anchor. + expectedTxes := 1 + if c == commitTypeAnchors { + expectedTxes = 2 + } + + // Carol's commitment transaction should now be in the mempool. If + // there are anchors, Carol also sweeps her anchor. _, err = waitForNTxsInMempool( net.Miner.Node, expectedTxes, minerMempoolTimeout, ) diff --git a/lntest/itest/lnd_multi-hop_local_force_close_on_chain_htlc_timeout_test.go b/lntest/itest/lnd_multi-hop_local_force_close_on_chain_htlc_timeout_test.go index 75c49690..83ede6e9 100644 --- a/lntest/itest/lnd_multi-hop_local_force_close_on_chain_htlc_timeout_test.go +++ b/lntest/itest/lnd_multi-hop_local_force_close_on_chain_htlc_timeout_test.go @@ -100,7 +100,11 @@ func testMultiHopLocalForceCloseOnChainHtlcTimeout(net *lntest.NetworkHarness, require.NoError(t.t, err) } - _, err = net.Miner.Node.Generate(defaultCSV) + // The sweep is broadcast on the block immediately before the CSV + // expires and the commitment was already mined inside + // closeChannelAndAssertType(), so mine one block less than defaultCSV + // in order to perform mempool assertions. + _, err = net.Miner.Node.Generate(defaultCSV - 1) require.NoError(t.t, err) _, err = waitForTxInMempool(net.Miner.Node, minerMempoolTimeout) @@ -108,7 +112,7 @@ func testMultiHopLocalForceCloseOnChainHtlcTimeout(net *lntest.NetworkHarness, // We'll now mine enough blocks for the HTLC to expire. After this, Bob // should hand off the now expired HTLC output to the utxo nursery. - numBlocks := padCLTV(uint32(finalCltvDelta - defaultCSV - 1)) + numBlocks := padCLTV(uint32(finalCltvDelta - defaultCSV)) _, err = net.Miner.Node.Generate(numBlocks) require.NoError(t.t, err) diff --git a/lntest/itest/lnd_test.go b/lntest/itest/lnd_test.go index 7f60c405..e2299e8b 100644 --- a/lntest/itest/lnd_test.go +++ b/lntest/itest/lnd_test.go @@ -409,7 +409,10 @@ func cleanupForceClose(t *harnessTest, net *lntest.NetworkHarness, // Mine enough blocks for the node to sweep its funds from the force // closed channel. - _, err = net.Miner.Node.Generate(defaultCSV) + // + // The commit sweep resolver is able to broadcast the sweep tx up to + // one block before the CSV elapses, so wait until defaulCSV-1. + _, err = net.Miner.Node.Generate(defaultCSV - 1) if err != nil { t.Fatalf("unable to generate blocks: %v", err) } @@ -7726,7 +7729,7 @@ func testFailingChannel(net *lntest.NetworkHarness, t *harnessTest) { // Mine enough blocks for Alice to sweep her funds from the force // closed channel. - _, err = net.Miner.Node.Generate(defaultCSV) + _, err = net.Miner.Node.Generate(defaultCSV - 1) if err != nil { t.Fatalf("unable to generate blocks: %v", err) } @@ -9505,7 +9508,11 @@ func assertDLPExecuted(net *lntest.NetworkHarness, t *harnessTest, } // After the Carol's output matures, she should also reclaim her funds. - mineBlocks(t, net, defaultCSV-1, 0) + // + // The commit sweep resolver publishes the sweep tx at defaultCSV-1 and + // we already mined one block after the commitmment was published, so + // take that into account. + mineBlocks(t, net, defaultCSV-1-1, 0) carolSweep, err := waitForTxInMempool( net.Miner.Node, minerMempoolTimeout, ) @@ -9796,7 +9803,7 @@ func testDataLossProtection(net *lntest.NetworkHarness, t *harnessTest) { } // Mine enough blocks for Carol to sweep her funds. - mineBlocks(t, net, defaultCSV, 0) + mineBlocks(t, net, defaultCSV-1, 0) carolSweep, err := waitForTxInMempool(net.Miner.Node, minerMempoolTimeout) if err != nil {