[15/n] Make MoveVM stateless, remove environment, refactor sessions #16088

georgemitenkov · 2025-03-10T10:35:55Z

Description

Refactoring:

MoveVM is now fully stateless. Removed VMRuntime moving methods to (de) serialize arguments to the move_vm.rs file. Wrapper calls to execute things are removed, and sessions call execute_loaded_function directly.
Session no longer owns MoveVM (it is stateless anyway), this allows to get rid of some lifetimes throughout the code. IIRC, the VM was added there purely for module access, which is now moved out.
As a result, MoveVMExt no longer owns MoveVM: it just uses its session creation methods.

With this change the Move VM is now completely stateless, and we should be able to swap the implementation easily.

Also, some basic renaming.

How Has This Been Tested?

Existing tests.

Key Areas to Review

Type of Change

Refactoring

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 · 2025-03-10T10:35:58Z

⏱️ 38m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
check-dynamic-deps	17m	🟩 🟩 🟩 🟩 🟩 (+4 more)
rust-cargo-deny	12m	🟩 🟩 🟩 🟩 🟩 (+2 more)
semgrep/ci	4m	🟩 🟩 🟩 🟩 🟩 (+5 more)
general-lints	3m	🟩 🟩 🟩 🟩 🟩 (+2 more)
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+2 more)
permission-check	25s	🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check	22s	🟩 🟩 🟩 🟩 🟩 (+4 more)
check-branch-prefix	1s	🟩 🟩

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

georgemitenkov · 2025-03-10T10:36:13Z

[15/n] Make MoveVM stateless, remove environment, refactor sessions #16088 : 2 dependent PRs (#16090 , #16115 ) 👈 (View in Graphite)
[14/n] Remove module view and versioned modules hashmaps #16087
main

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

gelash · 2025-03-14T18:29:14Z

api/src/transactions.rs

@@ -38,7 +38,7 @@ use aptos_types::{
    vm_status::StatusCode,
    AptosCoinType, CoinType,
 };
-use aptos_vm::{AptosSimulationVM, AptosVM};
+use aptos_vm::{AptosSimulationVM, AptosVm};


I think VM makes sense, since it's not one word but abbreviation

I think main motivation is when you have AptosVmBlockExecutor this reads nicer than AptosVMBlockExecutor so hence the change, I think in one of @igor-aptos's PR we discussed that some time ago. But I do not have an opinion here, @gelash would you prefer VM over Vm?

To me it's logical because it's an abbreviation, there are cases also where it looks arguably better but yeah in VMBlock something a bit worse. Sorry, didn't remember or follow the other discussion, if we had decided that way I'm totally okay w. it. But since we are doing it now, we can also tag @zekun000 :) see what everyone thinks

There is also things like VMError etc, we should most likely do a separate PR even if we do change to Vm

Ok, rolled back VM --> Vm for now - we can decide here and rename separately, so that this PR is not blocked on naming :)

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

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

gelash · 2025-03-14T18:43:35Z

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

+            .map_err(|e| e.finish(Location::Undefined))?;
+
+        // locals should not be dropped until all return values are serialized
+        drop(dummy_locals);


any benefit to explicitly dropping here?

