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
Concept ACK	w0xlt, hodlinator
Stale ACK	vasild

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:

#33210 (fuzz: enhance wallet_fees by mocking mempool stuff by brunoerg)
#32015 (net: replace manual reference counting of CNode with shared_ptr 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” starts with a consonant sound, so “a” is correct]
“datasent” → “data sent” [missing space between “data” and “sent”]

drahtbot_id_4_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: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:

👍

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: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:

👍

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: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:

👍

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

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>

72864e4628

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>

35324545ba

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>

03cce3b91b

style: modernize the style of SockMan::AcceptConnection() 2900c33f66

SockMan: generate sequential Ids for each newly accepted connection

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

7e68a72e8d

[move-only] Make GetBindAddress() callable from outside net.cpp 6e420ba127

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>

63eda80af9

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>

7d4d4b6fad

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>

6ef3e99280

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>

598bee6bd5

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

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.

in src/test/sockman_tests.cpp:26 in 598bee6bd5

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

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

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 {}

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

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.

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

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

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

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

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

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

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

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.

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

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

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

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

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

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

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.

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

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?

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

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 {

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

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

in src/test/sockman_tests.cpp:51 in 598bee6bd5

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)

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

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,

in src/common/sockman.cpp:329 in 598bee6bd5

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.

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

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;

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

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.

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

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.

in src/common/sockman.h:147 in 598bee6bd5

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.

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

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