net: wait for validation queue flush for missing compact filter for an already known block #35354

randomlogin commented at 10:07 PM on May 21, 2026: none

This PR is another approach to fix a race condition bug when a yet-unconstructed compact filter is requested for an already known block.

Fixes: #29655, #27085 Related discussion: 1, 2 Another approach which received NACK: #35262

The problem: Bitcoin Core receives a new block and processes it, then advertises it; however, by that moment the compact filter might not yet have been constructed. If a peer asks for a filter in that time window, Bitcoin Core simply does not respond. From the peer's perspective this is a misbehaviour (we advertised a block and don't provide filters for it), which results in disconnects or bans. While it does not critically affect core nodes, it breaks the network topology for BIP 157 clients.

More precisely, BIP157 specifies that for getcfilters requestor:

StopHash MUST be known to belong to a block accepted by the receiving peer. This is the case if the peer had previously sent a headers or inv message with that block or any descendents. A node that receives getcfilters with an unknown StopHash SHOULD NOT respond.

Current behaviour clearly contradicts the above specification.

Proposed solution:

As was suggested, we can:

attempt to respond to a request (getcfilters, getcfheaders, getcfcheckpt which uses LookupFilterHeader); if it fails,
check that we know the requested block hash and that the height difference between the last constructed filter block and the requested block is small — from which we deduce that pending validation events would fix the miss,
wait for validation-queue flush and then retry.

Waiting for the validation queue flush parks the single msghand thread, which blocks all peers' ProcessMessages for the duration of the drain. So the trigger has to be tight: an external actor can otherwise force us into that wait.

This PR introduces BaseIndex::WaitForRacingWrite(target, max_ahead), which only drains when:

the index is synced (m_synced == true),
we have a non-null target block index,
the target is at most max_ahead blocks ahead of the index's last processed block (and not below it).

BlockFilterIndex calls this from all four lookup paths (LookupFilter, LookupFilterHeader, LookupFilterRange, LookupFilterHashRange) with max_ahead = CF_MAX_BLOCKS_AHEAD_RACE_WAIT = 2, retrying the lookup once after the drain.

Reorg handling:

The same primitive cleanly handles the reorg cases the bug originally exposed:

New chain extends past the old tip (ahead > 0): the queued BlockConnected events for the new branch will, in order, run BaseIndex::BlockConnected's internal Rewind (which calls CustomRemove on the disconnected blocks — BlockFilterIndex::CustomRemove migrates each old filter from its height-keyed slot to a hash-keyed slot) and then CustomAppend for the new blocks. After the drain, both the old (hash-keyed) and new (height-keyed) filters are reachable.
Same-height sibling reorg (ahead == 0): the index's last processed block and the requested block sit at the same height on different branches. The first lookup misses (height-keyed slot still holds the old block's data, hash-keyed slot for the new block doesn't exist yet). The ahead == 0 branch of the helper drains the queue, which runs the disconnect/connect for the sibling reorg, after which the retry succeeds.
Stale-branch lookup after the reorg has settled (ahead < 0): the helper does not wait — there is no in-flight callback that would help. The lookup falls back to the hash-keyed slot independently via the existing dual-key storage.

This PR does not affect block propagation, as the very root of this bug is the independence of block processing and filter construction in CustomAppend. Nor does it affect IBD: during IBD m_synced is false, the helper returns immediately, and we simply don't respond to compact filter messages (the writes are happening on the sync thread, not via the validation queue, so draining wouldn't help anyway). The IBD short-circuit also closes the DoS surface during the period when the node is most resource-constrained.

Drive-by:

LookupFilterHeader previously held m_cs_headers_cache across the DB lookup. Now that the lookup path can call SyncWithValidationInterfaceQueue (which must not be invoked under a lock that a queued callback might need), the cache lock is narrowed to wrap only the cache reads/writes, and the cache insert is switched to try_emplace since the check-then-insert is no longer atomic.

Tests:

A new cfilter_race_tests suite uses a PreFilterBlocker CValidationInterface registered before the filter index, so its own BlockConnected fires first on the scheduler thread and stalls it before the filter index gets to run CustomAppend. While the scheduler is parked, the test issues the lookup on a worker thread and asserts:

