Skip to content

Commit 68c1250

Browse files
volivagithub-actions[bot]lexnv
authored
fix(rpc-spec-v2): best block not announced immediately after initialised (#10525)
# Description Fixes polkadot-api/polkadot-api#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 <label-name>` 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 <[email protected]>
1 parent 613e00e commit 68c1250

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: major

‎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
@@ -429,16 +429,14 @@ where
429429

430430
// Generate a new best block event.
431431
let best_block_hash = startup_point.best_hash;
432-
if best_block_hash != finalized_block_hash {
433-
if !self.announced_blocks.was_announced(&best_block_hash) {
434-
return Err(SubscriptionManagementError::BlockHeaderAbsent);
435-
}
436-
self.announced_blocks.insert(best_block_hash, true);
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);
437436

438-
let best_block = FollowEvent::BestBlockChanged(BestBlockChanged { best_block_hash });
439-
self.current_best_block = Some(best_block_hash);
440-
finalized_block_descendants.push(best_block);
441-
};
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);
442440

443441
Ok(finalized_block_descendants)
444442
}

‎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(_)
@@ -1389,6 +1411,10 @@ async fn separate_operation_ids_for_subscriptions() {
13891411
get_next_event::<FollowEvent<String>>(&mut sub_first).await,
13901412
FollowEvent::Initialized(_)
13911413
);
1414+
assert_matches!(
1415+
get_next_event::<FollowEvent<String>>(&mut sub_first).await,
1416+
FollowEvent::BestBlockChanged(_)
1417+
);
13921418
assert_matches!(
13931419
get_next_event::<FollowEvent<String>>(&mut sub_first).await,
13941420
FollowEvent::NewBlock(_)
@@ -1402,6 +1428,10 @@ async fn separate_operation_ids_for_subscriptions() {
14021428
get_next_event::<FollowEvent<String>>(&mut sub_second).await,
14031429
FollowEvent::Initialized(_)
14041430
);
1431+
assert_matches!(
1432+
get_next_event::<FollowEvent<String>>(&mut sub_second).await,
1433+
FollowEvent::BestBlockChanged(_)
1434+
);
14051435
assert_matches!(
14061436
get_next_event::<FollowEvent<String>>(&mut sub_second).await,
14071437
FollowEvent::NewBlock(_)
@@ -1626,6 +1656,10 @@ async fn follow_exceeding_pinned_blocks() {
16261656
get_next_event::<FollowEvent<String>>(&mut sub).await,
16271657
FollowEvent::Initialized(_)
16281658
);
1659+
assert_matches!(
1660+
get_next_event::<FollowEvent<String>>(&mut sub).await,
1661+
FollowEvent::BestBlockChanged(_)
1662+
);
16291663
assert_matches!(
16301664
get_next_event::<FollowEvent<String>>(&mut sub).await,
16311665
FollowEvent::NewBlock(_)
@@ -1706,6 +1740,10 @@ async fn follow_with_unpin() {
17061740
get_next_event::<FollowEvent<String>>(&mut sub).await,
17071741
FollowEvent::Initialized(_)
17081742
);
1743+
assert_matches!(
1744+
get_next_event::<FollowEvent<String>>(&mut sub).await,
1745+
FollowEvent::BestBlockChanged(_)
1746+
);
17091747
assert_matches!(
17101748
get_next_event::<FollowEvent<String>>(&mut sub).await,
17111749
FollowEvent::NewBlock(_)
@@ -1813,6 +1851,10 @@ async fn unpin_duplicate_hashes() {
18131851
get_next_event::<FollowEvent<String>>(&mut sub).await,
18141852
FollowEvent::Initialized(_)
18151853
);
1854+
assert_matches!(
1855+
get_next_event::<FollowEvent<String>>(&mut sub).await,
1856+
FollowEvent::BestBlockChanged(_)
1857+
);
18161858
assert_matches!(
18171859
get_next_event::<FollowEvent<String>>(&mut sub).await,
18181860
FollowEvent::NewBlock(_)
@@ -1940,6 +1982,10 @@ async fn follow_with_multiple_unpin_hashes() {
19401982
get_next_event::<FollowEvent<String>>(&mut sub).await,
19411983
FollowEvent::Initialized(_)
19421984
);
1985+
assert_matches!(
1986+
get_next_event::<FollowEvent<String>>(&mut sub).await,
1987+
FollowEvent::BestBlockChanged(_)
1988+
);
19431989
assert_matches!(
19441990
get_next_event::<FollowEvent<String>>(&mut sub).await,
19451991
FollowEvent::NewBlock(_)
@@ -2055,6 +2101,13 @@ async fn follow_prune_best_block() {
20552101
});
20562102
assert_eq!(event, expected);
20572103

2104+
// Then the best block
2105+
let event: FollowEvent<String> = get_next_event(&mut sub).await;
2106+
let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
2107+
best_block_hash: format!("{:?}", finalized_hash),
2108+
});
2109+
assert_eq!(event, expected);
2110+
20582111
// Block tree:
20592112
//
20602113
// finalized -> block 1 -> block 2
@@ -2325,6 +2378,12 @@ async fn follow_forks_pruned_block() {
23252378
});
23262379
assert_eq!(event, expected);
23272380

2381+
let event: FollowEvent<String> = get_next_event(&mut sub).await;
2382+
let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
2383+
best_block_hash: format!("{:?}", block_3_hash),
2384+
});
2385+
assert_eq!(event, expected);
2386+
23282387
// Block tree:
23292388
//
23302389
// finalized -> block 1 -> block 2 -> block 3 -> block 4
@@ -2682,6 +2741,10 @@ async fn pin_block_references() {
26822741
get_next_event::<FollowEvent<String>>(&mut sub).await,
26832742
FollowEvent::Initialized(_)
26842743
);
2744+
assert_matches!(
2745+
get_next_event::<FollowEvent<String>>(&mut sub).await,
2746+
FollowEvent::BestBlockChanged(_)
2747+
);
26852748
assert_matches!(
26862749
get_next_event::<FollowEvent<String>>(&mut sub).await,
26872750
FollowEvent::NewBlock(_)
@@ -2915,6 +2978,10 @@ async fn ensure_operation_limits_works() {
29152978
get_next_event::<FollowEvent<String>>(&mut sub).await,
29162979
FollowEvent::Initialized(_)
29172980
);
2981+
assert_matches!(
2982+
get_next_event::<FollowEvent<String>>(&mut sub).await,
2983+
FollowEvent::BestBlockChanged(_)
2984+
);
29182985
assert_matches!(
29192986
get_next_event::<FollowEvent<String>>(&mut sub).await,
29202987
FollowEvent::NewBlock(_)
@@ -3042,6 +3109,10 @@ async fn storage_is_backpressured() {
30423109
get_next_event::<FollowEvent<String>>(&mut sub).await,
30433110
FollowEvent::Initialized(_)
30443111
);
3112+
assert_matches!(
3113+
get_next_event::<FollowEvent<String>>(&mut sub).await,
3114+
FollowEvent::BestBlockChanged(_)
3115+
);
30453116
assert_matches!(
30463117
get_next_event::<FollowEvent<String>>(&mut sub).await,
30473118
FollowEvent::NewBlock(_)
@@ -3178,6 +3249,10 @@ async fn stop_storage_operation() {
31783249
get_next_event::<FollowEvent<String>>(&mut sub).await,
31793250
FollowEvent::Initialized(_)
31803251
);
3252+
assert_matches!(
3253+
get_next_event::<FollowEvent<String>>(&mut sub).await,
3254+
FollowEvent::BestBlockChanged(_)
3255+
);
31813256
assert_matches!(
31823257
get_next_event::<FollowEvent<String>>(&mut sub).await,
31833258
FollowEvent::NewBlock(_)
@@ -3474,6 +3549,10 @@ async fn chain_head_stop_all_subscriptions() {
34743549
get_next_event::<FollowEvent<String>>(&mut sub).await,
34753550
FollowEvent::Initialized(_)
34763551
);
3552+
assert_matches!(
3553+
get_next_event::<FollowEvent<String>>(&mut sub).await,
3554+
FollowEvent::BestBlockChanged(_)
3555+
);
34773556

34783557
// Import 6 blocks in total to trigger the suspension distance.
34793558
let mut parent_hash = client.chain_info().genesis_hash;
@@ -3742,6 +3821,13 @@ async fn follow_unique_pruned_blocks() {
37423821
});
37433822
assert_eq!(event, expected);
37443823

3824+
// Then the best block
3825+
let event: FollowEvent<String> = get_next_event(&mut sub).await;
3826+
let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
3827+
best_block_hash: format!("{:?}", finalized_hash),
3828+
});
3829+
assert_eq!(event, expected);
3830+
37453831
// Block tree:
37463832
//
37473833
// finalized -> block 1 -> block 2 -> block 3
@@ -3911,6 +3997,13 @@ async fn follow_report_best_block_of_a_known_block() {
39113997
});
39123998
assert_eq!(event, expected);
39133999

4000+
// Then the best block
4001+
let event: FollowEvent<String> = get_next_event(&mut sub).await;
4002+
let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
4003+
best_block_hash: format!("{:?}", finalized_hash),
4004+
});
4005+
assert_eq!(event, expected);
4006+
39144007
// Block tree:
39154008
//
39164009
// finalized -> block 1 -> block 2
@@ -4130,6 +4223,13 @@ async fn follow_event_with_unknown_parent() {
41304223
});
41314224
assert_eq!(event, expected);
41324225

4226+
// Then the best block
4227+
let event: FollowEvent<String> = get_next_event(&mut sub).await;
4228+
let expected = FollowEvent::BestBlockChanged(BestBlockChanged {
4229+
best_block_hash: format!("{:?}", finalized_hash),
4230+
});
4231+
assert_eq!(event, expected);
4232+
41334233
// Block tree:
41344234
//
41354235
// finalized -> (gap: block 1) -> block 2

0 commit comments

Comments
 (0)