add Modification type to WriteOp(WriteSet)

Cargo.toml

@@ -259,7 +259,7 @@ bencher = "0.1.5"
 bitflags = "1.3.2"
 bs58 = "0.3.1"
 byteorder = "1.3.4"
-bytes = "1"
+bytes = { version = "1.4.0", features = ["serde"] }
 chrono = { version = "0.4.19", default-features = false, features = ["clock"] }
 clap = { version = "3", features = ["derive"] }
 cli-table = "0.3.2"

etc/starcoin_types.yml

@@ -350,10 +350,13 @@ WithdrawCapabilityResource:
 WriteOp:
   ENUM:
     0:
-      Deletion: UNIT
+      Creation:
+        NEWTYPE: BYTES
     1:
-      Value:
+      Modification:
         NEWTYPE: BYTES
+    2:
+      Deletion: UNIT
 WriteSet:
   NEWTYPESTRUCT:
     TYPENAME: WriteSetMut

rpc/api/src/types.rs

@@ -1233,16 +1233,18 @@ impl From<(AccessPath, WriteOp)> for TransactionOutputAction {
     fn from((access_path, op): (AccessPath, WriteOp)) -> Self {
         let (action, value) = match op {
             WriteOp::Deletion => (WriteOpView::Deletion, None),
-            WriteOp::Value(v) => (
-                WriteOpView::Value,
-                Some(if access_path.path.is_resource() {
-                    WriteOpValueView::Resource(v.into())
-                } else {
-                    WriteOpValueView::Code(v.into())
-                }),
-            ),
+            WriteOp::Creation(v) => (WriteOpView::Creation, Some(v)),
+            WriteOp::Modification(v) => (WriteOpView::Modification, Some(v)),
         };
 
+        let value = value.map(|v| {
+            if access_path.path.is_resource() {
+                WriteOpValueView::Resource(v.into())
+            } else {
+                WriteOpValueView::Code(v.into())
+            }
+        });
+
         TransactionOutputAction {
             access_path,
             action,
@@ -1267,14 +1269,16 @@ pub enum WriteOpValueView {
 #[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
 pub enum WriteOpView {
     Deletion,
-    Value,
+    Creation,
+    Modification,
 }
 
 impl From<(TableItem, WriteOp)> for TransactionOutputTableItemAction {
     fn from((table_item, op): (TableItem, WriteOp)) -> Self {
         let (action, value) = match op {
             WriteOp::Deletion => (WriteOpView::Deletion, None),
-            WriteOp::Value(v) => (WriteOpView::Value, Some(StrView(v))),
+            WriteOp::Creation(v) => (WriteOpView::Creation, Some(StrView(v))),
+            WriteOp::Modification(v) => (WriteOpView::Modification, Some(StrView(v))),
         };
 
         TransactionOutputTableItemAction {

state/statedb/src/lib.rs

@@ -576,7 +576,7 @@ impl ChainStateWriter for ChainStateDB {
         self.apply_write_set(
             WriteSetMut::new(vec![(
                 StateKey::AccessPath(access_path.clone()),
-                WriteOp::Value(value),
+                WriteOp::Creation(value),
             )])
             .freeze()
             .expect("freeze write_set must success."),
@@ -630,11 +630,18 @@ impl ChainStateWriter for ChainStateDB {
                     locks.insert(access_path.address);
                     let (account_address, data_path) = access_path.into_inner();
                     match write_op {
-                        WriteOp::Value(value) => {
+                        WriteOp::Creation(value) => {
                             let account_state_object =
                                 self.get_account_state_object(&account_address, true)?;
                             account_state_object.set(data_path, value);
                         }
+                        WriteOp::Modification(value) => {
+                            // Just make sure the key has already existed.
+                            let account_state_object =
+                                self.get_account_state_object(&account_address, false)?;
+                            let _ = account_state_object.get(&data_path)?;
+                            account_state_object.set(data_path, value)
+                        }
                         WriteOp::Deletion => {
                             let account_state_object =
                                 self.get_account_state_object(&account_address, false)?;
@@ -648,7 +655,12 @@ impl ChainStateWriter for ChainStateDB {
                     let table_handle_state_object =
                         self.get_table_handle_state_object(&table_item.handle)?;
                     match write_op {
-                        WriteOp::Value(value) => {
+                        WriteOp::Creation(value) => {
+                            table_handle_state_object.set(table_item.key, value);
+                        }
+                        WriteOp::Modification(value) => {
+                            // Just make sure the key has already existed.
+                            let _ = table_handle_state_object.get(&table_item.key)?;
                             table_handle_state_object.set(table_item.key, value);
                         }
                         WriteOp::Deletion => {

state/statedb/src/tests.rs

@@ -14,7 +14,7 @@ fn random_bytes() -> Vec<u8> {
 fn to_write_set(access_path: AccessPath, value: Vec<u8>) -> WriteSet {
     WriteSetMut::new(vec![(
         StateKey::AccessPath(access_path),
-        WriteOp::Value(value),
+        WriteOp::Creation(value),
     )])
     .freeze()
     .expect("freeze write_set must success.")
@@ -26,7 +26,7 @@ fn state_keys_to_write_set(state_keys: Vec<StateKey>, values: Vec<Vec<u8>>) -> W
             .into_iter()
             .zip(values)
             .into_iter()
-            .map(|(key, val)| (key, WriteOp::Value(val)))
+            .map(|(key, val)| (key, WriteOp::Creation(val)))
             .collect::<Vec<_>>(),
     )
     .freeze()
@@ -159,7 +159,7 @@ fn check_write_set(chain_state_db: &ChainStateDB, write_set: &WriteSet) -> Resul
     for (state_key, value) in write_set.iter() {
         let val = chain_state_db.get_state_value(state_key)?;
         assert!(val.is_some());
-        assert_eq!(WriteOp::Value(val.unwrap()), *value);
+        assert_eq!(WriteOp::Creation(val.unwrap()), *value);
     }
     Ok(())
 }

types/src/account.rs

@@ -561,7 +561,7 @@ impl AccountData {
             .unwrap();
         write_set.push((
             StateKey::AccessPath(self.make_account_access_path()),
-            WriteOp::Value(account),
+            WriteOp::Creation(account),
         ));
         for (code, balance_blob) in balance_blobs.into_iter() {
             let balance = balance_blob
@@ -571,7 +571,7 @@ impl AccountData {
                 .unwrap();
             write_set.push((
                 StateKey::AccessPath(self.make_balance_access_path(code.as_str())),
-                WriteOp::Value(balance),
+                WriteOp::Creation(balance),
             ));
         }
 
@@ -582,7 +582,7 @@ impl AccountData {
             .unwrap();
         write_set.push((
             StateKey::AccessPath(self.make_event_generator_access_path()),
-            WriteOp::Value(event_generator),
+            WriteOp::Creation(event_generator),
         ));
         WriteSetMut::new(write_set).freeze().unwrap()
     }

vm/e2e-tests/src/account.rs

@@ -610,7 +610,7 @@ impl AccountData {
             .unwrap();
         write_set.push((
             StateKey::AccessPath(self.make_account_access_path()),
-            WriteOp::Value(account),
+            WriteOp::Creation(account),
         ));
 
         let balance = coinstore_blob
@@ -620,7 +620,7 @@ impl AccountData {
             .unwrap();
         write_set.push((
             StateKey::AccessPath(self.make_coin_store_access_path()),
-            WriteOp::Value(balance),
+            WriteOp::Creation(balance),
         ));
 
         WriteSetMut::new(write_set).freeze().unwrap()

vm/e2e-tests/src/data_store.rs

@@ -49,7 +49,7 @@ impl FakeDataStore {
         let mut write_handle = self.state_data.write().expect("Panic for lock");
         for (state_key, write_op) in write_set {
             match write_op {
-                WriteOp::Value(blob) => {
+                WriteOp::Creation(blob) => {
                     write_handle.insert(state_key.clone(), blob.clone());
                 }
                 WriteOp::Deletion => {

vm/starcoin-transactional-test-harness/src/lib.rs

@@ -1148,7 +1148,7 @@ impl<'a> MoveTestAdapter<'a> for StarcoinTestAdapter<'a> {
                             m.named_module.address.into_inner(),
                             Identifier::new(m.named_module.name.as_str()).unwrap(),
                         )),
-                        WriteOp::Value({
+                        WriteOp::Creation({
                             let mut bytes = vec![];
                             m.named_module.module.serialize(&mut bytes).unwrap();
                             bytes

vm/types/Cargo.toml

@@ -1,6 +1,7 @@
 [dependencies]
 anyhow = { workspace = true }
 bech32 = { workspace = true }
+bytes = { workspace = true }
 chrono = { default-features = false, features = ["clock"], workspace = true }
 hex = { workspace = true }
 log = { workspace = true }

vm/types/src/write_set.rs

@@ -10,26 +10,51 @@ use serde::{Deserialize, Serialize};
 
 #[derive(Clone, Eq, Hash, PartialEq, Serialize, Deserialize)]
 pub enum WriteOp {
+    Creation(#[serde(with = "serde_bytes")] Vec<u8>),
+    Modification(#[serde(with = "serde_bytes")] Vec<u8>),
     Deletion,
-    Value(#[serde(with = "serde_bytes")] Vec<u8>),
 }
 
 impl WriteOp {
     #[inline]
     pub fn is_deletion(&self) -> bool {
         match self {
             WriteOp::Deletion => true,
-            WriteOp::Value(_) => false,
+            WriteOp::Creation(_) | WriteOp::Modification(_) => false,
+        }
+    }
+
+    #[inline]
+    pub fn is_creation(&self) -> bool {
+        match self {
+            WriteOp::Deletion | WriteOp::Modification(_) => false,
+            WriteOp::Creation(_) => true,
+        }
+    }
+
+    #[inline]
+    pub fn is_modification(&self) -> bool {
+        match self {
+            WriteOp::Deletion | WriteOp::Creation(_) => false,
+            WriteOp::Modification(_) => true,
         }
     }
 }
 
 impl std::fmt::Debug for WriteOp {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
-            WriteOp::Value(value) => write!(
+            WriteOp::Creation(value) => write!(
+                f,
+                "Creation({})",
+                value
+                    .iter()
+                    .map(|byte| format!("{:02x}", byte))
+                    .collect::<String>()
+            ),
+            WriteOp::Modification(value) => write!(
                 f,
-                "Value({})",
+                "Modification({})",
                 value
                     .iter()
                     .map(|byte| format!("{:02x}", byte))

vm/vm-runtime/src/data_cache.rs

@@ -54,7 +54,15 @@ impl<'a, S: StateView> StateViewCache<'a, S> {
     pub(crate) fn push_write_set(&mut self, write_set: &WriteSet) {
         for (ref ap, ref write_op) in write_set.iter() {
             match write_op {
-                WriteOp::Value(blob) => {
+                WriteOp::Creation(blob) => {
+                    self.data_map.insert(ap.clone(), Some(blob.clone()));
+                }
+                WriteOp::Modification(blob) => {
+                    // Fixme: 1. make sure the key has already existed;
+                    // 2. keep a minimal impact on block execution performance
+                    #[cfg(feature = "testing")]
+                    let _ = self.get_state_value(ap).expect("{ap} didn't exist");
+
                     self.data_map.insert(ap.clone(), Some(blob.clone()));
                 }
                 WriteOp::Deletion => {

vm/vm-runtime/src/move_vm_ext/session.rs

@@ -93,44 +93,39 @@ impl SessionOutput {
             table_change_set,
         } = self;
 
-        // XXX FIXME YSG check write_set need upgrade? why aptos no need MoveStorageOp
+        // see `fn convert` in `aptos-move/aptos-vm/src/move_vm_ext/write_op_converter.rs`
+        let move_storage_op_to_write_op = |blob_opt: MoveStorageOp<Vec<u8>>| match blob_opt {
+            MoveStorageOp::Delete => WriteOp::Deletion,
+            MoveStorageOp::New(blob) => WriteOp::Creation(blob),
+            MoveStorageOp::Modify(blob) => WriteOp::Modification(blob),
+        };
+
         let mut write_set_mut = WriteSetMut::new(Vec::new());
         for (addr, account_changeset) in change_set.into_inner() {
             let (modules, resources) = account_changeset.into_inner();
             for (struct_tag, blob_opt) in resources {
                 let ap = ap_cache.get_resource_path(addr, struct_tag);
-                let op = match blob_opt {
-                    MoveStorageOp::Delete => WriteOp::Deletion,
-                    MoveStorageOp::New(blob) => WriteOp::Value(blob),
-                    MoveStorageOp::Modify(blob) => WriteOp::Value(blob),
-                };
+                let op = move_storage_op_to_write_op(blob_opt);
                 write_set_mut.push((StateKey::AccessPath(ap), op))
             }
 
-            // XXX FIXME YSG check write_set need upgrade? why aptos no need MoveStorageOp
             for (name, blob_opt) in modules {
                 let ap = ap_cache.get_module_path(ModuleId::new(addr, name));
-                let op = match blob_opt {
-                    MoveStorageOp::Delete => WriteOp::Deletion,
-                    MoveStorageOp::New(blob) => WriteOp::Value(blob),
-                    MoveStorageOp::Modify(blob) => WriteOp::Value(blob),
-                };
-
+                let op = move_storage_op_to_write_op(blob_opt);
                 write_set_mut.push((StateKey::AccessPath(ap), op))
             }
         }
 
         for (handle, change) in table_change_set.changes {
             for (key, value_op) in change.entries {
                 let state_key = StateKey::table_item(handle.into(), key);
-                // XXX FIXME YSG check write_set need upgrade? why aptos no need MoveStorageOp
                 match value_op {
                     MoveStorageOp::Delete => write_set_mut.push((state_key, WriteOp::Deletion)),
                     MoveStorageOp::New(bytes) => {
-                        write_set_mut.push((state_key, WriteOp::Value(bytes)))
+                        write_set_mut.push((state_key, WriteOp::Creation(bytes)))
                     }
                     MoveStorageOp::Modify(bytes) => {
-                        write_set_mut.push((state_key, WriteOp::Value(bytes)))
+                        write_set_mut.push((state_key, WriteOp::Modification(bytes)))
                     }
                 }
             }

vm/vm-runtime/src/parallel_executor/storage_wrapper.rs

@@ -30,7 +30,7 @@ impl<'a, S: StateView> StateView for VersionedView<'a, S> {
     fn get_state_value(&self, state_key: &StateKey) -> anyhow::Result<Option<Vec<u8>>> {
         match self.hashmap_view.read(state_key) {
             Some(v) => Ok(match v.as_ref() {
-                WriteOp::Value(w) => Some(w.clone()),
+                WriteOp::Creation(w) | WriteOp::Modification(w) => Some(w.clone()),
                 WriteOp::Deletion => None,
             }),
             None => self.base_view.get_state_value(state_key),

vm/vm-runtime/src/starcoin_vm.rs

@@ -1101,7 +1101,7 @@ impl StarcoinVM {
                                 );
                             }
                             // Push write set to write set
-                            data_cache.push_write_set(output.write_set())
+                            data_cache.push_write_set(output.write_set());
                         }
                         // TODO load config by config change event
                         self.check_reconfigure(&data_cache, &output)
@@ -1151,7 +1151,7 @@ impl StarcoinVM {
                             "Block metadata transaction keep status must been Executed."
                         );
                         // Push write set to write set
-                        data_cache.push_write_set(output.write_set())
+                        data_cache.push_write_set(output.write_set());
                     }
                     #[cfg(feature = "metrics")]
                     if let Some(timer) = timer {