From 9bec9834f5c7d4db4647cd1e33db6e0dd9fe4b0b Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 29 Aug 2024 18:33:39 +0300 Subject: [PATCH 01/13] chainHead/fix: Report bestBlock events only for newBlock reports Signed-off-by: Alexandru Vasile --- .../src/chain_head/chain_head_follow.rs | 97 +++++++++---------- 1 file changed, 48 insertions(+), 49 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 1d28d2071248..539cb96910ae 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 @@ -428,9 +428,12 @@ where /// Generates new block events from the given finalized hashes. /// /// It may be possible that the `Finalized` event fired before the `NewBlock` - /// event. In that case, for each finalized hash that was not reported yet - /// generate the `NewBlock` event. For the final finalized hash we must also - /// generate one `BestBlock` event. + /// event. Only in that case we generate: + /// - `NewBlock` event for all finalized hashes. + /// - `BestBlock` event for the last finalized hash. + /// + /// This function returns an empty list if all finalized hashes were already reported + /// and are pinned. fn generate_finalized_events( &mut self, finalized_block_hashes: &[Block::Hash], @@ -454,34 +457,33 @@ where } // Generate `NewBlock` events for all blocks beside the last block in the list - if i + 1 != finalized_block_hashes.len() { + let is_last = i + 1 == finalized_block_hashes.len(); + if !is_last { // Generate only the `NewBlock` event for this block. events.extend(self.generate_import_events(*hash, *parent, false)); - } else { - // If we end up here and the `best_block` is a descendent of the finalized block - // (last block in the list), it means that there were skipped notifications. - // Otherwise `pin_block` would had returned `true`. - // - // When the node falls out of sync and then syncs up to the tip of the chain, it can - // happen that we skip notifications. Then it is better to terminate the connection - // instead of trying to send notifications for all missed blocks. - if let Some(best_block_hash) = self.current_best_block { - let ancestor = sp_blockchain::lowest_common_ancestor( - &*self.client, - *hash, - best_block_hash, - )?; - - if ancestor.hash == *hash { - return Err(SubscriptionManagementError::Custom( - "A descendent of the finalized block was already reported".into(), - )) - } - } + continue; + } - // Let's generate the `NewBlock` and `NewBestBlock` events for the block. - events.extend(self.generate_import_events(*hash, *parent, true)) + // If we end up here and the `best_block` is a descendent of the finalized block + // (last block in the list), it means that there were skipped notifications. + // Otherwise `pin_block` would had returned `true`. + // + // When the node falls out of sync and then syncs up to the tip of the chain, it can + // happen that we skip notifications. Then it is better to terminate the connection + // instead of trying to send notifications for all missed blocks. + if let Some(best_block_hash) = self.current_best_block { + let ancestor = + sp_blockchain::lowest_common_ancestor(&*self.client, *hash, best_block_hash)?; + + if ancestor.hash == *hash { + return Err(SubscriptionManagementError::Custom( + "A descendent of the finalized block was already reported".into(), + )) + } } + + // Let's generate the `NewBlock` and `NewBestBlock` events for the block. + events.extend(self.generate_import_events(*hash, *parent, true)) } Ok(events) @@ -552,37 +554,34 @@ where // The best reported block is in the pruned list. Report a new best block. let is_in_pruned_list = pruned_block_hashes.iter().any(|hash| *hash == current_best_block); - // The block is not the last finalized block. + + // The best reported block is not the last finalized block. // // It can be either: // - a descendant of the last finalized block // - a block on a fork that will be pruned in the future. // // In those cases, we emit a new best block. + // + // When [`Self::generate_finalized_events`] reports the last finalized block as the best + // block, it also sets the `self.current_best_block`, if and only if the block + // was never reported as `NewBlock` before. + // + // Therefore, this condition is equivalent with + // `!std::matches(FollowEvent::BestBlockChanged(_), events.last())`. + // And that means we know for sure that at least a `NewBlock` was generated + // for the last finalized block. let is_not_last_finalized = current_best_block != last_finalized; if is_in_pruned_list || is_not_last_finalized { - // We need to generate a best block event. - let best_block_hash = self.client.info().best_hash; - - // Defensive check against state missmatch. - if best_block_hash == current_best_block { - // The client doest not have any new information about the best block. - // The information from `.info()` is updated from the DB as the last - // step of the finalization and it should be up to date. - // If the info is outdated, there is nothing the RPC can do for now. - debug!( - target: LOG_TARGET, - "[follow][id={:?}] Client does not contain different best block", - self.sub_id, - ); - } else { - // The RPC needs to also submit a new best block changed before the - // finalized event. - self.current_best_block = Some(best_block_hash); - events - .push(FollowEvent::BestBlockChanged(BestBlockChanged { best_block_hash })); - } + // It is ok to report the `BestBlockChanged` multiple times. However it is not + // allowed to report the `BestBlockChanged` before a `NewBlock` event. Therefore + // we use the last finalized as the best block, instead of `client.info()` that + // may contain newer information. + self.current_best_block = Some(last_finalized); + events.push(FollowEvent::BestBlockChanged(BestBlockChanged { + best_block_hash: last_finalized, + })); } } From cc9524bc8561fbd4eab374d68f52c98c67d23a51 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 29 Aug 2024 18:33:59 +0300 Subject: [PATCH 02/13] chainHead/tests: Check BestBlock is reported on edge-cases by known blocks Signed-off-by: Alexandru Vasile --- .../rpc-spec-v2/src/chain_head/tests.rs | 235 ++++++++++++++++++ 1 file changed, 235 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 38f091471f87..5f8c25691e17 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/tests.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/tests.rs @@ -3833,3 +3833,238 @@ async fn follow_unique_pruned_blocks() { }); assert_eq!(event, expected); } + +#[tokio::test] +async fn follow_report_best_block_of_a_known_block() { + let builder = TestClientBuilder::new(); + let backend = builder.backend(); + let client = Arc::new(builder.build()); + + let api = ChainHead::new( + client.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 -> block 1 -> block 2 + // ^^^ best block reported + // + // -> block 1 -> block 2_f -> block 4 -> block 5 (best) + // ^^^ finalized + // + // The block 4 is needed on the longest chain because we want the + // best block 2 to be reported as pruned. Pruning is happening at + // height (N - 1), where N is the finalized block number. + + 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_f = BlockBuilderBuilder::new(&*client) + .on_parent_block(block_1_hash) + .with_parent_block_number(1) + .build() + .unwrap() + .build() + .unwrap() + .block; + let block_2_f_hash = block_2_f.hash(); + client.import(BlockOrigin::Own, block_2_f.clone()).await.unwrap(); + + let block_4 = BlockBuilderBuilder::new(&*client) + .on_parent_block(block_2_f_hash) + .with_parent_block_number(2) + .build() + .unwrap() + .build() + .unwrap() + .block; + let block_4_hash = block_4.hash(); + client.import(BlockOrigin::Own, block_4.clone()).await.unwrap(); + + // Import block 2 as best on the fork. + let mut block_builder = BlockBuilderBuilder::new(&*client) + .on_parent_block(block_1_hash) + .with_parent_block_number(1) + .build() + .unwrap(); + // This push is required as otherwise block 3 has the same hash as block 2 and won't get + // imported + block_builder + .push_transfer(Transfer { + from: AccountKeyring::Alice.into(), + to: AccountKeyring::Ferdie.into(), + amount: 41, + nonce: 0, + }) + .unwrap(); + let block_2 = block_builder.build().unwrap().block; + let block_2_hash = block_2.header.hash(); + client.import_as_best(BlockOrigin::Own, block_2.clone()).await.unwrap(); + + // Check block 1. + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::NewBlock(NewBlock { + block_hash: format!("{:?}", block_1_hash), + parent_block_hash: format!("{:?}", finalized_hash), + new_runtime: None, + with_runtime: false, + }); + assert_eq!(event, expected); + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::BestBlockChanged(BestBlockChanged { + best_block_hash: format!("{:?}", block_1_hash), + }); + assert_eq!(event, expected); + + // Check block 2. + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::NewBlock(NewBlock { + block_hash: format!("{:?}", block_2_f_hash), + parent_block_hash: format!("{:?}", block_1_hash), + new_runtime: None, + with_runtime: false, + }); + assert_eq!(event, expected); + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::BestBlockChanged(BestBlockChanged { + best_block_hash: format!("{:?}", block_2_f_hash), + }); + assert_eq!(event, expected); + + // Check block 4. + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::NewBlock(NewBlock { + block_hash: format!("{:?}", block_4_hash), + parent_block_hash: format!("{:?}", block_2_f_hash), + new_runtime: None, + with_runtime: false, + }); + assert_eq!(event, expected); + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::BestBlockChanged(BestBlockChanged { + best_block_hash: format!("{:?}", block_4_hash), + }); + assert_eq!(event, expected); + + // Check block 2, that we imported as custom best. + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::NewBlock(NewBlock { + block_hash: format!("{:?}", block_2_hash), + parent_block_hash: format!("{:?}", block_1_hash), + new_runtime: None, + with_runtime: false, + }); + assert_eq!(event, expected); + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::BestBlockChanged(BestBlockChanged { + best_block_hash: format!("{:?}", block_2_hash), + }); + assert_eq!(event, expected); + + // Import block 5 as best. + let block_5 = BlockBuilderBuilder::new(&*client) + .on_parent_block(block_4_hash) + .with_parent_block_number(3) + .build() + .unwrap() + .build() + .unwrap() + .block; + let block_5_hash = block_5.hash(); + client.import_as_best(BlockOrigin::Own, block_5.clone()).await.unwrap(); + + // Check block 5. + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::NewBlock(NewBlock { + block_hash: format!("{:?}", block_5_hash), + parent_block_hash: format!("{:?}", block_4_hash), + new_runtime: None, + with_runtime: false, + }); + assert_eq!(event, expected); + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::BestBlockChanged(BestBlockChanged { + best_block_hash: format!("{:?}", block_5_hash), + }); + assert_eq!(event, expected); + + // Finalize the block 4 from the fork. + client.finalize_block(block_4_hash, None).unwrap(); + + // Expect to report the best block changed before the finalized event. + // This does not rely on the `generate_finalized_events`, that will + // not generate a new `BestBlock` event. Instead this is captured by the + // last condition of `handle_finalized_blocks`. + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::BestBlockChanged(BestBlockChanged { + best_block_hash: format!("{:?}", block_4_hash), + }); + assert_eq!(event, expected); + + // Block 2 must be reported as pruned, even if it was the previous best. + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::Finalized(Finalized { + finalized_block_hashes: vec![ + format!("{:?}", block_1_hash), + format!("{:?}", block_2_f_hash), + format!("{:?}", block_4_hash), + ], + pruned_block_hashes: vec![format!("{:?}", block_2_hash)], + }); + assert_eq!(event, expected); + + // Pruned hash can be unpinned. + let sub_id = sub.subscription_id(); + let sub_id = serde_json::to_string(&sub_id).unwrap(); + let hash = format!("{:?}", block_2_hash); + let _res: () = api.call("chainHead_v1_unpin", rpc_params![&sub_id, &hash]).await.unwrap(); + + // Finalize the block 5. + client.finalize_block(block_5_hash, None).unwrap(); + // Block 5 was reported as best in the past. It is ok to report this + // multiple time as an optimization to not calculate the tree routes. + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::BestBlockChanged(BestBlockChanged { + best_block_hash: format!("{:?}", block_5_hash), + }); + assert_eq!(event, expected); + + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::Finalized(Finalized { + finalized_block_hashes: vec![format!("{:?}", block_5_hash)], + pruned_block_hashes: vec![], + }); + assert_eq!(event, expected); +} From d57578f3f129507bd9da50f973219185f256c58f Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 29 Aug 2024 18:54:07 +0300 Subject: [PATCH 03/13] Add PRdoc Signed-off-by: Alexandru Vasile --- prdoc/pr_5527.prdoc | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 prdoc/pr_5527.prdoc diff --git a/prdoc/pr_5527.prdoc b/prdoc/pr_5527.prdoc new file mode 100644 index 000000000000..5b74e7274ba5 --- /dev/null +++ b/prdoc/pr_5527.prdoc @@ -0,0 +1,21 @@ +# 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: Report bestBlock events only for newBlock reports + +doc: + - audience: Node Dev + description: | + This PR ensures that the chainHead_v1_follow method of the RPC-v2 API are always + reporting a `BestBlock` event after a `NewBlock`. + There was a race condition in the chainHead follow logic which led to the `BestBlock` + event to be emitted without an associated `NewBlock` event. + Developers should note that multiple `BestBlock` events can be emitted for the same + block hash, as per spec. For example, this may be because a block that was considered best is now part of + a pruned fork, or that the chainHead logic decided to optimize tree route searches and + relied on this aspect of the RPC-v2 spec. + +crates: + - name: sc-rpc-spec-v2 + bump: minor + From 5d30d7e85af8fb483c51265fa435d5351371d033 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 30 Aug 2024 11:49:45 +0000 Subject: [PATCH 04/13] chainHead/follow: Move comment to the right section Signed-off-by: Alexandru Vasile --- .../src/chain_head/chain_head_follow.rs | 14 +++++++------- 1 file changed, 7 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 539cb96910ae..ed8e084ae1b1 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 @@ -464,17 +464,17 @@ where continue; } - // If we end up here and the `best_block` is a descendent of the finalized block - // (last block in the list), it means that there were skipped notifications. - // Otherwise `pin_block` would had returned `true`. - // - // When the node falls out of sync and then syncs up to the tip of the chain, it can - // happen that we skip notifications. Then it is better to terminate the connection - // instead of trying to send notifications for all missed blocks. if let Some(best_block_hash) = self.current_best_block { let ancestor = sp_blockchain::lowest_common_ancestor(&*self.client, *hash, best_block_hash)?; + // If we end up here and the `best_block` is a descendent of the finalized block + // (last block in the list), it means that there were skipped notifications. + // Otherwise `pin_block` would had returned `false`. + // + // When the node falls out of sync and then syncs up to the tip of the chain, it can + // happen that we skip notifications. Then it is better to terminate the connection + // instead of trying to send notifications for all missed blocks. if ancestor.hash == *hash { return Err(SubscriptionManagementError::Custom( "A descendent of the finalized block was already reported".into(), From e42f10479d76de943ac496f8b6ffeb83fb7dfc37 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 30 Aug 2024 13:30:55 +0000 Subject: [PATCH 05/13] chainHead/follow: Generate new event only if not descendant Signed-off-by: Alexandru Vasile --- .../src/chain_head/chain_head_follow.rs | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 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 ed8e084ae1b1..a1e746b07a8a 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 @@ -551,37 +551,37 @@ where }); if let Some(current_best_block) = self.current_best_block { - // The best reported block is in the pruned list. Report a new best block. + // There are two cases when the best block is on fork: + // - The best block is in the pruned list and is reported immediately + // - The best block is on another fork that will be pruned in the future. + // + // For both cases, we need to report a new best block. + + // Case 1. The best reported block is in the pruned list. let is_in_pruned_list = pruned_block_hashes.iter().any(|hash| *hash == current_best_block); - - // The best reported block is not the last finalized block. - // - // It can be either: - // - a descendant of the last finalized block - // - a block on a fork that will be pruned in the future. - // - // In those cases, we emit a new best block. - // - // When [`Self::generate_finalized_events`] reports the last finalized block as the best - // block, it also sets the `self.current_best_block`, if and only if the block - // was never reported as `NewBlock` before. - // - // Therefore, this condition is equivalent with - // `!std::matches(FollowEvent::BestBlockChanged(_), events.last())`. - // And that means we know for sure that at least a `NewBlock` was generated - // for the last finalized block. - let is_not_last_finalized = current_best_block != last_finalized; - - if is_in_pruned_list || is_not_last_finalized { - // It is ok to report the `BestBlockChanged` multiple times. However it is not - // allowed to report the `BestBlockChanged` before a `NewBlock` event. Therefore - // we use the last finalized as the best block, instead of `client.info()` that - // may contain newer information. + if is_in_pruned_list { self.current_best_block = Some(last_finalized); events.push(FollowEvent::BestBlockChanged(BestBlockChanged { best_block_hash: last_finalized, })); + } else { + // Case 2. Check if the best block is a descendant of the last finalized block. + let ancestor = sp_blockchain::lowest_common_ancestor( + &*self.client, + last_finalized, + current_best_block, + )?; + + let is_descendant = ancestor.hash == last_finalized; + // If the best block is not a descendant of the last finalized block, it means that + // the best block is on a fork, that will be pruned in the future. + if !is_descendant { + self.current_best_block = Some(last_finalized); + events.push(FollowEvent::BestBlockChanged(BestBlockChanged { + best_block_hash: last_finalized, + })); + } } } From cf73041e4bf4d52cdb40b97e7c1eef1560da2099 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 30 Aug 2024 14:01:37 +0000 Subject: [PATCH 06/13] chainHead/tests: Adjust to lowest_common_ancestor Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/chain_head/tests.rs | 18 +----------------- 1 file changed, 1 insertion(+), 17 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 5f8c25691e17..16c647bc8734 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/tests.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/tests.rs @@ -4023,16 +4023,7 @@ async fn follow_report_best_block_of_a_known_block() { // Finalize the block 4 from the fork. client.finalize_block(block_4_hash, None).unwrap(); - // Expect to report the best block changed before the finalized event. - // This does not rely on the `generate_finalized_events`, that will - // not generate a new `BestBlock` event. Instead this is captured by the - // last condition of `handle_finalized_blocks`. - let event: FollowEvent = get_next_event(&mut sub).await; - let expected = FollowEvent::BestBlockChanged(BestBlockChanged { - best_block_hash: format!("{:?}", block_4_hash), - }); - assert_eq!(event, expected); - + // Block 5 is already reported as best block. // Block 2 must be reported as pruned, even if it was the previous best. let event: FollowEvent = get_next_event(&mut sub).await; let expected = FollowEvent::Finalized(Finalized { @@ -4053,13 +4044,6 @@ async fn follow_report_best_block_of_a_known_block() { // Finalize the block 5. client.finalize_block(block_5_hash, None).unwrap(); - // Block 5 was reported as best in the past. It is ok to report this - // multiple time as an optimization to not calculate the tree routes. - let event: FollowEvent = get_next_event(&mut sub).await; - let expected = FollowEvent::BestBlockChanged(BestBlockChanged { - best_block_hash: format!("{:?}", block_5_hash), - }); - assert_eq!(event, expected); let event: FollowEvent = get_next_event(&mut sub).await; let expected = FollowEvent::Finalized(Finalized { From 020adee286d0118fc131cdbb71defa06c5446756 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 30 Aug 2024 14:05:13 +0000 Subject: [PATCH 07/13] Update prdoc Signed-off-by: Alexandru Vasile --- prdoc/pr_5527.prdoc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/prdoc/pr_5527.prdoc b/prdoc/pr_5527.prdoc index 5b74e7274ba5..847e38a83c83 100644 --- a/prdoc/pr_5527.prdoc +++ b/prdoc/pr_5527.prdoc @@ -10,10 +10,6 @@ doc: reporting a `BestBlock` event after a `NewBlock`. There was a race condition in the chainHead follow logic which led to the `BestBlock` event to be emitted without an associated `NewBlock` event. - Developers should note that multiple `BestBlock` events can be emitted for the same - block hash, as per spec. For example, this may be because a block that was considered best is now part of - a pruned fork, or that the chainHead logic decided to optimize tree route searches and - relied on this aspect of the RPC-v2 spec. crates: - name: sc-rpc-spec-v2 From 8a28364db6bb067c3e5075d773f8cef398b52fb0 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Mon, 2 Sep 2024 15:46:15 +0300 Subject: [PATCH 08/13] Update prdoc/pr_5527.prdoc Co-authored-by: Sebastian Kunert --- prdoc/pr_5527.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_5527.prdoc b/prdoc/pr_5527.prdoc index 847e38a83c83..94dfa1d30c03 100644 --- a/prdoc/pr_5527.prdoc +++ b/prdoc/pr_5527.prdoc @@ -6,7 +6,7 @@ title: Report bestBlock events only for newBlock reports doc: - audience: Node Dev description: | - This PR ensures that the chainHead_v1_follow method of the RPC-v2 API are always + This PR ensures that the chainHead_v1_follow method of the RPC-v2 API is always reporting a `BestBlock` event after a `NewBlock`. There was a race condition in the chainHead follow logic which led to the `BestBlock` event to be emitted without an associated `NewBlock` event. From b5bdb31fc4e4c314341cb150d6b06c5d93d3e57d Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Mon, 2 Sep 2024 15:46:21 +0300 Subject: [PATCH 09/13] Update prdoc/pr_5527.prdoc Co-authored-by: Sebastian Kunert --- prdoc/pr_5527.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_5527.prdoc b/prdoc/pr_5527.prdoc index 94dfa1d30c03..38eb75affe44 100644 --- a/prdoc/pr_5527.prdoc +++ b/prdoc/pr_5527.prdoc @@ -1,7 +1,7 @@ # 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: Report bestBlock events only for newBlock reports +title: Report BestBlock events only for newBlock reports doc: - audience: Node Dev From 46fe86b02b0d29f0abaa4a43bcc475e9dc35ad48 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 2 Sep 2024 12:54:52 +0000 Subject: [PATCH 10/13] chainHead/tests: Adjust testing to new prunning logic Signed-off-by: Alexandru Vasile --- .../rpc-spec-v2/src/chain_head/tests.rs | 67 +++++-------------- 1 file changed, 18 insertions(+), 49 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 16c647bc8734..b20ad27c83f6 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/tests.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/tests.rs @@ -3873,12 +3873,8 @@ async fn follow_report_best_block_of_a_known_block() { // finalized -> block 1 -> block 2 // ^^^ best block reported // - // -> block 1 -> block 2_f -> block 4 -> block 5 (best) - // ^^^ finalized - // - // The block 4 is needed on the longest chain because we want the - // best block 2 to be reported as pruned. Pruning is happening at - // height (N - 1), where N is the finalized block number. + // -> block 1 -> block 2_f -> block 3 (best) + // ^^^ finalized let block_1 = BlockBuilderBuilder::new(&*client) .on_parent_block(client.chain_info().genesis_hash) @@ -3902,17 +3898,6 @@ async fn follow_report_best_block_of_a_known_block() { let block_2_f_hash = block_2_f.hash(); client.import(BlockOrigin::Own, block_2_f.clone()).await.unwrap(); - let block_4 = BlockBuilderBuilder::new(&*client) - .on_parent_block(block_2_f_hash) - .with_parent_block_number(2) - .build() - .unwrap() - .build() - .unwrap() - .block; - let block_4_hash = block_4.hash(); - client.import(BlockOrigin::Own, block_4.clone()).await.unwrap(); - // Import block 2 as best on the fork. let mut block_builder = BlockBuilderBuilder::new(&*client) .on_parent_block(block_1_hash) @@ -3963,21 +3948,6 @@ async fn follow_report_best_block_of_a_known_block() { }); assert_eq!(event, expected); - // Check block 4. - let event: FollowEvent = get_next_event(&mut sub).await; - let expected = FollowEvent::NewBlock(NewBlock { - block_hash: format!("{:?}", block_4_hash), - parent_block_hash: format!("{:?}", block_2_f_hash), - new_runtime: None, - with_runtime: false, - }); - assert_eq!(event, expected); - let event: FollowEvent = get_next_event(&mut sub).await; - let expected = FollowEvent::BestBlockChanged(BestBlockChanged { - best_block_hash: format!("{:?}", block_4_hash), - }); - assert_eq!(event, expected); - // Check block 2, that we imported as custom best. let event: FollowEvent = get_next_event(&mut sub).await; let expected = FollowEvent::NewBlock(NewBlock { @@ -3993,44 +3963,43 @@ async fn follow_report_best_block_of_a_known_block() { }); assert_eq!(event, expected); - // Import block 5 as best. - let block_5 = BlockBuilderBuilder::new(&*client) - .on_parent_block(block_4_hash) - .with_parent_block_number(3) + // Import block 3 as best. + let block_3 = BlockBuilderBuilder::new(&*client) + .on_parent_block(block_2_f_hash) + .with_parent_block_number(2) .build() .unwrap() .build() .unwrap() .block; - let block_5_hash = block_5.hash(); - client.import_as_best(BlockOrigin::Own, block_5.clone()).await.unwrap(); + let block_3_hash = block_3.hash(); + client.import_as_best(BlockOrigin::Own, block_3.clone()).await.unwrap(); - // Check block 5. + // Check block 3. let event: FollowEvent = get_next_event(&mut sub).await; let expected = FollowEvent::NewBlock(NewBlock { - block_hash: format!("{:?}", block_5_hash), - parent_block_hash: format!("{:?}", block_4_hash), + block_hash: format!("{:?}", block_3_hash), + parent_block_hash: format!("{:?}", block_2_f_hash), new_runtime: None, with_runtime: false, }); assert_eq!(event, expected); let event: FollowEvent = get_next_event(&mut sub).await; let expected = FollowEvent::BestBlockChanged(BestBlockChanged { - best_block_hash: format!("{:?}", block_5_hash), + best_block_hash: format!("{:?}", block_3_hash), }); assert_eq!(event, expected); - // Finalize the block 4 from the fork. - client.finalize_block(block_4_hash, None).unwrap(); + // Finalize the block 2 from the fork. + client.finalize_block(block_2_f_hash, None).unwrap(); - // Block 5 is already reported as best block. + // Block 3 is already reported as best block. // Block 2 must be reported as pruned, even if it was the previous best. let event: FollowEvent = get_next_event(&mut sub).await; let expected = FollowEvent::Finalized(Finalized { finalized_block_hashes: vec![ format!("{:?}", block_1_hash), format!("{:?}", block_2_f_hash), - format!("{:?}", block_4_hash), ], pruned_block_hashes: vec![format!("{:?}", block_2_hash)], }); @@ -4042,12 +4011,12 @@ async fn follow_report_best_block_of_a_known_block() { let hash = format!("{:?}", block_2_hash); let _res: () = api.call("chainHead_v1_unpin", rpc_params![&sub_id, &hash]).await.unwrap(); - // Finalize the block 5. - client.finalize_block(block_5_hash, None).unwrap(); + // Finalize the block 3. + client.finalize_block(block_3_hash, None).unwrap(); let event: FollowEvent = get_next_event(&mut sub).await; let expected = FollowEvent::Finalized(Finalized { - finalized_block_hashes: vec![format!("{:?}", block_5_hash)], + finalized_block_hashes: vec![format!("{:?}", block_3_hash)], pruned_block_hashes: vec![], }); assert_eq!(event, expected); From 92e94006740c2bdbd981263cf55ce7a0c15b1453 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 2 Sep 2024 13:56:30 +0000 Subject: [PATCH 11/13] chainHead/follow: Update comment wrt finalization and pruning Signed-off-by: Alexandru Vasile --- .../src/chain_head/chain_head_follow.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 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 a1e746b07a8a..ebb72ed3d156 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 @@ -551,13 +551,7 @@ where }); if let Some(current_best_block) = self.current_best_block { - // There are two cases when the best block is on fork: - // - The best block is in the pruned list and is reported immediately - // - The best block is on another fork that will be pruned in the future. - // - // For both cases, we need to report a new best block. - - // Case 1. The best reported block is in the pruned list. + // We need to generate a new best block if the best block is in the pruned list. let is_in_pruned_list = pruned_block_hashes.iter().any(|hash| *hash == current_best_block); if is_in_pruned_list { @@ -566,16 +560,18 @@ where best_block_hash: last_finalized, })); } else { - // Case 2. Check if the best block is a descendant of the last finalized block. + // The pruning logic ensures that when the finalized block is announced, + // all blocks on forks that have the common ancestor lower or equal + // to the finalized block are reported. + // + // However, we double check if the best block is a descendant of the last finalized + // block to ensure we don't miss any events. let ancestor = sp_blockchain::lowest_common_ancestor( &*self.client, last_finalized, current_best_block, )?; - let is_descendant = ancestor.hash == last_finalized; - // If the best block is not a descendant of the last finalized block, it means that - // the best block is on a fork, that will be pruned in the future. if !is_descendant { self.current_best_block = Some(last_finalized); events.push(FollowEvent::BestBlockChanged(BestBlockChanged { From 91c407d20e31f8d2f3da06b1c2eee4c988a137a8 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 2 Sep 2024 18:33:11 +0300 Subject: [PATCH 12/13] chainHead/tests: Adjust testing to reporduce client.info race Signed-off-by: Alexandru Vasile --- .../rpc-spec-v2/src/chain_head/test_utils.rs | 12 ++- .../rpc-spec-v2/src/chain_head/tests.rs | 73 +++++++++++++------ 2 files changed, 57 insertions(+), 28 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs b/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs index ab5be1f24e5d..fa10fde388f9 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs @@ -69,7 +69,7 @@ impl ChainHeadMockClient { } } - pub async fn trigger_finality_stream(&self, header: Header) { + pub async fn trigger_finality_stream(&self, header: Header, stale_heads: Vec) { // Ensure the client called the `finality_notification_stream`. while self.finality_sinks.lock().is_empty() { tokio::time::sleep(tokio::time::Duration::from_secs(1)).await; @@ -77,11 +77,8 @@ impl ChainHeadMockClient { // Build the notification. let (sink, _stream) = tracing_unbounded("test_sink", 100_000); - let summary = FinalizeSummary { - header: header.clone(), - finalized: vec![header.hash()], - stale_heads: vec![], - }; + let summary = + FinalizeSummary { header: header.clone(), finalized: vec![header.hash()], stale_heads }; let notification = FinalityNotification::from_summary(summary, sink); for sink in self.finality_sinks.lock().iter_mut() { @@ -346,7 +343,8 @@ where fn number( &self, hash: Block::Hash, - ) -> sc_client_api::blockchain::Result::Header as HeaderT>::Number>> { + ) -> sc_client_api::blockchain::Result::Header as HeaderT>::Number>> + { self.client.number(hash) } 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 b20ad27c83f6..a638a9c7ec54 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/tests.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/tests.rs @@ -2743,7 +2743,7 @@ async fn follow_finalized_before_new_block() { // expect for the `chainHead` to generate `NewBlock`, `BestBlock` and `Finalized` events. // Trigger the Finalized notification before the NewBlock one. - run_with_timeout(client_mock.trigger_finality_stream(block_1.header.clone())).await; + run_with_timeout(client_mock.trigger_finality_stream(block_1.header.clone(), vec![])).await; // Initialized must always be reported first. let finalized_hash = client.info().finalized_hash; @@ -3840,8 +3840,10 @@ async fn follow_report_best_block_of_a_known_block() { let backend = builder.backend(); let client = Arc::new(builder.build()); + let client_mock = Arc::new(ChainHeadMockClient::new(client.clone())); + let api = ChainHead::new( - client.clone(), + client_mock.clone(), backend, Arc::new(TaskExecutor::default()), ChainHeadConfig { @@ -3849,7 +3851,6 @@ async fn follow_report_best_block_of_a_known_block() { 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, }, @@ -3858,7 +3859,6 @@ async fn follow_report_best_block_of_a_known_block() { 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 { @@ -3886,7 +3886,6 @@ async fn follow_report_best_block_of_a_known_block() { .block; let block_1_hash = block_1.hash(); client.import(BlockOrigin::Own, block_1.clone()).await.unwrap(); - let block_2_f = BlockBuilderBuilder::new(&*client) .on_parent_block(block_1_hash) .with_parent_block_number(1) @@ -3918,6 +3917,10 @@ async fn follow_report_best_block_of_a_known_block() { let block_2_hash = block_2.header.hash(); client.import_as_best(BlockOrigin::Own, block_2.clone()).await.unwrap(); + run_with_timeout(client_mock.trigger_import_stream(block_1.header.clone())).await; + run_with_timeout(client_mock.trigger_import_stream(block_2_f.header.clone())).await; + run_with_timeout(client_mock.trigger_import_stream(block_2.header.clone())).await; + // Check block 1. let event: FollowEvent = get_next_event(&mut sub).await; let expected = FollowEvent::NewBlock(NewBlock { @@ -3963,7 +3966,7 @@ async fn follow_report_best_block_of_a_known_block() { }); assert_eq!(event, expected); - // Import block 3 as best. + // Craft block 3 and import it later to simulate a race condition. let block_3 = BlockBuilderBuilder::new(&*client) .on_parent_block(block_2_f_hash) .with_parent_block_number(2) @@ -3973,7 +3976,49 @@ async fn follow_report_best_block_of_a_known_block() { .unwrap() .block; let block_3_hash = block_3.hash(); + + // Set best block info to block 3, that is not announced yet. + // + // This simulates the following edge-case: + // - The client imports a new block as best block. + // - The finality stream is triggered before the block is announced. + // + // This generated in the past a `BestBlock` event for the block that was not announced + // by `NewBlock` events. + // + // This happened because the chainHead was using the `client.info()` without verifying + // if the block was announced or not. This was fixed by using the latest finalized + // block instead as fallback. For more info see: https://github.com/paritytech/polkadot-sdk/issues/5512. + client_mock.set_best_block(block_3_hash, 3); + + // Finalize the block 2 from the fork. + client.finalize_block(block_2_f_hash, None).unwrap(); + run_with_timeout( + client_mock.trigger_finality_stream(block_2_f.header.clone(), vec![block_2_hash]), + ) + .await; + + // Block 2f is now the best block, not the block 3 that is not announced yet. + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::BestBlockChanged(BestBlockChanged { + best_block_hash: format!("{:?}", block_2_f_hash), + }); + assert_eq!(event, expected); + // Block 2 must be reported as pruned, even if it was the previous best. + let event: FollowEvent = get_next_event(&mut sub).await; + let expected = FollowEvent::Finalized(Finalized { + finalized_block_hashes: vec![ + // Note: the client mock is only reporting one block at a time. + // format!("{:?}", block_1_hash), + format!("{:?}", block_2_f_hash), + ], + pruned_block_hashes: vec![format!("{:?}", block_2_hash)], + }); + assert_eq!(event, expected); + + // Block 3 is now imported as best. client.import_as_best(BlockOrigin::Own, block_3.clone()).await.unwrap(); + run_with_timeout(client_mock.trigger_import_stream(block_3.header.clone())).await; // Check block 3. let event: FollowEvent = get_next_event(&mut sub).await; @@ -3990,21 +4035,6 @@ async fn follow_report_best_block_of_a_known_block() { }); assert_eq!(event, expected); - // Finalize the block 2 from the fork. - client.finalize_block(block_2_f_hash, None).unwrap(); - - // Block 3 is already reported as best block. - // Block 2 must be reported as pruned, even if it was the previous best. - let event: FollowEvent = get_next_event(&mut sub).await; - let expected = FollowEvent::Finalized(Finalized { - finalized_block_hashes: vec![ - format!("{:?}", block_1_hash), - format!("{:?}", block_2_f_hash), - ], - pruned_block_hashes: vec![format!("{:?}", block_2_hash)], - }); - assert_eq!(event, expected); - // Pruned hash can be unpinned. let sub_id = sub.subscription_id(); let sub_id = serde_json::to_string(&sub_id).unwrap(); @@ -4013,6 +4043,7 @@ async fn follow_report_best_block_of_a_known_block() { // Finalize the block 3. client.finalize_block(block_3_hash, None).unwrap(); + run_with_timeout(client_mock.trigger_finality_stream(block_3.header.clone(), vec![])).await; let event: FollowEvent = get_next_event(&mut sub).await; let expected = FollowEvent::Finalized(Finalized { From 2298b72c5f3025e99768f69e81821d9e2055a1f1 Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Tue, 3 Sep 2024 10:52:49 +0000 Subject: [PATCH 13/13] ".git/.scripts/commands/fmt/fmt.sh" --- substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs b/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs index fa10fde388f9..073ee34a79f3 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs @@ -343,8 +343,7 @@ where fn number( &self, hash: Block::Hash, - ) -> sc_client_api::blockchain::Result::Header as HeaderT>::Number>> - { + ) -> sc_client_api::blockchain::Result::Header as HeaderT>::Number>> { self.client.number(hash) }