[aptos-vm][move] Avoid module loads when getting the struct name (#15681

)

For transaction argument validation, when checking for struct names there is no
need to get them from module storage - the name exists in reindexing map and
lookup there should save us some module loads.

aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs

@@ -190,7 +190,10 @@ pub(crate) fn validate_combine_signer_and_txn_args(
     Ok(combined_args)
 }
 
-// Return whether the argument is valid/allowed and whether it needs construction.
+/// Returns true if the argument is valid (that is, it is a primitive type or a struct with a
+/// known constructor function). Otherwise, (for structs without constructors, signers or
+/// references) returns false. An error is returned in cases when a struct type is encountered and
+/// its name cannot be queried for some reason.
 pub(crate) fn is_valid_txn_arg(
     session: &SessionExt,
     module_storage: &impl AptosModuleStorage,
@@ -202,12 +205,21 @@ pub(crate) fn is_valid_txn_arg(
     match ty {
         Bool | U8 | U16 | U32 | U64 | U128 | U256 | Address => true,
         Vector(inner) => is_valid_txn_arg(session, module_storage, inner, allowed_structs),
-        Struct { idx, .. } | StructInstantiation { idx, .. } => session
-            .fetch_struct_ty_by_idx(*idx, module_storage)
-            .is_some_and(|st| {
-                let full_name = format!("{}::{}", st.module.short_str_lossless(), st.name);
-                allowed_structs.contains_key(&full_name)
-            }),
+        Struct { .. } | StructInstantiation { .. } => {
+            // Note: Original behavior was to return false even if the module loading fails (e.g.,
+            //       if struct does not exist. This preserves it.
+            session
+                .get_struct_name(ty, module_storage)
+                .ok()
+                .flatten()
+                .is_some_and(|(module_id, identifier)| {
+                    allowed_structs.contains_key(&format!(
+                        "{}::{}",
+                        module_id.short_str_lossless(),
+                        identifier
+                    ))
+                })
+        },
         Signer | Reference(_) | MutableReference(_) | TyParam(_) => false,
     }
 }
@@ -340,12 +352,16 @@ pub(crate) fn recursively_construct_arg(
                 len -= 1;
             }
         },
-        Struct { idx, .. } | StructInstantiation { idx, .. } => {
-            let st = session
-                .fetch_struct_ty_by_idx(*idx, module_storage)
+        Struct { .. } | StructInstantiation { .. } => {
+            let (module_id, identifier) = session
+                .get_struct_name(ty, module_storage)
+                .map_err(|_| {
+                    // Note: The original behaviour was to map all errors to an invalid signature
+                    //       error, here we want to preserve it for now.
+                    invalid_signature()
+                })?
                 .ok_or_else(invalid_signature)?;
-
-            let full_name = format!("{}::{}", st.module.short_str_lossless(), st.name);
+            let full_name = format!("{}::{}", module_id.short_str_lossless(), identifier);
             let constructor = allowed_structs
                 .get(&full_name)
                 .ok_or_else(invalid_signature)?;

third_party/move/move-vm/integration-tests/Cargo.toml

@@ -21,7 +21,7 @@ move-core-types = { workspace = true }
 move-stdlib = { path = "../../move-stdlib" }
 move-vm-runtime = { workspace = true, features = ["testing"] }
 move-vm-test-utils = { workspace = true }
-move-vm-types = { workspace = true }
+move-vm-types = { workspace = true, features = ["testing"] }
 smallvec = { workspace = true }
 tempfile = { workspace = true }
 

third_party/move/move-vm/integration-tests/src/tests/module_storage_tests.rs

@@ -15,7 +15,10 @@ use move_vm_runtime::{
 };
 use move_vm_test_utils::InMemoryStorage;
 use move_vm_types::{
-    loaded_data::runtime_types::{AbilityInfo, StructIdentifier, StructNameIndex, TypeBuilder},
+    loaded_data::{
+        runtime_types::{AbilityInfo, StructIdentifier, TypeBuilder},
+        struct_name_indexing::StructNameIndex,
+    },
     value_serde::FunctionValueExtension,
 };
 use std::str::FromStr;
@@ -74,7 +77,7 @@ fn test_function_value_extension() {
     let foo_ty = types.pop().unwrap();
     let name = module_storage
         .runtime_environment()
-        .idx_to_struct_name_for_test(StructNameIndex(0))
+        .idx_to_struct_name_for_test(StructNameIndex::new(0))
         .unwrap();
     assert_eq!(name, StructIdentifier {
         module: test_id.clone(),
@@ -84,8 +87,10 @@ fn test_function_value_extension() {
         foo_ty,
         ty_builder
             .create_ref_ty(
-                &ty_builder
-                    .create_struct_ty(StructNameIndex(0), AbilityInfo::struct_(AbilitySet::EMPTY)),
+                &ty_builder.create_struct_ty(
+                    StructNameIndex::new(0),
+                    AbilityInfo::struct_(AbilitySet::EMPTY)
+                ),
                 false
             )
             .unwrap()
@@ -121,7 +126,7 @@ fn test_function_value_extension() {
     let bar_ty = types.pop().unwrap();
     let name = module_storage
         .runtime_environment()
-        .idx_to_struct_name_for_test(StructNameIndex(1))
+        .idx_to_struct_name_for_test(StructNameIndex::new(1))
         .unwrap();
     assert_eq!(name, StructIdentifier {
         module: other_test_id,
@@ -130,7 +135,7 @@ fn test_function_value_extension() {
     assert_eq!(
         bar_ty,
         ty_builder.create_struct_ty(
-            StructNameIndex(1),
+            StructNameIndex::new(1),
             AbilityInfo::struct_(AbilitySet::from_u8(Ability::Drop as u8).unwrap())
         )
     );

third_party/move/move-vm/runtime/src/loader/mod.rs

@@ -28,17 +28,23 @@ use move_core_types::{
     value::MoveTypeLayout,
     vm_status::StatusCode,
 };
+use move_vm_metrics::{Timer, VM_TIMER};
 use move_vm_types::{
     gas::GasMeter,
-    loaded_data::runtime_types::{AbilityInfo, StructNameIndex, StructType, Type},
+    loaded_data::{
+        runtime_types::{AbilityInfo, StructType, Type},
+        struct_name_indexing::StructNameIndex,
+    },
     sha3_256,
+    value_serde::FunctionValueExtension,
 };
 use parking_lot::{Mutex, RwLock};
 use std::{
     collections::{btree_map, BTreeMap, BTreeSet},
     hash::Hash,
     sync::Arc,
 };
+use type_loader::intern_type;
 use typed_arena::Arena;
 
 mod access_specifier_loader;
@@ -51,8 +57,7 @@ use crate::{
     loader::modules::{StructVariantInfo, VariantFieldInfo},
     native_functions::NativeFunctions,
     storage::{
-        loader::LoaderV2, module_storage::FunctionValueExtensionAdapter,
-        struct_name_index_map::StructNameIndexMap, ty_cache::StructInfoCache,
+        loader::LoaderV2, module_storage::FunctionValueExtensionAdapter, ty_cache::StructInfoCache,
         ty_layout_converter::LoaderLayoutConverter, ty_tag_converter::TypeTagConverter,
     },
 };
@@ -64,14 +69,12 @@ use move_binary_format::file_format::{
     StructVariantHandleIndex, StructVariantInstantiationIndex, TypeParameterIndex,
     VariantFieldHandleIndex, VariantFieldInstantiationIndex, VariantIndex,
 };
-use move_vm_metrics::{Timer, VM_TIMER};
-use move_vm_types::{
-    loaded_data::runtime_types::{DepthFormula, StructLayout, TypeBuilder},
-    value_serde::FunctionValueExtension,
+use move_vm_types::loaded_data::{
+    runtime_types::{DepthFormula, StructLayout, TypeBuilder},
+    struct_name_indexing::StructNameIndexMap,
 };
 pub use script::Script;
 pub(crate) use script::ScriptCache;
-use type_loader::intern_type;
 
 type ScriptHash = [u8; 32];
 

third_party/move/move-vm/runtime/src/loader/modules.rs

@@ -9,7 +9,6 @@ use crate::{
         BinaryCache,
     },
     native_functions::NativeFunctions,
-    storage::struct_name_index_map::StructNameIndexMap,
 };
 use move_binary_format::{
     access::ModuleAccess,
@@ -29,8 +28,9 @@ use move_core_types::{
     vm_status::StatusCode,
 };
 use move_vm_metrics::{Timer, VM_TIMER};
-use move_vm_types::loaded_data::runtime_types::{
-    StructIdentifier, StructLayout, StructNameIndex, StructType, Type,
+use move_vm_types::loaded_data::{
+    runtime_types::{StructIdentifier, StructLayout, StructType, Type},
+    struct_name_indexing::{StructNameIndex, StructNameIndexMap},
 };
 use parking_lot::RwLock;
 use std::{
@@ -575,7 +575,6 @@ impl Module {
     ) -> PartialVMResult<StructType> {
         let struct_handle = module.struct_handle_at(struct_def.struct_handle);
         let abilities = struct_handle.abilities;
-        let name = module.identifier_at(struct_handle.name).to_owned();
         let ty_params = struct_handle.type_parameters.clone();
         let layout = match &struct_def.field_information {
             StructFieldInformation::Native => unreachable!("native structs have been removed"),
@@ -613,8 +612,6 @@ impl Module {
             abilities,
             ty_params,
             idx: struct_name_table[struct_def.struct_handle.0 as usize],
-            module: module.self_id(),
-            name,
         })
     }
 

third_party/move/move-vm/runtime/src/loader/script.rs

@@ -2,7 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use super::{intern_type, BinaryCache, Function, FunctionHandle, FunctionInstantiation};
-use crate::{loader::ScriptHash, storage::struct_name_index_map::StructNameIndexMap};
+use crate::loader::ScriptHash;
 use move_binary_format::{
     access::ScriptAccess,
     binary_views::BinaryIndexedView,
@@ -13,6 +13,7 @@ use move_core_types::{identifier::Identifier, language_storage::ModuleId, vm_sta
 use move_vm_types::loaded_data::{
     runtime_access_specifier::AccessSpecifier,
     runtime_types::{StructIdentifier, Type},
+    struct_name_indexing::StructNameIndexMap,
 };
 use std::{collections::BTreeMap, ops::Deref, sync::Arc};
 

third_party/move/move-vm/runtime/src/loader/type_loader.rs

@@ -7,7 +7,10 @@ use move_binary_format::{
     file_format::SignatureToken,
 };
 use move_core_types::vm_status::StatusCode;
-use move_vm_types::loaded_data::runtime_types::{AbilityInfo, StructNameIndex, Type};
+use move_vm_types::loaded_data::{
+    runtime_types::{AbilityInfo, Type},
+    struct_name_indexing::StructNameIndex,
+};
 use triomphe::Arc as TriompheArc;
 
 /// Converts a signature token into the in memory type representation used by the MoveVM.

third_party/move/move-vm/runtime/src/session.rs

@@ -18,17 +18,17 @@ use move_core_types::{
     account_address::AccountAddress,
     effects::{ChangeSet, Changes},
     gas_algebra::NumBytes,
-    identifier::IdentStr,
+    identifier::{IdentStr, Identifier},
     language_storage::{ModuleId, TypeTag},
     value::MoveTypeLayout,
     vm_status::StatusCode,
 };
 use move_vm_types::{
     gas::GasMeter,
-    loaded_data::runtime_types::{StructNameIndex, StructType, Type, TypeBuilder},
+    loaded_data::runtime_types::{Type, TypeBuilder},
     values::{GlobalValue, Value},
 };
-use std::{borrow::Borrow, collections::BTreeSet, sync::Arc};
+use std::{borrow::Borrow, collections::BTreeSet};
 
 pub struct Session<'r, 'l> {
     pub(crate) move_vm: &'l MoveVM,
@@ -527,16 +527,28 @@ impl<'r, 'l> Session<'r, 'l> {
         self.move_vm.runtime.loader().ty_builder()
     }
 
-    pub fn fetch_struct_ty_by_idx(
+    /// If type is a (generic or non-generic) struct or enum, returns its name. Otherwise, returns
+    /// [None].
+    pub fn get_struct_name(
         &self,
-        idx: StructNameIndex,
-        module_storage: &impl ModuleStorage,
-    ) -> Option<Arc<StructType>> {
-        self.move_vm
-            .runtime
-            .loader()
-            .fetch_struct_ty_by_idx(idx, &self.module_store, module_storage)
-            .ok()
+        ty: &Type,
+        module_storage: &dyn ModuleStorage,
+    ) -> PartialVMResult<Option<(ModuleId, Identifier)>> {
+        use Type::*;
+
+        Ok(match ty {
+            Struct { idx, .. } | StructInstantiation { idx, .. } => {
+                let struct_identifier = self
+                    .move_vm
+                    .runtime
+                    .loader()
+                    .struct_name_index_map(module_storage)
+                    .idx_to_struct_name(*idx)?;
+                Some((struct_identifier.module, struct_identifier.name))
+            },
+            Bool | U8 | U16 | U32 | U64 | U128 | U256 | Address | Signer | TyParam(_)
+            | Vector(_) | Reference(_) | MutableReference(_) => None,
+        })
     }
 
     pub fn check_type_tag_dependencies_and_charge_gas(

third_party/move/move-vm/runtime/src/storage/environment.rs

@@ -6,8 +6,8 @@ use crate::{
     loader::check_natives,
     native_functions::{NativeFunction, NativeFunctions},
     storage::{
-        struct_name_index_map::StructNameIndexMap, ty_cache::StructInfoCache,
-        ty_tag_converter::TypeTagCache, verified_module_cache::VERIFIED_MODULES_V2,
+        ty_cache::StructInfoCache, ty_tag_converter::TypeTagCache,
+        verified_module_cache::VERIFIED_MODULES_V2,
     },
     Module, Script,
 };
@@ -26,8 +26,11 @@ use move_core_types::{
     vm_status::{sub_status::unknown_invariant_violation::EPARANOID_FAILURE, StatusCode},
 };
 use move_vm_metrics::{Timer, VM_TIMER};
+use move_vm_types::loaded_data::struct_name_indexing::StructNameIndexMap;
 #[cfg(any(test, feature = "testing"))]
-use move_vm_types::loaded_data::runtime_types::{StructIdentifier, StructNameIndex};
+use move_vm_types::loaded_data::{
+    runtime_types::StructIdentifier, struct_name_indexing::StructNameIndex,
+};
 use std::sync::Arc;
 
 /// [MoveVM] runtime environment encapsulating different configurations. Shared between the VM and

third_party/move/move-vm/runtime/src/storage/mod.rs

@@ -2,7 +2,6 @@
 // SPDX-License-Identifier: Apache-2.0
 
 pub(crate) mod loader;
-pub(crate) mod struct_name_index_map;
 pub(crate) mod ty_cache;
 pub(crate) mod ty_tag_converter;
 mod verified_module_cache;

third_party/move/move-vm/runtime/src/storage/ty_cache.rs

@@ -1,10 +1,12 @@
 // Copyright (c) The Move Contributors
 // SPDX-License-Identifier: Apache-2.0
 
-use crate::storage::struct_name_index_map::StructNameIndexMap;
 use move_binary_format::errors::{PartialVMError, PartialVMResult};
 use move_core_types::{language_storage::StructTag, value::MoveTypeLayout, vm_status::StatusCode};
-use move_vm_types::loaded_data::runtime_types::{DepthFormula, StructNameIndex, Type};
+use move_vm_types::loaded_data::{
+    runtime_types::{DepthFormula, Type},
+    struct_name_indexing::{StructNameIndex, StructNameIndexMap},
+};
 use parking_lot::RwLock;
 
 /// Layout information of a single struct instantiation.

third_party/move/move-vm/runtime/src/storage/ty_layout_converter.rs

@@ -4,10 +4,7 @@
 use crate::{
     config::VMConfig,
     loader::{LegacyModuleStorageAdapter, Loader, PseudoGasContext, VALUE_DEPTH_MAX},
-    storage::{
-        struct_name_index_map::StructNameIndexMap, ty_cache::StructInfoCache,
-        ty_tag_converter::TypeTagConverter,
-    },
+    storage::{ty_cache::StructInfoCache, ty_tag_converter::TypeTagConverter},
     ModuleStorage,
 };
 use move_binary_format::errors::{PartialVMError, PartialVMResult};
@@ -16,7 +13,10 @@ use move_core_types::{
     value::{IdentifierMappingKind, MoveFieldLayout, MoveStructLayout, MoveTypeLayout},
     vm_status::StatusCode,
 };
-use move_vm_types::loaded_data::runtime_types::{StructLayout, StructNameIndex, StructType, Type};
+use move_vm_types::loaded_data::{
+    runtime_types::{StructLayout, StructType, Type},
+    struct_name_indexing::{StructNameIndex, StructNameIndexMap},
+};
 use std::sync::Arc;
 
 /// Maximal nodes which are allowed when converting to layout. This includes the types of