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

@@ -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);
 	}
 }

ethcore/light/src/client/mod.rs

@@ -318,7 +318,7 @@ impl<T: ChainDataFetcher> Client<T> {
 
 			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<T: ChainDataFetcher> Client<T> {
 		};
 
 		// 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);

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).and_then(|h| h.decode().ok())
-				=> self.block_header(id).map(|h| h.decode()),
 		}
 	}
 }
@@ -1915,7 +1914,11 @@ impl BlockChainClient for Client {
 
 	fn uncle_extra_info(&self, id: UncleId) -> Option<BTreeMap<String, String>> {
 		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;

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<encoded::Block> {

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<u8>) -> 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<FullHeader, rlp::DecoderError> {
+		rlp::decode(&self.0)
+	}
 
 	/// Get a borrowed header view onto the data.
 	#[inline]

ethcore/src/engines/authority_round/mod.rs

@@ -996,7 +996,7 @@ impl Engine<EthereumMachine> 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();

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<AccountsError> 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()
 	}
 }
 

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<C>(&self, chain: &C, block: ClosedBlock) -> bool where
+	fn seal_and_import_block_internally<C>(&self, chain: &C, block: ClosedBlock) -> bool
-		C: BlockChain + SealedBlockImporter,
+		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,
 		};
 

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::<f32>() <= 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 {

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,

ethcore/sync/src/light_sync/response.rs

@@ -16,13 +16,11 @@
 
 //! Helpers for decoding and verifying responses for headers.
 
-use std::fmt;
+use ethcore::{self, encoded, header::Header};
-
+use ethereum_types::H256;
-use ethcore::encoded;
-use ethcore::header::Header;
 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<Vec<Header>, BasicError> {
-	let headers: Vec<_> = headers.iter().map(|h| h.decode()).collect();
+	let headers: Result<Vec<_>, _> = 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)?;
+			SkipsBetween(request.skip).verify(&headers, reverse)?;
-	match request.start {
+
-		HashOrNumber::Number(ref num) => StartsAtNumber(*num).verify(&headers, reverse)?,
+			Ok(headers)
-		HashOrNumber::Hash(ref hash) => StartsAtHash(*hash).verify(&headers, reverse)?,
+		},
+		Err(e) => Err(e.into())
 	}
-
-	SkipsBetween(request.skip).verify(&headers, reverse)?;
-
-	Ok(headers)
 }
 
 struct StartsAtNumber(u64);

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();

rpc/src/v1/helpers/errors.rs

@@ -360,6 +360,19 @@ pub fn transaction<T: Into<EthcoreError>>(error: T) -> Error {
 	}
 }
 
+pub fn decode<T: Into<EthcoreError>>(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,

rpc/src/v1/impls/eth.rs

@@ -343,7 +343,10 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> EthClient<C, SN, S
 				let uncle_id = UncleId { block: block_id, position };
 
 				let uncle = match client.uncle(uncle_id) {
-					Some(hdr) => 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<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> 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<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> 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)

rpc/src/v1/impls/light/eth.rs

@@ -371,7 +371,7 @@ impl<T: LightChainClient + 'static> Eth for EthClient<T> {
 	}
 
 	fn send_raw_transaction(&self, raw: Bytes) -> Result<RpcH256> {
-		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)

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 {

rpc/src/v1/impls/parity.rs

@@ -487,9 +487,9 @@ impl<C, M, U, S> Parity for ParityClient<C, M, U> 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)

rpc/src/v1/impls/traces.rs

@@ -104,7 +104,7 @@ impl<C, S> Traces for TracesClient<C> 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<C, S> Traces for TracesClient<C> 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<C, S> Traces for TracesClient<C> 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)
 	}