net: enforce timeout interval for paused send peers #36667

set from false to true?

Ah yes i see how that is imprecise.

Done, thanks.

We already check and set fPauseSend in both SocketSendData() and PushMessage() - if you need to touch again, it may result in smaller diff if you extract it.

<details> <summary>Extract pause-send updates</summary>

diff --git a/src/net.cpp b/src/net.cpp
index 9bc7887a3c..c1c1fb3cc5 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1675,11 +1675,7 @@ std::pair<size_t, bool> CConnman::SocketSendData(CNode& node) const
         }
     }
 
-    const bool pause_send{node.m_send_memusage + node.m_transport->GetSendMemoryUsage() > nSendBufferMaxSize};
-    if (pause_send && !node.fPauseSend) {
-        node.m_pause_send_since = NodeClock::now();
-    }
-    node.fPauseSend = pause_send;
+    node.SetPauseSend(node.m_send_memusage + node.m_transport->GetSendMemoryUsage() > nSendBufferMaxSize);
 
     if (it == node.vSendMsg.end()) {
         assert(node.m_send_memusage == 0);
@@ -4257,10 +4253,7 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
 
         // Update memory usage of send buffer.
         pnode->m_send_memusage += msg.GetMemoryUsage();
-        if (pnode->m_send_memusage + pnode->m_transport->GetSendMemoryUsage() > nSendBufferMaxSize && !pnode->fPauseSend) {
-            pnode->m_pause_send_since = NodeClock::now();
-            pnode->fPauseSend = true;
-        }
+        if (pnode->m_send_memusage + pnode->m_transport->GetSendMemoryUsage() > nSendBufferMaxSize) pnode->SetPauseSend(true);
         // Move message to vSendMsg queue.
         pnode->vSendMsg.push_back(std::move(msg));
 
diff --git a/src/net.h b/src/net.h
index 76b886c610..c30a41d8cb 100644
--- a/src/net.h
+++ b/src/net.h
@@ -755,6 +755,13 @@ public:
     /** When was fPauseSend last set from false to true */
     std::atomic<NodeClock::time_point> m_pause_send_since{NodeClock::epoch};
 
+    /** Store the timestamp before the flag because InactivityCheck reads both without cs_vSend */
+    void SetPauseSend(bool pause_send) EXCLUSIVE_LOCKS_REQUIRED(cs_vSend)
+    {
+        if (pause_send && !fPauseSend) m_pause_send_since = NodeClock::now();
+        fPauseSend = pause_send;
+    }
+
     /** Network key used to prevent fingerprinting our node across networks.
      *  Influenced by the network and the bind address (+ bind port for inbounds) */
     const uint64_t m_network_key;

</details>

In a followup we could unify the boolean and the timestamp - that would keep the state in one atomic.

<details> <summary>Use one atomic for the pause state</summary>

diff --git a/src/net.cpp b/src/net.cpp
index 7667b9116d..13827d456f 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2081,8 +2081,7 @@ bool CConnman::InactivityCheck(const CNode& node, NodeClock::time_point now) con
         return true;
     }
 
-    if (node.fPauseSend) {
-        const auto last_pause{node.m_pause_send_since.load()};
+    if (const auto last_pause{node.m_pause_send_since.load()}; last_pause != NodeClock::epoch) {
         if (now > last_pause + TIMEOUT_INTERVAL) {
             LogDebug(BCLog::NET,
                 "send buffer full timeout: %is, %s", Ticks<std::chrono::seconds>(now - last_pause),
@@ -3261,7 +3260,7 @@ void CConnman::ThreadMessageHandler()
 
                 // Receive messages
                 bool fMoreNodeWork{m_msgproc->ProcessMessages(*pnode, flagInterruptMsgProc)};
-                fMoreWork |= (fMoreNodeWork && !pnode->fPauseSend);
+                fMoreWork |= (fMoreNodeWork && !pnode->IsSendPaused());
                 if (flagInterruptMsgProc)
                     return;
                 // Send messages
diff --git a/src/net.h b/src/net.h
index c30a41d8cb..9605c445ae 100644
--- a/src/net.h
+++ b/src/net.h
@@ -751,15 +751,18 @@ public:
 
     const uint64_t nKeyedNetGroup;
     std::atomic_bool fPauseRecv{false};
-    std::atomic_bool fPauseSend{false};
-    /** When was fPauseSend last set from false to true */
+    /** When sending was paused, or epoch if it is not paused */
     std::atomic<NodeClock::time_point> m_pause_send_since{NodeClock::epoch};
 
-    /** Store the timestamp before the flag because InactivityCheck reads both without cs_vSend */
+    bool IsSendPaused() const { return m_pause_send_since.load() != NodeClock::epoch; }
+
     void SetPauseSend(bool pause_send) EXCLUSIVE_LOCKS_REQUIRED(cs_vSend)
     {
-        if (pause_send && !fPauseSend) m_pause_send_since = NodeClock::now();
-        fPauseSend = pause_send;
+        if (!pause_send) {
+            m_pause_send_since = NodeClock::epoch;
+        } else if (!IsSendPaused()) {
+            m_pause_send_since = NodeClock::now();
+        }
     }
 
     /** Network key used to prevent fingerprinting our node across networks.
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 14be69e227..20b9019155 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2779,7 +2779,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
         if (interruptMsgProc) return;
         // The send buffer provides backpressure. If there's no space in
         // the buffer, pause processing until the next call.
-        if (pfrom.fPauseSend) break;
+        if (pfrom.IsSendPaused()) break;
 
         const CInv &inv = *it++;
 
@@ -2801,7 +2801,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
 
     // Only process one BLOCK item per call, since they're uncommon and can be
     // expensive to process.
-    if (it != peer.m_getdata_requests.end() && !pfrom.fPauseSend) {
+    if (it != peer.m_getdata_requests.end() && !pfrom.IsSendPaused()) {
         const CInv &inv = *it++;
         if (inv.IsGenBlkMsg()) {
             ProcessGetBlockData(pfrom, peer, inv);
@@ -5457,7 +5457,7 @@ bool PeerManagerImpl::ProcessMessages(CNode& node, std::atomic<bool>& interruptM
     }
 
     // Don't bother if send buffer is too full to respond anyway
-    if (node.fPauseSend) return false;
+    if (node.IsSendPaused()) return false;
 
     auto poll_result{node.PollMessage()};
     if (!poll_result) {
diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
index a5e7c9051d..d86efdd068 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -512,7 +512,7 @@ BOOST_FIXTURE_TEST_CASE(orphan_reconsideration_interruptible, TestChain100Setup)
     const auto send_tx{[&](const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) {
         connman.FlushSendBuffer(peer); // Drop messages we sent, the transport is shared with the fake peer.
         BOOST_REQUIRE(connman.ReceiveMsgFrom(peer, NetMsg::Make(NetMsgType::TX, TX_WITH_WITNESS(*tx))));
-        peer.fPauseSend = false;
+        peer.m_pause_send_since = NodeClock::epoch;
         return connman.ProcessMessagesOnce(peer);
     }};
 
diff --git a/src/test/fuzz/cmpctblock.cpp b/src/test/fuzz/cmpctblock.cpp
index fea6229790..cf46e92b31 100644
--- a/src/test/fuzz/cmpctblock.cpp
+++ b/src/test/fuzz/cmpctblock.cpp
@@ -432,7 +432,7 @@ FUZZ_TARGET(cmpctblock, .init = initialize_cmpctblock)
 
         bool more_work{true};
         while (more_work) {
-            random_node.fPauseSend = false;
+            random_node.m_pause_send_since = NodeClock::epoch;
 
             more_work = connman.ProcessMessagesOnce(random_node);
             peerman->SendMessages(random_node);
diff --git a/src/test/fuzz/p2p_handshake.cpp b/src/test/fuzz/p2p_handshake.cpp
index 0ab3e0c248..e4f8a3403c 100644
--- a/src/test/fuzz/p2p_handshake.cpp
+++ b/src/test/fuzz/p2p_handshake.cpp
@@ -103,7 +103,7 @@ FUZZ_TARGET(p2p_handshake, .init = ::initialize)
 
         bool more_work{true};
         while (more_work) {
-            connection.fPauseSend = false;
+            connection.m_pause_send_since = NodeClock::epoch;
 
             try {
                 more_work = connman.ProcessMessagesOnce(connection);
diff --git a/src/test/fuzz/p2p_headers_presync.cpp b/src/test/fuzz/p2p_headers_presync.cpp
index 00c6dbe06e..c5dc134b1c 100644
--- a/src/test/fuzz/p2p_headers_presync.cpp
+++ b/src/test/fuzz/p2p_headers_presync.cpp
@@ -91,7 +91,7 @@ void HeadersSyncSetup::SendMessage(FuzzedDataProvider& fuzzed_data_provider, CSe
 
     connman.FlushSendBuffer(connection);
     (void)connman.ReceiveMsgFrom(connection, std::move(msg));
-    connection.fPauseSend = false;
+    connection.m_pause_send_since = NodeClock::epoch;
     try {
         connman.ProcessMessagesOnce(connection);
     } catch (const std::ios_base::failure&) {
diff --git a/src/test/fuzz/p2p_private_broadcast.cpp b/src/test/fuzz/p2p_private_broadcast.cpp
index 253fd6410a..6a258e84ec 100644
--- a/src/test/fuzz/p2p_private_broadcast.cpp
+++ b/src/test/fuzz/p2p_private_broadcast.cpp
@@ -241,7 +241,7 @@ FUZZ_TARGET(p2p_private_broadcast, .init = ::initialize)
 
             bool more_work{true};
             while (more_work) {
-                p2p_node.fPauseSend = false;
+                p2p_node.m_pause_send_since = NodeClock::epoch;
                 try {
                     more_work = connman.ProcessMessagesOnce(p2p_node);
                 } catch (const std::ios_base::failure&) {
diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
index 9a30fd0719..a6edac4ebe 100644
--- a/src/test/fuzz/process_message.cpp
+++ b/src/test/fuzz/process_message.cpp
@@ -123,7 +123,7 @@ FUZZ_TARGET(process_message, .init = initialize_process_message)
 
     bool more_work{true};
     while (more_work) {
-        p2p_node.fPauseSend = false;
+        p2p_node.m_pause_send_since = NodeClock::epoch;
         try {
             more_work = connman.ProcessMessagesOnce(p2p_node);
         } catch (const std::ios_base::failure&) {
diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp
index d9a9bdcdaa..c14b8ada28 100644
--- a/src/test/fuzz/process_messages.cpp
+++ b/src/test/fuzz/process_messages.cpp
@@ -123,7 +123,7 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages)
 
         bool more_work{true};
         while (more_work) { // Ensure that every message is eventually processed in some way or another
-            random_node.fPauseSend = false;
+            random_node.m_pause_send_since = NodeClock::epoch;
 
             try {
                 more_work = connman.ProcessMessagesOnce(random_node);
diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index 81ff8c09e3..61a233ac94 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -892,7 +892,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
     auto msg_version_receive =
         NetMsg::Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, CAddress::V1_NETWORK(peer_us));
     Assert(connman.ReceiveMsgFrom(peer, std::move(msg_version_receive)));
-    peer.fPauseSend = false;
+    peer.m_pause_send_since = NodeClock::epoch;
     bool more_work{connman.ProcessMessagesOnce(peer)};
     Assert(!more_work);
 
@@ -900,7 +900,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
     connman.FlushSendBuffer(peer); // Drop sent verack message
 
     Assert(connman.ReceiveMsgFrom(peer, NetMsg::Make(NetMsgType::VERACK)));
-    peer.fPauseSend = false;
+    peer.m_pause_send_since = NodeClock::epoch;
     // Will set peer.fSuccessfullyConnected to true (necessary in SendMessages()).
     more_work = connman.ProcessMessagesOnce(peer);
     Assert(!more_work);
@@ -1657,7 +1657,7 @@ BOOST_AUTO_TEST_CASE(paused_send_timeout)
         connman.PushMessage(&peer, NetMsg::Make(NetMsgType::INV, std::vector{CInv{MSG_TX, uint256{id}}}));
     }};
     push_inv(1);
-    BOOST_REQUIRE(peer.fPauseSend);
+    BOOST_REQUIRE(peer.IsSendPaused());
     const auto send_memusage{WITH_LOCK(peer.cs_vSend, return peer.m_send_memusage)};
 
     clock += TIMEOUT_INTERVAL + 1s;
diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp
index de1de042ca..35e5b4ba06 100644
--- a/src/test/util/net.cpp
+++ b/src/test/util/net.cpp
@@ -50,7 +50,7 @@ void ConnmanTestMsg::Handshake(CNode& node,
     };
 
     (void)connman.ReceiveMsgFrom(node, std::move(msg_version));
-    node.fPauseSend = false;
+    node.m_pause_send_since = NodeClock::epoch;
     connman.ProcessMessagesOnce(node);
     peerman.SendMessages(node);
     FlushSendBuffer(node); // Drop the verack message added by SendMessages.
@@ -64,7 +64,7 @@ void ConnmanTestMsg::Handshake(CNode& node,
     if (successfully_connected) {
         CSerializedNetMsg msg_verack{NetMsg::Make(NetMsgType::VERACK)};
         (void)connman.ReceiveMsgFrom(node, std::move(msg_verack));
-        node.fPauseSend = false;
+        node.m_pause_send_since = NodeClock::epoch;
         connman.ProcessMessagesOnce(node);
         peerman.SendMessages(node);
         assert(node.fSuccessfullyConnected == true);

</details>