refactor(net): merge duplicate peer discouragement query

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();

src/llmq/net_signing.h

@@ -71,7 +71,7 @@ class NetSigning final : public NetHandler, public CValidationInterface
         std::unordered_map<NodeId, std::vector<CSigShare>>&& sigSharesByNodes,
         std::unordered_map<std::pair<Consensus::LLMQType, uint256>, 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

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

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<NetHandler>&& handler) = 0;

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();
 }