Strictly check metadata compatibility

aptos-labs · Dec 14, 2023 · 76d2d3d · 76d2d3d
1 parent 6d18420
commit 76d2d3d
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 13 deletions.
diff --git a/aptos-move/aptos-vm-types/src/tests/test_change_set.rs b/aptos-move/aptos-vm-types/src/tests/test_change_set.rs
@@ -830,7 +830,7 @@ mod tests {
         );
         additional_update.insert(
             key.clone(),
-            group_write(write_op_with_metadata(additional_type_idx, 200), vec![], 0),
+            group_write(write_op_with_metadata(additional_type_idx, 100), vec![], 0),
         );
 
         assert_ok!(VMChangeSet::squash_additional_resource_writes(
@@ -889,7 +889,7 @@ mod tests {
         additional_update.insert(
             key.clone(),
             group_write(
-                write_op_with_metadata(DELETION, 200), // delete
+                write_op_with_metadata(DELETION, 100), // delete
                 vec![],
                 0,
             ),
@@ -930,7 +930,7 @@ mod tests {
         additional_update.insert(
             key_1.clone(),
             group_write(
-                write_op_with_metadata(MODIFICATION, 200),
+                write_op_with_metadata(MODIFICATION, 100),
                 vec![
                     (
                         mock_tag_0(),
@@ -966,7 +966,7 @@ mod tests {
         additional_update.insert(
             key_2.clone(),
             group_write(
-                write_op_with_metadata(MODIFICATION, 200),
+                write_op_with_metadata(MODIFICATION, 100),
                 vec![
                     (
                         mock_tag_0(),
@@ -1012,7 +1012,7 @@ mod tests {
         let additional_update = BTreeMap::from([(
             key_2.clone(),
             group_write(
-                write_op_with_metadata(MODIFICATION, 200),
+                write_op_with_metadata(MODIFICATION, 100),
                 vec![(mock_tag_1(), (WriteOp::legacy_deletion(), None))],
                 0,
             ),

diff --git a/types/src/write_set.rs b/types/src/write_set.rs
@@ -12,7 +12,7 @@ use crate::{
     },
     write_set::WriteOp::{Creation, Deletion, Modification},
 };
-use anyhow::{bail, Result};
+use anyhow::{bail, ensure, Result};
 use aptos_crypto_derive::{BCSCryptoHash, CryptoHasher};
 use bytes::Bytes;
 use once_cell::sync::Lazy;
@@ -147,21 +147,27 @@ impl WriteOp {
             => {
                 bail!("The given change sets cannot be squashed")
             },
-            (Creation {metadata, .. } , Modification {data, ..}) => {
+            (Creation {metadata: old_meta, .. } , Modification {data, metadata}) => {
+                Self::ensure_metadata_compatible(old_meta, &metadata)?;
+
                 *op = Creation {
                     data,
-                    metadata: metadata.clone(),
+                    metadata,
                 };
             },
-            (Modification{metadata, .. } , Modification {data, ..}) => {
+            (Modification{metadata: old_meta, .. } , Modification {data, metadata}) => {
+                Self::ensure_metadata_compatible(old_meta, &metadata)?;
+
                 *op = Modification {
                     data,
-                    metadata: metadata.clone(),
+                    metadata,
                 };
             },
-            (Modification {metadata, ..}, Deletion {..}) => {
+            (Modification {metadata: old_meta, ..}, Deletion {metadata}) => {
+                Self::ensure_metadata_compatible(old_meta, &metadata)?;
+
                 *op = Deletion {
-                    metadata: metadata.clone(),
+                    metadata,
                 }
             },
             (Deletion {metadata}, Creation {data, ..}) => {
@@ -176,13 +182,28 @@ impl WriteOp {
                     metadata: metadata.clone(),
                 }
             },
-            (Creation { .. }, Deletion { .. }) => {
+            (Creation { metadata: old_meta, .. }, Deletion { metadata }) => {
+                Self::ensure_metadata_compatible(old_meta, &metadata)?;
+
                 return Ok(false)
             },
         }
         Ok(true)
     }
 
+    fn ensure_metadata_compatible(
+        old: &StateValueMetadata,
+        new: &StateValueMetadata,
+    ) -> Result<()> {
+        // Write ops shouldn't be squashed after the second one is charged for fees, which might
+        // result in metadata change (bytes_deposit increase, for example).
+        ensure!(
+            old == new,
+            "Squashing incompatible metadata: old:{old:?}, new:{new:?}",
+        );
+        Ok(())
+    }
+
     pub fn bytes(&self) -> Option<&Bytes> {
         use WriteOp::*;