Also, it's a lot cleaner now - great refactor. it's moved so a bit harder to review side by side (nothing two windows can't fix). But please do point out when things aren't just copy-paste (e.g. add a comment inline) so reviewers don't miss.

I think this method was full copy paste (hence not addressing some minor nits initially, so it is easier to compare). Same for the drop - kept it like in the original code.

I think here the intention was to make sure locals are live past serialization, but here rust should drop them there anyway, so not sure

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

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

github-actions · 2025-03-17T18:36:49Z

✅ Forge suite `realistic_env_max_load` success on `5051d3d476fa47906a2968c74159b621a61defbc`

two traffics test: inner traffic : committed: 12528.01 txn/s, latency: 3175.72 ms, (p50: 2700 ms, p70: 3300, p90: 4800 ms, p99: 7200 ms), latency samples: 4763480
two traffics test : committed: 100.00 txn/s, latency: 2836.05 ms, (p50: 2600 ms, p70: 3400, p90: 4500 ms, p99: 9300 ms), latency samples: 1780
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.692, avg: 0.701", "ConsensusProposalToOrdered: max: 0.319, avg: 0.310", "ConsensusOrderedToCommit: max: 0.474, avg: 0.405", "ConsensusProposalToCommit: max: 0.782, avg: 0.714"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.79s no progress at version 45520 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.97s no progress at version 1796582 (avg 0.88s) [limit 16].
Test Ok

github-actions · 2025-03-17T18:37:18Z

✅ Forge suite `compat` success on `82240c9c7137087c575bf5d670abfa0dddc3ae9f` ==> `5051d3d476fa47906a2968c74159b621a61defbc`

Compatibility test results for 82240c9c7137087c575bf5d670abfa0dddc3ae9f ==> 5051d3d476fa47906a2968c74159b621a61defbc (PR)
1. Check liveness of validators at old version: 82240c9c7137087c575bf5d670abfa0dddc3ae9f
compatibility::simple-validator-upgrade::liveness-check : committed: 4267.33 txn/s, submitted: 4268.17 txn/s, expired: 0.84 txn/s, latency: 4048.37 ms, (p50: 3800 ms, p70: 4500, p90: 5300 ms, p99: 8300 ms), latency samples: 279185
2. Upgrading first Validator to new version: 5051d3d476fa47906a2968c74159b621a61defbc
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 2755.83 txn/s, latency: 10474.22 ms, (p50: 11300 ms, p70: 13000, p90: 13500 ms, p99: 13600 ms), latency samples: 61920
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 2792.73 txn/s, latency: 11881.10 ms, (p50: 13300 ms, p70: 14100, p90: 14200 ms, p99: 14200 ms), latency samples: 104300
3. Upgrading rest of first batch to new version: 5051d3d476fa47906a2968c74159b621a61defbc
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 2736.61 txn/s, latency: 10686.71 ms, (p50: 11400 ms, p70: 13300, p90: 13700 ms, p99: 13800 ms), latency samples: 60960
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 2734.63 txn/s, latency: 12147.08 ms, (p50: 13800 ms, p70: 14200, p90: 14500 ms, p99: 14500 ms), latency samples: 102880
4. upgrading second batch to new version: 5051d3d476fa47906a2968c74159b621a61defbc
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 5006.23 txn/s, latency: 5976.05 ms, (p50: 5400 ms, p70: 7300, p90: 8100 ms, p99: 8400 ms), latency samples: 98080
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 1851.99 txn/s, submitted: 1852.11 txn/s, expired: 0.12 txn/s, latency: 7283.34 ms, (p50: 8100 ms, p70: 8300, p90: 8600 ms, p99: 8800 ms), latency samples: 168209
5. check swarm health
Compatibility test for 82240c9c7137087c575bf5d670abfa0dddc3ae9f ==> 5051d3d476fa47906a2968c74159b621a61defbc passed
Test Ok

github-actions · 2025-03-17T18:37:54Z

✅ Forge suite `framework_upgrade` success on `82240c9c7137087c575bf5d670abfa0dddc3ae9f` ==> `5051d3d476fa47906a2968c74159b621a61defbc`

Compatibility test results for 82240c9c7137087c575bf5d670abfa0dddc3ae9f ==> 5051d3d476fa47906a2968c74159b621a61defbc (PR)
Upgrade the nodes to version: 5051d3d476fa47906a2968c74159b621a61defbc
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1749.67 txn/s, submitted: 1754.89 txn/s, failed submission: 5.01 txn/s, expired: 5.23 txn/s, latency: 1701.82 ms, (p50: 1500 ms, p70: 1800, p90: 2400 ms, p99: 3400 ms), latency samples: 153701
Upgrade the remaining nodes to version: 5051d3d476fa47906a2968c74159b621a61defbc
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1753.99 txn/s, submitted: 1761.14 txn/s, failed submission: 7.15 txn/s, expired: 7.15 txn/s, latency: 1699.34 ms, (p50: 1600 ms, p70: 1800, p90: 2400 ms, p99: 3600 ms), latency samples: 157063
5. check swarm health
Compatibility test for 82240c9c7137087c575bf5d670abfa0dddc3ae9f ==> 5051d3d476fa47906a2968c74159b621a61defbc passed
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1639.23 txn/s, submitted: 1642.33 txn/s, failed submission: 3.10 txn/s, expired: 3.10 txn/s, latency: 1772.77 ms, (p50: 1800 ms, p70: 2000, p90: 2400 ms, p99: 3400 ms), latency samples: 147901
Test Ok

georgemitenkov mentioned this pull request Mar 10, 2025

[14/n] Remove module view and versioned modules hashmaps #16087

Merged

8 tasks

georgemitenkov marked this pull request as ready for review March 10, 2025 11:27

georgemitenkov requested review from msmouse, lightmark, grao1991, gregnazario, banool, JoshLind, davidiw, movekevin, wrwg, zekun000, vgao1996 and 0xmaayan as code owners March 10, 2025 11:27

georgemitenkov requested review from runtian-zhou, gelash and igor-aptos and removed request for gregnazario, lightmark, grao1991, JoshLind, banool, 0xmaayan and movekevin March 10, 2025 11:30

georgemitenkov mentioned this pull request Mar 10, 2025

[aptos-vm] Move Aptos metadata to aptos-types (1/2) #16090

Merged

8 tasks

georgemitenkov force-pushed the george/remove-feature-flag-3.13 branch from 6c5d6aa to 8348561 Compare March 10, 2025 12:14

georgemitenkov force-pushed the george/remove-feature-flag-3.12 branch from 7b92864 to d9497d3 Compare March 12, 2025 12:05

georgemitenkov force-pushed the george/remove-feature-flag-3.13 branch from 8348561 to 8f6c66e Compare March 12, 2025 12:05

georgemitenkov force-pushed the george/remove-feature-flag-3.13 branch from 8f6c66e to 08749a2 Compare March 13, 2025 17:51

Base automatically changed from george/remove-feature-flag-3.12 to main March 13, 2025 19:27

georgemitenkov requested review from sasha8 and danielxiangzl as code owners March 13, 2025 19:27

wrwg approved these changes Mar 14, 2025

View reviewed changes

georgemitenkov force-pushed the george/remove-feature-flag-3.13 branch from 08749a2 to 482fb3a Compare March 14, 2025 10:34

georgemitenkov removed request for sasha8 and danielxiangzl March 14, 2025 10:34

gelash requested changes Mar 14, 2025

View reviewed changes

georgemitenkov force-pushed the george/remove-feature-flag-3.13 branch from 482fb3a to 151745f Compare March 15, 2025 09:39

georgemitenkov added 4 commits March 17, 2025 16:55

[15/n] Make MoveVM stateless, remove environment, refactor sessions

7332010

use non-member methods, rename VMs

7adf4e1

keep mut ref outputs, cleanup

e8d8027

address comments

25be810

georgemitenkov force-pushed the george/remove-feature-flag-3.13 branch from 151745f to 25be810 Compare March 17, 2025 17:22

georgemitenkov requested a review from gelash March 17, 2025 17:23

comments: roll back VM - Vm change

5051d3d

gelash approved these changes Mar 17, 2025

View reviewed changes

georgemitenkov enabled auto-merge (squash) March 17, 2025 17:56

This comment has been minimized.

Sign in to view

georgemitenkov merged commit ef996bf into main Mar 17, 2025
93 of 94 checks passed

georgemitenkov deleted the george/remove-feature-flag-3.13 branch March 17, 2025 18:38

This was referenced Mar 19, 2025

[WIP] Session continuation #16150

Draft

[aptos-vm] Remove simulation for multisig because enhancement has been enabled #16167

Merged

[aptos-vm] Use code storage for validation, remove AsMoveResolver where possible #16168

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[15/n] Make MoveVM stateless, remove environment, refactor sessions #16088

[15/n] Make MoveVM stateless, remove environment, refactor sessions #16088

georgemitenkov commented Mar 10, 2025 •

edited

Loading

trunk-io bot commented Mar 10, 2025 •

edited

Loading

georgemitenkov commented Mar 10, 2025 •

edited

Loading

gelash Mar 14, 2025

georgemitenkov Mar 17, 2025 •

edited

Loading

gelash Mar 17, 2025

gelash Mar 17, 2025

georgemitenkov Mar 17, 2025

gelash Mar 14, 2025

georgemitenkov Mar 17, 2025

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Mar 17, 2025

github-actions bot commented Mar 17, 2025

github-actions bot commented Mar 17, 2025

[15/n] Make MoveVM stateless, remove environment, refactor sessions #16088

[15/n] Make MoveVM stateless, remove environment, refactor sessions #16088

Conversation

georgemitenkov commented Mar 10, 2025 • edited Loading

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

trunk-io bot commented Mar 10, 2025 • edited Loading

georgemitenkov commented Mar 10, 2025 • edited Loading

gelash Mar 14, 2025

Choose a reason for hiding this comment

georgemitenkov Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

gelash Mar 17, 2025

Choose a reason for hiding this comment

gelash Mar 17, 2025

Choose a reason for hiding this comment

georgemitenkov Mar 17, 2025

Choose a reason for hiding this comment

gelash Mar 14, 2025

Choose a reason for hiding this comment

georgemitenkov Mar 17, 2025

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Mar 17, 2025

✅ Forge suite realistic_env_max_load success on 5051d3d476fa47906a2968c74159b621a61defbc

github-actions bot commented Mar 17, 2025

✅ Forge suite compat success on 82240c9c7137087c575bf5d670abfa0dddc3ae9f ==> 5051d3d476fa47906a2968c74159b621a61defbc

github-actions bot commented Mar 17, 2025

✅ Forge suite framework_upgrade success on 82240c9c7137087c575bf5d670abfa0dddc3ae9f ==> 5051d3d476fa47906a2968c74159b621a61defbc

georgemitenkov commented Mar 10, 2025 •

edited

Loading

trunk-io bot commented Mar 10, 2025 •

edited

Loading

georgemitenkov commented Mar 10, 2025 •

edited

Loading

georgemitenkov Mar 17, 2025 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `5051d3d476fa47906a2968c74159b621a61defbc`

✅ Forge suite `compat` success on `82240c9c7137087c575bf5d670abfa0dddc3ae9f` ==> `5051d3d476fa47906a2968c74159b621a61defbc`

✅ Forge suite `framework_upgrade` success on `82240c9c7137087c575bf5d670abfa0dddc3ae9f` ==> `5051d3d476fa47906a2968c74159b621a61defbc`