From 8fa56add471f77bb165d66c836363fc0d70b2ec4 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 7 Feb 2019 14:34:07 +0100 Subject: [PATCH] Properly handle check_epoch_end_signal errors (#10015) * Make check_epoch_end_signal to only use immutable data * Move check_epoch_end_signals out of commit_block * Make check_epoch_end_signals possible to fail * Actually return the error from check_epoch_end_signals * Remove a clone * Fix import error --- ethcore/src/client/client.rs | 67 +++++++++++++++++++++--------------- ethcore/src/engines/mod.rs | 3 ++ ethcore/src/state/mod.rs | 7 ++++ 3 files changed, 49 insertions(+), 28 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 577829d45..b1f2f0d79 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -61,7 +61,8 @@ use client::{ IoClient, BadBlocks, }; use client::bad_blocks; -use engines::{EthEngine, EpochTransition, ForkChoice}; +use engines::{EthEngine, EpochTransition, ForkChoice, EngineError}; +use engines::epoch::PendingTransition; use error::{ ImportErrorKind, ExecutionError, CallError, BlockError, QueueError, QueueErrorKind, Error as EthcoreError, EthcoreResult, ErrorKind as EthcoreErrorKind @@ -295,8 +296,8 @@ impl Importer { continue; } - match self.check_and_lock_block(block, client) { - Ok(closed_block) => { + match self.check_and_lock_block(&bytes, block, client) { + Ok((closed_block, pending)) => { if self.engine.is_proposal(&header) { self.block_queue.mark_as_good(&[hash]); proposed_blocks.push(bytes); @@ -305,7 +306,7 @@ impl Importer { let transactions_len = closed_block.transactions().len(); - let route = self.commit_block(closed_block, &header, encoded::Block::new(bytes), client); + let route = self.commit_block(closed_block, &header, encoded::Block::new(bytes), pending, client); import_results.push(route); client.report.write().accrue_block(&header, transactions_len); @@ -357,7 +358,7 @@ impl Importer { imported } - fn check_and_lock_block(&self, block: PreverifiedBlock, client: &Client) -> EthcoreResult { + fn check_and_lock_block(&self, bytes: &[u8], block: PreverifiedBlock, client: &Client) -> EthcoreResult<(LockedBlock, Option)> { let engine = &*self.engine; let header = block.header.clone(); @@ -441,7 +442,15 @@ impl Importer { bail!(e); } - Ok(locked_block) + let pending = self.check_epoch_end_signal( + &header, + bytes, + locked_block.receipts(), + locked_block.state().db(), + client + )?; + + Ok((locked_block, pending)) } /// Import a block with transaction receipts. @@ -474,7 +483,7 @@ impl Importer { // // The header passed is from the original block data and is sealed. // TODO: should return an error if ImportRoute is none, issue #9910 - fn commit_block(&self, block: B, header: &Header, block_data: encoded::Block, client: &Client) -> ImportRoute where B: Drain { + fn commit_block(&self, block: B, header: &Header, block_data: encoded::Block, pending: Option, client: &Client) -> ImportRoute where B: Drain { let hash = &header.hash(); let number = header.number(); let parent = header.parent_hash(); @@ -529,15 +538,9 @@ impl Importer { // check epoch end signal, potentially generating a proof on the current // state. - self.check_epoch_end_signal( - &header, - block_data.raw(), - &receipts, - &state, - &chain, - &mut batch, - client - ); + if let Some(pending) = pending { + chain.insert_pending_transition(&mut batch, header.hash(), pending); + } state.journal_under(&mut batch, number, hash).expect("DB commit failed"); @@ -592,10 +595,8 @@ impl Importer { block_bytes: &[u8], receipts: &[Receipt], state_db: &StateDB, - chain: &BlockChain, - batch: &mut DBTransaction, client: &Client, - ) { + ) -> EthcoreResult> { use engines::EpochChange; let hash = header.hash(); @@ -606,7 +607,6 @@ impl Importer { match self.engine.signals_epoch_end(header, auxiliary) { EpochChange::Yes(proof) => { - use engines::epoch::PendingTransition; use engines::Proof; let proof = match proof { @@ -643,11 +643,9 @@ impl Importer { .transact(&transaction, options); let res = match res { - Err(ExecutionError::Internal(e)) => - Err(format!("Internal error: {}", e)), Err(e) => { trace!(target: "client", "Proved call failed: {}", e); - Ok((Vec::new(), state.drop().1.extract_proof())) + Err(e.to_string()) } Ok(res) => Ok((res.output, state.drop().1.extract_proof())), }; @@ -660,7 +658,7 @@ impl Importer { Err(e) => { warn!(target: "client", "Failed to generate transition proof for block {}: {}", hash, e); warn!(target: "client", "Snapshots produced by this client may be incomplete"); - Vec::new() + return Err(EngineError::FailedSystemCall(e).into()) } } } @@ -668,13 +666,13 @@ impl Importer { debug!(target: "client", "Block {} signals epoch end.", hash); - let pending = PendingTransition { proof: proof }; - chain.insert_pending_transition(batch, hash, pending); + Ok(Some(PendingTransition { proof: proof })) }, - EpochChange::No => {}, + EpochChange::No => Ok(None), EpochChange::Unsure(_) => { warn!(target: "client", "Detected invalid engine implementation."); warn!(target: "client", "Engine claims to require more block data, but everything provided."); + Err(EngineError::InvalidEngine.into()) } } } @@ -2380,7 +2378,20 @@ impl ImportSealedBlock for Client { let block_data = block.rlp_bytes(); - let route = self.importer.commit_block(block, &header, encoded::Block::new(block_data), self); + let pending = self.importer.check_epoch_end_signal( + &header, + &block_data, + block.receipts(), + block.state().db(), + self + )?; + let route = self.importer.commit_block( + block, + &header, + encoded::Block::new(block_data), + pending, + self + ); trace!(target: "client", "Imported sealed block #{} ({})", header.number(), hash); self.state_db.write().sync_cache(&route.enacted, &route.retracted, false); route diff --git a/ethcore/src/engines/mod.rs b/ethcore/src/engines/mod.rs index 64d05e5c4..deb85cd35 100644 --- a/ethcore/src/engines/mod.rs +++ b/ethcore/src/engines/mod.rs @@ -81,6 +81,8 @@ pub enum EngineError { MalformedMessage(String), /// Requires client ref, but none registered. RequiresClient, + /// Invalid engine specification or implementation. + InvalidEngine, } impl fmt::Display for EngineError { @@ -96,6 +98,7 @@ impl fmt::Display for EngineError { FailedSystemCall(ref msg) => format!("Failed to make system call: {}", msg), MalformedMessage(ref msg) => format!("Received malformed consensus message: {}", msg), RequiresClient => format!("Call requires client but none registered"), + InvalidEngine => format!("Invalid engine specification or implementation"), }; f.write_fmt(format_args!("Engine error ({})", msg)) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 2939c9729..4d24a1b15 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -1284,6 +1284,13 @@ impl fmt::Debug for State { } } +impl State { + /// Get a reference to the underlying state DB. + pub fn db(&self) -> &StateDB { + &self.db + } +} + // TODO: cloning for `State` shouldn't be possible in general; Remove this and use // checkpoints where possible. impl Clone for State {