From 3fef4bd3d596eb4a16fc63ae4c6795bfaa03b641 Mon Sep 17 00:00:00 2001 From: Noel Maersk Date: Thu, 30 Aug 2018 00:56:13 +0300 Subject: [PATCH 1/5] fixtures: update to db8ae9de91c303caad3eb9354015c07a0ec3adc3 Rolls forwards up to 2018-01-22. Passing of test_state_fixtures determined using `git bisect run`. --- fixtures | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fixtures b/fixtures index 47b09f42c0..db8ae9de91 160000 --- a/fixtures +++ b/fixtures @@ -1 +1 @@ -Subproject commit 47b09f42c0681548a00da5ab1c98808b368af49a +Subproject commit db8ae9de91c303caad3eb9354015c07a0ec3adc3 From b11a7b8887828d397aeb67dbcbc9e549f4ee1e81 Mon Sep 17 00:00:00 2001 From: Noel Maersk Date: Mon, 3 Sep 2018 17:45:23 +0300 Subject: [PATCH 2/5] tests/test_state_fixtures(): bump fixtures and mark new test `xfail`. The upstream generated test is not sufficiently specific, and it's hard to determine which of the two implementations is incorrect. The principal author of the test case (Yoichi Hirai, github @pirapira) seems currently unavailable, so it's difficult for me to get specific details. --- fixtures | 2 +- tests/json-fixtures/test_state.py | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/fixtures b/fixtures index db8ae9de91..9b1f07c58a 160000 --- a/fixtures +++ b/fixtures @@ -1 +1 @@ -Subproject commit db8ae9de91c303caad3eb9354015c07a0ec3adc3 +Subproject commit 9b1f07c58a70d1b17c4489c49eb9bebf4a27d290 diff --git a/tests/json-fixtures/test_state.py b/tests/json-fixtures/test_state.py index d35597e176..9e42524b5b 100644 --- a/tests/json-fixtures/test_state.py +++ b/tests/json-fixtures/test_state.py @@ -133,6 +133,19 @@ def expand_fixtures_forks(all_fixtures): } +# These are tests that are thought to be incorrect or buggy upstream, +# at the commit currently checked out in submodule `fixtures`. +# Ideally, this list should be empty. +# WHEN ADDING ENTRIES, ALWAYS PROVIDE AN EXPLANATION! +INCORRECT_UPSTREAM_TESTS = { + # Upstream seems to specify that the precompile call fails, but `py-evm` + # handles it just fine. + # * https://github.com/ethereum/py-evm/pull/1224#issuecomment-417351843 + # * https://github.com/ethereum/tests/pull/405#issuecomment-417855812 + ('stReturnDataTest/modexp_modsize0_returndatasize.json', 'modexp_modsize0_returndatasize', 'Byzantium', 4), # noqa: E501 +} + + def mark_statetest_fixtures(fixture_path, fixture_key, fixture_fork, fixture_index): fixture_id = (fixture_path, fixture_key, fixture_fork, fixture_index) if fixture_path.startswith('stTransactionTest/zeroSigTransa'): @@ -144,6 +157,8 @@ def mark_statetest_fixtures(fixture_path, fixture_key, fixture_fork, fixture_ind return pytest.mark.skip("Skipping slow test") elif fixture_path.startswith('stQuadraticComplexityTest'): return pytest.mark.skip("Skipping slow test") + elif fixture_id in INCORRECT_UPSTREAM_TESTS: + return pytest.mark.xfail(reason="Listed in INCORRECT_UPSTREAM_TESTS.") def pytest_generate_tests(metafunc): From 7752f05d1df95e519d2daab3b971678d7244b3c6 Mon Sep 17 00:00:00 2001 From: Noel Maersk Date: Tue, 4 Sep 2018 14:20:20 +0300 Subject: [PATCH 3/5] eth/precompiles/modexp: fix complexity calc (length^2, not 2^2). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There was a typo in the "complexity" calculation function, a special interim value from EIP-198 used to determine total gas use. The code path was never previously exercised. NOTE: previous-commit "fixtures bump" was to commit: 9b1f07c58a70d1b17c4489c49eb9bebf4a27d290 Squashed commit: tests: update "very big number" in test_modexp_gas_fee_calculation(). ... and also fix that test's name, from "calculTation". The very-big-number is not actually in EIP-198; the latter has this to say: > it’s not possible to provide enough gas to make that computation. That's a bit cryptic, but the gist is that the most that can be represented in a 256-bit number is 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff which is 115792089237316195423570985008687907853269984665640564039457584007913129639935 and that's less than the 10684346944173007063723051170445283632835119638284563472873463025465780712173320789629146724657549280936306536701227228889744512638312451529980055895215896 required by this vector, or even the (erroneous) 708647586132375115992254428253169996062012306153720251921480414128428353393856280 that was in the test previously. --- eth/precompiles/modexp.py | 2 +- tests/core/vm/test_modexp_precompile.py | 4 ++-- tests/json-fixtures/test_state.py | 5 ----- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/eth/precompiles/modexp.py b/eth/precompiles/modexp.py index f971bd4b93..e477ce6013 100644 --- a/eth/precompiles/modexp.py +++ b/eth/precompiles/modexp.py @@ -35,7 +35,7 @@ def _compute_complexity(length): length ** 2 // 4 + 96 * length - 3072 ) else: - return 2 ** 2 // 16 + 480 * length - 199680 + return length ** 2 // 16 + 480 * length - 199680 def _extract_lengths(data): diff --git a/tests/core/vm/test_modexp_precompile.py b/tests/core/vm/test_modexp_precompile.py index 1a2210981b..3d8f32af65 100644 --- a/tests/core/vm/test_modexp_precompile.py +++ b/tests/core/vm/test_modexp_precompile.py @@ -41,11 +41,11 @@ (EIP198_VECTOR_A, 13056), ( EIP198_VECTOR_C, - 708647586132375115992254428253169996062012306153720251921480414128428353393856280, + 10684346944173007063723051170445283632835119638284563472873463025465780712173320789629146724657549280936306536701227228889744512638312451529980055895215896, # noqa: E501 ), ), ) -def test_modexp_gas_fee_calcultation(data, expected): +def test_modexp_gas_fee_calculation(data, expected): actual = _compute_modexp_gas_fee(data) assert actual == expected diff --git a/tests/json-fixtures/test_state.py b/tests/json-fixtures/test_state.py index 9e42524b5b..71d0ee0159 100644 --- a/tests/json-fixtures/test_state.py +++ b/tests/json-fixtures/test_state.py @@ -138,11 +138,6 @@ def expand_fixtures_forks(all_fixtures): # Ideally, this list should be empty. # WHEN ADDING ENTRIES, ALWAYS PROVIDE AN EXPLANATION! INCORRECT_UPSTREAM_TESTS = { - # Upstream seems to specify that the precompile call fails, but `py-evm` - # handles it just fine. - # * https://github.com/ethereum/py-evm/pull/1224#issuecomment-417351843 - # * https://github.com/ethereum/tests/pull/405#issuecomment-417855812 - ('stReturnDataTest/modexp_modsize0_returndatasize.json', 'modexp_modsize0_returndatasize', 'Byzantium', 4), # noqa: E501 } From 11d61b005f73eef3f6ee441228b13ff9b97fa794 Mon Sep 17 00:00:00 2001 From: Noel Maersk Date: Tue, 4 Sep 2018 17:57:50 +0300 Subject: [PATCH 4/5] fixtures: update to 61185fe4b8762118fe9ee318539683b47cb04ed6 + mark RevertInCreateInInit as xfail. Rolls forwards up to 2018-03-01. Break in `RevertInCreateInInit.json` determined by `git bisect run`. The test is marked `xfail` to expicitly highlight the fact. This is done in 3 places - all are run as part of CI. --- fixtures | 2 +- tests/json-fixtures/test_blockchain.py | 18 +++++++++++++++++- tests/json-fixtures/test_state.py | 6 ++++++ .../test_rpc_fixtures.py | 15 +++++++++++++++ 4 files changed, 39 insertions(+), 2 deletions(-) diff --git a/fixtures b/fixtures index 9b1f07c58a..61185fe4b8 160000 --- a/fixtures +++ b/fixtures @@ -1 +1 @@ -Subproject commit 9b1f07c58a70d1b17c4489c49eb9bebf4a27d290 +Subproject commit 61185fe4b8762118fe9ee318539683b47cb04ed6 diff --git a/tests/json-fixtures/test_blockchain.py b/tests/json-fixtures/test_blockchain.py index 5d0de5382a..ce72ba1ca7 100644 --- a/tests/json-fixtures/test_blockchain.py +++ b/tests/json-fixtures/test_blockchain.py @@ -32,17 +32,33 @@ BASE_FIXTURE_PATH = os.path.join(ROOT_PROJECT_DIR, 'fixtures', 'BlockchainTests') +# These are tests that are thought to be incorrect or buggy upstream, +# at the commit currently checked out in submodule `fixtures`. +# Ideally, this list should be empty. +# WHEN ADDING ENTRIES, ALWAYS PROVIDE AN EXPLANATION! +INCORRECT_UPSTREAM_TESTS = { + # The test considers a "synthetic" scenario (the state described there can't + # be arrived at using regular consensus rules). + # * https://github.com/ethereum/py-evm/pull/1224#issuecomment-418775512 + # The result is in conflict with the yellow-paper: + # * https://github.com/ethereum/py-evm/pull/1224#issuecomment-418800369 + ('GeneralStateTests/stRevertTest/RevertInCreateInInit_d0g0v0.json', 'RevertInCreateInInit_d0g0v0_Byzantium'), # noqa: E501 +} + + def blockchain_fixture_mark_fn(fixture_path, fixture_name): if fixture_path.startswith('bcExploitTest'): return pytest.mark.skip("Exploit tests are slow") elif fixture_path == 'bcWalletTest/walletReorganizeOwners.json': return pytest.mark.skip("Wallet owner reorganization tests are slow") + elif (fixture_path, fixture_name) in INCORRECT_UPSTREAM_TESTS: + return pytest.mark.xfail(reason="Listed in INCORRECT_UPSTREAM_TESTS.") def blockchain_fixture_ignore_fn(fixture_path, fixture_name): if fixture_path.startswith('GeneralStateTests'): # General state tests are also exported as blockchain tests. We - # skip them here so we don't run them twice" + # skip them here so we don't run them twice return True diff --git a/tests/json-fixtures/test_state.py b/tests/json-fixtures/test_state.py index 71d0ee0159..91b0ab468e 100644 --- a/tests/json-fixtures/test_state.py +++ b/tests/json-fixtures/test_state.py @@ -138,6 +138,12 @@ def expand_fixtures_forks(all_fixtures): # Ideally, this list should be empty. # WHEN ADDING ENTRIES, ALWAYS PROVIDE AN EXPLANATION! INCORRECT_UPSTREAM_TESTS = { + # The test considers a "synthetic" scenario (the state described there can't + # be arrived at using regular consensus rules). + # * https://github.com/ethereum/py-evm/pull/1224#issuecomment-418775512 + # The result is in conflict with the yellow-paper: + # * https://github.com/ethereum/py-evm/pull/1224#issuecomment-418800369 + ('stRevertTest/RevertInCreateInInit.json', 'RevertInCreateInInit', 'Byzantium', 0), } diff --git a/tests/trinity/json-fixtures-over-rpc/test_rpc_fixtures.py b/tests/trinity/json-fixtures-over-rpc/test_rpc_fixtures.py index f65dbed88c..297a82b270 100644 --- a/tests/trinity/json-fixtures-over-rpc/test_rpc_fixtures.py +++ b/tests/trinity/json-fixtures-over-rpc/test_rpc_fixtures.py @@ -92,6 +92,19 @@ 'DelegateCallSpam_EIP150', ) +# These are tests that are thought to be incorrect or buggy upstream, +# at the commit currently checked out in submodule `fixtures`. +# Ideally, this list should be empty. +# WHEN ADDING ENTRIES, ALWAYS PROVIDE AN EXPLANATION! +INCORRECT_UPSTREAM_TESTS = { + # The test considers a "synthetic" scenario (the state described there can't + # be arrived at using regular consensus rules). + # * https://github.com/ethereum/py-evm/pull/1224#issuecomment-418775512 + # The result is in conflict with the yellow-paper: + # * https://github.com/ethereum/py-evm/pull/1224#issuecomment-418800369 + ('GeneralStateTests/stRevertTest/RevertInCreateInInit_d0g0v0.json', 'RevertInCreateInInit_d0g0v0_Byzantium'), # noqa: E501 +} + RPC_STATE_NORMALIZERS = { 'balance': remove_leading_zeros, 'code': empty_to_0x, @@ -162,6 +175,8 @@ def blockchain_fixture_mark_fn(fixture_path, fixture_name): if not should_run_slow_tests(): return pytest.mark.skip("skipping slow test on a quick run") break + if (fixture_path, fixture_name) in INCORRECT_UPSTREAM_TESTS: + return pytest.mark.xfail(reason="Listed in INCORRECT_UPSTREAM_TESTS.") def pytest_generate_tests(metafunc): From fd26ec65c725d40aaab11e8d87ca8c14a56ef45d Mon Sep 17 00:00:00 2001 From: Noel Maersk Date: Sun, 9 Sep 2018 13:51:50 +0300 Subject: [PATCH 5/5] fixtures: update to f4faae91c5ba192c3fd9b8cf418c24e627786312 Determined state tests as "good" by `git bisect run`. --- fixtures | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fixtures b/fixtures index 61185fe4b8..f4faae91c5 160000 --- a/fixtures +++ b/fixtures @@ -1 +1 @@ -Subproject commit 61185fe4b8762118fe9ee318539683b47cb04ed6 +Subproject commit f4faae91c5ba192c3fd9b8cf418c24e627786312