This PR adds a new option factor that can be used to adjust timeouts in various functional tests.
Several timeouts and functions from authproxy, mininode, test_node and util have been adapted to use this option. The factor-option definition is located in test_framework.py.
Fixes #18266 Also Fixes https://github.com/bitcoin/bitcoin/issues/18834
584 | @@ -585,7 +585,7 @@ def send_blocks_and_test(self, blocks, node, *, success=True, force_send=False, 585 | self.send_message(msg_block(block=b)) 586 | else: 587 | self.send_message(msg_headers([CBlockHeader(block) for block in blocks])) 588 | - wait_until(lambda: blocks[-1].sha256 in self.getdata_requests, timeout=timeout, lock=mininode_lock) 589 | + wait_until(lambda: blocks[-1].sha256 in self.getdata_requests, timeout=timeout * self.options.factor, lock=mininode_lock)
Given that they are all based on wait_until wouldn't it make more sense to treat wait_until as the basic mechanism for timed waits and only multiply it in there?
Meanwhile, I have changed a few other things, because I faced various p2p errors (the P2PInterface didn't have their factor variable, so I had to redo a few things). And yes, wait_until will be changed too.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
This one fails on travis, but succeeds on my macOS. Is there anything I could do to make it pass?
mempool_reorg.py | ✖ Failed | 773 s
Error Log
raise AssertionError("Mempool sync timed out:{}".format("".join("\n {!r}".format(m) for m in pool)))
AssertionError: Mempool sync timed out:
365 | @@ -366,7 +366,7 @@ def on_version(self, message): 366 | 367 | def wait_for_disconnect(self, timeout=60): 368 | test_function = lambda: not self.is_connected 369 | - wait_until(test_function, timeout=timeout, lock=mininode_lock) 370 | + wait_until(test_function, timeout=timeout, lock=mininode_lock, factor=self.factor)
self.wait_until(test_function, timeout=timeout)
Could this be a member function, so that the lock and factor members don't have to be passed each time?
212 | +def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, factor=1.0): 213 | if attempts == float('inf') and timeout == float('inf'): 214 | - timeout = 60 215 | + timeout = 60 * factor 216 | + else: 217 | + timeout = timeout * factor
timeout = timeout * factor
can remove the else branch?
331 | @contextlib.contextmanager 332 | def assert_debug_log(self, expected_msgs, unexpected_msgs=None, timeout=2): 333 | if unexpected_msgs is None: 334 | unexpected_msgs = [] 335 | - time_end = time.time() + timeout 336 | + time_end = time.time() + int(timeout * self.factor)
time_end = time.time() + timeout * self.factor
why is this needed?
I had problems in test_node.py's wait_for_rpc_connection. More precisely, the polling for-loop that refused to iterate over float values. Hence the conversion to int. But if there is a way to make it work without conversion, I'll gladly try it out. Right now, I am going through your list of nits and adapting the code.
time.time() is a float, so I don't see how this can cause issues
75 | @@ -76,7 +76,7 @@ def __init__(self, i, datadir, *, chain, rpchost, timewait, bitcoind, bitcoin_cl 76 | self.stderr_dir = os.path.join(self.datadir, "stderr") 77 | self.chain = chain 78 | self.rpchost = rpchost 79 | - self.rpc_timeout = timewait 80 | + self.rpc_timeout = int(timewait)
why is this needed, when authproxy does the casting already?
In some previous variant of my code I encountered such a problem. Maybe just because I was still writing it. Right now, it seems that all those conversions aren't needed anymore. I'm just executing some tests and they're all passing.
488 | @@ -488,6 +489,7 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs):
489 | kwargs['dstaddr'] = '127.0.0.1'
490 |
491 | p2p_conn.peer_connect(**kwargs, net=self.chain)()
p2p_conn.peer_connect(**kwargs, net=self.chain, factor=self.factor)()
I don't understand this change?
Ah, yes, now I see it. It's being forwarded to derived class P2PInterface!
ACK, just style nits
Could you please bump the factor to 2.0 for the valgrind run and mark this pull as fixing #18834 ?
207 | @@ -208,9 +208,10 @@ def str_to_b64str(string): 208 | def satoshi_round(amount): 209 | return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN) 210 | 211 | -def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None): 212 | +def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, factor=2.0):
Why increase the factor for everything? valgrind should be sufficient:
diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh
index d5b38a1d88..c850c13bbe 100644
--- a/ci/test/00_setup_env_native_valgrind.sh
+++ b/ci/test/00_setup_env_native_valgrind.sh
@@ -13,7 +13,7 @@ export NO_DEPENDS=1
if [[ "${TRAVIS}" == "true" && "${TRAVIS_REPO_SLUG}" != "bitcoin/bitcoin" ]]; then
export TEST_RUNNER_EXTRA="wallet_disable" # Only run wallet_disable as a smoke test to not hit the 50 min travis time limit
else
- export TEST_RUNNER_EXTRA="--exclude rpc_bind" # Excluded for now, see [#17765 (comment)](/bitcoin-bitcoin/17765/#issuecomment-602068547)
+ export TEST_RUNNER_EXTRA="--exclude rpc_bind --factor=2" # rpc_bind excluded for now, see [#17765 (comment)](/bitcoin-bitcoin/17765/#issuecomment-602068547)
fi
export GOAL="install"
export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=no CC=clang CXX=clang++" # TODO enable GUI
True. Have adapted the scripts.
207 | @@ -208,9 +208,10 @@ def str_to_b64str(string): 208 | def satoshi_round(amount): 209 | return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN) 210 | 211 | -def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None): 212 | +def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, factor=2.0): 213 | if attempts == float('inf') and timeout == float('inf'): 214 | - timeout = 60 215 | + timeout = 60 * factor
timeout = 60
324 | @@ -324,13 +325,13 @@ def is_node_stopped(self): 325 | return True 326 | 327 | def wait_until_stopped(self, timeout=BITCOIND_PROC_WAIT_TIMEOUT): 328 | - wait_until(self.is_node_stopped, timeout=timeout) 329 | + wait_until(self.is_node_stopped, timeout=timeout, lock=None, factor=self.factor)
wait_until(self.is_node_stopped, timeout=timeout, factor=self.factor)
None is the default value already
ACK
100 | @@ -101,6 +101,7 @@ def __init__(self): 101 | self.bind_to_localhost_only = True 102 | self.set_test_params() 103 | self.parse_args() 104 | + self.rpc_timeout = int(self.rpc_timeout * self.options.factor) # optionally, increase timeout by a factor
self.rpc_timeout = self.rpc_timeout * self.options.factor # optionally, increase timeout by a factor
Is this still needed?
I think it is. When I execute a test like this it throws the one error we talked about previously (the range poll function).
./test/functional/p2p_tx_download.py (4s 89ms)
2020-05-02T23:36:00.054000Z TestFramework (INFO): Initializing test directory /var/folders/7q/4ffytzk562dd2ky4bfg9_w7h0000gn/T/bitcoin_func_test_54bxj
085
2020-05-02T23:36:00.247000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/Users/brakmic/projects/bitcoin-dev/btc-factor/test/functional/test_framework/test_framework.py", line 452, in start_nodes
node.wait_for_rpc_connection()
File "/Users/brakmic/projects/bitcoin-dev/btc-factor/test/functional/test_framework/test_node.py", line 217, in wait_for_rpc_connection
for _ in range(poll_per_s * self.rpc_timeout):
TypeError: 'float' object cannot be interpreted as an integer
207 | @@ -208,9 +208,10 @@ def str_to_b64str(string): 208 | def satoshi_round(amount): 209 | return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN) 210 | 211 | -def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None): 212 | +def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, factor=1.0): 213 | if attempts == float('inf') and timeout == float('inf'): 214 | - timeout = 60 215 | + timeout = 60
why change the whitespace to 3 spaces? Seems unrelated
Unintentional. Corrected. :)
ACK
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Thanks! ACK 2742c3428633b6ceaab6714635dc3adb74bf121b