From df6fd20457b9c0aa44404bcfe4dd22f8bc3ebe12 Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Tue, 12 Mar 2019 09:37:46 +0100 Subject: [PATCH 1/3] fix(clique): err types --- ethcore/src/engines/clique/block_state.rs | 58 +++--- ethcore/src/engines/clique/mod.rs | 232 +++++++++++++--------- ethcore/src/engines/clique/tests.rs | 25 ++- ethcore/src/engines/mod.rs | 41 +++- 4 files changed, 230 insertions(+), 126 deletions(-) diff --git a/ethcore/src/engines/clique/block_state.rs b/ethcore/src/engines/clique/block_state.rs index f75fb543c4a..96d2e3e4c04 100644 --- a/ethcore/src/engines/clique/block_state.rs +++ b/ethcore/src/engines/clique/block_state.rs @@ -18,13 +18,15 @@ use std::collections::{HashMap, VecDeque}; use std::fmt; use std::time::{Duration, SystemTime, UNIX_EPOCH}; +use engines::EngineError; use engines::clique::util::{extract_signers, recover_creator}; use engines::clique::{VoteType, DIFF_INTURN, DIFF_NOTURN, NULL_AUTHOR, SIGNING_DELAY_NOTURN_MS}; -use error::Error; -use ethereum_types::Address; +use error::{Error, BlockError}; +use ethereum_types::{Address, H64}; use rand::Rng; use types::BlockNumber; use types::header::Header; +use unexpected::Mismatch; /// Type that keeps track of the state for a given vote // Votes that go against the proposal aren't counted since it's equivalent to not voting @@ -124,32 +126,34 @@ impl CliqueBlockState { fn verify(&self, header: &Header) -> Result { let creator = recover_creator(header)?.clone(); - // Check signer list + // The signer is not authorized if !self.signers.contains(&creator) { trace!(target: "engine", "current state: {}", self); - return Err(From::from(format!("Error applying #{}({}): {} is not in the signer list!", - header.number(), - header.hash(), - creator))); + Err(EngineError::NotAuthorized(creator))? } - // Check recent signer. + // The signer has signed a block too recently if self.recent_signers.contains(&creator) { trace!(target: "engine", "current state: {}", self); - return Err(From::from(format!("Error applying #{}({}): {} is in the recent_signer list!", - header.number(), - header.hash(), - creator))); + Err(EngineError::CliqueTooRecentlySigned(creator))? } - // Ensure that the difficulty corresponds to the turn-ness of the signer + + // Wrong difficulty let inturn = self.is_inturn(header.number(), &creator); - if (inturn && *header.difficulty() != DIFF_INTURN) || - (!inturn && *header.difficulty() != DIFF_NOTURN) { - return Err(From::from(format!("Error applying #{}({}): wrong difficulty!", - header.number(), - header.hash()))); + if inturn && *header.difficulty() != DIFF_INTURN { + Err(BlockError::InvalidDifficulty(Mismatch { + expected: DIFF_INTURN, + found: *header.difficulty(), + }))? + } + + if !inturn && *header.difficulty() != DIFF_NOTURN { + Err(BlockError::InvalidDifficulty(Mismatch { + expected: DIFF_NOTURN, + found: *header.difficulty(), + }))? } Ok(creator) @@ -165,7 +169,11 @@ impl CliqueBlockState { // checkpoint block should not affect previous tallying, so we check that. let signers = extract_signers(header)?; if self.signers != signers { - return Err(From::from("checkpoint block signers is different than expected")); + let invalid_signers: Vec = signers.into_iter() + .filter(|s| !self.signers.contains(s)) + .map(|s| format!("{}", s)) + .collect(); + Err(EngineError::CliqueFaultyRecoveredSigners(invalid_signers))? }; // TODO(niklasad1): I'm not sure if we should shrink here because it is likely that next epoch @@ -179,7 +187,11 @@ impl CliqueBlockState { // Contains vote if *header.author() != NULL_AUTHOR { - let nonce = *header.decode_seal::>()?.get(1).ok_or("Error decoding seal")?; + let nonce: H64 = header.decode_seal::>()?.get(1) + .cloned() + .map(Into::into) + .ok_or(BlockError::InvalidSeal)?; + self.update_signers_on_vote(VoteType::from_nonce(nonce)?, creator, *header.author(), header.number())?; } @@ -234,7 +246,7 @@ impl CliqueBlockState { } VoteType::Remove => { let pos = self.signers.binary_search(&beneficiary) - .map_err(|_| "Unable to find beneficiary in signer list when removing".to_string())?; + .expect("signer is in signer_list ensured by `is_valid_vote`; qed"); debug!(target: "engine", "removed signer: {}", beneficiary); self.signers.remove(pos); } @@ -268,13 +280,13 @@ impl CliqueBlockState { } pub fn is_authorized(&self, author: &Address) -> bool { - self.signers.contains(author) && !self.recent_signers.contains(author) + self.signers.binary_search(author).is_ok() && !self.recent_signers.contains(author) } // Returns whether it makes sense to cast the specified vote in the // current state (e.g. don't try to add an already authorized signer). pub fn is_valid_vote(&self, address: &Address, vote_type: VoteType) -> bool { - let in_signer = self.signers.contains(address); + let in_signer = self.signers.binary_search(address).is_ok(); match vote_type { VoteType::Add => !in_signer, VoteType::Remove => in_signer, diff --git a/ethcore/src/engines/clique/mod.rs b/ethcore/src/engines/clique/mod.rs index b8ccd82bd1d..3d3ddd6abfc 100644 --- a/ethcore/src/engines/clique/mod.rs +++ b/ethcore/src/engines/clique/mod.rs @@ -64,9 +64,9 @@ use std::time::{Duration, SystemTime, UNIX_EPOCH}; use block::{ExecutedBlock, IsBlock}; use client::{BlockId, EngineClient}; use engines::clique::util::{extract_signers, recover_creator}; -use engines::{Engine, Seal}; -use error::Error; -use ethereum_types::{Address, H160, H256, U256}; +use engines::{Engine, EngineError, Seal}; +use error::{BlockError, Error}; +use ethereum_types::{Address, H64, H160, H256, U256}; use ethkey::Signature; use hash::KECCAK_EMPTY_LIST_RLP; use itertools::Itertools; @@ -74,8 +74,8 @@ use lru_cache::LruCache; use machine::{Call, EthereumMachine}; use parking_lot::RwLock; use rand::Rng; -use rlp::encode; use super::signer::EngineSigner; +use unexpected::{Mismatch, OutOfBounds}; use types::BlockNumber; use types::header::{ExtendedHeader, Header}; @@ -92,7 +92,7 @@ mod util; #[cfg(test)] mod tests; -// protocol constants +// Protocol constants /// Fixed number of extra-data prefix bytes reserved for signer vanity pub const VANITY_LENGTH: usize = 32; /// Fixed number of extra-data suffix bytes reserved for signer signature @@ -100,9 +100,9 @@ pub const SIGNATURE_LENGTH: usize = 65; /// Address length of signer pub const ADDRESS_LENGTH: usize = 20; /// Nonce value for DROP vote -pub const NONCE_DROP_VOTE: &[u8] = &[0x00; 8]; +pub const NONCE_DROP_VOTE: H64 = H64([0; 8]); /// Nonce value for AUTH vote -pub const NONCE_AUTH_VOTE: &[u8] = &[0xff; 8]; +pub const NONCE_AUTH_VOTE: H64 = H64([0xff; 8]); /// Difficulty for INTURN block pub const DIFF_INTURN: U256 = U256([2, 0, 0, 0]); /// Difficulty for NOTURN block @@ -110,14 +110,17 @@ pub const DIFF_NOTURN: U256 = U256([1, 0, 0, 0]); /// Default empty author field value pub const NULL_AUTHOR: Address = H160([0x00; 20]); /// Default empty nonce value -pub const NULL_NONCE: &[u8] = NONCE_DROP_VOTE; +pub const NULL_NONCE: H64 = NONCE_DROP_VOTE; /// Default value for mixhash -pub const NULL_MIXHASH: &[u8] = &[0x00; 32]; +pub const NULL_MIXHASH: H256 = H256([0; 32]); /// Default value for uncles hash pub const NULL_UNCLES_HASH: H256 = KECCAK_EMPTY_LIST_RLP; /// Default noturn block wiggle factor defined in spec. pub const SIGNING_DELAY_NOTURN_MS: u64 = 500; +/// How many CliqueBlockState to cache in the memory. +pub const STATE_CACHE_NUM: usize = 128; + /// Vote to add or remove the beneficiary #[derive(Copy, Clone, Debug, PartialEq, PartialOrd)] pub enum VoteType { @@ -126,27 +129,26 @@ pub enum VoteType { } impl VoteType { - pub fn from_nonce(nonce: &[u8]) -> Result { - match nonce { - NONCE_AUTH_VOTE => Ok(VoteType::Add), - NONCE_DROP_VOTE => Ok(VoteType::Remove), - _ => Err(From::from("nonce was not AUTH or DROP")) + /// Try to construct a `Vote` from a nonce + pub fn from_nonce(nonce: H64) -> Result { + if nonce == NONCE_AUTH_VOTE { + Ok(VoteType::Add) + } else if nonce == NONCE_DROP_VOTE { + Ok(VoteType::Remove) + } else { + Err(EngineError::CliqueInvalidNonce(nonce))? } } /// Get the rlp encoding of the vote pub fn as_rlp(&self) -> Vec> { match self { - VoteType::Add => vec![encode(&NULL_MIXHASH.to_vec()), encode(&NONCE_AUTH_VOTE.to_vec())], - VoteType::Remove => vec![encode(&NULL_MIXHASH.to_vec()), encode(&NONCE_DROP_VOTE.to_vec())], + VoteType::Add => vec![rlp::encode(&NULL_MIXHASH), rlp::encode(&NONCE_AUTH_VOTE)], + VoteType::Remove => vec![rlp::encode(&NULL_MIXHASH), rlp::encode(&NONCE_DROP_VOTE)], } } } -// Caches -/// How many CliqueBlockState to cache in the memory. -pub const STATE_CACHE_NUM: usize = 128; - /// Clique Engine implementation /// block_state_by_hash -> block state indexed by header hash. #[cfg(not(test))] @@ -217,19 +219,16 @@ impl Clique { } fn sign_header(&self, header: &Header) -> Result<(Signature, H256), Error> { + match self.signer.read().as_ref() { None => { - return Err(Box::new("sign_header: No signer available.").into()); + Err(EngineError::RequiresSigner)? } Some(signer) => { let digest = header.hash(); match signer.sign(digest) { - Ok(sig) => { - return Ok((sig, digest)); - } - Err(e) => { - return Err(Box::new(format!("sign_header: failed to sign header, error: {}", e)).into()); - } + Ok(sig) => Ok((sig, digest)), + Err(e) => Err(EngineError::Custom(e.into()))?, } } } @@ -266,7 +265,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(From::from("failed to upgrade client reference")); + return Err(EngineError::RequiresClient)?; } Some(c) => { let last_checkpoint_number = header.number() - header.number() % self.epoch_length as u64; @@ -280,9 +279,9 @@ impl Clique { // populate chain to last checkpoint loop { - let (last_parent_hash, last_hash, last_num) = { + let (last_parent_hash, last_num) = { let l = chain.front().expect("chain has at least one element; qed"); - (*l.parent_hash(), l.hash(), l.number()) + (*l.parent_hash(), l.number()) }; if last_num == last_checkpoint_number + 1 { @@ -290,7 +289,7 @@ impl Clique { } match c.block_header(BlockId::Hash(last_parent_hash)) { None => { - return Err(Box::new(format!("parent block {} of {} could not be recovered.", last_parent_hash, last_hash)).into()); + return Err(BlockError::UnknownParent(last_parent_hash))?; } Some(next) => { chain.push_front(next.decode()?); @@ -301,16 +300,17 @@ impl Clique { // Catching up state, note that we don't really store block state for intermediary blocks, // for speed. let backfill_start = time::Instant::now(); - trace!(target: "engine", + info!(target: "engine", "Back-filling block state. last_checkpoint_number: {}, target: {}({}).", last_checkpoint_number, header.number(), header.hash()); // Get the state for last checkpoint. - let last_checkpoint_hash = *(chain.front().ok_or( - "just pushed to front, reference must exist; qed" - )?.parent_hash()); + let last_checkpoint_hash = *chain.front() + .expect("chain has at least one element; qed") + .parent_hash(); + let last_checkpoint_header = match c.block_header(BlockId::Hash(last_checkpoint_hash)) { - None => return Err(From::from("Unable to find last checkpoint block")), + None => return Err(EngineError::CliqueMissingCheckpoint(last_checkpoint_hash))?, Some(header) => header.decode()?, }; @@ -372,9 +372,8 @@ impl Engine for Clique { let mut header = block.header().clone(); - let state = self.state_no_backfill(header.parent_hash()).ok_or_else( - || format!("on_seal_block: Unable to get parent state: {}", header.parent_hash()) - )?; + let state = self.state_no_backfill(header.parent_hash()) + .ok_or_else(|| BlockError::UnknownParent(*header.parent_hash()))?; let is_checkpoint = header.number() % self.epoch_length == 0; @@ -406,7 +405,11 @@ impl Engine for Clique { // At this point, extra_data should only contain miner vanity. if header.extra_data().len() > VANITY_LENGTH { - panic!("on_seal_block: unexpected extra_data: {:?}", header); + return Err(EngineError::BadExtraDataFieldSize(OutOfBounds { + min: Some(0), + max: Some(VANITY_LENGTH), + found: header.extra_data().len() + }))?; } // vanity { @@ -529,60 +532,92 @@ impl Engine for Clique { // Don't waste time checking blocks from the future { - let limit = SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_default().as_secs() + self.period; - if header.timestamp() > limit { - return Err(Box::new( - format!("Block is too far in the future, timestamp: {}, limit: {}", header.timestamp(), limit) - ).into()); + // TODO(niklasad): checked_add + let limit = SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_default() + Duration::from_secs(self.period); + let hdr = Duration::from_secs(header.timestamp()); + if hdr > limit { + return Err(BlockError::TemporarilyInvalid(OutOfBounds { + min: None, + max: Some(UNIX_EPOCH + limit), + found: UNIX_EPOCH + Duration::from_secs(header.timestamp()), + }))?; } } let is_checkpoint = header.number() % self.epoch_length == 0; if is_checkpoint && *header.author() != NULL_AUTHOR { - return Err(Box::new("Checkpoint block must enforce zero beneficiary").into()); + return Err(EngineError::CliqueWrongAuthorCheckpoint(Mismatch { + expected: 0.into(), + found: *header.author(), + }))?; } - // Nonce must be 0x00..0 or 0xff..f let seal_fields = header.decode_seal::>()?; - let mixhash = seal_fields.get(0).ok_or("No mixhash field.")?; - let nonce = seal_fields.get(1).ok_or("No nonce field.")?; - if *nonce != NONCE_DROP_VOTE && *nonce != NONCE_AUTH_VOTE { - return Err(Box::new("nonce must be 0x00..0 or 0xff..f").into()); + if seal_fields.len() != 2 { + Err(BlockError::InvalidSealArity(Mismatch { + expected: 2, + found: seal_fields.len(), + }))? } - if is_checkpoint && *nonce != NULL_NONCE { - return Err(Box::new("Checkpoint block must have zero nonce").into()); + + let mixhash: H256 = seal_fields[0].into(); + let nonce: H64 = seal_fields[1].into(); + + // Nonce must be 0x00..0 or 0xff..f + if nonce != NONCE_DROP_VOTE && nonce != NONCE_AUTH_VOTE { + Err(EngineError::CliqueInvalidNonce(nonce))?; } - // Ensure that the extra-data contains a signer list on checkpoint, but none otherwise. - if (!is_checkpoint && header.extra_data().len() != (VANITY_LENGTH + SIGNATURE_LENGTH)) - || (is_checkpoint && header.extra_data().len() <= (VANITY_LENGTH + SIGNATURE_LENGTH)) - || (is_checkpoint && (header.extra_data().len() - (VANITY_LENGTH + SIGNATURE_LENGTH)) % ADDRESS_LENGTH != 0) { - return Err(Box::new(format!("Invalid extra_data length, got {}", header.extra_data().len())).into()); + if is_checkpoint && nonce != NULL_NONCE { + Err(EngineError::CliqueInvalidNonce(nonce))?; } // Ensure that the mix digest is zero as Clique don't have fork protection currently - if *mixhash != NULL_MIXHASH { - return Err(Box::new("mixhash must be 0x00..0 or 0xff..f.").into()); + if mixhash != NULL_MIXHASH { + Err(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)? + } + + if extra_data_len < VANITY_LENGTH + SIGNATURE_LENGTH { + Err(EngineError::CliqueMissingSignature)? + } + + 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)? + } + + // Addresses must be be divisable by 20 + if is_checkpoint && signers % ADDRESS_LENGTH != 0 { + Err(EngineError::CliqueCheckpointInvalidSigners(signers))? } // Ensure that the block doesn't contain any uncles which are meaningless in PoA if *header.uncles_hash() != NULL_UNCLES_HASH { - return Err(Box::new(format!( - "Invalid uncle hash, got: {}, expected: {}.", - header.uncles_hash(), - NULL_UNCLES_HASH, - )).into()); + Err(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 { - return Err(Box::new(format!( - "invalid difficulty: expected {} or {}, got: {}.", - DIFF_INTURN, - DIFF_NOTURN, - header.difficulty(), - )).into()); + Err(BlockError::DifficultyOutOfBounds(OutOfBounds { + min: Some(DIFF_NOTURN), + max: Some(DIFF_INTURN), + found: *header.difficulty(), + }))? } // All basic checks passed, continue to next phase @@ -604,12 +639,16 @@ impl Engine for Clique { // parent sanity check if parent.hash() != *header.parent_hash() || header.number() != parent.number() + 1 { - return Err(Box::new("invalid parent").into()); + Err(BlockError::UnknownParent(parent.hash()))? } // Ensure that the block's timestamp isn't too close to it's parent if parent.timestamp().saturating_add(self.period) > header.timestamp() { - return Err(Box::new("invalid timestamp").into()); + Err(BlockError::InvalidTimestamp(OutOfBounds { + min: Some(UNIX_EPOCH + Duration::from_secs(parent.timestamp().saturating_add(self.period))), + max: None, + found: UNIX_EPOCH + Duration::from_secs(header.timestamp()) + }))? } // Retrieve the parent state @@ -633,42 +672,41 @@ impl Engine for Clique { } // Our task here is to set difficulty + // TODO:(niklasad1): Return `Result<(), Error>` here instead fn populate_from_parent(&self, header: &mut Header, parent: &Header) { // TODO(https://github.com/paritytech/parity-ethereum/issues/10410): this is a horrible hack, // it is due to the fact that enact and miner both use OpenBlock::new() which will both call // this function. more refactoring is definitely needed. - match header.extra_data().len() >= VANITY_LENGTH + SIGNATURE_LENGTH { - true => { - // we are importing blocks, do nothing. - } - false => { - trace!(target: "engine", "populate_from_parent in sealing"); - - // It's unclear how to prevent creating new blocks unless we are authorized, the best way (and geth does this too) - // it's just to ignore setting an correct difficulty here, we will check authorization in next step in generate_seal anyway. - if let Some(signer) = self.signer.read().as_ref() { - match self.state(&parent) { - Err(e) => { - trace!(target: "engine", "populate_from_parent: Unable to find parent state: {}, ignored.", e); - } - Ok(state) => { - if state.is_authorized(&signer.address()) { - if state.is_inturn(header.number(), &signer.address()) { - header.set_difficulty(DIFF_INTURN); - } else { - header.set_difficulty(DIFF_NOTURN); - } - } - } + if header.extra_data().len() < VANITY_LENGTH + SIGNATURE_LENGTH { + trace!(target: "engine", "populate_from_parent in sealing"); + + let state = match self.state(&parent) { + Err(e) => { + trace!(target: "engine", "populate_from_parent: Unable to find parent state: {}, ignored.", e); + return; + } + Ok(state) => state, + }; + + + // It's unclear how to prevent creating new blocks unless we are authorized, the best way (and geth does this too) + // it's just to ignore setting an correct difficulty here, we will check authorization in next step in generate_seal anyway. + if let Some(signer) = self.signer.read().as_ref() { + if state.is_authorized(&signer.address()) { + if state.is_inturn(header.number(), &signer.address()) { + header.set_difficulty(DIFF_INTURN); + } else { + header.set_difficulty(DIFF_NOTURN); } } + } else { + trace!(target: "engine", "populate_from_parent: no signer registered"); } } } fn set_signer(&self, signer: Box) { trace!(target: "engine", "set_signer: {}", signer.address()); - *self.signer.write() = Some(signer); } diff --git a/ethcore/src/engines/clique/tests.rs b/ethcore/src/engines/clique/tests.rs index 2db7e31704a..a9dc4efa3d5 100644 --- a/ethcore/src/engines/clique/tests.rs +++ b/ethcore/src/engines/clique/tests.rs @@ -18,7 +18,7 @@ use block::*; use engines::Engine; -use error::Error; +use error::{Error, ErrorKind}; use ethereum_types::{Address, H256}; use ethkey::{Secret, KeyPair}; use state_db::StateDB; @@ -713,16 +713,27 @@ fn epoch_transition_reset_all_votes() { #[test] fn unauthorized_signer_should_not_be_able_to_sign_block() { let tester = CliqueTester::with(3, 1, vec!['A']); - assert!(tester.new_block_and_import(CliqueBlockType::Empty, &tester.genesis, None, 'B').is_err()); + let err = tester.new_block_and_import(CliqueBlockType::Empty, &tester.genesis, None, 'B').unwrap_err(); + + match err.kind() { + ErrorKind::Engine(EngineError::NotAuthorized(_)) => (), + _ => assert!(true == false, "Wrong error kind"), + } } #[test] fn signer_should_not_be_able_to_sign_two_consequtive_blocks() { let tester = CliqueTester::with(3, 1, vec!['A', 'B']); let b = tester.new_block_and_import(CliqueBlockType::Empty, &tester.genesis, None, 'A').unwrap(); - assert!(tester.new_block_and_import(CliqueBlockType::Empty, &b, None, 'A').is_err()); + let err = tester.new_block_and_import(CliqueBlockType::Empty, &b, None, 'A').unwrap_err(); + + match err.kind() { + ErrorKind::Engine(EngineError::CliqueTooRecentlySigned(_)) => (), + _ => assert!(true == false, "Wrong error kind"), + } } + #[test] fn recent_signers_should_not_reset_on_checkpoint() { let tester = CliqueTester::with(3, 1, vec!['A', 'B', 'C']); @@ -730,7 +741,13 @@ fn recent_signers_should_not_reset_on_checkpoint() { let block = tester.new_block_and_import(CliqueBlockType::Empty, &tester.genesis, None, 'A').unwrap(); let block = tester.new_block_and_import(CliqueBlockType::Empty, &block, None, 'B').unwrap(); let block = tester.new_block_and_import(CliqueBlockType::Checkpoint, &block, None, 'A').unwrap(); - assert!(tester.new_block_and_import(CliqueBlockType::Empty, &block, None, 'A').is_err()); + + let err = tester.new_block_and_import(CliqueBlockType::Empty, &block, None, 'A').unwrap_err(); + + match err.kind() { + ErrorKind::Engine(EngineError::CliqueTooRecentlySigned(_)) => (), + _ => assert!(true == false, "Wrong error kind"), + } } // Not part of http://eips.ethereum.org/EIPS/eip-225 diff --git a/ethcore/src/engines/mod.rs b/ethcore/src/engines/mod.rs index 1625c502053..c274d40484d 100644 --- a/ethcore/src/engines/mod.rs +++ b/ethcore/src/engines/mod.rs @@ -49,10 +49,10 @@ use types::BlockNumber; use snapshot::SnapshotComponents; use spec::CommonParams; use types::transaction::{self, UnverifiedTransaction, SignedTransaction}; -use ethkey::{Signature}; +use ethkey::Signature; use types::header::Header; use parity_machine::{Machine, LocalizedMachine as Localized, TotalScoredHeader, Header as MachineHeader}; -use ethereum_types::{H256, U256, Address}; +use ethereum_types::{H64, H256, U256, Address}; use unexpected::{Mismatch, OutOfBounds}; use bytes::Bytes; use types::ancestry_action::AncestryAction; @@ -72,6 +72,8 @@ pub enum EngineError { NotProposer(Mismatch
), /// Message was not expected. UnexpectedMessage, + /// Extra data field has an unexpected size. + BadExtraDataFieldSize(OutOfBounds), /// Seal field has an unexpected size. BadSealFieldSize(OutOfBounds), /// Validation proof insufficient. @@ -84,21 +86,56 @@ pub enum EngineError { RequiresClient, /// Invalid engine specification or implementation. InvalidEngine, + /// Requires signer ref, but none registered. + RequiresSigner, + /// Checkpoint is missing + CliqueMissingCheckpoint(H256), + /// Missing vanity data + CliqueMissingVanity, + /// Missing signature + CliqueMissingSignature, + /// Missing signers + CliqueCheckpointNoSigner, + /// List of signers is invalid + CliqueCheckpointInvalidSigners(usize), + /// Wrong author on a checkpoint + CliqueWrongAuthorCheckpoint(Mismatch
), + /// Wrong checkpoint authors recovered + CliqueFaultyRecoveredSigners(Vec), + /// Invalid nonce (should contain vote) + CliqueInvalidNonce(H64), + /// The signer signed a block to recently + CliqueTooRecentlySigned(Address), + /// Custom + Custom(String), } impl fmt::Display for EngineError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { use self::EngineError::*; let msg = match *self { + CliqueMissingCheckpoint(ref hash) => format!("Missing checkpoint block: {}", hash), + CliqueMissingVanity => format!("Extra data is missing vanity data"), + CliqueMissingSignature => format!("Extra data is missing signature"), + CliqueCheckpointInvalidSigners(len) => format!("Checkpoint block list was of length: {} of checkpoint but + it needs to be bigger than zero and a divisable by 20", len), + CliqueCheckpointNoSigner => format!("Checkpoint block list of signers was empty"), + CliqueInvalidNonce(ref mis) => format!("Unexpected nonce {} expected {} or {}", mis, 0_u64, u64::max_value()), + CliqueWrongAuthorCheckpoint(ref oob) => format!("Unexpected checkpoint author: {}", oob), + CliqueFaultyRecoveredSigners(ref mis) => format!("Faulty recovered signers {:?}", mis), + CliqueTooRecentlySigned(ref address) => format!("The signer: {} has signed a block too recently", address), + Custom(ref s) => s.clone(), DoubleVote(ref address) => format!("Author {} issued too many blocks.", address), NotProposer(ref mis) => format!("Author is not a current proposer: {}", mis), NotAuthorized(ref address) => format!("Signer {} is not authorized.", address), UnexpectedMessage => "This Engine should not be fed messages.".into(), + BadExtraDataFieldSize(ref oob) => format!("Extra data field has an unexpected length: {}", oob), BadSealFieldSize(ref oob) => format!("Seal field has an unexpected length: {}", oob), InsufficientProof(ref msg) => format!("Insufficient validation proof: {}", msg), 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"), + RequiresSigner => format!("Call requires client but none registered"), InvalidEngine => format!("Invalid engine specification or implementation"), }; From 3a999a6dbe427f1846964bd8ee005dced7862625 Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Wed, 13 Mar 2019 12:22:24 +0100 Subject: [PATCH 2/3] fix(clique utils): make use of errors --- ethcore/src/engines/clique/util.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/ethcore/src/engines/clique/util.rs b/ethcore/src/engines/clique/util.rs index 9c294f3f028..c0c8938567a 100644 --- a/ethcore/src/engines/clique/util.rs +++ b/ethcore/src/engines/clique/util.rs @@ -14,14 +14,14 @@ // You should have received a copy of the GNU General Public License // along with Parity Ethereum. If not, see . +use engines::EngineError; +use engines::clique::{ADDRESS_LENGTH, SIGNATURE_LENGTH, VANITY_LENGTH, NULL_NONCE, NULL_MIXHASH}; +use error::Error; use ethereum_types::{Address, H256}; +use ethkey::{public_to_address, recover as ec_recover, Signature}; use lru_cache::LruCache; use parking_lot::RwLock; use rlp::encode; - -use engines::clique::{ADDRESS_LENGTH, SIGNATURE_LENGTH, VANITY_LENGTH, NULL_NONCE, NULL_MIXHASH}; -use error::Error; -use ethkey::{public_to_address, recover as ec_recover, Signature}; use types::header::Header; /// How many recovered signature to cache in the memory. @@ -42,13 +42,16 @@ pub fn recover_creator(header: &Header) -> Result { } let data = header.extra_data(); + if data.len() < VANITY_LENGTH { + Err(EngineError::CliqueMissingVanity)? + } + if data.len() < VANITY_LENGTH + SIGNATURE_LENGTH { - return Err(From::from("extra_data length is not enough!")); + Err(EngineError::CliqueMissingSignature)? } - // Get vanity and signature data from `header.extra_data` - // Note, this shouldn't have a list of signers because this is only called on `non-checkpoints` - let (vanity_slice, signature_slice) = data.split_at(data.len() - SIGNATURE_LENGTH); + // Split `signed_extra data` and `signature` + let (signed_data_slice, signature_slice) = data.split_at(data.len() - SIGNATURE_LENGTH); // convert `&[u8]` to `[u8; 65]` let signature = { @@ -59,7 +62,7 @@ pub fn recover_creator(header: &Header) -> Result { // modify header and hash it let unsigned_header = &mut header.clone(); - unsigned_header.set_extra_data(vanity_slice.to_vec()); + unsigned_header.set_extra_data(signed_data_slice.to_vec()); let msg = unsigned_header.hash(); let pubkey = ec_recover(&Signature::from(signature), &msg)?; @@ -81,14 +84,14 @@ pub fn extract_signers(header: &Header) -> Result, Error> { let data = header.extra_data(); if data.len() <= VANITY_LENGTH + SIGNATURE_LENGTH { - return Err(Box::new("Invalid extra_data size.").into()); + Err(EngineError::CliqueCheckpointNoSigner)? } // extract only the portion of extra_data which includes the signer list let signers_raw = &data[(VANITY_LENGTH)..data.len() - (SIGNATURE_LENGTH)]; if signers_raw.len() % ADDRESS_LENGTH != 0 { - return Err(Box::new("bad signer list.").into()); + Err(EngineError::CliqueCheckpointInvalidSigners(signers_raw.len()))? } let num_signers = signers_raw.len() / 20; From 98218fecbe6539a474ac616293a2b818d930fdb8 Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Wed, 13 Mar 2019 12:23:23 +0100 Subject: [PATCH 3/3] fix(cleanup nits) --- ethcore/src/engines/clique/block_state.rs | 10 +++++----- ethcore/src/engines/clique/mod.rs | 9 ++++----- ethcore/src/engines/mod.rs | 3 --- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/ethcore/src/engines/clique/block_state.rs b/ethcore/src/engines/clique/block_state.rs index 96d2e3e4c04..2df1d869fc1 100644 --- a/ethcore/src/engines/clique/block_state.rs +++ b/ethcore/src/engines/clique/block_state.rs @@ -138,7 +138,6 @@ impl CliqueBlockState { Err(EngineError::CliqueTooRecentlySigned(creator))? } - // Wrong difficulty let inturn = self.is_inturn(header.number(), &creator); @@ -187,11 +186,12 @@ impl CliqueBlockState { // Contains vote if *header.author() != NULL_AUTHOR { - let nonce: H64 = header.decode_seal::>()?.get(1) - .cloned() - .map(Into::into) - .ok_or(BlockError::InvalidSeal)?; + let decoded_seal = header.decode_seal::>()?; + if decoded_seal.len() != 2 { + Err(BlockError::InvalidSealArity(Mismatch { expected: 2, found: decoded_seal.len() }))? + } + let nonce: H64 = decoded_seal[1].into(); self.update_signers_on_vote(VoteType::from_nonce(nonce)?, creator, *header.author(), header.number())?; } diff --git a/ethcore/src/engines/clique/mod.rs b/ethcore/src/engines/clique/mod.rs index 3d3ddd6abfc..8d16c211247 100644 --- a/ethcore/src/engines/clique/mod.rs +++ b/ethcore/src/engines/clique/mod.rs @@ -300,7 +300,7 @@ impl Clique { // Catching up state, note that we don't really store block state for intermediary blocks, // for speed. let backfill_start = time::Instant::now(); - info!(target: "engine", + trace!(target: "engine", "Back-filling block state. last_checkpoint_number: {}, target: {}({}).", last_checkpoint_number, header.number(), header.hash()); @@ -404,9 +404,9 @@ impl Engine for Clique { 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 { - return Err(EngineError::BadExtraDataFieldSize(OutOfBounds { - min: Some(0), + if header.extra_data().len() != VANITY_LENGTH { + Err(BlockError::ExtraDataOutOfBounds(OutOfBounds { + min: Some(VANITY_LENGTH), max: Some(VANITY_LENGTH), found: header.extra_data().len() }))?; @@ -688,7 +688,6 @@ impl Engine for Clique { Ok(state) => state, }; - // It's unclear how to prevent creating new blocks unless we are authorized, the best way (and geth does this too) // it's just to ignore setting an correct difficulty here, we will check authorization in next step in generate_seal anyway. if let Some(signer) = self.signer.read().as_ref() { diff --git a/ethcore/src/engines/mod.rs b/ethcore/src/engines/mod.rs index c274d40484d..fa23e3af06a 100644 --- a/ethcore/src/engines/mod.rs +++ b/ethcore/src/engines/mod.rs @@ -72,8 +72,6 @@ pub enum EngineError { NotProposer(Mismatch
), /// Message was not expected. UnexpectedMessage, - /// Extra data field has an unexpected size. - BadExtraDataFieldSize(OutOfBounds), /// Seal field has an unexpected size. BadSealFieldSize(OutOfBounds), /// Validation proof insufficient. @@ -129,7 +127,6 @@ impl fmt::Display for EngineError { NotProposer(ref mis) => format!("Author is not a current proposer: {}", mis), NotAuthorized(ref address) => format!("Signer {} is not authorized.", address), UnexpectedMessage => "This Engine should not be fed messages.".into(), - BadExtraDataFieldSize(ref oob) => format!("Extra data field has an unexpected length: {}", oob), BadSealFieldSize(ref oob) => format!("Seal field has an unexpected length: {}", oob), InsufficientProof(ref msg) => format!("Insufficient validation proof: {}", msg), FailedSystemCall(ref msg) => format!("Failed to make system call: {}", msg),