diff --git a/src/llmq/net_signing.cpp b/src/llmq/net_signing.cpp index 32cca59ac834..2a40d7d587d3 100644 --- a/src/llmq/net_signing.cpp +++ b/src/llmq/net_signing.cpp @@ -297,11 +297,10 @@ void NetSigning::WorkThreadSigning() } } -void NetSigning::RemoveBannedNodeStates() +void NetSigning::RemoveDiscouragedNodeStates() { 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->PeerShouldBeDiscouraged(node_id); }); } void NetSigning::BanNode(NodeId nodeId, bool mark_shares_banned) @@ -324,7 +323,7 @@ void NetSigning::WorkThreadCleaning() assert(m_shares_manager); while (!workInterrupt) { - RemoveBannedNodeStates(); + RemoveDiscouragedNodeStates(); m_shares_manager->SendMessages(); m_shares_manager->Cleanup(); diff --git a/src/llmq/net_signing.h b/src/llmq/net_signing.h index bbf20804c1a5..3bfb7f19b832 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 RemoveDiscouragedNodeStates(); //! 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 diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 53e4834bdbcf..d747b66c5349 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -622,7 +622,6 @@ class PeerManagerImpl final : public PeerManager EXCLUSIVE_LOCKS_REQUIRED(!m_object_request_mutex, !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(!m_object_request_mutex); /** Implements external handlers logic */ @@ -635,7 +634,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 PeerShouldBeDiscouraged(const NodeId node_id) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(!m_object_request_mutex, ::cs_main); bool PeerConsumeObjectRequest(NodeId nodeid, const CInv& inv) override @@ -1954,16 +1953,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::PeerShouldBeDiscouraged(const NodeId node_id) { - PeerRef peer = GetPeerRef(pnode); - if (peer == nullptr) - return false; + PeerRef peer = GetPeerRef(node_id); + if (peer == nullptr) return false; + 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, @@ -6904,11 +6900,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 cb5e03a9570e..713e1bad9d8e 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -84,7 +84,9 @@ 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 crossed the discouragement threshold and SendMessages() has not acted on it + * yet. Does not consult BanMan; returns false for peers that are not (or no longer) known. */ + virtual bool PeerShouldBeDiscouraged(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. @@ -224,8 +226,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 9286528571f1..baead3e0f625 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -339,8 +339,11 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) peerLogic->InitializeNode(*nodes[0], NODE_NETWORK); nodes[0]->fSuccessfullyConnected = true; connman->AddTestNode(*nodes[0]); + BOOST_CHECK(!peerLogic->PeerShouldBeDiscouraged(nodes[0]->GetId())); peerLogic->UnitTestMisbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged + BOOST_CHECK(peerLogic->PeerShouldBeDiscouraged(nodes[0]->GetId())); BOOST_CHECK(peerLogic->SendMessages(nodes[0])); + BOOST_CHECK(!peerLogic->PeerShouldBeDiscouraged(nodes[0]->GetId())); // Consumed by SendMessages BOOST_CHECK(banman->IsDiscouraged(addr[0])); BOOST_CHECK(nodes[0]->fDisconnect); BOOST_CHECK(!banman->IsDiscouraged(other_addr)); // Different address, not discouraged @@ -359,6 +362,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->PeerShouldBeDiscouraged(nodes[1]->GetId())); BOOST_CHECK(peerLogic->SendMessages(nodes[1])); // [0] is still discouraged/disconnected. BOOST_CHECK(banman->IsDiscouraged(addr[0])); @@ -400,6 +404,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) for (CNode* node : nodes) { peerLogic->FinalizeNode(*node); + BOOST_CHECK(!peerLogic->PeerShouldBeDiscouraged(node->GetId())); } connman->ClearTestNodes(); }