From 69f2d0561dc80bfd8d6fee237d9c7d3394bacb91 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 31 Jul 2018 17:47:13 +0800 Subject: [PATCH 01/14] Implement EIP234 --- rpc/src/v1/impls/light/eth.rs | 2 +- rpc/src/v1/types/filter.rs | 17 +++++++++++++++-- rpc/src/v1/types/pubsub.rs | 3 +++ 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/rpc/src/v1/impls/light/eth.rs b/rpc/src/v1/impls/light/eth.rs index e88ac2dab35..a0a86275505 100644 --- a/rpc/src/v1/impls/light/eth.rs +++ b/rpc/src/v1/impls/light/eth.rs @@ -514,7 +514,7 @@ impl Eth for EthClient { let limit = filter.limit; Box::new(Filterable::logs(self, filter.into()) - .map(move|logs| limit_logs(logs, limit))) + .map(move |logs| limit_logs(logs, limit))) } fn work(&self, _timeout: Trailing) -> Result { diff --git a/rpc/src/v1/types/filter.rs b/rpc/src/v1/types/filter.rs index dd8b823e879..fab03888799 100644 --- a/rpc/src/v1/types/filter.rs +++ b/rpc/src/v1/types/filter.rs @@ -62,6 +62,9 @@ pub struct Filter { /// To Block #[serde(rename="toBlock")] pub to_block: Option, + /// Block hash + #[serde(rename="blockHash")] + pub block_hash: Option, /// Address pub address: Option, /// Topics @@ -78,9 +81,17 @@ impl Into for Filter { BlockNumber::Latest | BlockNumber::Pending => BlockId::Latest, }; + let (from_block, to_block) = match self.block_hash { + Some(hash) => { + let hash = hash.into(); + (BlockId::Hash(hash), BlockId::Hash(hash)) + }, + None => (self.from_block.map_or_else(|| BlockId::Latest, &num_to_id), + self.to_block.map_or_else(|| BlockId::Latest, &num_to_id)), + }; + EthFilter { - from_block: self.from_block.map_or_else(|| BlockId::Latest, &num_to_id), - to_block: self.to_block.map_or_else(|| BlockId::Latest, &num_to_id), + from_block, to_block, address: self.address.and_then(|address| match address { VariadicValue::Null => None, VariadicValue::Single(a) => Some(vec![a.into()]), @@ -157,6 +168,7 @@ mod tests { assert_eq!(deserialized, Filter { from_block: Some(BlockNumber::Earliest), to_block: Some(BlockNumber::Latest), + block_hash: None, address: None, topics: None, limit: None, @@ -168,6 +180,7 @@ mod tests { let filter = Filter { from_block: Some(BlockNumber::Earliest), to_block: Some(BlockNumber::Latest), + block_hash: None, address: Some(VariadicValue::Multiple(vec![])), topics: Some(vec![ VariadicValue::Null, diff --git a/rpc/src/v1/types/pubsub.rs b/rpc/src/v1/types/pubsub.rs index ea01d6427bd..db4af4e8712 100644 --- a/rpc/src/v1/types/pubsub.rs +++ b/rpc/src/v1/types/pubsub.rs @@ -119,6 +119,7 @@ mod tests { assert_eq!(logs1, Params::Logs(Filter { from_block: None, to_block: None, + block_hash: None, address: None, topics: None, limit: None, @@ -126,6 +127,7 @@ mod tests { assert_eq!(logs2, Params::Logs(Filter { from_block: None, to_block: None, + block_hash: None, address: None, topics: None, limit: Some(10), @@ -133,6 +135,7 @@ mod tests { assert_eq!(logs3, Params::Logs(Filter { from_block: None, to_block: None, + block_hash: None, address: None, topics: Some(vec![ VariadicValue::Single("000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b".parse().unwrap() From 5a13b56973aa8c388159e90a571039da971197b7 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Wed, 1 Aug 2018 11:51:51 +0800 Subject: [PATCH 02/14] Make filter conversion returns error if both blockHash and from/toBlock is found This also changes PollFilter to store the EthFilter type, instead of the jsonrpc one, saving repeated conversion. --- rpc/src/v1/helpers/poll_filter.rs | 11 +++++++++-- rpc/src/v1/impls/eth.rs | 5 ++++- rpc/src/v1/impls/eth_filter.rs | 32 ++++++++++++++++++------------- rpc/src/v1/impls/eth_pubsub.rs | 9 +++++++-- rpc/src/v1/impls/light/eth.rs | 7 +++++-- rpc/src/v1/types/filter.rs | 16 +++++++++++----- 6 files changed, 55 insertions(+), 25 deletions(-) diff --git a/rpc/src/v1/helpers/poll_filter.rs b/rpc/src/v1/helpers/poll_filter.rs index 19979c814b8..48df7ca2a3c 100644 --- a/rpc/src/v1/helpers/poll_filter.rs +++ b/rpc/src/v1/helpers/poll_filter.rs @@ -22,7 +22,8 @@ use std::{ }; use ethereum_types::H256; use parking_lot::Mutex; -use v1::types::{Filter, Log}; +use ethcore::filter::Filter; +use v1::types::Log; pub type BlockNumber = u64; @@ -52,7 +53,13 @@ pub enum PollFilter { /// Hashes of all pending transactions the client knows about. PendingTransaction(BTreeSet), /// Number of From block number, last seen block hash, pending logs and log filter itself. - Logs(BlockNumber, Option, HashSet, Filter) + Logs { + block_number: BlockNumber, + last_block_hash: Option, + previous_logs: HashSet, + filter: Filter, + include_pending: bool, + } } /// Returns only last `n` logs diff --git a/rpc/src/v1/impls/eth.rs b/rpc/src/v1/impls/eth.rs index d20b2feb267..142f8a92115 100644 --- a/rpc/src/v1/impls/eth.rs +++ b/rpc/src/v1/impls/eth.rs @@ -722,7 +722,10 @@ impl Eth for EthClient< fn logs(&self, filter: Filter) -> BoxFuture> { let include_pending = filter.to_block == Some(BlockNumber::Pending); - let filter: EthcoreFilter = filter.into(); + let filter: EthcoreFilter = match filter.try_into() { + Ok(value) => value, + Err(err) => return Box::new(future::err(err)), + }; let mut logs = self.client.logs(filter.clone()) .into_iter() .map(From::from) diff --git a/rpc/src/v1/impls/eth_filter.rs b/rpc/src/v1/impls/eth_filter.rs index 926439cfc91..eb4b565df34 100644 --- a/rpc/src/v1/impls/eth_filter.rs +++ b/rpc/src/v1/impls/eth_filter.rs @@ -140,7 +140,13 @@ impl EthFilter for T { fn new_filter(&self, filter: Filter) -> Result { let mut polls = self.polls().lock(); let block_number = self.best_block_number(); - let id = polls.create_poll(SyncPollFilter::new(PollFilter::Logs(block_number, None, Default::default(), filter))); + let include_pending = filter.to_block == Some(BlockNumber::Pending); + let filter = filter.try_into()?; + let id = polls.create_poll(SyncPollFilter::new(PollFilter::Logs { + block_number, filter, include_pending, + last_block_hash: None, + previous_logs: Default::default() + })); Ok(id.into()) } @@ -195,15 +201,17 @@ impl EthFilter for T { // return new hashes Either::A(future::ok(FilterChanges::Hashes(new_hashes))) }, - PollFilter::Logs(ref mut block_number, ref mut last_block_hash, ref mut previous_logs, ref filter) => { + PollFilter::Logs { + ref mut block_number, + ref mut last_block_hash, + ref mut previous_logs, + ref filter, + include_pending, + } => { // retrive the current block number let current_number = self.best_block_number(); - // check if we need to check pending hashes - let include_pending = filter.to_block == Some(BlockNumber::Pending); - - // build appropriate filter - let mut filter: EthcoreFilter = filter.clone().into(); + let mut filter = filter.clone(); // retrieve reorg logs let (mut reorg, reorg_len) = last_block_hash.map_or_else(|| (Vec::new(), 0), |h| self.removed_logs(h, &filter)); @@ -250,21 +258,19 @@ impl EthFilter for T { } fn filter_logs(&self, index: Index) -> BoxFuture> { - let filter = { + let (filter, include_pending) = { let mut polls = self.polls().lock(); match polls.poll(&index.value()).and_then(|f| f.modify(|filter| match *filter { - PollFilter::Logs(.., ref filter) => Some(filter.clone()), + PollFilter::Logs { ref filter, include_pending, .. } => + Some((filter.clone(), include_pending)), _ => None, })) { - Some(filter) => filter, + Some((filter, include_pending)) => (filter, include_pending), None => return Box::new(future::err(errors::filter_not_found())), } }; - let include_pending = filter.to_block == Some(BlockNumber::Pending); - let filter: EthcoreFilter = filter.into(); - // fetch pending logs. let pending = if include_pending { let best_block = self.best_block_number(); diff --git a/rpc/src/v1/impls/eth_pubsub.rs b/rpc/src/v1/impls/eth_pubsub.rs index 9f592b1fa79..79be75492b0 100644 --- a/rpc/src/v1/impls/eth_pubsub.rs +++ b/rpc/src/v1/impls/eth_pubsub.rs @@ -283,8 +283,13 @@ impl EthPubSub for EthPubSubClient { errors::invalid_params("newHeads", "Expected no parameters.") }, (pubsub::Kind::Logs, Some(pubsub::Params::Logs(filter))) => { - self.logs_subscribers.write().push(subscriber, filter.into()); - return; + match filter.try_into() { + Ok(filter) => { + self.logs_subscribers.write().push(subscriber, filter); + return; + }, + Err(err) => err, + } }, (pubsub::Kind::Logs, _) => { errors::invalid_params("logs", "Expected a filter object.") diff --git a/rpc/src/v1/impls/light/eth.rs b/rpc/src/v1/impls/light/eth.rs index a0a86275505..708db581feb 100644 --- a/rpc/src/v1/impls/light/eth.rs +++ b/rpc/src/v1/impls/light/eth.rs @@ -513,8 +513,11 @@ impl Eth for EthClient { fn logs(&self, filter: Filter) -> BoxFuture> { let limit = filter.limit; - Box::new(Filterable::logs(self, filter.into()) - .map(move |logs| limit_logs(logs, limit))) + Box::new( + Filterable::logs(self, match filter.try_into() { + Ok(value) => value, + Err(err) => return Box::new(future::err(err)), + }).map(move |logs| limit_logs(logs, limit))) } fn work(&self, _timeout: Trailing) -> Result { diff --git a/rpc/src/v1/types/filter.rs b/rpc/src/v1/types/filter.rs index fab03888799..c5f5e30bff1 100644 --- a/rpc/src/v1/types/filter.rs +++ b/rpc/src/v1/types/filter.rs @@ -17,9 +17,11 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer}; use serde::de::{Error, DeserializeOwned}; use serde_json::{Value, from_value}; +use jsonrpc_core::{Error as RpcError}; use ethcore::filter::Filter as EthFilter; use ethcore::client::BlockId; use v1::types::{BlockNumber, H160, H256, Log}; +use v1::helpers::errors::invalid_params; /// Variadic value #[derive(Debug, PartialEq, Eq, Clone, Hash)] @@ -73,8 +75,12 @@ pub struct Filter { pub limit: Option, } -impl Into for Filter { - fn into(self) -> EthFilter { +impl Filter { + pub fn try_into(self) -> Result { + if self.block_hash.is_some() && (self.from_block.is_some() || self.to_block.is_some()) { + return Err(invalid_params("blockHash", "blockHash is mutually exclusive with fromBlock/toBlock")); + } + let num_to_id = |num| match num { BlockNumber::Num(n) => BlockId::Number(n), BlockNumber::Earliest => BlockId::Earliest, @@ -90,7 +96,7 @@ impl Into for Filter { self.to_block.map_or_else(|| BlockId::Latest, &num_to_id)), }; - EthFilter { + Ok(EthFilter { from_block, to_block, address: self.address.and_then(|address| match address { VariadicValue::Null => None, @@ -112,7 +118,7 @@ impl Into for Filter { ] }, limit: self.limit, - } + }) } } @@ -190,7 +196,7 @@ mod tests { limit: None, }; - let eth_filter: EthFilter = filter.into(); + let eth_filter: EthFilter = filter.try_into().unwrap(); assert_eq!(eth_filter, EthFilter { from_block: BlockId::Earliest, to_block: BlockId::Latest, From 74f0e034cb2c2bbaef623d7b21baa502cdee48c6 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Wed, 1 Aug 2018 12:12:12 +0800 Subject: [PATCH 03/14] Return error if block filtering target is not found in eth_getLogs Use the old behavior (unwrap_or_default) for anywhere else. --- ethcore/src/client/client.rs | 109 ++++++++++++++---------------- ethcore/src/client/test_client.rs | 6 +- ethcore/src/client/traits.rs | 4 +- ethcore/src/tests/client.rs | 4 +- rpc/src/v1/helpers/errors.rs | 8 +++ rpc/src/v1/impls/eth.rs | 11 +-- rpc/src/v1/impls/eth_filter.rs | 4 +- rpc/src/v1/impls/eth_pubsub.rs | 4 +- updater/src/updater.rs | 3 +- 9 files changed, 80 insertions(+), 73 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 47974356f1f..73310bc5a10 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1812,76 +1812,71 @@ impl BlockChainClient for Client { self.engine.additional_params().into_iter().collect() } - fn logs(&self, filter: Filter) -> Vec { - // Wrap the logic inside a closure so that we can take advantage of question mark syntax. - let fetch_logs = || { - 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(_) => true, - // If it is referred by hash, we see whether a hash -> number -> hash conversion gives us the same - // result. - &BlockId::Hash(ref hash) => chain.is_canon(hash), - } - }; + fn logs(&self, filter: Filter) -> Option> { + let chain = self.chain.read(); - 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)?; + // 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(_) => true, + // If it is referred by hash, we see whether a hash -> number -> hash conversion gives us the same + // result. + &BlockId::Hash(ref hash) => chain.is_canon(hash), + } + }; - chain.blocks_with_bloom(&filter.bloom_possibilities(), from, to) - .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 from_hash = Self::block_hash(&chain, filter.from_block)?; - let from_number = chain.block_number(&from_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 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 (blocks, last_hash) = { - let mut blocks = Vec::new(); - let mut current_hash = to_hash; + chain.blocks_with_bloom(&filter.bloom_possibilities(), from, to) + .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 from_hash = Self::block_hash(&chain, filter.from_block)?; + let from_number = chain.block_number(&from_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)) + }; - loop { - let header = chain.block_header_data(¤t_hash)?; - if bloom_match(&header) { - blocks.push(current_hash); - } + let (blocks, last_hash) = { + let mut blocks = Vec::new(); + let mut current_hash = to_hash; - // Stop if `from` block is reached. - if header.number() <= from_number { - break; - } - current_hash = header.parent_hash(); + loop { + let header = chain.block_header_data(¤t_hash)?; + if bloom_match(&header) { + blocks.push(current_hash); } - blocks.reverse(); - (blocks, current_hash) - }; - - // Check if we've actually reached the expected `from` block. - if last_hash != from_hash || blocks.is_empty() { - return None; + // Stop if `from` block is reached. + if header.number() <= from_number { + break; + } + current_hash = header.parent_hash(); } - blocks + blocks.reverse(); + (blocks, current_hash) }; - Some(self.chain.read().logs(blocks, |entry| filter.matches(entry), filter.limit)) + // Check if we've actually reached the expected `from` block. + if last_hash != from_hash || blocks.is_empty() { + return None; + } + + blocks }; - fetch_logs().unwrap_or_default() + Some(self.chain.read().logs(blocks, |entry| filter.matches(entry), filter.limit)) } fn filter_traces(&self, filter: TraceFilter) -> Option> { diff --git a/ethcore/src/client/test_client.rs b/ethcore/src/client/test_client.rs index 7a3dfcd0075..463495b04d5 100644 --- a/ethcore/src/client/test_client.rs +++ b/ethcore/src/client/test_client.rs @@ -663,13 +663,13 @@ impl BlockChainClient for TestBlockChainClient { self.receipts.read().get(&id).cloned() } - fn logs(&self, filter: Filter) -> Vec { + fn logs(&self, filter: Filter) -> Option> { let mut logs = self.logs.read().clone(); let len = logs.len(); - match filter.limit { + Some(match filter.limit { Some(limit) if limit <= len => logs.split_off(len - limit), _ => logs, - } + }) } fn last_hashes(&self) -> LastHashes { diff --git a/ethcore/src/client/traits.rs b/ethcore/src/client/traits.rs index 65bf0092118..01b2993909d 100644 --- a/ethcore/src/client/traits.rs +++ b/ethcore/src/client/traits.rs @@ -296,8 +296,8 @@ pub trait BlockChainClient : Sync + Send + AccountData + BlockChain + CallContra /// Get the registrar address, if it exists. fn additional_params(&self) -> BTreeMap; - /// Returns logs matching given filter. - fn logs(&self, filter: Filter) -> Vec; + /// Returns logs matching given filter. If one of the filtering block cannot be found, return None. + fn logs(&self, filter: Filter) -> Option>; /// Replays a given transaction for inspection. fn replay(&self, t: TransactionId, analytics: CallAnalytics) -> Result; diff --git a/ethcore/src/tests/client.rs b/ethcore/src/tests/client.rs index a7629313d17..6e68233c790 100644 --- a/ethcore/src/tests/client.rs +++ b/ethcore/src/tests/client.rs @@ -168,7 +168,7 @@ fn returns_logs() { address: None, topics: vec![], limit: None, - }); + }).unwrap(); assert_eq!(logs.len(), 0); } @@ -182,7 +182,7 @@ fn returns_logs_with_limit() { address: None, topics: vec![], limit: None, - }); + }).unwrap(); assert_eq!(logs.len(), 0); } diff --git a/rpc/src/v1/helpers/errors.rs b/rpc/src/v1/helpers/errors.rs index 710f7d7497a..6b6c7f5d25f 100644 --- a/rpc/src/v1/helpers/errors.rs +++ b/rpc/src/v1/helpers/errors.rs @@ -422,6 +422,14 @@ pub fn filter_not_found() -> Error { } } +pub fn filter_block_not_found() -> Error { + Error { + code: ErrorCode::ServerError(codes::UNSUPPORTED_REQUEST), + message: "One of the block specified in filter (fromBlock, toBlock or blockHash) cannot be found".into(), + data: None, + } +} + // on-demand sender cancelled. pub fn on_demand_cancel(_cancel: futures::sync::oneshot::Canceled) -> Error { internal("on-demand sender cancelled", "") diff --git a/rpc/src/v1/impls/eth.rs b/rpc/src/v1/impls/eth.rs index 142f8a92115..e4a76582991 100644 --- a/rpc/src/v1/impls/eth.rs +++ b/rpc/src/v1/impls/eth.rs @@ -726,10 +726,13 @@ impl Eth for EthClient< Ok(value) => value, Err(err) => return Box::new(future::err(err)), }; - let mut logs = self.client.logs(filter.clone()) - .into_iter() - .map(From::from) - .collect::>(); + let mut logs = match self.client.logs(filter.clone()) { + Some(logs) => logs + .into_iter() + .map(From::from) + .collect::>(), + None => return Box::new(future::err(errors::filter_block_not_found())), + }; if include_pending { let best_block = self.client.chain_info().best_block_number; diff --git a/rpc/src/v1/impls/eth_filter.rs b/rpc/src/v1/impls/eth_filter.rs index eb4b565df34..7bccc46d105 100644 --- a/rpc/src/v1/impls/eth_filter.rs +++ b/rpc/src/v1/impls/eth_filter.rs @@ -92,7 +92,7 @@ impl Filterable for EthFilterClient where } fn logs(&self, filter: EthcoreFilter) -> BoxFuture> { - Box::new(future::ok(self.client.logs(filter).into_iter().map(Into::into).collect())) + Box::new(future::ok(self.client.logs(filter).unwrap_or_default().into_iter().map(Into::into).collect())) } fn pending_logs(&self, block_number: u64, filter: &EthcoreFilter) -> Vec { @@ -125,7 +125,7 @@ impl Filterable for EthFilterClient where filter.from_block = BlockId::Hash(block_hash); filter.to_block = filter.from_block; - self.client.logs(filter).into_iter().map(|log| { + self.client.logs(filter).unwrap_or_default().into_iter().map(|log| { let mut log: Log = log.into(); log.log_type = "removed".into(); log.removed = true; diff --git a/rpc/src/v1/impls/eth_pubsub.rs b/rpc/src/v1/impls/eth_pubsub.rs index 79be75492b0..7961b1d187f 100644 --- a/rpc/src/v1/impls/eth_pubsub.rs +++ b/rpc/src/v1/impls/eth_pubsub.rs @@ -252,9 +252,9 @@ impl ChainNotify for ChainNotificationHandler { self.notify_logs(route.route(), |filter, ex| { match ex { &ChainRouteType::Enacted => - Ok(self.client.logs(filter).into_iter().map(Into::into).collect()), + Ok(self.client.logs(filter).unwrap_or_default().into_iter().map(Into::into).collect()), &ChainRouteType::Retracted => - Ok(self.client.logs(filter).into_iter().map(Into::into).map(|mut log: Log| { + Ok(self.client.logs(filter).unwrap_or_default().into_iter().map(Into::into).map(|mut log: Log| { log.log_type = "removed".into(); log.removed = true; log diff --git a/updater/src/updater.rs b/updater/src/updater.rs index a3ed413c44a..9dad11e9ac4 100644 --- a/updater/src/updater.rs +++ b/updater/src/updater.rs @@ -314,6 +314,7 @@ impl OperationsClient for OperationsContractClient { }; client.logs(filter) + .unwrap_or_default() .iter() .filter_map(|log| { let event = event.parse_log((log.topics.clone(), log.data.clone()).into()).ok()?; @@ -618,7 +619,7 @@ impl Updater Date: Wed, 1 Aug 2018 13:29:07 +0800 Subject: [PATCH 04/14] fix test: secret_store --- secret_store/src/listener/service_contract.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/secret_store/src/listener/service_contract.rs b/secret_store/src/listener/service_contract.rs index daf70cd6488..e4d54e0dc1d 100644 --- a/secret_store/src/listener/service_contract.rs +++ b/secret_store/src/listener/service_contract.rs @@ -283,7 +283,7 @@ impl ServiceContract for OnChainServiceContract { address: Some(vec![address]), topics: vec![Some(mask_topics(&self.mask))], limit: None, - }); + }).unwrap_or_default(); Box::new(request_logs.into_iter() .filter_map(|log| { From 5db8fc326255340e9003d48bcc20540d95a32956 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Fri, 3 Aug 2018 15:27:30 +0800 Subject: [PATCH 05/14] Fix weird indentation --- rpc/src/v1/types/filter.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/rpc/src/v1/types/filter.rs b/rpc/src/v1/types/filter.rs index c5f5e30bff1..6d3e94c70a7 100644 --- a/rpc/src/v1/types/filter.rs +++ b/rpc/src/v1/types/filter.rs @@ -92,8 +92,9 @@ impl Filter { let hash = hash.into(); (BlockId::Hash(hash), BlockId::Hash(hash)) }, - None => (self.from_block.map_or_else(|| BlockId::Latest, &num_to_id), - self.to_block.map_or_else(|| BlockId::Latest, &num_to_id)), + None => + (self.from_block.map_or_else(|| BlockId::Latest, &num_to_id), + self.to_block.map_or_else(|| BlockId::Latest, &num_to_id)), }; Ok(EthFilter { From ca22582bd103a28436f122ef68a352d9078ca669 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 7 Aug 2018 00:26:28 +0800 Subject: [PATCH 06/14] Make client log filter return error in case a block cannot be found --- ethcore/src/client/client.rs | 37 +++++++++++++++++++++++-------- ethcore/src/client/test_client.rs | 4 ++-- ethcore/src/client/traits.rs | 4 ++-- 3 files changed, 32 insertions(+), 13 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index c4a2aced2f1..a6c4f7d5e64 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1813,7 +1813,7 @@ impl BlockChainClient for Client { self.engine.additional_params().into_iter().collect() } - fn logs(&self, filter: Filter) -> Option> { + fn logs(&self, filter: Filter) -> Result, BlockId> { 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 @@ -1830,8 +1830,14 @@ impl BlockChainClient for Client { 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 from = match self.block_number_ref(&filter.from_block) { + Some(val) => val, + None => return Err(filter.from_block.clone()), + }; + let to = match self.block_number_ref(&filter.to_block) { + Some(val) => val, + None => return Err(filter.to_block.clone()), + }; chain.blocks_with_bloom(&filter.bloom_possibilities(), from, to) .into_iter() @@ -1839,9 +1845,18 @@ impl BlockChainClient for Client { .collect::>() } else { // Otherwise, we use a slower version that finds a link between from_block and to_block. - let from_hash = Self::block_hash(&chain, filter.from_block)?; - let from_number = chain.block_number(&from_hash)?; - let to_hash = Self::block_hash(&chain, filter.to_block)?; + let from_hash = match Self::block_hash(&chain, filter.from_block) { + Some(val) => val, + None => return Err(filter.from_block.clone()), + }; + let from_number = match chain.block_number(&from_hash) { + Some(val) => val, + None => return Err(BlockId::Hash(from_hash)), + }; + let to_hash = match Self::block_hash(&chain, filter.to_block) { + Some(val) => val, + None => return Err(filter.to_block.clone()), + }; let blooms = filter.bloom_possibilities(); let bloom_match = |header: &encoded::Header| { @@ -1853,7 +1868,10 @@ impl BlockChainClient for Client { let mut current_hash = to_hash; loop { - let header = chain.block_header_data(¤t_hash)?; + let header = match chain.block_header_data(¤t_hash) { + Some(val) => val, + None => return Err(BlockId::Hash(current_hash)), + }; if bloom_match(&header) { blocks.push(current_hash); } @@ -1871,13 +1889,14 @@ impl BlockChainClient for Client { // Check if we've actually reached the expected `from` block. if last_hash != from_hash || blocks.is_empty() { - return None; + // In this case, from_hash is the cause (for not matching last_hash). + return Err(BlockId::Hash(from_hash)); } blocks }; - Some(self.chain.read().logs(blocks, |entry| filter.matches(entry), filter.limit)) + Ok(self.chain.read().logs(blocks, |entry| filter.matches(entry), filter.limit)) } fn filter_traces(&self, filter: TraceFilter) -> Option> { diff --git a/ethcore/src/client/test_client.rs b/ethcore/src/client/test_client.rs index 00e36233c38..7a368d8186f 100644 --- a/ethcore/src/client/test_client.rs +++ b/ethcore/src/client/test_client.rs @@ -665,10 +665,10 @@ impl BlockChainClient for TestBlockChainClient { self.receipts.read().get(&id).cloned() } - fn logs(&self, filter: Filter) -> Option> { + fn logs(&self, filter: Filter) -> Result, BlockId> { let mut logs = self.logs.read().clone(); let len = logs.len(); - Some(match filter.limit { + Ok(match filter.limit { Some(limit) if limit <= len => logs.split_off(len - limit), _ => logs, }) diff --git a/ethcore/src/client/traits.rs b/ethcore/src/client/traits.rs index d0c62a3ef0a..189ca67f48b 100644 --- a/ethcore/src/client/traits.rs +++ b/ethcore/src/client/traits.rs @@ -297,8 +297,8 @@ pub trait BlockChainClient : Sync + Send + AccountData + BlockChain + CallContra /// Get the registrar address, if it exists. fn additional_params(&self) -> BTreeMap; - /// Returns logs matching given filter. If one of the filtering block cannot be found, return None. - fn logs(&self, filter: Filter) -> Option>; + /// Returns logs matching given filter. If one of the filtering block cannot be found, returns the block id that caused the error. + fn logs(&self, filter: Filter) -> Result, BlockId>; /// Replays a given transaction for inspection. fn replay(&self, t: TransactionId, analytics: CallAnalytics) -> Result; From c7e3f2f9b38f932c88ff249402e9d9eb4687ef1c Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 7 Aug 2018 00:37:41 +0800 Subject: [PATCH 07/14] Return blockId error in rpc --- rpc/src/v1/helpers/errors.rs | 10 ++++++++-- rpc/src/v1/impls/eth.rs | 4 ++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/rpc/src/v1/helpers/errors.rs b/rpc/src/v1/helpers/errors.rs index 6b6c7f5d25f..457d3ffbdbb 100644 --- a/rpc/src/v1/helpers/errors.rs +++ b/rpc/src/v1/helpers/errors.rs @@ -20,6 +20,7 @@ use std::fmt; use ethcore::account_provider::{SignError as AccountError}; use ethcore::error::{Error as EthcoreError, ErrorKind, CallError}; +use ethcore::client::BlockId; use jsonrpc_core::{futures, Error, ErrorCode, Value}; use rlp::DecoderError; use transaction::Error as TransactionError; @@ -422,11 +423,16 @@ pub fn filter_not_found() -> Error { } } -pub fn filter_block_not_found() -> Error { +pub fn filter_block_not_found(id: BlockId) -> Error { Error { code: ErrorCode::ServerError(codes::UNSUPPORTED_REQUEST), message: "One of the block specified in filter (fromBlock, toBlock or blockHash) cannot be found".into(), - data: None, + data: Some(Value::String(match id { + BlockId::Hash(hash) => format!("0x{:x}", hash), + BlockId::Number(number) => format!("0x{:x}", number), + BlockId::Earliest => "earliest".to_string(), + BlockId::Latest => "latest".to_string(), + })), } } diff --git a/rpc/src/v1/impls/eth.rs b/rpc/src/v1/impls/eth.rs index 47c93879031..4f34fd29f48 100644 --- a/rpc/src/v1/impls/eth.rs +++ b/rpc/src/v1/impls/eth.rs @@ -724,11 +724,11 @@ impl Eth for EthClient< Err(err) => return Box::new(future::err(err)), }; let mut logs = match self.client.logs(filter.clone()) { - Some(logs) => logs + Ok(logs) => logs .into_iter() .map(From::from) .collect::>(), - None => return Box::new(future::err(errors::filter_block_not_found())), + Err(id) => return Box::new(future::err(errors::filter_block_not_found(id))), }; if include_pending { From 9c9b6133f9a26412abf5090f24312656cdcecc9f Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 7 Aug 2018 00:45:58 +0800 Subject: [PATCH 08/14] test_client: allow return error on logs --- ethcore/src/client/test_client.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/ethcore/src/client/test_client.rs b/ethcore/src/client/test_client.rs index 7a368d8186f..9981f8d2cb7 100644 --- a/ethcore/src/client/test_client.rs +++ b/ethcore/src/client/test_client.rs @@ -94,6 +94,8 @@ pub struct TestBlockChainClient { pub receipts: RwLock>, /// Logs pub logs: RwLock>, + /// Should return errors on logs. + pub error_on_logs: RwLock>, /// Block queue size. pub queue_size: AtomicUsize, /// Miner @@ -178,6 +180,7 @@ impl TestBlockChainClient { traces: RwLock::new(None), history: RwLock::new(None), disabled: AtomicBool::new(false), + error_on_logs: RwLock::new(None), }; // insert genesis hash. @@ -233,6 +236,11 @@ impl TestBlockChainClient { *self.logs.write() = logs; } + /// Set return errors on logs. + pub fn set_error_on_logs(&self, val: Option) { + *self.error_on_logs.write() = val; + } + /// Add blocks to test client. pub fn add_blocks(&self, count: usize, with: EachBlockWith) { let len = self.numbers.read().len(); @@ -666,6 +674,11 @@ impl BlockChainClient for TestBlockChainClient { } fn logs(&self, filter: Filter) -> Result, BlockId> { + match self.error_on_logs.read().as_ref() { + Some(id) => return Err(id.clone()), + None => (), + } + let mut logs = self.logs.read().clone(); let len = logs.len(); Ok(match filter.limit { From 6f52ff98d36cb97c840d4c539eb60fe3896714b0 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 7 Aug 2018 00:56:29 +0800 Subject: [PATCH 09/14] Add a mocked test for eth_getLogs error --- rpc/src/v1/tests/mocked/eth.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/rpc/src/v1/tests/mocked/eth.rs b/rpc/src/v1/tests/mocked/eth.rs index 7213ad63da4..bd4e527a6bf 100644 --- a/rpc/src/v1/tests/mocked/eth.rs +++ b/rpc/src/v1/tests/mocked/eth.rs @@ -234,6 +234,15 @@ fn rpc_eth_logs() { assert_eq!(tester.io.handle_request_sync(request3), Some(response3.to_owned())); } +#[test] +fn rpc_eth_logs_error() { + let tester = EthTester::default(); + tester.client.set_error_on_logs(Some(BlockId::Hash(H256::from([5u8].as_ref())))); + let request = r#"{"jsonrpc": "2.0", "method": "eth_getLogs", "params": [{"limit":1,"blockHash":"0x0000000000000000000000000000000000000000000000000000000000000000"}], "id": 1}"#; + let response = r#"{"jsonrpc":"2.0","error":{"code":-32000,"message":"One of the block specified in filter (fromBlock, toBlock or blockHash) cannot be found","data":"0x0500000000000000000000000000000000000000000000000000000000000000"},"id":1}"#; + assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned())); +} + #[test] fn rpc_logs_filter() { let tester = EthTester::default(); From e16292ef18ccb6ae050f34e02fd33766ed12b687 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 7 Aug 2018 01:26:25 +0800 Subject: [PATCH 10/14] fix: should return error if from_block/to_block greater than best block number --- ethcore/src/client/client.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index a6c4f7d5e64..1eb7ed325bf 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1831,12 +1831,12 @@ impl BlockChainClient for Client { 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 = match self.block_number_ref(&filter.from_block) { - Some(val) => val, - None => return Err(filter.from_block.clone()), + Some(val) if val <= chain.best_block_number() => val, + _ => return Err(filter.from_block.clone()), }; let to = match self.block_number_ref(&filter.to_block) { - Some(val) => val, - None => return Err(filter.to_block.clone()), + Some(val) if val <= chain.best_block_number() => val, + _ => return Err(filter.to_block.clone()), }; chain.blocks_with_bloom(&filter.bloom_possibilities(), from, to) From be2e2e2f7f4206859bf30d98b42e44dc0651582f Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 7 Aug 2018 01:41:08 +0800 Subject: [PATCH 11/14] Add notes on pending --- ethcore/src/client/client.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 1eb7ed325bf..5ed92c14551 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1830,6 +1830,10 @@ impl BlockChainClient for Client { 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. + // + // If we are sure the block does not exist (where val > best_block_number), then return error. Note that we + // don't need to care about pending blocks here because RPC query sets pending back to latest (or handled + // pending logs themselves). let from = match self.block_number_ref(&filter.from_block) { Some(val) if val <= chain.best_block_number() => val, _ => return Err(filter.from_block.clone()), From 8bf6a954082fb66c8c5ddd76c97089b574735b6b Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 7 Aug 2018 17:16:01 +0800 Subject: [PATCH 12/14] Add comment for UNSUPPORTED_REQUEST --- rpc/src/v1/helpers/errors.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rpc/src/v1/helpers/errors.rs b/rpc/src/v1/helpers/errors.rs index 457d3ffbdbb..6bd8f8a52e7 100644 --- a/rpc/src/v1/helpers/errors.rs +++ b/rpc/src/v1/helpers/errors.rs @@ -425,7 +425,7 @@ pub fn filter_not_found() -> Error { pub fn filter_block_not_found(id: BlockId) -> Error { Error { - code: ErrorCode::ServerError(codes::UNSUPPORTED_REQUEST), + code: ErrorCode::ServerError(codes::UNSUPPORTED_REQUEST), // Specified in EIP-234. message: "One of the block specified in filter (fromBlock, toBlock or blockHash) cannot be found".into(), data: Some(Value::String(match id { BlockId::Hash(hash) => format!("0x{:x}", hash), From 4b45f9ac913f7ef9d7e3d3cda22a2d0c28dc23ea Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 7 Aug 2018 17:18:16 +0800 Subject: [PATCH 13/14] Address grumbles --- rpc/src/v1/helpers/errors.rs | 2 +- rpc/src/v1/tests/mocked/eth.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rpc/src/v1/helpers/errors.rs b/rpc/src/v1/helpers/errors.rs index 6bd8f8a52e7..4afd40ff843 100644 --- a/rpc/src/v1/helpers/errors.rs +++ b/rpc/src/v1/helpers/errors.rs @@ -426,7 +426,7 @@ pub fn filter_not_found() -> Error { pub fn filter_block_not_found(id: BlockId) -> Error { Error { code: ErrorCode::ServerError(codes::UNSUPPORTED_REQUEST), // Specified in EIP-234. - message: "One of the block specified in filter (fromBlock, toBlock or blockHash) cannot be found".into(), + message: "One of the blocks specified in filter (fromBlock, toBlock or blockHash) cannot be found".into(), data: Some(Value::String(match id { BlockId::Hash(hash) => format!("0x{:x}", hash), BlockId::Number(number) => format!("0x{:x}", number), diff --git a/rpc/src/v1/tests/mocked/eth.rs b/rpc/src/v1/tests/mocked/eth.rs index bd4e527a6bf..382b7e99426 100644 --- a/rpc/src/v1/tests/mocked/eth.rs +++ b/rpc/src/v1/tests/mocked/eth.rs @@ -239,7 +239,7 @@ fn rpc_eth_logs_error() { let tester = EthTester::default(); tester.client.set_error_on_logs(Some(BlockId::Hash(H256::from([5u8].as_ref())))); let request = r#"{"jsonrpc": "2.0", "method": "eth_getLogs", "params": [{"limit":1,"blockHash":"0x0000000000000000000000000000000000000000000000000000000000000000"}], "id": 1}"#; - let response = r#"{"jsonrpc":"2.0","error":{"code":-32000,"message":"One of the block specified in filter (fromBlock, toBlock or blockHash) cannot be found","data":"0x0500000000000000000000000000000000000000000000000000000000000000"},"id":1}"#; + let response = r#"{"jsonrpc":"2.0","error":{"code":-32000,"message":"One of the blocks specified in filter (fromBlock, toBlock or blockHash) cannot be found","data":"0x0500000000000000000000000000000000000000000000000000000000000000"},"id":1}"#; assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned())); } From 2ad31e5dabafcb1fdb379c51e8c38e0a7b522d2c Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Wed, 8 Aug 2018 01:11:40 +0800 Subject: [PATCH 14/14] Return err if from > to --- ethcore/src/client/client.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 5ed92c14551..84bcddfd6db 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1843,6 +1843,12 @@ impl BlockChainClient for Client { _ => return Err(filter.to_block.clone()), }; + // If from is greater than to, then the current bloom filter behavior is to just return empty + // result. There's no point to continue here. + if from > to { + return Err(filter.to_block.clone()); + } + chain.blocks_with_bloom(&filter.bloom_possibilities(), from, to) .into_iter() .filter_map(|n| chain.block_hash(n))