Wow great work @vasild! Love it. Will review.

Super strong Concept ACK obviously :)

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• p2p, refactor: make NetPermissionFlags an enum class #21506 (p2p, refactor: make NetPermissionFlags an enum class by jonatack)

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.

386676a9f...6b86187a8: use the proper socket variable after std::move

Random note:

AFAICT the only remaining non-test usage of SOCKET Socket::Get() after this PR is this call in CreateSockTCP(…):

    // Set the non-blocking option on the socket.
    if (!SetSocketNonBlocking(sock->Get(), true)) {

What about adding Socket::SetNonBlocking(bool) as part of this PR? That would allow us to drop Socket::Get (from sock.h at least), which in turn would mean that there is no risk of a SOCKET "escaping" Socket (other than using Socket::Release, and Socket::Release makes is clear that the responsibility of closing is taken over by the caller).

WDYT? :)

I actually did that, but then removed this change because I realized it is not necessary - CreateSockTCP() is never supposed to be fuzz- or unit- tested. In fuzz/unit tests we override the global CreateSock() to do something else than the real CreateSockTCP().

I think it is still useful to have Get(). If removed then what about the unit tests that use it? Maybe if at some point Sock is used all over the place, then both Get() and Release() could be removed.

More random notes:

These are the direct uses of socket related syscalls and library functions with this PR applied:

$ git grep -E '[^a-z_:.](accept|bind|connect|getsockopt|listen|poll|recv|select|send|setsockopt|socket)\(' -- \
      "src/" ":(exclude)src/compat/stdin.cpp" ":(exclude)src/qt/" ":(exclude)src/test/" \
      ":(exclude)src/wallet/" | grep -vE "\.(cpp|h): *(//|\*|Log|\"|ListenSocket|throw|strprintf)"
src/net.cpp:            nBytes = send(node.hSocket, reinterpret_cast<const char*>(data.data()) + node.nSendOffset, data.size() - node.nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT);
src/net.cpp:    SOCKET hSocket = accept(hListenSocket.socket, (struct sockaddr*)&sockaddr, &len);
src/net.cpp:    if (poll(vpollfds.data(), vpollfds.size(), SELECT_TIMEOUT_MILLISECONDS) < 0) return;
src/net.cpp:    int nSelect = select(hSocketMax + 1, &fdsetRecv, &fdsetSend, &fdsetError, &timeout);
src/net.cpp:                nBytes = recv(pnode->hSocket, (char*)pchBuf, sizeof(pchBuf), MSG_DONTWAIT);
src/netbase.cpp:    SOCKET hSocket = socket(((struct sockaddr*)&sockaddr)->sa_family, SOCK_STREAM, IPPROTO_TCP);
src/util/sock.cpp:    return send(m_socket, static_cast<const char*>(data), len, flags);
src/util/sock.cpp:    return recv(m_socket, static_cast<char*>(buf), len, flags);
src/util/sock.cpp:    return connect(m_socket, addr, addr_len);
src/util/sock.cpp:    return bind(m_socket, addr, addr_len);
src/util/sock.cpp:    return listen(m_socket, backlog);
src/util/sock.cpp:    return getsockopt(m_socket, level, opt_name, static_cast<char*>(opt_val), opt_len);
src/util/sock.cpp:    return setsockopt(m_socket, level, opt_name, static_cast<const char*>(opt_val), opt_len);
src/util/sock.cpp:    if (poll(&fd, 1, count_milliseconds(timeout)) == SOCKET_ERROR) {
src/util/sock.cpp:    if (select(m_socket + 1, &fdset_recv, &fdset_send, nullptr, &timeout_struct) == SOCKET_ERROR) {

Great to see our socket related syscalls and library functions being further encapsulated by Socket (src/util/sock.cpp). It simplifies fuzzing, it simplifies syscall analysis and generally makes our socket use easier to reason about. Great stuff! :)

The remaining src/net.cpp cases are probably out of scope for this PR, but what about moving CreateSockTCP from src/netbase.{cpp,h} to src/util/sock.{cpp,h}. I think it belongs there and it would then make src/net.cpp the only remaining place where we make direct use of socket related syscalls and library functions without using Socket. WDYT @vasild?

Moving CreateSockTCP() - it makes sense if it works (e.g. does not create a circular dependency).

Looking at the above list - it is not that many left! :) (btw close(2) is missing)

6b86187a8...9155dee01: use pointers to Sock so that mocking via inheritance works properly

Here is a patch that removes some unused code (+22 -30), further encapsulates socket operations to Sock (src/util/sock.{cpp,h}) and gets rid of the last non-test use of Sock::Get.

Feel free to use as you wish! :)

diff --git a/src/netbase.cpp b/src/netbase.cpp
index b03fd1028..69ef63b0e 100644
--- a/src/netbase.cpp
+++ b/src/netbase.cpp
@@ -303,8 +303,7 @@ enum class IntrRecvError {
  *          read.
  *
  * @see This function can be interrupted by calling InterruptSocks5(bool).
- *      Sockets can be made non-blocking with SetSocketNonBlocking(const
- *      SOCKET&, bool).
+ *      Sockets can be made non-blocking with Sock::SetNonBlocking().
  */
 static IntrRecvError InterruptibleRecv(uint8_t* data, size_t len, int timeout, const Sock& sock)
 {
@@ -520,7 +519,7 @@ std::unique_ptr<Sock> CreateSockTCP(const CService& address_family)
     }

     // Set the non-blocking option on the socket.
-    if (!SetSocketNonBlocking(sock->Get(), true)) {
+    if (!sock->SetNonBlocking()) {
         LogPrintf("Error setting socket to non-blocking: %s\n", NetworkErrorString(WSAGetLastError()));
         return nullptr;
     }
@@ -718,33 +717,6 @@ bool LookupSubNet(const std::string& strSubnet, CSubNet& ret, DNSLookupFn dns_lo
     return false;
 }

-bool SetSocketNonBlocking(const SOCKET& hSocket, bool fNonBlocking)
-{
-    if (fNonBlocking) {
-#ifdef WIN32
-        u_long nOne = 1;
-        if (ioctlsocket(hSocket, FIONBIO, &nOne) == SOCKET_ERROR) {
-#else
-        int fFlags = fcntl(hSocket, F_GETFL, 0);
-        if (fcntl(hSocket, F_SETFL, fFlags | O_NONBLOCK) == SOCKET_ERROR) {
-#endif
-            return false;
-        }
-    } else {
-#ifdef WIN32
-        u_long nZero = 0;
-        if (ioctlsocket(hSocket, FIONBIO, &nZero) == SOCKET_ERROR) {
-#else
-        int fFlags = fcntl(hSocket, F_GETFL, 0);
-        if (fcntl(hSocket, F_SETFL, fFlags & ~O_NONBLOCK) == SOCKET_ERROR) {
-#endif
-            return false;
-        }
-    }
-
-    return true;
-}
-
 void InterruptSocks5(bool interrupt)
 {
     interruptSocks5Recv = interrupt;
diff --git a/src/util/sock.cpp b/src/util/sock.cpp
index e560f5b90..7f9b6d17c 100644
--- a/src/util/sock.cpp
+++ b/src/util/sock.cpp
@@ -107,6 +107,20 @@ bool Sock::SetNoDelay() const
     return SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) == 0;
 }

+bool Sock::SetNonBlocking() const
+{
+#ifdef WIN32
+    const u_long one{1};
+    if (ioctlsocket(m_socket, FIONBIO, &nOne) == SOCKET_ERROR) {
+#else
+    const int flags{fcntl(m_socket, F_GETFL, 0)};
+    if (fcntl(m_socket, F_SETFL, flags | O_NONBLOCK) == SOCKET_ERROR) {
+#endif
+        return false;
+    }
+    return true;
+}
+
 bool Sock::IsSelectable() const
 {
 #if defined(USE_POLL) || defined(WIN32)
diff --git a/src/util/sock.h b/src/util/sock.h
index 0ce2decd8..9e33b9159 100644
--- a/src/util/sock.h
+++ b/src/util/sock.h
@@ -140,6 +140,12 @@ public:
      */
     [[nodiscard]] virtual bool SetNoDelay() const;

+    /**
+     * Set the non-blocking option on the socket.
+     * @return true if set successfully
+     */
+    [[nodiscard]] virtual bool SetNonBlocking() const;
+
     /**
      * Check if the underlying socket can be used for `select(2)` (or the `Wait()` method).
      * @return true if selectable

9155dee01...6a91cac15:

• take the above patch that adds Sock::SetNonBlocking()
• remove the newly added tests - they require "all or nothing" - to remove all occurrences of Sock::Get() and Sock::Release() otherwise dummy values of Sock::m_socket from the mocked Sock implementation leak into the main code during fuzzing.

This PR is reviewable and mergeable in its current state. I will see what it takes to remove Sock::Get() and Sock::Release() and decide whether to append to this PR or file a separate one.

6a91cac15...67097b29c: fix Windows compilation

Concept ACK

Closing: superseded by #21878 which replaces all usage of SOCKET with Sock and removes Sock::Get() and Sock::Release(), making it impossible to leak dummy file descriptors from mocked Sock implementations into the real code during tests (and accidentally call close(), write() or whatever on the dummy fds during tests).