Update ethereum/tests fixtures to v6.0.0-beta.2 + fix revealed consensus failures #1579

veox · 2018-12-12T00:31:01Z

What was wrong?

py-evm doesn't yet pass the full Constantinople test suite.

This started as a copy of PR #1181, so that Christoph doesn't have to act as a gatekeeper for every tiny fix I propose.

Closes #1181 as superseded/conflicting.

How was it fixed?

The PR updates the upstream ethereum/tests (git sub-module in dir fixtures), enables tests for Constantinople, and fixes the breaks introduced by that.

NOTE: some test are very slow or consume a lot of memory, making it impossible to pass CI on CircleCI's "open source contributor" tier (the process gets OOM-killed). All of these are in SLOW_TESTS. All tests pass locally for me.

Cute Animal Picture

Source: Valdas Augustinas @ efoto.lt

lithp · 2018-12-12T01:52:47Z

I wonder if a different workflow might make developing easier and give us slightly better PRs.

The current process seems to be: pick one of the failing tests here and try to fix the problem, once you fix it open a PR against master. Once it's merged rebase this branch and repeat.

What if instead we: add an equivalent of SLOWEST_TESTS, maybe it's called `KNOWN_FAILURES'. skip all tests in KNOWN_FAILURES and merge this PR. Then, future PRs can make changes and simultaneously remove the tests they fix from the KNOWN_FAILURES set.

This has some benefits:

PRs automatically document which tests they were designed to fix. This makes git blame a lot more useful. I think I remember @carver wanting this.
No rebasing or playing with branches. Just make a PR, merge it, and you're done.

I don't feel very strongly about this so I'm happy to be shot down, but I can add a commit with KNOWN_FAILURES (I think there are 83ish of them?) to this PR if that sounds reasonable.

veox · 2018-12-12T02:04:37Z

@lithp I'm OK with that approach, too, especially since I'm already sitting on lists of failing tests (didn't share previously because effort). EDIT: With a difference of marking xfail instead of skipping KNOWN_FAILURES.

Please don't push anything to a branch marked [WIP] (that's not yours ^_^).

The change can be done more easily when PR #1577 is merged, so that KNOWN_FAILURES doesn't have to be repeated across pytest tests handling both "blockchain" and "state" upstream tests (seeing how PR #1577 removes the "state" variants).

lithp · 2018-12-12T02:43:15Z

Sounds great, tomorrow morning I'll open a new PR based on #1577. Good call on using xfail instead!

cburgdorf · 2018-12-12T12:23:25Z

@lithp I like your proposed approach. 👍

The biggest risk with that that I see is that we forget a bout the known failures. But I one possible way to deal with that would be to just keep one PR open that removes everything from KNOWN_FAILURES (so CI will fail) and keep it open until we have worked out all of the cases.

At least to me an open PR is the strongest reminder of things that should not be forgotten.

veox · 2018-12-12T13:40:21Z

Regarding failures: it seems that most upstream tests generated from InitCollisionFiller.json fail in the same way that the tests already in INCORRECT_UPSTREAM_TESTS do, due to the "synthetic" state:

https://github.com/ethereum/tests/blob/v6.0.0-beta.2/src/GeneralStateTestsFiller/stSStoreTest/InitCollisionFiller.json#L117-L134

This results in a gas use difference in all four cases generated from that filler.

Only one of the four has a REVERT; the feature being tested here is gas use on collision.

Since there's also a lot of failing tests for SSTORE net gas metering, I'll refrain from classifying this group as "incorrect upstream" or "known failure", but investigate the EIP-1283 failures first.

veox · 2018-12-12T13:46:37Z

Note that, according to the list, there are only ~~four~~ three classes of failures currently:

InitCollision (discussed above);
some EIP-1283 tests, tracked upstream in Storage change tests tests#483 (EDIT: those that use STATICCALL);
stStaticCalls for Constantinople (but not Byzantium!) - haven't looked into yet;
~~9 of bcInvalidHeaderTests (for all chains, not just Constantinople)~~ my bad, see Update ethereum/tests fixtures to v6.0.0-beta.2 + fix revealed consensus failures #1579 (comment).

veox · 2018-12-12T14:08:49Z

The SSTORE failures are the d3 and d8 variants only; these are using STATICCALL:

https://github.com/ethereum/tests/blob/v6.0.0-beta.2/src/GeneralStateTestsFiller/stSStoreTest/sstore_0to0Filler.json#L175-L186

It seems that the STATICCALL failures should be reviewed first.

veox · 2018-12-12T15:00:22Z

From tracing it seems that Constantinople takes no heed of STATICCALL locking state.

E.g., running

pytest tests/json-fixtures/test_blockchain.py -k \
    'test_blockchain_fixtures[/home/veox/src/py-evm/fixtures/BlockchainTests/GeneralStateTests/stStaticCall/static_callBasic_d0g0v0.json:static_callBasic_d0g0v0_Constantinople]'

results in:

   TRACE  12-12 16:55:30            logging.py  OPCODE: 0xfa (STATICCALL) | pc: 15
   TRACE  12-12 16:55:30            logging.py  GAS CONSUMPTION: 978723 - 700 -> 978023 (STATICCALL)
   TRACE  12-12 16:55:30            logging.py  MEMORY: size (0 -> 0) | cost (0 -> 0)
   TRACE  12-12 16:55:30            logging.py  MEMORY: size (0 -> 0) | cost (0 -> 0)
   TRACE  12-12 16:55:30            logging.py  GAS CONSUMPTION: 978023 - 100000 -> 878023 (STATICCALL)
   TRACE  12-12 16:55:30            logging.py  COMPUTATION STARTING: gas: 100000 | from: 0x1000000000000000000000000000000000000000 | to: 0x1000000000000000000000000000000000000001 | value: 0 | depth 1 | static: y
   TRACE  12-12 16:55:30            logging.py  OPCODE: 0x60 (PUSH1) | pc: 0
   TRACE  12-12 16:55:30            logging.py  GAS CONSUMPTION: 100000 - 3 -> 99997 (PUSH1)
   TRACE  12-12 16:55:30            logging.py  OPCODE: 0x60 (PUSH1) | pc: 2
   TRACE  12-12 16:55:30            logging.py  GAS CONSUMPTION: 99997 - 3 -> 99994 (PUSH1)
   TRACE  12-12 16:55:30            logging.py  OPCODE: 0x55 (SSTORE) | pc: 4
   TRACE  12-12 16:55:30            logging.py  GAS CONSUMPTION: 99994 - 200 -> 99794 (SSTORE: 0x1000000000000000000000000000000000000001[1] -> 1 (current: 1 / original: 1))
   TRACE  12-12 16:55:30            logging.py  OPCODE: 0x0 (STOP) | pc: 4
   TRACE  12-12 16:55:30            logging.py  COMPUTATION SUCCESS: from: 0x1000000000000000000000000000000000000000 | to: 0x1000000000000000000000000000000000000001 | value: 0 | depth: 1 | static: y | gas-used: 206 | gas-remaining: 99794

It should have errored out on trying to SSTORE within a STATICCALL frame.

As mentioned before, Byzantium acts as expected in this same test.

This is due to the Constantinople version shadowing the Byzantium version:

py-evm/eth/vm/forks/byzantium/opcodes.py

Lines 115 to 119 in b35c524

    
           opcode_values.SSTORE: as_opcode( 
        
               logic_fn=ensure_no_static(storage.sstore), 
        
               mnemonic=mnemonics.SSTORE, 
        
               gas_cost=constants.GAS_NULL, 
        
           ),

py-evm/eth/vm/forks/constantinople/opcodes.py

Lines 58 to 62 in b35c524

    
           opcode_values.SSTORE: as_opcode( 
        
               logic_fn=sstore_eip1283, 
        
               mnemonic=mnemonics.SSTORE, 
        
               gas_cost=constants.GAS_NULL, 
        
           ),

veox · 2018-12-12T17:11:38Z

... and addressing that magically fixed every "state" failure revealed by CI (well, apart from InitCollision, but that's expected).

The remaining failures that I'm experiencing locally are actually not tested for on master or in this PR, so aren't revealed by CI.

Currently waiting on PR #1577 to get merged, so this PR can get re-worked without bothering with the "state" set of tests.

pipermerriam · 2018-12-12T17:23:58Z

@lithp I'd be fine with your KNOWN_FAILURES approach if it was a dedicated branch that we merged fixes into and then once things are all passing we merge into master, clearing out the KNOWN_FAILURES set. I'm uncomfortable doing it in master for the reason @cburgdorf described. I don't want to forget about them.

veox · 2018-12-12T17:31:50Z

@pipermerriam @lithp ~~I'm now reasonably certain~~ it will not come to KNOWN_FAILURES at all.

~~There's only one class of failure that I haven't looked at or fixed (locally), the bcInvalidHeaderTests.~~

All "blockchain" tests pass locally for me (that's including "passing because xfail" ones that are "invalid upstream"). (The bcInvalidHeaderTests I was getting was due to the hackish way I bypassed block validation to get state diff output.)

I'm not working at getting CI green here, because this work would be overridden by PR #1577 anyway.

pipermerriam · 2018-12-12T19:24:47Z

❤️ @veox

This made the EVM no longer care that SSTORE within a STATICCALL is forbidden. ethereum#1579 (comment)

veox · 2018-12-12T20:51:44Z

eth/vm/forks/constantinople/opcodes.py

@@ -11,7 +11,8 @@
    opcode_values,
 )
 from eth.vm.forks.byzantium.opcodes import (
-    BYZANTIUM_OPCODES
+    BYZANTIUM_OPCODES,
+    ensure_no_static


Could possibly be moved "out" of any fork-specific file.

tests/json-fixtures/test_state.py

tox.ini

2 COMMITS SQUASHED: fixtures: include missing Constantinople in helpers. Debugging CREATE2 using the full "Blockchain" tests in `fixtures` (they are disabled in `py-evm`, because they're slow and a duplication of "State" tests). A few definitions are missing - so add them. tests/conftest: fix VM-tracing log-to-file helper. Also move `import`s out of the helper, so they're not encountered by the interpreter on every invocation of the helper.

This made the EVM no longer care that SSTORE within a STATICCALL is forbidden. ethereum#1579 (comment)

@cburgdorf

This used to be a commit by @cburgdorf, but it got eaten in the rebase (as it had most of the conflicts).

This made the EVM no longer care that SSTORE within a STATICCALL is forbidden. ethereum#1579 (comment)

One is existing `RevertInCreateInInit`, but now for Constantinople, not just Byzantium. The other is `RevertInCreateInInitCreate2`, which contains the same "synthhetic" state which can't be arrived at by regular means in the EVM. It's likely a copy-paste atavism. The rest are variants testing specifically for what happens on collisions with a state like above. It has already been estabilished that there are two schools of thought: one with `geth` and `aleth` (and `testeth` that generates the tests); the other with `py-evm` and `parity`.

veox · 2018-12-13T00:12:40Z

Rebased branch on latest master and reworked it; old variant backed up in https://github.com/veox/py-evm/tree/update-fixtures-to-v6.0.0-beta.2-old.

(The cherry-picking and reordering didn't go well for Github's ability to display the current state.)

veox · 2018-12-13T01:23:44Z

Python3.6 + Constantinople tests fail because they gobble up memory; here's a gist of durations running

pytest --fork Constantinople \
    --numprocesses auto --durations 50 \
    tests/json-fixtures/test_blockchain.py

(This saw a peak usage of 8 GiB of RAM for me.)

A lot of these have "50000" in their names; wonder if the tests run their operations 50 thousand times each...

Will update SLOW_TESTS tomorrow.

Python3.5 + Constantinople don't exhibit this.

tests/json-fixtures/test_blockchain.py

SQUASHED: tests/fixtures/blockchain: sort SLOWEST_TESTS alphabetically. ... to reduce line count diff/churn: some are just being moved around needlessly. tests/fixtures/blockchain: add bcForkStressTest to class-disabled.

veox · 2018-12-13T14:58:48Z

tests/json-fixtures/test_blockchain.py

    ('GeneralStateTests/stChangedEIP150/Call1024BalanceTooLow_d0g0v0.json', 'Call1024BalanceTooLow_d0g0v0_EIP150'),  # noqa: E501
-    ('bcForkStressTest/ForkStressTest.json', 'ForkStressTest_EIP150'),  # noqa: E501


This is now disabled as a class, see blockchain_fixture_mark_fn() below.

EDIT: Brought up testing it separately in #1588 (comment).

carver · 2018-12-13T17:25:04Z

tox.ini

@@ -32,6 +32,8 @@ commands=
    beacon: pytest {posargs:tests/beacon/}
    # The following test seems to consume a lot of memory. Restricting to 3 processes reduces crashes
    rpc-state-byzantium: pytest -n3 {posargs:tests/trinity/json-fixtures-over-rpc/test_rpc_fixtures.py -k 'GeneralStateTests and not stQuadraticComplexityTest and Byzantium'}
+    # Uncomment the next line + modify test_rpc_fixtures.py when Constantinople is included in the mainnet config


Old comment can be deleted?

Suggested change

# Uncomment the next line + modify test_rpc_fixtures.py when Constantinople is included in the mainnet config

We can do it in a follow-up. I don't want to do it myself in case I misread the comment.

This was an oversight. It shouldn't have been there - oops.

Kinda figured, but also didn't want to hold up the release over it.

veox force-pushed the update-fixtures-to-v6.0.0-beta.2 branch 2 times, most recently from de1a884 to ff11358 Compare December 12, 2018 00:35

veox mentioned this pull request Dec 12, 2018

Upgrade py-ecc past a buggy release, to 1.4.6 #1576

Merged

veox force-pushed the update-fixtures-to-v6.0.0-beta.2 branch from d0f584a to c14d7f7 Compare December 12, 2018 20:49

veox added a commit to veox/py-evm that referenced this pull request Dec 12, 2018

eth/vm: fix accidental override of ensure_no_static() in Constantinople.

130ebf6

This made the EVM no longer care that SSTORE within a STATICCALL is forbidden. ethereum#1579 (comment)

veox commented Dec 12, 2018

View reviewed changes

tests/json-fixtures/test_state.py Outdated Show resolved Hide resolved

veox commented Dec 12, 2018

View reviewed changes

tox.ini Show resolved Hide resolved

cburgdorf and others added 3 commits December 13, 2018 01:22

Upgrade tests to v6.0.0-beta.2

b49489a

tests: Enable Constantinople transaction tests.

c9f03ec

veox added a commit to veox/py-evm that referenced this pull request Dec 12, 2018

eth/vm: fix accidental override of ensure_no_static() in Constantinople.

fcc3c87

This made the EVM no longer care that SSTORE within a STATICCALL is forbidden. ethereum#1579 (comment)

veox force-pushed the update-fixtures-to-v6.0.0-beta.2 branch from c14d7f7 to a7b3497 Compare December 13, 2018 00:07

veox and others added 4 commits December 13, 2018 02:11

circleci: revive/rework enabling Constantinople runs on CI.

55b2a06

This used to be a commit by @cburgdorf, but it got eaten in the rebase (as it had most of the conflicts).

Fix some failing tests

189d347

eth/vm: fix accidental override of ensure_no_static() in Constantinople.

067308b

This made the EVM no longer care that SSTORE within a STATICCALL is forbidden. ethereum#1579 (comment)

veox force-pushed the update-fixtures-to-v6.0.0-beta.2 branch from a7b3497 to 2b01068 Compare December 13, 2018 00:11

veox changed the title ~~[WIP] Update ethereum/tests fixtures to v6.0.0-beta.2 + fix arising issues~~ Update ethereum/tests fixtures to v6.0.0-beta.2 + fix arising issues Dec 13, 2018

veox force-pushed the update-fixtures-to-v6.0.0-beta.2 branch from bd11197 to c60ba44 Compare December 13, 2018 13:48

veox changed the title ~~Update ethereum/tests fixtures to v6.0.0-beta.2 + fix arising issues~~ [WIP] Update ethereum/tests fixtures to v6.0.0-beta.2 + fix revealed consensus failures Dec 13, 2018

veox changed the title ~~[WIP] Update ethereum/tests fixtures to v6.0.0-beta.2 + fix revealed consensus failures~~ Update ethereum/tests fixtures to v6.0.0-beta.2 + fix revealed consensus failures Dec 13, 2018

veox mentioned this pull request Dec 13, 2018

Add a nightly build to circleci which runs even the slow blockchain tests #1588

Open

veox commented Dec 13, 2018

View reviewed changes

tests/json-fixtures/test_blockchain.py Show resolved Hide resolved

veox commented Dec 13, 2018

View reviewed changes

tests/json-fixtures/test_blockchain.py Outdated Show resolved Hide resolved

veox force-pushed the update-fixtures-to-v6.0.0-beta.2 branch 2 times, most recently from 8ff9816 to 3017dce Compare December 13, 2018 14:53

tests/fixtures/blockchain: repopulate SLOWEST_TESTS.

fd537be

SQUASHED: tests/fixtures/blockchain: sort SLOWEST_TESTS alphabetically. ... to reduce line count diff/churn: some are just being moved around needlessly. tests/fixtures/blockchain: add bcForkStressTest to class-disabled.

veox force-pushed the update-fixtures-to-v6.0.0-beta.2 branch from 3017dce to fd537be Compare December 13, 2018 14:56

veox commented Dec 13, 2018

View reviewed changes

veox mentioned this pull request Dec 13, 2018

Enable state tests for Constantinople #1181

Closed

carver reviewed Dec 13, 2018

View reviewed changes

carver merged commit 55845dd into ethereum:master Dec 13, 2018

This was referenced Dec 13, 2018

EIP1283 - Net gas metering #1105

Closed

Enable state tests for constantinople #1152

Closed

veox deleted the update-fixtures-to-v6.0.0-beta.2 branch October 4, 2019 12:52

fselmo mentioned this pull request Sep 29, 2021

Run London fork tests #2021

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update ethereum/tests fixtures to v6.0.0-beta.2 + fix revealed consensus failures #1579

Update ethereum/tests fixtures to v6.0.0-beta.2 + fix revealed consensus failures #1579

veox commented Dec 12, 2018 •

edited

Loading

lithp commented Dec 12, 2018 •

edited

Loading

veox commented Dec 12, 2018 •

edited

Loading

lithp commented Dec 12, 2018

cburgdorf commented Dec 12, 2018 •

edited

Loading

veox commented Dec 12, 2018 •

edited

Loading

veox commented Dec 12, 2018 •

edited

Loading

veox commented Dec 12, 2018

veox commented Dec 12, 2018 •

edited

Loading

veox commented Dec 12, 2018 •

edited

Loading

pipermerriam commented Dec 12, 2018

veox commented Dec 12, 2018 •

edited

Loading

pipermerriam commented Dec 12, 2018

veox Dec 12, 2018

veox commented Dec 13, 2018 •

edited

Loading

veox commented Dec 13, 2018 •

edited

Loading

veox Dec 13, 2018 •

edited

Loading

carver Dec 13, 2018

carver Dec 13, 2018

veox Dec 13, 2018

carver Dec 13, 2018

		('GeneralStateTests/stChangedEIP150/Call1024BalanceTooLow_d0g0v0.json', 'Call1024BalanceTooLow_d0g0v0_EIP150'), # noqa: E501
		('bcForkStressTest/ForkStressTest.json', 'ForkStressTest_EIP150'), # noqa: E501

Update ethereum/tests fixtures to v6.0.0-beta.2 + fix revealed consensus failures #1579

Update ethereum/tests fixtures to v6.0.0-beta.2 + fix revealed consensus failures #1579

Conversation

veox commented Dec 12, 2018 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

lithp commented Dec 12, 2018 • edited Loading

veox commented Dec 12, 2018 • edited Loading

lithp commented Dec 12, 2018

cburgdorf commented Dec 12, 2018 • edited Loading

veox commented Dec 12, 2018 • edited Loading

veox commented Dec 12, 2018 • edited Loading

veox commented Dec 12, 2018

veox commented Dec 12, 2018 • edited Loading

veox commented Dec 12, 2018 • edited Loading

pipermerriam commented Dec 12, 2018

veox commented Dec 12, 2018 • edited Loading

pipermerriam commented Dec 12, 2018

veox Dec 12, 2018

Choose a reason for hiding this comment

veox commented Dec 13, 2018 • edited Loading

veox commented Dec 13, 2018 • edited Loading

veox Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

carver Dec 13, 2018

Choose a reason for hiding this comment

carver Dec 13, 2018

Choose a reason for hiding this comment

veox Dec 13, 2018

Choose a reason for hiding this comment

carver Dec 13, 2018

Choose a reason for hiding this comment

veox commented Dec 12, 2018 •

edited

Loading

lithp commented Dec 12, 2018 •

edited

Loading

veox commented Dec 12, 2018 •

edited

Loading

cburgdorf commented Dec 12, 2018 •

edited

Loading

veox commented Dec 12, 2018 •

edited

Loading

veox commented Dec 12, 2018 •

edited

Loading

veox commented Dec 12, 2018 •

edited

Loading

veox commented Dec 12, 2018 •

edited

Loading

veox commented Dec 12, 2018 •

edited

Loading

veox commented Dec 13, 2018 •

edited

Loading

veox commented Dec 13, 2018 •

edited

Loading

veox Dec 13, 2018 •

edited

Loading