net: improve max-connection limits code #28464

pull mzumsande wants to merge 4 commits into bitcoin:master from mzumsande:202308_refactor_limits changing 4 files +34 โˆ’35
  1. 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
  2. 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.

  3. DrahtBot added the label P2P on Sep 12, 2023
  4. mzumsande marked this as a draft on Sep 12, 2023
  5. mzumsande marked this as ready for review on Sep 12, 2023
  6. 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).
  7. 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.

  8. DrahtBot added the label CI failed on Sep 21, 2023
  9. DrahtBot removed the label CI failed on Sep 23, 2023
  10. DrahtBot added the label CI failed on Sep 24, 2023
  11. DrahtBot removed the label CI failed on Sep 27, 2023
  12. DrahtBot added the label Needs rebase on Oct 3, 2023
  13. 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
  14. 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
  15. 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
  16. doc: improve documentation around connection limit maximums
    Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
    df69b22f2e
  17. mzumsande force-pushed on Oct 3, 2023
  18. mzumsande commented at 6:44 pm on October 3, 2023: contributor
    96904e8 to df69b22: Rebased, and changed a comment slightly.
  19. DrahtBot removed the label Needs rebase on Oct 3, 2023
  20. DrahtBot added the label CI failed on Oct 4, 2023
  21. DrahtBot removed the label CI failed on Oct 5, 2023
  22. 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

  23. 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 .
  24. naumenkogs commented at 8:18 am on October 18, 2023: member
    ACK df69b22f2e3cc03764a582f29a16a36114f67e17
  25. DrahtBot requested review from amitiuttarwar on Oct 18, 2023
  26. DrahtBot removed review request from amitiuttarwar on Oct 18, 2023
  27. achow101 commented at 9:53 pm on November 7, 2023: member
    ACK df69b22f2e3cc03764a582f29a16a36114f67e17
  28. achow101 merged this on Nov 7, 2023
  29. achow101 closed this on Nov 7, 2023

  30. mzumsande deleted the branch on Mar 20, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-11 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me