refactor: move {`epoll`, `kqueue`} (de)init logic and wakeup pipes logic out of `CConnman` and into `EdgeTriggeredEvents` and `WakeupPipes`

kwvg commented 2 months ago

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 https://github.com/bitcoin/bitcoin/issues/19398) and moving socket-related logic to Sock (portions of which are aimed to be backported in #6004, tracking efforts from https://github.com/bitcoin/bitcoin/pull/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, d44b0d5dcb9b54821d582b267a8b92264be2da1b) 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 https://github.com/dashpay/dash/pull/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)

MrDefacto commented 2 months ago

I can do FreeBSD testing. I know that there are a couple hundred masternodes running on FreeBSD. I don't think it's unimportant.

UdjinM6 commented 2 months ago

works with no issues on mac (kqueue mode) it seems, a couple of suggestions: e0deb9e653 and maybe 4e7f80e1af

kwvg commented 2 months ago

@MrDefacto Messaged you on Keybase!

kwvg commented 2 months ago

a couple of suggestions: https://github.com/dashpay/dash/commit/e0deb9e653cb01edcd9c5a4536a1a6f2bbfa1dad and maybe https://github.com/dashpay/dash/commit/4e7f80e1af5896bcb6ce60d423cadaeea77642f0

With regards to e0deb9e653cb01edcd9c5a4536a1a6f2bbfa1dad , I was originally planning to move SocketEventsMode to util/sock.h in net_processing_6 (see https://github.com/kwvg/dash/commit/ed1409d88c65092b3d6a1f44d0cea30885abcd62) but getting rid of EdgeEventsMode and the need to sync enums was a good enough to reason to cherry-pick it. Thanks for the suggestion!

As for 4e7f80e1af5896bcb6ce60d423cadaeea77642f0, I'm leaning in the opposite direction and attempting to limit CConnman's knowledge of the macro (this PR still has a few uses of USE_{EPOLL, KQUEUE, POLL} that is done away with in the successor PR). The latest push (d5f0838) removes a few more USE_WAKEUP_PIPE usage and eventually am hoping to contain it to util/wpipes.cpp entirely.

Also, a condition inversion in WakeupPipe::Drain() has been resolved in the latest push (for some reason the infinite loop introduced didn't manifest in this PR but did in net_processing_6).

github-actions[bot] commented 2 months ago

This pull request has conflicts, please rebase.

dashpay / dash