p2p: Evict outbound peers with high minFeeRate

Having no peers with sufficiently broad mempool
limits may prevent our low-fee transactions from
propagating, even though they would seem perfectly
valid for us locally.

Having too few such peers may cause privacy leaks.

We should periodically check that we have sufficient
peers with mempool limits comparable to ours.

If that's not the case, we should evict some high-minFeeRate
outbound peers to make room for a new peer with hopefully
broader limits.

src/net.cpp

@@ -2355,7 +2355,7 @@ int CConnman::GetExtraFullOutboundCount() const
             }
         }
     }
-    return std::max(full_outbound_peers - m_max_outbound_full_relay, 0);
+    return full_outbound_peers - m_max_outbound_full_relay;
 }
 
 int CConnman::GetExtraBlockRelayCount() const

src/net_processing.cpp

@@ -505,6 +505,7 @@ public:
     /** Implement PeerManager */
     void StartScheduledTasks(CScheduler& scheduler) override;
     void CheckForStaleTipAndEvictPeers() override;
+    void CheckForLimitedMempoolAndEvictPeers() override;
     std::optional<std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override
         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
@@ -1832,6 +1833,8 @@ void PeerManagerImpl::StartScheduledTasks(CScheduler& scheduler)
     static_assert(EXTRA_PEER_CHECK_INTERVAL < STALE_CHECK_INTERVAL, "peer eviction timer should be less than stale tip check timer");
     scheduler.scheduleEvery([this] { this->CheckForStaleTipAndEvictPeers(); }, std::chrono::seconds{EXTRA_PEER_CHECK_INTERVAL});
 
+    scheduler.scheduleEvery([this] { this->CheckForLimitedMempoolAndEvictPeers(); }, std::chrono::seconds{60});
+
     // schedule next run for 10-15 minutes in the future
     const std::chrono::milliseconds delta = 10min + GetRandMillis(5min);
     scheduler.scheduleFromNow([&] { ReattemptInitialBroadcast(scheduler); }, delta);
@@ -5194,6 +5197,62 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers()
     }
 }
 
+void PeerManagerImpl::CheckForLimitedMempoolAndEvictPeers()
+{
+    LOCK(cs_main);
+    // It only makes sense once we're out of IBD, because otherwise our mempool
+    // is not a good source for fee estimation anyway.
+    // TODO: assumeUTXO?
+    if (!CanDirectFetch()) return;
+
+    // Don't bother if we are still seeking for outbound peers: eviction
+    // instead will reduce our chances to get blocks and transactions from
+    // them.
+    // TODO this could be more nuanced.
+    if (m_connman.GetExtraFullOutboundCount() < 0) return;
+
+    // Ideally we want to compare mempool sizes, not fee filters.
+    // Otherwise we easily get confused: e.g. at empty mempools this is less
+    // critical, but that'd be impossible to account for.
+    const auto our_min_feerate = WITH_LOCK(m_mempool.cs, return m_mempool.GetMinFee());
+
+    std::vector<std::pair<CNode*, CAmount>> peer_fee_filters;
+
+    size_t around_our_percent_fee_filter = 0;
+
+    m_connman.ForEachNode([&](CNode* pnode) {
+        if (!pnode->IsFullOutboundConn() || pnode->fDisconnect) return;
+        auto peer = GetPeerRef(pnode->GetId());
+
+        if (auto tx_relay = peer->GetTxRelay(); tx_relay != nullptr) {
+            if (tx_relay->m_fee_filter_received) {
+                peer_fee_filters.push_back(std::make_pair(pnode, tx_relay->m_fee_filter_received.load()));
+            } else {
+                ++around_our_percent_fee_filter;
+            }
+        }
+    });
+
+    if (peer_fee_filters.size() == 0) return;
+
+    std::sort(peer_fee_filters.begin(), peer_fee_filters.end());
+
+    for (auto [node_id, fee_filter] : peer_fee_filters) {
+        if (fee_filter == 0) ++around_our_percent_fee_filter;
+        // Lower than our filter means they are more permissive.
+        if (CFeeRate{fee_filter} <= our_min_feerate) ++around_our_percent_fee_filter;;
+    }
+
+    if (around_our_percent_fee_filter < 4) {
+        // Drop one peer at a time to preserve a sufficient total
+        // number of connections, and to avoid network DoS.
+        // TODO add randomness chosing among low-feerate candidates.
+        peer_fee_filters.back().first->fDisconnect = true;
+    }
+
+}
+
+
 void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::microseconds now)
 {
     if (m_connman.ShouldRunInactivityChecks(node_to, std::chrono::duration_cast<std::chrono::seconds>(now)) &&

src/net_processing.h

@@ -101,6 +101,14 @@ public:
      */
     virtual void CheckForStaleTipAndEvictPeers() = 0;
 
+    /**
+     * We should have outbound peers with broad-enough mempools (comparable to ours) to accept
+     * transactions constructed according to our mempool. Otherwise, the lower-fee transactions
+     * will never be relayed to the network.
+     */
+    virtual void CheckForLimitedMempoolAndEvictPeers() = 0;
+
+
     /** Process a single message from a peer. Public for fuzz testing */
     virtual void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
                                 const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;

test/functional/p2p_out_eviction.py

@@ -0,0 +1,67 @@
+#!/usr/bin/env python3
+# Copyright (c) 2023 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+import time
+
+from test_framework.blocktools import (
+    create_block,
+    create_coinbase,
+)
+from test_framework.messages import (
+    msg_pong,
+    msg_tx,
+    msg_feefilter,
+)
+from test_framework.p2p import (
+    P2PDataStore,
+    P2PInterface,
+)
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal, check_node_connections
+from test_framework.wallet import MiniWallet
+
+class P2POutEvict(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 1
+        # Start with all the same fee filters.
+        self.extra_args = [["-minrelaytxfee=0.00000100"]]
+
+    def mock_forward(self, delta):
+        self.mock_time += delta
+        self.nodes[0].setmocktime(self.mock_time)
+
+    def run_test(self):
+        self.mock_time = int(time.time())
+        self.mock_forward(0)
+        node = self.nodes[0]
+        peers = []
+        for id in range(8):
+            peers.append(node.add_outbound_p2p_connection(P2PDataStore(), p2p_idx=id, connection_type="outbound-full-relay"))
+
+        node.mockscheduler(60)
+        check_node_connections(node=node, num_in=0, num_out=8)
+
+        # Set a limited number of fee filters. Nothing is evicted because
+        # the rest of the peers are sufficient.
+        for id in range(4):
+            peers[id].send_and_ping(msg_feefilter(10))
+        node.mockscheduler(60)
+        check_node_connections(node=node, num_in=0, num_out=8)
+
+        # Set one more filter, but make it borderline acceptable.
+        peers[4].send_and_ping(msg_feefilter(81))
+        node.mockscheduler(60)
+        check_node_connections(node=node, num_in=0, num_out=8)
+
+        # Now there is too few peers with a comparable min tx relay fee,
+        # thus one of the peers must be evicted to have a room for a new peer
+        # (hopefully with sufficient fee filter).
+        peers[4].send_and_ping(msg_feefilter(10))
+        node.mockscheduler(60)
+        self.mock_forward(60)
+        self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == 7, timeout=10)
+
+if __name__ == '__main__':
+    P2POutEvict().main()