From ec83a57872eff5b98e556b958244c4ce7762b143 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 18 Apr 2023 12:10:45 +0200 Subject: [PATCH 1/3] Optimize level monitor reconstruction --- client/consensus/common/src/level_monitor.rs | 34 ++++++++------------ 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/client/consensus/common/src/level_monitor.rs b/client/consensus/common/src/level_monitor.rs index 294527f1f9f..573e7a3f79b 100644 --- a/client/consensus/common/src/level_monitor.rs +++ b/client/consensus/common/src/level_monitor.rs @@ -15,7 +15,7 @@ // along with Cumulus. If not, see . use sc_client_api::{blockchain::Backend as _, Backend, HeaderBackend as _}; -use sp_blockchain::{HashAndNumber, TreeRoute}; +use sp_blockchain::{HashAndNumber, HeaderMetadata, TreeRoute}; use sp_runtime::traits::{Block as BlockT, NumberFor, One, Saturating, UniqueSaturatedInto, Zero}; use std::{ collections::{HashMap, HashSet}, @@ -96,7 +96,9 @@ where /// /// Level limits are not enforced during this phase. fn restore(&mut self) { + const ERR_MSG: &str = "route from finalized to leaf should be available; qed"; let info = self.backend.blockchain().info(); + log::debug!( target: "parachain", "Restoring chain level monitor from last finalized block: {} {}", @@ -105,30 +107,22 @@ where self.lowest_level = info.finalized_number; self.import_counter = info.finalized_number; - self.block_imported(info.finalized_number, info.finalized_hash); - - let mut counter_max = info.finalized_number; for leaf in self.backend.blockchain().leaves().unwrap_or_default() { - let route = - sp_blockchain::tree_route(self.backend.blockchain(), info.finalized_hash, leaf) - .expect("Route from finalized to leaf should be available; qed"); - if !route.retracted().is_empty() { - continue + let mut info = self.backend.blockchain().header_metadata(leaf).expect(ERR_MSG); + + self.import_counter = self.import_counter.max(info.number); + + // Populate the monitor until we don't hit an already imported branch + // or the block number is below the last finalized block number. + while info.number >= self.lowest_level && !self.freshness.contains_key(&info.hash) { + self.freshness.insert(info.hash, info.number); + self.levels.entry(info.number).or_default().insert(info.hash); + info = self.backend.blockchain().header_metadata(info.parent).expect(ERR_MSG); } - route.enacted().iter().for_each(|elem| { - if !self.freshness.contains_key(&elem.hash) { - // Use the block height value as the freshness. - self.import_counter = elem.number; - self.block_imported(elem.number, elem.hash); - } - }); - counter_max = std::cmp::max(self.import_counter, counter_max); } - log::debug!(target: "parachain", "Restored chain level monitor up to height {}", counter_max); - - self.import_counter = counter_max; + log::debug!(target: "parachain", "Restored chain level monitor up to height {}", self.import_counter); } /// Check and enforce the limit bound at the given height. From 083f76ce51faeb67a091cdc9a792f2f571978c0d Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 18 Apr 2023 12:36:27 +0200 Subject: [PATCH 2/3] Fix counter increment and test --- client/consensus/common/src/level_monitor.rs | 18 ++++++++++-------- client/consensus/common/src/tests.rs | 13 +++++++++---- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/client/consensus/common/src/level_monitor.rs b/client/consensus/common/src/level_monitor.rs index 573e7a3f79b..86b490a0705 100644 --- a/client/consensus/common/src/level_monitor.rs +++ b/client/consensus/common/src/level_monitor.rs @@ -109,16 +109,18 @@ where self.import_counter = info.finalized_number; for leaf in self.backend.blockchain().leaves().unwrap_or_default() { - let mut info = self.backend.blockchain().header_metadata(leaf).expect(ERR_MSG); + let mut meta = self.backend.blockchain().header_metadata(leaf).expect(ERR_MSG); - self.import_counter = self.import_counter.max(info.number); + self.import_counter = self.import_counter.max(meta.number); // Populate the monitor until we don't hit an already imported branch - // or the block number is below the last finalized block number. - while info.number >= self.lowest_level && !self.freshness.contains_key(&info.hash) { - self.freshness.insert(info.hash, info.number); - self.levels.entry(info.number).or_default().insert(info.hash); - info = self.backend.blockchain().header_metadata(info.parent).expect(ERR_MSG); + while !self.freshness.contains_key(&meta.hash) { + self.freshness.insert(meta.hash, meta.number); + self.levels.entry(meta.number).or_default().insert(meta.hash); + if meta.number <= self.lowest_level { + break + } + meta = self.backend.blockchain().header_metadata(meta.parent).expect(ERR_MSG); } } @@ -349,9 +351,9 @@ where /// Add a new imported block information to the monitor. pub fn block_imported(&mut self, number: NumberFor, hash: Block::Hash) { + self.import_counter += One::one(); self.freshness.insert(hash, self.import_counter); self.levels.entry(number).or_default().insert(hash); - self.import_counter += One::one(); // Do cleanup once in a while, we are allowed to have some obsolete information. let finalized_num = self.backend.blockchain().info().finalized_number; diff --git a/client/consensus/common/src/tests.rs b/client/consensus/common/src/tests.rs index e44c26e85d1..23516d96388 100644 --- a/client/consensus/common/src/tests.rs +++ b/client/consensus/common/src/tests.rs @@ -765,6 +765,12 @@ fn restore_limit_monitor() { LevelLimit::Some(LEVEL_LIMIT), ); + let monitor_sd = para_import.monitor.clone().unwrap(); + + let monitor = monitor_sd.shared_data(); + assert_eq!(monitor.import_counter, 3); + std::mem::drop(monitor); + let block13 = build_and_import_block_ext( &*client, BlockOrigin::Own, @@ -783,14 +789,13 @@ fn restore_limit_monitor() { let expected = vec![blocks1[1].header.hash(), block13.header.hash()]; assert_eq!(leaves, expected); - let monitor = para_import.monitor.unwrap(); - let monitor = monitor.shared_data(); - assert_eq!(monitor.import_counter, 5); + let monitor = monitor_sd.shared_data(); + assert_eq!(monitor.import_counter, 4); assert!(monitor.levels.iter().all(|(number, hashes)| { hashes .iter() .filter(|hash| **hash != block13.header.hash()) .all(|hash| *number == *monitor.freshness.get(hash).unwrap()) })); - assert_eq!(*monitor.freshness.get(&block13.header.hash()).unwrap(), monitor.import_counter - 1); + assert_eq!(*monitor.freshness.get(&block13.header.hash()).unwrap(), monitor.import_counter); } From 6d4d279e7a891340d03404c2f701d5efe1e83d8a Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 18 Apr 2023 22:06:25 +0200 Subject: [PATCH 3/3] Struct comments as doc comments --- client/consensus/common/src/level_monitor.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/client/consensus/common/src/level_monitor.rs b/client/consensus/common/src/level_monitor.rs index 86b490a0705..4f344b505a7 100644 --- a/client/consensus/common/src/level_monitor.rs +++ b/client/consensus/common/src/level_monitor.rs @@ -48,17 +48,17 @@ pub enum LevelLimit { /// Support structure to constrain the number of leaves at each level. pub struct LevelMonitor { - // Max number of leaves for each level. + /// Max number of leaves for each level. level_limit: usize, - // Monotonic counter used to keep track of block freshness. + /// Monotonic counter used to keep track of block freshness. pub(crate) import_counter: NumberFor, - // Map between blocks hashes and freshness. + /// Map between blocks hashes and freshness. pub(crate) freshness: HashMap>, - // Blockchain levels cache. + /// Blockchain levels cache. pub(crate) levels: HashMap, HashSet>, - // Lower level number stored by the levels map. + /// Lower level number stored by the levels map. lowest_level: NumberFor, - // Backend reference to remove blocks on level saturation. + /// Backend reference to remove blocks on level saturation. backend: Arc, }