[fix] Fix a bug where non-veerified entry in captured read cache was …

…causing troubles

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(),

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<T: Transaction> CapturedReads<T> {
 
     /// 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(

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);
             }

aptos-move/block-executor/src/modules_and_scripts.rs

@@ -83,10 +83,11 @@ impl<'a, T: Transaction, S: TStateView<Key = T::Key>, X: Executable> ModuleStora
         address: &AccountAddress,
         module_name: &IdentStr,
     ) -> VMResult<bool> {
-        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<Key = T::Key>, 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<Key = T::Key>, 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<Key = T::Key>, 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,

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<Option<u64>> = vec![];
 
     // Generate a simple test workload.
     for abort_code in [1, 2, 3, 4, 5, 1] {

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<ModuleStorageEntry>) {
+        self.module_storage.borrow_mut().insert(key, entry);
     }
 
     pub fn set_base_value(&self, key: K, value: ValueWithLayout<V>) {

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<TxnIndex, StorageVersion>;
 
 /// 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<K: Debug + Hash + Clone + Eq + ModulePath> VersionedModuleStorage<K> {
             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<K: Debug + Hash + Clone + Eq + ModulePath> VersionedModuleStorage<K> {
     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<K: Debug + Hash + Clone + Eq + ModulePath> VersionedModuleStorage<K> {
         &self,
         key: &K,
         version: ModuleVersion,
-        entry: ModuleStorageEntry,
+        entry: Arc<ModuleStorageEntry>,
     ) {
         let mut versioned_entry = self
             .entries
@@ -205,7 +205,7 @@ impl<K: Debug + Hash + Clone + Eq + ModulePath> VersionedModuleStorage<K> {
         if !prev_entry.is_verified() {
             versioned_entry
                 .versions
-                .insert(committed_idx, CachePadded::new(Some(Arc::new(entry))));
+                .insert(committed_idx, CachePadded::new(Some(entry)));
         }
     }
 }

types/src/vm/modules.rs

@@ -28,9 +28,9 @@ enum Representation {
     Verified(Arc<Module>),
 }
 
-/// 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.