[move][vm][block-stm] Fix error types for resolvers (aptos-labs#11593)

Views/resolvers used by VM and Block-STM now also use `PartialVMError`
instead of `anyhow::Error`:
- `STORAGE_ERROR` is no longer speculative.
- Errors are propagated from where they originated, and are not ignored.

aptos-move/aptos-aggregator/src/delayed_field_extension.rs

@@ -21,6 +21,7 @@ fn get_delayed_field_value_from_storage(
 ) -> Result<DelayedFieldValue, PanicOr<DelayedFieldsSpeculativeError>> {
     resolver
         .get_delayed_field_value(id)
+        // TODO[agg_v2](fix): Is this error mapping correct?
         .map_err(|_err| PanicOr::Or(DelayedFieldsSpeculativeError::NotFound(*id)))
 }
 

aptos-move/aptos-aggregator/src/delta_change_set.rs

@@ -224,10 +224,8 @@ mod test {
         write_set::WriteOp,
     };
     use claims::{assert_err, assert_none, assert_ok, assert_ok_eq, assert_some_eq};
-    use move_core_types::{
-        value::MoveTypeLayout,
-        vm_status::{StatusCode, VMStatus},
-    };
+    use move_binary_format::errors::{PartialVMError, PartialVMResult};
+    use move_core_types::{value::MoveTypeLayout, vm_status::StatusCode};
     use once_cell::sync::Lazy;
     use std::{
         collections::{BTreeMap, HashSet},
@@ -499,11 +497,11 @@ mod test {
         fn get_aggregator_v1_state_value(
             &self,
             _id: &Self::Identifier,
-        ) -> anyhow::Result<Option<StateValue>> {
-            Err(anyhow::Error::new(VMStatus::error(
-                StatusCode::SPECULATIVE_EXECUTION_ABORT_ERROR,
-                Some("Error message from BadStorage.".to_string()),
-            )))
+        ) -> PartialVMResult<Option<StateValue>> {
+            Err(
+                PartialVMError::new(StatusCode::SPECULATIVE_EXECUTION_ABORT_ERROR)
+                    .with_message("Error message from BadStorage.".to_string()),
+            )
         }
     }
 

aptos-move/aptos-aggregator/src/resolver.rs

@@ -19,17 +19,11 @@ use aptos_types::{
     },
     write_set::WriteOp,
 };
-use move_binary_format::errors::{Location, PartialVMError, VMError, VMResult};
-use move_core_types::{
-    account_address::AccountAddress,
-    ident_str,
-    identifier::IdentStr,
-    language_storage::{ModuleId, StructTag, CORE_CODE_ADDRESS},
-    value::MoveTypeLayout,
-    vm_status::StatusCode,
-};
+use move_binary_format::errors::{PartialVMError, PartialVMResult};
+use move_core_types::{language_storage::StructTag, value::MoveTypeLayout, vm_status::StatusCode};
 use std::{
     collections::{BTreeMap, HashSet},
+    fmt::Debug,
     sync::Arc,
 };
 
@@ -38,7 +32,7 @@ use std::{
 
 /// Allows to query AggregatorV1 values from the state storage.
 pub trait TAggregatorV1View {
-    type Identifier;
+    type Identifier: Debug;
 
     /// Aggregator V1 is implemented as a state item, and therefore the API has
     /// the same pattern as for modules or resources:
@@ -49,12 +43,15 @@ pub trait TAggregatorV1View {
     fn get_aggregator_v1_state_value(
         &self,
         id: &Self::Identifier,
-    ) -> anyhow::Result<Option<StateValue>>;
+    ) -> PartialVMResult<Option<StateValue>>;
 
-    fn get_aggregator_v1_value(&self, id: &Self::Identifier) -> anyhow::Result<Option<u128>> {
+    fn get_aggregator_v1_value(&self, id: &Self::Identifier) -> PartialVMResult<Option<u128>> {
         let maybe_state_value = self.get_aggregator_v1_state_value(id)?;
         match maybe_state_value {
-            Some(state_value) => Ok(Some(bcs::from_bytes(state_value.bytes())?)),
+            Some(state_value) => Ok(Some(bcs::from_bytes(state_value.bytes()).map_err(|e| {
+                PartialVMError::new(StatusCode::UNEXPECTED_DESERIALIZATION_ERROR)
+                    .with_message(format!("Failed to deserialize aggregator value: {:?}", e))
+            })?)),
             None => Ok(None),
         }
     }
@@ -64,7 +61,7 @@ pub trait TAggregatorV1View {
     fn get_aggregator_v1_state_value_metadata(
         &self,
         id: &Self::Identifier,
-    ) -> anyhow::Result<Option<StateValueMetadata>> {
+    ) -> PartialVMResult<Option<StateValueMetadata>> {
         // When getting state value metadata for aggregator V1, we need to do a
         // precise read.
         let maybe_state_value = self.get_aggregator_v1_state_value(id)?;
@@ -78,32 +75,11 @@ pub trait TAggregatorV1View {
         &self,
         id: &Self::Identifier,
         delta_op: &DeltaOp,
-    ) -> VMResult<WriteOp> {
-        // We need to set abort location for Aggregator V1 to ensure correct VMStatus can
-        // be constructed.
-        const AGGREGATOR_V1_ADDRESS: AccountAddress = CORE_CODE_ADDRESS;
-        const AGGREGATOR_V1_MODULE_NAME: &IdentStr = ident_str!("aggregator");
-        let vm_error = |e: PartialVMError| -> VMError {
-            e.finish(Location::Module(ModuleId::new(
-                AGGREGATOR_V1_ADDRESS,
-                AGGREGATOR_V1_MODULE_NAME.into(),
-            )))
-        };
-
-        let base = self
-            .get_aggregator_v1_value(id)
-            .map_err(|e| {
-                vm_error(
-                    PartialVMError::new(StatusCode::SPECULATIVE_EXECUTION_ABORT_ERROR)
-                        .with_message(e.to_string()),
-                )
-            })?
-            .ok_or_else(|| {
-                vm_error(
-                    PartialVMError::new(StatusCode::SPECULATIVE_EXECUTION_ABORT_ERROR)
-                        .with_message("Cannot convert delta for deleted aggregator".to_string()),
-                )
-            })?;
+    ) -> PartialVMResult<WriteOp> {
+        let base = self.get_aggregator_v1_value(id)?.ok_or_else(|| {
+            PartialVMError::new(StatusCode::SPECULATIVE_EXECUTION_ABORT_ERROR)
+                .with_message("Cannot convert delta for deleted aggregator".to_string())
+        })?;
 
         delta_op
             .apply_to(base)
@@ -116,10 +92,11 @@ pub trait TAggregatorV1View {
                     reason: DeltaApplicationFailureReason::Underflow,
                     ..
                 }) => subtraction_v1_error(e),
+                // Because aggregator V1 never underflows or overflows, all other
+                // application errors are bugs.
                 _ => code_invariant_error(format!("Unexpected delta application error: {:?}", e))
                     .into(),
             })
-            .map_err(vm_error)
             .map(|result| WriteOp::legacy_modification(serialize(&result).into()))
     }
 }
@@ -137,8 +114,13 @@ where
     fn get_aggregator_v1_state_value(
         &self,
         state_key: &Self::Identifier,
-    ) -> anyhow::Result<Option<StateValue>> {
-        self.get_state_value(state_key).map_err(Into::into)
+    ) -> PartialVMResult<Option<StateValue>> {
+        self.get_state_value(state_key).map_err(|e| {
+            PartialVMError::new(StatusCode::STORAGE_ERROR).with_message(format!(
+                "Aggregator value not found for {:?}: {:?}",
+                state_key, e
+            ))
+        })
     }
 }
 

aptos-move/aptos-aggregator/src/tests/types.rs

@@ -16,6 +16,7 @@ use aptos_types::{
     state_store::{state_key::StateKey, state_value::StateValue},
     write_set::WriteOp,
 };
+use move_binary_format::errors::PartialVMResult;
 use move_core_types::{language_storage::StructTag, value::MoveTypeLayout};
 use std::{
     cell::RefCell,
@@ -73,7 +74,7 @@ impl TAggregatorV1View for FakeAggregatorView {
     fn get_aggregator_v1_state_value(
         &self,
         state_key: &Self::Identifier,
-    ) -> anyhow::Result<Option<StateValue>> {
+    ) -> PartialVMResult<Option<StateValue>> {
         Ok(self.v1_store.get(state_key).cloned())
     }
 }

aptos-move/aptos-vm-types/src/change_set.rs

@@ -25,7 +25,14 @@ use aptos_types::{
     write_set::{TransactionWrite, WriteOp, WriteOpSize, WriteSetMut},
 };
 use move_binary_format::errors::{Location, PartialVMError, PartialVMResult, VMResult};
-use move_core_types::{value::MoveTypeLayout, vm_status::StatusCode};
+use move_core_types::{
+    account_address::AccountAddress,
+    ident_str,
+    identifier::IdentStr,
+    language_storage::{ModuleId, CORE_CODE_ADDRESS},
+    value::MoveTypeLayout,
+    vm_status::StatusCode,
+};
 use std::{
     collections::{
         btree_map::Entry::{Occupied, Vacant},
@@ -450,8 +457,18 @@ impl VMChangeSet {
                 // Materialization is needed when committing a transaction, so
                 // we need precise mode to compute the true value of an
                 // aggregator.
-                let write =
-                    resolver.try_convert_aggregator_v1_delta_into_write_op(&state_key, &delta)?;
+                let write = resolver
+                    .try_convert_aggregator_v1_delta_into_write_op(&state_key, &delta)
+                    .map_err(|e| {
+                        // We need to set abort location for Aggregator V1 to ensure correct VMStatus can
+                        // be constructed.
+                        const AGGREGATOR_V1_ADDRESS: AccountAddress = CORE_CODE_ADDRESS;
+                        const AGGREGATOR_V1_MODULE_NAME: &IdentStr = ident_str!("aggregator");
+                        e.finish(Location::Module(ModuleId::new(
+                            AGGREGATOR_V1_ADDRESS,
+                            AGGREGATOR_V1_MODULE_NAME.into(),
+                        )))
+                    })?;
                 Ok((state_key, write))
             };
 

aptos-move/aptos-vm-types/src/resolver.rs

@@ -16,7 +16,8 @@ use aptos_types::{
     write_set::WriteOp,
 };
 use bytes::Bytes;
-use move_core_types::{language_storage::StructTag, value::MoveTypeLayout};
+use move_binary_format::errors::{PartialVMError, PartialVMResult};
+use move_core_types::{language_storage::StructTag, value::MoveTypeLayout, vm_status::StatusCode};
 use std::collections::{BTreeMap, HashMap};
 
 /// Allows to query resources from the state.
@@ -32,27 +33,27 @@ pub trait TResourceView {
         &self,
         state_key: &Self::Key,
         maybe_layout: Option<&Self::Layout>,
-    ) -> anyhow::Result<Option<StateValue>>;
+    ) -> PartialVMResult<Option<StateValue>>;
 
     fn get_resource_bytes(
         &self,
         state_key: &Self::Key,
         maybe_layout: Option<&Self::Layout>,
-    ) -> anyhow::Result<Option<Bytes>> {
+    ) -> PartialVMResult<Option<Bytes>> {
         let maybe_state_value = self.get_resource_state_value(state_key, maybe_layout)?;
         Ok(maybe_state_value.map(|state_value| state_value.bytes().clone()))
     }
 
     fn get_resource_state_value_metadata(
         &self,
         state_key: &Self::Key,
-    ) -> anyhow::Result<Option<StateValueMetadata>> {
+    ) -> PartialVMResult<Option<StateValueMetadata>> {
         // For metadata, layouts are not important.
         self.get_resource_state_value(state_key, None)
             .map(|maybe_state_value| maybe_state_value.map(StateValue::into_metadata))
     }
 
-    fn resource_exists(&self, state_key: &Self::Key) -> anyhow::Result<bool> {
+    fn resource_exists(&self, state_key: &Self::Key) -> PartialVMResult<bool> {
         // For existence, layouts are not important.
         self.get_resource_state_value(state_key, None)
             .map(|maybe_state_value| maybe_state_value.is_some())
@@ -68,7 +69,7 @@ pub trait TResourceGroupView {
     type Layout;
 
     /// Some resolvers might not be capable of the optimization, and should return false.
-    /// Others might return based on the config or the run paramaters.
+    /// Others might return based on the config or the run parameters.
     fn is_resource_group_split_in_change_set_capable(&self) -> bool {
         false
     }
@@ -86,14 +87,15 @@ pub trait TResourceGroupView {
     /// the parallel execution setting, as a wrong value will be (later) caught by validation.
     /// Thus, R/W conflicts are avoided, as long as the estimates are correct (e.g. updating
     /// struct members of a fixed size).
-    fn resource_group_size(&self, group_key: &Self::GroupKey) -> anyhow::Result<ResourceGroupSize>;
+    fn resource_group_size(&self, group_key: &Self::GroupKey)
+        -> PartialVMResult<ResourceGroupSize>;
 
     fn get_resource_from_group(
         &self,
         group_key: &Self::GroupKey,
         resource_tag: &Self::ResourceTag,
         maybe_layout: Option<&Self::Layout>,
-    ) -> anyhow::Result<Option<Bytes>>;
+    ) -> PartialVMResult<Option<Bytes>>;
 
     /// Needed for charging storage fees for a resource group write, as that requires knowing
     /// the size of the resource group AFTER the changeset of the transaction is applied (while
@@ -104,7 +106,7 @@ pub trait TResourceGroupView {
         &self,
         group_key: &Self::GroupKey,
         resource_tag: &Self::ResourceTag,
-    ) -> anyhow::Result<usize> {
+    ) -> PartialVMResult<usize> {
         Ok(self
             .get_resource_from_group(group_key, resource_tag, None)?
             .map_or(0, |bytes| bytes.len()))
@@ -124,7 +126,7 @@ pub trait TResourceGroupView {
         &self,
         group_key: &Self::GroupKey,
         resource_tag: &Self::ResourceTag,
-    ) -> anyhow::Result<bool> {
+    ) -> PartialVMResult<bool> {
         self.get_resource_from_group(group_key, resource_tag, None)
             .map(|maybe_bytes| maybe_bytes.is_some())
     }
@@ -142,22 +144,22 @@ pub trait TModuleView {
     ///   -  Ok(None)         if the module is not in storage,
     ///   -  Ok(Some(...))    if the module exists in storage,
     ///   -  Err(...)         otherwise (e.g. storage error).
-    fn get_module_state_value(&self, state_key: &Self::Key) -> anyhow::Result<Option<StateValue>>;
+    fn get_module_state_value(&self, state_key: &Self::Key) -> PartialVMResult<Option<StateValue>>;
 
-    fn get_module_bytes(&self, state_key: &Self::Key) -> anyhow::Result<Option<Bytes>> {
+    fn get_module_bytes(&self, state_key: &Self::Key) -> PartialVMResult<Option<Bytes>> {
         let maybe_state_value = self.get_module_state_value(state_key)?;
         Ok(maybe_state_value.map(|state_value| state_value.bytes().clone()))
     }
 
     fn get_module_state_value_metadata(
         &self,
         state_key: &Self::Key,
-    ) -> anyhow::Result<Option<StateValueMetadata>> {
+    ) -> PartialVMResult<Option<StateValueMetadata>> {
         let maybe_state_value = self.get_module_state_value(state_key)?;
         Ok(maybe_state_value.map(StateValue::into_metadata))
     }
 
-    fn module_exists(&self, state_key: &Self::Key) -> anyhow::Result<bool> {
+    fn module_exists(&self, state_key: &Self::Key) -> PartialVMResult<bool> {
         self.get_module_state_value(state_key)
             .map(|maybe_state_value| maybe_state_value.is_some())
     }
@@ -237,8 +239,13 @@ where
         &self,
         state_key: &Self::Key,
         _maybe_layout: Option<&Self::Layout>,
-    ) -> anyhow::Result<Option<StateValue>> {
-        self.get_state_value(state_key).map_err(Into::into)
+    ) -> PartialVMResult<Option<StateValue>> {
+        self.get_state_value(state_key).map_err(|e| {
+            PartialVMError::new(StatusCode::STORAGE_ERROR).with_message(format!(
+                "Unexpected storage error for resource at {:?}: {:?}",
+                state_key, e
+            ))
+        })
     }
 }
 
@@ -248,8 +255,13 @@ where
 {
     type Key = StateKey;
 
-    fn get_module_state_value(&self, state_key: &Self::Key) -> anyhow::Result<Option<StateValue>> {
-        self.get_state_value(state_key).map_err(Into::into)
+    fn get_module_state_value(&self, state_key: &Self::Key) -> PartialVMResult<Option<StateValue>> {
+        self.get_state_value(state_key).map_err(|e| {
+            PartialVMError::new(StatusCode::STORAGE_ERROR).with_message(format!(
+                "Unexpected storage error for module at {:?}: {:?}",
+                state_key, e
+            ))
+        })
     }
 }
 
@@ -266,25 +278,6 @@ where
     }
 }
 
-/// Allows to query storage metadata in the VM session. Needed for storage refunds.
-/// - Result being Err means storage error or some incostistency (e.g. during speculation,
-/// needing to abort/halt the transaction with an error status).
-/// - Ok(None) means that the corresponding data does not exist / was deleted.
-/// - Ok(Some(_ : MetadataKind)) may be internally None (within Kind) if the metadata was
-/// not previously provided (e.g. Legacy WriteOps).
-pub trait StateValueMetadataResolver {
-    fn get_module_state_value_metadata(
-        &self,
-        state_key: &StateKey,
-    ) -> anyhow::Result<Option<StateValueMetadata>>;
-
-    /// Can also be used to get the metadata of a resource group at a provided group key.
-    fn get_resource_state_value_metadata(
-        &self,
-        state_key: &StateKey,
-    ) -> anyhow::Result<Option<StateValueMetadata>>;
-}
-
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
 pub enum ResourceGroupSize {
     Concrete(u64),