From e354ae949964001a39a4d91761d0973775f5d7c4 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Wed, 28 Jun 2023 14:57:41 +0200 Subject: [PATCH] Make triecache generic and work for no-std environments (#14403) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Remove `AsLocalTrieCache` trait * Introduce new trait AsTrieDbCache * Use AsTrieDbCache trait * Make it compile * Docs * Improve naming of associated type, implement cache usage for no-std * Improve naming * Improve docs * Allow construction with optional cache * FMT * Remove unused variable * Revert unwanted change * Apply suggestions from code review Co-authored-by: Bastian Köcher * More comment adjustments * Docs * Trigger CI * ".git/.scripts/commands/fmt/fmt.sh" * Apply suggestions from code review Co-authored-by: Koute * Review comments * Review comments * Apply suggestions from code review Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> * fmt * Bump trie-db again --------- Co-authored-by: Bastian Köcher Co-authored-by: command-bot <> Co-authored-by: Koute Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> --- primitives/state-machine/Cargo.toml | 3 +- primitives/state-machine/src/lib.rs | 2 + primitives/state-machine/src/trie_backend.rs | 179 +++++++++++++----- .../state-machine/src/trie_backend_essence.rs | 90 ++++----- primitives/trie/src/cache/mod.rs | 2 +- 5 files changed, 183 insertions(+), 93 deletions(-) diff --git a/primitives/state-machine/Cargo.toml b/primitives/state-machine/Cargo.toml index eb5e2645e148e..4bdd665b2984d 100644 --- a/primitives/state-machine/Cargo.toml +++ b/primitives/state-machine/Cargo.toml @@ -27,13 +27,13 @@ sp-externalities = { version = "0.19.0", default-features = false, path = "../ex sp-panic-handler = { version = "8.0.0", optional = true, path = "../panic-handler" } sp-std = { version = "8.0.0", default-features = false, path = "../std" } sp-trie = { version = "22.0.0", default-features = false, path = "../trie" } +trie-db = { version = "0.27.1", default-features = false } [dev-dependencies] array-bytes = "4.1" pretty_assertions = "1.2.1" rand = "0.8.5" sp-runtime = { version = "24.0.0", path = "../runtime" } -trie-db = "0.27.1" assert_matches = "1.5" [features] @@ -49,6 +49,7 @@ std = [ "sp-panic-handler", "sp-std/std", "sp-trie/std", + "trie-db/std", "thiserror", "tracing", ] diff --git a/primitives/state-machine/src/lib.rs b/primitives/state-machine/src/lib.rs index 0001d0026c394..cc7de9080e3d0 100644 --- a/primitives/state-machine/src/lib.rs +++ b/primitives/state-machine/src/lib.rs @@ -36,6 +36,8 @@ mod testing; mod trie_backend; mod trie_backend_essence; +pub use trie_backend::TrieCacheProvider; + #[cfg(feature = "std")] pub use std_reexport::*; diff --git a/primitives/state-machine/src/trie_backend.rs b/primitives/state-machine/src/trie_backend.rs index abd58b383969a..b7940fa8c39df 100644 --- a/primitives/state-machine/src/trie_backend.rs +++ b/primitives/state-machine/src/trie_backend.rs @@ -24,61 +24,142 @@ use crate::{ trie_backend_essence::{RawIter, TrieBackendEssence, TrieBackendStorage}, Backend, StorageKey, StorageValue, }; + use codec::Codec; #[cfg(feature = "std")] use hash_db::HashDB; use hash_db::Hasher; use sp_core::storage::{ChildInfo, StateVersion}; #[cfg(feature = "std")] -use sp_trie::{cache::LocalTrieCache, recorder::Recorder}; -#[cfg(feature = "std")] -use sp_trie::{MemoryDB, StorageProof}; - -/// Dummy type to be used in `no_std`. -/// -/// This is required to have the type available for [`TrieBackendBuilder`] and [`TrieBackend`]. +use sp_trie::{ + cache::{LocalTrieCache, TrieCache}, + recorder::Recorder, + MemoryDB, StorageProof, +}; #[cfg(not(feature = "std"))] -pub struct LocalTrieCache(sp_std::marker::PhantomData); +use sp_trie::{Error, NodeCodec}; +use trie_db::TrieCache as TrieCacheT; +#[cfg(not(feature = "std"))] +use trie_db::{node::NodeOwned, CachedValue}; -/// Special trait to support taking the [`LocalTrieCache`] by value or by reference. -/// -/// This trait is internal use only and to emphasize this, the trait is sealed. -pub trait AsLocalTrieCache: sealed::Sealed { - /// Returns `self` as [`LocalTrieCache`]. - #[cfg(feature = "std")] - fn as_local_trie_cache(&self) -> &LocalTrieCache; +/// A provider of trie caches that are compatible with [`trie_db::TrieDB`]. +pub trait TrieCacheProvider { + /// Cache type that implements [`trie_db::TrieCache`]. + type Cache<'a>: TrieCacheT> + 'a + where + Self: 'a; + + /// Return a [`trie_db::TrieDB`] compatible cache. + /// + /// The `storage_root` parameter should be the storage root of the used trie. + fn as_trie_db_cache(&self, storage_root: H::Out) -> Self::Cache<'_>; + + /// Returns a cache that can be used with a [`trie_db::TrieDBMut`]. + /// + /// When finished with the operation on the trie, it is required to call [`Self::merge`] to + /// merge the cached items for the correct `storage_root`. + fn as_trie_db_mut_cache(&self) -> Self::Cache<'_>; + + /// Merge the cached data in `other` into the provider using the given `new_root`. + /// + /// This must be used for the cache returned by [`Self::as_trie_db_mut_cache`] as otherwise the + /// cached data is just thrown away. + fn merge<'a>(&'a self, other: Self::Cache<'a>, new_root: H::Out); } -impl AsLocalTrieCache for LocalTrieCache { - #[cfg(feature = "std")] - #[inline] - fn as_local_trie_cache(&self) -> &LocalTrieCache { - self +#[cfg(feature = "std")] +impl TrieCacheProvider for LocalTrieCache { + type Cache<'a> = TrieCache<'a, H> where H: 'a; + + fn as_trie_db_cache(&self, storage_root: H::Out) -> Self::Cache<'_> { + self.as_trie_db_cache(storage_root) + } + + fn as_trie_db_mut_cache(&self) -> Self::Cache<'_> { + self.as_trie_db_mut_cache() + } + + fn merge<'a>(&'a self, other: Self::Cache<'a>, new_root: H::Out) { + other.merge_into(self, new_root) } } #[cfg(feature = "std")] -impl AsLocalTrieCache for &LocalTrieCache { - #[inline] - fn as_local_trie_cache(&self) -> &LocalTrieCache { - self +impl TrieCacheProvider for &LocalTrieCache { + type Cache<'a> = TrieCache<'a, H> where Self: 'a; + + fn as_trie_db_cache(&self, storage_root: H::Out) -> Self::Cache<'_> { + (*self).as_trie_db_cache(storage_root) + } + + fn as_trie_db_mut_cache(&self) -> Self::Cache<'_> { + (*self).as_trie_db_mut_cache() + } + + fn merge<'a>(&'a self, other: Self::Cache<'a>, new_root: H::Out) { + other.merge_into(self, new_root) } } -/// Special module that contains the `Sealed` trait. -mod sealed { - use super::*; +/// Cache provider that allows construction of a [`TrieBackend`] and satisfies the requirements, but +/// can never be instantiated. +#[cfg(not(feature = "std"))] +pub struct UnimplementedCacheProvider { + // Not strictly necessary, but the H bound allows to use this as a drop-in + // replacement for the `LocalTrieCache` in no-std contexts. + _phantom: core::marker::PhantomData, + // Statically prevents construction. + _infallible: core::convert::Infallible, +} + +#[cfg(not(feature = "std"))] +impl trie_db::TrieCache> for UnimplementedCacheProvider { + fn lookup_value_for_key(&mut self, _key: &[u8]) -> Option<&CachedValue> { + unimplemented!() + } - /// A special trait which prevents externals to implement the [`AsLocalTrieCache`] outside - /// of this crate. - pub trait Sealed {} + fn cache_value_for_key(&mut self, _key: &[u8], _value: CachedValue) { + unimplemented!() + } - impl Sealed for LocalTrieCache {} - impl Sealed for &LocalTrieCache {} + fn get_or_insert_node( + &mut self, + _hash: H::Out, + _fetch_node: &mut dyn FnMut() -> trie_db::Result, H::Out, Error>, + ) -> trie_db::Result<&NodeOwned, H::Out, Error> { + unimplemented!() + } + + fn get_node(&mut self, _hash: &H::Out) -> Option<&NodeOwned> { + unimplemented!() + } } +#[cfg(not(feature = "std"))] +impl TrieCacheProvider for UnimplementedCacheProvider { + type Cache<'a> = UnimplementedCacheProvider where H: 'a; + + fn as_trie_db_cache(&self, _storage_root: ::Out) -> Self::Cache<'_> { + unimplemented!() + } + + fn as_trie_db_mut_cache(&self) -> Self::Cache<'_> { + unimplemented!() + } + + fn merge<'a>(&'a self, _other: Self::Cache<'a>, _new_root: ::Out) { + unimplemented!() + } +} + +#[cfg(feature = "std")] +type DefaultCache = LocalTrieCache; + +#[cfg(not(feature = "std"))] +type DefaultCache = UnimplementedCacheProvider; + /// Builder for creating a [`TrieBackend`]. -pub struct TrieBackendBuilder, H: Hasher, C = LocalTrieCache> { +pub struct TrieBackendBuilder, H: Hasher, C = DefaultCache> { storage: S, root: H::Out, #[cfg(feature = "std")] @@ -86,7 +167,7 @@ pub struct TrieBackendBuilder, H: Hasher, C = LocalTrie cache: Option, } -impl TrieBackendBuilder> +impl TrieBackendBuilder> where S: TrieBackendStorage, H: Hasher, @@ -108,6 +189,16 @@ where S: TrieBackendStorage, H: Hasher, { + /// Create a new builder instance. + pub fn new_with_cache(storage: S, root: H::Out, cache: C) -> Self { + Self { + storage, + root, + #[cfg(feature = "std")] + recorder: None, + cache: Some(cache), + } + } /// Wrap the given [`TrieBackend`]. /// /// This can be used for example if all accesses to the trie should @@ -121,10 +212,7 @@ where root: *other.essence.root(), #[cfg(feature = "std")] recorder: None, - #[cfg(feature = "std")] cache: other.essence.trie_node_cache.as_ref(), - #[cfg(not(feature = "std"))] - cache: None, } } @@ -141,23 +229,23 @@ where } /// Use the given optional `cache` for the to be configured [`TrieBackend`]. - #[cfg(feature = "std")] pub fn with_optional_cache(self, cache: Option) -> TrieBackendBuilder { TrieBackendBuilder { cache, root: self.root, storage: self.storage, + #[cfg(feature = "std")] recorder: self.recorder, } } /// Use the given `cache` for the to be configured [`TrieBackend`]. - #[cfg(feature = "std")] pub fn with_cache(self, cache: LC) -> TrieBackendBuilder { TrieBackendBuilder { cache: Some(cache), root: self.root, storage: self.storage, + #[cfg(feature = "std")] recorder: self.recorder, } } @@ -179,10 +267,8 @@ where /// Build the configured [`TrieBackend`]. #[cfg(not(feature = "std"))] pub fn build(self) -> TrieBackend { - let _ = self.cache; - TrieBackend { - essence: TrieBackendEssence::new(self.storage, self.root), + essence: TrieBackendEssence::new_with_cache(self.storage, self.root, self.cache), next_storage_key_cache: Default::default(), } } @@ -223,12 +309,13 @@ fn access_cache(cell: &CacheCell, callback: impl FnOnce(&mut T) -> R) - } /// Patricia trie-based backend. Transaction type is an overlay of changes to commit. -pub struct TrieBackend, H: Hasher, C = LocalTrieCache> { +pub struct TrieBackend, H: Hasher, C = DefaultCache> { pub(crate) essence: TrieBackendEssence, next_storage_key_cache: CacheCell>>, } -impl, H: Hasher, C: AsLocalTrieCache + Send + Sync> TrieBackend +impl, H: Hasher, C: TrieCacheProvider + Send + Sync> + TrieBackend where H::Out: Codec, { @@ -276,7 +363,7 @@ where } } -impl, H: Hasher, C: AsLocalTrieCache> sp_std::fmt::Debug +impl, H: Hasher, C: TrieCacheProvider> sp_std::fmt::Debug for TrieBackend { fn fmt(&self, f: &mut sp_std::fmt::Formatter<'_>) -> sp_std::fmt::Result { @@ -284,7 +371,7 @@ impl, H: Hasher, C: AsLocalTrieCache> sp_std::fmt::D } } -impl, H: Hasher, C: AsLocalTrieCache + Send + Sync> Backend +impl, H: Hasher, C: TrieCacheProvider + Send + Sync> Backend for TrieBackend where H::Out: Ord + Codec, diff --git a/primitives/state-machine/src/trie_backend_essence.rs b/primitives/state-machine/src/trie_backend_essence.rs index 1f6d71b2dce80..22c76b56deb05 100644 --- a/primitives/state-machine/src/trie_backend_essence.rs +++ b/primitives/state-machine/src/trie_backend_essence.rs @@ -20,7 +20,7 @@ use crate::{ backend::{Consolidate, IterArgs, StorageIterator}, - trie_backend::AsLocalTrieCache, + trie_backend::TrieCacheProvider, warn, StorageKey, StorageValue, }; use codec::Codec; @@ -39,7 +39,6 @@ use sp_trie::{ }; #[cfg(feature = "std")] use std::{collections::HashMap, sync::Arc}; - // In this module, we only use layout for read operation and empty root, // where V1 and V0 are equivalent. use sp_trie::LayoutV1 as Layout; @@ -100,7 +99,7 @@ where H: Hasher, S: TrieBackendStorage, H::Out: Codec + Ord, - C: AsLocalTrieCache + Send + Sync, + C: TrieCacheProvider + Send + Sync, { #[inline] fn prepare( @@ -160,7 +159,7 @@ where H: Hasher, S: TrieBackendStorage, H::Out: Codec + Ord, - C: AsLocalTrieCache + Send + Sync, + C: TrieCacheProvider + Send + Sync, { type Backend = crate::TrieBackend; type Error = crate::DefaultError; @@ -209,29 +208,28 @@ pub struct TrieBackendEssence, H: Hasher, C> { empty: H::Out, #[cfg(feature = "std")] pub(crate) cache: Arc>>, - #[cfg(feature = "std")] pub(crate) trie_node_cache: Option, #[cfg(feature = "std")] pub(crate) recorder: Option>, - #[cfg(not(feature = "std"))] - _phantom: PhantomData, } impl, H: Hasher, C> TrieBackendEssence { /// Create new trie-based backend. pub fn new(storage: S, root: H::Out) -> Self { + Self::new_with_cache(storage, root, None) + } + + /// Create new trie-based backend. + pub fn new_with_cache(storage: S, root: H::Out, cache: Option) -> Self { TrieBackendEssence { storage, root, empty: H::hash(&[0u8]), #[cfg(feature = "std")] cache: Arc::new(RwLock::new(Cache::new())), - #[cfg(feature = "std")] - trie_node_cache: None, + trie_node_cache: cache, #[cfg(feature = "std")] recorder: None, - #[cfg(not(feature = "std"))] - _phantom: PhantomData, } } @@ -289,11 +287,10 @@ impl, H: Hasher, C> TrieBackendEssence { } } -impl, H: Hasher, C: AsLocalTrieCache> TrieBackendEssence { +impl, H: Hasher, C: TrieCacheProvider> TrieBackendEssence { /// Call the given closure passing it the recorder and the cache. /// /// If the given `storage_root` is `None`, `self.root` will be used. - #[cfg(feature = "std")] #[inline] fn with_recorder_and_cache( &self, @@ -304,32 +301,23 @@ impl, H: Hasher, C: AsLocalTrieCache> TrieBackendEss ) -> R, ) -> R { let storage_root = storage_root.unwrap_or_else(|| self.root); - let mut recorder = self.recorder.as_ref().map(|r| r.as_trie_recorder(storage_root)); - let recorder = match recorder.as_mut() { - Some(recorder) => Some(recorder as &mut dyn TrieRecorder), - None => None, - }; - - let mut cache = self - .trie_node_cache - .as_ref() - .map(|c| c.as_local_trie_cache().as_trie_db_cache(storage_root)); + let mut cache = self.trie_node_cache.as_ref().map(|c| c.as_trie_db_cache(storage_root)); let cache = cache.as_mut().map(|c| c as _); - callback(recorder, cache) - } + #[cfg(feature = "std")] + { + let mut recorder = self.recorder.as_ref().map(|r| r.as_trie_recorder(storage_root)); + let recorder = match recorder.as_mut() { + Some(recorder) => Some(recorder as &mut dyn TrieRecorder), + None => None, + }; + callback(recorder, cache) + } - #[cfg(not(feature = "std"))] - #[inline] - fn with_recorder_and_cache( - &self, - _: Option, - callback: impl FnOnce( - Option<&mut dyn TrieRecorder>, - Option<&mut dyn TrieCache>>, - ) -> R, - ) -> R { - callback(None, None) + #[cfg(not(feature = "std"))] + { + callback(None, cache) + } } /// Call the given closure passing it the recorder and the cache. @@ -356,12 +344,12 @@ impl, H: Hasher, C: AsLocalTrieCache> TrieBackendEss }; let result = if let Some(local_cache) = self.trie_node_cache.as_ref() { - let mut cache = local_cache.as_local_trie_cache().as_trie_db_mut_cache(); + let mut cache = local_cache.as_trie_db_mut_cache(); let (new_root, r) = callback(recorder, Some(&mut cache)); if let Some(new_root) = new_root { - cache.merge_into(local_cache.as_local_trie_cache(), new_root); + local_cache.merge(cache, new_root); } r @@ -375,17 +363,29 @@ impl, H: Hasher, C: AsLocalTrieCache> TrieBackendEss #[cfg(not(feature = "std"))] fn with_recorder_and_cache_for_storage_root( &self, - _: Option, + _storage_root: Option, callback: impl FnOnce( Option<&mut dyn TrieRecorder>, Option<&mut dyn TrieCache>>, ) -> (Option, R), ) -> R { - callback(None, None).1 + if let Some(local_cache) = self.trie_node_cache.as_ref() { + let mut cache = local_cache.as_trie_db_mut_cache(); + + let (new_root, r) = callback(None, Some(&mut cache)); + + if let Some(new_root) = new_root { + local_cache.merge(cache, new_root); + } + + r + } else { + callback(None, None).1 + } } } -impl, H: Hasher, C: AsLocalTrieCache + Send + Sync> +impl, H: Hasher, C: TrieCacheProvider + Send + Sync> TrieBackendEssence where H::Out: Codec + Ord, @@ -805,8 +805,8 @@ where } } -impl, H: Hasher, C: AsLocalTrieCache + Send + Sync> AsHashDB - for TrieBackendEssence +impl, H: Hasher, C: TrieCacheProvider + Send + Sync> + AsHashDB for TrieBackendEssence { fn as_hash_db<'b>(&'b self) -> &'b (dyn HashDB + 'b) { self @@ -816,7 +816,7 @@ impl, H: Hasher, C: AsLocalTrieCache + Send + Sync> } } -impl, H: Hasher, C: AsLocalTrieCache + Send + Sync> HashDB +impl, H: Hasher, C: TrieCacheProvider + Send + Sync> HashDB for TrieBackendEssence { fn get(&self, key: &H::Out, prefix: Prefix) -> Option { @@ -849,7 +849,7 @@ impl, H: Hasher, C: AsLocalTrieCache + Send + Sync> } } -impl, H: Hasher, C: AsLocalTrieCache + Send + Sync> +impl, H: Hasher, C: TrieCacheProvider + Send + Sync> HashDBRef for TrieBackendEssence { fn get(&self, key: &H::Out, prefix: Prefix) -> Option { diff --git a/primitives/trie/src/cache/mod.rs b/primitives/trie/src/cache/mod.rs index 380ce4b8a46e4..01f08a78adcf2 100644 --- a/primitives/trie/src/cache/mod.rs +++ b/primitives/trie/src/cache/mod.rs @@ -529,7 +529,7 @@ impl<'a, H: Hasher> TrieCache<'a, H> { /// `storage_root` is the new storage root that was obtained after finishing all operations /// using the [`TrieDBMut`](trie_db::TrieDBMut). pub fn merge_into(self, local: &LocalTrieCache, storage_root: H::Out) { - let cache = if let ValueCache::Fresh(cache) = self.value_cache { cache } else { return }; + let ValueCache::Fresh(cache) = self.value_cache else { return }; if !cache.is_empty() { let mut value_cache = local.value_cache.lock();