sock: align exception monitoring between poll() and select() backends #36665

pull darosior wants to merge 1 commits into bitcoin:master from darosior:2610_sock_select_poll changing 1 files +4 −2

darosior commented at 1:59 PM on October 26, 2026: member

On non-Linux platforms, WaitMany monitors every socket for exceptional conditions, even when we are not looking to read from it. On Linux, it never monitors those.

Monitoring for certain socket events on some platforms but not others is inconsistent and surprising. In this instance it probably means these cases receive less testing as well, since they are ignored on our most well-tested platform.

Fix this issue by only monitoring for exceptional conditions (which is necessary to detect failed connection attempts early on Windows) for sockets we are also waiting to read from.
sock: align exception monitoring between poll() and select() backends 1e0c407377
DrahtBot commented at 1:59 PM on October 26, 2026: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



External sites
- Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36665
- Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36665.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36665#author-ralph


Reviews

See the guideline and AI policy for information on the review process.

Type Reviewers

ACK vasild, sedited, instagibbs, pinheadmz

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Type	Reviewers
ACK	vasild, sedited, instagibbs, pinheadmz

in src/util/sock.cpp:223 in 1e0c407377

 215 | @@ -216,12 +216,14 @@ bool Sock::WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_per
 216 |          const auto& s = sock->m_socket;
 217 |          if (events.requested & RecvEvent) {
 218 |              FD_SET(s, &recv);
 219 | +            FD_SET(s, &err);
 220 |          }
 221 |          if (events.requested & SendEvent) {
 222 |              FD_SET(s, &send);
 223 |          }
 224 | -        FD_SET(s, &err);

vasild commented at 2:50 PM on October 26, 2026:

Just a note to other reviewers: it looks fine to just drop FD_SET(s, &err); instead of moving it to under if (events.requested & RecvEvent). We do not expect and do not care about any "exceptional conditions":

from select(2):

exceptfds
       The  file  descriptors  in this set are watched for "exceptional conditions".  For examples of some excep‐
       tional conditions, see the discussion of POLLPRI in poll(2).

from poll(2):

 POLLPRI
       There is some exceptional condition on the file descriptor.  Possibilities include:
       •  There is out-of-band data on a TCP socket (see tcp(7)).
       •  A pseudoterminal master in packet mode has seen a state change on the slave (see ioctl_tty(2)).
       •  A cgroup.events file has been modified (see cgroups(7)).

Also, our poll() branch does not do POLLPRI.

However! https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-select:

exceptfds: If processing a connect call (nonblocking), connection attempt failed. OOB data is available for reading (only if SO_OOBINLINE is disabled).

So, FD_SET(s, &err); is needed on Windows to handle our non-blocking connect()s.

vasild approved
vasild commented at 3:08 PM on October 26, 2026: contributor

ACK 1e0c407377034e9cbb4d52f1f534c06cdf202a36

Indeed the poll(2) and select(2) branches seem to be inconsistent.

Thanks!
sedited approved
sedited commented at 3:17 PM on October 26, 2026: contributor

utACK 1e0c407377034e9cbb4d52f1f534c06cdf202a36
instagibbs approved
instagibbs commented at 3:41 PM on October 26, 2026: member

ACK 1e0c407377034e9cbb4d52f1f534c06cdf202a36

Validated a prefinal form in freebsd
pinheadmz approved

pinheadmz commented at 3:55 PM on October 26, 2026: member

ACK 1e0c407377034e9cbb4d52f1f534c06cdf202a36

built and tested on macos/arm64, clauded up a test that send an "exceptional" byte on master and compared to branch. Came to same conclusions as vasild in https://github.com/bitcoin/bitcoin/pull/36665/changes#r4378669650

This is a clean solution to the discrepency

<details><summary>Show Signature</summary>

pinheadmz's public key is on openpgp.org

</details>

fanquake merged this on Oct 26, 2026
fanquake closed this on Oct 26, 2026
fanquake referenced this in commit d7915a18de on Oct 26, 2026
fanquake commented at 4:08 PM on October 26, 2026: member

Backported to 32.x in #36658.
darosior deleted the branch on Oct 26, 2026
fanquake commented at 4:17 PM on October 26, 2026: member

Backported to 31.x in #36661.
fanquake referenced this in commit 9918ecdc06 on Oct 26, 2026
fanquake referenced this in commit d4a065bf0f on Oct 26, 2026
fanquake commented at 4:25 PM on October 26, 2026: member

Backported to 30.x in #36664.
fanquake commented at 4:25 PM on October 26, 2026: member

Backported to 29.x in #36666.
fanquake referenced this in commit b3526cf152 on Oct 26, 2026
fanquake referenced this in commit a4b4cefc14 on Oct 26, 2026

Contributors

Linked (view graph)

#36658 [32.x] Finalise 32.1 #36661 [31.x] Finalise 31.2 #36664 [30.x] Finalise 30.4 #36666 [29.x] Finalise 29.5

sock: align exception monitoring between poll() and select() backends #36665

External sites

Reviews