diff --git a/ethcore/client-traits/src/lib.rs b/ethcore/client-traits/src/lib.rs index 33d32c58f1f..88845f889fd 100644 --- a/ethcore/client-traits/src/lib.rs +++ b/ethcore/client-traits/src/lib.rs @@ -216,6 +216,9 @@ impl Tick for () {} pub trait BadBlocks { /// Returns a list of blocks that were recently not imported because they were invalid. fn bad_blocks(&self) -> Vec<(Unverified, String)>; + + /// Report a bad block + fn report_bad_block(&self, block: Bytes, message: String); } diff --git a/ethcore/engines/clique/src/block_state.rs b/ethcore/engines/clique/src/block_state.rs index f082ee8d32b..de605c89e28 100644 --- a/ethcore/engines/clique/src/block_state.rs +++ b/ethcore/engines/clique/src/block_state.rs @@ -131,6 +131,8 @@ impl CliqueBlockState { } // see https://github.com/ethereum/go-ethereum/blob/master/consensus/clique/clique.go#L474 + // + // NOTE(niklasad1): might return EthcoreError without `block bytes` fn verify(&self, header: &Header) -> Result { let creator = recover_creator(header)?.clone(); @@ -167,6 +169,8 @@ impl CliqueBlockState { } /// Verify and apply a new header to current state + // + // NOTE(niklasad1): might return EthcoreError without `block bytes` pub fn apply(&mut self, header: &Header, is_checkpoint: bool) -> Result { let creator = self.verify(header)?; self.recent_signers.push_front(creator); diff --git a/ethcore/engines/clique/src/lib.rs b/ethcore/engines/clique/src/lib.rs index 8843e07badd..acb1a8b2fff 100644 --- a/ethcore/engines/clique/src/lib.rs +++ b/ethcore/engines/clique/src/lib.rs @@ -263,6 +263,8 @@ impl Clique { } /// Construct an new state from given checkpoint header. + // + // NOTE(niklasad1): might return `BlockErrorWithData` without `block bytes` fn new_checkpoint_state(&self, header: &Header) -> Result { debug_assert_eq!(header.number() % self.epoch_length, 0); @@ -279,6 +281,8 @@ impl Clique { } /// Get `CliqueBlockState` for given header, backfill from last checkpoint if needed. + // + // NOTE(niklasad1): might return `BlockErrorWithData` without `block bytes` fn state(&self, header: &Header) -> Result { let mut block_state_by_hash = self.block_state_by_hash.write(); if let Some(state) = block_state_by_hash.get_mut(&header.hash()) { diff --git a/ethcore/engines/ethash/src/lib.rs b/ethcore/engines/ethash/src/lib.rs index 1f9223df78b..9e27b0c4b8a 100644 --- a/ethcore/engines/ethash/src/lib.rs +++ b/ethcore/engines/ethash/src/lib.rs @@ -175,6 +175,7 @@ impl Ethash { } } +// NOTE(niklasad1): might return `BlockError` without bytes fn verify_block_unordered(pow: &Arc, header: &Header) -> Result<(), Error> { let seal = EthashSeal::parse_seal(header.seal())?; @@ -186,20 +187,20 @@ fn verify_block_unordered(pow: &Arc, header: &Header) -> Result<( let mix = H256(result.mix_hash); let difficulty = ethash::boundary_to_difficulty(&H256(result.value)); trace!(target: "miner", "num: {num}, seed: {seed}, h: {h}, non: {non}, mix: {mix}, res: {res}", - num = header.number() as u64, - seed = H256(slow_hash_block_number(header.number() as u64)), + num = header.number(), + seed = H256(slow_hash_block_number(header.number())), h = header.bare_hash(), non = seal.nonce.to_low_u64_be(), mix = H256(result.mix_hash), res = H256(result.value)); if mix != seal.mix_hash { - return Err(From::from(BlockError::MismatchedH256SealElement(Mismatch { + return Err(Error::Block(BlockError::MismatchedH256SealElement(Mismatch { expected: mix, found: seal.mix_hash }))); } if &difficulty < header.difficulty() { - return Err(From::from(BlockError::InvalidProofOfWork(OutOfBounds { + return Err(Error::Block(BlockError::InvalidProofOfWork(OutOfBounds { min: Some(*header.difficulty()), max: None, found: difficulty @@ -331,11 +332,11 @@ impl Engine for Ethash { // TODO: consider removing these lines. let min_difficulty = self.ethash_params.minimum_difficulty; if header.difficulty() < &min_difficulty { - return Err(From::from(BlockError::DifficultyOutOfBounds(OutOfBounds { + return Err(Error::Block(BlockError::DifficultyOutOfBounds(OutOfBounds { min: Some(min_difficulty), max: None, - found: *header.difficulty(), - }))) + found: *header.difficulty() + }))); } let difficulty = ethash::boundary_to_difficulty(&H256(quick_get_difficulty( @@ -346,13 +347,12 @@ impl Engine for Ethash { ))); if &difficulty < header.difficulty() { - return Err(From::from(BlockError::InvalidProofOfWork(OutOfBounds { + return Err(Error::Block(BlockError::InvalidProofOfWork(OutOfBounds { min: Some(*header.difficulty()), max: None, found: difficulty }))); } - Ok(()) } @@ -363,7 +363,7 @@ impl Engine for Ethash { fn verify_block_family(&self, header: &Header, parent: &Header) -> Result<(), Error> { // we should not calculate difficulty for genesis blocks if header.number() == 0 { - return Err(From::from(BlockError::RidiculousNumber(OutOfBounds { + return Err(Error::Block(BlockError::RidiculousNumber(OutOfBounds { min: Some(1), max: None, found: header.number() @@ -376,7 +376,7 @@ impl Engine for Ethash { return Err(From::from(BlockError::InvalidDifficulty(Mismatch { expected: expected_difficulty, found: *header.difficulty() - }))) + }))); } Ok(()) @@ -672,8 +672,8 @@ mod tests { match verify_result { Err(Error::Block(BlockError::InvalidSealArity(_))) => {}, - Err(_) => { panic!("should be block seal-arity mismatch error (got {:?})", verify_result); }, - _ => { panic!("Should be error, got Ok"); }, + Err(_) => panic!("should be block seal-arity mismatch error (got {:?})", verify_result), + _ => panic!("Should be error, got Ok"), } } @@ -687,8 +687,8 @@ mod tests { match verify_result { Err(Error::Block(BlockError::DifficultyOutOfBounds(_))) => {}, - Err(_) => { panic!("should be block difficulty error (got {:?})", verify_result); }, - _ => { panic!("Should be error, got Ok"); }, + Err(_) => panic!("should be block difficulty error (got {:?})", verify_result), + _ => panic!("Should be error, got Ok"), } } @@ -703,8 +703,8 @@ mod tests { match verify_result { Err(Error::Block(BlockError::InvalidProofOfWork(_))) => {}, - Err(_) => { panic!("should be invalid proof of work error (got {:?})", verify_result); }, - _ => { panic!("Should be error, got Ok"); }, + Err(_) => panic!("should be invalid proof of work error (got {:?})", verify_result), + _ => panic!("Should be error, got Ok"), } } @@ -717,8 +717,8 @@ mod tests { match verify_result { Err(Error::Block(BlockError::InvalidSealArity(_))) => {}, - Err(_) => { panic!("should be block seal-arity mismatch error (got {:?})", verify_result); }, - _ => { panic!("Should be error, got Ok"); }, + Err(_) => panic!("should be block seal-arity mismatch error (got {:?})", verify_result), + _ => panic!("Should be error, got Ok"), } } @@ -742,8 +742,8 @@ mod tests { match verify_result { Err(Error::Block(BlockError::MismatchedH256SealElement(_))) => {}, - Err(_) => { panic!("should be invalid 256-bit seal fail (got {:?})", verify_result); }, - _ => { panic!("Should be error, got Ok"); }, + Err(_) => panic!("should be invalid 256-bit seal fail (got {:?})", verify_result), + _ => panic!("Should be error, got Ok"), } } @@ -758,8 +758,8 @@ mod tests { match verify_result { Err(Error::Block(BlockError::InvalidProofOfWork(_))) => {}, - Err(_) => { panic!("should be invalid proof-of-work fail (got {:?})", verify_result); }, - _ => { panic!("Should be error, got Ok"); }, + Err(_) => panic!("should be invalid proof-of-work fail (got {:?})", verify_result), + _ => panic!("Should be error, got Ok"), } } @@ -773,8 +773,8 @@ mod tests { match verify_result { Err(Error::Block(BlockError::RidiculousNumber(_))) => {}, - Err(_) => { panic!("should be invalid block number fail (got {:?})", verify_result); }, - _ => { panic!("Should be error, got Ok"); }, + Err(_) => panic!("should be invalid block number fail (got {:?})", verify_result), + _ => panic!("Should be error, got Ok"), } } @@ -790,8 +790,8 @@ mod tests { match verify_result { Err(Error::Block(BlockError::InvalidDifficulty(_))) => {}, - Err(_) => { panic!("should be invalid difficulty fail (got {:?})", verify_result); }, - _ => { panic!("Should be error, got Ok"); }, + Err(_) => panic!("should be invalid difficulty fail (got {:?})", verify_result), + _ => panic!("Should be error, got Ok"), } } diff --git a/ethcore/light/src/client/header_chain.rs b/ethcore/light/src/client/header_chain.rs index d21d705323b..165ebbfcb91 100644 --- a/ethcore/light/src/client/header_chain.rs +++ b/ethcore/light/src/client/header_chain.rs @@ -37,7 +37,7 @@ use common_types::{ Transition as EpochTransition, PendingTransition as PendingEpochTransition, }, - errors::{EthcoreError as Error, BlockError, EthcoreResult}, + errors::{EthcoreError as Error, BlockError}, header::Header, ids::BlockId, }; @@ -227,6 +227,9 @@ pub struct HeaderChain { impl HeaderChain { /// Create a new header chain given this genesis block and database to read from. + // + // NOTE(niklasad1): might return BlockError without `block bytes`, + // but this function is only called at startup so "ok". pub fn new( db: Arc, col: u32, @@ -240,7 +243,7 @@ impl HeaderChain { let decoded_header = spec.genesis_header(); let chain = if let Some(current) = db.get(col, CURRENT_KEY)? { - let curr : BestAndLatest = ::rlp::decode(¤t).expect("decoding db value failed"); + let curr: BestAndLatest = rlp::decode(¤t).expect("decoding db value failed"); let mut cur_number = curr.latest_num; let mut candidates = BTreeMap::new(); @@ -322,8 +325,12 @@ impl HeaderChain { // write the block in the DB. info!(target: "chain", "Inserting hardcoded block #{} in chain", decoded_header_num); - let pending = chain.insert_with_td(&mut batch, &decoded_header, - hardcoded_sync.total_difficulty, None)?; + let pending = chain.insert_with_td( + &mut batch, + &decoded_header, + hardcoded_sync.total_difficulty, + None + )?; // check that we have enough hardcoded CHT roots. avoids panicking later. let cht_num = cht::block_to_cht_number(decoded_header_num - 1) @@ -369,7 +376,7 @@ impl HeaderChain { transaction: &mut DBTransaction, header: &Header, transition_proof: Option>, - ) -> EthcoreResult { + ) -> Result { self.insert_inner(transaction, header, None, transition_proof) } @@ -382,7 +389,7 @@ impl HeaderChain { header: &Header, total_difficulty: U256, transition_proof: Option>, - ) -> EthcoreResult { + ) -> Result { self.insert_inner(transaction, header, Some(total_difficulty), transition_proof) } @@ -392,7 +399,7 @@ impl HeaderChain { header: &Header, total_difficulty: Option, transition_proof: Option>, - ) -> EthcoreResult { + ) -> Result { let hash = header.hash(); let number = header.number(); let parent_hash = *header.parent_hash(); @@ -420,8 +427,7 @@ impl HeaderChain { candidates.get(&(number - 1)) .and_then(|entry| entry.candidates.iter().find(|c| c.hash == parent_hash)) .map(|c| c.total_difficulty) - .ok_or_else(|| BlockError::UnknownParent(parent_hash)) - .map_err(Error::Block)? + .ok_or_else(|| BlockError::UnknownParent(parent_hash))? }; parent_td + *header.difficulty() diff --git a/ethcore/light/src/client/mod.rs b/ethcore/light/src/client/mod.rs index e835711e5ba..b657c8840d8 100644 --- a/ethcore/light/src/client/mod.rs +++ b/ethcore/light/src/client/mod.rs @@ -220,7 +220,7 @@ impl Client { /// Import a header to the queue for additional verification. pub fn import_header(&self, header: Header) -> EthcoreResult { - self.queue.import(header).map_err(|(e, _)| e) + self.queue.import(header) } /// Inquire about the status of a given header. diff --git a/ethcore/light/src/client/service.rs b/ethcore/light/src/client/service.rs index 0d901cdb83f..a3cd426f726 100644 --- a/ethcore/light/src/client/service.rs +++ b/ethcore/light/src/client/service.rs @@ -44,7 +44,6 @@ pub enum Error { } impl From for Error { - #[inline] fn from(err: CoreError) -> Error { Error::Core(err) } diff --git a/ethcore/src/block.rs b/ethcore/src/block.rs index 992437a1455..9e2f9a4d4c7 100644 --- a/ethcore/src/block.rs +++ b/ethcore/src/block.rs @@ -50,7 +50,7 @@ use hash::keccak; use rlp::{RlpStream, Encodable, encode_list}; use types::{ block::PreverifiedBlock, - errors::{EthcoreError as Error, BlockError}, + errors::{EthcoreError as Error, BlockError, BlockErrorWithData}, transaction::{SignedTransaction, Error as TransactionError}, header::Header, receipt::{Receipt, TransactionOutcome}, @@ -342,6 +342,8 @@ impl LockedBlock { /// Provide a valid seal in order to turn this into a `SealedBlock`. /// /// NOTE: This does not check the validity of `seal` with the engine. + // + // NOTE(niklasad1): might return `BlockError` without block bytes. pub fn seal(self, engine: &dyn Engine, seal: Vec) -> Result { let expected_seal_fields = engine.seal_fields(&self.header); let mut s = self; @@ -407,9 +409,7 @@ impl Drain for SealedBlock { /// Enact the block given by block header, transactions and uncles pub(crate) fn enact( - header: Header, - transactions: Vec, - uncles: Vec
, + block: PreverifiedBlock, engine: &dyn Engine, tracing: bool, db: StateDB, @@ -417,10 +417,15 @@ pub(crate) fn enact( last_hashes: Arc, factories: Factories, is_epoch_begin: bool, -) -> Result { +) -> Result<(LockedBlock, Bytes), Error> { // For trace log let trace_state = if log_enabled!(target: "enact", ::log::Level::Trace) { - Some(State::from_existing(db.boxed_clone(), parent.state_root().clone(), engine.account_start_nonce(parent.number() + 1), factories.clone())?) + Some(State::from_existing( + db.boxed_clone(), + *parent.state_root(), + engine.account_start_nonce(parent.number() + 1), + factories.clone() + )?) } else { None }; @@ -434,7 +439,7 @@ pub(crate) fn enact( last_hashes, // Engine such as Clique will calculate author from extra_data. // this is only important for executing contracts as the 'executive_author'. - engine.executive_author(&header)?, + engine.executive_author(&block.header)?, (3141562.into(), 31415620.into()), vec![], is_epoch_begin, @@ -448,14 +453,17 @@ pub(crate) fn enact( b.block.header.number(), root, env.author, author_balance); } - b.populate_from(&header); - b.push_transactions(transactions)?; + b.populate_from(&block.header); + b.push_transactions(block.transactions)?; - for u in uncles { - b.push_uncle(u)?; + for u in block.uncles { + if let Err(error) = b.push_uncle(u) { + return Err(Error::BadBlock(BlockErrorWithData { error, data: block.bytes })); + } } - b.close_and_lock() + let locked_block = b.close_and_lock()?; + Ok((locked_block, block.bytes)) } /// Enact the block given by `block_bytes` using `engine` on the database `db` with the given `parent` block header @@ -468,12 +476,10 @@ pub fn enact_verified( last_hashes: Arc, factories: Factories, is_epoch_begin: bool, -) -> Result { +) -> Result<(LockedBlock, Bytes), Error> { enact( - block.header, - block.transactions, - block.uncles, + block, engine, tracing, db, @@ -527,7 +533,7 @@ mod tests { { if ::log::max_level() >= ::log::Level::Trace { - let s = State::from_existing(db.boxed_clone(), parent.state_root().clone(), engine.account_start_nonce(parent.number() + 1), factories.clone())?; + let s = State::from_existing(db.boxed_clone(), *parent.state_root(), engine.account_start_nonce(parent.number() + 1), factories.clone())?; trace!(target: "enact", "num={}, root={}, author={}, author_balance={}\n", header.number(), s.root(), header.author(), s.balance(&header.author())?); } @@ -550,7 +556,9 @@ mod tests { b.push_transactions(transactions)?; for u in block.uncles { - b.push_uncle(u)?; + if let Err(error) = b.push_uncle(u) { + return Err(Error::BadBlock(BlockErrorWithData { error, data: block.bytes })); + } } b.close_and_lock() diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 55e0b1889df..04efd221ad1 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -118,7 +118,7 @@ use types::{ MAX_UNCLE_AGE, SealingState, }, - errors::{BlockError, EngineError, EthcoreError, EthcoreResult, ExecutionError, ImportError, SnapshotError}, + errors::{BlockError, BlockErrorWithData, EngineError, EthcoreError, EthcoreResult, ExecutionError, ImportError, SnapshotError}, filter::Filter, header::Header, ids::{BlockId, TraceId, TransactionId, UncleId}, @@ -299,7 +299,6 @@ impl Importer { for block in blocks { let header = block.header.clone(); - let bytes = block.bytes.clone(); let hash = header.hash(); let is_invalid = invalid_blocks.contains(header.parent_hash()); @@ -308,8 +307,8 @@ impl Importer { continue; } - match self.check_and_lock_block(&bytes, block, client) { - Ok((closed_block, pending)) => { + match self.check_and_lock_block(block, client) { + Ok((closed_block, bytes, pending)) => { imported_blocks.push(hash); let transactions_len = closed_block.transactions.len(); let route = self.commit_block(closed_block, &header, encoded::Block::new(bytes), pending, client); @@ -317,8 +316,10 @@ impl Importer { client.report.write().accrue_block(&header, transactions_len); }, Err(err) => { - self.bad_blocks.report(bytes, format!("{:?}", err)); invalid_blocks.insert(hash); + if let EthcoreError::BadBlock(BlockErrorWithData { error, data }) = err { + self.bad_blocks.report(data, error.to_string()); + } }, } } @@ -362,7 +363,11 @@ impl Importer { imported } - fn check_and_lock_block(&self, bytes: &[u8], block: PreverifiedBlock, client: &Client) -> EthcoreResult<(LockedBlock, Option)> { + fn check_and_lock_block( + &self, + block: PreverifiedBlock, + client: &Client + ) -> EthcoreResult<(LockedBlock, Bytes, Option)> { let engine = &*self.engine; let header = block.header.clone(); @@ -384,20 +389,22 @@ impl Importer { let chain = client.chain.read(); // Verify Block Family - let verify_family_result = verification::verify_block_family( - &header, + let verified_family_result = verification::verify_block_family( &parent, engine, - verification::FullFamilyParams { - block: &block, - block_provider: &**chain, - client - }, + block, + &**chain, + client ); - if let Err(e) = verify_family_result { - warn!(target: "client", "Stage 3 block verification failed for #{} ({})\nError: {:?}", header.number(), header.hash(), e); - return Err(e); + let verified_block = match verified_family_result { + Ok(verified_block) => verified_block, + Err(e) => { + warn!(target: "client", "Stage 3 block verification failed for #{} ({})\nError: {:?}", + header.number(), header.hash(), e + ); + return Err(e); + } }; let verify_external_result = engine.verify_block_external(&header); @@ -413,7 +420,7 @@ impl Importer { let is_epoch_begin = chain.epoch_transition(parent.number(), *header.parent_hash()).is_some(); let enact_result = enact_verified( - block, + verified_block, engine, client.tracedb.read().tracing_enabled(), db, @@ -423,8 +430,8 @@ impl Importer { is_epoch_begin, ); - let mut locked_block = match enact_result { - Ok(b) => b, + let (mut locked_block, bytes) = match enact_result { + Ok((l, b)) => (l, b), Err(e) => { warn!(target: "client", "Block import failed for #{} ({})\nError: {:?}", header.number(), header.hash(), e); return Err(e); @@ -441,20 +448,21 @@ impl Importer { } // Final Verification - if let Err(e) = verification::verify_block_final(&header, &locked_block.header) { - warn!(target: "client", "Stage 5 block verification failed for #{} ({})\nError: {:?}", header.number(), header.hash(), e); - return Err(e); + if let Err(error) = verification::verify_block_final(&header, &locked_block.header) { + warn!(target: "client", "Stage 5 block verification failed for #{} ({})\nError: {:?}", + header.number(), header.hash(), error); + return Err(EthcoreError::BadBlock(BlockErrorWithData { error, data: bytes })); } let pending = self.check_epoch_end_signal( &header, - bytes, + &bytes, &locked_block.receipts, locked_block.state.db(), client )?; - Ok((locked_block, pending)) + Ok((locked_block, bytes, pending)) } /// Import a block with transaction receipts. @@ -1449,7 +1457,10 @@ impl ImportBlock for Client { let status = self.block_status(BlockId::Hash(unverified.parent_hash())); if status == BlockStatus::Unknown { - return Err(EthcoreError::Block(BlockError::UnknownParent(unverified.parent_hash()))); + return Err(EthcoreError::BadBlock(BlockErrorWithData { + error: BlockError::UnknownParent(unverified.parent_hash()), + data: unverified.bytes + })); } let raw = if self.importer.block_queue.is_empty() { @@ -1465,16 +1476,8 @@ impl ImportBlock for Client { } Ok(hash) }, - // we only care about block errors (not import errors) - Err((EthcoreError::Block(e), Some(input))) => { - self.importer.bad_blocks.report(input.bytes, e.to_string()); - Err(EthcoreError::Block(e)) - }, - Err((EthcoreError::Block(e), None)) => { - error!(target: "client", "BlockError {} detected but it was missing raw_bytes of the block", e); - Err(EthcoreError::Block(e)) - } - Err((e, _input)) => Err(e), + // NOTE(niklasad1): block errors are reported by the `sync` module, to avoid clone. + err => err, } } @@ -1632,6 +1635,10 @@ impl BadBlocks for Client { fn bad_blocks(&self) -> Vec<(Unverified, String)> { self.importer.bad_blocks.bad_blocks() } + + fn report_bad_block(&self, block: Bytes, message: String) { + self.importer.bad_blocks.report(block, message) + } } impl BlockChainClient for Client { @@ -2225,7 +2232,10 @@ impl IoClient for Client { // (and attempt to acquire lock) let is_parent_pending = self.queued_ancient_blocks.read().0.contains(&parent_hash); if !is_parent_pending && !self.chain.read().is_known(&parent_hash) { - return Err(EthcoreError::Block(BlockError::UnknownParent(parent_hash))); + return Err(EthcoreError::BadBlock(BlockErrorWithData { + error: BlockError::UnknownParent(parent_hash), + data: unverified.bytes + })); } } @@ -2377,22 +2387,22 @@ impl ScheduleInfo for Client { impl ImportSealedBlock for Client { fn import_sealed_block(&self, block: SealedBlock) -> EthcoreResult { let start = Instant::now(); - let raw = block.rlp_bytes(); + let block_bytes_rlp = block.rlp_bytes(); let header = block.header.clone(); let hash = header.hash(); self.notify(|n| { - n.block_pre_import(&raw, &hash, header.difficulty()) + n.block_pre_import(&block_bytes_rlp, &hash, header.difficulty()) }); let route = { // Do a super duper basic verification to detect potential bugs - if let Err(e) = self.engine.verify_block_basic(&header) { - self.importer.bad_blocks.report( - raw, - format!("Detected an issue with locally sealed block: {}", e), - ); - return Err(e); - } + match self.engine.verify_block_basic(&header) { + Err(EthcoreError::Block(error)) => { + return Err(EthcoreError::BadBlock(BlockErrorWithData { error, data: block_bytes_rlp })); + }, + Err(err) => return Err(err), + Ok(_) => (), + }; // scope for self.import_lock let _import_lock = self.importer.import_lock.lock(); @@ -2477,8 +2487,12 @@ impl client_traits::EngineClient for Client { fn submit_seal(&self, block_hash: H256, seal: Vec) { let import = self.importer.miner.submit_seal(block_hash, seal) .and_then(|block| self.import_sealed_block(block)); - if let Err(err) = import { - warn!(target: "poa", "Wrong internal seal submission! {:?}", err); + + if let Err(e) = import { + warn!(target: "poa", "Wrong internal seal submission! {:?}", e); + if let EthcoreError::BadBlock(BlockErrorWithData { data, error }) = e { + self.importer.bad_blocks.report(data, error.to_string()); + } } } diff --git a/ethcore/src/test_helpers/test_client.rs b/ethcore/src/test_helpers/test_client.rs index 46cd854b3ec..d97b2310c9f 100644 --- a/ethcore/src/test_helpers/test_client.rs +++ b/ethcore/src/test_helpers/test_client.rs @@ -670,6 +670,10 @@ impl BadBlocks for TestBlockChainClient { }, "Invalid block".into()) ] } + + fn report_bad_block(&self, _bytes: Bytes, _message: String) { + unimplemented!("not implemented for TestBlockChainClient") + } } impl BlockChainClient for TestBlockChainClient { diff --git a/ethcore/sync/src/block_sync.rs b/ethcore/sync/src/block_sync.rs index 47d62ad3ca9..a4ae2cd2e7a 100644 --- a/ethcore/sync/src/block_sync.rs +++ b/ethcore/sync/src/block_sync.rs @@ -28,7 +28,7 @@ use crate::{ }; use ethereum_types::H256; -use log::{debug, trace}; +use log::{error, debug, trace}; use network::{client_version::ClientCapabilities, PeerId}; use rlp::Rlp; use parity_util_mem::MallocSizeOf; @@ -36,7 +36,7 @@ use common_types::{ BlockNumber, block_status::BlockStatus, ids::BlockId, - errors::{EthcoreError, BlockError, ImportError}, + errors::{EthcoreError, BlockError, BlockErrorWithData, ImportError}, }; const MAX_HEADERS_TO_REQUEST: usize = 128; @@ -560,6 +560,11 @@ impl BlockDownloader { }; match result { + Ok(_) => { + trace_sync!(self, "Block queued {:?}", h); + imported.insert(h); + self.block_imported(&h, number, &parent); + }, Err(EthcoreError::Import(ImportError::AlreadyInChain)) => { let is_canonical = if io.chain().block_hash(BlockId::Number(number)).is_some() { "canoncial" @@ -575,22 +580,17 @@ impl BlockDownloader { imported.insert(h.clone()); self.block_imported(&h, number, &parent); }, - Ok(_) => { - trace_sync!(self, "Block queued {:?}", h); - imported.insert(h); - self.block_imported(&h, number, &parent); - }, - Err(EthcoreError::Block(BlockError::UnknownParent(_))) if allow_out_of_order => { - break; - }, - Err(EthcoreError::Block(BlockError::UnknownParent(_))) => { - trace_sync!(self, "Unknown new block parent, restarting sync"); + Err(EthcoreError::BadBlock(BlockErrorWithData { error, data })) => { + io.chain().report_bad_block(data, error.to_string()); + self.handle_block_error(error, &h, &mut download_action, allow_out_of_order); break; - }, - Err(EthcoreError::Block(BlockError::TemporarilyInvalid(_))) => { - debug_sync!(self, "Block temporarily invalid: {:?}, restarting sync", h); + } + // NOTE(niklasad1): this branch should be unreachable + Err(EthcoreError::Block(err)) => { + error!(target: "sync", "received `EthcoreError::Block`, bug should be `EthcoreError::BadBlock`"); + self.handle_block_error(err, &h, &mut download_action, allow_out_of_order); break; - }, + } Err(EthcoreError::FullQueue(limit)) => { debug_sync!(self, "Block import queue full ({}), restarting sync", limit); download_action = DownloadAction::Reset; @@ -622,6 +622,28 @@ impl BlockDownloader { self.round_parents.pop_front(); } } + + fn handle_block_error( + &self, + error: BlockError, + block_hash: &H256, + download_action: &mut DownloadAction, + allow_out_of_order: bool + ) { + match error { + BlockError::UnknownParent(_) if allow_out_of_order => {}, + BlockError::UnknownParent(_) => { + trace_sync!(self, "Unknown new block parent, restarting sync"); + }, + BlockError::TemporarilyInvalid(_) => { + debug_sync!(self, "Block temporarily invalid: {:?}, restarting sync", block_hash); + }, + e => { + debug_sync!(self, "Bad block {:?} : {:?}", block_hash, e); + *download_action = DownloadAction::Reset; + } + } + } } // Determines if the first argument matches an ordered subset of the second, according to some predicate. diff --git a/ethcore/sync/src/chain/handler.rs b/ethcore/sync/src/chain/handler.rs index 2067c2c9e06..c9d09bdccb6 100644 --- a/ethcore/sync/src/chain/handler.rs +++ b/ethcore/sync/src/chain/handler.rs @@ -49,7 +49,7 @@ use common_types::{ BlockNumber, block_status::BlockStatus, ids::BlockId, - errors::{EthcoreError, ImportError, BlockError}, + errors::{EthcoreError, ImportError, BlockError, BlockErrorWithData}, verification::Unverified, snapshot::{ManifestData, RestorationStatus}, }; @@ -174,22 +174,29 @@ impl SyncHandler { return Err(DownloaderImportError::Invalid); } match io.chain().import_block(block) { - Err(EthcoreError::Import(ImportError::AlreadyInChain)) => { - trace!(target: "sync", "New block already in chain {:?}", hash); - }, - Err(EthcoreError::Import(ImportError::AlreadyQueued)) => { - trace!(target: "sync", "New block already queued {:?}", hash); - }, Ok(_) => { // abort current download of the same block sync.complete_sync(io); sync.new_blocks.mark_as_known(&hash, number); trace!(target: "sync", "New block queued {:?} ({})", hash, number); }, + Err(EthcoreError::Import(ImportError::AlreadyInChain)) => { + trace!(target: "sync", "New block already in chain {:?}", hash); + }, + Err(EthcoreError::Import(ImportError::AlreadyQueued)) => { + trace!(target: "sync", "New block already queued {:?}", hash); + }, Err(EthcoreError::Block(BlockError::UnknownParent(p))) => { + warn!(target: "sync", "Received EthcoreError::Block; bug should be `EthcoreError::BadBlock`"); unknown = true; trace!(target: "sync", "New block with unknown parent ({:?}) {:?}", p, hash); }, + Err(EthcoreError::BadBlock(BlockErrorWithData { error, .. })) => { + if let BlockError::UnknownParent(p) = error { + unknown = true; + trace!(target: "sync", "New block with unknown parent ({:?}) {:?}", p, hash); + } + } Err(e) => { debug!(target: "sync", "Bad new block {:?} : {:?}", hash, e); return Err(DownloaderImportError::Invalid); diff --git a/ethcore/sync/src/chain/supplier.rs b/ethcore/sync/src/chain/supplier.rs index 6596506ed71..d1a9f4ea1c1 100644 --- a/ethcore/sync/src/chain/supplier.rs +++ b/ethcore/sync/src/chain/supplier.rs @@ -263,7 +263,7 @@ impl SyncSupplier { fn return_node_data(io: &dyn SyncIo, r: &Rlp, peer_id: PeerId) -> RlpResponseResult { let payload_soft_limit = io.payload_soft_limit(); // 4Mb let mut count = r.item_count().unwrap_or(0); - trace!(target: "sync", "{} -> GetNodeData: {} entries requested", peer_id, count); + warn!(target: "sync", "{} -> GetNodeData: {} entries requested", peer_id, count); if count == 0 { debug!(target: "sync", "Empty GetNodeData request, ignoring."); return Ok(None); @@ -293,7 +293,7 @@ impl SyncSupplier { added += 1; } } - trace!(target: "sync", "{} -> GetNodeData: returning {}/{} entries ({} bytes total in {:?})", + warn!(target: "sync", "{} -> GetNodeData: returning {}/{} entries ({} bytes total in {:?})", peer_id, added, count, total_bytes, total_elpsd); let mut rlp = RlpStream::new_list(added); for d in data { diff --git a/ethcore/types/src/engines/mod.rs b/ethcore/types/src/engines/mod.rs index c1ce9807825..dba16c66574 100644 --- a/ethcore/types/src/engines/mod.rs +++ b/ethcore/types/src/engines/mod.rs @@ -56,6 +56,8 @@ pub struct EthashSeal { impl EthashSeal { /// Tries to parse rlp encoded bytes as an Ethash/Clique seal. + // + // NOTE(niklasad1): might return `BlockErrorWithData` without `block bytes` pub fn parse_seal>(seal: &[T]) -> Result { if seal.len() != 2 { Err(EthcoreError::Block(BlockError::InvalidSealArity(Mismatch { diff --git a/ethcore/types/src/errors/ethcore_error.rs b/ethcore/types/src/errors/ethcore_error.rs index 1a99c65dcd4..eece13e24e6 100644 --- a/ethcore/types/src/errors/ethcore_error.rs +++ b/ethcore/types/src/errors/ethcore_error.rs @@ -17,11 +17,13 @@ //! General error types for use in ethcore. use std::{error, fmt}; + +use bytes::Bytes; use derive_more::{Display, From}; use ethereum_types::{U256, U512}; use ethtrie::TrieError; -use parity_snappy::InvalidInput; use parity_crypto::publickey::Error as EthPublicKeyCryptoError; +use parity_snappy::InvalidInput; use errors::{BlockError, EngineError, ImportError, SnapshotError}; use transaction::Error as TransactionError; @@ -29,6 +31,16 @@ use transaction::Error as TransactionError; /// Ethcore Result pub type EthcoreResult = Result; +/// Block error type +#[derive(Clone, Display, Debug)] +#[display(fmt = "Bad block failed because of: {}, raw rlp bytes: {:?}", error, data)] +pub struct BlockErrorWithData { + /// Error. + pub error: BlockError, + /// Raw rlp bytes of the block. + pub data: Bytes, +} + /// Ethcore Error #[derive(Debug, Display, From)] pub enum EthcoreError { @@ -43,7 +55,7 @@ pub enum EthcoreError { Io(ethcore_io::IoError), /// Error concerning the Rust standard library's IO subsystem. #[display(fmt = "Std Io error: {}", _0)] - StdIo(::std::io::Error), + StdIo(std::io::Error), /// Error concerning TrieDBs. #[display(fmt = "Trie error: {}", _0)] Trie(TrieError), @@ -51,8 +63,11 @@ pub enum EthcoreError { #[display(fmt = "Execution error: {}", _0)] Execution(ExecutionError), /// Error concerning block processing. - #[display(fmt = "Block error: {}", _0)] + #[display(fmt = "{}", _0)] Block(BlockError), + /// TODO(niklasad1): document + #[display(fmt = "{}", _0)] + BadBlock(BlockErrorWithData), /// Error concerning transaction processing. #[display(fmt = "Transaction error: {}", _0)] Transaction(TransactionError), @@ -91,6 +106,7 @@ impl error::Error for EthcoreError { Trie(e) => Some(e), Execution(e) => Some(e), Block(e) => Some(e), + BadBlock(e) => Some(&e.error), Transaction(e) => Some(e), Snappy(e) => Some(e), Engine(e) => Some(e), @@ -173,6 +189,7 @@ impl From> for ExecutionError { ExecutionError::Internal(format!("{:?}", err)) } } + impl From for ExecutionError { fn from(err: TrieError) -> Self { ExecutionError::Internal(format!("{:?}", err)) diff --git a/ethcore/types/src/errors/mod.rs b/ethcore/types/src/errors/mod.rs index f0f8fa77f14..eaf6097ec95 100644 --- a/ethcore/types/src/errors/mod.rs +++ b/ethcore/types/src/errors/mod.rs @@ -24,6 +24,6 @@ mod snapshot_error; pub use self::{ block_error::{BlockError, ImportError}, engine_error::EngineError, - ethcore_error::{EthcoreError, ExecutionError, EthcoreResult}, + ethcore_error::{EthcoreError, ExecutionError, EthcoreResult, BlockErrorWithData}, snapshot_error::SnapshotError, }; diff --git a/ethcore/verification/benches/verification.rs b/ethcore/verification/benches/verification.rs index 81477cc48c0..4a817d21484 100644 --- a/ethcore/verification/benches/verification.rs +++ b/ethcore/verification/benches/verification.rs @@ -19,7 +19,7 @@ use std::collections::BTreeMap; use common_types::verification::Unverified; -use criterion::{Criterion, criterion_group, criterion_main}; +use criterion::{BatchSize, Criterion, criterion_group, criterion_main}; use ethash::{EthashParams, Ethash}; use ethereum_types::U256; use ethcore::test_helpers::TestBlockChainClient; @@ -27,11 +27,19 @@ use spec::new_constantinople_test_machine; use tempdir::TempDir; use ::verification::{ - FullFamilyParams, verification, test_helpers::TestBlockChain, }; +/// Proof +const PROOF: &str = "bytes from disk are ok"; +/// A fairly large block (32kb) with one uncle +const RLP_8481476: &[u8] = include_bytes!("./8481476-one-uncle.rlp"); +/// Parent of #8481476 +const RLP_8481475: &[u8] = include_bytes!("./8481475.rlp"); +/// Parent of the uncle in #8481476 +const RLP_8481474: &[u8] = include_bytes!("./8481474-parent-to-uncle.rlp"); + // These are current production values. Needed when using real blocks. fn ethash_params() -> EthashParams { EthashParams { @@ -81,65 +89,64 @@ fn build_ethash() -> Ethash { ) } -fn block_verification(c: &mut Criterion) { - const PROOF: &str = "bytes from disk are ok"; +fn build_block(rlp: &[u8]) -> Unverified { + Unverified::from_rlp(rlp.to_vec()).expect("bytes from disk are ok; qed") +} +fn block_verification(c: &mut Criterion) { let ethash = build_ethash(); - // A fairly large block (32kb) with one uncle - let rlp_8481476 = include_bytes!("./8481476-one-uncle.rlp").to_vec(); - // Parent of #8481476 - let rlp_8481475 = include_bytes!("./8481475.rlp").to_vec(); - // Parent of the uncle in #8481476 - let rlp_8481474 = include_bytes!("./8481474-parent-to-uncle.rlp").to_vec(); - // Phase 1 verification c.bench_function("verify_block_basic", |b| { - let block = Unverified::from_rlp(rlp_8481476.clone()).expect(PROOF); - b.iter(|| { - assert!(verification::verify_block_basic( - &block, - ðash, - true - ).is_ok()); - }) + b.iter_batched(|| build_block(RLP_8481476), |block| { + assert!(verification::verify_block_basic( + block, + ðash, + true + ).is_ok()) + }, + BatchSize::SmallInput + ) }); // Phase 2 verification c.bench_function("verify_block_unordered", |b| { - let block = Unverified::from_rlp(rlp_8481476.clone()).expect(PROOF); - b.iter( || { - assert!(verification::verify_block_unordered( - block.clone(), - ðash, - true - ).is_ok()); - }) + b.iter_batched(|| build_block(RLP_8481476), |block| { + assert!(verification::verify_block_unordered( + block, + ðash, + true + ).is_ok()) + }, + BatchSize::SmallInput + ) }); // Phase 3 verification - let block = Unverified::from_rlp(rlp_8481476.clone()).expect(PROOF); - let preverified = verification::verify_block_unordered(block, ðash, true).expect(PROOF); - let parent = Unverified::from_rlp(rlp_8481475.clone()).expect(PROOF); + let parent = build_block(RLP_8481475); let mut block_provider = TestBlockChain::new(); - block_provider.insert(rlp_8481476.clone()); // block to verify - block_provider.insert(rlp_8481475.clone()); // parent - block_provider.insert(rlp_8481474.clone()); // uncle's parent + block_provider.insert(RLP_8481476.to_vec()); // block to verify + block_provider.insert(RLP_8481475.to_vec()); // parent + block_provider.insert(RLP_8481474.to_vec()); // uncle's parent let client = TestBlockChainClient::default(); - c.bench_function("verify_block_family", |b| { - b.iter(|| { - let full = FullFamilyParams { block: &preverified, block_provider: &block_provider, client: &client }; - if let Err(e) = verification::verify_block_family::( - &preverified.header, - &parent.header, - ðash, - full, - ) { - panic!("verify_block_family (full) ERROR: {:?}", e) - } - }); + c.bench_function("verify_block_family (full)", |b| { + b.iter_batched( + || { + verification::verify_block_unordered(build_block(RLP_8481476), ðash, true).expect(PROOF) + }, + |preverified| { + assert!(verification::verify_block_family::( + &parent.header, + ðash, + preverified, + &block_provider, + &client + ).is_ok()) + }, + BatchSize::SmallInput + ) }); } diff --git a/ethcore/verification/src/queue/kind.rs b/ethcore/verification/src/queue/kind.rs index 546df2d5a50..3b2642c3335 100644 --- a/ethcore/verification/src/queue/kind.rs +++ b/ethcore/verification/src/queue/kind.rs @@ -16,11 +16,9 @@ //! Definition of valid items for the verification queue. +use ethereum_types::{H256, U256}; use engine::Engine; - use parity_util_mem::MallocSizeOf; -use ethereum_types::{H256, U256}; - use common_types::errors::EthcoreError as Error; pub use self::blocks::Blocks; @@ -62,14 +60,7 @@ pub trait Kind: 'static + Sized + Send + Sync { type Verified: Sized + Send + BlockLike + MallocSizeOf; /// Attempt to create the `Unverified` item from the input. - /// - /// The return type is quite complex because in some scenarios the input - /// is needed (typically for BlockError) to get the raw block bytes without cloning them - fn create( - input: Self::Input, - engine: &dyn Engine, - check_seal: bool - ) -> Result)>; + fn create(input: Self::Input, engine: &dyn Engine, check_seal: bool) -> Result; /// Attempt to verify the `Unverified` item using the given engine. fn verify(unverified: Self::Unverified, engine: &dyn Engine, check_seal: bool) -> Result; @@ -82,7 +73,7 @@ pub mod blocks { use engine::Engine; use common_types::{ block::PreverifiedBlock, - errors::{EthcoreError as Error, BlockError}, + errors::{EthcoreError as Error, BlockError, BlockErrorWithData}, verification::Unverified, }; use log::{debug, warn}; @@ -98,20 +89,19 @@ pub mod blocks { type Unverified = Unverified; type Verified = PreverifiedBlock; - fn create( - input: Self::Input, - engine: &dyn Engine, - check_seal: bool - ) -> Result)> { - match verify_block_basic(&input, engine, check_seal) { - Ok(()) => Ok(input), - Err(Error::Block(BlockError::TemporarilyInvalid(oob))) => { - debug!(target: "client", "Block received too early {}: {:?}", input.hash(), oob); - Err((BlockError::TemporarilyInvalid(oob).into(), Some(input))) + fn create(input: Self::Input, engine: &dyn Engine, check_seal: bool) -> Result { + let hash = input.hash(); + match verify_block_basic(input, engine, check_seal) { + Ok(input) => Ok(input), + Err(Error::BadBlock(BlockErrorWithData { error, data })) => { + if let BlockError::TemporarilyInvalid(ref oob) = error { + debug!(target: "client", "Block received too early {}: {:?}", hash, oob); + } + Err(Error::BadBlock(BlockErrorWithData { error, data })) }, Err(e) => { - warn!(target: "client", "Stage 1 block verification failed for {}: {:?}", input.hash(), e); - Err((e, Some(input))) + warn!(target: "client", "Stage 1 block verification failed for {}: {:?}", hash, e); + Err(e) } } } @@ -193,17 +183,13 @@ pub mod headers { type Unverified = Header; type Verified = Header; - fn create( - input: Self::Input, - engine: &dyn Engine, - check_seal: bool - ) -> Result)> { + fn create(input: Self::Input, engine: &dyn Engine, check_seal: bool) -> Result { let res = verify_header_params(&input, engine, check_seal) - .and_then(|_| verify_header_time(&input)); + .and_then(|_| Ok(verify_header_time(&input)))?; match res { Ok(_) => Ok(input), - Err(e) => Err((e, Some(input))), + Err(error) => Err(error.into()), } } diff --git a/ethcore/verification/src/queue/mod.rs b/ethcore/verification/src/queue/mod.rs index 31d205c798c..bbd4fde5dd5 100644 --- a/ethcore/verification/src/queue/mod.rs +++ b/ethcore/verification/src/queue/mod.rs @@ -25,7 +25,7 @@ use std::collections::{VecDeque, HashSet, HashMap}; use common_types::{ block_status::BlockStatus, io_message::ClientIoMessage, - errors::{BlockError, EthcoreError as Error, ImportError}, + errors::{BlockError, EthcoreError as Error, ImportError, BlockErrorWithData}, verification::VerificationQueueInfo as QueueInfo, }; use ethcore_io::*; @@ -467,31 +467,29 @@ impl VerificationQueue { } /// Add a block to the queue. - // - // TODO: #11403 - rework `EthcoreError::Block` to include raw bytes of the error cause - pub fn import(&self, input: K::Input) -> Result)> { + pub fn import(&self, input: K::Input) -> Result { let hash = input.hash(); let raw_hash = input.raw_hash(); { if self.processing.read().contains_key(&hash) { - return Err((Error::Import(ImportError::AlreadyQueued), Some(input))); + return Err(Error::Import(ImportError::AlreadyQueued)); } let mut bad = self.verification.bad.lock(); if bad.contains(&hash) || bad.contains(&raw_hash) { - return Err((Error::Import(ImportError::KnownBad), Some(input))); + return Err(Error::Import(ImportError::KnownBad)); } if bad.contains(&input.parent_hash()) { bad.insert(hash); - return Err((Error::Import(ImportError::KnownBad), Some(input))); + return Err(Error::Import(ImportError::KnownBad)); } } match K::create(input, &*self.engine, self.verification.check_seal) { Ok(item) => { if self.processing.write().insert(hash, item.difficulty()).is_some() { - return Err((Error::Import(ImportError::AlreadyQueued), None)); + return Err(Error::Import(ImportError::AlreadyQueued)); } self.verification.sizes.unverified.fetch_add(item.malloc_size_of(), AtomicOrdering::SeqCst); { @@ -502,25 +500,18 @@ impl VerificationQueue { self.more_to_verify.notify_all(); Ok(hash) }, - Err((err, input)) => { + Err(err) => { match err { - // Don't mark future blocks as bad. - Error::Block(BlockError::TemporarilyInvalid(_)) => {}, - // If the transaction root or uncles hash is invalid, it doesn't necessarily mean - // that the header is invalid. We might have just received a malformed block body, - // so we shouldn't put the header hash to `bad`. - // - // We still put the entire `Item` hash to bad, so that we can early reject - // the items that are malformed. - Error::Block(BlockError::InvalidTransactionsRoot(_)) | - Error::Block(BlockError::InvalidUnclesHash(_)) => { - self.verification.bad.lock().insert(raw_hash); - }, - _ => { - self.verification.bad.lock().insert(hash); + Error::Block(err) => { + self.handle_block_error(&err, hash, raw_hash); + Err(Error::Block(err)) } + Error::BadBlock(BlockErrorWithData { error, data }) => { + self.handle_block_error(&error, hash, raw_hash); + Err(Error::BadBlock(BlockErrorWithData { error, data })) + }, + err => Err(err), } - Err((err, input)) } } } @@ -719,6 +710,26 @@ impl VerificationQueue { *self.state.0.lock() = State::Work(target); self.state.1.notify_all(); } + + fn handle_block_error(&self, err: &BlockError, hash: H256, raw_hash: H256) { + match err { + // Don't mark future blocks as bad. + BlockError::TemporarilyInvalid(_) => {}, + // If the transaction root or uncles hash is invalid, it doesn't necessarily mean + // that the header is invalid. We might have just received a malformed block body, + // so we shouldn't put the header hash to `bad`. + // + // We still put the entire `Item` hash to bad, so that we can early reject + // the items that are malformed. + BlockError::InvalidTransactionsRoot(_) | + BlockError::InvalidUnclesHash(_) => { + self.verification.bad.lock().insert(raw_hash); + }, + _ => { + self.verification.bad.lock().insert(hash); + } + }; + } } impl Drop for VerificationQueue { @@ -760,7 +771,6 @@ mod tests { view, views::BlockView, }; - use spec; // create a test block queue. // auto_scaling enables verifier adjustment. @@ -809,13 +819,9 @@ mod tests { let duplicate_import = queue.import(new_unverified(get_good_dummy_block())); match duplicate_import { - Err(e) => { - match e { - (EthcoreError::Import(ImportError::AlreadyQueued), _) => {}, - _ => { panic!("must return AlreadyQueued error"); } - } - } - Ok(_) => { panic!("must produce error"); } + Err(EthcoreError::Import(ImportError::AlreadyQueued)) => {}, + Err(_) => panic!("must return AlreadyQueued error"), + Ok(_) => panic!("must produce error"), } } diff --git a/ethcore/verification/src/verification.rs b/ethcore/verification/src/verification.rs index b67cc127b23..839207a6070 100644 --- a/ethcore/verification/src/verification.rs +++ b/ethcore/verification/src/verification.rs @@ -32,11 +32,12 @@ use unexpected::{Mismatch, OutOfBounds}; use blockchain::BlockProvider; use call_contract::CallContract; use client_traits::BlockInfo; +use ethereum_types::U256; use engine::Engine; use common_types::{ BlockNumber, header::Header, - errors::{EthcoreError as Error, BlockError}, + errors::{EthcoreError as Error, BlockError, BlockErrorWithData}, engines::MAX_UNCLE_AGE, block::PreverifiedBlock, verification::Unverified, @@ -45,27 +46,52 @@ use common_types::{ use time_utils::CheckedSystemTime; /// Phase 1 quick block verification. Only does checks that are cheap. Operates on a single block -pub fn verify_block_basic(block: &Unverified, engine: &dyn Engine, check_seal: bool) -> Result<(), Error> { - verify_header_params(&block.header, engine, check_seal)?; - verify_header_time(&block.header)?; - verify_block_integrity(block)?; +pub fn verify_block_basic(block: Unverified, engine: &dyn Engine, check_seal: bool) -> Result { + if let Err(error) = verify_header_params(&block.header, engine, check_seal) { + return Err(Error::BadBlock(BlockErrorWithData { error, data: block.bytes })); + } + + if let Err(error) = verify_header_time(&block.header) { + return Err(Error::BadBlock(BlockErrorWithData { error, data: block.bytes })); + } + + let block = verify_block_integrity(block)?; if check_seal { - engine.verify_block_basic(&block.header)?; + match engine.verify_block_basic(&block.header) { + Err(Error::Block(error)) => { + return Err(Error::BadBlock(BlockErrorWithData { error, data: block.bytes })); + } + Err(err) => return Err(err), + _ => {}, + }; } for uncle in &block.uncles { - verify_header_params(uncle, engine, check_seal)?; + if let Err(error) = verify_header_params(uncle, engine, check_seal) { + return Err(Error::BadBlock(BlockErrorWithData { error, data: block.bytes })); + } if check_seal { - engine.verify_block_basic(uncle)?; + match engine.verify_block_basic(uncle) { + Err(Error::Block(error)) => { + return Err(Error::BadBlock(BlockErrorWithData { error, data: block.bytes })); + } + Err(err) => return Err(err), + _ => {}, + }; } } if let Some(gas_limit) = engine.gas_limit_override(&block.header) { if *block.header.gas_limit() != gas_limit { - return Err(From::from(BlockError::InvalidGasLimit( - OutOfBounds { min: Some(gas_limit), max: Some(gas_limit), found: *block.header.gas_limit() } - ))); + return Err(Error::BadBlock(BlockErrorWithData { + error: BlockError::InvalidGasLimit(OutOfBounds { + min: Some(gas_limit), + max: Some(gas_limit), + found: *block.header.gas_limit() + }), + data: block.bytes + })); } } @@ -73,7 +99,7 @@ pub fn verify_block_basic(block: &Unverified, engine: &dyn Engine, check_seal: b engine.verify_transaction_basic(t, &block.header)?; } - Ok(()) + Ok(block) } /// Phase 2 verification. Perform costly checks such as transaction signatures and block nonce for ethash. @@ -87,26 +113,28 @@ pub fn verify_block_unordered(block: Unverified, engine: &dyn Engine, check_seal engine.verify_block_unordered(uncle)?; } } - // Verify transactions. - let nonce_cap = if header.number() >= engine.params().dust_protection_transition { - Some((engine.params().nonce_cap_increment * header.number()).into()) - } else { - None - }; + // Verify transactions. let transactions = block.transactions .into_iter() .map(|t| { - let t = t.verify_unordered()?; - if let Some(max_nonce) = nonce_cap { - if t.nonce >= max_nonce { - return Err(BlockError::TooManyTransactions(t.sender()).into()); - } - } - Ok(t) + Ok(t.verify_unordered()?) }) .collect::, Error>>()?; + // check if nonce is too high + if header.number() >= engine.params().dust_protection_transition { + let max_nonce = U256::from(engine.params().nonce_cap_increment) * U256::from(header.number()); + for t in &transactions { + if t.nonce >= max_nonce { + return Err(Error::BadBlock(BlockErrorWithData { + error: BlockError::TooManyTransactions(t.sender()).into(), + data: block.bytes, + })); + } + } + } + Ok(PreverifiedBlock { header, transactions, @@ -118,7 +146,7 @@ pub fn verify_block_unordered(block: Unverified, engine: &dyn Engine, check_seal /// Parameters for full verification of block family pub struct FullFamilyParams<'a, C: BlockInfo + CallContract + 'a> { /// Preverified block - pub block: &'a PreverifiedBlock, + pub block: PreverifiedBlock, /// Block provider to use during verification pub block_provider: &'a dyn BlockProvider, @@ -129,36 +157,46 @@ pub struct FullFamilyParams<'a, C: BlockInfo + CallContract + 'a> { /// Phase 3 verification. Check block information against parent and uncles. pub fn verify_block_family( - header: &Header, parent: &Header, engine: &dyn Engine, - params: FullFamilyParams -) -> Result<(), Error> { + block: PreverifiedBlock, + block_provider: &dyn BlockProvider, + client: &C +) -> Result { // TODO: verify timestamp - verify_parent(&header, &parent, engine)?; - engine.verify_block_family(&header, &parent)?; - verify_uncles(params.block, params.block_provider, engine)?; + if let Err(error) = verify_parent(&block.header, &parent, engine) { + return Err(Error::BadBlock(BlockErrorWithData { error, data: block.bytes })); + } + engine.verify_block_family(&block.header, &parent)?; + let block = verify_uncles(block, block_provider, engine)?; - for tx in ¶ms.block.transactions { + for tx in &block.transactions { // transactions are verified against the parent header since the current // state wasn't available when the tx was created - engine.machine().verify_transaction(tx, parent, params.client)?; + engine.machine().verify_transaction(tx, parent, client)?; } - Ok(()) + Ok(block) } -fn verify_uncles(block: &PreverifiedBlock, bc: &dyn BlockProvider, engine: &dyn Engine) -> Result<(), Error> { +fn verify_uncles( + block: PreverifiedBlock, + bc: &dyn BlockProvider, + engine: &dyn Engine +) -> Result { let header = &block.header; let num_uncles = block.uncles.len(); let max_uncles = engine.maximum_uncle_count(header.number()); if num_uncles != 0 { if num_uncles > max_uncles { - return Err(From::from(BlockError::TooManyUncles(OutOfBounds { - min: None, - max: Some(max_uncles), - found: num_uncles, - }))); + return Err(Error::BadBlock(BlockErrorWithData { + error: BlockError::TooManyUncles(OutOfBounds { + min: None, + max: Some(max_uncles), + found: num_uncles, + }), + data: block.bytes, + })); } let mut excluded = HashSet::new(); @@ -180,11 +218,17 @@ fn verify_uncles(block: &PreverifiedBlock, bc: &dyn BlockProvider, engine: &dyn let mut verified = HashSet::new(); for uncle in &block.uncles { if excluded.contains(&uncle.hash()) { - return Err(From::from(BlockError::UncleInChain(uncle.hash()))) + return Err(Error::BadBlock(BlockErrorWithData { + error: BlockError::UncleInChain(uncle.hash()).into(), + data: block.bytes, + })); } if verified.contains(&uncle.hash()) { - return Err(From::from(BlockError::DuplicateUncle(uncle.hash()))) + return Err(Error::BadBlock(BlockErrorWithData { + error: BlockError::DuplicateUncle(uncle.hash()).into(), + data: block.bytes, + })); } // uncle.number() needs to be within specific number range which is @@ -197,11 +241,14 @@ fn verify_uncles(block: &PreverifiedBlock, bc: &dyn BlockProvider, engine: &dyn uncle.number() + MAX_UNCLE_AGE >= header.number() { header.number() - uncle.number() } else { - return Err(BlockError::UncleOutOfBounds(OutOfBounds { - min: Some(header.number() - MAX_UNCLE_AGE), - max: Some(header.number() - 1), - found: uncle.number() - }).into()); + return Err(Error::BadBlock(BlockErrorWithData { + error: BlockError::UncleOutOfBounds(OutOfBounds { + min: Some(header.number() - MAX_UNCLE_AGE), + max: Some(header.number() - 1), + found: uncle.number() + }), + data: block.bytes, + })); }; // cB @@ -214,8 +261,16 @@ fn verify_uncles(block: &PreverifiedBlock, bc: &dyn BlockProvider, engine: &dyn // cB.p^7 -------------/ // cB.p^8 let mut expected_uncle_parent = *header.parent_hash(); - let uncle_parent = bc.block_header_data(&uncle.parent_hash()) - .ok_or_else(|| BlockError::UnknownUncleParent(*uncle.parent_hash()))?; + let uncle_parent = match bc.block_header_data(&uncle.parent_hash()) { + Some(uncle_parent) => uncle_parent, + None => { + return Err(Error::BadBlock(BlockErrorWithData { + error: BlockError::UnknownUncleParent(*uncle.parent_hash()), + data: block.bytes, + })); + } + }; + for _ in 0..depth { match bc.block_details(&expected_uncle_parent) { Some(details) => { @@ -225,118 +280,122 @@ fn verify_uncles(block: &PreverifiedBlock, bc: &dyn BlockProvider, engine: &dyn } } if expected_uncle_parent != uncle_parent.hash() { - return Err(From::from(BlockError::UncleParentNotInChain(uncle_parent.hash()))); + return Err(Error::BadBlock(BlockErrorWithData { + error: BlockError::UncleParentNotInChain(uncle_parent.hash()).into(), + data: block.bytes, + })); } let uncle_parent = uncle_parent.decode()?; - verify_parent(&uncle, &uncle_parent, engine)?; + if let Err(error) = verify_parent(&uncle, &uncle_parent, engine) { + return Err(Error::BadBlock(BlockErrorWithData { error, data: block.bytes })); + } engine.verify_block_family(&uncle, &uncle_parent)?; verified.insert(uncle.hash()); } } - Ok(()) + Ok(block) } /// Phase 4 verification. Check block information against transaction enactment results, -pub fn verify_block_final(expected: &Header, got: &Header) -> Result<(), Error> { +pub fn verify_block_final(expected: &Header, got: &Header) -> Result<(), BlockError> { if expected.state_root() != got.state_root() { - return Err(From::from(BlockError::InvalidStateRoot(Mismatch { + return Err(BlockError::InvalidStateRoot(Mismatch { expected: *expected.state_root(), found: *got.state_root() - }))) + })); } if expected.gas_used() != got.gas_used() { - return Err(From::from(BlockError::InvalidGasUsed(Mismatch { + return Err(BlockError::InvalidGasUsed(Mismatch { expected: *expected.gas_used(), - found: *got.gas_used() - }))) + found: *got.gas_used(), + })); } if expected.log_bloom() != got.log_bloom() { - return Err(From::from(BlockError::InvalidLogBloom(Box::new(Mismatch { + return Err(BlockError::InvalidLogBloom(From::from(Mismatch { expected: *expected.log_bloom(), found: *got.log_bloom() - })))) + }))); } if expected.receipts_root() != got.receipts_root() { - return Err(From::from(BlockError::InvalidReceiptsRoot(Mismatch { + return Err(BlockError::InvalidReceiptsRoot(Mismatch { expected: *expected.receipts_root(), found: *got.receipts_root() - }))) + })); } Ok(()) } /// Check basic header parameters. -pub(crate) fn verify_header_params(header: &Header, engine: &dyn Engine, check_seal: bool) -> Result<(), Error> { +pub(crate) fn verify_header_params(header: &Header, engine: &dyn Engine, check_seal: bool) -> Result<(), BlockError> { if check_seal { let expected_seal_fields = engine.seal_fields(header); if header.seal().len() != expected_seal_fields { - return Err(From::from(BlockError::InvalidSealArity( - Mismatch { expected: expected_seal_fields, found: header.seal().len() } - ))); + return Err(BlockError::InvalidSealArity(Mismatch { + expected: expected_seal_fields, + found: header.seal().len() + })); } } - if header.number() >= From::from(BlockNumber::max_value()) { - return Err(From::from(BlockError::RidiculousNumber(OutOfBounds { + if header.number() >= BlockNumber::max_value() { + return Err(BlockError::RidiculousNumber(OutOfBounds { max: Some(From::from(BlockNumber::max_value())), min: None, found: header.number() - }))) + })); } if header.gas_used() > header.gas_limit() { - return Err(From::from(BlockError::TooMuchGasUsed(OutOfBounds { + return Err(BlockError::TooMuchGasUsed(OutOfBounds { max: Some(*header.gas_limit()), min: None, found: *header.gas_used() - }))); + })); } if engine.gas_limit_override(header).is_none() { let min_gas_limit = engine.min_gas_limit(); if header.gas_limit() < &min_gas_limit { - return Err(From::from(BlockError::InvalidGasLimit(OutOfBounds { + return Err(BlockError::InvalidGasLimit(OutOfBounds { min: Some(min_gas_limit), max: None, found: *header.gas_limit() - }))); + })); } if let Some(limit) = engine.maximum_gas_limit() { if header.gas_limit() > &limit { - return Err(From::from(BlockError::InvalidGasLimit(OutOfBounds { + return Err(BlockError::InvalidGasLimit(OutOfBounds { min: None, max: Some(limit), found: *header.gas_limit() - }))); + })); } } } let maximum_extra_data_size = engine.maximum_extra_data_size(); if header.number() != 0 && header.extra_data().len() > maximum_extra_data_size { - return Err(From::from(BlockError::ExtraDataOutOfBounds(OutOfBounds { + return Err(BlockError::ExtraDataOutOfBounds(OutOfBounds { min: None, max: Some(maximum_extra_data_size), found: header.extra_data().len() - }))); + })); } if let Some(ref ext) = engine.machine().ethash_extensions() { if header.number() >= ext.dao_hardfork_transition && header.number() <= ext.dao_hardfork_transition + 9 && header.extra_data()[..] != b"dao-hard-fork"[..] { - return Err(From::from(BlockError::ExtraDataOutOfBounds(OutOfBounds { + return Err(BlockError::ExtraDataOutOfBounds(From::from(OutOfBounds { min: None, max: None, found: 0 }))); } } - Ok(()) } -/// A header verification step that should be done for new block headers, but not for uncles. -pub(crate) fn verify_header_time(header: &Header) -> Result<(), Error> { +pub(crate) fn verify_header_time(header: &Header) -> Result<(), BlockError> { const ACCEPTABLE_DRIFT: Duration = Duration::from_secs(15); // this will resist overflow until `year 2037` let max_time = SystemTime::now() + ACCEPTABLE_DRIFT; @@ -345,26 +404,26 @@ pub(crate) fn verify_header_time(header: &Header) -> Result<(), Error> { .ok_or(BlockError::TimestampOverflow)?; if timestamp > invalid_threshold { - return Err(From::from(BlockError::InvalidTimestamp(OutOfBounds { + return Err(BlockError::InvalidTimestamp(From::from(OutOfBounds { max: Some(max_time), min: None, found: timestamp - }.into()))) + }))); } if timestamp > max_time { - return Err(From::from(BlockError::TemporarilyInvalid(OutOfBounds { + return Err(BlockError::TemporarilyInvalid(From::from(OutOfBounds { max: Some(max_time), min: None, found: timestamp - }.into()))) + }))); } Ok(()) } /// Check header parameters against parent header. -fn verify_parent(header: &Header, parent: &Header, engine: &dyn Engine) -> Result<(), Error> { +fn verify_parent(header: &Header, parent: &Header, engine: &dyn Engine) -> Result<(), BlockError> { assert!(header.parent_hash().is_zero() || &parent.hash() == header.parent_hash(), "Parent hash should already have been verified; qed"); @@ -374,13 +433,17 @@ fn verify_parent(header: &Header, parent: &Header, engine: &dyn Engine) -> Resul .ok_or(BlockError::TimestampOverflow)?; let found = CheckedSystemTime::checked_add(now, Duration::from_secs(header.timestamp())) .ok_or(BlockError::TimestampOverflow)?; - return Err(From::from(BlockError::InvalidTimestamp(OutOfBounds { max: None, min: Some(min), found }.into()))) + return Err(BlockError::InvalidTimestamp(From::from(OutOfBounds { + max: None, + min: Some(min), + found + }))); } if header.number() != parent.number() + 1 { - return Err(From::from(BlockError::InvalidNumber(Mismatch { + return Err(BlockError::InvalidNumber(Mismatch { expected: parent.number() + 1, found: header.number() - }))); + })); } if header.number() == 0 { @@ -388,7 +451,7 @@ fn verify_parent(header: &Header, parent: &Header, engine: &dyn Engine) -> Resul min: Some(1), max: None, found: header.number() - }).into()); + })); } if engine.gas_limit_override(header).is_none() { let gas_limit_divisor = engine.params().gas_limit_bound_divisor; @@ -396,11 +459,11 @@ fn verify_parent(header: &Header, parent: &Header, engine: &dyn Engine) -> Resul let min_gas = parent_gas_limit - parent_gas_limit / gas_limit_divisor; let max_gas = parent_gas_limit + parent_gas_limit / gas_limit_divisor; if header.gas_limit() <= &min_gas || header.gas_limit() >= &max_gas { - return Err(From::from(BlockError::InvalidGasLimit(OutOfBounds { + return Err(BlockError::InvalidGasLimit(OutOfBounds { min: Some(min_gas), max: Some(max_gas), found: *header.gas_limit() - }))); + })); } } @@ -408,24 +471,30 @@ fn verify_parent(header: &Header, parent: &Header, engine: &dyn Engine) -> Resul } /// Verify block data against header: transactions root and uncles hash. -fn verify_block_integrity(block: &Unverified) -> Result<(), Error> { +fn verify_block_integrity(block: Unverified) -> Result { let block_rlp = Rlp::new(&block.bytes); let tx = block_rlp.at(1)?; let expected_root = ordered_trie_root(tx.iter().map(|r| r.as_raw())); if &expected_root != block.header.transactions_root() { - return Err(BlockError::InvalidTransactionsRoot(Mismatch { - expected: expected_root, - found: *block.header.transactions_root(), - }).into()); + return Err(Error::BadBlock(BlockErrorWithData { + error: BlockError::InvalidTransactionsRoot(Mismatch { + expected: expected_root, + found: *block.header.transactions_root(), + }), + data: block.bytes + })); } let expected_uncles = keccak(block_rlp.at(2)?.as_raw()); - if &expected_uncles != block.header.uncles_hash(){ - return Err(BlockError::InvalidUnclesHash(Mismatch { - expected: expected_uncles, - found: *block.header.uncles_hash(), - }).into()); - } - Ok(()) + if &expected_uncles != block.header.uncles_hash() { + return Err(Error::BadBlock(BlockErrorWithData { + error: BlockError::InvalidUnclesHash(Mismatch { + expected: expected_uncles, + found: *block.header.uncles_hash(), + }), + data: block.bytes, + })); + } + Ok(block) } #[cfg(test)] @@ -445,7 +514,7 @@ mod tests { }; use common_types::{ engines::params::CommonParams, - errors::BlockError::*, + errors::{BlockErrorWithData, EthcoreError as Error, BlockError::*}, transaction::{SignedTransaction, Transaction, UnverifiedTransaction, Action}, }; use triehash::ordered_trie_root; @@ -460,7 +529,7 @@ mod tests { fn check_fail(result: Result<(), Error>, e: BlockError) { match result { - Err(Error::Block(ref error)) if *error == e => (), + Err(Error::BadBlock(BlockErrorWithData { error, .. })) if error == e => (), Err(other) => panic!("Block verification failed.\nExpected: {:?}\nGot: {:?}", e, other), Ok(_) => panic!("Block verification failed.\nExpected: {:?}\nGot: Ok", e), } @@ -469,8 +538,8 @@ mod tests { fn check_fail_timestamp(result: Result<(), Error>, temp: bool) { let name = if temp { "TemporarilyInvalid" } else { "InvalidTimestamp" }; match result { - Err(Error::Block(BlockError::InvalidTimestamp(_))) if !temp => (), - Err(Error::Block(BlockError::TemporarilyInvalid(_))) if temp => (), + Err(Error::BadBlock(BlockErrorWithData { error: BlockError::InvalidTimestamp(_), .. })) if !temp => (), + Err(Error::BadBlock(BlockErrorWithData { error: BlockError::TemporarilyInvalid(_), .. })) if temp => (), Err(other) => panic!("Block verification failed.\nExpected: {}\nGot: {:?}", name, other), Ok(_) => panic!("Block verification failed.\nExpected: {}\nGot: Ok", name), } @@ -478,7 +547,7 @@ mod tests { fn basic_test(bytes: &[u8], engine: &dyn Engine) -> Result<(), Error> { let unverified = Unverified::from_rlp(bytes.to_vec())?; - verify_block_basic(&unverified, engine, true) + verify_block_basic(unverified, engine, true).map(|_| ()) } fn family_test(bytes: &[u8], engine: &dyn Engine, bc: &BC) -> Result<(), Error> where BC: BlockProvider { @@ -494,9 +563,13 @@ mod tests { // no existing tests need access to test, so having this not function // is fine. let client = TestBlockChainClient::default(); - let parent = bc.block_header_data(header.parent_hash()) - .ok_or(BlockError::UnknownParent(*header.parent_hash()))? - .decode()?; + let parent = match bc.block_header_data(header.parent_hash()) { + Some(parent) => parent.decode()?, + None => return Err(Error::BadBlock(BlockErrorWithData { + error: BlockError::UnknownParent(*header.parent_hash()), + data: block.bytes + })), + }; let block = PreverifiedBlock { header, @@ -505,12 +578,7 @@ mod tests { bytes: bytes.to_vec(), }; - let full_params = FullFamilyParams { - block: &block, - block_provider: bc as &dyn BlockProvider, - client: &client, - }; - verify_block_family(&block.header, &parent, engine, full_params) + verify_block_family(&parent, engine, block, bc, &client).map(|_| ()) } fn unordered_test(bytes: &[u8], engine: &dyn Engine) -> Result<(), Error> { @@ -740,9 +808,9 @@ mod tests { header.set_gas_limit(0.into()); header.set_difficulty("0000000000000000000000000000000000000000000000000000000000020000".parse::().unwrap()); match family_test(&create_test_block(&header), engine, &bc) { - Err(Error::Block(InvalidGasLimit(_))) => {}, - Err(_) => { panic!("should be invalid difficulty fail"); }, - _ => { panic!("Should be error, got Ok"); }, + Err(Error::BadBlock(BlockErrorWithData { error: InvalidGasLimit(_), .. })) => {}, + Err(_) => panic!("should be invalid difficulty fail"), + _ => panic!("Should be error, got Ok"), } // TODO: some additional uncle checks