[Gas Metering] More accurate object size calculation (MystenLabs#1249)

* [Gas Metering] More accurate object size calculation

* address feedback

sui_core/src/authority.rs

@@ -148,7 +148,7 @@ impl AuthorityState {
         // fetching only unique objects.
         let total_size = all_objects
             .iter()
-            .map(|(_, obj)| obj.object_data_size())
+            .map(|(_, obj)| obj.object_size_for_gas_metering())
             .sum();
         gas_status.charge_storage_read(total_size)?;
 

sui_core/src/authority/temporary_store.rs

@@ -125,9 +125,9 @@ impl<S> AuthorityTemporaryStore<S> {
         for (object_id, (_object_ref, object)) in &self.written {
             // Objects in written can be either mutation or creation.
             // We figure it out by looking them up in `self.objects`.
-            let object_size = object.object_data_size();
+            let object_size = object.object_size_for_gas_metering();
             let old_object_size = if let Some(old_obj) = self.objects.get(object_id) {
-                old_obj.object_data_size()
+                old_obj.object_size_for_gas_metering()
             } else {
                 0
             };
@@ -139,7 +139,7 @@ impl<S> AuthorityTemporaryStore<S> {
             // object was unwrapped and then deleted. The rebate would have been provided already when
             // mutating the object that wrapped this object.
             if let Some(old_obj) = self.objects.get(object_id) {
-                gas_status.charge_storage_mutation(old_obj.object_data_size(), 0)?;
+                gas_status.charge_storage_mutation(old_obj.object_size_for_gas_metering(), 0)?;
             }
         }
         // Also charge gas for mutating the gas object in advance.

sui_core/src/execution_engine.rs

@@ -133,7 +133,7 @@ fn execute_transaction<S: BackingPackageStore>(
     }
     temporary_store.ensure_active_inputs_mutated();
     if let Err(err) = temporary_store
-        .charge_gas_for_storage_changes(&mut gas_status, gas_object.object_data_size())
+        .charge_gas_for_storage_changes(&mut gas_status, gas_object.object_size_for_gas_metering())
     {
         result = Err(err);
     }

sui_core/src/unit_tests/gas_tests.rs

@@ -106,10 +106,19 @@ async fn test_native_transfer_sufficient_gas() -> SuiResult {
     let mut gas_status = SuiGasStatus::new_with_budget(*MAX_GAS_BUDGET);
     gas_status.charge_min_tx_gas()?;
 
-    gas_status.charge_storage_read(object.object_data_size() + gas_object.object_data_size())?;
-    gas_status.charge_storage_mutation(object.object_data_size(), object.object_data_size())?;
-    gas_status
-        .charge_storage_mutation(gas_object.object_data_size(), gas_object.object_data_size())?;
+    // Both the object to be transferred and the gas object will be read
+    // from the store. Hence we need to charge for 2 reads.
+    gas_status.charge_storage_read(
+        object.object_size_for_gas_metering() + gas_object.object_size_for_gas_metering(),
+    )?;
+    gas_status.charge_storage_mutation(
+        object.object_size_for_gas_metering(),
+        object.object_size_for_gas_metering(),
+    )?;
+    gas_status.charge_storage_mutation(
+        gas_object.object_size_for_gas_metering(),
+        gas_object.object_size_for_gas_metering(),
+    )?;
     assert_eq!(gas_cost, &gas_status.summary(true));
     Ok(())
 }
@@ -214,14 +223,21 @@ async fn test_publish_gas() -> SuiResult {
     // Mimic the gas charge behavior and cross check the result with above.
     let mut gas_status = SuiGasStatus::new_with_budget(*MAX_GAS_BUDGET);
     gas_status.charge_min_tx_gas()?;
-    gas_status.charge_storage_read(genesis_objects.iter().map(|o| o.object_data_size()).sum())?;
-    gas_status.charge_storage_read(gas_object.object_data_size())?;
+    gas_status.charge_storage_read(
+        genesis_objects
+            .iter()
+            .map(|o| o.object_size_for_gas_metering())
+            .sum(),
+    )?;
+    gas_status.charge_storage_read(gas_object.object_size_for_gas_metering())?;
     gas_status.charge_publish_package(publish_bytes.iter().map(|v| v.len()).sum())?;
-    gas_status.charge_storage_mutation(0, package.object_data_size())?;
+    gas_status.charge_storage_mutation(0, package.object_size_for_gas_metering())?;
     // Remember the gas used so far. We will use this to create another failure case latter.
     let gas_used_after_package_creation = gas_status.summary(true).gas_used();
-    gas_status
-        .charge_storage_mutation(gas_object.object_data_size(), gas_object.object_data_size())?;
+    gas_status.charge_storage_mutation(
+        gas_object.object_size_for_gas_metering(),
+        gas_object.object_size_for_gas_metering(),
+    )?;
     assert_eq!(gas_cost, &gas_status.summary(true));
 
     // Create a transaction with budget DELTA less than the gas cost required.
@@ -342,19 +358,21 @@ async fn test_move_call_gas() -> SuiResult {
         .get_object(&package_object_ref.0)
         .await?
         .unwrap();
-    gas_status.charge_storage_read(package_object.object_data_size())?;
-    gas_status.charge_storage_read(gas_object.object_data_size())?;
+    gas_status.charge_storage_read(package_object.object_size_for_gas_metering())?;
+    gas_status.charge_storage_read(gas_object.object_size_for_gas_metering())?;
     let gas_used_before_vm_exec = gas_status.summary(true).gas_used();
     // The gas cost to execute the function in Move VM.
     // Hard code it here since it's difficult to mock that in test.
     const MOVE_VM_EXEC_COST: u64 = 17;
-    gas_status
-        .charge_storage_mutation(gas_object.object_data_size(), gas_object.object_data_size())?;
+    gas_status.charge_storage_mutation(
+        gas_object.object_size_for_gas_metering(),
+        gas_object.object_size_for_gas_metering(),
+    )?;
     let created_object = authority_state
         .get_object(&effects.created[0].0 .0)
         .await?
         .unwrap();
-    gas_status.charge_storage_mutation(0, created_object.object_data_size())?;
+    gas_status.charge_storage_mutation(0, created_object.object_size_for_gas_metering())?;
 
     let new_cost = gas_status.summary(true);
     assert_eq!(

sui_types/src/object.rs

@@ -3,6 +3,7 @@
 
 use std::convert::{TryFrom, TryInto};
 use std::fmt::{Debug, Display, Formatter};
+use std::mem::size_of;
 
 use move_binary_format::binary_views::BinaryIndexedView;
 use move_binary_format::CompiledModule;
@@ -162,6 +163,16 @@ impl MoveObject {
             error: e.to_string(),
         })
     }
+
+    /// Approximate size of the object in bytes. This is used for gas metering.
+    /// For the type tag field, we serialize it on the spot to get the accurate size.
+    /// This should not be very expensive since the type tag is usually simple, and
+    /// we only do this once per object being mutated.
+    pub fn object_size_for_gas_metering(&self) -> usize {
+        let seriealized_type_tag =
+            bcs::to_bytes(&self.type_).expect("Serializing type tag should not fail");
+        self.contents.len() + seriealized_type_tag.len()
+    }
 }
 
 #[derive(Eq, PartialEq, Debug, Clone, Deserialize, Serialize, Hash)]
@@ -403,18 +414,21 @@ impl Object {
         ObjectDigest::new(sha3_hash(self))
     }
 
-    // Size of the object in bytes.
-    // TODO: For now we just look at the size of the data.
-    // Do we need to be accurate and look at the serialized size?
-    pub fn object_data_size(&self) -> usize {
-        match &self.data {
-            Data::Move(m) => m.contents.len(),
+    /// Approximate size of the object in bytes. This is used for gas metering.
+    /// This will be slgihtly different from the serialized size, but
+    /// we also don't want to serialize the object just to get the size.
+    /// This approximation should be good enough for gas metering.
+    pub fn object_size_for_gas_metering(&self) -> usize {
+        let meta_data_size = size_of::<Owner>() + size_of::<TransactionDigest>();
+        let data_size = match &self.data {
+            Data::Move(m) => m.object_size_for_gas_metering(),
             Data::Package(p) => p
                 .serialized_module_map()
-                .values()
-                .map(|module| module.len())
+                .iter()
+                .map(|(name, module)| name.len() + module.len())
                 .sum(),
-        }
+        };
+        meta_data_size + data_size
     }
 
     /// Change the owner of `self` to `new_owner`

sui_types/src/unit_tests/base_types_tests.rs

@@ -4,9 +4,12 @@
 
 #![allow(clippy::blacklisted_name)]
 
+use move_binary_format::file_format;
+
 use crate::{
     crypto::{get_key_pair, BcsSignable, Signature},
     gas_coin::GasCoin,
+    object::Object,
 };
 use std::str::FromStr;
 
@@ -171,3 +174,29 @@ fn test_object_id_from_empty_string() {
     assert!(ObjectID::try_from("".to_string()).is_err());
     assert!(ObjectID::from_str("").is_err());
 }
+
+#[test]
+fn test_move_object_size_for_gas_metering() {
+    let object = Object::with_id_owner_for_testing(
+        ObjectID::random(),
+        SuiAddress::random_for_testing_only(),
+    );
+    let size = object.object_size_for_gas_metering();
+    let serialized = bcs::to_bytes(&object).unwrap();
+    // The result of object_size_for_gas_metering() will be smaller due to not including
+    // all the metadata data needed for serializing various types.
+    // If the following assertion breaks, it's likely you have changed MoveObject's fields.
+    // Make sure to adjust `object_size_for_gas_metering()` to include those changes.
+    assert_eq!(size + 16, serialized.len());
+}
+
+#[test]
+fn test_move_package_size_for_gas_metering() {
+    let module = file_format::empty_module();
+    let package = Object::new_package(vec![module], TransactionDigest::genesis());
+    let size = package.object_size_for_gas_metering();
+    let serialized = bcs::to_bytes(&package).unwrap();
+    // If the following assertion breaks, it's likely you have changed MovePackage's fields.
+    // Make sure to adjust `object_size_for_gas_metering()` to include those changes.
+    assert_eq!(size + 5, serialized.len());
+}