EVM: Implement reverts correctly (#864)

* add exit to runtime interface

* propagate revert data in contract invocations

* add data to ActorError

* propagate data in actor errors from send

* correctly handle reverts in calls

* implement exit with panics in mock runtime

* implement exit with panics in test vm

* fix clippy

* fix test vm: exit data should be in new context

* use pop instead of ugly gets in ret

* cosmetics

* add naked revert test

* rustfmt

* only test naked revert in unit test

nested call revert should go in integration test, really

* fix callvariants contract to check reverts

* really fix callvariants contract: iszero is what you want

* test_vm: missing panic handler

* callvariants is also testing mutation

* deduplicate actor exit handler code

* rustfmt

* remove unnecessary replaces for exits.

* fix runtime trampoline to propagate data from errors

* perform contract/constructor invocation abortive returns with errors

* don't synethesize exit data if it is not there

* propagate error data in test_vm

* rustfmt

* simplify/dedup identical code

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

@@ -14,9 +14,10 @@ use {
     crate::interpreter::System,
     crate::interpreter::U256,
     crate::RawBytes,
-    crate::{DelegateCallParams, Method, EVM_CONTRACT_REVERTED},
+    crate::{DelegateCallParams, Method, EVM_CONTRACT_EXECUTION_ERROR, EVM_CONTRACT_REVERTED},
     fil_actors_runtime::runtime::builtins::Type,
     fil_actors_runtime::runtime::Runtime,
+    fil_actors_runtime::ActorError,
     fvm_shared::econ::TokenAmount,
 };
 
@@ -113,7 +114,7 @@ pub fn call<RT: Runtime>(
     let input_region = get_memory_region(memory, input_offset, input_size)
         .map_err(|_| StatusCode::InvalidMemoryAccess)?;
 
-    state.return_data = {
+    let (call_result, return_data) = {
         // ref to memory is dropped after calling so we can mutate it on output later
         let input_data = if let Some(MemoryRegion { offset, size }) = input_region {
             &memory[offset..][..size.get()]
@@ -128,9 +129,15 @@ pub fn call<RT: Runtime>(
                 value,
             };
 
-            // TODO: DO NOT FAIL!!!
-            precompiles::Precompiles::call_precompile(system.rt, dst, input_data, context)
-                .map_err(StatusCode::from)?
+            match precompiles::Precompiles::call_precompile(system.rt, dst, input_data, context)
+                .map_err(StatusCode::from)
+            {
+                Ok(return_data) => (1, return_data),
+                Err(status) => {
+                    let msg = format!("{}", status);
+                    (0, msg.as_bytes().to_vec())
+                }
+            }
         } else {
             let call_result = match kind {
                 CallKind::Call | CallKind::StaticCall => {
@@ -199,41 +206,34 @@ pub fn call<RT: Runtime>(
                     )
                 }
 
-                CallKind::CallCode => {
-                    todo!()
-                }
+                CallKind::CallCode => Err(ActorError::unchecked(
+                    EVM_CONTRACT_EXECUTION_ERROR,
+                    "unsupported opcode".to_string(),
+                )),
             };
             match call_result {
                 Ok(result) => {
                     // Support the "empty" result. We often use this to mean "returned nothing" and
                     // it's important to support, e.g., sending to accounts.
                     if result.is_empty() {
-                        Vec::new()
+                        (1, Vec::new())
                     } else {
                         // TODO: support IPLD codecs #758
                         let BytesDe(result) = result.deserialize()?;
-                        result
+                        (1, result)
                     }
                 }
-                Err(ae) => {
-                    return if ae.exit_code() == EVM_CONTRACT_REVERTED {
-                        // reverted -- we don't have return data yet
-                        // push failure
-                        stack.push(U256::zero());
-                        Ok(())
-                    } else {
-                        Err(StatusCode::from(ae))
-                    };
-                }
+                Err(ae) => (0, ae.data().to_vec()),
             }
         }
-    }
-    .into();
+    };
+
+    state.return_data = return_data.into();
 
     // copy return data to output region if it is non-zero
     copy_to_memory(memory, output_offset, output_size, U256::zero(), &state.return_data, false)?;
 
-    stack.push(U256::from(1));
+    stack.push(U256::from(call_result));
     Ok(())
 }
 

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

@@ -6,8 +6,8 @@ use {
 
 #[inline]
 pub fn ret(state: &mut ExecutionState) -> Result<(), StatusCode> {
-    let offset = *state.stack.get(0);
-    let size = *state.stack.get(1);
+    let offset = state.stack.pop();
+    let size = state.stack.pop();
 
     if let Some(region) = super::memory::get_memory_region(&mut state.memory, offset, size)
         .map_err(|_| StatusCode::InvalidMemoryAccess)?

actors/evm/src/lib.rs

@@ -29,7 +29,8 @@ use {
 #[cfg(feature = "fil-actor")]
 fil_actors_runtime::wasm_trampoline!(EvmContractActor);
 
-pub const EVM_CONTRACT_REVERTED: ExitCode = ExitCode::new(27);
+pub const EVM_CONTRACT_REVERTED: ExitCode = ExitCode::new(33);
+pub const EVM_CONTRACT_EXECUTION_ERROR: ExitCode = ExitCode::new(34);
 
 const EVM_MAX_RESERVED_METHOD: u64 = 1023;
 pub const NATIVE_METHOD_SIGNATURE: &str = "handle_filecoin_method(uint64,uint64,bytes)";
@@ -118,14 +119,21 @@ impl EvmContractActor {
 
         // TODO this does not return revert data yet, but it has correct semantics.
         if exec_status.reverted {
-            Err(ActorError::unchecked(EVM_CONTRACT_REVERTED, "constructor reverted".to_string()))
+            Err(ActorError::unchecked_with_data(
+                EVM_CONTRACT_REVERTED,
+                "constructor reverted".to_string(),
+                RawBytes::from(exec_status.output_data.to_vec()),
+            ))
         } else if exec_status.status_code == StatusCode::Success {
             system.set_bytecode(&exec_status.output_data)?;
             system.flush()
         } else if let StatusCode::ActorError(e) = exec_status.status_code {
             Err(e)
         } else {
-            Err(ActorError::unspecified("EVM constructor failed".to_string()))
+            Err(ActorError::unchecked(
+                EVM_CONTRACT_EXECUTION_ERROR,
+                format!("constructor execution error: {}", exec_status.status_code),
+            ))
         }
     }
 
@@ -175,21 +183,21 @@ impl EvmContractActor {
                 _ => ActorError::unspecified(format!("EVM execution error: {e:?}")),
             })?;
 
-        // TODO this does not return revert data yet, but it has correct semantics.
         if exec_status.reverted {
-            return Err(ActorError::unchecked(
+            return Err(ActorError::unchecked_with_data(
                 EVM_CONTRACT_REVERTED,
                 "contract reverted".to_string(),
+                RawBytes::from(exec_status.output_data.to_vec()),
             ));
         } else if exec_status.status_code == StatusCode::Success {
             system.flush()?;
         } else if let StatusCode::ActorError(e) = exec_status.status_code {
             return Err(e);
         } else {
-            return Err(ActorError::unspecified(format!(
-                "EVM contract invocation failed: status: {}",
-                exec_status.status_code
-            )));
+            return Err(ActorError::unchecked(
+                EVM_CONTRACT_EXECUTION_ERROR,
+                format!("contract execution error: {}", exec_status.status_code),
+            ));
         }
 
         if let Some(addr) = exec_status.selfdestroyed {

actors/evm/tests/contracts/callvariants.hex

@@ -1 +1 @@
-306000556101cf8060106000396000f360003560e01c80600114607557806002146101af5780600314609157806004146101bb578060051460ad578060061460cf578060071460ed578060081461010f578060091461012d5780600a146101495780600b1461016b5780600c1461012d5780600d1461018d5780600e1461014957600080fd5b60206000600260e01b600052600460006004356000fa60206000f35b60206000600460e01b600052600460006004356000fa60206000f35b60206000600660e01b600052602435600452602460006004356000fa60206000f35b60206000600260e01b6000526004600060006004356000f160206000f35b60206000600860e01b600052602435600452602460006004356000fa60206000f35b60206000600460e01b6000526004600060006004356000f160206000f35b60206000600260e01b600052600460006004356000f460206000f35b60206000600460e01b600052600460006004356000f460005460005260206000f35b60206000600c60e01b600052602435600452602460006004356000fa60206000f35b60206000600e60e01b600052602435600452602460006004356000fa60206000f35b60005460005260206000f35b63ffffff4260005560005460005260206000f3
+306000556102108060106000396000f360003560e01c80600114607657806002146101e25780600314609757806004146101ee578060051460b8578060061460df57806007146101025780600814610129578060091461014c5780600a1461016d5780600b146101945780600c1461014c5780600d146101bb5780600e1461016d57600080fd5b60206000600260e01b600052600460006004356000fa156102025760206000f35b60206000600460e01b600052600460006004356000fa156102025760206000f35b60206000600660e01b600052602435600452602460006004356000fa156102025760206000f35b60206000600260e01b6000526004600060006004356000f1156102025760206000f35b60206000600860e01b600052602435600452602460006004356000fa156102025760206000f35b60206000600460e01b6000526004600060006004356000f1156102025760206000f35b60206000600260e01b600052600460006004356000f4156102025760206000f35b60206000600460e01b600052600460006004356000f4156102025760005460005260206000f35b60206000600c60e01b600052602435600452602460006004356000fa156102025760206000f35b60206000600e60e01b600052602435600452602460006004356000fa156102025760206000f35b60005460005260206000f35b63ffffff4260005560005460005260206000f35b63deadbeef6000526004601cfd

actors/evm/tests/contracts/callvariants_body.eas

@@ -22,6 +22,12 @@
   revert
 %end
 
+%macro check()
+  iszero
+  %push(call_failed)
+  jumpi
+%end
+
 # method dispatch
 %dispatch_begin()
 
@@ -77,6 +83,7 @@ push1 0x04    # input (dest) offset
 calldataload  # staticcall dest
 push1 0x00    # staticcall gas (ignored)
 staticcall    # do it!
+%check()
 push1 0x20    # ret length
 push1 0x00    # ret offset
 return
@@ -97,6 +104,7 @@ push1 0x04    # input (dest) offset
 calldataload  # staticcall dest
 push1 0x00    # staticcall gas (ignored)
 staticcall    # do it!
+%check()
 push1 0x20    # ret length
 push1 0x00    # ret offset
 return
@@ -121,6 +129,7 @@ push1 0x04    # input (dest) offset
 calldataload  # staticcall dest
 push1 0x00    # staticcall gas (ignored)
 staticcall    # do it!
+%check()
 push1 0x20    # ret length
 push1 0x00    # ret offset
 return
@@ -142,6 +151,7 @@ push1 0x04    # input (dest) offset
 calldataload  # call dest
 push1 0x00    # call gas (ignored)
 call          # do it!
+%check()
 push1 0x20    # ret length
 push1 0x00    # ret offset
 return
@@ -166,6 +176,7 @@ push1 0x04    # input (dest) offset
 calldataload  # staticcall dest
 push1 0x00    # staticcall gas (ignored)
 staticcall    # do it!
+%check()
 push1 0x20    # ret length
 push1 0x00    # ret offset
 return
@@ -187,6 +198,7 @@ push1 0x04    # input (dest) offset
 calldataload  # icall dest
 push1 0x00    # call gas (ignored)
 call          # do it!
+%check()
 push1 0x20    # ret length
 push1 0x00    # ret offset
 return
@@ -207,6 +219,7 @@ push1 0x04    # input (dest) offset
 calldataload  # delegate dest
 push1 0x00    # delegate gas (ignored)
 delegatecall  # do it!
+%check()
 push1 0x20    # ret length
 push1 0x00    # ret offset
 return
@@ -227,6 +240,7 @@ push1 0x04    # input (dest) offset
 calldataload  # delegate dest
 push1 0x00    # delegate gas (ignored)
 delegatecall  # do it!
+%check()
 push1 0x00    # slot
 sload         # read
 push1 0x00    # ret offset
@@ -255,6 +269,7 @@ push1 0x04    # input (dest) offset
 calldataload  # staticcall dest
 push1 0x00    # staticcall gas (ignored)
 staticcall    # do it!
+%check()
 push1 0x20    # ret length
 push1 0x00    # ret offset
 return
@@ -279,6 +294,7 @@ push1 0x04    # input (dest) offset
 calldataload  # staticcall dest
 push1 0x00    # staticcall gas (ignored)
 staticcall    # do it!
+%check()
 push1 0x20    # ret length
 push1 0x00    # ret offset
 return
@@ -310,3 +326,13 @@ mstore
 push1 0x20
 push1 0x00
 return
+
+# call_failed
+call_failed:
+jumpdest
+%push(0xdeadbeef)
+push1 0x00
+mstore
+push1 0x04
+push1 0x1c
+revert

actors/evm/tests/revert.rs

@@ -0,0 +1,34 @@
+use fil_actor_evm as evm;
+use fvm_ipld_encoding::{BytesSer, RawBytes};
+
+mod asm;
+mod util;
+
+#[test]
+fn test_revert() {
+    let contract = asm::new_contract(
+        "naked-revert",
+        "",
+        r#"
+%push(0xdeadbeef)
+push1 0x00
+mstore
+push1 0x04
+push1 0x1c # skip top 28 bytes
+revert
+"#,
+    )
+    .unwrap();
+
+    let mut rt = util::construct_and_verify(contract);
+    rt.expect_validate_caller_any();
+
+    let result = rt.call::<evm::EvmContractActor>(
+        evm::Method::InvokeContract as u64,
+        &RawBytes::serialize(BytesSer(&[])).unwrap(),
+    );
+    assert!(result.is_err());
+    let e = result.unwrap_err();
+    assert_eq!(e.exit_code(), evm::EVM_CONTRACT_REVERTED);
+    assert_eq!(e.data(), RawBytes::from(vec![0xde, 0xad, 0xbe, 0xef]));
+}