net: add new method Sock::GetSockName() that wraps getsockname() and use it in GetBindAddress() #25426

I just reallize, this is the only use of getsockname. We could also add GetBindAddress to Sock directly? Or is it better to keep it separate from the Address abstraction?

Yeah, the intention of Sock is to be a thin wrapper over syscalls and not depend on or use any of the other Bitcoin Core stuff (mostly). Right now sock.{h,cpp} does not include any of the net* stuff because the net* stuff includes sock.h. I checked and using CAddress in Sock does not create a circular dependency, here is the diff:

<details> <summary>make GetBindAddress() a Sock method</summary>

diff --git i/src/net.cpp w/src/net.cpp
index 7499d4c72f..d026770565 100644
--- i/src/net.cpp
+++ w/src/net.cpp
@@ -418,28 +418,12 @@ bool CConnman::CheckIncomingNonce(uint64_t nonce)
         if (!pnode->fSuccessfullyConnected && !pnode->IsInboundConn() && pnode->GetLocalNonce() == nonce)
             return false;
     }
     return true;
 }
 
-/** Get the bind address for a socket as CAddress */
-static CAddress GetBindAddress(const Sock& sock)
-{
-    CAddress addr_bind;
-    struct sockaddr_storage sockaddr_bind;
-    socklen_t sockaddr_bind_len = sizeof(sockaddr_bind);
-    if (sock.Get() != INVALID_SOCKET) {
-        if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {
-            addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind);
-        } else {
-            LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n");
-        }
-    }
-    return addr_bind;
-}
-
 CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type)
 {
     assert(conn_type != ConnectionType::INBOUND);
 
     if (pszDest == nullptr) {
         if (IsLocal(addrConnect))
@@ -537,13 +521,13 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
     }
 
     // Add node
     NodeId id = GetNewNodeId();
     uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize();
     if (!addr_bind.IsValid()) {
-        addr_bind = GetBindAddress(*sock);
+        addr_bind = sock->GetBindAddress();
     }
     CNode* pnode = new CNode(id,
                              nLocalServices,
                              std::move(sock),
                              addrConnect,
                              CalculateKeyedNetGroup(addrConnect),
@@ -1151,13 +1135,13 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
     if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr)) {
         LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Unknown socket family\n");
     } else {
         addr = CAddress{MaybeFlipIPv6toCJDNS(addr), NODE_NONE};
     }
 
-    const CAddress addr_bind{MaybeFlipIPv6toCJDNS(GetBindAddress(*sock)), NODE_NONE};
+    const CAddress addr_bind{MaybeFlipIPv6toCJDNS(sock->GetBindAddress()), NODE_NONE};
 
     NetPermissionFlags permissionFlags = NetPermissionFlags::None;
     hListenSocket.AddSocketPermissionFlags(permissionFlags);
 
     CreateNodeFromAcceptedSocket(std::move(sock), permissionFlags, addr_bind, addr);
 }
diff --git i/src/util/sock.cpp w/src/util/sock.cpp
index b4c0aa4205..6b77ddd12a 100644
--- i/src/util/sock.cpp
+++ w/src/util/sock.cpp
@@ -1,12 +1,13 @@
 // Copyright (c) 2020-2021 The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <compat.h>
 #include <logging.h>
+#include <protocol.h>
 #include <threadinterrupt.h>
 #include <tinyformat.h>
 #include <util/sock.h>
 #include <util/syserror.h>
 #include <util/system.h>
 #include <util/time.h>
@@ -113,12 +114,27 @@ int Sock::SetSockOpt(int level, int opt_name, const void* opt_val, socklen_t opt
 
 int Sock::GetSockName(sockaddr* name, socklen_t* name_len) const
 {
     return getsockname(m_socket, name, name_len);
 }
 
+CAddress Sock::GetBindAddress() const
+{
+    CAddress addr_bind;
+    struct sockaddr_storage sockaddr_bind;
+    socklen_t sockaddr_bind_len = sizeof(sockaddr_bind);
+    if (m_socket != INVALID_SOCKET) {
+        if (!GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {
+            addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind);
+        } else {
+            LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n");
+        }
+    }
+    return addr_bind;
+}
+
 bool Sock::Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred) const
 {
     // We need a `shared_ptr` owning `this` for `WaitMany()`, but don't want
     // `this` to be destroyed when the `shared_ptr` goes out of scope at the
     // end of this function. Create it with a custom noop deleter.
     std::shared_ptr<const Sock> shared{this, [](const Sock*) {}};
diff --git i/src/util/sock.h w/src/util/sock.h
index 96d0b3b56b..681126cdd2 100644
--- i/src/util/sock.h
+++ w/src/util/sock.h
@@ -3,12 +3,13 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #ifndef BITCOIN_UTIL_SOCK_H
 #define BITCOIN_UTIL_SOCK_H
 
 #include <compat.h>
+#include <protocol.h>
 #include <threadinterrupt.h>
 #include <util/time.h>
 
 #include <chrono>
 #include <memory>
 #include <string>
@@ -130,12 +131,14 @@ public:
      * getsockname(2) wrapper. Equivalent to
      * `getsockname(this->Get(), name, name_len)`. Code that uses this
      * wrapper can be unit tested if this method is overridden by a mock Sock implementation.
      */
     [[nodiscard]] virtual int GetSockName(sockaddr* name, socklen_t* name_len) const;
 
+    [[nodiscard]] virtual CAddress GetBindAddress() const;
+
     using Event = uint8_t;
 
     /**
      * If passed to `Wait()`, then it will wait for readiness to read from the socket.
      */
     static constexpr Event RECV = 0b001;

</details>

What do you think? Should I include the above patch as a separate commit in this PR?

Off-scope for this PR: I think the return value of GetBindAddress() should be CService, not CAddress. It is just an IP address and a port.

Yes, fair enough, i'm fine with keeping it as-is, just wanted to ask in case it was an obvious thing we forgot to consider.

Off-scope for this PR: I think the return value of GetBindAddress() should be CService, not CAddress. It is just an IP address and a port.

I agree! (actually had to look up what is what first the class names are so undescriptive :blush: ).