remove halo2 dependency and refactor types to better accomodate openvm #62
Conversation
Co-authored-by: georgehao <haohongfan@gmail.com>
…l-tech/scroll-proving-sdk into feat-integrate-proving-sdk
This reverts commit ea4b1e3.
Co-authored-by: Péter Garamvölgyi <th307q@gmail.com>
…erType to_u8 This reverts commit 575423f. refactor: ProverProviderType to_u8
omerfirmak
force-pushed
the
omerfirmak/remove-config-dependency
branch
6 times, most recently
from
4a09883 to
ca6f4c4
Compare
omerfirmak
force-pushed
the
omerfirmak/remove-config-dependency
branch
from
ca6f4c4 to
c15aae4
Compare
omerfirmak
force-pushed
the
omerfirmak/remove-config-dependency
branch
from
7ed5b8a to
030fc08
Compare
| impl ProverType { | ||
| pub fn from_u8(v: u8) -> Self { | ||
| match v { | ||
| 1 => ProverType::Chunk, |
There was a problem hiding this comment.
Notice that the ProverType here is not completly match with which has been defined in prover/types. Does it suppose that any "OpenVM" prover would support all types of proving (chunk/batch/bundle)?
There was a problem hiding this comment.
In practice, yes they will. But it is still possible to selectively support subset of proof types in a openvm prover. It is possible to only ask for certain types of tasks in a GetTask request. https://github.com/scroll-tech/scroll-proving-sdk/pull/62/files#diff-5b38dca990c13c0d1398729aebaaf85541958a4753de6ec938955b11a790cf7fL44-L47
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "openvm")] | ||
| async fn build_block_witness( |
There was a problem hiding this comment.
The openvm prover required a BlockWitness data struct which now can be constructed from the utility in sbv, not only the block data. So we still need quite a few works to be added here.
There was a problem hiding this comment.
What do you mean? I use DumpWitnessCommand already.
| async fn prove(&self, req: ProveRequest) -> ProveResponse; | ||
| async fn query_task(&self, req: QueryTaskRequest) -> QueryTaskResponse; | ||
| async fn prove(&mut self, req: ProveRequest) -> ProveResponse; | ||
| async fn query_task(&mut self, req: QueryTaskRequest) -> QueryTaskResponse; |
There was a problem hiding this comment.
A query action which would change the service seems a little confusion, do we really need this entry not to be idempotent?
There was a problem hiding this comment.
Our prover implementations unfortunately do require mutable access right now. It could be worked around, but it just gets ugly really quick.
| @@ -39,7 +36,7 @@ impl L2gethClient { | |||
| Ok(trace) | |||
| } | |||
|
|
|||
| pub async fn get_sorted_traces_by_hashes( | |||
| pub async fn get_traces_by_hashes( | |||
There was a problem hiding this comment.
Why do we remove the sorting logic here?
| circuit_version: String, | ||
| coordinator_clients: Vec<CoordinatorClient>, | ||
| l2geth_client: Option<L2gethClient>, | ||
| proving_service: Box<dyn ProvingService + Send + Sync>, | ||
| proving_service: RwLock<Backend>, |
There was a problem hiding this comment.
Compare to the original Box, will the RwLock brings about performance issue when the n_workers of a cloud prover is large?
| @@ -32,7 +35,8 @@ pub struct L2GethConfig { | |||
|
|
|||
| #[derive(Debug, Serialize, Deserialize, Clone)] | |||
| pub struct ProverConfig { | |||
| pub circuit_types: Vec<CircuitType>, | |||
| pub circuit_type: CircuitType, | |||
There was a problem hiding this comment.
Is the reason we specify circuit_type here that we need to know whether we should build_halo2_input or build_openvm_input?
If so, there might be a situation the low_circuit_version is halo2, and the high_circuit_version is openvm. In this case, neither set circuit_type to halo2 or openvm will be problematic?
I think we can decide how to build input base on the hardfork name.
No description provided.