client: fix decode block number as u32 (#3130)

* client: fix panic on decode block number as u32

* client: add test for number_index_key

* client: add another test at client level

* client: address review grumbles

substrate/core/client/db/src/lib.rs

@@ -883,7 +883,7 @@ impl<Block: BlockT<Hash=H256>> Backend<Block> {
 					transaction,
 					columns::KEY_LOOKUP,
 					r.number
-				);
+				)?;
 			}
 
 			// canonicalize: set the number lookup to map to this block's hash.
@@ -894,18 +894,18 @@ impl<Block: BlockT<Hash=H256>> Backend<Block> {
 					columns::KEY_LOOKUP,
 					e.number,
 					e.hash
-				);
+				)?;
 			}
 		}
 
-		let lookup_key = utils::number_and_hash_to_lookup_key(best_to.0, &best_to.1);
+		let lookup_key = utils::number_and_hash_to_lookup_key(best_to.0, &best_to.1)?;
 		transaction.put(columns::META, meta_keys::BEST_BLOCK, &lookup_key);
 		utils::insert_number_to_key_mapping(
 			transaction,
 			columns::KEY_LOOKUP,
 			best_to.0,
 			best_to.1,
-		);
+		)?;
 
 		Ok((enacted, retracted))
 	}
@@ -946,7 +946,7 @@ impl<Block: BlockT<Hash=H256>> Backend<Block> {
 		if let Some(justification) = justification {
 			transaction.put(
 				columns::JUSTIFICATION,
-				&utils::number_and_hash_to_lookup_key(number, hash),
+				&utils::number_and_hash_to_lookup_key(number, hash)?,
 				&justification.encode(),
 			);
 		}
@@ -1021,7 +1021,7 @@ impl<Block: BlockT<Hash=H256>> Backend<Block> {
 			let number = pending_block.header.number().clone();
 
 			// blocks are keyed by number + hash.
-			let lookup_key = utils::number_and_hash_to_lookup_key(number, hash);
+			let lookup_key = utils::number_and_hash_to_lookup_key(number, hash)?;
 
 			let (enacted, retracted) = if pending_block.leaf_state.is_best() {
 				self.set_head_with_transaction(&mut transaction, parent_hash, (number, hash))?
@@ -1034,7 +1034,7 @@ impl<Block: BlockT<Hash=H256>> Backend<Block> {
 				columns::KEY_LOOKUP,
 				number,
 				hash,
-			);
+			)?;
 
 			transaction.put(columns::HEADER, &lookup_key, &pending_block.header.encode());
 			if let Some(body) = pending_block.body {
@@ -1177,7 +1177,7 @@ impl<Block: BlockT<Hash=H256>> Backend<Block> {
 		if self.storage.state_db.best_canonical().map(|c| f_num.saturated_into::<u64>() > c).unwrap_or(true) {
 			let parent_hash = f_header.parent_hash().clone();
 
-			let lookup_key = utils::number_and_hash_to_lookup_key(f_num, f_hash.clone());
+			let lookup_key = utils::number_and_hash_to_lookup_key(f_num, f_hash.clone())?;
 			transaction.put(columns::META, meta_keys::FINALIZED_BLOCK, &lookup_key);
 
 			let commit = self.storage.state_db.canonicalize_block(&f_hash)
@@ -1344,7 +1344,7 @@ impl<Block> client::backend::Backend<Block, Blake2Hasher> for Backend<Block> whe
 					let hash = self.blockchain.hash(best)?.ok_or_else(
 						|| client::error::Error::UnknownBlock(
 							format!("Error reverting to {}. Block hash not found.", best)))?;
-					let key = utils::number_and_hash_to_lookup_key(best.clone(), &hash);
+					let key = utils::number_and_hash_to_lookup_key(best.clone(), &hash)?;
 					transaction.put(columns::META, meta_keys::BEST_BLOCK, &key);
 					transaction.delete(columns::KEY_LOOKUP, removed.hash().as_ref());
 					children::remove_children(&mut transaction, columns::META, meta_keys::CHILDREN_PREFIX, hash);

substrate/core/client/db/src/light.rs

@@ -211,7 +211,7 @@ impl<Block: BlockT> LightStorage<Block> {
 	/// should be the parent of `best_to`. In the case where we set an existing block
 	/// to be best, `route_to` should equal to `best_to`.
 	fn set_head_with_transaction(&self, transaction: &mut DBTransaction, route_to: Block::Hash, best_to: (NumberFor<Block>, Block::Hash)) -> Result<(), client::error::Error> {
-		let lookup_key = utils::number_and_hash_to_lookup_key(best_to.0, &best_to.1);
+		let lookup_key = utils::number_and_hash_to_lookup_key(best_to.0, &best_to.1)?;
 
 		// handle reorg.
 		let meta = self.meta.read();
@@ -234,7 +234,7 @@ impl<Block: BlockT> LightStorage<Block> {
 					transaction,
 					columns::KEY_LOOKUP,
 					retracted.number
-				);
+				)?;
 			}
 
 			for enacted in tree_route.enacted() {
@@ -243,7 +243,7 @@ impl<Block: BlockT> LightStorage<Block> {
 					columns::KEY_LOOKUP,
 					enacted.number,
 					enacted.hash
-				);
+				)?;
 			}
 		}
 
@@ -253,7 +253,7 @@ impl<Block: BlockT> LightStorage<Block> {
 			columns::KEY_LOOKUP,
 			best_to.0,
 			best_to.1,
-		);
+		)?;
 
 		Ok(())
 	}
@@ -274,7 +274,7 @@ impl<Block: BlockT> LightStorage<Block> {
 			).into())
 		}
 
-		let lookup_key = utils::number_and_hash_to_lookup_key(header.number().clone(), hash);
+		let lookup_key = utils::number_and_hash_to_lookup_key(header.number().clone(), hash)?;
 		transaction.put(columns::META, meta_keys::FINALIZED_BLOCK, &lookup_key);
 
 		// build new CHT(s) if required
@@ -293,7 +293,7 @@ impl<Block: BlockT> LightStorage<Block> {
 			)?;
 			transaction.put(
 				columns::CHT,
-				&cht_key(HEADER_CHT_PREFIX, new_cht_start),
+				&cht_key(HEADER_CHT_PREFIX, new_cht_start)?,
 				new_header_cht_root.as_ref()
 			);
 
@@ -311,7 +311,7 @@ impl<Block: BlockT> LightStorage<Block> {
 				)?;
 				transaction.put(
 					columns::CHT,
-					&cht_key(CHANGES_TRIE_CHT_PREFIX, new_cht_start),
+					&cht_key(CHANGES_TRIE_CHT_PREFIX, new_cht_start)?,
 					new_changes_trie_cht_root.as_ref()
 				);
 			}
@@ -331,7 +331,7 @@ impl<Block: BlockT> LightStorage<Block> {
 						columns::KEY_LOOKUP,
 						prune_block,
 						hash
-					);
+					)?;
 					transaction.delete(columns::HEADER, &lookup_key);
 				}
 				prune_block += One::one();
@@ -358,7 +358,7 @@ impl<Block: BlockT> LightStorage<Block> {
 
 		let cht_number = cht::block_to_cht_number(cht_size, block).ok_or_else(no_cht_for_block)?;
 		let cht_start = cht::start_number(cht_size, cht_number);
-		self.db.get(columns::CHT, &cht_key(cht_type, cht_start)).map_err(db_err)?
+		self.db.get(columns::CHT, &cht_key(cht_type, cht_start)?).map_err(db_err)?
 			.ok_or_else(no_cht_for_block)
 			.and_then(|hash| Block::Hash::decode(&mut &*hash).ok_or_else(no_cht_for_block))
 	}
@@ -414,7 +414,7 @@ impl<Block> LightBlockchainStorage<Block> for LightStorage<Block>
 		}
 
 		// blocks are keyed by number + hash.
-		let lookup_key = utils::number_and_hash_to_lookup_key(number, &hash);
+		let lookup_key = utils::number_and_hash_to_lookup_key(number, &hash)?;
 
 		if leaf_state.is_best() {
 			self.set_head_with_transaction(&mut transaction, parent_hash, (number, hash))?;
@@ -425,7 +425,7 @@ impl<Block> LightBlockchainStorage<Block> for LightStorage<Block>
 			columns::KEY_LOOKUP,
 			number,
 			hash,
-		);
+		)?;
 		transaction.put(columns::HEADER, &lookup_key, &header.encode());
 
 		let is_genesis = number.is_zero();
@@ -562,10 +562,10 @@ impl<Block> LightBlockchainStorage<Block> for LightStorage<Block>
 }
 
 /// Build the key for inserting header-CHT at given block.
-fn cht_key<N: TryInto<u32>>(cht_type: u8, block: N) -> [u8; 5] {
+fn cht_key<N: TryInto<u32>>(cht_type: u8, block: N) -> ClientResult<[u8; 5]> {
 	let mut key = [cht_type; 5];
-	key[1..].copy_from_slice(&utils::number_index_key(block));
-	key
+	key[1..].copy_from_slice(&utils::number_index_key(block)?);
+	Ok(key)
 }
 
 #[cfg(test)]

substrate/core/client/db/src/utils.rs

@@ -31,8 +31,8 @@ use parity_codec::Decode;
 use trie::DBValue;
 use runtime_primitives::generic::BlockId;
 use runtime_primitives::traits::{
-	Block as BlockT, Header as HeaderT, Zero, UniqueSaturatedFrom,
-	UniqueSaturatedInto, CheckedConversion
+	Block as BlockT, Header as HeaderT, Zero,
+	UniqueSaturatedFrom, UniqueSaturatedInto,
 };
 #[cfg(feature = "kvdb-rocksdb")]
 use crate::DatabaseSettings;
@@ -84,25 +84,31 @@ pub type NumberIndexKey = [u8; 4];
 ///
 /// In the current database schema, this kind of key is only used for
 /// lookups into an index, NOT for storing header data or others.
-pub fn number_index_key<N: TryInto<u32>>(n: N) -> NumberIndexKey {
-	let n = n.checked_into::<u32>().unwrap();
-	[
+pub fn number_index_key<N: TryInto<u32>>(n: N) -> client::error::Result<NumberIndexKey> {
+	let n = n.try_into().map_err(|_|
+		client::error::Error::Backend("Block number cannot be converted to u32".into())
+	)?;
+
+	Ok([
 		(n >> 24) as u8,
 		((n >> 16) & 0xff) as u8,
 		((n >> 8) & 0xff) as u8,
 		(n & 0xff) as u8
-	]
+	])
 }
 
 /// Convert number and hash into long lookup key for blocks that are
 /// not in the canonical chain.
-pub fn number_and_hash_to_lookup_key<N, H>(number: N, hash: H) -> Vec<u8> where
+pub fn number_and_hash_to_lookup_key<N, H>(
+	number: N,
+	hash: H,
+) -> client::error::Result<Vec<u8>>	where
 	N: TryInto<u32>,
-	H: AsRef<[u8]>
+	H: AsRef<[u8]>,
 {
-	let mut lookup_key = number_index_key(number).to_vec();
+	let mut lookup_key = number_index_key(number)?.to_vec();
 	lookup_key.extend_from_slice(hash.as_ref());
-	lookup_key
+	Ok(lookup_key)
 }
 
 /// Convert block lookup key into block number.
@@ -124,8 +130,9 @@ pub fn remove_number_to_key_mapping<N: TryInto<u32>>(
 	transaction: &mut DBTransaction,
 	key_lookup_col: Option<u32>,
 	number: N,
-) {
-	transaction.delete(key_lookup_col, number_index_key(number).as_ref())
+) -> client::error::Result<()> {
+	transaction.delete(key_lookup_col, number_index_key(number)?.as_ref());
+	Ok(())
 }
 
 /// Remove key mappings.
@@ -134,9 +141,10 @@ pub fn remove_key_mappings<N: TryInto<u32>, H: AsRef<[u8]>>(
 	key_lookup_col: Option<u32>,
 	number: N,
 	hash: H,
-) {
-	remove_number_to_key_mapping(transaction, key_lookup_col, number);
+) -> client::error::Result<()> {
+	remove_number_to_key_mapping(transaction, key_lookup_col, number)?;
 	transaction.delete(key_lookup_col, hash.as_ref());
+	Ok(())
 }
 
 /// Place a number mapping into the database. This maps number to current perceived
@@ -146,12 +154,13 @@ pub fn insert_number_to_key_mapping<N: TryInto<u32> + Clone, H: AsRef<[u8]>>(
 	key_lookup_col: Option<u32>,
 	number: N,
 	hash: H,
-) {
+) -> client::error::Result<()> {
 	transaction.put_vec(
 		key_lookup_col,
-		number_index_key(number.clone()).as_ref(),
-		number_and_hash_to_lookup_key(number, hash),
-	)
+		number_index_key(number.clone())?.as_ref(),
+		number_and_hash_to_lookup_key(number, hash)?,
+	);
+	Ok(())
 }
 
 /// Insert a hash to key mapping in the database.
@@ -160,12 +169,13 @@ pub fn insert_hash_to_key_mapping<N: TryInto<u32>, H: AsRef<[u8]> + Clone>(
 	key_lookup_col: Option<u32>,
 	number: N,
 	hash: H,
-) {
+) -> client::error::Result<()> {
 	transaction.put_vec(
 		key_lookup_col,
 		hash.clone().as_ref(),
-		number_and_hash_to_lookup_key(number, hash),
-	)
+		number_and_hash_to_lookup_key(number, hash)?,
+	);
+	Ok(())
 }
 
 /// Convert block id to block lookup key.
