Merge bitcoin/bitcoin#24357: refactor: make setsockopt() and SetSocketNoDelay() mockable/testable

a2c4a7acd1dfb2fb7e3c9dac6b3d8c9354b2e0a6 net: use Sock::SetSockOpt() instead of standalone SetSocketNoDelay() (Vasil Dimov)
d65b6c3fb9cdd41fa53bc76a7b8f49aaa089b0bc net: use Sock::SetSockOpt() instead of setsockopt() (Vasil Dimov)
184e56d6683d05fc84f5153cfff83a2e32883556 net: add new method Sock::SetSockOpt() that wraps setsockopt() (Vasil Dimov)

Pull request description:

  _This is a piece of #21878, chopped off to ease review._

  Add a `virtual` (thus mockable) method `Sock::SetSockOpt()` that wraps the system `setsockopt()`.

  Convert the standalone `SetSocketNoDelay()` function to a `virtual` (thus mockable) method `Sock::SetNoDelay()`.

  This will help avoid syscalls during testing and to mock them to return whatever is suitable for the tests.

ACKs for top commit:
  laanwj:
    Code review ACK a2c4a7acd1dfb2fb7e3c9dac6b3d8c9354b2e0a6
  jonatack:
    ACK a2c4a7acd1dfb2fb7e3c9dac6b3d8c9354b2e0a6 change since last review is folding `Sock::SetNoDelay()` into the callers

Tree-SHA512: 3e2b016c1e4128317a28c17dc9b30472949e1ac3b071b2697c6d30cbcc830df1ee4392a4e23b2ea1ab4e3fb0f59ef450e2a4f3c1df3d8c803dd081652b6c7387

8 files changed, 62 insertions, 20 deletions

diff --git a/src/net.cpp b/src/net.cpp
index 602d56ab98..5a0c057c3e 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1190,7 +1190,11 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
 
     // According to the internet TCP_NODELAY is not carried into accepted sockets
     // on all platforms.  Set it again here just to be sure.
-    SetSocketNoDelay(sock->Get());
+    const int on{1};
+    if (sock->SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) == SOCKET_ERROR) {
+        LogPrint(BCLog::NET, "connection from %s: unable to set TCP_NODELAY, continuing anyway\n",
+                 addr.ToString());
+    }
 
     // Don't accept connections from banned peers.
     bool banned = m_banman && m_banman->IsBanned(addr);
@@ -2395,17 +2399,26 @@ bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError,
 
     // Allow binding if the port is still in TIME_WAIT state after
     // the program was closed and restarted.
-    setsockopt(sock->Get(), SOL_SOCKET, SO_REUSEADDR, (sockopt_arg_type)&nOne, sizeof(int));
+    if (sock->SetSockOpt(SOL_SOCKET, SO_REUSEADDR, (sockopt_arg_type)&nOne, sizeof(int)) == SOCKET_ERROR) {
+        strError = strprintf(Untranslated("Error setting SO_REUSEADDR on socket: %s, continuing anyway"), NetworkErrorString(WSAGetLastError()));
+        LogPrintf("%s\n", strError.original);
+    }
 
     // some systems don't have IPV6_V6ONLY but are always v6only; others do have the option
     // and enable it by default or not. Try to enable it, if possible.
     if (addrBind.IsIPv6()) {
 #ifdef IPV6_V6ONLY
-        setsockopt(sock->Get(), IPPROTO_IPV6, IPV6_V6ONLY, (sockopt_arg_type)&nOne, sizeof(int));
+        if (sock->SetSockOpt(IPPROTO_IPV6, IPV6_V6ONLY, (sockopt_arg_type)&nOne, sizeof(int)) == SOCKET_ERROR) {
+            strError = strprintf(Untranslated("Error setting IPV6_V6ONLY on socket: %s, continuing anyway"), NetworkErrorString(WSAGetLastError()));
+            LogPrintf("%s\n", strError.original);
+        }
 #endif
 #ifdef WIN32
         int nProtLevel = PROTECTION_LEVEL_UNRESTRICTED;
-        setsockopt(sock->Get(), IPPROTO_IPV6, IPV6_PROTECTION_LEVEL, (const char*)&nProtLevel, sizeof(int));
+        if (sock->SetSockOpt(IPPROTO_IPV6, IPV6_PROTECTION_LEVEL, (const char*)&nProtLevel, sizeof(int)) == SOCKET_ERROR) {
+            strError = strprintf(Untranslated("Error setting IPV6_PROTECTION_LEVEL on socket: %s, continuing anyway"), NetworkErrorString(WSAGetLastError()));
+            LogPrintf("%s\n", strError.original);
+        }
 #endif
     }
 
diff --git a/src/netbase.cpp b/src/netbase.cpp
index e6d4f16ba0..9557297df1 100644
--- a/src/netbase.cpp
+++ b/src/netbase.cpp
@@ -499,10 +499,11 @@ std::unique_ptr<Sock> CreateSockTCP(const CService& address_family)
         return nullptr;
     }
 
+    auto sock = std::make_unique<Sock>(hSocket);
+
     // Ensure that waiting for I/O on this socket won't result in undefined
     // behavior.
