Fix blockchain tests #384
Fix blockchain tests #384
Conversation
| * @param {Buffer} buff | ||
| * @returns {Buffer} a slice of the given buffer starting at the first non-zero entry | ||
| */ | ||
| function dropLeadingZeroes (buff) { |
There was a problem hiding this comment.
This was introduced fairly recently (< 1 year ago). Can you please look up in git blame which PR introduced it and why? There was some reasoning.
There was a problem hiding this comment.
Never mind, you've introduced it, nevertheless having the PR number linked here helps future tracking why things change.
There was a problem hiding this comment.
PR was #295
There was a problem hiding this comment.
As mentioned in my original comment I've only reverted this to demonstrate the scope of test failures this change has caused. This change has also caused other failures to be masked which raises the question as to the correct solution?
mattdean-digicatapult
force-pushed
the
fix-blockchain-tests
branch
from
45390ac to
3dbc6ad
Compare
|
Short update: I did repository-wide searches on Parity Ethereum and go-ethereum searching for some of the test names mentioned above (e.g. to discover some skip list or similar), but didn't find anything matching. |
|
Hey, I saw that you asked me to chime in about #295. Several months ago I set out to ensure that Geth's RPC libraries could be used with Ganache, as I develop a Go application and wanted to use Ganache for testing. With that goal in mind, I made pull requests to a bunch of different dependencies of Ganache to ensure that things like block hashing and bloom filters were implemented to be compliant with the consensus clients. In this case, the PR I made was because if I loaded a Ganache generated bloom filter into the Geth bloom filter library, and tested it for the topics that should have been present, it would return false, that those items were not present in the bloom filter. I eventually determined that this was because Geth (and other consensus clients) trim leading zeroes before hashing the values for insertion into the bloom filter. When I made my PR seven months ago, the tests were passing. I was asked to make a few changes to style and naming conventions, but nothing that changed the behavior of my code. It took a while for me to get to the requested changes, and I suspect that during that time new tests were created that copied the bloom filter that would have been incompatible with Geth and Parity into new tests. After I made the requested stylistic changes, the tests on my branch were still passing, but I suspect the branch it got merged into was not compatible with the change. I'm reasonably confident that my implementation of bloom filters is a correct (at least in the sense that it is compatible with the consensus clients). The failing tests were probably introduced between when my changes were made, and when my changes got merged. |
|
Hi @AusIV, thanks for clarifying. I've just checked out your branch from your fork of the repo and indeed all the above tests do fail there as I would have expected. I think the issue is at the time we weren't running the blockchain tests as part of CI, we only ran the state tests, so this probably went by under the radar. I think we need to do some investigation into how Geth and Parity deal with this discrepancy; I completely agree with your interpretation as to their bloom filter implementation BTW. |
|
Yes, that's correct, blockchain tests weren't running |
|
My current tendency is to leave implementation here as is, add the tests to the skipped list in Will also continue to think about if we can find a place for inter-client consensus discussions like this, since I am currently also a bit involved in coordination around ethereum/tests, it might be a way to establish consensus discussions directly there (as meta issues e.g.), let me know, if you have ideas on this or you know that there is already some initiative on this. Long term solution would probably rather have an evolving up-to-date spec apart from the yellow paper. I know that there is some discussion around making K-EVM this up-to-date alternative/enhancing spec, not sure how far/concrete these discussions are. Maybe @cdetrio can say something on this, not sure? |
|
(so "as is" means, without this PR changes) |
|
On further inspection of geth I'm not entirely convinced that they use the The bloom filter is created on the block header here https://github.com/ethereum/go-ethereum/blob/c134e00e488bf4bfd88689ecf6b91ee351fb77e5/core/types/block.go#L197. Is there a potential inconsistency between using the |
|
Hmm, maybe it would generally make sense to add some common bloom filter tests to the |
|
@holgerd77, I'd actually say the The issue in geth, if I'm correct (and that's a big if), would be that the |
|
I'm about ready to admit #295 may have been a mistake on my part, based on the assumption that Geth's But the other perplexing piece of this puzzle is that Geth's The other thing that confuses me is that my code uses |
|
I've opened an issue with Geth, as I'm now convinced that the problem is in bloom.Add() and bloom.Test(). Sincere apologies for any confusion I've caused, #295 ought to be rolled back out. |
|
Thanks @AusIV, to be honest I initially read the geth code exactly the same as you did. @holgerd77, first I'm going to rebase this and once that's done I think it's ready for review? |
mattdean-digicatapult
force-pushed
the
fix-blockchain-tests
branch
from
3dbc6ad to
4b08400
Compare
|
@mattdean-digicatapult Yes, sounds great! @AusIV Thank you very much for doing this deep-dive follow-up on this! |
This fixes the last set of blockchain tests: * RPC_API_Test * randomStatetest224BC * randomStatetest234BC * randomStatetest529BC * ExtraData32 * log1_correct * timeDiff0 * timeDiff12 * timeDiff13 * timeDiff14 These were mostly caused by missing block validation and the intentional disparity between the bloom filter implementation and the yellow paper introduced in #295.
holgerd77
force-pushed
the
fix-blockchain-tests
branch
from
4b08400 to
d21f06d
Compare
|
Rebased this. |
This fixes the last set of failing blockchain tests:
These were mostly caused by missing block validation and the intentional disparity between the bloom filter implementation and the yellow paper introduced in #295.
The bloom filter changes I've introduced are obviously problematic (hence do-not-merge) but without them the following additional test failures, caused by a lack of block validation, were being hidden:
These tests pass in master purely because the bloom filter does not match as expected rather than because block validation isn't occurring. I guess the obvious question is how do other implementations handle this disparity between the bloom filters when it comes to testing?