fix(runtime): Fix state change check during view call (#2229)

Fixes #2226 
The current behavior of a view call is to prohibit state changes. This change moves check from the end of the state viewer to VMLogic by prohibiting state changes functions during view calls.

## Test plan:
- Fixed state change tests. Previously the test didn't verify error type. The error was `MethodNotFound` and also alice account was wrong.
- Added unit tests for the new prohibited methods.

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

@@ -1850,6 +1850,7 @@ impl<'a> VMLogic<'a> {
     /// * If the length of the key exceeds `max_length_storage_key` returns `KeyLengthExceeded`.
     /// * If the length of the value exceeds `max_length_storage_value` returns
     ///   `ValueLengthExceeded`.
+    /// * If called as view function returns `ProhibitedInView``.
     ///
     /// # Cost
     ///
@@ -1866,6 +1867,11 @@ impl<'a> VMLogic<'a> {
         register_id: u64,
     ) -> Result<u64> {
         self.gas_counter.pay_base(base)?;
+        if self.context.is_view {
+            return Err(
+                HostError::ProhibitedInView { method_name: "storage_write".to_string() }.into()
+            );
+        }
         self.gas_counter.pay_base(storage_write_base)?;
         // All iterators that were valid now become invalid
         for invalidated_iter_idx in self.valid_iterators.drain() {
@@ -1998,13 +2004,19 @@ impl<'a> VMLogic<'a> {
     /// * If returning the preempted value into the registers exceed the memory container it returns
     ///   `MemoryAccessViolation`.
     /// * If the length of the key exceeds `max_length_storage_key` returns `KeyLengthExceeded`.
+    /// * If called as view function returns `ProhibitedInView``.
     ///
     /// # Cost
     ///
     /// `base + storage_remove_base + storage_remove_key_byte * num_key_bytes + storage_remove_ret_value_byte * num_value_bytes
     /// + cost to read the key + cost to write the value`.
     pub fn storage_remove(&mut self, key_len: u64, key_ptr: u64, register_id: u64) -> Result<u64> {
         self.gas_counter.pay_base(base)?;
+        if self.context.is_view {
+            return Err(
+                HostError::ProhibitedInView { method_name: "storage_remove".to_string() }.into()
+            );
+        }
         self.gas_counter.pay_base(storage_remove_base)?;
         // All iterators that were valid now become invalid
         for invalidated_iter_idx in self.valid_iterators.drain() {

runtime/near-vm-logic/tests/test_view_method.rs

@@ -40,6 +40,8 @@ fn test_prohibited_view_methods() {
     test_prohibited!(promise_results_count);
     test_prohibited!(promise_result, 0, 0);
     test_prohibited!(promise_return, 0);
+    test_prohibited!(storage_write, 0, 0, 0, 0, 0);
+    test_prohibited!(storage_remove, 0, 0, 0);
 }
 
 #[test]

runtime/near-vm-runner/tests/test-contract-rs/build.sh

@@ -1,5 +1,5 @@
 #!/bin/bash
 
-RUSTFLAGS='-C link-arg=-s' cargo +nightly build --target wasm32-unknown-unknown --release
+RUSTFLAGS='-C link-arg=-s' cargo build --target wasm32-unknown-unknown --release
 cp target/wasm32-unknown-unknown/release/test_contract_rs.wasm ../res/
 rm -rf target

runtime/near-vm-runner/tests/test-contract-rs/src/lib.rs

@@ -271,6 +271,13 @@ pub unsafe fn run_test() {
     value_return(value.len() as u64, value.as_ptr() as _);
 }
 
+#[no_mangle]
+pub unsafe fn run_test_with_storage_change() {
+    let key = b"hello";
+    let value = b"world";
+    storage_write(key.len() as _, key.as_ptr() as _, value.len() as _, value.as_ptr() as _, 0);
+}
+
 #[no_mangle]
 pub unsafe fn sum_with_input() {
     input(0);

runtime/runtime/src/state_viewer.rs

@@ -161,10 +161,6 @@ impl TrieViewer {
             let outcome = outcome.unwrap();
             debug!(target: "runtime", "(exec time {}) result of execution: {:#?}", time_str, outcome);
             logs.extend(outcome.logs);
-            let trie_update = state_update.finalize()?;
-            if trie_update.new_root != root {
-                return Err("function call for viewing tried to change storage".into());
-            }
             let mut result = vec![];
             if let ReturnData::Value(buf) = &outcome.return_data {
                 result = buf.clone();
@@ -218,7 +214,11 @@ mod tests {
             &mut logs,
         );
 
-        assert!(result.is_err());
+        let err = result.unwrap_err();
+        assert!(
+            err.to_string().contains(r#"Contract ID "bad!contract" is not valid"#),
+            format!("Got different error that doesn't match: {}", err)
+        );
     }
 
     #[test]
@@ -230,13 +230,16 @@ mod tests {
             root,
             1,
             1,
-            &alice_account(),
+            &AccountId::from("test.contract"),
             "run_test_with_storage_change",
             &[],
             &mut logs,
         );
-        // run_test tries to change storage, so it should fail
-        assert!(result.is_err());
+        let err = result.unwrap_err();
+        assert!(
+            err.to_string().contains(r#"ProhibitedInView { method_name: "storage_write" }"#),
+            format!("Got different error that doesn't match: {}", err)
+        );
     }
 
     #[test]