Add assignment keys to session keys, no separate approvals key #2092

rphmeier · 2020-12-09T01:45:16Z

For Polkadot, Kusama, and Westend, sets up a runtime upgrade to migrate the keys that uses paritytech/substrate#7688.

For Rococo and Test-runtime, we still need to wire it up to the SessionInfo module somehow. That's beyond the scope of this issue in particular.

Unlike we previously thought, we don't need a separate key just for approvals. We can reuse the parachain validation key for this purpose. We'll also use the same key for disputes.

I'll test the migration on a local Westend before marking this ready for review.

rphmeier · 2020-12-09T03:14:41Z

I tested this on a local westend with no issues. I made sure it continued authoring & finalizing beyond the next session change.

burdges · 2020-12-09T09:12:44Z

roadmap/implementers-guide/src/protocol-approval.md

@@ -38,7 +38,7 @@ We need two separate keys for the approval subsystem:

 - **Approval assignment keys** are sr25519/schnorrkel keys used only for the assignment criteria VRFs.  We implicitly sign assignment notices with approval assignment keys by including their relay chain context and additional data in the VRF's extra message, but exclude these from its VRF input.  

- **Approval vote keys** would only sign off on candidate parablock validity and has no natural key type restrictions.  We could reuse the ed25519 grandpa keys for this purpose since these signatures control access to grandpa, although distant future node configurations might favor separate roles.
+- **Approval vote keys** would only sign off on candidate parablock validity and has no natural key type restrictions. There's no need for this to actualy embody a new session key type. We just want to make a distinction between assignments and approvals, although distant future node configurations might favor separate roles. We re-use the same keys as are used for parachain backing in practice.


Assignments and approvals need separate keys so that assignments keys can stay on the machine while approvals keys might live on a remote signer.

We employ the same keys for all candidate validity and invalidity statements (approvals, backing, and disputes) because logically these incur similar risks.

Is there a change in the text you want me to make? I agree with everything you just said

It's fine I guess. It just confused me on a first reading. What should we actually call them? (candidate) validation keys?

Yeah, I called them para_validation keys in the runtime description.

burdges · 2020-12-09T09:15:45Z

roadmap/implementers-guide/src/runtime/session_info.md

@@ -8,12 +8,13 @@ Helper structs:

 ```rust
 struct SessionInfo {
-    // validators in canonical ordering.
+    // validators in canonical ordering. These are the public keys used for backing,
+    // dispute participation, and approvals.
    validators: Vec<ValidatorId>,


Yes exactly.

rphmeier · 2020-12-09T17:06:46Z

I've tested a local rococo testnet with adder-collator; parachain consensus functions as expected.

ordian · 2020-12-09T19:31:38Z

runtime/kusama/src/lib.rs

@@ -90,7 +90,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	spec_name: create_runtime_str!("kusama"),
 	impl_name: create_runtime_str!("parity-kusama"),
 	authoring_version: 2,
-	spec_version: 2027,
+	spec_version: 2028,


the latest released version is on 2026, right?
https://github.com/paritytech/polkadot/blob/v0.8.26-1/runtime/kusama/src/lib.rs#L91
is the assumption that this will be merged after 0.8.27 branches off?

cc @s3krit - I believe there's a release queued up currently and we want this to go into the next one. Let me know what I should be doing with the version numbers here.

Correct. We have already branched off for v0.8.27, so if your intent is for this change to go into v0.8.28 (i.e., the release after v0.8.27, barring any security releases), this looks right

ordian · 2020-12-09T19:32:27Z

runtime/kusama/src/lib.rs

+			let mut id = AssignmentId::default();
+			let id_raw: &mut [u8] = id.as_mut();
+			id_raw.copy_from_slice(v.as_ref());
+			id_raw[0..5].copy_from_slice(b"assgn");


where does assgn come from? KeyTypeId is defined as asgn:

pub const ASSIGNMENT_KEY_TYPE_ID: KeyTypeId = KeyTypeId(*b"asgn");

The only goal of this section is uniqueness per validator and independence from other session keys, which we achieve by combining the (unique) ValidatorId with the text 'assgn'`. I've now changed this to 'asgn' for consistency, although it will not affect observed behavior in any meaningful way.

ordian · 2020-12-10T23:13:00Z

runtime/common/src/lib.rs

@@ -158,6 +158,35 @@ impl<T: pallet_session::Config>
 	fn on_disabled(_: usize) { }
 }

+/// A placeholder since there is currently no provided session key handler for parachain validator


can be replaced by SessionInfo I guess?

Not yet everywhere, but in the long term. This struct is used in the polkadot, kusama, westend runtimes where we don't yet enable parachain runtime modules.

bkchr

Looks good, just one question.

Sorry for being late.

bkchr · 2020-12-11T09:20:58Z

runtime/polkadot/src/lib.rs

