[stable2509] Backport #10525

prdoc/pr_10525.prdoc

@@ -0,0 +1,30 @@
+title: 'fix(rpc-spec-v2): best block not announced immediately after initialised'
+doc:
+- audience: Node Dev
+  description: |-
+    # Description
+
+    Fixes https://github.com/polkadot-api/polkadot-api/issues/1244
+
+    The current chainHead_v1 implementation is [not spec-compliant](https://paritytech.github.io/json-rpc-interface-spec/api/chainHead_v1_follow.html), as it states:
+
+    > - Generates an `initialized` notification
+    > - Generates one `newBlock` notification for each non-finalized block
+    > - Then a `bestBlockChanged` notification
+    > - When a new block arrives, generates a `newBlock` notification
+    > - When the node finalizes a block, generates a `finalized` notification
+
+    And the current implemention only emits the `bestBlockChanged` notification after initialized iif the best block is different from the finalized block.
+
+    PAPI recently is using this part of the spec as an assumption. Most chains are unaffected, but those that produce blocks on-demand (e.g. manual-seal) then have polkadot-api hanging until there's a higher block different than the finalized one.
+
+    ## Integration
+
+    This PR doesn't change any of the APIs of the node. Upgrade should be automatic.
+
+    ## Review Notes
+
+    This PR removes that condition so that the `bestBlockChanged` notification is always emited. All tests are updated to this new behaviour
+crates:
+- name: sc-rpc-spec-v2
+  bump: patch

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs

@@ -428,16 +428,14 @@ 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.was_announced(&best_block_hash) {
-				return Err(SubscriptionManagementError::BlockHeaderAbsent);
-			}
-			self.announced_blocks.insert(best_block_hash, true);
+		if !self.announced_blocks.was_announced(&best_block_hash) {
+			return Err(SubscriptionManagementError::BlockHeaderAbsent);
+		}
+		self.announced_blocks.insert(best_block_hash, true);
 
-			let best_block = FollowEvent::BestBlockChanged(BestBlockChanged { best_block_hash });
-			self.current_best_block = Some(best_block_hash);
-			finalized_block_descendants.push(best_block);
-		};
+		let best_block = FollowEvent::BestBlockChanged(BestBlockChanged { best_block_hash });
+		self.current_best_block = Some(best_block_hash);
+		finalized_block_descendants.push(best_block);
 
 		Ok(finalized_block_descendants)
 	}

substrate/client/rpc-spec-v2/src/chain_head/tests.rs

@@ -172,6 +172,10 @@ async fn setup_api() -> (
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::Initialized(_)
 	);
+	assert_matches!(
+		get_next_event::<FollowEvent<String>>(&mut sub).await,
+		FollowEvent::BestBlockChanged(_)
+	);
 	assert_matches!(
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::NewBlock(_)
@@ -273,6 +277,13 @@ async fn follow_subscription_produces_blocks() {
 	});
 	assert_eq!(event, expected);
 
+	// Then the best block
+	let event: FollowEvent<String> = get_next_event(&mut sub).await;
+	let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
+		best_block_hash: format!("{:?}", finalized_hash),
+	});
+	assert_eq!(event, expected);
+
 	let block = BlockBuilderBuilder::new(&*client)
 		.on_parent_block(client.chain_info().genesis_hash)
 		.with_parent_block_number(0)
@@ -357,6 +368,13 @@ async fn follow_with_runtime() {
 	});
 	pretty_assertions::assert_eq!(event, expected);
 
+	// Then the best block
+	let event: FollowEvent<String> = get_next_event(&mut sub).await;
+	let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
+		best_block_hash: format!("{:?}", finalized_hash),
+	});
+	pretty_assertions::assert_eq!(event, expected);
+
 	// Import a new block without runtime changes.
 	// The runtime field must be None in this case.
 	let block = BlockBuilderBuilder::new(&*client)
@@ -660,6 +678,10 @@ async fn call_runtime_without_flag() {
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::Initialized(_)
 	);
