Merge #6532: backport: merge bitcoin#21845, bitcoin#22732, bitcoin#23542, bitcoin#24468, bitcoin#25119, bitcoin#25176, bitcoin#25421, bitcoin#26248, bitcoin#26199, bitcoin#27036, bitcoin#27270, partial bitcoin#27106 (networking backports: part 10)

3260f2c partial bitcoin#27106: remove orphaned CSubNet::SanityCheck() (Kittywhiskers Van Gogh)
75cc94e merge bitcoin#27270: Avoid CNode::m_relays_txs usage (Kittywhiskers Van Gogh)
f961903 merge bitcoin#27036: Remove last uses of snprintf and simplify (Kittywhiskers Van Gogh)
d80bbe9 merge bitcoin#26199: Don't self-advertise during version processing (Kittywhiskers Van Gogh)
4b17baf merge bitcoin#26248: Set relay in version msg to peers with relay permission in -blocksonly mode (Kittywhiskers Van Gogh)
18738f5 merge bitcoin#25421: convert standalone IsSelectableSocket() and SetSocketNonBlocking() to Sock methods (Kittywhiskers Van Gogh)
8782575 merge bitcoin#25176: Fix frequent -netinfo JSON errors from missing getpeerinfo#relaytxes (Kittywhiskers Van Gogh)
1d96a47 merge bitcoin#25119: move StartExtraBlockRelayPeers() from header to implementation (Kittywhiskers Van Gogh)
37e0c58 merge bitcoin#24468: improve -onlynet help and related tor/i2p documentation (Kittywhiskers Van Gogh)
52c3b03 merge bitcoin#23542: open p2p connections to nodes that listen on non-default ports (Kittywhiskers Van Gogh)
8e2a12a merge bitcoin#22732: use m_client_interface rather than uiInterface (Kittywhiskers Van Gogh)
06a8e9c merge bitcoin#21845: Don't require locking cs_main before calling RelayTransactions() (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Due to changes introduced in [bitcoin#21845](bitcoin#21845), a `LOCK(cs_main)` had to be added to `PeerManagerImpl::ReattemptInitialBroadcast()` ([source](06a8e9c#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R1634)). This addition is in line with upstream ([source](https://github.com/bitcoin/bitcoin/blob/39e19713cd6594f93db835e8ef7eef5824a9ba02/src/net_processing.cpp#L1021)).

  * While nodes have been allowed to connect to non-default ports since [dash#2168](#2168), [bitcoin#23542](bitcoin#23542) also adds a list of ports considered "bad" that while not outright prohibited, are heavily discouraged from use as they are considered _de facto_ prohibited.

    In combination with port restrictions for masternodes (connections only permitted if matching listening port), port validation logic was better served by implementing it in a lambda block that's immediately executed (see `is_prohibited_port` for more information, [source](https://github.com/dashpay/dash/blob/01975fba32b8fcd8bccb0ce293b217c07b522c53/src/net.cpp#L3551-L3567)).

  * In [bitcoin#25176](bitcoin#25176), `is_block_relay` is renamed to `is_tx_relay` as the block relay-only = not transaction-relay assumption no longer holds true (see [dash#6365](#6365) for more information). One use of `is_block_relay` which relied on this now-obsolete assumption is incrementing `m_block_relay_peers_count`. It has been replaced with checking for a `block-relay-only` `conn_type`, matching upstream ([source](https://github.com/bitcoin/bitcoin/blob/a17c5e96b602fed65166037b78d98605e915206b/src/bitcoin-cli.cpp#L486)).

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 3260f2c
  PastaPastaPasta:
    utACK 3260f2c

Tree-SHA512: 2bb50deb77ffaf7f7c4b396a8efe03f8bbfa605b49b75eace5fbc9d9813d4de8b72b086c50fbb0c23b97237c1f4c1b30b68f4b456bb74875308a4fcaa82deb08

Author: PastaPastaPasta

doc/README.md

@@ -76,6 +76,7 @@ The Dash Core repo's [root README](/README.md) contains relevant information on
 - [I2P Support](i2p.md)
 - [Init Scripts (systemd/upstart/openrc)](init.md)
 - [Managing Wallets](managing-wallets.md)
+- [P2P bad ports definition and list](p2p-bad-ports.md)
 - [PSBT support](psbt.md)
 - [Reduce Memory](reduce-memory.md)
 - [Reduce Traffic](reduce-traffic.md)

doc/p2p-bad-ports.md

@@ -0,0 +1,114 @@
+When Dash Core automatically opens outgoing P2P connections, it chooses
+a peer (address and port) from its list of potential peers. This list is
+populated with unchecked data gossiped over the P2P network by other peers.
+
+A malicious actor may gossip an address:port where no Dash node is listening,
+or one where a service is listening that is not related to the Dash network.
+As a result, this service may occasionally get connection attempts from Dash
+nodes.
+
+"Bad" ports are ones used by services which are usually not open to the public
+and usually require authentication. A connection attempt (by Dash Core,
+trying to connect because it thinks there is a Dash node on that
+address:port) to such service may be considered a malicious action by an
+ultra-paranoid administrator. An example for such a port is 22 (ssh). On the
+other hand, connection attempts to public services that usually do not require
+authentication are unlikely to be considered a malicious action,
+e.g. port 80 (http).
+
+Below is a list of "bad" ports which Dash Core avoids when choosing a peer to
+connect to. If a node is listening on such a port, it will likely receive fewer
+incoming connections.
+
+    1:     tcpmux
+    7:     echo
+    9:     discard
+    11:    systat
+    13:    daytime
+    15:    netstat
+    17:    qotd
+    19:    chargen
+    20:    ftp data
+    21:    ftp access
+    22:    ssh
+    23:    telnet
+    25:    smtp
+    37:    time
+    42:    name
+    43:    nicname
+    53:    domain
+    69:    tftp
+    77:    priv-rjs
+    79:    finger
+    87:    ttylink
+    95:    supdup
+    101:   hostname
+    102:   iso-tsap
+    103:   gppitnp
+    104:   acr-nema
+    109:   pop2
+    110:   pop3
+    111:   sunrpc
+    113:   auth
+    115:   sftp
+    117:   uucp-path
+    119:   nntp
+    123:   NTP
+    135:   loc-srv /epmap
+    137:   netbios
+    139:   netbios
+    143:   imap2
+    161:   snmp
+    179:   BGP
+    389:   ldap
+    427:   SLP (Also used by Apple Filing Protocol)
+    465:   smtp+ssl
+    512:   print / exec
+    513:   login
+    514:   shell
+    515:   printer
+    526:   tempo
+    530:   courier
+    531:   chat
+    532:   netnews
+    540:   uucp
+    548:   AFP (Apple Filing Protocol)
+    554:   rtsp
+    556:   remotefs
+    563:   nntp+ssl
+    587:   smtp (rfc6409)
+    601:   syslog-conn (rfc3195)
+    636:   ldap+ssl
+    989:   ftps-data
+    990:   ftps
+    993:   ldap+ssl
+    995:   pop3+ssl
+    1719:  h323gatestat
+    1720:  h323hostcall
+    1723:  pptp
+    2049:  nfs
+    3659:  apple-sasl / PasswordServer
+    4045:  lockd
+    5060:  sip
+    5061:  sips
+    6000:  X11
+    6566:  sane-port
+    6665:  Alternate IRC
+    6666:  Alternate IRC
+    6667:  Standard IRC
+    6668:  Alternate IRC
+    6669:  Alternate IRC
+    6697:  IRC + TLS
+    10080: Amanda
+
+For further information see:
+
+[pull/23306](https://github.com/bitcoin/bitcoin/pull/23306#issuecomment-947516736)
+
+[pull/23542](https://github.com/bitcoin/bitcoin/pull/23542)
+
+[fetch.spec.whatwg.org](https://fetch.spec.whatwg.org/#port-blocking)
+
+[chromium.googlesource.com](https://chromium.googlesource.com/chromium/src.git/+/refs/heads/main/net/base/port_util.cc)
+
+[hg.mozilla.org](https://hg.mozilla.org/mozilla-central/file/tip/netwerk/base/nsIOService.cpp)

src/bitcoin-cli.cpp

@@ -433,8 +433,8 @@ class NetinfoRequestHandler : public BaseRequestHandler
         bool is_addr_relay_enabled;
         bool is_bip152_hb_from;
         bool is_bip152_hb_to;
-        bool is_block_relay;
         bool is_outbound;
+        bool is_tx_relay;
         bool operator<(const Peer& rhs) const { return std::tie(is_outbound, min_ping) < std::tie(rhs.is_outbound, rhs.min_ping); }
     };
     std::vector<Peer> m_peers;
@@ -498,13 +498,13 @@ class NetinfoRequestHandler : public BaseRequestHandler
             const int8_t network_id{NetworkStringToId(network)};
             if (network_id == UNKNOWN_NETWORK) continue;
             const bool is_outbound{!peer["inbound"].get_bool()};
-            const bool is_block_relay{peer["relaytxes"].isNull() ? false : !peer["relaytxes"].get_bool()};
+            const bool is_tx_relay{peer["relaytxes"].isNull() ? true : peer["relaytxes"].get_bool()};
             const std::string conn_type{peer["connection_type"].get_str()};
             ++m_counts.at(is_outbound).at(network_id);        // in/out by network
             ++m_counts.at(is_outbound).at(NETWORKS.size());   // in/out overall
             ++m_counts.at(2).at(network_id);                  // total by network
             ++m_counts.at(2).at(NETWORKS.size());             // total overall
-            if (is_block_relay) ++m_block_relay_peers_count;
+            if (conn_type == "block-relay-only") ++m_block_relay_peers_count;
             if (conn_type == "manual") ++m_manual_peers_count;
             if (DetailsRequested()) {
                 // Push data for this peer to the peers vector.
@@ -527,7 +527,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
                 const bool is_addr_relay_enabled{peer["addr_relay_enabled"].isNull() ? false : peer["addr_relay_enabled"].get_bool()};
                 const bool is_bip152_hb_from{peer["bip152_hb_from"].get_bool()};
                 const bool is_bip152_hb_to{peer["bip152_hb_to"].get_bool()};
-                m_peers.push_back({addr, sub_version, conn_type, NETWORK_SHORT_NAMES[network_id], age, transport, min_ping, ping, addr_processed, addr_rate_limited, last_blck, last_recv, last_send, last_trxn, peer_id, mapped_as, version, is_addr_relay_enabled, is_bip152_hb_from, is_bip152_hb_to, is_block_relay, is_outbound});
+                m_peers.push_back({addr, sub_version, conn_type, NETWORK_SHORT_NAMES[network_id], age, transport, min_ping, ping, addr_processed, addr_rate_limited, last_blck, last_recv, last_send, last_trxn, peer_id, mapped_as, version, is_addr_relay_enabled, is_bip152_hb_from, is_bip152_hb_to, is_outbound, is_tx_relay});
                 m_max_addr_length = std::max(addr.length() + 1, m_max_addr_length);
                 m_max_addr_processed_length = std::max(ToString(addr_processed).length(), m_max_addr_processed_length);
                 m_max_addr_rate_limited_length = std::max(ToString(addr_rate_limited).length(), m_max_addr_rate_limited_length);
@@ -561,7 +561,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
                     PingTimeToString(peer.ping),
                     peer.last_send ? ToString(time_now - peer.last_send) : "",
                     peer.last_recv ? ToString(time_now - peer.last_recv) : "",
-                    peer.last_trxn ? ToString((time_now - peer.last_trxn) / 60) : peer.is_block_relay ? "*" : "",
+                    peer.last_trxn ? ToString((time_now - peer.last_trxn) / 60) : peer.is_tx_relay ? "" : "*",
                     peer.last_blck ? ToString((time_now - peer.last_blck) / 60) : "",
                     strprintf("%s%s", peer.is_bip152_hb_to ? "." : " ", peer.is_bip152_hb_from ? "*" : " "),
                     m_max_addr_processed_length, // variable spacing

src/compat/compat.h

@@ -109,14 +109,6 @@ typedef char* sockopt_arg_type;
 #define USE_KQUEUE
 #endif
 
-bool static inline IsSelectableSocket(const SOCKET& s) {
-#if defined(USE_POLL) || defined(WIN32)
-    return true;
-#else
-    return (s < FD_SETSIZE);
-#endif
-}
-
 // MSG_NOSIGNAL is not available on some platforms, if it doesn't exist define it as 0
 #if !defined(MSG_NOSIGNAL)
 #define MSG_NOSIGNAL 0

src/init.cpp

@@ -567,6 +567,8 @@ void SetupServerArgs(ArgsManager& argsman)
     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("-peertimeout=<n>", strprintf("Specify a p2p connection timeout delay in seconds. After connecting to a peer, wait this amount of time before considering disconnection based on inactivity (minimum: 1, default: %d)", DEFAULT_PEER_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-permitbaremultisig", strprintf("Relay non-P2SH multisig (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+    // TODO: remove the sentence "Nodes not using ... incoming connections." once the changes from
+    // https://github.com/bitcoin/bitcoin/pull/23542 have become widespread.
     argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port>. Nodes not using the default ports (default: %u, testnet: %u, regtest: %u) are unlikely to get incoming connections. Not relevant for I2P (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
     argsman.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_ELISION, OptionsCategory::CONNECTION);
     argsman.AddArg("-proxyrandomize", strprintf("Randomize credentials for every proxy connection. This enables Tor stream isolation (default: %u)", DEFAULT_PROXYRANDOMIZE), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
@@ -2271,13 +2273,24 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     const uint16_t default_bind_port =
         static_cast<uint16_t>(args.GetArg("-port", Params().GetDefaultPort()));
 
+    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 Dash Core peers connect to it. See "
+                           "doc/p2p-bad-ports.md for details and a full list."),
+                         prefix,
+                         port);
+    };
+
     for (const std::string& bind_arg : args.GetArgs("-bind")) {
         std::optional<CService> bind_addr;
         const size_t index = bind_arg.rfind('=');
         if (index == std::string::npos) {
             bind_addr = Lookup(bind_arg, default_bind_port, /*fAllowLookup=*/false);
             if (bind_addr.has_value()) {
                 connOptions.vBinds.push_back(bind_addr.value());
+                if (IsBadPort(bind_addr.value().GetPort())) {
+                    InitWarning(BadPortWarning("-bind", bind_addr.value().GetPort()));
+                }
                 continue;
             }
         } else {
@@ -2305,6 +2318,15 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     // on any address - 0.0.0.0 (IPv4) and :: (IPv6).
     connOptions.bind_on_any = args.GetArgs("-bind").empty() && args.GetArgs("-whitebind").empty();
 
+    // Emit a warning if a bad port is given to -port= but only if -bind and -whitebind are not
+    // given, because if they are, then -port= is ignored.
+    if (connOptions.bind_on_any && args.IsArgSet("-port")) {
+        const uint16_t port_arg = args.GetArg("-port", 0);
+        if (IsBadPort(port_arg)) {
+            InitWarning(BadPortWarning("-port", port_arg));
+        }
+    }
+
     CService onion_service_target;
     if (!connOptions.onion_binds.empty()) {
         onion_service_target = connOptions.onion_binds.front();

src/masternode/node.cpp

@@ -162,7 +162,7 @@ void CActiveMasternodeManager::InitInternal(const CBlockIndex* pindex)
         LogPrintf("CActiveMasternodeManager::Init -- ERROR: %s\n", m_error);
         return;
     }
-    bool fConnected = ConnectSocketDirectly(m_info.service, *sock, nConnectTimeout, true) && IsSelectableSocket(sock->Get());
+    bool fConnected = ConnectSocketDirectly(m_info.service, *sock, nConnectTimeout, true) && sock->IsSelectable();
     sock->Reset();
 
     if (!fConnected && Params().RequireRoutableExternalIP()) {

src/net.cpp

@@ -2020,8 +2020,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
         return;
     }
 
-    if (!IsSelectableSocket(sock->Get()))
-    {
+    if (!sock->IsSelectable()) {
         LogPrintf("%s: non-selectable socket\n", strDropped);
         return;
     }
@@ -2306,10 +2305,11 @@ void CConnman::NotifyNumConnectionsChanged(CMasternodeSync& mn_sync)
         mn_sync.Reset();
     }
 
-    if(nodes_size != nPrevNodeCount) {
+    if (nodes_size != nPrevNodeCount) {
         nPrevNodeCount = nodes_size;
-        if(clientInterface)
-            clientInterface->NotifyNumConnectionsChanged(nodes_size);
+        if (m_client_interface) {
+            m_client_interface->NotifyNumConnectionsChanged(nodes_size);
+        }
 
         CalculateNumConnectionsChangedStats();
     }
@@ -3140,6 +3140,12 @@ void CConnman::SetTryNewOutboundPeer(bool flag)
     LogPrint(BCLog::NET, "net: setting try another outbound peer=%s\n", flag ? "true" : "false");
 }
 
+void CConnman::StartExtraBlockRelayPeers()
+{
+    LogPrint(BCLog::NET, "net: enabling extra block-relay-only peers\n");
+    m_start_extra_block_relay_peers = true;
+}
+
 // Return the number of peers we have over our outbound connection limit
 // Exclude peers that are marked for disconnect, or are going to be
 // disconnected soon (eg ADDR_FETCH and FEELER)
@@ -3299,7 +3305,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, CDe
                 // Therefore, we do not add them to addrman in the first place.
                 // In case previously unreachable networks become reachable
                 // (e.g. in case of -onlynet changes by the user), fixed seeds will
-                // be loaded only for networks for which we have no addressses.
+                // be loaded only for networks for which we have no addresses.
                 seed_addrs.erase(std::remove_if(seed_addrs.begin(), seed_addrs.end(),
                                                 [&fixed_seed_networks](const CAddress& addr) { return fixed_seed_networks.count(addr.GetNetwork()) == 0; }),
                                  seed_addrs.end());
@@ -3542,12 +3548,26 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, CDe
                 continue;
             }
 
-            // Do not allow non-default ports, unless after 50 invalid
-            // addresses selected already. This is to prevent malicious peers
-            // from advertising themselves as a service on another host and
-            // port, causing a DoS attack as nodes around the network attempt
-            // to connect to it fruitlessly.
-            if ((!isMasternode || !Params().AllowMultiplePorts()) && addr.GetPort() != Params().GetDefaultPort(addr.GetNetwork()) && addr.GetPort() != GetListenPort() && nTries < 50) {
+            // Port validation in Dash has additional rules. Some networks are prohibited
+            // from using a non-default port while others allow any arbitary port so long
+            // it isn't a bad port (and in the case of masternodes, it matches its listen
+            // port)
+            const bool is_prohibited_port = [this, &addr, &isMasternode](){
+                if (!Params().AllowMultiplePorts()) {
+                    const uint16_t default_port{Params().GetDefaultPort(addr.GetNetwork())};
+                    assert(!IsBadPort(default_port)); // Make sure we never set the default port to a bad port
+                    return addr.GetPort() != default_port;
+                }
+                const bool is_bad_port{IsBadPort(addr.GetPort())};
+                if (isMasternode) {
+                    return addr.GetPort() != GetListenPort() || is_bad_port;
+                } else {
+                    return is_bad_port;
+                }
+            }();
+
+            // Do not connect to prohibited ports, unless 50 invalid addresses have been selected already.
+            if (nTries < 50 && is_prohibited_port) {
                 continue;
             }
 
@@ -4196,7 +4216,9 @@ void CConnman::SetNetworkActive(bool active, CMasternodeSync* const mn_sync)
         mn_sync->Reset();
     }
 
-    uiInterface.NotifyNetworkActiveChanged(fNetworkActive);
+    if (m_client_interface) {
+        m_client_interface->NotifyNetworkActiveChanged(fNetworkActive);
+    }
 }
 
 CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In, AddrMan& addrman_in,
@@ -4225,8 +4247,8 @@ bool CConnman::Bind(const CService& addr_, unsigned int flags, NetPermissionFlag
 
     bilingual_str strError;
     if (!BindListenPort(addr, strError, permissions)) {
-        if ((flags & BF_REPORT_ERROR) && clientInterface) {
-            clientInterface->ThreadSafeMessageBox(strError, "", CClientUIInterface::MSG_ERROR);
+        if ((flags & BF_REPORT_ERROR) && m_client_interface) {
+            m_client_interface->ThreadSafeMessageBox(strError, "", CClientUIInterface::MSG_ERROR);
         }
         return false;
     }
@@ -4276,8 +4298,8 @@ bool CConnman::Start(CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_met
     }
 
     if (fListen && !InitBinds(connOptions)) {
-        if (clientInterface) {
-            clientInterface->ThreadSafeMessageBox(
+        if (m_client_interface) {
+            m_client_interface->ThreadSafeMessageBox(
                 _("Failed to listen on any port. Use -listen=0 if you want this."),
                 "", CClientUIInterface::MSG_ERROR);
         }
@@ -4303,7 +4325,9 @@ bool CConnman::Start(CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_met
         LogPrintf("%i block-relay-only anchors will be tried for connections.\n", m_anchors.size());
     }
 
-    uiInterface.InitMessage(_("Starting network threads…").translated);
+    if (m_client_interface) {
+        m_client_interface->InitMessage(_("Starting network threads…").translated);
+    }
 
     fAddressesInitialized = true;
 
@@ -4350,8 +4374,8 @@ bool CConnman::Start(CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_met
     threadOpenAddedConnections = std::thread(&util::TraceThread, "addcon", [this] { ThreadOpenAddedConnections(); });
 
     if (connOptions.m_use_addrman_outgoing && !connOptions.m_specified_outgoing.empty()) {
-        if (clientInterface) {
-            clientInterface->ThreadSafeMessageBox(
+        if (m_client_interface) {
+            m_client_interface->ThreadSafeMessageBox(
                 _("Cannot provide specific connections and have addrman find outgoing connections at the same time."),
                 "", CClientUIInterface::MSG_ERROR);
         }