Enable state tests for Constantinople

[cburgdorf]

What was wrong?

We need to run the global state tests from ethereum/test for the upcoming constantinople fork

How was it fixed?

• updated fixtures against ethereum/tests@10ab37c which is a WIP branch containing some of the new constantinople tests. We'll most likely have to update this to a different version prior to merge
• configured test_state.py to handle Constantinople tests

We need to keep this PR open and continue to rebase it until we get all tests passing. Also notice that there are some tests failing for other forks as well which either means, we have some evm bugs or just need to adjust the test setup somehow.

Cute Animal Picture

[veox]

The py3{5,6}-vm failures are... "interesting".

(A page of reasoning shredded as irrelevant...)

The failure tested for by exp8.json is present in py-evm. Good work, @ehildenb! :D

Will submit micro-fix PR...

[veox]

Regarding the state CI run failures:

I've tried running test_state_fixtures using latest ethereum/tests branch develop (at ethereum/tests@691680a) and latest py-evm branch master (at fcf73d0), since

• it's starting to see Constantinople-related PRs merged (e.g. create2 init code tests tests#491),
• that's the branch recommended in the README,
• I wanted something less WIP'y.

There were:

77 failed, 9960 passed, 1124 skipped, 30633 deselected

(JIC, gist of lastfailed)

---

EDITed, a lot:

I mean, it seemed to me at first that the failures are due to enabling ConstantinopleVM, but they're not!

[pipermerriam]

#1217 has been merged so this can be rebased.

[veox]

PR #1224 bumped fixtures, so this PR should be rebased against master. Should show 74 failures in CI job py36-rpc-state-byzantium (I think).

[cburgdorf]

@veox I didn't look into this yet but I rebased this and it doesn't show the 74 failures that you mentioned we should expect. Just wanted to ping you in case you wanted to look into that (otherwise, I'll do that soon).

Also, I assume the linked tests aren't yet up to date in regard to EIP1234 and EIP1283 because otherwise they should definitely fail as we don't have these yet (with EIP1234 basically being ready in #1303)

[veox]

@cburgdorf I meant they'd show if you rebased against master (to incorporate the few fixes) and bumped fixtures back up - either to latest commit in upstream's develop branch, or that WIP branch you were using originally.

It would then show 74 failures, instead of the 77 it was showing previously. That is, if there weren't new tests added upstream (I think there were) that would fail on py-evm (e.g. because "not implemented", as you note).

[cburgdorf]

@veox Ah gotcha! Thanks for clarifying!

[pipermerriam]

I suspect we can remove the blocked tag from this?

[cburgdorf]

@pipermerriam

I suspect we can remove the blocked tag from this?

Yes, I think so. I'll try to move this forward today. Btw, is it correct to assume that we can not activate the constantinople rpc test before the Constantinople VM actually landed in the mainnet config with a block number. Because, these tests are based on the MainnetFullChain so I suspect they can not work against Constantinople anyway, no?

py-evm/tests/trinity/json-fixtures-over-rpc/test_rpc_fixtures.py

Line 389 in 1aa06da

rpc = RPCServer(MainnetFullChain(None))

[pipermerriam]

@cburgdorf or @lithp (whomever ends up running with this)

Looking at the current failures after the CREATE2 fix was merged by @veox it looks like we may need to do something along the lines of clearing storage for the newly created account as well.

[veox]

@pipermerriam I'm not convinced the CI run has been rebased on current master. I've run the tests locally, and some of those shown as failing by CircleCI pass just fine on my end. :/

---

EDIT: Yup! Checked with a clean repo:

git clone "https://github.com/ethereum/py-evm.git"
cd py-evm
git fetch origin pull/1181/head:pr-1181-enable-state-tests-for-constantinople
git checkout pr-1181-enable-state-tests-for-constantinople
git log

Commit from PR #1560 is not there.

[pipermerriam]

@veox I just rebased on current master and pushed.

[pipermerriam]

Just pushed 010b840 which in theory fixes a few of the remaining failures but causes some other tests to fail that were not failing previously...

[veox]

RevertInCreateInInitCreate2 fails because of

E           AssertionError: State DB did not match expected state on 2 values:
E           0x2adc25665018aa1fe0e6bc666dac8fc2697ff9ba(balance) | Actual: 2000000000000113058 | Expected: 2000000000000128058 | Delta: 15000
E            - 0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b(balance) | Actual: 429496616542 | Expected: 429496601542 | Delta: -15000

(0x2adc.. is miner, 0xa94f.. is tx origin.)

Seems like the "Delta" above has incorrect sign (should be Actual minus Expected); also 15000 is strong pointer to miscalculation in new SSTORE metering.

Trace shows that post-REVERT there are these SSTOREs:

   TRACE  12-10 23:26:06            logging.py  OPCODE: 0x55 (SSTORE) | pc: 19
   TRACE  12-10 23:26:06            logging.py  GAS CONSUMPTION: 42949585131 - 200 -> 42949584931 (SSTORE: 0x6295ee1b4f6dd65047762f924ecd367c17eabf8f[2] -> 0 (current: 0 / original: 0))
...
   TRACE  12-10 23:26:06            logging.py  OPCODE: 0x55 (SSTORE) | pc: 23
   TRACE  12-10 23:26:06            logging.py  GAS CONSUMPTION: 42949584926 - 5000 -> 42949579926 (SSTORE: 0x6295ee1b4f6dd65047762f924ecd367c17eabf8f[0] -> 32 (current: 1 / original: 1))
...
   TRACE  12-10 23:26:06            logging.py  OPCODE: 0x55 (SSTORE) | pc: 36
   TRACE  12-10 23:26:06            logging.py  GAS CONSUMPTION: 42949579902 - 20000 -> 42949559902 (SSTORE: 0x6295ee1b4f6dd65047762f924ecd367c17eabf8f[1] -> 1122867 (current: 0 / original: 0))

[veox]

Should a check for collisions be made first?..

EDIT: E.g., same as on line 010b840#diff-8b72e7215cd1bf65062b0a7071e3ded0R175

EDIT2: Seems like no; at least a naiive copy-paste didn't help here.

[sorpaas]

@veox We skipped two RevertInCreateInInit tests in Parity as well. The issue is that those tests create some artificial storage items for some accounts where there's zero balance, nonce and code. Geth/Aleth would not revert storage items in case a contract creation fails, but Parity/py-evm would (I think you also mentioned the same error in another thread).

This is not a problem for Constantinople because it cannot happen on mainnet, but for the long-term future, we probably still want to find some resolutions.

[pipermerriam]

I'm good with skipping the tests mentioned by @sorpaas (but lets be sure to add a beefy comment explaining why and linking at minimum to this thread).

[veox]

@pipermerriam py-evm is already skipping the RevertInCreateInInit test that @sorpaas mentions. The facility for marking an upstream test "invalid" was added in PR #1224, as INCORRECT_UPSTREAM_TESTS, for both "state tests" and "blockchain tests":

https://github.com/ethereum/py-evm/blob/trinity-v0.1.0-alpha.17/tests/json-fixtures/test_state.py#L146

https://github.com/ethereum/py-evm/blob/trinity-v0.1.0-alpha.17/tests/json-fixtures/test_blockchain.py#L47

(Note that the latter is currently "masked" anyway, but would no longer be if PR #1573 is merged.)

---

However, in the comment above, I am not talking about RevertInCreateInInit; I'm talking about a much newer CREATE2-specific test, RevertInCreateInInitCreate2.

It seems that, indeed, just like the regular CREATE variant, this CREATE2 variant has a "synthetic" state:

https://github.com/ethereum/tests/blob/v6.0.0-beta.2/BlockchainTests/GeneralStateTests/stCreate2/RevertInCreateInInitCreate2_d0g0v0.json#L94-L101

For ref, the comment explaining what's odd here can be found at #1224 (comment).

[veox]

Just pushed 010b840 which in theory fixes a few of the remaining failures but causes some other tests to fail that were not failing previously...

I can not corroborate this, at least not when running tox py36-native-state-constantinople.

The diff between before commit 010b840 and after is:

<  test_state_fixtures[/home/veox/src/py-evm/fixtures/GeneralStateTests/stCreate2/create2collisionStorage.json:create2collisionStorage:Constantinople:0] 
<  test_state_fixtures[/home/veox/src/py-evm/fixtures/GeneralStateTests/stCreate2/create2collisionStorage.json:create2collisionStorage:Constantinople:1] 
<  test_state_fixtures[/home/veox/src/py-evm/fixtures/GeneralStateTests/stCreate2/create2collisionStorage.json:create2collisionStorage:Constantinople:2] 

(That is, three nodeIDs of a single test create2collisionStorage have been fixed.)

That said, my summary line (for the "post" run) is

============ 176 failed, 5142 passed, 30 skipped in 1561.03 seconds ============

..., whereas the CircleCI run shows

============= 10 failed, 2146 passed, 30 skipped in 58.59 seconds ==============

---

Running pytest tests/json-fixtures/test_state.py --fork Constantinople (like the CircleCI run) has the same success/failure counts (that is, 176 post-010b840). Not sure what's going on here.

[sorpaas]

@veox It may be still be that issue. Parity skipped two, one is RevertInCreateInInit and the other is RevertInCreateInInitCreate2. (Sorry if I wasn't clear in the previous comment!)

If you want to confirm, check the gas cost for SSTORE in those two tests. For those artificial items accessed, geth/aleth acted as if the original value is zero, but parity/py-evm acted as if the original value is the artificial values set.

[veox]

@sorpaas You are right - I didn't look at the link you posted, and so talked past you. Sorry.

In same comment, second section I confirm what you're saying.

[veox]

I've fixed the conflicts and continued in PR #1579.