From 137c4478ee21ff555da1c73e9a9b28c516b67abc Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 10 Nov 2022 17:48:13 -0800 Subject: [PATCH] EVM: simplify and fix evm storage (#831) * EVM: simplify and fix evm storage 1. The EVM doesn't distinguish between "exists" and "doesn't exist". 2. There was some interesting dead code. 3. Only mark the state-tree dirty if something has changed. * nit: make vyzo happy --- .../src/interpreter/instructions/storage.rs | 13 ++--- actors/evm/src/interpreter/system.rs | 58 +++++-------------- actors/evm/src/lib.rs | 5 +- actors/evm/tests/basic.rs | 19 +++--- 4 files changed, 33 insertions(+), 62 deletions(-) diff --git a/actors/evm/src/interpreter/instructions/storage.rs b/actors/evm/src/interpreter/instructions/storage.rs index 2614a1a0d..6d2296794 100644 --- a/actors/evm/src/interpreter/instructions/storage.rs +++ b/actors/evm/src/interpreter/instructions/storage.rs @@ -1,5 +1,5 @@ use { - crate::interpreter::{ExecutionState, StatusCode, System, U256}, + crate::interpreter::{ExecutionState, StatusCode, System}, fil_actors_runtime::runtime::Runtime, }; @@ -12,11 +12,7 @@ pub fn sload( let location = state.stack.pop(); // get from storage and place on stack - let value = match system.get_storage(location)? { - Some(val) => val, - None => U256::zero(), - }; - state.stack.push(value); + state.stack.push(system.get_storage(location)?); Ok(()) } @@ -29,10 +25,9 @@ pub fn sstore( return Err(StatusCode::StaticModeViolation); } - let location = state.stack.pop(); + let key = state.stack.pop(); let value = state.stack.pop(); - let opt_value = if value == U256::zero() { None } else { Some(value) }; - system.set_storage(location, opt_value)?; + system.set_storage(key, value)?; Ok(()) } diff --git a/actors/evm/src/interpreter/system.rs b/actors/evm/src/interpreter/system.rs index 3d134876f..4e1593e46 100644 --- a/actors/evm/src/interpreter/system.rs +++ b/actors/evm/src/interpreter/system.rs @@ -23,20 +23,6 @@ use { fvm_ipld_hamt::Hamt, }; -#[derive(Clone, Copy, Debug)] -pub enum StorageStatus { - /// The value of a storage item has been left unchanged: 0 -> 0 and X -> X. - Unchanged, - /// The value of a storage item has been modified: X -> Y. - Modified, - /// A storage item has been modified after being modified before: X -> Y -> Z. - ModifiedAgain, - /// A new storage item has been added: 0 -> X. - Added, - /// A storage item has been deleted: X -> 0. - Deleted, -} - /// Platform Abstraction Layer /// that bridges the FVM world to EVM world pub struct System<'r, RT: Runtime> { @@ -205,40 +191,28 @@ impl<'r, RT: Runtime> System<'r, RT> { } /// Get value of a storage key. - pub fn get_storage(&mut self, key: U256) -> Result, StatusCode> { - let mut key_bytes = [0u8; 32]; - key.to_big_endian(&mut key_bytes); - - Ok(self.slots.get(&key).map_err(|e| StatusCode::InternalError(e.to_string()))?.cloned()) + pub fn get_storage(&mut self, key: U256) -> Result { + Ok(self + .slots + .get(&key) + .map_err(|e| StatusCode::InternalError(e.to_string()))? + .cloned() + .unwrap_or_default()) } /// Set value of a storage key. - pub fn set_storage( - &mut self, - key: U256, - value: Option, - ) -> Result { - self.saved_state_root = None; // dirty. - - let mut key_bytes = [0u8; 32]; - key.to_big_endian(&mut key_bytes); - - let prev_value = - self.slots.get(&key).map_err(|e| StatusCode::InternalError(e.to_string()))?.cloned(); - - let mut storage_status = - if prev_value == value { StorageStatus::Unchanged } else { StorageStatus::Modified }; - - if value.is_none() { - self.slots.delete(&key).map_err(|e| StatusCode::InternalError(e.to_string()))?; - storage_status = StorageStatus::Deleted; + pub fn set_storage(&mut self, key: U256, value: U256) -> Result<(), StatusCode> { + let changed = if value.is_zero() { + self.slots.delete(&key).map(|v| v.is_some()) } else { - self.slots - .set(key, value.unwrap()) - .map_err(|e| StatusCode::InternalError(e.to_string()))?; + self.slots.set(key, value).map(|v| v != Some(value)) } + .map_err(|e| StatusCode::InternalError(e.to_string()))?; - Ok(storage_status) + if changed { + self.saved_state_root = None; // dirty. + }; + Ok(()) } /// Resolve the address to the ethereum equivalent, if possible. diff --git a/actors/evm/src/lib.rs b/actors/evm/src/lib.rs index e838e1fc2..d47a4bff7 100644 --- a/actors/evm/src/lib.rs +++ b/actors/evm/src/lib.rs @@ -1,6 +1,6 @@ use std::iter; -use fil_actors_runtime::{runtime::builtins::Type, EAM_ACTOR_ID}; +use fil_actors_runtime::{runtime::builtins::Type, AsActorError, EAM_ACTOR_ID}; use fvm_ipld_encoding::{strict_bytes, BytesDe, BytesSer}; use fvm_shared::address::{Address, Payload}; use interpreter::{address::EthAddress, system::load_bytecode}; @@ -233,8 +233,7 @@ impl EvmContractActor { System::load(rt, true)? .get_storage(params.storage_key) - .map_err(|st| ActorError::unspecified(format!("failed to get storage key: {}", &st)))? - .ok_or_else(|| ActorError::not_found(String::from("storage key not found"))) + .context_code(ExitCode::USR_ASSERTION_FAILED, "failed to get storage key") } } diff --git a/actors/evm/tests/basic.rs b/actors/evm/tests/basic.rs index f09bf159b..5dc0766e4 100644 --- a/actors/evm/tests/basic.rs +++ b/actors/evm/tests/basic.rs @@ -4,7 +4,6 @@ use cid::Cid; use evm::interpreter::U256; use fil_actor_evm as evm; use fil_actors_runtime::test_utils::*; -use fil_actors_runtime::ActorError; use fvm_ipld_blockstore::Blockstore; use fvm_ipld_encoding::RawBytes; use fvm_shared::address::Address; @@ -129,16 +128,20 @@ sstore"; assert_eq!(U256::from(0xfffa), value); // - // Get a storage key that doesn't exist. + // Get a storage key that doesn't exist, should default to zero. // let params = evm::GetStorageAtParams { storage_key: 0xaaaa.into() }; rt.expect_validate_caller_addr(vec![sender]); - let ret = rt.call::( - evm::Method::GetStorageAt as u64, - &RawBytes::serialize(params).unwrap(), - ); - rt.verify(); + let value: U256 = rt + .call::( + evm::Method::GetStorageAt as u64, + &RawBytes::serialize(params).unwrap(), + ) + .unwrap() + .deserialize() + .unwrap(); - assert_eq!(ActorError::not_found("storage key not found".to_string()), ret.err().unwrap()); + assert_eq!(U256::from(0), value); + rt.verify(); }