Skip to content
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.

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

Add UniScalarRng #123

Closed
wants to merge 1 commit into from
Closed

Add UniScalarRng #123

wants to merge 1 commit into from

Conversation

moCello
Copy link
Member

@moCello moCello commented Nov 22, 2023

Resolves: #121

Copy link
Member

@HDauven HDauven left a comment

Choose a reason for hiding this comment

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

LGTM, small comment.

CHANGELOG.md Show resolved Hide resolved
@moCello moCello force-pushed the mocello/121_uni_random branch 2 times, most recently from 68bc330 to 032c167 Compare November 22, 2023 10:27
@moCello moCello marked this pull request as draft November 22, 2023 14:18
@moCello moCello force-pushed the mocello/121_uni_random branch from 032c167 to 9514529 Compare November 22, 2023 17:06
@moCello moCello requested a review from vlopes11 November 22, 2023 17:07
@moCello moCello marked this pull request as ready for review November 22, 2023 17:08
@moCello moCello requested a review from HDauven November 22, 2023 17:08
Resolves: #121

Co-authored-by: Victor Lopez <[email protected]>
@moCello moCello force-pushed the mocello/121_uni_random branch from 9514529 to 81ccc66 Compare November 22, 2023 17:10
Copy link

@vlopes11 vlopes11 left a comment

Choose a reason for hiding this comment

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

I think we are adding unnecessary complexity by introducing a new RNG. Making it a function of JubJubScalar would be simpler & more consistent

Comment on lines +78 to +80
for integer in scalar.iter_mut() {
*integer = self.0.next_u64();
}

Choose a reason for hiding this comment

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

Not sure we need an iterator here. This will make it harder for the compiler to optimize. Being a small, fixed size array, why not just take the next_u64?

Comment on lines +92 to +95
for (i, integer) in scalar.iter().enumerate() {
let bytes = integer.to_le_bytes();
dest[i * 8..(i + 1) * 8].copy_from_slice(&bytes);
}

Choose a reason for hiding this comment

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

Ditto; unnecessary usage of iterators

if dest.len() > 32 {
dest[32..].fill(0);
}
return;

Choose a reason for hiding this comment

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

The returned value is a raw little-endian representation of the scalar limbs. This is not the expected serialized format, I wonder why we are doing it.

If we are to generate the limbs, we should probably send them directly to the scalar. We only have to return "bytes" because we opted to create a new RNG, when in fact what we need is to sample uniform 4-tuple u64.


use super::{Fr, MODULUS, R2};
// We use rejection sampling to generate a valid scalar.
fn fill_bytes(&mut self, dest: &mut [u8]) {

Choose a reason for hiding this comment

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

I'm not sure why we create a new RNG when we just want to create new random scalars from a given uniform RNG.

What we are attempting to achieve is to have a uniform implementation for JubJubScalar that takes a uniform sampler, not to create our own samples (that in the end will increase the type complexity of the stack).

Why not just implement this as a function JubJubScalar::random or JubJubScalar::uniform_random?

I favor replacing the previous implementation since the uniform bits is a desirable property in any context.

let mut buf32 = [0u8; 32];
let mut buf64 = [0u8; 64];

for _ in 0..100000 {

Choose a reason for hiding this comment

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

A more suitable approach would be to use a fuzzing library. Consider quickcheck

@moCello
Copy link
Member Author

moCello commented Nov 24, 2023

After extensive discussions we decided to not change the random scalar generation because it was deemed secure enough for our purpose:

We will use this paper to determine whether the current implementation of the random scalar implementation as done in the Field trait implmentation is cryptographically safe.

In the current implementation of random, we sample a random element with 512 bits, and reduce it by the modulo of the field of JubJubScalar to generate a valid scalar.

For JubJubScalar we have:

p := 0x0e7db4ea6533afa906673b0101343b00a6682093ccc81082d0970e5ed6f72cb7
p = 6554484396890773809930967563523245729705921265872317281365359162392183254199

and

s := 2^512
s = 13407807929942597099574024998205846127479365820592393377723561443721764030073546976801874298166903427690031858186486050853753882811946569946433649006084096

To check whether the above approach is secure, we do the following:

For p < s calculate r and m so that

s = rp + m

This leads to:

r := 2045593080716281616348203381729468609728209645786990242449482205581148743408809

and

m := s - p * r
m = 2244478849891746936202736009816130624903096691796347063256129649283183245105

Now we can check whether:

r > 2^64 * root(m/p)
r > 2^64 * root(0.34243408237518300501)
r > 18446744073709551616 * 0.58517867559847308999

This clearly holds true, which means that we fall in the first conversion of the paper above, and can safely sample an element from the field over 2^512 and take it modulo p.

This concludes that we don't need to make any adjustments to the current implementation of random scalar generation.

@moCello moCello closed this Nov 24, 2023
@moCello moCello deleted the mocello/121_uni_random branch November 30, 2023 15:19
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.

change implementation of dusks Fr::random() function to be uniformly distributed
3 participants