Skip to content
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

Refactor key management #3296

Merged
merged 96 commits into from Aug 7, 2019

Conversation

@gavofyork
Copy link
Member

commented Aug 3, 2019

Supersedes #3213

This PR is turning into a major refactoring over the crypto and key management system, which seems both long overdue and somewhat inescapable.

I'll want to integrate the general changes in (but not merge) #3034 and possibly #3193 together with #3137.

The main issues that this PR aims to solve are:

  • Independent subsystems that happened to use the same crypto had to share the same session keys (and key-store division).
  • Assumption that the twin consensus systems of block production (Aura/Babe) and finality (Grandpa) each required keys (which dirtied a lot of Substrate, placing APIs like fn fg_authority_key() all the way through the codebase). Though they do now, they won't forever and it shouldn't be an assumption that we bake in to internal APIs.
  • Keys passed in through CLI are sticky; their existence enabled the consensus system workers and force them to use these keys alone. Rather whatever key in the key store that happens to be an authority should be used at any given time.

TODO:

  • Refactor ImOnline:

    • Should get its own key and not piggy-back on other session keys.
  • Dynamic key selection:

    • Enable Babe and Grandpa to be able to determine current authority key (if there is one).
    • Dynamically select the correct authoring key to use in each subsystem.
    • Use --validator to enable active validation process; remove any validator key-management stuff from the CLI (--key). (A second --worker arg can enable off-chain workers.)
    • Remove all static-key selection code (--key all the way down to key-choice).
    • Remove authority_key/fg_authority_key APIs completely.
    • Make --password work (should accept a manually-entered password and use it for everything in the keystore).
    • Introduce RPCs for session rotation, using code from #3034 if it makes sense. There should be a single RPC author_rotateKeys which creates a new set of session keys, saves them to the key-store and returns their public keys, so that a rotation-transaction can be constructed. There should be no pollution of the APIs with session keys: the rotation API should be wholly independent of each of the individual keys. This may mean new APIs in the OpaqueKeys trait.
  • Refactor Client:

    • Create a basic Client trait that provides all the basic APIs and has Block as an associated item. Probably a separate PR.

I would also like to solve the fact that service construction is highly monolithic and polluted by consensus-module-specific code. Really it should just be a case of specifying the block production and finality systems and letting it do the rest. Code for set-up, take-down, RPC and key-store integration should be provided through traits that the consensus modules (Babe, Aura, Grandpa and eventually Rho and PoW) implement within their module's scope. There shouldn't be any consensus-module-specific logic dirtying up service.rs.

  • Refactor service:
    • Move out any consensus-module-specific code, and hardwired assumptions that they each have "authority keys", from service.rs into the respective consensus modules. Provide a single API endpoint (i.e. a type that implements some ConsensusSystem trait and have that as the only resource that service needs know of. Probably a separate PR.

This also adds:

  • Add Call type to extensible transactions.
  • Add swap to allow two storage items in the same map to be swapped.
gavofyork and others added 30 commits Jul 26, 2019
Fix
Introduce new AuthorityProvider API into Aura
This will eventually allow for dynamic determination of authority
keys and avoid having to set them directly on CLI.
Introduce authority determinator for Babe.
Experiment with modular consensus API.
@gnunicorn
Copy link
Member

left a comment

A few remarks, but nothing major.

Note: I only glanced over the macros and superficially looked at the SRML changes, as I missing the context to understand the design behind the changes - but nothing bad jumped out.

core/application-crypto/src/ed25519.rs Show resolved Hide resolved
#[cfg_attr(feature = "std", derive(Debug, Hash))]
pub struct Public($public);
}
// TODO: needed for verify since it takes an AsRef, but should be removed once that is

This comment has been minimized.

Copy link
@gnunicorn

gnunicorn Aug 7, 2019

Member

No TODO without a link to the issue pls.

core/application-crypto/src/lib.rs Outdated Show resolved Hide resolved
core/application-crypto/src/lib.rs Outdated Show resolved Hide resolved
core/application-crypto/src/lib.rs Outdated Show resolved Hide resolved
/// A type of supported crypto.
#[derive(Clone, Copy, PartialEq, Eq, Encode, Decode)]
#[cfg_attr(feature = "std", derive(Debug))]
#[repr(u32)]

This comment has been minimized.

Copy link
@gnunicorn

gnunicorn Aug 7, 2019

Member

any particular reason this is a u32 and not, for e.g. a u8? Are we expecting to ever have more than 255 known values in here?

core/service/src/components.rs Outdated Show resolved Hide resolved
core/sr-primitives/src/lib.rs Outdated Show resolved Hide resolved
@@ -68,7 +66,7 @@ construct_service_factory! {
},
AuthoritySetup = {
|service: Self::FullService| {
if let Some(key) = service.authority_key() {
if let Some(key) = None::<aura_primitives::sr25519::AuthorityPair> /* service.authority_key() */ {

This comment has been minimized.

Copy link
@gnunicorn

gnunicorn Aug 7, 2019

Member

I suppose that is not the template that we want to hand out after this PR, is it?....

@@ -56,7 +54,9 @@ fn staging_testnet_config_genesis() -> GenesisConfig {
// and
// for i in 1 2 3 4 ; do for j in session; do subkey --ed25519 inspect "$secret"//fir//$j//$i; done; done

let initial_authorities: Vec<(AccountId, AccountId, BabeId, GrandpaId)> = vec![(
// TODO: actually use sr25519 for babe keys (right now they seems to just be copies of the

This comment has been minimized.

Copy link
@gnunicorn

gnunicorn Aug 7, 2019

Member

ticket pls

bkchr and others added 7 commits Aug 7, 2019
Apply suggestions from code review
Co-Authored-By: Benjamin Kampmann <ben.kampmann@googlemail.com>
JSONRPC API for setting keys.
Also, rename traits::KeyStore* -> traits::BareCryptoStore*
gavofyork and others added 10 commits Aug 7, 2019

@gavofyork gavofyork merged commit ed61b1f into master Aug 7, 2019

@gavofyork gavofyork deleted the gav-in-progress branch Aug 7, 2019

@gavofyork gavofyork referenced this pull request Aug 8, 2019
4 of 4 tasks complete
kianenigma added a commit that referenced this pull request Aug 9, 2019
Refactor key management (#3296)
* Add Call type to extensible transactions.

Cleanup some naming

* Merge Resource and BlockExhausted into just Exhausted

* Fix

* Another fix

* Call

* Some fixes

* Fix srml tests.

* Fix all tests.

* Refactor crypto so each application of it has its own type.

* Introduce new AuthorityProvider API into Aura

This will eventually allow for dynamic determination of authority
keys and avoid having to set them directly on CLI.

* Introduce authority determinator for Babe.

Experiment with modular consensus API.

* Work in progress to introduce KeyTypeId and avoid polluting API
with validator IDs

* Finish up drafting imonline

* Rework offchain workers API.

* Rework API implementation.

* Make it compile for wasm, simplify app_crypto.

* Fix compilation of im-online.

* Fix compilation of im-online.

* Fix more compilation errors.

* Make it compile.

* Fixing tests.

* Rewrite `keystore`

* Fix session tests

* Bring back `TryFrom`'s'

* Fix `srml-grandpa`

* Fix `srml-aura`

* Fix consensus babe

* More fixes

* Make service generate keys from dev_seed

* Build fixes

* Remove offchain tests

* More fixes and cleanups

* Fixes finality grandpa

* Fix `consensus-aura`

* Fix cli

* Fix `node-cli`

* Fix chain_spec builder

* Fix doc tests

* Add authority getter for grandpa.

* Test fix

* Fixes

* Make keystore accessible from the runtime

* Move app crypto to its own crate

* Update `Cargo.lock`

* Make the crypto stuff usable from the runtime

* Adds some runtime crypto tests

* Use last finalized block for grandpa authority

* Fix warning

* Adds `SessionKeys` runtime api

* Remove `FinalityPair` and `ConsensusPair`

* Minor governance tweaks to get it inline with docs.

* Make the governance be up to date with the docs.

* Build fixes.

* Generate the inital session keys

* Failing keystore is a hard error

* Make babe work again

* Fix grandpa

* Fix tests

* Disable `keystore` in consensus critical stuff

* Build fix.

* ImOnline supports multiple authorities at once.

* Update core/application-crypto/src/ed25519.rs

* Merge branch 'master' into gav-in-progress

* Remove unneeded code for now.

* Some `session` testing

* Support querying the public keys

* Cleanup offchain

* Remove warnings

* More cleanup

* Apply suggestions from code review

Co-Authored-By: Benjamin Kampmann <ben.kampmann@googlemail.com>

* More cleanups

* JSONRPC API for setting keys.

Also, rename traits::KeyStore* -> traits::BareCryptoStore*

* Bad merge

* Fix integration tests

* Fix test build

* Test fix

* Fixes

* Warnings

* Another warning

* Bump version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.