Index blocks by number in the DB (#890)

* resolve #780

* in import tx insert after retracting so blocks don't disappear

* adapt db/cache to changes to db/utils

* db/light: remove old hash lookup insertion

* renames

* improve naming

* improve naming

* db/utils: improve docs for block lookup key stuff

* db/light/note_finalized: adapt to changes for issue #780

* db/light/cht/tests: add assertions for HASH_LOOKUP

* simplify

addresses
https://github.com/paritytech/substrate/pull/890#discussion_r226009746

* break long line

addresses
https://github.com/paritytech/substrate/pull/890#discussion_r226002211

* Whitespace

substrate/core/client/db/src/cache/list_storage.rs

@@ -23,7 +23,7 @@ use kvdb::{KeyValueDB, DBTransaction};
 use client::error::{Error as ClientError, ErrorKind as ClientErrorKind, Result as ClientResult};
 use codec::{Encode, Decode};
 use runtime_primitives::generic::BlockId;
-use runtime_primitives::traits::{Block as BlockT, NumberFor};
+use runtime_primitives::traits::{Block as BlockT, Header as HeaderT, NumberFor};
 use utils::{self, db_err, meta_keys};
 
 use cache::{CacheItemT, ComplexBlockId};
@@ -126,7 +126,8 @@ impl DbStorage {
 
 impl<Block: BlockT, T: CacheItemT> Storage<Block, T> for DbStorage {
 	fn read_id(&self, at: NumberFor<Block>) -> ClientResult<Option<Block::Hash>> {
-		utils::read_id::<Block>(&*self.db, self.columns.hash_lookup, BlockId::Number(at))
+		utils::read_header::<Block>(&*self.db, self.columns.hash_lookup, self.columns.header, BlockId::Number(at))
+			.map(|maybe_header| maybe_header.map(|header| header.hash()))
 	}
 
 	fn read_header(&self, at: &Block::Hash) -> ClientResult<Option<Block::Header>> {
@@ -246,7 +247,6 @@ mod meta {
 #[cfg(test)]
 pub mod tests {
 	use std::collections::{HashMap, HashSet};
-	use runtime_primitives::traits::Header as HeaderT;
 	use super::*;
 
 	pub struct FaultyStorage;

substrate/core/client/db/src/cache/mod.rs

@@ -194,10 +194,11 @@ impl<Block: BlockT> BlockchainCache<Block> for DbCacheSync<Block> {
 				ComplexBlockId::new(hash, *header.number())
 			},
 			BlockId::Number(number) => {
-				let hash = utils::read_id::<Block>(
+				let hash = utils::read_header::<Block>(
 					&**db,
 					columns.hash_lookup,
-					BlockId::Number(number.clone())).ok()??;
+					columns.header,
+					BlockId::Number(number.clone())).ok()??.hash();
 				ComplexBlockId::new(hash, number)
 			},
 		};

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

@@ -75,7 +75,7 @@ use runtime_primitives::BuildStorage;
 use state_machine::backend::Backend as StateBackend;
 use executor::RuntimeInfo;
 use state_machine::{CodeExecutor, DBValue, ExecutionStrategy};
-use utils::{Meta, db_err, meta_keys, open_database, read_db, read_id, read_meta};
+use utils::{Meta, db_err, meta_keys, open_database, read_db, block_id_to_lookup_key, read_meta};
 use client::LeafSet;
 use state_db::StateDb;
 pub use state_db::PruningMode;
@@ -118,6 +118,7 @@ mod columns {
 	pub const META: Option<u32> = ::utils::COLUMN_META;
 	pub const STATE: Option<u32> = Some(1);
 	pub const STATE_META: Option<u32> = Some(2);
+	/// maps hashes to lookup keys
 	pub const HASH_LOOKUP: Option<u32> = Some(3);
 	pub const HEADER: Option<u32> = Some(4);
 	pub const BODY: Option<u32> = Some(5);
@@ -219,15 +220,20 @@ impl<Block: BlockT> client::blockchain::HeaderBackend<Block> for BlockchainDb<Bl
 		}
 	}
 
-	fn number(&self, hash: Block::Hash) -> Result<Option<<Block::Header as HeaderT>::Number>, client::error::Error> {
-		self.header(BlockId::Hash(hash)).and_then(|key| match key {
-			Some(hdr) => Ok(Some(hdr.number().clone())),
-			None => Ok(None),
-		})
+	fn number(&self, hash: Block::Hash) -> Result<Option<NumberFor<Block>>, client::error::Error> {
+		if let Some(lookup_key) = block_id_to_lookup_key::<Block>(&*self.db, columns::HASH_LOOKUP, BlockId::Hash(hash))? {
+			let number = utils::lookup_key_to_number(&lookup_key)?;
+			Ok(Some(number))
+		} else {
+			Ok(None)
+		}
 	}
 
-	fn hash(&self, number: <Block::Header as HeaderT>::Number) -> Result<Option<Block::Hash>, client::error::Error> {
-		read_id::<Block>(&*self.db, columns::HASH_LOOKUP, BlockId::Number(number))
+	fn hash(&self, number: NumberFor<Block>) -> Result<Option<Block::Hash>, client::error::Error> {
+		self.header(BlockId::Number(number)).and_then(|maybe_header| match maybe_header {
+			Some(header) => Ok(Some(header.hash().clone())),
+			None => Ok(None),
+		})
 	}
 }
 
@@ -495,12 +501,9 @@ impl<Block: BlockT> Backend<Block> {
 			let hash = if new_canonical == number_u64 {
 				hash
 			} else {
-				read_id::<Block>(
-					&*self.blockchain.db,
-					columns::HASH_LOOKUP,
-					BlockId::Number(As::sa(new_canonical))
-				)?.expect("existence of block with number `new_canonical` \
-					implies existence of blocks with all nubmers before it; qed")
+				::client::blockchain::HeaderBackend::hash(&self.blockchain, As::sa(new_canonical))?
+					.expect("existence of block with number `new_canonical` \
+						implies existence of blocks with all numbers before it; qed")
 			};
 
 			trace!(target: "db", "Canonicalize block #{} ({:?})", new_canonical, hash);
@@ -588,14 +591,6 @@ impl<Block> client::backend::Backend<Block, Blake2Hasher> for Backend<Block> whe
 			let parent_hash = *pending_block.header.parent_hash();
 			let number = pending_block.header.number().clone();
 
-			transaction.put(columns::HEADER, hash.as_ref(), &pending_block.header.encode());
-			if let Some(body) = pending_block.body {
-				transaction.put(columns::BODY, hash.as_ref(), &body.encode());
-			}
-			if let Some(justification) = pending_block.justification {
-				transaction.put(columns::JUSTIFICATION, hash.as_ref(), &justification.encode());
-			}
-
 			if pending_block.leaf_state.is_best() {
 				let meta = self.blockchain.meta.read();
 
@@ -607,7 +602,7 @@ impl<Block> client::backend::Backend<Block, Blake2Hasher> for Backend<Block> whe
 						BlockId::Hash(parent_hash),
 					)?;
 
-					// update block number to hash lookup entries.
+					// uncanonicalize
 					for retracted in tree_route.retracted() {
 						if retracted.hash == meta.finalized_hash {
 							warn!("Potential safety failure: reverting finalized block {:?}",
@@ -616,30 +611,94 @@ impl<Block> client::backend::Backend<Block, Blake2Hasher> for Backend<Block> whe
 							return Err(::client::error::ErrorKind::NotInFinalizedChain.into());
 						}
 
-						transaction.delete(
-							columns::HASH_LOOKUP,
-							&::utils::number_to_lookup_key(retracted.number)
-						);
+						let prev_lookup_key = ::utils::number_to_lookup_key(retracted.number);
+						let new_lookup_key = ::utils::number_and_hash_to_lookup_key(retracted.number, retracted.hash);
+
+						// change mapping from `number -> header`
+						// to `number + hash -> header`
+						let retracted_header = if let Some(header) = ::client::blockchain::HeaderBackend::<Block>::header(&self.blockchain, BlockId::Number(retracted.number))? {
+							header
+						} else {
+							return Err(client::error::ErrorKind::UnknownBlock(format!("retracted {:?}", retracted)).into());
+						};
+						transaction.delete(columns::HEADER, &prev_lookup_key);
+						transaction.put(columns::HEADER, &new_lookup_key, &retracted_header.encode());
+
+						// if body is stored
+						// change mapping from `number -> body`
+						// to `number + hash -> body`
+						if let Some(retracted_body) = ::client::blockchain::Backend::<Block>::body(&self.blockchain, BlockId::Number(retracted.number))? {
+							transaction.delete(columns::BODY, &prev_lookup_key);
+							transaction.put(columns::BODY, &new_lookup_key, &retracted_body.encode());
+						}
+
+						// if justification is stored
+						// change mapping from `number -> justification`
+						// to `number + hash -> justification`
+						if let Some(retracted_justification) = ::client::blockchain::Backend::<Block>::justification(&self.blockchain, BlockId::Number(retracted.number))? {
+							transaction.delete(columns::JUSTIFICATION, &prev_lookup_key);
+							transaction.put(columns::JUSTIFICATION, &new_lookup_key, &retracted_justification.encode());
+						}
+
+						transaction.put(columns::HASH_LOOKUP, retracted.hash.as_ref(), &new_lookup_key);
 					}
 
+					// canonicalize
 					for enacted in tree_route.enacted() {
-						let hash: &Block::Hash = &enacted.hash;
-						transaction.put(
-							columns::HASH_LOOKUP,
-							&::utils::number_to_lookup_key(enacted.number),
-							hash.as_ref(),
-						)
+						let prev_lookup_key = ::utils::number_and_hash_to_lookup_key(enacted.number, enacted.hash);
+						let new_lookup_key = ::utils::number_to_lookup_key(enacted.number);
+
+						// change mapping from `number + hash -> header`
+						// to `number -> header`
+						let enacted_header = if let Some(header) = ::client::blockchain::HeaderBackend::<Block>::header(&self.blockchain, BlockId::Number(enacted.number))? {
+							header
+						} else {
+							return Err(client::error::ErrorKind::UnknownBlock(format!("enacted {:?}", enacted)).into());
+						};
+						transaction.delete(columns::HEADER, &prev_lookup_key);
+						transaction.put(columns::HEADER, &new_lookup_key, &enacted_header.encode());
+
+						// if body is stored
+						// change mapping from `number + hash -> body`
+						// to `number -> body`
+						if let Some(enacted_body) = ::client::blockchain::Backend::<Block>::body(&self.blockchain, BlockId::Number(enacted.number))? {
+							transaction.delete(columns::BODY, &prev_lookup_key);
+							transaction.put(columns::BODY, &new_lookup_key, &enacted_body.encode());
+						}
+
+						// if justification is stored
+						// change mapping from `number -> justification`
+						// to `number + hash -> justification`
+						if let Some(enacted_justification) = ::client::blockchain::Backend::<Block>::justification(&self.blockchain, BlockId::Number(enacted.number))? {
+							transaction.delete(columns::JUSTIFICATION, &prev_lookup_key);
+							transaction.put(columns::JUSTIFICATION, &new_lookup_key, &enacted_justification.encode());
+						}
+
+						transaction.put(columns::HASH_LOOKUP, enacted.hash.as_ref(), &new_lookup_key);
 					}
 				}
 
-				transaction.put(
-					columns::HASH_LOOKUP,
-					&::utils::number_to_lookup_key(number),
-					hash.as_ref()
-				);
 				transaction.put(columns::META, meta_keys::BEST_BLOCK, hash.as_ref());
 			}
 
+			// blocks in longest chain are keyed by number
+			let lookup_key = if pending_block.leaf_state.is_best() {
+				::utils::number_to_lookup_key(number).to_vec()
+			} else {
+			// other blocks are keyed by number + hash
+				::utils::number_and_hash_to_lookup_key(number, hash)
+			};
+
+			transaction.put(columns::HEADER, &lookup_key, &pending_block.header.encode());
+			if let Some(body) = pending_block.body {
+				transaction.put(columns::BODY, &lookup_key, &body.encode());
+			}
+			if let Some(justification) = pending_block.justification {
+				transaction.put(columns::JUSTIFICATION, &lookup_key, &justification.encode());
+			}
+
+			transaction.put(columns::HASH_LOOKUP, hash.as_ref(), &lookup_key);
+
 			if number == Zero::zero() {
 				transaction.put(columns::META, meta_keys::FINALIZED_BLOCK, hash.as_ref());
 				transaction.put(columns::META, meta_keys::GENESIS_HASH, hash.as_ref());
@@ -739,7 +798,7 @@ impl<Block> client::backend::Backend<Block, Blake2Hasher> for Backend<Block> whe
 							format!("Error reverting to {}. Block header not found.", best)))?;
 
 					transaction.put(columns::META, meta_keys::BEST_BLOCK, header.hash().as_ref());
-					transaction.delete(columns::HASH_LOOKUP, &::utils::number_to_lookup_key(removed));
+					transaction.delete(columns::HASH_LOOKUP, header.hash().as_ref());
 					self.storage.db.write(transaction).map_err(db_err)?;
 					self.blockchain.update_meta(header.hash().clone(), best.clone(), true, false);
 					self.blockchain.leaves.write().revert(header.hash().clone(), header.number().clone(), header.parent_hash().clone());

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

@@ -34,7 +34,7 @@ use runtime_primitives::traits::{Block as BlockT, Header as HeaderT,
 	Zero, One, As, NumberFor};
 use cache::{DbCacheSync, DbCache, ComplexBlockId};
 use utils::{meta_keys, Meta, db_err, number_to_lookup_key, open_database,
-	read_db, read_id, read_meta};
+	read_db, block_id_to_lookup_key, read_meta};
 use DatabaseSettings;
 
 pub(crate) mod columns {
@@ -168,14 +168,16 @@ impl<Block> BlockchainHeaderBackend<Block> for LightStorage<Block>
 	}
 
 	fn number(&self, hash: Block::Hash) -> ClientResult<Option<<<Block as BlockT>::Header as HeaderT>::Number>> {
-		self.header(BlockId::Hash(hash)).and_then(|key| match key {
-			Some(hdr) => Ok(Some(hdr.number().clone())),
-			None => Ok(None),
-		})
+		if let Some(lookup_key) = block_id_to_lookup_key::<Block>(&*self.db, columns::HASH_LOOKUP, BlockId::Hash(hash))? {
+			let number = ::utils::lookup_key_to_number(&lookup_key)?;
+			Ok(Some(number))
+		} else {
+			Ok(None)
+		}
 	}
 
 	fn hash(&self, number: <<Block as BlockT>::Header as HeaderT>::Number) -> ClientResult<Option<Block::Hash>> {
-		read_id::<Block>(&*self.db, columns::HASH_LOOKUP, BlockId::Number(number))
+		Ok(self.header(BlockId::Number(number))?.map(|header| header.hash().clone()))
 	}
 }
 
@@ -212,13 +214,13 @@ impl<Block: BlockT> LightStorage<Block> {
 				trace!(target: "db", "Replacing blocks [{}..{}] with CHT#{}", new_cht_start, new_cht_end, new_cht_number);
 
 				while prune_block <= new_cht_end {
-					let id = read_id::<Block>(&*self.db, columns::HASH_LOOKUP, BlockId::Number(prune_block))?;
-					if let Some(hash) = id {
-						let lookup_key = number_to_lookup_key(prune_block);
-						transaction.delete(columns::HASH_LOOKUP, &lookup_key);
-						transaction.delete(columns::HEADER, hash.as_ref());
+					if let Some(hash) = self.hash(prune_block)? {
+						let lookup_key = block_id_to_lookup_key::<Block>(&*self.db, columns::HASH_LOOKUP, BlockId::Number(prune_block))?
+							.expect("retrieved hash for `prune_block` right above. therefore retrieving lookup key must succeed. q.e.d.");
+						transaction.delete(columns::HASH_LOOKUP, hash.as_ref());
+						transaction.delete(columns::HEADER, &lookup_key);
 					}
-					prune_block += <<Block as BlockT>::Header as HeaderT>::Number::one();
+					prune_block += NumberFor::<Block>::one();
 				}
 			}
 		}
@@ -242,8 +244,6 @@ impl<Block> LightBlockchainStorage<Block> for LightStorage<Block>
 		let number = *header.number();
 		let parent_hash = *header.parent_hash();
 
-		transaction.put(columns::HEADER, hash.as_ref(), &header.encode());
-
 		if leaf_state.is_best() {
 			// handle reorg.
 			{
@@ -263,27 +263,55 @@ impl<Block> LightBlockchainStorage<Block> for LightStorage<Block>
 								(&retracted.number, &retracted.hash));
 						}
 
-						transaction.delete(
-							columns::HASH_LOOKUP,
-							&::utils::number_to_lookup_key(retracted.number)
-						);
+						let prev_lookup_key = ::utils::number_to_lookup_key(retracted.number);
+						let new_lookup_key = ::utils::number_and_hash_to_lookup_key(retracted.number, retracted.hash);
+
+						// change mapping from `number -> header`
+						// to `number + hash -> header`
+						let retracted_header = if let Some(header) = self.header(BlockId::Number(retracted.number))? {
+							header
+						} else {
+							return Err(::client::error::ErrorKind::UnknownBlock(format!("retracted {:?}", retracted)).into());
+						};
+						transaction.delete(columns::HEADER, &prev_lookup_key);
+						transaction.put(columns::HEADER, &new_lookup_key, &retracted_header.encode());
+
+						transaction.put(columns::HASH_LOOKUP, retracted.hash.as_ref(), &new_lookup_key);
 					}
 
 					for enacted in tree_route.enacted() {
-						let hash: &Block::Hash = &enacted.hash;
-						transaction.put(
-							columns::HASH_LOOKUP,
-							&::utils::number_to_lookup_key(enacted.number),
-							hash.as_ref(),
-						)
+						let prev_lookup_key = ::utils::number_and_hash_to_lookup_key(enacted.number, enacted.hash);
+						let new_lookup_key = ::utils::number_to_lookup_key(enacted.number);
+
+						// change mapping from `number + hash -> header`
+						// to `number -> header`
+						let enacted_header = if let Some(header) = self.header(BlockId::Number(enacted.number))? {
+							header
+						} else {
+							return Err(::client::error::ErrorKind::UnknownBlock(format!("enacted {:?}", enacted)).into());
+						};
+						transaction.delete(columns::HEADER, &prev_lookup_key);
+						transaction.put(columns::HEADER, &new_lookup_key, &enacted_header.encode());
+
+						transaction.put(columns::HASH_LOOKUP, enacted.hash.as_ref(), &new_lookup_key);
 					}
 				}
 			}
 
 			transaction.put(columns::META, meta_keys::BEST_BLOCK, hash.as_ref());
