fix: actually use -socketevents
#6027
Merged
+3
−1
Conversation
This file contains hidden or 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
6007 follow-up
PastaPastaPasta
closed this pull request by merging all changes
into
dashpay:develop
in
8b7f9c8
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Jul 9, 2025
…Events()` with mockable `Sock::WaitMany()`), implement `Sock::WaitMany{Epoll,KQueue}()`
c611fb0 fix: set `g_socket_events_mode` before starting `CConnman` (UdjinM6)
c6e0e96 chore: remove scaffolding (remove default args, make explicit choice) (Kittywhiskers Van Gogh)
aca5ec9 chore: remove scaffolding (SEM must be correct, no graceful fallback) (Kittywhiskers Van Gogh)
08a42c1 refactor: move `DEFAULT_SOCKETEVENTS` to `util/sock.h` (Kittywhiskers Van Gogh)
e4cc5ac net: implement `ToggleWakeupPipe` in all WaitMany variants (Kittywhiskers Van Gogh)
f01a871 net: add early bail out condition for empty `events_per_sock` for LTMs (Kittywhiskers Van Gogh)
5ae6f2a fix: merge `kqueue` events manually as they are not bitwise OR'ed (Kittywhiskers Van Gogh)
0d92d40 net: implement `WaitMany` variants for {`epoll`, `kqueue`} (Kittywhiskers Van Gogh)
0a8b8a6 merge bitcoin#24356: replace CConnman::SocketEvents() with mockable Sock::WaitMany() (Kittywhiskers Van Gogh)
4a7114f refactor: clean up `CConnman::SocketWait{Epoll,Kqueue}()` logic (Kittywhiskers Van Gogh)
a33f88f net: reintroduce `IsSelectableSocket()` and make it SEM-aware (Kittywhiskers Van Gogh)
41eaed2 net: allow selection of `Wait()` API by specifying `SocketEventsMode` (Kittywhiskers Van Gogh)
ca1ec0b net: split out `poll` and `select` variants from `Sock::Wait()` (Kittywhiskers Van Gogh)
7cffc0b fix: drain before winding down `WakeupPipe` object to avoid `SIGPIPE` (Kittywhiskers Van Gogh)
b69c1a1 fix: avoid dangling pipes during failed `WakeupPipe` initialization (Kittywhiskers Van Gogh)
Pull request description:
## Additional information
* Dependent on #6004
* Dependent on #6007
* Dependent on #6027
* Deviations from upstream
| Bitcoin | Dash | Reason |
| ------------------------------------------------------------ | ------------------------------------------------------------ | ------------------------------------------------------------ |
| `EventsPerSock` is a unordered map of `shared_ptr`s of `Sock` **wrappers** and `Events` | `EventsPerSock` is an unordered map of **raw** socket file descriptors (`SOCKET`) and `Events` | Dash implements `WakeupPipes`, which is constructed and destroyed using an entity outside `Sock`'s control. We need to be able to insert the read pipe raw socket into equivalent of the `recv` socket set and query for it later on.<br /><br />It would be _technically_ possible, though cumbersome, to wrap the read pipe raw socket in a `Sock` and overwrite the destructor if it wasn't for the support of edge-triggered modes which have an event-socket relationship, as opposed to level triggered modes, that have a socket-event relationship. |
| Sockets passed in an `EventsPerSock` map will **always** return with event data for every corresponding entry. | Sockets passed in an `EventsPerSock` map **may** return with event data for its corresponding entry. | The behaviour defined for Bitcoin will also be presented in Dash **if** the socket events mode (SEM) is `poll` or `select`. Otherwise, it will behave as described.<br /><br />This is due to the inversion of the socket-event relationship in edge-triggered modes (`epoll` and `kqueue`), as alluded to earlier. As edge-triggered modes return events and their corresponding socket (sockets registered through `EdgeTriggeredEvents::RegisterEntity()` and friends), the `EventsPerSock` map, will **have its contents completely discarded** and substituted with the results of {`epoll`, `kqueue`}. |
| You **must** have a `Sock` entity to call `Sock::WaitMany()` | You can directly access `Sock::WaitMany()`'s underlying logic through calling `Sock::WaitManyInternal()` (and access any *specific* event mode's implementation) **without** a `Sock` entity. | This change has been made as Bitcoin's behaviour was to call `WaitMany` by seeking to the first element to access it. This was possible because the unordered map consisted of `Sock` entities. As that isn't the case for Dash and `WaitMany` doesn't truly rely on instance-specific member values of a particular `Sock` instance (the values it relies on should remain constant throughout program runtime), it can be safely made a `static` function and that was exactly what was done.<br /><br />It has been named `WaitManyInternal()` as one of `Sock`'s purposes is mockability and `WaitMany()` (simply a passthrough to `WaitManyInternal()`) has been defined as a `virtual` function. |
| `Sock`'s usage of platform-specific APIs is *decided* exclusively at **compile-time**. | `Sock`'s usage of platform-specific APIs is determined by what is *supported* at compile-time and *decided* at **runtime** (mostly). | `Sock::Wait()` (which is transformed into `Sock::WaitMany()` in this pull request) supported only `poll` and `select` and behaved as described for Bitcoin.<br /><br />The described behaviour for Dash was only applicable for `CConnman::SocketEvents()`. But, as `SocketEvents()` is being replaced wholesale with `WaitMany()`, `WaitMany()` needed to be adapted to mirror `SocketEvents()` behaviour.<br /><br />This has resulted in changes that now require knowledge of the expected runtime SEM and file descriptor (if using an edge-triggered mode). |
| `Sock::Wait()` and `Sock::WaitMany()` behave **identically** | `Sock::Wait()` will respect the SEM selection argument **if** it is level-triggered **but** will fallback to `poll` or `select` (determined at compile-time) **if** the SEM selection is edge-triggered. | Due to the event-socket relationship of edge-triggered modes, they are unsuitable for querying the state of a *particular* socket.<br /><br />Because of that and **a)** the unlikelihood of the socket probed being registered with `EdgeTriggeredEvents::RegisterEntity()` and **b)** the overhead involved in fetching a list, filtering out for the particular socket we care about and flagging the result, it is more practical to use an LT-SEM instead. |
## How Has This Been Tested?
Correctness of `socket`, `poll` and `epoll` SEMs were tested using a Debian 12 (`bookworm`) Docker container with additional logging to ensure the correct syscalls were being made. Correctness of the `kqueue` SEM was tested using a GhostBSD 23.10.1 (based on FreeBSD 13.2-STABLE) virtual machine with similar additional logging.
## Breaking Changes
None expected. Behaviour should remain unchanged.
## Checklist
_Go over all the following points, and put an `x` in all the boxes that apply._
- [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
- [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:
UdjinM6:
re-utACK c611fb0
PastaPastaPasta:
utACK c611fb0
Tree-SHA512: 5daf093eafca94f4a3aad0ed4ee8b3d153c270b45294ef15c6b95bd83209a9bbc2212f88d1fe43c370b3e744e529c654c9530d7c0d7a0398bc0c3967fb362e5a
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Issue being fixed or feature implemented
#6007 follow-up
What was done?
How Has This Been Tested?
check
socketeventsingetnetworkinforesponseBreaking Changes
n/a
Checklist: