-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add fork transition spec tests #2363
Add fork transition spec tests #2363
Conversation
tests/core/pyspec/eth2spec/test/altair/transition/test_transition.py
Outdated
Show resolved
Hide resolved
tests/core/pyspec/eth2spec/test/altair/transition/test_transition.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice! have a handful of high level questions before we dig deeper into the meat of writing tests
tests/core/pyspec/eth2spec/test/altair/transition/test_transition.py
Outdated
Show resolved
Hide resolved
tests/core/pyspec/eth2spec/test/altair/transition/test_transition.py
Outdated
Show resolved
Hide resolved
tests/core/pyspec/eth2spec/test/altair/transition/test_transition.py
Outdated
Show resolved
Hide resolved
tests/core/pyspec/eth2spec/test/altair/transition/test_transition.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test format looks ok for Teku. Left some comments on the FlaggedContainer
but it's not a big deal for us either way - happy to go with what's easiest for other clients.
tests/formats/transition/README.md
Outdated
```python | ||
class FlaggedContainer(Container): | ||
flag: boolean | ||
obj: Container | ||
``` | ||
|
||
If `flag` is `False`, then the `obj` belongs to the **initial** fork. | ||
If `flag` is `True`, then the `obj` belongs to the **post** fork. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that only the blocks are prefixed, I'd be tempted to simplify this and just record in the metadata file the number of first post-fork block. Not the slot number, but more directly recording that the first 5 blocks are pre-fork and the 6th block onwards are all post-fork. That way the test runner can decide ahead of time how to decode the file rather than having to parse the initial boolean and then switching to the appropriate block schema.
That said, in Teku we already have code to extract the slot from blocks without knowing what fork it's from so at least for us we wouldn't have any issues decoding just the raw blocks given we know the fork_epoch. We could probably work with this FlaggedContainer
by just trimming the prefix it adds and then doing our usual fork-agnostic block parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that is not a bad suggestion and simplifies the test format... main tension is how general we want this type of test to be -- can we think of some fork condition where a "fork index" would bind our hands? i can't think of any at the moment....
and the thing w/ taking the slot from the raw block bytes is that the position of the slot
could change, e.g. in a future version of ssz
. so i'd rather stay at a higher level of abstraction than needing to peer into "opaque" BeaconBlock
bytes...
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments on the tests.
Really great work on getting all the wrappers and functions to play nicely together to get a clean pattern here :)
tests/core/pyspec/eth2spec/test/altair/transition/test_transition.py
Outdated
Show resolved
Hide resolved
tests/core/pyspec/eth2spec/test/altair/transition/test_transition.py
Outdated
Show resolved
Hide resolved
tests/core/pyspec/eth2spec/test/altair/transition/test_transition.py
Outdated
Show resolved
Hide resolved
tests/core/pyspec/eth2spec/test/altair/transition/test_transition.py
Outdated
Show resolved
Hide resolved
tests/core/pyspec/eth2spec/test/altair/transition/test_transition.py
Outdated
Show resolved
Hide resolved
tests/core/pyspec/eth2spec/test/altair/transition/test_transition.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Adrian Sutton <adrian@symphonious.net>
Adds a test format for generating spec tests across fork boundaries.
We will immediately want to use this format to generate tests from phase 0 to altair.
Have tests to test transition with:
Other test ideas (or chime in w/ more below!):
Putting this PR up to get feedback on direction, may add further tests here or can open a dependent PR.NOTE: this PR does not currently have the new format enabled in CI