[move][stdlib] Implement mem::swap native move call #14786

igor-aptos · 2024-09-27T23:01:34Z

Description

If we have a field that contains non-copyable type, it is impossible to change it, and get the old value back.

Adding two methods:

native mem::swap, that implements swap of contents of two mutable references
mem::replace, as a simple wrapper based on mem::swap

How Has This Been Tested?

provided unit tests

Type of Change

New feature

Which Components or Systems Does This Change Impact?

Move/Aptos Virtual Machine

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-09-27T23:01:38Z

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

Slowest 15 Jobs	Cumulative Duration	Recent Runs
rust-move-unit-coverage	16m	🟩
rust-move-unit-coverage	11m	🟩
rust-move-unit-coverage	10m	🟩
rust-move-tests	10m	🟩
rust-move-tests	10m	🟩
rust-move-unit-coverage	10m	🟩
rust-move-tests	10m	🟥
rust-move-tests	9m	🟥
rust-cargo-deny	7m	🟩 🟩 🟩 🟩
check-dynamic-deps	4m	🟩 🟩 🟩 🟩
general-lints	3m	🟩 🟩 🟩 🟩
semgrep/ci	2m	🟩 🟩 🟩 🟩
file_change_determinator	52s	🟩 🟩 🟩 🟩
permission-check	22s	🟩 🟩 🟩 🟩 🟩
file_change_determinator	20s	🟩 🟩

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

codecov · 2024-09-27T23:12:00Z

Codecov Report

Attention: Patch coverage is 80.08658% with 46 lines in your changes missing coverage. Please review.

Project coverage is 60.1%. Comparing base (0e4f5df) to head (8b0d35b).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...party/move/move-vm/types/src/values/values_impl.rs	72.5%	37 Missing ⚠️
...ptos-move/framework/move-stdlib/src/natives/mem.rs	75.0%	9 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #14786    +/-   ##
========================================
  Coverage    60.1%    60.1%            
========================================
  Files         856      857     +1     
  Lines      211110   211341   +231     
========================================
+ Hits       126962   127147   +185     
- Misses      84148    84194    +46

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lightmark · 2024-10-03T19:14:44Z

What's this for?

igor-aptos · 2024-10-03T19:44:01Z

This is needed to be able to modify fields, when they hold a non-copyable non-dropable type.

For example, you cannot do this today:

struct A<T> {
   field: T,
}

public fun swap<T>(self: &mut A<T>, value: T): T {
   let old_value = self.field;  <-- doesn't compile, expects copy
   self.field = value;          <-- doesn't compile, expects drop
   old_value
}

https://aptos-org.slack.com/archives/C05V6JEKZQ9/p1727393314700309

vgao1996 · 2024-10-09T18:46:46Z

aptos-move/framework/move-stdlib/src/natives/mem.rs

+
+    let cost = MEM_SWAP_BASE
+        + MEM_SWAP_PER_ABS_VAL_UNIT
+            * (context.abs_val_size(&args[0]) + context.abs_val_size(&args[1]));


You need to use abstract_value_size_dereferenced as it is the sizes of the referenced values that matter, not the size of the references themselves.
https://github.com/aptos-labs/aptos-core/blob/main/aptos-move/aptos-gas-schedule/src/gas_schedule/misc.rs#L293

You may need to add a helper to expose it through the native context.

Why dereferenced value? We are not allocating new memory space?

Because the current implementation effective makes deep clones of the two values being swapped.

Anyway, this discussion may no longer be relevant as I've come up with a more efficient constant-time implementation. Will share the PR very very soon.

vgao1996

Okay, I ended up just implementing the more efficient swap as I originally promised:

[move-vm] more efficent swap implementation #14921

Please take a careful look and help me check if the implementation is correct. I also just realized that swap is a really powerful feature, and we must take utmost caution to ensure its implementation is correct, or very bad things could happen. For this reason, I'd like to request at least one additional review from @runtian-zhou @ziaptos or @georgemitenkov + a third party audit.

