net: improve max-connection limits code #28464
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
src/net.h
Outdated
| /** | ||
| * Maximum number of automatic connections permitted, excluding manual | ||
| * connections but including inbounds. May be changed by the user and is | ||
| * potentially limited by the number of available file descriptors. |
There was a problem hiding this comment.
nit: Perhaps I'd be more generic saying something-something operating system limits
There was a problem hiding this comment.
I've mentioned both reasons now (the generic and the specific one).
|
ACK 96904e8 nice catching the negative number. I haven't found a way to exploit this bug. |
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>
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>
-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>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
mzumsande
force-pushed
the
202308_refactor_limits
branch
from
96904e8
to
df69b22
Compare
| @@ -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); | |||
There was a problem hiding this comment.
have you considered changing the named argument? -maxconnections -> -maxautoconnections.
It should in that case be backward-compatible (still allow legacy -maxconnections)
There was a problem hiding this comment.
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 .
|
ACK df69b22 |
DrahtBot
requested review from
amitiuttarwar
and removed request for
amitiuttarwar
|
ACK df69b22 |
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:
CConnMan::Init(). Before, they were dispersed betweenInit,CConnman::Initand other parts ofCConnman, resulting in some duplicated test code.