net: don't extra bind for Tor if binds are restricted

The semantic of `-bind` is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to `0.0.0.0`.
If specified then we should honor the restriction and bind only to the
specified address.

Before this change, if no `-bind` is given then we would bind to
`0.0.0.0` and to `127.0.0.1:8334` (incoming Tor) which is ok - the user
does not care to restrict the binding. However:

* If only `-bind=addr` is given (without `-bind=...=onion`) then we
  would bind to `addr` _and_ to `127.0.0.1:8334` in addition which may
  be unexpected assuming the perception of `-bind=addr` is
  "bind _only_ to `addr`".

* If only `-bind=addr=onion` is given (without ordinary `-bind=`)
  then we would bind to `addr` _and_ to `0.0.0.0` in addition.

Change the above two cases to not do the additional bind. If `-bind`
is given then only bind to the specified address:

* If only `-bind=addr` is given (without `-bind=...=onion`) then bind to
  `addr` (only). If we are creating tor hidden service then use `addr`
  as target. Same behavior as before
  bitcoin#19991.

* If only `-bind=addr=onion` is given (without ordinary `-bind=`)
  then bind to `addr` (only) and consider incoming connections as Tor
  and do not advertise `addr`. I.e. a Tor-only node.

Author: vasild

src/init.cpp

@@ -1965,25 +1965,39 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
         return InitError(ResolveErrMsg("bind", bind_arg));
     }
 
-    if (connOptions.onion_binds.empty()) {
-        connOptions.onion_binds.push_back(DefaultOnionServiceTarget());
-    }
-
-    if (args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) {
-        const auto bind_addr = 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."), bind_addr.ToStringIPPort()));
-        }
-        StartTorControl(bind_addr);
-    }
-
     for (const std::string& strBind : args.GetArgs("-whitebind")) {
         NetWhitebindPermissions whitebind;
         bilingual_str error;
         if (!NetWhitebindPermissions::TryParse(strBind, whitebind, error)) return InitError(error);
         connOptions.vWhiteBinds.push_back(whitebind);
     }
 
+    // If the user did not specify -bind= or -whitebind= then we bind
+    // on any address - 0.0.0.0 (IPv4) and :: (IPv6).
+    connOptions.bind_on_any = args.GetArgs("-bind").empty() && args.GetArgs("-whitebind").empty();
+
+    CService onion_service_target;
+    if (!connOptions.onion_binds.empty()) {
+        onion_service_target = connOptions.onion_binds.front();
+    } else if (!connOptions.vBinds.empty()) {
+        onion_service_target = connOptions.vBinds.front();
+    } else if (!connOptions.vWhiteBinds.empty()) {
+        onion_service_target = connOptions.vWhiteBinds.front().m_service;
+    } else {
+        assert(connOptions.bind_on_any);
+        onion_service_target = DefaultOnionServiceTarget();
+        connOptions.onion_binds.push_back(onion_service_target);
+    }
+
+    if (args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) {
+        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.ToStringIPPort()));
+        }
+        StartTorControl(onion_service_target);
+    }
+
     for (const auto& net : args.GetArgs("-whitelist")) {
         NetWhitelistPermissions subnet;
         bilingual_str error;

src/net.cpp

@@ -2391,30 +2391,25 @@ bool CConnman::Bind(const CService &addr, unsigned int flags, NetPermissionFlags
     return true;
 }
 
-bool CConnman::InitBinds(
-    const std::vector<CService>& binds,
-    const std::vector<NetWhitebindPermissions>& whiteBinds,
-    const std::vector<CService>& onion_binds)
+bool CConnman::InitBinds(const Options& options)
 {
     bool fBound = false;
-    for (const auto& addrBind : binds) {
+    for (const auto& addrBind : options.vBinds) {
         fBound |= Bind(addrBind, (BF_EXPLICIT | BF_REPORT_ERROR), NetPermissionFlags::PF_NONE);
     }
-    for (const auto& addrBind : whiteBinds) {
+    for (const auto& addrBind : options.vWhiteBinds) {
         fBound |= Bind(addrBind.m_service, (BF_EXPLICIT | BF_REPORT_ERROR), addrBind.m_flags);
     }
-    if (binds.empty() && whiteBinds.empty()) {
+    for (const auto& addr_bind : options.onion_binds) {
+        fBound |= Bind(addr_bind, BF_EXPLICIT | BF_REPORT_ERROR | BF_DONT_ADVERTISE, NetPermissionFlags::PF_NONE);
+    }
+    if (options.bind_on_any) {
         struct in_addr inaddr_any;
         inaddr_any.s_addr = htonl(INADDR_ANY);
         struct in6_addr inaddr6_any = IN6ADDR_ANY_INIT;
         fBound |= Bind(CService(inaddr6_any, GetListenPort()), BF_NONE, NetPermissionFlags::PF_NONE);
         fBound |= Bind(CService(inaddr_any, GetListenPort()), !fBound ? BF_REPORT_ERROR : BF_NONE, NetPermissionFlags::PF_NONE);
     }
-
-    for (const auto& addr_bind : onion_binds) {
-        fBound |= Bind(addr_bind, BF_EXPLICIT | BF_DONT_ADVERTISE, NetPermissionFlags::PF_NONE);
-    }
-
     return fBound;
 }
 
@@ -2433,7 +2428,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
         nMaxOutboundCycleStartTime = 0;
     }
 
-    if (fListen && !InitBinds(connOptions.vBinds, connOptions.vWhiteBinds, connOptions.onion_binds)) {
+    if (fListen && !InitBinds(connOptions)) {
         if (clientInterface) {
             clientInterface->ThreadSafeMessageBox(
                 _("Failed to listen on any port. Use -listen=0 if you want this."),

src/net.h

@@ -217,6 +217,9 @@ class CConnman
         std::vector<NetWhitebindPermissions> vWhiteBinds;
         std::vector<CService> vBinds;
         std::vector<CService> onion_binds;
+        /// 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;
         bool m_use_addrman_outgoing = true;
         std::vector<std::string> m_specified_outgoing;
         std::vector<std::string> m_added_nodes;
@@ -415,10 +418,7 @@ class CConnman
 
     bool BindListenPort(const CService& bindAddr, bilingual_str& strError, NetPermissionFlags permissions);
     bool Bind(const CService& addr, unsigned int flags, NetPermissionFlags permissions);
-    bool InitBinds(
-        const std::vector<CService>& binds,
-        const std::vector<NetWhitebindPermissions>& whiteBinds,
-        const std::vector<CService>& onion_binds);
+    bool InitBinds(const Options& options);
 
     void ThreadOpenAddedConnections();
     void AddAddrFetch(const std::string& strDest);

test/functional/feature_bind_extra.py

@@ -0,0 +1,114 @@
+#!/usr/bin/env python3
+# Copyright (c) 2014-2020 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 starting bitcoind with -bind and/or -bind=...=onion and confirm
+that bind happens on the expected ports.
+"""
+
+import sys
+
+from test_framework.netutil import (
+    addr_to_hex,
+    get_bind_addrs,
+)
+from test_framework.test_framework import (
+    BitcoinTestFramework,
+    SkipTest,
+)
+from test_framework.util import (
+    PORT_MIN,
+    PORT_RANGE,
+    assert_equal,
+    p2p_port,
+    rpc_port,
+)
+
+# From chainparamsbase.cpp:CreateBaseChainParams().
+REGTEST_TOR_TARGET_PORT = 18445
+
+class BindExtraTest(BitcoinTestFramework):
+    def set_test_params(self):
+        # Avoid any -bind= on the command line. Force the framework to avoid
+        # adding -bind=127.0.0.1.
+        self.setup_clean_chain = True
+        self.bind_to_localhost_only = False
+        self.num_nodes = 4
+
+    def setup_network(self):
+        # Override setup_network() because we want to put the result of
+        # p2p_port() in self.extra_args[], before the nodes are started.
+        # p2p_port() is not usable in set_test_params() because PortSeed.n is
+        # not set at that time.
+
+        # Due to OS-specific network stats queries, we only run on Linux.
+        self.log.info("Checking for Linux")
+        if not sys.platform.startswith('linux'):
+            raise SkipTest("This test can only be run on Linux.")
+
+        any_ipv4 = addr_to_hex('0.0.0.0')
+        loopback_ipv4 = addr_to_hex('127.0.0.1')
+
+        # Start custom ports after p2p and rpc ports.
+        port = PORT_MIN + 2 * PORT_RANGE
+
+        # Array of tuples [command line arguments, expected bind addresses].
+        self.expected = []
+
+        # Node0, no -bind, expected to bind on any + tor target.
+        self.expected.append(
+            [
+                [],
+                [(any_ipv4, p2p_port(0)), (loopback_ipv4, REGTEST_TOR_TARGET_PORT)]
+            ]
+        )
+
+        # Node1, no -bind=...=onion, thus no extra port for Tor target.
+        self.expected.append(
+            [
+                ['-bind=127.0.0.1:{}'.format(port)],
+                [(loopback_ipv4, port)]
+            ],
+        )
+        port += 1
+
+        # Node2, no normal -bind, thus only the Tor target.
+        self.expected.append(
+            [
+                ['-bind=127.0.0.1:{}=onion'.format(port)],
+                [(loopback_ipv4, port)]
+            ],
+        )
+        port += 1
+
+        # Node3, both -bind and -bind=...=onion.
+        self.expected.append(
+            [
+                ['-bind=127.0.0.1:{}'.format(port), '-bind=127.0.0.1:{}=onion'.format(port + 1)],
+                [(loopback_ipv4, port), (loopback_ipv4, port + 1)]
+            ],
+        )
+        port += 2
+
+        # Add RPC ports to the list of expected ports to bind to for all nodes.
+        # They are not relevant for this test.
+        for i in range(len(self.expected)):
+            self.expected[i][1].append((loopback_ipv4, rpc_port(i)))
+
+        self.extra_args = list(map(lambda e: e[0], self.expected))
+        self.setup_nodes()
+
+    def run_test(self):
+        for i in range(len(self.expected)):
+            pid = self.nodes[i].process.pid
+            actual = set(get_bind_addrs(pid))
+            # Remove IPv6 addresses because on some CI environments "::1" is not configured
+            # on the system (so our test_ipv6_local() would return False), but it is
+            # possible to bind on "::". This makes it unpredictable whether to expect
+            # that bitcoind has bound on "::1" (for RPC) and "::" (for P2P).
+            actual_without_ipv6 = set(filter(lambda e: len(e[0]) != 32, actual))
+            assert_equal(actual_without_ipv6, set(self.expected[i][1]))
+
+if __name__ == '__main__':
+    BindExtraTest().main()

test/functional/test_runner.py

@@ -129,6 +129,7 @@
     'feature_fee_estimation.py',
     'interface_zmq.py',
     'interface_bitcoin_cli.py',
+    'feature_bind_extra.py',
     'mempool_resurrect.py',
     'wallet_txn_doublespend.py --mineblock',
     'tool_wallet.py',