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

[Mega tracking] - Align Prysm to 0.8.2 #3144

Closed
5 tasks done
prestonvanloon opened this issue Aug 5, 2019 · 6 comments
Closed
5 tasks done

[Mega tracking] - Align Prysm to 0.8.2 #3144

prestonvanloon opened this issue Aug 5, 2019 · 6 comments
Assignees
Labels
Tracking Gotta Catch 'Em All

Comments

@prestonvanloon
Copy link
Member

prestonvanloon commented Aug 5, 2019

Reference: https://github.com/ethereum/eth2.0-specs/releases/tag/v0.8.2
Tasks below copied from the changelog.

Bug fixes:

Other:

I left out ethereum/consensus-specs#1283, ethereum/consensus-specs#1314, ethereum/consensus-specs#1308 as these seem to be typo or English clarification PRs.

@prestonvanloon prestonvanloon added the Tracking Gotta Catch 'Em All label Aug 5, 2019
@terencechain
Copy link
Member

Working on "ignore latest messages in fork choice prior to latest justified"

@prestonvanloon
Copy link
Member Author

New test vectors are here: https://github.com/ethereum/eth2.0-spec-tests/releases/tag/v0.8.2. Note that there are three test bundles instead of one. This will cause a bit of a dependency refactoring / reorganizing.

For SSZ, the test are split into 3 files for each scenario.

  • value.yaml
  • meta.yaml
  • serialized.ssz

The yaml files are still hex encoded strings so they will need preprocessing in the forked repository. The upstream issue (ethereum/consensus-spec-tests#5) is still valid, unfortunately, for SSZ tests.

For the mainnet / minimal tests, the bundles have been split into mainnet.tar.gz and minimal.tar.gz with a copy of the yaml and ssz encoded values. Given that we must continue to preprocess ssz test vectors, I'd argue that we remove the ssz encoded values and continue preprocessing the yaml to convert hex strings into canonical yaml format representation for binary data. This approach is the most minimal impact to current spectest suite.

In spec tests, the paths have change to include longer paths. Example for minimal.tar.gz.

tests/minimal/phase0/epoch_processing/crosslinks/pyspec_tests/double_late_crosslink/pre.ssz
tests/minimal/phase0/epoch_processing/crosslinks/pyspec_tests/double_late_crosslink/post.yaml
tests/minimal/phase0/epoch_processing/crosslinks/pyspec_tests/double_late_crosslink/post.ssz
tests/minimal/phase0/epoch_processing/crosslinks/pyspec_tests/double_late_crosslink/pre.yaml

These were previously:

tests/epoch_processing/crosslinks/crosslinks_minimal.yaml
tests/epoch_processing/crosslinks/crosslinks_mainnet.yaml

In any case the following are required:

  • Updating the dependency bundle (consolidation to a single bundle, preprocessing of yaml (if needed), removing ssz (if needed))
  • Updating the file paths in test
  • Updating tests to read multiple files for input / expected output
  • Update data structures to match new test input / output values

@prestonvanloon
Copy link
Member Author

Options for handling testnet refactor:

  1. Skip updating tests for 0.8.2. There are no substantial requirements here anyway. We can defer this to a later time and potentially wait for a better alternative format.
  2. Adopt the new yamls (preprocessed to replace hex strings with canonical base64).
  3. Adopt ssz encoded binary files (still need to preprocess yaml for ssz tests).

@prestonvanloon
Copy link
Member Author

Based on an offline discussion, we're opting to skip the test vector refactor (option 1) as there is little to no benefit at this time with a substantial change to the codebase.

@prestonvanloon
Copy link
Member Author

We're now targeting all aspects of 0.8.2, including test format.

@rauljordan
Copy link
Contributor

Completed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tracking Gotta Catch 'Em All
Projects
None yet
Development

No branches or pull requests

4 participants