From c8a9d0efc08f981de1458e80c777cf1bd863e896 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Thu, 1 Feb 2024 03:04:33 +0000 Subject: [PATCH 01/15] get rid of useless optional sender string --- aptos-move/aptos-vm/src/aptos_vm.rs | 21 +++++++++---------- .../aptos-vm/src/block_executor/vm_wrapper.rs | 21 +++++-------------- .../execution/ptx-executor/src/runner.rs | 2 +- 3 files changed, 16 insertions(+), 28 deletions(-) diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index 1ca09f430e2ef..9b15f391aab0a 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -1767,7 +1767,7 @@ impl AptosVM { Ok((VMStatus::Executed, output)) } - pub(crate) fn process_block_prologue( + fn process_block_prologue( &self, resolver: &impl AptosMoveResolver, block_metadata: BlockMetadata, @@ -1809,7 +1809,7 @@ impl AptosVM { Ok((VMStatus::Executed, output)) } - pub(crate) fn process_block_prologue_ext( + fn process_block_prologue_ext( &self, resolver: &impl AptosMoveResolver, block_metadata_ext: BlockMetadataExt, @@ -2023,13 +2023,13 @@ impl AptosVM { txn: &SignatureVerifiedTransaction, resolver: &impl AptosMoveResolver, log_context: &AdapterLogSchema, - ) -> Result<(VMStatus, VMOutput, Option), VMStatus> { + ) -> Result<(VMStatus, VMOutput), VMStatus> { assert!(!self.is_simulation, "VM has to be created for execution"); if let SignatureVerifiedTransaction::Invalid(_) = txn { let vm_status = VMStatus::error(StatusCode::INVALID_SIGNATURE, None); let discarded_output = discarded_output(vm_status.status_code()); - return Ok((vm_status, discarded_output, None)); + return Ok((vm_status, discarded_output)); } Ok(match txn.expect_valid() { @@ -2037,7 +2037,7 @@ impl AptosVM { fail_point!("aptos_vm::execution::block_metadata"); let (vm_status, output) = self.process_block_prologue(resolver, block_metadata.clone(), log_context)?; - (vm_status, output, Some("block_prologue".to_string())) + (vm_status, output) }, Transaction::BlockMetadataExt(block_metadata_ext) => { fail_point!("aptos_vm::execution::block_metadata_ext"); @@ -2046,7 +2046,7 @@ impl AptosVM { block_metadata_ext.clone(), log_context, )?; - (vm_status, output, Some("block_prologue_ext".to_string())) + (vm_status, output) }, GenesisTransaction(write_set_payload) => { let (vm_status, output) = self.process_waypoint_change_set( @@ -2054,11 +2054,10 @@ impl AptosVM { write_set_payload.clone(), log_context, )?; - (vm_status, output, Some("waypoint_write_set".to_string())) + (vm_status, output) }, UserTransaction(txn) => { fail_point!("aptos_vm::execution::user_transaction"); - let sender = txn.sender().to_hex(); let _timer = TXN_TOTAL_SECONDS.start_timer(); let (vm_status, output) = self.execute_user_transaction(resolver, txn, log_context); @@ -2129,17 +2128,17 @@ impl AptosVM { if let Some(label) = counter_label { USER_TRANSACTIONS_EXECUTED.with_label_values(&[label]).inc(); } - (vm_status, output, Some(sender)) + (vm_status, output) }, StateCheckpoint(_) => { let status = TransactionStatus::Keep(ExecutionStatus::Success); let output = VMOutput::empty_with_status(status); - (VMStatus::Executed, output, Some("state_checkpoint".into())) + (VMStatus::Executed, output) }, Transaction::ValidatorTransaction(txn) => { let (vm_status, output) = self.process_validator_transaction(resolver, txn.clone(), log_context)?; - (vm_status, output, Some("validator_transaction".to_string())) + (vm_status, output) }, }) } diff --git a/aptos-move/aptos-vm/src/block_executor/vm_wrapper.rs b/aptos-move/aptos-vm/src/block_executor/vm_wrapper.rs index 9c729e4d4a37b..34eed5e41eb49 100644 --- a/aptos-move/aptos-vm/src/block_executor/vm_wrapper.rs +++ b/aptos-move/aptos-vm/src/block_executor/vm_wrapper.rs @@ -63,23 +63,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( diff --git a/experimental/execution/ptx-executor/src/runner.rs b/experimental/execution/ptx-executor/src/runner.rs index e16417dff4c51..63192b931f1ea 100644 --- a/experimental/execution/ptx-executor/src/runner.rs +++ b/experimental/execution/ptx-executor/src/runner.rs @@ -270,7 +270,7 @@ impl<'scope, 'view: 'scope, BaseView: StateView + Sync> Worker<'view, BaseView> }; let _post = PER_WORKER_TIMER.timer_with(&[&idx, "run_txn_post_vm"]); // TODO(ptx): error handling - let (_vm_status, vm_output, _msg) = vm_output.expect("VM execution failed."); + let (_vm_status, vm_output) = vm_output.expect("VM execution failed."); // inform output state values to the manager // TODO use try_into_storage_change_set() instead, and ChangeSet it returns, instead of VMOutput. From d69214365dcda999a82902229fde95fbef1a00c2 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Thu, 1 Feb 2024 09:45:44 +0000 Subject: [PATCH 02/15] remove duplicated features, fix types --- aptos-move/aptos-vm/src/aptos_vm.rs | 83 +++++++++---------- .../aptos-vm/src/move_vm_ext/session.rs | 13 ++- aptos-move/aptos-vm/src/move_vm_ext/vm.rs | 14 ++-- 3 files changed, 54 insertions(+), 56 deletions(-) diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index 9b15f391aab0a..5a4106228c8a2 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -9,15 +9,15 @@ use crate::{ errors::{discarded_output, expect_only_successful_execution}, gas::{check_gas, get_gas_parameters}, move_vm_ext::{ - get_max_binary_format_version, AptosMoveResolver, MoveVmExt, RespawnedSession, SessionExt, - SessionId, + get_max_binary_format_version, get_max_identifier_size, AptosMoveResolver, MoveVmExt, + RespawnedSession, SessionExt, SessionId, }, sharded_block_executor::{executor_client::ExecutorClient, ShardedBlockExecutor}, system_module_names::*, transaction_metadata::TransactionMetadata, transaction_validation, verifier, zkid_validation, VMExecutor, VMValidator, }; -use anyhow::{anyhow, Result}; +use anyhow::anyhow; use aptos_block_executor::txn_commit_hook::NoOpTransactionCommitHook; use aptos_crypto::HashValue; use aptos_framework::{natives::code::PublishRequest, RuntimeModuleMetadataV1}; @@ -76,7 +76,6 @@ use move_binary_format::{ compatibility::Compatibility, deserializer::DeserializerConfig, errors::{verification_error, Location, PartialVMError, PartialVMResult, VMError, VMResult}, - file_format_common::{IDENTIFIER_SIZE_MAX, LEGACY_IDENTIFIER_SIZE_MAX}, CompiledModule, IndexKind, }; use move_core_types::{ @@ -170,7 +169,6 @@ pub struct AptosVM { gas_feature_version: u64, gas_params: Result, pub(crate) storage_gas_params: Result, - features: Features, timed_features: TimedFeatures, } @@ -205,7 +203,7 @@ impl AptosVM { misc_gas_params, gas_feature_version, chain_id.id(), - features.clone(), + features, timed_features.clone(), resolver, ) @@ -217,7 +215,6 @@ impl AptosVM { gas_feature_version, gas_params, storage_gas_params, - features, timed_features, } } @@ -230,6 +227,11 @@ impl AptosVM { self.move_vm.new_session(resolver, session_id) } + #[inline(always)] + fn features(&self) -> &Features { + self.move_vm.features() + } + /// Sets execution concurrency level when invoked the first time. pub fn set_concurrency_level_once(mut concurrency_level: usize) { concurrency_level = min(concurrency_level, num_cpus::get()); @@ -351,7 +353,7 @@ impl AptosVM { StorageAdapter::new_with_config( executor_view, self.gas_feature_version, - &self.features, + self.features(), None, ) } @@ -363,7 +365,7 @@ impl AptosVM { StorageAdapter::new_with_config( executor_view, self.gas_feature_version, - &self.features, + self.features(), Some(executor_view), ) } @@ -419,7 +421,7 @@ impl AptosVM { match TransactionStatus::from_vm_status( error_code.clone(), - self.features + self.features() .is_enabled(FeatureFlag::CHARGE_INVARIANT_VIOLATION), ) { TransactionStatus::Keep(status) => { @@ -485,7 +487,7 @@ impl AptosVM { const ZERO_STORAGE_REFUND: u64 = 0; let is_account_init_for_sponsored_transaction = - is_account_init_for_sponsored_transaction(txn_data, &self.features, resolver)?; + is_account_init_for_sponsored_transaction(txn_data, self.features(), resolver)?; if is_account_init_for_sponsored_transaction { let mut session = self.new_session(resolver, SessionId::run_on_abort(txn_data)); @@ -558,7 +560,7 @@ impl AptosVM { session, gas_meter.balance(), fee_statement, - &self.features, + self.features(), txn_data, log_context, ) @@ -576,7 +578,7 @@ impl AptosVM { &mut session, gas_meter.balance(), fee_statement, - &self.features, + self.features(), txn_data, log_context, )?; @@ -620,7 +622,7 @@ impl AptosVM { session, gas_meter.balance(), fee_statement, - &self.features, + self.features(), txn_data, log_context, ) @@ -647,7 +649,7 @@ impl AptosVM { script_fn.function(), script_fn.ty_args(), )?; - let struct_constructors = self.features.is_enabled(FeatureFlag::STRUCT_CONSTRUCTORS); + let struct_constructors = self.features().is_enabled(FeatureFlag::STRUCT_CONSTRUCTORS); let args = verifier::transaction_arg_validation::validate_combine_signer_and_txn_args( session, senders, @@ -703,7 +705,7 @@ impl AptosVM { txn_data.senders(), convert_txn_args(script.args()), &loaded_func, - self.features.is_enabled(FeatureFlag::STRUCT_CONSTRUCTORS), + self.features().is_enabled(FeatureFlag::STRUCT_CONSTRUCTORS), )?; session.execute_script( script.code(), @@ -767,7 +769,7 @@ impl AptosVM { txn_data.transaction_size, txn_data.gas_unit_price, )?; - if !self.features.is_storage_deletion_refund_enabled() { + if !self.features().is_storage_deletion_refund_enabled() { storage_refund = 0.into(); } @@ -1144,15 +1146,8 @@ impl AptosVM { /// Deserialize a module bundle. fn deserialize_module_bundle(&self, modules: &ModuleBundle) -> VMResult> { - let max_version = get_max_binary_format_version(&self.features, None); - let max_identifier_size = if self - .features - .is_enabled(FeatureFlag::LIMIT_MAX_IDENTIFIER_LENGTH) - { - IDENTIFIER_SIZE_MAX - } else { - LEGACY_IDENTIFIER_SIZE_MAX - }; + let max_version = get_max_binary_format_version(self.features(), None); + let max_identifier_size = get_max_identifier_size(self.features()); let config = DeserializerConfig::new(max_version, max_identifier_size); let mut result = vec![]; for module_blob in modules.iter() { @@ -1204,7 +1199,7 @@ impl AptosVM { true, true, !self - .features + .features() .is_enabled(FeatureFlag::TREAT_FRIEND_AS_PRIVATE), ), )?; @@ -1280,7 +1275,7 @@ impl AptosVM { true, true, !self - .features + .features() .is_enabled(FeatureFlag::TREAT_FRIEND_AS_PRIVATE), ), )); @@ -1329,13 +1324,14 @@ impl AptosVM { } } } - aptos_framework::verify_module_metadata(m, &self.features, &self.timed_features) + aptos_framework::verify_module_metadata(m, self.features(), &self.timed_features) .map_err(|err| Self::metadata_validation_error(&err.to_string()))?; } verifier::resource_groups::validate_resource_groups( session, modules, - self.features.is_enabled(FeatureFlag::SAFER_RESOURCE_GROUPS), + self.features() + .is_enabled(FeatureFlag::SAFER_RESOURCE_GROUPS), )?; verifier::event_validation::validate_module_events(session, modules)?; @@ -1391,12 +1387,13 @@ impl AptosVM { match &authenticators { Ok(authenticators) => { for (_, sig) in authenticators { - if !self.features.is_zkid_enabled() + if !self.features().is_zkid_enabled() && matches!(sig.sig, ZkpOrOpenIdSig::Groth16Zkp { .. }) { return Err(VMStatus::error(StatusCode::FEATURE_UNDER_GATING, None)); } - if (!self.features.is_zkid_enabled() || !self.features.is_zkid_zkless_enabled()) + if (!self.features().is_zkid_enabled() + || !self.features().is_zkid_zkless_enabled()) && matches!(sig.sig, ZkpOrOpenIdSig::OpenIdSig { .. }) { return Err(VMStatus::error(StatusCode::FEATURE_UNDER_GATING, None)); @@ -1411,7 +1408,7 @@ impl AptosVM { zkid_validation::validate_zkid_authenticators( &authenticators.unwrap(), resolver, - self.move_vm.get_chain_id(), + self.move_vm.chain_id(), )?; // The prologue MUST be run AFTER any validation. Otherwise you may run prologue and hit @@ -1449,7 +1446,7 @@ impl AptosVM { let txn_status = TransactionStatus::from_vm_status( err.clone(), - self.features + self.features() .is_enabled(FeatureFlag::CHARGE_INVARIANT_VIOLATION), ); if txn_status.is_discarded() { @@ -1497,7 +1494,7 @@ impl AptosVM { } let is_account_init_for_sponsored_transaction = - match is_account_init_for_sponsored_transaction(&txn_data, &self.features, resolver) { + match is_account_init_for_sponsored_transaction(&txn_data, self.features(), resolver) { Ok(result) => result, Err(err) => { let vm_status = err.into_vm_status(); @@ -1669,7 +1666,7 @@ impl AptosVM { senders, convert_txn_args(script.args()), &loaded_func, - self.features.is_enabled(FeatureFlag::STRUCT_CONSTRUCTORS), + self.features().is_enabled(FeatureFlag::STRUCT_CONSTRUCTORS), )?; return_on_failure!(tmp_session.execute_script( @@ -1851,7 +1848,7 @@ impl AptosVM { } fn extract_module_metadata(&self, module: &ModuleId) -> Option> { - if self.features.is_enabled(FeatureFlag::VM_BINARY_FORMAT_V6) { + if self.features().is_enabled(FeatureFlag::VM_BINARY_FORMAT_V6) { aptos_framework::get_vm_metadata(&self.move_vm, module) } else { aptos_framework::get_vm_metadata_v0(&self.move_vm, module) @@ -1895,7 +1892,7 @@ impl AptosVM { vm: &AptosVM, log_context: &AdapterLogSchema, gas_budget: u64, - ) -> Result>> { + ) -> anyhow::Result>> { let gas_meter = MemoryTrackedGasMeter::new(StandardGasMeter::new(StandardGasAlgebra::new( vm.gas_feature_version, get_or_vm_startup_failure(&vm.gas_params, log_context)? @@ -1925,7 +1922,7 @@ impl AptosVM { type_args: Vec, arguments: Vec>, gas_meter: &mut MemoryTrackedGasMeter>, - ) -> Result>> { + ) -> anyhow::Result>> { let func_inst = session.load_function(&module_id, &func_name, &type_args)?; let metadata = vm.extract_module_metadata(&module_id); let arguments = verifier::view_function::validate_view_function( @@ -1934,7 +1931,7 @@ impl AptosVM { func_name.as_ident_str(), &func_inst, metadata.as_ref().map(Arc::as_ref), - vm.features.is_enabled(FeatureFlag::STRUCT_CONSTRUCTORS), + vm.features().is_enabled(FeatureFlag::STRUCT_CONSTRUCTORS), )?; Ok(session @@ -1972,7 +1969,7 @@ impl AptosVM { self.gas_feature_version, resolver, txn_data, - &self.features, + self.features(), log_context, )?; @@ -2242,7 +2239,7 @@ impl VMValidator for AptosVM { let log_context = AdapterLogSchema::new(state_view.id(), 0); if !self - .features + .features() .is_enabled(FeatureFlag::SINGLE_SENDER_AUTHENTICATOR) { if let aptos_types::transaction::authenticator::TransactionAuthenticator::SingleSender{ .. } = transaction.authenticator_ref() { @@ -2250,7 +2247,7 @@ impl VMValidator for AptosVM { } } - if !self.features.is_enabled(FeatureFlag::WEBAUTHN_SIGNATURE) { + if !self.features().is_enabled(FeatureFlag::WEBAUTHN_SIGNATURE) { if let Ok(sk_authenticators) = transaction .authenticator_ref() .to_single_key_authenticators() diff --git a/aptos-move/aptos-vm/src/move_vm_ext/session.rs b/aptos-move/aptos-vm/src/move_vm_ext/session.rs index ea67d8db4eea0..0cdecda98e30f 100644 --- a/aptos-move/aptos-vm/src/move_vm_ext/session.rs +++ b/aptos-move/aptos-vm/src/move_vm_ext/session.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}; @@ -156,19 +156,19 @@ impl SessionId { pub struct SessionExt<'r, 'l> { inner: Session<'r, 'l>, remote: &'r dyn AptosMoveResolver, - features: Arc, + 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, + is_storage_slot_metadata_enabled: bool, ) -> Self { Self { inner, remote, - features, + is_storage_slot_metadata_enabled, } } @@ -209,10 +209,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, diff --git a/aptos-move/aptos-vm/src/move_vm_ext/vm.rs b/aptos-move/aptos-vm/src/move_vm_ext/vm.rs index de2742e714c50..86a54c7432f66 100644 --- a/aptos-move/aptos-vm/src/move_vm_ext/vm.rs +++ b/aptos-move/aptos-vm/src/move_vm_ext/vm.rs @@ -28,12 +28,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, } pub fn get_max_binary_format_version( @@ -137,7 +137,7 @@ impl MoveVmExt { resolver, )?, chain_id, - features: Arc::new(features), + features, }) } @@ -240,13 +240,17 @@ 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 { + pub(crate) fn chain_id(&self) -> ChainId { ChainId::new(self.chain_id) } + + pub(crate) fn features(&self) -> &Features { + &self.features + } } impl Deref for MoveVmExt { From 1bfffd6626339c64435a6d9340c2e0bcf5e63dd2 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Sat, 3 Feb 2024 08:31:47 +0000 Subject: [PATCH 03/15] cleanup unreachable discards --- aptos-move/aptos-vm/src/aptos_vm.rs | 73 ++++------- .../e2e-move-tests/src/tests/account.rs | 24 ++++ aptos-move/e2e-move-tests/src/tests/mod.rs | 1 + .../src/tests/failed_transaction_tests.rs | 118 ------------------ aptos-move/e2e-testsuite/src/tests/mod.rs | 1 - 5 files changed, 47 insertions(+), 170 deletions(-) create mode 100644 aptos-move/e2e-move-tests/src/tests/account.rs delete mode 100644 aptos-move/e2e-testsuite/src/tests/failed_transaction_tests.rs diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index 5a4106228c8a2..483c05f446833 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -324,28 +324,6 @@ impl AptosVM { get_or_vm_startup_failure(&self.gas_params, &log_context) } - /// Generates a transaction output for a transaction that encountered errors during the - /// execution process. This is public for now only for tests. - pub fn failed_transaction_cleanup( - &self, - error_code: VMStatus, - gas_meter: &mut impl AptosGasMeter, - txn_data: &TransactionMetadata, - resolver: &impl AptosMoveResolver, - log_context: &AdapterLogSchema, - change_set_configs: &ChangeSetConfigs, - ) -> VMOutput { - self.failed_transaction_cleanup_and_keep_vm_status( - error_code, - gas_meter, - txn_data, - resolver, - log_context, - change_set_configs, - ) - .1 - } - pub fn as_move_resolver<'r, R: ExecutorView>( &self, executor_view: &'r R, @@ -387,10 +365,11 @@ impl AptosVM { storage_fee_refund, ) } + - fn failed_transaction_cleanup_and_keep_vm_status( + fn failed_transaction_cleanup( &self, - error_code: VMStatus, + error_vm_status: VMStatus, gas_meter: &mut impl AptosGasMeter, txn_data: &TransactionMetadata, resolver: &impl AptosMoveResolver, @@ -419,16 +398,18 @@ impl AptosVM { } } - match TransactionStatus::from_vm_status( - error_code.clone(), + let txn_status = TransactionStatus::from_vm_status( + error_vm_status.clone(), self.features() .is_enabled(FeatureFlag::CHARGE_INVARIANT_VIOLATION), - ) { + ); + + match txn_status { TransactionStatus::Keep(status) => { // The transaction should be kept. Run the appropriate post transaction workflows // including epilogue. This runs a new session that ignores any side effects that // might abort the execution (e.g., spending additional funds needed to pay for - // gas). Eeven if the previous failure occurred while running the epilogue, it + // gas). Even if the previous failure occurred while running the epilogue, it // should not fail now. If it somehow fails here, there is no choice but to // discard the transaction. let txn_output = match self.finish_aborted_transaction( @@ -444,12 +425,12 @@ impl AptosVM { }, Err(err) => discarded_output(err.status_code()), }; - (error_code, txn_output) + (error_vm_status, txn_output) + }, + TransactionStatus::Discard(status_code) => { + let discarded_output = discarded_output(status_code); + (error_vm_status, discarded_output) }, - TransactionStatus::Discard(status_code) => ( - VMStatus::error(status_code, None), - discarded_output(status_code), - ), TransactionStatus::Retry => unreachable!(), } } @@ -1444,24 +1425,14 @@ impl AptosVM { self.move_vm.mark_loader_cache_as_invalid(); }; - let txn_status = TransactionStatus::from_vm_status( - err.clone(), - self.features() - .is_enabled(FeatureFlag::CHARGE_INVARIANT_VIOLATION), - ); - if txn_status.is_discarded() { - let discarded_output = discarded_output(err.status_code()); - (err, discarded_output) - } else { - self.failed_transaction_cleanup_and_keep_vm_status( - err, - gas_meter, - txn_data, - resolver, - log_context, - &storage_gas_params.change_set_configs, - ) - } + self.failed_transaction_cleanup( + err, + gas_meter, + txn_data, + resolver, + log_context, + &storage_gas_params.change_set_configs, + ) } fn execute_user_transaction_impl( diff --git a/aptos-move/e2e-move-tests/src/tests/account.rs b/aptos-move/e2e-move-tests/src/tests/account.rs new file mode 100644 index 0000000000000..e447213d445b6 --- /dev/null +++ b/aptos-move/e2e-move-tests/src/tests/account.rs @@ -0,0 +1,24 @@ +// 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)) + .sign(); + + let status = h.run(txn); + assert_err_eq!(status.status(), StatusCode::SENDING_ACCOUNT_DOES_NOT_EXIST,); +} diff --git a/aptos-move/e2e-move-tests/src/tests/mod.rs b/aptos-move/e2e-move-tests/src/tests/mod.rs index eda2c8aac84aa..05ae829511149 100644 --- a/aptos-move/e2e-move-tests/src/tests/mod.rs +++ b/aptos-move/e2e-move-tests/src/tests/mod.rs @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 mod access_path_test; +mod account; mod aggregator; mod aggregator_v2; mod attributes; diff --git a/aptos-move/e2e-testsuite/src/tests/failed_transaction_tests.rs b/aptos-move/e2e-testsuite/src/tests/failed_transaction_tests.rs deleted file mode 100644 index 8edcbf60a8a53..0000000000000 --- a/aptos-move/e2e-testsuite/src/tests/failed_transaction_tests.rs +++ /dev/null @@ -1,118 +0,0 @@ -// Copyright © Aptos Foundation -// Parts of the project are originally copyright © Meta Platforms, Inc. -// SPDX-License-Identifier: Apache-2.0 - -use aptos_gas_meter::{StandardGasAlgebra, StandardGasMeter}; -use aptos_gas_schedule::{AptosGasParameters, LATEST_GAS_FEATURE_VERSION}; -use aptos_language_e2e_tests::{common_transactions::peer_to_peer_txn, executor::FakeExecutor}; -use aptos_memory_usage_tracker::MemoryTrackedGasMeter; -use aptos_types::{ - state_store::{state_key::StateKey, TStateView}, - transaction::ExecutionStatus, - vm_status::{StatusCode, VMStatus}, - write_set::WriteOp, -}; -use aptos_vm::{data_cache::AsMoveResolver, transaction_metadata::TransactionMetadata, AptosVM}; -use aptos_vm_logging::log_schema::AdapterLogSchema; -use aptos_vm_types::storage::StorageGasParameters; -use claims::assert_some; -use move_core_types::vm_status::StatusCode::TYPE_MISMATCH; - -#[test] -fn failed_transaction_cleanup_test() { - let mut executor = FakeExecutor::from_head_genesis(); - // TODO(Gas): double check this - let sender = executor.create_raw_account_data(1_000_000, 10); - executor.add_account_data(&sender); - - let log_context = AdapterLogSchema::new(executor.get_state_view().id(), 0); - let aptos_vm = AptosVM::new(&executor.get_state_view().as_move_resolver()); - let data_cache = executor.get_state_view().as_move_resolver(); - - let txn_data = TransactionMetadata { - sender: *sender.address(), - max_gas_amount: 100_000.into(), - gas_unit_price: 0.into(), - sequence_number: 10, - ..Default::default() - }; - - let gas_params = AptosGasParameters::zeros(); - let storage_gas_params = - StorageGasParameters::unlimited(gas_params.vm.txn.legacy_free_write_bytes_quota); - - let change_set_configs = storage_gas_params.change_set_configs.clone(); - - let mut gas_meter = MemoryTrackedGasMeter::new(StandardGasMeter::new(StandardGasAlgebra::new( - LATEST_GAS_FEATURE_VERSION, - gas_params.vm, - storage_gas_params, - 10_000, - ))); - - // TYPE_MISMATCH should be kept and charged. - let out1 = aptos_vm.failed_transaction_cleanup( - VMStatus::error(StatusCode::TYPE_MISMATCH, None), - &mut gas_meter, - &txn_data, - &data_cache, - &log_context, - &change_set_configs, - ); - - let write_set: Vec<(&StateKey, &WriteOp)> = out1 - .change_set() - .concrete_write_set_iter() - .map(|(k, v)| (k, assert_some!(v))) - .collect(); - assert!(!write_set.is_empty()); - assert_eq!(out1.gas_used(), 90_000); - assert!(!out1.status().is_discarded()); - assert_eq!( - out1.status().status(), - // StatusCode::TYPE_MISMATCH - Ok(ExecutionStatus::MiscellaneousError(Some(TYPE_MISMATCH))) - ); - - // Invariant violations should be charged. - let out2 = aptos_vm.failed_transaction_cleanup( - VMStatus::error(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR, None), - &mut gas_meter, - &txn_data, - &data_cache, - &log_context, - &change_set_configs, - ); - assert!(out2.gas_used() != 0); - assert!(!out2.status().is_discarded()); - assert_eq!( - out2.status().status(), - Ok(ExecutionStatus::MiscellaneousError(Some( - StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR - ))) - ); -} - -#[test] -fn non_existent_sender() { - let mut executor = FakeExecutor::from_head_genesis(); - let sequence_number = 0; - let sender = executor.create_raw_account(); - let receiver = executor.create_raw_account_data(100_000, sequence_number); - executor.add_account_data(&receiver); - - let transfer_amount = 10; - let txn = peer_to_peer_txn( - &sender, - receiver.account(), - sequence_number, - transfer_amount, - 0, - ); - - let output = &executor.execute_transaction(txn); - assert_eq!( - output.status().status(), - Err(StatusCode::SENDING_ACCOUNT_DOES_NOT_EXIST), - ); -} diff --git a/aptos-move/e2e-testsuite/src/tests/mod.rs b/aptos-move/e2e-testsuite/src/tests/mod.rs index 7a1f974008f52..98ecf9deda90b 100644 --- a/aptos-move/e2e-testsuite/src/tests/mod.rs +++ b/aptos-move/e2e-testsuite/src/tests/mod.rs @@ -16,7 +16,6 @@ mod account_universe; mod create_account; mod data_store; mod execution_strategies; -mod failed_transaction_tests; mod genesis; mod genesis_initializations; mod invariant_violation; From 1345a3c32268398d598a12972560603f87f5f39c Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Sat, 3 Feb 2024 08:46:10 +0000 Subject: [PATCH 04/15] cleanup view function gas metering --- aptos-move/aptos-vm/src/aptos_vm.rs | 43 +++++++++-------------------- 1 file changed, 13 insertions(+), 30 deletions(-) diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index 483c05f446833..7ffe72fc2ef53 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -365,7 +365,6 @@ impl AptosVM { storage_fee_refund, ) } - fn failed_transaction_cleanup( &self, @@ -497,7 +496,7 @@ impl AptosVM { if let Err(err) = self.charge_change_set(&mut change_set, gas_meter, txn_data) { info!( *log_context, - "Failed during charge_change_set: {:?}. Most likely exceded gas limited.", err, + "Failed during charge_change_set: {:?}. Most likely exceeded gas limited.", err, ); }; @@ -1832,14 +1831,14 @@ impl AptosVM { func_name: Identifier, type_args: Vec, arguments: Vec>, - gas_budget: u64, + max_gas_amount: u64, ) -> ViewFunctionOutput { let resolver = state_view.as_move_resolver(); let vm = AptosVM::new(&resolver); let log_context = AdapterLogSchema::new(state_view.id(), 0); - let mut gas_meter = match Self::memory_tracked_gas_meter(&vm, &log_context, gas_budget) { + let mut gas_meter = match vm.make_standard_gas_meter(max_gas_amount.into(), &log_context) { Ok(gas_meter) => gas_meter, - Err(e) => return ViewFunctionOutput::new(Err(e), 0), + Err(e) => return ViewFunctionOutput::new(Err(anyhow::Error::msg(format!("{}", e))), 0), }; let mut session = vm.new_session(&resolver, SessionId::Void); @@ -1852,34 +1851,18 @@ impl AptosVM { arguments, &mut gas_meter, ) { - Ok(result) => { - ViewFunctionOutput::new(Ok(result), Self::gas_used(gas_budget, &gas_meter)) + Ok(result) => ViewFunctionOutput::new( + Ok(result), + Self::gas_used(max_gas_amount.into(), &gas_meter), + ), + Err(e) => { + ViewFunctionOutput::new(Err(e), Self::gas_used(max_gas_amount.into(), &gas_meter)) }, - Err(e) => ViewFunctionOutput::new(Err(e), Self::gas_used(gas_budget, &gas_meter)), } } - fn memory_tracked_gas_meter( - vm: &AptosVM, - log_context: &AdapterLogSchema, - gas_budget: u64, - ) -> anyhow::Result>> { - let gas_meter = MemoryTrackedGasMeter::new(StandardGasMeter::new(StandardGasAlgebra::new( - vm.gas_feature_version, - get_or_vm_startup_failure(&vm.gas_params, log_context)? - .vm - .clone(), - get_or_vm_startup_failure(&vm.storage_gas_params, log_context)?.clone(), - gas_budget, - ))); - Ok(gas_meter) - } - - fn gas_used( - gas_budget: u64, - gas_meter: &MemoryTrackedGasMeter>, - ) -> u64 { - GasQuantity::new(gas_budget) + fn gas_used(max_gas_amount: Gas, gas_meter: &impl AptosGasMeter) -> u64 { + max_gas_amount .checked_sub(gas_meter.balance()) .expect("Balance should always be less than or equal to max gas amount") .into() @@ -1892,7 +1875,7 @@ impl AptosVM { func_name: Identifier, type_args: Vec, arguments: Vec>, - gas_meter: &mut MemoryTrackedGasMeter>, + gas_meter: &mut impl AptosGasMeter, ) -> anyhow::Result>> { let func_inst = session.load_function(&module_id, &func_name, &type_args)?; let metadata = vm.extract_module_metadata(&module_id); From 5a6682c64611dbff87bc5a4a6045e3d996ab4ac0 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Sat, 3 Feb 2024 09:32:55 +0000 Subject: [PATCH 05/15] unify gas used calculation & remove TxnMeta::default() --- aptos-move/aptos-vm/src/aptos_vm.rs | 7 ++--- .../aptos-vm/src/transaction_metadata.rs | 31 ++----------------- 2 files changed, 5 insertions(+), 33 deletions(-) diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index 7ffe72fc2ef53..a79555a2b18b1 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -353,12 +353,9 @@ impl AptosVM { gas_meter: &impl AptosGasMeter, storage_fee_refund: u64, ) -> FeeStatement { - let gas_used = txn_data - .max_gas_amount() - .checked_sub(gas_meter.balance()) - .expect("Balance should always be less than or equal to max gas amount"); + let gas_used = Self::gas_used(txn_data.max_gas_amount(), gas_meter); FeeStatement::new( - gas_used.into(), + gas_used, u64::from(gas_meter.execution_gas_used()), u64::from(gas_meter.io_gas_used()), u64::from(gas_meter.storage_fee_used()), diff --git a/aptos-move/aptos-vm/src/transaction_metadata.rs b/aptos-move/aptos-vm/src/transaction_metadata.rs index 909859ed95d6d..52a0ed42e593c 100644 --- a/aptos-move/aptos-vm/src/transaction_metadata.rs +++ b/aptos-move/aptos-vm/src/transaction_metadata.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, @@ -117,30 +116,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() } } From b55bd1beab66f7f7867dc5cca4226877fa90b62b Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Sat, 3 Feb 2024 10:08:15 +0000 Subject: [PATCH 06/15] consolidate script validation --- aptos-move/aptos-vm/src/aptos_vm.rs | 106 +++++++++++++--------------- 1 file changed, 50 insertions(+), 56 deletions(-) diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index a79555a2b18b1..acaeb1260f49e 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -49,7 +49,8 @@ use aptos_types::{ authenticator::AnySignature, signature_verified_transaction::SignatureVerifiedTransaction, BlockOutput, EntryFunction, ExecutionError, ExecutionStatus, ModuleBundle, Multisig, - MultisigTransactionPayload, SignatureCheckedTransaction, SignedTransaction, Transaction, + MultisigTransactionPayload, Script, SignatureCheckedTransaction, SignedTransaction, + Transaction, Transaction::{ BlockMetadata as BlockMetadataTransaction, GenesisTransaction, StateCheckpoint, UserTransaction, @@ -614,30 +615,53 @@ impl AptosVM { Ok((VMStatus::Executed, output)) } + fn validate_and_execute_script( + &self, + session: &mut SessionExt, + // Note: cannot use AptosGasMeter because it is not implemented for + // UnmeteredGasMeter. + gas_meter: &mut impl GasMeter, + senders: Vec, + script: &Script, + ) -> Result { + let loaded_func = session.load_script(script.code(), script.ty_args().to_vec())?; + // TODO(Gerardo): consolidate the extended validation to verifier. + verifier::event_validation::verify_no_event_emission_in_script( + script.code(), + &session.get_vm_config().deserializer_config, + )?; + + let args = verifier::transaction_arg_validation::validate_combine_signer_and_txn_args( + session, + senders, + convert_txn_args(script.args()), + &loaded_func, + self.features().is_enabled(FeatureFlag::STRUCT_CONSTRUCTORS), + )?; + + Ok(session.execute_script(script.code(), script.ty_args().to_vec(), args, gas_meter)?) + } + fn validate_and_execute_entry_function( &self, session: &mut SessionExt, gas_meter: &mut impl AptosGasMeter, senders: Vec, - script_fn: &EntryFunction, + entry_fn: &EntryFunction, ) -> Result { - let function = session.load_function( - script_fn.module(), - script_fn.function(), - script_fn.ty_args(), - )?; - let struct_constructors = self.features().is_enabled(FeatureFlag::STRUCT_CONSTRUCTORS); + let function = + session.load_function(entry_fn.module(), entry_fn.function(), entry_fn.ty_args())?; let args = verifier::transaction_arg_validation::validate_combine_signer_and_txn_args( session, senders, - script_fn.args().to_vec(), + entry_fn.args().to_vec(), &function, - struct_constructors, + self.features().is_enabled(FeatureFlag::STRUCT_CONSTRUCTORS), )?; Ok(session.execute_entry_function( - script_fn.module(), - script_fn.function(), - script_fn.ty_args().to_vec(), + entry_fn.module(), + entry_fn.function(), + entry_fn.ty_args().to_vec(), args, gas_meter, )?) @@ -668,43 +692,25 @@ impl AptosVM { match payload { TransactionPayload::Script(script) => { - let loaded_func = - session.load_script(script.code(), script.ty_args().to_vec())?; - // Gerardo: consolidate the extended validation to verifier. - verifier::event_validation::verify_no_event_emission_in_script( - script.code(), - &session.get_vm_config().deserializer_config, - )?; - - let args = - verifier::transaction_arg_validation::validate_combine_signer_and_txn_args( - &mut session, - txn_data.senders(), - convert_txn_args(script.args()), - &loaded_func, - self.features().is_enabled(FeatureFlag::STRUCT_CONSTRUCTORS), - )?; - session.execute_script( - script.code(), - script.ty_args().to_vec(), - args, + self.validate_and_execute_script( + &mut session, gas_meter, + txn_data.senders(), + script, )?; }, - TransactionPayload::EntryFunction(script_fn) => { + TransactionPayload::EntryFunction(entry_fn) => { self.validate_and_execute_entry_function( &mut session, gas_meter, txn_data.senders(), - script_fn, + entry_fn, )?; }, // Not reachable as this function should only be invoked for entry or script // transaction payload. - _ => { - return Err(VMStatus::error(StatusCode::UNREACHABLE, None)); - }, + _ => unreachable!("Only scripts or entry functions are executed"), }; self.resolve_pending_code_publish( @@ -1607,7 +1613,6 @@ impl AptosVM { txn_sender: Option, session_id: SessionId, ) -> Result { - let mut gas_meter = UnmeteredGasMeter; let change_set_configs = ChangeSetConfigs::unlimited_at_gas_feature_version(self.gas_feature_version); @@ -1625,23 +1630,12 @@ impl AptosVM { Some(sender) => vec![sender, *execute_as], }; - let loaded_func = - tmp_session.load_script(script.code(), script.ty_args().to_vec())?; - let args = - verifier::transaction_arg_validation::validate_combine_signer_and_txn_args( - &mut tmp_session, - senders, - convert_txn_args(script.args()), - &loaded_func, - self.features().is_enabled(FeatureFlag::STRUCT_CONSTRUCTORS), - )?; - - return_on_failure!(tmp_session.execute_script( - script.code(), - script.ty_args().to_vec(), - args, - &mut gas_meter, - )); + self.validate_and_execute_script( + &mut tmp_session, + &mut UnmeteredGasMeter, + senders, + script, + )?; Ok(tmp_session.finish(&change_set_configs)?) }, } From 9a821e23982f00b78d31834da016e311b39afa71 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Sat, 3 Feb 2024 10:36:26 +0000 Subject: [PATCH 07/15] fix test --- aptos-move/e2e-move-tests/src/tests/account.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/aptos-move/e2e-move-tests/src/tests/account.rs b/aptos-move/e2e-move-tests/src/tests/account.rs index e447213d445b6..fddf1683fded8 100644 --- a/aptos-move/e2e-move-tests/src/tests/account.rs +++ b/aptos-move/e2e-move-tests/src/tests/account.rs @@ -17,8 +17,9 @@ fn non_existent_sender() { 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,); + assert_err_eq!(status.status(), StatusCode::SENDING_ACCOUNT_DOES_NOT_EXIST); } From a239e0eb9018419e005bb321ec12e943e06513b0 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Fri, 9 Feb 2024 10:50:18 +0100 Subject: [PATCH 08/15] fix merge --- aptos-move/aptos-vm/src/move_vm_ext/vm.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/aptos-move/aptos-vm/src/move_vm_ext/vm.rs b/aptos-move/aptos-vm/src/move_vm_ext/vm.rs index 86a54c7432f66..05064d6fc16c7 100644 --- a/aptos-move/aptos-vm/src/move_vm_ext/vm.rs +++ b/aptos-move/aptos-vm/src/move_vm_ext/vm.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, @@ -244,10 +241,6 @@ impl MoveVmExt { ) } - pub(crate) fn chain_id(&self) -> ChainId { - ChainId::new(self.chain_id) - } - pub(crate) fn features(&self) -> &Features { &self.features } From cc7e03abc94c1d72043a6030491441404de8b668 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Fri, 9 Feb 2024 11:32:56 +0100 Subject: [PATCH 09/15] Fix script hash calculation --- .../aptos-vm/src/move_vm_ext/session.rs | 30 +++++++++++++++++++ aptos-move/aptos-vm/src/move_vm_ext/vm.rs | 23 +------------- 2 files changed, 31 insertions(+), 22 deletions(-) diff --git a/aptos-move/aptos-vm/src/move_vm_ext/session.rs b/aptos-move/aptos-vm/src/move_vm_ext/session.rs index 17b8f9483c631..8e0ef389b37eb 100644 --- a/aptos-move/aptos-vm/src/move_vm_ext/session.rs +++ b/aptos-move/aptos-vm/src/move_vm_ext/session.rs @@ -151,6 +151,36 @@ impl SessionId { pub fn as_uuid(&self) -> HashValue { self.hash() } + + pub(crate) fn into_script_hash(self) -> Vec { + 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> { diff --git a/aptos-move/aptos-vm/src/move_vm_ext/vm.rs b/aptos-move/aptos-vm/src/move_vm_ext/vm.rs index 05064d6fc16c7..95ee5ace89221 100644 --- a/aptos-move/aptos-vm/src/move_vm_ext/vm.rs +++ b/aptos-move/aptos-vm/src/move_vm_ext/vm.rs @@ -200,30 +200,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()); From 3874189811fb6ceb570860f1c72964db577358b4 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Fri, 9 Feb 2024 23:55:30 +0100 Subject: [PATCH 10/15] address comments --- aptos-move/aptos-vm/src/aptos_vm.rs | 43 +++++----------------- aptos-move/aptos-vm/src/zkid_validation.rs | 18 +++++++-- 2 files changed, 24 insertions(+), 37 deletions(-) diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index 3d83adb44c8cc..1445de9319fd6 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -59,7 +59,6 @@ use aptos_types::{ ViewFunctionOutput, WriteSetPayload, }, vm_status::{AbortLocation, StatusCode, VMStatus}, - zkid::ZkpOrOpenIdSig, }; use aptos_utils::{aptos_try, return_on_failure}; use aptos_vm_logging::{log_schema::AdapterLogSchema, speculative_error, speculative_log}; @@ -1365,30 +1364,9 @@ impl AptosVM { )); } - // zkID feature gating - let authenticators = aptos_types::zkid::get_zkid_authenticators(transaction); - match &authenticators { - Ok(authenticators) => { - for (_, sig) in authenticators { - if !self.features().is_zkid_enabled() - && matches!(sig.sig, ZkpOrOpenIdSig::Groth16Zkp { .. }) - { - return Err(VMStatus::error(StatusCode::FEATURE_UNDER_GATING, None)); - } - if (!self.features().is_zkid_enabled() - || !self.features().is_zkid_zkless_enabled()) - && matches!(sig.sig, ZkpOrOpenIdSig::OpenIdSig { .. }) - { - return Err(VMStatus::error(StatusCode::FEATURE_UNDER_GATING, None)); - } - } - }, - Err(_) => { - return Err(VMStatus::error(StatusCode::INVALID_SIGNATURE, None)); - }, - } - - zkid_validation::validate_zkid_authenticators(&authenticators.unwrap(), resolver)?; + let authenticators = aptos_types::zkid::get_zkid_authenticators(transaction) + .map_err(|_| VMStatus::error(StatusCode::INVALID_SIGNATURE, None))?; + zkid_validation::validate_zkid_authenticators(&authenticators, self.features(), resolver)?; // The prologue MUST be run AFTER any validation. Otherwise you may run prologue and hit // SEQUENCE_NUMBER_TOO_NEW if there is more than one transaction from the same sender and @@ -1846,7 +1824,7 @@ impl AptosVM { }; let mut session = vm.new_session(&resolver, SessionId::Void); - match Self::execute_view_function_in_vm( + let execution_result = Self::execute_view_function_in_vm( &mut session, &vm, module_id, @@ -1854,14 +1832,11 @@ impl AptosVM { type_args, arguments, &mut gas_meter, - ) { - Ok(result) => ViewFunctionOutput::new( - Ok(result), - Self::gas_used(max_gas_amount.into(), &gas_meter), - ), - Err(e) => { - ViewFunctionOutput::new(Err(e), Self::gas_used(max_gas_amount.into(), &gas_meter)) - }, + ); + let gas_used = Self::gas_used(max_gas_amount.into(), &gas_meter); + match execution_result { + Ok(result) => ViewFunctionOutput::new(Ok(result), gas_used), + Err(e) => ViewFunctionOutput::new(Err(e), gas_used), } } diff --git a/aptos-move/aptos-vm/src/zkid_validation.rs b/aptos-move/aptos-vm/src/zkid_validation.rs index d449f7ebe0c29..fae94c3a984d5 100644 --- a/aptos-move/aptos-vm/src/zkid_validation.rs +++ b/aptos-move/aptos-vm/src/zkid_validation.rs @@ -8,7 +8,7 @@ use aptos_types::{ bn254_circom::{get_public_inputs_hash, Groth16VerificationKey}, 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, @@ -90,16 +90,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")); } From 0c8fbc02a9ed3c87b0dc0e4a02b8f67cea51d243 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Wed, 14 Feb 2024 00:44:17 +0100 Subject: [PATCH 11/15] fix merge --- aptos-move/aptos-vm/src/aptos_vm.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index 34f3af2833d88..ae18803789207 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -48,8 +48,8 @@ use aptos_types::{ transaction::{ authenticator::AnySignature, signature_verified_transaction::SignatureVerifiedTransaction, BlockOutput, EntryFunction, ExecutionError, ExecutionStatus, ModuleBundle, Multisig, - MultisigTransactionPayload, SignatureCheckedTransaction, SignedTransaction, Transaction, - TransactionOutput, TransactionPayload, TransactionStatus, VMValidatorResult, + MultisigTransactionPayload, Script, SignatureCheckedTransaction, SignedTransaction, + Transaction, TransactionOutput, TransactionPayload, TransactionStatus, VMValidatorResult, ViewFunctionOutput, WriteSetPayload, }, vm_status::{AbortLocation, StatusCode, VMStatus}, @@ -69,8 +69,8 @@ use move_binary_format::{ access::ModuleAccess, compatibility::Compatibility, deserializer::DeserializerConfig, - errors::{verification_error, Location, PartialVMError, PartialVMResult, VMError, VMResult}, - CompiledModule, IndexKind, + errors::{Location, PartialVMError, PartialVMResult, VMError, VMResult}, + CompiledModule, }; use move_core_types::{ account_address::AccountAddress, From 5dbcf88f6b028aaf905aa22c37ab4afe7afac269 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Wed, 14 Feb 2024 13:39:16 +0100 Subject: [PATCH 12/15] re-add and re-write failed transaction cleanup test --- Cargo.lock | 1 + aptos-move/aptos-vm/src/aptos_vm.rs | 87 ++++++++++------------ aptos-move/e2e-move-tests/Cargo.toml | 1 + aptos-move/e2e-move-tests/src/tests/mod.rs | 1 + aptos-move/e2e-move-tests/src/tests/vm.rs | 40 ++++++++++ 5 files changed, 84 insertions(+), 46 deletions(-) create mode 100644 aptos-move/e2e-move-tests/src/tests/vm.rs diff --git a/Cargo.lock b/Cargo.lock index 42018b4d93a8b..324b167cba450 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7182,6 +7182,7 @@ dependencies = [ "aptos-vm-genesis", "bcs 0.1.4", "claims", + "fail 0.5.1", "hex", "itertools 0.10.5", "move-binary-format", diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index ae18803789207..01ea9db682343 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -671,7 +671,11 @@ impl AptosVM { new_published_modules_loaded: &mut bool, change_set_configs: &ChangeSetConfigs, ) -> Result<(VMStatus, VMOutput), VMStatus> { - fail_point!("move_adapter::execute_script_or_entry_function", |_| { + gas_meter.charge_intrinsic_gas_for_transaction(txn_data.transaction_size())?; + + // For testing failed transactions cleanup: simulates failures in + // scripts or entry functions. + fail_point!("aptos_vm::execute_script_or_entry_function", |_| { Err(VMStatus::Error { status_code: StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR, sub_status: Some(move_core_types::vm_status::sub_status::unknown_invariant_violation::EPARANOID_FAILURE), @@ -679,55 +683,46 @@ impl AptosVM { }) }); - // Run the execution logic - { - gas_meter.charge_intrinsic_gas_for_transaction(txn_data.transaction_size())?; - - match payload { - TransactionPayload::Script(script) => { - self.validate_and_execute_script( - &mut session, - gas_meter, - txn_data.senders(), - script, - )?; - }, - TransactionPayload::EntryFunction(entry_fn) => { - self.validate_and_execute_entry_function( - &mut session, - gas_meter, - txn_data.senders(), - entry_fn, - )?; - }, + match payload { + TransactionPayload::Script(script) => { + self.validate_and_execute_script( + &mut session, + gas_meter, + txn_data.senders(), + script, + )?; + }, + TransactionPayload::EntryFunction(entry_fn) => { + self.validate_and_execute_entry_function( + &mut session, + gas_meter, + txn_data.senders(), + entry_fn, + )?; + }, - // Not reachable as this function should only be invoked for entry or script - // transaction payload. - _ => unreachable!("Only scripts or entry functions are executed"), - }; + // Not reachable as this function should only be invoked for entry or script + // transaction payload. + _ => unreachable!("Only scripts or entry functions are executed"), + }; - self.resolve_pending_code_publish( - &mut session, - gas_meter, - new_published_modules_loaded, - )?; + self.resolve_pending_code_publish(&mut session, gas_meter, new_published_modules_loaded)?; - let respawned_session = self.charge_change_set_and_respawn_session( - session, - resolver, - gas_meter, - change_set_configs, - txn_data, - )?; + let respawned_session = self.charge_change_set_and_respawn_session( + session, + resolver, + gas_meter, + change_set_configs, + txn_data, + )?; - self.success_transaction_cleanup( - respawned_session, - gas_meter, - txn_data, - log_context, - change_set_configs, - ) - } + self.success_transaction_cleanup( + respawned_session, + gas_meter, + txn_data, + log_context, + change_set_configs, + ) } fn charge_change_set( diff --git a/aptos-move/e2e-move-tests/Cargo.toml b/aptos-move/e2e-move-tests/Cargo.toml index 2e678a49170b4..572eaf38547c0 100644 --- a/aptos-move/e2e-move-tests/Cargo.toml +++ b/aptos-move/e2e-move-tests/Cargo.toml @@ -30,6 +30,7 @@ aptos-vm = { workspace = true, features = ["testing"] } aptos-vm-genesis = { workspace = true } bcs = { workspace = true } claims = { workspace = true } +fail = { workspace = true, features = ["failpoints"] } hex = { workspace = true } itertools = { workspace = true } move-binary-format = { workspace = true } diff --git a/aptos-move/e2e-move-tests/src/tests/mod.rs b/aptos-move/e2e-move-tests/src/tests/mod.rs index a56fd879d2099..bbc92fb0842f5 100644 --- a/aptos-move/e2e-move-tests/src/tests/mod.rs +++ b/aptos-move/e2e-move-tests/src/tests/mod.rs @@ -45,4 +45,5 @@ mod token_objects; mod transaction_fee; mod type_too_large; mod vector_numeric_address; +mod vm; mod vote; diff --git a/aptos-move/e2e-move-tests/src/tests/vm.rs b/aptos-move/e2e-move-tests/src/tests/vm.rs new file mode 100644 index 0000000000000..6cc7eb4d60ee4 --- /dev/null +++ b/aptos-move/e2e-move-tests/src/tests/vm.rs @@ -0,0 +1,40 @@ +// Copyright © Aptos Foundation +// SPDX-License-Identifier: Apache-2.0 + +use crate::MoveHarness; +use aptos_cached_packages::aptos_stdlib::aptos_account_transfer; +use aptos_types::transaction::ExecutionStatus; +use claims::assert_ok_eq; +use fail::FailScenario; +use move_core_types::vm_status::StatusCode; + +#[test] +fn failed_transaction_cleanup_charges_gas() { + // Fail in user transaction so we can run failed transaction cleanup. + let scenario = FailScenario::setup(); + assert!(fail::has_failpoints()); + fail::cfg("aptos_vm::execute_script_or_entry_function", "return()").unwrap(); + assert!(!fail::list().is_empty()); + + // Actual transaction is correct, so that we get to the failpoint. + let mut h = MoveHarness::new(); + let sender = h.new_account_with_balance_and_sequence_number(1_000_000, 10); + let receiver = h.new_account_with_balance_and_sequence_number(1_000_000, 10); + let txn = sender + .transaction() + .sequence_number(10) + .payload(aptos_account_transfer(*receiver.address(), 1)) + .sign(); + let output = h.run_block_get_output(vec![txn]).pop().unwrap(); + + // After failures in user transactions, even if these are invariant violations, + // gas should still be charged. + assert_ne!(output.gas_used(), 0); + assert!(!output.status().is_discarded()); + assert_ok_eq!( + output.status().status(), + ExecutionStatus::MiscellaneousError(Some(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR)) + ); + + scenario.teardown(); +} From a1884f264bc7ab67784562779ea18c20e983a89d Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Fri, 16 Feb 2024 15:43:19 +0100 Subject: [PATCH 13/15] revisit tests --- Cargo.lock | 1 - aptos-move/aptos-vm/src/aptos_vm.rs | 39 ++++++++++++++---- aptos-move/e2e-move-tests/Cargo.toml | 1 - aptos-move/e2e-move-tests/src/tests/vm.rs | 48 +++++++++++++---------- 4 files changed, 59 insertions(+), 30 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 324b167cba450..42018b4d93a8b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7182,7 +7182,6 @@ dependencies = [ "aptos-vm-genesis", "bcs 0.1.4", "claims", - "fail 0.5.1", "hex", "itertools 0.10.5", "move-binary-format", diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index 01ea9db682343..0407b0ca53dd4 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -44,7 +44,7 @@ use aptos_types::{ new_epoch_event_key, ConfigurationResource, FeatureFlag, Features, OnChainConfig, TimedFeatureOverride, TimedFeatures, TimedFeaturesBuilder, }, - state_store::StateView, + state_store::{StateView, TStateView}, transaction::{ authenticator::AnySignature, signature_verified_transaction::SignatureVerifiedTransaction, BlockOutput, EntryFunction, ExecutionError, ExecutionStatus, ModuleBundle, Multisig, @@ -357,6 +357,35 @@ impl AptosVM { ) } + #[cfg(any(test, feature = "testing"))] + pub fn test_failed_transaction_cleanup( + &self, + error_vm_status: VMStatus, + txn: &SignedTransaction, + state_view: &impl StateView, + ) -> (VMStatus, VMOutput) { + let txn_data = TransactionMetadata::new(txn); + let log_context = AdapterLogSchema::new(state_view.id(), 0); + + let balance = txn_data.max_gas_amount(); + let mut gas_meter = self + .make_standard_gas_meter(balance, &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, + ) + } + fn failed_transaction_cleanup( &self, error_vm_status: VMStatus, @@ -671,10 +700,6 @@ impl AptosVM { new_published_modules_loaded: &mut bool, change_set_configs: &ChangeSetConfigs, ) -> Result<(VMStatus, VMOutput), VMStatus> { - gas_meter.charge_intrinsic_gas_for_transaction(txn_data.transaction_size())?; - - // For testing failed transactions cleanup: simulates failures in - // scripts or entry functions. fail_point!("aptos_vm::execute_script_or_entry_function", |_| { Err(VMStatus::Error { status_code: StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR, @@ -683,6 +708,8 @@ impl AptosVM { }) }); + gas_meter.charge_intrinsic_gas_for_transaction(txn_data.transaction_size())?; + match payload { TransactionPayload::Script(script) => { self.validate_and_execute_script( @@ -2198,8 +2225,6 @@ fn vm_thread_safe() { fn assert_send() {} fn assert_sync() {} - use crate::AptosVM; - assert_send::(); assert_sync::(); assert_send::(); diff --git a/aptos-move/e2e-move-tests/Cargo.toml b/aptos-move/e2e-move-tests/Cargo.toml index 572eaf38547c0..2e678a49170b4 100644 --- a/aptos-move/e2e-move-tests/Cargo.toml +++ b/aptos-move/e2e-move-tests/Cargo.toml @@ -30,7 +30,6 @@ aptos-vm = { workspace = true, features = ["testing"] } aptos-vm-genesis = { workspace = true } bcs = { workspace = true } claims = { workspace = true } -fail = { workspace = true, features = ["failpoints"] } hex = { workspace = true } itertools = { workspace = true } move-binary-format = { workspace = true } diff --git a/aptos-move/e2e-move-tests/src/tests/vm.rs b/aptos-move/e2e-move-tests/src/tests/vm.rs index 6cc7eb4d60ee4..1b07b9502f3f0 100644 --- a/aptos-move/e2e-move-tests/src/tests/vm.rs +++ b/aptos-move/e2e-move-tests/src/tests/vm.rs @@ -3,38 +3,44 @@ use crate::MoveHarness; use aptos_cached_packages::aptos_stdlib::aptos_account_transfer; -use aptos_types::transaction::ExecutionStatus; -use claims::assert_ok_eq; -use fail::FailScenario; -use move_core_types::vm_status::StatusCode; +use aptos_types::{ + state_store::state_key::StateKey, transaction::ExecutionStatus, write_set::WriteOp, +}; +use aptos_vm::{data_cache::AsMoveResolver, AptosVM}; +use claims::{assert_ok_eq, assert_some}; +use move_core_types::vm_status::{StatusCode, VMStatus}; +use test_case::test_case; -#[test] -fn failed_transaction_cleanup_charges_gas() { - // Fail in user transaction so we can run failed transaction cleanup. - let scenario = FailScenario::setup(); - assert!(fail::has_failpoints()); - fail::cfg("aptos_vm::execute_script_or_entry_function", "return()").unwrap(); - assert!(!fail::list().is_empty()); - - // Actual transaction is correct, so that we get to the failpoint. +// Make sure verification and invariant violation errors are kept. +#[test_case(StatusCode::TYPE_MISMATCH)] +#[test_case(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR)] +fn failed_transaction_cleanup_charges_gas(status_code: StatusCode) { let mut h = MoveHarness::new(); let sender = h.new_account_with_balance_and_sequence_number(1_000_000, 10); let receiver = h.new_account_with_balance_and_sequence_number(1_000_000, 10); let txn = sender .transaction() .sequence_number(10) + .max_gas_amount(10_000) .payload(aptos_account_transfer(*receiver.address(), 1)) .sign(); - let output = h.run_block_get_output(vec![txn]).pop().unwrap(); - // After failures in user transactions, even if these are invariant violations, - // gas should still be charged. - assert_ne!(output.gas_used(), 0); + let state_view = h.executor.get_state_view(); + let vm = AptosVM::new(&state_view.as_move_resolver()); + + let output = vm + .test_failed_transaction_cleanup(VMStatus::error(status_code, None), &txn, state_view) + .1; + let write_set: Vec<(&StateKey, &WriteOp)> = output + .change_set() + .concrete_write_set_iter() + .map(|(k, v)| (k, assert_some!(v))) + .collect(); + assert!(!write_set.is_empty()); + assert!(!output.status().is_discarded()); assert_ok_eq!( - output.status().status(), - ExecutionStatus::MiscellaneousError(Some(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR)) + output.status().as_kept_status(), + ExecutionStatus::MiscellaneousError(Some(status_code)) ); - - scenario.teardown(); } From b8ef1471bf85c948d6b85fa2565f7a78fdb18694 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Fri, 16 Feb 2024 22:59:59 +0100 Subject: [PATCH 14/15] moved impl to testing --- aptos-move/aptos-vm/src/aptos_vm.rs | 33 ++----------------- aptos-move/aptos-vm/src/testing.rs | 39 +++++++++++++++++++++++ aptos-move/e2e-move-tests/src/tests/vm.rs | 15 +++++++-- 3 files changed, 53 insertions(+), 34 deletions(-) diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index 0407b0ca53dd4..e704c3ffd012f 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -357,36 +357,7 @@ impl AptosVM { ) } - #[cfg(any(test, feature = "testing"))] - pub fn test_failed_transaction_cleanup( - &self, - error_vm_status: VMStatus, - txn: &SignedTransaction, - state_view: &impl StateView, - ) -> (VMStatus, VMOutput) { - let txn_data = TransactionMetadata::new(txn); - let log_context = AdapterLogSchema::new(state_view.id(), 0); - - let balance = txn_data.max_gas_amount(); - let mut gas_meter = self - .make_standard_gas_meter(balance, &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, - ) - } - - fn failed_transaction_cleanup( + pub(crate) fn failed_transaction_cleanup( &self, error_vm_status: VMStatus, gas_meter: &mut impl AptosGasMeter, @@ -1254,7 +1225,7 @@ impl AptosVM { .finish(Location::Undefined) } - fn make_standard_gas_meter( + pub(crate) fn make_standard_gas_meter( &self, balance: Gas, log_context: &AdapterLogSchema, diff --git a/aptos-move/aptos-vm/src/testing.rs b/aptos-move/aptos-vm/src/testing.rs index 37d3b71e2df99..baae2c3f98705 100644 --- a/aptos-move/aptos-vm/src/testing.rs +++ b/aptos-move/aptos-vm/src/testing.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, + ) + } +} diff --git a/aptos-move/e2e-move-tests/src/tests/vm.rs b/aptos-move/e2e-move-tests/src/tests/vm.rs index 1b07b9502f3f0..a48c4604be3e1 100644 --- a/aptos-move/e2e-move-tests/src/tests/vm.rs +++ b/aptos-move/e2e-move-tests/src/tests/vm.rs @@ -18,26 +18,35 @@ fn failed_transaction_cleanup_charges_gas(status_code: StatusCode) { let mut h = MoveHarness::new(); let sender = h.new_account_with_balance_and_sequence_number(1_000_000, 10); let receiver = h.new_account_with_balance_and_sequence_number(1_000_000, 10); + + let max_gas_amount = 100_000; let txn = sender .transaction() .sequence_number(10) - .max_gas_amount(10_000) + .max_gas_amount(max_gas_amount) .payload(aptos_account_transfer(*receiver.address(), 1)) .sign(); let state_view = h.executor.get_state_view(); let vm = AptosVM::new(&state_view.as_move_resolver()); + let balance = 10_000; let output = vm - .test_failed_transaction_cleanup(VMStatus::error(status_code, None), &txn, state_view) + .test_failed_transaction_cleanup( + VMStatus::error(status_code, None), + &txn, + state_view, + balance, + ) .1; + let write_set: Vec<(&StateKey, &WriteOp)> = output .change_set() .concrete_write_set_iter() .map(|(k, v)| (k, assert_some!(v))) .collect(); assert!(!write_set.is_empty()); - + assert_eq!(output.gas_used(), max_gas_amount - balance); assert!(!output.status().is_discarded()); assert_ok_eq!( output.status().as_kept_status(), From 2421a1e40ef3a63016bf3ff91312c5904a9e66fd Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Tue, 20 Feb 2024 00:12:06 +0000 Subject: [PATCH 15/15] fix failpoint test --- aptos-move/e2e-testsuite/src/tests/invariant_violation.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/aptos-move/e2e-testsuite/src/tests/invariant_violation.rs b/aptos-move/e2e-testsuite/src/tests/invariant_violation.rs index 2a7d94f972757..9cef3a8dcb6b5 100644 --- a/aptos-move/e2e-testsuite/src/tests/invariant_violation.rs +++ b/aptos-move/e2e-testsuite/src/tests/invariant_violation.rs @@ -13,17 +13,12 @@ use move_core_types::{value::MoveValue, vm_status::StatusCode}; #[test] fn invariant_violation_error() { let _scenario = fail::FailScenario::setup(); - fail::cfg( - "move_adapter::execute_script_or_entry_function", - "100%return", - ) - .unwrap(); + fail::cfg("aptos_vm::execute_script_or_entry_function", "100%return").unwrap(); ::aptos_logger::Logger::init_for_testing(); let mut executor = FakeExecutor::from_head_genesis(); - // create and publish a sender with 1_000_000 coins and a receiver with 100_000 coins let sender = executor.create_raw_account_data(1_000_000, 10); let receiver = executor.create_raw_account_data(100_000, 10); executor.add_account_data(&sender);