From 2aac78fd5cc8f4144b34b21376134f24bf6484e9 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 12 Aug 2026 15:31:56 -0500 Subject: [PATCH] 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 --- src/llmq/net_signing.cpp | 17 +++++++++-------- src/llmq/net_signing.h | 6 +++++- src/llmq/signing.cpp | 14 ++++++-------- src/llmq/signing.h | 8 ++++---- src/net_processing.cpp | 11 +++++++++++ 5 files changed, 35 insertions(+), 21 deletions(-) diff --git a/src/llmq/net_signing.cpp b/src/llmq/net_signing.cpp index a2ed28b2929c..05afe5bf98fa 100644 --- a/src/llmq/net_signing.cpp +++ b/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); } } diff --git a/src/llmq/net_signing.h b/src/llmq/net_signing.h index 2dcbaf45a7b3..bbf20804c1a5 100644 --- a/src/llmq/net_signing.h +++ b/src/llmq/net_signing.h @@ -72,7 +72,11 @@ class NetSigning final : public NetHandler, public CValidationInterface std::unordered_map, 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; diff --git a/src/llmq/signing.cpp b/src/llmq/signing.cpp index 7c7454eb9d2a..471ef07e4d14 100644 --- a/src/llmq/signing.cpp +++ b/src/llmq/signing.cpp @@ -418,17 +418,15 @@ void CSigningManager::VerifyAndProcessRecoveredSig(NodeId from, std::shared_ptr< ++pendingRecoveredSigsCount; } -void CSigningManager::RemoveNodesIf(const std::function& 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( diff --git a/src/llmq/signing.h b/src/llmq/signing.h index 76b6c4326cb3..2266a2a7869f 100644 --- a/src/llmq/signing.h +++ b/src/llmq/signing.h @@ -14,7 +14,6 @@ #include #include -#include #include #include #include @@ -217,9 +216,10 @@ class CSigningManager size_t maxUniqueSessions, std::unordered_map>>& retSigShares, std::unordered_map, 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& 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 GetListeners() const EXCLUSIVE_LOCKS_REQUIRED(!cs_listeners); // Returns true if recovered sigs should be send to listeners [[nodiscard]] bool ProcessRecoveredSig(const std::shared_ptr& recoveredSig) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0e265c36bb2c..e051efc6487e 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1788,6 +1788,17 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) { } } // cs_main + if (m_llmq_ctx && 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