Make BlockStager the single staged-body store - #1156
metaphorics wants to merge 2 commits into
Conversation
The download window kept a shadow copy of every staged body: a received map with its own byte total and a stored height. A body that arrived before its header, or without a pending request, was recorded at height 0. When that entry was later pruned, evicted, or escalated, the retry path lowered the request cursor to 0, so the cursor collapsed to genesis. Fourteen update sites kept the mirror in step with the stager, and two repair passes rewrote the 0 heights once the tree could resolve them. Delete the mirror. The window now reads staged identity, count, and bytes from the BlockStager it is handed, and the block tree is the only source of a staged body's height. requeue_for_retry replaces the drop paths: it releases the pending and lowers the cursor only to a real tree height, or to the height the released pending recorded, so a body of unknown height never moves the cursor. A new regression test delivers an unsolicited body the tree cannot place, lets expiry prune it, and asserts the cursor did not move.
Two staging defects remained once the stager owned every staged body. A branch retarget released the losing branch's pending requests but left its staged bodies in the stager until they expired, so the freed capacity was not real, and a late delivery from the old branch staged again beside them. Any delivered body staged, requested or not. An unrequested body far ahead of the chain, or on a side branch, could fill the stager and evict bodies that were requested and in flight. The retarget now discards every staged body that is not on the new request branch. A body that no connection has in flight now stages only when Core's AcceptBlock would process it with fRequested false (validation.cpp:4327-4353): it is on the header tip's branch, has at least the applied tip's work, the header tip meets the minimum chain work, and it is at most 288 blocks above the applied tip. A body whose header is not yet known keeps the existing missing-header path.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR successfully consolidates block body management by making BlockStager the single source of truth for staged bodies, removing the redundant received map and associated height tracking from DownloadWindow. The refactoring resolves the cursor collapse defect and aligns unrequested body admission with Bitcoin Core 31.1.
The changes are well-structured with:
- Clear separation of concerns: BlockStager owns staging, BlockTree owns heights
- Comprehensive test coverage including three new tests pinning the fixed behaviors
- Detailed migration path with all removed symbols documented
- Proper lock ordering maintained (tree → scheduler)
The implementation correctly handles the tree-owned height resolution pattern throughout, with appropriate fallback behavior when heights cannot be resolved (no cursor rewind instead of collapse to genesis).
No blocking issues identified.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 23 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/p2p/src/sync/commit.rs">
<violation number="1" location="crates/p2p/src/sync/commit.rs:209">
P1: Permanent window failures invalidate the failed block itself, but this unconditional retry requeues it at its tree height after `purge_invalidated` intentionally avoided a cursor rewind. That moves `next_request_height` back onto a permanently invalid branch and can repeatedly rescan or stall there; only requeue retryable failure dispositions.</violation>
</file>
<file name="crates/p2p/src/sync/tests/head_sync.rs">
<violation number="1" location="crates/p2p/src/sync/tests/head_sync.rs:270">
P3: The assertion was correctly removed, but the comment directly above it still claims "the window's delivery record must be dropped outright too: keeping it would re-queue a body whose header can never admit (C19)." The download window no longer keeps any delivery/received record (there is no `window.received_len()` anymore), so this half of the comment now describes removed behavior and will mislead future readers. Trim the comment to only the staged-body discard rationale.</violation>
</file>
<file name="crates/p2p/src/sync/tests.rs">
<violation number="1" location="crates/p2p/src/sync/tests.rs:1266">
P2: This regression test does not cover the documented chainwork gates: Regtest makes the minimum-work check vacuous, and every active-branch candidate is above the applied tip. Add cases with a nonzero minimum-work floor and a candidate whose work is below the applied tip so those clauses cannot regress silently.</violation>
</file>
<file name="crates/p2p/src/sync/tests/transitions_2.rs">
<violation number="1" location="crates/p2p/src/sync/tests/transitions_2.rs:174">
P3: The late-delivery call's return value is unchecked. `buffer_received_block_chunk` returns the count of bodies staged, found already staged, or rejected for a failed body/header binding; unrequested bodies that fail admission are discarded and not counted. Asserting `0` here would pin that the off-branch body was discarded rather than merely asserting the observable stager state stayed empty.</violation>
</file>
<file name="crates/p2p/src/sync/tests/transitions_4.rs">
<violation number="1" location="crates/p2p/src/sync/tests/transitions_4.rs:325">
P3: The test acquires the scheduler lock before the block-tree read lock, inverting the lock order used in production. `sync/peers.rs` (lines ~194-196) documents "Lock order tree -> scheduler" and takes `block_tree().read()` first because the stall predicate resolves staged hashes against tree heights. Swap the two lines to acquire the tree guard first, so the test mirrors the production convention and stays safe if the tree ever gains a concurrent writer.</violation>
</file>
<file name="crates/p2p/src/sync/receive.rs">
<violation number="1" location="crates/p2p/src/sync/receive.rs:69">
P3: This prune path re-acquires the block-tree read lock once per dropped body. Take the guard once above the loop and resolve all heights inside it, as the previous code did.</violation>
<violation number="2" location="crates/p2p/src/sync/receive.rs:480">
P3: The byte-budget-refused arm passes `None` to `requeue_for_retry` without consulting the tree, unlike the count-eviction arm which requeues at the tree-resolved height. A refused unsolicited body therefore has no pending, no staged entry, and no cursor move, so it is never fetched again unless stall machinery later rewinds the cursor beneath it. If that body is the canonical next chain, this just delays progress; if the permanent drop is intended, the inconsistency with the Memory arm is worth a comment, otherwise resolve the tree height here as well.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| self.scheduler | ||
| .lock() | ||
| .window | ||
| .requeue_for_retry(&hash, failed_height); |
There was a problem hiding this comment.
P1: Permanent window failures invalidate the failed block itself, but this unconditional retry requeues it at its tree height after purge_invalidated intentionally avoided a cursor rewind. That moves next_request_height back onto a permanently invalid branch and can repeatedly rescan or stall there; only requeue retryable failure dispositions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/p2p/src/sync/commit.rs, line 209:
<comment>Permanent window failures invalidate the failed block itself, but this unconditional retry requeues it at its tree height after `purge_invalidated` intentionally avoided a cursor rewind. That moves `next_request_height` back onto a permanently invalid branch and can repeatedly rescan or stall there; only requeue retryable failure dispositions.</comment>
<file context>
@@ -195,15 +195,18 @@ impl BlockSync {
+ self.scheduler
+ .lock()
+ .window
+ .requeue_for_retry(&hash, failed_height);
}
self.advance_expected_apply_cache(&applied_hashes, failed_hash.is_some());
</file context>
| /// 288 blocks above the applied tip. Requested bodies are not gated. | ||
| #[test] | ||
| fn unrequested_body_admission_matches_core_acceptance() -> Result<(), Box<dyn std::error::Error>> { | ||
| let (mut tree, blocks) = mined_chain(300, 0)?; |
There was a problem hiding this comment.
P2: This regression test does not cover the documented chainwork gates: Regtest makes the minimum-work check vacuous, and every active-branch candidate is above the applied tip. Add cases with a nonzero minimum-work floor and a candidate whose work is below the applied tip so those clauses cannot regress silently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/p2p/src/sync/tests.rs, line 1266:
<comment>This regression test does not cover the documented chainwork gates: Regtest makes the minimum-work check vacuous, and every active-branch candidate is above the applied tip. Add cases with a nonzero minimum-work floor and a candidate whose work is below the applied tip so those clauses cannot regress silently.</comment>
<file context>
@@ -1244,6 +1257,66 @@ fn apply_fixture_block(sync: &BlockSync, block: Block) -> Result<(), Box<dyn std
+/// 288 blocks above the applied tip. Requested bodies are not gated.
+#[test]
+fn unrequested_body_admission_matches_core_acceptance() -> Result<(), Box<dyn std::error::Error>> {
+ let (mut tree, blocks) = mined_chain(300, 0)?;
+ let fork_parent = tree
+ .lookup(Hash256::from(blocks[4].block_hash()))
</file context>
| "the inadmissible body must be discarded, not retried" | ||
| ); | ||
| assert_eq!( | ||
| sync.scheduler.lock().window.received_len(), |
There was a problem hiding this comment.
P3: The assertion was correctly removed, but the comment directly above it still claims "the window's delivery record must be dropped outright too: keeping it would re-queue a body whose header can never admit (C19)." The download window no longer keeps any delivery/received record (there is no window.received_len() anymore), so this half of the comment now describes removed behavior and will mislead future readers. Trim the comment to only the staged-body discard rationale.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/p2p/src/sync/tests/head_sync.rs, line 270:
<comment>The assertion was correctly removed, but the comment directly above it still claims "the window's delivery record must be dropped outright too: keeping it would re-queue a body whose header can never admit (C19)." The download window no longer keeps any delivery/received record (there is no `window.received_len()` anymore), so this half of the comment now describes removed behavior and will mislead future readers. Trim the comment to only the staged-body discard rationale.</comment>
<file context>
@@ -266,11 +266,6 @@ fn staged_body_with_permanently_inadmissible_header_is_discarded()
- "the discarded body's window record must not linger and re-queue"
- );
Ok(())
}
</file context>
| ); | ||
|
|
||
| let mut late = vec![crate::InboundBlock::from_decoded(losing2)]; | ||
| sync.buffer_received_block_chunk(&mut late, None); |
There was a problem hiding this comment.
P3: The late-delivery call's return value is unchecked. buffer_received_block_chunk returns the count of bodies staged, found already staged, or rejected for a failed body/header binding; unrequested bodies that fail admission are discarded and not counted. Asserting 0 here would pin that the off-branch body was discarded rather than merely asserting the observable stager state stayed empty.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/p2p/src/sync/tests/transitions_2.rs, line 174:
<comment>The late-delivery call's return value is unchecked. `buffer_received_block_chunk` returns the count of bodies staged, found already staged, or rejected for a failed body/header binding; unrequested bodies that fail admission are discarded and not counted. Asserting `0` here would pin that the off-branch body was discarded rather than merely asserting the observable stager state stayed empty.</comment>
<file context>
@@ -102,6 +136,57 @@ fn retargeting_pending_requests_drops_losing_branch_hashes()
+ );
+
+ let mut late = vec![crate::InboundBlock::from_decoded(losing2)];
+ sync.buffer_received_block_chunk(&mut late, None);
+ let scheduler = sync.scheduler.lock();
+ assert_eq!(
</file context>
| sync.buffer_received_block_chunk(&mut late, None); | |
| assert_eq!( | |
| sync.buffer_received_block_chunk(&mut late, None), | |
| 0, | |
| "the late losing-branch delivery must be discarded, not staged" | |
| ); |
| let mut scheduler = sync.scheduler.lock(); | ||
| let tree = sync.chain.block_tree().read(); |
There was a problem hiding this comment.
P3: The test acquires the scheduler lock before the block-tree read lock, inverting the lock order used in production. sync/peers.rs (lines ~194-196) documents "Lock order tree -> scheduler" and takes block_tree().read() first because the stall predicate resolves staged hashes against tree heights. Swap the two lines to acquire the tree guard first, so the test mirrors the production convention and stays safe if the tree ever gains a concurrent writer.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/p2p/src/sync/tests/transitions_4.rs, line 325:
<comment>The test acquires the scheduler lock before the block-tree read lock, inverting the lock order used in production. `sync/peers.rs` (lines ~194-196) documents "Lock order tree -> scheduler" and takes `block_tree().read()` first because the stall predicate resolves staged hashes against tree heights. Swap the two lines to acquire the tree guard first, so the test mirrors the production convention and stays safe if the tree ever gains a concurrent writer.</comment>
<file context>
@@ -321,11 +321,18 @@ fn stall_eviction_does_not_disconnect_replacement_connection()
- Instant::now() + Duration::from_millis(150),
- );
+ let selected = {
+ let mut scheduler = sync.scheduler.lock();
+ let tree = sync.chain.block_tree().read();
+ let state = &mut *scheduler;
</file context>
| let mut scheduler = sync.scheduler.lock(); | |
| let tree = sync.chain.block_tree().read(); | |
| let tree = sync.chain.block_tree().read(); | |
| let mut scheduler = sync.scheduler.lock(); |
| let requeues: Vec<(Hash256, Option<u32>)> = dropped | ||
| .iter() | ||
| .filter_map(|dropped| { | ||
| let node_id = tree.lookup(dropped.hash)?; | ||
| tree.node(node_id) | ||
| .ok() | ||
| .map(|node| (dropped.hash, node.height)) | ||
| .map(|dropped| { | ||
| let height = { | ||
| let tree = self.chain.block_tree().read(); | ||
| tree.lookup(dropped.hash) | ||
| .and_then(|node_id| tree.node(node_id).ok()) | ||
| .map(|node| node.height) | ||
| }; | ||
| (dropped.hash, height) | ||
| }) |
There was a problem hiding this comment.
P3: This prune path re-acquires the block-tree read lock once per dropped body. Take the guard once above the loop and resolve all heights inside it, as the previous code did.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/p2p/src/sync/receive.rs, line 69:
<comment>This prune path re-acquires the block-tree read lock once per dropped body. Take the guard once above the loop and resolve all heights inside it, as the previous code did.</comment>
<file context>
@@ -62,24 +63,24 @@ impl BlockSync {
+ // The tree owns heights: a pruned body requeues at its tree
+ // height, or without a cursor move when the tree cannot resolve
+ // it (the old 0-sentinel rewind to genesis is unrepresentable).
+ let requeues: Vec<(Hash256, Option<u32>)> = dropped
.iter()
- .filter_map(|dropped| {
</file context>
| let requeues: Vec<(Hash256, Option<u32>)> = dropped | |
| .iter() | |
| .filter_map(|dropped| { | |
| let node_id = tree.lookup(dropped.hash)?; | |
| tree.node(node_id) | |
| .ok() | |
| .map(|node| (dropped.hash, node.height)) | |
| .map(|dropped| { | |
| let height = { | |
| let tree = self.chain.block_tree().read(); | |
| tree.lookup(dropped.hash) | |
| .and_then(|node_id| tree.node(node_id).ok()) | |
| .map(|node| node.height) | |
| }; | |
| (dropped.hash, height) | |
| }) | |
| let requeues: Vec<(Hash256, Option<u32>)> = { | |
| let tree = self.chain.block_tree().read(); | |
| dropped | |
| .iter() | |
| .map(|dropped| { | |
| let height = tree | |
| .lookup(dropped.hash) | |
| .and_then(|node_id| tree.node(node_id).ok()) | |
| .map(|node| node.height); | |
| (dropped.hash, height) | |
| }) | |
| .collect() | |
| }; |
| window.drop_for_retry(&dropped.hash); | ||
| // Count-evicted before staging: release what the | ||
| // window holds without a cursor rewind. | ||
| window.requeue_for_retry(&dropped.hash, None); |
There was a problem hiding this comment.
P3: The byte-budget-refused arm passes None to requeue_for_retry without consulting the tree, unlike the count-eviction arm which requeues at the tree-resolved height. A refused unsolicited body therefore has no pending, no staged entry, and no cursor move, so it is never fetched again unless stall machinery later rewinds the cursor beneath it. If that body is the canonical next chain, this just delays progress; if the permanent drop is intended, the inconsistency with the Memory arm is worth a comment, otherwise resolve the tree height here as well.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/p2p/src/sync/receive.rs, line 480:
<comment>The byte-budget-refused arm passes `None` to `requeue_for_retry` without consulting the tree, unlike the count-eviction arm which requeues at the tree-resolved height. A refused unsolicited body therefore has no pending, no staged entry, and no cursor move, so it is never fetched again unless stall machinery later rewinds the cursor beneath it. If that body is the canonical next chain, this just delays progress; if the permanent drop is intended, the inconsistency with the Memory arm is worth a comment, otherwise resolve the tree height here as well.</comment>
<file context>
@@ -450,15 +470,14 @@ impl BlockSync {
- window.drop_for_retry(&dropped.hash);
+ // Count-evicted before staging: release what the
+ // window holds without a cursor rewind.
+ window.requeue_for_retry(&dropped.hash, None);
retry_count = retry_count.saturating_add(1);
tracing::warn!(%hash, "block sync: received block buffer full; dropping block for retry");
</file context>
1 participant
What and why
The block download window kept its own copy of every staged body: a
receivedmap with a byte total and a stored height per body. TheBlockStageralready held the same bodies. About fourteen update siteskept the two in step, and two repair passes fixed the copy afterwards.
The copy caused a live defect. A body that arrived before its header, or
that no connection had requested, was recorded at height 0. When that entry
was pruned, evicted, or escalated, the retry path lowered the request cursor
to
min(cursor, 0), so the cursor collapsed to genesis.This PR makes
BlockStagerthe only staged-body store and the block tree theonly source of a staged body's height. It also closes three sync findings.
Each is a place where this node differed from Bitcoin Core 31.1:
unrequested body only when
AcceptBlockaccepts it withfRequested == false(src/validation.cpp:4306-4357).requested and still in flight, and Core never lets an unrequested block
displace requested work (
src/validation.cpp:4327-4353).requests but kept its staged bodies, and Core requests only blocks on the
active branch (
src/net_processing.cpp:3104-3135).Commits
Each commit builds, passes clippy with
-D warnings, and passes the fullbitcoin-rs-p2ptest suite.Demolition manifest
Removed from
crates/p2p/src/download_window.rs:received: HashMap<Hash256, ReceivedBlock>field, theReceivedBlock { height, bytes }struct, and thereceived_bytesfieldreceived_len,received_height,update_received_height,reconcile_received_heights,mark_received_applied,drop_received_for_retry,drop_for_retry,discard_received,remove_received,mark_received, and the test helpermark_appliedRemoved from the sync executor:
reconcile_staged_received_heightsand both of its call sites(
sync/headers.rs,sync/receive.rs)mark_received_appliedloop insync/commit.rs, the height-updateprune loop and the stored-height pin in
sync/receive.rs, and thewindow-mirror calls in
sync/branches.rsandsync/peers.rsNo re-export, alias, forwarding function, or compatibility path keeps any
removed symbol.
Replacement
BlockStager::staged_hashes()yields the identity of every staged bodyand no heights.
methods take
&BlockStager(or&mut BlockStagerat the retarget) andread staged count, bytes, and membership from it.
DownloadWindow::requeue_for_retry(hash, height: Option<u32>)replacesthe drop paths. It releases the pending and lowers the cursor to the lower
of the given tree height and the height the released pending recorded.
With neither known, it does not move the cursor.
tree for each staged body's height.
Behavior changes
pruned, evicted, or escalated no longer rewinds the request cursor.
Pinned by
unsolicited_staged_body_never_rewinds_request_cursor.that is not on the new request branch. Before, only the window's copy was
dropped and the bodies stayed until they expired. Pinned by
retarget_purges_staged_off_branch_bodies.stages only when it is on the header tip's branch, has at least the
applied tip's chainwork, the header tip meets the network's minimum chain
work, and it is at most 288 blocks above the applied tip. A body whose
header is not yet known keeps the existing missing-header path. Pinned by
unrequested_body_admission_matches_core_acceptance.Base failure and head pass for the three new tests, run at the base commit
with the tests added unchanged and one test-only cursor accessor:
unsolicited_staged_body_never_rewinds_request_cursorretarget_purges_staged_off_branch_bodiesunrequested_body_admission_matches_core_acceptanceThe request path already clamps each scan at the next required height, so
at the base the collapsed cursor could not make a
getdataname analready-applied block. The defect is the cursor state itself, and the test
pins that state through a test-only accessor.
Persisted data, outputs, and configuration
node.sync.*metric names and gauge sources staythe same (
telemetry.rsalready read the stager), and RPC and wireresponses are unchanged.
Tests
now stages a real body in a paired stager. The stall fixtures
window_blocked_on_stallerandlimit_cycle_window_statenow return thatstager. Test hashes resolve through a 256-header test chain, so the tree
can place every staged test body.
(
witness_staging_gate.rs,validation_1.rs,head_sync.rs,behavior_2.rs,behavior_5.rs) and the height-0 pinuntracked_delivery_records_tree_height, which the new cursor testreplaces.
tick_caps_requests_at_staged_byte_headroomnow skips header traffic,because its staged fixture bodies carry headers with unknown parents and
the tick also sends a recovery
getheaders.Documentation
live_head_carried_e2e.rsandhead_sync_e2e.rskeep their wire assertions. Their comments no longerdescribe a height-0 sentinel or a repair pass.
Verification
Lane exit codes at the head commit:
Benchmarks:
sync_pipelineapply proxies (cargo bench -p bitcoin-rs-node --bench sync_pipeline --features kernel), 12 alternating runs of the parent commit4fa20314c5and thishead
94c530887c. Median (min to max), in ms:sync_pipeline_apply_proxysync_pipeline_apply_signed_spend_proxysync_pipeline_apply_spend_heavy_proxyThe host was not idle: parallel builds ran during the runs, and the
slowest run of a proxy took more than four times as long as its fastest. At
that noise level the medians cannot show a regression or its absence.
The criterion group
deterministic_initial_sync_proxy_deep_headers_pure_128_blockspanics at
crates/node/benches/sync_pipeline.rs:754("expected getdata:receiving on an empty channel") at this head, at the parent, and at
origin/main748b784, so it has no numbers. The panic is not caused bythis change.
Grep gates: every removed name returns zero hits in
crates/p2p/srcandbin/bitcoin-rs/tests. Theself.received_bytespattern still matches 10lines, all in
block_stager.rs. That is the stager's own byte total, and ithas the same count at the base.
CONSTRAINTS.md rows
every staged body, and the admission gate only removes staging paths.
the 3% cap. An idle-host rerun is owed before this row can be true.
bitcoin-rs-p2p, and every consumer is in this repository and migrated.Summary by cubic
Makes
BlockStagerthe single store for staged bodies and the block tree the only source of a staged body's height, fixing a sync bug where a body of unknown height could rewind the request cursor to genesis.Bug Fixes
Refactors
receivedmirror and now reads staged count, bytes, and membership from the stager.requeue_for_retryreplaces the old drop-and-reconcile paths.AcceptBlockwould process them (active branch, at least the applied tip's chainwork, within 288 blocks of it); requested bodies are unaffected.Written for commit 94c5308. Summary will update on new commits.