From 47f0bf397eb5ca43257bfa775a6b28e7b7faf8b3 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Wed, 20 Nov 2024 20:34:36 +0000 Subject: [PATCH] [loader-v2] Small cleanups & tests (#15279) - Fixed naming for global module cache. - Added more counters, moved some old ones. - Added unit tests for TransactionSliceMetadata + renaming. --- Cargo.lock | 1 - .../aptos-debugger/src/aptos_debugger.rs | 2 +- .../src/transaction_bench_state.rs | 2 +- aptos-move/aptos-vm-environment/Cargo.toml | 2 +- .../aptos-vm-environment/src/environment.rs | 4 +- aptos-move/aptos-vm/src/aptos_vm.rs | 2 +- aptos-move/aptos-vm/src/block_executor/mod.rs | 4 +- aptos-move/aptos-vm/src/lib.rs | 4 +- .../sharded_executor_service.rs | 2 +- .../block-executor/src/captured_reads.rs | 10 +- aptos-move/block-executor/src/code_cache.rs | 7 +- .../block-executor/src/code_cache_global.rs | 110 ++++++------ .../src/code_cache_global_manager.rs | 5 +- aptos-move/block-executor/src/counters.rs | 13 +- aptos-move/block-executor/src/executor.rs | 6 +- aptos-move/e2e-tests/src/executor.rs | 2 +- .../executor-benchmark/src/native_executor.rs | 4 +- execution/executor/src/block_executor/mod.rs | 4 +- execution/executor/src/chunk_executor/mod.rs | 3 +- .../src/chunk_executor/transaction_chunk.rs | 3 +- execution/executor/src/db_bootstrapper/mod.rs | 3 +- execution/executor/src/fuzzing.rs | 4 +- execution/executor/src/tests/mock_vm/mod.rs | 4 +- execution/executor/src/tests/mod.rs | 3 +- .../src/workflow/do_get_execution_output.rs | 2 +- .../execution/ptx-executor/src/lib.rs | 4 +- .../move/move-vm/runtime/src/loader/mod.rs | 9 +- .../runtime/src/storage/environment.rs | 5 + .../runtime/src/storage/module_storage.rs | 3 + types/Cargo.toml | 2 +- types/src/block_executor/execution_state.rs | 107 ----------- types/src/block_executor/mod.rs | 2 +- .../transaction_slice_metadata.rs | 169 ++++++++++++++++++ types/src/transaction/webauthn.rs | 2 +- 34 files changed, 298 insertions(+), 211 deletions(-) delete mode 100644 types/src/block_executor/execution_state.rs create mode 100644 types/src/block_executor/transaction_slice_metadata.rs diff --git a/Cargo.lock b/Cargo.lock index 0e9aa46fcaee1..302f8c7eb3ef6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4533,7 +4533,6 @@ dependencies = [ "aptos-framework", "aptos-gas-algebra", "aptos-gas-schedule", - "aptos-language-e2e-tests", "aptos-move-stdlib", "aptos-native-interface", "aptos-table-natives", diff --git a/aptos-move/aptos-debugger/src/aptos_debugger.rs b/aptos-move/aptos-debugger/src/aptos_debugger.rs index b06f804c4bc18..2f6c08107f9ab 100644 --- a/aptos-move/aptos-debugger/src/aptos_debugger.rs +++ b/aptos-move/aptos-debugger/src/aptos_debugger.rs @@ -13,7 +13,7 @@ use aptos_types::{ account_address::AccountAddress, block_executor::{ config::{BlockExecutorConfig, BlockExecutorConfigFromOnchain, BlockExecutorLocalConfig}, - execution_state::TransactionSliceMetadata, + transaction_slice_metadata::TransactionSliceMetadata, }, contract_event::ContractEvent, state_store::TStateView, diff --git a/aptos-move/aptos-transaction-benchmarks/src/transaction_bench_state.rs b/aptos-move/aptos-transaction-benchmarks/src/transaction_bench_state.rs index 8feb67adefb88..39446d7943fe2 100644 --- a/aptos-move/aptos-transaction-benchmarks/src/transaction_bench_state.rs +++ b/aptos-move/aptos-transaction-benchmarks/src/transaction_bench_state.rs @@ -20,8 +20,8 @@ use aptos_language_e2e_tests::{ use aptos_types::{ block_executor::{ config::{BlockExecutorConfig, BlockExecutorConfigFromOnchain}, - execution_state::TransactionSliceMetadata, partitioner::PartitionedTransactions, + transaction_slice_metadata::TransactionSliceMetadata, }, block_metadata::BlockMetadata, on_chain_config::{OnChainConfig, ValidatorSet}, diff --git a/aptos-move/aptos-vm-environment/Cargo.toml b/aptos-move/aptos-vm-environment/Cargo.toml index 2ae1c4c98e7d2..5274a6694c00f 100644 --- a/aptos-move/aptos-vm-environment/Cargo.toml +++ b/aptos-move/aptos-vm-environment/Cargo.toml @@ -31,4 +31,4 @@ once_cell = { workspace = true } sha3 = { workspace = true } [dev-dependencies] -aptos-language-e2e-tests = { workspace = true } +aptos-types = { workspace = true, features = ["testing"] } diff --git a/aptos-move/aptos-vm-environment/src/environment.rs b/aptos-move/aptos-vm-environment/src/environment.rs index 151dc19247ef9..debdd654c92ac 100644 --- a/aptos-move/aptos-vm-environment/src/environment.rs +++ b/aptos-move/aptos-vm-environment/src/environment.rs @@ -272,12 +272,12 @@ fn fetch_config_and_update_hash( #[cfg(test)] pub mod test { use super::*; - use aptos_language_e2e_tests::data_store::FakeDataStore; + use aptos_types::state_store::MockStateView; #[test] fn test_new_environment() { // This creates an empty state. - let state_view = FakeDataStore::default(); + let state_view = MockStateView::empty(); let env = Environment::new(&state_view, false, None); // Check default values. diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index cc94c805c21aa..16ff4a35bcdc6 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -51,8 +51,8 @@ use aptos_types::{ BlockExecutorConfig, BlockExecutorConfigFromOnchain, BlockExecutorLocalConfig, BlockExecutorModuleCacheLocalConfig, }, - execution_state::TransactionSliceMetadata, partitioner::PartitionedTransactions, + transaction_slice_metadata::TransactionSliceMetadata, }, block_metadata::BlockMetadata, block_metadata_ext::{BlockMetadataExt, BlockMetadataWithRandomness}, diff --git a/aptos-move/aptos-vm/src/block_executor/mod.rs b/aptos-move/aptos-vm/src/block_executor/mod.rs index 838ebd1af2aed..f136e461a8c60 100644 --- a/aptos-move/aptos-vm/src/block_executor/mod.rs +++ b/aptos-move/aptos-vm/src/block_executor/mod.rs @@ -18,7 +18,9 @@ use aptos_block_executor::{ }; use aptos_infallible::Mutex; use aptos_types::{ - block_executor::{config::BlockExecutorConfig, execution_state::TransactionSliceMetadata}, + block_executor::{ + config::BlockExecutorConfig, transaction_slice_metadata::TransactionSliceMetadata, + }, contract_event::ContractEvent, error::PanicError, executable::ExecutableTestType, diff --git a/aptos-move/aptos-vm/src/lib.rs b/aptos-move/aptos-vm/src/lib.rs index a7526bdf03bf5..d18fde5c39b5c 100644 --- a/aptos-move/aptos-vm/src/lib.rs +++ b/aptos-move/aptos-vm/src/lib.rs @@ -129,8 +129,8 @@ use crate::sharded_block_executor::{executor_client::ExecutorClient, ShardedBloc use aptos_block_executor::txn_provider::default::DefaultTxnProvider; use aptos_types::{ block_executor::{ - config::BlockExecutorConfigFromOnchain, execution_state::TransactionSliceMetadata, - partitioner::PartitionedTransactions, + config::BlockExecutorConfigFromOnchain, partitioner::PartitionedTransactions, + transaction_slice_metadata::TransactionSliceMetadata, }, state_store::StateView, transaction::{ diff --git a/aptos-move/aptos-vm/src/sharded_block_executor/sharded_executor_service.rs b/aptos-move/aptos-vm/src/sharded_block_executor/sharded_executor_service.rs index cc41d58e83050..5d68d52a14e1b 100644 --- a/aptos-move/aptos-vm/src/sharded_block_executor/sharded_executor_service.rs +++ b/aptos-move/aptos-vm/src/sharded_block_executor/sharded_executor_service.rs @@ -23,8 +23,8 @@ use aptos_logger::{info, trace}; use aptos_types::{ block_executor::{ config::{BlockExecutorConfig, BlockExecutorLocalConfig}, - execution_state::TransactionSliceMetadata, partitioner::{ShardId, SubBlock, SubBlocksForShard, TransactionWithDependencies}, + transaction_slice_metadata::TransactionSliceMetadata, }, state_store::StateView, transaction::{ diff --git a/aptos-move/block-executor/src/captured_reads.rs b/aptos-move/block-executor/src/captured_reads.rs index 3deff5419c8a6..7a61f5e3fc16c 100644 --- a/aptos-move/block-executor/src/captured_reads.rs +++ b/aptos-move/block-executor/src/captured_reads.rs @@ -648,7 +648,7 @@ where } /// For every module read that was captured, checks if the reads are still the same: - /// 1. Entries read from the global cross-block module cache are still valid. + /// 1. Entries read from the global module cache are not overridden. /// 2. Entries that were not in per-block cache before are still not there. /// 3. Entries that were in per-block cache have the same commit index. pub(crate) fn validate_module_reads( @@ -661,7 +661,7 @@ where } self.module_reads.iter().all(|(key, read)| match read { - ModuleRead::GlobalCache(_) => global_module_cache.contains_valid(key), + ModuleRead::GlobalCache(_) => global_module_cache.contains_not_overridden(key), ModuleRead::PerBlockCache(previous) => { let current_version = per_block_module_cache.get_module_version(key); let previous_version = previous.as_ref().map(|(_, version)| *version); @@ -1560,7 +1560,7 @@ mod test { assert!(captured_reads.validate_module_reads(&global_module_cache, &per_block_module_cache)); // Now, mark one of the entries in invalid. Validations should fail! - global_module_cache.mark_invalid_if_contains(&1); + global_module_cache.mark_overridden(&1); let valid = captured_reads.validate_module_reads(&global_module_cache, &per_block_module_cache); assert!(!valid); @@ -1696,7 +1696,7 @@ mod test { // Assume we republish this module: validation must fail. let a = mock_deserialized_code(100, MockExtension::new(8)); - global_module_cache.mark_invalid_if_contains(&0); + global_module_cache.mark_overridden(&0); per_block_module_cache .insert_deserialized_module( 0, @@ -1713,6 +1713,6 @@ mod test { // Assume we re-read the new correct version. Then validation should pass again. captured_reads.capture_per_block_cache_read(0, Some((a, Some(10)))); assert!(captured_reads.validate_module_reads(&global_module_cache, &per_block_module_cache)); - assert!(!global_module_cache.contains_valid(&0)); + assert!(!global_module_cache.contains_not_overridden(&0)); } } diff --git a/aptos-move/block-executor/src/code_cache.rs b/aptos-move/block-executor/src/code_cache.rs index 081147ce45ae9..6465284f6e770 100644 --- a/aptos-move/block-executor/src/code_cache.rs +++ b/aptos-move/block-executor/src/code_cache.rs @@ -3,6 +3,7 @@ use crate::{ captured_reads::CacheRead, + counters::GLOBAL_MODULE_CACHE_MISS_SECONDS, view::{LatestView, ViewState}, }; use ambassador::delegate_to_methods; @@ -144,7 +145,7 @@ impl<'a, T: Transaction, S: TStateView, X: Executable> ModuleCache } // Otherwise, it is a miss. Check global cache. - if let Some(module) = self.global_module_cache.get_valid(key) { + if let Some(module) = self.global_module_cache.get(key) { state .captured_reads .borrow_mut() @@ -153,6 +154,7 @@ impl<'a, T: Transaction, S: TStateView, X: Executable> ModuleCache } // If not global cache, check per-block cache. + let _timer = GLOBAL_MODULE_CACHE_MISS_SECONDS.start_timer(); let read = state .versioned_map .module_cache() @@ -164,11 +166,12 @@ impl<'a, T: Transaction, S: TStateView, X: Executable> ModuleCache Ok(read) }, ViewState::Unsync(state) => { - if let Some(module) = self.global_module_cache.get_valid(key) { + if let Some(module) = self.global_module_cache.get(key) { state.read_set.borrow_mut().capture_module_read(key.clone()); return Ok(Some((module, Self::Version::default()))); } + let _timer = GLOBAL_MODULE_CACHE_MISS_SECONDS.start_timer(); let read = state .unsync_map .module_cache() diff --git a/aptos-move/block-executor/src/code_cache_global.rs b/aptos-move/block-executor/src/code_cache_global.rs index a74e96170c95c..18e0c0679a958 100644 --- a/aptos-move/block-executor/src/code_cache_global.rs +++ b/aptos-move/block-executor/src/code_cache_global.rs @@ -13,12 +13,12 @@ use std::{ }, }; -/// Entry stored in [GlobalModuleCache]. +/// Entry stored in [GlobalModuleCache]. Can be invalidated by module publishing. struct Entry { - /// True if this code is "valid" within the block execution context (i.e., there has been no - /// republishing of this module so far). If false, executor needs to read the module from the - /// sync/unsync module caches. - valid: AtomicBool, + /// False if this code is "valid" within the block execution context (i.e., there has been no + /// republishing of this module so far). If true, executor needs to read the module from the + /// per-block module caches. + overridden: AtomicBool, /// Cached verified module. Must always be verified. module: Arc>, } @@ -37,19 +37,19 @@ where } Ok(Self { - valid: AtomicBool::new(true), + overridden: AtomicBool::new(false), module, }) } - /// Marks the module as invalid. - fn mark_invalid(&self) { - self.valid.store(false, Ordering::Release) + /// Marks the module as overridden. + fn mark_overridden(&self) { + self.overridden.store(true, Ordering::Release) } - /// Returns true if the module is valid. - pub(crate) fn is_valid(&self) -> bool { - self.valid.load(Ordering::Acquire) + /// Returns true if the module is not overridden. + fn is_not_overridden(&self) -> bool { + !self.overridden.load(Ordering::Acquire) } /// Returns the module code stored is this [Entry]. @@ -81,28 +81,30 @@ where } } - /// Returns true if the key exists in cache and the corresponding module is valid. - pub fn contains_valid(&self, key: &K) -> bool { + /// Returns true if the key exists in cache and the corresponding module is not overridden. + pub fn contains_not_overridden(&self, key: &K) -> bool { self.module_cache .get(key) - .is_some_and(|entry| entry.is_valid()) + .is_some_and(|entry| entry.is_not_overridden()) } - /// Marks the cached module (if it exists) as invalid. As a result, all subsequent calls to the - /// cache for the associated key will result in a cache miss. If an entry does not to exist, it - /// is a no-op. - pub fn mark_invalid_if_contains(&self, key: &K) { + /// Marks the cached module (if it exists) as overridden. As a result, all subsequent calls to + /// the cache for the associated key will result in a cache miss. If an entry does not exist, + /// it is a no-op. + pub fn mark_overridden(&self, key: &K) { if let Some(entry) = self.module_cache.get(key) { - entry.mark_invalid(); + entry.mark_overridden(); } } /// Returns the module stored in cache. If the module has not been cached, or it exists but is - /// not valid, [None] is returned. - pub fn get_valid(&self, key: &K) -> Option>> { - self.module_cache - .get(key) - .and_then(|entry| entry.is_valid().then(|| Arc::clone(entry.module_code()))) + /// overridden, [None] is returned. + pub fn get(&self, key: &K) -> Option>> { + self.module_cache.get(key).and_then(|entry| { + entry + .is_not_overridden() + .then(|| Arc::clone(entry.module_code())) + }) } /// Returns the number of entries in the cache. @@ -124,9 +126,9 @@ where /// Inserts modules into the cache. /// Notes: /// 1. Only verified modules are inserted. - /// 2. Valid modules should not be removed, and new modules should have unique ownership. If - /// these constraints are violated, a panic error is returned. - pub fn insert_verified_unsync( + /// 2. Not overridden modules should not be removed, and new modules should have unique + /// ownership. If these constraints are violated, a panic error is returned. + pub fn insert_verified( &mut self, modules: impl Iterator>)>, ) -> Result<(), PanicError> { @@ -134,14 +136,12 @@ where for (key, module) in modules { if let Occupied(entry) = self.module_cache.entry(key.clone()) { - if entry.get().is_valid() { + if entry.get().is_not_overridden() { return Err(PanicError::CodeInvariantError( - "Should never overwrite a valid module".to_string(), + "Should never replace a non-overridden module".to_string(), )); } else { - // Otherwise, remove the invalid entry. - let size = entry.get().module_code().extension().size_in_bytes(); - self.size -= size; + self.size -= entry.get().module_code().extension().size_in_bytes(); entry.remove(); } } @@ -195,36 +195,36 @@ mod test { } #[test] - fn test_entry_mark_invalid() { + fn test_entry_mark_overridden() { let entry = assert_ok!(Entry::new(mock_verified_code(0, MockExtension::new(8)))); - assert!(entry.is_valid()); + assert!(entry.is_not_overridden()); - entry.mark_invalid(); - assert!(!entry.is_valid()); + entry.mark_overridden(); + assert!(!entry.is_not_overridden()); } #[test] - fn test_cache_contains_valid_and_get() { + fn test_cache_is_not_overridden_and_get() { let mut cache = GlobalModuleCache::empty(); // Set the state. cache.insert(0, mock_verified_code(0, MockExtension::new(8))); cache.insert(1, mock_verified_code(1, MockExtension::new(8))); - cache.mark_invalid_if_contains(&1); + cache.mark_overridden(&1); assert_eq!(cache.num_modules(), 2); - assert!(cache.contains_valid(&0)); - assert!(!cache.contains_valid(&1)); - assert!(!cache.contains_valid(&3)); + assert!(cache.contains_not_overridden(&0)); + assert!(!cache.contains_not_overridden(&1)); + assert!(!cache.contains_not_overridden(&3)); - assert!(cache.get_valid(&0).is_some()); - assert!(cache.get_valid(&1).is_none()); - assert!(cache.get_valid(&3).is_none()); + assert!(cache.get(&0).is_some()); + assert!(cache.get(&1).is_none()); + assert!(cache.get(&3).is_none()); } #[test] - fn test_cache_sizes_and_flush_unchecked() { + fn test_cache_sizes_and_flush() { let mut cache = GlobalModuleCache::empty(); assert_eq!(cache.num_modules(), 0); assert_eq!(cache.size_in_bytes(), 0); @@ -245,16 +245,14 @@ mod test { } #[test] - fn test_cache_insert_verified_unchecked() { + fn test_cache_insert_verified() { let mut cache = GlobalModuleCache::empty(); let mut new_modules = vec![]; for i in 0..10 { new_modules.push((i, mock_verified_code(i, MockExtension::new(8)))); } - assert!(cache - .insert_verified_unsync(new_modules.into_iter()) - .is_ok()); + assert_ok!(cache.insert_verified(new_modules.into_iter())); assert_eq!(cache.num_modules(), 10); assert_eq!(cache.size_in_bytes(), 80); @@ -265,14 +263,14 @@ mod test { let mut cache = GlobalModuleCache::empty(); let deserialized_modules = vec![(0, mock_deserialized_code(0, MockExtension::new(8)))]; - assert_ok!(cache.insert_verified_unsync(deserialized_modules.into_iter())); + assert_ok!(cache.insert_verified(deserialized_modules.into_iter())); assert_eq!(cache.num_modules(), 0); assert_eq!(cache.size_in_bytes(), 0); } #[test] - fn test_cache_insert_verified_unchecked_does_not_override_valid_modules() { + fn test_cache_insert_verified_does_not_override_valid_modules() { let mut cache = GlobalModuleCache::empty(); cache.insert(0, mock_verified_code(0, MockExtension::new(8))); @@ -280,20 +278,20 @@ mod test { assert_eq!(cache.size_in_bytes(), 8); let new_modules = vec![(0, mock_verified_code(100, MockExtension::new(32)))]; - assert_err!(cache.insert_verified_unsync(new_modules.into_iter())); + assert_err!(cache.insert_verified(new_modules.into_iter())); } #[test] - fn test_cache_insert_verified_unchecked_overrides_invalid_modules() { + fn test_cache_insert_verified_inserts_overridden_modules() { let mut cache = GlobalModuleCache::empty(); cache.insert(0, mock_verified_code(0, MockExtension::new(8))); - cache.mark_invalid_if_contains(&0); + cache.mark_overridden(&0); assert_eq!(cache.num_modules(), 1); assert_eq!(cache.size_in_bytes(), 8); let new_modules = vec![(0, mock_verified_code(100, MockExtension::new(32)))]; - assert_ok!(cache.insert_verified_unsync(new_modules.into_iter())); + assert_ok!(cache.insert_verified(new_modules.into_iter())); assert_eq!(cache.num_modules(), 1); assert_eq!(cache.size_in_bytes(), 32); diff --git a/aptos-move/block-executor/src/code_cache_global_manager.rs b/aptos-move/block-executor/src/code_cache_global_manager.rs index 084e7134c8c2d..5daaf4c0b7e90 100644 --- a/aptos-move/block-executor/src/code_cache_global_manager.rs +++ b/aptos-move/block-executor/src/code_cache_global_manager.rs @@ -10,7 +10,8 @@ use crate::{ }; use aptos_types::{ block_executor::{ - config::BlockExecutorModuleCacheLocalConfig, execution_state::TransactionSliceMetadata, + config::BlockExecutorModuleCacheLocalConfig, + transaction_slice_metadata::TransactionSliceMetadata, }, error::PanicError, state_store::StateView, @@ -298,7 +299,7 @@ fn prefetch_aptos_framework( // Framework must have been loaded. Drain verified modules from local cache into // global cache. let verified_module_code_iter = code_storage.into_verified_module_code_iter()?; - module_cache.insert_verified_unsync(verified_module_code_iter)?; + module_cache.insert_verified(verified_module_code_iter)?; } Ok(()) } diff --git a/aptos-move/block-executor/src/counters.rs b/aptos-move/block-executor/src/counters.rs index e75d1b3354805..79e98a7af3c53 100644 --- a/aptos-move/block-executor/src/counters.rs +++ b/aptos-move/block-executor/src/counters.rs @@ -198,7 +198,7 @@ pub static EFFECTIVE_BLOCK_GAS: Lazy = Lazy::new(|| { pub static APPROX_BLOCK_OUTPUT_SIZE: Lazy = Lazy::new(|| { register_histogram_vec!( "aptos_execution_approx_block_output_size", - "Historgram for different approx block output sizes - used for evaluting block ouptut limit.", + "Histogram for different approx block output sizes - used for evaluating block output limit.", &["mode"], output_buckets(), ) @@ -350,6 +350,17 @@ pub static GLOBAL_MODULE_CACHE_NUM_MODULES: Lazy = Lazy::new(|| { .unwrap() }); +pub static GLOBAL_MODULE_CACHE_MISS_SECONDS: Lazy = Lazy::new(|| { + register_histogram!( + // metric name + "global_module_cache_miss_seconds", + // metric description + "The time spent in seconds after global module cache miss to access per-block module cache", + time_buckets(), + ) + .unwrap() +}); + pub static STRUCT_NAME_INDEX_MAP_NUM_ENTRIES: Lazy = Lazy::new(|| { register_int_gauge!( "struct_name_index_map_num_entries", diff --git a/aptos-move/block-executor/src/executor.rs b/aptos-move/block-executor/src/executor.rs index 727153f0042b4..c0bd5ec56967e 100644 --- a/aptos-move/block-executor/src/executor.rs +++ b/aptos-move/block-executor/src/executor.rs @@ -1190,7 +1190,7 @@ where counters::update_state_counters(versioned_cache.stats(), true); module_cache_manager_guard .module_cache_mut() - .insert_verified_unsync(versioned_cache.take_modules_iter()) + .insert_verified(versioned_cache.take_modules_iter()) .map_err(|err| { alert!("[BlockSTM] Encountered panic error: {:?}", err); })?; @@ -1249,7 +1249,7 @@ where })?; let extension = Arc::new(AptosModuleExtension::new(state_value)); - global_module_cache.mark_invalid_if_contains(&id); + global_module_cache.mark_overridden(&id); per_block_module_cache .insert_deserialized_module(id.clone(), compiled_module, extension, Some(txn_idx)) .map_err(|err| { @@ -1670,7 +1670,7 @@ where counters::update_state_counters(unsync_map.stats(), false); module_cache_manager_guard .module_cache_mut() - .insert_verified_unsync(unsync_map.into_modules_iter())?; + .insert_verified(unsync_map.into_modules_iter())?; let block_end_info = if self .config diff --git a/aptos-move/e2e-tests/src/executor.rs b/aptos-move/e2e-tests/src/executor.rs index 9c9023c9c4b7d..cc145f190928d 100644 --- a/aptos-move/e2e-tests/src/executor.rs +++ b/aptos-move/e2e-tests/src/executor.rs @@ -34,7 +34,7 @@ use aptos_types::{ BlockExecutorConfig, BlockExecutorConfigFromOnchain, BlockExecutorLocalConfig, BlockExecutorModuleCacheLocalConfig, }, - execution_state::TransactionSliceMetadata, + transaction_slice_metadata::TransactionSliceMetadata, }, block_metadata::BlockMetadata, chain_id::ChainId, diff --git a/execution/executor-benchmark/src/native_executor.rs b/execution/executor-benchmark/src/native_executor.rs index 37f2253c685c2..69d4eba89ddca 100644 --- a/execution/executor-benchmark/src/native_executor.rs +++ b/execution/executor-benchmark/src/native_executor.rs @@ -11,8 +11,8 @@ use aptos_types::{ account_address::AccountAddress, account_config::{DepositEvent, WithdrawEvent}, block_executor::{ - config::BlockExecutorConfigFromOnchain, execution_state::TransactionSliceMetadata, - partitioner::PartitionedTransactions, + config::BlockExecutorConfigFromOnchain, partitioner::PartitionedTransactions, + transaction_slice_metadata::TransactionSliceMetadata, }, contract_event::ContractEvent, event::EventKey, diff --git a/execution/executor/src/block_executor/mod.rs b/execution/executor/src/block_executor/mod.rs index 352f42d93e643..f60548a94029a 100644 --- a/execution/executor/src/block_executor/mod.rs +++ b/execution/executor/src/block_executor/mod.rs @@ -31,8 +31,8 @@ use aptos_storage_interface::{ }; use aptos_types::{ block_executor::{ - config::BlockExecutorConfigFromOnchain, execution_state::TransactionSliceMetadata, - partitioner::ExecutableBlock, + config::BlockExecutorConfigFromOnchain, partitioner::ExecutableBlock, + transaction_slice_metadata::TransactionSliceMetadata, }, ledger_info::LedgerInfoWithSignatures, state_store::StateViewId, diff --git a/execution/executor/src/chunk_executor/mod.rs b/execution/executor/src/chunk_executor/mod.rs index b094eb7e5b5c8..7ae0c364a3699 100644 --- a/execution/executor/src/chunk_executor/mod.rs +++ b/execution/executor/src/chunk_executor/mod.rs @@ -29,7 +29,8 @@ use aptos_storage_interface::{ }; use aptos_types::{ block_executor::{ - config::BlockExecutorConfigFromOnchain, execution_state::TransactionSliceMetadata, + config::BlockExecutorConfigFromOnchain, + transaction_slice_metadata::TransactionSliceMetadata, }, contract_event::ContractEvent, ledger_info::LedgerInfoWithSignatures, diff --git a/execution/executor/src/chunk_executor/transaction_chunk.rs b/execution/executor/src/chunk_executor/transaction_chunk.rs index 974203e2a7f0e..89a3e39ffb285 100644 --- a/execution/executor/src/chunk_executor/transaction_chunk.rs +++ b/execution/executor/src/chunk_executor/transaction_chunk.rs @@ -12,7 +12,8 @@ use aptos_metrics_core::TimerHelper; use aptos_storage_interface::cached_state_view::CachedStateView; use aptos_types::{ block_executor::{ - config::BlockExecutorConfigFromOnchain, execution_state::TransactionSliceMetadata, + config::BlockExecutorConfigFromOnchain, + transaction_slice_metadata::TransactionSliceMetadata, }, transaction::{Transaction, TransactionOutput, Version}, }; diff --git a/execution/executor/src/db_bootstrapper/mod.rs b/execution/executor/src/db_bootstrapper/mod.rs index 28118993efcad..db0f85c28cb14 100644 --- a/execution/executor/src/db_bootstrapper/mod.rs +++ b/execution/executor/src/db_bootstrapper/mod.rs @@ -19,7 +19,8 @@ use aptos_types::{ account_config::CORE_CODE_ADDRESS, aggregate_signature::AggregateSignature, block_executor::{ - config::BlockExecutorConfigFromOnchain, execution_state::TransactionSliceMetadata, + config::BlockExecutorConfigFromOnchain, + transaction_slice_metadata::TransactionSliceMetadata, }, block_info::{BlockInfo, GENESIS_EPOCH, GENESIS_ROUND, GENESIS_TIMESTAMP_USECS}, ledger_info::{LedgerInfo, LedgerInfoWithSignatures}, diff --git a/execution/executor/src/fuzzing.rs b/execution/executor/src/fuzzing.rs index 17d415fa9918d..e820753c92199 100644 --- a/execution/executor/src/fuzzing.rs +++ b/execution/executor/src/fuzzing.rs @@ -10,8 +10,8 @@ use aptos_executor_types::BlockExecutorTrait; use aptos_storage_interface::{chunk_to_commit::ChunkToCommit, DbReader, DbReaderWriter, DbWriter}; use aptos_types::{ block_executor::{ - config::BlockExecutorConfigFromOnchain, execution_state::TransactionSliceMetadata, - partitioner::PartitionedTransactions, + config::BlockExecutorConfigFromOnchain, partitioner::PartitionedTransactions, + transaction_slice_metadata::TransactionSliceMetadata, }, ledger_info::LedgerInfoWithSignatures, state_store::StateView, diff --git a/execution/executor/src/tests/mock_vm/mod.rs b/execution/executor/src/tests/mock_vm/mod.rs index b1ab87984c7a4..42a62de226d2e 100644 --- a/execution/executor/src/tests/mock_vm/mod.rs +++ b/execution/executor/src/tests/mock_vm/mod.rs @@ -12,8 +12,8 @@ use aptos_types::{ account_address::AccountAddress, account_config::NEW_EPOCH_EVENT_V2_MOVE_TYPE_TAG, block_executor::{ - config::BlockExecutorConfigFromOnchain, execution_state::TransactionSliceMetadata, - partitioner::PartitionedTransactions, + config::BlockExecutorConfigFromOnchain, partitioner::PartitionedTransactions, + transaction_slice_metadata::TransactionSliceMetadata, }, bytes::NumToBytes, chain_id::ChainId, diff --git a/execution/executor/src/tests/mod.rs b/execution/executor/src/tests/mod.rs index 17b953f6d459a..163cbfda435a7 100644 --- a/execution/executor/src/tests/mod.rs +++ b/execution/executor/src/tests/mod.rs @@ -19,7 +19,8 @@ use aptos_types::{ account_address::AccountAddress, aggregate_signature::AggregateSignature, block_executor::{ - config::BlockExecutorConfigFromOnchain, execution_state::TransactionSliceMetadata, + config::BlockExecutorConfigFromOnchain, + transaction_slice_metadata::TransactionSliceMetadata, }, block_info::BlockInfo, bytes::NumToBytes, diff --git a/execution/executor/src/workflow/do_get_execution_output.rs b/execution/executor/src/workflow/do_get_execution_output.rs index 2a7d45795a6ca..4e5bb2312d331 100644 --- a/execution/executor/src/workflow/do_get_execution_output.rs +++ b/execution/executor/src/workflow/do_get_execution_output.rs @@ -27,8 +27,8 @@ use aptos_types::transaction::ExecutionStatus; use aptos_types::{ block_executor::{ config::BlockExecutorConfigFromOnchain, - execution_state::TransactionSliceMetadata, partitioner::{ExecutableTransactions, PartitionedTransactions}, + transaction_slice_metadata::TransactionSliceMetadata, }, contract_event::ContractEvent, epoch_state::EpochState, diff --git a/experimental/execution/ptx-executor/src/lib.rs b/experimental/execution/ptx-executor/src/lib.rs index 43a6c9ba24683..3f6bbd5bea8ed 100644 --- a/experimental/execution/ptx-executor/src/lib.rs +++ b/experimental/execution/ptx-executor/src/lib.rs @@ -27,8 +27,8 @@ use aptos_infallible::Mutex; use aptos_metrics_core::TimerHelper; use aptos_types::{ block_executor::{ - config::BlockExecutorConfigFromOnchain, execution_state::TransactionSliceMetadata, - partitioner::PartitionedTransactions, + config::BlockExecutorConfigFromOnchain, partitioner::PartitionedTransactions, + transaction_slice_metadata::TransactionSliceMetadata, }, state_store::StateView, transaction::{ diff --git a/third_party/move/move-vm/runtime/src/loader/mod.rs b/third_party/move/move-vm/runtime/src/loader/mod.rs index 171175aa868cf..4b74cc881dac6 100644 --- a/third_party/move/move-vm/runtime/src/loader/mod.rs +++ b/third_party/move/move-vm/runtime/src/loader/mod.rs @@ -258,6 +258,7 @@ impl Loader { I: IntoIterator, I::IntoIter: DoubleEndedIterator, { + let _timer = VM_TIMER.timer_with_label("Loader::check_dependencies_and_charge_gas"); match self { Self::V1(loader) => loader.check_dependencies_and_charge_gas( module_store, @@ -526,7 +527,7 @@ impl LoaderV1 { data_store: &mut TransactionDataCache, module_store: &LegacyModuleStorageAdapter, ) -> VMResult> { - let _timer = VM_TIMER.timer_with_label("Loader::deserialize_and_verify_script"); + let _timer = VM_TIMER.timer_with_label("LoaderV1::deserialize_and_verify_script"); let script = data_store.load_compiled_script_to_cache(script, hash_value)?; @@ -847,8 +848,6 @@ impl LoaderV1 { I: IntoIterator, I::IntoIter: DoubleEndedIterator, { - let _timer = VM_TIMER.timer_with_label("Loader::check_dependencies_and_charge_gas"); - // Initialize the work list (stack) and the map of visited modules. // // TODO: Determine the reserved capacity based on the max number of dependencies allowed. @@ -918,7 +917,7 @@ impl LoaderV1 { return Ok(cached); } - let _timer = VM_TIMER.timer_with_label("Loader::load_module [cache miss]"); + let _timer = VM_TIMER.timer_with_label("LoaderV1::load_module [cache miss]"); // otherwise, load the transitive closure of the target module let module_ref = self.load_and_verify_module_and_dependencies_and_friends( @@ -962,7 +961,7 @@ impl LoaderV1 { // Verify the module if it hasn't been verified before. if VERIFIED_MODULES.lock().get(&hash_value).is_none() { let _timer = VM_TIMER - .timer_with_label("Loader::load_and_verify_module [verification cache miss]"); + .timer_with_label("LoaderV1::load_and_verify_module [verification cache miss]"); move_bytecode_verifier::verify_module_with_config( &self.vm_config.verifier_config, diff --git a/third_party/move/move-vm/runtime/src/storage/environment.rs b/third_party/move/move-vm/runtime/src/storage/environment.rs index cf51100d91da8..5f53db11161e9 100644 --- a/third_party/move/move-vm/runtime/src/storage/environment.rs +++ b/third_party/move/move-vm/runtime/src/storage/environment.rs @@ -25,6 +25,7 @@ use move_core_types::{ identifier::{IdentStr, Identifier}, vm_status::{sub_status::unknown_invariant_violation::EPARANOID_FAILURE, StatusCode}, }; +use move_vm_metrics::{Timer, VM_TIMER}; #[cfg(any(test, feature = "testing"))] use move_vm_types::loaded_data::runtime_types::{StructIdentifier, StructNameIndex}; use std::sync::Arc; @@ -152,6 +153,10 @@ impl RuntimeEnvironment { module_hash: &[u8; 32], ) -> VMResult { if !VERIFIED_MODULES_V2.contains(module_hash) { + let _timer = VM_TIMER.timer_with_label( + "LoaderV2::build_locally_verified_module [verification cache miss]", + ); + // For regular execution, we cache already verified modules. Note that this even caches // verification for the published modules. This should be ok because as long as the // hash is the same, the deployed bytecode and any dependencies are the same, and so diff --git a/third_party/move/move-vm/runtime/src/storage/module_storage.rs b/third_party/move/move-vm/runtime/src/storage/module_storage.rs index 714871a70cc77..da64fc6dbd001 100644 --- a/third_party/move/move-vm/runtime/src/storage/module_storage.rs +++ b/third_party/move/move-vm/runtime/src/storage/module_storage.rs @@ -10,6 +10,7 @@ use move_core_types::{ account_address::AccountAddress, identifier::IdentStr, language_storage::ModuleId, metadata::Metadata, }; +use move_vm_metrics::{Timer, VM_TIMER}; use move_vm_types::{ code::{ModuleCache, ModuleCode, ModuleCodeBuilder, WithBytes, WithHash, WithSize}, module_cyclic_dependency_error, module_linker_error, @@ -191,6 +192,8 @@ where return Ok(Some(module.code().verified().clone())); } + let _timer = VM_TIMER.timer_with_label("ModuleStorage::fetch_verified_module [cache miss]"); + let mut visited = HashSet::new(); visited.insert(id.clone()); Ok(Some(visit_dependencies_and_verify( diff --git a/types/Cargo.toml b/types/Cargo.toml index 088b34699ac06..26e8db6d58771 100644 --- a/types/Cargo.toml +++ b/types/Cargo.toml @@ -94,7 +94,7 @@ url = { workspace = true } [features] default = [] -testing = [] +testing = ["aptos-crypto/fuzzing"] fuzzing = ["proptest", "proptest-derive", "aptos-crypto/fuzzing", "move-core-types/fuzzing", "arbitrary"] [[bench]] diff --git a/types/src/block_executor/execution_state.rs b/types/src/block_executor/execution_state.rs deleted file mode 100644 index a06aa8391cab2..0000000000000 --- a/types/src/block_executor/execution_state.rs +++ /dev/null @@ -1,107 +0,0 @@ -// Copyright © Aptos Foundation -// SPDX-License-Identifier: Apache-2.0 - -use crate::transaction::Version; -use aptos_crypto::HashValue; - -/// Specifies the kind of transactions for the block executor. -#[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub enum TransactionSliceMetadata { - /// Block execution. Specifies the parent (executed) block, and the child (to be executed) - /// block. - Block { parent: HashValue, child: HashValue }, - /// Chunk execution, e.g., state sync or replay. Specifies the start and the end versions of - /// transaction slice. - Chunk { begin: Version, end: Version }, - /// The origin of transactions is not known, e.g., running a test. - Unknown, -} - -impl TransactionSliceMetadata { - pub fn unknown() -> Self { - Self::Unknown - } - - pub fn block(parent: HashValue, child: HashValue) -> Self { - Self::Block { parent, child } - } - - #[cfg(any(test, feature = "testing"))] - pub fn block_from_u64(parent: u64, child: u64) -> Self { - Self::Block { - parent: HashValue::from_u64(parent), - child: HashValue::from_u64(child), - } - } - - pub fn chunk(begin: Version, end: Version) -> Self { - Self::Chunk { begin, end } - } - - /// Returns the hash of the block where to append the state checkpoint (i.e., the current hash - /// of [TransactionSliceMetadata::Block]). For other variants, returns [None]. - pub fn append_state_checkpoint_to_block(&self) -> Option { - use TransactionSliceMetadata::*; - - match self { - Unknown => None, - Block { child, .. } => Some(*child), - Chunk { .. } => None, - } - } - - /// Returns true if transaction slice immediately follows the previous one. That is, if: - /// 1. Both are [TransactionSliceMetadata::Block] and the previous child is equal to the - /// current parent. - /// 2. Both are [TransactionSliceMetadata::Chunk] and the previous end version is equal to - /// the current start version. - pub fn is_immediately_after(&self, previous: &TransactionSliceMetadata) -> bool { - use TransactionSliceMetadata::*; - - match (previous, self) { - (Unknown, Unknown) - | (Unknown, Block { .. }) - | (Unknown, Chunk { .. }) - | (Block { .. }, Unknown) - | (Block { .. }, Chunk { .. }) - | (Chunk { .. }, Unknown) - | (Chunk { .. }, Block { .. }) => false, - (Block { child, .. }, Block { parent, .. }) => parent == child, - (Chunk { end, .. }, Chunk { begin, .. }) => begin == end, - } - } -} - -#[cfg(test)] -mod test { - use super::*; - - #[test] - fn test_append_state_checkpoint_to_block() { - assert!(TransactionSliceMetadata::unknown() - .append_state_checkpoint_to_block() - .is_none()); - assert!(TransactionSliceMetadata::chunk(1, 2) - .append_state_checkpoint_to_block() - .is_none()); - - let parent = HashValue::from_u64(2); - let child = HashValue::from_u64(3); - let execution_state = TransactionSliceMetadata::block(parent, child); - assert_eq!( - execution_state.append_state_checkpoint_to_block(), - Some(child) - ); - } - - #[test] - fn test_is_immediately_after() { - let fst = TransactionSliceMetadata::block(HashValue::from_u64(2), HashValue::from_u64(3)); - let snd = TransactionSliceMetadata::block(HashValue::from_u64(3), HashValue::from_u64(4)); - assert!(snd.is_immediately_after(&fst)); - - let fst = TransactionSliceMetadata::block(HashValue::from_u64(2), HashValue::from_u64(3)); - let snd = TransactionSliceMetadata::block(HashValue::from_u64(4), HashValue::from_u64(5)); - assert!(!snd.is_immediately_after(&fst)); - } -} diff --git a/types/src/block_executor/mod.rs b/types/src/block_executor/mod.rs index fb0081dacd7ca..a75beececb574 100644 --- a/types/src/block_executor/mod.rs +++ b/types/src/block_executor/mod.rs @@ -3,5 +3,5 @@ // SPDX-License-Identifier: Apache-2.0 pub mod config; -pub mod execution_state; pub mod partitioner; +pub mod transaction_slice_metadata; diff --git a/types/src/block_executor/transaction_slice_metadata.rs b/types/src/block_executor/transaction_slice_metadata.rs new file mode 100644 index 0000000000000..8df707552253f --- /dev/null +++ b/types/src/block_executor/transaction_slice_metadata.rs @@ -0,0 +1,169 @@ +// Copyright © Aptos Foundation +// SPDX-License-Identifier: Apache-2.0 + +use crate::transaction::Version; +use aptos_crypto::HashValue; + +/// Specifies the kind of transactions for the block executor. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum TransactionSliceMetadata { + /// Block execution. Specifies the parent (executed) block, and the child (to be executed) + /// block. + Block { parent: HashValue, child: HashValue }, + /// Chunk execution, e.g., state sync or replay. Specifies the start (inclusive) and the end + /// (exclusive) versions of a transaction slice. + Chunk { begin: Version, end: Version }, + /// The origin of transactions is not known, e.g., running a test. + Unknown, +} + +impl TransactionSliceMetadata { + pub fn unknown() -> Self { + Self::Unknown + } + + pub fn block(parent: HashValue, child: HashValue) -> Self { + Self::Block { parent, child } + } + + #[cfg(any(test, feature = "testing"))] + pub fn block_from_u64(parent: u64, child: u64) -> Self { + Self::Block { + parent: HashValue::from_u64(parent), + child: HashValue::from_u64(child), + } + } + + pub fn chunk(begin: Version, end: Version) -> Self { + debug_assert!( + begin < end, + "Chunk must have non-negative size, but it has: {}-{}", + begin, + end + ); + Self::Chunk { begin, end } + } + + /// Returns the hash of the block where to append the state checkpoint (i.e., the current hash + /// of [TransactionSliceMetadata::Block]). For other variants, returns [None]. + pub fn append_state_checkpoint_to_block(&self) -> Option { + use TransactionSliceMetadata::*; + + match self { + Unknown => None, + Block { child, .. } => Some(*child), + Chunk { .. } => None, + } + } + + /// Returns true if transaction slice immediately follows the previous one. That is, if: + /// 1. Both are [TransactionSliceMetadata::Block] and the previous child is equal to the + /// current parent. + /// 2. Both are [TransactionSliceMetadata::Chunk] and the previous end version is equal to + /// the current start version. + pub fn is_immediately_after(&self, previous: &TransactionSliceMetadata) -> bool { + use TransactionSliceMetadata::*; + + match (previous, self) { + (Unknown, Unknown) + | (Unknown, Block { .. }) + | (Unknown, Chunk { .. }) + | (Block { .. }, Unknown) + | (Block { .. }, Chunk { .. }) + | (Chunk { .. }, Unknown) + | (Chunk { .. }, Block { .. }) => false, + (Block { child, .. }, Block { parent, .. }) => parent == child, + (Chunk { end, .. }, Chunk { begin, .. }) => begin == end, + } + } +} + +#[cfg(test)] +mod test { + use super::*; + use claims::assert_none; + + #[test] + fn test_append_state_checkpoint_to_block() { + assert_none!(TransactionSliceMetadata::unknown().append_state_checkpoint_to_block()); + assert_none!(TransactionSliceMetadata::chunk(1, 2).append_state_checkpoint_to_block()); + + let metadata = TransactionSliceMetadata::block_from_u64(2, 3); + assert_eq!( + metadata.append_state_checkpoint_to_block(), + Some(HashValue::from_u64(3)) + ); + } + + #[test] + fn test_is_immediately_after() { + let is_immediately_after = [ + ( + TransactionSliceMetadata::block_from_u64(2, 3), + TransactionSliceMetadata::block_from_u64(3, 4), + ), + ( + TransactionSliceMetadata::chunk(0, 1), + TransactionSliceMetadata::chunk(1, 2), + ), + ]; + + for (fst, snd) in is_immediately_after { + assert!(snd.is_immediately_after(&fst)); + } + } + + #[test] + fn test_is_not_immediately_after() { + let is_not_immediately_after = [ + ( + TransactionSliceMetadata::unknown(), + TransactionSliceMetadata::unknown(), + ), + ( + TransactionSliceMetadata::unknown(), + TransactionSliceMetadata::block_from_u64(3, 4), + ), + ( + TransactionSliceMetadata::unknown(), + TransactionSliceMetadata::chunk(3, 4), + ), + ( + TransactionSliceMetadata::block_from_u64(0, 1), + TransactionSliceMetadata::unknown(), + ), + ( + TransactionSliceMetadata::block_from_u64(1, 2), + TransactionSliceMetadata::block_from_u64(0, 1), + ), + ( + TransactionSliceMetadata::block_from_u64(1, 2), + TransactionSliceMetadata::block_from_u64(1, 2), + ), + ( + TransactionSliceMetadata::block_from_u64(0, 1), + TransactionSliceMetadata::chunk(2, 3), + ), + ( + TransactionSliceMetadata::chunk(0, 1), + TransactionSliceMetadata::unknown(), + ), + ( + TransactionSliceMetadata::chunk(0, 1), + TransactionSliceMetadata::block_from_u64(1, 2), + ), + ( + TransactionSliceMetadata::chunk(1, 2), + TransactionSliceMetadata::chunk(0, 1), + ), + ( + TransactionSliceMetadata::chunk(1, 2), + TransactionSliceMetadata::chunk(1, 2), + ), + ]; + + for (fst, snd) in is_not_immediately_after { + assert!(!snd.is_immediately_after(&fst)); + } + } +} diff --git a/types/src/transaction/webauthn.rs b/types/src/transaction/webauthn.rs index 84d76de586d19..cebdd2cec6262 100644 --- a/types/src/transaction/webauthn.rs +++ b/types/src/transaction/webauthn.rs @@ -57,7 +57,7 @@ pub enum AssertionSignature { /// Custom arbitrary implementation for fuzzing /// as the `secp256r1_ecdsa::Signature` type is an external dependency /// p256::ecdsa::Signature -#[cfg(any(test, feature = "fuzzing"))] +#[cfg(feature = "fuzzing")] impl<'a> arbitrary::Arbitrary<'a> for AssertionSignature { fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { // Generate a fixed-length byte array for the signature