Skip to content
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

Merged
merged 8 commits into from
May 11, 2021
Merged

Add fork transition spec tests #2363

merged 8 commits into from
May 11, 2021

Conversation

ralexstokes
Copy link
Member

@ralexstokes ralexstokes commented Apr 28, 2021

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:

  1. Every slot before, through, and after the slot has a block.
  2. Missing the first block of the fork but otherwise all blocks.
  3. Missing the last block of the fork but otherwise all blocks.
  4. Missing all blocks until the first slot of the second epoch of the fork.
  5. No blocks at all

Other test ideas (or chime in w/ more below!):

  • Random sets of blocks across the boundary
  • Alter finality across the fork boundary
  • Perturb validator balances across the fork

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

Copy link
Contributor

@djrtwo djrtwo left a 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/formats/transition/README.md Outdated Show resolved Hide resolved
tests/formats/transition/README.md Outdated Show resolved Hide resolved
tests/formats/transition/README.md Outdated Show resolved Hide resolved
tests/generators/transition/main.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ajsutton ajsutton left a 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 Show resolved Hide resolved
Comment on lines 21 to 28
```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.
Copy link
Contributor

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.

Copy link
Member Author

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...

@hwwhww hwwhww added this to the v1.1.0-alpha.4 milestone May 4, 2021
@ralexstokes
Copy link
Member Author

ralexstokes commented May 6, 2021

note: this may need to be updated if we merge #2391

Copy link
Contributor

@djrtwo djrtwo left a 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 :)

@djrtwo djrtwo merged commit 5074fca into ethereum:dev May 11, 2021
@ralexstokes ralexstokes deleted the add-altair-fork-transition-test branch May 11, 2021 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants