Skip to content

Commit b1181d4

Browse files
paritytech-release-backport-bot[bot]volivagithub-actions[bot]lexnvEgorPopelyaev
authored
[stable2509] Backport #10525 (#10555)
Backport #10525 into `stable2509` from voliva. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Co-authored-by: Victor Oliva <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Alexandru Vasile <[email protected]> Co-authored-by: Egor_P <[email protected]>
1 parent f53beff commit b1181d4

File tree

3 files changed

+137
-9
lines changed

3 files changed

+137
-9
lines changed

‎prdoc/pr_10525.prdoc‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
title: 'fix(rpc-spec-v2): best block not announced immediately after initialised'
2+
doc:
3+
- audience: Node Dev
4+
description: |-
5+
# Description
6+
7+
Fixes https://github.com/polkadot-api/polkadot-api/issues/1244
8+
9+
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:
10+
11+
> - Generates an `initialized` notification
12+
> - Generates one `newBlock` notification for each non-finalized block
13+
> - Then a `bestBlockChanged` notification
14+
> - When a new block arrives, generates a `newBlock` notification
15+
> - When the node finalizes a block, generates a `finalized` notification
16+
17+
And the current implemention only emits the `bestBlockChanged` notification after initialized iif the best block is different from the finalized block.
18+
19+
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.
20+
21+
## Integration
22+
23+
This PR doesn't change any of the APIs of the node. Upgrade should be automatic.
24+
25+
## Review Notes
26+
27+
This PR removes that condition so that the `bestBlockChanged` notification is always emited. All tests are updated to this new behaviour
28+
crates:
29+
- name: sc-rpc-spec-v2
30+
bump: patch

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

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -428,16 +428,14 @@ where
428428

429429
// Generate a new best block event.
430430
let best_block_hash = startup_point.best_hash;
431-
if best_block_hash != finalized_block_hash {
432-
if !self.announced_blocks.was_announced(&best_block_hash) {
433-
return Err(SubscriptionManagementError::BlockHeaderAbsent);
434-
}
435-
self.announced_blocks.insert(best_block_hash, true);
431+
if !self.announced_blocks.was_announced(&best_block_hash) {
432+
return Err(SubscriptionManagementError::BlockHeaderAbsent);
433+
}
434+
self.announced_blocks.insert(best_block_hash, true);
436435

437-
let best_block = FollowEvent::BestBlockChanged(BestBlockChanged { best_block_hash });
438-
self.current_best_block = Some(best_block_hash);
439-
finalized_block_descendants.push(best_block);
440-
};
436+
let best_block = FollowEvent::BestBlockChanged(BestBlockChanged { best_block_hash });
437+
self.current_best_block = Some(best_block_hash);
438+
finalized_block_descendants.push(best_block);
441439

442440
Ok(finalized_block_descendants)
443441
}

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

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,10 @@ async fn setup_api() -> (
172172
get_next_event::<FollowEvent<String>>(&mut sub).await,
173173
FollowEvent::Initialized(_)
174174
);
175+
assert_matches!(
176+
get_next_event::<FollowEvent<String>>(&mut sub).await,
177+
FollowEvent::BestBlockChanged(_)
178+
);
175179
assert_matches!(
176180
get_next_event::<FollowEvent<String>>(&mut sub).await,
177181
FollowEvent::NewBlock(_)
@@ -273,6 +277,13 @@ async fn follow_subscription_produces_blocks() {
273277
});
274278
assert_eq!(event, expected);
275279

280+
// Then the best block
281+
let event: FollowEvent<String> = get_next_event(&mut sub).await;
282+
let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
283+
best_block_hash: format!("{:?}", finalized_hash),
284+
});
285+
assert_eq!(event, expected);
286+
276287
let block = BlockBuilderBuilder::new(&*client)
277288
.on_parent_block(client.chain_info().genesis_hash)
278289
.with_parent_block_number(0)
@@ -357,6 +368,13 @@ async fn follow_with_runtime() {
357368
});
358369
pretty_assertions::assert_eq!(event, expected);
359370

371+
// Then the best block
372+
let event: FollowEvent<String> = get_next_event(&mut sub).await;
373+
let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
374+
best_block_hash: format!("{:?}", finalized_hash),
375+
});
376+
pretty_assertions::assert_eq!(event, expected);
377+
360378
// Import a new block without runtime changes.
361379
// The runtime field must be None in this case.
362380
let block = BlockBuilderBuilder::new(&*client)
@@ -660,6 +678,10 @@ async fn call_runtime_without_flag() {
660678
get_next_event::<FollowEvent<String>>(&mut sub).await,
661679
FollowEvent::Initialized(_)
662680
);
681+
assert_matches!(
682+
get_next_event::<FollowEvent<String>>(&mut sub).await,
683+
FollowEvent::BestBlockChanged(_)
684+
);
663685
assert_matches!(
664686
get_next_event::<FollowEvent<String>>(&mut sub).await,
665687
FollowEvent::NewBlock(_)
@@ -1325,6 +1347,10 @@ async fn separate_operation_ids_for_subscriptions() {
13251347
get_next_event::<FollowEvent<String>>(&mut sub_first).await,
13261348
FollowEvent::Initialized(_)
13271349
);
1350+
assert_matches!(
1351+
get_next_event::<FollowEvent<String>>(&mut sub_first).await,
1352+
FollowEvent::BestBlockChanged(_)
1353+
);
13281354
assert_matches!(
13291355
get_next_event::<FollowEvent<String>>(&mut sub_first).await,
13301356
FollowEvent::NewBlock(_)
@@ -1338,6 +1364,10 @@ async fn separate_operation_ids_for_subscriptions() {
13381364
get_next_event::<FollowEvent<String>>(&mut sub_second).await,
13391365
FollowEvent::Initialized(_)
13401366
);
1367+
assert_matches!(
1368+
get_next_event::<FollowEvent<String>>(&mut sub_second).await,
1369+
FollowEvent::BestBlockChanged(_)
1370+
);
13411371
assert_matches!(
13421372
get_next_event::<FollowEvent<String>>(&mut sub_second).await,
13431373
FollowEvent::NewBlock(_)
@@ -1562,6 +1592,10 @@ async fn follow_exceeding_pinned_blocks() {
15621592
get_next_event::<FollowEvent<String>>(&mut sub).await,
15631593
FollowEvent::Initialized(_)
15641594
);
1595+
assert_matches!(
1596+
get_next_event::<FollowEvent<String>>(&mut sub).await,
1597+
FollowEvent::BestBlockChanged(_)
1598+
);
15651599
assert_matches!(
15661600
get_next_event::<FollowEvent<String>>(&mut sub).await,
15671601
FollowEvent::NewBlock(_)
@@ -1642,6 +1676,10 @@ async fn follow_with_unpin() {
16421676
get_next_event::<FollowEvent<String>>(&mut sub).await,
16431677
FollowEvent::Initialized(_)
16441678
);
1679+
assert_matches!(
1680+
get_next_event::<FollowEvent<String>>(&mut sub).await,
1681+
FollowEvent::BestBlockChanged(_)
1682+
);
16451683
assert_matches!(
16461684
get_next_event::<FollowEvent<String>>(&mut sub).await,
16471685
FollowEvent::NewBlock(_)
@@ -1749,6 +1787,10 @@ async fn unpin_duplicate_hashes() {
17491787
get_next_event::<FollowEvent<String>>(&mut sub).await,
17501788
FollowEvent::Initialized(_)
17511789
);
1790+
assert_matches!(
1791+
get_next_event::<FollowEvent<String>>(&mut sub).await,
1792+
FollowEvent::BestBlockChanged(_)
1793+
);
17521794
assert_matches!(
17531795
get_next_event::<FollowEvent<String>>(&mut sub).await,
17541796
FollowEvent::NewBlock(_)
@@ -1876,6 +1918,10 @@ async fn follow_with_multiple_unpin_hashes() {
18761918
get_next_event::<FollowEvent<String>>(&mut sub).await,
18771919
FollowEvent::Initialized(_)
18781920
);
1921+
assert_matches!(
1922+
get_next_event::<FollowEvent<String>>(&mut sub).await,
1923+
FollowEvent::BestBlockChanged(_)
1924+
);
18791925
assert_matches!(
18801926
get_next_event::<FollowEvent<String>>(&mut sub).await,
18811927
FollowEvent::NewBlock(_)
@@ -1991,6 +2037,13 @@ async fn follow_prune_best_block() {
19912037
});
19922038
assert_eq!(event, expected);
19932039

2040+
// Then the best block
2041+
let event: FollowEvent<String> = get_next_event(&mut sub).await;
2042+
let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
2043+
best_block_hash: format!("{:?}", finalized_hash),
2044+
});
2045+
assert_eq!(event, expected);
2046+
19942047
// Block tree:
19952048
//
19962049
// finalized -> block 1 -> block 2
@@ -2261,6 +2314,12 @@ async fn follow_forks_pruned_block() {
22612314
});
22622315
assert_eq!(event, expected);
22632316

2317+
let event: FollowEvent<String> = get_next_event(&mut sub).await;
2318+
let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
2319+
best_block_hash: format!("{:?}", block_3_hash),
2320+
});
2321+
assert_eq!(event, expected);
2322+
22642323
// Block tree:
22652324
//
22662325
// finalized -> block 1 -> block 2 -> block 3 -> block 4
@@ -2612,6 +2671,10 @@ async fn pin_block_references() {
26122671
get_next_event::<FollowEvent<String>>(&mut sub).await,
26132672
FollowEvent::Initialized(_)
26142673
);
2674+
assert_matches!(
2675+
get_next_event::<FollowEvent<String>>(&mut sub).await,
2676+
FollowEvent::BestBlockChanged(_)
2677+
);
26152678
assert_matches!(
26162679
get_next_event::<FollowEvent<String>>(&mut sub).await,
26172680
FollowEvent::NewBlock(_)
@@ -2845,6 +2908,10 @@ async fn ensure_operation_limits_works() {
28452908
get_next_event::<FollowEvent<String>>(&mut sub).await,
28462909
FollowEvent::Initialized(_)
28472910
);
2911+
assert_matches!(
2912+
get_next_event::<FollowEvent<String>>(&mut sub).await,
2913+
FollowEvent::BestBlockChanged(_)
2914+
);
28482915
assert_matches!(
28492916
get_next_event::<FollowEvent<String>>(&mut sub).await,
28502917
FollowEvent::NewBlock(_)
@@ -2956,6 +3023,10 @@ async fn storage_is_backpressured() {
29563023
get_next_event::<FollowEvent<String>>(&mut sub).await,
29573024
FollowEvent::Initialized(_)
29583025
);
3026+
assert_matches!(
3027+
get_next_event::<FollowEvent<String>>(&mut sub).await,
3028+
FollowEvent::BestBlockChanged(_)
3029+
);
29593030
assert_matches!(
29603031
get_next_event::<FollowEvent<String>>(&mut sub).await,
29613032
FollowEvent::NewBlock(_)
@@ -3091,6 +3162,10 @@ async fn stop_storage_operation() {
30913162
get_next_event::<FollowEvent<String>>(&mut sub).await,
30923163
FollowEvent::Initialized(_)
30933164
);
3165+
assert_matches!(
3166+
get_next_event::<FollowEvent<String>>(&mut sub).await,
3167+
FollowEvent::BestBlockChanged(_)
3168+
);
30943169
assert_matches!(
30953170
get_next_event::<FollowEvent<String>>(&mut sub).await,
30963171
FollowEvent::NewBlock(_)
@@ -3378,6 +3453,10 @@ async fn chain_head_stop_all_subscriptions() {
33783453
get_next_event::<FollowEvent<String>>(&mut sub).await,
33793454
FollowEvent::Initialized(_)
33803455
);
3456+
assert_matches!(
3457+
get_next_event::<FollowEvent<String>>(&mut sub).await,
3458+
FollowEvent::BestBlockChanged(_)
3459+
);
33813460

33823461
// Import 6 blocks in total to trigger the suspension distance.
33833462
let mut parent_hash = client.chain_info().genesis_hash;
@@ -3638,6 +3717,13 @@ async fn follow_unique_pruned_blocks() {
36383717
});
36393718
assert_eq!(event, expected);
36403719

3720+
// Then the best block
3721+
let event: FollowEvent<String> = get_next_event(&mut sub).await;
3722+
let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
3723+
best_block_hash: format!("{:?}", finalized_hash),
3724+
});
3725+
assert_eq!(event, expected);
3726+
36413727
// Block tree:
36423728
//
36433729
// finalized -> block 1 -> block 2 -> block 3
@@ -3807,6 +3893,13 @@ async fn follow_report_best_block_of_a_known_block() {
38073893
});
38083894
assert_eq!(event, expected);
38093895

3896+
// Then the best block
3897+
let event: FollowEvent<String> = get_next_event(&mut sub).await;
3898+
let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
3899+
best_block_hash: format!("{:?}", finalized_hash),
3900+
});
3901+
assert_eq!(event, expected);
3902+
38103903
// Block tree:
38113904
//
38123905
// finalized -> block 1 -> block 2
@@ -4026,6 +4119,13 @@ async fn follow_event_with_unknown_parent() {
40264119
});
40274120
assert_eq!(event, expected);
40284121

4122+
// Then the best block
4123+
let event: FollowEvent<String> = get_next_event(&mut sub).await;
4124+
let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
4125+
best_block_hash: format!("{:?}", finalized_hash),
4126+
});
4127+
assert_eq!(event, expected);
4128+
40294129
// Block tree:
40304130
//
40314131
// finalized -> (gap: block 1) -> block 2

0 commit comments

Comments
 (0)