fix: multiple fixes to conditions we check while opening new connections

src/chainparams.cpp

@@ -284,7 +284,6 @@ class CMainParams : public CChainParams {
         fRequireRoutableExternalIP = true;
         m_is_test_chain = false;
         fAllowMultipleAddressesFromGroup = false;
-        fAllowMultiplePorts = false;
         nLLMQConnectionRetryTimeout = 60;
         m_is_mockable_chain = false;
 
@@ -476,7 +475,6 @@ class CTestNetParams : public CChainParams {
         fRequireRoutableExternalIP = true;
         m_is_test_chain = true;
         fAllowMultipleAddressesFromGroup = false;
-        fAllowMultiplePorts = true;
         nLLMQConnectionRetryTimeout = 60;
         m_is_mockable_chain = false;
 
@@ -659,7 +657,6 @@ class CDevNetParams : public CChainParams {
         fRequireRoutableExternalIP = true;
         m_is_test_chain = true;
         fAllowMultipleAddressesFromGroup = true;
-        fAllowMultiplePorts = true;
         nLLMQConnectionRetryTimeout = 60;
         m_is_mockable_chain = false;
 
@@ -861,7 +858,6 @@ class CRegTestParams : public CChainParams {
         fRequireRoutableExternalIP = false;
         m_is_test_chain = true;
         fAllowMultipleAddressesFromGroup = true;
-        fAllowMultiplePorts = true;
         nLLMQConnectionRetryTimeout = 1; // must be lower then the LLMQ signing session timeout so that tests have control over failing behavior
         m_is_mockable_chain = true;
 

src/chainparams.h

@@ -117,8 +117,6 @@ class CChainParams
     bool MineBlocksOnDemand() const { return consensus.fPowNoRetargeting; }
     /** Allow multiple addresses to be selected from the same network group (e.g. 192.168.x.x) */
     bool AllowMultipleAddressesFromGroup() const { return fAllowMultipleAddressesFromGroup; }
-    /** Allow nodes with the same address and multiple ports */
-    bool AllowMultiplePorts() const { return fAllowMultiplePorts; }
     /** How long to wait until we allow retrying of a LLMQ connection  */
     int LLMQConnectionRetryTimeout() const { return nLLMQConnectionRetryTimeout; }
     /** Return the network string */
@@ -176,7 +174,6 @@ class CChainParams
     bool fRequireRoutableExternalIP;
     bool m_is_test_chain;
     bool fAllowMultipleAddressesFromGroup;
-    bool fAllowMultiplePorts;
     bool m_is_mockable_chain;
     int nLLMQConnectionRetryTimeout;
     CCheckpointData checkpointData;

src/net.cpp

@@ -493,8 +493,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
     assert(conn_type != ConnectionType::INBOUND);
 
     if (pszDest == nullptr) {
-        bool fAllowLocal = Params().AllowMultiplePorts() && addrConnect.GetPort() != GetListenPort();
-        if (!fAllowLocal && IsLocal(addrConnect)) {
+        if (addrConnect.GetPort() == GetListenPort() && IsLocal(addrConnect)) {
             return nullptr;
         }
 
@@ -3508,25 +3507,18 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, CDe
                 std::tie(addr, addr_last_try) = addrman.Select(false, preferred_net);
             }
 
-            auto dmn = mnList.GetMNByService(addr);
-            bool isMasternode = dmn != nullptr;
-
             // Require outbound IPv4/IPv6 connections, other than feelers, to be to distinct network groups
             if (!fFeeler && outbound_ipv46_peer_netgroups.count(m_netgroupman.GetGroup(addr))) {
                 continue;
             }
 
             // if we selected an invalid address, restart
-            if (!addr.IsValid() || outbound_ipv46_peer_netgroups.count(m_netgroupman.GetGroup(addr)))
-                break;
-
-            // don't try to connect to masternodes that we already have a connection to (most likely inbound)
-            if (isMasternode && setConnectedMasternodes.count(dmn->proTxHash))
+            if (!addr.IsValid()) {
                 break;
+            }
 
-            // if we selected a local address, restart (local addresses are allowed in regtest and devnet)
-            bool fAllowLocal = Params().AllowMultiplePorts() && addrConnect.GetPort() != GetListenPort();
-            if (!fAllowLocal && IsLocal(addrConnect)) {
+            // don't connect to ourselves
+            if (addr.GetPort() == GetListenPort() && IsLocal(addr)) {
                 break;
             }
 
@@ -3542,32 +3534,17 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, CDe
             // for non-feelers, require all the services we'll want,
             // for feelers, only require they be a full node (only because most
             // SPV clients don't have a good address DB available)
-            if (!isMasternode && !fFeeler && !HasAllDesirableServiceFlags(addr.nServices)) {
+            if (!fFeeler && !HasAllDesirableServiceFlags(addr.nServices)) {
                 continue;
-            } else if (!isMasternode && fFeeler && !MayHaveUsefulAddressDB(addr.nServices)) {
+            } else if (fFeeler && !MayHaveUsefulAddressDB(addr.nServices)) {
                 continue;
             }
 
-            // 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;
-                }
-            }();
+            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
 
             // Do not connect to prohibited ports, unless 50 invalid addresses have been selected already.
-            if (nTries < 50 && is_prohibited_port) {
+            if (nTries < 50 && IsBadPort(addr.GetPort())) {
                 continue;
             }
 
@@ -3582,6 +3559,11 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, CDe
                 continue;
             }
 
+            // don't try to connect to masternodes that we already have a connection to (most likely inbound)
+            if (auto dmn = mnList.GetMNByService(addr); dmn && setConnectedMasternodes.count(dmn->proTxHash)) {
+                continue;
+            }
+
             addrConnect = addr;
             break;
         }
@@ -3918,20 +3900,8 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
         // banned, discouraged or exact match?
         if ((m_banman && (m_banman->IsDiscouraged(addrConnect) || m_banman->IsBanned(addrConnect))) || AlreadyConnectedToAddress(addrConnect))
             return;
-        // local and not a connection to itself?
-        bool fAllowLocal = Params().AllowMultiplePorts() && addrConnect.GetPort() != GetListenPort();
-        if (!fAllowLocal && IsLocal(addrConnect))
-            return;
-        // Search for IP:PORT match:
-        //  - if multiple ports for the same IP are allowed,
-        //  - for probe connections
-        // Search for IP-only match otherwise
-        bool searchIPPort = Params().AllowMultiplePorts() || masternode_probe_connection == MasternodeProbeConn::IsConnection;
-        bool skip = searchIPPort ?
-                FindNode(static_cast<CService>(addrConnect)) :
-                FindNode(static_cast<CNetAddr>(addrConnect));
-        if (skip) {
-            LogPrintf("CConnman::%s -- Failed to open new connection to %s, already connected\n", __func__, getIpStr());
+        // connecting to ourselves?
+        if (addrConnect.GetPort() == GetListenPort() && IsLocal(addrConnect)) {
             return;
         }
     } else if (FindNode(std::string(pszDest)))

src/netfulfilledman.cpp

@@ -32,15 +32,13 @@ CNetFulfilledRequestManager::~CNetFulfilledRequestManager()
 void CNetFulfilledRequestManager::AddFulfilledRequest(const CService& addr, const std::string& strRequest)
 {
     LOCK(cs_mapFulfilledRequests);
-    CService addrSquashed = Params().AllowMultiplePorts() ? addr : CService(addr, 0);
-    mapFulfilledRequests[addrSquashed][strRequest] = GetTime() + Params().FulfilledRequestExpireTime();
+    mapFulfilledRequests[addr][strRequest] = GetTime() + Params().FulfilledRequestExpireTime();
 }
 
 bool CNetFulfilledRequestManager::HasFulfilledRequest(const CService& addr, const std::string& strRequest)
 {
     LOCK(cs_mapFulfilledRequests);
-    CService addrSquashed = Params().AllowMultiplePorts() ? addr : CService(addr, 0);
-    fulfilledreqmap_t::iterator it = mapFulfilledRequests.find(addrSquashed);
+    fulfilledreqmap_t::iterator it = mapFulfilledRequests.find(addr);
 
     return  it != mapFulfilledRequests.end() &&
             it->second.find(strRequest) != it->second.end() &&
@@ -50,8 +48,7 @@ bool CNetFulfilledRequestManager::HasFulfilledRequest(const CService& addr, cons
 void CNetFulfilledRequestManager::RemoveAllFulfilledRequests(const CService& addr)
 {
     LOCK(cs_mapFulfilledRequests);
-    CService addrSquashed = Params().AllowMultiplePorts() ? addr : CService(addr, 0);
-    fulfilledreqmap_t::iterator it = mapFulfilledRequests.find(addrSquashed);
+    fulfilledreqmap_t::iterator it = mapFulfilledRequests.find(addr);
 
     if (it != mapFulfilledRequests.end()) {
         mapFulfilledRequests.erase(it++);