feat(spec-tests, spec-tools): run fork-transition fixtures through EELS - #3564
jochem-brouwer wants to merge 6 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #3564 +/- ##
===================================================
+ Coverage 94.39% 94.49% +0.10%
===================================================
Files 624 624
Lines 36927 36969 +42
Branches 3326 3334 +8
===================================================
+ Hits 34857 34935 +78
+ Misses 1459 1435 -24
+ Partials 611 599 -12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fork-transition tests are filled in CI, which executes the spec block by block through the in-process t8n. The resulting fixtures were never replayed through the spec's `state_transition`: a fixture whose network names a transition, such as `BPO2ToAmsterdamAtTime15k` or `BerlinToLondonAt5`, is not in the fork table, so the json-loader job and the `validate-blocks` step of the fill jobs dropped it at collection. Spec code that runs only in that block-level path at a fork boundary, header validation against the previous fork's parent header, `apply_fork`, and fork-block detection from `FORK_CRITERIA`, was therefore not exercised across a boundary in CI. Parse the transition name into the starting fork, the activating fork, and the activation criteria. Run the genesis and the blocks before the activation on the starting fork, hand the chain to the activating fork through `apply_fork` at the first block that meets the criteria, and set that fork's `FORK_CRITERIA` to the criteria for the duration of the test so it detects its own fork block. Blocks that carry only their RLP, as invalid blocks do, are placed by the number and timestamp read from the encoded header. Add an `apply_fork` accessor to `ForkLoad` and unit tests for the network parsing.
63c8c7d to
9777a1a
Compare
jochem-brouwer
added
the
backport benchmarks/amsterdam
…e EIP-8253 case map Add a `nonce-balance-storage` variant to the EXTCODEHASH empty-account test: a codeless account with storage and nonce one fails the EIP-684 creation check, so its storage can never be adopted and the case is valid at every fork. Bring `test_cases.md` in line with the consolidated tests: the renamed `test_targeted_accounts_without_storage`, the activation delay on the bump test, the synthetic factory wording, and the fact that the spec's own fork-block detection is exercised when `validate-blocks` replays transition fixtures (ethereum#3564).
|
Follow-up on #3552, now also tests transitions to directly add this to coverage. CC @danceratopz 😄 👍 This was found in #3535 with a low coverage warning despite having #3552 on there. |
spencer-tb
left a comment
spencer-tb
left a comment
There was a problem hiding this comment.
Woo! Lets go! Love to see coverage increments even the smallest amounts haha
LGTM, didn't see anything obvious (claude/codex like it also)!
|
Nothing sticks out, but review incoming :-) |
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.
danceratopz
left a comment
danceratopz
left a comment
There was a problem hiding this comment.
Nice, thanks Jochem! Suggestions for the three larger points are in jochem-brouwer#9.
Can you update the description? It says fork-block detection from FORK_CRITERIA is now exercised across a boundary in CI. It is not yet: only London and the DAO fork read it in their fork module and no London transition test exists, so that part becomes live with #3535.
fork_loader.py also gains accessors in #3535, it'll need a rebase if we apply the suggestions.
| from .exceptional_test_patterns import exceptional_blockchain_test_patterns | ||
| from .fixtures import Fixture, FixturesFile, FixtureTestItem | ||
|
|
||
| TRANSITION_NETWORK = re.compile( |
There was a problem hiding this comment.
This can be a lookup into the testing package's transition forks, which already declare these facts and whose names are the fixture networks: danceratopz@193b416
| fork activates at the given timestamp (`AtTime`) or block number (`At`). | ||
| """ | ||
|
|
||
| HEADER_NUMBER_INDEX = 8 |
There was a problem hiding this comment.
The filler already writes the decoded block of every decodable invalid block under rlp_decoded, so the header can be read from there instead of walking the RLP: danceratopz@a3645c7
| with a matching chain configuration would. | ||
| """ | ||
| fork_module = load.fork.hardfork.module("fork") | ||
| if hasattr(fork_module, "FORK_CRITERIA"): |
There was a problem hiding this comment.
Only London and the DAO fork bind FORK_CRITERIA in their fork module, so this is a no-op for every transition fixture CI fills today. Patching the package attribute as well, with a unit test: danceratopz@778aee1
| for json_block in json_data["blocks"]: | ||
| if ( | ||
| self.transition is not None | ||
| and current is not load |
There was a problem hiding this comment.
Optional renaming suggestion.
load now means the activating fork's loader while current carries the old meaning, so current is not load reads as an implicit "not handed over yet". Renaming load to activating, or keeping an explicit pending transition, would make these lines self-describing. Two smaller ones in the same spirit: _schedule_fork could return a context manager instead of mutating the caller's stack, and mock_pow can stay a bool, since proof_of_stake cannot differ across any transition the loader accepts.
…xtures-suggestions refactor(spec-tests): simplify fork-transition handling in the json-loader
3 participants
Description
Teach the json-loader to run fork-transition fixtures through EELS.
Fork-transition tests are filled in CI, which executes the spec block by block through the in-process t8n. The resulting fixtures were never replayed through the spec's
state_transition, though: a fixture whosenetworknames a transition, such asBPO2ToAmsterdamAtTime15korBerlinToLondonAt5, is not in the fork table, so the json-loader job and thevalidate-blocksstep from #3552 dropped it at collection. Spec code that runs only in that block-level path at a fork boundary, header validation against the previous fork's parent header,apply_fork, and fork-block detection fromFORK_CRITERIA, was therefore not exercised across a boundary in CI.The loader now parses the transition name into the starting fork, the activating fork and the activation criteria (timestamp for
AtTime, block number forAt). The genesis and the blocks before the activation run on the starting fork. The first block that meets the criteria hands the chain to the activating fork through itsapply_fork, the same hook the mainnet sync tool uses, and the remaining blocks run there. For the duration of the test the activating fork'sFORK_CRITERIAis set to the fixture's criteria, the way a client's chain configuration would schedule it, so the fork detects its own fork block. A transition fixture is collected when the fork it activates is among the requested forks.ForkLoadgains anapply_forkaccessor, and the network parsing has unit tests.Verified locally by filling every
not slow and primary_formattransition test up to Amsterdam and runningjust validate-blocksover the result; non-transition fixtures are unaffected.Related Issues or PRs
Prepares for #3535, whose fork-block nonce bump can only be exercised in the spec by a transition fixture.
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