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.
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
feat: run StaticFileProvider::check_consistency
on start up
#8143
base: main
Are you sure you want to change the base?
Conversation
loop { | ||
if let Some(indices) = provider.block_body_indices(last_block)? { | ||
if indices.last_tx_num() <= highest_tx { | ||
break | ||
} | ||
} | ||
if last_block == 0 { | ||
break | ||
} | ||
last_block -= 1; | ||
|
||
highest_block = Some(last_block); | ||
update_unwind_target(highest_block); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move this to static file manager, so that we can re-use it in stages as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a first pass, want to also look at unwrap_or_default()
usage
) -> ProviderResult<Option<PipelineTarget>> { | ||
let mut unwind_target: Option<BlockNumber> = None; | ||
let mut update_unwind_target = |new_target: Option<BlockNumber>| { | ||
let new_target = new_target.unwrap_or_default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This unwrap_or_default
seems correct, because we need to unwind to 0
if we pass None
as a new target. I would prefer passing just the BlockNumber
here instead, and do the unwrap_or_default()
only in the call where None
is possible.
let static_file_entry = static_file_entry.unwrap_or_default(); | ||
let highest_static_block = highest_static_block.unwrap_or_default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it correct that if we don't even have a genesis block in the segment, we unwrap these to 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few nits
@@ -138,6 +138,11 @@ impl StaticFileSegment { | |||
pub fn is_headers(&self) -> bool { | |||
matches!(self, StaticFileSegment::Headers) | |||
} | |||
|
|||
/// Returns `true` if the segment is `StaticFileSegment::Receipts`. | |||
pub fn is_receipts(&self) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub fn is_receipts(&self) -> bool { | |
pub const fn is_receipts(&self) -> bool { |
/// A [BlockExecutorProvider] implementation that does nothing. | ||
#[derive(Debug, Default, Clone)] | ||
#[non_exhaustive] | ||
pub struct NoopBlockExecutorProvider; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move this to noop.rs
/// Only used for NoopBlockExecutorProvider | ||
#[error("execution unavailable for noop")] | ||
UnavailableForNoop, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need this, we should use the Other variant instead
#[cfg(any(test, feature = "test-utils"))] | ||
/// Helper function to override block range for testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swap order
#[cfg(any(test, feature = "test-utils"))] | ||
/// Helper function to override block range for testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swap order
{ | ||
/// Returns the [ProviderFactory] for the attached database. | ||
pub fn create_provider_factory(&self) -> eyre::Result<ProviderFactory<DB>> { | ||
pub async fn create_provider_factory(&self) -> eyre::Result<ProviderFactory<DB>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this now needs more docs that describe what this does
has_receipt_pruning: bool, | ||
read_only: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to use a new type for this, in case we need more settings
Introduces a function to check the overall consistency between static files and database:
reth/crates/storage/provider/src/providers/static_file/manager.rs
Lines 476 to 481 in ea0ef7f
This will be called on starting up the node, before any data is queried from storage. If it fails the check, it will return a
PipelineTarget::Unwind(N)
that will be straight away handled by a standalone unwind-only pipeline.The tests live in
crates/stages/mod.rs
because there are other necessary test functions there and can not be imported into providers crate. Follow-up will refactor the test-utils in stages crate and move them to providers crate.Pending live testing - but open to review