Skip to content

fix: purge pending recovered sigs in BanNode #7563

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
PastaPastaPasta wants to merge 1 commit into
base: develop
Choose a base branch
from
+35 −21
Open

fix: purge pending recovered sigs in BanNode #7563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions src/llmq/net_signing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -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?
Expand All @@ -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);
Comment on lines +312 to +316

@thepastaclaw thepastaclaw Aug 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Prevent recovered signatures from being requeued after the eager purge

RemoveNode() only erases entries present while it holds cs_pending; it does not prevent a later VerifyAndProcessRecoveredSig() call for the same NodeId from inserting another entry. ProcessPendingRecoveredSigs() invokes BanNode() on the recovered-signature worker while the message handler can already be processing another QSIGREC. If removal wins the lock first, that in-flight message queues a residual signature afterward. Masternode connections can admit additional messages because their SendMessages() pass—and therefore MaybeDiscourageAndDisconnect()—is limited to the 100 ms cadence in CConnman::ThreadMessageHandler(). With the periodic sweep removed, these residual entries must be drained and BLS-verified after the peer is disconnected, defeating the stated cleanup guarantee. Fence further admission for the NodeId or perform another purge at a durable peer-disconnection/finalization point.

source: ['codex']

@PastaPastaPasta PastaPastaPasta Aug 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. Requeue after RemoveNode — real race, not treating as blocking

Agreed that RemoveNode is a one-shot erase under cs_pending and does not fence later VerifyAndProcessRecoveredSig inserts for the same NodeId. Concurrent msg-thread admission after a worker-path ban can re-queue residual entries.

Why I’m not expanding the PR for it:

  • Memory stays bounded by the fix: bound pending recovered sig queue to prevent remote OOM #7402 caps (MAX_PENDING_RECSIGS_PER_NODE / TOTAL).
  • For normal peers the window is short: score → next SendMessages / MaybeDiscourageAndDisconnect → disconnect → no more ProcessMessages.
  • The old 5s PeerIsBanned sweep was not a durable fence either (m_should_discourage is ~100ms and false again after finalize), so restoring a periodic sweep would not honestly close this.
  • An admission blacklist in CSigningManager would work but duplicates peer lifecycle state inside LLMQ; a proper fence belongs on disconnect/FinalizeNode (or similar) if we want that later, not as a second ban bit next to the queue.

So the PR guarantee is intentional: drop what is already queued at ban time, eagerly. Residual post-ban admission is accepted residual under BanNode-only + caps, not a regression of unbounded backlog.

@thepastaclaw thepastaclaw Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 249d088 — Prevent recovered signatures from being requeued after the eager purge no longer present.

Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.

if (mark_shares_banned && m_shares_manager) {
m_shares_manager->MarkAsBanned(nodeId);
}
}
Expand Down
6 changes: 5 additions & 1 deletion src/llmq/net_signing.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
14 changes: 6 additions & 8 deletions src/llmq/signing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
8 changes: 4 additions & 4 deletions src/llmq/signing.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include <sync.h>
#include <unordered_lru_cache.h>

#include <functional>
#include <memory>
#include <string_view>
#include <unordered_map>
Expand Down Expand Up @@ -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)
Expand Down
11 changes: 11 additions & 0 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve queued recovered signatures on ordinary disconnects

When an ordinary peer sends a valid QSIGREC and disconnects before the signing worker drains its queue (the worker can sleep for 100 ms), FinalizeNode() unconditionally erases that peer's pending signatures even though no ban occurred. The recovered signature is then never verified or processed, potentially delaying ChainLock or InstantSend propagation until another peer relays it; restrict this final purge to peers actually handled by the ban path rather than every disconnect.

AGENTS.md reference: AGENTS.md:L169-L170

Useful? React with 👍 / 👎.

}

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
Expand Down
Loading