[tests] Introduce TestNode

jnewbery commented at 1:54 pm on June 30, 2017: member

Continues #10082

TestNode is a class responsible for all state related to a bitcoind node under test. It stores local state, is responsible for tracking the bitcoind process and delegates unrecognised messages to the RPC connection.

This commit changes start_nodes and stop_nodes to start and stop the bitcoind nodes in parallel, making test setup and teardown much faster.

On my vm, this changeset reduces total test_runner runtime for the base set of tests (including building the cache) from 263s to 195s (a 25% speedup). Note that the time reported by test_runner does not include time spent building the cache:

with TestNode:

 0→ date +"%T" ; ./test_runner.py -q ; date +"%T"
 112:48:04
 2..................................................................................................................................................................................................................................................................................................................................
 3TEST                           | STATUS    | DURATION
 4
 5abandonconflict.py             | ✓ Passed  | 12 s
 6bip68-112-113-p2p.py           | ✓ Passed  | 19 s
 7blockchain.py                  | ✓ Passed  | 8 s 
 8bumpfee.py                     | ✓ Passed  | 13 s
 9decodescript.py                | ✓ Passed  | 3 s 
10disablewallet.py               | ✓ Passed  | 3 s 
11disconnect_ban.py              | ✓ Passed  | 6 s 
12fundrawtransaction.py          | ✓ Passed  | 37 s
13getchaintips.py                | ✓ Passed  | 4 s 
14httpbasics.py                  | ✓ Passed  | 3 s 
15import-rescan.py               | ✓ Passed  | 4 s 
16importmulti.py                 | ✓ Passed  | 6 s 
17importprunedfunds.py           | ✓ Passed  | 3 s 
18invalidblockrequest.py         | ✓ Passed  | 4 s 
19invalidtxrequest.py            | ✓ Passed  | 4 s 
20keypool.py                     | ✓ Passed  | 7 s 
21listsinceblock.py              | ✓ Passed  | 4 s 
22listtransactions.py            | ✓ Passed  | 33 s
23mempool_limit.py               | ✓ Passed  | 4 s 
24mempool_persist.py             | ✓ Passed  | 15 s
25mempool_reorg.py               | ✓ Passed  | 4 s 
26mempool_resurrect_test.py      | ✓ Passed  | 3 s 
27mempool_spendcoinbase.py       | ✓ Passed  | 3 s 
28merkle_blocks.py               | ✓ Passed  | 3 s 
29multi_rpc.py                   | ✓ Passed  | 4 s 
30net.py                         | ✓ Passed  | 3 s 
31nulldummy.py                   | ✓ Passed  | 3 s 
32p2p-compactblocks.py           | ✓ Passed  | 28 s
33p2p-fullblocktest.py           | ✓ Passed  | 126 s
34p2p-leaktests.py               | ✓ Passed  | 8 s 
35p2p-mempool.py                 | ✓ Passed  | 3 s 
36p2p-segwit.py                  | ✓ Passed  | 59 s
37p2p-versionbits-warning.py     | ✓ Passed  | 8 s 
38preciousblock.py               | ✓ Passed  | 3 s 
39prioritise_transaction.py      | ✓ Passed  | 5 s 
40proxy_test.py                  | ✓ Passed  | 3 s 
41rawtransactions.py             | ✓ Passed  | 9 s 
42receivedby.py                  | ✓ Passed  | 19 s
43reindex.py                     | ✓ Passed  | 12 s
44rest.py                        | ✓ Passed  | 9 s 
45rpcnamedargs.py                | ✓ Passed  | 3 s 
46segwit.py                      | ✓ Passed  | 7 s 
47sendheaders.py                 | ✓ Passed  | 24 s
48signmessages.py                | ✓ Passed  | 3 s 
49signrawtransactions.py         | ✓ Passed  | 3 s 
50txn_clone.py                   | ✓ Passed  | 4 s 
51txn_doublespend.py --mineblock | ✓ Passed  | 4 s 
52uptime.py                      | ✓ Passed  | 3 s 
53wallet-accounts.py             | ✓ Passed  | 3 s 
54wallet-dump.py                 | ✓ Passed  | 7 s 
55wallet-encryption.py           | ✓ Passed  | 8 s 
56wallet-hd.py                   | ✓ Passed  | 15 s
57wallet.py                      | ✓ Passed  | 31 s
58walletbackup.py                | ✓ Passed  | 104 s
59zapwallettxes.py               | ✓ Passed  | 9 s 
60zmq_test.py                    | ○ Skipped | 0 s 
61
62ALL                            | ✓ Passed  | 735 s (accumulated)
63Runtime: 189 s
64
6512:51:19

master:

 0→ date +"%T" ; ./test_runner.py -q ; date +"%T"
 112:40:13
 2..........................................................................................................................................................................................................................................................................................................................................................................................................................................
 3TEST                           | STATUS    | DURATION
 4
 5abandonconflict.py             | ✓ Passed  | 15 s
 6bip68-112-113-p2p.py           | ✓ Passed  | 19 s
 7blockchain.py                  | ✓ Passed  | 8 s
 8bumpfee.py                     | ✓ Passed  | 20 s
 9decodescript.py                | ✓ Passed  | 3 s
10disablewallet.py               | ✓ Passed  | 3 s
11disconnect_ban.py              | ✓ Passed  | 8 s
12fundrawtransaction.py          | ✓ Passed  | 36 s
13getchaintips.py                | ✓ Passed  | 11 s
14httpbasics.py                  | ✓ Passed  | 7 s
15import-rescan.py               | ✓ Passed  | 16 s
16importmulti.py                 | ✓ Passed  | 10 s
17importprunedfunds.py           | ✓ Passed  | 5 s
18invalidblockrequest.py         | ✓ Passed  | 4 s
19invalidtxrequest.py            | ✓ Passed  | 3 s
20keypool.py                     | ✓ Passed  | 7 s
21listsinceblock.py              | ✓ Passed  | 11 s
22listtransactions.py            | ✓ Passed  | 37 s
23mempool_limit.py               | ✓ Passed  | 4 s
24mempool_persist.py             | ✓ Passed  | 23 s
25mempool_reorg.py               | ✓ Passed  | 7 s
26mempool_resurrect_test.py      | ✓ Passed  | 3 s
27mempool_spendcoinbase.py       | ✓ Passed  | 3 s
28merkle_blocks.py               | ✓ Passed  | 10 s
29multi_rpc.py                   | ✓ Passed  | 6 s
30net.py                         | ✓ Passed  | 6 s
31nulldummy.py                   | ✓ Passed  | 3 s
32p2p-compactblocks.py           | ✓ Passed  | 30 s
33p2p-fullblocktest.py           | ✓ Passed  | 126 s
34p2p-leaktests.py               | ✓ Passed  | 8 s
35p2p-mempool.py                 | ✓ Passed  | 3 s
36p2p-segwit.py                  | ✓ Passed  | 62 s
37p2p-versionbits-warning.py     | ✓ Passed  | 8 s
38preciousblock.py               | ✓ Passed  | 8 s
39prioritise_transaction.py      | ✓ Passed  | 7 s
40proxy_test.py                  | ✓ Passed  | 10 s
41rawtransactions.py             | ✓ Passed  | 15 s
42receivedby.py                  | ✓ Passed  | 28 s
43reindex.py                     | ✓ Passed  | 12 s
44rest.py                        | ✓ Passed  | 12 s
45rpcnamedargs.py                | ✓ Passed  | 3 s
46segwit.py                      | ✓ Passed  | 12 s
47sendheaders.py                 | ✓ Passed  | 26 s
48signmessages.py                | ✓ Passed  | 3 s
49signrawtransactions.py         | ✓ Passed  | 3 s
50txn_clone.py                   | ✓ Passed  | 10 s
51txn_doublespend.py --mineblock | ✓ Passed  | 10 s
52uptime.py                      | ✓ Passed  | 3 s
53wallet-accounts.py             | ✓ Passed  | 3 s
54wallet-dump.py                 | ✓ Passed  | 6 s
55wallet-encryption.py           | ✓ Passed  | 8 s
56wallet-hd.py                   | ✓ Passed  | 18 s
57wallet.py                      | ✓ Passed  | 69 s
58walletbackup.py                | ✓ Passed  | 130 s
59zapwallettxes.py               | ✓ Passed  | 15 s
60zmq_test.py                    | ○ Skipped | 0 s
61
62ALL                            | ✓ Passed  | 936 s (accumulated)
63Runtime: 242 s
64
6512:44:36

fanquake added the label Tests on Jun 30, 2017

jnewbery commented at 4:04 pm on June 30, 2017: member

Wallet tests were failing on Travis because bitcoind startup was timing out. Increasing timeout from 2.5s to 10s for Travis.

meshcollider commented at 6:28 am on July 4, 2017: contributor

Concept ACK https://github.com/bitcoin/bitcoin/pull/10711/commits/e538dbf42d20338f758f415a37c07555403ef6c0, looks good but haven’t tested or gone through thoroughly

laanwj commented at 3:44 pm on July 10, 2017: member

The speed-up is nice, also like the idea of launching nodes in parallel. Concept ACK.

in test/functional/test_framework/util.py:247 in dcc0ed7434 outdated

243@@ -244,6 +244,7 @@ def get_auth_cookie(datadir, n):
244                 if line.startswith("rpcpassword="):
245                     assert password is None  # Ensure that there is only one rpcpassword line
246                     password = line.split("=")[1].strip("\n")
247+    # print(os.path.join(datadir, "regtest", ".cookie"))

jtimon commented at 6:10 pm on July 10, 2017:

Remove this?

jnewbery commented at 2:12 pm on July 11, 2017:

thanks. Removed

jnewbery force-pushed on Jul 11, 2017

jnewbery commented at 2:12 pm on July 11, 2017: member

fixed @jtimon’s review comment and rebased.

laanwj removed this from the "Blockers" column in a project

jnewbery force-pushed on Jul 21, 2017

jnewbery force-pushed on Jul 24, 2017

jnewbery commented at 3:35 pm on July 24, 2017: member

rebased with TestNode changes for multiwallet.

MarcoFalke commented at 10:04 pm on August 8, 2017: member

Needs rebase

jnewbery force-pushed on Aug 9, 2017

jnewbery commented at 8:49 pm on August 9, 2017: member

rebased @MarcoFalke @kallewoof @jimmysong @NicolasDorier I’d love some review of this if any of you have time.

in test/functional/test_framework/test_node.py:71 in 05259b0f2a outdated

66+
67+    def wait_for_rpc_connection(self):
68+        """Sets up an RPC connection to the bitcoind process. Returns False if unable to connect."""
69+
70+        attempts = 40
71+        while attempts > 0:

jimmysong commented at 9:06 pm on August 9, 2017:

This is a little harder to read than something like this:

for attempt in range(40):
    ...
    time.sleep(0.25)

jnewbery commented at 10:00 pm on August 9, 2017:

Thanks. changed

in test/functional/test_framework/test_node.py:101 in 05259b0f2a outdated

 96+        if not self.running:
 97+            return
 98+        self.log.debug("Stopping node")
 99+        try:
100+            self.stop()
101+        except http.client.CannotSendRequest as e:

jimmysong commented at 9:07 pm on August 9, 2017:

no need for as e

jnewbery commented at 10:00 pm on August 9, 2017:

Yep

in test/functional/test_framework/test_node.py:96 in e25b0cf5ea outdated

90@@ -91,6 +91,12 @@ def wait_for_rpc_connection(self):
91             attempts -= 1
92         raise AssertionError("Unable to connect to bitcoind")
93 
94+    def get_wallet_rpc(self, wallet_name):
95+        assert self.rpc_connected
96+        assert self.rpc
97+        wallet_path = "wallet/%s" % wallet_name
98+        return self.rpc / wallet_path

jimmysong commented at 9:12 pm on August 9, 2017:

What does this do? wallet_path is for sure a string, but how do you divide the rpc object by a string?

jnewbery commented at 9:59 pm on August 9, 2017:

By magic :)

Take a look at https://github.com/bitcoin/bitcoin/blob/e526ca6284b9e13be1b912b80dd73a34e739b539/test/functional/test_framework/authproxy.py#L195 and https://github.com/bitcoin/bitcoin/blob/e526ca6284b9e13be1b912b80dd73a34e739b539/test/functional/test_framework/coverage.py#L59

The interface here is supposed to be similar to Python’s pathlib

This functionality was added to support the multiwallet endpoints in https://github.com/bitcoin/bitcoin/commit/979d0b8a6533de58b6352666d90fff33841db63d . Take a look at the multiwallet.py test to see how it’s currently used.

