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.

Sign up for GitHub

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

refactor(prover): Using some std libs instead of thirdparty lib #1573

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

refactor(prover): Using some std libs instead of thirdparty lib #1573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 0 additions & 3 deletions prover/Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions prover/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,16 @@ log = "0.4"
env_logger = "0.11.3"
serde = { version = "1.0.198", features = ["derive"] }
serde_json = "1.0.116"
futures = "0.3.30"

ethers-core = { git = "https://github.com/scroll-tech/ethers-rs.git", branch = "v2.0.7" }
ethers-providers = { git = "https://github.com/scroll-tech/ethers-rs.git", branch = "v2.0.7" }
halo2_proofs = { git = "https://github.com/scroll-tech/halo2.git", branch = "v1.1" }
snark-verifier-sdk = { git = "https://github.com/scroll-tech/snark-verifier", branch = "develop", default-features = false, features = ["loader_halo2", "loader_evm", "halo2-pse"] }
prover_darwin = { git = "https://github.com/scroll-tech/zkevm-circuits.git", tag = "v0.12.2", package = "prover", default-features = false, features = ["parallel_syn", "scroll"] }
prover_darwin_v2 = { git = "https://github.com/scroll-tech/zkevm-circuits.git", tag = "v0.13.1", package = "prover", default-features = false, features = ["parallel_syn", "scroll"] }
base64 = "0.13.1"
reqwest = { version = "0.12.4", features = ["gzip"] }
reqwest-middleware = "0.3"
reqwest-retry = "0.5"
once_cell = "1.19.0"
hex = "0.4.3"
tiny-keccak = { version = "2.0.0", features = ["sha3", "keccak"] }
rand = "0.8.5"
Expand Down
64 changes: 33 additions & 31 deletions prover/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
use crate::types::ProverType;
use anyhow::{bail, Result};
use serde::{Deserialize, Serialize};
use std::fs::File;

use crate::types::ProverType;
use std::{fs::File, sync::OnceLock};

#[derive(Debug, Serialize, Deserialize)]
pub struct CircuitConfig {
Expand Down Expand Up @@ -52,7 +51,7 @@ impl Config {
}

static SCROLL_PROVER_ASSETS_DIR_ENV_NAME: &str = "SCROLL_PROVER_ASSETS_DIR";
static mut SCROLL_PROVER_ASSETS_DIRS: Vec<String> = vec![];
static SCROLL_PROVER_ASSETS_DIRS: OnceLock<Vec<String>> = OnceLock::new();

#[derive(Debug)]
pub struct AssetsDirEnvConfig {}
Expand All @@ -64,39 +63,42 @@ impl AssetsDirEnvConfig {
if dirs.len() != 2 {
bail!("env variable SCROLL_PROVER_ASSETS_DIR value must be 2 parts seperated by comma.")
}
unsafe {
SCROLL_PROVER_ASSETS_DIRS = dirs.into_iter().map(|s| s.to_string()).collect();
log::info!(
"init SCROLL_PROVER_ASSETS_DIRS: {:?}",
SCROLL_PROVER_ASSETS_DIRS
);
}

SCROLL_PROVER_ASSETS_DIRS.get_or_init(|| dirs.into_iter().map(|s| s.to_string()).collect());
log::info!(
"init SCROLL_PROVER_ASSETS_DIRS: {:?}",
SCROLL_PROVER_ASSETS_DIRS
);
Comment on lines +66 to +71
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider improving error handling in initialization

The initialization could benefit from better error handling and validation.

 pub fn init() -> Result<()> {
     let value = std::env::var(SCROLL_PROVER_ASSETS_DIR_ENV_NAME)?;
     let dirs: Vec<&str> = value.split(',').collect();
     if dirs.len() != 2 {
         bail!("env variable SCROLL_PROVER_ASSETS_DIR value must be 2 parts seperated by comma.")
     }
-    SCROLL_PROVER_ASSETS_DIRS.get_or_init(|| dirs.into_iter().map(|s| s.to_string()).collect());
+    if SCROLL_PROVER_ASSETS_DIRS.set(dirs.into_iter().map(|s| s.to_string()).collect()).is_err() {
+        bail!("SCROLL_PROVER_ASSETS_DIRS was already initialized");
+    }
     log::info!(
         "init SCROLL_PROVER_ASSETS_DIRS: {:?}",
         SCROLL_PROVER_ASSETS_DIRS
     );
     Ok(())

Committable suggestion skipped: line range outside the PR's diff.

Ok(())
}

pub fn enable_first() {
unsafe {
log::info!(
"set env {SCROLL_PROVER_ASSETS_DIR_ENV_NAME} to {}",
&SCROLL_PROVER_ASSETS_DIRS[0]
);
std::env::set_var(
SCROLL_PROVER_ASSETS_DIR_ENV_NAME,
&SCROLL_PROVER_ASSETS_DIRS[0],
);
}
debug_assert!(
SCROLL_PROVER_ASSETS_DIRS.get().is_some()
&& SCROLL_PROVER_ASSETS_DIRS.get().unwrap().len() >= 2
);
log::info!(
"set env {SCROLL_PROVER_ASSETS_DIR_ENV_NAME} to {}",
&SCROLL_PROVER_ASSETS_DIRS.get().unwrap()[0]
);
std::env::set_var(
SCROLL_PROVER_ASSETS_DIR_ENV_NAME,
&SCROLL_PROVER_ASSETS_DIRS.get().unwrap()[0],
);
Comment on lines +76 to +87
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the search results, I can now generate the final response:

Environment variable usage is consistent but has multiple setters

The SCROLL_PROVER_ASSETS_DIR environment variable is used consistently across the codebase, but there are multiple locations where it's being set:

  • prover/src/config.rs: Sets the variable twice with different values from a split array
  • common/libzkp/impl/src/verifier.rs: Sets using low_conf.assets_path
  • common/libzkp/impl/src/verifier/darwin.rs: Sets using assets_dir
  • common/libzkp/impl/src/verifier/darwin_v2.rs: Sets using assets_dir

The variable is properly validated in config.rs to ensure it contains at least 2 comma-separated paths, but the other setters might not follow this requirement. Consider centralizing the environment variable management to avoid potential conflicts.

🔗 Analysis chain

Verify environment variable usage across the codebase

Let's ensure that the SCROLL_PROVER_ASSETS_DIR environment variable is consistently used and that there are no conflicting usages.

Also applies to: 91-102

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent usage of SCROLL_PROVER_ASSETS_DIR environment variable

# Search for all occurrences of the environment variable
echo "Searching for environment variable usage..."
rg -A 2 "SCROLL_PROVER_ASSETS_DIR" 

# Search for potential environment variable setters
echo "Checking for environment variable setters..."
ast-grep --pattern 'std::env::set_var($name, $value)'

Length of output: 3884

}

pub fn enable_second() {
unsafe {
log::info!(
"set env {SCROLL_PROVER_ASSETS_DIR_ENV_NAME} to {}",
&SCROLL_PROVER_ASSETS_DIRS[1]
);
std::env::set_var(
SCROLL_PROVER_ASSETS_DIR_ENV_NAME,
&SCROLL_PROVER_ASSETS_DIRS[1],
);
}
debug_assert!(
SCROLL_PROVER_ASSETS_DIRS.get().is_some()
&& SCROLL_PROVER_ASSETS_DIRS.get().unwrap().len() >= 2
);
log::info!(
"set env {SCROLL_PROVER_ASSETS_DIR_ENV_NAME} to {}",
&SCROLL_PROVER_ASSETS_DIRS.get().unwrap()[1]
);
std::env::set_var(
SCROLL_PROVER_ASSETS_DIR_ENV_NAME,
&SCROLL_PROVER_ASSETS_DIRS.get().unwrap()[1],
);
}
}
6 changes: 3 additions & 3 deletions prover/src/version.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::cell::OnceCell;
use std::sync::OnceLock;

static DEFAULT_COMMIT: &str = "unknown";
static mut VERSION: OnceCell<String> = OnceCell::new();
static VERSION: OnceLock<String> = OnceLock::new();

pub const TAG: &str = "v0.0.0";
pub const DEFAULT_ZK_VERSION: &str = "000000-000000";
Expand All @@ -14,5 +14,5 @@ fn init_version() -> String {
}

pub fn get_version() -> String {
unsafe { VERSION.get_or_init(init_version).clone() }
VERSION.get_or_init(init_version).clone()
}
21 changes: 9 additions & 12 deletions prover/src/zk_circuits_handler/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,20 @@
use std::{collections::BTreeMap, rc::Rc};

use crate::types::ProverType;

use once_cell::sync::OnceCell;

use halo2_proofs::{halo2curves::bn256::Bn256, poly::kzg::commitment::ParamsKZG};
use std::{
collections::BTreeMap,
sync::{Arc, OnceLock},
};

static mut PARAMS_MAP: OnceCell<Rc<BTreeMap<u32, ParamsKZG<Bn256>>>> = OnceCell::new();
static PARAMS_MAP: OnceLock<Arc<BTreeMap<u32, ParamsKZG<Bn256>>>> = OnceLock::new();

pub fn get_params_map_instance<'a, F>(load_params_func: F) -> &'a BTreeMap<u32, ParamsKZG<Bn256>>
where
F: FnOnce() -> BTreeMap<u32, ParamsKZG<Bn256>>,
{
unsafe {
PARAMS_MAP.get_or_init(|| {
let params_map = load_params_func();
Rc::new(params_map)
})
}
PARAMS_MAP.get_or_init(|| {
let params_map = load_params_func();
Arc::new(params_map)
})
}

pub fn get_degrees<F>(prover_types: &std::collections::HashSet<ProverType>, f: F) -> Vec<u32>
Expand Down
6 changes: 2 additions & 4 deletions prover/src/zk_circuits_handler/darwin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,9 @@ use crate::{
types::{ProverType, TaskType},
};
use anyhow::{bail, Context, Ok, Result};
use once_cell::sync::Lazy;
use serde::Deserialize;

use crate::types::{CommonHash, Task};
use std::{cell::RefCell, cmp::Ordering, env, rc::Rc};

use prover_darwin::{
aggregator::Prover as BatchProver,
check_chunk_hashes,
Expand All @@ -19,9 +16,10 @@ use prover_darwin::{
BatchProof, BatchProvingTask, BlockTrace, BundleProof, BundleProvingTask, ChunkInfo,
ChunkProof, ChunkProvingTask,
};
use std::{cell::RefCell, cmp::Ordering, env, rc::Rc, sync::LazyLock};

// Only used for debugging.
static OUTPUT_DIR: Lazy<Option<String>> = Lazy::new(|| env::var("PROVER_OUTPUT_DIR").ok());
static OUTPUT_DIR: LazyLock<Option<String>> = LazyLock::new(|| env::var("PROVER_OUTPUT_DIR").ok());

#[derive(Debug, Clone, Deserialize)]
pub struct BatchTaskDetail {
Expand Down
6 changes: 2 additions & 4 deletions prover/src/zk_circuits_handler/darwin_v2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,9 @@ use crate::{
types::{ProverType, TaskType},
};
use anyhow::{bail, Context, Ok, Result};
use once_cell::sync::Lazy;
use serde::Deserialize;

use crate::types::{CommonHash, Task};
use std::{cell::RefCell, cmp::Ordering, env, rc::Rc};

use prover_darwin_v2::{
aggregator::Prover as BatchProver,
check_chunk_hashes,
Expand All @@ -19,9 +16,10 @@ use prover_darwin_v2::{
BatchProof, BatchProvingTask, BlockTrace, BundleProof, BundleProvingTask, ChunkInfo,
ChunkProof, ChunkProvingTask,
};
use std::{cell::RefCell, cmp::Ordering, env, rc::Rc, sync::LazyLock};

// Only used for debugging.
static OUTPUT_DIR: Lazy<Option<String>> = Lazy::new(|| env::var("PROVER_OUTPUT_DIR").ok());
static OUTPUT_DIR: LazyLock<Option<String>> = LazyLock::new(|| env::var("PROVER_OUTPUT_DIR").ok());

#[derive(Debug, Clone, Deserialize)]
pub struct BatchTaskDetail {
Expand Down