Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

*: Enable refactored authority discovery #601

Closed
wants to merge 5 commits into from
Closed

*: Enable refactored authority discovery #601

wants to merge 5 commits into from

Conversation

mxinden
Copy link
Contributor

@mxinden mxinden commented Nov 21, 2019

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 into master.

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?

@mxinden mxinden added enhancement A0-please_review Pull request needs code review. J0-enhancement An additional feature request. labels Nov 21, 2019
@parity-cla-bot
Copy link

It looks like @mxinden signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@jacogr
Copy link
Contributor

jacogr commented Nov 21, 2019

@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.

@gavofyork gavofyork marked this pull request as ready for review November 22, 2019 15:23
gavofyork
gavofyork reviewed Nov 22, 2019
View reviewed changes
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,
Copy link
Member

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.

gavofyork
gavofyork reviewed Nov 22, 2019
View reviewed changes
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)>,
Copy link
Member

Choose a reason for hiding this comment

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

same here.

gavofyork
gavofyork reviewed Nov 22, 2019
View reviewed changes
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),
Copy link
Member

Choose a reason for hiding this comment

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

same here

gavofyork
gavofyork reviewed Nov 22, 2019
View reviewed changes
service/src/chain_spec.rs Outdated
@@ -194,6 +203,7 @@ pub fn get_authority_keys_from_seed(seed: &str) -> (
BabeId,
GrandpaId,
ImOnlineId,
AuthorityDiscoveryId,
Copy link
Member

Choose a reason for hiding this comment

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

same here

gavofyork
gavofyork suggested changes Nov 22, 2019
View reviewed changes
Copy link
Member

@gavofyork gavofyork left a comment •
edited

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.

@gavofyork
Copy link
Member

looking at the code, i'm pretty sure a special migration will be needed for NextKeys and QueuedKeys since it'll try to decode the entire tuple, which is now different, all at once. annoyingly, we can't enumerate either.

the easiest way forward will probably just be to create a custom decode function for the Keys type which appends zeroes onto the end of the input in case it's too short.

wdyt @bkchr ?

@bkchr
Copy link
Member

bkchr commented Nov 22, 2019

As described in Riot, we can iterate the Session NextKeys and QueuedKeys as both are a double map with a fixed first key. However, your proposed solution with a custom decode could work as well.

@mxinden
*: List authority discovery id after parachain validator id
f951914 
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.
@gavofyork gavofyork removed J0-enhancement An additional feature request. enhancement labels Nov 22, 2019
@mxinden
Copy link
Contributor Author

mxinden commented Nov 22, 2019

To make sure I understand your suggestion @bkchr , I could:

  1. Wait for storage to be iteratable for a specific prefix (Related to Introduce prefixed storage with enumeration substrate#4185).

  2. Within the on_initialize of the session module of the next runtime, iterate
    over the keys of the QueuedKeys and NextKeys prefix, parse each T::Keys
    value as if it contains 4 keys (which it still would) and write it back as 5
    keys (4 + authority discovery key) with the last either all zero, or as an
    Option<Key>::None.

  3. Eventually once someone generates a new session key, the 5th key will be
    derived, replacing the 5th key within the storage of either all zero or None
    with an actual authority discovery key.

@gavofyork
Copy link
Member

that's right, though if we want to move things along without blocking on paritytech/substrate#4185 then we can also add a special Decode into key tuples inside the impl_opaque_keys macro, which makes it zero-pad the input (using e.g. TrailingZeroInput) so that if there's only 4 keys explicitly in storage (as there would be) it'll still find a bunch of trailing zeros for the 5th.

@gavofyork gavofyork added A4-gotissues A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. A4-gotissues labels Nov 22, 2019
@mxinden mxinden changed the title [WIP] *: Enable refactored authority discovery *: Enable refactored authority discovery Nov 27, 2019
@mxinden
Copy link
Contributor Author

mxinden commented Nov 27, 2019

Cherrypicking this change set on top of master right now.

@mxinden
Copy link
Contributor Author

mxinden commented Nov 27, 2019

6355956 gates the authority discovery module behind the --enable-authority-discovery feature flag.

@bkchr @gavofyork could you give this another review?

@mxinden
Copy link
Contributor Author

mxinden commented Nov 27, 2019

mxinden:enable-authority-discovery-master contains the same commits cherry-picked on top of current paritytech:master.

https://github.com/paritytech/polkadot/compare/master...mxinden:enable-authority-discovery-master?expand=1

@mxinden mxinden added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Nov 27, 2019
@mxinden mxinden mentioned this pull request Nov 28, 2019
Merged
@mxinden
Copy link
Contributor Author

mxinden commented Nov 28, 2019

Replaced by #624.

@mxinden mxinden closed this Nov 28, 2019
tomusdrw pushed a commit that referenced this pull request Mar 26, 2021
@svyatonik
Fix Ethereum pallet benchmarks (#601)
9344cb6
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gavofyork gavofyork gavofyork requested changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mxinden @parity-cla-bot @jacogr @gavofyork @bkchr