From 97febf4c3086742bd98b78f0f4c8195509cf0487 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Wed, 7 Aug 2019 21:21:44 +0200 Subject: [PATCH] Make Verifier::verify mutable (#3165) * Make Verifier::verify mutable * Fix GrandPa tests * Fix doctest * Fix more doctests --- substrate/core/consensus/aura/src/lib.rs | 20 +++--- substrate/core/consensus/babe/src/lib.rs | 4 +- substrate/core/consensus/babe/src/tests.rs | 8 +-- .../core/consensus/common/src/block_import.rs | 2 +- .../core/consensus/common/src/import_queue.rs | 6 +- .../common/src/import_queue/basic_queue.rs | 71 +++++++++++-------- .../core/finality-grandpa/src/light_import.rs | 6 +- substrate/core/finality-grandpa/src/tests.rs | 6 +- .../core/network/src/test/block_import.rs | 8 +-- substrate/core/network/src/test/mod.rs | 37 +++++++--- substrate/core/service/src/lib.rs | 6 +- 11 files changed, 102 insertions(+), 72 deletions(-) diff --git a/substrate/core/consensus/aura/src/lib.rs b/substrate/core/consensus/aura/src/lib.rs index ffce6dad32..ac711ba2bd 100644 --- a/substrate/core/consensus/aura/src/lib.rs +++ b/substrate/core/consensus/aura/src/lib.rs @@ -516,7 +516,7 @@ impl Verifier for AuraVerifier where P::Signature: Encode + Decode, { fn verify( - &self, + &mut self, origin: BlockOrigin, header: B::Header, justification: Option, @@ -696,13 +696,11 @@ pub fn import_queue( register_aura_inherent_data_provider(&inherent_data_providers, slot_duration.get())?; initialize_authorities_cache(&*client)?; - let verifier = Arc::new( - AuraVerifier { - client: client.clone(), - inherent_data_providers, - phantom: PhantomData, - } - ); + let verifier = AuraVerifier { + client: client.clone(), + inherent_data_providers, + phantom: PhantomData, + }; Ok(BasicQueue::new( verifier, block_import, @@ -783,7 +781,7 @@ mod tests { } fn make_verifier(&self, client: PeersClient, _cfg: &ProtocolConfig) - -> Arc + -> Self::Verifier { match client { PeersClient::Full(client) => { @@ -796,11 +794,11 @@ mod tests { ).expect("Registers aura inherent data provider"); assert_eq!(slot_duration.get(), SLOT_DURATION); - Arc::new(AuraVerifier { + AuraVerifier { client, inherent_data_providers, phantom: Default::default(), - }) + } }, PeersClient::Light(_) => unreachable!("No (yet) tests for light client + Aura"), } diff --git a/substrate/core/consensus/babe/src/lib.rs b/substrate/core/consensus/babe/src/lib.rs index c8b9f25559..873913e59b 100644 --- a/substrate/core/consensus/babe/src/lib.rs +++ b/substrate/core/consensus/babe/src/lib.rs @@ -624,7 +624,7 @@ impl Verifier for BabeVerifier where C::Api: BlockBuilderApi + BabeApi, { fn verify( - &self, + &mut self, origin: BlockOrigin, header: B::Header, justification: Option, @@ -1182,7 +1182,7 @@ pub fn import_queue, I, RA, PRA>( let timestamp_core = verifier.time_source.clone(); let queue = BasicQueue::new( - Arc::new(verifier), + verifier, Box::new(block_import.clone()), justification_import, finality_proof_import, diff --git a/substrate/core/consensus/babe/src/tests.rs b/substrate/core/consensus/babe/src/tests.rs index f1983a9962..eaa4fbe099 100644 --- a/substrate/core/consensus/babe/src/tests.rs +++ b/substrate/core/consensus/babe/src/tests.rs @@ -97,7 +97,7 @@ impl Verifier for TestVerifier { /// new set of validators to import. If not, err with an Error-Message /// presented to the User in the logs. fn verify( - &self, + &mut self, origin: BlockOrigin, mut header: TestHeader, justification: Option, @@ -124,7 +124,7 @@ impl TestNetFactory for BabeTestNet { /// KLUDGE: this function gets the mutator from thread-local storage. fn make_verifier(&self, client: PeersClient, _cfg: &ProtocolConfig) - -> Arc + -> Self::Verifier { let api = client.as_full().expect("only full clients are used in test"); trace!(target: "babe", "Creating a verifier"); @@ -137,7 +137,7 @@ impl TestNetFactory for BabeTestNet { ).expect("Registers babe inherent data provider"); trace!(target: "babe", "Provider registered"); - Arc::new(TestVerifier { + TestVerifier { inner: BabeVerifier { api, inherent_data_providers, @@ -145,7 +145,7 @@ impl TestNetFactory for BabeTestNet { time_source: Default::default(), }, mutator: MUTATOR.with(|s| s.borrow().clone()), - }) + } } fn peer(&mut self, i: usize) -> &mut Peer { diff --git a/substrate/core/consensus/common/src/block_import.rs b/substrate/core/consensus/common/src/block_import.rs index 722bf1caed..1910a7e775 100644 --- a/substrate/core/consensus/common/src/block_import.rs +++ b/substrate/core/consensus/common/src/block_import.rs @@ -244,6 +244,6 @@ pub trait FinalityProofImport { hash: B::Hash, number: NumberFor, finality_proof: Vec, - verifier: &dyn Verifier, + verifier: &mut dyn Verifier, ) -> Result<(B::Hash, NumberFor), Self::Error>; } diff --git a/substrate/core/consensus/common/src/import_queue.rs b/substrate/core/consensus/common/src/import_queue.rs index 99ca6e7ad7..f4febb5e23 100644 --- a/substrate/core/consensus/common/src/import_queue.rs +++ b/substrate/core/consensus/common/src/import_queue.rs @@ -25,7 +25,7 @@ //! instantiated. The `BasicQueue` and `BasicVerifier` traits allow serial //! queues to be instantiated simply. -use std::{sync::Arc, collections::HashMap}; +use std::collections::HashMap; use sr_primitives::{Justification, traits::{Block as BlockT, Header as _, NumberFor}}; use crate::{error::Error as ConsensusError, well_known_cache_keys::Id as CacheKeyId}; use crate::block_import::{ @@ -71,7 +71,7 @@ pub trait Verifier: Send + Sync { /// new set of validators to import. If not, err with an Error-Message /// presented to the User in the logs. fn verify( - &self, + &mut self, origin: BlockOrigin, header: B::Header, justification: Option, @@ -170,7 +170,7 @@ pub fn import_single_block>( import_handle: &mut dyn BlockImport, block_origin: BlockOrigin, block: IncomingBlock, - verifier: Arc, + verifier: &mut V, ) -> Result>, BlockImportError> { let peer = block.origin; diff --git a/substrate/core/consensus/common/src/import_queue/basic_queue.rs b/substrate/core/consensus/common/src/import_queue/basic_queue.rs index 851662dbc6..da6dcd0293 100644 --- a/substrate/core/consensus/common/src/import_queue/basic_queue.rs +++ b/substrate/core/consensus/common/src/import_queue/basic_queue.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -use std::{mem, pin::Pin, sync::Arc, time::Duration}; +use std::{mem, pin::Pin, time::Duration}; use futures::{prelude::*, channel::mpsc, task::SpawnExt as _, task::Context, task::Poll}; use futures_timer::Delay; use sr_primitives::{Justification, traits::{Block as BlockT, Header as HeaderT, NumberFor}}; @@ -48,7 +48,7 @@ impl BasicQueue { /// This creates a background task, and calls `on_start` on the justification importer and /// finality proof importer. pub fn new>( - verifier: Arc, + verifier: V, block_import: BoxBlockImport, justification_import: Option>, finality_proof_import: Option>, @@ -134,18 +134,17 @@ enum ToWorkerMsg { ImportFinalityProof(Origin, B::Hash, NumberFor, Vec), } -struct BlockImportWorker> { +struct BlockImportWorker { result_sender: BufferedLinkSender, justification_import: Option>, finality_proof_import: Option>, - verifier: Arc, delay_between_blocks: Duration, } -impl> BlockImportWorker { - fn new( +impl BlockImportWorker { + fn new>( result_sender: BufferedLinkSender, - verifier: Arc, + verifier: V, block_import: BoxBlockImport, justification_import: Option>, finality_proof_import: Option>, @@ -154,7 +153,6 @@ impl> BlockImportWorker { let mut worker = BlockImportWorker { result_sender, - verifier, justification_import, finality_proof_import, delay_between_blocks: Duration::new(0, 0), @@ -178,7 +176,7 @@ impl> BlockImportWorker { // `Future`, and `block_import` is `None`. // - Something else, in which case `block_import` is `Some` and `importing` is None. // - let mut block_import = Some(block_import); + let mut block_import_verifier = Some((block_import, verifier)); let mut importing = None; let future = futures::future::poll_fn(move |cx| { @@ -194,15 +192,15 @@ impl> BlockImportWorker { if let Some(imp_fut) = importing.as_mut() { match Future::poll(Pin::new(imp_fut), cx) { Poll::Pending => return Poll::Pending, - Poll::Ready(bi) => { - block_import = Some(bi); + Poll::Ready((bi, verif)) => { + block_import_verifier = Some((bi, verif)); importing = None; }, } } debug_assert!(importing.is_none()); - debug_assert!(block_import.is_some()); + debug_assert!(block_import_verifier.is_some()); // Grab the next action request sent to the import queue. let msg = match Stream::poll_next(Pin::new(&mut port), cx) { @@ -215,11 +213,14 @@ impl> BlockImportWorker { ToWorkerMsg::ImportBlocks(origin, blocks) => { // On blocks import request, we merely *start* the process and store // a `Future` into `importing`. - let bi = block_import.take().expect("block_import is always Some; qed"); - importing = Some(worker.import_a_batch_of_blocks(bi, origin, blocks)); + let (bi, verif) = block_import_verifier.take() + .expect("block_import_verifier is always Some; qed"); + importing = Some(worker.import_a_batch_of_blocks(bi, verif, origin, blocks)); }, ToWorkerMsg::ImportFinalityProof(who, hash, number, proof) => { - worker.import_finality_proof(who, hash, number, proof); + let (_, verif) = block_import_verifier.as_mut() + .expect("block_import_verifier is always Some; qed"); + worker.import_finality_proof(verif, who, hash, number, proof); }, ToWorkerMsg::ImportJustification(who, hash, number, justification) => { worker.import_justification(who, hash, number, justification); @@ -236,23 +237,30 @@ impl> BlockImportWorker { /// /// For lifetime reasons, the `BlockImport` implementation must be passed by value, and is /// yielded back in the output once the import is finished. - fn import_a_batch_of_blocks( + fn import_a_batch_of_blocks>( &mut self, block_import: BoxBlockImport, + verifier: V, origin: BlockOrigin, blocks: Vec> - ) -> impl Future> { + ) -> impl Future, V)> { let mut result_sender = self.result_sender.clone(); - import_many_blocks(block_import, origin, blocks, self.verifier.clone(), self.delay_between_blocks) - .then(move |(imported, count, results, block_import)| { + import_many_blocks(block_import, origin, blocks, verifier, self.delay_between_blocks) + .then(move |(imported, count, results, block_import, verifier)| { result_sender.blocks_processed(imported, count, results); - future::ready(block_import) + future::ready((block_import, verifier)) }) } - fn import_finality_proof(&mut self, who: Origin, hash: B::Hash, number: NumberFor, finality_proof: Vec) { - let verifier = &*self.verifier; + fn import_finality_proof>( + &mut self, + verifier: &mut V, + who: Origin, + hash: B::Hash, + number: NumberFor, + finality_proof: Vec + ) { let result = self.finality_proof_import.as_mut().map(|finality_proof_import| { finality_proof_import.import_finality_proof(hash, number, finality_proof, verifier) .map_err(|e| { @@ -307,12 +315,12 @@ fn import_many_blocks>( import_handle: BoxBlockImport, blocks_origin: BlockOrigin, blocks: Vec>, - verifier: Arc, + verifier: V, delay_between_blocks: Duration, ) -> impl Future>, BlockImportError>, B::Hash, -)>, BoxBlockImport)> { +)>, BoxBlockImport, V)> { let count = blocks.len(); let blocks_range = match ( @@ -332,6 +340,7 @@ fn import_many_blocks>( let mut blocks = blocks.into_iter(); let mut import_handle = Some(import_handle); let mut waiting = None; + let mut verifier = Some(verifier); // Blocks in the response/drain should be in ascending order. @@ -352,16 +361,20 @@ fn import_many_blocks>( // No block left to import, success! let import_handle = import_handle.take() .expect("Future polled again after it has finished"); + let verifier = verifier.take() + .expect("Future polled again after it has finished"); let results = mem::replace(&mut results, Vec::new()); - return Poll::Ready((imported, count, results, import_handle)); + return Poll::Ready((imported, count, results, import_handle, verifier)); }, }; - // We extract the content of `import_handle` only when the future ends, therefore - // `import_handle` is always `Some` here. It is illegal to poll a `Future` again after it - // has ended. + // We extract the content of `import_handle` and `verifier` only when the future ends, + // therefore `import_handle` and `verifier` are always `Some` here. It is illegal to poll + // a `Future` again after it has ended. let import_handle = import_handle.as_mut() .expect("Future polled again after it has finished"); + let verifier = verifier.as_mut() + .expect("Future polled again after it has finished"); let block_number = block.header.as_ref().map(|h| h.number().clone()); let block_hash = block.hash; @@ -373,7 +386,7 @@ fn import_many_blocks>( &mut **import_handle, blocks_origin.clone(), block, - verifier.clone(), + verifier, ) }; diff --git a/substrate/core/finality-grandpa/src/light_import.rs b/substrate/core/finality-grandpa/src/light_import.rs index 4dc934303c..6ecc24bd2b 100644 --- a/substrate/core/finality-grandpa/src/light_import.rs +++ b/substrate/core/finality-grandpa/src/light_import.rs @@ -174,7 +174,7 @@ impl, RA> FinalityProofImport hash: Block::Hash, number: NumberFor, finality_proof: Vec, - verifier: &dyn Verifier, + verifier: &mut dyn Verifier, ) -> Result<(Block::Hash, NumberFor), Self::Error> { do_import_finality_proof::<_, _, _, _, GrandpaJustification>( &*self.client, @@ -290,7 +290,7 @@ fn do_import_finality_proof, RA, J>( _hash: Block::Hash, _number: NumberFor, finality_proof: Vec, - verifier: &dyn Verifier, + verifier: &mut dyn Verifier, ) -> Result<(Block::Hash, NumberFor), ConsensusError> where B: Backend + 'static, @@ -608,7 +608,7 @@ pub mod tests { hash: Block::Hash, number: NumberFor, finality_proof: Vec, - verifier: &dyn Verifier, + verifier: &mut dyn Verifier, ) -> Result<(Block::Hash, NumberFor), Self::Error> { self.0.import_finality_proof(hash, number, finality_proof, verifier) } diff --git a/substrate/core/finality-grandpa/src/tests.rs b/substrate/core/finality-grandpa/src/tests.rs index 8b0cc3bc0c..45a91b336a 100644 --- a/substrate/core/finality-grandpa/src/tests.rs +++ b/substrate/core/finality-grandpa/src/tests.rs @@ -97,10 +97,8 @@ impl TestNetFactory for GrandpaTestNet { } } - fn make_verifier(&self, _client: PeersClient, _cfg: &ProtocolConfig) - -> Arc - { - Arc::new(PassThroughVerifier(false)) // use non-instant finality. + fn make_verifier(&self, _client: PeersClient, _cfg: &ProtocolConfig) -> Self::Verifier { + PassThroughVerifier(false) // use non-instant finality. } fn make_block_import(&self, client: PeersClient) diff --git a/substrate/core/network/src/test/block_import.rs b/substrate/core/network/src/test/block_import.rs index ce58755a89..ceb0451166 100644 --- a/substrate/core/network/src/test/block_import.rs +++ b/substrate/core/network/src/test/block_import.rs @@ -45,7 +45,7 @@ fn prepare_good_block() -> (TestClient, Hash, u64, PeerId, IncomingBlock) #[test] fn import_single_good_block_works() { let (_, _hash, number, peer_id, block) = prepare_good_block(); - match import_single_block(&mut test_client::new(), BlockOrigin::File, block, Arc::new(PassThroughVerifier(true))) { + match import_single_block(&mut test_client::new(), BlockOrigin::File, block, &mut PassThroughVerifier(true)) { Ok(BlockImportResult::ImportedUnknown(ref num, ref aux, ref org)) if *num == number && *aux == Default::default() && *org == Some(peer_id) => {} _ => panic!() @@ -55,7 +55,7 @@ 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(&mut client, BlockOrigin::File, block, Arc::new(PassThroughVerifier(true))) { + match import_single_block(&mut client, BlockOrigin::File, block, &mut PassThroughVerifier(true)) { Ok(BlockImportResult::ImportedKnown(ref n)) if *n == number => {} _ => panic!() } @@ -65,7 +65,7 @@ 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(&mut test_client::new(), BlockOrigin::File, block, Arc::new(PassThroughVerifier(true))) { + match import_single_block(&mut test_client::new(), BlockOrigin::File, block, &mut PassThroughVerifier(true)) { Err(BlockImportError::IncompleteHeader(ref org)) if *org == Some(peer_id) => {} _ => panic!() } @@ -75,7 +75,7 @@ fn import_single_good_block_without_header_fails() { fn async_import_queue_drops() { // Perform this test multiple times since it exhibits non-deterministic behavior. for _ in 0..100 { - let verifier = Arc::new(PassThroughVerifier(true)); + let verifier = PassThroughVerifier(true); let queue = BasicQueue::new(verifier, Box::new(test_client::new()), None, None); drop(queue); } diff --git a/substrate/core/network/src/test/mod.rs b/substrate/core/network/src/test/mod.rs index 110201057c..f12b4c8237 100644 --- a/substrate/core/network/src/test/mod.rs +++ b/substrate/core/network/src/test/mod.rs @@ -68,7 +68,7 @@ pub struct PassThroughVerifier(pub bool); /// This `Verifier` accepts all data as valid. impl Verifier for PassThroughVerifier { fn verify( - &self, + &mut self, origin: BlockOrigin, header: B::Header, justification: Option, @@ -212,7 +212,7 @@ pub struct Peer> { 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: Arc>, + 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: Box>, @@ -395,6 +395,27 @@ impl> BlockImport for BlockImportAdapter>`. Used internally. +struct VerifierAdapter(Arc>>); + +impl Clone for VerifierAdapter { + fn clone(&self) -> Self { + VerifierAdapter(self.0.clone()) + } +} + +impl> Verifier for VerifierAdapter { + fn verify( + &mut self, + origin: BlockOrigin, + header: B::Header, + justification: Option, + body: Option> + ) -> Result<(BlockImportParams, Option)>>), String> { + self.0.lock().verify(origin, header, justification, body) + } +} + pub trait TestNetFactory: Sized { type Specialization: NetworkSpecialization + SpecializationFactory; type Verifier: 'static + Verifier; @@ -402,7 +423,7 @@ pub trait TestNetFactory: Sized { /// These two need to be implemented! fn from_config(config: &ProtocolConfig) -> Self; - fn make_verifier(&self, client: PeersClient, config: &ProtocolConfig) -> Arc; + fn make_verifier(&self, client: PeersClient, config: &ProtocolConfig) -> Self::Verifier; /// Get reference to peer. fn peer(&mut self, i: usize) -> &mut Peer; @@ -448,6 +469,7 @@ pub trait TestNetFactory: Sized { fn add_full_peer(&mut self, config: &ProtocolConfig) { let client = Arc::new(test_client::new()); let verifier = self.make_verifier(PeersClient::Full(client.clone()), config); + let verifier = VerifierAdapter(Arc::new(Mutex::new(Box::new(verifier) as Box<_>))); let (block_import, justification_import, finality_proof_import, finality_proof_request_builder, data) = self.make_block_import(PeersClient::Full(client.clone())); let block_import = BlockImportAdapter(Arc::new(Mutex::new(block_import))); @@ -507,6 +529,7 @@ pub trait TestNetFactory: Sized { let client = Arc::new(test_client::new_light()); let verifier = self.make_verifier(PeersClient::Light(client.clone()), &config); + let verifier = VerifierAdapter(Arc::new(Mutex::new(Box::new(verifier) as Box<_>))); let (block_import, justification_import, finality_proof_import, finality_proof_request_builder, data) = self.make_block_import(PeersClient::Light(client.clone())); let block_import = BlockImportAdapter(Arc::new(Mutex::new(block_import))); @@ -625,9 +648,9 @@ impl TestNetFactory for TestNet { } fn make_verifier(&self, _client: PeersClient, _config: &ProtocolConfig) - -> Arc + -> Self::Verifier { - Arc::new(PassThroughVerifier(false)) + PassThroughVerifier(false) } fn peer(&mut self, i: usize) -> &mut Peer<(), Self::Specialization> { @@ -670,9 +693,7 @@ impl TestNetFactory for JustificationTestNet { JustificationTestNet(TestNet::from_config(config)) } - fn make_verifier(&self, client: PeersClient, config: &ProtocolConfig) - -> Arc - { + fn make_verifier(&self, client: PeersClient, config: &ProtocolConfig) -> Self::Verifier { self.0.make_verifier(client, config) } diff --git a/substrate/core/service/src/lib.rs b/substrate/core/service/src/lib.rs index f6f038cdf5..f28d41019b 100644 --- a/substrate/core/service/src/lib.rs +++ b/substrate/core/service/src/lib.rs @@ -891,7 +891,7 @@ impl network::TransactionPool, ComponentBlock< /// # struct MyVerifier; /// # impl Verifier for MyVerifier { /// # fn verify( -/// # &self, +/// # &mut self, /// # origin: BlockOrigin, /// # header: B::Header, /// # justification: Option, @@ -929,11 +929,11 @@ impl network::TransactionPool, ComponentBlock< /// LightService = LightComponents /// { |config| >::new(config) }, /// FullImportQueue = BasicQueue -/// { |_, client, _| Ok(BasicQueue::new(Arc::new(MyVerifier), Box::new(client), None, None)) }, +/// { |_, client, _| Ok(BasicQueue::new(MyVerifier, Box::new(client), None, None)) }, /// LightImportQueue = BasicQueue /// { |_, client| { /// let fprb = Box::new(DummyFinalityProofRequestBuilder::default()) as Box<_>; -/// Ok((BasicQueue::new(Arc::new(MyVerifier), Box::new(client), None, None), fprb)) +/// Ok((BasicQueue::new(MyVerifier, Box::new(client), None, None), fprb)) /// }}, /// SelectChain = LongestChain, Self::Block> /// { |config: &FactoryFullConfiguration, client: Arc>| {