Merge #6534: backport: Merge bitcoin#25427, 25428

ed928e3 Merge bitcoin#25428: Remove Sock::Release() and CloseSocket() (laanwj)
36fdffa Merge bitcoin#25427: wallet: remove extra wtx lookup in AddToSpends (Andrew Chow)

Pull request description:

  btc backport

ACKs for top commit:
  UdjinM6:
    utACK ed928e3
  PastaPastaPasta:
    utACK ed928e3

Tree-SHA512: 86c241f364dda6b9df56bbdbd917dd7de7b32139f3ebf14262f39d15151783a7fe878a1eafae2a380d75c59edf52620b7c0eeb10653be956b0a14a547ae5f95e

src/i2p.cpp

@@ -444,7 +444,7 @@ void Session::Disconnect()
             Log("Destroying SAM session %s", m_session_id);
         }
     }
-    m_control_sock->Reset();
+    m_control_sock = std::make_unique<Sock>(INVALID_SOCKET);
     m_session_id.clear();
 }
 } // namespace sam

src/masternode/node.cpp

@@ -163,7 +163,7 @@ void CActiveMasternodeManager::InitInternal(const CBlockIndex* pindex)
         return;
     }
     bool fConnected = ConnectSocketDirectly(m_info.service, *sock, nConnectTimeout, true) && sock->IsSelectable();
-    sock->Reset();
+    sock = std::make_unique<Sock>(INVALID_SOCKET);
 
     if (!fConnected && Params().RequireRoutableExternalIP()) {
         m_state = MasternodeState::SOME_ERROR;

src/test/fuzz/util.cpp

@@ -22,10 +22,10 @@ FuzzedSock::FuzzedSock(FuzzedDataProvider& fuzzed_data_provider)
 FuzzedSock::~FuzzedSock()
 {
     // Sock::~Sock() will be called after FuzzedSock::~FuzzedSock() and it will call
-    // Sock::Reset() (not FuzzedSock::Reset()!) which will call CloseSocket(m_socket).
+    // close(m_socket) if m_socket is not INVALID_SOCKET.
     // Avoid closing an arbitrary file descriptor (m_socket is just a random very high number which
     // theoretically may concide with a real opened file descriptor).
-    Reset();
+    m_socket = INVALID_SOCKET;
 }
 
 FuzzedSock& FuzzedSock::operator=(Sock&& other)
@@ -34,11 +34,6 @@ FuzzedSock& FuzzedSock::operator=(Sock&& other)
     return *this;
 }
 
-void FuzzedSock::Reset()
-{
-    m_socket = INVALID_SOCKET;
-}
-
 ssize_t FuzzedSock::Send(const void* data, size_t len, int flags) const
 {
     constexpr std::array send_errnos{

src/test/fuzz/util.h

@@ -67,8 +67,6 @@ class FuzzedSock : public Sock
 
     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;

src/test/sock_tests.cpp

@@ -69,24 +69,6 @@ BOOST_AUTO_TEST_CASE(move_assignment)
     BOOST_CHECK(SocketIsClosed(s));
 }
 
-BOOST_AUTO_TEST_CASE(release)
-{
-    SOCKET s = CreateSocket();
-    Sock* sock = new Sock(s);
-    BOOST_CHECK_EQUAL(sock->Release(), s);
-    delete sock;
-    BOOST_CHECK(!SocketIsClosed(s));
-    BOOST_REQUIRE(CloseSocket(s));
-}
-
-BOOST_AUTO_TEST_CASE(reset)
-{
-    const SOCKET s = CreateSocket();
-    Sock sock(s);
-    sock.Reset();
-    BOOST_CHECK(SocketIsClosed(s));
-}
-
 #ifndef WIN32 // Windows does not have socketpair(2).
 
 static void CreateSocketPair(int s[2])

src/test/util/net.h

@@ -139,19 +139,14 @@ class StaticContentsSock : public Sock
         m_socket = INVALID_SOCKET - 1;
     }
 
-    ~StaticContentsSock() override { Reset(); }
+    ~StaticContentsSock() override { m_socket = INVALID_SOCKET; }
 
     StaticContentsSock& operator=(Sock&& other) override
     {
         assert(false && "Move of Sock into MockSock not allowed.");
         return *this;
     }
 
-    void Reset() override
-    {
-        m_socket = INVALID_SOCKET;
-    }
-
     ssize_t Send(const void*, size_t len, int) const override { return len; }
 
     ssize_t Recv(void* buf, size_t len, int flags) const override

src/util/sock.cpp

@@ -39,27 +39,18 @@ Sock::Sock(Sock&& other)
     other.m_socket = INVALID_SOCKET;
 }
 
-Sock::~Sock() { Reset(); }
+Sock::~Sock() { Close(); }
 
 Sock& Sock::operator=(Sock&& other)
 {
-    Reset();
+    Close();
     m_socket = other.m_socket;
     other.m_socket = INVALID_SOCKET;
     return *this;
 }
 
 SOCKET Sock::Get() const { return m_socket; }
 
