Merge bitcoin#11121: TestNode tidyups

7148b74 [tests] Functional tests must explicitly set num_nodes (John Newbery)
5448a14 [tests] don't override __init__() in individual tests (John Newbery)
6cf094a [tests] Avoid passing around member variables in test_framework (John Newbery)
36b6268 [tests] TestNode: separate add_node from start_node (John Newbery)
be2a2ab [tests] fix - use rpc_timeout as rpc timeout (John Newbery)

Pull request description:

  Some additional tidyups after the introduction of TestNode:

  - commit 1 makes TestNode use the correct rpc timeout. This should have been included in bitcoin#11077
  - commit 2 separates `add_node()` from `start_node()` as originally discussed here: bitcoin#10556 (comment) with @kallewoof . The test writer no longer needs to assign to `self.nodes` when starting/stopping nodes.
  - commit 3 adds a `set_test_params()` method, so individual tests don't need to override `__init__()` and call `super().__init__()`

Tree-SHA512: 0adb030623b96675b5c29e2890ce99ccd837ed05f721d0c91b35378c5ac01b6658174aac12f1f77402e1d38b61f39b3c43b4df85c96952565dde1cda05b0db84

Author: MarcoFalke

test/functional/README.md

@@ -24,8 +24,8 @@ don't have test cases for.
 - Use a module-level docstring to describe what the test is testing, and how it
   is testing it.
 - When subclassing the BitcoinTestFramwork, place overrides for the
-  `__init__()`, and `setup_xxxx()` methods at the top of the subclass, then
-  locally-defined helper methods, then the `run_test()` method.
+  `set_test_params()`, `add_options()` and `setup_xxxx()` methods at the top of
+  the subclass, then locally-defined helper methods, then the `run_test()` method.
 
 #### General test-writing advice
 
@@ -36,7 +36,7 @@ don't have test cases for.
 - Avoid stop-starting the nodes multiple times during the test if possible. A
   stop-start takes several seconds, so doing it several times blows up the
   runtime of the test.
-- Set the `self.setup_clean_chain` variable in `__init__()` to control whether
+- Set the `self.setup_clean_chain` variable in `set_test_params()` to control whether
   or not to use the cached data directories. The cached data directories
   contain a 200-block pre-mined blockchain and wallets for four nodes. Each node
   has 25 mature blocks (25x500=12500 DASH) in its wallet.

test/functional/abandonconflict.py

@@ -14,10 +14,8 @@
 from test_framework.util import *
 
 class AbandonConflictTest(BitcoinTestFramework):
-    def __init__(self):
-        super().__init__()
+    def set_test_params(self):
         self.num_nodes = 2
-        self.setup_clean_chain = False
         self.extra_args = [["-minrelaytxfee=0.00001"], []]
 
     def run_test(self):
@@ -74,7 +72,7 @@ def run_test(self):
         # Restart the node with a higher min relay fee so the parent tx is no longer in mempool
         # TODO: redo with eviction
         self.stop_node(0)
-        self.nodes[0] = self.start_node(0, self.options.tmpdir, ["-minrelaytxfee=0.0001"])
+        self.start_node(0, extra_args=["-minrelaytxfee=0.0001"])
 
         # Verify txs no longer in either node's mempool
         assert_equal(len(self.nodes[0].getrawmempool()), 0)
@@ -101,7 +99,7 @@ def run_test(self):
 
         # Verify that even with a low min relay fee, the tx is not reaccepted from wallet on startup once abandoned
         self.stop_node(0)
-        self.nodes[0] = self.start_node(0, self.options.tmpdir, ["-minrelaytxfee=0.00001"])
+        self.start_node(0, extra_args=["-minrelaytxfee=0.00001"])
         assert_equal(len(self.nodes[0].getrawmempool()), 0)
         assert_equal(self.nodes[0].getbalance(), balance)
 
@@ -121,7 +119,7 @@ def run_test(self):
 
         # Remove using high relay fee again
         self.stop_node(0)
-        self.nodes[0] = self.start_node(0, self.options.tmpdir, ["-minrelaytxfee=0.0001"])
+        self.start_node(0, extra_args=["-minrelaytxfee=0.0001"])
         assert_equal(len(self.nodes[0].getrawmempool()), 0)
         newbalance = self.nodes[0].getbalance()
         assert_equal(newbalance, balance - Decimal("24.9996"))

test/functional/assumevalid.py

@@ -54,16 +54,17 @@ def send_header_for_blocks(self, new_blocks):
         self.send_message(headers_message)
 
 class AssumeValidTest(BitcoinTestFramework):
-    def __init__(self):
-        super().__init__()
+    def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 3
+        self.extra_args = ["-dip3params=9000:9000", "-checkblockindex=0"]
 
     def setup_network(self):
+        self.add_nodes(3)
         # Start node0. We don't start the other nodes yet since
         # we need to pre-mine a block with an invalid transaction
         # signature so we can pass in the block hash as assumevalid.
-        self.nodes = [self.start_node(0, self.options.tmpdir, ["-dip3params=9000:9000", "-checkblockindex=0"])]
+        self.start_node(0)
 
     def send_blocks_until_disconnected(self, node):
         """Keep sending blocks to the node until we're disconnected."""
@@ -168,15 +169,13 @@ def run_test(self):
             height += 1
 
         # Start node1 and node2 with assumevalid so they accept a block with a bad signature.
-        self.nodes.append(self.start_node(1, self.options.tmpdir,
-                                     ["-assumevalid=" + hex(block102.sha256), "-dip3params=9000:9000", "-checkblockindex=0"]))
+        self.start_node(1, extra_args=["-assumevalid=" + hex(block102.sha256)])
         node1 = BaseNode()  # connects to node1
         connections.append(NodeConn('127.0.0.1', p2p_port(1), self.nodes[1], node1))
         node1.add_connection(connections[1])
         node1.wait_for_verack()
 
-        self.nodes.append(self.start_node(2, self.options.tmpdir,
-                                     ["-assumevalid=" + hex(block102.sha256), "-dip3params=9000:9000", "-checkblockindex=0"]))
+        self.start_node(2, extra_args=["-assumevalid=" + hex(block102.sha256)])
         node2 = BaseNode()  # connects to node2
         connections.append(NodeConn('127.0.0.1', p2p_port(2), self.nodes[2], node2))
         node2.add_connection(connections[2])

test/functional/bip65-cltv-p2p.py

@@ -60,9 +60,7 @@ def create_transaction(node, coinbase, to_address, amount):
     return tx
 
 class BIP65Test(BitcoinTestFramework):
-
-    def __init__(self):
-        super().__init__()
+    def set_test_params(self):
         self.num_nodes = 1
         self.extra_args = [['-whitelist=127.0.0.1', '-dip3params=9000:9000']]
         self.setup_clean_chain = True

test/functional/bip68-112-113-p2p.py

@@ -92,10 +92,9 @@ def all_rlt_txs(txarray):
     return txs
 
 class BIP68_112_113Test(ComparisonTestFramework):
-    def __init__(self):
-        super().__init__()
+    def set_test_params(self):
         self.num_nodes = 1
-        # Must set the blockversion for this test
+        self.setup_clean_chain = True
         # Must also set '-maxtipage=600100' to allow syncing from very old blocks
         # and '-dip3params=2000:2000' to create pre-dip3 blocks only
         self.extra_args = [['-whitelist=127.0.0.1', '-blockversion=4', '-maxtipage=600100', '-dip3params=2000:2000']]

test/functional/bip68-sequence.py

@@ -17,10 +17,8 @@
 NOT_FINAL_ERROR = "64: non-BIP68-final"
 
 class BIP68Test(BitcoinTestFramework):
-    def __init__(self):
-        super().__init__()
+    def set_test_params(self):
         self.num_nodes = 2
-        self.setup_clean_chain = False
         self.extra_args = [[], ["-acceptnonstdtxn=0"]]
 
     def run_test(self):

test/functional/bip9-softforks.py

@@ -28,11 +28,10 @@
 from test_framework.script import CScript, OP_1NEGATE, OP_CHECKSEQUENCEVERIFY, OP_DROP
 
 class BIP9SoftForksTest(ComparisonTestFramework):
-
-    def __init__(self):
-        super().__init__()
+    def set_test_params(self):
         self.num_nodes = 1
         self.extra_args = [['-whitelist=127.0.0.1', '-dip3params=9000:9000']]
+        self.setup_clean_chain = True
 
     def run_test(self):
         self.test = TestManager(self, self.options.tmpdir)
@@ -240,6 +239,7 @@ def test_BIP(self, bipName, activated_version, invalidate, invalidatePostSignatu
         # Restart all
         self.test.clear_all_connections()
         self.stop_nodes()
+        self.nodes = []
         shutil.rmtree(self.options.tmpdir + "/node0")
         self.setup_chain()
         self.setup_network()

test/functional/bipdersig-p2p.py

@@ -49,9 +49,7 @@ def create_transaction(node, coinbase, to_address, amount):
 
 
 class BIP66Test(BitcoinTestFramework):
-
-    def __init__(self):
-        super().__init__()
+    def set_test_params(self):
         self.num_nodes = 1
         self.extra_args = [['-whitelist=127.0.0.1', '-dip3params=9000:9000']]
         self.setup_clean_chain = True

test/functional/blockchain.py

@@ -30,12 +30,8 @@
     assert_is_hash_string,
 )
 
-
 class BlockchainTest(BitcoinTestFramework):
-
-    def __init__(self):
-        super().__init__()
-        self.setup_clean_chain = False
+    def set_test_params(self):
         self.num_nodes = 1
         self.extra_args = [['-stopatheight=207']]
 
@@ -145,7 +141,7 @@ def _test_stopatheight(self):
             pass  # The node already shut down before response
         self.log.debug('Node should stop at this height...')
         self.nodes[0].process.wait(timeout=BITCOIND_PROC_WAIT_TIMEOUT)
-        self.nodes[0] = self.start_node(0, self.options.tmpdir)
+        self.start_node(0)
         assert_equal(self.nodes[0].getblockcount(), 207)
 
 

test/functional/create_cache.py

@@ -12,13 +12,10 @@
 from test_framework.test_framework import BitcoinTestFramework
 
 class CreateCache(BitcoinTestFramework):
+    # Test network and test nodes are not required:
 
-    def __init__(self):
-        super().__init__()
-
-        # Test network and test nodes are not required:
+    def set_test_params(self):
         self.num_nodes = 0
-        self.nodes = []
 
     def setup_network(self):
         pass