From d91027cb73ea7444a19e4b1d21a35ccab48154de Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 25 Jun 2019 12:13:35 +0200 Subject: [PATCH 1/8] generalized uncle processing --- Cargo.lock | 13 ++ Cargo.toml | 1 + core/sr-std/with_std.rs | 1 + core/sr-std/without_std.rs | 1 + srml/authorship/Cargo.toml | 27 +++ srml/authorship/src/lib.rs | 364 +++++++++++++++++++++++++++++++++++++ srml/support/src/lib.rs | 1 + srml/support/src/traits.rs | 15 ++ 8 files changed, 423 insertions(+) create mode 100644 srml/authorship/Cargo.toml create mode 100644 srml/authorship/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 70780e409c170..7c034435aea21 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3450,6 +3450,19 @@ dependencies = [ "substrate-primitives 2.0.0", ] +[[package]] +name = "srml-authorship" +version = "0.1.0" +dependencies = [ + "parity-codec 3.5.1 (registry+https://github.com/rust-lang/crates.io-index)", + "sr-io 2.0.0", + "sr-primitives 2.0.0", + "sr-std 2.0.0", + "srml-support 2.0.0", + "srml-system 2.0.0", + "substrate-primitives 2.0.0", +] + [[package]] name = "srml-babe" version = "2.0.0" diff --git a/Cargo.toml b/Cargo.toml index 2f4139c7afc8f..2a526d14cff2d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -61,6 +61,7 @@ members = [ "srml/support/procedural/tools", "srml/support/procedural/tools/derive", "srml/support/test", + "srml/authorship", "srml/assets", "srml/aura", "srml/balances", diff --git a/core/sr-std/with_std.rs b/core/sr-std/with_std.rs index 5824e26241675..0d5ee04ed51ab 100644 --- a/core/sr-std/with_std.rs +++ b/core/sr-std/with_std.rs @@ -38,4 +38,5 @@ pub use std::vec; pub mod collections { pub use std::collections::btree_map; pub use std::collections::btree_set; + pub use std::collections::vec_deque; } diff --git a/core/sr-std/without_std.rs b/core/sr-std/without_std.rs index db81372c2f070..664aef0d9b14d 100755 --- a/core/sr-std/without_std.rs +++ b/core/sr-std/without_std.rs @@ -74,4 +74,5 @@ pub use core::str; pub mod collections { pub use alloc::collections::btree_map; pub use alloc::collections::btree_set; + pub use alloc::collections::vec_deque; } diff --git a/srml/authorship/Cargo.toml b/srml/authorship/Cargo.toml new file mode 100644 index 0000000000000..adc0b408479f8 --- /dev/null +++ b/srml/authorship/Cargo.toml @@ -0,0 +1,27 @@ +[package] +name = "srml-authorship" +version = "0.1.0" +description = "Block and Uncle Author tracking for the SRML" +authors = ["Parity Technologies "] +edition = "2018" + +[dependencies] +substrate-primitives = { path = "../../core/primitives", default-features = false } +parity-codec = { version = "3.3", default-features = false, features = ["derive"] } +rstd = { package = "sr-std", path = "../../core/sr-std", default-features = false } +primitives = { package = "sr-primitives", path = "../../core/sr-primitives", default-features = false } +srml-support = { path = "../support", default-features = false } +system = { package = "srml-system", path = "../system", default-features = false } +runtime_io = { package = "sr-io", path = "../../core/sr-io", default-features = false } + +[features] +default = ["std"] +std = [ + "parity-codec/std", + "substrate-primitives/std", + "rstd/std", + "srml-support/std", + "primitives/std", + "system/std", + "runtime_io/std", +] diff --git a/srml/authorship/src/lib.rs b/srml/authorship/src/lib.rs new file mode 100644 index 0000000000000..eb0a0c8294bd2 --- /dev/null +++ b/srml/authorship/src/lib.rs @@ -0,0 +1,364 @@ +// Copyright 2019 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + +//! Authorship tracking for SRML runtimes. +//! +//! This tracks the current author of the block and recent uncles. + +use rstd::prelude::*; +use srml_support::{decl_module, decl_storage, StorageValue}; +use srml_support::traits::{FindAuthor, VerifySeal, Get}; +use srml_support::dispatch::Result as DispatchResult; +use parity_codec::{Encode, Decode}; +use system::ensure_none; +use primitives::traits::{SimpleArithmetic, Header as HeaderT, One, Zero}; + +pub trait Trait: system::Trait { + /// Find the author of a block. + type FindAuthor: FindAuthor; + /// The number of blocks back we should accept uncles. + /// This means that we will deal with uncle-parents that are + /// `UncleGenerations + 1` before `now`. + type UncleGenerations: Get; + /// A filter for uncles within a block. This is for implementing + /// further constraints on what uncles can be included, other than their ancestry. + /// + /// For PoW, as long as the seals are checked, there is no need to use anything + /// but the `VerifySeal` implementation as the filter. This is because the cost of making many equivocating + /// uncles is high. + /// + /// For PoS, there is no such limitation, so a further constraint must be imposed + /// beyond a seal check in order to prevent an arbitrary number of + /// equivocating uncles from being included. + /// + /// The `OnePerAuthorPerHeight` filter is good for many slot-based PoS + /// engines. + type FilterUncle: FilterUncle; +} + +/// Additional filtering on uncles that pass preliminary ancestry checks. +/// +/// This should do work such as checking seals +pub trait FilterUncle { + /// An accumulator of data about uncles included. + /// + /// In practice, this is used to validate uncles against others in the same block. + type Accumulator: Default; + + /// Do additional filtering on a seal-checked uncle block, with the accumulated + /// filter. + fn filter_uncle(header: &Header, acc: Self::Accumulator) + -> Result<(Option, Self::Accumulator), &'static str>; +} + +/// A filter on uncles which verifies seals and does no additional checks. +/// This is well-suited to consensus modes such as PoW where the cost of +/// equivocating is high. +pub struct SealVerify(rstd::marker::PhantomData); + +impl> FilterUncle + for SealVerify +{ + type Accumulator = (); + + fn filter_uncle(header: &Header, _acc: ()) + -> Result<(Option, ()), &'static str> + { + T::verify_seal(header).map(|author| (author, ())) + } +} + +/// A filter on uncles which verifies seals and ensures that there is only +/// one uncle included per author per height. +pub struct OnePerAuthorPerHeight(rstd::marker::PhantomData<(T, N)>); + +impl FilterUncle + for OnePerAuthorPerHeight +where + Header: HeaderT + PartialEq, + Author: Clone + PartialEq, + T: VerifySeal, +{ + type Accumulator = Vec<(Header::Number, Option)>; + + fn filter_uncle(header: &Header, mut acc: Self::Accumulator) + -> Result<(Option, Self::Accumulator), &'static str> + { + let author = T::verify_seal(header)?; + let number = header.number(); + + if let Some(ref author) = author { + for &(ref n, ref auth) in &acc { + if (n, auth.as_ref()) == (number, Some(author)) { + return Err("more than one uncle per number per author included"); + } + } + } + + acc.push((number.clone(), author.clone())); + Ok((author, acc)) + } +} + +#[derive(Encode, Decode)] +#[cfg_attr(any(feature = "std", test), derive(PartialEq, Debug))] +enum UncleEntryItem { + InclusionHeight(BlockNumber), + Uncle(Hash, Option), +} + +decl_storage! { + trait Store for Module as Authorship { + /// Uncles + Uncles: Vec>; + /// Author of current block. + Author: T::AccountId; + /// Whether uncles were already set in this block. + DidSetUncles: bool; + } +} + +fn prune_old_uncles( + minimum_height: BlockNumber, + uncles: &mut Vec> +) where BlockNumber: SimpleArithmetic { + let prune_entries = uncles.iter().take_while(|item| match item { + UncleEntryItem::Uncle(_, _) => true, + UncleEntryItem::InclusionHeight(height) => height < &minimum_height, + }); + let prune_index = prune_entries.count(); + + let _ = uncles.drain(..prune_index); +} + +decl_module! { + pub struct Module for enum Call where origin: T::Origin { + fn on_initialize(now: T::BlockNumber) { + let uncle_generations = T::UncleGenerations::get(); + let mut uncles = ::Uncles::get(); + + // prune uncles that are older than the allowed number of generations. + if uncle_generations <= now { + let minimum_height = now - uncle_generations; + prune_old_uncles(minimum_height, &mut uncles) + } + + let digest = >::digest(); + + let pre_runtime_digests = digest.logs.iter().filter_map(|d| d.as_pre_runtime()); + if let Some(author) = T::FindAuthor::find_author(pre_runtime_digests) { + ::Author::put(&author); + } + + ::DidSetUncles::put(false); + } + + fn on_finalize() { + // ensure we never go to trie with this value. + let _ = ::DidSetUncles::take(); + } + + /// Provide a set of uncles. + fn set_uncles(origin, new_uncles: Vec) -> DispatchResult { + use primitives::traits::Header as HeaderT; + + ensure_none(origin)?; + + if ::DidSetUncles::get() { + return Err("Uncles already set in block."); + } + ::DidSetUncles::put(true); + + let now = >::block_number(); + + let minimum_height = { + let uncle_generations = T::UncleGenerations::get(); + if now >= uncle_generations { + now - uncle_generations + } else { + Zero::zero() + } + }; + let mut uncles = ::Uncles::get(); + uncles.push(UncleEntryItem::InclusionHeight(now)); + + let mut acc: >::Accumulator = Default::default(); + + for uncle in new_uncles { + let hash = uncle.hash(); + + if uncle.number() < &One::one() { + return Err("uncle is genesis"); + } + + { + let parent_number = uncle.number().clone() - One::one(); + let parent_hash = >::block_hash(&parent_number); + if &parent_hash != uncle.parent_hash() { + return Err("uncle parent not in chain"); + } + } + + if uncle.number() < &minimum_height { + return Err("uncle not recent enough to be included"); + } + + let duplicate = uncles.iter().find(|entry| match entry { + UncleEntryItem::InclusionHeight(_) => false, + UncleEntryItem::Uncle(h, _) => h == &hash, + }).is_some(); + + let in_chain = >::block_hash(uncle.number()) == hash; + + if duplicate || in_chain { return Err("uncle already included") } + + // check uncle validity. + let (author, temp_acc) = T::FilterUncle::filter_uncle(&uncle, acc)?; + acc = temp_acc; + + // TODO [now]: poke something to note uncle included. + uncles.push(UncleEntryItem::Uncle(hash, author)); + } + + ::Uncles::put(&uncles); + Ok(()) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use runtime_io::with_externalities; + use substrate_primitives::H256; + use primitives::BuildStorage; + use primitives::traits::{BlakeTwo256, IdentityLookup}; + use primitives::testing::Header; + use primitives::generic::DigestItem; + use srml_support::{parameter_types, impl_outer_origin, ConsensusEngineId}; + + impl_outer_origin!{ + pub enum Origin for Test {} + } + + #[derive(Clone, Eq, PartialEq)] + pub struct Test; + + impl system::Trait for Test { + type Origin = Origin; + type Index = u64; + type BlockNumber = u64; + type Hash = H256; + type Hashing = BlakeTwo256; + type AccountId = u64; + type Lookup = IdentityLookup; + type Header = Header; + type Event = (); + } + + const TEST_ID: ConsensusEngineId = [1, 2, 3, 4]; + + pub struct AuthorGiven; + + impl FindAuthor for AuthorGiven { + fn find_author<'a, I>(digests: I) -> Option + where I: 'a + IntoIterator + { + for (id, data) in digests { + if id == TEST_ID { + return u64::decode(&mut &data[..]); + } + } + + None + } + } + + parameter_types! { + pub const UncleGenerations: u64 = 5; + } + + pub struct VerifyBlock; + + // impl VerifySeal for VerifyBlock { + // fn verify_seal(header: &Header) -> Result, &'static str> { + // let pre_runtime_digests = header.digest.logs.iter().filter_map(|d| d.as_pre_runtime()); + // let seals = header.digest.logs.iter().filter_map(|d| d.as_seal()); + + // let author = match AuthorGiven::find_author(pre_runtime_digests) { + // None => return Err("no author"), + // Some(author) => author, + // }; + + // for (id, seal) in seals { + // if id == TEST_ID { + // match u64::decode(&mut &seal[..]) { + // None => return Err("wrong seal"), + // Some(a) => { + // if a != author { + // return Err("wrong author in seal"); + // } + // break + // } + // } + // } + // } + + // Ok(Some(author)) + // } + // } + + impl Trait for Test { + type FindAuthor = AuthorGiven; + //type VerifySeal = VerifyBlock; + type UncleGenerations = UncleGenerations; + } + + fn seal_header(mut header: Header, author: u64) -> Header { + { + let digest = header.digest_mut(); + digest.logs.push(DigestItem::PreRuntime(TEST_ID, author.encode())); + digest.logs.push(DigestItem::Seal(TEST_ID, author.encode())); + } + + header + } + + + fn create_header(number: u64, parent_hash: H256, state_root: H256) -> Header { + Header::new( + number, + Default::default(), + state_root, + parent_hash, + Default::default(), + ) + } + + #[test] + fn prune_old_uncles_works() { + use UncleEntryItem::*; + let mut uncles = vec![ + InclusionHeight(1u32), Uncle((), Some(())), Uncle((), None), Uncle((), None), + InclusionHeight(2u32), Uncle((), None), + InclusionHeight(3u32), Uncle((), None), + ]; + + prune_old_uncles(3, &mut uncles); + + assert_eq!(uncles, vec![InclusionHeight(3), Uncle((), None)]); + } +} diff --git a/srml/support/src/lib.rs b/srml/support/src/lib.rs index ae825397a6ae4..9bf0e93b70f0c 100644 --- a/srml/support/src/lib.rs +++ b/srml/support/src/lib.rs @@ -64,6 +64,7 @@ pub use self::hashable::Hashable; pub use self::dispatch::{Parameter, Dispatchable, Callable, IsSubType}; pub use self::double_map::StorageDoubleMapWithHasher; pub use runtime_io::{print, storage_root}; +pub use runtime_primitives::ConsensusEngineId; /// Macro for easily creating a new implementation of the `Get` trait. Use similarly to /// how you would declare a `const`: diff --git a/srml/support/src/traits.rs b/srml/support/src/traits.rs index 3b3c63e223ce1..2a2af334d733f 100644 --- a/srml/support/src/traits.rs +++ b/srml/support/src/traits.rs @@ -23,6 +23,7 @@ use crate::codec::{Codec, Encode, Decode}; use crate::runtime_primitives::traits::{ MaybeSerializeDebug, SimpleArithmetic }; +use crate::runtime_primitives::ConsensusEngineId; use super::for_each_tuple; @@ -102,6 +103,20 @@ impl MakePayment for () { fn make_payment(_: &T, _: usize) -> Result<(), &'static str> { Ok(()) } } +/// A trait for finding the author of a block header based on the `PreRuntime` digests contained +/// within it. +pub trait FindAuthor { + /// Find the author of a block based on the pre-runtime digests. + fn find_author<'a, I>(digests: I) -> Option + where I: 'a + IntoIterator; +} + +/// A trait for verifying the seal of a header and returning the author. +pub trait VerifySeal { + /// Verify a header and return the author, if any. + fn verify_seal(header: &Header) -> Result, &'static str>; +} + /// Handler for when some currency "account" decreased in balance for /// some reason. /// From 050e3be5802f1919816122562577c9705098d017 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 25 Jun 2019 14:47:41 +0200 Subject: [PATCH 2/8] add some uncle tests --- srml/authorship/src/lib.rs | 266 +++++++++++++++++++++++++++---------- 1 file changed, 193 insertions(+), 73 deletions(-) diff --git a/srml/authorship/src/lib.rs b/srml/authorship/src/lib.rs index eb0a0c8294bd2..ffa563ecfb7b4 100644 --- a/srml/authorship/src/lib.rs +++ b/srml/authorship/src/lib.rs @@ -182,60 +182,66 @@ decl_module! { } ::DidSetUncles::put(true); - let now = >::block_number(); - - let minimum_height = { - let uncle_generations = T::UncleGenerations::get(); - if now >= uncle_generations { - now - uncle_generations - } else { - Zero::zero() - } - }; - let mut uncles = ::Uncles::get(); - uncles.push(UncleEntryItem::InclusionHeight(now)); + Self::verify_and_import_uncles(new_uncles) + } + } +} - let mut acc: >::Accumulator = Default::default(); +impl Module { + fn verify_and_import_uncles(new_uncles: Vec) -> DispatchResult { + let now = >::block_number(); - for uncle in new_uncles { - let hash = uncle.hash(); + let minimum_height = { + let uncle_generations = T::UncleGenerations::get(); + if now >= uncle_generations { + now - uncle_generations + } else { + Zero::zero() + } + }; + let mut uncles = ::Uncles::get(); + uncles.push(UncleEntryItem::InclusionHeight(now)); - if uncle.number() < &One::one() { - return Err("uncle is genesis"); - } + let mut acc: >::Accumulator = Default::default(); - { - let parent_number = uncle.number().clone() - One::one(); - let parent_hash = >::block_hash(&parent_number); - if &parent_hash != uncle.parent_hash() { - return Err("uncle parent not in chain"); - } - } + for uncle in new_uncles { + let hash = uncle.hash(); + + if uncle.number() < &One::one() { + return Err("uncle is genesis"); + } - if uncle.number() < &minimum_height { - return Err("uncle not recent enough to be included"); + { + let parent_number = uncle.number().clone() - One::one(); + let parent_hash = >::block_hash(&parent_number); + if &parent_hash != uncle.parent_hash() { + return Err("uncle parent not in chain"); } + } - let duplicate = uncles.iter().find(|entry| match entry { - UncleEntryItem::InclusionHeight(_) => false, - UncleEntryItem::Uncle(h, _) => h == &hash, - }).is_some(); + if uncle.number() < &minimum_height { + return Err("uncle not recent enough to be included"); + } - let in_chain = >::block_hash(uncle.number()) == hash; + let duplicate = uncles.iter().find(|entry| match entry { + UncleEntryItem::InclusionHeight(_) => false, + UncleEntryItem::Uncle(h, _) => h == &hash, + }).is_some(); - if duplicate || in_chain { return Err("uncle already included") } + let in_chain = >::block_hash(uncle.number()) == hash; - // check uncle validity. - let (author, temp_acc) = T::FilterUncle::filter_uncle(&uncle, acc)?; - acc = temp_acc; + if duplicate || in_chain { return Err("uncle already included") } - // TODO [now]: poke something to note uncle included. - uncles.push(UncleEntryItem::Uncle(hash, author)); - } + // check uncle validity. + let (author, temp_acc) = T::FilterUncle::filter_uncle(&uncle, acc)?; + acc = temp_acc; - ::Uncles::put(&uncles); - Ok(()) + // TODO [now]: poke something to note uncle included. + uncles.push(UncleEntryItem::Uncle(hash, author)); } + + ::Uncles::put(&uncles); + Ok(()) } } @@ -243,7 +249,7 @@ decl_module! { mod tests { use super::*; use runtime_io::with_externalities; - use substrate_primitives::H256; + use substrate_primitives::{H256, Blake2Hasher}; use primitives::BuildStorage; use primitives::traits::{BlakeTwo256, IdentityLookup}; use primitives::testing::Header; @@ -269,6 +275,15 @@ mod tests { type Event = (); } + impl Trait for Test { + type FindAuthor = AuthorGiven; + type UncleGenerations = UncleGenerations; + type FilterUncle = SealVerify; + } + + type System = system::Module; + type Authorship = Module; + const TEST_ID: ConsensusEngineId = [1, 2, 3, 4]; pub struct AuthorGiven; @@ -293,38 +308,32 @@ mod tests { pub struct VerifyBlock; - // impl VerifySeal for VerifyBlock { - // fn verify_seal(header: &Header) -> Result, &'static str> { - // let pre_runtime_digests = header.digest.logs.iter().filter_map(|d| d.as_pre_runtime()); - // let seals = header.digest.logs.iter().filter_map(|d| d.as_seal()); - - // let author = match AuthorGiven::find_author(pre_runtime_digests) { - // None => return Err("no author"), - // Some(author) => author, - // }; - - // for (id, seal) in seals { - // if id == TEST_ID { - // match u64::decode(&mut &seal[..]) { - // None => return Err("wrong seal"), - // Some(a) => { - // if a != author { - // return Err("wrong author in seal"); - // } - // break - // } - // } - // } - // } - - // Ok(Some(author)) - // } - // } + impl VerifySeal for VerifyBlock { + fn verify_seal(header: &Header) -> Result, &'static str> { + let pre_runtime_digests = header.digest.logs.iter().filter_map(|d| d.as_pre_runtime()); + let seals = header.digest.logs.iter().filter_map(|d| d.as_seal()); - impl Trait for Test { - type FindAuthor = AuthorGiven; - //type VerifySeal = VerifyBlock; - type UncleGenerations = UncleGenerations; + let author = match AuthorGiven::find_author(pre_runtime_digests) { + None => return Err("no author"), + Some(author) => author, + }; + + for (id, seal) in seals { + if id == TEST_ID { + match u64::decode(&mut &seal[..]) { + None => return Err("wrong seal"), + Some(a) => { + if a != author { + return Err("wrong author in seal"); + } + break + } + } + } + } + + Ok(Some(author)) + } } fn seal_header(mut header: Header, author: u64) -> Header { @@ -348,6 +357,11 @@ mod tests { ) } + fn new_test_ext() -> runtime_io::TestExternalities { + let mut t = system::GenesisConfig::::default().build_storage().unwrap().0; + t.into() + } + #[test] fn prune_old_uncles_works() { use UncleEntryItem::*; @@ -361,4 +375,110 @@ mod tests { assert_eq!(uncles, vec![InclusionHeight(3), Uncle((), None)]); } + + #[test] + fn rejects_bad_uncles() { + with_externalities(&mut new_test_ext(), || { + let author_a = 69; + + struct CanonChain { + inner: Vec
, + } + + impl CanonChain { + fn best_hash(&self) -> H256 { + self.inner.last().unwrap().hash() + } + + fn canon_hash(&self, index: usize) -> H256 { + self.inner[index].hash() + } + + fn header(&self, index: usize) -> &Header { + &self.inner[index] + } + + fn push(&mut self, header: Header) { + self.inner.push(header) + } + } + + let mut canon_chain = CanonChain { + inner: vec![seal_header(create_header(0, Default::default(), Default::default()), 999)], + }; + + for number in 1..7 { + System::initialize(&number, &canon_chain.best_hash(), &Default::default(), &Default::default()); + let header = seal_header(System::finalize(), author_a); + canon_chain.push(header); + } + + // initialize so system context is set up correctly. + System::initialize(&8, &canon_chain.best_hash(), &Default::default(), &Default::default()); + + // 2 of the same uncle at once + { + let uncle_a = seal_header( + create_header(3, canon_chain.canon_hash(2), [1; 32].into()), + author_a, + ); + assert_eq!( + Authorship::verify_and_import_uncles(vec![uncle_a.clone(), uncle_a.clone()]), + Err("uncle already included"), + ); + } + + // 2 of the same uncle at different times. + { + let uncle_a = seal_header( + create_header(3, canon_chain.canon_hash(2), [1; 32].into()), + author_a, + ); + + assert!( + Authorship::verify_and_import_uncles(vec![uncle_a.clone()]).is_ok(); + ); + + assert_eq!( + Authorship::verify_and_import_uncles(vec![uncle_a.clone()]), + Err("uncle already included"), + ); + } + + // same uncle as ancestor. + { + let uncle_clone = canon_chain.header(5).clone(); + + assert_eq!( + Authorship::verify_and_import_uncles(vec![uncle_clone]), + Err("uncle already included"), + ); + } + + // uncle without valid seal. + { + let unsealed = create_header(3, canon_chain.canon_hash(2), [2; 32].into()); + assert_eq!( + Authorship::verify_and_import_uncles(vec![unsealed]), + Err("no author"), + ); + } + + // old uncles can't get in. + { + assert_eq!(System::block_number(), 8); + assert_eq!(::UncleGenerations::get(), 5); + + let gen_2 = seal_header( + create_header(2, canon_chain.canon_hash(1), [3; 32].into()), + author_a, + ); + + assert_eq!( + Authorship::verify_and_import_uncles(vec![gen_2]), + Err("uncle not recent enough to be included"), + ); + } + }); + } } From 010e7399c463f77c86e037bd3dc4e78e42c1cdd5 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 25 Jun 2019 17:05:33 +0200 Subject: [PATCH 3/8] set author and do event handling --- srml/authorship/src/lib.rs | 103 +++++++++++++++++++++++++++++++------ 1 file changed, 86 insertions(+), 17 deletions(-) diff --git a/srml/authorship/src/lib.rs b/srml/authorship/src/lib.rs index ffa563ecfb7b4..2fa213170402a 100644 --- a/srml/authorship/src/lib.rs +++ b/srml/authorship/src/lib.rs @@ -47,6 +47,23 @@ pub trait Trait: system::Trait { /// The `OnePerAuthorPerHeight` filter is good for many slot-based PoS /// engines. type FilterUncle: FilterUncle; + /// An event handler for authored blocks. + type EventHandler: EventHandler; +} + +/// An event handler for the authorship module. +pub trait EventHandler { + /// Note that the given account ID is the author of the current block. + fn note_author(author: Author); + + /// Note that the given account ID authored the given uncle, and how many + /// blocks older than the current block it is (>= 0 -- siblings are allowed) + fn note_uncle(author: Author, age: BlockNumber); +} + +impl EventHandler for () { + fn note_author(_author: A) { } + fn note_uncle(_author: A, _age: B) { } } /// Additional filtering on uncles that pass preliminary ancestry checks. @@ -125,7 +142,7 @@ decl_storage! { /// Uncles Uncles: Vec>; /// Author of current block. - Author: T::AccountId; + Author: Option; /// Whether uncles were already set in this block. DidSetUncles: bool; } @@ -156,25 +173,19 @@ decl_module! { prune_old_uncles(minimum_height, &mut uncles) } - let digest = >::digest(); - - let pre_runtime_digests = digest.logs.iter().filter_map(|d| d.as_pre_runtime()); - if let Some(author) = T::FindAuthor::find_author(pre_runtime_digests) { - ::Author::put(&author); - } - ::DidSetUncles::put(false); + + T::EventHandler::note_author(Self::author()); } fn on_finalize() { - // ensure we never go to trie with this value. + // ensure we never go to trie with these values. + let _ = ::Author::take(); let _ = ::DidSetUncles::take(); } /// Provide a set of uncles. fn set_uncles(origin, new_uncles: Vec) -> DispatchResult { - use primitives::traits::Header as HeaderT; - ensure_none(origin)?; if ::DidSetUncles::get() { @@ -188,17 +199,39 @@ decl_module! { } impl Module { + /// Fetch the author of the block. + /// + /// This is safe to invoke `on_initialize` implementations as well as afterwards. + pub fn author() -> T::AccountId { + // Check the memoized storage value. + if let Some(author) = ::Author::get() { + return author; + } + + let digest = >::digest(); + let pre_runtime_digests = digest.logs.iter().filter_map(|d| d.as_pre_runtime()); + if let Some(author) = T::FindAuthor::find_author(pre_runtime_digests) { + ::Author::put(&author); + author + } else { + Default::default() + } + } + fn verify_and_import_uncles(new_uncles: Vec) -> DispatchResult { let now = >::block_number(); - let minimum_height = { + let (minimum_height, maximum_height) = { let uncle_generations = T::UncleGenerations::get(); - if now >= uncle_generations { + let min = if now >= uncle_generations { now - uncle_generations } else { Zero::zero() - } + }; + + (min, now) }; + let mut uncles = ::Uncles::get(); uncles.push(UncleEntryItem::InclusionHeight(now)); @@ -211,6 +244,10 @@ impl Module { return Err("uncle is genesis"); } + if uncle.number() > &maximum_height { + return Err("uncles too high in chain"); + } + { let parent_number = uncle.number().clone() - One::one(); let parent_hash = >::block_hash(&parent_number); @@ -236,7 +273,10 @@ impl Module { let (author, temp_acc) = T::FilterUncle::filter_uncle(&uncle, acc)?; acc = temp_acc; - // TODO [now]: poke something to note uncle included. + T::EventHandler::note_uncle( + author.clone().unwrap_or_default(), + now - uncle.number().clone(), + ); uncles.push(UncleEntryItem::Uncle(hash, author)); } @@ -279,6 +319,7 @@ mod tests { type FindAuthor = AuthorGiven; type UncleGenerations = UncleGenerations; type FilterUncle = SealVerify; + type EventHandler = (); } type System = system::Module; @@ -358,7 +399,7 @@ mod tests { } fn new_test_ext() -> runtime_io::TestExternalities { - let mut t = system::GenesisConfig::::default().build_storage().unwrap().0; + let t = system::GenesisConfig::::default().build_storage().unwrap().0; t.into() } @@ -407,7 +448,7 @@ mod tests { inner: vec![seal_header(create_header(0, Default::default(), Default::default()), 999)], }; - for number in 1..7 { + for number in 1..8 { System::initialize(&number, &canon_chain.best_hash(), &Default::default(), &Default::default()); let header = seal_header(System::finalize(), author_a); canon_chain.push(header); @@ -479,6 +520,34 @@ mod tests { Err("uncle not recent enough to be included"), ); } + + // siblings are also allowed + { + let other_8 = seal_header( + create_header(8, canon_chain.canon_hash(7), [1; 32].into()), + author_a, + ); + + assert!( + Authorship::verify_and_import_uncles(vec![other_8]).is_ok(); + ); + } + }); + } + + #[test] + fn sets_author_lazily() { + with_externalities(&mut new_test_ext(), || { + let author = 42; + let mut header = seal_header( + create_header(1, Default::default(), [1; 32].into()), + author, + ); + + header.digest_mut().pop(); // pop the seal off. + System::initialize(&1, &Default::default(), &Default::default(), header.digest()); + + assert_eq!(Authorship::author(), author); }); } } From b030b7c181867e3ced446a6ec10eb0d684461ad1 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 25 Jun 2019 17:52:47 +0200 Subject: [PATCH 4/8] OnePerAuthorPerHeight no longer O(n^2) and test --- srml/authorship/src/lib.rs | 62 +++++++++++++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 7 deletions(-) diff --git a/srml/authorship/src/lib.rs b/srml/authorship/src/lib.rs index 2fa213170402a..b87bf09bf59f4 100644 --- a/srml/authorship/src/lib.rs +++ b/srml/authorship/src/lib.rs @@ -19,6 +19,7 @@ //! This tracks the current author of the block and recent uncles. use rstd::prelude::*; +use rstd::collections::btree_set::BTreeSet; use srml_support::{decl_module, decl_storage, StorageValue}; use srml_support::traits::{FindAuthor, VerifySeal, Get}; use srml_support::dispatch::Result as DispatchResult; @@ -100,16 +101,19 @@ impl> FilterUncle /// A filter on uncles which verifies seals and ensures that there is only /// one uncle included per author per height. +/// +/// This does O(n^2) work in the number of uncles included. pub struct OnePerAuthorPerHeight(rstd::marker::PhantomData<(T, N)>); impl FilterUncle for OnePerAuthorPerHeight where Header: HeaderT + PartialEq, - Author: Clone + PartialEq, + Header::Number: Ord, + Author: Clone + PartialEq + Ord, T: VerifySeal, { - type Accumulator = Vec<(Header::Number, Option)>; + type Accumulator = BTreeSet<(Header::Number, Author)>; fn filter_uncle(header: &Header, mut acc: Self::Accumulator) -> Result<(Option, Self::Accumulator), &'static str> @@ -118,14 +122,11 @@ where let number = header.number(); if let Some(ref author) = author { - for &(ref n, ref auth) in &acc { - if (n, auth.as_ref()) == (number, Some(author)) { - return Err("more than one uncle per number per author included"); - } + if !acc.insert((number.clone(), author.clone())) { + return Err("more than one uncle per number per author included"); } } - acc.push((number.clone(), author.clone())); Ok((author, acc)) } } @@ -550,4 +551,51 @@ mod tests { assert_eq!(Authorship::author(), author); }); } + + #[test] + fn one_uncle_per_author_per_number() { + type Filter = OnePerAuthorPerHeight; + + let author_a = 42; + let author_b = 43; + + let mut acc: Option<>::Accumulator> = Some(Default::default()); + let header_a1 = seal_header( + create_header(1, Default::default(), [1; 32].into()), + author_a, + ); + let header_b1 = seal_header( + create_header(1, Default::default(), [1; 32].into()), + author_b, + ); + + let header_a2_1 = seal_header( + create_header(2, Default::default(), [1; 32].into()), + author_a, + ); + let header_a2_2 = seal_header( + create_header(2, Default::default(), [2; 32].into()), + author_a, + ); + + let mut check_filter = move |uncle| { + match Filter::filter_uncle(uncle, acc.take().unwrap()) { + Ok((author, a)) => { + acc = Some(a); + Ok(author) + } + Err(e) => Err(e), + } + }; + + // same height, different author is OK. + assert_eq!(check_filter(&header_a1), Ok(Some(author_a))); + assert_eq!(check_filter(&header_b1), Ok(Some(author_b))); + + // same author, different height. + assert_eq!(check_filter(&header_a2_1), Ok(Some(author_a))); + + // same author, same height (author a, height 2) + assert!(check_filter(&header_a2_2).is_err()); + } } From 0aff8a4bc83b5ec618c3d2bfac8d82f2f0d0d600 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 25 Jun 2019 18:06:08 +0200 Subject: [PATCH 5/8] bump impl_version of node --- node/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/runtime/src/lib.rs b/node/runtime/src/lib.rs index 15b81b4cc53e1..676aac5e72cc5 100644 --- a/node/runtime/src/lib.rs +++ b/node/runtime/src/lib.rs @@ -59,7 +59,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { impl_name: create_runtime_str!("substrate-node"), authoring_version: 10, spec_version: 95, - impl_version: 95, + impl_version: 96, apis: RUNTIME_API_VERSIONS, }; From de96df073acaf4cc7b74c6b58d7a10cf4d2cc005 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 25 Jun 2019 22:18:52 +0200 Subject: [PATCH 6/8] Documentation and style fixes Co-Authored-By: DemiMarie-parity <48690212+DemiMarie-parity@users.noreply.github.com> --- srml/authorship/src/lib.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/srml/authorship/src/lib.rs b/srml/authorship/src/lib.rs index b87bf09bf59f4..4e1fd3a57e677 100644 --- a/srml/authorship/src/lib.rs +++ b/srml/authorship/src/lib.rs @@ -52,13 +52,14 @@ pub trait Trait: system::Trait { type EventHandler: EventHandler; } -/// An event handler for the authorship module. +/// An event handler for the authorship module. There is a dummy implementation +/// for `()`, which does nothing. pub trait EventHandler { /// Note that the given account ID is the author of the current block. fn note_author(author: Author); /// Note that the given account ID authored the given uncle, and how many - /// blocks older than the current block it is (>= 0 -- siblings are allowed) + /// blocks older than the current block it is (age >= 0, so siblings are allowed) fn note_uncle(author: Author, age: BlockNumber); } @@ -102,7 +103,7 @@ impl> FilterUncle /// A filter on uncles which verifies seals and ensures that there is only /// one uncle included per author per height. /// -/// This does O(n^2) work in the number of uncles included. +/// This does O(n log n) work in the number of uncles included. pub struct OnePerAuthorPerHeight(rstd::marker::PhantomData<(T, N)>); impl FilterUncle @@ -202,7 +203,8 @@ decl_module! { impl Module { /// Fetch the author of the block. /// - /// This is safe to invoke `on_initialize` implementations as well as afterwards. + /// This is safe to invoke in `on_initialize` implementations, as well + /// as afterwards. pub fn author() -> T::AccountId { // Check the memoized storage value. if let Some(author) = ::Author::get() { From c98b599e5c0b85f957d7b17d3a5a2ac13dc33515 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 25 Jun 2019 22:25:52 +0200 Subject: [PATCH 7/8] fix #2949: index-based FindAuthor wrapper for srml-session --- srml/session/src/lib.rs | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/srml/session/src/lib.rs b/srml/session/src/lib.rs index 3ae7c9801299b..e2f4078b06110 100644 --- a/srml/session/src/lib.rs +++ b/srml/session/src/lib.rs @@ -34,7 +34,7 @@ //! - **Session key:** A session key is actually several keys kept together that provide the various signing //! functions required by network authorities/validators in pursuit of their duties. //! - **Session key configuration process:** A session key is set using `set_key` for use in the -//! next session. It is stored in `NextKeyFor`, a mapping between the caller's `AccountID` and the session +//! next session. It is stored in `NextKeyFor`, a mapping between the caller's `AccountId` and the session //! key provided. `set_key` allows users to set their session key prior to becoming a validator. //! It is a public call since it uses `ensure_signed`, which checks that the origin is a signed account. //! As such, the account ID of the origin stored in in `NextKeyFor` may not necessarily be associated with @@ -120,8 +120,11 @@ use rstd::{prelude::*, marker::PhantomData, ops::Rem}; use rstd::alloc::borrow::ToOwned; use parity_codec::Decode; use primitives::traits::{Zero, Saturating, Member, OpaqueKeys}; -use srml_support::{StorageValue, StorageMap, for_each_tuple, decl_module, decl_event, decl_storage}; -use srml_support::{ensure, traits::{OnFreeBalanceZero, Get}, Parameter, print}; +use srml_support::{ + ConsensusEngineId, StorageValue, StorageMap, for_each_tuple, decl_module, + decl_event, decl_storage, +}; +use srml_support::{ensure, traits::{OnFreeBalanceZero, Get, FindAuthor}, Parameter, print}; use system::ensure_signed; /// Simple index type with which we can count sessions. @@ -377,6 +380,24 @@ impl OnFreeBalanceZero for Module { } } +/// Wraps the author-scraping logic for consensus engines that can recover +/// the canonical index of an author. This then transforms it into the +/// registering account-ID of that session key index. +pub struct FindAccountFromAuthorIndex(rstd::marker::PhantomData<(T, Inner)>); + +impl> FindAuthor + for FindAccountFromAuthorIndex +{ + fn find_author<'a, I>(digests: I) -> Option + where I: 'a + IntoIterator + { + let i = Inner::find_author(digests)?; + + let validators = >::validators(); + validators.get(i as usize).map(|k| k.clone()) + } +} + #[cfg(test)] mod tests { use super::*; From 0aff8241a118c915a13e9f2a8bb0a5c5a353f2ca Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 26 Jun 2019 11:57:23 +0200 Subject: [PATCH 8/8] use for_each_tuple --- srml/authorship/src/lib.rs | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/srml/authorship/src/lib.rs b/srml/authorship/src/lib.rs index 4e1fd3a57e677..42deeceb628d4 100644 --- a/srml/authorship/src/lib.rs +++ b/srml/authorship/src/lib.rs @@ -20,7 +20,7 @@ use rstd::prelude::*; use rstd::collections::btree_set::BTreeSet; -use srml_support::{decl_module, decl_storage, StorageValue}; +use srml_support::{decl_module, decl_storage, for_each_tuple, StorageValue}; use srml_support::traits::{FindAuthor, VerifySeal, Get}; use srml_support::dispatch::Result as DispatchResult; use parity_codec::{Encode, Decode}; @@ -63,11 +63,30 @@ pub trait EventHandler { fn note_uncle(author: Author, age: BlockNumber); } -impl EventHandler for () { - fn note_author(_author: A) { } - fn note_uncle(_author: A, _age: B) { } +macro_rules! impl_event_handler { + () => ( + impl EventHandler for () { + fn note_author(_author: A) { } + fn note_uncle(_author: A, _age: B) { } + } + ); + + ( $($t:ident)* ) => { + impl),*> + EventHandler for ($($t,)*) + { + fn note_author(author: Author) { + $($t::note_author(author.clone());)* + } + fn note_uncle(author: Author, age: BlockNumber) { + $($t::note_uncle(author.clone(), age.clone());)* + } + } + } } +for_each_tuple!(impl_event_handler); + /// Additional filtering on uncles that pass preliminary ancestry checks. /// /// This should do work such as checking seals