EVM: fix EXTCODE_ ops (#870)

* wip fixes

* cleanup implementation

* return 0 for non existient address in extcodesize

* use enum instead of result result maddness

* fmt & cleanup some logic

* update tests for the remaining two ext opcodes

* redo logic to make things a bit more clearn

* fix bugs & keep bytecode logic seperate, verify expectations before asserting for better debugging help

* add another test case for ext op

* use closure for early return

* nit: reduce indentation

* fix: formatting

* remove unnecessary closure

* fix: make delegatecall behave more like the EVM

- address todo
- use the new get_cid_type

* fix some review nits

Co-authored-by: Steven Allen <steven@stebalien.com>

Author: 2 people

actors/evm/src/interpreter/instructions/call.rs

@@ -5,6 +5,8 @@ use fvm_shared::{address::Address, METHOD_SEND};
 
 use crate::interpreter::precompiles::PrecompileContext;
 
+use super::ext::{get_cid_type, get_evm_bytecode_cid, ContractType};
+
 use {
     super::memory::{copy_to_memory, get_memory_region},
     crate::interpreter::address::EthAddress,
@@ -255,22 +257,28 @@ pub fn call_generic<RT: Runtime>(
                         system.send_with_gas(&dst_addr, method, params, value, gas_limit, read_only)
                     }
                 }
-                CallKind::DelegateCall => {
-                    // first invoke GetBytecode to get the code CID from the target
-                    let code = crate::interpreter::instructions::ext::get_evm_bytecode_cid(
-                        system.rt, dst,
-                    )?;
-
-                    // and then invoke self with delegate; readonly context is sticky
-                    let params = DelegateCallParams { code, input: input_data.to_vec() };
-                    system.send(
-                        &system.rt.message().receiver(),
-                        Method::InvokeContractDelegate as u64,
-                        RawBytes::serialize(&params)?,
-                        TokenAmount::from(&value),
-                    )
-                }
+                CallKind::DelegateCall => match get_cid_type(system.rt, dst) {
+                    ContractType::EVM(dst_addr) => {
+                        // If we're calling an actual EVM actor, get it's code.
+                        let code = get_evm_bytecode_cid(system.rt, &dst_addr)?;
 
+                        // and then invoke self with delegate; readonly context is sticky
+                        let params = DelegateCallParams { code, input: input_data.into() };
+                        system.send(
+                            &system.rt.message().receiver(),
+                            Method::InvokeContractDelegate as u64,
+                            RawBytes::serialize(&params)?,
+                            TokenAmount::from(&value),
+                        )
+                    }
+                    // If we're calling an account or a non-existent actor, return nothing because
+                    // this is how the EVM behaves.
+                    ContractType::Account | ContractType::NotFound => Ok(RawBytes::default()),
+                    // If we're calling a "native" actor, always revert.
+                    ContractType::Native(_) => {
+                        Err(ActorError::forbidden("cannot delegate-call to native actors".into()))
+                    }
+                },
                 CallKind::CallCode => Err(ActorError::unchecked(
                     EVM_CONTRACT_EXECUTION_ERROR,
                     "unsupported opcode".to_string(),

actors/evm/src/interpreter/instructions/ext.rs

@@ -12,18 +12,22 @@ use {
     fil_actors_runtime::runtime::Runtime,
 };
 
-#[inline]
 pub fn extcodesize(
     _state: &mut ExecutionState,
     system: &System<impl Runtime>,
     addr: U256,
 ) -> Result<U256, StatusCode> {
-    // TODO we're fetching the entire block here just to get its size. We should instead use
+    // TODO (M2.2) we're fetching the entire block here just to get its size. We should instead use
     //  the ipld::block_stat syscall, but the Runtime nor the Blockstore expose it.
     //  Tracked in https://github.com/filecoin-project/ref-fvm/issues/867
-    let len = get_evm_bytecode_cid(system.rt, addr)
-        .and_then(|cid| get_evm_bytecode(system.rt, &cid))
-        .map(|bytecode| bytecode.len())?;
+    let len = match get_cid_type(system.rt, addr) {
+        ContractType::EVM(addr) => {
+            get_evm_bytecode(system.rt, &addr).map(|bytecode| bytecode.len())?
+        }
+        ContractType::Native(_) => 1,
+        // account and not found are flattened to 0 size
+        _ => 0,
+    };
 
     Ok(len.into())
 }
@@ -33,8 +37,16 @@ pub fn extcodehash(
     system: &System<impl Runtime>,
     addr: U256,
 ) -> Result<U256, StatusCode> {
-    let cid = get_evm_bytecode_cid(system.rt, addr)?;
-    let digest = cid.hash().digest();
+    let addr = match get_cid_type(system.rt, addr) {
+        ContractType::EVM(a) => Ok(a),
+        _ => Err(StatusCode::InvalidArgument(
+            "Cannot invoke EXTCODEHASH for non-EVM actor.".to_string(),
+        )),
+    }?;
+    let bytecode_cid = get_evm_bytecode_cid(system.rt, &addr)?;
+
+    let digest = bytecode_cid.hash().digest();
+
     // Take the first 32 bytes of the Multihash
     let digest_len = digest.len().min(32);
     Ok(digest[..digest_len].into())
@@ -48,40 +60,55 @@ pub fn extcodecopy(
     data_offset: U256,
     size: U256,
 ) -> Result<(), StatusCode> {
-    let bytecode =
-        get_evm_bytecode_cid(system.rt, addr).and_then(|cid| get_evm_bytecode(system.rt, &cid))?;
+    let bytecode = match get_cid_type(system.rt, addr) {
+        ContractType::EVM(addr) => get_evm_bytecode(system.rt, &addr)?,
+        ContractType::NotFound | ContractType::Account => Vec::new(),
+        // calling EXTCODECOPY on native actors results with a single byte 0xFE which solidtiy uses for its `assert`/`throw` methods
+        // and in general invalid EVM bytecode
+        _ => vec![0xFE],
+    };
 
     copy_to_memory(&mut state.memory, dest_offset, size, data_offset, bytecode.as_slice(), true)
 }
 
-pub fn get_evm_bytecode_cid(rt: &impl Runtime, addr: U256) -> Result<Cid, StatusCode> {
-    let addr: EthAddress = addr.into();
-    let addr: Address = addr.try_into()?;
-    // TODO: just return none in most of these cases?
-    let actor_id = rt.resolve_address(&addr).ok_or_else(|| {
-        StatusCode::InvalidArgument("failed to resolve address".to_string())
-        // TODO better error code
-    })?;
+#[derive(Debug)]
+pub enum ContractType {
+    /// EVM Address and the CID of the actor (not the bytecode)
+    EVM(Address),
+    Native(Cid),
+    Account,
+    NotFound,
+}
 
-    let evm_cid = rt.get_code_cid_for_type(Type::EVM);
-    let target_cid = rt.get_actor_code_cid(&actor_id);
+/// Resolves an address to the address type
+pub fn get_cid_type(rt: &impl Runtime, addr: U256) -> ContractType {
+    let addr: EthAddress = addr.into();
 
-    if Some(evm_cid) != target_cid {
-        return Err(StatusCode::InvalidArgument(
-            "cannot invoke EXTCODESIZE for non-EVM actor".to_string(),
-        )); // TODO better error code
-    }
+    addr.try_into()
+        .ok() // into filecoin address
+        .and_then(|addr| rt.resolve_address(&addr)) // resolve actor id
+        .and_then(|id| rt.get_actor_code_cid(&id).map(|cid| (id, cid))) // resolve code cid
+        .map(|(id, cid)| match rt.resolve_builtin_actor_type(&cid) {
+            // TODO part of current account abstraction hack where embryos are accounts
+            Some(Type::Account | Type::Embryo) => ContractType::Account,
+            Some(Type::EVM) => ContractType::EVM(Address::new_id(id)),
+            // remaining builtin actors are native
+            _ => ContractType::Native(cid),
+        })
+        .unwrap_or(ContractType::NotFound)
+}
 
-    let cid = rt
-        .send(&addr, crate::Method::GetBytecode as u64, Default::default(), TokenAmount::zero())?
-        .deserialize()?;
-    Ok(cid)
+pub fn get_evm_bytecode_cid(rt: &impl Runtime, addr: &Address) -> Result<Cid, ActorError> {
+    Ok(rt
+        .send(addr, crate::Method::GetBytecode as u64, Default::default(), TokenAmount::zero())?
+        .deserialize()?)
 }
 
-pub fn get_evm_bytecode(rt: &impl Runtime, cid: &Cid) -> Result<Vec<u8>, StatusCode> {
+pub fn get_evm_bytecode(rt: &impl Runtime, addr: &Address) -> Result<Vec<u8>, StatusCode> {
+    let cid = get_evm_bytecode_cid(rt, addr)?;
     let raw_bytecode = rt
         .store()
-        .get(cid) // TODO this is inefficient; should call stat here.
+        .get(&cid) // TODO this is inefficient; should call stat here.
         .map_err(|e| StatusCode::InternalError(format!("failed to get bytecode block: {}", &e)))?
         .ok_or_else(|| ActorError::not_found("bytecode block not found".to_string()))?;
     Ok(raw_bytecode)

actors/evm/tests/asm.rs

@@ -23,6 +23,15 @@ const PRELUDE: &str = r#"
   dup1
   revert
 %end
+
+%macro return_stack_word()
+    # store at 0x00
+    push1 0x00
+    mstore
+    push1 0x20 # always return a full word
+    push1 0x00
+    return
+%end
 "#;
 
 #[allow(dead_code)]

actors/evm/tests/create.rs

@@ -16,16 +16,6 @@ mod util;
 pub fn magic_precompile_contract() -> Vec<u8> {
     let init = "";
     let body = r#"
-
-
-%macro return_stack_word()
-    push1 0x00
-    mstore
-    push1 0x20 # always return a full word
-    push1 0x00
-    return
-%end
-
 # magic value, used as initcode
 push16 0x666F6F206261722062617A20626F7879 # foo bar baz boxy
 push2 0x0100 # offset of input data