Skip to content

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Aug 2, 2025

This PR changes random point generation to use rejection sampling by de-serialization instead of deriving a point from a random scalar.

Fixes #1140.

@andrewwhitehead
Copy link
Contributor

Wouldn't there be a modulo bias without reducing a field element from a larger random input?

@tarcieri
Copy link
Member

tarcieri commented Aug 2, 2025

@andrewwhitehead no, it's the other way around: reduction introduces a bias, and even a wide reduction has a minute bias. Rejection sampling is unbiased.

https://research.kudelskisecurity.com/2020/07/28/the-definitive-guide-to-modulo-bias-and-how-to-avoid-it/

@daxpedda
Copy link
Contributor Author

daxpedda commented Aug 2, 2025

Wouldn't there be a modulo bias without reducing a field element from a larger random input?

No, de-serialization actually declines field elements that don't fit the modulus.
(ed448-goldilocks still needs some fixes in that area)

@daxpedda daxpedda requested a review from tarcieri August 2, 2025 17:51
Comment on lines 267 to 278
let mut bytes = FieldBytes::<C>::default();
let mut sign = 0;

loop {
rng.try_fill_bytes(&mut bytes)?;
rng.try_fill_bytes(core::array::from_mut(&mut sign))?;
if let Some(point) =
AffinePoint::decompress(&bytes, Choice::from(sign & 1)).into_option()
{
return Ok(point.into());
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I know I just stamped approve, but now I'm kind of noticing that perhaps this should be an inherent pub fn try_from_rng impl'd on AffinePoint, and then the ProjectivePoint impl could be:

Suggested change
let mut bytes = FieldBytes::<C>::default();
let mut sign = 0;
loop {
rng.try_fill_bytes(&mut bytes)?;
rng.try_fill_bytes(core::array::from_mut(&mut sign))?;
if let Some(point) =
AffinePoint::decompress(&bytes, Choice::from(sign & 1)).into_option()
{
return Ok(point.into());
}
}
AffinePoint::try_from_rng(rng).map(Into::into)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but don't we need a trait for that to implement it in primeorder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently not.
Working on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

EdwardsPoint and DecafPoint still need a bit more work.
After this is merged I will do the necessary follow-up for EdwardsPoint in #1333.
Will make a PR for DecafPoint later.

@tarcieri tarcieri merged commit 9e8656b into RustCrypto:master Aug 2, 2025
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Random point has known discrete log (under some conditions)
3 participants