Split CConnman #30988

vasild · 2024-09-27T07:57:10Z

Currently CConnman is a mixture of:

low level socket handling, e.g. send, recv, poll, bind, listen, connect, and
higher level logic that is specific to the Bitcoin P2P protocol, e.g. V1/V2 transport, choosing which address to connect to, if we manage to connect mark the address good in AddrMan, maintaining the number of inbound and outbound connections, banning of peers, interacting with PeerManager.

This PR splits the socket handling into a new class which makes the code more modular and reusable. Having more modular and reusable code is a good thing on its own, even if the code is not reused. Stratum V2 and libevent-less RPC/HTTP server could benefit from this, but it makes sense on its own, even without those projects.

The socket operations are driven by the new class SockMan which informs the higher level via provided methods when e.g. new data arrives on the socket or a new connection is accepted. For this, SockMan provides some non-virtual methods to start it rolling and then it calls pure virtual methods which are implemented by the higher level (e.g. CConnman) on certain events, for example "got this new data on this node's socket".

The interface of SockMan is:

/**
 * A socket manager class which handles socket operations.
 * To use this class, inherit from it and implement the pure virtual methods.
 * Handled operations:
 * - binding and listening on sockets
 * - starting of necessary threads to process socket operations
 * - accepting incoming connections
 * - making outbound connections
 * - closing connections
 * - waiting for IO readiness on sockets and doing send/recv accordingly
 */
class SockMan
{
public:

    //
    // Non-virtual functions, to be reused by children classes.
    //

    /**
     * Bind to a new address:port, start listening and add the listen socket to `m_listen`.
     * Should be called before `StartSocketsThreads()`.
     * @param[in] to Where to bind.
     * @param[out] err_msg Error string if an error occurs.
     * @retval true Success.
     * @retval false Failure, `err_msg` will be set.
     */
    bool BindAndStartListening(const CService& to, bilingual_str& err_msg);

    /**
     * Start the necessary threads for sockets IO.
     */
    void StartSocketsThreads(const Options& options);

    /**
     * Join (wait for) the threads started by `StartSocketsThreads()` to exit.
     */
    void JoinSocketsThreads();

    /**
     * Make an outbound connection, save the socket internally and return a newly generated connection id.
     * @param[in] to The address to connect to, either as CService or a host as string and port as
     * an integer, if the later is used, then `proxy` must be valid.
     * @param[in] is_important If true, then log failures with higher severity.
     * @param[in] proxy Proxy to connect through, if set.
     * @param[out] proxy_failed If `proxy` is valid and the connection failed because of the
     * proxy, then it will be set to true.
     * @param[out] me If the connection was successful then this is set to the address on the
     * local side of the socket.
     * @return Newly generated id, or std::nullopt if the operation fails.
     */
    std::optional<SockMan::Id> ConnectAndMakeId(const std::variant<CService, StringHostIntPort>& to,
                                                bool is_important,
                                                std::optional<Proxy> proxy,
                                                bool& proxy_failed,
                                                CService& me)
        EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex, !m_unused_i2p_sessions_mutex);

    /**
     * Destroy a given connection by closing its socket and release resources occupied by it.
     * @param[in] id Connection to destroy.
     * @return Whether the connection existed and its socket was closed by this call.
     */
    bool CloseConnection(Id id)
        EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex);

    /**
     * Try to send some data over the given connection.
     * @param[in] id Identifier of the connection.
     * @param[in] data The data to send, it might happen that only a prefix of this is sent.
     * @param[in] will_send_more Used as an optimization if the caller knows that they will
     * be sending more data soon after this call.
     * @param[out] errmsg If <0 is returned then this will contain a human readable message
     * explaining the error.
     * @retval >=0 The number of bytes actually sent.
     * @retval <0 A permanent error has occurred.
     */
    ssize_t SendBytes(Id id,
                      std::span<const unsigned char> data,
                      bool will_send_more,
                      std::string& errmsg) const
        EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex);

    /**
     * Stop listening by closing all listening sockets.
     */
    void StopListening();

    //
    // Pure virtual functions must be implemented by children classes.
    //

    /**
     * Be notified when a new connection has been accepted.
     * @param[in] id Id of the newly accepted connection.
     * @param[in] me The address and port at our side of the connection.
     * @param[in] them The address and port at the peer's side of the connection.
     * @retval true The new connection was accepted at the higher level.
     * @retval false The connection was refused at the higher level, so the
     * associated socket and id should be discarded by `SockMan`.
     */
    virtual bool EventNewConnectionAccepted(Id id,
                                            const CService& me,
                                            const CService& them) = 0;

    /**
     * Called when the socket is ready to send data and `ShouldTryToSend()` has
     * returned true. This is where the higher level code serializes its messages
     * and calls `SockMan::SendBytes()`.
     * @param[in] id Id of the connection whose socket is ready to send.
     * @param[out] cancel_recv Should always be set upon return and if it is true,
     * then the next attempt to receive data from that connection will be omitted.
     */
    virtual void EventReadyToSend(Id id, bool& cancel_recv) = 0;

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

    /**
     * Called when the remote peer has sent an EOF on the socket. This is a graceful
     * close of their writing side, we can still send and they will receive, if it
     * makes sense at the application level.
     * @param[in] id Connection whose socket got EOF.
     */
    virtual void EventGotEOF(Id id) = 0;

    /**
     * Called when we get an irrecoverable error trying to read from a socket.
     * @param[in] id Connection whose socket got an error.
     * @param[in] errmsg Message describing the error.
     */
    virtual void EventGotPermanentReadError(Id id, const std::string& errmsg) = 0;
};