+		im_online: old.im_online,
+		para_validator: old.para_validator,
+		para_assignment: {
+			// We need to produce a dummy value that's unique for the validator.


DQ: Why does that needs to be unique?

The underlying reason is that frame-session has a KeyOwner: map (KeyTypeId, [u8]) -> ValidatorId. This map requires validators to have unique session keys. From the perspective of this PR, the reason is that upgrade_keys has documented that validators' keys must be unique after the upgrade.

kianenigma · 2020-12-17T14:08:24Z

runtime/kusama/src/lib.rs

@@ -1208,7 +1247,7 @@ pub type Executive = frame_executive::Executive<
 	frame_system::ChainContext<Runtime>,
 	Runtime,
 	AllModules,
-	FixCouncilHistoricalVotes,


This PR has mistakenly removed this migration from the next release, while it has not be applied yet. Should have used a tuple of both.

oh or we already have branched v27? then it is okay.

Okay -- sorry, it is in the 27 release. Then there are two questions:

is there a way to make it possible for this to also be noted in the release note? I first checked there and didn't find it, and then I checked the branch and it was there. cc @s3krit

Why is the struct definition not removed? :p

Yeah, let's add it to the v28 release notes.

"Includes a runtime migration to update session key types to be used for parachain validation and approval. All validators are encouraged to rotate their session keys following the upgrade."

rphmeier added 5 commits December 8, 2020 18:48

guide: merge backing and approval keys

c8b3f64

bump substrate master & update primitives

9217b8f

use new SessionInfo struct in session_info

de24f2c

session keys upgrade for Polkadot

4a68f4b

kusama & westend runtimes

e01081d

rphmeier added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Dec 9, 2020

rphmeier changed the title ~~Add assignment keys to session keys~~ Add assignment keys to session keys, no separate approvals key Dec 9, 2020

paritytech-ci added the D2-breaksapi label Dec 9, 2020

rphmeier mentioned this pull request Dec 9, 2020

Prepare SessionInfo module to be used in practice #2093

Closed

rphmeier added 2 commits December 8, 2020 21:04

bump westend, kusama, and polkadot versions

faaad1c

add session key to rococo & test-runtime

3b5a55f

rphmeier marked this pull request as ready for review December 9, 2020 03:14

github-actions bot 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 Dec 9, 2020

burdges reviewed Dec 9, 2020

View reviewed changes

rphmeier added 4 commits December 9, 2020 10:15

update prepare-test-net to latest subkey

3eaded6

update chain specs to support new para_assignment session key

d54864b

get cargo.lock from master

a4f2d26

formatting

7ea8630

rphmeier added 3 commits December 9, 2020 11:26

update kill_storage based on substrate master

1e3c315

Merge branch 'master' into rh-add-assignments-keys

93618d2

fix test-service

43a6161

ordian reviewed Dec 9, 2020

View reviewed changes

rphmeier added 2 commits December 10, 2020 13:08

assgn -> asgn

89f3f31

Merge branch 'master' into rh-add-assignments-keys

8b8db9d

ordian approved these changes Dec 10, 2020

View reviewed changes

ordian reviewed Dec 10, 2020

View reviewed changes

use session info module for assignment session key

57e3661

rphmeier merged commit 6701f77 into master Dec 11, 2020

rphmeier deleted the rh-add-assignments-keys branch December 11, 2020 02:30

bkchr reviewed Dec 11, 2020

View reviewed changes

kianenigma reviewed Dec 17, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add assignment keys to session keys, no separate approvals key #2092

Add assignment keys to session keys, no separate approvals key #2092

rphmeier commented Dec 9, 2020 •

edited

Loading

rphmeier commented Dec 9, 2020

burdges Dec 9, 2020

rphmeier Dec 9, 2020 •

edited

Loading

burdges Dec 9, 2020

rphmeier Dec 9, 2020

burdges Dec 9, 2020

rphmeier commented Dec 9, 2020

ordian Dec 9, 2020

rphmeier Dec 10, 2020

s3krit Dec 10, 2020

ordian Dec 9, 2020

rphmeier Dec 10, 2020

ordian Dec 10, 2020

rphmeier Dec 10, 2020

bkchr left a comment

bkchr Dec 11, 2020

rphmeier Dec 12, 2020

kianenigma Dec 17, 2020 •

edited

Loading

kianenigma Dec 17, 2020

kianenigma Dec 17, 2020

rphmeier Dec 17, 2020

rphmeier Dec 17, 2020

Add assignment keys to session keys, no separate approvals key #2092

Add assignment keys to session keys, no separate approvals key #2092

Conversation

rphmeier commented Dec 9, 2020 • edited Loading

rphmeier commented Dec 9, 2020

Choose a reason for hiding this comment

rphmeier Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier commented Dec 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier commented Dec 9, 2020 •

edited

Loading

rphmeier Dec 9, 2020 •

edited

Loading

kianenigma Dec 17, 2020 •

edited

Loading