#35558 p2p: Prefill compact blocks
| Reviewability | Reviews | Agreement | Size | |||||||
|---|---|---|---|---|---|---|---|---|---|---|
| #35558 p2p: Prefill compact blocks | Needs rebase | 0 (+2) | Strong | 548 + 83 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
Implements predictive prefilling for BIP 152 compact blocks, bounded dynamically by the peer connection's TCP window. When relaying a compact block, the node pre-includes transactions that were missing from its own mempool during reconstruction (retrieved via getblocktxn, pulled from the extrapool, or prefilled by upstream peers). A prefilled block is only sent if it fits within the same number of TCP window boundaries as an un-prefilled block, querying the OS socket layer across Linux, macOS, the BSDs, and Windows to determine current window and queue sizes.
Problem: Nodes currently prefill only coinbase transactions in compact blocks, requiring peers to spend an application round-trip requesting missing transactions via getblocktxn. This extra hop delays network-wide block propagation, elevating orphan risks and stale block rates. Unbounded prefilling would risk overflowing TCP congestion windows and introducing network-layer transmission delays on redundant transactions.
Evidence of importance:
- In live network measurements from 2026-02-15 to 2026-03-16, TCP-window-bounded prefilling raised immediate compact block reconstruction from 56.9% to 89.7% without a getblocktxn round-trip (PR description).
- Redundant bandwidth consumption averaged 865 bytes per block, representing roughly 1.2 to 1.4 MiB per node per day across three high-bandwidth peers (PR description).
Scope: Adds OS-specific socket queries (ioctl, getsockopt, SIO_TCP_INFO) across Linux, macOS, FreeBSD, NetBSD, OpenBSD, and Windows to inspect TCP window sizes and in-flight bytes, which bumps the Windows build target to Windows 10 version 1703.
Risk of a mistake: high (p2p). Bugs in compact block serialization, candidate tracking, or OS socket window calculations could stall block propagation, cause protocol desync, or trigger peer disconnects.
Changed since the previous assessment: Prerequisite PR #35724 has merged, removing external dependencies.
Reviewability
Ready · Needs rebase
Conflicts with master; review the logic now, the rebase is mechanical.
Waiting on (from the thread read):
- author: author marked PR as draft pending merge of logging prerequisite PR 35724 2026-10-09
- reviewer: andrewtoth to respond regarding std::launch::async vs deferred serialization 2026-07-14
- author: prerequisite PR #35724 to merge and PR to be taken out of draft 2026-10-09
Agreement
Strong
substantive support, no open objection (josibake, 0xB10C, johnnyasantoss, m4ycon, s00ly)
Thread notes: Added missing suggestions and questions raised across the thread during schema upgrade. All active objections and suggestions have been addressed or fixed, and logging changes were split into prerequisite PR 35724 on which this draft PR is rebased. / Added suggestions and questions identified across the thread during reread, including lock contention optimizations, logging improvements, and async serialization discussion. The author split logging changes out into PR #35724 and marked the PR as d
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| ismaelsadeeq (member) | suggestion, approach | Avoid sending prefilled extrapool transactions back to the peers that announced them | answered · author explained that transactions might have been evicted or RBF'd and per-peer tracking adds high complexity | answered | yes | 2026-06-18: 'We can reduce a bit of redundancy by not sending them again to the peers that announced them to us.' Status from: 2026-07-03: davidgumberg: 'Even if a peer has sent us a transaction that ends up in our extrapool, they may not have it in their mempool now' |
| 0xB10C (member) | suggestion, scope | Log information about wasted prefill bandwidth on the receiving side | fixed · fix pushed · author added redundant prefill logging to the PR | answered | yes | 2026-06-19: 'An idea mentioned in the delving post was to log information about wasted prefill bandwidth on the receiving side.' Status from: 2026-07-03: davidgumberg: 'That's a great point, I had logic for this in my observation nodes, but I've incorporated the changes into this PR.' |
| 0xB10C (member) | objection, safety | Logging all required transactions in non-test settings could cause log rate-limiting | fixed · fix pushed · author replied noting trace and debug logs are not rate limited, and switched to trace logging | answered | yes | 2026-06-19: 'Could this cause log rate-limiting? Maybe a good use for the trace log level?' Status from: 2026-06-30: davidgumberg: 'Trace and Debug level logs are not rate limited' |
| johnnyasantoss (none) | suggestion, style | Prefer WITH_LOCK when reading send_state | fixed · fix pushed · author applied the suggestion | answered | yes | 2026-06-27: 'Since we are just reading the send_state why not prefer WITH_LOCK?' Status from: 2026-07-03: davidgumberg: 'Good catch, fixed, thank you.' |
| andrewtoth (member) | suggestion, approach | Hoist cb_fut and prefilled_cb_fut out to release m_most_recent_block_mutex before executing syscalls | fixed · fix pushed · author released the lock earlier | answered | yes | 2026-06-27: 'I think we can hoist cb_fut and prefilled_cb_fut out and copy the shared futures while m_most_recent_block_mutex is held, then release the lock for the rest of this method.' Status from: 2026-07-03: davidgumberg: 'Nice! Much better to not hold a lock that prevents block updates while doing syscalls!!' |
| andrewtoth (member) | suggestion, approach | Avoid switching serialization launch policy from deferred to async | answered · reviewer edited to note eager background serialization makes sense for prefill, and author asked if resolved | answered | yes | 2026-07-03: 'I don't think we should be switching the serialization from std::launch::deferred to std::launch::async.' Status from: 2026-07-14: davidgumberg: 'Yeah, sorry if it's weird to combine both changes in one commit, I can split up if you think that makes sense.' |
| andrewtoth (member) | question, approach | When will the block hash in prefill_candidates differ from hash, and can they be merged? | answered · author explained candidates update on reconstruction but the block might still fail in ProcessBlock | answered | yes | 2026-07-03: 'When will the block hash in prefill_candidates differ from hash above? Can these be merged?' Status from: 2026-07-14: davidgumberg: 'right now the issue is that we'll update the prefill candidates on reconstruction, but the block might still fail somewhere in ProcessBlock()' |
| andrewtoth (member) | suggestion, style | Remove duplicate logging of window_size and bytes_available | fixed · fix pushed · author removed the caller-side logging | answered | yes | 2026-07-03: 'We log window_size and bytes_available here, but we also log them on the call side. Can we remove one of the logs?' Status from: 2026-07-14: davidgumberg: 'Ok, I removed it on the caller side.' |
| andrewtoth (member) | objection, correctness | Seeding prefill candidates with the coinbase forces unnecessary TCP syscalls and prefill branch execution even when no extra transactions are prefilled | fixed · fix pushed · author changed the condition to require more than one candidate | answered | yes | 2026-07-03: 'In that case, the check in net_processing for prefill_candidates.size() > 0 is always true, so we will always go through the prefill path and do all the syscalls even for the current behavior of alway' Status from: 2026-07-14: davidgumberg: 'Great catch, thank you, I took this by changing the check to be prefill_candidates.size() > 1' |
| andrewtoth (member) | suggestion, approach | Avoid taking and releasing m_send_mutex multiple times by returning early | fixed · fix pushed · author refactored lock handling and header size method | answered | yes | 2026-07-03: 'we should take m_send_mutex lock and return the value if send_state != SendState::v1, and then return the fallback after releasing the lock.' Status from: 2026-07-14: davidgumberg: 'Taken thanks, this also made me realize GetMessageHeaderSize shouldn't be virtual, public, or take a lock, so fixed that as well.' |
| andrewtoth (member) | objection, correctness | Use-after-free by resetting txn_available before retrieving its size | fixed · fix pushed · author fixed the use-after-free | changed | yes | 2026-07-03: 'This is a use-after-free. We need to get the size before we reset it 3 lines above.' Status from: 2026-07-03: davidgumberg: 'Thanks for catching, sorry the quality of this logging commit was low when I pushed it.' |
| andrewtoth (member) | objection, correctness | Logging fails to increment prefilled_size and fails to adjust extra_size | fixed · fix pushed · author acknowledged and resolved logging omissions | changed | yes | 2026-07-03: 'Also need prefilled_size += tx_size; We don't increment/decrement extra_size either. Seems like an oversight?' Status from: 2026-07-03: davidgumberg: 'Thanks for catching, sorry this went out broken.' |
| m4ycon (none) | question, correctness | Why redundant mempool stats are handled under a not-in-mempool condition | fixed · fix pushed · fixed when logging was extracted into PR 35724 and rebased | answered | by others | 2026-07-20: 'why inside this condition !pool->exists(tx_wtxid) which means "not in mempool", there's a mempool variable redundant_prefilled_mp_... handling?' Status from: 2026-07-28: m4ycon: 'I'll let both my comments open, although I see they are fixed in #35724.' |
| m4ycon (none) | objection, correctness | prefilled_size is never incremented, resulting in inaccurate reconstruction log stats | fixed · fix pushed · author resolved by rebasing on top of logging PR 35724 | answered | yes | 2026-07-27: 'the value is always zero. Which could not be true in any scenario as coinbase is always prefilled. I couldn't find any assignment to it, probably it's a simple fix.' Status from: 2026-08-04: davidgumberg: 'Thanks, I've rebased this on #35724, so marking as resolved.' |
Support:
- w0xlt (contributor) (Concept ACK, 2026-06-18): Concept ACK [not substantive]
- ismaelsadeeq (member) (Concept ACK, 2026-06-18) [read: extrapool prefill heuristics]: Concept ACK and expressed interest in prefill recipient logic [not substantive]
- josibake (member) (Concept ACK, 2026-06-18): simplicity and backwards compatibility of the approach make this a very strong proposal
- edilmedeiros (contributor) (Concept ACK, 2026-06-18): Concept ACK [not substantive]
- murchandamus (member) (Concept ACK, 2026-06-18): Concept ACK, Very excited to see this PR. [not substantive]
- 0xB10C (member) (Concept ACK, 2026-06-19) [read: mainnet reconstruction telemetry, wasted bandwidth logging]: thanks for picking this idea up and moving it closer to the finish line
- johnnyasantoss (none) (ACK, 2026-06-27) [read: src/net.cpp]: utACK after group deep dive on compact block relay BIP
- m4ycon (none) (Tested ACK, 2026-07-16) [tested]: tACK 9c3a0a5; ran node and collected telemetry confirming prefill effectiveness in research repo
- s00ly (none) (Tested ACK, 2026-07-22) [both: src/blockencodings.cpp, src/net_processing.cpp, test/functional/p2p_compactblocks.py, test/functional/p2p_compactblocks_blocksonly.py, test/functional/p2p_compactblocks_hb.py]: tACK 9f36a29393663bb2a70d0f0690ec30f2176f6ef6; ran unit tests and compact block python tests across v1 and v2, and verified rebase on master
Participants:
- edilmedeiros (contributor, support, 2 statements 2026-06-18 to 2026-06-19): Concept ACK; suggested pre-merge testing of prefill tuning
- ismaelsadeeq (member, support, 1 statements 2026-06-18 to 2026-06-18): Concept ACK; suggested avoiding redundant prefills to announcing peers
- josibake (member, support, 1 statements 2026-06-18 to 2026-06-18): Concept ACK praising simplicity and backwards compatibility
- murchandamus (member, support, 1 statements 2026-06-18 to 2026-06-18): Concept ACK
- w0xlt (contributor, support, 1 statements 2026-06-18 to 2026-06-18): Concept ACK
- 0xB10C (member, support, 4 statements 2026-06-19 to 2026-07-28): Concept ACK; raised log rate-limiting concern and suggested telemetry for wasted prefill bandwidth
- johnnyasantoss (none, support, 3 statements 2026-06-26 to 2026-06-27): utACK after group review; suggested using WITH_LOCK
- andrewtoth (member, objection, 9 statements 2026-06-27 to 2026-07-03): raised objections on UAF, broken logging counters, and TCP syscalls when seeding coinbase, plus locking/threading suggestions
- m4ycon (none, support, 5 statements 2026-07-16 to 2026-07-28): tACK with live telemetry data; raised logging inaccuracies which were split into PR 35724
- s00ly (none, support, 1 statements 2026-07-22 to 2026-07-22): tACK across unit and functional test suites with rebase on master
Files
137 lines under test/bench/ci.
- src/net_processing.cpp +160/-50
- src/util/sock.h +130/-0
- src/net.cpp +94/-0
- src/blockencodings.cpp +49/-39
- src/test/fuzz/cmpctblock.cpp +19/-40
- src/test/sock_tests.cpp +49/-4
- src/util/sock.cpp +46/-1
- src/net.h +29/-0
- src/compat/compat.h +27/-1
- src/test/blockencodings_tests.cpp +10/-7
- src/blockencodings.h +12/-1
- src/bench/blockencodings.cpp +2/-1
- src/test/fuzz/p2p_headers_presync.cpp +1/-1
- src/test/fuzz/partially_downloaded_block.cpp +1/-1
- CMakeLists.txt +1/-0
- src/test/util/net.cpp +1/-0
Card
The short text the ranking pass compares across PRs.
Predictively prefills BIP 152 compact blocks with transactions the node lacked in its mempool during reconstruction (requested via getblocktxn, pulled from the extrapool, or received as upstream prefills). Prefill transmissions are bounded by OS TCP window queries so that prefilling never pushes a message past a TCP window boundary. In author measurements, this improved single-hop compact block reconstructions from 56.9% to 89.7% at an overhead of approximately 1.4 MiB bandwidth per day. Because it conforms strictly to BIP 152, older peers benefit without software updates.
Data
thread read through 2026-10-09 (47 statements), head 5871f8666d
labels: P2P, Needs rebase · draft
Categories
P2P
P2 · speedup
Predictive prefilling bounded by TCP window size dramatically cuts block propagation delays across the network. Live telemetry showed that TCP-window-bounded prefilling raised immediate compact block reconstruction from 56.9% to 89.7% without a getblocktxn round-trip, avoiding propagation delays that contribute to stale blocks.
Membership: Carries the P2P label and modifies block relay in src/net_processing.cpp, src/net.cpp, and src/blockencodings.cpp.
Factors: security/stability 1, bug 0, performance 2, user value 1, leverage 1
Utilities (logging, arguments, libraries)
P3 · new feature
Extends the Sock utility class to query TCP window and queue sizes across Linux, macOS, the BSDs, and Windows. This provides worthwhile new socket-inspection capabilities, though its immediate application is specialized to P2P block transport.
Membership: Adds cross-platform socket inspection methods in src/util/sock.h and src/util/sock.cpp.
Factors: security/stability 1, bug 0, performance 2, user value 1, leverage 1