@@ -182,7 +192,7 @@ pub fn block_id_to_lookup_key<Block>(
 	let res = match id {
 		BlockId::Number(n) => db.get(
 			key_lookup_col,
-			number_index_key(n).as_ref(),
+			number_index_key(n)?.as_ref(),
 		),
 		BlockId::Hash(h) => db.get(key_lookup_col, h.as_ref()),
 	};
@@ -318,3 +328,19 @@ pub fn read_meta<Block>(db: &dyn KeyValueDB, col_meta: Option<u32>, col_header:
 		genesis_hash,
 	})
 }
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+	use runtime_primitives::testing::{Block as RawBlock, ExtrinsicWrapper};
+	type Block = RawBlock<ExtrinsicWrapper<u32>>;
+
+	#[test]
+	fn number_index_key_doesnt_panic() {
+		let id = BlockId::<Block>::Number(72340207214430721);
+		match id {
+			BlockId::Number(n) => number_index_key(n).expect_err("number should overflow u32"),
+			_ => unreachable!(),
+		};
+	}
+}

substrate/core/client/src/client.rs

@@ -2633,4 +2633,14 @@ pub(crate) mod tests {
 			b3.hash(),
 		);
 	}
+
+	#[test]
+	fn get_header_by_block_number_doesnt_panic() {
+		let client = test_client::new();
+
+		// backend uses u32 for block numbers, make sure we don't panic when
+		// trying to convert
+		let id = BlockId::<Block>::Number(72340207214430721);
+		client.header(&id).expect_err("invalid block number overflows u32");
+	}
 }