From 1d8ac46e06779926d490772d9e4adbb235fe2c75 Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Thu, 28 Sep 2023 18:29:53 +0200 Subject: [PATCH] Delta's go first, remove `get` from StateCheckpoint (#953) --- .../sov-modules-api/src/state/scratchpad.rs | 221 +++++++++--------- 1 file changed, 108 insertions(+), 113 deletions(-) diff --git a/module-system/sov-modules-api/src/state/scratchpad.rs b/module-system/sov-modules-api/src/state/scratchpad.rs index 0f24ee058..11c32f281 100644 --- a/module-system/sov-modules-api/src/state/scratchpad.rs +++ b/module-system/sov-modules-api/src/state/scratchpad.rs @@ -60,6 +60,53 @@ impl StateReaderAndWriter for Delta { type RevertableWrites = HashMap>; +struct AccessoryDelta { + // This inner storage is never accessed inside the zkVM because reads are + // not allowed, so it can result as dead code. + #[allow(dead_code)] + storage: S, + writes: RevertableWrites, +} + +impl AccessoryDelta { + fn new(storage: S) -> Self { + Self { + storage, + writes: Default::default(), + } + } + + fn freeze(&mut self) -> OrderedReadsAndWrites { + let mut reads_and_writes = OrderedReadsAndWrites::default(); + let writes = std::mem::take(&mut self.writes); + + for write in writes { + reads_and_writes.ordered_writes.push((write.0, write.1)); + } + + reads_and_writes + } +} + +impl StateReaderAndWriter for AccessoryDelta { + fn get(&mut self, key: &StorageKey) -> Option { + let cache_key = key.to_cache_key(); + if let Some(value) = self.writes.get(&cache_key) { + return value.clone().map(Into::into); + } + self.storage.get_accessory(key) + } + + fn set(&mut self, key: &StorageKey, value: StorageValue) { + self.writes + .insert(key.to_cache_key(), Some(value.into_cache_value())); + } + + fn delete(&mut self, key: &StorageKey) { + self.writes.insert(key.to_cache_key(), None); + } +} + /// This structure is responsible for storing the `read-write` set. /// /// A [`StateCheckpoint`] can be obtained from a [`WorkingSet`] in two ways: @@ -80,11 +127,6 @@ impl StateCheckpoint { } } - /// Fetches a value from the underlying storage. - pub fn get(&mut self, key: &StorageKey) -> Option { - self.delta.get(key) - } - /// Creates a new [`StateCheckpoint`] instance without any changes, backed /// by the given [`Storage`] and witness. pub fn with_witness( @@ -131,61 +173,82 @@ impl StateCheckpoint { } } -struct AccessoryDelta { - // This inner storage is never accessed inside the zkVM because reads are - // not allowed, so it can result as dead code. - #[allow(dead_code)] - storage: S, - writes: RevertableWrites, +/// This structure contains the read-write set and the events collected during the execution of a transaction. +/// There are two ways to convert it into a StateCheckpoint: +/// 1. By using the checkpoint() method, where all the changes are added to the underlying StateCheckpoint. +/// 2. By using the revert method, where the most recent changes are reverted and the previous `StateCheckpoint` is returned. +pub struct WorkingSet { + delta: RevertableWriter>, + accessory_delta: RevertableWriter>, + events: Vec, } -impl AccessoryDelta { - fn new(storage: S) -> Self { - Self { - storage, - writes: Default::default(), - } +impl WorkingSet { + /// Creates a new [`WorkingSet`] instance backed by the given [`Storage`]. + /// + /// The witness value is set to [`Default::default`]. Use + /// [`WorkingSet::with_witness`] to set a custom witness value. + pub fn new(inner: ::Storage) -> Self { + StateCheckpoint::new(inner).to_revertable() } - fn freeze(&mut self) -> OrderedReadsAndWrites { - let mut reads_and_writes = OrderedReadsAndWrites::default(); - let writes = std::mem::take(&mut self.writes); + /// Returns a handler for the accessory state (non-JMT state). + /// + /// You can use this method when calling getters and setters on accessory + /// state containers, like [`AccessoryStateMap`](crate::AccessoryStateMap). + pub fn accessory_state(&mut self) -> AccessoryWorkingSet { + AccessoryWorkingSet { ws: self } + } - for write in writes { - reads_and_writes.ordered_writes.push((write.0, write.1)); - } + /// Creates a new [`WorkingSet`] instance backed by the given [`Storage`] + /// and a custom witness value. + pub fn with_witness( + inner: ::Storage, + witness: <::Storage as Storage>::Witness, + ) -> Self { + StateCheckpoint::with_witness(inner, witness).to_revertable() + } - reads_and_writes + /// Turns this [`WorkingSet`] into a [`StateCheckpoint`], in preparation for + /// committing the changes to the underlying [`Storage`] via + /// [`StateCheckpoint::freeze`]. + pub fn checkpoint(self) -> StateCheckpoint { + StateCheckpoint { + delta: self.delta.commit(), + accessory_delta: self.accessory_delta.commit(), + } } -} -impl StateReaderAndWriter for AccessoryDelta { - fn get(&mut self, key: &StorageKey) -> Option { - let cache_key = key.to_cache_key(); - if let Some(value) = self.writes.get(&cache_key) { - return value.clone().map(Into::into); + /// Reverts the most recent changes to this [`WorkingSet`], returning a pristine + /// [`StateCheckpoint`] instance. + pub fn revert(self) -> StateCheckpoint { + StateCheckpoint { + delta: self.delta.revert(), + accessory_delta: self.accessory_delta.revert(), } - self.storage.get_accessory(key) } - fn set(&mut self, key: &StorageKey, value: StorageValue) { - self.writes - .insert(key.to_cache_key(), Some(value.into_cache_value())); + /// Adds an event to the working set. + pub fn add_event(&mut self, key: &str, value: &str) { + self.events.push(Event::new(key, value)); } - fn delete(&mut self, key: &StorageKey) { - self.writes.insert(key.to_cache_key(), None); + /// Extracts all events from this working set. + pub fn take_events(&mut self) -> Vec { + std::mem::take(&mut self.events) } -} -/// This structure contains the read-write set and the events collected during the execution of a transaction. -/// There are two ways to convert it into a StateCheckpoint: -/// 1. By using the checkpoint() method, where all the changes are added to the underlying StateCheckpoint. -/// 2. By using the revert method, where the most recent changes are reverted and the previous `StateCheckpoint` is returned. -pub struct WorkingSet { - delta: RevertableWriter>, - accessory_delta: RevertableWriter>, - events: Vec, + /// Returns an immutable slice of all events that have been previously + /// written to this working set. + pub fn events(&self) -> &[Event] { + &self.events + } + + /// Returns an immutable reference to the [`Storage`] instance backing this + /// working set. + pub fn backing(&self) -> &::Storage { + &self.delta.inner.inner + } } impl StateReaderAndWriter for WorkingSet { @@ -286,74 +349,6 @@ impl StateReaderAndWriter for RevertableWriter { } } -impl WorkingSet { - /// Creates a new [`WorkingSet`] instance backed by the given [`Storage`]. - /// - /// The witness value is set to [`Default::default`]. Use - /// [`WorkingSet::with_witness`] to set a custom witness value. - pub fn new(inner: ::Storage) -> Self { - StateCheckpoint::new(inner).to_revertable() - } - - /// Returns a handler for the accessory state (non-JMT state). - /// - /// You can use this method when calling getters and setters on accessory - /// state containers, like [`AccessoryStateMap`](crate::AccessoryStateMap). - pub fn accessory_state(&mut self) -> AccessoryWorkingSet { - AccessoryWorkingSet { ws: self } - } - - /// Creates a new [`WorkingSet`] instance backed by the given [`Storage`] - /// and a custom witness value. - pub fn with_witness( - inner: ::Storage, - witness: <::Storage as Storage>::Witness, - ) -> Self { - StateCheckpoint::with_witness(inner, witness).to_revertable() - } - - /// Turns this [`WorkingSet`] into a [`StateCheckpoint`], in preparation for - /// committing the changes to the underlying [`Storage`] via - /// [`StateCheckpoint::freeze`]. - pub fn checkpoint(self) -> StateCheckpoint { - StateCheckpoint { - delta: self.delta.commit(), - accessory_delta: self.accessory_delta.commit(), - } - } - - /// Reverts the most recent changes to this [`WorkingSet`], returning a pristine - /// [`StateCheckpoint`] instance. - pub fn revert(self) -> StateCheckpoint { - StateCheckpoint { - delta: self.delta.revert(), - accessory_delta: self.accessory_delta.revert(), - } - } - - /// Adds an event to the working set. - pub fn add_event(&mut self, key: &str, value: &str) { - self.events.push(Event::new(key, value)); - } - - /// Extracts all events from this working set. - pub fn take_events(&mut self) -> Vec { - std::mem::take(&mut self.events) - } - - /// Returns an immutable slice of all events that have been previously - /// written to this working set. - pub fn events(&self) -> &[Event] { - &self.events - } - - /// Returns an immutable reference to the [`Storage`] instance backing this - /// working set. - pub fn backing(&self) -> &::Storage { - &self.delta.inner.inner - } -} - pub(crate) trait StateReaderAndWriter { fn get(&mut self, key: &StorageKey) -> Option;