Skip to content

feat: add StaticFileSegment::StorageChangeSets#20896

Merged
Rjected merged 4 commits intomainfrom
dan/storage-changesets-static-files
Jan 22, 2026
Merged

feat: add StaticFileSegment::StorageChangeSets#20896
Rjected merged 4 commits intomainfrom
dan/storage-changesets-static-files

Conversation

@Rjected
Copy link
Member

@Rjected Rjected commented Jan 9, 2026

Adds storage changeset static files, which re-use the changeset offsets introduced in #18882

The entries are sorted by address and key rather than just address.

Adds support with the --static-files.storage-change-sets flag

Closes RETH-174

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Suggestion

Consider adding an equivalence test: same query set against MDBX-only vs static-file providers should produce identical results for:

  • storage_changesets_range(from..=to)
  • get_storage_before_block(block, addr, key)

fn storage_changeset_count(&self) -> ProviderResult<usize> {
let mut count = 0;

let static_files = iter_static_files(&self.path).map_err(ProviderError::other)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

there should be a better way rather than read from disk again

tracing::trace!(block_number, "Writing block change");
// sort changes by address.
storage_changes.par_sort_unstable_by_key(|a| a.address);
let mut changeset = Vec::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we call with_capacity here

Copy link
Member Author

Choose a reason for hiding this comment

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

yep now uses with_capacity

Copy link
Collaborator

@joshieDo joshieDo left a comment

Choose a reason for hiding this comment

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

would like to review again once conflicts are in - but it seems to me that we might have some perf impact, because instead of just iterating over the _range, we're collecting it

unsure if there is a better way to handle it tho, or if it's even a significant impact

@github-project-automation github-project-automation bot moved this from Backlog to In Progress in Reth Tracker Jan 20, 2026
@Rjected Rjected force-pushed the dan/storage-changesets-static-files branch 4 times, most recently from dfa7e37 to d22056d Compare January 20, 2026 21:22
Comment on lines +1309 to +1310
if self.cached_storage_settings().storage_changesets_in_static_files {
self.static_file_provider.storage_changesets_range(range)
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think it makes sense,

but we do it differently on account_changesets_range :

fn account_changesets_range(
&self,
range: impl core::ops::RangeBounds<BlockNumber>,
) -> ProviderResult<Vec<(BlockNumber, AccountBeforeTx)>> {
let range = to_range(range);
let mut changesets = Vec::new();
if self.cached_storage_settings().account_changesets_in_static_files &&
let Some(highest) = self
.static_file_provider
.get_highest_static_file_block(StaticFileSegment::AccountChangeSets)
{
let static_end = range.end.min(highest + 1);
if range.start < static_end {
for block in range.start..static_end {
let block_changesets = self.account_block_changeset(block)?;
for changeset in block_changesets {
changesets.push((block, changeset));
}
}
}
} else {
// Fetch from database for blocks not in static files
let mut cursor = self.tx.cursor_read::<tables::AccountChangeSets>()?;
for entry in cursor.walk_range(range)? {
let (block_num, account_before) = entry?;
changesets.push((block_num, account_before));
}
}
Ok(changesets)
}

unsure

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer this way of doing it, I think we should rely on storage settings as the source of truth for where we should be checking, and not have the other checks for highest static files

Copy link
Collaborator

Choose a reason for hiding this comment

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

agree

@Rjected Rjected force-pushed the dan/storage-changesets-static-files branch from d8b739c to 146006b Compare January 21, 2026 20:09
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 21, 2026

CodSpeed Performance Report

Merging this PR will not alter performance

Comparing dan/storage-changesets-static-files (113540b) with main (055bf63)

Summary

✅ 118 untouched benchmarks
⏩ 7 skipped benchmarks1

Footnotes

  1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@Rjected Rjected added this pull request to the merge queue Jan 22, 2026
Merged via the queue into main with commit ebe2ca1 Jan 22, 2026
45 checks passed
@Rjected Rjected deleted the dan/storage-changesets-static-files branch January 22, 2026 15:14
@github-project-automation github-project-automation bot moved this from In Progress to Done in Reth Tracker Jan 22, 2026
yongkangc added a commit that referenced this pull request Jan 27, 2026
…ic files

The storage changeset static file feature (PR #20896) added routing but
forgot to add write_storage_changesets to StateWriteConfig. This caused
storage changesets to be written twice when static files were enabled:
once via write_blocks_data and again via write_state_reverts.

- Add write_storage_changesets field to StateWriteConfig
- Pass sf_ctx.write_storage_changesets to the config in save_blocks
- Guard storage changeset writes in write_state_reverts with the config
- Remove stale workaround comments from RocksDB tests

Amp-Thread-ID: https://ampcode.com/threads/T-019bfe6a-bf5e-7207-851a-b05585d255b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants