net: improve max-connection limits code

mzumsande commented at 8:47 pm on September 12, 2023: contributor

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

DrahtBot commented at 8:47 pm on September 12, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	naumenkogs, amitiuttarwar, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27600 (net: Add new permission forceinbound to evict a random unprotected connection if all slots are otherwise full by pinheadmz)
#23352 (test: Extend stale_tip_peer_management test by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label P2P on Sep 12, 2023

mzumsande marked this as a draft on Sep 12, 2023

mzumsande marked this as ready for review on Sep 12, 2023

in src/net.h:1204 in 96904e8df0 outdated

1196@@ -1197,8 +1197,19 @@ class CConnman
1197 
1198     std::unique_ptr<CSemaphore> semOutbound;
1199     std::unique_ptr<CSemaphore> semAddnode;
1200+
1201+    /**
1202+     * Maximum number of automatic connections permitted, excluding manual
1203+     * connections but including inbounds. May be changed by the user and is
1204+     * potentially limited by the number of available file descriptors.

naumenkogs commented at 7:51 am on September 13, 2023:

96904e8df098ba35cc5b9b8a089e472f729c0b6a

nit: Perhaps I’d be more generic saying something-something operating system limits

mzumsande commented at 6:43 pm on October 3, 2023:

I’ve mentioned both reasons now (the generic and the specific one).

naumenkogs commented at 7:53 am on September 13, 2023: member

ACK 96904e8df098ba35cc5b9b8a089e472f729c0b6a

nice catching the negative number. I haven’t found a way to exploit this bug.

DrahtBot added the label CI failed on Sep 21, 2023

DrahtBot removed the label CI failed on Sep 23, 2023

DrahtBot added the label CI failed on Sep 24, 2023

DrahtBot removed the label CI failed on Sep 27, 2023

DrahtBot added the label Needs rebase on Oct 3, 2023

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 <mzumsande@gmail.com>

c25e0e0555

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 <mzumsande@gmail.com>

e9fd9c0225

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 <mzumsande@gmail.com>

adc171edf4

doc: improve documentation around connection limit maximums

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

df69b22f2e

mzumsande force-pushed on Oct 3, 2023

mzumsande commented at 6:44 pm on October 3, 2023: contributor

96904e8 to df69b22: Rebased, and changed a comment slightly.

DrahtBot removed the label Needs rebase on Oct 3, 2023

DrahtBot added the label CI failed on Oct 4, 2023

DrahtBot removed the label CI failed on Oct 5, 2023

amitiuttarwar commented at 0:33 am on October 18, 2023: contributor

co-author review ACK df69b22f2e3cc03764a582f29a16a36114f67e17

I think the windows failure is the known issue from #28509 that should be fixed post #28567

in src/init.cpp:492 in df69b22f2e

488@@ -489,7 +489,7 @@ void SetupServerArgs(ArgsManager& argsman)
489     argsman.AddArg("-forcednsseed", strprintf("Always query for peer addresses via DNS lookup (default: %u)", DEFAULT_FORCEDNSSEED), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
490     argsman.AddArg("-listen", strprintf("Accept connections from outside (default: %u if no -proxy, -connect or -maxconnections=0)", DEFAULT_LISTEN), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
491     argsman.AddArg("-listenonion", strprintf("Automatically create Tor onion service (default: %d)", DEFAULT_LISTEN_ONION), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
492-    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);
493+    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);

naumenkogs commented at 8:17 am on October 18, 2023:

df69b22f2e3cc03764a582f29a16a36114f67e17

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

mzumsande commented at 10:07 pm on November 7, 2023:

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 .

naumenkogs commented at 8:18 am on October 18, 2023: member

ACK df69b22f2e3cc03764a582f29a16a36114f67e17

DrahtBot requested review from amitiuttarwar on Oct 18, 2023

DrahtBot removed review request from amitiuttarwar on Oct 18, 2023

achow101 commented at 9:53 pm on November 7, 2023: member

ACK df69b22f2e3cc03764a582f29a16a36114f67e17

achow101 merged this on Nov 7, 2023

achow101 closed this on Nov 7, 2023

mzumsande deleted the branch on Mar 20, 2024

net: improve max-connection limits code #28464

Code Coverage

Reviews

Conflicts