-
Notifications
You must be signed in to change notification settings - Fork 128
fix(rust-client): replace RandomCoin with ChaCha20Rng
#2414
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
base: next
Are you sure you want to change the base?
Changes from 15 commits
fb3cc74
e2f2ad1
42aea8b
98e77fd
a69138b
f2c72b7
4efc3f3
2e5ebc7
9e408fe
a440cdb
18cf8d9
47b7d75
35e647e
e3b4748
b36b001
cd55c64
8c636ca
8fbc523
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,10 +5,10 @@ use alloc::vec::Vec; | |
|
|
||
| use miden_protocol::assembly::{DefaultSourceManager, SourceManagerSync}; | ||
| use miden_protocol::block::BlockNumber; | ||
| use miden_protocol::crypto::rand::RandomCoin; | ||
| use miden_protocol::{Felt, MAX_TX_EXECUTION_CYCLES, MIN_TX_EXECUTION_CYCLES}; | ||
| use miden_protocol::{MAX_TX_EXECUTION_CYCLES, MIN_TX_EXECUTION_CYCLES}; | ||
| use miden_tx::{ExecutionOptions, LocalTransactionProver}; | ||
| use rand::RngExt; | ||
| use rand::SeedableRng; | ||
| use rand_chacha::ChaCha20Rng; | ||
|
|
||
| #[cfg(any(feature = "tonic", feature = "std"))] | ||
| use crate::alloc::string::ToString; | ||
|
|
@@ -85,9 +85,9 @@ pub trait StoreFactory { | |
| /// - **Store** ([`Store`]): Provides persistence for accounts, notes, and transaction history. | ||
| /// Configure via [`store()`](Self::store). | ||
| /// | ||
| /// - **RNG** ([`FeltRng`](miden_protocol::crypto::rand::FeltRng)): Provides randomness for | ||
| /// generating keys, serial numbers, and other cryptographic operations. If not provided, a random | ||
| /// seed-based RNG is created automatically. Configure via [`rng()`](Self::rng). | ||
| /// - **RNG** ([`ClientCryptoRng`](crate::ClientCryptoRng)): Provides randomness for note serial | ||
| /// numbers, script arguments and account seeds. If not provided, a random seed-based RNG is | ||
| /// created automatically. Configure via [`rng()`](Self::rng). | ||
| /// | ||
| /// - **Authenticator** ([`TransactionAuthenticator`](miden_tx::auth::TransactionAuthenticator)): | ||
| /// Handles transaction signing when signatures are requested from within the VM. Configure via | ||
|
|
@@ -337,7 +337,10 @@ where | |
| self | ||
| } | ||
|
|
||
| /// Optionally provide a custom RNG. | ||
| /// Optionally provide a custom RNG for note serial numbers, script arguments and account | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Requiring |
||
| /// seeds. Defaults to `ChaCha20Rng`. Secret keys and transaction input sealing | ||
| /// use a separate, non-overridable generator; see | ||
| /// [`Client::secure_rng`](crate::Client::secure_rng). | ||
| #[must_use] | ||
| pub fn rng(mut self, rng: ClientRngBox) -> Self { | ||
| self.rng = Some(rng); | ||
|
|
@@ -474,14 +477,16 @@ where | |
| }; | ||
|
|
||
| // Use the provided RNG, or create a default one. | ||
| let rng = if let Some(user_rng) = self.rng { | ||
| let rng: ClientRngBox = if let Some(user_rng) = self.rng { | ||
| user_rng | ||
| } else { | ||
| let mut seed_rng = rand::rng(); | ||
| let coin_seed: [u64; 4] = seed_rng.random(); | ||
| Box::new(RandomCoin::new(coin_seed.map(Felt::new_unchecked).into())) | ||
| Box::new(ChaCha20Rng::from_rng(&mut rand::rng())) | ||
| }; | ||
|
|
||
| // Create a separate, secure RNG for the sealing of transaction inputs and secret key | ||
| // generation. | ||
| let secure_rng: ClientRngBox = Box::new(ChaCha20Rng::from_rng(&mut rand::rng())); | ||
|
|
||
| // Set default prover if not provided | ||
| let tx_prover: Arc<dyn TransactionProver + Send + Sync> = | ||
| self.tx_prover.unwrap_or_else(|| Arc::new(LocalTransactionProver::default())); | ||
|
|
@@ -524,6 +529,7 @@ where | |
| Ok(Client { | ||
| store, | ||
| rng: ClientRng::new(rng), | ||
| secure_rng: ClientRng::new(secure_rng), | ||
| rpc_api, | ||
| tx_prover, | ||
| authenticator: self.authenticator, | ||
|
|
@@ -593,3 +599,21 @@ impl ClientBuilder<FilesystemKeyStore> { | |
| Ok(self.authenticator(Arc::new(keystore))) | ||
| } | ||
| } | ||
|
|
||
| // TESTS | ||
| // ================================================================================================ | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| /// Checks that [`ClientBuilder::rng`] rejects a generator that is not a `CryptoRng`. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is already enforced by the compiler in the type system, we can remove this test IMO |
||
| /// | ||
| /// The driver lives here rather than in `tests/` because `make test` runs `--lib` only. | ||
| /// | ||
| /// The expected diagnostic is snapshotted in `tests/ui/*.stderr`. It quotes the compiler | ||
| /// verbatim, so a `rand` or `rustc` upgrade can reword it; regenerate with | ||
| /// `TRYBUILD=overwrite cargo test -p miden-client --features "testing std" --lib ui`. | ||
| #[test] | ||
| fn ui() { | ||
| trybuild::TestCases::new().compile_fail("tests/ui/*.rs"); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migration path here does not compile because both helpers live under
miden_client::rng. Please change this tomiden_client::rng::{draw_felt, draw_word}if the helpers remain public.