+	assert_matches!(
+		get_next_event::<FollowEvent<String>>(&mut sub).await,
+		FollowEvent::BestBlockChanged(_)
+	);
 	assert_matches!(
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::NewBlock(_)
@@ -1325,6 +1347,10 @@ async fn separate_operation_ids_for_subscriptions() {
 		get_next_event::<FollowEvent<String>>(&mut sub_first).await,
 		FollowEvent::Initialized(_)
 	);
+	assert_matches!(
+		get_next_event::<FollowEvent<String>>(&mut sub_first).await,
+		FollowEvent::BestBlockChanged(_)
+	);
 	assert_matches!(
 		get_next_event::<FollowEvent<String>>(&mut sub_first).await,
 		FollowEvent::NewBlock(_)
@@ -1338,6 +1364,10 @@ async fn separate_operation_ids_for_subscriptions() {
 		get_next_event::<FollowEvent<String>>(&mut sub_second).await,
 		FollowEvent::Initialized(_)
 	);
+	assert_matches!(
+		get_next_event::<FollowEvent<String>>(&mut sub_second).await,
+		FollowEvent::BestBlockChanged(_)
+	);
 	assert_matches!(
 		get_next_event::<FollowEvent<String>>(&mut sub_second).await,
 		FollowEvent::NewBlock(_)
@@ -1562,6 +1592,10 @@ async fn follow_exceeding_pinned_blocks() {
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::Initialized(_)
 	);
+	assert_matches!(
+		get_next_event::<FollowEvent<String>>(&mut sub).await,
+		FollowEvent::BestBlockChanged(_)
+	);
 	assert_matches!(
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::NewBlock(_)
@@ -1642,6 +1676,10 @@ async fn follow_with_unpin() {
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::Initialized(_)
 	);
+	assert_matches!(
+		get_next_event::<FollowEvent<String>>(&mut sub).await,
+		FollowEvent::BestBlockChanged(_)
+	);
 	assert_matches!(
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::NewBlock(_)
@@ -1749,6 +1787,10 @@ async fn unpin_duplicate_hashes() {
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::Initialized(_)
 	);
+	assert_matches!(
+		get_next_event::<FollowEvent<String>>(&mut sub).await,
+		FollowEvent::BestBlockChanged(_)
+	);
 	assert_matches!(
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::NewBlock(_)
@@ -1876,6 +1918,10 @@ async fn follow_with_multiple_unpin_hashes() {
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::Initialized(_)
 	);
+	assert_matches!(
+		get_next_event::<FollowEvent<String>>(&mut sub).await,
+		FollowEvent::BestBlockChanged(_)
+	);
 	assert_matches!(
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::NewBlock(_)
@@ -1991,6 +2037,13 @@ async fn follow_prune_best_block() {
 	});
 	assert_eq!(event, expected);
 
+	// Then the best block
+	let event: FollowEvent<String> = get_next_event(&mut sub).await;
+	let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
+		best_block_hash: format!("{:?}", finalized_hash),
+	});
+	assert_eq!(event, expected);
+
 	// Block tree:
 	//
 	// finalized -> block 1 -> block 2
@@ -2261,6 +2314,12 @@ async fn follow_forks_pruned_block() {
 	});
 	assert_eq!(event, expected);
 
+	let event: FollowEvent<String> = get_next_event(&mut sub).await;
+	let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
+		best_block_hash: format!("{:?}", block_3_hash),
+	});
+	assert_eq!(event, expected);
+
 	// Block tree:
 	//
 	// finalized -> block 1 -> block 2 -> block 3 -> block 4
@@ -2612,6 +2671,10 @@ async fn pin_block_references() {
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::Initialized(_)
 	);
+	assert_matches!(
+		get_next_event::<FollowEvent<String>>(&mut sub).await,
+		FollowEvent::BestBlockChanged(_)
+	);
 	assert_matches!(
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::NewBlock(_)
@@ -2845,6 +2908,10 @@ async fn ensure_operation_limits_works() {
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::Initialized(_)
 	);
+	assert_matches!(
+		get_next_event::<FollowEvent<String>>(&mut sub).await,
+		FollowEvent::BestBlockChanged(_)
+	);
 	assert_matches!(
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::NewBlock(_)
@@ -2956,6 +3023,10 @@ async fn storage_is_backpressured() {
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::Initialized(_)
 	);
+	assert_matches!(
+		get_next_event::<FollowEvent<String>>(&mut sub).await,
+		FollowEvent::BestBlockChanged(_)
+	);
 	assert_matches!(
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::NewBlock(_)
@@ -3091,6 +3162,10 @@ async fn stop_storage_operation() {
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::Initialized(_)
 	);
+	assert_matches!(
+		get_next_event::<FollowEvent<String>>(&mut sub).await,
+		FollowEvent::BestBlockChanged(_)
+	);
 	assert_matches!(
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::NewBlock(_)
@@ -3378,6 +3453,10 @@ async fn chain_head_stop_all_subscriptions() {
 		get_next_event::<FollowEvent<String>>(&mut sub).await,
 		FollowEvent::Initialized(_)
 	);
+	assert_matches!(
+		get_next_event::<FollowEvent<String>>(&mut sub).await,
+		FollowEvent::BestBlockChanged(_)
+	);
 
 	// Import 6 blocks in total to trigger the suspension distance.
 	let mut parent_hash = client.chain_info().genesis_hash;
@@ -3638,6 +3717,13 @@ async fn follow_unique_pruned_blocks() {
 	});
 	assert_eq!(event, expected);
 
+	// Then the best block
+	let event: FollowEvent<String> = get_next_event(&mut sub).await;
+	let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
+		best_block_hash: format!("{:?}", finalized_hash),
+	});
+	assert_eq!(event, expected);
+
 	// Block tree:
 	//
 	// finalized -> block 1 -> block 2 -> block 3
@@ -3807,6 +3893,13 @@ async fn follow_report_best_block_of_a_known_block() {
 	});
 	assert_eq!(event, expected);
 
+	// Then the best block
+	let event: FollowEvent<String> = get_next_event(&mut sub).await;
+	let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
+		best_block_hash: format!("{:?}", finalized_hash),
+	});
+	assert_eq!(event, expected);
+
 	// Block tree:
 	//
 	// finalized -> block 1 -> block 2
@@ -4026,6 +4119,13 @@ async fn follow_event_with_unknown_parent() {
 	});
 	assert_eq!(event, expected);
 
+	// Then the best block
+	let event: FollowEvent<String> = get_next_event(&mut sub).await;
+	let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
+		best_block_hash: format!("{:?}", finalized_hash),
+	});
+	assert_eq!(event, expected);
+
 	// Block tree:
 	//
 	// finalized -> (gap: block 1) -> block 2