diff --git a/substrate/client/rpc/src/state/state_full.rs b/substrate/client/rpc/src/state/state_full.rs index 698d42f101..582d3a0e7e 100644 --- a/substrate/client/rpc/src/state/state_full.rs +++ b/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 FullState from: Block::Hash, to: Option ) -> Result> { - 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::()); - 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::(from, to, e.to_string()))?; + + let invalid_block_err = |e: ClientError| invalid_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::(&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::()); + 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) -> (Range, (range1, range2) } -fn invalid_block_range(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( + from: &CachedHeaderMetadata, + to: &CachedHeaderMetadata, + details: String, +) -> Error { + let to_string = |h: &CachedHeaderMetadata| format!("{} ({:?})", h.number, h.hash); Error::InvalidBlockRange { from: to_string(from), to: to_string(to), - details: reason, + details, + } +} + +fn invalid_block( + from: B::Hash, + to: Option, + details: String, +) -> Error { + Error::InvalidBlockRange { + from: format!("{:?}", from), + to: format!("{:?}", to), + details, } } diff --git a/substrate/client/rpc/src/state/tests.rs b/substrate/client/rpc/src/state/tests.rs index b77e5a8291..2ae22df1a0 100644 --- a/substrate/client/rpc/src/state/tests.rs +++ b/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()));