Skip to content

Conversation

ec2
Copy link

@ec2 ec2 commented Jul 10, 2025

Issue Addressed

Proposed Changes

  • In shuffling, a the raw_pivot (u64) is cast to a usize which will break on 32 bit systems. Now it is modulo'ed with the list_size first then cast to a usize.
  • ruint doesn't implement shifting with u64's on 32-bit arch. Since prefix_bits is u8 and NODE_ID_BITS = 256, we use them as u32's instead.

See: https://docs.rs/ruint/latest/src/ruint/bits.rs.html#711

Additional Info

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

Copy link

cla-assistant bot commented Jul 10, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

cla-assistant bot commented Jul 10, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@michaelsproul
Copy link
Member

We currently do not support 32-bit platforms, and have a compile-time check that prevents compilation on 32-bit arch: https://github.com/sigp/lighthouse/blob/stable/common/target_check/src/lib.rs

Is this required for RISC0? I thought RISCV would be primarily 64-bit?

It's possible we could roll out 32-bit support one crate at a time, but I would like to do this in a way that is testable, rather than just updating the code which is prone to regressions.

@ec2
Copy link
Author

ec2 commented Jul 10, 2025

We only need 32 bit support for the some of the types in the types crate. We technically don't need it for the shuffling stuff, but then we would just be copy pasta-ing that whole file which seems a little silly.

EDIT: There's something in milhouse that breaks on 32 bit platforms as well. I will be opening a PR there soon.

@michaelsproul
Copy link
Member

Ok, I think we should add a CI job that runs the tests for the relevant crates on a 32-bit machine. I'm worried things will just regress otherwise.

Let me discuss with the rest of the team

@ec2
Copy link
Author

ec2 commented Jul 10, 2025

Thanks boss 🫡 I always appreciate you and the team <3

@ec2
Copy link
Author

ec2 commented Jul 10, 2025

Related-ish PR: sigp/milhouse#72

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

ec2 commented Aug 5, 2025

@michaelsproul can i get a CI run pl0x <3

@michaelsproul
Copy link
Member

I gotchu

@michaelsproul michaelsproul added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Aug 5, 2025
Copy link

mergify bot commented Aug 5, 2025

Some required checks have failed. Could you please take a look @ec2? 🙏

@mergify mergify bot 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 Aug 5, 2025
@michaelsproul
Copy link
Member

Beta compiler fail looks unrelated. We'll fix that in another PR

@ec2 ec2 requested a review from michaelsproul August 5, 2025 05:16
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.

Just needs a back-merge of unstable for CI to pass.

@ec2
Copy link
Author

ec2 commented Aug 6, 2025

@michaelsproul done! thanks 🙏🏼

@mergify mergify bot added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Aug 6, 2025
@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 6, 2025
@mergify mergify bot merged commit c06ac81 into sigp:unstable Aug 6, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-quality ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants