Merge bitcoin/bitcoin#31223: net, init: derive default onion port if …

…a user specified a -port

1dd3af8 Add release note for #31223 (Martin Zumsande)
997757d test: add functional test for -port behavior (Martin Zumsande)
0e2b12b net, init: derive default onion port if a user specified a -port (Martin Zumsande)

Pull request description:

  This resolves #31133 (setups with multiple local nodes each using a different `-port` no longer working with v28.0, see the issue description for more details) by deriving the default onion listening port to be the value specified by `-port` incremented by 1 (idea by vasild / laanwj).
  Note that with this fix, the chosen `-port` values of two local nodes cannot be adjacent, otherwise there will be port collisions again.

  From the discussion in the linked issue, this was the most popular option, followed by doing nothing and telling affected users to change their setups to use `-bind` instead of `-port`. But more opinions are certainly welcome!

  I think that if we decide to do something about the problem described in the issue, we should do so soon (in 28.1.), so I opened this PR.
  Fixes #31133

ACKs for top commit:
  achow101:
    ACK 1dd3af8
  laanwj:
    Tested ACK 1dd3af8
  tdb3:
    Code review ACK 1dd3af8

Tree-SHA512: 37fda2b23bbedcab5df3a401cf5afce66ae5318fb78f9660f83e3fd075b528e8156d7a0903f9a12ffe97ab5d83860587116b74af28670a1f4c2f0d1be4999f40

doc/release-notes-31223.md

@@ -0,0 +1,15 @@
+P2P and network changes
+-----------------------
+When the `-port` configuration option is used, the default onion listening port will now
+be derived to be that port + 1 instead of being set to a fixed value (8334 on mainnet).
+This re-allows setups with multiple local nodes using different `-port` and not using `-bind`,
+which would lead to a startup failure in v28.0 due to a port collision.
+
+Note that a `HiddenServicePort` manually configured in `torrc` may need adjustment if used in
+connection with the `-port` option.
+For example, if you are using `-port=5555` with a non-standard value and not using `-bind=...=onion`,
+previously Bitcoin Core would listen for incoming Tor connections on `127.0.0.1:8334`.
+Now it would listen on `127.0.0.1:5556` (`-port` plus one). If you configured the hidden service manually
+in torrc now you have to change it from `HiddenServicePort 8333 127.0.0.1:8334` to `HiddenServicePort 8333
+127.0.0.1:5556`, or configure bitcoind with `-bind=127.0.0.1:8334=onion` to get the previous behavior.
+(#31223)

src/chainparamsbase.cpp

@@ -41,15 +41,15 @@ std::unique_ptr<CBaseChainParams> CreateBaseChainParams(const ChainType chain)
 {
     switch (chain) {
     case ChainType::MAIN:
-        return std::make_unique<CBaseChainParams>("", 8332, 8334);
+        return std::make_unique<CBaseChainParams>("", 8332);
     case ChainType::TESTNET:
-        return std::make_unique<CBaseChainParams>("testnet3", 18332, 18334);
+        return std::make_unique<CBaseChainParams>("testnet3", 18332);
     case ChainType::TESTNET4:
-        return std::make_unique<CBaseChainParams>("testnet4", 48332, 48334);
+        return std::make_unique<CBaseChainParams>("testnet4", 48332);
     case ChainType::SIGNET:
-        return std::make_unique<CBaseChainParams>("signet", 38332, 38334);
+        return std::make_unique<CBaseChainParams>("signet", 38332);
     case ChainType::REGTEST:
-        return std::make_unique<CBaseChainParams>("regtest", 18443, 18445);
+        return std::make_unique<CBaseChainParams>("regtest", 18443);
     }
     assert(false);
 }

src/chainparamsbase.h

@@ -22,15 +22,13 @@ class CBaseChainParams
 public:
     const std::string& DataDir() const { return strDataDir; }
     uint16_t RPCPort() const { return m_rpc_port; }
-    uint16_t OnionServiceTargetPort() const { return m_onion_service_target_port; }
 
     CBaseChainParams() = delete;
-    CBaseChainParams(const std::string& data_dir, uint16_t rpc_port, uint16_t onion_service_target_port)
-        : m_rpc_port(rpc_port), m_onion_service_target_port(onion_service_target_port), strDataDir(data_dir) {}
+    CBaseChainParams(const std::string& data_dir, uint16_t rpc_port)
+        : m_rpc_port(rpc_port), strDataDir(data_dir) {}
 
 private:
     const uint16_t m_rpc_port;
-    const uint16_t m_onion_service_target_port;
     std::string strDataDir;
 };
 

