diff --git a/Cargo.lock b/Cargo.lock index 76d53a4e80..8e099375c2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1014,6 +1014,9 @@ name = "bytes" version = "1.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "89b2fd2a0dcf38d7971e2194b6b6eebab45ae01067456a7fd93d5547a61b70be" +dependencies = [ + "serde 1.0.152", +] [[package]] name = "bytesize" @@ -11033,6 +11036,7 @@ dependencies = [ "anyhow", "bcs-ext", "bech32", + "bytes 1.4.0", "chrono", "forkable-jellyfish-merkle", "hex", diff --git a/Cargo.toml b/Cargo.toml index d8cee9cb41..f8cd6efedd 100644 --- a/Cargo.toml +++ b/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" diff --git a/etc/starcoin_types.yml b/etc/starcoin_types.yml index ea11e85123..fae7095e71 100644 --- a/etc/starcoin_types.yml +++ b/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 diff --git a/rpc/api/src/types.rs b/rpc/api/src/types.rs index 532a140998..676be9a0a9 100644 --- a/rpc/api/src/types.rs +++ b/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 { diff --git a/state/statedb/src/lib.rs b/state/statedb/src/lib.rs index 752903de90..26df4ea057 100644 --- a/state/statedb/src/lib.rs +++ b/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 => { diff --git a/state/statedb/src/tests.rs b/state/statedb/src/tests.rs index 2765675fa2..380de2cdf2 100644 --- a/state/statedb/src/tests.rs +++ b/state/statedb/src/tests.rs @@ -14,7 +14,7 @@ fn random_bytes() -> Vec { fn to_write_set(access_path: AccessPath, value: Vec) -> 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, values: Vec>) -> W .into_iter() .zip(values) .into_iter() - .map(|(key, val)| (key, WriteOp::Value(val))) + .map(|(key, val)| (key, WriteOp::Creation(val))) .collect::>(), ) .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(()) } diff --git a/types/src/account.rs b/types/src/account.rs index 86843f82ee..6db2ab111b 100644 --- a/types/src/account.rs +++ b/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() } diff --git a/vm/e2e-tests/src/account.rs b/vm/e2e-tests/src/account.rs index 4eeb3bfe77..ff139f2535 100644 --- a/vm/e2e-tests/src/account.rs +++ b/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() diff --git a/vm/e2e-tests/src/data_store.rs b/vm/e2e-tests/src/data_store.rs index bf9c595b7f..d32ceb8aae 100644 --- a/vm/e2e-tests/src/data_store.rs +++ b/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 => { diff --git a/vm/starcoin-transactional-test-harness/src/lib.rs b/vm/starcoin-transactional-test-harness/src/lib.rs index 6e023aabfe..defcaa2e4a 100644 --- a/vm/starcoin-transactional-test-harness/src/lib.rs +++ b/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 diff --git a/vm/types/Cargo.toml b/vm/types/Cargo.toml index 45a44f8412..51e91cd736 100644 --- a/vm/types/Cargo.toml +++ b/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 } diff --git a/vm/types/src/write_set.rs b/vm/types/src/write_set.rs index e37993c855..0adae45eb8 100644 --- a/vm/types/src/write_set.rs +++ b/vm/types/src/write_set.rs @@ -10,8 +10,9 @@ use serde::{Deserialize, Serialize}; #[derive(Clone, Eq, Hash, PartialEq, Serialize, Deserialize)] pub enum WriteOp { + Creation(#[serde(with = "serde_bytes")] Vec), + Modification(#[serde(with = "serde_bytes")] Vec), Deletion, - Value(#[serde(with = "serde_bytes")] Vec), } impl WriteOp { @@ -19,7 +20,23 @@ impl WriteOp { 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, } } } @@ -27,9 +44,17 @@ impl WriteOp { 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::() + ), + WriteOp::Modification(value) => write!( f, - "Value({})", + "Modification({})", value .iter() .map(|byte| format!("{:02x}", byte)) diff --git a/vm/vm-runtime/src/data_cache.rs b/vm/vm-runtime/src/data_cache.rs index f380f71410..29f81fa243 100644 --- a/vm/vm-runtime/src/data_cache.rs +++ b/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 => { diff --git a/vm/vm-runtime/src/move_vm_ext/session.rs b/vm/vm-runtime/src/move_vm_ext/session.rs index 57c3b8aa42..35def82613 100644 --- a/vm/vm-runtime/src/move_vm_ext/session.rs +++ b/vm/vm-runtime/src/move_vm_ext/session.rs @@ -93,29 +93,25 @@ 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>| 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)) } } @@ -123,14 +119,13 @@ impl SessionOutput { 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))) } } } diff --git a/vm/vm-runtime/src/parallel_executor/storage_wrapper.rs b/vm/vm-runtime/src/parallel_executor/storage_wrapper.rs index 0a316ac2cc..fadd1e6a14 100644 --- a/vm/vm-runtime/src/parallel_executor/storage_wrapper.rs +++ b/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>> { 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), diff --git a/vm/vm-runtime/src/starcoin_vm.rs b/vm/vm-runtime/src/starcoin_vm.rs index c9402e5f51..fca57aeda6 100644 --- a/vm/vm-runtime/src/starcoin_vm.rs +++ b/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 {