Implement EIP234 block_hash for eth_getLogs (#9256)

* Implement EIP234

* 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.

* Return error if block filtering target is not found in eth_getLogs

Use the old behavior (unwrap_or_default) for anywhere else.

* fix test: secret_store

* Fix weird indentation

* Make client log filter return error in case a block cannot be found

* Return blockId error in rpc

* test_client: allow return error on logs

* Add a mocked test for eth_getLogs error

* fix: should return error if from_block/to_block greater than best block number

* Add notes on pending

* Add comment for UNSUPPORTED_REQUEST

* Address grumbles

* Return err if from > to

ethcore/src/client/client.rs

@@ -1813,76 +1813,100 @@ impl BlockChainClient for Client {
 		self.engine.additional_params().into_iter().collect()
 	}
 
-	fn logs(&self, filter: Filter) -> Vec<LocalizedLogEntry> {
-		// Wrap the logic inside a closure so that we can take advantage of question mark syntax.
-		let fetch_logs = || {
-			let chain = self.chain.read();
+	fn logs(&self, filter: Filter) -> Result<Vec<LocalizedLogEntry>, 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
-			// 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),
-				}
-			};
-
-			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)?;
-
-				chain.blocks_with_bloom(&filter.bloom_possibilities(), from, to)
-					.into_iter()
-					.filter_map(|n| chain.block_hash(n))
-					.collect::<Vec<H256>>()
-			} 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 blooms = filter.bloom_possibilities();
-				let bloom_match = |header: &encoded::Header| {
-					blooms.iter().any(|bloom| header.log_bloom().contains_bloom(bloom))
-				};
-
-				let (blocks, last_hash) = {
-					let mut blocks = Vec::new();
-					let mut current_hash = to_hash;
-
-					loop {
-						let header = chain.block_header_data(&current_hash)?;
-						if bloom_match(&header) {
-							blocks.push(current_hash);
-						}
-
-						// Stop if `from` block is reached.
-						if header.number() <= from_number {
-							break;
-						}
-						current_hash = header.parent_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;
-				}
-
-				blocks
-			};
-
-			Some(self.chain.read().logs(blocks, |entry| filter.matches(entry), filter.limit))
+		// 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),
+			}
 		};
 
-		fetch_logs().unwrap_or_default()
+		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()),
+			};
+			let to = match self.block_number_ref(&filter.to_block) {
+				Some(val) if val <= chain.best_block_number() => val,
+				_ => 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))
+				.collect::<Vec<H256>>()
+		} else {
+			// Otherwise, we use a slower version that finds a link between from_block and 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| {
+				blooms.iter().any(|bloom| header.log_bloom().contains_bloom(bloom))
+			};
+
+			let (blocks, last_hash) = {
+				let mut blocks = Vec::new();
+				let mut current_hash = to_hash;
+
+				loop {
+					let header = match chain.block_header_data(&current_hash) {
+						Some(val) => val,
+						None => return Err(BlockId::Hash(current_hash)),
+					};
+					if bloom_match(&header) {
+						blocks.push(current_hash);
+					}
+
+					// Stop if `from` block is reached.
+					if header.number() <= from_number {
+						break;
+					}
+					current_hash = header.parent_hash();
+				}
+
+				blocks.reverse();
+				(blocks, current_hash)
+			};
+
+			// Check if we've actually reached the expected `from` block.
+			if last_hash != from_hash || blocks.is_empty() {
+				// In this case, from_hash is the cause (for not matching last_hash).
+				return Err(BlockId::Hash(from_hash));
+			}
+
+			blocks
+		};
+
+		Ok(self.chain.read().logs(blocks, |entry| filter.matches(entry), filter.limit))
 	}
 
 	fn filter_traces(&self, filter: TraceFilter) -> Option<Vec<LocalizedTrace>> {

ethcore/src/client/test_client.rs

@@ -94,6 +94,8 @@ pub struct TestBlockChainClient {
 	pub receipts: RwLock<HashMap<TransactionId, LocalizedReceipt>>,
 	/// Logs
 	pub logs: RwLock<Vec<LocalizedLogEntry>>,
+	/// Should return errors on logs.
+	pub error_on_logs: RwLock<Option<BlockId>>,
 	/// 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<BlockId>) {
+		*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();
@@ -665,13 +673,18 @@ impl BlockChainClient for TestBlockChainClient {
 		self.receipts.read().get(&id).cloned()
 	}
 
-	fn logs(&self, filter: Filter) -> Vec<LocalizedLogEntry> {
+	fn logs(&self, filter: Filter) -> Result<Vec<LocalizedLogEntry>, 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();
-		match filter.limit {
+		Ok(match filter.limit {
 			Some(limit) if limit <= len => logs.split_off(len - limit),
 			_ => logs,
-		}
+		})
 	}
 
 	fn last_hashes(&self) -> LastHashes {

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<String, String>;
 
-	/// Returns logs matching given filter.
-	fn logs(&self, filter: Filter) -> Vec<LocalizedLogEntry>;
+	/// 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<Vec<LocalizedLogEntry>, BlockId>;
 
 	/// Replays a given transaction for inspection.
 	fn replay(&self, t: TransactionId, analytics: CallAnalytics) -> Result<Executed, CallError>;

ethcore/src/tests/client.rs

@@ -150,7 +150,7 @@ fn returns_logs() {
 		address: None,
 		topics: vec![],
 		limit: None,
-	});
+	}).unwrap();
 	assert_eq!(logs.len(), 0);
 }
 
@@ -164,7 +164,7 @@ fn returns_logs_with_limit() {
 		address: None,
 		topics: vec![],
 		limit: None,
-	});
+	}).unwrap();
 	assert_eq!(logs.len(), 0);
 }