-			transaction.put(columns::HASH_LOOKUP, &number_to_lookup_key(number), hash.as_ref());
 		}
 
+		// blocks in longest chain are keyed by number
+		let lookup_key = if leaf_state.is_best() {
+			::utils::number_to_lookup_key(number).to_vec()
+		} else {
+		// other blocks are keyed by number + hash
+			::utils::number_and_hash_to_lookup_key(number, hash)
+		};
+
+		transaction.put(columns::HEADER, &lookup_key, &header.encode());
+		transaction.put(columns::HASH_LOOKUP, hash.as_ref(), &lookup_key);
+
 		let finalized = match leaf_state {
 			NewBlockState::Final => true,
 			_ => false,
@@ -512,6 +540,7 @@ pub(crate) mod tests {
 			prev_hash = insert_block(&db, &prev_hash, 1 + number, None);
 		}
 		assert_eq!(db.db.iter(columns::HEADER).count(), (1 + cht::SIZE) as usize);
+		assert_eq!(db.db.iter(columns::HASH_LOOKUP).count(), (1 + cht::SIZE) as usize);
 		assert_eq!(db.db.iter(columns::CHT).count(), 0);
 
 		// insert next SIZE blocks && ensure that nothing is pruned
@@ -519,12 +548,14 @@ pub(crate) mod tests {
 			prev_hash = insert_block(&db, &prev_hash, 1 + cht::SIZE + number, None);
 		}
 		assert_eq!(db.db.iter(columns::HEADER).count(), (1 + cht::SIZE + cht::SIZE) as usize);
+		assert_eq!(db.db.iter(columns::HASH_LOOKUP).count(), (1 + cht::SIZE + cht::SIZE) as usize);
 		assert_eq!(db.db.iter(columns::CHT).count(), 0);
 
 		// insert block #{2 * cht::SIZE + 1} && check that new CHT is created + headers of this CHT are pruned
 		// nothing is yet finalized, so nothing is pruned.
 		prev_hash = insert_block(&db, &prev_hash, 1 + cht::SIZE + cht::SIZE, None);
 		assert_eq!(db.db.iter(columns::HEADER).count(), (2 + cht::SIZE + cht::SIZE) as usize);
+		assert_eq!(db.db.iter(columns::HASH_LOOKUP).count(), (2 + cht::SIZE + cht::SIZE) as usize);
 		assert_eq!(db.db.iter(columns::CHT).count(), 0);
 
 		// now finalize the block.
@@ -533,6 +564,7 @@ pub(crate) mod tests {
 		}
 		db.finalize_header(BlockId::Hash(prev_hash)).unwrap();
 		assert_eq!(db.db.iter(columns::HEADER).count(), (1 + cht::SIZE + 1) as usize);
+		assert_eq!(db.db.iter(columns::HASH_LOOKUP).count(), (1 + cht::SIZE + 1) as usize);
 		assert_eq!(db.db.iter(columns::CHT).count(), 1);
 		assert!((0..cht::SIZE).all(|i| db.db.get(columns::HEADER, &number_to_lookup_key(1 + i)).unwrap().is_none()));
 	}

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

@@ -67,10 +67,11 @@ pub struct Meta<N, H> {
 }
 
 /// A block lookup key: used for canonical lookup from block number to hash
-pub type BlockLookupKey = [u8; 4];
+pub type ShortBlockLookupKey = [u8; 4];
 
-/// Convert block number into lookup key (LE representation).
-pub fn number_to_lookup_key<N>(n: N) -> BlockLookupKey where N: As<u64> {
+/// Convert block number into short lookup key (LE representation) for
+/// blocks that are in the canonical chain.
+pub fn number_to_lookup_key<N>(n: N) -> ShortBlockLookupKey where N: As<u64> {
 	let n: u64 = n.as_();
 	assert!(n & 0xffffffff00000000 == 0);
 
@@ -82,6 +83,49 @@ pub fn number_to_lookup_key<N>(n: N) -> BlockLookupKey where N: As<u64> {
 	]
 }
 
+/// 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
+	N: As<u64>,
+	H: AsRef<[u8]>
+{
+	let mut lookup_key = number_to_lookup_key(number).to_vec();
+	lookup_key.extend_from_slice(hash.as_ref());
+	lookup_key
+}
+
+/// Convert block lookup key into block number.
+/// all block lookup keys start with the block number.
+pub fn lookup_key_to_number<N>(key: &[u8]) -> client::error::Result<N> where N: As<u64> {
+	if key.len() < 4 {
+		return Err(client::error::ErrorKind::Backend("Invalid block key".into()).into());
+	}
+	Ok((key[0] as u64) << 24
+		| (key[1] as u64) << 16
+		| (key[2] as u64) << 8
+		| (key[3] as u64)).map(As::sa)
+}
+
+/// Convert block id to block lookup key.
+/// block lookup key is the DB-key header, block and justification are stored under.
+/// looks up lookup key by hash from DB as necessary.
+pub fn block_id_to_lookup_key<Block>(
+	db: &KeyValueDB,
+	hash_lookup_col: Option<u32>,
+	id: BlockId<Block>
+) -> Result<Option<Vec<u8>>, client::error::Error> where
+	Block: BlockT,
+{
+	match id {
+		// numbers are solely looked up in canonical chain
+		BlockId::Number(n) => Ok(Some(number_to_lookup_key(n).to_vec())),
+		BlockId::Hash(h) => db.get(hash_lookup_col, h.as_ref()).map(|v|
+			v.map(|v| { v.into_vec() })
+		).map_err(db_err),
+	}
+}
+
+
 /// Maps database error to client error
 pub fn db_err(err: io::Error) -> client::error::Error {
 	use std::error::Error;
@@ -113,33 +157,12 @@ pub fn open_database(config: &DatabaseSettings, col_meta: Option<u32>, db_type:
 	Ok(Arc::new(db))
 }
 
-/// Convert block id to block key, looking up canonical hash by number from DB as necessary.
-pub fn read_id<Block>(db: &KeyValueDB, col_index: Option<u32>, id: BlockId<Block>) -> Result<Option<Block::Hash>, client::error::Error>
-	where
-		Block: BlockT,
-{
-	match id {
-		BlockId::Hash(h) => Ok(Some(h)),
-		BlockId::Number(n) => db.get(col_index, &number_to_lookup_key(n)).map(|v|
-			v.map(|v| {
-				let mut h = <Block::Hash>::default();
-				{
-					let h = h.as_mut();
-					let len = ::std::cmp::min(v.len(), h.len());
-					h.as_mut().copy_from_slice(&v[..len]);
-				}
-				h
-			})
-		).map_err(db_err),
-	}
-}
-
 /// Read database column entry for the given block.
 pub fn read_db<Block>(db: &KeyValueDB, col_index: Option<u32>, col: Option<u32>, id: BlockId<Block>) -> client::error::Result<Option<DBValue>>
 	where
 		Block: BlockT,
 {
-	read_id(db, col_index, id).and_then(|key| match key {
+	block_id_to_lookup_key(db, col_index, id).and_then(|key| match key {
 		Some(key) => db.get(col, key.as_ref()).map_err(db_err),
 		None => Ok(None),
 	})