net: Only use TorControl connection as proxy if proxy is not set yet #25688

pull fjahr wants to merge 1 commits into bitcoin:master from fjahr:2022-07-torcontrol changing 1 files +12 −5

fjahr commented at 11:04 am on July 24, 2022: contributor

Should close #25265

The onion proxy can be set either via -onion or via -proxy (grep init.cpp for onion_proxy). Additionally, when TorControl is configured and successfully connected, there is a fallback that tries to setup the onion proxy via that connection. This fallback was only checking if -onion was set but if -proxy was set it was still running and overwriting the existing onion proxy configured in -proxy.

Instead of using gArgs I am checking if a valid proxy is set already. This should not make a difference in behavior since init error are thrown if -onion or -proxy are set but not valid. There is also another check if -onion or -proxy are explicitly disabled. I think this is the most reasonable behavior for such corner cases, though I am not sure if there are valid use cases for such a configuration.

This also adds some more explicit logging that should help users better understand what is happening.
net: Only use TorControl as proxy if proxy is not set 5624a45d8c
fjahr commented at 11:05 am on July 24, 2022: contributor

@kroese could you check if this solves your problem from #25688?
kroese commented at 11:10 am on July 24, 2022: none

Yes, this fixes it!
ghost commented at 11:38 am on July 24, 2022: none

Concept ACK
DrahtBot added the label P2P on Jul 24, 2022
luke-jr commented at 3:03 am on July 26, 2022: member

Concept NACK. -torcontrol should override -proxy. (Plus, using GetProxy for the check will fail to update it when/if it changes during runtime.)
fjahr commented at 7:31 pm on July 26, 2022: contributor

Concept NACK. -torcontrol should override -proxy. (Plus, using GetProxy for the check will fail to update it when/if it changes during runtime.)

For anyone interested in following, I answered here. I would say let’s keep the conceptual discussion about parameter interaction in the issue @luke-jr, so we don’t have to argue the same things in two places.

in src/torcontrol.cpp:452 in 5624a45d8c

451+        if (GetProxy(NET_ONION, maybe_proxy)) {
452+            LogPrint(BCLog::TOR, "Onion proxy already configured for %s\n", maybe_proxy.proxy.ToStringIPPort());
453+        } else {
454+            if (gArgs.GetArg("-onion", "") != "0" && gArgs.GetArg("-proxy", "") != "0") {
455+                // Don't do this if -onion or -proxy are explicitly disabled
456+                LogPrint(BCLog::TOR, "Onion proxy not configured yet. Trying to use TorControl connection to set proxy.\n");

vasild commented at 5:27 am on August 18, 2022:

nit: “to set proxy” sounds like it will set the proxy for all, ie -proxy. Maybe “… to set the onion proxy” or “… to set it” would be better?

in src/torcontrol.cpp:450 in 5624a45d8c

449+        // if -onion or -proxy has not set it to something else yet.
450+        Proxy maybe_proxy;
451+        if (GetProxy(NET_ONION, maybe_proxy)) {
452+            LogPrint(BCLog::TOR, "Onion proxy already configured for %s\n", maybe_proxy.proxy.ToStringIPPort());
453+        } else {
454+            if (gArgs.GetArg("-onion", "") != "0" && gArgs.GetArg("-proxy", "") != "0") {

vasild commented at 5:27 am on August 18, 2022:

nit: can use } else if (...) { which would reduce the indentation of the block below

fjahr commented at 5:04 pm on August 28, 2022: contributor

Closing this, see discussion in #25265.
fjahr closed this on Aug 28, 2022
bitcoin locked this on Aug 28, 2023

Contributors
fjahr kroese ghost luke-jr vasild

Labels
P2P