From 68d5b895683190669be5f3090641e547b03de7a9 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 23 Aug 2024 13:11:37 +0200 Subject: [PATCH 01/16] net: reduce CAddress usage to CService or CNetAddr * `CConnman::CalculateKeyedNetGroup()` needs `CNetAddr`, not `CAddress`, thus change its argument. * Both callers of `CConnman::CreateNodeFromAcceptedSocket()` create a dummy `CAddress` from `CService`, so use `CService` instead. --- src/net.cpp | 19 +++++++++---------- src/net.h | 6 +++--- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index d87adcc03136..6267c52ae30b 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1702,7 +1702,6 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { struct sockaddr_storage sockaddr; socklen_t len = sizeof(sockaddr); auto sock = hListenSocket.sock->Accept((struct sockaddr*)&sockaddr, &len); - CAddress addr; if (!sock) { const int nErr = WSAGetLastError(); @@ -1712,13 +1711,14 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { return; } + CService addr; if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr)) { LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Unknown socket family\n"); } else { - addr = CAddress{MaybeFlipIPv6toCJDNS(addr), NODE_NONE}; + addr = MaybeFlipIPv6toCJDNS(addr); } - const CAddress addr_bind{MaybeFlipIPv6toCJDNS(GetBindAddress(*sock)), NODE_NONE}; + const CService addr_bind{MaybeFlipIPv6toCJDNS(GetBindAddress(*sock))}; NetPermissionFlags permission_flags = NetPermissionFlags::None; hListenSocket.AddSocketPermissionFlags(permission_flags); @@ -1728,8 +1728,8 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, NetPermissionFlags permission_flags, - const CAddress& addr_bind, - const CAddress& addr) + const CService& addr_bind, + const CService& addr) { int nInbound = 0; @@ -1796,10 +1796,10 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, CNode* pnode = new CNode(id, std::move(sock), - addr, + CAddress{addr, NODE_NONE}, CalculateKeyedNetGroup(addr), nonce, - addr_bind, + CAddress{addr_bind, NODE_NONE}, /*addrNameIn=*/"", ConnectionType::INBOUND, inbound_onion, @@ -3037,8 +3037,7 @@ void CConnman::ThreadI2PAcceptIncoming() continue; } - CreateNodeFromAcceptedSocket(std::move(conn.sock), NetPermissionFlags::None, - CAddress{conn.me, NODE_NONE}, CAddress{conn.peer, NODE_NONE}); + CreateNodeFromAcceptedSocket(std::move(conn.sock), NetPermissionFlags::None, conn.me, conn.peer); err_wait = err_wait_begin; } @@ -3900,7 +3899,7 @@ CSipHasher CConnman::GetDeterministicRandomizer(uint64_t id) const return CSipHasher(nSeed0, nSeed1).Write(id); } -uint64_t CConnman::CalculateKeyedNetGroup(const CAddress& address) const +uint64_t CConnman::CalculateKeyedNetGroup(const CNetAddr& address) const { std::vector vchNetGroup(m_netgroupman.GetGroup(address)); diff --git a/src/net.h b/src/net.h index 2f65a01ce12b..7e6969c5faf2 100644 --- a/src/net.h +++ b/src/net.h @@ -1293,8 +1293,8 @@ class CConnman */ void CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, NetPermissionFlags permission_flags, - const CAddress& addr_bind, - const CAddress& addr); + const CService& addr_bind, + const CService& addr); void DisconnectNodes() EXCLUSIVE_LOCKS_REQUIRED(!m_reconnections_mutex, !m_nodes_mutex); void NotifyNumConnectionsChanged(); @@ -1331,7 +1331,7 @@ class CConnman void ThreadSocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc, !m_nodes_mutex, !m_reconnections_mutex); void ThreadDNSAddressSeed() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_nodes_mutex); - uint64_t CalculateKeyedNetGroup(const CAddress& ad) const; + uint64_t CalculateKeyedNetGroup(const CNetAddr& ad) const; CNode* FindNode(const CNetAddr& ip); CNode* FindNode(const std::string& addrName); From 2b2a151ff4929985f22d1985508e381f2c637843 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 23 Aug 2024 15:36:40 +0200 Subject: [PATCH 02/16] net: split CConnman::BindListenPort() off CConnman Introduce a new low-level socket managing class `SockMan` and move the `CConnman::BindListenPort()` method to it. Also, separate the listening socket from the permissions - they were coupled in `struct ListenSocket`, but the socket is protocol agnostic, whereas the permissions are specific to the application of the Bitcoin P2P protocol. --- src/CMakeLists.txt | 1 + src/net.cpp | 95 ++++++++-------------------------------------- src/net.h | 25 ++++-------- src/sockman.cpp | 85 +++++++++++++++++++++++++++++++++++++++++ src/sockman.h | 44 +++++++++++++++++++++ 5 files changed, 154 insertions(+), 96 deletions(-) create mode 100644 src/sockman.cpp create mode 100644 src/sockman.h diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 36d76fbb27d6..cf8c8566559d 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -270,6 +270,7 @@ add_library(bitcoin_node STATIC EXCLUDE_FROM_ALL rpc/txoutproof.cpp script/sigcache.cpp signet.cpp + sockman.cpp torcontrol.cpp txdb.cpp txmempool.cpp diff --git a/src/net.cpp b/src/net.cpp index 6267c52ae30b..233513afd7ef 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1698,10 +1698,10 @@ bool CConnman::AttemptToEvictConnection() return false; } -void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { +void CConnman::AcceptConnection(const Sock& listen_sock) { struct sockaddr_storage sockaddr; socklen_t len = sizeof(sockaddr); - auto sock = hListenSocket.sock->Accept((struct sockaddr*)&sockaddr, &len); + auto sock = listen_sock.Accept((struct sockaddr*)&sockaddr, &len); if (!sock) { const int nErr = WSAGetLastError(); @@ -1721,7 +1721,10 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { const CService addr_bind{MaybeFlipIPv6toCJDNS(GetBindAddress(*sock))}; NetPermissionFlags permission_flags = NetPermissionFlags::None; - hListenSocket.AddSocketPermissionFlags(permission_flags); + auto it{m_listen_permissions.find(addr_bind)}; + if (it != m_listen_permissions.end()) { + NetPermissions::AddFlag(permission_flags, it->second); + } CreateNodeFromAcceptedSocket(std::move(sock), permission_flags, addr_bind, addr); } @@ -1997,8 +2000,8 @@ Sock::EventsPerSock CConnman::GenerateWaitSockets(Span nodes) { Sock::EventsPerSock events_per_sock; - for (const ListenSocket& hListenSocket : vhListenSocket) { - events_per_sock.emplace(hListenSocket.sock, Sock::Events{Sock::RECV}); + for (const auto& sock : m_listen) { + events_per_sock.emplace(sock, Sock::Events{Sock::RECV}); } for (CNode* pnode : nodes) { @@ -2149,13 +2152,13 @@ void CConnman::SocketHandlerConnected(const std::vector& nodes, void CConnman::SocketHandlerListening(const Sock::EventsPerSock& events_per_sock) { - for (const ListenSocket& listen_socket : vhListenSocket) { + for (const auto& sock : m_listen) { if (interruptNet) { return; } - const auto it = events_per_sock.find(listen_socket.sock); + const auto it = events_per_sock.find(sock); if (it != events_per_sock.end() && it->second.occurred & Sock::RECV) { - AcceptConnection(listen_socket); + AcceptConnection(*sock); } } } @@ -3043,75 +3046,6 @@ void CConnman::ThreadI2PAcceptIncoming() } } -bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError, NetPermissionFlags permissions) -{ - int nOne = 1; - - // Create socket for listening for incoming connections - struct sockaddr_storage sockaddr; - socklen_t len = sizeof(sockaddr); - if (!addrBind.GetSockAddr((struct sockaddr*)&sockaddr, &len)) - { - strError = strprintf(Untranslated("Bind address family for %s not supported"), addrBind.ToStringAddrPort()); - LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); - return false; - } - - std::unique_ptr sock = CreateSock(addrBind.GetSAFamily(), SOCK_STREAM, IPPROTO_TCP); - if (!sock) { - strError = strprintf(Untranslated("Couldn't open socket for incoming connections (socket returned error %s)"), NetworkErrorString(WSAGetLastError())); - LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); - return false; - } - - // Allow binding if the port is still in TIME_WAIT state after - // the program was closed and restarted. - 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 - 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; - 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 - } - - if (sock->Bind(reinterpret_cast(&sockaddr), len) == SOCKET_ERROR) { - int nErr = WSAGetLastError(); - if (nErr == WSAEADDRINUSE) - strError = strprintf(_("Unable to bind to %s on this computer. %s is probably already running."), addrBind.ToStringAddrPort(), PACKAGE_NAME); - else - strError = strprintf(_("Unable to bind to %s on this computer (bind returned error %s)"), addrBind.ToStringAddrPort(), NetworkErrorString(nErr)); - LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); - return false; - } - LogPrintf("Bound to %s\n", addrBind.ToStringAddrPort()); - - // Listen for incoming connections - if (sock->Listen(SOMAXCONN) == SOCKET_ERROR) - { - strError = strprintf(_("Listening for incoming connections failed (listen returned error %s)"), NetworkErrorString(WSAGetLastError())); - LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); - return false; - } - - vhListenSocket.emplace_back(std::move(sock), permissions); - return true; -} - void Discover() { if (!fDiscover) @@ -3210,13 +3144,15 @@ bool CConnman::Bind(const CService& addr_, unsigned int flags, NetPermissionFlag const CService addr{MaybeFlipIPv6toCJDNS(addr_)}; bilingual_str strError; - if (!BindListenPort(addr, strError, permissions)) { + if (!BindListenPort(addr, strError)) { if ((flags & BF_REPORT_ERROR) && m_client_interface) { m_client_interface->ThreadSafeMessageBox(strError, "", CClientUIInterface::MSG_ERROR); } return false; } + m_listen_permissions.emplace(addr, permissions); + if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && !NetPermissions::HasFlag(permissions, NetPermissionFlags::NoBan)) { AddLocal(addr, LOCAL_BIND); } @@ -3449,7 +3385,8 @@ void CConnman::StopNodes() DeleteNode(pnode); } m_nodes_disconnected.clear(); - vhListenSocket.clear(); + m_listen_permissions.clear(); + CloseSockets(); semOutbound.reset(); semAddnode.reset(); } diff --git a/src/net.h b/src/net.h index 7e6969c5faf2..6fce5548758c 100644 --- a/src/net.h +++ b/src/net.h @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -1029,7 +1030,7 @@ class NetEventsInterface ~NetEventsInterface() = default; }; -class CConnman +class CConnman : private SockMan { public: @@ -1254,24 +1255,10 @@ class CConnman bool MultipleManualOrFullOutboundConns(Network net) const EXCLUSIVE_LOCKS_REQUIRED(m_nodes_mutex); private: - struct ListenSocket { - public: - std::shared_ptr sock; - inline void AddSocketPermissionFlags(NetPermissionFlags& flags) const { NetPermissions::AddFlag(flags, m_permissions); } - ListenSocket(std::shared_ptr sock_, NetPermissionFlags permissions_) - : sock{sock_}, m_permissions{permissions_} - { - } - - private: - NetPermissionFlags m_permissions; - }; - //! returns the time left in the current max outbound cycle //! in case of no limit, it will always return 0 std::chrono::seconds GetMaxOutboundTimeLeftInCycle_() const EXCLUSIVE_LOCKS_REQUIRED(m_total_bytes_sent_mutex); - bool BindListenPort(const CService& bindAddr, bilingual_str& strError, NetPermissionFlags permissions); bool Bind(const CService& addr, unsigned int flags, NetPermissionFlags permissions); bool InitBinds(const Options& options); @@ -1281,7 +1268,7 @@ class CConnman void ThreadOpenConnections(std::vector connect, Span seed_nodes) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex); void ThreadMessageHandler() EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc); void ThreadI2PAcceptIncoming(); - void AcceptConnection(const ListenSocket& hListenSocket); + void AcceptConnection(const Sock& listen_sock); /** * Create a `CNode` object from a socket that has just been accepted and add the node to @@ -1411,7 +1398,11 @@ class CConnman unsigned int nSendBufferMaxSize{0}; unsigned int nReceiveFloodSize{0}; - std::vector vhListenSocket; + /** + * Permissions that incoming peers get based on our listening address they connected to. + */ + std::unordered_map m_listen_permissions; + std::atomic fNetworkActive{true}; bool fAddressesInitialized{false}; AddrMan& addrman; diff --git a/src/sockman.cpp b/src/sockman.cpp new file mode 100644 index 000000000000..794922e6d834 --- /dev/null +++ b/src/sockman.cpp @@ -0,0 +1,85 @@ +// Copyright (c) 2024-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/license/mit/. + +#include // IWYU pragma: keep + +#include +#include +#include +#include + +bool SockMan::BindListenPort(const CService& addrBind, bilingual_str& strError) +{ + int nOne = 1; + + // Create socket for listening for incoming connections + struct sockaddr_storage sockaddr; + socklen_t len = sizeof(sockaddr); + if (!addrBind.GetSockAddr((struct sockaddr*)&sockaddr, &len)) + { + strError = strprintf(Untranslated("Bind address family for %s not supported"), addrBind.ToStringAddrPort()); + LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); + return false; + } + + std::unique_ptr sock = CreateSock(addrBind.GetSAFamily(), SOCK_STREAM, IPPROTO_TCP); + if (!sock) { + strError = strprintf(Untranslated("Couldn't open socket for incoming connections (socket returned error %s)"), NetworkErrorString(WSAGetLastError())); + LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); + return false; + } + + // Allow binding if the port is still in TIME_WAIT state after + // the program was closed and restarted. + 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 + 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; + 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 + } + + if (sock->Bind(reinterpret_cast(&sockaddr), len) == SOCKET_ERROR) { + int nErr = WSAGetLastError(); + if (nErr == WSAEADDRINUSE) + strError = strprintf(_("Unable to bind to %s on this computer. %s is probably already running."), addrBind.ToStringAddrPort(), PACKAGE_NAME); + else + strError = strprintf(_("Unable to bind to %s on this computer (bind returned error %s)"), addrBind.ToStringAddrPort(), NetworkErrorString(nErr)); + LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); + return false; + } + LogPrintf("Bound to %s\n", addrBind.ToStringAddrPort()); + + // Listen for incoming connections + if (sock->Listen(SOMAXCONN) == SOCKET_ERROR) + { + strError = strprintf(_("Listening for incoming connections failed (listen returned error %s)"), NetworkErrorString(WSAGetLastError())); + LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); + return false; + } + + m_listen.emplace_back(std::move(sock)); + + return true; +} + +void SockMan::CloseSockets() +{ + m_listen.clear(); +} diff --git a/src/sockman.h b/src/sockman.h new file mode 100644 index 000000000000..6eacff451c62 --- /dev/null +++ b/src/sockman.h @@ -0,0 +1,44 @@ +// Copyright (c) 2024-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/license/mit/. + +#ifndef BITCOIN_SOCKMAN_H +#define BITCOIN_SOCKMAN_H + +#include +#include +#include + +#include +#include + +/** + * A socket manager class which handles socket operations. + * To use this class, inherit from it and implement the pure virtual methods. + * Handled operations: + * - binding and listening on sockets + */ +class SockMan +{ +public: + /** + * Bind to a new address:port, start listening and add the listen socket to `m_listen`. + * @param[in] addrBind Where to bind. + * @param[out] strError Error string if an error occurs. + * @retval true Success. + * @retval false Failure, `strError` will be set. + */ + bool BindListenPort(const CService& addrBind, bilingual_str& strError); + + /** + * Close all sockets. + */ + void CloseSockets(); + + /** + * List of listening sockets. + */ + std::vector> m_listen; +}; + +#endif // BITCOIN_SOCKMAN_H From 62eb178a0276e281102eb933006dd94e4cb3b00b Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 17 Sep 2024 15:05:46 +0200 Subject: [PATCH 03/16] style: modernize the style of SockMan::BindListenPort() It was copied verbatim from `CConnman::BindListenPort()` in the previous commit. Modernize its variables and style and log the error messages from the caller. --- src/net.cpp | 5 +++- src/sockman.cpp | 79 +++++++++++++++++++++++++++---------------------- src/sockman.h | 6 ++-- 3 files changed, 51 insertions(+), 39 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 233513afd7ef..fd8482acb2f3 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3144,13 +3144,16 @@ bool CConnman::Bind(const CService& addr_, unsigned int flags, NetPermissionFlag const CService addr{MaybeFlipIPv6toCJDNS(addr_)}; bilingual_str strError; - if (!BindListenPort(addr, strError)) { + if (!BindAndStartListening(addr, strError)) { + LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); if ((flags & BF_REPORT_ERROR) && m_client_interface) { m_client_interface->ThreadSafeMessageBox(strError, "", CClientUIInterface::MSG_ERROR); } return false; } + LogPrintLevel(BCLog::NET, BCLog::Level::Info, "Bound to and listening at %s\n", addr.ToStringAddrPort()); + m_listen_permissions.emplace(addr, permissions); if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && !NetPermissions::HasFlag(permissions, NetPermissionFlags::NoBan)) { diff --git a/src/sockman.cpp b/src/sockman.cpp index 794922e6d834..1d22df85029e 100644 --- a/src/sockman.cpp +++ b/src/sockman.cpp @@ -9,68 +9,77 @@ #include #include -bool SockMan::BindListenPort(const CService& addrBind, bilingual_str& strError) +bool SockMan::BindAndStartListening(const CService& to, bilingual_str& errmsg) { - int nOne = 1; - // Create socket for listening for incoming connections - struct sockaddr_storage sockaddr; - socklen_t len = sizeof(sockaddr); - if (!addrBind.GetSockAddr((struct sockaddr*)&sockaddr, &len)) - { - strError = strprintf(Untranslated("Bind address family for %s not supported"), addrBind.ToStringAddrPort()); - LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); + struct sockaddr_storage storage; + socklen_t len{sizeof(storage)}; + if (!to.GetSockAddr(reinterpret_cast(&storage), &len)) { + errmsg = strprintf(Untranslated("Bind address family for %s not supported"), to.ToStringAddrPort()); return false; } - std::unique_ptr sock = CreateSock(addrBind.GetSAFamily(), SOCK_STREAM, IPPROTO_TCP); + std::unique_ptr sock{CreateSock(to.GetSAFamily(), SOCK_STREAM, IPPROTO_TCP)}; if (!sock) { - strError = strprintf(Untranslated("Couldn't open socket for incoming connections (socket returned error %s)"), NetworkErrorString(WSAGetLastError())); - LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); + errmsg = strprintf(Untranslated("Cannot create %s listen socket: %s"), + to.ToStringAddrPort(), + NetworkErrorString(WSAGetLastError())); return false; } + int one{1}; + // Allow binding if the port is still in TIME_WAIT state after // the program was closed and restarted. - 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); + if (sock->SetSockOpt(SOL_SOCKET, SO_REUSEADDR, reinterpret_cast(&one), sizeof(one)) == SOCKET_ERROR) { + LogPrintLevel(BCLog::NET, + BCLog::Level::Info, + "Cannot set SO_REUSEADDR on %s listen socket: %s, continuing anyway\n", + to.ToStringAddrPort(), + NetworkErrorString(WSAGetLastError())); } // 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()) { + if (to.IsIPv6()) { #ifdef IPV6_V6ONLY - 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); + if (sock->SetSockOpt(IPPROTO_IPV6, IPV6_V6ONLY, reinterpret_cast(&one), sizeof(one)) == SOCKET_ERROR) { + LogPrintLevel(BCLog::NET, + BCLog::Level::Info, + "Cannot set IPV6_V6ONLY on %s listen socket: %s, continuing anyway\n", + to.ToStringAddrPort(), + NetworkErrorString(WSAGetLastError())); } #endif #ifdef WIN32 - int nProtLevel = PROTECTION_LEVEL_UNRESTRICTED; - 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); + int prot_level{PROTECTION_LEVEL_UNRESTRICTED}; + if (sock->SetSockOpt(IPPROTO_IPV6, + IPV6_PROTECTION_LEVEL, + reinterpret_cast(&prot_level), + sizeof(prot_level)) == SOCKET_ERROR) { + LogPrintLevel(BCLog::NET, + BCLog::Level::Info, + "Cannot set IPV6_PROTECTION_LEVEL on %s listen socket: %s, continuing anyway\n", + to.ToStringAddrPort(), + NetworkErrorString(WSAGetLastError())); } #endif } - if (sock->Bind(reinterpret_cast(&sockaddr), len) == SOCKET_ERROR) { - int nErr = WSAGetLastError(); - if (nErr == WSAEADDRINUSE) - strError = strprintf(_("Unable to bind to %s on this computer. %s is probably already running."), addrBind.ToStringAddrPort(), PACKAGE_NAME); - else - strError = strprintf(_("Unable to bind to %s on this computer (bind returned error %s)"), addrBind.ToStringAddrPort(), NetworkErrorString(nErr)); - LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); + if (sock->Bind(reinterpret_cast(&storage), len) == SOCKET_ERROR) { + const int err{WSAGetLastError()}; + errmsg = strprintf(_("Cannot bind to %s: %s%s"), + to.ToStringAddrPort(), + NetworkErrorString(err), + err == WSAEADDRINUSE + ? std::string{" ("} + PACKAGE_NAME + " already running?)" + : ""); return false; } - LogPrintf("Bound to %s\n", addrBind.ToStringAddrPort()); // Listen for incoming connections - if (sock->Listen(SOMAXCONN) == SOCKET_ERROR) - { - strError = strprintf(_("Listening for incoming connections failed (listen returned error %s)"), NetworkErrorString(WSAGetLastError())); - LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); + if (sock->Listen(SOMAXCONN) == SOCKET_ERROR) { + errmsg = strprintf(_("Cannot listen to %s: %s"), to.ToStringAddrPort(), NetworkErrorString(WSAGetLastError())); return false; } diff --git a/src/sockman.h b/src/sockman.h index 6eacff451c62..86dacfb35adb 100644 --- a/src/sockman.h +++ b/src/sockman.h @@ -23,12 +23,12 @@ class SockMan public: /** * Bind to a new address:port, start listening and add the listen socket to `m_listen`. - * @param[in] addrBind Where to bind. - * @param[out] strError Error string if an error occurs. + * @param[in] to Where to bind. + * @param[out] errmsg Error string if an error occurs. * @retval true Success. * @retval false Failure, `strError` will be set. */ - bool BindListenPort(const CService& addrBind, bilingual_str& strError); + bool BindAndStartListening(const CService& to, bilingual_str& errmsg); /** * Close all sockets. From 6107cbf9047d47258fb35c7d4c6ed743b6bbc490 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 26 Aug 2024 13:14:53 +0200 Subject: [PATCH 04/16] net: split CConnman::AcceptConnection() off CConnman Move the `CConnman::AcceptConnection()` method to `SockMan` and split parts of it: * the flip-to-CJDNS part: to just after the `AcceptConnection()` call * the permissions part: at the start of `CreateNodeFromAcceptedSocket()` --- src/net.cpp | 49 ++++++++++++++++--------------------------------- src/net.h | 3 --- src/sockman.cpp | 21 +++++++++++++++++++++ src/sockman.h | 9 +++++++++ 4 files changed, 46 insertions(+), 36 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index fd8482acb2f3..49d86182964b 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1698,27 +1698,11 @@ bool CConnman::AttemptToEvictConnection() return false; } -void CConnman::AcceptConnection(const Sock& listen_sock) { - struct sockaddr_storage sockaddr; - socklen_t len = sizeof(sockaddr); - auto sock = listen_sock.Accept((struct sockaddr*)&sockaddr, &len); - - if (!sock) { - const int nErr = WSAGetLastError(); - if (nErr != WSAEWOULDBLOCK) { - LogPrintf("socket error accept failed: %s\n", NetworkErrorString(nErr)); - } - return; - } - - CService addr; - if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr)) { - LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Unknown socket family\n"); - } else { - addr = MaybeFlipIPv6toCJDNS(addr); - } - - const CService addr_bind{MaybeFlipIPv6toCJDNS(GetBindAddress(*sock))}; +void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, + const CService& addr_bind, + const CService& addr) +{ + int nInbound = 0; NetPermissionFlags permission_flags = NetPermissionFlags::None; auto it{m_listen_permissions.find(addr_bind)}; @@ -1726,16 +1710,6 @@ void CConnman::AcceptConnection(const Sock& listen_sock) { NetPermissions::AddFlag(permission_flags, it->second); } - CreateNodeFromAcceptedSocket(std::move(sock), permission_flags, addr_bind, addr); -} - -void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, - NetPermissionFlags permission_flags, - const CService& addr_bind, - const CService& addr) -{ - int nInbound = 0; - AddWhitelistPermissionFlags(permission_flags, addr, vWhitelistedRangeIncoming); { @@ -2158,7 +2132,16 @@ void CConnman::SocketHandlerListening(const Sock::EventsPerSock& events_per_sock } const auto it = events_per_sock.find(sock); if (it != events_per_sock.end() && it->second.occurred & Sock::RECV) { - AcceptConnection(*sock); + CService addr_accepted; + + auto sock_accepted{AcceptConnection(*sock, addr_accepted)}; + + if (sock_accepted) { + addr_accepted = MaybeFlipIPv6toCJDNS(addr_accepted); + const CService addr_bind{MaybeFlipIPv6toCJDNS(GetBindAddress(*sock))}; + + CreateNodeFromAcceptedSocket(std::move(sock_accepted), addr_bind, addr_accepted); + } } } } @@ -3040,7 +3023,7 @@ void CConnman::ThreadI2PAcceptIncoming() continue; } - CreateNodeFromAcceptedSocket(std::move(conn.sock), NetPermissionFlags::None, conn.me, conn.peer); + CreateNodeFromAcceptedSocket(std::move(conn.sock), conn.me, conn.peer); err_wait = err_wait_begin; } diff --git a/src/net.h b/src/net.h index 6fce5548758c..eb0177703790 100644 --- a/src/net.h +++ b/src/net.h @@ -1268,18 +1268,15 @@ class CConnman : private SockMan void ThreadOpenConnections(std::vector connect, Span seed_nodes) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex); void ThreadMessageHandler() EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc); void ThreadI2PAcceptIncoming(); - void AcceptConnection(const Sock& listen_sock); /** * Create a `CNode` object from a socket that has just been accepted and add the node to * the `m_nodes` member. * @param[in] sock Connected socket to communicate with the peer. - * @param[in] permission_flags The peer's permissions. * @param[in] addr_bind The address and port at our side of the connection. * @param[in] addr The address and port at the peer's side of the connection. */ void CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, - NetPermissionFlags permission_flags, const CService& addr_bind, const CService& addr); diff --git a/src/sockman.cpp b/src/sockman.cpp index 1d22df85029e..a41d8a74497f 100644 --- a/src/sockman.cpp +++ b/src/sockman.cpp @@ -88,6 +88,27 @@ bool SockMan::BindAndStartListening(const CService& to, bilingual_str& errmsg) return true; } +std::unique_ptr SockMan::AcceptConnection(const Sock& listen_sock, CService& addr) +{ + struct sockaddr_storage sockaddr; + socklen_t len = sizeof(sockaddr); + auto sock = listen_sock.Accept((struct sockaddr*)&sockaddr, &len); + + if (!sock) { + const int nErr = WSAGetLastError(); + if (nErr != WSAEWOULDBLOCK) { + LogPrintf("socket error accept failed: %s\n", NetworkErrorString(nErr)); + } + return {}; + } + + if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr)) { + LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Unknown socket family\n"); + } + + return sock; +} + void SockMan::CloseSockets() { m_listen.clear(); diff --git a/src/sockman.h b/src/sockman.h index 86dacfb35adb..1e7f5687bff8 100644 --- a/src/sockman.h +++ b/src/sockman.h @@ -17,6 +17,7 @@ * To use this class, inherit from it and implement the pure virtual methods. * Handled operations: * - binding and listening on sockets + * - accepting incoming connections */ class SockMan { @@ -30,6 +31,14 @@ class SockMan */ bool BindAndStartListening(const CService& to, bilingual_str& errmsg); + /** + * Accept a connection. + * @param[in] listen_sock Socket on which to accept the connection. + * @param[out] addr Address of the peer that was accepted. + * @return Newly created socket for the accepted connection. + */ + std::unique_ptr AcceptConnection(const Sock& listen_sock, CService& addr); + /** * Close all sockets. */ From 3f9f99d33b9392641d50dc4a9140cd2bd20a0286 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 17 Sep 2024 17:29:07 +0200 Subject: [PATCH 05/16] style: modernize the style of SockMan::AcceptConnection() --- src/sockman.cpp | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/sockman.cpp b/src/sockman.cpp index a41d8a74497f..d3f5e6d785be 100644 --- a/src/sockman.cpp +++ b/src/sockman.cpp @@ -90,19 +90,23 @@ bool SockMan::BindAndStartListening(const CService& to, bilingual_str& errmsg) std::unique_ptr SockMan::AcceptConnection(const Sock& listen_sock, CService& addr) { - struct sockaddr_storage sockaddr; - socklen_t len = sizeof(sockaddr); - auto sock = listen_sock.Accept((struct sockaddr*)&sockaddr, &len); + sockaddr_storage storage; + socklen_t len{sizeof(storage)}; + + auto sock{listen_sock.Accept(reinterpret_cast(&storage), &len)}; if (!sock) { - const int nErr = WSAGetLastError(); - if (nErr != WSAEWOULDBLOCK) { - LogPrintf("socket error accept failed: %s\n", NetworkErrorString(nErr)); + const int err{WSAGetLastError()}; + if (err != WSAEWOULDBLOCK) { + LogPrintLevel(BCLog::NET, + BCLog::Level::Error, + "Cannot accept new connection: %s\n", + NetworkErrorString(err)); } return {}; } - if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr)) { + if (!addr.SetSockAddr(reinterpret_cast(&storage))) { LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Unknown socket family\n"); } From 0ab3a930117519cc19e73bc29a523e63beae9a28 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 27 Aug 2024 11:25:24 +0200 Subject: [PATCH 06/16] net: move the generation of ids for new nodes from CConnman to SockMan --- src/net.cpp | 5 ----- src/net.h | 5 ----- src/sockman.cpp | 5 +++++ src/sockman.h | 15 +++++++++++++++ 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 49d86182964b..34455a16e464 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3106,11 +3106,6 @@ CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In, AddrMan& addrman_in, SetNetworkActive(network_active); } -NodeId CConnman::GetNewNodeId() -{ - return nLastNodeId.fetch_add(1, std::memory_order_relaxed); -} - uint16_t CConnman::GetDefaultPort(Network net) const { return net == NET_I2P ? I2P_SAM31_PORT : m_params.GetDefaultPort(); diff --git a/src/net.h b/src/net.h index eb0177703790..bc7848634408 100644 --- a/src/net.h +++ b/src/net.h @@ -95,8 +95,6 @@ static const size_t DEFAULT_MAXSENDBUFFER = 1 * 1000; static constexpr bool DEFAULT_V2_TRANSPORT{true}; -typedef int64_t NodeId; - struct AddedNodeParams { std::string m_added_node; bool m_use_v2transport; @@ -1333,8 +1331,6 @@ class CConnman : private SockMan void DeleteNode(CNode* pnode); - NodeId GetNewNodeId(); - /** (Try to) send data from node's vSendMsg. Returns (bytes_sent, data_left). */ std::pair SocketSendData(CNode& node) const EXCLUSIVE_LOCKS_REQUIRED(node.cs_vSend); @@ -1414,7 +1410,6 @@ class CConnman : private SockMan std::vector m_nodes GUARDED_BY(m_nodes_mutex); std::list m_nodes_disconnected; mutable RecursiveMutex m_nodes_mutex; - std::atomic nLastNodeId{0}; unsigned int nPrevNodeCount{0}; // Stores number of full-tx connections (outbound and manual) per network diff --git a/src/sockman.cpp b/src/sockman.cpp index d3f5e6d785be..38a947820550 100644 --- a/src/sockman.cpp +++ b/src/sockman.cpp @@ -113,6 +113,11 @@ std::unique_ptr SockMan::AcceptConnection(const Sock& listen_sock, CServic return sock; } +NodeId SockMan::GetNewNodeId() +{ + return m_next_node_id.fetch_add(1, std::memory_order_relaxed); +} + void SockMan::CloseSockets() { m_listen.clear(); diff --git a/src/sockman.h b/src/sockman.h index 1e7f5687bff8..a0e76999d8a4 100644 --- a/src/sockman.h +++ b/src/sockman.h @@ -9,9 +9,12 @@ #include #include +#include #include #include +typedef int64_t NodeId; + /** * A socket manager class which handles socket operations. * To use this class, inherit from it and implement the pure virtual methods. @@ -39,6 +42,11 @@ class SockMan */ std::unique_ptr AcceptConnection(const Sock& listen_sock, CService& addr); + /** + * Generate an id for a newly created node. + */ + NodeId GetNewNodeId(); + /** * Close all sockets. */ @@ -48,6 +56,13 @@ class SockMan * List of listening sockets. */ std::vector> m_listen; + +private: + + /** + * The id to assign to the next created node. Used to generate ids of nodes. + */ + std::atomic m_next_node_id{0}; }; #endif // BITCOIN_SOCKMAN_H From b3730ed2b5be0541a4a845d102940e90bc2ee60d Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 27 Aug 2024 16:11:35 +0200 Subject: [PATCH 07/16] net: move CConnman-specific parts away from ThreadI2PAcceptIncoming() CConnman-specific or in other words, Bitcoin P2P specific. Now the `ThreadI2PAcceptIncoming()` method is protocol agnostic and can be moved to `SockMan`. --- src/net.cpp | 27 ++++++++++++++++++--------- src/net.h | 6 ++++++ src/sockman.cpp | 2 ++ src/sockman.h | 27 +++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 9 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 34455a16e464..8a24d86a2840 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2986,13 +2986,28 @@ void CConnman::ThreadMessageHandler() } } +void CConnman::EventI2PListen(const CService& addr, bool success) +{ + if (success) { + if (!m_i2p_advertising_listen_addr) { + AddLocal(addr, LOCAL_MANUAL); + m_i2p_advertising_listen_addr = true; + } + return; + } + // a failure to listen + if (m_i2p_advertising_listen_addr && addr.IsValid()) { + RemoveLocal(addr); + m_i2p_advertising_listen_addr = false; + } +} + void CConnman::ThreadI2PAcceptIncoming() { static constexpr auto err_wait_begin = 1s; static constexpr auto err_wait_cap = 5min; auto err_wait = err_wait_begin; - bool advertising_listen_addr = false; i2p::Connection conn; auto SleepOnFailure = [&]() { @@ -3005,18 +3020,12 @@ void CConnman::ThreadI2PAcceptIncoming() while (!interruptNet) { if (!m_i2p_sam_session->Listen(conn)) { - if (advertising_listen_addr && conn.me.IsValid()) { - RemoveLocal(conn.me); - advertising_listen_addr = false; - } + EventI2PListen(conn.me, /*success=*/false); SleepOnFailure(); continue; } - if (!advertising_listen_addr) { - AddLocal(conn.me, LOCAL_MANUAL); - advertising_listen_addr = true; - } + EventI2PListen(conn.me, /*success=*/true); if (!m_i2p_sam_session->Accept(conn)) { SleepOnFailure(); diff --git a/src/net.h b/src/net.h index bc7848634408..e05152ad3011 100644 --- a/src/net.h +++ b/src/net.h @@ -1265,6 +1265,12 @@ class CConnman : private SockMan void ProcessAddrFetch() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_unused_i2p_sessions_mutex); void ThreadOpenConnections(std::vector connect, Span seed_nodes) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex); void ThreadMessageHandler() EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc); + + /// Whether we are currently advertising our I2P address (via `AddLocal()`). + bool m_i2p_advertising_listen_addr{false}; + + virtual void EventI2PListen(const CService& addr, bool success) override; + void ThreadI2PAcceptIncoming(); /** diff --git a/src/sockman.cpp b/src/sockman.cpp index 38a947820550..12bd5ee8284b 100644 --- a/src/sockman.cpp +++ b/src/sockman.cpp @@ -122,3 +122,5 @@ void SockMan::CloseSockets() { m_listen.clear(); } + +void SockMan::EventI2PListen(const CService&, bool) {} diff --git a/src/sockman.h b/src/sockman.h index a0e76999d8a4..836548ef8987 100644 --- a/src/sockman.h +++ b/src/sockman.h @@ -25,6 +25,13 @@ typedef int64_t NodeId; class SockMan { public: + + virtual ~SockMan() = default; + + // + // Non-virtual functions, to be reused by children classes. + // + /** * Bind to a new address:port, start listening and add the listen socket to `m_listen`. * @param[in] to Where to bind. @@ -52,6 +59,26 @@ class SockMan */ void CloseSockets(); + // + // Pure virtual functions must be implemented by children classes. + // + + // + // Non-pure virtual functions can be overridden by children classes or left + // alone to use the default implementation from SockMan. + // + + /** + * Be notified of a change in the state of listening for incoming I2P connections. + * The default behavior, implemented by `SockMan`, is to ignore this event. + * @param[in] addr Our listening address. + * @param[in] success If true then the listen succeeded and we are now + * listening for incoming I2P connections at `addr`. If false then the + * call failed and now we are not listening (even if this was invoked + * before with `true`). + */ + virtual void EventI2PListen(const CService& addr, bool success); + /** * List of listening sockets. */ From 0f522c98dc89e7b6c847429c2b60ae7c09fc6387 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 27 Aug 2024 16:23:31 +0200 Subject: [PATCH 08/16] net: move I2P-accept-incoming code from CConnman to SockMan --- src/net.cpp | 69 +++++++++++-------------------------------------- src/net.h | 31 +++++----------------- src/sockman.cpp | 55 +++++++++++++++++++++++++++++++++++++++ src/sockman.h | 69 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+), 79 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 8a24d86a2840..be46de5818d3 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1698,9 +1698,9 @@ bool CConnman::AttemptToEvictConnection() return false; } -void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, - const CService& addr_bind, - const CService& addr) +void CConnman::EventNewConnectionAccepted(std::unique_ptr&& sock, + const CService& addr_bind, + const CService& addr) { int nInbound = 0; @@ -2140,7 +2140,7 @@ void CConnman::SocketHandlerListening(const Sock::EventsPerSock& events_per_sock addr_accepted = MaybeFlipIPv6toCJDNS(addr_accepted); const CService addr_bind{MaybeFlipIPv6toCJDNS(GetBindAddress(*sock))}; - CreateNodeFromAcceptedSocket(std::move(sock_accepted), addr_bind, addr_accepted); + EventNewConnectionAccepted(std::move(sock_accepted), addr_bind, addr_accepted); } } } @@ -3002,42 +3002,6 @@ void CConnman::EventI2PListen(const CService& addr, bool success) } } -void CConnman::ThreadI2PAcceptIncoming() -{ - static constexpr auto err_wait_begin = 1s; - static constexpr auto err_wait_cap = 5min; - auto err_wait = err_wait_begin; - - i2p::Connection conn; - - auto SleepOnFailure = [&]() { - interruptNet.sleep_for(err_wait); - if (err_wait < err_wait_cap) { - err_wait += 1s; - } - }; - - while (!interruptNet) { - - if (!m_i2p_sam_session->Listen(conn)) { - EventI2PListen(conn.me, /*success=*/false); - SleepOnFailure(); - continue; - } - - EventI2PListen(conn.me, /*success=*/true); - - if (!m_i2p_sam_session->Accept(conn)) { - SleepOnFailure(); - continue; - } - - CreateNodeFromAcceptedSocket(std::move(conn.sock), conn.me, conn.peer); - - err_wait = err_wait_begin; - } -} - void Discover() { if (!fDiscover) @@ -3198,12 +3162,6 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) return false; } - Proxy i2p_sam; - if (GetProxy(NET_I2P, i2p_sam) && connOptions.m_i2p_accept_incoming) { - m_i2p_sam_session = std::make_unique(gArgs.GetDataDirNet() / "i2p_private_key", - i2p_sam, &interruptNet); - } - // Randomize the order in which we may query seednode to potentially prevent connecting to the same one every restart (and signal that we have restarted) std::vector seed_nodes = connOptions.vSeedNodes; if (!seed_nodes.empty()) { @@ -3249,6 +3207,15 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) // Send and receive from sockets, accept connections threadSocketHandler = std::thread(&util::TraceThread, "net", [this] { ThreadSocketHandler(); }); + SockMan::Options sockman_options; + + Proxy i2p_sam; + if (GetProxy(NET_I2P, i2p_sam) && connOptions.m_i2p_accept_incoming) { + sockman_options.i2p.emplace(gArgs.GetDataDirNet() / "i2p_private_key", i2p_sam); + } + + StartSocketsThreads(sockman_options); + if (!gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED)) LogPrintf("DNS seeding disabled\n"); else @@ -3274,11 +3241,6 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) // Process messages threadMessageHandler = std::thread(&util::TraceThread, "msghand", [this] { ThreadMessageHandler(); }); - if (m_i2p_sam_session) { - threadI2PAcceptIncoming = - std::thread(&util::TraceThread, "i2paccept", [this] { ThreadI2PAcceptIncoming(); }); - } - // Dump network addresses scheduler.scheduleEvery([this] { DumpAddresses(); }, DUMP_PEERS_INTERVAL); @@ -3332,9 +3294,8 @@ void CConnman::Interrupt() void CConnman::StopThreads() { - if (threadI2PAcceptIncoming.joinable()) { - threadI2PAcceptIncoming.join(); - } + JoinSocketsThreads(); + if (threadMessageHandler.joinable()) threadMessageHandler.join(); if (threadOpenConnections.joinable()) diff --git a/src/net.h b/src/net.h index e05152ad3011..076583224378 100644 --- a/src/net.h +++ b/src/net.h @@ -1271,18 +1271,15 @@ class CConnman : private SockMan virtual void EventI2PListen(const CService& addr, bool success) override; - void ThreadI2PAcceptIncoming(); - /** - * Create a `CNode` object from a socket that has just been accepted and add the node to - * the `m_nodes` member. + * Create a `CNode` object and add it to the `m_nodes` member. * @param[in] sock Connected socket to communicate with the peer. - * @param[in] addr_bind The address and port at our side of the connection. - * @param[in] addr The address and port at the peer's side of the connection. + * @param[in] me The address and port at our side of the connection. + * @param[in] them The address and port at the peer's side of the connection. */ - void CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, - const CService& addr_bind, - const CService& addr); + virtual void EventNewConnectionAccepted(std::unique_ptr&& sock, + const CService& me, + const CService& them) override; void DisconnectNodes() EXCLUSIVE_LOCKS_REQUIRED(!m_reconnections_mutex, !m_nodes_mutex); void NotifyNumConnectionsChanged(); @@ -1510,27 +1507,11 @@ class CConnman : private SockMan Mutex mutexMsgProc; std::atomic flagInterruptMsgProc{false}; - /** - * This is signaled when network activity should cease. - * A pointer to it is saved in `m_i2p_sam_session`, so make sure that - * the lifetime of `interruptNet` is not shorter than - * the lifetime of `m_i2p_sam_session`. - */ - CThreadInterrupt interruptNet; - - /** - * I2P SAM session. - * Used to accept incoming and make outgoing I2P connections from a persistent - * address. - */ - std::unique_ptr m_i2p_sam_session; - std::thread threadDNSAddressSeed; std::thread threadSocketHandler; std::thread threadOpenAddedConnections; std::thread threadOpenConnections; std::thread threadMessageHandler; - std::thread threadI2PAcceptIncoming; /** flag for deciding to connect to an extra outbound peer, * in excess of m_max_outbound_full_relay diff --git a/src/sockman.cpp b/src/sockman.cpp index 12bd5ee8284b..e3810d79da39 100644 --- a/src/sockman.cpp +++ b/src/sockman.cpp @@ -8,6 +8,7 @@ #include #include #include +#include bool SockMan::BindAndStartListening(const CService& to, bilingual_str& errmsg) { @@ -88,6 +89,24 @@ bool SockMan::BindAndStartListening(const CService& to, bilingual_str& errmsg) return true; } +void SockMan::StartSocketsThreads(const Options& options) +{ + if (options.i2p.has_value()) { + m_i2p_sam_session = std::make_unique( + options.i2p->private_key_file, options.i2p->sam_proxy, &interruptNet); + + m_thread_i2p_accept = + std::thread(&util::TraceThread, "i2paccept", [this] { ThreadI2PAccept(); }); + } +} + +void SockMan::JoinSocketsThreads() +{ + if (m_thread_i2p_accept.joinable()) { + m_thread_i2p_accept.join(); + } +} + std::unique_ptr SockMan::AcceptConnection(const Sock& listen_sock, CService& addr) { sockaddr_storage storage; @@ -124,3 +143,39 @@ void SockMan::CloseSockets() } void SockMan::EventI2PListen(const CService&, bool) {} + +void SockMan::ThreadI2PAccept() +{ + static constexpr auto err_wait_begin = 1s; + static constexpr auto err_wait_cap = 5min; + auto err_wait = err_wait_begin; + + i2p::Connection conn; + + auto SleepOnFailure = [&]() { + interruptNet.sleep_for(err_wait); + if (err_wait < err_wait_cap) { + err_wait += 1s; + } + }; + + while (!interruptNet) { + + if (!m_i2p_sam_session->Listen(conn)) { + EventI2PListen(conn.me, /*success=*/false); + SleepOnFailure(); + continue; + } + + EventI2PListen(conn.me, /*success=*/true); + + if (!m_i2p_sam_session->Accept(conn)) { + SleepOnFailure(); + continue; + } + + EventNewConnectionAccepted(std::move(conn.sock), conn.me, conn.peer); + + err_wait = err_wait_begin; + } +} diff --git a/src/sockman.h b/src/sockman.h index 836548ef8987..71ac983f887d 100644 --- a/src/sockman.h +++ b/src/sockman.h @@ -5,12 +5,16 @@ #ifndef BITCOIN_SOCKMAN_H #define BITCOIN_SOCKMAN_H +#include #include +#include +#include #include #include #include #include +#include #include typedef int64_t NodeId; @@ -20,6 +24,7 @@ typedef int64_t NodeId; * To use this class, inherit from it and implement the pure virtual methods. * Handled operations: * - binding and listening on sockets + * - starting of necessary threads to process socket operations * - accepting incoming connections */ class SockMan @@ -34,6 +39,7 @@ class SockMan /** * Bind to a new address:port, start listening and add the listen socket to `m_listen`. + * Should be called before `StartSocketsThreads()`. * @param[in] to Where to bind. * @param[out] errmsg Error string if an error occurs. * @retval true Success. @@ -41,6 +47,33 @@ class SockMan */ bool BindAndStartListening(const CService& to, bilingual_str& errmsg); + /** + * Options to influence `StartSocketsThreads()`. + */ + struct Options { + struct I2P { + explicit I2P(const fs::path& file, const Proxy& proxy) : private_key_file{file}, sam_proxy{proxy} {} + + const fs::path private_key_file; + const Proxy sam_proxy; + }; + + /** + * I2P options. If set then a thread will be started that will accept incoming I2P connections. + */ + std::optional i2p; + }; + + /** + * Start the necessary threads for sockets IO. + */ + void StartSocketsThreads(const Options& options); + + /** + * Join (wait for) the threads started by `StartSocketsThreads()` to exit. + */ + void JoinSocketsThreads(); + /** * Accept a connection. * @param[in] listen_sock Socket on which to accept the connection. @@ -63,6 +96,16 @@ class SockMan // Pure virtual functions must be implemented by children classes. // + /** + * Be notified when a new connection has been accepted. + * @param[in] sock Connected socket to communicate with the peer. + * @param[in] me The address and port at our side of the connection. + * @param[in] them The address and port at the peer's side of the connection. + */ + virtual void EventNewConnectionAccepted(std::unique_ptr&& sock, + const CService& me, + const CService& them) = 0; + // // Non-pure virtual functions can be overridden by children classes or left // alone to use the default implementation from SockMan. @@ -79,6 +122,21 @@ class SockMan */ virtual void EventI2PListen(const CService& addr, bool success); + /** + * This is signaled when network activity should cease. + * A pointer to it is saved in `m_i2p_sam_session`, so make sure that + * the lifetime of `interruptNet` is not shorter than + * the lifetime of `m_i2p_sam_session`. + */ + CThreadInterrupt interruptNet; + + /** + * I2P SAM session. + * Used to accept incoming and make outgoing I2P connections from a persistent + * address. + */ + std::unique_ptr m_i2p_sam_session; + /** * List of listening sockets. */ @@ -86,10 +144,21 @@ class SockMan private: + /** + * Accept incoming I2P connections in a loop and call + * `EventNewConnectionAccepted()` for each new connection. + */ + void ThreadI2PAccept(); + /** * The id to assign to the next created node. Used to generate ids of nodes. */ std::atomic m_next_node_id{0}; + + /** + * Thread that accepts incoming I2P connections in a loop, can be stopped via `interruptNet`. + */ + std::thread m_thread_i2p_accept; }; #endif // BITCOIN_SOCKMAN_H From d2a65538549bc9ef7f022b56990bc4043ecb2248 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 20 Sep 2024 13:27:28 +0200 Subject: [PATCH 09/16] net: index nodes in CConnman by id Change `CConnman::m_nodes` from `std::vector` to `std::unordered_map` because interaction between `CConnman` and `SockMan` is going to be based on `NodeId` and finding a node by its id would better be fast. As a nice side effect the existent search-by-id operations in `CConnman::AttemptToEvictConnection()`, `CConnman::DisconnectNode()` and `CConnman::ForNode()` now become `O(1)` (were `O(number of nodes)`), as well as the erase in `CConnman::DisconnectNodes()`. --- src/net.cpp | 148 +++++++++++++------------ src/net.h | 12 +- src/net_processing.cpp | 13 ++- src/rpc/net.cpp | 9 ++ src/test/net_peer_connection_tests.cpp | 10 +- src/test/util/net.h | 6 +- 6 files changed, 109 insertions(+), 89 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index be46de5818d3..a4bbd68b1b43 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -326,7 +326,7 @@ bool IsLocal(const CService& addr) CNode* CConnman::FindNode(const CNetAddr& ip) { LOCK(m_nodes_mutex); - for (CNode* pnode : m_nodes) { + for (const auto& [id, pnode] : m_nodes) { if (static_cast(pnode->addr) == ip) { return pnode; } @@ -337,7 +337,7 @@ CNode* CConnman::FindNode(const CNetAddr& ip) CNode* CConnman::FindNode(const std::string& addrName) { LOCK(m_nodes_mutex); - for (CNode* pnode : m_nodes) { + for (const auto& [id, pnode] : m_nodes) { if (pnode->m_addr_name == addrName) { return pnode; } @@ -348,7 +348,7 @@ CNode* CConnman::FindNode(const std::string& addrName) CNode* CConnman::FindNode(const CService& addr) { LOCK(m_nodes_mutex); - for (CNode* pnode : m_nodes) { + for (const auto& [id, pnode] : m_nodes) { if (static_cast(pnode->addr) == addr) { return pnode; } @@ -364,7 +364,7 @@ bool CConnman::AlreadyConnectedToAddress(const CAddress& addr) bool CConnman::CheckIncomingNonce(uint64_t nonce) { LOCK(m_nodes_mutex); - for (const CNode* pnode : m_nodes) { + for (const auto& [id, pnode] : m_nodes) { if (!pnode->fSuccessfullyConnected && !pnode->IsInboundConn() && pnode->GetLocalNonce() == nonce) return false; } @@ -1661,11 +1661,11 @@ bool CConnman::AttemptToEvictConnection() { LOCK(m_nodes_mutex); - for (const CNode* node : m_nodes) { + for (const auto& [id, node] : m_nodes) { if (node->fDisconnect) continue; NodeEvictionCandidate candidate{ - .id = node->GetId(), + .id = id, .m_connected = node->m_connected, .m_min_ping_time = node->m_min_ping_time, .m_last_block_time = node->m_last_block_time, @@ -1688,12 +1688,13 @@ bool CConnman::AttemptToEvictConnection() return false; } LOCK(m_nodes_mutex); - for (CNode* pnode : m_nodes) { - if (pnode->GetId() == *node_id_to_evict) { - LogDebug(BCLog::NET, "selected %s connection for eviction peer=%d; disconnecting\n", pnode->ConnectionTypeAsString(), pnode->GetId()); - pnode->fDisconnect = true; - return true; - } + auto it{m_nodes.find(*node_id_to_evict)}; + if (it != m_nodes.end()) { + auto id{it->first}; + auto node{it->second}; + LogDebug(BCLog::NET, "selected %s connection for eviction peer=%d; disconnecting\n", node->ConnectionTypeAsString(), id); + node->fDisconnect = true; + return true; } return false; } @@ -1714,7 +1715,7 @@ void CConnman::EventNewConnectionAccepted(std::unique_ptr&& sock, { LOCK(m_nodes_mutex); - for (const CNode* pnode : m_nodes) { + for (const auto& [id, pnode] : m_nodes) { if (pnode->IsInboundConn()) nInbound++; } } @@ -1790,7 +1791,7 @@ void CConnman::EventNewConnectionAccepted(std::unique_ptr&& sock, m_msgproc->InitializeNode(*pnode, local_services); { LOCK(m_nodes_mutex); - m_nodes.push_back(pnode); + m_nodes.emplace(id, pnode); } LogDebug(BCLog::NET, "connection from %s accepted\n", addr.ToStringAddrPort()); @@ -1821,8 +1822,11 @@ bool CConnman::AddConnection(const std::string& address, ConnectionType conn_typ } // no default case, so the compiler can warn about missing cases // Count existing connections - int existing_connections = WITH_LOCK(m_nodes_mutex, - return std::count_if(m_nodes.begin(), m_nodes.end(), [conn_type](CNode* node) { return node->m_conn_type == conn_type; });); + int existing_connections = WITH_LOCK( + m_nodes_mutex, return std::count_if(m_nodes.begin(), m_nodes.end(), [conn_type](const auto& pair) { + const auto node{pair.second}; + return node->m_conn_type == conn_type; + });); // Max connections of specified type already exist if (max_connections != std::nullopt && existing_connections >= max_connections) return false; @@ -1849,49 +1853,51 @@ void CConnman::DisconnectNodes() if (!fNetworkActive) { // Disconnect any connected nodes - for (CNode* pnode : m_nodes) { + for (auto& [id, pnode] : m_nodes) { if (!pnode->fDisconnect) { - LogDebug(BCLog::NET, "Network not active, dropping peer=%d\n", pnode->GetId()); + LogDebug(BCLog::NET, "Network not active, dropping peer=%d\n", id); pnode->fDisconnect = true; } } } // Disconnect unused nodes - std::vector nodes_copy = m_nodes; - for (CNode* pnode : nodes_copy) - { - if (pnode->fDisconnect) - { - // remove from m_nodes - m_nodes.erase(remove(m_nodes.begin(), m_nodes.end(), pnode), m_nodes.end()); - - // Add to reconnection list if appropriate. We don't reconnect right here, because - // the creation of a connection is a blocking operation (up to several seconds), - // and we don't want to hold up the socket handler thread for that long. - if (pnode->m_transport->ShouldReconnectV1()) { - reconnections_to_add.push_back({ - .addr_connect = pnode->addr, - .grant = std::move(pnode->grantOutbound), - .destination = pnode->m_dest, - .conn_type = pnode->m_conn_type, - .use_v2transport = false}); - LogDebug(BCLog::NET, "retrying with v1 transport protocol for peer=%d\n", pnode->GetId()); - } + for (auto it{m_nodes.begin()}; it != m_nodes.end();) { + auto id{it->first}; + auto pnode{it->second}; - // release outbound grant (if any) - pnode->grantOutbound.Release(); + if (!pnode->fDisconnect) { + ++it; + continue; + } - // close socket and cleanup - pnode->CloseSocketDisconnect(); + it = m_nodes.erase(it); + + // Add to reconnection list if appropriate. We don't reconnect right here, because + // the creation of a connection is a blocking operation (up to several seconds), + // and we don't want to hold up the socket handler thread for that long. + if (pnode->m_transport->ShouldReconnectV1()) { + reconnections_to_add.push_back({ + .addr_connect = pnode->addr, + .grant = std::move(pnode->grantOutbound), + .destination = pnode->m_dest, + .conn_type = pnode->m_conn_type, + .use_v2transport = false}); + LogDebug(BCLog::NET, "retrying with v1 transport protocol for peer=%d\n", id); + } - // update connection count by network - if (pnode->IsManualOrFullOutboundConn()) --m_network_conn_counts[pnode->addr.GetNetwork()]; + // release outbound grant (if any) + pnode->grantOutbound.Release(); - // hold in disconnected pool until all refs are released - pnode->Release(); - m_nodes_disconnected.push_back(pnode); - } + // close socket and cleanup + pnode->CloseSocketDisconnect(); + + // update connection count by network + if (pnode->IsManualOrFullOutboundConn()) --m_network_conn_counts[pnode->addr.GetNetwork()]; + + // hold in disconnected pool until all refs are released + pnode->Release(); + m_nodes_disconnected.push_back(pnode); } } { @@ -2360,7 +2366,7 @@ int CConnman::GetFullOutboundConnCount() const int nRelevant = 0; { LOCK(m_nodes_mutex); - for (const CNode* pnode : m_nodes) { + for (const auto& [id, pnode] : m_nodes) { if (pnode->fSuccessfullyConnected && pnode->IsFullOutboundConn()) ++nRelevant; } } @@ -2378,7 +2384,7 @@ int CConnman::GetExtraFullOutboundCount() const int full_outbound_peers = 0; { LOCK(m_nodes_mutex); - for (const CNode* pnode : m_nodes) { + for (const auto& [id, pnode] : m_nodes) { if (pnode->fSuccessfullyConnected && !pnode->fDisconnect && pnode->IsFullOutboundConn()) { ++full_outbound_peers; } @@ -2392,7 +2398,7 @@ int CConnman::GetExtraBlockRelayCount() const int block_relay_peers = 0; { LOCK(m_nodes_mutex); - for (const CNode* pnode : m_nodes) { + for (const auto& [id, pnode] : m_nodes) { if (pnode->fSuccessfullyConnected && !pnode->fDisconnect && pnode->IsBlockOnlyConn()) { ++block_relay_peers; } @@ -2563,7 +2569,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, Spa { LOCK(m_nodes_mutex); - for (const CNode* pnode : m_nodes) { + for (const auto& [id, pnode] : m_nodes) { if (pnode->IsFullOutboundConn()) nOutboundFullRelay++; if (pnode->IsBlockOnlyConn()) nOutboundBlockRelay++; @@ -2808,7 +2814,7 @@ std::vector CConnman::GetCurrentBlockRelayOnlyConns() const { std::vector ret; LOCK(m_nodes_mutex); - for (const CNode* pnode : m_nodes) { + for (const auto& [id, pnode] : m_nodes) { if (pnode->IsBlockOnlyConn()) { ret.push_back(pnode->addr); } @@ -2834,7 +2840,7 @@ std::vector CConnman::GetAddedNodeInfo(bool include_connected) co std::map> mapConnectedByName; { LOCK(m_nodes_mutex); - for (const CNode* pnode : m_nodes) { + for (const auto& [id, pnode] : m_nodes) { if (pnode->addr.IsValid()) { mapConnected[pnode->addr] = pnode->IsInboundConn(); } @@ -2938,7 +2944,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai m_msgproc->InitializeNode(*pnode, nLocalServices); { LOCK(m_nodes_mutex); - m_nodes.push_back(pnode); + m_nodes.emplace(pnode->GetId(), pnode); // update connection count by network if (pnode->IsManualOrFullOutboundConn()) ++m_network_conn_counts[pnode->addr.GetNetwork()]; @@ -3325,9 +3331,9 @@ void CConnman::StopNodes() } // Delete peer connections. - std::vector nodes; + decltype(m_nodes) nodes; WITH_LOCK(m_nodes_mutex, nodes.swap(m_nodes)); - for (CNode* pnode : nodes) { + for (auto& [id, pnode] : nodes) { pnode->CloseSocketDisconnect(); DeleteNode(pnode); } @@ -3455,7 +3461,7 @@ size_t CConnman::GetNodeCount(ConnectionDirection flags) const return m_nodes.size(); int nNum = 0; - for (const auto& pnode : m_nodes) { + for (const auto& [id, pnode] : m_nodes) { if (flags & (pnode->IsInboundConn() ? ConnectionDirection::In : ConnectionDirection::Out)) { nNum++; } @@ -3481,7 +3487,7 @@ void CConnman::GetNodeStats(std::vector& vstats) const vstats.clear(); LOCK(m_nodes_mutex); vstats.reserve(m_nodes.size()); - for (CNode* pnode : m_nodes) { + for (const auto& [id, pnode] : m_nodes) { vstats.emplace_back(); pnode->CopyStats(vstats.back()); vstats.back().m_mapped_as = GetMappedAS(pnode->addr); @@ -3503,7 +3509,7 @@ bool CConnman::DisconnectNode(const CSubNet& subnet) { bool disconnected = false; LOCK(m_nodes_mutex); - for (CNode* pnode : m_nodes) { + for (auto& [id, pnode] : m_nodes) { if (subnet.Match(pnode->addr)) { LogDebug(BCLog::NET, "disconnect by subnet%s matched peer=%d; disconnecting\n", (fLogIPs ? strprintf("=%s", subnet.ToString()) : ""), pnode->GetId()); pnode->fDisconnect = true; @@ -3521,14 +3527,14 @@ bool CConnman::DisconnectNode(const CNetAddr& addr) bool CConnman::DisconnectNode(NodeId id) { LOCK(m_nodes_mutex); - for(CNode* pnode : m_nodes) { - if (id == pnode->GetId()) { - LogDebug(BCLog::NET, "disconnect by id peer=%d; disconnecting\n", pnode->GetId()); - pnode->fDisconnect = true; - return true; - } + auto it{m_nodes.find(id)}; + if (it == m_nodes.end()) { + return false; } - return false; + auto node{it->second}; + LogDebug(BCLog::NET, "disconnect by id peer=%d; disconnecting\n", id); + node->fDisconnect = true; + return true; } void CConnman::RecordBytesRecv(uint64_t bytes) @@ -3773,11 +3779,9 @@ bool CConnman::ForNode(NodeId id, std::function func) { CNode* found = nullptr; LOCK(m_nodes_mutex); - for (auto&& pnode : m_nodes) { - if(pnode->GetId() == id) { - found = pnode; - break; - } + auto it{m_nodes.find(id)}; + if (it != m_nodes.end()) { + found = it->second; } return found != nullptr && NodeFullyConnected(found) && func(found); } diff --git a/src/net.h b/src/net.h index 076583224378..a55d3a5a8630 100644 --- a/src/net.h +++ b/src/net.h @@ -44,6 +44,7 @@ #include #include #include +#include #include #include @@ -1131,7 +1132,7 @@ class CConnman : private SockMan void ForEachNode(const NodeFn& func) { LOCK(m_nodes_mutex); - for (auto&& node : m_nodes) { + for (auto& [id, node] : m_nodes) { if (NodeFullyConnected(node)) func(node); } @@ -1140,7 +1141,7 @@ class CConnman : private SockMan void ForEachNode(const NodeFn& func) const { LOCK(m_nodes_mutex); - for (auto&& node : m_nodes) { + for (auto& [id, node] : m_nodes) { if (NodeFullyConnected(node)) func(node); } @@ -1410,7 +1411,7 @@ class CConnman : private SockMan std::vector m_added_node_params GUARDED_BY(m_added_nodes_mutex); mutable Mutex m_added_nodes_mutex; - std::vector m_nodes GUARDED_BY(m_nodes_mutex); + std::unordered_map m_nodes GUARDED_BY(m_nodes_mutex); std::list m_nodes_disconnected; mutable RecursiveMutex m_nodes_mutex; unsigned int nPrevNodeCount{0}; @@ -1596,8 +1597,9 @@ class CConnman : private SockMan { { LOCK(connman.m_nodes_mutex); - m_nodes_copy = connman.m_nodes; - for (auto& node : m_nodes_copy) { + m_nodes_copy.reserve(connman.m_nodes.size()); + for (auto& [in, node] : connman.m_nodes) { + m_nodes_copy.push_back(node); node->AddRef(); } } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b7d0f5360dea..08864f870f39 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5523,10 +5523,15 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now) m_connman.ForEachNode([&](CNode* pnode) { if (!pnode->IsBlockOnlyConn() || pnode->fDisconnect) return; - if (pnode->GetId() > youngest_peer.first) { - next_youngest_peer = youngest_peer; - youngest_peer.first = pnode->GetId(); - youngest_peer.second = pnode->m_last_block_time; + if (pnode->GetId() > next_youngest_peer.first) { + if (pnode->GetId() > youngest_peer.first) { + next_youngest_peer = youngest_peer; + youngest_peer.first = pnode->GetId(); + youngest_peer.second = pnode->m_last_block_time; + } else { + next_youngest_peer.first = pnode->GetId(); + next_youngest_peer.second = pnode->m_last_block_time; + } } }); NodeId to_disconnect = youngest_peer.first; diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 1119a3e668e5..d31f40f144ab 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -203,6 +203,15 @@ static RPCHelpMan getpeerinfo() std::vector vstats; connman.GetNodeStats(vstats); + // An undocumented side effect of how CConnman previously stored nodes was + // that they were returned ordered by id. At least some functional tests + // rely on that, so keep it that way. An alternative is to remove this sort + // and fix the tests and take the risk of breaking other users of the + // "getpeerinfo" RPC. + std::sort(vstats.begin(), vstats.end(), [](const CNodeStats& a, const CNodeStats& b) { + return a.nodeid < b.nodeid; + }); + UniValue ret(UniValue::VARR); for (const CNodeStats& stats : vstats) { diff --git a/src/test/net_peer_connection_tests.cpp b/src/test/net_peer_connection_tests.cpp index e60ce8b99d36..33e8a7cc07ad 100644 --- a/src/test/net_peer_connection_tests.cpp +++ b/src/test/net_peer_connection_tests.cpp @@ -117,9 +117,9 @@ BOOST_FIXTURE_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection, BOOST_CHECK_EQUAL(nodes.back()->ConnectedThroughNetwork(), Network::NET_CJDNS); BOOST_TEST_MESSAGE("Call AddNode() for all the peers"); - for (auto node : connman->TestNodes()) { + for (const auto& [id, node] : connman->TestNodes()) { BOOST_CHECK(connman->AddNode({/*m_added_node=*/node->addr.ToStringAddrPort(), /*m_use_v2transport=*/true})); - BOOST_TEST_MESSAGE(strprintf("peer id=%s addr=%s", node->GetId(), node->addr.ToStringAddrPort())); + BOOST_TEST_MESSAGE(strprintf("peer id=%s addr=%s", id, node->addr.ToStringAddrPort())); } BOOST_TEST_MESSAGE("\nCall AddNode() with 2 addrs resolving to existing localhost addnode entry; neither should be added"); @@ -134,7 +134,7 @@ BOOST_FIXTURE_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection, BOOST_CHECK(connman->GetAddedNodeInfo(/*include_connected=*/false).empty()); // Test AddedNodesContain() - for (auto node : connman->TestNodes()) { + for (const auto& [id, node] : connman->TestNodes()) { BOOST_CHECK(connman->AddedNodesContain(node->addr)); } AddPeer(id, nodes, *peerman, *connman, ConnectionType::OUTBOUND_FULL_RELAY); @@ -151,12 +151,12 @@ BOOST_FIXTURE_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection, } BOOST_TEST_MESSAGE("\nCheck that all connected peers are correctly detected as connected"); - for (auto node : connman->TestNodes()) { + for (const auto& [id, node] : connman->TestNodes()) { BOOST_CHECK(connman->AlreadyConnectedPublic(node->addr)); } // Clean up - for (auto node : connman->TestNodes()) { + for (const auto& [id, node] : connman->TestNodes()) { peerman->FinalizeNode(*node); } connman->ClearTestNodes(); diff --git a/src/test/util/net.h b/src/test/util/net.h index 043e317bf080..10a11b09019d 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -44,7 +44,7 @@ struct ConnmanTestMsg : public CConnman { m_peer_connect_timeout = timeout; } - std::vector TestNodes() + auto TestNodes() { LOCK(m_nodes_mutex); return m_nodes; @@ -53,7 +53,7 @@ struct ConnmanTestMsg : public CConnman { void AddTestNode(CNode& node) { LOCK(m_nodes_mutex); - m_nodes.push_back(&node); + m_nodes.emplace(node.GetId(), &node); if (node.IsManualOrFullOutboundConn()) ++m_network_conn_counts[node.addr.GetNetwork()]; } @@ -61,7 +61,7 @@ struct ConnmanTestMsg : public CConnman { void ClearTestNodes() { LOCK(m_nodes_mutex); - for (CNode* node : m_nodes) { + for (auto& [id, node] : m_nodes) { delete node; } m_nodes.clear(); From aa86d01f9b76a5a1246a4bda7202760f648ef674 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Sat, 21 Sep 2024 10:05:34 +0200 Subject: [PATCH 10/16] net: isolate P2P specifics from GenerateWaitSockets() Move the parts of `CConnman::GenerateWaitSockets()` that are specific to the Bitcoin-P2P protocol to dedicated methods: `ShouldTryToSend()` and `ShouldTryToRecv()`. This brings us one step closer to moving `GenerateWaitSockets()` to the protocol agnostic `SockMan` (which would call `ShouldTry...()` from `CConnman`). --- src/net.cpp | 52 +++++++++++++++++++++++++++++++++++++++---------- src/net.h | 14 +++++++++++-- src/sockman.cpp | 4 ++++ src/sockman.h | 16 +++++++++++++++ 4 files changed, 74 insertions(+), 12 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index a4bbd68b1b43..bd3baa86a642 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1647,6 +1647,16 @@ std::pair CConnman::SocketSendData(CNode& node) const return {nSentSize, data_left}; } +CNode* CConnman::GetNodeById(NodeId node_id) const +{ + LOCK(m_nodes_mutex); + auto it{m_nodes.find(node_id)}; + if (it != m_nodes.end()) { + return it->second; + } + return nullptr; +} + /** Try to find a connection to evict when the node is full. * Extreme care must be taken to avoid opening the node to attacker * triggered network partitioning. @@ -1976,8 +1986,37 @@ bool CConnman::InactivityCheck(const CNode& node) const return false; } +bool CConnman::ShouldTryToSend(NodeId node_id) const +{ + AssertLockNotHeld(m_nodes_mutex); + + CNode* node{GetNodeById(node_id)}; + if (node == nullptr) { + return false; + } + LOCK(node->cs_vSend); + // Sending is possible if either there are bytes to send right now, or if there will be + // once a potential message from vSendMsg is handed to the transport. GetBytesToSend + // determines both of these in a single call. + const auto& [to_send, more, _msg_type] = node->m_transport->GetBytesToSend(!node->vSendMsg.empty()); + return !to_send.empty() || more; +} + +bool CConnman::ShouldTryToRecv(NodeId node_id) const +{ + AssertLockNotHeld(m_nodes_mutex); + + CNode* node{GetNodeById(node_id)}; + if (node == nullptr) { + return false; + } + return !node->fPauseRecv; +} + Sock::EventsPerSock CConnman::GenerateWaitSockets(Span nodes) { + AssertLockNotHeld(m_nodes_mutex); + Sock::EventsPerSock events_per_sock; for (const auto& sock : m_listen) { @@ -1985,16 +2024,8 @@ Sock::EventsPerSock CConnman::GenerateWaitSockets(Span nodes) } for (CNode* pnode : nodes) { - bool select_recv = !pnode->fPauseRecv; - bool select_send; - { - LOCK(pnode->cs_vSend); - // Sending is possible if either there are bytes to send right now, or if there will be - // once a potential message from vSendMsg is handed to the transport. GetBytesToSend - // determines both of these in a single call. - const auto& [to_send, more, _msg_type] = pnode->m_transport->GetBytesToSend(!pnode->vSendMsg.empty()); - select_send = !to_send.empty() || more; - } + const bool select_recv{ShouldTryToRecv(pnode->GetId())}; + const bool select_send{ShouldTryToSend(pnode->GetId())}; if (!select_recv && !select_send) continue; LOCK(pnode->m_sock_mutex); @@ -2154,6 +2185,7 @@ void CConnman::SocketHandlerListening(const Sock::EventsPerSock& events_per_sock void CConnman::ThreadSocketHandler() { + AssertLockNotHeld(m_nodes_mutex); AssertLockNotHeld(m_total_bytes_sent_mutex); while (!interruptNet) diff --git a/src/net.h b/src/net.h index a55d3a5a8630..d1a6e628ff11 100644 --- a/src/net.h +++ b/src/net.h @@ -1287,17 +1287,25 @@ class CConnman : private SockMan /** Return true if the peer is inactive and should be disconnected. */ bool InactivityCheck(const CNode& node) const; + virtual bool ShouldTryToSend(NodeId node_id) const override + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + + virtual bool ShouldTryToRecv(NodeId node_id) const override + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + /** * Generate a collection of sockets to check for IO readiness. * @param[in] nodes Select from these nodes' sockets. * @return sockets to check for readiness */ - Sock::EventsPerSock GenerateWaitSockets(Span nodes); + Sock::EventsPerSock GenerateWaitSockets(Span nodes) + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); /** * Check connected and listening sockets for IO readiness and process them accordingly. */ - void SocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc); + void SocketHandler() + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_total_bytes_sent_mutex, !mutexMsgProc); /** * Do the read/write for connected sockets that are ready for IO. @@ -1410,6 +1418,8 @@ class CConnman : private SockMan // connection string and whether to use v2 p2p std::vector m_added_node_params GUARDED_BY(m_added_nodes_mutex); + CNode* GetNodeById(NodeId node_id) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + mutable Mutex m_added_nodes_mutex; std::unordered_map m_nodes GUARDED_BY(m_nodes_mutex); std::list m_nodes_disconnected; diff --git a/src/sockman.cpp b/src/sockman.cpp index e3810d79da39..db85edfa195d 100644 --- a/src/sockman.cpp +++ b/src/sockman.cpp @@ -142,6 +142,10 @@ void SockMan::CloseSockets() m_listen.clear(); } +bool SockMan::ShouldTryToSend(NodeId node_id) const { return true; } + +bool SockMan::ShouldTryToRecv(NodeId node_id) const { return true; } + void SockMan::EventI2PListen(const CService&, bool) {} void SockMan::ThreadI2PAccept() diff --git a/src/sockman.h b/src/sockman.h index 71ac983f887d..29d8895b38b6 100644 --- a/src/sockman.h +++ b/src/sockman.h @@ -111,6 +111,22 @@ class SockMan // alone to use the default implementation from SockMan. // + /** + * SockMan would only call EventReadyToSend() if this returns true. + * Can be used to temporary pause sends for a node. + * The implementation in SockMan always returns true. + * @param[in] node_id Node for which to confirm or cancel a call to EventReadyToSend(). + */ + virtual bool ShouldTryToSend(NodeId node_id) const; + + /** + * SockMan would only call Recv() on a node's socket if this returns true. + * Can be used to temporary pause receives for a node. + * The implementation in SockMan always returns true. + * @param[in] node_id Node for which to confirm or cancel a receive. + */ + virtual bool ShouldTryToRecv(NodeId node_id) const; + /** * Be notified of a change in the state of listening for incoming I2P connections. * The default behavior, implemented by `SockMan`, is to ignore this event. From 4b6565c326d60298ac9657ee31f022808beaabc8 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Sat, 21 Sep 2024 10:31:53 +0200 Subject: [PATCH 11/16] net: isolate P2P specifics from SocketHandlerConnected() and ThreadSocketHandler() Move some parts of `CConnman::SocketHandlerConnected()` and `CConnman::ThreadSocketHandler()` that are specific to the Bitcoin-P2P protocol to dedicated methods: `EventIOLoopCompletedForNode()` and `EventIOLoopCompletedForAllPeers()`. This brings us one step closer to moving `SocketHandlerConnected()` and `ThreadSocketHandler()` to the protocol agnostic `SockMan` (which would call `EventIOLoopCompleted...()` from `CConnman`). --- src/net.cpp | 29 ++++++++++++++++++++++++++--- src/net.h | 8 +++++++- src/sockman.cpp | 4 ++++ src/sockman.h | 16 ++++++++++++++++ 4 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index bd3baa86a642..7029c35f6219 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2013,6 +2013,29 @@ bool CConnman::ShouldTryToRecv(NodeId node_id) const return !node->fPauseRecv; } +void CConnman::EventIOLoopCompletedForNode(NodeId node_id) +{ + AssertLockNotHeld(m_nodes_mutex); + + CNode* node{GetNodeById(node_id)}; + if (node == nullptr) { + return; + } + + if (InactivityCheck(*node)) { + node->fDisconnect = true; + } +} + +void CConnman::EventIOLoopCompletedForAllPeers() +{ + AssertLockNotHeld(m_nodes_mutex); + AssertLockNotHeld(m_reconnections_mutex); + + DisconnectNodes(); + NotifyNumConnectionsChanged(); +} + Sock::EventsPerSock CConnman::GenerateWaitSockets(Span nodes) { AssertLockNotHeld(m_nodes_mutex); @@ -2069,6 +2092,7 @@ void CConnman::SocketHandler() void CConnman::SocketHandlerConnected(const std::vector& nodes, const Sock::EventsPerSock& events_per_sock) { + AssertLockNotHeld(m_nodes_mutex); AssertLockNotHeld(m_total_bytes_sent_mutex); for (CNode* pnode : nodes) { @@ -2157,7 +2181,7 @@ void CConnman::SocketHandlerConnected(const std::vector& nodes, } } - if (InactivityCheck(*pnode)) pnode->fDisconnect = true; + EventIOLoopCompletedForNode(pnode->GetId()); } } @@ -2190,8 +2214,7 @@ void CConnman::ThreadSocketHandler() while (!interruptNet) { - DisconnectNodes(); - NotifyNumConnectionsChanged(); + EventIOLoopCompletedForAllPeers(); SocketHandler(); } } diff --git a/src/net.h b/src/net.h index d1a6e628ff11..26a6779c47b9 100644 --- a/src/net.h +++ b/src/net.h @@ -1293,6 +1293,12 @@ class CConnman : private SockMan virtual bool ShouldTryToRecv(NodeId node_id) const override EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + virtual void EventIOLoopCompletedForNode(NodeId node_id) override + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + + virtual void EventIOLoopCompletedForAllPeers() override + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_reconnections_mutex); + /** * Generate a collection of sockets to check for IO readiness. * @param[in] nodes Select from these nodes' sockets. @@ -1314,7 +1320,7 @@ class CConnman : private SockMan */ void SocketHandlerConnected(const std::vector& nodes, const Sock::EventsPerSock& events_per_sock) - EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc); + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_total_bytes_sent_mutex, !mutexMsgProc); /** * Accept incoming connections, one from each read-ready listening socket. diff --git a/src/sockman.cpp b/src/sockman.cpp index db85edfa195d..721ae5cec2bc 100644 --- a/src/sockman.cpp +++ b/src/sockman.cpp @@ -146,6 +146,10 @@ bool SockMan::ShouldTryToSend(NodeId node_id) const { return true; } bool SockMan::ShouldTryToRecv(NodeId node_id) const { return true; } +void SockMan::EventIOLoopCompletedForNode(NodeId node_id) {} + +void SockMan::EventIOLoopCompletedForAllPeers() {} + void SockMan::EventI2PListen(const CService&, bool) {} void SockMan::ThreadI2PAccept() diff --git a/src/sockman.h b/src/sockman.h index 29d8895b38b6..8b9ea632ddf0 100644 --- a/src/sockman.h +++ b/src/sockman.h @@ -127,6 +127,22 @@ class SockMan */ virtual bool ShouldTryToRecv(NodeId node_id) const; + /** + * SockMan has completed the current send+recv iteration for a node. + * It will do another send+recv for this node after processing all other nodes. + * Can be used to execute periodic tasks for a given node. + * The implementation in SockMan does nothing. + * @param[in] node_id Node for which send+recv has been done. + */ + virtual void EventIOLoopCompletedForNode(NodeId node_id); + + /** + * SockMan has completed send+recv for all nodes. + * Can be used to execute periodic tasks for all nodes. + * The implementation in SockMan does nothing. + */ + virtual void EventIOLoopCompletedForAllPeers(); + /** * Be notified of a change in the state of listening for incoming I2P connections. * The default behavior, implemented by `SockMan`, is to ignore this event. From cda7b3a1d8feb98483c2d71186b11a45c83fe698 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Sun, 22 Sep 2024 12:11:42 +0200 Subject: [PATCH 12/16] net: isolate all remaining P2P specifics from SocketHandlerConnected() Introduce 4 new methods for the interaction between `CConnman` and `SockMan`: * `EventReadyToSend()`: called when there is readiness to send and do the actual sending of data. * `EventGotData()`, `EventGotEOF()`, `EventGotPermanentReadError()`: called when the corresponing recv events occur. These methods contain logic that is specific to the Bitcoin-P2P protocol and move it away from `CConnman::SocketHandlerConnected()` which will become a protocol agnostic method of `SockMan`. Also, move the counting of sent bytes to `CConnman::SocketSendData()` - both callers of that method called `RecordBytesSent()` just after the call, so move it from the callers to inside `CConnman::SocketSendData()`. --- src/net.cpp | 126 ++++++++++++++++++++++++++++++++++++-------------- src/net.h | 15 +++++- src/sockman.h | 33 +++++++++++++ 3 files changed, 138 insertions(+), 36 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 7029c35f6219..54dcfee482bd 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1568,8 +1568,10 @@ Transport::Info V2Transport::GetInfo() const noexcept return info; } -std::pair CConnman::SocketSendData(CNode& node) const +std::pair CConnman::SocketSendData(CNode& node) { + AssertLockNotHeld(m_total_bytes_sent_mutex); + auto it = node.vSendMsg.begin(); size_t nSentSize = 0; bool data_left{false}; //!< second return value (whether unsent data remains) @@ -1644,6 +1646,11 @@ std::pair CConnman::SocketSendData(CNode& node) const assert(node.m_send_memusage == 0); } node.vSendMsg.erase(node.vSendMsg.begin(), it); + + if (nSentSize > 0) { + RecordBytesSent(nSentSize); + } + return {nSentSize, data_left}; } @@ -1986,6 +1993,79 @@ bool CConnman::InactivityCheck(const CNode& node) const return false; } +void CConnman::EventReadyToSend(NodeId node_id, bool& cancel_recv) +{ + AssertLockNotHeld(m_nodes_mutex); + + CNode* node{GetNodeById(node_id)}; + if (node == nullptr) { + cancel_recv = true; + return; + } + + const auto [bytes_sent, data_left] = WITH_LOCK(node->cs_vSend, return SocketSendData(*node);); + + // If both receiving and (non-optimistic) sending were possible, we first attempt + // sending. If that succeeds, but does not fully drain the send queue, do not + // attempt to receive. This avoids needlessly queueing data if the remote peer + // is slow at receiving data, by means of TCP flow control. We only do this when + // sending actually succeeded to make sure progress is always made; otherwise a + // deadlock would be possible when both sides have data to send, but neither is + // receiving. + cancel_recv = bytes_sent > 0 && data_left; +} + +void CConnman::EventGotData(NodeId node_id, const uint8_t* data, size_t n) +{ + AssertLockNotHeld(mutexMsgProc); + AssertLockNotHeld(m_nodes_mutex); + + CNode* node{GetNodeById(node_id)}; + if (node == nullptr) { + return; + } + + bool notify = false; + if (!node->ReceiveMsgBytes({data, n}, notify)) { + node->CloseSocketDisconnect(); + } + RecordBytesRecv(n); + if (notify) { + node->MarkReceivedMsgsForProcessing(); + WakeMessageHandler(); + } +} + +void CConnman::EventGotEOF(NodeId node_id) +{ + AssertLockNotHeld(m_nodes_mutex); + + CNode* node{GetNodeById(node_id)}; + if (node == nullptr) { + return; + } + + if (!node->fDisconnect) { + LogDebug(BCLog::NET, "socket closed for peer=%d\n", node_id); + } + node->CloseSocketDisconnect(); +} + +void CConnman::EventGotPermanentReadError(NodeId node_id, const std::string& errmsg) +{ + AssertLockNotHeld(m_nodes_mutex); + + CNode* node{GetNodeById(node_id)}; + if (node == nullptr) { + return; + } + + if (!node->fDisconnect) { + LogDebug(BCLog::NET, "socket recv error for peer=%d: %s\n", node_id, errmsg); + } + node->CloseSocketDisconnect(); +} + bool CConnman::ShouldTryToSend(NodeId node_id) const { AssertLockNotHeld(m_nodes_mutex); @@ -2119,19 +2199,12 @@ void CConnman::SocketHandlerConnected(const std::vector& nodes, } if (sendSet) { - // Send data - auto [bytes_sent, data_left] = WITH_LOCK(pnode->cs_vSend, return SocketSendData(*pnode)); - if (bytes_sent) { - RecordBytesSent(bytes_sent); - - // If both receiving and (non-optimistic) sending were possible, we first attempt - // sending. If that succeeds, but does not fully drain the send queue, do not - // attempt to receive. This avoids needlessly queueing data if the remote peer - // is slow at receiving data, by means of TCP flow control. We only do this when - // sending actually succeeded to make sure progress is always made; otherwise a - // deadlock would be possible when both sides have data to send, but neither is - // receiving. - if (data_left) recvSet = false; + bool cancel_recv; + + EventReadyToSend(pnode->GetId(), cancel_recv); + + if (cancel_recv) { + recvSet = false; } } @@ -2149,23 +2222,11 @@ void CConnman::SocketHandlerConnected(const std::vector& nodes, } if (nBytes > 0) { - bool notify = false; - if (!pnode->ReceiveMsgBytes({pchBuf, (size_t)nBytes}, notify)) { - pnode->CloseSocketDisconnect(); - } - RecordBytesRecv(nBytes); - if (notify) { - pnode->MarkReceivedMsgsForProcessing(); - WakeMessageHandler(); - } + EventGotData(pnode->GetId(), pchBuf, nBytes); } else if (nBytes == 0) { - // socket closed gracefully - if (!pnode->fDisconnect) { - LogDebug(BCLog::NET, "socket closed for peer=%d\n", pnode->GetId()); - } - pnode->CloseSocketDisconnect(); + EventGotEOF(pnode->GetId()); } else if (nBytes < 0) { @@ -2173,10 +2234,7 @@ void CConnman::SocketHandlerConnected(const std::vector& nodes, int nErr = WSAGetLastError(); if (nErr != WSAEWOULDBLOCK && nErr != WSAEMSGSIZE && nErr != WSAEINTR && nErr != WSAEINPROGRESS) { - if (!pnode->fDisconnect) { - LogDebug(BCLog::NET, "socket recv error for peer=%d: %s\n", pnode->GetId(), NetworkErrorString(nErr)); - } - pnode->CloseSocketDisconnect(); + EventGotPermanentReadError(pnode->GetId(), NetworkErrorString(nErr)); } } } @@ -3801,7 +3859,6 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg) msg.data.data() ); - size_t nBytesSent = 0; { LOCK(pnode->cs_vSend); // Check if the transport still has unsent bytes, and indicate to it that we're about to @@ -3824,10 +3881,9 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg) // results in sendable bytes there, but with V2Transport this is not the case (it may // still be in the handshake). if (queue_was_empty && more) { - std::tie(nBytesSent, std::ignore) = SocketSendData(*pnode); + SocketSendData(*pnode); } } - if (nBytesSent) RecordBytesSent(nBytesSent); } bool CConnman::ForNode(NodeId id, std::function func) diff --git a/src/net.h b/src/net.h index 26a6779c47b9..05e35e36a6bb 100644 --- a/src/net.h +++ b/src/net.h @@ -1287,6 +1287,18 @@ class CConnman : private SockMan /** Return true if the peer is inactive and should be disconnected. */ bool InactivityCheck(const CNode& node) const; + void EventReadyToSend(NodeId node_id, bool& cancel_recv) override + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + + virtual void EventGotData(NodeId node_id, const uint8_t* data, size_t n) override + EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc, !m_nodes_mutex); + + virtual void EventGotEOF(NodeId node_id) override + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + + virtual void EventGotPermanentReadError(NodeId node_id, const std::string& errmsg) override + EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); + virtual bool ShouldTryToSend(NodeId node_id) const override EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); @@ -1350,7 +1362,8 @@ class CConnman : private SockMan void DeleteNode(CNode* pnode); /** (Try to) send data from node's vSendMsg. Returns (bytes_sent, data_left). */ - std::pair SocketSendData(CNode& node) const EXCLUSIVE_LOCKS_REQUIRED(node.cs_vSend); + std::pair SocketSendData(CNode& node) + EXCLUSIVE_LOCKS_REQUIRED(node.cs_vSend, !m_total_bytes_sent_mutex); void DumpAddresses(); diff --git a/src/sockman.h b/src/sockman.h index 8b9ea632ddf0..90d0150fa3c1 100644 --- a/src/sockman.h +++ b/src/sockman.h @@ -106,6 +106,39 @@ class SockMan const CService& me, const CService& them) = 0; + /** + * Called when the socket is ready to send data and `ShouldTryToSend()` has + * returned true. This is where the higher level code serializes its messages + * and calls `SockMan::SendBytes()`. + * @param[in] node_id Id of the node whose socket is ready to send. + * @param[out] cancel_recv Should always be set upon return and if it is true, + * then the next attempt to receive data from that node will be omitted. + */ + virtual void EventReadyToSend(NodeId node_id, bool& cancel_recv) = 0; + + /** + * Called when new data has been received. + * @param[in] node_id Node for which the data arrived. + * @param[in] data Data buffer. + * @param[in] n Number of bytes in `data`. + */ + virtual void EventGotData(NodeId node_id, const uint8_t* data, size_t n) = 0; + + /** + * Called when the remote peer has sent an EOF on the socket. This is a graceful + * close of their writing side, we can still send and they will receive, if it + * makes sense at the application level. + * @param[in] node_id Node whose socket got EOF. + */ + virtual void EventGotEOF(NodeId node_id) = 0; + + /** + * Called when we get an irrecoverable error trying to read from a socket. + * @param[in] node_id Node whose socket got an error. + * @param[in] errmsg Message describing the error. + */ + virtual void EventGotPermanentReadError(NodeId node_id, const std::string& errmsg) = 0; + // // Non-pure virtual functions can be overridden by children classes or left // alone to use the default implementation from SockMan. From 2c22d06faa9d31d86c7e3ded30d0195f4f52106c Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 23 Sep 2024 12:50:25 +0200 Subject: [PATCH 13/16] net: split CConnman::ConnectNode() Move the protocol agnostic parts of `CConnman::ConnectNode()` into `SockMan::ConnectAndMakeNodeId()` and leave the Bitcoin-P2P specific stuff in `CConnman::ConnectNode()`. Move the protocol agnostic `CConnman::m_unused_i2p_sessions`, its mutex and `MAX_UNUSED_I2P_SESSIONS_SIZE` to `SockMan`. Move `GetBindAddress()` from `net.cpp` to `sockman.cpp`. --- src/net.cpp | 100 +++++++++++++------------------------------- src/net.h | 35 ++++------------ src/sockman.cpp | 90 +++++++++++++++++++++++++++++++++++++++ src/sockman.h | 56 +++++++++++++++++++++++++ src/test/util/net.h | 3 +- 5 files changed, 183 insertions(+), 101 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 54dcfee482bd..9e426a9162b3 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -371,23 +371,8 @@ bool CConnman::CheckIncomingNonce(uint64_t nonce) 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.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, bool use_v2transport) { - AssertLockNotHeld(m_unused_i2p_sessions_mutex); assert(conn_type != ConnectionType::INBOUND); if (pszDest == nullptr) { @@ -449,52 +434,29 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo // Connect std::unique_ptr sock; Proxy proxy; - CAddress addr_bind; - assert(!addr_bind.IsValid()); + assert(!proxy.IsValid()); std::unique_ptr i2p_transient_session; + std::optional node_id; + CService me; + for (auto& target_addr: connect_to) { if (target_addr.IsValid()) { const bool use_proxy{GetProxy(target_addr.GetNetwork(), proxy)}; bool proxyConnectionFailed = false; - if (target_addr.IsI2P() && use_proxy) { - i2p::Connection conn; - bool connected{false}; - - if (m_i2p_sam_session) { - connected = m_i2p_sam_session->Connect(target_addr, conn, proxyConnectionFailed); - } else { - { - LOCK(m_unused_i2p_sessions_mutex); - if (m_unused_i2p_sessions.empty()) { - i2p_transient_session = - std::make_unique(proxy, &interruptNet); - } else { - i2p_transient_session.swap(m_unused_i2p_sessions.front()); - m_unused_i2p_sessions.pop(); - } - } - connected = i2p_transient_session->Connect(target_addr, conn, proxyConnectionFailed); - if (!connected) { - LOCK(m_unused_i2p_sessions_mutex); - if (m_unused_i2p_sessions.size() < MAX_UNUSED_I2P_SESSIONS_SIZE) { - m_unused_i2p_sessions.emplace(i2p_transient_session.release()); - } - } - } - - if (connected) { - sock = std::move(conn.sock); - addr_bind = CAddress{conn.me, NODE_NONE}; - } - } else if (use_proxy) { + if (use_proxy && !target_addr.IsI2P()) { LogPrintLevel(BCLog::PROXY, BCLog::Level::Debug, "Using proxy: %s to connect to %s\n", proxy.ToString(), target_addr.ToStringAddrPort()); - sock = ConnectThroughProxy(proxy, target_addr.ToStringAddr(), target_addr.GetPort(), proxyConnectionFailed); - } else { - // no proxy needed (none set for target network) - sock = ConnectDirectly(target_addr, conn_type == ConnectionType::MANUAL); } + + node_id = ConnectAndMakeNodeId(target_addr, + /*is_important=*/conn_type == ConnectionType::MANUAL, + proxy, + proxyConnectionFailed, + me, + sock, + i2p_transient_session); + if (!proxyConnectionFailed) { // If a connection to the node was attempted, and failure (if any) is not caused by a problem connecting to // the proxy, mark this as an attempt. @@ -503,12 +465,19 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo } else if (pszDest && GetNameProxy(proxy)) { std::string host; uint16_t port{default_port}; - SplitHostPort(std::string(pszDest), port, host); - bool proxyConnectionFailed; - sock = ConnectThroughProxy(proxy, host, port, proxyConnectionFailed); + SplitHostPort(pszDest, port, host); + + bool dummy; + node_id = ConnectAndMakeNodeId(StringHostIntPort{host, port}, + /*is_important=*/conn_type == ConnectionType::MANUAL, + proxy, + dummy, + me, + sock, + i2p_transient_session); } // Check any other resolved address (if any) if we fail to connect - if (!sock) { + if (!node_id.has_value()) { continue; } @@ -516,18 +485,13 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo std::vector whitelist_permissions = conn_type == ConnectionType::MANUAL ? vWhitelistedRangeOutgoing : std::vector{}; AddWhitelistPermissionFlags(permission_flags, target_addr, whitelist_permissions); - // Add node - NodeId id = GetNewNodeId(); - uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize(); - if (!addr_bind.IsValid()) { - addr_bind = GetBindAddress(*sock); - } - CNode* pnode = new CNode(id, + const uint64_t nonce{GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(node_id.value()).Finalize()}; + CNode* pnode = new CNode(node_id.value(), std::move(sock), target_addr, CalculateKeyedNetGroup(target_addr), nonce, - addr_bind, + CAddress{me, NODE_NONE}, pszDest ? pszDest : "", conn_type, /*inbound_onion=*/false, @@ -540,7 +504,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo pnode->AddRef(); // We're making a new connection, harvest entropy from the time (and our peer count) - RandAddEvent((uint32_t)id); + RandAddEvent(static_cast(node_id.value())); return pnode; } @@ -1818,7 +1782,6 @@ void CConnman::EventNewConnectionAccepted(std::unique_ptr&& sock, bool CConnman::AddConnection(const std::string& address, ConnectionType conn_type, bool use_v2transport = false) { - AssertLockNotHeld(m_unused_i2p_sessions_mutex); std::optional max_connections; switch (conn_type) { case ConnectionType::INBOUND: @@ -2437,7 +2400,6 @@ void CConnman::DumpAddresses() void CConnman::ProcessAddrFetch() { - AssertLockNotHeld(m_unused_i2p_sessions_mutex); std::string strDest; { LOCK(m_addr_fetches_mutex); @@ -2557,7 +2519,6 @@ bool CConnman::MaybePickPreferredNetwork(std::optional& network) void CConnman::ThreadOpenConnections(const std::vector connect, Span seed_nodes) { - AssertLockNotHeld(m_unused_i2p_sessions_mutex); AssertLockNotHeld(m_reconnections_mutex); FastRandomContext rng; // Connect to specific addresses @@ -2998,7 +2959,6 @@ std::vector CConnman::GetAddedNodeInfo(bool include_connected) co void CConnman::ThreadOpenAddedConnections() { - AssertLockNotHeld(m_unused_i2p_sessions_mutex); AssertLockNotHeld(m_reconnections_mutex); while (true) { @@ -3028,7 +2988,6 @@ void CConnman::ThreadOpenAddedConnections() // if successful, this moves the passed grant to the constructed node void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant&& grant_outbound, const char *pszDest, ConnectionType conn_type, bool use_v2transport) { - AssertLockNotHeld(m_unused_i2p_sessions_mutex); assert(conn_type != ConnectionType::INBOUND); // @@ -3912,7 +3871,6 @@ uint64_t CConnman::CalculateKeyedNetGroup(const CNetAddr& address) const void CConnman::PerformReconnections() { AssertLockNotHeld(m_reconnections_mutex); - AssertLockNotHeld(m_unused_i2p_sessions_mutex); while (true) { // Move first element of m_reconnections to todo (avoiding an allocation inside the lock). decltype(m_reconnections) todo; diff --git a/src/net.h b/src/net.h index 05e35e36a6bb..3c72d0ad9e8c 100644 --- a/src/net.h +++ b/src/net.h @@ -42,7 +42,6 @@ #include #include #include -#include #include #include #include @@ -1117,7 +1116,7 @@ class CConnman : private SockMan bool GetNetworkActive() const { return fNetworkActive; }; bool GetUseAddrmanOutgoing() const { return m_use_addrman_outgoing; }; void SetNetworkActive(bool active); - void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant&& grant_outbound, const char* strDest, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); + void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant&& grant_outbound, const char* strDest, ConnectionType conn_type, bool use_v2transport); bool CheckIncomingNonce(uint64_t nonce); void ASMapHealthCheck(); @@ -1202,7 +1201,7 @@ class CConnman : private SockMan * - Max total outbound connection capacity filled * - Max connection capacity for type is filled */ - bool AddConnection(const std::string& address, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); + bool AddConnection(const std::string& address, ConnectionType conn_type, bool use_v2transport); size_t GetNodeCount(ConnectionDirection) const; std::map getNetLocalAddresses() const; @@ -1261,10 +1260,10 @@ class CConnman : private SockMan bool Bind(const CService& addr, unsigned int flags, NetPermissionFlags permissions); bool InitBinds(const Options& options); - void ThreadOpenAddedConnections() EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex); + void ThreadOpenAddedConnections() EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex, !m_reconnections_mutex); void AddAddrFetch(const std::string& strDest) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex); - void ProcessAddrFetch() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_unused_i2p_sessions_mutex); - void ThreadOpenConnections(std::vector connect, Span seed_nodes) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex); + void ProcessAddrFetch() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex); + void ThreadOpenConnections(std::vector connect, Span seed_nodes) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex, !m_reconnections_mutex); void ThreadMessageHandler() EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc); /// Whether we are currently advertising our I2P address (via `AddLocal()`). @@ -1356,7 +1355,7 @@ class CConnman : private SockMan bool AlreadyConnectedToAddress(const CAddress& addr); bool AttemptToEvictConnection(); - CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); + CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type, bool use_v2transport); void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr, const std::vector& ranges) const; void DeleteNode(CNode* pnode); @@ -1572,20 +1571,6 @@ class CConnman : private SockMan */ bool whitelist_relay; - /** - * Mutex protecting m_i2p_sam_sessions. - */ - Mutex m_unused_i2p_sessions_mutex; - - /** - * A pool of created I2P SAM transient sessions that should be used instead - * of creating new ones in order to reduce the load on the I2P network. - * Creating a session in I2P is not cheap, thus if this is not empty, then - * pick an entry from it instead of creating a new session. If connecting to - * a host fails, then the created session is put to this pool for reuse. - */ - std::queue> m_unused_i2p_sessions GUARDED_BY(m_unused_i2p_sessions_mutex); - /** * Mutex protecting m_reconnections. */ @@ -1607,13 +1592,7 @@ class CConnman : private SockMan std::list m_reconnections GUARDED_BY(m_reconnections_mutex); /** Attempt reconnections, if m_reconnections non-empty. */ - void PerformReconnections() EXCLUSIVE_LOCKS_REQUIRED(!m_reconnections_mutex, !m_unused_i2p_sessions_mutex); - - /** - * Cap on the size of `m_unused_i2p_sessions`, to ensure it does not - * unexpectedly use too much memory. - */ - static constexpr size_t MAX_UNUSED_I2P_SESSIONS_SIZE{10}; + void PerformReconnections() EXCLUSIVE_LOCKS_REQUIRED(!m_reconnections_mutex); /** * RAII helper to atomically create a copy of `m_nodes` and add a reference diff --git a/src/sockman.cpp b/src/sockman.cpp index 721ae5cec2bc..03908cfb1ee6 100644 --- a/src/sockman.cpp +++ b/src/sockman.cpp @@ -10,6 +10,19 @@ #include #include +CService GetBindAddress(const Sock& sock) +{ + CService addr_bind; + struct sockaddr_storage sockaddr_bind; + socklen_t sockaddr_bind_len = sizeof(sockaddr_bind); + 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; +} + bool SockMan::BindAndStartListening(const CService& to, bilingual_str& errmsg) { // Create socket for listening for incoming connections @@ -107,6 +120,83 @@ void SockMan::JoinSocketsThreads() } } +std::optional +SockMan::ConnectAndMakeNodeId(const std::variant& to, + bool is_important, + const Proxy& proxy, + bool& proxy_failed, + CService& me, + std::unique_ptr& sock, + std::unique_ptr& i2p_transient_session) +{ + AssertLockNotHeld(m_unused_i2p_sessions_mutex); + + Assume(!me.IsValid()); + + if (std::holds_alternative(to)) { + const CService& addr_to{std::get(to)}; + if (addr_to.IsI2P()) { + if (!Assume(proxy.IsValid())) { + return std::nullopt; + } + + i2p::Connection conn; + bool connected{false}; + + if (m_i2p_sam_session) { + connected = m_i2p_sam_session->Connect(addr_to, conn, proxy_failed); + } else { + { + LOCK(m_unused_i2p_sessions_mutex); + if (m_unused_i2p_sessions.empty()) { + i2p_transient_session = std::make_unique(proxy, &interruptNet); + } else { + i2p_transient_session.swap(m_unused_i2p_sessions.front()); + m_unused_i2p_sessions.pop(); + } + } + connected = i2p_transient_session->Connect(addr_to, conn, proxy_failed); + if (!connected) { + LOCK(m_unused_i2p_sessions_mutex); + if (m_unused_i2p_sessions.size() < MAX_UNUSED_I2P_SESSIONS_SIZE) { + m_unused_i2p_sessions.emplace(i2p_transient_session.release()); + } + } + } + + if (connected) { + sock = std::move(conn.sock); + me = conn.me; + } + } else if (proxy.IsValid()) { + sock = ConnectThroughProxy(proxy, addr_to.ToStringAddr(), addr_to.GetPort(), proxy_failed); + } else { + sock = ConnectDirectly(addr_to, is_important); + } + } else { + if (!Assume(proxy.IsValid())) { + return std::nullopt; + } + + const auto& hostport{std::get(to)}; + + bool dummy_proxy_failed; + sock = ConnectThroughProxy(proxy, hostport.host, hostport.port, dummy_proxy_failed); + } + + if (!sock) { + return std::nullopt; + } + + if (!me.IsValid()) { + me = GetBindAddress(*sock); + } + + const NodeId node_id{GetNewNodeId()}; + + return node_id; +} + std::unique_ptr SockMan::AcceptConnection(const Sock& listen_sock, CService& addr) { sockaddr_storage storage; diff --git a/src/sockman.h b/src/sockman.h index 90d0150fa3c1..e2f5ac9eaa97 100644 --- a/src/sockman.h +++ b/src/sockman.h @@ -14,11 +14,15 @@ #include #include +#include #include +#include #include typedef int64_t NodeId; +CService GetBindAddress(const Sock& sock); + /** * A socket manager class which handles socket operations. * To use this class, inherit from it and implement the pure virtual methods. @@ -26,6 +30,7 @@ typedef int64_t NodeId; * - binding and listening on sockets * - starting of necessary threads to process socket operations * - accepting incoming connections + * - making outbound connections */ class SockMan { @@ -74,6 +79,37 @@ class SockMan */ void JoinSocketsThreads(); + /** + * A more readable std::tuple for host and port. + */ + struct StringHostIntPort { + const std::string& host; + uint16_t port; + }; + + /** + * Make an outbound connection, save the socket internally and return a newly generated node id. + * @param[in] to The address to connect to, either as CService or a host as string and port as + * an integer, if the later is used, then `proxy` must be valid. + * @param[in] is_important If true, then log failures with higher severity. + * @param[in] proxy Proxy to connect through if `proxy.IsValid()` is true. + * @param[out] proxy_failed If `proxy` is valid and the connection failed because of the + * proxy, then it will be set to true. + * @param[out] me If the connection was successful then this is set to the address on the + * local side of the socket. + * @param[out] sock Connected socket, if the operation is successful. + * @param[out] i2p_transient_session I2P session, if the operation is successful. + * @return Newly generated node id, or std::nullopt if the operation fails. + */ + std::optional ConnectAndMakeNodeId(const std::variant& to, + bool is_important, + const Proxy& proxy, + bool& proxy_failed, + CService& me, + std::unique_ptr& sock, + std::unique_ptr& i2p_transient_session) + EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); + /** * Accept a connection. * @param[in] listen_sock Socket on which to accept the connection. @@ -209,6 +245,12 @@ class SockMan private: + /** + * Cap on the size of `m_unused_i2p_sessions`, to ensure it does not + * unexpectedly use too much memory. + */ + static constexpr size_t MAX_UNUSED_I2P_SESSIONS_SIZE{10}; + /** * Accept incoming I2P connections in a loop and call * `EventNewConnectionAccepted()` for each new connection. @@ -224,6 +266,20 @@ class SockMan * Thread that accepts incoming I2P connections in a loop, can be stopped via `interruptNet`. */ std::thread m_thread_i2p_accept; + + /** + * Mutex protecting m_i2p_sam_sessions. + */ + Mutex m_unused_i2p_sessions_mutex; + + /** + * A pool of created I2P SAM transient sessions that should be used instead + * of creating new ones in order to reduce the load on the I2P network. + * Creating a session in I2P is not cheap, thus if this is not empty, then + * pick an entry from it instead of creating a new session. If connecting to + * a host fails, then the created session is put to this pool for reuse. + */ + std::queue> m_unused_i2p_sessions GUARDED_BY(m_unused_i2p_sessions_mutex); }; #endif // BITCOIN_SOCKMAN_H diff --git a/src/test/util/net.h b/src/test/util/net.h index 10a11b09019d..f58e75b3c072 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -87,8 +87,7 @@ struct ConnmanTestMsg : public CConnman { bool AlreadyConnectedPublic(const CAddress& addr) { return AlreadyConnectedToAddress(addr); }; - CNode* ConnectNodePublic(PeerManager& peerman, const char* pszDest, ConnectionType conn_type) - EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); + CNode* ConnectNodePublic(PeerManager& peerman, const char* pszDest, ConnectionType conn_type); }; constexpr ServiceFlags ALL_SERVICE_FLAGS[]{ From ef92e0e750e952f4bee1215414182f39ba35d873 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 24 Sep 2024 09:41:47 +0200 Subject: [PATCH 14/16] net: tweak EventNewConnectionAccepted() Move `MaybeFlipIPv6toCJDNS()`, which is Bitcoin P2P specific from the callers of `CConnman::EventNewConnectionAccepted()` to inside that method. Move the IsSelectable check, the `TCP_NODELAY` option set and the generation of new node id out of `CConnman::EventNewConnectionAccepted()` because those are protocol agnostic. Move those to a new method `SockMan::NewSockAccepted()` which is called instead of `CConnman::EventNewConnectionAccepted()`. --- src/net.cpp | 37 ++++++++++++------------------------- src/net.h | 4 +++- src/sockman.cpp | 22 +++++++++++++++++++++- src/sockman.h | 13 ++++++++++++- 4 files changed, 48 insertions(+), 28 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 9e426a9162b3..3ac35b432efb 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1680,10 +1680,14 @@ bool CConnman::AttemptToEvictConnection() return false; } -void CConnman::EventNewConnectionAccepted(std::unique_ptr&& sock, - const CService& addr_bind, - const CService& addr) +void CConnman::EventNewConnectionAccepted(NodeId node_id, + std::unique_ptr&& sock, + const CService& addr_bind_, + const CService& addr_) { + const CService addr_bind{MaybeFlipIPv6toCJDNS(addr_bind_)}; + const CService addr{MaybeFlipIPv6toCJDNS(addr_)}; + int nInbound = 0; NetPermissionFlags permission_flags = NetPermissionFlags::None; @@ -1706,19 +1710,6 @@ void CConnman::EventNewConnectionAccepted(std::unique_ptr&& sock, return; } - if (!sock->IsSelectable()) { - LogPrintf("connection from %s dropped: non-selectable socket\n", addr.ToStringAddrPort()); - return; - } - - // According to the internet TCP_NODELAY is not carried into accepted sockets - // on all platforms. Set it again here just to be sure. - const int on{1}; - if (sock->SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) == SOCKET_ERROR) { - LogDebug(BCLog::NET, "connection from %s: unable to set TCP_NODELAY, continuing anyway\n", - addr.ToStringAddrPort()); - } - // Don't accept connections from banned peers. bool banned = m_banman && m_banman->IsBanned(addr); if (!NetPermissions::HasFlag(permission_flags, NetPermissionFlags::NoBan) && banned) @@ -1744,8 +1735,7 @@ void CConnman::EventNewConnectionAccepted(std::unique_ptr&& sock, } } - NodeId id = GetNewNodeId(); - uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize(); + const uint64_t nonce{GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(node_id).Finalize()}; const bool inbound_onion = std::find(m_onion_binds.begin(), m_onion_binds.end(), addr_bind) != m_onion_binds.end(); // The V2Transport transparently falls back to V1 behavior when an incoming V1 connection is @@ -1753,7 +1743,7 @@ void CConnman::EventNewConnectionAccepted(std::unique_ptr&& sock, ServiceFlags local_services = GetLocalServices(); const bool use_v2transport(local_services & NODE_P2P_V2); - CNode* pnode = new CNode(id, + CNode* pnode = new CNode(node_id, std::move(sock), CAddress{addr, NODE_NONE}, CalculateKeyedNetGroup(addr), @@ -1772,12 +1762,12 @@ void CConnman::EventNewConnectionAccepted(std::unique_ptr&& sock, m_msgproc->InitializeNode(*pnode, local_services); { LOCK(m_nodes_mutex); - m_nodes.emplace(id, pnode); + m_nodes.emplace(node_id, pnode); } LogDebug(BCLog::NET, "connection from %s accepted\n", addr.ToStringAddrPort()); // We received a new connection, harvest entropy from the time (and our peer count) - RandAddEvent((uint32_t)id); + RandAddEvent(static_cast(node_id)); } bool CConnman::AddConnection(const std::string& address, ConnectionType conn_type, bool use_v2transport = false) @@ -2219,10 +2209,7 @@ void CConnman::SocketHandlerListening(const Sock::EventsPerSock& events_per_sock auto sock_accepted{AcceptConnection(*sock, addr_accepted)}; if (sock_accepted) { - addr_accepted = MaybeFlipIPv6toCJDNS(addr_accepted); - const CService addr_bind{MaybeFlipIPv6toCJDNS(GetBindAddress(*sock))}; - - EventNewConnectionAccepted(std::move(sock_accepted), addr_bind, addr_accepted); + NewSockAccepted(std::move(sock_accepted), GetBindAddress(*sock), addr_accepted); } } } diff --git a/src/net.h b/src/net.h index 3c72d0ad9e8c..34c2a478d5bf 100644 --- a/src/net.h +++ b/src/net.h @@ -1273,11 +1273,13 @@ class CConnman : private SockMan /** * Create a `CNode` object and add it to the `m_nodes` member. + * @param[in] node_id Id of the newly accepted connection. * @param[in] sock Connected socket to communicate with the peer. * @param[in] me The address and port at our side of the connection. * @param[in] them The address and port at the peer's side of the connection. */ - virtual void EventNewConnectionAccepted(std::unique_ptr&& sock, + virtual void EventNewConnectionAccepted(NodeId node_id, + std::unique_ptr&& sock, const CService& me, const CService& them) override; diff --git a/src/sockman.cpp b/src/sockman.cpp index 03908cfb1ee6..644a27187f7c 100644 --- a/src/sockman.cpp +++ b/src/sockman.cpp @@ -222,6 +222,26 @@ std::unique_ptr SockMan::AcceptConnection(const Sock& listen_sock, CServic return sock; } +void SockMan::NewSockAccepted(std::unique_ptr&& sock, const CService& me, const CService& them) +{ + if (!sock->IsSelectable()) { + LogPrintf("connection from %s dropped: non-selectable socket\n", them.ToStringAddrPort()); + return; + } + + // According to the internet TCP_NODELAY is not carried into accepted sockets + // on all platforms. Set it again here just to be sure. + const int on{1}; + if (sock->SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) == SOCKET_ERROR) { + LogDebug(BCLog::NET, "connection from %s: unable to set TCP_NODELAY, continuing anyway\n", + them.ToStringAddrPort()); + } + + const NodeId node_id{GetNewNodeId()}; + + EventNewConnectionAccepted(node_id, std::move(sock), me, them); +} + NodeId SockMan::GetNewNodeId() { return m_next_node_id.fetch_add(1, std::memory_order_relaxed); @@ -272,7 +292,7 @@ void SockMan::ThreadI2PAccept() continue; } - EventNewConnectionAccepted(std::move(conn.sock), conn.me, conn.peer); + NewSockAccepted(std::move(conn.sock), conn.me, conn.peer); err_wait = err_wait_begin; } diff --git a/src/sockman.h b/src/sockman.h index e2f5ac9eaa97..fa8de15b38bf 100644 --- a/src/sockman.h +++ b/src/sockman.h @@ -118,6 +118,15 @@ class SockMan */ std::unique_ptr AcceptConnection(const Sock& listen_sock, CService& addr); + /** + * After a new socket with a peer has been created, configure its flags, + * make a new node id and call `EventNewConnectionAccepted()`. + * @param[in] sock The newly created socket. + * @param[in] me Address at our end of the connection. + * @param[in] them Address of the new peer. + */ + void NewSockAccepted(std::unique_ptr&& sock, const CService& me, const CService& them); + /** * Generate an id for a newly created node. */ @@ -134,11 +143,13 @@ class SockMan /** * Be notified when a new connection has been accepted. + * @param[in] node_id Id of the newly accepted connection. * @param[in] sock Connected socket to communicate with the peer. * @param[in] me The address and port at our side of the connection. * @param[in] them The address and port at the peer's side of the connection. */ - virtual void EventNewConnectionAccepted(std::unique_ptr&& sock, + virtual void EventNewConnectionAccepted(NodeId node_id, + std::unique_ptr&& sock, const CService& me, const CService& them) = 0; From a102f5a9377fbdcc89f8abdc5d52ec86d5d450a8 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 23 Sep 2024 11:03:32 +0200 Subject: [PATCH 15/16] net: move sockets from CNode to SockMan Move `CNode::m_sock` and `CNode::m_i2p_sam_session` to `SockMan::m_connected`. Also move all the code that handles sockets to `SockMan`. `CNode::CloseSocketDisconnect()` becomes `CConnman::MarkAsDisconnectAndCloseConnection()`. `CConnman::SocketSendData()` is renamed to `CConnman::SendMessagesAsBytes()` and its sockets-touching bits are moved to `SockMan::SendBytes()`. `CConnman::GenerateWaitSockets()` goes to `SockMan::GenerateWaitSockets()`. `CConnman::ThreadSocketHandler()` and `CConnman::SocketHandler()` are combined into `SockMan::ThreadSocketHandler()`. `CConnman::SocketHandlerConnected()` goes to `SockMan::SocketHandlerConnected()`. `CConnman::SocketHandlerListening()` goes to `SockMan::SocketHandlerListening()`. --- src/net.cpp | 280 ++++--------------------- src/net.h | 73 +------ src/sockman.cpp | 239 ++++++++++++++++++++- src/sockman.h | 156 ++++++++++++-- src/test/denialofservice_tests.cpp | 6 - src/test/fuzz/connman.cpp | 5 +- src/test/fuzz/net.cpp | 3 - src/test/fuzz/p2p_handshake.cpp | 2 +- src/test/fuzz/p2p_headers_presync.cpp | 2 +- src/test/fuzz/process_message.cpp | 2 +- src/test/fuzz/process_messages.cpp | 2 +- src/test/fuzz/util/net.h | 3 - src/test/net_peer_connection_tests.cpp | 1 - src/test/net_tests.cpp | 9 - src/test/util/net.h | 6 + 15 files changed, 442 insertions(+), 347 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 3ac35b432efb..45ae9b2050f1 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -100,10 +100,6 @@ enum BindFlags { BF_DONT_ADVERTISE = (1U << 1), }; -// The set of sockets cannot be modified while waiting -// The sleep time needs to be small to avoid new sockets stalling -static const uint64_t SELECT_TIMEOUT_MILLISECONDS = 50; - const std::string NET_MESSAGE_TYPE_OTHER = "*other*"; static const uint64_t RANDOMIZER_ID_NETGROUP = 0x6c0edd8036ef4036ULL; // SHA256("netgroup")[0:8] @@ -432,10 +428,8 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo } // Connect - std::unique_ptr sock; Proxy proxy; assert(!proxy.IsValid()); - std::unique_ptr i2p_transient_session; std::optional node_id; CService me; @@ -453,9 +447,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo /*is_important=*/conn_type == ConnectionType::MANUAL, proxy, proxyConnectionFailed, - me, - sock, - i2p_transient_session); + me); if (!proxyConnectionFailed) { // If a connection to the node was attempted, and failure (if any) is not caused by a problem connecting to @@ -472,9 +464,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo /*is_important=*/conn_type == ConnectionType::MANUAL, proxy, dummy, - me, - sock, - i2p_transient_session); + me); } // Check any other resolved address (if any) if we fail to connect if (!node_id.has_value()) { @@ -487,7 +477,6 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo const uint64_t nonce{GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(node_id.value()).Finalize()}; CNode* pnode = new CNode(node_id.value(), - std::move(sock), target_addr, CalculateKeyedNetGroup(target_addr), nonce, @@ -497,7 +486,6 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo /*inbound_onion=*/false, CNodeOptions{ .permission_flags = permission_flags, - .i2p_sam_session = std::move(i2p_transient_session), .recv_flood_size = nReceiveFloodSize, .use_v2transport = use_v2transport, }); @@ -512,17 +500,6 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo return nullptr; } -void CNode::CloseSocketDisconnect() -{ - fDisconnect = true; - LOCK(m_sock_mutex); - if (m_sock) { - LogDebug(BCLog::NET, "disconnecting peer=%d\n", id); - m_sock.reset(); - } - m_i2p_sam_session.reset(); -} - void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr, const std::vector& ranges) const { for (const auto& subnet : ranges) { if (subnet.m_subnet.Match(addr)) { @@ -1532,7 +1509,7 @@ Transport::Info V2Transport::GetInfo() const noexcept return info; } -std::pair CConnman::SocketSendData(CNode& node) +std::pair CConnman::SendMessagesAsBytes(CNode& node) { AssertLockNotHeld(m_total_bytes_sent_mutex); @@ -1560,45 +1537,29 @@ std::pair CConnman::SocketSendData(CNode& node) if (expected_more.has_value()) Assume(!data.empty() == *expected_more); expected_more = more; data_left = !data.empty(); // will be overwritten on next loop if all of data gets sent - int nBytes = 0; - if (!data.empty()) { - LOCK(node.m_sock_mutex); - // There is no socket in case we've already disconnected, or in test cases without - // real connections. In these cases, we bail out immediately and just leave things - // in the send queue and transport. - if (!node.m_sock) { - break; - } - int flags = MSG_NOSIGNAL | MSG_DONTWAIT; -#ifdef MSG_MORE - if (more) { - flags |= MSG_MORE; - } -#endif - nBytes = node.m_sock->Send(reinterpret_cast(data.data()), data.size(), flags); - } - if (nBytes > 0) { + + std::string errmsg; + + const ssize_t sent{SendBytes(node.GetId(), data, more, errmsg)}; + + if (sent > 0) { node.m_last_send = GetTime(); - node.nSendBytes += nBytes; + node.nSendBytes += sent; // Notify transport that bytes have been processed. - node.m_transport->MarkBytesSent(nBytes); + node.m_transport->MarkBytesSent(sent); // Update statistics per message type. if (!msg_type.empty()) { // don't report v2 handshake bytes for now - node.AccountForSentBytes(msg_type, nBytes); + node.AccountForSentBytes(msg_type, sent); } - nSentSize += nBytes; - if ((size_t)nBytes != data.size()) { + nSentSize += sent; + if (static_cast(sent) != data.size()) { // could not send full message; stop sending more break; } } else { - if (nBytes < 0) { - // error - int nErr = WSAGetLastError(); - if (nErr != WSAEWOULDBLOCK && nErr != WSAEMSGSIZE && nErr != WSAEINTR && nErr != WSAEINPROGRESS) { - LogDebug(BCLog::NET, "socket send error for peer=%d: %s\n", node.GetId(), NetworkErrorString(nErr)); - node.CloseSocketDisconnect(); - } + if (sent < 0) { + LogDebug(BCLog::NET, "socket send error for peer=%d: %s\n", node.GetId(), errmsg); + MarkAsDisconnectAndCloseConnection(node); } break; } @@ -1680,8 +1641,7 @@ bool CConnman::AttemptToEvictConnection() return false; } -void CConnman::EventNewConnectionAccepted(NodeId node_id, - std::unique_ptr&& sock, +bool CConnman::EventNewConnectionAccepted(NodeId node_id, const CService& addr_bind_, const CService& addr_) { @@ -1707,7 +1667,7 @@ void CConnman::EventNewConnectionAccepted(NodeId node_id, if (!fNetworkActive) { LogDebug(BCLog::NET, "connection from %s dropped: not accepting new connections\n", addr.ToStringAddrPort()); - return; + return false; } // Don't accept connections from banned peers. @@ -1715,7 +1675,7 @@ void CConnman::EventNewConnectionAccepted(NodeId node_id, if (!NetPermissions::HasFlag(permission_flags, NetPermissionFlags::NoBan) && banned) { LogDebug(BCLog::NET, "connection from %s dropped (banned)\n", addr.ToStringAddrPort()); - return; + return false; } // Only accept connections from discouraged peers if our inbound slots aren't (almost) full. @@ -1723,7 +1683,7 @@ void CConnman::EventNewConnectionAccepted(NodeId node_id, if (!NetPermissions::HasFlag(permission_flags, NetPermissionFlags::NoBan) && nInbound + 1 >= m_max_inbound && discouraged) { LogDebug(BCLog::NET, "connection from %s dropped (discouraged)\n", addr.ToStringAddrPort()); - return; + return false; } if (nInbound >= m_max_inbound) @@ -1731,7 +1691,7 @@ void CConnman::EventNewConnectionAccepted(NodeId node_id, if (!AttemptToEvictConnection()) { // No connection to evict, disconnect the new connection LogDebug(BCLog::NET, "failed to find an eviction candidate - connection dropped (full)\n"); - return; + return false; } } @@ -1744,7 +1704,6 @@ void CConnman::EventNewConnectionAccepted(NodeId node_id, const bool use_v2transport(local_services & NODE_P2P_V2); CNode* pnode = new CNode(node_id, - std::move(sock), CAddress{addr, NODE_NONE}, CalculateKeyedNetGroup(addr), nonce, @@ -1768,6 +1727,8 @@ void CConnman::EventNewConnectionAccepted(NodeId node_id, // We received a new connection, harvest entropy from the time (and our peer count) RandAddEvent(static_cast(node_id)); + + return true; } bool CConnman::AddConnection(const std::string& address, ConnectionType conn_type, bool use_v2transport = false) @@ -1809,6 +1770,14 @@ bool CConnman::AddConnection(const std::string& address, ConnectionType conn_typ return true; } +void CConnman::MarkAsDisconnectAndCloseConnection(CNode& node) +{ + node.fDisconnect = true; + if (CloseConnection(node.GetId())) { + LogDebug(BCLog::NET, "disconnecting peer=%d\n", node.GetId()); + } +} + void CConnman::DisconnectNodes() { AssertLockNotHeld(m_nodes_mutex); @@ -1859,8 +1828,7 @@ void CConnman::DisconnectNodes() // release outbound grant (if any) pnode->grantOutbound.Release(); - // close socket and cleanup - pnode->CloseSocketDisconnect(); + MarkAsDisconnectAndCloseConnection(*pnode); // update connection count by network if (pnode->IsManualOrFullOutboundConn()) --m_network_conn_counts[pnode->addr.GetNetwork()]; @@ -1956,7 +1924,7 @@ void CConnman::EventReadyToSend(NodeId node_id, bool& cancel_recv) return; } - const auto [bytes_sent, data_left] = WITH_LOCK(node->cs_vSend, return SocketSendData(*node);); + const auto [bytes_sent, data_left] = WITH_LOCK(node->cs_vSend, return SendMessagesAsBytes(*node);); // If both receiving and (non-optimistic) sending were possible, we first attempt // sending. If that succeeds, but does not fully drain the send queue, do not @@ -1980,7 +1948,7 @@ void CConnman::EventGotData(NodeId node_id, const uint8_t* data, size_t n) bool notify = false; if (!node->ReceiveMsgBytes({data, n}, notify)) { - node->CloseSocketDisconnect(); + MarkAsDisconnectAndCloseConnection(*node); } RecordBytesRecv(n); if (notify) { @@ -2001,7 +1969,7 @@ void CConnman::EventGotEOF(NodeId node_id) if (!node->fDisconnect) { LogDebug(BCLog::NET, "socket closed for peer=%d\n", node_id); } - node->CloseSocketDisconnect(); + MarkAsDisconnectAndCloseConnection(*node); } void CConnman::EventGotPermanentReadError(NodeId node_id, const std::string& errmsg) @@ -2016,7 +1984,7 @@ void CConnman::EventGotPermanentReadError(NodeId node_id, const std::string& err if (!node->fDisconnect) { LogDebug(BCLog::NET, "socket recv error for peer=%d: %s\n", node_id, errmsg); } - node->CloseSocketDisconnect(); + MarkAsDisconnectAndCloseConnection(*node); } bool CConnman::ShouldTryToSend(NodeId node_id) const @@ -2068,164 +2036,6 @@ void CConnman::EventIOLoopCompletedForAllPeers() DisconnectNodes(); NotifyNumConnectionsChanged(); } - -Sock::EventsPerSock CConnman::GenerateWaitSockets(Span nodes) -{ - AssertLockNotHeld(m_nodes_mutex); - - Sock::EventsPerSock events_per_sock; - - for (const auto& sock : m_listen) { - events_per_sock.emplace(sock, Sock::Events{Sock::RECV}); - } - - for (CNode* pnode : nodes) { - const bool select_recv{ShouldTryToRecv(pnode->GetId())}; - const bool select_send{ShouldTryToSend(pnode->GetId())}; - if (!select_recv && !select_send) continue; - - LOCK(pnode->m_sock_mutex); - if (pnode->m_sock) { - Sock::Event event = (select_send ? Sock::SEND : 0) | (select_recv ? Sock::RECV : 0); - events_per_sock.emplace(pnode->m_sock, Sock::Events{event}); - } - } - - return events_per_sock; -} - -void CConnman::SocketHandler() -{ - AssertLockNotHeld(m_total_bytes_sent_mutex); - - Sock::EventsPerSock events_per_sock; - - { - const NodesSnapshot snap{*this, /*shuffle=*/false}; - - const auto timeout = std::chrono::milliseconds(SELECT_TIMEOUT_MILLISECONDS); - - // Check for the readiness of the already connected sockets and the - // listening sockets in one call ("readiness" as in poll(2) or - // select(2)). If none are ready, wait for a short while and return - // empty sets. - events_per_sock = GenerateWaitSockets(snap.Nodes()); - if (events_per_sock.empty() || !events_per_sock.begin()->first->WaitMany(timeout, events_per_sock)) { - interruptNet.sleep_for(timeout); - } - - // Service (send/receive) each of the already connected nodes. - SocketHandlerConnected(snap.Nodes(), events_per_sock); - } - - // Accept new connections from listening sockets. - SocketHandlerListening(events_per_sock); -} - -void CConnman::SocketHandlerConnected(const std::vector& nodes, - const Sock::EventsPerSock& events_per_sock) -{ - AssertLockNotHeld(m_nodes_mutex); - AssertLockNotHeld(m_total_bytes_sent_mutex); - - for (CNode* pnode : nodes) { - if (interruptNet) - return; - - // - // Receive - // - bool recvSet = false; - bool sendSet = false; - bool errorSet = false; - { - LOCK(pnode->m_sock_mutex); - if (!pnode->m_sock) { - continue; - } - const auto it = events_per_sock.find(pnode->m_sock); - if (it != events_per_sock.end()) { - recvSet = it->second.occurred & Sock::RECV; - sendSet = it->second.occurred & Sock::SEND; - errorSet = it->second.occurred & Sock::ERR; - } - } - - if (sendSet) { - bool cancel_recv; - - EventReadyToSend(pnode->GetId(), cancel_recv); - - if (cancel_recv) { - recvSet = false; - } - } - - if (recvSet || errorSet) - { - // typical socket buffer is 8K-64K - uint8_t pchBuf[0x10000]; - int nBytes = 0; - { - LOCK(pnode->m_sock_mutex); - if (!pnode->m_sock) { - continue; - } - nBytes = pnode->m_sock->Recv(pchBuf, sizeof(pchBuf), MSG_DONTWAIT); - } - if (nBytes > 0) - { - EventGotData(pnode->GetId(), pchBuf, nBytes); - } - else if (nBytes == 0) - { - EventGotEOF(pnode->GetId()); - } - else if (nBytes < 0) - { - // error - int nErr = WSAGetLastError(); - if (nErr != WSAEWOULDBLOCK && nErr != WSAEMSGSIZE && nErr != WSAEINTR && nErr != WSAEINPROGRESS) - { - EventGotPermanentReadError(pnode->GetId(), NetworkErrorString(nErr)); - } - } - } - - EventIOLoopCompletedForNode(pnode->GetId()); - } -} - -void CConnman::SocketHandlerListening(const Sock::EventsPerSock& events_per_sock) -{ - for (const auto& sock : m_listen) { - if (interruptNet) { - return; - } - const auto it = events_per_sock.find(sock); - if (it != events_per_sock.end() && it->second.occurred & Sock::RECV) { - CService addr_accepted; - - auto sock_accepted{AcceptConnection(*sock, addr_accepted)}; - - if (sock_accepted) { - NewSockAccepted(std::move(sock_accepted), GetBindAddress(*sock), addr_accepted); - } - } - } -} - -void CConnman::ThreadSocketHandler() -{ - AssertLockNotHeld(m_nodes_mutex); - AssertLockNotHeld(m_total_bytes_sent_mutex); - - while (!interruptNet) - { - EventIOLoopCompletedForAllPeers(); - SocketHandler(); - } -} void CConnman::WakeMessageHandler() { @@ -3269,9 +3079,6 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) fMsgProcWake = false; } - // Send and receive from sockets, accept connections - threadSocketHandler = std::thread(&util::TraceThread, "net", [this] { ThreadSocketHandler(); }); - SockMan::Options sockman_options; Proxy i2p_sam; @@ -3369,8 +3176,6 @@ void CConnman::StopThreads() threadOpenAddedConnections.join(); if (threadDNSAddressSeed.joinable()) threadDNSAddressSeed.join(); - if (threadSocketHandler.joinable()) - threadSocketHandler.join(); } void CConnman::StopNodes() @@ -3393,7 +3198,7 @@ void CConnman::StopNodes() decltype(m_nodes) nodes; WITH_LOCK(m_nodes_mutex, nodes.swap(m_nodes)); for (auto& [id, pnode] : nodes) { - pnode->CloseSocketDisconnect(); + MarkAsDisconnectAndCloseConnection(*pnode); DeleteNode(pnode); } @@ -3711,7 +3516,6 @@ static std::unique_ptr MakeTransport(NodeId id, bool use_v2transport, } CNode::CNode(NodeId idIn, - std::shared_ptr sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, @@ -3722,7 +3526,6 @@ CNode::CNode(NodeId idIn, CNodeOptions&& node_opts) : m_transport{MakeTransport(idIn, node_opts.use_v2transport, conn_type_in == ConnectionType::INBOUND)}, m_permission_flags{node_opts.permission_flags}, - m_sock{sock}, m_connected{GetTime()}, addr{addrIn}, addrBind{addrBindIn}, @@ -3734,8 +3537,7 @@ CNode::CNode(NodeId idIn, m_conn_type{conn_type_in}, id{idIn}, nLocalHostNonce{nLocalHostNonceIn}, - m_recv_flood_size{node_opts.recv_flood_size}, - m_i2p_sam_session{std::move(node_opts.i2p_sam_session)} + m_recv_flood_size{node_opts.recv_flood_size} { if (inbound_onion) assert(conn_type_in == ConnectionType::INBOUND); @@ -3821,13 +3623,13 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg) // If there was nothing to send before, and there is now (predicted by the "more" value // returned by the GetBytesToSend call above), attempt "optimistic write": - // because the poll/select loop may pause for SELECT_TIMEOUT_MILLISECONDS before actually + // because the poll/select loop may pause for a while before actually // doing a send, try sending from the calling thread if the queue was empty before. // With a V1Transport, more will always be true here, because adding a message always // results in sendable bytes there, but with V2Transport this is not the case (it may // still be in the handshake). if (queue_was_empty && more) { - SocketSendData(*pnode); + SendMessagesAsBytes(*pnode); } } } diff --git a/src/net.h b/src/net.h index 34c2a478d5bf..30ce7c12ba7f 100644 --- a/src/net.h +++ b/src/net.h @@ -658,7 +658,6 @@ class V2Transport final : public Transport struct CNodeOptions { NetPermissionFlags permission_flags = NetPermissionFlags::None; - std::unique_ptr i2p_sam_session = nullptr; bool prefer_evict = false; size_t recv_flood_size{DEFAULT_MAXRECEIVEBUFFER * 1000}; bool use_v2transport = false; @@ -674,16 +673,6 @@ class CNode const NetPermissionFlags m_permission_flags; - /** - * Socket used for communication with the node. - * May not own a Sock object (after `CloseSocketDisconnect()` or during tests). - * `shared_ptr` (instead of `unique_ptr`) is used to avoid premature close of - * the underlying file descriptor by one thread while another thread is - * poll(2)-ing it for activity. - * @see https://github.com/bitcoin/bitcoin/issues/21744 for details. - */ - std::shared_ptr m_sock GUARDED_BY(m_sock_mutex); - /** Sum of GetMemoryUsage of all vSendMsg entries. */ size_t m_send_memusage GUARDED_BY(cs_vSend){0}; /** Total number of bytes sent on the wire to this peer. */ @@ -875,7 +864,6 @@ class CNode std::atomic m_min_ping_time{std::chrono::microseconds::max()}; CNode(NodeId id, - std::shared_ptr sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, @@ -937,8 +925,6 @@ class CNode nRefCount--; } - void CloseSocketDisconnect() EXCLUSIVE_LOCKS_REQUIRED(!m_sock_mutex); - void CopyStats(CNodeStats& stats) EXCLUSIVE_LOCKS_REQUIRED(!m_subver_mutex, !m_addr_local_mutex, !cs_vSend, !cs_vRecv); std::string ConnectionTypeAsString() const { return ::ConnectionTypeAsString(m_conn_type); } @@ -967,18 +953,6 @@ class CNode mapMsgTypeSize mapSendBytesPerMsgType GUARDED_BY(cs_vSend); mapMsgTypeSize mapRecvBytesPerMsgType GUARDED_BY(cs_vRecv); - - /** - * If an I2P session is created per connection (for outbound transient I2P - * connections) then it is stored here so that it can be destroyed when the - * socket is closed. I2P sessions involve a data/transport socket (in `m_sock`) - * and a control socket (in `m_i2p_sam_session`). For transient sessions, once - * the data socket is closed, the control socket is not going to be used anymore - * and is just taking up resources. So better close it as soon as `m_sock` is - * closed. - * Otherwise this unique_ptr is empty. - */ - std::unique_ptr m_i2p_sam_session GUARDED_BY(m_sock_mutex); }; /** @@ -1274,15 +1248,21 @@ class CConnman : private SockMan /** * Create a `CNode` object and add it to the `m_nodes` member. * @param[in] node_id Id of the newly accepted connection. - * @param[in] sock Connected socket to communicate with the peer. * @param[in] me The address and port at our side of the connection. * @param[in] them The address and port at the peer's side of the connection. + * @retval true on success + * @retval false on failure, meaning that the associated socket and node_id should be discarded */ - virtual void EventNewConnectionAccepted(NodeId node_id, - std::unique_ptr&& sock, + virtual bool EventNewConnectionAccepted(NodeId node_id, const CService& me, const CService& them) override; + /** + * Mark a node as disconnected and close its connection with the peer. + * @param[in] node Node to disconnect. + */ + void MarkAsDisconnectAndCloseConnection(CNode& node); + void DisconnectNodes() EXCLUSIVE_LOCKS_REQUIRED(!m_reconnections_mutex, !m_nodes_mutex); void NotifyNumConnectionsChanged(); /** Return true if the peer is inactive and should be disconnected. */ @@ -1311,37 +1291,7 @@ class CConnman : private SockMan virtual void EventIOLoopCompletedForAllPeers() override EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_reconnections_mutex); - - /** - * Generate a collection of sockets to check for IO readiness. - * @param[in] nodes Select from these nodes' sockets. - * @return sockets to check for readiness - */ - Sock::EventsPerSock GenerateWaitSockets(Span nodes) - EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex); - - /** - * Check connected and listening sockets for IO readiness and process them accordingly. - */ - void SocketHandler() - EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_total_bytes_sent_mutex, !mutexMsgProc); - - /** - * Do the read/write for connected sockets that are ready for IO. - * @param[in] nodes Nodes to process. The socket of each node is checked against `what`. - * @param[in] events_per_sock Sockets that are ready for IO. - */ - void SocketHandlerConnected(const std::vector& nodes, - const Sock::EventsPerSock& events_per_sock) - EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex, !m_total_bytes_sent_mutex, !mutexMsgProc); - - /** - * Accept incoming connections, one from each read-ready listening socket. - * @param[in] events_per_sock Sockets that are ready for IO. - */ - void SocketHandlerListening(const Sock::EventsPerSock& events_per_sock); - void ThreadSocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc, !m_nodes_mutex, !m_reconnections_mutex); void ThreadDNSAddressSeed() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_nodes_mutex); uint64_t CalculateKeyedNetGroup(const CNetAddr& ad) const; @@ -1363,8 +1313,8 @@ class CConnman : private SockMan void DeleteNode(CNode* pnode); /** (Try to) send data from node's vSendMsg. Returns (bytes_sent, data_left). */ - std::pair SocketSendData(CNode& node) - EXCLUSIVE_LOCKS_REQUIRED(node.cs_vSend, !m_total_bytes_sent_mutex); + std::pair SendMessagesAsBytes(CNode& node) EXCLUSIVE_LOCKS_REQUIRED(node.cs_vSend) + EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex); void DumpAddresses(); @@ -1539,7 +1489,6 @@ class CConnman : private SockMan std::atomic flagInterruptMsgProc{false}; std::thread threadDNSAddressSeed; - std::thread threadSocketHandler; std::thread threadOpenAddedConnections; std::thread threadOpenConnections; std::thread threadMessageHandler; diff --git a/src/sockman.cpp b/src/sockman.cpp index 644a27187f7c..7cb517079e81 100644 --- a/src/sockman.cpp +++ b/src/sockman.cpp @@ -10,7 +10,11 @@ #include #include -CService GetBindAddress(const Sock& sock) +// The set of sockets cannot be modified while waiting +// The sleep time needs to be small to avoid new sockets stalling +static constexpr auto SELECT_TIMEOUT{50ms}; + +static CService GetBindAddress(const Sock& sock) { CService addr_bind; struct sockaddr_storage sockaddr_bind; @@ -104,6 +108,8 @@ bool SockMan::BindAndStartListening(const CService& to, bilingual_str& errmsg) void SockMan::StartSocketsThreads(const Options& options) { + m_thread_socket_handler = std::thread(&util::TraceThread, "net", [this] { ThreadSocketHandler(); }); + if (options.i2p.has_value()) { m_i2p_sam_session = std::make_unique( options.i2p->private_key_file, options.i2p->sam_proxy, &interruptNet); @@ -118,6 +124,10 @@ void SockMan::JoinSocketsThreads() if (m_thread_i2p_accept.joinable()) { m_thread_i2p_accept.join(); } + + if (m_thread_socket_handler.joinable()) { + m_thread_socket_handler.join(); + } } std::optional @@ -125,12 +135,14 @@ SockMan::ConnectAndMakeNodeId(const std::variant& t bool is_important, const Proxy& proxy, bool& proxy_failed, - CService& me, - std::unique_ptr& sock, - std::unique_ptr& i2p_transient_session) + CService& me) { + AssertLockNotHeld(m_connected_mutex); AssertLockNotHeld(m_unused_i2p_sessions_mutex); + std::unique_ptr sock; + std::unique_ptr i2p_transient_session; + Assume(!me.IsValid()); if (std::holds_alternative(to)) { @@ -194,6 +206,12 @@ SockMan::ConnectAndMakeNodeId(const std::variant& t const NodeId node_id{GetNewNodeId()}; + { + LOCK(m_connected_mutex); + m_connected.emplace(node_id, std::make_shared(std::move(sock), + std::move(i2p_transient_session))); + } + return node_id; } @@ -224,6 +242,8 @@ std::unique_ptr SockMan::AcceptConnection(const Sock& listen_sock, CServic void SockMan::NewSockAccepted(std::unique_ptr&& sock, const CService& me, const CService& them) { + AssertLockNotHeld(m_connected_mutex); + if (!sock->IsSelectable()) { LogPrintf("connection from %s dropped: non-selectable socket\n", them.ToStringAddrPort()); return; @@ -239,7 +259,14 @@ void SockMan::NewSockAccepted(std::unique_ptr&& sock, const CService& me, const NodeId node_id{GetNewNodeId()}; - EventNewConnectionAccepted(node_id, std::move(sock), me, them); + { + LOCK(m_connected_mutex); + m_connected.emplace(node_id, std::make_shared(std::move(sock))); + } + + if (!EventNewConnectionAccepted(node_id, me, them)) { + CloseConnection(node_id); + } } NodeId SockMan::GetNewNodeId() @@ -247,6 +274,52 @@ NodeId SockMan::GetNewNodeId() return m_next_node_id.fetch_add(1, std::memory_order_relaxed); } +bool SockMan::CloseConnection(NodeId node_id) +{ + LOCK(m_connected_mutex); + return m_connected.erase(node_id) > 0; +} + +ssize_t SockMan::SendBytes(NodeId node_id, + Span data, + bool will_send_more, + std::string& errmsg) const +{ + AssertLockNotHeld(m_connected_mutex); + + if (data.empty()) { + return 0; + } + + auto node_sockets{GetNodeSockets(node_id)}; + if (!node_sockets) { + // Bail out immediately and just leave things in the caller's send queue. + return 0; + } + + int flags{MSG_NOSIGNAL | MSG_DONTWAIT}; +#ifdef MSG_MORE + if (will_send_more) { + flags |= MSG_MORE; + } +#endif + + const ssize_t sent{WITH_LOCK( + node_sockets->mutex, + return node_sockets->sock->Send(reinterpret_cast(data.data()), data.size(), flags);)}; + + if (sent >= 0) { + return sent; + } + + const int err{WSAGetLastError()}; + if (err == WSAEWOULDBLOCK || err == WSAEMSGSIZE || err == WSAEINTR || err == WSAEINPROGRESS) { + return 0; + } + errmsg = NetworkErrorString(err); + return -1; +} + void SockMan::CloseSockets() { m_listen.clear(); @@ -262,8 +335,16 @@ void SockMan::EventIOLoopCompletedForAllPeers() {} void SockMan::EventI2PListen(const CService&, bool) {} +void SockMan::TestOnlyAddExistentNode(NodeId node_id, std::unique_ptr&& sock) +{ + LOCK(m_connected_mutex); + m_connected.emplace(node_id, std::make_shared(std::move(sock))); +} + void SockMan::ThreadI2PAccept() { + AssertLockNotHeld(m_connected_mutex); + static constexpr auto err_wait_begin = 1s; static constexpr auto err_wait_cap = 5min; auto err_wait = err_wait_begin; @@ -297,3 +378,151 @@ void SockMan::ThreadI2PAccept() err_wait = err_wait_begin; } } + +void SockMan::ThreadSocketHandler() +{ + AssertLockNotHeld(m_connected_mutex); + + while (!interruptNet) { + EventIOLoopCompletedForAllPeers(); + + // Check for the readiness of the already connected sockets and the + // listening sockets in one call ("readiness" as in poll(2) or + // select(2)). If none are ready, wait for a short while and return + // empty sets. + auto io_readiness{GenerateWaitSockets()}; + if (io_readiness.events_per_sock.empty() || + !io_readiness.events_per_sock.begin()->first->WaitMany(SELECT_TIMEOUT, + io_readiness.events_per_sock)) { + interruptNet.sleep_for(SELECT_TIMEOUT); + } + + // Service (send/receive) each of the already connected sockets. + SocketHandlerConnected(io_readiness); + + // Accept new connections from listening sockets. + SocketHandlerListening(io_readiness.events_per_sock); + } +} + +SockMan::IOReadiness SockMan::GenerateWaitSockets() +{ + AssertLockNotHeld(m_connected_mutex); + + IOReadiness io_readiness; + + for (const auto& sock : m_listen) { + io_readiness.events_per_sock.emplace(sock, Sock::Events{Sock::RECV}); + } + + auto connected_snapshot{WITH_LOCK(m_connected_mutex, return m_connected;)}; + + for (const auto& [node_id, node_sockets] : connected_snapshot) { + const bool select_recv{ShouldTryToRecv(node_id)}; + const bool select_send{ShouldTryToSend(node_id)}; + if (!select_recv && !select_send) continue; + + Sock::Event event = (select_send ? Sock::SEND : 0) | (select_recv ? Sock::RECV : 0); + io_readiness.events_per_sock.emplace(node_sockets->sock, Sock::Events{event}); + io_readiness.node_ids_per_sock.emplace(node_sockets->sock, node_id); + } + + return io_readiness; +} + +void SockMan::SocketHandlerConnected(const IOReadiness& io_readiness) +{ + AssertLockNotHeld(m_connected_mutex); + + for (const auto& [sock, events] : io_readiness.events_per_sock) { + if (interruptNet) { + return; + } + + auto it{io_readiness.node_ids_per_sock.find(sock)}; + if (it == io_readiness.node_ids_per_sock.end()) { + continue; + } + const NodeId node_id{it->second}; + + bool send_ready = events.occurred & Sock::SEND; + bool recv_ready = events.occurred & Sock::RECV; + bool err_ready = events.occurred & Sock::ERR; + + if (send_ready) { + bool cancel_recv; + + EventReadyToSend(node_id, cancel_recv); + + if (cancel_recv) { + recv_ready = false; + } + } + + if (recv_ready || err_ready) { + uint8_t buf[0x10000]; // typical socket buffer is 8K-64K + + auto node_sockets{GetNodeSockets(node_id)}; + if (!node_sockets) { + continue; + } + + const ssize_t nrecv{WITH_LOCK( + node_sockets->mutex, + return node_sockets->sock->Recv(buf, sizeof(buf), MSG_DONTWAIT);)}; + + switch (nrecv) { + case -1: { + const int err = WSAGetLastError(); + if (err != WSAEWOULDBLOCK && err != WSAEMSGSIZE && err != WSAEINTR && err != WSAEINPROGRESS) { + EventGotPermanentReadError(node_id, NetworkErrorString(err)); + } + break; + } + case 0: + EventGotEOF(node_id); + break; + default: + EventGotData(node_id, buf, nrecv); + break; + } + } + + EventIOLoopCompletedForNode(node_id); + } +} + +void SockMan::SocketHandlerListening(const Sock::EventsPerSock& events_per_sock) +{ + AssertLockNotHeld(m_connected_mutex); + + for (const auto& sock : m_listen) { + if (interruptNet) { + return; + } + const auto it = events_per_sock.find(sock); + if (it != events_per_sock.end() && it->second.occurred & Sock::RECV) { + CService addr_accepted; + + auto sock_accepted{AcceptConnection(*sock, addr_accepted)}; + + if (sock_accepted) { + NewSockAccepted(std::move(sock_accepted), GetBindAddress(*sock), addr_accepted); + } + } + } +} + +std::shared_ptr SockMan::GetNodeSockets(NodeId node_id) const +{ + LOCK(m_connected_mutex); + + auto it{m_connected.find(node_id)}; + if (it == m_connected.end()) { + // There is no socket in case we've already disconnected, or in test cases without + // real connections. + return {}; + } + + return it->second; +} diff --git a/src/sockman.h b/src/sockman.h index fa8de15b38bf..617d8133f6c5 100644 --- a/src/sockman.h +++ b/src/sockman.h @@ -21,8 +21,6 @@ typedef int64_t NodeId; -CService GetBindAddress(const Sock& sock); - /** * A socket manager class which handles socket operations. * To use this class, inherit from it and implement the pure virtual methods. @@ -31,6 +29,8 @@ CService GetBindAddress(const Sock& sock); * - starting of necessary threads to process socket operations * - accepting incoming connections * - making outbound connections + * - closing connections + * - waiting for IO readiness on sockets and doing send/recv accordingly */ class SockMan { @@ -97,18 +97,14 @@ class SockMan * proxy, then it will be set to true. * @param[out] me If the connection was successful then this is set to the address on the * local side of the socket. - * @param[out] sock Connected socket, if the operation is successful. - * @param[out] i2p_transient_session I2P session, if the operation is successful. * @return Newly generated node id, or std::nullopt if the operation fails. */ std::optional ConnectAndMakeNodeId(const std::variant& to, bool is_important, const Proxy& proxy, bool& proxy_failed, - CService& me, - std::unique_ptr& sock, - std::unique_ptr& i2p_transient_session) - EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); + CService& me) + EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex, !m_unused_i2p_sessions_mutex); /** * Accept a connection. @@ -125,13 +121,38 @@ class SockMan * @param[in] me Address at our end of the connection. * @param[in] them Address of the new peer. */ - void NewSockAccepted(std::unique_ptr&& sock, const CService& me, const CService& them); + void NewSockAccepted(std::unique_ptr&& sock, const CService& me, const CService& them) + EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex); /** * Generate an id for a newly created node. */ NodeId GetNewNodeId(); + /** + * Disconnect a given peer by closing its socket and release resources occupied by it. + * @return Whether the peer existed and its socket was closed by this call. + */ + bool CloseConnection(NodeId node_id) + EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex); + + /** + * Try to send some data to the given node. + * @param[in] node_id Identifier of the node to send to. + * @param[in] data The data to send, it might happen that only a prefix of this is sent. + * @param[in] will_send_more Used as an optimization if the caller knows that they will + * be sending more data soon after this call. + * @param[out] errmsg If <0 is returned then this will contain a human readable message + * explaining the error. + * @retval >=0 The number of bytes actually sent. + * @retval <0 A permanent error has occurred. + */ + ssize_t SendBytes(NodeId node_id, + Span data, + bool will_send_more, + std::string& errmsg) const + EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex); + /** * Close all sockets. */ @@ -144,12 +165,13 @@ class SockMan /** * Be notified when a new connection has been accepted. * @param[in] node_id Id of the newly accepted connection. - * @param[in] sock Connected socket to communicate with the peer. * @param[in] me The address and port at our side of the connection. * @param[in] them The address and port at the peer's side of the connection. + * @retval true The new connection was accepted at the higher level. + * @retval false The connection was refused at the higher level, so the + * associated socket and node_id should be discarded by `SockMan`. */ - virtual void EventNewConnectionAccepted(NodeId node_id, - std::unique_ptr&& sock, + virtual bool EventNewConnectionAccepted(NodeId node_id, const CService& me, const CService& them) = 0; @@ -254,6 +276,15 @@ class SockMan */ std::vector> m_listen; +protected: + + /** + * During some tests mocked sockets are created outside of `SockMan`, make it + * possible to add those so that send/recv can be exercised. + */ + void TestOnlyAddExistentNode(NodeId node_id, std::unique_ptr&& sock) + EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex); + private: /** @@ -262,17 +293,107 @@ class SockMan */ static constexpr size_t MAX_UNUSED_I2P_SESSIONS_SIZE{10}; + /** + * The sockets used by a connected node - a data socket and an optional I2P session. + */ + struct NodeSockets { + explicit NodeSockets(std::unique_ptr&& s) + : sock{std::move(s)} + { + } + + explicit NodeSockets(std::shared_ptr&& s, std::unique_ptr&& sess) + : sock{std::move(s)}, + i2p_transient_session{std::move(sess)} + { + } + + /** + * Mutex that serializes the Send() and Recv() calls on `sock`. + */ + Mutex mutex; + + /** + * Underlying socket. + * `shared_ptr` (instead of `unique_ptr`) is used to avoid premature close of the + * underlying file descriptor by one thread while another thread is poll(2)-ing + * it for activity. + * @see https://github.com/bitcoin/bitcoin/issues/21744 for details. + */ + std::shared_ptr sock; + + /** + * When transient I2P sessions are used, then each node has its own session, otherwise + * all nodes use the session from `m_i2p_sam_session` and share the same I2P address. + * I2P sessions involve a data/transport socket (in `sock`) and a control socket + * (in `i2p_transient_session`). For transient sessions, once the data socket `sock` is + * closed, the control socket is not going to be used anymore and would be just taking + * resources. Storing it here makes its deletion together with `sock` automatic. + */ + std::unique_ptr i2p_transient_session; + }; + + /** + * Info about which socket has which event ready and its node id. + */ + struct IOReadiness { + Sock::EventsPerSock events_per_sock; + std::unordered_map node_ids_per_sock; + }; + /** * Accept incoming I2P connections in a loop and call * `EventNewConnectionAccepted()` for each new connection. */ - void ThreadI2PAccept(); + void ThreadI2PAccept() + EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex); + + /** + * Check connected and listening sockets for IO readiness and process them accordingly. + */ + void ThreadSocketHandler() + EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex); + + /** + * Generate a collection of sockets to check for IO readiness. + * @return Sockets to check for readiness plus an aux map to find the + * corresponding node id given a socket. + */ + IOReadiness GenerateWaitSockets() + EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex); + + /** + * Do the read/write for connected sockets that are ready for IO. + * @param[in] io_readiness Which sockets are ready and their node ids. + */ + void SocketHandlerConnected(const IOReadiness& io_readiness) + EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex); + + /** + * Accept incoming connections, one from each read-ready listening socket. + * @param[in] events_per_sock Sockets that are ready for IO. + */ + void SocketHandlerListening(const Sock::EventsPerSock& events_per_sock) + EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex); + + /** + * Retrieve an entry from m_connected. + * @param[in] node_id Node id to search for. + * @return NodeSockets for the given node id or empty shared_ptr if not found. + */ + std::shared_ptr GetNodeSockets(NodeId node_id) const + EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex); /** * The id to assign to the next created node. Used to generate ids of nodes. */ std::atomic m_next_node_id{0}; + /** + * Thread that sends to and receives from sockets and accepts connections. + */ + std::thread m_thread_socket_handler; + /** * Thread that accepts incoming I2P connections in a loop, can be stopped via `interruptNet`. */ @@ -291,6 +412,15 @@ class SockMan * a host fails, then the created session is put to this pool for reuse. */ std::queue> m_unused_i2p_sessions GUARDED_BY(m_unused_i2p_sessions_mutex); + + mutable Mutex m_connected_mutex; + + /** + * Sockets for connected peers. + * The `shared_ptr` makes it possible to create a snapshot of this by simply copying + * it (under `m_connected_mutex`). + */ + std::unordered_map> m_connected GUARDED_BY(m_connected_mutex); }; #endif // BITCOIN_SOCKMAN_H diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 9ee7e9c9fe24..d2e62c7395cd 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -55,7 +55,6 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) CAddress addr1(ip(0xa0b0c001), NODE_NONE); NodeId id{0}; CNode dummyNode1{id++, - /*sock=*/nullptr, addr1, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, @@ -121,7 +120,6 @@ void AddRandomOutboundPeer(NodeId& id, std::vector& vNodes, PeerManager& } vNodes.emplace_back(new CNode{id++, - /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, @@ -320,7 +318,6 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) banman->ClearBanned(); NodeId id{0}; nodes[0] = new CNode{id++, - /*sock=*/nullptr, addr[0], /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, @@ -340,7 +337,6 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) BOOST_CHECK(!banman->IsDiscouraged(other_addr)); // Different address, not discouraged nodes[1] = new CNode{id++, - /*sock=*/nullptr, addr[1], /*nKeyedNetGroupIn=*/1, /*nLocalHostNonceIn=*/1, @@ -370,7 +366,6 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) // Make sure non-IP peers are discouraged and disconnected properly. nodes[2] = new CNode{id++, - /*sock=*/nullptr, addr[2], /*nKeyedNetGroupIn=*/1, /*nLocalHostNonceIn=*/1, @@ -412,7 +407,6 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) CAddress addr(ip(0xa0b0c001), NODE_NONE); NodeId id{0}; CNode dummyNode{id++, - /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/4, /*nLocalHostNonceIn=*/4, diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index beefc9d82edb..03cd3c6abdb8 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -45,13 +45,14 @@ FUZZ_TARGET(connman, .init = initialize_connman) connman.Init(options); CNetAddr random_netaddr; - CNode random_node = ConsumeNode(fuzzed_data_provider); + CNode& random_node{*ConsumeNodeAsUniquePtr(fuzzed_data_provider).release()}; + connman.AddTestNode(random_node, std::make_unique(fuzzed_data_provider)); CSubNet random_subnet; std::string random_string; LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 100) { CNode& p2p_node{*ConsumeNodeAsUniquePtr(fuzzed_data_provider).release()}; - connman.AddTestNode(p2p_node); + connman.AddTestNode(p2p_node, std::make_unique(fuzzed_data_provider)); } LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) { diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 4cec96274e7a..42b5c7180bb3 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -41,9 +41,6 @@ FUZZ_TARGET(net, .init = initialize_net) LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) { CallOneOf( fuzzed_data_provider, - [&] { - node.CloseSocketDisconnect(); - }, [&] { CNodeStats stats; node.CopyStats(stats); diff --git a/src/test/fuzz/p2p_handshake.cpp b/src/test/fuzz/p2p_handshake.cpp index 6c1ed11d456a..d8271da2f65d 100644 --- a/src/test/fuzz/p2p_handshake.cpp +++ b/src/test/fuzz/p2p_handshake.cpp @@ -64,7 +64,7 @@ FUZZ_TARGET(p2p_handshake, .init = ::initialize) const auto num_peers_to_add = fuzzed_data_provider.ConsumeIntegralInRange(1, 3); for (int i = 0; i < num_peers_to_add; ++i) { peers.push_back(ConsumeNodeAsUniquePtr(fuzzed_data_provider, i).release()); - connman.AddTestNode(*peers.back()); + connman.AddTestNode(*peers.back(), std::make_unique(fuzzed_data_provider)); peerman->InitializeNode( *peers.back(), static_cast(fuzzed_data_provider.ConsumeIntegral())); diff --git a/src/test/fuzz/p2p_headers_presync.cpp b/src/test/fuzz/p2p_headers_presync.cpp index e22087303a57..f9d4512758f9 100644 --- a/src/test/fuzz/p2p_headers_presync.cpp +++ b/src/test/fuzz/p2p_headers_presync.cpp @@ -55,7 +55,7 @@ void HeadersSyncSetup::ResetAndInitialize() for (auto conn_type : conn_types) { CAddress addr{}; - m_connections.push_back(new CNode(id++, nullptr, addr, 0, 0, addr, "", conn_type, false)); + m_connections.push_back(new CNode(id++, addr, 0, 0, addr, "", conn_type, false)); CNode& p2p_node = *m_connections.back(); connman.Handshake( diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index 6373eac1c324..e77a26a5b96a 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -67,7 +67,7 @@ FUZZ_TARGET(process_message, .init = initialize_process_message) } CNode& p2p_node = *ConsumeNodeAsUniquePtr(fuzzed_data_provider).release(); - connman.AddTestNode(p2p_node); + connman.AddTestNode(p2p_node, std::make_unique(fuzzed_data_provider)); FillNode(fuzzed_data_provider, connman, p2p_node); const auto mock_time = ConsumeTime(fuzzed_data_provider); diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index 62f38967a391..4de9acb5cf11 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -59,7 +59,7 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages) FillNode(fuzzed_data_provider, connman, p2p_node); - connman.AddTestNode(p2p_node); + connman.AddTestNode(p2p_node, std::make_unique(fuzzed_data_provider)); } LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 30) diff --git a/src/test/fuzz/util/net.h b/src/test/fuzz/util/net.h index 1a5902329e26..1f5fd38d9e10 100644 --- a/src/test/fuzz/util/net.h +++ b/src/test/fuzz/util/net.h @@ -109,7 +109,6 @@ template auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional& node_id_in = std::nullopt) noexcept { const NodeId node_id = node_id_in.value_or(fuzzed_data_provider.ConsumeIntegralInRange(0, std::numeric_limits::max())); - const auto sock = std::make_shared(fuzzed_data_provider); const CAddress address = ConsumeAddress(fuzzed_data_provider); const uint64_t keyed_net_group = fuzzed_data_provider.ConsumeIntegral(); const uint64_t local_host_nonce = fuzzed_data_provider.ConsumeIntegral(); @@ -120,7 +119,6 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional(node_id, - sock, address, keyed_net_group, local_host_nonce, @@ -131,7 +129,6 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional& nodes, PeerManager& peerman, Connm const bool inbound_onion{onion_peer && conn_type == ConnectionType::INBOUND}; nodes.emplace_back(new CNode{++id, - /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 4c72d03ab18d..fd6dfb635830 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -60,7 +60,6 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) std::string pszDest; std::unique_ptr pnode1 = std::make_unique(id++, - /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, @@ -78,7 +77,6 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) BOOST_CHECK_EQUAL(pnode1->ConnectedThroughNetwork(), Network::NET_IPV4); std::unique_ptr pnode2 = std::make_unique(id++, - /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/1, /*nLocalHostNonceIn=*/1, @@ -96,7 +94,6 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) BOOST_CHECK_EQUAL(pnode2->ConnectedThroughNetwork(), Network::NET_IPV4); std::unique_ptr pnode3 = std::make_unique(id++, - /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, @@ -114,7 +111,6 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) BOOST_CHECK_EQUAL(pnode3->ConnectedThroughNetwork(), Network::NET_IPV4); std::unique_ptr pnode4 = std::make_unique(id++, - /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/1, /*nLocalHostNonceIn=*/1, @@ -613,7 +609,6 @@ BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test) ipv4AddrPeer.s_addr = 0xa0b0c001; CAddress addr = CAddress(CService(ipv4AddrPeer, 7777), NODE_NETWORK); std::unique_ptr pnode = std::make_unique(/*id=*/0, - /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, @@ -667,7 +662,6 @@ BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port) in_addr peer_out_in_addr; peer_out_in_addr.s_addr = htonl(0x01020304); CNode peer_out{/*id=*/0, - /*sock=*/nullptr, /*addrIn=*/CAddress{CService{peer_out_in_addr, 8333}, NODE_NETWORK}, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, @@ -688,7 +682,6 @@ BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port) in_addr peer_in_in_addr; peer_in_in_addr.s_addr = htonl(0x05060708); CNode peer_in{/*id=*/0, - /*sock=*/nullptr, /*addrIn=*/CAddress{CService{peer_in_in_addr, 8333}, NODE_NETWORK}, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, @@ -825,7 +818,6 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message) in_addr peer_in_addr; peer_in_addr.s_addr = htonl(0x01020304); CNode peer{/*id=*/0, - /*sock=*/nullptr, /*addrIn=*/CAddress{CService{peer_in_addr, 8333}, NODE_NETWORK}, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, @@ -900,7 +892,6 @@ BOOST_AUTO_TEST_CASE(advertise_local_address) { auto CreatePeer = [](const CAddress& addr) { return std::make_unique(/*id=*/0, - /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, diff --git a/src/test/util/net.h b/src/test/util/net.h index f58e75b3c072..e8e96138cc9d 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -50,6 +50,12 @@ struct ConnmanTestMsg : public CConnman { return m_nodes; } + void AddTestNode(CNode& node, std::unique_ptr&& sock) + { + TestOnlyAddExistentNode(node.GetId(), std::move(sock)); + AddTestNode(node); + } + void AddTestNode(CNode& node) { LOCK(m_nodes_mutex); From d8d9ffceb9383af9127e0b1b841010d44ec73508 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 23 Sep 2024 11:05:59 +0200 Subject: [PATCH 16/16] net: move-only: improve encapsulation of SockMan `SockMan` members `AcceptConnection()` `NewSockAccepted()` `GetNewNodeId()` `m_i2p_sam_session` `m_listen private` are now used only by `SockMan`, thus make them private. --- src/sockman.cpp | 118 ++++++++++++++++++++++++------------------------ src/sockman.h | 70 ++++++++++++++-------------- 2 files changed, 94 insertions(+), 94 deletions(-) diff --git a/src/sockman.cpp b/src/sockman.cpp index 7cb517079e81..4a9b41f1f3c6 100644 --- a/src/sockman.cpp +++ b/src/sockman.cpp @@ -215,65 +215,6 @@ SockMan::ConnectAndMakeNodeId(const std::variant& t return node_id; } -std::unique_ptr SockMan::AcceptConnection(const Sock& listen_sock, CService& addr) -{ - sockaddr_storage storage; - socklen_t len{sizeof(storage)}; - - auto sock{listen_sock.Accept(reinterpret_cast(&storage), &len)}; - - if (!sock) { - const int err{WSAGetLastError()}; - if (err != WSAEWOULDBLOCK) { - LogPrintLevel(BCLog::NET, - BCLog::Level::Error, - "Cannot accept new connection: %s\n", - NetworkErrorString(err)); - } - return {}; - } - - if (!addr.SetSockAddr(reinterpret_cast(&storage))) { - LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Unknown socket family\n"); - } - - return sock; -} - -void SockMan::NewSockAccepted(std::unique_ptr&& sock, const CService& me, const CService& them) -{ - AssertLockNotHeld(m_connected_mutex); - - if (!sock->IsSelectable()) { - LogPrintf("connection from %s dropped: non-selectable socket\n", them.ToStringAddrPort()); - return; - } - - // According to the internet TCP_NODELAY is not carried into accepted sockets - // on all platforms. Set it again here just to be sure. - const int on{1}; - if (sock->SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) == SOCKET_ERROR) { - LogDebug(BCLog::NET, "connection from %s: unable to set TCP_NODELAY, continuing anyway\n", - them.ToStringAddrPort()); - } - - const NodeId node_id{GetNewNodeId()}; - - { - LOCK(m_connected_mutex); - m_connected.emplace(node_id, std::make_shared(std::move(sock))); - } - - if (!EventNewConnectionAccepted(node_id, me, them)) { - CloseConnection(node_id); - } -} - -NodeId SockMan::GetNewNodeId() -{ - return m_next_node_id.fetch_add(1, std::memory_order_relaxed); -} - bool SockMan::CloseConnection(NodeId node_id) { LOCK(m_connected_mutex); @@ -405,6 +346,65 @@ void SockMan::ThreadSocketHandler() } } +std::unique_ptr SockMan::AcceptConnection(const Sock& listen_sock, CService& addr) +{ + sockaddr_storage storage; + socklen_t len{sizeof(storage)}; + + auto sock{listen_sock.Accept(reinterpret_cast(&storage), &len)}; + + if (!sock) { + const int err{WSAGetLastError()}; + if (err != WSAEWOULDBLOCK) { + LogPrintLevel(BCLog::NET, + BCLog::Level::Error, + "Cannot accept new connection: %s\n", + NetworkErrorString(err)); + } + return {}; + } + + if (!addr.SetSockAddr(reinterpret_cast(&storage))) { + LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Unknown socket family\n"); + } + + return sock; +} + +void SockMan::NewSockAccepted(std::unique_ptr&& sock, const CService& me, const CService& them) +{ + AssertLockNotHeld(m_connected_mutex); + + if (!sock->IsSelectable()) { + LogPrintf("connection from %s dropped: non-selectable socket\n", them.ToStringAddrPort()); + return; + } + + // According to the internet TCP_NODELAY is not carried into accepted sockets + // on all platforms. Set it again here just to be sure. + const int on{1}; + if (sock->SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) == SOCKET_ERROR) { + LogDebug(BCLog::NET, "connection from %s: unable to set TCP_NODELAY, continuing anyway\n", + them.ToStringAddrPort()); + } + + const NodeId node_id{GetNewNodeId()}; + + { + LOCK(m_connected_mutex); + m_connected.emplace(node_id, std::make_shared(std::move(sock))); + } + + if (!EventNewConnectionAccepted(node_id, me, them)) { + CloseConnection(node_id); + } +} + +NodeId SockMan::GetNewNodeId() +{ + return m_next_node_id.fetch_add(1, std::memory_order_relaxed); +} + SockMan::IOReadiness SockMan::GenerateWaitSockets() { AssertLockNotHeld(m_connected_mutex); diff --git a/src/sockman.h b/src/sockman.h index 617d8133f6c5..ccbb18243a7a 100644 --- a/src/sockman.h +++ b/src/sockman.h @@ -106,29 +106,6 @@ class SockMan CService& me) EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex, !m_unused_i2p_sessions_mutex); - /** - * Accept a connection. - * @param[in] listen_sock Socket on which to accept the connection. - * @param[out] addr Address of the peer that was accepted. - * @return Newly created socket for the accepted connection. - */ - std::unique_ptr AcceptConnection(const Sock& listen_sock, CService& addr); - - /** - * After a new socket with a peer has been created, configure its flags, - * make a new node id and call `EventNewConnectionAccepted()`. - * @param[in] sock The newly created socket. - * @param[in] me Address at our end of the connection. - * @param[in] them Address of the new peer. - */ - void NewSockAccepted(std::unique_ptr&& sock, const CService& me, const CService& them) - EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex); - - /** - * Generate an id for a newly created node. - */ - NodeId GetNewNodeId(); - /** * Disconnect a given peer by closing its socket and release resources occupied by it. * @return Whether the peer existed and its socket was closed by this call. @@ -264,18 +241,6 @@ class SockMan */ CThreadInterrupt interruptNet; - /** - * I2P SAM session. - * Used to accept incoming and make outgoing I2P connections from a persistent - * address. - */ - std::unique_ptr m_i2p_sam_session; - - /** - * List of listening sockets. - */ - std::vector> m_listen; - protected: /** @@ -354,6 +319,29 @@ class SockMan void ThreadSocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex); + /** + * Accept a connection. + * @param[in] listen_sock Socket on which to accept the connection. + * @param[out] addr Address of the peer that was accepted. + * @return Newly created socket for the accepted connection. + */ + std::unique_ptr AcceptConnection(const Sock& listen_sock, CService& addr); + + /** + * After a new socket with a peer has been created, configure its flags, + * make a new node id and call `EventNewConnectionAccepted()`. + * @param[in] sock The newly created socket. + * @param[in] me Address at our end of the connection. + * @param[in] them Address of the new peer. + */ + void NewSockAccepted(std::unique_ptr&& sock, const CService& me, const CService& them) + EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex); + + /** + * Generate an id for a newly created node. + */ + NodeId GetNewNodeId(); + /** * Generate a collection of sockets to check for IO readiness. * @return Sockets to check for readiness plus an aux map to find the @@ -413,6 +401,18 @@ class SockMan */ std::queue> m_unused_i2p_sessions GUARDED_BY(m_unused_i2p_sessions_mutex); + /** + * I2P SAM session. + * Used to accept incoming and make outgoing I2P connections from a persistent + * address. + */ + std::unique_ptr m_i2p_sam_session; + + /** + * List of listening sockets. + */ + std::vector> m_listen; + mutable Mutex m_connected_mutex; /**