Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split CConnman #30988

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Split CConnman #30988

wants to merge 16 commits into from

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 public 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] errmsg Error string if an error occurs.
     * @retval true Success.
     * @retval false Failure, `strError` will be set.
     */
    bool BindAndStartListening(const CService& to, bilingual_str& errmsg);

    /**
     * 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 node 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 `proxy.IsValid()` is true.
     * @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 node id, or std::nullopt if the operation fails.
     */
    std::optional<NodeId> ConnectAndMakeNodeId(const std::variant<CService, StringHostIntPort>& to,
                                               bool is_important,
                                               const Proxy& proxy,
                                               bool& proxy_failed,
                                               CService& me);

    /**
     * Disconnect a given peer by closing its socket and release resources occupied by it.
     * @return Whether the peer existed and its socket was closed by this call.
     */
    bool CloseConnection(NodeId node_id);

    /**
     * Try to send some data to the given node.
     * @param[in] node_id Identifier of the node to send to.
     * @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(NodeId node_id,
                      Span<const unsigned char> data,
                      bool will_send_more,
                      std::string& errmsg) const;

    /**
     * Close all sockets.
     */
    void CloseSockets();

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

    /**
     * Be notified when a new connection has been accepted.
     * @param[in] node_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 node_id should be discarded by `SockMan`.
     */
    virtual bool EventNewConnectionAccepted(NodeId node_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] node_id Id of the node 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 node will be omitted.
     */
    virtual void EventReadyToSend(NodeId node_id, bool& cancel_recv) = 0;

    /**
     * Called when new data has been received.
     * @param[in] node_id Node for which the data arrived.
     * @param[in] data Data buffer.
     * @param[in] n Number of bytes in `data`.
     */
    virtual void EventGotData(NodeId node_id, const uint8_t* data, size_t n) = 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] node_id Node whose socket got EOF.
     */
    virtual void EventGotEOF(NodeId node_id) = 0;

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

    //
    // Non-pure virtual functions can be overridden by children classes or left
    // alone to use the default implementation from SockMan.
    //

    /**
     * SockMan would only call EventReadyToSend() if this returns true.
     * Can be used to temporary pause sends for a node.
     * The implementation in SockMan always returns true.
     * @param[in] node_id Node for which to confirm or cancel a call to EventReadyToSend().
     */
    virtual bool ShouldTryToSend(NodeId node_id) const;

    /**
     * SockMan would only call Recv() on a node's socket if this returns true.
     * Can be used to temporary pause receives for a node.
     * The implementation in SockMan always returns true.
     * @param[in] node_id Node for which to confirm or cancel a receive.
     */
    virtual bool ShouldTryToRecv(NodeId node_id) const;

    /**
     * SockMan has completed the current send+recv iteration for a node.
     * It will do another send+recv for this node after processing all other nodes.
     * Can be used to execute periodic tasks for a given node.
     * The implementation in SockMan does nothing.
     * @param[in] node_id Node for which send+recv has been done.
     */
    virtual void EventIOLoopCompletedForNode(NodeId node_id);

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

    /**
     * Be notified of a change in the state of listening for incoming I2P connections.
     * The default behavior, implemented by `SockMan`, is to ignore this event.
     * @param[in] addr Our listening address.
     * @param[in] success If true then the listen succeeded and we are now
     * listening for incoming I2P connections at `addr`. If false then the
     * call failed and now we are not listening (even if this was invoked
     * before with `true`).
     */
    virtual void EventI2PListen(const CService& addr, bool success);
};

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

  • #31519 (refactor: Use std::span over Span by maflcko)
  • #31022 (test: Add mockable steady clock, tests for PCP and NATPMP implementations by laanwj)
  • #31014 (net: Use GetAdaptersAddresses to get local addresses on Windows by laanwj)
  • #30951 (net: option to disallow v1 connection on ipv4 and ipv6 peers by stratospher)
  • #30381 ([WIP] net: return result from addnode RPC by willcl-ark)
  • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
  • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
  • #28584 (Fuzz: extend CConnman tests by vasild)
  • #28521 (net, net_processing: additional and consistent disconnect logging by Sjors)
  • #28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)
  • #25832 (tracing: network connection tracepoints by 0xB10C)

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.

@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

src/rpc/net.cpp Outdated Show resolved Hide resolved
@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
Copy link
Contributor Author

vasild commented Nov 19, 2024

c4f51c7f61...2b0103705f: rebase and make the virtual methods of SockMan private since they are only used by SockMan.

About storing the CNode in SockMan (which would be templated like SockMan<CNode>) and making the communication between SockMan and the higher class (e.g. CConnman) based on CNode instead of node id: this is an excellent idea that will make the code more straight-forward and the higher classes simpler. However it would make this PR larger. I will leave it off for now. There is some specific CNode ref-counting in CConnman and the code around deleting a node is a bit complicated, so it would require further non-trivial changes to move that to SockMan.

I think in general, in the long term, independently of this PR, it would be good to get rid of the manual CNode ref-counting and tap std::shared_ptr to do that for us. In other words, to revive #28222.

@vasild
Copy link
Contributor Author

vasild commented Dec 3, 2024

2b0103705f...645f625e29: rebase and address suggestions

src/common/sockman.h Outdated Show resolved Hide resolved
@Sjors Sjors mentioned this pull request Dec 13, 2024
@Sjors
Copy link
Member

Sjors commented Dec 19, 2024

Looks like the merge conflict is with #31072 as well as #31223.

* `CConnman::CalculateKeyedNetGroup()` needs `CNetAddr`, not `CAddress`,
  thus change its argument.

* Both callers of `CConnman::CreateNodeFromAcceptedSocket()` create a
  dummy `CAddress` from `CService`, so use `CService` instead.

* `GetBindAddress()` only needs to return `CAddress`.

* `CNode::addrBind` does not need to be `CAddress`.
Introduce a new low-level socket managing class `SockMan`
and move the `CConnman::BindListenPort()` method to it.

Also, separate the listening socket from the permissions -
they were coupled in `struct ListenSocket`, but the socket
is protocol agnostic, whereas the permissions are specific
to the application of the Bitcoin P2P protocol.
It was copied verbatim from `CConnman::BindListenPort()` in the previous
commit. Modernize its variables and style and log the error messages
from the caller.
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()`
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.

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:
`EventIOLoopCompletedForNode()` and `EventIOLoopCompletedForAllPeers()`.

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::ConnectAndMakeNodeId()` 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 node 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()`
`GetNewNodeId()`
`m_i2p_sam_session`
`m_listen private`

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

vasild commented Dec 19, 2024

645f625e29...b8b042626e: rebase due to conflicts and use std::span instead of Span in new code: #30988 (comment)

@DrahtBot
Copy link
Contributor

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

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