From c37a4f59859160ee58c546191c3f5aefa63e86e2 Mon Sep 17 00:00:00 2001 From: Victor Oliva Date: Fri, 5 Dec 2025 11:58:57 +0100 Subject: [PATCH 1/2] fix(rpc-spec-v2): best block not announced immediately after initialised (#10525) # 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 # Checklist * [x] My PR includes a detailed description as outlined in the "Description" and its two subsections above. * [x] My PR follows the [labeling requirements]( https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process ) of this project (at minimum one label for `T` required) * External contributors: Use `/cmd label ` to add labels * Maintainers can also add labels manually * [x] I have made corresponding changes to the documentation (if applicable) * [x] I have added tests that prove my fix is effective or that my feature works (if applicable) --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> (cherry picked from commit 68c1250bbc3b28cfdec1e6effbaa91d9a8999b3d) --- prdoc/pr_10525.prdoc | 30 ++++++ .../src/chain_head/chain_head_follow.rs | 16 ++- .../rpc-spec-v2/src/chain_head/tests.rs | 100 ++++++++++++++++++ 3 files changed, 137 insertions(+), 9 deletions(-) create mode 100644 prdoc/pr_10525.prdoc diff --git a/prdoc/pr_10525.prdoc b/prdoc/pr_10525.prdoc new file mode 100644 index 0000000000000..97f92fd16d79e --- /dev/null +++ b/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: major 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 e9975b36b4a10..d188ddb5535a4 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,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) } 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 3ec5e805ecd5a..3307b5f628ba7 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/tests.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/tests.rs @@ -172,6 +172,10 @@ async fn setup_api() -> ( get_next_event::>(&mut sub).await, FollowEvent::Initialized(_) ); + assert_matches!( + get_next_event::>(&mut sub).await, + FollowEvent::BestBlockChanged(_) + ); assert_matches!( get_next_event::>(&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 = 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 = 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::>(&mut sub).await, FollowEvent::Initialized(_) ); + assert_matches!( + get_next_event::>(&mut sub).await, + FollowEvent::BestBlockChanged(_) + ); assert_matches!( get_next_event::>(&mut sub).await, FollowEvent::NewBlock(_) @@ -1325,6 +1347,10 @@ async fn separate_operation_ids_for_subscriptions() { get_next_event::>(&mut sub_first).await, FollowEvent::Initialized(_) ); + assert_matches!( + get_next_event::>(&mut sub_first).await, + FollowEvent::BestBlockChanged(_) + ); assert_matches!( get_next_event::>(&mut sub_first).await, FollowEvent::NewBlock(_) @@ -1338,6 +1364,10 @@ async fn separate_operation_ids_for_subscriptions() { get_next_event::>(&mut sub_second).await, FollowEvent::Initialized(_) ); + assert_matches!( + get_next_event::>(&mut sub_second).await, + FollowEvent::BestBlockChanged(_) + ); assert_matches!( get_next_event::>(&mut sub_second).await, FollowEvent::NewBlock(_) @@ -1562,6 +1592,10 @@ async fn follow_exceeding_pinned_blocks() { get_next_event::>(&mut sub).await, FollowEvent::Initialized(_) ); + assert_matches!( + get_next_event::>(&mut sub).await, + FollowEvent::BestBlockChanged(_) + ); assert_matches!( get_next_event::>(&mut sub).await, FollowEvent::NewBlock(_) @@ -1642,6 +1676,10 @@ async fn follow_with_unpin() { get_next_event::>(&mut sub).await, FollowEvent::Initialized(_) ); + assert_matches!( + get_next_event::>(&mut sub).await, + FollowEvent::BestBlockChanged(_) + ); assert_matches!( get_next_event::>(&mut sub).await, FollowEvent::NewBlock(_) @@ -1749,6 +1787,10 @@ async fn unpin_duplicate_hashes() { get_next_event::>(&mut sub).await, FollowEvent::Initialized(_) ); + assert_matches!( + get_next_event::>(&mut sub).await, + FollowEvent::BestBlockChanged(_) + ); assert_matches!( get_next_event::>(&mut sub).await, FollowEvent::NewBlock(_) @@ -1876,6 +1918,10 @@ async fn follow_with_multiple_unpin_hashes() { get_next_event::>(&mut sub).await, FollowEvent::Initialized(_) ); + assert_matches!( + get_next_event::>(&mut sub).await, + FollowEvent::BestBlockChanged(_) + ); assert_matches!( get_next_event::>(&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 = 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 = 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::>(&mut sub).await, FollowEvent::Initialized(_) ); + assert_matches!( + get_next_event::>(&mut sub).await, + FollowEvent::BestBlockChanged(_) + ); assert_matches!( get_next_event::>(&mut sub).await, FollowEvent::NewBlock(_) @@ -2845,6 +2908,10 @@ async fn ensure_operation_limits_works() { get_next_event::>(&mut sub).await, FollowEvent::Initialized(_) ); + assert_matches!( + get_next_event::>(&mut sub).await, + FollowEvent::BestBlockChanged(_) + ); assert_matches!( get_next_event::>(&mut sub).await, FollowEvent::NewBlock(_) @@ -2956,6 +3023,10 @@ async fn storage_is_backpressured() { get_next_event::>(&mut sub).await, FollowEvent::Initialized(_) ); + assert_matches!( + get_next_event::>(&mut sub).await, + FollowEvent::BestBlockChanged(_) + ); assert_matches!( get_next_event::>(&mut sub).await, FollowEvent::NewBlock(_) @@ -3091,6 +3162,10 @@ async fn stop_storage_operation() { get_next_event::>(&mut sub).await, FollowEvent::Initialized(_) ); + assert_matches!( + get_next_event::>(&mut sub).await, + FollowEvent::BestBlockChanged(_) + ); assert_matches!( get_next_event::>(&mut sub).await, FollowEvent::NewBlock(_) @@ -3378,6 +3453,10 @@ async fn chain_head_stop_all_subscriptions() { get_next_event::>(&mut sub).await, FollowEvent::Initialized(_) ); + assert_matches!( + get_next_event::>(&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 = 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 = 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 = 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 From 992ba68758398a4bbb989552dea3b43418c06550 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Tue, 9 Dec 2025 11:40:40 +0200 Subject: [PATCH 2/2] Apply suggestions from code review --- prdoc/pr_10525.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_10525.prdoc b/prdoc/pr_10525.prdoc index 97f92fd16d79e..734139d966bea 100644 --- a/prdoc/pr_10525.prdoc +++ b/prdoc/pr_10525.prdoc @@ -27,4 +27,4 @@ doc: 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: major + bump: patch