Run feature_bind_port_(discover|externalip).py in CI #33362

What is the effect on a developer running CI locally on their machine, of adding NET_ADMIN capability? Is is possible for the CI to then modify/break the developer machine networking?

   CAP_NET_ADMIN
          Perform various network-related operations:
          •  interface configuration;
          •  administration of IP firewall, masquerading, and
             accounting;
          •  modify routing tables;
          •  bind to any address for transparent proxying;
          •  set type-of-service (TOS);
          •  clear driver statistics;
          •  set promiscuous mode;
          •  enabling multicasting;
          •  use setsockopt(2) to set the following socket options:
             SO_DEBUG, SO_MARK, SO_PRIORITY (for a priority outside
             the range 0 to 6), SO_RCVBUFFORCE, and SO_SNDBUFFORCE.
             

We use our own --network in CI:

https://github.com/bitcoin/bitcoin/blob/ee42d59d4de970769ebabf77b89ff4269498f61e/ci/test/02_run_container.sh#L71

But I am unsure if e.g. ip addr add "1.1.1.1/32" adds it to this network only, or the host network?

If this is affecting (or has the potential to affect) host network, perhaps it can be gated by if $CI or similar?

The privileges only apply to the guest, not to host. The guest ("container" in docker language) has no way to manipulate the host's networking.

https://docs.docker.com/engine/containers/run/#container-networking https://docs.docker.com/engine/network/

We use our own --network in CI ...

Hmm. Maybe that could be an easier way to add the dummy routable addresses. Then they will be available for the whole duration of the entire CI run, not only during the functional tests as in this PR. I guess this should be ok, will look into this...

We use our own --network in CI ...

Maybe that could be an easier way to add the dummy routable addresses

Yes, done like that. Seems to be smaller change and looks more robust.

The container has been attached to both the default bridge network and ci-ip4net, hence two IPv4 addresses. Do you think getifaddrs() may return the bridge IP first, and cause Discover() to find the wrong address?

If thats the case then I think it might be worth it to disconnect from bridge after connecting to ci-ip4net.

Discover() finds all addresses, not just the first one?

Would it make sense to use RFC-5737 here? (I think they are reserved for documentation and testing)

That would mess with IsRoutable():

https://github.com/bitcoin/bitcoin/blob/3532e242134e8f92fb5fe99bbd024ed185839e8c/src/netaddress.cpp#L462-L465

https://github.com/bitcoin/bitcoin/blob/3532e242134e8f92fb5fe99bbd024ed185839e8c/src/netaddress.cpp#L334-L339

This error/cleanup code is duplicated in both files (with the exception of printing 1 or 2 addresses), would it be worth extracting this into a helper in test_node.py or somewhere?

Maybe, do you want me to take a short at that?

Using rg -C 2 "self.start_nodes()" . it does not seem like any other tests do this (surround with try/except) so I am wondering is this cleanup even necessary?

In case it is, is it only for this test or for all tests? In case you can't argue why it is necessary this should be removed in both files, in case it is only for these tests adding a helper is not worth it imo. in case its relevant for all tests this should be a separate PR (but you can leave the duplicated logic in this PR)

sure, something like this, I think:

diff --git a/test/functional/feature_bind_port_discover.py b/test/functional/feature_bind_port_discover.py
index 86eca321e5e..778fce2fd23 100755
--- a/test/functional/feature_bind_port_discover.py
+++ b/test/functional/feature_bind_port_discover.py
@@ -72,24 +72,13 @@ class BindPortDiscoverTest(BitcoinTestFramework):
         try:
             self.start_nodes()
         except FailedToStartError as e:
-            for node in self.nodes:
-                if node.running:
-                    if node.rpc_connected:
-                        node.stop_node(wait=node.rpc_timeout)
-                    else:
-                        node.process.kill()
-                        node.process.wait(timeout=node.rpc_timeout)
-                        node.process = None
-                        node.stdout.close()
-                        node.stderr.close()
-                        node.running = False
+            self.cleanup_partially_started_nodes()
             if 'Unable to bind to ' in str(e):
                 raise SkipTest(
                     f'To run this test make sure that {ADDR1} and {ADDR2} '
                     '(routable addresses) are assigned to non-loopback '
                     'interfaces on this machine')
-            else:
-                raise e
+            raise

     def run_test(self):
         self.log.info(
diff --git a/test/functional/feature_bind_port_externalip.py b/test/functional/feature_bind_port_externalip.py
index a8e3abe851d..28250611e13 100755
--- a/test/functional/feature_bind_port_externalip.py
+++ b/test/functional/feature_bind_port_externalip.py
@@ -64,24 +64,13 @@ class BindPortExternalIPTest(BitcoinTestFramework):
         try:
             self.start_nodes()
         except FailedToStartError as e:
-            for node in self.nodes:
-                if node.running:
-                    if node.rpc_connected:
-                        node.stop_node(wait=node.rpc_timeout)
-                    else:
-                        node.process.kill()
-                        node.process.wait(timeout=node.rpc_timeout)
-                        node.process = None
-                        node.stdout.close()
-                        node.stderr.close()
-                        node.running = False
+            self.cleanup_partially_started_nodes()
             if 'Unable to bind to ' in str(e):
                 raise SkipTest(
                     f'To run this test make sure that {ADDR} '
                     '(routable address) is assigned to non-loopback '
                     'interface on this machine')
-            else:
-                raise e
+            raise

     def run_test(self):
         self.log.info("Test the proper port is used for -externalip=")
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index b809e68f4df..8cc7121f102 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -535,6 +535,27 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
             # Wait for nodes to stop
             node.wait_until_stopped()

+    def cleanup_partially_started_nodes(self):
+        """Tear down nodes left running after a failed start_nodes().
+
+        After start_nodes() raises (e.g. FailedToStartError), some nodes may be
+        RPC-connected, some may have a live process without RPC, and some may
+        already have exited. Stop the connected ones cleanly and force-kill the
+        rest so the framework's teardown can proceed.
+        """
+        for node in self.nodes:
+            if not node.running:
+                continue
+            if node.rpc_connected:
+                node.stop_node(wait=node.rpc_timeout)
+            else:
+                node.process.kill()
+                node.process.wait(timeout=node.rpc_timeout)
+                node.process = None
+                node.stdout.close()
+                node.stderr.close()
+                node.running = False
+
     def restart_node(self, i, extra_args=None, clear_addrman=False, *, expected_stderr=''):
         """Stop and start a test node"""
         self.stop_node(i, expected_stderr=expected_stderr)

I replaced raise e with raise, as in python3 this gets you a fuller traceback, but can drop that part if you don't think it's necessary

Took the patch, thanks! (added you as co-author).

In case this PR is intended to build on top of #32757 you can add a commit that removes this as this is a workaround that is required before that PR.

See: #31293 (comment)

So, if that is fixed, then the above fix to the test will not be necessary.

Removing it also indirectly tests that the discover fix works and makes this test better.

feature_bind_port_externalip.py fails if I remove node.has_explicit_bind = True:

2026-04-10T11:11:30.108371Z TestFramework (ERROR): Unexpected exception:
Traceback (most recent call last):
  File ".../test/functional/test_framework/test_framework.py", line 143, in main
    self.run_test()
    ~~~~~~~~~~~~~^^
  File ".../test/functional/feature_bind_port_externalip.py", line 95, in run_test
    assert_equal(local['port'], expected_port)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../test/functional/test_framework/util.py", line 83, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(11826 == 30001)

This is because for node0 without node.has_explicit_bind = True the framework adds Command-line arg: bind="0.0.0.0:11826" which overrides the test's own Command-line arg: port="30001".

Did you mean feature_bind_port_discover.py? That test passes if node.has_explicit_bind = True is removed, but that would cripple the test, making it to test less cases. The test has this for node0 and node1:

['-discover', f'-port={self.bind_ports[0]}', '-listen=1'], # Without any -bind
['-discover', f'-bind=0.0.0.0:{self.bind_ports[1]}'], # Explicit -bind=0.0.0.0

If node.has_explicit_bind = True is removed then the first one, node0, also gets Command-line arg: bind="0.0.0.0:11894" which is then the same as for node1. In other words, then node0 and node1 will test the same case - with explicit -bind=0.0.0.0 and "without any -bind" will not be tested.

Yes sorry there were two errors in my reasoning: 1) yes I meant feature_bind_port_discover.py not the file I commented on. 2) I thought that since Discover now properly runs on explicit wildcard binds it is not required to add node.has_explicit_bind = True anymore.

As far as I understand it now it is still required as a hatch so the testing framework does not inject/override the bind...which seems like an issue with the framework but you are right that node.has_explicit_bind = True can not be removed.

edit: actually I might meant both files I can honestly not remember anymore but it does not matter.

