refactor(storage): extract shared find_changeset_block_from_index algorithm [1/3] #20542
+80
−36
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Dec 21, 2025
yongkangc
changed the title
yongkangc
requested review from
gakonst,
joshieDo,
rakita and
shekhirin
as code owners
yongkangc
force-pushed
the
yk/pr1-history-info-abstraction
branch
from
6f8341e to
244c864
Compare
yongkangc
changed the title
joshieDo
reviewed
Dec 22, 2025
joshieDo
reviewed
Dec 22, 2025
| } | ||
| if let Some(chunk) = cursor.seek(key)?.filter(|(key, _)| key_filter(key)).map(|x| x.1) { | ||
| // Check if there's a previous shard for the same key | ||
| let has_previous_shard = cursor.prev()?.is_some_and(|(key, _)| key_filter(&key)); |
Collaborator
There was a problem hiding this comment.
// This check is worth it, the
cursor.prev()check is rarely triggered (the if will
// short-circuit) and when it passes we save a full seek into the changeset/plain state
// table.
before, this line was rarely being triggered, but now it will always.
maybe we should make this a closure instead and pass that if we don't want to be passing the cursor?
Member
Author
There was a problem hiding this comment.
ahh before only when the target block is before the first write in the shard that it triggers with &&
good point, there might be a more elegant way to refactor this
before, this line was rarely being triggered, but now it will always.
joshieDo
reviewed
Dec 22, 2025
yongkangc
force-pushed
the
yk/pr1-history-info-abstraction
branch
from
7b5ad36 to
ebb67cf
Compare
Add HistoryInfo enum with documentation and extract the shared rank/select algorithm into history_info_from_shard function. This prepares for RocksDB integration by making the history lookup algorithm reusable across different storage backends (MDBX, RocksDB). Changes: - Add HistoryInfo enum with detailed doc comments - Add history_info_from_shard function with the core algorithm - Refactor history_info method to use the shared function - Export HistoryInfo and history_info_from_shard from providers module
…_index The new name better describes the function's purpose: given a history index shard, find which changeset block contains the historical value for a target block.
yongkangc
force-pushed
the
yk/pr1-history-info-abstraction
branch
4 times, most recently
from
82d60a2 to
43961c9
Compare
Split the history lookup logic: - `needs_prev_shard_check`: pure helper to check if cursor.prev() is needed - `find_changeset_block_from_index`: pure const fn for final decision The cursor.prev() call uses && short-circuit at the call site, so it's only executed in the rare case where rank==0 and found_block != target. Also reorganized file to follow ordering convention: enums -> structs -> functions
yongkangc
force-pushed
the
yk/pr1-history-info-abstraction
branch
from
43961c9 to
03e7abb
Compare
yongkangc
commented
Dec 23, 2025
| // happen if this is the last chunk and so we need to look in the plain state. | ||
| Ok(HistoryInfo::InPlainState) | ||
| } | ||
| let is_before_first_write = |
Member
Author
There was a problem hiding this comment.
@joshieDo instead of going with closure, i extracted to a pure method - thought it was simpler
Member
Author
|
@joshieDo just addressed your comments :) |
yongkangc
commented
Dec 23, 2025
Comment on lines
-194
to
-210
| if let (Some(_), Some(block_number)) = (lowest_available_block_number, block_number) | ||
| { | ||
| // The key may have been written, but due to pruning we may not have changesets | ||
| // and history, so we need to make a changeset lookup. | ||
| Ok(HistoryInfo::InChangeset(block_number)) | ||
| } else { | ||
| // The key is written to, but only after our block. | ||
| Ok(HistoryInfo::NotYetWritten) | ||
| } | ||
| } else if let Some(block_number) = block_number { | ||
| // The chunk contains an entry for a write after our block, return it. | ||
| Ok(HistoryInfo::InChangeset(block_number)) | ||
| } else { | ||
| // The chunk does not contain an entry for a write after our block. This can only | ||
| // happen if this is the last chunk and so we need to look in the plain state. | ||
| Ok(HistoryInfo::InPlainState) | ||
| } |
Member
Author
There was a problem hiding this comment.
this becomes part of find_changeset_block_from_index
yongkangc
commented
Dec 23, 2025
Comment on lines
-190
to
-191
| if rank == 0 && | ||
| block_number != Some(self.block_number) && |
Member
Author
There was a problem hiding this comment.
these 2 logic goes into needs_prev_shard_check imo its easier to read and understand
the part i didnt like about the previous code was that the code has too many conditionals and was hard to hold in my head
joshieDo
requested changes
Dec 23, 2025
| /// should be computed as: `rank == 0 && found_block != Some(block_number) && !has_previous_shard` | ||
| /// where `has_previous_shard` comes from a lazy `cursor.prev()` check. | ||
| /// * `lowest_available` - Lowest block where history is available (pruning boundary) | ||
| pub const fn find_changeset_block_from_index( |
Collaborator
There was a problem hiding this comment.
maybe as impl HistoryInfo?
Collaborator
There was a problem hiding this comment.
HistoryInfo::from_lookup
Member
Author
There was a problem hiding this comment.
Ah yes that's much nicer
| /// Indicates where to find the historical value for a given key at a specific block. | ||
| #[derive(Debug, Eq, PartialEq)] | ||
| pub enum HistoryInfo { | ||
| /// The key is written to, but only after our block (not yet written at the target block). |
Collaborator
There was a problem hiding this comment.
Suggested change
| /// The key is written to, but only after our block (not yet written at the target block). | |
| /// The key is written to, but only after our block (not yet written at the target block). Or it has never been written. |
i think
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Extract the history lookup algorithm into a reusable function that can be shared between MDBX and RocksDB backends.
This is PR 1/3 of a stacked PR series that splits #20412 for easier review. Closes #20388
Review focus
find_changeset_block_from_indexcorrectly captures the original algorithmHistoryInfovariants is accurateChanges
HistoryInfoenum with detailed documentation explaining each variantfind_changeset_block_from_index()function containing the core rank/select algorithmHistoricalStateProviderRef::history_info()to use the shared functionHistoryInfoandfind_changeset_block_from_indexfrom providers moduleStack