fix(net): clean recovered sig backlog after peer disconnect

src/llmq/net_signing.cpp

@@ -288,10 +288,8 @@ 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); });
+            // PeerIsBanned() is transient. Finalized node ids are never reused, so disconnection remains observable.
+            m_sig_manager.RemoveNodesIf([this](NodeId node_id) { return !m_peer_manager->PeerIsConnected(node_id); });
         }
 
         // TODO Wakeup when pending signing is needed?

src/llmq/signing.h

@@ -217,8 +217,7 @@ class CSigningManager
         size_t maxUniqueSessions, std::unordered_map<NodeId, std::list<std::shared_ptr<const CRecoveredSig>>>& retSigShares,
         std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CBLSPublicKey, StaticSaltedHasher>& ret_pubkeys)
         EXCLUSIVE_LOCKS_REQUIRED(!cs_pending);
-    // Drop the pending (not-yet-verified) recovered sigs of any node matching the predicate, e.g.
-    // banned peers. Without this, a flooded peer's backlog would persist even after it is banned.
+    // Drop pending (not-yet-verified) recovered sigs for matching node ids.
     void RemoveNodesIf(const std::function<bool(NodeId)>& predicate) EXCLUSIVE_LOCKS_REQUIRED(!cs_pending);
     [[nodiscard]] std::vector<CRecoveredSigsListener*> GetListeners() const EXCLUSIVE_LOCKS_REQUIRED(!cs_listeners);
     // Returns true if recovered sigs should be send to listeners

src/net_processing.cpp

@@ -633,6 +633,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 PeerIsConnected(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);
@@ -6718,6 +6719,11 @@ bool PeerManagerImpl::PeerIsBanned(const NodeId node_id)
     return IsBanned(node_id);
 }
 
+bool PeerManagerImpl::PeerIsConnected(const NodeId node_id)
+{
+    return GetPeerRef(node_id) != nullptr;
+}
+
 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

@@ -82,6 +82,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 exists and has not been finalized. */
+    virtual bool PeerIsConnected(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.

src/test/denialofservice_tests.cpp

@@ -338,8 +338,13 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
     peerLogic->InitializeNode(*nodes[0], NODE_NETWORK);
     nodes[0]->fSuccessfullyConnected = true;
     connman->AddTestNode(*nodes[0]);
+    BOOST_CHECK(peerLogic->PeerIsConnected(nodes[0]->GetId()));
     peerLogic->UnitTestMisbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged
+    BOOST_CHECK(WITH_LOCK(::cs_main, return peerLogic->PeerIsBanned(nodes[0]->GetId())));
+    BOOST_CHECK(peerLogic->PeerIsConnected(nodes[0]->GetId()));
     BOOST_CHECK(peerLogic->SendMessages(nodes[0]));
+    BOOST_CHECK(WITH_LOCK(::cs_main, return !peerLogic->PeerIsBanned(nodes[0]->GetId())));
+    BOOST_CHECK(peerLogic->PeerIsConnected(nodes[0]->GetId()));
     BOOST_CHECK(banman->IsDiscouraged(addr[0]));
     BOOST_CHECK(nodes[0]->fDisconnect);
     BOOST_CHECK(!banman->IsDiscouraged(other_addr)); // Different address, not discouraged
@@ -399,6 +404,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
 
     for (CNode* node : nodes) {
         peerLogic->FinalizeNode(*node);
+        BOOST_CHECK(!peerLogic->PeerIsConnected(node->GetId()));
     }
     connman->ClearTestNodes();
 }