Skip to content

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

Merged
MegaRedHand merged 1 commit into
lambdaclass:mainfrom
Aliemeka:refactor/remove-log-tree-param
Jul 23, 2026
Merged

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

Conversation

@Aliemeka

@Aliemeka Aliemeka commented Jul 22, 2026 •
edited
Loading

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 approved these changes Jul 22, 2026
View reviewed changes

@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

@MegaRedHand
MegaRedHand merged commit cdbca48 into lambdaclass:main Jul 23, 2026
7 checks passed
@MegaRedHand MegaRedHand mentioned this pull request Jul 31, 2026
Merged
MegaRedHand added a commit that referenced this pull request Jul 31, 2026
@MegaRedHand
fix(state-transition): bound distinct attestation data in the transition
29cc787 
The per-block cap on distinct AttestationData is a transition rule in
leanSpec (`process_attestations`), and `fork_choice.on_block` says so
explicitly: "The transition itself bounds the distinct-data count. Only the
wire-level duplicate prohibition lives here."

We enforced it only at the import boundary in `on_block`, so
`state_transition()` accepted an over-cap block and then failed on the state
root instead. Both block production (`build_block` -> `process_block`) and
spec-fixture replay call the transition without going through `on_block`, so
neither was bounded.

Check it at the top of `process_attestations`, ahead of the
justification-bookkeeping guards as the spec does. The `on_block` check stays
for now: it runs before signature verification, so an over-cap block is still
rejected without paying for proof verification.

(leanSpec #536)
MegaRedHand added a commit that referenced this pull request Aug 3, 2026
@MegaRedHand
fix(state-transition): bound distinct attestation data in the transition
63844a3 
The per-block cap on distinct AttestationData is a transition rule in
leanSpec (`process_attestations`), and `fork_choice.on_block` says so
explicitly: "The transition itself bounds the distinct-data count. Only the
wire-level duplicate prohibition lives here."

We enforced it only at the import boundary in `on_block`, so
`state_transition()` accepted an over-cap block and then failed on the state
root instead. Both block production (`build_block` -> `process_block`) and
spec-fixture replay call the transition without going through `on_block`, so
neither was bounded.

Check it at the top of `process_attestations`, ahead of the
justification-bookkeeping guards as the spec does. The `on_block` check stays
for now: it runs before signature verification, so an over-cap block is still
rejected without paying for proof verification.

(leanSpec #536)
MegaRedHand added a commit that referenced this pull request Aug 3, 2026
@MegaRedHand
fix(state-transition): bound distinct attestation data in the transit…
1236a44 
…ion (#555)

## What

Enforce the per-block cap on distinct `AttestationData` inside
`process_attestations`, where leanSpec has it, in addition to the
existing check at the import boundary in `on_block`.

## Why

leanSpec puts the bound in the transition
(`state_transition.process_attestations`) and `fork_choice.on_block`
defers to it explicitly:

> The transition itself bounds the distinct-data count. Only the
wire-level duplicate prohibition lives here.

We only had it in `on_block` (`store.rs`), so `state_transition()`
accepted an over-cap block and then failed on the state root instead.
Two callers reach the transition without passing through `on_block`:

| caller | before | after |
|---|---|---|
| `build_block` -> `process_block` | unbounded (the proposer-side clamp
is the only guard) | fails loudly instead of publishing an unimportable
block |
| spec-fixture replay / Hive `state_transition/run` | over-cap block
accepted, then `STATE_ROOT_MISMATCH` | rejected with the cap error |

The check goes first in `process_attestations`, ahead of the
justification-bookkeeping guards, matching the spec's order when a block
violates two rules at once.

## The duplicate check in `on_block` stays

Deliberately, for now: it runs before `verify_block_signatures`, so an
over-cap block is still rejected without paying for proof verification.
Whether we collapse the two sites into one is a follow-up decision.

## Testing

- `cargo test --workspace --profile release-fast`: green, no new tests
added here.
- The behavior is covered by the existing fixture
`test_block_exceeding_distinct_attestation_data_cap_rejects_block`,
which asserts failure but not yet the reason. Cross-checked against
#547, which adds the reason assertion to the fixture runners: with both
branches applied, that fixture passes for the right reason
(`TOO_MANY_ATTESTATION_DATA`, previously `STATE_ROOT_MISMATCH`).

Whichever of the two lands first, the other's exhaustive `From<&Error>
for RejectionReason` match makes the missing mapping arm a compile
error, so the reason cannot be silently dropped.

(leanSpec #536)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MegaRedHand MegaRedHand MegaRedHand approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove the log_tree parameter on accept_new_attestations

2 participants

@Aliemeka @MegaRedHand