From e730a3c8d0d41eb42047c3da12dc31283cc880d4 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Fri, 6 Sep 2024 10:41:55 +0100 Subject: [PATCH] address comments from Igor & fix rebase --- aptos-move/aptos-vm/src/errors.rs | 3 --- aptos-move/block-executor/src/captured_reads.rs | 11 +++++++---- aptos-move/block-executor/src/executor.rs | 1 + .../integration-tests/src/tests/bad_storage_tests.rs | 8 -------- .../storage/implementations/unsync_code_storage.rs | 1 - .../move-vm/runtime/src/storage/module_storage.rs | 10 ---------- 6 files changed, 8 insertions(+), 26 deletions(-) diff --git a/aptos-move/aptos-vm/src/errors.rs b/aptos-move/aptos-vm/src/errors.rs index ada6b313890ef..f054a7c819222 100644 --- a/aptos-move/aptos-vm/src/errors.rs +++ b/aptos-move/aptos-vm/src/errors.rs @@ -138,7 +138,6 @@ 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, @@ -160,7 +159,6 @@ 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(), @@ -215,7 +213,6 @@ 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 bc65c6a8b353c..fd229b20b74ff 100644 --- a/aptos-move/block-executor/src/captured_reads.rs +++ b/aptos-move/block-executor/src/captured_reads.rs @@ -620,8 +620,8 @@ impl CapturedReads { // and fetching it returned an error, the error is not recorded. Hence, it is safe here // to simply treat such errors as a non-existent value. self.module_storage_reads.iter().all(|(k, previous_read)| { - let new_read = module_storage.get(k, idx_to_validate); - previous_read == &new_read + let current_read = module_storage.get(k, idx_to_validate); + previous_read == ¤t_read }) } @@ -736,10 +736,13 @@ impl CapturedReads { } } - // TODO(loader_v2): We have a new set of reads now. We probably need to feature gate here as well? + // TODO(loader_v2): Test summaries are the same. for key in &self.module_reads { ret.insert(InputOutputKey::Resource(key.clone())); } + for key in self.module_storage_reads.keys() { + ret.insert(InputOutputKey::Resource(key.clone())); + } for (key, read) in &self.delayed_field_reads { if let DelayedFieldRead::Value { .. } = read { @@ -784,7 +787,7 @@ impl UnsyncReadSet { } } - // TODO(loader_v2): Should we be using a feature flag here? + // TODO(loader_v2): Test summaries are the same if we switch. for key in &self.module_reads { ret.insert(InputOutputKey::Resource(key.clone())); } diff --git a/aptos-move/block-executor/src/executor.rs b/aptos-move/block-executor/src/executor.rs index 62a521aa1f7ea..9f2413809600f 100644 --- a/aptos-move/block-executor/src/executor.rs +++ b/aptos-move/block-executor/src/executor.rs @@ -542,6 +542,7 @@ where ), )?; + // TODO(loader_v2): Remove duplication here. // Make sure we publish modules here before we wake up subsequent dependent // transactions and decrease the validation index. if runtime_environment.vm_config().use_loader_v2 { diff --git a/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs b/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs index d0e0338a29288..316f02b99f304 100644 --- a/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs +++ b/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs @@ -635,14 +635,6 @@ impl ModuleStorage for BogusModuleStorage { Err(PartialVMError::new(self.bad_status_code).finish(Location::Undefined)) } - fn fetch_module_bytes( - &self, - _address: &AccountAddress, - _module_name: &IdentStr, - ) -> PartialVMResult> { - Err(PartialVMError::new(self.bad_status_code)) - } - fn fetch_module_size_in_bytes( &self, _address: &AccountAddress, diff --git a/third_party/move/move-vm/runtime/src/storage/implementations/unsync_code_storage.rs b/third_party/move/move-vm/runtime/src/storage/implementations/unsync_code_storage.rs index 5a55b3d8161c7..ffc80d8fecec8 100644 --- a/third_party/move/move-vm/runtime/src/storage/implementations/unsync_code_storage.rs +++ b/third_party/move/move-vm/runtime/src/storage/implementations/unsync_code_storage.rs @@ -173,7 +173,6 @@ impl CodeStorage for UnsyncCodeStorag } } - #[cfg(test)] impl UnsyncCodeStorage { fn matches bool>( 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 e585761e67148..03d38ea81e4d4 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 @@ -74,13 +74,3 @@ pub trait ModuleBytesStorage { module_name: &IdentStr, ) -> VMResult>; } - -/// Storage that contains serialized modules. Clients can implement this trait -/// for their own backends, so that [ModuleStorage] can be built on top of it. -pub trait ModuleBytesStorage { - fn fetch_module_bytes( - &self, - address: &AccountAddress, - module_name: &IdentStr, - ) -> PartialVMResult>; -}