refactor: VM::run returns an enum instead of tuple (near#6571)

This refactor primarily tries to make the `VM::run` signature clearer.
It is also a preparation step towards changing where contract loading
gas costs are charged.

Co-authored-by: Simonas Kazlauskas <[email protected]>

runtime/near-vm-runner-standalone/src/main.rs

@@ -7,6 +7,7 @@ use clap::Parser;
 use near_vm_logic::ProtocolVersion;
 use near_vm_logic::VMOutcome;
 use near_vm_runner::internal::VMKind;
+use near_vm_runner::VMResult;
 use serde::{
     de::{MapAccess, Visitor},
     ser::SerializeMap,
@@ -105,7 +106,7 @@ struct CliArgs {
 #[allow(unused)]
 #[derive(Debug, Clone)]
 struct StandaloneOutput {
-    pub outcome: Option<VMOutcome>,
+    pub outcome: VMOutcome,
     pub err: Option<String>,
     pub state: State,
 }
@@ -163,28 +164,26 @@ fn main() {
     step.promise_results(promise_results);
 
     let mut results = script.run();
-    let (outcome, err) = results.outcomes.pop().unwrap();
-
-    println!(
-        "{:#?}",
-        StandaloneOutput {
-            outcome: outcome.clone(),
-            err: err.map(|it| it.to_string()),
-            state: State(results.state.fake_trie),
-        }
-    );
-
-    if let Some(outcome) = &outcome {
-        println!("\nLogs:");
-        for log in &outcome.logs {
-            println!("{}\n", log);
-        }
-    }
-
-    match &outcome {
-        Some(outcome) => {
+    let last_result = results.outcomes.pop().unwrap();
+    let maybe_error = last_result.error();
+
+    match &last_result {
+        VMResult::NotRun(_) => {}
+        VMResult::Aborted(outcome, _) | VMResult::Ok(outcome) => {
+            println!(
+                "{:#?}",
+                StandaloneOutput {
+                    outcome: outcome.clone(),
+                    err: maybe_error.map(|it| it.to_string()),
+                    state: State(results.state.fake_trie),
+                }
+            );
+
+            println!("\nLogs:");
+            for log in &outcome.logs {
+                println!("{}\n", log);
+            }
             println!("{:#?}", outcome.profile);
         }
-        _ => {}
     }
 }

runtime/near-vm-runner-standalone/src/script.rs

@@ -7,9 +7,9 @@ use near_primitives::types::CompiledContractCache;
 use near_primitives::version::PROTOCOL_VERSION;
 use near_vm_logic::mocks::mock_external::MockedExternal;
 use near_vm_logic::types::PromiseResult;
-use near_vm_logic::{ProtocolVersion, VMConfig, VMContext, VMOutcome};
+use near_vm_logic::{ProtocolVersion, VMConfig, VMContext};
 use near_vm_runner::internal::VMKind;
-use near_vm_runner::{MockCompiledContractCache, VMError};
+use near_vm_runner::{MockCompiledContractCache, VMResult};
 
 use crate::State;
 
@@ -37,7 +37,7 @@ pub struct Step {
 }
 
 pub struct ScriptResults {
-    pub outcomes: Vec<(Option<VMOutcome>, Option<VMError>)>,
+    pub outcomes: Vec<VMResult>,
     pub state: MockedExternal,
 }
 
@@ -216,10 +216,10 @@ fn vm_script_smoke_test() {
 
     assert_eq!(res.outcomes.len(), 4);
 
-    let logs = &res.outcomes[0].0.as_ref().unwrap().logs;
+    let logs = &res.outcomes[0].outcome().unwrap().logs;
     assert_eq!(logs, &vec!["hello".to_string()]);
 
-    let ret = res.outcomes.last().unwrap().0.as_ref().unwrap().return_data.clone();
+    let ret = res.outcomes.last().unwrap().outcome().unwrap().return_data.clone();
 
     let expected = ReturnData::Value(4950u64.to_le_bytes().to_vec());
     assert_eq!(ret, expected);
@@ -238,12 +238,12 @@ fn profile_data_is_per_outcome() {
     let res = script.run();
     assert_eq!(res.outcomes.len(), 4);
     assert_eq!(
-        res.outcomes[1].0.as_ref().unwrap().profile.host_gas(),
-        res.outcomes[2].0.as_ref().unwrap().profile.host_gas()
+        res.outcomes[1].outcome().unwrap().profile.host_gas(),
+        res.outcomes[2].outcome().unwrap().profile.host_gas()
     );
     assert!(
-        res.outcomes[1].0.as_ref().unwrap().profile.host_gas()
-            > res.outcomes[3].0.as_ref().unwrap().profile.host_gas()
+        res.outcomes[1].outcome().unwrap().profile.host_gas()
+            > res.outcomes[3].outcome().unwrap().profile.host_gas()
     );
 }
 
@@ -268,8 +268,8 @@ fn test_evm_slow_deserialize_repro() {
 
         script.step(contract, "deploy_code").input(input).repeat(3);
         let res = script.run();
-        assert_eq!(res.outcomes[0].1, None);
-        assert_eq!(res.outcomes[1].1, None);
+        assert_eq!(res.outcomes[0].error(), None);
+        assert_eq!(res.outcomes[1].error(), None);
     }
 
     evm_slow_deserialize_repro(VMKind::Wasmer0);

runtime/near-vm-runner/fuzz/fuzz_targets/runner.rs

@@ -6,17 +6,17 @@ use near_primitives::contract::ContractCode;
 use near_primitives::runtime::fees::RuntimeFeesConfig;
 use near_primitives::version::PROTOCOL_VERSION;
 use near_vm_logic::mocks::mock_external::MockedExternal;
-use near_vm_logic::{VMConfig, VMContext, VMOutcome};
+use near_vm_logic::{VMConfig, VMContext};
 use near_vm_runner::internal::wasmparser::{Export, ExternalKind, Parser, Payload, TypeDef};
 use near_vm_runner::internal::VMKind;
-use near_vm_runner::VMError;
+use near_vm_runner::VMResult;
 
 libfuzzer_sys::fuzz_target!(|module: ArbitraryModule| {
     let code = ContractCode::new(module.0.to_bytes(), None);
-    let (_outcome, _err) = run_fuzz(&code, VMKind::for_protocol_version(PROTOCOL_VERSION));
+    let _result = run_fuzz(&code, VMKind::for_protocol_version(PROTOCOL_VERSION));
 });
 
-fn run_fuzz(code: &ContractCode, vm_kind: VMKind) -> (Option<VMOutcome>, Option<VMError>) {
+fn run_fuzz(code: &ContractCode, vm_kind: VMKind) -> VMResult {
     let mut fake_external = MockedExternal::new();
     let mut context = create_context(vec![]);
     context.prepaid_gas = 10u64.pow(14);

runtime/near-vm-runner/src/lib.rs

@@ -27,7 +27,7 @@ pub use cache::{
 };
 #[cfg(target_arch = "x86_64")]
 pub use preload::{ContractCallPrepareRequest, ContractCallPrepareResult, ContractCaller};
-pub use runner::{run, VM};
+pub use runner::{run, VMResult, VM};
 
 /// This is public for internal experimentation use only, and should otherwise be considered an
 /// implementation detail of `near-vm-runner`.

runtime/near-vm-runner/src/preload.rs

@@ -8,10 +8,11 @@ use near_primitives::runtime::fees::RuntimeFeesConfig;
 use near_primitives::types::CompiledContractCache;
 use near_vm_errors::VMError;
 use near_vm_logic::types::PromiseResult;
-use near_vm_logic::{External, ProtocolVersion, VMConfig, VMContext, VMOutcome};
+use near_vm_logic::{External, ProtocolVersion, VMConfig, VMContext};
 
 use crate::cache::{self, into_vm_result};
 use crate::vm_kind::VMKind;
+use crate::VMResult;
 
 const SHARE_MEMORY_INSTANCE: bool = false;
 
@@ -125,12 +126,12 @@ impl ContractCaller {
         fees_config: &'a RuntimeFeesConfig,
         promise_results: &'a [PromiseResult],
         current_protocol_version: ProtocolVersion,
-    ) -> (Option<VMOutcome>, Option<VMError>) {
+    ) -> VMResult {
         match self.preloaded.get(preloaded.handle) {
             Some(call) => {
                 let call_data = call.rx.recv().unwrap();
                 match call_data.result {
-                    Err(err) => (None, Some(err)),
+                    Err(err) => VMResult::NotRun(err),
                     Ok(module) => match (&module, &mut self.vm_data_private) {
                         #[cfg(feature = "wasmer0_vm")]
                         (VMModule::Wasmer0(module), VMDataPrivate::Wasmer0(memory)) => {

runtime/near-vm-runner/src/runner.rs

@@ -6,7 +6,7 @@ use near_primitives::types::CompiledContractCache;
 use near_primitives::version::ProtocolVersion;
 use near_vm_errors::VMError;
 use near_vm_logic::types::PromiseResult;
-use near_vm_logic::{External, VMContext, VMOutcome};
+use near_vm_logic::{External, VMContext, VMLogic, VMOutcome};
 
 use crate::vm_kind::VMKind;
 
@@ -33,7 +33,7 @@ pub fn run(
     promise_results: &[PromiseResult],
     current_protocol_version: ProtocolVersion,
     cache: Option<&dyn CompiledContractCache>,
-) -> (Option<VMOutcome>, Option<VMError>) {
+) -> VMResult {
     let vm_kind = VMKind::for_protocol_version(current_protocol_version);
     if let Some(runtime) = vm_kind.runtime(wasm_config.clone()) {
         runtime.run(
@@ -72,7 +72,7 @@ pub trait VM {
         promise_results: &[PromiseResult],
         current_protocol_version: ProtocolVersion,
         cache: Option<&dyn CompiledContractCache>,
-    ) -> (Option<VMOutcome>, Option<VMError>);
+    ) -> VMResult;
 
     /// Precompile a WASM contract to a VM specific format and store the result into the `cache`.
     ///
@@ -108,3 +108,61 @@ impl VMKind {
         }
     }
 }
+
+/// Type returned by any VM instance after loading a contract and executing a
+/// function inside.
+#[derive(Debug, PartialEq)]
+pub enum VMResult {
+    /// Execution was not able to start because preconditions were not met.
+    /// TODO: Remove this with more refactoring or present a good reason why it
+    /// is needed.
+    NotRun(VMError),
+    /// Execution started but hit an error.
+    Aborted(VMOutcome, VMError),
+    /// Execution finished without error.
+    Ok(VMOutcome),
+}
+
+impl VMResult {
+    /// Consumes the `VMLogic` object and computes the final outcome with the
+    /// given error that stopped execution from finishing successfully.
+    pub fn abort(logic: VMLogic, error: VMError) -> VMResult {
+        let outcome = logic.compute_outcome_and_distribute_gas();
+        VMResult::Aborted(outcome, error)
+    }
+
+    /// Consumes the `VMLogic` object and computes the final outcome for a
+    /// successful execution.
+    pub fn ok(logic: VMLogic) -> VMResult {
+        let outcome = logic.compute_outcome_and_distribute_gas();
+        VMResult::Ok(outcome)
+    }
+
+    /// Borrow the internal outcome, if there is one.
+    pub fn outcome(&self) -> Option<&VMOutcome> {
+        match self {
+            VMResult::NotRun(_err) => None,
+            VMResult::Aborted(outcome, _err) => Some(outcome),
+            VMResult::Ok(outcome) => Some(outcome),
+        }
+    }
+
+    /// Borrow the internal error, if there is one.
+    pub fn error(&self) -> Option<&VMError> {
+        match self {
+            VMResult::NotRun(err) => Some(err),
+            VMResult::Aborted(_outcome, err) => Some(err),
+            VMResult::Ok(_outcome) => None,
+        }
+    }
+
+    /// Unpack the internal outcome and error. This method mostly exists for
+    /// easy compatibility with code that was written before `VMResult` existed.
+    pub fn outcome_error(self) -> (Option<VMOutcome>, Option<VMError>) {
+        match self {
+            VMResult::NotRun(err) => (None, Some(err)),
+            VMResult::Aborted(outcome, err) => (Some(outcome), Some(err)),
+            VMResult::Ok(outcome) => (Some(outcome), None),
+        }
+    }
+}

runtime/near-vm-runner/src/tests.rs

@@ -71,16 +71,18 @@ fn make_simple_contract_call_with_gas_vm(
 
     let code = ContractCode::new(code.to_vec(), None);
     let runtime = vm_kind.runtime(config).expect("runtime has not been compiled");
-    runtime.run(
-        &code,
-        method_name,
-        &mut fake_external,
-        context,
-        &fees,
-        &promise_results,
-        LATEST_PROTOCOL_VERSION,
-        None,
-    )
+    runtime
+        .run(
+            &code,
+            method_name,
+            &mut fake_external,
+            context,
+            &fees,
+            &promise_results,
+            LATEST_PROTOCOL_VERSION,
+            None,
+        )
+        .outcome_error()
 }
 
 fn make_simple_contract_call_with_protocol_version_vm(
@@ -99,16 +101,18 @@ fn make_simple_contract_call_with_protocol_version_vm(
 
     let promise_results = vec![];
     let code = ContractCode::new(code.to_vec(), None);
-    runtime.run(
-        &code,
-        method_name,
-        &mut fake_external,
-        context,
-        fees,
-        &promise_results,
-        protocol_version,
-        None,
-    )
+    runtime
+        .run(
+            &code,
+            method_name,
+            &mut fake_external,
+            context,
+            fees,
+            &promise_results,
+            protocol_version,
+            None,
+        )
+        .outcome_error()
 }
 
 fn make_simple_contract_call_vm(

runtime/near-vm-runner/src/tests/cache.rs

@@ -3,6 +3,7 @@
 
 use super::{create_context, with_vm_variants, LATEST_PROTOCOL_VERSION};
 use crate::internal::VMKind;
+use crate::runner::VMResult;
 use crate::wasmer2_runner::Wasmer2VM;
 use crate::{prepare, MockCompiledContractCache};
 use assert_matches::assert_matches;
@@ -12,7 +13,7 @@ use near_primitives::types::CompiledContractCache;
 use near_stable_hasher::StableHasher;
 use near_vm_errors::VMError;
 use near_vm_logic::mocks::mock_external::MockedExternal;
-use near_vm_logic::{VMConfig, VMOutcome};
+use near_vm_logic::VMConfig;
 use std::hash::{Hash, Hasher};
 use std::io;
 use std::sync::atomic::{AtomicBool, Ordering};
@@ -50,16 +51,20 @@ fn test_does_not_cache_io_error() {
         let mut cache = FaultingCompiledContractCache::default();
 
         cache.set_read_fault(true);
-        let (outcome, err) =
-            make_cached_contract_call_vm(&cache, &code, "main", prepaid_gas, vm_kind);
-        assert!(outcome.is_none());
-        assert_matches!(err, Some(VMError::CacheError(near_vm_errors::CacheError::ReadError)));
+        let result = make_cached_contract_call_vm(&cache, &code, "main", prepaid_gas, vm_kind);
+        assert!(result.outcome().is_none());
+        assert_matches!(
+            result.error(),
+            Some(&VMError::CacheError(near_vm_errors::CacheError::ReadError))
+        );
 
         cache.set_write_fault(true);
-        let (outcome, err) =
-            make_cached_contract_call_vm(&cache, &code, "main", prepaid_gas, vm_kind);
-        assert!(outcome.is_none());
-        assert_matches!(err, Some(VMError::CacheError(near_vm_errors::CacheError::WriteError)));
+        let result = make_cached_contract_call_vm(&cache, &code, "main", prepaid_gas, vm_kind);
+        assert!(result.outcome().is_none());
+        assert_matches!(
+            result.error(),
+            Some(&VMError::CacheError(near_vm_errors::CacheError::WriteError))
+        );
     })
 }
 
@@ -69,7 +74,7 @@ fn make_cached_contract_call_vm(
     method_name: &str,
     prepaid_gas: u64,
     vm_kind: VMKind,
-) -> (Option<VMOutcome>, Option<VMError>) {
+) -> VMResult {
     let mut fake_external = MockedExternal::new();
     let mut context = create_context(vec![]);
     let config = VMConfig::test();