test: SOCKS5 proxy: expect that connection may be reset during SOCKS5 handshake or data forwarding #35510

vasild commented at 9:33 AM on June 11, 2026: contributor

The forward_sockets() function used by the SOCKS5 proxy forwards data between two connected sockets. It might happen that one of those sockets gets closed/reset abruptly, without sending EOF first. This is to be expected if e.g. bitcoind is shutdown and shouldn't result in noisy harmless messages like:

2026-06-03T13:23:56.966859Z TestFramework.socks5 (ERROR): socks5 request handling failed (running True)
Traceback (most recent call last):
  File ".../socks5.py", line 199, in handle
    forward_sockets(self.conn, conn_to, self.wakeup_socket_pair[1], self.serv)
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../socks5.py", line 76, in forward_sockets
    data = s.recv(4096)
ConnectionResetError: [Errno 104] Connection reset by peer

Instead turn this into a debug log message with a nice prefix containing enough information to identify the two forwarded sockets.

Also expect that the connection might be closed during the SOCKS5 handshake and only log a debug message if that happens.

DrahtBot added the label Tests on Jun 11, 2026

DrahtBot commented at 9:33 AM on June 11, 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/35510.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	optout21, danielabrozzoni

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

danielabrozzoni commented at 9:51 AM on June 11, 2026: member

Concept ACK! I was about to open PR myself, haha :)

I wonder if it's worth it to also catch BrokenPipeError, although IIUC it's harder to hit. My attempt at this was: https://github.com/danielabrozzoni/bitcoin/commit/d9ccf9c4ec5616d5f1dabcaf2ab7bb4c73a8fdbc

Also, I was wondering if this could be removed in p2p_private_broadcast.py - was it added because of the ConnectionResetError, or for some other reason? https://github.com/bitcoin/bitcoin/blob/46d0e21d758c598017b5a9d86381c1c8ccfbe94d/test/functional/p2p_private_broadcast.py#L382-L385

vasild force-pushed on Jun 11, 2026

vasild commented at 9:57 AM on June 11, 2026: contributor

@optout21, you did some improvements around this code recently, might be interested into this PR @danielabrozzoni, you reported this in #35410#pullrequestreview-4419151702, might be interested as well

DrahtBot added the label CI failed on Jun 11, 2026

DrahtBot commented at 9:58 AM on June 11, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/27337603147/job/80765794480 LLM reason (✨ experimental): CI failed due to a Python linter syntax error (invalid f-string in test/functional/test_framework/socks5.py:91, “expecting '}'”).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

vasild commented at 10:33 AM on June 11, 2026: contributor

@danielabrozzoni, yes, I think handling of BrokenPipeError should be done as well. That is, it is handled even now but it would result in noisy error messages. I mean to change that to a debug log message and conclude the forwarding without too much excitement. recv() will not result in BrokenPipeError, but send() could. Maybe your approach of doing both recv() and sendall() in one try/except block is better.

About the comment you referred above: "disconnecting in the middle of the SOCKS5 handshake" -- that will not be affected by changes in forward_sockets(). The handshake is done in Socks5Connection.handle() before it calls forward_sockets(). In general, I think abrupt socket errors from recv() or send() during the SOCKS5 handshake or the forwarding should only result in debug log message.

in test/functional/test_framework/socks5.py:91 in 8a17a9f510

  89 | -            data = s.recv(4096)
  90 | +            data = None
  91 | +            try:
  92 | +                data = s.recv(4096)
  93 | +            except ConnectionResetError:
  94 | +                logger.debug(f"{log_prefix}connection reset on socket {'a' if s == a else 'b'}")

optout21 commented at 10:43 AM on June 11, 2026:

8a17a9f test: SOCKS5 proxy: expect that connection may be reset when forwarding:

There is a behavior change I notice here: with this exception being caught here, it will not get to the catch in handle(), so self.serv.queue.put(e) will not be executed. Isn't that a problem?

vasild commented at 11:40 AM on June 11, 2026:

Hmm, I missed that. Looking at how the queue is used - it is appended to in the proxy - once with the received SOCKS5 command and once with the exception. Then it is only read from feature_proxy.py and there is an assert that elements in the queue must be of type Socks5Command.

