Merge bitcoin/bitcoin#35867: test: classify SOCKS5 peers via getpeerinfo addrbind

4e8c4bc794 test: classify SOCKS5 peers via getpeerinfo addrbind (Henry Romp)

Pull request description:

  p2p_private_broadcast.py classifies each SOCKS5 connection by scanning the node's debug log for `trying v. connection (...) to <addr>:<port>`, then attaches a fake peer for that type. The helper returned the first match in the whole log, so when a feeler selected a clearnet address that private broadcast had used earlier in the run (in the CI failure, `[50::1]:8333`, about 10 seconds apart), the feeler was labelled private-broadcast, was given the `NoRelayP2PInterface`, and disconnected as a feeler rather than with the expected "connected in vain" message.

  Instead of relying on the debug log, identify the connection via the SOCKS5 proxy client socket's source address, which equals the node's `addrbind` for that peer, and read `connection_type` from getpeerinfo. The proxy replies to the SOCKS5 request before invoking `destinations_factory`, so the node has already registered the peer by the time classification runs. This also stops treating debug.log contents as a stable test interface. Dropping the log scrape removes a full re-read of debug.log per SOCKS5 connection; `p2p_private_broadcast.py` goes from ~23s to ~14s locally.

  Fixes #35843

  Tested with:
  `build/test/functional/test_runner.py p2p_private_broadcast.py p2p_private_broadcast_retry_v1.py --timeout-factor=2`, and against the forced-feeler repro from the issue, which no longer mislabels the feeler.

ACKs for top commit:
  jeanpablojp:
    tACK 4e8c4bc794
  andrewtoth:
    ACK 4e8c4bc794
  mzumsande:
    Code Review ACK 4e8c4bc794

Tree-SHA512: ce2db418787d7ecf518bd49b37d7d664748fee5991a2924522dfaf42b27d90ca001caa0611011310636b453d3aada1061d086f20bb866eb66605645935f55c74

test/functional/p2p_private_broadcast.py

@@ -6,7 +6,6 @@
 Test how locally submitted transactions are sent to the network when private broadcast is used.
 """
 
-import re
 import time
 import threading
 
@@ -68,33 +67,32 @@ class P2PPrivateBroadcast(BitcoinTestFramework):
         self.trigger_no_relay_peer = False
         self.no_relay_peer = None
 
-        def find_connection_type_in_debug_log(to_addr, to_port):
-            """
-            Scan the debug log of tx_originator for a connection attempt to to_addr:to_port.
-            Return the connection type (outbound-full-relay, private-broadcast, etc) or
-            None if there is no connection attempt to to_addr:to_port.
-            """
-            with open(self.tx_originator_debug_log_path, mode="r", encoding="utf-8") as debug_log:
-                for line in debug_log.readlines():
-                    match = re.match(f".*trying v. connection \\((.+)\\) to \\[?{to_addr}]?:{to_port},.*", line)
-                    if match:
-                        return match.group(1)
-            return None
-
-        def destinations_factory(requested_to_addr, requested_to_port):
+        def destinations_factory(requested_to_addr, requested_to_port, proxy_client):
             """
             Instruct the SOCKS5 proxy to redirect connections:
             * The first automatic outbound connection -> P2PDataStore
             * The first private broadcast connection -> nodes[1]
             * Anything else -> P2PInterface
