[pablodeymo]

🗒️ Description / Motivation

filter_block_tree found a block's children by scanning the whole block index
(Store::block_index, the full LiveChain window since the finalized block) at
every node it visited. The recursion visits every node of the justified subtree,
so the head walk was quadratic in the number of unfinalized blocks. That runs on
every block import and every tick through get_head, and the cost grows exactly
when the node is under pressure: a finality stall or a range catch-up from a
checkpoint anchor.

walk_head and invalidate_subtree already build a parent-to-children map once
per call; this brings the filter in line with them. Found by Kimi during an
improvement review.

What Changed

• crates/blockchain/state_transition/src/beacon/fork_choice.rs
  • get_filtered_block_tree builds HashMap<Root, Vec<Root>> from the index in
    one pass and hands it down.
  • filter_block_tree takes that map and does one lookup per node instead of a
    scan per node.
  • Rustdoc that described the per-node scan is updated (the module's "never needs
    a BTreeMap" section, the filter_block_tree doc, one cross-reference in
    compute_head).

compute_head and get_node_children keep their per-level scans on purpose:
they are the spec-literal references the tests compare the walk against.

Correctness / Behavior Guarantees

• The filtered tree is identical: child order only ORs viability and inserts
  into a map, so it cannot change the result.
• No storage change, no change to any signature the node or the spec harness
  calls (filter_block_tree is pub but has no callers outside this file).
• Cost goes from O(N × M) to O(N), N the live window and M the justified subtree.

Tests Added / Run

• New: the_filtered_tree_matches_the_spec_literal_children_scan_on_random_trees
  keeps the old scan as a reference and asserts the map-based result equals it on
  100 random trees, including slots where a branch is filtered out (asserts that
  happened at least once, so it cannot pass vacuously).
• Existing: the randomized differential test and the fork-choice fixture suite
  already cover get_filtered_block_tree.
• Ran: make fmt, make lint, fork_choice unit tests (94 passed), mainnet
  fork-choice fixture cases (206 passed), make test-consensus (1324 passed).

Related Issues / PRs

• None

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• Ran make test — test-consensus run locally; test-node left to CI
  (unrelated to this change)

[github-actions]

🤖 Claude Code Review

Review of PR 746: build the parent-to-children map once in get_filtered_block_tree

Overall: The change is correct, small and well scoped. I found no blocking issues. I read the diff but did not build it or run the tests.

Correctness

• get_filtered_block_tree builds children_of from index in one pass. filter_block_tree then does one HashMap::get per node where it used to scan the whole index. The leaf and non-leaf branches are unchanged. Child order only feeds an OR of viability and map inserts, so it cannot change the result, as the PR says.
• Using unwrap_or(&[]) for a missing key keeps the leaf-detection behaviour of the old empty scan.
• Recursion depth is the same as before. The map-building loop adds no new failure mode.
• A block whose parent is not in index, such as the finalized anchor, ends up as a key nobody visits. That is harmless.

Performance

• This takes the walk from O(N×M) to O(N), and it matches what walk_head and invalidate_subtree already do.
• The map is built over the whole live window, not just the justified subtree. That cost is linear and a single pass, so it is fine.
• Optional: if walk_head also runs from get_head, both could share one map per call. That would save a second pass and some allocation. It is not needed for this PR.

Tests

• The new test keeps the scan-based version as a reference and compares it with the map-based result on 100 random trees. It runs at two current slots. It also asserts that at least one branch was filtered out, so it cannot pass vacuously. That is good coverage.
• In the reference, the leaf case delegates to filter_block_tree with an empty map. This is reasonable and the comment explains it. It does mean the leaf viability logic is not independently checked, but the existing fixture suite covers it.
• The reference's any_viable insert matches the production non-leaf path as shown in the diff. I did not check the part of filter_block_tree outside the diff.

Docs and nits

• The rustdoc updates are consistent with the code. The module-level "never needs a BTreeMap" section now correctly lists the three map builds plus compute_head's scan.
• The filter_block_tree doc says the map is "built from index in one pass", which is accurate.
• filter_block_tree is pub with a changed signature. The PR states it has no callers outside this file, which is fine. Consider reducing it to pub(crate) or private in a follow-up.

Verdict: LGTM. Per the PR checklist, test-node is left to CI. That is fine, since the change is confined to this file.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which optimizes filter_block_tree from O(n²) to O(n) by pre-building a parent-to-children map. Let me analyze the changes carefully.

Overall Assessment

This is a solid performance optimization with good test coverage. The core idea—pre-building children_of in one pass—is correct and well-motivated. However, I've identified several issues ranging from minor to moderately serious.

---

Issues Found

1. Critical: Potential Stack Overflow in Deep Trees (fork_choice.rs, ~line 1908-1910)

for &child in children {
    if filter_block_tree(store, index, children_of, child, blocks, config)? {

The recursive filter_block_tree call uses the stack for each child traversal. With Ethereum's 12-second slots and potential long periods without finalization, trees can grow very deep (hundreds or thousands of blocks). This could overflow the stack.

Recommendation: Consider adding #[inline(never)] to limit stack frame growth, or better, document this risk and consider an iterative rewrite if deep finalization stalls are expected in production.

---

2. Bug: children_of Map Can Contain Duplicate Entries on Re-orgs (fork_choice.rs, ~line 1962-1965)

for (&root, &(_, parent_root)) in index {
    children_of.entry(parent_root).or_default().push(root);
}

This builds children_of from index, but index is HashMap<Root, (Slot, Root)>. If the same block root appears multiple times with different slots (which shouldn't happen but isn't enforced here), or if index contains stale entries from re-orgs where a block was later orphaned, the children_of map will accumulate all historical children.

Question: Does Store::block_index() guarantee it only contains the canonical chain and current forks? If it contains pruned or orphaned blocks that still pass the index filter, children_of will be larger than necessary and filter_block_tree will traverse stale branches.

Recommendation: Verify block_index's pruning semantics are documented. If stale entries exist, this is pre-existing behavior (the scan would also see them), but worth a comment.

---

3. Performance: HashMap Lookup per Child in Tight Loop (fork_choice.rs, ~line 1908)

for &child in children {
    if filter_block_tree(store, index, children_of, child, blocks, config)? {

Each recursive call does multiple HashMap lookups: index.get(&block_root), children_of.get(&block_root), and within the leaf branch, store.block_states.get(&block_root). For large trees, consider whether index and children_of could use FxHashMap or a faster hasher, since these are internal-only maps with no DoS risk from untrusted keys (roots are hashes).

Recommendation: Benchmark with rustc_hash::FxHashMap for children_of and blocks. This is a common optimization in consensus clients.

---

4. Code Clarity: no_children Empty Map in Test is Hacky (fork_choice.rs, ~line 9955-9957)

let no_children = HashMap::new();
return filter_block_tree(store, index, &no_children, block_root, blocks, config);

This test helper forces the leaf branch by passing an empty children_of. It's clever but fragile—if filter_block_tree's leaf logic ever changes to depend on children_of for something else, this breaks.

Recommendation: Add a comment explaining this coupling, or better, extract the leaf viability check to a named function that both filter_block_tree and the test can call directly.

---

5. Test Logic Issue: filter_block_tree_by_scan Doesn't Match Spec (fork_choice.rs, ~line 9933-9982)

The test claims to match "the specification's filter_block_tree" but the spec (consensus-specs) finds children by scanning store.blocks, not index. The test scans index instead:

let children: Vec<Root> = index
    .iter()
    .filter(|&(_, &(_, parent_root))| parent_root == block_root)

This is the old node code's scan target, not the spec's. The comment at line 9933 says "the specification's filter_block_tree finds a block's children by scanning every block in the store" but then implements it against index.

However: Looking more carefully, index is derived from store.block_index(), which the module docs say is "the store's blocks." So this may be equivalent in practice, but the test's claim to be "the reference" is slightly overstated.

Recommendation: Clarify the comment: this tests equivalence between the old O(n²) node implementation and the new O(n) one, not against the Python spec directly.

---

6. Minor: Typo in Test Variable Name (fork_choice.rs, ~line 9970)

current_slots.push(tree.first_gloas_slot + 3 * preset::SLOTS_PER_EPOCH);

first_gloas_slot → likely first_goal_slot or first_ghost_slot? This appears to be pre-existing, not introduced by this PR, but worth noting.

---

7. Documentation Inconsistency (fork_choice.rs, ~line 49-58)

The updated module doc says:

"The places this file iterates every block the store holds are the one-pass parent-to-children map builds..."

But compute_head (mentioned at line 1956) doesn't exist in the shown diff context—the function is get_head or walk_head. Verify the referenced function names match the actual codebase.

---

Positive Acknowledgments

1. The optimization is correct and well-bounded. Pre-building children_of is a standard and safe transformation from scan-per-node to lookup-per-node.

2. The test is thorough. 100 random seeds with both full-tree and partial-filter scenarios, plus the assertion that branches were actually filtered in some cases, gives good confidence.

3. The API change is minimal. Adding one parameter to filter_block_tree keeps the change localized.

4. The unwrap_or(&[]) pattern (line 1903-1905) for missing children is idiomatic and avoids allocation.

---

Summary

Severity	Issue	Line
Medium	Stack overflow risk in deep recursion	~1908
Medium	children_of may include stale/orphaned entries	~1962
Low	HashMap hasher choice for hot path	~1908
Low	Test "reference" claim slightly overstated	~9933
Trivial	Typo in variable name	~9970
Trivial	Doc function name consistency	~1956

The PR is correct and should ship after addressing or documenting items 1 and 2.

---

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

[github-actions]

🤖 Codex Code Review

Looks good overall. This is a targeted fork-choice performance fix, and the new regression test meaningfully checks semantic equivalence against the literal spec-style scan.

• No correctness bug stands out in the changed logic at crates/blockchain/state_transition/src/beacon/fork_choice.rs:1892. Replacing the per-node index.iter() child scan with a prebuilt children_of map preserves behavior because viability still depends only on recursively discovered viable descendants or the unchanged leaf checks.
• The change at crates/blockchain/state_transition/src/beacon/fork_choice.rs:1958 is a real performance win: it reduces filter_block_tree from repeated whole-index scans to a single index pass plus O(1) child lookups, which matters during long unfinalized windows and catch-up.
• I don’t see a new security risk. The new map is derived entirely from trusted in-memory fork-choice state, does not alter signature/SSZ/state-transition paths, and does not weaken validation.
• The test at crates/blockchain/state_transition/src/beacon/fork_choice.rs:9938 is well chosen: it uses the old scan behavior as a reference and compares full filtered-tree outputs across randomized trees, which is exactly the right guard for fork-choice refactors.

Minor note:

• children_of stores Vec<Root> in HashMap insertion order, which is nondeterministic across HashMap iteration, but this function only tests existence/viability of descendants and inserts into a HashMap, so output semantics remain order-independent here. I don’t think this needs a change, just calling out why it is safe.

Validation note:

• I couldn’t run the Rust test locally because rustup failed to write under a read-only home directory in this environment.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[MegaRedHand]

Block import benchmark (offline replay of mainnet blocks, steady state)

leg	STF per block	get_head per block
main 012dc4f	157.7 ms	16.7 ms
main + #746	158.0 ms	16.7 ms (difference -0.0 ms, 95% CI [-0.1, +0.0])

No change at this size, as expected: the replay's tree never holds more than about 90 blocks (one warm-up epoch plus 58 measured), and a healthy mainnet node's is similar (finality 2 to 3 epochs back), so the old per-node scan cost tens of µs.

Synthetic trees (finality stall)

A temporary #[ignore] test on main + #746 builds fulu trees with nothing justified or finalized past the anchor, and times get_filtered_block_tree (this PR) against the test module's filter_block_tree_by_scan (main's per-node index scan), checking both return the same tree. Medians on ethlambda-5 (Ryzen 7 9700X); two runs agreed within 3%.

Single chain:

blocks	stall length	main (scan)	#746 (map)	whole get_head_node with #746
97	3 epochs	0.015 ms	0.016 ms	0.06 ms
1,001	~3.3 h	0.77 ms	0.18 ms	0.70 ms
3,201	~11 h	5.8 ms	0.57 ms	2.3 ms
7,201	~24 h	37 ms	1.7 ms	6.5 ms
14,401	~48 h	200 ms	3.7 ms	14 ms

With a one-block orphan every 32 blocks:

blocks	leaves	main (scan)	#746 (map)
1,032	32	1.0 ms	0.46 ms
3,301	101	8.8 ms	3.4 ms
7,426	226	55 ms	17 ms
14,851	451	287 ms	74 ms

• get_head runs on every import and every tick, so a two-day stall costs main about 200 ms more per call than this PR.
• What stays quadratic after this PR is per leaf: get_checkpoint_block walks each leaf back to the finalized epoch, so the forked trees still grow as leaves × depth (74 ms at 14,851 blocks).
• The synthetic store has 12 validators, so the get_head_node column holds almost no vote weighting; on mainnet that part is most of the 16.7 ms above.
• The test ran with a 64 MiB stack. filter_block_tree recurses once per block of depth, before and after this PR, and on the default 2 MiB thread stack (what std::thread::spawn gives the chain actor's thread) it overflowed between 5,500 and 6,000 blocks under the release-fast profile: roughly 18 to 20 hours of a stall in which nothing new is justified.

Method

• Offline ethlambda benchmark import replay on ethlambda-5 (Ryzen 7 9700X), with the node stopped.
• 3 consecutive mainnet windows fetched from that node: 173 measured blocks (slots 15567200 to 15567385), each window after one unmeasured warm-up epoch. --block-delay 1000.
• One leg per PR: the PR head merged onto main 012dc4f. The BLS PRs (perf(crypto): sum validated public keys through blst's batched affine addition #737, perf(beacon): verify a block's signatures in one random-weight batch #738, perf(beacon): verify attestations against committee key sums #742, perf(crypto): resolve registry public keys through a validator-index table #743) are stacked, so each of those legs includes its parents. --warm-committee-sums on the legs that have the flag (perf(beacon): verify attestations against committee key sums #742, perf(crypto): resolve registry public keys through a validator-index table #743, nightly).
• 2 rounds over all legs, leg order rotated per window. Every run of a window imported the same block roots.
• Numbers are per-block STF means, paired against the reference leg on the same blocks, with a bootstrap 95% CI of the mean difference.
• Steady state: 9 of the 173 blocks are dropped from every leg, because their cost exists only in a fresh process with no gossip. They are the first measured block of each window (about 31k attesters whose keys the process has never validated) and, per epoch, the first block carrying that epoch's attestations (it builds the committee shuffling inside the timed span; a live node's gossip validation builds it first). Including them makes the legs with --warm-committee-sums look about 3 points better than they are, since the flag does both before the timer starts.
• Noise floor: legs whose code the replay cannot exercise move about ±1.5% between rounds.