Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Implementation of EIP106 breaks state tests #390

Closed
konradkonrad opened this issue Jul 8, 2016 · 7 comments
Closed

Implementation of EIP106 breaks state tests #390

konradkonrad opened this issue Jul 8, 2016 · 7 comments

Comments

@konradkonrad
Copy link
Contributor

See here: https://travis-ci.org/ethereum/pyethereum/jobs/143355805#L472-L553

EIP 106 was implemented in 03e1c88

ethereum/testutils.py:run_state_tests expects to create a Block object, even from invalid data. Therefore the current implementation of EIP 106 (raising a ValueError when __init__ializing with a too high gas_limit) is not compatible with the test function.
@konradkonrad
Copy link
Contributor Author

@heikoheiko any advise on this one?

@heikoheiko
Copy link
Member

heikoheiko commented Jul 9, 2016 •
edited
Loading

https://github.com/ethereum/tests/blob/develop/StateTests/RandomTests/st201504011547GO.json
I'd say above test is invalid as it tests a state transition where the block prestate is not according to the spec. The EIP106 requirements should be tested in a blocktest.

Above test asserts that an OOG exception is triggered.

As a hotfix I'd propose here to:
https://github.com/ethereum/pyethereum/blob/develop/ethereum/testutils.py#L142
gas_limit=min(2**64-1, parse_int_or_hex(env['currentGasLimit']))
and then set blk.gas_limit = parse_int_or_hex(env['currentGasLimit']) after the constructor.

But again, I think the actual error is with above state test, because it depends on a block prestate which is out of bounds.

@winsvega
Copy link

winsvega commented Jul 9, 2016

you mean current gasLimit that is out of bounds?
"currentGasLimit" : "115792089237316195423570985008687907853269984665640564039457584007913129639935",

@heikoheiko
Copy link
Member

yes

konradkonrad added a commit that referenced this issue Jul 11, 2016
@konradkonrad
Workaround for issue #390
604ec06
konradkonrad added a commit that referenced this issue Jul 11, 2016
@konradkonrad
Workaround for issue #390
c1ef067
konradkonrad added a commit that referenced this issue Jul 11, 2016
@konradkonrad
Workaround for issue #390
5a1fc12
@konradkonrad
Copy link
Contributor Author

thanks @heikoheiko , this works.
@winsvega should I create an issue at ethereum/tests?

konradkonrad added a commit that referenced this issue Jul 11, 2016
@konradkonrad
Bump version: 1.3.6 → 1.4.0
8cfcacb 
Changelog:
  - add py3 compatibility! thanks @pipermerriam!
  - add metropolis config #383
  - add EIP86 #377
  - add EIP98 #378
  - add EIP100 #380
  - add EIP106 see #390
  - fix lazy encoding #352
  - fix tester regression #370
  - some minor refactorings
@winsvega
Copy link

winsvega commented Jul 11, 2016 •
edited
Loading

Yes we could just fix the gasLimit to fit new requirements.
this test was auto-generated before we implemented this restriction.

any more tests in RandomTests folder with same issue?

konradkonrad added a commit that referenced this issue Jul 11, 2016
@konradkonrad
Bump version: 1.3.6 → 1.4.0
01d2cef 
Changelog:
  - add py3 compatibility! thanks @pipermerriam!
  - add metropolis config #383
  - add EIP86 #377
  - add EIP98 #378
  - add EIP100 #380
  - add EIP106 see #390
  - fix lazy encoding #352
  - fix tester regression #370
  - some minor refactorings
@konradkonrad konradkonrad mentioned this issue Jul 11, 2016
Closed
konradkonrad added a commit that referenced this issue Jul 25, 2016
@konradkonrad
Revert "Workaround for issue #390"
f3601a4 
This reverts commit 5a1fc12.
konradkonrad added a commit that referenced this issue Jul 25, 2016
@konradkonrad
Use newest fixtures (should fix #390)
51b450e
konradkonrad added a commit that referenced this issue Jul 27, 2016
@konradkonrad
Get newer version of upstream fixtures #390
4641fd0
konradkonrad added a commit that referenced this issue Jul 27, 2016
@konradkonrad
Try newer fixtures #390
977b258
konradkonrad added a commit that referenced this issue Jul 28, 2016
@konradkonrad
Revert "Workaround for issue #390"
840218c 
This reverts commit 5a1fc12.
konradkonrad added a commit that referenced this issue Jul 28, 2016
@konradkonrad
Use newest fixtures (should fix #390)
c7b1f01
konradkonrad added a commit that referenced this issue Jul 28, 2016
@konradkonrad
Implement out special handling for state tests #390
5c20c13 
There is a not yet handled case of fixtures formatting:
>  If the output data is prefixed with #, the following number represents the size of the output, and not the output
>  directly.
(https://github.com/ethereum/homestead-guide/blob/master/source/contracts-and-transactions/ethereum-tests/state_tests/index.rst)

After updates to `ethereum/tests`, that followed ticket #390, we
now see the cases of this, which are addressed by this commit.
@konradkonrad
Copy link
Contributor Author

I think this is not relevant anymore

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@heikoheiko @konradkonrad @winsvega