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

[move] Consolidate VM configs and allow VM startup failure #13276

Closed
wants to merge 24 commits into from

Conversation

georgemitenkov
Copy link
Contributor

@georgemitenkov georgemitenkov commented May 14, 2024

Description

Needs #13233 to land first.

The goal of this PR is 1) to ensure all configs are defined in one place, used correctly, 2) ensure if configs do not exist, VM is not even created.

Summary:

  1. Verification config changed from production to default, same for VM config. The reason is that both of these configs are not production, and it is important that APIs are called correctly.

  2. Verification APIs must take config. This allows the user, e.g. Aptos fuzzer to pass the config, and reduces the chances of errors. E.g., right now fuzzer is not initialised correctly. Future PRs will further refactor this logic.

  3. VM test harness uses paranoid mode by default. There is no reason why it should not, these are tests after all.

  4. All Aptos production configs are created in a single file, environement.rs. The goal of this file is to be a single place where all Aptos configs are defined, so that they can be later cached per block/epoch, and so that all initialization is the same! E.g., right now we create configs at multiple places, which is very bad.

  5. AptosVM no longer has results of gas parameters, and fails on initialisation. As a result, APIs have been changed to take this into account. In particular, reviewers should look closely in block executor how the error is propagated, because we panic if there is no sequential fallback (cc @gelash @igor-aptos @ziaptos)

  6. Other APIs have been changed to reflect that, i.e. simulation, validation, tests.

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 May 14, 2024

⏱️ 7h 10m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-targeted-unit-tests 3h 20m 🟥🟥🟥🟥
rust-move-unit-coverage 1h 41m 🟩🟩🟩🟩🟩
rust-move-tests 57m 🟥🟥🟥🟥🟥
rust-lints 32m 🟩🟥🟩🟩🟩
run-tests-main-branch 21m 🟩🟩🟩🟩🟩
general-lints 9m 🟩🟩🟩🟩🟩
check-dynamic-deps 6m 🟩🟩🟩🟩🟩
semgrep/ci 2m 🟩🟩🟩🟩🟩
file_change_determinator 52s 🟩🟩🟩🟩🟩
file_change_determinator 49s 🟩🟩🟩🟩🟩
permission-check 12s 🟩🟩🟩🟩
permission-check 12s 🟩🟩🟩🟩
permission-check 12s 🟩🟩🟩🟩
permission-check 8s 🟩🟩🟩🟩

🚨 1 job on the last run was significantly faster/slower than expected

Job Duration vs 7d avg Delta
rust-targeted-unit-tests 55m 19m +195%

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link

codecov bot commented May 14, 2024

Codecov Report

Attention: Patch coverage is 8.39161% with 262 lines in your changes are missing coverage. Please review.

Project coverage is 57.5%. Comparing base (2fd763e) to head (ec30a54).

Files Patch % Lines
aptos-move/aptos-vm/src/environment.rs 0.0% 111 Missing ⚠️
aptos-move/aptos-vm/src/aptos_vm.rs 0.0% 56 Missing ⚠️
aptos-move/aptos-vm/src/gas.rs 0.0% 35 Missing ⚠️
types/src/on_chain_config/aptos_features.rs 0.0% 17 Missing ⚠️
aptos-move/aptos-vm/src/move_vm_ext/vm.rs 0.0% 10 Missing ⚠️
aptos-move/block-executor/src/executor.rs 0.0% 8 Missing ⚠️
third_party/move/move-ir-compiler/src/main.rs 0.0% 6 Missing ⚠️
aptos-move/aptos-vm/src/data_cache.rs 0.0% 5 Missing ⚠️
...tos-move/aptos-vm/src/block_executor/vm_wrapper.rs 0.0% 4 Missing ⚠️
...os-move/aptos-gas-schedule/src/gas_schedule/mod.rs 0.0% 3 Missing ⚠️
... and 5 more
Additional details and impacted files
@@                       Coverage Diff                       @@
##           george/aptos-resource-viewer   #13276     +/-   ##
===============================================================
- Coverage                          57.6%    57.5%   -0.1%     
===============================================================
  Files                               832      833      +1     
  Lines                            198377   198327     -50     
===============================================================
- Hits                             114274   114178     -96     
- Misses                            84103    84149     +46     

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

@georgemitenkov georgemitenkov changed the base branch from main to george/aptos-resource-viewer May 15, 2024 09:45
@georgemitenkov georgemitenkov marked this pull request as ready for review May 15, 2024 16:36
@georgemitenkov georgemitenkov changed the title [move] Cache VM configs per block [move] Consolidate VM configs and allow VM startup failure May 15, 2024
@@ -313,17 +313,16 @@ pub trait AsMoveResolver<S> {

impl<S: StateView> AsMoveResolver<S> for S {
fn as_move_resolver(&self) -> StorageAdapter<S> {
let (_, gas_feature_version) = get_gas_config_from_storage(self);
let (_, gas_feature_version) = get_gas_config_from_storage(self).expect("TODO");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

Base automatically changed from george/aptos-resource-viewer to main May 16, 2024 10:33
@georgemitenkov georgemitenkov deleted the george/cleanup branch May 23, 2024 15:43
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.

None yet

1 participant