[aptos-vm] Split module write set from change sets #13809

georgemitenkov · 2024-06-25T12:06:19Z

Description

This PR is the first step to remove module writes from MoveVM session. Ideally, we want to

On module publish, run MoveVM passes to verify new packages.
Somehow stash these new modules so that we can run module initialization.
Pass down all the way to transaction output.
Module publishing should only be processed by the adapter (though genesis/write set transaction is a special case).

Here, specifically, we only consider relevant AptosVM changes:

Change set does not have modules anymore, instead they are kept on the side. This allows to ensure that prologue, epilogue, abort hook sessions do not see any new module updates, and that they are only passed down to the output.
User change set can have modules, and explicitly stores published code, if available. The code is still stored in MoveVM session, but is no longer part of the view with stashed change set changes. No more squashing of modules as well.
Change set configs have been adapted to work with both change set and user change sets.

Notes:

Some code has not ever been tested, e.g. change set size checks. Async with reviews, a TODO: ensure testing coverage with this changes is high.
As said above, MoveVM still caches published modules. For simplicity, this is left out for the next PR because it requires to change APIs to pass ModuleStorage to session, rather than store it inside as a part of MoveResolver.
Genesis is a bit ugly, but it is expected - it really a special case and does not belong to block execution/adapter. A more rigorous cleanup is needed there.

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

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-06-25T12:06:23Z

⏱️ 50h 35m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
test-fuzzers	4h 59m	🟩 🟥 🟩 🟩 🟩 (+4 more)
execution-performance / single-node-performance	4h 41m	🟥 🟩 🟩 🟩 🟥 (+6 more)
replay-mainnet / replay-verify (16)	3h 7m	⬜
replay-testnet / replay-verify (2)	3h 7m	⬜
replay-testnet / replay-verify (10)	3h 6m	⬜
replay-testnet / replay-verify (12)	3h 6m	⬜
replay-testnet / replay-verify (4)	3h 6m	⬜
replay-testnet / replay-verify (13)	3h 5m	⬜
replay-testnet / replay-verify (16)	3h 4m	⬜
replay-testnet / replay-verify (17)	3h 4m	⬜
replay-testnet / replay-verify (18)	3h 4m	⬜
forge-e2e-test / forge	1h 34m	⬜ 🟩 🟩 🟩 🟩 (+2 more)
forge-compat-test / forge	1h 25m	🟩 ⬜ 🟩 🟩 🟩 (+1 more)
rust-images / rust-all	1h 18m	🟩 ⬜ 🟩 ⬜ 🟩 (+5 more)
rust-smoke-tests	1h 7m	🟩 ⬜ 🟩
execution-performance / test-target-determinator	58m	🟩 🟩 🟩 🟩 🟩 (+4 more)
test-target-determinator	55m	🟩 ⬜ 🟩 🟩 🟩 (+5 more)
rust-targeted-unit-tests	51m	🟩 ⬜ 🟩
rust-move-tests	33m	🟩 ⬜ 🟩
check	30m	🟩 🟩 🟩 🟩 🟩 (+3 more)
rust-move-unit-coverage	29m	🟩 ⬜ 🟩
rust-lints	18m	🟩 ⬜ 🟩
cli-e2e-tests / run-cli-tests	16m	🟩 🟩
run-tests-main-branch	16m	🟩 🟩 🟥
general-lints	15m	🟩 🟩 🟩 🟩 🟩 (+3 more)
rust-build-cached-packages	13m	🟩 ⬜ 🟩
rust-move-tests	12m	🟩
rust-move-unit-coverage	11m	🟩
rust-move-unit-coverage	11m	🟩
check-dynamic-deps	10m	🟩 🟩 🟩 🟩 🟩 (+4 more)
indexer-grpc-e2e-tests / test-indexer-grpc-docker-compose	10m	🟥 🟩 🟩 🟩 🟩 (+1 more)
rust-move-tests	9m	🟩
rust-move-tests	9m	🟩
rust-move-unit-coverage	9m	🟩
rust-move-tests	9m	🟩
rust-move-unit-coverage	9m	🟩
rust-move-tests	7m	🟥
rust-move-tests	4m	⬜
rust-move-unit-coverage	4m	⬜
semgrep/ci	4m	🟩 🟩 🟩 🟩 🟩 (+4 more)
node-api-compatibility-tests / node-api-compatibility-tests	2m	🟩 🟩
file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+5 more)
file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+5 more)
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+3 more)
permission-check	38s	🟩 🟩 🟩 🟩 🟩 (+5 more)
permission-check	36s	🟩 🟩 🟩 🟩 🟩 (+6 more)
permission-check	34s	🟩 🟩 🟩 🟩 🟩 (+6 more)
permission-check	31s	🟩 🟩 🟩 🟩 🟩 (+5 more)
determine-docker-build-metadata	31s	🟩 🟩 🟩 🟩 🟩 (+5 more)
permission-check	29s	🟩 🟩 🟩 🟩 🟩 (+6 more)

