Update gas metering (#8526)

* Add accurate resource group gas metering

* Add accurate resource group gas metering

* fix

* add test

* Correct feature version

* change trait signature

* Clear any resource cache after proloque

* bump gas feature

* add comment

* fix trigger condition for build jobs

* test loadtest

* add improvement

* Fix respawn session whatever that maybe

---------

Co-authored-by: gerben <stavenga@gmail.com>
Co-authored-by: geekflyer <christian@aptoslabs.com>

aptos-move/aptos-gas/src/gas_meter.rs

@@ -33,6 +33,8 @@ use move_vm_types::{
 use std::collections::BTreeMap;
 
 // Change log:
+// - V9
+//   - Accurate tracking of the cost of loading resource groups
 // - V8
 //   - Added BLS12-381 operations.
 // - V7
@@ -59,7 +61,7 @@ use std::collections::BTreeMap;
 //       global operations.
 // - V1
 //   - TBA
-pub const LATEST_GAS_FEATURE_VERSION: u64 = 8;
+pub const LATEST_GAS_FEATURE_VERSION: u64 = 9;
 
 pub(crate) const EXECUTION_GAS_MULTIPLIER: u64 = 20;
 

aptos-move/aptos-vm/src/aptos_vm.rs

@@ -9,7 +9,7 @@ use crate::{
     aptos_vm_impl::{get_transaction_output, AptosVMImpl, AptosVMInternals},
     block_executor::BlockAptosVM,
     counters::*,
-    data_cache::{AsMoveResolver, StorageAdapter},
+    data_cache::StorageAdapter,
     errors::expect_only_successful_execution,
     move_vm_ext::{MoveResolverExt, RespawnedSession, SessionExt, SessionId},
     sharded_block_executor::ShardedBlockExecutor,
@@ -244,6 +244,14 @@ impl AptosVM {
         .1
     }
 
+    pub fn as_move_resolver<'a, S: StateView>(&self, state_view: &'a S) -> StorageAdapter<'a, S> {
+        StorageAdapter::new_with_cached_config(
+            state_view,
+            self.0.get_gas_feature_version(),
+            self.0.get_features(),
+        )
+    }
+
     fn failed_transaction_cleanup_and_keep_vm_status(
         &self,
         error_code: VMStatus,
@@ -1019,6 +1027,9 @@ impl AptosVM {
             // have been previously cached in the prologue.
             //
             // TODO(Gas): Do this in a better way in the future, perhaps without forcing the data cache to be flushed.
+            // By releasing resource group cache, we start with a fresh slate for resource group
+            // cost accounting.
+            resolver.release_resource_group_cache();
             session = self.0.new_session(resolver, SessionId::txn(txn), true);
         }
 
@@ -1137,8 +1148,7 @@ impl AptosVM {
         F: FnOnce(u64, AptosGasParameters, StorageGasParameters, Gas) -> Result<G, VMStatus>,
     {
         // TODO(Gas): revisit this.
-        let resolver = StorageAdapter::new(state_view);
-        let vm = AptosVM::new(&resolver);
+        let vm = AptosVM::new(state_view);
 
         // TODO(Gas): avoid creating txn metadata twice.
         let balance = TransactionMetadata::new(txn).max_gas_amount();
@@ -1149,6 +1159,11 @@ impl AptosVM {
             balance,
         )?;
 
+        let resolver = StorageAdapter::new_with_cached_config(
+            state_view,
+            vm.0.get_gas_feature_version(),
+            vm.0.get_features(),
+        );
         let (status, output) =
             vm.execute_user_transaction_impl(&resolver, txn, log_context, &mut gas_meter);
 
@@ -1332,7 +1347,7 @@ impl AptosVM {
 
         // Try to simulate with aggregator enabled.
         let (vm_status, vm_output) = simulation_vm.simulate_signed_transaction(
-            &state_view.as_move_resolver(),
+            &simulation_vm.0.as_move_resolver(state_view),
             txn,
             &log_context,
             true,
@@ -1349,7 +1364,7 @@ impl AptosVM {
             Err(_) => {
                 // Conversion to TransactionOutput failed, re-simulate without aggregators.
                 let (vm_status, vm_output) = simulation_vm.simulate_signed_transaction(
-                    &state_view.as_move_resolver(),
+                    &simulation_vm.0.as_move_resolver(state_view),
                     txn,
                     &log_context,
                     false,
@@ -1383,8 +1398,8 @@ impl AptosVM {
             vm.0.get_storage_gas_parameters(&log_context)?.clone(),
             gas_budget,
         );
-        let resolver = &state_view.as_move_resolver();
-        let mut session = vm.new_session(resolver, SessionId::Void, true);
+        let resolver = vm.as_move_resolver(state_view);
+        let mut session = vm.new_session(&resolver, SessionId::Void, true);
 
         let func_inst = session.load_function(&module_id, &func_name, &type_args)?;
         let metadata = vm.0.extract_module_metadata(&module_id);
@@ -1557,11 +1572,11 @@ impl VMValidator for AptosVM {
             },
         };
 
-        let resolver = &state_view.as_move_resolver();
-        let mut session = self.0.new_session(resolver, SessionId::txn(&txn), true);
+        let resolver = self.as_move_resolver(state_view);
+        let mut session = self.0.new_session(&resolver, SessionId::txn(&txn), true);
         let validation_result = self.validate_signature_checked_transaction(
             &mut session,
-            resolver,
+            &resolver,
             &txn,
             true,
             &log_context,

aptos-move/aptos-vm/src/aptos_vm_impl.rs

@@ -163,7 +163,7 @@ impl AptosVMImpl {
             features,
         };
         vm.version = Version::fetch_config(&storage);
-        vm.transaction_validation = Self::get_transaction_validation(&StorageAdapter::new(state));
+        vm.transaction_validation = Self::get_transaction_validation(&storage);
         vm
     }
 

aptos-move/aptos-vm/src/block_executor/vm_wrapper.rs

@@ -6,7 +6,6 @@ use crate::{
     adapter_common::{PreprocessedTransaction, VMAdapter},
     aptos_vm::AptosVM,
     block_executor::AptosTransactionOutput,
-    data_cache::{AsMoveResolver, StorageAdapter},
 };
 use aptos_block_executor::task::{ExecutionStatus, ExecutorTask};
 use aptos_logger::{enabled, Level};
@@ -43,7 +42,7 @@ impl<'a, S: 'a + StateView + Sync> ExecutorTask for AptosExecutorTask<'a, S> {
 
         let _ = vm.load_module(
             &ModuleId::new(CORE_CODE_ADDRESS, ident_str!("account").to_owned()),
-            &StorageAdapter::new(argument),
+            &vm.as_move_resolver(argument),
         );
 
         Self {
@@ -66,7 +65,7 @@ impl<'a, S: 'a + StateView + Sync> ExecutorTask for AptosExecutorTask<'a, S> {
 
         match self
             .vm
-            .execute_single_transaction(txn, &view.as_move_resolver(), &log_context)
+            .execute_single_transaction(txn, &self.vm.as_move_resolver(view), &log_context)
         {
             Ok((vm_status, mut vm_output, sender)) => {
                 if materialize_deltas {

aptos-move/aptos-vm/src/data_cache.rs

@@ -21,7 +21,7 @@ use move_core_types::{
     account_address::AccountAddress,
     language_storage::{ModuleId, StructTag},
     metadata::Metadata,
-    resolver::{ModuleResolver, ResourceResolver},
+    resolver::{resource_add_cost, ModuleResolver, ResourceResolver},
     vm_status::StatusCode,
 };
 use move_table_extension::{TableHandle, TableResolver};
@@ -42,16 +42,45 @@ pub(crate) fn get_resource_group_from_metadata(
 /// Adapter to convert a `StateView` into a `MoveResolverExt`.
 pub struct StorageAdapter<'a, S> {
     state_store: &'a S,
+    accurate_byte_count: bool,
+    max_binary_format_version: u32,
     resource_group_cache:
         RefCell<BTreeMap<AccountAddress, BTreeMap<StructTag, BTreeMap<StructTag, Vec<u8>>>>>,
 }
 
 impl<'a, S: StateView> StorageAdapter<'a, S> {
+    pub fn new_with_cached_config(
+        state_store: &'a S,
+        gas_feature_version: u64,
+        features: &Features,
+    ) -> Self {
+        let mut s = Self {
+            state_store,
+            accurate_byte_count: false,
+            max_binary_format_version: 0,
+            resource_group_cache: RefCell::new(BTreeMap::new()),
+        };
+        if gas_feature_version >= 9 {
+            s.accurate_byte_count = true;
+        }
+        s.max_binary_format_version = get_max_binary_format_version(features, gas_feature_version);
+        s
+    }
+
     pub fn new(state_store: &'a S) -> Self {
-        Self {
+        let mut s = Self {
             state_store,
+            accurate_byte_count: false,
+            max_binary_format_version: 0,
             resource_group_cache: RefCell::new(BTreeMap::new()),
+        };
+        let (_, gas_feature_version) = gas_config(&s);
+        let features = Features::fetch_config(&s).unwrap_or_default();
+        if gas_feature_version >= 9 {
+            s.accurate_byte_count = true;
         }
+        s.max_binary_format_version = get_max_binary_format_version(&features, gas_feature_version);
+        s
     }
 
     pub fn get(&self, access_path: AccessPath) -> PartialVMResult<Option<Vec<u8>>> {
@@ -65,32 +94,39 @@ impl<'a, S: StateView> StorageAdapter<'a, S> {
         address: &AccountAddress,
         struct_tag: &StructTag,
         metadata: &[Metadata],
-    ) -> Result<Option<Vec<u8>>, VMError> {
+    ) -> Result<Option<(Vec<u8>, u64)>, VMError> {
         let resource_group = get_resource_group_from_metadata(struct_tag, metadata);
         if let Some(resource_group) = resource_group {
             let mut cache = self.resource_group_cache.borrow_mut();
             let cache = cache.entry(*address).or_insert_with(BTreeMap::new);
             if let Some(group_data) = cache.get_mut(&resource_group) {
                 // This resource group is already cached for this address. So just return the
                 // cached value.
-                return Ok(group_data.get(struct_tag).cloned());
+                let buf = group_data.get(struct_tag).cloned();
+                return Ok(resource_add_cost(buf, 0));
             }
             let group_data = self.get_resource_group_data(address, &resource_group)?;
             if let Some(group_data) = group_data {
+                let len = if self.accurate_byte_count {
+                    group_data.len() as u64
+                } else {
+                    0
+                };
                 let group_data: BTreeMap<StructTag, Vec<u8>> = bcs::from_bytes(&group_data)
                     .map_err(|_| {
                         PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR)
                             .finish(Location::Undefined)
                     })?;
                 let res = group_data.get(struct_tag).cloned();
                 cache.insert(resource_group, group_data);
-                Ok(res)
+                Ok(resource_add_cost(res, len))
             } else {
                 cache.insert(resource_group, BTreeMap::new());
                 Ok(None)
             }
         } else {
-            self.get_standard_resource(address, struct_tag)
+            let buf = self.get_standard_resource(address, struct_tag)?;
+            Ok(resource_add_cost(buf, 0))
         }
     }
 }
@@ -118,13 +154,8 @@ impl<'a, S: StateView> MoveResolverExt for StorageAdapter<'a, S> {
 
     fn release_resource_group_cache(
         &self,
-        address: &AccountAddress,
-        resource_group: &StructTag,
-    ) -> Option<BTreeMap<StructTag, Vec<u8>>> {
-        self.resource_group_cache
-            .borrow_mut()
-            .get_mut(address)?
-            .remove(resource_group)
+    ) -> BTreeMap<AccountAddress, BTreeMap<StructTag, BTreeMap<StructTag, Vec<u8>>>> {
+        self.resource_group_cache.take()
     }
 }
 
@@ -134,7 +165,7 @@ impl<'a, S: StateView> ResourceResolver for StorageAdapter<'a, S> {
         address: &AccountAddress,
         struct_tag: &StructTag,
         metadata: &[Metadata],
-    ) -> Result<Option<Vec<u8>>, Error> {
+    ) -> anyhow::Result<Option<(Vec<u8>, u64)>> {
         Ok(self.get_any_resource(address, struct_tag, metadata)?)
     }
 }
@@ -145,13 +176,9 @@ impl<'a, S: StateView> ModuleResolver for StorageAdapter<'a, S> {
             Ok(Some(bytes)) => bytes,
             _ => return vec![],
         };
-        let (_, gas_feature_version) = gas_config(self);
-        let features = Features::fetch_config(self).unwrap_or_default();
-        let max_binary_format_version =
-            get_max_binary_format_version(&features, gas_feature_version);
         let module = match CompiledModule::deserialize_with_max_version(
             &module_bytes,
-            max_binary_format_version,
+            self.max_binary_format_version,
         ) {
             Ok(module) => module,
             _ => return vec![],

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

@@ -29,9 +29,7 @@ pub trait MoveResolverExt:
 
     fn release_resource_group_cache(
         &self,
-        address: &AccountAddress,
-        resource_group: &StructTag,
-    ) -> Option<BTreeMap<StructTag, Vec<u8>>>;
+    ) -> BTreeMap<AccountAddress, BTreeMap<StructTag, BTreeMap<StructTag, Vec<u8>>>>;
 
     // Move to API does not belong here
     fn is_resource_group(&self, struct_tag: &StructTag) -> bool {

aptos-move/aptos-vm/src/move_vm_ext/respawned_session.rs

@@ -3,7 +3,7 @@
 
 use crate::{
     aptos_vm_impl::AptosVMImpl,
-    data_cache::{AsMoveResolver, StorageAdapter},
+    data_cache::StorageAdapter,
     move_vm_ext::{SessionExt, SessionId},
 };
 use anyhow::{bail, Result};
@@ -44,7 +44,13 @@ impl<'r, 'l> RespawnedSession<'r, 'l> {
 
         Ok(RespawnedSessionBuilder {
             state_view,
-            resolver_builder: |state_view| state_view.as_move_resolver(),
+            resolver_builder: |state_view| {
+                StorageAdapter::new_with_cached_config(
+                    state_view,
+                    vm.get_gas_feature_version(),
+                    vm.get_features(),
+                )
+            },
             session_builder: |resolver| Some(vm.new_session(resolver, session_id, true)),
         }
         .build())

aptos-move/aptos-vm/src/move_vm_ext/session.rs

@@ -38,6 +38,7 @@ use move_table_extension::{NativeTableContext, TableChangeSet};
 use move_vm_runtime::{move_vm::MoveVM, session::Session};
 use serde::{Deserialize, Serialize};
 use std::{
+    borrow::BorrowMut,
     collections::BTreeMap,
     ops::{Deref, DerefMut},
     sync::Arc,
@@ -199,19 +200,21 @@ impl<'r, 'l> SessionExt<'r, 'l> {
         let mut change_set_filtered = MoveChangeSet::new();
         let mut resource_group_change_set = MoveChangeSet::new();
 
+        let mut resource_group_cache = remote.release_resource_group_cache();
         for (addr, account_changeset) in change_set.into_inner() {
             let mut resource_groups: BTreeMap<StructTag, AccountChangeSet> = BTreeMap::new();
             let mut resources_filtered = BTreeMap::new();
             let (modules, resources) = account_changeset.into_inner();
 
             for (struct_tag, blob_op) in resources {
-                let resource_group = runtime.with_module_metadata(&struct_tag.module_id(), |md| {
-                    get_resource_group_from_metadata(&struct_tag, md)
-                });
+                let resource_group_tag = runtime
+                    .with_module_metadata(&struct_tag.module_id(), |md| {
+                        get_resource_group_from_metadata(&struct_tag, md)
+                    });
 
-                if let Some(resource_group) = resource_group {
+                if let Some(resource_group_tag) = resource_group_tag {
                     resource_groups
-                        .entry(resource_group)
+                        .entry(resource_group_tag)
                         .or_insert_with(AccountChangeSet::new)
                         .add_resource_op(struct_tag, blob_op)
                         .map_err(|_| common_error())?;
@@ -227,9 +230,11 @@ impl<'r, 'l> SessionExt<'r, 'l> {
                 )
                 .map_err(|_| common_error())?;
 
-            for (resource_tag, resources) in resource_groups {
-                let mut source_data = remote
-                    .release_resource_group_cache(&addr, &resource_tag)
+            for (resource_group_tag, resources) in resource_groups {
+                let mut source_data = resource_group_cache
+                    .borrow_mut()
+                    .get_mut(&addr)
+                    .and_then(|t| t.remove(&resource_group_tag))
                     .unwrap_or_default();
                 let create = source_data.is_empty();
 
@@ -259,7 +264,7 @@ impl<'r, 'l> SessionExt<'r, 'l> {
                     MoveStorageOp::Modify(bcs::to_bytes(&source_data).map_err(|_| common_error())?)
                 };
                 resource_group_change_set
-                    .add_resource_op(addr, resource_tag, op)
+                    .add_resource_op(addr, resource_group_tag, op)
                     .map_err(|_| common_error())?;
             }
         }