fix(llmq): purge pending recovered sigs in BanNode

Widen BanNode to drop CSigningManager::pendingRecoveredSigs for the banned node, and route the two raw recsig PeerMisbehaving(100) sites (invalid QSIGREC llmqType, bad BLS batch source) through BanNode so every NetSigning score-100 path purges eagerly like shares already do via MarkAsBanned. Replace the unused-outside-BanNode RemoveNodesIf predicate API with a direct RemoveNode(NodeId), and remove the 5s PeerIsBanned pending-recsig sweep: that predicate was a ~100ms one-shot and never reliably ran. Caps from #7402 remain the bound for peers that disconnect without misbehavior.

The eager purge alone is racy: BanNode can run on a verification worker thread while the message handler is still processing a QSIGREC from the same peer, re-queueing an entry behind the purge. Back it up in FinalizeNode, which runs strictly after the peer's last ProcessMessages call (DeleteNode requires the node refcount to reach zero) and node ids are never reused, so that purge is final. It is gated on misbehavior >= DISCOURAGEMENT_THRESHOLD - BanNode always scores exactly 100 - so peers that disconnect without misbehaving keep their queue and drain as before; their pending sigs may be the only copy we ever receive.

The recovered-sig-only failure paths pass mark_shares_banned=false so they do not set the sticky CSigSharesManager nodeState.banned flag: NoBan/manual peers survive the misbehavior score with the connection intact, and marking them would mute their sig-share traffic for the connection lifetime. MarkAsBanned stays reserved for sig-share protocol failures, as before.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: PastaPastaPasta

src/llmq/net_signing.cpp

@@ -54,7 +54,7 @@ void NetSigning::ProcessMessage(CNode& pfrom, const std::string& msg_type, CData
                                                                                       recoveredSig->GetHash()}));
 
         if (!Params().GetLLMQ(recoveredSig->getLlmqType()).has_value()) {
-            m_peer_manager->PeerMisbehaving(pfrom.GetId(), 100);
+            BanNode(pfrom.GetId(), /*mark_shares_banned=*/false);
             return;
         }
 
@@ -264,7 +264,7 @@ bool NetSigning::ProcessPendingRecoveredSigs()
     for (const auto& [nodeId, v] : recSigsByNode) {
         if (batchVerifier.badSources.count(nodeId)) {
             LogPrint(BCLog::LLMQ, "NetSigning::%s -- invalid recSig from other node, banning peer=%d\n", __func__, nodeId);
-            m_peer_manager->PeerMisbehaving(nodeId, 100);
+            BanNode(nodeId, /*mark_shares_banned=*/false);
             continue;
         }
 
@@ -288,10 +288,6 @@ 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); });
         }
 
         // TODO Wakeup when pending signing is needed?
@@ -308,12 +304,17 @@ void NetSigning::RemoveBannedNodeStates()
     m_shares_manager->RemoveNodesIf([this](NodeId node_id) { return m_peer_manager->PeerIsBanned(node_id); });
 }
 
-void NetSigning::BanNode(NodeId nodeId)
+void NetSigning::BanNode(NodeId nodeId, bool mark_shares_banned)
 {
     if (nodeId == -1) return;
 
     m_peer_manager->PeerMisbehaving(nodeId, 100);
-    if (m_shares_manager) {
+    // Drop any not-yet-verified recovered sigs still queued for this peer so a flood's backlog
+    // does not keep burning the single recsig worker after we have already decided to ban. A
+    // QSIGREC processed concurrently with a ban issued from a worker thread can still re-queue
+    // entries; FinalizeNode() purges those once the peer is gone for good.
+    m_sig_manager.RemoveNode(nodeId);
+    if (mark_shares_banned && m_shares_manager) {
         m_shares_manager->MarkAsBanned(nodeId);
     }
 }

src/llmq/net_signing.h

@@ -72,7 +72,11 @@ class NetSigning final : public NetHandler, public CValidationInterface
         std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>&& quorums);
 
     void RemoveBannedNodeStates();
-    void BanNode(NodeId nodeid);
+    //! Score the peer with 100 misbehavior points and drop its not-yet-verified pending recovered
+    //! sigs. mark_shares_banned additionally suppresses the peer's sig-share channel and must stay
+    //! false for recovered-sig-only failures: NoBan/manual peers survive the misbehavior score, and
+    //! the sticky sig-share ban would otherwise mute a still-connected peer's shares for good.
+    void BanNode(NodeId nodeid, bool mark_shares_banned = true);
 
 private:
     CSigningManager& m_sig_manager;

src/llmq/signing.cpp

@@ -418,17 +418,15 @@ void CSigningManager::VerifyAndProcessRecoveredSig(NodeId from, std::shared_ptr<
     ++pendingRecoveredSigsCount;
 }
 
-void CSigningManager::RemoveNodesIf(const std::function<bool(NodeId)>& predicate)
+void CSigningManager::RemoveNode(NodeId node_id)
 {
     LOCK(cs_pending);
-    for (auto it = pendingRecoveredSigs.begin(); it != pendingRecoveredSigs.end();) {
-        if (predicate(it->first)) {
-            pendingRecoveredSigsCount -= it->second.size();
-            it = pendingRecoveredSigs.erase(it);
-        } else {
-            ++it;
-        }
+    auto it = pendingRecoveredSigs.find(node_id);
+    if (it == pendingRecoveredSigs.end()) {
+        return;
     }
+    pendingRecoveredSigsCount -= it->second.size();
+    pendingRecoveredSigs.erase(it);
 }
 
 bool CSigningManager::CollectPendingRecoveredSigsToVerify(

src/llmq/signing.h

@@ -14,7 +14,6 @@
 #include <sync.h>
 #include <unordered_lru_cache.h>
 
-#include <functional>
 #include <memory>
 #include <string_view>
 #include <unordered_map>
@@ -217,9 +216,10 @@ 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.
-    void RemoveNodesIf(const std::function<bool(NodeId)>& predicate) EXCLUSIVE_LOCKS_REQUIRED(!cs_pending);
+    // Drop pending (not-yet-verified) recovered sigs queued by this peer (called eagerly by
+    // NetSigning::BanNode and finally by PeerManagerImpl::FinalizeNode once a discouraged peer
+    // is gone). Does not touch pendingReconstructedRecoveredSigs (local, node id -1).
+    void RemoveNode(NodeId node_id) 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
     [[nodiscard]] bool ProcessRecoveredSig(const std::shared_ptr<const CRecoveredSig>& recoveredSig)

src/net_processing.cpp

@@ -1798,6 +1798,17 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) {
     }
     } // cs_main
 
+    if (misbehavior >= DISCOURAGEMENT_THRESHOLD) {
+        // Drop the not-yet-verified pending recovered sigs of a peer that crossed the
+        // discouragement threshold. NetSigning::BanNode purges eagerly (and always scores exactly
+        // DISCOURAGEMENT_THRESHOLD), but a QSIGREC processed concurrently with a ban issued from a
+        // verification worker thread can re-queue entries behind the purge. This runs strictly
+        // after the peer's last ProcessMessages call and node ids are never reused, so it is
+        // final. Peers that disconnect without misbehaving keep their queue and drain as before:
+        // their pending sigs may be the only copy we ever receive.
+        m_llmq_ctx.sigman->RemoveNode(nodeid);
+    }
+
     if (node.fSuccessfullyConnected && misbehavior == 0 && !node.IsBlockOnlyConn() && !node.IsInboundConn()) {
         // Only change visible addrman state for full outbound peers.  We don't
         // call Connected() for feeler connections since they don't have