-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
⏱️ 7h 10m total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
Codecov ReportAttention: Patch coverage is
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. |
9d504c6
to
67e3709
Compare
@@ -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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
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:
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.
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.
VM test harness uses paranoid mode by default. There is no reason why it should not, these are tests after all.
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.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)
Other APIs have been changed to reflect that, i.e. simulation, validation, tests.
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Key Areas to Review
Checklist