From 95e160d20ffb5c528b830c941f6afd0779e1db00 Mon Sep 17 00:00:00 2001 From: pasta Date: Thu, 2 Jul 2026 10:08:37 -0500 Subject: [PATCH 1/2] fix: bound pending recovered sig queue to prevent remote OOM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QSIGREC messages were deserialized and appended to CSigningManager::pendingRecoveredSigs after only cheap gating checks (quorum exists/active, dedup by hash). Verification is deferred to a single worker thread draining ~32 unique sign-hash sessions per cycle, while ingestion is unbounded and crypto-free on the network threads. The queue had no size cap and was never purged for banned/disconnected peers (RemoveBannedNodeStates only cleans the separate sig-shares subsystem), so a peer could enqueue faster than the queue drains and grow memory until the node is OOM-killed — a remote, unauthenticated DoS, worst against masternodes. Bound the queue with per-node (1000) and global (10000) caps, dropping over-cap messages silently (an honest peer can legitimately outrun the single-threaded drain during a burst, so no misbehaviour is scored). Add CSigningManager::RemoveNodesIf to purge banned peers' queues, invoked from WorkThreadSigning's periodic cleanup. Prune drained (empty) node entries during collection so the global-cap scan stays cheap and disconnected nodes' queues are reclaimed once drained. Co-Authored-By: Claude Opus 4.8 --- src/llmq/net_signing.cpp | 4 ++++ src/llmq/signing.cpp | 42 ++++++++++++++++++++++++++++++++++++++++ src/llmq/signing.h | 13 +++++++++++++ 3 files changed, 59 insertions(+) diff --git a/src/llmq/net_signing.cpp b/src/llmq/net_signing.cpp index d433072b4a13..b8f937b634bb 100644 --- a/src/llmq/net_signing.cpp +++ b/src/llmq/net_signing.cpp @@ -257,6 +257,10 @@ 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? diff --git a/src/llmq/signing.cpp b/src/llmq/signing.cpp index e44411f01d12..f5003f144ae6 100644 --- a/src/llmq/signing.cpp +++ b/src/llmq/signing.cpp @@ -394,9 +394,40 @@ void CSigningManager::VerifyAndProcessRecoveredSig(NodeId from, std::shared_ptr< return; } + // Backpressure: bound the pending queue so a peer cannot enqueue faster than we drain and + // exhaust memory. Drop silently (no misbehaviour) — verification is deferred to a single + // worker thread, so an honest peer can legitimately outrun the drain during a burst. + size_t total_pending{0}; + for (const auto& node_entry : pendingRecoveredSigs) { + total_pending += node_entry.second.size(); + } + if (total_pending >= MAX_PENDING_RECSIGS_TOTAL) { + LogPrint(BCLog::LLMQ, "CSigningManager::%s -- global pending recovered sigs cap reached (%d), dropping sig from node=%d\n", + __func__, MAX_PENDING_RECSIGS_TOTAL, from); + return; + } + if (auto it = pendingRecoveredSigs.find(from); + it != pendingRecoveredSigs.end() && it->second.size() >= MAX_PENDING_RECSIGS_PER_NODE) { + LogPrint(BCLog::LLMQ, "CSigningManager::%s -- per-node pending recovered sigs cap reached (%d), dropping sig from node=%d\n", + __func__, MAX_PENDING_RECSIGS_PER_NODE, from); + return; + } + pendingRecoveredSigs[from].emplace_back(std::move(recoveredSig)); } +void CSigningManager::RemoveNodesIf(const std::function& predicate) +{ + LOCK(cs_pending); + for (auto it = pendingRecoveredSigs.begin(); it != pendingRecoveredSigs.end();) { + if (predicate(it->first)) { + it = pendingRecoveredSigs.erase(it); + } else { + ++it; + } + } +} + bool CSigningManager::CollectPendingRecoveredSigsToVerify( size_t maxUniqueSessions, std::unordered_map>>& retSigShares, std::unordered_map, CBLSPublicKey, StaticSaltedHasher>& ret_pubkeys) @@ -428,6 +459,17 @@ bool CSigningManager::CollectPendingRecoveredSigsToVerify( return !ns.empty(); }, rnd); + // Prune drained (now-empty) node entries so the map only holds nodes with pending sigs. + // This keeps VerifyAndProcessRecoveredSig's global-cap scan cheap and reclaims the queues + // of disconnected nodes once drained, without waiting for them to be banned. + for (auto it = pendingRecoveredSigs.begin(); it != pendingRecoveredSigs.end();) { + if (it->second.empty()) { + it = pendingRecoveredSigs.erase(it); + } else { + ++it; + } + } + if (retSigShares.empty()) { return false; } diff --git a/src/llmq/signing.h b/src/llmq/signing.h index 0d9cc71bf7a4..b4c88a2e3bbd 100644 --- a/src/llmq/signing.h +++ b/src/llmq/signing.h @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -159,6 +160,15 @@ class CRecoveredSigsListener [[nodiscard]] virtual RecoveredSigResult HandleNewRecoveredSig(const CRecoveredSig& recoveredSig) = 0; }; +// Backpressure bounds for the pending (not-yet-verified) recovered sig queue. Verification of an +// incoming recovered sig is deferred to a single worker thread doing batched BLS verification, +// while ingestion happens on the network threads without any crypto cost. Without a bound, a peer +// (or several) can enqueue faster than the queue drains, growing memory without limit. These caps +// bound the queue; over-cap messages are dropped silently (no misbehaviour) since an honest peer +// can legitimately relay recovered sigs faster than we drain them during a burst. +static constexpr size_t MAX_PENDING_RECSIGS_PER_NODE{1000}; +static constexpr size_t MAX_PENDING_RECSIGS_TOTAL{10000}; + class CSigningManager { private: @@ -207,6 +217,9 @@ 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); [[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) From 5c10539a7626e1f2e7c2ce77b7a7d872deee35c4 Mon Sep 17 00:00:00 2001 From: pasta Date: Fri, 3 Jul 2026 10:14:49 -0500 Subject: [PATCH 2/2] perf: avoid O(n) map scan on every recovered sig in pending-queue cap check Maintain a running pendingRecoveredSigsCount alongside pendingRecoveredSigs instead of summing list sizes across the whole map on every incoming QSIGREC message. The scan added by the previous commit runs under cs_pending on the network thread and gets more expensive as the number of distinct peers with pending entries grows, exactly during the flood scenario the cap defends against. --- src/llmq/signing.cpp | 13 +++++++------ src/llmq/signing.h | 3 +++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/llmq/signing.cpp b/src/llmq/signing.cpp index f5003f144ae6..efdcd13cd747 100644 --- a/src/llmq/signing.cpp +++ b/src/llmq/signing.cpp @@ -397,11 +397,7 @@ void CSigningManager::VerifyAndProcessRecoveredSig(NodeId from, std::shared_ptr< // Backpressure: bound the pending queue so a peer cannot enqueue faster than we drain and // exhaust memory. Drop silently (no misbehaviour) — verification is deferred to a single // worker thread, so an honest peer can legitimately outrun the drain during a burst. - size_t total_pending{0}; - for (const auto& node_entry : pendingRecoveredSigs) { - total_pending += node_entry.second.size(); - } - if (total_pending >= MAX_PENDING_RECSIGS_TOTAL) { + if (pendingRecoveredSigsCount >= MAX_PENDING_RECSIGS_TOTAL) { LogPrint(BCLog::LLMQ, "CSigningManager::%s -- global pending recovered sigs cap reached (%d), dropping sig from node=%d\n", __func__, MAX_PENDING_RECSIGS_TOTAL, from); return; @@ -414,6 +410,7 @@ void CSigningManager::VerifyAndProcessRecoveredSig(NodeId from, std::shared_ptr< } pendingRecoveredSigs[from].emplace_back(std::move(recoveredSig)); + ++pendingRecoveredSigsCount; } void CSigningManager::RemoveNodesIf(const std::function& predicate) @@ -421,6 +418,7 @@ void CSigningManager::RemoveNodesIf(const std::function& predicate 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; @@ -442,6 +440,7 @@ bool CSigningManager::CollectPendingRecoveredSigsToVerify( // TODO: refactor it to remove duplicated code with `CSigSharesManager::CollectPendingSigSharesToVerify` std::unordered_set, StaticSaltedHasher> uniqueSignHashes; + size_t erasedCount{0}; IterateNodesRandom(pendingRecoveredSigs, [&]() { return uniqueSignHashes.size() < maxUniqueSessions; }, [&](NodeId nodeId, std::list>& ns) { @@ -456,11 +455,13 @@ bool CSigningManager::CollectPendingRecoveredSigsToVerify( retSigShares[nodeId].emplace_back(recSig); } ns.erase(ns.begin()); + ++erasedCount; return !ns.empty(); }, rnd); + pendingRecoveredSigsCount -= erasedCount; // Prune drained (now-empty) node entries so the map only holds nodes with pending sigs. - // This keeps VerifyAndProcessRecoveredSig's global-cap scan cheap and reclaims the queues + // This keeps VerifyAndProcessRecoveredSig's global-cap check cheap and reclaims the queues // of disconnected nodes once drained, without waiting for them to be banned. for (auto it = pendingRecoveredSigs.begin(); it != pendingRecoveredSigs.end();) { if (it->second.empty()) { diff --git a/src/llmq/signing.h b/src/llmq/signing.h index b4c88a2e3bbd..d91926edf6f2 100644 --- a/src/llmq/signing.h +++ b/src/llmq/signing.h @@ -179,6 +179,9 @@ class CSigningManager mutable Mutex cs_pending; // Incoming and not verified yet std::unordered_map>> pendingRecoveredSigs GUARDED_BY(cs_pending); + // Running total of entries across all of pendingRecoveredSigs, kept in sync with it so the + // MAX_PENDING_RECSIGS_TOTAL check doesn't need to rescan the map on every incoming message. + size_t pendingRecoveredSigsCount GUARDED_BY(cs_pending){0}; Uint256HashMap> pendingReconstructedRecoveredSigs GUARDED_BY(cs_pending); FastRandomContext rnd GUARDED_BY(cs_pending);