From 1dec2c1315555c298fd5ded41e6cc8183cbed077 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Wed, 4 Sep 2024 15:43:20 +0100 Subject: [PATCH] [fix] Fix a bug where non-veerified entry in captured read cache was causing troubles --- aptos-move/aptos-vm/src/errors.rs | 3 +++ aptos-move/block-executor/src/captured_reads.rs | 5 ++--- aptos-move/block-executor/src/executor.rs | 2 +- .../block-executor/src/modules_and_scripts.rs | 17 ++++++++++++++--- .../e2e-move-tests/src/tests/code_publishing.rs | 2 +- aptos-move/mvhashmap/src/unsync_map.rs | 6 ++---- .../mvhashmap/src/versioned_module_storage.rs | 12 ++++++------ types/src/vm/modules.rs | 6 +++--- 8 files changed, 32 insertions(+), 21 deletions(-) diff --git a/aptos-move/aptos-vm/src/errors.rs b/aptos-move/aptos-vm/src/errors.rs index f054a7c819222b..ada6b313890efb 100644 --- a/aptos-move/aptos-vm/src/errors.rs +++ b/aptos-move/aptos-vm/src/errors.rs @@ -138,6 +138,7 @@ pub fn convert_prologue_error( let err_msg = format!("[aptos_vm] Unexpected prologue Move abort: {:?}::{:?} (Category: {:?} Reason: {:?})", location, code, category, reason); speculative_error!(log_context, err_msg.clone()); + println!("B: {:?}", err_msg); return Err(VMStatus::Error { status_code: StatusCode::UNEXPECTED_ERROR_FROM_KNOWN_MOVE_FUNCTION, sub_status: None, @@ -159,6 +160,7 @@ pub fn convert_prologue_error( log_context, format!("[aptos_vm] Unexpected prologue error: {:?}", status), ); + println!("A: {:?}", status); VMStatus::Error { status_code: StatusCode::UNEXPECTED_ERROR_FROM_KNOWN_MOVE_FUNCTION, sub_status: status.sub_status(), @@ -213,6 +215,7 @@ pub fn convert_epilogue_error( status => { let err_msg = format!("[aptos_vm] Unexpected success epilogue error: {:?}", status); speculative_error!(log_context, err_msg.clone()); + println!("C: {:?}", err_msg); VMStatus::Error { status_code: StatusCode::UNEXPECTED_ERROR_FROM_KNOWN_MOVE_FUNCTION, sub_status: status.sub_status(), diff --git a/aptos-move/block-executor/src/captured_reads.rs b/aptos-move/block-executor/src/captured_reads.rs index 3f91d42b888040..8f24edd4d33757 100644 --- a/aptos-move/block-executor/src/captured_reads.rs +++ b/aptos-move/block-executor/src/captured_reads.rs @@ -25,7 +25,6 @@ use aptos_types::{ transaction::BlockExecutableTransaction as Transaction, write_set::TransactionWrite, }; use aptos_vm_types::resolver::ResourceGroupSize; -use claims::assert_none; use derivative::Derivative; use move_core_types::value::MoveTypeLayout; use std::{ @@ -488,8 +487,8 @@ impl CapturedReads { /// Captures the read of a module storage entry. pub(crate) fn capture_module_storage_read(&mut self, key: T::Key, read: ModuleStorageRead) { - let prev = self.module_storage_reads.insert(key, read); - assert_none!(prev); + let _prev = self.module_storage_reads.insert(key, read); + // assert!(prev.is_none() || prev.is_some_and(|e| e.)); } pub(crate) fn capture_delayed_field_read( diff --git a/aptos-move/block-executor/src/executor.rs b/aptos-move/block-executor/src/executor.rs index 003c17064f2e6c..62a521aa1f7ea2 100644 --- a/aptos-move/block-executor/src/executor.rs +++ b/aptos-move/block-executor/src/executor.rs @@ -1081,7 +1081,7 @@ where if runtime_environment.vm_config().use_loader_v2 { let entry = ModuleStorageEntry::from_transaction_write(runtime_environment, write_op)?; - unsync_map.write_module_storage_entry(key, entry); + unsync_map.write_module_storage_entry(key, Arc::new(entry)); } else { unsync_map.write_module(key, write_op); } diff --git a/aptos-move/block-executor/src/modules_and_scripts.rs b/aptos-move/block-executor/src/modules_and_scripts.rs index fbb6df80cbf375..1bf93eb59e1432 100644 --- a/aptos-move/block-executor/src/modules_and_scripts.rs +++ b/aptos-move/block-executor/src/modules_and_scripts.rs @@ -83,10 +83,11 @@ impl<'a, T: Transaction, S: TStateView, X: Executable> ModuleStora address: &AccountAddress, module_name: &IdentStr, ) -> VMResult { - Ok(self + let exists = self .read_module_storage(address, module_name)? .into_versioned() - .is_some()) + .is_some(); + Ok(exists) } fn fetch_module_bytes( @@ -182,6 +183,7 @@ impl<'a, T: Transaction, S: TStateView, X: Executable> LatestView< .get_or_else(key, self.txn_idx, || { self.get_base_module_storage_entry(key) })?; + state .captured_reads .borrow_mut() @@ -272,7 +274,9 @@ impl<'a, T: Transaction, S: TStateView, X: Executable> LatestView< // there must be a cycle. // Note: here we treat "verified" modules as graph nodes that exited the recursion, // which allows us to identify cycles. + assert!(!dep_entry.is_verified()); let dep_key = T::Key::from_address_and_module_name(addr, name); + if visited.insert(dep_key.clone()) { let module = self.traversed_published_dependencies(addr, name, visited)?; verified_dependencies.push(module); @@ -287,13 +291,20 @@ impl<'a, T: Transaction, S: TStateView, X: Executable> LatestView< self.runtime_environment .build_verified_module(partially_verified_module, &verified_dependencies)?, ); - let verified_entry = entry.make_verified(module.clone()); + let verified_entry = Arc::new(entry.make_verified(module.clone())); // Finally, change the entry in the module storage to the verified one, in order to // make sure that everyone sees the verified module. let key = T::Key::from_address_and_module_name(address, module_name); match &self.latest_view { ViewState::Sync(state) => { + state + .captured_reads + .borrow_mut() + .capture_module_storage_read( + key.clone(), + ModuleStorageRead::Versioned(version.clone(), verified_entry.clone()), + ); state.versioned_map.module_storage().write_if_not_verified( &key, version, diff --git a/aptos-move/e2e-move-tests/src/tests/code_publishing.rs b/aptos-move/e2e-move-tests/src/tests/code_publishing.rs index e0f4b80f08abe1..6b4f66066eb01a 100644 --- a/aptos-move/e2e-move-tests/src/tests/code_publishing.rs +++ b/aptos-move/e2e-move-tests/src/tests/code_publishing.rs @@ -442,7 +442,7 @@ fn test_module_publishing_does_not_fallback() { MemberId::from_str(&format!("{}::{}::{}", addr, module_name, function_name)).unwrap(); let mut txns = vec![]; - let mut expected_abort_codes = vec![]; + let mut expected_abort_codes: Vec> = vec![]; // Generate a simple test workload. for abort_code in [1, 2, 3, 4, 5, 1] { diff --git a/aptos-move/mvhashmap/src/unsync_map.rs b/aptos-move/mvhashmap/src/unsync_map.rs index c3885d5c4bb12e..a3cd61ffa5043d 100644 --- a/aptos-move/mvhashmap/src/unsync_map.rs +++ b/aptos-move/mvhashmap/src/unsync_map.rs @@ -265,10 +265,8 @@ impl< .insert(key, (Arc::new(value), None)); } - pub fn write_module_storage_entry(&self, key: K, entry: ModuleStorageEntry) { - self.module_storage - .borrow_mut() - .insert(key, Arc::new(entry)); + pub fn write_module_storage_entry(&self, key: K, entry: Arc) { + self.module_storage.borrow_mut().insert(key, entry); } pub fn set_base_value(&self, key: K, value: ValueWithLayout) { diff --git a/aptos-move/mvhashmap/src/versioned_module_storage.rs b/aptos-move/mvhashmap/src/versioned_module_storage.rs index 473edfe12fbf27..87c5b5f24fc4ad 100644 --- a/aptos-move/mvhashmap/src/versioned_module_storage.rs +++ b/aptos-move/mvhashmap/src/versioned_module_storage.rs @@ -14,8 +14,8 @@ use std::{collections::BTreeMap, fmt::Debug, hash::Hash, sync::Arc}; pub type ModuleVersion = Result; /// Result of a read query on the versioned module storage. -#[derive(Derivative)] -#[derivative(Clone(bound = ""), Debug(bound = ""), PartialEq(bound = ""))] +#[derive(Clone, Derivative)] +#[derivative(Debug, PartialEq)] pub enum ModuleStorageRead { /// An existing module at certain index of committed transaction or from the base storage. Versioned( @@ -128,7 +128,7 @@ impl VersionedModuleStorage { return Ok(result); } - // Otherwise, use the passed closure to compute thw base storage value. + // Otherwise, use the passed closure to compute the base storage value. let maybe_entry = init_func()?; let result = Ok(match &maybe_entry { Some(entry) => ModuleStorageRead::storage_version(entry.clone()), @@ -158,7 +158,7 @@ impl VersionedModuleStorage { pub fn write_pending(&self, key: K, txn_idx: TxnIndex) { let mut v = self .entries - .entry(key) + .entry(key.clone()) .or_insert_with(VersionedEntry::empty); v.versions .insert(ShiftedTxnIndex::new(txn_idx), CachePadded::new(None)); @@ -186,7 +186,7 @@ impl VersionedModuleStorage { &self, key: &K, version: ModuleVersion, - entry: ModuleStorageEntry, + entry: Arc, ) { let mut versioned_entry = self .entries @@ -205,7 +205,7 @@ impl VersionedModuleStorage { if !prev_entry.is_verified() { versioned_entry .versions - .insert(committed_idx, CachePadded::new(Some(Arc::new(entry)))); + .insert(committed_idx, CachePadded::new(Some(entry))); } } } diff --git a/types/src/vm/modules.rs b/types/src/vm/modules.rs index 97375b2be468e1..8795dca4ff4d24 100644 --- a/types/src/vm/modules.rs +++ b/types/src/vm/modules.rs @@ -28,9 +28,9 @@ enum Representation { Verified(Arc), } -/// An entry for Aptos code cache, capable of resolving different request including -/// its bytes, metadata, etc. Note that it is the responsibility of the code cache -/// to ensure the data is consistent with the latest on-chain configs. +/// An entry for Aptos code cache, capable of resolving different requests including +/// bytes, metadata, etc. Note that it is the responsibility of the code cache to +/// ensure the data is consistent with the latest on-chain configs. #[derive(Debug)] pub struct ModuleStorageEntry { /// Serialized representation of the module.