-
Notifications
You must be signed in to change notification settings - Fork 241
Enabling Nova to be used in no_std environments
#376
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: main
Are you sure you want to change the base?
Conversation
* checkpoint * OsRng fixed * checkpoint * builds on no_std
* feat: add batch commit to trait (microsoft#366) * feat: add batch_commit and default implementation to CommitmentEngineTrait * feat: add batch_vartime_multiscalar_mul and default implementation to DlogGroup trait * feat: implement batch_commit in hyperkzg * fix: batch_commit should support using a slice of blinding factors * KZG Key Generation, Save, Load (microsoft#368) * save load * fixed exp batch * tools * fix * fix * fix * improve interface * clean * fix typos * fix * clean * ptau * ptau save load * fix * fix * pedersen save --------- Co-authored-by: Srinath Setty <srinath@microsoft.com> * An initial implementation of NeutronNova (microsoft#370) * simplify types * placeholder * deduplicate code an initial draft of running instance in neutronnova * test default instance-witness pair * simplify poseidon sponge * checkpoint NIFS prover * add verify * add padding in test case * additional progress * finish testing neutronnova folding scheme * small cleanup * factor out contributions from eq * reorganize code * checkpoint * change power polynomial * checkpoint * checkpoint working test * switch the order * checkpoint * compute the right E vectors * leverage the nested structure * simplify the handling of E1 and E2 * simplify the handling of E1 and E2 * simplify tests * test when the left and right split unevenly * rearrange * general code cleanup * fix clippy warnings * simplify Poseidon state reset; cleanup unused code * cleanup unused code * checkpoint circuits from Nova folding scheme * checkpoint and reorganize gadgets a bit * use the version from gadgets * create various low-level gadgets * fix most of the build issues * fix build issues * more progress on circuit * fix duplicate paths * use expect test * replace params with vk to avoid confusion * builds * tests build * additional checks * print failures * simplify the RO usage * fix tests * fix univariate * checkpoint * checkpoint nifs circuit * checkpoint * test passes * cargo check passes * revive tests * add missing file * cleanup; fix test * fix clippy * add missing constraints * simplify * move neutron under an experimental feature flag * remove unused file * remove unused file * save on field multiplications * Simplify Nova APIs (microsoft#373) * checkpoint simplified APIs * cleanup code * fix benches to use the simplified APIs * de-duplicate hash check code * fix clippy errors * bring back MSM from https://github.com/microsoft/Nova/blob/7050052bc2… (microsoft#375) * bring back MSM from https://github.com/microsoft/Nova/blob/7050052bc2606129fbb52a1fa666eef22d92748d/src/provider/msm.rs * fix tests * fixed to work in no_std too --------- Co-authored-by: Jacob Trombetta <jacob.trombetta@spaceandtime.io> Co-authored-by: SunJc <331631622@qq.com> Co-authored-by: Srinath Setty <srinath@microsoft.com>
@microsoft-github-policy-service agree company="MVP Workshop" |
|
Thanks for submitting this PR! I'll add comments one by one. As a note: We should also add a CI job to check if the code builds in |
src/provider/mod.rs
Outdated
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| /// An implementation of Nova traits with HyperKZG over the BN256 curve | ||
| #[cfg(feature = "std")] |
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.
This seems quite hard to maintain going forward. Wouldn't it be simpler to make the underlying code "no std" compatible rather than introducing new types at the highest level?
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.
I agree, it is not ideal scenario. The current problem is that halo2curves are not no_std compatible. I started working on converting halo2curves crate to be no_std compatible, then we will be able to use halo2curves everywhere (in std & no_std environment).
You can see the changes here -> privacy-ethereum/halo2curves@main...milosdjurica:halo2curves:no_std
Any help with those changes is more than welcome!
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.
I think there may be a way to pass pasta_curves to Nova without needing Nova to maintain that code. For example, we could have the layer above Nova implement Nova's traits for pasta_curves types right?
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.
Latest commit b45a5db resolves this by removing pasta_curves completely, and uses halo2curves for STD and NO_STD. This will make code much more simpler to maintain going forward. I am waiting for PR in halo2curves to be accepted and then I will change halo2curves import in Cargo.toml -> b45a5db#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542R46
src/provider/pasta_curves.rs
Outdated
| @@ -0,0 +1,176 @@ | |||
| #![cfg(not(feature = "std"))] | |||
| //! This module implements the Nova traits for `pallas::Point`, `pallas::Scalar`, `vesta::Point`, `vesta::Scalar`. | |||
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.
We recently dropped support for pasta_curves because it is hard to maintain layers on multiple curve libraries and halo2curves implements pasta_curves. Is there a reason to bring this back?
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.
Responded here -> #376 (comment)
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.
I have made changes in the latest commit to resolve this, now I am just waiting for the PR in halo2curves to be accepted. Changes -> b45a5db
src/r1cs/mod.rs
Outdated
| #[cfg(feature = "std")] | ||
| let r_E = E::Scalar::random(&mut OsRng); | ||
| #[cfg(not(feature = "std"))] | ||
| let r_W = E::Scalar::random(&mut ChaCha20Rng::seed_from_u64(0xDEADBEEF)); |
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.
This is not safe. We need to sample random numbers to provide zero-knowledge. Please change these everywhere to call into a secure random number generator.
| ); | ||
|
|
||
| // Sequential execution instead of rayon::join | ||
| #[cfg(not(feature = "std"))] |
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.
Is there a reason to do this here than in the underlying code inner?
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.
Not really sure what do you mean by this?
|
Hi @milosdjurica thanks again for your work on this! I've left some comments. The most important one is related to random number generator used. It seems like the code hardcodes randomness instead of sampling from a secure rng. Is there no random number generator in no-std environment? If yes, then I'm not sure the prover and the setup can be safely implemented in such environments. |
|
Hi @srinathsetty , thank you for the effort! I have resolved some changes, and as I noted, I started working on I'm open for all suggestions, and once again, thank you very much! :) |
|
Hi @srinathsetty , here is the update. Halo2curvesRegarding halo2curves, I have opened PR to make it There is one more thing regarding this. Nova is using RNGRegarding RNG in no_std, seems like there is no universal and simple way to get secure random number (at least I am not aware of it right now). ChaCha is creating a PRNG from seed, but for the same seed, output will always be the same and predictable, which is not secure. The solution is to get a random number in no_std environment is to provide a ChaCha with secure entropy source, but this depends on a specific targets. Under the hood, Possible solution for RNGWhat we can do is to hide functionality that requires RNG behind STD feature flag. This would mean that STD users can do same things as they always could. In no_std mode users would only be able to verify the proofs, but not create them. This is still improvement, as in earlier versions, no_std users could not interact with library in any way.
Please let me know what you think. Thanks! |
Update: It is dangerous to provide APIs where someone can produce a proof in no_std mode that are easily distinguishable. In particular, if the prover uses a seed that's known or can be guessed, it's easy to break zero-knowledge. We cannot hope that no one runs the prover in no_std mode. If your goal is to only run the verifier in no_std mode, then why not simply write a stand-alone program that deserializes Nova's bincode-encoded proofs and verify them? The verification algorithm can be easily pieced together using code provided in this library. |
I completely agree with this, but this is not what I was suggesting. I will give a minimal example to explain what I was trying to say. struct CompressedSNARK {
// Fields...
}
impl CompressedSNARK {
#[cfg(feature = "std")]
pub fn prove() {
// This method uses RNG and it will only be available in STD mode. We will keep using OsRng as before.
// But, users WILL NOT BE ABLE to `prove()` in no_std environment due to the feature flag above the method -> `#[cfg(feature = "std")]`.
// This means that if users want to call this method, they must be in STD mode, and they will use safe OsRng for random number.
}
pub fn verify() {
// This method DOES NOT use RNG, so it is completely safe to let users call this function in both, std and no_std mode.
}
}
Users in STD environment can call both Long story short ->
Regarding this : In order to deserialize proof and verify it, you need to specify in which struct you want to deserialize into. That means you need to import a struct from Nova, and that means Nova has to be dependency in your project. If Nova is not no_std compatible, then the program calling it also can not be no_std compatible. Let me know what you think about this. I hope I explained it well now, and that it makes more sense. If you agree with this, I will make another commit that will remove using unsafe RNG in P.S. -> Latest commit b45a5db removes |
|
Hi @srinathsetty , Here is commit regarding RNG -> 402073b I decided to take a bit different approach from what I have explained earlier, as previous approach would bloat the whole codebase with This keeps the codebase clean, safe and easy to maintain. |
Diving a bit into this. I'm not sure it is necessary to have Nova as a dependency at all. There should be a way to build a stand-alone |
Hi @srinathsetty , Thanks for this, I will take a look. Do you maybe have any other suggestions for other changes made in this PR? P.S. I tested in both STD and NO_STD environments and in both cases it was faster. Might be worth exploring this deeper. You can see changes here -> MVPWorkshop@6c30073 . Note: this is not included in this current PR.
|
This is unexpected. Would you be able to run the commitment benchmark on your machine and report results here. The command is: cargo bench --bench=commit |
|
You are right regarding the benchmarks. I have ran benchmarks and it says that halo2curves is slower. However, verification test says different. Could you update your test fn test_ivc_nontrivial_with_spark_compression_with<E1, E2, EE1, EE2>()
where
E1: Engine<Base = <E2 as Engine>::Scalar>,
E2: Engine<Base = <E1 as Engine>::Scalar>,
EE1: EvaluationEngineTrait<E1>,
EE2: EvaluationEngineTrait<E2>,
{
// ...
// ...
+ let start = std::time::Instant::now();
// verify the compressed SNARK
let res = compressed_snark.verify(&vk, num_steps, &[<E1 as Engine>::Scalar::ZERO]);
+ let duration = start.elapsed();
+ println!("Duration is -> {:?}", duration);
and run test with the following command -> Those are my results -> ON MAIN -> USING HALO2CURVES MSM -> It is faster in all 3 cases, and it should also be faster not only for spark compression but also for normal compression in -> |
It is not clear why the microbenchmark will show halo2curves as slow but faster when used elsewhere. Thanks, will investigate! |
|
Thanks for looking into it! In the meantime, do you have any questions or suggestions about other things regarding this PR? Here is comment regarding latest RNG changes #376 (comment) |
|
@srinathsetty What is the current state of this PR? Can it be merged or still waiting for the upstream PR in halo2curves repo to be merged first? |
Closes #345
This PR enables Nova to be used in
no_stdenvironments. After this PR is merged users of this library will have the option to choose if they wantstdorno_stdversion of Nova.stdfunctionalities remain unchanged!Main points for
no_stdpart ->no_stdenvironments.halo2curvesdependency can not be used as it is notno_stdcompatible. Instead ofhalo2curves,pasta_curvescrate is used for Pallas and Vesta curves. Secp, Secq, Bn256 and Grumpkin curves are not available for now inno_stdenvironments.bincodedependency version is changed to 2.0.0-rc.3 as the old one was not compatible forno_std.rand_core+rand_chachafor RNG, as OsRng relies on the operating system and is unavailable in no_std environments.no_stdusingOnceCellfromcore::cell::OnceCellinstead ofonce_cellcrate .@srinathsetty
no_stdbranch was used as a reference for some of the changes -> main...no_stdAs I said, Nova's functionality remains unchanged by default, but this gives users an option to use it in a
no_stdenvironments too by disabling default features. Also, all tests are passing!To build the Nova library on
no_stdtarget, usecargo build --target thumbv7em-none-eabi --no-default-features.I’m more than open to all questions, suggestions, discussions, and corrections! Please let me know what you think! :)