Merge bitcoin/bitcoin#23218: p2p: Use mocktime for ping timeout

fadf1186c899f45787a91c28120b0608bdc6c246 p2p: Use mocktime for ping timeout (MarcoFalke)

Pull request description:

  It is slightly confusing to use mocktime for some times, but not others.

  Start fixing that by making the ping timeout use mocktime.

  The only downside would be that tests that use mocktime disconnect peers after this patch. However, I don't think this is an issue, as the inactivity check is already disabled for all functional tests after commit 6d76b57ca0cdf6f9c19ce065b9a4a628930a78b5. Only one unit test needed the inactivity check disabled as part of this patch.

  A nice side effect of this patch is that the `p2p_ping` functional test now runs 4 seconds faster.

ACKs for top commit:
  laanwj:
    Code review ACK fadf1186c899f45787a91c28120b0608bdc6c246

Tree-SHA512: e9e7b21040a89d9d574b3038f85a67e6336de6cd6e41aa286769cd03cada6e75a94ec01700e052e56d822ef85d7813cc06bf7e67b81543eff8917a16cdccf942

6 files changed, 25 insertions, 7 deletions

diff --git a/src/net.cpp b/src/net.cpp
index 7271ff22b2..ad558dd598 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1295,9 +1295,8 @@ void CConnman::NotifyNumConnectionsChanged()
     }
 }
 
-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;
 }
 
diff --git a/src/net.h b/src/net.h
index 3f0d4decae..48dfb3043f 100644
--- a/src/net.h
+++ b/src/net.h
@@ -942,7 +942,7 @@ public:
     std::chrono::microseconds PoissonNextSendInbound(std::chrono::microseconds now, std::chrono::seconds average_interval);
 
     /** 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 {
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 66b99aa2bb..22586ba7da 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -4312,8 +4312,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;
diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
index 0bfe6eecd9..668ff150ee 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -52,6 +52,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.chainman, *m_node.mempool, false);
 
diff --git a/src/test/util/net.h b/src/test/util/net.h
index 939ec322ed..d89fc34b75 100644
--- a/src/test/util/net.h
+++ b/src/test/util/net.h
@@ -17,6 +17,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 888e986fba..d67e97acf7 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]
@@ -110,8 +115,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__':