From bb117fc246ee61e76f58c82d9895dd0661046ceb Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 2 Nov 2023 15:37:51 -0400 Subject: [PATCH 1/2] Merge bitcoin/bitcoin#28172: refactor: use string_view for passing string literals to Parse{Hash,Hex} bb91131d545d986aab81c4bb13676c4520169259 doc: remove out-of-date external link in src/util/strencodings.h (Jon Atack) 7d494a48ddf4248ef3b1753b6e7f2eeab3a8ecb7 refactor: use string_view to pass string literals to Parse{Hash,Hex} (Jon Atack) Pull request description: as `string_view` is optimized to be trivially copiable, whereas the current code creates a `std::string` copy at each call. These utility methods are called by quite a few RPCs and tests, as well as by each other. ``` $ git grep "ParseHashV\|ParseHashO\|ParseHexV\|ParseHexO" | wc -l 61 ``` Also remove an out-of-date external link. ACKs for top commit: jonatack: Rebased per `git range-diff c9273f6 b94581a bb91131` for an include header from the merge of https://github.com/bitcoin/bitcoin/pull/28230. Should be trivial to re-ACK. maflcko: lgtm ACK bb91131d545d986aab81c4bb13676c4520169259 ns-xvrn: ACK https://github.com/bitcoin/bitcoin/commit/bb91131d545d986aab81c4bb13676c4520169259 achow101: ACK bb91131d545d986aab81c4bb13676c4520169259 brunoerg: crACK bb91131d545d986aab81c4bb13676c4520169259 Tree-SHA512: 9734fe022c9e43fd93c23a917770d332dbbd3132c80a234059714c32faa6469391e59349954749fc86c4ef0b18d5fd99bf8f4b7b82d9f799943799c1253272ae --- src/rpc/util.cpp | 15 ++++++++------- src/rpc/util.h | 9 +++++---- src/util/strencodings.h | 1 - 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 02be3a9b80a4..ec27f6ab5be8 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -18,6 +18,7 @@ #include #include +#include const std::string UNIX_EPOCH_TIME = "UNIX epoch time"; const std::string EXAMPLE_ADDRESS[2] = {"XunLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPw0", "XwQQkwA4FYkq2XERzMY2CiAZhJTEDAbtc0"}; @@ -95,29 +96,29 @@ CAmount AmountFromValue(const UniValue& value, int decimals) return amount; } -uint256 ParseHashV(const UniValue& v, std::string strName) +uint256 ParseHashV(const UniValue& v, std::string_view name) { const std::string& strHex(v.get_str()); if (64 != strHex.length()) - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", strName, 64, strHex.length(), strHex)); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", name, 64, strHex.length(), strHex)); if (!IsHex(strHex)) // Note: IsHex("") is false - throw JSONRPCError(RPC_INVALID_PARAMETER, strName+" must be hexadecimal string (not '"+strHex+"')"); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be hexadecimal string (not '%s')", name, strHex)); return uint256S(strHex); } -uint256 ParseHashO(const UniValue& o, std::string strKey) +uint256 ParseHashO(const UniValue& o, std::string_view strKey) { return ParseHashV(o.find_value(strKey), strKey); } -std::vector ParseHexV(const UniValue& v, std::string strName) +std::vector ParseHexV(const UniValue& v, std::string_view name) { std::string strHex; if (v.isStr()) strHex = v.get_str(); if (!IsHex(strHex)) - throw JSONRPCError(RPC_INVALID_PARAMETER, strName+" must be hexadecimal string (not '"+strHex+"')"); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be hexadecimal string (not '%s')", name, strHex)); return ParseHex(strHex); } -std::vector ParseHexO(const UniValue& o, std::string strKey) +std::vector ParseHexO(const UniValue& o, std::string_view strKey) { return ParseHexV(o.find_value(strKey), strKey); } diff --git a/src/rpc/util.h b/src/rpc/util.h index 5e113e992c13..3bdd9c2c34b6 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -28,6 +28,7 @@ #include #include #include +#include #include #include @@ -103,10 +104,10 @@ void RPCTypeCheckObj(const UniValue& o, * Utilities: convert hex-encoded Values * (throws error if not hex). */ -uint256 ParseHashV(const UniValue& v, std::string strName); -uint256 ParseHashO(const UniValue& o, std::string strKey); -std::vector ParseHexV(const UniValue& v, std::string strName); -std::vector ParseHexO(const UniValue& o, std::string strKey); +uint256 ParseHashV(const UniValue& v, std::string_view name); +uint256 ParseHashO(const UniValue& o, std::string_view strKey); +std::vector ParseHexV(const UniValue& v, std::string_view name); +std::vector ParseHexO(const UniValue& o, std::string_view strKey); bool ParseBoolV(const UniValue& v, const std::string &strName); diff --git a/src/util/strencodings.h b/src/util/strencodings.h index 322efe2b3142..0f01f8090bba 100644 --- a/src/util/strencodings.h +++ b/src/util/strencodings.h @@ -260,7 +260,6 @@ bool TimingResistantEqual(const T& a, const T& b) } /** Parse number as fixed point according to JSON number syntax. - * See https://json.org/number.gif * @returns true on success, false on error. * @note The result must be in the range (-10^18,10^18), otherwise an overflow error will trigger. */ From 062b3c5c40d5816b71a84199c7712448e074497a Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 7 Nov 2023 16:53:37 -0500 Subject: [PATCH 2/2] Merge bitcoin/bitcoin#28464: net: improve max-connection limits code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit df69b22f2e3cc03764a582f29a16a36114f67e17 doc: improve documentation around connection limit maximums (Amiti Uttarwar) adc171edf45ec90857d990b8ec570f3c8c2242b7 scripted-diff: Rename connection limit variables (Amiti Uttarwar) e9fd9c0225527ec7727d2a7ccbdf028784aadc6c net: add m_max_inbound to connman (Amiti Uttarwar) c25e0e05550426f29d79571368d90f63fb472b02 net, refactor: move calculations for connection type limits into connman (Amiti Uttarwar) Pull request description: This is joint work with amitiuttarwar. This has the first few commits of #28463. It is not strictly a prerequisite for that, but has changes that in our opinion make sense on their own. It improves the handling of maximum numbers for different connection types (that are set during init and don’t change after) by: * moving all calculations into one place, `CConnMan::Init()`. Before, they were dispersed between `Init`, `CConnman::Init` and other parts of `CConnman`, resulting in some duplicated test code. * removing the possibility of having a negative maximum of inbound connections, which is hard to argue about * renaming of variables and doc improvements ACKs for top commit: amitiuttarwar: co-author review ACK df69b22f2e3cc03764a582f29a16a36114f67e17 naumenkogs: ACK df69b22f2e3cc03764a582f29a16a36114f67e17 achow101: ACK df69b22f2e3cc03764a582f29a16a36114f67e17 Tree-SHA512: 913d56136bc1df739978de50db67302f88bac2a9d34748ae96763288d97093e998fc0f94f9b6eff12867712d7e86225af6128f4170bf2b5b8ab76f024870a22c --- src/init.cpp | 10 +++----- src/net.cpp | 17 +++++++------ src/net.h | 38 ++++++++++++++++++------------ src/test/denialofservice_tests.cpp | 8 ++----- 4 files changed, 36 insertions(+), 37 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 5998fa3a03d5..b898096afce1 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -631,7 +631,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 (temporary service connections excluded) (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 (temporary service connections excluded) (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); @@ -2619,12 +2619,8 @@ 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.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.nMaxConnections / 2); - connOptions.nMaxAddnode = MAX_ADDNODE_CONNECTIONS; - connOptions.nMaxFeeler = MAX_FEELER_CONNECTIONS; + connOptions.m_max_outbound_onion = std::min(MAX_DESIRED_ONION_CONNECTIONS, connOptions.m_max_automatic_connections / 2); + 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 4b8d3420a94d..281340a3a9a1 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1880,7 +1880,6 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, { int nInbound = 0; int nVerifiedInboundMasternodes = 0; - int nMaxInbound = nMaxConnections - m_max_outbound; AddWhitelistPermissionFlags(permission_flags, addr); if (NetPermissions::HasFlag(permission_flags, NetPermissionFlags::Implicit)) { @@ -1939,7 +1938,7 @@ 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_NETCONN, "connection from %s dropped (discouraged)\n", addr.ToStringAddrPort()); return; @@ -1950,7 +1949,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, // We don't evict verified MN connections and also don't take them into account when checking limits. We can do this // because we know that such connections are naturally limited by the total number of MNs, so this is not usable // for attacks. - while (nInbound - nVerifiedInboundMasternodes >= nMaxInbound) + while (nInbound - nVerifiedInboundMasternodes >= m_max_inbound) { if (!AttemptToEvictConnection()) { // No connection to evict, disconnect the new connection @@ -3284,7 +3283,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe // 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); @@ -4027,11 +4026,11 @@ bool CConnman::Start(CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_met 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); } // @@ -4140,13 +4139,13 @@ void CConnman::Interrupt() InterruptSocks5(true); if (semOutbound) { - for (int i=0; ipost(); } } if (semAddnode) { - for (int i=0; ipost(); } } @@ -4506,7 +4505,7 @@ std::map CConnman::getNetLocalAddresses() const size_t CConnman::GetMaxOutboundNodeCount() { - return m_max_outbound; + return m_max_automatic_outbound; } size_t CConnman::GetMaxOutboundOnionNodeCount() diff --git a/src/net.h b/src/net.h index 0cf55621919c..e4b9a0e284d1 100644 --- a/src/net.h +++ b/src/net.h @@ -1198,12 +1198,8 @@ friend class CNode; struct Options { ServiceFlags nLocalServices = NODE_NONE; - int nMaxConnections = 0; - int m_max_outbound_full_relay = 0; - int m_max_outbound_block_relay = 0; int m_max_outbound_onion = 0; - int nMaxAddnode = 0; - int nMaxFeeler = 0; + int m_max_automatic_connections = 0; CClientUIInterface* uiInterface = nullptr; NetEventsInterface* m_msgproc = nullptr; BanMan* m_banman = nullptr; @@ -1232,14 +1228,13 @@ friend class CNode; AssertLockNotHeld(m_total_bytes_sent_mutex); 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_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_max_outbound_onion = connOptions.m_max_outbound_onion; m_use_addrman_outgoing = connOptions.m_use_addrman_outgoing; - nMaxAddnode = connOptions.nMaxAddnode; - nMaxFeeler = connOptions.nMaxFeeler; - m_max_outbound = m_max_outbound_full_relay + m_max_outbound_block_relay + nMaxFeeler; m_client_interface = connOptions.uiInterface; m_banman = connOptions.m_banman; m_msgproc = connOptions.m_msgproc; @@ -1844,7 +1839,18 @@ friend class CNode; std::unique_ptr semOutbound; std::unique_ptr semAddnode; - int nMaxConnections; + + /** + * 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; @@ -1856,9 +1862,11 @@ friend class CNode; // How many onion outbound peers we want; don't care if full or block only; does not increase m_max_outbound int m_max_outbound_onion; - int nMaxAddnode; - int nMaxFeeler; - 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; NetEventsInterface* m_msgproc; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index d14407bed6cf..2cb85865088c 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -150,9 +150,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_outbound_full_relay = max_outbound_full_relay; - options.nMaxFeeler = MAX_FEELER_CONNECTIONS; + options.m_max_automatic_connections = DEFAULT_MAX_PEER_CONNECTIONS; const auto time_init{GetTime()}; SetMockTime(time_init); @@ -252,9 +250,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_outbound_full_relay = MAX_OUTBOUND_FULL_RELAY_CONNECTIONS; - options.m_max_outbound_block_relay = max_outbound_block_relay; + options.m_max_automatic_connections = DEFAULT_MAX_PEER_CONNECTIONS; connman->Init(options); std::vector vNodes;