Module cache upload and run (#1382)

This covers a two cases I failed to cover in #1375 that @dmkozh and
@sisuresh have identified:

- User calls contract without putting wasm for call in footprint or
storage
- User calls `upload_contract` (or its equivalent,
`update_current_contract_wasm`) mid-execution and then calls the
updated-or-new contract

As well as a third case that is, as far as I know, not possible; but it
seems to me it doesn't hurt to be defensive since someone could make it
possible in the future fairly easily and forget to cover this case in
the cache code:

  - User deletes wasm in transaction, and then tries to call it.

As always, there is probably room for a bit more testing here, but I
wanted to get these cases covered and fixed asap.

This should make-redundant PR #1380

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

@@ -657,15 +657,56 @@ impl Host {
                 #[cfg(not(feature = "recording_mode"))]
                 self.build_module_cache_if_needed()?;
                 let contract_id = id.metered_clone(self)?;
-                let vm = if let Some(cache) = &*self.try_borrow_module_cache()? {
-                    // This branch should be taken if we're on a protocol that
-                    // supports the module cache.
-                    let module = cache.get_module(self, wasm_hash)?;
+                let parsed_module = if let Some(cache) = &*self.try_borrow_module_cache()? {
+                    // Check that storage thinks the entry exists before
+                    // checking the cache: this seems like overkill but it
+                    // provides some future-proofing, see below.
+                    let wasm_key = self.contract_code_ledger_key(wasm_hash)?;
+                    if self
+                        .try_borrow_storage_mut()?
+                        .has(&wasm_key, self.budget_ref())?
+                    {
+                        cache.get_module(self, wasm_hash)?
+                    } else {
+                        None
+                    }
+                } else {
+                    None
+                };
+                let vm = if let Some(module) = parsed_module {
                     Vm::from_parsed_module(self, contract_id, module)?
                 } else {
-                    // If we got here we're running/replaying a protocol version
-                    // with no module cache. We'll parse the contract anew and
-                    // throw it away, as we did in that old protocol.
+                    // We can get here a few ways:
+                    //
+                    //   1. We are running/replaying a protocol that has no
+                    //      module cache.
+                    //
+                    //   2. We have a module cache, but it somehow doesn't have
+                    //      the module requested. This in turn has two
+                    //      sub-cases:
+                    //
+                    //     - User invoked us with bad input, eg. calling a
+                    //       contract that wasn't provided in footprint/storage.
+                    //
+                    //     - User uploaded the wasm _in this transaction_ so we
+                    //       didn't cache it when starting the transaction (and
+                    //       couldn't due to wasmi locking its engine while
+                    //       running).
+                    //
+                    //   3. Even more pathological: the module cache was built,
+                    //      and contained the module, but someone _removed_ the
+                    //      wasm from storage after the the cache was built
+                    //      (this is not currently possible from guest code, but
+                    //      we do some future-proofing here in case it becomes
+                    //      possible). This is the case we handle above with the
+                    //      early check for storage.has(wasm_key) before
+                    //      checking the cache as well.
+                    //
+                    // In all these cases, we want to try accessing storage, and
+                    // if it has the wasm, make a _throwaway_ module with its
+                    // own engine. If it doesn't have the wasm, we want to fail
+                    // with a storage error.
+
                     let (code, costs) = self.retrieve_wasm_from_storage(&wasm_hash)?;
                     Vm::new_with_cost_inputs(self, contract_id, code.as_slice(), costs)?
                 };

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

@@ -533,14 +533,14 @@ fn excessive_logging() -> Result<(), HostError> {
     #[cfg(feature = "next")]
     let expected_budget = expect![[r#"
         =================================================================
-        Cpu limit: 2000000; used: 214501
-        Mem limit: 500000; used: 166628
+        Cpu limit: 2000000; used: 215305
+        Mem limit: 500000; used: 166764
         =================================================================
         CostType                           cpu_insns      mem_bytes      
         WasmInsnExec                       300            0              
-        MemAlloc                           16609          67208          
-        MemCpy                             2607           0              
-        MemCmp                             416            0              
+        MemAlloc                           17058          67344          
+        MemCpy                             2866           0              
+        MemCmp                             512            0              
         DispatchHostFunction               310            0              
         VisitObject                        244            0              
         ValSer                             0              0              

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

@@ -572,9 +572,11 @@ fn test_large_contract() {
 mod cap_54_55_56 {
 
     use more_asserts::assert_gt;
+    use soroban_test_wasms::UPLOAD_CONTRACT;
 
     use super::*;
     use crate::{
+        host::crypto::sha256_hash_from_bytes,
         storage::{FootprintMap, StorageMap},
         test::observe::ObservedHost,
         testutils::wasm::wasm_module_with_a_bit_of_everything,
@@ -583,7 +585,7 @@ mod cap_54_55_56 {
             ContractCostType::{self, *},
             LedgerEntry, LedgerKey,
         },
-        AddressObject, HostError,
+        AddressObject, HostError, SymbolSmall,
     };
     use std::rc::Rc;
 
@@ -1140,5 +1142,232 @@ mod cap_54_55_56 {
         Ok(())
     }
 
+    // Test that, when running on protocol vNew and calling a wasm that is not
+    // in the footprint or storage, we just get a storage error, not any kind of
+    // internal error. This is a legitimate (if erroneous) way to invoke the host
+    // from outside and we should fail gracefully.
+    #[test]
+    fn test_v_new_call_nonexistent_wasm() -> Result<(), HostError> {
+        let (host, contract_id) = upload_and_make_host_for_next_ledger(
+            "test_v_new_call_nonexistent_wasm_upload",
+            V_NEW,
+            "test_v_new_call_nonexistent_wasm_call",
+            V_NEW,
+        )?;
+        let contract = host.add_host_object(crate::xdr::ScAddress::Contract(contract_id))?;
+
+        // Remove the wasm from the storage and footprint.
+        let wasm_to_delete = wasm_module_with_a_bit_of_everything(V_NEW);
+        let wasm_hash = Hash(
+            sha256_hash_from_bytes(&wasm_to_delete, host.as_budget())?
+                .try_into()
+                .unwrap(),
+        );
+        let wasm_key = host.contract_code_ledger_key(&wasm_hash)?;
+        host.with_mut_storage(|storage| {
+            let budget = host.budget_cloned();
+            storage.footprint.0 = storage
+                .footprint
+                .0
+                .remove::<Rc<LedgerKey>>(&wasm_key, &budget)?
+                .unwrap()
+                .0;
+            storage.map = storage
+                .map
+                .remove::<Rc<LedgerKey>>(&wasm_key, &budget)?
+                .unwrap()
+                .0;
+            Ok(())
+        })?;
+
+        // Cache is built here, wasm is not present by time cache is built so it fails.
+        let test_symbol = Symbol::try_from_small_str("test")?;
+        let args = host.vec_new()?;
+        let res = host.call(contract, test_symbol, args);
+        assert!(HostError::result_matches_err(
+            res,
+            (ScErrorType::Storage, ScErrorCode::ExceededLimit)
+        ));
+        Ok(())
+    }
+
+    // Test that, when running on protocol vNew and calling a wasm that is in
+    // the footprint but not in storage, we get a storage error as well (though
+    // a different one: ScErrorCode::MissingValue). This is a minor variant of
+    // the `test_v_new_call_nonexistent_wasm` test above.
+    #[test]
+    fn test_v_new_call_wasm_in_footprint_but_not_storage() -> Result<(), HostError> {
+        let (host, contract_id) = upload_and_make_host_for_next_ledger(
+            "test_v_new_call_wasm_in_footprint_but_not_storage_upload",
+            V_NEW,
+            "test_v_new_call_wasm_in_footprint_but_not_storage_call",
+            V_NEW,
+        )?;
+        let contract = host.add_host_object(crate::xdr::ScAddress::Contract(contract_id))?;
+
+        // Remove the wasm from storage by setting its value to `None`.
+        let wasm_to_delete = wasm_module_with_a_bit_of_everything(V_NEW);
+        let wasm_hash = Hash(
+            sha256_hash_from_bytes(&wasm_to_delete, host.as_budget())?
+                .try_into()
+                .unwrap(),
+        );
+        let wasm_key = host.contract_code_ledger_key(&wasm_hash)?;
+        host.with_mut_storage(|storage| {
+            let budget = host.budget_cloned();
+            storage.map = storage.map.insert(wasm_key, None, &budget)?;
+            Ok(())
+        })?;
+
+        // Cache is built here, wasm is not present by time cache is built so it fails.
+        let test_symbol = Symbol::try_from_small_str("test")?;
+        let args = host.vec_new()?;
+        let res = host.call(contract, test_symbol, args);
+        assert!(HostError::result_matches_err(
+            res,
+            (ScErrorType::Storage, ScErrorCode::MissingValue)
+        ));
+        Ok(())
+    }
+
+    // Test that, when running on protocol vNew and calling a wasm that
+    // initially _is_ in the footprint or storage, but is _deleted_ during
+    // execution, we then get a storage error. This is a minor variant of the
+    // `test_v_new_call_nonexistent_wasm` test above, and currently represents a
+    // scenario that can't happen, but it might in the future and it's nice to
+    // keep the cache as close to "reflecting storage" as possible.
+    #[test]
+    fn test_v_new_call_runtime_deleted_wasm() -> Result<(), HostError> {
+        let (host, contract_id) = upload_and_make_host_for_next_ledger(
+            "test_v_new_call_runtime_deleted_wasm_upload",
+            V_NEW,
+            "test_v_new_call_runtime_deleted_wasm_call",
+            V_NEW,
+        )?;
+        let contract = host.add_host_object(crate::xdr::ScAddress::Contract(contract_id))?;
+
+        // Cache is built here, wasm is present, so call succeeds.
+        let test_symbol = Symbol::try_from_small_str("test")?;
+        let args = host.vec_new()?;
+        let _ = host.call(contract, test_symbol, args)?;
+
+        // Remove the wasm from the storage and footprint.
+        let wasm_to_delete = wasm_module_with_a_bit_of_everything(V_NEW);
+        let wasm_hash = Hash(
+            sha256_hash_from_bytes(&wasm_to_delete, host.as_budget())?
+                .try_into()
+                .unwrap(),
+        );
+        let wasm_key = host.contract_code_ledger_key(&wasm_hash)?;
+        host.with_mut_storage(|storage| {
+            let budget = host.budget_cloned();
+            storage.footprint.0 = storage
+                .footprint
+                .0
+                .remove::<Rc<LedgerKey>>(&wasm_key, &budget)?
+                .unwrap()
+                .0;
+            storage.map = storage
+                .map
+                .remove::<Rc<LedgerKey>>(&wasm_key, &budget)?
+                .unwrap()
+                .0;
+            Ok(())
+        })?;
+
+        // Cache contains the wasm but storage doesn't, so we should fail.
+        let res = host.call(contract, test_symbol, args);
+        assert!(HostError::result_matches_err(
+            res,
+            (ScErrorType::Storage, ScErrorCode::ExceededLimit)
+        ));
+        Ok(())
+    }
+
+    // Test that, when running on protocol vNew and uploading a contract
+    // mid-transaction using a host function, one can call the contract even
+    // though it won't reside in the module cache because the cache is frozen on
+    // first access.
+    #[test]
+    fn test_v_new_update_contract_with_module_cache() -> Result<(), HostError> {
+        let host = Host::test_host_with_recording_footprint();
+        let host = ObservedHost::new("test_v_new_update_contract_with_module_cache", host);
+        host.enable_debug()?;
+        host.with_mut_ledger_info(|ledger_info| ledger_info.protocol_version = V_NEW)?;
+        let upload_contract_addr_obj = host.register_test_contract_wasm(UPLOAD_CONTRACT);
+        let updateable_contract_addr_obj = host.register_test_contract_wasm(UPDATEABLE_CONTRACT);
+
+        // Prep the footprint and storage map for accepting an upload, the way
+        // we would if we'd had a transaction declare a write-only footprint
+        // entry for the wasm key.
+        let wasm_to_upload = wasm_module_with_a_bit_of_everything(V_NEW);
+        let wasm_hash = Hash(
+            sha256_hash_from_bytes(&wasm_to_upload, host.as_budget())?
+                .try_into()
+                .unwrap(),
+        );
+        let wasm_code_key = host.contract_code_ledger_key(&wasm_hash)?;
+        host.with_mut_storage(|storage| {
+            storage.footprint.record_access(
+                &wasm_code_key,
+                AccessType::ReadWrite,
+                host.as_budget(),
+            )?;
+            storage.map = storage.map.insert(wasm_code_key, None, host.as_budget())?;
+            Ok(())
+        })?;
+
+        host.switch_to_enforcing_storage()?;
+
+        let wasm_bytes = host.bytes_new_from_slice(&wasm_to_upload)?;
+        let upload_args = host.vec_new_from_slice(&[wasm_bytes.to_val()])?;
+
+        // Module cache will be built _before_ this call is dispatched, and call
+        // will attempt to add a wasm to storage that is not in the cache.
+        let wasm_hash_val = host.call(
+            upload_contract_addr_obj,
+            Symbol::try_from_small_str("upload")?,
+            upload_args,
+        )?;
+
+        // Now we update the updatable contract to point to it and
+        // invoke it, which should actually work, just not go through
+        // the module cache.
+        let update_args = host
+            .vec_new_from_slice(&[wasm_hash_val, /*fail:*/ Val::from_bool(false).to_val()])?;
+        let _ = host.call(
+            updateable_contract_addr_obj,
+            Symbol::try_from_small_str("update")?,
+            update_args,
+        )?;
+
+        // Check that we have charged nonzero new-style wasm parsing costs.
+        let budget = host.budget_cloned();
+        let pre_parse_cost = budget.get_tracker(ParseWasmInstructions)?.cpu;
+        assert_ne!(pre_parse_cost, 0);
+
+        // Check that the module cache did not get populated with the new wasm.
+        if let Some(module_cache) = &*host.try_borrow_module_cache()? {
+            assert!(module_cache.get_module(&*host, &wasm_hash)?.is_none());
+        } else {
+            panic!("expected module cache");
+        }
+
+        let updated_args = host.vec_new()?;
+        let res = host.call(
+            updateable_contract_addr_obj,
+            Symbol::try_from_small_str("test")?,
+            updated_args,
+        )?;
+        assert_eq!(SymbolSmall::try_from(res)?.to_string(), "pass");
+
+        // Check that the call to the updated wasm did more parsing, i.e. did
+        // not have a cache hit.
+        let post_parse_cost = budget.get_tracker(ParseWasmInstructions)?.cpu;
+        assert_ne!(post_parse_cost, pre_parse_cost);
+
+        Ok(())
+    }
+
     // endregion: CAP-0056 ModuleCache related tests
 }

soroban-env-host/src/vm/module_cache.rs

@@ -129,17 +129,12 @@ impl ModuleCache {
     pub fn get_module(
         &self,
         host: &Host,
-        contract_id: &Hash,
-    ) -> Result<Rc<ParsedModule>, HostError> {
-        if let Some(m) = self.modules.get(contract_id, host)? {
-            return Ok(m.clone());
+        wasm_hash: &Hash,
+    ) -> Result<Option<Rc<ParsedModule>>, HostError> {
+        if let Some(m) = self.modules.get(wasm_hash, host)? {
+            Ok(Some(m.clone()))
         } else {
-            Err(host.err(
-                ScErrorType::Context,
-                ScErrorCode::InternalError,
-                "module cache missing contract",
-                &[],
-            ))
+            Ok(None)
         }
     }
 }

soroban-test-wasms/src/lib.rs

@@ -76,6 +76,8 @@ mod curr {
         include_bytes!("../wasm-workspace/opt/curr/auth_test_contract.wasm").as_slice();
     pub const UPDATEABLE_CONTRACT: &[u8] =
         include_bytes!("../wasm-workspace/opt/curr/example_updateable_contract.wasm").as_slice();
+    pub const UPLOAD_CONTRACT: &[u8] =
+        include_bytes!("../wasm-workspace/opt/curr/example_upload_contract.wasm").as_slice();
     pub const DELEGATED_ACCOUNT_TEST_CONTRACT: &[u8] =
         include_bytes!("../wasm-workspace/opt/curr/test_delegated_account.wasm").as_slice();
     pub const ERR: &[u8] = include_bytes!("../wasm-workspace/opt/curr/example_err.wasm").as_slice();