-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(rpc-spec-v2): best block not announced immediately after initialised #10525
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
/cmd label T0-node |
|
/cmd prdoc |
|
Command "label T0-node" has failed ❌! See logs here |
prdoc/pr_10525.prdoc
Outdated
| ## 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. | ||
| * [ ] 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: lets remove the checklist from the prdoc, is added by default but not really related to our release process
lexnv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me! @voliva thanks for handling this 🙏
68c1250
…sed (#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]> (cherry picked from commit 68c1250)
|
Successfully created backport PR for |
…sed (#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]> (cherry picked from commit 68c1250)
|
Successfully created backport PR for |
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]>
Backport #10525 into `stable2512` 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]>
Description
Fixes polkadot-api/polkadot-api#1244
The current chainHead_v1 implementation is not spec-compliant, as it states:
And the current implemention only emits the
bestBlockChangednotification 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
bestBlockChangednotification is always emited. All tests are updated to this new behaviourChecklist
Trequired)/cmd label <label-name>to add labels