Update workspace to newest Halo2 version #341

CPerezz · 2022-02-21T18:17:27Z

This updates the entire workspace to work with the latest version of
appliedzkp/main. Resolves: #337

For now it is at rev = 514152c8b83de27a5ce17ef70cc44c4092240bd1 but we should try to start at least tagging our versions.

It also introduces the usage of AssignedCell replacing the (Cell, F)
primitive. Which closes: #320

ChihChengLiang

First round of comments

ChihChengLiang · 2022-02-23T09:42:27Z

circuit-benchmarks/src/evm_circuit.rs

@@ -12,7 +13,7 @@ pub struct TestCircuit<F> {
    block: Block<F>,
 }

-impl<F: FieldExt> Circuit<F> for TestCircuit<F> {
+impl<F: FieldExt + PrimeField<Repr = [u8; 32]>> Circuit<F> for TestCircuit<F> {


Do we need PrimeField everywhere?

It seems if we use use halo2_proofs::arithmetic::FieldExt; instead of use pairing::arithmetic::FieldExt;, it can work with use pairing::bn256::Fr as Fp;.

https://github.com/appliedzkp/halo2/blob/641b4b5f94cb7bbdd710eba0e84f94d1d23ca0c3/halo2_proofs/examples/simple-example-3.rs#L10

@kilic Any comments on this?

I discussed with @kilic this and since there are no more to/from_bytes methods, to/from_repr has to be used.

That means that F needs to define it's repr type. And since it's trait bounds, anything that interacts with it needs to have the same bounds which has lead to the propagation of the PrimeField trait.

I see. Makes sense to me.

On a different point of view: could we define an alias like:

trait Field: FieldExt + PrimeField<Repr = [u8; 32]> {}

Just to reduce verbosity?

@ed255 I like your proposal

keccak256/src/gates/base_conversion.rs

keccak256/src/gates/rho_checks.rs

ed255

LGTM! Thanks for taking care of this tedious task :)

Take a look at my comment about making the usage of the Field trait less verbose.

ed255 · 2022-02-23T12:29:11Z

circuit-benchmarks/src/evm_circuit.rs

@@ -12,7 +13,7 @@ pub struct TestCircuit<F> {
    block: Block<F>,
 }

-impl<F: FieldExt> Circuit<F> for TestCircuit<F> {
+impl<F: FieldExt + PrimeField<Repr = [u8; 32]>> Circuit<F> for TestCircuit<F> {


On a different point of view: could we define an alias like:

trait Field: FieldExt + PrimeField<Repr = [u8; 32]> {}

Just to reduce verbosity?

CPerezz · 2022-02-23T15:01:39Z

Regarding #341 (comment), @ed255 it looks fine to me to add this alias/supertrait.

It kinda masks a bit the Repr but it might be worth it!
WDYT @ChihChengLiang @han0110 ? If you agree I'll change it since I like it!

CPerezz · 2022-02-24T13:05:41Z

After working a bit on the issue @ed255 I've seen that we can create quite an issue with this approach (Correct me if I'm wrong).

SO basically we're creating a new trait in each workspace member. And this trait is basically not implemented for pairing::bn256::Fr. Which leads to rustc having big issues in order to compile all this.

Also the traits are the same but defined in different crates (as there's no crate which is used everywhere in others).

So I'm not sure anymore this is the best solution.
We might prefer to stay with F: FieldExt + PrimeField<Repr = [u8; 32]>. Which I agree is verbose but it's not the worst.

WDYT?

CPerezz · 2022-02-25T10:14:29Z

@ed255 @ChihChengLiang I just added it.

Can you confirm whether this is ok and then I merge?

ed255

LGTM!

This updates the entire workspace to work with the latest version of `appliedzkp/main` which works towards #337. It also introduces the usage of `AssignedCell` replacing the `(Cell, F)` primitive. Which closes: #320

This adds the trait bounds require to be able to perform byte serialization for Field Elements. This updates `eth_types` member to work with the latest version of `appliedzkp/main` which works towards #337.

Instead of storing `Option<F>` and `Cell` is easier to simply store `AssignedCell<F,F>` and to reduce the verbosity.

- Add descriptions for each Lookup. - Add trait bounds to enable de/serialization with `To/From_repr`. - Remove legacy and unused code.

With the new version of Halo2, serialization is done via `to/from_repr` which requires to demmand for traits like `ToScalar<F>` which is the `Repr` used for it's byte repr.

- Make usage of the new Proof generation primitives. - Refactor the setup generation with the unsafe_fast setup gen. (Just for testing).

- Added `ff` as a dependency to gain access to `PrimeField` trait used to specify repr details. - Updated trait bounds for structs and fns that required them since they interact with evm/state circuits which have extra trait bounds associated to serialization.

@ed255

Instead of constraining to `FieldExt` + `PrimeField<Repr>` @ed255 suggested to add a supertrait that enforces both and use it across the workspace.

ChihChengLiang

LGTM

@han0110

As @han0110 said. Before #341 &[&[]] is passed (as single proof instance with no instnace columns values). Now we were passing &[] only for instances. Which basically means that num_proofs=0. This was causing the errors during the benchmarks triggered in #352. Co-authored-by: @han0110

@han0110

As @han0110 said. Before #341 &[&[]] is passed (as single proof instance with no instnace columns values). Now we were passing &[] only for instances. Which basically means that num_proofs=0. This was causing the errors during the benchmarks triggered in #352. Co-authored-by: @han0110

* Replace to Rust smart cache in CI. * Update * Update and try cache restore.

github-actions bot added the crate-keccak Issues related to the keccak workspace member label Feb 21, 2022

CPerezz force-pushed the halo2_update branch from 04cd956 to d2d42c0 Compare February 22, 2022 17:01

github-actions bot added T-opcode Type: opcode-related and focused PR/Issue crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Feb 22, 2022

CPerezz force-pushed the halo2_update branch from d2d42c0 to fa37617 Compare February 22, 2022 17:33

CPerezz marked this pull request as ready for review February 22, 2022 17:33

CPerezz requested review from ChihChengLiang, ed255, han0110 and kilic February 22, 2022 17:34

CPerezz changed the title ~~Update keccak256 to newest Halo2 version~~ Update workspace to newest Halo2 version Feb 23, 2022

This was referenced Feb 23, 2022

Bench Keccak Cost #168

Closed

[cleanup] Replace all instances of (Cell, F) with the AssignedCell primitive from halo2. #320

Closed

ChihChengLiang reviewed Feb 23, 2022

View reviewed changes

CPerezz mentioned this pull request Feb 23, 2022

AssignedCell refactor causes Verifier to unwrap None in tests #344

Closed

CPerezz requested a review from ChihChengLiang February 23, 2022 12:58

ed255 approved these changes Feb 23, 2022

View reviewed changes

CPerezz force-pushed the halo2_update branch from 3c10ee9 to 466d199 Compare February 25, 2022 10:14

ed255 approved these changes Feb 25, 2022

View reviewed changes

CPerezz force-pushed the halo2_update branch from 466d199 to 3c10ee9 Compare February 25, 2022 10:21

CPerezz added 7 commits February 25, 2022 11:22

Update keccak256 to newest Halo2 version

1d2894e

This updates the entire workspace to work with the latest version of `appliedzkp/main` which works towards #337. It also introduces the usage of `AssignedCell` replacing the `(Cell, F)` primitive. Which closes: #320

Update eth_types to latest Halo2 version

be7ebc1

This adds the trait bounds require to be able to perform byte serialization for Field Elements. This updates `eth_types` member to work with the latest version of `appliedzkp/main` which works towards #337.

Update zkevm-circuits to use halo_proofs latest

c7b4d0a

Refactor Variable type to use AssignedCell

776d6b6

Instead of storing `Option<F>` and `Cell` is easier to simply store `AssignedCell<F,F>` and to reduce the verbosity.

Update zkevm-circuits to latest Halo2 version

c8ced97

- Add descriptions for each Lookup. - Add trait bounds to enable de/serialization with `To/From_repr`. - Remove legacy and unused code.

Update eth_types trait bounds to enable de/serde with Halo2

be3af59

With the new version of Halo2, serialization is done via `to/from_repr` which requires to demmand for traits like `ToScalar<F>` which is the `Repr` used for it's byte repr.

Update prover workspace member to latest Halo2 vers

d3f0378

- Make usage of the new Proof generation primitives. - Refactor the setup generation with the unsafe_fast setup gen. (Just for testing).

CPerezz added 8 commits February 25, 2022 11:22

Fix Word usage withing new Halo2 version

838c551

zkevm-circ/ Fix Lookup err descriptions

86ec506

circ-benchmarks: Update benches to the new Halo2 API

60ed264

keccak: Fix AssignedCell unwrappings

550241f

Rebase into latest main

aa25193

workspace: Depend on halo2 as tag not rev

44eba6e

workspace: Add Field supertrait

ca9760b

Instead of constraining to `FieldExt` + `PrimeField<Repr>` @ed255 suggested to add a supertrait that enforces both and use it across the workspace.

ChihChengLiang approved these changes Feb 25, 2022

View reviewed changes

zkevm-circuits: Update calldatasize & selfbalance Halo2

cba3641

CPerezz force-pushed the halo2_update branch from 3c10ee9 to cba3641 Compare February 25, 2022 10:26

CPerezz merged commit fecbbcb into main Feb 25, 2022

CPerezz deleted the halo2_update branch February 25, 2022 11:20

CPerezz mentioned this pull request Mar 3, 2022

Benchmarks for evm_circuit fail for Transcript corrupted reads of FieldElements #363

Closed

CPerezz mentioned this pull request Mar 3, 2022

Fix EVM Circuit benchmark #364

Merged

zemse pushed a commit to zemse/zkevm-circuits that referenced this pull request Mar 22, 2023

CI: replace to Rust smart cache (privacy-scaling-explorations#341)

fcf59e7

* Replace to Rust smart cache in CI. * Update * Update and try cache restore.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update workspace to newest Halo2 version #341

Update workspace to newest Halo2 version #341

CPerezz commented Feb 21, 2022 •

edited

Loading

ChihChengLiang left a comment

ChihChengLiang Feb 23, 2022

CPerezz Feb 23, 2022

ChihChengLiang Feb 23, 2022

ed255 Feb 23, 2022

ChihChengLiang Feb 24, 2022

ed255 left a comment

ed255 Feb 23, 2022

CPerezz commented Feb 23, 2022

CPerezz commented Feb 24, 2022

CPerezz commented Feb 25, 2022 •

edited

Loading

ed255 left a comment

ChihChengLiang left a comment

Update workspace to newest Halo2 version #341

Update workspace to newest Halo2 version #341

Conversation

CPerezz commented Feb 21, 2022 • edited Loading

ChihChengLiang left a comment

Choose a reason for hiding this comment

ChihChengLiang Feb 23, 2022

Choose a reason for hiding this comment

CPerezz Feb 23, 2022

Choose a reason for hiding this comment

ChihChengLiang Feb 23, 2022

Choose a reason for hiding this comment

ed255 Feb 23, 2022

Choose a reason for hiding this comment

ChihChengLiang Feb 24, 2022

Choose a reason for hiding this comment

ed255 left a comment

Choose a reason for hiding this comment

ed255 Feb 23, 2022

Choose a reason for hiding this comment

CPerezz commented Feb 23, 2022

CPerezz commented Feb 24, 2022

CPerezz commented Feb 25, 2022 • edited Loading

ed255 left a comment

Choose a reason for hiding this comment

ChihChengLiang left a comment

Choose a reason for hiding this comment

CPerezz commented Feb 21, 2022 •

edited

Loading

CPerezz commented Feb 25, 2022 •

edited

Loading