From c4819b950e69663077c4fbf9ae7e1b9c04a137fb Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Thu, 23 Jan 2025 17:54:57 +0300 Subject: [PATCH 1/6] fix: 4213 follow-up We backported 15486 incorrectly... --- src/net.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/net.cpp b/src/net.cpp index b3d76edc8bcf4..e768c062988e7 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3517,8 +3517,9 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe } // if we selected an invalid address, restart - if (!addr.IsValid() || outbound_ipv46_peer_netgroups.count(m_netgroupman.GetGroup(addr))) + if (!addr.IsValid()) { break; + } // don't try to connect to masternodes that we already have a connection to (most likely inbound) if (isMasternode && setConnectedMasternodes.count(dmn->proTxHash)) From e5a19c33fddf17731ab6e90d2f5e25207b9f3527 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Thu, 23 Jan 2025 18:33:03 +0300 Subject: [PATCH 2/6] fix: drop useless `fAllowMultiplePorts` and some Dash-specific conditions Dropped conditions: - do not check masternode ports in `ThreadOpenConnections`, it's done on consensus level on DMN registration/updates - do not check connections on IP-only basis, masternodes handle duplicate mn connections via mnauth, regular nodes don't need that at all --- src/chainparams.cpp | 4 ---- src/chainparams.h | 3 --- src/net.cpp | 45 ++++++++--------------------------------- src/netfulfilledman.cpp | 9 +++------ 4 files changed, 11 insertions(+), 50 deletions(-) diff --git a/src/chainparams.cpp b/src/chainparams.cpp index 22fc808efb980..5c29dd9f955a8 100644 --- a/src/chainparams.cpp +++ b/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; diff --git a/src/chainparams.h b/src/chainparams.h index 8f1471f3698d3..a63eb668ed7c5 100644 --- a/src/chainparams.h +++ b/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; diff --git a/src/net.cpp b/src/net.cpp index e768c062988e7..688b7a89ddf87 100644 --- a/src/net.cpp +++ b/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; } @@ -3525,9 +3524,8 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe if (isMasternode && setConnectedMasternodes.count(dmn->proTxHash)) 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; } @@ -3549,26 +3547,11 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe 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; } @@ -3919,20 +3902,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(addrConnect)) : - FindNode(static_cast(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))) diff --git a/src/netfulfilledman.cpp b/src/netfulfilledman.cpp index 73295d0a7d622..65f2e19ff1c22 100644 --- a/src/netfulfilledman.cpp +++ b/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++); From 47f47d861676646f69b9408f90a253a54c7f68b3 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Thu, 23 Jan 2025 22:00:00 +0300 Subject: [PATCH 3/6] fix: don't skip mns when checking for required services --- src/net.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 688b7a89ddf87..13bf2d20fdc75 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3541,9 +3541,9 @@ void CConnman::ThreadOpenConnections(const std::vector 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; } From 8872ba379fe2348dc075a35db3eaffd743f94c0a Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Thu, 23 Jan 2025 22:07:48 +0300 Subject: [PATCH 4/6] fix: do not break when addr is from a set of already connected masternodes --- src/net.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net.cpp b/src/net.cpp index 13bf2d20fdc75..653247fbb4cef 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3522,7 +3522,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe // don't try to connect to masternodes that we already have a connection to (most likely inbound) if (isMasternode && setConnectedMasternodes.count(dmn->proTxHash)) - break; + continue; // don't connect to ourselves if (addr.GetPort() == GetListenPort() && IsLocal(addr)) { From db7f4cf62d69c9982cb574e4f44cd9ea5efd836b Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Fri, 24 Jan 2025 00:37:25 +0300 Subject: [PATCH 5/6] fix: move Dash-specific `GetMNByService()` check to the end of the `while `loop Allows non-specific conditions to trigger first for better compatibility --- src/net.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 653247fbb4cef..4dbc5de65360a 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3507,9 +3507,6 @@ void CConnman::ThreadOpenConnections(const std::vector 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; @@ -3520,10 +3517,6 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe break; } - // don't try to connect to masternodes that we already have a connection to (most likely inbound) - if (isMasternode && setConnectedMasternodes.count(dmn->proTxHash)) - continue; - // don't connect to ourselves if (addr.GetPort() == GetListenPort() && IsLocal(addr)) { break; @@ -3566,6 +3559,11 @@ void CConnman::ThreadOpenConnections(const std::vector 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; } From 8ee01a0688b7ef1567ab3f047395dd471dbe9303 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 29 Jan 2025 00:11:55 +0300 Subject: [PATCH 6/6] feat: check that no network has its default port set to a "bad" one Do this in CConnman's ctor instead of doing it for every addr we try to connect to --- src/net.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 4dbc5de65360a..3d9be31b2a0a4 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3540,9 +3540,6 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe continue; } - 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 && IsBadPort(addr.GetPort())) { continue; @@ -4198,6 +4195,12 @@ CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In, AddrMan& addrman_in, , nSeed0(nSeed0In) , nSeed1(nSeed1In) { + // Make sure we never set the default port to a bad port + for (int n = 0; n < NET_MAX; ++n) { + const bool is_bad_port = IsBadPort(Params().GetDefaultPort(static_cast(n))); + assert(!is_bad_port); + } + SetTryNewOutboundPeer(false); Options connOptions;