test: classify SOCKS5 peers via getpeerinfo addrbind #35867

151henry151 commented at 6:54 PM on August 2, 2026: contributor

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.

DrahtBot added the label Tests on Aug 2, 2026

DrahtBot commented at 6:55 PM on August 2, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35867.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	andrewtoth
Concept ACK	instagibbs, mzumsande

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

andrewtoth approved

andrewtoth commented at 7:29 PM on August 2, 2026: contributor

tACK 2d62d83e6709802e83d6b6ad8158958c5996195b

Here's a minimal reproducer. It fails before this change and passes after. Put it at test/functional/p2p_private_broadcast_reproducer.py and run it with python3 test/functional/p2p_private_broadcast_reproducer.py --configfile=build/test/config.ini. Replace build with your build directory.

<details><summary>Reproducer</summary>

#!/usr/bin/env python3
# Copyright (c) The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""
Minimal reproducer for [#35843](/bitcoin-bitcoin/35843/).
"""

import re

from p2p_private_broadcast import P2PPrivateBroadcast
from test_framework.authproxy import JSONRPCException
from test_framework.messages import CAddress
from test_framework.util import assert_equal
from test_framework.wallet import MiniWallet


class P2PPrivateBroadcastRepro(P2PPrivateBroadcast):
    def set_test_params(self):
        super().set_test_params()

    def count_feeler_attempts(self, addr, port):
        with open(self.tx_originator_debug_log_path, encoding="utf-8") as log:
            return len(re.findall(f"trying v. connection \\(feeler\\) to \\[?{re.escape(addr)}]?:{port},", log.read()))

    def run_test(self):
        node = self.nodes[0]
        self.tx_originator_debug_log_path = node.debug_log_path
        self.fill_node_addrman(node_index=0, address_types_to_add=[CAddress.NET_IPV4, CAddress.NET_IPV6, CAddress.NET_TORV3])
        wallet = MiniWallet(node)

        self.log.info("Privately broadcast a tx and wait for a clearnet private-broadcast destination")
        def clearnet_pb_dest():
            with self.destinations_lock:
                return next((d for d in self.destinations
                             if d["conn_type"] == "private-broadcast" and not d["requested_to"].endswith(".onion:8333")), None)
        for _ in range(10):
            node.sendrawtransaction(hexstring=wallet.create_self_transfer()["hex"], maxfeerate=0.1)
            try:
                self.wait_until(lambda: clearnet_pb_dest() is not None, timeout=15)
                break
            except AssertionError:
                self.log.info("No clearnet destination yet, trying another tx")
        addr_port = clearnet_pb_dest()["requested_to"]
        addr, port = addr_port.rsplit(":", 1)
        addr = addr.strip("[]")
        self.log.info(f"Private broadcast used {addr_port}")

        # Wait for all broadcasts to be confirmed so no pending private-broadcast
        # connection interferes, then for the reused address to disconnect
        # (addconnection is a no-op while still connected).
        self.wait_until(lambda: all(sum(1 for p in t["peers"] if "received" in p) >= 3
                                    for t in node.getprivatebroadcastinfo()["transactions"]))
        self.wait_until(lambda: all(p["addr"] != addr_port for p in node.getpeerinfo()))

        self.log.info(f"Force a feeler to {addr_port} and check its classification")
        with self.destinations_lock:
            num_destinations_before = len(self.destinations)
        feelers_before = self.count_feeler_attempts(addr, port)

        def feeler_attempted():
            if self.count_feeler_attempts(addr, port) > feelers_before:
                return True
            if all(p["addr"] != addr_port for p in node.getpeerinfo()):
                try:
                    node.addconnection(addr_port, "feeler", False)
                except JSONRPCException:
                    # At outbound capacity: free a slot and retry.
                    for p in node.getpeerinfo():
                        if p["addr"] != addr_port and p["connection_type"] in ("outbound-full-relay", "block-relay-only", "feeler"):
                            node.disconnectnode(address=p["addr"])
                            break
            return False
        self.wait_until(feeler_attempted)

        def classified():
            with self.destinations_lock:
                return next((d for d in self.destinations[num_destinations_before:]
                             if d["requested_to"] == addr_port), None)
        self.wait_until(lambda: classified() is not None)
        assert_equal(classified()["conn_type"], "feeler")
        self.log.info("OK: the forced feeler was classified as a feeler")

        self.socks5_server.stop()


if __name__ == "__main__":
    P2PPrivateBroadcastRepro(__file__).main()

</details>

maflcko added this to the milestone 32.0 on Aug 3, 2026

fanquake added the label Private Broadcast on Aug 3, 2026

fanquake commented at 8:51 AM on August 3, 2026: member

cc @vasild

fanquake requested review from instagibbs on Aug 3, 2026

instagibbs commented at 2:17 PM on August 3, 2026: member

https://github.com/instagibbs/bitcoin/tree/2026-08-classify_priv_conn @151henry151 an alternative commit for your consideration to remove the need for log scraping and potential misclassification. Let me know what you think, feel free to take any part of it

151henry151 commented at 2:32 PM on August 3, 2026: contributor

@151henry151 an alternative commit for your consideration to remove the need for log scraping and potential misclassification. Let me know what you think, feel free to take any part of it

At a glance, that looks preferable. I'll take a closer look and update the PR this evening when I'm at a keyboard. Thanks!

151henry151 renamed this:
~~test: classify SOCKS5 peers by latest debug-log connection type~~
test: classify SOCKS5 peers via getpeerinfo addrbind
on Aug 3, 2026

151henry151 force-pushed on Aug 3, 2026

151henry151 commented at 10:46 PM on August 3, 2026: contributor

@instagibbs took your addrbind approach — thanks. Force-pushed a single commit that classifies via getpeerinfo, matching the proxy client socket to the peer's addrbind, and drops the debug.log scrape. socks5.py now passes the client address into destinations_factory, so p2p_private_broadcast_retry_v1.py picks up the signature change.

A few small changes from your patch: documented proxy_client on the factory docstring so the third argument is clear at the call site; renamed the wait predicate to connection_type_found so it reads as a bool check rather than something that returns the type; noted in that predicate that SUCCESS is already sent so ConnectNode has registered the peer (otherwise the getpeerinfo wait looks like a deadlock); hoisted proxy_client so we don't call format_sock twice on the same socket; and replaced the "dummy response" comment in socks5.py with that same ordering note so the SUCCESS-before-factory invariant isn't silently broken later.

instagibbs commented at 11:00 AM on August 4, 2026: member

approach and concept ACK

mzumsande commented at 11:17 AM on August 4, 2026: contributor

Concept ACK, great to get rid of the reliance on the log (https://github.com/bitcoin/bitcoin/pull/34410#pullrequestreview-3875564818). @151henry151 would be good to add instagibbs as co-author (Co-authored-by).

test: classify SOCKS5 peers via getpeerinfo addrbind

The SOCKS5 destinations factory classified connections by scanning
debug.log for connection attempts to the requested address and port.
The destination is not unique per connection, so the log cannot
identify which attempt is being served: first-match returned a stale
type when an automatic connection reused an address private broadcast
had already used, and latest-match still breaks if two attempts to the
same address overlap.

Match the exact connection instead: the source addr:port of the
proxy's client socket equals the node's addrbind for that peer, so
looking it up in getpeerinfo identifies precisely the connection being
served and returns its connection_type. This also stops treating
debug.log contents as a stable interface.

Co-authored-by: Greg Sanders <gsanders87@gmail.com>

4e8c4bc794

151henry151 force-pushed on Aug 4, 2026

151henry151 commented at 5:56 PM on August 4, 2026: contributor

Thanks @mzumsande — added Co-authored-by: Greg Sanders <gsanders87@gmail.com> and force-pushed (4e8c4bc794). @instagibbs — thanks again for the addrbind approach; classifying via the proxy client / getpeerinfo addrbind made it possible to drop the debug.log scrape cleanly. (Skipping an @ mention in the PR description per CONTRIBUTING.md.)

andrewtoth approved

andrewtoth commented at 1:50 AM on August 5, 2026: contributor

ACK 4e8c4bc794c045beb678854e6e326fc04322c7c3

I managed to generate a reproducer that fails on master and the previous last log approach, but passes with this approach. This test instead creates a private broadcast connection but stalls before the SOCKS5 handshake finishes, then induces a feeler connection to the same address. So, the last log will be the feeler, but we will still correctly classify the private broadcast connection.

Run by copying the script to test/functional/p2p_private_broadcast_reproducer.py and running python3 test/functional/p2p_private_broadcast_reproducer.py --configfile=build/test/config.ini and replace build with your build directory.

<details><summary>Reproducer</summary>

#!/usr/bin/env python3
# Copyright (c) The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""
Discriminating reproducer for the residual race in log-based SOCKS5 peer
classification (bitcoin/bitcoin#35867).

Log-based classification (both first-match and latest-match) keys on the
requested address, which is ambiguous when two connection attempts to the same
address overlap. The getpeerinfo/addrbind approach keys on the connection
itself, so it cannot be confused. The overlap window is sub-millisecond in
practice, so this script widens it by simulating a slow proxy:

1. The proxy-side client socket is wrapped so that the SOCKS5 SUCCESS reply
   for a designated connection is withheld. The node then sits blocked in
   Socks5() and has NOT yet registered the peer, so its own already-connected
   dedup cannot prevent a second attempt to the same address.
2. A private-broadcast connection to a clearnet address X is designated.
   While it is stalled, a feeler to X is forced via addconnection, writing a
   second "trying ... (feeler) ... to X" line into debug.log.
3. The stall is released (SUCCESS delivered, node registers the peer) and the
   parent test's unmodified destinations factory classifies the connection.

Expected results for the destinations entries of X:
- master (first log match):      feeler misclassified as private-broadcast -> FAIL
- latest log match:              private-broadcast misclassified as feeler -> FAIL
- getpeerinfo addrbind (#35867): both classified correctly -> PASS

Only the proxy's reply timing is instrumented. The node tolerates it: its
SOCKS5 reply timeout is 20s and the stall lasts well under that.
"""

import threading

import test_framework.socks5 as socks5_module

from p2p_private_broadcast import P2PPrivateBroadcast
from test_framework.authproxy import JSONRPCException
from test_framework.messages import CAddress
from test_framework.netutil import format_addr_port
from test_framework.util import assert_equal
from test_framework.wallet import MiniWallet

SOCKS5_SUCCESS_REPLY = bytes([0x05, 0x00, 0x00, 0x01] + [0x00] * 6)


class StallController:
    """Coordinates the SUCCESS-withholding between the socket wrapper (which
    sees the reply but not the address), the factory wrapper (which sees the
    address and runs on the same handler thread), and the main test thread."""
    def __init__(self):
        self.lock = threading.Lock()
        self.hunting = False              # looking for a connection to stall
        self.designated_thread = None     # handler thread whose SUCCESS is withheld
        self.withheld = None              # (raw socket, SUCCESS bytes) to send on release
        self.stalled_addr_port = None
        self.captured = threading.Event() # a clearnet connection is stalled
        self.release = threading.Event()

    def try_designate(self, raw_sock, data):
        with self.lock:
            if not self.hunting or self.designated_thread is not None:
                return False
            self.designated_thread = threading.current_thread()
            self.withheld = (raw_sock, data)
            return True

    def send_withheld(self):
        with self.lock:
            raw_sock, data = self.withheld
            self.withheld = None
            self.designated_thread = None
        raw_sock.sendall(data)


CONTROLLER = StallController()


class InterceptingSocket:
    """Delegates everything to the wrapped socket, but can withhold the SOCKS5
    SUCCESS reply. Only sendall() is intercepted; the data forwarding path
    (forward_sockets) uses send() and is unaffected."""
    def __init__(self, sock):
        self._sock = sock

    def __getattr__(self, name):
        return getattr(self._sock, name)

    def sendall(self, data):
        d = bytes(data)
        if d == SOCKS5_SUCCESS_REPLY and CONTROLLER.try_designate(self._sock, d):
            return  # withheld; the factory wrapper sends it on release
        return self._sock.sendall(d)


_orig_socks5_conn_init = socks5_module.Socks5Connection.__init__

def _patched_socks5_conn_init(conn_self, serv, conn):
    _orig_socks5_conn_init(conn_self, serv, conn)
    conn_self.conn = InterceptingSocket(conn_self.conn)

socks5_module.Socks5Connection.__init__ = _patched_socks5_conn_init


class P2PPrivateBroadcastDiscriminator(P2PPrivateBroadcast):
    # BitcoinTestMetaClass requires this override; inheriting is not enough.
    def set_test_params(self):
        super().set_test_params()

    def force_feeler(self, addr_port):
        node = self.nodes[0]

        def feeler_added():
            try:
                node.addconnection(addr_port, "feeler", False)
                return True
            except JSONRPCException:
                # At outbound capacity: free a slot and retry.
                for peer in node.getpeerinfo():
                    if peer["connection_type"] in ("outbound-full-relay", "block-relay-only", "feeler"):
                        node.disconnectnode(address=peer["addr"])
                        break
                return False
        self.wait_until(feeler_added)

    def run_test(self):
        node = self.nodes[0]
        # Read by the log-scanning factories (pre-#35867), normally set by the
        # parent's run_test, which we override.
        self.tx_originator_debug_log_path = node.debug_log_path

        # The handler thread sends (here: withholds) SUCCESS right before
        # calling the factory, so thread identity pairs the two. *args because
        # the factory signature grew a third parameter in [#35867](/bitcoin-bitcoin/35867/).
        parent_factory = self.socks5_server.conf.destinations_factory

        def stalling_factory(*args):
            requested_addr, requested_port = args[0], args[1]
            if threading.current_thread() is CONTROLLER.designated_thread:
                if requested_addr.endswith((".onion", ".i2p")):
                    # Only a clearnet victim will do (addconnection cannot
                    # force a feeler to onion): let this one go, keep hunting.
                    CONTROLLER.send_withheld()
                else:
                    with CONTROLLER.lock:
                        CONTROLLER.hunting = False
                        CONTROLLER.stalled_addr_port = format_addr_port(requested_addr, requested_port)
                    CONTROLLER.captured.set()
                    # Must stay under the node's 20s SOCKS5 reply timeout.
                    CONTROLLER.release.wait(timeout=15)
                    CONTROLLER.send_withheld()
            return parent_factory(*args)

        self.socks5_server.conf.destinations_factory = stalling_factory

        self.fill_node_addrman(node_index=0, address_types_to_add=[CAddress.NET_IPV4, CAddress.NET_IPV6, CAddress.NET_TORV3])
        wallet = MiniWallet(node)

        def broadcast_txs_until(done):
            """Privately broadcast fresh txs until done() returns True."""
            for _ in range(10):
                node.sendrawtransaction(hexstring=wallet.create_self_transfer()["hex"], maxfeerate=0.1)
                if done():
                    return
            raise AssertionError("condition not reached after 10 transactions")

        self.log.info("Warmup: broadcast until a clearnet address is used (proves Tor is ok, which enables clearnet picks)")
        def used_clearnet():
            with self.destinations_lock:
                return any(d["conn_type"] == "private-broadcast" and not d["requested_to"].endswith(".onion:8333")
                           for d in self.destinations)
        def wait_used_clearnet():
            try:
                self.wait_until(used_clearnet, timeout=15)
                return True
            except AssertionError:
                return False
        broadcast_txs_until(wait_used_clearnet)
        # Let all pending broadcasts finish so they don't interfere below.
        self.wait_until(lambda: all(sum(1 for p in t["peers"] if "received" in p) >= 3
                                    for t in node.getprivatebroadcastinfo()["transactions"]))

        self.log.info("Arm the stall and broadcast until a clearnet private-broadcast connection is captured mid-handshake")
        with self.destinations_lock:
            baseline = len(self.destinations)
        with CONTROLLER.lock:
            CONTROLLER.hunting = True
        broadcast_txs_until(lambda: CONTROLLER.captured.wait(timeout=10))
        addr_port = CONTROLLER.stalled_addr_port
        self.log.info(f"Captured and stalled private-broadcast connection to {addr_port}")

        self.log.info("While it is stalled (peer not yet registered in the node), force a feeler to the same address")
        self.force_feeler(addr_port)

        def entries_for_addr():
            with self.destinations_lock:
                return [d for d in self.destinations[baseline:] if d["requested_to"] == addr_port]
        self.wait_until(lambda: len(entries_for_addr()) == 1)  # the feeler; the stalled connection is not classified yet
        assert_equal(entries_for_addr()[0]["conn_type"], "feeler")  # fails with first-match (stale private-broadcast line)
        self.log.info("OK: concurrent feeler classified as feeler")

        self.log.info("Release the stall; the private-broadcast connection now gets classified")
        CONTROLLER.release.set()
        self.wait_until(lambda: len(entries_for_addr()) == 2)
        pb_entry = entries_for_addr()[1]

        # Guard: nServices=0 in the dummy version proves this really is a
        # private broadcast connection, independent of the classification.
        peer = pb_entry["node"]
        peer.wait_until(lambda: peer.message_count["version"] == 1, check_connected=False)
        assert_equal(peer.last_message["version"].nServices, 0)

        assert_equal(pb_entry["conn_type"], "private-broadcast")  # fails with latest-match (shadowed by the feeler line)
        self.log.info("OK: stalled private-broadcast connection classified as private-broadcast")

        self.socks5_server.stop()


if __name__ == "__main__":
    P2PPrivateBroadcastDiscriminator(__file__).main()

</details>

DrahtBot requested review from mzumsande on Aug 5, 2026