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

srml/authority-discovery: Abstract session key type #3698

Merged
andresilva merged 7 commits into from
Oct 1, 2019
Merged

srml/authority-discovery: Abstract session key type #3698

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
da0a9e0
srml/authority-discovery: Abstract session key type
mxinden Sep 26, 2019
2181d2c
srml/authority-discovery: Fix line length
mxinden Sep 26, 2019
b3eae71
srml/authority-discovery/Cargo: Move babe to dev-dependencies
mxinden Sep 27, 2019
82de19a
Merge remote-tracking branch 'paritytech/master' into authority-disco…
mxinden Sep 27, 2019
a2ea720
node/runtime: Bump implementation version
mxinden Sep 27, 2019
9d6b4c2
srml/authority-discovery: Add doc comment for authority discovery Trait
mxinden Sep 30, 2019
44b5060
Merge remote-tracking branch 'paritytech/master' into authority-disco…
mxinden Oct 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 8 additions & 6 deletions node/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use node_primitives::{
AccountId, AccountIndex, Balance, BlockNumber, Hash, Index,
Moment, Signature, ContractExecResult,
};
use babe_primitives::{AuthorityId as BabeId};
use babe_primitives::{AuthorityId as BabeId, AuthoritySignature as BabeSignature};
use grandpa::fg_primitives;
use client::{
block_builder::api::{self as block_builder_api, InherentData, CheckInherentsResult},
Expand All @@ -50,7 +50,7 @@ use elections::VoteIndex;
use version::NativeVersion;
use primitives::OpaqueMetadata;
use grandpa::{AuthorityId as GrandpaId, AuthorityWeight as GrandpaWeight};
use im_online::sr25519::{AuthorityId as ImOnlineId, AuthoritySignature as ImOnlineSignature};
use im_online::sr25519::{AuthorityId as ImOnlineId};
use authority_discovery_primitives::{AuthorityId as EncodedAuthorityId, Signature as EncodedSignature};
use codec::{Encode, Decode};
use system::offchain::TransactionSubmitter;
Expand Down Expand Up @@ -85,7 +85,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
// implementation changes and behavior does not, then leave spec_version as
// is and increment impl_version.
spec_version: 165,
Copy link
Contributor

@andresilva andresilva Sep 30, 2019

Choose a reason for hiding this comment

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

Shouldn't the spec version be increased (and not impl version)?

Copy link
Contributor Author

@mxinden mxinden Sep 30, 2019

Choose a reason for hiding this comment

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

@andresilva do I understand correctly that we increase the spec_version whenever we have breaking changes visible only outside of the runtime boundary?

This patch set does not touch the core/authority-discovery/primitives::AuthorityDiscoveryApi, thus as far as I can tell it is not visible outside of the runtime boundary. Am I missing something?

Copy link
Member

@bkchr bkchr Sep 30, 2019

Choose a reason for hiding this comment

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

Given that we switch to Babe as authority id, @andresilva is right. You change the behavior of runtime and this new runtime would not be compatible with the old runtime when it comes to authority discovery.

Copy link
Contributor

@andresilva andresilva Sep 30, 2019

Choose a reason for hiding this comment

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

My vague definition for when to increment spec_version is a bit stricter than that, since internal behavior of the module can change without any changes being introduced at the boundaries. Keeping the same spec_version means that a previous compiled blob of the runtime without these changes can be executed as is.

impl_version: 165,
impl_version: 166,
apis: RUNTIME_API_VERSIONS,
};

Expand Down Expand Up @@ -436,7 +436,9 @@ impl offences::Trait for Runtime {
type OnOffenceHandler = Staking;
}

impl authority_discovery::Trait for Runtime {}
impl authority_discovery::Trait for Runtime {
type AuthorityId = BabeId;
}

