Skip to content

Merge bitcoin/bitcoin#27675: p2p: Drop m_recently_announced_invs bloom filter #899

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
DashCoreAutoGuix wants to merge 1 commit into from
Closed

Merge bitcoin/bitcoin#27675: p2p: Drop m_recently_announced_invs bloom filter #899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/bench/mempool_eviction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@ static void AddTx(const CTransactionRef& tx, const CAmount& nFee, CTxMemPool& po
{
int64_t nTime = 0;
unsigned int nHeight = 1;
uint64_t sequence = 0;
bool spendsCoinbase = false;
unsigned int sigOps = 1;
LockPoints lp;
pool.addUnchecked(CTxMemPoolEntry(
tx, nFee, nTime, nHeight,
tx, nFee, nTime, nHeight, sequence,
spendsCoinbase, sigOps, lp));
}

Expand Down
3 changes: 2 additions & 1 deletion src/bench/mempool_stress.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@ static void AddTx(const CTransactionRef& tx, CTxMemPool& pool) EXCLUSIVE_LOCKS_R
{
int64_t nTime = 0;
unsigned int nHeight = 1;
uint64_t sequence = 0;
bool spendsCoinbase = false;
unsigned int sigOpCost = 4;
LockPoints lp;
pool.addUnchecked(CTxMemPoolEntry(tx, 1000, nTime, nHeight, spendsCoinbase, sigOpCost, lp));
pool.addUnchecked(CTxMemPoolEntry(tx, 1000, nTime, nHeight, sequence, spendsCoinbase, sigOpCost, lp));
}

