Update state tests #1396
Update state tests #1396
Conversation
|
Failing tests may be related to ethereum/go-ethereum#16470 |
|
@carver @pipermerriam I've been trying to track this down for hours. From what I see, it seems that the expectations about how the state should be has changed for the scenario where all transactions failed. It seems ethereum/go-ethereum@32f28a9#diff-f53696be8527ac422b8d4de7c8e945c1R149 |
cburgdorf
force-pushed
the
christoph/update/tests
branch
from
c58de66
to
f80381f
Compare
|
Just throwing out some findings to memorize for myself and share with anyone interested: Applying f80381f makes some tests go green that otherwise would fail e.g.: But at the same time, that patch makes other tests go red that succeed without that patch, e.g.: |
tests/json-fixtures/test_state.py
Outdated
| # block and thus, the mechanisms which would touch/create/clear the | ||
| # coinbase account based on the mining reward are present during test | ||
| # generation, but not part of the execution, thus we must artificially | ||
| # touch the coinbase account to satisfy the state root that the tests |
There was a problem hiding this comment.
This last sentence should be updated:
thus we must artificially create the account in VMs prior to the state clearing rules, as well as conditionally cleaning up the coinbase account when left empty in VMs after the state clearing rules came into effect.
cburgdorf
force-pushed
the
christoph/update/tests
branch
from
3b96000
to
8007af4
Compare
|
@pipermerriam CI is green, even though we don't see it due to that rate limiting issue. |
|
kicked the tests (and working on escalating the broader issue so this stops happening) |
| # Primitives | ||
| # | ||
| @functools.lru_cache(maxsize=1024) | ||
| def normalize_int(value): |
There was a problem hiding this comment.
So we now have two copies of every method in this module:
- one in
eth/tools/_utils/normalization.py - one in
eth/tools/fixtures/normalization.py
Can't these be deleted out of the fixtures/normalization module? Otherwise I'm worried the files are going to diverge, and maintenance will be a nightmare.
There was a problem hiding this comment.
Yeah, that wasn't intentional, probably just an accidental leftover from when I shuffled that stuff around a while back. Removing the ones from fixtures/normalization seems correct.
| # generation, but not part of the execution, thus we must artificially | ||
| # create the account in VMs prior to the state clearing rules, | ||
| # as well as conditionally cleaning up the coinbase account when left | ||
| # empty in VMs after the state clearing rules came into effect. |
There was a problem hiding this comment.
Ugh, bummer. Thanks for the writeup.
What was wrong?
State tests are out of date and there seem to be some structural changes to fix before we can move #1181 forward
How was it fixed?
ethereum/testssubmodulesealEnginepropertyCute Animal Picture