Worth adding a comment here (or perhaps better in in run_container.py) that these must match: https://github.com/bitcoin/bitcoin/blob/285a5de4b9457f3b385fcc04753840cacb1eb61f/ci/test/02_run_container.py#L117 to try and avoid them getting out of sync?

Done

In commit "test: make feature_bind_port_(discover|externalip).py auto-detect the skip condition" (1b93983bf5c8460b2e5741f5efe88c0adbed4a79)

Would be nice to write $INTERFACE_NAME here and below it stands out more and the command is directly pastable after setting the variable.

In commit "test: make feature_bind_port_(discover|externalip).py auto-detect the skip condition" (1b93983bf5c8460b2e5741f5efe88c0adbed4a79)

Note this is using rpc_timeout for a wait which isn't not an RPC. This is reasonable since rpc_timeout is used in similar ways elsewhere, but might be something to clean up later. (I've suggested renaming rpc_timeout to failure_timeout let it be used as a general timeout for how long a test is allowed to be idle before it fails.)

In commit "test: make feature_bind_port_(discover|externalip).py auto-detect the skip condition" (1b93983bf5c8460b2e5741f5efe88c0adbed4a79)

I think this comment should say why this test needs to avoid -bind arguments. I understand vaguely the it helps with coverage but it would be good to know a more specific reason. I think comment should also mention the node.has_explicit_bind = True setting. I don't understand why has_explicit_bind is only needed now but not needed previously. It seems like it should have always been added?

(Separately, these option names both seem misleading. bind_to_localhost_only seems like it should be called bind_to_localhost since it does bind to localhost but doesn't actually prevent other binds. The has_explicit_bind name is also wrong if goal of setting the option is to test the node behavior when there are no -bind arguments. Seems better to call something like bind_to_unique_ports or bind_auto and invert it, setting it true by default.

This fails on Windows: https://github.com/bitcoin/bitcoin/actions/runs/26174639836/job/77001441263?pr=32150#step:14:2224:

feature_bind_port_externalip.py", line 65, in setup_nodes
                                       self.start_nodes()
                                       ~~~~~~~~~~~~~~~~^^
                                     File "D:\a\bitcoin\bitcoin\build\test\functional\test_framework\test_framework.py", line 518, in start_nodes
                                       node.wait_for_rpc_connection()
                                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
                                     File "D:\a\bitcoin\bitcoin\build\test\functional\test_framework\test_node.py", line 332, in wait_for_rpc_connection
                                       rpc.getblockcount()
                                       ~~~~~~~~~~~~~~~~~^^
                                     File "D:\a\bitcoin\bitcoin\build\test\functional\test_framework\coverage.py", line 50, in __call__
                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                                     File "D:\a\bitcoin\bitcoin\build\test\functional\test_framework\authproxy.py", line 133, in __call__
                                       response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
                                                          ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "D:\a\bitcoin\bitcoin\build\test\functional\test_framework\authproxy.py", line 106, in _request
                                       self.__conn.request(method, path, postdata, headers)
                                       ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "C:\hostedtoolcache\windows\Python\3.14.5\x64\Lib\http\client.py", line 1367, in request
                                       self._send_request(method, url, body, headers, encode_chunked)
                                       ~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "C:\hostedtoolcache\windows\Python\3.14.5\x64\Lib\http\client.py", line 1413, in _send_request
                                       self.endheaders(body, encode_chunked=encode_chunked)
                                       ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "C:\hostedtoolcache\windows\Python\3.14.5\x64\Lib\http\client.py", line 1362, in endheaders
                                       self._send_output(message_body, encode_chunked=encode_chunked)
                                       ~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "C:\hostedtoolcache\windows\Python\3.14.5\x64\Lib\http\client.py", line 1161, in _send_output
                                       self.send(chunk)
                                       ~~~~~~~~~^^^^^^^
                                     File "C:\hostedtoolcache\windows\Python\3.14.5\x64\Lib\http\client.py", line 1086, in send
                                       self.sock.sendall(data)
                                       ~~~~~~~~~~~~~~~~~^^^^^^
                                   ConnectionAbortedError: [WinError 10053] An established connection was aborted by the software in your host machine

 node1 stderr Error: Unable to bind to 1.1.1.5:30002 on this computer (bind returned error The requested address is not valid in its context.  (10049))
Error: Failed to listen on any port. Use -listen=0 if you want this. 
 node4 stderr Error: Unable to bind to 1.1.1.5:30005 on this computer (bind returned error The requested address is not valid in its context.  (10049))
Error: Failed to listen on any port. Use -listen=0 if you want this.