Skip to content

Feature gate arbitrary crate in the consensus types crate #7743

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jul 23, 2025
Merged

Feature gate arbitrary crate in the consensus types crate #7743

merged 10 commits into from
Jul 23, 2025

Conversation

ec2
Copy link

@ec2 ec2 commented Jul 14, 2025

Issue Addressed

Which issue # does this PR address?

Proposed Changes

Puts the arbitrary crate behind a feature flag in the types crate.

Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.

@michaelsproul michaelsproul added code-quality consensus An issue/PR that touches consensus code, such as state_processing or block verification. labels Jul 15, 2025
michaelsproul
michaelsproul reviewed Jul 15, 2025
View reviewed changes
@michaelsproul michaelsproul added the backwards-incompat Backwards-incompatible API change label Jul 15, 2025
@michaelsproul
Copy link
Member

michaelsproul commented Jul 15, 2025 •
edited
Loading

CI is failing because we've bumped the MSRV while fixing Clippy lints on Milhouse (somewhat inadvertently). It's fine, we just need to bump the MSRV here to 1.87.0 (I think this is when is_multiple_of was stabilised):

lighthouse/lighthouse/Cargo.toml

Line 7 in cfb1f73

rust-version = "1.83.0"

@ec2
Copy link
Author

ec2 commented Jul 15, 2025

CI is failing because we've bumped the MSRV while fixing Clippy lints on Milhouse (somewhat inadvertently). It's fine, we just need to bump the MSRV here to 1.87.0 (I think this is when is_multiple_of was stabilised):

lighthouse/lighthouse/Cargo.toml

Line 7 in cfb1f73

rust-version = "1.83.0"

fixed!

michaelsproul
michaelsproul reviewed Jul 15, 2025
View reviewed changes
@michaelsproul
Copy link
Member

@ec2 Still a few failing CI jobs.

Looks like network-tests is just flakyness, idk what's wrong with cargo-sort, but dockerfile-ubuntu should be fixed by bumping this:

lighthouse/Dockerfile

Line 1 in cfb1f73

FROM rust:1.84.0-bullseye AS builder

May as well go to 1.88 I think.

@ec2
Copy link
Author

ec2 commented Jul 17, 2025

@ec2 Still a few failing CI jobs.

Looks like network-tests is just flakyness, idk what's wrong with cargo-sort, but dockerfile-ubuntu should be fixed by bumping this:

lighthouse/Dockerfile

Line 1 in cfb1f73

FROM rust:1.84.0-bullseye AS builder

May as well go to 1.88 I think.

Both should be fixed now!

@ec2
Copy link
Author

ec2 commented Jul 18, 2025

@ec2 Still a few failing CI jobs.

Looks like network-tests is just flakyness, idk what's wrong with cargo-sort, but dockerfile-ubuntu should be fixed by bumping this:

lighthouse/Dockerfile

Line 1 in cfb1f73

FROM rust:1.84.0-bullseye AS builder

May as well go to 1.88 I think.

Looks like doppelganger-protection-success-test is flakey as well?

@michaelsproul
Copy link
Member

Yeah, it can be. I've triggered a re-run

@ec2
Copy link
Author

ec2 commented Jul 18, 2025

There was conflicts with the base, so would love another rerun thanks

macladson
macladson reviewed Jul 22, 2025
View reviewed changes
Copy link
Member

@macladson macladson 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 to me, just a small nit

consensus/types/src/contribution_and_proof.rs
Comment on lines +12 to +15
#[cfg_attr(
feature = "arbitrary",
derive(arbitrary::Arbitrary),
arbitrary(bound = "E: EthSpec")
Copy link
Member

Choose a reason for hiding this comment

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

We should move this to underneath the comment

Copy link
Member

Choose a reason for hiding this comment

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

Agree. @ec2 if you could turn on "allow maintainers to make changes to my PRs" setting this would help us get your changes merged faster.

michaelsproul
michaelsproul approved these changes Jul 23, 2025
View reviewed changes
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Approved modulo the small doc comment change that Mac requested.

@michaelsproul michaelsproul added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Jul 23, 2025
@ec2
Copy link
Author

ec2 commented Jul 23, 2025

Yeah I somehow dont have the option top turn on that feature... But I've fixed your suggestion @macladson @michaelsproul
image

@macladson macladson added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jul 23, 2025
mergify bot added a commit that referenced this pull request Jul 23, 2025
@mergify
Merge of #7743
070d689
@mergify mergify bot mentioned this pull request Jul 23, 2025
Closed
6 tasks
@mergify mergify bot merged commit 9911f34 into sigp:unstable Jul 23, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelsproul michaelsproul michaelsproul approved these changes

@macladson macladson macladson approved these changes

Assignees
No one assigned
Labels
backwards-incompat Backwards-incompatible API change code-quality consensus An issue/PR that touches consensus code, such as state_processing or block verification. ready-for-merge This PR is ready to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ec2 @michaelsproul @macladson