diff --git a/src/llmq/net_signing.cpp b/src/llmq/net_signing.cpp index a2ed28b2929c..aeba134b3ea8 100644 --- a/src/llmq/net_signing.cpp +++ b/src/llmq/net_signing.cpp @@ -288,10 +288,8 @@ 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); }); + // PeerIsBanned() is transient. Finalized node ids are never reused, so disconnection remains observable. + m_sig_manager.RemoveNodesIf([this](NodeId node_id) { return !m_peer_manager->PeerIsConnected(node_id); }); } // TODO Wakeup when pending signing is needed? diff --git a/src/llmq/signing.h b/src/llmq/signing.h index 76b6c4326cb3..d5a44fbe55e9 100644 --- a/src/llmq/signing.h +++ b/src/llmq/signing.h @@ -217,8 +217,7 @@ 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. + // Drop pending (not-yet-verified) recovered sigs for matching node ids. 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 diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0e265c36bb2c..cc9a6005d3a1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -633,6 +633,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 PeerIsConnected(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); @@ -6718,6 +6719,11 @@ bool PeerManagerImpl::PeerIsBanned(const NodeId node_id) return IsBanned(node_id); } +bool PeerManagerImpl::PeerIsConnected(const NodeId node_id) +{ + return GetPeerRef(node_id) != nullptr; +} + 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..1e9ea2ec9dca 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -82,6 +82,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 exists and has not been finalized. */ + virtual bool PeerIsConnected(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. diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 99cdb089819f..1b5d16dd6152 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -338,8 +338,13 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) peerLogic->InitializeNode(*nodes[0], NODE_NETWORK); nodes[0]->fSuccessfullyConnected = true; connman->AddTestNode(*nodes[0]); + BOOST_CHECK(peerLogic->PeerIsConnected(nodes[0]->GetId())); peerLogic->UnitTestMisbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged + BOOST_CHECK(WITH_LOCK(::cs_main, return peerLogic->PeerIsBanned(nodes[0]->GetId()))); + BOOST_CHECK(peerLogic->PeerIsConnected(nodes[0]->GetId())); BOOST_CHECK(peerLogic->SendMessages(nodes[0])); + BOOST_CHECK(WITH_LOCK(::cs_main, return !peerLogic->PeerIsBanned(nodes[0]->GetId()))); + BOOST_CHECK(peerLogic->PeerIsConnected(nodes[0]->GetId())); BOOST_CHECK(banman->IsDiscouraged(addr[0])); BOOST_CHECK(nodes[0]->fDisconnect); BOOST_CHECK(!banman->IsDiscouraged(other_addr)); // Different address, not discouraged @@ -399,6 +404,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) for (CNode* node : nodes) { peerLogic->FinalizeNode(*node); + BOOST_CHECK(!peerLogic->PeerIsConnected(node->GetId())); } connman->ClearTestNodes(); }