jimmysong commented at 9:12 pm on August 9, 2017: contributor

utACK. Couple of nits and one thing I don’t get.

jnewbery force-pushed on Aug 9, 2017

NicolasDorier commented at 0:40 am on August 10, 2017: contributor

utACK 2d189c89f6ecdb846a046bdd57883e190df7b3bf I am a big fan of this idea, it makes writing tests so much easier.

in test/functional/test_framework/test_framework.py:200 in 2d189c89f6 outdated

196@@ -203,67 +197,60 @@ def main(self):
197 
198     # Public helper methods. These can be accessed by the subclass test scripts.
199 
200-    def start_node(self, i, dirname, extra_args=None, rpchost=None, timewait=None, binary=None, stderr=None):
201+    def start_node(self, i, dirname, extra_args=[], rpchost=None, timewait=None, binary=None, stderr=None):

kallewoof commented at 2:20 am on August 10, 2017:

Why not allow None for extra_args? I’m generally wary of passing in objects as defaults, as if those default objects are ever altered, the altered version will be passed as the default in future calls.

E.g. try the following snippet:

0def foo(x=[]):
1    x.append(5)
2    print(x)
3
4foo()
5foo()
6foo()

You will get as output

0[5]
1[5, 5]
2[5, 5, 5]

NicolasDorier commented at 2:50 am on August 10, 2017:

wow I did not know.

But even if it was not the case, I think that the user should be able to pass null (Nothing), when he means “default value”.

This code can be easily fixed by keeping the default value to Nothing and first line, checking if extra_args is nothing, if that is the case, init extra_args to an empty array.

jimmysong commented at 2:58 am on August 10, 2017:

Totally didn’t catch that, but I’ve run into this in the past and it’s pretty nasty to debug unless you know to look for it. I agree with the others, put this back to None.

kallewoof commented at 2:58 am on August 10, 2017:

null in Python is None, so yeah, I think None should be the default here and just set it to [] if it’s None at the start.

jnewbery commented at 4:12 pm on August 14, 2017:

Nice. Thanks @kallewoof , I wasn’t aware of that, but it makes perfect sense.

Changed the default argument to None as suggested.

kallewoof approved

kallewoof commented at 2:22 am on August 10, 2017: member

utACK with one nit.

jnewbery force-pushed on Aug 14, 2017

in test/functional/multiwallet.py:41 in cb8579bd3f outdated

34@@ -35,11 +35,15 @@ def run_test(self):
35 
36         self.nodes[0] = self.start_node(0, self.options.tmpdir, self.extra_args[0])
37 
38-        w1 = self.nodes[0] / "wallet/w1"
39+        w1 = self.nodes[0].get_wallet_rpc("w1")
40+        w2 = self.nodes[0].get_wallet_rpc("w2")
41+        w3 = self.nodes[0].get_wallet_rpc("w3")
42+        wallet_bad = self.nodes[0].get_wallet_rpc("wallet/bad")

MarcoFalke commented at 1:39 pm on August 15, 2017:

nit: Should be get_wallet_rpc('bad') to qualify for refactor-only.

jnewbery commented at 5:10 pm on August 15, 2017:

fixed

in test/functional/test_framework/test_node.py:62 in cb8579bd3f outdated

57+        assert self.rpc_connected and self.rpc is not None, "Error: no RPC connection"
58+        return self.rpc.__getattr__(*args, **kwargs)
59+
60+    def start(self):
61+        """Start the node."""
62+        # import pdb; pdb.set_trace()

MarcoFalke commented at 2:11 pm on August 15, 2017:

nit: debug artifact

jnewbery commented at 5:10 pm on August 15, 2017:

removed

in test/functional/test_framework/test_framework.py:238 in cb8579bd3f outdated

253-            self.nodes = []
254             raise
255-        return rpcs
256+
257+        if self.options.coveragedir is not None:
258+            coverage.write_all_rpc_commands(self.options.coveragedir, node.rpc)

MarcoFalke commented at 2:19 pm on August 15, 2017:

missing for node in nodes?

jnewbery commented at 5:11 pm on August 15, 2017:

fixed

in test/functional/test_framework/test_framework.py:252 in cb8579bd3f outdated

280-        for i in range(len(self.nodes)):
281-            self.stop_node(i)
282-        assert not self.bitcoind_processes.values()  # All connections must be gone now
283+        for node in self.nodes:
284+            node.stop_node()
285+        # All connections must be gone now

MarcoFalke commented at 2:24 pm on August 15, 2017:

nit: remove outdated comment

jnewbery commented at 5:11 pm on August 15, 2017:

removed

in test/functional/test_framework/test_node.py:80 in cb8579bd3f outdated

75+                self.rpc.getblockcount()
76+                # If the call to getblockcount() succeeds then the RPC connection is up
77+                self.rpc_connected = True
78+                self.url = self.rpc.url
79+                self.log.debug("RPC successfully started")
80+                return True

MarcoFalke commented at 2:33 pm on August 15, 2017:

nit: No need to return a value if it is never used. Can be replaced by a plain return.

jnewbery commented at 5:10 pm on August 15, 2017:

fixed

in test/functional/test_framework/test_framework.py:228 in cb8579bd3f outdated

238-        rpcs = []
239+        nodes = []
240         try:
241             for i in range(num_nodes):
242-                rpcs.append(self.start_node(i, dirname, extra_args[i], rpchost, timewait=timewait, binary=binary[i]))
243+                nodes.append(TestNode(i, dirname, extra_args[i], rpchost, timewait=timewait, binary=binary[i], stderr=None, mocktime=self.mocktime, coverage_dir=self.options.coveragedir))

MarcoFalke commented at 2:48 pm on August 15, 2017:

stderr must be set to not None. Otherwise, we will ignore any output to stderr, no?

jnewbery commented at 5:11 pm on August 15, 2017:

This is the same as the existing behaviour. start_nodes() currently calls start_node() without the stderr argument set, so it defaults to None.

This could be changed in a future PR.

MarcoFalke changes_requested

MarcoFalke commented at 3:00 pm on August 15, 2017: member

utACK cb8579bd3fb96d6204c07bf3b2d2476d0f077a91, but needs some nits and apparent regressions fixed.

[tests] Introduce TestNode

TestNode is a class responsible for all state related to a bitcoind node
under test. It stores local state, is responsible for tracking the
bitcoind process and delegates unrecognised messages to the RPC
connection.

This commit changes start_nodes and stop_nodes to start and stop the
bitcoind nodes in parallel, making test setup and teardown much faster.

7897338918

jnewbery force-pushed on Aug 15, 2017

jnewbery commented at 5:14 pm on August 15, 2017: member

Thanks for the review @MarcoFalke - I’ve addressed all your nits (except the stderr in start_nodes() which I think maintains existing behaviour).

I’ve updated and squashed the commits. Unsquashed history is here: https://github.com/jnewbery/bitcoin/tree/pr10711.1 if you just want to see what’s changed.

MarcoFalke commented at 9:32 pm on August 15, 2017: member

re-utACK 7897338, thanks for addressing the feedback.

I keep a local copy of all commits, so no need to provide branches of the history for me.

MarcoFalke merged this on Aug 15, 2017

MarcoFalke closed this on Aug 15, 2017

MarcoFalke referenced this in commit 85aec87b11 on Aug 15, 2017

jnewbery deleted the branch on Aug 15, 2017

MarcoFalke referenced this in commit 4ae6d0fbef on Aug 24, 2017

MarcoFalke referenced this in commit fc2aa09cf3 on Oct 3, 2017

PastaPastaPasta referenced this in commit 74ab3e9dae on Sep 19, 2019

PastaPastaPasta referenced this in commit f524b78783 on Sep 19, 2019

PastaPastaPasta referenced this in commit ee44950e7d on Sep 23, 2019

PastaPastaPasta referenced this in commit 78c04e8126 on Sep 23, 2019

PastaPastaPasta referenced this in commit e221b458ca on Sep 24, 2019

PastaPastaPasta referenced this in commit 095eb5d1cf on Sep 24, 2019

codablock referenced this in commit f55da3aa54 on Sep 24, 2019

codablock referenced this in commit 1adc2001a8 on Sep 24, 2019

barrystyle referenced this in commit 1195059eac on Jan 22, 2020

barrystyle referenced this in commit e8bcd62ce1 on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021

[tests] Introduce TestNode #10711