#35354 net: wait for validation queue flush for missing compact filter for an already known block
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35354 net: wait for validation queue flush for missing compact filter for an already known block | Ready | 0 | Strong | 176 + 387 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Defers serving BIP 157 compact filter requests (getcfilters, getcfheaders, getcfcheckpt) in net_processing when a requested block is known to the active chain but the block filter index has not yet caught up to it. When a race condition is detected via the filter index summary, the request is stored in a per-peer pending request variant, pausing subsequent message processing for that peer. Once the index catches up or covers the requested block hash, ProcessMessages retries and serves the parked request before resuming normal message handling.
Problem: When Bitcoin Core connects a new block, it advertises it to peers before the background validation interface queue has constructed the compact filter. If a BIP 157 light client requests the filter or filter headers for the announced block during this window, Core silently ignores the request. Under BIP 157, clients interpret non-response for an advertised block as protocol misbehavior and disconnect or ban the node.
Evidence of importance:
- Fixes issue #29655: 'CFilters reject and disconnect peers with a valid block hash'
- Linked to issue #27085 and discussion on lightninglabs/neutrino#338 where light clients encounter disconnects against Core nodes announcing new blocks
Scope: The PR description describes an alternative approach based on BaseIndex::WaitForRacingWrite and validation queue flushing, but the actual diff and commits implement per-peer deferred request parking in net_processing.cpp.
Risk of a mistake: medium (p2p). A flaw in the peer message-pausing logic or resume condition could stall message processing for peers or create a denial-of-service vector.
Changed since the previous assessment: Added an Initial Block Download guard in CFilterIndexMayBeRacing to avoid parking filter requests during IBD.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- author: address ajtowns' suggestions regarding PendingRequest timestamps, retry timers, comment updates, and updating the PR description 2026-10-31
Agreement
Strong
substantive support, no open objection (ajtowns, mzumsande)
Thread notes: ajtowns reviewed the updated implementation, approving the approach with 'Looks good to me' while leaving several suggestions regarding pending request timestamps, parked peer retry intervals, comment updates, and updating the PR description.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| sedited (member) | objection, approach | Handling peer race conditions inside BaseIndex couples indexing code to P2P transport concerns and risks stalls in other callers | fixed · fix pushed · author moved the handling out of the index into net_processing | changed | yes | 2026-06-21: 'Reading through this, I don't think this should be handled on the index level.' Status from: 2026-08-05: randomlogin: 'Added deferring of a response, we store the racing request in a PendingCFilterRequest struct in a Peer and then check it' |
| ajtowns (member) | suggestion, maintenance | Remove the benchmarking commit instead of squashing it, and mark PR as draft while commits need replacing | fixed · fix pushed · benchmark commit removed | answered | yes | 2026-08-11: 'The benchmarking commit should be removed, not squashed, presumably? It doesn't seem like it'll be useful beyond developing this PR?' Status from: 2026-08-14: randomlogin: 'Removed benchmark commit.' |
| ajtowns (member) | suggestion, interface | Use std::list instead of std::optional for pending cfilter requests to handle multiple requests and save memory | fixed · fix pushed · switched to std::list with a limit | answered | yes | 2026-08-11: 'Rather than an optional, I'd suggest considering std::list<PendingCFilterREquest> and using push_back to add requests to the end' Status from: 2026-08-14: randomlogin: 'Changed an optional in favor of a list with up to MAX_PENDING_CFILTER_REQUESTS=2 entries.' |
| mzumsande (member) | objection, interface | Touching index internals violates separation of concerns when GetSummary already exposes sufficient state | fixed · fix pushed · author removed IsRacing and used GetSummary without touching index code | changed | yes | 2026-08-11: 'Races in net_processing shouldn't be the business of indexes, and GetSummary() should have enough info... so I'd prefer if the index code was not touched at all' Status from: 2026-08-14: randomlogin: 'Removed IsRacing, now using GetSummary. No changes in GetSummary were required.' |
| ajtowns (member) | objection, scope | Handle getcfheaders and getcfcheckpt the same as getcfilters since disconnects happen frequently for getcfheaders | accepted · fix pushed · author implemented uniform handling across all three messages and ajtowns approved the revised approach | answered | yes | 2026-08-16: 'Based on the logs in #29655 this seems backwards -- there were more disconnects for getcfheaders than getcfilters? So I think this ought to be handled for all the cases?' Status from: 2026-10-31: ajtowns: 'Looks good to me, I think.' |
| ajtowns (member) | suggestion, approach | Pause the peer entirely on a pending cfilter request instead of queueing cfilter responses and continuing to process other messages | accepted · fix pushed · author adopted the pausing approach and ajtowns approved the revised code | answered | yes | 2026-08-16: 'I wonder if it wouldn't be better to adopt the "pause this peer" approach, rather than queueing messages but continuing.' Status from: 2026-10-31: ajtowns: 'Looks good to me, I think.' |
| ajtowns (member) | suggestion, maintenance | Update the PR description to match the revised approach | open | answered | no | 2026-10-31: 'PR description needs an update.' |
| ajtowns (member) | suggestion, maintenance | Update the comment on CfilterIndexMayBeRacing (and consider renaming to ShouldDeferCFilterRequest) to reflect behavior during IBD | open | answered | no | 2026-10-31: 'You should update the comment on the declaration of `CfilterIndexMayBeRacing` to match this change in behaviour.' |
| ajtowns (member) | suggestion, interface | Add req_time to PendingRequest struct to debug delays and abort requests that sit idle for an extended period | open | answered | no | 2026-10-31: 'so that we can more easily debug how long a request was delayed, and automatically abort/drop requests that sit idle for an extended period is probably a good idea.' |
| ajtowns (member) | suggestion, approach | Consider adding a retry timer to only retry parked peers when ~100ms has elapsed to avoid blocking message processing if cs_main is held | open | answered | no | 2026-10-31: 'Could have two timers associated with the request so that as well as tracking when the peer was parked, we only do retries of parked peers when it's been 100ms or so since the last retry.' |
Support:
- ajtowns (member) (2026-07-30) [read]: Compact filter calculations are slow enough that races occur occasionally even for peers not on high-bandwidth compact block relay, so the issue is worth handling
- mzumsande (member) (2026-07-30) [read]: Agrees that handling racing compact filter requests in net_processing makes sense to prevent disconnects and avoid delaying getblocktxn responses
- ajtowns (member) (2026-10-31): Looks good to me, I think [not substantive]
Participants:
- sedited (member, objection, 2 statements 2026-06-21 to 2026-07-24): objected to handling the race at the index level, prefer handling in net_processing
- ajtowns (member, objection, 11 statements 2026-07-29 to 2026-10-31): objected to missing getcfheaders/getcfcheckpt and suggested pausing peer; now supportive of the approach with suggestions on timers, structs, and comments
- mzumsande (member, objection, 4 statements 2026-07-29 to 2026-08-11): supported addressing the race in net_processing but objected to modifying index internals when GetSummary suffices
Files
387 lines under test/bench/ci.
- src/test/cfilter_race_tests.cpp +386/-0
- src/net_processing.cpp +176/-13
- src/test/CMakeLists.txt +1/-0
Uncertainties
- The PR description outlines a BaseIndex::WaitForRacingWrite mechanism that flushes the validation queue, which contradicts the patch and commits implementing per-peer message deferral in net_processing.
Card
The short text the ranking pass compares across PRs.
This PR resolves a race condition where BIP 157 light clients request compact filters or filter headers for a newly connected block before the background validation queue builds the filter. Currently, Bitcoin Core drops these requests without responding, prompting peers like Neutrino to treat the node as misbehaving and disconnect or ban it (issues #29655 and #27085). The change updates net_processing to detect when the filter index is racing the active chain tip and parks the request in a per-peer slot, pausing that peer's message processing until the filter index catches up. Requests are bypassed and not parked during Initial Block Download.
Data
thread read through 2026-10-31 (28 statements), head 0dc77bcacf
labels: P2P
Categories
P2P
P2 · bug fix
Fixes a network interoperability bug where BIP 157 light clients disconnect or ban Bitcoin Core nodes after requesting filters for newly announced blocks. Under BIP 157, clients interpret non-response for an advertised block as protocol misbehavior, which is a significant recurring issue for light client ecosystems.
Membership: Has the P2P label and modifies message processing in src/net_processing.cpp to park racing compact filter requests.
Factors: security/stability 1, bug 2, performance 0, user value 2, leverage 0