From 8614ff46c920c956537bbf4d40ef3cdcaa5507e9 Mon Sep 17 00:00:00 2001 From: merge-script Date: Tue, 16 Jul 2024 17:31:59 +0100 Subject: [PATCH] Merge bitcoin/bitcoin#30435: init: change shutdown order of load block thread and scheduler 5fd48360198d2ac49e43b24cc1469557b03567b8 init: change shutdown order of load block thread and scheduler (Martin Zumsande) Pull request description: This avoids situations during a reindex, in which the shutdown doesn't finish since `LimitValidationInterfaceQueue()` is called by the load block thread when the scheduler is already stopped, in which case it would block indefinitely. This can lead to intermittent failures in `feature_reindex.py` (#30424), which I could locally reproduce with ```diff diff --git a/src/validation.cpp b/src/validation.cpp index 74f0e4975c..be1706fdaf 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3446,6 +3446,7 @@ static void LimitValidationInterfaceQueue(ValidationSignals& signals) LOCKS_EXCL AssertLockNotHeld(cs_main); if (signals.CallbacksPending() > 10) { + std::this_thread::sleep_for(std::chrono::milliseconds(50)); signals.SyncWithValidationInterfaceQueue(); } } ``` It has also been reported by users running `reindex-chainstate` (#23234). I thought for a bit about potential downsides of changing this order, but couldn't find any. Fixes #30424 Fixes #23234 ACKs for top commit: maflcko: review ACK 5fd48360198d2ac49e43b24cc1469557b03567b8 hebasto: re-ACK 5fd48360198d2ac49e43b24cc1469557b03567b8. tdb3: ACK 5fd48360198d2ac49e43b24cc1469557b03567b8 BrandonOdiwuor: Code Review ACK 5fd48360198d2ac49e43b24cc1469557b03567b8 Tree-SHA512: 3b8894e99551c5d4392b55eaa718eee05841a7287aeef2978699e1d633d5234399fa2f5a3e71eac1508d97845906bd33e0e63e5351855139e7be04c421359b36 --- src/init.cpp | 2 +- src/test/net_peer_connection_tests.cpp | 163 ------------------------- test/functional/feature_filelock.py | 4 +- 3 files changed, 3 insertions(+), 166 deletions(-) delete mode 100644 src/test/net_peer_connection_tests.cpp diff --git a/src/init.cpp b/src/init.cpp index eac326fae8..27addf275a 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -238,9 +238,9 @@ void Shutdown(NodeContext& node) // After everything has been shut down, but before things get flushed, stop the // CScheduler/checkqueue, scheduler and load block thread. + if (node.chainman && node.chainman->m_load_block.joinable()) node.chainman->m_load_block.join(); if (node.scheduler) node.scheduler->stop(); if (node.reverification_scheduler) node.reverification_scheduler->stop(); - if (node.chainman && node.chainman->m_load_block.joinable()) node.chainman->m_load_block.join(); StopScriptCheckWorkerThreads(); // After the threads that potentially access these pointers have been stopped, diff --git a/src/test/net_peer_connection_tests.cpp b/src/test/net_peer_connection_tests.cpp deleted file mode 100644 index 00bc1fdb6a..0000000000 --- a/src/test/net_peer_connection_tests.cpp +++ /dev/null @@ -1,163 +0,0 @@ -// Copyright (c) 2023-present The Bitcoin Core developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. - -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -#include -#include -#include -#include -#include -#include - -#include - -struct LogIPsTestingSetup : public TestingSetup { - LogIPsTestingSetup() - : TestingSetup{ChainType::MAIN, /*extra_args=*/{"-logips"}} {} -}; - -BOOST_FIXTURE_TEST_SUITE(net_peer_connection_tests, LogIPsTestingSetup) - -static CService ip(uint32_t i) -{ - struct in_addr s; - s.s_addr = i; - return CService{CNetAddr{s}, Params().GetDefaultPort()}; -} - -/** Create a peer and connect to it. If the optional `address` (IP/CJDNS only) isn't passed, a random address is created. */ -static void AddPeer(NodeId& id, std::vector& nodes, PeerManager& peerman, ConnmanTestMsg& connman, ConnectionType conn_type, bool onion_peer = false, std::optional address = std::nullopt) -{ - CAddress addr{}; - - if (address.has_value()) { - addr = CAddress{MaybeFlipIPv6toCJDNS(LookupNumeric(address.value(), Params().GetDefaultPort())), NODE_NONE}; - } else if (onion_peer) { - auto tor_addr{g_insecure_rand_ctx.randbytes(ADDR_TORV3_SIZE)}; - BOOST_REQUIRE(addr.SetSpecial(OnionToString(tor_addr))); - } - - while (!addr.IsLocal() && !addr.IsRoutable()) { - addr = CAddress{ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE}; - } - - BOOST_REQUIRE(addr.IsValid()); - - const bool inbound_onion{onion_peer && conn_type == ConnectionType::INBOUND}; - - nodes.emplace_back(new CNode{++id, - /*sock=*/nullptr, - addr, - /*nKeyedNetGroupIn=*/0, - /*nLocalHostNonceIn=*/0, - CAddress{}, - /*addrNameIn=*/"", - conn_type, - /*inbound_onion=*/inbound_onion}); - CNode& node = *nodes.back(); - node.SetCommonVersion(PROTOCOL_VERSION); - - peerman.InitializeNode(node, ServiceFlags(NODE_NETWORK | NODE_WITNESS)); - node.fSuccessfullyConnected = true; - - connman.AddTestNode(node); -} - -BOOST_AUTO_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection) -{ - auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman, Params()); - auto peerman = PeerManager::make(*connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, {}); - NodeId id{0}; - std::vector nodes; - - // Connect a localhost peer. - { - ASSERT_DEBUG_LOG("Added connection to 127.0.0.1:8333 peer=1"); - AddPeer(id, nodes, *peerman, *connman, ConnectionType::MANUAL, /*onion_peer=*/false, /*address=*/"127.0.0.1"); - BOOST_REQUIRE(nodes.back() != nullptr); - } - - // Call ConnectNode(), which is also called by RPC addnode onetry, for a localhost - // address that resolves to multiple IPs, including that of the connected peer. - // The connection attempt should consistently fail due to the check in ConnectNode(). - for (int i = 0; i < 10; ++i) { - ASSERT_DEBUG_LOG("Not opening a connection to localhost, already connected to 127.0.0.1:8333"); - BOOST_CHECK(!connman->ConnectNodePublic(*peerman, "localhost", ConnectionType::MANUAL)); - } - - // Add 3 more peer connections. - AddPeer(id, nodes, *peerman, *connman, ConnectionType::OUTBOUND_FULL_RELAY); - AddPeer(id, nodes, *peerman, *connman, ConnectionType::BLOCK_RELAY, /*onion_peer=*/true); - AddPeer(id, nodes, *peerman, *connman, ConnectionType::INBOUND); - - // Add a CJDNS peer connection. - AddPeer(id, nodes, *peerman, *connman, ConnectionType::INBOUND, /*onion_peer=*/false, - /*address=*/"[fc00:3344:5566:7788:9900:aabb:ccdd:eeff]:1234"); - BOOST_CHECK(nodes.back()->IsInboundConn()); - BOOST_CHECK_EQUAL(nodes.back()->ConnectedThroughNetwork(), Network::NET_CJDNS); - - BOOST_TEST_MESSAGE("Call AddNode() for all the peers"); - for (auto 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("\nCall AddNode() with 2 addrs resolving to existing localhost addnode entry; neither should be added"); - BOOST_CHECK(!connman->AddNode({/*m_added_node=*/"127.0.0.1", /*m_use_v2transport=*/true})); - // OpenBSD doesn't support the IPv4 shorthand notation with omitted zero-bytes. -#if !defined(__OpenBSD__) - BOOST_CHECK(!connman->AddNode({/*m_added_node=*/"127.1", /*m_use_v2transport=*/true})); -#endif - - BOOST_TEST_MESSAGE("\nExpect GetAddedNodeInfo to return expected number of peers with `include_connected` true/false"); - BOOST_CHECK_EQUAL(connman->GetAddedNodeInfo(/*include_connected=*/true).size(), nodes.size()); - BOOST_CHECK(connman->GetAddedNodeInfo(/*include_connected=*/false).empty()); - - // Test AddedNodesContain() - for (auto node : connman->TestNodes()) { - BOOST_CHECK(connman->AddedNodesContain(node->addr)); - } - AddPeer(id, nodes, *peerman, *connman, ConnectionType::OUTBOUND_FULL_RELAY); - BOOST_CHECK(!connman->AddedNodesContain(nodes.back()->addr)); - - BOOST_TEST_MESSAGE("\nPrint GetAddedNodeInfo contents:"); - for (const auto& info : connman->GetAddedNodeInfo(/*include_connected=*/true)) { - BOOST_TEST_MESSAGE(strprintf("\nadded node: %s", info.m_params.m_added_node)); - BOOST_TEST_MESSAGE(strprintf("connected: %s", info.fConnected)); - if (info.fConnected) { - BOOST_TEST_MESSAGE(strprintf("IP address: %s", info.resolvedAddress.ToStringAddrPort())); - BOOST_TEST_MESSAGE(strprintf("direction: %s", info.fInbound ? "inbound" : "outbound")); - } - } - - BOOST_TEST_MESSAGE("\nCheck that all connected peers are correctly detected as connected"); - for (auto node : connman->TestNodes()) { - BOOST_CHECK(connman->AlreadyConnectedPublic(node->addr)); - } - - // Clean up - for (auto node : connman->TestNodes()) { - peerman->FinalizeNode(*node); - } - connman->ClearTestNodes(); -} - -BOOST_AUTO_TEST_SUITE_END() diff --git a/test/functional/feature_filelock.py b/test/functional/feature_filelock.py index 82c27d6aa7..9e3939f101 100755 --- a/test/functional/feature_filelock.py +++ b/test/functional/feature_filelock.py @@ -27,8 +27,8 @@ class FilelockTest(BitcoinTestFramework): self.log.info("Check that we can't start a second bitcoind instance using the same datadir") expected_msg = f"Error: Cannot obtain a lock on data directory {datadir}. {self.config['environment']['PACKAGE_NAME']} is probably already running." self.nodes[1].assert_start_raises_init_error(extra_args=[f'-datadir={self.nodes[0].datadir}', '-noserver'], expected_msg=expected_msg) - cookie_file = datadir / ".cookie" - assert cookie_file.exists() # should not be deleted during the second bitcoind instance shutdown + cookie_file = datadir + "/.cookie" + assert os.path.isfile(cookie_file) # should not be deleted during the second bitcoind instance shutdown if self.is_wallet_compiled(): def check_wallet_filelock(descriptors): wallet_name = ''.join([random.choice(string.ascii_lowercase) for _ in range(6)])