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

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(())
 }

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<Option<U256>, 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<U256, StatusCode> {
+        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<U256>,
-    ) -> Result<StorageStatus, StatusCode> {
-        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.

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")
     }
 }
 

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::EvmContractActor>(
-        evm::Method::GetStorageAt as u64,
-        &RawBytes::serialize(params).unwrap(),
-    );
-    rt.verify();
+    let value: U256 = rt
+        .call::<evm::EvmContractActor>(
+            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();
 }