From d1743d7bd7da3e89dde9cc20bd75d9f8b4677ed7 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 22 Jan 2024 13:44:47 +0800 Subject: [PATCH 1/2] apply updates recursively --- src/interface.rs | 5 +++ src/lib.rs | 18 +++++++++-- src/list.rs | 8 ++++- src/tests/mod.rs | 1 + src/tests/proptest/mod.rs | 10 +++++- src/tests/recursion.rs | 66 +++++++++++++++++++++++++++++++++++++++ src/update_map.rs | 20 ++++++++++++ src/vector.rs | 8 ++++- 8 files changed, 131 insertions(+), 5 deletions(-) create mode 100644 src/tests/recursion.rs diff --git a/src/interface.rs b/src/interface.rs index ba3c5a2..a48ae31 100644 --- a/src/interface.rs +++ b/src/interface.rs @@ -80,8 +80,13 @@ where Ok(()) } + pub fn apply_recursive_updates(&mut self) -> Result<(), Error> { + self.updates.for_each_mut(|item| item.apply()) + } + pub fn apply_updates(&mut self) -> Result<(), Error> { if !self.updates.is_empty() { + self.apply_recursive_updates()?; let updates = std::mem::take(&mut self.updates); self.backing.update(updates, None) } else { diff --git a/src/lib.rs b/src/lib.rs index 0ce2cde..0ed7acc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -32,6 +32,20 @@ pub use vector::Vector; use ssz::{Decode, Encode}; use tree_hash::TreeHash; -pub trait Value: Encode + Decode + TreeHash + PartialEq + Clone {} +pub trait PendingUpdates { + fn apply(&mut self) -> Result<(), Error> { + Ok(()) + } +} -impl Value for T where T: Encode + Decode + TreeHash + PartialEq + Clone {} +pub trait Value: Encode + Decode + TreeHash + PartialEq + Clone + PendingUpdates {} + +impl Value for T where T: Encode + Decode + TreeHash + PartialEq + Clone + PendingUpdates {} + +// Default impls for known types +impl PendingUpdates for u8 {} +impl PendingUpdates for u16 {} +impl PendingUpdates for u32 {} +impl PendingUpdates for u64 {} +impl PendingUpdates for u128 {} +impl PendingUpdates for tree_hash::Hash256 {} diff --git a/src/list.rs b/src/list.rs index 1d449a0..8edba02 100644 --- a/src/list.rs +++ b/src/list.rs @@ -6,7 +6,7 @@ use crate::serde::ListVisitor; use crate::tree::RebaseAction; use crate::update_map::MaxMap; use crate::utils::{arb_arc, int_log, opt_packing_depth, updated_length, Length}; -use crate::{Arc, Cow, Error, Tree, UpdateMap, Value}; +use crate::{Arc, Cow, Error, PendingUpdates, Tree, UpdateMap, Value}; use arbitrary::Arbitrary; use derivative::Derivative; use itertools::process_results; @@ -39,6 +39,12 @@ pub struct ListInner { _phantom: PhantomData, } +impl> PendingUpdates for List { + fn apply(&mut self) -> Result<(), Error> { + self.interface.apply_updates() + } +} + impl> List { pub fn new(vec: Vec) -> Result { Self::try_from_iter(vec) diff --git a/src/tests/mod.rs b/src/tests/mod.rs index c6f2a81..2bb6bc3 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -4,5 +4,6 @@ mod builder; mod iterator; mod packed; mod proptest; +mod recursion; mod repeat; mod size_of; diff --git a/src/tests/proptest/mod.rs b/src/tests/proptest/mod.rs index 991219b..c30eb5f 100644 --- a/src/tests/proptest/mod.rs +++ b/src/tests/proptest/mod.rs @@ -1,4 +1,4 @@ -use crate::List; +use crate::{List, PendingUpdates}; use proptest::prelude::*; use ssz_derive::{Decode, Encode}; use tree_hash::Hash256; @@ -44,6 +44,14 @@ pub struct Large { d: List, } +impl PendingUpdates for Large { + fn apply(&mut self) -> Result<(), crate::Error> { + // TODO use macro derive + self.d.apply()?; + Ok(()) + } +} + pub fn arb_large() -> impl Strategy { ( any::(), diff --git a/src/tests/recursion.rs b/src/tests/recursion.rs new file mode 100644 index 0000000..3672437 --- /dev/null +++ b/src/tests/recursion.rs @@ -0,0 +1,66 @@ +use crate::{List, PendingUpdates}; +use ssz_derive::{Decode, Encode}; +use tree_hash::TreeHash; +use tree_hash_derive::TreeHash; +use typenum::U16; + +#[test] +fn recursive_list_list() { + let mut l = List::, U16>::default(); + + l.push(<_>::default()).unwrap(); + l.get_mut(0).unwrap().push(1).unwrap(); + l.apply_updates().unwrap(); + // assert it does not throw + let h_1 = l.tree_hash_root(); + + assert!(!l.has_pending_updates()); + assert_eq!(*l.get(0).unwrap().get(0).unwrap(), 1); + + // Replace value + *l.get_mut(0).unwrap().get_mut(0).unwrap() = 2; + assert_eq!(*l.get(0).unwrap().get(0).unwrap(), 2); + + // Commit only top list + l.apply_updates().unwrap(); + // Root should be different + assert_ne!(h_1, l.tree_hash_root()); +} + +/// Struct with multiple fields shared by multiple proptests. +#[derive(Default, Debug, Clone, PartialEq, Encode, Decode, TreeHash)] +pub struct ListContainer { + a: u8, + b: List, +} + +impl PendingUpdates for ListContainer { + fn apply(&mut self) -> Result<(), crate::Error> { + // TODO use macro derive + self.b.apply()?; + Ok(()) + } +} + +#[test] +fn recursive_list_container_list() { + let mut l = List::::default(); + + l.push(<_>::default()).unwrap(); + l.get_mut(0).unwrap().b.push(1).unwrap(); + l.apply_updates().unwrap(); + // assert it does not throw + let h_1 = l.tree_hash_root(); + + assert!(!l.has_pending_updates()); + assert_eq!(*l.get(0).unwrap().b.get(0).unwrap(), 1); + + // Replace value + *l.get_mut(0).unwrap().b.get_mut(0).unwrap() = 2; + assert_eq!(*l.get(0).unwrap().b.get(0).unwrap(), 2); + + // Commit only top list + l.apply_updates().unwrap(); + // Root should be different + assert_ne!(h_1, l.tree_hash_root()); +} diff --git a/src/update_map.rs b/src/update_map.rs index 27945a8..7fc806c 100644 --- a/src/update_map.rs +++ b/src/update_map.rs @@ -25,6 +25,8 @@ pub trait UpdateMap: Default + Clone { where F: FnMut(usize, &T) -> ControlFlow<(), Result<(), E>>; + fn for_each_mut Result<(), E>>(&mut self, f: F) -> Result<(), E>; + fn max_index(&self) -> Option; fn len(&self) -> usize; @@ -90,6 +92,13 @@ impl UpdateMap for BTreeMap { Ok(()) } + fn for_each_mut Result<(), E>>(&mut self, mut f: F) -> Result<(), E> { + for (_, item) in self.iter_mut() { + f(item)?; + } + Ok(()) + } + fn max_index(&self) -> Option { max_btree_index(self) } @@ -159,6 +168,13 @@ impl UpdateMap for VecMap { Ok(()) } + fn for_each_mut Result<(), E>>(&mut self, mut f: F) -> Result<(), E> { + for (_, item) in self.iter_mut() { + f(item)?; + } + Ok(()) + } + fn max_index(&self) -> Option { // FIXME(sproul): this is slow, make a wrapper type that tracks the max index self.keys().next_back() @@ -214,6 +230,10 @@ where self.inner.for_each_range(start, end, f) } + fn for_each_mut Result<(), E>>(&mut self, f: F) -> Result<(), E> { + self.inner.for_each_mut(f) + } + fn len(&self) -> usize { self.inner.len() } diff --git a/src/vector.rs b/src/vector.rs index 7316bff..50c92ec 100644 --- a/src/vector.rs +++ b/src/vector.rs @@ -4,7 +4,7 @@ use crate::iter::Iter; use crate::tree::RebaseAction; use crate::update_map::MaxMap; use crate::utils::{arb_arc, Length}; -use crate::{Arc, Cow, Error, List, Tree, UpdateMap, Value}; +use crate::{Arc, Cow, Error, List, PendingUpdates, Tree, UpdateMap, Value}; use arbitrary::Arbitrary; use derivative::Derivative; use serde::{Deserialize, Serialize}; @@ -40,6 +40,12 @@ pub struct VectorInner { _phantom: PhantomData, } +impl> PendingUpdates for Vector { + fn apply(&mut self) -> Result<(), Error> { + self.interface.apply_updates() + } +} + impl> Vector { pub fn new(vec: Vec) -> Result { if vec.len() == N::to_usize() { From f558cb94f40d53d0b7a2db44406c0b49c227d0d9 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 22 Jan 2024 15:45:29 +0800 Subject: [PATCH 2/2] Return early for lists of basic types --- src/interface.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/interface.rs b/src/interface.rs index a48ae31..510a8bf 100644 --- a/src/interface.rs +++ b/src/interface.rs @@ -8,7 +8,7 @@ use crate::{ use arbitrary::Arbitrary; use std::collections::BTreeMap; use std::marker::PhantomData; -use tree_hash::Hash256; +use tree_hash::{Hash256, TreeHashType}; pub trait ImmList { fn get(&self, idx: usize) -> Option<&T>; @@ -81,7 +81,16 @@ where } pub fn apply_recursive_updates(&mut self) -> Result<(), Error> { - self.updates.for_each_mut(|item| item.apply()) + let is_recursive = match T::tree_hash_type() { + TreeHashType::Basic => false, + TreeHashType::Container | TreeHashType::List | TreeHashType::Vector => true, + }; + + if is_recursive { + self.updates.for_each_mut(|item| item.apply()) + } else { + Ok(()) + } } pub fn apply_updates(&mut self) -> Result<(), Error> {