Introduce SockMan (“lite”): low-level socket handling for HTTP #32747

pull pinheadmz wants to merge 10 commits into bitcoin:master from pinheadmz:sockman-lite changing 10 files +919 −14
  1. pinheadmz commented at 6:36 pm on June 13, 2025: member

    Introduces a new low-level socket manager SockMan as an abstract class with virtual functions for implementing higher-level networking protocols like HTTP. This is the next step in #31194

    This is a minimal, alternative version of #30988 (“Split CConnman”) without any changes to working code (P2P is not affected). It adds a stripped-down version of the SockMan introduced in that pull request that implements only what is needed for the HTTP server implemented in #32061 (i.e. no I2P stuff and for now, no outbound connection stuff). Exclusions from the original SockMan pull request can be checked with:

    0git diff vasild/sockman \
1src/common/sockman.h \
2src/common/sockman.cpp

    The commit order has been flipped quite a bit because the original PR incrementally pulls logic out of net wheras this PR builds a new system from the bottom-up. Otherwise I tried to keep all the SockMan code in order so reviewers of the original PR would be familiar with it.

    It also adds unit tests by introducing a SocketTestingSetup which mocks network sockets by returning a queue of DynSock from CreateSock(). HTTP server tests in #32061 will be rebased on this framework as well.

  2. DrahtBot commented at 6:36 pm on June 13, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32747.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK vasild
    Concept ACK w0xlt

    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:

    • #32015 (net: replace manual reference counting of CNode with shared_ptr by vasild)
    • #30988 (Split CConnman by vasild)
    • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
    • #26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)

    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.

    LLM Linter (✨ experimental)

    Possible typos and grammar issues:

    • “an unique id” → “a unique id” [“unique” begins with a consonant sound, so “a” is correct]
    • “response datasent to it” → “response data sent to it” [missing space between “data” and “sent”]

    drahtbot_id_4_m

  3. pinheadmz force-pushed on Jun 13, 2025
  4. DrahtBot added the label CI failed on Jun 13, 2025
  5. DrahtBot commented at 7:03 pm on June 13, 2025: contributor

    🚧 At least one of the CI tasks failed. Task MSan, depends: https://github.com/bitcoin/bitcoin/runs/44070031695 LLM reason (✨ experimental): MemorySanitizer detected use of uninitialized memory during sockman_tests, causing CI failure.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  6. pinheadmz force-pushed on Jun 13, 2025
  7. pinheadmz force-pushed on Jun 13, 2025
  8. pinheadmz force-pushed on Jun 14, 2025
  9. DrahtBot removed the label CI failed on Jun 14, 2025
  10. Sjors commented at 8:21 am on June 16, 2025: member
    I was able to switch https://github.com/Sjors/bitcoin/pull/50 to use this instead of #30988 without have to change anything. So it makes sense to me to focus on this PR first, as it’s smaller. Thoughts @vasild?
  11. w0xlt commented at 11:19 pm on June 17, 2025: contributor
    Concept ACK. Are the functions in this PR to replace Connam functions like CConnman::BindListenPort, CConnman::InitBinds, etc…?
  12. pinheadmz commented at 10:04 am on June 18, 2025: member

    replace Connman functions

    In this PR they are just protocol-agnostic copies of those ConnMan functions. Just the socket stuff without the Bitcoin stuff. We could still proceed with the reactor in #30988 after this is merged and actually replace the socket stuff in ConnMan with SockMan but this PR is minimized with the focus being the socket needs of the HTTP server in #32061

  13. in src/test/sockman_tests.cpp:86 in 5f7941187a outdated 
    85+    };
86+
87+    TestSockMan sockman;
88+
89+    // This address won't actually get used because we stubbed CreateSock()
90+    const std::optional<CService> addr_bind{Lookup("0.0.0.0", 0, false)};
    vasild commented at 10:15 am on June 20, 2025:

    9299d5dbc5 SockMan: introduce class and implement binding to listening socket

    nit, ensure Lookup() succeeded before continuing because below addr_bind.value() is used unconditionally:

    0BOOST_REQUIRE(addr.has_value());
    pinheadmz commented at 6:18 pm on June 23, 2025:
    👍
  14. in src/common/sockman.cpp:29 in 5f7941187a outdated 
    24+        addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind, sockaddr_bind_len);
25+    } else {
26+        LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n");
27+    }
28+    return addr_bind;
29+}
    vasild commented at 12:14 pm on June 20, 2025:
    GetBindAddress() is the same as in net.cpp. It is nice to have this PR remove 0 lines, but I think it is better to make an exception and move the function from net.cpp to netbase.{h,cpp} and use that from both net.cpp and common/sockman.cpp.
    pinheadmz commented at 6:32 pm on June 23, 2025:
    Oh yes thanks, done by inserting a move-only commit
  15. pinheadmz force-pushed on Jun 21, 2025
  16. in src/common/sockman.cpp:253 in 130708803d outdated 
    262+
