Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[move-vm][closures] Type and Value representation and serialization #15670

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+1,545 −110
Open

[move-vm][closures] Type and Value representation and serialization #15670

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
94627e3
[move-vm][closures] Type and Value representation and serialization
wrwg Dec 31, 2024
140dce0
Adding tests for closure value serialization and closure masks.
wrwg Jan 27, 2025
fad9573
Addressing more reviewer comments.
wrwg Jan 27, 2025
10b1041
Removing resolution_error cache.
wrwg Jan 27, 2025
41baffe
Update serde format test -- we are not breaking but extending yaml apis
wrwg Jan 27, 2025
b3eebaf
Addressing reviewer comments
wrwg Feb 6, 2025
7fe820f
Addressing more reviewer comments
wrwg Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions api/types/src/convert.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -892,6 +892,11 @@ impl<'a, S: StateView> MoveConverter<'a, S> {
MoveTypeLayout::Struct(struct_layout) => {
self.try_into_vm_value_struct(struct_layout, val)?
},
MoveTypeLayout::Function(..) => {
// TODO(#15664): do we actually need this? It appears the code here is dead and
wrwg marked this conversation as resolved.
Show resolved Hide resolved
// nowhere used
bail!("unexpected move type {:?} for value {:?}", layout, val)
},

// Some values, e.g., signer or ones with custom serialization
// (native), are not stored to storage and so we do not expect
Expand Down
8 changes: 8 additions & 0 deletions api/types/src/move_types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,6 +681,10 @@ impl From<TypeTag> for MoveType {
items: Box::new(MoveType::from(*v)),
},
TypeTag::Struct(v) => MoveType::Struct((*v).into()),
TypeTag::Function(..) => {
// TODO(#15664): determine whether functions and closures need to be supported
panic!("functions not supported by API types")
},
}
}
}
Expand All @@ -701,6 +705,10 @@ impl From<&TypeTag> for MoveType {
items: Box::new(MoveType::from(v.as_ref())),
},
TypeTag::Struct(v) => MoveType::Struct((&**v).into()),
TypeTag::Function(..) => {
wrwg marked this conversation as resolved.
Show resolved Hide resolved
// TODO(#15664): determine whether functions and closures need to be supported
panic!("functions not supported by API types")
},
}
}
}
Expand Down
26 changes: 26 additions & 0 deletions aptos-move/aptos-gas-schedule/src/gas_schedule/misc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,11 @@ where
self.offset = 1;
true
}

#[inline]
fn visit_closure(&mut self, depth: usize, len: usize) -> bool {
self.inner.visit_closure(depth, len)
}
}

struct AbstractValueSizeVisitor<'a> {
Expand Down Expand Up @@ -200,6 +205,13 @@ impl<'a> ValueVisitor for AbstractValueSizeVisitor<'a> {
true
}

#[inline]
fn visit_closure(&mut self, _depth: usize, _len: usize) -> bool {
// TODO(#15664): introduce a dedicated gas parameter?
self.size += self.params.struct_;
true
}

#[inline]
fn visit_vec(&mut self, _depth: usize, _len: usize) -> bool {
self.size += self.params.vector;
Expand Down Expand Up @@ -366,6 +378,13 @@ impl AbstractValueSizeGasParameters {
false
}

#[inline]
fn visit_closure(&mut self, _depth: usize, _len: usize) -> bool {
// TODO(#15664): independent gas parameter for closures?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vgao1996 can you look at gas related aspects?

self.res = Some(self.params.struct_);
false
}

#[inline]
fn visit_vec(&mut self, _depth: usize, _len: usize) -> bool {
self.res = Some(self.params.vector);
Expand Down Expand Up @@ -509,6 +528,13 @@ impl AbstractValueSizeGasParameters {
false
}

#[inline]
fn visit_closure(&mut self, _depth: usize, _len: usize) -> bool {
// TODO(#15664): independent gas parameter
self.res = Some(self.params.struct_);
false
}

#[inline]
fn visit_vec(&mut self, _depth: usize, _len: usize) -> bool {
self.res = Some(self.params.vector);
Expand Down
4 changes: 2 additions & 2 deletions aptos-move/aptos-sdk-builder/src/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ fn quote_type_as_format(type_tag: &TypeTag) -> Format {
tag if &**tag == Lazy::force(&str_tag) => Format::Seq(Box::new(Format::U8)),
_ => type_not_allowed(type_tag),
},
Signer => type_not_allowed(type_tag),
Signer | Function(..) => type_not_allowed(type_tag),
}
}

Expand Down Expand Up @@ -122,7 +122,7 @@ pub(crate) fn mangle_type(type_tag: &TypeTag) -> String {
tag if &**tag == Lazy::force(&str_tag) => "string".into(),
_ => type_not_allowed(type_tag),
},
Signer => type_not_allowed(type_tag),
Signer | Function(..) => type_not_allowed(type_tag),
}
}

