Skip to content

Conversation

vasild
Copy link
Contributor

@vasild vasild commented Sep 27, 2024

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
Copy link
Contributor

DrahtBot commented Sep 27, 2024

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

  • #32983 (rpc: refactor: use string_view in Arg/MaybeArg by stickies-v)
  • #32822 (fuzz: Make process_message(s) more deterministic by maflcko)
  • #32747 (Introduce SockMan ("lite"): low-level socket handling for HTTP by pinheadmz)
  • #32394 (net: make m_nodes_mutex non-recursive by vasild)
  • #32326 (net: improve the interface around FindNode() and avoid a recursive mutex lock by vasild)
  • #32278 (doc: better document NetEventsInterface and the deletion of "CNode"s by vasild)
  • #32065 (i2p: make a time gap between creating transient sessions and using them 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)
  • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
  • #29418 (rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats' by vasild)
  • #28584 (Fuzz: extend CConnman tests 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:

  • "an unique id" → "a unique id" [“unique” begins with a consonant sound, so it takes “a” rather than “an.”]

No other typos were found.

drahtbot_id_4_m

@Sjors
Copy link
Member

Sjors commented Sep 27, 2024

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

@vasild
Copy link
Contributor Author

vasild commented Sep 27, 2024

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

Can you put sockman.h in libbitcoin_common

Done.

@Sjors
Copy link
Member

Sjors commented Sep 27, 2024

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

Copy link
Contributor

@tdb3 tdb3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

@pinheadmz
Copy link
Member

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
Copy link
Member

maflcko commented Sep 30, 2024

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
Copy link
Contributor Author

vasild commented Oct 3, 2024

@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 Sjors mentioned this pull request Oct 4, 2024
3 tasks
@Sjors
Copy link
Member

Sjors commented Oct 4, 2024

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

vasild added 16 commits June 13, 2025 14:19
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()`
`GetNewId()`
`m_i2p_sam_session`
`m_listen`

are now used only by `SockMan`, thus make them private.
@vasild
Copy link
Contributor Author

vasild commented Jun 13, 2025

a8fcd8385e...6aa43a298d: rebase and address minor suggestion in comment

@Sjors
Copy link
Member

Sjors commented Jun 23, 2025

I suggest marking this as draft while there is momentum on the "lite" version in #32747. Depending on how that goes, this PR could be rebased on it.

@vasild vasild marked this pull request as draft June 25, 2025 14:42
@vasild
Copy link
Contributor Author

vasild commented Jun 25, 2025

Marked as draft as suggested. Will focus on #32747, thanks!

@DrahtBot
Copy link
Contributor

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

@pinheadmz
Copy link
Member

@tdb3 @hodlinator @Sjors I'm hoping to turn your concept ACKs into reviews on the stripped-down version of this PR: #32747

@Sjors
Copy link
Member

Sjors commented Aug 22, 2025

@pinheadmz it's on my review list, will do soon(tm)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split socket handling out of CConnman