the worker remains blocked (proves the wait path is engaged — a regression that bypasses it would let the worker return immediately with false);
after releasing the scheduler, the worker returns the correct filter/header/range.

Cases covered:

cfilter_available_during_append_window — LookupFilter for the new tip racing with BlockConnected.
cfilter_range_available_during_append_window — LookupFilterRange covering indexed history plus the racing tip.
cfilter_header_available_during_append_window — LookupFilterHeader on the racing tip.
cfilter_available_during_same_height_reorg — same-height sibling reorg path (ahead == 0).

I've used Claude Code for this PR.

DrahtBot added the label P2P on May 21, 2026

DrahtBot commented at 10:08 PM on May 21, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35354.

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35229 (refactor: Use CBlockIndex parameters as reference by optout21)
#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

NetMsg::Make(NetMsgType::VERSION, PROTOCOL_VERSION, uint64_t{NODE_NETWORK}, int64_t{0}, uint64_t{NODE_NETWORK}, CAddress::V1_NETWORK(CService{})) in src/test/cfilter_race_tests.cpp

2026-08-05 11:50:50

randomlogin marked this as ready for review on May 21, 2026

DrahtBot added the label CI failed on May 22, 2026

DrahtBot commented at 10:47 AM on May 22, 2026: contributor

🚧 At least one of the CI tasks failed. Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/26255963544/job/77353717155 LLM reason (✨ experimental): CI failed because ctest detected a test failure in cfilter_race_tests (1 test failed).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

randomlogin force-pushed on May 22, 2026

randomlogin commented at 10:50 AM on May 22, 2026: none

Squashed two commits in one (previously there was a distinct commit for tests addition).

DrahtBot removed the label CI failed on May 22, 2026

sedited requested review from ajtowns on May 25, 2026

sedited commented at 9:47 PM on June 21, 2026: contributor

Reading through this, I don't think this should be handled on the index level. While I'm still not sold that this is not something that is just a bit flawed in the spec, I would prefer an approach where net_processing dealt with this directly in the case of an index query returning nothing by checking if the requested block lies in the two block tolerance range and if so syncing the validation interface queue.

sedited requested review from rustaceanrob on Jun 21, 2026

sedited commented at 9:56 AM on July 24, 2026: contributor

@randomlogin how do you want to proceed here?

randomlogin commented at 3:52 PM on July 28, 2026: none

@sedited sorry for a long response.

There are two ways: the first one is simply to move race-window check logic to net_processing and then again wait for SyncWithValidationInterfaceQueue.

On the other hand, instead of blocking the caller (msghand) on SyncWithValidationInterfaceQueue completion, on a request and hitting the race-window (known block, no filters yet) we could remember the peer awaiting response, move on to other work happening and once the filters are constructed send a response.

Something like PeerManagerImpl::ProcessPendingCFilterRequests.

This will not block other p2p messages while we wait for filters being constructed.

However I'm not sure that it's worth, as with the window gap being small enough we'd save only fractions of second. Additionally we could use WakeMessageHandler to avoid waiting next loop iteration (up to 100ms).

Does that sound right to you?

ajtowns commented at 9:03 AM on July 29, 2026: contributor

On the other hand, instead of blocking the caller (msghand) on SyncWithValidationInterfaceQueue completion, on a request and hitting the race-window (known block, no filters yet) we could remember the peer awaiting response, move on to other work happening and once the filters are constructed send a response.

That seems like a plausible approach to me, fwiw. Just set a flag in ProcessMessage when you receive the request that this peer wants a a cfilter response, then in SendMessages attempt to handle the cfilter requests, deferring it to a later call if the block is pending.

That will mean cfilter responses for that peer may be out of order vs other messages on that connection, but I think that's okay?

mzumsande commented at 10:13 AM on July 29, 2026: contributor

The problem formulation in the OP is not precise: Currently, when we receive a block, we first build the filter and then advertise the block, so there usually is no race.The race can only happen if the client asks for the newest headers themselves unprompted. This could happen because they just newly connected, or because they do active getheaders polling for some other reason. But this explains why this is very rare.