Expand Down
8 changes: 4 additions & 4 deletions aptos-move/aptos-sdk-builder/src/golang.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,7 @@ func encode_{}_argument(arg {}) []byte {{
U8 => ("U8Vector", default_stmt),
_ => common::type_not_allowed(type_tag),
},
Struct(_) | Signer => common::type_not_allowed(type_tag),
Struct(_) | Signer | Function(..) => common::type_not_allowed(type_tag),
};
writeln!(
self.out,
Expand Down Expand Up @@ -793,7 +793,7 @@ func decode_{0}_argument(arg aptostypes.TransactionArgument) (value {1}, err err
tag if &**tag == Lazy::force(&str_tag) => "[]uint8".into(),
_ => common::type_not_allowed(type_tag),
},
Signer => common::type_not_allowed(type_tag),
Signer | Function(..) => common::type_not_allowed(type_tag),
}
}

Expand All @@ -820,7 +820,7 @@ func decode_{0}_argument(arg aptostypes.TransactionArgument) (value {1}, err err
U8 => format!("(*aptostypes.TransactionArgument__U8Vector)(&{})", name),
_ => common::type_not_allowed(type_tag),
},
Struct(_) | Signer => common::type_not_allowed(type_tag),
Struct(_) | Signer | Function(..) => common::type_not_allowed(type_tag),
}
}

Expand Down Expand Up @@ -854,7 +854,7 @@ func decode_{0}_argument(arg aptostypes.TransactionArgument) (value {1}, err err
tag if &**tag == Lazy::force(&str_tag) => Some("Bytes"),
_ => common::type_not_allowed(type_tag),
},
Signer => common::type_not_allowed(type_tag),
Signer | Function(..) => common::type_not_allowed(type_tag),
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions aptos-move/aptos-sdk-builder/src/rust.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,7 +737,7 @@ static SCRIPT_FUNCTION_DECODER_MAP: once_cell::sync::Lazy<EntryFunctionDecoderMa
U8 => ("U8Vector", "Some(value)".to_string()),
_ => common::type_not_allowed(type_tag),
},
Struct(_) | Signer => common::type_not_allowed(type_tag),
Struct(_) | Signer | Function(..) => common::type_not_allowed(type_tag),
};
writeln!(
self.out,
Expand Down Expand Up @@ -880,7 +880,7 @@ fn decode_{}_argument(arg: TransactionArgument) -> Option<{}> {{
tag if &**tag == Lazy::force(&str_tag) => "Vec<u8>".into(),
_ => common::type_not_allowed(type_tag),
},
Signer => common::type_not_allowed(type_tag),
Signer | Function(..) => common::type_not_allowed(type_tag),
}
}

Expand Down Expand Up @@ -909,7 +909,7 @@ fn decode_{}_argument(arg: TransactionArgument) -> Option<{}> {{
_ => common::type_not_allowed(type_tag),
},

Struct(_) | Signer => common::type_not_allowed(type_tag),
Struct(_) | Signer | Function(..) => common::type_not_allowed(type_tag),
}
}
}
Expand Down
10 changes: 7 additions & 3 deletions aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ pub(crate) fn is_valid_txn_arg(
))
})
},
Signer | Reference(_) | MutableReference(_) | TyParam(_) => false,
Signer | Reference(_) | MutableReference(_) | TyParam(_) | Function { .. } => false,
wrwg marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems unneeded? Maybe affected by rebase?

}
}

Expand Down Expand Up @@ -312,7 +312,9 @@ fn construct_arg(
Err(invalid_signature())
}
},
Reference(_) | MutableReference(_) | TyParam(_) => Err(invalid_signature()),
Reference(_) | MutableReference(_) | TyParam(_) | Function { .. } => {
Err(invalid_signature())
},
}
}