🚨 6 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg
execution-performance / single-node-performance	1h	20m
rust-move-tests	16m	9m
rust-targeted-unit-tests	24m	15m
run-tests-main-branch	7m	5m
test-target-determinator	6m	5m
rust-move-unit-coverage	14m	12m

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

codecov · 2024-06-25T12:18:21Z

Codecov Report

Attention: Patch coverage is 0% with 803 lines in your changes missing coverage. Please review.

Project coverage is 58.9%. Comparing base (533f3c3) to head (2590f5d).
Report is 1 commits behind head on main.

Files	Patch %	Lines
aptos-move/aptos-vm/src/aptos_vm.rs	0.0%	364 Missing ⚠️
aptos-move/aptos-vm-types/src/change_set.rs	0.0%	86 Missing ⚠️
...n/user_transaction_sessions/session_change_sets.rs	0.0%	68 Missing ⚠️
..._ext/session/user_transaction_sessions/epilogue.rs	0.0%	66 Missing ⚠️
aptos-move/aptos-vm-types/src/output.rs	0.0%	60 Missing ⚠️
aptos-move/aptos-vm-types/src/module_write_set.rs	0.0%	50 Missing ⚠️
aptos-move/aptos-vm/src/move_vm_ext/session/mod.rs	0.0%	22 Missing ⚠️
...xt/session/user_transaction_sessions/abort_hook.rs	0.0%	21 Missing ⚠️
..._ext/session/user_transaction_sessions/prologue.rs	0.0%	19 Missing ⚠️
...e/aptos-vm-types/src/storage/change_set_configs.rs	0.0%	14 Missing ⚠️
... and 9 more

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #13809     +/-   ##
=========================================
- Coverage    59.0%    58.9%   -0.1%     
=========================================
  Files         822      824      +2     
  Lines      198161   198320    +159     
=========================================
- Hits       117026   117008     -18     
- Misses      81135    81312    +177

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

aptos-move/aptos-vm-types/src/change_set.rs

aptos-move/aptos-gas-meter/src/traits.rs

aptos-move/aptos-vm-types/src/change_set.rs

aptos-move/e2e-tests/src/executor.rs

aptos-move/aptos-vm-types/src/change_set.rs

github-actions · 2024-07-11T09:33:38Z

✅ Forge suite `compat` success on `1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5` ==> `2590f5df354257bab66f5ed11c0648079f92617b`

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 2590f5df354257bab66f5ed11c0648079f92617b (PR)
1. Check liveness of validators at old version: 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5
compatibility::simple-validator-upgrade::liveness-check : committed: 6214.1169182447875 txn/s, latency: 4743.562600806452 ms, (p50: 3900 ms, p90: 8900 ms, p99: 14500 ms), latency samples: 248000
2. Upgrading first Validator to new version: 2590f5df354257bab66f5ed11c0648079f92617b
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 3167.6357509300274 txn/s, latency: 8245.548976497346 ms, (p50: 9000 ms, p90: 11200 ms, p99: 11700 ms), latency samples: 79140
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3218.3875781890993 txn/s, latency: 9663.415134431916 ms, (p50: 9700 ms, p90: 14800 ms, p99: 15100 ms), latency samples: 138360
3. Upgrading rest of first batch to new version: 2590f5df354257bab66f5ed11c0648079f92617b
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 3588.294035493357 txn/s, latency: 7262.732650520597 ms, (p50: 8700 ms, p90: 9700 ms, p99: 10000 ms), latency samples: 88360
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3252.43112292261 txn/s, latency: 9570.273980412221 ms, (p50: 9400 ms, p90: 14800 ms, p99: 15100 ms), latency samples: 136820
4. upgrading second batch to new version: 2590f5df354257bab66f5ed11c0648079f92617b
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 6935.398283387525 txn/s, latency: 4020.939172156975 ms, (p50: 4600 ms, p90: 5100 ms, p99: 5200 ms), latency samples: 139640
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 5848.60366132839 txn/s, latency: 5562.564072367298 ms, (p50: 5100 ms, p90: 9900 ms, p99: 11100 ms), latency samples: 234360
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 2590f5df354257bab66f5ed11c0648079f92617b passed
Test Ok

github-actions · 2024-07-11T09:36:22Z

✅ Forge suite `framework_upgrade` success on `1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5` ==> `2590f5df354257bab66f5ed11c0648079f92617b`

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 2590f5df354257bab66f5ed11c0648079f92617b (PR)
Upgrade the nodes to version: 2590f5df354257bab66f5ed11c0648079f92617b
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1111.0050924595316 txn/s, submitted: 1112.1473966878468 txn/s, failed submission: 1.1423042283153728 txn/s, expired: 1.1423042283153728 txn/s, latency: 2752.2320378367262 ms, (p50: 2100 ms, p90: 4800 ms, p99: 9300 ms), latency samples: 97260
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1111.8368100128023 txn/s, submitted: 1114.7674423181888 txn/s, failed submission: 2.9306323053865433 txn/s, expired: 2.9306323053865433 txn/s, latency: 2785.94451540957 ms, (p50: 2100 ms, p90: 5100 ms, p99: 8400 ms), latency samples: 98640
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 2590f5df354257bab66f5ed11c0648079f92617b passed
Upgrade the remaining nodes to version: 2590f5df354257bab66f5ed11c0648079f92617b
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1115.1297941634675 txn/s, submitted: 1117.5894748804913 txn/s, failed submission: 2.4596807170238906 txn/s, expired: 2.4596807170238906 txn/s, latency: 2900.694936835773 ms, (p50: 2100 ms, p90: 5200 ms, p99: 10200 ms), latency samples: 99740
Test Ok

github-actions · 2024-07-11T10:24:12Z

✅ Forge suite `realistic_env_max_load` success on `2590f5df354257bab66f5ed11c0648079f92617b`

two traffics test: inner traffic : committed: 8513.611829704776 txn/s, latency: 4606.858044519429 ms, (p50: 4500 ms, p90: 5700 ms, p99: 10500 ms), latency samples: 3674800
two traffics test : committed: 100.08240816072451 txn/s, latency: 2077.7764367816094 ms, (p50: 2000 ms, p90: 2300 ms, p99: 3000 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.220, avg: 0.213", "QsPosToProposal: max: 0.251, avg: 0.239", "ConsensusProposalToOrdered: max: 0.316, avg: 0.291", "ConsensusOrderedToCommit: max: 0.384, avg: 0.372", "ConsensusProposalToCommit: max: 0.674, avg: 0.663"]
Max round gap was 1 [limit 4] at version 1663637. Max no progress secs was 5.310035 [limit 15] at version 1663637.
Test Ok

- Change set does not have modules anymore, instead they are kept on the side. This allows to ensure that prologue, epilogue, abort hook sessions do not see any new module updates, and that they are only passed down to the output. - User change set can have modules, and explicitly stores published code, if available. The code is still stored in MoveVM session, but is no longer part of the view with stashed change set changes. No more squashing of modules as well. - Change set configs have been adapted to work with both change set and user change sets.

georgemitenkov added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Jun 25, 2024

georgemitenkov requested review from davidiw, movekevin, gelash, vgao1996, wrwg and zekun000 as code owners June 25, 2024 12:06

georgemitenkov requested review from msmouse, runtian-zhou and ziaptos June 25, 2024 12:06