net_processing: Replace CompareInvMempoolOrder
What the developer wrote
Authored by Anthony Towns
net_processing: Replace CompareInvMempoolOrder
Remove CompareInvMempoolOrder, replacing it with the new
ExtractBestByMiningScoreWithTopology. The trickle send code is reworked
accordingly.
What changed, and why it matters
This Bitcoin Core commit refactors how transactions are selected and sorted before being announced to network peers. It replaces an older in-place heap sort with a new mempool function that picks the best transactions by mining fee score while respecting dependency order. The change is a code-quality and performance improvement; there is no direct evidence in the commit or supplied references that it fixes a security vulnerability.
Treat as a routine refactor/maintenance change. Review the new `ExtractBestByMiningScoreWithTopology` implementation (not supplied) for correctness, lock ordering, and potential denial-of-service or privacy regressions in transaction relay ordering. No immediate security response is indicated by this commit alone.
Security signals we found
Refactor of P2P transaction inventory relay ordering logic
Change of container type from std::set to std::vector for pending inventory
Introduction of new mempool dependency-aware selection API
Fee-filter check repositioned and now operates on mempool iterator metadata
No explicit security claim, CVE, or vulnerability description in commit or supplied references
Evidence from the diff
The patch changes m_tx_inventory_to_send from a std::set<Wtxid> to a std::vector<Wtxid> and removes the local CompareInvMempoolOrder comparator/heap logic in net_processing.cpp. Instead, it calls m_mempool.ExtractBestByMiningScoreWithTopology(invs, invs.size()) under m_mempool.cs to obtain topologically and fee-sorted transaction iterators. The fee-filter and bloom-filter checks are preserved, but the fee check now happens before constructing the inv and uses txiter->GetFee() / txiter->GetTxSize(). The m_last_inv_sequence update is moved inside the lambda while holding the mempool lock. A redundant m_tx_inventory_to_send.clear() is added when responding to mempool requests. The commit message frames this as a replacement/rework, not a security fix.
Changed components
src/net_processing.cppPeer transaction inventory relay (m_tx_inventory_to_send)Trickle send logic in PeerManagerImpl::SendMessagesMempool interaction via ExtractBestByMiningScoreWithTopologyInspect captured patch +37 / −55
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index b5b2375f..b3e32499 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -295,11 +295,11 @@ struct Peer {
* us or we have announced to the peer. We use this to avoid announcing
* the same (w)txid to a peer that already has the transaction. */
CRollingBloomFilter m_tx_inventory_known_filter GUARDED_BY(m_tx_inventory_mutex){50000, 0.000001};
- /** Set of wtxids we still have to announce. For non-wtxid-relay peers,
+ /** Vector of wtxids we still have to announce. For non-wtxid-relay peers,
* we retrieve the txid from the corresponding mempool transaction when
* constructing the `inv` message. We use the mempool to sort transactions
* in dependency order before relay, so this does not have to be sorted. */
- std::set<Wtxid> m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex);
+ std::vector<Wtxid> m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex);
/** Whether the peer has requested us to send our complete mempool. Only
* permitted if the peer has NetPermissionFlags::Mempool or we advertise
* NODE_BLOOM. See BIP35. */
@@ -2285,7 +2285,7 @@ void PeerManagerImpl::InitiateTxBroadcastToAll(const Txid& txid, const Wtxid& wt
const uint256& hash{peer.m_wtxid_relay ? wtxid.ToUint256() : txid.ToUint256()};
if (!tx_relay->m_tx_inventory_known_filter.contains(hash)) {
- tx_relay->m_tx_inventory_to_send.insert(wtxid);
+ tx_relay->m_tx_inventory_to_send.push_back(wtxid);
}
}
}
@@ -5627,22 +5627,6 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::mi
}
}
-namespace {
-class CompareInvMempoolOrder
-{
- const CTxMemPool* m_mempool;
-public:
- explicit CompareInvMempoolOrder(CTxMemPool* mempool) : m_mempool{mempool} {}
-
- bool operator()(std::set<Wtxid>::iterator a, std::set<Wtxid>::iterator b)
- {
- /* As std::make_heap produces a max-heap, we want the entries with the
- * higher mining score to sort later. */
- return m_mempool->CompareMiningScoreWithTopology(*b, *a);
- }
-};
-} // namespace
-
bool PeerManagerImpl::RejectIncomingTxs(const CNode& peer) const
{
// block-relay-only peers may never send txs to us
@@ -6112,6 +6096,9 @@ bool PeerManagerImpl::SendMessages(CNode& node)
tx_relay->m_send_mempool = false;
const CFeeRate filterrate{tx_relay->m_fee_filter_received.load()};
+ // we'll send everything in the mempool momentarily, so this is redundant
+ tx_relay->m_tx_inventory_to_send.clear();
+
LOCK(tx_relay->m_bloom_filter_mutex);
for (const auto& txinfo : vtxinfo) {
@@ -6120,7 +6107,6 @@ bool PeerManagerImpl::SendMessages(CNode& node)
const auto inv = peer.m_wtxid_relay ?
CInv{MSG_WTX, wtxid.ToUint256()} :
CInv{MSG_TX, txid.ToUint256()};
- tx_relay->m_tx_inventory_to_send.erase(wtxid);
// Don't send transactions that peers will not put into their mempool
if (txinfo.fee < filterrate.GetFee(txinfo.vsize)) {
@@ -6140,47 +6126,47 @@ bool PeerManagerImpl::SendMessages(CNode& node)
// Determine transactions to relay
if (fSendTrickle) {
- // Produce a vector with all candidates for sending
- std::vector<std::set<Wtxid>::iterator> vInvTx;
- vInvTx.reserve(tx_relay->m_tx_inventory_to_send.size());
- for (std::set<Wtxid>::iterator it = tx_relay->m_tx_inventory_to_send.begin(); it != tx_relay->m_tx_inventory_to_send.end(); it++) {
- vInvTx.push_back(it);
- }
- const CFeeRate filterrate{tx_relay->m_fee_filter_received.load()};
// Topologically and fee-rate sort the inventory we send for privacy and priority reasons.
- // A heap is used even though all items are being processed/sent.
- CompareInvMempoolOrder compareInvMempoolOrder(&m_mempool);
- std::make_heap(vInvTx.begin(), vInvTx.end(), compareInvMempoolOrder);
- LOCK(tx_relay->m_bloom_filter_mutex);
- vInv.reserve(std::min<size_t>(MAX_INV_SZ, tx_relay->m_tx_inventory_to_send.size()));
- while (!vInvTx.empty()) {
- // Fetch the top element from the heap
- std::pop_heap(vInvTx.begin(), vInvTx.end(), compareInvMempoolOrder);
- std::set<Wtxid>::iterator it = vInvTx.back();
- vInvTx.pop_back();
- auto wtxid = *it;
- // Remove it from the to-be-sent set
- tx_relay->m_tx_inventory_to_send.erase(it);
- // Not in the mempool anymore? don't bother sending it.
- auto txinfo = m_mempool.info(wtxid);
- if (!txinfo.tx) {
- continue;
+ // (sorted from higher priority to lowest, skipping low fee)
+ const CFeeRate filterrate{tx_relay->m_fee_filter_received.load()};
+
+ auto inv_tx = [&]() EXCLUSIVE_LOCKS_REQUIRED(tx_relay->m_tx_inventory_mutex) {
+ auto& invs = tx_relay->m_tx_inventory_to_send;
+ std::vector<CTransactionRef> res;
+
+ if (invs.size() == 0) return res;
+
+ // if previous allocations were excessive, shrink to the current size
+ if (invs.capacity() > 2 * invs.size()) invs.shrink_to_fit();
+
+ LOCK(m_mempool.cs);
+ auto txiters = m_mempool.ExtractBestByMiningScoreWithTopology(invs, invs.size());
+ res.reserve(txiters.size());
+ for (auto txiter : txiters) {
+ if (txiter->GetFee() < filterrate.GetFee(txiter->GetTxSize())) {
+ continue; // higher feerate CPFP txs may follow, so just skip, don't stop
+ }
+ res.push_back(txiter->GetSharedTx());
}
+ // Ensure we'll respond to GETDATA requests for anything we're about to announce
+ tx_relay->m_last_inv_sequence = m_mempool.GetSequence();
+ return res;
+ }();
+
+ LOCK(tx_relay->m_bloom_filter_mutex);
+ vInv.reserve(std::min<size_t>(MAX_INV_SZ, vInv.size() + inv_tx.size()));
+ for (auto& tx : inv_tx) {
// `TxRelay::m_tx_inventory_known_filter` contains either txids or wtxids
// depending on whether our peer supports wtxid-relay. Therefore, first
// construct the inv and then use its hash for the filter check.
const auto inv = peer.m_wtxid_relay ?
- CInv{MSG_WTX, wtxid.ToUint256()} :
- CInv{MSG_TX, txinfo.tx->GetHash().ToUint256()};
+ CInv{MSG_WTX, tx->GetWitnessHash().ToUint256()} :
+ CInv{MSG_TX, tx->GetHash().ToUint256()};
// Check if not in the filter already
if (tx_relay->m_tx_inventory_known_filter.contains(inv.hash)) {
continue;
}
- // Peer told you to not send transactions at that feerate? Don't bother sending it.
- if (txinfo.fee < filterrate.GetFee(txinfo.vsize)) {
- continue;
- }
- if (tx_relay->m_bloom_filter && !tx_relay->m_bloom_filter->IsRelevantAndUpdate(*txinfo.tx)) continue;
+ if (tx_relay->m_bloom_filter && !tx_relay->m_bloom_filter->IsRelevantAndUpdate(*tx)) continue;
// Send
vInv.push_back(inv);
if (vInv.size() == MAX_INV_SZ) {
@@ -6189,10 +6175,6 @@ bool PeerManagerImpl::SendMessages(CNode& node)
}
tx_relay->m_tx_inventory_known_filter.insert(inv.hash);
}
-
- // Ensure we'll respond to GETDATA requests for anything we've just announced
- LOCK(m_mempool.cs);
- tx_relay->m_last_inv_sequence = m_mempool.GetSequence();
}
}
if (!vInv.empty())
Why this scored 32/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.