[tests] Remove is_network_split from functional test framework

jnewbery commented at 4:08 pm on April 12, 2017: member

The split_network() and join_network() functions in bitcoin test framework are problematic:

They make assumptions about the network topology (4 nodes, either connected linearly A-B-C-D or split A-B // C-D), and force these assumptions on all test cases. The setup_network() and sync_all() functions use is_network_split, which means that many test cases have to set this variable for no other reason than to make those functions work. Removing the requirement to set is_network_split means many test cases don’t need to override the setup_network() method, which removes some 200 loc. split_network() and join_network() are only used by 2 test cases - that’s a lot of redundant code for the benefit of two test cases.
They’re horribly inefficient. They both stop all 4 nodes, then restart. That takes ~10 seconds on my pc.
They may have unexpected side-effects. stop-starting removes ephemeral state on the node, and that behaviour may change between releases (eg mempool persistence).

This PR:

removes the is_network_split variable
makes setup_network() and sync_all() more generic by making fewer assumptions about the test network topology
uses a new disconnect_nodes() helper function in split_network() and join_network()

This requires #10143, which isn’t yet merged

benefits:

removes ~200 lines of unnecessary test setup code
getchaintips and listsinceblock now run faster:

before:

0 ./listsinceblock.py && ./getchaintips.py 
12017-04-12 15:43:53.949000 TestFramework (INFO): Initializing test directory /tmp/user/1000/test58b95puo/15463
22017-04-12 15:44:05.686000 TestFramework (INFO): lastblockhash=798a876d18cb546c59e76d7d0f62611b588dd6046ba989e5800cc8a981e025db
32017-04-12 15:44:15.959000 TestFramework (INFO): Stopping nodes
42017-04-12 15:44:24.526000 TestFramework (INFO): Cleaning up
52017-04-12 15:44:24.529000 TestFramework (INFO): Tests successful
62017-04-12 15:44:24.590000 TestFramework (INFO): Initializing test directory /tmp/user/1000/testk_xibf92/15632
72017-04-12 15:44:46.263000 TestFramework (INFO): Stopping nodes
82017-04-12 15:44:54.658000 TestFramework (INFO): Cleaning up
92017-04-12 15:44:54.667000 TestFramework (INFO): Tests successful

after:

02017-04-12 15:43:12.662000 TestFramework (INFO): Initializing test directory /tmp/user/1000/testqi2c2skb/15342
12017-04-12 15:43:14.616000 TestFramework (INFO): lastblockhash=7c70eee0979069504d6059c3cd9658fc8c57d8ae30a3872716c1f9a244a53ae1
22017-04-12 15:43:14.834000 TestFramework (INFO): Stopping nodes
32017-04-12 15:43:23.205000 TestFramework (INFO): Cleaning up
42017-04-12 15:43:23.207000 TestFramework (INFO): Tests successful
52017-04-12 15:43:23.270000 TestFramework (INFO): Initializing test directory /tmp/user/1000/testmxve6ryf/15399
62017-04-12 15:43:25.363000 TestFramework (INFO): Stopping nodes
72017-04-12 15:43:33.648000 TestFramework (INFO): Cleaning up
82017-04-12 15:43:33.658000 TestFramework (INFO): Tests successful

excluding the test shutdown time (which will be improved by #10082), the change is from 44s to 4.2s (10x speedup).

bitcoin test framework is now more generic, and can be extended later to automatically spin up different test network topologies.

jnewbery commented at 4:08 pm on April 12, 2017: member

closing until #10143 is merged

jnewbery closed this on Apr 12, 2017

jnewbery commented at 3:15 pm on April 20, 2017: member

#10143 is now merged, so rebasing and re-opening this. @kallewoof - you’ve been working on listsinceblock recently. Do you mind reviewing this and letting me know what you think?

jnewbery reopened this on Apr 20, 2017

jnewbery force-pushed on Apr 20, 2017

in test/functional/test_framework/util.py:395 in c5881ae815 outdated

390+    while True:
391+        peer_ids = [peer['id'] for peer in from_connection.getpeerinfo() if "testnode%d" % node_num in peer['subver']]
392+        if not peer_ids:
393+            break
394+        [from_connection.disconnectnode(nodeid=peer_id) for peer_id in peer_ids]
395+        assert timeout > 0.1, "timed out waiting for disconnect"

jimmysong commented at 10:10 pm on April 20, 2017:

This seems like it would be made better as the for loop condition:

for _ in range(50): 
    ...
    time.sleep(0.1)
else:
    assert 0, "timed out waiting for disconnect"

At least for me, that’s a bit easier to read.

jnewbery commented at 3:09 pm on April 25, 2017:

yes, makes sense. Changed

in test/functional/test_framework/util.py:394 in c5881ae815 outdated

389+    timeout = 5
390+    while True:
391+        peer_ids = [peer['id'] for peer in from_connection.getpeerinfo() if "testnode%d" % node_num in peer['subver']]
392+        if not peer_ids:
393+            break
394+        [from_connection.disconnectnode(nodeid=peer_id) for peer_id in peer_ids]

jimmysong commented at 10:12 pm on April 20, 2017:

This happens every time through the loop. Is that your intention? Doesn’t this only have to be done once?

jnewbery commented at 3:04 pm on April 25, 2017:

It should only need to be called once, but I called this every loop in case either:

disconnectnode fails for one of the nodes and we need to rerun it.
a new connection to the node is added while we’re in the loop.

I don’t think either of those should happen, so we can probably just remove this.

in test/functional/test_framework/test_framework.py:88 in ec27a4bc91 outdated

100+                node_groups = [self.nodes[:2], self.nodes[2:]]
101+            else:
102+                node_groups = [self.nodes]
103+
104+        [sync_blocks(group) for group in node_groups]
105+        [sync_mempools(group) for group in node_groups]

jimmysong commented at 10:14 pm on April 20, 2017:

If you’re going to do this, why not just a single for loop?

for group in node_groups:
    sync_blocks(group)
    sync_mempools(group)

For me, that’s a little easier to read.

in test/functional/bip68-sequence.py:27 in b9e9ef2415 outdated

26-    def setup_network(self):
27-        self.nodes = []
28-        self.nodes.append(start_node(0, self.options.tmpdir))
29-        self.nodes.append(start_node(1, self.options.tmpdir, ["-acceptnonstdtxn=0"]))
30+    def run_test(self):
31         self.relayfee = self.nodes[0].getnetworkinfo()["relayfee"]

jimmysong commented at 10:19 pm on April 20, 2017:

Shouldn’t this go in __init__?

in test/functional/bip68-sequence.py:31 in b9e9ef2415 outdated

27-        self.nodes = []
28-        self.nodes.append(start_node(0, self.options.tmpdir))
29-        self.nodes.append(start_node(1, self.options.tmpdir, ["-acceptnonstdtxn=0"]))
30+    def run_test(self):
31         self.relayfee = self.nodes[0].getnetworkinfo()["relayfee"]
32-        connect_nodes(self.nodes[0], 1)

jimmysong commented at 10:19 pm on April 20, 2017:

Doesn’t this or setup_nodes need to be called at some point?

jnewbery commented at 5:22 pm on April 25, 2017:

The base class calls setup_network(), which calls setup_nodes()

in test/functional/bipdersig.py:15 in b9e9ef2415 outdated

11@@ -12,12 +12,10 @@ def __init__(self):
12         super().__init__()
13         self.num_nodes = 3
14         self.setup_clean_chain = False
15+        self.extra_args = [["-blockversion=2"], ["-blockversion=3"]]

jimmysong commented at 10:20 pm on April 20, 2017:

Shouldn’t this have an empty list first? Node 0 has no opts, Node 1 has blockversion=2, etc?

jnewbery commented at 3:53 pm on April 25, 2017:

correct. Fixed

in test/functional/mempool_limit.py:19 in b9e9ef2415 outdated

20         self.setup_clean_chain = True
21         self.num_nodes = 1
22+        self.extra_args = [["-maxmempool=5", "-spendzeroconfchange=0"]]
23 
24+    def run_test(self):
25         self.txouts = gen_return_txouts()

jimmysong commented at 10:23 pm on April 20, 2017:

shouldn’t this be in __init__?

jnewbery commented at 3:41 pm on April 25, 2017:

As below. I’d prefer __init__() to just be for setting up the test network topology.

However, these could be changed to be local variables instead of object variables.

in test/functional/mempool_spendcoinbase.py:27 in b9e9ef2415 outdated

21@@ -22,12 +22,7 @@ def __init__(self):
22         super().__init__()
23         self.num_nodes = 1
24         self.setup_clean_chain = False
25-
26-    def setup_network(self):
27-        # Just need one node for this test

jimmysong commented at 10:24 pm on April 20, 2017:

keep comment?

jnewbery commented at 3:39 pm on April 25, 2017:

meh. Should be clear from the self.num_nodes assignment in __init__(). I don’t think we need this comment (and if we do need it, then there should be a similar comment in every other test case.

in test/functional/getblocktemplate_proposals.py:78 in b9e9ef2415 outdated

72@@ -73,10 +73,6 @@ def __init__(self):
73         self.num_nodes = 2
74         self.setup_clean_chain = False
75 
76-    def setup_network(self):
77-        self.nodes = self.setup_nodes()
78-        connect_nodes_bi(self.nodes, 0, 1)

jimmysong commented at 10:26 pm on April 20, 2017:

isn’t this necessary?

jnewbery commented at 3:52 pm on April 25, 2017:

This is now done by the base class setup_network()

in test/functional/bip65-cltv.py:21 in b9e9ef2415 outdated

20-        self.nodes.append(start_node(1, self.options.tmpdir, ["-blockversion=3"]))
21-        self.nodes.append(start_node(2, self.options.tmpdir, ["-blockversion=4"]))
22+        self.setup_nodes()
23         connect_nodes(self.nodes[1], 0)
24         connect_nodes(self.nodes[2], 0)
25         self.sync_all()

jimmysong commented at 10:28 pm on April 20, 2017:

do you need the sync_all ?

in test/functional/bipdersig.py:21 in b9e9ef2415 outdated

20-        self.nodes.append(start_node(1, self.options.tmpdir, ["-blockversion=2"]))
21-        self.nodes.append(start_node(2, self.options.tmpdir, ["-blockversion=3"]))
22+        self.setup_nodes()
23         connect_nodes(self.nodes[1], 0)
24         connect_nodes(self.nodes[2], 0)
25         self.sync_all()

jimmysong commented at 10:29 pm on April 20, 2017:

Necessary?

jnewbery commented at 5:23 pm on April 25, 2017:

probably not, but harmless

in test/functional/importmulti.py:16 in b9e9ef2415 outdated

11@@ -12,8 +12,8 @@ def __init__(self):
12         self.num_nodes = 2
13         self.setup_clean_chain = True
14 
15-    def setup_network(self, split=False):
16-        self.nodes = start_nodes(2, self.options.tmpdir)
17+    def setup_network(self):
18+        self.setup_nodes()

jimmysong commented at 10:29 pm on April 20, 2017:

This is the same as the parent, no? Can be deleted?

jnewbery commented at 5:24 pm on April 25, 2017:

setup_network() in the base class also connects the nodes, which we don’t want to do for this test.

in test/functional/invalidateblock.py:18 in b9e9ef2415 outdated

20-        self.nodes = []
21-        self.nodes.append(start_node(0, self.options.tmpdir))
22-        self.nodes.append(start_node(1, self.options.tmpdir))
23-        self.nodes.append(start_node(2, self.options.tmpdir))
24-        
25+        self.setup_nodes()

jimmysong commented at 10:29 pm on April 20, 2017:

Deleted?

jnewbery commented at 3:43 pm on April 25, 2017:

setup_nodes() starts the nodes, but doesn’t add connections between them. That’s what we need here.

in test/functional/mempool_limit.py:21 in b9e9ef2415 outdated

23 
24+    def run_test(self):
25         self.txouts = gen_return_txouts()
26 
27-    def run_test(self):
28+        self.relayfee = self.nodes[0].getnetworkinfo()['relayfee']

jimmysong commented at 10:30 pm on April 20, 2017:

Also in __init__?

jnewbery commented at 3:41 pm on April 25, 2017:

ditto

in test/functional/p2p-acceptblock.py:107 in b9e9ef2415 outdated

108-        self.nodes.append(start_node(0, self.options.tmpdir,
109-                                     binary=self.options.testbinary))
110-        self.nodes.append(start_node(1, self.options.tmpdir,
111-                                     ["-whitelist=127.0.0.1"],
112-                                     binary=self.options.testbinary))
113+        self.nodes = self.setup_nodes()

jimmysong commented at 10:31 pm on April 20, 2017:

delete?

jnewbery commented at 3:38 pm on April 25, 2017:

No. The base class setup_network() would connect the two nodes, which we don’t want in this test, so it is correct to override here. However, there is a bug here - I shouldn’t assign self.nodes to the return value, which is None.

in test/functional/p2p-segwit.py:133 in b9e9ef2415 outdated

194         connect_nodes(self.nodes[0], 1)
195-
196-        # Disable segwit's bip9 parameter to simulate upgrading after activation.
197-        self.nodes.append(start_node(2, self.options.tmpdir, ["-whitelist=127.0.0.1", "-bip9params=segwit:0:0"]))
198         connect_nodes(self.nodes[0], 2)
199+        self.sync_all()

jimmysong commented at 10:32 pm on April 20, 2017:

not needed?

jnewbery commented at 3:15 pm on April 25, 2017:

probably not, but I don’t see any harm in calling sync_all() once all connections have been added.

in test/functional/p2p-mempool.py:78 in b9e9ef2415 outdated

76 
77     def setup_network(self):
78-        # Start a node with maxuploadtarget of 200 MB (/24h)
79-        self.nodes = []
80-        self.nodes.append(start_node(0, self.options.tmpdir, ["-peerbloomfilters=0"]))
81+        self.setup_nodes()

jimmysong commented at 10:32 pm on April 20, 2017:

delete?

jnewbery commented at 3:27 pm on April 25, 2017:

Yes, and I can also reduce the number of nodes in this test to 1.

in test/functional/pruning.py:331 in b9e9ef2415 outdated

326+        # Determine default relay fee
327+        self.relayfee = self.nodes[0].getnetworkinfo()["relayfee"]
328+
329+        # Cache for utxos, as the listunspent may take a long time later in the test
330+        self.utxo_cache_0 = []
331+        self.utxo_cache_1 = []

jimmysong commented at 10:33 pm on April 20, 2017:

why did this move from __init__?

jnewbery commented at 3:14 pm on April 25, 2017:

Because I’d really prefer __init__() to only set up the network topology. (eventually I think we should try to move away from individual test cases overriding the __init__() method entirely).

in test/functional/rawtransactions.py:27 in b9e9ef2415 outdated

32-        #proxy.url = url # store URL on proxy for info
33-        #self.nodes.append(proxy)
34-
35-        connect_nodes_bi(self.nodes,0,1)
36-        connect_nodes_bi(self.nodes,1,2)
37+        super().setup_nodes()

jimmysong commented at 10:33 pm on April 20, 2017:

self.setup_nodes() or super().setup_network()?

jnewbery commented at 3:12 pm on April 25, 2017:

should be super().setup_network(). Fixed

jimmysong commented at 10:36 pm on April 20, 2017: contributor

Concept ACK

Added a bunch of nits.

kallewoof commented at 1:52 am on April 21, 2017: member

ACK b9e9ef2415e3df508b0c0663502e60196bc37f0f

Nice, -231 lines of code. :) The listsinceblock stuff looks fine to me. I ran tests and only briefly looked at code changes for other tests.

jimmysong commented at 1:53 pm on April 24, 2017: contributor

@fanquake Can you label this Tests?

fanquake added the label Tests on Apr 24, 2017

jnewbery commented at 4:03 pm on April 25, 2017: member

@jimmysong thanks for the review. I’ve addressed you nits. I’ll rebase the commits locally and then push the new version.

jnewbery force-pushed on Apr 25, 2017

jnewbery commented at 5:26 pm on April 25, 2017: member

Rebased and added a fixup commit to address the nits.

I’m planning to squash these all down to a single commit before merge.

jimmysong commented at 5:30 pm on April 25, 2017: contributor

ACK d64f739b3bc78e67212a252a690556bca8098745

edit: Tested.

jimmysong approved

kallewoof commented at 4:22 am on April 26, 2017: member

utACK d64f739b3bc78e67212a252a690556bca8098745

laanwj commented at 7:37 am on May 1, 2017: member

utACK d64f739, please squash

MarcoFalke commented at 2:32 pm on May 1, 2017: member

Strong concept ACK. Going to review after squash.

jnewbery force-pushed on May 1, 2017

jnewbery commented at 2:47 pm on May 1, 2017: member

Commits squashed. There should be no difference between https://github.com/bitcoin/bitcoin/commit/d64f739b3bc78e67212a252a690556bca8098745 and 20f67c90c001123ab5bca7e70382c5fe26362aa3. Compare https://github.com/jnewbery/bitcoin/tree/pr10198.1 and https://github.com/jnewbery/bitcoin/tree/pr10198.2

in test/functional/bipdersig-p2p.py:50 in 20f67c90c0 outdated

50-    def setup_network(self):
51         # Must set the blockversion for this test
52-        self.nodes = start_nodes(self.num_nodes, self.options.tmpdir,
53-                                 extra_args=[['-whitelist=127.0.0.1', '-blockversion=2']],
54-                                 binary=[self.options.testbinary])
55+        self.extra_args = [[], ['-whitelist=127.0.0.1', '-blockversion=2']]

MarcoFalke commented at 2:53 pm on May 2, 2017:

This does not work for the ComparisionFramework, as it is later overwritten. I don’t understand why this commit does not fail, but I’d still like the bug fixed in this pull.

See test/functional/test_framework/test_framework.py:245L

Additionally, I’d like to see a sanity check in start_nodes:

 0diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
 1index 7730e14..9186c3c 100644
 2--- a/test/functional/test_framework/util.py
 3+++ b/test/functional/test_framework/util.py
 4@@ -354,6 +354,8 @@ def start_nodes(num_nodes, dirname, extra_args=None, rpchost=None, timewait=None
 5     """
 6     if extra_args is None: extra_args = [ None for _ in range(num_nodes) ]
 7     if binary is None: binary = [ None for _ in range(num_nodes) ]
 8+    assert_equal(len(extra_args), num_nodes)
 9+    assert_equal(len(binary), num_nodes)
10     rpcs = []
11     try:
12         for i in range(num_nodes):

jnewbery commented at 5:03 pm on May 2, 2017:

It turns out the comment was wrong (and had presumably been copy-pasted from another test). blockversion is not required in this test, since the bitcoin node is not generating any blocks. I’ve removed the extra_args entirely.

I’ve also added your suggested asserts.

in test/functional/disablewallet.py:22 in 20f67c90c0 outdated

18@@ -19,11 +19,6 @@ def __init__(self):
19         self.setup_clean_chain = True
20         self.num_nodes = 1
21

MarcoFalke commented at 2:56 pm on May 2, 2017:

Please add the missing self.extra_args here, and also add an additional test in this file:

0assert_raises_jsonrpc(-12, "Keypool ran out", lambda: self.nodes[0].generate(1))

jnewbery commented at 5:04 pm on May 2, 2017:

yes, I’ve put the extra_args back in. Good spot.

I don’t understand why adding the “Keypool ran out” assert should be part of this PR?

MarcoFalke commented at 3:01 pm on May 2, 2017: member

utACK 20f67c90c001123ab5bca7e70382c5fe26362aa3, but needs feedback addressed.

MarcoFalke commented at 5:20 pm on May 2, 2017: member

Ok, feel free to squash. Planning to merge this.

jnewbery force-pushed on May 2, 2017

[tests] Remove is_network_split from funtional test cases c9cc76dcaa

jnewbery force-pushed on May 2, 2017

jnewbery commented at 5:32 pm on May 2, 2017: member

squashed and rebased. I’m currently running the extended test locally.

jnewbery commented at 5:55 pm on May 2, 2017: member

extended tests all pass locally. This should be good for merge.

MarcoFalke merged this on May 2, 2017

MarcoFalke closed this on May 2, 2017

MarcoFalke referenced this in commit dc8fc0c73b on May 2, 2017

jnewbery deleted the branch on May 2, 2017

PastaPastaPasta referenced this in commit afd545fd23 on Jun 10, 2019

PastaPastaPasta referenced this in commit 8b1bb469be on Jun 10, 2019

PastaPastaPasta referenced this in commit 832176a775 on Jun 10, 2019

PastaPastaPasta referenced this in commit 19601755ad on Jun 10, 2019

PastaPastaPasta referenced this in commit be12e70a6a on Jun 11, 2019

PastaPastaPasta referenced this in commit 3c99b8afde on Jun 11, 2019

PastaPastaPasta referenced this in commit e9ad12394d on Jun 15, 2019

PastaPastaPasta referenced this in commit 842c27273d on Jun 19, 2019

PastaPastaPasta referenced this in commit 5b654fd720 on Jun 19, 2019

PastaPastaPasta referenced this in commit 627ce6e3ee on Jun 19, 2019

PastaPastaPasta referenced this in commit 47f2aeb761 on Jun 19, 2019

PastaPastaPasta referenced this in commit c468734f87 on Jun 19, 2019

PastaPastaPasta referenced this in commit 7c47cae060 on Jun 19, 2019

PastaPastaPasta referenced this in commit b28cfec782 on Jun 19, 2019

PastaPastaPasta referenced this in commit f7e1d392d5 on Jun 20, 2019

barrystyle referenced this in commit 6993d9d9a3 on Jan 22, 2020

MarcoFalke locked this on Sep 8, 2021

[tests] Remove is_network_split from functional test framework #10198