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

wrwg · 2025-01-04T22:56:15Z

Description

[PR 4/n vm closures]

This implements types and values for functions and closures, as well as serialization. For a detailed discussion, see the design doc. It does not yet implement the interpreter and runtime verification.

Overview:

MoveValue and MoveTypeLayout are extended to represent closures and functions. A closure carries the function name, the closure mask, the layout of the captured arguments, and their values. The layout enables serialization of the captured arguments without any context.
Runtime Type is extended by functions.
ValueImpl is extended by a closure variant. This representation uses trait AbstractFunction to represent the function value and implement core functionality like comparison and printing. The existing trait FunctionValueExtension is used to create AbstractFunction from the serialized representation.
Similar as with MoveValue, the serialized representation of ValueImpl::Closure contains the captured parameter layouts, enabling to deserialize the captured arguments context independently. This design has been chosen for robustness, avoiding a dependency of serialization from loaded functions, and to enable fully 'lazy' deserialization of closures. The later allows a contract to store hundreds of function values and on loading them from storage, avoiding loading the associated code until the moment the closure is actually executed.
AbstractFunction is implemented in the loader such that it can be either in 'unresolved' or in `resolved' state.

How Has This Been Tested?

Testing postponed until e2e wiring is up

Type of Change

Which Components or Systems Does This Change Impact?

trunk-io · 2025-01-04T22:56:18Z

⏱️ 1h 6m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-cargo-deny	9m	🟩 ⬜ 🟩 🟩 🟩 (+1 more)
rust-move-tests	8m	🟥
rust-move-tests	8m	🟥
check-dynamic-deps	6m	🟩 🟩 🟩 🟩 🟩 (+1 more)
general-lints	3m	🟩 🟩 🟩 🟩 🟩 (+1 more)
rust-move-tests	2m	🟥
semgrep/ci	2m	🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+1 more)
rust-move-tests	29s	⬜
permission-check	17s	🟩 🟩 🟩 🟩 🟩 (+1 more)
permission-check	12s	🟩 🟩 🟩 🟩 🟩 (+1 more)
check-branch-prefix	1s	🟩

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

wrwg · 2025-01-04T22:56:35Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

third_party/move/move-core/types/src/function.rs

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

wrwg

Thanks for the reviews! All comments of this round addressed.

third_party/move/move-core/types/src/function.rs

third_party/move/move-vm/types/src/loaded_data/runtime_types.rs

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

[PR 3/n vm closures] This implements types and values for functions and closures, as well as serialization. For a detailed discussion, see the design doc. It does not yet implement the interpreter and runtime verification. Overview: - `MoveValue` and `MoveTypeLayout` are extended to represent closures and functions. A closure carries the function name, the closure mask, the function layout, and the captured arguments. The function layout enables serialization of the captured arguments without any context. - Runtime `Type` is extended by functions. - `ValueImpl` is extended by a closure variant. This representation uses `trait AbstractFunction` to represent the function value and implement core functionality like comparison and printing. The existing trait `FunctionValueExtension` is used to create `AbstracFunction` from the serialized representation. - Similar as with `MoveValue`, the serialized representation of `ValueImpl::Closure` contains the full function layout, enabling to deserialize the captured arguments context independently. This design has been chosen for robustness, avoiding a dependency of serialization from loaded functions, and to enable fully 'lazy' deserialization of closures. The later allows a contract to store hundreds of function values and on loading them from storage, avoiding loading the associated code until the moment the closure is actually executed. - `AbstractFunction` is implemented in the loader such that it can be either in 'unresolved' or in `resolved' state.

georgemitenkov · 2025-02-05T16:42:43Z

aptos-move/aptos-gas-schedule/src/gas_schedule/misc.rs

@@ -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?


@vgao1996 can you look at gas related aspects?

api/types/src/move_types.rs

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

aptos-move/framework/src/natives/string_utils.rs

georgemitenkov · 2025-02-05T16:58:38Z

aptos-move/framework/src/natives/string_utils.rs

+            out.push_str(&fun.to_stable_string());
+            format_vector(
+                context,
+                data.captured_layouts.iter(),


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, });

third_party/move/move-vm/types/src/loaded_data/runtime_types.rs

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

third_party/move/tools/move-resource-viewer/src/lib.rs

third_party/move/move-core/types/src/function.rs

third_party/move/move-core/types/src/value.rs

runtian-zhou

I think we can already add tests for closure de/serialization. We have two representations here: MoveClosure and ValueImpl and we can implement some round trip tests there

runtian-zhou · 2025-02-07T19:02:00Z

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

@@ -220,7 +220,7 @@ pub(crate) fn is_valid_txn_arg(
                    ))
                })
        },
-        Signer | Reference(_) | MutableReference(_) | TyParam(_) => false,
+        Signer | Reference(_) | MutableReference(_) | TyParam(_) | Function { .. } => false,


This change seems unneeded? Maybe affected by rebase?

runtian-zhou · 2025-02-07T19:25:47Z

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

+        let converter = StorageLayoutConverter::new(module_storage);
+        if captured_arg_types.len() != captured_layouts.len() {
+            return Err(
+                PartialVMError::new(StatusCode::FUNCTION_RESOLUTION_FAILURE).with_message(


I‘d prefer not having ok but return error immediately here and at line 235

runtian-zhou · 2025-02-07T19:28:21Z

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

+    /// immutable (public), also store.
+    pub fn abilities(&self) -> AbilitySet {
+        let result = AbilitySet::singleton(Ability::Copy).add(Ability::Drop);
+        if !self.is_friend_or_private {


Do you need to check the captured types as well?

runtian-zhou · 2025-02-07T19:50:51Z

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

        })
    }
 }

 // Matches the actual returned type to the expected type, binding any type args to the
 // necessary type as stored in the map. The expected type must be a concrete type (no TyParam).
 // Returns true if a successful match is made.
+// TODO: is this really needed in presence of paranoid mode? This does a deep structural
+//   comparison and is expensive.
 fn match_return_type<'a>(


IIRC this function is needed for handling entry function with returned types needed by Sui, we didn't need such functionality and can probably clean up such logic later

runtian-zhou

Overall LGTM. I felt this PR needs some unit tests on ser/de logic as they can be tested rightaway.

runtian-zhou

Also changes in the VM seems to be gated reasonably so we should be safe with merging it.

This was referenced Jan 4, 2025

[move-vm][closures] Types and opcodes for closures #15667

Merged

[move-vm][closures] Refactor: move abilities and closure mask into core types #15668

Merged

wrwg mentioned this pull request Jan 4, 2025

[move-cm][closures] Refactor: Move type conversions out of Loader into a trait #15669

Merged

16 tasks

wrwg force-pushed the wrwg/clos_type_conv branch from 4972a5c to 2fed681 Compare January 5, 2025 05:50

wrwg force-pushed the wrwg/clos_values branch from c1ff3d8 to 4cf0964 Compare January 5, 2025 05:50

wrwg force-pushed the wrwg/clos_type_conv branch from 2fed681 to 9c0814c Compare January 6, 2025 02:47

wrwg force-pushed the wrwg/clos_values branch from 4cf0964 to 60546b9 Compare January 6, 2025 02:47

wrwg force-pushed the wrwg/clos_type_conv branch from 9c0814c to 8b11451 Compare January 6, 2025 02:47

wrwg force-pushed the wrwg/clos_values branch from 60546b9 to f305df7 Compare January 6, 2025 02:48

wrwg force-pushed the wrwg/clos_type_conv branch from 8b11451 to dcb6bc5 Compare January 6, 2025 03:25

wrwg force-pushed the wrwg/clos_values branch 2 times, most recently from 95728c9 to 6eea07c Compare January 7, 2025 04:04

wrwg mentioned this pull request Jan 7, 2025

[move-vm][closures] Interpreter #15680

Open

16 tasks

wrwg requested review from georgemitenkov, runtian-zhou, vgao1996, vineethk and ziaptos January 8, 2025 06:20

wrwg marked this pull request as ready for review January 8, 2025 06:20

wrwg requested review from davidiw, movekevin, zekun000, banool, gregnazario and 0xmaayan as code owners January 8, 2025 06:20

wrwg force-pushed the wrwg/clos_type_conv branch from dcb6bc5 to bfc6f4c Compare January 13, 2025 07:08

wrwg force-pushed the wrwg/clos_values branch from 6eea07c to 06363e0 Compare January 13, 2025 07:08

wrwg force-pushed the wrwg/clos_type_conv branch from bfc6f4c to 355525d Compare January 14, 2025 02:32

runtian-zhou reviewed Jan 24, 2025

View reviewed changes

third_party/move/move-core/types/src/function.rs Outdated Show resolved Hide resolved

runtian-zhou reviewed Jan 24, 2025

View reviewed changes

third_party/move/move-vm/runtime/src/loader/function.rs Outdated Show resolved Hide resolved

wrwg force-pushed the wrwg/clos_type_conv branch from 6f6afe3 to f178823 Compare January 25, 2025 01:07

wrwg force-pushed the wrwg/clos_values branch from e6560c5 to b3492b5 Compare January 25, 2025 01:07

wrwg force-pushed the wrwg/clos_type_conv branch from f178823 to 2707a22 Compare January 25, 2025 06:39

Base automatically changed from wrwg/clos_type_conv to main January 25, 2025 07:19

wrwg commented Jan 27, 2025

View reviewed changes

wrwg force-pushed the wrwg/clos_values branch from b3492b5 to c668c04 Compare January 27, 2025 04:12

wrwg requested review from georgemitenkov and runtian-zhou January 27, 2025 05:42

wrwg force-pushed the wrwg/clos_values branch 2 times, most recently from 3864e40 to cfde21d Compare January 27, 2025 23:52

wrwg added 4 commits February 4, 2025 21:39

Adding tests for closure value serialization and closure masks.

140dce0

Addressing more reviewer comments.

fad9573

Removing resolution_error cache.

10b1041

wrwg force-pushed the wrwg/clos_values branch from cfde21d to 70bf138 Compare February 5, 2025 05:40

Update serde format test -- we are not breaking but extending yaml apis

41baffe

wrwg force-pushed the wrwg/clos_values branch from 70bf138 to 41baffe Compare February 5, 2025 15:51

georgemitenkov reviewed Feb 5, 2025

View reviewed changes

third_party/move/move-core/types/src/function.rs Show resolved Hide resolved

third_party/move/move-core/types/src/function.rs Show resolved Hide resolved

third_party/move/move-core/types/src/value.rs Show resolved Hide resolved

wrwg added 2 commits February 5, 2025 20:37

Addressing reviewer comments

b3eebaf

Addressing more reviewer comments

7fe820f

wrwg requested a review from georgemitenkov February 6, 2025 05:20

runtian-zhou reviewed Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wrwg commented Jan 4, 2025 •

edited

Loading

trunk-io bot commented Jan 4, 2025 •

edited

Loading

wrwg commented Jan 4, 2025 •

edited

Loading

wrwg left a comment

georgemitenkov Feb 5, 2025

georgemitenkov Feb 5, 2025

runtian-zhou left a comment

runtian-zhou Feb 7, 2025

runtian-zhou Feb 7, 2025

runtian-zhou Feb 7, 2025

runtian-zhou Feb 7, 2025

runtian-zhou left a comment

runtian-zhou left a comment

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

Are you sure you want to change the base?

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

Conversation

wrwg commented Jan 4, 2025 • edited Loading

Description

How Has This Been Tested?

Type of Change

Which Components or Systems Does This Change Impact?

trunk-io bot commented Jan 4, 2025 • edited Loading

wrwg commented Jan 4, 2025 • edited Loading

wrwg left a comment

Choose a reason for hiding this comment

georgemitenkov Feb 5, 2025

Choose a reason for hiding this comment

georgemitenkov Feb 5, 2025

Choose a reason for hiding this comment

runtian-zhou left a comment

Choose a reason for hiding this comment

runtian-zhou Feb 7, 2025

Choose a reason for hiding this comment

runtian-zhou Feb 7, 2025

Choose a reason for hiding this comment

runtian-zhou Feb 7, 2025

Choose a reason for hiding this comment

runtian-zhou Feb 7, 2025

Choose a reason for hiding this comment

runtian-zhou left a comment

Choose a reason for hiding this comment

runtian-zhou left a comment

Choose a reason for hiding this comment

wrwg commented Jan 4, 2025 •

edited

Loading

trunk-io bot commented Jan 4, 2025 •

edited

Loading

wrwg commented Jan 4, 2025 •

edited

Loading