Implement EIP234 block_hash for eth_getLogs by sorpaas · Pull Request #9256 · openethereum/parity-ethereum

sorpaas · 2018-07-31T09:49:21Z

If block_hash and from_block/to_block present, return error. This also changes eth_getLogs to return error if any of block_hash/from_block/to_block cannot be found.

MicahZoltu · 2018-07-31T12:27:47Z

If the blockhash supplied doesn't match any known block, will it return the error specified in EIP-234?

fabioberger · 2018-07-31T12:32:05Z

Thanks for this @sorpaas!

The part of EIP 234 @MicahZoltu is referring to is:

If no block exists with that hash then an error should be returned with a code of -32000, a message of "Block not found." and a data of "0xbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0c"

Where 0xbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0cbl0c is substituted with the blockHash of the request.

tomusdrw · 2018-07-31T12:41:50Z

 			BlockNumber::Latest | BlockNumber::Pending => BlockId::Latest,
 		};

+		let (from_block, to_block) = match self.block_hash {


IMHO it would be good to return a RPC error in case block_hash and from_block &/ to_block is specified.

sorpaas · 2018-07-31T13:21:20Z

@MicahZoltu The current implementation will return empty vector. I think this may be just better align with the rest of eth_getLogs definition? If you specify a non-existent fromBlock or toBlock, we would also return empty vector right now.

MicahZoltu · 2018-07-31T13:31:20Z

@sorpaas Returning an empty array not only is against EIP-234, but it is also a massive source of problems for dapps right now. Augur is all but broken because of this behavior and the devs are grasping at some pretty crazy solutions to try to work around it while waiting for EIP-234 to be implemented.

You are correct that it aligns with previous behavior, but the previous behavior is not good behavior. Also, Geth has implemented EIP-234 and they return an error if blockhash does not exist so returning an error would be in line with Geth behavior.

For some context, the reason the current behavior is so problematic is because when a user requests logs for some block and they get back an empty array they cannot differentiate between "the block you requested has no logs" and "we don't know about the block you requested". This problem is magnified greatly when a load balanced backend like Infura is used, where each of your requests go to a different node. For example, you may get notification that block 0xabcd is the new head, but then when you ask for logs in block 0xabcd the node you speak to hasn't yet received block 0xabcd, so it returns an empty array.

Even without load balanced backends, this is still a problem because reorgs could occur such that you receive notification about a block, but by the time you ask for logs for that block it has been reorged out so you get an empty array. You then wait to poll for a new head for a while and by the time you poll for new head the block has been reorged back in. In this scenario, if the reorged block is pruned from the node when reorged out (so not available to fetch logs) you will incorrectly think that the block held no logs.

sorpaas · 2018-07-31T13:45:50Z

@MicahZoltu Sounds good! Will change this.

Given that this is a developer's pain point, I wonder whether we would also consider to change fromBlock and toBlock's behavior to be return error if it is non-existent? It's also really easy for any user to fallback to the old return-empty-vector behavior (by casting undefined/error to empty array). cc @5chdn @tomusdrw

tomusdrw · 2018-07-31T13:51:57Z

@sorpaas Makes perfect sense for me, let's expose the errors and mark that as a breaking change.

…ck is found This also changes PollFilter to store the EthFilter type, instead of the jsonrpc one, saving repeated conversion.

Use the old behavior (unwrap_or_default) for anywhere else.

MicahZoltu · 2018-08-02T03:57:35Z

IMO, "fixing" this (return an error instead of empty if block is missing) makes sense for block by number as well. I can't come up with a use-case where you would want an empty array instead of an error.

I tend to advise people against using the block number mechanism in general for anything live. If you are trying to backfill all of history, then a number range works fine and isn't likely to cause you any problems, but you really should stop ~10 blocks back from head (beyond reasonable reorg distance) and then switch to something more robust like fetching blocks by hash (or currently, since that doesn't exist, do really complicated things to verify what you are getting near head is what you think).

