From 56a27503e607b24e9ac539f38d781eb7e3d29bbf Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Tue, 26 Jul 2022 22:40:53 +0200 Subject: [PATCH] [fix] Removed deltas from WriteOps --- Cargo.lock | 1 + api/types/src/convert.rs | 4 - aptos-move/aptos-vm/src/data_cache.rs | 3 - .../src/parallel_executor/storage_wrapper.rs | 3 - aptos-move/e2e-tests/src/data_store.rs | 1 - aptos-move/genesis-viewer/src/main.rs | 3 - aptos-move/transaction-replay/src/lib.rs | 2 - .../specifications/move_adapter/README.md | 17 +-- .../src/in_memory_state_calculator.rs | 1 - types/Cargo.toml | 1 + types/src/delta_set.rs | 128 ++++++++++++++++++ types/src/lib.rs | 1 + types/src/write_set.rs | 35 +---- 13 files changed, 133 insertions(+), 67 deletions(-) create mode 100644 types/src/delta_set.rs diff --git a/Cargo.lock b/Cargo.lock index b9d067e4f38db..6b33da506ea9e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1198,6 +1198,7 @@ dependencies = [ "aptos-crypto-derive", "bcs", "chrono", + "claim", "hex", "itertools", "mirai-annotations", diff --git a/api/types/src/convert.rs b/api/types/src/convert.rs index e01aefd500c19..202592308c57c 100644 --- a/api/types/src/convert.rs +++ b/api/types/src/convert.rs @@ -260,8 +260,6 @@ impl<'a, R: MoveResolverExt + ?Sized> MoveConverter<'a, R> { data: self.try_into_resource(&typ, &val)?, }), }, - // Deltas never use access paths. - WriteOp::Delta(..) => unreachable!("unexpected conversion"), }; Ok(ret) } @@ -287,8 +285,6 @@ impl<'a, R: MoveResolverExt + ?Sized> MoveConverter<'a, R> { key, value: value.into(), }), - // Deltas are materialized into WriteOP::Value(..) in executor. - WriteOp::Delta(..) => unreachable!("unexpected conversion"), }; Ok(ret) } diff --git a/aptos-move/aptos-vm/src/data_cache.rs b/aptos-move/aptos-vm/src/data_cache.rs index 9a8a45679f860..5bf86f807bf56 100644 --- a/aptos-move/aptos-vm/src/data_cache.rs +++ b/aptos-move/aptos-vm/src/data_cache.rs @@ -72,9 +72,6 @@ impl<'a, S: StateView> StateViewCache<'a, S> { self.data_map.remove(ap); self.data_map.insert(ap.clone(), None); } - WriteOp::Delta(..) => { - unimplemented!("sequential execution is not supported for deltas") - } } } } diff --git a/aptos-move/aptos-vm/src/parallel_executor/storage_wrapper.rs b/aptos-move/aptos-vm/src/parallel_executor/storage_wrapper.rs index f8faee478b623..45f57fc402abf 100644 --- a/aptos-move/aptos-vm/src/parallel_executor/storage_wrapper.rs +++ b/aptos-move/aptos-vm/src/parallel_executor/storage_wrapper.rs @@ -35,9 +35,6 @@ impl<'a, S: StateView> StateView for VersionedView<'a, S> { Some(v) => Ok(match v.as_ref() { WriteOp::Value(w) => Some(w.clone()), WriteOp::Deletion => None, - WriteOp::Delta(..) => { - unimplemented!("parallel execution is not supported for deltas") - } }), None => self.base_view.get_state_value(state_key), } diff --git a/aptos-move/e2e-tests/src/data_store.rs b/aptos-move/e2e-tests/src/data_store.rs index 50dce426ebe1a..f4d95fe1e48aa 100644 --- a/aptos-move/e2e-tests/src/data_store.rs +++ b/aptos-move/e2e-tests/src/data_store.rs @@ -50,7 +50,6 @@ impl FakeDataStore { WriteOp::Deletion => { self.remove(state_key); } - WriteOp::Delta(..) => unreachable!("deltas are only used in executor"), } } } diff --git a/aptos-move/genesis-viewer/src/main.rs b/aptos-move/genesis-viewer/src/main.rs index cfe3c0b980655..da91ae97eca9a 100644 --- a/aptos-move/genesis-viewer/src/main.rs +++ b/aptos-move/genesis-viewer/src/main.rs @@ -188,7 +188,6 @@ fn print_modules(ws: &WriteSet) { AccessPath::try_from(k.clone()).expect("State key can't be converted to access path"); match v { WriteOp::Deletion => panic!("found WriteOp::Deletion in WriteSet"), - WriteOp::Delta(..) => panic!("found WriteOp::Delta in WriteSet"), WriteOp::Value(blob) => { let tag = ap.path.get(0).expect("empty blob in WriteSet"); if *tag == 0 { @@ -214,7 +213,6 @@ fn print_resources(storage: &impl MoveResolverExt, ws: &WriteSet) { AccessPath::try_from(k.clone()).expect("State key can't be converted to access path"); match v { WriteOp::Deletion => panic!("found WriteOp::Deletion in WriteSet"), - WriteOp::Delta(..) => panic!("found WriteOp::Delta in WriteSet"), WriteOp::Value(blob) => { let tag = ap.path.get(0).expect("empty blob in WriteSet"); if *tag == 1 { @@ -245,7 +243,6 @@ fn print_account_states(storage: &impl MoveResolverExt, ws: &WriteSet) { AccessPath::try_from(k.clone()).expect("State key can't be converted to access path"); match v { WriteOp::Deletion => panic!("found WriteOp::Deletion in WriteSet"), - WriteOp::Delta(..) => panic!("found WriteOp::Delta in WriteSet"), WriteOp::Value(blob) => { let tag = ap.path.get(0).expect("empty blob in WriteSet"); if *tag == 1 { diff --git a/aptos-move/transaction-replay/src/lib.rs b/aptos-move/transaction-replay/src/lib.rs index 437d46a5adf2a..405424cd95a18 100644 --- a/aptos-move/transaction-replay/src/lib.rs +++ b/aptos-move/transaction-replay/src/lib.rs @@ -177,12 +177,10 @@ impl AptosDebugger { access_path::Path::Resource(tag) => match op { WriteOp::Deletion => state_view.delete_resource(addr, tag)?, WriteOp::Value(bytes) => state_view.save_resource(addr, tag, bytes)?, - WriteOp::Delta(..) => unreachable!("deltas are only used in executor"), }, access_path::Path::Code(module_id) => match op { WriteOp::Deletion => state_view.delete_module(&module_id)?, WriteOp::Value(bytes) => state_view.save_module(&module_id, bytes)?, - WriteOp::Delta(..) => unreachable!("deltas are only used in executor"), }, } } diff --git a/documentation/specifications/move_adapter/README.md b/documentation/specifications/move_adapter/README.md index ecea93a548bf1..17daa933611bb 100644 --- a/documentation/specifications/move_adapter/README.md +++ b/documentation/specifications/move_adapter/README.md @@ -531,28 +531,13 @@ pub struct TransactionOutput { /// `WriteSet` contains all access paths that one transaction modifies. /// Each of them is a `WriteOp` where `Value(val)` means that serialized /// representation should be updated to `val`, and `Deletion` means that -/// we are going to delete this access path. A special `WriteOp` used by -/// aggregator - `Delta(op, limit)`, means that `op` should be applied to -/// deserialized representation and a postcondition `result <= limit` must be -/// ensured. +/// we are going to delete this access path. pub struct WriteSet { write_set: Vec<(AccessPath, WriteOp)>, } -/// `DeltaOperation` specifies the partial function which is used to apply -/// delta. -pub enum DeltaOperation { - Addition(u128), - Subtraction(u128), -} - -/// Value when delta application overflows, i.e. the postcondition of delta -/// application. -pub struct DeltaLimit(pub u128); - pub enum WriteOp { Deletion, - Delta(DeltaOperation, DeltaLimit), Value(Vec), } diff --git a/execution/executor-types/src/in_memory_state_calculator.rs b/execution/executor-types/src/in_memory_state_calculator.rs index 29ea5a4d7bb9b..40783691dbaa3 100644 --- a/execution/executor-types/src/in_memory_state_calculator.rs +++ b/execution/executor-types/src/in_memory_state_calculator.rs @@ -244,7 +244,6 @@ fn process_state_key_write_op( let state_value = match write_op { WriteOp::Value(new_value) => StateValue::from(new_value), WriteOp::Deletion => StateValue::empty(), - WriteOp::Delta(..) => unreachable!("deltas are only used in executor"), }; match state_cache.entry(state_key.clone()) { hash_map::Entry::Occupied(mut entry) => { diff --git a/types/Cargo.toml b/types/Cargo.toml index dac333230cb3e..b803390ab34c9 100644 --- a/types/Cargo.toml +++ b/types/Cargo.toml @@ -36,6 +36,7 @@ aptos-crypto-derive = { path = "../crates/aptos-crypto-derive" } move-deps = { path = "../aptos-move/move-deps", features = ["address32"] } [dev-dependencies] +claim = "0.5.0" proptest = "1.0.0" proptest-derive = "0.3.0" regex = "1.5.5" diff --git a/types/src/delta_set.rs b/types/src/delta_set.rs new file mode 100644 index 0000000000000..927287a7a4950 --- /dev/null +++ b/types/src/delta_set.rs @@ -0,0 +1,128 @@ +// Copyright (c) Aptos +// SPDX-License-Identifier: Apache-2.0 + +//! Parallel data aggregation uses a `Delta` op. Every delta is is a state key +//! (for accessing the storage) and an operation: a partial function with a +//! postcondition. + +use crate::state_store::state_key::StateKey; + +/// Specifies different delta partial function specifications. +#[derive(Copy, Clone, Hash, PartialOrd, Ord, PartialEq, Eq)] +pub enum DeltaOp { + /// Addition of `value` which overflows on `limit`. + Addition { value: u128, limit: u128 }, + /// Subtraction of `value` which cannot go below zero. + Subtraction { value: u128 }, +} + +impl DeltaOp { + /// Returns optional result of delta application to `base` (None if + /// postocndition not satisfied). + pub fn apply_to(&self, base: u128) -> Option { + match self { + DeltaOp::Addition { value, limit } => addition(base, *value, *limit), + DeltaOp::Subtraction { value } => subtraction(base, *value), + } + } +} + +/// Implements application of `Addition` to `base`. +fn addition(base: u128, value: u128, limit: u128) -> Option { + if value > (limit - base) { + None + } else { + Some(base + value) + } +} + +/// Implements application of `Subtraction` to `base`. +fn subtraction(base: u128, value: u128) -> Option { + if value > base { + None + } else { + Some(base - value) + } +} + +impl std::fmt::Debug for DeltaOp { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + DeltaOp::Addition { value, limit } => { + write!(f, "+{} ensures result <= {}", value, limit) + } + DeltaOp::Subtraction { value } => { + write!(f, "-{} ensures 0 <= result", value) + } + } + } +} + +/// Serializes value after delta application. +pub fn serialize(value: &u128) -> Vec { + bcs::to_bytes(value).expect("unexpected serialization error") +} + +/// Deserializes value for delta application. +pub fn deserialize(value_bytes: &[u8]) -> u128 { + bcs::from_bytes(value_bytes).expect("unexpected deserialization error") +} + +/// `DeltaSet` contains all access paths that one transaction wants to update with deltas. +#[derive(Clone, Debug, Default, Eq, Hash, PartialEq)] +pub struct DeltaSet { + delta_set: Vec<(StateKey, DeltaOp)>, +} + +impl DeltaSet { + pub fn new(delta_set: Vec<(StateKey, DeltaOp)>) -> Self { + DeltaSet { delta_set } + } + + pub fn push(&mut self, delta: (StateKey, DeltaOp)) { + self.delta_set.push(delta); + } + + pub fn pop(&mut self) { + self.delta_set.pop(); + } + + #[inline] + pub fn is_empty(&self) -> bool { + self.delta_set.is_empty() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use claim::{assert_matches, assert_some_eq}; + + fn addition(value: u128, limit: u128) -> DeltaOp { + DeltaOp::Addition { value, limit } + } + + fn subtraction(value: u128) -> DeltaOp { + DeltaOp::Subtraction { value } + } + + #[test] + fn test_delta_addition() { + let add5 = addition(5, 100); + assert_some_eq!(add5.apply_to(0), 5); + assert_some_eq!(add5.apply_to(5), 10); + assert_some_eq!(add5.apply_to(95), 100); + + assert_matches!(add5.apply_to(96), None); + } + + #[test] + fn test_delta_subtraction() { + let sub5 = subtraction(5); + assert_matches!(sub5.apply_to(0), None); + assert_matches!(sub5.apply_to(1), None); + + assert_some_eq!(sub5.apply_to(5), 0); + assert_some_eq!(sub5.apply_to(100), 95); + } +} diff --git a/types/src/lib.rs b/types/src/lib.rs index 31b6fb22f9e92..f523b8d87bb67 100644 --- a/types/src/lib.rs +++ b/types/src/lib.rs @@ -11,6 +11,7 @@ pub mod block_info; pub mod block_metadata; pub mod chain_id; pub mod contract_event; +pub mod delta_set; pub mod epoch_change; pub mod epoch_state; pub mod event; diff --git a/types/src/write_set.rs b/types/src/write_set.rs index 00c2d5df8b758..689216955056e 100644 --- a/types/src/write_set.rs +++ b/types/src/write_set.rs @@ -2,46 +2,17 @@ // SPDX-License-Identifier: Apache-2.0 //! 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. For -//! aggregator, delta updates are used (note: this is a temporary solution and ideally we should -//! modify `ChangeSet` and `TransactionOutput` to keep deltas internal to executor). +//! 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; use anyhow::Result; use aptos_crypto_derive::{BCSCryptoHash, CryptoHasher}; use serde::{Deserialize, Serialize}; -/// Specifies partial function such as +X or -X to use with `WriteOp::Delta`. -#[derive(Copy, Clone, Hash, PartialOrd, Ord, PartialEq, Eq)] -pub enum DeltaOperation { - Addition(u128), - Subtraction(u128), -} - -impl std::fmt::Debug for DeltaOperation { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - DeltaOperation::Addition(value) => write!(f, "+{}", value), - DeltaOperation::Subtraction(value) => write!(f, "-{}", value), - } - } -} - -#[derive(Copy, Clone, Hash, PartialOrd, Ord, PartialEq, Eq)] -pub struct DeltaLimit(pub u128); - -impl std::fmt::Debug for DeltaLimit { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "result <= {}", self.0) - } -} - #[derive(Clone, Eq, Hash, PartialEq, Serialize, Deserialize)] pub enum WriteOp { Deletion, Value(#[serde(with = "serde_bytes")] Vec), - #[serde(skip)] - Delta(DeltaOperation, DeltaLimit), } impl WriteOp { @@ -49,7 +20,6 @@ impl WriteOp { pub fn is_deletion(&self) -> bool { match self { WriteOp::Deletion => true, - WriteOp::Delta(..) => false, WriteOp::Value(_) => false, } } @@ -66,9 +36,6 @@ impl std::fmt::Debug for WriteOp { .map(|byte| format!("{:02x}", byte)) .collect::() ), - WriteOp::Delta(op, limit) => { - write!(f, "Delta({:?} ensures {:?})", op, limit) - } WriteOp::Deletion => write!(f, "Deletion"), } }