-    if (!IsSelectableSocket(hSocket)) {
-        CloseSocket(hSocket);
+    if (!IsSelectableSocket(sock->Get())) {
         LogPrintf("Cannot create connection: non-selectable socket created (fd >= FD_SETSIZE ?)\n");
         return nullptr;
     }
@@ -511,19 +512,24 @@ std::unique_ptr<Sock> CreateSockTCP(const CService& address_family)
     int set = 1;
     // Set the no-sigpipe option on the socket for BSD systems, other UNIXes
     // should use the MSG_NOSIGNAL flag for every send.
-    setsockopt(hSocket, SOL_SOCKET, SO_NOSIGPIPE, (void*)&set, sizeof(int));
+    if (sock->SetSockOpt(SOL_SOCKET, SO_NOSIGPIPE, (void*)&set, sizeof(int)) == SOCKET_ERROR) {
+        LogPrintf("Error setting SO_NOSIGPIPE on socket: %s, continuing anyway\n",
+                  NetworkErrorString(WSAGetLastError()));
+    }
 #endif
 
     // Set the no-delay option (disable Nagle's algorithm) on the TCP socket.
-    SetSocketNoDelay(hSocket);
+    const int on{1};
+    if (sock->SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) == SOCKET_ERROR) {
+        LogPrint(BCLog::NET, "Unable to set TCP_NODELAY on a newly created socket, continuing anyway\n");
+    }
 
     // Set the non-blocking option on the socket.
-    if (!SetSocketNonBlocking(hSocket)) {
-        CloseSocket(hSocket);
+    if (!SetSocketNonBlocking(sock->Get())) {
         LogPrintf("Error setting socket to non-blocking: %s\n", NetworkErrorString(WSAGetLastError()));
         return nullptr;
     }
-    return std::make_unique<Sock>(hSocket);
+    return sock;
 }
 
 std::function<std::unique_ptr<Sock>(const CService&)> CreateSock = CreateSockTCP;
@@ -726,13 +732,6 @@ bool SetSocketNonBlocking(const SOCKET& hSocket)
     return true;
 }
 
-bool SetSocketNoDelay(const SOCKET& hSocket)
-{
-    int set = 1;
-    int rc = setsockopt(hSocket, IPPROTO_TCP, TCP_NODELAY, (const char*)&set, sizeof(int));
-    return rc == 0;
-}
-
 void InterruptSocks5(bool interrupt)
 {
     interruptSocks5Recv = interrupt;
diff --git a/src/netbase.h b/src/netbase.h
index c226e6b73d..bf7522210d 100644
--- a/src/netbase.h
+++ b/src/netbase.h
@@ -223,8 +223,6 @@ bool ConnectThroughProxy(const Proxy& proxy, const std::string& strDest, uint16_
 
 /** Enable non-blocking mode for a socket */
 bool SetSocketNonBlocking(const SOCKET& hSocket);
-/** Set the TCP_NODELAY flag on a socket */
-bool SetSocketNoDelay(const SOCKET& hSocket);
 void InterruptSocks5(bool interrupt);
 
 /**
diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp
index 6766fbf2d9..033c6e18d5 100644
--- a/src/test/fuzz/util.cpp
+++ b/src/test/fuzz/util.cpp
@@ -193,6 +193,19 @@ int FuzzedSock::GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* op
     return 0;
 }
 
+int FuzzedSock::SetSockOpt(int, int, const void*, socklen_t) const
+{
+    constexpr std::array setsockopt_errnos{
+        ENOMEM,
+        ENOBUFS,
+    };
+    if (m_fuzzed_data_provider.ConsumeBool()) {
+        SetFuzzedErrNo(m_fuzzed_data_provider, setsockopt_errnos);
+        return -1;
+    }
+    return 0;
+}
+
 bool FuzzedSock::Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred) const
 {
     constexpr std::array wait_errnos{
diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h
index 6c91844633..580105e442 100644
--- a/src/test/fuzz/util.h
+++ b/src/test/fuzz/util.h
@@ -68,6 +68,8 @@ public:
 
     int GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const override;
 
+    int SetSockOpt(int level, int opt_name, const void* opt_val, socklen_t opt_len) const override;
+
     bool Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred = nullptr) const override;
 
     bool IsConnected(std::string& errmsg) const override;
diff --git a/src/test/util/net.h b/src/test/util/net.h
index 20c45058a1..e980fe4967 100644
--- a/src/test/util/net.h
+++ b/src/test/util/net.h
@@ -150,6 +150,8 @@ public:
         return 0;
     }
 
+    int SetSockOpt(int, int, const void*, socklen_t) const override { return 0; }
+
     bool Wait(std::chrono::milliseconds timeout,
               Event requested,
               Event* occurred = nullptr) const override
diff --git a/src/util/sock.cpp b/src/util/sock.cpp
index 2029d70a37..b5c1e28294 100644
--- a/src/util/sock.cpp
+++ b/src/util/sock.cpp
@@ -105,6 +105,11 @@ int Sock::GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len)
     return getsockopt(m_socket, level, opt_name, static_cast<char*>(opt_val), opt_len);
 }
 
+int Sock::SetSockOpt(int level, int opt_name, const void* opt_val, socklen_t opt_len) const
+{
+    return setsockopt(m_socket, level, opt_name, static_cast<const char*>(opt_val), opt_len);
+}
+
 bool Sock::Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred) const
 {
 #ifdef USE_POLL
diff --git a/src/util/sock.h b/src/util/sock.h
index 7510482857..dd2913a66c 100644
--- a/src/util/sock.h
+++ b/src/util/sock.h
@@ -115,6 +115,16 @@ public:
                                          void* opt_val,
                                          socklen_t* opt_len) const;
 
+    /**
+     * setsockopt(2) wrapper. Equivalent to
+     * `setsockopt(this->Get(), level, opt_name, opt_val, opt_len)`. Code that uses this
+     * wrapper can be unit tested if this method is overridden by a mock Sock implementation.
+     */
+    [[nodiscard]] virtual int SetSockOpt(int level,
+                                         int opt_name,
+                                         const void* opt_val,
+                                         socklen_t opt_len) const;
+
     using Event = uint8_t;
 
     /**