Resolves: #30694

Review hint: this PR moves some code around, so reviewers may find this helpful: git show --color-moved --color-moved-ws=allow-indentation-change.

DrahtBot · 2024-09-27T07:57:13Z

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/30988.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky
Concept NACK	theuni
Concept ACK	tdb3, Sjors, hodlinator, jonatack
Stale ACK	pinheadmz

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:

#33956 (net: fix use-after-free with v2->v1 reconnection logic by Crypt-iQ)
#32394 (net: make m_nodes_mutex non-recursive by vasild)
#32278 (doc: better document NetEventsInterface and the deletion of "CNode"s by vasild)
#32015 (net: replace manual reference counting of CNode with shared_ptr by vasild)
#30951 (net: option to disallow v1 connection on ipv4 and ipv6 peers by stratospher)
#30116 (p2p: Fill reconciliation sets (Erlay) attempt 2 by sr-gi)
#29641 (scripted-diff: Use LogInfo over LogPrintf by maflcko)
#29418 (rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats' by vasild)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)

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:

"Singal this to tell SockMan to cease network activity." -> "Signal this to tell SockMan to cease network activity." [spelling error: "Singal" -> "Signal"]
"Each connection is assigned an unique id of this type." -> "Each connection is assigned a unique id of this type." [article error: "an unique" -> "a unique"]
"if the later is used, then proxy must be valid." -> "if the latter is used, then proxy must be valid." [wrong word: "later" -> "latter"]

No other typographic or grammatical errors that make the English invalid or incomprehensible were found in the added lines.

^{drahtbot_id_5_m}

Sjors · 2024-09-27T11:41:28Z

Nice! I'll try to use this for Sv2Connman in Sjors#50 and will let you know if anything is missing.

Can you put sockman.h in libbitcoin_common instead of libbitcoin_node? For the Template Provider I'm trying to prevent a circular dependency on the node. This should do the trick: 4dd51b2

src/rpc/net.cpp

vasild · 2024-09-27T15:07:28Z

03f6cc2b4a...70c2f13f83: fix CI failure, and address suggestions

Can you put sockman.h in libbitcoin_common

Done.

Sjors · 2024-09-27T16:51:49Z

Here's an initial sketch of making Sv2Connman a subclass of SockMan. The test gets through the handshake but fails later on, so I'll need to study it a bit more closely.

Sjors#64

tdb3

Concept ACK

pinheadmz · 2024-09-30T13:38:34Z

Concept ACK

This looks great and the API in the header looks easy, thanks.

I'm in the process of cleaning up my HTTP branch for a pull request and then I can start reviewing this and rebasing on top.

One element of libevent I'm not immediately seeing here is timed events. Really the only thing HTTP needs it for is walletpassphrase which calls RPCRunLater() which interacts with HTTPRPCTimerInterface(). I don't think Conman has a specific mechanism for this because timed things are attached directly to nodes like m_last_getheaders_timestamp etc. The current HTTPRPCTimerInterface uses libevent event_new() and evtimer_add(), I accomplish this with a map of timestamps and callback functions in my event loop: pinheadmz@42b7240

maflcko · 2024-09-30T14:07:19Z

I accomplish this with a map of timestamps and callback functions in my event loop

I wonder why the existing scheduler can't be used for re-locking the wallet? I know there is #18488 and #14289, but the thread is already filled with random stuff such as BerkeleyDatabase::PeriodicFlush(), and relocking the wallet seems(?) fast (I haven't benchmarked), so should be fine to put in there as well, at least from that perspective?

vasild · 2024-10-03T10:07:55Z

@pinheadmz, I think that the functionality of "execute this code after some time", is not much related to the sockets handling and better be implemented at some higher level, not inside SockMan. Maybe the scheduler, like @maflcko suggested, or in the EventIOLoopCompletedForAllPeers() method which will be called periodically by SockMan:

    /**
     * SockMan has completed send+recv for all nodes.
     * Can be used to execute periodic tasks for all nodes.
     * The implementation in SockMan does nothing.
     */
    virtual void EventIOLoopCompletedForAllPeers();

Edit: I guess TriggerEvents() from pinheadmz@42b7240 can be called from EventIOLoopCompletedForAllPeers() or from the scheduler.

Sjors · 2024-10-04T13:54:41Z

@vasild if you rebase past #31011, tidy might point out that sockman.cpp.o depends on i2p.cpp. So you probably need to either move i2p.cpp to common as well, or remove the dependency.

They were coupled in `struct ListenSocket`, but the socket belongs to the lower level transport protocol, whereas the permissions are specific to the higher Bitcoin P2P protocol.

Now that `CConnman::ListenSocket` is a `struct` that contains only one member variable of type `std::shared_ptr<Sock>`, drop `ListenSocket` and use `shared_ptr` directly. Replace the vector of `ListenSocket` with a vector of `shared_ptr`.

Introduce a new low-level socket managing class `SockMan` and move the `CConnman::BindListenPort()` method to it.

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.

Move the `CConnman::AcceptConnection()` method to `SockMan` and split parts of it: * the flip-to-CJDNS part: to just after the `AcceptConnection()` call * the permissions part: at the start of `CreateNodeFromAcceptedSocket()`

Move `CConnman::GetNewNodeId()` to `SockMan::GetNewId()`. Avoid using the word "node" because that is too specific for `CConnman`.

CConnman-specific or in other words, Bitcoin P2P specific. Now the `ThreadI2PAcceptIncoming()` method is protocol agnostic and can be moved to `SockMan`.

Change `CConnman::m_nodes` from `std::vector<CNode*>` to `std::unordered_map<NodeId, CNode*>` because interaction between `CConnman` and `SockMan` is going to be based on `NodeId` and finding a node by its id would better be fast. Change `PeerManagerImpl::EvictExtraOutboundPeers()` to account for nodes no longer always being in order of id. The old code would have failed to update `next_youngest_peer` correctly if `CConnman::m_nodes` hadn't always had nodes in ascending order of id. During fuzzing make sure that we don't generate duplicate `CNode` ids. The easiest way to do that is to use sequential ids. As a nice side effect the existent search-by-id operations in `CConnman::AttemptToEvictConnection()`, `CConnman::DisconnectNode()` and `CConnman::ForNode()` now become `O(1)` (were `O(number of nodes)`), as well as the erase in `CConnman::DisconnectNodes()`.

