test: add extra_args to BitcoinTestFramework class #26617

problem

If you try to add extra_args when using TestShell, you will get the following error:

>>> import sys
>>> 
>>> sys.path.insert(0, "/home/josibake/bitcoin/test/functional")
>>> 
>>> from test_framework.test_shell import TestShell
>>> test = TestShell().setup(num_nodes=2, extra_args=[[],['-fallbackfee=0.0002']])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/josibake/bitcoin/test/functional/test_framework/test_shell.py", line 41, in setup
    raise KeyError(key + " not a valid parameter key!")
KeyError: 'extra_args not a valid parameter key!'
>>> 

solution

add self.extra_args = None so that extra_args is recognized as a valid parameter to be passed to BitcoinTestFramework

>>> import sys
>>> 
>>> sys.path.insert(0, "/home/josibake/bitcoin/test/functional")
>>> 
>>> from test_framework.test_shell import TestShell
>>> test = TestShell().setup(num_nodes=2, extra_args=[[],['-fallbackfee=0.0002']])
2022-12-01T11:23:23.765000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_sbwthbb_

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Approach ACK

ACK 989a52e0a50c0ae30a5c2bd3c08bb3ad1363a250

Tested that arguments passed using extra_args are detected and used as expected.

Having this functionality seems very useful for testshell (and I'm surprised it was not available until now).

This allows to remove dead code, no?

diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index e77676a365..3f6e82a772 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -408,10 +408,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
 
     def setup_nodes(self):
         """Override this method to customize test node setup"""
-        extra_args = [[]] * self.num_nodes
-        if hasattr(self, "extra_args"):
-            extra_args = self.extra_args
-        self.add_nodes(self.num_nodes, extra_args)
+        self.add_nodes(self.num_nodes, self.extra_args)
         self.start_nodes()
         if self._requires_wallet:
             self.import_deterministic_coinbase_privkeys()

This allows to remove dead code, no?

ah, good point! because add_node is already checking if extra_args is None and then creating an empty list of lists if it is:

https://github.com/bitcoin/bitcoin/blob/e334f7a54592ba9f05e4a5578dd933a7a31c3444/test/functional/test_framework/test_framework.py#L497-L498

lgtm

re-ACK 150340aeacb5e884cb875c77dc10c0d8b9984a33

Is this related to #17380?

Is this related to #17380?

I don't think so? This PR is strictly about passing extra_args via TestShell, which are arguments for configuring bitcoind nodes. #17380 seems to be about arguments passed to ./test_runner.py for the purpose of configuring the test framework and arguments passed to TestShell for also configuring the test framework having different naming and documentation in different places. I could be misunderstanding #17380 tho