Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[itests] Bitcoind integration test #1583

Merged

Conversation

@halseth
Copy link
Collaborator

commented Jul 18, 2018

This PR enables the bitcoind backend for integration tests.

@halseth halseth force-pushed the halseth:multibackend-integration-test branch 24 times, most recently from d524723 to 19bfc14 Jul 18, 2018
@halseth halseth force-pushed the halseth:multibackend-integration-test branch from 19bfc14 to 0e292f4 Aug 6, 2018
@halseth halseth force-pushed the halseth:multibackend-integration-test branch 5 times, most recently from 8c78286 to 5b4296a Sep 4, 2018
@wpaulino wpaulino added the P2 label Jul 30, 2019
@@ -14358,7 +14358,7 @@ func TestLightningNetworkDaemon(t *testing.T) {

// Next mine enough blocks in order for segwit and the CSV package
// soft-fork to activate on SimNet.
numBlocks := chaincfg.SimNetParams.MinerConfirmationWindow * 2
numBlocks := chaincfg.RegressionNetParams.MinerConfirmationWindow * 2

This comment has been minimized.

Copy link
@wpaulino

wpaulino Jul 31, 2019

Collaborator

Nit: use harnessNetParams?

This comment has been minimized.

Copy link
@halseth

halseth Aug 7, 2019

Author Collaborator

Done.

@@ -76,7 +76,7 @@ func NewBackend(miner string) (*BtcdBackendConfig, func(), error) {
"--logdir=" + logDir,
"--connect=" + miner,
}
netParams := &chaincfg.SimNetParams
netParams := &chaincfg.RegressionNetParams

This comment has been minimized.

Copy link
@wpaulino

wpaulino Jul 31, 2019

Collaborator

Nit: would be nice if this was provided as a function parameter.

This comment has been minimized.

Copy link
@halseth

halseth Aug 7, 2019

Author Collaborator

Done.

@@ -389,7 +389,7 @@ func (n *NetworkHarness) RegisterNode(node *HarnessNode) {
func (n *NetworkHarness) connect(ctx context.Context,
req *lnrpc.ConnectPeerRequest, a *HarnessNode) error {

syncTimeout := time.After(15 * time.Second)
syncTimeout := time.After(60 * time.Second)

This comment has been minimized.

Copy link
@wpaulino

wpaulino Jul 31, 2019

Collaborator

Any insights as to why this had to be increased?

This comment has been minimized.

Copy link
@halseth

halseth Aug 7, 2019

Author Collaborator

Because bitcoind was slower to catch up block hashes IIRC. Since this should be faster now I can try to revert this.

lntest/bitcoind.go Show resolved Hide resolved
lntest/bitcoind.go Show resolved Hide resolved
)

// logDir is the name of the temporary log directory.
const logDir = "./.backendlogs"

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Aug 6, 2019

Member

IIRC, this is defined elsewhere already?

This comment has been minimized.

Copy link
@halseth

halseth Aug 7, 2019

Author Collaborator

It is re-defined since it is behind a different build flag.

@Roasbeef

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

itests look to be failing with an infrequent error?

@halseth halseth force-pushed the halseth:multibackend-integration-test branch 2 times, most recently from 2ac4811 to 190a40c Aug 7, 2019
@halseth

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 7, 2019

itests look to be failing with an infrequent error?

Should be fixed by d531fd3

@halseth

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 7, 2019

btw @Roasbeef, any reasons not to switch to regtest also for btcd/neutrino?

@wpaulino

This comment has been minimized.

Copy link
Collaborator

commented Aug 8, 2019

Did a couple more travis runs and found the following flakes:

    --- FAIL: TestLightningNetworkDaemon/test_multi-hop_htlc_local_force_close_immediate_expiry (44.08s)
        lnd_test.go:99: Failed: (test multi-hop htlc local force close immediate expiry): exited with error: 
            *errors.errorString unable to find bob's funding output sweep tx: wanted 1, found 2 txs in mempool: [053fe1382d7e4b376490c2b9d18bf8c2cde6e46e6ff89bf649f1d4dadd8eca61 05b1d7d90eea4cdc8c91b2b4f2da68bab3d8525fd3ff0f5c5f47550c06a2af02]
            /home/travis/gopath/src/github.com/lightningnetwork/lnd/lntest/itest/lnd_test.go:10014 (0xd3ce49)
            	testMultiHopHtlcLocalTimeout: t.Fatalf("unable to find bob's funding output sweep tx: %v", err)
            /home/travis/gopath/src/github.com/lightningnetwork/lnd/lntest/itest/lnd_test.go:123 (0xd03d85)
            	(*harnessTest).RunTestCase: testCase.test(h.lndHarness, h)
            /home/travis/gopath/src/github.com/lightningnetwork/lnd/lntest/itest/lnd_test.go:14318 (0xd6de74)
            	TestLightningNetworkDaemon.func4: ht.RunTestCase(testCase)
            /home/travis/.gimme/versions/go1.12.7.linux.amd64/src/testing/testing.go:865 (0x4fcd20)
            	tRunner: fn(t)
            /home/travis/.gimme/versions/go1.12.7.linux.amd64/src/runtime/asm_amd64.s:1337 (0x45e6d1)
            	goexit: BYTE	$0x90	// NOP
    --- FAIL: TestLightningNetworkDaemon/test_multi-hop_htlc_local_force_close_immediate_expiry (41.93s)
        lnd_test.go:99: Failed: (test multi-hop htlc local force close immediate expiry): exited with error: 
            *errors.errorString bob still has pending channels but shouldn't: (*lnrpc.PendingChannelsResponse)(0xc000d70870)(total_limbo_balance:21713 pending_force_closing_channels:<channel:<remote_node_pub:"02aae00790835d168caa7ae46eab9fdaf317b65cf5bfbda0230e1cf065f08d48c9" channel_point:"5250884a5f380defa16cc768ba6d9a13d841745ca37a863b26473ebca5fd1f43:1" capacity:1000000 local_balance:958700 > closing_txid:"a95012857089bc3e06614a6b11cf850211a4803d10b1c9a0600a463a4c7e5673" limbo_balance:21713 maturity_height:1046 blocks_til_maturity:-6 recovered_balance:958700 pending_htlcs:<amount:21713 outpoint:"cc4fc894a49cacf84b3ca4bb621ae61a4c99b1f888eb7dc9d6252648d13f11b2:0" maturity_height:1051 blocks_til_maturity:-1 stage:2 > > )
            /home/travis/gopath/src/github.com/lightningnetwork/lnd/lntest/itest/lnd_test.go:10115 (0xd3d764)
            	testMultiHopHtlcLocalTimeout: t.Fatalf(predErr.Error())
            /home/travis/gopath/src/github.com/lightningnetwork/lnd/lntest/itest/lnd_test.go:123 (0xd03d85)
            	(*harnessTest).RunTestCase: testCase.test(h.lndHarness, h)
            /home/travis/gopath/src/github.com/lightningnetwork/lnd/lntest/itest/lnd_test.go:14318 (0xd6de74)
            	TestLightningNetworkDaemon.func4: ht.RunTestCase(testCase)
            /home/travis/.gimme/versions/go1.12.7.linux.amd64/src/testing/testing.go:865 (0x4fcd20)
            	tRunner: fn(t)
            /home/travis/.gimme/versions/go1.12.7.linux.amd64/src/runtime/asm_amd64.s:1337 (0x45e6d1)
            	goexit: BYTE	$0x90	// NOP
@wpaulino

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

Tracked down the failures above and they should be resolved with the following patch:

diff --git a/lntest/bitcoind.go b/lntest/bitcoind.go
index dd0cc5e9..3e7c1d35 100644
--- a/lntest/bitcoind.go
+++ b/lntest/bitcoind.go
@@ -102,6 +102,7 @@ func NewBackend(miner string, netParams *chaincfg.Params) (
 		"-datadir="+tempBitcoindDir,
 		"-regtest",
 		"-txindex",
+		"-debug",
 		"-whitelist=127.0.0.1", // whitelist localhost to speed up relay
 		"-rpcauth=weks:469e9bb14ab2360f8e226efed5ca6f"+
 			"d$507c670e800a95284294edb5773b05544b"+
diff --git a/lntest/itest/lnd_test.go b/lntest/itest/lnd_test.go
index 85212435..c2f9e5ab 100644
--- a/lntest/itest/lnd_test.go
+++ b/lntest/itest/lnd_test.go
@@ -10004,19 +10004,13 @@ func testMultiHopHtlcLocalTimeout(net *lntest.NetworkHarness, t *harnessTest) {
 
 	// We'll mine defaultCSV blocks in order to generate the sweep
 	// transaction of Bob's funding output. This will also bring us to the
-	// maturity height of the htlc tx output.
+	// maturity height of the htlc tx output, so we should expect to see
+	// both transactions in the mempool.
 	if _, err := net.Miner.Node.Generate(defaultCSV); err != nil {
 		t.Fatalf("unable to generate blocks: %v", err)
 	}
 
-	_, err = waitForTxInMempool(net.Miner.Node, minerMempoolTimeout)
-	if err != nil {
-		t.Fatalf("unable to find bob's funding output sweep tx: %v", err)
-	}
-
-	// The second layer HTLC timeout transaction should now have been
-	// broadcast on-chain.
-	secondLayerHash, err := waitForTxInMempool(net.Miner.Node, minerMempoolTimeout)
+	txids, err := waitForNTxsInMempool(net.Miner.Node, 2, minerMempoolTimeout)
 	if err != nil {
 		t.Fatalf("unable to find bob's second layer transaction")
 	}
@@ -10045,12 +10039,14 @@ func testMultiHopHtlcLocalTimeout(net *lntest.NetworkHarness, t *harnessTest) {
 
 	// Now we'll mine an additional block, which should include the second
 	// layer sweep tx.
-	block := mineBlocks(t, net, 1, 1)[0]
+	block := mineBlocks(t, net, 1, 2)[0]
 
 	// The block should have confirmed Bob's second layer sweeping
 	// transaction. Therefore, at this point, there should be no active
 	// HTLC's on the commitment transaction from Alice -> Bob.
-	assertTxInBlock(t, block, secondLayerHash)
+	for _, txid := range txids {
+		assertTxInBlock(t, block, txid)
+	}
 	nodes = []*lntest.HarnessNode{net.Alice}
 	err = lntest.WaitPredicate(func() bool {
 		predErr = assertNumActiveHtlcs(nodes, 0)
@Roasbeef

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@wpaulino so the flakes weren't fundamental to bitcoind, but just showed up more often, making the patch above a general flake fix for all backends?

@wpaulino

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

Correct. Things seem to be slower with bitcoind, likely due to the several HTTP requests made. The patch above solves an incorrect assumption within the test itself. We should expect both bob's commit sweep and HTLC timeout transactions in the mempool before proceeding. Without the patch we'd only wait for one of these twice, causing us to proceed with further assumptions that rely on this one.

@wpaulino

This comment has been minimized.

Copy link
Collaborator

commented Aug 27, 2019

This PR should now be rebased on top of #3434 in order to address the flake pointed out above.

@halseth halseth force-pushed the halseth:multibackend-integration-test branch from 190a40c to f5b5fd4 Aug 28, 2019
halseth and others added 6 commits Jul 11, 2019
…annel

Otherwise following tests would be flaky because of unexpected sweep transaction in the mempool.
Co-authored-by: Wilmer Paulino <wilmer.paulino@gmail.com>
@halseth halseth force-pushed the halseth:multibackend-integration-test branch from f5b5fd4 to c3480c6 Sep 2, 2019
The local timeout could be too short for certain backends.
@halseth

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 3, 2019

Looks good after rebase on #3434 👍

@halseth

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 3, 2019

@wpaulino wpaulino requested review from Roasbeef and wpaulino Sep 4, 2019
Copy link
Collaborator

left a comment

LGTM ⚡️

"bitcoind",
"-datadir="+tempBitcoindDir,
"-regtest",
"-txindex",

This comment has been minimized.

Copy link
@wpaulino

wpaulino Sep 4, 2019

Collaborator

Should we run without this? Would ensure that the fallback path (no txindex, scan manually) is tested instead.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Sep 5, 2019

Member

We could run it with both options, though this would significantly increase the running time of the current integration tests. On the other hand, we properly test both variants in the unit tests of the notifier.

This comment has been minimized.

Copy link
@halseth

halseth Sep 5, 2019

Author Collaborator

Could randomize it lol

Copy link
Member

left a comment

LGTM 🖌

@Roasbeef Roasbeef merged commit b43a9f2 into lightningnetwork:master Sep 5, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 61.15%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.