Move the parts of `CConnman::GenerateWaitSockets()` that are specific to the Bitcoin-P2P protocol to dedicated methods: `ShouldTryToSend()` and `ShouldTryToRecv()`. This brings us one step closer to moving `GenerateWaitSockets()` to the protocol agnostic `SockMan` (which would call `ShouldTry...()` from `CConnman`).

…cketHandler() Move some parts of `CConnman::SocketHandlerConnected()` and `CConnman::ThreadSocketHandler()` that are specific to the Bitcoin-P2P protocol to dedicated methods: `EventIOLoopCompletedForOne(id)` and `EventIOLoopCompletedForAll()`. This brings us one step closer to moving `SocketHandlerConnected()` and `ThreadSocketHandler()` to the protocol agnostic `SockMan` (which would call `EventIOLoopCompleted...()` from `CConnman`).

Introduce 4 new methods for the interaction between `CConnman` and `SockMan`: * `EventReadyToSend()`: called when there is readiness to send and do the actual sending of data. * `EventGotData()`, `EventGotEOF()`, `EventGotPermanentReadError()`: called when the corresponing recv events occur. These methods contain logic that is specific to the Bitcoin-P2P protocol and move it away from `CConnman::SocketHandlerConnected()` which will become a protocol agnostic method of `SockMan`. Also, move the counting of sent bytes to `CConnman::SocketSendData()` - both callers of that method called `RecordBytesSent()` just after the call, so move it from the callers to inside `CConnman::SocketSendData()`.

Move the protocol agnostic parts of `CConnman::ConnectNode()` into `SockMan::ConnectAndMakeId()` and leave the Bitcoin-P2P specific stuff in `CConnman::ConnectNode()`. Move the protocol agnostic `CConnman::m_unused_i2p_sessions`, its mutex and `MAX_UNUSED_I2P_SESSIONS_SIZE` to `SockMan`. Move `GetBindAddress()` from `net.cpp` to `sockman.cpp`.

Move `MaybeFlipIPv6toCJDNS()`, which is Bitcoin P2P specific from the callers of `CConnman::EventNewConnectionAccepted()` to inside that method. Move the IsSelectable check, the `TCP_NODELAY` option set and the generation of new connection id out of `CConnman::EventNewConnectionAccepted()` because those are protocol agnostic. Move those to a new method `SockMan::NewSockAccepted()` which is called instead of `CConnman::EventNewConnectionAccepted()`.

Move `CNode::m_sock` and `CNode::m_i2p_sam_session` to `SockMan::m_connected`. Also move all the code that handles sockets to `SockMan`. `CNode::CloseSocketDisconnect()` becomes `CConnman::MarkAsDisconnectAndCloseConnection()`. `CConnman::SocketSendData()` is renamed to `CConnman::SendMessagesAsBytes()` and its sockets-touching bits are moved to `SockMan::SendBytes()`. `CConnman::GenerateWaitSockets()` goes to `SockMan::GenerateWaitSockets()`. `CConnman::ThreadSocketHandler()` and `CConnman::SocketHandler()` are combined into `SockMan::ThreadSocketHandler()`. `CConnman::SocketHandlerConnected()` goes to `SockMan::SocketHandlerConnected()`. `CConnman::SocketHandlerListening()` goes to `SockMan::SocketHandlerListening()`.

`SockMan` members `AcceptConnection()` `NewSockAccepted()` `m_i2p_sam_session` `m_listen` are now used only by `SockMan`, thus make them private. `GetNewId()` is used by `SockMan` and in tests, so make it protected.

vasild · 2025-11-04T14:09:30Z

69c015b258...6091e94750: rebase due to conflicts

vasild · 2025-11-04T17:10:21Z

There are a lot of code moves in this PR. I put the following in my ~/.gitconfig:

[diff]
        colorMoved = dimmed-zebra
        colorMovedWS = allow-indentation-change

it makes reviewing such mechanical moves easier.

