From 4562c390bfb86537e3417e9bd69d4d12765d0df3 Mon Sep 17 00:00:00 2001 From: lightsing Date: Wed, 16 Oct 2024 10:43:17 +0800 Subject: [PATCH 1/6] fix: ignore failures in `load_access_list` --- crates/interpreter/src/host.rs | 21 +++++++++ .../interpreter/src/instructions/contract.rs | 19 +++++++- crates/interpreter/src/instructions/host.rs | 43 ++++++++++++++++--- crates/revm/src/context/inner_evm_context.rs | 11 ++++- 4 files changed, 86 insertions(+), 8 deletions(-) diff --git a/crates/interpreter/src/host.rs b/crates/interpreter/src/host.rs index 55f1a8272c..cd6c714249 100644 --- 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; diff --git a/crates/interpreter/src/instructions/contract.rs b/crates/interpreter/src/instructions/contract.rs index 281e147582..06431ba49e 100644 --- a/crates/interpreter/src/instructions/contract.rs +++ b/crates/interpreter/src/instructions/contract.rs @@ -412,10 +412,15 @@ pub fn call(interpreter: &mut Interpreter, host: & return; }; - let Some(account_load) = host.load_account_delegated(to) else { + #[allow(unused_mut)] + let Some(mut account_load) = host.load_account_delegated(to) else { interpreter.instruction_result = InstructionResult::FatalExternalError; return; }; + #[cfg(feature = "scroll")] + if account_load.is_cold && host.is_address_in_access_list(interpreter.contract.target_address) { + account_load.is_cold = false; + } let Some(mut gas_limit) = calc_call_gas::(interpreter, account_load, has_transfer, local_gas_limit) else { @@ -464,6 +469,10 @@ pub fn call_code(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(interpreter.contract.target_address) { + load.is_cold = false; + } let Some(mut gas_limit) = calc_call_gas::(interpreter, load, !value.is_zero(), local_gas_limit) else { @@ -512,6 +521,10 @@ pub fn delegate_call(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(interpreter.contract.target_address) { + load.is_cold = false; + } let Some(gas_limit) = calc_call_gas::(interpreter, load, false, local_gas_limit) else { return; }; @@ -553,6 +566,10 @@ pub fn static_call(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(interpreter.contract.target_address) { + load.is_cold = false; + } let Some(gas_limit) = calc_call_gas::(interpreter, load, false, local_gas_limit) else { return; }; diff --git a/crates/interpreter/src/instructions/host.rs b/crates/interpreter/src/instructions/host.rs index 0da5d9dea2..9ba8b00f1c 100644 --- a/crates/interpreter/src/instructions/host.rs +++ b/crates/interpreter/src/instructions/host.rs @@ -9,10 +9,15 @@ use std::vec::Vec; pub fn balance(interpreter: &mut Interpreter, host: &mut H) { pop_address!(interpreter, address); - let Some(balance) = host.balance(address) else { + #[allow(unused_mut)] + let Some(mut balance) = host.balance(address) else { interpreter.instruction_result = InstructionResult::FatalExternalError; return; }; + #[cfg(feature = "scroll")] + if balance.is_cold && host.is_address_in_access_list(interpreter.contract.target_address) { + balance.is_cold = false; + } gas!( interpreter, if SPEC::enabled(BERLIN) { @@ -62,10 +67,14 @@ pub fn extcodesize(interpreter: &mut Interpreter, #[cfg(feature = "scroll")] pub fn extcodesize(interpreter: &mut Interpreter, host: &mut H) { pop_address!(interpreter, address); - let Some((code_size, is_cold)) = host.code_size(address) else { + let Some((code_size, mut is_cold)) = host.code_size(address) else { interpreter.instruction_result = InstructionResult::FatalExternalError; return; }; + #[cfg(feature = "scroll")] + if is_cold && host.is_address_in_access_list(interpreter.contract.target_address) { + is_cold = false; + } gas!(interpreter, warm_cold_cost(is_cold)); push!(interpreter, U256::from(code_size)); @@ -75,10 +84,15 @@ pub fn extcodesize(interpreter: &mut Interpreter, pub fn extcodehash(interpreter: &mut Interpreter, host: &mut H) { check!(interpreter, CONSTANTINOPLE); pop_address!(interpreter, address); - let Some(code_hash) = host.code_hash(address) else { + #[allow(unused_mut)] + let Some(mut code_hash) = host.code_hash(address) else { interpreter.instruction_result = InstructionResult::FatalExternalError; return; }; + #[cfg(feature = "scroll")] + if code_hash.is_cold && host.is_address_in_access_list(interpreter.contract.target_address) { + code_hash.is_cold = false; + } let (code_hash, load) = code_hash.into_components(); if SPEC::enabled(BERLIN) { gas!(interpreter, warm_cold_cost_with_delegation(load)) @@ -94,10 +108,15 @@ pub fn extcodecopy(interpreter: &mut Interpreter, pop_address!(interpreter, address); pop!(interpreter, memory_offset, code_offset, len_u256); - let Some(code) = host.code(address) else { + #[allow(unused_mut)] + let Some(mut code) = host.code(address) else { interpreter.instruction_result = InstructionResult::FatalExternalError; return; }; + #[cfg(feature = "scroll")] + if code.is_cold && host.is_address_in_access_list(interpreter.contract.target_address) { + code.is_cold = false; + } let len = as_usize_or_fail!(interpreter, len_u256); let (code, load) = code.into_components(); @@ -164,10 +183,17 @@ pub fn blockhash(interpreter: &mut Interpreter, ho pub fn sload(interpreter: &mut Interpreter, host: &mut H) { pop_top!(interpreter, index); - let Some(value) = host.sload(interpreter.contract.target_address, *index) else { + #[allow(unused_mut)] + let Some(mut value) = host.sload(interpreter.contract.target_address, *index) else { 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) + { + value.is_cold = false; + } gas!(interpreter, gas::sload_cost(SPEC::SPEC_ID, value.is_cold)); *index = value.data; } @@ -180,6 +206,13 @@ pub fn sstore(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 index e451488942..b5434a90e7 100644 --- a/crates/revm/src/context/inner_evm_context.rs +++ b/crates/revm/src/context/inner_evm_context.rs @@ -107,11 +107,18 @@ impl InnerEvmContext { 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")] { + result.ok(); + } else { + result?; + } + } } Ok(()) } From 0fb08c54a2dfb00f958b95bda82b47b443807a04 Mon Sep 17 00:00:00 2001 From: lightsing Date: Wed, 16 Oct 2024 10:50:57 +0800 Subject: [PATCH 2/6] add comments --- crates/revm/src/context/inner_evm_context.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/revm/src/context/inner_evm_context.rs b/crates/revm/src/context/inner_evm_context.rs index b5434a90e7..12382351e1 100644 --- a/crates/revm/src/context/inner_evm_context.rs +++ b/crates/revm/src/context/inner_evm_context.rs @@ -114,6 +114,10 @@ impl InnerEvmContext { ); cfg_if::cfg_if! { if #[cfg(feature = "scroll")] { + // In scroll, we don't include the account in access list + // 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 account was never accessed. result.ok(); } else { result?; From 3af83b64a49e2c2b9ce31597f49f12ca392cbc32 Mon Sep 17 00:00:00 2001 From: lightsing Date: Wed, 16 Oct 2024 11:15:48 +0800 Subject: [PATCH 3/6] restrict error type --- crates/revm/src/context/inner_evm_context.rs | 22 ++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/crates/revm/src/context/inner_evm_context.rs b/crates/revm/src/context/inner_evm_context.rs index 12382351e1..fd2429c0f0 100644 --- a/crates/revm/src/context/inner_evm_context.rs +++ b/crates/revm/src/context/inner_evm_context.rs @@ -114,11 +114,25 @@ impl InnerEvmContext { ); cfg_if::cfg_if! { if #[cfg(feature = "scroll")] { - // In scroll, we don't include the account in access list - // if it was not actually accessed in the transaction. + // 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 account was never accessed. - result.ok(); + // This is not a problem as the accounts/storages was never accessed. + match result { + Err(EVMError::Database(e)) => { + // 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. + assert_eq!( + "sbv_core::error::DatabaseError", + core::any::type_name_of_val(&e), + "unexpected error type" + ); + } + _ => { result?; } + } } else { result?; } From c6e129af47b1b431183a4d2c8ea97f49e6b7f152 Mon Sep 17 00:00:00 2001 From: lightsing Date: Wed, 16 Oct 2024 11:18:45 +0800 Subject: [PATCH 4/6] return error otherwise --- crates/revm/src/context/inner_evm_context.rs | 23 +++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/crates/revm/src/context/inner_evm_context.rs b/crates/revm/src/context/inner_evm_context.rs index fd2429c0f0..9b666f70c2 100644 --- a/crates/revm/src/context/inner_evm_context.rs +++ b/crates/revm/src/context/inner_evm_context.rs @@ -119,19 +119,16 @@ impl InnerEvmContext { // 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 { - Err(EVMError::Database(e)) => { - // 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. - assert_eq!( - "sbv_core::error::DatabaseError", - core::any::type_name_of_val(&e), - "unexpected error type" - ); - } - _ => { 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?; From 4eba564842a45bb2508b89f8d926a52bbbfd09a2 Mon Sep 17 00:00:00 2001 From: lightsing Date: Wed, 16 Oct 2024 12:09:07 +0800 Subject: [PATCH 5/6] panic on use of not loaded account --- .../interpreter/src/instructions/contract.rs | 11 ++++----- crates/interpreter/src/instructions/host.rs | 24 ++++++++----------- 2 files changed, 15 insertions(+), 20 deletions(-) diff --git a/crates/interpreter/src/instructions/contract.rs b/crates/interpreter/src/instructions/contract.rs index 06431ba49e..3bf8987a8e 100644 --- a/crates/interpreter/src/instructions/contract.rs +++ b/crates/interpreter/src/instructions/contract.rs @@ -412,14 +412,13 @@ pub fn call(interpreter: &mut Interpreter, host: & return; }; - #[allow(unused_mut)] - let Some(mut account_load) = host.load_account_delegated(to) else { + let Some(account_load) = host.load_account_delegated(to) else { interpreter.instruction_result = InstructionResult::FatalExternalError; return; }; #[cfg(feature = "scroll")] if account_load.is_cold && host.is_address_in_access_list(interpreter.contract.target_address) { - account_load.is_cold = false; + panic!("access list account should be either loaded or never accessed"); } let Some(mut gas_limit) = calc_call_gas::(interpreter, account_load, has_transfer, local_gas_limit) @@ -471,7 +470,7 @@ pub fn call_code(interpreter: &mut Interpreter, ho load.is_empty = false; #[cfg(feature = "scroll")] if load.is_cold && host.is_address_in_access_list(interpreter.contract.target_address) { - load.is_cold = false; + panic!("access list account should be either loaded or never accessed"); } let Some(mut gas_limit) = calc_call_gas::(interpreter, load, !value.is_zero(), local_gas_limit) @@ -523,7 +522,7 @@ pub fn delegate_call(interpreter: &mut Interpreter load.is_empty = false; #[cfg(feature = "scroll")] if load.is_cold && host.is_address_in_access_list(interpreter.contract.target_address) { - load.is_cold = false; + panic!("access list account should be either loaded or never accessed"); } let Some(gas_limit) = calc_call_gas::(interpreter, load, false, local_gas_limit) else { return; @@ -568,7 +567,7 @@ pub fn static_call(interpreter: &mut Interpreter, load.is_empty = false; #[cfg(feature = "scroll")] if load.is_cold && host.is_address_in_access_list(interpreter.contract.target_address) { - load.is_cold = false; + panic!("access list account should be either loaded or never accessed"); } let Some(gas_limit) = calc_call_gas::(interpreter, load, false, local_gas_limit) else { return; diff --git a/crates/interpreter/src/instructions/host.rs b/crates/interpreter/src/instructions/host.rs index 9ba8b00f1c..ced1b1fa90 100644 --- a/crates/interpreter/src/instructions/host.rs +++ b/crates/interpreter/src/instructions/host.rs @@ -9,14 +9,13 @@ use std::vec::Vec; pub fn balance(interpreter: &mut Interpreter, host: &mut H) { pop_address!(interpreter, address); - #[allow(unused_mut)] - let Some(mut balance) = host.balance(address) else { + let Some(balance) = host.balance(address) else { interpreter.instruction_result = InstructionResult::FatalExternalError; return; }; #[cfg(feature = "scroll")] if balance.is_cold && host.is_address_in_access_list(interpreter.contract.target_address) { - balance.is_cold = false; + panic!("access list account should be either loaded or never accessed"); } gas!( interpreter, @@ -67,13 +66,13 @@ pub fn extcodesize(interpreter: &mut Interpreter, #[cfg(feature = "scroll")] pub fn extcodesize(interpreter: &mut Interpreter, host: &mut H) { pop_address!(interpreter, address); - let Some((code_size, mut is_cold)) = host.code_size(address) else { + let Some((code_size, is_cold)) = host.code_size(address) else { interpreter.instruction_result = InstructionResult::FatalExternalError; return; }; #[cfg(feature = "scroll")] if is_cold && host.is_address_in_access_list(interpreter.contract.target_address) { - is_cold = false; + panic!("access list account should be either loaded or never accessed"); } gas!(interpreter, warm_cold_cost(is_cold)); @@ -84,14 +83,13 @@ pub fn extcodesize(interpreter: &mut Interpreter, pub fn extcodehash(interpreter: &mut Interpreter, host: &mut H) { check!(interpreter, CONSTANTINOPLE); pop_address!(interpreter, address); - #[allow(unused_mut)] - let Some(mut code_hash) = host.code_hash(address) else { + let Some(code_hash) = host.code_hash(address) else { interpreter.instruction_result = InstructionResult::FatalExternalError; return; }; #[cfg(feature = "scroll")] if code_hash.is_cold && host.is_address_in_access_list(interpreter.contract.target_address) { - code_hash.is_cold = false; + panic!("access list account should be either loaded or never accessed"); } let (code_hash, load) = code_hash.into_components(); if SPEC::enabled(BERLIN) { @@ -108,14 +106,13 @@ pub fn extcodecopy(interpreter: &mut Interpreter, pop_address!(interpreter, address); pop!(interpreter, memory_offset, code_offset, len_u256); - #[allow(unused_mut)] - let Some(mut code) = host.code(address) else { + let Some(code) = host.code(address) else { interpreter.instruction_result = InstructionResult::FatalExternalError; return; }; #[cfg(feature = "scroll")] if code.is_cold && host.is_address_in_access_list(interpreter.contract.target_address) { - code.is_cold = false; + panic!("access list account should be either loaded or never accessed"); } let len = as_usize_or_fail!(interpreter, len_u256); @@ -183,8 +180,7 @@ pub fn blockhash(interpreter: &mut Interpreter, ho pub fn sload(interpreter: &mut Interpreter, host: &mut H) { pop_top!(interpreter, index); - #[allow(unused_mut)] - let Some(mut value) = host.sload(interpreter.contract.target_address, *index) else { + let Some(value) = host.sload(interpreter.contract.target_address, *index) else { interpreter.instruction_result = InstructionResult::FatalExternalError; return; }; @@ -192,7 +188,7 @@ pub fn sload(interpreter: &mut Interpreter, host: if value.is_cold && host.is_storage_key_in_access_list(interpreter.contract.target_address, *index) { - value.is_cold = false; + 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; From 057e1bec147380c04870291f606f238c67da6f9d Mon Sep 17 00:00:00 2001 From: lightsing Date: Wed, 16 Oct 2024 14:20:58 +0800 Subject: [PATCH 6/6] fix --- crates/interpreter/src/instructions/contract.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/interpreter/src/instructions/contract.rs b/crates/interpreter/src/instructions/contract.rs index 3bf8987a8e..d5d8f49ccd 100644 --- a/crates/interpreter/src/instructions/contract.rs +++ b/crates/interpreter/src/instructions/contract.rs @@ -417,7 +417,7 @@ pub fn call(interpreter: &mut Interpreter, host: & return; }; #[cfg(feature = "scroll")] - if account_load.is_cold && host.is_address_in_access_list(interpreter.contract.target_address) { + 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) = @@ -469,7 +469,7 @@ pub fn call_code(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(interpreter.contract.target_address) { + 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) = @@ -521,7 +521,7 @@ pub fn delegate_call(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(interpreter.contract.target_address) { + 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::(interpreter, load, false, local_gas_limit) else { @@ -566,7 +566,7 @@ pub fn static_call(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(interpreter.contract.target_address) { + 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::(interpreter, load, false, local_gas_limit) else {