From 34e08b3510c64e35fc51327562d15d938f4b656e Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Thu, 7 Dec 2017 13:40:39 -0500
Subject: [PATCH] [tests] Fix network threading in functional tests

assumevalid.py, example_test.py and p2p-acceptblocks.py add
p2p_connections after the NetworkThread has been started. This isn't
permitted. Fix test to restart the network thread when adding new
connections.

p2p-leaktest.py had a potential race condition if the NetworkThread
hadn't terminated by the time we tried to restart it.
---
 test/functional/assumevalid.py     | 20 +++++++++++++++-----
 test/functional/example_test.py    | 10 +++++++++-
 test/functional/p2p-acceptblock.py |  6 +++++-
 test/functional/p2p-leaktests.py   |  3 ++-
 4 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/test/functional/assumevalid.py b/test/functional/assumevalid.py
index 68ff319d1..362b94e0d 100755
--- a/test/functional/assumevalid.py
+++ b/test/functional/assumevalid.py
@@ -38,10 +38,11 @@ from test_framework.mininode import (CBlockHeader,
                                      CTransaction,
                                      CTxIn,
                                      CTxOut,
+                                     network_thread_join,
+                                     network_thread_start,
                                      P2PInterface,
                                      msg_block,
-                                     msg_headers,
-                                     network_thread_start)
+                                     msg_headers)
 from test_framework.script import (CScript, OP_TRUE)
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
@@ -159,13 +160,22 @@ class AssumeValidTest(BitcoinTestFramework):
             self.block_time += 1
             height += 1
 
+        # We're adding new connections so terminate the network thread
+        self.nodes[0].disconnect_p2ps()
+        network_thread_join()
+
         # Start node1 and node2 with assumevalid so they accept a block with a bad signature.
         self.start_node(1, extra_args=["-assumevalid=" + hex(block102.sha256)])
-        p2p1 = self.nodes[1].add_p2p_connection(BaseNode())
-        p2p1.wait_for_verack()
-
         self.start_node(2, extra_args=["-assumevalid=" + hex(block102.sha256)])
+
+        p2p0 = self.nodes[0].add_p2p_connection(BaseNode())
+        p2p1 = self.nodes[1].add_p2p_connection(BaseNode())
         p2p2 = self.nodes[2].add_p2p_connection(BaseNode())
+
+        network_thread_start()
+
+        p2p0.wait_for_verack()
+        p2p1.wait_for_verack()
         p2p2.wait_for_verack()
 
         # send header lists to all three nodes
diff --git a/test/functional/example_test.py b/test/functional/example_test.py
index c9c5c6fc4..12be685ec 100755
--- a/test/functional/example_test.py
+++ b/test/functional/example_test.py
@@ -21,6 +21,7 @@ from test_framework.mininode import (
     mininode_lock,
     msg_block,
     msg_getdata,
+    network_thread_join,
     network_thread_start,
 )
 from test_framework.test_framework import BitcoinTestFramework
@@ -131,7 +132,7 @@ class ExampleTest(BitcoinTestFramework):
     def run_test(self):
         """Main test logic"""
 
-        # Create a P2P connection to one of the nodes
+        # Create P2P connections to two of the nodes
         self.nodes[0].add_p2p_connection(BaseNode())
 
         # Start up network handling in another thread. This needs to be called
@@ -188,7 +189,14 @@ class ExampleTest(BitcoinTestFramework):
         connect_nodes(self.nodes[1], 2)
 
         self.log.info("Add P2P connection to node2")
+        # We can't add additional P2P connections once the network thread has started. Disconnect the connection
+        # to node0, wait for the network thread to terminate, then connect to node2. This is specific to
+        # the current implementation of the network thread and may be improved in future.
+        self.nodes[0].disconnect_p2ps()
+        network_thread_join()
+
         self.nodes[2].add_p2p_connection(BaseNode())
+        network_thread_start()
         self.nodes[2].p2p.wait_for_verack()
 
         self.log.info("Wait for node2 reach current tip. Test that it has propagated all the blocks to us")
diff --git a/test/functional/p2p-acceptblock.py b/test/functional/p2p-acceptblock.py
index b56cafc98..bb204322e 100755
--- a/test/functional/p2p-acceptblock.py
+++ b/test/functional/p2p-acceptblock.py
@@ -207,9 +207,13 @@ class AcceptBlockTest(BitcoinTestFramework):
         # disconnect/reconnect first
 
         self.nodes[0].disconnect_p2ps()
-        test_node = self.nodes[0].add_p2p_connection(P2PInterface())
+        self.nodes[1].disconnect_p2ps()
+        network_thread_join()
 
+        test_node = self.nodes[0].add_p2p_connection(P2PInterface())
+        network_thread_start()
         test_node.wait_for_verack()
+
         test_node.send_message(msg_block(block_h1f))
 
         test_node.sync_with_ping()
diff --git a/test/functional/p2p-leaktests.py b/test/functional/p2p-leaktests.py
index 9b59d753f..ce4e6e914 100755
--- a/test/functional/p2p-leaktests.py
+++ b/test/functional/p2p-leaktests.py
@@ -126,8 +126,9 @@ class P2PLeakTest(BitcoinTestFramework):
 
         self.nodes[0].disconnect_p2ps()
 
-        # Wait until all connections are closed
+        # Wait until all connections are closed and the network thread has terminated
         wait_until(lambda: len(self.nodes[0].getpeerinfo()) == 0)
+        network_thread_join()
 
         # Make sure no unexpected messages came in
         assert(no_version_bannode.unexpected_msg == False)