Use header metadata in state full and refactor. (#3757)

* Use header metadata in state full and refactor.
* better log

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

* Apply review suggestions.

substrate/client/rpc/src/state/state_full.rs

@@ -29,7 +29,9 @@ use rpc::{
 
 use api::Subscriptions;
 use client_api::backend::Backend;
-use sp_blockchain::Result as ClientResult;
+use sp_blockchain::{
+	Result as ClientResult, Error as ClientError, HeaderMetadata, CachedHeaderMetadata
+};
 use client::{
 	Client, CallExecutor, BlockchainEvents, 
 };
@@ -40,7 +42,7 @@ use runtime_version::RuntimeVersion;
 use state_machine::ExecutionStrategy;
 use sp_runtime::{
 	generic::BlockId,
-	traits::{Block as BlockT, Header, NumberFor, ProvideRuntimeApi, SaturatedConversion},
+	traits::{Block as BlockT, NumberFor, ProvideRuntimeApi, SaturatedConversion},
 };
 
 use sp_api::Metadata;
@@ -91,59 +93,49 @@ impl<B, E, Block: BlockT, RA> FullState<B, E, Block, RA>
 		from: Block::Hash,
 		to: Option<Block::Hash>
 	) -> Result<QueryStorageRange<Block>> {
-		let to = self.block_or_best(to).map_err(client_err)?;
-		let from_hdr = self.client.header(&BlockId::hash(from)).map_err(client_err)?;
-		let to_hdr = self.client.header(&BlockId::hash(to)).map_err(client_err)?;
-		match (from_hdr, to_hdr) {
-			(Some(ref from), Some(ref to)) if from.number() <= to.number() => {
-				// check if we can get from `to` to `from` by going through parent_hashes.
-				let from_number = *from.number();
-				let blocks = {
-					let mut blocks = vec![to.hash()];
-					let mut last = to.clone();
-					while *last.number() > from_number {
-						let hdr = self.client
-							.header(&BlockId::hash(*last.parent_hash()))
-							.map_err(client_err)?;
-						if let Some(hdr) = hdr {
-							blocks.push(hdr.hash());
-							last = hdr;
-						} else {
-							return Err(invalid_block_range(
-								Some(from),
-								Some(to),
-								format!("Parent of {} ({}) not found", last.number(), last.hash()),
-							))
-						}
-					}
-					if last.hash() != from.hash() {
-						return Err(invalid_block_range(
-							Some(from),
-							Some(to),
-							format!("Expected to reach `from`, got {} ({})", last.number(), last.hash()),
-						))
-					}
-					blocks.reverse();
-					blocks
-				};
-				// check if we can filter blocks-with-changes from some (sub)range using changes tries
-				let changes_trie_range = self.client
-					.max_key_changes_range(from_number, BlockId::Hash(to.hash()))
-					.map_err(client_err)?;
-				let filtered_range_begin = changes_trie_range
-					.map(|(begin, _)| (begin - from_number).saturated_into::<usize>());
-				let (unfiltered_range, filtered_range) = split_range(blocks.len(), filtered_range_begin);
-				Ok(QueryStorageRange {
-					hashes: blocks,
-					first_number: from_number,
-					unfiltered_range,
-					filtered_range,
-				})
-			},
-			(from, to) => Err(
-				invalid_block_range(from.as_ref(), to.as_ref(), "Invalid range or unknown block".into())
-			),
+		let to = self.block_or_best(to).map_err(|e| invalid_block::<Block>(from, to, e.to_string()))?;
+
+		let invalid_block_err = |e: ClientError| invalid_block::<Block>(from, Some(to), e.to_string());
+		let from_meta = self.client.header_metadata(from).map_err(invalid_block_err)?;
+		let to_meta = self.client.header_metadata(to).map_err(invalid_block_err)?;
+
+		if from_meta.number >= to_meta.number {
+			return Err(invalid_block_range(&from_meta, &to_meta, "from number >= to number".to_owned()))
 		}
+
+		// check if we can get from `to` to `from` by going through parent_hashes.
+		let from_number = from_meta.number;
+		let hashes = {
+			let mut hashes = vec![to_meta.hash];
+			let mut last = to_meta.clone();
+			while last.number > from_number {
+				let header_metadata = self.client
+					.header_metadata(last.parent)
+					.map_err(|e| invalid_block_range::<Block>(&last, &to_meta, e.to_string()))?;
+				hashes.push(header_metadata.hash);
+				last = header_metadata;
+			}
+			if last.hash != from_meta.hash {
+				return Err(invalid_block_range(&from_meta, &to_meta, "from and to are on different forks".to_owned()))
+			}
+			hashes.reverse();
+			hashes
+		};
+
+		// check if we can filter blocks-with-changes from some (sub)range using changes tries
+		let changes_trie_range = self.client
+			.max_key_changes_range(from_number, BlockId::Hash(to_meta.hash))
+			.map_err(client_err)?;
+		let filtered_range_begin = changes_trie_range
+			.map(|(begin, _)| (begin - from_number).saturated_into::<usize>());
+		let (unfiltered_range, filtered_range) = split_range(hashes.len(), filtered_range_begin);
+
+		Ok(QueryStorageRange {
+			hashes,
+			first_number: from_number,
+			unfiltered_range,
+			filtered_range,
+		})
 	}
 
 	/// Iterates through range.unfiltered_range and check each block for changes of keys' values.
@@ -501,15 +493,28 @@ pub(crate) fn split_range(size: usize, middle: Option<usize>) -> (Range<usize>,
 	(range1, range2)
 }
 
-fn invalid_block_range<H: Header>(from: Option<&H>, to: Option<&H>, reason: String) -> Error {
-	let to_string = |x: Option<&H>| match x {
-		None => "unknown hash".into(),
-		Some(h) => format!("{} ({})", h.number(), h.hash()),
-	};
+fn invalid_block_range<B: BlockT>(
+	from: &CachedHeaderMetadata<B>,
+	to: &CachedHeaderMetadata<B>,
+	details: String,
+) -> Error {
+	let to_string = |h: &CachedHeaderMetadata<B>| format!("{} ({:?})", h.number, h.hash);
 
 	Error::InvalidBlockRange {
 		from: to_string(from),
 		to: to_string(to),
-		details: reason,
+		details,
+	}
+}
+
+fn invalid_block<B: BlockT>(
+	from: B::Hash,
+	to: Option<B::Hash>,
+	details: String,
+) -> Error {
+	Error::InvalidBlockRange {
+		from: format!("{:?}", from),
+		to: format!("{:?}", to),
+		details,
 	}
 }

substrate/client/rpc/src/state/tests.rs

@@ -22,6 +22,7 @@ use std::sync::Arc;
 use assert_matches::assert_matches;
 use futures01::stream::Stream;
 use primitives::storage::well_known_keys;
+use primitives::hash::H256;
 use sp_io::hashing::blake2_256;
 use test_client::{
 	prelude::*,
@@ -255,6 +256,98 @@ fn should_query_storage() {
 			],
 		});
 		assert_eq!(result.wait().unwrap(), expected);
+
+		// Query changes up to block2.
+		let result = api.query_storage(
+			keys.clone(),
+			genesis_hash,
+			Some(block2_hash),
+		);
+
+		assert_eq!(result.wait().unwrap(), expected);
+
+		// Inverted range.
+		let result = api.query_storage(
+			keys.clone(),
+			block1_hash,
+			Some(genesis_hash),
+		);
+
+		assert_eq!(
+			result.wait().map_err(|e| e.to_string()),
+			Err(Error::InvalidBlockRange {
+				from: format!("1 ({:?})", block1_hash),
+				to: format!("0 ({:?})", genesis_hash),
+				details: "from number >= to number".to_owned(),
+			}).map_err(|e| e.to_string())
+		);
+
+		let random_hash1 = H256::random();
+		let random_hash2 = H256::random();
+
+		// Invalid second hash.
+		let result = api.query_storage(
+			keys.clone(),
+			genesis_hash,
+			Some(random_hash1),
+		);
+
+		assert_eq!(
+			result.wait().map_err(|e| e.to_string()),
+			Err(Error::InvalidBlockRange {
+				from: format!("{:?}", genesis_hash),
+				to: format!("{:?}", Some(random_hash1)),
+				details: format!("UnknownBlock: header not found in db: {}", random_hash1),
+			}).map_err(|e| e.to_string())
+		);
+
+		// Invalid first hash with Some other hash.
+		let result = api.query_storage(
+			keys.clone(),
+			random_hash1,
+			Some(genesis_hash),
+		);
+
+		assert_eq!(
+			result.wait().map_err(|e| e.to_string()),
+			Err(Error::InvalidBlockRange {
+				from: format!("{:?}", random_hash1),
+				to: format!("{:?}", Some(genesis_hash)),
+				details: format!("UnknownBlock: header not found in db: {}", random_hash1),
+			}).map_err(|e| e.to_string()),
+		);
+
+		// Invalid first hash with None.
+		let result = api.query_storage(
+			keys.clone(),
+			random_hash1,
+			None,
+		);
+
+		assert_eq!(
+			result.wait().map_err(|e| e.to_string()),
+			Err(Error::InvalidBlockRange {
+				from: format!("{:?}", random_hash1),
+				to: format!("{:?}", Some(block2_hash)), // Best block hash.
+				details: format!("UnknownBlock: header not found in db: {}", random_hash1),
+			}).map_err(|e| e.to_string()),
+		);
+
+		// Both hashes invalid.
+		let result = api.query_storage(
+			keys.clone(), 
+			random_hash1,
+			Some(random_hash2),
+		);
+
+		assert_eq!(
+			result.wait().map_err(|e| e.to_string()),
+			Err(Error::InvalidBlockRange {
+				from: format!("{:?}", random_hash1), // First hash not found.
+				to: format!("{:?}", Some(random_hash2)),
+				details: format!("UnknownBlock: header not found in db: {}", random_hash1),
+			}).map_err(|e| e.to_string()),
+		);
 	}
 
 	run_tests(Arc::new(test_client::new()));