merge bitcoin#23218: Use mocktime for ping timeout

Author: kwvg

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<int64_t> 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;
 }
 

src/net.h

@@ -1237,7 +1237,7 @@ friend class CNode;
     void SetAsmap(std::vector<bool> 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<int64_t> now=std::nullopt) const;
+    bool ShouldRunInactivityChecks(const CNode& node, int64_t secs_now) const;
 
 private:
     struct ListenSocket {

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<std::chrono::seconds>(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;

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<CConnman>(0x1337, 0x1337, *m_node.addrman);
+    // Disable inactivity checks for this test to avoid interference
+    static_cast<ConnmanTestMsg*>(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);

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);

test/functional/p2p_ping.py

@@ -30,11 +30,16 @@ def on_ping(self, message):
         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 @@ def run_test(self):
         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__':