backport: merge bitcoin#21167, #22782, #21943, #22829, #24079, #24108, #24157, #25109 (network backports: part 5) #6004
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kwvg
changed the title
Sock aware of event mode, move event (de)registration to Sock, backport merge bitcoin#20210, #22782, #21943, #21879, #23604 (networking backports: part 5)Sock aware of event mode, move event (de)registration to Sock, backport merge bitcoin#21167, #22782, #21943, #21879, #23604 (networking backports: part 5)
kwvg
force-pushed
the
net_processing_5
branch
3 times, most recently
from
fb3de5f
to
4cdde7f
Compare
5 tasks
kwvg
changed the title
Sock aware of event mode, move event (de)registration to Sock, backport merge bitcoin#21167, #22782, #21943, #21879, #23604 (networking backports: part 5)
kwvg
force-pushed
the
net_processing_5
branch
2 times, most recently
from
f09374a
to
6d5ff0a
Compare
UdjinM6
reviewed
May 8, 2024
…tex and rename it
5 tasks
knst
approved these changes
May 9, 2024
PastaPastaPasta
added a commit
that referenced
this pull request
May 14, 2024
…keup pipes logic out of `CConnman` and into `EdgeTriggeredEvents` and `WakeupPipes` bd8b5d4 net: add more details to log information in ETE and `WakeupPipes` (Kittywhiskers Van Gogh) ec99294 net: restrict access `EdgeTriggerEvents` members (Kittywhiskers Van Gogh) f24520a net: log `close` failures in `EdgeTriggerEvents` and `WakeupPipe` (Kittywhiskers Van Gogh) b8c3b48 refactor: introduce `WakeupPipe`, move wakeup select pipe logic there (Kittywhiskers Van Gogh) ed7d976 refactor: move wakeup pipe (de)registration to ETE (Kittywhiskers Van Gogh) f50c710 refactor: move `CConnman::`(`Un`)`registerEvents` to ETE (Kittywhiskers Van Gogh) 3a9f386 refactor: move `SOCKET` addition/removal from interest list to ETE (Kittywhiskers Van Gogh) 212df06 refactor: introduce `EdgeTriggeredEvents`, move {epoll, kqueue} fd there (Kittywhiskers Van Gogh) 3b11ef9 refactor: move `CConnman::SocketEventsMode` to `util/sock.h` (Kittywhiskers Van Gogh) Pull request description: ## Motivation `CConnman` is an entity that contains a lot of platform-specific implementation logic, both inherited from upstream and added upon by Dash (support for edge-triggered socket events modes like `epoll` on Linux and `kqueue` on FreeBSD/Darwin). Bitcoin has since moved to strip down `CConnman` by moving peer-related logic to the `Peer` struct in `net_processing` (portions of which are backported in #5982 and friends, tracking efforts from bitcoin#19398) and moving socket-related logic to `Sock` (portions of which are aimed to be backported in #6004, tracking efforts from bitcoin#21878). Due to the direction being taken and the difference in how edge-triggered events modes operate (utilizing interest lists and events instead of iterating over each socket) in comparison to level-triggered modes (which are inherited from upstream), it would be reasonable to therefore, isolate Dash-specific code into its own entities and minimize the information `CConnman` has about its internal workings. One of the visible benefits of this approach is comparing `develop` (as of this writing, d44b0d5) and this pull request for interactions between wakeup pipes logic and {`epoll`, `kqueue`} logic. This is what construction looks like: https://github.com/dashpay/dash/blob/d44b0d5dcb9b54821d582b267a8b92264be2da1b/src/net.cpp#L3358-L3397 But, if we segment wakeup pipes logic (that work on any platform with POSIX APIs and excludes Windows) and {`epoll`, `kqueue`} logic (calling them `EdgeTriggeredEvents` instead), construction looks different: https://github.com/dashpay/dash/blob/907a3515170abed4ce9018115ed591e6ca9f4800/src/util/wpipe.cpp#L12-L38 Now wakeup pipes logic doesn't need to know what socket events mode is being used nor are the implementation aspects of (de)registering it its concern, that is now `EdgeTriggeredEvents` problem. ## Additional Information * This pull request will need testing on macOS (FreeBSD isn't a tier-one target) to ensure that lack of breakage in `kqueue`-specific logic. ## Breaking Changes * Dependency for #6018 * More logging has been introduced and existing log messages have been made more exhaustive. If there is parsing that relies on a particular template, they will have to be updated. * If `EdgeTriggeredEvents` or `WakeupPipes` fail to initialize or are incorrectly initialized and not destroyed immediately, any further attempts at calling any of its functions will result in an `assert`-induced crash. Earlier behavior may have allowed for silent failure but segmentation of logic from `CConnman` means the newly created instances must only exist if the circumstances needed for it to initialize correctly are present. This is to ensure that `CConnman` doesn't have to concern itself with internal workings of either entities. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK bd8b5d4 Tree-SHA512: 8f793d4b4f2d8091e05bb9cc108013e924bbfbf19081290d9c0dfd91b0f2c80652ccf853f1596562942b4433509149c526e111396937988db605707ae1fe7366
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Additional Information
Dependent on refactor: replace
LOCKS_EXCLUDEDwith stricter negativeEXCLUSIVE_LOCKS_REQUIREDin Dash-specific code #6001Dependency for refactor: backport bitcoin#24356, extend
-socketeventstoSock, implementSock::WaitMany{Epoll,KQueue} #6018Partially reverts ff69e0d from refactor: conver std::vector const references to Span #5336 due to
Span<CNode*>'s incompatibility withCConnman::NodesSnapshot::Snap()(returningconst std::vector<CNode*>&)Dash already implements its own
CNode*iteration logic in dash#1382 and implemented additional capabilities in dash#1575, which meant backporting bitcoin#21943 involved migrating Dash-specific code to upstream logic that needed to be modified to implement expected functionality.Unlike Bitcoin, Dash maintains a map of every raw
SOCKETcorresponding to a pointer of theirCNodeinstance and uses it to translate socket sets to their correspondingCNode*sets. This is done to accommodate for edge-triggered modes which have an event-socket relationship, as opposed to level-triggered modes, which have a socket-event relationship.This means that
CConnman::SocketHandlerConnected()doesn't require access to a vector of allCNodepointers and therefore, the argumentnodeshas been omitted.Checklist: