Skip to content

refactor(blockchain): remove log_tree param from accept_new_attestations#536

Open
Aliemeka wants to merge 1 commit into
lambdaclass:mainfrom
Aliemeka:refactor/remove-log-tree-param
Open

refactor(blockchain): remove log_tree param from accept_new_attestations#536
Aliemeka wants to merge 1 commit into
lambdaclass:mainfrom
Aliemeka:refactor/remove-log-tree-param

Conversation

@Aliemeka

@Aliemeka Aliemeka commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

🗒️ Description / Motivation

  • Removes the log_tree boolean parameter from accept_new_attestations (and update_head), which threaded a UI concern through the fork-choice code path.
  • crates/blockchain/src/store.rs is meant to mirror the spec's architecture, and the log_tree flag was inconsistent with that goal — logging is a caller-side concern, not part of the fork-choice update.
  • Moves the fork choice tree logging to the call site (on_tick, end-of-slot) so the update functions stay purely about fork-choice computation.

What Changed

  • crates/blockchain/src/store.rs
    • Dropped the log_tree: bool parameter from accept_new_attestations and update_head.
    • Added a standalone log_fork_choice_tree(store, &HeadUpdate) helper that renders the ASCII tree; called from on_tick at SlotInterval::EndOfSlot.
    • update_head now returns a HeadUpdate { blocks, weights, head } so the tree renders from already-computed data instead of recomputing LMD GHOST.
    • Updated the (previously stale) update_head doc comment.
  • crates/blockchain/src/spec_test_runner.rs
    • Updated the update_head call to the new no-bool signature.

Correctness / Behavior Guarantees

  • No behavior change. The tree is still logged only at end-of-slot, and log_fork_choice_tree still reads latest_justified/latest_finalized after the checkpoint update — so the rendered tree is byte-for-byte equivalent to before.
  • Efficiency improvement: end-of-slot previously computed LMD GHOST twice (once in update_head, once for the tree). It now computes once and reuses the result via HeadUpdate, eliminating a redundant get_live_chain() + compute_lmd_ghost_head() per slot.
  • All other call sites (BlockPublication, on_block_core, get_proposal_head) simply drop the false argument — no functional difference.

Tests Added / Run

  • No new tests — this is a behavior-preserving refactor covered by existing fork-choice spec tests.
  • Ran:
    • cargo clippy -p ethlambda-blockchain --all-targets -- -D warnings — clean
    • cargo fmt --all --check — clean
    • cargo test -p ethlambda-blockchain --test forkchoice_spectests — compiled and ran; requires make leanSpec/fixtures locally to execute assertions.

Related Issues / PRs

✅ Verification Checklist

  • Ran make fmt — clean
  • Ran make lint (clippy with -D warnings) — clean
  • Ran cargo test --workspace --release

@greptile-apps

greptile-apps Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR moves fork-choice tree logging out of the head update functions. The main changes are:

  • Returns computed blocks, weights, and head in a new HeadUpdate value.
  • Logs the tree from the end-of-slot caller without recomputing LMD GHOST.
  • Updates callers to use the simplified function signatures.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.
  • The head is still committed before HeadUpdate is returned.
  • End-of-slot logging uses the same computed data and checkpoint ordering as before.

Important Files Changed

Filename Overview
crates/blockchain/src/store.rs Moves tree logging to end-of-slot processing and returns the existing fork-choice computation through HeadUpdate.
crates/blockchain/src/spec_test_runner.rs Updates the test runner for the simplified update_head signature.

Reviews (1): Last reviewed commit: "refactor(blockchain): remove log_tree pa..." | Re-trigger Greptile

@MegaRedHand MegaRedHand left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove the log_tree parameter on accept_new_attestations

2 participants