src/init.cpp

@@ -523,7 +523,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
     argsman.AddArg("-addnode=<ip>", strprintf("Add a node to connect to and attempt to keep the connection open (see the addnode RPC help for more info). This option can be specified multiple times to add multiple nodes; connections are limited to %u at a time and are counted separately from the -maxconnections limit.", MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
     argsman.AddArg("-asmap=<file>", strprintf("Specify asn mapping used for bucketing of the peers (default: %s). Relative paths will be prefixed by the net-specific datadir location.", DEFAULT_ASMAP_FILENAME), 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, testnet3: 127.0.0.1:%u=onion, testnet4: 127.0.0.1:%u=onion, signet: 127.0.0.1:%u=onion, regtest: 127.0.0.1:%u=onion)", defaultBaseParams->OnionServiceTargetPort(), testnetBaseParams->OnionServiceTargetPort(), testnet4BaseParams->OnionServiceTargetPort(), signetBaseParams->OnionServiceTargetPort(), regtestBaseParams->OnionServiceTargetPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, 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, testnet3: 127.0.0.1:%u=onion, testnet4: 127.0.0.1:%u=onion, signet: 127.0.0.1:%u=onion, regtest: 127.0.0.1:%u=onion)", defaultChainParams->GetDefaultPort() + 1, testnetChainParams->GetDefaultPort() + 1, testnet4ChainParams->GetDefaultPort() + 1, signetChainParams->GetDefaultPort() + 1, regtestChainParams->GetDefaultPort() + 1), 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);
@@ -550,7 +550,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
     argsman.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-txreconciliation", strprintf("Enable transaction reconciliations per BIP 330 (default: %d)", DEFAULT_TXRECONCILIATION_ENABLE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
-    argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port> (default: %u, testnet3: %u, testnet4: %u, signet: %u, regtest: %u). Not relevant for I2P (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), testnet4ChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
+    argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port> (default: %u, testnet3: %u, testnet4: %u, signet: %u, regtest: %u). Not relevant for I2P (see doc/i2p.md). If set to a value x, the default onion listening port will be set to x+1.", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), testnet4ChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
 #ifdef HAVE_SOCKADDR_UN
     argsman.AddArg("-proxy=<ip:port|path>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled). May be a local file path prefixed with 'unix:' if the proxy supports it.", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_ELISION, OptionsCategory::CONNECTION);
 #else
@@ -1858,6 +1858,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     const uint16_t default_bind_port =
         static_cast<uint16_t>(args.GetIntArg("-port", Params().GetDefaultPort()));
 
+    const uint16_t default_bind_port_onion = default_bind_port + 1;
+
     const auto BadPortWarning = [](const char* prefix, uint16_t port) {
         return strprintf(_("%s request to listen on port %u. This port is considered \"bad\" and "
                            "thus it is unlikely that any peer will connect to it. See "
@@ -1882,7 +1884,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
             const std::string network_type = bind_arg.substr(index + 1);
             if (network_type == "onion") {
                 const std::string truncated_bind_arg = bind_arg.substr(0, index);
-                bind_addr = Lookup(truncated_bind_arg, BaseParams().OnionServiceTargetPort(), false);
+                bind_addr = Lookup(truncated_bind_arg, default_bind_port_onion, false);
                 if (bind_addr.has_value()) {
                     connOptions.onion_binds.push_back(bind_addr.value());
                     continue;
@@ -1918,7 +1920,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     } else if (!connOptions.vBinds.empty()) {
         onion_service_target = connOptions.vBinds.front();
     } else {
-        onion_service_target = DefaultOnionServiceTarget();
+        onion_service_target = DefaultOnionServiceTarget(default_bind_port_onion);
         connOptions.onion_binds.push_back(onion_service_target);
     }
 

src/torcontrol.cpp

@@ -711,9 +711,9 @@ void StopTorControl()
     }
 }
 
-CService DefaultOnionServiceTarget()
+CService DefaultOnionServiceTarget(uint16_t port)
 {
     struct in_addr onion_service_target;
     onion_service_target.s_addr = htonl(INADDR_LOOPBACK);
-    return {onion_service_target, BaseParams().OnionServiceTargetPort()};
+    return {onion_service_target, port};
 }

src/torcontrol.h

@@ -27,7 +27,7 @@ void StartTorControl(CService onion_service_target);
 void InterruptTorControl();
 void StopTorControl();
 
-CService DefaultOnionServiceTarget();
+CService DefaultOnionServiceTarget(uint16_t port);
 
 /** Reply from Tor, can be single or multi-line */
 class TorControlReply

test/functional/feature_port.py

@@ -0,0 +1,60 @@
+#!/usr/bin/env python3
+# Copyright (c) 2024-present The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""
+Test the -port option and its interactions with
+-bind.
+"""
+
+from test_framework.test_framework import (
+    BitcoinTestFramework,
+)
+from test_framework.util import (
+    p2p_port,
+)
+
+
+class PortTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        # Avoid any -bind= on the command line.
+        self.bind_to_localhost_only = False
+        self.num_nodes = 1
+
+    def run_test(self):
+        node = self.nodes[0]
+        node.has_explicit_bind = True
+        port1 = p2p_port(self.num_nodes)
+        port2 = p2p_port(self.num_nodes + 5)
+
+        self.log.info("When starting with -port, bitcoind binds to it and uses port + 1 for an onion bind")
+        with node.assert_debug_log(expected_msgs=[f'Bound to 0.0.0.0:{port1}', f'Bound to 127.0.0.1:{port1 + 1}']):
+            self.restart_node(0, extra_args=["-listen", f"-port={port1}"])
+
+        self.log.info("When specifying -port multiple times, only the last one is taken")
+        with node.assert_debug_log(expected_msgs=[f'Bound to 0.0.0.0:{port2}', f'Bound to 127.0.0.1:{port2 + 1}'], unexpected_msgs=[f'Bound to 0.0.0.0:{port1}']):
+            self.restart_node(0, extra_args=["-listen", f"-port={port1}", f"-port={port2}"])
+
+        self.log.info("When specifying ports with both -port and -bind, the one from -port is ignored")
+        with node.assert_debug_log(expected_msgs=[f'Bound to 0.0.0.0:{port2}'], unexpected_msgs=[f'Bound to 0.0.0.0:{port1}']):
+            self.restart_node(0, extra_args=["-listen", f"-port={port1}", f"-bind=0.0.0.0:{port2}"])
+
+        self.log.info("When -bind specifies no port, the values from -port and -bind are combined")
+        with self.nodes[0].assert_debug_log(expected_msgs=[f'Bound to 0.0.0.0:{port1}']):
+            self.restart_node(0, extra_args=["-listen", f"-port={port1}", "-bind=0.0.0.0"])
+
+        self.log.info("When an onion bind specifies no port, the value from -port, incremented by 1, is taken")
+        with self.nodes[0].assert_debug_log(expected_msgs=[f'Bound to 127.0.0.1:{port1 + 1}']):
+            self.restart_node(0, extra_args=["-listen", f"-port={port1}", "-bind=127.0.0.1=onion"])
+
+        self.log.info("Invalid values for -port raise errors")
+        self.stop_node(0)
+        node.extra_args = ["-listen", "-port=65536"]
+        node.assert_start_raises_init_error(expected_msg="Error: Invalid port specified in -port: '65536'")
+        node.extra_args = ["-listen", "-port=0"]
+        node.assert_start_raises_init_error(expected_msg="Error: Invalid port specified in -port: '0'")
+
+
+if __name__ == '__main__':
+    PortTest(__file__).main()

test/functional/test_framework/test_node.py

@@ -221,10 +221,9 @@ def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, env=None
             extra_args = self.extra_args
 
         # If listening and no -bind is given, then bitcoind would bind P2P ports on
-        # 0.0.0.0:P and 127.0.0.1:18445 (for incoming Tor connections), where P is
+        # 0.0.0.0:P and 127.0.0.1:P+1 (for incoming Tor connections), where P is
         # a unique port chosen by the test framework and configured as port=P in
-        # bitcoin.conf. To avoid collisions on 127.0.0.1:18445, change it to
-        # 127.0.0.1:tor_port().
+        # bitcoin.conf. To avoid collisions, change it to 127.0.0.1:tor_port().
         will_listen = all(e != "-nolisten" and e != "-listen=0" for e in extra_args)
         has_explicit_bind = self.has_explicit_bind or any(e.startswith("-bind=") for e in extra_args)
         if will_listen and not has_explicit_bind:

test/functional/test_runner.py

@@ -341,6 +341,7 @@
     'feature_minchainwork.py',
     'rpc_estimatefee.py',
     'rpc_getblockstats.py',
+    'feature_port.py',
     'feature_bind_port_externalip.py',
     'wallet_create_tx.py --legacy-wallet',
     'wallet_send.py --legacy-wallet',