vgao1996 · 2024-10-10T00:27:26Z

aptos-move/framework/move-stdlib/tests/mem_tests.move

@@ -0,0 +1,65 @@
+#[test_only]
+module std::mem_tests {


The tests here are inadequate considering how important and dangerous the swap feature is.

We need to have full test coverage for all these swaps:

(container, container) (indexed ref - generic, indexed ref - generic) (indexed ref - specialized, indexed ref - specialized) (indexed ref - generic, indexed ref - specialized) (indexed ref - specialized, indexed ref - generic)

I would also encourage us add (rust-level) unit tests in move-vm/types/src/values/value_tests.

I added some tests, but can add more.

can you explain when does a &mut become each of the :

indexer ref - generic

indexed ref - specialized

container

vgao1996 · 2024-10-10T00:28:16Z

execution/executor/tests/internal_indexer_test.rs

+        .map(|(s, _)| s)
+        .partition(|s| s.is_aptos_code());
+
+    let expected_code = vec![


Are these changes related? Not familiar with the code here and don't know what it is trying to achieve.

there is a check that all resources published in genesis are indexed correctly.

here we added mem.move, so it failed. failing on 153>152 is confusing.

so I just pulled the list of resources - and am asserting on them, and failure will tell which resource is different.

cc @areshand

vgao1996 · 2024-10-10T00:37:25Z

@wrwg I'm also wondering if we should just make this a bytecode instruction. Can probably do it later though.

igor-aptos

is there a way to unit-test call_native?

i.e. we are relying for a lot of tests in third_party/move/move-vm/types/src/values/value_tests.rs that are not failing checks internally, to fail checks when native call types get checked.

Otherwise I am not sure how to have that test coverage, as in end-to-end tests - such code would not compile.

igor-aptos · 2024-11-08T23:23:21Z

addressed all comments and added more tests

georgemitenkov

Pass 1: everything but the values

georgemitenkov · 2024-10-04T17:13:20Z

aptos-move/framework/move-stdlib/sources/mem.move

@@ -0,0 +1,27 @@
+/// Module with methods for safe memory manipulation.
+/// I.e. swapping/replacing non-copyable/non-droppable types.


nit: I.e. reads a bit weird, maybe For example? Also why break into 2 sentences?

I think the example is just redundant, removing. swap below has the docstring as well.

georgemitenkov · 2024-10-24T04:15:51Z

aptos-move/aptos-gas-schedule/src/ver.rs

@@ -69,7 +69,7 @@
 ///       global operations.
 /// - V1
 ///   - TBA
-pub const LATEST_GAS_FEATURE_VERSION: u64 = gas_feature_versions::RELEASE_V1_22;
+pub const LATEST_GAS_FEATURE_VERSION: u64 = gas_feature_versions::RELEASE_V1_23;


Please add documentation to new 1.23 in the comment above that new natives have been added

aptos-move/aptos-release-builder/src/components/feature_flags.rs

georgemitenkov · 2024-10-24T04:21:48Z

aptos-move/framework/move-stdlib/src/natives/mem.rs

+// Copyright © Aptos Foundation
+// SPDX-License-Identifier: Apache-2.0
+
+// Copyright (c) The Diem Core Contributors


I think this is wrong, multiple licenses and why we even have Diem here?

georgemitenkov · 2024-10-24T04:23:25Z

aptos-move/framework/move-stdlib/src/natives/mem.rs

+
+/// The feature is not enabled.
+/// (0xD is unavailable)
+pub const EFEATURE_NOT_ENABLED: u64 = 0x0D_0001;


Can we document this in Move code? Running into these errors is such a pain because you need to search through aptos-core hoping to get the right constant....

Also, don't we have errors::unavailable Rust helper already? I think it exists in aptos-types though...

types/src/on_chain_config/aptos_features.rs

georgemitenkov · 2024-10-24T04:28:40Z

types/src/on_chain_config/aptos_features.rs

@@ -317,6 +320,10 @@ impl Features {
        self.is_enabled(FeatureFlag::TRANSACTION_SIMULATION_ENHANCEMENT)
    }

+    pub fn is_native_memory_operations_enabled(&self) -> bool {


nit: are instead of is

everything is using is?

It just reads weird to me because it is plural, but maybe a personal taste

georgemitenkov · 2024-10-24T04:32:54Z

third_party/move/move-vm/types/src/values/values_impl.rs

+    fn swap_contents(&self, other: &Self) -> PartialVMResult<()> {
+        use Container::*;
+
+        // TODO: check if unique ownership?


If you add a todo, add a github handle or create a ticket to track this so someone else can have more context about it.

oh this was from Victor's change, missed it. removing, and adding comment.

georgemitenkov · 2024-11-12T17:07:09Z

aptos-move/framework/move-stdlib/sources/mem.move

+    /// Swap contents of two passed mutable references.
+    public(friend) native fun swap<T>(left: &mut T, right: &mut T);
+
+    /// Replace value reference points to with the given new value,


I think articles are missing, but I am not a native speaker 😄

georgemitenkov · 2024-11-12T17:14:41Z

aptos-move/framework/move-stdlib/src/natives/mem.rs

+    debug_assert!(args.len() == 2);
+
+    if args.len() != 2 {
+        return Err(SafeNativeError::InvariantViolation(PartialVMError::new(


Add a message?

aptos-move/framework/move-stdlib/tests/mem_tests.move

igor-aptos

oh I missed your previous review, addressing that one and the new one

aptos-move/aptos-release-builder/src/components/feature_flags.rs

igor-aptos · 2024-11-12T17:41:16Z

aptos-move/framework/move-stdlib/sources/mem.move

@@ -0,0 +1,27 @@
+/// Module with methods for safe memory manipulation.
+/// I.e. swapping/replacing non-copyable/non-droppable types.


I think the example is just redundant, removing. swap below has the docstring as well.

aptos-move/framework/move-stdlib/tests/mem_tests.move

igor-aptos · 2024-11-12T17:52:29Z

third_party/move/move-vm/types/src/values/values_impl.rs

+    fn swap_contents(&self, other: &Self) -> PartialVMResult<()> {
+        use Container::*;
+
+        // TODO: check if unique ownership?


oh this was from Victor's change, missed it. removing, and adding comment.

types/src/on_chain_config/aptos_features.rs

igor-aptos · 2024-11-12T17:54:08Z

types/src/on_chain_config/aptos_features.rs

@@ -317,6 +320,10 @@ impl Features {
        self.is_enabled(FeatureFlag::TRANSACTION_SIMULATION_ENHANCEMENT)
    }

+    pub fn is_native_memory_operations_enabled(&self) -> bool {


everything is using is?

igor-aptos requested review from runtian-zhou, wrwg, georgemitenkov and ziaptos September 27, 2024 23:01

igor-aptos requested review from davidiw and movekevin as code owners September 27, 2024 23:01

wrwg approved these changes Oct 3, 2024

View reviewed changes

igor-aptos force-pushed the igor/move_mem_swap branch from 093a454 to 06d6cb3 Compare October 3, 2024 20:04

igor-aptos requested review from alinush, heliuchuan, msmouse, lightmark, grao1991, hariria, a team, banool, gregnazario, bchocho, sasha8, gelash, zekun000, ibalajiarun, JoshLind and 0xmaayan as code owners October 3, 2024 20:04

igor-aptos changed the base branch from igor/native_compare to main October 3, 2024 20:04

igor-aptos removed request for hariria and zekun000 October 3, 2024 20:05

igor-aptos force-pushed the igor/move_mem_swap branch 4 times, most recently from 6db4bf8 to bd33c50 Compare October 8, 2024 19:00

vgao1996 reviewed Oct 9, 2024

View reviewed changes

igor-aptos force-pushed the igor/move_mem_swap branch 2 times, most recently from 783b338 to 3d7877e Compare October 10, 2024 00:07

vgao1996 requested changes Oct 10, 2024

View reviewed changes

igor-aptos force-pushed the igor/move_mem_swap branch from efbef47 to 7c0fbc1 Compare October 10, 2024 18:09

igor-aptos requested a review from vgao1996 October 11, 2024 05:37

igor-aptos force-pushed the igor/move_mem_swap branch 3 times, most recently from c5f381e to 8b0d35b Compare October 16, 2024 21:17

igor-aptos commented Oct 21, 2024

View reviewed changes

igor-aptos force-pushed the igor/move_mem_swap branch from 8b0d35b to 170eca3 Compare October 21, 2024 23:10

igor-aptos added 5 commits November 8, 2024 09:43

[move][stdlib] Implement mem::swap native move call

3d2870b

[move-vm] more efficent swap implementation

e3bb087

fix swap between same IndexedRef

b46899a

unit test and mark dirty

7984b0a

minor fixes

a99eccd

igor-aptos force-pushed the igor/move_mem_swap branch from 170eca3 to a99eccd Compare November 8, 2024 23:21

georgemitenkov reviewed Nov 12, 2024

View reviewed changes

aptos-move/framework/move-stdlib/tests/mem_tests.move Show resolved Hide resolved

igor-aptos commented Nov 12, 2024

View reviewed changes

addressing comments

c95646e

igor-aptos force-pushed the igor/move_mem_swap branch from 23c63a3 to c95646e Compare November 12, 2024 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move][stdlib] Implement mem::swap native move call #14786

[move][stdlib] Implement mem::swap native move call #14786

igor-aptos commented Sep 27, 2024

trunk-io bot commented Sep 27, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

lightmark commented Oct 3, 2024

igor-aptos commented Oct 3, 2024

vgao1996 Oct 9, 2024

runtian-zhou Oct 9, 2024

vgao1996 Oct 10, 2024

vgao1996 left a comment

vgao1996 Oct 10, 2024

igor-aptos Oct 11, 2024

vgao1996 Oct 10, 2024

igor-aptos Oct 10, 2024

vgao1996 commented Oct 10, 2024

igor-aptos left a comment

igor-aptos commented Nov 8, 2024

georgemitenkov left a comment

georgemitenkov Oct 4, 2024

igor-aptos Nov 12, 2024

georgemitenkov Oct 24, 2024

georgemitenkov Oct 24, 2024

georgemitenkov Oct 24, 2024

georgemitenkov Oct 24, 2024

georgemitenkov Oct 24, 2024

igor-aptos Nov 12, 2024

georgemitenkov Nov 12, 2024

georgemitenkov Oct 24, 2024

igor-aptos Nov 12, 2024

georgemitenkov Nov 12, 2024

georgemitenkov Nov 12, 2024

igor-aptos left a comment

igor-aptos Nov 12, 2024

igor-aptos Nov 12, 2024

igor-aptos Nov 12, 2024

		@@ -0,0 +1,27 @@
		/// Module with methods for safe memory manipulation.
		/// I.e. swapping/replacing non-copyable/non-droppable types.

[move][stdlib] Implement mem::swap native move call #14786

Are you sure you want to change the base?

[move][stdlib] Implement mem::swap native move call #14786

Conversation

igor-aptos commented Sep 27, 2024

Description

How Has This Been Tested?

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

trunk-io bot commented Sep 27, 2024 • edited Loading

codecov bot commented Sep 27, 2024 • edited Loading

Codecov Report

lightmark commented Oct 3, 2024

igor-aptos commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgao1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgao1996 commented Oct 10, 2024

igor-aptos left a comment

Choose a reason for hiding this comment

igor-aptos commented Nov 8, 2024

georgemitenkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igor-aptos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trunk-io bot commented Sep 27, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading