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

SeekableRng trait for Rngs with a block_pos? #1375

Open
nstilt1 opened this issue Jan 24, 2024 · 2 comments
Open

SeekableRng trait for Rngs with a block_pos? #1375

nstilt1 opened this issue Jan 24, 2024 · 2 comments

Comments

@nstilt1
Copy link

nstilt1 commented Jan 24, 2024

Background

#1369 has brought up a use for set_block_pos() and get_block_pos() for rand_chacha.

In implementing it for chacha20 with BlockRng, there is a minor issue with the API without using a new trait. The methods seem like they should belong on the core instead of the rng, but when implementing just those 2 methods, it results in these calls:

// this is essentially how the Rng is defined in this scenario
pub struct ChaCha20Rng {
  pub rng: BlockRng<ChaCha20Core>,
}

let mut rng = ChaCha20Rng::from_entropy();
rng.rng.core.set_block_pos(x);
rng.rng.core.get_block_pos();

This could be simplified by adding a ChaCha20Rng::get_core_mut() method, down to:

rng.get_core_mut().set_block_pos(x);

But now there are even more characters to type.

There is also a potential issue regarding set_block_pos() located on the core. The following code will fail because changing the block_pos on the core will not affect the index of the rng. So we will need to decide if that is okay or not:

let mut rng = ChaCha20Rng::from_entropy();

rng.core.set_block_pos(5);
let a = rng.next_u32();

rng.core.set_block_pos(5);
let b = rng.next_u32();

// this would fail
assert_eq!(a, b);

A SeekableRng trait might suffice, allowing for a shorter method call, and it could also handle the above issue depending on if that is an issue that needs to be fixed.

Feature request

I don't know if it would be possible to make a simplified call such as this:

let mut rng = ChaCha20Rng::from_entropy();
rng.core.set_block_pos();

But it seems like for that method to be called that way, SeekableRng would need to be implemented for BlockRng if the rng is a wrapper around it. The method does not need to be called that way though. I personally don't mind whether it is on the rng or the core, but it just seems like something that would go on the core.

Proposed trait definition:

pub trait SeekableRng {
  type Counter;
  fn set_block_pos(block_pos: Counter);
  fn get_block_pos() -> Counter;
}
@dhardy
Copy link
Member

dhardy commented Jan 24, 2024

struct ChaCha20Rng(BlockRng<ChaCha20Core>);
impl ChaCha20Rng {
    fn set_block_pos(x: _) {
        self.0.core.set_block_pos(x);
        self.0.reset(); // or generate_and_set(index)
    }
}

This should be enough for a == b. I don't think we need any impls directly on BlockRng.

@ShonFrazier
Copy link

Definitely an important feature for verification. For example, when one needs to replay an RNG.

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

No branches or pull requests

3 participants