net: Warn when Tor onion service lacks a dedicated onion bind #34892

HouseOfHufflepuff commented at 12:35 AM on March 22, 2026: contributor

Summary

When -bind is specified without a dedicated onion socket (-bind=<addr>=onion) but -listenonion is enabled, the auto-created Tor hidden service routes incoming connections to the regular bind address. This makes it impossible to distinguish incoming Tor connections from regular IPv4 connections, causing -netinfo to report 0 onion connections even when they exist.

This adds a startup warning to alert users so they can add -bind=127.0.0.1:8334=onion for accurate connection tracking.

Fixes #33458

Test plan

Start bitcoind with -bind but without a dedicated onion bind:

./build/bin/bitcoind -bind=0.0.0.0:8333 -listenonion=1

Before this change: No warning is emitted. Users have no indication that incoming Tor connections will be misidentified.

After this change: A warning is printed at startup:

Warning: The Tor onion service is being directed to a -bind address without a dedicated onion socket (-bind=<addr>=onion). Incoming Tor connections will not be identified as onion connections.

The warning does not appear when a dedicated onion bind is provided:

./build/bin/bitcoind -bind=0.0.0.0:8333 -bind=127.0.0.1:8334=onion -listenonion=1

Functional test feature_proxy.py updated to expect the new warning.

DrahtBot added the label P2P on Mar 22, 2026

DrahtBot commented at 12:36 AM on March 22, 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/34892.

Reviews

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

Type	Reviewers
ACK	winterrdog

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:

#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.

HouseOfHufflepuff commented at 12:36 AM on March 22, 2026: contributor

@ArmchairCryptologist

DrahtBot added the label CI failed on Mar 23, 2026

kevkevinpal commented at 8:40 PM on March 23, 2026: contributor

Looks like the functional test feature_proxy.py is failing

You can fix it by modifying this bit of code to looks like this

self.log.info("Test passing -onlynet=onion without -proxy or -onion but with -listenonion=1 is ok")
self.start_node(1, extra_args=["-onlynet=onion", "-listenonion=1"])
- self.stop_node(1)
+ self.stop_node(1, expected_stderr="Warning: The Tor onion service is being directed to a -bind address without a dedicated on  ion socket (-bind=<addr>=onion). Incoming Tor connections will not be identified as onion connections.")

HouseOfHufflepuff commented at 9:33 PM on March 23, 2026: contributor

Thanks @kevkevinpal! Good catch — applied the fix to feature_proxy.py to pass expected_stderr with the new warning message on stop_node. Test passes locally now. Ready for re-review.

maflcko commented at 5:52 AM on March 24, 2026: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

Also, the test plan is clearly wrong.

Was this LLM generated? What are the steps to test this? What is the output before and after the changes here?

HouseOfHufflepuff commented at 2:40 PM on March 24, 2026: contributor

Sorry about that @maflcko — you're right, the test plan was inadequate and there were unrelated changes that shouldn't have been in the branch. I've squashed into a single commit, removed the unrelated changes, and updated the PR description with actual before/after output. Should be ready for another look when you have time.

HouseOfHufflepuff force-pushed on Mar 24, 2026

kevkevinpal commented at 3:49 PM on March 24, 2026: contributor

I think it would be good to add a test asserting that this warning gets logged. In addition to modifying the existing test that was failing.

maflcko commented at 6:22 AM on March 25, 2026: member

You'll have to run the tests locally before pushing to CI

HouseOfHufflepuff force-pushed on Mar 25, 2026

DrahtBot removed the label CI failed on Mar 25, 2026

HouseOfHufflepuff commented at 9:13 PM on March 25, 2026: contributor

@maflcko Apologies for the messy state earlier — commits are squashed, unrelated changes removed, and tests verified locally before pushing. Lesson learned on the workflow. Ready for re-review whenever you have time.

HouseOfHufflepuff commented at 9:15 PM on March 25, 2026: contributor

@kevkevinpal Thanks for the proxy test catch and the suggestion to add a warning assertion — both are addressed now. CI is passing clean. Would appreciate another look when you have a moment.

naiyoma commented at 11:15 AM on March 27, 2026: contributor

I’m not sure about this approach. I think the right direction would be to try to get the count right. I’ve explained what I think the two options might be -> #33458 (comment)

naiyoma commented at 10:56 AM on March 30, 2026: contributor

I’m not sure about this approach. I think the right direction would be to try to get the count right. I’ve explained what I think the two options might be -> #33458 (comment)

I was wrong about the alternative approaches I suggested. I don't think we can get the correct count. This approach is fine.

in test/functional/feature_proxy.py:439 in 2e63783ae1 outdated

 435 | @@ -436,6 +436,17 @@ def networks_dict(d):
 436 |  
 437 |          self.log.info("Test passing -onlynet=onion without -proxy or -onion but with -listenonion=1 is ok")
 438 |          self.start_node(1, extra_args=["-onlynet=onion", "-listenonion=1"])
 439 | +        self.stop_node(1, expected_stderr="Warning: The Tor onion service is being directed to a -bind address without a dedicated onion socket (-bind=<addr>=onion). Incoming Tor connections will not be identified as onion connections.")

naiyoma commented at 11:15 AM on March 30, 2026:

Nit: IMO this test would be better placed in feature_bind_extra.py rather than in this file. feature_bind_extra.py also sets bind_to_localhost_only = False, meaning we can explicitly control -bind configuration and not rely on the default. I initially found it confusing why the test was passing, and then I realized that by default, bind_to_localhost_only is true.

HouseOfHufflepuff commented at 1:56 AM on May 2, 2026:

Done, moved and using explicit bind.

naiyoma commented at 11:28 AM on March 30, 2026: contributor

and when using -netinfo it should show a "?" or "-" instead of "0".

This is a good suggestion from the issue have you considered implementing it?

HouseOfHufflepuff force-pushed on Mar 30, 2026

DrahtBot added the label CI failed on Mar 30, 2026

DrahtBot removed the label CI failed on Mar 30, 2026

HouseOfHufflepuff commented at 5:23 PM on March 30, 2026: contributor

Thanks for the review @naiyoma!

Moved the warning tests from feature_proxy.py to feature_bind_extra.py where bind_to_localhost_only = False gives us explicit control over -bind configuration. The tests now pass explicit -bind args so it's clear why the warning fires (or doesn't). The -onlynet=onion -listenonion=1 test stays in feature_proxy.py since it's testing the onlynet behavior, and just handles the warning as expected stderr.

Re: showing "?" or "-" instead of "0" in -netinfo — that's a good idea from the original issue, but I think it's better suited as a follow-up PR since it touches different code (-netinfo output formatting in bitcoin-cli). Happy to open an issue and work if you would like. Happy to do here as well but wanted to check.

HouseOfHufflepuff requested review from naiyoma on Mar 30, 2026

in src/init.cpp:2185 in f23b35aea1

