Introduce SockMan ("lite"): low-level socket handling for HTTP

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.

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, hodlinator

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:

#33454 (net: support overriding the proxy selection in ConnectNode() by vasild)
#33210 (fuzz: enhance wallet_fees by mocking mempool stuff by brunoerg)
#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:

“Each connection is assigned an unique id of this type.” -> “a unique id” [use “a” before words starting with a consonant sound; “an unique” is grammatically incorrect]
“Returns the I/O pipes from the mock client so we can read response datasent to it.” -> “response data sent” [missing space between “data” and “sent”, making the phrase hard to parse]

drahtbot_id_5_m

pinheadmz force-pushed on Jun 13, 2025

DrahtBot added the label CI failed on Jun 13, 2025

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.

pinheadmz force-pushed on Jun 13, 2025

pinheadmz force-pushed on Jun 14, 2025

DrahtBot removed the label CI failed on Jun 14, 2025

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?

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…?

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

in src/test/sockman_tests.cpp:94 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:

👍

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

pinheadmz force-pushed on Jun 21, 2025

in src/common/sockman.cpp:256 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:

👍

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 ;-)

in src/common/sockman.cpp:123 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:

👍

vasild commented at 10:09 am on June 23, 2025: contributor

Approach ACK 5f7941187a12f6d1d180ee29b72b2a5ee7a578b8

pinheadmz force-pushed on Jun 23, 2025

pinheadmz commented at 0:43 am on June 24, 2025: member

Rebase to address review by @vasild THANKS!

vasild approved

vasild commented at 3:57 pm on June 25, 2025: contributor

ACK 3e7abceecfd790bc0887f647d3f731328e19810f

DrahtBot requested review from w0xlt on Jun 25, 2025

pinheadmz removed review request from w0xlt on Jul 2, 2025

pinheadmz requested review from Sjors on Jul 2, 2025

pinheadmz requested review from hodlinator on Jul 15, 2025

in src/test/util/setup_common.h:19 in 3e7abceecf outdated

16@@ -17,6 +17,7 @@
17 #include <pubkey.h>
18 #include <stdexcept>
19 #include <test/util/random.h>
20+#include <test/util/net.h>

jonatack commented at 10:24 pm on July 23, 2025:

535daaf15fd754335116d17833a45261cdff4e93 nit, sort

pinheadmz commented at 3:26 pm on August 7, 2025:

fixed, thanks

in src/test/sockman_tests.cpp:1 in 3e7abceecf outdated

0@@ -0,0 +1,152 @@
1+// Copyright (c) 2021-2022 The Bitcoin Core developers

jonatack commented at 10:25 pm on July 23, 2025:

535daaf15fd754335116d17833a45261cdff4e93 (edit, or is this due to the code already existing, if yes, please mention this in the commit message)

0// Copyright (c) 2025-present The Bitcoin Core developers

pinheadmz commented at 3:34 pm on August 7, 2025:

Thanks, removed the years entirely which seems to be the style for new files.

jonatack commented at 10:42 pm on July 23, 2025: member

Began reviewing, then realized that I was reviewing the same code twice with the two “modernize” commits following “original” commits.

Propose to either (1) make clear in the commit message where the code is being pulled from, and that it will be re-styled afterward, or (2) squash the modernization commits into the previous ones. Thereby easing life a bit for your reviewers :)

DrahtBot requested review from w0xlt on Jul 23, 2025

pinheadmz force-pushed on Aug 7, 2025

pinheadmz commented at 3:35 pm on August 7, 2025: member

3e7abceecf -> 598bee6bd5

Address nits from @jonatack including commit messages that indicate code is copied from CConnman and will be modernized in following commits.

in src/common/sockman.h:128 in 598bee6bd5 outdated

123+    CThreadInterrupt interruptNet;
124+
125+    /**
126+     * List of listening sockets.
127+     */
128+    std::vector<std::shared_ptr<Sock>> m_listen;

hodlinator commented at 2:33 pm on August 20, 2025:

Could we avoid public data in the initial version?

0    void InterruptNet() { interruptNet(); }
1
2    const std::vector<std::shared_ptr<Sock>>& ListenSockets() const { return m_listen; }

Alternatively make them protected and expose them in TestSockMan.