263+        // Service (send/receive) each of the already connected sockets.
264+        SocketHandlerConnected(io_readiness);
265+
266+        // Accept new connections from listening sockets.
267+        SocketHandlerListening(io_readiness.events_per_sock);
    vasild commented at 9:48 am on June 23, 2025:

    674a5ff8f1 SockMan: handle connected sockets: read data from socket

    In the commit message: s/conencts/connects/

    pinheadmz commented at 6:47 pm on June 23, 2025:
    👍
  17. in src/test/sockman_tests.cpp:26 in 130708803d outdated 
    21+        std::vector<std::pair<Id, CService>> m_connections;
22+
23+        // Received data is written here by the SockMan I/O thread
24+        // and tested by the main thread.
25+        Mutex m_received_mutex;
26+        std::vector<uint8_t> m_received;
    vasild commented at 9:57 am on June 23, 2025:

    674a5ff8f1 SockMan: handle connected sockets: read data from socket

    m_received would better be per-client:

    0std::unordered_map<Id, std::vector<uint8_t>> m_received;

    and then adjust EventGotData() to plug the data in the client’s slot:

    0m_received[id].assign(data.begin(), data.end());

    and GetReceivedData() to get the data for the client:

    0GetReceivedData(Id id)
1{
2    return m_received[id];
3}
    pinheadmz commented at 6:47 pm on June 23, 2025:
    Good catch, taken. I was hoping to get away with only ever using one client in this test but this makes more sense for coverage anyway ;-)
  18. in src/common/sockman.cpp:177 in 130708803d outdated 
    186+{
187+    LOCK(m_connected_mutex);
188+    return m_connected.erase(id) > 0;
189+}
190+
191+ssize_t SockMan::SendBytes(Id id,
    vasild commented at 10:02 am on June 23, 2025:

    3f796b69c8 SockMan: handle connected sockets: write data to socket

    In the commit message: s/recevied/received/

    pinheadmz commented at 6:51 pm on June 23, 2025:
    👍
  19. vasild commented at 10:09 am on June 23, 2025: contributor
    Approach ACK 5f7941187a12f6d1d180ee29b72b2a5ee7a578b8
  20. SockMan: introduce class and implement binding to listening socket 
    Introduce a new low-level socket managing class `SockMan`.
Unit-test it with a new class `SocketTestingSetup` which mocks
`CreateSock()` and will enable mock client I/O in future commits.

`SockMan` and `SocketTestingSetup` are designed to be generic and
reusbale for higher-level network protocol implementation and testing.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    535daaf15f
  21. style: modernize the style of SockMan::BindListenPort() 
    It was copied verbatim from `CConnman::BindListenPort()` in the previous
commit. Modernize its variables and style and log the error messages
from the caller. Also categorize the informative messages to the "net"
category because they are quite specific to the networking layer.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    32c57d0843
  22. SockMan: implement and test AcceptConnection() 
    Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    3f9d8691a0
  23. style: modernize the style of SockMan::AcceptConnection() 5e0e1114b8
  24. SockMan: generate sequential Ids for each newly accepted connection 
    Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    939c2220e8
  25. [move-only] Make GetBindAddress() callable from outside net.cpp 935332c47c
  26. SockMan: start an I/O loop in a new thread and accept connections 
    Socket handling methods are copied from CConnMan:

`CConnman::GenerateWaitSockets()` goes to
`SockMan::GenerateWaitSockets()`.

`CConnman::ThreadSocketHandler()` and
`CConnman::SocketHandler()` are combined into
`SockMan::ThreadSocketHandler()`.

`CConnman::SocketHandlerListening()` goes to
`SockMan::SocketHandlerListening()`.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    a1e847d68d
  27. SockMan: handle connected sockets: read data from socket 
    `CConnman::SocketHandlerConnected()` copied to
`SockMan::SocketHandlerConnected()`.

Testing this requires adding a new feature to the SocketTestingSetup,
inserting a "request" payload into the mock client that connects
to us.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    d43dc9320c
  28. SockMan: handle connected sockets: write data to socket 
    Sockets-touching bits from `CConnman::SocketSendData()` copied to
`SockMan::SendBytes()`.

Testing this requires adding a new feature to the SocketTestingSetup,
returning the DynSock I/O pipes from the mock socket so the received
data can be checked.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    2e87b097a3
  29. SockMan: dispatch cyclical events from I/O loop 
    Copy from some parts of `CConnman::SocketHandlerConnected()` and
`CConnman::ThreadSocketHandler()` to:
`EventIOLoopCompletedForOne(id)` and `EventIOLoopCompletedForAll()`.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    3e7abceecf
  30. pinheadmz force-pushed on Jun 23, 2025
  31. pinheadmz commented at 0:43 am on June 24, 2025: member
    Rebase to address review by @vasild THANKS!
  32. vasild approved
  33. vasild commented at 3:57 pm on June 25, 2025: contributor
    ACK 3e7abceecfd790bc0887f647d3f731328e19810f
  34. DrahtBot requested review from w0xlt on Jun 25, 2025
  35. pinheadmz removed review request from w0xlt on Jul 2, 2025
  36. pinheadmz requested review from Sjors on Jul 2, 2025


pinheadmz DrahtBot Sjors w0xlt vasild


Sjors

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: 2025-07-09 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me