Make some host errors non-recoverable in try_call. (#945)

* Make some host errors non-recoverable in `try_call`.

- Internal errors that indicate host implementation/setup bugs - a good chunk of it indicates severely broken host state and we probably don't want to continue execution if that happens. We also would want to learn about these ASAP
- Running out of budget - while this would be handled 'automatically' during the normal execution, this makes test experience a bit more consistent (as compiled contracts are not metered)
- Accessing entries outside of the footprint - this is probably the most controversial change, but there is really no indication/expectation that storage interface doesn't behave as a key-value database and hence it's pretty easy to write logic that relies on that fact without considering the existence of the footprint. `try_call` recovering from footprint errors is functionally equivalent to `get_contract_data` recovering from footprint errors (imagine a simple storage wrapper contract) and we really don't want allowing that.

* Make generic wasmi errors `InvalidAction`.

This allows reserving internal errors for wasm-related logic errors on the host side.

soroban-env-common/src/error.rs

@@ -161,7 +161,7 @@ impl From<stellar_xdr::Error> for Error {
 impl From<wasmi::core::TrapCode> for Error {
     fn from(code: wasmi::core::TrapCode) -> Self {
         let ec = match code {
-            wasmi::core::TrapCode::UnreachableCodeReached => ScErrorCode::InternalError,
+            wasmi::core::TrapCode::UnreachableCodeReached => ScErrorCode::InvalidAction,
 
             wasmi::core::TrapCode::MemoryOutOfBounds | wasmi::core::TrapCode::TableOutOfBounds => {
                 ScErrorCode::IndexBounds

soroban-env-host/src/host.rs

@@ -701,7 +701,7 @@ impl Host {
             .ok_or_else(|| {
                 self.err(
                     ScErrorType::Auth,
-                    ScErrorCode::InternalError,
+                    ScErrorCode::InvalidAction,
                     "previous invocation is missing - no auth data to get",
                     &[],
                 )
@@ -774,7 +774,7 @@ impl Host {
                 .ok_or_else(|| {
                     self.err(
                         ScErrorType::Auth,
-                        ScErrorCode::InternalError,
+                        ScErrorCode::InvalidAction,
                         "previous invocation is missing - no auth data to get",
                         &[],
                     )
@@ -2372,7 +2372,14 @@ impl VmCallerEnv for Host {
                         &[func.to_val(), args.to_val()],
                     )
                 })?;
-                Ok(e.error.to_val())
+                // Only allow to gracefully handle the recoverable errors.
+                // Non-recoverable errors should still cause guest to panic and
+                // abort execution.
+                if e.is_recoverable() {
+                    Ok(e.error.to_val())
+                } else {
+                    Err(e)
+                }
             }
         }
     }
@@ -2921,7 +2928,7 @@ impl VmCallerEnv for Host {
                 Frame::HostFunction(_) => {
                     return Err(self.err(
                         ScErrorType::Context,
-                        ScErrorCode::InvalidAction,
+                        ScErrorCode::InternalError,
                         "require_auth is not suppported for host fns",
                         &[],
                     ))

soroban-env-host/src/host/error.rs

@@ -109,6 +109,31 @@ impl HostError {
             }
         }
     }
+
+    /// Identifies whether the error can be meaningfully recovered from.
+    ///
+    /// We consider errors that occur due to broken execution preconditions (
+    /// such as incorrect footprint) non-recoverable.
+    pub fn is_recoverable(&self) -> bool {
+        // All internal errors that originate from the host can be considered
+        // non-recoverable (they should only appear if there is some bug in the
+        // host implementation or setup).
+        if !self.error.is_type(ScErrorType::Contract)
+            && self.error.is_code(ScErrorCode::InternalError)
+        {
+            return false;
+        }
+        // Exceeding the budget or storage limit is non-recoverable. Exceeding
+        // storage 'limit' is basically accessing entries outside of the
+        // supplied footprint.
+        if self.error.is_code(ScErrorCode::ExceededLimit)
+            && (self.error.is_type(ScErrorType::Storage) || self.error.is_type(ScErrorType::Budget))
+        {
+            return false;
+        }
+
+        true
+    }
 }
 
 impl<T> From<T> for HostError

soroban-env-host/src/storage.rs

@@ -113,17 +113,23 @@ impl Footprint {
         ty: AccessType,
         budget: &Budget,
     ) -> Result<(), HostError> {
+        // `ExceededLimit` is not the most precise term here, but footprint has
+        // to be externally supplied in a similar fashion to budget and it's
+        // also representing an execution resource limit (number of ledger
+        // entries to access), so it might be considered 'exceeded'.
+        // This also helps distinguish access errors from the values simply
+        // being  missing from storage (but with a valid footprint).
         if let Some(existing) = self.0.get::<Rc<LedgerKey>>(key, budget)? {
             match (existing, ty) {
                 (AccessType::ReadOnly, AccessType::ReadOnly) => Ok(()),
                 (AccessType::ReadOnly, AccessType::ReadWrite) => {
-                    Err((ScErrorType::Storage, ScErrorCode::InvalidAction).into())
+                    Err((ScErrorType::Storage, ScErrorCode::ExceededLimit).into())
                 }
                 (AccessType::ReadWrite, AccessType::ReadOnly) => Ok(()),
                 (AccessType::ReadWrite, AccessType::ReadWrite) => Ok(()),
             }
         } else {
-            Err((ScErrorType::Storage, ScErrorCode::MissingValue).into())
+            Err((ScErrorType::Storage, ScErrorCode::ExceededLimit).into())
         }
     }
 }
@@ -420,7 +426,7 @@ mod test_footprint {
         let res = fp.enforce_access(&key, AccessType::ReadOnly, &budget);
         assert!(HostError::result_matches_err(
             res,
-            (ScErrorType::Storage, ScErrorCode::MissingValue)
+            (ScErrorType::Storage, ScErrorCode::ExceededLimit)
         ));
         Ok(())
     }
@@ -440,7 +446,7 @@ mod test_footprint {
         let res = fp.enforce_access(&key, AccessType::ReadWrite, &budget);
         assert!(HostError::result_matches_err(
             res,
-            (ScErrorType::Storage, ScErrorCode::InvalidAction)
+            (ScErrorType::Storage, ScErrorCode::ExceededLimit)
         ));
         Ok(())
     }

soroban-env-host/src/test/hostile.rs

@@ -55,7 +55,7 @@ fn hostile_ssmash_traps() -> Result<(), HostError> {
 
     assert!(HostError::result_matches_err(
         res,
-        (ScErrorType::WasmVm, ScErrorCode::InternalError)
+        (ScErrorType::WasmVm, ScErrorCode::InvalidAction)
     ));
     Ok(())
 }
@@ -73,7 +73,7 @@ fn hostile_oob1_traps() -> Result<(), HostError> {
 
     assert!(HostError::result_matches_err(
         res,
-        (ScErrorType::WasmVm, ScErrorCode::InternalError)
+        (ScErrorType::WasmVm, ScErrorCode::InvalidAction)
     ));
     Ok(())
 }
@@ -90,7 +90,7 @@ fn hostile_oob2_traps() -> Result<(), HostError> {
     );
     assert!(HostError::result_matches_err(
         res,
-        (ScErrorType::WasmVm, ScErrorCode::InternalError)
+        (ScErrorType::WasmVm, ScErrorCode::InvalidAction)
     ));
     Ok(())
 }

soroban-env-host/src/test/invocation.rs

@@ -24,7 +24,7 @@ fn invoke_single_contract_function() -> Result<(), HostError> {
         Symbol::try_from_small_str("add")?,
         host.test_vec_obj(&[a, c])?,
     );
-    let code = (ScErrorType::WasmVm, ScErrorCode::InternalError);
+    let code = (ScErrorType::WasmVm, ScErrorCode::InvalidAction);
     assert!(HostError::result_matches_err(res, code));
     Ok(())
 }
@@ -126,7 +126,7 @@ fn invoke_cross_contract_indirect_err() -> Result<(), HostError> {
 
     // try call -- add will trap, and add_with will trap, but we will get a status
     let status = host.try_call(id0_obj, sym, args)?;
-    let code = (ScErrorType::WasmVm, ScErrorCode::InternalError);
+    let code = (ScErrorType::WasmVm, ScErrorCode::InvalidAction);
     let exp: Error = code.into();
     assert_eq!(status.get_payload(), exp.to_val().get_payload());
 
@@ -135,7 +135,7 @@ fn invoke_cross_contract_indirect_err() -> Result<(), HostError> {
     let last_event = events.last().unwrap();
     // run `UPDATE_EXPECT=true cargo test` to update this.
     let expected = expect![[
-        r#"[Diagnostic Event] topics:[error, Error(WasmVm, InternalError)], data:["contract try_call failed", add_with, [2147483647, 1, Bytes()]]"#
+        r#"[Diagnostic Event] topics:[error, Error(WasmVm, InvalidAction)], data:["contract try_call failed", add_with, [2147483647, 1, Bytes()]]"#
     ]];
     let actual = format!("{}", last_event);
     expected.assert_eq(&actual);
@@ -149,7 +149,7 @@ fn invoke_cross_contract_indirect_err() -> Result<(), HostError> {
     let last_event = events.last().unwrap();
     // run `UPDATE_EXPECT=true cargo test` to update this.
     let expected = expect![[
-        r#"[Diagnostic Event] topics:[error, Error(WasmVm, InternalError)], data:["contract call failed", add_with, [2147483647, 1, Bytes()]]"#
+        r#"[Diagnostic Event] topics:[error, Error(WasmVm, InvalidAction)], data:["contract call failed", add_with, [2147483647, 1, Bytes()]]"#
     ]];
     let actual = format!("{}", last_event);
     expected.assert_eq(&actual);