From d5c14272d23323e5e7bc2af5dfb0655c716027c7 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Thu, 22 Dec 2022 14:27:55 +0100 Subject: [PATCH 01/37] Worker --- client/api/src/backend.rs | 6 +++++ client/api/src/in_mem.rs | 8 ++++++ client/db/src/lib.rs | 8 ++++++ client/service/src/builder.rs | 3 ++- client/service/src/client/client.rs | 42 +++++++++++++++++++++++------ client/state-db/src/lib.rs | 8 +++--- test-utils/client/src/lib.rs | 1 + 7 files changed, 64 insertions(+), 12 deletions(-) diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs index 79cc0d7a16bcc..c0bfc785399b8 100644 --- a/client/api/src/backend.rs +++ b/client/api/src/backend.rs @@ -502,6 +502,12 @@ pub trait Backend: AuxStore + Send + Sync { /// Returns a handle to offchain storage. fn offchain_storage(&self) -> Option; + /// Pin the block to prevent pruning. + fn pin_block(&self, hash: &Block::Hash) -> sp_blockchain::Result<()>; + + /// Unpin the block to allow pruning. + fn unpin_block(&self, hash: &Block::Hash) -> sp_blockchain::Result<()>; + /// Returns true if state for given block is available. fn have_state_at(&self, hash: Block::Hash, _number: NumberFor) -> bool { self.state_at(hash).is_ok() diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs index 5a3e25ab5987b..b0dd9d5620635 100644 --- a/client/api/src/in_mem.rs +++ b/client/api/src/in_mem.rs @@ -790,6 +790,14 @@ where fn requires_full_sync(&self) -> bool { false } + + fn pin_block(&self, hash: &::Hash) -> blockchain::Result<()> { + todo!() + } + + fn unpin_block(&self, hash: &::Hash) -> blockchain::Result<()> { + todo!() + } } impl backend::LocalBackend for Backend where Block::Hash: Ord {} diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 426876f5cba8c..28cb6d1e8adc4 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -2398,6 +2398,14 @@ impl sc_client_api::backend::Backend for Backend { PruningMode::ArchiveAll | PruningMode::ArchiveCanonical ) } + + fn pin_block(&self, hash: &::Hash) -> sp_blockchain::Result<()> { + todo!() + } + + fn unpin_block(&self, hash: &::Hash) -> sp_blockchain::Result<()> { + todo!() + } } impl sc_client_api::backend::LocalBackend for Backend {} diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index 7153672030d6a..bb4851497ef05 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -309,12 +309,13 @@ where let executor = crate::client::LocalCallExecutor::new( backend.clone(), executor, - spawn_handle, + spawn_handle.clone(), config.clone(), execution_extensions, )?; crate::client::Client::new( backend, + spawn_handle.clone(), executor, genesis_storage, fork_blocks, diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index 8ded5ec95c166..82b11f6e2c5d5 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -57,10 +57,14 @@ use sp_blockchain::{ }; use sp_consensus::{BlockOrigin, BlockStatus, Error as ConsensusError}; +use futures::{FutureExt, StreamExt}; use sc_utils::mpsc::{tracing_unbounded, TracingUnboundedSender}; -use sp_core::storage::{ - well_known_keys, ChildInfo, ChildType, PrefixedStorageKey, Storage, StorageChild, StorageData, - StorageKey, +use sp_core::{ + storage::{ + well_known_keys, ChildInfo, ChildType, PrefixedStorageKey, Storage, StorageChild, + StorageData, StorageKey, + }, + traits::SpawnNamed, }; #[cfg(feature = "test-helpers")] use sp_keystore::SyncCryptoStorePtr; @@ -88,9 +92,7 @@ use std::{ #[cfg(feature = "test-helpers")] use { - super::call_executor::LocalCallExecutor, - sc_client_api::in_mem, - sp_core::traits::{CodeExecutor, SpawnNamed}, + super::call_executor::LocalCallExecutor, sc_client_api::in_mem, sp_core::traits::CodeExecutor, }; type NotificationSinks = Mutex>>; @@ -116,6 +118,7 @@ where block_rules: BlockRules, config: ClientConfig, telemetry: Option, + unpin_worker_sender: futures::channel::mpsc::Sender, _phantom: PhantomData, } @@ -239,13 +242,14 @@ where let call_executor = LocalCallExecutor::new( backend.clone(), executor, - spawn_handle, + spawn_handle.clone(), config.clone(), extensions, )?; Client::new( backend, + spawn_handle, call_executor, build_genesis_storage, Default::default(), @@ -341,7 +345,7 @@ where impl Client where - B: backend::Backend, + B: backend::Backend + 'static, E: CallExecutor, Block: BlockT, Block::Header: Clone, @@ -349,6 +353,7 @@ where /// Creates new Substrate Client with given blockchain and code executor. pub fn new( backend: Arc, + spawn_handle: Box, executor: E, build_genesis_storage: &dyn BuildStorage, fork_blocks: ForkBlocks, @@ -389,6 +394,23 @@ where backend.commit_operation(op)?; } + let (unpin_worker_sender, mut rx) = futures::channel::mpsc::channel::(100); + let task_backend = backend.clone(); + spawn_handle.spawn( + "pinning-worker", + None, + async move { + log::info!(target: "db", "Starting worker task for unpinning."); + loop { + if let Some(message) = rx.next().await { + log::info!(target: "db", "Unpinning block {}", message); + task_backend.unpin_block(&message); + } + } + } + .boxed(), + ); + Ok(Client { backend, executor, @@ -401,6 +423,7 @@ where block_rules: BlockRules::new(fork_blocks, bad_blocks), config, telemetry, + unpin_worker_sender, _phantom: Default::default(), }) } @@ -939,6 +962,9 @@ where }, }; + log::info!(target: "db", "Finality notification for block {}, pinning block", notification.hash); + self.backend.pin_block(¬ification.hash); + telemetry!( self.telemetry; SUBSTRATE_INFO; diff --git a/client/state-db/src/lib.rs b/client/state-db/src/lib.rs index 94d41787701b3..61f6b4a047ef9 100644 --- a/client/state-db/src/lib.rs +++ b/client/state-db/src/lib.rs @@ -187,8 +187,9 @@ impl fmt::Debug for StateDbError { Self::TooManySiblingBlocks => write!(f, "Too many sibling blocks inserted"), Self::BlockAlreadyExists => write!(f, "Block already exists"), Self::Metadata(message) => write!(f, "Invalid metadata: {}", message), - Self::BlockUnavailable => - write!(f, "Trying to get a block record from db while it is not commit to db yet"), + Self::BlockUnavailable => { + write!(f, "Trying to get a block record from db while it is not commit to db yet") + }, Self::BlockMissing => write!(f, "Block record is missing from the pruning window"), } } @@ -344,6 +345,7 @@ impl StateDbSync { if let Some(ref mut pruning) = self.pruning { pruning.note_canonical(hash, number, &mut commit)?; } + log::info!(target: "skunert", "Looking to prune block {:?}", hash); self.prune(&mut commit)?; Ok(commit) } @@ -381,7 +383,7 @@ impl StateDbSync { (&mut self.pruning, &self.mode) { loop { - if pruning.window_size() <= constraints.max_blocks.unwrap_or(0) as u64 { + if dbg!(pruning.window_size()) <= dbg!(constraints.max_blocks.unwrap_or(0) as u64) { break } diff --git a/test-utils/client/src/lib.rs b/test-utils/client/src/lib.rs index 8ee652abe2c70..2212bf70c0ed9 100644 --- a/test-utils/client/src/lib.rs +++ b/test-utils/client/src/lib.rs @@ -225,6 +225,7 @@ impl let client = client::Client::new( self.backend.clone(), + todo!(), executor, &storage, self.fork_blocks, From 6131598823d91e6851bf0854036290a1925d1481 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Fri, 6 Jan 2023 09:26:37 +0100 Subject: [PATCH 02/37] Reorganize and unpin onnotification drop --- client/api/src/client.rs | 46 +++++++++++++-- client/db/src/lib.rs | 15 ++++- client/finality-grandpa/src/until_imported.rs | 3 +- client/service/src/builder.rs | 23 +++++++- client/service/src/client/client.rs | 59 +++++++++++-------- client/state-db/src/lib.rs | 2 +- test-utils/client/src/lib.rs | 2 +- 7 files changed, 111 insertions(+), 39 deletions(-) diff --git a/client/api/src/client.rs b/client/api/src/client.rs index 05e3163dcc7bd..a0a127eee56f8 100644 --- a/client/api/src/client.rs +++ b/client/api/src/client.rs @@ -18,6 +18,7 @@ //! A set of APIs supported by the client along with their primitives. +use futures::channel::mpsc::Sender; use sp_consensus::BlockOrigin; use sp_core::storage::StorageKey; use sp_runtime::{ @@ -265,6 +266,26 @@ impl fmt::Display for UsageInfo { } } +#[derive(Debug)] +pub struct UnpinHandle { + hash: Block::Hash, + unpin_worker_sender: Sender, +} + +impl UnpinHandle { + pub fn new(hash: Block::Hash, unpin_worker_sender: Sender) -> Self { + Self { hash, unpin_worker_sender } + } +} + +impl Drop for UnpinHandle { + fn drop(&mut self) { + if let Err(err) = self.unpin_worker_sender.try_send(self.hash) { + log::error!(target: "db", "Unable to unpin block with hash: {}, error: {:?}", self.hash, err); + }; + } +} + /// Summary of an imported block #[derive(Clone, Debug)] pub struct BlockImportNotification { @@ -280,6 +301,8 @@ pub struct BlockImportNotification { /// /// If `None`, there was no re-org while importing. pub tree_route: Option>>, + /// Handle to unpin the block this notification is for + _unpin_handle: Arc>, } /// Summary of a finalized block. @@ -295,6 +318,8 @@ pub struct FinalityNotification { pub tree_route: Arc<[Block::Hash]>, /// Stale branches heads. pub stale_heads: Arc<[Block::Hash]>, + /// Handle to unpin the block this notification is for + _unpin_handle: Arc>, } impl TryFrom> for ChainEvent { @@ -315,26 +340,37 @@ impl From> for ChainEvent { } } -impl From> for FinalityNotification { - fn from(mut summary: FinalizeSummary) -> Self { +impl FinalityNotification { + /// Create finality notification from finality summary. + pub fn from_summary( + mut summary: FinalizeSummary, + unpin_worker_sender: Sender, + ) -> FinalityNotification { let hash = summary.finalized.pop().unwrap_or_default(); FinalityNotification { hash, header: summary.header, tree_route: Arc::from(summary.finalized), stale_heads: Arc::from(summary.stale_heads), + _unpin_handle: Arc::new(UnpinHandle { hash, unpin_worker_sender }), } } } -impl From> for BlockImportNotification { - fn from(summary: ImportSummary) -> Self { +impl BlockImportNotification { + /// Create finality notification from finality summary. + pub fn from_summary( + summary: ImportSummary, + unpin_worker_sender: Sender, + ) -> BlockImportNotification { + let hash = summary.hash; BlockImportNotification { - hash: summary.hash, + hash, origin: summary.origin, header: summary.header, is_new_best: summary.is_new_best, tree_route: summary.tree_route.map(Arc::new), + _unpin_handle: Arc::new(UnpinHandle { hash, unpin_worker_sender }), } } } diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 28cb6d1e8adc4..3aad35f9a80f6 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -1041,6 +1041,7 @@ impl FrozenForDuration { /// Disk backend keeps data in a key-value store. In archive mode, trie nodes are kept from all /// blocks. Otherwise, trie nodes are kept only from some recent blocks. pub struct Backend { + pinned_block_store: RwLock>, storage: Arc>, offchain_storage: offchain::LocalStorage, blockchain: BlockchainDb, @@ -1155,6 +1156,7 @@ impl Backend { let offchain_storage = offchain::LocalStorage::new(db.clone()); let backend = Backend { + pinned_block_store: RwLock::new(HashSet::new()), storage: Arc::new(storage_db), offchain_storage, blockchain, @@ -2400,11 +2402,20 @@ impl sc_client_api::backend::Backend for Backend { } fn pin_block(&self, hash: &::Hash) -> sp_blockchain::Result<()> { - todo!() + let mut handle = self.pinned_block_store.write(); + handle.insert(*hash); + log::info!(target: "db", "Pinning block in backend, hash: {}, num_pinned_blocks: {}", hash, handle.len()); + log::info!(target: "skunert", "Pinned_block_contents: {:?}", handle); + let hint = || false; + self.storage.state_db.pin(hash, number, hint); + Ok(()) } fn unpin_block(&self, hash: &::Hash) -> sp_blockchain::Result<()> { - todo!() + log::info!(target: "db", "Unpinning block in backend, hash: {}", hash); + let mut handle = self.pinned_block_store.write(); + handle.remove(hash); + Ok(()) } } diff --git a/client/finality-grandpa/src/until_imported.rs b/client/finality-grandpa/src/until_imported.rs index df0b63348e94b..8a879fdd67d92 100644 --- a/client/finality-grandpa/src/until_imported.rs +++ b/client/finality-grandpa/src/until_imported.rs @@ -565,7 +565,7 @@ mod tests { use finality_grandpa::Precommit; use futures::future::Either; use futures_timer::Delay; - use sc_client_api::BlockImportNotification; + use sc_client_api::{BlockImportNotification, UnpinHandle}; use sc_utils::mpsc::{tracing_unbounded, TracingUnboundedSender}; use sp_consensus::BlockOrigin; use sp_core::crypto::UncheckedFrom; @@ -602,6 +602,7 @@ mod tests { header, is_new_best: false, tree_route: None, + _unpin_handle: todo!(), }) .unwrap(); } diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index bb4851497ef05..0214cc283d30a 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -30,8 +30,9 @@ use log::info; use prometheus_endpoint::Registry; use sc_chain_spec::get_extension; use sc_client_api::{ - execution_extensions::ExecutionExtensions, proof_provider::ProofProvider, BadBlocks, - BlockBackend, BlockchainEvents, ExecutorProvider, ForkBlocks, StorageProvider, UsageProvider, + execution_extensions::ExecutionExtensions, proof_provider::ProofProvider, Backend as BackendT, + BadBlocks, BlockBackend, BlockchainEvents, ExecutorProvider, ForkBlocks, StorageProvider, + UsageProvider, }; use sc_client_db::{Backend, DatabaseSettings}; use sc_consensus::import_queue::ImportQueue; @@ -313,10 +314,26 @@ where config.clone(), execution_extensions, )?; + + let (unpin_worker_sender, mut rx) = futures::channel::mpsc::channel::(100); + let task_backend = backend.clone(); + spawn_handle.spawn( + "pinning-worker", + None, + async move { + log::info!(target: "db", "Starting worker task for unpinning."); + loop { + if let Some(message) = rx.next().await { + task_backend.unpin_block(&message); + } + } + } + .boxed(), + ); crate::client::Client::new( backend, - spawn_handle.clone(), executor, + unpin_worker_sender, genesis_storage, fork_blocks, bad_blocks, diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index 82b11f6e2c5d5..a6c10ba794f2d 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -57,7 +57,6 @@ use sp_blockchain::{ }; use sp_consensus::{BlockOrigin, BlockStatus, Error as ConsensusError}; -use futures::{FutureExt, StreamExt}; use sc_utils::mpsc::{tracing_unbounded, TracingUnboundedSender}; use sp_core::{ storage::{ @@ -177,6 +176,7 @@ where new_with_backend( Arc::new(in_mem::Backend::new()), executor, + todo!(), genesis_storage, keystore, spawn_handle, @@ -220,6 +220,7 @@ impl Default for ClientConfig { pub fn new_with_backend( backend: Arc, executor: E, + unpin_worker_sender: futures::channel::mpsc::Sender, build_genesis_storage: &S, keystore: Option, spawn_handle: Box, @@ -249,8 +250,8 @@ where Client::new( backend, - spawn_handle, call_executor, + unpin_worker_sender, build_genesis_storage, Default::default(), Default::default(), @@ -293,11 +294,20 @@ where let ClientImportOperation { mut op, notify_imported, notify_finalized } = op; - let finality_notification = notify_finalized.map(|summary| summary.into()); + let finality_notification = notify_finalized.map(|summary| { + FinalityNotification::from_summary(summary, self.unpin_worker_sender.clone()) + }); + let (import_notification, storage_changes) = match notify_imported { Some(mut summary) => { let storage_changes = summary.storage_changes.take(); - (Some(summary.into()), storage_changes) + ( + Some(BlockImportNotification::from_summary( + summary, + self.unpin_worker_sender.clone(), + )), + storage_changes, + ) }, None => (None, None), }; @@ -315,6 +325,23 @@ where self.backend.commit_operation(op)?; + match (&import_notification, &finality_notification) { + (Some(ref fin), Some(ref imp)) if fin.hash == imp.hash => { + self.backend.pin_block(&fin.hash); + }, + (Some(ref fin), Some(ref imp)) => { + self.backend.pin_block(&fin.hash); + self.backend.pin_block(&imp.hash); + }, + (None, Some(ref imp)) => { + self.backend.pin_block(&imp.hash); + }, + (Some(ref fin), None) => { + self.backend.pin_block(&fin.hash); + }, + (_, _) => {}, + } + self.notify_finalized(finality_notification)?; self.notify_imported(import_notification, storage_changes)?; @@ -345,7 +372,7 @@ where impl Client where - B: backend::Backend + 'static, + B: backend::Backend, E: CallExecutor, Block: BlockT, Block::Header: Clone, @@ -353,8 +380,8 @@ where /// Creates new Substrate Client with given blockchain and code executor. pub fn new( backend: Arc, - spawn_handle: Box, executor: E, + unpin_worker_sender: futures::channel::mpsc::Sender, build_genesis_storage: &dyn BuildStorage, fork_blocks: ForkBlocks, bad_blocks: BadBlocks, @@ -394,23 +421,6 @@ where backend.commit_operation(op)?; } - let (unpin_worker_sender, mut rx) = futures::channel::mpsc::channel::(100); - let task_backend = backend.clone(); - spawn_handle.spawn( - "pinning-worker", - None, - async move { - log::info!(target: "db", "Starting worker task for unpinning."); - loop { - if let Some(message) = rx.next().await { - log::info!(target: "db", "Unpinning block {}", message); - task_backend.unpin_block(&message); - } - } - } - .boxed(), - ); - Ok(Client { backend, executor, @@ -962,9 +972,6 @@ where }, }; - log::info!(target: "db", "Finality notification for block {}, pinning block", notification.hash); - self.backend.pin_block(¬ification.hash); - telemetry!( self.telemetry; SUBSTRATE_INFO; diff --git a/client/state-db/src/lib.rs b/client/state-db/src/lib.rs index 61f6b4a047ef9..31715b83a0809 100644 --- a/client/state-db/src/lib.rs +++ b/client/state-db/src/lib.rs @@ -345,7 +345,7 @@ impl StateDbSync { if let Some(ref mut pruning) = self.pruning { pruning.note_canonical(hash, number, &mut commit)?; } - log::info!(target: "skunert", "Looking to prune block {:?}", hash); + log::info!(target: "skunert", "Looking to canonicalize block {:?}", hash); self.prune(&mut commit)?; Ok(commit) } diff --git a/test-utils/client/src/lib.rs b/test-utils/client/src/lib.rs index 2212bf70c0ed9..7c67d92e2ffe1 100644 --- a/test-utils/client/src/lib.rs +++ b/test-utils/client/src/lib.rs @@ -225,8 +225,8 @@ impl let client = client::Client::new( self.backend.clone(), - todo!(), executor, + todo!(), &storage, self.fork_blocks, self.bad_blocks, From ccdf8edfddb2d0e0aadb8be8b53f20d8bbead5c9 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 9 Jan 2023 10:55:08 +0100 Subject: [PATCH 03/37] Pin in state-db, pass block number --- client/api/src/backend.rs | 2 +- client/api/src/in_mem.rs | 2 +- client/db/src/lib.rs | 6 ++++-- client/service/src/client/client.rs | 31 +++++++++++++++++++++++------ 4 files changed, 31 insertions(+), 10 deletions(-) diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs index c0bfc785399b8..b7557ea39466e 100644 --- a/client/api/src/backend.rs +++ b/client/api/src/backend.rs @@ -503,7 +503,7 @@ pub trait Backend: AuxStore + Send + Sync { fn offchain_storage(&self) -> Option; /// Pin the block to prevent pruning. - fn pin_block(&self, hash: &Block::Hash) -> sp_blockchain::Result<()>; + fn pin_block(&self, hash: &Block::Hash, number: u64) -> sp_blockchain::Result<()>; /// Unpin the block to allow pruning. fn unpin_block(&self, hash: &Block::Hash) -> sp_blockchain::Result<()>; diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs index b0dd9d5620635..0d055f42bb9c0 100644 --- a/client/api/src/in_mem.rs +++ b/client/api/src/in_mem.rs @@ -791,7 +791,7 @@ where false } - fn pin_block(&self, hash: &::Hash) -> blockchain::Result<()> { + fn pin_block(&self, hash: &::Hash, number: u64) -> blockchain::Result<()> { todo!() } diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 3aad35f9a80f6..47d28aacc6c67 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -2401,13 +2401,14 @@ impl sc_client_api::backend::Backend for Backend { ) } - fn pin_block(&self, hash: &::Hash) -> sp_blockchain::Result<()> { + fn pin_block(&self, hash: &::Hash, number: u64) -> sp_blockchain::Result<()> { let mut handle = self.pinned_block_store.write(); handle.insert(*hash); - log::info!(target: "db", "Pinning block in backend, hash: {}, num_pinned_blocks: {}", hash, handle.len()); + log::info!(target: "db", "Pinning block in backend, hash: {}, number: {}, num_pinned_blocks: {}", hash, number, handle.len()); log::info!(target: "skunert", "Pinned_block_contents: {:?}", handle); let hint = || false; self.storage.state_db.pin(hash, number, hint); + self.storage..pin(hash, number, hint); Ok(()) } @@ -2415,6 +2416,7 @@ impl sc_client_api::backend::Backend for Backend { log::info!(target: "db", "Unpinning block in backend, hash: {}", hash); let mut handle = self.pinned_block_store.write(); handle.remove(hash); + self.storage.state_db.unpin(hash); Ok(()) } } diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index a6c10ba794f2d..69ec5a993c9ad 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -294,10 +294,14 @@ where let ClientImportOperation { mut op, notify_imported, notify_finalized } = op; + let finality_num = + notify_finalized.as_ref().map(|summary| summary.header.number().clone()); let finality_notification = notify_finalized.map(|summary| { FinalityNotification::from_summary(summary, self.unpin_worker_sender.clone()) }); + let import_num = + notify_imported.as_ref().map(|summary| summary.header.number().clone()); let (import_notification, storage_changes) = match notify_imported { Some(mut summary) => { let storage_changes = summary.storage_changes.take(); @@ -325,19 +329,34 @@ where self.backend.commit_operation(op)?; - match (&import_notification, &finality_notification) { + match (&finality_notification, &import_notification) { (Some(ref fin), Some(ref imp)) if fin.hash == imp.hash => { - self.backend.pin_block(&fin.hash); + self.backend.pin_block( + &fin.hash, + finality_num.expect("Should work").saturated_into::(), + ); }, (Some(ref fin), Some(ref imp)) => { - self.backend.pin_block(&fin.hash); - self.backend.pin_block(&imp.hash); + self.backend.pin_block( + &fin.hash, + finality_num.expect("Should work").saturated_into::(), + ); + self.backend.pin_block( + &imp.hash, + import_num.expect("Should work").saturated_into::(), + ); }, (None, Some(ref imp)) => { - self.backend.pin_block(&imp.hash); + self.backend.pin_block( + &imp.hash, + import_num.expect("Should work").saturated_into::(), + ); }, (Some(ref fin), None) => { - self.backend.pin_block(&fin.hash); + self.backend.pin_block( + &fin.hash, + finality_num.expect("Should work").saturated_into::(), + ); }, (_, _) => {}, } From 0443235a7e83a3e6e851beb3cb5f19dede6b8418 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Tue, 10 Jan 2023 10:41:20 +0100 Subject: [PATCH 04/37] Pin blocks in blockchain db --- Cargo.lock | 4 +-- client/api/src/backend.rs | 2 +- client/api/src/in_mem.rs | 2 +- client/db/src/lib.rs | 76 ++++++++++++++++++++++++++++++++++++--- 4 files changed, 76 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 110f6fcdc9a19..640b589ae1d03 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10762,9 +10762,9 @@ checksum = "eafc1b9b2dfc6f5529177b62cf806484db55b32dc7c9658a118e11bbeb33061d" [[package]] name = "version_check" -version = "0.9.2" +version = "0.9.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b5a972e5669d67ba988ce3dc826706fb0a8b01471c088cb0b6110b805cc36aed" +checksum = "49874b5167b65d7193b8aba1567f5c7d93d001cafc34600cee003eda787e483f" [[package]] name = "void" diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs index b7557ea39466e..71152905ee02e 100644 --- a/client/api/src/backend.rs +++ b/client/api/src/backend.rs @@ -506,7 +506,7 @@ pub trait Backend: AuxStore + Send + Sync { fn pin_block(&self, hash: &Block::Hash, number: u64) -> sp_blockchain::Result<()>; /// Unpin the block to allow pruning. - fn unpin_block(&self, hash: &Block::Hash) -> sp_blockchain::Result<()>; + fn unpin_block(&self, hash: &Block::Hash); /// Returns true if state for given block is available. fn have_state_at(&self, hash: Block::Hash, _number: NumberFor) -> bool { diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs index 0d055f42bb9c0..1561d787245b2 100644 --- a/client/api/src/in_mem.rs +++ b/client/api/src/in_mem.rs @@ -795,7 +795,7 @@ where todo!() } - fn unpin_block(&self, hash: &::Hash) -> blockchain::Result<()> { + fn unpin_block(&self, hash: &::Hash) { todo!() } } diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 47d28aacc6c67..b7afbd3832fd3 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -46,6 +46,7 @@ use parking_lot::{Mutex, RwLock}; use std::{ collections::{HashMap, HashSet}, io, + num::NonZeroUsize, path::{Path, PathBuf}, sync::Arc, }; @@ -474,6 +475,46 @@ fn cache_header( } } +pub struct PinnedBlockCache { + body_cache: RwLock>>>, + justification_cache: RwLock>>, +} + +impl PinnedBlockCache { + pub fn new() -> Self { + Self { body_cache: HashMap::new().into(), justification_cache: HashMap::new().into() } + } + + pub fn insert( + &self, + hash: Block::Hash, + extrinsics: Option>, + justifications: Option, + ) { + let mut body_cache = self.body_cache.write(); + body_cache.insert(hash, extrinsics); + let mut justification_cache = self.justification_cache.write(); + justification_cache.insert(hash, justifications); + } + + pub fn remove(&self, hash: &Block::Hash) { + let mut body_cache = self.body_cache.write(); + body_cache.remove(hash); + let mut justification_cache = self.justification_cache.write(); + justification_cache.remove(hash); + } + + pub fn justification(&self, hash: &Block::Hash) -> Option> { + let justification_cache = self.justification_cache.read(); + justification_cache.get(hash).cloned() + } + + pub fn body(&self, hash: &Block::Hash) -> Option>> { + let body_cache = self.body_cache.read(); + body_cache.get(hash).cloned() + } +} + /// Block database pub struct BlockchainDb { db: Arc>, @@ -481,6 +522,7 @@ pub struct BlockchainDb { leaves: RwLock>>, header_metadata_cache: Arc>, header_cache: Mutex>>, + pinned_blocks_cache: Arc>, } impl BlockchainDb { @@ -493,6 +535,7 @@ impl BlockchainDb { meta: Arc::new(RwLock::new(meta)), header_metadata_cache: Arc::new(HeaderMetadataCache::default()), header_cache: Default::default(), + pinned_blocks_cache: Arc::new(PinnedBlockCache::new()), }) } @@ -521,6 +564,17 @@ impl BlockchainDb { let mut meta = self.meta.write(); meta.block_gap = gap; } + + fn pin(&self, hash: Block::Hash) -> ClientResult<()> { + let justifications = self.justifications(hash)?; + let body = self.body(hash)?; + self.pinned_blocks_cache.insert(hash, body, justifications); + Ok(()) + } + + fn unpin(&self, hash: &Block::Hash) { + self.pinned_blocks_cache.remove(hash); + } } impl sc_client_api::blockchain::HeaderBackend for BlockchainDb { @@ -578,6 +632,10 @@ impl sc_client_api::blockchain::HeaderBackend for Blockcha impl sc_client_api::blockchain::Backend for BlockchainDb { fn body(&self, hash: Block::Hash) -> ClientResult>> { + if let Some(result) = self.pinned_blocks_cache.body(&hash) { + return Ok(result) + } + if let Some(body) = read_db(&*self.db, columns::KEY_LOOKUP, columns::BODY, BlockId::Hash::(hash))? { @@ -643,6 +701,10 @@ impl sc_client_api::blockchain::Backend for BlockchainDb ClientResult> { + if let Some(result) = self.pinned_blocks_cache.justification(&hash) { + return Ok(result) + } + match read_db( &*self.db, columns::KEY_LOOKUP, @@ -1772,6 +1834,11 @@ impl Backend { let id = BlockId::::hash(hash); match self.blockchain.header(id)? { Some(header) => { + let pinned_block_store = self.pinned_block_store.read(); + if pinned_block_store.contains(&hash) { + self.blockchain.pin(hash)?; + } + self.prune_block(transaction, id)?; number = header.number().saturating_sub(One::one()); hash = *header.parent_hash(); @@ -2407,17 +2474,18 @@ impl sc_client_api::backend::Backend for Backend { log::info!(target: "db", "Pinning block in backend, hash: {}, number: {}, num_pinned_blocks: {}", hash, number, handle.len()); log::info!(target: "skunert", "Pinned_block_contents: {:?}", handle); let hint = || false; - self.storage.state_db.pin(hash, number, hint); - self.storage..pin(hash, number, hint); + self.storage.state_db.pin(hash, number, hint).map_err(|_| { + sp_blockchain::Error::UnknownBlock(format!("State already discarded for {:?}", hash)) + })?; Ok(()) } - fn unpin_block(&self, hash: &::Hash) -> sp_blockchain::Result<()> { + fn unpin_block(&self, hash: &::Hash) { log::info!(target: "db", "Unpinning block in backend, hash: {}", hash); let mut handle = self.pinned_block_store.write(); handle.remove(hash); self.storage.state_db.unpin(hash); - Ok(()) + self.blockchain.unpin(hash); } } From c1a25dc3a63816891ba7ab3d6ca21368dd72bca3 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Tue, 10 Jan 2023 17:16:24 +0100 Subject: [PATCH 05/37] Switch to reference counted LRU --- Cargo.lock | 66 +++++++++++++++++++++++++++++----------- client/api/src/client.rs | 2 ++ client/api/src/in_mem.rs | 8 ++--- client/db/Cargo.toml | 1 + client/db/src/lib.rs | 51 +++++++++++++++++++++++++------ 5 files changed, 96 insertions(+), 32 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 640b589ae1d03..517630b7fb809 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -68,7 +68,18 @@ version = "0.7.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fcb51a0695d8f838b1ee009b3fbf66bda078cd64590202a864a8f3e8c4315c47" dependencies = [ - "getrandom 0.2.3", + "getrandom 0.2.8", + "once_cell", + "version_check", +] + +[[package]] +name = "ahash" +version = "0.8.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bf6ccdb167abbf410dcb915cabd428929d7f6a04980b54a11f26a39f1c7f7107" +dependencies = [ + "cfg-if", "once_cell", "version_check", ] @@ -2428,13 +2439,13 @@ dependencies = [ [[package]] name = "getrandom" -version = "0.2.3" +version = "0.2.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7fcd999463524c52659517fe2cea98493cfe485d10565e7b0fb07dbba7ad2753" +checksum = "c05aeb6a22b8f62540c194aac980f2115af067bfe15a0734d7277a768d396b31" dependencies = [ "cfg-if", "libc", - "wasi 0.10.0+wasi-snapshot-preview1", + "wasi 0.11.0+wasi-snapshot-preview1", ] [[package]] @@ -2567,7 +2578,16 @@ version = "0.12.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8a9ee70c43aaf417c914396645a0fa852624801b24ebb7ae78fe8272889ac888" dependencies = [ - "ahash", + "ahash 0.7.6", +] + +[[package]] +name = "hashbrown" +version = "0.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "33ff8ae62cd3a9102e5637afc8452c55acf3844001bd5374e0b0bd7b6616c038" +dependencies = [ + "ahash 0.8.2", ] [[package]] @@ -3266,7 +3286,7 @@ dependencies = [ "bytes", "futures", "futures-timer", - "getrandom 0.2.3", + "getrandom 0.2.8", "instant", "lazy_static", "libp2p-core", @@ -3350,7 +3370,7 @@ dependencies = [ "libp2p-core", "libp2p-swarm", "log", - "lru", + "lru 0.8.1", "prost", "prost-build", "prost-codec", @@ -3749,6 +3769,15 @@ dependencies = [ "hashbrown 0.12.3", ] +[[package]] +name = "lru" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "71e7d46de488603ffdd5f30afbc64fbba2378214a2c3a2fb83abf3d33126df17" +dependencies = [ + "hashbrown 0.13.1", +] + [[package]] name = "lru-cache" version = "0.1.2" @@ -6748,7 +6777,7 @@ version = "0.6.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "34cf66eb183df1c5876e2dcf6b13d57340741e8dc255b48e40a26de954d06ae7" dependencies = [ - "getrandom 0.2.3", + "getrandom 0.2.8", ] [[package]] @@ -6834,7 +6863,7 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "528532f3d801c87aec9def2add9ca802fe569e44a544afe633765267840abe64" dependencies = [ - "getrandom 0.2.3", + "getrandom 0.2.8", "redox_syscall", ] @@ -7301,6 +7330,7 @@ dependencies = [ "kvdb-rocksdb", "linked-hash-map", "log", + "lru 0.9.0", "parity-db", "parity-scale-codec", "parking_lot 0.12.1", @@ -7574,7 +7604,7 @@ dependencies = [ "array-bytes", "criterion", "env_logger", - "lru", + "lru 0.8.1", "num_cpus", "parity-scale-codec", "parking_lot 0.12.1", @@ -7655,7 +7685,7 @@ dependencies = [ name = "sc-finality-grandpa" version = "0.10.0-dev" dependencies = [ - "ahash", + "ahash 0.7.6", "array-bytes", "assert_matches", "async-trait", @@ -7774,7 +7804,7 @@ dependencies = [ "linked-hash-map", "linked_hash_set", "log", - "lru", + "lru 0.8.1", "parity-scale-codec", "parking_lot 0.12.1", "pin-project", @@ -7864,12 +7894,12 @@ dependencies = [ name = "sc-network-gossip" version = "0.10.0-dev" dependencies = [ - "ahash", + "ahash 0.7.6", "futures", "futures-timer", "libp2p", "log", - "lru", + "lru 0.8.1", "quickcheck", "sc-network-common", "sc-peerset", @@ -7910,7 +7940,7 @@ dependencies = [ "futures", "libp2p", "log", - "lru", + "lru 0.8.1", "mockall", "parity-scale-codec", "prost", @@ -8988,7 +9018,7 @@ version = "4.0.0-dev" dependencies = [ "futures", "log", - "lru", + "lru 0.8.1", "parity-scale-codec", "parking_lot 0.12.1", "sp-api", @@ -9595,13 +9625,13 @@ dependencies = [ name = "sp-trie" version = "7.0.0" dependencies = [ - "ahash", + "ahash 0.7.6", "array-bytes", "criterion", "hash-db", "hashbrown 0.12.3", "lazy_static", - "lru", + "lru 0.8.1", "memory-db", "nohash-hasher", "parity-scale-codec", diff --git a/client/api/src/client.rs b/client/api/src/client.rs index a0a127eee56f8..81f75dfbf419f 100644 --- a/client/api/src/client.rs +++ b/client/api/src/client.rs @@ -266,6 +266,7 @@ impl fmt::Display for UsageInfo { } } +/// Sends a message to the pinning-worker once dropped to unpin a block in the backend. #[derive(Debug)] pub struct UnpinHandle { hash: Block::Hash, @@ -273,6 +274,7 @@ pub struct UnpinHandle { } impl UnpinHandle { + /// Create a new UnpinHandle pub fn new(hash: Block::Hash, unpin_worker_sender: Sender) -> Self { Self { hash, unpin_worker_sender } } diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs index 1561d787245b2..68e9c6899fbf1 100644 --- a/client/api/src/in_mem.rs +++ b/client/api/src/in_mem.rs @@ -791,12 +791,12 @@ where false } - fn pin_block(&self, hash: &::Hash, number: u64) -> blockchain::Result<()> { - todo!() + fn pin_block(&self, _: &::Hash, _: u64) -> blockchain::Result<()> { + unimplemented!() } - fn unpin_block(&self, hash: &::Hash) { - todo!() + fn unpin_block(&self, _: &::Hash) { + unimplemented!() } } diff --git a/client/db/Cargo.toml b/client/db/Cargo.toml index ee879a161edfe..603118e7461d7 100644 --- a/client/db/Cargo.toml +++ b/client/db/Cargo.toml @@ -22,6 +22,7 @@ kvdb-memorydb = "0.13.0" kvdb-rocksdb = { version = "0.17.0", optional = true } linked-hash-map = "0.5.4" log = "0.4.17" +lru = "0.9.0" parity-db = "0.4.2" parking_lot = "0.12.1" sc-client-api = { version = "4.0.0-dev", path = "../api" } diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index b7afbd3832fd3..1ec0a5c46abd7 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -42,6 +42,7 @@ mod utils; use linked_hash_map::LinkedHashMap; use log::{debug, trace, warn}; +use lru::LruCache; use parking_lot::{Mutex, RwLock}; use std::{ collections::{HashMap, HashSet}, @@ -111,6 +112,8 @@ pub type DbStateBuilder = sp_state_machine::TrieBackendBuilder< /// Length of a [`DbHash`]. const DB_HASH_LEN: usize = 32; +const PINNING_CACHE_SIZE: usize = 256; + /// Hash type that this backend uses for the database. pub type DbHash = sp_core::H256; @@ -475,14 +478,18 @@ fn cache_header( } } -pub struct PinnedBlockCache { - body_cache: RwLock>>>, - justification_cache: RwLock>>, +struct PinnedBlockCache { + body_cache: RwLock>)>>, + justification_cache: RwLock)>>, } impl PinnedBlockCache { pub fn new() -> Self { - Self { body_cache: HashMap::new().into(), justification_cache: HashMap::new().into() } + Self { + body_cache: LruCache::new(NonZeroUsize::new(PINNING_CACHE_SIZE).unwrap()).into(), + justification_cache: LruCache::new(NonZeroUsize::new(PINNING_CACHE_SIZE).unwrap()) + .into(), + } } pub fn insert( @@ -492,26 +499,48 @@ impl PinnedBlockCache { justifications: Option, ) { let mut body_cache = self.body_cache.write(); - body_cache.insert(hash, extrinsics); + if let Some(mut entry) = body_cache.get_mut(&hash) { + entry.0 += 1; + entry.1 = extrinsics; + } else { + body_cache.put(hash, (1, extrinsics)); + } + let mut justification_cache = self.justification_cache.write(); - justification_cache.insert(hash, justifications); + if let Some(mut entry) = justification_cache.get_mut(&hash) { + entry.0 += 1; + entry.1 = justifications; + } else { + justification_cache.put(hash, (1, justifications)); + } } pub fn remove(&self, hash: &Block::Hash) { let mut body_cache = self.body_cache.write(); - body_cache.remove(hash); + if let Some(mut entry) = body_cache.peek_mut(hash) { + entry.0 -= 1; + if entry.0 == 0 { + body_cache.pop(hash); + } + } + let mut justification_cache = self.justification_cache.write(); - justification_cache.remove(hash); + if let Some(mut entry) = justification_cache.peek_mut(hash) { + entry.0 -= 1; + if entry.0 == 0 { + justification_cache.pop(hash); + } + } } pub fn justification(&self, hash: &Block::Hash) -> Option> { let justification_cache = self.justification_cache.read(); - justification_cache.get(hash).cloned() + justification_cache.peek(hash).map(|element| element.1.clone()) } pub fn body(&self, hash: &Block::Hash) -> Option>> { let body_cache = self.body_cache.read(); - body_cache.get(hash).cloned() + body_cache.peek(hash).map(|element| element.1.clone()) } } @@ -633,6 +662,7 @@ impl sc_client_api::blockchain::HeaderBackend for Blockcha impl sc_client_api::blockchain::Backend for BlockchainDb { fn body(&self, hash: Block::Hash) -> ClientResult>> { if let Some(result) = self.pinned_blocks_cache.body(&hash) { + log::info!(target: "skunert", "Found block data for {:?} in pinned cache.", hash); return Ok(result) } @@ -702,6 +732,7 @@ impl sc_client_api::blockchain::Backend for BlockchainDb ClientResult> { if let Some(result) = self.pinned_blocks_cache.justification(&hash) { + log::info!(target: "skunert", "Found justification data for {:?} in pinned cache.", hash); return Ok(result) } From 2f7f943a0ad3bb68ebf366993fb52f716b86065f Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Wed, 11 Jan 2023 16:52:47 +0100 Subject: [PATCH 06/37] Disable pinning when we keep all blocks --- client/api/src/client.rs | 20 ++ client/db/src/lib.rs | 279 ++++++++++++++++-- client/finality-grandpa/src/until_imported.rs | 14 +- client/service/src/client/call_executor.rs | 4 + client/state-db/src/lib.rs | 2 +- test-utils/client/src/lib.rs | 3 +- 6 files changed, 295 insertions(+), 27 deletions(-) diff --git a/client/api/src/client.rs b/client/api/src/client.rs index aa4bb1b92d154..9738a67ceb1f6 100644 --- a/client/api/src/client.rs +++ b/client/api/src/client.rs @@ -306,6 +306,26 @@ pub struct BlockImportNotification { _unpin_handle: Arc>, } +impl BlockImportNotification { + pub fn new( + hash: Block::Hash, + origin: BlockOrigin, + header: Block::Header, + is_new_best: bool, + tree_route: Option>>, + unpin_worker_sender: Sender, + ) -> Self { + Self { + hash, + origin, + header, + is_new_best, + tree_route, + _unpin_handle: Arc::new(UnpinHandle::new(hash, unpin_worker_sender)), + } + } +} + /// Summary of a finalized block. #[derive(Clone, Debug)] pub struct FinalityNotification { diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index ae2b2ab61ef5d..1418bf08e0604 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -45,7 +45,7 @@ use log::{debug, trace, warn}; use lru::LruCache; use parking_lot::{Mutex, RwLock}; use std::{ - collections::{HashMap, HashSet}, + collections::{hash_map::Entry, HashMap, HashSet}, io, num::NonZeroUsize, path::{Path, PathBuf}, @@ -498,27 +498,49 @@ impl PinnedBlockCache { extrinsics: Option>, justifications: Option, ) { + self.insert_with_ref(hash, extrinsics, justifications, 1); + } + + pub fn bump_ref(&self, hash: Block::Hash) { let mut body_cache = self.body_cache.write(); if let Some(mut entry) = body_cache.get_mut(&hash) { entry.0 += 1; + } + + let mut justification_cache = self.justification_cache.write(); + if let Some(mut entry) = justification_cache.get_mut(&hash) { + entry.0 += 1; + } + } + + pub fn insert_with_ref( + &self, + hash: Block::Hash, + extrinsics: Option>, + justifications: Option, + ref_count: u32, + ) { + let mut body_cache = self.body_cache.write(); + if let Some(mut entry) = body_cache.get_mut(&hash) { + entry.0 += ref_count; entry.1 = extrinsics; } else { - body_cache.put(hash, (1, extrinsics)); + body_cache.put(hash, (ref_count, extrinsics)); } let mut justification_cache = self.justification_cache.write(); if let Some(mut entry) = justification_cache.get_mut(&hash) { - entry.0 += 1; + entry.0 += ref_count; entry.1 = justifications; } else { - justification_cache.put(hash, (1, justifications)); + justification_cache.put(hash, (ref_count, justifications)); } } pub fn remove(&self, hash: &Block::Hash) { let mut body_cache = self.body_cache.write(); if let Some(mut entry) = body_cache.peek_mut(hash) { - entry.0 -= 1; + entry.0 = entry.0.saturating_sub(1); if entry.0 == 0 { body_cache.pop(hash); } @@ -526,7 +548,7 @@ impl PinnedBlockCache { let mut justification_cache = self.justification_cache.write(); if let Some(mut entry) = justification_cache.peek_mut(hash) { - entry.0 -= 1; + entry.0 = entry.0.saturating_sub(1); if entry.0 == 0 { justification_cache.pop(hash); } @@ -552,10 +574,11 @@ pub struct BlockchainDb { header_metadata_cache: Arc>, header_cache: Mutex>>, pinned_blocks_cache: Arc>, + pinning_enabled: bool, } impl BlockchainDb { - fn new(db: Arc>) -> ClientResult { + fn new(db: Arc>, pinning_enabled: bool) -> ClientResult { let meta = read_meta::(&*db, columns::HEADER)?; let leaves = LeafSet::read_from_db(&*db, columns::META, meta_keys::LEAF_PREFIX)?; Ok(BlockchainDb { @@ -565,6 +588,7 @@ impl BlockchainDb { header_metadata_cache: Arc::new(HeaderMetadataCache::default()), header_cache: Default::default(), pinned_blocks_cache: Arc::new(PinnedBlockCache::new()), + pinning_enabled, }) } @@ -594,7 +618,29 @@ impl BlockchainDb { meta.block_gap = gap; } + fn pin_with_ref(&self, hash: Block::Hash, ref_count: u32) -> ClientResult<()> { + if !self.pinning_enabled { + return Ok(()) + } + + let justifications = self.justifications(hash)?; + let body = self.body(hash)?; + self.pinned_blocks_cache.insert_with_ref(hash, body, justifications, ref_count); + Ok(()) + } + + fn bump_ref(&self, hash: Block::Hash) { + if !self.pinning_enabled { + return + } + self.pinned_blocks_cache.bump_ref(hash); + } + fn pin(&self, hash: Block::Hash) -> ClientResult<()> { + if !self.pinning_enabled { + return Ok(()) + } + let justifications = self.justifications(hash)?; let body = self.body(hash)?; self.pinned_blocks_cache.insert(hash, body, justifications); @@ -602,6 +648,10 @@ impl BlockchainDb { } fn unpin(&self, hash: &Block::Hash) { + if !self.pinning_enabled { + return + } + self.pinned_blocks_cache.remove(hash); } } @@ -1133,7 +1183,7 @@ impl FrozenForDuration { /// Disk backend keeps data in a key-value store. In archive mode, trie nodes are kept from all /// blocks. Otherwise, trie nodes are kept only from some recent blocks. pub struct Backend { - pinned_block_store: RwLock>, + pinned_block_store: RwLock>, storage: Arc>, offchain_storage: offchain::LocalStorage, blockchain: BlockchainDb, @@ -1240,7 +1290,8 @@ impl Backend { let state_pruning_used = state_db.pruning_mode(); let is_archive_pruning = state_pruning_used.is_archive(); - let blockchain = BlockchainDb::new(db.clone())?; + let blockchain = + BlockchainDb::new(db.clone(), config.blocks_pruning != BlocksPruning::KeepAll)?; let storage_db = StorageDb { db: db.clone(), state_db, prefix_keys: !db.supports_ref_counting() }; @@ -1248,7 +1299,7 @@ impl Backend { let offchain_storage = offchain::LocalStorage::new(db.clone()); let backend = Backend { - pinned_block_store: RwLock::new(HashSet::new()), + pinned_block_store: RwLock::new(HashMap::new()), storage: Arc::new(storage_db), offchain_storage, blockchain, @@ -1822,6 +1873,14 @@ impl Backend { let keep = std::cmp::max(blocks_pruning, 1); if finalized >= keep.into() { let number = finalized.saturating_sub(keep.into()); + + let pinned_block_store = self.pinned_block_store.read(); + if let Some(hash) = self.blockchain.hash(number)? { + if let Some(ref_count) = pinned_block_store.get(&hash) { + self.blockchain.pin_with_ref(hash, *ref_count)?; + } + }; + self.prune_block(transaction, BlockId::::number(number))?; } self.prune_displaced_branches(transaction, finalized, displaced)?; @@ -1851,8 +1910,8 @@ impl Backend { match self.blockchain.header(hash)? { Some(header) => { let pinned_block_store = self.pinned_block_store.read(); - if pinned_block_store.contains(&hash) { - self.blockchain.pin(hash)?; + if let Some(ref_count) = pinned_block_store.get(&hash) { + self.blockchain.pin_with_ref(hash, *ref_count)?; } self.prune_block(transaction, BlockId::::hash(hash))?; @@ -2484,21 +2543,35 @@ impl sc_client_api::backend::Backend for Backend { } fn pin_block(&self, hash: &::Hash, number: u64) -> sp_blockchain::Result<()> { - let mut handle = self.pinned_block_store.write(); - handle.insert(*hash); - log::info!(target: "db", "Pinning block in backend, hash: {}, number: {}, num_pinned_blocks: {}", hash, number, handle.len()); - log::info!(target: "skunert", "Pinned_block_contents: {:?}", handle); + let mut pin_store_guard = self.pinned_block_store.write(); + match pin_store_guard.entry(*hash) { + Entry::Occupied(mut entry) => { + *entry.get_mut() += 1; + }, + Entry::Vacant(v) => { + v.insert(1); + }, + }; + //handle.insert(*hash, ); + log::info!(target: "db", "Pinning block in backend, hash: {}, number: {}, num_pinned_blocks: {}", hash, number, pin_store_guard.len()); + log::info!(target: "skunert", "Pinned_block_contents: {:?}", pin_store_guard); let hint = || false; self.storage.state_db.pin(hash, number, hint).map_err(|_| { sp_blockchain::Error::UnknownBlock(format!("State already discarded for {:?}", hash)) })?; + self.blockchain.bump_ref(*hash); Ok(()) } fn unpin_block(&self, hash: &::Hash) { log::info!(target: "db", "Unpinning block in backend, hash: {}", hash); - let mut handle = self.pinned_block_store.write(); - handle.remove(hash); + let mut pin_store_guard = self.pinned_block_store.write(); + if let Entry::Occupied(mut entry) = pin_store_guard.entry(*hash) { + *entry.get_mut() -= 1; + if *entry.get() == 0 { + entry.remove(); + } + }; self.storage.state_db.unpin(hash); self.blockchain.unpin(hash); } @@ -4129,4 +4202,174 @@ pub(crate) mod tests { assert_eq!(block4, backend.blockchain().hash(4).unwrap().unwrap()); } } + + #[test] + fn test_pinned_blocks_on_finalize() { + let backend = Backend::::new_test_with_tx_storage(BlocksPruning::Some(1), 10); + let mut blocks = Vec::new(); + let mut prev_hash = Default::default(); + + // Block tree: + // 0 -> 1 -> 2 -> 3 -> 4 + for i in 0..5 { + let hash = insert_block( + &backend, + i, + prev_hash, + None, + Default::default(), + vec![i.into()], + None, + ) + .unwrap(); + blocks.push(hash); + + // Avoid block pruning. + backend.pin_block(&blocks[i as usize], i).unwrap(); + + prev_hash = hash; + } + // Block 1 gets pinned twice + backend.pin_block(&blocks[1], 1).unwrap(); + + let mut op = backend.begin_operation().unwrap(); + backend.begin_state_operation(&mut op, blocks[4]).unwrap(); + for i in 1..5 { + op.mark_finalized(blocks[i], None).unwrap(); + } + backend.commit_operation(op).unwrap(); + + let bc = backend.blockchain(); + // Block 0, 1, 2, 3 are pinned and pruning is delayed, + // while block 4 is never delayed for pruning as it is finalized. + assert_eq!(Some(vec![0.into()]), bc.body(blocks[0]).unwrap()); + assert_eq!(Some(vec![1.into()]), bc.body(blocks[1]).unwrap()); + assert_eq!(Some(vec![2.into()]), bc.body(blocks[2]).unwrap()); + assert_eq!(Some(vec![3.into()]), bc.body(blocks[3]).unwrap()); + assert_eq!(Some(vec![4.into()]), bc.body(blocks[4]).unwrap()); + + // Unpin all blocks. + for block in &blocks { + backend.unpin_block(&block); + } + + assert!(bc.body(blocks[0]).unwrap().is_none()); + // Block 1 was pinned twice, we expect it to be in-memory + assert!(bc.body(blocks[1]).unwrap().is_some()); + assert!(bc.body(blocks[2]).unwrap().is_none()); + assert!(bc.body(blocks[3]).unwrap().is_none()); + + // After this second unpin, block 1 should be removed + backend.unpin_block(&blocks[1]); + assert!(bc.body(blocks[1]).unwrap().is_none()); + + // Block 4 is inside the pruning window and still kept + assert!(bc.body(blocks[4]).unwrap().is_some()); + + // Block tree: + // 0 -> 1 -> 2 -> 3 -> 4 -> 5 + let hash = + insert_block(&backend, 5, prev_hash, None, Default::default(), vec![5.into()], None) + .unwrap(); + blocks.push(hash); + + // Mark block 5 as finalized. + let mut op = backend.begin_operation().unwrap(); + backend.begin_state_operation(&mut op, blocks[5]).unwrap(); + op.mark_finalized(blocks[5], None).unwrap(); + backend.commit_operation(op).unwrap(); + + assert!(bc.body(blocks[0]).unwrap().is_none()); + assert!(bc.body(blocks[1]).unwrap().is_none()); + assert!(bc.body(blocks[2]).unwrap().is_none()); + assert!(bc.body(blocks[3]).unwrap().is_none()); + + // Block 4 was unpinned before pruning, it must also get pruned. + assert!(bc.body(blocks[4]).unwrap().is_none()); + assert_eq!(Some(vec![5.into()]), bc.body(blocks[5]).unwrap()); + } + + #[test] + fn test_pinned_blocks_on_finalize_with_fork() { + let backend = Backend::::new_test_with_tx_storage(BlocksPruning::Some(1), 10); + let mut blocks = Vec::new(); + let mut prev_hash = Default::default(); + + // Block tree: + // 0 -> 1 -> 2 -> 3 -> 4 + for i in 0..5 { + let hash = insert_block( + &backend, + i, + prev_hash, + None, + Default::default(), + vec![i.into()], + None, + ) + .unwrap(); + blocks.push(hash); + + // Avoid block pruning. + backend.pin_block(&blocks[i as usize], i).unwrap(); + + prev_hash = hash; + } + + // Insert a fork at the second block. + // Block tree: + // 0 -> 1 -> 2 -> 3 -> 4 + // \ -> 2 -> 3 + let fork_hash_root = + insert_block(&backend, 2, blocks[1], None, H256::random(), vec![2.into()], None) + .unwrap(); + let fork_hash_3 = insert_block( + &backend, + 3, + fork_hash_root, + None, + H256::random(), + vec![3.into(), 11.into()], + None, + ) + .unwrap(); + + // Do not prune the fork hash. + backend.pin_block(&fork_hash_3, 3).unwrap(); + + let mut op = backend.begin_operation().unwrap(); + backend.begin_state_operation(&mut op, blocks[4]).unwrap(); + op.mark_head(blocks[4]).unwrap(); + backend.commit_operation(op).unwrap(); + + for i in 1..5 { + let mut op = backend.begin_operation().unwrap(); + backend.begin_state_operation(&mut op, blocks[4]).unwrap(); + op.mark_finalized(blocks[i], None).unwrap(); + backend.commit_operation(op).unwrap(); + } + + let bc = backend.blockchain(); + assert_eq!(Some(vec![0.into()]), bc.body(blocks[0]).unwrap()); + assert_eq!(Some(vec![1.into()]), bc.body(blocks[1]).unwrap()); + assert_eq!(Some(vec![2.into()]), bc.body(blocks[2]).unwrap()); + assert_eq!(Some(vec![3.into()]), bc.body(blocks[3]).unwrap()); + assert_eq!(Some(vec![4.into()]), bc.body(blocks[4]).unwrap()); + // Check the fork hashes. + assert_eq!(None, bc.body(fork_hash_root).unwrap()); + assert_eq!(Some(vec![3.into(), 11.into()]), bc.body(fork_hash_3).unwrap()); + + // Unpin all blocks, except the forked one. + for block in &blocks { + backend.unpin_block(&block); + } + assert!(bc.body(blocks[0]).unwrap().is_none()); + assert!(bc.body(blocks[1]).unwrap().is_none()); + assert!(bc.body(blocks[2]).unwrap().is_none()); + assert!(bc.body(blocks[3]).unwrap().is_none()); + + assert!(bc.body(fork_hash_3).unwrap().is_some()); + backend.unpin_block(&fork_hash_3); + assert!(bc.body(fork_hash_3).unwrap().is_none()); + } } diff --git a/client/finality-grandpa/src/until_imported.rs b/client/finality-grandpa/src/until_imported.rs index 46eb71188f61f..5a29b382fce4b 100644 --- a/client/finality-grandpa/src/until_imported.rs +++ b/client/finality-grandpa/src/until_imported.rs @@ -593,17 +593,17 @@ mod tests { fn import_header(&self, header: Header) { let hash = header.hash(); let number = *header.number(); - + let (tx, rx) = futures::channel::mpsc::channel(100); self.known_blocks.lock().insert(hash, number); self.sender - .unbounded_send(BlockImportNotification { + .unbounded_send(BlockImportNotification::::new( hash, - origin: BlockOrigin::File, + BlockOrigin::File, header, - is_new_best: false, - tree_route: None, - _unpin_handle: todo!(), - }) + false, + None, + tx, + )) .unwrap(); } } diff --git a/client/service/src/client/call_executor.rs b/client/service/src/client/call_executor.rs index 7ee19671dd688..a3a3116b4db3e 100644 --- a/client/service/src/client/call_executor.rs +++ b/client/service/src/client/call_executor.rs @@ -399,11 +399,13 @@ mod tests { ) .expect("Creates genesis block builder"); + let (unpin_worker_sender, _rx) = futures::channel::mpsc::channel(100); // client is used for the convenience of creating and inserting the genesis block. let _client = crate::client::new_with_backend::<_, _, runtime::Block, _, runtime::RuntimeApi>( backend.clone(), executor.clone(), + unpin_worker_sender, genesis_block_builder, None, Box::new(TaskExecutor::new()), @@ -475,11 +477,13 @@ mod tests { ) .expect("Creates genesis block builder"); + let (unpin_worker_sender, _rx) = futures::channel::mpsc::channel(100); // client is used for the convenience of creating and inserting the genesis block. let client = crate::client::new_with_backend::<_, _, runtime::Block, _, runtime::RuntimeApi>( backend.clone(), executor.clone(), + unpin_worker_sender, genesis_block_builder, None, Box::new(TaskExecutor::new()), diff --git a/client/state-db/src/lib.rs b/client/state-db/src/lib.rs index 1d3f97abc02c8..68bab755f3254 100644 --- a/client/state-db/src/lib.rs +++ b/client/state-db/src/lib.rs @@ -408,7 +408,7 @@ impl StateDbSync { (&mut self.pruning, &self.mode) { loop { - if dbg!(pruning.window_size()) <= dbg!(constraints.max_blocks.unwrap_or(0) as u64) { + if pruning.window_size() <= constraints.max_blocks.unwrap_or(0) as u64 { break } diff --git a/test-utils/client/src/lib.rs b/test-utils/client/src/lib.rs index 141fd4db8cc4d..e4270640669aa 100644 --- a/test-utils/client/src/lib.rs +++ b/test-utils/client/src/lib.rs @@ -237,10 +237,11 @@ impl ) .expect("Creates genesis block builder"); + let (tx, _rx) = futures::channel::mpsc::channel(100); let client = client::Client::new( self.backend.clone(), executor, - todo!(), + tx, genesis_block_builder, self.fork_blocks, self.bad_blocks, From 65caa1a9925ea22576dbb3fcd38a5900d88ac8ec Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Thu, 12 Jan 2023 09:33:47 +0100 Subject: [PATCH 07/37] Fix pinning hint for state-db --- client/db/src/lib.rs | 33 +++++++++++------------------ client/service/src/client/client.rs | 3 ++- 2 files changed, 14 insertions(+), 22 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 1418bf08e0604..6b399bc20d72e 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -574,11 +574,10 @@ pub struct BlockchainDb { header_metadata_cache: Arc>, header_cache: Mutex>>, pinned_blocks_cache: Arc>, - pinning_enabled: bool, } impl BlockchainDb { - fn new(db: Arc>, pinning_enabled: bool) -> ClientResult { + fn new(db: Arc>) -> ClientResult { let meta = read_meta::(&*db, columns::HEADER)?; let leaves = LeafSet::read_from_db(&*db, columns::META, meta_keys::LEAF_PREFIX)?; Ok(BlockchainDb { @@ -588,7 +587,6 @@ impl BlockchainDb { header_metadata_cache: Arc::new(HeaderMetadataCache::default()), header_cache: Default::default(), pinned_blocks_cache: Arc::new(PinnedBlockCache::new()), - pinning_enabled, }) } @@ -619,10 +617,6 @@ impl BlockchainDb { } fn pin_with_ref(&self, hash: Block::Hash, ref_count: u32) -> ClientResult<()> { - if !self.pinning_enabled { - return Ok(()) - } - let justifications = self.justifications(hash)?; let body = self.body(hash)?; self.pinned_blocks_cache.insert_with_ref(hash, body, justifications, ref_count); @@ -630,17 +624,10 @@ impl BlockchainDb { } fn bump_ref(&self, hash: Block::Hash) { - if !self.pinning_enabled { - return - } self.pinned_blocks_cache.bump_ref(hash); } fn pin(&self, hash: Block::Hash) -> ClientResult<()> { - if !self.pinning_enabled { - return Ok(()) - } - let justifications = self.justifications(hash)?; let body = self.body(hash)?; self.pinned_blocks_cache.insert(hash, body, justifications); @@ -648,10 +635,6 @@ impl BlockchainDb { } fn unpin(&self, hash: &Block::Hash) { - if !self.pinning_enabled { - return - } - self.pinned_blocks_cache.remove(hash); } } @@ -1290,8 +1273,7 @@ impl Backend { let state_pruning_used = state_db.pruning_mode(); let is_archive_pruning = state_pruning_used.is_archive(); - let blockchain = - BlockchainDb::new(db.clone(), config.blocks_pruning != BlocksPruning::KeepAll)?; + let blockchain = BlockchainDb::new(db.clone())?; let storage_db = StorageDb { db: db.clone(), state_db, prefix_keys: !db.supports_ref_counting() }; @@ -2555,7 +2537,16 @@ impl sc_client_api::backend::Backend for Backend { //handle.insert(*hash, ); log::info!(target: "db", "Pinning block in backend, hash: {}, number: {}, num_pinned_blocks: {}", hash, number, pin_store_guard.len()); log::info!(target: "skunert", "Pinned_block_contents: {:?}", pin_store_guard); - let hint = || false; + let hint = || { + let header_metadata = self.blockchain.header_metadata(*hash); + header_metadata + .map(|hdr| { + sc_state_db::NodeDb::get(self.storage.as_ref(), hdr.state_root.as_ref()) + .unwrap_or(None) + .is_some() + }) + .unwrap_or(false) + }; self.storage.state_db.pin(hash, number, hint).map_err(|_| { sp_blockchain::Error::UnknownBlock(format!("State already discarded for {:?}", hash)) })?; diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index 237d91a306634..0dbe3ed98743b 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -177,10 +177,11 @@ where BlockImportOperation = as backend::Backend>::BlockImportOperation, >, { + let (tx, rx) = futures::channel::mpsc::channel(100); new_with_backend( backend, executor, - todo!(), + tx, genesis_block_builder, keystore, spawn_handle, From cc000a310051ee40b07d2a5fc55e12470bbbf0f7 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Thu, 12 Jan 2023 11:12:32 +0100 Subject: [PATCH 08/37] Remove pinning from backend layer --- client/db/src/lib.rs | 104 +++++++++++++------------------------------ 1 file changed, 32 insertions(+), 72 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 6b399bc20d72e..1191fee657743 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -479,8 +479,8 @@ fn cache_header( } struct PinnedBlockCache { - body_cache: RwLock>)>>, - justification_cache: RwLock)>>, + body_cache: RwLock>>)>>, + justification_cache: RwLock>)>>, } impl PinnedBlockCache { @@ -492,49 +492,34 @@ impl PinnedBlockCache { } } - pub fn insert( - &self, - hash: Block::Hash, - extrinsics: Option>, - justifications: Option, - ) { - self.insert_with_ref(hash, extrinsics, justifications, 1); - } - pub fn bump_ref(&self, hash: Block::Hash) { let mut body_cache = self.body_cache.write(); - if let Some(mut entry) = body_cache.get_mut(&hash) { - entry.0 += 1; - } + let mut entry = body_cache.get_or_insert_mut(hash, || (0, None)); + entry.0 += 1; let mut justification_cache = self.justification_cache.write(); - if let Some(mut entry) = justification_cache.get_mut(&hash) { - entry.0 += 1; - } + let mut entry = justification_cache.get_or_insert_mut(hash, || (0, None)); + entry.0 += 1; + } + + pub fn contains(&self, hash: &Block::Hash) -> bool { + let body_cache = self.body_cache.read(); + body_cache.contains(hash) } - pub fn insert_with_ref( + pub fn insert_value( &self, hash: Block::Hash, extrinsics: Option>, justifications: Option, - ref_count: u32, ) { let mut body_cache = self.body_cache.write(); - if let Some(mut entry) = body_cache.get_mut(&hash) { - entry.0 += ref_count; - entry.1 = extrinsics; - } else { - body_cache.put(hash, (ref_count, extrinsics)); - } + let mut entry = body_cache.get_or_insert_mut(hash, || (0, None)); + entry.1 = Some(extrinsics); let mut justification_cache = self.justification_cache.write(); - if let Some(mut entry) = justification_cache.get_mut(&hash) { - entry.0 += ref_count; - entry.1 = justifications; - } else { - justification_cache.put(hash, (ref_count, justifications)); - } + let mut entry = justification_cache.get_or_insert_mut(hash, || (0, None)); + entry.1 = Some(justifications); } pub fn remove(&self, hash: &Block::Hash) { @@ -557,12 +542,18 @@ impl PinnedBlockCache { pub fn justification(&self, hash: &Block::Hash) -> Option> { let justification_cache = self.justification_cache.read(); - justification_cache.peek(hash).map(|element| element.1.clone()) + if let Some(result) = justification_cache.peek(hash) { + return result.1.clone() + }; + None } pub fn body(&self, hash: &Block::Hash) -> Option>> { let body_cache = self.body_cache.read(); - body_cache.peek(hash).map(|element| element.1.clone()) + if let Some(result) = body_cache.peek(hash) { + return result.1.clone() + }; + None } } @@ -616,10 +607,14 @@ impl BlockchainDb { meta.block_gap = gap; } - fn pin_with_ref(&self, hash: Block::Hash, ref_count: u32) -> ClientResult<()> { + fn pin_value(&self, hash: Block::Hash) -> ClientResult<()> { + if !self.pinned_blocks_cache.contains(&hash) { + return Ok(()) + } + let justifications = self.justifications(hash)?; let body = self.body(hash)?; - self.pinned_blocks_cache.insert_with_ref(hash, body, justifications, ref_count); + self.pinned_blocks_cache.insert_value(hash, body, justifications); Ok(()) } @@ -627,13 +622,6 @@ impl BlockchainDb { self.pinned_blocks_cache.bump_ref(hash); } - fn pin(&self, hash: Block::Hash) -> ClientResult<()> { - let justifications = self.justifications(hash)?; - let body = self.body(hash)?; - self.pinned_blocks_cache.insert(hash, body, justifications); - Ok(()) - } - fn unpin(&self, hash: &Block::Hash) { self.pinned_blocks_cache.remove(hash); } @@ -1166,7 +1154,6 @@ impl FrozenForDuration { /// Disk backend keeps data in a key-value store. In archive mode, trie nodes are kept from all /// blocks. Otherwise, trie nodes are kept only from some recent blocks. pub struct Backend { - pinned_block_store: RwLock>, storage: Arc>, offchain_storage: offchain::LocalStorage, blockchain: BlockchainDb, @@ -1281,7 +1268,6 @@ impl Backend { let offchain_storage = offchain::LocalStorage::new(db.clone()); let backend = Backend { - pinned_block_store: RwLock::new(HashMap::new()), storage: Arc::new(storage_db), offchain_storage, blockchain, @@ -1856,11 +1842,8 @@ impl Backend { if finalized >= keep.into() { let number = finalized.saturating_sub(keep.into()); - let pinned_block_store = self.pinned_block_store.read(); if let Some(hash) = self.blockchain.hash(number)? { - if let Some(ref_count) = pinned_block_store.get(&hash) { - self.blockchain.pin_with_ref(hash, *ref_count)?; - } + self.blockchain.pin_value(hash)?; }; self.prune_block(transaction, BlockId::::number(number))?; @@ -1891,10 +1874,7 @@ impl Backend { while self.blockchain.hash(number)? != Some(hash) { match self.blockchain.header(hash)? { Some(header) => { - let pinned_block_store = self.pinned_block_store.read(); - if let Some(ref_count) = pinned_block_store.get(&hash) { - self.blockchain.pin_with_ref(hash, *ref_count)?; - } + self.blockchain.pin_value(hash)?; self.prune_block(transaction, BlockId::::hash(hash))?; number = header.number().saturating_sub(One::one()); @@ -2525,18 +2505,6 @@ impl sc_client_api::backend::Backend for Backend { } fn pin_block(&self, hash: &::Hash, number: u64) -> sp_blockchain::Result<()> { - let mut pin_store_guard = self.pinned_block_store.write(); - match pin_store_guard.entry(*hash) { - Entry::Occupied(mut entry) => { - *entry.get_mut() += 1; - }, - Entry::Vacant(v) => { - v.insert(1); - }, - }; - //handle.insert(*hash, ); - log::info!(target: "db", "Pinning block in backend, hash: {}, number: {}, num_pinned_blocks: {}", hash, number, pin_store_guard.len()); - log::info!(target: "skunert", "Pinned_block_contents: {:?}", pin_store_guard); let hint = || { let header_metadata = self.blockchain.header_metadata(*hash); header_metadata @@ -2555,14 +2523,6 @@ impl sc_client_api::backend::Backend for Backend { } fn unpin_block(&self, hash: &::Hash) { - log::info!(target: "db", "Unpinning block in backend, hash: {}", hash); - let mut pin_store_guard = self.pinned_block_store.write(); - if let Entry::Occupied(mut entry) = pin_store_guard.entry(*hash) { - *entry.get_mut() -= 1; - if *entry.get() == 0 { - entry.remove(); - } - }; self.storage.state_db.unpin(hash); self.blockchain.unpin(hash); } From 8a223253fb414d2bf76c508de467e9211cc5550d Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Thu, 12 Jan 2023 11:41:55 +0100 Subject: [PATCH 09/37] Improve readability --- client/db/src/lib.rs | 75 ++++++++++++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 23 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 1191fee657743..cf867371d10d6 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -45,7 +45,7 @@ use log::{debug, trace, warn}; use lru::LruCache; use parking_lot::{Mutex, RwLock}; use std::{ - collections::{hash_map::Entry, HashMap, HashSet}, + collections::{HashMap, HashSet}, io, num::NonZeroUsize, path::{Path, PathBuf}, @@ -478,9 +478,35 @@ fn cache_header( } } +struct PinnedBlockCacheEntry { + ref_count: u32, + pub value: Option, +} + +impl Default for PinnedBlockCacheEntry { + fn default() -> Self { + Self { ref_count: 0, value: None } + } +} + +impl PinnedBlockCacheEntry { + pub fn decrease_ref(&mut self) { + self.ref_count = self.ref_count.saturating_sub(1); + } + + pub fn increase_ref(&mut self) { + self.ref_count = self.ref_count.saturating_add(1); + } + + pub fn has_no_references(&self) -> bool { + self.ref_count == 0 + } +} + struct PinnedBlockCache { - body_cache: RwLock>>)>>, - justification_cache: RwLock>)>>, + body_cache: RwLock>>>>, + justification_cache: + RwLock>>>, } impl PinnedBlockCache { @@ -494,12 +520,12 @@ impl PinnedBlockCache { pub fn bump_ref(&self, hash: Block::Hash) { let mut body_cache = self.body_cache.write(); - let mut entry = body_cache.get_or_insert_mut(hash, || (0, None)); - entry.0 += 1; + let entry = body_cache.get_or_insert_mut(hash, Default::default); + entry.increase_ref(); let mut justification_cache = self.justification_cache.write(); - let mut entry = justification_cache.get_or_insert_mut(hash, || (0, None)); - entry.0 += 1; + let entry = justification_cache.get_or_insert_mut(hash, Default::default); + entry.increase_ref(); } pub fn contains(&self, hash: &Block::Hash) -> bool { @@ -514,27 +540,27 @@ impl PinnedBlockCache { justifications: Option, ) { let mut body_cache = self.body_cache.write(); - let mut entry = body_cache.get_or_insert_mut(hash, || (0, None)); - entry.1 = Some(extrinsics); + let mut entry = body_cache.get_or_insert_mut(hash, Default::default); + entry.value = Some(extrinsics); let mut justification_cache = self.justification_cache.write(); - let mut entry = justification_cache.get_or_insert_mut(hash, || (0, None)); - entry.1 = Some(justifications); + let mut entry = justification_cache.get_or_insert_mut(hash, Default::default); + entry.value = Some(justifications); } pub fn remove(&self, hash: &Block::Hash) { let mut body_cache = self.body_cache.write(); - if let Some(mut entry) = body_cache.peek_mut(hash) { - entry.0 = entry.0.saturating_sub(1); - if entry.0 == 0 { + if let Some(entry) = body_cache.peek_mut(hash) { + entry.decrease_ref(); + if entry.has_no_references() { body_cache.pop(hash); } } let mut justification_cache = self.justification_cache.write(); - if let Some(mut entry) = justification_cache.peek_mut(hash) { - entry.0 = entry.0.saturating_sub(1); - if entry.0 == 0 { + if let Some(entry) = justification_cache.peek_mut(hash) { + entry.decrease_ref(); + if entry.has_no_references() { justification_cache.pop(hash); } } @@ -542,16 +568,16 @@ impl PinnedBlockCache { pub fn justification(&self, hash: &Block::Hash) -> Option> { let justification_cache = self.justification_cache.read(); - if let Some(result) = justification_cache.peek(hash) { - return result.1.clone() + if let Some(cache_entry) = justification_cache.peek(hash) { + return cache_entry.value.clone() }; None } pub fn body(&self, hash: &Block::Hash) -> Option>> { let body_cache = self.body_cache.read(); - if let Some(result) = body_cache.peek(hash) { - return result.1.clone() + if let Some(cache_entry) = body_cache.peek(hash) { + return cache_entry.value.clone() }; None } @@ -4180,7 +4206,8 @@ pub(crate) mod tests { prev_hash = hash; } - // Block 1 gets pinned twice + // Block 1 gets pinned three times + backend.pin_block(&blocks[1], 1).unwrap(); backend.pin_block(&blocks[1], 1).unwrap(); let mut op = backend.begin_operation().unwrap(); @@ -4210,7 +4237,9 @@ pub(crate) mod tests { assert!(bc.body(blocks[2]).unwrap().is_none()); assert!(bc.body(blocks[3]).unwrap().is_none()); - // After this second unpin, block 1 should be removed + // After these unpins, block 1 should be removed + backend.unpin_block(&blocks[1]); + assert!(bc.body(blocks[1]).unwrap().is_some()); backend.unpin_block(&blocks[1]); assert!(bc.body(blocks[1]).unwrap().is_none()); From d3d208a46bd6c68b62cfa311fc61a7ebb42ea81f Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Thu, 12 Jan 2023 19:11:30 +0100 Subject: [PATCH 10/37] Add justifications to test --- client/db/src/lib.rs | 81 +++++++++++++++++++++++++++++++++----------- 1 file changed, 61 insertions(+), 20 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index cf867371d10d6..56d1a7fc72a5e 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -533,16 +533,18 @@ impl PinnedBlockCache { body_cache.contains(hash) } - pub fn insert_value( - &self, - hash: Block::Hash, - extrinsics: Option>, - justifications: Option, - ) { + pub fn insert_body(&self, hash: Block::Hash, extrinsics: Option>) { + println!("Inserting value for hash {}, body: {:?}", hash, extrinsics); let mut body_cache = self.body_cache.write(); let mut entry = body_cache.get_or_insert_mut(hash, Default::default); entry.value = Some(extrinsics); + } + pub fn insert_justifications(&self, hash: Block::Hash, justifications: Option) { + println!( + "Inserting justifications for hash {}, justifications: {:?}", + hash, justifications + ); let mut justification_cache = self.justification_cache.write(); let mut entry = justification_cache.get_or_insert_mut(hash, Default::default); entry.value = Some(justifications); @@ -553,6 +555,7 @@ impl PinnedBlockCache { if let Some(entry) = body_cache.peek_mut(hash) { entry.decrease_ref(); if entry.has_no_references() { + println!("Removing value for hash {}, ref count zero", hash); body_cache.pop(hash); } } @@ -569,6 +572,10 @@ impl PinnedBlockCache { pub fn justification(&self, hash: &Block::Hash) -> Option> { let justification_cache = self.justification_cache.read(); if let Some(cache_entry) = justification_cache.peek(hash) { + println!( + "Returning cached justification for hash {}, justifications: {:?}", + hash, cache_entry.value + ); return cache_entry.value.clone() }; None @@ -577,6 +584,7 @@ impl PinnedBlockCache { pub fn body(&self, hash: &Block::Hash) -> Option>> { let body_cache = self.body_cache.read(); if let Some(cache_entry) = body_cache.peek(hash) { + println!("Returning cached body for hash {}, body: {:?}", hash, cache_entry.value); return cache_entry.value.clone() }; None @@ -633,14 +641,22 @@ impl BlockchainDb { meta.block_gap = gap; } - fn pin_value(&self, hash: Block::Hash) -> ClientResult<()> { + fn insert_justifications_if_pinned(&self, hash: Block::Hash) -> ClientResult<()> { if !self.pinned_blocks_cache.contains(&hash) { return Ok(()) } - let justifications = self.justifications(hash)?; + self.pinned_blocks_cache.insert_justifications(hash, justifications); + Ok(()) + } + + fn insert_body_if_pinned(&self, hash: Block::Hash) -> ClientResult<()> { + if !self.pinned_blocks_cache.contains(&hash) { + return Ok(()) + } + let body = self.body(hash)?; - self.pinned_blocks_cache.insert_value(hash, body, justifications); + self.pinned_blocks_cache.insert_body(hash, body); Ok(()) } @@ -1857,7 +1873,7 @@ impl Backend { fn prune_blocks( &self, transaction: &mut Transaction, - finalized: NumberFor, + finalized_number: NumberFor, displaced: &FinalizationOutcome>, ) -> ClientResult<()> { match self.blocks_pruning { @@ -1865,19 +1881,21 @@ impl Backend { BlocksPruning::Some(blocks_pruning) => { // Always keep the last finalized block let keep = std::cmp::max(blocks_pruning, 1); - if finalized >= keep.into() { - let number = finalized.saturating_sub(keep.into()); + if finalized_number >= keep.into() { + let number = finalized_number.saturating_sub(keep.into()); + println!("Finaliztion: Pruning block number {}", number); if let Some(hash) = self.blockchain.hash(number)? { - self.blockchain.pin_value(hash)?; + self.blockchain.insert_body_if_pinned(hash)?; + self.blockchain.insert_justifications_if_pinned(hash)?; }; self.prune_block(transaction, BlockId::::number(number))?; } - self.prune_displaced_branches(transaction, finalized, displaced)?; + self.prune_displaced_branches(transaction, finalized_number, displaced)?; }, BlocksPruning::KeepFinalized => { - self.prune_displaced_branches(transaction, finalized, displaced)?; + self.prune_displaced_branches(transaction, finalized_number, displaced)?; }, } Ok(()) @@ -1900,7 +1918,7 @@ impl Backend { while self.blockchain.hash(number)? != Some(hash) { match self.blockchain.header(hash)? { Some(header) => { - self.blockchain.pin_value(hash)?; + self.blockchain.insert_body_if_pinned(hash)?; self.prune_block(transaction, BlockId::::hash(hash))?; number = header.number().saturating_sub(One::one()); @@ -4186,6 +4204,7 @@ pub(crate) mod tests { let mut blocks = Vec::new(); let mut prev_hash = Default::default(); + let build_justification = |i: u64| ([0, 0, 0, 0], vec![i.try_into().unwrap()]); // Block tree: // 0 -> 1 -> 2 -> 3 -> 4 for i in 0..5 { @@ -4200,7 +4219,7 @@ pub(crate) mod tests { ) .unwrap(); blocks.push(hash); - + println!("Block {}: {}", i, hash); // Avoid block pruning. backend.pin_block(&blocks[i as usize], i).unwrap(); @@ -4213,7 +4232,8 @@ pub(crate) mod tests { let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, blocks[4]).unwrap(); for i in 1..5 { - op.mark_finalized(blocks[i], None).unwrap(); + op.mark_finalized(blocks[i], Some(build_justification(i.try_into().unwrap()))) + .unwrap(); } backend.commit_operation(op).unwrap(); @@ -4221,10 +4241,18 @@ pub(crate) mod tests { // Block 0, 1, 2, 3 are pinned and pruning is delayed, // while block 4 is never delayed for pruning as it is finalized. assert_eq!(Some(vec![0.into()]), bc.body(blocks[0]).unwrap()); + assert_eq!(Some(vec![1.into()]), bc.body(blocks[1]).unwrap()); + assert_eq!(Some(vec![2.into()]), bc.body(blocks[2]).unwrap()); + assert_eq!(Some(vec![3.into()]), bc.body(blocks[3]).unwrap()); + assert_eq!(Some(vec![4.into()]), bc.body(blocks[4]).unwrap()); + assert_eq!( + Some(Justifications::from(build_justification(4))), + bc.justifications(blocks[4]).unwrap() + ); // Unpin all blocks. for block in &blocks { @@ -4244,7 +4272,11 @@ pub(crate) mod tests { assert!(bc.body(blocks[1]).unwrap().is_none()); // Block 4 is inside the pruning window and still kept - assert!(bc.body(blocks[4]).unwrap().is_some()); + assert_eq!(Some(vec![4.into()]), bc.body(blocks[4]).unwrap()); + assert_eq!( + Some(Justifications::from(build_justification(4))), + bc.justifications(blocks[4]).unwrap() + ); // Block tree: // 0 -> 1 -> 2 -> 3 -> 4 -> 5 @@ -4253,6 +4285,7 @@ pub(crate) mod tests { .unwrap(); blocks.push(hash); + backend.pin_block(&blocks[4], 4).unwrap(); // Mark block 5 as finalized. let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, blocks[5]).unwrap(); @@ -4265,8 +4298,16 @@ pub(crate) mod tests { assert!(bc.body(blocks[3]).unwrap().is_none()); // Block 4 was unpinned before pruning, it must also get pruned. - assert!(bc.body(blocks[4]).unwrap().is_none()); + assert_eq!(Some(vec![4.into()]), bc.body(blocks[4]).unwrap()); + assert_eq!( + Some(Justifications::from(build_justification(4))), + bc.justifications(blocks[4]).unwrap() + ); assert_eq!(Some(vec![5.into()]), bc.body(blocks[5]).unwrap()); + + backend.unpin_block(&blocks[4]); + assert!(bc.body(blocks[4]).unwrap().is_none()); + assert!(bc.justifications(blocks[4]).unwrap().is_none()); } #[test] From bc2d40c4ad413ce52d959dd76ff2691740e972cf Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Thu, 12 Jan 2023 21:29:21 +0100 Subject: [PATCH 11/37] Fix justification behaviour --- client/db/src/lib.rs | 111 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 99 insertions(+), 12 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 56d1a7fc72a5e..5bc6c1483e0b0 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -528,6 +528,13 @@ impl PinnedBlockCache { entry.increase_ref(); } + pub fn clear(&self) { + let mut body_cache = self.body_cache.write(); + body_cache.clear(); + let mut justification_cache = self.justification_cache.write(); + justification_cache.clear(); + } + pub fn contains(&self, hash: &Block::Hash) -> bool { let body_cache = self.body_cache.read(); body_cache.contains(hash) @@ -641,7 +648,20 @@ impl BlockchainDb { meta.block_gap = gap; } - fn insert_justifications_if_pinned(&self, hash: Block::Hash) -> ClientResult<()> { + fn clear_pinning_cache(&self) { + self.pinned_blocks_cache.clear(); + } + + fn insert_justifications_if_pinned(&self, hash: Block::Hash, justification: Justification) { + if !self.pinned_blocks_cache.contains(&hash) { + return + } + + let justifications = Justifications::from(justification); + self.pinned_blocks_cache.insert_justifications(hash, Some(justifications)); + } + + fn insert_persisted_justifications_if_pinned(&self, hash: Block::Hash) -> ClientResult<()> { if !self.pinned_blocks_cache.contains(&hash) { return Ok(()) } @@ -650,7 +670,7 @@ impl BlockchainDb { Ok(()) } - fn insert_body_if_pinned(&self, hash: Block::Hash) -> ClientResult<()> { + fn insert_persisted_body_if_pinned(&self, hash: Block::Hash) -> ClientResult<()> { if !self.pinned_blocks_cache.contains(&hash) { return Ok(()) } @@ -1447,20 +1467,23 @@ impl Backend { header: &Block::Header, last_finalized: Option, justification: Option, + seen_justifications: &mut HashMap, ) -> ClientResult> { // TODO: ensure best chain contains this block. let number = *header.number(); self.ensure_sequential_finalization(header, last_finalized)?; let with_state = sc_client_api::Backend::have_state_at(self, hash, number); - self.note_finalized(transaction, header, hash, with_state)?; + self.note_finalized(transaction, header, hash, with_state, seen_justifications)?; if let Some(justification) = justification { transaction.set_from_vec( columns::JUSTIFICATIONS, &utils::number_and_hash_to_lookup_key(number, hash)?, - Justifications::from(justification).encode(), + Justifications::from(justification.clone()).encode(), ); + println!("\tFinaliztion: adding justification to map for {}", hash); + seen_justifications.insert(hash, justification); } Ok(MetaUpdate { hash, number, is_best: false, is_finalized: true, with_state }) } @@ -1527,6 +1550,7 @@ impl Backend { (meta.best_number, meta.finalized_hash, meta.finalized_number, meta.block_gap) }; + let mut seen_justifications: HashMap = HashMap::new(); for (block_hash, justification) in operation.finalized_blocks { let block_header = self.blockchain.expect_header(block_hash)?; meta_updates.push(self.finalize_block_with_transaction( @@ -1535,6 +1559,7 @@ impl Backend { &block_header, Some(last_finalized_hash), justification, + &mut seen_justifications, )?); last_finalized_hash = block_hash; last_finalized_num = *block_header.number(); @@ -1707,7 +1732,14 @@ impl Backend { if finalized { // TODO: ensure best chain contains this block. self.ensure_sequential_finalization(header, Some(last_finalized_hash))?; - self.note_finalized(&mut transaction, header, hash, operation.commit_state)?; + let mut seen_justifications = HashMap::new(); + self.note_finalized( + &mut transaction, + header, + hash, + operation.commit_state, + &mut seen_justifications, + )?; } else { // canonicalize blocks which are old enough, regardless of finality. self.force_delayed_canonicalize(&mut transaction)? @@ -1840,6 +1872,7 @@ impl Backend { f_header: &Block::Header, f_hash: Block::Hash, with_state: bool, + seen_justifications: &mut HashMap, ) -> ClientResult<()> { let f_num = *f_header.number(); @@ -1865,7 +1898,7 @@ impl Backend { } let new_displaced = self.blockchain.leaves.write().finalize_height(f_num); - self.prune_blocks(transaction, f_num, &new_displaced)?; + self.prune_blocks(transaction, f_num, &new_displaced, seen_justifications)?; Ok(()) } @@ -1875,6 +1908,7 @@ impl Backend { transaction: &mut Transaction, finalized_number: NumberFor, displaced: &FinalizationOutcome>, + seen_justifications: &mut HashMap, ) -> ClientResult<()> { match self.blocks_pruning { BlocksPruning::KeepAll => {}, @@ -1884,10 +1918,21 @@ impl Backend { if finalized_number >= keep.into() { let number = finalized_number.saturating_sub(keep.into()); - println!("Finaliztion: Pruning block number {}", number); + println!("\tFinaliztion: Pruning block number {}", number); + println!("\tFinaliztion: Finalized block number {}", finalized_number); if let Some(hash) = self.blockchain.hash(number)? { - self.blockchain.insert_body_if_pinned(hash)?; - self.blockchain.insert_justifications_if_pinned(hash)?; + self.blockchain.insert_persisted_body_if_pinned(hash)?; + println!("\tFinaliztion: checking seen justifications for {}", hash); + if let Some(justification) = seen_justifications.remove(&hash) { + println!("\tFinaliztion: found justification in map for {}", hash); + self.blockchain.insert_justifications_if_pinned(hash, justification); + } else { + println!( + "\tFinaliztion: did not find justification in map for {}", + hash + ); + self.blockchain.insert_persisted_justifications_if_pinned(hash)?; + } }; self.prune_block(transaction, BlockId::::number(number))?; @@ -1918,7 +1963,7 @@ impl Backend { while self.blockchain.hash(number)? != Some(hash) { match self.blockchain.header(hash)? { Some(header) => { - self.blockchain.insert_body_if_pinned(hash)?; + self.blockchain.insert_persisted_body_if_pinned(hash)?; self.prune_block(transaction, BlockId::::hash(hash))?; number = header.number().saturating_sub(One::one()); @@ -2150,6 +2195,7 @@ impl sc_client_api::backend::Backend for Backend { .state_db .reset(state_meta_db) .map_err(sp_blockchain::Error::from_state_db)?; + self.blockchain.clear_pinning_cache(); Err(e) } else { self.storage.state_db.sync(); @@ -2165,12 +2211,14 @@ impl sc_client_api::backend::Backend for Backend { let mut transaction = Transaction::new(); let header = self.blockchain.expect_header(hash)?; + let mut seen_justifications = HashMap::new(); let m = self.finalize_block_with_transaction( &mut transaction, hash, &header, None, justification, + &mut seen_justifications, )?; self.storage.db.commit(transaction)?; @@ -4243,10 +4291,22 @@ pub(crate) mod tests { assert_eq!(Some(vec![0.into()]), bc.body(blocks[0]).unwrap()); assert_eq!(Some(vec![1.into()]), bc.body(blocks[1]).unwrap()); + assert_eq!( + Some(Justifications::from(build_justification(1))), + bc.justifications(blocks[1]).unwrap() + ); assert_eq!(Some(vec![2.into()]), bc.body(blocks[2]).unwrap()); + assert_eq!( + Some(Justifications::from(build_justification(2))), + bc.justifications(blocks[2]).unwrap() + ); assert_eq!(Some(vec![3.into()]), bc.body(blocks[3]).unwrap()); + assert_eq!( + Some(Justifications::from(build_justification(3))), + bc.justifications(blocks[3]).unwrap() + ); assert_eq!(Some(vec![4.into()]), bc.body(blocks[4]).unwrap()); assert_eq!( @@ -4262,14 +4322,19 @@ pub(crate) mod tests { assert!(bc.body(blocks[0]).unwrap().is_none()); // Block 1 was pinned twice, we expect it to be in-memory assert!(bc.body(blocks[1]).unwrap().is_some()); + assert!(bc.justifications(blocks[1]).unwrap().is_some()); assert!(bc.body(blocks[2]).unwrap().is_none()); + assert!(bc.justifications(blocks[2]).unwrap().is_none()); assert!(bc.body(blocks[3]).unwrap().is_none()); + assert!(bc.justifications(blocks[3]).unwrap().is_none()); // After these unpins, block 1 should be removed backend.unpin_block(&blocks[1]); assert!(bc.body(blocks[1]).unwrap().is_some()); + assert!(bc.justifications(blocks[1]).unwrap().is_some()); backend.unpin_block(&blocks[1]); assert!(bc.body(blocks[1]).unwrap().is_none()); + assert!(bc.justifications(blocks[1]).unwrap().is_none()); // Block 4 is inside the pruning window and still kept assert_eq!(Some(vec![4.into()]), bc.body(blocks[4]).unwrap()); @@ -4289,7 +4354,7 @@ pub(crate) mod tests { // Mark block 5 as finalized. let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, blocks[5]).unwrap(); - op.mark_finalized(blocks[5], None).unwrap(); + op.mark_finalized(blocks[5], Some(build_justification(5))).unwrap(); backend.commit_operation(op).unwrap(); assert!(bc.body(blocks[0]).unwrap().is_none()); @@ -4297,7 +4362,6 @@ pub(crate) mod tests { assert!(bc.body(blocks[2]).unwrap().is_none()); assert!(bc.body(blocks[3]).unwrap().is_none()); - // Block 4 was unpinned before pruning, it must also get pruned. assert_eq!(Some(vec![4.into()]), bc.body(blocks[4]).unwrap()); assert_eq!( Some(Justifications::from(build_justification(4))), @@ -4308,6 +4372,29 @@ pub(crate) mod tests { backend.unpin_block(&blocks[4]); assert!(bc.body(blocks[4]).unwrap().is_none()); assert!(bc.justifications(blocks[4]).unwrap().is_none()); + + // Test that appended justifications are transferred correctly to the in-memory cache. + backend.append_justification(blocks[5], ([0, 0, 0, 1], vec![42])).unwrap(); + + let hash = + insert_block(&backend, 6, blocks[5], None, Default::default(), vec![6.into()], None) + .unwrap(); + blocks.push(hash); + + // Pin block 5 so it gets loaded into the cache on prune + backend.pin_block(&blocks[5], 5).unwrap(); + + // Finalize block 6 so block 5 gets pruned. Since it is pinned both justifications should be + // in memory. + let mut op = backend.begin_operation().unwrap(); + backend.begin_state_operation(&mut op, blocks[6]).unwrap(); + op.mark_finalized(blocks[6], None).unwrap(); + backend.commit_operation(op).unwrap(); + + assert_eq!(Some(vec![5.into()]), bc.body(blocks[5]).unwrap()); + let mut expected = Justifications::from(build_justification(5)); + expected.append(([0, 0, 0, 1], vec![42])); + assert_eq!(Some(expected), bc.justifications(blocks[5]).unwrap()); } #[test] From da7a855dab0dc8c81ff82f2bd8743e13004b27ca Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Thu, 12 Jan 2023 22:12:55 +0100 Subject: [PATCH 12/37] Remove debug prints --- client/db/src/lib.rs | 21 ------------ client/service/src/client/client.rs | 50 ++++++++++++----------------- 2 files changed, 21 insertions(+), 50 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 5bc6c1483e0b0..7e2326c839395 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -541,17 +541,12 @@ impl PinnedBlockCache { } pub fn insert_body(&self, hash: Block::Hash, extrinsics: Option>) { - println!("Inserting value for hash {}, body: {:?}", hash, extrinsics); let mut body_cache = self.body_cache.write(); let mut entry = body_cache.get_or_insert_mut(hash, Default::default); entry.value = Some(extrinsics); } pub fn insert_justifications(&self, hash: Block::Hash, justifications: Option) { - println!( - "Inserting justifications for hash {}, justifications: {:?}", - hash, justifications - ); let mut justification_cache = self.justification_cache.write(); let mut entry = justification_cache.get_or_insert_mut(hash, Default::default); entry.value = Some(justifications); @@ -562,7 +557,6 @@ impl PinnedBlockCache { if let Some(entry) = body_cache.peek_mut(hash) { entry.decrease_ref(); if entry.has_no_references() { - println!("Removing value for hash {}, ref count zero", hash); body_cache.pop(hash); } } @@ -579,10 +573,6 @@ impl PinnedBlockCache { pub fn justification(&self, hash: &Block::Hash) -> Option> { let justification_cache = self.justification_cache.read(); if let Some(cache_entry) = justification_cache.peek(hash) { - println!( - "Returning cached justification for hash {}, justifications: {:?}", - hash, cache_entry.value - ); return cache_entry.value.clone() }; None @@ -591,7 +581,6 @@ impl PinnedBlockCache { pub fn body(&self, hash: &Block::Hash) -> Option>> { let body_cache = self.body_cache.read(); if let Some(cache_entry) = body_cache.peek(hash) { - println!("Returning cached body for hash {}, body: {:?}", hash, cache_entry.value); return cache_entry.value.clone() }; None @@ -1482,7 +1471,6 @@ impl Backend { &utils::number_and_hash_to_lookup_key(number, hash)?, Justifications::from(justification.clone()).encode(), ); - println!("\tFinaliztion: adding justification to map for {}", hash); seen_justifications.insert(hash, justification); } Ok(MetaUpdate { hash, number, is_best: false, is_finalized: true, with_state }) @@ -1918,19 +1906,11 @@ impl Backend { if finalized_number >= keep.into() { let number = finalized_number.saturating_sub(keep.into()); - println!("\tFinaliztion: Pruning block number {}", number); - println!("\tFinaliztion: Finalized block number {}", finalized_number); if let Some(hash) = self.blockchain.hash(number)? { self.blockchain.insert_persisted_body_if_pinned(hash)?; - println!("\tFinaliztion: checking seen justifications for {}", hash); if let Some(justification) = seen_justifications.remove(&hash) { - println!("\tFinaliztion: found justification in map for {}", hash); self.blockchain.insert_justifications_if_pinned(hash, justification); } else { - println!( - "\tFinaliztion: did not find justification in map for {}", - hash - ); self.blockchain.insert_persisted_justifications_if_pinned(hash)?; } }; @@ -4267,7 +4247,6 @@ pub(crate) mod tests { ) .unwrap(); blocks.push(hash); - println!("Block {}: {}", i, hash); // Avoid block pruning. backend.pin_block(&blocks[i as usize], i).unwrap(); diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index 0dbe3ed98743b..c7bb63034fc26 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -22,7 +22,7 @@ use super::{ block_rules::{BlockRules, LookupResult as BlockLookupResult}, genesis::BuildGenesisBlock, }; -use log::{info, trace, warn}; +use log::{error, info, trace, warn}; use parking_lot::{Mutex, RwLock}; use prometheus_endpoint::Registry; use rand::Rng; @@ -337,36 +337,28 @@ where self.backend.commit_operation(op)?; - match (&finality_notification, &import_notification) { - (Some(ref fin), Some(ref imp)) if fin.hash == imp.hash => { - self.backend.pin_block( - &fin.hash, - finality_num.expect("Should work").saturated_into::(), - ); - }, - (Some(ref fin), Some(ref imp)) => { - self.backend.pin_block( - &fin.hash, - finality_num.expect("Should work").saturated_into::(), - ); - self.backend.pin_block( - &imp.hash, - import_num.expect("Should work").saturated_into::(), - ); - }, - (None, Some(ref imp)) => { - self.backend.pin_block( - &imp.hash, - import_num.expect("Should work").saturated_into::(), + if let Some(ref notification) = finality_notification { + if let Err(err) = self.backend.pin_block( + ¬ification.hash, + finality_num.expect("Should work").saturated_into::(), + ) { + error!( + "Unable to pin block for finality notification. hash: {}, Error: {}", + notification.hash, err ); - }, - (Some(ref fin), None) => { - self.backend.pin_block( - &fin.hash, - finality_num.expect("Should work").saturated_into::(), + }; + } + + if let Some(ref notification) = import_notification { + if let Err(err) = self.backend.pin_block( + ¬ification.hash, + import_num.expect("Should work").saturated_into::(), + ) { + error!( + "Unable to pin block for import notification. hash: {}, Error: {}", + notification.hash, err ); - }, - (_, _) => {}, + }; } self.notify_finalized(finality_notification)?; From 7daf11c4fec79a977b126222c00336f8f32c8597 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 16 Jan 2023 10:12:44 +0100 Subject: [PATCH 13/37] Convert channels to tracing_unbounded --- Cargo.lock | 1 + client/api/src/client.rs | 18 ++++++++++-------- client/finality-grandpa/src/until_imported.rs | 4 ++-- client/service/src/builder.rs | 5 +++-- client/service/src/client/call_executor.rs | 5 +++-- client/service/src/client/client.rs | 8 ++++---- test-utils/client/Cargo.toml | 1 + test-utils/client/src/lib.rs | 3 ++- 8 files changed, 26 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a615569be9887..599f3a9f52533 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10654,6 +10654,7 @@ dependencies = [ "sc-executor", "sc-offchain", "sc-service", + "sc-utils", "serde", "serde_json", "sp-blockchain", diff --git a/client/api/src/client.rs b/client/api/src/client.rs index 9738a67ceb1f6..44b3accd6e964 100644 --- a/client/api/src/client.rs +++ b/client/api/src/client.rs @@ -18,7 +18,6 @@ //! A set of APIs supported by the client along with their primitives. -use futures::channel::mpsc::Sender; use sp_consensus::BlockOrigin; use sp_core::storage::StorageKey; use sp_runtime::{ @@ -31,7 +30,7 @@ use std::{collections::HashSet, fmt, sync::Arc}; use crate::{blockchain::Info, notifications::StorageEventStream, FinalizeSummary, ImportSummary}; use sc_transaction_pool_api::ChainEvent; -use sc_utils::mpsc::TracingUnboundedReceiver; +use sc_utils::mpsc::{TracingUnboundedReceiver, TracingUnboundedSender}; use sp_blockchain; /// Type that implements `futures::Stream` of block import events. @@ -269,19 +268,22 @@ impl fmt::Display for UsageInfo { #[derive(Debug)] pub struct UnpinHandle { hash: Block::Hash, - unpin_worker_sender: Sender, + unpin_worker_sender: TracingUnboundedSender, } impl UnpinHandle { /// Create a new UnpinHandle - pub fn new(hash: Block::Hash, unpin_worker_sender: Sender) -> Self { + pub fn new( + hash: Block::Hash, + unpin_worker_sender: TracingUnboundedSender, + ) -> Self { Self { hash, unpin_worker_sender } } } impl Drop for UnpinHandle { fn drop(&mut self) { - if let Err(err) = self.unpin_worker_sender.try_send(self.hash) { + if let Err(err) = self.unpin_worker_sender.unbounded_send(self.hash) { log::error!(target: "db", "Unable to unpin block with hash: {}, error: {:?}", self.hash, err); }; } @@ -313,7 +315,7 @@ impl BlockImportNotification { header: Block::Header, is_new_best: bool, tree_route: Option>>, - unpin_worker_sender: Sender, + unpin_worker_sender: TracingUnboundedSender, ) -> Self { Self { hash, @@ -365,7 +367,7 @@ impl FinalityNotification { /// Create finality notification from finality summary. pub fn from_summary( mut summary: FinalizeSummary, - unpin_worker_sender: Sender, + unpin_worker_sender: TracingUnboundedSender, ) -> FinalityNotification { let hash = summary.finalized.pop().unwrap_or_default(); FinalityNotification { @@ -382,7 +384,7 @@ impl BlockImportNotification { /// Create finality notification from finality summary. pub fn from_summary( summary: ImportSummary, - unpin_worker_sender: Sender, + unpin_worker_sender: TracingUnboundedSender, ) -> BlockImportNotification { let hash = summary.hash; BlockImportNotification { diff --git a/client/finality-grandpa/src/until_imported.rs b/client/finality-grandpa/src/until_imported.rs index 5a29b382fce4b..c03aebc6ab404 100644 --- a/client/finality-grandpa/src/until_imported.rs +++ b/client/finality-grandpa/src/until_imported.rs @@ -565,7 +565,7 @@ mod tests { use finality_grandpa::Precommit; use futures::future::Either; use futures_timer::Delay; - use sc_client_api::{BlockImportNotification, UnpinHandle}; + use sc_client_api::BlockImportNotification; use sc_utils::mpsc::{tracing_unbounded, TracingUnboundedSender}; use sp_consensus::BlockOrigin; use sp_core::crypto::UncheckedFrom; @@ -593,7 +593,7 @@ mod tests { fn import_header(&self, header: Header) { let hash = header.hash(); let number = *header.number(); - let (tx, rx) = futures::channel::mpsc::channel(100); + let (tx, rx) = tracing_unbounded("unpin-worker-channel", 10_000); self.known_blocks.lock().insert(hash, number); self.sender .unbounded_send(BlockImportNotification::::new( diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index 5abd0bc7a6768..d44340e7dbe22 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -335,10 +335,11 @@ where execution_extensions, )?; - let (unpin_worker_sender, mut rx) = futures::channel::mpsc::channel::(100); + let (unpin_worker_sender, mut rx) = + tracing_unbounded::("unpin-worker-channel", 10_000); let task_backend = backend.clone(); spawn_handle.spawn( - "pinning-worker", + "unpin-worker", None, async move { log::info!(target: "db", "Starting worker task for unpinning."); diff --git a/client/service/src/client/call_executor.rs b/client/service/src/client/call_executor.rs index a3a3116b4db3e..f452470cb591b 100644 --- a/client/service/src/client/call_executor.rs +++ b/client/service/src/client/call_executor.rs @@ -361,6 +361,7 @@ mod tests { use super::*; use sc_client_api::in_mem; use sc_executor::{NativeElseWasmExecutor, WasmExecutionMethod}; + use sc_utils::mpsc::tracing_unbounded; use sp_core::{ testing::TaskExecutor, traits::{FetchRuntimeCode, WrappedRuntimeCode}, @@ -399,7 +400,7 @@ mod tests { ) .expect("Creates genesis block builder"); - let (unpin_worker_sender, _rx) = futures::channel::mpsc::channel(100); + let (unpin_worker_sender, _rx) = tracing_unbounded("unpin-worker", 10_000); // client is used for the convenience of creating and inserting the genesis block. let _client = crate::client::new_with_backend::<_, _, runtime::Block, _, runtime::RuntimeApi>( @@ -477,7 +478,7 @@ mod tests { ) .expect("Creates genesis block builder"); - let (unpin_worker_sender, _rx) = futures::channel::mpsc::channel(100); + let (unpin_worker_sender, _rx) = tracing_unbounded("unpin_worker", 10_000); // client is used for the convenience of creating and inserting the genesis block. let client = crate::client::new_with_backend::<_, _, runtime::Block, _, runtime::RuntimeApi>( diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index c7bb63034fc26..06e5b6aecd936 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -117,7 +117,7 @@ where block_rules: BlockRules, config: ClientConfig, telemetry: Option, - unpin_worker_sender: futures::channel::mpsc::Sender, + unpin_worker_sender: TracingUnboundedSender, _phantom: PhantomData, } @@ -177,7 +177,7 @@ where BlockImportOperation = as backend::Backend>::BlockImportOperation, >, { - let (tx, rx) = futures::channel::mpsc::channel(100); + let (tx, _rx) = sc_utils::mpsc::tracing_unbounded("unpin-worker-channel", 10_000); new_with_backend( backend, executor, @@ -225,7 +225,7 @@ impl Default for ClientConfig { pub fn new_with_backend( backend: Arc, executor: E, - unpin_worker_sender: futures::channel::mpsc::Sender, + unpin_worker_sender: TracingUnboundedSender, genesis_block_builder: G, keystore: Option, spawn_handle: Box, @@ -400,7 +400,7 @@ where pub fn new( backend: Arc, executor: E, - unpin_worker_sender: futures::channel::mpsc::Sender, + unpin_worker_sender: TracingUnboundedSender, genesis_block_builder: G, fork_blocks: ForkBlocks, bad_blocks: BadBlocks, diff --git a/test-utils/client/Cargo.toml b/test-utils/client/Cargo.toml index 106ec21d79464..34a3796ea5c53 100644 --- a/test-utils/client/Cargo.toml +++ b/test-utils/client/Cargo.toml @@ -20,6 +20,7 @@ futures = "0.3.21" serde = "1.0.136" serde_json = "1.0.85" sc-client-api = { version = "4.0.0-dev", path = "../../client/api" } +sc-utils = { version = "4.0.0-dev", path = "../../client/utils" } sc-client-db = { version = "0.10.0-dev", default-features = false, features = [ "test-helpers", ], path = "../../client/db" } diff --git a/test-utils/client/src/lib.rs b/test-utils/client/src/lib.rs index e4270640669aa..3bd845e345af5 100644 --- a/test-utils/client/src/lib.rs +++ b/test-utils/client/src/lib.rs @@ -29,6 +29,7 @@ pub use sc_client_api::{ pub use sc_client_db::{self, Backend, BlocksPruning}; pub use sc_executor::{self, NativeElseWasmExecutor, WasmExecutionMethod}; pub use sc_service::{client, RpcHandlers}; +use sc_utils::mpsc::tracing_unbounded; pub use sp_consensus; pub use sp_keyring::{ ed25519::Keyring as Ed25519Keyring, sr25519::Keyring as Sr25519Keyring, AccountKeyring, @@ -237,7 +238,7 @@ impl ) .expect("Creates genesis block builder"); - let (tx, _rx) = futures::channel::mpsc::channel(100); + let (tx, _rx) = tracing_unbounded("unpin-worker", 10_000); let client = client::Client::new( self.backend.clone(), executor, From 2fb995cda7fdde0f7b4b63b04ce9d5b0bb84676a Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 16 Jan 2023 10:18:03 +0100 Subject: [PATCH 14/37] Add comments to the test --- client/db/src/lib.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 7e2326c839395..4e67777c3429d 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -4252,10 +4252,12 @@ pub(crate) mod tests { prev_hash = hash; } + // Block 1 gets pinned three times backend.pin_block(&blocks[1], 1).unwrap(); backend.pin_block(&blocks[1], 1).unwrap(); + // Finalize all blocks. This will trigger pruning. let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, blocks[4]).unwrap(); for i in 1..5 { @@ -4265,8 +4267,8 @@ pub(crate) mod tests { backend.commit_operation(op).unwrap(); let bc = backend.blockchain(); - // Block 0, 1, 2, 3 are pinned and pruning is delayed, - // while block 4 is never delayed for pruning as it is finalized. + // Block 0, 1, 2, 3 are pinned, so all values should be cached. + // Block 4 is inside the pruning window, its value is in db. assert_eq!(Some(vec![0.into()]), bc.body(blocks[0]).unwrap()); assert_eq!(Some(vec![1.into()]), bc.body(blocks[1]).unwrap()); @@ -4293,13 +4295,13 @@ pub(crate) mod tests { bc.justifications(blocks[4]).unwrap() ); - // Unpin all blocks. + // Unpin all blocks. Values should be removed from cache. for block in &blocks { backend.unpin_block(&block); } assert!(bc.body(blocks[0]).unwrap().is_none()); - // Block 1 was pinned twice, we expect it to be in-memory + // Block 1 was pinned twice, we expect it to be still cached assert!(bc.body(blocks[1]).unwrap().is_some()); assert!(bc.justifications(blocks[1]).unwrap().is_some()); assert!(bc.body(blocks[2]).unwrap().is_none()); @@ -4307,7 +4309,7 @@ pub(crate) mod tests { assert!(bc.body(blocks[3]).unwrap().is_none()); assert!(bc.justifications(blocks[3]).unwrap().is_none()); - // After these unpins, block 1 should be removed + // After these unpins, block 1 should also be removed backend.unpin_block(&blocks[1]); assert!(bc.body(blocks[1]).unwrap().is_some()); assert!(bc.justifications(blocks[1]).unwrap().is_some()); @@ -4352,7 +4354,7 @@ pub(crate) mod tests { assert!(bc.body(blocks[4]).unwrap().is_none()); assert!(bc.justifications(blocks[4]).unwrap().is_none()); - // Test that appended justifications are transferred correctly to the in-memory cache. + // Append a justification to block 5. backend.append_justification(blocks[5], ([0, 0, 0, 1], vec![42])).unwrap(); let hash = From 7a072dd081cd9c8cb80c3b8fe589f1dc3947a0fb Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 16 Jan 2023 14:34:36 +0100 Subject: [PATCH 15/37] Documentation and Cleanup --- client/api/src/backend.rs | 18 +++++++++++++-- client/api/src/client.rs | 46 +++++++++++++++++++++++++++++++-------- client/api/src/in_mem.rs | 6 ++--- client/db/src/lib.rs | 24 +++++++++++++++++--- 4 files changed, 76 insertions(+), 18 deletions(-) diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs index 71152905ee02e..62eac1b45cf64 100644 --- a/client/api/src/backend.rs +++ b/client/api/src/backend.rs @@ -436,12 +436,24 @@ pub trait StorageProvider> { /// /// Manages the data layer. /// -/// Note on state pruning: while an object from `state_at` is alive, the state +/// # State Pruning +/// +/// While an object from `state_at` is alive, the state /// should not be pruned. The backend should internally reference-count /// its state objects. /// /// The same applies for live `BlockImportOperation`s: while an import operation building on a /// parent `P` is alive, the state for `P` should not be pruned. +/// +/// # Block Pruning +/// +/// Users can pin blocks in memory by calling `pin_block`. When +/// a block would be pruned, its value is kept in an in-memory cache +/// until it is unpinned via `unpin_block`. +/// +/// While a block is pinned, its state is also preserved. +/// +/// The backend should internally reference count the number of pin / unpin calls. pub trait Backend: AuxStore + Send + Sync { /// Associated block insertion operation type. type BlockImportOperation: BlockImportOperation; @@ -502,7 +514,9 @@ pub trait Backend: AuxStore + Send + Sync { /// Returns a handle to offchain storage. fn offchain_storage(&self) -> Option; - /// Pin the block to prevent pruning. + /// Pin the block to keep bodies, justification and state available after pruning. + /// Number of pins are reference counted. Users need to make sure to perform + /// one call to `unpin_block` per call to `pin_block`. fn pin_block(&self, hash: &Block::Hash, number: u64) -> sp_blockchain::Result<()>; /// Unpin the block to allow pruning. diff --git a/client/api/src/client.rs b/client/api/src/client.rs index 44b3accd6e964..89f50d0fde3e0 100644 --- a/client/api/src/client.rs +++ b/client/api/src/client.rs @@ -266,12 +266,13 @@ impl fmt::Display for UsageInfo { /// Sends a message to the pinning-worker once dropped to unpin a block in the backend. #[derive(Debug)] -pub struct UnpinHandle { - hash: Block::Hash, +pub struct UnpinHandleInner { + /// Hash of the block pinned by this handle + pub hash: Block::Hash, unpin_worker_sender: TracingUnboundedSender, } -impl UnpinHandle { +impl UnpinHandleInner { /// Create a new UnpinHandle pub fn new( hash: Block::Hash, @@ -281,7 +282,7 @@ impl UnpinHandle { } } -impl Drop for UnpinHandle { +impl Drop for UnpinHandleInner { fn drop(&mut self) { if let Err(err) = self.unpin_worker_sender.unbounded_send(self.hash) { log::error!(target: "db", "Unable to unpin block with hash: {}, error: {:?}", self.hash, err); @@ -289,6 +290,25 @@ impl Drop for UnpinHandle { } } +/// Keeps a specific block pinned while the handle is alive. +#[derive(Debug, Clone)] +pub struct UnpinHandle(Arc>); + +impl UnpinHandle { + /// Create a new UnpinHandle + pub fn new( + hash: Block::Hash, + unpin_worker_sender: TracingUnboundedSender, + ) -> UnpinHandle { + UnpinHandle(Arc::new(UnpinHandleInner::new(hash, unpin_worker_sender))) + } + + /// Hash of the block this handle is unpinning on drop + pub fn hash(&self) -> Block::Hash { + self.0.hash + } +} + /// Summary of an imported block #[derive(Clone, Debug)] pub struct BlockImportNotification { @@ -305,10 +325,11 @@ pub struct BlockImportNotification { /// If `None`, there was no re-org while importing. pub tree_route: Option>>, /// Handle to unpin the block this notification is for - _unpin_handle: Arc>, + _unpin_handle: UnpinHandle, } impl BlockImportNotification { + /// Create new notification pub fn new( hash: Block::Hash, origin: BlockOrigin, @@ -323,9 +344,16 @@ impl BlockImportNotification { header, is_new_best, tree_route, - _unpin_handle: Arc::new(UnpinHandle::new(hash, unpin_worker_sender)), + _unpin_handle: UnpinHandle::new(hash, unpin_worker_sender), } } + + /// Consume this notification and extract the unpin handle. + /// + /// Note: Only use this if you want to keep the block pinned in the backend. + pub fn into_unpin_handle(self) -> UnpinHandle { + self._unpin_handle + } } /// Summary of a finalized block. @@ -342,7 +370,7 @@ pub struct FinalityNotification { /// Stale branches heads. pub stale_heads: Arc<[Block::Hash]>, /// Handle to unpin the block this notification is for - _unpin_handle: Arc>, + _unpin_handle: UnpinHandle, } impl TryFrom> for ChainEvent { @@ -375,7 +403,7 @@ impl FinalityNotification { header: summary.header, tree_route: Arc::from(summary.finalized), stale_heads: Arc::from(summary.stale_heads), - _unpin_handle: Arc::new(UnpinHandle { hash, unpin_worker_sender }), + _unpin_handle: UnpinHandle::new(hash, unpin_worker_sender), } } } @@ -393,7 +421,7 @@ impl BlockImportNotification { header: summary.header, is_new_best: summary.is_new_best, tree_route: summary.tree_route.map(Arc::new), - _unpin_handle: Arc::new(UnpinHandle { hash, unpin_worker_sender }), + _unpin_handle: UnpinHandle::new(hash, unpin_worker_sender), } } } diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs index c63cb7557f159..82a0868f6c72a 100644 --- a/client/api/src/in_mem.rs +++ b/client/api/src/in_mem.rs @@ -790,12 +790,10 @@ where } fn pin_block(&self, _: &::Hash, _: u64) -> blockchain::Result<()> { - unimplemented!() + Ok(()) } - fn unpin_block(&self, _: &::Hash) { - unimplemented!() - } + fn unpin_block(&self, _: &::Hash) {} } impl backend::LocalBackend for Backend where Block::Hash: Ord {} diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 4e67777c3429d..a6691bf1b7d49 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -112,7 +112,7 @@ pub type DbStateBuilder = sp_state_machine::TrieBackendBuilder< /// Length of a [`DbHash`]. const DB_HASH_LEN: usize = 32; -const PINNING_CACHE_SIZE: usize = 256; +const PINNING_CACHE_SIZE: usize = 1024; /// Hash type that this backend uses for the database. pub type DbHash = sp_core::H256; @@ -520,6 +520,10 @@ impl PinnedBlockCache { pub fn bump_ref(&self, hash: Block::Hash) { let mut body_cache = self.body_cache.write(); + if body_cache.len() == PINNING_CACHE_SIZE { + log::warn!(target: "db", "Maximum size of pinning cache reached. Removing items to make space. max_size = {}", PINNING_CACHE_SIZE); + } + let entry = body_cache.get_or_insert_mut(hash, Default::default); entry.increase_ref(); @@ -637,10 +641,14 @@ impl BlockchainDb { meta.block_gap = gap; } + /// Empty the cache of pinned items. fn clear_pinning_cache(&self) { self.pinned_blocks_cache.clear(); } + /// Load a justification into the cache of pinned items. + /// Reference count of the item will not be increased. Use this + /// to load values for items items into the cache which have already been pinned. fn insert_justifications_if_pinned(&self, hash: Block::Hash, justification: Justification) { if !self.pinned_blocks_cache.contains(&hash) { return @@ -650,6 +658,9 @@ impl BlockchainDb { self.pinned_blocks_cache.insert_justifications(hash, Some(justifications)); } + /// Load a justification from the db into the cache of pinned items. + /// Reference count of the item will not be increased. Use this + /// to load values for items items into the cache which have already been pinned. fn insert_persisted_justifications_if_pinned(&self, hash: Block::Hash) -> ClientResult<()> { if !self.pinned_blocks_cache.contains(&hash) { return Ok(()) @@ -659,6 +670,9 @@ impl BlockchainDb { Ok(()) } + /// Load a block body from the db into the cache of pinned items. + /// Reference count of the item will not be increased. Use this + /// to load values for items items into the cache which have already been pinned. fn insert_persisted_body_if_pinned(&self, hash: Block::Hash) -> ClientResult<()> { if !self.pinned_blocks_cache.contains(&hash) { return Ok(()) @@ -669,10 +683,12 @@ impl BlockchainDb { Ok(()) } + /// Bump reference count for pinned item. fn bump_ref(&self, hash: Block::Hash) { self.pinned_blocks_cache.bump_ref(hash); } + /// Decrease reference count for pinned item and remove if reference count is 0. fn unpin(&self, hash: &Block::Hash) { self.pinned_blocks_cache.remove(hash); } @@ -733,7 +749,6 @@ impl sc_client_api::blockchain::HeaderBackend for Blockcha impl sc_client_api::blockchain::Backend for BlockchainDb { fn body(&self, hash: Block::Hash) -> ClientResult>> { if let Some(result) = self.pinned_blocks_cache.body(&hash) { - log::info!(target: "skunert", "Found block data for {:?} in pinned cache.", hash); return Ok(result) } @@ -803,7 +818,6 @@ impl sc_client_api::blockchain::Backend for BlockchainDb ClientResult> { if let Some(result) = self.pinned_blocks_cache.justification(&hash) { - log::info!(target: "skunert", "Found justification data for {:?} in pinned cache.", hash); return Ok(result) } @@ -1906,8 +1920,12 @@ impl Backend { if finalized_number >= keep.into() { let number = finalized_number.saturating_sub(keep.into()); + // Before we prune a block, check if it is pinned if let Some(hash) = self.blockchain.hash(number)? { self.blockchain.insert_persisted_body_if_pinned(hash)?; + + // If the block was finalized in this transaction, it will not be in the db + // yet. if let Some(justification) = seen_justifications.remove(&hash) { self.blockchain.insert_justifications_if_pinned(hash, justification); } else { From 92e392882abf9bbcd13ad593857f9faf8a2251a9 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 16 Jan 2023 15:47:32 +0100 Subject: [PATCH 16/37] Move task start to client --- client/db/src/lib.rs | 2 +- client/service/src/builder.rs | 23 +++-------------------- client/service/src/client/client.rs | 25 ++++++++++++++++++++----- test-utils/client/src/lib.rs | 10 +++++----- 4 files changed, 29 insertions(+), 31 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index a6691bf1b7d49..6b1abf2b974b9 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -521,7 +521,7 @@ impl PinnedBlockCache { pub fn bump_ref(&self, hash: Block::Hash) { let mut body_cache = self.body_cache.write(); if body_cache.len() == PINNING_CACHE_SIZE { - log::warn!(target: "db", "Maximum size of pinning cache reached. Removing items to make space. max_size = {}", PINNING_CACHE_SIZE); + log::warn!(target: "db-pinning", "Maximum size of pinning cache reached. Removing items to make space. max_size = {}", PINNING_CACHE_SIZE); } let entry = body_cache.get_or_insert_mut(hash, Default::default); diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index d44340e7dbe22..0b09f550ce338 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -31,9 +31,8 @@ use log::info; use prometheus_endpoint::Registry; use sc_chain_spec::get_extension; use sc_client_api::{ - execution_extensions::ExecutionExtensions, proof_provider::ProofProvider, Backend as BackendT, - BadBlocks, BlockBackend, BlockchainEvents, ExecutorProvider, ForkBlocks, StorageProvider, - UsageProvider, + execution_extensions::ExecutionExtensions, proof_provider::ProofProvider, BadBlocks, + BlockBackend, BlockchainEvents, ExecutorProvider, ForkBlocks, StorageProvider, UsageProvider, }; use sc_client_db::{Backend, DatabaseSettings}; use sc_consensus::import_queue::ImportQueue; @@ -335,26 +334,10 @@ where execution_extensions, )?; - let (unpin_worker_sender, mut rx) = - tracing_unbounded::("unpin-worker-channel", 10_000); - let task_backend = backend.clone(); - spawn_handle.spawn( - "unpin-worker", - None, - async move { - log::info!(target: "db", "Starting worker task for unpinning."); - loop { - if let Some(message) = rx.next().await { - task_backend.unpin_block(&message); - } - } - } - .boxed(), - ); crate::client::Client::new( backend, executor, - unpin_worker_sender, + spawn_handle, genesis_block_builder, fork_blocks, bad_blocks, diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index 06e5b6aecd936..ba83f77b02238 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -22,6 +22,7 @@ use super::{ block_rules::{BlockRules, LookupResult as BlockLookupResult}, genesis::BuildGenesisBlock, }; +use futures::{FutureExt, StreamExt}; use log::{error, info, trace, warn}; use parking_lot::{Mutex, RwLock}; use prometheus_endpoint::Registry; @@ -177,11 +178,9 @@ where BlockImportOperation = as backend::Backend>::BlockImportOperation, >, { - let (tx, _rx) = sc_utils::mpsc::tracing_unbounded("unpin-worker-channel", 10_000); new_with_backend( backend, executor, - tx, genesis_block_builder, keystore, spawn_handle, @@ -225,7 +224,6 @@ impl Default for ClientConfig { pub fn new_with_backend( backend: Arc, executor: E, - unpin_worker_sender: TracingUnboundedSender, genesis_block_builder: G, keystore: Option, spawn_handle: Box, @@ -259,7 +257,7 @@ where Client::new( backend, call_executor, - unpin_worker_sender, + spawn_handle, genesis_block_builder, Default::default(), Default::default(), @@ -400,7 +398,7 @@ where pub fn new( backend: Arc, executor: E, - unpin_worker_sender: TracingUnboundedSender, + spawn_handle: Box, genesis_block_builder: G, fork_blocks: ForkBlocks, bad_blocks: BadBlocks, @@ -413,6 +411,7 @@ where Block, BlockImportOperation = >::BlockImportOperation, >, + B: 'static, { let info = backend.blockchain().info(); if info.finalized_state.is_none() { @@ -434,6 +433,22 @@ where backend.commit_operation(op)?; } + let (unpin_worker_sender, mut rx) = + tracing_unbounded::("unpin-worker-channel", 10_000); + let task_backend = backend.clone(); + spawn_handle.spawn( + "unpin-worker", + None, + async move { + log::info!(target: "db", "Starting worker task for unpinning."); + loop { + if let Some(message) = rx.next().await { + task_backend.unpin_block(&message); + } + } + } + .boxed(), + ); Ok(Client { backend, executor, diff --git a/test-utils/client/src/lib.rs b/test-utils/client/src/lib.rs index 3bd845e345af5..ff744b80cbff4 100644 --- a/test-utils/client/src/lib.rs +++ b/test-utils/client/src/lib.rs @@ -29,7 +29,6 @@ pub use sc_client_api::{ pub use sc_client_db::{self, Backend, BlocksPruning}; pub use sc_executor::{self, NativeElseWasmExecutor, WasmExecutionMethod}; pub use sc_service::{client, RpcHandlers}; -use sc_utils::mpsc::tracing_unbounded; pub use sp_consensus; pub use sp_keyring::{ ed25519::Keyring as Ed25519Keyring, sr25519::Keyring as Sr25519Keyring, AccountKeyring, @@ -42,7 +41,7 @@ use futures::{future::Future, stream::StreamExt}; use sc_client_api::BlockchainEvents; use sc_service::client::{ClientConfig, LocalCallExecutor}; use serde::Deserialize; -use sp_core::storage::ChildInfo; +use sp_core::{storage::ChildInfo, testing::TaskExecutor}; use sp_runtime::{codec::Encode, traits::Block as BlockT, OpaqueExtrinsic}; use std::{ collections::{HashMap, HashSet}, @@ -63,7 +62,7 @@ impl GenesisInit for () { } /// A builder for creating a test client instance. -pub struct TestClientBuilder { +pub struct TestClientBuilder { execution_strategies: ExecutionStrategies, genesis_init: G, /// The key is an unprefixed storage key, this only contains @@ -238,11 +237,12 @@ impl ) .expect("Creates genesis block builder"); - let (tx, _rx) = tracing_unbounded("unpin-worker", 10_000); + let spawn_handle = Box::new(TaskExecutor::new()); + let client = client::Client::new( self.backend.clone(), executor, - tx, + spawn_handle, genesis_block_builder, self.fork_blocks, self.bad_blocks, From 3ec854e31b1e91d66597949fcf949173ef925e0b Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 16 Jan 2023 16:17:09 +0100 Subject: [PATCH 17/37] Simplify cache --- client/db/src/lib.rs | 83 +++++++------------ client/finality-grandpa/src/until_imported.rs | 2 +- client/service/src/client/call_executor.rs | 5 -- 3 files changed, 33 insertions(+), 57 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 6b1abf2b974b9..abf98535255c4 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -478,18 +478,19 @@ fn cache_header( } } -struct PinnedBlockCacheEntry { +struct PinnedBlockCacheEntry { ref_count: u32, - pub value: Option, + pub justifications: Option>, + pub body: Option>>, } -impl Default for PinnedBlockCacheEntry { +impl Default for PinnedBlockCacheEntry { fn default() -> Self { - Self { ref_count: 0, value: None } + Self { ref_count: 0, justifications: None, body: None } } } -impl PinnedBlockCacheEntry { +impl PinnedBlockCacheEntry { pub fn decrease_ref(&mut self) { self.ref_count = self.ref_count.saturating_sub(1); } @@ -504,88 +505,68 @@ impl PinnedBlockCacheEntry { } struct PinnedBlockCache { - body_cache: RwLock>>>>, - justification_cache: - RwLock>>>, + cache: RwLock>>, } impl PinnedBlockCache { pub fn new() -> Self { - Self { - body_cache: LruCache::new(NonZeroUsize::new(PINNING_CACHE_SIZE).unwrap()).into(), - justification_cache: LruCache::new(NonZeroUsize::new(PINNING_CACHE_SIZE).unwrap()) - .into(), - } + Self { cache: LruCache::new(NonZeroUsize::new(PINNING_CACHE_SIZE).unwrap()).into() } } pub fn bump_ref(&self, hash: Block::Hash) { - let mut body_cache = self.body_cache.write(); - if body_cache.len() == PINNING_CACHE_SIZE { + let mut cache = self.cache.write(); + if cache.len() == PINNING_CACHE_SIZE { log::warn!(target: "db-pinning", "Maximum size of pinning cache reached. Removing items to make space. max_size = {}", PINNING_CACHE_SIZE); } - let entry = body_cache.get_or_insert_mut(hash, Default::default); - entry.increase_ref(); - - let mut justification_cache = self.justification_cache.write(); - let entry = justification_cache.get_or_insert_mut(hash, Default::default); + let entry = cache.get_or_insert_mut(hash, Default::default); entry.increase_ref(); } pub fn clear(&self) { - let mut body_cache = self.body_cache.write(); - body_cache.clear(); - let mut justification_cache = self.justification_cache.write(); - justification_cache.clear(); + let mut cache = self.cache.write(); + cache.clear(); } pub fn contains(&self, hash: &Block::Hash) -> bool { - let body_cache = self.body_cache.read(); - body_cache.contains(hash) + let cache = self.cache.read(); + cache.contains(hash) } pub fn insert_body(&self, hash: Block::Hash, extrinsics: Option>) { - let mut body_cache = self.body_cache.write(); - let mut entry = body_cache.get_or_insert_mut(hash, Default::default); - entry.value = Some(extrinsics); + let mut cache = self.cache.write(); + let mut entry = cache.get_or_insert_mut(hash, Default::default); + entry.body = Some(extrinsics); } pub fn insert_justifications(&self, hash: Block::Hash, justifications: Option) { - let mut justification_cache = self.justification_cache.write(); - let mut entry = justification_cache.get_or_insert_mut(hash, Default::default); - entry.value = Some(justifications); + let mut cache = self.cache.write(); + let mut entry = cache.get_or_insert_mut(hash, Default::default); + entry.justifications = Some(justifications); } pub fn remove(&self, hash: &Block::Hash) { - let mut body_cache = self.body_cache.write(); - if let Some(entry) = body_cache.peek_mut(hash) { - entry.decrease_ref(); - if entry.has_no_references() { - body_cache.pop(hash); - } - } - - let mut justification_cache = self.justification_cache.write(); - if let Some(entry) = justification_cache.peek_mut(hash) { + let mut cache = self.cache.write(); + if let Some(entry) = cache.peek_mut(hash) { entry.decrease_ref(); if entry.has_no_references() { - justification_cache.pop(hash); + cache.pop(hash); } } } - pub fn justification(&self, hash: &Block::Hash) -> Option> { - let justification_cache = self.justification_cache.read(); - if let Some(cache_entry) = justification_cache.peek(hash) { - return cache_entry.value.clone() + pub fn justifications(&self, hash: &Block::Hash) -> Option> { + let cache = self.cache.read(); + if let Some(cache_entry) = cache.peek(hash) { + return cache_entry.justifications.clone() }; None } pub fn body(&self, hash: &Block::Hash) -> Option>> { - let body_cache = self.body_cache.read(); - if let Some(cache_entry) = body_cache.peek(hash) { - return cache_entry.value.clone() + let cache = self.cache.read(); + if let Some(cache_entry) = cache.peek(hash) { + return cache_entry.body.clone() }; None } @@ -817,7 +798,7 @@ impl sc_client_api::blockchain::Backend for BlockchainDb ClientResult> { - if let Some(result) = self.pinned_blocks_cache.justification(&hash) { + if let Some(result) = self.pinned_blocks_cache.justifications(&hash) { return Ok(result) } diff --git a/client/finality-grandpa/src/until_imported.rs b/client/finality-grandpa/src/until_imported.rs index c03aebc6ab404..3bca77ae8393f 100644 --- a/client/finality-grandpa/src/until_imported.rs +++ b/client/finality-grandpa/src/until_imported.rs @@ -593,7 +593,7 @@ mod tests { fn import_header(&self, header: Header) { let hash = header.hash(); let number = *header.number(); - let (tx, rx) = tracing_unbounded("unpin-worker-channel", 10_000); + let (tx, _rx) = tracing_unbounded("unpin-worker-channel", 10_000); self.known_blocks.lock().insert(hash, number); self.sender .unbounded_send(BlockImportNotification::::new( diff --git a/client/service/src/client/call_executor.rs b/client/service/src/client/call_executor.rs index f452470cb591b..7ee19671dd688 100644 --- a/client/service/src/client/call_executor.rs +++ b/client/service/src/client/call_executor.rs @@ -361,7 +361,6 @@ mod tests { use super::*; use sc_client_api::in_mem; use sc_executor::{NativeElseWasmExecutor, WasmExecutionMethod}; - use sc_utils::mpsc::tracing_unbounded; use sp_core::{ testing::TaskExecutor, traits::{FetchRuntimeCode, WrappedRuntimeCode}, @@ -400,13 +399,11 @@ mod tests { ) .expect("Creates genesis block builder"); - let (unpin_worker_sender, _rx) = tracing_unbounded("unpin-worker", 10_000); // client is used for the convenience of creating and inserting the genesis block. let _client = crate::client::new_with_backend::<_, _, runtime::Block, _, runtime::RuntimeApi>( backend.clone(), executor.clone(), - unpin_worker_sender, genesis_block_builder, None, Box::new(TaskExecutor::new()), @@ -478,13 +475,11 @@ mod tests { ) .expect("Creates genesis block builder"); - let (unpin_worker_sender, _rx) = tracing_unbounded("unpin_worker", 10_000); // client is used for the convenience of creating and inserting the genesis block. let client = crate::client::new_with_backend::<_, _, runtime::Block, _, runtime::RuntimeApi>( backend.clone(), executor.clone(), - unpin_worker_sender, genesis_block_builder, None, Box::new(TaskExecutor::new()), From d5e2b1d942370e3772b65626eff1654fdf62092e Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 16 Jan 2023 16:47:56 +0100 Subject: [PATCH 18/37] Improve test, remove unwanted log --- client/db/src/lib.rs | 7 ++++++- client/state-db/src/lib.rs | 1 - 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index abf98535255c4..9c331ccf75cc4 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -4252,6 +4252,12 @@ pub(crate) mod tests { prev_hash = hash; } + let bc = backend.blockchain(); + + // Check that we can properly access values when there is reference count + // but no value. + assert_eq!(Some(vec![1.into()]), bc.body(blocks[1]).unwrap()); + // Block 1 gets pinned three times backend.pin_block(&blocks[1], 1).unwrap(); backend.pin_block(&blocks[1], 1).unwrap(); @@ -4265,7 +4271,6 @@ pub(crate) mod tests { } backend.commit_operation(op).unwrap(); - let bc = backend.blockchain(); // Block 0, 1, 2, 3 are pinned, so all values should be cached. // Block 4 is inside the pruning window, its value is in db. assert_eq!(Some(vec![0.into()]), bc.body(blocks[0]).unwrap()); diff --git a/client/state-db/src/lib.rs b/client/state-db/src/lib.rs index 68bab755f3254..b68dbd4b2c221 100644 --- a/client/state-db/src/lib.rs +++ b/client/state-db/src/lib.rs @@ -357,7 +357,6 @@ impl StateDbSync { if let Some(ref mut pruning) = self.pruning { pruning.note_canonical(hash, number, &mut commit)?; } - log::info!(target: "skunert", "Looking to canonicalize block {:?}", hash); self.prune(&mut commit)?; Ok(commit) } From 03051ca74125baeae277e00f86bd3195a7d53243 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 16 Jan 2023 17:14:24 +0100 Subject: [PATCH 19/37] Add tracing logs, remove expect for block number --- client/db/src/lib.rs | 5 +++++ client/service/src/client/client.rs | 18 ++++++++---------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 9c331ccf75cc4..a392139d81687 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -534,12 +534,14 @@ impl PinnedBlockCache { } pub fn insert_body(&self, hash: Block::Hash, extrinsics: Option>) { + log::trace!(target: "db-pin", "Caching body for hash {}", hash); let mut cache = self.cache.write(); let mut entry = cache.get_or_insert_mut(hash, Default::default); entry.body = Some(extrinsics); } pub fn insert_justifications(&self, hash: Block::Hash, justifications: Option) { + log::trace!(target: "db-pin", "Caching justification for hash {}", hash); let mut cache = self.cache.write(); let mut entry = cache.get_or_insert_mut(hash, Default::default); entry.justifications = Some(justifications); @@ -550,6 +552,7 @@ impl PinnedBlockCache { if let Some(entry) = cache.peek_mut(hash) { entry.decrease_ref(); if entry.has_no_references() { + log::trace!(target: "db-pin", "Removing pinned cache entries for hash {}", hash); cache.pop(hash); } } @@ -2589,6 +2592,8 @@ impl sc_client_api::backend::Backend for Backend { self.storage.state_db.pin(hash, number, hint).map_err(|_| { sp_blockchain::Error::UnknownBlock(format!("State already discarded for {:?}", hash)) })?; + + // Only increase reference count for this hash. Value is loaded once we prune. self.blockchain.bump_ref(*hash); Ok(()) } diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index ba83f77b02238..d245798075731 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -335,11 +335,10 @@ where self.backend.commit_operation(op)?; - if let Some(ref notification) = finality_notification { - if let Err(err) = self.backend.pin_block( - ¬ification.hash, - finality_num.expect("Should work").saturated_into::(), - ) { + if let (Some(ref notification), Some(number)) = (&finality_notification, finality_num) { + if let Err(err) = + self.backend.pin_block(¬ification.hash, number.saturated_into::()) + { error!( "Unable to pin block for finality notification. hash: {}, Error: {}", notification.hash, err @@ -347,11 +346,10 @@ where }; } - if let Some(ref notification) = import_notification { - if let Err(err) = self.backend.pin_block( - ¬ification.hash, - import_num.expect("Should work").saturated_into::(), - ) { + if let (Some(ref notification), Some(number)) = (&import_notification, import_num) { + if let Err(err) = + self.backend.pin_block(¬ification.hash, number.saturated_into::()) + { error!( "Unable to pin block for import notification. hash: {}, Error: {}", notification.hash, err From c4434bb60efff1e9514cefda375cff2e8c1caaf9 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 16 Jan 2023 17:33:34 +0100 Subject: [PATCH 20/37] Cleanup --- client/db/src/lib.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index a392139d81687..37be9ff23c49e 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -519,6 +519,7 @@ impl PinnedBlockCache { log::warn!(target: "db-pinning", "Maximum size of pinning cache reached. Removing items to make space. max_size = {}", PINNING_CACHE_SIZE); } + log::trace!(target: "db-pin", "Bumping cache refcount. hash = {}, num_entries = {}", hash, cache.len()); let entry = cache.get_or_insert_mut(hash, Default::default); entry.increase_ref(); } @@ -534,15 +535,15 @@ impl PinnedBlockCache { } pub fn insert_body(&self, hash: Block::Hash, extrinsics: Option>) { - log::trace!(target: "db-pin", "Caching body for hash {}", hash); let mut cache = self.cache.write(); + log::trace!(target: "db-pin", "Caching body. hash = {}, num_entries = {}", hash, cache.len()); let mut entry = cache.get_or_insert_mut(hash, Default::default); entry.body = Some(extrinsics); } pub fn insert_justifications(&self, hash: Block::Hash, justifications: Option) { - log::trace!(target: "db-pin", "Caching justification for hash {}", hash); let mut cache = self.cache.write(); + log::trace!(target: "db-pin", "Caching justification. hash = {}, num_entries = {}", hash, cache.len()); let mut entry = cache.get_or_insert_mut(hash, Default::default); entry.justifications = Some(justifications); } @@ -552,8 +553,8 @@ impl PinnedBlockCache { if let Some(entry) = cache.peek_mut(hash) { entry.decrease_ref(); if entry.has_no_references() { - log::trace!(target: "db-pin", "Removing pinned cache entries for hash {}", hash); cache.pop(hash); + log::trace!(target: "db-pin", "Removing pinned cache entry. hash = {}, num_entries = {}", hash, cache.len()); } } } @@ -2593,14 +2594,19 @@ impl sc_client_api::backend::Backend for Backend { sp_blockchain::Error::UnknownBlock(format!("State already discarded for {:?}", hash)) })?; - // Only increase reference count for this hash. Value is loaded once we prune. - self.blockchain.bump_ref(*hash); + if self.blocks_pruning != BlocksPruning::KeepAll { + // Only increase reference count for this hash. Value is loaded once we prune. + self.blockchain.bump_ref(*hash); + } Ok(()) } fn unpin_block(&self, hash: &::Hash) { self.storage.state_db.unpin(hash); - self.blockchain.unpin(hash); + + if self.blocks_pruning != BlocksPruning::KeepAll { + self.blockchain.unpin(hash); + } } } From 347c80a196464f83393685e95a9534e6e056edb7 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Tue, 17 Jan 2023 10:31:09 +0100 Subject: [PATCH 21/37] Add conversion method for unpin handle to Finalitynotification --- client/api/src/client.rs | 9 +++++++++ client/db/src/lib.rs | 8 ++++---- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/client/api/src/client.rs b/client/api/src/client.rs index 89f50d0fde3e0..aea6f57439718 100644 --- a/client/api/src/client.rs +++ b/client/api/src/client.rs @@ -291,6 +291,8 @@ impl Drop for UnpinHandleInner { } /// Keeps a specific block pinned while the handle is alive. +/// Once the last handle instance for a given block is dropped, the +/// block is unpinned in the backend. #[derive(Debug, Clone)] pub struct UnpinHandle(Arc>); @@ -406,6 +408,13 @@ impl FinalityNotification { _unpin_handle: UnpinHandle::new(hash, unpin_worker_sender), } } + + /// Consume this notification and extract the unpin handle. + /// + /// Note: Only use this if you want to keep the block pinned in the backend. + pub fn into_unpin_handle(self) -> UnpinHandle { + self._unpin_handle + } } impl BlockImportNotification { diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 37be9ff23c49e..e8896d2766b8a 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -519,9 +519,9 @@ impl PinnedBlockCache { log::warn!(target: "db-pinning", "Maximum size of pinning cache reached. Removing items to make space. max_size = {}", PINNING_CACHE_SIZE); } - log::trace!(target: "db-pin", "Bumping cache refcount. hash = {}, num_entries = {}", hash, cache.len()); let entry = cache.get_or_insert_mut(hash, Default::default); entry.increase_ref(); + log::trace!(target: "db-pin", "Bumped cache refcount. hash = {}, num_entries = {}", hash, cache.len()); } pub fn clear(&self) { @@ -536,16 +536,16 @@ impl PinnedBlockCache { pub fn insert_body(&self, hash: Block::Hash, extrinsics: Option>) { let mut cache = self.cache.write(); - log::trace!(target: "db-pin", "Caching body. hash = {}, num_entries = {}", hash, cache.len()); let mut entry = cache.get_or_insert_mut(hash, Default::default); entry.body = Some(extrinsics); + log::trace!(target: "db-pin", "Cached body. hash = {}, num_entries = {}", hash, cache.len()); } pub fn insert_justifications(&self, hash: Block::Hash, justifications: Option) { let mut cache = self.cache.write(); - log::trace!(target: "db-pin", "Caching justification. hash = {}, num_entries = {}", hash, cache.len()); let mut entry = cache.get_or_insert_mut(hash, Default::default); entry.justifications = Some(justifications); + log::trace!(target: "db-pin", "Cached justification. hash = {}, num_entries = {}", hash, cache.len()); } pub fn remove(&self, hash: &Block::Hash) { @@ -554,7 +554,7 @@ impl PinnedBlockCache { entry.decrease_ref(); if entry.has_no_references() { cache.pop(hash); - log::trace!(target: "db-pin", "Removing pinned cache entry. hash = {}, num_entries = {}", hash, cache.len()); + log::trace!(target: "db-pin", "Removed pinned cache entry. hash = {}, num_entries = {}", hash, cache.len()); } } } From 1198568a3d8ccb1049f1259cc1a0ebb6c88d3bca Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Tue, 17 Jan 2023 10:42:08 +0100 Subject: [PATCH 22/37] Revert unwanted changes --- Cargo.lock | 1 - client/state-db/src/lib.rs | 5 ++--- test-utils/client/Cargo.toml | 1 - 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 599f3a9f52533..a615569be9887 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10654,7 +10654,6 @@ dependencies = [ "sc-executor", "sc-offchain", "sc-service", - "sc-utils", "serde", "serde_json", "sp-blockchain", diff --git a/client/state-db/src/lib.rs b/client/state-db/src/lib.rs index b68dbd4b2c221..1befd6dff3bc8 100644 --- a/client/state-db/src/lib.rs +++ b/client/state-db/src/lib.rs @@ -188,9 +188,8 @@ impl fmt::Debug for StateDbError { write!(f, "Too many sibling blocks at #{number} inserted"), Self::BlockAlreadyExists => write!(f, "Block already exists"), Self::Metadata(message) => write!(f, "Invalid metadata: {}", message), - Self::BlockUnavailable => { - write!(f, "Trying to get a block record from db while it is not commit to db yet") - }, + Self::BlockUnavailable => + write!(f, "Trying to get a block record from db while it is not commit to db yet"), Self::BlockMissing => write!(f, "Block record is missing from the pruning window"), } } diff --git a/test-utils/client/Cargo.toml b/test-utils/client/Cargo.toml index 34a3796ea5c53..106ec21d79464 100644 --- a/test-utils/client/Cargo.toml +++ b/test-utils/client/Cargo.toml @@ -20,7 +20,6 @@ futures = "0.3.21" serde = "1.0.136" serde_json = "1.0.85" sc-client-api = { version = "4.0.0-dev", path = "../../client/api" } -sc-utils = { version = "4.0.0-dev", path = "../../client/utils" } sc-client-db = { version = "0.10.0-dev", default-features = false, features = [ "test-helpers", ], path = "../../client/db" } From 363f01a2998def3b9e0af650af01141445990262 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Tue, 17 Jan 2023 10:55:16 +0100 Subject: [PATCH 23/37] Improve naming --- client/db/src/lib.rs | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index e8896d2766b8a..ea1b05c4d7f9d 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -1455,14 +1455,20 @@ impl Backend { header: &Block::Header, last_finalized: Option, justification: Option, - seen_justifications: &mut HashMap, + current_transaction_justifications: &mut HashMap, ) -> ClientResult> { // TODO: ensure best chain contains this block. let number = *header.number(); self.ensure_sequential_finalization(header, last_finalized)?; let with_state = sc_client_api::Backend::have_state_at(self, hash, number); - self.note_finalized(transaction, header, hash, with_state, seen_justifications)?; + self.note_finalized( + transaction, + header, + hash, + with_state, + current_transaction_justifications, + )?; if let Some(justification) = justification { transaction.set_from_vec( @@ -1470,7 +1476,7 @@ impl Backend { &utils::number_and_hash_to_lookup_key(number, hash)?, Justifications::from(justification.clone()).encode(), ); - seen_justifications.insert(hash, justification); + current_transaction_justifications.insert(hash, justification); } Ok(MetaUpdate { hash, number, is_best: false, is_finalized: true, with_state }) } @@ -1537,7 +1543,8 @@ impl Backend { (meta.best_number, meta.finalized_hash, meta.finalized_number, meta.block_gap) }; - let mut seen_justifications: HashMap = HashMap::new(); + let mut current_transaction_justifications: HashMap = + HashMap::new(); for (block_hash, justification) in operation.finalized_blocks { let block_header = self.blockchain.expect_header(block_hash)?; meta_updates.push(self.finalize_block_with_transaction( @@ -1546,7 +1553,7 @@ impl Backend { &block_header, Some(last_finalized_hash), justification, - &mut seen_justifications, + &mut current_transaction_justifications, )?); last_finalized_hash = block_hash; last_finalized_num = *block_header.number(); @@ -1719,13 +1726,13 @@ impl Backend { if finalized { // TODO: ensure best chain contains this block. self.ensure_sequential_finalization(header, Some(last_finalized_hash))?; - let mut seen_justifications = HashMap::new(); + let mut current_transaction_justifications = HashMap::new(); self.note_finalized( &mut transaction, header, hash, operation.commit_state, - &mut seen_justifications, + &mut current_transaction_justifications, )?; } else { // canonicalize blocks which are old enough, regardless of finality. @@ -1859,7 +1866,7 @@ impl Backend { f_header: &Block::Header, f_hash: Block::Hash, with_state: bool, - seen_justifications: &mut HashMap, + current_transaction_justifications: &mut HashMap, ) -> ClientResult<()> { let f_num = *f_header.number(); @@ -1885,7 +1892,7 @@ impl Backend { } let new_displaced = self.blockchain.leaves.write().finalize_height(f_num); - self.prune_blocks(transaction, f_num, &new_displaced, seen_justifications)?; + self.prune_blocks(transaction, f_num, &new_displaced, current_transaction_justifications)?; Ok(()) } @@ -1895,7 +1902,7 @@ impl Backend { transaction: &mut Transaction, finalized_number: NumberFor, displaced: &FinalizationOutcome>, - seen_justifications: &mut HashMap, + current_transaction_justifications: &mut HashMap, ) -> ClientResult<()> { match self.blocks_pruning { BlocksPruning::KeepAll => {}, @@ -1911,7 +1918,9 @@ impl Backend { // If the block was finalized in this transaction, it will not be in the db // yet. - if let Some(justification) = seen_justifications.remove(&hash) { + if let Some(justification) = + current_transaction_justifications.remove(&hash) + { self.blockchain.insert_justifications_if_pinned(hash, justification); } else { self.blockchain.insert_persisted_justifications_if_pinned(hash)?; @@ -2194,14 +2203,14 @@ impl sc_client_api::backend::Backend for Backend { let mut transaction = Transaction::new(); let header = self.blockchain.expect_header(hash)?; - let mut seen_justifications = HashMap::new(); + let mut current_transaction_justifications = HashMap::new(); let m = self.finalize_block_with_transaction( &mut transaction, hash, &header, None, justification, - &mut seen_justifications, + &mut current_transaction_justifications, )?; self.storage.db.commit(transaction)?; From b22055306d04d614f8d1be12712b62d294be90b2 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Tue, 17 Jan 2023 11:29:42 +0100 Subject: [PATCH 24/37] Make clippy happy --- client/service/src/client/client.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index d245798075731..27c29d2c03690 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -300,14 +300,12 @@ where let ClientImportOperation { mut op, notify_imported, notify_finalized } = op; - let finality_num = - notify_finalized.as_ref().map(|summary| summary.header.number().clone()); + let finality_num = notify_finalized.as_ref().map(|summary| *summary.header.number()); let finality_notification = notify_finalized.map(|summary| { FinalityNotification::from_summary(summary, self.unpin_worker_sender.clone()) }); - let import_num = - notify_imported.as_ref().map(|summary| summary.header.number().clone()); + let import_num = notify_imported.as_ref().map(|summary| *summary.header.number()); let (import_notification, storage_changes) = match notify_imported { Some(mut summary) => { let storage_changes = summary.storage_changes.take(); From 10dfdf78ba872db8ec0ce3289bd14a46dca017a8 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Tue, 17 Jan 2023 15:55:29 +0100 Subject: [PATCH 25/37] Fix docs Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> --- client/db/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index ea1b05c4d7f9d..65a06fe8b4e36 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -633,7 +633,7 @@ impl BlockchainDb { /// Load a justification into the cache of pinned items. /// Reference count of the item will not be increased. Use this - /// to load values for items items into the cache which have already been pinned. + /// to load values for items into the cache which have already been pinned. fn insert_justifications_if_pinned(&self, hash: Block::Hash, justification: Justification) { if !self.pinned_blocks_cache.contains(&hash) { return @@ -645,7 +645,7 @@ impl BlockchainDb { /// Load a justification from the db into the cache of pinned items. /// Reference count of the item will not be increased. Use this - /// to load values for items items into the cache which have already been pinned. + /// to load values for items into the cache which have already been pinned. fn insert_persisted_justifications_if_pinned(&self, hash: Block::Hash) -> ClientResult<()> { if !self.pinned_blocks_cache.contains(&hash) { return Ok(()) From c4608dad8e35eecd43d2fb99c69aa7a447741f27 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Tue, 17 Jan 2023 15:52:58 +0100 Subject: [PATCH 26/37] Use `NumberFor` instead of u64 in API --- client/api/src/backend.rs | 2 +- client/api/src/in_mem.rs | 6 +++++- client/db/src/lib.rs | 18 ++++++++++++++---- client/service/src/client/client.rs | 8 ++------ 4 files changed, 22 insertions(+), 12 deletions(-) diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs index 62eac1b45cf64..bce88abb01ee5 100644 --- a/client/api/src/backend.rs +++ b/client/api/src/backend.rs @@ -517,7 +517,7 @@ pub trait Backend: AuxStore + Send + Sync { /// Pin the block to keep bodies, justification and state available after pruning. /// Number of pins are reference counted. Users need to make sure to perform /// one call to `unpin_block` per call to `pin_block`. - fn pin_block(&self, hash: &Block::Hash, number: u64) -> sp_blockchain::Result<()>; + fn pin_block(&self, hash: &Block::Hash, number: NumberFor) -> sp_blockchain::Result<()>; /// Unpin the block to allow pruning. fn unpin_block(&self, hash: &Block::Hash); diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs index 82a0868f6c72a..09d86a1a6579e 100644 --- a/client/api/src/in_mem.rs +++ b/client/api/src/in_mem.rs @@ -789,7 +789,11 @@ where false } - fn pin_block(&self, _: &::Hash, _: u64) -> blockchain::Result<()> { + fn pin_block( + &self, + _: &::Hash, + _: NumberFor, + ) -> blockchain::Result<()> { Ok(()) } diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 65a06fe8b4e36..4e2fb9da68741 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -2588,7 +2588,11 @@ impl sc_client_api::backend::Backend for Backend { ) } - fn pin_block(&self, hash: &::Hash, number: u64) -> sp_blockchain::Result<()> { + fn pin_block( + &self, + hash: &::Hash, + number: NumberFor, + ) -> sp_blockchain::Result<()> { let hint = || { let header_metadata = self.blockchain.header_metadata(*hash); header_metadata @@ -2599,9 +2603,15 @@ impl sc_client_api::backend::Backend for Backend { }) .unwrap_or(false) }; - self.storage.state_db.pin(hash, number, hint).map_err(|_| { - sp_blockchain::Error::UnknownBlock(format!("State already discarded for {:?}", hash)) - })?; + self.storage + .state_db + .pin(hash, number.saturated_into::(), hint) + .map_err(|_| { + sp_blockchain::Error::UnknownBlock(format!( + "State already discarded for {:?}", + hash + )) + })?; if self.blocks_pruning != BlocksPruning::KeepAll { // Only increase reference count for this hash. Value is loaded once we prune. diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index 27c29d2c03690..abdd230e05538 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -334,9 +334,7 @@ where self.backend.commit_operation(op)?; if let (Some(ref notification), Some(number)) = (&finality_notification, finality_num) { - if let Err(err) = - self.backend.pin_block(¬ification.hash, number.saturated_into::()) - { + if let Err(err) = self.backend.pin_block(¬ification.hash, number) { error!( "Unable to pin block for finality notification. hash: {}, Error: {}", notification.hash, err @@ -345,9 +343,7 @@ where } if let (Some(ref notification), Some(number)) = (&import_notification, import_num) { - if let Err(err) = - self.backend.pin_block(¬ification.hash, number.saturated_into::()) - { + if let Err(err) = self.backend.pin_block(¬ification.hash, number) { error!( "Unable to pin block for import notification. hash: {}, Error: {}", notification.hash, err From 68cc76f16804e8a1d3dec16d2bc0826cd76f9103 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Tue, 17 Jan 2023 16:14:16 +0100 Subject: [PATCH 27/37] Hand over weak reference to unpin worker task --- client/service/src/client/client.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index abdd230e05538..362c91c75bbcf 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -427,15 +427,19 @@ where let (unpin_worker_sender, mut rx) = tracing_unbounded::("unpin-worker-channel", 10_000); - let task_backend = backend.clone(); + let task_backend = Arc::downgrade(&backend); spawn_handle.spawn( "unpin-worker", None, async move { - log::info!(target: "db", "Starting worker task for unpinning."); loop { if let Some(message) = rx.next().await { - task_backend.unpin_block(&message); + if let Some(backend) = task_backend.upgrade() { + backend.unpin_block(&message); + } else { + log::warn!(target: "db", "Terminating unpin-worker, backend reference was dropped."); + return + } } } } From fe2d4cc6659df8d8d7490eae37d72edfca8a62e0 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Tue, 17 Jan 2023 18:02:38 +0100 Subject: [PATCH 28/37] Unwanted --- Cargo.lock | 67 +++++++++++++------------------------------- client/db/Cargo.toml | 2 +- 2 files changed, 20 insertions(+), 49 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c33bc039628d7..8b7d2f35615ec 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -137,17 +137,6 @@ dependencies = [ "version_check", ] -[[package]] -name = "ahash" -version = "0.8.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bf6ccdb167abbf410dcb915cabd428929d7f6a04980b54a11f26a39f1c7f7107" -dependencies = [ - "cfg-if", - "once_cell", - "version_check", -] - [[package]] name = "aho-corasick" version = "0.7.20" @@ -1847,7 +1836,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7c24f403d068ad0b359e577a77f92392118be3f3c927538f2bb544a5ecd828c6" dependencies = [ "curve25519-dalek 3.2.0", - "hashbrown 0.12.3", + "hashbrown", "hex", "rand_core 0.6.4", "sha2 0.9.9", @@ -2817,16 +2806,7 @@ version = "0.12.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8a9ee70c43aaf417c914396645a0fa852624801b24ebb7ae78fe8272889ac888" dependencies = [ - "ahash 0.7.6", -] - -[[package]] -name = "hashbrown" -version = "0.13.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "33ff8ae62cd3a9102e5637afc8452c55acf3844001bd5374e0b0bd7b6616c038" -dependencies = [ - "ahash 0.8.2", + "ahash", ] [[package]] @@ -3142,7 +3122,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1885e79c1fc4b10f0e172c475f458b7f7b93061064d98c3293e98c5ba0c8b399" dependencies = [ "autocfg", - "hashbrown 0.12.3", + "hashbrown", "serde", ] @@ -3716,7 +3696,7 @@ dependencies = [ "libp2p-core", "libp2p-swarm", "log", - "lru 0.8.1", + "lru", "prost", "prost-build", "prost-codec", @@ -4192,16 +4172,7 @@ version = "0.8.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b6e8aaa3f231bb4bd57b84b2d5dc3ae7f350265df8aa96492e0bc394a1571909" dependencies = [ - "hashbrown 0.12.3", -] - -[[package]] -name = "lru" -version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "71e7d46de488603ffdd5f30afbc64fbba2378214a2c3a2fb83abf3d33126df17" -dependencies = [ - "hashbrown 0.13.1", + "hashbrown", ] [[package]] @@ -4339,7 +4310,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5e0c7cba9ce19ac7ffd2053ac9f49843bbd3f4318feedfd74e85c19d5fb0ba66" dependencies = [ "hash-db", - "hashbrown 0.12.3", + "hashbrown", ] [[package]] @@ -5114,7 +5085,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "21158b2c33aa6d4561f1c0a6ea283ca92bc54802a93b263e910746d679a7eb53" dependencies = [ "crc32fast", - "hashbrown 0.12.3", + "hashbrown", "indexmap", "memchr", ] @@ -8013,7 +7984,7 @@ dependencies = [ "kvdb-rocksdb", "linked-hash-map", "log", - "lru 0.9.0", + "lru", "parity-db", "parity-scale-codec", "parking_lot 0.12.1", @@ -8283,7 +8254,7 @@ dependencies = [ "array-bytes", "criterion", "env_logger", - "lru 0.8.1", + "lru", "num_cpus", "parity-scale-codec", "parking_lot 0.12.1", @@ -8364,7 +8335,7 @@ dependencies = [ name = "sc-finality-grandpa" version = "0.10.0-dev" dependencies = [ - "ahash 0.7.6", + "ahash", "array-bytes", "assert_matches", "async-trait", @@ -8476,7 +8447,7 @@ dependencies = [ "ip_network", "libp2p", "log", - "lru 0.8.1", + "lru", "parity-scale-codec", "parking_lot 0.12.1", "pin-project", @@ -8564,12 +8535,12 @@ dependencies = [ name = "sc-network-gossip" version = "0.10.0-dev" dependencies = [ - "ahash 0.7.6", + "ahash", "futures", "futures-timer", "libp2p", "log", - "lru 0.8.1", + "lru", "quickcheck", "sc-network-common", "sc-peerset", @@ -8610,7 +8581,7 @@ dependencies = [ "futures", "libp2p", "log", - "lru 0.8.1", + "lru", "mockall", "parity-scale-codec", "prost", @@ -9708,7 +9679,7 @@ version = "4.0.0-dev" dependencies = [ "futures", "log", - "lru 0.8.1", + "lru", "parity-scale-codec", "parking_lot 0.12.1", "sp-api", @@ -10305,13 +10276,13 @@ dependencies = [ name = "sp-trie" version = "7.0.0" dependencies = [ - "ahash 0.7.6", + "ahash", "array-bytes", "criterion", "hash-db", - "hashbrown 0.12.3", + "hashbrown", "lazy_static", - "lru 0.8.1", + "lru", "memory-db", "nohash-hasher", "parity-scale-codec", @@ -11309,7 +11280,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "004e1e8f92535694b4cb1444dc5a8073ecf0815e3357f729638b9f8fc4062908" dependencies = [ "hash-db", - "hashbrown 0.12.3", + "hashbrown", "log", "rustc-hex", "smallvec", diff --git a/client/db/Cargo.toml b/client/db/Cargo.toml index 9575464d830cb..90764e1777391 100644 --- a/client/db/Cargo.toml +++ b/client/db/Cargo.toml @@ -22,7 +22,7 @@ kvdb-memorydb = "0.13.0" kvdb-rocksdb = { version = "0.17.0", optional = true } linked-hash-map = "0.5.4" log = "0.4.17" -lru = "0.9.0" +lru = "0.8.1" parity-db = "0.4.2" parking_lot = "0.12.1" sc-client-api = { version = "4.0.0-dev", path = "../api" } From f5d6b08a8ac3a655d4b34a6a5c14f0796e17aae5 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Tue, 17 Jan 2023 18:09:20 +0100 Subject: [PATCH 29/37] &Hash -> Hash --- client/api/src/backend.rs | 4 +-- client/api/src/in_mem.rs | 8 ++--- client/db/src/lib.rs | 56 ++++++++++++++--------------- client/service/src/client/client.rs | 6 ++-- 4 files changed, 35 insertions(+), 39 deletions(-) diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs index bce88abb01ee5..eb6b77d6f9517 100644 --- a/client/api/src/backend.rs +++ b/client/api/src/backend.rs @@ -517,10 +517,10 @@ pub trait Backend: AuxStore + Send + Sync { /// Pin the block to keep bodies, justification and state available after pruning. /// Number of pins are reference counted. Users need to make sure to perform /// one call to `unpin_block` per call to `pin_block`. - fn pin_block(&self, hash: &Block::Hash, number: NumberFor) -> sp_blockchain::Result<()>; + fn pin_block(&self, hash: Block::Hash, number: NumberFor) -> sp_blockchain::Result<()>; /// Unpin the block to allow pruning. - fn unpin_block(&self, hash: &Block::Hash); + fn unpin_block(&self, hash: Block::Hash); /// Returns true if state for given block is available. fn have_state_at(&self, hash: Block::Hash, _number: NumberFor) -> bool { diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs index 09d86a1a6579e..ab8607e805bfe 100644 --- a/client/api/src/in_mem.rs +++ b/client/api/src/in_mem.rs @@ -789,15 +789,11 @@ where false } - fn pin_block( - &self, - _: &::Hash, - _: NumberFor, - ) -> blockchain::Result<()> { + fn pin_block(&self, _: ::Hash, _: NumberFor) -> blockchain::Result<()> { Ok(()) } - fn unpin_block(&self, _: &::Hash) {} + fn unpin_block(&self, _: ::Hash) {} } impl backend::LocalBackend for Backend where Block::Hash: Ord {} diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 4e2fb9da68741..f84f743e32ab9 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -529,9 +529,9 @@ impl PinnedBlockCache { cache.clear(); } - pub fn contains(&self, hash: &Block::Hash) -> bool { + pub fn contains(&self, hash: Block::Hash) -> bool { let cache = self.cache.read(); - cache.contains(hash) + cache.contains(&hash) } pub fn insert_body(&self, hash: Block::Hash, extrinsics: Option>) { @@ -548,12 +548,12 @@ impl PinnedBlockCache { log::trace!(target: "db-pin", "Cached justification. hash = {}, num_entries = {}", hash, cache.len()); } - pub fn remove(&self, hash: &Block::Hash) { + pub fn remove(&self, hash: Block::Hash) { let mut cache = self.cache.write(); - if let Some(entry) = cache.peek_mut(hash) { + if let Some(entry) = cache.peek_mut(&hash) { entry.decrease_ref(); if entry.has_no_references() { - cache.pop(hash); + cache.pop(&hash); log::trace!(target: "db-pin", "Removed pinned cache entry. hash = {}, num_entries = {}", hash, cache.len()); } } @@ -635,7 +635,7 @@ impl BlockchainDb { /// Reference count of the item will not be increased. Use this /// to load values for items into the cache which have already been pinned. fn insert_justifications_if_pinned(&self, hash: Block::Hash, justification: Justification) { - if !self.pinned_blocks_cache.contains(&hash) { + if !self.pinned_blocks_cache.contains(hash) { return } @@ -647,7 +647,7 @@ impl BlockchainDb { /// Reference count of the item will not be increased. Use this /// to load values for items into the cache which have already been pinned. fn insert_persisted_justifications_if_pinned(&self, hash: Block::Hash) -> ClientResult<()> { - if !self.pinned_blocks_cache.contains(&hash) { + if !self.pinned_blocks_cache.contains(hash) { return Ok(()) } let justifications = self.justifications(hash)?; @@ -659,7 +659,7 @@ impl BlockchainDb { /// Reference count of the item will not be increased. Use this /// to load values for items items into the cache which have already been pinned. fn insert_persisted_body_if_pinned(&self, hash: Block::Hash) -> ClientResult<()> { - if !self.pinned_blocks_cache.contains(&hash) { + if !self.pinned_blocks_cache.contains(hash) { return Ok(()) } @@ -674,7 +674,7 @@ impl BlockchainDb { } /// Decrease reference count for pinned item and remove if reference count is 0. - fn unpin(&self, hash: &Block::Hash) { + fn unpin(&self, hash: Block::Hash) { self.pinned_blocks_cache.remove(hash); } } @@ -2590,11 +2590,11 @@ impl sc_client_api::backend::Backend for Backend { fn pin_block( &self, - hash: &::Hash, + hash: ::Hash, number: NumberFor, ) -> sp_blockchain::Result<()> { let hint = || { - let header_metadata = self.blockchain.header_metadata(*hash); + let header_metadata = self.blockchain.header_metadata(hash); header_metadata .map(|hdr| { sc_state_db::NodeDb::get(self.storage.as_ref(), hdr.state_root.as_ref()) @@ -2605,7 +2605,7 @@ impl sc_client_api::backend::Backend for Backend { }; self.storage .state_db - .pin(hash, number.saturated_into::(), hint) + .pin(&hash, number.saturated_into::(), hint) .map_err(|_| { sp_blockchain::Error::UnknownBlock(format!( "State already discarded for {:?}", @@ -2615,13 +2615,13 @@ impl sc_client_api::backend::Backend for Backend { if self.blocks_pruning != BlocksPruning::KeepAll { // Only increase reference count for this hash. Value is loaded once we prune. - self.blockchain.bump_ref(*hash); + self.blockchain.bump_ref(hash); } Ok(()) } - fn unpin_block(&self, hash: &::Hash) { - self.storage.state_db.unpin(hash); + fn unpin_block(&self, hash: ::Hash) { + self.storage.state_db.unpin(&hash); if self.blocks_pruning != BlocksPruning::KeepAll { self.blockchain.unpin(hash); @@ -4277,7 +4277,7 @@ pub(crate) mod tests { .unwrap(); blocks.push(hash); // Avoid block pruning. - backend.pin_block(&blocks[i as usize], i).unwrap(); + backend.pin_block(blocks[i as usize], i).unwrap(); prev_hash = hash; } @@ -4289,8 +4289,8 @@ pub(crate) mod tests { assert_eq!(Some(vec![1.into()]), bc.body(blocks[1]).unwrap()); // Block 1 gets pinned three times - backend.pin_block(&blocks[1], 1).unwrap(); - backend.pin_block(&blocks[1], 1).unwrap(); + backend.pin_block(blocks[1], 1).unwrap(); + backend.pin_block(blocks[1], 1).unwrap(); // Finalize all blocks. This will trigger pruning. let mut op = backend.begin_operation().unwrap(); @@ -4331,7 +4331,7 @@ pub(crate) mod tests { // Unpin all blocks. Values should be removed from cache. for block in &blocks { - backend.unpin_block(&block); + backend.unpin_block(*block); } assert!(bc.body(blocks[0]).unwrap().is_none()); @@ -4344,10 +4344,10 @@ pub(crate) mod tests { assert!(bc.justifications(blocks[3]).unwrap().is_none()); // After these unpins, block 1 should also be removed - backend.unpin_block(&blocks[1]); + backend.unpin_block(blocks[1]); assert!(bc.body(blocks[1]).unwrap().is_some()); assert!(bc.justifications(blocks[1]).unwrap().is_some()); - backend.unpin_block(&blocks[1]); + backend.unpin_block(blocks[1]); assert!(bc.body(blocks[1]).unwrap().is_none()); assert!(bc.justifications(blocks[1]).unwrap().is_none()); @@ -4365,7 +4365,7 @@ pub(crate) mod tests { .unwrap(); blocks.push(hash); - backend.pin_block(&blocks[4], 4).unwrap(); + backend.pin_block(blocks[4], 4).unwrap(); // Mark block 5 as finalized. let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, blocks[5]).unwrap(); @@ -4384,7 +4384,7 @@ pub(crate) mod tests { ); assert_eq!(Some(vec![5.into()]), bc.body(blocks[5]).unwrap()); - backend.unpin_block(&blocks[4]); + backend.unpin_block(blocks[4]); assert!(bc.body(blocks[4]).unwrap().is_none()); assert!(bc.justifications(blocks[4]).unwrap().is_none()); @@ -4397,7 +4397,7 @@ pub(crate) mod tests { blocks.push(hash); // Pin block 5 so it gets loaded into the cache on prune - backend.pin_block(&blocks[5], 5).unwrap(); + backend.pin_block(blocks[5], 5).unwrap(); // Finalize block 6 so block 5 gets pruned. Since it is pinned both justifications should be // in memory. @@ -4434,7 +4434,7 @@ pub(crate) mod tests { blocks.push(hash); // Avoid block pruning. - backend.pin_block(&blocks[i as usize], i).unwrap(); + backend.pin_block(blocks[i as usize], i).unwrap(); prev_hash = hash; } @@ -4458,7 +4458,7 @@ pub(crate) mod tests { .unwrap(); // Do not prune the fork hash. - backend.pin_block(&fork_hash_3, 3).unwrap(); + backend.pin_block(fork_hash_3, 3).unwrap(); let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, blocks[4]).unwrap(); @@ -4484,7 +4484,7 @@ pub(crate) mod tests { // Unpin all blocks, except the forked one. for block in &blocks { - backend.unpin_block(&block); + backend.unpin_block(*block); } assert!(bc.body(blocks[0]).unwrap().is_none()); assert!(bc.body(blocks[1]).unwrap().is_none()); @@ -4492,7 +4492,7 @@ pub(crate) mod tests { assert!(bc.body(blocks[3]).unwrap().is_none()); assert!(bc.body(fork_hash_3).unwrap().is_some()); - backend.unpin_block(&fork_hash_3); + backend.unpin_block(fork_hash_3); assert!(bc.body(fork_hash_3).unwrap().is_none()); } } diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index 362c91c75bbcf..3e2454b87f1f2 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -334,7 +334,7 @@ where self.backend.commit_operation(op)?; if let (Some(ref notification), Some(number)) = (&finality_notification, finality_num) { - if let Err(err) = self.backend.pin_block(¬ification.hash, number) { + if let Err(err) = self.backend.pin_block(notification.hash, number) { error!( "Unable to pin block for finality notification. hash: {}, Error: {}", notification.hash, err @@ -343,7 +343,7 @@ where } if let (Some(ref notification), Some(number)) = (&import_notification, import_num) { - if let Err(err) = self.backend.pin_block(¬ification.hash, number) { + if let Err(err) = self.backend.pin_block(notification.hash, number) { error!( "Unable to pin block for import notification. hash: {}, Error: {}", notification.hash, err @@ -435,7 +435,7 @@ where loop { if let Some(message) = rx.next().await { if let Some(backend) = task_backend.upgrade() { - backend.unpin_block(&message); + backend.unpin_block(message); } else { log::warn!(target: "db", "Terminating unpin-worker, backend reference was dropped."); return From 946e06aa85eb86bb553bed610d016fc115457ccb Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Wed, 18 Jan 2023 09:19:10 +0100 Subject: [PATCH 30/37] Remove number from interface, rename `_unpin_handle`, LOG_TARGET --- client/api/src/backend.rs | 2 +- client/api/src/client.rs | 14 +++--- client/api/src/in_mem.rs | 2 +- client/db/src/lib.rs | 76 +++++++++++++++++++---------- client/service/src/client/client.rs | 10 ++-- 5 files changed, 63 insertions(+), 41 deletions(-) diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs index eb6b77d6f9517..180e8b49ba6b0 100644 --- a/client/api/src/backend.rs +++ b/client/api/src/backend.rs @@ -517,7 +517,7 @@ pub trait Backend: AuxStore + Send + Sync { /// Pin the block to keep bodies, justification and state available after pruning. /// Number of pins are reference counted. Users need to make sure to perform /// one call to `unpin_block` per call to `pin_block`. - fn pin_block(&self, hash: Block::Hash, number: NumberFor) -> sp_blockchain::Result<()>; + fn pin_block(&self, hash: Block::Hash) -> sp_blockchain::Result<()>; /// Unpin the block to allow pruning. fn unpin_block(&self, hash: Block::Hash); diff --git a/client/api/src/client.rs b/client/api/src/client.rs index aea6f57439718..cc5cfa0bb2cc3 100644 --- a/client/api/src/client.rs +++ b/client/api/src/client.rs @@ -327,7 +327,7 @@ pub struct BlockImportNotification { /// If `None`, there was no re-org while importing. pub tree_route: Option>>, /// Handle to unpin the block this notification is for - _unpin_handle: UnpinHandle, + unpin_handle: UnpinHandle, } impl BlockImportNotification { @@ -346,7 +346,7 @@ impl BlockImportNotification { header, is_new_best, tree_route, - _unpin_handle: UnpinHandle::new(hash, unpin_worker_sender), + unpin_handle: UnpinHandle::new(hash, unpin_worker_sender), } } @@ -354,7 +354,7 @@ impl BlockImportNotification { /// /// Note: Only use this if you want to keep the block pinned in the backend. pub fn into_unpin_handle(self) -> UnpinHandle { - self._unpin_handle + self.unpin_handle } } @@ -372,7 +372,7 @@ pub struct FinalityNotification { /// Stale branches heads. pub stale_heads: Arc<[Block::Hash]>, /// Handle to unpin the block this notification is for - _unpin_handle: UnpinHandle, + unpin_handle: UnpinHandle, } impl TryFrom> for ChainEvent { @@ -405,7 +405,7 @@ impl FinalityNotification { header: summary.header, tree_route: Arc::from(summary.finalized), stale_heads: Arc::from(summary.stale_heads), - _unpin_handle: UnpinHandle::new(hash, unpin_worker_sender), + unpin_handle: UnpinHandle::new(hash, unpin_worker_sender), } } @@ -413,7 +413,7 @@ impl FinalityNotification { /// /// Note: Only use this if you want to keep the block pinned in the backend. pub fn into_unpin_handle(self) -> UnpinHandle { - self._unpin_handle + self.unpin_handle } } @@ -430,7 +430,7 @@ impl BlockImportNotification { header: summary.header, is_new_best: summary.is_new_best, tree_route: summary.tree_route.map(Arc::new), - _unpin_handle: UnpinHandle::new(hash, unpin_worker_sender), + unpin_handle: UnpinHandle::new(hash, unpin_worker_sender), } } } diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs index ab8607e805bfe..5e82757e7d9cd 100644 --- a/client/api/src/in_mem.rs +++ b/client/api/src/in_mem.rs @@ -789,7 +789,7 @@ where false } - fn pin_block(&self, _: ::Hash, _: NumberFor) -> blockchain::Result<()> { + fn pin_block(&self, _: ::Hash) -> blockchain::Result<()> { Ok(()) } diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index f84f743e32ab9..a9d7433ebff2c 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -98,6 +98,7 @@ pub use sp_database::Database; pub use bench::BenchmarkingState; const CACHE_HEADERS: usize = 8; +const LOG_TARGET: &str = "db-pin"; /// DB-backed patricia trie state, transaction type is an overlay of changes to commit. pub type DbState = @@ -516,12 +517,17 @@ impl PinnedBlockCache { pub fn bump_ref(&self, hash: Block::Hash) { let mut cache = self.cache.write(); if cache.len() == PINNING_CACHE_SIZE { - log::warn!(target: "db-pinning", "Maximum size of pinning cache reached. Removing items to make space. max_size = {}", PINNING_CACHE_SIZE); + log::warn!(target: LOG_TARGET, "Maximum size of pinning cache reached. Removing items to make space. max_size = {}", PINNING_CACHE_SIZE); } let entry = cache.get_or_insert_mut(hash, Default::default); entry.increase_ref(); - log::trace!(target: "db-pin", "Bumped cache refcount. hash = {}, num_entries = {}", hash, cache.len()); + log::trace!( + target: LOG_TARGET, + "Bumped cache refcount. hash = {}, num_entries = {}", + hash, + cache.len() + ); } pub fn clear(&self) { @@ -538,14 +544,24 @@ impl PinnedBlockCache { let mut cache = self.cache.write(); let mut entry = cache.get_or_insert_mut(hash, Default::default); entry.body = Some(extrinsics); - log::trace!(target: "db-pin", "Cached body. hash = {}, num_entries = {}", hash, cache.len()); + log::trace!( + target: LOG_TARGET, + "Cached body. hash = {}, num_entries = {}", + hash, + cache.len() + ); } pub fn insert_justifications(&self, hash: Block::Hash, justifications: Option) { let mut cache = self.cache.write(); let mut entry = cache.get_or_insert_mut(hash, Default::default); entry.justifications = Some(justifications); - log::trace!(target: "db-pin", "Cached justification. hash = {}, num_entries = {}", hash, cache.len()); + log::trace!( + target: LOG_TARGET, + "Cached justification. hash = {}, num_entries = {}", + hash, + cache.len() + ); } pub fn remove(&self, hash: Block::Hash) { @@ -554,7 +570,12 @@ impl PinnedBlockCache { entry.decrease_ref(); if entry.has_no_references() { cache.pop(&hash); - log::trace!(target: "db-pin", "Removed pinned cache entry. hash = {}, num_entries = {}", hash, cache.len()); + log::trace!( + target: LOG_TARGET, + "Removed pinned cache entry. hash = {}, num_entries = {}", + hash, + cache.len() + ); } } } @@ -2588,11 +2609,7 @@ impl sc_client_api::backend::Backend for Backend { ) } - fn pin_block( - &self, - hash: ::Hash, - number: NumberFor, - ) -> sp_blockchain::Result<()> { + fn pin_block(&self, hash: ::Hash) -> sp_blockchain::Result<()> { let hint = || { let header_metadata = self.blockchain.header_metadata(hash); header_metadata @@ -2603,15 +2620,22 @@ impl sc_client_api::backend::Backend for Backend { }) .unwrap_or(false) }; - self.storage - .state_db - .pin(&hash, number.saturated_into::(), hint) - .map_err(|_| { - sp_blockchain::Error::UnknownBlock(format!( - "State already discarded for {:?}", - hash - )) - })?; + + if let Some(number) = self.blockchain.number(hash)? { + self.storage.state_db.pin(&hash, number.saturated_into::(), hint).map_err( + |_| { + sp_blockchain::Error::UnknownBlock(format!( + "State already discarded for {:?}", + hash + )) + }, + )?; + } else { + return Err(ClientError::UnknownBlock(format!( + "Can not pin block with hash {}. Block not found.", + hash + ))) + } if self.blocks_pruning != BlocksPruning::KeepAll { // Only increase reference count for this hash. Value is loaded once we prune. @@ -4277,7 +4301,7 @@ pub(crate) mod tests { .unwrap(); blocks.push(hash); // Avoid block pruning. - backend.pin_block(blocks[i as usize], i).unwrap(); + backend.pin_block(blocks[i as usize]).unwrap(); prev_hash = hash; } @@ -4289,8 +4313,8 @@ pub(crate) mod tests { assert_eq!(Some(vec![1.into()]), bc.body(blocks[1]).unwrap()); // Block 1 gets pinned three times - backend.pin_block(blocks[1], 1).unwrap(); - backend.pin_block(blocks[1], 1).unwrap(); + backend.pin_block(blocks[1]).unwrap(); + backend.pin_block(blocks[1]).unwrap(); // Finalize all blocks. This will trigger pruning. let mut op = backend.begin_operation().unwrap(); @@ -4365,7 +4389,7 @@ pub(crate) mod tests { .unwrap(); blocks.push(hash); - backend.pin_block(blocks[4], 4).unwrap(); + backend.pin_block(blocks[4]).unwrap(); // Mark block 5 as finalized. let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, blocks[5]).unwrap(); @@ -4397,7 +4421,7 @@ pub(crate) mod tests { blocks.push(hash); // Pin block 5 so it gets loaded into the cache on prune - backend.pin_block(blocks[5], 5).unwrap(); + backend.pin_block(blocks[5]).unwrap(); // Finalize block 6 so block 5 gets pruned. Since it is pinned both justifications should be // in memory. @@ -4434,7 +4458,7 @@ pub(crate) mod tests { blocks.push(hash); // Avoid block pruning. - backend.pin_block(blocks[i as usize], i).unwrap(); + backend.pin_block(blocks[i as usize]).unwrap(); prev_hash = hash; } @@ -4458,7 +4482,7 @@ pub(crate) mod tests { .unwrap(); // Do not prune the fork hash. - backend.pin_block(fork_hash_3, 3).unwrap(); + backend.pin_block(fork_hash_3).unwrap(); let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, blocks[4]).unwrap(); diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index 3e2454b87f1f2..d40341d57d6ac 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -300,12 +300,10 @@ where let ClientImportOperation { mut op, notify_imported, notify_finalized } = op; - let finality_num = notify_finalized.as_ref().map(|summary| *summary.header.number()); let finality_notification = notify_finalized.map(|summary| { FinalityNotification::from_summary(summary, self.unpin_worker_sender.clone()) }); - let import_num = notify_imported.as_ref().map(|summary| *summary.header.number()); let (import_notification, storage_changes) = match notify_imported { Some(mut summary) => { let storage_changes = summary.storage_changes.take(); @@ -333,8 +331,8 @@ where self.backend.commit_operation(op)?; - if let (Some(ref notification), Some(number)) = (&finality_notification, finality_num) { - if let Err(err) = self.backend.pin_block(notification.hash, number) { + if let Some(ref notification) = finality_notification { + if let Err(err) = self.backend.pin_block(notification.hash) { error!( "Unable to pin block for finality notification. hash: {}, Error: {}", notification.hash, err @@ -342,8 +340,8 @@ where }; } - if let (Some(ref notification), Some(number)) = (&import_notification, import_num) { - if let Err(err) = self.backend.pin_block(notification.hash, number) { + if let Some(ref notification) = import_notification { + if let Err(err) = self.backend.pin_block(notification.hash) { error!( "Unable to pin block for import notification. hash: {}, Error: {}", notification.hash, err From a48e44bec9b4ee44159a9560f103bf986815f500 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Wed, 18 Jan 2023 10:47:17 +0100 Subject: [PATCH 31/37] Move RwLock one layer up --- client/db/src/lib.rs | 231 ++++++++++++++++++++++--------------------- 1 file changed, 121 insertions(+), 110 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index a9d7433ebff2c..9cf6eb166ef3f 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -506,7 +506,7 @@ impl PinnedBlockCacheEntry { } struct PinnedBlockCache { - cache: RwLock>>, + cache: LruCache>, } impl PinnedBlockCache { @@ -514,83 +514,79 @@ impl PinnedBlockCache { Self { cache: LruCache::new(NonZeroUsize::new(PINNING_CACHE_SIZE).unwrap()).into() } } - pub fn bump_ref(&self, hash: Block::Hash) { - let mut cache = self.cache.write(); - if cache.len() == PINNING_CACHE_SIZE { + pub fn bump_ref(&mut self, hash: Block::Hash) { + if self.cache.len() == PINNING_CACHE_SIZE { log::warn!(target: LOG_TARGET, "Maximum size of pinning cache reached. Removing items to make space. max_size = {}", PINNING_CACHE_SIZE); } - let entry = cache.get_or_insert_mut(hash, Default::default); + let entry = self.cache.get_or_insert_mut(hash, Default::default); entry.increase_ref(); log::trace!( target: LOG_TARGET, "Bumped cache refcount. hash = {}, num_entries = {}", hash, - cache.len() + self.cache.len() ); } - pub fn clear(&self) { - let mut cache = self.cache.write(); - cache.clear(); + pub fn clear(&mut self) { + self.cache.clear(); } pub fn contains(&self, hash: Block::Hash) -> bool { - let cache = self.cache.read(); - cache.contains(&hash) + self.cache.contains(&hash) } - pub fn insert_body(&self, hash: Block::Hash, extrinsics: Option>) { - let mut cache = self.cache.write(); - let mut entry = cache.get_or_insert_mut(hash, Default::default); + pub fn insert_body(&mut self, hash: Block::Hash, extrinsics: Option>) { + let mut entry = self.cache.get_or_insert_mut(hash, Default::default); entry.body = Some(extrinsics); log::trace!( target: LOG_TARGET, "Cached body. hash = {}, num_entries = {}", hash, - cache.len() + self.cache.len() ); } - pub fn insert_justifications(&self, hash: Block::Hash, justifications: Option) { - let mut cache = self.cache.write(); - let mut entry = cache.get_or_insert_mut(hash, Default::default); + pub fn insert_justifications( + &mut self, + hash: Block::Hash, + justifications: Option, + ) { + let mut entry = self.cache.get_or_insert_mut(hash, Default::default); entry.justifications = Some(justifications); log::trace!( target: LOG_TARGET, "Cached justification. hash = {}, num_entries = {}", hash, - cache.len() + self.cache.len() ); } - pub fn remove(&self, hash: Block::Hash) { - let mut cache = self.cache.write(); - if let Some(entry) = cache.peek_mut(&hash) { + pub fn remove(&mut self, hash: Block::Hash) { + if let Some(entry) = self.cache.peek_mut(&hash) { entry.decrease_ref(); if entry.has_no_references() { - cache.pop(&hash); + self.cache.pop(&hash); log::trace!( target: LOG_TARGET, "Removed pinned cache entry. hash = {}, num_entries = {}", hash, - cache.len() + self.cache.len() ); } } } pub fn justifications(&self, hash: &Block::Hash) -> Option> { - let cache = self.cache.read(); - if let Some(cache_entry) = cache.peek(hash) { + if let Some(cache_entry) = self.cache.peek(hash) { return cache_entry.justifications.clone() }; None } pub fn body(&self, hash: &Block::Hash) -> Option>> { - let cache = self.cache.read(); - if let Some(cache_entry) = cache.peek(hash) { + if let Some(cache_entry) = self.cache.peek(hash) { return cache_entry.body.clone() }; None @@ -604,7 +600,7 @@ pub struct BlockchainDb { leaves: RwLock>>, header_metadata_cache: Arc>, header_cache: Mutex>>, - pinned_blocks_cache: Arc>, + pinned_blocks_cache: Arc>>, } impl BlockchainDb { @@ -617,7 +613,7 @@ impl BlockchainDb { meta: Arc::new(RwLock::new(meta)), header_metadata_cache: Arc::new(HeaderMetadataCache::default()), header_cache: Default::default(), - pinned_blocks_cache: Arc::new(PinnedBlockCache::new()), + pinned_blocks_cache: Arc::new(RwLock::new(PinnedBlockCache::new())), }) } @@ -649,30 +645,34 @@ impl BlockchainDb { /// Empty the cache of pinned items. fn clear_pinning_cache(&self) { - self.pinned_blocks_cache.clear(); + let mut cache = self.pinned_blocks_cache.write(); + cache.clear(); } /// Load a justification into the cache of pinned items. /// Reference count of the item will not be increased. Use this /// to load values for items into the cache which have already been pinned. fn insert_justifications_if_pinned(&self, hash: Block::Hash, justification: Justification) { - if !self.pinned_blocks_cache.contains(hash) { + let mut cache = self.pinned_blocks_cache.write(); + if !cache.contains(hash) { return } let justifications = Justifications::from(justification); - self.pinned_blocks_cache.insert_justifications(hash, Some(justifications)); + cache.insert_justifications(hash, Some(justifications)); } /// Load a justification from the db into the cache of pinned items. /// Reference count of the item will not be increased. Use this /// to load values for items into the cache which have already been pinned. fn insert_persisted_justifications_if_pinned(&self, hash: Block::Hash) -> ClientResult<()> { - if !self.pinned_blocks_cache.contains(hash) { + let mut cache = self.pinned_blocks_cache.write(); + if !cache.contains(hash) { return Ok(()) } - let justifications = self.justifications(hash)?; - self.pinned_blocks_cache.insert_justifications(hash, justifications); + + let justifications = self.justifications_uncached(hash)?; + cache.insert_justifications(hash, justifications); Ok(()) } @@ -680,84 +680,46 @@ impl BlockchainDb { /// Reference count of the item will not be increased. Use this /// to load values for items items into the cache which have already been pinned. fn insert_persisted_body_if_pinned(&self, hash: Block::Hash) -> ClientResult<()> { - if !self.pinned_blocks_cache.contains(hash) { + let mut cache = self.pinned_blocks_cache.write(); + if !cache.contains(hash) { return Ok(()) } - let body = self.body(hash)?; - self.pinned_blocks_cache.insert_body(hash, body); + let body = self.body_uncached(hash)?; + cache.insert_body(hash, body); Ok(()) } /// Bump reference count for pinned item. fn bump_ref(&self, hash: Block::Hash) { - self.pinned_blocks_cache.bump_ref(hash); + self.pinned_blocks_cache.write().bump_ref(hash); } /// Decrease reference count for pinned item and remove if reference count is 0. fn unpin(&self, hash: Block::Hash) { - self.pinned_blocks_cache.remove(hash); + self.pinned_blocks_cache.write().remove(hash); } -} -impl sc_client_api::blockchain::HeaderBackend for BlockchainDb { - fn header(&self, hash: Block::Hash) -> ClientResult> { - let mut cache = self.header_cache.lock(); - if let Some(result) = cache.get_refresh(&hash) { - return Ok(result.clone()) - } - let header = utils::read_header( + fn justifications_uncached(&self, hash: Block::Hash) -> ClientResult> { + match read_db( &*self.db, columns::KEY_LOOKUP, - columns::HEADER, + columns::JUSTIFICATIONS, BlockId::::Hash(hash), - )?; - cache_header(&mut cache, hash, header.clone()); - Ok(header) - } - - fn info(&self) -> sc_client_api::blockchain::Info { - let meta = self.meta.read(); - sc_client_api::blockchain::Info { - best_hash: meta.best_hash, - best_number: meta.best_number, - genesis_hash: meta.genesis_hash, - finalized_hash: meta.finalized_hash, - finalized_number: meta.finalized_number, - finalized_state: meta.finalized_state, - number_leaves: self.leaves.read().count(), - block_gap: meta.block_gap, - } - } - - fn status(&self, hash: Block::Hash) -> ClientResult { - match self.header(hash)?.is_some() { - true => Ok(sc_client_api::blockchain::BlockStatus::InChain), - false => Ok(sc_client_api::blockchain::BlockStatus::Unknown), + )? { + Some(justifications) => match Decode::decode(&mut &justifications[..]) { + Ok(justifications) => Ok(Some(justifications)), + Err(err) => + return Err(sp_blockchain::Error::Backend(format!( + "Error decoding justifications: {}", + err + ))), + }, + None => Ok(None), } } - fn number(&self, hash: Block::Hash) -> ClientResult>> { - Ok(self.header_metadata(hash).ok().map(|header_metadata| header_metadata.number)) - } - - fn hash(&self, number: NumberFor) -> ClientResult> { - Ok(utils::read_header::( - &*self.db, - columns::KEY_LOOKUP, - columns::HEADER, - BlockId::Number(number), - )? - .map(|header| header.hash())) - } -} - -impl sc_client_api::blockchain::Backend for BlockchainDb { - fn body(&self, hash: Block::Hash) -> ClientResult>> { - if let Some(result) = self.pinned_blocks_cache.body(&hash) { - return Ok(result) - } - + fn body_uncached(&self, hash: Block::Hash) -> ClientResult>> { if let Some(body) = read_db(&*self.db, columns::KEY_LOOKUP, columns::BODY, BlockId::Hash::(hash))? { @@ -821,28 +783,77 @@ impl sc_client_api::blockchain::Backend for BlockchainDb ClientResult> { - if let Some(result) = self.pinned_blocks_cache.justifications(&hash) { - return Ok(result) +impl sc_client_api::blockchain::HeaderBackend for BlockchainDb { + fn header(&self, hash: Block::Hash) -> ClientResult> { + let mut cache = self.header_cache.lock(); + if let Some(result) = cache.get_refresh(&hash) { + return Ok(result.clone()) } - - match read_db( + let header = utils::read_header( &*self.db, columns::KEY_LOOKUP, - columns::JUSTIFICATIONS, + columns::HEADER, BlockId::::Hash(hash), - )? { - Some(justifications) => match Decode::decode(&mut &justifications[..]) { - Ok(justifications) => Ok(Some(justifications)), - Err(err) => - return Err(sp_blockchain::Error::Backend(format!( - "Error decoding justifications: {}", - err - ))), - }, - None => Ok(None), + )?; + cache_header(&mut cache, hash, header.clone()); + Ok(header) + } + + fn info(&self) -> sc_client_api::blockchain::Info { + let meta = self.meta.read(); + sc_client_api::blockchain::Info { + best_hash: meta.best_hash, + best_number: meta.best_number, + genesis_hash: meta.genesis_hash, + finalized_hash: meta.finalized_hash, + finalized_number: meta.finalized_number, + finalized_state: meta.finalized_state, + number_leaves: self.leaves.read().count(), + block_gap: meta.block_gap, + } + } + + fn status(&self, hash: Block::Hash) -> ClientResult { + match self.header(hash)?.is_some() { + true => Ok(sc_client_api::blockchain::BlockStatus::InChain), + false => Ok(sc_client_api::blockchain::BlockStatus::Unknown), + } + } + + fn number(&self, hash: Block::Hash) -> ClientResult>> { + Ok(self.header_metadata(hash).ok().map(|header_metadata| header_metadata.number)) + } + + fn hash(&self, number: NumberFor) -> ClientResult> { + Ok(utils::read_header::( + &*self.db, + columns::KEY_LOOKUP, + columns::HEADER, + BlockId::Number(number), + )? + .map(|header| header.hash())) + } +} + +impl sc_client_api::blockchain::Backend for BlockchainDb { + fn body(&self, hash: Block::Hash) -> ClientResult>> { + let cache = self.pinned_blocks_cache.read(); + if let Some(result) = cache.body(&hash) { + return Ok(result) } + + self.body_uncached(hash) + } + + fn justifications(&self, hash: Block::Hash) -> ClientResult> { + let cache = self.pinned_blocks_cache.read(); + if let Some(result) = cache.justifications(&hash) { + return Ok(result) + } + + self.justifications_uncached(hash) } fn last_finalized(&self) -> ClientResult { From 4014c16a99f920d39c08e8a5e77ed7857d67ef9f Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Wed, 18 Jan 2023 12:15:02 +0100 Subject: [PATCH 32/37] Apply code style suggestions --- client/db/src/lib.rs | 17 ++++++++--------- client/service/src/client/client.rs | 16 ++++++++-------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 9cf6eb166ef3f..c0420da5a3e80 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -511,7 +511,12 @@ struct PinnedBlockCache { impl PinnedBlockCache { pub fn new() -> Self { - Self { cache: LruCache::new(NonZeroUsize::new(PINNING_CACHE_SIZE).unwrap()).into() } + Self { + cache: LruCache::new( + NonZeroUsize::new(PINNING_CACHE_SIZE).expect("Capacity is larger than 0; qed"), + ) + .into(), + } } pub fn bump_ref(&mut self, hash: Block::Hash) { @@ -579,17 +584,11 @@ impl PinnedBlockCache { } pub fn justifications(&self, hash: &Block::Hash) -> Option> { - if let Some(cache_entry) = self.cache.peek(hash) { - return cache_entry.justifications.clone() - }; - None + self.cache.peek(hash).and_then(|entry| entry.justifications.clone()) } pub fn body(&self, hash: &Block::Hash) -> Option>> { - if let Some(cache_entry) = self.cache.peek(hash) { - return cache_entry.body.clone() - }; - None + self.cache.peek(hash).and_then(|entry| entry.body.clone()) } } diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index d40341d57d6ac..00eb732ed9bc9 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -430,19 +430,19 @@ where "unpin-worker", None, async move { - loop { - if let Some(message) = rx.next().await { - if let Some(backend) = task_backend.upgrade() { - backend.unpin_block(message); - } else { - log::warn!(target: "db", "Terminating unpin-worker, backend reference was dropped."); - return - } + while let Some(message) = rx.next().await { + if let Some(backend) = task_backend.upgrade() { + backend.unpin_block(message); + } else { + log::warn!("Terminating unpin-worker, backend reference was dropped."); + return } } + log::warn!("Terminating unpin-worker, stream terminated.") } .boxed(), ); + Ok(Client { backend, executor, From d547fc26ecd9d0cb02b202bf57be362eb9055188 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Wed, 18 Jan 2023 14:08:24 +0100 Subject: [PATCH 33/37] Improve comments --- client/api/src/client.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/client/api/src/client.rs b/client/api/src/client.rs index cc5cfa0bb2cc3..9d3e5056a9812 100644 --- a/client/api/src/client.rs +++ b/client/api/src/client.rs @@ -273,7 +273,7 @@ pub struct UnpinHandleInner { } impl UnpinHandleInner { - /// Create a new UnpinHandle + /// Create a new [`UnpinHandleInner`] pub fn new( hash: Block::Hash, unpin_worker_sender: TracingUnboundedSender, @@ -292,12 +292,12 @@ impl Drop for UnpinHandleInner { /// Keeps a specific block pinned while the handle is alive. /// Once the last handle instance for a given block is dropped, the -/// block is unpinned in the backend. +/// block is unpinned in the [`Backend`](crate::backend::Backend::unpin_block). #[derive(Debug, Clone)] pub struct UnpinHandle(Arc>); impl UnpinHandle { - /// Create a new UnpinHandle + /// Create a new [`UnpinHandle`] pub fn new( hash: Block::Hash, unpin_worker_sender: TracingUnboundedSender, From de6aa3691afdbeaa5c2a13201366c41bf6eec6bb Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Wed, 18 Jan 2023 15:28:04 +0100 Subject: [PATCH 34/37] Replace lru crate by schnellru --- Cargo.lock | 53 +++++-- client/db/Cargo.toml | 2 +- client/db/src/lib.rs | 123 +--------------- client/db/src/pinned_blocks_cache.rs | 207 +++++++++++++++++++++++++++ 4 files changed, 253 insertions(+), 132 deletions(-) create mode 100644 client/db/src/pinned_blocks_cache.rs diff --git a/Cargo.lock b/Cargo.lock index 8b7d2f35615ec..100e0d2152096 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -137,6 +137,18 @@ dependencies = [ "version_check", ] +[[package]] +name = "ahash" +version = "0.8.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bf6ccdb167abbf410dcb915cabd428929d7f6a04980b54a11f26a39f1c7f7107" +dependencies = [ + "cfg-if", + "getrandom 0.2.8", + "once_cell", + "version_check", +] + [[package]] name = "aho-corasick" version = "0.7.20" @@ -1836,7 +1848,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7c24f403d068ad0b359e577a77f92392118be3f3c927538f2bb544a5ecd828c6" dependencies = [ "curve25519-dalek 3.2.0", - "hashbrown", + "hashbrown 0.12.3", "hex", "rand_core 0.6.4", "sha2 0.9.9", @@ -2806,9 +2818,15 @@ version = "0.12.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8a9ee70c43aaf417c914396645a0fa852624801b24ebb7ae78fe8272889ac888" dependencies = [ - "ahash", + "ahash 0.7.6", ] +[[package]] +name = "hashbrown" +version = "0.13.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "43a3c133739dddd0d2990f9a4bdf8eb4b21ef50e4851ca85ab661199821d510e" + [[package]] name = "heck" version = "0.4.0" @@ -3122,7 +3140,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1885e79c1fc4b10f0e172c475f458b7f7b93061064d98c3293e98c5ba0c8b399" dependencies = [ "autocfg", - "hashbrown", + "hashbrown 0.12.3", "serde", ] @@ -4172,7 +4190,7 @@ version = "0.8.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b6e8aaa3f231bb4bd57b84b2d5dc3ae7f350265df8aa96492e0bc394a1571909" dependencies = [ - "hashbrown", + "hashbrown 0.12.3", ] [[package]] @@ -4310,7 +4328,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5e0c7cba9ce19ac7ffd2053ac9f49843bbd3f4318feedfd74e85c19d5fb0ba66" dependencies = [ "hash-db", - "hashbrown", + "hashbrown 0.12.3", ] [[package]] @@ -5085,7 +5103,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "21158b2c33aa6d4561f1c0a6ea283ca92bc54802a93b263e910746d679a7eb53" dependencies = [ "crc32fast", - "hashbrown", + "hashbrown 0.12.3", "indexmap", "memchr", ] @@ -7984,7 +8002,6 @@ dependencies = [ "kvdb-rocksdb", "linked-hash-map", "log", - "lru", "parity-db", "parity-scale-codec", "parking_lot 0.12.1", @@ -7992,6 +8009,7 @@ dependencies = [ "rand 0.8.5", "sc-client-api", "sc-state-db", + "schnellru", "sp-arithmetic", "sp-blockchain", "sp-core", @@ -8335,7 +8353,7 @@ dependencies = [ name = "sc-finality-grandpa" version = "0.10.0-dev" dependencies = [ - "ahash", + "ahash 0.7.6", "array-bytes", "assert_matches", "async-trait", @@ -8535,7 +8553,7 @@ dependencies = [ name = "sc-network-gossip" version = "0.10.0-dev" dependencies = [ - "ahash", + "ahash 0.7.6", "futures", "futures-timer", "libp2p", @@ -9133,6 +9151,17 @@ dependencies = [ "windows-sys 0.36.1", ] +[[package]] +name = "schnellru" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "772575a524feeb803e5b0fcbc6dd9f367e579488197c94c6e4023aad2305774d" +dependencies = [ + "ahash 0.8.2", + "cfg-if", + "hashbrown 0.13.2", +] + [[package]] name = "schnorrkel" version = "0.9.1" @@ -10276,11 +10305,11 @@ dependencies = [ name = "sp-trie" version = "7.0.0" dependencies = [ - "ahash", + "ahash 0.7.6", "array-bytes", "criterion", "hash-db", - "hashbrown", + "hashbrown 0.12.3", "lazy_static", "lru", "memory-db", @@ -11280,7 +11309,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "004e1e8f92535694b4cb1444dc5a8073ecf0815e3357f729638b9f8fc4062908" dependencies = [ "hash-db", - "hashbrown", + "hashbrown 0.12.3", "log", "rustc-hex", "smallvec", diff --git a/client/db/Cargo.toml b/client/db/Cargo.toml index 90764e1777391..2d7291a014d6e 100644 --- a/client/db/Cargo.toml +++ b/client/db/Cargo.toml @@ -22,11 +22,11 @@ kvdb-memorydb = "0.13.0" kvdb-rocksdb = { version = "0.17.0", optional = true } linked-hash-map = "0.5.4" log = "0.4.17" -lru = "0.8.1" parity-db = "0.4.2" parking_lot = "0.12.1" sc-client-api = { version = "4.0.0-dev", path = "../api" } sc-state-db = { version = "0.10.0-dev", path = "../state-db" } +schnellru = "0.2.1" sp-arithmetic = { version = "6.0.0", path = "../../primitives/arithmetic" } sp-blockchain = { version = "4.0.0-dev", path = "../../primitives/blockchain" } sp-core = { version = "7.0.0", path = "../../primitives/core" } diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index c0420da5a3e80..e1065617a1509 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -34,6 +34,7 @@ pub mod bench; mod children; mod parity_db; +mod pinned_blocks_cache; mod record_stats_state; mod stats; #[cfg(any(feature = "rocksdb", test))] @@ -42,17 +43,16 @@ mod utils; use linked_hash_map::LinkedHashMap; use log::{debug, trace, warn}; -use lru::LruCache; use parking_lot::{Mutex, RwLock}; use std::{ collections::{HashMap, HashSet}, io, - num::NonZeroUsize, path::{Path, PathBuf}, sync::Arc, }; use crate::{ + pinned_blocks_cache::PinnedBlocksCache, record_stats_state::RecordStatsState, stats::StateUsageStats, utils::{meta_keys, read_db, read_meta, DatabaseType, Meta}, @@ -113,8 +113,6 @@ pub type DbStateBuilder = sp_state_machine::TrieBackendBuilder< /// Length of a [`DbHash`]. const DB_HASH_LEN: usize = 32; -const PINNING_CACHE_SIZE: usize = 1024; - /// Hash type that this backend uses for the database. pub type DbHash = sp_core::H256; @@ -479,119 +477,6 @@ fn cache_header( } } -struct PinnedBlockCacheEntry { - ref_count: u32, - pub justifications: Option>, - pub body: Option>>, -} - -impl Default for PinnedBlockCacheEntry { - fn default() -> Self { - Self { ref_count: 0, justifications: None, body: None } - } -} - -impl PinnedBlockCacheEntry { - pub fn decrease_ref(&mut self) { - self.ref_count = self.ref_count.saturating_sub(1); - } - - pub fn increase_ref(&mut self) { - self.ref_count = self.ref_count.saturating_add(1); - } - - pub fn has_no_references(&self) -> bool { - self.ref_count == 0 - } -} - -struct PinnedBlockCache { - cache: LruCache>, -} - -impl PinnedBlockCache { - pub fn new() -> Self { - Self { - cache: LruCache::new( - NonZeroUsize::new(PINNING_CACHE_SIZE).expect("Capacity is larger than 0; qed"), - ) - .into(), - } - } - - pub fn bump_ref(&mut self, hash: Block::Hash) { - if self.cache.len() == PINNING_CACHE_SIZE { - log::warn!(target: LOG_TARGET, "Maximum size of pinning cache reached. Removing items to make space. max_size = {}", PINNING_CACHE_SIZE); - } - - let entry = self.cache.get_or_insert_mut(hash, Default::default); - entry.increase_ref(); - log::trace!( - target: LOG_TARGET, - "Bumped cache refcount. hash = {}, num_entries = {}", - hash, - self.cache.len() - ); - } - - pub fn clear(&mut self) { - self.cache.clear(); - } - - pub fn contains(&self, hash: Block::Hash) -> bool { - self.cache.contains(&hash) - } - - pub fn insert_body(&mut self, hash: Block::Hash, extrinsics: Option>) { - let mut entry = self.cache.get_or_insert_mut(hash, Default::default); - entry.body = Some(extrinsics); - log::trace!( - target: LOG_TARGET, - "Cached body. hash = {}, num_entries = {}", - hash, - self.cache.len() - ); - } - - pub fn insert_justifications( - &mut self, - hash: Block::Hash, - justifications: Option, - ) { - let mut entry = self.cache.get_or_insert_mut(hash, Default::default); - entry.justifications = Some(justifications); - log::trace!( - target: LOG_TARGET, - "Cached justification. hash = {}, num_entries = {}", - hash, - self.cache.len() - ); - } - - pub fn remove(&mut self, hash: Block::Hash) { - if let Some(entry) = self.cache.peek_mut(&hash) { - entry.decrease_ref(); - if entry.has_no_references() { - self.cache.pop(&hash); - log::trace!( - target: LOG_TARGET, - "Removed pinned cache entry. hash = {}, num_entries = {}", - hash, - self.cache.len() - ); - } - } - } - - pub fn justifications(&self, hash: &Block::Hash) -> Option> { - self.cache.peek(hash).and_then(|entry| entry.justifications.clone()) - } - - pub fn body(&self, hash: &Block::Hash) -> Option>> { - self.cache.peek(hash).and_then(|entry| entry.body.clone()) - } -} - /// Block database pub struct BlockchainDb { db: Arc>, @@ -599,7 +484,7 @@ pub struct BlockchainDb { leaves: RwLock>>, header_metadata_cache: Arc>, header_cache: Mutex>>, - pinned_blocks_cache: Arc>>, + pinned_blocks_cache: Arc>>, } impl BlockchainDb { @@ -612,7 +497,7 @@ impl BlockchainDb { meta: Arc::new(RwLock::new(meta)), header_metadata_cache: Arc::new(HeaderMetadataCache::default()), header_cache: Default::default(), - pinned_blocks_cache: Arc::new(RwLock::new(PinnedBlockCache::new())), + pinned_blocks_cache: Arc::new(RwLock::new(PinnedBlocksCache::new())), }) } diff --git a/client/db/src/pinned_blocks_cache.rs b/client/db/src/pinned_blocks_cache.rs new file mode 100644 index 0000000000000..16a0a384d021d --- /dev/null +++ b/client/db/src/pinned_blocks_cache.rs @@ -0,0 +1,207 @@ +// This file is part of Substrate. + +// Copyright (C) 2017-2023 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +use crate::LOG_TARGET; +use schnellru::{Limiter, LruMap}; +use sp_runtime::{traits::Block as BlockT, Justifications}; + +const PINNING_CACHE_SIZE: u32 = 1024; + +struct PinnedBlockCacheEntry { + ref_count: u32, + pub justifications: Option>, + pub body: Option>>, +} + +impl Default for PinnedBlockCacheEntry { + fn default() -> Self { + Self { ref_count: 0, justifications: None, body: None } + } +} + +impl PinnedBlockCacheEntry { + pub fn decrease_ref(&mut self) { + self.ref_count = self.ref_count.saturating_sub(1); + } + + pub fn increase_ref(&mut self) { + self.ref_count = self.ref_count.saturating_add(1); + } + + pub fn has_no_references(&self) -> bool { + self.ref_count == 0 + } +} + +/// A limiter for a map which is limited by the number of elements. +#[derive(Copy, Clone, Debug)] +struct LoggingByLength { + max_length: u32, +} + +/// Limiter that limits the cache by length and logs removed items. +impl LoggingByLength { + /// Creates a new length limiter with a given `max_length`. + /// + /// If you don't need to strictly cap the number of elements and just want to limit + /// the memory usage then prefer using [`ByMemoryUsage`]. + pub const fn new(max_length: u32) -> LoggingByLength { + LoggingByLength { max_length } + } +} + +impl Limiter> for LoggingByLength { + type KeyToInsert<'a> = Block::Hash; + type LinkType = u32; + + fn is_over_the_limit(&self, length: usize) -> bool { + length > self.max_length as usize + } + + fn on_insert( + &mut self, + _length: usize, + key: Self::KeyToInsert<'_>, + value: PinnedBlockCacheEntry, + ) -> Option<(Block::Hash, PinnedBlockCacheEntry)> { + if self.max_length > 0 { + Some((key, value)) + } else { + None + } + } + + fn on_replace( + &mut self, + _length: usize, + _old_key: &mut Block::Hash, + _new_key: Block::Hash, + _old_value: &mut PinnedBlockCacheEntry, + _new_value: &mut PinnedBlockCacheEntry, + ) -> bool { + true + } + + fn on_removed(&mut self, key: &mut Block::Hash, value: &mut PinnedBlockCacheEntry) { + if value.ref_count > 0 { + log::warn!( + target: LOG_TARGET, + "Pinned block cache limit reached. Evicting value. hash = {}", + key + ); + } else { + log::trace!( + target: LOG_TARGET, + "Evicting value from pinned block cache. hash = {}", + key + ) + } + } + + fn on_cleared(&mut self) {} + + fn on_grow(&mut self, _new_memory_usage: usize) -> bool { + true + } +} + +pub struct PinnedBlocksCache { + cache: LruMap, LoggingByLength>, +} + +impl PinnedBlocksCache { + pub fn new() -> Self { + Self { cache: LruMap::new(LoggingByLength::new(PINNING_CACHE_SIZE)) } + } + + pub fn bump_ref(&mut self, hash: Block::Hash) { + match self.cache.get_or_insert(hash, Default::default) { + Some(entry) => { + entry.increase_ref(); + log::trace!( + target: LOG_TARGET, + "Bumped cache refcount. hash = {}, num_entries = {}", + hash, + self.cache.len() + ); + }, + None => + log::warn!(target: LOG_TARGET, "Unable to bump reference count. hash = {}", hash), + }; + } + + pub fn clear(&mut self) { + self.cache.clear(); + } + + pub fn contains(&self, hash: Block::Hash) -> bool { + self.cache.peek(&hash).is_some() + } + + pub fn insert_body(&mut self, hash: Block::Hash, extrinsics: Option>) { + match self.cache.get_or_insert(hash, Default::default) { + Some(mut entry) => { + entry.body = Some(extrinsics); + log::trace!( + target: LOG_TARGET, + "Cached body. hash = {}, num_entries = {}", + hash, + self.cache.len() + ); + }, + None => log::warn!(target: LOG_TARGET, "Unable to insert body. hash = {}", hash), + } + } + + pub fn insert_justifications( + &mut self, + hash: Block::Hash, + justifications: Option, + ) { + match self.cache.get_or_insert(hash, Default::default) { + Some(mut entry) => { + entry.justifications = Some(justifications); + log::trace!( + target: LOG_TARGET, + "Cached justification. hash = {}, num_entries = {}", + hash, + self.cache.len() + ); + }, + None => + log::warn!(target: LOG_TARGET, "Unable to insert justification. hash = {}", hash), + } + } + + pub fn remove(&mut self, hash: Block::Hash) { + if let Some(entry) = self.cache.peek_mut(&hash) { + entry.decrease_ref(); + if entry.has_no_references() { + self.cache.remove(&hash); + } + } + } + + pub fn justifications(&self, hash: &Block::Hash) -> Option> { + self.cache.peek(hash).and_then(|entry| entry.justifications.clone()) + } + + pub fn body(&self, hash: &Block::Hash) -> Option>> { + self.cache.peek(hash).and_then(|entry| entry.body.clone()) + } +} From 6fa6d5c6a9c9ea746020f9811ca8b3e677a3144a Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Thu, 19 Jan 2023 13:08:38 +0100 Subject: [PATCH 35/37] Only insert values for pinned items + better docs --- client/db/src/pinned_blocks_cache.rs | 48 +++++++++++++++++++--------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/client/db/src/pinned_blocks_cache.rs b/client/db/src/pinned_blocks_cache.rs index 16a0a384d021d..b1e93695979e9 100644 --- a/client/db/src/pinned_blocks_cache.rs +++ b/client/db/src/pinned_blocks_cache.rs @@ -50,22 +50,19 @@ impl PinnedBlockCacheEntry { /// A limiter for a map which is limited by the number of elements. #[derive(Copy, Clone, Debug)] -struct LoggingByLength { +struct LoggingByLengthLimiter { max_length: u32, } /// Limiter that limits the cache by length and logs removed items. -impl LoggingByLength { +impl LoggingByLengthLimiter { /// Creates a new length limiter with a given `max_length`. - /// - /// If you don't need to strictly cap the number of elements and just want to limit - /// the memory usage then prefer using [`ByMemoryUsage`]. - pub const fn new(max_length: u32) -> LoggingByLength { - LoggingByLength { max_length } + pub const fn new(max_length: u32) -> LoggingByLengthLimiter { + LoggingByLengthLimiter { max_length } } } -impl Limiter> for LoggingByLength { +impl Limiter> for LoggingByLengthLimiter { type KeyToInsert<'a> = Block::Hash; type LinkType = u32; @@ -98,6 +95,10 @@ impl Limiter> for Loggi } fn on_removed(&mut self, key: &mut Block::Hash, value: &mut PinnedBlockCacheEntry) { + // If reference count was larger than 0 on removal, + // the item was removed due to capacity limitations. + // Since the cache should be large enough for pinned items, + // we want to know about these evictions. if value.ref_count > 0 { log::warn!( target: LOG_TARGET, @@ -120,15 +121,18 @@ impl Limiter> for Loggi } } +/// Reference counted cache for pinned block bodies and justifications. pub struct PinnedBlocksCache { - cache: LruMap, LoggingByLength>, + cache: LruMap, LoggingByLengthLimiter>, } impl PinnedBlocksCache { pub fn new() -> Self { - Self { cache: LruMap::new(LoggingByLength::new(PINNING_CACHE_SIZE)) } + Self { cache: LruMap::new(LoggingByLengthLimiter::new(PINNING_CACHE_SIZE)) } } + /// Increase reference count of an item. + /// Create an entry with empty value in the cache if necessary. pub fn bump_ref(&mut self, hash: Block::Hash) { match self.cache.get_or_insert(hash, Default::default) { Some(entry) => { @@ -145,16 +149,19 @@ impl PinnedBlocksCache { }; } + /// Clear the cache pub fn clear(&mut self) { self.cache.clear(); } + /// Check if item is contained in the cache pub fn contains(&self, hash: Block::Hash) -> bool { self.cache.peek(&hash).is_some() } + /// Attach body to an existing cache item pub fn insert_body(&mut self, hash: Block::Hash, extrinsics: Option>) { - match self.cache.get_or_insert(hash, Default::default) { + match self.cache.peek_mut(&hash) { Some(mut entry) => { entry.body = Some(extrinsics); log::trace!( @@ -164,16 +171,21 @@ impl PinnedBlocksCache { self.cache.len() ); }, - None => log::warn!(target: LOG_TARGET, "Unable to insert body. hash = {}", hash), + None => log::warn!( + target: LOG_TARGET, + "Unable to insert body for uncached item. hash = {}", + hash + ), } } + /// Attach justification to an existing cache item pub fn insert_justifications( &mut self, hash: Block::Hash, justifications: Option, ) { - match self.cache.get_or_insert(hash, Default::default) { + match self.cache.peek_mut(&hash) { Some(mut entry) => { entry.justifications = Some(justifications); log::trace!( @@ -183,11 +195,15 @@ impl PinnedBlocksCache { self.cache.len() ); }, - None => - log::warn!(target: LOG_TARGET, "Unable to insert justification. hash = {}", hash), + None => log::warn!( + target: LOG_TARGET, + "Unable to insert justifications for uncached item. hash = {}", + hash + ), } } + /// Remove item from cache pub fn remove(&mut self, hash: Block::Hash) { if let Some(entry) = self.cache.peek_mut(&hash) { entry.decrease_ref(); @@ -197,10 +213,12 @@ impl PinnedBlocksCache { } } + /// Get justifications for cached block pub fn justifications(&self, hash: &Block::Hash) -> Option> { self.cache.peek(hash).and_then(|entry| entry.justifications.clone()) } + /// Get body for cached block pub fn body(&self, hash: &Block::Hash) -> Option>> { self.cache.peek(hash).and_then(|entry| entry.body.clone()) } From a177b50cb8bf5acd14c58e2c530d638971296e58 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Thu, 19 Jan 2023 14:30:54 +0100 Subject: [PATCH 36/37] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- client/api/src/backend.rs | 2 +- client/api/src/client.rs | 4 ++-- client/db/src/lib.rs | 7 +++---- client/service/src/client/client.rs | 4 ++-- 4 files changed, 8 insertions(+), 9 deletions(-) diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs index 0ce5ea526425b..d5cdafbe26590 100644 --- a/client/api/src/backend.rs +++ b/client/api/src/backend.rs @@ -516,7 +516,7 @@ pub trait Backend: AuxStore + Send + Sync { /// Pin the block to keep bodies, justification and state available after pruning. /// Number of pins are reference counted. Users need to make sure to perform - /// one call to `unpin_block` per call to `pin_block`. + /// one call to [`Self::unpin_block`] per call to [`Self::pin_block`]. fn pin_block(&self, hash: Block::Hash) -> sp_blockchain::Result<()>; /// Unpin the block to allow pruning. diff --git a/client/api/src/client.rs b/client/api/src/client.rs index 9d3e5056a9812..8e7ceb68704b2 100644 --- a/client/api/src/client.rs +++ b/client/api/src/client.rs @@ -268,7 +268,7 @@ impl fmt::Display for UsageInfo { #[derive(Debug)] pub struct UnpinHandleInner { /// Hash of the block pinned by this handle - pub hash: Block::Hash, + hash: Block::Hash, unpin_worker_sender: TracingUnboundedSender, } @@ -285,7 +285,7 @@ impl UnpinHandleInner { impl Drop for UnpinHandleInner { fn drop(&mut self) { if let Err(err) = self.unpin_worker_sender.unbounded_send(self.hash) { - log::error!(target: "db", "Unable to unpin block with hash: {}, error: {:?}", self.hash, err); + log::debug!(target: "db", "Unable to unpin block with hash: {}, error: {:?}", self.hash, err); }; } } diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index e1065617a1509..70d2c93423e02 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -529,8 +529,7 @@ impl BlockchainDb { /// Empty the cache of pinned items. fn clear_pinning_cache(&self) { - let mut cache = self.pinned_blocks_cache.write(); - cache.clear(); + self.pinned_blocks_cache.write().clear(); } /// Load a justification into the cache of pinned items. @@ -2520,14 +2519,14 @@ impl sc_client_api::backend::Backend for Backend { self.storage.state_db.pin(&hash, number.saturated_into::(), hint).map_err( |_| { sp_blockchain::Error::UnknownBlock(format!( - "State already discarded for {:?}", + "State already discarded for `{:?}`", hash )) }, )?; } else { return Err(ClientError::UnknownBlock(format!( - "Can not pin block with hash {}. Block not found.", + "Can not pin block with hash `{:?}`. Block not found.", hash ))) } diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index c48c5ab839870..5f6af00020cf9 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -434,11 +434,11 @@ where if let Some(backend) = task_backend.upgrade() { backend.unpin_block(message); } else { - log::warn!("Terminating unpin-worker, backend reference was dropped."); + log::debug!("Terminating unpin-worker, backend reference was dropped."); return } } - log::warn!("Terminating unpin-worker, stream terminated.") + log::debug!("Terminating unpin-worker, stream terminated.") } .boxed(), ); From cbd0868f0314d7284cab6cbcc84d1e45d460851e Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Thu, 19 Jan 2023 14:54:49 +0100 Subject: [PATCH 37/37] Improve comments, log target and test --- client/api/src/backend.rs | 2 +- client/db/src/lib.rs | 13 +++++++---- client/db/src/pinned_blocks_cache.rs | 34 ++++++++++++++++------------ client/service/src/client/client.rs | 3 +++ 4 files changed, 32 insertions(+), 20 deletions(-) diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs index d5cdafbe26590..4ef609bdd4525 100644 --- a/client/api/src/backend.rs +++ b/client/api/src/backend.rs @@ -514,7 +514,7 @@ pub trait Backend: AuxStore + Send + Sync { /// Returns a handle to offchain storage. fn offchain_storage(&self) -> Option; - /// Pin the block to keep bodies, justification and state available after pruning. + /// Pin the block to keep body, justification and state available after pruning. /// Number of pins are reference counted. Users need to make sure to perform /// one call to [`Self::unpin_block`] per call to [`Self::pin_block`]. fn pin_block(&self, hash: Block::Hash) -> sp_blockchain::Result<()>; diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 70d2c93423e02..09ccfef1cc28b 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -98,7 +98,6 @@ pub use sp_database::Database; pub use bench::BenchmarkingState; const CACHE_HEADERS: usize = 8; -const LOG_TARGET: &str = "db-pin"; /// DB-backed patricia trie state, transaction type is an overlay of changes to commit. pub type DbState = @@ -575,12 +574,12 @@ impl BlockchainDb { /// Bump reference count for pinned item. fn bump_ref(&self, hash: Block::Hash) { - self.pinned_blocks_cache.write().bump_ref(hash); + self.pinned_blocks_cache.write().pin(hash); } /// Decrease reference count for pinned item and remove if reference count is 0. fn unpin(&self, hash: Block::Hash) { - self.pinned_blocks_cache.write().remove(hash); + self.pinned_blocks_cache.write().unpin(hash); } fn justifications_uncached(&self, hash: Block::Hash) -> ClientResult> { @@ -724,7 +723,7 @@ impl sc_client_api::blockchain::Backend for BlockchainDb ClientResult>> { let cache = self.pinned_blocks_cache.read(); if let Some(result) = cache.body(&hash) { - return Ok(result) + return Ok(result.clone()) } self.body_uncached(hash) @@ -733,7 +732,7 @@ impl sc_client_api::blockchain::Backend for BlockchainDb ClientResult> { let cache = self.pinned_blocks_cache.read(); if let Some(result) = cache.justifications(&hash) { - return Ok(result) + return Ok(result.clone()) } self.justifications_uncached(hash) @@ -4256,6 +4255,8 @@ pub(crate) mod tests { // Block 1 was pinned twice, we expect it to be still cached assert!(bc.body(blocks[1]).unwrap().is_some()); assert!(bc.justifications(blocks[1]).unwrap().is_some()); + // Headers should also be available while pinned + assert!(bc.header(blocks[1]).ok().flatten().is_some()); assert!(bc.body(blocks[2]).unwrap().is_none()); assert!(bc.justifications(blocks[2]).unwrap().is_none()); assert!(bc.body(blocks[3]).unwrap().is_none()); @@ -4301,6 +4302,7 @@ pub(crate) mod tests { bc.justifications(blocks[4]).unwrap() ); assert_eq!(Some(vec![5.into()]), bc.body(blocks[5]).unwrap()); + assert!(bc.header(blocks[5]).ok().flatten().is_some()); backend.unpin_block(blocks[4]); assert!(bc.body(blocks[4]).unwrap().is_none()); @@ -4325,6 +4327,7 @@ pub(crate) mod tests { backend.commit_operation(op).unwrap(); assert_eq!(Some(vec![5.into()]), bc.body(blocks[5]).unwrap()); + assert!(bc.header(blocks[5]).ok().flatten().is_some()); let mut expected = Justifications::from(build_justification(5)); expected.append(([0, 0, 0, 1], vec![42])); assert_eq!(Some(expected), bc.justifications(blocks[5]).unwrap()); diff --git a/client/db/src/pinned_blocks_cache.rs b/client/db/src/pinned_blocks_cache.rs index b1e93695979e9..39ff1c5277871 100644 --- a/client/db/src/pinned_blocks_cache.rs +++ b/client/db/src/pinned_blocks_cache.rs @@ -16,15 +16,21 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -use crate::LOG_TARGET; use schnellru::{Limiter, LruMap}; use sp_runtime::{traits::Block as BlockT, Justifications}; -const PINNING_CACHE_SIZE: u32 = 1024; +const LOG_TARGET: &str = "db::pin"; +const PINNING_CACHE_SIZE: usize = 1024; +/// Entry for pinned blocks cache. struct PinnedBlockCacheEntry { + /// How many times this item has been pinned ref_count: u32, + + /// Cached justifications for this block pub justifications: Option>, + + /// Cached body for this block pub body: Option>>, } @@ -51,23 +57,22 @@ impl PinnedBlockCacheEntry { /// A limiter for a map which is limited by the number of elements. #[derive(Copy, Clone, Debug)] struct LoggingByLengthLimiter { - max_length: u32, + max_length: usize, } -/// Limiter that limits the cache by length and logs removed items. impl LoggingByLengthLimiter { /// Creates a new length limiter with a given `max_length`. - pub const fn new(max_length: u32) -> LoggingByLengthLimiter { + pub const fn new(max_length: usize) -> LoggingByLengthLimiter { LoggingByLengthLimiter { max_length } } } impl Limiter> for LoggingByLengthLimiter { type KeyToInsert<'a> = Block::Hash; - type LinkType = u32; + type LinkType = usize; fn is_over_the_limit(&self, length: usize) -> bool { - length > self.max_length as usize + length > self.max_length } fn on_insert( @@ -133,7 +138,7 @@ impl PinnedBlocksCache { /// Increase reference count of an item. /// Create an entry with empty value in the cache if necessary. - pub fn bump_ref(&mut self, hash: Block::Hash) { + pub fn pin(&mut self, hash: Block::Hash) { match self.cache.get_or_insert(hash, Default::default) { Some(entry) => { entry.increase_ref(); @@ -203,8 +208,9 @@ impl PinnedBlocksCache { } } - /// Remove item from cache - pub fn remove(&mut self, hash: Block::Hash) { + /// Decreases reference count of an item. + /// If the count hits 0, the item is removed. + pub fn unpin(&mut self, hash: Block::Hash) { if let Some(entry) = self.cache.peek_mut(&hash) { entry.decrease_ref(); if entry.has_no_references() { @@ -214,12 +220,12 @@ impl PinnedBlocksCache { } /// Get justifications for cached block - pub fn justifications(&self, hash: &Block::Hash) -> Option> { - self.cache.peek(hash).and_then(|entry| entry.justifications.clone()) + pub fn justifications(&self, hash: &Block::Hash) -> Option<&Option> { + self.cache.peek(hash).and_then(|entry| entry.justifications.as_ref()) } /// Get body for cached block - pub fn body(&self, hash: &Block::Hash) -> Option>> { - self.cache.peek(hash).and_then(|entry| entry.body.clone()) + pub fn body(&self, hash: &Block::Hash) -> Option<&Option>> { + self.cache.peek(hash).and_then(|entry| entry.body.as_ref()) } } diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index 5f6af00020cf9..d32baa671f54c 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -331,6 +331,9 @@ where self.backend.commit_operation(op)?; + // We need to pin the block in the backend once + // for each notification. Once all notifications are + // dropped, the block will be unpinned automatically. if let Some(ref notification) = finality_notification { if let Err(err) = self.backend.pin_block(notification.hash) { error!(