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