From 4b58d55878db55372d1b09de49c6caf363fe3c06 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 6 Dec 2022 13:38:21 +0100 Subject: [PATCH 01/22] test: move the implementation of StaticContentsSock to .cpp Move the implementation (method definitions) from `test/util/net.h` to `test/util/net.cpp` to make the header easier to follow. --- src/test/util/net.cpp | 90 +++++++++++++++++++++++++++++++++++++++++++ src/test/util/net.h | 89 ++++++++---------------------------------- 2 files changed, 107 insertions(+), 72 deletions(-) diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index beefc32bee4b7..0861c2cc09b93 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -137,3 +137,93 @@ std::vector GetRandomNodeEvictionCandidates(int n_candida } return candidates; } + +StaticContentsSock::StaticContentsSock(const std::string& contents) + : Sock{INVALID_SOCKET}, m_contents{contents} +{ +} + +StaticContentsSock::~StaticContentsSock() { m_socket = INVALID_SOCKET; } + +StaticContentsSock& StaticContentsSock::operator=(Sock&& other) +{ + assert(false && "Move of Sock into MockSock not allowed."); + return *this; +} + +ssize_t StaticContentsSock::Send(const void*, size_t len, int) const { return len; } + +ssize_t StaticContentsSock::Recv(void* buf, size_t len, int flags) const +{ + const size_t consume_bytes{std::min(len, m_contents.size() - m_consumed)}; + std::memcpy(buf, m_contents.data() + m_consumed, consume_bytes); + if ((flags & MSG_PEEK) == 0) { + m_consumed += consume_bytes; + } + return consume_bytes; +} + +int StaticContentsSock::Connect(const sockaddr*, socklen_t) const { return 0; } + +int StaticContentsSock::Bind(const sockaddr*, socklen_t) const { return 0; } + +int StaticContentsSock::Listen(int) const { return 0; } + +std::unique_ptr StaticContentsSock::Accept(sockaddr* addr, socklen_t* addr_len) const +{ + if (addr != nullptr) { + // Pretend all connections come from 5.5.5.5:6789 + memset(addr, 0x00, *addr_len); + const socklen_t write_len = static_cast(sizeof(sockaddr_in)); + if (*addr_len >= write_len) { + *addr_len = write_len; + sockaddr_in* addr_in = reinterpret_cast(addr); + addr_in->sin_family = AF_INET; + memset(&addr_in->sin_addr, 0x05, sizeof(addr_in->sin_addr)); + addr_in->sin_port = htons(6789); + } + } + return std::make_unique(""); +}; + +int StaticContentsSock::GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const +{ + std::memset(opt_val, 0x0, *opt_len); + return 0; +} + +int StaticContentsSock::SetSockOpt(int, int, const void*, socklen_t) const { return 0; } + +int StaticContentsSock::GetSockName(sockaddr* name, socklen_t* name_len) const +{ + std::memset(name, 0x0, *name_len); + return 0; +} + +bool StaticContentsSock::SetNonBlocking() const { return true; } + +bool StaticContentsSock::IsSelectable() const { return true; } + +bool StaticContentsSock::Wait(std::chrono::milliseconds timeout, + Event requested, + Event* occurred) const +{ + if (occurred != nullptr) { + *occurred = requested; + } + return true; +} + +bool StaticContentsSock::WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_per_sock) const +{ + for (auto& [sock, events] : events_per_sock) { + (void)sock; + events.occurred = events.requested; + } + return true; +} + +bool StaticContentsSock::IsConnected(std::string&) const +{ + return true; +} diff --git a/src/test/util/net.h b/src/test/util/net.h index 043e317bf080f..9397dbad3d730 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -141,96 +141,41 @@ constexpr auto ALL_NETWORKS = std::array{ class StaticContentsSock : public Sock { public: - explicit StaticContentsSock(const std::string& contents) - : Sock{INVALID_SOCKET}, - m_contents{contents} - { - } + explicit StaticContentsSock(const std::string& contents); - ~StaticContentsSock() override { m_socket = INVALID_SOCKET; } + ~StaticContentsSock() override; - StaticContentsSock& operator=(Sock&& other) override - { - assert(false && "Move of Sock into MockSock not allowed."); - return *this; - } + StaticContentsSock& operator=(Sock&& other) override; - ssize_t Send(const void*, size_t len, int) const override { return len; } + ssize_t Send(const void*, size_t len, int) const override; - ssize_t Recv(void* buf, size_t len, int flags) const override - { - const size_t consume_bytes{std::min(len, m_contents.size() - m_consumed)}; - std::memcpy(buf, m_contents.data() + m_consumed, consume_bytes); - if ((flags & MSG_PEEK) == 0) { - m_consumed += consume_bytes; - } - return consume_bytes; - } + ssize_t Recv(void* buf, size_t len, int flags) const override; - int Connect(const sockaddr*, socklen_t) const override { return 0; } + int Connect(const sockaddr*, socklen_t) const override; - int Bind(const sockaddr*, socklen_t) const override { return 0; } + int Bind(const sockaddr*, socklen_t) const override; - int Listen(int) const override { return 0; } + int Listen(int) const override; - std::unique_ptr Accept(sockaddr* addr, socklen_t* addr_len) const override - { - if (addr != nullptr) { - // Pretend all connections come from 5.5.5.5:6789 - memset(addr, 0x00, *addr_len); - const socklen_t write_len = static_cast(sizeof(sockaddr_in)); - if (*addr_len >= write_len) { - *addr_len = write_len; - sockaddr_in* addr_in = reinterpret_cast(addr); - addr_in->sin_family = AF_INET; - memset(&addr_in->sin_addr, 0x05, sizeof(addr_in->sin_addr)); - addr_in->sin_port = htons(6789); - } - } - return std::make_unique(""); - }; + std::unique_ptr Accept(sockaddr* addr, socklen_t* addr_len) const override; - int GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const override - { - std::memset(opt_val, 0x0, *opt_len); - return 0; - } + int GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const override; - int SetSockOpt(int, int, const void*, socklen_t) const override { return 0; } + int SetSockOpt(int, int, const void*, socklen_t) const override; - int GetSockName(sockaddr* name, socklen_t* name_len) const override - { - std::memset(name, 0x0, *name_len); - return 0; - } + int GetSockName(sockaddr* name, socklen_t* name_len) const override; - bool SetNonBlocking() const override { return true; } + bool SetNonBlocking() const override; - bool IsSelectable() const override { return true; } + bool IsSelectable() const override; bool Wait(std::chrono::milliseconds timeout, Event requested, - Event* occurred = nullptr) const override - { - if (occurred != nullptr) { - *occurred = requested; - } - return true; - } + Event* occurred = nullptr) const override; - bool WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_per_sock) const override - { - for (auto& [sock, events] : events_per_sock) { - (void)sock; - events.occurred = events.requested; - } - return true; - } + bool WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_per_sock) const override; - bool IsConnected(std::string&) const override - { - return true; - } + bool IsConnected(std::string&) const override; private: const std::string m_contents; From 2f6ce54212926f7f8642dfa5021f4ff089a4de61 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 6 Sep 2024 11:16:50 +0200 Subject: [PATCH 02/22] test: put the generic parts from StaticContentsSock into a separate class This allows reusing them in other mocked implementations. --- src/test/util/net.cpp | 83 +++++++++++++++++++++++++------------------ src/test/util/net.h | 44 +++++++++++++++++------ 2 files changed, 81 insertions(+), 46 deletions(-) diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 0861c2cc09b93..77ce3b7585de4 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -138,38 +138,31 @@ std::vector GetRandomNodeEvictionCandidates(int n_candida return candidates; } -StaticContentsSock::StaticContentsSock(const std::string& contents) - : Sock{INVALID_SOCKET}, m_contents{contents} -{ -} +// Have different ZeroSock (or others that inherit from it) objects have different +// m_socket because EqualSharedPtrSock compares m_socket and we want to avoid two +// different objects comparing as equal. +static std::atomic g_mocked_sock_fd{0}; -StaticContentsSock::~StaticContentsSock() { m_socket = INVALID_SOCKET; } +ZeroSock::ZeroSock() : Sock{g_mocked_sock_fd++} {} -StaticContentsSock& StaticContentsSock::operator=(Sock&& other) -{ - assert(false && "Move of Sock into MockSock not allowed."); - return *this; -} +// Sock::~Sock() would try to close(2) m_socket if it is not INVALID_SOCKET, avoid that. +ZeroSock::~ZeroSock() { m_socket = INVALID_SOCKET; } -ssize_t StaticContentsSock::Send(const void*, size_t len, int) const { return len; } +ssize_t ZeroSock::Send(const void*, size_t len, int) const { return len; } -ssize_t StaticContentsSock::Recv(void* buf, size_t len, int flags) const +ssize_t ZeroSock::Recv(void* buf, size_t len, int flags) const { - const size_t consume_bytes{std::min(len, m_contents.size() - m_consumed)}; - std::memcpy(buf, m_contents.data() + m_consumed, consume_bytes); - if ((flags & MSG_PEEK) == 0) { - m_consumed += consume_bytes; - } - return consume_bytes; + memset(buf, 0x0, len); + return len; } -int StaticContentsSock::Connect(const sockaddr*, socklen_t) const { return 0; } +int ZeroSock::Connect(const sockaddr*, socklen_t) const { return 0; } -int StaticContentsSock::Bind(const sockaddr*, socklen_t) const { return 0; } +int ZeroSock::Bind(const sockaddr*, socklen_t) const { return 0; } -int StaticContentsSock::Listen(int) const { return 0; } +int ZeroSock::Listen(int) const { return 0; } -std::unique_ptr StaticContentsSock::Accept(sockaddr* addr, socklen_t* addr_len) const +std::unique_ptr ZeroSock::Accept(sockaddr* addr, socklen_t* addr_len) const { if (addr != nullptr) { // Pretend all connections come from 5.5.5.5:6789 @@ -183,30 +176,28 @@ std::unique_ptr StaticContentsSock::Accept(sockaddr* addr, socklen_t* addr addr_in->sin_port = htons(6789); } } - return std::make_unique(""); -}; + return std::make_unique(); +} -int StaticContentsSock::GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const +int ZeroSock::GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const { std::memset(opt_val, 0x0, *opt_len); return 0; } -int StaticContentsSock::SetSockOpt(int, int, const void*, socklen_t) const { return 0; } +int ZeroSock::SetSockOpt(int, int, const void*, socklen_t) const { return 0; } -int StaticContentsSock::GetSockName(sockaddr* name, socklen_t* name_len) const +int ZeroSock::GetSockName(sockaddr* name, socklen_t* name_len) const { std::memset(name, 0x0, *name_len); return 0; } -bool StaticContentsSock::SetNonBlocking() const { return true; } +bool ZeroSock::SetNonBlocking() const { return true; } -bool StaticContentsSock::IsSelectable() const { return true; } +bool ZeroSock::IsSelectable() const { return true; } -bool StaticContentsSock::Wait(std::chrono::milliseconds timeout, - Event requested, - Event* occurred) const +bool ZeroSock::Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred) const { if (occurred != nullptr) { *occurred = requested; @@ -214,7 +205,7 @@ bool StaticContentsSock::Wait(std::chrono::milliseconds timeout, return true; } -bool StaticContentsSock::WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_per_sock) const +bool ZeroSock::WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_per_sock) const { for (auto& [sock, events] : events_per_sock) { (void)sock; @@ -223,7 +214,29 @@ bool StaticContentsSock::WaitMany(std::chrono::milliseconds timeout, EventsPerSo return true; } -bool StaticContentsSock::IsConnected(std::string&) const +ZeroSock& ZeroSock::operator=(Sock&& other) { - return true; + assert(false && "Move of Sock into ZeroSock not allowed."); + return *this; +} + +StaticContentsSock::StaticContentsSock(const std::string& contents) + : m_contents{contents} +{ +} + +ssize_t StaticContentsSock::Recv(void* buf, size_t len, int flags) const +{ + const size_t consume_bytes{std::min(len, m_contents.size() - m_consumed)}; + std::memcpy(buf, m_contents.data() + m_consumed, consume_bytes); + if ((flags & MSG_PEEK) == 0) { + m_consumed += consume_bytes; + } + return consume_bytes; +} + +StaticContentsSock& StaticContentsSock::operator=(Sock&& other) +{ + assert(false && "Move of Sock into StaticContentsSock not allowed."); + return *this; } diff --git a/src/test/util/net.h b/src/test/util/net.h index 9397dbad3d730..dfea9dd44b83c 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -133,19 +133,18 @@ constexpr auto ALL_NETWORKS = std::array{ Network::NET_INTERNAL, }; +std::vector GetRandomNodeEvictionCandidates(int n_candidates, FastRandomContext& random_context); + /** - * A mocked Sock alternative that returns a statically contained data upon read and succeeds - * and ignores all writes. The data to be returned is given to the constructor and when it is - * exhausted an EOF is returned by further reads. + * A mocked Sock alternative that succeeds on all operations. + * Returns infinite amount of 0x0 bytes on reads. */ -class StaticContentsSock : public Sock +class ZeroSock : public Sock { public: - explicit StaticContentsSock(const std::string& contents); - - ~StaticContentsSock() override; + ZeroSock(); - StaticContentsSock& operator=(Sock&& other) override; + ~ZeroSock() override; ssize_t Send(const void*, size_t len, int) const override; @@ -175,13 +174,36 @@ class StaticContentsSock : public Sock bool WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_per_sock) const override; - bool IsConnected(std::string&) const override; +private: + ZeroSock& operator=(Sock&& other) override; +}; + +/** + * A mocked Sock alternative that returns a statically contained data upon read and succeeds + * and ignores all writes. The data to be returned is given to the constructor and when it is + * exhausted an EOF is returned by further reads. + */ +class StaticContentsSock : public ZeroSock +{ +public: + explicit StaticContentsSock(const std::string& contents); + + /** + * Return parts of the contents that was provided at construction until it is exhausted + * and then return 0 (EOF). + */ + ssize_t Recv(void* buf, size_t len, int flags) const override; + + bool IsConnected(std::string&) const override + { + return true; + } private: + StaticContentsSock& operator=(Sock&& other) override; + const std::string m_contents; mutable size_t m_consumed{0}; }; -std::vector GetRandomNodeEvictionCandidates(int n_candidates, FastRandomContext& random_context); - #endif // BITCOIN_TEST_UTIL_NET_H From 5766bbefa94d40f8d37639c2752961617d7a262a Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 6 Dec 2022 13:42:03 +0100 Subject: [PATCH 03/22] test: add a mocked Sock that allows inspecting what has been Send() to it And also allows gradually providing the data to be returned by `Recv()` and sending and receiving net messages (`CNetMessage`). --- src/test/util/net.cpp | 168 ++++++++++++++++++++++++++++++++++++++++++ src/test/util/net.h | 154 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 322 insertions(+) diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 77ce3b7585de4..ddd96a50640bf 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -14,7 +14,10 @@ #include #include #include +#include +#include +#include #include void ConnmanTestMsg::Handshake(CNode& node, @@ -240,3 +243,168 @@ StaticContentsSock& StaticContentsSock::operator=(Sock&& other) assert(false && "Move of Sock into StaticContentsSock not allowed."); return *this; } + +ssize_t DynSock::Pipe::GetBytes(void* buf, size_t len, int flags) +{ + WAIT_LOCK(m_mutex, lock); + + if (m_data.empty()) { + if (m_eof) { + return 0; + } + errno = EAGAIN; // Same as recv(2) on a non-blocking socket. + return -1; + } + + const size_t read_bytes{std::min(len, m_data.size())}; + + std::memcpy(buf, m_data.data(), read_bytes); + if ((flags & MSG_PEEK) == 0) { + m_data.erase(m_data.begin(), m_data.begin() + read_bytes); + } + + return read_bytes; +} + +std::optional DynSock::Pipe::GetNetMsg() +{ + V1Transport transport{NodeId{0}}; + + { + WAIT_LOCK(m_mutex, lock); + + WaitForDataOrEof(lock); + if (m_eof && m_data.empty()) { + return std::nullopt; + } + + for (;;) { + Span s{m_data}; + if (!transport.ReceivedBytes(s)) { // Consumed bytes are removed from the front of s. + return std::nullopt; + } + m_data.erase(m_data.begin(), m_data.begin() + m_data.size() - s.size()); + if (transport.ReceivedMessageComplete()) { + break; + } + if (m_data.empty()) { + WaitForDataOrEof(lock); + if (m_eof && m_data.empty()) { + return std::nullopt; + } + } + } + } + + bool reject{false}; + CNetMessage msg{transport.GetReceivedMessage(/*time=*/{}, reject)}; + if (reject) { + return std::nullopt; + } + return std::make_optional(std::move(msg)); +} + +void DynSock::Pipe::PushBytes(const void* buf, size_t len) +{ + LOCK(m_mutex); + const uint8_t* b = static_cast(buf); + m_data.insert(m_data.end(), b, b + len); + m_cond.notify_all(); +} + +void DynSock::Pipe::Eof() +{ + LOCK(m_mutex); + m_eof = true; + m_cond.notify_all(); +} + +void DynSock::Pipe::WaitForDataOrEof(UniqueLock& lock) +{ + Assert(lock.mutex() == &m_mutex); + + m_cond.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(m_mutex) { + AssertLockHeld(m_mutex); + return !m_data.empty() || m_eof; + }); +} + +DynSock::DynSock(std::shared_ptr pipes, std::shared_ptr accept_sockets) + : m_pipes{pipes}, m_accept_sockets{accept_sockets} +{ +} + +DynSock::~DynSock() +{ + m_pipes->send.Eof(); +} + +ssize_t DynSock::Recv(void* buf, size_t len, int flags) const +{ + return m_pipes->recv.GetBytes(buf, len, flags); +} + +ssize_t DynSock::Send(const void* buf, size_t len, int) const +{ + m_pipes->send.PushBytes(buf, len); + return len; +} + +std::unique_ptr DynSock::Accept(sockaddr* addr, socklen_t* addr_len) const +{ + ZeroSock::Accept(addr, addr_len); + return m_accept_sockets->Pop().value_or(nullptr); +} + +bool DynSock::Wait(std::chrono::milliseconds timeout, + Event requested, + Event* occurred) const +{ + EventsPerSock ev; + ev.emplace(this, Events{requested}); + const bool ret{WaitMany(timeout, ev)}; + if (occurred != nullptr) { + *occurred = ev.begin()->second.occurred; + } + return ret; +} + +bool DynSock::WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_per_sock) const +{ + const auto deadline = std::chrono::steady_clock::now() + timeout; + bool at_least_one_event_occurred{false}; + + for (;;) { + // Check all sockets for readiness without waiting. + for (auto& [sock, events] : events_per_sock) { + if ((events.requested & Sock::SEND) != 0) { + // Always ready for Send(). + events.occurred |= Sock::SEND; + at_least_one_event_occurred = true; + } + + if ((events.requested & Sock::RECV) != 0) { + auto dyn_sock = reinterpret_cast(sock.get()); + uint8_t b; + if (dyn_sock->m_pipes->recv.GetBytes(&b, 1, MSG_PEEK) == 1 || !dyn_sock->m_accept_sockets->Empty()) { + events.occurred |= Sock::RECV; + at_least_one_event_occurred = true; + } + } + } + + if (at_least_one_event_occurred || std::chrono::steady_clock::now() > deadline) { + break; + } + + std::this_thread::sleep_for(10ms); + } + + return true; +} + +DynSock& DynSock::operator=(Sock&&) +{ + assert(false && "Move of Sock into DynSock not allowed."); + return *this; +} diff --git a/src/test/util/net.h b/src/test/util/net.h index dfea9dd44b83c..63817f70fee32 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -6,6 +6,7 @@ #define BITCOIN_TEST_UTIL_NET_H #include +#include #include #include #include @@ -19,9 +20,11 @@ #include #include #include +#include #include #include #include +#include #include #include #include @@ -206,4 +209,155 @@ class StaticContentsSock : public ZeroSock mutable size_t m_consumed{0}; }; +/** + * A mocked Sock alternative that allows providing the data to be returned by Recv() + * and inspecting the data that has been supplied to Send(). + */ +class DynSock : public ZeroSock +{ +public: + /** + * Unidirectional bytes or CNetMessage queue (FIFO). + */ + class Pipe + { + public: + /** + * Get bytes and remove them from the pipe. + * @param[in] buf Destination to write bytes to. + * @param[in] len Write up to this number of bytes. + * @param[in] flags Same as the flags of `recv(2)`. Just `MSG_PEEK` is honored. + * @return The number of bytes written to `buf`. `0` if `Eof()` has been called. + * If no bytes are available then `-1` is returned and `errno` is set to `EAGAIN`. + */ + ssize_t GetBytes(void* buf, size_t len, int flags = 0) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + + /** + * Deserialize a `CNetMessage` and remove it from the pipe. + * If not enough bytes are available then the function will wait. If parsing fails + * or EOF is signaled to the pipe, then `std::nullopt` is returned. + */ + std::optional GetNetMsg() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + + /** + * Push bytes to the pipe. + */ + void PushBytes(const void* buf, size_t len) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + + /** + * Construct and push CNetMessage to the pipe. + */ + template + void PushNetMsg(const std::string& type, Args&&... payload) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + + /** + * Signal end-of-file on the receiving end (`GetBytes()` or `GetNetMsg()`). + */ + void Eof() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + + private: + /** + * Return when there is some data to read or EOF has been signaled. + * @param[in,out] lock Unique lock that must have been derived from `m_mutex` by `WAIT_LOCK(m_mutex, lock)`. + */ + void WaitForDataOrEof(UniqueLock& lock) EXCLUSIVE_LOCKS_REQUIRED(m_mutex); + + Mutex m_mutex; + std::condition_variable m_cond; + std::vector m_data GUARDED_BY(m_mutex); + bool m_eof GUARDED_BY(m_mutex){false}; + }; + + struct Pipes { + Pipe recv; + Pipe send; + }; + + /** + * A basic thread-safe queue, used for queuing sockets to be returned by Accept(). + */ + class Queue + { + public: + using S = std::unique_ptr; + + void Push(S s) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) + { + LOCK(m_mutex); + m_queue.push(std::move(s)); + } + + std::optional Pop() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) + { + LOCK(m_mutex); + if (m_queue.empty()) { + return std::nullopt; + } + S front{std::move(m_queue.front())}; + m_queue.pop(); + return front; + } + + bool Empty() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) + { + LOCK(m_mutex); + return m_queue.empty(); + } + + private: + mutable Mutex m_mutex; + std::queue m_queue GUARDED_BY(m_mutex); + }; + + /** + * Create a new mocked sock. + * @param[in] pipes Send/recv pipes used by the Send() and Recv() methods. + * @param[in] accept_sockets Sockets to return by the Accept() method. + */ + explicit DynSock(std::shared_ptr pipes, std::shared_ptr accept_sockets); + + ~DynSock(); + + ssize_t Recv(void* buf, size_t len, int flags) const override; + + ssize_t Send(const void* buf, size_t len, int) const override; + + std::unique_ptr Accept(sockaddr* addr, socklen_t* addr_len) const override; + + bool Wait(std::chrono::milliseconds timeout, + Event requested, + Event* occurred = nullptr) const override; + + bool WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_per_sock) const override; + +private: + DynSock& operator=(Sock&&) override; + + std::shared_ptr m_pipes; + std::shared_ptr m_accept_sockets; +}; + +template +void DynSock::Pipe::PushNetMsg(const std::string& type, Args&&... payload) +{ + auto msg = NetMsg::Make(type, std::forward(payload)...); + V1Transport transport{NodeId{0}}; + + const bool queued{transport.SetMessageToSend(msg)}; + assert(queued); + + LOCK(m_mutex); + + for (;;) { + const auto& [bytes, _more, _msg_type] = transport.GetBytesToSend(/*have_next_message=*/true); + if (bytes.empty()) { + break; + } + m_data.insert(m_data.end(), bytes.begin(), bytes.end()); + transport.MarkBytesSent(bytes.size()); + } + + m_cond.notify_all(); +} + #endif // BITCOIN_TEST_UTIL_NET_H From de880608345618c9e59833a7806905151c59b9f5 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 23 Aug 2024 13:11:37 +0200 Subject: [PATCH 04/22] 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. * `GetBindAddress()` only needs to return `CAddress`. * `CNode::addrBind` does not need to be `CAddress`. --- src/net.cpp | 27 +++++++++++++-------------- src/net.h | 12 ++++++------ src/test/net_tests.cpp | 6 +++--- 3 files changed, 22 insertions(+), 23 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 802dc947bea9c..889ea3723c408 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -377,9 +377,9 @@ bool CConnman::CheckIncomingNonce(uint64_t nonce) } /** Get the bind address for a socket as CAddress */ -static CAddress GetBindAddress(const Sock& sock) +static CService GetBindAddress(const Sock& sock) { - CAddress addr_bind; + 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)) { @@ -454,7 +454,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo // Connect std::unique_ptr sock; Proxy proxy; - CAddress addr_bind; + CService addr_bind; assert(!addr_bind.IsValid()); std::unique_ptr i2p_transient_session; @@ -491,7 +491,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo if (connected) { sock = std::move(conn.sock); - addr_bind = CAddress{conn.me, NODE_NONE}; + addr_bind = conn.me; } } else if (use_proxy) { LogPrintLevel(BCLog::PROXY, BCLog::Level::Debug, "Using proxy: %s to connect to %s\n", proxy.ToString(), target_addr.ToStringAddrPort()); @@ -1707,7 +1707,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(); @@ -1717,13 +1716,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); @@ -1733,8 +1733,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; @@ -1801,7 +1801,7 @@ 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, @@ -3042,8 +3042,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; } @@ -3732,7 +3731,7 @@ CNode::CNode(NodeId idIn, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, - const CAddress& addrBindIn, + const CService& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion, @@ -3869,7 +3868,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 fc096ff7b8680..99abf6eb9496e 100644 --- a/src/net.h +++ b/src/net.h @@ -211,7 +211,7 @@ class CNodeStats // Address of this peer CAddress addr; // Bind address of our side of the connection - CAddress addrBind; + CService addrBind; // Network the peer connected through Network m_network; uint32_t m_mapped_as; @@ -707,7 +707,7 @@ class CNode // Address of this peer const CAddress addr; // Bind address of our side of the connection - const CAddress addrBind; + const CService addrBind; const std::string m_addr_name; /** The pszDest argument provided to ConnectNode(). Only used for reconnections. */ const std::string m_dest; @@ -883,7 +883,7 @@ class CNode const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, - const CAddress& addrBindIn, + const CService& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion, @@ -1296,8 +1296,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(); @@ -1334,7 +1334,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); diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 384b1d7cc92d1..5f0f05c842ad4 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -671,7 +671,7 @@ BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port) /*addrIn=*/CAddress{CService{peer_out_in_addr, 8333}, NODE_NETWORK}, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, - /*addrBindIn=*/CAddress{}, + /*addrBindIn=*/CService{}, /*addrNameIn=*/std::string{}, /*conn_type_in=*/ConnectionType::OUTBOUND_FULL_RELAY, /*inbound_onion=*/false}; @@ -692,7 +692,7 @@ BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port) /*addrIn=*/CAddress{CService{peer_in_in_addr, 8333}, NODE_NETWORK}, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, - /*addrBindIn=*/CAddress{}, + /*addrBindIn=*/CService{}, /*addrNameIn=*/std::string{}, /*conn_type_in=*/ConnectionType::INBOUND, /*inbound_onion=*/false}; @@ -829,7 +829,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message) /*addrIn=*/CAddress{CService{peer_in_addr, 8333}, NODE_NETWORK}, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, - /*addrBindIn=*/CAddress{}, + /*addrBindIn=*/CService{}, /*addrNameIn=*/std::string{}, /*conn_type_in=*/ConnectionType::OUTBOUND_FULL_RELAY, /*inbound_onion=*/false}; From 89ef5287dc379638fff737f90536249ad02a0d08 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 23 Aug 2024 15:36:40 +0200 Subject: [PATCH 05/22] 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/common/sockman.cpp | 85 +++++++++++++++++++++++++++++++++++++ src/common/sockman.h | 44 +++++++++++++++++++ src/net.cpp | 95 +++++++----------------------------------- src/net.h | 25 ++++------- 5 files changed, 154 insertions(+), 96 deletions(-) create mode 100644 src/common/sockman.cpp create mode 100644 src/common/sockman.h diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 889c00c78327f..40f4ec87e4f7e 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -123,6 +123,7 @@ add_library(bitcoin_common STATIC EXCLUDE_FROM_ALL common/run_command.cpp common/settings.cpp common/signmessage.cpp + common/sockman.cpp common/system.cpp common/url.cpp compressor.cpp diff --git a/src/common/sockman.cpp b/src/common/sockman.cpp new file mode 100644 index 0000000000000..7cc7edb809a8c --- /dev/null +++ b/src/common/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 = Untranslated(strprintf("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 = Untranslated(strprintf("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 = Untranslated(strprintf("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 = Untranslated(strprintf("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 = Untranslated(strprintf("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(), CLIENT_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/common/sockman.h b/src/common/sockman.h new file mode 100644 index 0000000000000..d96b59491b879 --- /dev/null +++ b/src/common/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_COMMON_SOCKMAN_H +#define BITCOIN_COMMON_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_COMMON_SOCKMAN_H diff --git a/src/net.cpp b/src/net.cpp index 889ea3723c408..e6adcdad5c08f 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1703,10 +1703,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(); @@ -1726,7 +1726,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); } @@ -2002,8 +2005,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) { @@ -2154,13 +2157,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); } } } @@ -3048,75 +3051,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 = Untranslated(strprintf("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 = Untranslated(strprintf("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 = Untranslated(strprintf("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 = Untranslated(strprintf("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 = Untranslated(strprintf("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(), CLIENT_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) @@ -3179,13 +3113,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); } @@ -3418,7 +3354,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 99abf6eb9496e..c841bbe1ca9a6 100644 --- a/src/net.h +++ b/src/net.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -1032,7 +1033,7 @@ class NetEventsInterface ~NetEventsInterface() = default; }; -class CConnman +class CConnman : private SockMan { public: @@ -1257,24 +1258,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); @@ -1284,7 +1271,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 @@ -1414,7 +1401,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; From a0a8a7221889766405027173de06b0299d5e2871 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 17 Sep 2024 15:05:46 +0200 Subject: [PATCH 06/22] 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/common/sockman.cpp | 79 ++++++++++++++++++--------------- src/common/sockman.h | 6 +-- src/net.cpp | 5 ++- test/functional/feature_port.py | 10 ++--- 4 files changed, 56 insertions(+), 44 deletions(-) diff --git a/src/common/sockman.cpp b/src/common/sockman.cpp index 7cc7edb809a8c..dd793ecd90639 100644 --- a/src/common/sockman.cpp +++ b/src/common/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 = Untranslated(strprintf("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 = Untranslated(strprintf("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 = Untranslated(strprintf("Couldn't open socket for incoming connections (socket returned error %s)", NetworkErrorString(WSAGetLastError()))); - LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original); + errmsg = Untranslated(strprintf("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 = Untranslated(strprintf("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 = Untranslated(strprintf("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 = Untranslated(strprintf("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(), CLIENT_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{" ("} + CLIENT_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/common/sockman.h b/src/common/sockman.h index d96b59491b879..0dd72326135ad 100644 --- a/src/common/sockman.h +++ b/src/common/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. diff --git a/src/net.cpp b/src/net.cpp index e6adcdad5c08f..414428884b3f6 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3113,13 +3113,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/test/functional/feature_port.py b/test/functional/feature_port.py index 2746d7d79c1d6..1310fff2bdd06 100755 --- a/test/functional/feature_port.py +++ b/test/functional/feature_port.py @@ -29,23 +29,23 @@ def run_test(self): port2 = p2p_port(self.num_nodes + 5) self.log.info("When starting with -port, bitcoind binds to it and uses port + 1 for an onion bind") - with node.assert_debug_log(expected_msgs=[f'Bound to 0.0.0.0:{port1}', f'Bound to 127.0.0.1:{port1 + 1}']): + with node.assert_debug_log(expected_msgs=[f'Bound to and listening at 0.0.0.0:{port1}', f'Bound to and listening at 127.0.0.1:{port1 + 1}']): self.restart_node(0, extra_args=["-listen", f"-port={port1}"]) self.log.info("When specifying -port multiple times, only the last one is taken") - with node.assert_debug_log(expected_msgs=[f'Bound to 0.0.0.0:{port2}', f'Bound to 127.0.0.1:{port2 + 1}'], unexpected_msgs=[f'Bound to 0.0.0.0:{port1}']): + with node.assert_debug_log(expected_msgs=[f'Bound to and listening at 0.0.0.0:{port2}', f'Bound to and listening at 127.0.0.1:{port2 + 1}'], unexpected_msgs=[f'Bound to and listening at 0.0.0.0:{port1}']): self.restart_node(0, extra_args=["-listen", f"-port={port1}", f"-port={port2}"]) self.log.info("When specifying ports with both -port and -bind, the one from -port is ignored") - with node.assert_debug_log(expected_msgs=[f'Bound to 0.0.0.0:{port2}'], unexpected_msgs=[f'Bound to 0.0.0.0:{port1}']): + with node.assert_debug_log(expected_msgs=[f'Bound to and listening at 0.0.0.0:{port2}'], unexpected_msgs=[f'Bound to and listening at 0.0.0.0:{port1}']): self.restart_node(0, extra_args=["-listen", f"-port={port1}", f"-bind=0.0.0.0:{port2}"]) self.log.info("When -bind specifies no port, the values from -port and -bind are combined") - with self.nodes[0].assert_debug_log(expected_msgs=[f'Bound to 0.0.0.0:{port1}']): + with self.nodes[0].assert_debug_log(expected_msgs=[f'Bound to and listening at 0.0.0.0:{port1}']): self.restart_node(0, extra_args=["-listen", f"-port={port1}", "-bind=0.0.0.0"]) self.log.info("When an onion bind specifies no port, the value from -port, incremented by 1, is taken") - with self.nodes[0].assert_debug_log(expected_msgs=[f'Bound to 127.0.0.1:{port1 + 1}']): + with self.nodes[0].assert_debug_log(expected_msgs=[f'Bound to and listening at 127.0.0.1:{port1 + 1}']): self.restart_node(0, extra_args=["-listen", f"-port={port1}", "-bind=127.0.0.1=onion"]) self.log.info("Invalid values for -port raise errors") From 919d275a5096ec4aae4d67c218198a0f44543dd9 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 26 Aug 2024 13:14:53 +0200 Subject: [PATCH 07/22] 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/common/sockman.cpp | 21 ++++++++++++++++++ src/common/sockman.h | 9 ++++++++ src/net.cpp | 49 ++++++++++++++---------------------------- src/net.h | 3 --- 4 files changed, 46 insertions(+), 36 deletions(-) diff --git a/src/common/sockman.cpp b/src/common/sockman.cpp index dd793ecd90639..610771b90c9c2 100644 --- a/src/common/sockman.cpp +++ b/src/common/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/common/sockman.h b/src/common/sockman.h index 0dd72326135ad..3aaed8df12307 100644 --- a/src/common/sockman.h +++ b/src/common/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. */ diff --git a/src/net.cpp b/src/net.cpp index 414428884b3f6..eab003228a63e 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1703,27 +1703,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)}; @@ -1731,16 +1715,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); { @@ -2163,7 +2137,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); + } } } } @@ -3045,7 +3028,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 c841bbe1ca9a6..5254acbc0ffca 100644 --- a/src/net.h +++ b/src/net.h @@ -1271,18 +1271,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); From 552a0f638211819be7e3f6700862f22eedc6a9a8 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 17 Sep 2024 17:29:07 +0200 Subject: [PATCH 08/22] style: modernize the style of SockMan::AcceptConnection() --- src/common/sockman.cpp | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/common/sockman.cpp b/src/common/sockman.cpp index 610771b90c9c2..87b9e9b9f53dc 100644 --- a/src/common/sockman.cpp +++ b/src/common/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 e0cb576c778f18f646eda2f5238cba74d60fe39a Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 27 Aug 2024 11:25:24 +0200 Subject: [PATCH 09/22] net: move the generation of ids for new nodes from CConnman to SockMan --- src/common/sockman.cpp | 5 +++++ src/common/sockman.h | 15 +++++++++++++++ src/net.cpp | 5 ----- src/net.h | 5 ----- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/common/sockman.cpp b/src/common/sockman.cpp index 87b9e9b9f53dc..35605170958f9 100644 --- a/src/common/sockman.cpp +++ b/src/common/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/common/sockman.h b/src/common/sockman.h index 3aaed8df12307..540ab27a6897f 100644 --- a/src/common/sockman.h +++ b/src/common/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_COMMON_SOCKMAN_H diff --git a/src/net.cpp b/src/net.cpp index eab003228a63e..66c3c7efb8155 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3075,11 +3075,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 5254acbc0ffca..7406a59a4dd9c 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; @@ -1336,8 +1334,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); @@ -1417,7 +1413,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 From 5eb0b66c9a95806ca8c1f17003036a1ebfa6ab8e Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 27 Aug 2024 16:11:35 +0200 Subject: [PATCH 10/22] 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/common/sockman.cpp | 2 ++ src/common/sockman.h | 27 +++++++++++++++++++++++++++ src/net.cpp | 27 ++++++++++++++++++--------- src/net.h | 6 ++++++ 4 files changed, 53 insertions(+), 9 deletions(-) diff --git a/src/common/sockman.cpp b/src/common/sockman.cpp index 35605170958f9..b474986031d00 100644 --- a/src/common/sockman.cpp +++ b/src/common/sockman.cpp @@ -122,3 +122,5 @@ void SockMan::CloseSockets() { m_listen.clear(); } + +void SockMan::EventI2PListen(const CService&, bool) {} diff --git a/src/common/sockman.h b/src/common/sockman.h index 540ab27a6897f..615971463f872 100644 --- a/src/common/sockman.h +++ b/src/common/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. @@ -59,6 +66,26 @@ class SockMan private: + // + // 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); + /** * The id to assign to the next created node. Used to generate ids of nodes. */ diff --git a/src/net.cpp b/src/net.cpp index 66c3c7efb8155..1f36e49880e3b 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2991,13 +2991,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 = [&]() { @@ -3010,18 +3025,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 7406a59a4dd9c..d44b666a741ae 100644 --- a/src/net.h +++ b/src/net.h @@ -1268,6 +1268,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(); /** From 8e2ea1c3440ebc81bf9365590383dd6c71e90a01 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 27 Aug 2024 16:23:31 +0200 Subject: [PATCH 11/22] net: move I2P-accept-incoming code from CConnman to SockMan --- src/CMakeLists.txt | 2 +- src/common/sockman.cpp | 55 +++++++++++++++++++++++++++++++++ src/common/sockman.h | 69 ++++++++++++++++++++++++++++++++++++++++++ src/net.cpp | 69 +++++++++--------------------------------- src/net.h | 31 ++++--------------- 5 files changed, 146 insertions(+), 80 deletions(-) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 40f4ec87e4f7e..a511215dc7886 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -131,6 +131,7 @@ add_library(bitcoin_common STATIC EXCLUDE_FROM_ALL core_write.cpp deploymentinfo.cpp external_signer.cpp + i2p.cpp init/common.cpp kernel/chainparams.cpp key.cpp @@ -209,7 +210,6 @@ add_library(bitcoin_node STATIC EXCLUDE_FROM_ALL headerssync.cpp httprpc.cpp httpserver.cpp - i2p.cpp index/base.cpp index/blockfilterindex.cpp index/coinstatsindex.cpp diff --git a/src/common/sockman.cpp b/src/common/sockman.cpp index b474986031d00..6d1a0ea1ef5a7 100644 --- a/src/common/sockman.cpp +++ b/src/common/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/common/sockman.h b/src/common/sockman.h index 615971463f872..b51de9b68e095 100644 --- a/src/common/sockman.h +++ b/src/common/sockman.h @@ -5,12 +5,16 @@ #ifndef BITCOIN_COMMON_SOCKMAN_H #define BITCOIN_COMMON_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. @@ -59,6 +92,21 @@ class SockMan */ void CloseSockets(); + /** + * 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. */ @@ -70,6 +118,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. @@ -86,10 +144,21 @@ class SockMan */ virtual void EventI2PListen(const CService& addr, bool success); + /** + * 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_COMMON_SOCKMAN_H diff --git a/src/net.cpp b/src/net.cpp index 1f36e49880e3b..573641f7f7245 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1703,9 +1703,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; @@ -2145,7 +2145,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); } } } @@ -3007,42 +3007,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) @@ -3167,12 +3131,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()) { @@ -3218,6 +3176,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 @@ -3243,11 +3210,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); @@ -3301,9 +3263,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 d44b666a741ae..1f0a94f30ab3e 100644 --- a/src/net.h +++ b/src/net.h @@ -1274,18 +1274,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(); @@ -1513,27 +1510,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 From 014ea3b37826f9105541c1fecec386eddd9e5649 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 20 Sep 2024 13:27:28 +0200 Subject: [PATCH 12/22] 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 | 4 + src/test/fuzz/connman.cpp | 5 +- src/test/net_peer_connection_tests.cpp | 10 +- src/test/util/net.h | 6 +- 7 files changed, 107 insertions(+), 91 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 573641f7f7245..17eedc2a32f82 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -331,7 +331,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; } @@ -342,7 +342,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; } @@ -353,7 +353,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; } @@ -369,7 +369,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; } @@ -1666,11 +1666,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, @@ -1693,12 +1693,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; } @@ -1719,7 +1720,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++; } } @@ -1795,7 +1796,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()); @@ -1826,8 +1827,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; @@ -1854,49 +1858,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); } } { @@ -2365,7 +2371,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; } } @@ -2383,7 +2389,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; } @@ -2397,7 +2403,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; } @@ -2568,7 +2574,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++; @@ -2813,7 +2819,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); } @@ -2839,7 +2845,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(); } @@ -2943,7 +2949,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai m_msgproc->InitializeNode(*pnode, m_local_services); { 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()]; @@ -3294,9 +3300,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); } @@ -3424,7 +3430,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++; } @@ -3450,7 +3456,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); @@ -3472,7 +3478,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; @@ -3490,14 +3496,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) @@ -3742,11 +3748,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 1f0a94f30ab3e..668d94814e506 100644 --- a/src/net.h +++ b/src/net.h @@ -44,6 +44,7 @@ #include #include #include +#include #include #include @@ -1134,7 +1135,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); } @@ -1143,7 +1144,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); } @@ -1413,7 +1414,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}; @@ -1599,8 +1600,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 e503a6838276b..f0cec7f6de198 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5078,10 +5078,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 bda07365e0e76..26ab94fa9374e 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -203,6 +203,10 @@ static RPCHelpMan getpeerinfo() std::vector vstats; connman.GetNodeStats(vstats); + 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/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index a62d227da8efb..5d2bdaf98b591 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -64,12 +64,13 @@ FUZZ_TARGET(connman, .init = initialize_connman) connman.Init(options); CNetAddr random_netaddr; - CNode random_node = ConsumeNode(fuzzed_data_provider); + NodeId node_id{0}; + CNode random_node = ConsumeNode(fuzzed_data_provider, node_id++); CSubNet random_subnet; std::string random_string; LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 100) { - CNode& p2p_node{*ConsumeNodeAsUniquePtr(fuzzed_data_provider).release()}; + CNode& p2p_node{*ConsumeNodeAsUniquePtr(fuzzed_data_provider, node_id++).release()}; connman.AddTestNode(p2p_node); } diff --git a/src/test/net_peer_connection_tests.cpp b/src/test/net_peer_connection_tests.cpp index e60ce8b99d360..33e8a7cc07ad9 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 043e317bf080f..10a11b09019d1 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 9fda56c03361492c105585eca866e89dab499c95 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Sat, 21 Sep 2024 10:05:34 +0200 Subject: [PATCH 13/22] 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/common/sockman.cpp | 4 ++++ src/common/sockman.h | 16 +++++++++++++ src/net.cpp | 52 ++++++++++++++++++++++++++++++++++-------- src/net.h | 14 ++++++++++-- 4 files changed, 74 insertions(+), 12 deletions(-) diff --git a/src/common/sockman.cpp b/src/common/sockman.cpp index 6d1a0ea1ef5a7..4d9db32bfd5aa 100644 --- a/src/common/sockman.cpp +++ b/src/common/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/common/sockman.h b/src/common/sockman.h index b51de9b68e095..e030b91dd4905 100644 --- a/src/common/sockman.h +++ b/src/common/sockman.h @@ -133,6 +133,22 @@ class SockMan // alone to use the default implementation from SockMan. // + /** + * SockMan would only call EventReadyToSend() if this returns true. + * Can be used to temporarily 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 temporarily 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. diff --git a/src/net.cpp b/src/net.cpp index 17eedc2a32f82..3e7d9bbfd0e61 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1652,6 +1652,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. @@ -1981,8 +1991,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) { @@ -1990,16 +2029,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); @@ -2159,6 +2190,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 668d94814e506..ef9f9326cc110 100644 --- a/src/net.h +++ b/src/net.h @@ -1290,17 +1290,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. @@ -1413,6 +1421,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; From 361673b59583f368c399d7bd7b3c0552a95f80b9 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Sat, 21 Sep 2024 10:31:53 +0200 Subject: [PATCH 14/22] 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/common/sockman.cpp | 4 ++++ src/common/sockman.h | 17 +++++++++++++++++ src/net.cpp | 29 ++++++++++++++++++++++++++--- src/net.h | 8 +++++++- 4 files changed, 54 insertions(+), 4 deletions(-) diff --git a/src/common/sockman.cpp b/src/common/sockman.cpp index 4d9db32bfd5aa..0a9aa1a72904f 100644 --- a/src/common/sockman.cpp +++ b/src/common/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/common/sockman.h b/src/common/sockman.h index e030b91dd4905..3a7eb91819fc2 100644 --- a/src/common/sockman.h +++ b/src/common/sockman.h @@ -149,6 +149,23 @@ 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, like disconnecting + * nodes due to higher level logic. + * 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. diff --git a/src/net.cpp b/src/net.cpp index 3e7d9bbfd0e61..b1fd4ed56f08b 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2018,6 +2018,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); @@ -2074,6 +2097,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) { @@ -2162,7 +2186,7 @@ void CConnman::SocketHandlerConnected(const std::vector& nodes, } } - if (InactivityCheck(*pnode)) pnode->fDisconnect = true; + EventIOLoopCompletedForNode(pnode->GetId()); } } @@ -2195,8 +2219,7 @@ void CConnman::ThreadSocketHandler() while (!interruptNet) { - DisconnectNodes(); - NotifyNumConnectionsChanged(); + EventIOLoopCompletedForAllPeers(); SocketHandler(); } } diff --git a/src/net.h b/src/net.h index ef9f9326cc110..abeb23bf413c4 100644 --- a/src/net.h +++ b/src/net.h @@ -1296,6 +1296,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. @@ -1317,7 +1323,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. From a30c7a62129662cae71c244115e4b0f11975931c Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Sun, 22 Sep 2024 12:11:42 +0200 Subject: [PATCH 15/22] 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/common/sockman.h | 33 ++++++++++++ src/net.cpp | 126 +++++++++++++++++++++++++++++++------------ src/net.h | 15 +++++- 3 files changed, 138 insertions(+), 36 deletions(-) diff --git a/src/common/sockman.h b/src/common/sockman.h index 3a7eb91819fc2..af089243e9d99 100644 --- a/src/common/sockman.h +++ b/src/common/sockman.h @@ -128,6 +128,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. diff --git a/src/net.cpp b/src/net.cpp index b1fd4ed56f08b..7a43b52e47001 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1573,8 +1573,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) @@ -1649,6 +1651,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}; } @@ -1991,6 +1998,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); @@ -2124,19 +2204,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; } } @@ -2154,23 +2227,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) { @@ -2178,10 +2239,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)); } } } @@ -3770,7 +3828,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 @@ -3793,10 +3850,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 abeb23bf413c4..6f2a59ed5c536 100644 --- a/src/net.h +++ b/src/net.h @@ -1290,6 +1290,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); @@ -1353,7 +1365,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(); From 35db6971ce9c3104215616b8453a867b50768cc5 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 23 Sep 2024 12:50:25 +0200 Subject: [PATCH 16/22] 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/common/sockman.cpp | 90 +++++++++++++++++++++++++++++++++++++ src/common/sockman.h | 56 +++++++++++++++++++++++ src/net.cpp | 100 ++++++++++++----------------------------- src/net.h | 35 +++------------ src/test/util/net.h | 3 +- 5 files changed, 183 insertions(+), 101 deletions(-) diff --git a/src/common/sockman.cpp b/src/common/sockman.cpp index 0a9aa1a72904f..a696787e1b441 100644 --- a/src/common/sockman.cpp +++ b/src/common/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/common/sockman.h b/src/common/sockman.h index af089243e9d99..c81d30a12aefb 100644 --- a/src/common/sockman.h +++ b/src/common/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. @@ -114,6 +150,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}; + // // Pure virtual functions must be implemented by children classes. // @@ -225,6 +267,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_COMMON_SOCKMAN_H diff --git a/src/net.cpp b/src/net.cpp index 7a43b52e47001..9d7c6f34ee947 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -376,23 +376,8 @@ bool CConnman::CheckIncomingNonce(uint64_t nonce) return true; } -/** Get the bind address for a socket as CAddress */ -static 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; -} - 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) { @@ -454,52 +439,29 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo // Connect std::unique_ptr sock; Proxy proxy; - CService 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 = conn.me; - } - } 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. @@ -508,12 +470,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; } @@ -521,18 +490,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, + me, pszDest ? pszDest : "", conn_type, /*inbound_onion=*/false, @@ -545,7 +509,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; } @@ -1823,7 +1787,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: @@ -2442,7 +2405,6 @@ void CConnman::DumpAddresses() void CConnman::ProcessAddrFetch() { - AssertLockNotHeld(m_unused_i2p_sessions_mutex); std::string strDest; { LOCK(m_addr_fetches_mutex); @@ -2562,7 +2524,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 @@ -3003,7 +2964,6 @@ std::vector CConnman::GetAddedNodeInfo(bool include_connected) co void CConnman::ThreadOpenAddedConnections() { - AssertLockNotHeld(m_unused_i2p_sessions_mutex); AssertLockNotHeld(m_reconnections_mutex); while (true) { @@ -3033,7 +2993,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); // @@ -3881,7 +3840,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 6f2a59ed5c536..aed5dfcb5409f 100644 --- a/src/net.h +++ b/src/net.h @@ -42,7 +42,6 @@ #include #include #include -#include #include #include #include @@ -1120,7 +1119,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(); @@ -1205,7 +1204,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; @@ -1264,10 +1263,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()`). @@ -1359,7 +1358,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); @@ -1575,20 +1574,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. */ @@ -1610,13 +1595,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/test/util/net.h b/src/test/util/net.h index 10a11b09019d1..f58e75b3c0722 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 22c04d49a95488d71998e64a369a1d1c58acdf65 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 24 Sep 2024 09:41:47 +0200 Subject: [PATCH 17/22] 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/common/sockman.cpp | 22 +++++++++++++++++++++- src/common/sockman.h | 13 ++++++++++++- src/net.cpp | 37 ++++++++++++------------------------- src/net.h | 4 +++- 4 files changed, 48 insertions(+), 28 deletions(-) diff --git a/src/common/sockman.cpp b/src/common/sockman.cpp index a696787e1b441..ca5292ef858c5 100644 --- a/src/common/sockman.cpp +++ b/src/common/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/common/sockman.h b/src/common/sockman.h index c81d30a12aefb..19bc4929d6133 100644 --- a/src/common/sockman.h +++ b/src/common/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. */ @@ -162,11 +171,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; diff --git a/src/net.cpp b/src/net.cpp index 9d7c6f34ee947..ada29bc725651 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1685,10 +1685,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; @@ -1711,19 +1715,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) @@ -1749,8 +1740,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 @@ -1758,7 +1748,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), @@ -1777,12 +1767,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) @@ -2224,10 +2214,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 aed5dfcb5409f..1d7c2cbf88d91 100644 --- a/src/net.h +++ b/src/net.h @@ -1276,11 +1276,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; From 8d0894849128df7360ded78917da3ca717ef18e3 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 23 Sep 2024 11:03:32 +0200 Subject: [PATCH 18/22] 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/common/sockman.cpp | 243 ++++++++++++++++++++- src/common/sockman.h | 157 ++++++++++++-- src/net.cpp | 280 ++++--------------------- src/net.h | 73 +------ 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, 447 insertions(+), 347 deletions(-) diff --git a/src/common/sockman.cpp b/src/common/sockman.cpp index ca5292ef858c5..fcbb29da87f35 100644 --- a/src/common/sockman.cpp +++ b/src/common/sockman.cpp @@ -10,7 +10,13 @@ #include #include -CService GetBindAddress(const Sock& sock) +#include + +// 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 +110,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 +126,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 +137,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 +208,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 +244,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 +261,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 +276,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, + std::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 +337,17 @@ void SockMan::EventIOLoopCompletedForAllPeers() {} void SockMan::EventI2PListen(const CService&, bool) {} +void SockMan::TestOnlyAddExistentNode(NodeId node_id, std::unique_ptr&& sock) +{ + LOCK(m_connected_mutex); + const auto result{m_connected.emplace(node_id, std::make_shared(std::move(sock)))}; + assert(result.second); +} + 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 +381,152 @@ 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() || + // WaitMany() may as well be a static method, the context of the first Sock in the vector is not relevant. + !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/common/sockman.h b/src/common/sockman.h index 19bc4929d6133..570467d965077 100644 --- a/src/common/sockman.h +++ b/src/common/sockman.h @@ -15,14 +15,13 @@ #include #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. @@ -31,6 +30,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 +98,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 +122,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, + std::span data, + bool will_send_more, + std::string& errmsg) const + EXCLUSIVE_LOCKS_REQUIRED(!m_connected_mutex); + /** * Close all sockets. */ @@ -157,6 +179,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: /** @@ -172,12 +203,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; @@ -263,17 +295,107 @@ class SockMan */ virtual void EventI2PListen(const CService& addr, bool success); + /** + * 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`. */ @@ -292,6 +414,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_COMMON_SOCKMAN_H diff --git a/src/net.cpp b/src/net.cpp index ada29bc725651..257a5be420e96 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -103,10 +103,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] @@ -437,10 +433,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; @@ -458,9 +452,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 @@ -477,9 +469,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()) { @@ -492,7 +482,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, @@ -502,7 +491,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, }); @@ -517,17 +505,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)) { @@ -1537,7 +1514,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); @@ -1565,45 +1542,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; } @@ -1685,8 +1646,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_) { @@ -1712,7 +1672,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. @@ -1720,7 +1680,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. @@ -1728,7 +1688,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) @@ -1736,7 +1696,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; } } @@ -1749,7 +1709,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, @@ -1773,6 +1732,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) @@ -1814,6 +1775,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); @@ -1864,8 +1833,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()]; @@ -1961,7 +1929,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 @@ -1985,7 +1953,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) { @@ -2006,7 +1974,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) @@ -2021,7 +1989,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 @@ -2073,164 +2041,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() { @@ -3238,9 +3048,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; @@ -3338,8 +3145,6 @@ void CConnman::StopThreads() threadOpenAddedConnections.join(); if (threadDNSAddressSeed.joinable()) threadDNSAddressSeed.join(); - if (threadSocketHandler.joinable()) - threadSocketHandler.join(); } void CConnman::StopNodes() @@ -3362,7 +3167,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); } @@ -3680,7 +3485,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, @@ -3691,7 +3495,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}, @@ -3703,8 +3506,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); @@ -3790,13 +3592,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 1d7c2cbf88d91..75a48c2e9f411 100644 --- a/src/net.h +++ b/src/net.h @@ -661,7 +661,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; @@ -677,16 +676,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. */ @@ -878,7 +867,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, @@ -940,8 +928,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); } @@ -970,18 +956,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); }; /** @@ -1277,15 +1251,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. */ @@ -1314,37 +1294,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; @@ -1366,8 +1316,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(); @@ -1542,7 +1492,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/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 9ee7e9c9fe24b..d2e62c7395cd8 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 5d2bdaf98b591..ce5ed6eacbc0e 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -65,13 +65,14 @@ FUZZ_TARGET(connman, .init = initialize_connman) CNetAddr random_netaddr; NodeId node_id{0}; - CNode random_node = ConsumeNode(fuzzed_data_provider, node_id++); + CNode& random_node{*ConsumeNodeAsUniquePtr(fuzzed_data_provider, node_id++).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, node_id++).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 4cec96274e7a2..42b5c7180bb39 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 d608efd87aceb..8d5d65655b415 100644 --- a/src/test/fuzz/p2p_handshake.cpp +++ b/src/test/fuzz/p2p_handshake.cpp @@ -65,7 +65,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 873eb2b1cc9c3..7a922833bde2b 100644 --- a/src/test/fuzz/p2p_headers_presync.cpp +++ b/src/test/fuzz/p2p_headers_presync.cpp @@ -60,7 +60,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 4bd38a1ac684b..e94f5b2b3d79b 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -68,7 +68,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 0688868c02b21..dbb221e6056e5 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -60,7 +60,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 cc73cdff4b795..e2ea4a340770a 100644 --- a/src/test/fuzz/util/net.h +++ b/src/test/fuzz/util/net.h @@ -221,7 +221,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(); @@ -232,7 +231,6 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional(node_id, - sock, address, keyed_net_group, local_host_nonce, @@ -243,7 +241,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 5f0f05c842ad4..b4d898b3eabb5 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 f58e75b3c0722..e8e96138cc9d8 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 b8b042626ea65c2c2b355a6de69cb7c98bd04de3 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 23 Sep 2024 11:05:59 +0200 Subject: [PATCH 19/22] 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/common/sockman.cpp | 118 ++++++++++++++++++++--------------------- src/common/sockman.h | 70 ++++++++++++------------ 2 files changed, 94 insertions(+), 94 deletions(-) diff --git a/src/common/sockman.cpp b/src/common/sockman.cpp index fcbb29da87f35..e9f9907d6d65d 100644 --- a/src/common/sockman.cpp +++ b/src/common/sockman.cpp @@ -217,65 +217,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); @@ -409,6 +350,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/common/sockman.h b/src/common/sockman.h index 570467d965077..ec9b251464360 100644 --- a/src/common/sockman.h +++ b/src/common/sockman.h @@ -107,29 +107,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. @@ -167,18 +144,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: /** @@ -356,6 +321,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 @@ -415,6 +403,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; /** From ebd5b77fe732c5ba4e5b5ae3687efb503e0dfbff Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 15 Jul 2024 13:30:39 +0200 Subject: [PATCH 20/22] Add sv2 SETUP_CONNECTION messages Co-Authored-By: Christopher Coverdale --- src/sv2/messages.h | 151 ++++++++++++++++++++++++++++++++ src/test/CMakeLists.txt | 1 + src/test/sv2_messages_tests.cpp | 100 +++++++++++++++++++++ 3 files changed, 252 insertions(+) create mode 100644 src/test/sv2_messages_tests.cpp diff --git a/src/sv2/messages.h b/src/sv2/messages.h index fbbe68d63d6cf..706e72b06abb6 100644 --- a/src/sv2/messages.h +++ b/src/sv2/messages.h @@ -6,11 +6,22 @@ #define BITCOIN_SV2_MESSAGES_H #include // for CSerializedNetMsg and CNetMessage +#include +#include +#include +#include