fix(net): don't refuse to start over the automatic onion service unless -listenonion=1

doc/release-notes-7300.md

@@ -1,16 +1,28 @@
 P2P and network changes
 -----------------------
 
-* Startup now fails if any configured `-bind`, `-whitebind` or the implicit Tor
-  onion-service bind (`127.0.0.1:9996` by default) cannot be set up. Dash Core
-  v23 and earlier started as long as at least one bind succeeded.
+* Startup now fails if any configured `-bind` (including `-bind=...=onion`) or
+  `-whitebind` cannot be set up. Dash Core v23 and earlier started as long as
+  at least one bind succeeded. The implicit Tor onion-service bind
+  (`127.0.0.1:9996` by default) is still best effort, as in v23, unless
+  `-listenonion=1` is set explicitly.
 
-* Nodes configured with `-bind` but no specific `-bind=<addr:port>=onion` now
-  refuse to start when `-listenonion` is enabled. This includes nodes without
-  Tor configured, since `-listenonion` is enabled by default when listening.
-  Shared binds cannot distinguish Tor-forwarded connections from direct
-  connections, which can grant Tor peers unintended IP-based whitelist
-  permissions. Nodes without `-bind`, including those using only `-whitebind`,
-  continue to get the default onion target. Users should add a specific
-  `-bind=<addr:port>=onion` to accept incoming Tor connections, or set
-  `-listenonion=0` to disable automatic onion service creation.
+* The automatic Tor onion service (`-listenonion`) only forwards to a
+  dedicated, non-wildcard onion listener that the node actually bound, so peers
+  arriving through Tor are always recognized as onion peers. Without a
+  `-bind=<addr:port>=onion`, Dash Core adds the default onion bind as v23 did,
+  also when `-bind` is given; with `-bind` and `-listenonion=0` it no longer
+  does. If the default onion bind fails, for example because another instance
+  on the same host holds it, the node logs a warning and creates no onion
+  service rather than pointing Tor at whatever holds that port. It still
+  starts and still uses Tor for outbound onion connections if its control port
+  is reachable; the debug log notes that no onion service was created. With an
+  explicit `-listenonion=1`, a failed default onion bind makes startup fail. To
+  run onion services for several instances on one host, give each its own
+  `-bind=127.0.0.1:<port>=onion`.
+
+* A wildcard onion bind such as `-bind=0.0.0.0:<port>=onion` is a startup error
+  unless `-listenonion=0` is set, also with the default `-listenonion`. The node
+  can't tell which connections to a wildcard listener came through Tor, so Tor
+  peers would be treated as clearnet peers. Dash Core v23 accepted it. Use a
+  specific address such as `-bind=127.0.0.1:<port>=onion` instead.

doc/tor.md

@@ -83,8 +83,17 @@ it requires a Tor connection to work. It can be explicitly disabled with
 `-listenonion=0`. If it is not disabled, it can be configured using the
 `-torcontrol` and `-torpassword` settings.
 
-When enabled with an explicit `-bind`, the automatic service requires a non-wildcard onion bind such as `-bind=127.0.0.1:<port>=onion`.
-Without `-bind`, Dash Core adds the default onion bind.
+The automatic service forwards incoming connections to a dedicated,
+non-wildcard onion bind. Unless one is given with `-bind=<addr>:<port>=onion`,
+Dash Core adds the default onion bind (`127.0.0.1:9996` on mainnet). If that
+port is taken, for example by another instance on the same host, no onion
+service is created; give each instance its own `-bind=127.0.0.1:<port>=onion`.
+With `-bind` and `-listenonion=0`, the default onion bind is not added. If the
+default onion bind fails, Dash Core still uses Tor for outbound onion
+connections; only an explicit `-listenonion=1` then stops startup. A wildcard
+onion bind such as `-bind=0.0.0.0:<port>=onion` is a startup error unless
+`-listenonion=0` is set, because Dash Core can't recognize the Tor peers
+connecting to it.
 
 To see verbose Tor information in the dashd debug log, pass `-debug=tor`.
 

src/init.cpp

