fix: block apply in the past #11767
fix: block apply in the past #11767
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11767 +/- ##
==========================================
- Coverage 71.82% 71.80% -0.02%
==========================================
Files 792 792
Lines 162765 162806 +41
Branches 162765 162806 +41
==========================================
+ Hits 116899 116901 +2
- Misses 40822 40853 +31
- Partials 5044 5052 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| let mut storage = | ||
| RuntimeStorageConfig::new(*chunk_inner.prev_state_root(), use_flat_storage); | ||
| if use_trie_for_free { | ||
| storage.source = StorageDataSource::DbTrieOnly; | ||
| } |
There was a problem hiding this comment.
It seems like use flat storage and source should be exclusive options. Can you structure it that way in code?
There was a problem hiding this comment.
I have started in such a way, but then I realized you could potentially use both together.. for example use flat storage and still access trie for free.
No strong opinion if you prefer I'll make the options in CLI exclusive
There was a problem hiding this comment.
When would it be possible to use both?
There was a problem hiding this comment.
I think when flat storage is enabled we don't read trie at all (except for the non-inlined values).
There was a problem hiding this comment.
I'm referring to this struct
pub struct RuntimeStorageConfig {
pub state_root: StateRoot,
pub use_flat_storage: bool,
pub source: StorageDataSource,
pub state_patch: SandboxStatePatch,
}
API-wise it gives the impression use-flat-storage is orthogonal from the StorageDataSource. You can use flat storage or not over a DB that charges gas costs or not.
Yes not charging gas costs is similar to using flat-storage in some contexts (replaying block)
There was a problem hiding this comment.
Ah well then this struct is wrong too :) Given the source is already there the 'use_flat_storage' should be derivable from it. I would just remove it. If you don't want to go so far in this PR perhaps you can just add a new ctor? I just dislike this pattern of calling ctor and then immediately overwriting one of the fields, it seems very volatile.
There was a problem hiding this comment.
Agree, I'll add another constructor!
There was a problem hiding this comment.
I have made some sweeping changes to make stuff more explicit. No more flags to enable flat storage or free tries, instead now there's an enum StorageSource with all different options
| /// TODO(#8741): doesn't work. Remove dependency on flat storage | ||
| /// by simulating correct costs. Consider reintroducing DbTrieOnly | ||
| /// read mode removed at #10490. |
tools/state-viewer/src/cli.rs
Outdated
| #[clap(long)] | ||
| use_flat_storage: bool, | ||
| /// Use the data stored in trie, but without paying extra gas costs. | ||
| /// This could be used to simulate flat storage when the latter is not present. | ||
| #[clap(long)] | ||
| use_trie_for_free: bool, |
There was a problem hiding this comment.
This is just fine as is but I wonder if we could auto detect the right configuration. It should be possible to check
a) if flat storage should be used, based on the protocol version of the block
b) if flat storage is present at the given height
Then you can setup the db source based on that. This is totally optional as in practice we will almost always use (false, true).
There was a problem hiding this comment.
Interesting idea, maybe for improvement as a default it could use autodetect, I'll file an issue. I feel there are interesting scenarios (to check some stuff) where someone would prefer to fine tune the flags, so it makes sense to leave such option available
There was a problem hiding this comment.
Yeah in the follow up you could have something like enum StorageSource { AutoDetect, Trie, TrieFree, FS }
| pub(crate) fn resulting_chunk_extra( | ||
| result: &ApplyChunkResult, | ||
| gas_limit: Gas, | ||
| protocol_version: ProtocolVersion, |
| @@ -180,27 +181,42 @@ impl StateViewerSubCommand { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(clap::ValueEnum, Debug, Clone, Copy)] | |||
| #[clap(rename_all = "kebab_case")] | |||
This PR fixes commands such as
neard view-state applyandneard view-state apply-rangewhen used on a block height that doesn't have flat storage built for it.It works by re enabling the feature of accessing trie nodes without paying gas costs (removed in #10490).
Probably it also fixes #8741.