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

Conversation

@dvc94ch
Copy link
Contributor

@dvc94ch dvc94ch commented May 29, 2019

The session module already supports tuple based session keys. You can add a custom session key type and provide Into<sr25519::Public> and Into<ed25519::Public> or Into<ble::Public> implementations. I updated the test to include such an example. Please let me know if I've missed the point :) @gavofyork @kianenigma

@rphmeier
Copy link
Contributor

I'm fine with the "just have your SessionKey type be a tuple" approach -- but we will need a little further glue for consensus engines to adapt SessionKey into their consensus key type.

e.g. we have two keys (a, b) for two consensus engines (A, B). We probably want something like

trait MyConsensusTrait: session::Trait {
    type OurKey;
    type ConvertSessionKey: Convert<<Self as session::Trait>::SessionKey, Self::OurKey>;
}

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Jun 3, 2019

So this glue has to go into the aura::Trait, grandpa::Trait and babe::Trait?

@dvc94ch dvc94ch force-pushed the dvc-session-key-tuple branch from a008a63 to 580ac33 Compare June 3, 2019 10:23
@dvc94ch dvc94ch changed the title Tuple based session key module and disabling validators. Tuple based session key module. Jun 3, 2019
@dvc94ch dvc94ch force-pushed the dvc-session-key-tuple branch from 580ac33 to 7b26636 Compare June 3, 2019 14:21
@dvc94ch dvc94ch force-pushed the dvc-session-key-tuple branch 2 times, most recently from 94bfcc6 to 57e747d Compare June 3, 2019 15:24
@gavofyork
Copy link
Member

So this glue has to go into the aura::Trait, grandpa::Trait and babe::Trait?

Yup

@gavofyork gavofyork added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A4-gotissues labels Jun 4, 2019
@burdges
Copy link

burdges commented Jun 4, 2019

Just some considerations about BLS keys:

It's fine if GRANDPA reads them in using some trait like Into<bls::Public>, but we actually want all validator BLS keys presented simultaneously as some type satisfying the bls-pixel::pop::ProofsOfPossession trait, which permits public key lookups by index or public key. Just fyi, you verify by making a bls-pixel::pop::BitPoPSignedMessage and adding all the signatures.

Any type that satisfies the ProofsOfPossession trait should check the proofs-of-possession for all keys. You can check many/all simultaneously using bls-pixel::distinct::DistinctMessages. You'll might batch verify the whole session certificate at the same time.

The session keys database should ideally be pulled out from the chain's data into some in-memory database like once per validator set election. It'll get run very slow if you end up checking the proofs-of-possession with every block or something.

I've no idea how this interacts with forks around the validator set election or whatever.

@gavofyork
Copy link
Member

gavofyork commented Jun 5, 2019

@dvc94ch The original design for this probably won't work; session module will need to keep an eye on the individual keys to ensure that validators do not use any key that some other validator has registered.

Something like #2628 is therefore needed, though it should be made to work with all elements of the tuple.

So: session module itself will need to know how to deconstruct session key "tuples" into individual keys, and it should maintain a per-key ordered list of active session keys in order to have efficient means of prevent a duplicate being registered.

@gavofyork
Copy link
Member

I'll write a bit of pseudocode to illustrate...

@burdges
Copy link

burdges commented Jun 5, 2019

Some considerations about the structure listed in paritytech/polkadot#47 (comment)

@gavofyork gavofyork mentioned this pull request Jun 5, 2019
16 tasks
@dvc94ch dvc94ch closed this Jun 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A3-in_progress Pull request is in progress. No review needed at this stage.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants