Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix older HF blockchain tests test runner / test on older HFs #813

Closed
holgerd77 opened this issue Jul 15, 2020 · 3 comments · Fixed by #815
Closed

Fix older HF blockchain tests test runner / test on older HFs #813

holgerd77 opened this issue Jul 15, 2020 · 3 comments · Fixed by #815

Comments

@holgerd77
Copy link
Member

Currently the blockchain test runner is not working on older HF settings, e.g. on using the following Byzantium test command: node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain --fork=Byzantium

The tests read are mixed up on the HF side (might be an issue on either ethereumjs-testing or vm side and tests are therefore failing.

This should be fixed. As a follow-up blockchain tests should be at least manually run once (or/and - maybe separate PR) integrated into nightly CI.

It is likely that not all tests will pass due to all the HF related changes lately. It would be good if this can be tackled before another HF implementation is started.

//cc @jochem-brouwer

@holgerd77
Copy link
Member Author

@jochem-brouwer It might be practical to address this along the update of the ethereumjs-testing repo for the BLS test integration. Then - if there is some fix in the ethereumjs-testing code needed - this can be released in one release.

For doing a release there the submodule needs to be updated to the latest state of the (attention!) develop branch of the https://github.com/ethereum/tests repo, there are also some instructions on this on the testing repo. Then a release PR like this ethereumjs/ethereumjs-testing#47 is done and the release itself is done directly on GitHub by creating a new tag on the Releases page. If you have not sufficient permissions @evertonfraga will likely be able to help you on this.

@jochem-brouwer
Copy link
Member

Byzantium is a fork which we have supported for a long time, right? I find it confusing that it thus seems that the Byzantium tests are not working?

"The tests read are mixed up on the HF side". What do you mean by this? Does it read the wrong state roots from the test file?

@holgerd77
Copy link
Member Author

Regarding the blockchain tests, these are only run on the latest fork configuration in our current CI setup, this is something we also should improve on, this is some historic relic from a time when test runner times where a lot higher + we didn't have the nightly runs yet.

So with this actual setup it happens relatively easily that over time some things/bugs sneak in which are outside the scope of the GeneralStateTests and cause the blockchain tests too fail.

This is not confirmed though yet if this is the case here: currently it is at first just the test runner which is not working, just run the command I stated and you will see. The runner takes in the test data from the wrong forks (this will likely just be some string/regex comparison thing) and - as a follow-up - produces test failures (I think the VM is running on a fork set - e.g. Byzantium - but the state root is compared to the state root from the other-fork-test-data).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants