[WIP] Update upstream tests for Istanbul #1852

voith · 2019-09-18T22:36:50Z

What was wrong?

Please ignore this for now. I'm just experimenting. I will ping the team once it's ready or if I need help

How was it fixed?

not fixed yet.

TODO:

Blockchain Tests are not yet included for Istanbul yet. Update the commit when they get included.
blake2 tests are still WIP. Update them when they are ready.
Find out what's wrong with the blockchain tests for older VMs and fix them.
Update CircleCI to include Istanbul tests
Fix Istanbul blockchain tests

To-Do

Clean up commit history

Add entry to the release notes

Cute Animal Picture

voith · 2019-09-21T12:14:29Z

Find out what's wrong with the blockchain tests for older VMs and fix them.

Test failures are related to this issue: ethereum/tests#637.
The format of tests has changed. I'll have to wait until the format is finalized.

voith · 2019-09-22T17:19:46Z

Tests are crashing on CircleCi. I ssh'd into the machine and monitored the memory usage using htop. When the tests are about to crash the memory usage shoots to 4gb. CircleCI by default allocates 4gb of RAM which is the reason for the crash IMO. I tried increasing the machine size but currently, the paid tier doesn't support large instances (d7bd8c1).
cc @pipermerriam
BTW, Tests are passing locally on my machine (16gb RAM)

voith · 2019-09-22T18:04:54Z

scripts/benchmark/checks/deploy_dos.py

@@ -43,8 +43,8 @@
 )

 FIRST_TX_GAS_LIMIT = 367724
-SECOND_TX_GAS_LIMIT = 62050
-THIRD_TX_GAS_LIMIT = 104789
+SECOND_TX_GAS_LIMIT = 63042


Had to increase gas price(+992) here in order to get the tests passing. Don't know how updating upstream tests affects benchmark tests. Is this expected or should I dig into this? cc @cburgdorf

Istanbul reprices some opcodes so I'm relatively sure that this change in the tx gas limit is legit. 👍

voith · 2019-09-23T07:34:58Z

UPDATE: I got tests to turn green by marking the tests slow that were consuming a lot of memory.
These tests have the following comment:
test takes a lot for time because of sha3 calculation on a huge range.
Is it okay to mark these tests as slow?

fix copy/paste

voith · 2019-09-23T08:10:51Z

upstream tests now include blockchain tests for Istanbul. 11 tests are failing at the moment. I will look at these test failures later.
Below are some stats for Istanbul blockchain tests:
11 failed, 10607 passed, 32 skipped

veox · 2019-09-23T19:16:46Z

Semi-OT: may I also note that in the test durations, there's

42.96s call     tests/json-fixtures/test_blockchain.py::test_blockchain_fixtures[/home/circleci/repo/fixtures/BlockchainTests/GeneralStateTests/stCreate2/Create2Recursive.json:Create2Recursive_d0g0v0_Istanbul:Istanbul]
41.59s call     tests/json-fixtures/test_blockchain.py::test_blockchain_fixtures[/home/circleci/repo/fixtures/BlockchainTests/GeneralStateTests/stCreate2/Create2Recursive.json:Create2Recursive_d0g1v0_Istanbul:Istanbul]
26.76s call     tests/json-fixtures/test_blockchain.py::test_blockchain_fixtures[/home/circleci/repo/fixtures/BlockchainTests/GeneralStateTests/stStaticCall/static_Call50000.json:static_Call50000_d0g0v0_Istanbul:Istanbul]

Perhaps these should be marked SLOW_TESTS as well.

OTOH, the entire "run tox" section is 02:26, so maybe that's fine.

…ost_stae` to handle this case. renamed existing method `normalize_post_state` to `normalize_post_state_hash`

voith · 2019-09-26T09:38:08Z

OTOH, the entire "run tox" section is 02:26, so maybe that's fine.

Yeah CI runs them in parallel. 2:26 doesn't seem too long to me.

@veox I could borrow a little help from you. This is regarding the failing tests in Istanbul. I did not get time to look closely into the issue but I thought that I'd gather some information before I'd start digging. Until now I have figured out that the state_root for the computed block header and the one from the tests fixtures don't match. This happens when the storage is empty. I remember that you faced a similar issue while updating tests for Constantinople. Can you describe that issue in brief to me or do you have any hunch if the two are related?

veox · 2019-09-26T13:45:41Z

@voith I don't remember the specifics (poor memory). I think it was Trinity not applying new EIP-1283 gas refund rules to pre-compiles (as it rightfully shouldn't); but again, not even sure on that.

Until now I have figured out that the state_root for the computed block header and the one from the tests fixtures don't match. This happens when the storage is empty.

This can happen for any reason. Different gas use/stipend if storage is empty, or a different value being written to storage (0 instead of 1, say - resulting in, again, different gas usage)... Need to dig into specific tests that fail.

Copy-paste from gitter:

Sorry I can't help better with [this PR] ATM. I made some bad choices y-day, and ended up with a hangover.

But from the failing test names, it looks like the same bunch as already in INCORRECT_UPSTREAM_TESTS.

There's still the disconnect between py-evm and all other implementations - including parity-ethereum, which caved in and did it like go-ethereum does.

The particular issue is not covered in any EIP, and is about a "synthetic" state of a contract that can't be arrived to by regular EVM operation.

So, perhaps add the _Istanbulvariants of InitCollision et. al. to INCORRECT_UPSTREAM_TESTS, and see what actual issues remain.

cburgdorf · 2019-10-02T07:29:51Z

eth/tools/fixtures/helpers.py

@@ -156,7 +161,7 @@ def chain_vm_configuration(fixture: Dict[str, Any]) -> Iterable[Tuple[int, Type[
            (0, SpuriousDragonVM),
            (5, ByzantiumVM),
        )
-    elif network == 'ByzantiumToConstantinopleAt5':
+    elif network == 'ByzantiumToConstantinopleFixAt5':


This looks suspicious, the previous name ByzantiumToConstantinopleAt5 was correctly reflected with the selected VMs ByzantiumVM and ConstantinopleVM. But now the transition seems to go from Byzantium to ConstantinopleFix where ConstantinopleFix is the ethereum/test way to refer to PetersburgVM.

Thanks for catching this. I need to verify this again. But this was something that I had changed just to get the tests passing. Since the tests didn't complain, I didn't forget to verify this change. If the tests have in fact been renamed then I need to change line 167 to PetersburgVM 👍 . I will verify this and make sure that I haven't missed any case(since tests didn't raise ValueError, I assume I didn't).

I've grepped fixtures, and there are no longer instances of string ByzantiumToConstantinopleAt5 there.

The upstream test checks for main-net transition from ByzantiumVM to PetersburgVM, it seems. These two files (just one generated test file):

fixtures/BlockchainTests/TransitionTests/bcByzantiumToConstantinopleFix/ConstantinopleFixTransition.json: "network" : "ByzantiumToConstantinopleFixAt5", fixtures/src/BlockchainTestsFiller/TransitionTests/bcByzantiumToConstantinopleFix/ConstantinopleFixTransitionFiller.json: "network" : "ByzantiumToConstantinopleFixAt5",

cburgdorf · 2019-10-02T07:49:47Z

@voith You pinged me for a review. I skimmed over this and left a view comments. I noticed the failing tests but didn't get into debugging those. Do you need help debugging them? Do you know anything about the failures yet? I know upgrading these tests is more like an art than a craft so I'm trying to gather as much info from you as I can before I start diving in ;)

voith · 2019-10-02T08:35:51Z

@cburgdorf I triggered the review button by mistake. I've been meaning to sit and debug these tests but haven't got time.

Do you need help debugging them?

Actually, I wanted to do this by myself. On Saturday I tried debugging for a while and I ended up learning a lot about the VM. But I only ended up learning about the VM, didn't debug anything. But I will be unable to get back to this anytime soon.
But I understand very well that this PR is quite urgent, so please feel free to continue this. If no one takes it up then I'll surely complete it the following week after Devcon(Too late, I know).

trying to gather as much info from you as I can before I start diving in

So I didn't get too far with debugging this. But here are a few pointers:

For now, I have marked 5 tests as incorrect. These tests are of the same type that @veox marked as Incorrect while trying to update tests for Constantinople.
The RevertPrecompiledTouch failing tests need to debugged. Here's a comment from the ethereum/tests gitter channel that might help you.
Istanbul has some tests for blake, some of which are very very slow. These need to be marked slow. But I haven't spent time trying to identify which of these are slow.

cburgdorf · 2019-10-02T09:25:03Z

I triggered the review button by mistake.

Ah, no problem!

I've been meaning to sit and debug these tests but haven't got time.
But I understand very well that this PR is quite urgent, so please feel free to continue this. If no one takes it up then I'll surely complete it the following week after Devcon(Too late, I know).

To be honest, I'm not sure if I could complete it any earlier. I'm knee deep in adding Görli support and would rather like to avoid the context switch if I can 😅

So, carry on and if it's helpful we can also stick heads together at devcon or the days right after 👍

veox · 2019-10-03T20:48:53Z

FTR, I started looking into this, too.

Failing locally:

test_blockchain_fixtures[/home/veox/src/py-evm/fixtures/BlockchainTests/GeneralStateTests/stRevertTest/RevertPrecompiledTouch.json:RevertPrecompiledTouch_d0g0v0_Istanbul:Istanbul]
test_blockchain_fixtures[/home/veox/src/py-evm/fixtures/BlockchainTests/GeneralStateTests/stRevertTest/RevertPrecompiledTouchExactOOG.json:RevertPrecompiledTouchExactOOG_d31g2v0_Istanbul:Istanbul]
test_blockchain_fixtures[/home/veox/src/py-evm/fixtures/BlockchainTests/GeneralStateTests/stRevertTest/RevertPrecompiledTouch_storage.json:RevertPrecompiledTouch_storage_d0g0v0_Istanbul:Istanbul]
test_blockchain_fixtures[/home/veox/src/py-evm/fixtures/BlockchainTests/GeneralStateTests/stRevertTest/RevertPrecompiledTouch.json:RevertPrecompiledTouch_d3g0v0_Istanbul:Istanbul]
test_blockchain_fixtures[/home/veox/src/py-evm/fixtures/BlockchainTests/GeneralStateTests/stRevertTest/RevertPrecompiledTouchExactOOG.json:RevertPrecompiledTouchExactOOG_d7g2v0_Istanbul:Istanbul]
test_blockchain_fixtures[/home/veox/src/py-evm/fixtures/BlockchainTests/GeneralStateTests/stRevertTest/RevertPrecompiledTouch_storage.json:RevertPrecompiledTouch_storage_d3g0v0_Istanbul:Istanbul]

Those look suspiciously like the existing cases where "synthetic" state stays cleared even after a REVERT.

I vaguely remember someone from go-ethereum arguing that "this is valid because EIP-158 ("State clearing")", but that didn't seem ~~like valid logic~~ correct to me, since EIP-158 was long before the introduction of REVERT.

(EDIT: I did see how that can be seen as "valid logic", based on which order the rules are applied in: first the "clear on touch", or first the "no change on revert". I'll still maintain that py-evm is correct here. q:D)

Or maybe that's just a faulty memory chip.

Anyway, I hope to look into EVM stack traces tomorrow, see if it's "same old" or a genuine new bug.

voith · 2019-10-15T13:29:22Z

superseded by #1858.

update upstream tests for Istanbul

fe74e9b

voith added 4 commits September 22, 2019 20:40

updated fixtures and handle case when postState is a string

c023144

add more gas for sstore_uint64 tests

d1b657e

change ByzantiumToConstantinopleAt5 to ByzantiumToConstantinopleFixAt5

b3eb09c

update gas for create_empty_contract

b413fa9

voith force-pushed the update-istanbul-fixture branch 3 times, most recently from 62c1fa4 to d7bd8c1 Compare September 22, 2019 17:42

voith commented Sep 22, 2019

View reviewed changes

mark randomStatetest94 as slow tests

4b96de7

voith force-pushed the update-istanbul-fixture branch from b142505 to 4b96de7 Compare September 23, 2019 07:23

add istanbul blockchain tests to CI

0d0c58c

fix copy/paste

voith force-pushed the update-istanbul-fixture branch from e864aee to 0d0c58c Compare September 23, 2019 07:53

voith mentioned this pull request Sep 23, 2019

[json-tests] stRevertTest blockchain test failures openethereum/parity-ethereum#11073

Closed

Some fixtures do not have post state. Added a normalizer `normalize_p…

cbaeba7

…ost_stae` to handle this case. renamed existing method `normalize_post_state` to `normalize_post_state_hash`

voith requested a review from cburgdorf October 1, 2019 19:30

cburgdorf reviewed Oct 2, 2019

View reviewed changes

mark some tests as incorrect

cd3e634

veox mentioned this pull request Oct 4, 2019

Tracking: some upstream test fixtures fail when updating to v7.0.0-beta.1 #1857

Closed

veox mentioned this pull request Oct 9, 2019

Update upstream fixtures to v7.0.0 beta.1 + fix consensus failures #1858

Closed

2 tasks

voith closed this Oct 15, 2019

voith deleted the update-istanbul-fixture branch October 15, 2019 13:29

voith mentioned this pull request Nov 7, 2019

Upgrade to py-evm 0.3.0a8 ethereum/trinity#1255

Merged

veox mentioned this pull request Dec 18, 2019

Do not skip tests for IstanbulVM ethereum/trinity#1372

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Update upstream tests for Istanbul #1852

[WIP] Update upstream tests for Istanbul #1852

voith commented Sep 18, 2019 •

edited

Loading

voith commented Sep 21, 2019 •

edited

Loading

voith commented Sep 22, 2019 •

edited

Loading

voith Sep 22, 2019

cburgdorf Sep 23, 2019

voith commented Sep 23, 2019

voith commented Sep 23, 2019 •

edited

Loading

veox commented Sep 23, 2019 •

edited

Loading

voith commented Sep 26, 2019

veox commented Sep 26, 2019 •

edited

Loading

cburgdorf Oct 2, 2019

voith Oct 2, 2019

veox Oct 3, 2019 •

edited

Loading

cburgdorf commented Oct 2, 2019

voith commented Oct 2, 2019

cburgdorf commented Oct 2, 2019

veox commented Oct 3, 2019 •

edited

Loading

voith commented Oct 15, 2019

[WIP] Update upstream tests for Istanbul #1852

[WIP] Update upstream tests for Istanbul #1852

Conversation

voith commented Sep 18, 2019 • edited Loading

What was wrong?

How was it fixed?

To-Do

Cute Animal Picture

voith commented Sep 21, 2019 • edited Loading

voith commented Sep 22, 2019 • edited Loading

voith Sep 22, 2019

Choose a reason for hiding this comment

cburgdorf Sep 23, 2019

Choose a reason for hiding this comment

voith commented Sep 23, 2019

voith commented Sep 23, 2019 • edited Loading

veox commented Sep 23, 2019 • edited Loading

voith commented Sep 26, 2019

veox commented Sep 26, 2019 • edited Loading

cburgdorf Oct 2, 2019

Choose a reason for hiding this comment

voith Oct 2, 2019

Choose a reason for hiding this comment

veox Oct 3, 2019 • edited Loading

Choose a reason for hiding this comment

cburgdorf commented Oct 2, 2019

voith commented Oct 2, 2019

cburgdorf commented Oct 2, 2019

veox commented Oct 3, 2019 • edited Loading

voith commented Oct 15, 2019

voith commented Sep 18, 2019 •

edited

Loading

voith commented Sep 21, 2019 •

edited

Loading

voith commented Sep 22, 2019 •

edited

Loading

voith commented Sep 23, 2019 •

edited

Loading

veox commented Sep 23, 2019 •

edited

Loading

veox commented Sep 26, 2019 •

edited

Loading

veox Oct 3, 2019 •

edited

Loading

veox commented Oct 3, 2019 •

edited

Loading