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.

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

[loader] Enable V2 implementation as default, V1 uses an env var #14473

Merged
merged 14 commits into from
Sep 6, 2024

Conversation

georgemitenkov
Copy link
Contributor

@georgemitenkov georgemitenkov commented Aug 30, 2024

Description

  1. Enables V2 loader as default implementation in features and VM configs.
  2. AptosVM module publish validation is applied to both V1 and V2 now. While V2 implementation allows for hooks so that validate_publish_request runs after bytecode verifier, we keep this as is for now to run tests and minimize the divergence in behaviour.
  3. Fixed genesis module publishing for V2 flow. V1 was actually broken but worked because of loader cache :/
  4. Fixed init module where the user session changes were not found in init_module temporary session.
  5. Fixed runtime environment sharing to allow for multiple versions (like with warm VM cache).
  6. Fixed a few tests failures due to error messages/error codes.

Note: proptests for Block-STM for module publishing are still broken.

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Other (specify)

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

Copy link

trunk-io bot commented Aug 30, 2024

⏱️ 14h 9m total CI duration on this PR
Job Cumulative Duration Recent Runs
execution-performance / single-node-performance 3h 45m 🟥🟥🟥🟥🟥 (+2 more)
forge-compat-test / forge 1h 5m 🟩🟩🟩🟥
forge-e2e-test / forge 1h 4m 🟥🟥🟥🟥
check-dynamic-deps 40m 🟩🟩🟩🟩 (+16 more)
general-lints 34m 🟩🟩🟩🟩🟩 (+14 more)
rust-cargo-deny 33m 🟩🟩🟩🟩🟩 (+14 more)
execution-performance / test-target-determinator 32m 🟩🟩🟩🟩🟩 (+2 more)
test-target-determinator 25m 🟩🟩🟩🟩🟩
check 21m 🟩🟩🟩🟩 (+1 more)
rust-move-unit-coverage 16m 🟩
rust-move-unit-coverage 13m 🟩
rust-move-unit-coverage 13m 🟩
rust-move-unit-coverage 13m 🟩
rust-move-unit-coverage 12m 🟩
rust-move-tests 10m 🟥
rust-move-tests 10m 🟩
rust-move-unit-coverage 9m 🟩
rust-move-tests 9m 🟩
rust-move-unit-coverage 9m 🟩
rust-move-tests 9m 🟥
rust-move-tests 9m 🟩
rust-move-tests 9m 🟥
rust-move-tests 9m 🟥
rust-move-tests 9m 🟩
rust-move-tests 8m 🟥
semgrep/ci 8m 🟩🟩🟩🟩🟩 (+16 more)
rust-move-unit-coverage 8m 🟩
rust-move-unit-coverage 8m 🟩
rust-move-tests 8m 🟥
rust-move-tests 7m
rust-move-unit-coverage 7m 🟩
rust-move-unit-coverage 7m 🟩
rust-smoke-tests 7m
indexer-grpc-e2e-tests / test-indexer-grpc-docker-compose 7m 🟩🟩🟩🟩
rust-doc-tests 5m 🟩
rust-doc-tests 5m 🟩
rust-doc-tests 5m 🟩
rust-doc-tests 5m 🟩
rust-move-unit-coverage 4m 🟥
file_change_determinator 4m 🟩🟩🟩🟩🟩 (+14 more)
file_change_determinator 4m 🟩🟩🟩🟩🟩 (+15 more)
rust-move-unit-coverage 4m 🟥
rust-move-unit-coverage 4m
rust-move-unit-coverage 3m 🟥
rust-move-unit-coverage 3m 🟥
rust-move-tests 3m 🟥
rust-move-unit-coverage 3m 🟥
rust-move-unit-coverage 3m
rust-move-tests 3m 🟥
rust-move-tests 3m
rust-move-tests 3m 🟥
rust-move-tests 3m 🟥
rust-move-tests 3m 🟥
rust-move-tests 2m 🟥
rust-move-tests 2m
permission-check 1m 🟩🟩🟩🟩🟩 (+15 more)
permission-check 1m 🟩🟩🟩🟩🟩 (+16 more)
permission-check 1m 🟩🟩🟩🟩🟩 (+16 more)
permission-check 1m 🟩🟩🟩🟩🟩 (+15 more)
file_change_determinator 56s 🟩🟩🟩🟩🟩
permission-check 20s 🟩🟩🟩
Backport PR 18s 🟥🟥🟥
permission-check 14s 🟩🟩🟩🟩🟩
determine-docker-build-metadata 12s 🟩🟩🟩🟩🟩

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

