feat(snapshots): write Headers to snapshots during HeaderStage
#6273
Conversation
There was a problem hiding this comment.
I think this is fairly readable, and I was able to follow along, tho I didn't review all checks in detail
pending @shekhirin
|
|
||
| debug!(target: "reth::cli", chain=%self.chain.chain, genesis=?self.chain.genesis_hash(), "Initializing genesis"); | ||
|
|
||
| init_genesis(db.clone(), self.chain.clone())?; | ||
| init_genesis(provider_factory.clone())?; |
crates/node-core/src/init.rs
Outdated
| return Ok(hash) | ||
| } | ||
| // Check if we already have the genesis header or if we have the wrong one. | ||
| match factory.sealed_header(0) { |
There was a problem hiding this comment.
can we use factory.block_hash() here? should be easier because we just need the hash
There was a problem hiding this comment.
hm, im not sure i understand your question. here we check if what's in static files exists OR if it's the right one
There was a problem hiding this comment.
we use only hash from the retrieved header, can we use factory.block_hash(0) instead, and match on Ok(Some(hash)) => { ... }? saves us retrieving the actual header
crates/node-core/src/init.rs
Outdated
| match snapshot_provider.header_by_number(0) { | ||
| Ok(None) | Err(ProviderError::MissingSnapshotBlock(SnapshotSegment::Headers, 0)) => { | ||
| let (difficulty, hash) = (header.difficulty, header.hash); | ||
| let mut writer = snapshot_provider.latest_writer(SnapshotSegment::Headers)?; | ||
| writer.append_header(header.header, difficulty, hash)?; | ||
| writer.commit()?; | ||
| } | ||
| Ok(Some(_)) => {} | ||
| Err(e) => return Err(e), | ||
| } | ||
|
|
There was a problem hiding this comment.
we're sure at this point that no header for block 0 is written just as before, right? so we can just write to snapshot using the provider
There was a problem hiding this comment.
not quite, the stage drop command calls this function on a drop bodystage for example
| HeaderSyncMode::Continuous => SyncTarget::TipNum(head_num + 1), | ||
| }, | ||
| _ => return Err(ProviderError::InconsistentHeaderGap.into()), | ||
| let target = match mode { |
| /// provider. | ||
| /// * `fetch_from_database` - A closure that defines how to fetch the data from the database | ||
| /// when the snapshot doesn't contain the required data or is not available. | ||
| pub fn get_with_snapshot_or_database<T, FS, FD>( |
There was a problem hiding this comment.
this a duplicate from another function which was part of DatabaseProvider, will dedup on a follow-up
| /// * `predicate` - A function used to evaluate each item in the fetched data. Fetching is | ||
| /// terminated when this function returns false, thereby filtering the data based on the | ||
| /// provided condition. | ||
| pub fn get_range_with_snapshot_or_database<T, P, FS, FD>( |
There was a problem hiding this comment.
this a duplicate from another function which was part of DatabaseProvider, will dedup on a follow-up
|
|
||
| let gap = provider.sync_gap(mode.clone(), checkpoint).unwrap(); | ||
| assert_eq!(gap.local_head, head); | ||
| assert_eq!(gap.target.tip(), consensus_tip.into()); | ||
|
|
There was a problem hiding this comment.
with static files, we no longer push to storage in reverse order, so this scenario no longer holds
|
rfr, state tests are failing because they're getting canceled , they pass locally |
joshieDo
requested review from
rakita,
rkrasiuk,
Rjected and
onbjerg
as code owners
|
|
||
| if hashes.len() < num_hashes { | ||
| let snapshot_provider = | ||
| self.provider_factory.snapshot_provider().expect("should exist"); |
There was a problem hiding this comment.
better to return the result so there is more context if this fails?
There was a problem hiding this comment.
being removed in another PR: #6344 it will no longer be an Option<_>
| let snapshot_provider = | ||
| self.provider_factory.snapshot_provider().expect("should exist"); | ||
| let total_headers = snapshot_provider.count_entries::<tables::Headers>()? as u64; | ||
| if total_headers > 0 { |
There was a problem hiding this comment.
hmm this is ok for now, i'd add a comment on what this is doing, but ideally the switch between fetching from db vs snapshots would be a bit more transparent 😬
| self.executor_factory.with_state(LatestStateProviderRef::new(provider.tx_ref())); | ||
| let mut executor = self.executor_factory.with_state(LatestStateProviderRef::new( | ||
| provider.tx_ref(), | ||
| provider.snapshot_provider().expect("should exist").clone(), |
There was a problem hiding this comment.
same as above, rly not a big fan of us doing expect in a stage
There was a problem hiding this comment.
being removed in another PR: #6344 it will no longer be an Option<_>
|
|
||
| for entry in provider.fetch_range_iter( | ||
| SnapshotSegment::Headers, | ||
| *range.start()..*range.end() + 1, |
There was a problem hiding this comment.
should this take a RangeInclusive instead? big opportunity for off by one errors
Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>
…lock range on Headers
This reverts commit f6a8b5e.
Writes
Header,HeaderTDandHeaderHashto static files during theHeaderStage.Some necessary changes that might require a more careful look at
ProviderFactoryequipped with a snapshot providersync_gap()and the header downloader changes.Tested holesky + sepolia ✔️