struct Available {
Expand Down
2 changes: 1 addition & 1 deletion src/bench/rpc_mempool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
static void AddTx(const CTransactionRef& tx, const CAmount& fee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs)
{
LockPoints lp;
pool.addUnchecked(CTxMemPoolEntry(tx, fee, /*time=*/0, /*entry_height=*/1, /*spends_coinbase=*/false, /*sigops_count=*/1, lp));
pool.addUnchecked(CTxMemPoolEntry(tx, fee, /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*sigops_count=*/1, lp));
}

static void RpcMempool(benchmark::Bench& bench)
Expand Down
88 changes: 22 additions & 66 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,6 @@ static const unsigned int MAX_GETDATA_SZ = 1000;

/** How long to cache transactions in mapRelay for normal relay */
static constexpr auto RELAY_TX_CACHE_TIME = 15min;
/** How long a transaction has to be in the mempool before it can unconditionally be relayed (even when not in mapRelay). */
static constexpr auto UNCONDITIONAL_RELAY_DELAY = 2min;
/** Headers download timeout.
* Timeout = base + per_header * (expected number of headers) */
static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_BASE = 15min;
Expand Down Expand Up @@ -181,13 +179,6 @@ static constexpr auto OUTBOUND_INVENTORY_BROADCAST_INTERVAL{2s};
static constexpr unsigned int INVENTORY_BROADCAST_PER_SECOND = 7;
/** Maximum number of inventory items to send per transmission. */
static constexpr unsigned int INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK = 4 * INVENTORY_BROADCAST_PER_SECOND * count_seconds(INBOUND_INVENTORY_BROADCAST_INTERVAL);
/** The number of most recently announced transactions a peer can request. */
static constexpr unsigned int INVENTORY_MAX_RECENT_RELAY = 3500;
/** Verify that INVENTORY_MAX_RECENT_RELAY is enough to cache everything typically
* relayed before unconditional relay from the mempool kicks in. This is only a
* lower bound, and it should be larger to account for higher inv rate to outbound
* peers, and random variations in the broadcast mechanism. */
static_assert(INVENTORY_MAX_RECENT_RELAY >= INVENTORY_BROADCAST_PER_SECOND * UNCONDITIONAL_RELAY_DELAY / std::chrono::seconds{1}, "INVENTORY_RELAY_MAX too low");
/** Maximum number of compact filters that may be requested with one getcfilters. See BIP 157. */
static constexpr uint32_t MAX_GETCFILTERS_SIZE = 1000;
/** Maximum number of cf hashes that may be requested with one getcfheaders. See BIP 157. */
Expand Down Expand Up @@ -312,11 +303,12 @@ struct Peer {
* permitted if the peer has NetPermissionFlags::Mempool or we advertise
* NODE_BLOOM. See BIP35. */
bool m_send_mempool GUARDED_BY(m_tx_inventory_mutex){false};
/** The last time a BIP35 `mempool` request was serviced. */
std::atomic<std::chrono::seconds> m_last_mempool_req{0s};
/** The next time after which we will send an `inv` message containing
* transaction announcements to this peer. */
std::chrono::microseconds m_next_inv_send_time GUARDED_BY(NetEventsInterface::g_msgproc_mutex){0};
/** The mempool sequence num at which we sent the last `inv` message to this peer.
* Can relay txs with lower sequence numbers than this (see CTxMempool::info_for_relay). */
uint64_t m_last_inv_sequence GUARDED_BY(NetEventsInterface::g_msgproc_mutex){1};
};
Comment on lines +309 to 312

@coderabbitai coderabbitai Bot Aug 2, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

VALIDATION: FAIL - Not a faithful backport of Bitcoin PR bitcoin#27675

This change introduces mempool sequence tracking to the relay logic, which is not part of the original Bitcoin PR bitcoin#27675. The original Bitcoin commit only modified a test file (test/functional/p2p_filter.py) with 17 lines of changes, not core relay logic.

🤖 Prompt for AI Agents 
In src/net_processing.cpp around lines 309 to 312, remove the addition of
mempool sequence tracking variables and related relay logic changes, as these
were not part of the original Bitcoin PR #27675. Revert the code to match the
original Bitcoin commit, which only modified the test/functional/p2p_filter.py
file without altering core relay logic.


/**
Expand Down Expand Up @@ -583,9 +575,6 @@ struct CNodeState {
//! Whether this peer is an inbound connection
const bool m_is_inbound;

//! A rolling bloom filter of all announced tx CInvs to this peer.
CRollingBloomFilter m_recently_announced_invs = CRollingBloomFilter{INVENTORY_MAX_RECENT_RELAY, 0.000001};

CNodeState(bool is_inbound) : m_is_inbound(is_inbound) {}
};

Expand Down Expand Up @@ -1010,7 +999,8 @@ class PeerManagerImpl final : public PeerManager
std::atomic<std::chrono::seconds> m_last_tip_update{0s};

/** Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). */
CTransactionRef FindTxForGetData(const CNode* peer, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main);
CTransactionRef FindTxForGetData(const Peer::TxRelay& tx_relay, const uint256& txid)
EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, NetEventsInterface::g_msgproc_mutex);
Comment on lines +1002 to +1003

@coderabbitai coderabbitai Bot Aug 2, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Major API change not present in original Bitcoin PR

The FindTxForGetData method signature has been completely changed to use sequence-based relay instead of time-based relay. This is not part of Bitcoin PR bitcoin#27675 which only touched test files.

🤖 Prompt for AI Agents 
In src/net_processing.cpp at lines 1002-1003, the method signature of
FindTxForGetData has been changed to use sequence-based relay instead of the
original time-based relay, which deviates from the original Bitcoin PR #27675.
To fix this, revert the method signature and implementation to match the
original PR by using time-based relay parameters and logic, ensuring consistency
with the upstream Bitcoin codebase.


void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(peer.m_getdata_requests_mutex) LOCKS_EXCLUDED(::cs_main);

Expand Down Expand Up @@ -2660,29 +2650,23 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
}
}

//! Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed).
CTransactionRef PeerManagerImpl::FindTxForGetData(const CNode* peer, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds now)
CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const uint256& txid)
{
auto txinfo = m_mempool.info(txid);
// If a tx was in the mempool prior to the last INV for this peer, permit the request.
auto txinfo = m_mempool.info_for_relay(txid, tx_relay.m_last_inv_sequence);
if (txinfo.tx) {
// If a TX could have been INVed in reply to a MEMPOOL request,
// or is older than UNCONDITIONAL_RELAY_DELAY, permit the request
// unconditionally.
if ((mempool_req.count() && txinfo.m_time <= mempool_req) || txinfo.m_time <= now - UNCONDITIONAL_RELAY_DELAY) {
return std::move(txinfo.tx);
}
return std::move(txinfo.tx);
}

// Or it might be from the most recent block
{
LOCK(cs_main);

// Otherwise, the transaction must have been announced recently.
if (State(peer->GetId())->m_recently_announced_invs.contains(txid)) {
// If it was, it can be relayed from either the mempool...
if (txinfo.tx) return std::move(txinfo.tx);
// ... or the relay pool.
auto mi = mapRelay.find(txid);
if (mi != mapRelay.end()) return mi->second;
LOCK(m_most_recent_block_mutex);
if (m_most_recent_block) {
for (const auto& tx : m_most_recent_block->vtx) {
if (tx->GetHash() == txid) {
return tx;
}
}
}
}

Expand All @@ -2699,11 +2683,6 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
std::vector<CInv> vNotFound;
const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());

const auto now{GetTime<std::chrono::seconds>()};
// Get last mempool request time
const auto mempool_req = tx_relay != nullptr ? tx_relay->m_last_mempool_req.load()
: std::chrono::seconds::min();

// Process as many TX items from the front of the getdata queue as
// possible, since they're common and it's efficient to batch process
// them.
Expand Down Expand Up @@ -2731,7 +2710,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic

bool push = false;
if (inv.IsGenTxMsg()) {
CTransactionRef tx = FindTxForGetData(&pfrom, inv.hash, mempool_req, now);
CTransactionRef tx = FindTxForGetData(*tx_relay, inv.hash);
if (tx) {
CCoinJoinBroadcastTx dstx;
if (inv.IsMsgDstx()) {
Expand All @@ -2744,30 +2723,6 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
}
m_mempool.RemoveUnbroadcastTx(tx->GetHash());
push = true;

// As we're going to send tx, make sure its unconfirmed parents are made requestable.
std::vector<uint256> parent_ids_to_add;
{
LOCK(m_mempool.cs);
auto txiter = m_mempool.GetIter(tx->GetHash());
if (txiter) {
const CTxMemPoolEntry::Parents& parents = (*txiter)->GetMemPoolParentsConst();
parent_ids_to_add.reserve(parents.size());
for (const CTxMemPoolEntry& parent : parents) {
if (parent.GetTime() > now - UNCONDITIONAL_RELAY_DELAY) {
parent_ids_to_add.push_back(parent.GetTx().GetHash());
}
}
}
}

for (const uint256& parent_txid : parent_ids_to_add) {
// Relaying a transaction with a recent but unconfirmed parent.
if (WITH_LOCK(tx_relay->m_tx_inventory_mutex, return !tx_relay->m_tx_inventory_known_filter.contains(parent_txid))) {
LOCK(cs_main);
State(pfrom.GetId())->m_recently_announced_invs.insert(parent_txid);
}
}
}
}

Expand Down Expand Up @@ -6054,7 +6009,6 @@ bool PeerManagerImpl::SendMessages(CNode* pto)

auto queueAndMaybePushInv = [this, pto, peer, &vInv, &msgMaker](const CInv& invIn) {
LogPrint(BCLog::NET, "SendMessages -- queued inv: %s index=%d peer=%d\n", invIn.ToString(), vInv.size(), pto->GetId());
// Responses to MEMPOOL requests bypass the m_recently_announced_invs filter.
vInv.push_back(invIn);
if (vInv.size() == MAX_INV_SZ) {
LogPrint(BCLog::NET, "SendMessages -- pushing invs: count=%d peer=%d\n", vInv.size(), pto->GetId());
Expand Down Expand Up @@ -6119,7 +6073,6 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
tx_relay->m_tx_inventory_known_filter.insert(chainlockHash);
queueAndMaybePushInv(CInv(MSG_CLSIG, chainlockHash));
}
tx_relay->m_last_mempool_req = std::chrono::duration_cast<std::chrono::seconds>(current_time);
}

// Determine transactions to relay
Expand Down Expand Up @@ -6161,7 +6114,6 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
}
if (tx_relay->m_bloom_filter && !tx_relay->m_bloom_filter->IsRelevantAndUpdate(*txinfo.tx)) continue;
// Send
State(pto->GetId())->m_recently_announced_invs.insert(hash);
nRelayedTransactions++;
{
// Expire old relay messages
Expand All @@ -6180,6 +6132,10 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
tx_relay->m_tx_inventory_known_filter.insert(hash);
queueAndMaybePushInv(CInv(nInvType, 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();
}
Comment on lines +6136 to 6139

@coderabbitai coderabbitai Bot Aug 2, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Sequence number tracking after inventory announcements

Updates m_last_inv_sequence after sending transaction inventories. This sequence-based approach is completely different from the bloom filter mechanism it replaces and is not part of the original Bitcoin PR bitcoin#27675.

🤖 Prompt for AI Agents 
In src/net_processing.cpp around lines 6136 to 6139, the code updates
m_last_inv_sequence after sending transaction inventories, which uses a
sequence-based approach different from the previous bloom filter mechanism and
is not part of the original Bitcoin PR #27675. Review this change to ensure it
aligns with the intended design; if the sequence tracking is required, confirm
it is correctly synchronized with m_mempool and properly documented to clarify
its purpose and difference from the bloom filter method.

}
{
Expand Down
2 changes: 1 addition & 1 deletion src/node/interfaces.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -925,7 +925,7 @@ class ChainImpl : public Chain
{
if (!m_node.mempool) return true;
LockPoints lp;
CTxMemPoolEntry entry(tx, 0, 0, 0, false, 0, lp);
CTxMemPoolEntry entry(tx, 0, 0, 0, 0, false, 0, lp);
CTxMemPool::setEntries ancestors;
auto limit_ancestor_count = gArgs.GetIntArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT);
auto limit_ancestor_size = gArgs.GetIntArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT) * 1000;
Expand Down
4 changes: 2 additions & 2 deletions src/test/util/setup_common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,7 @@ std::vector<CTransactionRef> TestChainSetup::PopulateMempool(FastRandomContext&
if (submit) {
LOCK2(m_node.mempool->cs, cs_main);
LockPoints lp;
m_node.mempool->addUnchecked(CTxMemPoolEntry(ptx, 1000, 0, 1, false, 4, lp));
m_node.mempool->addUnchecked(CTxMemPoolEntry(ptx, 1000, 0, 1, 0, false, 4, lp));
}
--num_transactions;
}
Expand All @@ -670,7 +670,7 @@ CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CMutableTransaction& tx) co

CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CTransactionRef& tx) const
{
return CTxMemPoolEntry(tx, nFee, TicksSinceEpoch<std::chrono::seconds>(time), nHeight,
return CTxMemPoolEntry(tx, nFee, TicksSinceEpoch<std::chrono::seconds>(time), nHeight, 0,
spendsCoinbase, sigOpCount, lp);
}

Expand Down
14 changes: 13 additions & 1 deletion src/txmempool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,14 @@ bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp)
}

CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee,
int64_t time, unsigned int entry_height,
int64_t time, unsigned int entry_height, uint64_t entry_sequence,
bool spends_coinbase, int64_t sigops_count, LockPoints lp)
: tx{tx},
nFee{fee},
nTxSize(tx->GetTotalSize()),
nUsageSize{RecursiveDynamicUsage(tx)},
nTime{time},
entry_sequence{entry_sequence},
entryHeight{entry_height},
spendsCoinbase{spends_coinbase},
sigOpCount{sigops_count},
Expand Down Expand Up @@ -1327,6 +1328,17 @@ TxMempoolInfo CTxMemPool::info(const uint256& hash) const
return GetInfo(i);
}

TxMempoolInfo CTxMemPool::info_for_relay(const uint256& txid, uint64_t last_sequence) const
{
LOCK(cs);
indexed_transaction_set::const_iterator i = mapTx.find(txid);
if (i != mapTx.end() && i->GetSequence() < last_sequence) {
return GetInfo(i);
} else {
return TxMempoolInfo();
}
}
Comment on lines +1331 to +1340

