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

Allow extending LC merkle proof tests #3066

Merged
merged 3 commits into from
Nov 17, 2022
Merged

Conversation

etan-status
Copy link
Contributor

Currently, test_single_merkle_proof only supports BeaconState tests. For future tests, different object classes are desirable. Update format to allow testing other objects as well.

Currently, `test_single_merkle_proof` only supports `BeaconState` tests.
For future tests, different object classes are desirable. Update format
to allow testing other objects as well.
etan-status added a commit to etan-status/consensus-specs that referenced this pull request Oct 27, 2022
Future light client tests will also incorporate execution payload data.
To avoid confusion, rename the current `root` check to `beacon_root`.
Doing this now, as ethereum#3066 already requires LC test runners to update.
@ralexstokes
Copy link
Member

what types of tests do you have in mind?

this approach isn't impossible but in general we would handle this kind of thing by making a new type of test "handler" that has the exact format we want -- that way consumers of the test data have less dynamism at runtime, which is desirable for all of the compiled language targets that consume these tests

@etan-status
Copy link
Contributor Author

Using a different subfolder for each type is indeed cleaner, and more in line with e.g., ssz_static tests.
Changed it to use separate suite for now (within the single_merkle_proof handler, within the light_client runner).

As for future tests, I'd like to add one for BeaconBlockBody as well.

Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

LGTM

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.

lgtm 👍

If we want to extend the coverage of Merkle proof tests, we can consider reworking the test generator main.py to something similar to ssz_static rather than using pytest + @spec_state_test. @etan-status Do you think that would be helpful?

@etan-status
Copy link
Contributor Author

Currently, the idea for the LC merkle proof tests is to test for very specific cases (the 3-4 generalized indices necessary for the LC sync protocol) to also allow testing concrete implementations that only support what is actually needed over a generalized SSZ library. So, the tests are much more manual than something that can be generated dynamically.

The other difference is that the LC tests are all generated as part of make gen_light_client, so the output is one folder deeper as it's not a separate generator.

For now, would keep it as a LC test and do only the minimal prep work to allow tests against BlockBody on top of State. If we need more generalized testing in the future, it's a point that can be re-evaluated when there is a need for it, though.

@hwwhww hwwhww merged commit b6df4b5 into ethereum:dev Nov 17, 2022
@etan-status etan-status deleted the lc-prooftest branch November 17, 2022 20:46
etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Nov 18, 2022
`state.ssz_snappy` file was renamed to `object.ssz_snappy` and
`pyspec_tests` folder was renamed to `BeaconState` as part of
ethereum/consensus-specs#3066
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