Skip to content

Commit 5e7fae7

Browse files
Merge dashpay#6018: backport: merge bitcoin#24356 (replace CConnman::SocketEvents() 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 dashpay#6004 * Dependent on dashpay#6007 * Dependent on dashpay#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
2 parents d9f52ac + c611fb0 commit 5e7fae7

File tree

17 files changed

+540
-469
lines changed

17 files changed

+540
-469
lines changed

src/i2p.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ bool Session::Accept(Connection& conn)
160160

161161
while (!*m_interrupt) {
162162
Sock::Event occurred;
163-
if (!conn.sock->Wait(MAX_WAIT_FOR_IO, Sock::RECV, &occurred)) {
163+
if (!conn.sock->Wait(MAX_WAIT_FOR_IO, Sock::RECV, SocketEventsParams{::g_socket_events_mode}, &occurred)) {
164164
errmsg = "wait on socket failed";
165165
break;
166166
}

src/init.cpp

Lines changed: 7 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -480,21 +480,6 @@ static void OnRPCStopped()
480480
LogPrint(BCLog::RPC, "RPC stopped.\n");
481481
}
482482

483-
std::string GetSupportedSocketEventsStr()
484-
{
485-
std::string strSupportedModes = "'select'";
486-
#ifdef USE_POLL
487-
strSupportedModes += ", 'poll'";
488-
#endif
489-
#ifdef USE_EPOLL
490-
strSupportedModes += ", 'epoll'";
491-
#endif
492-
#ifdef USE_KQUEUE
493-
strSupportedModes += ", 'kqueue'";
494-
#endif
495-
return strSupportedModes;
496-
}
497-
498483
void SetupServerArgs(ArgsManager& argsman)
499484
{
500485
SetupHelpOptions(argsman);
@@ -1642,6 +1627,12 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
16421627
}
16431628
}
16441629

1630+
std::string sem_str = args.GetArg("-socketevents", DEFAULT_SOCKETEVENTS);
1631+
::g_socket_events_mode = SEMFromString(sem_str);
1632+
if (::g_socket_events_mode == SocketEventsMode::Unknown) {
1633+
return InitError(strprintf(_("Invalid -socketevents ('%s') specified. Only these modes are supported: %s"), sem_str, GetSupportedSocketEventsStr()));
1634+
}
1635+
16451636
assert(!node.banman);
16461637
node.banman = std::make_unique<BanMan>(gArgs.GetDataDirNet() / "banlist", &uiInterface, args.GetIntArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
16471638
assert(!node.connman);
@@ -2420,6 +2411,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
24202411
connOptions.m_added_nodes = args.GetArgs("-addnode");
24212412
connOptions.nMaxOutboundLimit = *opt_max_upload;
24222413
connOptions.m_peer_connect_timeout = peer_connect_timeout;
2414+
connOptions.socketEventsMode = ::g_socket_events_mode;
24232415

24242416
// Port to bind to if `-bind=addr` is provided without a `:port` suffix.
24252417
const uint16_t default_bind_port =
@@ -2527,13 +2519,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
25272519
}
25282520
}
25292521

2530-
std::string sem_str = args.GetArg("-socketevents", DEFAULT_SOCKETEVENTS);
2531-
const auto sem = SEMFromString(sem_str);
2532-
if (sem == SocketEventsMode::Unknown) {
2533-
return InitError(strprintf(_("Invalid -socketevents ('%s') specified. Only these modes are supported: %s"), sem_str, GetSupportedSocketEventsStr()));
2534-
}
2535-
connOptions.socketEventsMode = sem;
2536-
25372522
const std::string& i2psam_arg = args.GetArg("-i2psam", "");
25382523
if (!i2psam_arg.empty()) {
25392524
const std::optional<CService> addr{Lookup(i2psam_arg, 7656, fNameLookup)};

src/masternode/node.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ void CActiveMasternodeManager::InitInternal(const CBlockIndex* pindex)
156156
// Check socket connectivity
157157
LogPrintf("CActiveMasternodeManager::Init -- Checking inbound connection to '%s'\n", m_info.service.ToStringAddrPort());
158158
std::unique_ptr<Sock> sock{ConnectDirectly(m_info.service, /*manual_connection=*/true)};
159-
bool fConnected{sock && sock->IsSelectable()};
159+
bool fConnected{sock && sock->IsSelectable(/*is_select=*/::g_socket_events_mode == SocketEventsMode::Select)};
160160
sock = std::make_unique<Sock>(INVALID_SOCKET);
161161
if (!fConnected && Params().RequireRoutableExternalIP()) {
162162
m_state = MasternodeState::SOME_ERROR;

0 commit comments

Comments
 (0)