2181 | @@ -2182,6 +2182,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
2182 |      }
2183 |  
2184 |      if (listenonion) {
2185 | +        if (connOptions.onion_binds.empty() && !connOptions.vBinds.empty()) {

winterrdog commented at 5:52 PM on April 27, 2026:

for more accurate Tor traffic detection, I think we also need to check connOptions.bind_on_any.

when a user runs bitcoind -listenonion=1 without an explicit -bind, Bitcoin Core automatically binds to 0.0.0.0 or :: (as seen here in init.cpp). in these cases, connOptions.vBinds will be empty, but the node is still listening.

suggestion:

        if (connOptions.onion_binds.empty() && (!connOptions.vBinds.empty() || connOptions.bind_on_any)) {

what do you think ?

HouseOfHufflepuff commented at 1:56 AM on May 2, 2026:

Good catch, added bind_on_any check.

DrahtBot added the label CI failed on Apr 27, 2026

HouseOfHufflepuff force-pushed on Apr 28, 2026

HouseOfHufflepuff commented at 4:33 AM on April 28, 2026: contributor

Windows CI failures are runner infrastructure timeouts, not code issues. Could someone re-run those jobs?

winterrdog commented at 2:09 PM on April 28, 2026: contributor

tested ACK 716812baa6c073096a23cc10446d1964f469796c

code compiles fine, and all pertinent tests pass successfully (tested on Linux x86_64)

winterrdog commented at 10:22 PM on April 30, 2026: contributor

Could someone re-run those jobs?

perhaps the day you pushed these changes, github had run into reliability issues thus returning a timeout. so i think a small hack you can use to re-trigger the CI is to amend the last commit:

git commit --amend --no-edit
git push --force-with-lease

it won’t add extra commits, so no need to squash later. if it fails twice in the same way, stop retrying and treat it as a real issue in the test or environment.

HouseOfHufflepuff force-pushed on Apr 30, 2026

DrahtBot removed the label CI failed on Apr 30, 2026

HouseOfHufflepuff commented at 11:21 PM on April 30, 2026: contributor

Could someone re-run those jobs?

perhaps the day you pushed these changes, github had run into reliability issues thus returning a timeout. so i think a small hack you can use to re-trigger the CI is to amend the last commit:
git commit --amend --no-edit
git push --force-with-lease
it won’t add extra commits, so no need to squash later. if it fails twice in the same way, stop retrying and treat it as a real issue in the test or environment.

Thank you. I re-ran and CI is green.

HouseOfHufflepuff requested review from winterrdog on Apr 30, 2026

in test/functional/feature_torcontrol.py:111 in 7025bf39cf

 107 | @@ -107,6 +108,7 @@ def _get_response(self, command):
 108 |  class TorControlTest(BitcoinTestFramework):
 109 |      def set_test_params(self):
 110 |          self.num_nodes = 1
 111 | +        self.extra_args = [[f"-bind=127.0.0.1:{tor_port(0)}=onion"]]

winterrdog commented at 12:15 AM on May 2, 2026:

i think this extra_args property can be removed in favour of the one on line 123. this is because it has no clear effect on the tests (actually they pass perfectly, even without it)

HouseOfHufflepuff commented at 1:56 AM on May 2, 2026:

Removed, you're right it's redundant.

in test/functional/feature_proxy.py:438 in 7025bf39cf

 435 | @@ -436,7 +436,7 @@ def networks_dict(d):
 436 |  
 437 |          self.log.info("Test passing -onlynet=onion without -proxy or -onion but with -listenonion=1 is ok")
 438 |          self.start_node(1, extra_args=["-onlynet=onion", "-listenonion=1"])

winterrdog commented at 12:29 AM on May 2, 2026:

i think we're not very interested in testing for the warning's presence here as much as we do in feature_bind_extra.py. for that reason, we can pass an explicit -bind here in order to avoid triggering the warning's condition introduced by the PR's init.cpp

        self.start_node(1, extra_args=["-onlynet=onion", "-listenonion=1", f"-bind=127.0.0.1:{tor_port(0)}=onion"])

i think this will fulfill @naiyoma's nit and also prevents tight coupling in the tests.

HouseOfHufflepuff commented at 1:56 AM on May 2, 2026:

Fixed, passing explicit bind now.

in test/functional/p2p_private_broadcast.py:539 in 7025bf39cf

 533 | @@ -534,6 +534,10 @@ def set_tx_returner_and_other():
 534 |          assert_raises_rpc_error(-1, "none of the Tor or I2P networks is reachable",
 535 |                                  tx_originator.sendrawtransaction, hexstring=txs[0]["hex"], maxfeerate=0.1)
 536 |  
 537 | +        # Restart node 0 with original args so that cleanup does not fail due to
 538 | +        # the warning about missing dedicated onion bind on stderr.
 539 | +        self.restart_node(0, expected_stderr="Warning: The Tor onion service is being directed to a -bind address without a dedicated onion socket (-bind=<addr>=onion). Incoming Tor connections will not be identified as onion connections.")

winterrdog commented at 12:33 AM on May 2, 2026:

even here, it'll be similar to what i suggested here. we can pass an explicit -bind just below line 532.

HouseOfHufflepuff commented at 1:56 AM on May 2, 2026:

Fixed, passing explicit bind now.

winterrdog commented at 12:40 AM on May 2, 2026: contributor

@HouseOfHufflepuff tested and left some review comments for you.

HouseOfHufflepuff force-pushed on May 2, 2026

HouseOfHufflepuff commented at 1:58 AM on May 2, 2026: contributor

@HouseOfHufflepuff tested and left some review comments for you.

Thank you. I have addressed the comments with the suggested implementation.

winterrdog commented at 8:30 PM on May 4, 2026: contributor

Re-ACK cc1bf1da5441f59e78cc90d45cca173436bc873c

sedited commented at 4:21 PM on June 21, 2026: contributor

@naiyoma can you take another look here?

naiyoma commented at 2:45 PM on July 8, 2026: contributor

Sorry, I was AFK. I'll re-review soon

winterrdog commented at 8:36 PM on July 26, 2026: contributor

@HouseOfHufflepuff

i think you will need to edit the commit message and remove Anthropic's Claude as a co-author. a recent AI policy was passed saying that LLMs are just tools, so they should not be credited as co-authors

if there were other contributors, they can be added as co-authors; otherwise, the Co-Authored-By field can just be left out entirely

also, i think the commit message could be trimmed down. right now it repeats the PR description almost line-for-line, and since the merge script folds the full PR description into the merge commit anyway, having both means the same explanation ends up duplicated in permanent history. a shorter commit message focused on the "what/why" would be enough.

sth like (you are completely free to modify it into what you want):

net: Warn when Tor onion service lacks a dedicated onion bind

When -bind is specified without a dedicated -bind=<addr>=onion, the
auto-created Tor hidden service routes incoming connections to the
regular bind address, making it impossible to distinguish incoming
Tor connections from regular ones (-netinfo reports 0 onion
connections even when they exist).

Warn at startup when listenonion is set but no dedicated onion bind
is configured, so users can add -bind=127.0.0.1:8334=onion for
accurate connection tracking.

Update functional tests to cover the new warning and to pass an
explicit onion bind where the warning would otherwise be spurious.

Fixes [#33458](/bitcoin-bitcoin/33458/)

net: Warn when Tor onion service lacks a dedicated onion bind

When -bind is specified without a dedicated -bind=<addr>=onion, the
auto-created Tor hidden service routes incoming connections to the
regular bind address, making it impossible to distinguish incoming
Tor connections from regular ones (-netinfo reports 0 onion
connections even when they exist).

Warn at startup when listenonion is set but no dedicated onion bind
is configured, so users can add -bind=127.0.0.1:8334=onion for
accurate connection tracking.

Update functional tests to cover the new warning and to pass an
explicit onion bind where the warning would otherwise be spurious.

Fixes #33458

4736431f21

HouseOfHufflepuff force-pushed on Jul 27, 2026

HouseOfHufflepuff commented at 2:11 PM on July 27, 2026: contributor

@HouseOfHufflepuff

i think you will need to edit the commit message and remove Anthropic's Claude as a co-author. a recent AI policy was passed saying that LLMs are just tools, so they should not be credited as co-authors

if there were other contributors, they can be added as co-authors; otherwise, the Co-Authored-By field can just be left out entirely

also, i think the commit message could be trimmed down. right now it repeats the PR description almost line-for-line, and since the merge script folds the full PR description into the merge commit anyway, having both means the same explanation ends up duplicated in permanent history. a shorter commit message focused on the "what/why" would be enough.

sth like (you are completely free to modify it into what you want):
net: Warn when Tor onion service lacks a dedicated onion bind

When -bind is specified without a dedicated -bind=<addr>=onion, the
auto-created Tor hidden service routes incoming connections to the
regular bind address, making it impossible to distinguish incoming
Tor connections from regular ones (-netinfo reports 0 onion
connections even when they exist).

Warn at startup when listenonion is set but no dedicated onion bind
is configured, so users can add -bind=127.0.0.1:8334=onion for
accurate connection tracking.

Update functional tests to cover the new warning and to pass an
explicit onion bind where the warning would otherwise be spurious.

Fixes [#33458](/bitcoin-bitcoin/33458/)

I have removed this. Thank you.

HouseOfHufflepuff requested review from winterrdog on Jul 27, 2026

winterrdog commented at 9:35 PM on July 27, 2026: contributor

reACK 4736431f212b9549ab31ec419e06144394b0c536

ZERO code changes were made between this commit and the previous one. only the commit message was edited to cut back on the verbosity thus changing the commit hash as a result