feat(network): verify Zakura header-sync commitment roots before persisting#458
feat(network): verify Zakura header-sync commitment roots before persisting#458p0mvn wants to merge 12 commits into
Conversation
And one more auto-invalidated finding. Analyzed 19 files, diff |
| (`TreeAuxRootHeightMismatch` / `validate_tree_aux_root_heights`) before the roots reach | ||
| state. State re-checks the count and alignment invariants in `CommitHeaderRange` | ||
| (`prepare_header_range_batch_with_roots`) as defense in depth, and never | ||
| (`TreeAuxRootHeightMismatch` / `validate_tree_aux_root_heights`) and authenticates the roots |
There was a problem hiding this comment.
Note to reviewer: most of these are AI notes. Please refer to PR description for my hand-written notes.
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit bb3687f. Configure here.
1774d10 to
8e1c1fe
Compare
| // The tree is behind this range's parent — a non-Zakura path (checkpoint/legacy | ||
| // sync) committed ahead of the header-commit frontier, so `best_header_tip` was | ||
| // bumped past where the tree is folded. This is the *only* reload trigger: rebuild | ||
| // the tree at the current frontier from durable state, then retry the range. In the | ||
| // normal Zakura path (header sync leading) this never fires. Only arm the in-flight | ||
| // guard once the reload action is actually queued — `dispatch_action` is a | ||
| // non-blocking `try_send` that drops on a full/closed channel, and arming the guard | ||
| // on a dropped send would suppress every future rebuild permanently (the guard is | ||
| // only cleared when the reload completes). A dropped send just leaves the guard | ||
| // clear so the next retry re-dispatches. | ||
| if !self.state.rebuild_in_flight | ||
| && self.dispatch_action(HeaderSyncAction::QueryBestHeaderHistoryTree { | ||
| verified_block_tip: self.state.verified_block_tip, |
There was a problem hiding this comment.
Note to reviewer: this is the reload on the hot path. In practice, it is never hit because in Zakura sync headers stay ahead of bodies.
| // Commitment-root work only runs through the VCT fast-sync handoff boundary. | ||
| // The root at `last_checkpoint` is confirmed by the next header, so root-carrying | ||
| // ranges stop once the frontier reaches `last_checkpoint + 1`. | ||
| let last_checkpoint = startup.last_checkpoint_height; | ||
| let root_regime_end = next_height(last_checkpoint).unwrap_or(last_checkpoint); | ||
| // Only persist roots for below-checkpoint heights this node forward-syncs but has | ||
| // not committed yet. | ||
| let root_region_floor = self.anchor.0.max(self.finalized_height); | ||
| let below_root_boundary = | ||
| root_region_floor < last_checkpoint && self.best_header_tip < root_regime_end; | ||
| let want_tree_aux_roots = below_root_boundary; | ||
|
|
||
| // Root-carrying ranges redeliver the tip header so its root can be confirmed. | ||
| let overlap_forward_range = below_root_boundary | ||
| && self | ||
| .best_header_parent_hash | ||
| .is_some_and(|_| self.best_header_tip > block::Height(0)); | ||
| let Some(start) = (if overlap_forward_range { |
There was a problem hiding this comment.
Note to reviewer: this is the key logic where we decide on the overlapping ranges and whether to fetch roots
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Tip bump clears overlap anchor
- FullBlockCommitted and NewBlockAccepted tip bumps now preserve and publish the committed header parent hash, and regression tests verify forward overlap remains anchored at the bumped tip.
Or push these changes by commenting:
@cursor push 5427871ad0
Preview (5427871ad0)
diff --git a/zebra-network/src/zakura/header_sync/reactor.rs b/zebra-network/src/zakura/header_sync/reactor.rs
--- a/zebra-network/src/zakura/header_sync/reactor.rs
+++ b/zebra-network/src/zakura/header_sync/reactor.rs
@@ -132,8 +132,12 @@
HeaderSyncEvent::FullBlockCommitted {
height,
hash,
- header: _,
- } => self.handle_full_block_committed(height, hash).await,
+ header,
+ } => {
+ let parent_hash = previous_height(height).map(|_| header.previous_block_hash);
+ self.handle_full_block_committed(height, hash, parent_hash)
+ .await
+ }
HeaderSyncEvent::NewBlockAccepted {
peer,
height,
@@ -444,7 +448,12 @@
self.publish_candidate_state();
}
- async fn handle_full_block_committed(&mut self, height: block::Height, hash: block::Hash) {
+ async fn handle_full_block_committed(
+ &mut self,
+ height: block::Height,
+ hash: block::Hash,
+ parent_hash: Option<block::Hash>,
+ ) {
self.state.pending_new_blocks.remove(&hash);
let _ = self.state.seen.insert(hash);
self.update_verified_block_tip(height, hash);
@@ -456,7 +465,7 @@
// left untouched — a below-checkpoint forward range that then finds the tree behind rebuilds it
// lazily (the single `MissingHeaderHistoryTree` reload path).
if height > self.state.best_header_tip {
- self.publish_best_tip(height, hash, None).await;
+ self.publish_best_tip(height, hash, parent_hash).await;
}
self.schedule().await;
}
@@ -480,7 +489,8 @@
self.state.schedule.mark_height_covered(height);
self.cancel_covered_outstanding();
if height > self.state.best_header_tip {
- self.publish_best_tip(height, hash, None).await;
+ let parent_hash = previous_height(height).map(|_| block.header.previous_block_hash);
+ self.publish_best_tip(height, hash, parent_hash).await;
}
let destinations = self.eligible_tip_destinations(&peer, height);
diff --git a/zebra-network/src/zakura/header_sync/tests.rs b/zebra-network/src/zakura/header_sync/tests.rs
--- a/zebra-network/src/zakura/header_sync/tests.rs
+++ b/zebra-network/src/zakura/header_sync/tests.rs
@@ -2805,6 +2805,48 @@
assert_no_commit_or_misbehavior(&mut fixture.actions).await;
}
+#[tokio::test(flavor = "current_thread")]
+async fn full_block_commit_tip_bump_keeps_overlap_anchor() {
+ let network = regtest_network();
+ let block = mainnet_block(&BLOCK_MAINNET_1_BYTES);
+ let hash = block.hash();
+ let height = block.coinbase_height().expect("test block has height");
+ let mut fixture = spawn_test_reactor(startup_for(
+ network.clone(),
+ (block::Height(0), network.genesis_hash()),
+ None,
+ ));
+ let peer_id = peer(144);
+
+ connect_peer(&fixture, peer_id.clone()).await;
+ fixture
+ .handle
+ .send(HeaderSyncEvent::FullBlockCommitted {
+ height,
+ hash,
+ header: block.header.clone(),
+ })
+ .await
+ .unwrap();
+ advertise_tip(
+ &fixture,
+ peer_id,
+ block::Height(0),
+ block::Height(5),
+ DEFAULT_HS_RANGE,
+ 1,
+ )
+ .await;
+
+ let (start_height, _count, want_tree_aux_roots) =
+ next_forward_get_headers(&mut fixture.actions).await;
+ assert_eq!(
+ start_height, height,
+ "tip bumps from full-block commits must keep overlap anchored at the bumped tip",
+ );
+ assert!(want_tree_aux_roots);
+}
+
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn inbound_unseen_valid_new_block_is_seen_and_forwarded_to_eligible_peers() {
let network = Network::Mainnet;
@@ -4389,6 +4431,49 @@
);
}
+#[tokio::test(flavor = "current_thread")]
+async fn accepted_new_block_tip_bump_keeps_overlap_anchor() {
+ let network = regtest_network();
+ let block = mainnet_block(&BLOCK_MAINNET_1_BYTES);
+ let hash = block.hash();
+ let height = block.coinbase_height().expect("test block has height");
+ let source = peer(145);
+ let mut fixture = spawn_test_reactor(startup_for(
+ network.clone(),
+ (block::Height(0), network.genesis_hash()),
+ None,
+ ));
+
+ connect_peer(&fixture, source.clone()).await;
+ fixture
+ .handle
+ .send(HeaderSyncEvent::NewBlockAccepted {
+ peer: source.clone(),
+ height,
+ hash,
+ block: block.clone(),
+ })
+ .await
+ .unwrap();
+ advertise_tip(
+ &fixture,
+ source,
+ block::Height(0),
+ block::Height(5),
+ DEFAULT_HS_RANGE,
+ 1,
+ )
+ .await;
+
+ let (start_height, _count, want_tree_aux_roots) =
+ next_forward_get_headers(&mut fixture.actions).await;
+ assert_eq!(
+ start_height, height,
+ "tip bumps from accepted NewBlock events must keep overlap anchored at the bumped tip",
+ );
+ assert!(want_tree_aux_roots);
+}
+
/// The root-carrying regime runs *through* the last checkpoint inclusive — the VCT handoff block at
/// `last_checkpoint` reads its own persisted root — and stops one block above. Confirming the root at
/// `last_checkpoint` needs the `last_checkpoint + 1` header, so the range is capped there.You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit d7394a5. Configure here.
|
@v12-auditor review |
And one more auto-invalidated finding. Analyzed 19 files, diff |
There was a problem hiding this comment.
overall makes sense! so I think I'm aligned. The optional request part is still a bit murky to me.
We also have backwards sync still, we're not using that and afaiu does not work here therefore we should just disable very explicitly.
I didn't see but definitely could have missed a test to confirm that we're not disconnecting from peers who don't send us roots but might be worthwhile. This to me could just be a decoding test to make sure we don't disconnect from the peer if no roots are sent.
ai findings I'replicated. Perhaps I can just open a new PR with the tests targeting this one? can optionally add fixes but would defer to you on how we fix precisely
1. Blocker — for your question about wedging; short-served ranges permanently wedge the scheduler. I re-verified this one by hand, it's real. The serving side now truncates headers to root coverage (header_sync_driver.rs:485 — a server can never serve the root for its own header tip, since that root is deliberately unpersisted). But on the requester: a successful-but-short response commits the short prefix, prune_covered never fires because the requested end was never covered, and there is no other success-path clear_assignment (only disconnect, Local commit-failure, and timeout clear it — reactor.rs:682 is the Local-only path). The stale assignment then blocks RangeScheduler::ensure's start-height dedupe (scheduler.rs:68-75) from ever queueing that start height again. Concrete case: a follower requesting up to a header-leading peer's tip — exactly the 2-node bootstrap shape — wedges below the checkpoint. The testkit cluster can't reproduce it because its mock server serves roots for every header (no truncation).
2. Major — for your question about memory leaks; pending-commit tree install keyed on requested, not delivered, count. pending_header_history_tree (reactor.rs:1690-1704) matches on the requested range end, so every legally-short delivery discards the just-verified tree, and the next range pays a MissingHeaderHistoryTree rebuild that folds every durable root from verified tip to frontier — O(frontier) per short range, quadratic while headers race ahead of bodies. Findings 1, 2, and a pending-commits memory leak (~0.5–1 MB per leaked entry) share one root cause: requested-vs-delivered range identity. One fix covers all three — key the bookkeeping by delivered count and add a success-path clear_assignment (or cap request ends at peer_tip − 1 for root-carrying ranges).
3. Potential blocker, not 100% we could hit this as a test is difficult — legacy unverified roots are trusted wholesale. Answering your migration question directly: there is no migration for existing roots. No format-version bump (constants.rs untouched), no cleanup, no re-validation. The old code persisted every peer root unverified ("advisory until verified during block commit"); the new startup rebuild folds those rows via append_confirmed_roots, whose doc literally assumes "roots … were verified when they were persisted." One poisoned legacy root → the frontier tree is wrong → every honest forward range fails InvalidHeaderCommitment and honest peers get scored InvalidRange forever. The lazy rebuild never fires (it only triggers when the tree is behind, never when it's wrong), and a restart rebuilds the same poison. A one-time delete-range of provisional roots above the body tip on upgrade would close this cheaply.
> note: I could see how the above could bite us, but I'm also okay with just expecting folks to resync or snapshot to avoid adding a feature we will only use hopefully once
4. Low -- Opaque UX issue — unbounded startup fold. The rebuild replays the whole header lead with no span cap or progress logging. It's correctly on spawn_blocking (no runtime stall), but the fleet has seen a 3.34M-header lead — that's minutes of silent startup, per restart, and finding 2 makes the same cost recur at runtime.
5. Low -- Docs are out of sync with the code: the design doc's "Increment 6e (current)" claims the QueryBestHeaderHistoryTree reload machinery was removed, but the shipped code adds and load-bears it (it's the sole recovery path when checkpoint/legacy/gossip commits outrun the tree). Also commitment_aux_verify.rs:206/223 comments misstate the network-upgrade boundaries (says Heartwood–NU6_2 / NU6_3+; actually Heartwood–Canopy / NU5+ — the logic is right, the comments aren't).
|
Thanks. Trying to add tests to confirm the findings |
|
@v12-auditor review |
And two more auto-invalidated findings. Analyzed 22 files, diff |
Robustness fixes to the below-checkpoint header-sync history-tree reconstruction/rebuild path, from a self-audit: - Arm the rebuild in-flight guard only when the QueryBestHeaderHistoryTree action is actually queued. dispatch_action is a non-blocking try_send that drops on a full/closed channel; arming the guard on a dropped send would suppress every future rebuild permanently (the guard clears only when the reload completes). A dropped send now leaves the guard clear so the next range retry re-dispatches. - Re-base the reconstruction at the current snapshot tip when the caller's cached verified_block_tip has no stored tree. The finalized history tree is tip-only, so a concurrent checkpoint/legacy commit advancing the finalized tip during the round-trip made read::tree::history_tree return None, then Err, then a network-paced no-progress rebuild loop. Reading the tip from the same snapshot is atomic and is the correct committed base below the checkpoint. - Clamp finalized_height with max() in handle_state_frontiers_changed so a stale or out-of-order frontier update cannot move the root-regime floor backward. verified_block_tip is left free (it can reorg above the checkpoint). Adds failed_rebuild_clears_guard_and_retriggers, and updates docs/design/verified-commitment-trees.md (6.4 robustness details, 13 the sync.header.history_tree.rebuild counter).
…w it
The runtime header-frontier history-tree lazy rebuild discarded the frontier
that `ReadResponse::BestHeaderHistoryTree` returns, keeping only the tree
(`{ tree, .. }`) and echoing the requested `best_header_tip` back to the
reactor. When durable roots have a gap — a non-Zakura path (checkpoint/legacy
sync) committed below the checkpoint without persisting header-commit roots —
the fold stops below `best_header_tip - 1`, so the rebuilt tree never reaches
the current tip's parent. The reactor installed the low tree but left the tip
high, so every forward range re-detected the behind tree and re-triggered the
identical deterministic rebuild forever (a network-paced no-progress loop that
also re-ran the O(gap) durable-root fold each retry).
The startup path already honors the frontier contract (resume from
`frontier + 1`, anchor on the frontier hash); the runtime path did not. This
threads the frontier from where it is known (the zebrad driver, which owns the
state read) to where it is actionable (the reactor, which owns the tip):
- Add `HeaderFrontierReanchor` and carry it on `BestHeaderHistoryTreeLoaded`.
- Driver `header_frontier_reanchor` resolves the resume anchor on a gap
(mirroring the startup resume), `None` in the common no-gap case.
- Reactor `reanchor_header_frontier` drops the tip onto the rebuilt tree,
clearing the stranded forward schedule and repositioning tip/parent-hash,
then reschedules so the resumed range verifies against the reinstalled tree.
- Generalize `publish_best_tip_reanchored` to take the overlap parent hash.
Adds `lazy_rebuild_reanchors_tip_onto_lower_frontier_tree` (verified to fail
without the reanchor) and the `sync.header.history_tree.reanchor` counter;
documents both in docs/design/verified-commitment-trees.md (6.4, 13).
5f44263 to
46f41c6
Compare
|
@v12-auditor review |
And one more auto-invalidated finding. Analyzed 26 files, diff |
2 participants
Zakura Header-Sync Commitment Roots
In the header-sync network layer, maintain history tree cache to authenticate peer-provided note commitment roots and auth data.
Goals:
Design:
Hare not confirmed by headerH. They become confirmed only after headerH + 1validates against the tree that includesH.[start..=end]confirms roots only for[start..=end - 1]. The root forendremains unconfirmed and must be re-delivered in the next overlapping range.[X - 3999, X],[X, X + 3999], etc. The overlap block is requested twice so its root can be confirmed by the successor header in the second batch.Risks
best_header_tip - 1, stopping at the first missing header/root frontier and returning the highest contiguous frontier. Header sync then resumes fromfrontier + 1with an overlapping request, rather than blindly capping back to the verified body tip. The best tip can be way ahead of the verified tip, so the CPU work may be meaningful. I have not observed practical issues, but I can see needing to move this computation off to a background worker in the future.Implementation Details
There was something called backward/checkpoint header backfill which allows specifying an anchor height/hash and syncing backwards. Initial implementation broke it by allowing forward sync only. Had to rebuild to account for that.
Testing