impl grandpa::Trait for Runtime {
type Event = Event;
Expand Down Expand Up @@ -634,12 +636,12 @@ impl_runtime_apis! {
}

fn verify(payload: &Vec<u8>, signature: &EncodedSignature, authority_id: &EncodedAuthorityId) -> bool {
let signature = match ImOnlineSignature::decode(&mut &signature.0[..]) {
let signature = match BabeSignature::decode(&mut &signature.0[..]) {
Ok(s) => s,
_ => return false,
};

let authority_id = match ImOnlineId::decode(&mut &authority_id.0[..]) {
let authority_id = match BabeId::decode(&mut &authority_id.0[..]) {
Ok(id) => id,
_ => return false,
};
Expand Down
16 changes: 8 additions & 8 deletions srml/authority-discovery/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,16 @@ authors = ["Parity Technologies <[email protected]>"]
edition = "2018"

[dependencies]
app-crypto = { package = "substrate-application-crypto", path = "../../core/application-crypto", default-features = false }
babe-primitives = { package = "substrate-consensus-babe-primitives", path = "../../core/consensus/babe/primitives", default-features = false }
Copy link
Member

@bkchr bkchr Sep 27, 2019

Choose a reason for hiding this comment

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

This needs to go into dev-dependencies. You don't need it for the normal build.

Copy link
Member

@bkchr bkchr Sep 27, 2019

Choose a reason for hiding this comment

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

.

codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false, features = ["derive"] }
sr-primitives = { path = "../../core/sr-primitives", default-features = false }
primitives = { package = "substrate-primitives", path = "../../core/primitives", default-features = false }
app-crypto = { package = "substrate-application-crypto", path = "../../core/application-crypto", default-features = false }
rstd = { package = "sr-std", path = "../../core/sr-std", default-features = false }
runtime-io = { package = "sr-io", path = "../../core/sr-io", default-features = false }
serde = { version = "1.0", optional = true }
session = { package = "srml-session", path = "../session", default-features = false }
im-online = { package = "srml-im-online", path = "../im-online", default-features = false }
sr-primitives = { path = "../../core/sr-primitives", default-features = false }
support = { package = "srml-support", path = "../support", default-features = false }
runtime-io = { package = "sr-io", path = "../../core/sr-io", default-features = false }
system = { package = "srml-system", path = "../system", default-features = false }

[dev-dependencies]
Expand All @@ -23,15 +23,15 @@ sr-staking-primitives = { path = "../../core/sr-staking-primitives", default-fea
[features]
default = ["std"]
std = [
"app-crypto/std",
"babe-primitives/std",
"codec/std",
"sr-primitives/std",
"primitives/std",
"rstd/std",
"runtime-io/std",
"serde",
"session/std",
"im-online/std",
"sr-primitives/std",
"support/std",
"runtime-io/std",
"system/std",
"app-crypto/std",
]
72 changes: 35 additions & 37 deletions srml/authority-discovery/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,29 +22,28 @@
//!
//! ## Dependencies
//!
//! This module depends on the [I’m online module](../srml_im_online/index.html)
//! using its session key.
//! This module depends on an externally defined session key type, specified via
//! `Trait::AuthorityId` in the respective node runtime implementation.

// Ensure we're `no_std` when compiling for Wasm.
#![cfg_attr(not(feature = "std"), no_std)]

use app_crypto::RuntimeAppPublic;
use codec::{Decode, Encode};
use rstd::prelude::*;
use support::{decl_module, decl_storage};

pub trait Trait: system::Trait + session::Trait + im_online::Trait {}

type AuthorityIdFor<T> = <T as im_online::Trait>::AuthorityId;
type AuthoritySignatureFor<T> =
<<T as im_online::Trait>::AuthorityId as RuntimeAppPublic>::Signature;
pub trait Trait: system::Trait + session::Trait {
type AuthorityId: RuntimeAppPublic + Default + Decode + Encode + PartialEq;
Copy link
Contributor

@rphmeier rphmeier Sep 30, 2019

Choose a reason for hiding this comment

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

needs docs

}

decl_storage! {
trait Store for Module<T: Trait> as AuthorityDiscovery {
/// The current set of keys that may issue a heartbeat.
Keys get(keys): Vec<AuthorityIdFor<T>>;
Keys get(keys): Vec<T::AuthorityId>;
}
add_extra_genesis {
config(keys): Vec<AuthorityIdFor<T>>;
config(keys): Vec<T::AuthorityId>;
build(|config| Module::<T>::initialize_keys(&config.keys))
}
}
Expand All @@ -59,10 +58,10 @@ impl<T: Trait> Module<T> {
/// set, otherwise this function returns None. The restriction might be
/// softened in the future in case a consumer needs to learn own authority
/// identifier.
fn authority_id() -> Option<AuthorityIdFor<T>> {
fn authority_id() -> Option<T::AuthorityId> {
let authorities = Keys::<T>::get();

let local_keys = <AuthorityIdFor<T>>::all();
let local_keys = T::AuthorityId::all();

authorities.into_iter().find_map(|authority| {
if local_keys.contains(&authority) {
Expand All @@ -74,12 +73,17 @@ impl<T: Trait> Module<T> {
}

/// Retrieve authority identifiers of the current authority set.
pub fn authorities() -> Vec<AuthorityIdFor<T>> {
pub fn authorities() -> Vec<T::AuthorityId> {
Keys::<T>::get()
}

/// Sign the given payload with the private key corresponding to the given authority id.
pub fn sign(payload: &Vec<u8>) -> Option<(AuthoritySignatureFor<T>, AuthorityIdFor<T>)> {
pub fn sign(
payload: &Vec<u8>,
) -> Option<(
<<T as Trait>::AuthorityId as RuntimeAppPublic>::Signature,
T::AuthorityId,
)> {
let authority_id = Module::<T>::authority_id()?;
authority_id.sign(payload).map(|s| (s, authority_id))
}
Expand All @@ -88,13 +92,13 @@ impl<T: Trait> Module<T> {
/// authority identifier.
pub fn verify(
payload: &Vec<u8>,
signature: AuthoritySignatureFor<T>,
authority_id: AuthorityIdFor<T>,
signature: <<T as Trait>::AuthorityId as RuntimeAppPublic>::Signature,
authority_id: T::AuthorityId,
) -> bool {
authority_id.verify(payload, &signature)
}

fn initialize_keys(keys: &[AuthorityIdFor<T>]) {
fn initialize_keys(keys: &[T::AuthorityId]) {
if !keys.is_empty() {
assert!(Keys::<T>::get().is_empty(), "Keys are already initialized!");
Keys::<T>::put_ref(keys);
Expand All @@ -103,7 +107,7 @@ impl<T: Trait> Module<T> {
}

impl<T: Trait> session::OneSessionHandler<T::AccountId> for Module<T> {
type Key = AuthorityIdFor<T>;
type Key = T::AuthorityId;

fn on_genesis_session<'a, I: 'a>(validators: I)
where
Expand Down Expand Up @@ -144,9 +148,11 @@ mod tests {

#[derive(Clone, Eq, PartialEq)]
pub struct Test;
impl Trait for Test {}
impl Trait for Test {
type AuthorityId = babe_primitives::AuthorityId;
}

type AuthorityId = im_online::sr25519::AuthorityId;
type AuthorityId = babe_primitives::AuthorityId;

pub struct TestOnSessionEnding;
impl session::OnSessionEnding<AuthorityId> for TestOnSessionEnding {
Expand Down Expand Up @@ -176,18 +182,6 @@ mod tests {
type FullIdentificationOf = ();
}

impl im_online::Trait for Test {
type AuthorityId = AuthorityId;
type Call = im_online::Call<Test>;
type Event = ();
type SubmitTransaction = system::offchain::TransactionSubmitter<
(),
im_online::Call<Test>,
UncheckedExtrinsic<(), im_online::Call<Test>, (), ()>,
>;
type ReportUnresponsiveness = ();
}

pub type BlockNumber = u64;

parameter_types! {
Expand Down Expand Up @@ -243,13 +237,13 @@ mod tests {
let key_store = KeyStore::new();
key_store
.write()
.sr25519_generate_new(key_types::IM_ONLINE, None)
.sr25519_generate_new(key_types::BABE, None)
.expect("Generates key.");

// Retrieve key to later check if we got the right one.
let public_key = key_store
.read()
.sr25519_public_keys(key_types::IM_ONLINE)
.sr25519_public_keys(key_types::BABE)
.pop()
.unwrap();
let authority_id = AuthorityId::from(public_key);
Expand Down Expand Up @@ -283,7 +277,7 @@ mod tests {
let key_store = KeyStore::new();
key_store
.write()
.sr25519_generate_new(key_types::IM_ONLINE, None)
.sr25519_generate_new(key_types::BABE, None)
.expect("Generates key.");

// Build genesis.
Expand Down Expand Up @@ -317,13 +311,13 @@ mod tests {
let key_store = KeyStore::new();
key_store
.write()
.sr25519_generate_new(key_types::IM_ONLINE, None)
.sr25519_generate_new(key_types::BABE, None)
.expect("Generates key.");

// Retrieve key to later check if we got the right one.
let public_key = key_store
.read()
.sr25519_public_keys(key_types::IM_ONLINE)
.sr25519_public_keys(key_types::BABE)
.pop()
.unwrap();
let authority_id = AuthorityId::from(public_key);
Expand All @@ -347,7 +341,11 @@ mod tests {
let payload = String::from("test payload").into_bytes();
let (sig, authority_id) = AuthorityDiscovery::sign(&payload).expect("signature");

assert!(AuthorityDiscovery::verify(&payload, sig.clone(), authority_id.clone(),));
assert!(AuthorityDiscovery::verify(
&payload,
sig.clone(),
authority_id.clone(),
));

assert!(!AuthorityDiscovery::verify(
&String::from("other payload").into_bytes(),
Expand Down