This means that nobody is actively seeking or checking for the exception put in the queue. So nobody will miss it if we do not put it there. Thus, "Isn't that a problem?", I guess the answer is "no".

And then a new question - why do we put the exception in the queue? The most it can happen is that its existence can trigger the asserts in feature_proxy.py that it is not of type Socks5Command.

optout21 commented at 11:49 AM on June 11, 2026:

Thanks for checking. I guess that if ever in the future a tests wants to check this exception, it can add to the queue, no need to worry about it now.

optout21 commented at 10:46 AM on June 11, 2026: contributor

crACK 9a8ef9b0a377faae11a8a07af23978c4cd573fcb

Re-reviewed, LGTM.

Prev: crACK 8a17a9f510e4b1abb6511850759fcc126e1a61fe

The log messages are extended with socket endpoint info, in two error cases, that's nice. There is a new catch & log for the Connection reset case, which is also nice. There is a behavior change I noticed, see comment. The change is around the code last affected by #34863, but it's rather orthogonal. Test code only.

DrahtBot requested review from danielabrozzoni on Jun 11, 2026

DrahtBot removed the label CI failed on Jun 11, 2026

vasild force-pushed on Jun 11, 2026

vasild commented at 1:08 PM on June 11, 2026: contributor

8a17a9f510e4b1abb6511850759fcc126e1a61fe...1ad1d33bb6d170fdfd6948985000fd8575d21c6f: expand to soft-handle abrupt connection closes also during the SOCKS5 handshake, in addition to the forwarding of sockets. Also catch BrokenPipeError from sending during forwarding, thanks @danielabrozzoni for the suggestion!

Also, I was wondering if this could be removed in p2p_private_broadcast.py - was it added because of the ConnectionResetError, or for some other reason?

I do not remember, but probably yes, this was the reason it was added.

Now, without this PR (bare master) I tried to remove that self.socks5_server.stop() and ran p2p_private_broadcast.py many times - it passes cleanly without any python backtraces printed. So, my guess is that the self.socks5_server.stop() can be removed, but I am wary of removing it without being able to confirm that 1. on master without stop() ugly exceptions are being printed; and 2. this PR silences them.

vasild renamed this:
~~test: SOCKS5 proxy: expect that connection may be reset when forwarding~~
test: SOCKS5 proxy: expect that connection may be reset during SOCKS5 handshake or data forwarding
on Jun 11, 2026

in test/functional/test_framework/socks5.py:222 in 1ad1d33bb6

 215 | @@ -216,7 +216,14 @@ def handle(self):
 216 |                  else:
 217 |                      logger.debug(f"Can't serve the connection to {requested_to}: no destinations factory")
 218 |  
 219 | -            # Fall through to disconnect
 220 | +            # Disconnect happens in the "finally" block below.
 221 | +
 222 | +        except (BrokenPipeError, ConnectionResetError) as e:
 223 | +            client_addr, client_port = self.conn.getpeername()

danielabrozzoni commented at 6:41 PM on June 12, 2026:

On MacOS, if I hit the ConnectionResetError, I then get a OSError on getpeername:

2026-06-12T17:24:19.553188Z TestFramework (INFO): Tor proxy: got 180.0.0.1:8333 v2, v1
Exception in thread Thread-36 (handle):
Traceback (most recent call last):
  File "/Users/daniela/Developer/BitcoinCore/bitcoin/test/functional/test_framework/socks5.py", line 166, in handle
    ver = recvall(self.conn, 1)[0]
          ~~~~~~~^^^^^^^^^^^^^^
  File "/Users/daniela/Developer/BitcoinCore/bitcoin/test/functional/test_framework/socks5.py", line 34, in recvall
    d = s.recv(n)
ConnectionResetError: [Errno 54] Connection reset by peer

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/homebrew/Cellar/python@3.14/3.14.5/Frameworks/Python.framework/Versions/3.14/lib/python3.14/threading.py", line 1082, in _bootstrap_inner
    self._context.run(self.run)
    ~~~~~~~~~~~~~~~~~^^^^^^^^^^
  File "/opt/homebrew/Cellar/python@3.14/3.14.5/Frameworks/Python.framework/Versions/3.14/lib/python3.14/threading.py", line 1024, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/daniela/Developer/BitcoinCore/bitcoin/test/functional/test_framework/socks5.py", line 222, in handle
    client_addr, client_port = self.conn.getpeername()
                               ~~~~~~~~~~~~~~~~~~~~~^^
