feat!: enforce ranged header requests have roots

docs/plans/headersync_roots_review.md

@@ -0,0 +1,88 @@
+# PR #282 review — `feat!: enforce ranged header requests have roots`
+
+Branch `review/headersync-roots` @ `8c2f7d379` onto `perf-note-commit-tree` (`e73e09d71`, includes #254).
+
+Scope: ranged Zakura header-sync responses/commits must now carry exactly one
+`tree_aux_root` per header (previously optional / all-or-nothing). Threaded through
+wire decode → reactor serving/inbound → state commit → root-covered best-header-tip
+capping (state service + header-sync driver, startup + steady-state).
+
+## Verdict
+
+Design and implementation are consistent end-to-end. One real compile bug found and
+fixed; remaining red tests are all pre-existing on the base branch (documented as flups
+below). The roots invariant holds transitively: a header only enters a peer's store via
+`CommitHeaderRange` (now mandates roots → persists provisional roots) or a full-block
+commit (roots derivable from state), so any header a peer can serve, it can also serve a
+root for. Tip propagation still flows over full-block `NewBlock` gossip, so the
+mandatory-roots rule on _ranged_ requests does not starve the tip.
+
+## Bug fixed in this review
+
+- **zebrad lib tests did not compile.** `start.rs`'s `zakura_header_sync_driver_tests`
+  imports `block_roots_cover_range` and `root_covered_query_best_header_tip` via
+  `super::zakura::`, but `zebrad/src/commands/start/zakura/mod.rs` never re-exported them
+  (both are `pub(crate)` in `header_sync_driver.rs` and used in-module by production code).
+  The PR author missed this because their local `librocksdb-sys` build failed before
+  reaching zebrad, so the zebrad tests never compiled. Fix: added both to the
+  `#[cfg(test)]` re-export block in `mod.rs`. The reported `E0282` was a cascade from the
+  unresolved import.
+
+## Flups — pre-existing test failures (NOT caused by #282; reproduce on base `e73e09d71`)
+
+1. **`zebra-state` proptest `service::finalized_state::tests::prop::vct_frozen_frontier_survives_reopen`.**
+   Panics at `finalized_state.rs:551`: "database was previously synced in verified
+   commitment tree mode ... fast path ... is disabled. Set `consensus.checkpoint_sync = true`
+   and `consensus.disable_vct_fast_sync = false`...". This is #254's VCT fast-sync resume
+   gate; the proptest reopen config doesn't satisfy the resume preconditions. Verified to
+   fail identically on the base branch. Relies on later VCT-resume wiring → flup.
+
+2. **`zebrad` legacy block-sync vectors (run via nextest):**
+   `components::sync::tests::vectors::request_genesis_accepts_duplicate_finalized_genesis`,
+   `...::sync_block_too_high_obtain_tips`, `...::sync_block_too_high_extend_tips`.
+   Legacy (non-Zakura) sync component, untouched by this PR. Verified to fail identically
+   on the base branch → flup.
+
+3. **`zebra-network` testkit network tests (env-flaky):**
+   `zakura::testkit::cluster::tests::connected_peers_import_each_others_signed_records` and
+   `...::native_stream5_status_exchange_uses_handler_wire_path`. Real iroh peer
+   registration with 5s timeouts; fail only under parallel-build CPU load, **pass in
+   isolation**. Harness flakiness, not a sync defect → flup.
+
+## Harness notes (not failures)
+
+- `cargo test -p zebrad --lib` (single process) cascades ~76 failures from one root panic:
+  `zebra_test::init()` → color-eyre `install().unwrap()` → "a hook has already been
+  installed", poisoning the init `Once`. CI uses **nextest** (process-per-test), which
+  sidesteps this. Always validate zebrad with `cargo nextest run`, not `cargo test --lib`.
+- `cargo clippy --workspace -- -D warnings` fails on **pre-existing** zebra-chain lints
+  (`unexpected_cfgs: tx_v6` at `transaction.rs:1099`; 4× `ValueCommitment` Copy-clone),
+  not on anything in #282. PR-touched files are clippy-clean.
+- Build requires `CXXFLAGS="-include cstdint"` on GCC 15 (the `librocksdb-sys` C++ /
+  `<cstdint>` failure the PR author hit). Not a code issue.
+
+## Non-blocking review observations (candidate follow-ups for the author)
+
+- **Redundant double root-cover.** `ReadRequest::BestHeaderTip` already returns the
+  root-covered tip (`root_covered_best_header_tip` in the state service), yet
+  `drive_zakura_header_sync_actions` re-applies `root_covered_query_best_header_tip` to
+  that result on every `query_best_header_tip` tick — two extra state reads
+  (`Tip` + `BlockRoots`) plus a duplicated root scan. Correct (idempotent/monotonic) but
+  wasteful; consider keeping the cap in one layer.
+- **Per-height serving cost.** `block_roots_by_height_range` does point lookups per height
+  (`finalized_tip_height()` + `serve_block_roots(h..=h)` + provisional read each iteration),
+  up to `MAX_HEADER_SYNC_HEIGHT_RANGE` = 4000, on a hot serving path that previously used a
+  single range scan. Consider batching the finalized/provisional reads.
+- **Stream version.** `ZAKURA_HEADER_SYNC_STREAM_VERSION` stays `4` while v4 semantics flip
+  from "optional all-or-nothing roots" to "mandatory one-per-header". An old-v4 peer
+  answering a non-finalized range would now be rejected (`TreeAuxRootCountMismatch` →
+  `MalformedMessage`). Fine for a pre-GA fleet upgraded together, but a deliberate
+  bump-to-5 would make the incompatibility explicit.
+- **No backfill migration for pre-existing rootless header rows.** A DB written under the
+  old optional-roots regime has header rows without provisional roots; after upgrade those
+  ranges serve empty and the advertised tip is capped to the verified tip until re-synced
+  with roots. Self-heals (no wedge), but there is no explicit migration. Confirm this is the
+  intended degradation path (cross-ref the earlier "header-carried roots" plan that leaned
+  toward keeping roots optional).
+- **CHANGELOG.** `feat!` with no CHANGELOG entry; intentional for experimental Zakura
+  internals, but worth a deliberate note.

zebra-network/src/zakura/header_sync/config.rs

@@ -203,10 +203,17 @@ pub fn truncate_headers_to_byte_budget(
     network: &Network,
     max_frame_bytes: u32,
 ) -> (Vec<Arc<block::Header>>, Vec<u32>, Vec<BlockCommitmentRoots>) {
+    if headers.len() != tree_aux_roots.len() {
+        headers.clear();
+        body_sizes.clear();
+        tree_aux_roots.clear();
+        return (headers, body_sizes, tree_aux_roots);
+    }
+
     let max_count = usize::try_from(header_sync_count_by_byte_budget(
         network,
         max_frame_bytes,
-        !tree_aux_roots.is_empty(),
+        true,
     ))
     .expect("header-sync byte-budget count fits in usize");
     headers.truncate(max_count);

zebra-network/src/zakura/header_sync/error.rs

@@ -45,7 +45,7 @@ pub enum HeaderSyncWireError {
         body_sizes: usize,
     },
 
-    /// A locally constructed or inbound `Headers` message had a different number of roots.
+    /// A locally constructed or inbound `Headers` message did not carry exactly one root per header.
     #[error("Zakura header-sync Headers tree-aux root count {roots} does not match header count {headers}")]
     TreeAuxRootCountMismatch {
         /// Header count.

zebra-network/src/zakura/header_sync/pipe.rs

@@ -467,7 +467,7 @@ mod tests {
     fn deliver_correlated_headers_decodes_against_expectation() {
         let (handle, mut events) = test_handle();
         let expected =
-            ExpectedHeadersResponse::new(block::Height(1), 1, false).expect("count is valid");
+            ExpectedHeadersResponse::new(block::Height(1), 1, true).expect("count is valid");
 
         let flow = deliver(&handle, Some(expected), peer(), headers_frame(Vec::new()));
 
@@ -579,7 +579,7 @@ mod tests {
     /// timeout and desynchronizing the peer-local FIFO from the outstanding range.
     #[test]
     fn saturated_events_queue_restores_solicited_expectation() {
-        use zebra_chain::serialization::ZcashDeserializeInto;
+        use zebra_chain::{orchard, sapling, serialization::ZcashDeserializeInto};
         use zebra_test::vectors::BLOCK_MAINNET_1_BYTES;
 
         // Keep `_events_rx` alive so the saturated queue rejects with `Full`
@@ -588,7 +588,7 @@ mod tests {
         let (commands_tx, commands_rx) = mpsc::unbounded_channel();
 
         let expected =
-            ExpectedHeadersResponse::new(block::Height(1), 1, false).expect("count is valid");
+            ExpectedHeadersResponse::new(block::Height(1), 1, true).expect("count is valid");
         commands_tx
             .send(HeaderSyncPeerCommand::RecordExpectedHeaders(expected))
             .expect("pipe is alive");
@@ -601,14 +601,14 @@ mod tests {
                 .zcash_deserialize_into()
                 .expect("block 1 vector parses"),
         );
-        // The expectation above did not request tree-aux roots (a non-finalized
-        // range), so a roots-bearing response would be rejected at decode as
-        // `UnrequestedTreeAuxRoots`. This test exercises queue-saturation
-        // expectation restoral, not roots, so the response carries none.
         let solicited_headers = HeaderSyncMessage::Headers {
             headers: vec![block_one.header.clone()],
             body_sizes: vec![0],
-            tree_aux_roots: Vec::new(),
+            tree_aux_roots: vec![BlockCommitmentRoots {
+                height: block::Height(1),
+                sapling_root: sapling::tree::NoteCommitmentTree::default().root(),
+                orchard_root: orchard::tree::NoteCommitmentTree::default().root(),
+            }],
         }
         .encode_frame()
         .expect("headers frame encodes");
@@ -624,10 +624,27 @@ mod tests {
         // Drain the recorded expectation into `HsLocal`, mirroring `run_peer`'s
         // pre-frame command drain so the `Headers` frame is correlated.
         pipe.local_mut().drain_ready_commands();
+        assert_eq!(
+            pipe.local_mut().pop_expected_headers_response(),
+            Some(expected),
+            "the solicited response expectation should be available after draining commands"
+        );
+        pipe.local_mut().restore_expected_headers(expected);
+        HeaderSyncMessage::decode_frame(
+            solicited_headers.clone(),
+            HeaderSyncDecodeContext::for_headers_response(expected, expected.count),
+        )
+        .expect("test Headers frame decodes against its expectation");
 
         // The decoded response cannot be delivered (events queue is full); the
         // pipe logs and continues, exactly as production does.
-        assert!(matches!(pipe.run_one(solicited_headers), Flow::Done));
+        let flow = pipe.run_one(solicited_headers);
+        match flow {
+            Flow::Done => {}
+            Flow::Continue(()) => panic!("unexpected successful forward"),
+            Flow::Reject(SinkReject::Protocol(_)) => panic!("unexpected protocol reject"),
+            Flow::Reject(SinkReject::Local(_)) => panic!("unexpected local reject"),
+        }
 
         // The popped expectation must be restored so the still-outstanding range
         // stays correlated. Without the fix the expectation is gone (returns None).

zebra-network/src/zakura/header_sync/reactor.rs

@@ -647,24 +647,25 @@ impl HeaderSyncReactor {
         start_height: block::Height,
         requested_count: u32,
         want_tree_aux_roots: bool,
-        headers: Vec<Arc<block::Header>>,
-        body_sizes: Vec<u32>,
-        tree_aux_roots: Vec<BlockCommitmentRoots>,
+        mut headers: Vec<Arc<block::Header>>,
+        mut body_sizes: Vec<u32>,
+        mut tree_aux_roots: Vec<BlockCommitmentRoots>,
     ) {
         let Some(peer_state) = self.state.peers.get_mut(&peer) else {
             return;
         };
-        if validate_body_sizes_len(headers.len(), body_sizes.len()).is_err()
-            || validate_tree_aux_roots_len(headers.len(), tree_aux_roots.len()).is_err()
-            || validate_tree_aux_root_heights(start_height, &tree_aux_roots).is_err()
-        {
+        if validate_body_sizes_len(headers.len(), body_sizes.len()).is_err() {
             peer_state.finish_serving_headers();
             return;
         }
-        let tree_aux_roots = if want_tree_aux_roots {
-            tree_aux_roots
-        } else {
-            Vec::new()
+
+        let roots_complete = validate_tree_aux_roots_len(headers.len(), tree_aux_roots.len())
+            .and_then(|()| validate_tree_aux_root_heights(start_height, &tree_aux_roots))
+            .is_ok();
+        if !headers.is_empty() && (!want_tree_aux_roots || !roots_complete) {
+            headers.clear();
+            body_sizes.clear();
+            tree_aux_roots.clear();
         };
         let returned_count = u32::try_from(headers.len()).unwrap_or(u32::MAX);
         let served_tree_aux_roots_len = u32::try_from(tree_aux_roots.len()).unwrap_or(u32::MAX);

zebra-network/src/zakura/header_sync/state.rs

@@ -82,13 +82,12 @@ impl HeaderSyncCore {
         if count == 0 {
             return;
         }
-        let want_tree_aux_roots = self.wants_tree_aux_roots(start, end, startup);
         self.schedule.ensure_forward(RangeRequest {
             start_height: start,
             count,
             anchor_hash: self.best_header_hash,
             finalized,
-            want_tree_aux_roots,
+            want_tree_aux_roots: true,
             priority: RangePriority::Forward,
         });
     }
@@ -122,17 +121,6 @@ impl HeaderSyncCore {
             priority: RangePriority::Backward,
         });
     }
-
-    fn wants_tree_aux_roots(
-        &self,
-        start: block::Height,
-        end: block::Height,
-        startup: &HeaderSyncStartup,
-    ) -> bool {
-        let last_checkpoint_height = startup.network.checkpoint_list().max_height();
-
-        start <= last_checkpoint_height && end > self.verified_block_tip
-    }
 }
 
 #[derive(Clone, Debug, Default)]