From 95679f0b03a7ed413648abb36ef90b6ad13484d6 Mon Sep 17 00:00:00 2001 From: br33zybail <192743600+br33zybail@users.noreply.github.com> Date: Tue, 21 Jul 2026 13:47:49 +0000 Subject: [PATCH 1/2] viz: tolerate a best-chain switch during tfl_block_sequence walk tfl_block_sequence resolves a start hash, then walks FindBlockHashes from it. FindBlockHashes follows the CURRENT best chain, so if the chain switches between those two steps the first returned hash differs from the requested one. The assert here treated that as an internal error and took the whole node down. The window is hit in practice by same-height sibling switches: a stale internal-miner solution (the race #39 fixes) submits after a block was already committed at that height, the node switches best chain, and the viz service thread panics. Three operators reported this crash on 2026-07-20/21 (lib.rs:1939 on s2v9 builds, :1959 on newer); one crashed twice ~8h apart while mining, with the submit->switch->panic sequence visible in his own log. A non-mining node can hit the same window when a sibling arrives over the network. Replace the assert with the surrounding code's own fail-soft idiom: log a PARANOID line and return what we have. The viz2 callers already guard for short or empty sequences and re-request against the new chain on the next loop tick, so a reorg mid-walk now costs one retry instead of the node. Verified: cargo check -p zebra-crosslink clean (no new diagnostics). Co-Authored-By: Claude Fable 5 --- zebra-crosslink/zebra-crosslink/src/lib.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/zebra-crosslink/zebra-crosslink/src/lib.rs b/zebra-crosslink/zebra-crosslink/src/lib.rs index 6fe95a10..1d1548e3 100644 --- a/zebra-crosslink/zebra-crosslink/src/lib.rs +++ b/zebra-crosslink/zebra-crosslink/src/lib.rs @@ -2005,11 +2005,22 @@ async fn tfl_block_sequence( .await; if let Ok(StateResponse::BlockHashes(chunk_hashes)) = res { - if c == 0 && include_start_hash && !chunk_hashes.is_empty() { - assert_eq!( - chunk_hashes[0], start_hash, - "first hash is not the one requested" + if c == 0 && include_start_hash && !chunk_hashes.is_empty() && chunk_hashes[0] != start_hash { + // The best chain switched between resolving start_hash and this + // walk: FindBlockHashes follows the CURRENT best chain, so a + // same-height sibling switch (e.g. a stale internal-miner submit + // displacing a just-committed block, see #39) makes the first + // returned hash differ from the one requested. That is a normal + // reorg race, not an internal error. Return what we have; the + // callers' paranoid guards already retry against the new chain. + // This was a fatal assert that took whole nodes down whenever + // the race hit (multiple operator reports 2026-07-20/21, + // "panicked at zebra-crosslink/src/lib.rs:1939/:1959"). + println!( + "PARANOID != WRONG: first hash {} is not the requested {} (best chain switched mid-walk)", + chunk_hashes[0], start_hash ); + break; } chunk = chunk_hashes; From 2a41ad4e437e0997ec7cc36804dc7e876c351127 Mon Sep 17 00:00:00 2001 From: br33zybail <192743600+br33zybail@users.noreply.github.com> Date: Tue, 21 Jul 2026 15:41:43 +0000 Subject: [PATCH 2/2] viz: add regression tests for the mid-walk best-chain switch Covers the exact scenario from review: mock the BlockHeader lookup for the start hash, then have FindBlockHashes return a same-height sibling as element 0. Asserts no panic, empty result, and (via read_extra_info=true) that no block data is fetched for the sibling walk, so nothing can be published before the caller rereads the tip. A second test pins the consistent-walk behavior so the guard cannot over-fire. Mocks TFLServiceCalls directly: the state procedure is a canned closure, the four unused procedures fail the call if ever hit. cargo test -p zebra-crosslink tfl_block_sequence_reorg: 2 passed, 0 failed. Co-Authored-By: Claude Fable 5 --- zebra-crosslink/zebra-crosslink/src/lib.rs | 124 +++++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/zebra-crosslink/zebra-crosslink/src/lib.rs b/zebra-crosslink/zebra-crosslink/src/lib.rs index 1d1548e3..28b82b01 100644 --- a/zebra-crosslink/zebra-crosslink/src/lib.rs +++ b/zebra-crosslink/zebra-crosslink/src/lib.rs @@ -2197,3 +2197,127 @@ async fn _tfl_dump_block_sequence( .await; tfl_dump_blocks(&blocks[..], &infos[..]); } + +#[cfg(test)] +mod tfl_block_sequence_reorg_tests { + use super::*; + use crate::service::TFLServiceCalls; + use std::sync::atomic::{AtomicUsize, Ordering}; + use zebra_chain::serialization::ZcashDeserializeInto; + + /// Builds `TFLServiceCalls` whose state procedure answers with the given + /// sync closure. The other procedures are unused by `tfl_block_sequence` + /// and fail the call if ever hit. + fn state_only_calls( + state: impl Fn(StateRequest) -> StateResponse + Send + Sync + 'static, + ) -> TFLServiceCalls { + let state = Arc::new(state); + TFLServiceCalls { + state: Arc::new(move |req| { + let state = state.clone(); + Box::pin(async move { Ok(state(req)) }) + }), + read_state: Arc::new(|_| Box::pin(async { Err("read_state unused in test".into()) })), + mempool: Arc::new(|_| Box::pin(async { Err("mempool unused in test".into()) })), + force_feed_pos: Arc::new(|_, _| { + Box::pin(async { Err("force_feed_pos unused in test".to_string()) }) + }), + } + } + + fn test_blocks() -> (Arc, Arc) { + let block_1 = zebra_test::vectors::BLOCK_MAINNET_1_BYTES + .zcash_deserialize_into::>() + .expect("block 1 should deserialize"); + let block_2 = zebra_test::vectors::BLOCK_MAINNET_2_BYTES + .zcash_deserialize_into::>() + .expect("block 2 should deserialize"); + (block_1, block_2) + } + + /// A best-chain switch between resolving the start hash and walking + /// `FindBlockHashes` makes element 0 a same-height sibling of the block we + /// started from. This used to be a fatal assert that aborted the node; it + /// must return empty vectors instead, so the viz2 caller's paranoid guards + /// reject the empty sequence and re-read the tip on the next loop tick. + #[tokio::test] + async fn sibling_first_hash_returns_empty_instead_of_panicking() { + let (block_1, block_2) = test_blocks(); + let start_hash = block_1.hash(); + // stands in for the sibling that displaced block_1 at the same height + let sibling_hash = block_2.hash(); + let header = block_1.header.clone(); + + let calls = state_only_calls(move |req| match req { + StateRequest::BlockHeader(_) => StateResponse::BlockHeader { + header: header.clone(), + hash: start_hash, + height: ZebBlockHeight(1), + next_block_hash: None, + }, + StateRequest::FindBlockHashes { .. } => { + StateResponse::BlockHashes(vec![sibling_hash]) + } + other => panic!("unexpected state request in test: {other:?}"), + }); + + let (hashes, blocks) = tfl_block_sequence( + &calls, + start_hash, + Some((ZebBlockHeight(3), sibling_hash)), + true, + true, + ) + .await; + + assert!(hashes.is_empty(), "no sibling sequence may be consumed"); + assert!( + blocks.is_empty(), + "no blocks may be fetched or published for a sibling walk" + ); + } + + /// The guard must not fire on a consistent walk: element 0 equal to the + /// requested start hash returns the sequence as before. + #[tokio::test] + async fn consistent_walk_still_returns_the_sequence() { + let (block_1, block_2) = test_blocks(); + let start_hash = block_1.hash(); + let next_hash = block_2.hash(); + let header = block_1.header.clone(); + + let find_calls = AtomicUsize::new(0); + let calls = state_only_calls(move |req| match req { + StateRequest::BlockHeader(_) => StateResponse::BlockHeader { + header: header.clone(), + hash: start_hash, + height: ZebBlockHeight(1), + next_block_hash: None, + }, + StateRequest::FindBlockHashes { .. } => { + if find_calls.fetch_add(1, Ordering::SeqCst) == 0 { + StateResponse::BlockHashes(vec![start_hash, next_hash]) + } else { + // end of chain + StateResponse::BlockHashes(Vec::new()) + } + } + other => panic!("unexpected state request in test: {other:?}"), + }); + + let (hashes, blocks) = tfl_block_sequence( + &calls, + start_hash, + Some((ZebBlockHeight(2), next_hash)), + true, + false, + ) + .await; + + assert_eq!( + hashes.iter().map(|h| h.1).collect::>(), + vec![start_hash, next_hash], + ); + assert!(blocks.is_empty(), "read_extra_info=false returns no blocks"); + } +}