From bf95b91d7c172be74e65ae2d7f632105099e0ca2 Mon Sep 17 00:00:00 2001 From: Oleg Bondar Date: Tue, 30 Jan 2024 16:34:19 +0000 Subject: [PATCH 1/4] Add read only storage access sketch --- .../sov-modules-core/src/storage/mod.rs | 1 + .../src/storage/read_access.rs | 31 +++++++++++++++++++ .../src/storage/scratchpad.rs | 13 ++++++++ 3 files changed, 45 insertions(+) create mode 100644 module-system/sov-modules-core/src/storage/read_access.rs diff --git a/module-system/sov-modules-core/src/storage/mod.rs b/module-system/sov-modules-core/src/storage/mod.rs index 2023893e2..00a468464 100644 --- a/module-system/sov-modules-core/src/storage/mod.rs +++ b/module-system/sov-modules-core/src/storage/mod.rs @@ -12,6 +12,7 @@ use crate::common::{AlignedVec, Prefix, Version, Witness}; mod cache; mod codec; +mod read_access; mod scratchpad; pub use cache::*; diff --git a/module-system/sov-modules-core/src/storage/read_access.rs b/module-system/sov-modules-core/src/storage/read_access.rs new file mode 100644 index 000000000..eedae5d0f --- /dev/null +++ b/module-system/sov-modules-core/src/storage/read_access.rs @@ -0,0 +1,31 @@ +use crate::{Storage, StorageKey, StorageValue}; + +pub trait StateReader { + /// Get a value from the storage. + fn get(&self, key: &StorageKey) -> Option; +} + +pub trait AsReadonly { + /// Readonly version of the access. + type Readonly; + + /// Performs the conversion. + fn as_readonly(&self) -> Self::Readonly; +} + +pub struct StateSnapshot { + inner: S, +} + +impl StateSnapshot { + pub fn new(inner: S) -> Self { + Self { inner } + } +} + +impl StateReader for StateSnapshot { + fn get(&self, key: &StorageKey) -> Option { + // TODO propagate witness + self.inner.get(key, None, Default::default()) + } +} diff --git a/module-system/sov-modules-core/src/storage/scratchpad.rs b/module-system/sov-modules-core/src/storage/scratchpad.rs index 3fb315b52..cad88aa2d 100644 --- a/module-system/sov-modules-core/src/storage/scratchpad.rs +++ b/module-system/sov-modules-core/src/storage/scratchpad.rs @@ -10,6 +10,7 @@ use sov_rollup_interface::stf::Event; use crate::archival_state::{ArchivalAccessoryWorkingSet, ArchivalJmtWorkingSet}; use crate::common::{GasMeter, Prefix}; use crate::module::{Context, Spec}; +use crate::storage::read_access::{AsReadonly, StateSnapshot}; use crate::storage::{ CacheKey, CacheValue, EncodeKeyLike, NativeStorage, OrderedReadsAndWrites, StateCodec, StateValueCodec, Storage, StorageInternalCache, StorageKey, StorageProof, StorageValue, @@ -335,6 +336,18 @@ impl StateCheckpoint { } } +impl AsReadonly for StateCheckpoint { + type Readonly = StateSnapshot; + + fn as_readonly(&self) -> Self::Readonly { + assert!( + self.delta.cache.tx_cache.is_empty(), + "There shouldn't be uncommitted changes to prevent dirty reads" + ); + StateSnapshot::new(self.delta.inner.clone()) + } +} + /// 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. From 4b25be55f1d122fae00ee6504c7b5aad759855f2 Mon Sep 17 00:00:00 2001 From: Oleg Bondar Date: Tue, 30 Jan 2024 16:47:19 +0000 Subject: [PATCH 2/4] improve --- .../src/storage/read_access.rs | 9 +++++++- .../src/storage/scratchpad.rs | 23 +++++++++++++------ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/module-system/sov-modules-core/src/storage/read_access.rs b/module-system/sov-modules-core/src/storage/read_access.rs index eedae5d0f..714924b7b 100644 --- a/module-system/sov-modules-core/src/storage/read_access.rs +++ b/module-system/sov-modules-core/src/storage/read_access.rs @@ -10,7 +10,14 @@ pub trait AsReadonly { type Readonly; /// Performs the conversion. - fn as_readonly(&self) -> Self::Readonly; + fn as_readonly(&self, level: Option) -> Self::Readonly; +} + +pub enum IsolationLevel { + /// Read isolation level. + ReadCommitted, + /// Write isolation level. + DirtyRead, } pub struct StateSnapshot { diff --git a/module-system/sov-modules-core/src/storage/scratchpad.rs b/module-system/sov-modules-core/src/storage/scratchpad.rs index cad88aa2d..1d45fdb3f 100644 --- a/module-system/sov-modules-core/src/storage/scratchpad.rs +++ b/module-system/sov-modules-core/src/storage/scratchpad.rs @@ -10,7 +10,7 @@ use sov_rollup_interface::stf::Event; use crate::archival_state::{ArchivalAccessoryWorkingSet, ArchivalJmtWorkingSet}; use crate::common::{GasMeter, Prefix}; use crate::module::{Context, Spec}; -use crate::storage::read_access::{AsReadonly, StateSnapshot}; +use crate::storage::read_access::{AsReadonly, IsolationLevel, StateSnapshot}; use crate::storage::{ CacheKey, CacheValue, EncodeKeyLike, NativeStorage, OrderedReadsAndWrites, StateCodec, StateValueCodec, Storage, StorageInternalCache, StorageKey, StorageProof, StorageValue, @@ -339,12 +339,21 @@ impl StateCheckpoint { impl AsReadonly for StateCheckpoint { type Readonly = StateSnapshot; - fn as_readonly(&self) -> Self::Readonly { - assert!( - self.delta.cache.tx_cache.is_empty(), - "There shouldn't be uncommitted changes to prevent dirty reads" - ); - StateSnapshot::new(self.delta.inner.clone()) + fn as_readonly(&self, level: Option) -> Self::Readonly { + let level = level.unwrap_or_else(|| IsolationLevel::ReadCommitted); + match level { + IsolationLevel::ReadCommitted => + // read just from storage + { + StateSnapshot::new(self.delta.inner.clone()) + } + + IsolationLevel::DirtyRead => + // this is tricky because we need to propagate cache to the snapshot + { + StateSnapshot::new(self.delta.inner.clone()) + } + } } } From 6e0211c3360c443e67f338884bd0eec7a961de95 Mon Sep 17 00:00:00 2001 From: Oleg Bondar Date: Tue, 30 Jan 2024 16:57:59 +0000 Subject: [PATCH 3/4] add doc --- .../sov-modules-core/src/storage/scratchpad.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/module-system/sov-modules-core/src/storage/scratchpad.rs b/module-system/sov-modules-core/src/storage/scratchpad.rs index 1d45fdb3f..4246e2a4f 100644 --- a/module-system/sov-modules-core/src/storage/scratchpad.rs +++ b/module-system/sov-modules-core/src/storage/scratchpad.rs @@ -340,19 +340,15 @@ impl AsReadonly for StateCheckpoint { type Readonly = StateSnapshot; fn as_readonly(&self, level: Option) -> Self::Readonly { + // TODO think about uncommitted changes and how to handle them. There are three options + // 1. Return a snapshot of the current state, ignoring uncommitted changes + // 2. Return a snapshot of the current state, but also include uncommitted changes + // 3. Return an error + let level = level.unwrap_or_else(|| IsolationLevel::ReadCommitted); match level { - IsolationLevel::ReadCommitted => - // read just from storage - { - StateSnapshot::new(self.delta.inner.clone()) - } - - IsolationLevel::DirtyRead => - // this is tricky because we need to propagate cache to the snapshot - { - StateSnapshot::new(self.delta.inner.clone()) - } + IsolationLevel::ReadCommitted => StateSnapshot::new(self.delta.inner.clone()), + IsolationLevel::DirtyRead => StateSnapshot::new(self.delta.inner.clone()), // TODO clone cache? } } } From d8c668cb3890d4fa3e640d7be1647c85d8914c0d Mon Sep 17 00:00:00 2001 From: Oleg Bondar Date: Tue, 30 Jan 2024 21:04:35 +0000 Subject: [PATCH 4/4] minor --- module-system/sov-modules-core/src/storage/read_access.rs | 5 +++-- module-system/sov-modules-core/src/storage/scratchpad.rs | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/module-system/sov-modules-core/src/storage/read_access.rs b/module-system/sov-modules-core/src/storage/read_access.rs index 714924b7b..6f23b3d78 100644 --- a/module-system/sov-modules-core/src/storage/read_access.rs +++ b/module-system/sov-modules-core/src/storage/read_access.rs @@ -32,7 +32,8 @@ impl StateSnapshot { impl StateReader for StateSnapshot { fn get(&self, key: &StorageKey) -> Option { - // TODO propagate witness - self.inner.get(key, None, Default::default()) + // TODO propagate witness? + let witness: S::Witness = Default::default(); + self.inner.get(key, None, &witness) } } diff --git a/module-system/sov-modules-core/src/storage/scratchpad.rs b/module-system/sov-modules-core/src/storage/scratchpad.rs index 4246e2a4f..42b289f01 100644 --- a/module-system/sov-modules-core/src/storage/scratchpad.rs +++ b/module-system/sov-modules-core/src/storage/scratchpad.rs @@ -336,8 +336,8 @@ impl StateCheckpoint { } } -impl AsReadonly for StateCheckpoint { - type Readonly = StateSnapshot; +impl AsReadonly for StateCheckpoint { + type Readonly = StateSnapshot<::Storage>; fn as_readonly(&self, level: Option) -> Self::Readonly { // TODO think about uncommitted changes and how to handle them. There are three options