Some additional tidyups after the introduction of TestNode:
add_node() from start_node() as originally discussed here: #10556 (review) with @kallewoof . The test writer no longer needs to assign to self.nodes when starting/stopping nodes.set_test_params() method, so individual tests don't need to override __init__() and call super().__init__()7 | @@ -8,12 +8,6 @@ 8 | from test_framework.util import * 9 | 10 | class TxnMallTest(BitcoinTestFramework): 11 | - 12 | - def __init__(self): 13 | - super().__init__() 14 | - self.num_nodes = 4 15 | - self.setup_clean_chain = False
I prefer to explicitly set the network topology and initial state of the chain.
Faint agreement. It's easy to forget and start up 4 nodes when you only really need 2, for example. Not seeing the number of nodes makes that even harder to remember.
Sounds good. I've added a new commit that makes setting self.num_nodes mandatory. I haven't done the same for self.setup_clean_chain, since I think it makes more sense for that to have a default. If people feel strongly, it can be added in a future commit.
174 | @@ -202,51 +175,91 @@ def main(self): 175 | logging.shutdown() 176 | sys.exit(TEST_EXIT_FAILED) 177 | 178 | - # Public helper methods. These can be accessed by the subclass test scripts. 179 | + # Methods to override in subclass test scripts. 180 | + def set_test_params(self): 181 | + """Override this method to change default values for number of nodes, topology, etc""" 182 | + pass
should raise RuntimeError('Not implemented') to enforce the docstring.
Yes - done
Concept ACK after quick glance
230 | 231 | - def start_nodes(self, num_nodes, dirname, extra_args=None, rpchost=None, timewait=None, binary=None): 232 | - """Start multiple bitcoinds, return RPC connections to them""" 233 | + # Public helper methods. These can be accessed by the subclass test scripts. 234 | + 235 | + def add_nodes(self, num_nodes, dirname, extra_args=None, rpchost=None, timewait=None, binary=None):
Since self knows about self.options.tmpdir, and all calls to self.add_nodes passes self.options.tmpdir as dirname argument, it seems clean to just give it a default None and to set to self.options.tmpdir if None in add_nodes.
Or perhaps remove it entirely and always use self.options.tmpdir. Will it ever be anything else?
Good point. Thanks! Added this in a new commit.
60 | @@ -61,9 +61,13 @@ def __getattr__(self, *args, **kwargs): 61 | assert self.rpc_connected and self.rpc is not None, "Error: no RPC connection" 62 | return self.rpc.__getattr__(*args, **kwargs) 63 | 64 | - def start(self): 65 | + def start(self, extra_args, stderr=None):
extra_args=None, stderr=None maybe?
TestNode.start is only ever called by TestFramework.start_node, which has its own default for extra_args, so not strictly necessary, but I agree having a default here makes it clearer. Added.
21 | - self.num_nodes = 4 22 | self.extra_args = [['-usehd={:d}'.format(i%2==0)] for i in range(4)] 23 | 24 | def setup_network(self): 25 | - self.nodes = self.start_nodes(3, self.options.tmpdir, self.extra_args[:3]) 26 | + self.add_nodes(4, self.options.tmpdir, self.extra_args)
Why does it not just add_nodes(3, ..) here and then add/start/connect the fourth node below? That would remove a lot of necessary [self.nodes[0:3]] stuff.
Because there's not currently a method to add a single node. The model after this PR is to add all nodes at the top of the test and then start/stop them as required during the test.
We could potentially add an add_node() method to be called during the test, but lets leave that for a future PR.
utACK 06bde74f5ec8660975275f8594aee3a99064e327
@MarcoFalke @kallewoof - thanks for the review. I think I've responded to all your comments.
New commits pushed, and rebased on master.
utACK df824a9
53 | @@ -54,16 +54,16 @@ def send_header_for_blocks(self, new_blocks): 54 | self.send_message(headers_message) 55 | 56 | class AssumeValidTest(BitcoinTestFramework): 57 | - def __init__(self): 58 | - super().__init__() 59 | + def set_test_params(self): 60 | self.setup_clean_chain = True
It seems there are more setup_clean_chain = True so why not make it the default?
I haven't made any changes to setup_clean_chain. See earlier comment:
I haven't done the same for self.setup_clean_chain, since I think it makes more sense for that to have a default. If people feel strongly, it can be added in a future commit.
Feel free to open a PR to change/remove the default if you think that's necessary. I don't think it's required for this PR.
Have you thought of not having any node started automatically? There are some tests that start with stop_node(0)..
So eachrun_test(self) the nodes are explicitly started, num_nodes can go away, even setup_clean_chain would be a start_node argument.
Thanks for the reivew @promag
Have you thought of not having any node started automatically? There are some tests that start with stop_node(0)
Tests that don't need their nodes to start should override the setup_network() or setup_nodes() methods. The vast majority of tests require nodes to be started at the top of the test, so that's the default.
setup_clean_chain would be a start_node argument.
setup_clean_chain is a property of the pre-mined chain. I don't think we should change it to be a property of the individual node (if you disagree please open a separate PR to demonstrate the change).
47 | self.nodes[1].node_encrypt_wallet(WALLET_PASSPHRASE) 48 | - self.nodes[1] = self.start_node(1, self.options.tmpdir, extra_args[1]) 49 | + self.start_node(1) 50 | self.nodes[1].walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT) 51 | 52 | connect_nodes_bi(self.nodes, 0, 1)
This is already done by setup_network.
Same for the line below.
L41 stops node 1 (the encryptwallet RPC shuts down the node), so I believe all the p2p connections are lost and we need to reconnect the nodes after restarting node 1.
I've tested removing these two lines and the test fails (the next call to sync_all() fails)
Sorry, missed that the node shuts down.
312 | @@ -307,15 +313,15 @@ def has_block(index): 313 | 314 | # stop node, start back up with auto-prune at 550MB, make sure still runs 315 | self.stop_node(node_number) 316 | - self.nodes[node_number] = self.start_node(node_number, self.options.tmpdir, ["-prune=550"], timewait=900) 317 | + self.start_node(node_number, extra_args=["-prune=550"])
You are removing timewait=900 from all calls but the first. I assume this does not cause issues?
rpc_timeout is a property of TestNode set when instantiating the python object, not when starting up the bitcoind node.
210 | @@ -208,8 +211,10 @@ def run_test(self): 211 | zeroValueTxid= decRawTx['txid'] 212 | sendResp = self.nodes[1].sendrawtransaction(signedRawTx['hex']) 213 | 214 | + # self.sync_all([self.nodes[0:3]])
debug statement?
derp. Fixed
210 | @@ -208,8 +211,10 @@ def run_test(self): 211 | zeroValueTxid= decRawTx['txid'] 212 | sendResp = self.nodes[1].sendrawtransaction(signedRawTx['hex']) 213 | 214 | + # self.sync_all([self.nodes[0:3]]) 215 | self.sync_all() 216 | self.nodes[1].generate(1) #mine a block 217 | + # self.sync_all([self.nodes[0:3]])
debug statement?
yep
260 | + self.start_node(2) 261 | connect_nodes_bi(self.nodes,0,1) 262 | connect_nodes_bi(self.nodes,1,2) 263 | connect_nodes_bi(self.nodes,0,2) 264 | - sync_blocks(self.nodes) 265 | + sync_blocks(self.nodes[0:2])
why not self.nodes[:3]?
fixed
264 | - sync_blocks(self.nodes) 265 | + sync_blocks(self.nodes[0:2]) 266 | 267 | self.nodes[0].generate(1) 268 | - sync_blocks(self.nodes) 269 | + sync_blocks(self.nodes[0:2])
why not self.nodes[:3]?
fixed
24 | - def __init__(self): 25 | - super().__init__() 26 | + def set_test_params(self): 27 | self.setup_clean_chain = True 28 | - self.num_nodes = 2 29 | + self.num_nodes = 3
why is this changed in a refactoring commit?
fixed
utACK df824a92121eb68849e474e87098a441d4e52926
Separates the act of creating a TestNode object from starting the node.
The test_framework now keeps track of its list of TestNodes, and test
writers can call start_node() and stop_node() without having to update
the self.nodes list.
Almost all test scripts currently need to override the __init__()
method. When they do that they need to call into super().__init__() as
the base class does some generic initialization.
This commit makes the base class __init__() call into set_test_params()
method. Individual test cases can override set_test_params() to setup
their test parameters.
Thanks @MarcoFalke . Review comments addressed.
re-utACK 7148b74dc39110f53c665b94fa9d994c6ad6dc1c