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

added srs_cache cli arg #2940

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions o1vm/src/cli/cannon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ impl From<MipsVmConfigurationArgs> for VmConfiguration {
pub struct RunArgs {
#[arg(long = "preimage-db-dir", value_name = "PREIMAGE_DB_DIR")]
pub preimage_db_dir: Option<String>,
#[arg(long = "srs-cache", value_name = "SRS_CACHE")]
Copy link
Member

@dannywillems dannywillems Jan 10, 2025

Choose a reason for hiding this comment

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

Maybe simply srs or srs_filename or srs_path?

pub srs_cache: Option<String>,
// it's important that vm_cfg is last in order to properly parse the host field
#[command(flatten)]
pub vm_cfg: MipsVmConfigurationArgs,
Expand Down
37 changes: 32 additions & 5 deletions o1vm/src/pickles/main.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use ark_ff::UniformRand;
use clap::Parser;
use kimchi::circuits::domains::EvaluationDomains;
use kimchi::{circuits::domains::EvaluationDomains, precomputed_srs::TestSRS};
Copy link
Member

Choose a reason for hiding this comment

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

Out of the scope of this PR: it would be nice to move this structure TestSRS in poly_commitment in ipa.rs, next to SRS. We should also have a unified structure for TestSRS and SRS. It is mostly the encoding the difference. I guess we can have a parameter or something to serde.

use log::debug;
use mina_curves::pasta::{Fp, Vesta, VestaParameters};
use mina_poseidon::{
Expand Down Expand Up @@ -48,10 +48,37 @@ pub fn cannon_main(args: cli::cannon::RunArgs) {
let start = Start::create(state.step as usize);

let domain_fp = EvaluationDomains::<Fp>::create(DOMAIN_SIZE).unwrap();
let srs: SRS<Vesta> = {
let srs = SRS::create(DOMAIN_SIZE);
srs.get_lagrange_basis(domain_fp.d1);
srs
let srs: SRS<Vesta> = match &args.srs_cache {
Some(cache) => {
debug!("Loading SRS from cache {}", cache);
Copy link
Member

Choose a reason for hiding this comment

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

I would check that the SRS is of the expected size (DOMAIN_SIZE).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't the requirement that srs.size() >= DOMAIN_SIZE ? because I'm looking that the size of Vesta srs and it's actually 2^16, even though our domain size is half that

let file_path = Path::new(cache);
let file = File::open(file_path).expect("Error opening SRS cache file");
let srs: SRS<Vesta> = {
// By convention, proof systems serializes a TestSRS with filename 'test_<CURVE_NAME>.srs'.
// The benefit of using this is you don't waste time verifying the SRS.
if file_path
.file_name()
.unwrap()
.to_str()
.unwrap()
.starts_with("test_")
{
let test_srs: TestSRS<Vesta> = rmp_serde::from_read(&file).unwrap();
From::from(test_srs)
} else {
rmp_serde::from_read(&file).unwrap()
}
};
debug!("SRS loaded successfully from cache");
srs
}
None => {
debug!("No SRS cache provided. Creating SRS from scratch");
let srs = SRS::create(DOMAIN_SIZE);
srs.get_lagrange_basis(domain_fp.d1);
debug!("SRS created successfully");
srs
}
};

// Initialize the environments
Expand Down
Loading