From a736c0553f790a8ff6d3c7ad8bd1b282af7ab0a3 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 3 Feb 2020 06:59:08 +0100 Subject: [PATCH 1/4] block_import: switch to Box for intermediates representation --- client/consensus/pow/src/lib.rs | 15 ++++++---- .../consensus/common/src/block_import.rs | 29 ++++++++++++++++++- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/client/consensus/pow/src/lib.rs b/client/consensus/pow/src/lib.rs index ca7285b7fe559..1db33891d3913 100644 --- a/client/consensus/pow/src/lib.rs +++ b/client/consensus/pow/src/lib.rs @@ -30,6 +30,7 @@ //! clients. use std::sync::Arc; +use std::any::Any; use std::thread; use std::collections::HashMap; use std::marker::PhantomData; @@ -257,6 +258,7 @@ impl BlockImport for PowBlockImport + Send + Sync + HeaderBackend + AuxStore + ProvideCache + BlockOf, C::Api: BlockBuilderApi, Algorithm: PowAlgorithm, + Algorithm::Difficulty: 'static, { type Error = ConsensusError; type Transaction = sp_api::TransactionFor; @@ -312,10 +314,9 @@ impl BlockImport for PowBlockImport return Err(Error::::HeaderUnsealed(block.header.hash()).into()), }; - let intermediate = PowIntermediate::::decode( - &mut &block.intermediates.remove(INTERMEDIATE_KEY) - .ok_or(Error::::NoIntermediate)?[..] - ).map_err(|_| Error::::NoIntermediate)?; + let intermediate = block.take_intermediate::>( + INTERMEDIATE_KEY + ).ok_or(Error::::NoIntermediate)?; let difficulty = match intermediate.difficulty { Some(difficulty) => difficulty, @@ -392,6 +393,7 @@ impl PowVerifier { impl Verifier for PowVerifier where Algorithm: PowAlgorithm + Send + Sync, + Algorithm::Difficulty: 'static, { fn verify( &mut self, @@ -418,7 +420,7 @@ impl Verifier for PowVerifier where justification, intermediates: { let mut ret = HashMap::new(); - ret.insert(INTERMEDIATE_KEY.to_vec(), intermediate.encode()); + ret.insert(INTERMEDIATE_KEY, Box::new(intermediate) as Box); ret }, auxiliary: vec![], @@ -553,6 +555,7 @@ fn mine_loop( ) -> Result<(), Error> where C: HeaderBackend + AuxStore + ProvideRuntimeApi, Algorithm: PowAlgorithm, + Algorithm::Difficulty: 'static, E: Environment, E::Proposer: Proposer>, E::Error: std::fmt::Debug, @@ -659,7 +662,7 @@ fn mine_loop( storage_changes: Some(proposal.storage_changes), intermediates: { let mut ret = HashMap::new(); - ret.insert(INTERMEDIATE_KEY.to_vec(), intermediate.encode()); + ret.insert(INTERMEDIATE_KEY, Box::new(intermediate) as Box); ret }, finalized: false, diff --git a/primitives/consensus/common/src/block_import.rs b/primitives/consensus/common/src/block_import.rs index af1b61a9febde..1bd808cc358a9 100644 --- a/primitives/consensus/common/src/block_import.rs +++ b/primitives/consensus/common/src/block_import.rs @@ -22,6 +22,7 @@ use serde::{Serialize, Deserialize}; use std::borrow::Cow; use std::collections::HashMap; use std::sync::Arc; +use std::any::Any; use crate::import_queue::{Verifier, CacheKeyId}; @@ -144,7 +145,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, Vec>, + pub intermediates: HashMap<&'static [u8], Box>, /// Auxiliary consensus data produced by the block. /// Contains a list of key-value pairs. If values are `None`, the keys /// will be deleted. @@ -223,6 +224,32 @@ impl BlockImportParams { import_existing: self.import_existing, } } + + /// Take interemdiate by given key, and remove it from the processing list. + pub fn take_intermediate(&mut self, key: &'static [u8]) -> Option> { + self.intermediates.remove(key) + .and_then(|value| { + match value.downcast::() { + Ok(v) => Some(v), + Err(v) => { + self.intermediates.insert(key, v); + None + }, + } + }) + } + + /// Get a reference to a given intermediate. + pub fn intermediate(&self, key: &'static [u8]) -> Option<&T> { + self.intermediates.get(key) + .and_then(|value| value.downcast_ref::()) + } + + /// Get a mutable reference to a given intermediate. + pub fn intermediate_mut(&mut self, key: &'static [u8]) -> Option<&mut T> { + self.intermediates.get_mut(key) + .and_then(|value| value.downcast_mut::()) + } } /// Block import trait. From dc9243e7aaa6032c40ada39cbc9d2260756ba11b Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 3 Feb 2020 07:38:58 +0100 Subject: [PATCH 2/4] Use Cow and return Error instead of Option --- client/consensus/pow/src/lib.rs | 7 +++-- .../consensus/common/src/block_import.rs | 31 ++++++++++--------- primitives/consensus/common/src/error.rs | 3 ++ 3 files changed, 24 insertions(+), 17 deletions(-) diff --git a/client/consensus/pow/src/lib.rs b/client/consensus/pow/src/lib.rs index 1db33891d3913..185834171920b 100644 --- a/client/consensus/pow/src/lib.rs +++ b/client/consensus/pow/src/lib.rs @@ -31,6 +31,7 @@ use std::sync::Arc; use std::any::Any; +use std::borrow::Cow; use std::thread; use std::collections::HashMap; use std::marker::PhantomData; @@ -316,7 +317,7 @@ impl BlockImport for PowBlockImport>( INTERMEDIATE_KEY - ).ok_or(Error::::NoIntermediate)?; + )?; let difficulty = match intermediate.difficulty { Some(difficulty) => difficulty, @@ -420,7 +421,7 @@ impl Verifier for PowVerifier where justification, intermediates: { let mut ret = HashMap::new(); - ret.insert(INTERMEDIATE_KEY, Box::new(intermediate) as Box); + ret.insert(Cow::from(INTERMEDIATE_KEY), Box::new(intermediate) as Box); ret }, auxiliary: vec![], @@ -662,7 +663,7 @@ fn mine_loop( storage_changes: Some(proposal.storage_changes), intermediates: { let mut ret = HashMap::new(); - ret.insert(INTERMEDIATE_KEY, Box::new(intermediate) as Box); + ret.insert(Cow::from(INTERMEDIATE_KEY), Box::new(intermediate) as Box); ret }, finalized: false, diff --git a/primitives/consensus/common/src/block_import.rs b/primitives/consensus/common/src/block_import.rs index 1bd808cc358a9..1ac8f5b6e2102 100644 --- a/primitives/consensus/common/src/block_import.rs +++ b/primitives/consensus/common/src/block_import.rs @@ -24,6 +24,7 @@ use std::collections::HashMap; use std::sync::Arc; use std::any::Any; +use crate::Error; use crate::import_queue::{Verifier, CacheKeyId}; /// Block import result. @@ -145,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<&'static [u8], 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. @@ -226,29 +227,31 @@ impl BlockImportParams { } /// Take interemdiate by given key, and remove it from the processing list. - pub fn take_intermediate(&mut self, key: &'static [u8]) -> Option> { - self.intermediates.remove(key) - .and_then(|value| { - match value.downcast::() { - Ok(v) => Some(v), - Err(v) => { - self.intermediates.insert(key, v); - None - }, - } - }) + pub fn take_intermediate(&mut self, key: &[u8]) -> Result, Error> { + if self.intermediates.contains_key(key) { + self.intermediates.remove(key) + .ok_or(Error::NoIntermediate) + .and_then(|value| { + value.downcast::() + .map_err(|_| Error::NoIntermediate) + }) + } else { + Err(Error::NoIntermediate) + } } /// Get a reference to a given intermediate. - pub fn intermediate(&self, key: &'static [u8]) -> Option<&T> { + pub fn intermediate(&self, key: &[u8]) -> Result<&T, Error> { self.intermediates.get(key) .and_then(|value| value.downcast_ref::()) + .ok_or(Error::NoIntermediate) } /// Get a mutable reference to a given intermediate. - pub fn intermediate_mut(&mut self, key: &'static [u8]) -> Option<&mut T> { + pub fn intermediate_mut(&mut self, key: &[u8]) -> Result<&mut T, Error> { self.intermediates.get_mut(key) .and_then(|value| value.downcast_mut::()) + .ok_or(Error::NoIntermediate) } } diff --git a/primitives/consensus/common/src/error.rs b/primitives/consensus/common/src/error.rs index 972e4a2d48b21..5a4f85ee67f03 100644 --- a/primitives/consensus/common/src/error.rs +++ b/primitives/consensus/common/src/error.rs @@ -31,6 +31,9 @@ pub enum Error { /// I/O terminated unexpectedly #[display(fmt="I/O terminated unexpectedly.")] IoTerminated, + /// Intermediate missing. + #[display(fmt="Missing or invalid intermediate.")] + NoIntermediate, /// Unable to schedule wakeup. #[display(fmt="Timer error: {}", _0)] FaultyTimer(std::io::Error), From 7643ed440b59749784f8f6795bd9e176522b904f Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 3 Feb 2020 07:40:02 +0100 Subject: [PATCH 3/4] Remove unused error --- client/consensus/pow/src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/client/consensus/pow/src/lib.rs b/client/consensus/pow/src/lib.rs index 185834171920b..87f51be216f60 100644 --- a/client/consensus/pow/src/lib.rs +++ b/client/consensus/pow/src/lib.rs @@ -65,8 +65,6 @@ pub enum Error { InvalidSeal, #[display(fmt = "PoW validation error: invalid difficulty")] InvalidDifficulty, - #[display(fmt = "PoW block import expects an intermediate, but not found one")] - NoIntermediate, #[display(fmt = "Rejecting block too far in future")] TooFarInFuture, #[display(fmt = "Fetching best header failed using select chain: {:?}", _0)] From 05e7e51030742c9afdfba89182baf25720867777 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 3 Feb 2020 09:31:54 +0100 Subject: [PATCH 4/4] Distinguish NoIntermediate/InvalidIntermediate --- primitives/consensus/common/src/block_import.rs | 12 +++++++----- primitives/consensus/common/src/error.rs | 5 ++++- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/primitives/consensus/common/src/block_import.rs b/primitives/consensus/common/src/block_import.rs index 1ac8f5b6e2102..952a044e9b035 100644 --- a/primitives/consensus/common/src/block_import.rs +++ b/primitives/consensus/common/src/block_import.rs @@ -233,7 +233,7 @@ impl BlockImportParams { .ok_or(Error::NoIntermediate) .and_then(|value| { value.downcast::() - .map_err(|_| Error::NoIntermediate) + .map_err(|_| Error::InvalidIntermediate) }) } else { Err(Error::NoIntermediate) @@ -243,15 +243,17 @@ impl BlockImportParams { /// Get a reference to a given intermediate. pub fn intermediate(&self, key: &[u8]) -> Result<&T, Error> { self.intermediates.get(key) - .and_then(|value| value.downcast_ref::()) - .ok_or(Error::NoIntermediate) + .ok_or(Error::NoIntermediate)? + .downcast_ref::() + .ok_or(Error::InvalidIntermediate) } /// Get a mutable reference to a given intermediate. pub fn intermediate_mut(&mut self, key: &[u8]) -> Result<&mut T, Error> { self.intermediates.get_mut(key) - .and_then(|value| value.downcast_mut::()) - .ok_or(Error::NoIntermediate) + .ok_or(Error::NoIntermediate)? + .downcast_mut::() + .ok_or(Error::InvalidIntermediate) } } diff --git a/primitives/consensus/common/src/error.rs b/primitives/consensus/common/src/error.rs index 5a4f85ee67f03..c802831d6501f 100644 --- a/primitives/consensus/common/src/error.rs +++ b/primitives/consensus/common/src/error.rs @@ -32,8 +32,11 @@ pub enum Error { #[display(fmt="I/O terminated unexpectedly.")] IoTerminated, /// Intermediate missing. - #[display(fmt="Missing or invalid intermediate.")] + #[display(fmt="Missing intermediate.")] NoIntermediate, + /// Intermediate is of wrong type. + #[display(fmt="Invalid intermediate.")] + InvalidIntermediate, /// Unable to schedule wakeup. #[display(fmt="Timer error: {}", _0)] FaultyTimer(std::io::Error),