-
Notifications
You must be signed in to change notification settings - Fork 1.6k
*: Enable refactored authority discovery #601
Conversation
It looks like @mxinden signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
@mxinden I have a PR for that already in the API, e.g. polkadot-js/api#1573 It is a bit tricky since it will need to go live with the Kusama upgrade. (However as soon as merged here, will merge there and then merge the appropriate UI PR as the change goes live on Kusama with a runtime upgrade) I'm more worried about the validators swapping over than anything else. |
runtime/src/lib.rs
Outdated
@@ -253,6 +254,7 @@ impl_opaque_keys! { | |||
pub grandpa: Grandpa, | |||
pub babe: Babe, | |||
pub im_online: ImOnline, | |||
pub authority_discovery: AuthorityDiscovery, |
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.
should probably be in the line below so the existing key types don't switch their location.
service/src/chain_spec.rs
Outdated
get_from_seed::<ValidatorId>(seed), | ||
) | ||
} | ||
|
||
/// Helper function to create GenesisConfig for testing | ||
pub fn testnet_genesis( | ||
initial_authorities: Vec<(AccountId, AccountId, BabeId, GrandpaId, ImOnlineId, ValidatorId)>, | ||
initial_authorities: Vec<(AccountId, AccountId, BabeId, GrandpaId, ImOnlineId, AuthorityDiscoveryId, ValidatorId)>, |
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.
same here.
service/src/chain_spec.rs
Outdated
@@ -202,13 +212,14 @@ pub fn get_authority_keys_from_seed(seed: &str) -> ( | |||
get_from_seed::<BabeId>(seed), | |||
get_from_seed::<GrandpaId>(seed), | |||
get_from_seed::<ImOnlineId>(seed), | |||
get_from_seed::<AuthorityDiscoveryId>(seed), |
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.
same here
service/src/chain_spec.rs
Outdated
@@ -194,6 +203,7 @@ pub fn get_authority_keys_from_seed(seed: &str) -> ( | |||
BabeId, | |||
GrandpaId, | |||
ImOnlineId, | |||
AuthorityDiscoveryId, |
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.
same 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.
ordering.
also, is it certain that simply adding an entry here without any kind of migration will not break session, given that it's currently storing session keys with only 4 entries? if it tries to decode one of the old keys with 4 entries and find the new 5th entry, it should gracefully return the default key or something, definitely should not panic or remove the ability to correctly inspect the other keys.
looking at the code, i'm pretty sure a special migration will be needed for the easiest way forward will probably just be to create a custom decode function for the wdyt @bkchr ? |
As described in Riot, we can iterate the |
Make sure existing key types don't change their order by appending the authority discovery id instead of injecting it between im online id and parachain validator id.
To make sure I understand your suggestion @bkchr , I could:
|
that's right, though if we want to move things along without blocking on paritytech/substrate#4185 then we can also add a special |
Cherrypicking this change set on top of |
6355956 gates the authority discovery module behind the @bkchr @gavofyork could you give this another review? |
|
Replaced by #624. |
This pull request introduces the refactored version of the authority discovery module into the
v0.6
Kusama branch. I would later on cherry-pick it intomaster
.Rollout strategy
The authority discovery module has not been tested on a large network so far. I would suggest not to roll it out to all nodes at once, but at the same time I am also not familiar with how we roll out new features in Kusama in general. Thoughts? Is this too careful?
Authority discovery requires a runtime module. In addition it depends on other nodes putting their identity onto the DHT. Thus we can't just run a single custom node. We could gate the feature behind a cli flag and enable it on two of our validators once the feature is rolled out.
Additional required changes due to session key order change
@bkchr mentioned that the UI depends on the order of the session keys as of today. Given that this pull request alters this order @jacogr what are the necessary external changes needed?