From c25e0e05550426f29d79571368d90f63fb472b02 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Thu, 31 Aug 2023 13:40:37 -0700 Subject: [PATCH 1/4] net, refactor: move calculations for connection type limits into connman Currently the logic is fragmented between init and connman. Encapsulating this logic within connman allows for less mental overhead and easier reuse in tests. Co-authored-by: Martin Zumsande --- src/init.cpp | 4 ---- src/net.h | 16 +++++----------- src/test/denialofservice_tests.cpp | 4 ---- 3 files changed, 5 insertions(+), 19 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index a0b44258981..1c8771133d2 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1749,10 +1749,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) CConnman::Options connOptions; connOptions.nLocalServices = nLocalServices; connOptions.nMaxConnections = nMaxConnections; - connOptions.m_max_outbound_full_relay = std::min(MAX_OUTBOUND_FULL_RELAY_CONNECTIONS, connOptions.nMaxConnections); - connOptions.m_max_outbound_block_relay = std::min(MAX_BLOCK_RELAY_ONLY_CONNECTIONS, connOptions.nMaxConnections-connOptions.m_max_outbound_full_relay); - connOptions.nMaxAddnode = MAX_ADDNODE_CONNECTIONS; - connOptions.nMaxFeeler = MAX_FEELER_CONNECTIONS; connOptions.uiInterface = &uiInterface; connOptions.m_banman = node.banman.get(); connOptions.m_msgproc = node.peerman.get(); diff --git a/src/net.h b/src/net.h index 2f7b832fbaa..46ab8dad33d 100644 --- a/src/net.h +++ b/src/net.h @@ -1058,10 +1058,6 @@ public: { ServiceFlags nLocalServices = NODE_NONE; int nMaxConnections = 0; - int m_max_outbound_full_relay = 0; - int m_max_outbound_block_relay = 0; - int nMaxAddnode = 0; - int nMaxFeeler = 0; CClientUIInterface* uiInterface = nullptr; NetEventsInterface* m_msgproc = nullptr; BanMan* m_banman = nullptr; @@ -1089,12 +1085,10 @@ public: nLocalServices = connOptions.nLocalServices; nMaxConnections = connOptions.nMaxConnections; - m_max_outbound_full_relay = std::min(connOptions.m_max_outbound_full_relay, connOptions.nMaxConnections); - m_max_outbound_block_relay = connOptions.m_max_outbound_block_relay; - m_use_addrman_outgoing = connOptions.m_use_addrman_outgoing; - nMaxAddnode = connOptions.nMaxAddnode; - nMaxFeeler = connOptions.nMaxFeeler; + m_max_outbound_full_relay = std::min(MAX_OUTBOUND_FULL_RELAY_CONNECTIONS, nMaxConnections); + m_max_outbound_block_relay = std::min(MAX_BLOCK_RELAY_ONLY_CONNECTIONS, nMaxConnections - m_max_outbound_full_relay); m_max_outbound = m_max_outbound_full_relay + m_max_outbound_block_relay + nMaxFeeler; + m_use_addrman_outgoing = connOptions.m_use_addrman_outgoing; m_client_interface = connOptions.uiInterface; m_banman = connOptions.m_banman; m_msgproc = connOptions.m_msgproc; @@ -1488,8 +1482,8 @@ private: // We do not relay tx or addr messages with these peers int m_max_outbound_block_relay; - int nMaxAddnode; - int nMaxFeeler; + int nMaxAddnode{MAX_ADDNODE_CONNECTIONS}; + int nMaxFeeler{MAX_FEELER_CONNECTIONS}; int m_max_outbound; bool m_use_addrman_outgoing; CClientUIInterface* m_client_interface; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 6e740a4f53a..2abbaf1b239 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -148,8 +148,6 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) constexpr int max_outbound_full_relay = MAX_OUTBOUND_FULL_RELAY_CONNECTIONS; CConnman::Options options; options.nMaxConnections = DEFAULT_MAX_PEER_CONNECTIONS; - options.m_max_outbound_full_relay = max_outbound_full_relay; - options.nMaxFeeler = MAX_FEELER_CONNECTIONS; const auto time_init{GetTime()}; SetMockTime(time_init); @@ -249,8 +247,6 @@ BOOST_AUTO_TEST_CASE(block_relay_only_eviction) constexpr int64_t MINIMUM_CONNECT_TIME{30}; CConnman::Options options; options.nMaxConnections = DEFAULT_MAX_PEER_CONNECTIONS; - options.m_max_outbound_full_relay = MAX_OUTBOUND_FULL_RELAY_CONNECTIONS; - options.m_max_outbound_block_relay = max_outbound_block_relay; connman->Init(options); std::vector vNodes; From e9fd9c0225527ec7727d2a7ccbdf028784aadc6c Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Thu, 31 Aug 2023 13:41:30 -0700 Subject: [PATCH 2/4] net: add m_max_inbound to connman Extract the logic for calculating & maintaining inbound connection limits to be a member within connman for consistency with other maximum connection limits. Note that we now limit m_max_inbound to 0 and don't call AttemptToEvictConnection() when we don't have any inbounds. Previously, nMaxInbound could become negative if the user ran with a low -maxconnections, which didn't break any logic but didn't make sense. Co-authored-by: Martin Zumsande --- src/net.cpp | 5 ++--- src/net.h | 2 ++ 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 13f44304240..565e54875b1 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1777,7 +1777,6 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, const CAddress& addr) { int nInbound = 0; - int nMaxInbound = nMaxConnections - m_max_outbound; AddWhitelistPermissionFlags(permission_flags, addr); if (NetPermissions::HasFlag(permission_flags, NetPermissionFlags::Implicit)) { @@ -1823,13 +1822,13 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, // Only accept connections from discouraged peers if our inbound slots aren't (almost) full. bool discouraged = m_banman && m_banman->IsDiscouraged(addr); - if (!NetPermissions::HasFlag(permission_flags, NetPermissionFlags::NoBan) && nInbound + 1 >= nMaxInbound && discouraged) + if (!NetPermissions::HasFlag(permission_flags, NetPermissionFlags::NoBan) && nInbound + 1 >= m_max_inbound && discouraged) { LogPrint(BCLog::NET, "connection from %s dropped (discouraged)\n", addr.ToStringAddrPort()); return; } - if (nInbound >= nMaxInbound) + if (nInbound >= m_max_inbound) { if (!AttemptToEvictConnection()) { // No connection to evict, disconnect the new connection diff --git a/src/net.h b/src/net.h index 46ab8dad33d..7d2e0e77ee9 100644 --- a/src/net.h +++ b/src/net.h @@ -1088,6 +1088,7 @@ public: m_max_outbound_full_relay = std::min(MAX_OUTBOUND_FULL_RELAY_CONNECTIONS, nMaxConnections); m_max_outbound_block_relay = std::min(MAX_BLOCK_RELAY_ONLY_CONNECTIONS, nMaxConnections - m_max_outbound_full_relay); m_max_outbound = m_max_outbound_full_relay + m_max_outbound_block_relay + nMaxFeeler; + m_max_inbound = std::max(0, nMaxConnections - m_max_outbound); m_use_addrman_outgoing = connOptions.m_use_addrman_outgoing; m_client_interface = connOptions.uiInterface; m_banman = connOptions.m_banman; @@ -1485,6 +1486,7 @@ private: int nMaxAddnode{MAX_ADDNODE_CONNECTIONS}; int nMaxFeeler{MAX_FEELER_CONNECTIONS}; int m_max_outbound; + int m_max_inbound; bool m_use_addrman_outgoing; CClientUIInterface* m_client_interface; NetEventsInterface* m_msgproc; From adc171edf45ec90857d990b8ec570f3c8c2242b7 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Wed, 30 Aug 2023 13:51:09 -0700 Subject: [PATCH 3/4] scripted-diff: Rename connection limit variables -BEGIN VERIFY SCRIPT- sed -i 's/nMaxConnections/m_max_automatic_connections/g' src/net.h src/net.cpp sed -i 's/\.nMaxConnections/\.m_max_automatic_connections/g' src/init.cpp src/test/denialofservice_tests.cpp sed -i 's/nMaxFeeler/m_max_feeler/g' src/net.h sed -i 's/nMaxAddnode/m_max_addnode/g' src/net.h src/net.cpp sed -i 's/m_max_outbound\([^_]\)/m_max_automatic_outbound\1/g' src/net.h src/net.cpp -END VERIFY SCRIPT- Co-authored-by: Martin Zumsande --- src/init.cpp | 2 +- src/net.cpp | 10 +++++----- src/net.h | 20 ++++++++++---------- src/test/denialofservice_tests.cpp | 4 ++-- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 1c8771133d2..7ddb175556a 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1748,7 +1748,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) CConnman::Options connOptions; connOptions.nLocalServices = nLocalServices; - connOptions.nMaxConnections = nMaxConnections; + connOptions.m_max_automatic_connections = nMaxConnections; connOptions.uiInterface = &uiInterface; connOptions.m_banman = node.banman.get(); connOptions.m_msgproc = node.peerman.get(); diff --git a/src/net.cpp b/src/net.cpp index 565e54875b1..cfd9b85b112 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2778,7 +2778,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) // different netgroups in ipv4/ipv6 networks + all peers in Tor/I2P/CJDNS networks. // Don't record addrman failure attempts when node is offline. This can be identified since all local // network connections (if any) belong in the same netgroup, and the size of `outbound_ipv46_peer_netgroups` would only be 1. - const bool count_failures{((int)outbound_ipv46_peer_netgroups.size() + outbound_privacy_network_peers) >= std::min(nMaxConnections - 1, 2)}; + const bool count_failures{((int)outbound_ipv46_peer_netgroups.size() + outbound_privacy_network_peers) >= std::min(m_max_automatic_connections - 1, 2)}; // Use BIP324 transport when both us and them have NODE_V2_P2P set. const bool use_v2transport(addrConnect.nServices & GetLocalServices() & NODE_P2P_V2); OpenNetworkConnection(addrConnect, count_failures, std::move(grant), /*strDest=*/nullptr, conn_type, use_v2transport); @@ -3248,11 +3248,11 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) if (semOutbound == nullptr) { // initialize semaphore - semOutbound = std::make_unique(std::min(m_max_outbound, nMaxConnections)); + semOutbound = std::make_unique(std::min(m_max_automatic_outbound, m_max_automatic_connections)); } if (semAddnode == nullptr) { // initialize semaphore - semAddnode = std::make_unique(nMaxAddnode); + semAddnode = std::make_unique(m_max_addnode); } // @@ -3334,13 +3334,13 @@ void CConnman::Interrupt() InterruptSocks5(true); if (semOutbound) { - for (int i=0; ipost(); } } if (semAddnode) { - for (int i=0; ipost(); } } diff --git a/src/net.h b/src/net.h index 7d2e0e77ee9..19c613ab634 100644 --- a/src/net.h +++ b/src/net.h @@ -1057,7 +1057,7 @@ public: struct Options { ServiceFlags nLocalServices = NODE_NONE; - int nMaxConnections = 0; + int m_max_automatic_connections = 0; CClientUIInterface* uiInterface = nullptr; NetEventsInterface* m_msgproc = nullptr; BanMan* m_banman = nullptr; @@ -1084,11 +1084,11 @@ public: AssertLockNotHeld(m_total_bytes_sent_mutex); nLocalServices = connOptions.nLocalServices; - nMaxConnections = connOptions.nMaxConnections; - m_max_outbound_full_relay = std::min(MAX_OUTBOUND_FULL_RELAY_CONNECTIONS, nMaxConnections); - m_max_outbound_block_relay = std::min(MAX_BLOCK_RELAY_ONLY_CONNECTIONS, nMaxConnections - m_max_outbound_full_relay); - m_max_outbound = m_max_outbound_full_relay + m_max_outbound_block_relay + nMaxFeeler; - m_max_inbound = std::max(0, nMaxConnections - m_max_outbound); + m_max_automatic_connections = connOptions.m_max_automatic_connections; + m_max_outbound_full_relay = std::min(MAX_OUTBOUND_FULL_RELAY_CONNECTIONS, m_max_automatic_connections); + m_max_outbound_block_relay = std::min(MAX_BLOCK_RELAY_ONLY_CONNECTIONS, m_max_automatic_connections - m_max_outbound_full_relay); + m_max_automatic_outbound = m_max_outbound_full_relay + m_max_outbound_block_relay + m_max_feeler; + m_max_inbound = std::max(0, m_max_automatic_connections - m_max_automatic_outbound); m_use_addrman_outgoing = connOptions.m_use_addrman_outgoing; m_client_interface = connOptions.uiInterface; m_banman = connOptions.m_banman; @@ -1474,7 +1474,7 @@ private: std::unique_ptr semOutbound; std::unique_ptr semAddnode; - int nMaxConnections; + int m_max_automatic_connections; // How many full-relay (tx, block, addr) outbound peers we want int m_max_outbound_full_relay; @@ -1483,9 +1483,9 @@ private: // We do not relay tx or addr messages with these peers int m_max_outbound_block_relay; - int nMaxAddnode{MAX_ADDNODE_CONNECTIONS}; - int nMaxFeeler{MAX_FEELER_CONNECTIONS}; - int m_max_outbound; + int m_max_addnode{MAX_ADDNODE_CONNECTIONS}; + int m_max_feeler{MAX_FEELER_CONNECTIONS}; + int m_max_automatic_outbound; int m_max_inbound; bool m_use_addrman_outgoing; CClientUIInterface* m_client_interface; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 2abbaf1b239..0fef8c59069 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -147,7 +147,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) constexpr int max_outbound_full_relay = MAX_OUTBOUND_FULL_RELAY_CONNECTIONS; CConnman::Options options; - options.nMaxConnections = DEFAULT_MAX_PEER_CONNECTIONS; + options.m_max_automatic_connections = DEFAULT_MAX_PEER_CONNECTIONS; const auto time_init{GetTime()}; SetMockTime(time_init); @@ -246,7 +246,7 @@ BOOST_AUTO_TEST_CASE(block_relay_only_eviction) constexpr int max_outbound_block_relay{MAX_BLOCK_RELAY_ONLY_CONNECTIONS}; constexpr int64_t MINIMUM_CONNECT_TIME{30}; CConnman::Options options; - options.nMaxConnections = DEFAULT_MAX_PEER_CONNECTIONS; + options.m_max_automatic_connections = DEFAULT_MAX_PEER_CONNECTIONS; connman->Init(options); std::vector vNodes; From df69b22f2e3cc03764a582f29a16a36114f67e17 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Wed, 30 Aug 2023 14:38:47 -0700 Subject: [PATCH 4/4] doc: improve documentation around connection limit maximums Co-authored-by: Martin Zumsande --- src/init.cpp | 2 +- src/net.h | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/init.cpp b/src/init.cpp index 7ddb175556a..ec61ae80906 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -489,7 +489,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-forcednsseed", strprintf("Always query for peer addresses via DNS lookup (default: %u)", DEFAULT_FORCEDNSSEED), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-listen", strprintf("Accept connections from outside (default: %u if no -proxy, -connect or -maxconnections=0)", DEFAULT_LISTEN), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-listenonion", strprintf("Automatically create Tor onion service (default: %d)", DEFAULT_LISTEN_ONION), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); - argsman.AddArg("-maxconnections=", strprintf("Maintain at most connections to peers (default: %u). This limit does not apply to connections manually added via -addnode or the addnode RPC, which have a separate limit of %u.", DEFAULT_MAX_PEER_CONNECTIONS, MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); + argsman.AddArg("-maxconnections=", strprintf("Maintain at most automatic connections to peers (default: %u). This limit does not apply to connections manually added via -addnode or the addnode RPC, which have a separate limit of %u.", DEFAULT_MAX_PEER_CONNECTIONS, MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-maxreceivebuffer=", strprintf("Maximum per-connection receive buffer, *1000 bytes (default: %u)", DEFAULT_MAXRECEIVEBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-maxsendbuffer=", strprintf("Maximum per-connection memory usage for the send buffer, *1000 bytes (default: %u)", DEFAULT_MAXSENDBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-maxtimeadjustment", strprintf("Maximum allowed median peer time offset adjustment. Local perspective of time may be influenced by outbound peers forward or backward by this amount (default: %u seconds).", DEFAULT_MAX_TIME_ADJUSTMENT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); diff --git a/src/net.h b/src/net.h index 19c613ab634..b746e5a6385 100644 --- a/src/net.h +++ b/src/net.h @@ -1474,8 +1474,19 @@ private: std::unique_ptr semOutbound; std::unique_ptr semAddnode; + + /** + * Maximum number of automatic connections permitted, excluding manual + * connections but including inbounds. May be changed by the user and is + * potentially limited by the operating system (number of file descriptors). + */ int m_max_automatic_connections; + /* + * Maximum number of peers by connection type. Might vary from defaults + * based on -maxconnections init value. + */ + // How many full-relay (tx, block, addr) outbound peers we want int m_max_outbound_full_relay; @@ -1487,6 +1498,7 @@ private: int m_max_feeler{MAX_FEELER_CONNECTIONS}; int m_max_automatic_outbound; int m_max_inbound; + bool m_use_addrman_outgoing; CClientUIInterface* m_client_interface; NetEventsInterface* m_msgproc;