Skip to content

Commit cdbca48

Browse files
authored
refactor(blockchain): remove log_tree param from accept_new_attestations (lambdaclass#536)
## 🗒️ 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 - Closes lambdaclass#520 ## ✅ Verification Checklist - [x] Ran `make fmt` — clean - [x] Ran `make lint` (clippy with `-D warnings`) — clean - [x] Ran `cargo test --workspace --release`
1 parent 15d40a4 commit cdbca48

2 files changed

Lines changed: 41 additions & 24 deletions

File tree

crates/blockchain/src/spec_test_runner.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ pub fn apply_fork_choice_step(
6161
)
6262
});
6363
store.insert_known_aggregated_payloads_batch(entries.collect());
64-
store::update_head(store, false);
64+
store::update_head(store);
6565
Ok(())
6666
}
6767
"attestation" => {

crates/blockchain/src/store.rs

Lines changed: 40 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use std::collections::HashSet;
1+
use std::collections::{HashMap, HashSet};
22

33
use ethlambda_state_transition::{is_proposer, slot_is_justifiable_after};
44
use ethlambda_storage::{ForkCheckpoints, Store};
@@ -25,19 +25,44 @@ use crate::{
2525

2626
const JUSTIFICATION_LOOKBACK_SLOTS: u64 = 3;
2727

28+
/// Intermediate fork-choice data produced by [`update_head`], carried so the
29+
/// fork choice tree can be rendered without recomputing LMD GHOST.
30+
pub struct HeadUpdate {
31+
blocks: HashMap<H256, (u64, H256)>,
32+
weights: HashMap<H256, u64>,
33+
head: H256,
34+
}
35+
36+
/// Log an ASCII fork choice tree to the terminal, reusing the weights and
37+
/// block set already computed by [`update_head`].
38+
fn log_fork_choice_tree(store: &Store, update: &HeadUpdate) {
39+
let tree = crate::fork_choice_tree::format_fork_choice_tree(
40+
&update.blocks,
41+
&update.weights,
42+
update.head,
43+
store
44+
.latest_justified()
45+
.expect("latest justified checkpoint exists"),
46+
store
47+
.latest_finalized()
48+
.expect("latest finalized checkpoint exists"),
49+
);
50+
info!("\n{tree}");
51+
}
52+
2853
/// Accept new aggregated payloads, promoting them to known for fork choice.
29-
fn accept_new_attestations(store: &mut Store, log_tree: bool) {
54+
fn accept_new_attestations(store: &mut Store) -> HeadUpdate {
3055
store.promote_new_aggregated_payloads();
3156
metrics::update_latest_new_aggregated_payloads(store.new_aggregated_payloads_count());
3257
metrics::update_latest_known_aggregated_payloads(store.known_aggregated_payloads_count());
33-
update_head(store, log_tree);
58+
update_head(store)
3459
}
3560

3661
/// Update the head based on the fork choice rule.
3762
///
38-
/// When `log_tree` is true, also computes block weights and logs an ASCII
39-
/// fork choice tree to the terminal.
40-
pub fn update_head(store: &mut Store, log_tree: bool) {
63+
/// Returns the block set, block weights, and new head computed during the
64+
/// update so callers can render the fork choice tree without recomputing it.
65+
pub fn update_head(store: &mut Store) -> HeadUpdate {
4166
let blocks = store
4267
.get_live_chain()
4368
.expect("get_live_chain should succeed");
@@ -104,19 +129,10 @@ pub fn update_head(store: &mut Store, log_tree: bool) {
104129
);
105130
}
106131

107-
if log_tree {
108-
let tree = crate::fork_choice_tree::format_fork_choice_tree(
109-
&blocks,
110-
&weights,
111-
new_head,
112-
store
113-
.latest_justified()
114-
.expect("latest justified checkpoint exists"),
115-
store
116-
.latest_finalized()
117-
.expect("latest finalized checkpoint exists"),
118-
);
119-
info!("\n{tree}");
132+
HeadUpdate {
133+
blocks,
134+
weights,
135+
head: new_head,
120136
}
121137
}
122138

@@ -350,7 +366,7 @@ pub fn on_tick(store: &mut Store, timestamp_ms: u64, has_proposal: bool) {
350366
SlotInterval::BlockPublication => {
351367
// Start of slot - process attestations if proposal exists
352368
if should_signal_proposal {
353-
accept_new_attestations(store, false);
369+
accept_new_attestations(store);
354370
}
355371
}
356372
SlotInterval::AttestationProduction => {
@@ -365,7 +381,8 @@ pub fn on_tick(store: &mut Store, timestamp_ms: u64, has_proposal: bool) {
365381
}
366382
SlotInterval::EndOfSlot => {
367383
// End of slot - accept accumulated attestations and log tree
368-
accept_new_attestations(store, true);
384+
let update = accept_new_attestations(store);
385+
log_fork_choice_tree(store, &update);
369386
}
370387
}
371388
}
@@ -690,7 +707,7 @@ fn on_block_core(
690707
// `lean_state_transition_attestations_processed_total` instead.
691708

692709
// Update forkchoice head based on new block and attestations
693-
update_head(store, false);
710+
update_head(store);
694711

695712
let block_total = block_start.elapsed();
696713
info!(
@@ -872,7 +889,7 @@ fn get_proposal_head(store: &mut Store, slot: u64) -> H256 {
872889
on_tick(store, slot_time_ms, true);
873890

874891
// Process any pending attestations before proposal
875-
accept_new_attestations(store, false);
892+
accept_new_attestations(store);
876893

877894
store.head().expect("store head exists")
878895
}

0 commit comments

Comments
 (0)