torcontrol: Use reconnect backoff after dropped connections #36260

fjahr commented at 12:10 PM on September 15, 2026: contributor

Since #34158 the reconnect backoff was only applied when connecting to the Tor control port failed. When an established connection was dropped, for example by Tor closing it after a failed AUTHENTICATE because of a wrong password, the control thread reconnected immediately in a loop without any wait. This was resulting in us trying to make tons of connections to torcontrol and producing tons of log entries in very short time. The fix restores the pre-#34158 behavior where every reconnect waits for the backoff timeout by going through disconnected_cb, which now also does the waiting. Also adds a functional test to cover that the waiting behavior is actually applied.

DrahtBot commented at 12:10 PM on September 15, 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/36260.

Reviews

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

Type	Reviewers
ACK	willcl-ark, winterrdog, sedited
Concept ACK	fanquake

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36142 (net: validate Tor onion service replies and cached keys by l0rinc)
#35292 (test: Add coverage for Tor control HASHEDPASSWORD authentication by winterrdog)
#34486 (net: Reduce local network activity when networkactive=0 by willcl-ark)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

willcl-ark approved

willcl-ark commented at 2:01 PM on September 15, 2026: member

crACK 7fb0f7536773adb7cd7e52ff1be8adb455381bee

Restoring the reconnect backoff seems wise here; no point spamming the tor daemon with failures and flooding our own logs with fail spam in the case that e.g. the tor daemon is restarting or something.

The functional test covers the missing delay after a server closes the connection which seems OK.

fanquake requested review from pinheadmz on Sep 15, 2026

fanquake commented at 2:03 PM on September 15, 2026: member

https://github.com/bitcoin/bitcoin/actions/runs/34967300433/job/104380659347?pr=36260#step:11:1818:


 test  2026-09-15T13:34:29.088864Z TestFramework.utils (ERROR): wait_until() failed. Predicate: '''' 
                                               self.wait_until(lambda: len(mock_tor.received_commands) == initial_len + 1)
                                   '''
 test  2026-09-15T13:34:29.089353Z TestFramework (ERROR): Unexpected exception: 
                                   Traceback (most recent call last):
                                     File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_framework.py", line 145, in main
                                       self.run_test()
                                       ~~~~~~~~~~~~~^^
                                     File "D:\a\bitcoin\bitcoin/test/functional/feature_torcontrol.py", line 280, in run_test
                                       self.test_reconnect_backoff()
                                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
                                     File "D:\a\bitcoin\bitcoin/test/functional/feature_torcontrol.py", line 267, in test_reconnect_backoff
                                       with self.expect_disconnect(True, mock_tor):
                                            ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^
                                     File "C:\hostedtoolcache\windows\Python\3.14.7\x64\Lib\contextlib.py", line 148, in __exit__
                                       next(self.gen)
                                       ~~~~^^^^^^^^^^
                                     File "D:\a\bitcoin\bitcoin/test/functional/feature_torcontrol.py", line 138, in expect_disconnect
                                       self.wait_until(lambda: len(mock_tor.received_commands) == initial_len + 1)
                                       ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_framework.py", line 747, in wait_until
                                       return wait_until_helper_internal(test_function, timeout=timeout, timeout_factor=self.options.timeout_factor, check_interval=check_interval)
                                     File "D:\a\bitcoin\bitcoin\test\functional\test_framework\util.py", line 451, in wait_until_helper_internal
                                       raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
                                   AssertionError: Predicate ''''
                                               self.wait_until(lambda: len(mock_tor.received_commands) == initial_len + 1)
                                   ''' not true after 2400.0 seconds
 node0 2026-09-15T13:34:29.109503Z [torcontrol] [torcontrol.cpp:161] [ReceiveAndProcess] [warning] tor: Error reading from socket: An existing connection was forcibly closed by the remote host.  (10054) 
 test  2026-09-15T13:34:29.109532Z TestFramework (DEBUG): Closing down network thread

DrahtBot added the label CI failed on Sep 15, 2026

fjahr force-pushed on Sep 15, 2026

torcontrol: Apply reconnect backoff after dropped connections 4556ef6267

in test/functional/feature_torcontrol.py:270 in 7fb0f75367

 265 | +        self.restart_with_mock(mock_tor)
 266 | +
 267 | +        with self.expect_disconnect(True, mock_tor):
 268 | +            # Reply before closing, like Tor does after a failed AUTHENTICATE
 269 | +            mock_tor.send_raw("515 Authentication failed\r\n")
 270 | +            mock_tor.conn.shutdown(socket.SHUT_RDWR)

willcl-ark commented at 4:15 PM on September 15, 2026:

In 7fb0f7536773adb7cd7e52ff1be8adb455381bee

Codex thinks that we should use this in the test:

            mock_tor.conn.shutdown(socket.SHUT_WR)

With the explanation:

The mock server handles one connection at a time. It must finish reading the old connection before returning to accept() to handle Bitcoin Core’s reconnect.

The test originally used:
mock_tor.conn.shutdown(socket.SHUT_RDWR)
That shuts down both sending and receiving. Bitcoin Core sees EOF and reconnects as expected. But on Windows, the mock’s next recv() can raise OSError because receiving was shut down. That exception reaches _serve(), whose error handler breaks out of the server loop.

The listening socket stays open, so Bitcoin Core’s reconnect succeeds at the TCP level. Nobody accepts and reads it, though, so the test waits forever for another PROTOCOLINFO command.

The fix uses:
mock_tor.conn.shutdown(socket.SHUT_WR)
This shuts down only sending. Bitcoin Core still receives the failure reply followed by EOF. The mock can continue receiving until Bitcoin Core closes its side, at which point recv() returns empty data normally. The handler finishes, the server accepts the reconnect, and it records the new PROTOCOLINFO command.

...which it also claims is cross-platform, though I didn't verify any of this myself.

fjahr commented at 4:25 PM on September 15, 2026:

Thanks, I got a similar result when looking into it but didn't notice that option, I dealt with the OSError like it was an EOF in my first push but this seems more elegant, so I have taken this instead.

fjahr force-pushed on Sep 15, 2026

DrahtBot removed the label CI failed on Sep 15, 2026

willcl-ark approved

willcl-ark commented at 7:34 AM on September 16, 2026: member

reACK 4556ef62675400a4702d73c0a3e1dd4c7061a077

sedited added the label Needs Backport (32.x) on Sep 16, 2026

sedited added this to the milestone 32.0 on Sep 16, 2026

winterrdog commented at 7:57 AM on September 16, 2026: contributor

tACK 4556ef62675400a4702d73c0a3e1dd4c7061a077

makes sense to have all disconnects go through disconnected_cb() so that they consistently get the same reconnect backoff logic

fanquake commented at 10:01 AM on September 16, 2026: member

Concept ACK

sedited approved

sedited commented at 10:51 AM on September 16, 2026: contributor

ACK 4556ef62675400a4702d73c0a3e1dd4c7061a077

DrahtBot requested review from fanquake on Sep 16, 2026

sedited merged this on Sep 16, 2026

sedited closed this on Sep 16, 2026

fanquake referenced this in commit deac114123 on Sep 16, 2026

fanquake removed the label Needs Backport (32.x) on Sep 16, 2026

fanquake commented at 11:04 AM on September 16, 2026: member

Backported to 32.x in #36267.