test: sock: Enable all socket tests on Windows

davidgumberg commented at 11:40 pm on September 29, 2025: contributor

Some class Sock tests were previously disabled because Windows lacks socketpair(2) which is used as a helper function in the socket tests, but is not strictly necessary or related to the Socket class under testing. This PR adds a CreateSocketPair() helper which creates a sender socket and receiver socket with a TCP connection, enabling these test cases for Windows. This also enables future tests that require more granular control over sockets than what socketpair() allows for, like using setsockopt() before connecting a socket.

This change is generally an improvement, but is also broken out of a branch that does compact block prefilling up to the available bytes in the connection’s current TCP window (see delving post). Creating connected socket pairs is useful for added tests in that branch that validate querying the current TCP window state, and without this change those tests don’t run on Windows.

DrahtBot added the label Tests on Sep 29, 2025

DrahtBot commented at 11:40 pm on September 29, 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/33506.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sedited
Stale ACK	laanwj, w0xlt

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

sender.Send(msg, msg_len, 0) in src/test/sock_tests.cpp
receiver.Recv(recv_buf, sizeof(recv_buf), 0) in src/test/sock_tests.cpp
socks.sender.Send(“a”, 1, 0) in src/test/sock_tests.cpp

2026-03-05 20:22:07

bitcoin deleted a comment on Sep 30, 2025

davidgumberg force-pushed on Sep 30, 2025

davidgumberg commented at 7:34 pm on September 30, 2025: contributor

Rebased to split into two commits, a mostly refactor commit https://github.com/bitcoin/bitcoin/pull/33506/commits/06a5140e96a75948cd2e490b414419a5485f2cfd and the commit that adds the new CreateSocketPair() for windows and enables Windows socket tests https://github.com/bitcoin/bitcoin/pull/33506/commits/7586205c4a5e94c7e2357d8928c68dfab95dc47a

w0xlt commented at 11:49 pm on September 30, 2025: contributor

Concept ACK

davidgumberg force-pushed on Oct 1, 2025

laanwj requested review from laanwj on Oct 23, 2025

laanwj commented at 2:11 pm on October 23, 2025: member

Concept ACK. Should we keep using socketpair on platforms that support it? (One reason to do this is that TCP ports might be heavily contended resource on CI systems, though it should be okay)

in src/test/sock_tests.cpp:111 in 95775080da outdated

123+        BOOST_REQUIRE_EQUAL(receiver->Listen(1), 0);
124+
125+        // Get the address of the listener.
126+        sockaddr_in bound{};
127+        socklen_t blen = sizeof(bound);
128+        BOOST_REQUIRE_EQUAL(receiver->GetSockName(reinterpret_cast<sockaddr*>(&bound), &blen), 0);

laanwj commented at 2:15 pm on October 23, 2025:

Might want to check blen == sizeof(bound) after this as well

davidgumberg commented at 6:23 pm on December 10, 2025:

Thanks, I’ve taken this in 15cefcd7d3729b3af59f63b1ba406b5dfb405e99 https://github.com/bitcoin/bitcoin/blob/15cefcd7d3729b3af59f63b1ba406b5dfb405e99/src/test/sock_tests.cpp#L108-L112

davidgumberg force-pushed on Dec 10, 2025

davidgumberg commented at 6:44 pm on December 10, 2025: contributor

[9577508..15cefcd] Pushed to address @laanwj’s suggestion..

Should we keep using socketpair on platforms that support it? (One reason to do this is that TCP ports might be heavily contended resource on CI systems, though it should be okay)

My gut reaction is that it would not be worth the extra code to do this, but I don’t feel very strongly and am happy to add this if you think it’s better.

laanwj commented at 8:19 pm on December 16, 2025: member

My gut reaction is that it would not be worth the extra code to do this, but I don’t feel very strongly and am happy to add this if you think it’s better.

Not 100% sure. As long as this is only used for the tests, like now, i don’t mind it being like this. But if it were to be used for anything in the actual code, i think a socket pair should be a socket pair. Using a fallback work-around for functionality that’s plainly supported seems strange.

laanwj approved

laanwj commented at 8:24 pm on December 16, 2025: member

Code review ACK 15cefcd7d3729b3af59f63b1ba406b5dfb405e99

DrahtBot requested review from w0xlt on Dec 16, 2025

davidgumberg commented at 10:38 pm on December 16, 2025: contributor

My gut reaction is that it would not be worth the extra code to do this, but I don’t feel very strongly and am happy to add this if you think it’s better.

Not 100% sure. As long as this is only used for the tests, like now, i don’t mind it being like this. But if it were to be used for anything in the actual code, i think a socket pair should be a socket pair. Using a fallback work-around for functionality that’s plainly supported seems strange.

100% agree that if we were using socketpair() in non-test code that we should do this. I also recall now that part of the reason I’ve done it this way is that some of the tests I’m adding in my branch depend on being able to create and connect the sockets separately, and making TCP sockets instead of UNIX sockets which can’t be done with socketpair(). This is useful so that e.g. sockopt’s can be set that matter for the connection handshake: https://github.com/bitcoin/bitcoin/blob/132d41a220c8e99ecf4b0c60a144606d32dd28ae/src/test/sock_tests.cpp#L195-L210

Even though it’s probably a bad idea to make changes in the present to suit a future use case that might never come about, I think it is generally useful to be able to create and connect the sockets separately in sock_tests.cpp and to be able to create TCP sockets like the application does.

laanwj commented at 9:03 am on December 17, 2025: member

Even though it’s probably a bad idea to make changes in the present to suit a future use case that might never come about, I think it is generally useful to be able to create and connect the sockets separately in sock_tests.cpp and to be able to create TCP sockets like the application does.

If it’s intentionally TCP then calling it tcp_socket_pair might be even better. So that no one decides to make this improvement in the meantime.

davidgumberg force-pushed on Dec 19, 2025

davidgumberg commented at 0:37 am on December 19, 2025: contributor

calling it tcp_socket_pair might be even better

Thanks, fixed!

in src/test/sock_tests.cpp:114 in 9553f7872b

126+        sockaddr_in bound{};
127+        socklen_t blen = sizeof(bound);
128+        BOOST_REQUIRE_EQUAL(receiver->GetSockName(reinterpret_cast<sockaddr*>(&bound), &blen), 0);
129+        BOOST_REQUIRE_EQUAL(blen, sizeof(bound));
130+
131+        // Sender attempts to initiate connection to listener.

sedited commented at 4:40 pm on March 4, 2026:

I don’t think these comments add anything.

davidgumberg commented at 7:04 pm on March 4, 2026:

Done, thanks, I left in the comment for the part of the socket api I find the most opaque / difficult to read, but I can remove that too if you think it’s no help:

https://github.com/bitcoin/bitcoin/blob/5dc3352be41a874272e92dd61f47bb08c386f523/src/test/sock_tests.cpp#L108-L112

sedited approved

sedited commented at 4:54 pm on March 4, 2026: contributor