+
+            proxy_client is the client's socket address as seen by the proxy (host:port),
+            equal to the node's addrbind for this connection.
             """
             conn_type = None
-            def found_connection_in_debug_log():
-                nonlocal conn_type
-                conn_type = find_connection_type_in_debug_log(requested_to_addr, requested_to_port)
-                return conn_type is not None
+            # SOCKS handlers run in separate threads, so each needs its own RPC connection.
+            rpc = self.nodes[0].create_new_rpc_connection()
 
-            self.wait_until(found_connection_in_debug_log)
+            def connection_type_found():
+                nonlocal conn_type
+                # The proxy has already replied SUCCESS to the SOCKS5 request, so the node
+                # has finished ConnectNode and registered the peer (or is about to).
+                # The proxy client address equals the node's addrbind for this connection.
+                for peer in rpc.getpeerinfo():
+                    if peer.get("addrbind") == proxy_client:
+                        conn_type = peer["connection_type"]
+                        return True
+                return False
+
+            self.wait_until(connection_type_found)
 
             with self.destinations_lock:
                 i = len(self.destinations)
@@ -233,7 +231,6 @@ class P2PPrivateBroadcast(BitcoinTestFramework):
 
     def run_test(self):
         tx_originator = self.nodes[0]
-        self.tx_originator_debug_log_path = tx_originator.debug_log_path
         tx_receiver = self.nodes[1]
         far_observer = tx_receiver.add_p2p_connection(P2PInterface())
 

test/functional/p2p_private_broadcast_retry_v1.py

@@ -77,7 +77,7 @@ class P2PPrivateBroadcastRetryV1(BitcoinTestFramework):
         self.ipv4_via_tor_proxy_conn_versions.append(v2or1)
 
     def setup_nodes(self):
-        def destinations_factory_all_proxy(requested_to_addr, requested_to_port):
+        def destinations_factory_all_proxy(requested_to_addr, requested_to_port, _proxy_client):
             """
             Instruct the SOCKS5 proxy to redirect all connections to newly created P2PInterface
             objects that claim support for P2P_V2.
@@ -102,7 +102,7 @@ class P2PPrivateBroadcastRetryV1(BitcoinTestFramework):
         self.ipv4_via_tor_proxy_addr_port = None # Remember the first IPv4 address connected to via the Tor proxy.
         self.ipv4_via_tor_proxy_conn_versions = [] # Transport versions tried on that address.
 
-        def destinations_factory_tor_proxy(requested_to_addr, requested_to_port):
+        def destinations_factory_tor_proxy(requested_to_addr, requested_to_port, _proxy_client):
             """
             Instruct the SOCKS5 proxy to redirect all connections to newly created P2PInterface,
             except the first connection to an IPv4 address and all subsequent connections to that

test/functional/test_framework/socks5.py

@@ -104,6 +104,7 @@ class Socks5Configuration():
         # and it decides where the connection is redirected to. It is passed:
         # - the address the client requested to connect to
         # - the port the client requested to connect to
+        # - the client's socket address as seen by the proxy, formatted as host:port
         # It is supposed to return an object like:
         # {
         #     "actual_to_addr": "127.0.0.1"
@@ -140,8 +141,9 @@ class Socks5Connection():
         """Handle socks5 request according to RFC1928."""
         log_exception_prefix = "Socks5Connection.handle(): "
         try:
+            proxy_client = format_sock(self.conn, local=False)
             log_exception_prefix = ("Socks5Connection.handle("
-                                    f"client={format_sock(self.conn, local=False)}, "
+                                    f"client={proxy_client}, "
                                     f"proxy={format_sock(self.conn, local=True)}): ")
 
             # Verify socks version
@@ -193,7 +195,9 @@ class Socks5Connection():
             port_hi,port_lo = recvall(self.conn, 2)
             port = (port_hi << 8) | port_lo
 
-            # Send dummy response
+            # Reply SUCCESS before calling destinations_factory, so the client can finish
+            # establishing the connection and register the peer; factories that consult
+            # getpeerinfo depend on that order.
             self.conn.sendall(bytearray([0x05, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
 
             cmdin = Socks5Command(cmd, atyp, addr, port, username, password)
@@ -205,7 +209,7 @@ class Socks5Connection():
 
             if self.serv.is_running():
                 if self.serv.conf.destinations_factory is not None:
-                    dest = self.serv.conf.destinations_factory(requested_to_addr, port)
+                    dest = self.serv.conf.destinations_factory(requested_to_addr, port, proxy_client)
                     if dest is not None:
                         logger.debug(f"Serving connection to {requested_to}, will redirect it to "
                                     f"{dest['actual_to_addr']}:{dest['actual_to_port']} instead")