From c3af86cee45d98dd5f97fb2de419ba4e25ac14b7 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Mon, 20 Jan 2020 12:57:49 +0100 Subject: [PATCH 01/19] Move DummySpecialization to sc-network (#4680) --- client/network/src/protocol/specialization.rs | 26 ++++++++++++++++++ client/network/test/src/lib.rs | 27 +------------------ 2 files changed, 27 insertions(+), 26 deletions(-) diff --git a/client/network/src/protocol/specialization.rs b/client/network/src/protocol/specialization.rs index 9b1452160e535..af6d5f7a239cd 100644 --- a/client/network/src/protocol/specialization.rs +++ b/client/network/src/protocol/specialization.rs @@ -57,6 +57,32 @@ pub trait NetworkSpecialization: Send + Sync + 'static { fn on_block_imported(&mut self, _ctx: &mut dyn Context, _hash: B::Hash, _header: &B::Header) { } } +/// A specialization that does nothing. +#[derive(Clone)] +pub struct DummySpecialization; + +impl NetworkSpecialization for DummySpecialization { + fn status(&self) -> Vec { + vec![] + } + + fn on_connect( + &mut self, + _ctx: &mut dyn Context, + _peer_id: PeerId, + _status: crate::message::Status + ) {} + + fn on_disconnect(&mut self, _ctx: &mut dyn Context, _peer_id: PeerId) {} + + fn on_message( + &mut self, + _ctx: &mut dyn Context, + _peer_id: PeerId, + _message: Vec, + ) {} +} + /// Construct a simple protocol that is composed of several sub protocols. /// Each "sub protocol" needs to implement `Specialization` and needs to provide a `new()` function. /// For more fine grained implementations, this macro is not usable. diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index f1b7fa478c6c8..1b13e83343e58 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -62,6 +62,7 @@ use substrate_test_runtime_client::{self, AccountKeyring}; pub use substrate_test_runtime_client::runtime::{Block, Extrinsic, Hash, Transfer}; pub use substrate_test_runtime_client::{TestClient, TestClientBuilder, TestClientBuilderExt}; +pub use sc_network::specialization::DummySpecialization; type AuthorityId = sp_consensus_babe::AuthorityId; @@ -101,32 +102,6 @@ impl Verifier for PassThroughVerifier { } } -/// The test specialization. -#[derive(Clone)] -pub struct DummySpecialization; - -impl NetworkSpecialization for DummySpecialization { - fn status(&self) -> Vec { - vec![] - } - - fn on_connect( - &mut self, - _ctx: &mut dyn Context, - _peer_id: PeerId, - _status: sc_network::message::Status - ) {} - - fn on_disconnect(&mut self, _ctx: &mut dyn Context, _peer_id: PeerId) {} - - fn on_message( - &mut self, - _ctx: &mut dyn Context, - _peer_id: PeerId, - _message: Vec, - ) {} -} - pub type PeersFullClient = sc_client::Client; pub type PeersLightClient = From 561bd727489501cfb24d29020062b3a5d4fdcca9 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Mon, 20 Jan 2020 15:58:39 +0100 Subject: [PATCH 02/19] keep nominations after getting kicked with zero slash (#4681) * keep nominations after getting kicked with zero slash * rename next_key to maybe_next_key Co-Authored-By: Gavin Wood Co-authored-by: Gavin Wood --- frame/staking/src/lib.rs | 4 +- frame/staking/src/migration.rs | 58 +++++++++++++++++++-- frame/staking/src/slashing.rs | 27 +++++++--- frame/staking/src/tests.rs | 93 +++++++++++++++++++++++++++++++++- 4 files changed, 169 insertions(+), 13 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 326a01599034c..c547545c389a5 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -1482,11 +1482,11 @@ impl Module { let Nominations { submitted_in, mut targets, suppressed: _ } = nominations; // Filter out nomination targets which were nominated before the most recent - // slashing span. + // non-zero slash. targets.retain(|stash| { ::SlashingSpans::get(&stash).map_or( true, - |spans| submitted_in >= spans.last_start(), + |spans| submitted_in >= spans.last_nonzero_slash(), ) }); diff --git a/frame/staking/src/migration.rs b/frame/staking/src/migration.rs index bb020b0fc0e5f..6cb472375a48d 100644 --- a/frame/staking/src/migration.rs +++ b/frame/staking/src/migration.rs @@ -20,12 +20,14 @@ pub type VersionNumber = u32; // the current expected version of the storage -pub const CURRENT_VERSION: VersionNumber = 1; +pub const CURRENT_VERSION: VersionNumber = 2; +/// The inner logic of migrations. #[cfg(any(test, feature = "migrate"))] -mod inner { +pub mod inner { use crate::{Store, Module, Trait}; - use frame_support::{StorageLinkedMap, StorageValue}; + use frame_support::{StorageLinkedMap, StoragePrefixedMap, StorageValue}; + use codec::{Encode, Decode}; use sp_std::vec::Vec; use super::{CURRENT_VERSION, VersionNumber}; @@ -60,6 +62,55 @@ mod inner { frame_support::print("Finished migrating Staking storage to v1."); } + // migrate storage from v1 to v2: adds another field to the `SlashingSpans` + // struct. + pub fn to_v2(version: &mut VersionNumber) { + use crate::{EraIndex, slashing::SpanIndex}; + #[derive(Decode)] + struct V1SlashingSpans { + span_index: SpanIndex, + last_start: EraIndex, + prior: Vec, + } + + #[derive(Encode)] + struct V2SlashingSpans { + span_index: SpanIndex, + last_start: EraIndex, + last_nonzero_slash: EraIndex, + prior: Vec, + } + + if *version != 1 { return } + *version += 1; + + let prefix = as Store>::SlashingSpans::final_prefix(); + let mut current_key = prefix.to_vec(); + loop { + let maybe_next_key = sp_io::storage::next_key(¤t_key[..]) + .filter(|v| v.starts_with(&prefix[..])); + + match maybe_next_key { + Some(next_key) => { + let maybe_spans = sp_io::storage::get(&next_key[..]) + .and_then(|v| V1SlashingSpans::decode(&mut &v[..]).ok()); + if let Some(spans) = maybe_spans { + let new_val = V2SlashingSpans { + span_index: spans.span_index, + last_start: spans.last_start, + last_nonzero_slash: spans.last_start, + prior: spans.prior, + }.encode(); + + sp_io::storage::set(&next_key[..], &new_val[..]); + } + current_key = next_key; + } + None => break, + } + } + } + pub(super) fn perform_migrations() { as Store>::StorageVersion::mutate(|version| { if *version < MIN_SUPPORTED_VERSION { @@ -72,6 +123,7 @@ mod inner { if *version == CURRENT_VERSION { return } to_v1::(version); + to_v2::(version); }); } } diff --git a/frame/staking/src/slashing.rs b/frame/staking/src/slashing.rs index 7322b9a1d3118..df36b1c763c4b 100644 --- a/frame/staking/src/slashing.rs +++ b/frame/staking/src/slashing.rs @@ -90,7 +90,9 @@ pub struct SlashingSpans { span_index: SpanIndex, // the start era of the most recent (ongoing) slashing span. last_start: EraIndex, - // all prior slashing spans start indices, in reverse order (most recent first) + // the last era at which a non-zero slash occurred. + last_nonzero_slash: EraIndex, + // all prior slashing spans' start indices, in reverse order (most recent first) // encoded as offsets relative to the slashing span after it. prior: Vec, } @@ -102,6 +104,10 @@ impl SlashingSpans { SlashingSpans { span_index: 0, last_start: window_start, + // initialize to zero, as this structure is lazily created until + // the first slash is applied. setting equal to `window_start` would + // put a time limit on nominations. + last_nonzero_slash: 0, prior: Vec::new(), } } @@ -136,9 +142,9 @@ impl SlashingSpans { sp_std::iter::once(last).chain(prior) } - /// Yields the era index where the last (current) slashing span started. - pub(crate) fn last_start(&self) -> EraIndex { - self.last_start + /// Yields the era index where the most recent non-zero slash occurred. + pub(crate) fn last_nonzero_slash(&self) -> EraIndex { + self.last_nonzero_slash } // prune the slashing spans against a window, whose start era index is given. @@ -457,8 +463,12 @@ impl<'a, T: 'a + Trait> InspectingSpans<'a, T> { self.dirty = self.spans.end_span(now) || self.dirty; } - fn add_slash(&mut self, amount: BalanceOf) { + // add some value to the slash of the staker. + // invariant: the staker is being slashed for non-zero value here + // although `amount` may be zero, as it is only a difference. + fn add_slash(&mut self, amount: BalanceOf, slash_era: EraIndex) { *self.slash_of += amount; + self.spans.last_nonzero_slash = sp_std::cmp::max(self.spans.last_nonzero_slash, slash_era); } // find the span index of the given era, if covered. @@ -489,7 +499,7 @@ impl<'a, T: 'a + Trait> InspectingSpans<'a, T> { let reward = REWARD_F1 * (self.reward_proportion * slash).saturating_sub(span_record.paid_out); - self.add_slash(difference); + self.add_slash(difference, slash_era); changed = true; reward @@ -681,6 +691,7 @@ mod tests { let spans = SlashingSpans { span_index: 0, last_start: 1000, + last_nonzero_slash: 0, prior: Vec::new(), }; @@ -695,6 +706,7 @@ mod tests { let spans = SlashingSpans { span_index: 10, last_start: 1000, + last_nonzero_slash: 0, prior: vec![10, 9, 8, 10], }; @@ -715,6 +727,7 @@ mod tests { let mut spans = SlashingSpans { span_index: 10, last_start: 1000, + last_nonzero_slash: 0, prior: vec![10, 9, 8, 10], }; @@ -768,6 +781,7 @@ mod tests { let mut spans = SlashingSpans { span_index: 10, last_start: 1000, + last_nonzero_slash: 0, prior: vec![10, 9, 8, 10], }; assert_eq!(spans.prune(2000), Some((6, 10))); @@ -784,6 +798,7 @@ mod tests { let mut spans = SlashingSpans { span_index: 1, last_start: 10, + last_nonzero_slash: 0, prior: Vec::new(), }; diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index c290c6a8581d4..bb1fe32025ebc 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -18,12 +18,13 @@ use super::*; use mock::*; +use codec::Encode; use sp_runtime::{assert_eq_error_rate, traits::{OnInitialize, BadOrigin}}; use sp_staking::offence::OffenceDetails; use frame_support::{ assert_ok, assert_noop, traits::{Currency, ReservableCurrency}, - dispatch::DispatchError, + dispatch::DispatchError, StorageMap, }; use substrate_test_utils::assert_eq_uvec; @@ -2710,7 +2711,95 @@ fn slash_kicks_validators_not_nominators() { // and make sure that the vote will be ignored even if the validator // re-registers. - let last_slash = ::SlashingSpans::get(&11).unwrap().last_start(); + let last_slash = ::SlashingSpans::get(&11).unwrap().last_nonzero_slash(); assert!(nominations.submitted_in < last_slash); }); } + +#[test] +fn migration_v2() { + ExtBuilder::default().build().execute_with(|| { + use crate::{EraIndex, slashing::SpanIndex}; + + #[derive(Encode)] + struct V1SlashingSpans { + span_index: SpanIndex, + last_start: EraIndex, + prior: Vec, + } + + // inject old-style values directly into storage. + let set = |stash, spans: V1SlashingSpans| { + let key = ::SlashingSpans::hashed_key_for(stash); + sp_io::storage::set(&key, &spans.encode()); + }; + + let spans_11 = V1SlashingSpans { + span_index: 10, + last_start: 1, + prior: vec![0], + }; + + let spans_21 = V1SlashingSpans { + span_index: 1, + last_start: 5, + prior: vec![], + }; + + set(11, spans_11); + set(21, spans_21); + + ::StorageVersion::put(1); + + // perform migration. + crate::migration::inner::to_v2::(&mut 1); + + assert_eq!( + ::SlashingSpans::get(&11).unwrap().last_nonzero_slash(), + 1, + ); + + assert_eq!( + ::SlashingSpans::get(&21).unwrap().last_nonzero_slash(), + 5, + ); + }); +} + +#[test] +fn zero_slash_keeps_nominators() { + ExtBuilder::default().build().execute_with(|| { + start_era(1); + + assert_eq!(Balances::free_balance(&11), 1000); + + let exposure = Staking::stakers(&11); + assert_eq!(Balances::free_balance(&101), 2000); + + on_offence_now( + &[ + OffenceDetails { + offender: (11, exposure.clone()), + reporters: vec![], + }, + ], + &[Perbill::from_percent(0)], + ); + + assert_eq!(Balances::free_balance(&11), 1000); + assert_eq!(Balances::free_balance(&101), 2000); + + // This is the best way to check that the validator was chilled; `get` will + // return default value. + for (stash, _) in ::Validators::enumerate() { + assert!(stash != 11); + } + + let nominations = ::Nominators::get(&101).unwrap(); + + // and make sure that the vote will not be ignored, because the slash was + // zero. + let last_slash = ::SlashingSpans::get(&11).unwrap().last_nonzero_slash(); + assert!(nominations.submitted_in >= last_slash); + }); +} From a90c72e6d7088d0473670bf74253c65791411589 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Mon, 20 Jan 2020 16:00:43 +0100 Subject: [PATCH 03/19] deprecate chain_status field of network handshake (#4675) * deprecate chain_status field of network handshake * Update client/network/src/protocol/message.rs remove unneeded whitespace. Co-Authored-By: Pierre Krieger Co-authored-by: Pierre Krieger --- client/network/src/protocol.rs | 2 +- client/network/src/protocol/message.rs | 58 +++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 2aa29ea2793e3..6914ea9efe138 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -74,7 +74,7 @@ const MAX_KNOWN_BLOCKS: usize = 1024; // ~32kb per peer + LruHashSet overhead const MAX_KNOWN_EXTRINSICS: usize = 4096; // ~128kb per peer + overhead /// Current protocol version. -pub(crate) const CURRENT_VERSION: u32 = 5; +pub(crate) const CURRENT_VERSION: u32 = 6; /// Lowest version we support pub(crate) const MIN_VERSION: u32 = 3; diff --git a/client/network/src/protocol/message.rs b/client/network/src/protocol/message.rs index 30f0c34175aa5..ef7d550de6cbe 100644 --- a/client/network/src/protocol/message.rs +++ b/client/network/src/protocol/message.rs @@ -252,7 +252,29 @@ pub mod generic { } /// Status sent on connection. + // TODO https://github.com/paritytech/substrate/issues/4674: replace the `Status` + // struct with this one, after waiting a few releases beyond `NetworkSpecialization`'s + // removal (https://github.com/paritytech/substrate/pull/4665) + // + // and set MIN_VERSION to 6. #[derive(Debug, PartialEq, Eq, Clone, Encode, Decode)] + pub struct CompactStatus { + /// Protocol version. + pub version: u32, + /// Minimum supported version. + pub min_supported_version: u32, + /// Supported roles. + pub roles: Roles, + /// Best block number. + pub best_number: Number, + /// Best block hash. + pub best_hash: Hash, + /// Genesis block hash. + pub genesis_hash: Hash, + } + + /// Status sent on connection. + #[derive(Debug, PartialEq, Eq, Clone, Encode)] pub struct Status { /// Protocol version. pub version: u32, @@ -266,10 +288,44 @@ pub mod generic { pub best_hash: Hash, /// Genesis block hash. pub genesis_hash: Hash, - /// Chain-specific status. + /// DEPRECATED. Chain-specific status. pub chain_status: Vec, } + impl Decode for Status { + fn decode(value: &mut I) -> Result { + const LAST_CHAIN_STATUS_VERSION: u32 = 5; + let compact = CompactStatus::decode(value)?; + let chain_status = match >::decode(value) { + Ok(v) => v, + Err(e) => if compact.version <= LAST_CHAIN_STATUS_VERSION { + return Err(e) + } else { + Vec::new() + } + }; + + let CompactStatus { + version, + min_supported_version, + roles, + best_number, + best_hash, + genesis_hash, + } = compact; + + Ok(Status { + version, + min_supported_version, + roles, + best_number, + best_hash, + genesis_hash, + chain_status, + }) + } + } + /// Request block data from a peer. #[derive(Debug, PartialEq, Eq, Clone, Encode, Decode)] pub struct BlockRequest { From a083572ebfb4b85864c6016447b1481cc701b96b Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 20 Jan 2020 17:25:56 +0100 Subject: [PATCH 04/19] client/finality-grandpa/communication: Add doc comment for PeerReport (#4684) --- client/finality-grandpa/src/communication/gossip.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/client/finality-grandpa/src/communication/gossip.rs b/client/finality-grandpa/src/communication/gossip.rs index ec74393d80ff1..7b21c1d0797d3 100644 --- a/client/finality-grandpa/src/communication/gossip.rs +++ b/client/finality-grandpa/src/communication/gossip.rs @@ -1445,6 +1445,7 @@ impl sc_network_gossip::Validator for GossipValidator Date: Tue, 21 Jan 2020 00:26:19 +0800 Subject: [PATCH 05/19] ci: increase retention for logs of tests to 144 hours (#4677) * ci: increase retention for logs of tests to 144 hours * change to days --- .gitlab-ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 05834f11d1944..b98e859182bc3 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -146,7 +146,7 @@ test-linux-stable: &test-linux - awk '/^warning:/,/^$/ { print }' output.log > ${CI_COMMIT_SHORT_SHA}_warnings.log artifacts: name: $CI_COMMIT_SHORT_SHA - expire_in: 24 hrs + expire_in: 3 days paths: - ${CI_COMMIT_SHORT_SHA}_warnings.log @@ -210,7 +210,7 @@ test-linux-stable-int: artifacts: name: $CI_COMMIT_SHORT_SHA when: on_failure - expire_in: 24 hrs + expire_in: 3 days paths: - ${CI_COMMIT_SHORT_SHA}_int_failure.log From 1f9d09d597a250e11f8f742b2d6d2cc9ae911c6c Mon Sep 17 00:00:00 2001 From: thiolliere Date: Mon, 20 Jan 2020 17:26:53 +0100 Subject: [PATCH 06/19] Pallet session new API (#4609) * Initial work * Fix most things * fix test * fix old comment * migration * fix * remove useless stuff * fix * less spaghetti implementation * fix initial session * fix --- bin/node/runtime/src/lib.rs | 3 +- frame/authority-discovery/src/lib.rs | 10 +-- frame/babe/src/mock.rs | 3 +- frame/im-online/src/mock.rs | 22 ++--- frame/session/src/historical.rs | 129 ++++++++++++--------------- frame/session/src/lib.rs | 84 ++++++++--------- frame/session/src/mock.rs | 25 +++--- frame/staking/src/lib.rs | 63 +++++++------ frame/staking/src/mock.rs | 3 +- frame/staking/src/tests.rs | 12 +-- 10 files changed, 159 insertions(+), 195 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index d769be7b505d5..e158cbe2cbb88 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -236,14 +236,13 @@ parameter_types! { } impl pallet_session::Trait for Runtime { - type OnSessionEnding = Staking; + type SessionManager = Staking; type SessionHandler = ::KeyTypeIdProviders; type ShouldEndSession = Babe; type Event = Event; type Keys = SessionKeys; type ValidatorId = ::AccountId; type ValidatorIdOf = pallet_staking::StashOf; - type SelectInitialValidators = Staking; type DisabledValidatorsThreshold = DisabledValidatorsThreshold; } diff --git a/frame/authority-discovery/src/lib.rs b/frame/authority-discovery/src/lib.rs index b3911859f4787..c427043397075 100644 --- a/frame/authority-discovery/src/lib.rs +++ b/frame/authority-discovery/src/lib.rs @@ -109,26 +109,18 @@ mod tests { pub struct Test; impl Trait for Test {} - pub struct TestOnSessionEnding; - impl pallet_session::OnSessionEnding for TestOnSessionEnding { - fn on_session_ending(_: SessionIndex, _: SessionIndex) -> Option> { - None - } - } - parameter_types! { pub const DisabledValidatorsThreshold: Perbill = Perbill::from_percent(33); } impl pallet_session::Trait for Test { - type OnSessionEnding = TestOnSessionEnding; + type SessionManager = (); type Keys = UintAuthorityId; type ShouldEndSession = pallet_session::PeriodicSessions; type SessionHandler = TestSessionHandler; type Event = (); type ValidatorId = AuthorityId; type ValidatorIdOf = ConvertInto; - type SelectInitialValidators = (); type DisabledValidatorsThreshold = DisabledValidatorsThreshold; } diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index 3f0c42a6cb9c6..e65f305dc4de2 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -79,9 +79,8 @@ impl pallet_session::Trait for Test { type ValidatorId = ::AccountId; type ShouldEndSession = Babe; type SessionHandler = (Babe,Babe,); - type OnSessionEnding = (); + type SessionManager = (); type ValidatorIdOf = (); - type SelectInitialValidators = (); type Keys = MockSessionKeys; type DisabledValidatorsThreshold = DisabledValidatorsThreshold; } diff --git a/frame/im-online/src/mock.rs b/frame/im-online/src/mock.rs index 387ff47c09fa5..5c428c38582ff 100644 --- a/frame/im-online/src/mock.rs +++ b/frame/im-online/src/mock.rs @@ -43,28 +43,25 @@ thread_local! { pub static VALIDATORS: RefCell>> = RefCell::new(Some(vec![1, 2, 3])); } -pub struct TestOnSessionEnding; -impl pallet_session::OnSessionEnding for TestOnSessionEnding { - fn on_session_ending(_ending_index: SessionIndex, _will_apply_at: SessionIndex) - -> Option> - { +pub struct TestSessionManager; +impl pallet_session::SessionManager for TestSessionManager { + fn new_session(_new_index: SessionIndex) -> Option> { VALIDATORS.with(|l| l.borrow_mut().take()) } + fn end_session(_: SessionIndex) {} } -impl pallet_session::historical::OnSessionEnding for TestOnSessionEnding { - fn on_session_ending(_ending_index: SessionIndex, _will_apply_at: SessionIndex) - -> Option<(Vec, Vec<(u64, u64)>)> - { +impl pallet_session::historical::SessionManager for TestSessionManager { + fn new_session(_new_index: SessionIndex) -> Option> { VALIDATORS.with(|l| l .borrow_mut() .take() .map(|validators| { - let full_identification = validators.iter().map(|v| (*v, *v)).collect(); - (validators, full_identification) + validators.iter().map(|v| (*v, *v)).collect() }) ) } + fn end_session(_: SessionIndex) {} } /// An extrinsic type used for tests. @@ -131,13 +128,12 @@ parameter_types! { impl pallet_session::Trait for Runtime { type ShouldEndSession = pallet_session::PeriodicSessions; - type OnSessionEnding = pallet_session::historical::NoteHistoricalRoot; + type SessionManager = pallet_session::historical::NoteHistoricalRoot; type SessionHandler = (ImOnline, ); type ValidatorId = u64; type ValidatorIdOf = ConvertInto; type Keys = UintAuthorityId; type Event = (); - type SelectInitialValidators = (); type DisabledValidatorsThreshold = DisabledValidatorsThreshold; } diff --git a/frame/session/src/historical.rs b/frame/session/src/historical.rs index 939e6133e8520..e67a2ae1b0eab 100644 --- a/frame/session/src/historical.rs +++ b/frame/session/src/historical.rs @@ -48,8 +48,7 @@ pub trait Trait: super::Trait { /// validator, since they may be outdated by the time this is queried from a /// historical trie. /// - /// This mapping is expected to remain stable in between calls to - /// `Self::OnSessionEnding::on_session_ending` which return new validators. + /// It must return the identification for the current session index. type FullIdentificationOf: Convert>; } @@ -57,16 +56,19 @@ decl_storage! { trait Store for Module as Session { /// Mapping from historical session indices to session-data root hash and validator count. HistoricalSessions get(fn historical_root): map SessionIndex => Option<(T::Hash, ValidatorCount)>; - /// Queued full identifications for queued sessions whose validators have become obsolete. - CachedObsolete get(fn cached_obsolete): map SessionIndex - => Option>; /// The range of historical sessions we store. [first, last) StoredRange: Option<(SessionIndex, SessionIndex)>; + /// Deprecated. + CachedObsolete: map SessionIndex => Option>; } } decl_module! { - pub struct Module for enum Call where origin: T::Origin { } + pub struct Module for enum Call where origin: T::Origin { + fn on_initialize(_n: T::BlockNumber) { + CachedObsolete::::remove_all(); + } + } } impl Module { @@ -97,52 +99,52 @@ impl Module { } } -/// Specialization of the crate-level `OnSessionEnding` which returns the old -/// set of full identification when changing the validator set. -pub trait OnSessionEnding: crate::OnSessionEnding { - /// If there was a validator set change, its returns the set of new validators along with the - /// old validators and their full identifications. - fn on_session_ending(ending: SessionIndex, will_apply_at: SessionIndex) - -> Option<(Vec, Vec<(ValidatorId, FullIdentification)>)>; +/// Specialization of the crate-level `SessionManager` which returns the set of full identification +/// when creating a new session. +pub trait SessionManager: crate::SessionManager { + /// If there was a validator set change, its returns the set of new validators along with their + /// full identifications. + fn new_session(new_index: SessionIndex) -> Option>; + fn end_session(end_index: SessionIndex); } -/// An `OnSessionEnding` implementation that wraps an inner `I` and also +/// An `SessionManager` implementation that wraps an inner `I` and also /// sets the historical trie root of the ending session. pub struct NoteHistoricalRoot(sp_std::marker::PhantomData<(T, I)>); -impl crate::OnSessionEnding for NoteHistoricalRoot - where I: OnSessionEnding +impl crate::SessionManager for NoteHistoricalRoot + where I: SessionManager { - fn on_session_ending(ending: SessionIndex, applied_at: SessionIndex) -> Option> { + fn new_session(new_index: SessionIndex) -> Option> { StoredRange::mutate(|range| { - range.get_or_insert_with(|| (ending, ending)).1 = ending + 1; + range.get_or_insert_with(|| (new_index, new_index)).1 = new_index + 1; }); - // do all of this _before_ calling the other `on_session_ending` impl - // so that we have e.g. correct exposures from the _current_. + let new_validators_and_id = >::new_session(new_index); + let new_validators = new_validators_and_id.as_ref().map(|new_validators| { + new_validators.iter().map(|(v, _id)| v.clone()).collect() + }); - let count = >::validators().len() as u32; - match ProvingTrie::::generate_for(ending) { - Ok(trie) => >::insert(ending, &(trie.root, count)), - Err(reason) => { - print("Failed to generate historical ancestry-inclusion proof."); - print(reason); + if let Some(new_validators) = new_validators_and_id { + let count = new_validators.len() as u32; + match ProvingTrie::::generate_for(new_validators) { + Ok(trie) => >::insert(new_index, &(trie.root, count)), + Err(reason) => { + print("Failed to generate historical ancestry-inclusion proof."); + print(reason); + } + }; + } else { + let previous_index = new_index.saturating_sub(1); + if let Some(previous_session) = >::get(previous_index) { + >::insert(new_index, previous_session); } - }; - - // trie has been generated for this session, so it's no longer queued. - >::remove(&ending); - - let (new_validators, old_exposures) = >::on_session_ending(ending, applied_at)?; - - // every session from `ending+1 .. applied_at` now has obsolete `FullIdentification` - // now that a new validator election has occurred. - // we cache these in the trie until those sessions themselves end. - for obsolete in (ending + 1) .. applied_at { - >::insert(obsolete, &old_exposures); } - Some(new_validators) + new_validators + } + fn end_session(end_index: SessionIndex) { + >::end_session(end_index) } } @@ -158,15 +160,14 @@ pub struct ProvingTrie { } impl ProvingTrie { - fn generate_for(now: SessionIndex) -> Result { + fn generate_for(validators: I) -> Result + where I: IntoIterator + { let mut db = MemoryDB::default(); let mut root = Default::default(); - fn build(root: &mut T::Hash, db: &mut MemoryDB>, validators: I) - -> Result<(), &'static str> - where I: IntoIterator)> { - let mut trie = TrieDBMut::new(db, root); + let mut trie = TrieDBMut::new(&mut db, &mut root); for (i, (validator, full_id)) in validators.into_iter().enumerate() { let i = i as u32; let keys = match >::load_keys(&validator) { @@ -174,11 +175,7 @@ impl ProvingTrie { Some(k) => k, }; - let full_id = full_id.or_else(|| T::FullIdentificationOf::convert(validator.clone())); - let full_id = match full_id { - None => return Err("no full identification for a current validator"), - Some(full) => (validator, full), - }; + let full_id = (validator, full_id); // map each key to the owner index. for key_id in T::Keys::key_ids() { @@ -194,17 +191,6 @@ impl ProvingTrie { let _ = i.using_encoded(|k| full_id.using_encoded(|v| trie.insert(k, v))) .map_err(|_| "failed to insert into trie")?; } - - Ok(()) - } - - // if the current session's full identifications are obsolete but cached, - // use those. - if let Some(obsolete) = >::get(&now) { - build::(&mut root, &mut db, obsolete.into_iter().map(|(v, f)| (v, Some(f))))? - } else { - let validators = >::validators(); - build::(&mut root, &mut db, validators.into_iter().map(|v| (v, None)))? } Ok(ProvingTrie { @@ -281,7 +267,12 @@ impl> frame_support::traits::KeyOwnerProofSystem<(KeyTy fn prove(key: (KeyTypeId, D)) -> Option { let session = >::current_index(); - let trie = ProvingTrie::::generate_for(session).ok()?; + let validators = >::validators().into_iter() + .filter_map(|validator| { + T::FullIdentificationOf::convert(validator.clone()) + .map(|full_id| (validator, full_id)) + }); + let trie = ProvingTrie::::generate_for(validators).ok()?; let (id, data) = key; @@ -348,13 +339,9 @@ mod tests { set_next_validators(vec![1, 2, 4]); force_new_session(); - assert!(Historical::cached_obsolete(&(proof.session + 1)).is_none()); - System::set_block_number(2); Session::on_initialize(2); - assert!(Historical::cached_obsolete(&(proof.session + 1)).is_some()); - assert!(Historical::historical_root(proof.session).is_some()); assert!(Session::current_index() > proof.session); @@ -366,15 +353,13 @@ mod tests { force_new_session(); System::set_block_number(3); Session::on_initialize(3); - - assert!(Historical::cached_obsolete(&(proof.session + 1)).is_none()); }); } #[test] fn prune_up_to_works() { new_test_ext().execute_with(|| { - for i in 1..101u64 { + for i in 1..99u64 { set_next_validators(vec![i]); force_new_session(); @@ -385,7 +370,7 @@ mod tests { assert_eq!(StoredRange::get(), Some((0, 100))); - for i in 1..100 { + for i in 0..100 { assert!(Historical::historical_root(i).is_some()) } @@ -405,7 +390,7 @@ mod tests { Historical::prune_up_to(100); assert_eq!(StoredRange::get(), None); - for i in 101..201u64 { + for i in 99..199u64 { set_next_validators(vec![i]); force_new_session(); @@ -416,14 +401,14 @@ mod tests { assert_eq!(StoredRange::get(), Some((100, 200))); - for i in 101..200 { + for i in 100..200 { assert!(Historical::historical_root(i).is_some()) } Historical::prune_up_to(9999); assert_eq!(StoredRange::get(), None); - for i in 101..200 { + for i in 100..200 { assert!(Historical::historical_root(i).is_none()) } }); diff --git a/frame/session/src/lib.rs b/frame/session/src/lib.rs index 2200221af42fe..098b5330779ef 100644 --- a/frame/session/src/lib.rs +++ b/frame/session/src/lib.rs @@ -162,29 +162,30 @@ impl< } } -/// An event handler for when the session is ending. -/// TODO [slashing] consider renaming to OnSessionStarting -pub trait OnSessionEnding { - /// Handle the fact that the session is ending, and optionally provide the new validator set. +/// A trait for managing creation of new validator set. +pub trait SessionManager { + /// Plan a new session, and optionally provide the new validator set. /// /// Even if the validator-set is the same as before, if any underlying economic /// conditions have changed (i.e. stake-weights), the new validator set must be returned. /// This is necessary for consensus engines making use of the session module to /// issue a validator-set change so misbehavior can be provably associated with the new /// economic conditions as opposed to the old. + /// The returned validator set, if any, will not be applied until `new_index`. + /// `new_index` is strictly greater than from previous call. /// - /// `ending_index` is the index of the currently ending session. - /// The returned validator set, if any, will not be applied until `will_apply_at`. - /// `will_apply_at` is guaranteed to be at least `ending_index + 1`, since session indices don't - /// repeat, but it could be some time after in case we are staging authority set changes. - fn on_session_ending( - ending_index: SessionIndex, - will_apply_at: SessionIndex - ) -> Option>; + /// The first session start at index 0. + fn new_session(new_index: SessionIndex) -> Option>; + /// End the session. + /// + /// Because the session pallet can queue validator set the ending session can be lower than the + /// last new session index. + fn end_session(end_index: SessionIndex); } -impl OnSessionEnding for () { - fn on_session_ending(_: SessionIndex, _: SessionIndex) -> Option> { None } +impl SessionManager for () { + fn new_session(_: SessionIndex) -> Option> { None } + fn end_session(_: SessionIndex) {} } /// Handler for session lifecycle events. @@ -214,7 +215,7 @@ pub trait SessionHandler { /// A notification for end of the session. /// - /// Note it is triggered before any `OnSessionEnding` handlers, + /// Note it is triggered before any `SessionManager::end_session` handlers, /// so we can still affect the validator set. fn on_before_session_ending() {} @@ -248,7 +249,7 @@ pub trait OneSessionHandler: BoundToRuntimeAppPublic { /// A notification for end of the session. /// - /// Note it is triggered before any `OnSessionEnding` handlers, + /// Note it is triggered before any `SessionManager::end_session` handlers, /// so we can still affect the validator set. fn on_before_session_ending() {} @@ -318,21 +319,6 @@ impl SessionHandler for TestSessionHandler { fn on_disabled(_: usize) {} } -/// Handler for selecting the genesis validator set. -pub trait SelectInitialValidators { - /// Returns the initial validator set. If `None` is returned - /// all accounts that have session keys set in the genesis block - /// will be validators. - fn select_initial_validators() -> Option>; -} - -/// Implementation of `SelectInitialValidators` that does nothing. -impl SelectInitialValidators for () { - fn select_initial_validators() -> Option> { - None - } -} - impl ValidatorRegistration for Module { fn is_registered(id: &T::ValidatorId) -> bool { Self::load_keys(id).is_some() @@ -352,8 +338,8 @@ pub trait Trait: frame_system::Trait { /// Indicator for when to end the session. type ShouldEndSession: ShouldEndSession; - /// Handler for when a session is about to end. - type OnSessionEnding: OnSessionEnding; + /// Handler for managing new session. + type SessionManager: SessionManager; /// Handler when a session has changed. type SessionHandler: SessionHandler; @@ -366,9 +352,6 @@ pub trait Trait: frame_system::Trait { /// After the threshold is reached `disabled` method starts to return true, /// which in combination with `pallet_staking` forces a new era. type DisabledValidatorsThreshold: Get; - - /// Select initial validators. - type SelectInitialValidators: SelectInitialValidators; } const DEDUP_KEY_PREFIX: &[u8] = b":session:keys"; @@ -435,12 +418,19 @@ decl_storage! { .expect("genesis config must not contain duplicates; qed"); } - let initial_validators = T::SelectInitialValidators::select_initial_validators() - .unwrap_or_else(|| config.keys.iter().map(|(ref v, _)| v.clone()).collect()); + let initial_validators_0 = T::SessionManager::new_session(0) + .unwrap_or_else(|| { + frame_support::print("No initial validator provided by `SessionManager`, use \ + session config keys to generate initial validator set."); + config.keys.iter().map(|(ref v, _)| v.clone()).collect() + }); + assert!(!initial_validators_0.is_empty(), "Empty validator set for session 0 in genesis block!"); - assert!(!initial_validators.is_empty(), "Empty validator set in genesis block!"); + let initial_validators_1 = T::SessionManager::new_session(1) + .unwrap_or_else(|| initial_validators_0.clone()); + assert!(!initial_validators_1.is_empty(), "Empty validator set for session 1 in genesis block!"); - let queued_keys: Vec<_> = initial_validators + let queued_keys: Vec<_> = initial_validators_1 .iter() .cloned() .map(|v| ( @@ -452,7 +442,7 @@ decl_storage! { // Tell everyone about the genesis session keys T::SessionHandler::on_genesis_session::(&queued_keys); - >::put(initial_validators); + >::put(initial_validators_0); >::put(queued_keys); }); } @@ -545,10 +535,14 @@ impl Module { DisabledValidators::take(); } - let applied_at = session_index + 2; + T::SessionManager::end_session(session_index); + + // Increment session index. + let session_index = session_index + 1; + CurrentIndex::put(session_index); // Get next validator set. - let maybe_next_validators = T::OnSessionEnding::on_session_ending(session_index, applied_at); + let maybe_next_validators = T::SessionManager::new_session(session_index + 1); let (next_validators, next_identities_changed) = if let Some(validators) = maybe_next_validators { @@ -560,10 +554,6 @@ impl Module { (>::get(), false) }; - // Increment session index. - let session_index = session_index + 1; - CurrentIndex::put(session_index); - // Queue next session keys. let (queued_amalgamated, next_changed) = { // until we are certain there has been a change, iterate the prior diff --git a/frame/session/src/mock.rs b/frame/session/src/mock.rs index 7fe9cd01f43bf..ff84743a61596 100644 --- a/frame/session/src/mock.rs +++ b/frame/session/src/mock.rs @@ -88,9 +88,10 @@ impl SessionHandler for TestSessionHandler { } } -pub struct TestOnSessionEnding; -impl OnSessionEnding for TestOnSessionEnding { - fn on_session_ending(_: SessionIndex, _: SessionIndex) -> Option> { +pub struct TestSessionManager; +impl SessionManager for TestSessionManager { + fn end_session(_: SessionIndex) {} + fn new_session(_: SessionIndex) -> Option> { if !TEST_SESSION_CHANGED.with(|l| *l.borrow()) { VALIDATORS.with(|v| { let mut v = v.borrow_mut(); @@ -108,14 +109,13 @@ impl OnSessionEnding for TestOnSessionEnding { } #[cfg(feature = "historical")] -impl crate::historical::OnSessionEnding for TestOnSessionEnding { - fn on_session_ending(ending_index: SessionIndex, will_apply_at: SessionIndex) - -> Option<(Vec, Vec<(u64, u64)>)> +impl crate::historical::SessionManager for TestSessionManager { + fn end_session(_: SessionIndex) {} + fn new_session(new_index: SessionIndex) + -> Option> { - let pair_with_ids = |vals: &[u64]| vals.iter().map(|&v| (v, v)).collect::>(); - >::on_session_ending(ending_index, will_apply_at) - .map(|vals| (pair_with_ids(&vals), vals)) - .map(|(ids, vals)| (vals, ids)) + >::new_session(new_index) + .map(|vals| vals.into_iter().map(|val| (val, val)).collect()) } } @@ -190,15 +190,14 @@ parameter_types! { impl Trait for Test { type ShouldEndSession = TestShouldEndSession; #[cfg(feature = "historical")] - type OnSessionEnding = crate::historical::NoteHistoricalRoot; + type SessionManager = crate::historical::NoteHistoricalRoot; #[cfg(not(feature = "historical"))] - type OnSessionEnding = TestOnSessionEnding; + type SessionManager = TestSessionManager; type SessionHandler = TestSessionHandler; type ValidatorId = u64; type ValidatorIdOf = ConvertInto; type Keys = MockSessionKeys; type Event = (); - type SelectInitialValidators = (); type DisabledValidatorsThreshold = DisabledValidatorsThreshold; } diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index c547545c389a5..7f1ad37b32cf6 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -265,7 +265,7 @@ use frame_support::{ WithdrawReasons, OnUnbalanced, Imbalance, Get, Time } }; -use pallet_session::{historical::OnSessionEnding, SelectInitialValidators}; +use pallet_session::historical::SessionManager; use sp_runtime::{ Perbill, RuntimeDebug, @@ -575,8 +575,7 @@ impl SessionInterface<::AccountId> for T whe FullIdentificationOf = ExposureOf, >, T::SessionHandler: pallet_session::SessionHandler<::AccountId>, - T::OnSessionEnding: pallet_session::OnSessionEnding<::AccountId>, - T::SelectInitialValidators: pallet_session::SelectInitialValidators<::AccountId>, + T::SessionManager: pallet_session::SessionManager<::AccountId>, T::ValidatorIdOf: Convert<::AccountId, Option<::AccountId>> { fn disable_validator(validator: &::AccountId) -> Result { @@ -1346,11 +1345,8 @@ impl Module { imbalance } - /// Session has just ended. Provide the validator set for the next session if it's an era-end, along - /// with the exposure of the prior validator set. - fn new_session(session_index: SessionIndex) - -> Option<(Vec, Vec<(T::AccountId, Exposure>)>)> - { + /// Session has just ended. Provide the validator set for the next session if it's an era-end. + fn new_session(session_index: SessionIndex) -> Option> { let era_length = session_index.checked_sub(Self::current_era_start_session_index()).unwrap_or(0); match ForceEra::get() { Forcing::ForceNew => ForceEra::kill(), @@ -1358,12 +1354,17 @@ impl Module { Forcing::NotForcing if era_length >= T::SessionsPerEra::get() => (), _ => return None, } - let validators = T::SessionInterface::validators(); - let prior = validators.into_iter() - .map(|v| { let e = Self::stakers(&v); (v, e) }) - .collect(); - Self::new_era(session_index).map(move |new| (new, prior)) + Self::new_era(session_index) + } + + /// Initialise the first session (and consequently the first era) + fn initial_session() -> Option> { + // note: `CurrentEraStart` is set in `on_finalize` of the first block because now is not + // available yet. + CurrentEraStartSessionIndex::put(0); + BondedEras::mutate(|bonded| bonded.push((0, 0))); + Self::select_validators().1 } /// The era has changed - enact new staking set. @@ -1646,19 +1647,30 @@ impl Module { } } -impl pallet_session::OnSessionEnding for Module { - fn on_session_ending(_ending: SessionIndex, start_session: SessionIndex) -> Option> { +impl pallet_session::SessionManager for Module { + fn new_session(new_index: SessionIndex) -> Option> { Self::ensure_storage_upgraded(); - Self::new_session(start_session - 1).map(|(new, _old)| new) + if new_index == 0 { + return Self::initial_session(); + } + Self::new_session(new_index - 1) } + fn end_session(_end_index: SessionIndex) {} } -impl OnSessionEnding>> for Module { - fn on_session_ending(_ending: SessionIndex, start_session: SessionIndex) - -> Option<(Vec, Vec<(T::AccountId, Exposure>)>)> +impl SessionManager>> for Module { + fn new_session(new_index: SessionIndex) + -> Option>)>> { - Self::ensure_storage_upgraded(); - Self::new_session(start_session - 1) + >::new_session(new_index).map(|validators| { + validators.into_iter().map(|v| { + let exposure = >::get(&v); + (v, exposure) + }).collect() + }) + } + fn end_session(end_index: SessionIndex) { + >::end_session(end_index) } } @@ -1707,12 +1719,6 @@ impl Convert> } } -impl SelectInitialValidators for Module { - fn select_initial_validators() -> Option> { - >::select_validators().1 - } -} - /// This is intended to be used with `FilterHistoricalOffences`. impl OnOffenceHandler> for Module where T: pallet_session::Trait::AccountId>, @@ -1721,8 +1727,7 @@ impl OnOffenceHandler, >, T::SessionHandler: pallet_session::SessionHandler<::AccountId>, - T::OnSessionEnding: pallet_session::OnSessionEnding<::AccountId>, - T::SelectInitialValidators: pallet_session::SelectInitialValidators<::AccountId>, + T::SessionManager: pallet_session::SessionManager<::AccountId>, T::ValidatorIdOf: Convert<::AccountId, Option<::AccountId>> { fn on_offence( diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 3c238b56ed317..8aa20c19c6e29 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -162,14 +162,13 @@ parameter_types! { pub const DisabledValidatorsThreshold: Perbill = Perbill::from_percent(25); } impl pallet_session::Trait for Test { - type OnSessionEnding = pallet_session::historical::NoteHistoricalRoot; + type SessionManager = pallet_session::historical::NoteHistoricalRoot; type Keys = UintAuthorityId; type ShouldEndSession = pallet_session::PeriodicSessions; type SessionHandler = TestSessionHandler; type Event = (); type ValidatorId = AccountId; type ValidatorIdOf = crate::StashOf; - type SelectInitialValidators = Staking; type DisabledValidatorsThreshold = DisabledValidatorsThreshold; } diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index bb1fe32025ebc..43a2d0079164f 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -365,7 +365,7 @@ fn less_than_needed_candidates_works() { #[test] fn no_candidate_emergency_condition() { ExtBuilder::default() - .minimum_validator_count(10) + .minimum_validator_count(1) .validator_count(15) .num_validators(4) .validator_pool(true) @@ -374,21 +374,21 @@ fn no_candidate_emergency_condition() { .execute_with(|| { // initial validators assert_eq_uvec!(validator_controllers(), vec![10, 20, 30, 40]); + let prefs = ValidatorPrefs { commission: Perbill::one() }; + ::Validators::insert(11, prefs.clone()); // set the minimum validator count. ::MinimumValidatorCount::put(10); - ::ValidatorCount::put(15); - assert_eq!(Staking::validator_count(), 15); let _ = Staking::chill(Origin::signed(10)); // trigger era - System::set_block_number(1); - Session::on_initialize(System::block_number()); + start_era(1); // Previous ones are elected. chill is invalidates. TODO: #2494 assert_eq_uvec!(validator_controllers(), vec![10, 20, 30, 40]); - assert_eq!(Staking::current_elected().len(), 0); + // Though the validator preferences has been removed. + assert!(Staking::validators(11) != prefs); }); } From 5cd952bbf6405a165d0399fd3e5004961ace6819 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 20 Jan 2020 17:28:27 +0100 Subject: [PATCH 07/19] Only support ECDSA compressed public keys (#4667) Some fixes after: https://github.com/paritytech/substrate/pull/4502 This removes the unwanted `expect`s from `MultiSigner`. Instead we convert from full to compressed in `TryFrom` and can return an error on invalid input. --- primitives/core/src/ecdsa.rs | 120 +++++++++++-------------------- primitives/runtime/src/lib.rs | 6 +- primitives/runtime/src/traits.rs | 4 +- 3 files changed, 46 insertions(+), 84 deletions(-) diff --git a/primitives/core/src/ecdsa.rs b/primitives/core/src/ecdsa.rs index afd75dae99522..99db0e6b2d1a0 100644 --- a/primitives/core/src/ecdsa.rs +++ b/primitives/core/src/ecdsa.rs @@ -46,14 +46,9 @@ use secp256k1::{PublicKey, SecretKey}; #[cfg(feature = "full_crypto")] type Seed = [u8; 32]; -/// The ECDSA public key. +/// The ECDSA compressed public key. #[derive(Clone, Encode, Decode)] -pub enum Public { - /// A full raw ECDSA public key. - Full([u8; 64]), - /// A compressed ECDSA public key. - Compressed([u8; 33]), -} +pub struct Public([u8; 33]); impl PartialOrd for Public { fn partial_cmp(&self, other: &Self) -> Option { @@ -90,42 +85,12 @@ pub enum PublicError { } impl Public { - /// A new instance from the given 64-byte `data`. - /// - /// NOTE: No checking goes on to ensure this is a real public key. Only use it if - /// you are certain that the array actually is a pubkey. GIGO! - pub fn from_raw(data: [u8; 64]) -> Self { - Self::Full(data) - } - - /// A new instance from the given 65-byte `data`. + /// A new instance from the given 33-byte `data`. /// /// NOTE: No checking goes on to ensure this is a real public key. Only use it if /// you are certain that the array actually is a pubkey. GIGO! - pub fn from_full(data: [u8; 65]) -> Self { - let raw_key = &data[1..]; - let mut key = [0u8; 64]; - key.copy_from_slice(raw_key); - Self::Full(key) - } - - /// Return in compressed format. - /// - /// Returns an error if `self` is an invalid full public key. - pub fn as_compressed(&self) -> Result<[u8; 33], ()> { - match self { - Self::Full(d) => secp256k1::PublicKey::parse_slice(d, None) - .map(|k| k.serialize_compressed()) - .map_err(|_| ()), - Self::Compressed(d) => Ok(*d), - } - } - - /// Convert `Self` into a compressed public key. - /// - /// Returns an error if `self` is an invalid full public key. - pub fn into_compressed(self) -> Result { - self.as_compressed().map(Self::Compressed) + pub fn from_raw(data: [u8; 33]) -> Self { + Self(data) } } @@ -135,15 +100,9 @@ impl TraitPublic for Public { /// NOTE: No checking goes on to ensure this is a real public key. Only use it if /// you are certain that the array actually is a pubkey. GIGO! fn from_slice(data: &[u8]) -> Self { - if data.len() == 33 { - let mut r = [0u8; 33]; - r.copy_from_slice(data); - Self::Compressed(r) - } else { - let mut r = [0u8; 64]; - r.copy_from_slice(data); - Self::Full(r) - } + let mut r = [0u8; 33]; + r.copy_from_slice(data); + Self(r) } } @@ -151,25 +110,19 @@ impl Derive for Public {} impl Default for Public { fn default() -> Self { - Public::Full([0u8; 64]) + Public([0u8; 33]) } } impl AsRef<[u8]> for Public { fn as_ref(&self) -> &[u8] { - match self { - Self::Full(d) => &d[..], - Self::Compressed(d) => &d[..], - } + &self.0[..] } } impl AsMut<[u8]> for Public { fn as_mut(&mut self) -> &mut [u8] { - match self { - Self::Full(d) => &mut d[..], - Self::Compressed(d) => &mut d[..], - } + &mut self.0[..] } } @@ -177,10 +130,13 @@ impl sp_std::convert::TryFrom<&[u8]> for Public { type Error = (); fn try_from(data: &[u8]) -> Result { - if data.len() == 33 || data.len() == 64 { + if data.len() == 33 { Ok(Self::from_slice(data)) } else { - Err(()) + secp256k1::PublicKey::parse_slice(data, None) + .map(|k| k.serialize_compressed()) + .map(Self) + .map_err(|_| ()) } } } @@ -192,9 +148,9 @@ impl From for Public { } } -impl UncheckedFrom<[u8; 64]> for Public { - fn unchecked_from(x: [u8; 64]) -> Self { - Public::Full(x) +impl UncheckedFrom<[u8; 33]> for Public { + fn unchecked_from(x: [u8; 33]) -> Self { + Public(x) } } @@ -354,8 +310,9 @@ impl Signature { pub fn recover>(&self, message: M) -> Option { let message = secp256k1::Message::parse(&blake2_256(message.as_ref())); let sig: (_, _) = self.try_into().ok()?; - secp256k1::recover(&message, &sig.0, &sig.1).ok() - .map(|recovered| Public::from_full(recovered.serialize())) + secp256k1::recover(&message, &sig.0, &sig.1) + .ok() + .map(|recovered| Public(recovered.serialize_compressed())) } } @@ -476,7 +433,7 @@ impl TraitPair for Pair { /// Get the public key. fn public(&self) -> Public { - Public::from_full(self.public.serialize()) + Public(self.public.serialize_compressed()) } /// Sign a message. @@ -490,9 +447,7 @@ impl TraitPair for Pair { let message = secp256k1::Message::parse(&blake2_256(message.as_ref())); let sig: (_, _) = match sig.try_into() { Ok(x) => x, _ => return false }; match secp256k1::recover(&message, &sig.0, &sig.1) { - Ok(actual) => pubkey.as_compressed() - .map(|p| &p[..] == &actual.serialize_compressed()[..]) - .unwrap_or(false), + Ok(actual) => &pubkey.0[..] == &actual.serialize_compressed()[..], _ => false, } } @@ -587,9 +542,12 @@ mod test { &hex!("9d61b19deffd5a60ba844af492ec2cc44449c5697b326919703bac031cae7f60") ); let public = pair.public(); - assert_eq!(public, Public::from_raw( - hex!("8db55b05db86c0b1786ca49f095d76344c9e6056b2f02701a7e7f3c20aabfd913ebbe148dd17c56551a52952371071a6c604b3f3abe8f2c8fa742158ea6dd7d4") - )); + assert_eq!( + public, + Public::try_from( + &hex!("8db55b05db86c0b1786ca49f095d76344c9e6056b2f02701a7e7f3c20aabfd913ebbe148dd17c56551a52952371071a6c604b3f3abe8f2c8fa742158ea6dd7d4")[..], + ).unwrap(), + ); let message = b""; let signature = hex!("3dde91174bd9359027be59a428b8146513df80a2a3c7eda2194f64de04a69ab97b753169e94db6ffd50921a2668a48b94ca11e3d32c1ff19cfe88890aa7e8f3c00"); let signature = Signature::from_raw(signature); @@ -604,9 +562,12 @@ mod test { None ).unwrap(); let public = pair.public(); - assert_eq!(public, Public::from_raw( - hex!("8db55b05db86c0b1786ca49f095d76344c9e6056b2f02701a7e7f3c20aabfd913ebbe148dd17c56551a52952371071a6c604b3f3abe8f2c8fa742158ea6dd7d4") - )); + assert_eq!( + public, + Public::try_from( + &hex!("8db55b05db86c0b1786ca49f095d76344c9e6056b2f02701a7e7f3c20aabfd913ebbe148dd17c56551a52952371071a6c604b3f3abe8f2c8fa742158ea6dd7d4")[..], + ).unwrap(), + ); let message = b""; let signature = hex!("3dde91174bd9359027be59a428b8146513df80a2a3c7eda2194f64de04a69ab97b753169e94db6ffd50921a2668a48b94ca11e3d32c1ff19cfe88890aa7e8f3c00"); let signature = Signature::from_raw(signature); @@ -628,9 +589,12 @@ mod test { fn seeded_pair_should_work() { let pair = Pair::from_seed(b"12345678901234567890123456789012"); let public = pair.public(); - assert_eq!(public, Public::from_raw( - hex!("5676109c54b9a16d271abeb4954316a40a32bcce023ac14c8e26e958aa68fba995840f3de562156558efbfdac3f16af0065e5f66795f4dd8262a228ef8c6d813") - )); + assert_eq!( + public, + Public::try_from( + &hex!("5676109c54b9a16d271abeb4954316a40a32bcce023ac14c8e26e958aa68fba995840f3de562156558efbfdac3f16af0065e5f66795f4dd8262a228ef8c6d813")[..], + ).unwrap(), + ); let message = hex!("2f8c6129d816cf51c374bc7f08c3e63ed156cf78aefb4a6550d97b87997977ee00000000000000000200d75a980182b10ab7d54bfed3c964073a0ee172f3daa62325af021a68f707511a4500000000000000"); let signature = pair.sign(&message[..]); println!("Correct signature: {:?}", signature); diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index 6033f221a1f5f..46930c35e8e8d 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -238,9 +238,7 @@ impl traits::IdentifyAccount for MultiSigner { match self { MultiSigner::Ed25519(who) => <[u8; 32]>::from(who).into(), MultiSigner::Sr25519(who) => <[u8; 32]>::from(who).into(), - MultiSigner::Ecdsa(who) => sp_io::hashing::blake2_256( - &who.as_compressed().expect("`who` is a valid `ECDSA` public key; qed")[..], - ).into(), + MultiSigner::Ecdsa(who) => sp_io::hashing::blake2_256(&who.as_ref()[..]).into(), } } } @@ -724,7 +722,7 @@ mod tests { let multi_signer = MultiSigner::from(pair.public()); assert!(multi_sig.verify(msg, &multi_signer.into_account())); - let multi_signer = MultiSigner::from(pair.public().into_compressed().unwrap()); + let multi_signer = MultiSigner::from(pair.public()); assert!(multi_sig.verify(msg, &multi_signer.into_account())); } } diff --git a/primitives/runtime/src/traits.rs b/primitives/runtime/src/traits.rs index c02856d20d917..2547ce1072185 100644 --- a/primitives/runtime/src/traits.rs +++ b/primitives/runtime/src/traits.rs @@ -102,7 +102,7 @@ impl Verify for sp_core::ecdsa::Signature { self.as_ref(), &sp_io::hashing::blake2_256(msg.get()), ) { - Ok(pubkey) => signer.as_compressed().map(|s| &s[..] == &pubkey[..]).unwrap_or(false), + Ok(pubkey) => &signer.as_ref()[..] == &pubkey[..], _ => false, } } @@ -1357,6 +1357,6 @@ mod tests { assert!(ecdsa::Pair::verify(&signature, msg, &pair.public())); assert!(signature.verify(msg, &pair.public())); - assert!(signature.verify(msg, &pair.public().into_compressed().unwrap())); + assert!(signature.verify(msg, &pair.public())); } } From cfbb24cbdbc84410624a84daec55bfe178c7549f Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Mon, 20 Jan 2020 19:52:02 +0100 Subject: [PATCH 08/19] Call enable_all() when building tokio runtime (#4690) --- bin/node/cli/src/cli.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/bin/node/cli/src/cli.rs b/bin/node/cli/src/cli.rs index dcc5d39dbb7e7..8156cd766cc79 100644 --- a/bin/node/cli/src/cli.rs +++ b/bin/node/cli/src/cli.rs @@ -108,6 +108,7 @@ pub fn run(args: I, exit: E, version: sc_cli::VersionInfo) -> error::Re let runtime = RuntimeBuilder::new() .thread_name("main-tokio-") .threaded_scheduler() + .enable_all() .build() .map_err(|e| format!("{:?}", e))?; match config.roles { From cb9c1818f198b946b402652d173bbd76e8fbe7d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 21 Jan 2020 12:33:28 +0100 Subject: [PATCH 09/19] Make debug builds more usable (#4683) * Make debug builds more usable This pr makes debug builds more usable in terms of `cargo run -- --dev`. 1. `--dev` activates `--execution native`, iff `--execution` is not given or no sub `--execution-*` is given. 2. It was probably a mistake to compile WASM in debug for a debug build. So, we now build the WASM binary always as `release` (if not requested differently by the user). So, we trade compilation time for a better debug experience. * Make sure we only overwrite default values * Make it work * Apply suggestion --- Cargo.lock | 2 +- bin/node-template/runtime/build.rs | 2 +- bin/node/cli/src/cli.rs | 7 +++- bin/node/runtime/build.rs | 2 +- client/cli/src/execution_strategy.rs | 24 +++++++++++- client/cli/src/lib.rs | 39 ++++++++++++++----- client/cli/src/params.rs | 11 +++--- client/executor/runtime-test/build.rs | 2 +- .../runtime-interface/test-wasm/build.rs | 2 +- test-utils/runtime/build.rs | 2 +- utils/wasm-builder/Cargo.toml | 2 +- utils/wasm-builder/src/wasm_project.rs | 2 +- 12 files changed, 73 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 630e372dff82e..8030a4cd13ad2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6973,7 +6973,7 @@ version = "2.0.0" [[package]] name = "substrate-wasm-builder" -version = "1.0.8" +version = "1.0.9" dependencies = [ "atty 0.2.13 (registry+https://github.com/rust-lang/crates.io-index)", "build-helper 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/bin/node-template/runtime/build.rs b/bin/node-template/runtime/build.rs index ed0e28ec0d140..dc7e949d809f8 100644 --- a/bin/node-template/runtime/build.rs +++ b/bin/node-template/runtime/build.rs @@ -19,7 +19,7 @@ use wasm_builder_runner::{build_current_project_with_rustflags, WasmBuilderSourc fn main() { build_current_project_with_rustflags( "wasm_binary.rs", - WasmBuilderSource::Crates("1.0.8"), + WasmBuilderSource::Crates("1.0.9"), // This instructs LLD to export __heap_base as a global variable, which is used by the // external memory allocator. "-Clink-arg=--export=__heap_base", diff --git a/bin/node/cli/src/cli.rs b/bin/node/cli/src/cli.rs index 8156cd766cc79..6a2e02efb6209 100644 --- a/bin/node/cli/src/cli.rs +++ b/bin/node/cli/src/cli.rs @@ -141,7 +141,12 @@ pub fn run(args: I, exit: E, version: sc_cli::VersionInfo) -> error::Re &version, None, )?; - sc_cli::fill_import_params(&mut config, &cli_args.import_params, ServiceRoles::FULL)?; + sc_cli::fill_import_params( + &mut config, + &cli_args.import_params, + ServiceRoles::FULL, + cli_args.shared_params.dev, + )?; match ChainSpec::from(config.chain_spec.id()) { Some(ref c) if c == &ChainSpec::Development || c == &ChainSpec::LocalTestnet => {}, diff --git a/bin/node/runtime/build.rs b/bin/node/runtime/build.rs index 1ed2fa43e685c..9c81ea6f38b8d 100644 --- a/bin/node/runtime/build.rs +++ b/bin/node/runtime/build.rs @@ -21,7 +21,7 @@ fn main() { "wasm_binary.rs", WasmBuilderSource::CratesOrPath { path: "../../../utils/wasm-builder", - version: "1.0.8", + version: "1.0.9", }, // This instructs LLD to export __heap_base as a global variable, which is used by the // external memory allocator. diff --git a/client/cli/src/execution_strategy.rs b/client/cli/src/execution_strategy.rs index fe353da80ddef..888d7b6c4a096 100644 --- a/client/cli/src/execution_strategy.rs +++ b/client/cli/src/execution_strategy.rs @@ -20,7 +20,7 @@ use structopt::clap::arg_enum; arg_enum! { /// How to execute blocks - #[derive(Debug, Clone, Copy)] + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ExecutionStrategy { // Execute with native build (if available, WebAssembly otherwise). Native, @@ -33,3 +33,25 @@ arg_enum! { } } +impl ExecutionStrategy { + /// Returns the variant as `'&static str`. + pub fn as_str(&self) -> &'static str { + match self { + Self::Native => "Native", + Self::Wasm => "Wasm", + Self::Both => "Both", + Self::NativeElseWasm => "NativeElseWasm", + } + } +} + +/// Default value for the `--execution-syncing` parameter. +pub const DEFAULT_EXECUTION_SYNCING: ExecutionStrategy = ExecutionStrategy::NativeElseWasm; +/// Default value for the `--execution-import-block` parameter. +pub const DEFAULT_EXECUTION_IMPORT_BLOCK: ExecutionStrategy = ExecutionStrategy::NativeElseWasm; +/// Default value for the `--execution-block-construction` parameter. +pub const DEFAULT_EXECUTION_BLOCK_CONSTRUCTION: ExecutionStrategy = ExecutionStrategy::Wasm; +/// Default value for the `--execution-offchain-worker` parameter. +pub const DEFAULT_EXECUTION_OFFCHAIN_WORKER: ExecutionStrategy = ExecutionStrategy::Native; +/// Default value for the `--execution-other` parameter. +pub const DEFAULT_EXECUTION_OTHER: ExecutionStrategy = ExecutionStrategy::Native; diff --git a/client/cli/src/lib.rs b/client/cli/src/lib.rs index 52a3fc46ec726..fc5025952860b 100644 --- a/client/cli/src/lib.rs +++ b/client/cli/src/lib.rs @@ -40,6 +40,7 @@ use sc_network::{ }, }; use sp_core::H256; +use execution_strategy::*; use std::{ io::{Write, Read, Seek, Cursor, stdin, stdout, ErrorKind}, iter, fmt::Debug, fs::{self, File}, @@ -556,7 +557,12 @@ impl<'a> ParseAndPrepareImport<'a> { self.version, None, )?; - fill_import_params(&mut config, &self.params.import_params, sc_service::Roles::FULL)?; + fill_import_params( + &mut config, + &self.params.import_params, + sc_service::Roles::FULL, + self.params.shared_params.dev, + )?; let file: Box = match self.params.input { Some(filename) => Box::new(File::open(filename)?), @@ -620,7 +626,12 @@ impl<'a> CheckBlock<'a> { self.version, None, )?; - fill_import_params(&mut config, &self.params.import_params, sc_service::Roles::FULL)?; + fill_import_params( + &mut config, + &self.params.import_params, + sc_service::Roles::FULL, + self.params.shared_params.dev, + )?; fill_config_keystore_in_memory(&mut config)?; let input = if self.params.input.starts_with("0x") { &self.params.input[2..] } else { &self.params.input[..] }; @@ -904,6 +915,7 @@ pub fn fill_import_params( config: &mut Configuration, cli: &ImportParams, role: sc_service::Roles, + is_dev: bool, ) -> error::Result<()> where C: Default, @@ -943,13 +955,22 @@ pub fn fill_import_params( config.wasm_method = cli.wasm_method.into(); let exec = &cli.execution_strategies; - let exec_all_or = |strat: ExecutionStrategy| exec.execution.unwrap_or(strat).into(); + let exec_all_or = |strat: ExecutionStrategy, default: ExecutionStrategy| { + exec.execution.unwrap_or(if strat == default && is_dev { + ExecutionStrategy::Native + } else { + strat + }).into() + }; + config.execution_strategies = ExecutionStrategies { - syncing: exec_all_or(exec.execution_syncing), - importing: exec_all_or(exec.execution_import_block), - block_construction: exec_all_or(exec.execution_block_construction), - offchain_worker: exec_all_or(exec.execution_offchain_worker), - other: exec_all_or(exec.execution_other), + syncing: exec_all_or(exec.execution_syncing, DEFAULT_EXECUTION_SYNCING), + importing: exec_all_or(exec.execution_import_block, DEFAULT_EXECUTION_IMPORT_BLOCK), + block_construction: + exec_all_or(exec.execution_block_construction, DEFAULT_EXECUTION_BLOCK_CONSTRUCTION), + offchain_worker: + exec_all_or(exec.execution_offchain_worker, DEFAULT_EXECUTION_OFFCHAIN_WORKER), + other: exec_all_or(exec.execution_other, DEFAULT_EXECUTION_OTHER), }; Ok(()) } @@ -987,7 +1008,7 @@ where sc_service::Roles::FULL }; - fill_import_params(&mut config, &cli.import_params, role)?; + fill_import_params(&mut config, &cli.import_params, role, is_dev)?; config.impl_name = impl_name; config.impl_commit = version.commit; diff --git a/client/cli/src/params.rs b/client/cli/src/params.rs index 07fbe9b73885c..57b90c2e73d5c 100644 --- a/client/cli/src/params.rs +++ b/client/cli/src/params.rs @@ -18,6 +18,7 @@ use crate::traits::GetSharedParams; use std::{str::FromStr, path::PathBuf}; use structopt::{StructOpt, StructOptInternal, clap::{arg_enum, App, AppSettings, SubCommand, Arg}}; +use crate::execution_strategy::*; pub use crate::execution_strategy::ExecutionStrategy; @@ -321,7 +322,7 @@ pub struct ExecutionStrategies { value_name = "STRATEGY", possible_values = &ExecutionStrategy::variants(), case_insensitive = true, - default_value = "NativeElseWasm" + default_value = DEFAULT_EXECUTION_SYNCING.as_str(), )] pub execution_syncing: ExecutionStrategy, @@ -331,7 +332,7 @@ pub struct ExecutionStrategies { value_name = "STRATEGY", possible_values = &ExecutionStrategy::variants(), case_insensitive = true, - default_value = "NativeElseWasm" + default_value = DEFAULT_EXECUTION_IMPORT_BLOCK.as_str(), )] pub execution_import_block: ExecutionStrategy, @@ -341,7 +342,7 @@ pub struct ExecutionStrategies { value_name = "STRATEGY", possible_values = &ExecutionStrategy::variants(), case_insensitive = true, - default_value = "Wasm" + default_value = DEFAULT_EXECUTION_BLOCK_CONSTRUCTION.as_str(), )] pub execution_block_construction: ExecutionStrategy, @@ -351,7 +352,7 @@ pub struct ExecutionStrategies { value_name = "STRATEGY", possible_values = &ExecutionStrategy::variants(), case_insensitive = true, - default_value = "Native" + default_value = DEFAULT_EXECUTION_OFFCHAIN_WORKER.as_str(), )] pub execution_offchain_worker: ExecutionStrategy, @@ -361,7 +362,7 @@ pub struct ExecutionStrategies { value_name = "STRATEGY", possible_values = &ExecutionStrategy::variants(), case_insensitive = true, - default_value = "Native" + default_value = DEFAULT_EXECUTION_OTHER.as_str(), )] pub execution_other: ExecutionStrategy, diff --git a/client/executor/runtime-test/build.rs b/client/executor/runtime-test/build.rs index 1ed2fa43e685c..9c81ea6f38b8d 100644 --- a/client/executor/runtime-test/build.rs +++ b/client/executor/runtime-test/build.rs @@ -21,7 +21,7 @@ fn main() { "wasm_binary.rs", WasmBuilderSource::CratesOrPath { path: "../../../utils/wasm-builder", - version: "1.0.8", + version: "1.0.9", }, // This instructs LLD to export __heap_base as a global variable, which is used by the // external memory allocator. diff --git a/primitives/runtime-interface/test-wasm/build.rs b/primitives/runtime-interface/test-wasm/build.rs index 8e1d17e821195..9c81ea6f38b8d 100644 --- a/primitives/runtime-interface/test-wasm/build.rs +++ b/primitives/runtime-interface/test-wasm/build.rs @@ -21,7 +21,7 @@ fn main() { "wasm_binary.rs", WasmBuilderSource::CratesOrPath { path: "../../../utils/wasm-builder", - version: "1.0.6", + version: "1.0.9", }, // This instructs LLD to export __heap_base as a global variable, which is used by the // external memory allocator. diff --git a/test-utils/runtime/build.rs b/test-utils/runtime/build.rs index a90270d85d55f..8a9ee642c4fee 100644 --- a/test-utils/runtime/build.rs +++ b/test-utils/runtime/build.rs @@ -21,7 +21,7 @@ fn main() { "wasm_binary.rs", WasmBuilderSource::CratesOrPath { path: "../../utils/wasm-builder", - version: "1.0.8", + version: "1.0.9", }, // Note that we set the stack-size to 1MB explicitly even though it is set // to this value by default. This is because some of our tests (`restoration_of_globals`) diff --git a/utils/wasm-builder/Cargo.toml b/utils/wasm-builder/Cargo.toml index 66d94c32ab330..5941a39ffb6a6 100644 --- a/utils/wasm-builder/Cargo.toml +++ b/utils/wasm-builder/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "substrate-wasm-builder" -version = "1.0.8" +version = "1.0.9" authors = ["Parity Technologies "] description = "Utility for building WASM binaries" edition = "2018" diff --git a/utils/wasm-builder/src/wasm_project.rs b/utils/wasm-builder/src/wasm_project.rs index a028d00930a4e..c9b573ff1940f 100644 --- a/utils/wasm-builder/src/wasm_project.rs +++ b/utils/wasm-builder/src/wasm_project.rs @@ -357,7 +357,7 @@ fn is_release_build() -> bool { ), } } else { - !build_helper::debug() + true } } From f52ea97fb6074598e64989711f9b882418d087c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 21 Jan 2020 11:45:36 +0000 Subject: [PATCH 10/19] grandpa: reduce allocations when verifying multiple messages (#4693) --- .../finality-grandpa/src/communication/mod.rs | 66 +++++++++++++++++-- client/finality-grandpa/src/justification.rs | 4 +- 2 files changed, 62 insertions(+), 8 deletions(-) diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index 7723047d1b423..5077d435e8550 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -600,8 +600,28 @@ impl> Clone for NetworkBridge { } } -pub(crate) fn localized_payload(round: RoundNumber, set_id: SetIdNumber, message: &E) -> Vec { - (message, round, set_id).encode() +/// Encode round message localized to a given round and set id. +pub(crate) fn localized_payload( + round: RoundNumber, + set_id: SetIdNumber, + message: &E, +) -> Vec { + let mut buf = Vec::new(); + localized_payload_with_buffer(round, set_id, message, &mut buf); + buf +} + +/// Encode round message localized to a given round and set id using the given +/// buffer. The given buffer will be cleared and the resulting encoded payload +/// will always be written to the start of the buffer. +pub(crate) fn localized_payload_with_buffer( + round: RoundNumber, + set_id: SetIdNumber, + message: &E, + buf: &mut Vec, +) { + buf.clear(); + (message, round, set_id).encode_to(buf) } /// Type-safe wrapper around a round number. @@ -612,17 +632,41 @@ pub struct Round(pub RoundNumber); #[derive(Debug, Clone, Copy, Eq, PartialEq, PartialOrd, Ord, Encode, Decode)] pub struct SetId(pub SetIdNumber); -// check a message. +/// Check a message signature by encoding the message as a localized payload and +/// verifying the provided signature using the expected authority id. pub(crate) fn check_message_sig( message: &Message, id: &AuthorityId, signature: &AuthoritySignature, round: RoundNumber, set_id: SetIdNumber, +) -> Result<(), ()> { + check_message_sig_with_buffer::( + message, + id, + signature, + round, + set_id, + &mut Vec::new(), + ) +} + +/// Check a message signature by encoding the message as a localized payload and +/// verifying the provided signature using the expected authority id. +/// The encoding necessary to verify the signature will be done using the given +/// buffer, the original content of the buffer will be cleared. +pub(crate) fn check_message_sig_with_buffer( + message: &Message, + id: &AuthorityId, + signature: &AuthoritySignature, + round: RoundNumber, + set_id: SetIdNumber, + buf: &mut Vec, ) -> Result<(), ()> { let as_public = id.clone(); - let encoded_raw = localized_payload(round, set_id, message); - if AuthorityPair::verify(signature, &encoded_raw, &as_public) { + localized_payload_with_buffer(round, set_id, message, buf); + + if AuthorityPair::verify(signature, buf, &as_public) { Ok(()) } else { debug!(target: "afg", "Bad signature on message from {:?}", id); @@ -752,6 +796,7 @@ fn check_compact_commit( } // check signatures on all contained precommits. + let mut buf = Vec::new(); for (i, (precommit, &(ref sig, ref id))) in msg.precommits.iter() .zip(&msg.auth_data) .enumerate() @@ -759,12 +804,13 @@ fn check_compact_commit( use crate::communication::gossip::Misbehavior; use finality_grandpa::Message as GrandpaMessage; - if let Err(()) = check_message_sig::( + if let Err(()) = check_message_sig_with_buffer::( &GrandpaMessage::Precommit(precommit.clone()), id, sig, round.0, set_id.0, + &mut buf, ) { debug!(target: "afg", "Bad commit message signature {}", id); telemetry!(CONSENSUS_DEBUG; "afg.bad_commit_msg_signature"; "id" => ?id); @@ -836,6 +882,7 @@ fn check_catch_up( round: RoundNumber, set_id: SetIdNumber, mut signatures_checked: usize, + buf: &mut Vec, ) -> Result where B: BlockT, I: Iterator, &'a AuthorityId, &'a AuthoritySignature)>, @@ -845,12 +892,13 @@ fn check_catch_up( for (msg, id, sig) in messages { signatures_checked += 1; - if let Err(()) = check_message_sig::( + if let Err(()) = check_message_sig_with_buffer::( &msg, id, sig, round, set_id, + buf, ) { debug!(target: "afg", "Bad catch up message signature {}", id); telemetry!(CONSENSUS_DEBUG; "afg.bad_catch_up_msg_signature"; "id" => ?id); @@ -866,6 +914,8 @@ fn check_catch_up( Ok(signatures_checked) } + let mut buf = Vec::new(); + // check signatures on all contained prevotes. let signatures_checked = check_signatures::( msg.prevotes.iter().map(|vote| { @@ -874,6 +924,7 @@ fn check_catch_up( msg.round_number, set_id.0, 0, + &mut buf, )?; // check signatures on all contained precommits. @@ -884,6 +935,7 @@ fn check_catch_up( msg.round_number, set_id.0, signatures_checked, + &mut buf, )?; Ok(()) diff --git a/client/finality-grandpa/src/justification.rs b/client/finality-grandpa/src/justification.rs index 308056725f67b..ad96956454fd7 100644 --- a/client/finality-grandpa/src/justification.rs +++ b/client/finality-grandpa/src/justification.rs @@ -132,14 +132,16 @@ impl GrandpaJustification { } } + let mut buf = Vec::new(); let mut visited_hashes = HashSet::new(); for signed in self.commit.precommits.iter() { - if let Err(_) = communication::check_message_sig::( + if let Err(_) = communication::check_message_sig_with_buffer::( &finality_grandpa::Message::Precommit(signed.precommit.clone()), &signed.id, &signed.signature, self.round, set_id, + &mut buf, ) { return Err(ClientError::BadJustification( "invalid signature for precommit in grandpa justification".to_string()).into()); From 6ee1244e2d018333746d82131222308e0d802e6a Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 21 Jan 2020 13:06:15 +0100 Subject: [PATCH 11/19] Pass an executor through the Configuration (#4688) * Pass an executor through the Configuration * Make tasks_executor mandatory * Fix tests --- bin/node-template/src/cli.rs | 6 +++++- bin/node/cli/src/cli.rs | 6 +++++- client/service/src/builder.rs | 6 +++++- client/service/src/config.rs | 6 ++++-- client/service/src/error.rs | 3 +++ client/service/src/lib.rs | 25 ++++--------------------- client/service/test/src/lib.rs | 21 +++++++++++++++++++-- utils/browser/src/lib.rs | 3 +++ 8 files changed, 48 insertions(+), 28 deletions(-) diff --git a/bin/node-template/src/cli.rs b/bin/node-template/src/cli.rs index 44764e5c9db41..fcfd330816cd1 100644 --- a/bin/node-template/src/cli.rs +++ b/bin/node-template/src/cli.rs @@ -18,7 +18,7 @@ pub fn run(args: I, exit: E, version: VersionInfo) -> error::Result<()> type Config = Configuration<(), T>; match parse_and_prepare::(&version, "substrate-node", args) { ParseAndPrepare::Run(cmd) => cmd.run(load_spec, exit, - |exit, _cli_args, _custom_args, config: Config<_>| { + |exit, _cli_args, _custom_args, mut config: Config<_>| { info!("{}", version.name); info!(" version {}", config.full_version()); info!(" by {}, 2017, 2018", version.author); @@ -26,6 +26,10 @@ pub fn run(args: I, exit: E, version: VersionInfo) -> error::Result<()> info!("Node name: {}", config.name); info!("Roles: {}", display_role(&config)); let runtime = Runtime::new().map_err(|e| format!("{:?}", e))?; + config.tasks_executor = { + let runtime_handle = runtime.handle().clone(); + Some(Box::new(move |fut| { runtime_handle.spawn(fut); })) + }; match config.roles { ServiceRoles::LIGHT => run_until_exit( runtime, diff --git a/bin/node/cli/src/cli.rs b/bin/node/cli/src/cli.rs index 6a2e02efb6209..5ade700513e53 100644 --- a/bin/node/cli/src/cli.rs +++ b/bin/node/cli/src/cli.rs @@ -98,7 +98,7 @@ pub fn run(args: I, exit: E, version: sc_cli::VersionInfo) -> error::Re match parse_and_prepare::(&version, "substrate-node", args) { ParseAndPrepare::Run(cmd) => cmd.run(load_spec, exit, - |exit, _cli_args, _custom_args, config: Config<_, _>| { + |exit, _cli_args, _custom_args, mut config: Config<_, _>| { info!("{}", version.name); info!(" version {}", config.full_version()); info!(" by Parity Technologies, 2017-2019"); @@ -111,6 +111,10 @@ pub fn run(args: I, exit: E, version: sc_cli::VersionInfo) -> error::Re .enable_all() .build() .map_err(|e| format!("{:?}", e))?; + config.tasks_executor = { + let runtime_handle = runtime.handle().clone(); + Some(Box::new(move |fut| { runtime_handle.spawn(fut); })) + }; match config.roles { ServiceRoles::LIGHT => run_until_exit( runtime, diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index 5449b00b061a3..044798701c6e1 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -1147,7 +1147,11 @@ ServiceBuilder< essential_failed_rx, to_spawn_tx, to_spawn_rx, - to_poll: Vec::new(), + tasks_executor: if let Some(exec) = config.tasks_executor { + exec + } else { + return Err(Error::TasksExecutorRequired); + }, rpc_handlers, _rpc: rpc, _telemetry: telemetry, diff --git a/client/service/src/config.rs b/client/service/src/config.rs index 75c626821479d..8a145dec165b4 100644 --- a/client/service/src/config.rs +++ b/client/service/src/config.rs @@ -21,7 +21,7 @@ pub use sc_client_db::{kvdb::KeyValueDB, PruningMode}; pub use sc_network::config::{ExtTransport, NetworkConfiguration, Roles}; pub use sc_executor::WasmExecutionMethod; -use std::{path::{PathBuf, Path}, net::SocketAddr, sync::Arc}; +use std::{future::Future, path::{PathBuf, Path}, pin::Pin, net::SocketAddr, sync::Arc}; pub use sc_transaction_pool::txpool::Options as TransactionPoolOptions; use sc_chain_spec::{ChainSpec, RuntimeGenesis, Extension, NoExtension}; use sp_core::crypto::Protected; @@ -29,7 +29,6 @@ use target_info::Target; use sc_telemetry::TelemetryEndpoints; /// Service configuration. -#[derive(Clone)] pub struct Configuration { /// Implementation name pub impl_name: &'static str, @@ -39,6 +38,8 @@ pub struct Configuration { pub impl_commit: &'static str, /// Node roles. pub roles: Roles, + /// How to spawn background tasks. Mandatory, otherwise creating a `Service` will error. + pub tasks_executor: Option + Send>>) + Send>>, /// Extrinsic pool configuration. pub transaction_pool: TransactionPoolOptions, /// Network configuration. @@ -160,6 +161,7 @@ impl Configuration where config_dir: config_dir.clone(), name: Default::default(), roles: Roles::FULL, + tasks_executor: None, transaction_pool: Default::default(), network: Default::default(), keystore: KeystoreConfig::None, diff --git a/client/service/src/error.rs b/client/service/src/error.rs index 6516b1c62c6d6..14b03d7e95de7 100644 --- a/client/service/src/error.rs +++ b/client/service/src/error.rs @@ -40,6 +40,9 @@ pub enum Error { /// Best chain selection strategy is missing. #[display(fmt="Best chain selection strategy (SelectChain) is not provided.")] SelectChainRequired, + /// Tasks executor is missing. + #[display(fmt="Tasks executor hasn't been provided.")] + TasksExecutorRequired, /// Other error. Other(String), } diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index 87327d0967583..c1b87e4491904 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -38,7 +38,7 @@ use parking_lot::Mutex; use sc_client::Client; use exit_future::Signal; use futures::{ - Future, FutureExt, Stream, StreamExt, TryFutureExt, + Future, FutureExt, Stream, StreamExt, future::select, channel::mpsc, compat::*, sink::SinkExt, @@ -95,10 +95,8 @@ pub struct Service { to_spawn_tx: mpsc::UnboundedSender + Send>>>, /// Receiver for futures that must be spawned as background tasks. to_spawn_rx: mpsc::UnboundedReceiver + Send>>>, - /// List of futures to poll from `poll`. - /// If spawning a background task is not possible, we instead push the task into this `Vec`. - /// The elements must then be polled manually. - to_poll: Vec + Send>>>, + /// How to spawn background tasks. + tasks_executor: Box + Send>>) + Send>, rpc_handlers: sc_rpc_server::RpcHandler, _rpc: Box, _telemetry: Option, @@ -322,22 +320,7 @@ impl Future for } while let Poll::Ready(Some(task_to_spawn)) = Pin::new(&mut this.to_spawn_rx).poll_next(cx) { - // TODO: Update to tokio 0.2 when libp2p get switched to std futures (#4383) - let executor = tokio_executor::DefaultExecutor::current(); - use futures01::future::Executor; - if let Err(err) = executor.execute(task_to_spawn.unit_error().compat()) { - debug!( - target: "service", - "Failed to spawn background task: {:?}; falling back to manual polling", - err - ); - this.to_poll.push(Box::pin(err.into_future().compat().map(drop))); - } - } - - // Polling all the `to_poll` futures. - while let Some(pos) = this.to_poll.iter_mut().position(|t| Pin::new(t).poll(cx).is_ready()) { - let _ = this.to_poll.remove(pos); + (this.tasks_executor)(task_to_spawn); } // The service future never ends. diff --git a/client/service/test/src/lib.rs b/client/service/test/src/lib.rs index dd6395e9c6228..961c8d98ffd09 100644 --- a/client/service/test/src/lib.rs +++ b/client/service/test/src/lib.rs @@ -19,9 +19,11 @@ use std::iter; use std::sync::{Arc, Mutex, MutexGuard}; use std::net::Ipv4Addr; +use std::pin::Pin; use std::time::Duration; use log::info; use futures01::{Future, Stream, Poll}; +use futures::{FutureExt as _, TryFutureExt as _}; use tempfile::TempDir; use tokio::{runtime::Runtime, prelude::FutureExt}; use tokio::timer::Interval; @@ -131,6 +133,7 @@ fn node_config ( index: usize, spec: &ChainSpec, role: Roles, + tasks_executor: Box + Send>>) + Send>, key_seed: Option, base_port: u16, root: &TempDir, @@ -172,6 +175,7 @@ fn node_config ( impl_version: "0.1", impl_commit: "", roles: role, + tasks_executor: Some(tasks_executor), transaction_pool: Default::default(), network: network_config, keystore: KeystoreConfig::Path { @@ -251,10 +255,15 @@ impl TestNet where let executor = self.runtime.executor(); for (key, authority) in authorities { + let tasks_executor = { + let executor = executor.clone(); + Box::new(move |fut: Pin + Send>>| executor.spawn(fut.unit_error().compat())) + }; let node_config = node_config( self.nodes, &self.chain_spec, Roles::AUTHORITY, + tasks_executor, Some(key), self.base_port, &temp, @@ -270,7 +279,11 @@ impl TestNet where } for full in full { - let node_config = node_config(self.nodes, &self.chain_spec, Roles::FULL, None, self.base_port, &temp); + let tasks_executor = { + let executor = executor.clone(); + Box::new(move |fut: Pin + Send>>| executor.spawn(fut.unit_error().compat())) + }; + let node_config = node_config(self.nodes, &self.chain_spec, Roles::FULL, tasks_executor, None, self.base_port, &temp); let addr = node_config.network.listen_addresses.iter().next().unwrap().clone(); let (service, user_data) = full(node_config).expect("Error creating test node service"); let service = SyncService::from(service); @@ -282,7 +295,11 @@ impl TestNet where } for light in light { - let node_config = node_config(self.nodes, &self.chain_spec, Roles::LIGHT, None, self.base_port, &temp); + let tasks_executor = { + let executor = executor.clone(); + Box::new(move |fut: Pin + Send>>| executor.spawn(fut.unit_error().compat())) + }; + let node_config = node_config(self.nodes, &self.chain_spec, Roles::LIGHT, tasks_executor, None, self.base_port, &temp); let addr = node_config.network.listen_addresses.iter().next().unwrap().clone(); let service = SyncService::from(light(node_config).expect("Error creating test node service")); diff --git a/utils/browser/src/lib.rs b/utils/browser/src/lib.rs index 0dbde57182766..7b7fda45839ee 100644 --- a/utils/browser/src/lib.rs +++ b/utils/browser/src/lib.rs @@ -52,6 +52,9 @@ where allow_private_ipv4: true, enable_mdns: false, }; + config.tasks_executor = Some(Box::new(move |fut| { + wasm_bindgen_futures::spawn_local(fut) + })); config.telemetry_external_transport = Some(transport); config.roles = Roles::LIGHT; config.name = format!("{} (Browser)", name); From 14720148ee5a07911fbba8c91ffd9f4490bded68 Mon Sep 17 00:00:00 2001 From: Hero Bird Date: Tue, 21 Jan 2020 13:06:54 +0100 Subject: [PATCH 12/19] contracts: New contract events + unconfusions (#4685) * contracts: during execution -> contract trapped during execution This message confused many people so we are improving it to make clear what happened. * contracts: rename Event::Contract -> Event::ContractExecution * contracts: fix tests after ContractExecution renaming * contracts: Add Evicted and Restored events * fix doc comment * wrap to not go over (soft) 100 column line limit * add event deposit for eventual eviction upon pay_rent * contracts: adjust tests for the new events * emit Evicted event immediately and add tombstone flag bool --- frame/contracts/src/exec.rs | 2 +- frame/contracts/src/lib.rs | 30 ++++++++- frame/contracts/src/rent.rs | 6 +- frame/contracts/src/tests.rs | 98 +++++++++++++++++++++++++++-- frame/contracts/src/wasm/mod.rs | 6 +- frame/contracts/src/wasm/runtime.rs | 2 +- 6 files changed, 131 insertions(+), 13 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 2c77173135077..9d786c320b5a9 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -782,7 +782,7 @@ where fn deposit_event(&mut self, topics: Vec, data: Vec) { self.ctx.deferred.push(DeferredAction::DepositEvent { topics, - event: RawEvent::Contract(self.ctx.self_account.clone(), data), + event: RawEvent::ContractExecution(self.ctx.self_account.clone(), data), }); } diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index df8da8866020d..40ce86518a5e4 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -786,7 +786,12 @@ impl Module { rent_allowance, delta, } => { - let _result = Self::restore_to(donor, dest, code_hash, rent_allowance, delta); + let result = Self::restore_to( + donor.clone(), dest.clone(), code_hash.clone(), rent_allowance.clone(), delta + ); + Self::deposit_event( + RawEvent::Restored(donor, dest, code_hash, rent_allowance, result.is_ok()) + ); } } }); @@ -896,6 +901,25 @@ decl_event! { /// Contract deployed by address at the specified address. Instantiated(AccountId, AccountId), + /// Contract has been evicted and is now in tombstone state. + /// + /// # Params + /// + /// - `contract`: `AccountId`: The account ID of the evicted contract. + /// - `tombstone`: `bool`: True if the evicted contract left behind a tombstone. + Evicted(AccountId, bool), + + /// Restoration for a contract has been initiated. + /// + /// # Params + /// + /// - `donor`: `AccountId`: Account ID of the restoring contract + /// - `dest`: `AccountId`: Account ID of the restored contract + /// - `code_hash`: `Hash`: Code hash of the restored contract + /// - `rent_allowance: `Balance`: Rent allowance of the restored contract + /// - `success`: `bool`: True if the restoration was successful + Restored(AccountId, AccountId, Hash, Balance, bool), + /// Code with the specified hash has been stored. CodeStored(Hash), @@ -906,8 +930,8 @@ decl_event! { /// successful execution or not. Dispatched(AccountId, bool), - /// An event from contract of account. - Contract(AccountId, Vec), + /// An event deposited upon execution of a contract from the account. + ContractExecution(AccountId, Vec), } } diff --git a/frame/contracts/src/rent.rs b/frame/contracts/src/rent.rs index 59c8b02d199cf..508511da4cbe3 100644 --- a/frame/contracts/src/rent.rs +++ b/frame/contracts/src/rent.rs @@ -14,7 +14,8 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -use crate::{BalanceOf, ContractInfo, ContractInfoOf, TombstoneContractInfo, Trait, AliveContractInfo}; +use crate::{Module, RawEvent, BalanceOf, ContractInfo, ContractInfoOf, TombstoneContractInfo, + Trait, AliveContractInfo}; use sp_runtime::traits::{Bounded, CheckedDiv, CheckedMul, Saturating, Zero, SaturatedConversion}; use frame_support::traits::{Currency, ExistenceRequirement, Get, WithdrawReason, OnUnbalanced}; @@ -101,6 +102,7 @@ fn try_evict_or_and_pay_rent( // The contract cannot afford to leave a tombstone, so remove the contract info altogether. >::remove(account); child::kill_storage(&contract.trie_id, contract.child_trie_unique_id()); + >::deposit_event(RawEvent::Evicted(account.clone(), false)); return (RentOutcome::Evicted, None); } @@ -160,6 +162,8 @@ fn try_evict_or_and_pay_rent( child::kill_storage(&contract.trie_id, contract.child_trie_unique_id()); + >::deposit_event(RawEvent::Evicted(account.clone(), true)); + return (RentOutcome::Evicted, Some(tombstone_info)); } diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 91679a9216a16..9a2ef36bb86f0 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -453,7 +453,7 @@ fn instantiate_and_call_and_deposit_event() { }, EventRecord { phase: Phase::ApplyExtrinsic(0), - event: MetaEvent::contract(RawEvent::Contract(BOB, vec![1, 2, 3, 4])), + event: MetaEvent::contract(RawEvent::ContractExecution(BOB, vec![1, 2, 3, 4])), topics: vec![], }, EventRecord { @@ -650,7 +650,7 @@ fn dispatch_call_not_dispatched_after_top_level_transaction_failure() { 100_000, vec![], ), - "during execution" + "contract trapped during execution" ); assert_eq!(System::events(), vec![ EventRecord { @@ -1139,8 +1139,16 @@ fn call_removed_contract() { Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, call::null()), "contract has been evicted" ); + // Calling a contract that is about to evict shall emit an event. + assert_eq!(System::events(), vec![ + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract(RawEvent::Evicted(BOB, true)), + topics: vec![], + }, + ]); - // Subsequent contract calls should also fail. + // Subsequent contract calls should also fail. assert_err!( Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, call::null()), "contract has been evicted" @@ -1367,6 +1375,9 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: // Advance 4 blocks, to the 5th. initialize_block(5); + /// Preserve `BOB`'s code hash for later introspection. + let bob_code_hash = ContractInfoOf::::get(BOB).unwrap() + .get_alive().unwrap().code_hash; // Call `BOB`, which makes it pay rent. Since the rent allowance is set to 0 // we expect that it will get removed leaving tombstone. assert_err!( @@ -1374,6 +1385,15 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: "contract has been evicted" ); assert!(ContractInfoOf::::get(BOB).unwrap().get_tombstone().is_some()); + assert_eq!(System::events(), vec![ + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract( + RawEvent::Evicted(BOB.clone(), true) + ), + topics: vec![], + }, + ]); /// Create another account with the address `DJANGO` with `CODE_RESTORATION`. /// @@ -1416,6 +1436,60 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: assert_eq!(django_contract.storage_size, 16); assert_eq!(django_contract.trie_id, django_trie_id); assert_eq!(django_contract.deduct_block, System::block_number()); + match (test_different_storage, test_restore_to_with_dirty_storage) { + (true, false) => { + assert_eq!(System::events(), vec![ + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract( + RawEvent::Restored(DJANGO, BOB, bob_code_hash, 50, false) + ), + topics: vec![], + }, + ]); + } + (_, true) => { + assert_eq!(System::events(), vec![ + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract(RawEvent::Evicted(BOB, true)), + topics: vec![], + }, + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::balances(pallet_balances::RawEvent::NewAccount(CHARLIE, 1_000_000)), + topics: vec![], + }, + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::balances(pallet_balances::RawEvent::NewAccount(DJANGO, 30_000)), + topics: vec![], + }, + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract(RawEvent::Transfer(CHARLIE, DJANGO, 30_000)), + topics: vec![], + }, + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract(RawEvent::Instantiated(CHARLIE, DJANGO)), + topics: vec![], + }, + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract(RawEvent::Restored( + DJANGO, + BOB, + bob_code_hash, + 50, + false, + )), + topics: vec![], + }, + ]); + } + _ => unreachable!(), + } } else { // Here we expect that the restoration is succeeded. Check that the restoration // contract `DJANGO` ceased to exist and that `BOB` returned back. @@ -1427,6 +1501,20 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: assert_eq!(bob_contract.trie_id, django_trie_id); assert_eq!(bob_contract.deduct_block, System::block_number()); assert!(ContractInfoOf::::get(DJANGO).is_none()); + assert_eq!(System::events(), vec![ + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::balances(balances::RawEvent::ReapedAccount(DJANGO, 0)), + topics: vec![], + }, + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract( + RawEvent::Restored(DJANGO, BOB, bob_contract.code_hash, 50, true) + ), + topics: vec![], + }, + ]); } }); } @@ -1533,7 +1621,7 @@ fn storage_max_value_limit() { 100_000, Encode::encode(&(self::MaxValueSize::get() + 1)), ), - "during execution" + "contract trapped during execution" ); }); } @@ -2056,7 +2144,7 @@ fn cannot_self_destruct_while_live() { 100_000, vec![0], ), - "during execution" + "contract trapped during execution" ); // Check that BOB is still alive. diff --git a/frame/contracts/src/wasm/mod.rs b/frame/contracts/src/wasm/mod.rs index 8e02eb482bb8a..60402cf3a09cf 100644 --- a/frame/contracts/src/wasm/mod.rs +++ b/frame/contracts/src/wasm/mod.rs @@ -1430,7 +1430,9 @@ mod tests { MockExt::default(), &mut gas_meter ), - Err(ExecError { reason: DispatchError::Other("during execution"), buffer: _ }) + Err(ExecError { + reason: DispatchError::Other("contract trapped during execution"), buffer: _ + }) ); } @@ -1472,7 +1474,7 @@ mod tests { MockExt::default(), &mut gas_meter ), - Err(ExecError { reason: DispatchError::Other("during execution"), buffer: _ }) + Err(ExecError { reason: DispatchError::Other("contract trapped during execution"), buffer: _ }) ); } diff --git a/frame/contracts/src/wasm/runtime.rs b/frame/contracts/src/wasm/runtime.rs index 81b0809f82b02..75751b6d359a3 100644 --- a/frame/contracts/src/wasm/runtime.rs +++ b/frame/contracts/src/wasm/runtime.rs @@ -109,7 +109,7 @@ pub(crate) fn to_execution_result( Err(ExecError { reason: "validation error".into(), buffer: runtime.scratch_buf }), // Any other kind of a trap should result in a failure. Err(sp_sandbox::Error::Execution) | Err(sp_sandbox::Error::OutOfBounds) => - Err(ExecError { reason: "during execution".into(), buffer: runtime.scratch_buf }), + Err(ExecError { reason: "contract trapped during execution".into(), buffer: runtime.scratch_buf }), } } From ef970571ccd87d973a6a359bf709a357303161dc Mon Sep 17 00:00:00 2001 From: Nikolay Volf Date: Tue, 21 Jan 2020 08:01:55 -0800 Subject: [PATCH 13/19] fix docs deadlinks (#4698) --- client/consensus/slots/src/lib.rs | 2 +- client/src/lib.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/client/consensus/slots/src/lib.rs b/client/consensus/slots/src/lib.rs index 3aa243af72b06..c6185d5d307b9 100644 --- a/client/consensus/slots/src/lib.rs +++ b/client/consensus/slots/src/lib.rs @@ -45,7 +45,7 @@ use parking_lot::Mutex; /// The changes that need to applied to the storage to create the state for a block. /// -/// See [`state_machine::StorageChanges`] for more information. +/// See [`sp_state_machine::StorageChanges`] for more information. pub type StorageChanges = sp_state_machine::StorageChanges, NumberFor>; diff --git a/client/src/lib.rs b/client/src/lib.rs index db2d4785a2c98..8aa71c5ec5c88 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -20,20 +20,20 @@ //! parts: //! //! - A database containing the blocks and chain state, generally referred to as -//! the [`Backend`](backend::Backend). +//! the [`Backend`](sc_client_api::backend::Backend). //! - A runtime environment, generally referred to as the [`Executor`](CallExecutor). //! //! # Initialization //! //! Creating a [`Client`] is done by calling the `new` method and passing to it a -//! [`Backend`](backend::Backend) and an [`Executor`](CallExecutor). +//! [`Backend`](sc_client_api::backend::Backend) and an [`Executor`](CallExecutor). //! //! The former is typically provided by the `sc-client-db` crate. //! //! The latter typically requires passing one of: //! //! - A [`LocalCallExecutor`] running the runtime locally. -//! - A [`RemoteCallExecutor`](light::call_executor::RemoteCallExecutor) that will ask a +//! - A [`RemoteCallExecutor`](light::call_executor::RemoteCallRequest) that will ask a //! third-party to perform the executions. //! - A [`RemoteOrLocalCallExecutor`](light::call_executor::RemoteOrLocalCallExecutor), combination //! of the two. From 4b2f70f8476a8c4092df952e549090268f936d1d Mon Sep 17 00:00:00 2001 From: Nikolay Volf Date: Tue, 21 Jan 2020 13:10:26 -0800 Subject: [PATCH 14/19] remove license preamble from node-template (#4699) --- bin/node-template/runtime/build.rs | 16 ---------------- bin/node-template/src/main.rs | 2 -- 2 files changed, 18 deletions(-) diff --git a/bin/node-template/runtime/build.rs b/bin/node-template/runtime/build.rs index dc7e949d809f8..8bdf7584bb81a 100644 --- a/bin/node-template/runtime/build.rs +++ b/bin/node-template/runtime/build.rs @@ -1,19 +1,3 @@ -// Copyright 2019-2020 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 . - use wasm_builder_runner::{build_current_project_with_rustflags, WasmBuilderSource}; fn main() { diff --git a/bin/node-template/src/main.rs b/bin/node-template/src/main.rs index 2942bb68a37aa..ea64bc1413642 100644 --- a/bin/node-template/src/main.rs +++ b/bin/node-template/src/main.rs @@ -1,7 +1,5 @@ //! Substrate Node Template CLI library. - #![warn(missing_docs)] -#![warn(unused_extern_crates)] mod chain_spec; #[macro_use] From cf020addf0e2d9d5fdaa7a34aa83b380f49471bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 21 Jan 2020 21:14:44 +0000 Subject: [PATCH 15/19] grandpa: filter some telemetry events on larger voter sets (#4700) --- .../finality-grandpa/src/communication/mod.rs | 65 +++++++++++-------- 1 file changed, 37 insertions(+), 28 deletions(-) diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index 5077d435e8550..18cb14c739636 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -106,6 +106,11 @@ mod benefit { pub(super) const PER_EQUIVOCATION: i32 = 10; } +/// If the voter set is larger than this value some telemetry events are not +/// sent to avoid increasing usage resource on the node and flooding the +/// telemetry server (e.g. received votes, received commits.) +const TELEMETRY_VOTERS_LIMIT: usize = 10; + /// A handle to the network. /// /// Something that provides both the capabilities needed for the `gossip_network::Network` trait as @@ -308,29 +313,31 @@ impl> NetworkBridge { return Ok(None); } - match &msg.message.message { - PrimaryPropose(propose) => { - telemetry!(CONSENSUS_INFO; "afg.received_propose"; - "voter" => ?format!("{}", msg.message.id), - "target_number" => ?propose.target_number, - "target_hash" => ?propose.target_hash, - ); - }, - Prevote(prevote) => { - telemetry!(CONSENSUS_INFO; "afg.received_prevote"; - "voter" => ?format!("{}", msg.message.id), - "target_number" => ?prevote.target_number, - "target_hash" => ?prevote.target_hash, - ); - }, - Precommit(precommit) => { - telemetry!(CONSENSUS_INFO; "afg.received_precommit"; - "voter" => ?format!("{}", msg.message.id), - "target_number" => ?precommit.target_number, - "target_hash" => ?precommit.target_hash, - ); - }, - }; + if voters.len() <= TELEMETRY_VOTERS_LIMIT { + match &msg.message.message { + PrimaryPropose(propose) => { + telemetry!(CONSENSUS_INFO; "afg.received_propose"; + "voter" => ?format!("{}", msg.message.id), + "target_number" => ?propose.target_number, + "target_hash" => ?propose.target_hash, + ); + }, + Prevote(prevote) => { + telemetry!(CONSENSUS_INFO; "afg.received_prevote"; + "voter" => ?format!("{}", msg.message.id), + "target_number" => ?prevote.target_number, + "target_hash" => ?prevote.target_hash, + ); + }, + Precommit(precommit) => { + telemetry!(CONSENSUS_INFO; "afg.received_precommit"; + "voter" => ?format!("{}", msg.message.id), + "target_number" => ?precommit.target_number, + "target_hash" => ?precommit.target_hash, + ); + }, + }; + } Ok(Some(msg.message)) } @@ -474,11 +481,13 @@ fn incoming_global( format!("{}", a) }).collect(); - telemetry!(CONSENSUS_INFO; "afg.received_commit"; - "contains_precommits_signed_by" => ?precommits_signed_by, - "target_number" => ?msg.message.target_number.clone(), - "target_hash" => ?msg.message.target_hash.clone(), - ); + if voters.len() <= TELEMETRY_VOTERS_LIMIT { + telemetry!(CONSENSUS_INFO; "afg.received_commit"; + "contains_precommits_signed_by" => ?precommits_signed_by, + "target_number" => ?msg.message.target_number.clone(), + "target_hash" => ?msg.message.target_hash.clone(), + ); + } if let Err(cost) = check_compact_commit::( &msg.message, From ef578cd5102e73dcca8ee902555467fed1031792 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 22 Jan 2020 12:17:52 +0100 Subject: [PATCH 16/19] Support `u128`/`i128` in runtime interface (#4703) * Support `u128`/`i128` in runtime interface This implements support for `u128`/`i128` as parameters/return value in runtime interfaces. As we can not pass them as identity, as for the other primitives types, we pass them as an pointer to an `[u8; 16]` array. * Remove some unsafe code usage --- primitives/runtime-interface/src/impls.rs | 52 +++++++++++++++++++ primitives/runtime-interface/src/lib.rs | 3 ++ .../runtime-interface/test-wasm/src/lib.rs | 20 +++++++ primitives/runtime-interface/test/src/lib.rs | 5 ++ 4 files changed, 80 insertions(+) diff --git a/primitives/runtime-interface/src/impls.rs b/primitives/runtime-interface/src/impls.rs index 3cd114268bbd3..cde8e60eea350 100644 --- a/primitives/runtime-interface/src/impls.rs +++ b/primitives/runtime-interface/src/impls.rs @@ -471,3 +471,55 @@ impl IntoFFIValue for Pointer { Ok(self.into()) } } + +/// Implement the traits for `u128`/`i128` +macro_rules! for_u128_i128 { + ($type:ty) => { + /// `u128`/`i128` is passed as `u32`. + /// + /// The `u32` is a pointer to an `[u8; 16]` array. + impl RIType for $type { + type FFIType = u32; + } + + #[cfg(not(feature = "std"))] + impl IntoFFIValue for $type { + type Owned = (); + + fn into_ffi_value(&self) -> WrappedFFIValue { + unsafe { (mem::transmute::<&Self, *const u8>(self) as u32).into() } + } + } + + #[cfg(not(feature = "std"))] + impl FromFFIValue for $type { + fn from_ffi_value(arg: u32) -> $type { + <$type>::from_le_bytes(<[u8; mem::size_of::<$type>()]>::from_ffi_value(arg)) + } + } + + #[cfg(feature = "std")] + impl FromFFIValue for $type { + type SelfInstance = $type; + + fn from_ffi_value(context: &mut dyn FunctionContext, arg: u32) -> Result<$type> { + let data = context.read_memory(Pointer::new(arg), mem::size_of::<$type>() as u32)?; + let mut res = [0u8; mem::size_of::<$type>()]; + res.copy_from_slice(&data); + Ok(<$type>::from_le_bytes(res)) + } + } + + #[cfg(feature = "std")] + impl IntoFFIValue for $type { + fn into_ffi_value(self, context: &mut dyn FunctionContext) -> Result { + let addr = context.allocate_memory(mem::size_of::<$type>() as u32)?; + context.write_memory(addr, &self.to_le_bytes())?; + Ok(addr.into()) + } + } + } +} + +for_u128_i128!(u128); +for_u128_i128!(i128); diff --git a/primitives/runtime-interface/src/lib.rs b/primitives/runtime-interface/src/lib.rs index 30d74ad2db399..609f4f600b784 100644 --- a/primitives/runtime-interface/src/lib.rs +++ b/primitives/runtime-interface/src/lib.rs @@ -81,10 +81,12 @@ //! | `u16` | `u16` | `Identity` | //! | `u32` | `u32` | `Identity` | //! | `u64` | `u64` | `Identity` | +//! | `i128` | `u32` | `v.as_ptr()` (pointer to a 16 byte array) | //! | `i8` | `i8` | `Identity` | //! | `i16` | `i16` | `Identity` | //! | `i32` | `i32` | `Identity` | //! | `i64` | `i64` | `Identity` | +//! | `u128` | `u32` | `v.as_ptr()` (pointer to a 16 byte array) | //! | `bool` | `u8` | `if v { 1 } else { 0 }` | //! | `&str` | `u64` | v.len() 32bit << 32 | v.as_ptr() 32bit | //! | `&[u8]` | `u64` | v.len() 32bit << 32 | v.as_ptr() 32bit | @@ -93,6 +95,7 @@ //! | `&[T] where T: Encode` | `u64` | `let e = v.encode();`

e.len() 32bit << 32 | e.as_ptr() 32bit | //! | `[u8; N]` | `u32` | `v.as_ptr()` | //! | `*const T` | `u32` | `Identity` | +//! | `Option` | `u64` | `let e = v.encode();`

e.len() 32bit << 32 | e.as_ptr() 32bit | //! | [`T where T: PassBy`](pass_by::Inner) | Depends on inner | Depends on inner | //! | [`T where T: PassBy`](pass_by::Codec) | `u64`| v.len() 32bit << 32 | v.as_ptr() 32bit | //! diff --git a/primitives/runtime-interface/test-wasm/src/lib.rs b/primitives/runtime-interface/test-wasm/src/lib.rs index a7e7e3e983a97..67fbfdcfec602 100644 --- a/primitives/runtime-interface/test-wasm/src/lib.rs +++ b/primitives/runtime-interface/test-wasm/src/lib.rs @@ -82,6 +82,16 @@ pub trait TestApi { fn overwrite_native_function_implementation() -> bool { false } + + /// Gets an `u128` and returns this value + fn get_and_return_u128(val: u128) -> u128 { + val + } + + /// Gets an `i128` and returns this value + fn get_and_return_i128(val: i128) -> i128 { + val + } } /// Two random external functions from the old runtime interface. @@ -191,4 +201,14 @@ wasm_export_functions! { assert!(test_api::overwrite_native_function_implementation()); } + + fn test_u128_i128_as_parameter_and_return_value() { + for val in &[u128::max_value(), 1u128, 5000u128, u64::max_value() as u128] { + assert_eq!(*val, test_api::get_and_return_u128(*val)); + } + + for val in &[i128::max_value(), i128::min_value(), 1i128, 5000i128, u64::max_value() as i128] { + assert_eq!(*val, test_api::get_and_return_i128(*val)); + } + } } diff --git a/primitives/runtime-interface/test/src/lib.rs b/primitives/runtime-interface/test/src/lib.rs index b209e1f71ce55..48c120b2c9fd1 100644 --- a/primitives/runtime-interface/test/src/lib.rs +++ b/primitives/runtime-interface/test/src/lib.rs @@ -108,3 +108,8 @@ fn test_invalid_utf8_data_should_return_an_error() { fn test_overwrite_native_function_implementation() { call_wasm_method::("test_overwrite_native_function_implementation"); } + +#[test] +fn test_u128_i128_as_parameter_and_return_value() { + call_wasm_method::("test_u128_i128_as_parameter_and_return_value"); +} From 2cc177251d600482953a91d92d0f66c55a0385d9 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 22 Jan 2020 14:49:28 +0100 Subject: [PATCH 17/19] client/authority-discovery/Cargo.toml: Update dependency (#4706) * client/authority-discovery/Cargo.toml: Update dependency * client/authority-discovery: Pass packet payload and addresses as slice Starting with Bytes 0.5 `Vec` does not implement `Buf`, but `&[T]` does. --- Cargo.lock | 25 ++++++++++++++++++++++++- client/authority-discovery/Cargo.toml | 2 +- client/authority-discovery/src/lib.rs | 10 +++++----- 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8030a4cd13ad2..04a7a33595364 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4500,6 +4500,15 @@ dependencies = [ "prost-derive 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "prost" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "bytes 0.5.3 (registry+https://github.com/rust-lang/crates.io-index)", + "prost-derive 0.6.1 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "prost-build" version = "0.5.0" @@ -4529,6 +4538,18 @@ dependencies = [ "syn 0.15.44 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "prost-derive" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "anyhow 1.0.25 (registry+https://github.com/rust-lang/crates.io-index)", + "itertools 0.8.2 (registry+https://github.com/rust-lang/crates.io-index)", + "proc-macro2 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)", + "quote 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)", + "syn 1.0.11 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "prost-types" version = "0.5.0" @@ -5012,7 +5033,7 @@ dependencies = [ "libp2p 0.14.0-alpha.1 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", "parity-scale-codec 1.1.2 (registry+https://github.com/rust-lang/crates.io-index)", - "prost 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)", + "prost 0.6.1 (registry+https://github.com/rust-lang/crates.io-index)", "prost-build 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)", "quickcheck 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", "rand 0.7.2 (registry+https://github.com/rust-lang/crates.io-index)", @@ -8590,8 +8611,10 @@ dependencies = [ "checksum proc-macro2 0.4.30 (registry+https://github.com/rust-lang/crates.io-index)" = "cf3d2011ab5c909338f7887f4fc896d35932e29146c12c8d01da6b22a80ba759" "checksum proc-macro2 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)" = "9c9e470a8dc4aeae2dee2f335e8f533e2d4b347e1434e5671afc49b054592f27" "checksum prost 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)" = "96d14b1c185652833d24aaad41c5832b0be5616a590227c1fbff57c616754b23" +"checksum prost 0.6.1 (registry+https://github.com/rust-lang/crates.io-index)" = "ce49aefe0a6144a45de32927c77bd2859a5f7677b55f220ae5b744e87389c212" "checksum prost-build 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)" = "eb788126ea840817128183f8f603dce02cb7aea25c2a0b764359d8e20010702e" "checksum prost-derive 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)" = "5e7dc378b94ac374644181a2247cebf59a6ec1c88b49ac77f3a94b86b79d0e11" +"checksum prost-derive 0.6.1 (registry+https://github.com/rust-lang/crates.io-index)" = "537aa19b95acde10a12fec4301466386f757403de4cd4e5b4fa78fb5ecb18f72" "checksum prost-types 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)" = "1de482a366941c8d56d19b650fac09ca08508f2a696119ee7513ad590c8bac6f" "checksum protobuf 2.8.1 (registry+https://github.com/rust-lang/crates.io-index)" = "40361836defdd5871ff7e84096c6f6444af7fc157f8ef1789f54f147687caa20" "checksum pwasm-utils 0.12.0 (registry+https://github.com/rust-lang/crates.io-index)" = "4f7a12f176deee919f4ba55326ee17491c8b707d0987aed822682c821b660192" diff --git a/client/authority-discovery/Cargo.toml b/client/authority-discovery/Cargo.toml index 7381e5add19a1..25c3f161d4018 100644 --- a/client/authority-discovery/Cargo.toml +++ b/client/authority-discovery/Cargo.toml @@ -16,7 +16,7 @@ futures = "0.3.1" futures-timer = "2.0" libp2p = { version = "0.14.0-alpha.1", default-features = false, features = ["secp256k1", "libp2p-websocket"] } log = "0.4.8" -prost = "0.5.0" +prost = "0.6.1" rand = "0.7.2" sc-client-api = { version = "2.0.0", path = "../api" } sc-keystore = { version = "2.0.0", path = "../keystore" } diff --git a/client/authority-discovery/src/lib.rs b/client/authority-discovery/src/lib.rs index ee2b655b925a6..6260ac9a85b12 100644 --- a/client/authority-discovery/src/lib.rs +++ b/client/authority-discovery/src/lib.rs @@ -315,10 +315,10 @@ where let remote_addresses: Vec = values.into_iter() .map(|(_k, v)| { - let schema::SignedAuthorityAddresses { - signature, - addresses, - } = schema::SignedAuthorityAddresses::decode(v).map_err(Error::DecodingProto)?; + let schema::SignedAuthorityAddresses { signature, addresses } = + schema::SignedAuthorityAddresses::decode(v.as_slice()) + .map_err(Error::DecodingProto)?; + let signature = AuthoritySignature::decode(&mut &signature[..]) .map_err(Error::EncodingDecodingScale)?; @@ -326,7 +326,7 @@ where return Err(Error::VerifyingDhtPayload); } - let addresses: Vec = schema::AuthorityAddresses::decode(addresses) + let addresses = schema::AuthorityAddresses::decode(addresses.as_slice()) .map(|a| a.addresses) .map_err(Error::DecodingProto)? .into_iter() From 6c3b86d16ad19751fbbab86e369883b5425c72df Mon Sep 17 00:00:00 2001 From: Nikolay Volf Date: Wed, 22 Jan 2020 05:53:40 -0800 Subject: [PATCH 18/19] More cleanups in node-template (#4705) * remove and reformat * remove some more * commit Cargo.lock --- Cargo.lock | 4 -- bin/node-template/Cargo.toml | 4 -- bin/node-template/src/chain_spec.rs | 62 +++++++++++++++-------------- 3 files changed, 33 insertions(+), 37 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 04a7a33595364..eadf251f759ee 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3221,8 +3221,6 @@ dependencies = [ "futures 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", "node-template-runtime 2.0.0", - "parity-scale-codec 1.1.2 (registry+https://github.com/rust-lang/crates.io-index)", - "parking_lot 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", "sc-basic-authorship 0.8.0", "sc-cli 0.8.0", "sc-client 0.8.0", @@ -3237,12 +3235,10 @@ dependencies = [ "sp-core 2.0.0", "sp-finality-grandpa 2.0.0", "sp-inherents 2.0.0", - "sp-io 2.0.0", "sp-runtime 2.0.0", "sp-transaction-pool 2.0.0", "substrate-build-script-utils 2.0.0", "tokio 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", - "trie-root 0.15.2 (registry+https://github.com/rust-lang/crates.io-index)", "vergen 3.0.4 (registry+https://github.com/rust-lang/crates.io-index)", ] diff --git a/bin/node-template/Cargo.toml b/bin/node-template/Cargo.toml index f956bbb22193a..cf3d7509f45d3 100644 --- a/bin/node-template/Cargo.toml +++ b/bin/node-template/Cargo.toml @@ -14,10 +14,6 @@ futures = "0.3.1" ctrlc = { version = "3.1.3", features = ["termination"] } log = "0.4.8" tokio = { version = "0.2", features = ["rt-threaded"] } -parking_lot = "0.9.0" -codec = { package = "parity-scale-codec", version = "1.0.0" } -trie-root = "0.15.2" -sp-io = { version = "2.0.0", path = "../../primitives/io" } sc-cli = { version = "0.8.0", path = "../../client/cli" } sp-core = { version = "2.0.0", path = "../../primitives/core" } sc-executor = { version = "0.8", path = "../../client/executor" } diff --git a/bin/node-template/src/chain_spec.rs b/bin/node-template/src/chain_spec.rs index fae9feaf5113b..6afb67547bd6c 100644 --- a/bin/node-template/src/chain_spec.rs +++ b/bin/node-template/src/chain_spec.rs @@ -56,17 +56,19 @@ impl Alternative { Alternative::Development => ChainSpec::from_genesis( "Development", "dev", - || testnet_genesis(vec![ - get_authority_keys_from_seed("Alice"), - ], - get_account_id_from_seed::("Alice"), - vec![ + || testnet_genesis( + vec![ + get_authority_keys_from_seed("Alice"), + ], get_account_id_from_seed::("Alice"), - get_account_id_from_seed::("Bob"), - get_account_id_from_seed::("Alice//stash"), - get_account_id_from_seed::("Bob//stash"), - ], - true), + vec![ + get_account_id_from_seed::("Alice"), + get_account_id_from_seed::("Bob"), + get_account_id_from_seed::("Alice//stash"), + get_account_id_from_seed::("Bob//stash"), + ], + true, + ), vec![], None, None, @@ -76,26 +78,28 @@ impl Alternative { Alternative::LocalTestnet => ChainSpec::from_genesis( "Local Testnet", "local_testnet", - || testnet_genesis(vec![ - get_authority_keys_from_seed("Alice"), - get_authority_keys_from_seed("Bob"), - ], - get_account_id_from_seed::("Alice"), - vec![ + || testnet_genesis( + vec![ + get_authority_keys_from_seed("Alice"), + get_authority_keys_from_seed("Bob"), + ], get_account_id_from_seed::("Alice"), - get_account_id_from_seed::("Bob"), - get_account_id_from_seed::("Charlie"), - get_account_id_from_seed::("Dave"), - get_account_id_from_seed::("Eve"), - get_account_id_from_seed::("Ferdie"), - get_account_id_from_seed::("Alice//stash"), - get_account_id_from_seed::("Bob//stash"), - get_account_id_from_seed::("Charlie//stash"), - get_account_id_from_seed::("Dave//stash"), - get_account_id_from_seed::("Eve//stash"), - get_account_id_from_seed::("Ferdie//stash"), - ], - true), + vec![ + get_account_id_from_seed::("Alice"), + get_account_id_from_seed::("Bob"), + get_account_id_from_seed::("Charlie"), + get_account_id_from_seed::("Dave"), + get_account_id_from_seed::("Eve"), + get_account_id_from_seed::("Ferdie"), + get_account_id_from_seed::("Alice//stash"), + get_account_id_from_seed::("Bob//stash"), + get_account_id_from_seed::("Charlie//stash"), + get_account_id_from_seed::("Dave//stash"), + get_account_id_from_seed::("Eve//stash"), + get_account_id_from_seed::("Ferdie//stash"), + ], + true, + ), vec![], None, None, From ea9278a50f52a628b5c1b4e2f1541ce46a6beae8 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 23 Jan 2020 17:59:58 +0100 Subject: [PATCH 19/19] utils/prometheus: Make crate spawn onto global executor With 6ee1244e2 the Substrate green threading executor is now configurable. The Substrate Prometheus crate can use this executor to spawn futures per connection (scrape request). --- Cargo.lock | 6 ++- client/service/src/builder.rs | 2 +- utils/prometheus/Cargo.toml | 4 +- utils/prometheus/src/lib.rs | 46 ++++++++++----------- utils/prometheus/src/networking.rs | 66 ------------------------------ 5 files changed, 30 insertions(+), 94 deletions(-) delete mode 100644 utils/prometheus/src/networking.rs diff --git a/Cargo.lock b/Cargo.lock index da87ea8549aef..699be70286c91 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1954,6 +1954,7 @@ dependencies = [ "httparse 1.3.4 (registry+https://github.com/rust-lang/crates.io-index)", "itoa 0.4.4 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", + "net2 0.2.33 (registry+https://github.com/rust-lang/crates.io-index)", "pin-project 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", "time 0.1.42 (registry+https://github.com/rust-lang/crates.io-index)", "tokio 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", @@ -4494,7 +4495,7 @@ version = "0.8.0" dependencies = [ "async-std 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "derive_more 0.99.2 (registry+https://github.com/rust-lang/crates.io-index)", - "futures-util 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", + "futures 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "hyper 0.13.1 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", "prometheus 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -7275,9 +7276,12 @@ source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "bytes 0.5.3 (registry+https://github.com/rust-lang/crates.io-index)", "fnv 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)", + "lazy_static 1.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "memchr 2.2.1 (registry+https://github.com/rust-lang/crates.io-index)", + "mio 0.6.21 (registry+https://github.com/rust-lang/crates.io-index)", "num_cpus 1.11.1 (registry+https://github.com/rust-lang/crates.io-index)", "pin-project-lite 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", + "slab 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index 60b0888112eec..9e6b25584a6a1 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -1029,7 +1029,7 @@ ServiceBuilder< let metrics = ServiceMetrics::register(®istry)?; let future = select( - prometheus_exporter::init_prometheus(port, registry).boxed(), + prometheus_exporter::init_prometheus(port, registry, to_spawn_tx.clone()).boxed(), exit.clone() ).map(drop); diff --git a/utils/prometheus/Cargo.toml b/utils/prometheus/Cargo.toml index 8e50b4024e3bc..37da93257f933 100644 --- a/utils/prometheus/Cargo.toml +++ b/utils/prometheus/Cargo.toml @@ -8,10 +8,10 @@ edition = "2018" [dependencies] log = "0.4.8" -hyper = { version = "0.13.1", default-features = false, features = ["stream"] } +hyper = { version = "0.13.1", default-features = false, features = ["tcp"] } prometheus = { version = "0.7", features = ["nightly", "process"]} tokio = "0.2" -futures-util = { version = "0.3.1", default-features = false, features = ["io"] } +futures = "0.3.1" derive_more = "0.99" [target.'cfg(not(target_os = "unknown"))'.dependencies] diff --git a/utils/prometheus/src/lib.rs b/utils/prometheus/src/lib.rs index 25f058acc377d..d6436fe696940 100644 --- a/utils/prometheus/src/lib.rs +++ b/utils/prometheus/src/lib.rs @@ -14,13 +14,10 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -use futures_util::{FutureExt, future::Future}; -use hyper::http::StatusCode; -use hyper::{Server, Body, Response, service::{service_fn, make_service_fn}}; +use futures::{channel::mpsc, prelude::*}; +use hyper::{Body, http::StatusCode, Response, Server, service::{service_fn, make_service_fn}}; use prometheus::{Encoder, Opts, TextEncoder, core::Atomic}; -use std::net::SocketAddr; -#[cfg(not(target_os = "unknown"))] -mod networking; +use std::{net::SocketAddr, pin::Pin}; pub use prometheus::{ Registry, Error as PrometheusError, @@ -70,29 +67,28 @@ async fn request_metrics(registry: Registry) -> Result, Error> { } #[derive(Clone)] -pub struct Executor; +pub struct Executor { + to_spawn_tx: mpsc::UnboundedSender + Send>>>, +} #[cfg(not(target_os = "unknown"))] impl hyper::rt::Executor for Executor - where - T: Future + Send + 'static, - T::Output: Send + 'static, +where + T: Future + Send + 'static, { - fn execute(&self, future: T) { - async_std::task::spawn(future); + fn execute(&self, fut: T) { + self.to_spawn_tx.unbounded_send(Box::pin(fut.map(drop))) + .expect("sending on unbounded channel never fails; qed"); } } /// Initializes the metrics context, and starts an HTTP server /// to serve metrics. #[cfg(not(target_os = "unknown"))] -pub async fn init_prometheus(prometheus_addr: SocketAddr, registry: Registry) -> Result<(), Error>{ - use networking::Incoming; - let listener = async_std::net::TcpListener::bind(&prometheus_addr) - .await - .map_err(|_| Error::PortInUse(prometheus_addr))?; - - log::info!("Prometheus server started at {}", prometheus_addr); - +pub async fn init_prometheus( + prometheus_addr: SocketAddr, + registry: Registry, + to_spawn_tx: mpsc::UnboundedSender + Send>>>, +) -> Result<(), Error>{ let service = make_service_fn(move |_| { let registry = registry.clone(); @@ -103,14 +99,16 @@ pub async fn init_prometheus(prometheus_addr: SocketAddr, registry: Registry) -> } }); - let server = Server::builder(Incoming(listener.incoming())) - .executor(Executor) + let executor = Executor { to_spawn_tx }; + + let server = hyper::server::Server::try_bind(&prometheus_addr)? + .executor(executor) .serve(service) .boxed(); - let result = server.await.map_err(Into::into); + log::info!("Prometheus metrics served at {}/metrics", prometheus_addr); - result + server.await.map_err(Into::into) } #[cfg(target_os = "unknown")] diff --git a/utils/prometheus/src/networking.rs b/utils/prometheus/src/networking.rs deleted file mode 100644 index 5c8c036d44597..0000000000000 --- a/utils/prometheus/src/networking.rs +++ /dev/null @@ -1,66 +0,0 @@ -// Copyright 2019-2020 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 . - -use async_std::pin::Pin; -use std::task::{Poll, Context}; -use futures_util::{stream::Stream, io::{AsyncRead, AsyncWrite}}; - -pub struct Incoming<'a>(pub async_std::net::Incoming<'a>); - -impl hyper::server::accept::Accept for Incoming<'_> { - type Conn = TcpStream; - type Error = async_std::io::Error; - - fn poll_accept(self: Pin<&mut Self>, cx: &mut Context) -> Poll>> { - Pin::new(&mut Pin::into_inner(self).0) - .poll_next(cx) - .map(|opt| opt.map(|res| res.map(TcpStream))) - } -} - -pub struct TcpStream(pub async_std::net::TcpStream); - -impl tokio::io::AsyncRead for TcpStream { - fn poll_read( - self: Pin<&mut Self>, - cx: &mut Context, - buf: &mut [u8] - ) -> Poll> { - Pin::new(&mut Pin::into_inner(self).0) - .poll_read(cx, buf) - } -} - -impl tokio::io::AsyncWrite for TcpStream { - fn poll_write( - self: Pin<&mut Self>, - cx: &mut Context, - buf: &[u8] - ) -> Poll> { - Pin::new(&mut Pin::into_inner(self).0) - .poll_write(cx, buf) - } - - fn poll_flush(self: Pin<&mut Self>, cx: &mut Context) -> Poll> { - Pin::new(&mut Pin::into_inner(self).0) - .poll_flush(cx) - } - - fn poll_shutdown(self: Pin<&mut Self>, cx: &mut Context) -> Poll> { - Pin::new(&mut Pin::into_inner(self).0) - .poll_close(cx) - } -}