fix(net): clean up disconnected peer state

src/llmq/net_signing.cpp

@@ -288,10 +288,11 @@ void NetSigning::WorkThreadSigning()
         constexpr auto CLEANUP_INTERVAL{5s};
         if (cleanupThrottler.TryCleanup(CLEANUP_INTERVAL)) {
             m_sig_manager.Cleanup();
-            // Drop pending recovered sigs queued by banned peers so a flood's backlog does not
-            // persist after the peer is banned (RemoveBannedNodeStates only cleans the sig-shares
-            // subsystem, not m_sig_manager's pending recovered sigs).
-            m_sig_manager.RemoveNodesIf([this](NodeId node_id) { return m_peer_manager->PeerIsBanned(node_id); });
+            // Drop pending recovered sigs from disconnected or discouraged peers so their backlog
+            // does not outlive the connection that created it.
+            m_sig_manager.RemoveNodesIf([this](NodeId node_id) {
+                return m_peer_manager->PeerIsDisconnectedOrDiscouraged(node_id);
+            });
         }
 
         // TODO Wakeup when pending signing is needed?
@@ -301,11 +302,12 @@ void NetSigning::WorkThreadSigning()
     }
 }
 
-void NetSigning::RemoveBannedNodeStates()
+void NetSigning::RemoveDisconnectedOrDiscouragedNodeStates()
 {
     assert(m_shares_manager != nullptr);
-    // Called regularly to cleanup local node states for banned nodes
-    m_shares_manager->RemoveNodesIf([this](NodeId node_id) { return m_peer_manager->PeerIsBanned(node_id); });
+    m_shares_manager->RemoveNodesIf([this](NodeId node_id) {
+        return m_peer_manager->PeerIsDisconnectedOrDiscouraged(node_id);
+    });
 }
 
 void NetSigning::BanNode(NodeId nodeId)
@@ -323,7 +325,7 @@ void NetSigning::WorkThreadCleaning()
     assert(m_shares_manager);
 
     while (!workInterrupt) {
-        RemoveBannedNodeStates();
+        RemoveDisconnectedOrDiscouragedNodeStates();
 
         m_shares_manager->SendMessages();
         m_shares_manager->Cleanup();

src/llmq/net_signing.h

@@ -71,7 +71,7 @@ class NetSigning final : public NetHandler, public CValidationInterface
         std::unordered_map<NodeId, std::vector<CSigShare>>&& sigSharesByNodes,
         std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>&& quorums);
 
-    void RemoveBannedNodeStates();
+    void RemoveDisconnectedOrDiscouragedNodeStates();
     void BanNode(NodeId nodeid);
 
 private:

src/net_processing.cpp

@@ -619,7 +619,6 @@ class PeerManagerImpl final : public PeerManager
                         const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) override
         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, g_msgproc_mutex);
     void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) override;
-    bool IsBanned(NodeId pnode) override EXCLUSIVE_LOCKS_REQUIRED(cs_main, !m_peer_mutex);
     size_t GetRequestedObjectCount(NodeId nodeid) const override EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     /** Implements external handlers logic */
@@ -632,7 +631,7 @@ class PeerManagerImpl final : public PeerManager
 
     /** Implement PeerManagerInternal */
     void PeerMisbehaving(const NodeId pnode, const int howmuch, const std::string& message = "") override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
-    bool PeerIsBanned(const NodeId node_id) override EXCLUSIVE_LOCKS_REQUIRED(cs_main, !m_peer_mutex);
+    bool PeerIsDisconnectedOrDiscouraged(const NodeId node_id) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
     bool PeerConsumeObjectRequest(NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
     GetDataResponse PeerConsumeGetDataResponse(NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
@@ -1897,16 +1896,13 @@ void PeerManagerImpl::Misbehaving(Peer& peer, int howmuch, const std::string& me
              peer.m_id, score_before, score_now, warning, message_prefixed);
 }
 
-bool PeerManagerImpl::IsBanned(NodeId pnode)
+bool PeerManagerImpl::PeerIsDisconnectedOrDiscouraged(const NodeId node_id)
 {
-    PeerRef peer = GetPeerRef(pnode);
-    if (peer == nullptr)
-        return false;
+    PeerRef peer = GetPeerRef(node_id);
+    if (peer == nullptr) return true;
+
     LOCK(peer->m_misbehavior_mutex);
-    if (peer->m_should_discourage) {
-        return true;
-    }
-    return false;
+    return peer->m_should_discourage;
 }
 
 bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& state,
@@ -6713,11 +6709,6 @@ void PeerManagerImpl::PeerMisbehaving(const NodeId pnode, const int howmuch, con
     if (peer) Misbehaving(*peer, howmuch, message);
 }
 
-bool PeerManagerImpl::PeerIsBanned(const NodeId node_id)
-{
-    return IsBanned(node_id);
-}
-
 void PeerManagerImpl::PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv)
 {
     // Completing only this peer's announcement is deliberate: an invalid or unusable object must

src/net_processing.h

@@ -81,7 +81,8 @@ class PeerManagerInternal
 {
 public:
     virtual void PeerMisbehaving(const NodeId pnode, const int howmuch, const std::string& message = "") = 0;
-    virtual bool PeerIsBanned(const NodeId node_id) = 0;
+    /** Whether the peer is no longer connected or has crossed the discouragement threshold. */
+    virtual bool PeerIsDisconnectedOrDiscouraged(const NodeId node_id) = 0;
     /** Complete this peer's pending announcement of the inv, so it is not requested from them
      *  again. Announcements of the same inv by other peers are unaffected: an invalid or unusable
      *  object must not stop us from fetching it from honest peers.
@@ -220,8 +221,6 @@ class PeerManager : public CValidationInterface, public NetEventsInterface, publ
     /** This function is used for testing the stale tip eviction logic, see denialofservice_tests.cpp */
     virtual void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) = 0;
 
-    virtual bool IsBanned(NodeId pnode) = 0;
-
     virtual size_t GetRequestedObjectCount(NodeId nodeid) const = 0;
 
     virtual void AddExtraHandler(std::unique_ptr<NetHandler>&& handler) = 0;

src/test/denialofservice_tests.cpp

@@ -338,7 +338,9 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
     peerLogic->InitializeNode(*nodes[0], NODE_NETWORK);
     nodes[0]->fSuccessfullyConnected = true;
     connman->AddTestNode(*nodes[0]);
+    BOOST_CHECK(!peerLogic->PeerIsDisconnectedOrDiscouraged(nodes[0]->GetId()));
     peerLogic->UnitTestMisbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged
+    BOOST_CHECK(peerLogic->PeerIsDisconnectedOrDiscouraged(nodes[0]->GetId()));
     BOOST_CHECK(peerLogic->SendMessages(nodes[0]));
     BOOST_CHECK(banman->IsDiscouraged(addr[0]));
     BOOST_CHECK(nodes[0]->fDisconnect);
@@ -358,6 +360,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
     nodes[1]->fSuccessfullyConnected = true;
     connman->AddTestNode(*nodes[1]);
     peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD - 1);
+    BOOST_CHECK(!peerLogic->PeerIsDisconnectedOrDiscouraged(nodes[1]->GetId()));
     BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
     // [0] is still discouraged/disconnected.
     BOOST_CHECK(banman->IsDiscouraged(addr[0]));
@@ -399,6 +402,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
 
     for (CNode* node : nodes) {
         peerLogic->FinalizeNode(*node);
+        BOOST_CHECK(peerLogic->PeerIsDisconnectedOrDiscouraged(node->GetId()));
     }
     connman->ClearTestNodes();
 }