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

Arbitrary trait for eth2/types #1040

Merged
merged 12 commits into from May 5, 2020
Merged

Arbitrary trait for eth2/types #1040

merged 12 commits into from May 5, 2020

Conversation

kirk-baird
Copy link
Member

@kirk-baird kirk-baird commented Apr 22, 2020

Issue Addressed

#1031

Proposed Changes

Add the Arbitrary trait for all types in eth2/types

Additional Info

  • This is a work in process and currently does not build, due to conflicting types.
  • It is pointing to a sigp fork but should be updated once this PR is merged.

Signed-off-by: Kirk Baird <baird.k@outlook.com>
@kirk-baird kirk-baird added work-in-progress security labels Apr 22, 2020
@kirk-baird kirk-baird self-assigned this Apr 22, 2020
@AgeManning AgeManning changed the base branch from v0.2.0 to master Apr 22, 2020
kirk-baird added 3 commits Apr 28, 2020
Signed-off-by: Kirk Baird <baird.k@outlook.com>
Signed-off-by: Kirk Baird <baird.k@outlook.com>
Signed-off-by: Kirk Baird <baird.k@outlook.com>
@kirk-baird kirk-baird changed the title [WIP] Arbitrary trait for eth2/types Arbitrary trait for eth2/types Apr 28, 2020
@kirk-baird kirk-baird added ready-for-review work-in-progress and removed work-in-progress ready-for-review labels Apr 28, 2020
kirk-baird added 4 commits Apr 28, 2020
Signed-off-by: Kirk Baird <baird.k@outlook.com>
Signed-off-by: Kirk Baird <baird.k@outlook.com>
Signed-off-by: Kirk Baird <baird.k@outlook.com>
Signed-off-by: Kirk Baird <baird.k@outlook.com>
@kirk-baird kirk-baird added ready-for-review and removed work-in-progress labels Apr 29, 2020
Copy link
Member

@paulhauner paulhauner left a comment

Looks good! Only a few small things!

eth2/types/src/beacon_state.rs Show resolved Hide resolved
eth2/utils/ssz_types/Cargo.toml Show resolved Hide resolved
eth2/utils/ssz_types/src/bitfield.rs Outdated Show resolved Hide resolved
eth2/utils/ssz_types/src/variable_list.rs Outdated Show resolved Hide resolved
eth2/utils/bls/Cargo.toml Show resolved Hide resolved
@paulhauner paulhauner added waiting-on-author and removed ready-for-review labels May 1, 2020
Signed-off-by: Kirk Baird <baird.k@outlook.com>
@kirk-baird
Copy link
Member Author

kirk-baird commented May 2, 2020

The other thing I'm not sure if it's worth adding to the CI to ensure it compiles as currently the CI won't check the --features arbitrary-fuzz case for state_processing which in turn should check arbitrary for all the dependencies.

Possibly do

$ cd eth2/state_processing

$ cargo check --features arbitrary-fuzz

@paulhauner
Copy link
Member

paulhauner commented May 3, 2020

The other thing I'm not sure if it's worth adding to the CI to ensure it compiles as currently the CI won't check the --features arbitrary-fuzz case for state_processing which in turn should check arbitrary for all the dependencies.

I think this is a good idea. You can basically just copy-paste-modify this section and it'll run on the CI:

clippy:
runs-on: ubuntu-latest
needs: cargo-fmt
steps:
- uses: actions/checkout@v1
- name: Lint code for quality and style with Clippy
run: make lint

@paulhauner
Copy link
Member

paulhauner commented May 3, 2020

Looks good! Happy to merge, but I think it's worth throwing in the CI check if you have the time. It might save some time/frustration for the fuzzing team :)

@zedt3ster
Copy link
Member

zedt3ster commented May 4, 2020

I had to derive the Arbitrary trait on one or two more types to be able to fuzz some of the epoch state transitions. Please do not merge this yet - I'll push my changes to this PR shortly :)

@zedt3ster
Copy link
Member

zedt3ster commented May 4, 2020

I had to derive the Arbitrary trait on one or two more types to be able to fuzz some of the epoch state transitions. Please do not merge this yet - I'll push my changes to this PR shortly :)

Done!

@kirk-baird kirk-baird removed the waiting-on-author label May 4, 2020
kirk-baird added 2 commits May 4, 2020
Signed-off-by: Kirk Baird <baird.k@outlook.com>
Signed-off-by: Kirk Baird <baird.k@outlook.com>
@kirk-baird kirk-baird added the ready-for-review label May 4, 2020
Copy link
Member

@paulhauner paulhauner left a comment

Happy to merge! 🎉

@paulhauner paulhauner added ready-to-squerge and removed ready-for-review labels May 5, 2020
@zedt3ster zedt3ster merged commit 611a0c7 into master May 5, 2020
11 checks passed
@zedt3ster zedt3ster deleted the arbitrary-fuzzing branch May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants