From 315f68377f2ba80ae8450c71479c17cd1881b1b5 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 11 Aug 2026 22:11:21 -0500 Subject: [PATCH] fix(llmq): clean up disconnected peer state --- src/llmq/net_signing.cpp | 18 ++++++++++-------- src/llmq/net_signing.h | 2 +- src/net_processing.cpp | 21 ++++++--------------- src/net_processing.h | 5 ++--- src/test/denialofservice_tests.cpp | 4 ++++ 5 files changed, 23 insertions(+), 27 deletions(-) diff --git a/src/llmq/net_signing.cpp b/src/llmq/net_signing.cpp index a2ed28b2929c..9a92279ba22c 100644 --- a/src/llmq/net_signing.cpp +++ b/src/llmq/net_signing.cpp @@ -288,10 +288,11 @@ 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); }); + // Drop pending recovered sigs from disconnected or discouraged peers so their backlog + // does not outlive the connection that created it. + m_sig_manager.RemoveNodesIf([this](NodeId node_id) { + return m_peer_manager->PeerIsDisconnectedOrDiscouraged(node_id); + }); } // TODO Wakeup when pending signing is needed? @@ -301,11 +302,12 @@ void NetSigning::WorkThreadSigning() } } -void NetSigning::RemoveBannedNodeStates() +void NetSigning::RemoveDisconnectedOrDiscouragedNodeStates() { assert(m_shares_manager != nullptr); - // Called regularly to cleanup local node states for banned nodes - m_shares_manager->RemoveNodesIf([this](NodeId node_id) { return m_peer_manager->PeerIsBanned(node_id); }); + m_shares_manager->RemoveNodesIf([this](NodeId node_id) { + return m_peer_manager->PeerIsDisconnectedOrDiscouraged(node_id); + }); } void NetSigning::BanNode(NodeId nodeId) @@ -323,7 +325,7 @@ void NetSigning::WorkThreadCleaning() assert(m_shares_manager); while (!workInterrupt) { - RemoveBannedNodeStates(); + RemoveDisconnectedOrDiscouragedNodeStates(); m_shares_manager->SendMessages(); m_shares_manager->Cleanup(); diff --git a/src/llmq/net_signing.h b/src/llmq/net_signing.h index 2dcbaf45a7b3..e690edf4261a 100644 --- a/src/llmq/net_signing.h +++ b/src/llmq/net_signing.h @@ -71,7 +71,7 @@ class NetSigning final : public NetHandler, public CValidationInterface std::unordered_map>&& sigSharesByNodes, std::unordered_map, CQuorumCPtr, StaticSaltedHasher>&& quorums); - void RemoveBannedNodeStates(); + void RemoveDisconnectedOrDiscouragedNodeStates(); void BanNode(NodeId nodeid); private: diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0e265c36bb2c..ca113372a242 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -619,7 +619,6 @@ class PeerManagerImpl final : public PeerManager const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, g_msgproc_mutex); void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) override; - bool IsBanned(NodeId pnode) override EXCLUSIVE_LOCKS_REQUIRED(cs_main, !m_peer_mutex); size_t GetRequestedObjectCount(NodeId nodeid) const override EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** Implements external handlers logic */ @@ -632,7 +631,7 @@ class PeerManagerImpl final : public PeerManager /** Implement PeerManagerInternal */ void PeerMisbehaving(const NodeId pnode, const int howmuch, const std::string& message = "") override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); - bool PeerIsBanned(const NodeId node_id) override EXCLUSIVE_LOCKS_REQUIRED(cs_main, !m_peer_mutex); + bool PeerIsDisconnectedOrDiscouraged(const NodeId node_id) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(::cs_main); bool PeerConsumeObjectRequest(NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(::cs_main); GetDataResponse PeerConsumeGetDataResponse(NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(::cs_main); @@ -1897,16 +1896,13 @@ void PeerManagerImpl::Misbehaving(Peer& peer, int howmuch, const std::string& me peer.m_id, score_before, score_now, warning, message_prefixed); } -bool PeerManagerImpl::IsBanned(NodeId pnode) +bool PeerManagerImpl::PeerIsDisconnectedOrDiscouraged(const NodeId node_id) { - PeerRef peer = GetPeerRef(pnode); - if (peer == nullptr) - return false; + PeerRef peer = GetPeerRef(node_id); + if (peer == nullptr) return true; + LOCK(peer->m_misbehavior_mutex); - if (peer->m_should_discourage) { - return true; - } - return false; + return peer->m_should_discourage; } bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& state, @@ -6713,11 +6709,6 @@ void PeerManagerImpl::PeerMisbehaving(const NodeId pnode, const int howmuch, con if (peer) Misbehaving(*peer, howmuch, message); } -bool PeerManagerImpl::PeerIsBanned(const NodeId node_id) -{ - return IsBanned(node_id); -} - void PeerManagerImpl::PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) { // Completing only this peer's announcement is deliberate: an invalid or unusable object must diff --git a/src/net_processing.h b/src/net_processing.h index 761da8001c07..ffdd805d76a2 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -81,7 +81,8 @@ class PeerManagerInternal { public: virtual void PeerMisbehaving(const NodeId pnode, const int howmuch, const std::string& message = "") = 0; - virtual bool PeerIsBanned(const NodeId node_id) = 0; + /** Whether the peer is no longer connected or has crossed the discouragement threshold. */ + virtual bool PeerIsDisconnectedOrDiscouraged(const NodeId node_id) = 0; /** Complete this peer's pending announcement of the inv, so it is not requested from them * again. Announcements of the same inv by other peers are unaffected: an invalid or unusable * object must not stop us from fetching it from honest peers. @@ -220,8 +221,6 @@ class PeerManager : public CValidationInterface, public NetEventsInterface, publ /** This function is used for testing the stale tip eviction logic, see denialofservice_tests.cpp */ virtual void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) = 0; - virtual bool IsBanned(NodeId pnode) = 0; - virtual size_t GetRequestedObjectCount(NodeId nodeid) const = 0; virtual void AddExtraHandler(std::unique_ptr&& handler) = 0; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 99cdb089819f..d6510563683f 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -338,7 +338,9 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) peerLogic->InitializeNode(*nodes[0], NODE_NETWORK); nodes[0]->fSuccessfullyConnected = true; connman->AddTestNode(*nodes[0]); + BOOST_CHECK(!peerLogic->PeerIsDisconnectedOrDiscouraged(nodes[0]->GetId())); peerLogic->UnitTestMisbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged + BOOST_CHECK(peerLogic->PeerIsDisconnectedOrDiscouraged(nodes[0]->GetId())); BOOST_CHECK(peerLogic->SendMessages(nodes[0])); BOOST_CHECK(banman->IsDiscouraged(addr[0])); BOOST_CHECK(nodes[0]->fDisconnect); @@ -358,6 +360,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) nodes[1]->fSuccessfullyConnected = true; connman->AddTestNode(*nodes[1]); peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD - 1); + BOOST_CHECK(!peerLogic->PeerIsDisconnectedOrDiscouraged(nodes[1]->GetId())); BOOST_CHECK(peerLogic->SendMessages(nodes[1])); // [0] is still discouraged/disconnected. BOOST_CHECK(banman->IsDiscouraged(addr[0])); @@ -399,6 +402,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) for (CNode* node : nodes) { peerLogic->FinalizeNode(*node); + BOOST_CHECK(peerLogic->PeerIsDisconnectedOrDiscouraged(node->GetId())); } connman->ClearTestNodes(); }