[MegaRedHand]

🗒️ Description / Motivation

filter_block_tree recursed once per block below the justified checkpoint. While the chain does not justify, that depth grows by one block every slot, and get_head runs on the chain actor's thread: spawned-concurrency's Backend::Thread starts it with std::thread::spawn, so it gets the default 2 MiB stack (nothing in the node sets a stack size).

On that stack the recursive walk overflowed between 5,500 and 6,000 blocks of depth (release-fast, measured on an M3 Pro and on a Ryzen 7 9700X). On mainnet that is about 18 to 20 hours of a stall in which nothing new is justified. A stack overflow aborts the process, and the next head computation after a restart walks the same tree.

Found while timing #746 on synthetic stall trees (results in a comment there). #746 kept the recursion; its parent-to-children map is what makes each step of the explicit stack a lookup rather than an index scan.

What Changed

• crates/blockchain/state_transition/src/beacon/fork_choice.rs
  • filter_block_tree keeps its own stack of the path from block_root to the block in hand (one frame per block: its index entry, its children, the next child to visit, and whether any child so far is viable) instead of recursing. Same signature.
  • The leaf check moved unchanged into a private is_viable_leaf, which returns the verdict and leaves the insertion to the walk.
  • The doc comment that called the recursion safe ("stays shallow in ordinary operation") now explains why the stack is explicit.
  • New test the_head_walk_does_not_overflow_a_default_stack_on_a_stall_deep_chain.

Correctness / Behavior Guarantees

• Same blocks visited, in the same order (each block's children_of slice order), so the same filtered tree and return value.
• The block_root in store.blocks assertion runs when a block is entered, where each recursive call made it, so a pruned justified root still fails the walk with the same error (update_head_from_fork_choice logs it and keeps the head).
• The first error still ends the walk at the same block.
• The path lives in a heap Vec with one small frame per level of depth, instead of one stack frame per level.

Tests Added / Run

• New: the_head_walk_does_not_overflow_a_default_stack_on_a_stall_deep_chain. It builds a 20,000-block chain with nothing justified past the anchor and runs get_filtered_block_tree and get_head_node on a thread with an explicit 2 MiB stack. With perf(fork_choice): build the parent-to-children map once in get_filtered_block_tree #746's recursive walk it aborts with a stack overflow; with this change it passes in 0.07 s. A stack overflow takes down the whole test binary rather than failing one test, which the test's doc comment says.
• Existing: perf(fork_choice): build the parent-to-children map once in get_filtered_block_tree #746's the_filtered_tree_matches_the_spec_literal_children_scan_on_random_trees compares the walk with the recursive spec-literal reference on 100 random trees, including trees where branches are filtered out.
• Ran:
  • make fmt, make lint: clean
  • fork_choice unit tests: 95 passed
  • make test-consensus: 1,325 passed, 0 failed
  • mainnet fork-choice fixture cases (--test beacon_spec_tests --features beacon-spec-tests -- fork_choice): 206 passed

Not checked: the overflow depth under the release profile, where LTO can change frame sizes.

Related Issues / PRs

• Follows perf(fork_choice): build the parent-to-children map once in get_filtered_block_tree #746 (merged).

✅ Verification Checklist

• Ran make fmt: clean
• Ran make lint (clippy with -D warnings): clean
• Ran make test (test-consensus plus test-node, at release-fast): test-consensus run locally; test-node left to CI (unrelated to this change)

[github-actions]

🤖 Kimi Code Review

I'll review this PR which converts a recursive filter_block_tree to an iterative implementation to prevent stack overflow during finality stalls.

Summary

The PR correctly addresses a critical stack overflow vulnerability in the fork choice filter_block_tree function. The iterative conversion is well-structured, but I have several concerns ranging from minor code quality issues to a potential logic bug in error propagation.

---

Critical Issues

1. Potential Logic Bug: any_viable Not Updated When Child Is Viable Leaf

File: fork_choice.rs, lines 1943-1955

In the iterative loop, when a leaf node is found viable and the stack unwinds:

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) {
    // ...
} else {
    top.any_viable
};

After path.pop(), the parent's any_viable is OR'd with viable. However, consider: when a leaf is viable (is_viable_leaf returns true), it's inserted into blocks. But when we pop back to its parent, we set parent.any_viable |= viable. If that parent has no other children, its own any_viable becomes true, and when we later evaluate it (after all children processed), we use top.any_viable which is correct.

But wait—re-examine the original recursive code:

if filter_block_tree(store, index, children_of, child, blocks, config)? {
    any_viable = true;
}
// ...
if any_viable {
    blocks.insert(block_root, entry);
    return Ok(true);
}
return Ok(false);

The recursive version returns true immediately when any_viable becomes true after processing all children. The iterative version processes all children regardless. This is behaviorally equivalent for correctness (still checks all children), but less efficient. Not a bug, just noting the divergence.

Actually, re-reading more carefully: the iterative version does not short-circuit. This is fine since the spec doesn't require short-circuiting.

Status: Not a bug, behavior is correct.

---

Moderate Issues

2. Missing #[inline] or Visibility Consideration for is_viable_leaf

File: fork_choice.rs, lines 1970-1991

is_viable_leaf is a new pub(crate) or module-private function. Given it's only called from filter_block_tree, consider making it pub(crate) explicitly or adding a doc comment about why it's extracted. The extraction is good for readability, but:

fn is_viable_leaf(

Should this be pub(crate)? It's currently implicitly private to the module. If tests need it, they'd be in the same module. The current test doesn't use it directly. This is fine, but consider whether future tests might want to test leaf viability in isolation.

Status: Minor, acceptable as-is.

---

Code Quality Issues

3. Unnecessary mut on blocks Parameter

File: fork_choice.rs, line 1899

blocks: &mut HashMap<Root, (Slot, Root)>,

This was present in the original, but the PR doesn't address it. The blocks parameter is mutated, but the function signature already reflects this. No issue here—just verifying the PR didn't introduce unnecessary mutability.

Actually, checking: the new code uses blocks.insert(done.root, done.entry); which requires &mut. Correct.

4. Comment Typo/Clarity

File: fork_choice.rs, line 1878

-    /// `block_root`; only the recursive calls below pass anything else.
+    /// `block_root`; only the spec-literal reference in the tests passes anything
+    /// else.

This comment change is slightly misleading. The original said "recursive calls below" pass other values. The new version says "spec-literal reference in the tests" passes anything else. But looking at the actual callers—there are no other callers in production code that pass anything other than store.justified_checkpoint.root. The comment is trying to say that only tests exercise the spec's literal behavior of passing other roots.

However, this is confusing because there are no recursive calls anymore. The comment should be updated to remove the implication that anything in this function passes other values:

Suggested change:

/// *Note*: external callers must pass `store.justified_checkpoint.root` for
/// `block_root`. The specification allows other values, but only test code
/// exercises that path.

5. Test Stack Size Assumption

File: fork_choice.rs, line 10066-10067

// `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;

This is platform-dependent. On some platforms (e.g., musl Linux), the default thread stack is smaller (80KB or 128KB). The comment acknowledges RUST_MIN_STACK but the hardcoded value may not match reality on all targets.

Suggestion: Consider using std::thread::available_parallelism or document that this test assumes glibc-style defaults. Or, better: spawn with an explicitly small stack to prove the fix works even under tight constraints, rather than assuming default:

.stack_size(512 * 1024) // intentionally small to stress-test

This would make the test more robust across platforms and actually validate that the iterative approach works with minimal stack.

6. Test RNG Seed Reuse Pattern

File: fork_choice.rs, line 10072

let mut rng = TestRng(DEPTH);

Using DEPTH as the seed is deterministic but slightly odd—usually seeds are fixed constants. This is fine since it's deterministic, but if DEPTH changes, the "random" values change too. Not a bug, just unconventional.

---

Positive Observations

1. Correct iterative transformation: The explicit stack Frame struct correctly models the call stack, with next_child as the program counter and any_viable as the return value accumulator.

2. Memory safety: No unsafe code introduced; the borrow checker is correctly satisfied with 'a lifetime on children.

3. Error propagation: The ? operator in enter and is_viable_leaf correctly propagates errors without leaking resources (the path Vec is dropped normally on panic or error return).

4. Test coverage: The deep chain test is excellent— it actually validates the fix under realistic stall conditions.

5. Performance: The iterative version avoids stack growth; heap allocation for path is bounded by chain depth (same as recursive stack growth, but on heap where it's safe). No additional allocations per node compared to recursive version.

---

Suggested Changes

Change 1: Clarify comment (line 1877-1880)

/// *Note*: external callers must pass `store.justified_checkpoint.root` for
/// `block_root`. The specification's recursive formulation allows passing
/// other roots; only test code exercises that path.

Change 2: Make test more portable (line 10066-10075)

Consider using a smaller explicit stack to prove robustness:

const TEST_STACK_SIZE: usize = 512 * 1024; // 512KB: smaller than most defaults
// ...
.stack_size(TEST_STACK_SIZE)

This makes the test pass on more platforms and be a stronger regression test.

---

Final Assessment

Aspect	Rating	Notes
Correctness	✅ Good	Iterative transformation preserves semantics
Security	✅ Improved	Eliminates DoS via stack overflow
Performance	✅ Neutral	Same complexity, heap instead of stack
Safety	✅ Good	No unsafe, proper error handling
Maintainability	⚠️ Acceptable	Frame struct is clear; comments could be sharper
Testing	✅ Good	Deep chain test validates the fix

Overall: Approve with minor suggestions. The core fix is correct and important. Address the comment clarity and consider the test stack size adjustment.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

[github-actions]

🤖 Codex Code Review

⚠️ The review job did not complete (result: failure). See the job log

---

Automated review by OpenAI Codex

[github-actions]

🤖 Claude Code Review

Review: fix(fork_choice): walk the filtered block tree with an explicit stack

I read the change in crates/blockchain/state_transition/src/beacon/fork_choice.rs:1898-1990 and its reference test near line 9983. I did not build it or run the tests.

The change looks correct.

• Same traversal as the recursion. The walk enters the children in order and records a node only after its subtree finishes. A node with children is viable if any child is, and a leaf is judged by is_viable_leaf. any_viable |= viable after the pop matches the original any_viable = true in the loop.
• Same assert, same place. enter does the block_root in store.blocks lookup when a node is entered, as the recursive call did, so a missing justified row still gives SpecAssert at the same point. The ? on is_viable_leaf and enter returns early, as before.
• Leaf logic unchanged. is_viable_leaf is the old leaf check, now returning a verdict instead of inserting. The walk does the insert.
• Stack use is bounded. A Frame is heap-allocated and about 80 bytes, so the path costs O(depth) heap and no thread stack. children borrows from children_of, so there is no extra cloning.
• The tests cover it. filter_block_tree_by_scan keeps the spec-literal recursive version and is compared against the new walk on 100 random trees. It calls the new function with an empty child map to get the leaf verdict, which is a sound way to share that logic. There is also a deep-chain test on a default-stack thread.

Minor points, none blocking:

1. Other recursion over chain depth. The commit message says walk_head and invalidate_subtree are already iterative. Before closing the stall-overflow issue, grep the rest of the head path (get_voting_source, get_checkpoint_block, and any gloas-side walk) for recursion proportional to depth. get_checkpoint_block is called per leaf and should be a parent-pointer loop.
2. Unreachable .expects. The two .expect(...) calls on path can't fire. The first holds because the loop returns as soon as the path empties. The second holds because top is the last element. That is fine, but a short // invariant: comment would save the next reader from re-deriving it.
3. Scope of the PR. The diff includes unrelated files (.cargo/config.toml, .claude/...) because it appears to be based on a whole-repo commit. If that is not intended, rebase so the PR shows only the fork-choice change.

Otherwise this is a clean, behavior-preserving fix for a real abort scenario (about 5,500 blocks of depth on a 2 MiB stack). I'd approve it.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt