From c08dcff4af3f74cf439ffc284e6cf0bb841fe27c Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 23 Apr 2018 21:05:23 +0800 Subject: [PATCH 01/12] Fetch logs by hash in blockchain database --- ethcore/src/blockchain/blockchain.rs | 10 +++---- ethcore/src/client/client.rs | 41 ++++++++++++++++++---------- 2 files changed, 32 insertions(+), 19 deletions(-) diff --git a/ethcore/src/blockchain/blockchain.rs b/ethcore/src/blockchain/blockchain.rs index 6ad8bf8111a..cc3e6af3970 100644 --- a/ethcore/src/blockchain/blockchain.rs +++ b/ethcore/src/blockchain/blockchain.rs @@ -148,7 +148,7 @@ pub trait BlockProvider { fn blocks_with_bloom(&self, bloom: &Bloom, from_block: BlockNumber, to_block: BlockNumber) -> Vec; /// Returns logs matching given filter. - fn logs(&self, blocks: Vec, matches: F, limit: Option) -> Vec + fn logs(&self, blocks: Vec, matches: F, limit: Option) -> Vec where F: Fn(&LogEntry) -> bool + Send + Sync, Self: Sized; } @@ -332,16 +332,16 @@ impl BlockProvider for BlockChain { .collect() } - fn logs(&self, mut blocks: Vec, matches: F, limit: Option) -> Vec + fn logs(&self, mut blocks: Vec, matches: F, limit: Option) -> Vec where F: Fn(&LogEntry) -> bool + Send + Sync, Self: Sized { // sort in reverse order - blocks.sort_by(|a, b| b.cmp(a)); + blocks.reverse(); let mut logs = blocks .chunks(128) .flat_map(move |blocks_chunk| { blocks_chunk.into_par_iter() - .filter_map(|number| self.block_hash(*number).map(|hash| (*number, hash))) + .filter_map(|hash| self.block_number(&hash).map(|r| (r, hash))) .filter_map(|(number, hash)| self.block_receipts(&hash).map(|r| (number, hash, r.receipts))) .filter_map(|(number, hash, receipts)| self.block_body(&hash).map(|ref b| (number, hash, receipts, b.transaction_hashes()))) .flat_map(|(number, hash, mut receipts, mut hashes)| { @@ -368,7 +368,7 @@ impl BlockProvider for BlockChain { .enumerate() .map(move |(i, log)| LocalizedLogEntry { entry: log, - block_hash: hash, + block_hash: *hash, block_number: number, transaction_hash: tx_hash, // iterating in reverse order diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 6ec318a03f9..4a38a1dd8e2 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1853,21 +1853,34 @@ impl BlockChainClient for Client { } fn logs(&self, filter: Filter) -> Vec { - let (from, to) = match (self.block_number_ref(&filter.from_block), self.block_number_ref(&filter.to_block)) { - (Some(from), Some(to)) => (from, to), - _ => return Vec::new(), - }; + let blocks = match &filter.from_block { + &BlockId::Hash(hash) if filter.from_block == filter.to_block => { + // Skip bloom check and directly use the hash. + vec![hash] + }, + _ => { + let (from, to) = match (self.block_number_ref(&filter.from_block), self.block_number_ref(&filter.to_block)) { + (Some(from), Some(to)) => (from, to), + _ => return Vec::new(), + }; - let chain = self.chain.read(); - let blocks = filter.bloom_possibilities().iter() - .map(move |bloom| { - chain.blocks_with_bloom(bloom, from, to) - }) - .flat_map(|m| m) - // remove duplicate elements - .collect::>() - .into_iter() - .collect::>(); + let chain = self.chain.read(); + let mut numbers = filter.bloom_possibilities().iter() + .map(|bloom| { + chain.blocks_with_bloom(bloom, from, to) + }) + .flat_map(|m| m) + // remove duplicate elements + .collect::>() + .into_iter() + .collect::>(); + + numbers.sort_by(|a, b| a.cmp(b)); + numbers.into_iter() + .filter_map(|n| chain.block_hash(n)) + .collect::>() + }, + }; self.chain.read().logs(blocks, |entry| filter.matches(entry), filter.limit) } From 6eae998915e4d9f21859573663bc737f271eedac Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 23 Apr 2018 21:08:07 +0800 Subject: [PATCH 02/12] Fix tests --- ethcore/src/blockchain/blockchain.rs | 4 ++-- ethcore/src/verification/verification.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ethcore/src/blockchain/blockchain.rs b/ethcore/src/blockchain/blockchain.rs index cc3e6af3970..f1579dd50db 100644 --- a/ethcore/src/blockchain/blockchain.rs +++ b/ethcore/src/blockchain/blockchain.rs @@ -1976,8 +1976,8 @@ mod tests { ]); // when - let logs1 = bc.logs(vec![1, 2], |_| true, None); - let logs2 = bc.logs(vec![1, 2], |_| true, Some(1)); + let logs1 = bc.logs(vec![b1_hash, b2_hash], |_| true, None); + let logs2 = bc.logs(vec![b1_hash, b2_hash], |_| true, Some(1)); // then assert_eq!(logs1, vec![ diff --git a/ethcore/src/verification/verification.rs b/ethcore/src/verification/verification.rs index 814a12aadfe..92f3e77f902 100644 --- a/ethcore/src/verification/verification.rs +++ b/ethcore/src/verification/verification.rs @@ -476,7 +476,7 @@ mod tests { unimplemented!() } - fn logs(&self, _blocks: Vec, _matches: F, _limit: Option) -> Vec + fn logs(&self, _blocks: Vec, _matches: F, _limit: Option) -> Vec where F: Fn(&LogEntry) -> bool, Self: Sized { unimplemented!() } From cd805d23894249efb49b175616031e4f63d4d2f3 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 23 Apr 2018 21:40:40 +0800 Subject: [PATCH 03/12] Add unit test for branch block logs fetching --- ethcore/src/blockchain/blockchain.rs | 38 ++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/ethcore/src/blockchain/blockchain.rs b/ethcore/src/blockchain/blockchain.rs index f1579dd50db..5ba3f5d599a 100644 --- a/ethcore/src/blockchain/blockchain.rs +++ b/ethcore/src/blockchain/blockchain.rs @@ -1933,17 +1933,33 @@ mod tests { value: 103.into(), data: "601080600c6000396000f3006000355415600957005b60203560003555".from_hex().unwrap(), }.sign(&secret(), None); + let t4 = Transaction { + nonce: 0.into(), + gas_price: 0.into(), + gas: 100_000.into(), + action: Action::Create, + value: 104.into(), + data: "601080600c6000396000f3006000355415600957005b60203560003555".from_hex().unwrap(), + }.sign(&secret(), None); let tx_hash1 = t1.hash(); let tx_hash2 = t2.hash(); let tx_hash3 = t3.hash(); + let tx_hash4 = t4.hash(); let genesis = BlockBuilder::genesis(); let b1 = genesis.add_block_with_transactions(vec![t1, t2]); let b2 = b1.add_block_with_transactions(iter::once(t3)); + let b3 = genesis.add_block_with(|| BlockOptions { + transactions: vec![t4.clone()], + difficulty: U256::from(9), + ..Default::default() + }); // Branch block let b1_hash = b1.last().hash(); let b1_number = b1.last().number(); let b2_hash = b2.last().hash(); let b2_number = b2.last().number(); + let b3_hash = b3.last().hash(); + let b3_number = b3.last().number(); let db = new_db(); let bc = new_chain(&genesis.last().encoded(), db.clone()); @@ -1974,10 +1990,21 @@ mod tests { ], } ]); + insert_block(&db, &bc, &b3.last().encoded(), vec![ + Receipt { + outcome: TransactionOutcome::StateRoot(H256::default()), + gas_used: 10_000.into(), + log_bloom: Default::default(), + logs: vec![ + LogEntry { address: Default::default(), topics: vec![], data: vec![5], }, + ], + } + ]); // when let logs1 = bc.logs(vec![b1_hash, b2_hash], |_| true, None); let logs2 = bc.logs(vec![b1_hash, b2_hash], |_| true, Some(1)); + let logs3 = bc.logs(vec![b3_hash], |_| true, None); // then assert_eq!(logs1, vec![ @@ -2029,6 +2056,17 @@ mod tests { log_index: 0, } ]); + assert_eq!(logs3, vec![ + LocalizedLogEntry { + entry: LogEntry { address: Default::default(), topics: vec![], data: vec![5] }, + block_hash: b3_hash, + block_number: b3_number, + transaction_hash: tx_hash4, + transaction_index: 0, + transaction_log_index: 0, + log_index: 0, + } + ]); } #[test] From 20c556738839bd6f5e44d0d048a570935e459ee3 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 23 Apr 2018 22:21:18 +0800 Subject: [PATCH 04/12] Add docs that blocks must already be sorted --- ethcore/src/blockchain/blockchain.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ethcore/src/blockchain/blockchain.rs b/ethcore/src/blockchain/blockchain.rs index 5ba3f5d599a..5145d51c085 100644 --- a/ethcore/src/blockchain/blockchain.rs +++ b/ethcore/src/blockchain/blockchain.rs @@ -332,6 +332,8 @@ impl BlockProvider for BlockChain { .collect() } + /// Returns logs matching given filter. The order of logs returned will be the same as the order of the blocks + /// provided. And it's the callers responsibility to sort blocks provided in advance. fn logs(&self, mut blocks: Vec, matches: F, limit: Option) -> Vec where F: Fn(&LogEntry) -> bool + Send + Sync, Self: Sized { // sort in reverse order From 63df877ebab29427a3a1ea065022cec921e4fde3 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 23 Apr 2018 23:45:03 +0800 Subject: [PATCH 05/12] Handle branch block cases properly --- ethcore/src/client/client.rs | 114 +++++++++++++++++++++++++++-------- 1 file changed, 88 insertions(+), 26 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 4a38a1dd8e2..29bd45e6568 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1853,33 +1853,95 @@ impl BlockChainClient for Client { } fn logs(&self, filter: Filter) -> Vec { - let blocks = match &filter.from_block { - &BlockId::Hash(hash) if filter.from_block == filter.to_block => { - // Skip bloom check and directly use the hash. - vec![hash] - }, - _ => { - let (from, to) = match (self.block_number_ref(&filter.from_block), self.block_number_ref(&filter.to_block)) { - (Some(from), Some(to)) => (from, to), - _ => return Vec::new(), - }; + macro_rules! return_empty_if_none { + ( $v: expr ) => ( + match $v { + Some(value) => value, + None => return Vec::new(), + } + ) + } - let chain = self.chain.read(); - let mut numbers = filter.bloom_possibilities().iter() - .map(|bloom| { - chain.blocks_with_bloom(bloom, from, to) - }) - .flat_map(|m| m) - // remove duplicate elements - .collect::>() - .into_iter() - .collect::>(); - - numbers.sort_by(|a, b| a.cmp(b)); - numbers.into_iter() - .filter_map(|n| chain.block_hash(n)) - .collect::>() - }, + let chain = self.chain.read(); + + // First, check whether `filter.from_block` and `filter.to_block` is on the canon chain. If so, we can use the + // optimized version. + let is_canon = |id| { + match id { + // If it is referred by number, then it is always on the canon chain. + &BlockId::Earliest | &BlockId::Latest | &BlockId::Number(_) => Some(true), + // If it is referred by hash, we see whether a hash -> number -> hash conversion gives us the same + // result. + &BlockId::Hash(hash) => + Some(hash == chain.block_hash(chain.block_number(&hash)?)?), + } + }; + + let blocks = if return_empty_if_none!(is_canon(&filter.from_block)) && return_empty_if_none!(is_canon(&filter.to_block)) { + // If we are on the canon chain, use bloom filter to fetch required hashes. + let from = return_empty_if_none!(self.block_number_ref(&filter.from_block)); + let to = return_empty_if_none!(self.block_number_ref(&filter.to_block)); + + let mut numbers = filter.bloom_possibilities().iter() + .map(|bloom| { + chain.blocks_with_bloom(bloom, from, to) + }) + .flat_map(|m| m) + // remove duplicate elements + .collect::>() + .into_iter() + .collect::>(); + + numbers.sort_by(|a, b| a.cmp(b)); + numbers.into_iter() + .filter_map(|n| chain.block_hash(n)) + .collect::>() + + } else { + // Otherwise, we use a slower version that finds a link between from_block and to_block. + let get_hash = |id| { + match id { + &BlockId::Earliest | &BlockId::Latest | &BlockId::Number(_) => + chain.block_hash(self.block_number_ref(id)?), + &BlockId::Hash(hash) => Some(hash), + } + }; + + let from_hash = return_empty_if_none!(get_hash(&filter.from_block)); + let from_number = return_empty_if_none!(chain.block_number(&from_hash)); + let to_hash = return_empty_if_none!(get_hash(&filter.to_block)); + let to_number = return_empty_if_none!(chain.block_number(&to_hash)); + + let blooms = filter.bloom_possibilities(); + let bloom_match = |header: &encoded::Header| { + blooms.iter().any(|bloom| header.log_bloom().contains_bloom(bloom)) + }; + + let mut blocks = Vec::new(); + let mut current_hash = to_hash; + let mut current_number = to_number; + + if bloom_match(&return_empty_if_none!(chain.block_header_data(¤t_hash))) { + blocks.push(current_hash); + } + + while current_number > from_number { + let header = return_empty_if_none!(chain.block_header_data(¤t_hash)); + + if bloom_match(&header) { + blocks.push(current_hash); + } + + current_hash = header.parent_hash(); + current_number = current_number - 1; + } + + if current_hash != from_hash || blocks.empty() { + return Vec::new(); + } + + blocks.reverse(); + blocks }; self.chain.read().logs(blocks, |entry| filter.matches(entry), filter.limit) From 1f99d7798120d489bd17f089a79039fd3ae18417 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 23 Apr 2018 23:48:02 +0800 Subject: [PATCH 06/12] typo: empty -> is_empty --- ethcore/src/client/client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 29bd45e6568..3d7d13b0c16 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1936,7 +1936,7 @@ impl BlockChainClient for Client { current_number = current_number - 1; } - if current_hash != from_hash || blocks.empty() { + if current_hash != from_hash || blocks.is_empty() { return Vec::new(); } From 27d294ebf1ff4bdbfb67df0a8671342f01621299 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 24 Apr 2018 00:10:18 +0800 Subject: [PATCH 07/12] Remove return_empty_if_none by using a closure --- ethcore/src/client/client.rs | 146 +++++++++++++++++------------------ 1 file changed, 71 insertions(+), 75 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 3d7d13b0c16..0ecd847b40d 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1853,98 +1853,94 @@ impl BlockChainClient for Client { } fn logs(&self, filter: Filter) -> Vec { - macro_rules! return_empty_if_none { - ( $v: expr ) => ( - match $v { - Some(value) => value, - None => return Vec::new(), - } - ) - } - - let chain = self.chain.read(); - - // First, check whether `filter.from_block` and `filter.to_block` is on the canon chain. If so, we can use the - // optimized version. - let is_canon = |id| { - match id { - // If it is referred by number, then it is always on the canon chain. - &BlockId::Earliest | &BlockId::Latest | &BlockId::Number(_) => Some(true), - // If it is referred by hash, we see whether a hash -> number -> hash conversion gives us the same - // result. - &BlockId::Hash(hash) => - Some(hash == chain.block_hash(chain.block_number(&hash)?)?), - } - }; - - let blocks = if return_empty_if_none!(is_canon(&filter.from_block)) && return_empty_if_none!(is_canon(&filter.to_block)) { - // If we are on the canon chain, use bloom filter to fetch required hashes. - let from = return_empty_if_none!(self.block_number_ref(&filter.from_block)); - let to = return_empty_if_none!(self.block_number_ref(&filter.to_block)); - - let mut numbers = filter.bloom_possibilities().iter() - .map(|bloom| { - chain.blocks_with_bloom(bloom, from, to) - }) - .flat_map(|m| m) - // remove duplicate elements - .collect::>() - .into_iter() - .collect::>(); - - numbers.sort_by(|a, b| a.cmp(b)); - numbers.into_iter() - .filter_map(|n| chain.block_hash(n)) - .collect::>() + // Wrap the logic inside a closure so that we can take advantage of question mark syntax. + let fetch_logs = || { + let chain = self.chain.read(); - } else { - // Otherwise, we use a slower version that finds a link between from_block and to_block. - let get_hash = |id| { + // First, check whether `filter.from_block` and `filter.to_block` is on the canon chain. If so, we can use the + // optimized version. + let is_canon = |id| { match id { - &BlockId::Earliest | &BlockId::Latest | &BlockId::Number(_) => - chain.block_hash(self.block_number_ref(id)?), - &BlockId::Hash(hash) => Some(hash), + // If it is referred by number, then it is always on the canon chain. + &BlockId::Earliest | &BlockId::Latest | &BlockId::Number(_) => Some(true), + // If it is referred by hash, we see whether a hash -> number -> hash conversion gives us the same + // result. + &BlockId::Hash(hash) => + Some(hash == chain.block_hash(chain.block_number(&hash)?)?), } }; - let from_hash = return_empty_if_none!(get_hash(&filter.from_block)); - let from_number = return_empty_if_none!(chain.block_number(&from_hash)); - let to_hash = return_empty_if_none!(get_hash(&filter.to_block)); - let to_number = return_empty_if_none!(chain.block_number(&to_hash)); + let blocks = if is_canon(&filter.from_block)? && is_canon(&filter.to_block)? { + // If we are on the canon chain, use bloom filter to fetch required hashes. + let from = self.block_number_ref(&filter.from_block)?; + let to = self.block_number_ref(&filter.to_block)?; - let blooms = filter.bloom_possibilities(); - let bloom_match = |header: &encoded::Header| { - blooms.iter().any(|bloom| header.log_bloom().contains_bloom(bloom)) - }; + let mut numbers = filter.bloom_possibilities().iter() + .map(|bloom| { + chain.blocks_with_bloom(bloom, from, to) + }) + .flat_map(|m| m) + // remove duplicate elements + .collect::>() + .into_iter() + .collect::>(); - let mut blocks = Vec::new(); - let mut current_hash = to_hash; - let mut current_number = to_number; + numbers.sort_by(|a, b| a.cmp(b)); + numbers.into_iter() + .filter_map(|n| chain.block_hash(n)) + .collect::>() - if bloom_match(&return_empty_if_none!(chain.block_header_data(¤t_hash))) { - blocks.push(current_hash); - } + } else { + // Otherwise, we use a slower version that finds a link between from_block and to_block. + let get_hash = |id| { + match id { + &BlockId::Earliest | &BlockId::Latest | &BlockId::Number(_) => + chain.block_hash(self.block_number_ref(id)?), + &BlockId::Hash(hash) => Some(hash), + } + }; - while current_number > from_number { - let header = return_empty_if_none!(chain.block_header_data(¤t_hash)); + let from_hash = get_hash(&filter.from_block)?; + let from_number = chain.block_number(&from_hash)?; + let to_hash = get_hash(&filter.to_block)?; + let to_number = chain.block_number(&to_hash)?; + + let blooms = filter.bloom_possibilities(); + let bloom_match = |header: &encoded::Header| { + blooms.iter().any(|bloom| header.log_bloom().contains_bloom(bloom)) + }; - if bloom_match(&header) { + let mut blocks = Vec::new(); + let mut current_hash = to_hash; + let mut current_number = to_number; + + if bloom_match(&chain.block_header_data(¤t_hash)?) { blocks.push(current_hash); } - current_hash = header.parent_hash(); - current_number = current_number - 1; - } + while current_number > from_number { + let header = chain.block_header_data(¤t_hash)?; - if current_hash != from_hash || blocks.is_empty() { - return Vec::new(); - } + if bloom_match(&header) { + blocks.push(current_hash); + } + + current_hash = header.parent_hash(); + current_number = current_number - 1; + } + + if current_hash != from_hash || blocks.is_empty() { + return None; + } + + blocks.reverse(); + blocks + }; - blocks.reverse(); - blocks + Some(self.chain.read().logs(blocks, |entry| filter.matches(entry), filter.limit)) }; - self.chain.read().logs(blocks, |entry| filter.matches(entry), filter.limit) + fetch_logs().unwrap_or_default() } fn filter_traces(&self, filter: TraceFilter) -> Option> { From bd27639e405c40044622e41174de364029cc3898 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 24 Apr 2018 00:16:32 +0800 Subject: [PATCH 08/12] Use BTreeSet to avoid sorting again --- ethcore/src/client/client.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 0ecd847b40d..9ade62e303f 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Parity. If not, see . -use std::collections::{HashSet, HashMap, BTreeMap, VecDeque}; +use std::collections::{HashSet, HashMap, BTreeMap, BTreeSet, VecDeque}; use std::str::FromStr; use std::sync::{Arc, Weak}; use std::sync::atomic::{AtomicUsize, AtomicBool, Ordering as AtomicOrdering}; @@ -1875,18 +1875,14 @@ impl BlockChainClient for Client { let from = self.block_number_ref(&filter.from_block)?; let to = self.block_number_ref(&filter.to_block)?; - let mut numbers = filter.bloom_possibilities().iter() + filter.bloom_possibilities().iter() .map(|bloom| { chain.blocks_with_bloom(bloom, from, to) }) .flat_map(|m| m) // remove duplicate elements - .collect::>() + .collect::>() .into_iter() - .collect::>(); - - numbers.sort_by(|a, b| a.cmp(b)); - numbers.into_iter() .filter_map(|n| chain.block_hash(n)) .collect::>() From a7dd897aa33b42aaa94bd587443ec492fe6aa774 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 24 Apr 2018 00:26:14 +0800 Subject: [PATCH 09/12] Move is_canon to BlockChain --- ethcore/src/blockchain/blockchain.rs | 6 ++++++ ethcore/src/client/client.rs | 7 +++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/ethcore/src/blockchain/blockchain.rs b/ethcore/src/blockchain/blockchain.rs index 5145d51c085..57bcdf2bc28 100644 --- a/ethcore/src/blockchain/blockchain.rs +++ b/ethcore/src/blockchain/blockchain.rs @@ -57,6 +57,12 @@ pub trait BlockProvider { /// (though not necessarily a part of the canon chain). fn is_known(&self, hash: &H256) -> bool; + /// Returns true if the given block is known and in the canon chain. + fn is_canon(&self, hash: &H256) -> bool { + let is_canon = || Some(hash == &self.block_hash(self.block_number(hash)?)?); + is_canon().unwrap_or(false) + } + /// Get the first block of the best part of the chain. /// Return `None` if there is no gap and the first block is the genesis. /// Any queries of blocks which precede this one are not guaranteed to diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 9ade62e303f..958bb64880c 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1862,15 +1862,14 @@ impl BlockChainClient for Client { let is_canon = |id| { match id { // If it is referred by number, then it is always on the canon chain. - &BlockId::Earliest | &BlockId::Latest | &BlockId::Number(_) => Some(true), + &BlockId::Earliest | &BlockId::Latest | &BlockId::Number(_) => true, // If it is referred by hash, we see whether a hash -> number -> hash conversion gives us the same // result. - &BlockId::Hash(hash) => - Some(hash == chain.block_hash(chain.block_number(&hash)?)?), + &BlockId::Hash(hash) => chain.is_canon(hash), } }; - let blocks = if is_canon(&filter.from_block)? && is_canon(&filter.to_block)? { + let blocks = if is_canon(&filter.from_block) && is_canon(&filter.to_block) { // If we are on the canon chain, use bloom filter to fetch required hashes. let from = self.block_number_ref(&filter.from_block)?; let to = self.block_number_ref(&filter.to_block)?; From a9e49777009efff0cea902bd60b029edf34e77ee Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 24 Apr 2018 00:26:38 +0800 Subject: [PATCH 10/12] typo: pass value by reference --- ethcore/src/client/client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 958bb64880c..d917a60bbda 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1865,7 +1865,7 @@ impl BlockChainClient for Client { &BlockId::Earliest | &BlockId::Latest | &BlockId::Number(_) => true, // If it is referred by hash, we see whether a hash -> number -> hash conversion gives us the same // result. - &BlockId::Hash(hash) => chain.is_canon(hash), + &BlockId::Hash(ref hash) => chain.is_canon(hash), } }; From a6407eccf84947763532b058ebd4428bf847424b Mon Sep 17 00:00:00 2001 From: tomusdrw Date: Tue, 24 Apr 2018 00:38:11 +0800 Subject: [PATCH 11/12] Use loop and wrap inside blocks to simplify the code Borrowed from https://github.com/paritytech/parity/pull/8463#discussion_r183453326 --- ethcore/src/client/client.rs | 46 +++++++++++++++--------------------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index d917a60bbda..c75a02bd001 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1887,48 +1887,40 @@ impl BlockChainClient for Client { } else { // Otherwise, we use a slower version that finds a link between from_block and to_block. - let get_hash = |id| { - match id { - &BlockId::Earliest | &BlockId::Latest | &BlockId::Number(_) => - chain.block_hash(self.block_number_ref(id)?), - &BlockId::Hash(hash) => Some(hash), - } - }; - - let from_hash = get_hash(&filter.from_block)?; + let from_hash = Self::block_hash(&chain, filter.from_block)?; let from_number = chain.block_number(&from_hash)?; - let to_hash = get_hash(&filter.to_block)?; - let to_number = chain.block_number(&to_hash)?; + let to_hash = Self::block_hash(&chain, filter.from_block)?; let blooms = filter.bloom_possibilities(); let bloom_match = |header: &encoded::Header| { blooms.iter().any(|bloom| header.log_bloom().contains_bloom(bloom)) }; - let mut blocks = Vec::new(); - let mut current_hash = to_hash; - let mut current_number = to_number; - - if bloom_match(&chain.block_header_data(¤t_hash)?) { - blocks.push(current_hash); - } + let (blocks, last_hash) = { + let mut blocks = Vec::new(); + let mut current_hash = to_hash; - while current_number > from_number { - let header = chain.block_header_data(¤t_hash)?; + loop { + let header = chain.block_header_data(¤t_hash)?; + if bloom_match(&header) { + blocks.push(current_hash); + } - if bloom_match(&header) { - blocks.push(current_hash); + if header.number() <= from_number { + break; + } + current_hash = header.parent_hash(); } - current_hash = header.parent_hash(); - current_number = current_number - 1; - } + blocks.reverse(); + (blocks, current_hash) + }; - if current_hash != from_hash || blocks.is_empty() { + // Check if we've actually reached the expected `from` block. + if last_hash != from_hash || blocks.is_empty() { return None; } - blocks.reverse(); blocks }; From 2963c3b0fd538ac61925706b017076a2a6998c76 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 24 Apr 2018 00:40:10 +0800 Subject: [PATCH 12/12] typo: missed a comment --- ethcore/src/client/client.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index c75a02bd001..bd6201a15f3 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1906,6 +1906,7 @@ impl BlockChainClient for Client { blocks.push(current_hash); } + // Stop if `from` block is reached. if header.number() <= from_number { break; }