@coderabbitai coderabbitai Bot Aug 2, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Additional scope creep in new method implementation.

The info_for_relay method represents new functionality for sequence-based relay filtering, which is not consistent with a simple bloom filter removal as described in the original Bitcoin PR bitcoin#27675.

This method implements entirely new relay logic rather than removing existing bloom filter code. Combined with the constructor changes, this confirms the PR is a complete reimplementation rather than a faithful backport.

The original Bitcoin commit should be reviewed to determine if these changes align with the intended scope, or if this PR needs to be rewritten to match the actual Bitcoin changes.

🤖 Prompt for AI Agents 
In src/txmempool.cpp around lines 1331 to 1340, the info_for_relay method
introduces new sequence-based relay filtering logic that deviates from the
original Bitcoin PR #27675's scope, which focused on removing bloom filter code.
Review the original Bitcoin commit to verify the intended changes and adjust
this method accordingly to either remove bloom filter logic as originally
intended or rewrite this PR to align with the actual Bitcoin changes, avoiding
adding unrelated new relay logic.


bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const {
assert(m_dmnman);

Expand Down
8 changes: 7 additions & 1 deletion src/txmempool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ class CTxMemPoolEntry
const size_t nTxSize; //!< ... and avoid recomputing tx size
const size_t nUsageSize; //!< ... and total memory usage
const int64_t nTime; //!< Local time when entering the mempool
const uint64_t entry_sequence; //!< Sequence number used to determine whether this transaction is too recent for relay
const unsigned int entryHeight; //!< Chain height when entering the mempool
const bool spendsCoinbase; //!< keep track of transactions that spend a coinbase
const int64_t sigOpCount; //!< Legacy sig ops plus P2SH sig op count
Expand All @@ -132,7 +133,7 @@ class CTxMemPoolEntry

public:
CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee,
int64_t time, unsigned int entry_height,
int64_t time, unsigned int entry_height, uint64_t entry_sequence,
bool spends_coinbase,
int64_t sigops_count, LockPoints lp);

Expand All @@ -142,6 +143,7 @@ class CTxMemPoolEntry
size_t GetTxSize() const;
std::chrono::seconds GetTime() const { return std::chrono::seconds{nTime}; }
unsigned int GetHeight() const { return entryHeight; }
uint64_t GetSequence() const { return entry_sequence; }
int64_t GetSigOpCount() const { return sigOpCount; }
CAmount GetModifiedFee() const { return m_modified_fee; }
size_t DynamicMemoryUsage() const { return nUsageSize; }
Expand Down Expand Up @@ -793,6 +795,10 @@ class CTxMemPool

CTransactionRef get(const uint256& hash) const;
TxMempoolInfo info(const uint256& hash) const;

/** Returns info for a transaction if its entry_sequence < last_sequence */
TxMempoolInfo info_for_relay(const uint256& txid, uint64_t last_sequence) const;

std::vector<TxMempoolInfo> infoAll() const;

/**
Expand Down
3 changes: 2 additions & 1 deletion src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -853,7 +853,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
}
}

entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(),
entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(),
bypass_limits ? 0 : m_pool.GetSequence(),
fSpendsCoinbase, nSigOps, lp));
ws.m_vsize = entry->GetTxSize();

Expand Down
Loading
Loading