From 8c2f7d37973a1cb5481a7f022cfaed5a5bb5b83c Mon Sep 17 00:00:00 2001 From: evan-forbes Date: Thu, 25 Jun 2026 15:02:08 -0500 Subject: [PATCH 1/4] feat!: enforce ranged header requests have roots --- .../src/zakura/header_sync/config.rs | 9 +- zebra-network/src/zakura/header_sync/error.rs | 2 +- zebra-network/src/zakura/header_sync/pipe.rs | 35 +++- .../src/zakura/header_sync/reactor.rs | 23 ++- zebra-network/src/zakura/header_sync/state.rs | 14 +- zebra-network/src/zakura/header_sync/tests.rs | 91 ++++++--- .../src/zakura/header_sync/validation.rs | 2 +- zebra-network/src/zakura/header_sync/wire.rs | 7 +- zebra-network/src/zakura/testkit/cluster.rs | 36 +--- zebra-state/src/error.rs | 2 +- zebra-state/src/request.rs | 6 +- zebra-state/src/service.rs | 190 +++++++++++++----- .../service/finalized_state/zebra_db/block.rs | 34 +++- zebra-state/src/service/tests.rs | 114 ++++++++++- zebrad/src/commands/start.rs | 72 ++++++- .../start/zakura/header_sync_driver.rs | 138 ++++++++++++- 16 files changed, 599 insertions(+), 176 deletions(-) diff --git a/zebra-network/src/zakura/header_sync/config.rs b/zebra-network/src/zakura/header_sync/config.rs index 785101dd116..dd42577587c 100644 --- a/zebra-network/src/zakura/header_sync/config.rs +++ b/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>, Vec, Vec) { + 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); diff --git a/zebra-network/src/zakura/header_sync/error.rs b/zebra-network/src/zakura/header_sync/error.rs index 9d3b3b5849d..dbaabf5461e 100644 --- a/zebra-network/src/zakura/header_sync/error.rs +++ b/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. diff --git a/zebra-network/src/zakura/header_sync/pipe.rs b/zebra-network/src/zakura/header_sync/pipe.rs index c7d276a1b97..e4777d2a18d 100644 --- a/zebra-network/src/zakura/header_sync/pipe.rs +++ b/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). diff --git a/zebra-network/src/zakura/header_sync/reactor.rs b/zebra-network/src/zakura/header_sync/reactor.rs index 7a26521a15a..7b1baab2a47 100644 --- a/zebra-network/src/zakura/header_sync/reactor.rs +++ b/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>, - body_sizes: Vec, - tree_aux_roots: Vec, + mut headers: Vec>, + mut body_sizes: Vec, + mut tree_aux_roots: Vec, ) { 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); diff --git a/zebra-network/src/zakura/header_sync/state.rs b/zebra-network/src/zakura/header_sync/state.rs index 84dcaf7df45..d1b6b55ee84 100644 --- a/zebra-network/src/zakura/header_sync/state.rs +++ b/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)] diff --git a/zebra-network/src/zakura/header_sync/tests.rs b/zebra-network/src/zakura/header_sync/tests.rs index 6db4b3f8bc0..51356500825 100644 --- a/zebra-network/src/zakura/header_sync/tests.rs +++ b/zebra-network/src/zakura/header_sync/tests.rs @@ -130,15 +130,12 @@ fn headers_message_from( start_height: block::Height, headers: Vec>, ) -> HeaderSyncMessage { - // Non-finalized ranges never request tree-aux roots, so the common test - // builder omits them. The reactor rejects roots on a non-finalized range as - // `MalformedMessage`; finalized-range tests opt in via `finalized_*`. - let _ = start_height; let body_sizes = vec![0; headers.len()]; + let tree_aux_roots = roots_from_height(start_height, headers.len()); HeaderSyncMessage::Headers { headers, body_sizes, - tree_aux_roots: Vec::new(), + tree_aux_roots, } } @@ -146,6 +143,24 @@ fn headers_message_with_sizes( headers: Vec>, body_sizes: Vec, ) -> HeaderSyncMessage { + let start_height = headers + .first() + .map(|header| test_header_height(header.as_ref())) + .unwrap_or(block::Height(1)); + let tree_aux_roots = roots_from_height(start_height, headers.len()); + HeaderSyncMessage::Headers { + headers, + body_sizes, + tree_aux_roots, + } +} + +fn rootless_headers_message_from( + start_height: block::Height, + headers: Vec>, +) -> HeaderSyncMessage { + let _ = start_height; + let body_sizes = vec![0; headers.len()]; HeaderSyncMessage::Headers { headers, body_sizes, @@ -153,8 +168,6 @@ fn headers_message_with_sizes( } } -/// Finalized-range `Headers` carrying one tree-aux root per header, as a peer -/// answering a `want_tree_aux_roots` request would send. fn finalized_headers_message(headers: Vec>) -> HeaderSyncMessage { let start_height = headers .first() @@ -250,7 +263,6 @@ fn headers_context(count: u32, peer_cap: u32) -> HeaderSyncDecodeContext { ) } -/// Decode context for a finalized-range response that requested tree-aux roots. fn finalized_headers_context(count: u32, peer_cap: u32) -> HeaderSyncDecodeContext { HeaderSyncDecodeContext::for_headers_response( ExpectedHeadersResponse::new(block::Height(1), count, true).unwrap(), @@ -546,7 +558,7 @@ async fn advisory_summary_status_mismatch_uses_status_without_misbehavior_and_ba HeaderSyncMessage::GetHeaders { start_height, count, - want_tree_aux_roots: false, + want_tree_aux_roots: true, }, } if peer == peer_id => { assert_eq!(start_height, block::Height(1)); @@ -787,7 +799,7 @@ async fn next_outbound_get_headers( HeaderSyncMessage::GetHeaders { start_height, count, - want_tree_aux_roots: false, + want_tree_aux_roots: true, }, } => return (peer, start_height, count), HeaderSyncAction::Misbehavior { peer, reason } => { @@ -995,18 +1007,16 @@ fn headers_codec_rejects_body_size_mismatch_truncation_and_trailing_bytes() { }) )); - // Empty roots is the valid all-or-nothing "none" case; a non-empty count - // that disagrees with the header count is the rejection. assert!(matches!( HeaderSyncMessage::Headers { headers: headers.clone(), body_sizes: vec![100], - tree_aux_roots: vec![root_at(block::Height(1)), root_at(block::Height(2))], + tree_aux_roots: Vec::new(), } .encode(), Err(HeaderSyncWireError::TreeAuxRootCountMismatch { headers: 1, - roots: 2, + roots: 0, }) )); @@ -1020,7 +1030,9 @@ fn headers_codec_rejects_body_size_mismatch_truncation_and_trailing_bytes() { let mut truncated_mid_size = message.encode().unwrap(); truncated_mid_size.pop(); - assert!(HeaderSyncMessage::decode(&truncated_mid_size, headers_context(1, 1)).is_err()); + assert!( + HeaderSyncMessage::decode(&truncated_mid_size, finalized_headers_context(1, 1)).is_err() + ); let mut truncated_mid_header = vec![MSG_HS_HEADERS]; truncated_mid_header.write_u32::(1).unwrap(); @@ -1030,11 +1042,27 @@ fn headers_codec_rejects_body_size_mismatch_truncation_and_trailing_bytes() { let mut with_trailing = message.encode().unwrap(); with_trailing.push(0); assert!(matches!( - HeaderSyncMessage::decode(&with_trailing, headers_context(1, 1)), + HeaderSyncMessage::decode(&with_trailing, finalized_headers_context(1, 1)), Err(HeaderSyncWireError::TrailingBytes) )); } +#[test] +fn decode_rejects_non_empty_headers_without_tree_aux_roots() { + let headers = vec![mainnet_header(&BLOCK_MAINNET_1_BYTES)]; + let mut encoded = headers_message(headers).encode().unwrap(); + encoded[HEADER_SYNC_MESSAGE_TYPE_BYTES + HEADER_SYNC_COUNT_BYTES] = 0; + encoded.truncate(encoded.len() - HEADER_SYNC_BLOCK_COMMITMENT_ROOTS_BYTES); + + assert!(matches!( + HeaderSyncMessage::decode(&encoded, finalized_headers_context(1, 1)), + Err(HeaderSyncWireError::TreeAuxRootCountMismatch { + headers: 1, + roots: 0, + }) + )); +} + #[test] fn frame_decode_rejects_oversized_payload_length_before_allocating() { let mut bytes = Vec::new(); @@ -1275,7 +1303,7 @@ async fn restart_rebuilds_schedule_from_durable_best_tip_and_peer_status() { HeaderSyncMessage::GetHeaders { start_height, count, - want_tree_aux_roots: false, + want_tree_aux_roots: true, }, .. } = next_non_query_action(&mut fixture.actions).await @@ -1337,7 +1365,7 @@ async fn status_updates_peer_caps_and_scheduler_respects_them() { HeaderSyncMessage::GetHeaders { start_height, count, - want_tree_aux_roots: false, + want_tree_aux_roots: true, }, } = next_non_query_action(&mut fixture.actions).await { @@ -1416,7 +1444,7 @@ async fn scheduler_fans_out_same_forward_range_to_three_peers() { HeaderSyncMessage::GetHeaders { start_height, count, - want_tree_aux_roots: false, + want_tree_aux_roots: true, }, } = next_non_query_action(&mut fixture.actions).await { @@ -1559,7 +1587,7 @@ async fn scheduler_creates_checkpoint_forward_before_backward_ranges() { HeaderSyncMessage::GetHeaders { start_height, count, - want_tree_aux_roots: false, + want_tree_aux_roots: true, }, .. } = next_non_query_action(&mut fixture.actions).await @@ -1752,7 +1780,7 @@ async fn incoming_headers_match_outstanding_before_commit() { } #[tokio::test(flavor = "current_thread")] -async fn unrequested_response_carrying_tree_aux_roots_is_malformed() { +async fn rootless_non_empty_response_is_malformed() { let checkpoint_hash = block::Hash::from(mainnet_header(&BLOCK_MAINNET_3_BYTES).as_ref()); let (network, _) = checkpoint_testnet_with_hash(block::Height(3), checkpoint_hash); let first_checkpoint = block::Height(3); @@ -1771,7 +1799,7 @@ async fn unrequested_response_carrying_tree_aux_roots_is_malformed() { next_non_query_action(&mut fixture.actions).await, HeaderSyncAction::SendMessage { msg: HeaderSyncMessage::GetHeaders { - want_tree_aux_roots: false, + want_tree_aux_roots: true, .. }, .. @@ -1781,14 +1809,11 @@ async fn unrequested_response_carrying_tree_aux_roots_is_malformed() { } } - // This network's checkpoint handoff is the first checkpoint, so the range - // does not ask for roots. A peer that volunteers roots anyway is reported - // as MalformedMessage and the range is retried rather than committed. fixture .handle .send(HeaderSyncEvent::WireMessage { peer: peer_id.clone(), - msg: finalized_headers_message(vec![mainnet_header(&BLOCK_MAINNET_4_BYTES)]), + msg: rootless_headers_message_from(start, vec![mainnet_header(&BLOCK_MAINNET_4_BYTES)]), }) .await .unwrap(); @@ -1801,7 +1826,7 @@ async fn unrequested_response_carrying_tree_aux_roots_is_malformed() { break; } HeaderSyncAction::CommitHeaderRange { .. } => { - panic!("an unrequested roots-bearing response must not commit") + panic!("a rootless non-empty response must not commit") } _ => {} } @@ -2171,7 +2196,7 @@ async fn late_covered_response_does_not_reanchor_newer_outstanding_range() { HeaderSyncMessage::GetHeaders { start_height: block::Height(1), count: 1, - want_tree_aux_roots: false, + want_tree_aux_roots: true, }, } if peer == peer_id => break, _ => {} @@ -2195,7 +2220,7 @@ async fn late_covered_response_does_not_reanchor_newer_outstanding_range() { HeaderSyncMessage::GetHeaders { start_height: block::Height(2), count: 1, - want_tree_aux_roots: false, + want_tree_aux_roots: true, }, } if peer == peer_id => break, _ => {} @@ -2310,7 +2335,7 @@ async fn local_commit_failure_retries_without_peer_misbehavior() { HeaderSyncMessage::GetHeaders { start_height, count, - want_tree_aux_roots: false, + want_tree_aux_roots: true, }, } if peer == first_peer || peer == second_peer => { assert_eq!(start_height, start); @@ -2498,7 +2523,7 @@ async fn reconnect_clears_session_bound_outstanding_ranges() { msg: HeaderSyncMessage::GetHeaders { start_height: block::Height(1), count: 1, - want_tree_aux_roots: false, + want_tree_aux_roots: true, }, } if peer == peer_id )); @@ -2527,7 +2552,7 @@ async fn reconnect_clears_session_bound_outstanding_ranges() { msg: HeaderSyncMessage::GetHeaders { start_height: block::Height(1), count: 1, - want_tree_aux_roots: false, + want_tree_aux_roots: true, }, } if peer == peer_id )); @@ -3927,7 +3952,7 @@ async fn forward_link_wedge_reanchors_to_verified_tip_without_banning() { HeaderSyncMessage::GetHeaders { start_height, count: _, - want_tree_aux_roots: false, + want_tree_aux_roots: true, }, .. } if saw_reanchor_action && start_height == expected_start => { diff --git a/zebra-network/src/zakura/header_sync/validation.rs b/zebra-network/src/zakura/header_sync/validation.rs index d44b21465d6..6e16519e27a 100644 --- a/zebra-network/src/zakura/header_sync/validation.rs +++ b/zebra-network/src/zakura/header_sync/validation.rs @@ -343,7 +343,7 @@ pub(super) fn validate_tree_aux_roots_len( headers: usize, roots: usize, ) -> Result<(), HeaderSyncWireError> { - if roots != 0 && headers != roots { + if headers != roots { return Err(HeaderSyncWireError::TreeAuxRootCountMismatch { headers, roots }); } Ok(()) diff --git a/zebra-network/src/zakura/header_sync/wire.rs b/zebra-network/src/zakura/header_sync/wire.rs index 07430d5b447..eff54299071 100644 --- a/zebra-network/src/zakura/header_sync/wire.rs +++ b/zebra-network/src/zakura/header_sync/wire.rs @@ -4,8 +4,7 @@ use super::{config::*, error::*, validation::*, *}; pub const ZAKURA_STREAM_HEADER_SYNC: u16 = 5; /// Version of the native header-sync stream. /// -/// Version 4 makes header-carried tree-aux roots optional all-or-nothing -/// metadata. Headers remain the required propagation payload. +/// Version 4 carries one tree-aux root for each non-empty range header. pub const ZAKURA_HEADER_SYNC_STREAM_VERSION: u16 = 4; /// Peer status advertisement. @@ -201,6 +200,10 @@ impl HeaderSyncMessage { tree_aux_roots.push(BlockCommitmentRoots::zcash_deserialize(&mut reader)?); } } + validate_tree_aux_roots_len(count, tree_aux_roots.len())?; + if let Some(requested) = context.requested { + validate_tree_aux_root_heights(requested.start_height, &tree_aux_roots)?; + } Self::Headers { headers, body_sizes, diff --git a/zebra-network/src/zakura/testkit/cluster.rs b/zebra-network/src/zakura/testkit/cluster.rs index 227424e4cb7..952a23d17be 100644 --- a/zebra-network/src/zakura/testkit/cluster.rs +++ b/zebra-network/src/zakura/testkit/cluster.rs @@ -202,18 +202,6 @@ mod tests { } } - /// A `Headers` response with no tree-aux roots, as a peer answers a request - /// that did not set `want_tree_aux_roots` (a non-finalized range). The - /// reactor rejects roots on a non-finalized range as `MalformedMessage`. - fn headers_message_without_roots(headers: Vec>) -> HeaderSyncMessage { - let body_sizes = vec![0; headers.len()]; - HeaderSyncMessage::Headers { - headers, - body_sizes, - tree_aux_roots: Vec::new(), - } - } - fn root_at(height: block::Height) -> BlockCommitmentRoots { BlockCommitmentRoots { height, @@ -898,7 +886,7 @@ mod tests { peer, start, count, - want_tree_aux_roots, + want_tree_aux_roots: _, } => { let headers = local .store @@ -907,15 +895,7 @@ mod tests { .headers_by_range(start, count); let returned_count = u32::try_from(headers.len()).unwrap_or(u32::MAX); if let Some(target) = peer_to_index.get(&peer) { - // Mirror production serving: only a finalized-range - // requester (`want_tree_aux_roots`) receives roots; a - // non-finalized requester must get a roots-free response - // or the reactor rejects it as `MalformedMessage`. - let msg = if want_tree_aux_roots { - headers_message(headers) - } else { - headers_message_without_roots(headers) - }; + let msg = headers_message(headers); let _ = nodes[*target] .handle .send(HeaderSyncEvent::WireMessage { @@ -3026,9 +3006,7 @@ mod tests { .inject( victim, out_of_range, - headers_message_without_roots(vec![mainnet_block(&BLOCK_MAINNET_2_BYTES) - .header - .clone()]), + headers_message(vec![mainnet_block(&BLOCK_MAINNET_2_BYTES).header.clone()]), ) .await; cluster @@ -3071,7 +3049,7 @@ mod tests { .inject( victim, response_too_long, - headers_message_without_roots(vec![ + headers_message(vec![ mainnet_block(&BLOCK_MAINNET_1_BYTES).header.clone(), mainnet_block(&BLOCK_MAINNET_2_BYTES).header.clone(), ]), @@ -3109,7 +3087,7 @@ mod tests { .inject( bad_continuity_victim, bad_continuity, - headers_message_without_roots(vec![ + headers_message(vec![ mainnet_block(&BLOCK_MAINNET_1_BYTES).header.clone(), Arc::new(non_contiguous), ]), @@ -3141,7 +3119,7 @@ mod tests { .inject( bad_pow_victim, bad_pow, - headers_message_without_roots(vec![Arc::new(bad_pow_header)]), + headers_message(vec![Arc::new(bad_pow_header)]), ) .await; cluster @@ -3174,7 +3152,7 @@ mod tests { .inject( bad_daa_victim, bad_daa, - headers_message_without_roots(vec![ + headers_message(vec![ mainnet_block(&BLOCK_MAINNET_1_BYTES).header.clone(), mainnet_block(&BLOCK_MAINNET_2_BYTES).header.clone(), mainnet_block(&BLOCK_MAINNET_3_BYTES).header.clone(), diff --git a/zebra-state/src/error.rs b/zebra-state/src/error.rs index 7e52e52896f..ea0876b3d9b 100644 --- a/zebra-state/src/error.rs +++ b/zebra-state/src/error.rs @@ -221,7 +221,7 @@ pub enum CommitHeaderRangeError { body_sizes: usize, }, - /// The request supplied a non-empty root list with a different number of roots than headers. + /// The request supplied a different number of roots than headers. #[error("header range tree-aux root count {roots} does not match header count {headers}")] TreeAuxRootCountMismatch { /// Header count. diff --git a/zebra-state/src/request.rs b/zebra-state/src/request.rs index 3217a8488fe..264842f4eff 100644 --- a/zebra-state/src/request.rs +++ b/zebra-state/src/request.rs @@ -1009,10 +1009,10 @@ pub enum Request { /// /// A `0` value means unknown. These hints are not consensus data. body_sizes: Vec, - /// Optional all-or-nothing tree-aux roots, parallel to `headers` when non-empty. + /// Tree-aux roots, parallel to `headers`. /// - /// Empty means the peer did not provide roots. Non-empty roots are - /// advisory until verified during block commit. + /// Every non-empty Zakura header range must provide one root per header. + /// Roots are advisory until verified during block commit. tree_aux_roots: Vec, }, diff --git a/zebra-state/src/service.rs b/zebra-state/src/service.rs index 4894c05f1ca..ef40175172f 100644 --- a/zebra-state/src/service.rs +++ b/zebra-state/src/service.rs @@ -1487,6 +1487,126 @@ where headers } +fn block_roots_by_height_range( + chain: Option, + db: &ZebraDb, + start: block::Height, + count: u32, +) -> Vec +where + C: AsRef, +{ + let mut roots = Vec::with_capacity( + usize::try_from(count.min(MAX_HEADER_SYNC_HEIGHT_RANGE)) + .expect("capped root count fits in usize"), + ); + + for offset in 0..count.min(MAX_HEADER_SYNC_HEIGHT_RANGE) { + let Some(height) = start + i64::from(offset) else { + break; + }; + + let root = if db + .finalized_tip_height() + .is_some_and(|finalized_tip| height <= finalized_tip) + { + finalized_state::serve_block_roots(db, height..=height) + .into_iter() + .next() + } else if let Some(chain) = chain + .as_ref() + .map(|chain| chain.as_ref()) + .filter(|chain| chain.contains_block_height(height)) + { + match ( + chain.sapling_tree(height.into()), + chain.orchard_tree(height.into()), + ) { + (Some(sapling), Some(orchard)) => Some(BlockCommitmentRoots { + height, + sapling_root: sapling.root(), + orchard_root: orchard.root(), + }), + _ => None, + } + } else { + db.zakura_header_commitment_roots_by_height_range(height..=height) + .into_iter() + .next() + }; + + let Some(root) = root else { + break; + }; + + if root.height != height { + break; + } + + roots.push(root); + } + + roots +} + +fn block_roots_cover_range( + start_height: block::Height, + count: u32, + roots: &[BlockCommitmentRoots], +) -> bool { + if roots.len() != usize::try_from(count).unwrap_or(usize::MAX) { + return false; + } + + roots.iter().enumerate().all(|(offset, roots)| { + let Ok(offset) = u32::try_from(offset) else { + return false; + }; + start_height + .0 + .checked_add(offset) + .is_some_and(|height| roots.height == block::Height(height)) + }) +} + +fn root_covered_best_header_tip( + chain: Option, + db: &ZebraDb, + best_disk_header_tip: Option<(block::Height, block::Hash)>, + verified_block_tip: Option<(block::Height, block::Hash)>, +) -> Option<(block::Height, block::Hash)> +where + C: AsRef, +{ + let best_header_tip = match (best_disk_header_tip, verified_block_tip) { + (Some(header_tip), Some(block_tip)) if block_tip.0 > header_tip.0 => Some(block_tip), + (Some(header_tip), _) => Some(header_tip), + (None, block_tip) => block_tip, + }?; + + let Some(verified_block_tip) = verified_block_tip else { + return Some(best_header_tip); + }; + + if best_header_tip.0 <= verified_block_tip.0 { + return Some(best_header_tip); + } + + let Ok(start_height) = verified_block_tip.0.next() else { + return Some(verified_block_tip); + }; + let best_header_height = best_header_tip.0; + let verified_block_height = verified_block_tip.0; + let count = best_header_height.0.checked_sub(verified_block_height.0)?; + let roots = block_roots_by_height_range(chain, db, start_height, count); + + if block_roots_cover_range(start_height, count, &roots) { + Some(best_header_tip) + } else { + Some(verified_block_tip) + } +} + impl Service for ReadStateService { type Response = ReadResponse; type Error = BoxError; @@ -1554,53 +1674,15 @@ impl Service for ReadStateService { start_height, count, } => { - // Serve stitched committed verified roots first, then provisional - // header-ahead roots for heights that have headers but no committed - // body yet. Committed roots win for overlapping heights because - // they have already been verified during block commit. let roots = if count == 0 { Vec::new() - } else if let Some((tip, _hash)) = state.db.best_header_tip() { - if start_height > tip { - Vec::new() - } else { - let last = start_height.0.saturating_add(count - 1).min(tip.0); - let requested = start_height..=block::Height(last); - let committed_end = state - .db - .finalized_tip_height() - .map(|finalized_tip| finalized_tip.min(*requested.end())) - .filter(|committed_end| start_height <= *committed_end); - - let mut roots = if let Some(committed_end) = committed_end { - finalized_state::serve_block_roots( - &state.db, - start_height..=committed_end, - ) - } else { - Vec::new() - }; - - let next_height = roots - .last() - .and_then(|root| root.height.next().ok()) - .unwrap_or(start_height); - if next_height <= *requested.end() { - // Extend the committed prefix with provisional Zakura header-ahead roots. - // These are peer-supplied advisory roots for heights whose headers are known - // but whose block bodies have not been committed yet. Once a block is - // committed, its verified roots move to the committed index and the - // provisional row for that height is deleted. - let provisional = - state.db.zakura_header_commitment_roots_by_height_range( - next_height..=*requested.end(), - ); - roots.extend(provisional); - } - roots - } } else { - Vec::new() + block_roots_by_height_range( + state.latest_best_chain(), + &state.db, + start_height, + count, + ) }; Ok(ReadResponse::BlockRoots(roots)) } @@ -1778,17 +1860,15 @@ impl Service for ReadStateService { ReadRequest::BestHeaderTip => { let best_disk_header_tip = state.db.best_header_tip(); - let verified_block_tip = read::tip(state.latest_best_chain(), &state.db); - - Ok(ReadResponse::BestHeaderTip( - match (best_disk_header_tip, verified_block_tip) { - (Some(header_tip), Some(block_tip)) if block_tip.0 > header_tip.0 => { - Some(block_tip) - } - (Some(header_tip), _) => Some(header_tip), - (None, block_tip) => block_tip, - }, - )) + let best_chain = state.latest_best_chain(); + let verified_block_tip = read::tip(best_chain.clone(), &state.db); + + Ok(ReadResponse::BestHeaderTip(root_covered_best_header_tip( + best_chain, + &state.db, + best_disk_header_tip, + verified_block_tip, + ))) } ReadRequest::MissingBlockBodies { from, limit } => { diff --git a/zebra-state/src/service/finalized_state/zebra_db/block.rs b/zebra-state/src/service/finalized_state/zebra_db/block.rs index 68886c1aac2..d8a9fd7a270 100644 --- a/zebra-state/src/service/finalized_state/zebra_db/block.rs +++ b/zebra-state/src/service/finalized_state/zebra_db/block.rs @@ -1422,6 +1422,32 @@ impl RetentionPlan { } } +#[cfg(test)] +fn inferred_header_range_roots( + zebra_db: &ZebraDb, + anchor: block::Hash, + count: usize, +) -> Result, CommitHeaderRangeError> { + let anchor_height = zebra_db + .header_height(anchor) + .or_else(|| (anchor == zebra_db.network().genesis_hash()).then_some(block::Height(0))) + .unwrap_or(block::Height(0)); + + (0..count) + .map(|index| { + let offset = + u32::try_from(index + 1).map_err(|_| CommitHeaderRangeError::HeightOverflow)?; + let height = (anchor_height + i64::from(offset)) + .ok_or(CommitHeaderRangeError::HeightOverflow)?; + Ok(BlockCommitmentRoots { + height, + sapling_root: sapling::tree::NoteCommitmentTree::default().root(), + orchard_root: orchard::tree::NoteCommitmentTree::default().root(), + }) + }) + .collect() +} + impl DiskWriteBatch { // Write block methods @@ -1838,6 +1864,7 @@ impl DiskWriteBatch { } /// Prepare a database batch containing a contextually validated header range. + #[cfg(test)] pub fn prepare_header_range_batch( &mut self, zebra_db: &ZebraDb, @@ -1845,11 +1872,12 @@ impl DiskWriteBatch { headers: &[Arc], body_sizes: &[u32], ) -> Result { - self.prepare_header_range_batch_with_roots(zebra_db, anchor, headers, body_sizes, &[]) + let roots = inferred_header_range_roots(zebra_db, anchor, headers.len())?; + self.prepare_header_range_batch_with_roots(zebra_db, anchor, headers, body_sizes, &roots) } /// Prepare a database batch containing a contextually validated header range - /// and optional all-or-nothing provisional tree-aux roots. + /// and one provisional tree-aux root per header. pub fn prepare_header_range_batch_with_roots( &mut self, zebra_db: &ZebraDb, @@ -1869,7 +1897,7 @@ impl DiskWriteBatch { }); } - if !tree_aux_roots.is_empty() && headers.len() != tree_aux_roots.len() { + if headers.len() != tree_aux_roots.len() { return Err(CommitHeaderRangeError::TreeAuxRootCountMismatch { headers: headers.len(), roots: tree_aux_roots.len(), diff --git a/zebra-state/src/service/tests.rs b/zebra-state/src/service/tests.rs index ffe9b0c2ecc..81ad0d0c48c 100644 --- a/zebra-state/src/service/tests.rs +++ b/zebra-state/src/service/tests.rs @@ -13,7 +13,10 @@ use zebra_chain::{ block::{self, Block, CountedHeader, Height}, chain_tip::ChainTip, fmt::SummaryDebug, + orchard, + parallel::commitment_aux::BlockCommitmentRoots, parameters::{Network, NetworkUpgrade}, + sapling, serialization::{ZcashDeserialize, ZcashDeserializeInto, ZcashSerialize}, transaction, transparent, value_balance::ValueBalance, @@ -25,8 +28,9 @@ use crate::{ arbitrary::Prepare, init_test, service::{ - arbitrary::populated_state, chain_tip::TipAction, headers_by_height_range, - non_finalized_state::Chain, StateService, + arbitrary::populated_state, block_roots_by_height_range, chain_tip::TipAction, + headers_by_height_range, non_finalized_state::Chain, root_covered_best_header_tip, + StateService, }, tests::{ setup::{partial_nu5_chain_strategy, transaction_v4_from_coinbase}, @@ -38,6 +42,23 @@ use crate::{ const LAST_BLOCK_HEIGHT: u32 = 10; +fn root_at(height: Height) -> BlockCommitmentRoots { + BlockCommitmentRoots { + height, + sapling_root: sapling::tree::NoteCommitmentTree::default().root(), + orchard_root: orchard::tree::NoteCommitmentTree::default().root(), + } +} + +fn roots_from_height(start_height: Height, count: usize) -> Vec { + (0..count) + .map(|offset| { + let offset = u32::try_from(offset).expect("test root count fits in u32"); + root_at(Height(start_height.0 + offset)) + }) + .collect() +} + async fn test_populated_state_responds_correctly( mut state: Buffer, Request>, ) -> Result<()> { @@ -517,7 +538,7 @@ async fn header_only_service_requests_preserve_body_boundary() -> std::result::R anchor: genesis.hash(), headers: vec![block1.header.clone(), block2.header.clone()], body_sizes: vec![999_999, 0], - tree_aux_roots: Vec::new(), + tree_aux_roots: roots_from_height(Height(1), 2), }) .await?, Response::Committed(block2_hash), @@ -645,6 +666,49 @@ async fn header_only_service_requests_preserve_body_boundary() -> std::result::R Ok(()) } +#[tokio::test(flavor = "multi_thread")] +async fn commit_header_range_rejects_missing_tree_aux_roots() -> std::result::Result<(), BoxError> { + let _init_guard = zebra_test::init(); + let network = Network::Mainnet; + let (state_service, _read_state, _, _) = + StateService::new(Config::ephemeral(), &network, Height::MAX, 0).await; + let genesis = + zebra_test::vectors::BLOCK_MAINNET_GENESIS_BYTES.zcash_deserialize_into::>()?; + let block1 = + zebra_test::vectors::BLOCK_MAINNET_1_BYTES.zcash_deserialize_into::>()?; + + let state = Buffer::new(BoxService::new(state_service), 1); + assert_eq!( + state + .clone() + .oneshot(Request::CommitCheckpointVerifiedBlock( + CheckpointVerifiedBlock::from(genesis.clone()), + )) + .await?, + Response::Committed(genesis.hash()), + ); + + let error = state + .oneshot(Request::CommitHeaderRange { + anchor: genesis.hash(), + headers: vec![block1.header.clone()], + body_sizes: vec![0], + tree_aux_roots: Vec::new(), + }) + .await + .expect_err("missing roots must reject a non-empty header range"); + + assert!(matches!( + error.downcast_ref::(), + Some(crate::CommitHeaderRangeError::TreeAuxRootCountMismatch { + headers: 1, + roots: 0, + }) + )); + + Ok(()) +} + /// A node still in the finalized (checkpoint) write phase must be able to commit /// a Zakura header range. /// @@ -704,7 +768,7 @@ async fn commit_header_range_completes_while_in_finalized_write_phase( anchor: genesis.hash(), headers: vec![block1.header.clone(), block2.header.clone()], body_sizes: vec![999_999, 0], - tree_aux_roots: Vec::new(), + tree_aux_roots: roots_from_height(Height(1), 2), }), ) .await @@ -757,9 +821,11 @@ async fn header_range_reads_include_non_finalized_best_chain_blocks() -> Result< chain = chain.push(block1.clone().prepare().test_with_zero_spent_utxos())?; chain = chain.push(block2.clone().prepare().test_with_zero_spent_utxos())?; + let chain = Arc::new(chain); + assert_eq!( headers_by_height_range( - Some(Arc::new(chain)), + Some(chain.clone()), &state_service.read_service.db, start, 2, @@ -769,6 +835,44 @@ async fn header_range_reads_include_non_finalized_best_chain_blocks() -> Result< (start.next().unwrap(), block2_hash, block2.header.clone()), ], ); + let roots = block_roots_by_height_range(Some(chain), &state_service.read_service.db, start, 2); + assert_eq!(roots.len(), 2); + assert_eq!(roots[0].height, start); + assert_eq!(roots[1].height, start.next().unwrap()); + let verified_tip = ((start - 1).unwrap(), block::Hash([0; 32])); + let best_header_tip = (start.next().unwrap(), block2_hash); + assert_eq!( + root_covered_best_header_tip( + None::>, + &state_service.read_service.db, + Some(best_header_tip), + Some(verified_tip), + ), + Some(verified_tip), + "rootless durable header tips are capped to the verified block tip" + ); + assert_eq!( + root_covered_best_header_tip( + Some(Arc::new( + Chain::new( + &network, + (start - 1).unwrap(), + Default::default(), + Default::default(), + Default::default(), + Default::default(), + ValueBalance::fake_populated_pool(), + ) + .push(block1.prepare().test_with_zero_spent_utxos())? + .push(block2.prepare().test_with_zero_spent_utxos())?, + )), + &state_service.read_service.db, + Some(best_header_tip), + Some(verified_tip), + ), + Some(best_header_tip), + "verified non-finalized roots allow the header tip to stay ahead" + ); assert_eq!( headers_by_height_range(None::>, &state_service.read_service.db, start, 2), Vec::new(), diff --git a/zebrad/src/commands/start.rs b/zebrad/src/commands/start.rs index a14aafd719a..9abacb0cb02 100644 --- a/zebrad/src/commands/start.rs +++ b/zebrad/src/commands/start.rs @@ -2065,13 +2065,14 @@ mod zakura_header_sync_driver_tests { use zebra_test::vectors::{BLOCK_MAINNET_1_BYTES, BLOCK_MAINNET_2_BYTES}; use super::zakura::{ - apply_block_sync_body, block_apply_class, block_sync_chain_tip_event, - block_sync_missing_body_window, block_sync_needed_blocks_from_state, - block_verify_error_is_duplicate, body_sizes_for_served_header_range, - chain_tip_mirror_frontier_change, coalesce_ready_needed_block_queries, - coalesce_stale_needed_block_queries, commit_block_sync_body, drive_block_sync_actions, - drive_zakura_header_sync_actions, header_range_commit_failure_kind, - notify_block_sync_header_tip, query_block_sync_frontiers, query_block_sync_needed_blocks, + apply_block_sync_body, block_apply_class, block_roots_cover_range, + block_sync_chain_tip_event, block_sync_missing_body_window, + block_sync_needed_blocks_from_state, block_verify_error_is_duplicate, + body_sizes_for_served_header_range, chain_tip_mirror_frontier_change, + coalesce_ready_needed_block_queries, coalesce_stale_needed_block_queries, + commit_block_sync_body, drive_block_sync_actions, drive_zakura_header_sync_actions, + header_range_commit_failure_kind, notify_block_sync_header_tip, query_block_sync_frontiers, + query_block_sync_needed_blocks, root_covered_query_best_header_tip, tree_aux_roots_for_served_header_range, verified_block_tip_from_state, BlockApplyClass, BlocksyncThroughputProbe, ZakuraHeaderSyncDriverHandles, ZAKURA_BLOCK_SYNC_CHECKPOINT_FRONTIER_REFRESH_INTERVAL, ZAKURA_BLOCK_SYNC_DRIVER_TIMEOUT, @@ -2170,6 +2171,15 @@ mod zakura_header_sync_driver_tests { body_sizes_for_served_header_range(start, header_heights, &[]), vec![0, 0, 0, 0], ); + + assert_eq!( + body_sizes_for_served_header_range( + start, + [block::Height(9), block::Height(10)], + &body_size_hints, + ), + vec![0, 100], + ); } #[test] @@ -2212,6 +2222,54 @@ mod zakura_header_sync_driver_tests { ); } + #[test] + fn startup_root_backfill_gate_requires_complete_root_coverage() { + let start = block::Height(10); + let complete_roots = [ + root_at(block::Height(10)), + root_at(block::Height(11)), + root_at(block::Height(12)), + ]; + assert!(block_roots_cover_range(start, 3, &complete_roots)); + assert!(!block_roots_cover_range(start, 3, &complete_roots[..2])); + + let roots_with_gap = [ + root_at(block::Height(10)), + root_at(block::Height(12)), + root_at(block::Height(13)), + ]; + assert!(!block_roots_cover_range(start, 3, &roots_with_gap)); + } + + #[tokio::test] + async fn query_best_header_tip_is_capped_when_roots_are_missing() { + let verified_tip = (block::Height(0), block::Hash([0; 32])); + let durable_header_tip = (block::Height(2), block::Hash([2; 32])); + let read_state = service_fn(move |request: zebra_state::ReadRequest| async move { + match request { + zebra_state::ReadRequest::Tip => Ok::<_, zebra_state::BoxError>( + zebra_state::ReadResponse::Tip(Some(verified_tip)), + ), + zebra_state::ReadRequest::BlockRoots { + start_height, + count, + } => { + assert_eq!(start_height, block::Height(1)); + assert_eq!(count, 2); + Ok(zebra_state::ReadResponse::BlockRoots(Vec::new())) + } + request => panic!("unexpected read request: {request:?}"), + } + }); + + assert_eq!( + root_covered_query_best_header_tip(read_state, durable_header_tip) + .await + .expect("capped query succeeds"), + verified_tip + ); + } + #[test] fn block_verify_error_duplicate_classifier_detects_router_and_block_errors() { let hash = block::Hash([1; 32]); diff --git a/zebrad/src/commands/start/zakura/header_sync_driver.rs b/zebrad/src/commands/start/zakura/header_sync_driver.rs index 6c5f0dc690b..a1d9ca0bec7 100644 --- a/zebrad/src/commands/start/zakura/header_sync_driver.rs +++ b/zebrad/src/commands/start/zakura/header_sync_driver.rs @@ -49,6 +49,7 @@ pub(crate) async fn zakura_header_sync_driver_startup( }; let verified_block_tip = match read_state + .clone() .oneshot(zebra_state::ReadRequest::Tip) .await .map_err(|error| eyre!("{error}"))? @@ -61,17 +62,118 @@ pub(crate) async fn zakura_header_sync_driver_startup( let finalized_height = finalized_tip.map_or(block::Height(0), |(height, _)| height); let verified_block_tip = verified_block_tip_from_state(finalized_tip, verified_block_tip, empty_state_tip); + let best_header_tip = root_covered_best_header_tip_or_verified( + read_state, + best_header_tip.unwrap_or(empty_state_tip), + verified_block_tip, + ) + .await?; + Ok(ZakuraHeaderSyncDriverStartup { frontiers: HeaderSyncFrontiers { finalized_height, verified_block_tip: verified_block_tip.0, verified_block_hash: verified_block_tip.1, }, - best_header_tip: Some(best_header_tip.unwrap_or(empty_state_tip)), + best_header_tip: Some(best_header_tip), verified_block_tip_hash: verified_block_tip.1, }) } +async fn root_covered_best_header_tip_or_verified( + read_state: ReadState, + best_header_tip: (block::Height, block::Hash), + verified_block_tip: (block::Height, block::Hash), +) -> Result<(block::Height, block::Hash), Report> +where + ReadState: Service< + zebra_state::ReadRequest, + Response = zebra_state::ReadResponse, + Error = zebra_state::BoxError, + > + Send + + 'static, + ReadState::Future: Send + 'static, +{ + if best_header_tip.0 <= verified_block_tip.0 { + return Ok(best_header_tip); + } + + let Ok(start_height) = verified_block_tip.0.next() else { + return Ok(verified_block_tip); + }; + let best_header_height = best_header_tip.0; + let verified_block_height = verified_block_tip.0; + let count = best_header_height + .0 + .checked_sub(verified_block_height.0) + .ok_or_else(|| eyre!("best header tip is unexpectedly below verified block tip"))?; + let roots = match read_state + .oneshot(zebra_state::ReadRequest::BlockRoots { + start_height, + count, + }) + .await + .map_err(|error| eyre!("{error}"))? + { + zebra_state::ReadResponse::BlockRoots(roots) => roots, + response => Err(eyre!("unexpected BlockRoots response: {response:?}"))?, + }; + + if block_roots_cover_range(start_height, count, &roots) { + Ok(best_header_tip) + } else { + Ok(verified_block_tip) + } +} + +pub(crate) async fn root_covered_query_best_header_tip( + read_state: ReadState, + best_header_tip: (block::Height, block::Hash), +) -> Result<(block::Height, block::Hash), Report> +where + ReadState: Service< + zebra_state::ReadRequest, + Response = zebra_state::ReadResponse, + Error = zebra_state::BoxError, + > + Clone + + Send + + 'static, + ReadState::Future: Send + 'static, +{ + let verified_block_tip = match read_state + .clone() + .oneshot(zebra_state::ReadRequest::Tip) + .await + .map_err(|error| eyre!("{error}"))? + { + zebra_state::ReadResponse::Tip(Some(tip)) => tip, + zebra_state::ReadResponse::Tip(None) => return Ok(best_header_tip), + response => Err(eyre!("unexpected Tip response: {response:?}"))?, + }; + + root_covered_best_header_tip_or_verified(read_state, best_header_tip, verified_block_tip).await +} + +pub(crate) fn block_roots_cover_range( + start_height: block::Height, + count: u32, + roots: &[BlockCommitmentRoots], +) -> bool { + if roots.len() != usize::try_from(count).unwrap_or(usize::MAX) { + return false; + } + + roots.iter().enumerate().all(|(offset, roots)| { + let Ok(offset) = u32::try_from(offset) else { + return false; + }; + start_height + .0 + .checked_add(offset) + .is_some_and(|height| roots.height == block::Height(height)) + }) +} + #[derive(Clone)] pub(crate) struct ZakuraHeaderSyncDriverHandles { pub(crate) endpoint: ZakuraEndpoint, @@ -678,12 +780,37 @@ pub(crate) async fn drive_zakura_header_sync_actions { + Ok(zebra_state::ReadResponse::BestHeaderTip(Some(best_header_tip))) => { + let (tip_height, tip_hash) = match root_covered_query_best_header_tip( + read_state.clone(), + best_header_tip, + ) + .await + { + Ok(tip) => tip, + Err(error) => { + trace_state_read_error( + &trace, + "query_best_header_tip_roots", + None, + best_header_tip.0, + 1, + &format!("{error}"), + started, + ); + warn!( + ?error, + "failed to apply Zakura root coverage to best header tip" + ); + continue; + } + }; emit_commit_state( &trace, cs_trace::STATE_READ_SUCCESS, @@ -825,6 +952,10 @@ pub(crate) fn body_sizes_for_served_header_range( header_heights .into_iter() .map(|height| { + if height < start { + return 0; + } + let Some(offset) = usize::try_from(height - start).ok() else { return 0; }; @@ -986,6 +1117,9 @@ pub(crate) fn header_range_commit_failure_kind( } zebra_state::CommitHeaderRangeError::EmptyRange | zebra_state::CommitHeaderRangeError::RangeTooLong { .. } + | zebra_state::CommitHeaderRangeError::BodySizeCountMismatch { .. } + | zebra_state::CommitHeaderRangeError::TreeAuxRootCountMismatch { .. } + | zebra_state::CommitHeaderRangeError::TreeAuxRootHeightMismatch { .. } | zebra_state::CommitHeaderRangeError::UnknownAnchor { .. } | zebra_state::CommitHeaderRangeError::HeightOverflow | zebra_state::CommitHeaderRangeError::ImmutableConflict { .. } From 58ba9da0a4e9cf2898aa04050f8a077ff0efd104 Mon Sep 17 00:00:00 2001 From: evan-forbes Date: Fri, 26 Jun 2026 20:40:05 -0500 Subject: [PATCH 2/4] test(zebrad): re-export header root-coverage helpers for driver tests The zakura_header_sync_driver_tests module imports block_roots_cover_range and root_covered_query_best_header_tip through super::zakura::, but the zakura mod never re-exported them, so the zebrad lib test target failed to compile (E0432, with a cascading E0282). Add both to the #[cfg(test)] re-export block. They are pub(crate) in header_sync_driver and already used by production code in that module. --- zebrad/src/commands/start/zakura/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/zebrad/src/commands/start/zakura/mod.rs b/zebrad/src/commands/start/zakura/mod.rs index 67051364a35..bff8f791fd8 100644 --- a/zebrad/src/commands/start/zakura/mod.rs +++ b/zebrad/src/commands/start/zakura/mod.rs @@ -24,9 +24,10 @@ pub(crate) use block_sync_driver::{ pub(crate) use frontier::{query_block_sync_frontiers, verified_block_tip_from_state}; #[cfg(test)] pub(crate) use header_sync_driver::{ - block_sync_chain_tip_event, body_sizes_for_served_header_range, + block_roots_cover_range, block_sync_chain_tip_event, body_sizes_for_served_header_range, chain_tip_mirror_frontier_change, header_range_commit_failure_kind, - notify_block_sync_header_tip, tree_aux_roots_for_served_header_range, + notify_block_sync_header_tip, root_covered_query_best_header_tip, + tree_aux_roots_for_served_header_range, }; pub(crate) use header_sync_driver::{ drive_zakura_header_sync_actions, mirror_zakura_full_block_commits, From c718280da877e85fa034a15af932aceab2c04b83 Mon Sep 17 00:00:00 2001 From: evan-forbes Date: Fri, 26 Jun 2026 20:40:19 -0500 Subject: [PATCH 3/4] docs: PR #282 review notes and header-sync-roots follow-ups --- docs/plans/headersync_roots_review.md | 88 +++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 docs/plans/headersync_roots_review.md diff --git a/docs/plans/headersync_roots_review.md b/docs/plans/headersync_roots_review.md new file mode 100644 index 00000000000..dd5e62366bd --- /dev/null +++ b/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++ / + `` 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. From 28ef2408340984d190db4bcac9a6cd54f025ee27 Mon Sep 17 00:00:00 2001 From: roman Date: Fri, 26 Jun 2026 23:16:30 -0600 Subject: [PATCH 4/4] comments --- zebra-state/src/service.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/zebra-state/src/service.rs b/zebra-state/src/service.rs index ef40175172f..751e35e7f87 100644 --- a/zebra-state/src/service.rs +++ b/zebra-state/src/service.rs @@ -1487,6 +1487,7 @@ where headers } +// Returns the block commitment roots for the given height range fn block_roots_by_height_range( chain: Option, db: &ZebraDb, @@ -1496,16 +1497,19 @@ fn block_roots_by_height_range( where C: AsRef, { + // Cap the count to the maximum header sync height range let mut roots = Vec::with_capacity( usize::try_from(count.min(MAX_HEADER_SYNC_HEIGHT_RANGE)) .expect("capped root count fits in usize"), ); + // Iterate over the height range for offset in 0..count.min(MAX_HEADER_SYNC_HEIGHT_RANGE) { let Some(height) = start + i64::from(offset) else { break; }; + // If the height is at or below the finalized tip height, serve the roots from the finalized state let root = if db .finalized_tip_height() .is_some_and(|finalized_tip| height <= finalized_tip) @@ -1513,6 +1517,7 @@ where finalized_state::serve_block_roots(db, height..=height) .into_iter() .next() + // If the height is in the chain, serve the roots from the chain } else if let Some(chain) = chain .as_ref() .map(|chain| chain.as_ref()) @@ -1529,6 +1534,7 @@ where }), _ => None, } + // If the height is not in the chain, serve the roots from the zakura header commitment roots by height range } else { db.zakura_header_commitment_roots_by_height_range(height..=height) .into_iter() @@ -1549,6 +1555,7 @@ where roots } +// Returns true if the given roots cover the given height range fn block_roots_cover_range( start_height: block::Height, count: u32, @@ -1569,6 +1576,8 @@ fn block_roots_cover_range( }) } +// Return the highest known tip, but cap it to the verified block tip +// if the header-only extension is not root-covered. fn root_covered_best_header_tip( chain: Option, db: &ZebraDb, @@ -1578,12 +1587,15 @@ fn root_covered_best_header_tip( where C: AsRef, { + // Choose the best candidate between the best disk header tip and the verified block tip let best_header_tip = match (best_disk_header_tip, verified_block_tip) { (Some(header_tip), Some(block_tip)) if block_tip.0 > header_tip.0 => Some(block_tip), (Some(header_tip), _) => Some(header_tip), (None, block_tip) => block_tip, }?; + // Is the chosen candidate already at or below the verified block tip? + // If yes, there no header-only gap. let Some(verified_block_tip) = verified_block_tip else { return Some(best_header_tip); };