chore(trie): Use trie changesets for engine unwinding#18878
Merged
mediocregopher merged 8 commits into18460-trie-changesetsfrom Oct 7, 2025
Merged
chore(trie): Use trie changesets for engine unwinding#18878mediocregopher merged 8 commits into18460-trie-changesetsfrom
mediocregopher merged 8 commits into18460-trie-changesetsfrom
Conversation
bdaeb41 to
70d24d4
Compare
mediocregopher
commented
Oct 6, 2025
Rjected
approved these changes
Oct 6, 2025
Member
Rjected
left a comment
There was a problem hiding this comment.
I like that this makes things simpler, I have a question and suggestion for a helper method but otherwise this looks good to me
Comment on lines
+357
to
+358
| let trie_revert = self.trie_reverts(range.clone())?; | ||
| self.write_trie_updates_sorted(&trie_revert)?; |
Member
There was a problem hiding this comment.
nice, i like that this is now just a call to a provider fn
Comment on lines
+62
to
+90
| // Sanity check that we have static files available to unwind to the target block. | ||
| let highest_static_file_block = | ||
| provider_factory.static_file_provider().get_highest_static_files().max_block_num(); | ||
| if highest_static_file_block | ||
| .filter(|highest_static_file_block| *highest_static_file_block > target) | ||
| .is_none() | ||
| { | ||
| return Err(eyre::eyre!("static files not available for target block {target}, highest is {highest_static_file_block:?}")); | ||
| } | ||
|
|
||
| // Move all applicable data from database to static files. | ||
| pipeline.move_to_static_files()?; | ||
| if self.offline { | ||
| info!(target: "reth::cli", "Performing an unwind for offline-only data!"); | ||
| } | ||
|
|
||
| pipeline.unwind(target, None)?; | ||
| if let Some(highest_static_file_block) = highest_static_file_block { | ||
| info!(target: "reth::cli", ?target, ?highest_static_file_block, "Executing a pipeline unwind."); | ||
| } else { | ||
| info!(target: "reth::cli", ?target, "Executing a database unwind."); | ||
| let provider = provider_factory.provider_rw()?; | ||
| info!(target: "reth::cli", ?target, "Executing a pipeline unwind."); | ||
| } | ||
| info!(target: "reth::cli", prune_config=?config.prune, "Using prune settings"); | ||
|
|
||
| provider | ||
| .remove_block_and_execution_above(target) | ||
| .map_err(|err| eyre::eyre!("Transaction error on unwind: {err}"))?; | ||
| // This will build an offline-only pipeline if the `offline` flag is enabled | ||
| let mut pipeline = | ||
| self.build_pipeline(config, provider_factory, components.evm_config().clone())?; | ||
|
|
||
| // update finalized block if needed | ||
| let last_saved_finalized_block_number = provider.last_finalized_block_number()?; | ||
| if last_saved_finalized_block_number.is_none_or(|f| f > target) { | ||
| provider.save_finalized_block_number(target)?; | ||
| } | ||
| // Move all applicable data from database to static files. | ||
| pipeline.move_to_static_files()?; | ||
|
|
||
| provider.commit()?; | ||
| } | ||
| pipeline.unwind(target, None)?; |
Member
There was a problem hiding this comment.
is the reasoning behind this, that currently we pretty much always have static files and will now always be doing a pipeline unwind?
Member
Author
There was a problem hiding this comment.
Yes, with the work @shekhirin has been doing we always have static files, there's never a situation where static files are behind mdbx because we don't even write that data to mdbx anymore. So if static files don't have the data we can't do anything, just error out, otherwise always pipeline.
yongkangc
reviewed
Oct 7, 2025
| } | ||
|
|
||
| #[test] | ||
| fn test_write_trie_updates_sorted() { |
Contributor
There was a problem hiding this comment.
yea this test is helpful to understand this pr
shekhirin
reviewed
Oct 7, 2025
shekhirin
reviewed
Oct 7, 2025
shekhirin
approved these changes
Oct 7, 2025
Member
shekhirin
left a comment
There was a problem hiding this comment.
LGTM, appreciate the cleanup in reth stage unwind
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Closes #18517
This PR primarily targets the
unwind_trie_state_rangeprovider method, which is used by the engine API to unwind the db during reorgs.This method was previously also potentially used by the unwind pipeline stage, but only in an
elseblock which should never get hit anymore. This else block was replaced with an error.unwind_trie_state_rangewas previously recomputing trie updates using a StateRoot calculation, but it is now able to skip that step and simply read trie updates from the trie changeset tables instead. To make applying these updates easier thewrite_trie_updatesmethod is superceded bywrite_trie_updates_sorted.