ghost · 2018-08-03T04:40:37Z

FWIW, the Forecast Foundation has an active 250 REP bounty on this implementation once it's merged. Whoever is responsible for the implementation, please ping me (@tk) on the Augur Discord or (@tomkysar) on Twitter to arrange payment.

sorpaas · 2018-08-03T07:26:08Z

+				(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)),


This indentation displays correctly on my editor. I think why it looks weird here is probably because the diff's + mark pushed tabs one level deeper.

Let me wrap that to a new line anyway.

avonian · 2018-08-03T16:51:39Z

seems to me like that reward needs to go to @sorpaas :)

...once its merged that is

andresilva · 2018-08-06T15:14:26Z

@sorpaas Please rebase/merge master and make sure to include the fix from #9285 when resolving conflicts.

…-eip234

andresilva

LGTM overall. Just a question regarding the returned error. Also can we add a test for this in mocked/eth.rs?

andresilva · 2018-08-06T15:41:24Z

+	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,


EIP234 requires that data include the hash of the requested block. Since we're also returning an error on to/from what do you think we should do? Is it really useful to return the requested hash if not found?

I think it's equally useful to return the from/to block, since there're two and the caller might want to know which. Let me fix this.

A few thoughts on this:

If a user asks for blocks 6,000,000 to 7,000,000 but the highest block is 6,500,000, should we interpret that to mean the user wants "all blocks from 6,000,000 to latest"?

If we consider it an error to ask for more blocks than latest, can the error data be the lowest block number that wasn't present? So in the above example the error data would be 6,500,001?

I haven't put a whole lot of thought into the above, @epheph may have some feelings since he has experienced a lot of the pain around this. My gut tells me that it should return an error because otherwise the user doesn't know which blocks had no logs and which blocks were missing, so it doesn't know what to ask for again. This is especially problematic when dealing with load balanced servers like Infura where each request may be routed differently and thus requests may go to a server that is behind (thus missing blocks that another had near head).

Typing all that out makes me reasonably confident that an error is in fact correct behavior. In that case, letting the user know what blocks are missing (rather than just "some blocks in the range") could prove useful to them, especially if they are attempting to fetch a very large range.

@MicahZoltu Yes, the current behavior would return error with the error data pointing to block 7,000,000. :)

I think returning 6,500,001 may be too costly on the client side. It needs to (at least partially) iterate over the block range.

An error when your range exceeds the available blocks, even by a single block, is the best action. Returning that 6,500,001 is where that cut-off is would be nice for troubleshooting (and more correct), but not an absolutely requirement. None of the applications I've worked on would have taken any other action besides re-requesting the entire batch that was originally requested.

(It's possible the app could read up to the available block and then wait for more to become available, but requesting large ranges of blocks is usually done for blocks significantly in the past. I believe this error will occur most when you are asking for small numbers of blocks very close to head)

Yeah like you said, I can't think of a use-case where we may query a non-existent fromBlock or toBlock. If a query doesn't care about it, it can just set toBlock to latest or null.

We have cases where we query potentially-non-existent blocks. When using a pool of eth nodes (such as Infura), one might receive a websocket notification of newHead block X (or when getBlockByNumber("latest") ), then in a follow-up query to getLogs from that block, it might hit a node that does not yet have that block. In this case, latest probably isn't good because latest is likely to have changed. My view of the purpose of EIP-234 is to remove the race condition that exists between a blockNumber and a blockHash.

I don't mind getting an error back if the block doesn't exist, i just don't want to get back something I can't distinguish from a successful fetch of no logs.

…ck number

sorpaas · 2018-08-06T17:49:55Z

+			//
+			// 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).


Our current use cases are mostly fine. We have cases where we set fromBlock to a nonexistent future block (in filter_changes), but we actually just expect empty result in that case. I didn't find any case where we set fromBlock to a current block, but toBlock to a nonexistent block.

But please do note that this is a breaking change.

Do we want to validate that from <= to as well? I don't think we validate that on the RPC layer.

dvdplm

lgtm with some minor grumbles

dvdplm · 2018-08-07T08:18:54Z

-	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()),


Would it make sense to return both the block ID and the block hash?

This BlockId contains BlockId::Number and BlockId::Hash, so it may be fine here -- the tester can choose which type of BlockId it wants to test. :)

dvdplm · 2018-08-07T08:20:09Z


+pub fn filter_block_not_found(id: BlockId) -> Error {
+	Error {
+		code: ErrorCode::ServerError(codes::UNSUPPORTED_REQUEST),


Is UNSUPPORTED_REQUEST the correct code here? Is that the -32000 from the EIP? If yes, maybe add a comment to that effect.

We use UNSUPPORTED_REQUEST for other params-invalid errors, like the above filter_not_found. So I think it may be fine. :)

Ok, you know best! It sounds a little like HTTP 501 Not Implemented but is more like HTTP 400 Bad Request, hence my confusion.

It will return the correct error code https://github.com/paritytech/parity-ethereum/blob/master/rpc/src/v1/helpers/errors.rs#L31.

For reference: https://www.jsonrpc.org/historical/json-rpc-over-http.html#errors

dvdplm · 2018-08-07T08:33:10Z

+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(),


typo: "One of the blocks…"

dvdplm · 2018-08-07T08:36:46Z

+	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}"#;


same typo here: "blockS"

dvdplm · 2018-08-07T08:38:52Z

 		let num_to_id = |num| match num {
 			BlockNumber::Num(n) => BlockId::Number(n),
 			BlockNumber::Earliest => BlockId::Earliest,
 			BlockNumber::Latest | BlockNumber::Pending => BlockId::Latest,


Not sure if it's relevant here, but if BlockNumber::Pending is deprecated, maybe we should add a match arm here and print a warn! about the deprecation? I've seen other places in the code where we do this.

We still use that -- it's only that we don't deal with it for eth_getLogs. For eth_filter namespace, pending logs are added on top of the logs returned by Client::logs if we specify BlockNumber::Latest.

andresilva

Changes LGTM. Just one more question regarding errors/validation.

andresilva · 2018-08-07T15:46:12Z

+			//
+			// 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).


Do we want to validate that from <= to as well? I don't think we validate that on the RPC layer.

andresilva · 2018-08-07T15:49:55Z

+				loop {
+					let header = match chain.block_header_data(&current_hash) {
+						Some(val) => val,
+						None => return Err(BlockId::Hash(current_hash)),


From RPC-side you'll input block numbers for from/to but in this case you would get back an error that is a block hash. I don't think it's very problematic since this error should be rare, since we do range validation before starting the loop. We could coerce the blockid error depending on the type used in filter, i.e. if it uses block numbers return error using block number, otherwise use hash. But maybe this is added complexity for a case that is rare (those blocks should be on your local db and it shouldn't fail).

This part of the code only deals with the case where if either from_block or to_block is a block hash. If both of them are numbers (which is the case for RPC's fromBlock and toBlock, then we won't go into this else branch (https://github.com/paritytech/parity-ethereum/pull/9256/files/4b45f9ac913f7ef9d7e3d3cda22a2d0c28dc23ea#diff-40c7c65265e511d07158f62712f69573R1850). If user inputs block numbers, he/she can only get back block numbers as errors. :)

sorpaas · 2018-08-07T17:03:23Z

(Github's interface is probably broken. Cannot comment on your first review message.)

Do we want to validate that from <= to as well? I don't think we validate that on the RPC layer.

Yeah in that case currently the bloom filter's iterator would just return empty result. I'm going to change this to Err(to_block).

andresilva

LGTM

jimpo · 2018-08-15T16:35:59Z

+			// 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());


The error returned from the RPC is "One of the blocks specified in filter (fromBlock, toBlock or blockHash) cannot be found", which seems confusing because the from and to blocks can be found, they are just out of order.

ghost · 2018-08-19T02:26:51Z

Following up here - happy to see this has been merged!

Who here is the claimer of the bounty? Please get in touch, and we can pay you out :-)

Implement EIP234

69f2d05

sorpaas added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Jul 31, 2018

sorpaas added this to the 2.1 milestone Jul 31, 2018

tomusdrw reviewed Jul 31, 2018

View reviewed changes

sorpaas added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 31, 2018

tomusdrw added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Jul 31, 2018

sorpaas added 2 commits August 1, 2018 11:51

Make filter conversion returns error if both blockHash and from/toBlo…

5a13b56

…ck 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

74f0e03

Use the old behavior (unwrap_or_default) for anywhere else.

sorpaas added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Aug 1, 2018

fix test: secret_store

6702767

ordian mentioned this pull request Aug 1, 2018

Better support for eth_getLogs in light mode #9186

Merged

sorpaas commented Aug 3, 2018

View reviewed changes

Fix weird indentation

5db8fc3

MicahZoltu mentioned this pull request Aug 4, 2018

Race condition ethereumjs/ethereumjs-blockstream#10

Closed

Merge branch 'master' of https://github.com/paritytech/parity into sp…

73684a8

…-eip234

andresilva suggested changes Aug 6, 2018

View reviewed changes

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

ca22582

sorpaas added 5 commits August 7, 2018 00:37

Return blockId error in rpc

c7e3f2f

test_client: allow return error on logs

9c9b613

Add a mocked test for eth_getLogs error

6f52ff9

fix: should return error if from_block/to_block greater than best blo…

e16292e

…ck number

Add notes on pending

be2e2e2

sorpaas force-pushed the sp-eip234 branch from bb9c668 to be2e2e2 Compare August 6, 2018 17:43

sorpaas commented Aug 6, 2018

View reviewed changes

dvdplm approved these changes Aug 7, 2018

View reviewed changes

sorpaas added 2 commits August 7, 2018 17:16

Add comment for UNSUPPORTED_REQUEST

8bf6a95

Address grumbles

4b45f9a

andresilva reviewed Aug 7, 2018

View reviewed changes

Return err if from > to

2ad31e5

andresilva approved these changes Aug 12, 2018

View reviewed changes

sorpaas merged commit a6df452 into master Aug 13, 2018

sorpaas deleted the sp-eip234 branch August 13, 2018 07:47

jimpo reviewed Aug 15, 2018

View reviewed changes

This was referenced Aug 15, 2018

Fix filter_block_not_found error message #9359

Closed

eth_getLogs error message can be confusing #9373

Closed

nionis mentioned this pull request Aug 19, 2018

Implement EIP-234 ConsenSys-archive/ganache#136

Closed

fabioberger mentioned this pull request Sep 21, 2018

Fix dropped events issue in Order-watcher and Contract-wrappers subscriptions 0xProject/0x-monorepo#1080

Merged

5 tasks

jam10o-new mentioned this pull request Feb 20, 2019

Add blockHash option to eth_getLogs (EIP 234) #9251

Closed

Conversation

sorpaas commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MicahZoltu commented Jul 31, 2018

Uh oh!

fabioberger commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorpaas commented Jul 31, 2018

Uh oh!

MicahZoltu commented Jul 31, 2018

Uh oh!

sorpaas commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomusdrw commented Jul 31, 2018

Uh oh!

MicahZoltu commented Aug 2, 2018

Uh oh!

ghost commented Aug 3, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avonian commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andresilva commented Aug 6, 2018

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresilva Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorpaas commented Jul 31, 2018 •

edited

Loading

fabioberger commented Jul 31, 2018 •

edited

Loading

sorpaas commented Jul 31, 2018 •

edited

Loading

ghost commented Aug 3, 2018 •

edited by ghost

Loading

avonian commented Aug 3, 2018 •

edited

Loading

andresilva Aug 7, 2018 •

edited

Loading

andresilva Aug 7, 2018 •

edited

Loading