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

38c3dd9c706e7e84b2a4dbaf1424a3f1c3b694fc docs: Add notes on how to diasble rpc timeout in functional tests while attatching gdb. (codeShark149)
784ae096259955ea7a294114f5c021c9489d2717 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 38c3dd9c706e7e84b2a4dbaf1424a3f1c3b694fc and thanks for fixing up all my typos :sweat_smile:
  jnewbery:
    ACK 38c3dd9c706e7e84b2a4dbaf1424a3f1c3b694fc.

Tree-SHA512: 9458dd1010288c62f8bb83f7a4893284fbbf938882dd65fc9e08810a910db07ef676e3100266028e5d4c8ce407b2267b3860595015da070c84a9d4a9816797db

5 files changed, 22 insertions, 16 deletions

diff --git a/test/README.md b/test/README.md
index e1dab92a06..0210907878 100644
--- a/test/README.md
+++ b/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
diff --git a/test/functional/test_framework/mininode.py b/test/functional/test_framework/mininode.py
index ba0391625e..bbd7350bf1 100755
--- a/test/functional/test_framework/mininode.py
+++ b/test/functional/test_framework/mininode.py
@@ -122,9 +122,9 @@ class P2PConnection(asyncio.Protocol):
     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 @@ class P2PInterface(P2PConnection):
     # 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
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index c84a7e7c12..6126efd842 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -102,7 +102,9 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         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 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
                             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 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
                 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 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
                     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,
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
index 826faece68..d52aff6f7e 100755
--- a/test/functional/test_framework/test_node.py
+++ b/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 @@ class TestNode():
         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 @@ class TestNode():
                     # 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 @@ class TestNode():
         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 @@ class TestNode():
         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 @@ class TestNode():
             # 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
diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index 7466a3cab3..6dfea7efd2 100644
--- a/test/functional/test_framework/util.py
+++ b/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