Missing blocks in filter_changes RPC

[mattrutherford]

Fixes: #9858

Implement validity check of reported hashes (in case of re-org)
to determine the last block reported that is still valid. This will return hashes for
block numbers that have previously been reported if the hash is different.

Add const to set history length for validity check. Currently set to 32 blocks.

[parity-cla-bot]

It looks like @mattrutherford signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[tomusdrw]

Seems that this line is actually changing hash of block1, why isn't that block part of the response?

[mattrutherford]

Good catch, that would be the case if the hash was different, but actually in import_block() in test_client.rs, that value gets overwritten again by the old hash! So I just need to take that line out because it effectively does nothing.

[andresilva]

The current implementation LGTM, but I think we could also implement this by just storing (last_reported_number, last_reported_hash). We can calculate the tree_route between the current best block and the last reported block. If there's no re-org then last_reported_hash is an ancestor of best_block_hash, so tree_route(last, best).ancestor == last (and index should be 0). If there was a re-org then tree route will get us the common ancestor between the forks and we need to push to the client the route from common ancestor until best block. There's also the case where tree_route might return None if one of the provided hashes doesn't exist in the db, I'm not sure what to do in this case (just send everything from last_reported_block_number until best_block_number? Or from last_reported_block_number - 32?).

[mattrutherford]

OK, thanks for the comments @andresilva . I like the elegant solution you mentioned, but depends in practice on how often a re-org would exceed 1 block; and if so whether duplicating returned hashes would be less less nice from an end-user perspective? Personally I think on balance I would prefer to keep as it is, but I am open to re-working it, if it's deemed the additional overhead is undesirable?

[andresilva]

@mattrutherford I think assuming re-orgs are smaller than 32 blocks both methods would return the same results, the common ancestor found by tree_route would also be the same block we find when searching in recent_reported_hashes. The case where tree_route returns None shouldn't happen because both blocks should exist on the database (since we've seen it before) and we don't prune blocks on re-orgs. Either way I'm OK with the current solution, we can log this as an improvement.

[mattrutherford]

OK, noted, thanks. I notice that tree_root also includes forked blocks so we'd need to remove those if we care about duplication of the returned hashes.

[andresilva]

Ah, I see what you mean, indeed if there was a re-org tree_route would also report the retracted route. In this case we would just ignore the retracted part of the route, this method would be more useful if we could notify about previously reported blocks that were retracted (we do this for logs) but the API doesn't allow this right now and we can't break compatibility.

[mattrutherford]

I guess we could also rename tree_root, add the extra param and functionality and then make a new fn tree_root(&self, from: H256, to: H256) that basically just calls the modified fn.

[ankitchiplunkar]

Thanks a lot guys, this is great!
When can I test the improved functionality?