-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Split CConnman #30988
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
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. LLM Linter (✨ experimental)Possible typos and grammar issues:
No other typos were found. drahtbot_id_4_m |
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 |
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.
|
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. |
🐙 This pull request conflicts with the target branch and needs rebase. |
@tdb3 @hodlinator @Sjors I'm hoping to turn your concept ACKs into reviews on the stripped-down version of this PR: #32747 |
@pinheadmz it's on my review list, will do soon(tm) |
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 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
.