ryanofsky

Code review ACK 6091e94

I'm about halfway through reviewing this, and so far it seems like has a lot of nice code improvements in addition to conceptual benefits mentioned earlier #30988 (comment).

One thing I don't like about the API however, is the use of integer connection ids and introduction of hash maps to to tie connection ids, sockets, and application data together:

CConnman::m_listen_permissions - mapping listening addresses to permissions
CConnman::m_nodes - mapping connection ids to CNodes
IOReadiness::ids_per_sock - mapping Sock pointers to connection ids
SockMan::m_connected``` - mapping connection ids to Sock` pointers

I think it would be better to:

Keep the ListeningSock struct instead of getting rid of it, and give it a std::any member so application data can be associated with it. The data could be passed to BindAndStartListening and EventNewConnectionAccepted.
Rename struct ConnectionSockets to class Connection and add a std::any member to it that can hold the CNode reference. Delete the SockMan::Id type, and use Connection references everywhere ids are currently used.

I think this would provide a safer API and simplify code, allowing all the unordered maps to be dropped. Not very important though, since this PR is mostly moving code, and API improvements could be implemented in followups.

I plan to continue reviewing the PR.

ryanofsky · 2025-11-17T14:25:35Z

src/net.h

        }
-
-    private:
-        NetPermissionFlags m_permissions;


In commit "net: separate the listening socket from the permissions" (1b2a87b)

