Customize onion port & disable bind if service not started #26862

andyoknen commented at 10:46 AM on January 10, 2023: none

The problem is caused by the inability to run multiple nodes on the same PC due to a conflict in the listening port for Tor connections.

Here also people faced a similar problem: #22668

My idea is to prevent the array of ports for the onion process from filling up before running ConnMan - thus, the listenonion=0 option will not only prevent the Tor service from starting, but will also not occupy the port.

Additionally, I added port initialization for onion to keep the ability to run multiple nodes with Tor functionality.

Added argument -onionport for customize Tor connections port & disable bind onion port if service not started 652e388f25

DrahtBot commented at 10:46 AM on January 10, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	vasild

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26177 (refactor / kernel: Move non-gArgs chainparams functionality to kernel by TheCharlatan)
#22729 (Make it possible to disable Tor binds and abort startup on bind failure by vasild)

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.

Update torcontrol.cpp 99612afb7a

Update torcontrol.cpp 934034299a

Update torcontrol.cpp a6f6f38619

in src/init.cpp:1746 in 99612afb7a outdated

1741 | @@ -1741,7 +1742,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1742 |              }
1743 |          } else {
1744 |              const std::string network_type = bind_arg.substr(index + 1);
1745 | -            if (network_type == "onion") {
1746 | +            // Do not fill in connOptions.onion_binds if you are not running the Tor service
1747 | +            if (args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION) && network_type == "onion") {

vasild commented at 11:05 AM on January 10, 2023:

This would ignore bind=...=onion if listenonion=0. That will break the static Tor configurations where the hidden service is configured in the Tor daemon and bitcoind is running with -bind=...=onion -listenonion=0.

See ## 3. Manually create a Bitcoin Core onion service in doc/tor.md.

in src/init.cpp:475 in 99612afb7a outdated

 471 | @@ -472,6 +472,7 @@ void SetupServerArgs(ArgsManager& argsman)
 472 |      argsman.AddArg("-maxtimeadjustment", strprintf("Maximum allowed median peer time offset adjustment. Local perspective of time may be influenced by outbound peers forward or backward by this amount (default: %u seconds).", DEFAULT_MAX_TIME_ADJUSTMENT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 473 |      argsman.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target per 24h. Limit does not apply to peers with 'download' permission or blocks created within past week. 0 = no limit (default: %s). Optional suffix units [k|K|m|M|g|G|t|T] (default: M). Lowercase is 1000 base while uppercase is 1024 base", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 474 |      argsman.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 475 | +    argsman.AddArg("-onionport=<port>", strprintf("Use this port for Tor connections (default: %u, testnet: %u, signet: %u, regtest: %u)", defaultBaseParams->OnionServiceTargetPort(), testnetBaseParams->OnionServiceTargetPort(), signetBaseParams->OnionServiceTargetPort(), regtestBaseParams->OnionServiceTargetPort()), ArgsManager::ALLOW_ANY, OptionsCategory::RPC);

vasild commented at 11:08 AM on January 10, 2023:

This option conflicts with -bind=addr:port=onion because both provide a way to specify the port. Better have one way to specify things.

vasild changes_requested

vasild commented at 11:21 AM on January 10, 2023: contributor

Concept ACK

Thanks for looking into this. Also related: #22726 #22727 #22729

I think a better approach is described in: #22729 (comment) or, in other words:

If bind=...=onion is explicitly specified by the user, then bind on it, same behavior as of now
If bind=...onion is not explicitly specified by the user, then change the behavior to:
- not bind at startup by CConnman::Start() / CConnman::InitBinds() to a Tor port
- if listenonion=1 this means that the Tor control thread will be started and it will try to connect to torcontrol and create the hidden service, telling it to send connections to e.g. 127.0.0.1:8334. Bind on that only then, from the tor control thread, either immediately before or immediately after creating the hidden service.

andyoknen commented at 8:06 AM on January 11, 2023: none

Thanks for your review!

In general, I understood how to avoid the problem of duplicating the port bind in tests through bind=address:port=onion by increasing the port for each subsequent node.

In this case, it remains only to exclude the forced onion binding of the port.. I think one simple argument is missing, something like usetor=1|0 - which would include or exclude all functionality. Although I understand that this is a major change, given the reach of users.

I think I'll close this PR, since I don't have a solution yet)

andyoknen closed this on Jan 11, 2023

andyoknen deleted the branch on Jan 11, 2023

vasild commented at 12:45 PM on January 14, 2023: contributor

it remains only to exclude the forced onion binding of the port

What is that "forced onion binding"?

andyoknen commented at 5:29 AM on January 18, 2023: none

it remains only to exclude the forced onion binding of the port

What is that "forced onion binding"?

I mean that port binding happens anyway whether I want to use TOR or not

bitcoin locked this on Jan 18, 2024