In my opinion, both a SyncWithValidationInterfaceQueue call in net_processing (it's not like we have anything more urgent to do than processing the block anyway?) and to schedule it for later sound fine.

randomlogin commented at 1:31 PM on July 29, 2026: none

The problem formulation in the OP is not precise: Currently, when we receive a block, we first build the filter and then advertise the block, so there usually is no race.

OP is precise for BIP152 block transmission, when we go through BIP152, we transmit compact block header to peers before we run BlockConnected which internally calls filters construction. (To be accurate, I wrongly called it advertising, but rather it is an actual sending of the header).

Also I took a moment to review how other requests are handled when we announced compact block header:

if a peer asks getheaders in the moment we have not yet connected the block, they will receive stale response
getdata for a block - we synchronously connect the block and then respond (which blocks all other peers for the time block connects)

Then maybe it is better to have some kind of a unified approach for both getdata and getcfilters, for example both block and wait for block connection/filters constructions to complete?

Deferring and responding later does not increase blocking time, and personally is more appealing and I'd go this way.

mzumsande commented at 1:40 PM on July 29, 2026: contributor

OP is precise for BIP152 block transmission

But how does this make sense / how is this a relevant use case? To be chosen as a high-bandwidth block relay peer, you need to be a full node. In this case you don't need to download the blockfilters for new blocks from peers - this is a feature meant for light clients.

randomlogin commented at 2:39 PM on July 29, 2026: none

@mzumsande you are right, I confused BIP152 with BIP130 (light clients use BIP130). And as you pointed, this bug cannot happen with BIP130 header transmission.

ajtowns commented at 6:06 AM on July 30, 2026: contributor

In my opinion, both a SyncWithValidationInterfaceQueue call in net_processing (it's not like we have anything more urgent to do than processing the block anyway?)

My understanding is that the way we currently work is:

send the compact block info to peers that have requested high bandwidth cb relay ASAP
pause the message handling thread to validate the block
queue the cfilter updates for another thread
send headers and inv messages to non-cb and low-bandwidth cb peers
resume the message handling thread
message handling thread "immediately" processes a cfilter request, and we attempt to respond before the cfilter updates have finished

Apart from the hb compact block path, we don't announce blocks until we've validated them, but we do announce them before we've calculated their compact filter.

I don't think we should block the message handling thread further, because we do have more important things to do: responding to getblocktxn requests that came in while we were validating the block (ideally we'd be responding to those requests while validating the block), and getdata requests that came in after our headers message.

Those "immediate" requests could come from peers that selected us as hb for cb, or could just be a very quick response to a headers announcement compared to the calculations required to update the compact filters. I think those calcs are slow enough that this is likely to occur occasionally even for peers that aren't getting the high-bandwidth compact block path, so is worth handling, but it would probably good to confirm the exact flow and timings? (How long does calculating the compact filters actually take? How much does that vary across blocks? What's the real turn-around time for real compact filter clients between seeing a header and requesting the compact filter?)

mzumsande commented at 12:19 PM on July 30, 2026: contributor

Yes, that makes sense - I hadn't thought of getblocktxn requests.

send headers and inv messages to non-cb and low-bandwidth cb peers

resume the message handling thread

I'd say it's

queue the headers and inv message to non-cb and low-bandwidth cb peers for the other thread (UpdatedBlockTip())
resume the message handling thread which sends out headers and inv messages to non-cb and low-bandwidth cb peers once the queue entry has been processed

bench: log block filter construction timing

Log per-block construction and write timing for the block filter
index under the bench log category, to gather real data on how long
filter construction takes on a running node.

b1d4868485

randomlogin force-pushed on Aug 5, 2026

randomlogin commented at 1:30 AM on August 5, 2026: none

Changes

Added deferring of a response, we store the racing request in a PendingCFilterRequest struct in a Peer and then check it in SendMessages.

To avoid code repetition, added a helper TryRespondToCFilterRequest which is used in both original ProcessGetCFilters and SendMessages handlings of a request.

Rewrote the test.

If we receive two requests while the first one has not yet been responded, we do not overwrite it, thus responding only to the first one.

Benchmarks

Added benchmarks commit (b1d4868485770777e46950460852d61f9ad4b6c2) to this PR (I'll squash it later).

Here is the data for the initial filter construction since genesis as well as for processing during normal operation, i.e. I have an already running node in which I turned on filters and measured the times. Initial blocks' measurements are shown over latest 100k blocks, since ~861k).

Metric	mean	median	p90	p99	max
Cfilters construction (from 861k initial)	4.50ms	4.50ms	6.74ms	8.04ms	112.86ms
Cfilters construction (212 live blocks)	5.92ms	6.10ms	8.35ms	11.33ms	13.96ms
Connect block (212 live blocks)	172.85ms	51.14ms	63.89ms	4408.39ms	10423.97ms

Maximum time (112 ms for filters construction) is perhaps some I/O problem or something else not connected to normal behaviour. All times include I/O.

We can say filters processing takes around 10% of block processing.

Computer that runs benchmarks has the following specs:

CPU: Intel Xeon E5-2686 v4 (Broadwell) — 2 vCPUs, 2.30GHz, 1 thread/core
RAM: 3.8GB

Going further

Currently only added handling getcfilters, but the same bug stands for other bip157 requests: getcfheaders and getcfcheckpt. It is even less likely for them to encounter a race, do we want them handled in the same way?
Should we call m_connman.WakeMessageHandler() in block connection, or the current approach is fine?

net: defer response to racing cfilters request

Previously a compact filters request might falsely be ignored because the
requested block has been connected to the chain, but the filters have
not yet been constructed.

This commit adds deferring of such racing request. We write the pending
request using newly added `PendingCFilterRequest` struct and modify
`SendMessages` to check whether the request is awaiting a response. This
way `msghand` is not blocked and is able to process other messages.

Checking if we have a race condition is done via a newly added
`IsRacing` function on `BaseIndex` and a tuned version for the CFilters
index.

As we might also respond to a peer with filters in `SendMessages` as
well as in the "ordinary" handler `ProcessGetCFilters`, a new helper
`TryRespondToCFilterRequest` is introduced to avoid repeating code.

Added a test for this race condition.

Added `CF_MAX_BLOCKS_AHEAD_RACE_WAIT=2` constant.

21c65a024c

randomlogin force-pushed on Aug 5, 2026

DrahtBot added the label CI failed on Aug 5, 2026

DrahtBot removed the label CI failed on Aug 6, 2026

randomlogin requested review from mzumsande on Aug 10, 2026

in src/index/blockfilterindex.cpp:268 in b1d4868485

 263 | +
 264 | +    ++m_num_blocks_total;
 265 | +    m_time_construct += time_filter - time_start;
 266 | +    m_time_header += time_header - time_filter;
 267 | +    m_time_write += time_write - time_header;
 268 | +    m_time_total += time_write - time_start;

ajtowns commented at 2:09 AM on August 11, 2026:

The benchmarking commit should be removed, not squashed, presumably? It doesn't seem like it'll be useful beyond developing this PR?

I think this PR should be marked as draft if it has commits that need updating/replacing prior to merge.

in src/net_processing.cpp:266 in 21c65a024c

 261 | +        BlockFilterType filter_type;
 262 | +        uint32_t start_height;
 263 | +        uint256 stop_hash;
 264 | +    };
 265 | +    /** At most one outstanding deferred cfilter request at a time. */
 266 | +    std::optional<PendingCFilterRequest> m_pending_cfilter_request GUARDED_BY(NetEventsInterface::g_msgproc_mutex);

ajtowns commented at 2:11 AM on August 11, 2026:

Rather than an optional, I'd suggest considering std::list<PendingCFilterREquest> and using push_back to add requests to the end, and front to get the oldest request in order to attempt to service it. Allows handling multiple requests, though still needs a limit to avoid growing unbounded. Also wastes a little less memory than an optional if there's nothing queued, which should be true for most peers for most of the time.