Skip to content

Conversation

@kirk-baird
Copy link
Member

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.

@kirk-baird kirk-baird added work-in-progress PR is a 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 April 22, 2020 14:56
Signed-off-by: Kirk Baird <[email protected]>
Signed-off-by: Kirk Baird <[email protected]>
@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 The code is ready for review work-in-progress PR is a work-in-progress and removed work-in-progress PR is a work-in-progress ready-for-review The code is ready for review labels Apr 28, 2020
@kirk-baird kirk-baird added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Apr 29, 2020
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Looks good! Only a few small things!

@paulhauner paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels May 1, 2020
Signed-off-by: Kirk Baird <[email protected]>
@kirk-baird
Copy link
Member Author

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

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

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

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

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 The reviewer has suggested changes and awaits thier implementation. label May 4, 2020
@kirk-baird kirk-baird added the ready-for-review The code is ready for review label May 4, 2020
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Happy to merge! 🎉

@paulhauner paulhauner added ready-to-squerge and removed ready-for-review The code is ready for review labels May 5, 2020
@zedt3ster zedt3ster merged commit 611a0c7 into master May 5, 2020
@zedt3ster zedt3ster deleted the arbitrary-fuzzing branch May 5, 2020 23:12
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.

4 participants