This is a refactor for self-validating and self-documenting code.
Currently, the test assumes that extra ports are available and just increments them without checking. However, this may not be the case when the test is modified to use more ports. In this case, the tests may fail intermittently and the failure is hard to debug.
Fix this confusion, by calling p2p_port each time. This ensures the required assert n <= MAX_NODES is checked each time.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33260.
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.
36@@ -37,37 +37,41 @@ def setup_network(self):
37 loopback_ipv4 = addr_to_hex("127.0.0.1")
38
39 # Start custom ports by reusing unused p2p ports
40- port = p2p_port(self.num_nodes)
41+ def extra_port():
iterator
That doesn’t work, because it is just what the current code does: Take a port and +=1 it. The goal of this pull request is to properly derive (and check) each port by its own.
0 def new_port():
NIT; wouldn’t new_port() be more suitable ? line 50 is just the use of a new port not an extra port. “extra to what?”
46
47 # Array of tuples [command line arguments, expected bind addresses].
48 self.expected = []
49
50 # Node0, no normal -bind=... with -bind=...=onion, thus only the tor target.
51+ port = extra_port()
crACK fabc2615af26c61a503f23ae4fd0353f90602bbe
PR adds extra function that returns a port. In this new function the utility function p2p_port(n) is called, which does a basic check and returns a (pseudo) random port number. This to reduce errors of using port numbers that are unavailable.
This looks like a simple solution without to much churn to solve #33250
ACK fabc2615af26c61a503f23ae4fd0353f90602bbe
Adding the patch below can confirm that the condition described in #33250 (comment) is reached.
0diff --git a/test/functional/feature_bind_extra.py b/test/functional/feature_bind_extra.py
1index 7dfa9c06ee..48b31092d2 100755
2--- a/test/functional/feature_bind_extra.py
3+++ b/test/functional/feature_bind_extra.py
4@@ -27,7 +27,7 @@ class BindExtraTest(BitcoinTestFramework):
5 # Avoid any -bind= on the command line. Force the framework to avoid
6 # adding -bind=127.0.0.1.
7 self.bind_to_localhost_only = False
8- self.num_nodes = 3
9+ self.num_nodes = 6
10
11 def skip_test_if_missing_module(self):
12 # Due to OS-specific network stats queries, we only run on Linux.
13@@ -73,6 +73,33 @@ class BindExtraTest(BitcoinTestFramework):
14 ],
15 )
16
17+ # Node1, both -bind=... and -bind=...=onion.
18+ port = [extra_port(), extra_port()]
19+ self.expected.append(
20+ [
21+ [f"-bind=127.0.0.1:{port[0]}", f"-bind=127.0.0.1:{port[1]}=onion"],
22+ [(loopback_ipv4, port[0]), (loopback_ipv4, port[1])],
23+ ],
24+ )
25+
26+ # Node1, both -bind=... and -bind=...=onion.
27+ port = [extra_port(), extra_port()]
28+ self.expected.append(
29+ [
30+ [f"-bind=127.0.0.1:{port[0]}", f"-bind=127.0.0.1:{port[1]}=onion"],
31+ [(loopback_ipv4, port[0]), (loopback_ipv4, port[1])],
32+
33+ # Node1, both -bind=... and -bind=...=onion.
34+ port = [extra_port(), extra_port()]
35+ self.expected.append(
36+ [
37+ [f"-bind=127.0.0.1:{port[0]}", f"-bind=127.0.0.1:{port[1]}=onion"],
38+ [(loopback_ipv4, port[0]), (loopback_ipv4, port[1])],
36@@ -37,37 +37,41 @@ def setup_network(self):
37 loopback_ipv4 = addr_to_hex("127.0.0.1")
38
39 # Start custom ports by reusing unused p2p ports
40- port = p2p_port(self.num_nodes)
41+ def extra_port():
42+ port = p2p_port(extra_port.index)
43+ extra_port.index += 1
44+ return port
45+ extra_port.index = self.num_nodes
If you apply the suggestion below, the patch above will work. Is there a reason why the starting port index should be the same as the number of nodes?
0 extra_port.index = 0