Remove BlockNumber <-> u64 conversions from light-client related code (#2666)

* Remove As usage from CHT

* Remove As usage from CHT (continue)

* Restrict BN <-> int conversions in CT

* more BN <-> u64 conversions removed

* upd spec_version

* Apply suggestions from code review

Co-Authored-By: Gavin Wood <github@gavwood.com>

* Apply suggestions from code review

Co-Authored-By: Gavin Wood <github@gavwood.com>

* more grumbles

* fix last grumbles + compilation

* too long lines

* too long lines

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

@@ -35,7 +35,7 @@ mod list_cache;
 mod list_entry;
 mod list_storage;
 
-/// Minimal post-finalization age age of finalized blocks before they'll pruned.
+/// Minimal post-finalization age of finalized blocks before they'll pruned.
 const PRUNE_DEPTH: u32 = 1024;
 
 /// The type of entry that is inserted to the cache.

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

@@ -48,7 +48,7 @@ use primitives::storage::well_known_keys;
 use runtime_primitives::{generic::BlockId, Justification, StorageOverlay, ChildrenStorageOverlay};
 use runtime_primitives::traits::{
 	Block as BlockT, Header as HeaderT, NumberFor, Zero, One, Digest, DigestItem,
-	SaturatedConversion,
+	SaturatedConversion
 };
 use runtime_primitives::BuildStorage;
 use state_machine::backend::Backend as StateBackend;
@@ -67,7 +67,7 @@ pub use state_db::PruningMode;
 use client::in_mem::Backend as InMemoryBackend;
 
 const CANONICALIZATION_DELAY: u64 = 4096;
-const MIN_BLOCKS_TO_KEEP_CHANGES_TRIES_FOR: u64 = 32768;
+const MIN_BLOCKS_TO_KEEP_CHANGES_TRIES_FOR: u32 = 32768;
 
 /// DB-backed patricia trie state, transaction type is an overlay of changes to commit.
 pub type DbState = state_machine::TrieBackend<Arc<state_machine::Storage<Blake2Hasher>>, Blake2Hasher>;
@@ -423,11 +423,11 @@ impl state_machine::Storage<Blake2Hasher> for DbGenesisStorage {
 pub struct DbChangesTrieStorage<Block: BlockT> {
 	db: Arc<KeyValueDB>,
 	meta: Arc<RwLock<Meta<NumberFor<Block>, Block::Hash>>>,
-	min_blocks_to_keep: Option<u64>,
+	min_blocks_to_keep: Option<u32>,
 	_phantom: ::std::marker::PhantomData<Block>,
 }
 
-impl<Block: BlockT> DbChangesTrieStorage<Block> {
+impl<Block: BlockT<Hash=H256>> DbChangesTrieStorage<Block> {
 	/// Commit new changes trie.
 	pub fn commit(&self, tx: &mut DBTransaction, mut changes_trie: MemoryDB<Blake2Hasher>) {
 		for (key, (val, _)) in changes_trie.drain() {
@@ -446,53 +446,79 @@ impl<Block: BlockT> DbChangesTrieStorage<Block> {
 		state_machine::prune_changes_tries(
 			config,
 			&*self,
-			min_blocks_to_keep,
+			min_blocks_to_keep.into(),
 			&state_machine::ChangesTrieAnchorBlockId {
 				hash: convert_hash(&block_hash),
-				number: block_num.saturated_into::<u64>(),
+				number: block_num,
 			},
 			|node| tx.delete(columns::CHANGES_TRIE, node.as_ref()));
 	}
 }
 
-impl<Block: BlockT> client::backend::PrunableStateChangesTrieStorage<Blake2Hasher> for DbChangesTrieStorage<Block> {
+impl<Block> client::backend::PrunableStateChangesTrieStorage<Block, Blake2Hasher>
+	for DbChangesTrieStorage<Block>
+where
+	Block: BlockT<Hash=H256>,
+{
 	fn oldest_changes_trie_block(
 		&self,
 		config: &ChangesTrieConfiguration,
-		best_finalized_block: u64
-	) -> u64 {
+		best_finalized_block: NumberFor<Block>,
+	) -> NumberFor<Block> {
 		match self.min_blocks_to_keep {
 			Some(min_blocks_to_keep) => state_machine::oldest_non_pruned_changes_trie(
 				config,
-				min_blocks_to_keep,
+				min_blocks_to_keep.into(),
 				best_finalized_block,
 			),
-			None => 1,
+			None => One::one(),
 		}
 	}
 }
 
-impl<Block: BlockT> state_machine::ChangesTrieRootsStorage<Blake2Hasher> for DbChangesTrieStorage<Block> {
-	fn root(&self, anchor: &state_machine::ChangesTrieAnchorBlockId<H256>, block: u64) -> Result<Option<H256>, String> {
+impl<Block> state_machine::ChangesTrieRootsStorage<Blake2Hasher, NumberFor<Block>>
+	for DbChangesTrieStorage<Block>
+where
+	Block: BlockT<Hash=H256>,
+{
+	fn build_anchor(
+		&self,
+		hash: H256,
+	) -> Result<state_machine::ChangesTrieAnchorBlockId<H256, NumberFor<Block>>, String> {
+		utils::read_header::<Block>(&*self.db, columns::KEY_LOOKUP, columns::HEADER, BlockId::Hash(hash))
+			.map_err(|e| e.to_string())
+			.and_then(|maybe_header| maybe_header.map(|header|
+				state_machine::ChangesTrieAnchorBlockId {
+					hash,
+					number: *header.number(),
+				}
+			).ok_or_else(|| format!("Unknown header: {}", hash)))
+	}
+
+	fn root(
+		&self,
+		anchor: &state_machine::ChangesTrieAnchorBlockId<H256, NumberFor<Block>>,
+		block: NumberFor<Block>,
+	) -> Result<Option<H256>, String> {
 		// check API requirement: we can't get NEXT block(s) based on anchor
 		if block > anchor.number {
 			return Err(format!("Can't get changes trie root at {} using anchor at {}", block, anchor.number));
 		}
 
 		// we need to get hash of the block to resolve changes trie root
-		let block_id = if block <= self.meta.read().finalized_number.saturated_into::<u64>() {
+		let block_id = if block <= self.meta.read().finalized_number {
 			// if block is finalized, we could just read canonical hash
-			BlockId::Number(block.saturated_into())
+			BlockId::Number(block)
 		} else {
 			// the block is not finalized
 			let mut current_num = anchor.number;
 			let mut current_hash: Block::Hash = convert_hash(&anchor.hash);
 			let maybe_anchor_header: Block::Header = utils::require_header::<Block>(
-				&*self.db, columns::KEY_LOOKUP, columns::HEADER, BlockId::Number(current_num.saturated_into())
+				&*self.db, columns::KEY_LOOKUP, columns::HEADER, BlockId::Number(current_num)
 			).map_err(|e| e.to_string())?;
 			if maybe_anchor_header.hash() == current_hash {
 				// if anchor is canonicalized, then the block is also canonicalized
-				BlockId::Number(block.saturated_into())
+				BlockId::Number(block)
 			} else {
 				// else (block is not finalized + anchor is not canonicalized):
 				// => we should find the required block hash by traversing
@@ -503,7 +529,7 @@ impl<Block: BlockT> state_machine::ChangesTrieRootsStorage<Blake2Hasher> for DbC
 					).map_err(|e| e.to_string())?;
 
 					current_hash = *current_header.parent_hash();
-					current_num = current_num - 1;
+					current_num = current_num - One::one();
 				}
 
 				BlockId::Hash(current_hash)
@@ -517,7 +543,11 @@ impl<Block: BlockT> state_machine::ChangesTrieRootsStorage<Blake2Hasher> for DbC
 	}
 }
 
-impl<Block: BlockT> state_machine::ChangesTrieStorage<Blake2Hasher> for DbChangesTrieStorage<Block> {
+impl<Block> state_machine::ChangesTrieStorage<Blake2Hasher, NumberFor<Block>>
+	for DbChangesTrieStorage<Block>
+where
+	Block: BlockT<Hash=H256>,
+{
 	fn get(&self, key: &H256, _prefix: &[u8]) -> Result<Option<DBValue>, String> {
 		self.db.get(columns::CHANGES_TRIE, &key[..])
 			.map_err(|err| format!("{}", err))

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

@@ -32,8 +32,9 @@ use client::light::blockchain::Storage as LightBlockchainStorage;
 use parity_codec::{Decode, Encode};
 use primitives::Blake2Hasher;
 use runtime_primitives::generic::BlockId;
-use runtime_primitives::traits::{Block as BlockT, Header as HeaderT,
-	Zero, One, SaturatedConversion, NumberFor, Digest, DigestItem
+use runtime_primitives::traits::{
+	Block as BlockT, Header as HeaderT,
+	Zero, One, NumberFor, Digest, DigestItem,
 };
 use consensus_common::well_known_cache_keys;
 use crate::cache::{DbCacheSync, DbCache, ComplexBlockId, EntryType as CacheEntryType};
@@ -269,12 +270,18 @@ impl<Block: BlockT> LightStorage<Block> {
 		transaction.put(columns::META, meta_keys::FINALIZED_BLOCK, &lookup_key);
 
 		// build new CHT(s) if required
-		if let Some(new_cht_number) = cht::is_build_required(cht::SIZE, *header.number()) {
-			let new_cht_start: NumberFor<Block> = cht::start_number(cht::SIZE, new_cht_number);
+		if let Some(new_cht_number) = cht::is_build_required(cht::size(), *header.number()) {
+			let new_cht_start: NumberFor<Block> = cht::start_number(cht::size(), new_cht_number);
+
+			let mut current_num = new_cht_start;
+			let cht_range = ::std::iter::from_fn(|| {
+				let old_current_num = current_num;
+				current_num = current_num + One::one();
+				Some(old_current_num)
+			});
 
 			let new_header_cht_root = cht::compute_root::<Block::Header, Blake2Hasher, _>(
-				cht::SIZE, new_cht_number, (new_cht_start.saturated_into::<u64>()..)
-				.map(|num| self.hash(num.saturated_into()))
+				cht::size(), new_cht_number, cht_range.map(|num| self.hash(num))
 			)?;
 			transaction.put(
 				columns::CHT,
@@ -284,9 +291,15 @@ impl<Block: BlockT> LightStorage<Block> {
 
 			// if the header includes changes trie root, let's build a changes tries roots CHT
 			if header.digest().log(DigestItem::as_changes_trie_root).is_some() {
+				let mut current_num = new_cht_start;
+				let cht_range = ::std::iter::from_fn(|| {
+					let old_current_num = current_num;
+					current_num = current_num + One::one();
+					Some(old_current_num)
+				});
 				let new_changes_trie_cht_root = cht::compute_root::<Block::Header, Blake2Hasher, _>(
-					cht::SIZE, new_cht_number, (new_cht_start.saturated_into::<u64>()..)
-					.map(|num| self.changes_trie_root(BlockId::Number(num.saturated_into())))
+					cht::size(), new_cht_number, cht_range
+						.map(|num| self.changes_trie_root(BlockId::Number(num)))
 				)?;
 				transaction.put(
 					columns::CHT,
@@ -297,7 +310,7 @@ impl<Block: BlockT> LightStorage<Block> {
 
 			// prune headers that are replaced with CHT
 			let mut prune_block = new_cht_start;
-			let new_cht_end = cht::end_number(cht::SIZE, new_cht_number);
+			let new_cht_end = cht::end_number(cht::size(), new_cht_number);
 			trace!(target: "db", "Replacing blocks [{}..{}] with CHT#{}",
 				new_cht_start, new_cht_end, new_cht_number);
 
@@ -330,7 +343,7 @@ impl<Block: BlockT> LightStorage<Block> {
 	fn read_cht_root(
 		&self,
 		cht_type: u8,
-		cht_size: u64,
+		cht_size: NumberFor<Block>,
 		block: NumberFor<Block>
 	) -> ClientResult<Block::Hash> {
 		let no_cht_for_block = || ClientError::Backend(format!("CHT for block {} not exists", block));
@@ -482,11 +495,19 @@ impl<Block> LightBlockchainStorage<Block> for LightStorage<Block>
 		}
 	}
 
-	fn header_cht_root(&self, cht_size: u64, block: NumberFor<Block>) -> ClientResult<Block::Hash> {
+	fn header_cht_root(
+		&self,
+		cht_size: NumberFor<Block>,
+		block: NumberFor<Block>,
+	) -> ClientResult<Block::Hash> {
 		self.read_cht_root(HEADER_CHT_PREFIX, cht_size, block)
 	}
 
-	fn changes_trie_cht_root(&self, cht_size: u64, block: NumberFor<Block>) -> ClientResult<Block::Hash> {
+	fn changes_trie_cht_root(
+		&self,
+		cht_size: NumberFor<Block>,
+		block: NumberFor<Block>,
+	) -> ClientResult<Block::Hash> {
 		self.read_cht_root(CHANGES_TRIE_CHT_PREFIX, cht_size, block)
 	}
 
@@ -670,33 +691,44 @@ pub(crate) mod tests {
 	fn finalized_ancient_headers_are_replaced_with_cht() {
 		fn insert_headers<F: Fn(&Hash, u64) -> Header>(header_producer: F) -> LightStorage<Block> {
 			let db = LightStorage::new_test();
+			let cht_size: u64 = cht::size();
+			let ucht_size: usize = cht_size as _;
 
 			// insert genesis block header (never pruned)
 			let mut prev_hash = insert_final_block(&db, HashMap::new(), || header_producer(&Default::default(), 0));
 
 			// insert SIZE blocks && ensure that nothing is pruned
-			for number in 0..cht::SIZE {
+			
+			for number in 0..cht::size() {
 				prev_hash = insert_block(&db, HashMap::new(), || header_producer(&prev_hash, 1 + number));
 			}
-			assert_eq!(db.db.iter(columns::HEADER).count(), (1 + cht::SIZE) as usize);
+			assert_eq!(db.db.iter(columns::HEADER).count(), 1 + ucht_size);
 			assert_eq!(db.db.iter(columns::CHT).count(), 0);
 
 			// insert next SIZE blocks && ensure that nothing is pruned
-			for number in 0..cht::SIZE {
-				prev_hash = insert_block(&db, HashMap::new(), || header_producer(&prev_hash, 1 + cht::SIZE + number));
+			for number in 0..(cht_size as _) {
+				prev_hash = insert_block(
+					&db,
+					HashMap::new(),
+					|| header_producer(&prev_hash, 1 + cht_size + number),
+				);
 			}
-			assert_eq!(db.db.iter(columns::HEADER).count(), (1 + cht::SIZE + cht::SIZE) as usize);
+			assert_eq!(db.db.iter(columns::HEADER).count(), 1 + ucht_size + ucht_size);
 			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
+			// 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, HashMap::new(), || header_producer(&prev_hash, 1 + cht::SIZE + cht::SIZE));
-			assert_eq!(db.db.iter(columns::HEADER).count(), (2 + cht::SIZE + cht::SIZE) as usize);
+			prev_hash = insert_block(
+				&db,
+				HashMap::new(),
+				|| header_producer(&prev_hash, 1 + cht_size + cht_size),
+			);
+			assert_eq!(db.db.iter(columns::HEADER).count(), 2 + ucht_size + ucht_size);
 			assert_eq!(db.db.iter(columns::CHT).count(), 0);
 
 			// now finalize the block.
-			for i in (0..(cht::SIZE + cht::SIZE)).map(|i| i + 1) {
-				db.finalize_header(BlockId::Number(i)).unwrap();
+			for i in (0..(ucht_size + ucht_size)).map(|i| i + 1) {
+				db.finalize_header(BlockId::Number(i as _)).unwrap();
 			}
 			db.finalize_header(BlockId::Hash(prev_hash)).unwrap();
 			db
@@ -704,34 +736,36 @@ pub(crate) mod tests {
 
 		// when headers are created without changes tries roots
 		let db = insert_headers(default_header);
-		assert_eq!(db.db.iter(columns::HEADER).count(), (1 + cht::SIZE + 1) as usize);
-		assert_eq!(db.db.iter(columns::KEY_LOOKUP).count(), (2 * (1 + cht::SIZE + 1)) as usize);
+		let cht_size: u64 = cht::size();
+		assert_eq!(db.db.iter(columns::HEADER).count(), (1 + cht_size + 1) as usize);
+		assert_eq!(db.db.iter(columns::KEY_LOOKUP).count(), (2 * (1 + cht_size + 1)) as usize);
 		assert_eq!(db.db.iter(columns::CHT).count(), 1);
-		assert!((0..cht::SIZE).all(|i| db.header(BlockId::Number(1 + i)).unwrap().is_none()));
-		assert!(db.header_cht_root(cht::SIZE, cht::SIZE / 2).is_ok());
-		assert!(db.header_cht_root(cht::SIZE, cht::SIZE + cht::SIZE / 2).is_err());
-		assert!(db.changes_trie_cht_root(cht::SIZE, cht::SIZE / 2).is_err());
-		assert!(db.changes_trie_cht_root(cht::SIZE, cht::SIZE + cht::SIZE / 2).is_err());
+		assert!((0..cht_size as _).all(|i| db.header(BlockId::Number(1 + i)).unwrap().is_none()));
+		assert!(db.header_cht_root(cht_size, cht_size / 2).is_ok());
+		assert!(db.header_cht_root(cht_size, cht_size + cht_size / 2).is_err());
+		assert!(db.changes_trie_cht_root(cht_size, cht_size / 2).is_err());
+		assert!(db.changes_trie_cht_root(cht_size, cht_size + cht_size / 2).is_err());
 
 		// when headers are created with changes tries roots
 		let db = insert_headers(header_with_changes_trie);
-		assert_eq!(db.db.iter(columns::HEADER).count(), (1 + cht::SIZE + 1) as usize);
+		assert_eq!(db.db.iter(columns::HEADER).count(), (1 + cht_size + 1) as usize);
 		assert_eq!(db.db.iter(columns::CHT).count(), 2);
-		assert!((0..cht::SIZE).all(|i| db.header(BlockId::Number(1 + i)).unwrap().is_none()));
-		assert!(db.header_cht_root(cht::SIZE, cht::SIZE / 2).is_ok());
-		assert!(db.header_cht_root(cht::SIZE, cht::SIZE + cht::SIZE / 2).is_err());
-		assert!(db.changes_trie_cht_root(cht::SIZE, cht::SIZE / 2).is_ok());
-		assert!(db.changes_trie_cht_root(cht::SIZE, cht::SIZE + cht::SIZE / 2).is_err());
+		assert!((0..cht_size as _).all(|i| db.header(BlockId::Number(1 + i)).unwrap().is_none()));
+		assert!(db.header_cht_root(cht_size, cht_size / 2).is_ok());
+		assert!(db.header_cht_root(cht_size, cht_size + cht_size / 2).is_err());
+		assert!(db.changes_trie_cht_root(cht_size, cht_size / 2).is_ok());
+		assert!(db.changes_trie_cht_root(cht_size, cht_size + cht_size / 2).is_err());
 	}
 
 	#[test]
 	fn get_cht_fails_for_genesis_block() {
-		assert!(LightStorage::<Block>::new_test().header_cht_root(cht::SIZE, 0).is_err());
+		assert!(LightStorage::<Block>::new_test().header_cht_root(cht::size(), 0).is_err());
 	}
 
 	#[test]
 	fn get_cht_fails_for_non_existant_cht() {
-		assert!(LightStorage::<Block>::new_test().header_cht_root(cht::SIZE, (cht::SIZE / 2) as u64).is_err());
+		let cht_size: u64 = cht::size();
+		assert!(LightStorage::<Block>::new_test().header_cht_root(cht_size, cht_size / 2).is_err());
 	}
 
 	#[test]
@@ -740,20 +774,22 @@ pub(crate) mod tests {
 
 		// insert 1 + SIZE + SIZE + 1 blocks so that CHT#0 is created
 		let mut prev_hash = insert_final_block(&db, HashMap::new(), || header_with_changes_trie(&Default::default(), 0));
-		for i in 1..1 + cht::SIZE + cht::SIZE + 1 {
+		let cht_size: u64 = cht::size();
+		let ucht_size: usize = cht_size as _;
+		for i in 1..1 + ucht_size + ucht_size + 1 {
 			prev_hash = insert_block(&db, HashMap::new(), || header_with_changes_trie(&prev_hash, i as u64));
 			db.finalize_header(BlockId::Hash(prev_hash)).unwrap();
 		}
 
-		let cht_root_1 = db.header_cht_root(cht::SIZE, cht::start_number(cht::SIZE, 0)).unwrap();
-		let cht_root_2 = db.header_cht_root(cht::SIZE, (cht::start_number(cht::SIZE, 0) + cht::SIZE / 2) as u64).unwrap();
-		let cht_root_3 = db.header_cht_root(cht::SIZE, cht::end_number(cht::SIZE, 0)).unwrap();
+		let cht_root_1 = db.header_cht_root(cht_size, cht::start_number(cht_size, 0)).unwrap();
+		let cht_root_2 = db.header_cht_root(cht_size, cht::start_number(cht_size, 0) + cht_size / 2).unwrap();
+		let cht_root_3 = db.header_cht_root(cht_size, cht::end_number(cht_size, 0)).unwrap();
 		assert_eq!(cht_root_1, cht_root_2);
 		assert_eq!(cht_root_2, cht_root_3);
 
-		let cht_root_1 = db.changes_trie_cht_root(cht::SIZE, cht::start_number(cht::SIZE, 0)).unwrap();
-		let cht_root_2 = db.changes_trie_cht_root(cht::SIZE, (cht::start_number(cht::SIZE, 0) + cht::SIZE / 2) as u64).unwrap();
-		let cht_root_3 = db.changes_trie_cht_root(cht::SIZE, cht::end_number(cht::SIZE, 0)).unwrap();
+		let cht_root_1 = db.changes_trie_cht_root(cht_size, cht::start_number(cht_size, 0)).unwrap();
+		let cht_root_2 = db.changes_trie_cht_root(cht_size, cht::start_number(cht_size, 0) + cht_size / 2).unwrap();
+		let cht_root_3 = db.changes_trie_cht_root(cht_size, cht::end_number(cht_size, 0)).unwrap();
 		assert_eq!(cht_root_1, cht_root_2);
 		assert_eq!(cht_root_2, cht_root_3);
 	}