@@ -664,7 +664,7 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-allowprivatenet", strprintf("Allow RFC1918 addresses to be relayed and connected to (default: %u)", DEFAULT_ALLOWPRIVATENET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-bantime=<n>", strprintf("Default duration (in seconds) of manually configured bans (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-bind=<addr>[:<port>][=onion]", strprintf("Bind to given address and always listen on it (default: 0.0.0.0). Use [host]:port notation for IPv6. Append =onion to tag any incoming connections to that address and port as incoming Tor connections (default: 127.0.0.1:%u=onion, testnet: 127.0.0.1:%u=onion, devnet: 127.0.0.1:%u=onion, regtest: 127.0.0.1:%u=onion). "
-                                                      "The default onion bind is added only when no -bind is specified. When -listenonion is enabled and -bind is specified, a non-wildcard =onion bind is required, and wildcard =onion binds are rejected.", defaultBaseParams->OnionServiceTargetPort(), testnetBaseParams->OnionServiceTargetPort(), devnetBaseParams->OnionServiceTargetPort(), regtestBaseParams->OnionServiceTargetPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
+                                                      "Without an =onion bind, the default onion bind is added unless -bind is given and -listenonion is disabled; failing to bind it stops startup only with an explicit -listenonion=1, and otherwise no onion service is created. While -listenonion is enabled, a wildcard =onion bind is an error.", defaultBaseParams->OnionServiceTargetPort(), testnetBaseParams->OnionServiceTargetPort(), devnetBaseParams->OnionServiceTargetPort(), regtestBaseParams->OnionServiceTargetPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
     argsman.AddArg("-cjdnsreachable", "If set, then this host is configured for CJDNS (connecting to fc00::/8 addresses would lead us to the CJDNS network, see doc/cjdns.md) (default: 0)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-connect=<ip>", "Connect only to the specified node; -noconnect disables automatic connections (the rules for this peer are the same as for -addnode). This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
     argsman.AddArg("-discover", "Discover own IP addresses (default: 1 when listening and no -externalip or -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
@@ -674,7 +674,7 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-fixedseeds", strprintf("Allow fixed seeds if DNS seeds don't provide peers (default: %u)", DEFAULT_FIXEDSEEDS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-forcednsseed", strprintf("Always query for peer addresses via DNS lookup (default: %u)", DEFAULT_FORCEDNSSEED), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-listen", strprintf("Accept connections from outside (default: %u if no -proxy, -connect or -maxconnections=0)", DEFAULT_LISTEN), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
-    argsman.AddArg("-listenonion", strprintf("Automatically create Tor onion service (default: %d)", DEFAULT_LISTEN_ONION), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+    argsman.AddArg("-listenonion", strprintf("Automatically create Tor onion service if Tor is available and the onion bind could be bound (see -bind). If set to 1 explicitly, fail to start if the onion bind cannot be bound (default: %d)", DEFAULT_LISTEN_ONION), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-maxconnections=<n>", strprintf("Maintain at most <n> connections to peers (temporary service connections excluded) (default: %u). This limit does not apply to connections manually added via -addnode or the addnode RPC, which have a separate limit of %u.", DEFAULT_MAX_PEER_CONNECTIONS, MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-maxreceivebuffer=<n>", strprintf("Maximum per-connection receive buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXRECEIVEBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-maxsendbuffer=<n>", strprintf("Maximum per-connection memory usage for the send buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXSENDBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
@@ -2701,24 +2701,25 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         }
     }
 
-    if (connOptions.onion_binds.empty() && connOptions.vBinds.empty()) {
+    const bool listen_onion{args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)};
+    const bool listen_onion_explicit{listen_onion && args.IsArgSet("-listenonion")};
+    if (connOptions.onion_binds.empty() && (connOptions.vBinds.empty() || listen_onion)) {
         connOptions.onion_binds.push_back(DefaultOnionServiceTarget());
+        connOptions.onion_bind_optional = !listen_onion_explicit;
     }
 
-    if (args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) {
-        if (connOptions.onion_binds.empty()) {
-            return InitError(_("The automatic Tor onion service requires a dedicated onion bind. Use a specific address such as -bind=127.0.0.1:<port>=onion, or disable the service with -listenonion=0."));
-        }
+    std::optional<CService> onion_service_target;
+    if (listen_onion) {
+        // A wildcard listener can't tag the peers Tor forwards to it as onion.
         if (std::ranges::any_of(connOptions.onion_binds, [](auto& b) { return b.IsBindAny(); })) {
             return InitError(_("The automatic Tor onion service cannot use a wildcard onion bind because the Tor daemon wouldn't be able to forward incoming connections to us. Use a specific address such as -bind=127.0.0.1:<port>=onion, or disable the service with -listenonion=0."));
         }
-        const CService& onion_service_target{connOptions.onion_binds[0]};
+        onion_service_target = connOptions.onion_binds.front();
         if (connOptions.onion_binds.size() > 1) {
             InitWarning(strprintf(_("More than one onion bind address is provided. Using %s "
                                     "for the automatically created Tor onion service."),
-                                  onion_service_target.ToStringAddrPort()));
+                                  onion_service_target->ToStringAddrPort()));
         }
-        StartTorControl(onion_service_target);
     }
 
     if (connOptions.bind_on_any) {
@@ -2780,6 +2781,14 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         return false;
     }
 
+    if (listen_onion) {
+        // The optional default onion bind may have failed to bind.
+        if (onion_service_target && !node.connman->HasOnionBind(*onion_service_target)) {
+            onion_service_target.reset();
+        }
+        StartTorControl(onion_service_target);
+    }
+
     // ********************************************************* Step 13: finished
 
     // At this point, the RPC is "started", but still in warmup, which means it

src/net.cpp

@@ -3986,7 +3986,12 @@ bool CConnman::InitBinds(const Options& options)
         }
     }
     for (const auto& addr_bind : options.onion_binds) {
-        if (!Bind(addr_bind, BF_REPORT_ERROR | BF_DONT_ADVERTISE, NetPermissionFlags::None)) {
+        if (options.onion_bind_optional) {
+            if (!Bind(addr_bind, BF_DONT_ADVERTISE, NetPermissionFlags::None)) {
+                LogWarning("Unable to bind the default onion service target %s, continuing without it\n", addr_bind.ToStringAddrPort());
+                std::erase(m_onion_binds, addr_bind);
+            }
+        } else if (!Bind(addr_bind, BF_REPORT_ERROR | BF_DONT_ADVERTISE, NetPermissionFlags::None)) {
             return false;
         }
     }

src/net.h

@@ -35,6 +35,7 @@
 #include <util/threadinterrupt.h>
 #include <util/wpipe.h>
 
+#include <algorithm>
 #include <atomic>
 #include <condition_variable>
 #include <cstdint>
@@ -1238,6 +1239,9 @@ friend class CNode;
         std::vector<NetWhitebindPermissions> vWhiteBinds;
         std::vector<CService> vBinds;
         std::vector<CService> onion_binds;
+        /// True if onion_binds holds only the default onion bind and failing to bind it should not
+        /// stop startup.
+        bool onion_bind_optional{false};
         /// True if the user did not specify -bind= or -whitebind= and thus
         /// we should bind on `0.0.0.0` (IPv4) and `::` (IPv6).
         bool bind_on_any;
@@ -1315,6 +1319,8 @@ friend class CNode;
     void Interrupt() EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc);
     bool GetNetworkActive() const { return fNetworkActive; };
     bool GetUseAddrmanOutgoing() const { return m_use_addrman_outgoing; };
+    /** Whether we listen on addr and tag the peers connecting there as onion. */
+    bool HasOnionBind(const CService& addr) const { return std::ranges::find(m_onion_binds, addr) != m_onion_binds.end(); }
     void SetNetworkActive(bool active, CMasternodeSync* const mn_sync);
     bool GetMasternodeThreadActive() const { return m_masternode_thread_active; };
     void SetMasternodeThreadActive(bool active) { m_masternode_thread_active = active; };

src/torcontrol.cpp

@@ -312,7 +312,7 @@ std::map<std::string,std::string> ParseTorReplyMapping(const std::string &s)
     return mapping;
 }
 
-TorController::TorController(struct event_base* _base, const std::string& tor_control_center, const CService& target):
+TorController::TorController(struct event_base* _base, const std::string& tor_control_center, const std::optional<CService>& target):
     base(_base),
     m_tor_control_center(tor_control_center), conn(base), reconnect(true), reconnect_timeout(RECONNECT_TIMEOUT_START),
     m_target(target)
@@ -457,13 +457,21 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
             _conn.Command("GETINFO net/listeners/socks", std::bind(&TorController::get_socks_cb, this, std::placeholders::_1, std::placeholders::_2));
         }
 
+        if (!m_target) {
+            if (!m_logged_no_target) {
+                LogPrintf("tor: Not creating an onion service because there is no dedicated onion bind. Use -bind=127.0.0.1:<port>=onion to create one.\n");
+                m_logged_no_target = true;
+            }
+            return;
+        }
+
         // Finally - now create the service
         if (private_key.empty()) { // No private key, generate one
             private_key = "NEW:ED25519-V3"; // Explicitly request key type - see issue #9214
         }
         // Request onion service, redirect port.
         // Note that the 'virtual' port is always the default port to avoid decloaking nodes using other ports.
-        _conn.Command(strprintf("ADD_ONION %s Port=%i,%s", private_key, Params().GetDefaultPort(), m_target.ToStringAddrPort()),
+        _conn.Command(strprintf("ADD_ONION %s Port=%i,%s", private_key, Params().GetDefaultPort(), m_target->ToStringAddrPort()),
             std::bind(&TorController::add_onion_cb, this, std::placeholders::_1, std::placeholders::_2));
     } else {
         LogPrintf("tor: Authentication failed\n");
@@ -660,14 +668,14 @@ void TorController::reconnect_cb(evutil_socket_t fd, short what, void *arg)
 static struct event_base *gBase;
 static std::thread torControlThread;
 
-static void TorControlThread(CService onion_service_target)
+static void TorControlThread(std::optional<CService> onion_service_target)
 {
     TorController ctrl(gBase, gArgs.GetArg("-torcontrol", DEFAULT_TOR_CONTROL), onion_service_target);
 
     event_base_dispatch(gBase);
 }
 
-void StartTorControl(CService onion_service_target)
+void StartTorControl(std::optional<CService> onion_service_target)
 {
     assert(!gBase);
 #ifdef WIN32

src/torcontrol.h

@@ -16,14 +16,15 @@
 #include <cstdint>
 #include <deque>
 #include <functional>
+#include <optional>
 #include <string>
 #include <vector>
 
 constexpr int DEFAULT_TOR_CONTROL_PORT = 9051;
 extern const std::string DEFAULT_TOR_CONTROL;
 static const bool DEFAULT_LISTEN_ONION = true;
 
-void StartTorControl(CService onion_service_target);
+void StartTorControl(std::optional<CService> onion_service_target);
 void InterruptTorControl();
 void StopTorControl();
 
@@ -101,12 +102,13 @@ class TorControlConnection
 /****** Bitcoin specific TorController implementation ********/
 
 /** Controller that connects to Tor control socket, authenticate, then create
- * and maintain an ephemeral onion service.
+ * and maintain an ephemeral onion service. Without a target it only configures
+ * the onion proxy and creates no onion service.
  */
 class TorController
 {
 public:
-    TorController(struct event_base* base, const std::string& tor_control_center, const CService& target);
+    TorController(struct event_base* base, const std::string& tor_control_center, const std::optional<CService>& target);
     TorController() : conn{nullptr} {
         // Used for testing only.
     }
@@ -127,7 +129,8 @@ class TorController
     struct event *reconnect_ev = nullptr;
     float reconnect_timeout;
     CService service;
-    const CService m_target;
+    const std::optional<CService> m_target;
+    bool m_logged_no_target{false};
     /** Cookie for SAFECOOKIE auth */
     std::vector<uint8_t> cookie;
     /** ClientNonce for SAFECOOKIE auth */