test: use IP_PORTRANGE_HIGH on FreeBSD for dynamic port allocation #34346

    framework's static port range starting at TEST_RUNNER_PORT_MIN (default=11000). Using IP_PORTRANGE_HIGH avoids

nit: Could avoid hard-coding the upper range, which may change in the future.

Done. Thanks.

Maybe better to use more generic name for the function. It contains if sys.platform.startswith('freebsd') and would accommodate other OSes well if at some point it would be necessary to do tweaks for others. E.g. if freebsd tweak1, else if openbsd tweak2, if linuxcool tweak3, if windoz ugh.

What about set_ephemeral_port_range()?

Done. Thanks.

I checked that this works as expected - without it some ports are below 49152 and with it the selected ports are above (for a couple of dozen tries).

This covers only the port for the Socks5 server itself, but not ports used by the P2P listeners, created with network_event_loop.create_server(). Here is an extension to this patch that would cover the later as well:

<details> <summary>[patch] set port range also for P2P listeners</summary>

diff --git i/test/functional/test_framework/netutil.py w/test/functional/test_framework/netutil.py
index 792afabff6..8fda0baa31 100644
--- i/test/functional/test_framework/netutil.py
+++ w/test/functional/test_framework/netutil.py
@@ -180,13 +180,13 @@ def format_addr_port(addr, port):
     if ":" in addr:
         return f"[{addr}]:{port}"
     else:
         return f"{addr}:{port}"
 
 
-def set_freebsd_high_port_range(sock):
+def set_ephemeral_port_range(sock):
     '''On FreeBSD, set socket to use the high ephemeral port range (49152-65535).
 
     FreeBSD's default ephemeral port range (10000-65535) overlaps with the test
     framework's static port range (11000-26000). Using IP_PORTRANGE_HIGH avoids
     this overlap when binding to port 0 for dynamic port allocation.
     '''
diff --git i/test/functional/test_framework/p2p.py w/test/functional/test_framework/p2p.py
index 986eaf1e88..b6903a6ba2 100755
--- i/test/functional/test_framework/p2p.py
+++ w/test/functional/test_framework/p2p.py
@@ -19,16 +19,18 @@ P2PDataStore: A p2p interface class that keeps a store of transactions and block
               and can respond correctly to getdata and getheaders messages
 P2PTxInvStore: A p2p interface class that inherits from P2PDataStore, and keeps
               a count of how many times each txid has been announced."""
 
 import asyncio
 from collections import defaultdict
+import ipaddress
 from io import BytesIO
 import logging
 import platform
 import struct
+import socket
 import sys
 import threading
 
 from test_framework.messages import (
     CBlockHeader,
     MAX_HEADERS_RESULTS,
@@ -73,12 +75,15 @@ from test_framework.messages import (
     msg_wtxidrelay,
     NODE_NETWORK,
     NODE_WITNESS,
     MAGIC_BYTES,
     sha256,
 )
+from test_framework.netutil import (
+    set_ephemeral_port_range,
+)
 from test_framework.util import (
     assert_not_equal,
     MAX_NODES,
     p2p_port,
     wait_until_helper_internal,
 )
@@ -744,12 +749,14 @@ class NetworkThread(threading.Thread):
         self.network_event_loop.run_forever()
 
     def close(self, *, timeout=10):
         """Close the connections and network event loop."""
         self.network_event_loop.call_soon_threadsafe(self.network_event_loop.stop)
         wait_until_helper_internal(lambda: not self.network_event_loop.is_running(), timeout=timeout)
+        for listener in NetworkThread.listeners.values():
+            listener.close()
         self.network_event_loop.close()
         self.join(timeout)
         # Safe to remove event loop.
         NetworkThread.network_event_loop = None [@classmethod](/bitcoin-bitcoin/contributor/classmethod/)
@@ -790,14 +797,28 @@ class NetworkThread(threading.Thread):
         if port == 0 or (addr, port) not in cls.listeners:
             # When creating a listener on a given (addr, port) we only need to
             # do it once. If we want different behaviors for different
             # connections, we can accomplish this by providing different
             # `proto` functions
 
-            listener = await cls.network_event_loop.create_server(peer_protocol, addr, port)
-            port = listener.sockets[0].getsockname()[1]
+            if port == 0:
+                # Manually create the socket in order to set the range to be
+                # used for the port before the bind() call.
+                if ipaddress.ip_address(addr).version == 4:
+                    address_family = socket.AF_INET
+                else:
+                    address_family = socket.AF_INET6
+                s = socket.socket(address_family)
+                set_ephemeral_port_range(s)
+                s.bind((addr, 0))
+                s.listen()
+                listener = await cls.network_event_loop.create_server(peer_protocol, sock=s)
+                port = listener.sockets[0].getsockname()[1]
+            else:
+                listener = await cls.network_event_loop.create_server(peer_protocol, addr, port)
+
             logger.debug("Listening server on %s:%d should be started" % (addr, port))
             cls.listeners[(addr, port)] = listener
 
         cls.protos[(addr, port)] = proto
         callback(addr, port)
 
diff --git i/test/functional/test_framework/socks5.py w/test/functional/test_framework/socks5.py
index 4c77a6ff25..085c5a2e32 100644
--- i/test/functional/test_framework/socks5.py
+++ w/test/functional/test_framework/socks5.py
@@ -9,13 +9,13 @@ import socket
 import threading
 import queue
 import logging
 
 from .netutil import (
     format_addr_port,
-    set_freebsd_high_port_range,
+    set_ephemeral_port_range,
 )
 
 logger = logging.getLogger("TestFramework.socks5")
 
 # Protocol constants
 class Command:
@@ -203,13 +203,13 @@ class Socks5Server():
         self.conf = conf
         self.s = socket.socket(conf.af)
         self.s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
         # When using dynamic port allocation (port=0), ensure we don't get a
         # port that conflicts with the test framework's static port range.
         if conf.addr[1] == 0:
-            set_freebsd_high_port_range(self.s)
+            set_ephemeral_port_range(self.s)
         self.s.bind(conf.addr)
         # When port=0, the OS assigns an available port. Update conf.addr
         # to reflect the actual bound address so callers can use it.
         self.conf.addr = self.s.getsockname()
         self.s.listen(5)
         self.running = False

</details>

Done. Added the patch. Thanks.

With this patch, p2p_add_connections.py fails on macOS, ARM, i686, and Windows with: TypeError: 'NoneType' object is not iterable. Looking into this.

The issue in this patch appears to stem from the listener.close() calls in NetworkThread.close(). Removing these calls allows the test to pass.

It seems that these calls can trigger asyncio cleanup errors on macOS (and likely other platforms), because they may be executed after the event loop has already stopped. This results in a TypeError: 'NoneType' object is not iterable originating from asyncio’s _SelectorTransport.__del__.

Given that the event loop’s close() method apparently already performs resource cleanup—and that all tests pass without the explicit listener.close() calls, are these calls still necessary?

Re-added that patch in a separate commit https://github.com/bitcoin/bitcoin/pull/34346/commits/23f613acf94b1cefdc7d2e339cdd3c1bb0d33d0e . Removed the listener.close().

I was not sure if the added close() calls are necessary. The python docs say:

https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.create_server

sock can optionally be specified in order to use a preexisting socket object. If specified, host and port must not be specified.

Note

The sock argument transfers ownership of the socket to the server created. To close the socket, call the server’s close() method.

We only put the listeners in the NetworkThread.listeners list, but never read from that, so it is probably recognized and used by the Python system code itself? NetworkThread.close() would call close() on each member?

Technically, per the Python docs, Server.close() (i.e., listener.close() in the code) should be called explicitly.

However, we would need to await wait_closed() after close(), before closing the event loop, which adds complexity and can introduce cross-platform timing issues (I tested a run_coroutine_threadsafe approach with wait_closed() and it deadlocked on macOS).

Given that:

• The test framework processes are short-lived
• The OS reclaims all sockets when the process exits
• Explicit cleanup introduces complexity without practical benefit here

I think it's fine to skip the explicit listener.close() calls.

I think it is fine too.

Just curious, where did you find that: "per the Python docs, Server.close() (i.e., listener.close() in the code) should be called explicitly"?

This was my (possibly an over-interpretation) of: "To close the socket, call the server’s close() method." https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.create_server

This error isn’t related to your patch itself. If the same change is applied to master, it fails too.

@@ -744,12 +749,14 @@ class NetworkThread(threading.Thread):
         self.network_event_loop.run_forever()
 
     def close(self, *, timeout=10):
         """Close the connections and network event loop."""
         self.network_event_loop.call_soon_threadsafe(self.network_event_loop.stop)
         wait_until_helper_internal(lambda: not self.network_event_loop.is_running(), timeout=timeout)
+        for listener in NetworkThread.listeners.values():
+            listener.close()
         self.network_event_loop.close()
         self.join(timeout)
         # Safe to remove event loop.
         NetworkThread.network_event_loop = None

Even if the listeners are closed before stopping the event loop, the error persists:

     def close(self, *, timeout=10):
         """Close the connections and network event loop."""
+        for listener in NetworkThread.listeners.values():
+            listener.close()
         self.network_event_loop.call_soon_threadsafe(self.network_event_loop.stop)
         wait_until_helper_internal(lambda: not self.network_event_loop.is_running(), timeout=timeout)

The error only happens on macOS, ARM, i686, and Windows. On Linux, it proceeds as expected.

Should an issue be opened for it ?

Should an issue be opened for it ?

I think no.

"To close the socket, call the server’s close() method."

Ok, that probably is intended to hint the user that now the socket is owned by the server and the proper way to close it is to use the server's close() method instead of closing the socket directly.

I assume self.network_event_loop.close() will call those close() methods on the servers, but couldn't find an explicit docs about it.

I think no further changes are needed here.

In the commit message of 23f613acf94b1cefdc7d2e339cdd3c1bb0d33d0e test: use IP_PORTRANGE_HIGH on FreeBSD for dynamic port allocation:

s/commit added/commit added/

Done. Thanks.