-SOCKET Sock::Release()
-{
-    const SOCKET s = m_socket;
-    m_socket = INVALID_SOCKET;
-    return s;
-}
-
-void Sock::Reset() { CloseSocket(m_socket); }
-
 ssize_t Sock::Send(const void* data, size_t len, int flags) const
 {
     return send(m_socket, static_cast<const char*>(data), len, flags);
@@ -369,6 +360,22 @@ bool Sock::IsConnected(std::string& errmsg) const
     }
 }
 
+void Sock::Close()
+{
+    if (m_socket == INVALID_SOCKET) {
+        return;
+    }
+#ifdef WIN32
+    int ret = closesocket(m_socket);
+#else
+    int ret = close(m_socket);
+#endif
+    if (ret) {
+        LogPrintf("Error closing socket %d: %s\n", m_socket, NetworkErrorString(WSAGetLastError()));
+    }
+    m_socket = INVALID_SOCKET;
+}
+
 #ifdef WIN32
 std::string NetworkErrorString(int err)
 {
@@ -392,19 +399,3 @@ std::string NetworkErrorString(int err)
     return SysErrorString(err);
 }
 #endif
-
-bool CloseSocket(SOCKET& hSocket)
-{
-    if (hSocket == INVALID_SOCKET)
-        return false;
-#ifdef WIN32
-    int ret = closesocket(hSocket);
-#else
-    int ret = close(hSocket);
-#endif
-    if (ret) {
-        LogPrintf("Socket close failed: %d. Error: %s\n", hSocket, NetworkErrorString(WSAGetLastError()));
-    }
-    hSocket = INVALID_SOCKET;
-    return ret != SOCKET_ERROR;
-}

src/util/sock.h

@@ -113,18 +113,6 @@ class Sock
      */
     [[nodiscard]] virtual SOCKET Get() const;
 
-    /**
-     * Get the value of the contained socket and drop ownership. It will not be closed by the
-     * destructor after this call.
-     * @return socket or INVALID_SOCKET if empty
-     */
-    virtual SOCKET Release();
-
-    /**
-     * Close if non-empty.
-     */
-    virtual void Reset();
-
     /**
      * send(2) wrapper. Equivalent to `send(this->Get(), data, len, flags);`. Code that uses this
      * wrapper can be unit tested if this method is overridden by a mock Sock implementation.
@@ -270,12 +258,15 @@ class Sock
      * Contained socket. `INVALID_SOCKET` designates the object is empty.
      */
     SOCKET m_socket;
+
+private:
+    /**
+     * Close `m_socket` if it is not `INVALID_SOCKET`.
+     */
+    void Close();
 };
 
 /** Return readable error string for a network error code */
 std::string NetworkErrorString(int err);
 
-/** Close socket and set hSocket to INVALID_SOCKET */
-bool CloseSocket(SOCKET& hSocket);
-
 #endif // BITCOIN_UTIL_SOCK_H

src/wallet/wallet.cpp

@@ -636,16 +636,13 @@ void CWallet::AddToSpends(const COutPoint& outpoint, const uint256& wtxid, Walle
 }
 
 
-void CWallet::AddToSpends(const uint256& wtxid, WalletBatch* batch)
+void CWallet::AddToSpends(const CWalletTx& wtx, WalletBatch* batch)
 {
-    auto it = mapWallet.find(wtxid);
-    assert(it != mapWallet.end());
-    const CWalletTx& thisTx = it->second;
-    if (thisTx.IsCoinBase()) // Coinbases don't spend anything!
+    if (wtx.IsCoinBase()) // Coinbases don't spend anything!
         return;
 
-    for (const CTxIn& txin : thisTx.tx->vin)
-        AddToSpends(txin.prevout, wtxid, batch);
+    for (const CTxIn& txin : wtx.tx->vin)
+        AddToSpends(txin.prevout, wtx.GetHash(), batch);
 }
 
 bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
@@ -914,7 +911,7 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmatio
         wtx.nOrderPos = IncOrderPosNext(&batch);
         wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx));
         wtx.nTimeSmart = ComputeTimeSmart(wtx);
-        AddToSpends(hash, &batch);
+        AddToSpends(wtx, &batch);
         candidates = AddWalletUTXOs(wtx.tx, /*ret_dups=*/true);
     }
 
@@ -1017,7 +1014,7 @@ bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx
     if (/* insertion took place */ ins.second) {
         wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx));
     }
-    AddToSpends(hash);
+    AddToSpends(wtx);
     for (const CTxIn& txin : wtx.tx->vin) {
         auto it = mapWallet.find(txin.prevout.hash);
         if (it != mapWallet.end()) {

src/wallet/wallet.h

@@ -333,7 +333,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     typedef std::multimap<COutPoint, uint256> TxSpends;
     TxSpends mapTxSpends GUARDED_BY(cs_wallet);
     void AddToSpends(const COutPoint& outpoint, const uint256& wtxid, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
-    void AddToSpends(const uint256& wtxid, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    void AddToSpends(const CWalletTx& wtx, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     std::set<COutPoint> setWalletUTXO;
     /** Add new UTXOs to the wallet UTXO set