[zk-sdk] Replace internal rand usage with getrandom - #580
Open
samkim-crypto wants to merge 1 commit into
Open
samkim-crypto wants to merge 1 commit into
samkim-crypto wants to merge 1 commit into
Conversation
samkim-crypto
force-pushed
the
zk-sdk-remove-rand-v8
branch
2 times, most recently
from
October 1, 2026 01:55
82d5152 to
8343047
Compare
samkim-crypto
force-pushed
the
zk-sdk-remove-rand-v8
branch
from
October 2, 2026 02:42
8343047 to
69f9ff2
Compare
samkim-crypto
marked this pull request as ready for review
October 2, 2026 03:18
joncinque
reviewed
Oct 2, 2026
joncinque
left a comment
joncinque
left a comment
Contributor
There was a problem hiding this comment.
Looks great to me! Just some potential little optimizations using MaybeUninit since we don't need initialized types
| /// | ||
| /// Panics if the operating system's entropy source fails. | ||
| pub(crate) fn fill_random_bytes(bytes: &mut [u8]) { | ||
| getrandom::getrandom(bytes).expect("secure randomness unavailable"); |
Contributor
There was a problem hiding this comment.
If you want, we could keep using rand but just as an implementation detail.
On the flip-side, this refactoring shows that we don't really need it, which is great!
|
|
||
| /// Samples a scalar with the same 64-byte reduction as `Scalar::random`. | ||
| pub(crate) fn random_scalar() -> Scalar { | ||
| let mut bytes = Zeroizing::new([0u8; 64]); |
Contributor
There was a problem hiding this comment.
nit: we could change this to MaybeUninit since it'll get filled immediately anyway and use https://docs.rs/getrandom/latest/getrandom/fn.fill_uninit.html
| /// Fills a buffer with cryptographically secure randomness. | ||
| /// | ||
| /// Panics if the operating system's entropy source fails. | ||
| pub(crate) fn fill_random_bytes(bytes: &mut [u8]) { |
Contributor
There was a problem hiding this comment.
All of the uses of this function start by zero-initializing -- how about changing to MaybeUninit here too?
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
zk-sdkcurrently usesrandto generate random scalars, keys, and nonces. Scalar generation also depends on the RNG traits expected bycurve25519-dalek, which makes dependency upgrades more difficult as we prepare to replace it withsolana-ed25519in #360.We only need cryptographically secure random bytes here.
getrandomalready backs theOsRngcalls being replaced, so using it directly preserves the randomness source while avoiding coupling scalar generation to the curve library'srand_coreversion.Summary of Changes
I replaced direct
randusage withgetrandom. This keeps randomness generation independent of the curve library's RNG interface. Together with disabling the RNG features insolana-ed25519, this will let us make the switch without requiring matching RNG trait versions across thecryptography,zk-elgamal-proof, andagaverepos.To do this, I added private helpers in
random.rsthat obtain random bytes directly throughgetrandom. Scalar generation uses the same 64-byte reduction asScalar::random, and the temporary scalar sampling buffer is zeroized.Public APIs and proof formats are preserved. Dalek's existing
rand_corefeature stays enabled for downstream compatibility, making this suitable for v8.1.0 ahead of #360, which targets v9. The retained feature can be dropped as part of that migration.In the long term, it would be useful to consider updating the API to accept caller-provided random bytes, as some crates in
solana-sdkalready do. This would allow the core SDK to evolve independently of dependencies that supply randomness, sincegetrandomupgrades can still require maintenance. I decided to usegetrandomfor now becausezk-sdkalready generates randomness internally and this preserves the existing APIs. Making callers responsible for proof randomness would also need careful API design to reduce the risk of weak or reused randomness.