From deeda8dcd5d49cf0cc31ab447c9acce77dfecbfd Mon Sep 17 00:00:00 2001 From: Damian Straszak Date: Fri, 11 Mar 2022 09:18:06 +0100 Subject: [PATCH] Highest caching bug (#331) * Updating heights more frequently. * Got rid of highest imported in cached chain info provider. --- finality-aleph/src/data_io/chain_info.rs | 77 +++++-------------- finality-aleph/src/data_io/data_store.rs | 10 +-- finality-aleph/src/data_io/status_provider.rs | 2 +- 3 files changed, 26 insertions(+), 63 deletions(-) diff --git a/finality-aleph/src/data_io/chain_info.rs b/finality-aleph/src/data_io/chain_info.rs index f783fb155f..52142d61c6 100644 --- a/finality-aleph/src/data_io/chain_info.rs +++ b/finality-aleph/src/data_io/chain_info.rs @@ -6,12 +6,6 @@ use sp_runtime::traits::One; use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor}; use std::sync::Arc; -#[derive(Clone, Debug)] -pub struct HighestBlocks { - pub finalized: BlockHashNum, - pub imported: BlockHashNum, -} - pub trait ChainInfoProvider { fn is_block_imported(&mut self, block: &BlockHashNum) -> bool; @@ -19,7 +13,7 @@ pub trait ChainInfoProvider { fn get_parent_hash(&mut self, block: &BlockHashNum) -> Result; - fn get_highest(&mut self) -> HighestBlocks; + fn get_highest_finalized(&mut self) -> BlockHashNum; } impl ChainInfoProvider for Arc @@ -64,12 +58,9 @@ where } } - fn get_highest(&mut self) -> HighestBlocks { + fn get_highest_finalized(&mut self) -> BlockHashNum { let status = self.info(); - HighestBlocks { - finalized: (status.finalized_hash, status.finalized_number).into(), - imported: (status.best_hash, status.best_number).into(), - } + (status.finalized_hash, status.finalized_number).into() } } @@ -81,7 +72,6 @@ where available_block_with_parent_cache: LruCache, B::Hash>, available_blocks_cache: LruCache, ()>, finalized_cache: LruCache, B::Hash>, - highest: HighestBlocks, chain_info_provider: CIP, } @@ -90,21 +80,15 @@ where B: BlockT, CIP: ChainInfoProvider, { - pub fn new(mut chain_info_provider: CIP, config: ChainInfoCacheConfig) -> Self { - let highest = chain_info_provider.get_highest(); + pub fn new(chain_info_provider: CIP, config: ChainInfoCacheConfig) -> Self { CachedChainInfoProvider { available_block_with_parent_cache: LruCache::new(config.block_cache_capacity), available_blocks_cache: LruCache::new(config.block_cache_capacity), finalized_cache: LruCache::new(config.block_cache_capacity), - highest, chain_info_provider, } } - fn update_highest_blocks(&mut self) { - self.highest = self.chain_info_provider.get_highest(); - } - pub fn inner(&mut self) -> &mut CIP { &mut self.chain_info_provider } @@ -116,17 +100,10 @@ where CIP: ChainInfoProvider, { fn is_block_imported(&mut self, block: &BlockHashNum) -> bool { - if self.highest.imported.num < block.num { - // We are lazy about updating highest blocks as this requires copying quite a bit of data - // from the client and requires a read lock. - self.update_highest_blocks(); - if self.highest.imported.num < block.num { - return false; - } - } if self.available_blocks_cache.contains(block) { return true; } + if self.chain_info_provider.is_block_imported(block) { self.available_blocks_cache.put(block.clone(), ()); return true; @@ -135,17 +112,14 @@ where } fn get_finalized_at(&mut self, num: NumberFor) -> Result, ()> { - if self.highest.finalized.num < num { - // We are lazy about updating highest blocks as this requires copying quite a bit of data - // from the client and requires a read lock. - self.update_highest_blocks(); - if self.highest.finalized.num < num { - return Err(()); - } - } if let Some(hash) = self.finalized_cache.get(&num) { return Ok((*hash, num).into()); } + + if self.get_highest_finalized().num < num { + return Err(()); + } + if let Ok(block) = self.chain_info_provider.get_finalized_at(num) { self.finalized_cache.put(num, block.hash); return Ok(block); @@ -154,17 +128,10 @@ where } fn get_parent_hash(&mut self, block: &BlockHashNum) -> Result { - if self.highest.imported.num < block.num { - // We are lazy about updating highest blocks as this requires copying quite a bit of data - // from the client and requires a read lock. - self.update_highest_blocks(); - if self.highest.imported.num < block.num { - return Err(()); - } - } if let Some(parent) = self.available_block_with_parent_cache.get(block) { return Ok(*parent); } + if let Ok(parent) = self.chain_info_provider.get_parent_hash(block) { self.available_block_with_parent_cache .put(block.clone(), parent); @@ -173,9 +140,8 @@ where Err(()) } - fn get_highest(&mut self) -> HighestBlocks { - self.update_highest_blocks(); - self.highest.clone() + fn get_highest_finalized(&mut self) -> BlockHashNum { + self.chain_info_provider.get_highest_finalized() } } @@ -219,8 +185,8 @@ where } fn get_finalized_at(&mut self, num: NumberFor) -> Result, ()> { - let internal_highest = self.chain_info_provider.get_highest(); - if num <= internal_highest.finalized.num { + let highest_finalized_inner = self.chain_info_provider.get_highest_finalized(); + if num <= highest_finalized_inner.num { return self.chain_info_provider.get_finalized_at(num); } if num > self.aux_finalized.num { @@ -239,15 +205,12 @@ where self.chain_info_provider.get_parent_hash(block) } - fn get_highest(&mut self) -> HighestBlocks { - let highest = self.chain_info_provider.get_highest(); - if self.aux_finalized.num > highest.finalized.num { - HighestBlocks { - finalized: self.aux_finalized.clone(), - imported: highest.imported, - } + fn get_highest_finalized(&mut self) -> BlockHashNum { + let highest_finalized_inner = self.chain_info_provider.get_highest_finalized(); + if self.aux_finalized.num > highest_finalized_inner.num { + self.aux_finalized.clone() } else { - highest + highest_finalized_inner } } } diff --git a/finality-aleph/src/data_io/data_store.rs b/finality-aleph/src/data_io/data_store.rs index 737b162fa5..37d29ae45b 100644 --- a/finality-aleph/src/data_io/data_store.rs +++ b/finality-aleph/src/data_io/data_store.rs @@ -273,14 +273,14 @@ where } // Updates our highest known and highest finalized block info directly from the client. - fn update_highest_blocks(&mut self) { - let highest = self.chain_info_provider.get_highest(); - self.on_block_imported(highest.imported); - self.on_block_finalized(highest.finalized); + fn update_highest_finalized(&mut self) { + let highest_finalized = self.chain_info_provider.get_highest_finalized(); + self.on_block_imported(highest_finalized.clone()); + self.on_block_finalized(highest_finalized); } fn run_maintenance(&mut self) { - self.update_highest_blocks(); + self.update_highest_finalized(); let proposals_with_timestamps: Vec<_> = self .pending_proposals diff --git a/finality-aleph/src/data_io/status_provider.rs b/finality-aleph/src/data_io/status_provider.rs index 70b9a830fa..918234d796 100644 --- a/finality-aleph/src/data_io/status_provider.rs +++ b/finality-aleph/src/data_io/status_provider.rs @@ -18,7 +18,7 @@ where use crate::data_io::proposal::PendingProposalStatus::*; use crate::data_io::proposal::ProposalStatus::*; - if chain_info_provider.get_highest().finalized.num >= proposal.number_top_block() { + if chain_info_provider.get_highest_finalized().num >= proposal.number_top_block() { return Ignore; }