From 6c82029bcbc656a3cd423d403d7551974818d45d Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 27 Oct 2023 09:21:04 +1100 Subject: [PATCH] Remove diff feature (#30) --- src/diff.rs | 217 ------------------------------- src/error.rs | 5 - src/lib.rs | 2 - src/list.rs | 12 -- src/tests/diff.rs | 98 -------------- src/tests/mod.rs | 1 - src/tests/proptest/operations.rs | 45 ++----- src/tree.rs | 136 ------------------- src/vector.rs | 12 -- 9 files changed, 14 insertions(+), 514 deletions(-) delete mode 100644 src/diff.rs delete mode 100644 src/tests/diff.rs diff --git a/src/diff.rs b/src/diff.rs deleted file mode 100644 index 65e5e05..0000000 --- a/src/diff.rs +++ /dev/null @@ -1,217 +0,0 @@ -use crate::{ - interface::MutList, tree::TreeDiff, update_map::MaxMap, Error, List, UpdateMap, Value, Vector, -}; -use serde::{Deserialize, Serialize}; -use ssz::{Decode, Encode}; -use ssz_derive::{Decode, Encode}; -use std::marker::PhantomData; -use typenum::Unsigned; -use vec_map::VecMap; - -/// Trait for diffs that can be applied to a given `Target` type. -pub trait Diff: Sized { - /// The type acted upon. - type Target; - /// The type of errors produced by diffing `Target`. - type Error: From; - - /// Produce a diff between `orig` and `other` where `other` is an updated version of `orig`. - fn compute_diff(orig: &Self::Target, other: &Self::Target) -> Result; - - /// Apply a diff to `target`, updating it mutably. - fn apply_diff(self, target: &mut Self::Target) -> Result<(), Self::Error>; -} - -/// The most primitive type of diff which just stores the entire updated value. -#[derive(Debug, PartialEq, Deserialize, Serialize)] -#[serde(transparent)] -pub struct CloneDiff(pub T); - -impl Diff for CloneDiff { - type Target = T; - type Error = Error; - - fn compute_diff(_: &T, other: &T) -> Result { - Ok(CloneDiff(other.clone())) - } - - fn apply_diff(self, target: &mut T) -> Result<(), Error> { - *target = self.0; - Ok(()) - } -} - -impl Encode for CloneDiff -where - T: Encode + Clone, -{ - fn is_ssz_fixed_len() -> bool { - T::is_ssz_fixed_len() - } - - fn ssz_fixed_len() -> usize { - T::ssz_fixed_len() - } - - fn ssz_append(&self, buf: &mut Vec) { - self.0.ssz_append(buf) - } - - fn ssz_bytes_len(&self) -> usize { - self.0.ssz_bytes_len() - } -} - -impl Decode for CloneDiff -where - T: Decode + Clone, -{ - fn is_ssz_fixed_len() -> bool { - T::is_ssz_fixed_len() - } - - fn ssz_fixed_len() -> usize { - T::ssz_fixed_len() - } - - fn from_ssz_bytes(bytes: &[u8]) -> Result { - T::from_ssz_bytes(bytes).map(CloneDiff) - } -} - -/// Newtype for List diffs. -#[derive(Debug, PartialEq, Decode, Encode, Deserialize, Serialize)] -#[serde(bound( - deserialize = "T: Value + Deserialize<'de>", - serialize = "T: Value + Serialize" -))] -pub struct ListDiff = MaxMap>> { - tree_diff: TreeDiff, - #[serde(skip, default)] - #[ssz(skip_serializing, skip_deserializing)] - _phantom: PhantomData<(N, U)>, -} - -impl Diff for ListDiff -where - T: Value, - N: Unsigned, - U: UpdateMap, -{ - type Target = List; - type Error = Error; - - fn compute_diff(orig: &Self::Target, other: &Self::Target) -> Result { - if orig.has_pending_updates() || other.has_pending_updates() { - return Err(Error::InvalidDiffPendingUpdates); - } - let mut tree_diff = TreeDiff::default(); - orig.interface.backing.tree.diff( - &other.interface.backing.tree, - 0, - orig.interface.backing.depth, - &mut tree_diff, - )?; - Ok(Self { - tree_diff, - _phantom: PhantomData, - }) - } - - fn apply_diff(self, target: &mut Self::Target) -> Result<(), Error> { - target - .interface - .backing - .update(self.tree_diff.leaves, Some(self.tree_diff.hashes)) - } -} - -/// List diff that gracefully handles removals by falling back to a `CloneDiff`. -/// -/// If removals definitely don't need to be handled then a `ListDiff` is preferable as it is -/// more space-efficient. -#[derive(Debug, PartialEq, Decode, Encode, Deserialize, Serialize)] -#[serde(bound( - deserialize = "T: Value + Deserialize<'de>", - serialize = "T: Value + Serialize" -))] -#[ssz(enum_behaviour = "union")] -pub enum ResetListDiff -where - T: Value, - N: Unsigned, -{ - Reset(CloneDiff>), - Update(ListDiff), -} - -impl Diff for ResetListDiff -where - T: Value, - N: Unsigned, -{ - type Target = List; - type Error = Error; - - fn compute_diff(orig: &Self::Target, other: &Self::Target) -> Result { - // Detect shortening/removals which the current tree diff algorithm can't handle. - if other.len() < orig.len() { - Ok(Self::Reset(CloneDiff(other.clone()))) - } else { - Ok(Self::Update(ListDiff::compute_diff(orig, other)?)) - } - } - - fn apply_diff(self, target: &mut Self::Target) -> Result<(), Error> { - match self { - Self::Reset(diff) => diff.apply_diff(target), - Self::Update(diff) => diff.apply_diff(target), - } - } -} - -/// Newtype for Vector diffs. -#[derive(Debug, PartialEq, Decode, Encode, Deserialize, Serialize)] -#[serde(bound( - deserialize = "T: Value + Deserialize<'de>", - serialize = "T: Value + Serialize" -))] -pub struct VectorDiff = MaxMap>> { - tree_diff: TreeDiff, - #[ssz(skip_serializing, skip_deserializing)] - _phantom: PhantomData<(N, U)>, -} - -impl Diff for VectorDiff -where - T: Value, - N: Unsigned, - U: UpdateMap, -{ - type Target = Vector; - type Error = Error; - - fn compute_diff(orig: &Self::Target, other: &Self::Target) -> Result { - if orig.has_pending_updates() || other.has_pending_updates() { - return Err(Error::InvalidDiffPendingUpdates); - } - let mut tree_diff = TreeDiff::default(); - orig.interface.backing.tree.diff( - &other.interface.backing.tree, - 0, - orig.interface.backing.depth, - &mut tree_diff, - )?; - Ok(Self { - tree_diff, - _phantom: PhantomData, - }) - } - - fn apply_diff(self, target: &mut Self::Target) -> Result<(), Error> { - target - .interface - .backing - .update(self.tree_diff.leaves, Some(self.tree_diff.hashes)) - } -} diff --git a/src/error.rs b/src/error.rs index 56008ea..89a6ef4 100644 --- a/src/error.rs +++ b/src/error.rs @@ -15,13 +15,8 @@ pub enum Error { PushNotSupported, UpdateLeafError, UpdateLeavesError, - InvalidDiffDeleteNotSupported, - InvalidDiffLeaf, - InvalidDiffNode, - InvalidDiffPendingUpdates, InvalidRebaseNode, InvalidRebaseLeaf, - AddToDiffError, BuilderExpectedLeaf, BuilderStackEmptyMerge, BuilderStackEmptyMergeLeft, diff --git a/src/lib.rs b/src/lib.rs index d9c5b73..0ce2cde 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -3,7 +3,6 @@ pub mod builder; pub mod cow; -pub mod diff; pub mod error; pub mod interface; pub mod interface_iter; @@ -20,7 +19,6 @@ pub mod utils; pub mod vector; pub use cow::Cow; -pub use diff::{CloneDiff, Diff, ListDiff, ResetListDiff, VectorDiff}; pub use error::Error; pub use interface::ImmList; pub use leaf::Leaf; diff --git a/src/list.rs b/src/list.rs index d331f7b..1d449a0 100644 --- a/src/list.rs +++ b/src/list.rs @@ -1,5 +1,4 @@ use crate::builder::Builder; -use crate::diff::{Diff, ListDiff}; use crate::interface::{ImmList, Interface, MutList}; use crate::interface_iter::{InterfaceIter, InterfaceIterCow}; use crate::iter::Iter; @@ -264,17 +263,6 @@ impl> List { } Ok(()) } - - pub fn rebase_via_diff(&self, base: &Self) -> Result { - // Diff self from base. - let diff = ListDiff::compute_diff(base, self)?; - - // Apply diff to base, yielding a new list rooted in base. - let mut new = base.clone(); - diff.apply_diff(&mut new)?; - - Ok(new) - } } impl Default for List { diff --git a/src/tests/diff.rs b/src/tests/diff.rs deleted file mode 100644 index 4d4bc68..0000000 --- a/src/tests/diff.rs +++ /dev/null @@ -1,98 +0,0 @@ -use crate::{Diff, Error, List, ListDiff, Value}; -use std::fmt::Debug; -use tree_hash::TreeHash; -use typenum::{Unsigned, U16}; - -fn check_apply(orig: &T, expected: &T, diff: D) -where - T: PartialEq + Debug + Clone, - D: Diff, -{ - let mut updated = orig.clone(); - diff.apply_diff(&mut updated).unwrap(); - assert_eq!(&updated, expected); -} - -fn diff_and_check_apply(orig: &T, updated: &T) -where - T: PartialEq + Debug + Clone, - D: Diff + Debug, -{ - let diff = D::compute_diff(orig, updated).unwrap(); - check_apply(orig, updated, diff); -} - -fn check_confluence(orig: &T, a1: &T, a2: &T, b1: &T, b2: &T) -where - T: PartialEq + Debug + Clone, - D: Diff + PartialEq + Debug, -{ - // Every path to a2 and b2 should be part of a valid diff that reproduces the original. - diff_and_check_apply::<_, D>(orig, a1); - diff_and_check_apply::<_, D>(a1, a2); - diff_and_check_apply::<_, D>(orig, a2); - - diff_and_check_apply::<_, D>(orig, b1); - diff_and_check_apply::<_, D>(b1, b2); - diff_and_check_apply::<_, D>(orig, b2); - - // a2 and b2 should be equal and have equal diffs from orig. - assert_eq!(a2, b2); - let a_diff = D::compute_diff(orig, a2).unwrap(); - let b_diff = D::compute_diff(orig, b2).unwrap(); - assert_eq!(a_diff, b_diff); -} - -fn with_updated_index(list: &List, index: usize, value: T) -> List -where - T: Value + Send + Sync, - N: Unsigned, -{ - let mut updated = list.clone(); - *updated.get_mut(index).unwrap() = value; - - updated.apply_updates().unwrap(); - updated.tree_hash_root(); - updated -} - -fn extended(list: &List, values: Vec) -> List -where - T: Value + Send + Sync, - N: Unsigned, -{ - let mut updated = list.clone(); - for value in values { - updated.push(value).unwrap(); - } - - updated.apply_updates().unwrap(); - updated.tree_hash_root(); - updated -} - -#[test] -fn confluent_diff_list_u64() { - let orig = List::::new(vec![0, 1, 4, 6, 9]).unwrap(); - - let a1 = with_updated_index(&orig, 1, 2); - let a2 = with_updated_index(&a1, 4, 8); - - let b1 = with_updated_index(&orig, 4, 8); - let b2 = with_updated_index(&b1, 1, 2); - - check_confluence::<_, ListDiff<_, _>>(&orig, &a1, &a2, &b1, &b2); -} - -#[test] -fn confluent_diff_list_u64_push_empty() { - let orig = List::::new(vec![]).unwrap(); - - let a1 = extended(&orig, vec![1, 2, 3, 4, 5, 6]); - let a2 = extended(&a1, vec![7, 8, 9, 10, 11]); - - let b1 = extended(&orig, vec![1, 2, 3]); - let b2 = extended(&b1, vec![4, 5, 6, 7, 8, 9, 10, 11]); - - check_confluence::<_, ListDiff<_, _>>(&orig, &a1, &a2, &b1, &b2); -} diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 4d9dab2..c6f2a81 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -1,7 +1,6 @@ #![cfg(test)] mod builder; -mod diff; mod iterator; mod packed; mod proptest; diff --git a/src/tests/proptest/operations.rs b/src/tests/proptest/operations.rs index 5e5c103..f882ddd 100644 --- a/src/tests/proptest/operations.rs +++ b/src/tests/proptest/operations.rs @@ -1,5 +1,5 @@ use super::{arb_hash256, arb_index, arb_large, arb_list, arb_vect, Large}; -use crate::{Diff, Error, List, ListDiff, Value, Vector, VectorDiff}; +use crate::{Error, List, Value, Vector}; use proptest::prelude::*; use ssz::{Decode, Encode}; use std::fmt::Debug; @@ -101,11 +101,9 @@ pub enum Op { ApplyUpdates, /// Compute the tree hash of the list, modifying its internal nodes. TreeHash, - /// Set the current state of the list as the checkpoint for the next diff. - DiffCheckpoint, - /// Compute a diff with respect to the most recent checkpoint and verify its correctness. - DiffCompute, - /// Rebase the list on the most recent (diff) checkpoint. + /// Set the current state of the list as the checkpoint for the next rebase. + Checkpoint, + /// Rebase the list on the most recent checkpoint. Rebase, /// Create a new list which shares no data with its ancestors. Debase, @@ -134,15 +132,14 @@ where Just(Op::TreeHash), ]; let b_block = prop_oneof![ - Just(Op::DiffCheckpoint), - Just(Op::DiffCompute), + Just(Op::Checkpoint), Just(Op::Rebase), Just(Op::Debase), Just(Op::FromIntoRoundtrip) ]; prop_oneof![ 10 => a_block, - 5 => b_block + 4 => b_block ] } @@ -163,7 +160,7 @@ where T: Value + Debug + Send + Sync, N: Unsigned + Debug, { - let mut diff_checkpoint = list.clone(); + let mut checkpoint = list.clone(); for op in ops { match op { @@ -203,24 +200,17 @@ where Op::ApplyUpdates => { list.apply_updates().unwrap(); } - Op::DiffCheckpoint => { + Op::Checkpoint => { list.apply_updates().unwrap(); - diff_checkpoint = list.clone(); + checkpoint = list.clone(); } Op::TreeHash => { list.apply_updates().unwrap(); list.tree_hash_root(); } - Op::DiffCompute => { - list.apply_updates().unwrap(); - let diff = ListDiff::compute_diff(&diff_checkpoint, list).unwrap(); - let mut diffed_list = diff_checkpoint.clone(); - diff.apply_diff(&mut diffed_list).unwrap(); - assert_eq!(diffed_list, *list); - } Op::Rebase => { list.apply_updates().unwrap(); - let new_list = list.rebase(&diff_checkpoint).unwrap(); + let new_list = list.rebase(&checkpoint).unwrap(); assert_eq!(new_list, *list); } Op::Debase => { @@ -248,7 +238,7 @@ where T: Value + Debug + Send + Sync, N: Unsigned + Debug, { - let mut diff_checkpoint = vect.clone(); + let mut checkpoint = vect.clone(); for op in ops { match op { @@ -292,20 +282,13 @@ where vect.apply_updates().unwrap(); vect.tree_hash_root(); } - Op::DiffCheckpoint => { - vect.apply_updates().unwrap(); - diff_checkpoint = vect.clone(); - } - Op::DiffCompute => { + Op::Checkpoint => { vect.apply_updates().unwrap(); - let diff = VectorDiff::compute_diff(&diff_checkpoint, vect).unwrap(); - let mut diffed_vect = diff_checkpoint.clone(); - diff.apply_diff(&mut diffed_vect).unwrap(); - assert_eq!(diffed_vect, *vect); + checkpoint = vect.clone(); } Op::Rebase => { vect.apply_updates().unwrap(); - let new_vect = vect.rebase(&diff_checkpoint).unwrap(); + let new_vect = vect.rebase(&checkpoint).unwrap(); assert_eq!(new_vect, *vect); } Op::Debase => { diff --git a/src/tree.rs b/src/tree.rs index d816e6e..77bb09e 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -4,8 +4,6 @@ use arbitrary::Arbitrary; use derivative::Derivative; use ethereum_hashing::{hash32_concat, ZERO_HASHES}; use parking_lot::RwLock; -use serde::{Deserialize, Serialize}; -use ssz_derive::{Decode, Encode}; use std::collections::BTreeMap; use std::ops::ControlFlow; use tree_hash::Hash256; @@ -250,132 +248,6 @@ pub enum RebaseAction<'a, T> { } impl Tree { - pub fn diff( - &self, - other: &Self, - prefix: usize, - depth: usize, - diff: &mut TreeDiff, - ) -> Result<(), Error> { - match (self, other) { - (Self::Leaf(l1), Self::Leaf(l2)) if depth == 0 => { - if l1.value != l2.value { - let hash = *l2.hash.read(); - diff.hashes.insert((depth, prefix), hash); - diff.leaves.insert(prefix, (*l2.value).clone()); - } - Ok(()) - } - (Self::PackedLeaf(l1), Self::PackedLeaf(l2)) if depth == 0 => { - let mut equal = true; - for i in 0..l2.values.len() { - let v2 = &l2.values[i]; - match l1.values.get(i) { - Some(v1) if v1 == v2 => continue, - _ => { - equal = false; - let index = prefix | i; - diff.leaves.insert(index, v2.clone()); - } - } - } - if !equal { - let hash = *l2.hash.read(); - diff.hashes.insert((depth, prefix), hash); - } - Ok(()) - } - (Self::Zero(z1), Self::Zero(z2)) if z1 == z2 && *z1 == depth => Ok(()), - ( - Self::Node { - hash: h1, - left: l1, - right: r1, - }, - Self::Node { - hash: h2, - left: l2, - right: r2, - }, - ) if depth > 0 => { - let h1 = *h1.read(); - let h2 = *h2.read(); - - // Conditions for recursing: - // - Hashes are different. Implies different subtree data. - // - Left or right pointers are different. Implies subtree *could* contain - // different data with the *same hash* (e.g. leaves equal to 0 value). - // - // The second condition is prone to misfiring on identical trees that have been - // built from scratch in different parts of memory. Wasting some effort - // in this case is considered an acceptable trade-off, because this should occur - // rarely in practice when performing copy-on-write updates to the same tree - // structure. There is no risk of leaving out differences because subtrees that - // contain different data must necessarily have different pointers. - // - // FIXME(sproul): consider alternatives + improvements: - // - check whether subtree diff did anything before adding node hash to diff - // - add subtree length to node (extra 8 bytes) - if h1 != h2 || !Arc::ptr_eq(l1, l2) || !Arc::ptr_eq(r1, r2) { - diff.hashes.insert((depth, prefix), h2); - - let packing_depth = opt_packing_depth::().unwrap_or(0); - let new_depth = depth - 1; - let left_prefix = prefix; - let right_prefix = prefix | (1 << (new_depth + packing_depth)); - - l1.diff(l2, left_prefix, new_depth, diff)?; - r1.diff(r2, right_prefix, new_depth, diff)?; - } - Ok(()) - } - (Self::Zero(_), rhs) => rhs.add_to_diff(prefix, depth, diff), - (_, Self::Zero(_)) => Err(Error::InvalidDiffDeleteNotSupported), - (Self::Leaf(_) | Self::PackedLeaf(_), _) | (_, Self::Leaf(_) | Self::PackedLeaf(_)) => { - Err(Error::InvalidDiffLeaf) - } - (Self::Node { .. }, Self::Node { .. }) => Err(Error::InvalidDiffNode), - } - } - - /// Add every node in this subtree to the diff. - fn add_to_diff( - &self, - prefix: usize, - depth: usize, - diff: &mut TreeDiff, - ) -> Result<(), Error> { - match self { - Self::Leaf(leaf) if depth == 0 => { - diff.hashes.insert((depth, prefix), *leaf.hash.read()); - diff.leaves.insert(prefix, (*leaf.value).clone()); - Ok(()) - } - Self::PackedLeaf(packed_leaf) if depth == 0 => { - diff.hashes - .insert((depth, prefix), *packed_leaf.hash.read()); - for (i, value) in packed_leaf.values.iter().enumerate() { - diff.leaves.insert(prefix | i, value.clone()); - } - Ok(()) - } - Self::Node { hash, left, right } if depth > 0 => { - diff.hashes.insert((depth, prefix), *hash.read()); - - let packing_depth = opt_packing_depth::().unwrap_or(0); - let new_depth = depth - 1; - let left_prefix = prefix; - let right_prefix = prefix | (1 << (new_depth + packing_depth)); - - left.add_to_diff(left_prefix, new_depth, diff)?; - right.add_to_diff(right_prefix, new_depth, diff)?; - Ok(()) - } - Self::Zero(_) => Ok(()), - _ => Err(Error::AddToDiffError), - } - } - pub fn rebase_on<'a>( orig: &'a Arc, base: &'a Arc, @@ -519,14 +391,6 @@ impl Tree { } } -#[derive(Debug, PartialEq, Encode, Decode, Deserialize, Serialize, Derivative)] -#[derivative(Default(bound = "T: Value"))] -pub struct TreeDiff { - pub leaves: BTreeMap, - /// Map from `(depth, prefix)` to node hash. - pub hashes: BTreeMap<(usize, usize), Hash256>, -} - impl Tree { pub fn tree_hash(&self) -> Hash256 { match self { diff --git a/src/vector.rs b/src/vector.rs index c30439f..7316bff 100644 --- a/src/vector.rs +++ b/src/vector.rs @@ -1,4 +1,3 @@ -use crate::diff::{Diff, VectorDiff}; use crate::interface::{ImmList, Interface, MutList}; use crate::interface_iter::InterfaceIter; use crate::iter::Iter; @@ -161,17 +160,6 @@ impl> Vector { } Ok(()) } - - pub fn rebase_via_diff(&self, base: &Self) -> Result { - // Diff self from base. - let diff = VectorDiff::compute_diff(base, self)?; - - // Apply diff to base, yielding a new list rooted in base. - let mut new = base.clone(); - diff.apply_diff(&mut new)?; - - Ok(new) - } } impl> From> for List {