[move] Fixing unit tests

- Changed errors to VMErrors to be compatible with V1 locations - Fixed a few bugs in integration tests - Added failpoints to V2 so that paranoid tests work - Fixed verification for runtime environment (it was not taking config before)
aptos-labs · Aug 23, 2024 · 7691132 · 7691132
1 parent 8ae4fc6
commit 7691132
Show file tree

Hide file tree

Showing 32 changed files with 440 additions and 347 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/aptos-move/aptos-vm-types/src/module_and_script_storage/state_view_adapter.rs b/aptos-move/aptos-vm-types/src/module_and_script_storage/state_view_adapter.rs
@@ -6,7 +6,11 @@ use crate::module_and_script_storage::{
 };
 use aptos_types::state_store::{state_key::StateKey, state_value::StateValueMetadata, StateView};
 use bytes::Bytes;
-use move_binary_format::{errors::PartialVMResult, file_format::CompiledScript, CompiledModule};
+use move_binary_format::{
+    errors::{PartialVMResult, VMResult},
+    file_format::CompiledScript,
+    CompiledModule,
+};
 use move_core_types::{account_address::AccountAddress, identifier::IdentStr, metadata::Metadata};
 use move_vm_runtime::{
     module_storage_error, move_vm::MoveVM, IntoUnsyncCodeStorage, Module, ModuleBytesStorage,
@@ -24,7 +28,7 @@ impl<'s, S: StateView> ModuleBytesStorage for StateViewAdapter<'s, S> {
         &self,
         address: &AccountAddress,
         module_name: &IdentStr,
-    ) -> PartialVMResult<Option<Bytes>> {
+    ) -> VMResult<Option<Bytes>> {
         let state_key = StateKey::module(address, module_name);
         self.state_view
             .get_state_value_bytes(&state_key)
@@ -52,23 +56,23 @@ impl<'s, S: StateView> ModuleStorage for AptosCodeStorageAdapter<'s, S> {
         &self,
         address: &AccountAddress,
         module_name: &IdentStr,
-    ) -> PartialVMResult<bool> {
+    ) -> VMResult<bool> {
         self.storage.check_module_exists(address, module_name)
     }
 
     fn fetch_module_bytes(
         &self,
         address: &AccountAddress,
         module_name: &IdentStr,
-    ) -> PartialVMResult<Option<Bytes>> {
+    ) -> VMResult<Option<Bytes>> {
         self.storage.fetch_module_bytes(address, module_name)
     }
 
     fn fetch_module_size_in_bytes(
         &self,
         address: &AccountAddress,
         module_name: &IdentStr,
-    ) -> PartialVMResult<usize> {
+    ) -> VMResult<usize> {
         self.storage
             .fetch_module_size_in_bytes(address, module_name)
     }
@@ -77,23 +81,23 @@ impl<'s, S: StateView> ModuleStorage for AptosCodeStorageAdapter<'s, S> {
         &self,
         address: &AccountAddress,
         module_name: &IdentStr,
-    ) -> PartialVMResult<Vec<Metadata>> {
+    ) -> VMResult<Vec<Metadata>> {
         self.storage.fetch_module_metadata(address, module_name)
     }
 
     fn fetch_deserialized_module(
         &self,
         address: &AccountAddress,
         module_name: &IdentStr,
-    ) -> PartialVMResult<Arc<CompiledModule>> {
+    ) -> VMResult<Arc<CompiledModule>> {
         self.storage.fetch_deserialized_module(address, module_name)
     }
 
     fn fetch_verified_module(
         &self,
         address: &AccountAddress,
         module_name: &IdentStr,
-    ) -> PartialVMResult<Arc<Module>> {
+    ) -> VMResult<Arc<Module>> {
         self.storage.fetch_verified_module(address, module_name)
     }
 }
@@ -111,20 +115,17 @@ impl<'s, S: StateView> AptosModuleStorage for AptosCodeStorageAdapter<'s, S> {
             .byte_storage()
             .state_view
             .get_state_value(&state_key)
-            .map_err(|e| module_storage_error!(address, module_name, e))?
+            .map_err(|e| module_storage_error!(address, module_name, e).to_partial())?
             .map(|s| s.into_metadata()))
     }
 }
 
 impl<'s, S: StateView> ScriptStorage for AptosCodeStorageAdapter<'s, S> {
-    fn fetch_deserialized_script(
-        &self,
-        serialized_script: &[u8],
-    ) -> PartialVMResult<Arc<CompiledScript>> {
+    fn fetch_deserialized_script(&self, serialized_script: &[u8]) -> VMResult<Arc<CompiledScript>> {
         self.storage.fetch_deserialized_script(serialized_script)
     }
 
-    fn fetch_verified_script(&self, serialized_script: &[u8]) -> PartialVMResult<Arc<Script>> {
+    fn fetch_verified_script(&self, serialized_script: &[u8]) -> VMResult<Arc<Script>> {
         self.storage.fetch_verified_script(serialized_script)
     }
 }

diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs
@@ -110,6 +110,7 @@ use move_core_types::{
 };
 use move_vm_runtime::{
     logging::expect_no_verification_errors,
+    module_linker_error,
     module_traversal::{TraversalContext, TraversalStorage},
     TemporaryModuleStorage, UnreachableCodeStorage,
 };
@@ -851,7 +852,11 @@ impl AptosVM {
         _txn_data: &TransactionMetadata,
     ) -> Result<(), VMStatus> {
         if self.features().use_loader_v2() {
-            // TODO(loader_v2): Check that the module & function actually exist?
+            let addr = entry_fn.module().address();
+            let name = entry_fn.module().name();
+            if !module_storage.check_module_exists(addr, name)? {
+                return Err(module_linker_error!(addr, name).into_vm_status());
+            }
         }
 
         // Note: Feature gating is needed here because the traversal of the dependencies could
@@ -1570,13 +1575,8 @@ impl AptosVM {
                         }
 
                         let size_if_module_exists = if self.features().use_loader_v2() {
-                            if module_storage
-                                .check_module_exists(addr, name)
-                                .map_err(|e| e.finish(Location::Undefined))?
-                            {
-                                let size = module_storage
-                                    .fetch_module_size_in_bytes(addr, name)
-                                    .map_err(|e| e.finish(Location::Undefined))?;
+                            if module_storage.check_module_exists(addr, name)? {
+                                let size = module_storage.fetch_module_size_in_bytes(addr, name)?;
                                 Some(size as u64)
                             } else {
                                 None
@@ -1674,8 +1674,7 @@ impl AptosVM {
                         compat,
                         module_storage,
                         bundle.into_bytes(),
-                    )
-                    .map_err(|e| e.finish(Location::Undefined))?;
+                    )?;
 
                     // Run init_module for each new module. We use a new session for that as well.
                     let mut tmp_session = tmp_vm.new_session(
@@ -1688,8 +1687,7 @@ impl AptosVM {
                     for module in modules {
                         // Check if module existed previously.
                         let module_exists = module_storage
-                            .check_module_exists(module.self_addr(), module.self_name())
-                            .map_err(|e| e.finish(Location::Undefined))?;
+                            .check_module_exists(module.self_addr(), module.self_name())?;
                         if module_exists {
                             continue;
                         }
@@ -3149,9 +3147,7 @@ pub(crate) fn fetch_module_metadata_for_struct_tag(
     module_storage: &impl AptosModuleStorage,
 ) -> VMResult<Vec<Metadata>> {
     if features.use_loader_v2() {
-        module_storage
-            .fetch_module_metadata(&struct_tag.address, &struct_tag.module)
-            .map_err(|e| e.finish(Location::Undefined))
+        module_storage.fetch_module_metadata(&struct_tag.address, &struct_tag.module)
     } else {
         Ok(resolver.get_module_metadata(&struct_tag.module_id()))
     }

diff --git a/aptos-move/aptos-vm/src/move_vm_ext/write_op_converter.rs b/aptos-move/aptos-vm/src/move_vm_ext/write_op_converter.rs
@@ -178,7 +178,9 @@ impl<'r> WriteOpConverter<'r> {
             let addr = compiled_module.self_addr();
             let name = compiled_module.self_name();
 
-            let module_exists = module_storage.check_module_exists(addr, name)?;
+            let module_exists = module_storage
+                .check_module_exists(addr, name)
+                .map_err(|e| e.to_partial())?;
             let op = if module_exists {
                 Op::Modify(bytes)
             } else {

diff --git a/aptos-move/aptos-vm/src/verifier/randomness.rs b/aptos-move/aptos-vm/src/verifier/randomness.rs
@@ -5,7 +5,7 @@ use crate::move_vm_ext::{AptosMoveResolver, SessionExt};
 use aptos_framework::{KnownAttribute, RandomnessAnnotation};
 use aptos_types::transaction::EntryFunction;
 use aptos_vm_types::module_and_script_storage::module_storage::AptosModuleStorage;
-use move_binary_format::errors::{Location, VMResult};
+use move_binary_format::errors::VMResult;
 
 pub(crate) fn get_randomness_annotation(
     resolver: &impl AptosMoveResolver,
@@ -17,8 +17,7 @@ pub(crate) fn get_randomness_annotation(
     let module = if use_loader_v2 {
         // TODO(loader_v2): Enhance this further by querying RuntimeModuleMetadataV1 directly.
         module_storage
-            .fetch_deserialized_module(entry_fn.module().address(), entry_fn.module().name())
-            .map_err(|e| e.finish(Location::Undefined))?
+            .fetch_deserialized_module(entry_fn.module().address(), entry_fn.module().name())?
     } else {
         #[allow(deprecated)]
         session

diff --git a/third_party/move/move-bytecode-verifier/src/dependencies.rs b/third_party/move/move-bytecode-verifier/src/dependencies.rs
@@ -164,6 +164,7 @@ pub fn verify_module<'a>(
     module: &CompiledModule,
     dependencies: impl IntoIterator<Item = &'a CompiledModule>,
 ) -> VMResult<()> {
+    fail::fail_point!("skip-verification-for-paranoid-tests", |_| { Ok(()) });
     verify_module_impl(module, dependencies)
         .map_err(|e| e.finish(Location::Module(module.self_id())))
 }
@@ -184,6 +185,7 @@ pub fn verify_script<'a>(
     script: &CompiledScript,
     dependencies: impl IntoIterator<Item = &'a CompiledModule>,
 ) -> VMResult<()> {
+    fail::fail_point!("skip-verification-for-paranoid-tests", |_| { Ok(()) });
     verify_script_impl(script, dependencies).map_err(|e| e.finish(Location::Script))
 }
 

diff --git a/third_party/move/move-bytecode-verifier/src/verifier.rs b/third_party/move/move-bytecode-verifier/src/verifier.rs
@@ -90,6 +90,8 @@ pub fn verify_module_with_config_for_test(
 }
 
 pub fn verify_module_with_config(config: &VerifierConfig, module: &CompiledModule) -> VMResult<()> {
+    fail::fail_point!("skip-verification-for-paranoid-tests", |_| { Ok(()) });
+
     let prev_state = move_core_types::state::set_state(VMState::VERIFIER);
     let result = std::panic::catch_unwind(|| {
         // Always needs to run bound checker first as subsequent passes depend on it
@@ -149,7 +151,7 @@ pub fn verify_script(script: &CompiledScript) -> VMResult<()> {
 }
 
 pub fn verify_script_with_config(config: &VerifierConfig, script: &CompiledScript) -> VMResult<()> {
-    fail::fail_point!("verifier-failpoint-3", |_| { Ok(()) });
+    fail::fail_point!("skip-verification-for-paranoid-tests", |_| { Ok(()) });
 
     let prev_state = move_core_types::state::set_state(VMState::VERIFIER);
     let result = std::panic::catch_unwind(|| {

diff --git a/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs b/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs
@@ -5,7 +5,7 @@
 use crate::compiler::{as_module, as_script, compile_units};
 use bytes::Bytes;
 use move_binary_format::{
-    errors::{PartialVMError, PartialVMResult},
+    errors::{Location, PartialVMError, PartialVMResult, VMResult},
     CompiledModule,
 };
 use move_core_types::{
@@ -627,48 +627,48 @@ impl ModuleStorage for BogusModuleStorage {
         &self,
         _address: &AccountAddress,
         _module_name: &IdentStr,
-    ) -> PartialVMResult<bool> {
-        Err(PartialVMError::new(self.bad_status_code))
+    ) -> VMResult<bool> {
+        Err(PartialVMError::new(self.bad_status_code).finish(Location::Undefined))
     }
 
     fn fetch_module_bytes(
         &self,
         _address: &AccountAddress,
         _module_name: &IdentStr,
-    ) -> PartialVMResult<Option<Bytes>> {
-        Err(PartialVMError::new(self.bad_status_code))
+    ) -> VMResult<Option<Bytes>> {
+        Err(PartialVMError::new(self.bad_status_code).finish(Location::Undefined))
     }
 
     fn fetch_module_size_in_bytes(
         &self,
         _address: &AccountAddress,
         _module_name: &IdentStr,
-    ) -> PartialVMResult<usize> {
-        Err(PartialVMError::new(self.bad_status_code))
+    ) -> VMResult<usize> {
+        Err(PartialVMError::new(self.bad_status_code).finish(Location::Undefined))
     }
 
     fn fetch_module_metadata(
         &self,
         _address: &AccountAddress,
         _module_name: &IdentStr,
-    ) -> PartialVMResult<Vec<Metadata>> {
-        Err(PartialVMError::new(self.bad_status_code))
+    ) -> VMResult<Vec<Metadata>> {
+        Err(PartialVMError::new(self.bad_status_code).finish(Location::Undefined))
     }
 
     fn fetch_deserialized_module(
         &self,
         _address: &AccountAddress,
         _module_name: &IdentStr,
-    ) -> PartialVMResult<Arc<CompiledModule>> {
-        Err(PartialVMError::new(self.bad_status_code))
+    ) -> VMResult<Arc<CompiledModule>> {
+        Err(PartialVMError::new(self.bad_status_code).finish(Location::Undefined))
     }
 
     fn fetch_verified_module(
         &self,
         _address: &AccountAddress,
         _module_name: &IdentStr,
-    ) -> PartialVMResult<Arc<Module>> {
-        Err(PartialVMError::new(self.bad_status_code))
+    ) -> VMResult<Arc<Module>> {
+        Err(PartialVMError::new(self.bad_status_code).finish(Location::Undefined))
     }
 }