Job Duration vs 7d avg Delta
check-dynamic-deps 18m 1m +1512%
check 6m 4m +60%
test-target-determinator 7m 5m +39%
execution-performance / test-target-determinator 7m 5m +38%

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link

codecov bot commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 35.54633% with 466 lines in your changes missing coverage. Please review.

Please upload report for BASE (george/main@e9f53e6). Learn more about missing BASE report.

Files with missing lines Patch % Lines
aptos-move/aptos-vm/src/aptos_vm.rs 0.0% 158 Missing ⚠️
...e_vm_ext/session/user_transaction_sessions/user.rs 0.0% 96 Missing ⚠️
...tos-move/aptos-vm/src/move_vm_ext/warm_vm_cache.rs 0.0% 47 Missing ⚠️
...ptos-move/aptos-vm/src/verifier/resource_groups.rs 0.0% 43 Missing ⚠️
...tos-move/aptos-vm/src/verifier/event_validation.rs 0.0% 29 Missing ⚠️
aptos-move/aptos-vm/src/move_vm_ext/vm.rs 0.0% 20 Missing ⚠️
third_party/move/move-vm/runtime/src/loader/mod.rs 42.4% 19 Missing ⚠️
...ty/move/move-vm/runtime/src/storage/environment.rs 68.8% 19 Missing ⚠️
...tos-move/block-executor/src/modules_and_scripts.rs 0.0% 11 Missing ⚠️
types/src/vm/modules.rs 0.0% 7 Missing ⚠️
... and 9 more
Additional details and impacted files
@@              Coverage Diff               @@
##             george/main   #14473   +/-   ##
==============================================
  Coverage               ?    58.9%           
==============================================
  Files                  ?      858           
  Lines                  ?   209460           
  Branches               ?        0           
==============================================
  Hits                   ?   123570           
  Misses                 ?    85890           
  Partials               ?        0           

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

@georgemitenkov georgemitenkov marked this pull request as ready for review August 30, 2024 09:54
@georgemitenkov georgemitenkov requested review from runtian-zhou and ziaptos and removed request for davidiw and wrwg August 30, 2024 09:55
@georgemitenkov 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 Aug 30, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@georgemitenkov georgemitenkov force-pushed the george/enable-loader-v2-by-default branch from 6fa985e to d336f0f Compare August 30, 2024 10:35

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@georgemitenkov georgemitenkov force-pushed the george/enable-loader-v2-by-default branch from 0322c35 to c164354 Compare September 4, 2024 18:37
@georgemitenkov georgemitenkov force-pushed the george/enable-loader-v2-by-default branch from c164354 to 63b019e Compare September 5, 2024 21:11
@georgemitenkov georgemitenkov force-pushed the george/enable-loader-v2-by-default branch from 63b019e to 6284a5a Compare September 5, 2024 21:46
@georgemitenkov georgemitenkov force-pushed the george/enable-loader-v2-by-default branch from 6284a5a to d49c535 Compare September 5, 2024 21:48
@georgemitenkov georgemitenkov force-pushed the george/enable-loader-v2-by-default branch from d49c535 to e33a2a0 Compare September 5, 2024 21:59
@georgemitenkov georgemitenkov force-pushed the george/enable-loader-v2-by-default branch from e33a2a0 to 591110c Compare September 5, 2024 22:05
Base automatically changed from george/script-mv-cache to george/main September 6, 2024 09:54
 - Fix runtime environement to be shared / updated properly
 - Fix Block-STM code publish with delayed fields
- Updated linker error message to be like in V1
- Added checks for friends existing when publishing
- Removed bytecode verifier tests which check friendship cycles
- Commented out async vm tests
  - Added natives check in V2 implementation. This is needed to
    keep 1:1 mapping with V1 implementation, but whatever it
    checks seems useless legacy native struct.
  - Added a paranoid check that address/name of the module correspond
    to its storage location.
  1. Loader V2 also uses verification cache like loader V1. This
     should help compare both.
  2. Factored out module deserialization so that code is not duplicated.
  3. Fixed a few TODOs, and made sure MoveVM tests pass again.
  - Move verified cache into its own file, add nits
  - Move env Loader V1 variable to lib.rs
  - hash() returns a reference
  - deleted commented out cargo dependency in async testsuite
@georgemitenkov georgemitenkov force-pushed the george/enable-loader-v2-by-default branch from 5d59a9f to 4cef9e3 Compare September 6, 2024 10:05
@georgemitenkov georgemitenkov merged commit cd736a0 into george/main Sep 6, 2024
32 of 35 checks passed
@georgemitenkov georgemitenkov deleted the george/enable-loader-v2-by-default branch September 6, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant