Skip to content

Commit

Permalink
WriteOp carries StateValue
Browse files Browse the repository at this point in the history
  • Loading branch information
msmouse committed Nov 8, 2024
1 parent 68ae56e commit a177db0
Show file tree
Hide file tree
Showing 13 changed files with 134 additions and 193 deletions.
4 changes: 2 additions & 2 deletions aptos-move/aptos-vm-types/src/abstract_write_op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,15 +111,15 @@ 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) -> &mut StateValueMetadata {
pub fn metadata_mut(&mut self) -> &mut StateValueMetadata {
use AbstractResourceWriteOp::*;
match self {
Write(write_op)
| WriteWithDelayedFields(WriteWithDelayedFieldsOp { write_op, .. })
| WriteResourceGroup(GroupWrite {
metadata_op: write_op,
..
}) => write_op.get_metadata_mut(),
}) => write_op.metadata_mut(),
InPlaceDelayedFieldChange(InPlaceDelayedFieldChangeOp { metadata, .. })
| ResourceGroupInPlaceDelayedFieldChange(ResourceGroupInPlaceDelayedFieldChangeOp {
metadata,
Expand Down
12 changes: 6 additions & 6 deletions aptos-move/aptos-vm-types/src/change_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -883,7 +883,7 @@ impl ChangeSetInterface for VMChangeSet {
key,
op_size: op.materialized_size(),
prev_size: op.prev_materialized_size(key, executor_view)?,
metadata_mut: op.get_metadata_mut(),
metadata_mut: op.metadata_mut(),
})
});
let v1_aggregators = self.aggregator_v1_write_set.iter_mut().map(|(key, op)| {
Expand All @@ -893,7 +893,7 @@ impl ChangeSetInterface for VMChangeSet {
prev_size: executor_view
.get_aggregator_v1_state_value_size(key)?
.unwrap_or(0),
metadata_mut: op.get_metadata_mut(),
metadata_mut: op.metadata_mut(),
})
});

Expand Down
2 changes: 1 addition & 1 deletion aptos-move/aptos-vm-types/src/module_write_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ impl ModuleWriteSet {
key,
op_size: write.write_op().write_op_size(),
prev_size,
metadata_mut: write.write_op_mut().get_metadata_mut(),
metadata_mut: write.write_op_mut().metadata_mut(),
})
})
}
Expand Down
38 changes: 8 additions & 30 deletions aptos-move/aptos-vm-types/src/tests/test_change_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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"),
}
}
Expand Down Expand Up @@ -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
);
Expand Down Expand Up @@ -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])
);
Expand All @@ -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])
);
Expand Down
22 changes: 6 additions & 16 deletions aptos-move/aptos-vm/src/move_vm_ext/write_op_converter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,6 @@ impl<'r> WriteOpConverter<'r> {
legacy_creation_as_modification: bool,
) -> PartialVMResult<WriteOp> {
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.
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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());
}
}
2 changes: 1 addition & 1 deletion aptos-move/e2e-move-tests/src/tests/storage_refund.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
8 changes: 4 additions & 4 deletions crates/aptos-rosetta/src/test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,10 @@ fn resource_group_modification_write_op<T: Serialize>(
) -> (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)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,7 @@ impl IndexerStreamCoordinator {
.iter()
.map(|(state_key, write_op)| WriteOpSizeInfo {
key_bytes: Self::ser_size_u32(state_key),
value_bytes: write_op.size() as u32,
value_bytes: write_op.bytes_size() as u32,
})
.collect(),
}
Expand Down
4 changes: 1 addition & 3 deletions testsuite/generate-format/src/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,7 @@ pub fn get_registry() -> Result<Registry> {
// 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
Expand Down
9 changes: 2 additions & 7 deletions testsuite/generate-format/src/aptos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -90,9 +87,7 @@ pub fn get_registry() -> Result<Registry> {
// 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::<contract_event::ContractEvent>(&samples)?;
Expand Down
9 changes: 2 additions & 7 deletions testsuite/generate-format/src/consensus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -89,9 +86,7 @@ pub fn get_registry() -> Result<Registry> {
&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::<contract_event::ContractEvent>(&samples)?;
Expand Down
17 changes: 17 additions & 0 deletions types/src/state_store/state_value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Loading

0 comments on commit a177db0

Please sign in to comment.