From f772f0370f0494db273bf165260ef66213c388fe Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 30 Oct 2026 17:52:35 -0300 Subject: [PATCH 1/2] Build the parent-to-children map once in get_filtered_block_tree instead of scanning the whole block index at every node of filter_block_tree. The scan ran once per node of the justified subtree over the full live window since finality, so the head walk was quadratic in the number of unfinalized blocks, exactly what a finality stall or a range catch-up grows. walk_head and invalidate_subtree already build the map this way; this brings the filter in line with them. A new test keeps the spec-literal scan as a reference and checks the two agree on random trees, including ones where a branch is filtered out. --- .../src/beacon/fork_choice.rs | 137 +++++++++++++++--- 1 file changed, 115 insertions(+), 22 deletions(-) diff --git a/crates/blockchain/state_transition/src/beacon/fork_choice.rs b/crates/blockchain/state_transition/src/beacon/fork_choice.rs index 02c959376..ac83d85fe 100644 --- a/crates/blockchain/state_transition/src/beacon/fork_choice.rs +++ b/crates/blockchain/state_transition/src/beacon/fork_choice.rs @@ -49,11 +49,14 @@ //! //! # Why `Store::block_index` never needs to be a `BTreeMap` //! -//! The one place this file iterates every block the store holds is -//! `filter_block_tree`'s scan of [`Store::block_index`](ethlambda_storage::Store::block_index) -//! for a block's children, and [`get_head`]'s equivalent scan of the tree -//! `filter_block_tree` already filtered down. Both immediately reduce that -//! scan to a single winner via an explicit, fully-ordered sort key +//! The places this file iterates every block the store holds are the one-pass +//! parent-to-children map builds over +//! [`Store::block_index`](ethlambda_storage::Store::block_index) +//! (`get_filtered_block_tree`, `walk_head`, `invalidate_subtree`), and +//! [`compute_head`]'s scan of the tree `filter_block_tree` already filtered +//! down. A child map's `Vec`s hold the children in whatever order the map +//! yielded them, and every reader either visits all of them or reduces them +//! to a single winner via an explicit, fully-ordered sort key //! (`(weight, root)`, with `root` breaking ties the same way Python compares //! two `bytes` values, since [`Root`]'s `Ord` compares its bytes in the same //! order). Two distinct blocks never share a root, so that key never actually @@ -1878,16 +1881,18 @@ pub fn get_voting_source( /// /// Takes `index`, [`Store::block_index`] built once by /// [`get_filtered_block_tree`] and threaded through every recursive call, -/// rather than re-scanning the store's blocks at each node: the children scan -/// below is exactly the whole-map iteration the module documentation says -/// this file never needs a `BTreeMap` for, and it runs once per node visited, -/// not once per node per DB round trip. `blocks`' value is `index`'s own -/// `(slot, parent_root)` shape rather than a whole [`SignedBeaconBlock`], -/// since [`get_head`], the only reader of this function's output, never needs -/// more than that. +/// rather than re-scanning the store's blocks at each node, and `children_of`, +/// the parent-to-children map [`get_filtered_block_tree`] builds from `index` +/// in one pass. The specification finds a block's children by scanning every +/// block in the store; doing that here at every node visited made the walk +/// quadratic in the live window, which is what a finality stall or a range +/// catch-up grows. `blocks`' value is `index`'s own `(slot, parent_root)` +/// shape rather than a whole [`SignedBeaconBlock`], since [`get_head`], the +/// only reader of this function's output, never needs more than that. pub fn filter_block_tree( store: &Store, index: &HashMap, + children_of: &HashMap>, block_root: Root, blocks: &mut HashMap, config: &Config, @@ -1896,18 +1901,17 @@ pub fn filter_block_tree( .get(&block_root) .ok_or(Error::SpecAssert("block_root in store.blocks"))?; - let children: Vec = index - .iter() - .filter(|&(_, &(_, parent_root))| parent_root == block_root) - .map(|(&root, _)| root) - .collect(); + let children = children_of + .get(&block_root) + .map(Vec::as_slice) + .unwrap_or(&[]); // If any children branches contain expected finalized/justified // checkpoints, add to filtered block-tree and signal viability to parent. if !children.is_empty() { let mut any_viable = false; - for child in children { - if filter_block_tree(store, index, child, blocks, config)? { + for &child in children { + if filter_block_tree(store, index, children_of, child, blocks, config)? { any_viable = true; } } @@ -1947,14 +1951,22 @@ pub fn filter_block_tree( /// The filtered block tree: every block, from the justified checkpoint down, /// whose leaf state's justified/finalized info agrees with `store`'s own. +/// +/// One index scan builds the whole child map before the walk starts, the way +/// [`invalidate_subtree`] and [`walk_head`] do, so [`filter_block_tree`] does +/// a lookup per node rather than a scan per node. pub fn get_filtered_block_tree( store: &Store, index: &HashMap, config: &Config, ) -> Result> { + let mut children_of: HashMap> = HashMap::new(); + for (&root, &(_, parent_root)) in index { + children_of.entry(parent_root).or_default().push(root); + } let base = store.beacon_justified_checkpoint().root; let mut blocks = HashMap::new(); - filter_block_tree(store, index, base, &mut blocks, config)?; + filter_block_tree(store, index, &children_of, base, &mut blocks, config)?; Ok(blocks) } @@ -1970,8 +1982,8 @@ pub fn get_filtered_block_tree( /// /// The children scan in the loop below reads `blocks`, [`get_filtered_block_tree`]'s /// already-filtered, already in-memory result, not [`Store::block_index`] -/// itself: it is the specification's own second whole-`Dict` scan the module -/// documentation calls out, but it never costs a further backend round trip. +/// itself: it is the specification's own whole-`Dict` scan, kept literal here +/// because this is the reference, and it never costs a backend round trip. /// /// Takes `&Store` rather than `&mut Store`: nothing here records the head it /// finds. No node code calls it; only tests do. @@ -9918,6 +9930,87 @@ mod tests { ); } + /// The specification's `filter_block_tree` finds a block's children by + /// scanning every block in the store. The node's version looks them up in + /// a parent-to-children map built once per call, so this keeps the scan + /// as the reference and checks the two agree on every random tree, at + /// current slots where the filter keeps everything and where it drops a + /// branch. + fn filter_block_tree_by_scan( + store: &Store, + index: &HashMap, + block_root: Root, + blocks: &mut HashMap, + config: &Config, + ) -> Result { + let children: Vec = index + .iter() + .filter(|&(_, &(_, parent_root))| parent_root == block_root) + .map(|(&root, _)| root) + .collect(); + if children.is_empty() { + // A leaf's viability is the same question either way, so the + // reference asks the node's own code for it: an empty child map + // makes `filter_block_tree` take its leaf branch. + let no_children = HashMap::new(); + return filter_block_tree(store, index, &no_children, block_root, blocks, config); + } + let mut any_viable = false; + for child in children { + if filter_block_tree_by_scan(store, index, child, blocks, config)? { + any_viable = true; + } + } + if any_viable { + blocks.insert(block_root, index[&block_root]); + } + Ok(any_viable) + } + + #[test] + fn the_filtered_tree_matches_the_spec_literal_children_scan_on_random_trees() { + let mut branches_filtered_out = 0; + for seed in 0..100 { + let mut tree = random_tree(seed, Arc::new(InMemoryBackend::new())); + let config = tree.config.clone(); + let max_slot = tree.slots.iter().copied().max().unwrap(); + let mut current_slots = vec![max_slot + 1]; + if tree.justified_moved { + current_slots.push(tree.first_gloas_slot + 3 * preset::SLOTS_PER_EPOCH); + } + for current_slot in current_slots { + tree.store + .set_time_ms(current_slot * config.slot_duration_ms) + .unwrap(); + let index = tree.store.block_index(); + let justified = tree.store.beacon_justified_checkpoint().root; + + let mut by_scan = HashMap::new(); + filter_block_tree_by_scan(&tree.store, &index, justified, &mut by_scan, &config) + .unwrap_or_else(|err| panic!("seed {seed}: scan reference: {err:?}")); + let by_map = get_filtered_block_tree(&tree.store, &index, &config) + .unwrap_or_else(|err| panic!("seed {seed}: filtered tree: {err:?}")); + + assert_eq!( + by_map, by_scan, + "seed {seed} slot {current_slot}: filtered tree differs from the scan" + ); + let justified_subtree = tree + .roots + .iter() + .filter(|&&root| descends_from_justified(&index, root, justified)) + .count(); + if by_map.len() < justified_subtree { + branches_filtered_out += 1; + } + } + } + assert!( + branches_filtered_out > 0, + "the filter never dropped a branch, so the comparison covered only full trees" + ); + } + /// The payload link derived lazily from the blocks is the specification's /// parent payload status. /// From b102e35ce95bfcc9479a6c6fcc3d273b7f15fe1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Mon, 2 Nov 2026 16:04:34 -0300 Subject: [PATCH 2/2] fix(fork_choice): walk the filtered block tree with an explicit stack filter_block_tree recursed once per block below the justified checkpoint. That depth grows by a block every slot for as long as the chain does not justify, and get_head runs on the chain actor's thread, which spawned-concurrency starts with std::thread::spawn and so the default 2 MiB stack. On that stack the recursive walk overflowed between 5,500 and 6,000 blocks of depth (release-fast): about 18 to 20 hours of a stall with no new justification. A stack overflow aborts the process, and the next head computation after a restart walks the same tree. The walk now keeps its own stack of the path from the justified block to the block in hand. It visits the same blocks in the same order, asserts `block_root in store.blocks` at the same points and returns the same tree. The leaf check moved unchanged into is_viable_leaf, which returns the verdict and leaves the insertion to the walk. --- .../src/beacon/fork_choice.rs | 189 ++++++++++++++---- 1 file changed, 153 insertions(+), 36 deletions(-) diff --git a/crates/blockchain/state_transition/src/beacon/fork_choice.rs b/crates/blockchain/state_transition/src/beacon/fork_choice.rs index ac83d85fe..f0216fddf 100644 --- a/crates/blockchain/state_transition/src/beacon/fork_choice.rs +++ b/crates/blockchain/state_transition/src/beacon/fork_choice.rs @@ -1873,17 +1873,23 @@ pub fn get_voting_source( /// `blocks`, and reporting whether `block_root` itself sits on one. /// /// *Note*: external callers must pass `store.justified_checkpoint.root` for -/// `block_root`; only the recursive calls below pass anything else. -/// -/// Recursive, following the specification directly rather than an explicit -/// stack: the subtree walked here is the unfinalized suffix since the -/// justified checkpoint, which stays shallow in ordinary operation. +/// `block_root`; only the spec-literal reference in the tests passes anything +/// else. +/// +/// The specification recurses once per child. This visits the same blocks in +/// the same order, failing at the same block, but keeps its own stack of the +/// path from `block_root` to the block in hand. That path is as long as the +/// chain below the justified checkpoint: short while the chain justifies, but +/// a block longer every slot for as long as it does not, and the chain actor +/// runs this on a thread with the default-sized stack. Recursing, the walk +/// overflowed that stack once a stall had left a few thousand blocks below the +/// justified one, and a stack overflow aborts the whole process. /// /// Takes `index`, [`Store::block_index`] built once by -/// [`get_filtered_block_tree`] and threaded through every recursive call, -/// rather than re-scanning the store's blocks at each node, and `children_of`, -/// the parent-to-children map [`get_filtered_block_tree`] builds from `index` -/// in one pass. The specification finds a block's children by scanning every +/// [`get_filtered_block_tree`] and read throughout the walk, rather than +/// re-scanning the store's blocks at each node, and `children_of`, the +/// parent-to-children map [`get_filtered_block_tree`] builds from `index` in +/// one pass. The specification finds a block's children by scanning every /// block in the store; doing that here at every node visited made the walk /// quadratic in the live window, which is what a finality stall or a range /// catch-up grows. `blocks`' value is `index`'s own `(slot, parent_root)` @@ -1897,31 +1903,76 @@ pub fn filter_block_tree( blocks: &mut HashMap, config: &Config, ) -> Result { - let entry = *index - .get(&block_root) - .ok_or(Error::SpecAssert("block_root in store.blocks"))?; + /// A block on the path from `block_root` to the block in hand: which of + /// its children is next, and whether any visited so far is viable. + struct Frame<'a> { + root: Root, + entry: (Slot, Root), + children: &'a [Root], + next_child: usize, + any_viable: bool, + } - let children = children_of - .get(&block_root) - .map(Vec::as_slice) - .unwrap_or(&[]); + /// Enters `root`, where the specification's recursive call would: its + /// `block_root in store.blocks` assertion included, which is what fails + /// the walk when the justified block's row is gone. + fn enter<'a>( + index: &HashMap, + children_of: &'a HashMap>, + root: Root, + ) -> Result> { + let entry = *index + .get(&root) + .ok_or(Error::SpecAssert("block_root in store.blocks"))?; + let children = children_of.get(&root).map(Vec::as_slice).unwrap_or(&[]); + Ok(Frame { + root, + entry, + children, + next_child: 0, + any_viable: false, + }) + } - // If any children branches contain expected finalized/justified - // checkpoints, add to filtered block-tree and signal viability to parent. - if !children.is_empty() { - let mut any_viable = false; - for &child in children { - if filter_block_tree(store, index, children_of, child, blocks, config)? { - any_viable = true; - } + let mut path = vec![enter(index, children_of, block_root)?]; + loop { + let top = path + .last_mut() + .expect("the walk returns once the path is empty"); + // A block with children is viable if any child branch contains the + // expected finalized/justified checkpoints; a leaf, if its own do. + let viable = if top.children.is_empty() { + is_viable_leaf(store, index, top.root, config)? + } else if let Some(&child) = top.children.get(top.next_child) { + top.next_child += 1; + path.push(enter(index, children_of, child)?); + continue; + } else { + top.any_viable + }; + + // Add it to the filtered block-tree if viable, and signal viability + // to its parent. + let done = path.pop().expect("`top` was on the path"); + if viable { + blocks.insert(done.root, done.entry); } - if any_viable { - blocks.insert(block_root, entry); - return Ok(true); + match path.last_mut() { + Some(parent) => parent.any_viable |= viable, + None => return Ok(viable), } - return Ok(false); } +} +/// Whether the leaf `block_root` is viable: its voting source is the store's +/// justified checkpoint or recent enough, and it descends from the store's +/// finalized checkpoint. The leaf case of [`filter_block_tree`]. +fn is_viable_leaf( + store: &Store, + index: &HashMap, + block_root: Root, + config: &Config, +) -> Result { let current_epoch = get_current_store_epoch(store, config); let voting_source = get_voting_source(store, index, block_root, config)?; @@ -1939,14 +1990,7 @@ pub fn filter_block_tree( let correct_finalized = finalized_checkpoint.epoch == constants::GENESIS_EPOCH || finalized_checkpoint.root == finalized_checkpoint_block; - // If expected finalized/justified, add to viable block-tree and signal - // viability to parent. - if correct_justified && correct_finalized { - blocks.insert(block_root, entry); - return Ok(true); - } - - Ok(false) + Ok(correct_justified && correct_finalized) } /// The filtered block tree: every block, from the justified checkpoint down, @@ -10011,6 +10055,79 @@ mod tests { ); } + /// The head walk over a chain as deep as a long finality stall leaves + /// below the justified block, on a thread with the stack + /// `std::thread::spawn` gives by default, which is what the chain actor's + /// thread runs on. + /// + /// A filter that recursed once per block overflowed that stack long before + /// this depth. A stack overflow aborts the whole test binary rather than + /// failing this test alone, so a run that loses every test at once points + /// here. + #[test] + fn the_head_walk_does_not_overflow_a_default_stack_on_a_stall_deep_chain() { + const DEPTH: u64 = 20_000; + // `std::thread`'s documented default, unless `RUST_MIN_STACK` says + // otherwise. Set explicitly so the environment cannot hide a + // regression. + const DEFAULT_THREAD_STACK: usize = 2 * 1024 * 1024; + + let walk = std::thread::Builder::new() + .name("deep-chain-head-walk".into()) + .stack_size(DEFAULT_THREAD_STACK) + .spawn(|| { + let config = Config::active(); + let mut rng = TestRng(DEPTH); + let anchor_root = rng.root(); + let mut store = store_anchored_at(anchor_root); + store + .insert_signed_block(anchor_root, fulu_block(0, Root::ZERO)) + .unwrap(); + let anchor_state = test_state::with_validators_at(ForkName::Fulu, TREE_VALIDATORS); + store.insert_state(anchor_root, anchor_state).unwrap(); + store.set_unrealized_justification(anchor_root, 0, Checkpoint::default()); + + // One chain, nothing justified or finalized past the anchor: + // the justified block is the anchor, so the filter walks the + // whole chain. + let mut tip = anchor_root; + for slot in 1..=DEPTH { + let root = rng.root(); + store + .insert_signed_block(root, fulu_block(slot, tip)) + .unwrap(); + store.set_unrealized_justification(root, slot, Checkpoint::default()); + tip = root; + } + for validator_index in 0..TREE_VALIDATORS as u64 { + let vote = LatestMessage { + epoch: 0, + slot: DEPTH, + root: tip, + payload_present: false, + }; + store.set_latest_message(validator_index, vote); + } + // Past every block's epoch, so the leaf reads its stored + // unrealized justification rather than a state. + let current_slot = DEPTH + 2 * preset::SLOTS_PER_EPOCH; + store + .set_time_ms(current_slot * config.slot_duration_ms) + .unwrap(); + + let index = store.block_index(); + let filtered = get_filtered_block_tree(&store, &index, &config).unwrap(); + assert_eq!( + filtered.len(), + index.len(), + "with nothing justified past the anchor, every block is viable" + ); + assert_eq!(get_head_node(&store, &config).unwrap().root, tip); + }) + .unwrap(); + walk.join().unwrap(); + } + /// The payload link derived lazily from the blocks is the specification's /// parent payload status. ///