From 022b76f20b0f57c2993a00f6e95f91f84f611f14 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 25 Mar 2024 10:50:27 +0000 Subject: [PATCH] merge bitcoin#23218: Use mocktime for ping timeout --- src/net.cpp | 3 +-- src/net.h | 2 +- src/net_processing.cpp | 5 ++++- src/test/denialofservice_tests.cpp | 2 ++ src/test/util/net.h | 6 ++++++ test/functional/p2p_ping.py | 14 +++++++++++--- 6 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 59e15169e7..78b28003ed 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1491,9 +1491,8 @@ void CConnman::CalculateNumConnectionsChangedStats() statsClient.gauge("peers.torConnections", torNodes, 1.0f); } -bool CConnman::ShouldRunInactivityChecks(const CNode& node, std::optional now_in) const +bool CConnman::ShouldRunInactivityChecks(const CNode& node, int64_t now) const { - const int64_t now = now_in ? now_in.value() : GetTimeSeconds(); return node.nTimeConnected + m_peer_connect_timeout < now; } diff --git a/src/net.h b/src/net.h index 6dc77b830b..acbc3c3ad4 100644 --- a/src/net.h +++ b/src/net.h @@ -1237,7 +1237,7 @@ public: void SetAsmap(std::vector asmap) { addrman.m_asmap = std::move(asmap); } /** Return true if we should disconnect the peer for failing an inactivity check. */ - bool ShouldRunInactivityChecks(const CNode& node, std::optional now=std::nullopt) const; + bool ShouldRunInactivityChecks(const CNode& node, int64_t secs_now) const; private: struct ListenSocket { diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3b9260e3da..41cd78c329 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5017,8 +5017,11 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers() void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::microseconds now) { - if (m_connman.ShouldRunInactivityChecks(node_to) && peer.m_ping_nonce_sent && + if (m_connman.ShouldRunInactivityChecks(node_to, std::chrono::duration_cast(now).count()) && + peer.m_ping_nonce_sent && now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL}) { + // The ping timeout is using mocktime. To disable the check during + // testing, increase -peertimeout. LogPrint(BCLog::NET, "ping timeout: %fs peer=%d\n", 0.000001 * count_microseconds(now - peer.m_ping_start.load()), peer.m_id); node_to.fDisconnect = true; return; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 0ae1ad5c31..1fc2330cf8 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -64,6 +64,8 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) { const CChainParams& chainparams = Params(); auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); + // Disable inactivity checks for this test to avoid interference + static_cast(connman.get())->SetPeerConnectTimeout(99999); auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool, *m_node.govman, *m_node.sporkman, ::deterministicMNManager, m_node.cj_ctx, m_node.llmq_ctx, false); diff --git a/src/test/util/net.h b/src/test/util/net.h index c6cebed8de..74fd43970f 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -15,6 +15,12 @@ struct ConnmanTestMsg : public CConnman { using CConnman::CConnman; + + void SetPeerConnectTimeout(int64_t timeout) + { + m_peer_connect_timeout = timeout; + } + void AddTestNode(CNode& node) { LOCK(cs_vNodes); diff --git a/test/functional/p2p_ping.py b/test/functional/p2p_ping.py index 73ac780741..b43f316faa 100755 --- a/test/functional/p2p_ping.py +++ b/test/functional/p2p_ping.py @@ -30,11 +30,16 @@ class NodeNoPong(P2PInterface): pass +TIMEOUT_INTERVAL = 20 * 60 + + class PingPongTest(BitcoinTestFramework): def set_test_params(self): self.setup_clean_chain = True self.num_nodes = 1 - self.extra_args = [['-peertimeout=3']] + # Set the peer connection timeout low. It does not matter for this + # test, as long as it is less than TIMEOUT_INTERVAL. + self.extra_args = [['-peertimeout=1']] def check_peer_info(self, *, pingtime, minping, pingwait): stats = self.nodes[0].getpeerinfo()[0] @@ -114,8 +119,11 @@ class PingPongTest(BitcoinTestFramework): self.nodes[0].ping() no_pong_node.wait_until(lambda: 'ping' in no_pong_node.last_message) with self.nodes[0].assert_debug_log(['ping timeout: 1201.000000s']): - self.mock_forward(20 * 60 + 1) - time.sleep(4) # peertimeout + 1 + self.mock_forward(TIMEOUT_INTERVAL // 2) + # Check that sending a ping does not prevent the disconnect + no_pong_node.sync_with_ping() + self.mock_forward(TIMEOUT_INTERVAL // 2 + 1) + no_pong_node.wait_for_disconnect() if __name__ == '__main__':