From 72c1a4697615547306f83588a7ea006f3cb97084 Mon Sep 17 00:00:00 2001 From: aldenhu Date: Mon, 11 Dec 2023 17:32:34 +0000 Subject: [PATCH] Automatically upgrade StateValueMetadata So that the bytes deposit can be tracked as soon as possible --- .../aptos-aggregator/src/delta_change_set.rs | 10 +- aptos-move/aptos-aggregator/src/resolver.rs | 6 +- aptos-move/aptos-gas-meter/src/traits.rs | 10 +- .../aptos-vm-types/src/abstract_write_op.rs | 23 +- aptos-move/aptos-vm-types/src/change_set.rs | 9 +- aptos-move/aptos-vm-types/src/resolver.rs | 10 +- .../src/storage/space_pricing.rs | 59 ++-- .../src/tests/test_change_set.rs | 100 +++--- aptos-move/aptos-vm-types/src/tests/utils.rs | 21 +- aptos-move/aptos-vm/src/data_cache.rs | 6 +- .../src/move_vm_ext/respawned_session.rs | 16 +- .../src/move_vm_ext/write_op_converter.rs | 64 ++-- .../block-executor/src/captured_reads.rs | 61 +++- aptos-move/block-executor/src/executor.rs | 6 +- .../src/proptest_types/types.rs | 32 +- aptos-move/block-executor/src/view.rs | 17 +- aptos-move/e2e-move-tests/src/harness.rs | 2 +- .../src/tests/state_metadata.rs | 8 +- .../src/tests/storage_refund.rs | 7 +- aptos-move/e2e-tests/src/account.rs | 4 +- config/src/config/node_config_loader.rs | 15 +- .../executor-benchmark/src/native_executor.rs | 12 +- .../executor/src/mock_vm/mock_vm_test.rs | 14 +- execution/executor/src/mock_vm/mod.rs | 14 +- execution/executor/src/tests/mod.rs | 6 +- .../executor/tests/db_bootstrapper_test.rs | 8 +- .../src/account_config/resources/coin_info.rs | 4 +- types/src/proptest_types.rs | 6 +- types/src/state_store/state_value.rs | 205 ++++++++---- types/src/transaction/change_set.rs | 2 +- types/src/write_set.rs | 300 +++++++++++------- 31 files changed, 610 insertions(+), 447 deletions(-) diff --git a/aptos-move/aptos-aggregator/src/delta_change_set.rs b/aptos-move/aptos-aggregator/src/delta_change_set.rs index 51a120acb239d..c7f7800631f5b 100644 --- a/aptos-move/aptos-aggregator/src/delta_change_set.rs +++ b/aptos-move/aptos-aggregator/src/delta_change_set.rs @@ -589,10 +589,16 @@ mod test { let sub_op = delta_sub(100, 200); let add_result = state_view.try_convert_aggregator_v1_delta_into_write_op(&KEY, &add_op); - assert_ok_eq!(add_result, WriteOp::Modification(serialize(&200).into())); + assert_ok_eq!( + add_result, + WriteOp::legacy_modification(serialize(&200).into()) + ); let sub_result = state_view.try_convert_aggregator_v1_delta_into_write_op(&KEY, &sub_op); - assert_ok_eq!(sub_result, WriteOp::Modification(serialize(&0).into())); + assert_ok_eq!( + sub_result, + WriteOp::legacy_modification(serialize(&0).into()) + ); } #[test] diff --git a/aptos-move/aptos-aggregator/src/resolver.rs b/aptos-move/aptos-aggregator/src/resolver.rs index b4751aa585633..f2c084e7ff445 100644 --- a/aptos-move/aptos-aggregator/src/resolver.rs +++ b/aptos-move/aptos-aggregator/src/resolver.rs @@ -15,7 +15,7 @@ use aptos_types::{ aggregator::PanicError, state_store::{ state_key::StateKey, - state_value::{StateValue, StateValueMetadataKind}, + state_value::{StateValue, StateValueMetadata}, }, write_set::WriteOp, }; @@ -64,7 +64,7 @@ pub trait TAggregatorV1View { fn get_aggregator_v1_state_value_metadata( &self, id: &Self::Identifier, - ) -> anyhow::Result> { + ) -> anyhow::Result> { // When getting state value metadata for aggregator V1, we need to do a // precise read. let maybe_state_value = self.get_aggregator_v1_state_value(id)?; @@ -120,7 +120,7 @@ pub trait TAggregatorV1View { ))) .into_vm_status() }) - .map(|result| WriteOp::Modification(serialize(&result).into())) + .map(|result| WriteOp::legacy_modification(serialize(&result).into())) } } diff --git a/aptos-move/aptos-gas-meter/src/traits.rs b/aptos-move/aptos-gas-meter/src/traits.rs index 720b0a2be8ea3..2f52737032f55 100644 --- a/aptos-move/aptos-gas-meter/src/traits.rs +++ b/aptos-move/aptos-gas-meter/src/traits.rs @@ -144,13 +144,9 @@ pub trait AptosGasMeter: MoveGasMeter { // Write set let mut writeset_charge_and_refund = ChargeAndRefund::zero(); - for (key, op_size, metadata_opt) in change_set.write_set_iter_mut() { - writeset_charge_and_refund.combine(pricing.charge_refund_write_op( - params, - key, - &op_size, - metadata_opt, - )); + for (key, op_size, metadata) in change_set.write_set_iter_mut() { + writeset_charge_and_refund + .combine(pricing.charge_refund_write_op(params, key, &op_size, metadata)); } let ChargeAndRefund { non_discountable, diff --git a/aptos-move/aptos-vm-types/src/abstract_write_op.rs b/aptos-move/aptos-vm-types/src/abstract_write_op.rs index 20bbcc113db5d..b8f1bf57198aa 100644 --- a/aptos-move/aptos-vm-types/src/abstract_write_op.rs +++ b/aptos-move/aptos-vm-types/src/abstract_write_op.rs @@ -5,7 +5,6 @@ use aptos_types::{ state_store::state_value::StateValueMetadata, write_set::{TransactionWrite, WriteOp, WriteOpSize}, }; -use claims::assert_none; use move_core_types::{language_storage::StructTag, value::MoveTypeLayout}; use std::{collections::BTreeMap, sync::Arc}; @@ -55,15 +54,13 @@ impl AbstractResourceWriteOp { }) => { use WriteOp::*; match write_op { - Creation(_) | CreationWithMetadata { .. } => WriteOpSize::Creation { + Creation { .. } => WriteOpSize::Creation { write_len: materialized_size.expect("Creation must have size"), }, - Modification(_) | ModificationWithMetadata { .. } => { - WriteOpSize::Modification { - write_len: materialized_size.expect("Modification must have size"), - } + Modification { .. } => WriteOpSize::Modification { + write_len: materialized_size.expect("Modification must have size"), }, - Deletion | DeletionWithMetadata { .. } => WriteOpSize::Deletion, + Deletion { .. } => WriteOpSize::Deletion, } }, InPlaceDelayedFieldChange(InPlaceDelayedFieldChangeOp { @@ -80,7 +77,7 @@ impl AbstractResourceWriteOp { /// Deposit amount is inserted into metadata at a different time than the WriteOp is created. /// So this method is needed to be able to update metadata generically across different variants. - pub fn get_metadata_mut(&mut self) -> Option<&mut StateValueMetadata> { + pub fn get_metadata_mut(&mut self) -> &mut StateValueMetadata { use AbstractResourceWriteOp::*; match self { Write(write_op) @@ -89,7 +86,10 @@ impl AbstractResourceWriteOp { metadata_op: write_op, .. }) => write_op.get_metadata_mut(), - InPlaceDelayedFieldChange(_) | ResourceGroupInPlaceDelayedFieldChange(_) => None, + InPlaceDelayedFieldChange(_) | ResourceGroupInPlaceDelayedFieldChange(_) => { + // FIXME(aldenhu): confirm this is correct + unreachable!("No metadata for delayed field changes") + }, } } @@ -148,7 +148,10 @@ impl GroupWrite { metadata_op ); for (_tag, (v, _layout)) in &inner_ops { - assert_none!(v.metadata(), "Group inner ops must have no metadata"); + assert!( + v.metadata().is_dummy(), + "Group inner ops must have no metadata" + ) } let maybe_group_op_size = (!metadata_op.is_deletion()).then_some(group_size); diff --git a/aptos-move/aptos-vm-types/src/change_set.rs b/aptos-move/aptos-vm-types/src/change_set.rs index c00f45b22f6ef..cb8861d12258f 100644 --- a/aptos-move/aptos-vm-types/src/change_set.rs +++ b/aptos-move/aptos-vm-types/src/change_set.rs @@ -341,7 +341,7 @@ impl VMChangeSet { /// So this method is needed to be able to update metadata generically across different variants. pub fn write_set_iter_mut( &mut self, - ) -> impl Iterator)> { + ) -> impl Iterator { self.resource_write_set .iter_mut() .map(|(k, v)| (k, v.materialized_size(), v.get_metadata_mut())) @@ -477,10 +477,7 @@ 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) - | CreationWithMetadata { data, .. } - | ModificationWithMetadata { data, .. } => { + Creation { data, .. } | Modification { data, .. } => { // 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. @@ -492,7 +489,7 @@ impl VMChangeSet { .map_err(|e| e.finish(Location::Undefined).into_vm_status())?; *data = serialize(&value).into(); }, - Deletion | DeletionWithMetadata { .. } => { + 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/resolver.rs b/aptos-move/aptos-vm-types/src/resolver.rs index e43f973dcf416..5c737e3688889 100644 --- a/aptos-move/aptos-vm-types/src/resolver.rs +++ b/aptos-move/aptos-vm-types/src/resolver.rs @@ -10,7 +10,7 @@ use aptos_types::{ state_store::{ state_key::StateKey, state_storage_usage::StateStorageUsage, - state_value::{StateValue, StateValueMetadataKind}, + state_value::{StateValue, StateValueMetadata}, }, write_set::WriteOp, }; @@ -45,7 +45,7 @@ pub trait TResourceView { fn get_resource_state_value_metadata( &self, state_key: &Self::Key, - ) -> anyhow::Result> { + ) -> anyhow::Result> { // For metadata, layouts are not important. self.get_resource_state_value(state_key, None) .map(|maybe_state_value| maybe_state_value.map(StateValue::into_metadata)) @@ -151,7 +151,7 @@ pub trait TModuleView { fn get_module_state_value_metadata( &self, state_key: &Self::Key, - ) -> anyhow::Result> { + ) -> anyhow::Result> { let maybe_state_value = self.get_module_state_value(state_key)?; Ok(maybe_state_value.map(StateValue::into_metadata)) } @@ -275,13 +275,13 @@ pub trait StateValueMetadataResolver { fn get_module_state_value_metadata( &self, state_key: &StateKey, - ) -> anyhow::Result>; + ) -> anyhow::Result>; /// Can also be used to get the metadata of a resource group at a provided group key. fn get_resource_state_value_metadata( &self, state_key: &StateKey, - ) -> anyhow::Result>; + ) -> anyhow::Result>; } #[derive(Clone, Copy, Debug, Eq, PartialEq)] diff --git a/aptos-move/aptos-vm-types/src/storage/space_pricing.rs b/aptos-move/aptos-vm-types/src/storage/space_pricing.rs index c6fc92693f0ae..22d898cfe213f 100644 --- a/aptos-move/aptos-vm-types/src/storage/space_pricing.rs +++ b/aptos-move/aptos-vm-types/src/storage/space_pricing.rs @@ -71,7 +71,7 @@ impl DiskSpacePricing { params: &TransactionGasParameters, key: &StateKey, op_size: &WriteOpSize, - metadata: Option<&mut StateValueMetadata>, + metadata: &mut StateValueMetadata, ) -> ChargeAndRefund { match self { Self::V1 => Self::charge_refund_write_op_v1(params, key, op_size, metadata), @@ -160,7 +160,7 @@ impl DiskSpacePricing { params: &TransactionGasParameters, key: &StateKey, op_size: &WriteOpSize, - metadata: Option<&mut StateValueMetadata>, + metadata: &mut StateValueMetadata, ) -> ChargeAndRefund { use WriteOpSize::*; @@ -170,8 +170,8 @@ impl DiskSpacePricing { let bytes_fee = Self::discounted_write_op_size_for_v1(params, key, *write_len) * params.legacy_storage_fee_per_excess_state_byte; - if let Some(m) = metadata { - m.set_slot_deposit(slot_fee.into()) + if !metadata.is_dummy() { + metadata.set_slot_deposit(slot_fee.into()) } ChargeAndRefund { @@ -190,18 +190,10 @@ impl DiskSpacePricing { refund: 0.into(), } }, - Deletion => { - let refund = match metadata { - None => 0, - Some(m) => m.total_deposit(), - } - .into(); - - ChargeAndRefund { - non_discountable: 0.into(), - discountable: 0.into(), - refund, - } + Deletion => ChargeAndRefund { + non_discountable: 0.into(), + discountable: 0.into(), + refund: metadata.total_deposit().into(), }, } } @@ -210,7 +202,7 @@ impl DiskSpacePricing { params: &TransactionGasParameters, key: &StateKey, op_size: &WriteOpSize, - metadata: Option<&mut StateValueMetadata>, + metadata: &mut StateValueMetadata, ) -> ChargeAndRefund { use WriteOpSize::*; @@ -227,11 +219,9 @@ impl DiskSpacePricing { let slot_deposit = params.storage_fee_per_state_slot_refundable * NumSlots::new(1); let bytes_deposit = num_bytes * params.storage_fee_per_state_byte_refundable; - if let Some(m) = metadata { - m.set_deposits(slot_deposit.into(), bytes_deposit.into()) - } else { - // FIXME(aldenhu): this shouldn't happen - } + metadata.assert_v1(); + metadata.set_slot_deposit(slot_deposit.into()); + metadata.set_bytes_deposit(bytes_deposit.into()); ChargeAndRefund { non_discountable: slot_deposit + bytes_deposit, @@ -243,7 +233,7 @@ impl DiskSpacePricing { // change of slot size or per byte price can result in a charge or refund of permanent bytes fee let num_bytes = NumBytes::new(key.size() as u64) + NumBytes::new(*write_len); let target_bytes_deposit = num_bytes * params.storage_fee_per_state_byte_refundable; - let old_bytes_deposit = metadata.as_ref().map_or(0, |m| m.bytes_deposit()).into(); + let old_bytes_deposit = Fee::new(metadata.bytes_deposit()); let (state_bytes_charge, state_bytes_refund) = if target_bytes_deposit > old_bytes_deposit { @@ -255,10 +245,9 @@ impl DiskSpacePricing { (0.into(), bytes_refund) }; - // FIXME(aldenhu): upgrade to new format automatically, otherwise old slots will be heavily punished whenever modified. - if let Some(m) = metadata { - m.set_bytes_deposit(target_bytes_deposit.into()) - } + metadata + .upgrade() + .set_bytes_deposit(target_bytes_deposit.into()); ChargeAndRefund { non_discountable: state_bytes_charge, @@ -266,18 +255,10 @@ impl DiskSpacePricing { refund: state_bytes_refund, } }, - Deletion => { - let refund = match metadata { - None => 0, - Some(m) => m.total_deposit(), - } - .into(); - - ChargeAndRefund { - non_discountable: 0.into(), - discountable, - refund, - } + Deletion => ChargeAndRefund { + non_discountable: 0.into(), + discountable, + refund: metadata.total_deposit().into(), }, } } 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 d78f8c8f8c34b..583ae6c484385 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 @@ -385,8 +385,8 @@ fn test_roundtrip_to_storage_change_set() { ); let module_key = StateKey::access_path(AccessPath::code_access_path(test_module_id)); let write_set = WriteSetMut::new(vec![ - (resource_key, WriteOp::Deletion), - (module_key, WriteOp::Deletion), + (resource_key, WriteOp::legacy_deletion()), + (module_key, WriteOp::legacy_deletion()), ]) .freeze() .unwrap(); @@ -502,19 +502,19 @@ fn test_aggregator_v2_snapshots_and_derived() { #[test] fn test_resource_groups_squashing() { - let modification_metadata = WriteOp::ModificationWithMetadata { + let modification_metadata = WriteOp::Modification { data: Bytes::new(), metadata: raw_metadata(2000), }; macro_rules! as_create_op { ($val:expr) => { - (WriteOp::Creation(as_bytes!($val).into()), None) + (WriteOp::legacy_creation(as_bytes!($val).into()), None) }; } macro_rules! as_modify_op { ($val:expr) => { - (WriteOp::Modification(as_bytes!($val).into()), None) + (WriteOp::legacy_modification(as_bytes!($val).into()), None) }; } @@ -649,18 +649,18 @@ fn test_write_and_read_discrepancy_caught() { assert_err!(ExpandedVMChangeSetBuilder::new() .with_resource_write_set(vec![( as_state_key!("1"), - (WriteOp::Modification(as_bytes!(1).into()), None), + (WriteOp::legacy_modification(as_bytes!(1).into()), None), )]) .with_reads_needing_delayed_field_exchange(vec![( as_state_key!("1"), ( - WriteOp::Modification(as_bytes!(1).into()), + WriteOp::legacy_modification(as_bytes!(1).into()), Arc::new(MoveTypeLayout::U64) ) )]) .try_build()); - let metadata_op = WriteOp::ModificationWithMetadata { + let metadata_op = WriteOp::Modification { data: Bytes::new(), metadata: raw_metadata(1000), }; @@ -696,15 +696,15 @@ mod tests { pub(crate) fn write_op_with_metadata(type_idx: u8, v: u128) -> WriteOp { match type_idx { - CREATION => WriteOp::CreationWithMetadata { + CREATION => WriteOp::Creation { data: vec![].into(), metadata: raw_metadata(v as u64), }, - MODIFICATION => WriteOp::ModificationWithMetadata { + MODIFICATION => WriteOp::Modification { data: vec![].into(), metadata: raw_metadata(v as u64), }, - DELETION => WriteOp::DeletionWithMetadata { + DELETION => WriteOp::Deletion { metadata: raw_metadata(v as u64), }, _ => unreachable!("Wrong type index for test"), @@ -741,9 +741,9 @@ mod tests { #[test] fn test_group_write_size() { // Deletions should lead to size 0. - assert_group_write_size!(WriteOp::Deletion, 0, None); + assert_group_write_size!(WriteOp::legacy_deletion(), 0, None); assert_group_write_size!( - WriteOp::DeletionWithMetadata { + WriteOp::Deletion { metadata: raw_metadata(10) }, 0, @@ -751,9 +751,13 @@ mod tests { ); let sizes = [20, 100, 45279432, 5]; - assert_group_write_size!(WriteOp::Creation(Bytes::new()), sizes[0], Some(sizes[0])); assert_group_write_size!( - WriteOp::CreationWithMetadata { + WriteOp::legacy_creation(Bytes::new()), + sizes[0], + Some(sizes[0]) + ); + assert_group_write_size!( + WriteOp::Creation { data: Bytes::new(), metadata: raw_metadata(20) }, @@ -761,12 +765,12 @@ mod tests { Some(sizes[1]) ); assert_group_write_size!( - WriteOp::Modification(Bytes::new()), + WriteOp::legacy_modification(Bytes::new()), sizes[2], Some(sizes[2]) ); assert_group_write_size!( - WriteOp::ModificationWithMetadata { + WriteOp::Modification { data: Bytes::new(), metadata: raw_metadata(30) }, @@ -797,13 +801,13 @@ mod tests { )); assert_eq!(base_update.len(), 2); - assert_some_eq!( + assert_eq!( extract_group_op(base_update.get(&key_1).unwrap()) .metadata_op .metadata(), &raw_metadata(100) ); - assert_some_eq!( + assert_eq!( extract_group_op(base_update.get(&key_2).unwrap()) .metadata_op .metadata(), @@ -835,7 +839,7 @@ mod tests { )); assert_eq!(base_update.len(), 1); - assert_some_eq!( + assert_eq!( extract_group_op(base_update.get(&key).unwrap()) .metadata_op .metadata(), @@ -911,8 +915,14 @@ mod tests { group_write( write_op_with_metadata(MODIFICATION, 100), vec![ - (mock_tag_0(), (WriteOp::Creation(vec![100].into()), None)), - (mock_tag_2(), (WriteOp::Modification(vec![2].into()), None)), + ( + mock_tag_0(), + (WriteOp::legacy_creation(vec![100].into()), None), + ), + ( + mock_tag_2(), + (WriteOp::legacy_modification(vec![2].into()), None), + ), ], 0, ), @@ -922,8 +932,14 @@ mod tests { group_write( write_op_with_metadata(MODIFICATION, 200), vec![ - (mock_tag_0(), (WriteOp::Modification(vec![0].into()), None)), - (mock_tag_1(), (WriteOp::Modification(vec![1].into()), None)), + ( + mock_tag_0(), + (WriteOp::legacy_modification(vec![0].into()), None), + ), + ( + mock_tag_1(), + (WriteOp::legacy_modification(vec![1].into()), None), + ), ], 0, ), @@ -934,9 +950,15 @@ mod tests { group_write( write_op_with_metadata(MODIFICATION, 100), vec![ - (mock_tag_0(), (WriteOp::Deletion, None)), - (mock_tag_1(), (WriteOp::Modification(vec![2].into()), None)), - (mock_tag_2(), (WriteOp::Creation(vec![2].into()), None)), + (mock_tag_0(), (WriteOp::legacy_deletion(), None)), + ( + mock_tag_1(), + (WriteOp::legacy_modification(vec![2].into()), None), + ), + ( + mock_tag_2(), + (WriteOp::legacy_creation(vec![2].into()), None), + ), ], 0, ), @@ -946,9 +968,12 @@ mod tests { group_write( write_op_with_metadata(MODIFICATION, 200), vec![ - (mock_tag_0(), (WriteOp::Creation(vec![0].into()), None)), - (mock_tag_1(), (WriteOp::Deletion, None)), - (mock_tag_2(), (WriteOp::Deletion, None)), + ( + mock_tag_0(), + (WriteOp::legacy_creation(vec![0].into()), None), + ), + (mock_tag_1(), (WriteOp::legacy_deletion(), None)), + (mock_tag_2(), (WriteOp::legacy_deletion(), None)), ], 0, ), @@ -963,29 +988,32 @@ mod tests { assert_eq!(inner_ops_1.len(), 3); assert_some_eq!( inner_ops_1.get(&mock_tag_0()), - &(WriteOp::Creation(vec![0].into()), None) + &(WriteOp::legacy_creation(vec![0].into()), None) ); assert_some_eq!( inner_ops_1.get(&mock_tag_1()), - &(WriteOp::Modification(vec![1].into()), None) + &(WriteOp::legacy_modification(vec![1].into()), None) ); assert_some_eq!( inner_ops_1.get(&mock_tag_2()), - &(WriteOp::Modification(vec![2].into()), None) + &(WriteOp::legacy_modification(vec![2].into()), None) ); let inner_ops_2 = &extract_group_op(base_update.get(&key_2).unwrap()).inner_ops; assert_eq!(inner_ops_2.len(), 2); assert_some_eq!( inner_ops_2.get(&mock_tag_0()), - &(WriteOp::Modification(vec![0].into()), None) + &(WriteOp::legacy_modification(vec![0].into()), None) + ); + assert_some_eq!( + inner_ops_2.get(&mock_tag_1()), + &(WriteOp::legacy_deletion(), None) ); - assert_some_eq!(inner_ops_2.get(&mock_tag_1()), &(WriteOp::Deletion, None)); let additional_update = BTreeMap::from([( key_2.clone(), group_write( write_op_with_metadata(MODIFICATION, 200), - vec![(mock_tag_1(), (WriteOp::Deletion, None))], + vec![(mock_tag_1(), (WriteOp::legacy_deletion(), None))], 0, ), )]); diff --git a/aptos-move/aptos-vm-types/src/tests/utils.rs b/aptos-move/aptos-vm-types/src/tests/utils.rs index 0fbb68c20786c..5be5e62d329a7 100644 --- a/aptos-move/aptos-vm-types/src/tests/utils.rs +++ b/aptos-move/aptos-vm-types/src/tests/utils.rs @@ -63,15 +63,21 @@ pub(crate) fn raw_metadata(v: u64) -> StateValueMetadata { } pub(crate) fn mock_create(k: impl ToString, v: u128) -> (StateKey, WriteOp) { - (as_state_key!(k), WriteOp::Creation(as_bytes!(v).into())) + ( + as_state_key!(k), + WriteOp::legacy_creation(as_bytes!(v).into()), + ) } pub(crate) fn mock_modify(k: impl ToString, v: u128) -> (StateKey, WriteOp) { - (as_state_key!(k), WriteOp::Modification(as_bytes!(v).into())) + ( + as_state_key!(k), + WriteOp::legacy_modification(as_bytes!(v).into()), + ) } pub(crate) fn mock_delete(k: impl ToString) -> (StateKey, WriteOp) { - (as_state_key!(k), WriteOp::Deletion) + (as_state_key!(k), WriteOp::legacy_deletion()) } pub(crate) fn mock_create_with_layout( @@ -82,7 +88,7 @@ pub(crate) fn mock_create_with_layout( ( as_state_key!(k), AbstractResourceWriteOp::from_resource_write_with_maybe_layout( - WriteOp::Creation(as_bytes!(v).into()), + WriteOp::legacy_creation(as_bytes!(v).into()), layout, ), ) @@ -96,7 +102,7 @@ pub(crate) fn mock_modify_with_layout( ( as_state_key!(k), AbstractResourceWriteOp::from_resource_write_with_maybe_layout( - WriteOp::Modification(as_bytes!(v).into()), + WriteOp::legacy_modification(as_bytes!(v).into()), layout, ), ) @@ -105,7 +111,10 @@ pub(crate) fn mock_modify_with_layout( pub(crate) fn mock_delete_with_layout(k: impl ToString) -> (StateKey, AbstractResourceWriteOp) { ( as_state_key!(k), - AbstractResourceWriteOp::from_resource_write_with_maybe_layout(WriteOp::Deletion, None), + AbstractResourceWriteOp::from_resource_write_with_maybe_layout( + WriteOp::legacy_deletion(), + None, + ), ) } diff --git a/aptos-move/aptos-vm/src/data_cache.rs b/aptos-move/aptos-vm/src/data_cache.rs index b06db54853edd..02f09117d577b 100644 --- a/aptos-move/aptos-vm/src/data_cache.rs +++ b/aptos-move/aptos-vm/src/data_cache.rs @@ -26,7 +26,7 @@ use aptos_types::{ state_store::{ state_key::StateKey, state_storage_usage::StateStorageUsage, - state_value::{StateValue, StateValueMetadataKind}, + state_value::{StateValue, StateValueMetadata}, }, write_set::WriteOp, }; @@ -389,7 +389,7 @@ impl<'e, E: ExecutorView> StateValueMetadataResolver for StorageAdapter<'e, E> { fn get_module_state_value_metadata( &self, state_key: &StateKey, - ) -> anyhow::Result> { + ) -> anyhow::Result> { self.executor_view .get_module_state_value_metadata(state_key) } @@ -397,7 +397,7 @@ impl<'e, E: ExecutorView> StateValueMetadataResolver for StorageAdapter<'e, E> { fn get_resource_state_value_metadata( &self, state_key: &StateKey, - ) -> anyhow::Result> { + ) -> anyhow::Result> { self.executor_view .get_resource_state_value_metadata(state_key) } diff --git a/aptos-move/aptos-vm/src/move_vm_ext/respawned_session.rs b/aptos-move/aptos-vm/src/move_vm_ext/respawned_session.rs index 809377af213c5..0084a9083baf1 100644 --- a/aptos-move/aptos-vm/src/move_vm_ext/respawned_session.rs +++ b/aptos-move/aptos-vm/src/move_vm_ext/respawned_session.rs @@ -23,7 +23,7 @@ use aptos_types::{ state_store::{ state_key::StateKey, state_storage_usage::StateStorageUsage, - state_value::{StateValue, StateValueMetadataKind}, + state_value::{StateValue, StateValueMetadata}, }, write_set::{TransactionWrite, WriteOp}, }; @@ -357,7 +357,7 @@ impl<'r> TResourceView for ExecutorViewWithChangeSet<'r> { fn get_resource_state_value_metadata( &self, state_key: &Self::Key, - ) -> anyhow::Result> { + ) -> anyhow::Result> { match self.change_set.resource_write_set().get(state_key) { Some( AbstractResourceWriteOp::Write(write_op) @@ -487,7 +487,7 @@ mod test { } fn write(v: u128) -> WriteOp { - WriteOp::Modification(serialize(&v).into()) + WriteOp::legacy_modification(serialize(&v).into()) } fn read_resource(view: &ExecutorViewWithChangeSet, s: impl ToString) -> u128 { @@ -586,15 +586,15 @@ mod test { ( key("resource_group_both"), GroupWrite::new( - WriteOp::Deletion, + WriteOp::legacy_deletion(), vec![ ( mock_tag_0(), - (WriteOp::Modification(serialize(&1000).into()), None), + (WriteOp::legacy_modification(serialize(&1000).into()), None), ), ( mock_tag_2(), - (WriteOp::Modification(serialize(&300).into()), None), + (WriteOp::legacy_modification(serialize(&300).into()), None), ), ], 0, @@ -603,10 +603,10 @@ mod test { ( key("resource_group_write_set"), GroupWrite::new( - WriteOp::Deletion, + WriteOp::legacy_deletion(), vec![( mock_tag_1(), - (WriteOp::Modification(serialize(&5000).into()), None), + (WriteOp::legacy_modification(serialize(&5000).into()), None), )], 0, ), 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 823c40df58ee2..7e4facec53cd1 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 @@ -5,10 +5,7 @@ use crate::move_vm_ext::{session::BytesWithResourceLayout, AptosMoveResolver}; use aptos_aggregator::delta_change_set::serialize; use aptos_types::{ on_chain_config::{CurrentTimeMicroseconds, OnChainConfig}, - state_store::{ - state_key::StateKey, - state_value::{StateValueMetadata, StateValueMetadataKind}, - }, + state_store::{state_key::StateKey, state_value::StateValueMetadata}, write_set::WriteOp, }; use aptos_vm_types::{ @@ -247,11 +244,13 @@ impl<'r> WriteOpConverter<'r> { }; let legacy_op = match current_op { - MoveStorageOp::Delete => (WriteOp::Deletion, None), + MoveStorageOp::Delete => (WriteOp::legacy_deletion(), None), MoveStorageOp::Modify((data, maybe_layout)) => { - (WriteOp::Modification(data), maybe_layout) + (WriteOp::legacy_modification(data), maybe_layout) + }, + MoveStorageOp::New((data, maybe_layout)) => { + (WriteOp::legacy_creation(data), maybe_layout) }, - MoveStorageOp::New((data, maybe_layout)) => (WriteOp::Creation(data), maybe_layout), }; inner_ops.insert(tag, legacy_op); } @@ -279,7 +278,7 @@ impl<'r> WriteOpConverter<'r> { fn convert( &self, - state_value_metadata_result: anyhow::Result>, + state_value_metadata_result: anyhow::Result>, move_storage_op: MoveStorageOp, legacy_creation_as_modification: bool, ) -> Result { @@ -311,28 +310,27 @@ impl<'r> WriteOpConverter<'r> { (None, New((data, _))) => match &self.new_slot_metadata { None => { if legacy_creation_as_modification { - Modification(data) + WriteOp::legacy_modification(data) } else { - Creation(data) + WriteOp::legacy_creation(data) } }, - Some(metadata) => CreationWithMetadata { + Some(metadata) => Creation { data, metadata: metadata.clone(), }, }, (Some(existing_metadata), Modify((data, _))) => { // Inherit metadata even if the feature flags is turned off, for compatibility. - match existing_metadata { - None => Modification(data), - Some(metadata) => ModificationWithMetadata { data, metadata }, + Modification { + data, + metadata: existing_metadata, } }, (Some(existing_metadata), Delete) => { // Inherit metadata even if the feature flags is turned off, for compatibility. - match existing_metadata { - None => Deletion, - Some(metadata) => DeletionWithMetadata { metadata }, + Deletion { + metadata: existing_metadata, } }, }; @@ -359,17 +357,14 @@ impl<'r> WriteOpConverter<'r> { None => { match &self.new_slot_metadata { // n.b. Aggregator writes historically did not distinguish Create vs Modify. - None => WriteOp::Modification(data), - Some(metadata) => WriteOp::CreationWithMetadata { + None => WriteOp::legacy_modification(data), + Some(metadata) => WriteOp::Creation { data, metadata: metadata.clone(), }, } }, - Some(existing_metadata) => match existing_metadata { - None => WriteOp::Modification(data), - Some(metadata) => WriteOp::ModificationWithMetadata { data, metadata }, - }, + Some(metadata) => WriteOp::Modification { data, metadata }, }; Ok(op) @@ -488,7 +483,7 @@ mod tests { .convert_resource_group_v1(&key, group_changes) .unwrap(); - assert_eq!(group_write.metadata_op().metadata(), Some(&metadata)); + assert_eq!(group_write.metadata_op().metadata(), &metadata); let expected_new_size = bcs::serialized_size(&mock_tag_1()).unwrap() + bcs::serialized_size(&mock_tag_2()).unwrap() + 7; // values bytes size: 2 + 5 @@ -496,11 +491,14 @@ mod tests { assert_eq!(group_write.inner_ops().len(), 2); assert_some_eq!( group_write.inner_ops().get(&mock_tag_0()), - &(WriteOp::Deletion, None) + &(WriteOp::legacy_deletion(), None) ); assert_some_eq!( group_write.inner_ops().get(&mock_tag_2()), - &(WriteOp::Modification(vec![5, 5, 5, 5, 5].into()), None) + &( + WriteOp::legacy_modification(vec![5, 5, 5, 5, 5].into()), + None + ) ); } @@ -532,7 +530,7 @@ mod tests { .convert_resource_group_v1(&key, group_changes) .unwrap(); - assert_eq!(group_write.metadata_op().metadata(), Some(&metadata)); + assert_eq!(group_write.metadata_op().metadata(), &metadata); let expected_new_size = bcs::serialized_size(&mock_tag_0()).unwrap() + bcs::serialized_size(&mock_tag_1()).unwrap() + bcs::serialized_size(&mock_tag_2()).unwrap() @@ -541,7 +539,7 @@ mod tests { assert_eq!(group_write.inner_ops().len(), 1); assert_some_eq!( group_write.inner_ops().get(&mock_tag_2()), - &(WriteOp::Creation(vec![3, 3, 3].into()), None) + &(WriteOp::legacy_creation(vec![3, 3, 3].into()), None) ); } @@ -561,13 +559,13 @@ mod tests { .convert_resource_group_v1(&key, group_changes) .unwrap(); - assert_none!(group_write.metadata_op().metadata()); + assert!(group_write.metadata_op().metadata().is_dummy()); let expected_new_size = bcs::serialized_size(&mock_tag_1()).unwrap() + 2; assert_some_eq!(group_write.maybe_group_op_size(), expected_new_size as u64); assert_eq!(group_write.inner_ops().len(), 1); assert_some_eq!( group_write.inner_ops().get(&mock_tag_1()), - &(WriteOp::Creation(vec![2, 2].into()), None) + &(WriteOp::legacy_creation(vec![2, 2].into()), None) ); } @@ -599,10 +597,8 @@ mod tests { .unwrap(); // Deletion should still contain the metadata - for storage refunds. - assert_eq!(group_write.metadata_op().metadata(), Some(&metadata)); - assert_eq!(group_write.metadata_op(), &WriteOp::DeletionWithMetadata { - metadata - }); + assert_eq!(group_write.metadata_op().metadata(), &metadata); + assert_eq!(group_write.metadata_op(), &WriteOp::Deletion { metadata }); assert_none!(group_write.metadata_op().bytes()); } } diff --git a/aptos-move/block-executor/src/captured_reads.rs b/aptos-move/block-executor/src/captured_reads.rs index 63622b60f9d60..59f113e0a349a 100644 --- a/aptos-move/block-executor/src/captured_reads.rs +++ b/aptos-move/block-executor/src/captured_reads.rs @@ -19,7 +19,7 @@ use aptos_mvhashmap::{ versioned_group_data::VersionedGroupData, }; use aptos_types::{ - aggregator::PanicError, state_store::state_value::StateValueMetadataKind, + aggregator::PanicError, state_store::state_value::StateValueMetadata, transaction::BlockExecutableTransaction as Transaction, write_set::TransactionWrite, }; use aptos_vm_types::resolver::ResourceGroupSize; @@ -62,7 +62,7 @@ pub(crate) enum DataRead { #[derivative(PartialEq = "ignore", Debug = "ignore")] Arc, Option>, ), - Metadata(Option), + Metadata(Option), Exists(bool), /// Read resolved an aggregatorV1 delta to a value. /// TODO[agg_v1](cleanup): deprecate. @@ -137,7 +137,9 @@ impl DataRead { DataRead::Metadata(v.as_state_value_metadata()) }, (DataRead::Versioned(_, v, _), ReadKind::Exists) => DataRead::Exists(!v.is_deletion()), - (DataRead::Resolved(_), ReadKind::Metadata) => DataRead::Metadata(Some(None)), + (DataRead::Resolved(_), ReadKind::Metadata) => { + DataRead::Metadata(Some(StateValueMetadata::Dummy)) + }, (DataRead::Resolved(_), ReadKind::Exists) => DataRead::Exists(true), (DataRead::Metadata(maybe_metadata), ReadKind::Exists) => { DataRead::Exists(maybe_metadata.is_some()) @@ -691,7 +693,10 @@ mod test { assert_eq!( DataRead::Versioned( Err(StorageVersion), - Arc::new(ValueType::with_len_and_metadata(1, None)), + Arc::new(ValueType::with_len_and_metadata( + 1, + StateValueMetadata::Dummy + )), None, ) .get_kind(), @@ -702,7 +707,7 @@ mod test { ReadKind::Value ); assert_eq!( - DataRead::Metadata::(Some(None)).get_kind(), + DataRead::Metadata::(Some(StateValueMetadata::Dummy)).get_kind(), ReadKind::Metadata ); assert_eq!( @@ -756,12 +761,18 @@ mod test { // Legacy state values do not have metadata. let versioned_legacy = DataRead::Versioned( Err(StorageVersion), - Arc::new(ValueType::with_len_and_metadata(1, None)), + Arc::new(ValueType::with_len_and_metadata( + 1, + StateValueMetadata::Dummy, + )), None, ); let versioned_deletion = DataRead::Versioned( Ok((5, 1)), - Arc::new(ValueType::with_len_and_metadata(0, None)), + Arc::new(ValueType::with_len_and_metadata( + 0, + StateValueMetadata::Dummy, + )), None, ); let versioned_with_metadata = DataRead::Versioned( @@ -771,7 +782,7 @@ mod test { ); let resolved = DataRead::Resolved::(200); let deletion_metadata = DataRead::Metadata(None); - let legacy_metadata = DataRead::Metadata(Some(None)); + let legacy_metadata = DataRead::Metadata(Some(StateValueMetadata::Dummy)); let metadata = DataRead::Metadata(Some(raw_metadata(1))); let exists = DataRead::Exists(true); let not_exists = DataRead::Exists(false); @@ -832,7 +843,10 @@ mod test { versioned_legacy, DataRead::Versioned( Err(StorageVersion), - Arc::new(ValueType::with_len_and_metadata(10, None)), + Arc::new(ValueType::with_len_and_metadata( + 10, + StateValueMetadata::Dummy + )), None, ) ); @@ -896,12 +910,18 @@ mod test { // Legacy state values do not have metadata. let versioned_legacy = DataRead::Versioned( Err(StorageVersion), - Arc::new(ValueType::with_len_and_metadata(1, None)), + Arc::new(ValueType::with_len_and_metadata( + 1, + StateValueMetadata::Dummy, + )), None, ); let versioned_deletion = DataRead::Versioned( Ok((5, 1)), - Arc::new(ValueType::with_len_and_metadata(0, None)), + Arc::new(ValueType::with_len_and_metadata( + 0, + StateValueMetadata::Dummy, + )), None, ); let versioned_with_metadata = DataRead::Versioned( @@ -911,7 +931,7 @@ mod test { ); let resolved = DataRead::Resolved::(200); let deletion_metadata = DataRead::Metadata(None); - let legacy_metadata = DataRead::Metadata(Some(None)); + let legacy_metadata = DataRead::Metadata(Some(StateValueMetadata::Dummy)); let metadata = DataRead::Metadata(Some(raw_metadata(1))); let exists = DataRead::Exists(true); let not_exists = DataRead::Exists(false); @@ -979,10 +999,13 @@ mod test { fn legacy_reads_by_kind() -> Vec> { let exists = DataRead::Exists(true); - let legacy_metadata = DataRead::Metadata(Some(None)); + let legacy_metadata = DataRead::Metadata(Some(StateValueMetadata::Dummy)); let versioned_legacy = DataRead::Versioned( Err(StorageVersion), - Arc::new(ValueType::with_len_and_metadata(1, None)), + Arc::new(ValueType::with_len_and_metadata( + 1, + StateValueMetadata::Dummy, + )), None, ); vec![exists, legacy_metadata, versioned_legacy] @@ -991,7 +1014,10 @@ mod test { fn deletion_reads_by_kind() -> Vec> { let versioned_deletion = DataRead::Versioned( Ok((5, 1)), - Arc::new(ValueType::with_len_and_metadata(0, None)), + Arc::new(ValueType::with_len_and_metadata( + 0, + StateValueMetadata::Dummy, + )), None, ); let deletion_metadata = DataRead::Metadata(None); @@ -1148,7 +1174,10 @@ mod test { let mut captured_reads = CapturedReads::::new(); let versioned_legacy = DataRead::Versioned( Err(StorageVersion), - Arc::new(ValueType::with_len_and_metadata(1, None)), + Arc::new(ValueType::with_len_and_metadata( + 1, + StateValueMetadata::Dummy, + )), None, ); let resolved = DataRead::Resolved::(200); diff --git a/aptos-move/block-executor/src/executor.rs b/aptos-move/block-executor/src/executor.rs index 5d0b4aac8cfee..b2796c2a0eaa7 100644 --- a/aptos-move/block-executor/src/executor.rs +++ b/aptos-move/block-executor/src/executor.rs @@ -806,8 +806,10 @@ where }); // Must contain committed value as we set the base value above. - aggregator_v1_delta_writes - .push((k, WriteOp::Modification(serialize(&committed_delta).into()))); + aggregator_v1_delta_writes.push(( + k, + WriteOp::legacy_modification(serialize(&committed_delta).into()), + )); } aggregator_v1_delta_writes } diff --git a/aptos-move/block-executor/src/proptest_types/types.rs b/aptos-move/block-executor/src/proptest_types/types.rs index 5d4d1b508e4dd..a86012013b233 100644 --- a/aptos-move/block-executor/src/proptest_types/types.rs +++ b/aptos-move/block-executor/src/proptest_types/types.rs @@ -23,7 +23,7 @@ use aptos_types::{ on_chain_config::CurrentTimeMicroseconds, state_store::{ state_storage_usage::StateStorageUsage, - state_value::{StateValue, StateValueMetadata, StateValueMetadataKind}, + state_value::{StateValue, StateValueMetadata}, }, transaction::BlockExecutableTransaction as Transaction, write_set::{TransactionWrite, WriteOp, WriteOpKind}, @@ -174,7 +174,7 @@ impl ModulePath for KeyType pub(crate) struct ValueType { /// Wrapping the types used for testing to add TransactionWrite trait implementation (below). bytes: Option, - metadata: StateValueMetadataKind, + metadata: StateValueMetadata, write_op_kind: ExplicitSyncWrapper, } @@ -206,7 +206,7 @@ impl Arbitrary for ValueType { impl ValueType { pub(crate) fn new( bytes: Option, - metadata: StateValueMetadataKind, + metadata: StateValueMetadata, kind: WriteOpKind, ) -> Self { Self { @@ -229,7 +229,7 @@ impl ValueType { v.resize(16, 1); v.into() }), - metadata: None, + metadata: StateValueMetadata::Dummy, write_op_kind: ExplicitSyncWrapper::new( if !use_value { WriteOpKind::Deletion @@ -241,7 +241,7 @@ impl ValueType { } /// If len = 0, treated as Deletion for testing. - pub(crate) fn with_len_and_metadata(len: usize, metadata: StateValueMetadataKind) -> Self { + pub(crate) fn with_len_and_metadata(len: usize, metadata: StateValueMetadata) -> Self { Self { bytes: (len > 0).then_some(vec![100_u8; len].into()), metadata, @@ -263,9 +263,9 @@ impl TransactionWrite for ValueType { fn from_state_value(maybe_state_value: Option) -> Self { let (maybe_metadata, maybe_bytes) = - match maybe_state_value.map(|state_value| state_value.into()) { + match maybe_state_value.map(|state_value| state_value.into_parts()) { Some((maybe_metadata, bytes)) => (maybe_metadata, Some(bytes)), - None => (None, None), + None => (StateValueMetadata::Dummy, None), }; let empty = maybe_bytes.is_none(); @@ -288,10 +288,8 @@ impl TransactionWrite for ValueType { } fn as_state_value(&self) -> Option { - self.extract_raw_bytes().map(|bytes| match &self.metadata { - Some(metadata) => StateValue::new_with_metadata(bytes, metadata.clone()), - None => StateValue::new_legacy(bytes), - }) + self.extract_raw_bytes() + .map(|bytes| StateValue::new_with_metadata(bytes, self.metadata.clone())) } fn set_bytes(&mut self, bytes: Bytes) { @@ -928,7 +926,11 @@ where } else { new_inner_ops.insert( *tag, - ValueType::new(None, None, WriteOpKind::Deletion), + ValueType::new( + None, + StateValueMetadata::Dummy, + WriteOpKind::Deletion, + ), ); } } @@ -970,10 +972,8 @@ where } } -pub(crate) fn raw_metadata(v: u64) -> StateValueMetadataKind { - Some(StateValueMetadata::new_v0(v, &CurrentTimeMicroseconds { - microseconds: v, - })) +pub(crate) fn raw_metadata(v: u64) -> StateValueMetadata { + StateValueMetadata::new_v0(v, &CurrentTimeMicroseconds { microseconds: v }) } #[derive(Debug)] diff --git a/aptos-move/block-executor/src/view.rs b/aptos-move/block-executor/src/view.rs index 912aa067a1845..da9e218f887d8 100644 --- a/aptos-move/block-executor/src/view.rs +++ b/aptos-move/block-executor/src/view.rs @@ -36,7 +36,7 @@ use aptos_types::{ executable::{Executable, ModulePath}, state_store::{ state_storage_usage::StateStorageUsage, - state_value::{StateValue, StateValueMetadataKind}, + state_value::{StateValue, StateValueMetadata}, }, transaction::BlockExecutableTransaction as Transaction, write_set::TransactionWrite, @@ -73,7 +73,7 @@ use std::{ #[derive(Debug)] pub(crate) enum ReadResult { Value(Option, Option>), - Metadata(Option), + Metadata(Option), Exists(bool), Uninitialized, // Must halt the execution of the calling transaction. This might be because @@ -1218,17 +1218,12 @@ impl<'a, T: Transaction, S: TStateView, X: Executable> LatestView< return None; } - if let Ok(Some(maybe_metadata)) = self.get_resource_state_value_metadata(&key) { - let maybe_metadata = maybe_metadata.map( - |metadata: aptos_types::state_store::state_value::StateValueMetadata| { - StateValue::new_with_metadata(Bytes::new(), metadata) - }, - ); + if let Ok(Some(metadata)) = self.get_resource_state_value_metadata(&key) { + let metadata = Some(StateValue::new_with_metadata(Bytes::new(), metadata)); if let Ok(GroupReadResult::Size(group_size)) = parallel_state.read_group_size(&key, self.txn_idx) { - let metadata_op: T::Value = - TransactionWrite::from_state_value(maybe_metadata); + let metadata_op: T::Value = TransactionWrite::from_state_value(metadata); if let Some(metadata_op) = metadata_op.convert_read_to_modification() { return Some(Ok((key.clone(), (metadata_op, group_size.get())))); } @@ -1420,7 +1415,7 @@ impl<'a, T: Transaction, S: TStateView, X: Executable> TResourceVi fn get_resource_state_value_metadata( &self, state_key: &Self::Key, - ) -> anyhow::Result> { + ) -> anyhow::Result> { self.get_resource_state_value_impl(state_key, UnknownOrLayout::Unknown, ReadKind::Metadata) .map(|res| { if let ReadResult::Metadata(v) = res { diff --git a/aptos-move/e2e-move-tests/src/harness.rs b/aptos-move/e2e-move-tests/src/harness.rs index 528fd5354828d..6c7bc0c5db063 100644 --- a/aptos-move/e2e-move-tests/src/harness.rs +++ b/aptos-move/e2e-move-tests/src/harness.rs @@ -562,7 +562,7 @@ impl MoveHarness { &self, addr: &AccountAddress, struct_tag: StructTag, - ) -> Option> { + ) -> Option { self.read_state_value(&StateKey::access_path( AccessPath::resource_access_path(*addr, struct_tag).expect("access path in test"), )) diff --git a/aptos-move/e2e-move-tests/src/tests/state_metadata.rs b/aptos-move/e2e-move-tests/src/tests/state_metadata.rs index 7f94932043a69..a7d041797d6ad 100644 --- a/aptos-move/e2e-move-tests/src/tests/state_metadata.rs +++ b/aptos-move/e2e-move-tests/src/tests/state_metadata.rs @@ -35,7 +35,7 @@ fn test_metadata_tracking() { // Observe that metadata is not tracked for address2 resources assert_eq!( harness.read_resource_metadata(&address2, coin_store.clone()), - Some(None), + Some(StateValueMetadata::Dummy), ); // Enable storage slot metadata tracking @@ -59,7 +59,7 @@ fn test_metadata_tracking() { // Observe that metadata is tracked for address3 resources assert_eq!( harness.read_resource_metadata(&address3, coin_store.clone()), - Some(Some(StateValueMetadata::new_v0(slot_fee, ×tamp,))), + Some(StateValueMetadata::new_v0(slot_fee, ×tamp,)), ); // Bump the timestamp and modify the resources, observe that metadata doesn't change. @@ -74,10 +74,10 @@ fn test_metadata_tracking() { ); assert_eq!( harness.read_resource_metadata(&address2, coin_store.clone()), - Some(None), + Some(StateValueMetadata::Dummy), ); assert_eq!( harness.read_resource_metadata(&address3, coin_store), - Some(Some(StateValueMetadata::new_v0(slot_fee, ×tamp))), + Some(StateValueMetadata::new_v0(slot_fee, ×tamp)), ); } 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 e92836aaea1a7..1723a0afc2401 100644 --- a/aptos-move/e2e-move-tests/src/tests/storage_refund.rs +++ b/aptos-move/e2e-move-tests/src/tests/storage_refund.rs @@ -153,10 +153,9 @@ fn assert_result( let mut deletes = 0; for (_state_key, write_op) in txn_out.write_set() { match write_op { - WriteOp::CreationWithMetadata { .. } | WriteOp::Creation(_) => creates += 1, - WriteOp::DeletionWithMetadata { .. } => deletes += 1, - WriteOp::Deletion => panic!("This test expects all deletions to have metadata"), - WriteOp::Modification(_) | WriteOp::ModificationWithMetadata { .. } => (), + WriteOp::Creation { .. } => creates += 1, + WriteOp::Deletion { .. } => deletes += 1, + WriteOp::Modification { .. } => (), } } if expect_success { diff --git a/aptos-move/e2e-tests/src/account.rs b/aptos-move/e2e-tests/src/account.rs index 686d87d53f85f..71fdf07d3fc82 100644 --- a/aptos-move/e2e-tests/src/account.rs +++ b/aptos-move/e2e-tests/src/account.rs @@ -480,11 +480,11 @@ impl AccountData { let write_set = vec![ ( StateKey::access_path(self.make_account_access_path()), - WriteOp::Modification(self.to_bytes().into()), + WriteOp::legacy_modification(self.to_bytes().into()), ), ( StateKey::access_path(self.make_coin_store_access_path()), - WriteOp::Modification(self.coin_store.to_bytes().into()), + WriteOp::legacy_modification(self.coin_store.to_bytes().into()), ), ]; diff --git a/config/src/config/node_config_loader.rs b/config/src/config/node_config_loader.rs index 8aac07791a67f..5a9752dc01bc3 100644 --- a/config/src/config/node_config_loader.rs +++ b/config/src/config/node_config_loader.rs @@ -13,7 +13,6 @@ use aptos_types::{ on_chain_config::OnChainConfig, state_store::state_key::{StateKey, StateKeyInner}, transaction::{Transaction, WriteSetPayload}, - write_set::WriteOp, }; use serde_yaml::Value; use std::path::Path; @@ -184,17 +183,9 @@ fn get_chain_id(node_config: &NodeConfig) -> Result { })?; // Extract the chain ID from the write op - let write_op_bytes = match write_op { - WriteOp::Creation(bytes) => bytes, - WriteOp::Modification(bytes) => bytes, - WriteOp::CreationWithMetadata { data, metadata: _ } => data, - WriteOp::ModificationWithMetadata { data, metadata: _ } => data, - _ => { - return Err(Error::InvariantViolation( - "The genesis transaction does not contain the correct write op for the chain ID!".into(), - )); - }, - }; + let write_op_bytes = write_op.bytes().ok_or_else(|| Error::InvariantViolation( + "The genesis transaction does not contain the correct write op for the chain ID!".into(), + ))?; let chain_id = ChainId::deserialize_into_config(write_op_bytes).map_err(|error| { Error::InvariantViolation(format!( "Failed to deserialize the chain ID: {:?}", diff --git a/execution/executor-benchmark/src/native_executor.rs b/execution/executor-benchmark/src/native_executor.rs index 37e624d390583..60f0668dc3aaf 100644 --- a/execution/executor-benchmark/src/native_executor.rs +++ b/execution/executor-benchmark/src/native_executor.rs @@ -117,15 +117,15 @@ impl NativeExecutor { let write_set = vec![ ( sender_account_key, - WriteOp::Modification(bcs::to_bytes(&sender_account)?.into()), + WriteOp::legacy_modification(bcs::to_bytes(&sender_account)?.into()), ), ( sender_coin_store_key, - WriteOp::Modification(bcs::to_bytes(&sender_coin_store)?.into()), + WriteOp::legacy_modification(bcs::to_bytes(&sender_coin_store)?.into()), ), // ( // TOTAL_SUPPLY_STATE_KEY.clone(), - // WriteOp::Modification(bcs::to_bytes(&total_supply)?), + // WriteOp::legacy_modification(bcs::to_bytes(&total_supply)?), // ), ]; @@ -179,7 +179,7 @@ impl NativeExecutor { write_set.push(( recipient_coin_store_key, - WriteOp::Modification(bcs::to_bytes(&recipient_coin_store)?.into()), + WriteOp::legacy_modification(bcs::to_bytes(&recipient_coin_store)?.into()), )); } } else { @@ -215,11 +215,11 @@ impl NativeExecutor { write_set.push(( recipient_account_key, - WriteOp::Creation(bcs::to_bytes(&recipient_account)?.into()), + WriteOp::legacy_creation(bcs::to_bytes(&recipient_account)?.into()), )); write_set.push(( recipient_coin_store_key, - WriteOp::Creation(bcs::to_bytes(&recipient_coin_store)?.into()), + WriteOp::legacy_creation(bcs::to_bytes(&recipient_coin_store)?.into()), )); } diff --git a/execution/executor/src/mock_vm/mock_vm_test.rs b/execution/executor/src/mock_vm/mock_vm_test.rs index c1c075955d44e..80709db94486e 100644 --- a/execution/executor/src/mock_vm/mock_vm_test.rs +++ b/execution/executor/src/mock_vm/mock_vm_test.rs @@ -62,11 +62,11 @@ fn test_mock_vm_different_senders() { [ ( StateKey::access_path(balance_ap(sender)), - WriteOp::Modification(amount.le_bytes()), + WriteOp::legacy_modification(amount.le_bytes()), ), ( StateKey::access_path(seqnum_ap(sender)), - WriteOp::Modification(1u64.le_bytes()), + WriteOp::legacy_modification(1u64.le_bytes()), ), ] .into_iter() @@ -101,11 +101,11 @@ fn test_mock_vm_same_sender() { [ ( StateKey::access_path(balance_ap(sender)), - WriteOp::Modification((amount * (i as u64 + 1)).le_bytes()), + WriteOp::legacy_modification((amount * (i as u64 + 1)).le_bytes()), ), ( StateKey::access_path(seqnum_ap(sender)), - WriteOp::Modification((i as u64 + 1).le_bytes()), + WriteOp::legacy_modification((i as u64 + 1).le_bytes()), ), ] .into_iter() @@ -143,15 +143,15 @@ fn test_mock_vm_payment() { [ ( StateKey::access_path(balance_ap(gen_address(0))), - WriteOp::Modification(50u64.le_bytes()) + WriteOp::legacy_modification(50u64.le_bytes()) ), ( StateKey::access_path(seqnum_ap(gen_address(0))), - WriteOp::Modification(2u64.le_bytes()) + WriteOp::legacy_modification(2u64.le_bytes()) ), ( StateKey::access_path(balance_ap(gen_address(1))), - WriteOp::Modification(150u64.le_bytes()) + WriteOp::legacy_modification(150u64.le_bytes()) ), ] .into_iter() diff --git a/execution/executor/src/mock_vm/mod.rs b/execution/executor/src/mock_vm/mod.rs index a02eceeee308d..0356403aea08a 100644 --- a/execution/executor/src/mock_vm/mod.rs +++ b/execution/executor/src/mock_vm/mod.rs @@ -269,14 +269,14 @@ fn gen_genesis_writeset() -> WriteSet { access_path_for_config(ValidatorSet::CONFIG_ID).expect("access path in test"); write_set.insert(( StateKey::access_path(validator_set_ap), - WriteOp::Modification(bcs::to_bytes(&ValidatorSet::new(vec![])).unwrap().into()), + WriteOp::legacy_modification(bcs::to_bytes(&ValidatorSet::new(vec![])).unwrap().into()), )); write_set.insert(( StateKey::access_path(AccessPath::new( CORE_CODE_ADDRESS, ConfigurationResource::resource_path(), )), - WriteOp::Modification( + WriteOp::legacy_modification( bcs::to_bytes(&ConfigurationResource::default()) .unwrap() .into(), @@ -291,11 +291,11 @@ fn gen_mint_writeset(sender: AccountAddress, balance: u64, seqnum: u64) -> Write let mut write_set = WriteSetMut::default(); write_set.insert(( StateKey::access_path(balance_ap(sender)), - WriteOp::Modification(balance.le_bytes()), + WriteOp::legacy_modification(balance.le_bytes()), )); write_set.insert(( StateKey::access_path(seqnum_ap(sender)), - WriteOp::Modification(seqnum.le_bytes()), + WriteOp::legacy_modification(seqnum.le_bytes()), )); write_set.freeze().expect("mint writeset should be valid") } @@ -310,15 +310,15 @@ fn gen_payment_writeset( let mut write_set = WriteSetMut::default(); write_set.insert(( StateKey::access_path(balance_ap(sender)), - WriteOp::Modification(sender_balance.le_bytes()), + WriteOp::legacy_modification(sender_balance.le_bytes()), )); write_set.insert(( StateKey::access_path(seqnum_ap(sender)), - WriteOp::Modification(sender_seqnum.le_bytes()), + WriteOp::legacy_modification(sender_seqnum.le_bytes()), )); write_set.insert(( StateKey::access_path(balance_ap(recipient)), - WriteOp::Modification(recipient_balance.le_bytes()), + WriteOp::legacy_modification(recipient_balance.le_bytes()), )); write_set .freeze() diff --git a/execution/executor/src/tests/mod.rs b/execution/executor/src/tests/mod.rs index e64bef0bcece1..894a577f071c1 100644 --- a/execution/executor/src/tests/mod.rs +++ b/execution/executor/src/tests/mod.rs @@ -565,14 +565,14 @@ fn test_deleted_key_from_state_store() { let transaction2 = create_test_transaction(1); let write_set1 = WriteSetMut::new(vec![( dummy_state_key1.clone(), - WriteOp::Modification(dummy_value1.clone()), + WriteOp::legacy_modification(dummy_value1.clone()), )]) .freeze() .unwrap(); let write_set2 = WriteSetMut::new(vec![( dummy_state_key2.clone(), - WriteOp::Modification(dummy_value2.clone()), + WriteOp::legacy_modification(dummy_value2.clone()), )]) .freeze() .unwrap(); @@ -601,7 +601,7 @@ fn test_deleted_key_from_state_store() { assert_eq!(state_value2_from_db, StateValue::from(dummy_value2.clone())); let transaction3 = create_test_transaction(2); - let write_set3 = WriteSetMut::new(vec![(dummy_state_key1.clone(), WriteOp::Deletion)]) + let write_set3 = WriteSetMut::new(vec![(dummy_state_key1.clone(), WriteOp::legacy_deletion())]) .freeze() .unwrap(); diff --git a/execution/executor/tests/db_bootstrapper_test.rs b/execution/executor/tests/db_bootstrapper_test.rs index 7e5bcb0c18115..5d22fa9c7dd30 100644 --- a/execution/executor/tests/db_bootstrapper_test.rs +++ b/execution/executor/tests/db_bootstrapper_test.rs @@ -229,14 +229,16 @@ fn test_new_genesis() { StateKey::access_path( access_path_for_config(ValidatorSet::CONFIG_ID).expect("access path in test"), ), - WriteOp::Modification(bcs::to_bytes(&ValidatorSet::new(vec![])).unwrap().into()), + WriteOp::legacy_modification( + bcs::to_bytes(&ValidatorSet::new(vec![])).unwrap().into(), + ), ), ( StateKey::access_path(AccessPath::new( CORE_CODE_ADDRESS, ConfigurationResource::resource_path(), )), - WriteOp::Modification( + WriteOp::legacy_modification( bcs::to_bytes(&configuration.bump_epoch_for_test()) .unwrap() .into(), @@ -247,7 +249,7 @@ fn test_new_genesis() { account1, CoinStoreResource::resource_path(), )), - WriteOp::Modification( + WriteOp::legacy_modification( bcs::to_bytes(&CoinStoreResource::new( 100_000_000, false, diff --git a/types/src/account_config/resources/coin_info.rs b/types/src/account_config/resources/coin_info.rs index 3f3a402b44f91..3ee5e1002f34b 100644 --- a/types/src/account_config/resources/coin_info.rs +++ b/types/src/account_config/resources/coin_info.rs @@ -127,11 +127,11 @@ impl CoinInfoResource { let write_set = vec![ ( StateKey::access_path(ap), - WriteOp::Modification(bcs::to_bytes(&self).unwrap().into()), + WriteOp::legacy_modification(bcs::to_bytes(&self).unwrap().into()), ), ( value_state_key, - WriteOp::Modification(bcs::to_bytes(&0_u128).unwrap().into()), + WriteOp::legacy_modification(bcs::to_bytes(&0_u128).unwrap().into()), ), ]; Ok(WriteSetMut::new(write_set).freeze().unwrap()) diff --git a/types/src/proptest_types.rs b/types/src/proptest_types.rs index 89a951abd7f2a..fa47dd1b118c2 100644 --- a/types/src/proptest_types.rs +++ b/types/src/proptest_types.rs @@ -59,11 +59,11 @@ use std::{ impl WriteOp { pub fn value_strategy() -> impl Strategy { - vec(any::(), 0..64).prop_map(|bytes| WriteOp::Modification(bytes.into())) + vec(any::(), 0..64).prop_map(|bytes| WriteOp::legacy_modification(bytes.into())) } pub fn deletion_strategy() -> impl Strategy { - Just(WriteOp::Deletion) + Just(WriteOp::legacy_deletion()) } } @@ -812,7 +812,7 @@ impl TransactionToCommitGen { state_key.clone(), Some(StateValue::new_legacy(Bytes::copy_from_slice(&value))), ), - (state_key, WriteOp::Modification(value.into())), + (state_key, WriteOp::legacy_modification(value.into())), ) }) }) diff --git a/types/src/state_store/state_value.rs b/types/src/state_store/state_value.rs index 33698366cf57c..05818281b861e 100644 --- a/types/src/state_store/state_value.rs +++ b/types/src/state_store/state_value.rs @@ -5,6 +5,7 @@ use crate::{ on_chain_config::CurrentTimeMicroseconds, proof::SparseMerkleRangeProof, state_store::state_key::StateKey, transaction::Version, }; +use anyhow::ensure; use aptos_crypto::{ hash::{CryptoHash, SPARSE_MERKLE_PLACEHOLDER_HASH}, HashValue, @@ -15,6 +16,7 @@ use once_cell::sync::OnceCell; #[cfg(any(test, feature = "fuzzing"))] use proptest::{arbitrary::Arbitrary, prelude::*}; use serde::{Deserialize, Deserializer, Serialize, Serializer}; + #[derive( BCSCryptoHash, Clone, @@ -38,11 +40,11 @@ pub enum StateValueMetadata { bytes_deposit: u64, creation_time_usecs: u64, }, + /// In memory place holder for those state values that were created before StateValueMetadata + /// was a thing. + Dummy, } -// To avoid nested options when fetching a resource and its metadata. -pub type StateValueMetadataKind = Option; - impl StateValueMetadata { // FIXME(aldenhu): update tests and remove pub fn new_v0(deposit: u64, creation_time_usecs: &CurrentTimeMicroseconds) -> Self { @@ -56,30 +58,57 @@ impl StateValueMetadata { Self::new_v0(0, creation_time_usecs) } + pub fn into_persistable(self) -> Option { + use StateValueMetadata::*; + + match self { + V0 { .. } | V1 { .. } => Some(self), + Dummy => None, + } + } + + pub fn is_dummy(&self) -> bool { + use StateValueMetadata::*; + + match self { + Dummy => true, + V0 { .. } | V1 { .. } => false, + } + } + pub fn creation_time_usecs(&self) -> u64 { + use StateValueMetadata::*; + match self { - StateValueMetadata::V0 { + V0 { creation_time_usecs, .. } - | StateValueMetadata::V1 { + | V1 { creation_time_usecs, .. } => *creation_time_usecs, + Dummy => 0, } } pub fn slot_deposit(&self) -> u64 { + use StateValueMetadata::*; + match self { - StateValueMetadata::V0 { deposit, .. } => *deposit, - StateValueMetadata::V1 { slot_deposit, .. } => *slot_deposit, + V0 { deposit, .. } => *deposit, + V1 { slot_deposit, .. } => *slot_deposit, + Dummy => 0, } } pub fn bytes_deposit(&self) -> u64 { + use StateValueMetadata::*; + match self { - StateValueMetadata::V0 { .. } => 0, - StateValueMetadata::V1 { bytes_deposit, .. } => *bytes_deposit, + V0 { .. } => 0, + V1 { bytes_deposit, .. } => *bytes_deposit, + Dummy => 0, } } @@ -88,39 +117,69 @@ impl StateValueMetadata { } pub fn set_slot_deposit(&mut self, amount: u64) { + use StateValueMetadata::*; + match self { - StateValueMetadata::V0 { deposit, .. } => *deposit = amount, - StateValueMetadata::V1 { slot_deposit, .. } => *slot_deposit = amount, + V0 { deposit, .. } => *deposit = amount, + V1 { slot_deposit, .. } => *slot_deposit = amount, + Dummy => { + unreachable!("Not allowed to set slot deposit on Dummy. Upgrade first.") + }, } } pub fn set_bytes_deposit(&mut self, amount: u64) { - let creation_time_usecs = self.creation_time_usecs(); - let slot_deposit = match self { - StateValueMetadata::V0 { deposit, .. } => *deposit, - StateValueMetadata::V1 { slot_deposit, .. } => *slot_deposit, - }; + use StateValueMetadata::*; - *self = Self::V1 { - slot_deposit, - bytes_deposit: amount, - creation_time_usecs, + match self { + V1 { slot_deposit, .. } => *slot_deposit = amount, + V0 { .. } | Dummy => { + unreachable!("Not allowed to set slot deposit on Dummy or V0. Upgrade first.") + }, } } - pub fn set_deposits(&mut self, slot_deposit: u64, bytes_deposit: u64) { - let creation_time_usecs = self.creation_time_usecs(); + pub fn assert_v1(&self) { + assert!(matches!(self, Self::V1 { .. }), "Expecting V1.") + } + + pub fn upgrade(&mut self) -> &mut Self { *self = Self::V1 { - slot_deposit, - bytes_deposit, - creation_time_usecs, + slot_deposit: self.slot_deposit(), + bytes_deposit: self.bytes_deposit(), + creation_time_usecs: self.creation_time_usecs(), + }; + self + } + + pub fn ensure_equivalent(&self, other: &Self) -> anyhow::Result<()> { + match self.clone().upgrade() { + StateValueMetadata::V1 { + slot_deposit, + bytes_deposit, + creation_time_usecs, + } => { + ensure!( + *slot_deposit == other.slot_deposit() + && *bytes_deposit == other.bytes_deposit() + && *creation_time_usecs == other.creation_time_usecs(), + "Not equivalent: {:?} vs {:?}", + self, + other, + ); + }, + StateValueMetadata::V0 { .. } | StateValueMetadata::Dummy => { + unreachable!("StateValueMetadata::upgrade() failed.") + }, } + + Ok(()) } } #[derive(Clone, Debug, CryptoHasher)] pub struct StateValue { - inner: StateValueInner, + inner: InMemoryStateValue, hash: OnceCell, } @@ -132,21 +191,9 @@ impl PartialEq for StateValue { impl Eq for StateValue {} -#[derive( - BCSCryptoHash, - Clone, - CryptoHasher, - Debug, - Deserialize, - Eq, - PartialEq, - Serialize, - Ord, - PartialOrd, - Hash, -)] +#[derive(BCSCryptoHash, CryptoHasher, Deserialize, Serialize)] #[serde(rename = "StateValue")] -pub enum StateValueInner { +enum PersistedStateValue { V0(Bytes), WithMetadata { data: Bytes, @@ -154,6 +201,39 @@ pub enum StateValueInner { }, } +impl PersistedStateValue { + fn into_in_mem_form(self) -> InMemoryStateValue { + match self { + PersistedStateValue::V0(data) => InMemoryStateValue { + data, + metadata: StateValueMetadata::Dummy, + }, + PersistedStateValue::WithMetadata { data, metadata } => { + InMemoryStateValue { data, metadata } + }, + } + } +} + +#[derive(Clone, Debug, Eq, PartialEq)] +struct InMemoryStateValue { + data: Bytes, + metadata: StateValueMetadata, +} + +impl InMemoryStateValue { + fn to_persistable_form(&self) -> PersistedStateValue { + let Self { data, metadata } = self.clone(); + + match metadata { + StateValueMetadata::V0 { .. } | StateValueMetadata::V1 { .. } => { + PersistedStateValue::WithMetadata { data, metadata } + }, + StateValueMetadata::Dummy => PersistedStateValue::V0(data), + } + } +} + #[cfg(any(test, feature = "fuzzing"))] impl Arbitrary for StateValue { type Parameters = (); @@ -171,7 +251,7 @@ impl<'de> Deserialize<'de> for StateValue { where D: Deserializer<'de>, { - let inner = StateValueInner::deserialize(deserializer)?; + let inner = PersistedStateValue::deserialize(deserializer)?.into_in_mem_form(); let hash = OnceCell::new(); Ok(Self { inner, hash }) } @@ -182,20 +262,17 @@ impl Serialize for StateValue { where S: Serializer, { - self.inner.serialize(serializer) + self.inner.to_persistable_form().serialize(serializer) } } impl StateValue { pub fn new_legacy(bytes: Bytes) -> Self { - Self::new_impl(StateValueInner::V0(bytes)) + Self::new_with_metadata(bytes, StateValueMetadata::Dummy) } pub fn new_with_metadata(data: Bytes, metadata: StateValueMetadata) -> Self { - Self::new_impl(StateValueInner::WithMetadata { data, metadata }) - } - - fn new_impl(inner: StateValueInner) -> Self { + let inner = InMemoryStateValue { data, metadata }; let hash = OnceCell::new(); Self { inner, hash } } @@ -205,9 +282,7 @@ impl StateValue { } pub fn bytes(&self) -> &Bytes { - match &self.inner { - StateValueInner::V0(data) | StateValueInner::WithMetadata { data, .. } => data, - } + &self.inner.data } /// Applies a bytes-to-bytes transformation on the state value contents, @@ -216,27 +291,19 @@ impl StateValue { self, f: F, ) -> anyhow::Result { - Ok(StateValue::new_impl(match self.inner { - StateValueInner::V0(data) => StateValueInner::V0(f(data)?), - StateValueInner::WithMetadata { data, metadata } => StateValueInner::WithMetadata { - data: f(data)?, - metadata, - }, - })) + Ok(Self::new_with_metadata( + f(self.inner.data)?, + self.inner.metadata, + )) } - pub fn into_metadata(self) -> Option { - match self.inner { - StateValueInner::V0(_) => None, - StateValueInner::WithMetadata { metadata, .. } => Some(metadata), - } + pub fn into_metadata(self) -> StateValueMetadata { + self.inner.metadata } - pub fn into(self) -> (Option, Bytes) { - match self.inner { - StateValueInner::V0(bytes) => (None, bytes), - StateValueInner::WithMetadata { data, metadata } => (Some(metadata), data), - } + pub fn into_parts(self) -> (StateValueMetadata, Bytes) { + let InMemoryStateValue { data, metadata } = self.inner; + (metadata, data) } } @@ -258,7 +325,9 @@ impl CryptoHash for StateValue { type Hasher = StateValueHasher; fn hash(&self) -> HashValue { - *self.hash.get_or_init(|| CryptoHash::hash(&self.inner)) + *self + .hash + .get_or_init(|| CryptoHash::hash(&self.inner.to_persistable_form())) } } diff --git a/types/src/transaction/change_set.rs b/types/src/transaction/change_set.rs index 28780454d5780..1b269cedde151 100644 --- a/types/src/transaction/change_set.rs +++ b/types/src/transaction/change_set.rs @@ -5,7 +5,7 @@ use crate::{contract_event::ContractEvent, write_set::WriteSet}; use serde::{Deserialize, Serialize}; -#[derive(Clone, Debug, Hash, Eq, PartialEq, Serialize, Deserialize)] +#[derive(Clone, Debug, Eq, Hash, PartialEq, Serialize, Deserialize)] pub struct ChangeSet { write_set: WriteSet, events: Vec, diff --git a/types/src/write_set.rs b/types/src/write_set.rs index 48db97d689a98..043170e414740 100644 --- a/types/src/write_set.rs +++ b/types/src/write_set.rs @@ -7,13 +7,13 @@ use crate::state_store::{ state_key::StateKey, - state_value::{StateValue, StateValueMetadata, StateValueMetadataKind}, + state_value::{StateValue, StateValueMetadata}, }; -use anyhow::{bail, Result}; +use anyhow::{bail, Context, Result}; use aptos_crypto_derive::{BCSCryptoHash, CryptoHasher}; use bytes::Bytes; use once_cell::sync::Lazy; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; use std::{ collections::{btree_map, BTreeMap}, fmt::Debug, @@ -41,8 +41,9 @@ pub enum WriteOpKind { Deletion, } -#[derive(Clone, Eq, Hash, PartialEq, Serialize, Deserialize)] -pub enum WriteOp { +#[derive(Serialize, Deserialize)] +#[serde(rename = "WriteOp")] +enum PersistedWriteOp { Creation(Bytes), Modification(Bytes), Deletion, @@ -59,7 +60,64 @@ pub enum WriteOp { }, } +impl PersistedWriteOp { + fn into_in_mem_form(self) -> WriteOp { + use PersistedWriteOp::*; + + match self { + Creation(data) => WriteOp::Creation { + data, + metadata: StateValueMetadata::Dummy, + }, + Modification(data) => WriteOp::Modification { + data, + metadata: StateValueMetadata::Dummy, + }, + Deletion => WriteOp::Deletion { + metadata: StateValueMetadata::Dummy, + }, + CreationWithMetadata { data, metadata } => WriteOp::Creation { data, metadata }, + ModificationWithMetadata { data, metadata } => WriteOp::Modification { data, metadata }, + DeletionWithMetadata { metadata } => WriteOp::Deletion { metadata }, + } + } +} + +#[derive(Clone, Eq, Hash, PartialEq)] +pub enum WriteOp { + Creation { + data: Bytes, + metadata: StateValueMetadata, + }, + Modification { + data: Bytes, + metadata: StateValueMetadata, + }, + Deletion { + metadata: StateValueMetadata, + }, +} + impl WriteOp { + fn to_persistable_form(&self) -> PersistedWriteOp { + use PersistedWriteOp::*; + + match self.clone() { + Self::Creation { data, metadata } => match metadata.into_persistable() { + None => Creation(data), + Some(metadata) => CreationWithMetadata { data, metadata }, + }, + Self::Modification { data, metadata } => match metadata.into_persistable() { + None => Modification(data), + Some(metadata) => ModificationWithMetadata { data, metadata }, + }, + Self::Deletion { metadata } => match metadata.into_persistable() { + None => Deletion, + Some(metadata) => DeletionWithMetadata { metadata }, + }, + } + } + /// Merges two write ops on the same state item. /// /// returns `false` if the result indicates no op has happened -- that's when the first op @@ -67,53 +125,45 @@ impl WriteOp { pub fn squash(op: &mut Self, other: Self) -> Result { use WriteOp::*; - // 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. - match (&op, other) { - ( - Modification(_) - | ModificationWithMetadata { .. } - | Creation(_) - | CreationWithMetadata { .. }, - Creation(_) | CreationWithMetadata {..}, - ) // create existing - | ( - Deletion | DeletionWithMetadata {..}, - Deletion | DeletionWithMetadata {..} | Modification(_) | ModificationWithMetadata { .. }, - ) // delete or modify already deleted + (Modification { .. } | Creation { .. }, Creation { .. }) // create existing + | (Deletion { .. }, Modification { .. } | Deletion { .. }) // delete or modify already deleted => { - bail!( - "The given change sets cannot be squashed", - ) - }, - (Modification(_), Modification(data) | ModificationWithMetadata {data, ..}) => *op = Modification(data), - (ModificationWithMetadata{metadata, ..}, Modification(data) | ModificationWithMetadata{data, ..}) => { - *op = ModificationWithMetadata{data, metadata: metadata.clone()} - }, - (Creation(_), Modification(data) | ModificationWithMetadata {data, ..} ) => { - *op = Creation(data) + bail!("The given change sets cannot be squashed") }, - (CreationWithMetadata{metadata , ..}, Modification(data) | ModificationWithMetadata{data, ..}) => { - *op = CreationWithMetadata{data, metadata: metadata.clone()} + (Creation {metadata: old_meta, .. } | Modification {metadata: old_meta, ..}, Modification {data, metadata}) => { + // It's not allowed to squash a op with an already charged op (in which case the metadata will change -- for example the bytes_deposit might increase) + old_meta.ensure_equivalent(&metadata).context("Squashing Creation | Modification with Modification")?; + + *op = Modification { + data, + metadata, + }; }, - (Modification(_) , Deletion | DeletionWithMetadata {..}) => { - *op = Deletion - }, - (ModificationWithMetadata{metadata, ..} , Deletion | DeletionWithMetadata {..}) => { - *op = DeletionWithMetadata {metadata: metadata.clone()} - }, - (Deletion, Creation(data) | CreationWithMetadata {data, ..}) => { - *op = Modification(data) + (Modification {metadata: old_meta, ..}, Deletion {metadata}) => { + // It's not allowed to squash a op with an already charged op (in which case the metadata will change -- for example the bytes_deposit might increase) + old_meta.ensure_equivalent(&metadata).context("Squashing Modification with Deletion")?; + + *op = Deletion { + metadata, + } }, - (DeletionWithMetadata {metadata, ..}, Creation(data) | CreationWithMetadata {data, ..}) => { - *op = ModificationWithMetadata{data, metadata: metadata.clone()} + (Deletion {metadata: old_meta}, Creation {data, ..}) => { + // 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: old_meta.clone(), + } }, - (Creation(_) | CreationWithMetadata {..}, Deletion | DeletionWithMetadata {..}) => { + (Creation {metadata: old_meta, .. }, Deletion {metadata}) => { + // It's not allowed to squash a op with an already charged op (in which case the metadata will change -- for example the bytes_deposit might increase) + old_meta.ensure_equivalent(&metadata).context("Squashing Creation with Deletion")?; + return Ok(false) }, } @@ -124,37 +174,70 @@ impl WriteOp { use WriteOp::*; match self { - Creation(data) - | CreationWithMetadata { data, .. } - | Modification(data) - | ModificationWithMetadata { data, .. } => Some(data), - Deletion | DeletionWithMetadata { .. } => None, + Creation { data, .. } | Modification { data, .. } => Some(data), + Deletion { .. } => None, } } - pub fn metadata(&self) -> Option<&StateValueMetadata> { + pub fn metadata(&self) -> &StateValueMetadata { use WriteOp::*; match self { - Creation(_) | Modification(_) | Deletion => None, - CreationWithMetadata { metadata, .. } - | ModificationWithMetadata { metadata, .. } - | DeletionWithMetadata { metadata, .. } => Some(metadata), + Creation { metadata, .. } | Modification { metadata, .. } | Deletion { metadata } => { + metadata + }, } } - pub fn get_metadata_mut(&mut self) -> Option<&mut StateValueMetadata> { + pub fn get_metadata_mut(&mut self) -> &mut StateValueMetadata { use WriteOp::*; match self { - Creation(_) | Modification(_) | Deletion => None, - CreationWithMetadata { metadata, .. } - | ModificationWithMetadata { metadata, .. } - | DeletionWithMetadata { metadata, .. } => Some(metadata), + Creation { metadata, .. } | Modification { metadata, .. } | Deletion { metadata } => { + metadata + }, + } + } + + pub fn legacy_creation(data: Bytes) -> Self { + Self::Creation { + data, + metadata: StateValueMetadata::Dummy, + } + } + + pub fn legacy_modification(data: Bytes) -> Self { + Self::Modification { + data, + metadata: StateValueMetadata::Dummy, + } + } + + pub fn legacy_deletion() -> Self { + Self::Deletion { + metadata: StateValueMetadata::Dummy, } } } +impl<'de> Deserialize<'de> for WriteOp { + fn deserialize(deserializer: D) -> std::result::Result + where + D: Deserializer<'de>, + { + PersistedWriteOp::deserialize(deserializer).map(|persisted| persisted.into_in_mem_form()) + } +} + +impl Serialize for WriteOp { + fn serialize(&self, serializer: S) -> std::result::Result + where + S: Serializer, + { + self.to_persistable_form().serialize(serializer) + } +} + pub enum WriteOpSize { Creation { write_len: u64 }, Modification { write_len: u64 }, @@ -165,15 +248,13 @@ impl From<&WriteOp> for WriteOpSize { fn from(value: &WriteOp) -> Self { use WriteOp::*; match value { - Creation(data) | CreationWithMetadata { data, .. } => WriteOpSize::Creation { + Creation { data, .. } => WriteOpSize::Creation { write_len: data.len() as u64, }, - Modification(data) | ModificationWithMetadata { data, .. } => { - WriteOpSize::Modification { - write_len: data.len() as u64, - } + Modification { data, .. } => WriteOpSize::Modification { + write_len: data.len() as u64, }, - Deletion | DeletionWithMetadata { .. } => WriteOpSize::Deletion, + Deletion { .. } => WriteOpSize::Deletion, } } } @@ -198,7 +279,7 @@ pub trait TransactionWrite: Debug { // Returns metadata that would be observed by a read following the 'self' write. // Provided as a separate method to avoid the clone in as_state_value method // (although default implementation below does just that). - fn as_state_value_metadata(&self) -> Option { + fn as_state_value_metadata(&self) -> Option { self.as_state_value() .map(|state_value| state_value.into_metadata()) } @@ -252,36 +333,36 @@ impl TransactionWrite for WriteOp { } fn as_state_value(&self) -> Option { - self.bytes().map(|bytes| match self.metadata() { - None => StateValue::new_legacy(bytes.clone()), - Some(metadata) => StateValue::new_with_metadata(bytes.clone(), metadata.clone()), - }) + self.bytes() + .map(|bytes| StateValue::new_with_metadata(bytes.clone(), self.metadata().clone())) } // Note that even if WriteOp is DeletionWithMetadata, the method returns None, as a later // read would not read the metadata of the deletion op. - fn as_state_value_metadata(&self) -> Option { - self.bytes().map(|_| self.metadata().cloned()) + fn as_state_value_metadata(&self) -> Option { + self.bytes().map(|_| self.metadata().clone()) } fn from_state_value(maybe_state_value: Option) -> Self { - match maybe_state_value.map(|state_value| state_value.into()) { - None => WriteOp::Deletion, - Some((None, bytes)) => WriteOp::Creation(bytes), - Some((Some(metadata), bytes)) => WriteOp::CreationWithMetadata { - data: bytes, - metadata, + match maybe_state_value { + None => Self::legacy_deletion(), + Some(state_value) => { + let (metadata, data) = state_value.into_parts(); + if data.is_empty() { + Self::Deletion { metadata } + } else { + Self::Modification { data, metadata } + } }, } } fn write_op_kind(&self) -> WriteOpKind { + use WriteOpKind::*; match self { - WriteOp::Creation(_) | WriteOp::CreationWithMetadata { .. } => WriteOpKind::Creation, - WriteOp::Modification(_) | WriteOp::ModificationWithMetadata { .. } => { - WriteOpKind::Modification - }, - WriteOp::Deletion | WriteOp::DeletionWithMetadata { .. } => WriteOpKind::Deletion, + WriteOp::Creation { .. } => Creation, + WriteOp::Modification { .. } => Modification, + WriteOp::Deletion { .. } => Deletion, } } @@ -289,24 +370,20 @@ impl TransactionWrite for WriteOp { use WriteOp::*; match self { - Creation(data) | CreationWithMetadata { data, .. } => *data = bytes, - Modification(data) | ModificationWithMetadata { data, .. } => *data = bytes, - Deletion | DeletionWithMetadata { .. } => (), + Creation { data, .. } | Modification { data, .. } => *data = bytes, + Deletion { .. } => (), } } fn convert_read_to_modification(&self) -> Option { use WriteOp::*; - match self { - Creation(data) | Modification(data) => Some(Modification(data.clone())), - CreationWithMetadata { data, metadata } - | ModificationWithMetadata { data, metadata } => Some(ModificationWithMetadata { - data: data.clone(), - metadata: metadata.clone(), - }), + match self.clone() { + Creation { data, metadata } | Modification { data, metadata } => { + Some(Modification { data, metadata }) + }, // Deletion don't have data to become modification. - Deletion | DeletionWithMetadata { .. } => None, + Deletion { .. } => None, } } } @@ -314,41 +391,24 @@ impl TransactionWrite for WriteOp { impl std::fmt::Debug for WriteOp { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - WriteOp::Modification(value) => write!( - f, - "Modification({})", - value - .iter() - .map(|byte| format!("{:02x}", byte)) - .collect::() - ), - WriteOp::Creation(value) => write!( - f, - "Creation({})", - value - .iter() - .map(|byte| format!("{:02x}", byte)) - .collect::() - ), - WriteOp::Deletion => write!(f, "Deletion"), - WriteOp::CreationWithMetadata { data, metadata } => write!( + WriteOp::Creation { data, metadata } => write!( f, - "CreationWithMetadata({}, metadata:{:?})", + "Creation({}, metadata:{:?})", data.iter() .map(|byte| format!("{:02x}", byte)) .collect::(), metadata, ), - WriteOp::ModificationWithMetadata { data, metadata } => write!( + WriteOp::Modification { data, metadata } => write!( f, - "ModificationWithMetadata({}, metadata:{:?})", + "Modification({}, metadata:{:?})", data.iter() .map(|byte| format!("{:02x}", byte)) .collect::(), metadata, ), - WriteOp::DeletionWithMetadata { metadata } => { - write!(f, "DeletionWithMetadata(metadata:{:?})", metadata,) + WriteOp::Deletion { metadata } => { + write!(f, "Deletion(metadata:{:?})", metadata,) }, } } @@ -435,7 +495,7 @@ impl WriteSetV0 { .write_set .insert( TOTAL_SUPPLY_STATE_KEY.clone(), - WriteOp::Modification(bcs::to_bytes(&value).unwrap().into()) + WriteOp::legacy_modification(bcs::to_bytes(&value).unwrap().into()) ) .is_some()); }