Skip to content

Optimise state_root_at_slot for finalized slot #8353

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mergify merged 1 commit into from
Nov 5, 2025
Merged

Optimise state_root_at_slot for finalized slot #8353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions beacon_node/beacon_chain/src/beacon_chain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,6 +883,12 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
return Ok(None);
}

// Fast-path for the split slot (which usually corresponds to the finalized slot).
let split = self.store.get_split_info();
Copy link
Member Author

@michaelsproul michaelsproul Nov 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This introduces a new read of the store.split RwLock in this function, prior to reading the canonical_head RwLock.

The risk here is:

Some other thread is holding the split lock exclusively (i.e. the DB finalization migration is running), and this blocks our read. Counter point: this will happen anyway (and with other downsides) if we miss the fast_lookup on the head state and need to load states from disk.

Maybe we could sequence the split read after the head read, as we mostly care about optimising the case where the finalized_slot is out of range of the head but known to the DB. Conversely, maybe using the split over the head is preferable, as there is more write-contention for the head (the head changes every slot whereas the split only changes at most once per epoch).

I suspect the effect sizes of both these phenomena are small. What we have now seems to be performing very well on Holesky, so I'm inclined to just leave it as-is pending further benchmarking.

Copy link
Collaborator

@dapplion dapplion Nov 4, 2025

Choose a reason for hiding this comment

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

Why not use a try_read here to prevent the deadlock risk?

Copy link
Member Author

@michaelsproul michaelsproul Nov 4, 2025

Choose a reason for hiding this comment

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

There's no deadlock risk. I figure try_read isn't worth it because the alternative is doing state reconstruction. We may as well wait for the split lock, as it will be less work in total

if request_slot == split.slot {
return Ok(Some(split.state_root));
}

// Try an optimized path of reading the root directly from the head state.
let fast_lookup: Option<Hash256> = self.with_head(|head| {
if head.beacon_block.slot() <= request_slot {
Expand Down
4 changes: 4 additions & 0 deletions beacon_node/beacon_chain/tests/store_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3152,6 +3152,10 @@ async fn weak_subjectivity_sync_test(
.get_state(&state_root, Some(slot), CACHE_STATE_IN_TESTS)
.unwrap()
.unwrap();
assert_eq!(
state_root,
beacon_chain.state_root_at_slot(slot).unwrap().unwrap()
);
assert_eq!(state.slot(), slot);
assert_eq!(state.canonical_root().unwrap(), state_root);
}
Expand Down
Loading