-
Notifications
You must be signed in to change notification settings - Fork 36.5k
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
base: master
Are you sure you want to change the base?
Split CConnman #30988
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30988. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Done. |
Here's an initial sketch of making |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
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 |
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 |
@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 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 |
About storing the I think in general, in the long term, independently of this PR, it would be good to get rid of the manual |
|
* `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.
|
🐙 This pull request conflicts with the target branch and needs rebase. |
Currently
CConnman
is a mixture of:AddrMan
, maintaining the number of inbound and outbound connections, banning of peers, interacting withPeerManager
.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: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
.