Skip to content

Remove call storage - #2644

Closed
carneiro-cw wants to merge 16 commits into
mainfrom
remove_call_storage
Closed

Remove call storage#2644
carneiro-cw wants to merge 16 commits into
mainfrom
remove_call_storage

Conversation

@carneiro-cw

@carneiro-cw carneiro-cw commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

PR Type

Enhancement, Tests


Description

  • Introduce generic State replacing Changes everywhere

  • Refactor storage & cache layers to use new State API

  • Remove call-specific temporary storage & simplify EntityRead

  • Update transaction execution & EVM output to use State

  • Adjust tests to new State and TransactionExecutionResult


File Walkthrough

Relevant files
Enhancement
18 files
stratus_storage.rs
Refactor StratusStorage to use State                     
+62/-114
values.rs
Add Change trait and value wrappers                                           
+109/-153
transaction_execution.rs
Update TransactionExecutionOutput with State field             
+51/-46 
resolve_pending.rs
Simplify Resolve logic and rename variants                             
+17/-86 
mod.rs
Pass state into save_execution calls                                         
+33/-38 
transaction.rs
In-memory temp storage uses State                           
+31/-27 
rocks_state.rs
Rocks state uses State and account update                 
+25/-39 
mod.rs
Introduce State and stage markers                                         
+194/-0 
miner.rs
Miner.save_execution accepts State                         
+18/-27 
cache.rs
Simplify cache, remove slot/account temp cache                     
+15/-70 
mod.rs
Remove call_storage, unify temp storage                                   
+13/-31 
transaction_execution.rs
Rename fields to input/output with State                                 
+16/-16 
events.rs
Use output.logs instead of result.logs                                     
+12/-12 
transaction_mined.rs
Map TransactionExecutionResult fields in rocks types         
+19/-21 
block_with_changes.rs
Fetcher returns State now                                       
+3/-3     
server.rs
rpc_call returns CallExecutionOutput                                         
+1/-2     
call_execution.rs
Add is_success helper for call output                                       
+4/-0     
rocks_permanent.rs
save_block uses State instead of Changes                   
+4/-3     
Tests
1 files
mod.rs
Use State in importer tests                                       
+13/-13 
Bug fix
1 files
session.rs
Skip ignored accounts in RevmSession                                         
+11/-0   
Additional files
21 files
Cargo.toml +2/-1     
mod.rs +5/-5     
call_execution.rs +0/-15   
mod.rs +1/-0     
util.rs +4/-4     
mod.rs +2/-9     
fake_leader.rs +9/-29   
replication.rs +3/-3     
log_filter_input.rs +1/-1     
mod.rs +0/-1     
account.rs +19/-0   
block_changes.rs +14/-0   
call.rs +0/-127 
block.rs +3/-3     
pending_block_header.rs +1/-1     
execution_kind.rs +0/-58   
mod.rs +0/-1     
unix_time_now.rs +1/-1     
transaction_mined.rs +4/-4     
transaction_stage.rs +4/-4     
metrics_definitions.rs +1/-1     

@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 5 🔵🔵🔵🔵🔵
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Silent Error Handling

The new EntityRead::read_temp methods drop the StorageError from underlying calls and always return an Option, so any I/O or deserialization failure in s.temp.read_* will be swallowed without logging. This hides temporary‐storage errors and makes debugging read failures impossible.

    fn read_temp(s: &StratusStorage, key: Self::Key) -> Option<Self>;
    /// Reads from permanent storage at the resolved mined point.
    fn read_perm(s: &StratusStorage, key: Self::Key, point: MinedPointInTime<'_>) -> Result<Self, StorageError>;
    /// Caches the value as a latest (mined tip) entry, if not already cached.
    fn cache_latest_if_missing(s: &StratusStorage, key: Self::Key, value: Self);
}

impl EntityRead for Account {
    type Key = Address;

    fn read_temp(s: &StratusStorage, address: Address) -> Option<Self> {
        tracing::debug!(storage = %label::TEMP, %address, "reading account");
        timed(|| s.temp.read_account(address)).with(|m| {
            if m.result.is_some() {
                metrics::inc_storage_read_account(m.elapsed, label::TEMP, PointInTime::Pending, true);
            }
        })
    }

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Match on point_in_time for caching

The cache decision should be based on the point-in-time rather than the full
ExecutionKind so that RPC reads and call reads are handled correctly. Match on
kind.point_in_time() to distinguish Pending vs Latest reads when deciding to cache.

src/eth/storage/stratus_storage.rs [380-387]

-match (kind, found_at) {
-    (ExecutionKind::Transaction, _) => (),
-    // A pending read that hit perm (i.e. not in any cache/temp) is already mined, so cache latest.
-    // OR A mined read that hit perm is the latest state, so populate the latest cache.
-    (_, FoundAt::PermLatest) => {
+match (kind.point_in_time(), found_at) {
+    (PointInTime::Pending, FoundAt::PermLatest)
+    | (PointInTime::Latest, FoundAt::PermLatest) => {
         E::cache_latest_if_missing(self, key, value.clone());
     }
-    // Cache / Historical / (Mined, Temp): nothing to cache.
+    _ => {}
 }
Suggestion importance[1-10]: 5

__

Why: Matching on kind.point_in_time() instead of the full ExecutionKind can prevent unintended caching for historical reads, but the proposed change may not fully align with the existing Resolve logic and needs further validation.

Low

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant