STR-1308: EE state diff prototype #800
Conversation
evgenyzdanovich
changed the title
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #800 +/- ##
==========================================
+ Coverage 52.72% 53.05% +0.32%
==========================================
Files 300 302 +2
Lines 32940 33289 +349
==========================================
+ Hits 17367 17660 +293
- Misses 15573 15629 +56
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Commit: 6a432d0 SP1 Execution Results
|
There was a problem hiding this comment.
I'm not sure what from this design comes out of how the exex API is designed. Including tracking in the BatchStateDiff structure feels undesirable, see comments
I know this is a prototype and that's why we're using serde for it, but that seems like a huge overhead. At least adding #[serde(rename = ...)] to fields to make them encode as single chars would slim this down and skipping serializing entries in AccountInfo if they're unchanged.
Should we also include in the prototype a checker/applicator that applies the diff on top of a previous state and ensures it matches a final state? It would be nice to have that since that's how I imagine the flow in the proof would be more like, instead of recomputing the diff while checking the blocks and checking it matches a hash passed.
| /// N.B. "original" counterparts are needed to correctly construct the [`BatchStateDiff`] | ||
| /// for the range of blocks and not include changes for the keys whose values | ||
| /// were changed inside batch, but end up having the same value as before the batch. |
There was a problem hiding this comment.
This seems like it's not just a diff then? We should have some builder type that remembers the original values and tracks modifications we're making, comparing against the originals, and updates an inner diff structure we extract at the end?
There was a problem hiding this comment.
Well, agree. Introduced a thin builder type that does pretty much the same.
| pub fn apply(&mut self, diff: BlockStateDiff) { | ||
| // Accumulate deployed contracts. | ||
| for (addr, bytecode) in diff.contracts.into_iter() { | ||
| self.contracts.insert(addr, bytecode); | ||
| } | ||
|
|
||
| for (addr, acc) in &diff.state { | ||
| // Update original account if seen for the first time in the batch. | ||
| if !self.original_accounts.contains_key(addr) { | ||
| self.original_accounts | ||
| .insert(*addr, acc.original_info.clone()); | ||
| } | ||
|
|
||
| // Now, modify the actual account entry. | ||
| let cur_account = acc.account_info(); | ||
| if &cur_account == self.original_accounts.get(addr).unwrap() { | ||
| // Remove if the actual account equals to the original. | ||
| self.accounts.remove(addr); | ||
| } else { | ||
| // The current account is different, update it. | ||
| self.accounts.insert(*addr, cur_account); | ||
| } | ||
|
|
||
| let original_acc_storage = self.original_storage_slots.entry(*addr).or_default(); | ||
| let cur_acc_storage = self.storage_slots.entry(*addr).or_default(); | ||
|
|
||
| for (key, value) in &acc.storage { | ||
| // Update original account storage if seen for the first time in the batch. | ||
| if !original_acc_storage.contains_key(key) { | ||
| original_acc_storage.insert(*key, value.original_value()); | ||
| } |
There was a problem hiding this comment.
There's this weirdness here when we merge these diffs because we have to carry along the extra originals information inside the diff. What happens if the two diffs have different originals? That would probably be a bug, but this feels weird to me.
There was a problem hiding this comment.
I mean, this weirdness, IIUC, is imposed by the exex interface. If that happens - it pretty much means there's a bug in there.
…e root based off state diffs.
evgenyzdanovich
force-pushed
the
str-1308-ee-state-diff-prototype
branch
from
d5e9a14 to
aef42c7
Compare
| /// block number => hash mapping for easier testing. | ||
| (BlockNumberByHash) u64 => Vec<u8> |
There was a problem hiding this comment.
Shouldn't this be u64 => B256? And wouldn't it be called BlockHashByNumber? I'm not sure of the real schema here.
| #[derive(ThisError, Debug)] | ||
| /// A concrete error that may happen during state reconstruction. | ||
| pub enum StateError { | ||
| #[error("Mpt Error: {0}")] | ||
| MptError(#[from] strata_mpt::Error), |
There was a problem hiding this comment.
In the main strata repo we've been doing this as
/// docs
#[derive(Debug, ThisError)]
pub enum StateError {
#[error("mpt: {0}")]
...
not sure what the proper style in here ought be.
|
|
||
| /// An (in-memory) representation of the EVM state reconstructed only from [`BatchStateDiff`]. | ||
| #[derive(Clone, Default, Debug)] | ||
| pub struct StateReconstructed { |
There was a problem hiding this comment.
ReconstructedState?
Also the order of items in this module is weird and split up into different parts.
| if account_info.is_none() { | ||
| // Account was actually destructed. | ||
| self.state_trie.delete(&acc_info_trie_path)?; | ||
| continue; | ||
| } | ||
| let account_info = account_info.unwrap(); |
There was a problem hiding this comment.
Could write this as
let Some(account_info) = account_info else {
// was deleted
...
continue;
};
...| .map_err(to_jsonrpsee_error("Failed fetching block state diff")) | ||
| } | ||
|
|
||
| fn get_state_root_by_diffs(&self, block_number: u64) -> RpcResult<Option<B256>> { |
There was a problem hiding this comment.
I would call this _via_diffs. Saying "by" here implies that the diffs are a key we're looking up the state root by.
Description
A prototype impl of EE DA using state diffs (without compression).
Type of Change
Notes to Reviewers
Checklist
Related Issues