Merge bitcoin/bitcoin#23604: Use Sock in CNode

ef5014d256638735b292672c774446db4003f03b style: wrap long lines in CNode creation and add some comments (Vasil Dimov)
b68349164827f14c472201cad54c4e19a3321261 scripted-diff: rename CNode::cs_hSocket to CNode::m_sock_mutex (Vasil Dimov)
c41a1162ac4da437c5d755e8fe2bf636bed22b0f net: use Sock in CNode (Vasil Dimov)
c5dd72e146dd8fa77d29c8689a42322a4d1ec780 fuzz: move FuzzedSock earlier in src/test/fuzz/util.h (Vasil Dimov)

Pull request description:

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

  Change `CNode` to use a pointer to `Sock` instead of a bare `SOCKET`.
  This will help mocking / testing / fuzzing more code.

ACKs for top commit:
  jonatack:
    re-ACK ef5014d256638735b292672c774446db4003f03b changes since last review are the removal of an unneeded dtor and the addition of a style commit
  w0xlt:
    reACK ef5014d
  PastaPastaPasta:
    utACK ef5014d256638735b292672c774446db4003f03b, I have reviewed the code, and believe it makes sense to merge
  theStack:
    Cod-review ACK ef5014d256638735b292672c774446db4003f03b

Tree-SHA512: 7f5414dd339cd2f16f7cbdc5fcec238d68b6d50072934aea10b901f409da28ff1ece6db6e899196616aa8127b8b25ab5b86d000bdcee58b4cadd7a3c1cf560c5

3 files changed, 171 insertions, 81 deletions

diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
index 42ca41f4ee..a17cc87730 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -59,7 +59,16 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
 
     // Mock an outbound peer
     CAddress addr1(ip(0xa0b0c001), NODE_NONE);
-    CNode dummyNode1(id++, ServiceFlags(NODE_NETWORK | NODE_WITNESS), INVALID_SOCKET, addr1, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, CAddress(), /*addrNameIn=*/"", ConnectionType::OUTBOUND_FULL_RELAY, /*inbound_onion=*/false);
+    CNode dummyNode1{id++,
+                     ServiceFlags(NODE_NETWORK | NODE_WITNESS),
+                     /*sock=*/nullptr,
+                     addr1,
+                     /*nKeyedNetGroupIn=*/0,
+                     /*nLocalHostNonceIn=*/0,
+                     CAddress(),
+                     /*addrNameIn=*/"",
+                     ConnectionType::OUTBOUND_FULL_RELAY,
+                     /*inbound_onion=*/false};
     dummyNode1.SetCommonVersion(PROTOCOL_VERSION);
 
     peerLogic->InitializeNode(&dummyNode1);
@@ -108,7 +117,16 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
 static void AddRandomOutboundPeer(std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connType)
 {
     CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE);
-    vNodes.emplace_back(new CNode(id++, ServiceFlags(NODE_NETWORK | NODE_WITNESS), INVALID_SOCKET, addr, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, CAddress(), /*addrNameIn=*/"", connType, /*inbound_onion=*/false));
+    vNodes.emplace_back(new CNode{id++,
+                                  ServiceFlags(NODE_NETWORK | NODE_WITNESS),
+                                  /*sock=*/nullptr,
+                                  addr,
+                                  /*nKeyedNetGroupIn=*/0,
+                                  /*nLocalHostNonceIn=*/0,
+                                  CAddress(),
+                                  /*addrNameIn=*/"",
+                                  connType,
+                                  /*inbound_onion=*/false});
     CNode &node = *vNodes.back();
     node.SetCommonVersion(PROTOCOL_VERSION);
 
@@ -279,9 +297,16 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
     std::array<CNode*, 3> nodes;
 
     banman->ClearBanned();
-    nodes[0] = new CNode{id++, NODE_NETWORK, INVALID_SOCKET, addr[0], /*nKeyedNetGroupIn=*/0,
-                         /*nLocalHostNonceIn=*/0, CAddress(), /*addrNameIn=*/"",
-                         ConnectionType::INBOUND, /*inbound_onion=*/false};
+    nodes[0] = new CNode{id++,
+                         NODE_NETWORK,
+                         /*sock=*/nullptr,
+                         addr[0],
+                         /*nKeyedNetGroupIn=*/0,
+                         /*nLocalHostNonceIn=*/0,
+                         CAddress(),
+                         /*addrNameIn=*/"",
+                         ConnectionType::INBOUND,
+                         /*inbound_onion=*/false};
     nodes[0]->SetCommonVersion(PROTOCOL_VERSION);
     peerLogic->InitializeNode(nodes[0]);
     nodes[0]->fSuccessfullyConnected = true;
@@ -295,9 +320,16 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
     BOOST_CHECK(nodes[0]->fDisconnect);
     BOOST_CHECK(!banman->IsDiscouraged(other_addr)); // Different address, not discouraged
 
-    nodes[1] = new CNode{id++, NODE_NETWORK, INVALID_SOCKET, addr[1], /*nKeyedNetGroupIn=*/1,
-                         /*nLocalHostNonceIn=*/1, CAddress(), /*addrNameIn=*/"",
-                         ConnectionType::INBOUND, /*inbound_onion=*/false};
+    nodes[1] = new CNode{id++,
+                         NODE_NETWORK,
+                         /*sock=*/nullptr,
+                         addr[1],
+                         /*nKeyedNetGroupIn=*/1,
+                         /*nLocalHostNonceIn=*/1,
+                         CAddress(),
+                         /*addrNameIn=*/"",
+                         ConnectionType::INBOUND,
+                         /*inbound_onion=*/false};
     nodes[1]->SetCommonVersion(PROTOCOL_VERSION);
     peerLogic->InitializeNode(nodes[1]);
     nodes[1]->fSuccessfullyConnected = true;
@@ -326,9 +358,16 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
 
     // Make sure non-IP peers are discouraged and disconnected properly.
 
-    nodes[2] = new CNode{id++, NODE_NETWORK, INVALID_SOCKET, addr[2], /*nKeyedNetGroupIn=*/1,
-                         /*nLocalHostNonceIn=*/1, CAddress(), /*addrNameIn=*/"",
-                         ConnectionType::OUTBOUND_FULL_RELAY, /*inbound_onion=*/false};
+    nodes[2] = new CNode{id++,
+                         NODE_NETWORK,
+                         /*sock=*/nullptr,
+                         addr[2],
+                         /*nKeyedNetGroupIn=*/1,
+                         /*nLocalHostNonceIn=*/1,
+                         CAddress(),
+                         /*addrNameIn=*/"",
+                         ConnectionType::OUTBOUND_FULL_RELAY,
+                         /*inbound_onion=*/false};
     nodes[2]->SetCommonVersion(PROTOCOL_VERSION);
     peerLogic->InitializeNode(nodes[2]);
     nodes[2]->fSuccessfullyConnected = true;
@@ -364,7 +403,16 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
     SetMockTime(nStartTime); // Overrides future calls to GetTime()
 
     CAddress addr(ip(0xa0b0c001), NODE_NONE);
-    CNode dummyNode(id++, NODE_NETWORK, INVALID_SOCKET, addr, /*nKeyedNetGroupIn=*/4, /*nLocalHostNonceIn=*/4, CAddress(), /*addrNameIn=*/"", ConnectionType::INBOUND, /*inbound_onion=*/false);
+    CNode dummyNode{id++,
+                    NODE_NETWORK,
+                    /*sock=*/nullptr,
+                    addr,
+                    /*nKeyedNetGroupIn=*/4,
+                    /*nLocalHostNonceIn=*/4,
+                    CAddress(),
+                    /*addrNameIn=*/"",
+                    ConnectionType::INBOUND,
+                    /*inbound_onion=*/false};
     dummyNode.SetCommonVersion(PROTOCOL_VERSION);
     peerLogic->InitializeNode(&dummyNode);
     dummyNode.fSuccessfullyConnected = true;
diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h
index 3bc62878bd..6c91844633 100644
--- a/src/test/fuzz/util.h
+++ b/src/test/fuzz/util.h
@@ -38,6 +38,46 @@
 
 class PeerManager;
 
+class FuzzedSock : public Sock
+{
+    FuzzedDataProvider& m_fuzzed_data_provider;
+
+    /**
+     * Data to return when `MSG_PEEK` is used as a `Recv()` flag.
+     * If `MSG_PEEK` is used, then our `Recv()` returns some random data as usual, but on the next
+     * `Recv()` call we must return the same data, thus we remember it here.
+     */
+    mutable std::optional<uint8_t> m_peek_data;
+
+public:
+    explicit FuzzedSock(FuzzedDataProvider& fuzzed_data_provider);
+
+    ~FuzzedSock() override;
+
+    FuzzedSock& operator=(Sock&& other) override;
+
+    void Reset() override;
+
+    ssize_t Send(const void* data, size_t len, int flags) const override;
+
+    ssize_t Recv(void* buf, size_t len, int flags) const override;
+
+    int Connect(const sockaddr*, socklen_t) const override;
+
+    std::unique_ptr<Sock> Accept(sockaddr* addr, socklen_t* addr_len) const override;
+
+    int GetSockOpt(int level, int opt_name, 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;
+};
+
+[[nodiscard]] inline FuzzedSock ConsumeSock(FuzzedDataProvider& fuzzed_data_provider)
+{
+    return FuzzedSock{fuzzed_data_provider};
+}
+
 template <typename... Callables>
 size_t CallOneOf(FuzzedDataProvider& fuzzed_data_provider, Callables... callables)
 {
@@ -250,7 +290,7 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
 {
     const NodeId node_id = node_id_in.value_or(fuzzed_data_provider.ConsumeIntegralInRange<NodeId>(0, std::numeric_limits<NodeId>::max()));
     const ServiceFlags local_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
-    const SOCKET socket = INVALID_SOCKET;
+    const auto sock = std::make_shared<FuzzedSock>(fuzzed_data_provider);
     const CAddress address = ConsumeAddress(fuzzed_data_provider);
     const uint64_t keyed_net_group = fuzzed_data_provider.ConsumeIntegral<uint64_t>();
     const uint64_t local_host_nonce = fuzzed_data_provider.ConsumeIntegral<uint64_t>();
@@ -259,9 +299,27 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
     const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray(ALL_CONNECTION_TYPES);
     const bool inbound_onion{conn_type == ConnectionType::INBOUND ? fuzzed_data_provider.ConsumeBool() : false};
     if constexpr (ReturnUniquePtr) {
-        return std::make_unique<CNode>(node_id, local_services, socket, address, keyed_net_group, local_host_nonce, addr_bind, addr_name, conn_type, inbound_onion);
+        return std::make_unique<CNode>(node_id,
+                                       local_services,
+                                       sock,
+                                       address,
+                                       keyed_net_group,
+                                       local_host_nonce,
+                                       addr_bind,
+                                       addr_name,
+                                       conn_type,
+                                       inbound_onion);
     } else {
-        return CNode{node_id, local_services, socket, address, keyed_net_group, local_host_nonce, addr_bind, addr_name, conn_type, inbound_onion};
+        return CNode{node_id,
+                     local_services,
+                     sock,
+                     address,
+                     keyed_net_group,
+                     local_host_nonce,
+                     addr_bind,
+                     addr_name,
+                     conn_type,
+                     inbound_onion};
     }
 }
 inline std::unique_ptr<CNode> ConsumeNodeAsUniquePtr(FuzzedDataProvider& fdp, const std::optional<NodeId>& node_id_in = std::nullopt) { return ConsumeNode<true>(fdp, node_id_in); }
@@ -373,44 +431,4 @@ void ReadFromStream(FuzzedDataProvider& fuzzed_data_provider, Stream& stream) no
     }
 }
 
-class FuzzedSock : public Sock
-{
-    FuzzedDataProvider& m_fuzzed_data_provider;
-
-    /**
-     * Data to return when `MSG_PEEK` is used as a `Recv()` flag.
-     * If `MSG_PEEK` is used, then our `Recv()` returns some random data as usual, but on the next
-     * `Recv()` call we must return the same data, thus we remember it here.
-     */
-    mutable std::optional<uint8_t> m_peek_data;
-
-public:
-    explicit FuzzedSock(FuzzedDataProvider& fuzzed_data_provider);
-
-    ~FuzzedSock() override;
-
-    FuzzedSock& operator=(Sock&& other) override;
-
-    void Reset() override;
-
-    ssize_t Send(const void* data, size_t len, int flags) const override;
-
-    ssize_t Recv(void* buf, size_t len, int flags) const override;
-
-    int Connect(const sockaddr*, socklen_t) const override;
-
-    std::unique_ptr<Sock> Accept(sockaddr* addr, socklen_t* addr_len) const override;
-
-    int GetSockOpt(int level, int opt_name, 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;
-};
-
-[[nodiscard]] inline FuzzedSock ConsumeSock(FuzzedDataProvider& fuzzed_data_provider)
-{
-    return FuzzedSock{fuzzed_data_provider};
-}
-
 #endif // BITCOIN_TEST_FUZZ_UTIL_H
diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index b0befe2f58..908b030eea 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -43,7 +43,6 @@ BOOST_AUTO_TEST_CASE(cnode_listen_port)
 
 BOOST_AUTO_TEST_CASE(cnode_simple_test)
 {
-    SOCKET hSocket = INVALID_SOCKET;
     NodeId id = 0;
 
     in_addr ipv4Addr;
@@ -52,12 +51,16 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test)
     CAddress addr = CAddress(CService(ipv4Addr, 7777), NODE_NETWORK);
     std::string pszDest;
 
-    std::unique_ptr<CNode> pnode1 = std::make_unique<CNode>(
-        id++, NODE_NETWORK, hSocket, addr,
-        /* nKeyedNetGroupIn = */ 0,
-        /* nLocalHostNonceIn = */ 0,
-        CAddress(), pszDest, ConnectionType::OUTBOUND_FULL_RELAY,
-        /* inbound_onion = */ false);
+    std::unique_ptr<CNode> pnode1 = std::make_unique<CNode>(id++,
+                                                            NODE_NETWORK,
+                                                            /*sock=*/nullptr,
+                                                            addr,
+                                                            /*nKeyedNetGroupIn=*/0,
+                                                            /*nLocalHostNonceIn=*/0,
+                                                            CAddress(),
+                                                            pszDest,
+                                                            ConnectionType::OUTBOUND_FULL_RELAY,
+                                                            /*inbound_onion=*/false);
     BOOST_CHECK(pnode1->IsFullOutboundConn() == true);
     BOOST_CHECK(pnode1->IsManualConn() == false);
     BOOST_CHECK(pnode1->IsBlockOnlyConn() == false);
@@ -67,12 +70,16 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test)
     BOOST_CHECK(pnode1->m_inbound_onion == false);
     BOOST_CHECK_EQUAL(pnode1->ConnectedThroughNetwork(), Network::NET_IPV4);
 
-    std::unique_ptr<CNode> pnode2 = std::make_unique<CNode>(
-        id++, NODE_NETWORK, hSocket, addr,
-        /* nKeyedNetGroupIn = */ 1,
-        /* nLocalHostNonceIn = */ 1,
-        CAddress(), pszDest, ConnectionType::INBOUND,
-        /* inbound_onion = */ false);
+    std::unique_ptr<CNode> pnode2 = std::make_unique<CNode>(id++,
+                                                            NODE_NETWORK,
+                                                            /*sock=*/nullptr,
+                                                            addr,
+                                                            /*nKeyedNetGroupIn=*/1,
+                                                            /*nLocalHostNonceIn=*/1,
+                                                            CAddress(),
+                                                            pszDest,
+                                                            ConnectionType::INBOUND,
+                                                            /*inbound_onion=*/false);
     BOOST_CHECK(pnode2->IsFullOutboundConn() == false);
     BOOST_CHECK(pnode2->IsManualConn() == false);
     BOOST_CHECK(pnode2->IsBlockOnlyConn() == false);
@@ -82,12 +89,16 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test)
     BOOST_CHECK(pnode2->m_inbound_onion == false);
     BOOST_CHECK_EQUAL(pnode2->ConnectedThroughNetwork(), Network::NET_IPV4);
 
-    std::unique_ptr<CNode> pnode3 = std::make_unique<CNode>(
-        id++, NODE_NETWORK, hSocket, addr,
-        /* nKeyedNetGroupIn = */ 0,
-        /* nLocalHostNonceIn = */ 0,
-        CAddress(), pszDest, ConnectionType::OUTBOUND_FULL_RELAY,
-        /* inbound_onion = */ false);
+    std::unique_ptr<CNode> pnode3 = std::make_unique<CNode>(id++,
+                                                            NODE_NETWORK,
+                                                            /*sock=*/nullptr,
+                                                            addr,
+                                                            /*nKeyedNetGroupIn=*/0,
+                                                            /*nLocalHostNonceIn=*/0,
+                                                            CAddress(),
+                                                            pszDest,
+                                                            ConnectionType::OUTBOUND_FULL_RELAY,
+                                                            /*inbound_onion=*/false);
     BOOST_CHECK(pnode3->IsFullOutboundConn() == true);
     BOOST_CHECK(pnode3->IsManualConn() == false);
     BOOST_CHECK(pnode3->IsBlockOnlyConn() == false);
@@ -97,12 +108,16 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test)
     BOOST_CHECK(pnode3->m_inbound_onion == false);
     BOOST_CHECK_EQUAL(pnode3->ConnectedThroughNetwork(), Network::NET_IPV4);
 
-    std::unique_ptr<CNode> pnode4 = std::make_unique<CNode>(
-        id++, NODE_NETWORK, hSocket, addr,
-        /* nKeyedNetGroupIn = */ 1,
-        /* nLocalHostNonceIn = */ 1,
-        CAddress(), pszDest, ConnectionType::INBOUND,
-        /* inbound_onion = */ true);
+    std::unique_ptr<CNode> pnode4 = std::make_unique<CNode>(id++,
+                                                            NODE_NETWORK,
+                                                            /*sock=*/nullptr,
+                                                            addr,
+                                                            /*nKeyedNetGroupIn=*/1,
+                                                            /*nLocalHostNonceIn=*/1,
+                                                            CAddress(),
+                                                            pszDest,
+                                                            ConnectionType::INBOUND,
+                                                            /*inbound_onion=*/true);
     BOOST_CHECK(pnode4->IsFullOutboundConn() == false);
     BOOST_CHECK(pnode4->IsManualConn() == false);
     BOOST_CHECK(pnode4->IsBlockOnlyConn() == false);
@@ -607,7 +622,16 @@ BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test)
     in_addr ipv4AddrPeer;
     ipv4AddrPeer.s_addr = 0xa0b0c001;
     CAddress addr = CAddress(CService(ipv4AddrPeer, 7777), NODE_NETWORK);
-    std::unique_ptr<CNode> pnode = std::make_unique<CNode>(0, NODE_NETWORK, INVALID_SOCKET, addr, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, CAddress{}, /*pszDest=*/std::string{}, ConnectionType::OUTBOUND_FULL_RELAY, /*inbound_onion=*/false);
+    std::unique_ptr<CNode> pnode = std::make_unique<CNode>(/*id=*/0,
+                                                           NODE_NETWORK,
+                                                           /*sock=*/nullptr,
+                                                           addr,
+                                                           /*nKeyedNetGroupIn=*/0,
+                                                           /*nLocalHostNonceIn=*/0,
+                                                           CAddress{},
+                                                           /*pszDest=*/std::string{},
+                                                           ConnectionType::OUTBOUND_FULL_RELAY,
+                                                           /*inbound_onion=*/false);
     pnode->fSuccessfullyConnected.store(true);
 
     // the peer claims to be reaching us via IPv6