Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

net: improve max-connection limits code #28464

Merged
merged 4 commits into from
Nov 7, 2023
Merged

net: improve max-connection limits code #28464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c25e0e0
net, refactor: move calculations for connection type limits into connman
amitiuttarwar Aug 31, 2023
e9fd9c0
net: add m_max_inbound to connman
amitiuttarwar Aug 31, 2023
adc171e
scripted-diff: Rename connection limit variables
amitiuttarwar Aug 30, 2023
df69b22
doc: improve documentation around connection limit maximums
amitiuttarwar Aug 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 2 additions & 6 deletions src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=<n>", strprintf("Maintain at most <n> 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=<n>", strprintf("Maintain at most <n> 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

df69b22

have you considered changing the named argument? -maxconnections -> -maxautoconnections.
It should in that case be backward-compatible (still allow legacy -maxconnections)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot about this comment - I hadn't considered it, and I think I would prefer maxautoconnections, but changing it (even with keeping an alias) seems like too much effort for too little benefit for my taste .

argsman.AddArg("-maxreceivebuffer=<n>", strprintf("Maximum per-connection receive buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXRECEIVEBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
argsman.AddArg("-maxsendbuffer=<n>", strprintf("Maximum per-connection memory usage for the send buffer, <n>*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);
Expand Down Expand Up @@ -1748,11 +1748,7 @@ 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.m_max_automatic_connections = nMaxConnections;
connOptions.uiInterface = &uiInterface;
connOptions.m_banman = node.banman.get();
connOptions.m_msgproc = node.peerman.get();
Expand Down
15 changes: 7 additions & 8 deletions src/net.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1777,7 +1777,6 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
const CAddress& addr)
{
int nInbound = 0;
int nMaxInbound = nMaxConnections - m_max_outbound;

AddWhitelistPermissionFlags(permission_flags, addr);
if (NetPermissions::HasFlag(permission_flags, NetPermissionFlags::Implicit)) {
Expand Down Expand Up @@ -1823,13 +1822,13 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& 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
Expand Down Expand Up @@ -2779,7 +2778,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> 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);
Expand Down Expand Up @@ -3249,11 +3248,11 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)

if (semOutbound == nullptr) {
// initialize semaphore
semOutbound = std::make_unique<CSemaphore>(std::min(m_max_outbound, nMaxConnections));
semOutbound = std::make_unique<CSemaphore>(std::min(m_max_automatic_outbound, m_max_automatic_connections));
}
if (semAddnode == nullptr) {
// initialize semaphore
semAddnode = std::make_unique<CSemaphore>(nMaxAddnode);
semAddnode = std::make_unique<CSemaphore>(m_max_addnode);
}

//
Expand Down Expand Up @@ -3335,13 +3334,13 @@ void CConnman::Interrupt()
InterruptSocks5(true);

if (semOutbound) {
for (int i=0; i<m_max_outbound; i++) {
for (int i=0; i<m_max_automatic_outbound; i++) {
semOutbound->post();
}
}

if (semAddnode) {
for (int i=0; i<nMaxAddnode; i++) {
for (int i=0; i<m_max_addnode; i++) {
semAddnode->post();
}
}
Expand Down
38 changes: 23 additions & 15 deletions src/net.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1057,11 +1057,7 @@ class CConnman
struct Options
{
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;
int m_max_automatic_connections = 0;
CClientUIInterface* uiInterface = nullptr;
NetEventsInterface* m_msgproc = nullptr;
BanMan* m_banman = nullptr;
Expand All @@ -1088,13 +1084,12 @@ class CConnman
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_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;
Expand Down Expand Up @@ -1479,7 +1474,18 @@ class CConnman

std::unique_ptr<CSemaphore> semOutbound;
std::unique_ptr<CSemaphore> 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;
Expand All @@ -1488,9 +1494,11 @@ class CConnman
// We do not relay tx or addr messages with these peers
int m_max_outbound_block_relay;

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;
Expand Down
8 changes: 2 additions & 6 deletions src/test/denialofservice_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +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_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<std::chrono::seconds>()};
SetMockTime(time_init);
Expand Down Expand Up @@ -248,9 +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_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<CNode*> vNodes;
Expand Down