It would seem less fragile to just replace the m_permissions field with a more generic field like std::any m_metadata or std::any m_user_data instead of requiring the application to maintain a separate unordered map and needing to deal with conditions when the map and vector could get out of sync (#30988 (comment), #30988 (comment)).

ryanofsky · 2025-11-17T15:16:13Z

src/common/sockman.cpp

 #include <util/sock.h>

-bool SockMan::BindListenPort(const CService& addrBind, bilingual_str& strError)
+bool SockMan::BindAndStartListening(const CService& to, bilingual_str& err_msg)


In commit "style: modernize the style of SockMan::BindListenPort()" (6ba0aab)

Could be good to note in commit message what behaviors this is changing, so it clear they are intentional. After this commit:

err_msg / strError output is no longer set if there are minor errors like failing to set SO_REUSEADDR

"Bound to [address]" log message is dropped and moved to caller

Other log messages are changed slightly

ryanofsky · 2025-11-17T15:32:17Z

src/net.cpp

+            auto sock_accepted{AcceptConnection(*sock, addr_accepted)};
+
+            if (sock_accepted) {
+                addr_accepted = MaybeFlipIPv6toCJDNS(addr_accepted);


In commit "net: split CConnman::AcceptConnection() off CConnman" (3e88116)

re: #30988 (comment)

Yes, good observation! Indeed the flip will do nothing on the invalid 0.0.0.0 addr_accepted. [...]

IMO this information would be useful to mention in the commit message

ryanofsky · 2025-11-17T16:58:58Z

src/common/sockman.h

+        /// The listen succeeded and we are now listening for incoming I2P connections.
+        START_LISTENING,
+
+        /// The listen failed and now we are not listening (even if START_LISTENING was signaled before).
+        STOP_LISTENING,


In commit "net: move CConnman-specific parts away from ThreadI2PAcceptIncoming()" (07d731b)

Names and comments seem confusing and maybe inaccurate:

The name STOP_LISTENING implies I2P was previously listening but now stopped, but the comment implies it might have never been listening.

The name STOP_LISTENING seems like it could happen for reasons other than failing (like shutting down), so it's inconsistent with the comment which only mentions failing.

Comments mention STOP could happen after START, but not that START could happen after STOP, even though the code seems to allow that.

Comment implies these notifications are only sent on status changes, but code looks like these will be sent repeatedly even if nothing changes.

Would suggest rewriting and renaming to be clearer. Maybe:

// Indicates whether the I2P node is currently listening for new connections. // A status update is sent on each accept attempt; it may be identical to the // previous update, and the state may temporarily switch between listening and // not listening. enum class I2PStatus { LISTENING, NOT_LISTENING };

ryanofsky · 2025-11-17T17:46:37Z

src/common/sockman.h


+    /**
+     * Constructor.
+     * @param[in] interrupt_net Singal this to tell SockMan to cease network activity.


In commit "net: move I2P-accept-incoming code from CConnman to SockMan" (3e91c8b)

s/Singal/Signal

ryanofsky · 2025-11-17T18:28:06Z

src/net.cpp

-            found = pnode;
-            break;
-        }
+    auto it{m_nodes.find(id)};


In commit "net: index nodes in CConnman by id" (6375134)

Not important but seems like the ForNode and DisconnectNode methods changed here could be simpler if they used the GetNodeById helper method introduced in the next commit (3bb0f2d). Might be good to backport it.

ryanofsky · 2025-11-17T18:58:12Z

src/common/sockman.h

+     * @retval >=0 The number of bytes actually sent.
+     * @retval <0 A permanent error has occurred.
+     */
+    ssize_t SendBytes(Id id,


In commit "net: move sockets from CNode to SockMan" (a59b12a)

This commit is larger than the other commits. It would be nice if there's way to break it up since a few things are moving at the same time.

ryanofsky · 2025-11-18T18:40:53Z

re: #30988 (comment)

I think I agree with @theuni's comment in #30988 (comment) that the http server and sv2 server probably would not benefit very much from using this, but that seems like more of a concern for #32061 than this PR.

I would be interested to know more about how the HTTP and sv2 implementations "ended up with very non-optimal performance because of the optimistic send" though because that would seem like something that would be good to correct or at least document in the sockman class even if it will not be used for these purposes.

After looking at #32061 more closely, I think I understand @theuni's concern about how the current SockMan interface does not work well for applications like the HTTP server.

As I understand it, the problem in #32061 is that when an RPC method finishes and a reply is ready, the RPC thread has no clean way to wake up the SockMan thread and tell it there is data ready to send on this connection. The only thing HTTPRequest::WriteReply can do is flip the m_send_ready flag returned by SockMan::ShouldTryToSend from false to true, and then wait for the SockMan loop to wake up on its own (up to a 50ms poll timeout) and notice the change.

The latest version of #32061 works around this partially by using the "optimistic send" pattern: HTTPRequest::WriteReply tries to write data directly to the socket itself if no other write is pending. This makes the code more complicated, and it also doesn’t fully solve the underlying problem: if that optimistic write only sends the first chunk of the response, the socket can still sit idle until the next periodic wakeup even though more data is already available in user space.

IMO the SockMan interface could be improved to avoid this pitfall. One way would be to rename SockMan::SendBytes to something like SockMan::TrySendNow, with documentation that it is only meant to be called from inside the EventReadyToSend callback (i.e. when we already know the socket is writable), and maybe in rare cases as an optimization to avoid a thread hop.

Alternately, if we go in the direction suggested in #30988 (review) and callbacks start passing Connection objects instead of integer ids, TrySendNow could become a Connection::SendBytes method that is only ever called from within those callbacks, reducing the chances of it it being misused from other threads.

#32061 probably shouldn’t need to care about any of this if it is rewritten to not use SockMan, but if we did want to generalize SockMan, we could think about adding a thread-safe SockMan::Wake method that interrupts its select/poll wait so the event loop can immediately re-run ShouldTryToSend and call back into EventReadyToSend as needed. Then applications like HTTPRequest::WriteReply wouldn't need to do optimistic sends at all, they could just mark the connection as ready-to-send and call SockMan::Wake().

DrahtBot · 2025-12-05T10:47:29Z

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

vasild mentioned this pull request Sep 27, 2024

Split socket handling out of CConnman #30694

Closed

DrahtBot added the CI failed label Sep 27, 2024

maflcko reviewed Sep 27, 2024

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

vasild force-pushed the sockman branch from 03f6cc2 to 70c2f13 Compare September 27, 2024 15:05

DrahtBot removed the CI failed label Sep 27, 2024

DrahtBot added CI failed and removed CI failed labels Sep 29, 2024

tdb3 reviewed Sep 29, 2024

View reviewed changes

This was referenced Oct 1, 2024

rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats' #29418

Open

net: Use GetAdaptersAddresses to get local addresses on Windows #31014

Merged

test: Add mockable steady clock, tests for PCP and NATPMP implementations #31022

Merged

Sjors mentioned this pull request Oct 4, 2024

Stratum v2 connman Sjors/bitcoin#50

Closed

3 tasks

Jacksonearl2468 approved these changes Oct 5, 2024

View reviewed changes

vasild added 17 commits November 4, 2025 14:56

net: separate the listening socket from the permissions

1b2a87b

They were coupled in `struct ListenSocket`, but the socket belongs to the lower level transport protocol, whereas the permissions are specific to the higher Bitcoin P2P protocol.

net: drop CConnman::ListenSocket

24625de

Now that `CConnman::ListenSocket` is a `struct` that contains only one member variable of type `std::shared_ptr<Sock>`, drop `ListenSocket` and use `shared_ptr` directly. Replace the vector of `ListenSocket` with a vector of `shared_ptr`.

net: split CConnman::BindListenPort() off CConnman

0a22cda

Introduce a new low-level socket managing class `SockMan` and move the `CConnman::BindListenPort()` method to it.

net: split CConnman::AcceptConnection() off CConnman

3e88116

Move the `CConnman::AcceptConnection()` method to `SockMan` and split parts of it: * the flip-to-CJDNS part: to just after the `AcceptConnection()` call * the permissions part: at the start of `CreateNodeFromAcceptedSocket()`

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

9526562

net: move the generation of ids for new nodes from CConnman to SockMan

44d4b62

Move `CConnman::GetNewNodeId()` to `SockMan::GetNewId()`. Avoid using the word "node" because that is too specific for `CConnman`.

net: move CConnman-specific parts away from ThreadI2PAcceptIncoming()

07d731b

CConnman-specific or in other words, Bitcoin P2P specific. Now the `ThreadI2PAcceptIncoming()` method is protocol agnostic and can be moved to `SockMan`.

net: move I2P-accept-incoming code from CConnman to SockMan

3e91c8b

net: move-only: improve encapsulation of SockMan

6091e94

`SockMan` members `AcceptConnection()` `NewSockAccepted()` `m_i2p_sam_session` `m_listen` are now used only by `SockMan`, thus make them private. `GetNewId()` is used by `SockMan` and in tests, so make it protected.

vasild force-pushed the sockman branch from 69c015b to 6091e94 Compare November 4, 2025 14:09

DrahtBot removed the Needs rebase label Nov 4, 2025

ryanofsky reviewed Nov 17, 2025

View reviewed changes

DrahtBot requested review from Sjors, hodlinator, jonatack and pinheadmz November 17, 2025 19:41

DrahtBot mentioned this pull request Nov 30, 2025

net: fix use-after-free with v2->v1 reconnection logic #33956

Merged

DrahtBot added the Needs rebase label Dec 5, 2025

Split CConnman #30988

Are you sure you want to change the base?

Split CConnman #30988

Uh oh!

Conversation

vasild commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

Sjors commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vasild commented Sep 27, 2024

Uh oh!

Sjors commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Sep 30, 2024

Uh oh!

maflcko commented Sep 30, 2024

Uh oh!

vasild commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Nov 4, 2025

Uh oh!

vasild commented Nov 4, 2025

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Nov 18, 2025

Uh oh!

DrahtBot commented Dec 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

vasild commented Sep 27, 2024 •

edited

Loading

DrahtBot commented Sep 27, 2024 •

edited

Loading

Sjors commented Sep 27, 2024 •

edited

Loading

Sjors commented Sep 27, 2024 •

edited

Loading

vasild commented Oct 3, 2024 •

edited

Loading

Sjors commented Oct 4, 2024 •

edited

Loading