Expand Down Expand Up @@ -385,7 +387,9 @@ pub(crate) fn recursively_construct_arg(
U64 => read_n_bytes(8, cursor, arg)?,
U128 => read_n_bytes(16, cursor, arg)?,
U256 | Address => read_n_bytes(32, cursor, arg)?,
Signer | Reference(_) | MutableReference(_) | TyParam(_) => return Err(invalid_signature()),
Signer | Reference(_) | MutableReference(_) | TyParam(_) | Function { .. } => {
return Err(invalid_signature())
},
};
Ok(())
}
Expand Down
5 changes: 3 additions & 2 deletions aptos-move/framework/move-stdlib/src/natives/bcs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,10 +193,11 @@ fn constant_serialized_size(ty_layout: &MoveTypeLayout) -> (u64, PartialVMResult
MoveTypeLayout::Signer => Ok(None),
// vectors have no constant size
MoveTypeLayout::Vector(_) => Ok(None),
// enums have no constant size
// enums and functions have no constant size
MoveTypeLayout::Struct(
MoveStructLayout::RuntimeVariants(_) | MoveStructLayout::WithVariants(_),
) => Ok(None),
)
| MoveTypeLayout::Function(..) => Ok(None),
MoveTypeLayout::Struct(MoveStructLayout::Runtime(fields)) => {
let mut total = Some(0);
for field in fields {
Expand Down
24 changes: 21 additions & 3 deletions aptos-move/framework/src/natives/string_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use move_core_types::{
use move_vm_runtime::native_functions::NativeFunction;
use move_vm_types::{
loaded_data::runtime_types::Type,
values::{Reference, Struct, Value, Vector, VectorRef},
values::{Closure, Reference, Struct, Value, Vector, VectorRef},
};
use smallvec::{smallvec, SmallVec};
use std::{collections::VecDeque, fmt::Write, ops::Deref};
Expand Down Expand Up @@ -350,9 +350,27 @@ fn native_format_impl(
)?;
out.push('}');
},
MoveTypeLayout::Function(_) => {
// TODO(#15664): The captured layouts are not decorated, do we actually and
wrwg marked this conversation as resolved.
Show resolved Hide resolved
// if so, how, print this?
let (fun, args) = val.value_as::<Closure>()?.unpack();
let data = context
.context
.function_value_extension()
.get_serialization_data(fun.as_ref())?;
out.push_str(&fun.to_stable_string());
format_vector(
context,
data.captured_layouts.iter(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Captured layouts will not be annotated, right? Inconsistent with other printing, we might want to revisit this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest avoid formatting unless there's a certain ask?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should revisit this in practice. I don't want to omit information in the first step, its not breaking to change this later, I think we can use this only in tests? In any case, I put the generic bug here for revisiting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to return an error here instead of doing this? get_serialization_data is also not cheap for resolved functions with all conversions, I do not see a value of printing partial information + we should be charging gas here like we do for structs. If we want to keep it in tests, let's put it under cfg(test) etc, and otherwise return an error, e.g.

return Err(SafeNativeError::Abort {
  abort_code: EINVALID_FORMAT,
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought a bit more about this, and want to keep this.

Many typically use cases of capturing, for example f(@0xcafe, _), will be printing fine. Only field names are omitted. All experience in debugging tells me we would only create frustration for users if we omitting something here.

The experience to see closures printed by function name plus undecorated captured values will be also all over our ecosystem. MoveValue which is used outside of the VM does the same, the JSON representation is the same. It is not realistic for those parts of the code to look up the function for the full types, so I think we also need not to do this here.

Regards gas, this does exactly the same as for structs. For the structs, layout is constructed in the native top-level call without gas charge, then elements are visisted and each single one is charged. For closures the lookup for layout maybe just a bit delayed, otherwise the charge is the same. Layout is the only relevant cost for get_serialization_data because other info is already there. No code will be loaded for unresolved closures.

args.collect(),
depth,
!context.single_line,
out,
)?;
out.push(')');
},

// This is unreachable because we check layout at the start. Still, return
// an error to be safe.
// Return error for native types
MoveTypeLayout::Native(..) => {
return Err(SafeNativeError::Abort {
abort_code: EUNABLE_TO_FORMAT_DELAYED_FIELD,
Expand Down
15 changes: 11 additions & 4 deletions aptos-move/script-composer/src/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ pub(crate) fn import_type_tag(
type_tag: &TypeTag,
module_resolver: &BTreeMap<ModuleId, CompiledModule>,
) -> PartialVMResult<SignatureToken> {
let to_list = |script_builder: &mut CompiledScriptBuilder, ts: &[TypeTag]| {
ts.iter()
.map(|t| import_type_tag(script_builder, t, module_resolver))
.collect::<PartialVMResult<Vec<_>>>()
};
Ok(match type_tag {
TypeTag::Address => SignatureToken::Address,
TypeTag::U8 => SignatureToken::U8,
Expand All @@ -53,13 +58,15 @@ pub(crate) fn import_type_tag(
} else {
SignatureToken::StructInstantiation(
struct_idx,
s.type_args
.iter()
.map(|ty| import_type_tag(script_builder, ty, module_resolver))
.collect::<PartialVMResult<Vec<_>>>()?,
to_list(script_builder, &s.type_args)?,
)
}
},
TypeTag::Function(f) => SignatureToken::Function(
to_list(script_builder, &f.args)?,
to_list(script_builder, &f.results)?,
f.abilities,
),
})
}

Expand Down
8 changes: 7 additions & 1 deletion testsuite/fuzzer/fuzz/fuzz_targets/move/utils/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
use aptos_language_e2e_tests::{account::Account, executor::FakeExecutor};
use arbitrary::Arbitrary;
use move_binary_format::file_format::CompiledModule;
use move_core_types::value::{MoveStructLayout, MoveTypeLayout};
use move_core_types::{
function::MoveFunctionLayout,
value::{MoveStructLayout, MoveTypeLayout},
};

#[macro_export]
macro_rules! tdbg {
Expand Down Expand Up @@ -82,6 +85,9 @@ pub(crate) fn is_valid_layout(layout: &MoveTypeLayout) -> bool {
}
fields.iter().all(is_valid_layout)
},
L::Function(MoveFunctionLayout(args, results, _)) => {
args.iter().chain(results).all(is_valid_layout)
},
L::Struct(_) => {
// decorated layouts not supported
false
Expand Down
16 changes: 16 additions & 0 deletions testsuite/generate-format/tests/staged/api.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
---
AbilitySet:
NEWTYPESTRUCT: U8
AbortInfo:
STRUCT:
- reason_name: STR
Expand Down Expand Up @@ -341,6 +343,16 @@ FunctionInfo:
TYPENAME: AccountAddress
- module_name: STR
- function_name: STR
FunctionTag:
STRUCT:
- args:
SEQ:
TYPENAME: TypeTag
- results:
SEQ:
TYPENAME: TypeTag
- abilities:
TYPENAME: AbilitySet
G1Bytes:
NEWTYPESTRUCT:
TUPLEARRAY:
Expand Down Expand Up @@ -782,6 +794,10 @@ TypeTag:
u32: UNIT
10:
u256: UNIT
11:
Function:
NEWTYPE:
TYPENAME: FunctionTag
ValidatorTransaction:
ENUM:
0:
Expand Down
16 changes: 16 additions & 0 deletions testsuite/generate-format/tests/staged/aptos.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
---
AbilitySet:
NEWTYPESTRUCT: U8
AbstractionAuthData:
ENUM:
0:
Expand Down Expand Up @@ -287,6 +289,16 @@ FunctionInfo:
TYPENAME: AccountAddress
- module_name: STR
- function_name: STR
FunctionTag:
STRUCT:
- args:
SEQ:
TYPENAME: TypeTag
- results:
SEQ:
TYPENAME: TypeTag
- abilities:
TYPENAME: AbilitySet
G1Bytes:
NEWTYPESTRUCT:
TUPLEARRAY:
Expand Down Expand Up @@ -667,6 +679,10 @@ TypeTag:
u32: UNIT
10:
u256: UNIT
11:
Function:
NEWTYPE:
TYPENAME: FunctionTag
ValidatorTransaction:
ENUM:
0:
Expand Down
Loading
Loading