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

(WIP)feat/upgrade5 prover #1557

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

(WIP)feat/upgrade5 prover #1557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27eaebd
preliminary (WIP)
roynalnaruto Oct 23, 2024
d25bca8
keep darwin_v2 instead of darwin
amoylan2 Oct 26, 2024
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
527 changes: 392 additions & 135 deletions prover/Cargo.lock
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion prover/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ ethers-core = { git = "https://github.com/scroll-tech/ethers-rs.git", branch = "
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"] }
prover_euclid = { git = "https://github.com/scroll-tech/zkevm-circuits.git", branch = "feat/hybrid-snark-agg", package = "prover", default-features = false, features = ["parallel_syn", "scroll"] }
base64 = "0.13.1"
reqwest = { version = "0.12.4", features = ["gzip"] }
reqwest-middleware = "0.3"
Expand Down
1 change: 0 additions & 1 deletion prover/rust-toolchain
View file
Open in desktop

This file was deleted.

2 changes: 2 additions & 0 deletions prover/rust-toolchain.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[toolchain]
channel = "nightly-2024-01-25"
1 change: 1 addition & 0 deletions prover/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#![feature(lazy_cell)]
#![allow(internal_features)]
#![feature(core_intrinsics)]

mod config;
Expand Down
10 changes: 8 additions & 2 deletions prover/src/prover.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ impl<'a> Prover<'a> {
let keystore_path = &config.keystore_path;
let keystore_password = &config.keystore_password;

let geth_client = if config.prover_type == ProverType::Chunk {
let geth_client = if matches!(
config.prover_type,
ProverType::ChunkHalo2 | ProverType::ChunkSp1 | ProverType::ChunkAll
) {
Some(Rc::new(RefCell::new(
GethClient::new(
&config.prover_name,
Expand Down Expand Up @@ -73,7 +76,10 @@ impl<'a> Prover<'a> {
prover_height: None,
};

if self.config.prover_type == ProverType::Chunk {
if matches!(
self.config.prover_type,
ProverType::ChunkHalo2 | ProverType::ChunkSp1 | ProverType::ChunkAll
) {
Comment on lines +79 to +82
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Error handling needs improvement in task fetching

The verification revealed some concerns in the error handling:

  • The fetch_task result is unwrapped in task_processor.rs without proper error context
  • The error handling in the task fetching logic could be more robust, especially for the new prover types

Key locations that need attention:

  • prover/src/task_processor.rs: Replace fetch_result.unwrap() with proper error handling
  • prover/src/prover.rs: Consider adding more specific error types for the new prover type cases
🔗 Analysis chain

LGTM! Verify error handling for edge cases.

The task fetching logic correctly handles the new prover types and includes appropriate error handling.

Let's verify the error handling paths are tested:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for tests covering error cases in task fetching
rg -A 5 "fetch_task.*Error|test.*fetch_task" prover/src/

# Look for potential panic scenarios in error paths
ast-grep --pattern 'unwrap()'

Length of output: 90

Script:

#!/bin/bash
# Search for tests covering task fetching with more context
rg -A 10 "fetch_task" prover/src/

# Look for error handling in task fetching implementation
rg -A 10 "fn fetch_task" prover/src/

# Look for unwrap calls specifically in prover.rs
rg "unwrap\(\)" prover/src/prover.rs

Length of output: 2325

let latest_block_number = self.get_latest_block_number_value()?;
if let Some(v) = latest_block_number {
if v.as_u64() == 0 {
Expand Down
25 changes: 18 additions & 7 deletions prover/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,19 @@ pub type CommonHash = H256;
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum TaskType {
Undefined,
Chunk,
ChunkHalo2,
ChunkSp1,
Comment on lines +11 to +12
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Update required: Several files still use the old TaskType::Chunk variant

The verification has found multiple files that need to be updated to use the new ChunkHalo2 and ChunkSp1 variants instead of the old Chunk variant:

  • prover/src/zk_circuits_handler/darwin_v2.rs: Lines containing chunk_vk = chunk_handler.get_vk(TaskType::Chunk) and check_vk(TaskType::Chunk, chunk_vk, ...)
  • common/libzkp/impl/src/lib.rs: Uses TaskType::Chunk in proof verification
  • common/libzkp/impl/src/verifier/darwin.rs and darwin_v2.rs: Match arms using TaskType::Chunk

These references need to be updated to use either TaskType::ChunkHalo2 or TaskType::ChunkSp1 based on the prover type being used. The changes are not just simple renames but require determining the correct variant to use in each context.

🔗 Analysis chain

Verify the impact of TaskType enum changes.

The renaming of Chunk to ChunkHalo2 and addition of ChunkSp1 could impact external systems that depend on these values. Please ensure all consumers are updated accordingly.

Let's verify the usage of these enum variants:

Also applies to: 20-23, 36-39

Update remaining references to TaskType::Chunk to use the new variants

The verification reveals several files still using the old TaskType::Chunk variant that need to be updated:

  • prover/src/zk_circuits_handler/darwin_v2.rs: Uses TaskType::Chunk in verification logic
  • common/libzkp/impl/src/lib.rs: Uses TaskType::Chunk in proof verification functions
  • common/libzkp/impl/src/verifier/darwin.rs and darwin_v2.rs: Uses TaskType::Chunk in match arms

Let's verify if these are the only instances and check their context:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to the old Chunk variant
rg -l "TaskType::Chunk"

# Search for places where TaskType is used to ensure they handle the new ChunkSp1 variant
rg -A 5 "TaskType::"

Length of output: 11203

Script:

#!/bin/bash
# Show the context around TaskType::Chunk usage
rg -B 3 -A 3 "TaskType::Chunk"

# Check if there are any other enum variants we might have missed
ast-grep --pattern 'enum TaskType {
  $$$
}'

Length of output: 7946

Batch,
Bundle,
}

impl TaskType {
fn from_u8(v: u8) -> Self {
match v {
1 => TaskType::Chunk,
1 => TaskType::ChunkHalo2,
2 => TaskType::Batch,
3 => TaskType::Bundle,
4 => TaskType::ChunkSp1,
_ => TaskType::Undefined,
}
}
Expand All @@ -31,9 +33,10 @@ impl Serialize for TaskType {
{
match *self {
TaskType::Undefined => serializer.serialize_u8(0),
TaskType::Chunk => serializer.serialize_u8(1),
TaskType::ChunkHalo2 => serializer.serialize_u8(1),
TaskType::Batch => serializer.serialize_u8(2),
TaskType::Bundle => serializer.serialize_u8(3),
TaskType::ChunkSp1 => serializer.serialize_u8(4),
}
}
}
Expand All @@ -56,15 +59,19 @@ impl Default for TaskType {

#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub enum ProverType {
Chunk,
ChunkHalo2,
ChunkSp1,
ChunkAll,
Batch,
}

impl ProverType {
fn from_u8(v: u8) -> Self {
match v {
1 => ProverType::Chunk,
1 => ProverType::ChunkHalo2,
2 => ProverType::Batch,
3 => ProverType::ChunkSp1,
4 => ProverType::ChunkAll,
_ => {
panic!("invalid prover_type")
}
Comment on lines +71 to 77
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 returning Result instead of panicking.

The from_u8 method currently panics on invalid input. Consider returning a Result type instead, which would allow callers to handle invalid values gracefully:

-    fn from_u8(v: u8) -> Self {
+    fn from_u8(v: u8) -> Result<Self, &'static str> {
         match v {
-            1 => ProverType::ChunkHalo2,
-            2 => ProverType::Batch,
-            3 => ProverType::ChunkSp1,
-            4 => ProverType::ChunkAll,
-            _ => {
-                panic!("invalid prover_type")
-            }
+            1 => Ok(ProverType::ChunkHalo2),
+            2 => Ok(ProverType::Batch),
+            3 => Ok(ProverType::ChunkSp1),
+            4 => Ok(ProverType::ChunkAll),
+            _ => Err("invalid prover_type")
         }
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
1 => ProverType::ChunkHalo2,
2 => ProverType::Batch,
3 => ProverType::ChunkSp1,
4 => ProverType::ChunkAll,
_ => {
panic!("invalid prover_type")
}
match v {
1 => Ok(ProverType::ChunkHalo2),
2 => Ok(ProverType::Batch),
3 => Ok(ProverType::ChunkSp1),
4 => Ok(ProverType::ChunkAll),
_ => Err("invalid prover_type")
}

Expand All @@ -73,8 +80,10 @@ impl ProverType {

pub fn to_u8(self) -> u8 {
match self {
ProverType::Chunk => 1,
ProverType::ChunkHalo2 => 1,
ProverType::Batch => 2,
ProverType::ChunkSp1 => 3,
ProverType::ChunkAll => 4,
Comment on lines +83 to +86
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 using a const array to define variant mappings.

The serialization values are duplicated across multiple methods. Consider defining them once to improve maintainability:

impl ProverType {
    const VARIANT_MAPPINGS: [(ProverType, u8); 4] = [
        (ProverType::ChunkHalo2, 1),
        (ProverType::Batch, 2),
        (ProverType::ChunkSp1, 3),
        (ProverType::ChunkAll, 4),
    ];

    fn to_u8(self) -> u8 {
        Self::VARIANT_MAPPINGS
            .iter()
            .find(|(variant, _)| *variant == self)
            .map(|(_, value)| *value)
            .unwrap()
    }
}

Also applies to: 97-100

}
}
}
Expand All @@ -85,8 +94,10 @@ impl Serialize for ProverType {
S: Serializer,
{
match *self {
ProverType::Chunk => serializer.serialize_u8(1),
ProverType::ChunkHalo2 => serializer.serialize_u8(1),
ProverType::Batch => serializer.serialize_u8(2),
ProverType::ChunkSp1 => serializer.serialize_u8(3),
ProverType::ChunkAll => serializer.serialize_u8(4),
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion prover/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ pub fn log_init(log_file: Option<String>) {

pub fn get_task_types(prover_type: ProverType) -> Vec<TaskType> {
match prover_type {
ProverType::Chunk => vec![TaskType::Chunk],
ProverType::ChunkHalo2 => vec![TaskType::ChunkHalo2],
ProverType::ChunkSp1 => vec![TaskType::ChunkSp1],
ProverType::ChunkAll => vec![TaskType::ChunkHalo2, TaskType::ChunkSp1],
ProverType::Batch => vec![TaskType::Batch, TaskType::Bundle],
}
}
8 changes: 4 additions & 4 deletions prover/src/zk_circuits_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
mod common;
mod darwin;
mod darwin_v2;
mod euclid;

use super::geth_client::GethClient;
use crate::{
Expand All @@ -9,8 +9,8 @@ use crate::{
utils::get_task_types,
};
use anyhow::{bail, Result};
use darwin::DarwinHandler;
use darwin_v2::DarwinV2Handler;
use euclid::EuclidHandler;
use std::{cell::RefCell, collections::HashMap, rc::Rc};

type HardForkName = String;
Expand Down Expand Up @@ -61,7 +61,7 @@ impl<'a> CircuitsHandlerProvider<'a> {
&config.low_version_circuit.hard_fork_name
);
AssetsDirEnvConfig::enable_first();
DarwinHandler::new(
DarwinV2Handler::new(
prover_type,
&config.low_version_circuit.params_path,
&config.low_version_circuit.assets_path,
Comment on lines +64 to 67
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Implementation for Sp1 task type is incomplete

The codebase shows several issues with the Sp1 task type implementation:

  • gen_sp1_chunk_proof is not implemented (contains todo!())
  • ChunkProver struct has commented out Sp1 prover field
  • The Sp1 prover initialization is commented out with TODO

This indicates that while the task type is defined, its actual implementation is missing or incomplete.

🔗 Analysis chain

Verify task type handling in both handlers.

Since this PR aims to support Sp1 tasks, let's verify the implementation of task type handling in both handlers.

Also applies to: 87-91

Let me gather more information about the implementation of task handlers, particularly focusing on the Sp1 chunk proof generation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify task type handling in both handlers

# Test 1: Search for TaskType handling in both handlers
echo "Searching for TaskType handling..."
rg -A 5 "match.*TaskType" prover/src/

# Test 2: Search for Sp1 specific handling
echo "Searching for Sp1 task handling..."
rg -A 5 "ChunkSp1" prover/src/

Length of output: 5091

Script:

#!/bin/bash
# Search for Sp1 chunk proof generation implementation
echo "Searching for Sp1 chunk proof generation..."
rg -A 10 "fn gen_sp1_chunk_proof" prover/src/

# Search for ChunkProver struct implementation
echo "Searching for ChunkProver implementation..."
rg -A 10 "struct ChunkProver" prover/src/

Length of output: 1968

Expand All @@ -84,7 +84,7 @@ impl<'a> CircuitsHandlerProvider<'a> {
&config.high_version_circuit.hard_fork_name
);
AssetsDirEnvConfig::enable_second();
DarwinV2Handler::new(
EuclidHandler::new(
prover_type,
&config.high_version_circuit.params_path,
&config.high_version_circuit.assets_path,
Expand Down
Loading
Loading