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

Enable test generation for phase1 #1957

Merged
merged 27 commits into from
Sep 18, 2020
Merged

Enable test generation for phase1 #1957

merged 27 commits into from
Sep 18, 2020

Conversation

protolambda
Copy link
Contributor

  • Updates SSZ static test gen to deal with phase 1 types too. Needed a minor fix in random-output generation.
  • Update epoch processing, block operations, sanity, rewards test gen to:
    • Output for phase0 like before
    • Output for phase-1 specific tests
    • Run phase0 module tests, with phase 1 spec. (Incompatible tests are handled by the test decorators)
  • Rewards and penalties mainnet epoch processing was still disabled, partially because we have rewards tests, partially because it used to be very slow. Let's try to enable them.
  • Rewards tests are now split into random/basic/leak, instead of all in core. We can change that back, but it seemed more consistent
  • Added trace-backs to test gen errors, to make debugging easier, as the error message is sometimes empty (e.g. asserts).
  • Unsure about phase-1 "genesis" tests. To start immediately into a phase 1 state? Should that be a thing to test clients for?

Before merge:

  • Some block-operations have errors in phase 1 mode. It didn't get past attester slashing tests for me. If someone can help look into this, that would be nice.
  • I temporarily commented mainnet mode tests, to re-run test gen faster to check if it works. Since mainnet is much slower. Need to uncomment them.

CC @mkalinin @djrtwo @hwwhww Let me know if you like to change anything for phase 1 test generation.

@mkalinin
Copy link
Contributor

mkalinin commented Jul 8, 2020

Unsure about phase-1 "genesis" tests. To start immediately into a phase 1 state? Should that be a thing to test clients for?

It might be relevant for testnets or simulations that are starting from GENESIS_SLOT right away, i.e. with PHASE_1_GENESIS_SLOT = GENESIS_SLOT

@protolambda
Copy link
Contributor Author

@mkalinin Sounds good, but we'll need the spec to define what that phase-1 genesis looks like (I see you are working on that and shared details in the discord, thanks). Right now I'm afraid a bare phase-0 genesis test that runs with phase-1 spec will break other phase-1 spec assumptions.

@mkalinin
Copy link
Contributor

mkalinin commented Jul 9, 2020

we'll need the spec to define what that phase-1 genesis looks like

Totally agree. I currently use pretty naive way to get phase-1 genesis:

genesis_state = initialize_beacon_state_from_eth1(eth1_block_hash, eth1_timestamp, deposits)
phase_1_genesis_state = upgrade_to_phase1(genesis_state)

But this approach works well in my case.

@hwwhww
Copy link
Contributor

hwwhww commented Jul 9, 2020

Some block-operations have errors in phase 1 mode. It didn't get past attester slashing tests for me. If someone can help look into this, that would be nice.

The error message, for example:

generating test vectors from tests source: eth2spec.test.phase0.block_processing.test_process_attester_slashing
Generating test: output/minimal/phase1/operations/attester_slashing/pyspec_tests/all_empty_indices
ERROR: failed to generate vector(s) for test output/minimal/phase1/operations/attester_slashing/pyspec_tests/all_empty_indices: 'NoneType' object is not iterable
Traceback (most recent call last):
  File "/Users/hwwang/.pyenv/versions/env38a/lib/python3.8/site-packages/gen_base/gen_runner.py", line 138, in run_generator
    for (name, out_kind, data) in test_case.case_fn():
TypeError: 'NoneType' object is not iterable
  • It's because we set these tests to with_phases([PHASE0]) since phase1 AttestationData format was different from phase0. And thanks to 0.01 bit proof of custody [depends on 256 bit custody atoms] #1889, these attester slashing tests can be enabled now. (I pushed 60f4bd2)

  • However, we still have some other test cases for test generators with with_phases([PHASE0]).

  • Also, we have some phase1 test cases that only work with minimal config since we need enough validators to form crosslink for every slot for every slot in these test cases (example).

A quick workaround is skipping the test case if test_case.case_fn() returns None in gen_runner.py.
@protolambda any thought? :)

hwwhww and others added 17 commits August 11, 2020 15:24
1. Add `with_configs` decorator to assign available configs
2. Add `only_full_crosslink` decorator to detect if the configuation can
do full crosslinking
3. Add `context.is_pytest` flag: True if calling via pytest. False if
calling from test generator.
Add new decorators to skip tests and handle it in testgen
Skip the too-slow custody tests and turn on the generators
…back to the spec function for other test cases afterwards
Add `disable_process_reveal_deadlines` decorator and `reveal_deadlines_setting` meta tag
@protolambda
Copy link
Contributor Author

@hwwhww great work on all the phase 1 changes. Testing PRs were merged into this PR, and I just resolved a small merge conflict with dev. Is this ready to be merged now?

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@protolambda
I pushed some commits:

  1. Remove redundant keys in the rewards test generator.
  2. Use Dict for phase_0_mods, phase_1_mods instead of List.
  3. Add make lint_generators command for test generators flake8 checks. Also added it to the CI.

LGTM now. :)

@hwwhww
Copy link
Contributor

hwwhww commented Sep 18, 2020

@djrtwo

Changes to the phase 0 test vectors outputs:

  1. [ssz] fix in random-output generation.
  2. [rewards] Rewards tests are now split into random/basic/leak, instead of all in core.
  3. [sanity/block] Adds a reveal_deadlines_setting field to the sanity/block meta.yaml.
  4. [Config files]: add CONFIG_NAME meta field to distinguish between "mainnet" and "minimal" configs.

@djrtwo djrtwo merged commit 68bcc19 into dev Sep 18, 2020
@djrtwo djrtwo deleted the testgenphase1 branch September 18, 2020 14:44
@prestonvanloon
Copy link
Contributor

Was it intentional to remove eth2.0-spec-tests/tests/mainnet/phase0/ssz_static?

@protolambda
Copy link
Contributor Author

No, thanks for reporting, just heard from Terence too. It was meant to be disabled just for phase1 mainnet I think (and at first all mainnet tests were disabled, to testrun the changes in this PR quicker). We'll re-introduce them. The types have not changed, so you can keep running the v0.12.2 SSZ tests. Sorry for the inconvenience.

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.

5 participants