Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Add constantinople conf to EvmTestClient. #9570

Merged
merged 14 commits into from
Sep 25, 2018

Conversation

cheme
Copy link
Contributor

@cheme cheme commented Sep 17, 2018

Fixes #9568 .
Constantinople spec was not resolve when using EvmTestClient. This adds it.

@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

@cheme cheme added A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). labels Sep 17, 2018
@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 17, 2018
@debris
Copy link
Collaborator

debris commented Sep 17, 2018

lgtm, but some of the constantinople json tests are failing. @sorpaas do you know what's missing / should be fixed?

@cheme
Copy link
Contributor Author

cheme commented Sep 17, 2018

I did not see the failing test, those are partly related to outdated ethtest submodule reference (last time I checked the test did not include some eip and I had to go back to some old block reward value and others changes to make them pass). I think I will try to update the test or at least makes change required to avoid breaking CI.

@cheme cheme added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Sep 17, 2018
@sorpaas
Copy link
Collaborator

sorpaas commented Sep 17, 2018

Tried to look into this via CreateOOGafterInitCodeReturndata2_d0g0v0, and it turned out to be a blockReward error. It can be fixed by changing the Constantinople block reward transition to a block other than zero. I suspect many of the other test failures are also due to this.

@cheme cheme removed A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). A8-looksgood 🦄 Pull request is reviewed well. labels Sep 17, 2018
@cheme
Copy link
Contributor Author

cheme commented Sep 17, 2018

From my tests many error seems related to the tests (for instance when testing with old config many constantinople tests are passing). I also got too many error to consider fixing them under this PR.

I started to put a filter to skip tests that are considered 'under issue' : this is for keeping our state tests up to date. I believe it is better to have the state test close to latest commit from 'ethereum/tests' (with identified failing one) than keeping an old branch for the submodule.
Skipped test got a 'reference' field (in the json conf) that must refer to a pending issue (in our case a hub issue needs to be created to update all those field).
I see two categories of 'under issue' tests :

  • those that requires a fix from parity
  • those that requires stabilization from the submodule (currently there is other issue with geth or aleth).

@5chdn 5chdn added this to the 2.2 milestone Sep 17, 2018
@sorpaas
Copy link
Collaborator

sorpaas commented Sep 17, 2018

Indeed, right now it looks like we don't have any skip method for jsontests. It seems that we just passed all tests for ethereum/tests in the past?

Copy link
Collaborator

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may be a good idea if we can take at least one or two days to try to fix the test first. If we determined that they are too difficult to fix, it's definitely a fine idea to have a skip tests framework!

"CreateOOGafterInitCode_d0g1v0_Constantinople",
"CreateOOGafterInitCodeReturndataSize_d0g0v0_Constantinople",
"CreateOOGafterInitCode_d0g0v0_Constantinople",
"CreateOOGafterInitCodeReturndata2_d0g0v0_Constantinople",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please cherry-pick 544f6a3
I think the only one left here right now should be CreateOOGafterInitCodeReturndata2.

"reference": 9568,
"failing": "stRevertTest",
"subtests": [
"RevertOnEmptyStack_d0g0v0_Constantinople"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is fixed? Right now what I get from GeneralStateTest_stRevertTest is only ["LoopCallsDepthThenRevert2"].

"reference": 9568,
"failing": "stSpecialTest",
"subtests": [
"FailedCreateRevertsDeletion_d0g0v0_Constantinople",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one doesn't appear any more with 544f6a3

},
{
"reference": 9568,
"failing": "stSystemOperationsTest",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed as well with 544f6a3

},
{
"reference": 9568,
"failing": "stStaticCall",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are passing except static_checkOpcodes2.

"reference": 9568,
"failing": "stZeroKnowledge",
"subtests": {
"pointAddTrunc": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like those are passing except pointAdd!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is something not so good with the way the test are currently running : having a first failure is hiding other tests failure (it reports only one failure but if I skip it, it shows the other ones). So the other test in this branch are still failing for me.

[

{
"reference": 9568,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this type to a string? Sometimes it may be easier to reference it via ethereum/tests's issue number.

"subnumbers": ["*"],
"chain": "Constantinople (test)"
},
"pointAdd": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is indeed ethereum/tests's issue: ethereum/tests#512

@5chdn 5chdn added M4-core ⛓ Core client code / Rust. M2-config 📂 Chain specifications and node configurations. labels Sep 18, 2018
"reference": "https://github.com/ethereum/tests/issues/512",
"failing": "stZeroKnowledge",
"subtests": {
"pointAddTrunc": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those one are all actually passing except pointAdd? @cheme Do you mind to check this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not passing :( , there is something bad with our current code : if you run the test without skipping anything it fails on only one test, but as soon as you skip it, it reveals others failure.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Can you also cherry-pick 16f661a? Our previous definition of modexp and bn128_mul has wrong gas price. I think it may fix some other tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it does reduce the number of failing zk tests. Many still don't pass, but it is due to ethereum/tests#512 (with 1283 disabled everything is fine).

@cheme
Copy link
Contributor Author

cheme commented Sep 18, 2018

@sorpaas , extending the test macro to skip the tests seems possible, but I fear that it will somehow make those skips less visible. I like having a json (I do not say that its current schema is right) to keep trace of those skips because it is accessible quickly to anyone (non rust developpers included).

@sorpaas
Copy link
Collaborator

sorpaas commented Sep 18, 2018

@cheme Yeah that's certainly a trade-off! I just think having a global variable doesn't really look nice for the long-term (we also need to look into the declare_test! macros to know which folders are being ran!) -- and IMHO we really should aim to only use skip tests for the short-term. :)

Anyway, regarding current skip tests, if we add something like below somewhere in the code, it should avoid the situation where someone accidentally enabled ci-skip-tests for non-testing build. This may be important because those jsontests functions are also used in evmbin:

#[cfg(all(not(test), feature = "ci-skip-tests"))]
compile_error!("ci-skip-tests can only be enabled for testing builds.")

sorpaas and others added 2 commits September 18, 2018 12:16
that it stop testing at the first file failure).
Add some missing tests.
Add skip for those (block create2 is one hundred percent false but on
hive we can see that geth and aleth got similar issue for this item).
Copy link
Collaborator

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A final grumble. I think it should be fine to merge this right now -- some of the jsontests are indeed not fix-able on our side.

@@ -0,0 +1,374 @@
{ "block":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this file to use tabs for indentation.

@sorpaas
Copy link
Collaborator

sorpaas commented Sep 19, 2018

ethereum/tests#511 should be the one that fixes most of those test failures.

@cheme cheme added the A0-pleasereview 🤓 Pull request needs code review. label Sep 19, 2018
@5chdn 5chdn added the A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. label Sep 19, 2018
@cheme cheme mentioned this pull request Sep 19, 2018
17 tasks
@cheme cheme removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Sep 19, 2018
@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Sep 25, 2018
@5chdn 5chdn merged commit 692d5b4 into openethereum:master Sep 25, 2018
dvdplm added a commit that referenced this pull request Sep 27, 2018
…mon-deps

* origin/master:
  ethereum libfuzzer integration small change (#9547)
  cli: remove reference to --no-ui in --unlock flag help (#9616)
  remove master from releasable branches (#9655)
  ethcore/VerificationQueue don't spawn up extra `worker-threads` when explictly specified not to (#9620)
  RPC: parity_getBlockReceipts (#9527)
  Remove unused dependencies (#9589)
  ignore key_server_cluster randomly failing tests (#9639)
  ethcore: handle vm exception when estimating gas (#9615)
  fix bad-block reporting no reason (#9638)
  Use static call and apparent value transfer for block reward contract code (#9603)
  HF in POA Sokol (2018-09-19) (#9607)
  bump smallvec to 0.6 in ethcore-light, ethstore and whisper (#9588)
  Add constantinople conf to EvmTestClient. (#9570)
andresilva pushed a commit that referenced this pull request Oct 9, 2018
* Add constantinople conf to EvmTestClient.

* Skip some test to update submodule etheureum/tests submodule to latest.

* Put skipping 'under issue' test behind a feature.

* Change blockReward for const-test to pass ethereum/tests

* Update tests to new constantinple definition (change of reward at block
5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible)Update tests to new constantinple definition (change
of reward at block 5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible).

* Fix modexp and bn128_mul gas prices in chain config

* Changes `run_test_path` method to append its directory results (without
that it stop testing at the first file failure).
Add some missing tests.
Add skip for those (block create2 is one hundred percent false but on
hive we can see that geth and aleth got similar issue for this item).

* retab current.json

* Update reference to parity issue for failing tests.
andresilva pushed a commit that referenced this pull request Oct 9, 2018
* Add constantinople conf to EvmTestClient.

* Skip some test to update submodule etheureum/tests submodule to latest.

* Put skipping 'under issue' test behind a feature.

* Change blockReward for const-test to pass ethereum/tests

* Update tests to new constantinple definition (change of reward at block
5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible)Update tests to new constantinple definition (change
of reward at block 5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible).

* Fix modexp and bn128_mul gas prices in chain config

* Changes `run_test_path` method to append its directory results (without
that it stop testing at the first file failure).
Add some missing tests.
Add skip for those (block create2 is one hundred percent false but on
hive we can see that geth and aleth got similar issue for this item).

* retab current.json

* Update reference to parity issue for failing tests.
andresilva pushed a commit that referenced this pull request Oct 9, 2018
* Add constantinople conf to EvmTestClient.

* Skip some test to update submodule etheureum/tests submodule to latest.

* Put skipping 'under issue' test behind a feature.

* Change blockReward for const-test to pass ethereum/tests

* Update tests to new constantinple definition (change of reward at block
5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible)Update tests to new constantinple definition (change
of reward at block 5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible).

* Fix modexp and bn128_mul gas prices in chain config

* Changes `run_test_path` method to append its directory results (without
that it stop testing at the first file failure).
Add some missing tests.
Add skip for those (block create2 is one hundred percent false but on
hive we can see that geth and aleth got similar issue for this item).

* retab current.json

* Update reference to parity issue for failing tests.
andresilva pushed a commit that referenced this pull request Oct 9, 2018
* Add constantinople conf to EvmTestClient.

* Skip some test to update submodule etheureum/tests submodule to latest.

* Put skipping 'under issue' test behind a feature.

* Change blockReward for const-test to pass ethereum/tests

* Update tests to new constantinple definition (change of reward at block
5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible)Update tests to new constantinple definition (change
of reward at block 5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible).

* Fix modexp and bn128_mul gas prices in chain config

* Changes `run_test_path` method to append its directory results (without
that it stop testing at the first file failure).
Add some missing tests.
Add skip for those (block create2 is one hundred percent false but on
hive we can see that geth and aleth got similar issue for this item).

* retab current.json

* Update reference to parity issue for failing tests.
andresilva pushed a commit that referenced this pull request Oct 10, 2018
* Add constantinople conf to EvmTestClient.

* Skip some test to update submodule etheureum/tests submodule to latest.

* Put skipping 'under issue' test behind a feature.

* Change blockReward for const-test to pass ethereum/tests

* Update tests to new constantinple definition (change of reward at block
5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible)Update tests to new constantinple definition (change
of reward at block 5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible).

* Fix modexp and bn128_mul gas prices in chain config

* Changes `run_test_path` method to append its directory results (without
that it stop testing at the first file failure).
Add some missing tests.
Add skip for those (block create2 is one hundred percent false but on
hive we can see that geth and aleth got similar issue for this item).

* retab current.json

* Update reference to parity issue for failing tests.
5chdn pushed a commit that referenced this pull request Oct 10, 2018
* ethash: implement EIP-1234 (#9187)

* Implement EIP-1052 (EXTCODEHASH) and fix several issues in state account cache (#9234)

* Implement EIP-1052 and fix several issues related to account cache

* Fix jsontests

* Merge two matches together

* Avoid making unnecessary Arc<Vec>

* Address grumbles

* Comply EIP-86 with the new definition (#9140)

* Comply EIP-86 with the new CREATE2 opcode

* Fix rpc compile

* Fix interpreter CREATE/CREATE2 stack pop difference

* Add unreachable! to fix compile

* Fix instruction_info

* Fix gas check due to new stack item

* Add new tests in executive

* Fix have_create2 comment

* Remove all unused references of eip86_transition and block_number

* Implement KIP4: create2 for wasm (#9277)

* Basic implementation for kip4

* Add KIP-4 config flags

* typo: docs fix

* Fix args offset

* Add tests for create2

* tests: evm

* Update wasm-tests and fix all gas costs

* Update wasm-tests

* Update wasm-tests and fix gas costs

* `gasleft` extern implemented for WASM runtime (kip-6) (#9357)

* Wasm gasleft extern added

* wasm_gasleft_activation_transition -> kip4_transition

* use kip-6 switch

* gasleft_panic -> gasleft_fail rename

* call_msg_gasleft test added and gas_left agustments because this openethereum/wasm-tests#52

* change .. to _

* fix comment for the have_gasleft param

* update tests (openethereum/wasm-tests@0edbf86)

* Add EIP-1014 transition config flag (#9268)

* Add EIP-1014 transition config flag

* Remove EIP-86 configs

* Change CREATE2 opcode index to 0xf5

* Move salt to the last item in the stack

* Change sendersaltandaddress scheme to comply with current EIP-1014

* Fix json configs

* Fix create2 test

* Fix deprecated comments

* EIP 1283: Net gas metering for SSTORE without dirty maps (#9319)

* Implement last_checkpoint_storage_at

* Add reverted_storage_at for externalities

* sstore_clears_count -> sstore_clears_refund

* Implement eip1283 for evm

* Add eip1283Transition params

* evm: fix tests

* jsontests: fix test

* Return checkpoint index when creating

* Comply with spec Version II

* Fix docs

* Fix jsontests feature compile

* Address grumbles

* Fix no-checkpoint-entry case

* Remove unnecessary expect

* Add test for State::checkpoint_storage_at

* Add executive level test for eip1283

* Hard-code transaction_checkpoint_index to 0

* Fix jsontests

* Add tests for checkpoint discard/revert

* Require checkpoint to be empty for kill_account and commit

* Get code coverage

* Use saturating_add/saturating_sub

* Fix issues in insert_cache

* Clear the state again

* Fix original_storage_at

* Early return for empty RLP trie storage

* Update comments

* Fix borrow_mut issue

* Simplify checkpoint_storage_at if branches

* Better commenting for gas handling code

* Address naming grumbles

* More tests

* Fix an issue in overwrite_with

* Add another test

* Fix comment

* Remove unnecessary bracket

* Move orig to inner if

* Remove test coverage for this PR

* Add tests for executive original value

* Add warn! for an unreachable cause

* Update state tests execution model (#9440)

* Update & fix JSON state tests.

* Update tests to be able to run ethtest at
021fe3d410773024cd5f0387e62db6e6ec800f32.

- Touch user in state
- Adjust transaction tests to new json format

* Switch to same commit for submodule ethereum/test as geth (next includes constantinople changes).
Added test `json_tests::trie::generic::TrieTests_trieanyorder` and a few
difficulty tests.

* Remove trietestnextprev as it would require to parse differently and
implement it.

* Support new (shitty) format of transaction tests.

* Ignore junk in ethereum/tests repo.

* Ignore incorrect test.

* Update to a later commit

* Move block number to a constant.

* Fix ZK2 test - touched account should also be cleared.

* Fix conflict resolution

* Fix checkpointing when creating contract failed (#9514)

* In create memory calculation is the same for create2 because the additional parameter was popped before. (#9522)

* Enable all Constantinople hard fork changes in constantinople_test.json (#9505)

* Enable all Constantinople hard fork changes in constantinople_test.json

* Address grumbles

* Remove EIP-210 activation

* 8m -> 5m

* Temporarily add back eip210 transition so we can get test passed

* Add eip210_test and remove eip210 transition from const_test

* Add constantinople conf to EvmTestClient. (#9570)

* Add constantinople conf to EvmTestClient.

* Skip some test to update submodule etheureum/tests submodule to latest.

* Put skipping 'under issue' test behind a feature.

* Change blockReward for const-test to pass ethereum/tests

* Update tests to new constantinple definition (change of reward at block
5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible)Update tests to new constantinple definition (change
of reward at block 5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible).

* Fix modexp and bn128_mul gas prices in chain config

* Changes `run_test_path` method to append its directory results (without
that it stop testing at the first file failure).
Add some missing tests.
Add skip for those (block create2 is one hundred percent false but on
hive we can see that geth and aleth got similar issue for this item).

* retab current.json

* Update reference to parity issue for failing tests.

* Hardfork the testnets (#9562)

* ethcore: propose hardfork block number 4230000 for ropsten

* ethcore: propose hardfork block number 9000000 for kovan

* ethcore: enable kip-4 and kip-6 on kovan

* etcore: bump kovan hardfork to block 9.2M

* ethcore: fix ropsten constantinople block number to 4.2M

* ethcore: disable difficulty_test_ropsten until ethereum/tests are updated upstream

* Don't hash the init_code of CREATE. (#9688)

* Implement CREATE2 gas changes and fix some potential overflowing (#9694)

* Implement CREATE2 gas changes and fix some potential overflowing

* Ignore create2 state tests

* Split CREATE and CREATE2 in gasometer

* Generalize rounding (x + 31) / 32 to to_word_size

* ethcore: delay ropsten hardfork (#9704)

* Add hardcoded headers (#9730)

* add foundation hardcoded header #6486017

* add ropsten hardcoded headers #4202497

* add kovan hardcoded headers #9023489

* gitlab ci: releasable_branches: change variables condition to schedule (#9729)

* HF in POA Core (2018-10-22) (#9724)

poanetwork/poa-chain-spec#87
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M2-config 📂 Chain specifications and node configurations. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Constantinople statetest support
6 participants