Skip to content
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

[async_backing] Bump ParachainHost to api version 10 on polkadot #222

Conversation

alexggh
Copy link
Contributor

@alexggh alexggh commented Mar 7, 2024

This will enable async-backing subsystems on polkadot, the relaychain would work in legacy mode where only candiddates built on top of pervious relay chain are activated.

Notes

  • Had to bring an unrelated change as well minimum_backing_votes because that was the v6, we can't skip versions, the value of that configuration on polkadot is 2, so that's what is going to be used after this runtime update. Changes have been running on kusama.
  • disabled_validators, node_features, approval_voting_params are not related with async-backing, but given they are low risk, we decided to include them as well, see comments.

Known risks

Async backing subsytems is a major change in the way collator-protocol and the backing work, so there are still some unknowns if this is completely bug free. It has been running on kusama for a month already, but not without issues:

  • Validators that did not upgrade to compatible versions will not be able to participate in backing, so if enough of those are randomly picked that group won't be able to back anything. With backing_group_size = 5 and minimum_backing_votes = 2, 10% validator not upgraded, that chance is about 2.5%.

  • Additionally, same un-upgraded groups won't be able to include the backing votes on chain when they author blocks, so 10% of the blocks won't back any candidates.

  • We are still not sure if item 2) from here Kusama parachains block production slowed down paritytech/polkadot-sdk#3314 (comment) is caused by async backing, the observable issue is sometimes after restart/upgrade some validators are getting 0 rewards and apparently they are not backing anything.

This will enable async-backing subsystems on polkadot, the relaychain
would work in legacy mode where only candiddates built on top of
pervious relay chain are activated.

- Had to bring an unrelated change as well `minimum_backing_votes`
  because that was the v6, we can't skip versions, the value of that
  configuration on polkadot is 2, so that's what is going to be used
  after this runtime update. Changes have been running on kusama, so I
  would.

Async backing subsytems is a major change in the way collator-protocol
and  the backing work, so there are still some unknowns if this is
completely bug free. It has been running on kusama for a month
already, but not without issues:
 - Validators that did not upgrade to compatible versions will not be
   able to participate in backing, so if enough of those are randomly
   picked that group won't be able to back anything. With
   backing_group_size = 5 and minimum_backing_votes = 2, 10% validator
   not upgraded that chance is about 2.5%.

 - Additionally, same un-upgraded groups won't be able to include the
   backing votes on chain when they author blocks, so 10% of the blocks
   won't back any candidates.

 - We are still not sure if item 2) from here paritytech/polkadot-sdk#3314 (comment)
   is caused by async backing, the observable issue is sometimes after restart/upgrade
   some validators are getting 0 rewards and apparently they are not
   backing anything.

Signed-off-by: Alexandru Gheorghe <[email protected]>
Signed-off-by: Alexandru Gheorghe <[email protected]>
@alexggh alexggh changed the title [async_backing] Bump ParachainHost to api version 7 on polkadot [async_backing] Bump ParachainHost to api version 10 on polkadot Mar 8, 2024
@eskimor eskimor mentioned this pull request Mar 11, 2024
19 tasks
@bkchr
Copy link
Contributor

bkchr commented Mar 11, 2024

@alexggh can you please finish the pr, to make it reviewable?

@alexggh
Copy link
Contributor Author

alexggh commented Mar 11, 2024

@alexggh can you please finish the pr, to make it reviewable?

Done, so that is clear for everyone we still have the risks presented in the description and if from now till 1.2 release gets enacted we would find any blocker issue, we would have to cancel 1.2 release and remove this changes or add a fix on top.

@alexggh alexggh marked this pull request as ready for review March 11, 2024 16:16
Comment on lines +2122 to +2132
fn disabled_validators() -> Vec<ValidatorIndex> {
parachains_vstaging_api_impl::disabled_validators::<Runtime>()
}

fn node_features() -> NodeFeatures {
parachains_vstaging_api_impl::node_features::<Runtime>()
}

fn approval_voting_params() -> ApprovalVotingParams {
parachains_vstaging_api_impl::approval_voting_params::<Runtime>()
}
Copy link
Contributor

@ordian ordian Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a blocker as we can do it as follow-up cleanup PR, but we should not use use vstaging runtime impls on Polkadot and instead stabilize them once tested on Kusama

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, then I'll clean this up with a follow up PR.

@eskimor
Copy link
Contributor

eskimor commented Mar 12, 2024

/merge

@fellowship-merge-bot fellowship-merge-bot bot enabled auto-merge (squash) March 12, 2024 10:51
@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

@alexggh
Copy link
Contributor Author

alexggh commented Mar 13, 2024

Bot fails to merge it, it seems another required migration is failing @bkontur @bkchr thoughts on how to get this merged ?

@bkontur
Copy link
Contributor

bkontur commented Mar 13, 2024

Bot fails to merge it, it seems another required migration is failing @bkontur @bkchr thoughts on how to get this merged ?

e.g. push empty commit and wait for CI again or find someone who can restart that job: https://github.com/polkadot-fellows/runtimes/actions/runs/8248792955/job/22560279689?pr=222, maybe @ggwpez could help?

@fellowship-merge-bot fellowship-merge-bot bot merged commit 845da76 into polkadot-fellows:main Mar 13, 2024
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants