Repository: bitcoin/bitcoin
File: test/functional/feature_bind_extra.py (Python)

Code before:
        # Start custom ports by reusing unused p2p ports
        # We'll use different ports for each test to avoid conflicts
        port = p2p_port(self.num_nodes)

        # Array of tuples [command line arguments, expected bind addresses, test description].
        test_cases = []

        # Test case 1: no normal -bind=... with -bind=...=onion, thus only the tor target.
        test_cases.append(
            [
                [f"-bind=127.0.0.1:{port}=onion"],
                [(loopback_ipv4, port)],
                "no normal -bind with -bind=...=onion"
            ],
        )
        port += 1 # Increment port to avoid conflicts

        # Test case 2: both -bind=... and -bind=...=onion.
        test_cases.append(
            [
                [f"-bind=127.0.0.1:{port}", f"-bind=127.0.0.1:{port + 1}=onion"],
                [(loopback_ipv4, port), (loopback_ipv4, port + 1)],
                "both -bind and -bind=...=onion"
            ],
        )
        port += 2 # Increment port to avoid conflicts

        # Test case 3: no -bind=...=onion, thus no extra port for Tor target.
        test_cases.append(
            [
                [f"-bind=127.0.0.1:{port}"],
                [(loopback_ipv4, port)],
                "no -bind=...=onion"
            ],
        )
        port += 1 # Increment port to avoid conflicts

        # Run each test case by restarting the node with different configurations
        for i, (args, expected_services, description) in enumerate(test_cases):
            self.log.info(f"Test case {i + 1}: {description}")
            self.log.info(f"Restarting node 0 with args: {args}")

            # Restart the node with the new configuration
            self.restart_node(0, extra_args=args)

            # Check the listening ports
            pid = self.nodes[0].process.pid
            binds = set(get_bind_addrs(pid))

            # Remove IPv6 addresses because on some CI environments "::1" is not configured
            # on the system (so our test_ipv6_local() would return False), but it is

Review comment (refactor):
nit: in first case `increment` means +1, here it's `+1` - but more generally, the code is already, it's enough if we just document the intent:
```suggestion
        port += 2 # avoid conflicts since we've used two ports before
```

----

or even better, why are we even reusing the same variable, why not create a port iterator and instead of the list a lambda accepting two ports something like (untested, since this test is skipped on a Mac):
```python
def run_test(self):
    lp = addr_to_hex("127.0.0.1")
    test_cases = [
        lambda p, _: ([[f"-bind=127.0.0.1:{p}=onion"], # onion-only
                       [(lp, p)], "no normal -bind with -bind=...=onion"]),
        lambda p, q: ([[f"-bind=127.0.0.1:{p}", f"-bind=127.0.0.1:{q}=onion"], # bind + onion
                       [(lp, p), (lp, q)], "both -bind and -bind=...=onion"]),
        lambda p, _: ([[f"-bind=127.0.0.1:{p}"], # bind only
                       [(lp, p)], "no -bind=...=onion"]),
        lambda p, q: ([[f"-bind=127.0.0.1:{p}", f"-bind=127.0.0.1:{p}", f"-bind=127.0.0.1:{q}"], # duplicated bind
                       [(lp, p), (lp, p), (lp, q)], "duplicated -bind=... and -bind=..."]),
        lambda p, q: ([[f"-bind=127.0.0.1:{p}=onion", f"-bind=127.0.0.1:{p}=onion", f"-bind=127.0.0.1:{q}=onion"], # duplicated onion bind
                       [(lp, p), (lp, p), (lp, q)], "duplicated -bind=...=onion and -bind=...=onion"]),
    ]

    ports = itertools.count(p2p_port(self.num_nodes))
    for i, make in enumerate(test_cases):
        args, expected_services, description = make(next(ports), next(ports))
        self.log.info(f"Test case {i + 1}: {description}")
        self.log.info(f"Restarting node 0 with args: {args}")
```

---

It's also a bit inconsistent that at the end we just hardcode `-bind=127.0.0.1:11012`, maybe we could extract that port as well

Code after:

        # Array of tuples [command line arguments, expected bind addresses].
        self.expected = []

        # Node0, no normal -bind=... with -bind=...=onion, thus only the tor target.
        self.expected.append(
            [
                [f"-bind=127.0.0.1:{port}=onion"],
                [(loopback_ipv4, port)]
            ],
        )
        port += 1

        # Node1, both -bind=... and -bind=...=onion.
        self.expected.append(
            [
                [f"-bind=127.0.0.1:{port}", f"-bind=127.0.0.1:{port + 1}=onion"],
                [(loopback_ipv4, port), (loopback_ipv4, port + 1)]
            ],
        )
        port += 2

        # Node2, no -bind=...=onion, thus no extra port for Tor target.
        self.expected.append(
            [
                [f"-bind=127.0.0.1:{port}"],
                [(loopback_ipv4, port)]
            ],
        )
        port += 1

        self.extra_args = list(map(lambda e: e[0], self.expected))
        self.setup_nodes()

    def run_test(self):
        for i, (args, expected_services) in enumerate(self.expected):
            self.log.info(f"Checking listening ports of node {i} with {args}")
            pid = self.nodes[i].process.pid
            binds = set(get_bind_addrs(pid))
            # Remove IPv6 addresses because on some CI environments "::1" is not configured
            # on the system (so our test_ipv6_local() would return False), but it is
            # possible to bind on "::". This makes it unpredictable whether to expect
            # that bitcoind has bound on "::1" (for RPC) and "::" (for P2P).
            ipv6_addr_len_bytes = 32
            binds = set(filter(lambda e: len(e[0]) != ipv6_addr_len_bytes, binds))
            # Remove RPC ports. They are not relevant for this test.
            binds = set(filter(lambda e: e[1] != rpc_port(i), binds))
            assert_equal(binds, set(expected_services))

        self.stop_node(0)