https://github.com/bitcoin/bitcoin/pull/28488

Motivation

This PR tries to fix ‣ and it is somewhat related to PR #28538

Having outbound peers with mempool limits that greatly differ from ours is detrimental, given such links won’t be useful to propagate data we sent trough them (the transactions that fit under our limits but not theirs will bounce). Therefore, is it desirable to maintain a minimum number of outbound connections to peers with mempool limits similar to ours.

Notice how, similar to https://github.com/bitcoin/bitcoin/pull/28538, a low usable outbound connection count is also detrimental to our node’s privacy, given transaction originated from us will have a smaller initial flooding source set.

Thoughts/Questions

• At first sight, this seems will collide with #28538 given both are trying to evict peers if CConnman::GetExtraFullOutboundCount is zero (or negative in this case, but I think zero should suffice). Therefore, I wonder whether this should be part of PeerManagerImpl::EvictExtraOutboundPeers so we’re not racing here.
• Why is the distinction between being at the limit or above the limit being made? (m_connman.GetExtraFullOutboundCount() < 0)
  • NVM, my bad, being at the limit, or above, means that we need to evict, hence we need to distinguish between 0+ and negatives
• I don’t get the values for msg_feefilter in the tests. Is the higher the better, I thought it would be the other way around?
  • Answer: I really don’t see how this feefilter logic works, how does 10 relate to 0.1 sat/byte and how is it 81 less restrictive than 10.

Code

Once we’ve made sure that were are not in IBD (it does not make sense to evict outbound peers due to mempool limits in IBD) and we’ve reached the outbound connection limit, we can start to get picky about what outbound connections we want to keep arround.

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;

For each of the nodes we are connected to we are not fully connected to (done internally by CConnman::ForEachNode and the ones that are not fully outbound (!pnode-> IsFullOutboundConn(), i.e. blocks-only, block relay, …). From the remaining ones, we check out transaction relay details, and whether a filter has been set by the peer.

We record all peers that set a filter (peer_fee_filters) and accumulate the others as having limits similar to ours (around_our_percent_fee_filter).

If after all this there are no filters set, we are done.

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

• I don’t get why we need to sort peer_fee_filters here. We are latter iterating over the whole vector anyway, with no early exit what it the point of it being ordered?
  • Ok, I get it now, we want to pick the most restrictive filter if we need to, however it feels this could be done conditionally to being needed, as in within the if (around_our_percent_fee_filter < 4) context.