From c42a4d8d182baeecc63c2c81857cfcb693421bbd Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Sep 2024 07:56:33 +0300 Subject: [PATCH 01/19] chainHead: Introduce state for block tracking Signed-off-by: Alexandru Vasile --- .../src/chain_head/chain_head_follow.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs index ebb72ed3d156..babdef8e22f9 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs @@ -53,6 +53,19 @@ use std::{ /// `Initialized` event. const MAX_FINALIZED_BLOCKS: usize = 16; +/// The type of the block announcement. +/// +/// The event lifecycle moves thorugh the following states: +/// `NewBlock` -> `BestBlock` -> `Finalized` (or pruned) +enum BlockType { + /// The block was announced by the `NewBlock` event. + NewBlock, + /// The block was announced by the `BestBlock` event. + BestBlock, + /// The block was announced by the `Finalized` event. + FinalizedBlock, +} + use super::subscription::InsertedSubscriptionData; /// Generates the events of the `chainHead_follow` method. @@ -71,6 +84,9 @@ pub struct ChainHeadFollower, Block: BlockT, Client> { current_best_block: Option, /// LRU cache of pruned blocks. pruned_blocks: LruMap, + + /// LRU cache of pruned blocks. + announced_blocks: LruMap, /// Stop all subscriptions if the distance between the leaves and the current finalized /// block is larger than this value. max_lagging_distance: usize, @@ -96,6 +112,9 @@ impl, Block: BlockT, Client> ChainHeadFollower Date: Fri, 27 Sep 2024 07:58:09 +0300 Subject: [PATCH 02/19] chainHead: Fix typo in distace_within_reason Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/chain_head/chain_head_follow.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs index babdef8e22f9..c6addb5bed47 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs @@ -84,7 +84,6 @@ pub struct ChainHeadFollower, Block: BlockT, Client> { current_best_block: Option, /// LRU cache of pruned blocks. pruned_blocks: LruMap, - /// LRU cache of pruned blocks. announced_blocks: LruMap, /// Stop all subscriptions if the distance between the leaves and the current finalized @@ -233,7 +232,7 @@ where /// /// This edge-case can happen for parachains where the relay chain syncs slower to /// the head of the chain than the parachain node that is synced already. - fn distace_within_reason( + fn distance_within_reason( &self, block: Block::Hash, finalized: Block::Hash, @@ -269,7 +268,7 @@ where // Ensure all leaves are within a reasonable distance from the finalized block, // before traversing the tree. for leaf in &leaves { - self.distace_within_reason(*leaf, finalized)?; + self.distance_within_reason(*leaf, finalized)?; } for leaf in leaves { From 00e8eab6897b93a505becf0b807e5c6d28935936 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Sep 2024 08:04:16 +0300 Subject: [PATCH 03/19] chainHead: Keep track of blocks from the initial event Signed-off-by: Alexandru Vasile --- .../src/chain_head/chain_head_follow.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs index c6addb5bed47..b9cd4db3fa2e 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs @@ -344,6 +344,10 @@ where let finalized_block_hash = startup_point.finalized_hash; let finalized_block_runtime = self.generate_runtime_event(finalized_block_hash, None); + for finalized in &finalized_block_hashes { + self.announced_blocks.insert(*finalized, BlockType::FinalizedBlock); + } + let initialized_event = FollowEvent::Initialized(Initialized { finalized_block_hashes: finalized_block_hashes.into(), finalized_block_runtime, @@ -354,6 +358,13 @@ where finalized_block_descendants.push(initialized_event); for (child, parent) in initial_blocks.into_iter() { + // If the parent was not announced we have a gap currently. + // This can happen during a WarpSync. + if self.announced_blocks.get(&parent).is_none() { + return Err(SubscriptionManagementError::BlockHeaderAbsent); + } + self.announced_blocks.insert(child, BlockType::NewBlock); + let new_runtime = self.generate_runtime_event(child, Some(parent)); let event = FollowEvent::NewBlock(NewBlock { @@ -369,6 +380,11 @@ where // Generate a new best block event. let best_block_hash = startup_point.best_hash; if best_block_hash != finalized_block_hash { + if self.announced_blocks.get(&best_block_hash).is_none() { + return Err(SubscriptionManagementError::BlockHeaderAbsent); + } + self.announced_blocks.insert(best_block_hash, BlockType::BestBlock); + let best_block = FollowEvent::BestBlockChanged(BestBlockChanged { best_block_hash }); self.current_best_block = Some(best_block_hash); finalized_block_descendants.push(best_block); From 6e55454d0509e54f9d96d0296d818c179611fa08 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Sep 2024 11:36:57 +0300 Subject: [PATCH 04/19] chainHead: Track reported blocks from the NewBlock import path Signed-off-by: Alexandru Vasile --- .../src/chain_head/chain_head_follow.rs | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs index b9cd4db3fa2e..309a45a4efde 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs @@ -442,21 +442,30 @@ where notification: BlockImportNotification, startup_point: &StartupPoint, ) -> Result>, SubscriptionManagementError> { - // The block was already pinned by the initial block events or by the finalized event. - if !self.sub_handle.pin_block(&self.sub_id, notification.hash)? { - return Ok(Default::default()) - } + let block_hash = notification.hash; + + // Ensure the block is pinned before generating the events. + self.sub_handle.pin_block(&self.sub_id, block_hash)?; // Ensure we are only reporting blocks after the starting point. if *notification.header.number() < startup_point.finalized_number { return Ok(Default::default()) } - Ok(self.generate_import_events( - notification.hash, - *notification.header.parent_hash(), - notification.is_new_best, - )) + if self.announced_blocks.get(&block_hash).is_some() { + // Block was already reported by the finalized branch. + return Ok(Default::default()) + } + + // Double check the parent hash. If the parent hash is not reported, we have a gap. + let parent_block_hash = *notification.header.parent_hash(); + if self.announced_blocks.get(&parent_block_hash).is_none() { + // The parent block was not reported, we have a gap. + return Err(SubscriptionManagementError::Custom("Parent block was not reported".into())) + } + + self.announced_blocks.insert(block_hash, BlockType::NewBlock); + Ok(self.generate_import_events(block_hash, parent_block_hash, notification.is_new_best)) } /// Generates new block events from the given finalized hashes. From 678e9bf5d52c10e8a809c380d446e53987fdc6ee Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Sep 2024 11:43:07 +0300 Subject: [PATCH 05/19] chainHead: Track reported blocks from the Finalized import path Signed-off-by: Alexandru Vasile --- .../src/chain_head/chain_head_follow.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs index 309a45a4efde..79cbfe0a83d6 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs @@ -491,12 +491,21 @@ where return Err(SubscriptionManagementError::BlockHeaderAbsent) }; + if self.announced_blocks.get(first_header.parent_hash()).is_none() { + return Err(SubscriptionManagementError::Custom( + "Parent block was not reported for a finalized block".into(), + )); + } + let parents = std::iter::once(first_header.parent_hash()).chain(finalized_block_hashes.iter()); for (i, (hash, parent)) in finalized_block_hashes.iter().zip(parents).enumerate() { - // Check if the block was already reported and thus, is already pinned. - if !self.sub_handle.pin_block(&self.sub_id, *hash)? { - continue + // Ensure the block is pinned before generating the events. + self.sub_handle.pin_block(&self.sub_id, *hash)?; + + // Check if the block was already reported. + if self.announced_blocks.get(hash).is_some() { + continue; } // Generate `NewBlock` events for all blocks beside the last block in the list @@ -504,6 +513,7 @@ where if !is_last { // Generate only the `NewBlock` event for this block. events.extend(self.generate_import_events(*hash, *parent, false)); + self.announced_blocks.insert(*hash, BlockType::NewBlock); continue; } @@ -526,7 +536,8 @@ where } // Let's generate the `NewBlock` and `NewBestBlock` events for the block. - events.extend(self.generate_import_events(*hash, *parent, true)) + events.extend(self.generate_import_events(*hash, *parent, true)); + self.announced_blocks.insert(*hash, BlockType::NewBlock); } Ok(events) From 35b6573ba815b8269b9f12c5c70740749b070ab9 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Sep 2024 11:48:53 +0300 Subject: [PATCH 06/19] chainHead: Track announced blocks as finalized Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/chain_head/chain_head_follow.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs index 79cbfe0a83d6..f814361f2e95 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs @@ -599,6 +599,10 @@ where let pruned_block_hashes = self.get_pruned_hashes(¬ification.stale_heads, last_finalized)?; + for finalized in &finalized_block_hashes { + self.announced_blocks.insert(*finalized, BlockType::FinalizedBlock); + } + let finalized_event = FollowEvent::Finalized(Finalized { finalized_block_hashes, pruned_block_hashes: pruned_block_hashes.clone(), From 5e03600d2eaf809237d27f79608664b2db8e4b1a Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Sep 2024 15:59:01 +0300 Subject: [PATCH 07/19] chainHead/tests: Detect block gaps with unknwon parents Signed-off-by: Alexandru Vasile --- .../rpc-spec-v2/src/chain_head/tests.rs | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/tests.rs b/substrate/client/rpc-spec-v2/src/chain_head/tests.rs index 30a01b93b315..77be0bcb341e 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/tests.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/tests.rs @@ -4052,3 +4052,72 @@ async fn follow_report_best_block_of_a_known_block() { }); assert_eq!(event, expected); } + +#[tokio::test] +async fn follow_event_with_unknown_parent() { + let builder = TestClientBuilder::new(); + let backend = builder.backend(); + let client = Arc::new(builder.build()); + + let client_mock = Arc::new(ChainHeadMockClient::new(client.clone())); + + let api = ChainHead::new( + client_mock.clone(), + backend, + Arc::new(TaskExecutor::default()), + ChainHeadConfig { + global_max_pinned_blocks: MAX_PINNED_BLOCKS, + subscription_max_pinned_duration: Duration::from_secs(MAX_PINNED_SECS), + subscription_max_ongoing_operations: MAX_OPERATIONS, + operation_max_storage_items: MAX_PAGINATION_LIMIT, + max_lagging_distance: MAX_LAGGING_DISTANCE, + max_follow_subscriptions_per_connection: MAX_FOLLOW_SUBSCRIPTIONS_PER_CONNECTION, + }, + ) + .into_rpc(); + + let finalized_hash = client.info().finalized_hash; + let mut sub = api.subscribe_unbounded("chainHead_v1_follow", [false]).await.unwrap(); + // Initialized must always be reported first. + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::Initialized(Initialized { + finalized_block_hashes: vec![format!("{:?}", finalized_hash)], + finalized_block_runtime: None, + with_runtime: false, + }); + assert_eq!(event, expected); + + // Block tree: + // + // finalized -> (gap: block 1) -> block 2 + // + // Block 1 is not announced yet. ChainHead should report the stop + // event when encountering an unknown parent of block 2. + + // Note: `client` is used just for constructing the blocks. + // The blocks are imported to chainHead using the `client_mock`. + let block_1 = BlockBuilderBuilder::new(&*client) + .on_parent_block(client.chain_info().genesis_hash) + .with_parent_block_number(0) + .build() + .unwrap() + .build() + .unwrap() + .block; + let block_1_hash = block_1.hash(); + client.import(BlockOrigin::Own, block_1.clone()).await.unwrap(); + + let block_2 = BlockBuilderBuilder::new(&*client) + .on_parent_block(block_1_hash) + .with_parent_block_number(1) + .build() + .unwrap() + .build() + .unwrap() + .block; + client.import(BlockOrigin::Own, block_2.clone()).await.unwrap(); + + run_with_timeout(client_mock.trigger_import_stream(block_2.header)).await; + // When importing the block 2, chainHead detects a gap in our blocks and stops. + assert_matches!(get_next_event::>(&mut sub).await, FollowEvent::Stop); +} From 5c80006fc768297406fb6d4032df06ebff0f9c62 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Sep 2024 16:34:58 +0300 Subject: [PATCH 08/19] chainHead: Simplify state tracking Signed-off-by: Alexandru Vasile --- .../src/chain_head/chain_head_follow.rs | 33 +++++-------------- 1 file changed, 9 insertions(+), 24 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs index f814361f2e95..408c50d41d3e 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs @@ -24,7 +24,7 @@ use crate::chain_head::{ BestBlockChanged, Finalized, FollowEvent, Initialized, NewBlock, RuntimeEvent, RuntimeVersionEvent, }, - subscription::{SubscriptionManagement, SubscriptionManagementError}, + subscription::{InsertedSubscriptionData, SubscriptionManagement, SubscriptionManagementError}, }; use futures::{ channel::oneshot, @@ -53,21 +53,6 @@ use std::{ /// `Initialized` event. const MAX_FINALIZED_BLOCKS: usize = 16; -/// The type of the block announcement. -/// -/// The event lifecycle moves thorugh the following states: -/// `NewBlock` -> `BestBlock` -> `Finalized` (or pruned) -enum BlockType { - /// The block was announced by the `NewBlock` event. - NewBlock, - /// The block was announced by the `BestBlock` event. - BestBlock, - /// The block was announced by the `Finalized` event. - FinalizedBlock, -} - -use super::subscription::InsertedSubscriptionData; - /// Generates the events of the `chainHead_follow` method. pub struct ChainHeadFollower, Block: BlockT, Client> { /// Substrate client. @@ -85,7 +70,7 @@ pub struct ChainHeadFollower, Block: BlockT, Client> { /// LRU cache of pruned blocks. pruned_blocks: LruMap, /// LRU cache of pruned blocks. - announced_blocks: LruMap, + announced_blocks: LruMap, /// Stop all subscriptions if the distance between the leaves and the current finalized /// block is larger than this value. max_lagging_distance: usize, @@ -345,7 +330,7 @@ where let finalized_block_runtime = self.generate_runtime_event(finalized_block_hash, None); for finalized in &finalized_block_hashes { - self.announced_blocks.insert(*finalized, BlockType::FinalizedBlock); + self.announced_blocks.insert(*finalized, ()); } let initialized_event = FollowEvent::Initialized(Initialized { @@ -363,7 +348,7 @@ where if self.announced_blocks.get(&parent).is_none() { return Err(SubscriptionManagementError::BlockHeaderAbsent); } - self.announced_blocks.insert(child, BlockType::NewBlock); + self.announced_blocks.insert(child, ()); let new_runtime = self.generate_runtime_event(child, Some(parent)); @@ -383,7 +368,7 @@ where if self.announced_blocks.get(&best_block_hash).is_none() { return Err(SubscriptionManagementError::BlockHeaderAbsent); } - self.announced_blocks.insert(best_block_hash, BlockType::BestBlock); + self.announced_blocks.insert(best_block_hash, ()); let best_block = FollowEvent::BestBlockChanged(BestBlockChanged { best_block_hash }); self.current_best_block = Some(best_block_hash); @@ -464,7 +449,7 @@ where return Err(SubscriptionManagementError::Custom("Parent block was not reported".into())) } - self.announced_blocks.insert(block_hash, BlockType::NewBlock); + self.announced_blocks.insert(block_hash, ()); Ok(self.generate_import_events(block_hash, parent_block_hash, notification.is_new_best)) } @@ -513,7 +498,7 @@ where if !is_last { // Generate only the `NewBlock` event for this block. events.extend(self.generate_import_events(*hash, *parent, false)); - self.announced_blocks.insert(*hash, BlockType::NewBlock); + self.announced_blocks.insert(*hash, ()); continue; } @@ -537,7 +522,7 @@ where // Let's generate the `NewBlock` and `NewBestBlock` events for the block. events.extend(self.generate_import_events(*hash, *parent, true)); - self.announced_blocks.insert(*hash, BlockType::NewBlock); + self.announced_blocks.insert(*hash, ()); } Ok(events) @@ -600,7 +585,7 @@ where self.get_pruned_hashes(¬ification.stale_heads, last_finalized)?; for finalized in &finalized_block_hashes { - self.announced_blocks.insert(*finalized, BlockType::FinalizedBlock); + self.announced_blocks.insert(*finalized, ()); } let finalized_event = FollowEvent::Finalized(Finalized { From 7e74e4a59c6d322e34726f93ea4678e88dd6313c Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Sep 2024 16:42:48 +0300 Subject: [PATCH 09/19] prdoc: Add prdoc Signed-off-by: Alexandru Vasile --- prdoc/pr_5856.prdoc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 prdoc/pr_5856.prdoc diff --git a/prdoc/pr_5856.prdoc b/prdoc/pr_5856.prdoc new file mode 100644 index 000000000000..383e95e3da88 --- /dev/null +++ b/prdoc/pr_5856.prdoc @@ -0,0 +1,17 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Extend state tracking of chainHead to capture notification gaps + +doc: + - audience: Node Dev + description: | + This PR extends the state tracking of the RPC-v2 chainHead methods. + ChainHead tracks the reported blocks to detect notification gaps. + This state tracking ensures we can detect `NewBlock` events for + which we did not report previously the parent hash. + +crates: + - name: sc-rpc-spec-v2 + bump: minor + From fbb8b160f058bafac3ace24a648f14d92c7a1691 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Tue, 1 Oct 2024 19:28:20 +0300 Subject: [PATCH 10/19] Update substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Co-authored-by: Sebastian Kunert --- .../client/rpc-spec-v2/src/chain_head/chain_head_follow.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs index 408c50d41d3e..846afd9c825e 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs @@ -69,7 +69,7 @@ pub struct ChainHeadFollower, Block: BlockT, Client> { current_best_block: Option, /// LRU cache of pruned blocks. pruned_blocks: LruMap, - /// LRU cache of pruned blocks. + /// LRU cache of announced blocks. announced_blocks: LruMap, /// Stop all subscriptions if the distance between the leaves and the current finalized /// block is larger than this value. From 2b44eaf68ccce92e116f8ada7fbeb1fe26d2bedc Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 2 Oct 2024 19:05:37 +0300 Subject: [PATCH 11/19] chainHead: Move finalized blocks to a shorter lived LRU Signed-off-by: Alexandru Vasile --- .../src/chain_head/chain_head_follow.rs | 68 ++++++++++++++----- 1 file changed, 51 insertions(+), 17 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs index 846afd9c825e..5963a1228581 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs @@ -70,12 +70,48 @@ pub struct ChainHeadFollower, Block: BlockT, Client> { /// LRU cache of pruned blocks. pruned_blocks: LruMap, /// LRU cache of announced blocks. - announced_blocks: LruMap, + announced_blocks: AnnouncedBlocks, /// Stop all subscriptions if the distance between the leaves and the current finalized /// block is larger than this value. max_lagging_distance: usize, } +struct AnnouncedBlocks { + /// Unfinalized blocks. + blocks: LruMap, + /// Finalized blocks. + finalized: LruMap, +} + +impl AnnouncedBlocks { + /// Creates a new `AnnouncedBlocks`. + fn new() -> Self { + Self { + blocks: LruMap::new(ByLength::new((MAX_PINNED_BLOCKS - MAX_FINALIZED_BLOCKS) as u32)), + finalized: LruMap::new(ByLength::new(MAX_FINALIZED_BLOCKS as u32)), + } + } + + /// Insert the block into the announced blocks. + fn insert(&mut self, block: Block::Hash, finalized: bool) { + if finalized { + // When a block is declared as finalized, it is removed from the unfinalized blocks. + // + // Given that the finalized blocks are bounded to `MAX_FINALIZED_BLOCKS`, + // this ensures we keep the minimum number of blocks in memory. + self.blocks.remove(&block); + self.finalized.insert(block, ()); + } else { + self.blocks.insert(block, ()); + } + } + + /// Check if the block was previously announced. + fn was_announced(&mut self, block: &Block::Hash) -> bool { + self.blocks.get(block).is_some() || self.finalized.get(block).is_some() + } +} + impl, Block: BlockT, Client> ChainHeadFollower { /// Create a new [`ChainHeadFollower`]. pub fn new( @@ -96,9 +132,7 @@ impl, Block: BlockT, Client> ChainHeadFollower Date: Wed, 2 Oct 2024 19:17:05 +0300 Subject: [PATCH 12/19] chainHead: Use LRU::peak instead of get to not change order Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/chain_head/chain_head_follow.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs index 5963a1228581..a09b5e4ab909 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs @@ -108,7 +108,10 @@ impl AnnouncedBlocks { /// Check if the block was previously announced. fn was_announced(&mut self, block: &Block::Hash) -> bool { - self.blocks.get(block).is_some() || self.finalized.get(block).is_some() + // For the finalized blocks we use `peek` to avoid moving the block counter to the front. + // This effectively means that the LRU acts as a FIFO queue. Otherwise, we might + // end up with scenarios where the last inserted finalized block is removed. + self.blocks.get(block).is_some() || self.finalized.peek(block).is_some() } } From 45f2bce5efe58496de30ed6f0d559e211ac378ba Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 2 Oct 2024 19:37:15 +0300 Subject: [PATCH 13/19] chainHead: Extra sanity check for subscription pinning and races Signed-off-by: Alexandru Vasile --- .../src/chain_head/chain_head_follow.rs | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs index a09b5e4ab909..f469ad8e5758 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs @@ -87,7 +87,11 @@ impl AnnouncedBlocks { /// Creates a new `AnnouncedBlocks`. fn new() -> Self { Self { + // The total number of pinned blocks is `MAX_PINNED_BLOCKS`, ensure we don't + // exceed the limit. blocks: LruMap::new(ByLength::new((MAX_PINNED_BLOCKS - MAX_FINALIZED_BLOCKS) as u32)), + // We are keeping a smaller number of announced finalized blocks in memory. + // This is because the `Finalized` event might be triggered before the `NewBlock` event. finalized: LruMap::new(ByLength::new(MAX_FINALIZED_BLOCKS as u32)), } } @@ -466,9 +470,6 @@ where ) -> Result>, SubscriptionManagementError> { let block_hash = notification.hash; - // Ensure the block is pinned before generating the events. - self.sub_handle.pin_block(&self.sub_id, block_hash)?; - // Ensure we are only reporting blocks after the starting point. if *notification.header.number() < startup_point.finalized_number { return Ok(Default::default()) @@ -486,6 +487,20 @@ where return Err(SubscriptionManagementError::Custom("Parent block was not reported".into())) } + // Ensure the block can be pinned before generating the events. + if !self.sub_handle.pin_block(&self.sub_id, block_hash)? { + // The block is already pinned, this is similar to the check above. + // + // The `SubscriptionManagement` ensures the block is tracked until (short lived): + // - 2 calls to `pin_block` are made (from `Finalized` and `NewBlock` branches). + // - the block is unpinned by the user + // + // This is rather a sanity checks for edge-cases (in theory), where + // [`MAX_FINALIZED_BLOCKS` + 1] finalized events are triggered before the `NewBlock` + // event of the first `Finalized` event. + return Ok(Default::default()) + } + self.announced_blocks.insert(block_hash, false); Ok(self.generate_import_events(block_hash, parent_block_hash, notification.is_new_best)) } From 28911be60fba1bb81dd76ae7ece4adc2dfc84e5f Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 2 Oct 2024 19:48:57 +0300 Subject: [PATCH 14/19] chainHead: Move `pin_block` logic above Signed-off-by: Alexandru Vasile --- .../src/chain_head/chain_head_follow.rs | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs index f469ad8e5758..ef2e07495120 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs @@ -475,18 +475,6 @@ where return Ok(Default::default()) } - if self.announced_blocks.was_announced(&block_hash) { - // Block was already reported by the finalized branch. - return Ok(Default::default()) - } - - // Double check the parent hash. If the parent hash is not reported, we have a gap. - let parent_block_hash = *notification.header.parent_hash(); - if !self.announced_blocks.was_announced(&parent_block_hash) { - // The parent block was not reported, we have a gap. - return Err(SubscriptionManagementError::Custom("Parent block was not reported".into())) - } - // Ensure the block can be pinned before generating the events. if !self.sub_handle.pin_block(&self.sub_id, block_hash)? { // The block is already pinned, this is similar to the check above. @@ -501,6 +489,18 @@ where return Ok(Default::default()) } + if self.announced_blocks.was_announced(&block_hash) { + // Block was already reported by the finalized branch. + return Ok(Default::default()) + } + + // Double check the parent hash. If the parent hash is not reported, we have a gap. + let parent_block_hash = *notification.header.parent_hash(); + if !self.announced_blocks.was_announced(&parent_block_hash) { + // The parent block was not reported, we have a gap. + return Err(SubscriptionManagementError::Custom("Parent block was not reported".into())) + } + self.announced_blocks.insert(block_hash, false); Ok(self.generate_import_events(block_hash, parent_block_hash, notification.is_new_best)) } From 1d5af1a82831100c84c3b81ada8ae3c03b04f4af Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 3 Oct 2024 16:44:15 +0300 Subject: [PATCH 15/19] chainHead: Add most recently finalized blocks wrapper Signed-off-by: Alexandru Vasile --- .../src/chain_head/chain_head_follow.rs | 31 ++++++++++++++----- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs index ef2e07495120..af3b74983472 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs @@ -80,7 +80,25 @@ struct AnnouncedBlocks { /// Unfinalized blocks. blocks: LruMap, /// Finalized blocks. - finalized: LruMap, + finalized: MostRecentFinalizedBlocks, +} + +/// Wrapper over LRU to efficiently lookup hashes and remove elements as FIFO queue. +struct MostRecentFinalizedBlocks(LruMap); + +impl MostRecentFinalizedBlocks { + /// Insert the finalized block hash into the LRU cache. + fn insert(&mut self, block: Block::Hash) { + self.0.insert(block, ()); + } + + /// Check if the block is contained in the LRU cache. + fn contains(&mut self, block: &Block::Hash) -> Option<&()> { + // For the finalized blocks we use `peek` to avoid moving the block counter to the front. + // This effectively means that the LRU acts as a FIFO queue. Otherwise, we might + // end up with scenarios where the last inserted finalized block is removed. + self.0.peek(block) + } } impl AnnouncedBlocks { @@ -92,7 +110,9 @@ impl AnnouncedBlocks { blocks: LruMap::new(ByLength::new((MAX_PINNED_BLOCKS - MAX_FINALIZED_BLOCKS) as u32)), // We are keeping a smaller number of announced finalized blocks in memory. // This is because the `Finalized` event might be triggered before the `NewBlock` event. - finalized: LruMap::new(ByLength::new(MAX_FINALIZED_BLOCKS as u32)), + finalized: MostRecentFinalizedBlocks(LruMap::new(ByLength::new( + MAX_FINALIZED_BLOCKS as u32, + ))), } } @@ -104,7 +124,7 @@ impl AnnouncedBlocks { // Given that the finalized blocks are bounded to `MAX_FINALIZED_BLOCKS`, // this ensures we keep the minimum number of blocks in memory. self.blocks.remove(&block); - self.finalized.insert(block, ()); + self.finalized.insert(block); } else { self.blocks.insert(block, ()); } @@ -112,10 +132,7 @@ impl AnnouncedBlocks { /// Check if the block was previously announced. fn was_announced(&mut self, block: &Block::Hash) -> bool { - // For the finalized blocks we use `peek` to avoid moving the block counter to the front. - // This effectively means that the LRU acts as a FIFO queue. Otherwise, we might - // end up with scenarios where the last inserted finalized block is removed. - self.blocks.get(block).is_some() || self.finalized.peek(block).is_some() + self.blocks.get(block).is_some() || self.finalized.contains(block).is_some() } } From 308d388606ff27e68b9cfc265491bf0d9fa83813 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 3 Oct 2024 16:45:20 +0300 Subject: [PATCH 16/19] chainHead: Apply suggestion Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/chain_head/chain_head_follow.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs index af3b74983472..b51e444b443b 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs @@ -96,7 +96,9 @@ impl MostRecentFinalizedBlocks { fn contains(&mut self, block: &Block::Hash) -> Option<&()> { // For the finalized blocks we use `peek` to avoid moving the block counter to the front. // This effectively means that the LRU acts as a FIFO queue. Otherwise, we might - // end up with scenarios where the last inserted finalized block is removed. + // end up with scenarios where the "finalized block" in the end of LRU is overwritten which + // may not necessarily be the oldest finalized block i.e, possible that "get" promotes an + // older finalized block because it was accessed more recently. self.0.peek(block) } } From a05ae723334b62ac9382e6c06224b691b9d9b476 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 4 Oct 2024 13:08:47 +0300 Subject: [PATCH 17/19] chainHead: Move comment to the top of the struct Signed-off-by: Alexandru Vasile --- .../rpc-spec-v2/src/chain_head/chain_head_follow.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs index b51e444b443b..f2326f015677 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs @@ -84,6 +84,12 @@ struct AnnouncedBlocks { } /// Wrapper over LRU to efficiently lookup hashes and remove elements as FIFO queue. +/// +/// For the finalized blocks we use `peek` to avoid moving the block counter to the front. +/// This effectively means that the LRU acts as a FIFO queue. Otherwise, we might +/// end up with scenarios where the "finalized block" in the end of LRU is overwritten which +/// may not necessarily be the oldest finalized block i.e, possible that "get" promotes an +/// older finalized block because it was accessed more recently. struct MostRecentFinalizedBlocks(LruMap); impl MostRecentFinalizedBlocks { @@ -94,11 +100,6 @@ impl MostRecentFinalizedBlocks { /// Check if the block is contained in the LRU cache. fn contains(&mut self, block: &Block::Hash) -> Option<&()> { - // For the finalized blocks we use `peek` to avoid moving the block counter to the front. - // This effectively means that the LRU acts as a FIFO queue. Otherwise, we might - // end up with scenarios where the "finalized block" in the end of LRU is overwritten which - // may not necessarily be the oldest finalized block i.e, possible that "get" promotes an - // older finalized block because it was accessed more recently. self.0.peek(block) } } From 5135e898faeeffcd612a4706a4e68b916a33b6d5 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 4 Oct 2024 14:51:40 +0300 Subject: [PATCH 18/19] chainHead/tests: Adjust interface to latest origin/master Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/chain_head/tests.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/tests.rs b/substrate/client/rpc-spec-v2/src/chain_head/tests.rs index 72f4484d98f1..3b4201c38bd6 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/tests.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/tests.rs @@ -3975,16 +3975,15 @@ async fn follow_event_with_unknown_parent() { let client_mock = Arc::new(ChainHeadMockClient::new(client.clone())); let api = ChainHead::new( - client_mock.clone(), + client.clone(), backend, - Arc::new(TaskExecutor::default()), + Arc::new(TokioTestExecutor::default()), ChainHeadConfig { global_max_pinned_blocks: MAX_PINNED_BLOCKS, subscription_max_pinned_duration: Duration::from_secs(MAX_PINNED_SECS), subscription_max_ongoing_operations: MAX_OPERATIONS, - operation_max_storage_items: MAX_PAGINATION_LIMIT, - max_lagging_distance: MAX_LAGGING_DISTANCE, max_follow_subscriptions_per_connection: MAX_FOLLOW_SUBSCRIPTIONS_PER_CONNECTION, + max_lagging_distance: MAX_LAGGING_DISTANCE, }, ) .into_rpc(); From a5bf18e7b361c6095dc754dc2b08522455664128 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 4 Oct 2024 19:18:47 +0300 Subject: [PATCH 19/19] chainHead/tests: Use mock client instead of client Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/chain_head/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/tests.rs b/substrate/client/rpc-spec-v2/src/chain_head/tests.rs index 3b4201c38bd6..0c2486157bd0 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/tests.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/tests.rs @@ -3975,7 +3975,7 @@ async fn follow_event_with_unknown_parent() { let client_mock = Arc::new(ChainHeadMockClient::new(client.clone())); let api = ChainHead::new( - client.clone(), + client_mock.clone(), backend, Arc::new(TokioTestExecutor::default()), ChainHeadConfig {