Remove unnecessary casts when calling socket operations #33378

pull pinheadmz wants to merge 1 commits into bitcoin:master from pinheadmz:sockopt-cast changing 5 files +8 −16

pinheadmz commented at 4:55 PM on September 12, 2025: member
During review of #32747 several casting operations were questioned in existing code that had been copied or moved. That lead me to find a few other similar casts in the codebase.

It turns out that since the Sock class wraps syscalls with its own internal casting (see #24357 and #20788 written in 2020-2022) we no longer need to cast the arguments when calling these functions. The original argument-casts are old and were cleaned up a bit in #12855 written in 2018.

The casting is only needed for windows compatibility, where those syscalls require a data argument to be of type char* specifically:

https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-getsockopt
```
int getsockopt(
  [in]      SOCKET s,
  [in]      int    level,
  [in]      int    optname,
  [out]     char   *optval,
  [in, out] int    *optlen
);
```
but on POSIX the argument is void*:

https://www.man7.org/linux/man-pages/man2/getsockopt.2.html
```
       int getsockopt(socklen *restrict optlen;
                      int sockfd, int level, int optname,
                      void optval[_Nullable restrict *optlen],
                      socklen_t *restrict optlen);
```
DrahtBot commented at 4:55 PM on September 12, 2025: contributor


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



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33378.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK hodlinator, vasild, Raimo33, davidgumberg, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Type	Reviewers
ACK	hodlinator, vasild, Raimo33, davidgumberg, achow101

fanquake commented at 9:16 AM on September 13, 2025: member

https://github.com/bitcoin/bitcoin/actions/runs/17680889551/job/50254583517?pr=33378#step:12:357:

 D:\a\bitcoin\bitcoin\src\test\sock_tests.cpp(37): Entering test case "constructor_and_destructor"
2025-09-12T17:23:00.992709Z [unknown] [D:\a\bitcoin\bitcoin\src\test\util\random.cpp:48] [void __cdecl SeedRandomStateForTest(enum SeedRand)] Setting random seed for current tests to RANDOM_CTX_SEED=936d3154550e893420e302712f2c7f1031a35cebf124c36896c0cee3e5c2ece2
2025-09-12T17:23:00.994377Z [test] [D:\a\bitcoin\bitcoin\src\init\common.cpp:153] [void __cdecl init::LogPackageVersion(void)] Bitcoin Core version v30.99.0-5ee8bacfaf42 (release build)
2025-09-12T17:23:00.995687Z [test] [D:\a\bitcoin\bitcoin\src\kernel\context.cpp:20] [auto __cdecl kernel::Context::{ctor}::<lambda_1>::operator ()(void) const] Using the 'standard' SHA256 implementation
D:/a/bitcoin/bitcoin/src/test/sock_tests.cpp(42): error: in "sock_tests/constructor_and_destructor": check !SocketIsClosed(s) has failed
D:\a\bitcoin\bitcoin\src\test\sock_tests.cpp(37): Leaving test case "constructor_and_destructor"; testing time: 4943us

pinheadmz force-pushed on Sep 13, 2025
pinheadmz force-pushed on Sep 13, 2025
pinheadmz marked this as ready for review on Sep 15, 2025
pinheadmz requested review from hodlinator on Sep 15, 2025
pinheadmz requested review from vasild on Sep 15, 2025

in src/httpserver.cpp:404 in da23e2e2e1 outdated

 400 | @@ -401,7 +401,7 @@ static bool HTTPBindAddresses(struct evhttp* http)
 401 |              // Set the no-delay option (disable Nagle's algorithm) on the TCP socket.
 402 |              evutil_socket_t fd = evhttp_bound_socket_get_fd(bind_handle);
 403 |              int one = 1;
 404 | -            if (setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (sockopt_arg_type)&one, sizeof(one)) == SOCKET_ERROR) {
 405 | +            if (setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (char*)&one, sizeof(one)) == SOCKET_ERROR) {

hodlinator commented at 7:18 PM on September 15, 2025:

Agree that we can get rid of the typedef that is so rarely used.

Should probably use another kind of cast here and in sock_tests.cpp? https://github.com/bitcoin/bitcoin/blob/da23e2e2e104fc9d8e210c3989f2ae4945c072fa/doc/developer-notes.md?plain=1#L60-L63

pinheadmz commented at 10:26 AM on September 16, 2025:

right thanks!

net: remove unnecessary casts in socket operations

These methods in the Sock class wrap corresponding syscalls,
accepting void* arguments and casting to char* internally, which is
needed for Windows support and ignored on other platforms because
the syscall itself accepts void*:

Send()
Recv()
GetSockOpt()
SetSockOpt()

67f632b6de

pinheadmz force-pushed on Sep 16, 2025
hodlinator approved
hodlinator commented at 11:40 AM on September 16, 2025: contributor

ACK 67f632b6deb8b4aa190c458b71d2bc8c793626d5

(PR description still ends with "Opening as a draft for now to test CI on supported platforms.")
pinheadmz commented at 12:51 PM on September 16, 2025: member

Thanks for the edit @fanquake 🥰
vasild approved
vasild commented at 1:03 PM on September 16, 2025: contributor

ACK 67f632b6deb8b4aa190c458b71d2bc8c793626d5
Raimo33 commented at 12:02 AM on September 18, 2025: none

ACK 67f632b6deb8b4aa190c458b71d2bc8c793626d5
davidgumberg approved
davidgumberg commented at 1:06 AM on September 18, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/commit/67f632b6deb8b4aa190c458b71d2bc8c793626d5

As described in the commit messages, the Sock::(G|S)etSockOpt functions are already casting correctly, so there shouldn't be casting when using those functions. The only other users of (g|s)etsockopt are the httpserver (which doesn't use Sock):

https://github.com/bitcoin/bitcoin/blob/67f632b6deb8b4aa190c458b71d2bc8c793626d5/src/httpserver.cpp#L404

and the socket unit tests:

https://github.com/bitcoin/bitcoin/blob/67f632b6deb8b4aa190c458b71d2bc8c793626d5/src/test/sock_tests.cpp#L20-L28

SocketIsClosed() can't use Sock::GetSockOpt() since it's used to check the status of a SOCKET after e.g. the Sock object has been destructed:

https://github.com/bitcoin/bitcoin/blob/67f632b6deb8b4aa190c458b71d2bc8c793626d5/src/test/sock_tests.cpp#L37-L45
achow101 commented at 8:43 PM on September 18, 2025: member

ACK 67f632b6deb8b4aa190c458b71d2bc8c793626d5
achow101 merged this on Sep 18, 2025
achow101 closed this on Sep 18, 2025

Contributors

Linked (view graph)

#12855 net: Minor accumulated cleanups #20788 net: add RAII socket and use it instead of bare SOCKET #24357 refactor: make setsockopt() and SetSocketNoDelay() mockable/testable #32747 Introduce SockMan ("lite"): low-level socket handling for HTTP

Remove unnecessary casts when calling socket operations #33378

Code Coverage & Benchmarks

Reviews

Conflicts