From f1c9b46fdd2166e87f9be88fed961c908bbb1ed5 Mon Sep 17 00:00:00 2001 From: aldenhu Date: Tue, 5 Nov 2024 23:08:35 +0000 Subject: [PATCH] WriteOp carries StateValue --- aptos-move/aptos-vm-types/src/change_set.rs | 8 +- .../src/tests/test_change_set.rs | 38 +--- .../src/move_vm_ext/write_op_converter.rs | 22 +-- .../src/tests/storage_refund.rs | 2 +- crates/aptos-rosetta/src/test/mod.rs | 8 +- testsuite/generate-format/src/api.rs | 4 +- testsuite/generate-format/src/aptos.rs | 9 +- testsuite/generate-format/src/consensus.rs | 9 +- types/src/state_store/state_value.rs | 17 ++ types/src/write_set.rs | 176 ++++++++---------- 10 files changed, 118 insertions(+), 175 deletions(-) diff --git a/aptos-move/aptos-vm-types/src/change_set.rs b/aptos-move/aptos-vm-types/src/change_set.rs index 5f6029dd2f984a..1e3236b80a153c 100644 --- a/aptos-move/aptos-vm-types/src/change_set.rs +++ b/aptos-move/aptos-vm-types/src/change_set.rs @@ -411,18 +411,18 @@ impl VMChangeSet { if let Some(write_op) = aggregator_v1_write_set.get_mut(&state_key) { // In this case, delta follows a write op. match write_op { - Creation { data, .. } | Modification { data, .. } => { + Creation(v) | Modification(v) => { // Apply delta on top of creation or modification. // TODO[agg_v1](cleanup): This will not be needed anymore once aggregator // change sets carry non-serialized information. - let base: u128 = bcs::from_bytes(data) + let base: u128 = bcs::from_bytes(v.bytes()) .expect("Deserializing into an aggregator value always succeeds"); let value = additional_delta_op .apply_to(base) .map_err(PartialVMError::from)?; - *data = serialize(&value).into(); + v.set_bytes(serialize(&value).into()) }, - Deletion { .. } => { + Deletion(..) => { // This case (applying a delta to deleted item) should // never happen. Let's still return an error instead of // panicking. 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 index 2d887e12e5c803..ff5f0c16512c8d 100644 --- a/aptos-move/aptos-vm-types/src/tests/test_change_set.rs +++ b/aptos-move/aptos-vm-types/src/tests/test_change_set.rs @@ -436,10 +436,7 @@ fn test_aggregator_v2_snapshots_and_derived() { #[test] fn test_resource_groups_squashing() { - let modification_metadata = WriteOp::Modification { - data: Bytes::new(), - metadata: raw_metadata(2000), - }; + let modification_metadata = WriteOp::modification(Bytes::new(), raw_metadata(2000)); macro_rules! as_create_op { ($val:expr) => { @@ -593,10 +590,7 @@ fn test_write_and_read_discrepancy_caught() { )]) .try_build()); - let metadata_op = WriteOp::Modification { - data: Bytes::new(), - metadata: raw_metadata(1000), - }; + let metadata_op = WriteOp::modification(Bytes::new(), raw_metadata(1000)); let group_size = ResourceGroupSize::Combined { num_tagged_resources: 1, all_tagged_resources_size: 14, @@ -637,17 +631,9 @@ mod tests { pub(crate) fn write_op_with_metadata(type_idx: u8, v: u128) -> WriteOp { match type_idx { - CREATION => WriteOp::Creation { - data: vec![].into(), - metadata: raw_metadata(v as u64), - }, - MODIFICATION => WriteOp::Modification { - data: vec![].into(), - metadata: raw_metadata(v as u64), - }, - DELETION => WriteOp::Deletion { - metadata: raw_metadata(v as u64), - }, + CREATION => WriteOp::creation(vec![].into(), raw_metadata(v as u64)), + MODIFICATION => WriteOp::modification(vec![].into(), raw_metadata(v as u64)), + DELETION => WriteOp::deletion(raw_metadata(v as u64)), _ => unreachable!("Wrong type index for test"), } } @@ -694,9 +680,7 @@ mod tests { None ); assert_group_write_size!( - WriteOp::Deletion { - metadata: raw_metadata(10) - }, + WriteOp::deletion(raw_metadata(10)), ResourceGroupSize::zero_combined(), None ); @@ -729,10 +713,7 @@ mod tests { Some(sizes[0]) ); assert_group_write_size!( - WriteOp::Creation { - data: Bytes::new(), - metadata: raw_metadata(20) - }, + WriteOp::creation(Bytes::new(), raw_metadata(20)), sizes[1], Some(sizes[1]) ); @@ -742,10 +723,7 @@ mod tests { Some(sizes[2]) ); assert_group_write_size!( - WriteOp::Modification { - data: Bytes::new(), - metadata: raw_metadata(30) - }, + WriteOp::modification(Bytes::new(), raw_metadata(30)), sizes[3], Some(sizes[3]) ); diff --git a/aptos-move/aptos-vm/src/move_vm_ext/write_op_converter.rs b/aptos-move/aptos-vm/src/move_vm_ext/write_op_converter.rs index 353a4dd202b15a..1d54be49fe16fd 100644 --- a/aptos-move/aptos-vm/src/move_vm_ext/write_op_converter.rs +++ b/aptos-move/aptos-vm/src/move_vm_ext/write_op_converter.rs @@ -210,7 +210,6 @@ impl<'r> WriteOpConverter<'r> { legacy_creation_as_modification: bool, ) -> PartialVMResult { use MoveStorageOp::*; - use WriteOp::*; let write_op = match (state_value_metadata, move_storage_op) { (None, Modify(_) | Delete) => { // Possible under speculative execution, returning speculative error waiting for re-execution. @@ -238,18 +237,12 @@ impl<'r> WriteOpConverter<'r> { WriteOp::legacy_creation(data) } }, - Some(metadata) => Creation { - data, - metadata: metadata.clone(), - }, - }, - (Some(metadata), Modify(data)) => { - // Inherit metadata even if the feature flags is turned off, for compatibility. - Modification { data, metadata } + Some(metadata) => WriteOp::creation(data, metadata.clone()), }, + (Some(metadata), Modify(data)) => WriteOp::modification(data, metadata), (Some(metadata), Delete) => { // Inherit metadata even if the feature flags is turned off, for compatibility. - Deletion { metadata } + WriteOp::deletion(metadata) }, }; Ok(write_op) @@ -270,13 +263,10 @@ impl<'r> WriteOpConverter<'r> { match &self.new_slot_metadata { // n.b. Aggregator writes historically did not distinguish Create vs Modify. None => WriteOp::legacy_modification(data), - Some(metadata) => WriteOp::Creation { - data, - metadata: metadata.clone(), - }, + Some(metadata) => WriteOp::creation(data, metadata.clone()), } }, - Some(metadata) => WriteOp::Modification { data, metadata }, + Some(metadata) => WriteOp::modification(data, metadata), }; Ok(op) @@ -623,7 +613,7 @@ mod tests { // Deletion should still contain the metadata - for storage refunds. assert_eq!(group_write.metadata_op().metadata(), &metadata); - assert_eq!(group_write.metadata_op(), &WriteOp::Deletion { metadata }); + assert_eq!(group_write.metadata_op(), &WriteOp::deletion(metadata)); assert_none!(group_write.metadata_op().bytes()); } } diff --git a/aptos-move/e2e-move-tests/src/tests/storage_refund.rs b/aptos-move/e2e-move-tests/src/tests/storage_refund.rs index ff510308a8b6b9..15c1e536eed115 100644 --- a/aptos-move/e2e-move-tests/src/tests/storage_refund.rs +++ b/aptos-move/e2e-move-tests/src/tests/storage_refund.rs @@ -159,7 +159,7 @@ fn assert_result( for (_state_key, write_op) in txn_out.write_set() { match write_op { WriteOp::Creation { .. } => creates += 1, - WriteOp::Deletion { metadata } => { + WriteOp::Deletion(metadata) => { if metadata.is_none() { panic!("This test expects all deletions to have metadata") } diff --git a/crates/aptos-rosetta/src/test/mod.rs b/crates/aptos-rosetta/src/test/mod.rs index 173b55935e5741..4dc68738700050 100644 --- a/crates/aptos-rosetta/src/test/mod.rs +++ b/crates/aptos-rosetta/src/test/mod.rs @@ -98,10 +98,10 @@ fn resource_group_modification_write_op( ) -> (StateKey, WriteOp) { let encoded = bcs::to_bytes(input).unwrap(); let state_key = StateKey::resource_group(address, resource); - let write_op = WriteOp::Modification { - data: encoded.into(), - metadata: StateValueMetadata::new(0, 0, &CurrentTimeMicroseconds { microseconds: 0 }), - }; + let write_op = WriteOp::modification( + encoded.into(), + StateValueMetadata::new(0, 0, &CurrentTimeMicroseconds { microseconds: 0 }), + ); (state_key, write_op) } diff --git a/testsuite/generate-format/src/api.rs b/testsuite/generate-format/src/api.rs index e9290bb7fdd5ef..4be4dfcb71592c 100644 --- a/testsuite/generate-format/src/api.rs +++ b/testsuite/generate-format/src/api.rs @@ -95,9 +95,7 @@ pub fn get_registry() -> Result { // 1. Record samples for types with custom deserializers. trace_crypto_values(&mut tracer, &mut samples)?; tracer.trace_value(&mut samples, &event::EventKey::random())?; - tracer.trace_value(&mut samples, &write_set::WriteOp::Deletion { - metadata: StateValueMetadata::none(), - })?; + tracer.trace_value(&mut samples, &write_set::WriteOp::legacy_deletion())?; // 2. Trace the main entry point(s) + every enum separately. // stdlib types diff --git a/testsuite/generate-format/src/aptos.rs b/testsuite/generate-format/src/aptos.rs index 0fa55466b8cdc9..0c4e5fc70da053 100644 --- a/testsuite/generate-format/src/aptos.rs +++ b/testsuite/generate-format/src/aptos.rs @@ -14,10 +14,7 @@ use aptos_crypto_derive::{BCSCryptoHash, CryptoHasher}; use aptos_types::{ block_metadata_ext::BlockMetadataExt, contract_event, event, - state_store::{ - state_key::StateKey, - state_value::{PersistedStateValueMetadata, StateValueMetadata}, - }, + state_store::{state_key::StateKey, state_value::PersistedStateValueMetadata}, transaction, transaction::block_epilogue::BlockEpiloguePayload, validator_txn::ValidatorTransaction, @@ -90,9 +87,7 @@ pub fn get_registry() -> Result { // 1. Record samples for types with custom deserializers. trace_crypto_values(&mut tracer, &mut samples)?; tracer.trace_value(&mut samples, &event::EventKey::random())?; - tracer.trace_value(&mut samples, &write_set::WriteOp::Deletion { - metadata: StateValueMetadata::none(), - })?; + tracer.trace_value(&mut samples, &write_set::WriteOp::legacy_deletion())?; // 2. Trace the main entry point(s) + every enum separately. tracer.trace_type::(&samples)?; diff --git a/testsuite/generate-format/src/consensus.rs b/testsuite/generate-format/src/consensus.rs index 5ef14eb37c69be..17c079dfcbbfc8 100644 --- a/testsuite/generate-format/src/consensus.rs +++ b/testsuite/generate-format/src/consensus.rs @@ -14,10 +14,7 @@ use aptos_crypto_derive::{BCSCryptoHash, CryptoHasher}; use aptos_types::{ block_metadata_ext::BlockMetadataExt, contract_event, event, - state_store::{ - state_key::StateKey, - state_value::{PersistedStateValueMetadata, StateValueMetadata}, - }, + state_store::{state_key::StateKey, state_value::PersistedStateValueMetadata}, transaction, transaction::block_epilogue::BlockEpiloguePayload, validator_txn::ValidatorTransaction, @@ -89,9 +86,7 @@ pub fn get_registry() -> Result { &aptos_consensus_types::block::Block::make_genesis_block(), )?; tracer.trace_value(&mut samples, &event::EventKey::random())?; - tracer.trace_value(&mut samples, &write_set::WriteOp::Deletion { - metadata: StateValueMetadata::none(), - })?; + tracer.trace_value(&mut samples, &write_set::WriteOp::legacy_deletion())?; // 2. Trace the main entry point(s) + every enum separately. tracer.trace_type::(&samples)?; diff --git a/types/src/state_store/state_value.rs b/types/src/state_store/state_value.rs index d69b8b3fc8ec5c..5db5a22b088b2f 100644 --- a/types/src/state_store/state_value.rs +++ b/types/src/state_store/state_value.rs @@ -280,6 +280,23 @@ impl StateValue { )) } + pub fn into_bytes(self) -> Bytes { + self.inner.data + } + + pub fn set_bytes(&mut self, data: Bytes) { + self.inner.data = data; + self.hash = OnceCell::new(); + } + + pub fn metadata(&self) -> &StateValueMetadata { + &self.inner.metadata + } + + pub fn metadata_mut(&mut self) -> &mut StateValueMetadata { + &mut self.inner.metadata + } + pub fn into_metadata(self) -> StateValueMetadata { self.inner.metadata } diff --git a/types/src/write_set.rs b/types/src/write_set.rs index d579f5cc951f7f..711e6e536a3f8e 100644 --- a/types/src/write_set.rs +++ b/types/src/write_set.rs @@ -5,12 +5,9 @@ //! For each transaction the VM executes, the VM will output a `WriteSet` that contains each access //! path it updates. For each access path, the VM can either give its new value or delete it. -use crate::{ - state_store::{ - state_key::StateKey, - state_value::{PersistedStateValueMetadata, StateValue, StateValueMetadata}, - }, - write_set::WriteOp::{Creation, Deletion, Modification}, +use crate::state_store::{ + state_key::StateKey, + state_value::{PersistedStateValueMetadata, StateValue, StateValueMetadata}, }; use anyhow::{bail, ensure, Result}; use aptos_crypto_derive::{BCSCryptoHash, CryptoHasher}; @@ -68,45 +65,25 @@ impl PersistedWriteOp { use PersistedWriteOp::*; match self { - Creation(data) => WriteOp::Creation { - data, - metadata: StateValueMetadata::none(), - }, - Modification(data) => WriteOp::Modification { - data, - metadata: StateValueMetadata::none(), - }, - Deletion => WriteOp::Deletion { - metadata: StateValueMetadata::none(), - }, - CreationWithMetadata { data, metadata } => WriteOp::Creation { - data, - metadata: metadata.into_in_mem_form(), + Creation(data) => WriteOp::legacy_creation(data), + Modification(data) => WriteOp::legacy_modification(data), + Deletion => WriteOp::legacy_deletion(), + CreationWithMetadata { data, metadata } => { + WriteOp::creation(data, metadata.into_in_mem_form()) }, - ModificationWithMetadata { data, metadata } => WriteOp::Modification { - data, - metadata: metadata.into_in_mem_form(), - }, - DeletionWithMetadata { metadata } => WriteOp::Deletion { - metadata: metadata.into_in_mem_form(), + ModificationWithMetadata { data, metadata } => { + WriteOp::modification(data, metadata.into_in_mem_form()) }, + DeletionWithMetadata { metadata } => WriteOp::deletion(metadata.into_in_mem_form()), } } } #[derive(Clone, Eq, PartialEq)] pub enum WriteOp { - Creation { - data: Bytes, - metadata: StateValueMetadata, - }, - Modification { - data: Bytes, - metadata: StateValueMetadata, - }, - Deletion { - metadata: StateValueMetadata, - }, + Creation(StateValue), + Modification(StateValue), + Deletion(StateValueMetadata), } impl WriteOp { @@ -116,17 +93,17 @@ impl WriteOp { let metadata = self.metadata().clone().into_persistable(); match metadata { None => match self { - WriteOp::Creation { data, .. } => Creation(data.clone()), - WriteOp::Modification { data, .. } => Modification(data.clone()), + WriteOp::Creation(v) => Creation(v.bytes().clone()), + WriteOp::Modification(v) => Modification(v.bytes().clone()), WriteOp::Deletion { .. } => Deletion, }, Some(metadata) => match self { - WriteOp::Creation { data, .. } => CreationWithMetadata { - data: data.clone(), + WriteOp::Creation(v) => CreationWithMetadata { + data: v.bytes().clone(), metadata, }, - WriteOp::Modification { data, .. } => ModificationWithMetadata { - data: data.clone(), + WriteOp::Modification(v) => ModificationWithMetadata { + data: v.bytes().clone(), metadata, }, WriteOp::Deletion { .. } => DeletionWithMetadata { metadata }, @@ -147,43 +124,32 @@ impl WriteOp { => { bail!("The given change sets cannot be squashed") }, - (Creation {metadata: old_meta, .. } , Modification {data, metadata}) => { - Self::ensure_metadata_compatible(old_meta, &metadata)?; + (Creation(c) , Modification(m)) => { + Self::ensure_metadata_compatible(c.metadata(), m.metadata())?; - *op = Creation { - data, - metadata, - }; + *op = Creation(m) }, - (Modification{metadata: old_meta, .. } , Modification {data, metadata}) => { - Self::ensure_metadata_compatible(old_meta, &metadata)?; + (Modification(c) , Modification(m)) => { + Self::ensure_metadata_compatible(c.metadata(), m.metadata())?; - *op = Modification { - data, - metadata, - }; + *op = Modification(m); }, - (Modification {metadata: old_meta, ..}, Deletion {metadata}) => { - Self::ensure_metadata_compatible(old_meta, &metadata)?; + (Modification(m), Deletion(d_meta)) => { + Self::ensure_metadata_compatible(m.metadata(), &d_meta)?; - *op = Deletion { - metadata, - } + *op = Deletion(d_meta) }, - (Deletion {metadata}, Creation {data, ..}) => { + (Deletion(d_meta), Creation(c)) => { // n.b. With write sets from multiple sessions being squashed together, it's possible // to see two ops carrying different metadata (or one with it the other without) // due to deleting in one session and recreating in another. The original metadata // shouldn't change due to the squash. // And because the deposit or refund happens after all squashing is finished, it's // not a concern of fairness. - *op = Modification { - data, - metadata: metadata.clone(), - } + *op = Modification(StateValue::new_with_metadata(c.into_bytes(), d_meta.clone())) }, - (Creation { metadata: old_meta, .. }, Deletion { metadata }) => { - Self::ensure_metadata_compatible(old_meta, &metadata)?; + (Creation(c), Deletion(d_meta)) => { + Self::ensure_metadata_compatible(c.metadata(), &d_meta)?; return Ok(false) }, @@ -208,7 +174,7 @@ impl WriteOp { use WriteOp::*; match self { - Creation { data, .. } | Modification { data, .. } => Some(data), + Creation(v) | Modification(v) => Some(v.bytes()), Deletion { .. } => None, } } @@ -217,8 +183,8 @@ impl WriteOp { use WriteOp::*; match self { - Creation { data, .. } | Modification { data, .. } => data.len(), - Deletion { .. } => 0, + Creation(v) | Modification(v) => v.bytes().len(), + Deletion(..) => 0, } } @@ -226,9 +192,8 @@ impl WriteOp { use WriteOp::*; match self { - Creation { metadata, .. } | Modification { metadata, .. } | Deletion { metadata } => { - metadata - }, + Creation(v) | Modification(v) => v.metadata(), + Deletion(d_meta) => d_meta, } } @@ -236,38 +201,42 @@ impl WriteOp { use WriteOp::*; match self { - Creation { metadata, .. } | Modification { metadata, .. } | Deletion { metadata } => { - metadata - }, + Creation(v) | Modification(v) => v.metadata_mut(), + Deletion(d_meta) => d_meta, } } pub fn into_metadata(self) -> StateValueMetadata { + use WriteOp::*; + match self { - Creation { metadata, .. } | Modification { metadata, .. } | Deletion { metadata } => { - metadata - }, + Creation(v) | Modification(v) => v.into_metadata(), + Deletion(d_meta) => d_meta, } } + pub fn creation(data: Bytes, metadata: StateValueMetadata) -> Self { + Self::Creation(StateValue::new_with_metadata(data, metadata)) + } + + pub fn modification(data: Bytes, metadata: StateValueMetadata) -> Self { + Self::Modification(StateValue::new_with_metadata(data, metadata)) + } + + pub fn deletion(metadata: StateValueMetadata) -> Self { + Self::Deletion(metadata) + } + pub fn legacy_creation(data: Bytes) -> Self { - Self::Creation { - data, - metadata: StateValueMetadata::none(), - } + Self::Creation(StateValue::new_legacy(data)) } pub fn legacy_modification(data: Bytes) -> Self { - Self::Modification { - data, - metadata: StateValueMetadata::none(), - } + Self::Modification(StateValue::new_legacy(data)) } pub fn legacy_deletion() -> Self { - Self::Deletion { - metadata: StateValueMetadata::none(), - } + Self::Deletion(StateValueMetadata::none()) } } @@ -389,10 +358,7 @@ impl TransactionWrite for WriteOp { fn from_state_value(maybe_state_value: Option) -> Self { match maybe_state_value { None => Self::legacy_deletion(), - Some(state_value) => { - let (metadata, data) = state_value.unpack(); - Self::Modification { data, metadata } - }, + Some(state_value) => Self::Modification(state_value), } } @@ -409,33 +375,37 @@ impl TransactionWrite for WriteOp { use WriteOp::*; match self { - Creation { data, .. } | Modification { data, .. } => *data = bytes, + Creation(v) | Modification(v) => v.set_bytes(bytes), Deletion { .. } => (), } } } #[allow(clippy::format_collect)] -impl std::fmt::Debug for WriteOp { +impl Debug for WriteOp { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + use WriteOp::*; + match self { - Creation { data, metadata } => write!( + Creation(v) => write!( f, "Creation({}, metadata:{:?})", - data.iter() + v.bytes() + .iter() .map(|byte| format!("{:02x}", byte)) .collect::(), - metadata, + v.metadata(), ), - Modification { data, metadata } => write!( + Modification(v) => write!( f, "Modification({}, metadata:{:?})", - data.iter() + v.bytes() + .iter() .map(|byte| format!("{:02x}", byte)) .collect::(), - metadata, + v.metadata(), ), - Deletion { metadata } => { + Deletion(metadata) => { write!(f, "Deletion(metadata:{:?})", metadata,) }, }