Skip to content

Conversation

@RCasatta
Copy link
Collaborator

As suggested in #147 (comment)

Comment on lines +299 to +323
impl rand::RngCore for CrappyRng {

fn next_u32(&mut self) -> u32 {
self.next_u64() as u32
}

fn next_u64(&mut self) -> u64 {
let mut x = self.0;
x ^= x << 13;
x ^= x >> 7;
x ^= x << 17;
self.0 = x;
x
}

fn fill_bytes(&mut self, dest: &mut [u8]) {
for chunk in dest.chunks_mut(8) {
let x = self.next_u64().to_be_bytes();
chunk.copy_from_slice(&x[..chunk.len()]);

}
}

fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), rand::Error> {
Ok(self.fill_bytes(dest))
Copy link
Collaborator Author

@RCasatta RCasatta Sep 29, 2022

Choose a reason for hiding this comment

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

This code is duplicated in the 2 examples, I thought it to be better to duplicate the code instead of having it in the main lib

Copy link
Member

Choose a reason for hiding this comment

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

I think we should put it in the main lib, but in a module which has #[cfg(test)] on it?

Copy link
Collaborator Author

@RCasatta RCasatta Sep 29, 2022

Choose a reason for hiding this comment

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

From the examples, I cannot access something I put behind a #[cfg(test)] in the lib.

eg.
If I put #[cfg(test)] pub const CIAO: u8 = 0u8; in src/lib.rs

and I do cargo run --example pset_blind_coinjoin

I got

error[E0425]: cannot find value `CIAO` in crate `elements`

Copy link
Member

Choose a reason for hiding this comment

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

Oh, derp, I missed that this was in examples rather than unit tests.

Now I'm unsure if we should be using a broken RNG given that this is "example" code ... we should probably go back to using rng_chacha :P

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, but the wrong interpretation due the ambivalent nature of this examples/tests leave me the doubt of moving those in /tests

Copy link
Member

Choose a reason for hiding this comment

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

I think for now let's just go with this, since it's blocking our move to rand 0.8. We can rearrange things later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Opened issue #149



/// Xorshift
pub struct CrappyRng(u64);
Copy link
Collaborator Author

@RCasatta RCasatta Sep 29, 2022

Choose a reason for hiding this comment

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

I took a simple PRNG from Wikipedia,

for laziness it fail something like

rng = CrappyRng(1);
rng2 = CrappyRng(1);
assert_eq!( concat!( rng.fill_bytes( 2_bytes_buffer), rng.fill_bytes( 2_bytes_buffer)), rng2.fill_bytes(4_bytes_buffer))

I don't know if it matters here or in general

Copy link
Member

Choose a reason for hiding this comment

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

Can you link the wikipedia page in the comments?

My impulse would've been to use a LCG, eg any of the params from https://en.wikipedia.org/wiki/Linear_congruential_generator#Parameters_in_common_use but if this one is on wikipedia somewhere I'm sure it's just as good :P.

I don't think that invariant matters at all .. is it supposed to hold for RngCore? Why would it be your responsibility to make it hold when you only implement one method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you link the wikipedia page in the comments?

To start I post it here https://en.wikipedia.org/wiki/Xorshift#Example_implementation

I don't think that invariant matters at all .. is it supposed to hold for RngCore?

No, I found in the doc:

many implementations of fill_bytes consume a whole number of u32 or u64 values and drop any remaining unused bytes

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 948416b

@RCasatta RCasatta merged commit 327ef6e into ElementsProject:master Sep 30, 2022
@RCasatta RCasatta mentioned this pull request Sep 30, 2022
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.

2 participants