[aptos-vm][move] Avoid module loads when getting the struct name #15681
Conversation
⏱️ 51m total CI duration on this PR
|
georgemitenkov
requested review from
runtian-zhou,
vgao1996,
gelash and
ziaptos
georgemitenkov
requested review from
davidiw,
wrwg and
zekun000
as code owners
| .move_vm | ||
| .runtime | ||
| .loader() | ||
| .struct_name_index_map(module_storage) |
There was a problem hiding this comment.
what if the module contains the struct is not loaded?
There was a problem hiding this comment.
The module cannot contain non-loaded struct? Or what do you mean?
My goal here is to do the following:
- If we have a struct
Type, it contains an index. - The only way to create the index is via
struct_name_index_map(I guess having a public inner field + constructor is a problem you are referring to? It might make sense to enforce that we can construct indices only via the map, or it should be test-only construction) - Hence, we must get the correct indexed name.
There was a problem hiding this comment.
@zekun000 updated the code so that struct name index has private constructor and only can be got via indexed map.
There was a problem hiding this comment.
It also assumes that the map can never be cleared or things removed, right?
There was a problem hiding this comment.
Yes, but that is also true for other caches? We stored indices in Type::Struct { .. }s, and these types can be in per-frame cache, etc. cached by interpreter. If struct name index map is flashed during the transaction execution, we have dangling indices.
georgemitenkov
force-pushed
the
george/struct-names
branch
from
976cd65 to
b56edda
Compare
| use move_vm_types::loaded_data::runtime_types::Type::*; | ||
|
|
||
| match ty { | ||
| Ok(match ty { |
There was a problem hiding this comment.
The comment of the fn says "whether argument is valid / allowed and whether it needs construction"
Is it an actual &? so true means it is valid and does not need construction? Let's clarify the comment (maybe also adding error condition)
There was a problem hiding this comment.
No, true means it is valid, e.g., an integer, or struct has a name for which we have a constructor. Updated the comment to make it more clear.
| Struct { .. } | StructInstantiation { .. } => { | ||
| let (module_id, identifier) = session | ||
| .get_struct_name(ty, module_storage) | ||
| .map_err(|_| invalid_signature())? |
There was a problem hiding this comment.
Is this correct - i.e. invalid_signature mapping?
There was a problem hiding this comment.
The code was using .ok_or_else(invalid_signature)?; before, so for compatibility I just re-used the mapping. The code is a mess and probably we need to rewrite it in any case at some point, without this kind of remapping, or at least with clear error messages.
| .move_vm | ||
| .runtime | ||
| .loader() | ||
| .struct_name_index_map(module_storage) |
There was a problem hiding this comment.
It also assumes that the map can never be cleared or things removed, right?
Description
For transaction argument validation, we check if the struct type name matches the allowed constructor. Previously, we were loading modules to check that, which is unnecessary because we can get the name from the struct re-indexing map. This PR changes the check to this.
How Has This Been Tested?
Existing tests.
Key Areas to Review
Double-check the behaviour is the same.
Type of Change
Which Components or Systems Does This Change Impact?
Checklist