eth/filters, interfaces.go: EIP-234 Add blockHash option to eth_getLogs

[reductionista]

Adds a blockHash option to params field of eth_getLogs JSON-RPC call.

Returns logs matching filter criteria only for all transactions in the block with hash blockHash. This works the same as if you specify fromBlock= toBlock = (# of block whose hash is blockHash), but allows the call to work if the RPC client knows only the block hash but not the block number.

Returns invalid param error if both blockHash and either fromBlock or toBlock are specified.

This implements EIP-234, which includes details on the motivation for it.

I've tested this and it works both for a full node and for a light node, as long as they have finished initial syncing. See some examples here.

[holiman]

LGTM!

[reductionista]

@karalabe Just a reminder, that this is all ready to merge.

Because it's only a wrapper around current functionality that adds a new way of calling the same thing, very low risk that it could negatively affect existing functionality.

[tinybike]

@karalabe this is something that is pretty important for augur, any idea when this will be merged?

[reductionista]

I've added documentation for this option to the JSON-RPC wiki . I've marked it as "future" in bold; will remove that once this is merged.

[karalabe]

I'm not sure this PR fully implements the desired functionality. It assumes that blockHash will be from the canonical chain, because it just converts the hash into a number and start filtering that way. However if the hash is from a side chain, it will filter the canonical block corresponding to the same height, not the side block.

[reductionista]

@karalabe Oh, you're right! A big oversight on my part.

I think all I need to add here is a check to make sure the block requested is part of the canonical chain. If it's not, it should just return an error instead of the logs.

@MicahZoltu I hope this behavior is acceptable to you. If you need side-chain logs to be returned, that would be a much larger change, and I'm not even sure it would be possible (as I don't think there is any guarantee that logs from side-chain blocks are stored permanently). Please confirm that returning an error when the requested block hash is not part of the canonical block chain would solve the underlying client problem this is aimed at solving.

[MicahZoltu]

It should return an error if the block for the given hash is not found. It would be very desirable if it could pull logs that are still available and return them, even if the block isn't part of the current canonical chain, e.g., if the block just got reorgs out and hasn't yet been deleted/pruned.

I think in the majority of cases where this matters, the user is asking for logs of a block they just got told about and even if a reorg happened in the meantime the node very likely still has the block hanging around (though, certainly I understand that there are no guarantees here).

However, if that isn't possible then a "not-found" error response seems appropriate.

[MicahZoltu]

Side note: I have actually solved my specific problem client side, but if I hadn't then returning an error for blocks that just got reorged out would not have solved the problem. The problem we were facing is that reorgs (which are common) resulted in sometimes racing and asking for logs of a block that just got reorged out.

This is further complicated by the fact that the block could get reorged back in before we fetch the next block, so we can't just drop the logs for that block, we would have to drop the whole block. If we could get the logs for the block, then either it will stay reorged out and we'll replace the whole block soon enough or it will get reorged back in and we'll have gotten the logs for it already.

[reductionista]

@MicahZoltu thanks for explaining.

Today I looked through the source code more thoroughly, and apparently geth does keep all block receipts permanently, even if they were removed from the chain. So I withdraw my comment about it potentially being impossible (although there still may be no guarantees about this in the protocol itself, in future versions of geth, or in other clients). However, I'm trying to get EIP758 finished before I start a new job in July, and unfortunately I can't afford to take a break right now for a week to add this additional functionality to EIP234.

"This is further complicated by the fact that the block could get reorged back in before we fetch the next block, so we can't just drop the logs for that block, we would have to drop the whole block."

So, would I be correct in saying that this case of a block being removed from the chain temporarily and then added back in again would be the only known instance so far where receiving the logs from non-canonical blocks would be useful? It seems like the same thing could be accomplished by keeping the block, but re-requesting the logs if and when it gets reorg'd back in, right? Is that difficult or cumbersome for a client to implement? Just trying to get a sense of how important this additional functionality would be compared to the rest. Also curious to hear @karalabe's thoughts on it. One possibility is to merge in what we have now, and then if and when I get a chance later I will add the rest.

[MicahZoltu]

