fix: ignore failures in load_access_list (#27) (#37)

* fix: ignore failures in `load_access_list` * add comments * restrict error type * return error otherwise * panic on use of not loaded account * fix
scroll-tech · Oct 30, 2024 · 229ead9 · 229ead9
1 parent 6470403
commit 229ead9
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 2 deletions.
diff --git a/crates/interpreter/src/host.rs b/crates/interpreter/src/host.rs
@@ -12,6 +12,27 @@ pub trait Host {
     /// Returns a mutable reference to the environment.
     fn env_mut(&mut self) -> &mut Env;
 
+    #[cfg(feature = "scroll")]
+    /// Check an address is in the access list.
+    fn is_address_in_access_list(&self, address: Address) -> bool {
+        self.env()
+            .tx
+            .access_list
+            .iter()
+            .any(|item| item.address == address)
+    }
+
+    #[cfg(feature = "scroll")]
+    /// Check a storage key is in the access list.
+    fn is_storage_key_in_access_list(&self, address: Address, index: U256) -> bool {
+        self.env()
+            .tx
+            .access_list
+            .iter()
+            .filter(|item| item.address == address)
+            .any(|item| item.storage_keys.contains(&B256::from(index)))
+    }
+
     /// Load an account code.
     fn load_account_delegated(&mut self, address: Address) -> Option<AccountLoad>;
 

diff --git a/crates/interpreter/src/instructions/contract.rs b/crates/interpreter/src/instructions/contract.rs
@@ -416,6 +416,10 @@ pub fn call<H: Host + ?Sized, SPEC: Spec>(interpreter: &mut Interpreter, host: &
         interpreter.instruction_result = InstructionResult::FatalExternalError;
         return;
     };
+    #[cfg(feature = "scroll")]
+    if account_load.is_cold && host.is_address_in_access_list(to) {
+        panic!("access list account should be either loaded or never accessed");
+    }
     let Some(mut gas_limit) =
         calc_call_gas::<SPEC>(interpreter, account_load, has_transfer, local_gas_limit)
     else {
@@ -464,6 +468,10 @@ pub fn call_code<H: Host + ?Sized, SPEC: Spec>(interpreter: &mut Interpreter, ho
     };
     // set is_empty to false as we are not creating this account.
     load.is_empty = false;
+    #[cfg(feature = "scroll")]
+    if load.is_cold && host.is_address_in_access_list(to) {
+        panic!("access list account should be either loaded or never accessed");
+    }
     let Some(mut gas_limit) =
         calc_call_gas::<SPEC>(interpreter, load, !value.is_zero(), local_gas_limit)
     else {
@@ -512,6 +520,10 @@ pub fn delegate_call<H: Host + ?Sized, SPEC: Spec>(interpreter: &mut Interpreter
     };
     // set is_empty to false as we are not creating this account.
     load.is_empty = false;
+    #[cfg(feature = "scroll")]
+    if load.is_cold && host.is_address_in_access_list(to) {
+        panic!("access list account should be either loaded or never accessed");
+    }
     let Some(gas_limit) = calc_call_gas::<SPEC>(interpreter, load, false, local_gas_limit) else {
         return;
     };
@@ -553,6 +565,10 @@ pub fn static_call<H: Host + ?Sized, SPEC: Spec>(interpreter: &mut Interpreter,
     };
     // set is_empty to false as we are not creating this account.
     load.is_empty = false;
+    #[cfg(feature = "scroll")]
+    if load.is_cold && host.is_address_in_access_list(to) {
+        panic!("access list account should be either loaded or never accessed");
+    }
     let Some(gas_limit) = calc_call_gas::<SPEC>(interpreter, load, false, local_gas_limit) else {
         return;
     };

diff --git a/crates/interpreter/src/instructions/host.rs b/crates/interpreter/src/instructions/host.rs
@@ -13,6 +13,10 @@ pub fn balance<H: Host + ?Sized, SPEC: Spec>(interpreter: &mut Interpreter, host
         interpreter.instruction_result = InstructionResult::FatalExternalError;
         return;
     };
+    #[cfg(feature = "scroll")]
+    if balance.is_cold && host.is_address_in_access_list(interpreter.contract.target_address) {
+        panic!("access list account should be either loaded or never accessed");
+    }
     gas!(
         interpreter,
         if SPEC::enabled(BERLIN) {
@@ -66,6 +70,10 @@ pub fn extcodesize<H: Host + ?Sized, SPEC: Spec>(interpreter: &mut Interpreter,
         interpreter.instruction_result = InstructionResult::FatalExternalError;
         return;
     };
+    #[cfg(feature = "scroll")]
+    if is_cold && host.is_address_in_access_list(interpreter.contract.target_address) {
+        panic!("access list account should be either loaded or never accessed");
+    }
     gas!(interpreter, warm_cold_cost(is_cold));
 
     push!(interpreter, U256::from(code_size));
@@ -79,6 +87,10 @@ pub fn extcodehash<H: Host + ?Sized, SPEC: Spec>(interpreter: &mut Interpreter,
         interpreter.instruction_result = InstructionResult::FatalExternalError;
         return;
     };
+    #[cfg(feature = "scroll")]
+    if code_hash.is_cold && host.is_address_in_access_list(interpreter.contract.target_address) {
+        panic!("access list account should be either loaded or never accessed");
+    }
     let (code_hash, load) = code_hash.into_components();
     if SPEC::enabled(BERLIN) {
         gas!(interpreter, warm_cold_cost_with_delegation(load))
@@ -98,6 +110,10 @@ pub fn extcodecopy<H: Host + ?Sized, SPEC: Spec>(interpreter: &mut Interpreter,
         interpreter.instruction_result = InstructionResult::FatalExternalError;
         return;
     };
+    #[cfg(feature = "scroll")]
+    if code.is_cold && host.is_address_in_access_list(interpreter.contract.target_address) {
+        panic!("access list account should be either loaded or never accessed");
+    }
 
     let len = as_usize_or_fail!(interpreter, len_u256);
     let (code, load) = code.into_components();
@@ -168,6 +184,12 @@ pub fn sload<H: Host + ?Sized, SPEC: Spec>(interpreter: &mut Interpreter, host:
         interpreter.instruction_result = InstructionResult::FatalExternalError;
         return;
     };
+    #[cfg(feature = "scroll")]
+    if value.is_cold
+        && host.is_storage_key_in_access_list(interpreter.contract.target_address, *index)
+    {
+        panic!("access list account should be either loaded or never accessed");
+    }
     gas!(interpreter, gas::sload_cost(SPEC::SPEC_ID, value.is_cold));
     *index = value.data;
 }
@@ -180,6 +202,13 @@ pub fn sstore<H: Host + ?Sized, SPEC: Spec>(interpreter: &mut Interpreter, host:
         interpreter.instruction_result = InstructionResult::FatalExternalError;
         return;
     };
+    #[cfg(feature = "scroll")]
+    if state_load.is_cold
+        && host.is_storage_key_in_access_list(interpreter.contract.target_address, index)
+    {
+        interpreter.instruction_result = InstructionResult::NotActivated;
+        return;
+    }
     gas_or_fail!(interpreter, {
         let remaining_gas = interpreter.gas.remaining();
         gas::sstore_cost(

diff --git a/crates/revm/src/context/inner_evm_context.rs b/crates/revm/src/context/inner_evm_context.rs
@@ -107,11 +107,33 @@ impl<DB: Database> InnerEvmContext<DB> {
             storage_keys,
         } in self.env.tx.access_list.iter()
         {
-            self.journaled_state.initial_account_load(
+            let result = self.journaled_state.initial_account_load(
                 *address,
                 storage_keys.iter().map(|i| U256::from_be_bytes(i.0)),
                 &mut self.db,
-            )?;
+            );
+            cfg_if::cfg_if! {
+                if #[cfg(feature = "scroll")] {
+                    // In scroll, we don't include the access list accounts/storages in the partial
+                    // merkle trie proofs if it was not actually accessed in the transaction.
+                    // The load will fail in that case, we just ignore the error.
+                    // This is not a problem as the accounts/storages was never accessed.
+                    match result {
+                         // the concrete error in scroll is
+                         // https://github.com/scroll-tech/stateless-block-verifier/blob/851f5141ded76ddba7594814b9761df1dc469a12/crates/core/src/error.rs#L4-L13
+                         // We cannot check it since `Database::Error` is an opaque type
+                         // without any trait bounds (like `Debug` or `Display`).
+                         // only thing we can do is to check the type name.
+                         Err(EVMError::Database(e))
+                             if core::any::type_name_of_val(&e) == "sbv_core::error::DatabaseError" => {}
+                         _ => {
+                             result?;
+                         }
+                    }
+                } else {
+                    result?;
+                }
+            }
         }
         Ok(())
     }