Isn’t the usage of a socket pair kind of incidental to the test here? We do use unix sockets in a bunch of places now, but not through the socket class. From what I glean from the original PR, the purpose really is primarily to test the socket class itself (https://github.com/bitcoin/bitcoin/pull/20788/changes/615ba0eb96cf131364c1ceca9d3dedf006fa1e1c), so this change makes sense to me. I think it would be good to mention that in the PR description.

davidgumberg renamed this:
~~test: sock: Enable socket pair tests on Windows~~
test: sock: Enable all socket tests on Windows
on Mar 4, 2026

davidgumberg force-pushed on Mar 4, 2026

davidgumberg commented at 7:05 pm on March 4, 2026: contributor

Isn’t the usage of a socket pair kind of incidental to the test here? […] I think it would be good to mention that in the PR description.

Updated PR title and description to communicate this better.

in src/test/sock_tests.cpp:100 in 5dc3352be4

109-    BOOST_CHECK_EQUAL(receiver.Recv(recv_buf, sizeof(recv_buf), 0), msg_len);
110-    BOOST_CHECK_EQUAL(strncmp(msg, recv_buf, msg_len), 0);
111-}
112+    void connect()
113+    {
114+        sockaddr_in addr;

w0xlt commented at 7:27 pm on March 4, 2026:

0        sockaddr_in addr{};

davidgumberg commented at 8:18 pm on March 4, 2026:

Done, thanks.

in src/test/sock_tests.cpp:136 in 5dc3352be4 outdated

160 
161-    delete sock0moved;
162-    delete sock1moved;
163+BOOST_AUTO_TEST_CASE(send_and_receive)
164+{
165+    tcp_socket_pair socks = tcp_socket_pair::create(/*connect=*/true);

w0xlt commented at 7:29 pm on March 4, 2026:

nit:

0    tcp_socket_pair socks = tcp_socket_pair::create();

davidgumberg commented at 8:18 pm on March 4, 2026:

fair enough, I’ve gone the other direction and removed the (unused) default, since I think it should be legible at the callsite whether or not the sockets are connected.

in src/test/sock_tests.cpp:98 in 5dc3352be4

107 
108-    BOOST_CHECK_EQUAL(sender.Send(msg, msg_len, 0), msg_len);
109-    BOOST_CHECK_EQUAL(receiver.Recv(recv_buf, sizeof(recv_buf), 0), msg_len);
110-    BOOST_CHECK_EQUAL(strncmp(msg, recv_buf, msg_len), 0);
111-}
112+    void connect()

w0xlt commented at 7:32 pm on March 4, 2026:

nit: The method name connect shadows the POSIX/Winsock connect() function.

0    void connect_pair()

davidgumberg commented at 8:17 pm on March 4, 2026:

Thanks, fixed.

w0xlt commented at 7:45 pm on March 4, 2026: contributor

ACK 5dc3352be41a874272e92dd61f47bb08c386f523

DrahtBot requested review from laanwj on Mar 4, 2026

davidgumberg force-pushed on Mar 4, 2026

DrahtBot added the label CI failed on Mar 4, 2026

w0xlt commented at 8:21 pm on March 4, 2026: contributor

reACK 0a8f5199048d56066bf87ea9e8f5b584b968140b

in src/test/sock_tests.cpp:140 in 0a8f519904

166+    socks.send_and_receive();
167 
168-    BOOST_CHECK(SocketIsClosed(s[0]));
169-    BOOST_CHECK(SocketIsClosed(s[1]));
170+    // Sockets are still connected after being moved.
171+    tcp_socket_pair socks_moved = std::move(socks);

sedited commented at 8:49 pm on March 4, 2026:

Isn’t this just checking the move constructor for the unique pointers now, instead of exercising the actual move constructor?

davidgumberg commented at 7:18 pm on March 5, 2026:

My mistake, thanks for catching this: I’ve changed TcpSocketPair()::receiver and sender to be Sock instead of unique_ptr<Sock> so we test the sock move rather than the pointer move, and I’ve added a test-the-test to make sure that the Socks are moved:

https://github.com/bitcoin/bitcoin/blob/4e9d6c719c144365df3ec2e5869cd781f4fb4372/src/test/sock_tests.cpp#L141-L145

sedited commented at 8:53 pm on March 4, 2026: contributor

There is some simple style cleanup that could be done here: Using UpperCamelCase for clases, structs, and functions, dropping static methods in favour of a constructor, and dropping the seemingly unused connect bool:

 0diff --git a/src/test/sock_tests.cpp b/src/test/sock_tests.cpp
 1index 0cd5072758..2353b1b933 100644
 2--- a/src/test/sock_tests.cpp
 3+++ b/src/test/sock_tests.cpp
 4@@ -84 +84 @@ BOOST_AUTO_TEST_CASE(move_assignment)
 5-struct tcp_socket_pair {
 6+struct TcpSocketPair {
 7@@ -88 +88,3 @@ struct tcp_socket_pair {
 8-    static tcp_socket_pair create(bool connect)
 9+    TcpSocketPair()
10+        : sender{std::make_unique<Sock>(CreateSocket())},
11+        receiver{std::make_unique<Sock>(CreateSocket())}
12@@ -90,6 +92 @@ struct tcp_socket_pair {
13-        tcp_socket_pair socks{};
14-        socks.sender = std::make_unique<Sock>(CreateSocket());
15-        socks.receiver = std::make_unique<Sock>(CreateSocket());
16-        if (connect) socks.connect_pair();
17-
18-        return socks;
19+        Connect();
20@@ -98 +95 @@ struct tcp_socket_pair {
21-    void connect_pair()
22+    void Connect()
23@@ -122 +119 @@ struct tcp_socket_pair {
24-    void send_and_receive()
25+    void SendAndReceive()
26@@ -136,2 +133,2 @@ BOOST_AUTO_TEST_CASE(send_and_receive)
27-    tcp_socket_pair socks = tcp_socket_pair::create(/*connect=*/true);
28-    socks.send_and_receive();
29+    TcpSocketPair socks;
30+    socks.SendAndReceive();
31@@ -140,2 +137,2 @@ BOOST_AUTO_TEST_CASE(send_and_receive)
32-    tcp_socket_pair socks_moved = std::move(socks);
33-    socks_moved.send_and_receive();
34+    TcpSocketPair socks_moved = std::move(socks);
35+    socks_moved.SendAndReceive();
36@@ -146 +143 @@ BOOST_AUTO_TEST_CASE(wait)
37-    tcp_socket_pair socks = tcp_socket_pair::create(/*connect=*/true);
38+    TcpSocketPair socks;
39@@ -160 +157 @@ BOOST_AUTO_TEST_CASE(recv_until_terminator_limit)
40-    tcp_socket_pair socks = tcp_socket_pair::create(/*connect=*/true);
41+    TcpSocketPair socks;

DrahtBot removed the label CI failed on Mar 5, 2026

davidgumberg force-pushed on Mar 5, 2026

DrahtBot added the label CI failed on Mar 5, 2026

davidgumberg commented at 7:22 pm on March 5, 2026: contributor

There is some simple style cleanup that could be done here: Using UpperCamelCase for clases, structs, and functions, dropping static methods in favour of a constructor, and dropping the seemingly unused connect bool:

Thanks for the suggestions, I’ve taken most of them, the reason for the unused connect bool is the branch where this commit was originally extracted from but you are right to call for it’s removal, I can add it when I add the code that uses it.

test: sock: Enable socket pair tests on Windows

Adds a helper struct `socket_pair` for constructing and connecting TCP
sockets, this replaces socketpair() which is not available on Windows.
Making the socket pair TCP sockets instead of Unix sockets, and
separating socket creation from socket connection also enables more
detailed tests to be added in the future.

9316d96240

davidgumberg force-pushed on Mar 5, 2026

in src/test/sock_tests.cpp:98 in 9316d96240

110+    }
111+
112+    TcpSocketPair(const TcpSocketPair&) = delete;
113+    TcpSocketPair& operator= (const TcpSocketPair&) = delete;
114+    TcpSocketPair(TcpSocketPair&&) = default;
115+    TcpSocketPair& operator= (TcpSocketPair&&) = default;

sedited commented at 8:42 pm on March 5, 2026:

Are these actually needed?

sedited approved

sedited commented at 8:42 pm on March 5, 2026: contributor

ACK 9316d96240ff1bd4c98a823022655c44a04d1b20

DrahtBot requested review from w0xlt on Mar 5, 2026

test: sock: Enable all socket tests on Windows #33506

Code Coverage & Benchmarks

Reviews

LLM Linter (✨ experimental)