Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Use trie-cache for validate_block #2813

Merged
merged 30 commits into from
Jul 19, 2023
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
23ac3a0
Simple cache
skunert Jun 22, 2023
7c5f862
Fix node insertion
skunert Jun 23, 2023
3658334
Switch to hashbrown hashmap
skunert Jun 23, 2023
9b6fff5
Remove unused phantomdata
skunert Jun 26, 2023
fc8ef69
Return error when fetch_node fails
skunert Jun 27, 2023
2fdddaf
Merge branch 'master' into skunert/trie-cache
skunert Jun 27, 2023
3f94561
Remove cargo patches
skunert Jun 29, 2023
259dd4e
Merge branch 'master' into skunert/trie-cache
skunert Jun 29, 2023
d1a1058
Move trie cache to extra module
skunert Jun 29, 2023
59b7b7f
Add ReadOnceBackend
skunert Jun 29, 2023
07cbd1c
Add readonlybackend
skunert Jun 29, 2023
c461e41
Merge branch 'master' into skunert/trie-cache
skunert Jun 30, 2023
2491f68
Improve naming and get_or_insert
skunert Jun 30, 2023
869138a
Stylistic improvements
skunert Jun 30, 2023
9e77c4d
Improve naming, add docs
skunert Jun 30, 2023
9e181f8
Merge branch 'master' into skunert/trie-cache
skunert Jul 3, 2023
abef482
Revert unwanted changes
skunert Jul 3, 2023
9422541
Remove unused dependencies
skunert Jul 3, 2023
297b5a3
Improve docs
skunert Jul 3, 2023
aa13d51
Use RefCell
skunert Jul 3, 2023
9fb05c0
lockfile
skunert Jul 3, 2023
8148892
Remove ReadOnceBackend
skunert Jul 4, 2023
d78f377
Merge branch 'master' into skunert/trie-cache
skunert Jul 4, 2023
c283268
Apply suggestions from code review
skunert Jul 5, 2023
7f1d06e
Code review
skunert Jul 5, 2023
56e24a6
Do not use value cache when calculating storage roots
skunert Jul 5, 2023
6bdbb82
Apply suggestions from code review
skunert Jul 18, 2023
827f6e4
Remove hash-db dep
skunert Jul 18, 2023
16f257d
Merge branch 'master' into skunert/trie-cache
skunert Jul 18, 2023
4b47f06
Update pallets/parachain-system/src/validate_block/trie_cache.rs
skunert Jul 19, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions pallets/parachain-system/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ codec = { package = "parity-scale-codec", version = "3.0.0", default-features =
environmental = { version = "1.1.4", default-features = false }
impl-trait-for-tuples = "0.2.1"
log = { version = "0.4.19", default-features = false }
trie-db = { version = "0.27.1", default-features = false }
hash-db = { version = "0.16.0", default-features = false }
hashbrown = "0.14.0"
scale-info = { version = "2.9.0", default-features = false, features = ["derive"] }

# Substrate
Expand Down Expand Up @@ -69,6 +72,8 @@ std = [
"sp-std/std",
"sp-trie/std",
"xcm/std",
"trie-db/std",
"hash-db/std"
]

runtime-benchmarks = [
Expand Down
21 changes: 17 additions & 4 deletions pallets/parachain-system/src/validate_block/implementation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

//! The actual implementation of the validate block functionality.

use super::MemoryOptimizedValidationParams;
use super::{trie_cache, MemoryOptimizedValidationParams};
use cumulus_primitives_core::{
relay_chain::Hash as RHash, ParachainBlockData, PersistedValidationData,
};
Expand All @@ -34,14 +34,22 @@ use sp_runtime::traits::{Block as BlockT, Extrinsic, HashFor, Header as HeaderT}
use sp_std::prelude::*;
use sp_trie::MemoryDB;

type TrieBackend<B> = sp_state_machine::TrieBackend<MemoryDB<HashFor<B>>, HashFor<B>>;
type TrieBackend<B> = sp_state_machine::TrieBackend<
MemoryDB<HashFor<B>>,
HashFor<B>,
trie_cache::CacheProvider<HashFor<B>>,
>;

type Ext<'a, B> = sp_state_machine::Ext<'a, HashFor<B>, TrieBackend<B>>;

fn with_externalities<F: FnOnce(&mut dyn Externalities) -> R, R>(f: F) -> R {
sp_externalities::with_externalities(f).expect("Environmental externalities not set.")
}

use core::marker::PhantomData;
use hash_db::Hasher;
use sp_trie::NodeCodec;

skunert marked this conversation as resolved.
Show resolved Hide resolved
/// Validate the given parachain block.
///
/// This function is doing roughly the following:
Expand Down Expand Up @@ -114,10 +122,15 @@ where

sp_std::mem::drop(storage_proof);

let cache_provider = trie_cache::CacheProvider::new();
// We use the storage root of the `parent_head` to ensure that it is the correct root.
// This is already being done above while creating the in-memory db, but let's be paranoid!!
let backend =
sp_state_machine::TrieBackendBuilder::new(db, *parent_header.state_root()).build();
let backend = sp_state_machine::TrieBackendBuilder::new_with_cache(
db,
*parent_header.state_root(),
cache_provider,
)
.build();

let _guard = (
// Replace storage calls with our own implementations
Expand Down
4 changes: 4 additions & 0 deletions pallets/parachain-system/src/validate_block/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ pub mod implementation;
#[cfg(test)]
mod tests;

#[cfg(not(feature = "std"))]
#[doc(hidden)]
mod trie_cache;

#[cfg(not(feature = "std"))]
#[doc(hidden)]
pub use bytes;
Expand Down
110 changes: 110 additions & 0 deletions pallets/parachain-system/src/validate_block/trie_cache.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
// This file is part of Substrate.

// Copyright (C) Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

extern crate alloc;

skunert marked this conversation as resolved.
Show resolved Hide resolved
use core::cell::RefCell;
skunert marked this conversation as resolved.
Show resolved Hide resolved
use hash_db::{HashDB, Hasher};
use hashbrown::hash_map::{Entry, HashMap};
use sp_state_machine::{TrieBackendStorage, TrieCacheProvider};
use sp_std::boxed::Box;
use sp_trie::NodeCodec;
use trie_db::{node::NodeOwned, TrieCache, TrieError};

/// Special purpose trie cache implementation that is able to cache an unlimited number
/// of values. To be used in `validate_block` to serve values and nodes that
/// have already been loaded and decoded from the storage proof.
pub(crate) struct SimpleTrieCache<'a, H: Hasher> {
skunert marked this conversation as resolved.
Show resolved Hide resolved
node_cache: core::cell::RefMut<'a, HashMap<H::Out, NodeOwned<H::Out>>>,
value_cache: core::cell::RefMut<'a, HashMap<Box<[u8]>, trie_db::CachedValue<H::Out>>>,
skunert marked this conversation as resolved.
Show resolved Hide resolved
}

impl<'a, H: Hasher> trie_db::TrieCache<NodeCodec<H>> for SimpleTrieCache<'a, H> {
fn lookup_value_for_key(&mut self, key: &[u8]) -> Option<&trie_db::CachedValue<H::Out>> {
self.value_cache.get(key)
}

fn cache_value_for_key(&mut self, key: &[u8], value: trie_db::CachedValue<H::Out>) {
self.value_cache.insert(key.into(), value);
}

fn get_or_insert_node(
&mut self,
hash: <NodeCodec<H> as trie_db::NodeCodec>::HashOut,
fetch_node: &mut dyn FnMut() -> trie_db::Result<
NodeOwned<H::Out>,
H::Out,
<NodeCodec<H> as trie_db::NodeCodec>::Error,
>,
) -> trie_db::Result<&NodeOwned<H::Out>, H::Out, <NodeCodec<H> as trie_db::NodeCodec>::Error> {
match self.node_cache.entry(hash) {
Entry::Occupied(entry) => Ok(entry.into_mut()),
Entry::Vacant(entry) => {
let fetched = match fetch_node() {
Ok(new_node) => new_node,
Err(e) => return Err(e),
};
Ok(entry.insert(fetched))
skunert marked this conversation as resolved.
Show resolved Hide resolved
},
}
}

fn get_node(
&mut self,
hash: &H::Out,
) -> Option<&NodeOwned<<NodeCodec<H> as trie_db::NodeCodec>::HashOut>> {
self.node_cache.get(hash)
}
}

/// Provider of [`SimpleTrieCache`] instances.
pub(crate) struct CacheProvider<H: Hasher> {
node_cache: RefCell<HashMap<H::Out, NodeOwned<H::Out>>>,
value_cache: RefCell<HashMap<Box<[u8]>, trie_db::CachedValue<H::Out>>>,
}

impl<H: Hasher> CacheProvider<H> {
/// Constructs a new instance of CacheProvider with an uninitialized state
skunert marked this conversation as resolved.
Show resolved Hide resolved
/// and empty node and value caches.
pub fn new() -> Self {
CacheProvider { node_cache: Default::default(), value_cache: Default::default() }
}
}

impl<H: Hasher> TrieCacheProvider<H> for CacheProvider<H> {
type Cache<'a> = SimpleTrieCache<'a, H> where H: 'a;

fn as_trie_db_cache(&self, _storage_root: <H as Hasher>::Out) -> Self::Cache<'_> {
SimpleTrieCache {
value_cache: self.value_cache.borrow_mut(),
node_cache: self.node_cache.borrow_mut(),
}
}

fn as_trie_db_mut_cache(&self) -> Self::Cache<'_> {
SimpleTrieCache {
value_cache: self.value_cache.borrow_mut(),
node_cache: self.node_cache.borrow_mut(),
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is called when we call storage_root. The values that are being created while calculating the storage_root should not be added to the value_cache as otherwise they may would overwrite some cached value from the "real trie" (we only distinguish using the key and the key stays the same in each trie). If another operation then would want to fetch this overwritten value, it would get some invalid data.

The nodes are accessed by their hash, so that should be no problem. However, the question in general is if we even need to cache these nodes. These nodes will never be accessible from the "storage root" we are operating on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value cache is now disabled when building storage roots. Due to how get_or_insert_node works I think it does still make sense to take ownership of the fetched node and cache it.


fn merge<'a>(&'a self, _other: Self::Cache<'a>, _new_root: <H as Hasher>::Out) {}
}

// This is safe here since we are single-threaded in WASM
unsafe impl<H: Hasher> Send for CacheProvider<H> {}
unsafe impl<H: Hasher> Sync for CacheProvider<H> {}