From cd7018007e851cbf6d0bb232b8aede4de9a9d079 Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Tue, 25 Feb 2020 15:32:13 +0100 Subject: [PATCH] [ethcore cleanup]: various unrelated fixes from `#11493` (#11507) * [cleanup]: various unrelated fixes from `#11493` * [revert]: too verbose logging `seal` * fix nit: don't mix from() and into() --- ethcore/engines/authority-round/src/lib.rs | 17 ++-- ethcore/engines/basic-authority/src/lib.rs | 4 +- ethcore/engines/clique/src/block_state.rs | 36 ++++---- ethcore/engines/clique/src/lib.rs | 89 ++++++++----------- ethcore/engines/ethash/src/lib.rs | 36 ++++++-- .../validator-set/src/safe_contract.rs | 7 +- ethcore/src/block.rs | 16 ++-- ethcore/src/client/client.rs | 2 +- ethcore/src/miner/miner.rs | 55 ++++++------ ethcore/sync/src/block_sync.rs | 2 +- ethcore/types/src/engines/mod.rs | 27 +++--- ethcore/verification/src/verification.rs | 8 +- 12 files changed, 153 insertions(+), 146 deletions(-) diff --git a/ethcore/engines/authority-round/src/lib.rs b/ethcore/engines/authority-round/src/lib.rs index 553aaecd8..588b23987 100644 --- a/ethcore/engines/authority-round/src/lib.rs +++ b/ethcore/engines/authority-round/src/lib.rs @@ -874,8 +874,7 @@ impl AuthorityRound { }; durations.push(dur_info); for (time, dur) in our_params.step_durations.iter().skip(1) { - let (step, time) = next_step_time_duration(dur_info, *time) - .ok_or(BlockError::TimestampOverflow)?; + let (step, time) = next_step_time_duration(dur_info, *time).ok_or(BlockError::TimestampOverflow)?; dur_info.transition_step = step; dur_info.transition_timestamp = time; dur_info.step_duration = *dur; @@ -1595,9 +1594,11 @@ impl Engine for AuthorityRound { /// Check the number of seal fields. fn verify_block_basic(&self, header: &Header) -> Result<(), Error> { if header.number() >= self.validate_score_transition && *header.difficulty() >= U256::from(U128::max_value()) { - return Err(From::from(BlockError::DifficultyOutOfBounds( - OutOfBounds { min: None, max: Some(U256::from(U128::max_value())), found: *header.difficulty() } - ))); + return Err(Error::Block(BlockError::DifficultyOutOfBounds(OutOfBounds { + min: None, + max: Some(U256::from(U128::max_value())), + found: *header.difficulty() + }))); } match verify_timestamp(&self.step.inner, header_step(header, self.empty_steps_transition)?) { @@ -1618,7 +1619,7 @@ impl Engine for AuthorityRound { self.validators.report_benign(header.author(), set_number, header.number()); } - Err(BlockError::InvalidSeal.into()) + Err(Error::Block(BlockError::InvalidSeal)) } Err(e) => Err(e.into()), Ok(()) => Ok(()), @@ -1724,9 +1725,9 @@ impl Engine for AuthorityRound { if header.number() >= self.validate_score_transition { let expected_difficulty = calculate_score(parent_step.into(), step.into(), empty_steps_len.into()); if header.difficulty() != &expected_difficulty { - return Err(From::from(BlockError::InvalidDifficulty(Mismatch { + return Err(Error::Block(BlockError::InvalidDifficulty(Mismatch { expected: expected_difficulty, - found: header.difficulty().clone() + found: *header.difficulty() }))); } } diff --git a/ethcore/engines/basic-authority/src/lib.rs b/ethcore/engines/basic-authority/src/lib.rs index f4501f698..5c5efc88e 100644 --- a/ethcore/engines/basic-authority/src/lib.rs +++ b/ethcore/engines/basic-authority/src/lib.rs @@ -75,7 +75,7 @@ fn verify_external(header: &Header, validators: &dyn ValidatorSet) -> Result<(), } match validators.contains(header.parent_hash(), &signer) { - false => Err(BlockError::InvalidSeal.into()), + false => Err(Error::Block(BlockError::InvalidSeal)), true => Ok(()) } } @@ -91,7 +91,7 @@ impl BasicAuthority { /// Create a new instance of BasicAuthority engine pub fn new(our_params: BasicAuthorityParams, machine: Machine) -> Self { BasicAuthority { - machine: machine, + machine, signer: RwLock::new(None), validators: new_validator_set(our_params.validators), } diff --git a/ethcore/engines/clique/src/block_state.rs b/ethcore/engines/clique/src/block_state.rs index a6f810357..f082ee8d3 100644 --- a/ethcore/engines/clique/src/block_state.rs +++ b/ethcore/engines/clique/src/block_state.rs @@ -33,7 +33,7 @@ use unexpected::Mismatch; use crate::{ util::{extract_signers, recover_creator}, - {VoteType, DIFF_INTURN, DIFF_NOTURN, NULL_AUTHOR, SIGNING_DELAY_NOTURN_MS}, + VoteType, DIFF_INTURN, DIFF_NOTURN, NULL_AUTHOR, SIGNING_DELAY_NOTURN_MS, }; /// Type that keeps track of the state for a given vote @@ -137,30 +137,30 @@ impl CliqueBlockState { // The signer is not authorized if !self.signers.contains(&creator) { trace!(target: "engine", "current state: {}", self); - Err(EngineError::NotAuthorized(creator))? + return Err(EngineError::NotAuthorized(creator).into()); } // The signer has signed a block too recently if self.recent_signers.contains(&creator) { trace!(target: "engine", "current state: {}", self); - Err(EngineError::CliqueTooRecentlySigned(creator))? + return Err(EngineError::CliqueTooRecentlySigned(creator).into()); } // Wrong difficulty let inturn = self.is_inturn(header.number(), &creator); if inturn && *header.difficulty() != DIFF_INTURN { - Err(BlockError::InvalidDifficulty(Mismatch { + return Err(Error::Block(BlockError::InvalidDifficulty(Mismatch { expected: DIFF_INTURN, found: *header.difficulty(), - }))? + }))); } if !inturn && *header.difficulty() != DIFF_NOTURN { - Err(BlockError::InvalidDifficulty(Mismatch { + return Err(Error::Block(BlockError::InvalidDifficulty(Mismatch { expected: DIFF_NOTURN, found: *header.difficulty(), - }))? + }))); } Ok(creator) @@ -178,9 +178,10 @@ impl CliqueBlockState { if self.signers != signers { let invalid_signers: Vec = signers.into_iter() .filter(|s| !self.signers.contains(s)) - .map(|s| format!("{}", s)) + .map(|s| s.to_string()) .collect(); - Err(EngineError::CliqueFaultyRecoveredSigners(invalid_signers))? + + return Err(EngineError::CliqueFaultyRecoveredSigners(invalid_signers).into()); }; // TODO(niklasad1): I'm not sure if we should shrink here because it is likely that next epoch @@ -196,11 +197,14 @@ impl CliqueBlockState { if *header.author() != NULL_AUTHOR { let decoded_seal = header.decode_seal::>()?; if decoded_seal.len() != 2 { - Err(BlockError::InvalidSealArity(Mismatch { expected: 2, found: decoded_seal.len() }))? + return Err(Error::Block(BlockError::InvalidSealArity(Mismatch { + expected: 2, + found: decoded_seal.len() + }))); } let nonce = H64::from_slice(decoded_seal[1]); - self.update_signers_on_vote(VoteType::from_nonce(nonce)?, creator, *header.author(), header.number())?; + self.update_signers_on_vote(VoteType::from_nonce(nonce)?, creator, *header.author(), header.number()); } Ok(creator) @@ -212,7 +216,7 @@ impl CliqueBlockState { signer: Address, beneficiary: Address, block_number: u64 - ) -> Result<(), Error> { + ) { trace!(target: "engine", "Attempt vote {:?} {:?}", kind, beneficiary); @@ -239,7 +243,7 @@ impl CliqueBlockState { // If no vote was found for the beneficiary return `early` but don't propogate an error let (votes, vote_kind) = match self.get_current_votes_and_kind(beneficiary) { Some((v, k)) => (v, k), - None => return Ok(()), + None => return, }; let threshold = self.signers.len() / 2; @@ -263,8 +267,6 @@ impl CliqueBlockState { self.rotate_recent_signers(); self.remove_all_votes_from(beneficiary); } - - Ok(()) } /// Calculate the next timestamp for `inturn` and `noturn` fails if any of them can't be represented as @@ -272,7 +274,7 @@ impl CliqueBlockState { // TODO(niklasad1): refactor this method to be in constructor of `CliqueBlockState` instead. // This is a quite bad API because we must mutate both variables even when already `inturn` fails // That's why we can't return early and must have the `if-else` in the end - pub fn calc_next_timestamp(&mut self, timestamp: u64, period: u64) -> Result<(), Error> { + pub fn calc_next_timestamp(&mut self, timestamp: u64, period: u64) -> Result<(), BlockError> { let inturn = CheckedSystemTime::checked_add(UNIX_EPOCH, Duration::from_secs(timestamp.saturating_add(period))); self.next_timestamp_inturn = inturn; @@ -286,7 +288,7 @@ impl CliqueBlockState { if self.next_timestamp_inturn.is_some() && self.next_timestamp_noturn.is_some() { Ok(()) } else { - Err(BlockError::TimestampOverflow)? + Err(BlockError::TimestampOverflow) } } diff --git a/ethcore/engines/clique/src/lib.rs b/ethcore/engines/clique/src/lib.rs index 9080e09db..8843e07ba 100644 --- a/ethcore/engines/clique/src/lib.rs +++ b/ethcore/engines/clique/src/lib.rs @@ -104,7 +104,6 @@ use crate::{ params::CliqueParams, }; - mod params; mod block_state; mod util; @@ -157,7 +156,7 @@ impl VoteType { } else if nonce == NONCE_DROP_VOTE { Ok(VoteType::Remove) } else { - Err(EngineError::CliqueInvalidNonce(nonce))? + Err(EngineError::CliqueInvalidNonce(nonce).into()) } } @@ -250,17 +249,14 @@ impl Clique { } } - fn sign_header(&self, header: &Header) -> Result<(Signature, H256), Error> { - + fn sign_header(&self, header: &Header) -> Result<(Signature, H256), EngineError> { match self.signer.read().as_ref() { - None => { - Err(EngineError::RequiresSigner)? - } + None => Err(EngineError::RequiresSigner), Some(signer) => { let digest = header.hash(); match signer.sign(digest) { Ok(sig) => Ok((sig, digest)), - Err(e) => Err(EngineError::Custom(e.into()))?, + Err(e) => Err(EngineError::Custom(e.to_string())), } } } @@ -296,9 +292,7 @@ impl Clique { } // BlockState is not found in memory, which means we need to reconstruct state from last checkpoint. match self.client.read().as_ref().and_then(|w| w.upgrade()) { - None => { - return Err(EngineError::RequiresClient)?; - } + None => return Err(EngineError::RequiresClient.into()), Some(c) => { let last_checkpoint_number = header.number() - header.number() % self.epoch_length as u64; debug_assert_ne!(last_checkpoint_number, header.number()); @@ -327,7 +321,7 @@ impl Clique { } match c.block_header(BlockId::Hash(last_parent_hash)) { None => { - return Err(BlockError::UnknownParent(last_parent_hash))?; + return Err(Error::Block(BlockError::UnknownParent(last_parent_hash))); } Some(next) => { chain.push_front(next.decode()?); @@ -341,7 +335,7 @@ impl Clique { .parent_hash(); let last_checkpoint_header = match c.block_header(BlockId::Hash(last_checkpoint_hash)) { - None => return Err(EngineError::CliqueMissingCheckpoint(last_checkpoint_hash))?, + None => return Err(EngineError::CliqueMissingCheckpoint(last_checkpoint_hash).into()), Some(header) => header.decode()?, }; @@ -353,15 +347,14 @@ impl Clique { block_state_by_hash.insert(last_checkpoint_header.hash(), last_checkpoint_state.clone()); // Backfill! - let mut new_state = last_checkpoint_state.clone(); + let mut new_state = last_checkpoint_state; for item in &chain { new_state.apply(item, false)?; } new_state.calc_next_timestamp(header.timestamp(), self.period)?; block_state_by_hash.insert(header.hash(), new_state.clone()); - let elapsed = backfill_start.elapsed(); - trace!(target: "engine", "Back-filling succeed, took {} ms.", elapsed.as_millis()); + trace!(target: "engine", "Back-filling succeed, took {} ms.", backfill_start.elapsed().as_millis()); Ok(new_state) } } @@ -409,9 +402,8 @@ impl Engine for Clique { trace!(target: "engine", "on_seal_block"); let header = &mut block.header; - - let state = self.state_no_backfill(header.parent_hash()) - .ok_or_else(|| BlockError::UnknownParent(*header.parent_hash()))?; + let mut state = self.state_no_backfill(header.parent_hash()) + .ok_or_else(|| Error::Block(BlockError::UnknownParent(*header.parent_hash())))?; let is_checkpoint = header.number() % self.epoch_length == 0; @@ -438,16 +430,15 @@ impl Engine for Clique { } // Work on clique seal. - let mut seal: Vec = Vec::with_capacity(VANITY_LENGTH + SIGNATURE_LENGTH); // At this point, extra_data should only contain miner vanity. if header.extra_data().len() != VANITY_LENGTH { - Err(BlockError::ExtraDataOutOfBounds(OutOfBounds { + return Err(Error::Block(BlockError::ExtraDataOutOfBounds(OutOfBounds { min: Some(VANITY_LENGTH), max: Some(VANITY_LENGTH), found: header.extra_data().len() - }))?; + }))); } // vanity { @@ -467,15 +458,14 @@ impl Engine for Clique { // append signature onto extra_data let (sig, _msg) = self.sign_header(&header)?; seal.extend_from_slice(&sig[..]); - header.set_extra_data(seal.clone()); + header.set_extra_data(seal); header.compute_hash(); // locally sealed block don't go through valid_block_family(), so we have to record state here. - let mut new_state = state.clone(); - new_state.apply(&header, is_checkpoint)?; - new_state.calc_next_timestamp(header.timestamp(), self.period)?; - self.block_state_by_hash.write().insert(header.hash(), new_state); + state.apply(&header, is_checkpoint)?; + state.calc_next_timestamp(header.timestamp(), self.period)?; + self.block_state_by_hash.write().insert(header.hash(), state); trace!(target: "engine", "on_seal_block: finished, final header: {:?}", header); @@ -574,18 +564,18 @@ impl Engine for Clique { // This should succeed under the constraints that the system clock works let limit_as_dur = limit.duration_since(UNIX_EPOCH).map_err(|e| { - Box::new(format!("Converting SystemTime to Duration failed: {}", e)) + format!("Converting SystemTime to Duration failed: {}", e) })?; let hdr = Duration::from_secs(header.timestamp()); if hdr > limit_as_dur { let found = CheckedSystemTime::checked_add(UNIX_EPOCH, hdr).ok_or(BlockError::TimestampOverflow)?; - Err(BlockError::TemporarilyInvalid(OutOfBounds { + return Err(Error::Block(BlockError::TemporarilyInvalid(From::from(OutOfBounds { min: None, max: Some(limit), found, - }.into()))? + })))); } } @@ -600,10 +590,10 @@ impl Engine for Clique { let seal_fields = header.decode_seal::>()?; if seal_fields.len() != 2 { - Err(BlockError::InvalidSealArity(Mismatch { + return Err(Error::Block(BlockError::InvalidSealArity(Mismatch { expected: 2, found: seal_fields.len(), - }))? + }))); } let mixhash = H256::from_slice(seal_fields[0]); @@ -611,58 +601,58 @@ impl Engine for Clique { // Nonce must be 0x00..0 or 0xff..f if nonce != NONCE_DROP_VOTE && nonce != NONCE_AUTH_VOTE { - Err(EngineError::CliqueInvalidNonce(nonce))?; + return Err(EngineError::CliqueInvalidNonce(nonce).into()); } if is_checkpoint && nonce != NULL_NONCE { - Err(EngineError::CliqueInvalidNonce(nonce))?; + return Err(EngineError::CliqueInvalidNonce(nonce).into()); } // Ensure that the mix digest is zero as Clique don't have fork protection currently if mixhash != NULL_MIXHASH { - Err(BlockError::MismatchedH256SealElement(Mismatch { + return Err(Error::Block(BlockError::MismatchedH256SealElement(Mismatch { expected: NULL_MIXHASH, found: mixhash, - }))? + }))); } let extra_data_len = header.extra_data().len(); if extra_data_len < VANITY_LENGTH { - Err(EngineError::CliqueMissingVanity)? + return Err(EngineError::CliqueMissingVanity.into()); } if extra_data_len < VANITY_LENGTH + SIGNATURE_LENGTH { - Err(EngineError::CliqueMissingSignature)? + return Err(EngineError::CliqueMissingSignature.into()); } let signers = extra_data_len - (VANITY_LENGTH + SIGNATURE_LENGTH); // Checkpoint blocks must at least contain one signer if is_checkpoint && signers == 0 { - Err(EngineError::CliqueCheckpointNoSigner)? + return Err(EngineError::CliqueCheckpointNoSigner.into()); } // Addresses must be be divisable by 20 if is_checkpoint && signers % ADDRESS_LENGTH != 0 { - Err(EngineError::CliqueCheckpointInvalidSigners(signers))? + return Err(EngineError::CliqueCheckpointInvalidSigners(signers).into()); } // Ensure that the block doesn't contain any uncles which are meaningless in PoA if *header.uncles_hash() != NULL_UNCLES_HASH { - Err(BlockError::InvalidUnclesHash(Mismatch { + return Err(Error::Block(BlockError::InvalidUnclesHash(Mismatch { expected: NULL_UNCLES_HASH, found: *header.uncles_hash(), - }))? + }))); } // Ensure that the block's difficulty is meaningful (may not be correct at this point) if *header.difficulty() != DIFF_INTURN && *header.difficulty() != DIFF_NOTURN { - Err(BlockError::DifficultyOutOfBounds(OutOfBounds { + return Err(Error::Block(BlockError::DifficultyOutOfBounds(OutOfBounds { min: Some(DIFF_NOTURN), max: Some(DIFF_INTURN), found: *header.difficulty(), - }))? + }))); } // All basic checks passed, continue to next phase @@ -684,7 +674,7 @@ impl Engine for Clique { // parent sanity check if parent.hash() != *header.parent_hash() || header.number() != parent.number() + 1 { - Err(BlockError::UnknownParent(parent.hash()))? + return Err(Error::Block(BlockError::UnknownParent(parent.hash()))); } // Ensure that the block's timestamp isn't too close to it's parent @@ -694,17 +684,16 @@ impl Engine for Clique { let found = CheckedSystemTime::checked_add(UNIX_EPOCH, Duration::from_secs(limit)) .ok_or(BlockError::TimestampOverflow)?; - Err(BlockError::InvalidTimestamp(OutOfBounds { + return Err(Error::Block(BlockError::InvalidTimestamp(From::from(OutOfBounds { min: None, max, found, - }.into()))? + })))); } // Retrieve the parent state - let parent_state = self.state(&parent)?; // Try to apply current state, apply() will further check signer and recent signer. - let mut new_state = parent_state.clone(); + let mut new_state = self.state(&parent)?; new_state.apply(header, header.number() % self.epoch_length == 0)?; new_state.calc_next_timestamp(header.timestamp(), self.period)?; self.block_state_by_hash.write().insert(header.hash(), new_state); @@ -714,7 +703,7 @@ impl Engine for Clique { fn genesis_epoch_data(&self, header: &Header, _call: &Call) -> Result, String> { let mut state = self.new_checkpoint_state(header).expect("Unable to parse genesis data."); - state.calc_next_timestamp(header.timestamp(), self.period).map_err(|e| format!("{}", e))?; + state.calc_next_timestamp(header.timestamp(), self.period).map_err(|e| e.to_string())?; self.block_state_by_hash.write().insert(header.hash(), state); // no proof. diff --git a/ethcore/engines/ethash/src/lib.rs b/ethcore/engines/ethash/src/lib.rs index afe572ec9..1f9223df7 100644 --- a/ethcore/engines/ethash/src/lib.rs +++ b/ethcore/engines/ethash/src/lib.rs @@ -179,7 +179,7 @@ fn verify_block_unordered(pow: &Arc, header: &Header) -> Result<( let seal = EthashSeal::parse_seal(header.seal())?; let result = pow.compute_light( - header.number() as u64, + header.number(), &header.bare_hash().0, seal.nonce.to_low_u64_be() ); @@ -193,10 +193,17 @@ fn verify_block_unordered(pow: &Arc, header: &Header) -> Result<( mix = H256(result.mix_hash), res = H256(result.value)); if mix != seal.mix_hash { - return Err(From::from(BlockError::MismatchedH256SealElement(Mismatch { expected: mix, found: seal.mix_hash }))); + return Err(From::from(BlockError::MismatchedH256SealElement(Mismatch { + expected: mix, + found: seal.mix_hash + }))); } if &difficulty < header.difficulty() { - return Err(From::from(BlockError::InvalidProofOfWork(OutOfBounds { min: Some(header.difficulty().clone()), max: None, found: difficulty }))); + return Err(From::from(BlockError::InvalidProofOfWork(OutOfBounds { + min: Some(*header.difficulty()), + max: None, + found: difficulty + }))); } Ok(()) } @@ -324,7 +331,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 { min: Some(min_difficulty), max: None, found: header.difficulty().clone() }))) + return Err(From::from(BlockError::DifficultyOutOfBounds(OutOfBounds { + min: Some(min_difficulty), + max: None, + found: *header.difficulty(), + }))) } let difficulty = ethash::boundary_to_difficulty(&H256(quick_get_difficulty( @@ -335,7 +346,11 @@ impl Engine for Ethash { ))); if &difficulty < header.difficulty() { - return Err(From::from(BlockError::InvalidProofOfWork(OutOfBounds { min: Some(header.difficulty().clone()), max: None, found: difficulty }))); + return Err(From::from(BlockError::InvalidProofOfWork(OutOfBounds { + min: Some(*header.difficulty()), + max: None, + found: difficulty + }))); } Ok(()) @@ -348,13 +363,20 @@ 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 { min: Some(1), max: None, found: header.number() }))); + return Err(From::from(BlockError::RidiculousNumber(OutOfBounds { + min: Some(1), + max: None, + found: header.number() + }))); } // Check difficulty is correct given the two timestamps. let expected_difficulty = self.calculate_difficulty(header, parent); if header.difficulty() != &expected_difficulty { - return Err(From::from(BlockError::InvalidDifficulty(Mismatch { expected: expected_difficulty, found: header.difficulty().clone() }))) + return Err(From::from(BlockError::InvalidDifficulty(Mismatch { + expected: expected_difficulty, + found: *header.difficulty() + }))) } Ok(()) diff --git a/ethcore/engines/validator-set/src/safe_contract.rs b/ethcore/engines/validator-set/src/safe_contract.rs index 06bdd0035..c0f056204 100644 --- a/ethcore/engines/validator-set/src/safe_contract.rs +++ b/ethcore/engines/validator-set/src/safe_contract.rs @@ -503,9 +503,10 @@ impl ValidatorSet for ValidatorSafeContract { receipts.iter().map(::rlp::encode) ); if found_root != *old_header.receipts_root() { - return Err(BlockError::InvalidReceiptsRoot( - Mismatch { expected: *old_header.receipts_root(), found: found_root } - ).into()); + return Err(EthcoreError::Block(BlockError::InvalidReceiptsRoot(Mismatch { + expected: *old_header.receipts_root(), + found: found_root + }))); } let bloom = self.expected_bloom(&old_header); diff --git a/ethcore/src/block.rs b/ethcore/src/block.rs index 7dc8c4276..992437a14 100644 --- a/ethcore/src/block.rs +++ b/ethcore/src/block.rs @@ -102,7 +102,7 @@ pub trait Drain { impl<'x> OpenBlock<'x> { /// Create a new `OpenBlock` ready for transaction pushing. - pub fn new<'a>( + pub fn new( engine: &'x dyn Engine, factories: Factories, tracing: bool, @@ -204,12 +204,11 @@ impl<'x> OpenBlock<'x> { let hash = t.hash(); let start = time::Instant::now(); self.push_transaction(t)?; - let took = start.elapsed(); - let took_ms = took.as_secs() * 1000 + took.subsec_nanos() as u64 / 1000000; - if took > time::Duration::from_millis(slow_tx) { - warn!("Heavy ({} ms) transaction in block {:?}: {:?}", took_ms, self.block.header.number(), hash); + let elapsed_millis = start.elapsed().as_millis(); + if elapsed_millis > slow_tx { + warn!("Heavy ({} ms) transaction in block {:?}: {:?}", elapsed_millis, self.block.header.number(), hash); } - debug!(target: "tx", "Transaction {:?} took: {} ms", hash, took_ms); + debug!(target: "tx", "Transaction {:?} took: {} ms", hash, elapsed_millis); } Ok(()) @@ -347,10 +346,10 @@ impl LockedBlock { let expected_seal_fields = engine.seal_fields(&self.header); let mut s = self; if seal.len() != expected_seal_fields { - Err(BlockError::InvalidSealArity(Mismatch { + return Err(Error::Block(BlockError::InvalidSealArity(Mismatch { expected: expected_seal_fields, found: seal.len() - }))?; + }))); } s.block.header.set_seal(seal); @@ -504,7 +503,6 @@ mod tests { verification::Unverified, }; use hash_db::EMPTY_PREFIX; - use spec; /// Enact the block given by `block_bytes` using `engine` on the database `db` with given `parent` block header fn enact_bytes( diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 4033ce347..55e0b1889 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -2388,7 +2388,7 @@ impl ImportSealedBlock for Client { // 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( - block.rlp_bytes(), + raw, format!("Detected an issue with locally sealed block: {}", e), ); return Err(e); diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index afb2baeee..ff209e05b 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -716,36 +716,34 @@ impl Miner { }, }; - let sealing_result = - match self.engine.generate_seal(&block, &parent_header) { - // Directly import a regular sealed block. - Seal::Regular(seal) => { - trace!(target: "miner", "Block #{}: Received a Regular seal.", block_number); - { - let mut sealing = self.sealing.lock(); - sealing.next_mandatory_reseal = Instant::now() + self.options.reseal_max_period; - } + match self.engine.generate_seal(&block, &parent_header) { + // Directly import a regular sealed block. + Seal::Regular(seal) => { + trace!(target: "miner", "Block #{}: Received a Regular seal.", block_number); + { + let mut sealing = self.sealing.lock(); + sealing.next_mandatory_reseal = Instant::now() + self.options.reseal_max_period; + } - block - .lock() - .seal(&*self.engine, seal) - .map(|sealed| { - match chain.import_sealed_block(sealed) { - Ok(_) => true, - Err(e) => { - error!(target: "miner", "Block #{}: seal_and_import_block_internally: import_sealed_block returned {:?}", block_number, e); - false - } + block + .lock() + .seal(&*self.engine, seal) + .map(|sealed| { + match chain.import_sealed_block(sealed) { + Ok(_) => true, + Err(e) => { + error!(target: "miner", "Block #{}: seal_and_import_block_internally: import_sealed_block returned {:?}", block_number, e); + false } - }) - .unwrap_or_else(|e| { - warn!("ERROR: Block #{}, importing sealed block failed when given internally generated seal: {}", block_number, e); - false - }) - }, - Seal::None => false, - }; - sealing_result + } + }) + .unwrap_or_else(|e| { + warn!("ERROR: Block #{}, importing sealed block failed when given internally generated seal: {}", block_number, e); + false + }) + }, + Seal::None => false, + } } /// Prepares work which has to be done to seal. @@ -1276,7 +1274,6 @@ impl miner::MinerService for Miner { if self.seal_and_import_block_internally(chain, block) { trace!(target: "miner", "update_sealing: imported internally sealed block"); } - return }, SealingState::NotReady => unreachable!("We returned right after sealing_state was computed. qed."), SealingState::External => { diff --git a/ethcore/sync/src/block_sync.rs b/ethcore/sync/src/block_sync.rs index 783f0bae7..47d62ad3c 100644 --- a/ethcore/sync/src/block_sync.rs +++ b/ethcore/sync/src/block_sync.rs @@ -577,7 +577,7 @@ impl BlockDownloader { }, Ok(_) => { trace_sync!(self, "Block queued {:?}", h); - imported.insert(h.clone()); + imported.insert(h); self.block_imported(&h, number, &parent); }, Err(EthcoreError::Block(BlockError::UnknownParent(_))) if allow_out_of_order => { diff --git a/ethcore/types/src/engines/mod.rs b/ethcore/types/src/engines/mod.rs index 0aca91c77..c1ce98078 100644 --- a/ethcore/types/src/engines/mod.rs +++ b/ethcore/types/src/engines/mod.rs @@ -18,11 +18,13 @@ use ethereum_types::{Address, H256, H64}; use bytes::Bytes; -use ethjson; use rlp::Rlp; use unexpected::Mismatch; -use crate::{BlockNumber, errors::{BlockError, EthcoreError}}; +use crate::{ + BlockNumber, + errors::{BlockError, EthcoreError} +}; pub mod epoch; pub mod params; @@ -56,21 +58,18 @@ impl EthashSeal { /// Tries to parse rlp encoded bytes as an Ethash/Clique seal. pub fn parse_seal>(seal: &[T]) -> Result { if seal.len() != 2 { - return Err(BlockError::InvalidSealArity( - Mismatch { - expected: 2, - found: seal.len() - } - ).into()); + Err(EthcoreError::Block(BlockError::InvalidSealArity(Mismatch { + expected: 2, + found: seal.len() + }))) + } else { + let mix_hash = Rlp::new(seal[0].as_ref()).as_val::()?; + let nonce = Rlp::new(seal[1].as_ref()).as_val::()?; + Ok(EthashSeal { mix_hash, nonce }) } - - let mix_hash = Rlp::new(seal[0].as_ref()).as_val::()?; - let nonce = Rlp::new(seal[1].as_ref()).as_val::()?; - Ok(EthashSeal { mix_hash, nonce }) } } - /// Seal type. #[derive(Debug, PartialEq, Eq)] pub enum Seal { @@ -96,7 +95,7 @@ pub const MAX_UNCLE_AGE: u64 = 6; /// Default EIP-210 contract code. /// As defined in https://github.com/ethereum/EIPs/pull/210 -pub const DEFAULT_BLOCKHASH_CONTRACT: &'static [u8] = &[ +pub const DEFAULT_BLOCKHASH_CONTRACT: &[u8] = &[ 0x73, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfe, 0x33, 0x14, 0x15, 0x61, 0x00, 0x6a, 0x57, 0x60, 0x01, 0x43, 0x03, 0x60, 0x00, 0x35, 0x61, 0x01, 0x00, 0x82, 0x07, 0x55, 0x61, 0x01, 0x00, 0x81, 0x07, 0x15, 0x15, diff --git a/ethcore/verification/src/verification.rs b/ethcore/verification/src/verification.rs index 2eebdf05e..b67cc127b 100644 --- a/ethcore/verification/src/verification.rs +++ b/ethcore/verification/src/verification.rs @@ -163,8 +163,8 @@ fn verify_uncles(block: &PreverifiedBlock, bc: &dyn BlockProvider, engine: &dyn let mut excluded = HashSet::new(); excluded.insert(header.hash()); - let mut hash = header.parent_hash().clone(); - excluded.insert(hash.clone()); + let mut hash = *header.parent_hash(); + excluded.insert(hash); for _ in 0..MAX_UNCLE_AGE { match bc.block_details(&hash) { Some(details) => { @@ -213,7 +213,7 @@ fn verify_uncles(block: &PreverifiedBlock, bc: &dyn BlockProvider, engine: &dyn // cB.p^6 -----------/ 6 // cB.p^7 -------------/ // cB.p^8 - let mut expected_uncle_parent = header.parent_hash().clone(); + 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()))?; for _ in 0..depth { @@ -440,7 +440,6 @@ mod tests { use keccak_hash::keccak; use engine::Engine; use parity_crypto::publickey::{Random, Generator}; - use spec; use ethcore::test_helpers::{ create_test_block_with_data, create_test_block, TestBlockChainClient }; @@ -449,7 +448,6 @@ mod tests { errors::BlockError::*, transaction::{SignedTransaction, Transaction, UnverifiedTransaction, Action}, }; - use rlp; use triehash::ordered_trie_root; use machine::Machine; use null_engine::NullEngine;