-
Notifications
You must be signed in to change notification settings - Fork 105
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
Enable elastic scaling node side feature on Polkadot #340
Enable elastic scaling node side feature on Polkadot #340
Conversation
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
pub struct EnableElasticScalingNodeFeature; | ||
impl OnRuntimeUpgrade for EnableElasticScalingNodeFeature { | ||
fn on_runtime_upgrade() -> Weight { | ||
let _ = Configuration::set_node_feature(RawOrigin::Root.into(), 1, true); |
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.
Why are we not using the constant?
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.
It's currently in staging (based on current crate version usage), felt that we don't want to use any of the staging stuff here.
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.
Hmm - don't we have v7 already in the current release? In any case, seems like we would be using staging stuff anyways, just hiding it by using a magic number? Also I think this can wait for the next polkadot-sdk release, where this should be v7 then.
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.
That makes sense, but this needs to go in same or before release with core time.
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.
Do we again try to set some configuration option, that could just be a separate referendum? :P
Please close and just open a referendum on chain.
I think the reasoning of doing this as a migration was that it's more of a bugfix than a new feature. Once coretime is enabled on polkadot, if this feature bit is not set, one para that gets scheduled on two cores will brick itself. |
This can already be opened today and ready before Coretime lands on Polkadot. Not really an argument to put this into a runtime upgrade. We have done it here using a runtime upgrade, because the feature was added in the same release AFAIR. However, this is not the case anymore. |
This feature is needed on Polkadot to ensure parachains can still make progress if they have multiple cores assigned. Relay chain support for elastic scaling is implemented but requires bumping
polkadot-runtime-parachains
crate version to at least11.0.0
which will happen later.