refactor(spec-tests): simplify fork-transition handling in the json-loader - #9
Open
danceratopz wants to merge 3 commits into
Conversation
The json-loader parsed a transition fixture's network name with a regex to recover the starting fork, the activating fork and the activation criteria. The testing package already declares every transition fork with exactly those three facts, and the transition fork's name is the fixture's network, so look the name up there instead. A network that names no transition fork is a plain fork name, as before. A unit test checks that both ends of every transition fork map to a fork of the spec.
…locks To decide which fork an invalid block of a transition fixture belongs to, the json-loader decoded the block's RLP itself and read the number and the timestamp at fixed header positions. The filler already writes the decoded block under `rlp_decoded` for every invalid block whose RLP decodes, which is exactly the case the manual decoding covered, so read that header instead. A block without a decoded header stays with the fork before it, as before, and so does a header whose timestamp does not fit in a `U256`, which previously escaped as an `OverflowError` before the expected exception was checked.
For a transition fixture, the json-loader sets the activating fork's `FORK_CRITERIA` to the fixture's schedule so that the fork detects its own fork block. It patched the name only in the fork's `fork` module, and only when that module binds it, which today is the case for London and the DAO fork alone. For every other fork the helper did nothing, and the fork tooling that reads the constant from the fork package kept reporting the mainnet schedule. Patch the package attribute as well, unconditionally, and keep patching the module's own binding when there is one. Unit tests cover both shapes and check that the patches are undone.
2 tasks
jochem-brouwer
left a comment
Owner
There was a problem hiding this comment.
One question on this one, thanks @danceratopz 😄 👍
| if header is None: | ||
| return False | ||
| number = Uint(int(header["number"], 16)) | ||
| timestamp = int(header["timestamp"], 16) |
Owner
There was a problem hiding this comment.
Why is the conversion here different? number has Uint(int( and timestamp int(. What is the reason for this? 🤔 Why are these not the same?
Is this some kind of overflow error? Then number should also have this check, right?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Three follow-ups to the fork-transition support in the json-loader, one commit each. They do not change which fixtures run or how they run:
just static, the json-loader unit tests, the transition fixtures undertests/json_loader/fixtures, and a fresh fill of the Amsterdam transition tests pass before and after each commit.Resolve transition networks from the testing forks. The loader parsed a fixture's network name with a regex to recover the starting fork, the activating fork and the activation point. The testing package already declares every transition fork with exactly those facts in
forks/transition.py, and the transition fork's name is the fixture's network, so the loader now looks the name up inget_transition_forks(). This removes the regex and theksuffix expansion. A unit test checks that both ends of every transition fork map to a fork of the spec, so a transition fork the spec cannot run fails loudly instead of being dropped at collection.Read the decoded header of invalid transition blocks. To decide which fork an invalid block belongs to, the loader decoded the block's RLP itself and read the number and the timestamp at fixed header positions. The filler writes the decoded block under
rlp_decodedfor every invalid block whose RLP decodes (seeInvalidFixtureBlockinfixtures/blockchain.py), which is exactly the case the manual decoding covered, so the loader reads that header instead. A block without a decoded header stays with the fork before it, as before. A header whose timestamp does not fit in aU256now stays there too; previously it raised anOverflowErrorbefore the expected exception was checked.Schedule transition forks in the fork package as well. The loader set the activating fork's
FORK_CRITERIAonly in the fork'sforkmodule, and only when that module binds the name, which today is the case for London and the DAO fork alone. For every other fork the helper did nothing, and the fork tooling, which reads the constant from the fork package, kept reporting the mainnet schedule during the test. The helper now patches the package attribute unconditionally and the module's own binding when there is one. Two unit tests cover both shapes and check that the patches are undone.The unit tests grow from 9 to 23.
Related Issues or PRs
Suggestions for ethereum#3564. The third change is the seam ethereum#3535 relies on, since its Amsterdam fork module imports
FORK_CRITERIAthe way London's does.Checklist
just static<type>(<area>): <title>, where<type>and<area>come from an appropriateC-<type>, respectivelyA-<area>, label. The title should match the target squash commit message.Cute Animal Picture