Merge bitcoin/bitcoin#26312: Remove Sock::Get() and Sock::Sock()

7df450836969b81e98322c9a09c08b35d1095a25 test: improve sock_tests/move_assignment (Vasil Dimov)
5086a99b84367a45706af7197da1016dd966e6d9 net: remove Sock default constructor, it's not necessary (Vasil Dimov)
7829272f7826511241defd34954e6040ea963f07 net: remove now unnecessary Sock::Get() (Vasil Dimov)
944b21b70ae490a5a746bcc1810a5074d74e9d34 net: don't check if the socket is valid in ConnectSocketDirectly() (Vasil Dimov)
aeac68d036e3cff57ce155f1a904d77f98b357d4 net: don't check if the socket is valid in GetBindAddress() (Vasil Dimov)
5ac1a51ee5a57da59f1ff1986b7d9054484d3c80 i2p: avoid using Sock::Get() for checking for a valid socket (Vasil Dimov)

Pull request description:

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

  Peeking at the underlying socket file descriptor of `Sock` and checkig if it is `INVALID_SOCKET` is bad encapsulation and stands in the way of testing/mocking/fuzzing.

  Instead use an empty `unique_ptr` to denote that there is no valid socket where appropriate or outright remove such checks where they are not necessary.

  The default constructor `Sock::Sock()` is unnecessary now after recent changes, thus remove it.

ACKs for top commit:
  ajtowns:
    ACK 7df450836969b81e98322c9a09c08b35d1095a25
  jonatack:
    ACK 7df450836969b81e98322c9a09c08b35d1095a25

Tree-SHA512: 9742aeeeabe8690530bf74caa6ba296787028c52f4a3342afd193b05dbbb1f6645935c33ba0a5230199a09af01c666bd3c7fb16b48692a0d185356ea59a8ddbf

3 files changed, 32 insertions, 14 deletions

diff --git a/src/test/fuzz/util/net.cpp b/src/test/fuzz/util/net.cpp
index 1545e11065..d23e997719 100644
--- a/src/test/fuzz/util/net.cpp
+++ b/src/test/fuzz/util/net.cpp
@@ -77,9 +77,10 @@ template CNetAddr::SerParams ConsumeDeserializationParams(FuzzedDataProvider&) n
 template CAddress::SerParams ConsumeDeserializationParams(FuzzedDataProvider&) noexcept;
 
 FuzzedSock::FuzzedSock(FuzzedDataProvider& fuzzed_data_provider)
-    : m_fuzzed_data_provider{fuzzed_data_provider}, m_selectable{fuzzed_data_provider.ConsumeBool()}
+    : Sock{fuzzed_data_provider.ConsumeIntegralInRange<SOCKET>(INVALID_SOCKET - 1, INVALID_SOCKET)},
+      m_fuzzed_data_provider{fuzzed_data_provider},
+      m_selectable{fuzzed_data_provider.ConsumeBool()}
 {
-    m_socket = fuzzed_data_provider.ConsumeIntegralInRange<SOCKET>(INVALID_SOCKET - 1, INVALID_SOCKET);
 }
 
 FuzzedSock::~FuzzedSock()
diff --git a/src/test/sock_tests.cpp b/src/test/sock_tests.cpp
index 26ee724bf8..5dd73dc101 100644
--- a/src/test/sock_tests.cpp
+++ b/src/test/sock_tests.cpp
@@ -38,7 +38,7 @@ BOOST_AUTO_TEST_CASE(constructor_and_destructor)
 {
     const SOCKET s = CreateSocket();
     Sock* sock = new Sock(s);
-    BOOST_CHECK_EQUAL(sock->Get(), s);
+    BOOST_CHECK(*sock == s);
     BOOST_CHECK(!SocketIsClosed(s));
     delete sock;
     BOOST_CHECK(SocketIsClosed(s));
@@ -51,22 +51,34 @@ BOOST_AUTO_TEST_CASE(move_constructor)
     Sock* sock2 = new Sock(std::move(*sock1));
     delete sock1;
     BOOST_CHECK(!SocketIsClosed(s));
-    BOOST_CHECK_EQUAL(sock2->Get(), s);
+    BOOST_CHECK(*sock2 == s);
     delete sock2;
     BOOST_CHECK(SocketIsClosed(s));
 }
 
 BOOST_AUTO_TEST_CASE(move_assignment)
 {
-    const SOCKET s = CreateSocket();
-    Sock* sock1 = new Sock(s);
-    Sock* sock2 = new Sock();
+    const SOCKET s1 = CreateSocket();
+    const SOCKET s2 = CreateSocket();
+    Sock* sock1 = new Sock(s1);
+    Sock* sock2 = new Sock(s2);
+
+    BOOST_CHECK(!SocketIsClosed(s1));
+    BOOST_CHECK(!SocketIsClosed(s2));
+
     *sock2 = std::move(*sock1);
+    BOOST_CHECK(!SocketIsClosed(s1));
+    BOOST_CHECK(SocketIsClosed(s2));
+    BOOST_CHECK(*sock2 == s1);
+
     delete sock1;
-    BOOST_CHECK(!SocketIsClosed(s));
-    BOOST_CHECK_EQUAL(sock2->Get(), s);
+    BOOST_CHECK(!SocketIsClosed(s1));
+    BOOST_CHECK(SocketIsClosed(s2));
+    BOOST_CHECK(*sock2 == s1);
+
     delete sock2;
-    BOOST_CHECK(SocketIsClosed(s));
+    BOOST_CHECK(SocketIsClosed(s1));
+    BOOST_CHECK(SocketIsClosed(s2));
 }
 
 #ifndef WIN32 // Windows does not have socketpair(2).
@@ -98,7 +110,7 @@ BOOST_AUTO_TEST_CASE(send_and_receive)
     SendAndRecvMessage(*sock0, *sock1);
 
     Sock* sock0moved = new Sock(std::move(*sock0));
-    Sock* sock1moved = new Sock();
+    Sock* sock1moved = new Sock(INVALID_SOCKET);
     *sock1moved = std::move(*sock1);
 
     delete sock0;
diff --git a/src/test/util/net.h b/src/test/util/net.h
index 0d41cf550e..497292542b 100644
--- a/src/test/util/net.h
+++ b/src/test/util/net.h
@@ -108,10 +108,10 @@ constexpr auto ALL_NETWORKS = std::array{
 class StaticContentsSock : public Sock
 {
 public:
-    explicit StaticContentsSock(const std::string& contents) : m_contents{contents}
+    explicit StaticContentsSock(const std::string& contents)
+        : Sock{INVALID_SOCKET},
+          m_contents{contents}
     {
-        // Just a dummy number that is not INVALID_SOCKET.
-        m_socket = INVALID_SOCKET - 1;
     }
 
     ~StaticContentsSock() override { m_socket = INVALID_SOCKET; }
@@ -194,6 +194,11 @@ public:
         return true;
     }
 
+    bool IsConnected(std::string&) const override
+    {
+        return true;
+    }
+
 private:
     const std::string m_contents;
     mutable size_t m_consumed{0};