pinheadmz commented at 5:07 pm on September 12, 2025:

Yes will make these data private/protected and add public helpers.

in src/test/sockman_tests.cpp:26 in 598bee6bd5 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::unordered_map<Id, std::vector<uint8_t>> m_received;

hodlinator commented at 6:39 pm on August 20, 2025:

Might as well:

0        std::vector<std::pair<Id, CService>> m_connections GUARDED_BY(m_connections_mutex);
1
2        // Received data is written here by the SockMan I/O thread
3        // and tested by the main thread.
4        Mutex m_received_mutex;
5        std::unordered_map<Id, std::vector<uint8_t>> m_received GUARDED_BY(m_received_mutex);

pinheadmz commented at 6:26 pm on September 12, 2025:

👍

in src/test/sockman_tests.cpp:66 in 598bee6bd5 outdated

61+        {
62+            LOCK(m_received_mutex);
63+            m_received[id].assign(data.begin(), data.end());
64+        };
65+        virtual void EventGotEOF(Id id) override {};
66+        virtual void EventGotPermanentReadError(Id id, const std::string& errmsg) override {};

hodlinator commented at 6:42 pm on August 20, 2025:

nit:

0        }
1        virtual void EventGotEOF(Id id) override {}
2        virtual void EventGotPermanentReadError(Id id, const std::string& errmsg) override {}

pinheadmz commented at 7:07 pm on September 12, 2025:

👍

in src/test/sockman_tests.cpp:88 in 598bee6bd5 outdated

83+    TestSockMan sockman;
84+
85+    // This address won't actually get used because we stubbed CreateSock()
86+    const std::optional<CService> addr_bind{Lookup("0.0.0.0", 0, false)};
87+    BOOST_REQUIRE(addr_bind.has_value());
88+    bilingual_str strError;

hodlinator commented at 6:44 pm on August 20, 2025:

nits:

Should be snake_case.
Could be put with the block that uses it.

pinheadmz commented at 5:27 pm on September 12, 2025:

done

in src/test/util/setup_common.h:282 in 598bee6bd5 outdated

277+    explicit SocketTestingSetup();
278+    ~SocketTestingSetup();
279+
280+    // Connect to the socket with a mock client (a DynSock) and send pre-loaded data.
281+    // Returns the I/O pipes from the mock client so we can read response datasent to it.
282+    std::shared_ptr<DynSock::Pipes> ConnectClient(const std::vector<uint8_t>& data);

hodlinator commented at 6:53 pm on August 20, 2025:

More modern:

0    std::shared_ptr<DynSock::Pipes> ConnectClient(std::span<const std::byte> data);

pinheadmz commented at 7:11 pm on September 12, 2025:

i like it thanks

in src/test/sockman_tests.cpp:134 in 598bee6bd5 outdated

129+    attempts = 6000;
130+    size_t expected_response_size = sockman.m_respond.size();
131+    std::vector<uint8_t> actually_received(expected_response_size);
132+    while (!std::ranges::equal(actually_received, sockman.m_respond)) {
133+        // Read the data received by the mock socket
134+        ssize_t bytes_read = pipes->send.GetBytes((void *)actually_received.data(), expected_response_size);

hodlinator commented at 7:21 pm on August 20, 2025:

nit: No need to cast to void*:

0        ssize_t bytes_read = pipes->send.GetBytes(actually_received.data(), expected_response_size);

hodlinator commented at 9:04 am on August 26, 2025:

Implemented simulating partial transfer of stream in 6a74e1e9a7393ad51d7a1590384c8a6771b04972 (requires parent commit).

pinheadmz commented at 8:04 pm on September 12, 2025:

you’re right thanks

pinheadmz commented at 7:25 pm on September 15, 2025:

see above re: packets and chunks

in src/common/sockman.cpp:1 in 598bee6bd5 outdated

0@@ -0,0 +1,372 @@
1+// Copyright (c) 2024-present The Bitcoin Core developers

hodlinator commented at 9:39 am on August 22, 2025:

nit: Maybe update/remove year? (Not a copyright lawyer).

pinheadmz commented at 5:29 pm on September 12, 2025:

You’re right for new files we’re removing the year(s) entirely.

in src/common/sockman.h:303 in 598bee6bd5 outdated

298+     */
299+    std::shared_ptr<ConnectionSockets> GetConnectionSockets(Id id) const
300+        EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex);
301+
302+    /**
303+     * The id to assign to the next created connection. Used to generate ids of connections.

hodlinator commented at 9:44 am on August 22, 2025:

nit:

0     * Used to generate ids for new connections.

pinheadmz commented at 2:30 pm on September 15, 2025:

I guess the two sentences are a bit redundant, I’m going to keep the first and lose the second, though.

in src/common/sockman.h:34 in 598bee6bd5 outdated

29+    /**
30+     * Each connection is assigned an unique id of this type.
31+     */
32+    using Id = int64_t;
33+
34+    virtual ~SockMan() = default;

hodlinator commented at 10:11 am on August 22, 2025:

Could assert that various conditions are true:

0    virtual ~SockMan()
1    {
2        assert(!m_thread_socket_handler.joinable()); // Missing call to JoinSocketsThreads()
3        assert(m_connected.empty()); // Missing call to CloseConnection()
4        assert(m_listen.empty()); // Missing call to StopListening()
5    }

https://en.cppreference.com/w/cpp/thread/thread/join.html#Postconditions

pinheadmz commented at 6:58 pm on September 12, 2025:

Ok I’ll add these checks but I’m nervous about assert() so I’m going to use Assume() for now. In this case if there is a broken assumption it will only occur during server shutdown, but I don’t want to terminate the program early in case there is still wallet cleanup scheduled after ~SockMan. Lemme know if you think it should still be assert().

in src/common/sockman.cpp:113 in 598bee6bd5 outdated

108+        m_thread_socket_handler.join();
109+    }
110+}
111+
112+std::unique_ptr<Sock> SockMan::AcceptConnection(const Sock& listen_sock, CService& addr)
113+{

hodlinator commented at 10:19 am on August 22, 2025:

Could verify that we are operating on one of our own listen sockets?

0{
1    Assume(std::ranges::find_if(m_listen, [&](const auto& sock) { return sock.get() == &listen_sock; }) != m_listen.end());

pinheadmz commented at 5:41 pm on September 12, 2025:

Sure taken

in src/common/sockman.cpp:39 in 598bee6bd5 outdated

34+
35+    int one{1};
36+
37+    // Allow binding if the port is still in TIME_WAIT state after
38+    // the program was closed and restarted.
39+    if (sock->SetSockOpt(SOL_SOCKET, SO_REUSEADDR, reinterpret_cast<sockopt_arg_type>(&one), sizeof(one)) == SOCKET_ERROR) {

hodlinator commented at 1:46 pm on August 22, 2025:

nit: Cast can be omitted here and a few lines down for IPV6_V6ONLY and IPV6_PROTECTION_LEVEL:

0    if (sock->SetSockOpt(SOL_SOCKET, SO_REUSEADDR, &one, sizeof(one)) == SOCKET_ERROR) {

pinheadmz commented at 5:35 pm on September 12, 2025:

Good catch see https://github.com/bitcoin/bitcoin/pull/33378

in src/test/sockman_tests.cpp:18 in 72864e4628 outdated

13+BOOST_AUTO_TEST_CASE(test_sockman)
14+{
15+    SockMan sockman;
16+
17+    // This address won't actually get used because we stubbed CreateSock()
18+    const std::optional<CService> addr{Lookup("0.0.0.0", 0, false)};

hodlinator commented at 2:03 pm on August 22, 2025:

nit: Could call this addr_bind from first commit instead of renaming it later.

pinheadmz commented at 5:11 pm on September 12, 2025:

yep thanks

in src/common/sockman.h:88 in 598bee6bd5 outdated

83+        EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex);
84+
85+    /**
86+     * Generate an id for a newly created connection.
87+     */
88+    Id GetNewId();

hodlinator commented at 2:07 pm on August 22, 2025:

Should these 3 not be protected/private?

pinheadmz commented at 6:07 pm on September 12, 2025:

Yeah they can be thanks.

in src/common/sockman.h:222 in 598bee6bd5 outdated

217+    virtual void EventIOLoopCompletedForAll();
218+
219+    /**
220+     * The sockets used by a connection.
221+     */
222+    struct ConnectionSockets {

hodlinator commented at 2:10 pm on August 22, 2025:

What is the reason for plural form?

0    struct ConnectionSocket {

pinheadmz commented at 6:32 pm on September 12, 2025:

I’ll fix, this is leftover from the original SockMan PR #30988 where this struct also included an I2P session.

in src/common/sockman.cpp:203 in 598bee6bd5 outdated

198+    }
199+#endif
200+
201+    const ssize_t sent{WITH_LOCK(
202+        sockets->mutex,
203+        return sockets->sock->Send(reinterpret_cast<const char*>(data.data()), data.size(), flags);)};

hodlinator commented at 2:17 pm on August 22, 2025:

nit: -cast +newline

0        return sockets->sock->Send(data.data(), data.size(), flags);
1    )};

pinheadmz commented at 5:01 pm on September 12, 2025:

Thanks for catching this, opened #33378 to clean up several such casts in existing code since this chunk was essentially copied from net.cpp.

in src/test/sockman_tests.cpp:55 in 598bee6bd5 outdated

46+
47+    private:
48+        virtual bool EventNewConnectionAccepted(Id id,
49+                                            const CService& me,
50+                                            const CService& them) override
51+        EXCLUSIVE_LOCKS_REQUIRED(!m_connections_mutex)

hodlinator commented at 2:29 pm on August 22, 2025:

nit: I liked the style you had in sockman.h

0            EXCLUSIVE_LOCKS_REQUIRED(!m_connections_mutex)

pinheadmz commented at 7:02 pm on September 12, 2025:

🆒

in src/common/sockman.h:110 in 598bee6bd5 outdated

105+     * explaining the error.
106+     * @retval >=0 The number of bytes actually sent.
107+     * @retval <0 A permanent error has occurred.
108+     */
109+    ssize_t SendBytes(Id id,
110+                      std::span<const unsigned char> data,

hodlinator commented at 7:13 am on August 25, 2025:

0                      std::span<const std::byte> data,

pinheadmz commented at 7:49 pm on September 12, 2025:

yes thanks

in src/common/sockman.cpp:332 in 598bee6bd5 outdated

324+                const int err = WSAGetLastError();
325+                if (err != WSAEWOULDBLOCK && err != WSAEMSGSIZE && err != WSAEINTR && err != WSAEINPROGRESS) {
326+                    EventGotPermanentReadError(id, NetworkErrorString(err));
327+                }
328+            } else if (nrecv == 0) {
329+                EventGotEOF(id);

hodlinator commented at 7:15 am on August 25, 2025:

Should we close the connection if we get EOF? See 972ee5cf6544c5c1ea4d445dc9c6fed16136c10c.

pinheadmz commented at 3:19 pm on September 12, 2025:

That’s for the consumer / child class of SockMan to decide!

For example in the HTTP server follow-up to this it’s here:

https://github.com/bitcoin/bitcoin/pull/32061/commits/badf3c38b3b1dab2d209a8e2c709e83afe9e4402#diff-63c8cb9c9dd61d50d59afd5c39914e1c259f8743030b637a7896a0746c851ef1R1208

in src/common/sockman.h:164 in 598bee6bd5 outdated

159+    /**
160+     * Called when new data has been received.
161+     * @param[in] id Connection for which the data arrived.
162+     * @param[in] data Received data.
163+     */
164+    virtual void EventGotData(Id id, std::span<const uint8_t> data) = 0;

hodlinator commented at 7:15 am on August 25, 2025:

0    virtual void EventGotData(Id id, std::span<const std::byte> data) = 0;

pinheadmz commented at 7:37 pm on September 12, 2025:

yes thanks

in src/test/sockman_tests.cpp:63 in 598bee6bd5 outdated

58+        // When we receive data just store it in a member variable for testing.
59+        virtual void EventGotData(Id id, std::span<const uint8_t> data) override
60+        EXCLUSIVE_LOCKS_REQUIRED(!m_received_mutex)
61+        {
62+            LOCK(m_received_mutex);
63+            m_received[id].assign(data.begin(), data.end());

hodlinator commented at 7:29 am on August 25, 2025:

It could be more realistic to account for network stream being broken up:

0            auto& vec{m_received[id]};
1            vec.insert(vec.end(), data.begin(), data.end());

Implemented a slightly tangential change in my suggestions branch to make DynSock[::Pipe] support simulating partial I/O in the vein of TCP/IP streams, see a5e7157563fc9866a75908cef65f7353109b6976.

pinheadmz commented at 7:25 pm on September 15, 2025:

This is cool but I don’t think it’s necessary for this PR. In #32061 there is more robust testing with real sockets (not DynSock) and I think the combining of chunks of data from separate packets is more appropriate one level up (for example in this HTTP server). But I’m also open to integrating this kind of test in the current sockman PR if you think its important

in src/test/util/setup_common.cpp:632 in 598bee6bd5 outdated

627+    // Insert the payload
628+    connected_socket_pipes->recv.PushBytes(data.data(), data.size());
629+
630+    // Create the Mock Connected Socket that represents a client.
631+    // It needs I/O pipes but its queue can remain empty
632+    std::unique_ptr<DynSock> connected_socket{std::make_unique<DynSock>(connected_socket_pipes, std::make_shared<DynSock::Queue>())};

hodlinator commented at 9:03 am on August 26, 2025:

Managed to remove shared_ptr usage internally in DynSock, see commit 7e894f39c41e2ba332aafc5879d7d7380cbcf2fd from my suggestions branch.

pinheadmz commented at 7:23 pm on September 15, 2025:

I’m going to keep the shared_ptr advice under consideration but didn’t apply it to this rebase.

in src/common/sockman.h:123 in 598bee6bd5 outdated

142+     * @retval false The connection was refused at the higher level, so the
143+     * associated socket and id should be discarded by `SockMan`.
144+     */
145+    virtual bool EventNewConnectionAccepted(Id id,
146+                                            const CService& me,
147+                                            const CService& them) = 0;

hodlinator commented at 9:20 am on August 26, 2025:

Could add an EventConnectionClosed() to mirror this? See 0ca4fac1009017cddff9864ec9cac73d613d67f4.

pinheadmz commented at 7:28 pm on September 15, 2025:

No harm in this I suppose but it’s not really needed by the HTTP server, we already log client disconnections when calling CloseConnection()

hodlinator commented at 9:42 am on August 26, 2025: contributor

Concept ACK 598bee6bd590757565d2564ae86cf46b5eea4399

Concept

Agree with exploring this avenue of introducing SockMan without touching CConnman. Given @theuni’s comment in #30988 (comment), I’m still curious how he would suggest improving it as far as event-loop/send-receive/threading goes.

Commit approach

Think it’s fine so far to pair the commits (copy+modernize, …). It enables reviewers to review the copying and then diffing over 2 commits at once to see what changed in combination.

Suggestions branch

https://github.com/bitcoin/bitcoin/compare/598bee6bd590757565d2564ae86cf46b5eea4399...hodlinator:bitcoin:pr/32747_suggestions

Ordered my suggestion commits starting from:

Trivial - Example: use std::byte (44bf80cd90b573d739b3519810449eab6a109992).
Slightly controversial - Example: decreased shared_ptr mentioned ~~above~~below.
More arbitrary - Example: rename ConnectionSockets => ConnectionSocket (650ff1d0762d05b9ea34d527840c5e6c7762afce), probably there are reasons I don’t know for the naming.

Hodlinator’s `shared_ptr` corner

shared_ptr signals heavily ambiguous ownership. It seems like if there is a socket manager, it should be the one controlling the lifetime of all of them. But since the socket code is already so shared_ptr-heavy, it’s more like a global network API with some objects with value-like semantics thrown in. And maybe I can learn to live with that for now. Few socket operations entail looping over a vector of them anyway, which weakens the argument for centralizing ownership. Wrapping them in shared_ptr means they can contain inlined data that is “pinned” to that place in memory with decreased risk of being moved and causing issues (even though that could be trivially worked around through internal pointer indirection).

SockMan: introduce class and implement binding to listening socket

Introduce a new low-level socket managing class `SockMan`.

BindListenPort() is copied from CConnMan in net.cpp and will
be modernized in the next commit.

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>

3d25944c98

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>

6b5b652ac3

SockMan: implement and test AcceptConnection()

AcceptConnection() is mostly copied from CConmann in net.cpp
and will be modernized in the following commit.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

6daed75331

style: modernize the style of SockMan::AcceptConnection() 64b13605dc

SockMan: generate sequential Ids for each newly accepted connection

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

e863a98131

[move-only] Make GetBindAddress() callable from outside net.cpp 531128df9f

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>

8ea7d739be

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>

5136c49dea

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>

2a5e94bcf7

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>

e007e1b57d

pinheadmz force-pushed on Sep 15, 2025

pinheadmz commented at 7:31 pm on September 15, 2025: member

Rebase to e007e1b57d addresses excellent thorough review from @hodlinator (THANKS!). Most significant changes involve casting arguments to socket functions (see #33378) and cleaning up protected/private membership of SockMan. I have locally rebased #32061 on these changes and everything still passes ;-) I will update that PR soon based on these changes.

achow101 referenced this in commit eaf2c46475 on Sep 18, 2025

vasild approved

vasild commented at 1:37 pm on September 29, 2025: contributor

ACK e007e1b57d5d42c2a8d932d5b91eec8a3ca76e14

DrahtBot requested review from hodlinator on Sep 29, 2025

in src/common/sockman.cpp:30 in 6b5b652ac3 outdated

37+                                         to.ToStringAddrPort(),
38+                                         NetworkErrorString(WSAGetLastError())));
39         return false;
40     }
41 
42+    int one{1};

l0rinc commented at 2:40 am on September 30, 2025:

nit: I know this was the original name as well, but variables (by their name) should reflect the purpose of their usage, not their value. ipv6_only{1} or tcp_nodelay{1} might make the context more obvious (if that’s indeed what they’re goal is.

in src/common/sockman.cpp:24 in e007e1b57d

19+    // Create socket for listening for incoming connections
20+    sockaddr_storage storage;
21+    socklen_t len{sizeof(storage)};
22+    if (!to.GetSockAddr(reinterpret_cast<sockaddr*>(&storage), &len)) {
23+        err_msg = Untranslated(strprintf("Bind address family for %s not supported", to.ToStringAddrPort()));
24+        return false;

l0rinc commented at 2:43 am on September 30, 2025:

if error message is tied to a false return value, can we return std::optional<bilingual_str> instead of having both a return value and an output parameter?

0std::optional<bilingual_str> BindAndStartListening(const CService& to)
1{
2    sockaddr_storage storage;
3    socklen_t len{sizeof(storage)};
4    if (!to.GetSockAddr(reinterpret_cast<sockaddr*>(&storage), &len)) {
5        return Untranslated(strprintf("Bind address family for %s not supported", to.ToStringAddrPort()));
6    }
7...

in src/netbase.cpp:955 in e007e1b57d

950+CService GetBindAddress(const Sock& sock)
951+{
952+    CService addr_bind;
953+    struct sockaddr_storage sockaddr_bind;
954+    socklen_t sockaddr_bind_len = sizeof(sockaddr_bind);
955+    if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {

l0rinc commented at 2:54 am on September 30, 2025:

whenreinterpret_cast<sockaddr*>(&storage) as in SockMan::BindAndStartListening and when c-style cast?

in src/common/sockman.cpp:51 in e007e1b57d

46+
47+    // some systems don't have IPV6_V6ONLY but are always v6only; others do have the option
48+    // and enable it by default or not. Try to enable it, if possible.
49+    if (to.IsIPv6()) {
50+#ifdef IPV6_V6ONLY
51+        if (sock->SetSockOpt(IPPROTO_IPV6, IPV6_V6ONLY, &one, sizeof(one)) == SOCKET_ERROR) {

l0rinc commented at 3:06 am on September 30, 2025:

are we testing this code in any way? And can we enable compile and be type-checking and make it if constexpr instead of using macros?

in src/common/sockman.cpp:23 in e007e1b57d

18+{
19+    // Create socket for listening for incoming connections
20+    sockaddr_storage storage;
21+    socklen_t len{sizeof(storage)};
22+    if (!to.GetSockAddr(reinterpret_cast<sockaddr*>(&storage), &len)) {
23+        err_msg = Untranslated(strprintf("Bind address family for %s not supported", to.ToStringAddrPort()));

l0rinc commented at 3:10 am on September 30, 2025:

seems to me we’re not testing any of these unhappy paths - could we do that or is it too much work?

in src/common/sockman.cpp:187 in e007e1b57d

182+        return;
183+    }
184+
185+    // According to the internet TCP_NODELAY is not carried into accepted sockets
186+    // on all platforms.  Set it again here just to be sure.
187+    const int on{1};

l0rinc commented at 3:16 am on September 30, 2025:

what’s the difference between this an the one (without const) above? Can we unify their naming so that they represent the meaning of what they’re storing instead of the value?

in src/common/sockman.cpp:189 in e007e1b57d

184+
185+    // According to the internet TCP_NODELAY is not carried into accepted sockets
186+    // on all platforms.  Set it again here just to be sure.
187+    const int on{1};
188+    if (sock->SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) == SOCKET_ERROR) {
189+        LogDebug(BCLog::NET, "connection from %s: unable to set TCP_NODELAY, continuing anyway\n",

l0rinc commented at 3:18 am on September 30, 2025:

nit: most logs don’t need a trailing newline anymore

in src/common/sockman.cpp:208 in e007e1b57d

203+}
204+
205+std::unique_ptr<Sock> SockMan::AcceptConnection(const Sock& listen_sock, CService& addr)
206+{
207+    // Make sure we only operate on our own listening sockets
208+    Assume(std::ranges::find_if(m_listen, [&](const auto& sock) { return sock.get() == &listen_sock; }) != m_listen.end());

l0rinc commented at 3:19 am on September 30, 2025:

We don’t actually need the value here, just that it exists, right?

0    Assume(std::ranges::any_of(m_listen, [&](const auto& sock) { return sock.get() == &listen_sock; }));

in src/common/sockman.cpp:22 in e007e1b57d

17+bool SockMan::BindAndStartListening(const CService& to, bilingual_str& err_msg)
18+{
19+    // Create socket for listening for incoming connections
20+    sockaddr_storage storage;
21+    socklen_t len{sizeof(storage)};
22+    if (!to.GetSockAddr(reinterpret_cast<sockaddr*>(&storage), &len)) {

l0rinc commented at 3:40 am on September 30, 2025:

I understand that we need different sizes for ipv4 and ipv6, hence these ugly polimorphism-mimicking structures, but I’m wondering if there’s a way to hide this ugliness in the C++ code with either decicated structures or something lke std::variant<sockaddr_in, sockaddr_in6>

in src/common/sockman.cpp:141 in e007e1b57d

136+        // Bail out immediately and just leave things in the caller's send queue.
137+        return 0;
138+    }
139+
140+    int flags{MSG_NOSIGNAL | MSG_DONTWAIT};
141+#ifdef MSG_MORE

l0rinc commented at 3:44 am on September 30, 2025:

can we have some explanation here, it’s not self-explanatory. The source location had:

   // We rely on the 'more' value returned by GetBytesToSend to correctly predict whether more
   // bytes are still to be sent, to correctly set the MSG_MORE flag. As a sanity check,
   // verify that the previously returned 'more' was correct.

l0rinc commented at 7:15 pm on September 30, 2025: contributor

I’m not immersed in this to give a meaningful ack yet, but please see a few of my comments.

My main observation is that we’re introducing tests and new code at the same time and I find it hard to follow what the relation between the two is - how do I tell if they’re doing what they’re supposed to. Given that this is meant to replace libevent, I wonder if we could add either a wrapper to libevent and cover it with tests as a first step (could be done in a separate standalone PR as well) and slowly replace the methods we rely on from libevent with our own implementation while the tests keep passing and as a last step delete libevent abstraction completely. That would reassure me personally that the new code isn’t just dead weight we’re adding to the code, but that we’re slowly strangling out a dependency with small focused changes covered with tests where I can debug both the old or new behavior and make absolutely sure they behave in the same way.

I would also prefer new code to be fully covered with tests, especially since none of the error cases and ipv6 part seem to be.

DrahtBot added the label Needs rebase on Oct 6, 2025

DrahtBot commented at 5:51 pm on October 6, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

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