diff options
| -rw-r--r-- | contrib/completions/bash/bitcoin-cli.bash (renamed from contrib/completions/bash/bitcoin-cli.bash-completion) | 0 | ||||
| -rw-r--r-- | contrib/completions/bash/bitcoin-tx.bash (renamed from contrib/completions/bash/bitcoin-tx.bash-completion) | 0 | ||||
| -rw-r--r-- | contrib/completions/bash/bitcoind.bash (renamed from contrib/completions/bash/bitcoind.bash-completion) | 0 | ||||
| -rw-r--r-- | depends/packages/qt.mk | 2 | ||||
| -rw-r--r-- | depends/patches/qt/memory_resource.patch | 49 | ||||
| -rw-r--r-- | src/addrman.cpp | 38 | ||||
| -rw-r--r-- | src/addrman.h | 14 | ||||
| -rw-r--r-- | src/addrman_impl.h | 5 | ||||
| -rw-r--r-- | src/i2p.cpp | 10 | ||||
| -rw-r--r-- | src/i2p.h | 1 | ||||
| -rw-r--r-- | src/net.cpp | 10 | ||||
| -rw-r--r-- | src/netbase.cpp | 4 | ||||
| -rw-r--r-- | src/qt/bitcoingui.cpp | 10 | ||||
| -rw-r--r-- | src/rpc/net.cpp | 70 | ||||
| -rw-r--r-- | src/test/fuzz/rpc.cpp | 1 | ||||
| -rw-r--r-- | src/test/fuzz/util/net.cpp | 5 | ||||
| -rw-r--r-- | src/test/sock_tests.cpp | 30 | ||||
| -rw-r--r-- | src/test/util/net.h | 11 | ||||
| -rw-r--r-- | src/util/sock.cpp | 9 | ||||
| -rw-r--r-- | src/util/sock.h | 37 | ||||
| -rwxr-xr-x | test/functional/rpc_net.py | 112 |
21 files changed, 355 insertions, 63 deletions
diff --git a/contrib/completions/bash/bitcoin-cli.bash-completion b/contrib/completions/bash/bitcoin-cli.bash index 89e01bc09a..89e01bc09a 100644 --- a/contrib/completions/bash/bitcoin-cli.bash-completion +++ b/contrib/completions/bash/bitcoin-cli.bash diff --git a/contrib/completions/bash/bitcoin-tx.bash-completion b/contrib/completions/bash/bitcoin-tx.bash index 51a9fe31cb..51a9fe31cb 100644 --- a/contrib/completions/bash/bitcoin-tx.bash-completion +++ b/contrib/completions/bash/bitcoin-tx.bash diff --git a/contrib/completions/bash/bitcoind.bash-completion b/contrib/completions/bash/bitcoind.bash index c11d99ef31..c11d99ef31 100644 --- a/contrib/completions/bash/bitcoind.bash-completion +++ b/contrib/completions/bash/bitcoind.bash diff --git a/depends/packages/qt.mk b/depends/packages/qt.mk index b898bf2713..7b4ee64776 100644 --- a/depends/packages/qt.mk +++ b/depends/packages/qt.mk @@ -23,6 +23,7 @@ $(package)_patches += duplicate_lcqpafonts.patch $(package)_patches += fast_fixed_dtoa_no_optimize.patch $(package)_patches += guix_cross_lib_path.patch $(package)_patches += fix-macos-linker.patch +$(package)_patches += memory_resource.patch $(package)_qttranslations_file_name=qttranslations-$($(package)_suffix) $(package)_qttranslations_sha256_hash=c92af4171397a0ed272330b4fa0669790fcac8d050b07c8b8cc565ebeba6735e @@ -248,6 +249,7 @@ define $(package)_preprocess_cmds patch -p1 -i $($(package)_patch_dir)/qtbase-moc-ignore-gcc-macro.patch && \ patch -p1 -i $($(package)_patch_dir)/fix_montery_include.patch && \ patch -p1 -i $($(package)_patch_dir)/use_android_ndk23.patch && \ + patch -p1 -i $($(package)_patch_dir)/memory_resource.patch && \ patch -p1 -i $($(package)_patch_dir)/rcc_hardcode_timestamp.patch && \ patch -p1 -i $($(package)_patch_dir)/duplicate_lcqpafonts.patch && \ patch -p1 -i $($(package)_patch_dir)/fast_fixed_dtoa_no_optimize.patch && \ diff --git a/depends/patches/qt/memory_resource.patch b/depends/patches/qt/memory_resource.patch new file mode 100644 index 0000000000..e41d68db30 --- /dev/null +++ b/depends/patches/qt/memory_resource.patch @@ -0,0 +1,49 @@ +Fix unusable memory_resource on macos + +See https://bugreports.qt.io/browse/QTBUG-117484 +and https://bugreports.qt.io/browse/QTBUG-114316 + +--- a/qtbase/src/corelib/tools/qduplicatetracker_p.h ++++ b/qtbase/src/corelib/tools/qduplicatetracker_p.h +@@ -52,7 +52,7 @@ + + #include <qglobal.h> + +-#if QT_HAS_INCLUDE(<memory_resource>) && __cplusplus > 201402L ++#ifdef __cpp_lib_memory_resource + # include <unordered_set> + # include <memory_resource> + #else + +--- a/qtbase/src/corelib/global/qcompilerdetection.h ++++ b/qtbase/src/corelib/global/qcompilerdetection.h +@@ -1041,16 +1041,22 @@ + # endif // !_HAS_CONSTEXPR + # endif // !__GLIBCXX__ && !_LIBCPP_VERSION + # endif // Q_OS_QNX +-# if (defined(Q_CC_CLANG) || defined(Q_CC_INTEL)) && defined(Q_OS_MAC) && defined(__GNUC_LIBSTD__) \ +- && ((__GNUC_LIBSTD__-0) * 100 + __GNUC_LIBSTD_MINOR__-0 <= 402) ++# if (defined(Q_CC_CLANG) || defined(Q_CC_INTEL)) && defined(Q_OS_MAC) ++# if defined(__GNUC_LIBSTD__) && ((__GNUC_LIBSTD__-0) * 100 + __GNUC_LIBSTD_MINOR__-0 <= 402) + // Apple has not updated libstdc++ since 2007, which means it does not have + // <initializer_list> or std::move. Let's disable these features +-# undef Q_COMPILER_INITIALIZER_LISTS +-# undef Q_COMPILER_RVALUE_REFS +-# undef Q_COMPILER_REF_QUALIFIERS ++# undef Q_COMPILER_INITIALIZER_LISTS ++# undef Q_COMPILER_RVALUE_REFS ++# undef Q_COMPILER_REF_QUALIFIERS + // Also disable <atomic>, since it's clearly not there +-# undef Q_COMPILER_ATOMICS +-# endif ++# undef Q_COMPILER_ATOMICS ++# endif ++# if defined(__cpp_lib_memory_resource) \ ++ && ((defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED < 140000) \ ++ || (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED < 170000)) ++# undef __cpp_lib_memory_resource // Only supported on macOS 14 and iOS 17 ++# endif ++# endif // (defined(Q_CC_CLANG) || defined(Q_CC_INTEL)) && defined(Q_OS_MAC) + # if defined(Q_CC_CLANG) && defined(Q_CC_INTEL) && Q_CC_INTEL >= 1500 + // ICC 15.x and 16.0 have their own implementation of std::atomic, which is activated when in Clang mode + // (probably because libc++'s <atomic> on OS X failed to compile), but they're missing some diff --git a/src/addrman.cpp b/src/addrman.cpp index 212baab9d4..6ce9c81c63 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -838,6 +838,30 @@ std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct return addresses; } +std::vector<std::pair<AddrInfo, AddressPosition>> AddrManImpl::GetEntries_(bool from_tried) const +{ + AssertLockHeld(cs); + + const int bucket_count = from_tried ? ADDRMAN_TRIED_BUCKET_COUNT : ADDRMAN_NEW_BUCKET_COUNT; + std::vector<std::pair<AddrInfo, AddressPosition>> infos; + for (int bucket = 0; bucket < bucket_count; ++bucket) { + for (int position = 0; position < ADDRMAN_BUCKET_SIZE; ++position) { + int id = GetEntry(from_tried, bucket, position); + if (id >= 0) { + AddrInfo info = mapInfo.at(id); + AddressPosition location = AddressPosition( + from_tried, + /*multiplicity_in=*/from_tried ? 1 : info.nRefCount, + bucket, + position); + infos.push_back(std::make_pair(info, location)); + } + } + } + + return infos; +} + void AddrManImpl::Connected_(const CService& addr, NodeSeconds time) { AssertLockHeld(cs); @@ -1199,6 +1223,15 @@ std::vector<CAddress> AddrManImpl::GetAddr(size_t max_addresses, size_t max_pct, return addresses; } +std::vector<std::pair<AddrInfo, AddressPosition>> AddrManImpl::GetEntries(bool from_tried) const +{ + LOCK(cs); + Check(); + auto addrInfos = GetEntries_(from_tried); + Check(); + return addrInfos; +} + void AddrManImpl::Connected(const CService& addr, NodeSeconds time) { LOCK(cs); @@ -1289,6 +1322,11 @@ std::vector<CAddress> AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std return m_impl->GetAddr(max_addresses, max_pct, network); } +std::vector<std::pair<AddrInfo, AddressPosition>> AddrMan::GetEntries(bool use_tried) const +{ + return m_impl->GetEntries(use_tried); +} + void AddrMan::Connected(const CService& addr, NodeSeconds time) { m_impl->Connected(addr, time); diff --git a/src/addrman.h b/src/addrman.h index f41687dcff..4d44c943ac 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -25,11 +25,12 @@ public: }; class AddrManImpl; +class AddrInfo; /** Default for -checkaddrman */ static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0}; -/** Test-only struct, capturing info about an address in AddrMan */ +/** Location information for an address in AddrMan */ struct AddressPosition { // Whether the address is in the new or tried table const bool tried; @@ -168,6 +169,17 @@ public: */ std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const; + /** + * Returns an information-location pair for all addresses in the selected addrman table. + * If an address appears multiple times in the new table, an information-location pair + * is returned for each occurence. Addresses only ever appear once in the tried table. + * + * @param[in] from_tried Selects which table to return entries from. + * + * @return A vector consisting of pairs of AddrInfo and AddressPosition. + */ + std::vector<std::pair<AddrInfo, AddressPosition>> GetEntries(bool from_tried) const; + /** We have successfully connected to this peer. Calling this function * updates the CAddress's nTime, which is used in our IsTerrible() * decisions and gossiped to peers. Callers should be careful that updating diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 1cfaca04a3..512f085a21 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -132,6 +132,9 @@ public: std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const EXCLUSIVE_LOCKS_REQUIRED(!cs); + std::vector<std::pair<AddrInfo, AddressPosition>> GetEntries(bool from_tried) const + EXCLUSIVE_LOCKS_REQUIRED(!cs); + void Connected(const CService& addr, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(!cs); @@ -260,6 +263,8 @@ private: std::vector<CAddress> GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::vector<std::pair<AddrInfo, AddressPosition>> GetEntries_(bool from_tried) const EXCLUSIVE_LOCKS_REQUIRED(cs); + void Connected_(const CService& addr, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs); void SetServices_(const CService& addr, ServiceFlags nServices) EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/i2p.cpp b/src/i2p.cpp index f03e375adf..5a3dde54ce 100644 --- a/src/i2p.cpp +++ b/src/i2p.cpp @@ -119,7 +119,6 @@ Session::Session(const fs::path& private_key_file, : m_private_key_file{private_key_file}, m_control_host{control_host}, m_interrupt{interrupt}, - m_control_sock{std::make_unique<Sock>(INVALID_SOCKET)}, m_transient{false} { } @@ -127,7 +126,6 @@ Session::Session(const fs::path& private_key_file, Session::Session(const CService& control_host, CThreadInterrupt* interrupt) : m_control_host{control_host}, m_interrupt{interrupt}, - m_control_sock{std::make_unique<Sock>(INVALID_SOCKET)}, m_transient{true} { } @@ -315,7 +313,7 @@ void Session::CheckControlSock() LOCK(m_mutex); std::string errmsg; - if (!m_control_sock->IsConnected(errmsg)) { + if (m_control_sock && !m_control_sock->IsConnected(errmsg)) { Log("Control socket error: %s", errmsg); Disconnect(); } @@ -364,7 +362,7 @@ Binary Session::MyDestination() const void Session::CreateIfNotCreatedAlready() { std::string errmsg; - if (m_control_sock->IsConnected(errmsg)) { + if (m_control_sock && m_control_sock->IsConnected(errmsg)) { return; } @@ -437,14 +435,14 @@ std::unique_ptr<Sock> Session::StreamAccept() void Session::Disconnect() { - if (m_control_sock->Get() != INVALID_SOCKET) { + if (m_control_sock) { if (m_session_id.empty()) { Log("Destroying incomplete SAM session"); } else { Log("Destroying SAM session %s", m_session_id); } + m_control_sock.reset(); } - m_control_sock = std::make_unique<Sock>(INVALID_SOCKET); m_session_id.clear(); } } // namespace sam @@ -261,6 +261,7 @@ private: * ("SESSION CREATE"). With the established session id we later open * other connections to the SAM service to accept incoming I2P * connections and make outgoing ones. + * If not connected then this unique_ptr will be empty. * See https://geti2p.net/en/docs/api/samv3 */ std::unique_ptr<Sock> m_control_sock GUARDED_BY(m_mutex); diff --git a/src/net.cpp b/src/net.cpp index 6b2ef5f43d..13f4430424 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -429,12 +429,10 @@ static CAddress GetBindAddress(const Sock& sock) CAddress addr_bind; struct sockaddr_storage sockaddr_bind; socklen_t sockaddr_bind_len = sizeof(sockaddr_bind); - if (sock.Get() != INVALID_SOCKET) { - if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) { - addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind); - } else { - LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n"); - } + 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; } diff --git a/src/netbase.cpp b/src/netbase.cpp index a8419217f4..ca1a80d72f 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -514,10 +514,6 @@ bool ConnectSocketDirectly(const CService &addrConnect, const Sock& sock, int nT // Create a sockaddr from the specified service. struct sockaddr_storage sockaddr; socklen_t len = sizeof(sockaddr); - if (sock.Get() == INVALID_SOCKET) { - LogPrintf("Cannot connect to %s: invalid socket\n", addrConnect.ToStringAddrPort()); - return false; - } if (!addrConnect.GetSockAddr((struct sockaddr*)&sockaddr, &len)) { LogPrintf("Cannot connect to %s: unsupported network\n", addrConnect.ToStringAddrPort()); return false; diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index 2862dddb56..8a46d46437 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -239,7 +239,6 @@ BitcoinGUI::~BitcoinGUI() trayIcon->hide(); #ifdef Q_OS_MACOS delete m_app_nap_inhibitor; - delete appMenuBar; MacDockIconHandler::cleanup(); #endif @@ -479,13 +478,7 @@ void BitcoinGUI::createActions() void BitcoinGUI::createMenuBar() { -#ifdef Q_OS_MACOS - // Create a decoupled menu bar on Mac which stays even if the window is closed - appMenuBar = new QMenuBar(); -#else - // Get the main window's menu bar on other platforms appMenuBar = menuBar(); -#endif // Configure the menus QMenu *file = appMenuBar->addMenu(tr("&File")); @@ -876,6 +869,7 @@ void BitcoinGUI::createTrayIconMenu() // Note: On macOS, the Dock icon is used to provide the tray's functionality. MacDockIconHandler* dockIconHandler = MacDockIconHandler::instance(); connect(dockIconHandler, &MacDockIconHandler::dockIconClicked, [this] { + if (m_node.shutdownRequested()) return; // nothing to show, node is shutting down. show(); activateWindow(); }); @@ -887,6 +881,8 @@ void BitcoinGUI::createTrayIconMenu() // See https://bugreports.qt.io/browse/QTBUG-91697 trayIconMenu.get(), &QMenu::aboutToShow, [this, show_hide_action, send_action, receive_action, sign_action, verify_action, options_action, node_window_action, quit_action] { + if (m_node.shutdownRequested()) return; // nothing to do, node is shutting down. + if (show_hide_action) show_hide_action->setText( (!isHidden() && !isMinimized() && !GUIUtil::isObscured(this)) ? tr("&Hide") : diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 8d796b8e9b..96d06b6b9f 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -5,6 +5,7 @@ #include <rpc/server.h> #include <addrman.h> +#include <addrman_impl.h> #include <banman.h> #include <chainparams.h> #include <clientversion.h> @@ -1079,6 +1080,74 @@ static RPCHelpMan getaddrmaninfo() }; } +UniValue AddrmanEntryToJSON(const AddrInfo& info) +{ + UniValue ret(UniValue::VOBJ); + ret.pushKV("address", info.ToStringAddr()); + ret.pushKV("port", info.GetPort()); + ret.pushKV("services", (uint64_t)info.nServices); + ret.pushKV("time", int64_t{TicksSinceEpoch<std::chrono::seconds>(info.nTime)}); + ret.pushKV("network", GetNetworkName(info.GetNetClass())); + ret.pushKV("source", info.source.ToStringAddr()); + ret.pushKV("source_network", GetNetworkName(info.source.GetNetClass())); + return ret; +} + +UniValue AddrmanTableToJSON(const std::vector<std::pair<AddrInfo, AddressPosition>>& tableInfos) +{ + UniValue table(UniValue::VOBJ); + for (const auto& e : tableInfos) { + AddrInfo info = e.first; + AddressPosition location = e.second; + std::ostringstream key; + key << location.bucket << "/" << location.position; + // Address manager tables have unique entries so there is no advantage + // in using UniValue::pushKV, which checks if the key already exists + // in O(N). UniValue::pushKVEnd is used instead which currently is O(1). + table.pushKVEnd(key.str(), AddrmanEntryToJSON(info)); + } + return table; +} + +static RPCHelpMan getrawaddrman() +{ + return RPCHelpMan{"getrawaddrman", + "EXPERIMENTAL warning: this call may be changed in future releases.\n" + "\nReturns information on all address manager entries for the new and tried tables.\n", + {}, + RPCResult{ + RPCResult::Type::OBJ_DYN, "", "", { + {RPCResult::Type::OBJ_DYN, "table", "buckets with addresses in the address manager table ( new, tried )", { + {RPCResult::Type::OBJ, "bucket/position", "the location in the address manager table (<bucket>/<position>)", { + {RPCResult::Type::STR, "address", "The address of the node"}, + {RPCResult::Type::NUM, "port", "The port number of the node"}, + {RPCResult::Type::STR, "network", "The network (" + Join(GetNetworkNames(), ", ") + ") of the address"}, + {RPCResult::Type::NUM, "services", "The services offered by the node"}, + {RPCResult::Type::NUM_TIME, "time", "The " + UNIX_EPOCH_TIME + " when the node was last seen"}, + {RPCResult::Type::STR, "source", "The address that relayed the address to us"}, + {RPCResult::Type::STR, "source_network", "The network (" + Join(GetNetworkNames(), ", ") + ") of the source address"}, + }} + }} + } + }, + RPCExamples{ + HelpExampleCli("getrawaddrman", "") + + HelpExampleRpc("getrawaddrman", "") + }, + [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { + NodeContext& node = EnsureAnyNodeContext(request.context); + if (!node.addrman) { + throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Address manager functionality missing or disabled"); + } + + UniValue ret(UniValue::VOBJ); + ret.pushKV("new", AddrmanTableToJSON(node.addrman->GetEntries(false))); + ret.pushKV("tried", AddrmanTableToJSON(node.addrman->GetEntries(true))); + return ret; + }, + }; +} + void RegisterNetRPCCommands(CRPCTable& t) { static const CRPCCommand commands[]{ @@ -1099,6 +1168,7 @@ void RegisterNetRPCCommands(CRPCTable& t) {"hidden", &addpeeraddress}, {"hidden", &sendmsgtopeer}, {"hidden", &getaddrmaninfo}, + {"hidden", &getrawaddrman}, }; for (const auto& c : commands) { t.appendCommand(c.name, &c); diff --git a/src/test/fuzz/rpc.cpp b/src/test/fuzz/rpc.cpp index 27bb60d6b6..270cab58e2 100644 --- a/src/test/fuzz/rpc.cpp +++ b/src/test/fuzz/rpc.cpp @@ -142,6 +142,7 @@ const std::vector<std::string> RPC_COMMANDS_SAFE_FOR_FUZZING{ "getnodeaddresses", "getpeerinfo", "getprioritisedtransactions", + "getrawaddrman", "getrawmempool", "getrawtransaction", "getrpcinfo", diff --git a/src/test/fuzz/util/net.cpp b/src/test/fuzz/util/net.cpp index 1545e11065..d23e997719 100644 --- a/src/test/fuzz/util/net.cpp +++ b/src/test/fuzz/util/net.cpp @@ -77,9 +77,10 @@ template CNetAddr::SerParams ConsumeDeserializationParams(FuzzedDataProvider&) n template CAddress::SerParams ConsumeDeserializationParams(FuzzedDataProvider&) noexcept; FuzzedSock::FuzzedSock(FuzzedDataProvider& fuzzed_data_provider) - : m_fuzzed_data_provider{fuzzed_data_provider}, m_selectable{fuzzed_data_provider.ConsumeBool()} + : Sock{fuzzed_data_provider.ConsumeIntegralInRange<SOCKET>(INVALID_SOCKET - 1, INVALID_SOCKET)}, + m_fuzzed_data_provider{fuzzed_data_provider}, + m_selectable{fuzzed_data_provider.ConsumeBool()} { - m_socket = fuzzed_data_provider.ConsumeIntegralInRange<SOCKET>(INVALID_SOCKET - 1, INVALID_SOCKET); } FuzzedSock::~FuzzedSock() diff --git a/src/test/sock_tests.cpp b/src/test/sock_tests.cpp index 26ee724bf8..5dd73dc101 100644 --- a/src/test/sock_tests.cpp +++ b/src/test/sock_tests.cpp @@ -38,7 +38,7 @@ BOOST_AUTO_TEST_CASE(constructor_and_destructor) { const SOCKET s = CreateSocket(); Sock* sock = new Sock(s); - BOOST_CHECK_EQUAL(sock->Get(), s); + BOOST_CHECK(*sock == s); BOOST_CHECK(!SocketIsClosed(s)); delete sock; BOOST_CHECK(SocketIsClosed(s)); @@ -51,22 +51,34 @@ BOOST_AUTO_TEST_CASE(move_constructor) Sock* sock2 = new Sock(std::move(*sock1)); delete sock1; BOOST_CHECK(!SocketIsClosed(s)); - BOOST_CHECK_EQUAL(sock2->Get(), s); + BOOST_CHECK(*sock2 == s); delete sock2; BOOST_CHECK(SocketIsClosed(s)); } BOOST_AUTO_TEST_CASE(move_assignment) { - const SOCKET s = CreateSocket(); - Sock* sock1 = new Sock(s); - Sock* sock2 = new Sock(); + const SOCKET s1 = CreateSocket(); + const SOCKET s2 = CreateSocket(); + Sock* sock1 = new Sock(s1); + Sock* sock2 = new Sock(s2); + + BOOST_CHECK(!SocketIsClosed(s1)); + BOOST_CHECK(!SocketIsClosed(s2)); + *sock2 = std::move(*sock1); + BOOST_CHECK(!SocketIsClosed(s1)); + BOOST_CHECK(SocketIsClosed(s2)); + BOOST_CHECK(*sock2 == s1); + delete sock1; - BOOST_CHECK(!SocketIsClosed(s)); - BOOST_CHECK_EQUAL(sock2->Get(), s); + BOOST_CHECK(!SocketIsClosed(s1)); + BOOST_CHECK(SocketIsClosed(s2)); + BOOST_CHECK(*sock2 == s1); + delete sock2; - BOOST_CHECK(SocketIsClosed(s)); + BOOST_CHECK(SocketIsClosed(s1)); + BOOST_CHECK(SocketIsClosed(s2)); } #ifndef WIN32 // Windows does not have socketpair(2). @@ -98,7 +110,7 @@ BOOST_AUTO_TEST_CASE(send_and_receive) SendAndRecvMessage(*sock0, *sock1); Sock* sock0moved = new Sock(std::move(*sock0)); - Sock* sock1moved = new Sock(); + Sock* sock1moved = new Sock(INVALID_SOCKET); *sock1moved = std::move(*sock1); delete sock0; diff --git a/src/test/util/net.h b/src/test/util/net.h index 0d41cf550e..497292542b 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -108,10 +108,10 @@ constexpr auto ALL_NETWORKS = std::array{ class StaticContentsSock : public Sock { public: - explicit StaticContentsSock(const std::string& contents) : m_contents{contents} + explicit StaticContentsSock(const std::string& contents) + : Sock{INVALID_SOCKET}, + m_contents{contents} { - // Just a dummy number that is not INVALID_SOCKET. - m_socket = INVALID_SOCKET - 1; } ~StaticContentsSock() override { m_socket = INVALID_SOCKET; } @@ -194,6 +194,11 @@ public: return true; } + bool IsConnected(std::string&) const override + { + return true; + } + private: const std::string m_contents; mutable size_t m_consumed{0}; diff --git a/src/util/sock.cpp b/src/util/sock.cpp index fd64cae404..d16dc56aa3 100644 --- a/src/util/sock.cpp +++ b/src/util/sock.cpp @@ -24,8 +24,6 @@ static inline bool IOErrorIsPermanent(int err) return err != WSAEAGAIN && err != WSAEINTR && err != WSAEWOULDBLOCK && err != WSAEINPROGRESS; } -Sock::Sock() : m_socket(INVALID_SOCKET) {} - Sock::Sock(SOCKET s) : m_socket(s) {} Sock::Sock(Sock&& other) @@ -44,8 +42,6 @@ Sock& Sock::operator=(Sock&& other) return *this; } -SOCKET Sock::Get() const { return m_socket; } - ssize_t Sock::Send(const void* data, size_t len, int flags) const { return send(m_socket, static_cast<const char*>(data), len, flags); @@ -411,6 +407,11 @@ void Sock::Close() m_socket = INVALID_SOCKET; } +bool Sock::operator==(SOCKET s) const +{ + return m_socket == s; +}; + std::string NetworkErrorString(int err) { #if defined(WIN32) diff --git a/src/util/sock.h b/src/util/sock.h index 6bac2dfd34..d78e01929b 100644 --- a/src/util/sock.h +++ b/src/util/sock.h @@ -21,16 +21,12 @@ static constexpr auto MAX_WAIT_FOR_IO = 1s; /** - * RAII helper class that manages a socket. Mimics `std::unique_ptr`, but instead of a pointer it - * contains a socket and closes it automatically when it goes out of scope. + * RAII helper class that manages a socket and closes it automatically when it goes out of scope. */ class Sock { public: - /** - * Default constructor, creates an empty object that does nothing when destroyed. - */ - Sock(); + Sock() = delete; /** * Take ownership of an existent socket. @@ -63,43 +59,37 @@ public: virtual Sock& operator=(Sock&& other); /** - * Get the value of the contained socket. - * @return socket or INVALID_SOCKET if empty - */ - [[nodiscard]] virtual SOCKET Get() const; - - /** - * send(2) wrapper. Equivalent to `send(this->Get(), data, len, flags);`. Code that uses this + * send(2) wrapper. Equivalent to `send(m_socket, data, len, flags);`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual ssize_t Send(const void* data, size_t len, int flags) const; /** - * recv(2) wrapper. Equivalent to `recv(this->Get(), buf, len, flags);`. Code that uses this + * recv(2) wrapper. Equivalent to `recv(m_socket, buf, len, flags);`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual ssize_t Recv(void* buf, size_t len, int flags) const; /** - * connect(2) wrapper. Equivalent to `connect(this->Get(), addr, addrlen)`. Code that uses this + * connect(2) wrapper. Equivalent to `connect(m_socket, addr, addrlen)`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual int Connect(const sockaddr* addr, socklen_t addr_len) const; /** - * bind(2) wrapper. Equivalent to `bind(this->Get(), addr, addr_len)`. Code that uses this + * bind(2) wrapper. Equivalent to `bind(m_socket, addr, addr_len)`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual int Bind(const sockaddr* addr, socklen_t addr_len) const; /** - * listen(2) wrapper. Equivalent to `listen(this->Get(), backlog)`. Code that uses this + * listen(2) wrapper. Equivalent to `listen(m_socket, backlog)`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual int Listen(int backlog) const; /** - * accept(2) wrapper. Equivalent to `std::make_unique<Sock>(accept(this->Get(), addr, addr_len))`. + * accept(2) wrapper. Equivalent to `std::make_unique<Sock>(accept(m_socket, addr, addr_len))`. * Code that uses this wrapper can be unit tested if this method is overridden by a mock Sock * implementation. * The returned unique_ptr is empty if `accept()` failed in which case errno will be set. @@ -108,7 +98,7 @@ public: /** * getsockopt(2) wrapper. Equivalent to - * `getsockopt(this->Get(), level, opt_name, opt_val, opt_len)`. Code that uses this + * `getsockopt(m_socket, level, opt_name, opt_val, opt_len)`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual int GetSockOpt(int level, @@ -118,7 +108,7 @@ public: /** * setsockopt(2) wrapper. Equivalent to - * `setsockopt(this->Get(), level, opt_name, opt_val, opt_len)`. Code that uses this + * `setsockopt(m_socket, level, opt_name, opt_val, opt_len)`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual int SetSockOpt(int level, @@ -128,7 +118,7 @@ public: /** * getsockname(2) wrapper. Equivalent to - * `getsockname(this->Get(), name, name_len)`. Code that uses this + * `getsockname(m_socket, name, name_len)`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual int GetSockName(sockaddr* name, socklen_t* name_len) const; @@ -266,6 +256,11 @@ public: */ [[nodiscard]] virtual bool IsConnected(std::string& errmsg) const; + /** + * Check if the internal socket is equal to `s`. Use only in tests. + */ + bool operator==(SOCKET s) const; + protected: /** * Contained socket. `INVALID_SOCKET` designates the object is empty. diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index a87944a062..2c7f974d0b 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -12,6 +12,7 @@ from itertools import product import time import test_framework.messages +from test_framework.netutil import ADDRMAN_NEW_BUCKET_COUNT, ADDRMAN_TRIED_BUCKET_COUNT, ADDRMAN_BUCKET_SIZE from test_framework.p2p import ( P2PInterface, P2P_SERVICES, @@ -67,6 +68,7 @@ class NetTest(BitcoinTestFramework): self.test_addpeeraddress() self.test_sendmsgtopeer() self.test_getaddrmaninfo() + self.test_getrawaddrman() def test_connection_count(self): self.log.info("Test getconnectioncount") @@ -388,5 +390,115 @@ class NetTest(BitcoinTestFramework): assert_equal(res[net]["tried"], 0) assert_equal(res[net]["total"], 0) + def test_getrawaddrman(self): + self.log.info("Test getrawaddrman") + node = self.nodes[1] + + self.log.debug("Test that getrawaddrman is a hidden RPC") + # It is hidden from general help, but its detailed help may be called directly. + assert "getrawaddrman" not in node.help() + assert "getrawaddrman" in node.help("getrawaddrman") + + def check_addr_information(result, expected): + """Utility to compare a getrawaddrman result entry with an expected entry""" + assert_equal(result["address"], expected["address"]) + assert_equal(result["port"], expected["port"]) + assert_equal(result["services"], expected["services"]) + assert_equal(result["network"], expected["network"]) + assert_equal(result["source"], expected["source"]) + assert_equal(result["source_network"], expected["source_network"]) + # To avoid failing on slow test runners, use a 10s vspan here. + assert_approx(result["time"], time.time(), vspan=10) + + def check_getrawaddrman_entries(expected): + """Utility to compare a getrawaddrman result with expected addrman contents""" + getrawaddrman = node.getrawaddrman() + getaddrmaninfo = node.getaddrmaninfo() + for (table_name, table_info) in expected.items(): + assert_equal(len(getrawaddrman[table_name]), len(table_info["entries"])) + assert_equal(len(getrawaddrman[table_name]), getaddrmaninfo["all_networks"][table_name]) + + for bucket_position in getrawaddrman[table_name].keys(): + bucket = int(bucket_position.split("/")[0]) + position = int(bucket_position.split("/")[1]) + + # bucket and position only be sanity checked here as the + # test-addrman isn't deterministic + assert 0 <= int(bucket) < table_info["bucket_count"] + assert 0 <= int(position) < ADDRMAN_BUCKET_SIZE + + entry = getrawaddrman[table_name][bucket_position] + expected_entry = list(filter(lambda e: e["address"] == entry["address"], table_info["entries"]))[0] + check_addr_information(entry, expected_entry) + + # we expect one addrman new and tried table entry, which were added in a previous test + expected = { + "new": { + "bucket_count": ADDRMAN_NEW_BUCKET_COUNT, + "entries": [ + { + "address": "2.0.0.0", + "port": 8333, + "services": 9, + "network": "ipv4", + "source": "2.0.0.0", + "source_network": "ipv4", + } + ] + }, + "tried": { + "bucket_count": ADDRMAN_TRIED_BUCKET_COUNT, + "entries": [ + { + "address": "1.2.3.4", + "port": 8333, + "services": 9, + "network": "ipv4", + "source": "1.2.3.4", + "source_network": "ipv4", + } + ] + } + } + + self.log.debug("Test that the getrawaddrman contains information about the addresses added in a previous test") + check_getrawaddrman_entries(expected) + + self.log.debug("Add one new address to each addrman table") + expected["new"]["entries"].append({ + "address": "2803:0:1234:abcd::1", + "services": 9, + "network": "ipv6", + "source": "2803:0:1234:abcd::1", + "source_network": "ipv6", + "port": -1, # set once addpeeraddress is successful + }) + expected["tried"]["entries"].append({ + "address": "nrfj6inpyf73gpkyool35hcmne5zwfmse3jl3aw23vk7chdemalyaqad.onion", + "services": 9, + "network": "onion", + "source": "nrfj6inpyf73gpkyool35hcmne5zwfmse3jl3aw23vk7chdemalyaqad.onion", + "source_network": "onion", + "port": -1, # set once addpeeraddress is successful + }) + + port = 0 + for (table_name, table_info) in expected.items(): + # There's a slight chance that the to-be-added address collides with an already + # present table entry. To avoid this, we increment the port until an address has been + # added. Incrementing the port changes the position in the new table bucket (bucket + # stays the same) and changes both the bucket and the position in the tried table. + while True: + if node.addpeeraddress(address=table_info["entries"][1]["address"], port=port, tried=table_name == "tried")["success"]: + table_info["entries"][1]["port"] = port + self.log.debug(f"Added {table_info['entries'][1]['address']} to {table_name} table") + break + else: + port += 1 + + self.log.debug("Test that the newly added addresses appear in getrawaddrman") + check_getrawaddrman_entries(expected) + + if __name__ == '__main__': NetTest().main() |