Merge #18986: tests: Add capability to disable RPC timeout in functional tests

38c3dd9 docs: Add notes on how to diasble rpc timeout in functional tests while attatching gdb. (codeShark149)
784ae09 test: Add capability to disable RPC timeout in functional tests. (codeShark149)

Pull request description:

  Many times, especially while debugging RPC callbacks to core using gdb, the test timeout kicks in before the response can get back. This can be annoying and requires restarting the functional test as well as gdb attachment.

  This PR adds a `--notimeout` flag into `test_framework` and sets the `rpc_timeout` accordingly if the flag is set.

  The same effect can be achieved with newly added `--factor` flag but keeping a separate flag that explicitly disables the timeout can be easier for new testers to find it out and separates its purpose from the `--factor` flag.

  Requesting review ryanofsky jnewbery as per the IRC discussion.

  Update: After initial round of review, the approach is modified to accommodate the functionality in already existing `--factor` flag. `--factor` is changed to `--timeout-factor` to express its intent better.

ACKs for top commit:
  MarcoFalke:
    ACK 38c3dd9 and thanks for fixing up all my typos 😅
  jnewbery:
    ACK 38c3dd9.

Tree-SHA512: 9458dd1010288c62f8bb83f7a4893284fbbf938882dd65fc9e08810a910db07ef676e3100266028e5d4c8ce407b2267b3860595015da070c84a9d4a9816797db

Author: MarcoFalke

test/README.md

@@ -225,6 +225,10 @@ gdb /home/example/bitcoind <pid>
 Note: gdb attach step may require ptrace_scope to be modified, or `sudo` preceding the `gdb`.
 See this link for considerations: https://www.kernel.org/doc/Documentation/security/Yama.txt
 
+Often while debugging rpc calls from functional tests, the test might reach timeout before
+process can return a response. Use `--timeout-factor 0` to disable all rpc timeouts for that partcular
+functional test. Ex: `test/functional/wallet_hd.py --timeout-factor 0`.
+
 ##### Profiling
 
 An easy way to profile node performance during functional tests is provided

test/functional/test_framework/mininode.py

@@ -122,9 +122,9 @@ def __init__(self):
     def is_connected(self):
         return self._transport is not None
 
-    def peer_connect(self, dstaddr, dstport, *, net, factor):
+    def peer_connect(self, dstaddr, dstport, *, net, timeout_factor):
         assert not self.is_connected
-        self.factor = factor
+        self.timeout_factor = timeout_factor
         self.dstaddr = dstaddr
         self.dstport = dstport
         # The initial message to send after the connection was made:
@@ -372,7 +372,7 @@ def on_version(self, message):
     # Connection helper methods
 
     def wait_until(self, test_function, timeout):
-        wait_until(test_function, timeout=timeout, lock=mininode_lock, factor=self.factor)
+        wait_until(test_function, timeout=timeout, lock=mininode_lock, timeout_factor=self.timeout_factor)
 
     def wait_for_disconnect(self, timeout=60):
         test_function = lambda: not self.is_connected

test/functional/test_framework/test_framework.py

@@ -102,7 +102,9 @@ def __init__(self):
         self.bind_to_localhost_only = True
         self.set_test_params()
         self.parse_args()
-        self.rpc_timeout = int(self.rpc_timeout * self.options.factor) # optionally, increase timeout by a factor
+        if self.options.timeout_factor == 0 :
+            self.options.timeout_factor = 99999
+        self.rpc_timeout = int(self.rpc_timeout * self.options.timeout_factor) # optionally, increase timeout by a factor
 
     def main(self):
         """Main function. This should not be overridden by the subclass test scripts."""
@@ -169,7 +171,7 @@ def parse_args(self):
                             help="set a random seed for deterministically reproducing a previous test run")
         parser.add_argument("--descriptors", default=False, action="store_true",
                             help="Run test using a descriptor wallet")
-        parser.add_argument('--factor', type=float, default=1.0, help='adjust test timeouts by a factor')
+        parser.add_argument('--timeout-factor', dest="timeout_factor", type=float, default=1.0, help='adjust test timeouts by a factor. Setting it to 0 disables all timeouts')
         self.add_options(parser)
         self.options = parser.parse_args()
 
@@ -445,7 +447,7 @@ def get_bin_from_version(version, bin_name, bin_default):
                 chain=self.chain,
                 rpchost=rpchost,
                 timewait=self.rpc_timeout,
-                factor=self.options.factor,
+                timeout_factor=self.options.timeout_factor,
                 bitcoind=binary[i],
                 bitcoin_cli=binary_cli[i],
                 version=versions[i],
@@ -592,7 +594,7 @@ def _initialize_chain(self):
                     extra_args=['-disablewallet'],
                     rpchost=None,
                     timewait=self.rpc_timeout,
-                    factor=self.options.factor,
+                    timeout_factor=self.options.timeout_factor,
                     bitcoind=self.options.bitcoind,
                     bitcoin_cli=self.options.bitcoincli,
                     coverage_dir=None,

test/functional/test_framework/test_node.py

@@ -62,7 +62,7 @@ class TestNode():
     To make things easier for the test writer, any unrecognised messages will
     be dispatched to the RPC connection."""
 
-    def __init__(self, i, datadir, *, chain, rpchost, timewait, factor, bitcoind, bitcoin_cli, coverage_dir, cwd, extra_conf=None, extra_args=None, use_cli=False, start_perf=False, use_valgrind=False, version=None, descriptors=False):
+    def __init__(self, i, datadir, *, chain, rpchost, timewait, timeout_factor, bitcoind, bitcoin_cli, coverage_dir, cwd, extra_conf=None, extra_args=None, use_cli=False, start_perf=False, use_valgrind=False, version=None, descriptors=False):
         """
         Kwargs:
             start_perf (bool): If True, begin profiling the node with `perf` as soon as
@@ -128,7 +128,7 @@ def __init__(self, i, datadir, *, chain, rpchost, timewait, factor, bitcoind, bi
         self.perf_subprocesses = {}
 
         self.p2ps = []
-        self.factor = factor
+        self.timeout_factor = timeout_factor
 
     AddressKeyPair = collections.namedtuple('AddressKeyPair', ['address', 'key'])
     PRIV_KEYS = [
@@ -241,7 +241,7 @@ def wait_for_rpc_connection(self):
                     # The wait is done here to make tests as robust as possible
                     # and prevent racy tests and intermittent failures as much
                     # as possible. Some tests might not need this, but the
-                    # overhead is trivial, and the added gurantees are worth
+                    # overhead is trivial, and the added guarantees are worth
                     # the minimal performance cost.
                 self.log.debug("RPC successfully started")
                 if self.use_cli:
@@ -349,13 +349,13 @@ def is_node_stopped(self):
         return True
 
     def wait_until_stopped(self, timeout=BITCOIND_PROC_WAIT_TIMEOUT):
-        wait_until(self.is_node_stopped, timeout=timeout, factor=self.factor)
+        wait_until(self.is_node_stopped, timeout=timeout, timeout_factor=self.timeout_factor)
 
     @contextlib.contextmanager
     def assert_debug_log(self, expected_msgs, unexpected_msgs=None, timeout=2):
         if unexpected_msgs is None:
             unexpected_msgs = []
-        time_end = time.time() + timeout * self.factor
+        time_end = time.time() + timeout * self.timeout_factor
         debug_log = os.path.join(self.datadir, self.chain, 'debug.log')
         with open(debug_log, encoding='utf-8') as dl:
             dl.seek(0, 2)
@@ -512,7 +512,7 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs):
         if 'dstaddr' not in kwargs:
             kwargs['dstaddr'] = '127.0.0.1'
 
-        p2p_conn.peer_connect(**kwargs, net=self.chain, factor=self.factor)()
+        p2p_conn.peer_connect(**kwargs, net=self.chain, timeout_factor=self.timeout_factor)()
         self.p2ps.append(p2p_conn)
         if wait_for_verack:
             # Wait for the node to send us the version and verack
@@ -526,7 +526,7 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs):
             # transaction that will be added to the mempool as soon as we return here.
             #
             # So syncing here is redundant when we only want to send a message, but the cost is low (a few milliseconds)
-            # in comparision to the upside of making tests less fragile and unexpected intermittent errors less likely.
+            # in comparison to the upside of making tests less fragile and unexpected intermittent errors less likely.
             p2p_conn.sync_with_ping()
 
         return p2p_conn

test/functional/test_framework/util.py

@@ -208,10 +208,10 @@ def str_to_b64str(string):
 def satoshi_round(amount):
     return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN)
 
-def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, factor=1.0):
+def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, timeout_factor=1.0):
     if attempts == float('inf') and timeout == float('inf'):
         timeout = 60
-    timeout = timeout * factor
+    timeout = timeout * timeout_factor
     attempt = 0
     time_end = time.time() + timeout