OSError: [Errno 22] Invalid argument

I think it might be because the socket is shutting down and MacOS returns EINVAL: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/getpeername.2.html

It should be ok if you put these assignments at the start of handle(), at the start of the try block

vasild commented at 9:01 AM on June 15, 2026:

Right, tweaked it like that. Better now?

How do you trigger the ConnectionResetError? I pepper the code with some close()s to trigger the errors, but that is not convenient and too artificial. Do you have a better way? :)

danielabrozzoni commented at 1:05 PM on June 15, 2026:

Thanks for fixing it! I don't have a way to trigger the errors, I just let the functional tests run indefinitely until I see something, haha :)

I saw a similar error on Linux in forward_sockets... However, I'm not sure if it's worth fixing it. It's pretty hard to reach, and, similarly to other ConnectionResetErrors, it doesn't make the test fail.

2026-06-15T11:32:33.993355Z TestFramework.socks5 (ERROR): Socks5Connection.handle(client=127.0.0.1:34752, proxy=127.0.0.1:37927): exception: [Errno 107] Transport endpoint is not connected (running True)
Traceback (most recent call last):
  File "/home/daniela/Developer/BitcoinCore/bitcoin/test/functional/test_framework/socks5.py", line 213, in handle
    forward_sockets(self.conn, conn_to, self.wakeup_socket_pair[1], self.serv)
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/daniela/Developer/BitcoinCore/bitcoin/test/functional/test_framework/socks5.py", line 62, in forward_sockets
    f"a{{remote={format_sock(a, local=False)}, local={format_sock(a, local=True)}}} <-> "
                 ~~~~~~~~~~~^^^^^^^^^^^^^^^^
  File "/home/daniela/Developer/BitcoinCore/bitcoin/test/functional/test_framework/netutil.py", line 226, in format_sock
    name = sock.getpeername()
OSError: [Errno 107] Transport endpoint is not connected
2026-06-15T11:32:33.993355Z TestFramework.socks5 (ERROR): Socks5Connection.handle(client=127.0.0.1:34752, proxy=127.0.0.1:37927): exception: [Errno 107] Transport endpoint is not connected (running True)
Traceback (most recent call last):
  File "/home/daniela/Developer/BitcoinCore/bitcoin/test/functional/test_framework/socks5.py", line 213, in handle
    forward_sockets(self.conn, conn_to, self.wakeup_socket_pair[1], self.serv)
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/daniela/Developer/BitcoinCore/bitcoin/test/functional/test_framework/socks5.py", line 62, in forward_sockets
    f"a{{remote={format_sock(a, local=False)}, local={format_sock(a, local=True)}}} <-> "
                 ~~~~~~~~~~~^^^^^^^^^^^^^^^^
  File "/home/daniela/Developer/BitcoinCore/bitcoin/test/functional/test_framework/netutil.py", line 226, in format_sock
    name = sock.getpeername()
OSError: [Errno 107] Transport endpoint is not connected

vasild commented at 7:25 AM on June 16, 2026:

What do you think about the following in format_sock()?

-    if local:
-        name = sock.getsockname()
-    else:
-        name = sock.getpeername()
+    try:
+        if local:
+            name = sock.getsockname()
+        else:
+            name = sock.getpeername()
+    except:
+        return "n/a"

Given that format_sock() is just for display purposes, maybe it should return whatever string instead of throwing an exception if it cannot do its job?

danielabrozzoni commented at 9:35 AM on June 16, 2026:

Yes, I think that would be better! Happy to reACK if you retouch

vasild commented at 1:15 PM on June 16, 2026:

Done!

vasild force-pushed on Jun 15, 2026

vasild commented at 8:57 AM on June 15, 2026: contributor

1ad1d33bb6d170fdfd6948985000fd8575d21c6f...ffbf9dc133749f91f876c53adfa516d3270241a4: do #35510 (review)

vasild force-pushed on Jun 15, 2026

DrahtBot added the label CI failed on Jun 15, 2026

vasild commented at 9:02 AM on June 15, 2026: contributor

ffbf9dc133749f91f876c53adfa516d3270241a4...fa604eaed3d0c75d77200fa1ad43bc44635221fd: remove Python f-string where not needed.

DrahtBot commented at 9:02 AM on June 15, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/27535281850/job/81383045885 LLM reason (✨ experimental): CI failed due to a Python lint (ruff) error: an extraneous f prefix in test/functional/test_framework/socks5.py (line 145).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

vasild force-pushed on Jun 15, 2026

vasild commented at 10:50 AM on June 15, 2026: contributor

fa604eaed3d0c75d77200fa1ad43bc44635221fd...216f3f3fb279fe6499fc274557d82cc53c259f1c: introduce format_sock() in netutil.py to help with visualizing either the local or a remote side of a socket.

DrahtBot removed the label CI failed on Jun 15, 2026

danielabrozzoni commented at 1:58 PM on June 15, 2026: member

tACK 216f3f3fb279fe6499fc274557d82cc53c259f1c

Thanks for taking up my suggestions @vasild! And thanks for taking the time to explain what's happening in #35510 (comment)

I ran the functional test for a while on both linux and macos and the only verbose exception logging I saw was #35510 (review). This one could be fixed by catching OSError in format_sock, but I think it's okay even without doing so.

Re #35510 (comment):

Now, without this PR (bare master) I tried to remove that self.socks5_server.stop() and ran p2p_private_broadcast.py many times - it passes cleanly without any python backtraces printed. So, my guess is that the self.socks5_server.stop() can be removed, but I am wary of removing it without being able to confirm that 1. on master without stop() ugly exceptions are being printed; and 2. this PR silences them.

Agreed. I wasn't really able to see any ugly exceptions on master either, I think it's okay to leave as-is.

DrahtBot requested review from optout21 on Jun 15, 2026

vasild force-pushed on Jun 16, 2026

vasild commented at 1:15 PM on June 16, 2026: contributor

216f3f3fb279fe6499fc274557d82cc53c259f1c...e2e78dba3df9e9083c4651c27def75cbc996b399: do #35510 (review)

DrahtBot added the label CI failed on Jun 16, 2026

DrahtBot commented at 2:29 PM on June 16, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/27620193042/job/81666644414 LLM reason (✨ experimental): CI failed because the Python lint step (ruff/py_lint) reported an error: bare except (E722) in test/functional/test_framework/netutil.py.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

test: SOCKS5 proxy: expect that connection may be reset when forwarding

The `forward_sockets()` function used by the SOCKS5 proxy forwards data
between two connected sockets. It might happen that one of those sockets
gets closed/reset abruptly, without sending EOF first. This is to be
expected if e.g. `bitcoind` is shutdown and shouldn't result in noisy
harmless messages like:

```
2026-06-03T13:23:56.966859Z TestFramework.socks5 (ERROR): socks5 request handling failed (running True)
Traceback (most recent call last):
  File ".../socks5.py", line 199, in handle
    forward_sockets(self.conn, conn_to, self.wakeup_socket_pair[1], self.serv)
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../socks5.py", line 76, in forward_sockets
    data = s.recv(4096)
ConnectionResetError: [Errno 104] Connection reset by peer
```

Instead turn this into a debug log message with a nice prefix containing
enough information to identify the two forwarded sockets.

eb3208364a

test: SOCKS5 proxy: expect that connection may be reset during handshake

The client (e.g. `bitcoind`) may open a connection to the proxy and
close it in the middle of the SOCKS5 handshake if it is restarted for
example. Log these as debug messages instead of full blown Python
exception error messages with backtraces.

9a8ef9b0a3

vasild force-pushed on Jun 17, 2026

vasild commented at 6:38 AM on June 17, 2026: contributor

e2e78dba3df9e9083c4651c27def75cbc996b399...9a8ef9b0a377faae11a8a07af23978c4cd573fcb: do not use bare except in Python

DrahtBot removed the label CI failed on Jun 17, 2026

danielabrozzoni commented at 8:53 AM on June 17, 2026: member

reACK 9a8ef9b0a377faae11a8a07af23978c4cd573fcb

Thanks for picking up all my suggestions! I run the tests for a while and haven't seen any ugly exceptions printed :)