It seems like the same thing could be accomplished by keeping the block, but re-requesting the logs if and when it gets reorg'd back in, right?

It is possible to never witness the block getting reorged out or back in. Take the following example:

1. Block 123 hash 0xabc is announced to client

2. Client asks for logs of block 123 3. Server responds with logs from block 0xdef 4. Client asks for block 123 5. Server responds with block 123 hash 0xabc

In this example, the only hint of a reorg the client has is that the logs it got back don't match the logs it expected. Otherwise, it always witnesses the 0xabc chain when talking to the node. The problem here is that if it gets back no logs then it won't witness the reorg and it won't know to throw out the block!

There are two ways for the client to deal with this:

1. Throw away any block that the client cannot get logs for, this requires the client have a way to identify whether they actually got logs from the expected block or not.
2. Talk to a server that will give it logs for blocks outside the canonical chain.

[MicahZoltu]

I do think that what you have now is significantly better than nothing. Even with my recent client side fix, I just realized that there are still failure scenarios where the client will miss some logs unless what you have is merged.

[reductionista]

@MicahZoltu Ok, if I understand correctly, the double-reorg scenario you've described is currently a problem, but would be solved if my present implementation of EIP-234 is merged:

1. Block 123 hash 0xabc is announced to client
2. Client requests logs for blockHash 0xabc
3. Server responds with error: "Block with hash 0xabc was removed from canonical chain."
4. Client waits until new announcements come, since it's now aware this block was removed.
5. Block 123 hash 0xdef is announced to client
6. Client requests logs for blockHash 0xdef
7. Server responds with error: "Block with hash 0xdef was removed from canonical chain."
8. Client waits again, since 0xdef was also removed.
9. Block 123 hash 0xabc is announced to client
10. Client requests logs for blockHash 0xabc
11. Server returns logs for blockHash 0xabc
    ...
    The server letting the client know when it's trying to request logs for a removed block allows the two to synchronize, whereas before there was a race condition where if the 2nd two announcements get delayed, invalid logs could be returned and confuse the client.

The main question I'm wondering is whether there is an example of a case where my current implementation would not fix the problem. Based on your most recent comment, it sounds like the answer is no, even though adding the ability to receive logs for removed blocks might potentially be useful in some other cases (that Augur currently doesn't care about). Is that right?

[MicahZoltu]

@reductionista You are correct. With the changes I recently made to ethereumjs-blockstream I am now able to solve all known problems except the one described, which would be solved by the node returning an error instead of the logs for the wrong block. Thus, I am satisfied with the solution you have. 😄

[reductionista]

@karalabe Please advise if anything else needs to be done in order for this to be merged.

[karalabe]

The PR didn't solve the issue it set out to solve. If I understand correctly, the original issue was that we wanted to filter for some logs, but the returned logs were from a different block. The PR in the current implementation verified whether the block hash is part of the canonical chain, and if so, did a filter based on block number. But a reorg can happen after verifying that it's on the canonical chain, but before filtering it. That would lead to the exact same erroneous behavior.

The only way to properly implement this EIP is to extend the filtering mechanism itself so that it can operate either on block numbers or on block hashes. Without explicit support for hash based filtering, this PR will forever have a racey behavior.

I've pushed a commit on top which extends the low level filtering itself to be able to do both range queries as well as block hash queries. Could please all of you verify that a) the new mechanism works as expected and b) the old one still works (i.e. that I didn't bork anything)?

[holiman]

Have not tested the PR, but the changes LGTM

[karalabe]

@reductionista @tinybike @MicahZoltu Btw, I didn't know there was a bounty put on this PR, nor do I want to have a claim in it. The reason I figured I'll fix up the code is to get this merged in faster since we went back and forth for a long time already. The original contribution IMHO did a good job, alas didn't have the full picture. Nonetheless it's because of the original contribution that this PR gets merged, so feel free to award the full bounty to @reductionista.

[reductionista]

@karalabe, you are extremely generous!!

Thank you. I will avoid more back-and-forth and accept your generosity. If you ever change your mind and decide you want your half of it, just let me know and I'll send it your way! Assuming I haven't foolishly cashed it in before it goes to the moon, of course. ;-)