From 202c64928a928eb4628c2bb4f440bb454768dd11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 24 Mar 2021 16:02:25 +0100 Subject: [PATCH 01/27] Make grandpa work --- client/finality-grandpa/src/authorities.rs | 107 ++++++++++++++++++--- client/finality-grandpa/src/environment.rs | 4 +- client/finality-grandpa/src/import.rs | 44 +++++---- client/finality-grandpa/src/lib.rs | 2 +- client/finality-grandpa/src/observer.rs | 2 +- 5 files changed, 124 insertions(+), 35 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 1854a33d29f1f..35e04267305e1 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -19,7 +19,7 @@ //! Utilities for dealing with authorities, authority sets, and handoffs. use fork_tree::ForkTree; -use parking_lot::RwLock; +use parking_lot::{Mutex, MappedMutexGuard, Condvar, MutexGuard}; use finality_grandpa::voter_set::VoterSet; use parity_scale_codec::{Encode, Decode}; use log::debug; @@ -68,21 +68,103 @@ impl From for Error { } } +struct Inner { + authority_set: AuthoritySet, + locked: bool, +} + +pub(crate) struct InnerLocked2 { + shared_authority_set: SharedAuthoritySet, +} + +impl InnerLocked2 { + pub fn upgrade(&mut self) -> MappedMutexGuard> { + MutexGuard::map(self.shared_authority_set.inner.lock(), |i| &mut i.authority_set) + } +} + +impl Drop for InnerLocked2 { + fn drop(&mut self) { + let mut inner = self.shared_authority_set.inner.lock(); + inner.locked = false; + self.shared_authority_set.cond_var.notify_all(); + } +} + +pub(crate) struct InnerLocked<'a, H, N> { + inner: MutexGuard<'a, Inner>, + shared_authority_set: Option>, +} + +impl<'a, H, N> InnerLocked<'a, H, N> { + pub fn release_lock(mut self) -> InnerLocked2 { + InnerLocked2 { shared_authority_set: self.shared_authority_set.take().unwrap() } + } +} + +impl<'a, H, N> Drop for InnerLocked<'a, H, N> { + fn drop(&mut self) { + if let Some(authority_set) = self.shared_authority_set.take() { + self.inner.locked = false; + authority_set.cond_var.notify_all(); + } + } +} + +impl<'a, H, N> std::ops::Deref for InnerLocked<'a, H, N> { + type Target = AuthoritySet; + + fn deref(&self) -> &Self::Target { + &self.inner.authority_set + } +} + +impl<'a, H, N> std::ops::DerefMut for InnerLocked<'a, H, N> { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.inner.authority_set + } +} + /// A shared authority set. pub struct SharedAuthoritySet { - inner: Arc>>, + inner: Arc>>, + cond_var: Arc, } impl Clone for SharedAuthoritySet { fn clone(&self) -> Self { - SharedAuthoritySet { inner: self.inner.clone() } + SharedAuthoritySet { + inner: self.inner.clone(), + cond_var: self.cond_var.clone(), + } } } impl SharedAuthoritySet { /// Acquire a reference to the inner read-write lock. - pub(crate) fn inner(&self) -> &RwLock> { - &*self.inner + pub(crate) fn inner(&self) -> MappedMutexGuard> { + let mut guard = self.inner.lock(); + + if guard.locked { + self.cond_var.wait(&mut guard); + } + + debug_assert!(!guard.locked); + guard.locked = true; + + MutexGuard::map(guard, |i| &mut i.authority_set) + } + + pub(crate) fn inner_locked(&self) -> InnerLocked { + let mut guard = self.inner.lock(); + + if guard.locked { + self.cond_var.wait(&mut guard); + } + InnerLocked { + inner: guard, + shared_authority_set: Some(self.clone()), + } } } @@ -93,17 +175,17 @@ where N: Add + Ord + Clone + Debug, /// Get the earliest limit-block number that's higher or equal to the given /// min number, if any. pub(crate) fn current_limit(&self, min: N) -> Option { - self.inner.read().current_limit(min) + self.inner().current_limit(min) } /// Get the current set ID. This is incremented every time the set changes. pub fn set_id(&self) -> u64 { - self.inner.read().set_id + self.inner().set_id } /// Get the current authorities and their weights (for the current set ID). pub fn current_authorities(&self) -> VoterSet { - VoterSet::new(self.inner.read().current_authorities.iter().cloned()).expect( + VoterSet::new(self.inner().current_authorities.iter().cloned()).expect( "current_authorities is non-empty and weights are non-zero; \ constructor and all mutating operations on `AuthoritySet` ensure this; \ qed.", @@ -112,18 +194,21 @@ where N: Add + Ord + Clone + Debug, /// Clone the inner `AuthoritySet`. pub fn clone_inner(&self) -> AuthoritySet { - self.inner.read().clone() + self.inner().clone() } /// Clone the inner `AuthoritySetChanges`. pub fn authority_set_changes(&self) -> AuthoritySetChanges { - self.inner.read().authority_set_changes.clone() + self.inner().authority_set_changes.clone() } } impl From> for SharedAuthoritySet { fn from(set: AuthoritySet) -> Self { - SharedAuthoritySet { inner: Arc::new(RwLock::new(set)) } + SharedAuthoritySet { + inner: Arc::new(Mutex::new(Inner { authority_set: set, locked: false })), + cond_var: Default::default(), + } } } diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 27ff1e57b670c..3786355d2db4e 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -508,7 +508,7 @@ where .best_chain() .map_err(|e| Error::Blockchain(e.to_string()))?; - let authority_set = self.authority_set.inner().read(); + let authority_set = self.authority_set.inner(); // block hash and number of the next pending authority set change in the // given best chain. @@ -1228,7 +1228,7 @@ where // NOTE: lock must be held through writing to DB to avoid race. this lock // also implicitly synchronizes the check for last finalized number // below. - let mut authority_set = authority_set.inner().write(); + let mut authority_set = authority_set.inner(); let status = client.info(); diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index 6814d5dfb6195..d02d99569d4f3 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -99,7 +99,7 @@ impl JustificationImport let chain_info = self.inner.info(); // request justifications for all pending changes for which change blocks have already been imported - let authorities = self.authority_set.inner().read(); + let authorities = self.authority_set.inner(); for pending_change in authorities.pending_changes() { if pending_change.delay_kind == DelayKind::Finalized && pending_change.effective_number() > chain_info.finalized_number && @@ -157,30 +157,30 @@ impl AppliedChanges { } } -struct PendingSetChanges<'a, Block: 'a + BlockT> { +struct PendingSetChanges { just_in_case: Option<( AuthoritySet>, - RwLockWriteGuard<'a, AuthoritySet>>, + crate::authorities::InnerLocked2>, )>, applied_changes: AppliedChanges>, do_pause: bool, } -impl<'a, Block: 'a + BlockT> PendingSetChanges<'a, Block> { +impl PendingSetChanges { // revert the pending set change explicitly. fn revert(self) { } fn defuse(mut self) -> (AppliedChanges>, bool) { self.just_in_case = None; - let applied_changes = ::std::mem::replace(&mut self.applied_changes, AppliedChanges::None); + let applied_changes = std::mem::replace(&mut self.applied_changes, AppliedChanges::None); (applied_changes, self.do_pause) } } -impl<'a, Block: 'a + BlockT> Drop for PendingSetChanges<'a, Block> { +impl Drop for PendingSetChanges { fn drop(&mut self) { if let Some((old_set, mut authorities)) = self.just_in_case.take() { - *authorities = old_set; + *authorities.upgrade() = old_set; } } } @@ -269,33 +269,36 @@ where // when we update the authorities, we need to hold the lock // until the block is written to prevent a race if we need to restore // the old authority set on error or panic. - struct InnerGuard<'a, T: 'a> { - old: Option, - guard: Option>, + struct InnerGuard<'a, H, N> { + old: Option>, + guard: Option>, } - impl<'a, T: 'a> InnerGuard<'a, T> { - fn as_mut(&mut self) -> &mut T { + impl<'a, H, N> InnerGuard<'a, H, N> { + fn as_mut(&mut self) -> &mut AuthoritySet { &mut **self.guard.as_mut().expect("only taken on deconstruction; qed") } - fn set_old(&mut self, old: T) { + fn set_old(&mut self, old: AuthoritySet) { if self.old.is_none() { // ignore "newer" old changes. self.old = Some(old); } } - fn consume(mut self) -> Option<(T, RwLockWriteGuard<'a, T>)> { + fn consume(mut self) -> Option<(AuthoritySet, crate::authorities::InnerLocked<'a, H, N>)> { if let Some(old) = self.old.take() { - Some((old, self.guard.take().expect("only taken on deconstruction; qed"))) + Some(( + old, + self.guard.take().expect("only taken on deconstruction; qed"), + )) } else { None } } } - impl<'a, T: 'a> Drop for InnerGuard<'a, T> { + impl<'a, H, N> Drop for InnerGuard<'a, H, N> { fn drop(&mut self) { if let (Some(mut guard), Some(old)) = (self.guard.take(), self.old.take()) { *guard = old; @@ -315,7 +318,7 @@ where let is_descendent_of = is_descendent_of(&*self.inner, Some((hash, parent_hash))); let mut guard = InnerGuard { - guard: Some(self.authority_set.inner().write()), + guard: Some(self.authority_set.inner_locked()), old: None, }; @@ -413,6 +416,8 @@ where ); } + let just_in_case = just_in_case.map(|(o, i)| (o, i.release_lock())); + Ok(PendingSetChanges { just_in_case, applied_changes, do_pause }) } } @@ -580,8 +585,7 @@ impl GrandpaBlockImport GrandpaBlockImport Date: Wed, 24 Mar 2021 22:52:17 +0100 Subject: [PATCH 02/27] Introduce `SharedData` --- Cargo.lock | 1 + client/consensus/babe/src/lib.rs | 21 +- client/consensus/common/Cargo.toml | 1 + client/consensus/common/src/lib.rs | 5 + client/consensus/common/src/shared_data.rs | 236 +++++++++++++++++++++ client/finality-grandpa/src/authorities.rs | 5 +- 6 files changed, 258 insertions(+), 11 deletions(-) create mode 100644 client/consensus/common/src/shared_data.rs diff --git a/Cargo.lock b/Cargo.lock index c869f0c8dfcf3..94ce94fc78b8d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7105,6 +7105,7 @@ dependencies = [ name = "sc-consensus" version = "0.9.0" dependencies = [ + "parking_lot 0.11.1", "sc-client-api", "sp-blockchain", "sp-consensus", diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index db13d0f3e420a..26ddd2af2a3f0 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -448,7 +448,7 @@ pub fn start_babe(BabeParams { let worker = BabeSlotWorker { client: client.clone(), - block_import: Arc::new(Mutex::new(block_import)), + block_import, env, sync_oracle: sync_oracle.clone(), force_authoring, @@ -605,7 +605,7 @@ type SlotNotificationSinks = Arc< struct BabeSlotWorker { client: Arc, - block_import: Arc>, + block_import: I, env: E, sync_oracle: SO, force_authoring: bool, @@ -647,8 +647,8 @@ where "babe" } - fn block_import(&self) -> Arc> { - self.block_import.clone() + fn block_import(&mut self) -> &mut Self::BlockImport { + &mut self.block_import } fn epoch_data( @@ -768,7 +768,7 @@ where import_block.storage_changes = Some(storage_changes); import_block.intermediates.insert( Cow::from(INTERMEDIATE_KEY), - Box::new(BabeIntermediate:: { epoch_descriptor }) as Box, + Box::new(BabeIntermediate:: { epoch_descriptor }) as Box<_>, ); Ok(import_block) @@ -1197,7 +1197,7 @@ where import_block.justifications = justifications; import_block.intermediates.insert( Cow::from(INTERMEDIATE_KEY), - Box::new(BabeIntermediate:: { epoch_descriptor }) as Box, + Box::new(BabeIntermediate:: { epoch_descriptor }) as Box<_>, ); import_block.post_hash = Some(hash); @@ -1275,6 +1275,7 @@ impl BabeBlockImport { } } +#[async_trait::async_trait] impl BlockImport for BabeBlockImport where Block: BlockT, Inner: BlockImport> + Send + Sync, @@ -1286,7 +1287,7 @@ impl BlockImport for BabeBlockImport; - fn import_block( + async fn import_block( &mut self, mut block: BlockImportParams, new_cache: HashMap>, @@ -1506,7 +1507,7 @@ impl BlockImport for BabeBlockImport BlockImport for BabeBlockImport, ) -> Result { - self.inner.check_block(block).map_err(Into::into) + self.inner.check_block(block).await.map_err(Into::into) } } diff --git a/client/consensus/common/Cargo.toml b/client/consensus/common/Cargo.toml index 41c42866e7272..5762b9c998b67 100644 --- a/client/consensus/common/Cargo.toml +++ b/client/consensus/common/Cargo.toml @@ -17,3 +17,4 @@ sc-client-api = { version = "3.0.0", path = "../../api" } sp-blockchain = { version = "3.0.0", path = "../../../primitives/blockchain" } sp-runtime = { version = "3.0.0", path = "../../../primitives/runtime" } sp-consensus = { version = "0.9.0", path = "../../../primitives/consensus/common" } +parking_lot = "0.11.1" diff --git a/client/consensus/common/src/lib.rs b/client/consensus/common/src/lib.rs index a53517c5c35ea..7e3f28b5079cd 100644 --- a/client/consensus/common/src/lib.rs +++ b/client/consensus/common/src/lib.rs @@ -17,6 +17,11 @@ // along with this program. If not, see . //! Collection of common consensus specific implementations + +use std::sync::Arc; +use parking_lot::{Mutex, MappedMutexGuard, Condvar, MutexGuard}; + mod longest_chain; +pub mod shared_data; pub use longest_chain::LongestChain; diff --git a/client/consensus/common/src/shared_data.rs b/client/consensus/common/src/shared_data.rs new file mode 100644 index 0000000000000..1070bed7570a8 --- /dev/null +++ b/client/consensus/common/src/shared_data.rs @@ -0,0 +1,236 @@ +// This file is part of Substrate. + +// Copyright (C) 2021 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program 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. + +// This program 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 this program. If not, see . + +//! Provides the a generic wrapper around shared data. See [`SharedData`] for more information. + +use std::sync::Arc; +use parking_lot::{Mutex, MappedMutexGuard, Condvar, MutexGuard}; + +/// Created by [`SharedDataLocked::release_mutex`]. +/// +/// As long as the object isn't dropped, the shared data is locked. Is it advised to drop this +/// object when the shared data doesn't need to be locked anymore. To get access to the shared data +/// [`Self::upgrade`] is provided. +#[must_use = "Shared data will be unlocked on drop!"] +pub struct SharedDataLockedUpgradable { + shared_data: SharedData, +} + +impl SharedDataLockedUpgradable { + /// Upgrade to a *real* mutex guard that will give access to the inner data. + /// + /// Every call to this function will reaquire the mutex again. + pub fn upgrade(&mut self) -> MappedMutexGuard { + MutexGuard::map(self.shared_data.inner.lock(), |i| &mut i.shared_data) + } +} + +impl Drop for SharedDataLockedUpgradable { + fn drop(&mut self) { + let mut inner = self.shared_data.inner.lock(); + // It should not be locked anymore + inner.locked = false; + + // Notify all waiting threads. + self.shared_data.cond_var.notify_all(); + } +} + +/// Created by [`SharedData::shared_data_locked`]. +/// +/// As long as this object isn't dropped, the shared data is hold in a mutex guard and the shared +/// data is tagged as locked. Access to the shared data is provided through [`Deref`] and +/// [`DerefMut`]. The trick is to use [`Self::release_mutex`] to release the mutex, but still keep +/// the shared data locked. This means every other thread trying to access the shared data in this +/// time will need to wait until this lock is freed. +/// +/// If this object is dropped without calling [`Self::release_mutex`], the lock will be dropped +/// immediatley. +#[must_use = "Shared data will be unlocked on drop!"] +pub struct SharedDataLocked<'a, T> { + /// The current active mutex guard holding the inner data. + inner: MutexGuard<'a, SharedDataInner>, + /// The [`SharedData`] instance that created this instance. + /// + /// This instance is only taken on drop or when calling [`Self::release_mutex`]. + shared_data: Option>, +} + +impl<'a, T> SharedDataLocked<'a, T> { + /// Release the mutex, but keep the shared data locked. + pub fn release_mutex(mut self) -> SharedDataLockedUpgradable { + SharedDataLockedUpgradable { + shared_data: self.shared_data.take() + .expect("`shared_data` is only taken on drop; qed"), + } + } +} + +impl<'a, T> Drop for SharedDataLocked<'a, T> { + fn drop(&mut self) { + if let Some(shared_data) = self.shared_data.take() { + // If the `shared_data` is still set, it means [`Self::release_mutex`] wasn't + // called and the lock should be released. + self.inner.locked = false; + + // Notify all waiting threads about the released lock. + shared_data.cond_var.notify_all(); + } + } +} + +impl<'a, T> std::ops::Deref for SharedDataLocked<'a, T> { + type Target = T; + + fn deref(&self) -> &Self::Target { + &self.inner.shared_data + } +} + +impl<'a, T> std::ops::DerefMut for SharedDataLocked<'a, T> { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.inner.shared_data + } +} + +/// Holds the shared data and if the shared data is currently locked. +/// +/// For more information see [`SharedData`]. +struct SharedDataInner { + /// The actual shared data that is protected here against concurrent access. + shared_data: T, + /// Is `shared_data` currently locked and can not be accessed? + locked: bool, +} + +/// Some shared data that provides support for locking this shared data for some time. +/// +/// When working with consensus engines there is often data that needs to be shared between multiple +/// parts of the system, like block production and block import. This struct provides an abstraction +/// for this shared data in a generic way. +/// +/// The pain point when sharing this data is often the usage of mutex guards in an async context as +/// this doesn't work for most of them as these guards don't implement `Send`. This abstraction +/// provides a way to lock the shared data, while not having the mutex locked. So, the data stays +/// locked and we are still able to hold this lock over an `await` call. +/// +/// # Example +/// +/// ``` +///# use sc_consensus::SharedData; +/// +/// let shared_data = SharedData::new(String::from("hello world")); +/// +/// let lock = shared_data.shared_data_locked(); +/// +/// let shared_data2 = shared_data.clone(); +/// let join_handle1 = std::thread::spawn(move || { +/// // This will need to wait for the outer lock to be released before it can access the data. +/// shared_data2.shared_data().push_str("1"); +/// }); +/// +/// assert_eq!(*lock, "hello world"); +/// +/// // Let us release the mutex, but we still keep it locked. +/// // Now we could call `await` for example. +/// let mut lock = lock.release_mutex(); +/// +/// let shared_data2 = shared_data.clone(); +/// let join_handle2 = std::thread::spawn(move || { +/// shared_data2.shared_data().push_str("2"); +/// }); +/// +/// // We still have the lock and can upgrade it to access the data. +/// assert_eq!(*lock.upgrade(), "hello world"); +/// lock.upgrade().push_str("3"); +/// +/// drop(lock); +/// join_handle1.join().unwrap(); +/// join_handle2.join().unwrap(); +/// +/// let data = shared_data.shared_data(); +/// // As we don't know the order of the threads, we need to check for both combinations +/// assert!(*data == "hello world321" || *data == "hello world312"); +/// ``` +pub struct SharedData { + inner: Arc>>, + cond_var: Arc, +} + +impl Clone for SharedData { + fn clone(&self) -> Self { + Self { + inner: self.inner.clone(), + cond_var: self.cond_var.clone(), + } + } +} + +impl SharedData { + /// Create a new instance of [`SharedData`] to share the given `shared_data`. + pub fn new(shared_data: T) -> Self { + Self { + inner: Arc::new(Mutex::new(SharedDataInner { shared_data, locked: false })), + cond_var: Default::default(), + } + } + + /// Acquire access to the shared data. + /// + /// This will give mutable access to the shared data. After the returned mutex guard is dropped, + /// the shared data is accessable by other threads. So, this function should be used when + /// reading/writing of the shared data in a local context is required. + /// + /// When requiring to lock shared data for some longer time, even with temporarily releasing the + /// lock, [`Self::shared_data_locked`] should be used. + pub fn shared_data(&self) -> MappedMutexGuard { + let mut guard = self.inner.lock(); + + if guard.locked { + self.cond_var.wait(&mut guard); + } + + debug_assert!(!guard.locked); + + MutexGuard::map(guard, |i| &mut i.shared_data) + } + + /// Acquire access to the shared data and lock it. + /// + /// This will give mutable access to the shared data. The returned [`SharedDataLocked`] + /// provides the function [`SharedDataLocked::release_mutex`] to release the mutex, but + /// keeping the data locked. This is useful in async contexts for example where the data needs to + /// be locked, but a mutex guard can not be held. + /// + /// For an example see [`SharedData`]. + pub fn shared_data_locked(&self) -> SharedDataLocked { + let mut guard = self.inner.lock(); + + if guard.locked { + self.cond_var.wait(&mut guard); + } + + debug_assert!(!guard.locked); + guard.locked = true; + + SharedDataLocked { + inner: guard, + shared_data: Some(self.clone()), + } + } +} diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 35e04267305e1..f2fe75f400f2d 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -150,7 +150,6 @@ impl SharedAuthoritySet { } debug_assert!(!guard.locked); - guard.locked = true; MutexGuard::map(guard, |i| &mut i.authority_set) } @@ -161,6 +160,10 @@ impl SharedAuthoritySet { if guard.locked { self.cond_var.wait(&mut guard); } + + debug_assert!(!guard.locked); + guard.locked = true; + InnerLocked { inner: guard, shared_authority_set: Some(self.clone()), From 750975426a9a1c08109e4f355c2c803bb25a8dd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 25 Mar 2021 11:17:21 +0100 Subject: [PATCH 03/27] Add test and fix bugs --- client/consensus/common/src/lib.rs | 3 -- client/consensus/common/src/shared_data.rs | 41 ++++++++++++++++++++-- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/client/consensus/common/src/lib.rs b/client/consensus/common/src/lib.rs index 7e3f28b5079cd..9b4d705769196 100644 --- a/client/consensus/common/src/lib.rs +++ b/client/consensus/common/src/lib.rs @@ -18,9 +18,6 @@ //! Collection of common consensus specific implementations -use std::sync::Arc; -use parking_lot::{Mutex, MappedMutexGuard, Condvar, MutexGuard}; - mod longest_chain; pub mod shared_data; diff --git a/client/consensus/common/src/shared_data.rs b/client/consensus/common/src/shared_data.rs index 1070bed7570a8..4a09133852d2a 100644 --- a/client/consensus/common/src/shared_data.rs +++ b/client/consensus/common/src/shared_data.rs @@ -132,7 +132,7 @@ struct SharedDataInner { /// # Example /// /// ``` -///# use sc_consensus::SharedData; +///# use sc_consensus::shared_data::SharedData; /// /// let shared_data = SharedData::new(String::from("hello world")); /// @@ -201,7 +201,7 @@ impl SharedData { pub fn shared_data(&self) -> MappedMutexGuard { let mut guard = self.inner.lock(); - if guard.locked { + while guard.locked { self.cond_var.wait(&mut guard); } @@ -221,7 +221,7 @@ impl SharedData { pub fn shared_data_locked(&self) -> SharedDataLocked { let mut guard = self.inner.lock(); - if guard.locked { + while guard.locked { self.cond_var.wait(&mut guard); } @@ -234,3 +234,38 @@ impl SharedData { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn shared_data_locking_works() { + const THREADS: u32 = 100; + let shared_data = SharedData::new(0u32); + + let lock = shared_data.shared_data_locked(); + + for i in 0..THREADS { + let data = shared_data.clone(); + std::thread::spawn(move || { + if i % 2 == 1 { + *data.shared_data() += 1; + } else { + let mut lock = data.shared_data_locked().release_mutex(); + // Give the other threads some time to wake up + std::thread::sleep(std::time::Duration::from_millis(10)); + *lock.upgrade() += 1; + } + }); + } + + let lock = lock.release_mutex(); + std::thread::sleep(std::time::Duration::from_millis(100)); + drop(lock); + + while *shared_data.shared_data() < THREADS { + std::thread::sleep(std::time::Duration::from_millis(100)); + } + } +} From 5048c15e3ac5a9ec7986953f97bf5a57ac767834 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 25 Mar 2021 12:07:24 +0100 Subject: [PATCH 04/27] Switch to `SharedData` --- client/finality-grandpa/src/authorities.rs | 101 +++------------------ client/finality-grandpa/src/import.rs | 14 +-- 2 files changed, 21 insertions(+), 94 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index f2fe75f400f2d..056460ac9ed80 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -19,17 +19,17 @@ //! Utilities for dealing with authorities, authority sets, and handoffs. use fork_tree::ForkTree; -use parking_lot::{Mutex, MappedMutexGuard, Condvar, MutexGuard}; +use parking_lot::MappedMutexGuard; use finality_grandpa::voter_set::VoterSet; use parity_scale_codec::{Encode, Decode}; use log::debug; use sc_telemetry::{telemetry, TelemetryHandle, CONSENSUS_INFO}; use sp_finality_grandpa::{AuthorityId, AuthorityList}; +use sc_consensus::shared_data::{SharedData, SharedDataLocked}; use std::cmp::Ord; use std::fmt::Debug; use std::ops::Add; -use std::sync::Arc; /// Error type returned on operations on the `AuthoritySet`. #[derive(Debug, derive_more::Display)] @@ -68,106 +68,32 @@ impl From for Error { } } -struct Inner { - authority_set: AuthoritySet, - locked: bool, -} - -pub(crate) struct InnerLocked2 { - shared_authority_set: SharedAuthoritySet, -} - -impl InnerLocked2 { - pub fn upgrade(&mut self) -> MappedMutexGuard> { - MutexGuard::map(self.shared_authority_set.inner.lock(), |i| &mut i.authority_set) - } -} - -impl Drop for InnerLocked2 { - fn drop(&mut self) { - let mut inner = self.shared_authority_set.inner.lock(); - inner.locked = false; - self.shared_authority_set.cond_var.notify_all(); - } -} - -pub(crate) struct InnerLocked<'a, H, N> { - inner: MutexGuard<'a, Inner>, - shared_authority_set: Option>, -} - -impl<'a, H, N> InnerLocked<'a, H, N> { - pub fn release_lock(mut self) -> InnerLocked2 { - InnerLocked2 { shared_authority_set: self.shared_authority_set.take().unwrap() } - } -} - -impl<'a, H, N> Drop for InnerLocked<'a, H, N> { - fn drop(&mut self) { - if let Some(authority_set) = self.shared_authority_set.take() { - self.inner.locked = false; - authority_set.cond_var.notify_all(); - } - } -} - -impl<'a, H, N> std::ops::Deref for InnerLocked<'a, H, N> { - type Target = AuthoritySet; - - fn deref(&self) -> &Self::Target { - &self.inner.authority_set - } -} - -impl<'a, H, N> std::ops::DerefMut for InnerLocked<'a, H, N> { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.inner.authority_set - } -} - /// A shared authority set. pub struct SharedAuthoritySet { - inner: Arc>>, - cond_var: Arc, + inner: SharedData>, } impl Clone for SharedAuthoritySet { fn clone(&self) -> Self { SharedAuthoritySet { inner: self.inner.clone(), - cond_var: self.cond_var.clone(), } } } impl SharedAuthoritySet { - /// Acquire a reference to the inner read-write lock. + /// Returns access to the [`AuthoritySet`]. pub(crate) fn inner(&self) -> MappedMutexGuard> { - let mut guard = self.inner.lock(); - - if guard.locked { - self.cond_var.wait(&mut guard); - } - - debug_assert!(!guard.locked); - - MutexGuard::map(guard, |i| &mut i.authority_set) + self.inner.shared_data() } - pub(crate) fn inner_locked(&self) -> InnerLocked { - let mut guard = self.inner.lock(); - - if guard.locked { - self.cond_var.wait(&mut guard); - } - - debug_assert!(!guard.locked); - guard.locked = true; - - InnerLocked { - inner: guard, - shared_authority_set: Some(self.clone()), - } + /// Returns access to the [`AuthoritySet`] and locks it. + /// + /// For more information see [`SharedDataLocked`]. + pub(crate) fn inner_locked( + &self, + ) -> SharedDataLocked> { + self.inner.shared_data_locked() } } @@ -209,8 +135,7 @@ where N: Add + Ord + Clone + Debug, impl From> for SharedAuthoritySet { fn from(set: AuthoritySet) -> Self { SharedAuthoritySet { - inner: Arc::new(Mutex::new(Inner { authority_set: set, locked: false })), - cond_var: Default::default(), + inner: SharedData::new(set), } } } diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index d02d99569d4f3..9ceb3791e4078 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -20,13 +20,13 @@ use std::{sync::Arc, collections::HashMap}; use log::debug; use parity_scale_codec::Encode; -use parking_lot::RwLockWriteGuard; use sp_blockchain::{BlockStatus, well_known_cache_keys}; use sc_client_api::{backend::Backend, utils::is_descendent_of}; use sc_telemetry::TelemetryHandle; use sp_utils::mpsc::TracingUnboundedSender; use sp_api::TransactionFor; +use sc_consensus::shared_data::{SharedDataLockedUpgradable, SharedDataLocked}; use sp_consensus::{ BlockImport, Error as ConsensusError, @@ -160,7 +160,7 @@ impl AppliedChanges { struct PendingSetChanges { just_in_case: Option<( AuthoritySet>, - crate::authorities::InnerLocked2>, + SharedDataLockedUpgradable>>, )>, applied_changes: AppliedChanges>, do_pause: bool, @@ -168,7 +168,7 @@ struct PendingSetChanges { impl PendingSetChanges { // revert the pending set change explicitly. - fn revert(self) { } + fn revert(self) {} fn defuse(mut self) -> (AppliedChanges>, bool) { self.just_in_case = None; @@ -271,7 +271,7 @@ where // the old authority set on error or panic. struct InnerGuard<'a, H, N> { old: Option>, - guard: Option>, + guard: Option>>, } impl<'a, H, N> InnerGuard<'a, H, N> { @@ -286,7 +286,9 @@ where } } - fn consume(mut self) -> Option<(AuthoritySet, crate::authorities::InnerLocked<'a, H, N>)> { + fn consume( + mut self, + ) -> Option<(AuthoritySet, SharedDataLocked<'a, AuthoritySet>)> { if let Some(old) = self.old.take() { Some(( old, @@ -416,7 +418,7 @@ where ); } - let just_in_case = just_in_case.map(|(o, i)| (o, i.release_lock())); + let just_in_case = just_in_case.map(|(o, i)| (o, i.release_mutex())); Ok(PendingSetChanges { just_in_case, applied_changes, do_pause }) } From ee26e09b4f44a20c987df4f34a5596398de71880 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 25 Mar 2021 20:55:00 +0100 Subject: [PATCH 05/27] Make grandpa tests working --- Cargo.lock | 4 + client/consensus/babe/src/aux_schema.rs | 11 +- client/consensus/babe/src/lib.rs | 310 ++++++++++--------- client/consensus/epochs/Cargo.toml | 1 + client/consensus/epochs/src/lib.rs | 6 +- client/finality-grandpa/src/aux_schema.rs | 10 +- client/finality-grandpa/src/tests.rs | 66 ++-- client/finality-grandpa/src/voting_rule.rs | 2 +- client/network/test/Cargo.toml | 1 + client/network/test/src/lib.rs | 256 ++++++++------- client/service/Cargo.toml | 1 + client/service/src/client/client.rs | 16 +- test-utils/client/Cargo.toml | 1 + test-utils/client/src/client_ext.rs | 77 +++-- test-utils/runtime/client/src/trait_tests.rs | 61 ++-- 15 files changed, 453 insertions(+), 370 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 94ce94fc78b8d..ebdd76da2bf0e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7245,6 +7245,7 @@ dependencies = [ "parity-scale-codec 2.0.1", "parking_lot 0.11.1", "sc-client-api", + "sc-consensus", "sp-blockchain", "sp-runtime", ] @@ -7680,6 +7681,7 @@ name = "sc-network-test" version = "0.8.0" dependencies = [ "async-std", + "async-trait", "futures 0.3.13", "futures-timer 3.0.2", "libp2p", @@ -7860,6 +7862,7 @@ name = "sc-service" version = "0.9.0" dependencies = [ "async-std", + "async-trait", "directories", "exit-future", "futures 0.1.31", @@ -9484,6 +9487,7 @@ dependencies = [ name = "substrate-test-client" version = "2.0.1" dependencies = [ + "async-trait", "futures 0.1.31", "futures 0.3.13", "hash-db", diff --git a/client/consensus/babe/src/aux_schema.rs b/client/consensus/babe/src/aux_schema.rs index 7d5df77c92176..b753535c6b7aa 100644 --- a/client/consensus/babe/src/aux_schema.rs +++ b/client/consensus/babe/src/aux_schema.rs @@ -79,18 +79,19 @@ pub fn load_epoch_changes( }, }; - let epoch_changes = Arc::new(Mutex::new(maybe_epoch_changes.unwrap_or_else(|| { - info!(target: "babe", - "👶 Creating empty BABE epoch changes on what appears to be first startup." + let epoch_changes = SharedEpochChanges::::new(maybe_epoch_changes.unwrap_or_else(|| { + info!( + target: "babe", + "👶 Creating empty BABE epoch changes on what appears to be first startup.", ); EpochChangesFor::::default() - }))); + })); // rebalance the tree after deserialization. this isn't strictly necessary // since the tree is now rebalanced on every update operation. but since the // tree wasn't rebalanced initially it's useful to temporarily leave it here // to avoid having to wait until an import for rebalancing. - epoch_changes.lock().rebalance(); + epoch_changes.shared_data().rebalance(); Ok(epoch_changes) } diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 26ddd2af2a3f0..e5910f5b35e61 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -502,7 +502,7 @@ async fn answer_requests( match request { BabeRequest::EpochForChild(parent_hash, parent_number, slot_number, response) => { let lookup = || { - let epoch_changes = epoch_changes.lock(); + let epoch_changes = epoch_changes.shared_data(); let epoch_descriptor = epoch_changes.epoch_descriptor_for_child_of( descendent_query(&*client), &parent_hash, @@ -656,7 +656,7 @@ where parent: &B::Header, slot: Slot, ) -> Result { - self.epoch_changes.lock().epoch_descriptor_for_child_of( + self.epoch_changes.shared_data().epoch_descriptor_for_child_of( descendent_query(&*self.client), &parent.hash(), parent.number().clone(), @@ -667,7 +667,8 @@ where } fn authorities_len(&self, epoch_descriptor: &Self::EpochData) -> Option { - self.epoch_changes.lock() + self.epoch_changes + .shared_data() .viable_epoch(&epoch_descriptor, |slot| Epoch::genesis(&self.config, slot)) .map(|epoch| epoch.as_ref().authorities.len()) } @@ -681,7 +682,7 @@ where debug!(target: "babe", "Attempting to claim slot {}", slot); let s = authorship::claim_slot( slot, - self.epoch_changes.lock().viable_epoch( + self.epoch_changes.shared_data().viable_epoch( &epoch_descriptor, |slot| Epoch::genesis(&self.config, slot) )?.as_ref(), @@ -1125,7 +1126,7 @@ where .map_err(Error::::FetchParentHeader)?; let pre_digest = find_pre_digest::(&header)?; - let epoch_changes = self.epoch_changes.lock(); + let epoch_changes = self.epoch_changes.shared_data(); let epoch_descriptor = epoch_changes.epoch_descriptor_for_child_of( descendent_query(&*self.client), &parent_hash, @@ -1329,182 +1330,189 @@ impl BlockImport for BabeBlockImport::ParentBlockNoAssociatedWeight(hash)).into() ))? - }; + }; - let intermediate = block.take_intermediate::>( - INTERMEDIATE_KEY - )?; + let intermediate = block.take_intermediate::>( + INTERMEDIATE_KEY + )?; - let epoch_descriptor = intermediate.epoch_descriptor; - let first_in_epoch = parent_slot < epoch_descriptor.start_slot(); - (epoch_descriptor, first_in_epoch, parent_weight) - }; + let epoch_descriptor = intermediate.epoch_descriptor; + let first_in_epoch = parent_slot < epoch_descriptor.start_slot(); + (epoch_descriptor, first_in_epoch, parent_weight) + }; - let total_weight = parent_weight + pre_digest.added_weight(); - - // search for this all the time so we can reject unexpected announcements. - let next_epoch_digest = find_next_epoch_digest::(&block.header) - .map_err(|e| ConsensusError::ClientImport(e.to_string()))?; - let next_config_digest = find_next_config_digest::(&block.header) - .map_err(|e| ConsensusError::ClientImport(e.to_string()))?; - - match (first_in_epoch, next_epoch_digest.is_some(), next_config_digest.is_some()) { - (true, true, _) => {}, - (false, false, false) => {}, - (false, false, true) => { - return Err( - ConsensusError::ClientImport( - babe_err(Error::::UnexpectedConfigChange).into(), + let total_weight = parent_weight + pre_digest.added_weight(); + + // search for this all the time so we can reject unexpected announcements. + let next_epoch_digest = find_next_epoch_digest::(&block.header) + .map_err(|e| ConsensusError::ClientImport(e.to_string()))?; + let next_config_digest = find_next_config_digest::(&block.header) + .map_err(|e| ConsensusError::ClientImport(e.to_string()))?; + + match (first_in_epoch, next_epoch_digest.is_some(), next_config_digest.is_some()) { + (true, true, _) => {}, + (false, false, false) => {}, + (false, false, true) => { + return Err( + ConsensusError::ClientImport( + babe_err(Error::::UnexpectedConfigChange).into(), + ) ) - ) - }, - (true, false, _) => { - return Err( - ConsensusError::ClientImport( - babe_err(Error::::ExpectedEpochChange(hash, slot)).into(), + }, + (true, false, _) => { + return Err( + ConsensusError::ClientImport( + babe_err(Error::::ExpectedEpochChange(hash, slot)).into(), + ) ) - ) - }, - (false, true, _) => { - return Err( - ConsensusError::ClientImport( - babe_err(Error::::UnexpectedEpochChange).into(), + }, + (false, true, _) => { + return Err( + ConsensusError::ClientImport( + babe_err(Error::::UnexpectedEpochChange).into(), + ) ) - ) - }, - } + }, + } - // if there's a pending epoch we'll save the previous epoch changes here - // this way we can revert it if there's any error - let mut old_epoch_changes = None; + let info = self.client.info(); - let info = self.client.info(); + if let Some(next_epoch_descriptor) = next_epoch_digest { + old_epoch_changes = Some((*epoch_changes).clone()); - if let Some(next_epoch_descriptor) = next_epoch_digest { - old_epoch_changes = Some(epoch_changes.clone()); + let viable_epoch = epoch_changes.viable_epoch( + &epoch_descriptor, + |slot| Epoch::genesis(&self.config, slot) + ).ok_or_else(|| { + ConsensusError::ClientImport(Error::::FetchEpoch(parent_hash).into()) + })?; - let viable_epoch = epoch_changes.viable_epoch( - &epoch_descriptor, - |slot| Epoch::genesis(&self.config, slot) - ).ok_or_else(|| { - ConsensusError::ClientImport(Error::::FetchEpoch(parent_hash).into()) - })?; + let epoch_config = next_config_digest.map(Into::into).unwrap_or_else( + || viable_epoch.as_ref().config.clone() + ); - let epoch_config = next_config_digest.map(Into::into).unwrap_or_else( - || viable_epoch.as_ref().config.clone() - ); + // restrict info logging during initial sync to avoid spam + let log_level = if block.origin == BlockOrigin::NetworkInitialSync { + log::Level::Debug + } else { + log::Level::Info + }; - // restrict info logging during initial sync to avoid spam - let log_level = if block.origin == BlockOrigin::NetworkInitialSync { - log::Level::Debug - } else { - log::Level::Info - }; + log!(target: "babe", + log_level, + "👶 New epoch {} launching at block {} (block slot {} >= start slot {}).", + viable_epoch.as_ref().epoch_index, + hash, + slot, + viable_epoch.as_ref().start_slot, + ); - log!(target: "babe", - log_level, - "👶 New epoch {} launching at block {} (block slot {} >= start slot {}).", - viable_epoch.as_ref().epoch_index, - hash, - slot, - viable_epoch.as_ref().start_slot, - ); + let next_epoch = viable_epoch.increment((next_epoch_descriptor, epoch_config)); - let next_epoch = viable_epoch.increment((next_epoch_descriptor, epoch_config)); + log!(target: "babe", + log_level, + "👶 Next epoch starts at slot {}", + next_epoch.as_ref().start_slot, + ); - log!(target: "babe", - log_level, - "👶 Next epoch starts at slot {}", - next_epoch.as_ref().start_slot, - ); + // prune the tree of epochs not part of the finalized chain or + // that are not live anymore, and then track the given epoch change + // in the tree. + // NOTE: it is important that these operations are done in this + // order, otherwise if pruning after import the `is_descendent_of` + // used by pruning may not know about the block that is being + // imported. + let prune_and_import = || { + prune_finalized( + self.client.clone(), + &mut epoch_changes, + )?; - // prune the tree of epochs not part of the finalized chain or - // that are not live anymore, and then track the given epoch change - // in the tree. - // NOTE: it is important that these operations are done in this - // order, otherwise if pruning after import the `is_descendent_of` - // used by pruning may not know about the block that is being - // imported. - let prune_and_import = || { - prune_finalized( - self.client.clone(), - &mut epoch_changes, - )?; + epoch_changes.import( + descendent_query(&*self.client), + hash, + number, + *block.header.parent_hash(), + next_epoch, + ).map_err(|e| ConsensusError::ClientImport(format!("{:?}", e)))?; - epoch_changes.import( - descendent_query(&*self.client), - hash, - number, - *block.header.parent_hash(), - next_epoch, - ).map_err(|e| ConsensusError::ClientImport(format!("{:?}", e)))?; + Ok(()) + }; - Ok(()) - }; + if let Err(e) = prune_and_import() { + debug!(target: "babe", "Failed to launch next epoch: {:?}", e); + *epoch_changes = old_epoch_changes.expect("set `Some` above and not taken; qed"); + return Err(e); + } - if let Err(e) = prune_and_import() { - debug!(target: "babe", "Failed to launch next epoch: {:?}", e); - *epoch_changes = old_epoch_changes.expect("set `Some` above and not taken; qed"); - return Err(e); + crate::aux_schema::write_epoch_changes::( + &*epoch_changes, + |insert| block.auxiliary.extend( + insert.iter().map(|(k, v)| (k.to_vec(), Some(v.to_vec()))) + ) + ); } - crate::aux_schema::write_epoch_changes::( - &*epoch_changes, - |insert| block.auxiliary.extend( - insert.iter().map(|(k, v)| (k.to_vec(), Some(v.to_vec()))) - ) + aux_schema::write_block_weight( + hash, + total_weight, + |values| block.auxiliary.extend( + values.iter().map(|(k, v)| (k.to_vec(), Some(v.to_vec()))) + ), ); - } - - aux_schema::write_block_weight( - hash, - total_weight, - |values| block.auxiliary.extend( - values.iter().map(|(k, v)| (k.to_vec(), Some(v.to_vec()))) - ), - ); - // The fork choice rule is that we pick the heaviest chain (i.e. - // more primary blocks), if there's a tie we go with the longest - // chain. - block.fork_choice = { - let (last_best, last_best_number) = (info.best_hash, info.best_number); - - let last_best_weight = if &last_best == block.header.parent_hash() { - // the parent=genesis case is already covered for loading parent weight, - // so we don't need to cover again here. - parent_weight - } else { - aux_schema::load_block_weight(&*self.client, last_best) - .map_err(|e| ConsensusError::ChainLookup(format!("{:?}", e)))? + // The fork choice rule is that we pick the heaviest chain (i.e. + // more primary blocks), if there's a tie we go with the longest + // chain. + block.fork_choice = { + let (last_best, last_best_number) = (info.best_hash, info.best_number); + + let last_best_weight = if &last_best == block.header.parent_hash() { + // the parent=genesis case is already covered for loading parent weight, + // so we don't need to cover again here. + parent_weight + } else { + aux_schema::load_block_weight(&*self.client, last_best) + .map_err(|e| ConsensusError::ChainLookup(format!("{:?}", e)))? .ok_or_else( || ConsensusError::ChainLookup("No block weight for parent header.".to_string()) )? + }; + + Some(ForkChoiceStrategy::Custom(if total_weight > last_best_weight { + true + } else if total_weight == last_best_weight { + number > last_best_number + } else { + false + })) }; - Some(ForkChoiceStrategy::Custom(if total_weight > last_best_weight { - true - } else if total_weight == last_best_weight { - number > last_best_number - } else { - false - })) + // Release the mutex, but it stays locked + epoch_changes.release_mutex() }; let import_result = self.inner.import_block(block, new_cache).await; @@ -1513,7 +1521,7 @@ impl BlockImport for BabeBlockImport( // startup rather than waiting until importing the next epoch change block. prune_finalized( client.clone(), - &mut epoch_changes.lock(), + &mut epoch_changes.shared_data(), )?; let import = BabeBlockImport::new( diff --git a/client/consensus/epochs/Cargo.toml b/client/consensus/epochs/Cargo.toml index bebe6979e694e..4024b7848f9d4 100644 --- a/client/consensus/epochs/Cargo.toml +++ b/client/consensus/epochs/Cargo.toml @@ -19,3 +19,4 @@ fork-tree = { version = "3.0.0", path = "../../../utils/fork-tree" } sp-runtime = { path = "../../../primitives/runtime" , version = "3.0.0"} sp-blockchain = { version = "3.0.0", path = "../../../primitives/blockchain" } sc-client-api = { path = "../../api" , version = "3.0.0"} +sc-consensus = { path = "../common" , version = "0.9.0"} diff --git a/client/consensus/epochs/src/lib.rs b/client/consensus/epochs/src/lib.rs index 5c5ef446993a2..d0c6cde3711e1 100644 --- a/client/consensus/epochs/src/lib.rs +++ b/client/consensus/epochs/src/lib.rs @@ -645,10 +645,12 @@ impl EpochChanges where } /// Type alias to produce the epoch-changes tree from a block type. -pub type EpochChangesFor = EpochChanges<::Hash, NumberFor, Epoch>; +pub type EpochChangesFor = + EpochChanges<::Hash, NumberFor, Epoch>; /// A shared epoch changes tree. -pub type SharedEpochChanges = Arc>>; +pub type SharedEpochChanges = + sc_consensus::shared_data::SharedData>; #[cfg(test)] mod tests { diff --git a/client/finality-grandpa/src/aux_schema.rs b/client/finality-grandpa/src/aux_schema.rs index 43c45b9f10ae1..8ecfae40f68c7 100644 --- a/client/finality-grandpa/src/aux_schema.rs +++ b/client/finality-grandpa/src/aux_schema.rs @@ -592,7 +592,7 @@ mod test { ).unwrap(); assert_eq!( - *authority_set.inner().read(), + *authority_set.inner(), AuthoritySet::new( authorities.clone(), set_id, @@ -616,7 +616,7 @@ mod test { votes: vec![], }, set_id, - &*authority_set.inner().read(), + &*authority_set.inner(), ), current_rounds, }, @@ -688,7 +688,7 @@ mod test { ).unwrap(); assert_eq!( - *authority_set.inner().read(), + *authority_set.inner(), AuthoritySet::new( authorities.clone(), set_id, @@ -712,7 +712,7 @@ mod test { votes: vec![], }, set_id, - &*authority_set.inner().read(), + &*authority_set.inner(), ), current_rounds, }, @@ -781,7 +781,7 @@ mod test { ).unwrap(); assert_eq!( - *authority_set.inner().read(), + *authority_set.inner(), AuthoritySet::new( authorities.clone(), set_id, diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index d0a6b0874fa77..026a62939de09 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -28,6 +28,7 @@ use sc_network_test::{ use sc_network::config::ProtocolConfig; use parking_lot::{RwLock, Mutex}; use futures_timer::Delay; +use futures::executor::block_on; use tokio::runtime::{Runtime, Handle}; use sp_keyring::Ed25519Keyring; use sc_client_api::backend::TransactionFor; @@ -43,7 +44,9 @@ use sp_runtime::{Justifications, traits::{Block as BlockT, Header as HeaderT}}; use sp_runtime::generic::{BlockId, DigestItem}; use sp_core::H256; use sp_keystore::{SyncCryptoStorePtr, SyncCryptoStore}; -use sp_finality_grandpa::{GRANDPA_ENGINE_ID, AuthorityList, EquivocationProof, GrandpaApi, OpaqueKeyOwnershipProof}; +use sp_finality_grandpa::{ + GRANDPA_ENGINE_ID, AuthorityList, EquivocationProof, GrandpaApi, OpaqueKeyOwnershipProof, +}; use authorities::AuthoritySet; use sc_block_builder::BlockBuilderProvider; @@ -54,7 +57,13 @@ use sp_application_crypto::key_types::GRANDPA; type TestLinkHalf = LinkHalf>; type PeerData = Mutex>; -type GrandpaPeer = Peer; +type GrandpaPeer = Peer; +type GrandpaBlockImport = crate::GrandpaBlockImport< + substrate_test_runtime_client::Backend, + Block, + PeersFullClient, + LongestChain +>; struct GrandpaTestNet { peers: Vec, @@ -93,6 +102,7 @@ impl GrandpaTestNet { impl TestNetFactory for GrandpaTestNet { type Verifier = PassThroughVerifier; type PeerData = PeerData; + type BlockImport = GrandpaBlockImport; /// Create new test network with peers and given config. fn from_config(_config: &ProtocolConfig) -> Self { @@ -124,9 +134,9 @@ impl TestNetFactory for GrandpaTestNet { PassThroughVerifier::new(false) // use non-instant finality. } - fn make_block_import(&self, client: PeersClient) + fn make_block_import(&self, client: PeersClient) -> ( - BlockImportAdapter, + BlockImportAdapter, Option>, PeerData, ) @@ -141,7 +151,7 @@ impl TestNetFactory for GrandpaTestNet { ).expect("Could not create block import for fresh peer."); let justification_import = Box::new(import.clone()); ( - BlockImportAdapter::new_full(import), + BlockImportAdapter::new(import), Some(justification_import), Mutex::new(Some(link)), ) @@ -820,11 +830,7 @@ fn allows_reimporting_change_blocks() { let mut net = GrandpaTestNet::new(api.clone(), 3, 0); let client = net.peer(0).client().clone(); - let (mut block_import, ..) = net.make_block_import::< - TransactionFor - >( - client.clone(), - ); + let (mut block_import, ..) = net.make_block_import(client.clone()); let full_client = client.as_full().unwrap(); let builder = full_client.new_block_at(&BlockId::Number(0), Default::default(), false).unwrap(); @@ -844,7 +850,7 @@ fn allows_reimporting_change_blocks() { }; assert_eq!( - block_import.import_block(block(), HashMap::new()).unwrap(), + block_on(block_import.import_block(block(), HashMap::new())).unwrap(), ImportResult::Imported(ImportedAux { needs_justification: true, clear_justification_requests: false, @@ -855,7 +861,7 @@ fn allows_reimporting_change_blocks() { ); assert_eq!( - block_import.import_block(block(), HashMap::new()).unwrap(), + block_on(block_import.import_block(block(), HashMap::new())).unwrap(), ImportResult::AlreadyInChain ); } @@ -869,11 +875,7 @@ fn test_bad_justification() { let mut net = GrandpaTestNet::new(api.clone(), 3, 0); let client = net.peer(0).client().clone(); - let (mut block_import, ..) = net.make_block_import::< - TransactionFor - >( - client.clone(), - ); + let (mut block_import, ..) = net.make_block_import(client.clone()); let full_client = client.as_full().expect("only full clients are used in test"); let builder = full_client.new_block_at(&BlockId::Number(0), Default::default(), false).unwrap(); @@ -895,7 +897,7 @@ fn test_bad_justification() { }; assert_eq!( - block_import.import_block(block(), HashMap::new()).unwrap(), + block_on(block_import.import_block(block(), HashMap::new())).unwrap(), ImportResult::Imported(ImportedAux { needs_justification: true, clear_justification_requests: false, @@ -906,7 +908,7 @@ fn test_bad_justification() { ); assert_eq!( - block_import.import_block(block(), HashMap::new()).unwrap(), + block_on(block_import.import_block(block(), HashMap::new())).unwrap(), ImportResult::AlreadyInChain ); } @@ -950,9 +952,7 @@ fn voter_persists_its_votes() { let set_state = { let bob_client = net.peer(1).client().clone(); let (_, _, link) = net - .make_block_import::< - TransactionFor - >(bob_client); + .make_block_import(bob_client); let LinkHalf { persistent_data, .. } = link.lock().take().unwrap(); let PersistentData { set_state, .. } = persistent_data; set_state @@ -1019,9 +1019,7 @@ fn voter_persists_its_votes() { let alice_client = net.peer(0).client().clone(); let (_block_import, _, link) = net - .make_block_import::< - TransactionFor - >(alice_client); + .make_block_import(alice_client); let link = link.lock().take().unwrap(); let grandpa_params = GrandpaParams { @@ -1422,7 +1420,7 @@ fn grandpa_environment_respects_voting_rules() { // the unrestricted environment should just return the best block assert_eq!( - futures::executor::block_on(unrestricted_env.best_chain_containing( + block_on(unrestricted_env.best_chain_containing( peer.client().info().finalized_hash )).unwrap().unwrap().1, 21, @@ -1431,14 +1429,14 @@ fn grandpa_environment_respects_voting_rules() { // both the other environments should return block 16, which is 3/4 of the // way in the unfinalized chain assert_eq!( - futures::executor::block_on(three_quarters_env.best_chain_containing( + block_on(three_quarters_env.best_chain_containing( peer.client().info().finalized_hash )).unwrap().unwrap().1, 16, ); assert_eq!( - futures::executor::block_on(default_env.best_chain_containing( + block_on(default_env.best_chain_containing( peer.client().info().finalized_hash )).unwrap().unwrap().1, 16, @@ -1449,7 +1447,7 @@ fn grandpa_environment_respects_voting_rules() { // the 3/4 environment should propose block 21 for voting assert_eq!( - futures::executor::block_on(three_quarters_env.best_chain_containing( + block_on(three_quarters_env.best_chain_containing( peer.client().info().finalized_hash )).unwrap().unwrap().1, 21, @@ -1458,7 +1456,7 @@ fn grandpa_environment_respects_voting_rules() { // while the default environment will always still make sure we don't vote // on the best block (2 behind) assert_eq!( - futures::executor::block_on(default_env.best_chain_containing( + block_on(default_env.best_chain_containing( peer.client().info().finalized_hash )).unwrap().unwrap().1, 19, @@ -1471,7 +1469,7 @@ fn grandpa_environment_respects_voting_rules() { // best block, there's a hard rule that we can't cast any votes lower than // the given base (#21). assert_eq!( - futures::executor::block_on(default_env.best_chain_containing( + block_on(default_env.best_chain_containing( peer.client().info().finalized_hash )).unwrap().unwrap().1, 21, @@ -1557,9 +1555,7 @@ fn imports_justification_for_regular_blocks_on_import() { let mut net = GrandpaTestNet::new(api.clone(), 1, 0); let client = net.peer(0).client().clone(); - let (mut block_import, ..) = net.make_block_import::< - TransactionFor - >(client.clone()); + let (mut block_import, ..) = net.make_block_import(client.clone()); let full_client = client.as_full().expect("only full clients are used in test"); let builder = full_client.new_block_at(&BlockId::Number(0), Default::default(), false).unwrap(); @@ -1607,7 +1603,7 @@ fn imports_justification_for_regular_blocks_on_import() { import.fork_choice = Some(ForkChoiceStrategy::LongestChain); assert_eq!( - block_import.import_block(import, HashMap::new()).unwrap(), + block_on(block_import.import_block(import, HashMap::new())).unwrap(), ImportResult::Imported(ImportedAux { needs_justification: false, clear_justification_requests: false, diff --git a/client/finality-grandpa/src/voting_rule.rs b/client/finality-grandpa/src/voting_rule.rs index 9b3fb9b328560..3ede7649a1387 100644 --- a/client/finality-grandpa/src/voting_rule.rs +++ b/client/finality-grandpa/src/voting_rule.rs @@ -372,7 +372,7 @@ mod tests { .unwrap() .block; - client.import(BlockOrigin::Own, block).unwrap(); + futures::executor::block_on(client.import(BlockOrigin::Own, block)).unwrap(); } let genesis = client diff --git a/client/network/test/Cargo.toml b/client/network/test/Cargo.toml index 7ba468fa3f78f..4fc1aa740040d 100644 --- a/client/network/test/Cargo.toml +++ b/client/network/test/Cargo.toml @@ -34,3 +34,4 @@ substrate-test-runtime = { version = "2.0.0", path = "../../../test-utils/runtim tempfile = "3.1.0" sp-tracing = { version = "3.0.0", path = "../../../primitives/tracing" } sc-service = { version = "0.9.0", default-features = false, features = ["test-helpers"], path = "../../service" } +async-trait = "0.1.42" diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index 32a6e07eab428..00e13a2646180 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -104,8 +104,9 @@ impl PassThroughVerifier { } /// This `Verifier` accepts all data as valid. +#[async_trait::async_trait] impl Verifier for PassThroughVerifier { - fn verify( + async fn verify( &mut self, origin: BlockOrigin, header: B::Header, @@ -154,13 +155,8 @@ impl PeersClient { } } - pub fn as_block_import(&self) -> BlockImportAdapter { - match *self { - PeersClient::Full(ref client, ref _backend) => - BlockImportAdapter::new_full(client.clone()), - PeersClient::Light(ref client, ref _backend) => - BlockImportAdapter::Light(Arc::new(Mutex::new(client.clone())), PhantomData), - } + pub fn as_block_import(&self) -> BlockImportAdapter { + BlockImportAdapter::new(self.clone()) } pub fn get_aux(&self, key: &[u8]) -> ClientResult>> { @@ -218,15 +214,44 @@ impl PeersClient { } } -pub struct Peer { +#[async_trait::async_trait] +impl BlockImport for PeersClient { + type Error = ConsensusError; + type Transaction = (); + + async fn check_block( + &mut self, + block: BlockCheckParams, + ) -> Result { + match self { + PeersClient::Full(client, _) => client.check_block(block).await, + PeersClient::Light(client, _) => client.check_block(block).await, + } + } + + async fn import_block( + &mut self, + block: BlockImportParams, + cache: HashMap>, + ) -> Result { + match self { + PeersClient::Full(client, _) => + client.import_block(block.convert_transaction(), cache).await, + PeersClient::Light(client, _) => + client.import_block(block.convert_transaction(), cache).await, + } + } +} + +pub struct Peer { pub data: D, client: PeersClient, /// We keep a copy of the verifier so that we can invoke it for locally-generated blocks, /// instead of going through the import queue. - verifier: VerifierAdapter, + verifier: VerifierAdapter, /// We keep a copy of the block_import so that we can invoke it for locally-generated blocks, /// instead of going through the import queue. - block_import: BlockImportAdapter<()>, + block_import: BlockImportAdapter, select_chain: Option>, backend: Option>, network: NetworkWorker::Hash>, @@ -235,7 +260,11 @@ pub struct Peer { listen_addr: Multiaddr, } -impl Peer { +impl Peer where + V: Verifier, + B: BlockImport + Send + Sync, + B::Transaction: Send, +{ /// Get this peer ID. pub fn id(&self) -> PeerId { self.network.service().local_peer_id().clone() @@ -277,13 +306,24 @@ impl Peer { } /// Request explicit fork sync. - pub fn set_sync_fork_request(&self, peers: Vec, hash: ::Hash, number: NumberFor) { + pub fn set_sync_fork_request( + &self, + peers: Vec, + hash: ::Hash, + number: NumberFor, + ) { self.network.service().set_sync_fork_request(peers, hash, number); } /// Add blocks to the peer -- edit the block before adding - pub fn generate_blocks(&mut self, count: usize, origin: BlockOrigin, edit_block: F) -> H256 - where F: FnMut(BlockBuilder) -> Block + pub fn generate_blocks( + &mut self, + count: usize, + origin: BlockOrigin, + edit_block: F, + ) -> H256 + where + F: FnMut(BlockBuilder) -> Block { let best_hash = self.client.info().best_hash; self.generate_blocks_at(BlockId::Hash(best_hash), count, origin, edit_block, false, true, true) @@ -320,19 +360,21 @@ impl Peer { block.header.parent_hash, ); let header = block.header.clone(); - let (import_block, cache) = self.verifier.verify( + let (import_block, cache) = futures::executor::block_on(self.verifier.verify( origin, header.clone(), None, if headers_only { None } else { Some(block.extrinsics) }, - ).unwrap(); + )).unwrap(); let cache = if let Some(cache) = cache { cache.into_iter().collect() } else { Default::default() }; - self.block_import.import_block(import_block, cache).expect("block_import failed"); + futures::executor::block_on( + self.block_import.import_block(import_block, cache) + ).expect("block_import failed"); if announce_block { self.network.service().announce_block(hash, None); } @@ -478,102 +520,80 @@ impl Peer { } } +pub trait BlockImportAdapterFull: + BlockImport< + Block, + Transaction = TransactionFor, + Error = ConsensusError, + > + + Send + + Sync + + Clone +{} + +impl BlockImportAdapterFull for T where + T: BlockImport< + Block, + Transaction = TransactionFor, + Error = ConsensusError, + > + + Send + + Sync + + Clone +{} + /// Implements `BlockImport` for any `Transaction`. Internally the transaction is /// "converted", aka the field is set to `None`. /// /// This is required as the `TestNetFactory` trait does not distinguish between /// full and light nodes. -pub enum BlockImportAdapter { - Full( - Arc, - Error = ConsensusError - > + Send>>, - PhantomData, - ), - Light( - Arc, - Error = ConsensusError - > + Send>>, - PhantomData, - ), +#[derive(Clone)] +pub struct BlockImportAdapter { + inner: I, } -impl BlockImportAdapter { +impl BlockImportAdapter { /// Create a new instance of `Self::Full`. - pub fn new_full( - full: impl BlockImport< - Block, - Transaction = TransactionFor, - Error = ConsensusError - > - + 'static - + Send - ) -> Self { - Self::Full(Arc::new(Mutex::new(full)), PhantomData) - } - - /// Create a new instance of `Self::Light`. - pub fn new_light( - light: impl BlockImport< - Block, - Transaction = TransactionFor, - Error = ConsensusError - > - + 'static - + Send - ) -> Self { - Self::Light(Arc::new(Mutex::new(light)), PhantomData) - } -} - -impl Clone for BlockImportAdapter { - fn clone(&self) -> Self { - match self { - Self::Full(full, _) => Self::Full(full.clone(), PhantomData), - Self::Light(light, _) => Self::Light(light.clone(), PhantomData), + pub fn new(inner: I) -> Self { + Self { + inner, } } } -impl BlockImport for BlockImportAdapter { +#[async_trait::async_trait] +impl BlockImport for BlockImportAdapter where + I: BlockImport + Send + Sync, + I::Transaction: Send, +{ type Error = ConsensusError; - type Transaction = Transaction; + type Transaction = (); - fn check_block( + async fn check_block( &mut self, block: BlockCheckParams, ) -> Result { - match self { - Self::Full(full, _) => full.lock().check_block(block), - Self::Light(light, _) => light.lock().check_block(block), - } + self.inner.check_block(block).await } - fn import_block( + async fn import_block( &mut self, - block: BlockImportParams, + block: BlockImportParams, cache: HashMap>, ) -> Result { - match self { - Self::Full(full, _) => full.lock().import_block(block.convert_transaction(), cache), - Self::Light(light, _) => light.lock().import_block(block.convert_transaction(), cache), - } + self.inner.import_block(block.convert_transaction(), cache).await } } -/// Implements `Verifier` on an `Arc>`. Used internally. -#[derive(Clone)] -struct VerifierAdapter { - verifier: Arc>>>, +/// Implements `Verifier` and keeps track of failed verifications. +struct VerifierAdapter { + verifier: Verifier, failed_verifications: Arc>>, } -impl Verifier for VerifierAdapter { - fn verify( +#[async_trait::async_trait] +impl> Verifier for VerifierAdapter { + async fn verify( &mut self, origin: BlockOrigin, header: B::Header, @@ -581,15 +601,15 @@ impl Verifier for VerifierAdapter { body: Option> ) -> Result<(BlockImportParams, Option)>>), String> { let hash = header.hash(); - self.verifier.lock().verify(origin, header, justifications, body).map_err(|e| { + self.verifier.verify(origin, header, justifications, body).await.map_err(|e| { self.failed_verifications.lock().insert(hash, e.clone()); e }) } } -impl VerifierAdapter { - fn new(verifier: Arc>>>) -> VerifierAdapter { +impl VerifierAdapter { + fn new(verifier: Verifier) -> Self { VerifierAdapter { verifier, failed_verifications: Default::default(), @@ -614,8 +634,9 @@ pub struct FullPeerConfig { pub is_authority: bool, } -pub trait TestNetFactory: Sized { - type Verifier: 'static + Verifier; +pub trait TestNetFactory: Sized where >::Transaction: Send { + type Verifier: 'static + Verifier + Clone; + type BlockImport: BlockImport + Clone + Send + Sync + 'static; type PeerData: Default; /// These two need to be implemented! @@ -628,23 +649,20 @@ pub trait TestNetFactory: Sized { ) -> Self::Verifier; /// Get reference to peer. - fn peer(&mut self, i: usize) -> &mut Peer; - fn peers(&self) -> &Vec>; - fn mut_peers>)>( + fn peer(&mut self, i: usize) -> &mut Peer; + fn peers(&self) -> &Vec>; + fn mut_peers>)>( &mut self, closure: F, ); /// Get custom block import handle for fresh client, along with peer data. - fn make_block_import(&self, client: PeersClient) + fn make_block_import(&self, client: PeersClient) -> ( - BlockImportAdapter, + BlockImportAdapter, Option>, Self::PeerData, - ) - { - (client.as_block_import(), None, Default::default()) - } + ); fn default_config() -> ProtocolConfig { ProtocolConfig::default() @@ -688,7 +706,6 @@ pub trait TestNetFactory: Sized { &Default::default(), &data, ); - let verifier = VerifierAdapter::new(Arc::new(Mutex::new(Box::new(verifier) as Box<_>))); let import_queue = Box::new(BasicQueue::new( verifier.clone(), @@ -776,13 +793,13 @@ pub trait TestNetFactory: Sized { peers.push(Peer { data, - client: PeersClient::Full(client, backend.clone()), + client: PeersClient::Full(client.clone(), backend.clone()), select_chain: Some(longest_chain), backend: Some(backend), imported_blocks_stream, finality_notification_stream, block_import, - verifier, + verifier: VerifierAdapter::new(verifier.clone()), network, listen_addr, }); @@ -804,7 +821,6 @@ pub trait TestNetFactory: Sized { &Default::default(), &data, ); - let verifier = VerifierAdapter::new(Arc::new(Mutex::new(Box::new(verifier) as Box<_>))); let import_queue = Box::new(BasicQueue::new( verifier.clone(), @@ -861,7 +877,7 @@ pub trait TestNetFactory: Sized { peers.push(Peer { data, - verifier, + verifier: VerifierAdapter::new(verifier.clone()), select_chain: None, backend: None, block_import, @@ -986,7 +1002,7 @@ pub trait TestNetFactory: Sized { } pub struct TestNet { - peers: Vec>, + peers: Vec>, fork_choice: ForkChoiceStrategy, } @@ -1003,6 +1019,7 @@ impl TestNet { impl TestNetFactory for TestNet { type Verifier = PassThroughVerifier; type PeerData = (); + type BlockImport = PeersClient; /// Create new test network with peers and given config. fn from_config(_config: &ProtocolConfig) -> Self { @@ -1018,15 +1035,25 @@ impl TestNetFactory for TestNet { PassThroughVerifier::new_with_fork_choice(false, self.fork_choice.clone()) } - fn peer(&mut self, i: usize) -> &mut Peer<()> { + fn make_block_import(&self, client: PeersClient) + -> ( + BlockImportAdapter, + Option>, + Self::PeerData, + ) + { + (client.as_block_import(), None, ()) + } + + fn peer(&mut self, i: usize) -> &mut Peer<(), Self::Verifier, Self::BlockImport> { &mut self.peers[i] } - fn peers(&self) -> &Vec> { + fn peers(&self) -> &Vec> { &self.peers } - fn mut_peers>)>(&mut self, closure: F) { + fn mut_peers>)>(&mut self, closure: F) { closure(&mut self.peers); } } @@ -1052,6 +1079,7 @@ pub struct JustificationTestNet(TestNet); impl TestNetFactory for JustificationTestNet { type Verifier = PassThroughVerifier; type PeerData = (); + type BlockImport = PeersClient; fn from_config(config: &ProtocolConfig) -> Self { JustificationTestNet(TestNet::from_config(config)) @@ -1061,23 +1089,23 @@ impl TestNetFactory for JustificationTestNet { self.0.make_verifier(client, config, peer_data) } - fn peer(&mut self, i: usize) -> &mut Peer { + fn peer(&mut self, i: usize) -> &mut Peer { self.0.peer(i) } - fn peers(&self) -> &Vec> { + fn peers(&self) -> &Vec> { self.0.peers() } fn mut_peers>, + &mut Vec>, )>(&mut self, closure: F) { self.0.mut_peers(closure) } - fn make_block_import(&self, client: PeersClient) + fn make_block_import(&self, client: PeersClient) -> ( - BlockImportAdapter, + BlockImportAdapter, Option>, Self::PeerData, ) diff --git a/client/service/Cargo.toml b/client/service/Cargo.toml index 6ce1ed8b34e14..cff05390d7874 100644 --- a/client/service/Cargo.toml +++ b/client/service/Cargo.toml @@ -78,6 +78,7 @@ sp-tracing = { version = "3.0.0", path = "../../primitives/tracing" } tracing = "0.1.25" tracing-futures = { version = "0.2.4" } parity-util-mem = { version = "0.9.0", default-features = false, features = ["primitive-types"] } +async-trait = "0.1.42" [target.'cfg(not(target_os = "unknown"))'.dependencies] tempfile = "3.1.0" diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index a39c456641920..66bb794f7e7ae 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -1698,6 +1698,7 @@ impl CallApiAt for Client where /// NOTE: only use this implementation when you are sure there are NO consensus-level BlockImport /// objects. Otherwise, importing blocks directly into the client would be bypassing /// important verification work. +#[async_trait::async_trait] impl sp_consensus::BlockImport for &Client where B: backend::Backend, E: CallExecutor + Send + Sync, @@ -1705,6 +1706,7 @@ impl sp_consensus::BlockImport for &Client: ProvideRuntimeApi, as ProvideRuntimeApi>::Api: CoreApi + ApiExt, + RA: Sync + Send, { type Error = ConsensusError; type Transaction = backend::TransactionFor; @@ -1718,7 +1720,7 @@ impl sp_consensus::BlockImport for &Client>, new_cache: HashMap>, @@ -1742,7 +1744,7 @@ impl sp_consensus::BlockImport for &Client, ) -> Result { @@ -1798,6 +1800,7 @@ impl sp_consensus::BlockImport for &Client sp_consensus::BlockImport for Client where B: backend::Backend, E: CallExecutor + Send + Sync, @@ -1805,23 +1808,24 @@ impl sp_consensus::BlockImport for Client, >::Api: CoreApi + ApiExt, + RA: Sync + Send, { type Error = ConsensusError; type Transaction = backend::TransactionFor; - fn import_block( + async fn import_block( &mut self, import_block: BlockImportParams, new_cache: HashMap>, ) -> Result { - (&*self).import_block(import_block, new_cache) + (&*self).import_block(import_block, new_cache).await } - fn check_block( + async fn check_block( &mut self, block: BlockCheckParams, ) -> Result { - (&*self).check_block(block) + (&*self).check_block(block).await } } diff --git a/test-utils/client/Cargo.toml b/test-utils/client/Cargo.toml index df1cca2101ad7..925a69e41bb48 100644 --- a/test-utils/client/Cargo.toml +++ b/test-utils/client/Cargo.toml @@ -33,3 +33,4 @@ sp-keystore = { version = "0.9.0", path = "../../primitives/keystore" } sp-keyring = { version = "3.0.0", path = "../../primitives/keyring" } sp-runtime = { version = "3.0.0", path = "../../primitives/runtime" } sp-state-machine = { version = "0.9.0", path = "../../primitives/state-machine" } +async-trait = "0.1.42" diff --git a/test-utils/client/src/client_ext.rs b/test-utils/client/src/client_ext.rs index aa4856f6baf66..edba96d760fc2 100644 --- a/test-utils/client/src/client_ext.rs +++ b/test-utils/client/src/client_ext.rs @@ -43,18 +43,20 @@ pub trait ClientExt: Sized { } /// Extension trait for a test client around block importing. +#[async_trait::async_trait] pub trait ClientBlockImportExt: Sized { /// Import block to the chain. No finality. - fn import(&mut self, origin: BlockOrigin, block: Block) -> Result<(), ConsensusError>; + async fn import(&mut self, origin: BlockOrigin, block: Block) -> Result<(), ConsensusError>; /// Import a block and make it our best block if possible. - fn import_as_best(&mut self, origin: BlockOrigin, block: Block) -> Result<(), ConsensusError>; + async fn import_as_best(&mut self, origin: BlockOrigin, block: Block) -> Result<(), ConsensusError>; /// Import a block and finalize it. - fn import_as_final(&mut self, origin: BlockOrigin, block: Block) -> Result<(), ConsensusError>; + async fn import_as_final(&mut self, origin: BlockOrigin, block: Block) + -> Result<(), ConsensusError>; /// Import block with justification(s), finalizes block. - fn import_justified( + async fn import_justified( &mut self, origin: BlockOrigin, block: Block, @@ -83,38 +85,54 @@ impl ClientExt for Client } /// This implementation is required, because of the weird api requirements around `BlockImport`. +#[async_trait::async_trait] impl ClientBlockImportExt for std::sync::Arc - where for<'r> &'r T: BlockImport + where + for<'r> &'r T: BlockImport, + Transaction: Send + 'static, + T: Send + Sync, { - fn import(&mut self, origin: BlockOrigin, block: Block) -> Result<(), ConsensusError> { + async fn import( + &mut self, + origin: BlockOrigin, + block: Block, + ) -> Result<(), ConsensusError> { let (header, extrinsics) = block.deconstruct(); let mut import = BlockImportParams::new(origin, header); import.body = Some(extrinsics); import.fork_choice = Some(ForkChoiceStrategy::LongestChain); - BlockImport::import_block(self, import, HashMap::new()).map(|_| ()) + BlockImport::import_block(self, import, HashMap::new()).await.map(|_| ()) } - fn import_as_best(&mut self, origin: BlockOrigin, block: Block) -> Result<(), ConsensusError> { + async fn import_as_best( + &mut self, + origin: BlockOrigin, + block: Block, + ) -> Result<(), ConsensusError> { let (header, extrinsics) = block.deconstruct(); let mut import = BlockImportParams::new(origin, header); import.body = Some(extrinsics); import.fork_choice = Some(ForkChoiceStrategy::Custom(true)); - BlockImport::import_block(self, import, HashMap::new()).map(|_| ()) + BlockImport::import_block(self, import, HashMap::new()).await.map(|_| ()) } - fn import_as_final(&mut self, origin: BlockOrigin, block: Block) -> Result<(), ConsensusError> { + async fn import_as_final( + &mut self, + origin: BlockOrigin, + block: Block, + ) -> Result<(), ConsensusError> { let (header, extrinsics) = block.deconstruct(); let mut import = BlockImportParams::new(origin, header); import.body = Some(extrinsics); import.finalized = true; import.fork_choice = Some(ForkChoiceStrategy::Custom(true)); - BlockImport::import_block(self, import, HashMap::new()).map(|_| ()) + BlockImport::import_block(self, import, HashMap::new()).await.map(|_| ()) } - fn import_justified( + async fn import_justified( &mut self, origin: BlockOrigin, block: Block, @@ -127,43 +145,60 @@ impl ClientBlockImportExt for std::sync::A import.finalized = true; import.fork_choice = Some(ForkChoiceStrategy::LongestChain); - BlockImport::import_block(self, import, HashMap::new()).map(|_| ()) + BlockImport::import_block(self, import, HashMap::new()).await.map(|_| ()) } } +#[async_trait::async_trait] impl ClientBlockImportExt for Client where Self: BlockImport, + RA: Send, + B: Send + Sync, + E: Send, + >::Transaction: Send, { - fn import(&mut self, origin: BlockOrigin, block: Block) -> Result<(), ConsensusError> { + async fn import( + &mut self, + origin: BlockOrigin, + block: Block, + ) -> Result<(), ConsensusError> { let (header, extrinsics) = block.deconstruct(); let mut import = BlockImportParams::new(origin, header); import.body = Some(extrinsics); import.fork_choice = Some(ForkChoiceStrategy::LongestChain); - BlockImport::import_block(self, import, HashMap::new()).map(|_| ()) + BlockImport::import_block(self, import, HashMap::new()).await.map(|_| ()) } - fn import_as_best(&mut self, origin: BlockOrigin, block: Block) -> Result<(), ConsensusError> { + async fn import_as_best( + &mut self, + origin: BlockOrigin, + block: Block, + ) -> Result<(), ConsensusError> { let (header, extrinsics) = block.deconstruct(); let mut import = BlockImportParams::new(origin, header); import.body = Some(extrinsics); import.fork_choice = Some(ForkChoiceStrategy::Custom(true)); - BlockImport::import_block(self, import, HashMap::new()).map(|_| ()) + BlockImport::import_block(self, import, HashMap::new()).await.map(|_| ()) } - fn import_as_final(&mut self, origin: BlockOrigin, block: Block) -> Result<(), ConsensusError> { + async fn import_as_final( + &mut self, + origin: BlockOrigin, + block: Block, + ) -> Result<(), ConsensusError> { let (header, extrinsics) = block.deconstruct(); let mut import = BlockImportParams::new(origin, header); import.body = Some(extrinsics); import.finalized = true; import.fork_choice = Some(ForkChoiceStrategy::Custom(true)); - BlockImport::import_block(self, import, HashMap::new()).map(|_| ()) + BlockImport::import_block(self, import, HashMap::new()).await.map(|_| ()) } - fn import_justified( + async fn import_justified( &mut self, origin: BlockOrigin, block: Block, @@ -176,6 +211,6 @@ impl ClientBlockImportExt for Client(backend: Arc) where @@ -57,7 +58,7 @@ pub fn test_leaves_for_backend(backend: Arc) where // G -> A1 let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a1.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap(); assert_eq!( blockchain.leaves().unwrap(), vec![a1.hash()], @@ -69,7 +70,7 @@ pub fn test_leaves_for_backend(backend: Arc) where Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a2.clone())).unwrap(); #[allow(deprecated)] assert_eq!( @@ -83,7 +84,7 @@ pub fn test_leaves_for_backend(backend: Arc) where Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a3.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a3.clone())).unwrap(); assert_eq!( blockchain.leaves().unwrap(), @@ -96,7 +97,7 @@ pub fn test_leaves_for_backend(backend: Arc) where Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a4.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a4.clone())).unwrap(); assert_eq!( blockchain.leaves().unwrap(), vec![a4.hash()], @@ -109,7 +110,7 @@ pub fn test_leaves_for_backend(backend: Arc) where false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a5.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a5.clone())).unwrap(); assert_eq!( blockchain.leaves().unwrap(), vec![a5.hash()], @@ -130,7 +131,7 @@ pub fn test_leaves_for_backend(backend: Arc) where nonce: 0, }).unwrap(); let b2 = builder.build().unwrap().block; - client.import(BlockOrigin::Own, b2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b2.clone())).unwrap(); assert_eq!( blockchain.leaves().unwrap(), vec![a5.hash(), b2.hash()], @@ -143,7 +144,7 @@ pub fn test_leaves_for_backend(backend: Arc) where false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, b3.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b3.clone())).unwrap(); assert_eq!( blockchain.leaves().unwrap(), vec![a5.hash(), b3.hash()], @@ -155,7 +156,7 @@ pub fn test_leaves_for_backend(backend: Arc) where Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, b4.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b4.clone())).unwrap(); assert_eq!( blockchain.leaves().unwrap(), vec![a5.hash(), b4.hash()], @@ -175,7 +176,7 @@ pub fn test_leaves_for_backend(backend: Arc) where nonce: 1, }).unwrap(); let c3 = builder.build().unwrap().block; - client.import(BlockOrigin::Own, c3.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, c3.clone())).unwrap(); assert_eq!( blockchain.leaves().unwrap(), vec![a5.hash(), b4.hash(), c3.hash()], @@ -195,7 +196,7 @@ pub fn test_leaves_for_backend(backend: Arc) where nonce: 0, }).unwrap(); let d2 = builder.build().unwrap().block; - client.import(BlockOrigin::Own, d2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, d2.clone())).unwrap(); assert_eq!( blockchain.leaves().unwrap(), vec![a5.hash(), b4.hash(), c3.hash(), d2.hash()], @@ -220,7 +221,7 @@ pub fn test_children_for_backend(backend: Arc) where // G -> A1 let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a1.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap(); // A1 -> A2 let a2 = client.new_block_at( @@ -228,7 +229,7 @@ pub fn test_children_for_backend(backend: Arc) where Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a2.clone())).unwrap(); // A2 -> A3 let a3 = client.new_block_at( @@ -236,7 +237,7 @@ pub fn test_children_for_backend(backend: Arc) where Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a3.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a3.clone())).unwrap(); // A3 -> A4 let a4 = client.new_block_at( @@ -244,7 +245,7 @@ pub fn test_children_for_backend(backend: Arc) where Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a4.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a4.clone())).unwrap(); // A4 -> A5 let a5 = client.new_block_at( @@ -252,7 +253,7 @@ pub fn test_children_for_backend(backend: Arc) where Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a5.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a5.clone())).unwrap(); // A1 -> B2 let mut builder = client.new_block_at( @@ -268,7 +269,7 @@ pub fn test_children_for_backend(backend: Arc) where nonce: 0, }).unwrap(); let b2 = builder.build().unwrap().block; - client.import(BlockOrigin::Own, b2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b2.clone())).unwrap(); // B2 -> B3 let b3 = client.new_block_at( @@ -276,7 +277,7 @@ pub fn test_children_for_backend(backend: Arc) where Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, b3.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b3.clone())).unwrap(); // B3 -> B4 let b4 = client.new_block_at( @@ -284,7 +285,7 @@ pub fn test_children_for_backend(backend: Arc) where Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, b4).unwrap(); + block_on(client.import(BlockOrigin::Own, b4)).unwrap(); // // B2 -> C3 let mut builder = client.new_block_at( @@ -300,7 +301,7 @@ pub fn test_children_for_backend(backend: Arc) where nonce: 1, }).unwrap(); let c3 = builder.build().unwrap().block; - client.import(BlockOrigin::Own, c3.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, c3.clone())).unwrap(); // A1 -> D2 let mut builder = client.new_block_at( @@ -316,7 +317,7 @@ pub fn test_children_for_backend(backend: Arc) where nonce: 0, }).unwrap(); let d2 = builder.build().unwrap().block; - client.import(BlockOrigin::Own, d2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, d2.clone())).unwrap(); let genesis_hash = client.chain_info().genesis_hash; @@ -349,7 +350,7 @@ pub fn test_blockchain_query_by_number_gets_canonical(backend: Arc A1 let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a1.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap(); // A1 -> A2 let a2 = client.new_block_at( @@ -357,7 +358,7 @@ pub fn test_blockchain_query_by_number_gets_canonical(backend: Arc A3 let a3 = client.new_block_at( @@ -365,7 +366,7 @@ pub fn test_blockchain_query_by_number_gets_canonical(backend: Arc A4 let a4 = client.new_block_at( @@ -373,7 +374,7 @@ pub fn test_blockchain_query_by_number_gets_canonical(backend: Arc A5 let a5 = client.new_block_at( @@ -381,7 +382,7 @@ pub fn test_blockchain_query_by_number_gets_canonical(backend: Arc B2 let mut builder = client.new_block_at( @@ -397,7 +398,7 @@ pub fn test_blockchain_query_by_number_gets_canonical(backend: Arc B3 let b3 = client.new_block_at( @@ -405,7 +406,7 @@ pub fn test_blockchain_query_by_number_gets_canonical(backend: Arc B4 let b4 = client.new_block_at( @@ -413,7 +414,7 @@ pub fn test_blockchain_query_by_number_gets_canonical(backend: Arc C3 let mut builder = client.new_block_at( @@ -429,7 +430,7 @@ pub fn test_blockchain_query_by_number_gets_canonical(backend: Arc D2 let mut builder = client.new_block_at( @@ -445,7 +446,7 @@ pub fn test_blockchain_query_by_number_gets_canonical(backend: Arc Date: Fri, 26 Mar 2021 00:01:28 +0100 Subject: [PATCH 06/27] More Babe work --- client/consensus/babe/src/aux_schema.rs | 2 -- client/consensus/babe/src/tests.rs | 15 ++++++++++----- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/client/consensus/babe/src/aux_schema.rs b/client/consensus/babe/src/aux_schema.rs index b753535c6b7aa..49b742cc6b256 100644 --- a/client/consensus/babe/src/aux_schema.rs +++ b/client/consensus/babe/src/aux_schema.rs @@ -18,8 +18,6 @@ //! Schema for BABE epoch changes in the aux-db. -use std::sync::Arc; -use parking_lot::Mutex; use log::info; use codec::{Decode, Encode}; diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index 70b4cd7b0b61d..1dfc7a358839b 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -67,6 +67,9 @@ enum Stage { type Mutator = Arc; +type BabeBlockImport = + PanickingBlockImport>; + #[derive(Clone)] struct DummyFactory { client: Arc, @@ -210,8 +213,10 @@ impl> BlockImport for PanickingBlockImport< } } +type BabePeer = Peer, TestVerifier, BabeBlockImport>; + pub struct BabeTestNet { - peers: Vec>>, + peers: Vec, } type TestHeader = ::Header; @@ -326,17 +331,17 @@ impl TestNetFactory for BabeTestNet { } } - fn peer(&mut self, i: usize) -> &mut Peer { + fn peer(&mut self, i: usize) -> &mut BabePeer { trace!(target: "babe", "Retrieving a peer"); &mut self.peers[i] } - fn peers(&self) -> &Vec> { + fn peers(&self) -> &Vec { trace!(target: "babe", "Retrieving peers"); &self.peers } - fn mut_peers>)>( + fn mut_peers)>( &mut self, closure: F, ) { @@ -623,7 +628,7 @@ fn propose_and_import_block( import.body = Some(block.extrinsics); import.intermediates.insert( Cow::from(INTERMEDIATE_KEY), - Box::new(BabeIntermediate:: { epoch_descriptor }) as Box, + Box::new(BabeIntermediate:: { epoch_descriptor }) as Box<_>, ); import.fork_choice = Some(ForkChoiceStrategy::LongestChain); let import_result = block_import.import_block(import, Default::default()).unwrap(); From 076d4dc93c43cdb2eb408bc63bd838b1805764ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 26 Mar 2021 11:38:59 +0100 Subject: [PATCH 07/27] Make it async --- Cargo.lock | 1 + primitives/consensus/common/Cargo.toml | 1 + .../consensus/common/src/block_import.rs | 37 ++++++++++------- .../consensus/common/src/import_queue.rs | 41 +++++++++++-------- .../common/src/import_queue/basic_queue.rs | 8 ++-- 5 files changed, 52 insertions(+), 36 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ebdd76da2bf0e..6a9900b77e8e0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8668,6 +8668,7 @@ dependencies = [ name = "sp-consensus" version = "0.9.0" dependencies = [ + "async-trait", "futures 0.3.13", "futures-timer 3.0.2", "libp2p", diff --git a/primitives/consensus/common/Cargo.toml b/primitives/consensus/common/Cargo.toml index 3d71cf63f55d1..6c3ae5fc060bf 100644 --- a/primitives/consensus/common/Cargo.toml +++ b/primitives/consensus/common/Cargo.toml @@ -34,6 +34,7 @@ parking_lot = "0.11.1" serde = { version = "1.0", features = ["derive"] } prometheus-endpoint = { package = "substrate-prometheus-endpoint", path = "../../../utils/prometheus", version = "0.9.0"} wasm-timer = "0.2.5" +async-trait = "0.1.42" [dev-dependencies] futures = "0.3.9" diff --git a/primitives/consensus/common/src/block_import.rs b/primitives/consensus/common/src/block_import.rs index 9b7995a2b00bd..8d01da64b4cd6 100644 --- a/primitives/consensus/common/src/block_import.rs +++ b/primitives/consensus/common/src/block_import.rs @@ -146,7 +146,7 @@ pub struct BlockImportParams { /// Intermediate values that are interpreted by block importers. Each block importer, /// upon handling a value, removes it from the intermediate list. The final block importer /// rejects block import if there are still intermediate values that remain unhandled. - pub intermediates: HashMap, Box>, + pub intermediates: HashMap, Box>, /// Auxiliary consensus data produced by the block. /// Contains a list of key-value pairs. If values are `None`, the keys /// will be deleted. @@ -264,14 +264,15 @@ impl BlockImportParams { } /// Block import trait. +#[async_trait::async_trait] pub trait BlockImport { /// The error type. type Error: std::error::Error + Send + 'static; /// The transaction type used by the backend. - type Transaction; + type Transaction: Send + 'static; /// Check block preconditions. - fn check_block( + async fn check_block( &mut self, block: BlockCheckParams, ) -> Result; @@ -279,56 +280,64 @@ pub trait BlockImport { /// Import a block. /// /// Cached data can be accessed through the blockchain cache. - fn import_block( + async fn import_block( &mut self, block: BlockImportParams, cache: HashMap>, ) -> Result; } -impl BlockImport for crate::import_queue::BoxBlockImport { +#[async_trait::async_trait] +impl BlockImport for crate::import_queue::BoxBlockImport + where + Transaction: Send + 'static, +{ type Error = crate::error::Error; type Transaction = Transaction; /// Check block preconditions. - fn check_block( + async fn check_block( &mut self, block: BlockCheckParams, ) -> Result { - (**self).check_block(block) + (**self).check_block(block).await } /// Import a block. /// /// Cached data can be accessed through the blockchain cache. - fn import_block( + async fn import_block( &mut self, block: BlockImportParams, cache: HashMap>, ) -> Result { - (**self).import_block(block, cache) + (**self).import_block(block, cache).await } } +#[async_trait::async_trait] impl BlockImport for Arc - where for<'r> &'r T: BlockImport + where + for<'r> &'r T: BlockImport, + T: Send + Sync, + Transaction: Send + 'static, { type Error = E; type Transaction = Transaction; - fn check_block( + async fn check_block( &mut self, block: BlockCheckParams, ) -> Result { - (&**self).check_block(block) + (&**self).check_block(block).await } - fn import_block( + async fn import_block( &mut self, block: BlockImportParams, cache: HashMap>, ) -> Result { - (&**self).import_block(block, cache) + (&**self).import_block(block, cache).await } } diff --git a/primitives/consensus/common/src/import_queue.rs b/primitives/consensus/common/src/import_queue.rs index b6067645a8920..73ec2e08d3160 100644 --- a/primitives/consensus/common/src/import_queue.rs +++ b/primitives/consensus/common/src/import_queue.rs @@ -82,11 +82,12 @@ pub struct IncomingBlock { pub type CacheKeyId = [u8; 4]; /// Verify a justification of a block +#[async_trait::async_trait] pub trait Verifier: Send + Sync { /// Verify the given data and return the BlockImportParams and an optional /// new set of validators to import. If not, err with an Error-Message /// presented to the User in the logs. - fn verify( + async fn verify( &mut self, origin: BlockOrigin, header: B::Header, @@ -163,17 +164,17 @@ pub enum BlockImportError { } /// Single block import function. -pub fn import_single_block, Transaction>( +pub async fn import_single_block, Transaction: Send + 'static>( import_handle: &mut dyn BlockImport, block_origin: BlockOrigin, block: IncomingBlock, verifier: &mut V, ) -> Result>, BlockImportError> { - import_single_block_metered(import_handle, block_origin, block, verifier, None) + import_single_block_metered(import_handle, block_origin, block, verifier, None).await } /// Single block import function with metering. -pub(crate) fn import_single_block_metered, Transaction>( +pub(crate) async fn import_single_block_metered, Transaction: Send + 'static>( import_handle: &mut dyn BlockImport, block_origin: BlockOrigin, block: IncomingBlock, @@ -232,24 +233,28 @@ pub(crate) fn import_single_block_metered, Transaction parent_hash, allow_missing_state: block.allow_missing_state, import_existing: block.import_existing, - }))? { + }).await)? { BlockImportResult::ImportedUnknown { .. } => (), r => return Ok(r), // Any other successful result means that the block is already imported. } let started = wasm_timer::Instant::now(); - let (mut import_block, maybe_keys) = verifier.verify(block_origin, header, justifications, block.body) - .map_err(|msg| { - if let Some(ref peer) = peer { - trace!(target: "sync", "Verifying {}({}) from {} failed: {}", number, hash, peer, msg); - } else { - trace!(target: "sync", "Verifying {}({}) failed: {}", number, hash, msg); - } - if let Some(metrics) = metrics.as_ref() { - metrics.report_verification(false, started.elapsed()); - } - BlockImportError::VerificationFailed(peer.clone(), msg) - })?; + let (mut import_block, maybe_keys) = verifier.verify( + block_origin, + header, + justifications, + block.body + ).await.map_err(|msg| { + if let Some(ref peer) = peer { + trace!(target: "sync", "Verifying {}({}) from {} failed: {}", number, hash, peer, msg); + } else { + trace!(target: "sync", "Verifying {}({}) failed: {}", number, hash, msg); + } + if let Some(metrics) = metrics.as_ref() { + metrics.report_verification(false, started.elapsed()); + } + BlockImportError::VerificationFailed(peer.clone(), msg) + })?; if let Some(metrics) = metrics.as_ref() { metrics.report_verification(true, started.elapsed()); @@ -261,7 +266,7 @@ pub(crate) fn import_single_block_metered, Transaction } import_block.allow_missing_state = block.allow_missing_state; - let imported = import_handle.import_block(import_block.convert_transaction(), cache); + let imported = import_handle.import_block(import_block.convert_transaction(), cache).await; if let Some(metrics) = metrics.as_ref() { metrics.report_verification_and_import(started.elapsed()); } diff --git a/primitives/consensus/common/src/import_queue/basic_queue.rs b/primitives/consensus/common/src/import_queue/basic_queue.rs index eb2b4b1fa7fcd..2a1c615b68dba 100644 --- a/primitives/consensus/common/src/import_queue/basic_queue.rs +++ b/primitives/consensus/common/src/import_queue/basic_queue.rs @@ -155,7 +155,7 @@ mod worker_messages { /// to be run. /// /// Returns when `block_import` ended. -async fn block_import_process( +async fn block_import_process( mut block_import: BoxBlockImport, mut verifier: impl Verifier, mut result_sender: BufferedLinkSender, @@ -195,7 +195,7 @@ struct BlockImportWorker { } impl BlockImportWorker { - fn new, Transaction: Send>( + fn new, Transaction: Send + 'static>( result_sender: BufferedLinkSender, verifier: V, block_import: BoxBlockImport, @@ -322,7 +322,7 @@ struct ImportManyBlocksResult { /// Import several blocks at once, returning import result for each block. /// /// This will yield after each imported block once, to ensure that other futures can be called as well. -async fn import_many_blocks, Transaction>( +async fn import_many_blocks, Transaction: Send + 'static>( import_handle: &mut BoxBlockImport, blocks_origin: BlockOrigin, blocks: Vec>, @@ -371,7 +371,7 @@ async fn import_many_blocks, Transaction>( block, verifier, metrics.clone(), - ) + ).await }; if let Some(metrics) = metrics.as_ref() { From 9fe6eadba0439e3c99138cd894400c1e24d47cb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 26 Mar 2021 14:02:19 +0100 Subject: [PATCH 08/27] Fix fix --- primitives/consensus/common/src/import_queue.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/primitives/consensus/common/src/import_queue.rs b/primitives/consensus/common/src/import_queue.rs index 73ec2e08d3160..4220c7b14162d 100644 --- a/primitives/consensus/common/src/import_queue.rs +++ b/primitives/consensus/common/src/import_queue.rs @@ -165,7 +165,7 @@ pub enum BlockImportError { /// Single block import function. pub async fn import_single_block, Transaction: Send + 'static>( - import_handle: &mut dyn BlockImport, + import_handle: &mut impl BlockImport, block_origin: BlockOrigin, block: IncomingBlock, verifier: &mut V, @@ -175,7 +175,7 @@ pub async fn import_single_block, Transaction: Send + /// Single block import function with metering. pub(crate) async fn import_single_block_metered, Transaction: Send + 'static>( - import_handle: &mut dyn BlockImport, + import_handle: &mut impl BlockImport, block_origin: BlockOrigin, block: IncomingBlock, verifier: &mut V, From 656044419df62a86b3839c49104b0d7e88c8c59b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 26 Mar 2021 14:03:04 +0100 Subject: [PATCH 09/27] Use `async_trait` in sc-consensus-slots This makes the code a little bit easier to read and also expresses that there can always only be one call at a time to `on_slot`. --- Cargo.lock | 1 + client/consensus/babe/src/lib.rs | 2 +- client/consensus/slots/Cargo.toml | 1 + client/consensus/slots/src/lib.rs | 276 ++++++++++++++++------------ client/consensus/slots/src/slots.rs | 33 ++-- 5 files changed, 171 insertions(+), 142 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6a9900b77e8e0..ed301508fefcf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7315,6 +7315,7 @@ dependencies = [ name = "sc-consensus-slots" version = "0.9.0" dependencies = [ + "async-trait", "futures 0.3.13", "futures-timer 3.0.2", "log", diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index e5910f5b35e61..1faac542652c0 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -438,7 +438,7 @@ pub fn start_babe(BabeParams { + Sync + 'static, Error: std::error::Error + Send + From + From + 'static, SO: SyncOracle + Send + Sync + Clone + 'static, - CAW: CanAuthorWith + Send + 'static, + CAW: CanAuthorWith + Send + Sync + 'static, BS: BackoffAuthoringBlocksStrategy> + Send + 'static, { const HANDLE_BUFFER_SIZE: usize = 1024; diff --git a/client/consensus/slots/Cargo.toml b/client/consensus/slots/Cargo.toml index 34162cfae71e2..623c4c4abd84c 100644 --- a/client/consensus/slots/Cargo.toml +++ b/client/consensus/slots/Cargo.toml @@ -34,6 +34,7 @@ futures-timer = "3.0.1" parking_lot = "0.11.1" log = "0.4.11" thiserror = "1.0.21" +async-trait = "0.1.42" [dev-dependencies] substrate-test-runtime-client = { version = "2.0.0", path = "../../../test-utils/runtime/client" } diff --git a/client/consensus/slots/src/lib.rs b/client/consensus/slots/src/lib.rs index 83dd88a8d49ff..dbf62ccad23c8 100644 --- a/client/consensus/slots/src/lib.rs +++ b/client/consensus/slots/src/lib.rs @@ -32,9 +32,9 @@ pub use slots::SlotInfo; use slots::Slots; pub use aux_schema::{check_equivocation, MAX_SLOT_CAPACITY, PRUNING_BOUND}; -use std::{fmt::Debug, ops::Deref, pin::Pin, sync::Arc, time::Duration}; +use std::{fmt::Debug, ops::Deref, sync::Arc, time::Duration}; use codec::{Decode, Encode}; -use futures::{prelude::*, future::{self, Either}}; +use futures::{future::Either, Future, TryFutureExt}; use futures_timer::Delay; use log::{debug, error, info, warn}; use parking_lot::Mutex; @@ -68,21 +68,23 @@ pub struct SlotResult { /// /// The implementation should not make any assumptions of the slot being bound to the time or /// similar. The only valid assumption is that the slot number is always increasing. +#[async_trait::async_trait] pub trait SlotWorker { /// Called when a new slot is triggered. /// /// Returns a future that resolves to a [`SlotResult`] iff a block was successfully built in /// the slot. Otherwise `None` is returned. - fn on_slot( + async fn on_slot( &mut self, chain_head: B::Header, slot_info: SlotInfo, - ) -> Pin>> + Send>>; + ) -> Option>; } /// A skeleton implementation for `SlotWorker` which tries to claim a slot at /// its beginning and tries to produce a block if successfully claimed, timing /// out if block production takes too long. +#[async_trait::async_trait] pub trait SimpleSlotWorker { /// A handle to a `BlockImport`. type BlockImport: BlockImport>::Transaction> @@ -96,7 +98,7 @@ pub trait SimpleSlotWorker { + Send + Unpin + 'static; /// The type of proposer to use to build blocks. - type Proposer: Proposer; + type Proposer: Proposer + Send; /// Data associated with a slot claim. type Claim: Send + 'static; @@ -191,36 +193,38 @@ pub trait SimpleSlotWorker { ) -> Duration; /// Implements [`SlotWorker::on_slot`]. - fn on_slot( + async fn on_slot( &mut self, chain_head: B::Header, slot_info: SlotInfo, - ) -> Pin>::Proof>>> + Send>> - where - >::Proposal: Unpin + Send + 'static, - { + ) -> Option>::Proof>> { let (timestamp, slot) = (slot_info.timestamp, slot_info.slot); let telemetry = self.telemetry(); + let logging_target = self.logging_target(); let proposing_remaining_duration = self.proposing_remaining_duration(&chain_head, &slot_info); let proposing_remaining = if proposing_remaining_duration == Duration::default() { debug!( - target: self.logging_target(), + target: logging_target, "Skipping proposal slot {} since there's no time left to propose", slot, ); - return Box::pin(future::ready(None)); + return None } else { - Box::new(Delay::new(proposing_remaining_duration)) - as Box + Unpin + Send> + Delay::new(proposing_remaining_duration) }; let epoch_data = match self.epoch_data(&chain_head, slot) { Ok(epoch_data) => epoch_data, Err(err) => { - warn!("Unable to fetch epoch data at block {:?}: {:?}", chain_head.hash(), err); + warn!( + target: logging_target, + "Unable to fetch epoch data at block {:?}: {:?}", + chain_head.hash(), + err, + ); telemetry!( telemetry; @@ -230,7 +234,7 @@ pub trait SimpleSlotWorker { "err" => ?err, ); - return Box::pin(future::ready(None)); + return None; } }; @@ -242,7 +246,7 @@ pub trait SimpleSlotWorker { self.sync_oracle().is_offline() && authorities_len.map(|a| a > 1).unwrap_or(false) { - debug!(target: self.logging_target(), "Skipping proposal slot. Waiting for the network."); + debug!(target: logging_target, "Skipping proposal slot. Waiting for the network."); telemetry!( telemetry; CONSENSUS_DEBUG; @@ -250,16 +254,16 @@ pub trait SimpleSlotWorker { "authorities_len" => authorities_len, ); - return Box::pin(future::ready(None)); + return None; } let claim = match self.claim_slot(&chain_head, slot, &epoch_data) { - None => return Box::pin(future::ready(None)), + None => return None, Some(claim) => claim, }; if self.should_backoff(slot, &chain_head) { - return Box::pin(future::ready(None)); + return None; } debug!( @@ -277,10 +281,15 @@ pub trait SimpleSlotWorker { "timestamp" => *timestamp, ); - let awaiting_proposer = { - let telemetry = telemetry.clone(); - self.proposer(&chain_head).map_err(move |err| { - warn!("Unable to author block in slot {:?}: {:?}", slot, err); + let proposer = match self.proposer(&chain_head).await { + Ok(p) => p, + Err(err) => { + warn!( + target: logging_target, + "Unable to author block in slot {:?}: {:?}", + slot, + err, + ); telemetry!( telemetry; @@ -290,8 +299,8 @@ pub trait SimpleSlotWorker { "err" => ?err ); - err - }) + return None + } }; let logs = self.pre_digest_data(slot, &claim); @@ -299,106 +308,127 @@ pub trait SimpleSlotWorker { // deadline our production to 98% of the total time left for proposing. As we deadline // the proposing below to the same total time left, the 2% margin should be enough for // the result to be returned. - let proposing = awaiting_proposer.and_then(move |proposer| proposer.propose( + let proposing = proposer.propose( slot_info.inherent_data, sp_runtime::generic::Digest { logs, }, proposing_remaining_duration.mul_f32(0.98), - ).map_err(|e| sp_consensus::Error::ClientImport(format!("{:?}", e)))); - - let proposal_work = { - let telemetry = telemetry.clone(); - futures::future::select(proposing, proposing_remaining).map(move |v| match v { - Either::Left((b, _)) => b.map(|b| (b, claim)), - Either::Right(_) => { - info!( - "⌛️ Discarding proposal for slot {}; block production took too long", - slot, - ); - // If the node was compiled with debug, tell the user to use release optimizations. - #[cfg(build_type="debug")] - info!("👉 Recompile your node in `--release` mode to mitigate this problem."); - telemetry!( - telemetry; - CONSENSUS_INFO; - "slots.discarding_proposal_took_too_long"; - "slot" => *slot, - ); + ).map_err(|e| sp_consensus::Error::ClientImport(format!("{:?}", e))); - Err(sp_consensus::Error::ClientImport("Timeout in the Slots proposer".into())) - }, - }) + let proposal = match futures::future::select(proposing, proposing_remaining).await { + Either::Left((Ok(p), _)) => p, + Either::Left((Err(err), _)) => { + warn!( + target: logging_target, + "Proposing failed: {:?}", + err, + ); + + return None + }, + Either::Right(_) => { + info!( + target: logging_target, + "⌛️ Discarding proposal for slot {}; block production took too long", + slot, + ); + // If the node was compiled with debug, tell the user to use release optimizations. + #[cfg(build_type="debug")] + info!( + target: logging_target, + "👉 Recompile your node in `--release` mode to mitigate this problem.", + ); + telemetry!( + telemetry; + CONSENSUS_INFO; + "slots.discarding_proposal_took_too_long"; + "slot" => *slot, + ); + + return None + }, }; let block_import_params_maker = self.block_import_params(); let block_import = self.block_import(); - let logging_target = self.logging_target(); - - proposal_work.and_then(move |(proposal, claim)| async move { - let (block, storage_proof) = (proposal.block, proposal.proof); - let (header, body) = block.deconstruct(); - let header_num = *header.number(); - let header_hash = header.hash(); - let parent_hash = *header.parent_hash(); - - let block_import_params = block_import_params_maker( - header, - &header_hash, - body.clone(), - proposal.storage_changes, - claim, - epoch_data, - )?; - - info!( - "🔖 Pre-sealed block for proposal at {}. Hash now {:?}, previously {:?}.", - header_num, - block_import_params.post_hash(), - header_hash, - ); - telemetry!( - telemetry; - CONSENSUS_INFO; - "slots.pre_sealed_block"; - "header_num" => ?header_num, - "hash_now" => ?block_import_params.post_hash(), - "hash_previously" => ?header_hash, - ); - - let header = block_import_params.post_header(); - if let Err(err) = block_import.lock().import_block(block_import_params, Default::default()) { + let (block, storage_proof) = (proposal.block, proposal.proof); + let (header, body) = block.deconstruct(); + let header_num = *header.number(); + let header_hash = header.hash(); + let parent_hash = *header.parent_hash(); + + let block_import_params = match block_import_params_maker( + header, + &header_hash, + body.clone(), + proposal.storage_changes, + claim, + epoch_data, + ) { + Ok(bi) => bi, + Err(err) => { warn!( target: logging_target, - "Error with block built on {:?}: {:?}", - parent_hash, + "Failed to create block import params: {:?}", err, ); - telemetry!( - telemetry; - CONSENSUS_WARN; - "slots.err_with_block_built_on"; - "hash" => ?parent_hash, - "err" => ?err, - ); + return None } + }; + + info!( + target: logging_target, + "🔖 Pre-sealed block for proposal at {}. Hash now {:?}, previously {:?}.", + header_num, + block_import_params.post_hash(), + header_hash, + ); - Ok(SlotResult { block: B::new(header, body), storage_proof }) - }).then(|r| async move { - r.map_err(|e| warn!(target: "slots", "Encountered consensus error: {:?}", e)).ok() - }).boxed() + telemetry!( + telemetry; + CONSENSUS_INFO; + "slots.pre_sealed_block"; + "header_num" => ?header_num, + "hash_now" => ?block_import_params.post_hash(), + "hash_previously" => ?header_hash, + ); + + let header = block_import_params.post_header(); + if let Err(err) = block_import + .lock() + .import_block(block_import_params, Default::default()) + { + warn!( + target: logging_target, + "Error with block built on {:?}: {:?}", + parent_hash, + err, + ); + + telemetry!( + telemetry; + CONSENSUS_WARN; + "slots.err_with_block_built_on"; + "hash" => ?parent_hash, + "err" => ?err, + ); + } + + Some(SlotResult { block: B::new(header, body), storage_proof }) } } -impl> SlotWorker>::Proof> for T { - fn on_slot( +#[async_trait::async_trait] +impl + Send> SlotWorker>::Proof> for T { + async fn on_slot( &mut self, chain_head: B::Header, slot_info: SlotInfo, - ) -> Pin>::Proof>>> + Send>> { - SimpleSlotWorker::on_slot(self, chain_head, slot_info) + ) -> Option>::Proof>> { + SimpleSlotWorker::on_slot(self, chain_head, slot_info).await } } @@ -436,25 +466,39 @@ where let SlotDuration(slot_duration) = slot_duration; // rather than use a timer interval, we schedule our waits ourselves - Slots::::new( + let mut slots = Slots::::new( slot_duration.slot_duration(), inherent_data_providers, timestamp_extractor, - ).inspect_err(|e| debug!(target: "slots", "Faulty timer: {:?}", e)) - .try_for_each(move |slot_info| { + ); + + async move { + loop { + let slot_info = match slots.next_slot().await { + Ok(slot) => slot, + Err(err) => { + debug!(target: "slots", "Faulty timer: {:?}", err); + return + }, + }; + // only propose when we are not syncing. if sync_oracle.is_major_syncing() { debug!(target: "slots", "Skipping proposal slot due to sync."); - return Either::Right(future::ready(Ok(()))); + continue; } let slot = slot_info.slot; let chain_head = match client.best_chain() { Ok(x) => x, Err(e) => { - warn!(target: "slots", "Unable to author block in slot {}. \ - no best block header: {:?}", slot, e); - return Either::Right(future::ready(Ok(()))); + warn!( + target: "slots", + "Unable to author block in slot {}. No best block header: {:?}", + slot, + e, + ); + continue; } }; @@ -466,19 +510,11 @@ where slot, err, ); - Either::Right(future::ready(Ok(()))) } else { - Either::Left( - worker.on_slot(chain_head, slot_info) - .then(|_| future::ready(Ok(()))) - ) - } - }).then(|res| { - if let Err(err) = res { - warn!(target: "slots", "Slots stream terminated with an error: {:?}", err); + worker.on_slot(chain_head, slot_info).await; } - future::ready(()) - }) + } + } } /// A header which has been checked diff --git a/client/consensus/slots/src/slots.rs b/client/consensus/slots/src/slots.rs index 1cf7c30b9ed9e..d7ed1eda64c09 100644 --- a/client/consensus/slots/src/slots.rs +++ b/client/consensus/slots/src/slots.rs @@ -22,10 +22,9 @@ use super::{SlotCompatible, Slot}; use sp_consensus::Error; -use futures::{prelude::*, task::Context, task::Poll}; use sp_inherents::{InherentData, InherentDataProviders}; -use std::{pin::Pin, time::{Duration, Instant}}; +use std::time::{Duration, Instant}; use futures_timer::Delay; /// Returns current duration since unix epoch. @@ -107,57 +106,49 @@ impl Slots { } } -impl Stream for Slots { - type Item = Result; - - fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context) -> Poll> { +impl Slots { + /// Returns a future that fires when the next slot starts. + pub async fn next_slot(&mut self) -> Result { loop { - let slot_duration = self.slot_duration; self.inner_delay = match self.inner_delay.take() { None => { // schedule wait. - let wait_dur = time_until_next(duration_now(), slot_duration); + let wait_dur = time_until_next(duration_now(), self.slot_duration); Some(Delay::new(wait_dur)) } Some(d) => Some(d), }; - if let Some(ref mut inner_delay) = self.inner_delay { - match Future::poll(Pin::new(inner_delay), cx) { - Poll::Pending => return Poll::Pending, - Poll::Ready(()) => {} - } + if let Some(inner_delay) = self.inner_delay.take() { + inner_delay.await; } - // timeout has fired. let inherent_data = match self.inherent_data_providers.create_inherent_data() { Ok(id) => id, - Err(err) => return Poll::Ready(Some(Err(sp_consensus::Error::InherentData(err)))), + Err(err) => return Err(sp_consensus::Error::InherentData(err)), }; let result = self.timestamp_extractor.extract_timestamp_and_slot(&inherent_data); let (timestamp, slot, offset) = match result { Ok(v) => v, - Err(err) => return Poll::Ready(Some(Err(err))), + Err(err) => return Err(err), }; // reschedule delay for next slot. let ends_in = offset + - time_until_next(timestamp.as_duration(), slot_duration); + time_until_next(timestamp.as_duration(), self.slot_duration); self.inner_delay = Some(Delay::new(ends_in)); // never yield the same slot twice. if slot > self.last_slot { self.last_slot = slot; - break Poll::Ready(Some(Ok(SlotInfo::new( + break Ok(SlotInfo::new( slot, timestamp, inherent_data, self.slot_duration, - )))) + )) } } } } - -impl Unpin for Slots {} From 4479a55ec779f5990ce48d28ca2a9ce08dfc6160 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 26 Mar 2021 14:15:47 +0100 Subject: [PATCH 10/27] Make grandpa tests compile --- Cargo.lock | 4 ++-- client/consensus/babe/Cargo.toml | 1 + client/consensus/babe/src/lib.rs | 5 +++-- client/consensus/epochs/Cargo.toml | 1 - client/consensus/epochs/src/lib.rs | 3 +-- client/consensus/slots/Cargo.toml | 1 - client/consensus/slots/src/lib.rs | 7 +++---- client/finality-grandpa/Cargo.toml | 1 + client/finality-grandpa/src/import.rs | 11 +++++++---- client/service/src/client/client.rs | 2 ++ 10 files changed, 20 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ed301508fefcf..fdebc52eb33f7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7157,6 +7157,7 @@ dependencies = [ name = "sc-consensus-babe" version = "0.9.0" dependencies = [ + "async-trait", "derive_more", "fork-tree", "futures 0.3.13", @@ -7243,7 +7244,6 @@ version = "0.9.0" dependencies = [ "fork-tree", "parity-scale-codec 2.0.1", - "parking_lot 0.11.1", "sc-client-api", "sc-consensus", "sp-blockchain", @@ -7320,7 +7320,6 @@ dependencies = [ "futures-timer 3.0.2", "log", "parity-scale-codec 2.0.1", - "parking_lot 0.11.1", "sc-client-api", "sc-telemetry", "sp-api", @@ -7444,6 +7443,7 @@ name = "sc-finality-grandpa" version = "0.9.0" dependencies = [ "assert_matches", + "async-trait", "derive_more", "dyn-clone", "finality-grandpa", diff --git a/client/consensus/babe/Cargo.toml b/client/consensus/babe/Cargo.toml index 14d48fba1bb57..4f171f386de12 100644 --- a/client/consensus/babe/Cargo.toml +++ b/client/consensus/babe/Cargo.toml @@ -53,6 +53,7 @@ merlin = "2.0" pdqselect = "0.1.0" derive_more = "0.99.2" retain_mut = "0.1.2" +async-trait = "0.1.42" [dev-dependencies] sp-keyring = { version = "3.0.0", path = "../../../primitives/keyring" } diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 1faac542652c0..48efe72dd5a87 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -77,7 +77,7 @@ pub use sp_consensus::SyncOracle; pub use sc_consensus_slots::SlotProportion; use std::{ collections::HashMap, sync::Arc, u64, pin::Pin, time::{Instant, Duration}, - any::Any, borrow::Cow, convert::TryInto, + borrow::Cow, convert::TryInto, }; use sp_consensus::{ImportResult, CanAuthorWith, import_queue::BoxJustificationImport}; use sp_core::crypto::Public; @@ -1084,6 +1084,7 @@ where } } +#[async_trait::async_trait] impl Verifier for BabeVerifier where @@ -1094,7 +1095,7 @@ where SelectChain: sp_consensus::SelectChain, CAW: CanAuthorWith + Send + Sync, { - fn verify( + async fn verify( &mut self, origin: BlockOrigin, header: Block::Header, diff --git a/client/consensus/epochs/Cargo.toml b/client/consensus/epochs/Cargo.toml index 4024b7848f9d4..8e2fe77100967 100644 --- a/client/consensus/epochs/Cargo.toml +++ b/client/consensus/epochs/Cargo.toml @@ -14,7 +14,6 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] codec = { package = "parity-scale-codec", version = "2.0.0", features = ["derive"] } -parking_lot = "0.11.1" fork-tree = { version = "3.0.0", path = "../../../utils/fork-tree" } sp-runtime = { path = "../../../primitives/runtime" , version = "3.0.0"} sp-blockchain = { version = "3.0.0", path = "../../../primitives/blockchain" } diff --git a/client/consensus/epochs/src/lib.rs b/client/consensus/epochs/src/lib.rs index d0c6cde3711e1..98a3e83530510 100644 --- a/client/consensus/epochs/src/lib.rs +++ b/client/consensus/epochs/src/lib.rs @@ -20,8 +20,7 @@ pub mod migration; -use std::{sync::Arc, ops::Add, collections::BTreeMap, borrow::{Borrow, BorrowMut}}; -use parking_lot::Mutex; +use std::{ops::Add, collections::BTreeMap, borrow::{Borrow, BorrowMut}}; use codec::{Encode, Decode}; use fork_tree::ForkTree; use sc_client_api::utils::is_descendent_of; diff --git a/client/consensus/slots/Cargo.toml b/client/consensus/slots/Cargo.toml index 623c4c4abd84c..64beea50fcf63 100644 --- a/client/consensus/slots/Cargo.toml +++ b/client/consensus/slots/Cargo.toml @@ -31,7 +31,6 @@ sp-inherents = { version = "3.0.0", path = "../../../primitives/inherents" } sp-timestamp = { version = "3.0.0", path = "../../../primitives/timestamp" } futures = "0.3.9" futures-timer = "3.0.1" -parking_lot = "0.11.1" log = "0.4.11" thiserror = "1.0.21" async-trait = "0.1.42" diff --git a/client/consensus/slots/src/lib.rs b/client/consensus/slots/src/lib.rs index dbf62ccad23c8..c1f13fea1f9ef 100644 --- a/client/consensus/slots/src/lib.rs +++ b/client/consensus/slots/src/lib.rs @@ -32,12 +32,11 @@ pub use slots::SlotInfo; use slots::Slots; pub use aux_schema::{check_equivocation, MAX_SLOT_CAPACITY, PRUNING_BOUND}; -use std::{fmt::Debug, ops::Deref, sync::Arc, time::Duration}; +use std::{fmt::Debug, ops::Deref, time::Duration}; use codec::{Decode, Encode}; use futures::{future::Either, Future, TryFutureExt}; use futures_timer::Delay; use log::{debug, error, info, warn}; -use parking_lot::Mutex; use sp_api::{ProvideRuntimeApi, ApiRef}; use sp_arithmetic::traits::BaseArithmetic; use sp_consensus::{BlockImport, Proposer, SyncOracle, SelectChain, CanAuthorWith, SlotData}; @@ -110,7 +109,7 @@ pub trait SimpleSlotWorker { fn logging_target(&self) -> &'static str; /// A handle to a `BlockImport`. - fn block_import(&self) -> Arc>; + fn block_import(&mut self) -> &mut Self::BlockImport; /// Returns the epoch data necessary for authoring. For time-dependent epochs, /// use the provided slot number as a canonical source of time. @@ -398,8 +397,8 @@ pub trait SimpleSlotWorker { let header = block_import_params.post_header(); if let Err(err) = block_import - .lock() .import_block(block_import_params, Default::default()) + .await { warn!( target: logging_target, diff --git a/client/finality-grandpa/Cargo.toml b/client/finality-grandpa/Cargo.toml index 7ae5666c7bc84..1f21f454491b3 100644 --- a/client/finality-grandpa/Cargo.toml +++ b/client/finality-grandpa/Cargo.toml @@ -47,6 +47,7 @@ sc-block-builder = { version = "0.9.0", path = "../block-builder" } finality-grandpa = { version = "0.14.0", features = ["derive-codec"] } pin-project = "1.0.4" linked-hash-map = "0.5.2" +async-trait = "0.1.42" [dev-dependencies] assert_matches = "1.3.0" diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index 9ceb3791e4078..b2fcca019bcb1 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -424,6 +424,7 @@ where } } +#[async_trait::async_trait] impl BlockImport for GrandpaBlockImport where NumberFor: finality_grandpa::BlockNumberOps, @@ -432,11 +433,13 @@ impl BlockImport Client: crate::ClientForGrandpa, for<'a> &'a Client: BlockImport>, + TransactionFor: Send + 'static, + SC: Send, { type Error = ConsensusError; type Transaction = TransactionFor; - fn import_block( + async fn import_block( &mut self, mut block: BlockImportParams, new_cache: HashMap>, @@ -459,7 +462,7 @@ impl BlockImport // we don't want to finalize on `inner.import_block` let mut justifications = block.justifications.take(); - let import_result = (&*self.inner).import_block(block, new_cache); + let import_result = (&*self.inner).import_block(block, new_cache).await; let mut imported_aux = { match import_result { @@ -563,11 +566,11 @@ impl BlockImport Ok(ImportResult::Imported(imported_aux)) } - fn check_block( + async fn check_block( &mut self, block: BlockCheckParams, ) -> Result { - self.inner.check_block(block) + self.inner.check_block(block).await } } diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index 66bb794f7e7ae..f975961c3b4e5 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -1707,6 +1707,7 @@ impl sp_consensus::BlockImport for &Client as ProvideRuntimeApi>::Api: CoreApi + ApiExt, RA: Sync + Send, + backend::TransactionFor: Send + 'static, { type Error = ConsensusError; type Transaction = backend::TransactionFor; @@ -1809,6 +1810,7 @@ impl sp_consensus::BlockImport for Client>::Api: CoreApi + ApiExt, RA: Sync + Send, + backend::TransactionFor: Send + 'static, { type Error = ConsensusError; type Transaction = backend::TransactionFor; From fcae728208861081acc52ca98a0f87fe0fb0aa58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 26 Mar 2021 00:01:28 +0100 Subject: [PATCH 11/27] More Babe tests work --- client/consensus/babe/src/aux_schema.rs | 4 +- client/consensus/babe/src/lib.rs | 7 ++- client/consensus/babe/src/tests.rs | 73 ++++++++++++++----------- client/network/test/src/lib.rs | 20 +++++-- 4 files changed, 63 insertions(+), 41 deletions(-) diff --git a/client/consensus/babe/src/aux_schema.rs b/client/consensus/babe/src/aux_schema.rs index 49b742cc6b256..8b8804e3bfb02 100644 --- a/client/consensus/babe/src/aux_schema.rs +++ b/client/consensus/babe/src/aux_schema.rs @@ -188,7 +188,7 @@ mod test { ).unwrap(); assert!( - epoch_changes.lock() + epoch_changes.shared_data() .tree() .iter() .map(|(_, _, epoch)| epoch.clone()) @@ -200,7 +200,7 @@ mod test { ); // PersistedEpochHeader does not implement Debug, so we use assert! directly. write_epoch_changes::( - &epoch_changes.lock(), + &epoch_changes.shared_data(), |values| { client.insert_aux(values, &[]).unwrap(); }, diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 48efe72dd5a87..df08e267fdfd0 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -76,8 +76,8 @@ pub use sp_consensus_babe::{ pub use sp_consensus::SyncOracle; pub use sc_consensus_slots::SlotProportion; use std::{ - collections::HashMap, sync::Arc, u64, pin::Pin, time::{Instant, Duration}, - borrow::Cow, convert::TryInto, + collections::HashMap, sync::Arc, u64, pin::Pin, borrow::Cow, convert::TryInto, + time::{Duration, Instant}, }; use sp_consensus::{ImportResult, CanAuthorWith, import_queue::BoxJustificationImport}; use sp_core::crypto::Public; @@ -1191,7 +1191,8 @@ where self.telemetry; CONSENSUS_TRACE; "babe.checked_and_importing"; - "pre_header" => ?pre_header); + "pre_header" => ?pre_header, + ); let mut import_block = BlockImportParams::new(origin, pre_header); import_block.post_digests.push(verified_info.seal); diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index 1dfc7a358839b..74d40864da3ca 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -47,6 +47,7 @@ use rand_chacha::{ }; use sc_keystore::LocalKeystore; use sp_application_crypto::key_types::BABE; +use futures::executor::block_on; type Item = DigestItem; @@ -68,7 +69,7 @@ enum Stage { type Mutator = Arc; type BabeBlockImport = - PanickingBlockImport>; + PanickingBlockImport>>; #[derive(Clone)] struct DummyFactory { @@ -137,7 +138,7 @@ impl DummyProposer { // figure out if we should add a consensus digest, since the test runtime // doesn't. - let epoch_changes = self.factory.epoch_changes.lock(); + let epoch_changes = self.factory.epoch_changes.shared_data(); let epoch = epoch_changes.epoch_data_for_child_of( descendent_query(&*self.factory.client), &self.parent_hash, @@ -191,25 +192,30 @@ thread_local! { } #[derive(Clone)] -struct PanickingBlockImport(B); - -impl> BlockImport for PanickingBlockImport { +pub struct PanickingBlockImport(B); + +#[async_trait::async_trait] +impl> BlockImport for PanickingBlockImport + where + B::Transaction: Send, + B: Send, +{ type Error = B::Error; type Transaction = B::Transaction; - fn import_block( + async fn import_block( &mut self, block: BlockImportParams, new_cache: HashMap>, ) -> Result { - Ok(self.0.import_block(block, new_cache).expect("importing block failed")) + Ok(self.0.import_block(block, new_cache).await.expect("importing block failed")) } - fn check_block( + async fn check_block( &mut self, block: BlockCheckParams, ) -> Result { - Ok(self.0.check_block(block).expect("checking block failed")) + Ok(self.0.check_block(block).await.expect("checking block failed")) } } @@ -227,16 +233,20 @@ type TestSelectChain = substrate_test_runtime_client::LongestChain< TestBlock, >; +#[derive(Clone)] pub struct TestVerifier { - inner: BabeVerifier, + inner: Arc< + futures::lock::Mutex> + >, mutator: Mutator, } +#[async_trait::async_trait] impl Verifier for TestVerifier { /// Verify the given data and return the BlockImportParams and an optional /// new set of validators to import. If not, err with an Error-Message /// presented to the User in the logs. - fn verify( + async fn verify( &mut self, origin: BlockOrigin, mut header: TestHeader, @@ -245,7 +255,7 @@ impl Verifier for TestVerifier { ) -> Result<(BlockImportParams, Option)>>), String> { // apply post-sealing mutations (i.e. stripping seal, if desired). (self.mutator)(&mut header, Stage::PostSeal); - self.inner.verify(origin, header, justifications, body) + self.inner.lock().await.verify(dbg!(origin), header, justifications, body).await } } @@ -260,6 +270,7 @@ pub struct PeerData { impl TestNetFactory for BabeTestNet { type Verifier = TestVerifier; type PeerData = Option; + type BlockImport = BabeBlockImport; /// Create new test network with peers and given config. fn from_config(_config: &ProtocolConfig) -> Self { @@ -269,9 +280,9 @@ impl TestNetFactory for BabeTestNet { } } - fn make_block_import(&self, client: PeersClient) + fn make_block_import(&self, client: PeersClient) -> ( - BlockImportAdapter, + BlockImportAdapter, Option>, Option, ) @@ -292,7 +303,7 @@ impl TestNetFactory for BabeTestNet { Some(Box::new(block_import.clone()) as BoxBlockImport<_, _>) ); ( - BlockImportAdapter::new_full(block_import), + BlockImportAdapter::new(block_import), None, Some(PeerData { link, inherent_data_providers, block_import: data_block_import }), ) @@ -317,7 +328,7 @@ impl TestNetFactory for BabeTestNet { let (_, longest_chain) = TestClientBuilder::new().build_with_longest_chain(); TestVerifier { - inner: BabeVerifier { + inner: Arc::new(futures::lock::Mutex::new(BabeVerifier { client: client.clone(), select_chain: longest_chain, inherent_data_providers: data.inherent_data_providers.clone(), @@ -326,7 +337,7 @@ impl TestNetFactory for BabeTestNet { time_source: data.link.time_source.clone(), can_author_with: AlwaysCanAuthor, telemetry: None, - }, + })), mutator: MUTATOR.with(|m| m.borrow().clone()), } } @@ -410,8 +421,8 @@ fn run_one_test( // run each future until we get one of our own blocks with number higher than 5 // that was produced locally. client.import_notification_stream() - .take_while(move |n| future::ready(n.header.number() < &5 || { - if n.origin == BlockOrigin::Own { + .take_while(move |n| future::ready(dbg!(n.header.number()) < &5 || { + if dbg!(n.origin) == BlockOrigin::Own { got_own = true; } else { got_other = true; @@ -441,7 +452,7 @@ fn run_one_test( telemetry: None, }).expect("Starts babe")); } - futures::executor::block_on(future::select( + block_on(future::select( futures::future::poll_fn(move |cx| { let mut net = net.lock(); net.poll(cx); @@ -572,7 +583,7 @@ fn can_author_block() { } // Propose and import a new BABE block on top of the given parent. -fn propose_and_import_block( +fn propose_and_import_block( parent: &TestHeader, slot: Option, proposer_factory: &mut DummyFactory, @@ -600,7 +611,7 @@ fn propose_and_import_block( let mut block = futures::executor::block_on(proposer.propose_with(pre_digest)).unwrap().block; - let epoch_descriptor = proposer_factory.epoch_changes.lock().epoch_descriptor_for_child_of( + let epoch_descriptor = proposer_factory.epoch_changes.shared_data().epoch_descriptor_for_child_of( descendent_query(&*proposer_factory.client), &parent_hash, *parent.number(), @@ -631,7 +642,7 @@ fn propose_and_import_block( Box::new(BabeIntermediate:: { epoch_descriptor }) as Box<_>, ); import.fork_choice = Some(ForkChoiceStrategy::LongestChain); - let import_result = block_import.import_block(import, Default::default()).unwrap(); + let import_result = block_on(block_import.import_block(import, Default::default())).unwrap(); match import_result { ImportResult::Imported(_) => {}, @@ -669,7 +680,7 @@ fn importing_block_one_sets_genesis_epoch() { let genesis_epoch = Epoch::genesis(&data.link.config, 999.into()); - let epoch_changes = data.link.epoch_changes.lock(); + let epoch_changes = data.link.epoch_changes.shared_data(); let epoch_for_second_block = epoch_changes.epoch_data_for_child_of( descendent_query(&*client), &block_hash, @@ -744,13 +755,13 @@ fn importing_epoch_change_block_prunes_tree() { // We should be tracking a total of 9 epochs in the fork tree assert_eq!( - epoch_changes.lock().tree().iter().count(), + epoch_changes.shared_data().tree().iter().count(), 9, ); // And only one root assert_eq!( - epoch_changes.lock().tree().roots().count(), + epoch_changes.shared_data().tree().roots().count(), 1, ); @@ -761,16 +772,16 @@ fn importing_epoch_change_block_prunes_tree() { // at this point no hashes from the first fork must exist on the tree assert!( - !epoch_changes.lock().tree().iter().map(|(h, _, _)| h).any(|h| fork_1.contains(h)), + !epoch_changes.shared_data().tree().iter().map(|(h, _, _)| h).any(|h| fork_1.contains(h)), ); // but the epoch changes from the other forks must still exist assert!( - epoch_changes.lock().tree().iter().map(|(h, _, _)| h).any(|h| fork_2.contains(h)) + epoch_changes.shared_data().tree().iter().map(|(h, _, _)| h).any(|h| fork_2.contains(h)) ); assert!( - epoch_changes.lock().tree().iter().map(|(h, _, _)| h).any(|h| fork_3.contains(h)), + epoch_changes.shared_data().tree().iter().map(|(h, _, _)| h).any(|h| fork_3.contains(h)), ); // finalizing block #25 from the canon chain should prune out the second fork @@ -779,12 +790,12 @@ fn importing_epoch_change_block_prunes_tree() { // at this point no hashes from the second fork must exist on the tree assert!( - !epoch_changes.lock().tree().iter().map(|(h, _, _)| h).any(|h| fork_2.contains(h)), + !epoch_changes.shared_data().tree().iter().map(|(h, _, _)| h).any(|h| fork_2.contains(h)), ); // while epoch changes from the last fork should still exist assert!( - epoch_changes.lock().tree().iter().map(|(h, _, _)| h).any(|h| fork_3.contains(h)), + epoch_changes.shared_data().tree().iter().map(|(h, _, _)| h).any(|h| fork_3.contains(h)), ); } diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index 00e13a2646180..1b9a54d2a9c03 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -23,8 +23,7 @@ mod block_import; mod sync; use std::{ - borrow::Cow, collections::HashMap, pin::Pin, sync::Arc, marker::PhantomData, - task::{Poll, Context as FutureContext} + borrow::Cow, collections::HashMap, pin::Pin, sync::Arc, task::{Poll, Context as FutureContext} }; use libp2p::build_multiaddr; @@ -64,7 +63,7 @@ use sc_network::config::ProtocolConfig; use sp_runtime::generic::{BlockId, OpaqueDigestItemId}; use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor}; use sp_runtime::{Justification, Justifications}; -use substrate_test_runtime_client::{self, AccountKeyring}; +use substrate_test_runtime_client::AccountKeyring; use sc_service::client::Client; pub use sc_network::config::EmptyTransactionPool; pub use substrate_test_runtime_client::runtime::{Block, Extrinsic, Hash, Transfer}; @@ -608,6 +607,15 @@ impl> Verifier for VerifierAdapter { } } +impl Clone for VerifierAdapter where Verifier: Clone { + fn clone(&self) -> Self { + Self { + verifier: self.verifier.clone(), + failed_verifications: self.failed_verifications.clone(), + } + } +} + impl VerifierAdapter { fn new(verifier: Verifier) -> Self { VerifierAdapter { @@ -706,6 +714,7 @@ pub trait TestNetFactory: Sized where >: &Default::default(), &data, ); + let verifier = VerifierAdapter::new(verifier); let import_queue = Box::new(BasicQueue::new( verifier.clone(), @@ -799,7 +808,7 @@ pub trait TestNetFactory: Sized where >: imported_blocks_stream, finality_notification_stream, block_import, - verifier: VerifierAdapter::new(verifier.clone()), + verifier, network, listen_addr, }); @@ -821,6 +830,7 @@ pub trait TestNetFactory: Sized where >: &Default::default(), &data, ); + let verifier = VerifierAdapter::new(verifier); let import_queue = Box::new(BasicQueue::new( verifier.clone(), @@ -877,7 +887,7 @@ pub trait TestNetFactory: Sized where >: peers.push(Peer { data, - verifier: VerifierAdapter::new(verifier.clone()), + verifier, select_chain: None, backend: None, block_import, From a850851f7f780bf26f45a3efac9b9fc3eff3288e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 26 Mar 2021 21:55:25 +0100 Subject: [PATCH 12/27] Fix network test --- client/network/test/src/block_import.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/client/network/test/src/block_import.rs b/client/network/test/src/block_import.rs index 200c7357c4244..b3641d4b41214 100644 --- a/client/network/test/src/block_import.rs +++ b/client/network/test/src/block_import.rs @@ -26,12 +26,13 @@ use substrate_test_runtime_client::{self, prelude::*}; use substrate_test_runtime_client::runtime::{Block, Hash}; use sp_runtime::generic::BlockId; use sc_block_builder::BlockBuilderProvider; +use futures::executor::block_on; use super::*; fn prepare_good_block() -> (TestClient, Hash, u64, PeerId, IncomingBlock) { let mut client = substrate_test_runtime_client::new(); let block = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::File, block).unwrap(); + block_on(client.import(BlockOrigin::File, block)).unwrap(); let (hash, number) = (client.block_hash(1).unwrap().unwrap(), 1); let header = client.header(&BlockId::Number(1)).unwrap(); @@ -55,12 +56,12 @@ fn import_single_good_block_works() { let mut expected_aux = ImportedAux::default(); expected_aux.is_new_best = true; - match import_single_block( + match block_on(import_single_block( &mut substrate_test_runtime_client::new(), BlockOrigin::File, block, &mut PassThroughVerifier::new(true) - ) { + )) { Ok(BlockImportResult::ImportedUnknown(ref num, ref aux, ref org)) if *num == number && *aux == expected_aux && *org == Some(peer_id) => {} r @ _ => panic!("{:?}", r) @@ -70,12 +71,12 @@ fn import_single_good_block_works() { #[test] fn import_single_good_known_block_is_ignored() { let (mut client, _hash, number, _, block) = prepare_good_block(); - match import_single_block( + match block_on(import_single_block( &mut client, BlockOrigin::File, block, &mut PassThroughVerifier::new(true) - ) { + )) { Ok(BlockImportResult::ImportedKnown(ref n, _)) if *n == number => {} _ => panic!() } @@ -85,12 +86,12 @@ fn import_single_good_known_block_is_ignored() { fn import_single_good_block_without_header_fails() { let (_, _, _, peer_id, mut block) = prepare_good_block(); block.header = None; - match import_single_block( + match block_on(import_single_block( &mut substrate_test_runtime_client::new(), BlockOrigin::File, block, &mut PassThroughVerifier::new(true) - ) { + )) { Err(BlockImportError::IncompleteHeader(ref org)) if *org == Some(peer_id) => {} _ => panic!() } From 03013782a1cef4a961c1a6fda7fa953614f43823 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sat, 27 Mar 2021 00:01:44 +0100 Subject: [PATCH 13/27] Start fixing service test --- client/service/test/src/client/mod.rs | 51 ++++++++++++++------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/client/service/test/src/client/mod.rs b/client/service/test/src/client/mod.rs index d8a09734bebb6..7a2ef102b3496 100644 --- a/client/service/test/src/client/mod.rs +++ b/client/service/test/src/client/mod.rs @@ -54,6 +54,7 @@ use sp_storage::StorageKey; use sp_trie::{TrieConfiguration, trie_types::Layout}; use sp_runtime::{generic::BlockId, DigestItem, Justifications}; use hex_literal::hex; +use futures::executor::block_on; mod light; mod db; @@ -363,7 +364,7 @@ fn block_builder_works_with_no_transactions() { let block = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); assert_eq!(client.chain_info().best_number, 1); } @@ -382,7 +383,7 @@ fn block_builder_works_with_transactions() { }).unwrap(); let block = builder.build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); assert_eq!(client.chain_info().best_number, 1); assert_ne!( @@ -428,7 +429,7 @@ fn block_builder_does_not_include_invalid() { ); let block = builder.build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); assert_eq!(client.chain_info().best_number, 1); assert_ne!( @@ -476,7 +477,7 @@ fn uncles_with_only_ancestors() { // G -> A1 let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a1.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap(); // A1 -> A2 let a2 = client.new_block(Default::default()).unwrap().build().unwrap().block; @@ -496,7 +497,7 @@ fn uncles_with_multiple_forks() { // G -> A1 let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a1.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap(); // A1 -> A2 let a2 = client.new_block_at( @@ -504,7 +505,7 @@ fn uncles_with_multiple_forks() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a2.clone())).unwrap(); // A2 -> A3 let a3 = client.new_block_at( @@ -512,7 +513,7 @@ fn uncles_with_multiple_forks() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a3.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a3.clone())).unwrap(); // A3 -> A4 let a4 = client.new_block_at( @@ -520,7 +521,7 @@ fn uncles_with_multiple_forks() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a4.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a4.clone())).unwrap(); // A4 -> A5 let a5 = client.new_block_at( @@ -528,7 +529,7 @@ fn uncles_with_multiple_forks() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a5.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a5.clone())).unwrap(); // A1 -> B2 let mut builder = client.new_block_at( @@ -544,7 +545,7 @@ fn uncles_with_multiple_forks() { nonce: 0, }).unwrap(); let b2 = builder.build().unwrap().block; - client.import(BlockOrigin::Own, b2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b2.clone())).unwrap(); // B2 -> B3 let b3 = client.new_block_at( @@ -552,7 +553,7 @@ fn uncles_with_multiple_forks() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, b3.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b3.clone())).unwrap(); // B3 -> B4 let b4 = client.new_block_at( @@ -560,7 +561,7 @@ fn uncles_with_multiple_forks() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, b4.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b4.clone())).unwrap(); // // B2 -> C3 let mut builder = client.new_block_at( @@ -576,7 +577,7 @@ fn uncles_with_multiple_forks() { nonce: 1, }).unwrap(); let c3 = builder.build().unwrap().block; - client.import(BlockOrigin::Own, c3.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, c3.clone())).unwrap(); // A1 -> D2 let mut builder = client.new_block_at( @@ -592,7 +593,7 @@ fn uncles_with_multiple_forks() { nonce: 0, }).unwrap(); let d2 = builder.build().unwrap().block; - client.import(BlockOrigin::Own, d2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, d2.clone())).unwrap(); let genesis_hash = client.chain_info().genesis_hash; @@ -624,11 +625,11 @@ fn best_containing_on_longest_chain_with_single_chain_3_blocks() { // G -> A1 let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a1.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap(); // A1 -> A2 let a2 = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a2.clone())).unwrap(); let genesis_hash = client.chain_info().genesis_hash; @@ -656,7 +657,7 @@ fn best_containing_on_longest_chain_with_multiple_forks() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a2.clone())).unwrap(); // A2 -> A3 let a3 = client.new_block_at( @@ -664,7 +665,7 @@ fn best_containing_on_longest_chain_with_multiple_forks() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a3.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a3.clone())).unwrap(); // A3 -> A4 let a4 = client.new_block_at( @@ -672,7 +673,7 @@ fn best_containing_on_longest_chain_with_multiple_forks() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a4.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a4.clone())).unwrap(); // A4 -> A5 let a5 = client.new_block_at( @@ -680,7 +681,7 @@ fn best_containing_on_longest_chain_with_multiple_forks() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a5.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a5.clone())).unwrap(); // A1 -> B2 let mut builder = client.new_block_at( @@ -696,7 +697,7 @@ fn best_containing_on_longest_chain_with_multiple_forks() { nonce: 0, }).unwrap(); let b2 = builder.build().unwrap().block; - client.import(BlockOrigin::Own, b2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b2.clone())).unwrap(); // B2 -> B3 let b3 = client.new_block_at( @@ -704,7 +705,7 @@ fn best_containing_on_longest_chain_with_multiple_forks() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, b3.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b3.clone())).unwrap(); // B3 -> B4 let b4 = client.new_block_at( @@ -712,7 +713,7 @@ fn best_containing_on_longest_chain_with_multiple_forks() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, b4.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b4.clone())).unwrap(); // // B2 -> C3 let mut builder = client.new_block_at( @@ -728,7 +729,7 @@ fn best_containing_on_longest_chain_with_multiple_forks() { nonce: 1, }).unwrap(); let c3 = builder.build().unwrap().block; - client.import(BlockOrigin::Own, c3.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, c3.clone())).unwrap(); // A1 -> D2 let mut builder = client.new_block_at( @@ -744,7 +745,7 @@ fn best_containing_on_longest_chain_with_multiple_forks() { nonce: 0, }).unwrap(); let d2 = builder.build().unwrap().block; - client.import(BlockOrigin::Own, d2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, d2.clone())).unwrap(); assert_eq!(client.chain_info().best_hash, a5.hash()); From 30954040a11a9a55d4a61ef998493bd301b1336a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sat, 27 Mar 2021 00:44:40 +0100 Subject: [PATCH 14/27] Finish service-test --- client/service/test/Cargo.toml | 2 +- client/service/test/src/client/light.rs | 6 +- client/service/test/src/client/mod.rs | 222 +++++++++++++++--------- 3 files changed, 148 insertions(+), 82 deletions(-) diff --git a/client/service/test/Cargo.toml b/client/service/test/Cargo.toml index e55320d6c5fb7..2108d7e26fa83 100644 --- a/client/service/test/Cargo.toml +++ b/client/service/test/Cargo.toml @@ -28,7 +28,7 @@ sp-trie = { version = "3.0.0", path = "../../../primitives/trie" } sp-storage = { version = "3.0.0", path = "../../../primitives/storage" } sc-client-db = { version = "0.9.0", default-features = false, path = "../../db" } futures = { version = "0.3.1", features = ["compat"] } -sc-service = { version = "0.9.0", default-features = false, features = ["test-helpers"], path = "../../service" } +sc-service = { version = "0.9.0", features = ["test-helpers"], path = "../../service" } sc-network = { version = "0.9.0", path = "../../network" } sp-consensus = { version = "0.9.0", path = "../../../primitives/consensus/common" } sp-runtime = { version = "3.0.0", path = "../../../primitives/runtime" } diff --git a/client/service/test/src/client/light.rs b/client/service/test/src/client/light.rs index 02d54a24c3135..a183cbce62bdb 100644 --- a/client/service/test/src/client/light.rs +++ b/client/service/test/src/client/light.rs @@ -375,11 +375,11 @@ fn execution_proof_is_generated_and_checked() { for i in 1u32..3u32 { let mut digest = Digest::default(); digest.push(sp_runtime::generic::DigestItem::Other::(i.to_le_bytes().to_vec())); - remote_client.import_justified( + futures::executor::block_on(remote_client.import_justified( BlockOrigin::Own, remote_client.new_block(digest).unwrap().build().unwrap().block, Justifications::from((*b"TEST", Default::default())), - ).unwrap(); + )).unwrap(); } // check method that doesn't requires environment @@ -540,7 +540,7 @@ fn prepare_for_header_proof_check(insert_cht: bool) -> (TestChecker, Hash, Heade let mut local_headers_hashes = Vec::new(); for i in 0..4 { let block = remote_client.new_block(Default::default()).unwrap().build().unwrap().block; - remote_client.import(BlockOrigin::Own, block).unwrap(); + futures::executor::block_on(remote_client.import(BlockOrigin::Own, block)).unwrap(); local_headers_hashes.push( remote_client.block_hash(i + 1) .map_err(|_| ClientError::Backend("TestError".into())) diff --git a/client/service/test/src/client/mod.rs b/client/service/test/src/client/mod.rs index 7a2ef102b3496..0234f43513d56 100644 --- a/client/service/test/src/client/mod.rs +++ b/client/service/test/src/client/mod.rs @@ -109,7 +109,7 @@ pub fn prepare_client_with_key_changes() -> ( }).unwrap(); } let block = builder.build().unwrap().block; - remote_client.import(BlockOrigin::Own, block).unwrap(); + block_on(remote_client.import(BlockOrigin::Own, block)).unwrap(); let header = remote_client.header(&BlockId::Number(i as u64 + 1)).unwrap().unwrap(); let trie_root = header.digest().log(DigestItem::as_changes_trie_root) @@ -481,7 +481,7 @@ fn uncles_with_only_ancestors() { // A1 -> A2 let a2 = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a2.clone())).unwrap(); let v: Vec = Vec::new(); assert_eq!(v, client.uncles(a2.hash(), 3).unwrap()); } @@ -649,7 +649,7 @@ fn best_containing_on_longest_chain_with_multiple_forks() { // G -> A1 let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a1.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap(); // A1 -> A2 let a2 = client.new_block_at( @@ -953,11 +953,15 @@ fn best_containing_on_longest_chain_with_multiple_forks() { assert_eq!(None, longest_chain_select.finality_target( b4.hash(), Some(0)).unwrap()); - assert_eq!(None, longest_chain_select.finality_target( - c3.hash().clone(), Some(0)).unwrap()); + assert_eq!( + None, + longest_chain_select.finality_target(c3.hash().clone(), Some(0)).unwrap(), + ); - assert_eq!(None, longest_chain_select.finality_target( - d2.hash().clone(), Some(0)).unwrap()); + assert_eq!( + None, + longest_chain_select.finality_target(d2.hash().clone(), Some(0)).unwrap(), + ); } #[test] @@ -969,15 +973,18 @@ fn best_containing_on_longest_chain_with_max_depth_higher_than_best() { // G -> A1 let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a1.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap(); // A1 -> A2 let a2 = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a2.clone())).unwrap(); let genesis_hash = client.chain_info().genesis_hash; - assert_eq!(a2.hash(), longest_chain_select.finality_target(genesis_hash, Some(10)).unwrap().unwrap()); + assert_eq!( + a2.hash(), + longest_chain_select.finality_target(genesis_hash, Some(10)).unwrap().unwrap(), + ); } #[test] @@ -1009,7 +1016,7 @@ fn import_with_justification() { // G -> A1 let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a1.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap(); // A1 -> A2 let a2 = client.new_block_at( @@ -1017,7 +1024,7 @@ fn import_with_justification() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a2.clone())).unwrap(); // A2 -> A3 let justification = Justifications::from((TEST_ENGINE_ID, vec![1, 2, 3])); @@ -1026,7 +1033,7 @@ fn import_with_justification() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import_justified(BlockOrigin::Own, a3.clone(), justification.clone()).unwrap(); + block_on(client.import_justified(BlockOrigin::Own, a3.clone(), justification.clone())).unwrap(); assert_eq!( client.chain_info().finalized_hash, @@ -1061,14 +1068,14 @@ fn importing_diverged_finalized_block_should_trigger_reorg() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a1.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap(); let a2 = client.new_block_at( &BlockId::Hash(a1.hash()), Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a2.clone())).unwrap(); let mut b1 = client.new_block_at( &BlockId::Number(0), @@ -1093,7 +1100,7 @@ fn importing_diverged_finalized_block_should_trigger_reorg() { // importing B1 as finalized should trigger a re-org and set it as new best let justification = Justifications::from((TEST_ENGINE_ID, vec![1, 2, 3])); - client.import_justified(BlockOrigin::Own, b1.clone(), justification).unwrap(); + block_on(client.import_justified(BlockOrigin::Own, b1.clone(), justification)).unwrap(); assert_eq!( client.chain_info().best_hash, @@ -1118,14 +1125,14 @@ fn finalizing_diverged_block_should_trigger_reorg() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a1.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap(); let a2 = client.new_block_at( &BlockId::Hash(a1.hash()), Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a2.clone())).unwrap(); let mut b1 = client.new_block_at( &BlockId::Number(0), @@ -1140,14 +1147,14 @@ fn finalizing_diverged_block_should_trigger_reorg() { nonce: 0, }).unwrap(); let b1 = b1.build().unwrap().block; - client.import(BlockOrigin::Own, b1.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b1.clone())).unwrap(); let b2 = client.new_block_at( &BlockId::Hash(b1.hash()), Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, b2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b2.clone())).unwrap(); // A2 is the current best since it's the longest chain assert_eq!( @@ -1185,7 +1192,7 @@ fn finalizing_diverged_block_should_trigger_reorg() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, b3.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b3.clone())).unwrap(); assert_eq!( client.chain_info().best_hash, @@ -1228,7 +1235,7 @@ fn state_reverted_on_reorg() { nonce: 0, }).unwrap(); let a1 = a1.build().unwrap().block; - client.import(BlockOrigin::Own, a1.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap(); let mut b1 = client.new_block_at( &BlockId::Number(0), @@ -1243,7 +1250,7 @@ fn state_reverted_on_reorg() { }).unwrap(); let b1 = b1.build().unwrap().block; // Reorg to B1 - client.import_as_best(BlockOrigin::Own, b1.clone()).unwrap(); + block_on(client.import_as_best(BlockOrigin::Own, b1.clone())).unwrap(); assert_eq!(950, current_balance(&client)); let mut a2 = client.new_block_at( @@ -1259,7 +1266,7 @@ fn state_reverted_on_reorg() { }).unwrap(); let a2 = a2.build().unwrap().block; // Re-org to A2 - client.import_as_best(BlockOrigin::Own, a2).unwrap(); + block_on(client.import_as_best(BlockOrigin::Own, a2)).unwrap(); assert_eq!(980, current_balance(&client)); } @@ -1298,14 +1305,14 @@ fn doesnt_import_blocks_that_revert_finality() { Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a1.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap(); let a2 = client.new_block_at( &BlockId::Hash(a1.hash()), Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, a2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, a2.clone())).unwrap(); let mut b1 = client.new_block_at(&BlockId::Number(0), Default::default(), false).unwrap(); @@ -1317,11 +1324,11 @@ fn doesnt_import_blocks_that_revert_finality() { nonce: 0, }).unwrap(); let b1 = b1.build().unwrap().block; - client.import(BlockOrigin::Own, b1.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b1.clone())).unwrap(); let b2 = client.new_block_at(&BlockId::Hash(b1.hash()), Default::default(), false) .unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, b2.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, b2.clone())).unwrap(); // prepare B3 before we finalize A2, because otherwise we won't be able to // read changes trie configuration after A2 is finalized @@ -1332,7 +1339,7 @@ fn doesnt_import_blocks_that_revert_finality() { // B3 at the same height but that doesn't include it ClientExt::finalize_block(&client, BlockId::Hash(a2.hash()), None).unwrap(); - let import_err = client.import(BlockOrigin::Own, b3).err().unwrap(); + let import_err = block_on(client.import(BlockOrigin::Own, b3)).err().unwrap(); let expected_err = ConsensusError::ClientImport( sp_blockchain::Error::RuntimeApiError( sp_api::ApiError::Application(Box::new(sp_blockchain::Error::NotInFinalizedChain)) @@ -1357,7 +1364,7 @@ fn doesnt_import_blocks_that_revert_finality() { }).unwrap(); let c1 = c1.build().unwrap().block; - let import_err = client.import(BlockOrigin::Own, c1).err().unwrap(); + let import_err = block_on(client.import(BlockOrigin::Own, c1)).err().unwrap(); let expected_err = ConsensusError::ClientImport( sp_blockchain::Error::NotInFinalizedChain.to_string() ); @@ -1368,7 +1375,6 @@ fn doesnt_import_blocks_that_revert_finality() { ); } - #[test] fn respects_block_rules() { fn run_test( @@ -1397,7 +1403,7 @@ fn respects_block_rules() { allow_missing_state: false, import_existing: false, }; - assert_eq!(client.check_block(params).unwrap(), ImportResult::imported(false)); + assert_eq!(block_on(client.check_block(params)).unwrap(), ImportResult::imported(false)); // this is 0x0d6d6612a10485370d9e085aeea7ec427fb3f34d961c6a816cdbe5cde2278864 let mut block_not_ok = client.new_block_at(&BlockId::Number(0), Default::default(), false) @@ -1415,11 +1421,11 @@ fn respects_block_rules() { if record_only { known_bad.insert(block_not_ok.hash()); } else { - assert_eq!(client.check_block(params).unwrap(), ImportResult::KnownBad); + assert_eq!(block_on(client.check_block(params)).unwrap(), ImportResult::KnownBad); } // Now going to the fork - client.import_as_final(BlockOrigin::Own, block_ok).unwrap(); + block_on(client.import_as_final(BlockOrigin::Own, block_ok)).unwrap(); // And check good fork let mut block_ok = client.new_block_at(&BlockId::Number(1), Default::default(), false) @@ -1437,7 +1443,7 @@ fn respects_block_rules() { if record_only { fork_rules.push((1, block_ok.hash().clone())); } - assert_eq!(client.check_block(params).unwrap(), ImportResult::imported(false)); + assert_eq!(block_on(client.check_block(params)).unwrap(), ImportResult::imported(false)); // And now try bad fork let mut block_not_ok = client.new_block_at(&BlockId::Number(1), Default::default(), false) @@ -1454,7 +1460,7 @@ fn respects_block_rules() { }; if !record_only { - assert_eq!(client.check_block(params).unwrap(), ImportResult::KnownBad); + assert_eq!(block_on(client.check_block(params)).unwrap(), ImportResult::KnownBad); } } @@ -1492,8 +1498,11 @@ fn returns_status_for_pruned_blocks() { let mut client = TestClientBuilder::with_backend(backend).build(); - let a1 = client.new_block_at(&BlockId::Number(0), Default::default(), false) - .unwrap().build().unwrap().block; + let a1 = client.new_block_at( + &BlockId::Number(0), + Default::default(), + false, + ).unwrap().build().unwrap().block; let mut b1 = client.new_block_at(&BlockId::Number(0), Default::default(), false).unwrap(); @@ -1514,17 +1523,32 @@ fn returns_status_for_pruned_blocks() { import_existing: false, }; - assert_eq!(client.check_block(check_block_a1.clone()).unwrap(), ImportResult::imported(false)); - assert_eq!(client.block_status(&BlockId::hash(check_block_a1.hash)).unwrap(), BlockStatus::Unknown); + assert_eq!( + block_on(client.check_block(check_block_a1.clone())).unwrap(), + ImportResult::imported(false), + ); + assert_eq!( + client.block_status(&BlockId::hash(check_block_a1.hash)).unwrap(), + BlockStatus::Unknown, + ); - client.import_as_final(BlockOrigin::Own, a1.clone()).unwrap(); + block_on(client.import_as_final(BlockOrigin::Own, a1.clone())).unwrap(); - assert_eq!(client.check_block(check_block_a1.clone()).unwrap(), ImportResult::AlreadyInChain); - assert_eq!(client.block_status(&BlockId::hash(check_block_a1.hash)).unwrap(), BlockStatus::InChainWithState); + assert_eq!( + block_on(client.check_block(check_block_a1.clone())).unwrap(), + ImportResult::AlreadyInChain, + ); + assert_eq!( + client.block_status(&BlockId::hash(check_block_a1.hash)).unwrap(), + BlockStatus::InChainWithState, + ); - let a2 = client.new_block_at(&BlockId::Hash(a1.hash()), Default::default(), false) - .unwrap().build().unwrap().block; - client.import_as_final(BlockOrigin::Own, a2.clone()).unwrap(); + let a2 = client.new_block_at( + &BlockId::Hash(a1.hash()), + Default::default(), + false, + ).unwrap().build().unwrap().block; + block_on(client.import_as_final(BlockOrigin::Own, a2.clone())).unwrap(); let check_block_a2 = BlockCheckParams { hash: a2.hash().clone(), @@ -1534,15 +1558,30 @@ fn returns_status_for_pruned_blocks() { import_existing: false, }; - assert_eq!(client.check_block(check_block_a1.clone()).unwrap(), ImportResult::AlreadyInChain); - assert_eq!(client.block_status(&BlockId::hash(check_block_a1.hash)).unwrap(), BlockStatus::InChainPruned); - assert_eq!(client.check_block(check_block_a2.clone()).unwrap(), ImportResult::AlreadyInChain); - assert_eq!(client.block_status(&BlockId::hash(check_block_a2.hash)).unwrap(), BlockStatus::InChainWithState); + assert_eq!( + block_on(client.check_block(check_block_a1.clone())).unwrap(), + ImportResult::AlreadyInChain, + ); + assert_eq!( + client.block_status(&BlockId::hash(check_block_a1.hash)).unwrap(), + BlockStatus::InChainPruned, + ); + assert_eq!( + block_on(client.check_block(check_block_a2.clone())).unwrap(), + ImportResult::AlreadyInChain, + ); + assert_eq!( + client.block_status(&BlockId::hash(check_block_a2.hash)).unwrap(), + BlockStatus::InChainWithState, + ); - let a3 = client.new_block_at(&BlockId::Hash(a2.hash()), Default::default(), false) - .unwrap().build().unwrap().block; + let a3 = client.new_block_at( + &BlockId::Hash(a2.hash()), + Default::default(), + false, + ).unwrap().build().unwrap().block; - client.import_as_final(BlockOrigin::Own, a3.clone()).unwrap(); + block_on(client.import_as_final(BlockOrigin::Own, a3.clone())).unwrap(); let check_block_a3 = BlockCheckParams { hash: a3.hash().clone(), number: 2, @@ -1552,12 +1591,30 @@ fn returns_status_for_pruned_blocks() { }; // a1 and a2 are both pruned at this point - assert_eq!(client.check_block(check_block_a1.clone()).unwrap(), ImportResult::AlreadyInChain); - assert_eq!(client.block_status(&BlockId::hash(check_block_a1.hash)).unwrap(), BlockStatus::InChainPruned); - assert_eq!(client.check_block(check_block_a2.clone()).unwrap(), ImportResult::AlreadyInChain); - assert_eq!(client.block_status(&BlockId::hash(check_block_a2.hash)).unwrap(), BlockStatus::InChainPruned); - assert_eq!(client.check_block(check_block_a3.clone()).unwrap(), ImportResult::AlreadyInChain); - assert_eq!(client.block_status(&BlockId::hash(check_block_a3.hash)).unwrap(), BlockStatus::InChainWithState); + assert_eq!( + block_on(client.check_block(check_block_a1.clone())).unwrap(), + ImportResult::AlreadyInChain, + ); + assert_eq!( + client.block_status(&BlockId::hash(check_block_a1.hash)).unwrap(), + BlockStatus::InChainPruned, + ); + assert_eq!( + block_on(client.check_block(check_block_a2.clone())).unwrap(), + ImportResult::AlreadyInChain, + ); + assert_eq!( + client.block_status(&BlockId::hash(check_block_a2.hash)).unwrap(), + BlockStatus::InChainPruned, + ); + assert_eq!( + block_on(client.check_block(check_block_a3.clone())).unwrap(), + ImportResult::AlreadyInChain, + ); + assert_eq!( + client.block_status(&BlockId::hash(check_block_a3.hash)).unwrap(), + BlockStatus::InChainWithState, + ); let mut check_block_b1 = BlockCheckParams { hash: b1.hash().clone(), @@ -1566,11 +1623,20 @@ fn returns_status_for_pruned_blocks() { allow_missing_state: false, import_existing: false, }; - assert_eq!(client.check_block(check_block_b1.clone()).unwrap(), ImportResult::MissingState); + assert_eq!( + block_on(client.check_block(check_block_b1.clone())).unwrap(), + ImportResult::MissingState, + ); check_block_b1.allow_missing_state = true; - assert_eq!(client.check_block(check_block_b1.clone()).unwrap(), ImportResult::imported(false)); + assert_eq!( + block_on(client.check_block(check_block_b1.clone())).unwrap(), + ImportResult::imported(false), + ); check_block_b1.parent_hash = H256::random(); - assert_eq!(client.check_block(check_block_b1.clone()).unwrap(), ImportResult::UnknownParent); + assert_eq!( + block_on(client.check_block(check_block_b1.clone())).unwrap(), + ImportResult::UnknownParent, + ); } #[test] @@ -1601,18 +1667,18 @@ fn imports_blocks_with_changes_tries_config_change() { (1..11).for_each(|number| { let block = client.new_block_at(&BlockId::Number(number - 1), Default::default(), false) .unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); }); (11..12).for_each(|number| { let mut block = client.new_block_at(&BlockId::Number(number - 1), Default::default(), false).unwrap(); block.push_storage_change(vec![42], Some(number.to_le_bytes().to_vec())).unwrap(); let block = block.build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); }); (12..23).for_each(|number| { let block = client.new_block_at(&BlockId::Number(number - 1), Default::default(), false) .unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); }); (23..24).for_each(|number| { let mut block = client.new_block_at(&BlockId::Number(number - 1), Default::default(), false).unwrap(); @@ -1621,24 +1687,24 @@ fn imports_blocks_with_changes_tries_config_change() { digest_levels: 1, })).unwrap(); let block = block.build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); }); (24..26).for_each(|number| { let mut block = client.new_block_at(&BlockId::Number(number - 1), Default::default(), false).unwrap(); block.push_storage_change(vec![42], Some(number.to_le_bytes().to_vec())).unwrap(); let block = block.build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); }); (26..27).for_each(|number| { let block = client.new_block_at(&BlockId::Number(number - 1), Default::default(), false) .unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); }); (27..28).for_each(|number| { let mut block = client.new_block_at(&BlockId::Number(number - 1), Default::default(), false).unwrap(); block.push_storage_change(vec![42], Some(number.to_le_bytes().to_vec())).unwrap(); let block = block.build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); }); (28..29).for_each(|number| { let mut block = client.new_block_at(&BlockId::Number(number - 1), Default::default(), false).unwrap(); @@ -1647,23 +1713,23 @@ fn imports_blocks_with_changes_tries_config_change() { digest_levels: 1, })).unwrap(); let block = block.build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); }); (29..30).for_each(|number| { let block = client.new_block_at(&BlockId::Number(number - 1), Default::default(), false) .unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); }); (30..31).for_each(|number| { let mut block = client.new_block_at(&BlockId::Number(number - 1), Default::default(), false).unwrap(); block.push_storage_change(vec![42], Some(number.to_le_bytes().to_vec())).unwrap(); let block = block.build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); }); (31..32).for_each(|number| { let block = client.new_block_at(&BlockId::Number(number - 1), Default::default(), false) .unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); }); // now check that configuration cache works @@ -1779,7 +1845,7 @@ fn cleans_up_closed_notification_sinks_on_block_import() { let mut import = BlockImportParams::new(origin, header); import.body = Some(extrinsics); import.fork_choice = Some(ForkChoiceStrategy::LongestChain); - client.import_block(import, Default::default()).unwrap(); + block_on(client.import_block(import, Default::default())).unwrap(); }; // after importing a block we should still have 4 notification sinks @@ -1822,14 +1888,14 @@ fn reorg_triggers_a_notification_even_for_sources_that_should_not_trigger_notifi Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::NetworkInitialSync, a1.clone()).unwrap(); + block_on(client.import(BlockOrigin::NetworkInitialSync, a1.clone())).unwrap(); let a2 = client.new_block_at( &BlockId::Hash(a1.hash()), Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::NetworkInitialSync, a2.clone()).unwrap(); + block_on(client.import(BlockOrigin::NetworkInitialSync, a2.clone())).unwrap(); let mut b1 = client.new_block_at( &BlockId::Number(0), @@ -1844,7 +1910,7 @@ fn reorg_triggers_a_notification_even_for_sources_that_should_not_trigger_notifi nonce: 0, }).unwrap(); let b1 = b1.build().unwrap().block; - client.import(BlockOrigin::NetworkInitialSync, b1.clone()).unwrap(); + block_on(client.import(BlockOrigin::NetworkInitialSync, b1.clone())).unwrap(); let b2 = client.new_block_at( &BlockId::Hash(b1.hash()), @@ -1853,7 +1919,7 @@ fn reorg_triggers_a_notification_even_for_sources_that_should_not_trigger_notifi ).unwrap().build().unwrap().block; // Should trigger a notification because we reorg - client.import_as_best(BlockOrigin::NetworkInitialSync, b2.clone()).unwrap(); + block_on(client.import_as_best(BlockOrigin::NetworkInitialSync, b2.clone())).unwrap(); // There should be one notification let notification = notification_stream.next().unwrap(); From 36c6bb585573ea08c65d3d11d017926c99aa5892 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sat, 27 Mar 2021 19:44:57 +0100 Subject: [PATCH 15/27] Fix sc-consensus-aura --- Cargo.lock | 1 + client/consensus/aura/Cargo.toml | 3 +- client/consensus/aura/src/import_queue.rs | 13 ++++--- client/consensus/aura/src/lib.rs | 36 ++++++++++++------ client/network/test/src/lib.rs | 45 +++++++++++------------ 5 files changed, 57 insertions(+), 41 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fdebc52eb33f7..8a483b5305701 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7116,6 +7116,7 @@ dependencies = [ name = "sc-consensus-aura" version = "0.9.0" dependencies = [ + "async-trait", "derive_more", "futures 0.3.13", "futures-timer 3.0.2", diff --git a/client/consensus/aura/Cargo.toml b/client/consensus/aura/Cargo.toml index 1465119c81d08..b2301fa9c5de5 100644 --- a/client/consensus/aura/Cargo.toml +++ b/client/consensus/aura/Cargo.toml @@ -26,7 +26,6 @@ futures = "0.3.9" futures-timer = "3.0.1" sp-inherents = { version = "3.0.0", path = "../../../primitives/inherents" } log = "0.4.8" -parking_lot = "0.11.1" sp-core = { version = "3.0.0", path = "../../../primitives/core" } sp-blockchain = { version = "3.0.0", path = "../../../primitives/blockchain" } sp-io = { version = "3.0.0", path = "../../../primitives/io" } @@ -38,6 +37,7 @@ sp-timestamp = { version = "3.0.0", path = "../../../primitives/timestamp" } sp-keystore = { version = "0.9.0", path = "../../../primitives/keystore" } sc-telemetry = { version = "3.0.0", path = "../../telemetry" } prometheus-endpoint = { package = "substrate-prometheus-endpoint", path = "../../../utils/prometheus", version = "0.9.0"} +async-trait = "0.1.42" # We enable it only for web-wasm check # See https://docs.rs/getrandom/0.2.1/getrandom/#webassembly-support getrandom = { version = "0.2", features = ["js"], optional = true } @@ -52,3 +52,4 @@ sc-network-test = { version = "0.8.0", path = "../../network/test" } sc-service = { version = "0.9.0", default-features = false, path = "../../service" } substrate-test-runtime-client = { version = "2.0.0", path = "../../../test-utils/runtime/client" } tempfile = "3.1.0" +parking_lot = "0.11.1" diff --git a/client/consensus/aura/src/import_queue.rs b/client/consensus/aura/src/import_queue.rs index a0d08202da2f6..736c89aff6b09 100644 --- a/client/consensus/aura/src/import_queue.rs +++ b/client/consensus/aura/src/import_queue.rs @@ -220,6 +220,7 @@ impl AuraVerifier where } } +#[async_trait::async_trait] impl Verifier for AuraVerifier where C: ProvideRuntimeApi + Send + @@ -234,7 +235,7 @@ impl Verifier for AuraVerifier where P::Signature: Encode + Decode, CAW: CanAuthorWith + Send + Sync + 'static, { - fn verify( + async fn verify( &mut self, origin: BlockOrigin, header: B::Header, @@ -405,6 +406,7 @@ impl, P> AuraBlockImport } } +#[async_trait::async_trait] impl BlockImport for AuraBlockImport where I: BlockImport> + Send + Sync, I::Error: Into, @@ -412,18 +414,19 @@ impl BlockImport for AuraBlockImport: Send + 'static, { type Error = ConsensusError; type Transaction = sp_api::TransactionFor; - fn check_block( + async fn check_block( &mut self, block: BlockCheckParams, ) -> Result { - self.inner.check_block(block).map_err(Into::into) + self.inner.check_block(block).await.map_err(Into::into) } - fn import_block( + async fn import_block( &mut self, block: BlockImportParams, new_cache: HashMap>, @@ -453,7 +456,7 @@ impl BlockImport for AuraBlockImport( { AuraWorker { client, - block_import: Arc::new(Mutex::new(block_import)), + block_import, env: proposer_factory, keystore, sync_oracle, @@ -286,7 +285,7 @@ pub fn build_aura_worker( struct AuraWorker { client: Arc, - block_import: Arc>, + block_import: I, env: E, keystore: SyncCryptoStorePtr, sync_oracle: SO, @@ -326,8 +325,8 @@ where "aura" } - fn block_import(&self) -> Arc> { - self.block_import.clone() + fn block_import(&mut self) -> &mut Self::BlockImport { + &mut self.block_import } fn epoch_data( @@ -581,6 +580,7 @@ mod tests { use super::*; use sp_consensus::{ NoNetwork as DummyOracle, Proposal, AlwaysCanAuthor, DisableProofRecording, + import_queue::BoxJustificationImport, }; use sc_network_test::{Block as TestBlock, *}; use sp_runtime::traits::{Block as BlockT, DigestFor}; @@ -642,13 +642,17 @@ mod tests { const SLOT_DURATION: u64 = 1000; + type AuraVerifier = import_queue::AuraVerifier; + type AuraPeer = Peer<(), PeersClient>; + pub struct AuraTestNet { - peers: Vec>, + peers: Vec, } impl TestNetFactory for AuraTestNet { - type Verifier = import_queue::AuraVerifier; + type Verifier = AuraVerifier; type PeerData = (); + type BlockImport = PeersClient; /// Create new test network with peers and given config. fn from_config(_config: &ProtocolConfig) -> Self { @@ -682,14 +686,22 @@ mod tests { } } - fn peer(&mut self, i: usize) -> &mut Peer { + fn make_block_import(&self, client: PeersClient) -> ( + BlockImportAdapter, + Option>, + Self::PeerData, + ) { + (client.as_block_import(), None, ()) + } + + fn peer(&mut self, i: usize) -> &mut AuraPeer { &mut self.peers[i] } - fn peers(&self) -> &Vec> { + fn peers(&self) -> &Vec { &self.peers } - fn mut_peers>)>(&mut self, closure: F) { + fn mut_peers)>(&mut self, closure: F) { closure(&mut self.peers); } } @@ -805,7 +817,7 @@ mod tests { let worker = AuraWorker { client: client.clone(), - block_import: Arc::new(Mutex::new(client)), + block_import: client, env: environ, keystore: keystore.into(), sync_oracle: DummyOracle.clone(), @@ -854,7 +866,7 @@ mod tests { let mut worker = AuraWorker { client: client.clone(), - block_import: Arc::new(Mutex::new(client.clone())), + block_import: client.clone(), env: environ, keystore: keystore.into(), sync_oracle: DummyOracle.clone(), diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index 1b9a54d2a9c03..5e05f57175490 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -242,12 +242,12 @@ impl BlockImport for PeersClient { } } -pub struct Peer { +pub struct Peer { pub data: D, client: PeersClient, /// We keep a copy of the verifier so that we can invoke it for locally-generated blocks, /// instead of going through the import queue. - verifier: VerifierAdapter, + verifier: VerifierAdapter, /// We keep a copy of the block_import so that we can invoke it for locally-generated blocks, /// instead of going through the import queue. block_import: BlockImportAdapter, @@ -259,8 +259,7 @@ pub struct Peer { listen_addr: Multiaddr, } -impl Peer where - V: Verifier, +impl Peer where B: BlockImport + Send + Sync, B::Transaction: Send, { @@ -585,13 +584,13 @@ impl BlockImport for BlockImportAdapter where } /// Implements `Verifier` and keeps track of failed verifications. -struct VerifierAdapter { - verifier: Verifier, +struct VerifierAdapter { + verifier: Arc>>>, failed_verifications: Arc>>, } #[async_trait::async_trait] -impl> Verifier for VerifierAdapter { +impl Verifier for VerifierAdapter { async fn verify( &mut self, origin: BlockOrigin, @@ -600,14 +599,14 @@ impl> Verifier for VerifierAdapter { body: Option> ) -> Result<(BlockImportParams, Option)>>), String> { let hash = header.hash(); - self.verifier.verify(origin, header, justifications, body).await.map_err(|e| { + self.verifier.lock().await.verify(origin, header, justifications, body).await.map_err(|e| { self.failed_verifications.lock().insert(hash, e.clone()); e }) } } -impl Clone for VerifierAdapter where Verifier: Clone { +impl Clone for VerifierAdapter { fn clone(&self) -> Self { Self { verifier: self.verifier.clone(), @@ -616,10 +615,10 @@ impl Clone for VerifierAdapter where Verifier: } } -impl VerifierAdapter { - fn new(verifier: Verifier) -> Self { +impl VerifierAdapter { + fn new(verifier: impl Verifier + 'static) -> Self { VerifierAdapter { - verifier, + verifier: Arc::new(futures::lock::Mutex::new(Box::new(verifier))), failed_verifications: Default::default(), } } @@ -643,7 +642,7 @@ pub struct FullPeerConfig { } pub trait TestNetFactory: Sized where >::Transaction: Send { - type Verifier: 'static + Verifier + Clone; + type Verifier: 'static + Verifier; type BlockImport: BlockImport + Clone + Send + Sync + 'static; type PeerData: Default; @@ -657,9 +656,9 @@ pub trait TestNetFactory: Sized where >: ) -> Self::Verifier; /// Get reference to peer. - fn peer(&mut self, i: usize) -> &mut Peer; - fn peers(&self) -> &Vec>; - fn mut_peers>)>( + fn peer(&mut self, i: usize) -> &mut Peer; + fn peers(&self) -> &Vec>; + fn mut_peers>)>( &mut self, closure: F, ); @@ -1012,7 +1011,7 @@ pub trait TestNetFactory: Sized where >: } pub struct TestNet { - peers: Vec>, + peers: Vec>, fork_choice: ForkChoiceStrategy, } @@ -1055,15 +1054,15 @@ impl TestNetFactory for TestNet { (client.as_block_import(), None, ()) } - fn peer(&mut self, i: usize) -> &mut Peer<(), Self::Verifier, Self::BlockImport> { + fn peer(&mut self, i: usize) -> &mut Peer<(), Self::BlockImport> { &mut self.peers[i] } - fn peers(&self) -> &Vec> { + fn peers(&self) -> &Vec> { &self.peers } - fn mut_peers>)>(&mut self, closure: F) { + fn mut_peers>)>(&mut self, closure: F) { closure(&mut self.peers); } } @@ -1099,16 +1098,16 @@ impl TestNetFactory for JustificationTestNet { self.0.make_verifier(client, config, peer_data) } - fn peer(&mut self, i: usize) -> &mut Peer { + fn peer(&mut self, i: usize) -> &mut Peer { self.0.peer(i) } - fn peers(&self) -> &Vec> { + fn peers(&self) -> &Vec> { self.0.peers() } fn mut_peers>, + &mut Vec>, )>(&mut self, closure: F) { self.0.mut_peers(closure) } From d9a8986526508657fb642f9d8573b5fb3a3eed82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sat, 27 Mar 2021 20:04:04 +0100 Subject: [PATCH 16/27] Fix fix fix --- client/consensus/babe/src/tests.rs | 13 +++++-------- client/finality-grandpa/src/tests.rs | 3 +-- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index 74d40864da3ca..f5a37ed028597 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -219,7 +219,7 @@ impl> BlockImport for PanickingBlockImport< } } -type BabePeer = Peer, TestVerifier, BabeBlockImport>; +type BabePeer = Peer, BabeBlockImport>; pub struct BabeTestNet { peers: Vec, @@ -233,11 +233,8 @@ type TestSelectChain = substrate_test_runtime_client::LongestChain< TestBlock, >; -#[derive(Clone)] pub struct TestVerifier { - inner: Arc< - futures::lock::Mutex> - >, + inner: BabeVerifier, mutator: Mutator, } @@ -255,7 +252,7 @@ impl Verifier for TestVerifier { ) -> Result<(BlockImportParams, Option)>>), String> { // apply post-sealing mutations (i.e. stripping seal, if desired). (self.mutator)(&mut header, Stage::PostSeal); - self.inner.lock().await.verify(dbg!(origin), header, justifications, body).await + self.inner.verify(dbg!(origin), header, justifications, body).await } } @@ -328,7 +325,7 @@ impl TestNetFactory for BabeTestNet { let (_, longest_chain) = TestClientBuilder::new().build_with_longest_chain(); TestVerifier { - inner: Arc::new(futures::lock::Mutex::new(BabeVerifier { + inner: BabeVerifier { client: client.clone(), select_chain: longest_chain, inherent_data_providers: data.inherent_data_providers.clone(), @@ -337,7 +334,7 @@ impl TestNetFactory for BabeTestNet { time_source: data.link.time_source.clone(), can_author_with: AlwaysCanAuthor, telemetry: None, - })), + }, mutator: MUTATOR.with(|m| m.borrow().clone()), } } diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 026a62939de09..b87bbefc11375 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -31,7 +31,6 @@ use futures_timer::Delay; use futures::executor::block_on; use tokio::runtime::{Runtime, Handle}; use sp_keyring::Ed25519Keyring; -use sc_client_api::backend::TransactionFor; use sp_blockchain::Result; use sp_api::{ApiRef, ProvideRuntimeApi}; use substrate_test_runtime_client::runtime::BlockNumber; @@ -57,7 +56,7 @@ use sp_application_crypto::key_types::GRANDPA; type TestLinkHalf = LinkHalf>; type PeerData = Mutex>; -type GrandpaPeer = Peer; +type GrandpaPeer = Peer; type GrandpaBlockImport = crate::GrandpaBlockImport< substrate_test_runtime_client::Backend, Block, From 5104bf6e997affbae90df6587c041d10789725ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sat, 27 Mar 2021 21:20:35 +0100 Subject: [PATCH 17/27] More fixes --- Cargo.lock | 1 + client/consensus/babe/src/lib.rs | 2 +- client/consensus/pow/Cargo.toml | 1 + client/consensus/pow/src/lib.rs | 27 ++++++++++--------- client/consensus/pow/src/worker.rs | 11 ++++---- .../common/src/import_queue/basic_queue.rs | 8 +++--- 6 files changed, 29 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8a483b5305701..57e42707c4d2d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7293,6 +7293,7 @@ dependencies = [ name = "sc-consensus-pow" version = "0.9.0" dependencies = [ + "async-trait", "derive_more", "futures 0.3.13", "futures-timer 3.0.2", diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index df08e267fdfd0..388d3288b1ec4 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -1680,7 +1680,7 @@ pub mod test_helpers { HeaderMetadata, C::Api: BabeApi, { - let epoch_changes = link.epoch_changes.lock(); + let epoch_changes = link.epoch_changes.shared_data(); let epoch = epoch_changes.epoch_data_for_child_of( descendent_query(client), &parent.hash(), diff --git a/client/consensus/pow/Cargo.toml b/client/consensus/pow/Cargo.toml index 8be43a8fa04bc..86b0b1df54e26 100644 --- a/client/consensus/pow/Cargo.toml +++ b/client/consensus/pow/Cargo.toml @@ -30,3 +30,4 @@ parking_lot = "0.11.1" sp-timestamp = { version = "3.0.0", path = "../../../primitives/timestamp" } derive_more = "0.99.2" prometheus-endpoint = { package = "substrate-prometheus-endpoint", path = "../../../utils/prometheus", version = "0.9.0"} +async-trait = "0.1.42" diff --git a/client/consensus/pow/src/lib.rs b/client/consensus/pow/src/lib.rs index d1df2875a1cb6..ea2e30afdc485 100644 --- a/client/consensus/pow/src/lib.rs +++ b/client/consensus/pow/src/lib.rs @@ -36,7 +36,7 @@ mod worker; pub use crate::worker::{MiningWorker, MiningMetadata, MiningBuild}; use std::{ - sync::Arc, any::Any, borrow::Cow, collections::HashMap, marker::PhantomData, + sync::Arc, borrow::Cow, collections::HashMap, marker::PhantomData, cmp::Ordering, time::Duration, }; use futures::{prelude::*, future::Either}; @@ -307,6 +307,7 @@ impl PowBlockImport wher } } +#[async_trait::async_trait] impl BlockImport for PowBlockImport where B: BlockT, I: BlockImport> + Send + Sync, @@ -314,21 +315,21 @@ impl BlockImport for PowBlockImport, C: ProvideRuntimeApi + Send + Sync + HeaderBackend + AuxStore + ProvideCache + BlockOf, C::Api: BlockBuilderApi, - Algorithm: PowAlgorithm, - Algorithm::Difficulty: 'static, - CAW: CanAuthorWith, + Algorithm: PowAlgorithm + Send, + Algorithm::Difficulty: 'static + Send, + CAW: CanAuthorWith + Send, { type Error = ConsensusError; type Transaction = sp_api::TransactionFor; - fn check_block( + async fn check_block( &mut self, block: BlockCheckParams, ) -> Result { - self.inner.check_block(block).map_err(Into::into) + self.inner.check_block(block).await.map_err(Into::into) } - fn import_block( + async fn import_block( &mut self, mut block: BlockImportParams, new_cache: HashMap>, @@ -403,7 +404,7 @@ impl BlockImport for PowBlockImport PowVerifier { } } +#[async_trait::async_trait] impl Verifier for PowVerifier where Algorithm: PowAlgorithm + Send + Sync, - Algorithm::Difficulty: 'static, + Algorithm::Difficulty: 'static + Send, { - fn verify( + async fn verify( &mut self, origin: BlockOrigin, header: B::Header, @@ -473,7 +475,7 @@ impl Verifier for PowVerifier where import_block.justifications = justifications; import_block.intermediates.insert( Cow::from(INTERMEDIATE_KEY), - Box::new(intermediate) as Box + Box::new(intermediate) as Box<_>, ); import_block.post_hash = Some(hash); @@ -513,6 +515,7 @@ pub fn import_queue( B: BlockT, Transaction: Send + Sync + 'static, Algorithm: PowAlgorithm + Clone + Send + Sync + 'static, + Algorithm::Difficulty: Send, { register_pow_inherent_data_provider(&inherent_data_providers)?; @@ -556,7 +559,7 @@ pub fn start_mining_worker( C: ProvideRuntimeApi + BlockchainEvents + 'static, S: SelectChain + 'static, Algorithm: PowAlgorithm + Clone, - Algorithm::Difficulty: 'static, + Algorithm::Difficulty: Send + 'static, E: Environment + Send + Sync + 'static, E::Error: std::fmt::Debug, E::Proposer: Proposer>, diff --git a/client/consensus/pow/src/worker.rs b/client/consensus/pow/src/worker.rs index d64596e48cf1a..18844e51ce418 100644 --- a/client/consensus/pow/src/worker.rs +++ b/client/consensus/pow/src/worker.rs @@ -16,7 +16,7 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -use std::{pin::Pin, time::Duration, collections::HashMap, any::Any, borrow::Cow}; +use std::{pin::Pin, time::Duration, collections::HashMap, borrow::Cow}; use sc_client_api::ImportNotifications; use sp_runtime::{DigestItem, traits::Block as BlockT, generic::BlockId}; use sp_consensus::{Proposal, BlockOrigin, BlockImportParams, import_queue::BoxBlockImport}; @@ -68,7 +68,8 @@ impl MiningWorker where Block: BlockT, C: sp_api::ProvideRuntimeApi, Algorithm: PowAlgorithm, - Algorithm::Difficulty: 'static, + Algorithm::Difficulty: 'static + Send, + sp_api::TransactionFor: Send + 'static, { /// Get the current best hash. `None` if the worker has just started or the client is doing /// major syncing. @@ -94,7 +95,7 @@ impl MiningWorker where /// Submit a mined seal. The seal will be validated again. Returns true if the submission is /// successful. - pub fn submit(&mut self, seal: Seal) -> bool { + pub async fn submit(&mut self, seal: Seal) -> bool { if let Some(build) = self.build.take() { match self.algorithm.verify( &BlockId::Hash(build.metadata.best_hash), @@ -135,10 +136,10 @@ impl MiningWorker where import_block.intermediates.insert( Cow::from(INTERMEDIATE_KEY), - Box::new(intermediate) as Box + Box::new(intermediate) as Box<_>, ); - match self.block_import.import_block(import_block, HashMap::default()) { + match self.block_import.import_block(import_block, HashMap::default()).await { Ok(_) => { info!( target: "pow", diff --git a/primitives/consensus/common/src/import_queue/basic_queue.rs b/primitives/consensus/common/src/import_queue/basic_queue.rs index 2a1c615b68dba..7998ba1b3ec76 100644 --- a/primitives/consensus/common/src/import_queue/basic_queue.rs +++ b/primitives/consensus/common/src/import_queue/basic_queue.rs @@ -439,8 +439,9 @@ mod tests { use sp_test_primitives::{Block, BlockNumber, Extrinsic, Hash, Header}; use std::collections::HashMap; + #[async_trait::async_trait] impl Verifier for () { - fn verify( + async fn verify( &mut self, origin: BlockOrigin, header: Header, @@ -451,18 +452,19 @@ mod tests { } } + #[async_trait::async_trait] impl BlockImport for () { type Error = crate::Error; type Transaction = Extrinsic; - fn check_block( + async fn check_block( &mut self, _block: BlockCheckParams, ) -> Result { Ok(ImportResult::imported(false)) } - fn import_block( + async fn import_block( &mut self, _block: BlockImportParams, _cache: HashMap>, From efcd0b53e140558d8ddab95fc33e22195e1bc1db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sat, 27 Mar 2021 22:51:49 +0100 Subject: [PATCH 18/27] Make everything compile *yeah* --- Cargo.lock | 3 +++ bin/node/cli/src/service.rs | 8 +++--- bin/node/executor/Cargo.toml | 1 + bin/node/executor/tests/basic.rs | 2 +- bin/node/testing/src/bench.rs | 2 +- .../basic-authorship/src/basic_authorship.rs | 27 ++++++++++--------- client/consensus/babe/rpc/src/lib.rs | 2 +- client/consensus/manual-seal/Cargo.toml | 1 + .../manual-seal/src/consensus/babe.rs | 15 ++++------- client/consensus/manual-seal/src/lib.rs | 3 ++- .../consensus/manual-seal/src/seal_block.rs | 2 +- .../finality-grandpa-warp-sync/src/proof.rs | 2 +- client/network/src/gossip/tests.rs | 4 ++- client/network/src/protocol/sync.rs | 14 +++++----- client/network/src/service/tests.rs | 4 ++- client/offchain/src/lib.rs | 5 ++-- client/rpc/src/chain/tests.rs | 12 ++++----- client/rpc/src/state/tests.rs | 6 ++--- client/sync-state-rpc/src/lib.rs | 6 ++--- client/transaction-pool/src/testing/pool.rs | 2 +- test-utils/runtime/Cargo.toml | 1 + test-utils/runtime/src/lib.rs | 2 +- 22 files changed, 66 insertions(+), 58 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 57e42707c4d2d..f41f50baeeb55 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4171,6 +4171,7 @@ dependencies = [ "frame-benchmarking", "frame-support", "frame-system", + "futures 0.3.13", "node-primitives", "node-runtime", "node-testing", @@ -7256,6 +7257,7 @@ name = "sc-consensus-manual-seal" version = "0.9.0" dependencies = [ "assert_matches", + "async-trait", "derive_more", "futures 0.3.13", "jsonrpc-core", @@ -9523,6 +9525,7 @@ dependencies = [ "frame-support", "frame-system", "frame-system-rpc-runtime-api", + "futures 0.3.13", "log", "memory-db", "pallet-babe", diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 1351782315be7..6c709273be198 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -534,7 +534,7 @@ pub fn new_light( #[cfg(test)] mod tests { - use std::{sync::Arc, borrow::Cow, any::Any, convert::TryInto}; + use std::{sync::Arc, borrow::Cow, convert::TryInto}; use sc_consensus_babe::{CompatibleDigestItem, BabeIntermediate, INTERMEDIATE_KEY}; use sc_consensus_epochs::descendent_query; use sp_consensus::{ @@ -638,7 +638,7 @@ mod tests { None, ); - let epoch_descriptor = babe_link.epoch_changes().lock().epoch_descriptor_for_child_of( + let epoch_descriptor = babe_link.epoch_changes().shared_data().epoch_descriptor_for_child_of( descendent_query(&*service.client()), &parent_hash, parent_number, @@ -696,11 +696,11 @@ mod tests { params.body = Some(new_body); params.intermediates.insert( Cow::from(INTERMEDIATE_KEY), - Box::new(BabeIntermediate:: { epoch_descriptor }) as Box, + Box::new(BabeIntermediate:: { epoch_descriptor }) as Box<_>, ); params.fork_choice = Some(ForkChoiceStrategy::LongestChain); - block_import.import_block(params, Default::default()) + futures::executor::block_on(block_import.import_block(params, Default::default())) .expect("error importing test block"); }, |service, _| { diff --git a/bin/node/executor/Cargo.toml b/bin/node/executor/Cargo.toml index fb7fc9191141c..54a44d59c2591 100644 --- a/bin/node/executor/Cargo.toml +++ b/bin/node/executor/Cargo.toml @@ -44,6 +44,7 @@ sp-runtime = { version = "3.0.0", path = "../../../primitives/runtime" } sp-externalities = { version = "0.9.0", path = "../../../primitives/externalities" } substrate-test-client = { version = "2.0.0", path = "../../../test-utils/client" } wat = "1.0" +futures = "0.3.9" [features] wasmtime = [ diff --git a/bin/node/executor/tests/basic.rs b/bin/node/executor/tests/basic.rs index 5f20c502e4953..fe3ae5f14cc37 100644 --- a/bin/node/executor/tests/basic.rs +++ b/bin/node/executor/tests/basic.rs @@ -841,5 +841,5 @@ fn should_import_block_with_test_client() { let block_data = block1.0; let block = node_primitives::Block::decode(&mut &block_data[..]).unwrap(); - client.import(BlockOrigin::Own, block).unwrap(); + futures::executor::block_on(client.import(BlockOrigin::Own, block)).unwrap(); } diff --git a/bin/node/testing/src/bench.rs b/bin/node/testing/src/bench.rs index cc6d7587dd517..edb99c617771a 100644 --- a/bin/node/testing/src/bench.rs +++ b/bin/node/testing/src/bench.rs @@ -691,7 +691,7 @@ impl BenchContext { assert_eq!(self.client.chain_info().best_number, 0); assert_eq!( - self.client.import_block(import_params, Default::default()) + futures::executor::block_on(self.client.import_block(import_params, Default::default())) .expect("Failed to import block"), ImportResult::Imported( ImportedAux { diff --git a/client/basic-authorship/src/basic_authorship.rs b/client/basic-authorship/src/basic_authorship.rs index 93ee4fc1445de..910abfad5ae1e 100644 --- a/client/basic-authorship/src/basic_authorship.rs +++ b/client/basic-authorship/src/basic_authorship.rs @@ -420,6 +420,7 @@ mod tests { use sp_blockchain::HeaderBackend; use sp_runtime::traits::NumberFor; use sc_client_api::Backend; + use futures::executor::block_on; const SOURCE: TransactionSource = TransactionSource::External; @@ -454,11 +455,11 @@ mod tests { client.clone(), ); - futures::executor::block_on( + block_on( txpool.submit_at(&BlockId::number(0), SOURCE, vec![extrinsic(0), extrinsic(1)]) ).unwrap(); - futures::executor::block_on( + block_on( txpool.maintain(chain_event( client.header(&BlockId::Number(0u64)) .expect("header get error") @@ -492,7 +493,7 @@ mod tests { // when let deadline = time::Duration::from_secs(3); - let block = futures::executor::block_on( + let block = block_on( proposer.propose(Default::default(), Default::default(), deadline) ).map(|r| r.block).unwrap(); @@ -538,7 +539,7 @@ mod tests { ); let deadline = time::Duration::from_secs(1); - futures::executor::block_on( + block_on( proposer.propose(Default::default(), Default::default(), deadline) ).map(|r| r.block).unwrap(); } @@ -559,11 +560,11 @@ mod tests { let genesis_hash = client.info().best_hash; let block_id = BlockId::Hash(genesis_hash); - futures::executor::block_on( + block_on( txpool.submit_at(&BlockId::number(0), SOURCE, vec![extrinsic(0)]), ).unwrap(); - futures::executor::block_on( + block_on( txpool.maintain(chain_event( client.header(&BlockId::Number(0u64)) .expect("header get error") @@ -585,7 +586,7 @@ mod tests { ); let deadline = time::Duration::from_secs(9); - let proposal = futures::executor::block_on( + let proposal = block_on( proposer.propose(Default::default(), Default::default(), deadline), ).unwrap(); @@ -625,7 +626,7 @@ mod tests { client.clone(), ); - futures::executor::block_on( + block_on( txpool.submit_at(&BlockId::number(0), SOURCE, vec![ extrinsic(0), extrinsic(1), @@ -667,7 +668,7 @@ mod tests { // when let deadline = time::Duration::from_secs(9); - let block = futures::executor::block_on( + let block = block_on( proposer.propose(Default::default(), Default::default(), deadline) ).map(|r| r.block).unwrap(); @@ -679,7 +680,7 @@ mod tests { block }; - futures::executor::block_on( + block_on( txpool.maintain(chain_event( client.header(&BlockId::Number(0u64)) .expect("header get error") @@ -689,9 +690,9 @@ mod tests { // let's create one block and import it let block = propose_block(&client, 0, 2, 7); - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); - futures::executor::block_on( + block_on( txpool.maintain(chain_event( client.header(&BlockId::Number(1)) .expect("header get error") @@ -701,6 +702,6 @@ mod tests { // now let's make sure that we can still make some progress let block = propose_block(&client, 1, 2, 5); - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); } } diff --git a/client/consensus/babe/rpc/src/lib.rs b/client/consensus/babe/rpc/src/lib.rs index ca14a764eece5..6696a65040a5e 100644 --- a/client/consensus/babe/rpc/src/lib.rs +++ b/client/consensus/babe/rpc/src/lib.rs @@ -217,7 +217,7 @@ fn epoch_data( SC: SelectChain, { let parent = select_chain.best_chain()?; - epoch_changes.lock().epoch_data_for_child_of( + epoch_changes.shared_data().epoch_data_for_child_of( descendent_query(&**client), &parent.hash(), parent.number().clone(), diff --git a/client/consensus/manual-seal/Cargo.toml b/client/consensus/manual-seal/Cargo.toml index 679fd5a3eb388..32cc89034fb1d 100644 --- a/client/consensus/manual-seal/Cargo.toml +++ b/client/consensus/manual-seal/Cargo.toml @@ -23,6 +23,7 @@ parking_lot = "0.11.1" codec = { package = "parity-scale-codec", version = "2.0.0" } serde = { version = "1.0", features=["derive"] } assert_matches = "1.3.0" +async-trait = "0.1.42" sc-client-api = { path = "../../api", version = "3.0.0"} sc-consensus-babe = { path = "../../consensus/babe", version = "0.9.0"} diff --git a/client/consensus/manual-seal/src/consensus/babe.rs b/client/consensus/manual-seal/src/consensus/babe.rs index a3f8a825e61dd..d627ea2a25c3a 100644 --- a/client/consensus/manual-seal/src/consensus/babe.rs +++ b/client/consensus/manual-seal/src/consensus/babe.rs @@ -21,12 +21,7 @@ use super::ConsensusDataProvider; use crate::Error; use codec::Encode; -use std::{ - any::Any, - borrow::Cow, - sync::{Arc, atomic}, - time::SystemTime, -}; +use std::{borrow::Cow, sync::{Arc, atomic}, time::SystemTime}; use sc_client_api::AuxStore; use sc_consensus_babe::{ Config, Epoch, authorship, CompatibleDigestItem, BabeIntermediate, @@ -102,7 +97,7 @@ impl BabeConsensusDataProvider } fn epoch(&self, parent: &B::Header, slot: Slot) -> Result { - let epoch_changes = self.epoch_changes.lock(); + let epoch_changes = self.epoch_changes.shared_data(); let epoch_descriptor = epoch_changes .epoch_descriptor_for_child_of( descendent_query(&*self.client), @@ -156,7 +151,7 @@ impl ConsensusDataProvider for BabeConsensusDataProvider authority_index: 0_u32, }); - let mut epoch_changes = self.epoch_changes.lock(); + let mut epoch_changes = self.epoch_changes.shared_data(); let epoch_descriptor = epoch_changes .epoch_descriptor_for_child_of( descendent_query(&*self.client), @@ -200,7 +195,7 @@ impl ConsensusDataProvider for BabeConsensusDataProvider inherents: &InherentData ) -> Result<(), Error> { let slot = inherents.babe_inherent_data()?; - let epoch_changes = self.epoch_changes.lock(); + let epoch_changes = self.epoch_changes.shared_data(); let mut epoch_descriptor = epoch_changes .epoch_descriptor_for_child_of( descendent_query(&*self.client), @@ -239,7 +234,7 @@ impl ConsensusDataProvider for BabeConsensusDataProvider params.intermediates.insert( Cow::from(INTERMEDIATE_KEY), - Box::new(BabeIntermediate:: { epoch_descriptor }) as Box, + Box::new(BabeIntermediate:: { epoch_descriptor }) as Box<_>, ); Ok(()) diff --git a/client/consensus/manual-seal/src/lib.rs b/client/consensus/manual-seal/src/lib.rs index 870640c1f2012..a5351c63bc3b4 100644 --- a/client/consensus/manual-seal/src/lib.rs +++ b/client/consensus/manual-seal/src/lib.rs @@ -55,8 +55,9 @@ pub const MANUAL_SEAL_ENGINE_ID: ConsensusEngineId = [b'm', b'a', b'n', b'l']; /// The verifier for the manual seal engine; instantly finalizes. struct ManualSealVerifier; +#[async_trait::async_trait] impl Verifier for ManualSealVerifier { - fn verify( + async fn verify( &mut self, origin: BlockOrigin, header: B::Header, diff --git a/client/consensus/manual-seal/src/seal_block.rs b/client/consensus/manual-seal/src/seal_block.rs index 2176973f3a298..23a560cebd54b 100644 --- a/client/consensus/manual-seal/src/seal_block.rs +++ b/client/consensus/manual-seal/src/seal_block.rs @@ -144,7 +144,7 @@ pub async fn seal_block( digest_provider.append_block_import(&parent, &mut params, &id)?; } - match block_import.import_block(params, HashMap::new())? { + match block_import.import_block(params, HashMap::new()).await? { ImportResult::Imported(aux) => { Ok(CreatedBlock { hash: ::Header::hash(&header), aux }) }, diff --git a/client/finality-grandpa-warp-sync/src/proof.rs b/client/finality-grandpa-warp-sync/src/proof.rs index e6fb989abc9d8..4677d2401e835 100644 --- a/client/finality-grandpa-warp-sync/src/proof.rs +++ b/client/finality-grandpa-warp-sync/src/proof.rs @@ -237,7 +237,7 @@ mod tests { block.header.digest_mut().logs.push(digest); } - client.import(BlockOrigin::Own, block).unwrap(); + futures::executor::block_on(client.import(BlockOrigin::Own, block)).unwrap(); if let Some(new_authorities) = new_authorities { // generate a justification for this block, finalize it and note the authority set diff --git a/client/network/src/gossip/tests.rs b/client/network/src/gossip/tests.rs index cd637f162721e..b000cf575ddb3 100644 --- a/client/network/src/gossip/tests.rs +++ b/client/network/src/gossip/tests.rs @@ -47,8 +47,10 @@ fn build_test_full_node(network_config: config::NetworkConfiguration) #[derive(Clone)] struct PassThroughVerifier(bool); + + #[async_trait::async_trait] impl sp_consensus::import_queue::Verifier for PassThroughVerifier { - fn verify( + async fn verify( &mut self, origin: sp_consensus::BlockOrigin, header: B::Header, diff --git a/client/network/src/protocol/sync.rs b/client/network/src/protocol/sync.rs index 37f9a451b67d3..22cfcc5eb4f6d 100644 --- a/client/network/src/protocol/sync.rs +++ b/client/network/src/protocol/sync.rs @@ -2016,7 +2016,7 @@ mod test { let mut new_blocks = |n| { for _ in 0..n { let block = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, block.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, block.clone())).unwrap(); } let info = client.info(); @@ -2147,7 +2147,7 @@ mod test { let block = block_builder.build().unwrap().block; - client.import(BlockOrigin::Own, block.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, block.clone())).unwrap(); block } @@ -2188,7 +2188,7 @@ mod test { let block = block_builder.build().unwrap().block; if import { - client2.import(BlockOrigin::Own, block.clone()).unwrap(); + block_on(client2.import(BlockOrigin::Own, block.clone())).unwrap(); } block @@ -2213,7 +2213,7 @@ mod test { send_block_announce(block3_fork.header().clone(), &peer_id2, &mut sync); // Import and tell sync that we now have the fork. - client.import(BlockOrigin::Own, block3_fork.clone()).unwrap(); + block_on(client.import(BlockOrigin::Own, block3_fork.clone())).unwrap(); sync.update_chain_info(&block3_fork.hash(), 3); let block4 = build_block_at(block3_fork.hash(), false); @@ -2325,7 +2325,7 @@ mod test { resp_blocks.into_iter() .rev() - .for_each(|b| client.import_as_final(BlockOrigin::Own, b).unwrap()); + .for_each(|b| block_on(client.import_as_final(BlockOrigin::Own, b)).unwrap()); } // Let peer2 announce that it finished syncing @@ -2388,7 +2388,7 @@ mod test { let mut client = Arc::new(TestClientBuilder::new().build()); let fork_blocks = blocks[..MAX_BLOCKS_TO_LOOK_BACKWARDS as usize * 2] .into_iter() - .inspect(|b| client.import(BlockOrigin::Own, (*b).clone()).unwrap()) + .inspect(|b| block_on(client.import(BlockOrigin::Own, (*b).clone())).unwrap()) .cloned() .collect::>(); @@ -2492,7 +2492,7 @@ mod test { resp_blocks.into_iter() .rev() - .for_each(|b| client.import(BlockOrigin::Own, b).unwrap()); + .for_each(|b| block_on(client.import(BlockOrigin::Own, b)).unwrap()); } // Request the tip diff --git a/client/network/src/service/tests.rs b/client/network/src/service/tests.rs index fd8cf4c3d105f..dd4a0597cbcbc 100644 --- a/client/network/src/service/tests.rs +++ b/client/network/src/service/tests.rs @@ -47,8 +47,10 @@ fn build_test_full_node(config: config::NetworkConfiguration) #[derive(Clone)] struct PassThroughVerifier(bool); + + #[async_trait::async_trait] impl sp_consensus::import_queue::Verifier for PassThroughVerifier { - fn verify( + async fn verify( &mut self, origin: sp_consensus::BlockOrigin, header: B::Header, diff --git a/client/offchain/src/lib.rs b/client/offchain/src/lib.rs index 717f02eccd5dc..26975edbd6b63 100644 --- a/client/offchain/src/lib.rs +++ b/client/offchain/src/lib.rs @@ -240,6 +240,7 @@ mod tests { use sp_consensus::BlockOrigin; use sc_client_api::Backend as _; use sc_block_builder::BlockBuilderProvider as _; + use futures::executor::block_on; struct TestNetwork(); @@ -331,7 +332,7 @@ mod tests { ).unwrap(); let block = block_builder.build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); assert_eq!(value, &offchain_db.get(sp_offchain::STORAGE_PREFIX, &key).unwrap()); @@ -341,7 +342,7 @@ mod tests { ).unwrap(); let block = block_builder.build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); assert!(offchain_db.get(sp_offchain::STORAGE_PREFIX, &key).is_none()); } diff --git a/client/rpc/src/chain/tests.rs b/client/rpc/src/chain/tests.rs index 025ff53c2fa95..bb673d65ea0f2 100644 --- a/client/rpc/src/chain/tests.rs +++ b/client/rpc/src/chain/tests.rs @@ -67,7 +67,7 @@ fn should_return_a_block() { let block = client.new_block(Default::default()).unwrap().build().unwrap().block; let block_hash = block.hash(); - client.import(BlockOrigin::Own, block).unwrap(); + executor::block_on(client.import(BlockOrigin::Own, block)).unwrap(); // Genesis block is not justified assert_matches!( @@ -133,7 +133,7 @@ fn should_return_block_hash() { ); let block = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, block.clone()).unwrap(); + executor::block_on(client.import(BlockOrigin::Own, block.clone())).unwrap(); assert_matches!( api.block_hash(Some(ListOrValue::Value(0u64.into())).into()), @@ -167,7 +167,7 @@ fn should_return_finalized_hash() { // import new block let block = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + executor::block_on(client.import(BlockOrigin::Own, block)).unwrap(); // no finalization yet assert_matches!( api.finalized_head(), @@ -199,7 +199,7 @@ fn should_notify_about_latest_block() { )); let block = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + executor::block_on(client.import(BlockOrigin::Own, block)).unwrap(); } // assert initial head sent. @@ -229,7 +229,7 @@ fn should_notify_about_best_block() { )); let block = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + executor::block_on(client.import(BlockOrigin::Own, block)).unwrap(); } // assert initial head sent. @@ -259,7 +259,7 @@ fn should_notify_about_finalized_block() { )); let block = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + executor::block_on(client.import(BlockOrigin::Own, block)).unwrap(); client.finalize_block(BlockId::number(1), None).unwrap(); } diff --git a/client/rpc/src/state/tests.rs b/client/rpc/src/state/tests.rs index 87b0fae1d6b3c..b5d30b3413903 100644 --- a/client/rpc/src/state/tests.rs +++ b/client/rpc/src/state/tests.rs @@ -180,7 +180,7 @@ fn should_notify_about_storage_changes() { nonce: 0, }).unwrap(); let block = builder.build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + executor::block_on(client.import(BlockOrigin::Own, block)).unwrap(); } // assert notification sent to transport @@ -222,7 +222,7 @@ fn should_send_initial_storage_changes_and_notifications() { nonce: 0, }).unwrap(); let block = builder.build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + executor::block_on(client.import(BlockOrigin::Own, block)).unwrap(); } // assert initial values sent to transport @@ -258,7 +258,7 @@ fn should_query_storage() { builder.push_storage_change(vec![5], Some(vec![nonce as u8])).unwrap(); let block = builder.build().unwrap().block; let hash = block.header.hash(); - client.import(BlockOrigin::Own, block).unwrap(); + executor::block_on(client.import(BlockOrigin::Own, block)).unwrap(); hash }; let block1_hash = add_block(0); diff --git a/client/sync-state-rpc/src/lib.rs b/client/sync-state-rpc/src/lib.rs index 8466523116440..4cb4955995540 100644 --- a/client/sync-state-rpc/src/lib.rs +++ b/client/sync-state-rpc/src/lib.rs @@ -37,7 +37,7 @@ type SharedEpochChanges = sc_consensus_epochs::SharedEpochChanges { #[error(transparent)] Blockchain(#[from] sp_blockchain::Error), - + #[error("Failed to load the block weight for block {0:?}")] LoadingBlockWeightFailed(::Hash), @@ -94,7 +94,7 @@ impl SyncStateRpcHandler chain_spec, client, shared_authority_set, shared_epoch_changes, deny_unsafe, } } - + fn build_sync_state(&self) -> Result, Error> { let finalized_hash = self.client.info().finalized_hash; let finalized_header = self.client.header(BlockId::Hash(finalized_hash))? @@ -108,7 +108,7 @@ impl SyncStateRpcHandler Ok(sc_chain_spec::LightSyncState { finalized_block_header: finalized_header, - babe_epoch_changes: self.shared_epoch_changes.lock().clone(), + babe_epoch_changes: self.shared_epoch_changes.shared_data().clone(), babe_finalized_block_weight: finalized_block_weight, grandpa_authority_set: self.shared_authority_set.clone_inner(), }) diff --git a/client/transaction-pool/src/testing/pool.rs b/client/transaction-pool/src/testing/pool.rs index a41632ed8de88..063947b383d03 100644 --- a/client/transaction-pool/src/testing/pool.rs +++ b/client/transaction-pool/src/testing/pool.rs @@ -985,7 +985,7 @@ fn import_notification_to_pool_maintain_works() { let mut block_builder = client.new_block(Default::default()).unwrap(); block_builder.push(xt).unwrap(); let block = block_builder.build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); + block_on(client.import(BlockOrigin::Own, block)).unwrap(); // Get the notification of the block import and maintain the pool with it, // Now, the pool should not contain any transactions. diff --git a/test-utils/runtime/Cargo.toml b/test-utils/runtime/Cargo.toml index 89da7929e64b8..96b7efff83380 100644 --- a/test-utils/runtime/Cargo.toml +++ b/test-utils/runtime/Cargo.toml @@ -53,6 +53,7 @@ serde = { version = "1.0.101", optional = true, features = ["derive"] } sc-block-builder = { version = "0.9.0", path = "../../client/block-builder" } sc-executor = { version = "0.9.0", path = "../../client/executor" } substrate-test-runtime-client = { version = "2.0.0", path = "./client" } +futures = "0.3.9" [build-dependencies] substrate-wasm-builder = { version = "4.0.0", path = "../../utils/wasm-builder" } diff --git a/test-utils/runtime/src/lib.rs b/test-utils/runtime/src/lib.rs index b0d0c63218f88..460494bfbd93a 100644 --- a/test-utils/runtime/src/lib.rs +++ b/test-utils/runtime/src/lib.rs @@ -1261,7 +1261,7 @@ mod tests { (BlockId::Hash(hash), block) }; - client.import(BlockOrigin::Own, block).unwrap(); + futures::executor::block_on(client.import(BlockOrigin::Own, block)).unwrap(); // Allocation of 1024k while having ~2048k should succeed. let ret = client.runtime_api().vec_with_capacity(&new_block_id, 1048576); From 682da05b03130af84973f20b6e3b6805f059cd3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sun, 28 Mar 2021 16:56:45 +0200 Subject: [PATCH 19/27] Fix build when we have Rust 1.51 --- .gitlab-ci.yml | 4 ++-- Cargo.toml | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 5cf4749eac645..e9f17f54503f4 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -336,7 +336,7 @@ check-web-wasm: # Note: we don't need to test crates imported in `bin/node/cli` - time cargo build --manifest-path=client/consensus/aura/Cargo.toml --target=wasm32-unknown-unknown --features getrandom # Note: the command below is a bit weird because several Cargo issues prevent us from compiling the node in a more straight-forward way. - - time cargo +nightly build --manifest-path=bin/node/cli/Cargo.toml --no-default-features --features browser --target=wasm32-unknown-unknown -Z features=itarget + - time cargo +nightly build --manifest-path=bin/node/cli/Cargo.toml --no-default-features --features browser --target=wasm32-unknown-unknown # with-tracing must be explicitly activated, we run a test to ensure this works as expected in both cases - time cargo +nightly test --manifest-path primitives/tracing/Cargo.toml --no-default-features - time cargo +nightly test --manifest-path primitives/tracing/Cargo.toml --no-default-features --features=with-tracing @@ -407,7 +407,7 @@ test-browser-node: CARGO_TARGET_WASM32_UNKNOWN_UNKNOWN_RUNNER: "wasm-bindgen-test-runner" WASM_BINDGEN_TEST_TIMEOUT: 120 script: - - cargo +nightly test --target wasm32-unknown-unknown -p node-browser-testing -Z features=itarget + - cargo +nightly test --target wasm32-unknown-unknown -p node-browser-testing build-linux-substrate: &build-binary stage: build diff --git a/Cargo.toml b/Cargo.toml index 57052a8d38e05..3e4787770e053 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,4 +1,6 @@ [workspace] +resolver = "2" + members = [ "bin/node-template/node", "bin/node-template/pallets/template", From 0727f1d91d93abd0dce06e911604cc08f6acfbb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 29 Mar 2021 21:44:57 +0200 Subject: [PATCH 20/27] Update client/consensus/common/src/shared_data.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> --- client/consensus/common/src/shared_data.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/consensus/common/src/shared_data.rs b/client/consensus/common/src/shared_data.rs index 4a09133852d2a..3a2bd0977dacc 100644 --- a/client/consensus/common/src/shared_data.rs +++ b/client/consensus/common/src/shared_data.rs @@ -16,7 +16,7 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -//! Provides the a generic wrapper around shared data. See [`SharedData`] for more information. +//! Provides a generic wrapper around shared data. See [`SharedData`] for more information. use std::sync::Arc; use parking_lot::{Mutex, MappedMutexGuard, Condvar, MutexGuard}; From f5756cd06bcef9fc77036cb85fe75a8fb82fcb87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 29 Mar 2021 21:45:06 +0200 Subject: [PATCH 21/27] Update client/consensus/common/src/shared_data.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> --- client/consensus/common/src/shared_data.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/consensus/common/src/shared_data.rs b/client/consensus/common/src/shared_data.rs index 3a2bd0977dacc..748516a2deb04 100644 --- a/client/consensus/common/src/shared_data.rs +++ b/client/consensus/common/src/shared_data.rs @@ -23,7 +23,7 @@ use parking_lot::{Mutex, MappedMutexGuard, Condvar, MutexGuard}; /// Created by [`SharedDataLocked::release_mutex`]. /// -/// As long as the object isn't dropped, the shared data is locked. Is it advised to drop this +/// As long as the object isn't dropped, the shared data is locked. It is advised to drop this /// object when the shared data doesn't need to be locked anymore. To get access to the shared data /// [`Self::upgrade`] is provided. #[must_use = "Shared data will be unlocked on drop!"] From 70c3d69f7260a42aac1b019e2807d19af245b762 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 29 Mar 2021 21:45:13 +0200 Subject: [PATCH 22/27] Update client/consensus/common/src/shared_data.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> --- client/consensus/common/src/shared_data.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/consensus/common/src/shared_data.rs b/client/consensus/common/src/shared_data.rs index 748516a2deb04..91a8645c5aa2e 100644 --- a/client/consensus/common/src/shared_data.rs +++ b/client/consensus/common/src/shared_data.rs @@ -60,7 +60,7 @@ impl Drop for SharedDataLockedUpgradable { /// time will need to wait until this lock is freed. /// /// If this object is dropped without calling [`Self::release_mutex`], the lock will be dropped -/// immediatley. +/// immediately. #[must_use = "Shared data will be unlocked on drop!"] pub struct SharedDataLocked<'a, T> { /// The current active mutex guard holding the inner data. From 315ddacb207868baf5e14d81c2e07aa8304995d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 29 Mar 2021 21:45:21 +0200 Subject: [PATCH 23/27] Update client/consensus/common/src/shared_data.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> --- client/consensus/common/src/shared_data.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/consensus/common/src/shared_data.rs b/client/consensus/common/src/shared_data.rs index 91a8645c5aa2e..8ce5a2f98ebe4 100644 --- a/client/consensus/common/src/shared_data.rs +++ b/client/consensus/common/src/shared_data.rs @@ -193,7 +193,7 @@ impl SharedData { /// Acquire access to the shared data. /// /// This will give mutable access to the shared data. After the returned mutex guard is dropped, - /// the shared data is accessable by other threads. So, this function should be used when + /// the shared data is accessible by other threads. So, this function should be used when /// reading/writing of the shared data in a local context is required. /// /// When requiring to lock shared data for some longer time, even with temporarily releasing the From 97ef6bb0968f0b36558acce8528413b6621d63f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 29 Mar 2021 21:45:28 +0200 Subject: [PATCH 24/27] Update client/consensus/common/src/shared_data.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> --- client/consensus/common/src/shared_data.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/consensus/common/src/shared_data.rs b/client/consensus/common/src/shared_data.rs index 8ce5a2f98ebe4..d90fc6273e056 100644 --- a/client/consensus/common/src/shared_data.rs +++ b/client/consensus/common/src/shared_data.rs @@ -53,7 +53,7 @@ impl Drop for SharedDataLockedUpgradable { /// Created by [`SharedData::shared_data_locked`]. /// -/// As long as this object isn't dropped, the shared data is hold in a mutex guard and the shared +/// As long as this object isn't dropped, the shared data is held in a mutex guard and the shared /// data is tagged as locked. Access to the shared data is provided through [`Deref`] and /// [`DerefMut`]. The trick is to use [`Self::release_mutex`] to release the mutex, but still keep /// the shared data locked. This means every other thread trying to access the shared data in this From 6a92c3886bc38f8da444afe60e426db2827e1259 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 29 Mar 2021 21:45:38 +0200 Subject: [PATCH 25/27] Update client/consensus/babe/src/tests.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> --- client/consensus/babe/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index f5a37ed028597..73c07f5b94509 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -418,7 +418,7 @@ fn run_one_test( // run each future until we get one of our own blocks with number higher than 5 // that was produced locally. client.import_notification_stream() - .take_while(move |n| future::ready(dbg!(n.header.number()) < &5 || { + .take_while(move |n| future::ready(n.header.number() < &5 || { if dbg!(n.origin) == BlockOrigin::Own { got_own = true; } else { From de62a7d3f01f9b2de209cd67f2e0c1fce4c78d89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 29 Mar 2021 21:45:46 +0200 Subject: [PATCH 26/27] Update client/consensus/babe/src/tests.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> --- client/consensus/babe/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index 73c07f5b94509..839d38b94a933 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -419,7 +419,7 @@ fn run_one_test( // that was produced locally. client.import_notification_stream() .take_while(move |n| future::ready(n.header.number() < &5 || { - if dbg!(n.origin) == BlockOrigin::Own { + if n.origin == BlockOrigin::Own { got_own = true; } else { got_other = true; From 7757ec956b06c4ab219d4e80dc5f0e6aae749501 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 29 Mar 2021 22:00:57 +0200 Subject: [PATCH 27/27] Fix warning --- bin/node-template/pallets/template/src/benchmarking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/node-template/pallets/template/src/benchmarking.rs b/bin/node-template/pallets/template/src/benchmarking.rs index 5296ed7261d98..93d7fa395ad6b 100644 --- a/bin/node-template/pallets/template/src/benchmarking.rs +++ b/bin/node-template/pallets/template/src/benchmarking.rs @@ -5,7 +5,7 @@ use super::*; use frame_system::RawOrigin; use frame_benchmarking::{benchmarks, whitelisted_caller, impl_benchmark_test_suite}; #[allow(unused)] -use crate::Module as Template; +use crate::Pallet as Template; benchmarks! { do_something {