chore: migrate from Bn256Engine to Bn256EngineKGZ #1093
Conversation
|
This is great, but I'd love to make sure we have benchmarks on Lurk switched to the correct curve cycle before we further change the PCS. |
arthurpaulino
force-pushed
the
ap/migrate-to-Bn256EngineKGZ
branch
from
c7b9342 to
d8ec7b7
Compare
|
!gpu-benchmark |
Benchmark for 222255fClick to view benchmark
|
|
!gpu-benchmark |
arthurpaulino
force-pushed
the
ap/migrate-to-Bn256EngineKGZ
branch
from
13c0e68 to
db4c5fd
Compare
|
!gpu-benchmark |
|
!gpu-benchmark |
Benchmark for 222255fClick to view benchmark
|
| @@ -96,12 +96,12 @@ fn compute_coeffs<F: LurkField>(store: &Store<F>) -> (usize, usize) { | |||
| } | |||
|
|
|||
| pub(crate) fn test_coeffs() { | |||
| let store = Store::<pasta_curves::Fq>::default(); | |||
| let store = Store::<halo2curves::bn256::Fr>::default(); | |||
There was a problem hiding this comment.
Nit: we have a clippy config to refer to the field types of our main fields in https://github.com/lurk-lab/lurk-rs/blob/main/.clippy.toml and we should adapt this to bn254/grumpkin (or open an issue to do it later)
There was a problem hiding this comment.
I want to open an issue but I don't know the names we want to actually use to refer to them. I don't see the equivalent *::Scalar/*::Base aliases in halo2curves. What do you suggest?
There was a problem hiding this comment.
Here's how we create that alias on Arecibo:
https://github.com/lurk-lab/arecibo/blob/788e8788dc190c1875f0b37faccd6a7b13e51d76/src/provider/bn256_grumpkin.rs#L21-L32
I'd suggest copying those lines in a sensible place in Lurk.
| @@ -188,25 +182,25 @@ fn eval_benchmark(c: &mut Criterion) { | |||
| // .sample_size(60); | |||
|
|
|||
| // let limit = 1_000_000_000; | |||
| // let _lang_pallas = Lang::<Fq, Coproc<Fq>>::new(); | |||
| // let lang_pallas = Lang::<Fq, Coproc<Fq>>::new(); | |||
| // let _lang = Lang::<Bn, Coproc<Bn>>::new(); | |||
There was a problem hiding this comment.
I would leave the todo and try uncommenting, so that this function does not bit-rot. As long as this function is not included in a criterion_main! invocation, it won't be run. We can open an issue and assign to whomever git blame is pointing at for leaving this around.
There was a problem hiding this comment.
This function doesn't contain key code we'd want to maintain in order to eventually implement a circuit generation benchmark. So I'd rather to just delete it and then open an issue for this task.
WDYT?
There was a problem hiding this comment.
I'm OK with deleting, we already have an issue (#283)
Closes #1079 by following the instructions from #1039 (comment)
Closes #1022
Addresses benchmarks mentioned in #1078