From 842b75c0e63f76e1b77d75529c7f6c3f4969d8d7 Mon Sep 17 00:00:00 2001 From: David Date: Wed, 9 May 2018 12:05:56 +0200 Subject: [PATCH] Decoding headers can fail (#8570) * rlp::decode returns Result * Fix journaldb to handle rlp::decode Result * Fix ethcore to work with rlp::decode returning Result * Light client handles rlp::decode returning Result * Fix tests in rlp_derive * Fix tests * Cleanup * cleanup * Allow panic rather than breaking out of iterator * Let decoding failures when reading from disk blow up * syntax * Fix the trivial grumbles * Fix failing tests * Make Account::from_rlp return Result * Syntx, sigh * Temp-fix for decoding failures * Header::decode returns Result Handle new return type throughout the code base. * Do not continue reading from the DB when a value could not be read * Fix tests * Handle header decoding in light_sync * Handling header decoding errors * Let the DecodeError bubble up unchanged * Remove redundant error conversion --- ethcore/light/src/client/header_chain.rs | 10 ++++--- ethcore/light/src/client/mod.rs | 12 ++++++-- ethcore/src/client/client.rs | 14 ++++++---- ethcore/src/client/test_client.rs | 5 ++-- ethcore/src/encoded.rs | 6 ++-- ethcore/src/engines/authority_round/mod.rs | 2 +- ethcore/src/error.rs | 10 +++++-- ethcore/src/miner/miner.rs | 11 ++++++-- ethcore/src/snapshot/mod.rs | 2 +- ethcore/src/verification/verification.rs | 5 ++-- ethcore/sync/src/light_sync/response.rs | 32 ++++++++++++---------- ethcore/sync/src/light_sync/tests/mod.rs | 2 +- rpc/src/v1/helpers/errors.rs | 13 +++++++++ rpc/src/v1/impls/eth.rs | 13 +++++---- rpc/src/v1/impls/light/eth.rs | 2 +- rpc/src/v1/impls/light/parity.rs | 2 +- rpc/src/v1/impls/parity.rs | 4 +-- rpc/src/v1/impls/traces.rs | 6 ++-- 18 files changed, 98 insertions(+), 53 deletions(-) diff --git a/ethcore/light/src/client/header_chain.rs b/ethcore/light/src/client/header_chain.rs index 02a18a60d..b85091e53 100644 --- a/ethcore/light/src/client/header_chain.rs +++ b/ethcore/light/src/client/header_chain.rs @@ -305,7 +305,7 @@ impl HeaderChain { batch.put(col, cht_key(cht_num as u64).as_bytes(), &::rlp::encode(cht_root)); } - let decoded_header = hardcoded_sync.header.decode(); + let decoded_header = hardcoded_sync.header.decode()?; let decoded_header_num = decoded_header.number(); // write the block in the DB. @@ -585,7 +585,7 @@ impl HeaderChain { bail!(ErrorKind::Database(msg.into())); }; - let decoded = header.decode(); + let decoded = header.decode().expect("decoding db value failed"); let entry: Entry = { let bytes = self.db.get(self.col, era_key(h_num).as_bytes())? @@ -815,7 +815,9 @@ impl HeaderChain { for hdr in self.ancestry_iter(BlockId::Hash(parent_hash)) { if let Some(transition) = live_proofs.get(&hdr.hash()).cloned() { - return Some((hdr.decode(), transition.proof)) + return hdr.decode().map(|decoded_hdr| { + (decoded_hdr, transition.proof) + }).ok(); } } @@ -1224,7 +1226,7 @@ mod tests { let hardcoded_sync = chain.read_hardcoded_sync().expect("failed reading hardcoded sync").expect("failed unwrapping hardcoded sync"); assert_eq!(hardcoded_sync.chts.len(), 3); assert_eq!(hardcoded_sync.total_difficulty, total_difficulty); - let decoded: Header = hardcoded_sync.header.decode(); + let decoded: Header = hardcoded_sync.header.decode().expect("decoding failed"); assert_eq!(decoded.number(), h_num); } } diff --git a/ethcore/light/src/client/mod.rs b/ethcore/light/src/client/mod.rs index cf603d853..82b424cc8 100644 --- a/ethcore/light/src/client/mod.rs +++ b/ethcore/light/src/client/mod.rs @@ -318,7 +318,7 @@ impl Client { let epoch_proof = self.engine.is_epoch_end( &verified_header, - &|h| self.chain.block_header(BlockId::Hash(h)).map(|hdr| hdr.decode()), + &|h| self.chain.block_header(BlockId::Hash(h)).and_then(|hdr| hdr.decode().ok()), &|h| self.chain.pending_transition(h), ); @@ -426,7 +426,15 @@ impl Client { }; // Verify Block Family - let verify_family_result = self.engine.verify_block_family(&verified_header, &parent_header.decode()); + + let verify_family_result = { + parent_header.decode() + .map_err(|dec_err| dec_err.into()) + .and_then(|decoded| { + self.engine.verify_block_family(&verified_header, &decoded) + }) + + }; if let Err(e) = verify_family_result { warn!(target: "client", "Stage 3 block verification failed for #{} ({})\nError: {:?}", verified_header.number(), verified_header.hash(), e); diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index bffa4e38b..76d78e3df 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1219,8 +1219,7 @@ impl Client { => Some(self.chain.read().best_block_header()), BlockId::Number(number) if number == self.chain.read().best_block_number() => Some(self.chain.read().best_block_header()), - _ - => self.block_header(id).map(|h| h.decode()), + _ => self.block_header(id).and_then(|h| h.decode().ok()) } } } @@ -1915,7 +1914,11 @@ impl BlockChainClient for Client { fn uncle_extra_info(&self, id: UncleId) -> Option> { self.uncle(id) - .map(|header| self.engine.extra_info(&header.decode())) + .and_then(|h| { + h.decode().map(|dh| { + self.engine.extra_info(&dh) + }).ok() + }) } fn pruning_info(&self) -> PruningInfo { @@ -2033,7 +2036,8 @@ impl ReopenBlock for Client { for h in uncles { if !block.uncles().iter().any(|header| header.hash() == h) { let uncle = chain.block_header_data(&h).expect("find_uncle_hashes only returns hashes for existing headers; qed"); - block.push_uncle(uncle.decode()).expect("pushing up to maximum_uncle_count; + let uncle = uncle.decode().expect("decoding failure"); + block.push_uncle(uncle).expect("pushing up to maximum_uncle_count; push_uncle is not ok only if more than maximum_uncle_count is pushed; so all push_uncle are Ok; qed"); @@ -2074,7 +2078,7 @@ impl PrepareOpenBlock for Client { .into_iter() .take(engine.maximum_uncle_count(open_block.header().number())) .foreach(|h| { - open_block.push_uncle(h.decode()).expect("pushing maximum_uncle_count; + open_block.push_uncle(h.decode().expect("decoding failure")).expect("pushing maximum_uncle_count; open_block was just created; push_uncle is not ok only if more than maximum_uncle_count is pushed; so all push_uncle are Ok; diff --git a/ethcore/src/client/test_client.rs b/ethcore/src/client/test_client.rs index b22915966..6a3166f7c 100644 --- a/ethcore/src/client/test_client.rs +++ b/ethcore/src/client/test_client.rs @@ -289,7 +289,7 @@ impl TestBlockChainClient { /// Make a bad block by setting invalid extra data. pub fn corrupt_block(&self, n: BlockNumber) { let hash = self.block_hash(BlockId::Number(n)).unwrap(); - let mut header: BlockHeader = self.block_header(BlockId::Number(n)).unwrap().decode(); + let mut header: BlockHeader = self.block_header(BlockId::Number(n)).unwrap().decode().expect("decoding failed"); header.set_extra_data(b"This extra data is way too long to be considered valid".to_vec()); let mut rlp = RlpStream::new_list(3); rlp.append(&header); @@ -301,7 +301,7 @@ impl TestBlockChainClient { /// Make a bad block by setting invalid parent hash. pub fn corrupt_block_parent(&self, n: BlockNumber) { let hash = self.block_hash(BlockId::Number(n)).unwrap(); - let mut header: BlockHeader = self.block_header(BlockId::Number(n)).unwrap().decode(); + let mut header: BlockHeader = self.block_header(BlockId::Number(n)).unwrap().decode().expect("decoding failed"); header.set_parent_hash(H256::from(42)); let mut rlp = RlpStream::new_list(3); rlp.append(&header); @@ -479,6 +479,7 @@ impl BlockInfo for TestBlockChainClient { self.block_header(BlockId::Hash(self.chain_info().best_block_hash)) .expect("Best block always has header.") .decode() + .expect("decoding failed") } fn block(&self, id: BlockId) -> Option { diff --git a/ethcore/src/encoded.rs b/ethcore/src/encoded.rs index 01df386cc..c436607f8 100644 --- a/ethcore/src/encoded.rs +++ b/ethcore/src/encoded.rs @@ -28,7 +28,7 @@ use ethereum_types::{H256, Bloom, U256, Address}; use hash::keccak; use header::{BlockNumber, Header as FullHeader}; use heapsize::HeapSizeOf; -use rlp::{Rlp, RlpStream}; +use rlp::{self, Rlp, RlpStream}; use transaction::UnverifiedTransaction; use views::{self, BlockView, HeaderView, BodyView}; @@ -47,7 +47,9 @@ impl Header { pub fn new(encoded: Vec) -> Self { Header(encoded) } /// Upgrade this encoded view to a fully owned `Header` object. - pub fn decode(&self) -> FullHeader { ::rlp::decode(&self.0).expect("decoding failure") } + pub fn decode(&self) -> Result { + rlp::decode(&self.0) + } /// Get a borrowed header view onto the data. #[inline] diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index c2aee7c6e..ed9a9a4f2 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -996,7 +996,7 @@ impl Engine for AuthorityRound { let parent = client.block_header(::client::BlockId::Hash(*block.header().parent_hash())) .expect("hash is from parent; parent header must exist; qed") - .decode(); + .decode()?; let parent_step = header_step(&parent, self.empty_steps_transition)?; let current_step = self.step.load(); diff --git a/ethcore/src/error.rs b/ethcore/src/error.rs index 561701e76..bec749297 100644 --- a/ethcore/src/error.rs +++ b/ethcore/src/error.rs @@ -290,6 +290,12 @@ error_chain! { description("Unknown engine name") display("Unknown engine name ({})", name) } + + #[doc = "RLP decoding errors"] + Decoder(err: ::rlp::DecoderError) { + description("decoding value failed") + display("decoding value failed with error: {}", err) + } } } @@ -310,11 +316,11 @@ impl From for Error { fn from(err: AccountsError) -> Error { ErrorKind::AccountProvider(err).into() } -} +} impl From<::rlp::DecoderError> for Error { fn from(err: ::rlp::DecoderError) -> Error { - UtilError::from(err).into() + ErrorKind::Decoder(err).into() } } diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 76a011343..3168ff1a8 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -528,8 +528,8 @@ impl Miner { } /// Attempts to perform internal sealing (one that does not require work) and handles the result depending on the type of Seal. - fn seal_and_import_block_internally(&self, chain: &C, block: ClosedBlock) -> bool where - C: BlockChain + SealedBlockImporter, + fn seal_and_import_block_internally(&self, chain: &C, block: ClosedBlock) -> bool + where C: BlockChain + SealedBlockImporter, { { let sealing = self.sealing.lock(); @@ -544,7 +544,12 @@ impl Miner { trace!(target: "miner", "seal_block_internally: attempting internal seal."); let parent_header = match chain.block_header(BlockId::Hash(*block.header().parent_hash())) { - Some(hdr) => hdr.decode(), + Some(h) => { + match h.decode() { + Ok(decoded_hdr) => decoded_hdr, + Err(_) => return false + } + } None => return false, }; diff --git a/ethcore/src/snapshot/mod.rs b/ethcore/src/snapshot/mod.rs index 94236e9e9..8871ced26 100644 --- a/ethcore/src/snapshot/mod.rs +++ b/ethcore/src/snapshot/mod.rs @@ -487,7 +487,7 @@ pub fn verify_old_block(rng: &mut OsRng, header: &Header, engine: &EthEngine, ch if always || rng.gen::() <= POW_VERIFY_RATE { engine.verify_block_unordered(header)?; match chain.block_header_data(header.parent_hash()) { - Some(parent) => engine.verify_block_family(header, &parent.decode()), + Some(parent) => engine.verify_block_family(header, &parent.decode()?), None => Ok(()), } } else { diff --git a/ethcore/src/verification/verification.rs b/ethcore/src/verification/verification.rs index 92f3e77f9..03a6d6f8d 100644 --- a/ethcore/src/verification/verification.rs +++ b/ethcore/src/verification/verification.rs @@ -224,7 +224,7 @@ fn verify_uncles(header: &Header, bytes: &[u8], bc: &BlockProvider, engine: &Eth return Err(From::from(BlockError::UncleParentNotInChain(uncle_parent.hash()))); } - let uncle_parent = uncle_parent.decode(); + let uncle_parent = uncle_parent.decode()?; verify_parent(&uncle, &uncle_parent, engine)?; engine.verify_block_family(&uncle, &uncle_parent)?; verified.insert(uncle.hash()); @@ -500,10 +500,9 @@ mod tests { // no existing tests need access to test, so having this not function // is fine. let client = ::client::TestBlockChainClient::default(); - let parent = bc.block_header_data(header.parent_hash()) .ok_or(BlockError::UnknownParent(header.parent_hash().clone()))? - .decode(); + .decode()?; let full_params = FullFamilyParams { block_bytes: bytes, diff --git a/ethcore/sync/src/light_sync/response.rs b/ethcore/sync/src/light_sync/response.rs index 4dfb383d4..74665118b 100644 --- a/ethcore/sync/src/light_sync/response.rs +++ b/ethcore/sync/src/light_sync/response.rs @@ -16,13 +16,11 @@ //! Helpers for decoding and verifying responses for headers. -use std::fmt; - -use ethcore::encoded; -use ethcore::header::Header; +use ethcore::{self, encoded, header::Header}; +use ethereum_types::H256; use light::request::{HashOrNumber, CompleteHeadersRequest as HeadersRequest}; use rlp::DecoderError; -use ethereum_types::H256; +use std::fmt; /// Errors found when decoding headers and verifying with basic constraints. #[derive(Debug, PartialEq)] @@ -74,19 +72,23 @@ pub trait Constraint { /// Do basic verification of provided headers against a request. pub fn verify(headers: &[encoded::Header], request: &HeadersRequest) -> Result, BasicError> { - let headers: Vec<_> = headers.iter().map(|h| h.decode()).collect(); + let headers: Result, _> = headers.iter().map(|h| h.decode() ).collect(); + match headers { + Ok(headers) => { + let reverse = request.reverse; - let reverse = request.reverse; + Max(request.max as usize).verify(&headers, reverse)?; + match request.start { + HashOrNumber::Number(ref num) => StartsAtNumber(*num).verify(&headers, reverse)?, + HashOrNumber::Hash(ref hash) => StartsAtHash(*hash).verify(&headers, reverse)?, + } - Max(request.max as usize).verify(&headers, reverse)?; - match request.start { - HashOrNumber::Number(ref num) => StartsAtNumber(*num).verify(&headers, reverse)?, - HashOrNumber::Hash(ref hash) => StartsAtHash(*hash).verify(&headers, reverse)?, + SkipsBetween(request.skip).verify(&headers, reverse)?; + + Ok(headers) + }, + Err(e) => Err(e.into()) } - - SkipsBetween(request.skip).verify(&headers, reverse)?; - - Ok(headers) } struct StartsAtNumber(u64); diff --git a/ethcore/sync/src/light_sync/tests/mod.rs b/ethcore/sync/src/light_sync/tests/mod.rs index 9fd270838..3fee1c717 100644 --- a/ethcore/sync/src/light_sync/tests/mod.rs +++ b/ethcore/sync/src/light_sync/tests/mod.rs @@ -45,7 +45,7 @@ fn fork_post_cht() { for id in (0..CHAIN_LENGTH).map(|x| x + 1).map(BlockId::Number) { let (light_peer, full_peer) = (net.peer(0), net.peer(1)); let light_chain = light_peer.light_chain(); - let header = full_peer.chain().block_header(id).unwrap().decode(); + let header = full_peer.chain().block_header(id).unwrap().decode().expect("decoding failure"); let _ = light_chain.import_header(header); light_chain.flush_queue(); light_chain.import_verified(); diff --git a/rpc/src/v1/helpers/errors.rs b/rpc/src/v1/helpers/errors.rs index 4f3289a11..c85beef7d 100644 --- a/rpc/src/v1/helpers/errors.rs +++ b/rpc/src/v1/helpers/errors.rs @@ -360,6 +360,19 @@ pub fn transaction>(error: T) -> Error { } } +pub fn decode>(error: T) -> Error { + let error = error.into(); + match *error.kind() { + ErrorKind::Decoder(ref dec_err) => rlp(dec_err.clone()), + _ => Error { + code: ErrorCode::InternalError, + message: "decoding error".into(), + data: None, + } + + } +} + pub fn rlp(error: DecoderError) -> Error { Error { code: ErrorCode::InvalidParams, diff --git a/rpc/src/v1/impls/eth.rs b/rpc/src/v1/impls/eth.rs index a7ff4916d..389805c17 100644 --- a/rpc/src/v1/impls/eth.rs +++ b/rpc/src/v1/impls/eth.rs @@ -343,7 +343,10 @@ impl EthClient hdr.decode(), + Some(hdr) => match hdr.decode() { + Ok(h) => h, + Err(e) => return Err(errors::decode(e)) + }, None => { return Ok(None); } }; @@ -851,9 +854,9 @@ impl Eth for EthClient< }; let state = try_bf!(self.client.state_at(id).ok_or(errors::state_pruned())); - let header = try_bf!(self.client.block_header(id).ok_or(errors::state_pruned())); + let header = try_bf!(self.client.block_header(id).ok_or(errors::state_pruned()).and_then(|h| h.decode().map_err(errors::decode))); - (state, header.decode()) + (state, header) }; let result = self.client.call(&signed, Default::default(), &mut state, &header); @@ -890,9 +893,9 @@ impl Eth for EthClient< }; let state = try_bf!(self.client.state_at(id).ok_or(errors::state_pruned())); - let header = try_bf!(self.client.block_header(id).ok_or(errors::state_pruned())); + let header = try_bf!(self.client.block_header(id).ok_or(errors::state_pruned()).and_then(|h| h.decode().map_err(errors::decode))); - (state, header.decode()) + (state, header) }; Box::new(future::done(self.client.estimate_gas(&signed, &state, &header) diff --git a/rpc/src/v1/impls/light/eth.rs b/rpc/src/v1/impls/light/eth.rs index eeef12da6..35f7792b5 100644 --- a/rpc/src/v1/impls/light/eth.rs +++ b/rpc/src/v1/impls/light/eth.rs @@ -371,7 +371,7 @@ impl Eth for EthClient { } fn send_raw_transaction(&self, raw: Bytes) -> Result { - let best_header = self.client.best_block_header().decode(); + let best_header = self.client.best_block_header().decode().map_err(errors::decode)?; Rlp::new(&raw.into_vec()).as_val() .map_err(errors::rlp) diff --git a/rpc/src/v1/impls/light/parity.rs b/rpc/src/v1/impls/light/parity.rs index 3d31d9e67..982c7ff36 100644 --- a/rpc/src/v1/impls/light/parity.rs +++ b/rpc/src/v1/impls/light/parity.rs @@ -395,7 +395,7 @@ impl Parity for ParityClient { let engine = self.light_dispatch.client.engine().clone(); let from_encoded = move |encoded: encoded::Header| { - let header = encoded.decode(); + let header = encoded.decode().expect("decoding error"); // REVIEW: not sure what to do here; what is a decent return value for the error case here? let extra_info = engine.extra_info(&header); RichHeader { inner: Header { diff --git a/rpc/src/v1/impls/parity.rs b/rpc/src/v1/impls/parity.rs index db66bddc7..08d514720 100644 --- a/rpc/src/v1/impls/parity.rs +++ b/rpc/src/v1/impls/parity.rs @@ -487,9 +487,9 @@ impl Parity for ParityClient where }; let state = self.client.state_at(id).ok_or(errors::state_pruned())?; - let header = self.client.block_header(id).ok_or(errors::state_pruned())?; + let header = self.client.block_header(id).ok_or(errors::state_pruned())?.decode().map_err(errors::decode)?; - (state, header.decode()) + (state, header) }; self.client.call_many(&requests, &mut state, &header) diff --git a/rpc/src/v1/impls/traces.rs b/rpc/src/v1/impls/traces.rs index bf4dc83be..0130b3b9c 100644 --- a/rpc/src/v1/impls/traces.rs +++ b/rpc/src/v1/impls/traces.rs @@ -104,7 +104,7 @@ impl Traces for TracesClient where let mut state = self.client.state_at(id).ok_or(errors::state_pruned())?; let header = self.client.block_header(id).ok_or(errors::state_pruned())?; - self.client.call(&signed, to_call_analytics(flags), &mut state, &header.decode()) + self.client.call(&signed, to_call_analytics(flags), &mut state, &header.decode().map_err(errors::decode)?) .map(TraceResults::from) .map_err(errors::call) } @@ -131,7 +131,7 @@ impl Traces for TracesClient where let mut state = self.client.state_at(id).ok_or(errors::state_pruned())?; let header = self.client.block_header(id).ok_or(errors::state_pruned())?; - self.client.call_many(&requests, &mut state, &header.decode()) + self.client.call_many(&requests, &mut state, &header.decode().map_err(errors::decode)?) .map(|results| results.into_iter().map(TraceResults::from).collect()) .map_err(errors::call) } @@ -153,7 +153,7 @@ impl Traces for TracesClient where let mut state = self.client.state_at(id).ok_or(errors::state_pruned())?; let header = self.client.block_header(id).ok_or(errors::state_pruned())?; - self.client.call(&signed, to_call_analytics(flags), &mut state, &header.decode()) + self.client.call(&signed, to_call_analytics(flags), &mut state, &header.decode().map_err(errors::decode)?) .map(TraceResults::from) .map_err(errors::call) }