Add ASCII fork choice tree to terminal logs

[pablodeymo]

Motivation

This PR adds an ASCII tree directly to terminal logs so developers can monitor fork choice state without leaving the terminal.

Description

Adds a new fork_choice_tree module that renders the fork choice tree as ASCII art and logs it once per slot at interval 4 (end-of-slot attestation acceptance).

Output format

Fork Choice Tree:
  Finalized: slot 38 | root 5678abcd
  Justified: slot 40 | root 1234efgh
  Head:      slot 45 | root abcd1234

  5678abcd(38)── 9abc0123(39)── 1234efgh(40)── a1b2c3d4(41) ─ 2 branches
  ├── dead0001(42)── beef0002(43)── abcd1234(44) *  [w:5]
  └── fade0003(42)── [ ]── cafe0004(44)  [w:2]

Conventions

• root(slot) format using ShortRoot (8 hex chars)
• ── connectors for linear chains
• ├── / └── Unicode tree characters for forks
• * marker on the head block
• [w:N] weight annotation on leaf nodes
• [ ] for single missing slot, [N] for N consecutive missing slots
• Branches sorted by weight (heaviest first), tiebreaker on root hash
• Depth truncation with ... when chain exceeds 20 nodes

Changes

File	Action	Description
crates/blockchain/src/fork_choice_tree.rs	New	format_fork_choice_tree() pure formatter + TreeRenderer struct for recursive rendering
crates/blockchain/src/lib.rs	Modified	Add pub(crate) mod fork_choice_tree declaration
crates/blockchain/src/store.rs	Modified	Add log_tree parameter to update_head/accept_new_attestations; log tree at interval 4 only

Logging frequency

To avoid excessive output, the tree is only printed at interval 4 (end-of-slot), giving one tree per slot. Block imports (interval 0) and on_block calls pass log_tree: false.

No new dependencies

The formatter uses only std::fmt::Write and types already available in the blockchain crate (H256, ShortRoot, Checkpoint).

How to Test

make fmt    # Passes
make lint   # Passes (0 warnings)
make test   # All tests pass (including 9 new unit tests for the formatter)

Unit tests

• Linear chain renders correctly with all nodes in sequence
• Fork with two branches shows ├── / └── connectors and branch count
• Single missing slot shows [ ] indicator
• Multiple missing slots show [N] indicator
• Empty blocks map returns minimal (empty) output
• Head marker * appears on the correct node
• Single-block chain (genesis) renders correctly
• Depth truncation triggers ... at MAX_DISPLAY_DEPTH (20)
• Nested forks (sub-fork within a branch) render both levels of branching

[github-actions]

🤖 Kimi Code Review

Review Summary

This PR introduces a fork choice tree visualization feature. The code is generally well-structured and follows Rust best practices. Here are my findings:

Issues Found

1. Potential Panic in find_tree_root (fork_choice_tree.rs:187-195)

   • Uses .expect("blocks is non-empty") but the function is only called from format_fork_choice_tree which has an early return for empty blocks. This creates a potential panic path if called from elsewhere. Consider returning Option<H256> instead.

2. Inconsistent Head Display (fork_choice_tree.rs:33-39)

   • When blocks.get(&head) returns None, the code displays slot 0. This could be misleading. Consider handling this case more explicitly, perhaps showing "unknown" or returning an error.

3. Unicode Rendering Issues (fork_choice_tree.rs:57, 136, 165)

   • The Unicode box-drawing characters may not render correctly on all terminals. Consider adding a fallback ASCII-only mode for environments that don't support Unicode.

4. Performance Concern in find_tree_root (fork_choice_tree.rs:187-195)

   • This function iterates over all blocks on every call. For large trees, this could be inefficient. Consider caching the root or using a more efficient data structure.

5. Missing Documentation (fork_choice_tree.rs:19-26)

   • The function signature could benefit from documentation explaining the expected format of the blocks parameter (mapping from block root to (slot, parent_root)).

Security Considerations

• No critical security issues identified. The code is read-only and doesn't handle cryptographic operations.

Code Quality Suggestions

1. Extract Magic Numbers (fork_choice_tree.rs:7)

   • Consider making MAX_DISPLAY_DEPTH configurable via parameter rather than a constant.

2. Improve Error Handling (fork_choice_tree.rs:33-39)

   • Instead of .unwrap_or(0) for missing head, consider:

   let head_slot = blocks.get(&head)
       .map(|(slot, _)| *slot)
       .unwrap_or_else(|| {
           warn!("Head block {} not found in blocks map", ShortRoot(&head.0));
           0
       });

3. Test Coverage (fork_choice_tree.rs:200-350)

   • Consider adding tests for edge cases:
   • Deep trees exceeding MAX_DISPLAY_DEPTH
   • Trees with zero-weight branches
   • Trees with duplicate slot numbers

4. Code Duplication (fork_choice_tree.rs:136-165 and 220-250)

   • The branch rendering logic is duplicated between render_trunk and render_branch_line. Consider extracting common patterns.

Minor Issues

• Typo in comment (fork_choice_tree.rs:1): "terminal logging" should probably be "terminal output" or "debug logging"
• Inconsistent naming: ShortRoot vs H256 - consider if this is the clearest naming convention

The implementation is solid overall and follows good Rust practices with proper error handling in most places. The visualization will be very helpful for debugging fork choice issues.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

Summary
This PR adds fork-choice tree logging and a formatter. Consensus logic isn’t changed, but there are a couple of performance/logging correctness issues worth addressing.

Findings

1. Performance: unconditional weight computation and tree rendering even when logs are disabled
   crates/blockchain/src/store.rs:72-90
update_head computes weights and builds the tree whenever log_tree is true, regardless of log level. This can be expensive in large forks and happens every slot end (on_tick interval 4). Suggest guarding with tracing::level_enabled!(Level::INFO) (or move to debug!) to avoid extra compute when not logging.
   Suggested change:

• Add use tracing::Level;
• Wrap block with if log_tree && tracing::level_enabled!(Level::INFO) { ... }

2. Logging correctness: only a single root is rendered
   crates/blockchain/src/fork_choice_tree.rs:169-181
find_tree_root selects the lowest-slot root and renders only that subtree. If get_live_chain() can contain multiple disconnected roots (e.g., due to pruning boundaries or divergent finalized roots), the output will omit other components. For diagnostics this can be misleading. Consider rendering all roots (sorted by slot) or at least indicating when multiple roots exist.
   Suggested change: iterate roots and render each with a header like “Component N”.

3. Header slot fallback can mislead
   crates/blockchain/src/fork_choice_tree.rs:31-44
   If head isn’t present in blocks, the header prints slot 0. That looks like genesis rather than “unknown”. Suggest showing None or ? in that case to avoid misdiagnosis.
   Example: format as Head: slot ? | root ... when missing.

Consensus-layer review
No changes to fork choice, attestation processing, justification/finalization logic, state transition functions, XMSS verification, or SSZ. The additions are logging-only and do not alter consensus outcomes.

If you want next steps

1. I can propose a patch to gate logging on log level and render multiple roots.
2. I can run local tests for the new fork-choice tree formatter.

---

Automated review by OpenAI Codex · custom prompt

[greptile-apps]

Greptile Summary

Adds ASCII fork choice tree visualization logged to terminal at end-of-slot (interval 4), enabling developers to monitor chain state without external tools.

Key changes:

• New fork_choice_tree.rs module with pure formatter function and recursive TreeRenderer for branch rendering
• Module visibility correctly restricted to pub(crate) to avoid API exposure
• Integrates cleanly into existing fork choice flow: update_head computes weights and calls formatter only when log_tree=true
• Logging frequency limited to interval 4 (once per slot) to prevent output spam
• Comprehensive test coverage (9 tests) covering linear chains, forks, missing slots, truncation, nested forks, and head markers
• Uses only std::fmt::Write - no new dependencies

Implementation quality:

• Clean separation of concerns: formatting logic isolated in dedicated module
• All unwrap() calls are safe (writing to String never fails)
• Proper depth limiting prevents unbounded output
• Weight-based branch sorting matches fork choice semantics

Confidence Score: 5/5

• This PR is safe to merge with no blocking issues
• Clean implementation with well-isolated changes, comprehensive test coverage (9 tests), proper error handling (safe unwraps on String writes), appropriate module visibility, and minimal integration surface area (single boolean parameter)
• No files require special attention

Important Files Changed

Filename	Overview
crates/blockchain/src/fork_choice_tree.rs	New ASCII fork choice tree formatter with comprehensive tests, clean implementation using String::fmt::Write, proper depth truncation, and well-structured recursive rendering
crates/blockchain/src/lib.rs	Simple module declaration restricted to pub(crate) visibility as intended
crates/blockchain/src/store.rs	Adds log_tree parameter to control fork choice tree logging, only enabled at interval 4 to avoid excessive output, integrates cleanly with existing fork choice logic

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[on_tick - Interval 4] --> B{log_tree == true?}
    B -->|Yes| C[accept_new_attestations]
    C --> D[update_head]
    D --> E[compute_lmd_ghost_head]
    D --> F[compute_block_weights]
    F --> G[format_fork_choice_tree]
    G --> H[TreeRenderer.render_trunk]
    H --> I{Fork point?}
    I -->|Yes| J[TreeRenderer.render_branches]
    I -->|No| K[render single chain]
    J --> L[render_branch_line for each child]
    L --> M{Sub-fork?}
    M -->|Yes| J
    M -->|No| N[Leaf node with weight]
    G --> O[info! log to terminal]
    
    B -->|No| P[Skip logging]

Loading

_{Last reviewed commit: e5892ae}

[github-actions]

🤖 Claude Code Review

Now I have enough context for a thorough review. Let me compile it.

---

PR #143 Review — ASCII Fork Choice Tree

Summary

Clean addition of a pure ASCII renderer with no new dependencies and a solid test suite. The logic is generally sound. A few correctness and performance issues worth addressing before merging.

---

Bugs

render_trunk never marks the head with *

render_trunk traverses linear segments and simply writes root(slot)── for every node, including the head, without checking current == self.head. The * guard only exists in render_branch_line and at the very tip of the trunk in format_fork_choice_tree.

This means that if the head block sits inside a linear segment whose tip is a deeper descendant — a realistic scenario when a new block extends a chain but doesn't yet have enough weight to become the new head — the * marker is silently dropped.

render_branch_line:203-210 handles the mid-chain case correctly:

1 => {
    if is_head { write!(output, " *").unwrap(); }
    write!(output, "── ").unwrap();
    ...
}

The same guard needs to be added in render_trunk before writing ──.

---

find_tree_root panics / silently drops disconnected subtrees

fork_choice_tree.rs:230-235:

blocks
    .iter()
    .filter(|(_, (_, parent))| parent.is_zero() || !blocks.contains_key(parent))
    .min_by_key(|(_, (slot, _))| *slot)
    .map(|(root, _)| *root)
    .expect("blocks is non-empty")

Two problems:

• If all blocks in the map have their parent also present (a cycle, or a temporarily incomplete window), the filter returns nothing and the .expect panics. In production, panicking inside a logging path is unacceptable.
• min_by_key picks only one root. If the live chain window ever contains multiple disconnected subtrees (e.g., two equally-old roots after equivocation), all but one are silently omitted from the output.

Both are low-probability in normal operation but worth guarding against. Return an Option or fall back gracefully.

---

Performance

compute_block_weights called twice per slot when log_tree = true

fork_choice/src/lib.rs:51 — compute_lmd_ghost_head already calls compute_block_weights internally on every invocation.

store.rs:78-79:

let weights =
    ethlambda_fork_choice::compute_block_weights(finalized.slot, &blocks, &attestations);

This runs again immediately after. For a large live chain, you're walking all attestations twice per slot. Consider exposing the weights alongside the head from compute_lmd_ghost_head, or having the function return them as a pair.

---

Minor Issues

render_gap depth accounting is inconsistent with gap size

fork_choice_tree.rs:239-249: A gap of 1 missing slot and a gap of 100 missing slots both increment depth by 1. Since MAX_DISPLAY_DEPTH is checked against depth, a chain with many skipped slots will truncate later than expected based on visual token count. Not a correctness issue, but could surprise when debugging long chains with skipped slots.

pub mod fork_choice_tree is unnecessarily public

lib.rs:22: The module is only used within the blockchain crate. Declaring it pub leaks the internal formatter as part of the crate's public API. pub(crate) is sufficient.

---

Test Gaps

The unit tests are well-structured and cover the documented cases. Two scenarios worth adding:

• Head in a non-tip linear position: a chain where head has exactly one child (so it's traversed by render_trunk, not at the tip). This would catch the render_trunk head-marker bug above.
• MAX_DISPLAY_DEPTH truncation: verify the ... indicator appears and no panic occurs.

---

What's Good

• format_fork_choice_tree is a pure function with no side effects — easy to test and reason about.
• TreeRenderer struct avoids threading many arguments through recursive calls cleanly.
• Branch sorting by weight (heaviest first) gives the most relevant view at a glance.
• render_gap correctly reads parent slot from the parent's entry in blocks rather than guessing, so gap detection is accurate across branch boundaries.
• log_tree: bool propagation is minimal and contained — the parameter addition to update_head/accept_new_attestations doesn't touch any non-logging logic paths.
• No new dependencies.

---

Automated review by Claude (Anthropic) · custom prompt

[pablodeymo]

@greptile-apps review this PR again