[aptosvm] Simplify VM flows (#11888)

* Duplicated logic for creating the gas meter for view functions has been removed.
* Duplicated logic for calculating gas used for view functions has been removed.
* There was unreachable code in failure transaction cleanup, where the discarded
status has been returned immediately, but then re-checked again. The first check
is shifted inside.
* No more default transaction metadata.
* Scripts are now validated consistently.
* Simplifies transaction execution function signature to avoid `Option<String>`.
* Removes duplicated features from `AptosVM` and keeps them in `MoveVMExt`.
* Fixes a bug when script hash was not computed for `RunOnAbort`.

Related tests are moved  to `move-e2e-tests`.

Author: georgemitenkov

aptos-move/aptos-vm/src/aptos_vm.rs

@@ -64,23 +64,12 @@ impl<'a, S: 'a + StateView + Sync> ExecutorTask for AptosExecutorTask<'a, S> {
             .vm
             .execute_single_transaction(txn, &resolver, &log_context)
         {
-            Ok((vm_status, vm_output, sender)) => {
+            Ok((vm_status, vm_output)) => {
                 if vm_output.status().is_discarded() {
-                    match sender {
-                        Some(s) => speculative_trace!(
-                            &log_context,
-                            format!(
-                                "Transaction discarded, sender: {}, error: {:?}",
-                                s, vm_status
-                            ),
-                        ),
-                        None => {
-                            speculative_trace!(
-                                &log_context,
-                                format!("Transaction malformed, error: {:?}", vm_status),
-                            )
-                        },
-                    };
+                    speculative_trace!(
+                        &log_context,
+                        format!("Transaction discarded, status: {:?}", vm_status),
+                    );
                 }
                 if vm_status.status_code() == StatusCode::SPECULATIVE_EXECUTION_ABORT_ERROR {
                     ExecutionStatus::SpeculativeExecutionAbortError(

aptos-move/aptos-vm/src/block_executor/vm_wrapper.rs

@@ -16,7 +16,7 @@ use aptos_framework::natives::{
 use aptos_table_natives::{NativeTableContext, TableChangeSet};
 use aptos_types::{
     access_path::AccessPath, block_metadata::BlockMetadata, block_metadata_ext::BlockMetadataExt,
-    contract_event::ContractEvent, on_chain_config::Features, state_store::state_key::StateKey,
+    contract_event::ContractEvent, state_store::state_key::StateKey,
     validator_txn::ValidatorTransaction,
 };
 use aptos_vm_types::{change_set::VMChangeSet, storage::change_set_configs::ChangeSetConfigs};
@@ -151,24 +151,54 @@ impl SessionId {
     pub fn as_uuid(&self) -> HashValue {
         self.hash()
     }
+
+    pub(crate) fn into_script_hash(self) -> Vec<u8> {
+        match self {
+            Self::Txn {
+                sender: _,
+                sequence_number: _,
+                script_hash,
+            }
+            | Self::Prologue {
+                sender: _,
+                sequence_number: _,
+                script_hash,
+            }
+            | Self::Epilogue {
+                sender: _,
+                sequence_number: _,
+                script_hash,
+            }
+            | Self::RunOnAbort {
+                sender: _,
+                sequence_number: _,
+                script_hash,
+            }
+            | Self::ValidatorTxn { script_hash } => script_hash,
+            Self::BlockMeta { id: _ }
+            | Self::Genesis { id: _ }
+            | Self::Void
+            | Self::BlockMetaExt { id: _ } => vec![],
+        }
+    }
 }
 
 pub struct SessionExt<'r, 'l> {
     inner: Session<'r, 'l>,
     remote: &'r dyn AptosMoveResolver,
-    features: Arc<Features>,
+    is_storage_slot_metadata_enabled: bool,
 }
 
 impl<'r, 'l> SessionExt<'r, 'l> {
     pub fn new(
         inner: Session<'r, 'l>,
         remote: &'r dyn AptosMoveResolver,
-        features: Arc<Features>,
+        is_storage_slot_metadata_enabled: bool,
     ) -> Self {
         Self {
             inner,
             remote,
-            features,
+            is_storage_slot_metadata_enabled,
         }
     }
 
@@ -219,10 +249,7 @@ impl<'r, 'l> SessionExt<'r, 'l> {
         let event_context: NativeEventContext = extensions.remove();
         let events = event_context.into_events();
 
-        let woc = WriteOpConverter::new(
-            self.remote,
-            self.features.is_storage_slot_metadata_enabled(),
-        );
+        let woc = WriteOpConverter::new(self.remote, self.is_storage_slot_metadata_enabled);
 
         let change_set = Self::convert_change_set(
             &woc,

aptos-move/aptos-vm/src/move_vm_ext/session.rs

@@ -14,10 +14,7 @@ use aptos_gas_algebra::DynamicExpression;
 use aptos_gas_schedule::{MiscGasParameters, NativeGasParameters};
 use aptos_native_interface::SafeNativeBuilder;
 use aptos_table_natives::NativeTableContext;
-use aptos_types::{
-    chain_id::ChainId,
-    on_chain_config::{FeatureFlag, Features, TimedFeatureFlag, TimedFeatures},
-};
+use aptos_types::on_chain_config::{FeatureFlag, Features, TimedFeatureFlag, TimedFeatures};
 use move_binary_format::{
     deserializer::DeserializerConfig,
     errors::VMResult,
@@ -28,12 +25,12 @@ use move_bytecode_verifier::VerifierConfig;
 use move_vm_runtime::{
     config::VMConfig, move_vm::MoveVM, native_extensions::NativeContextExtensions,
 };
-use std::{ops::Deref, sync::Arc};
+use std::ops::Deref;
 
 pub struct MoveVmExt {
     inner: MoveVM,
     chain_id: u8,
-    features: Arc<Features>,
+    features: Features,
 }
 
 pub fn get_max_binary_format_version(
@@ -134,7 +131,7 @@ impl MoveVmExt {
                 resolver,
             )?,
             chain_id,
-            features: Arc::new(features),
+            features,
         })
     }
 
@@ -204,30 +201,9 @@ impl MoveVmExt {
         extensions.add(NativeRistrettoPointContext::new());
         extensions.add(AlgebraContext::new());
         extensions.add(NativeAggregatorContext::new(txn_hash, resolver, resolver));
-
-        let script_hash = match session_id {
-            SessionId::Txn {
-                sender: _,
-                sequence_number: _,
-                script_hash,
-            }
-            | SessionId::Prologue {
-                sender: _,
-                sequence_number: _,
-                script_hash,
-            }
-            | SessionId::Epilogue {
-                sender: _,
-                sequence_number: _,
-                script_hash,
-            } => script_hash,
-            SessionId::ValidatorTxn { script_hash } => script_hash,
-            _ => vec![],
-        };
-
         extensions.add(NativeTransactionContext::new(
             txn_hash.to_vec(),
-            script_hash,
+            session_id.into_script_hash(),
             self.chain_id,
         ));
         extensions.add(NativeCodeContext::default());
@@ -241,12 +217,12 @@ impl MoveVmExt {
         SessionExt::new(
             self.inner.new_session_with_extensions(resolver, extensions),
             resolver,
-            self.features.clone(),
+            self.features.is_storage_slot_metadata_enabled(),
         )
     }
 
-    pub fn get_chain_id(&self) -> ChainId {
-        ChainId::new(self.chain_id)
+    pub(crate) fn features(&self) -> &Features {
+        &self.features
     }
 }
 

aptos-move/aptos-vm/src/move_vm_ext/vm.rs

@@ -1,7 +1,15 @@
 // Copyright © Aptos Foundation
 // SPDX-License-Identifier: Apache-2.0
 
+use crate::{
+    aptos_vm::get_or_vm_startup_failure, data_cache::AsMoveResolver,
+    transaction_metadata::TransactionMetadata, AptosVM,
+};
+use aptos_types::{state_store::StateView, transaction::SignedTransaction};
+use aptos_vm_logging::log_schema::AdapterLogSchema;
+use aptos_vm_types::output::VMOutput;
 use move_binary_format::errors::VMResult;
+use move_core_types::vm_status::VMStatus;
 
 #[derive(Debug, Eq, Hash, PartialEq)]
 pub enum InjectedError {
@@ -47,3 +55,34 @@ pub mod testing_only {
         })
     }
 }
+
+impl AptosVM {
+    #[cfg(any(test, feature = "testing"))]
+    pub fn test_failed_transaction_cleanup(
+        &self,
+        error_vm_status: VMStatus,
+        txn: &SignedTransaction,
+        state_view: &impl StateView,
+        gas_meter_balance: u64,
+    ) -> (VMStatus, VMOutput) {
+        let txn_data = TransactionMetadata::new(txn);
+        let log_context = AdapterLogSchema::new(state_view.id(), 0);
+
+        let mut gas_meter = self
+            .make_standard_gas_meter(gas_meter_balance.into(), &log_context)
+            .expect("Should be able to create a gas meter for tests");
+        let change_set_configs = &get_or_vm_startup_failure(&self.storage_gas_params, &log_context)
+            .expect("Storage gas parameters should exist for tests")
+            .change_set_configs;
+
+        let resolver = state_view.as_move_resolver();
+        self.failed_transaction_cleanup(
+            error_vm_status,
+            &mut gas_meter,
+            &txn_data,
+            &resolver,
+            &log_context,
+            change_set_configs,
+        )
+    }
+}

aptos-move/aptos-vm/src/testing.rs

@@ -2,14 +2,13 @@
 // Parts of the project are originally copyright © Meta Platforms, Inc.
 // SPDX-License-Identifier: Apache-2.0
 
-use aptos_crypto::{ed25519::Ed25519PrivateKey, HashValue, PrivateKey};
+use aptos_crypto::HashValue;
 use aptos_gas_algebra::{FeePerGasUnit, Gas, NumBytes};
 use aptos_types::{
     account_address::AccountAddress,
     chain_id::ChainId,
-    transaction::{authenticator::AuthenticationKey, SignedTransaction, TransactionPayload},
+    transaction::{SignedTransaction, TransactionPayload},
 };
-use std::convert::TryFrom;
 
 pub struct TransactionMetadata {
     pub sender: AccountAddress,
@@ -118,30 +117,6 @@ impl TransactionMetadata {
     }
 
     pub fn is_multi_agent(&self) -> bool {
-        !(self.secondary_signers.is_empty() && self.fee_payer.is_none())
-    }
-}
-
-impl Default for TransactionMetadata {
-    fn default() -> Self {
-        let mut buf = [0u8; Ed25519PrivateKey::LENGTH];
-        buf[Ed25519PrivateKey::LENGTH - 1] = 1;
-        let public_key = Ed25519PrivateKey::try_from(&buf[..]).unwrap().public_key();
-        TransactionMetadata {
-            sender: AccountAddress::ZERO,
-            authentication_key: AuthenticationKey::ed25519(&public_key).to_vec(),
-            secondary_signers: vec![],
-            secondary_authentication_keys: vec![],
-            sequence_number: 0,
-            fee_payer: None,
-            fee_payer_authentication_key: None,
-            max_gas_amount: 100_000_000.into(),
-            gas_unit_price: 0.into(),
-            transaction_size: 0.into(),
-            expiration_timestamp_secs: 0,
-            chain_id: ChainId::test(),
-            script_hash: vec![],
-            script_size: NumBytes::zero(),
-        }
+        !self.secondary_signers.is_empty() || self.fee_payer.is_some()
     }
 }

aptos-move/aptos-vm/src/transaction_metadata.rs

@@ -7,7 +7,7 @@ use aptos_crypto::ed25519::Ed25519PublicKey;
 use aptos_types::{
     invalid_signature,
     jwks::{jwk::JWK, PatchedJWKs},
-    on_chain_config::{CurrentTimeMicroseconds, OnChainConfig},
+    on_chain_config::{CurrentTimeMicroseconds, Features, OnChainConfig},
     transaction::authenticator::EphemeralPublicKey,
     vm_status::{StatusCode, VMStatus},
     zkid::{
@@ -100,16 +100,28 @@ fn get_jwk_for_zkid_authenticator(
     Ok(jwk)
 }
 
-pub fn validate_zkid_authenticators(
+pub(crate) fn validate_zkid_authenticators(
     authenticators: &Vec<(ZkIdPublicKey, ZkIdSignature)>,
+    features: &Features,
     resolver: &impl AptosMoveResolver,
 ) -> Result<(), VMStatus> {
+    // zkID feature gating.
+    for (_, sig) in authenticators {
+        if !features.is_zkid_enabled() && matches!(sig.sig, ZkpOrOpenIdSig::Groth16Zkp { .. }) {
+            return Err(VMStatus::error(StatusCode::FEATURE_UNDER_GATING, None));
+        }
+        if (!features.is_zkid_enabled() || !features.is_zkid_zkless_enabled())
+            && matches!(sig.sig, ZkpOrOpenIdSig::OpenIdSig { .. })
+        {
+            return Err(VMStatus::error(StatusCode::FEATURE_UNDER_GATING, None));
+        }
+    }
+
     if authenticators.is_empty() {
         return Ok(());
     }
 
     let config = &get_configs_onchain(resolver)?;
-
     if authenticators.len() > config.max_zkid_signatures_per_txn as usize {
         return Err(invalid_signature!("Too many zkID authenticators"));
     }

aptos-move/aptos-vm/src/zkid_validation.rs

@@ -0,0 +1,25 @@
+// Copyright © Aptos Foundation
+// SPDX-License-Identifier: Apache-2.0
+
+use crate::MoveHarness;
+use aptos_cached_packages::aptos_stdlib::aptos_account_transfer;
+use aptos_language_e2e_tests::account::Account;
+use claims::assert_err_eq;
+use move_core_types::vm_status::StatusCode;
+
+#[test]
+fn non_existent_sender() {
+    let mut h = MoveHarness::new();
+
+    let sender = Account::new();
+    let receiver = h.new_account_with_balance_and_sequence_number(100_000, 0);
+
+    let txn = sender
+        .transaction()
+        .payload(aptos_account_transfer(*receiver.address(), 10))
+        .sequence_number(0)
+        .sign();
+
+    let status = h.run(txn);
+    assert_err_eq!(status.status(), StatusCode::SENDING_ACCOUNT_DOES_NOT_EXIST);
+}

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

@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 mod access_path_test;
+mod account;
 mod aggregator;
 mod aggregator_v2;
 mod aggregator_v2_events;
@@ -46,4 +47,5 @@ mod token_objects;
 mod transaction_fee;
 mod type_too_large;
 mod vector_numeric_address;
+mod vm;
 mod vote;