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

Tests with signatures, fixes #1074 #1102

Merged
merged 38 commits into from
May 22, 2019
Merged

Tests with signatures, fixes #1074 #1102

merged 38 commits into from
May 22, 2019

Conversation

protolambda
Copy link
Collaborator

@protolambda protolambda commented May 20, 2019

Made the pytests respect BLS signatures. This enables us to use the tests to generate properly BLS-signed test-vectors with.
Note: there are a few exceptions:

  • finality tests are too slow (many transitions), hence they run with disabled BLS. I'm not sure how/when we include these in the test-vectors for clients
  • there are a few sanity tests that I just disabled, as they cover long-range (empty slots) block transitions, costing lots of hashing.

The block-operations and epoch transitions are fully signed now 🎉

TODO:

  • Invalid-signature tests for block operations
  • Cover more of epoch transition with new tests future PR
  • Call the tests in generator-mode to generate test-vectors. (epoch transition generator in future PR)
  • Implement some basic SSZ caching, to speed up tests (the hashes of fields of empty blocks being cached would help a lot! Tests typically change only a part of the block). CI runs fast enough, SSZ caching to be implemented in separate PR

@djrtwo
Copy link
Contributor

djrtwo commented May 20, 2019

Ah, looks like this branch doesn't have the changes from #1052 integrated. Is the intention to modify generate_from_tests to override the default of BLS being off to on?

@hwwhww
Copy link
Contributor

hwwhww commented May 21, 2019

IMO it's safer to make it more explicit by removing the default value False from signed parameter.

@protolambda
Copy link
Collaborator Author

@djrtwo

Ah, looks like this branch doesn't have the changes from #1052 integrated.

It's branched off of that, so it should. You mean the proposal for BLS test-improvements? This PR is in draft mode mostly because:

  • want to implement some invalid-signature tests
  • need to go over the BLS things listed in this comment: BLS and testing #1074 (comment) to see what we really want, and what is better to adjust. (i.e. test-markers for BLS set to always ON)

Is the intention to modify generate_from_tests to override the default of BLS being off to on?

Ideally I think BLS should be ON, if pytests were fast enough. But we already discussed this, and it will be off for CI where possible, but ON where needed + in generated vectors.

For now I just like the explicitness, as it helped me tracking what needs to be updated, and finding testing bugs (things not being signed). If things are stable we may go for implicit signed True/False, but I don't think it's a netto win now.

@protolambda protolambda changed the title Tests with signatures Tests with signatures, fixes #1074 May 21, 2019
@protolambda protolambda marked this pull request as ready for review May 22, 2019 00:08
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.

Last set of cleanups and forced-on bls tests look good.

Headed to bed. good to merge when you're up

@djrtwo djrtwo merged commit 43bb64e into v06x May 22, 2019
@djrtwo djrtwo deleted the tests-with-sigs branch May 22, 2019 20:41
@hwwhww hwwhww mentioned this pull request May 24, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants