Keccak circuit implementation #268

CPerezz · 2022-01-10T22:54:22Z

This PR implements the entire keccak permutation circuit (No padding) to be used as a gadget or a final circuit.

Resolves: #218
Resolves: #273

Since the state is always carried as an `AssignedCell`, this utility fn helps to dettach the `Cell` from it's value `F`. And this is implemented for N-lenght arrays.

This is the way on which al the configs are implemented. And so, we cannot be using `region + offset` approach in any part of the keccak_circuit. Therefore Xi has been refactored to use a `Layouter` in order to witness it's data.

This makes the implementation of the out-of-circuit primitives for keccak much closer and similar to the inner circuit ones. That makes it a lot more confortable to make tests or work within the codebase.

Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>

Adapts the configure fn of `KeccakPermutation` according to: #305

keccak256/src/circuit.rs

Since the base conversion table is provided outside but the rho one is internal to the permutation, they both require to be loaded from different sources. Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>

therealyingtong

First-pass review.

keccak256/src/gates/absorb.rs

circuit-benchmarks/src/keccak_permutation.rs

keccak256/src/arith_helpers.rs

keccak256/src/gates/absorb.rs

therealyingtong · 2022-02-07T12:06:34Z

keccak256/src/gates/base_conversion.rs

@@ -148,7 +148,7 @@ impl<F: FieldExt> BaseConversionConfig<F> {
                        region.constrain_equal(input_acc_cell, input_coef_cell)?;
                        region.constrain_equal(output_acc_cell, output_coef_cell)?;
                    } else if offset == input_coefs.len() - 1 {
-                        region.constrain_equal(input_acc_cell, input.0)?;
+                        //region.constrain_equal(input_acc_cell, input.0)?;


Is this meant to be commented out?

It is. We reviewed that with @ChihChengLiang and this leads to a Permutation err as we copy from different cells different rounds.

We should maybe look into this a bit more.

I think we need to enable that copy constraint to bind the output of the base conversion. I imagine we now have the flow like the following. The

| ["Mixing result copies and constraints"] flag(cell) ------(copy)--> computation_pathA ---> resultA(cell) ---(copy)--> | right_side(cell) neg_flag(cell) --(copy)--> computation_pathB ---> resultB(cell) ---(copy)--> | left_side(cell)

per offline discussion with @CPerezz, the copy constraint is required, but it might be difficult to enable in this PR. We can enable it in a future PR.

keccak256/src/gates/mixing.rs

ChihChengLiang

First pass on gates

keccak256/src/gates/mixing.rs

Add missing comments, explanations and spelling errors. Co-authored-by: ying tong <yingtong@z.cash>

As said by CC: We should split 3 polys here. Otherwise, the prover can witness values not satisfying individual polys but satisfying the summation poly. Proof: let x=flag, y=negated_flag flag_consistency: x + y - 1 bool_constraint for x: x(1-x) bool_constraint for y: y(1-y) summation: x + y - 1 + x(1-x) + y(1-y) = 0 falsifying example: x=2 and y=1 Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>

Brechtpd · 2022-02-09T03:33:30Z

https://github.com/appliedzkp/zkevm-circuits/blob/4732717f959cb407a8b8bf6514dbc79c55b64291/keccak256/src/gates/rho_checks.rs#L459-L479

The step2_acc constraint (with final degree 14) and especially the step3_acc constraint (with final degree 171!!!) make the extended degree of the keccak circuit to be 256x the size of the normal domain, which has an enormous impact on performance and memory use. It will definitely be worth it to lower the degree of these constraints by either doing the checks differently in a way that has a lower degree, or use some extra cells so that the full expression can be split up into multiple sub expressions of a smaller degree.

Removing just these two constraints makes the circuit have a much more reasonable extended domain of just x8 bigger. The max degree of 7 is then caused by lookups (which have expressions up to degree 3 as input, but because they are used in the lookup an extra degree of 4 is added with current halo2 with zk), so probably still possible to get it down further with some additional tweaks, but of course will have much less impact than fixing the two constraints above and may end up not being worth it.

ChihChengLiang · 2022-02-09T09:18:33Z

@Brechtpd Nice catch for the performance bottleneck. Do you think using a lookup table check for step2_acc and step3_acc would improve the performance?

therealyingtong

LGTM modulo some small suggestions.

Another nitpick: I think we should avoid prefixing variable names with an underscore _, because that makes the compiler not warn us when these variables are unused.

keccak256/src/circuit.rs

keccak256/src/gates/mixing.rs

therealyingtong · 2022-02-11T05:58:46Z

keccak256/src/circuit.rs

+            // start of the loop.
+            state = {
+                // TODO: That could be a Fixed column.
+                // Witness 1 for the activation flag.


Yes, I think we should make this a Selector.

ChihChengLiang

Found some undeleted comments

keccak256/src/keccak_arith.rs

CPerezz · 2022-02-14T09:40:30Z

Found some undeleted comments

Sorry @ChihChengLiang as mentioned in private, this was the result of pushing some debugging lines that I was not planning to push.

I've force-pushed so that the history is clean now.

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

CPerezz force-pushed the keccak_circuit branch 3 times, most recently from e9c2cec to 065c930 Compare January 26, 2022 13:47

CPerezz and others added 16 commits January 26, 2022 17:55

Add split_state_cells fn to arith helpers

1b6c80e

Since the state is always carried as an `AssignedCell`, this utility fn helps to dettach the `Cell` from it's value `F`. And this is implemented for N-lenght arrays.

Fix round usage for IotaB9 and IotaB13

87c9982

Use Layouter to witness & allocate the state

058332a

This is the way on which al the configs are implemented. And so, we cannot be using `region + offset` approach in any part of the keccak_circuit. Therefore Xi has been refactored to use a `Layouter` in order to witness it's data.

Adapt Mixing gate to circuit usage

ef4def1

Add configure and complete assign fns for Keccak

00acdbe

Add const_generics to state_to_biguint

864971c

Add ABSORB_NEXT_INPUTS const def

3f7f798

Change permute_and_absorb and mixing to match circuit

0e8980f

This makes the implementation of the out-of-circuit primitives for keccak much closer and similar to the inner circuit ones. That makes it a lot more confortable to make tests or work within the codebase.

Add rho checks

857c5ae

Fix round_ctant value to 24 instead of 25

0c61b0d

Change Absorb to internally convert base for next_inp

3b12a69

Add MixingConfig with tests passing

4ff40a6

Add Keccak permutation round circuit

4c119f8

Fix rebase nits

e580cc5

Fix number of iterations for Keccak circuit

421fb3c

Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>

change: Adapt keccak_perm circuit to latest Rho gate

720a44a

Adapts the configure fn of `KeccakPermutation` according to: #305

CPerezz force-pushed the keccak_circuit branch from 065c930 to 720a44a Compare January 26, 2022 17:08

tmp: Add debug prints

f673258

ChihChengLiang reviewed Jan 26, 2022

View reviewed changes

keccak256/src/circuit.rs Show resolved Hide resolved

CPerezz and others added 2 commits January 26, 2022 18:27

Load correct tables for keccak circuit

18481bf

Since the base conversion table is provided outside but the rho one is internal to the permutation, they both require to be loaded from different sources. Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>

fix: Iterate and break CORRECTLY in keccak perm

4947518

CPerezz changed the title ~~[WIP] Keccak circuit implementation~~ Keccak circuit implementation Jan 27, 2022

Merge branch 'main' into keccak_circuit

f9c62e5

CPerezz marked this pull request as ready for review January 27, 2022 11:56

CPerezz added 2 commits January 27, 2022 15:22

change: Rename ABSORB_NEXT_INPUTS to make it clearer

f62b017

Add benchmarks for keccak_permutation

f7892ad

therealyingtong self-requested a review February 7, 2022 10:01

CPerezz requested a review from han0110 February 7, 2022 10:30

therealyingtong requested changes Feb 7, 2022

View reviewed changes

therealyingtong mentioned this pull request Feb 7, 2022

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

Closed

ChihChengLiang reviewed Feb 7, 2022

View reviewed changes

keccak256/src/gates/mixing.rs Outdated Show resolved Hide resolved

CPerezz mentioned this pull request Feb 8, 2022

keccak: Use AssignedCell frol halo2 instead of (Cell,F) #323

Closed

fix: Address YT's marked nits

bdce9c0

Add missing comments, explanations and spelling errors. Co-authored-by: ying tong <yingtong@z.cash>

CPerezz force-pushed the keccak_circuit branch from 877f471 to fcfcda0 Compare February 8, 2022 11:59

CPerezz force-pushed the keccak_circuit branch from fcfcda0 to c496844 Compare February 8, 2022 12:25

CPerezz requested review from therealyingtong and ChihChengLiang February 8, 2022 12:28

This was referenced Feb 9, 2022

reduce degree of stepx_acc range check with a lookup table #324

Closed

Keccak: Add range check table #325

Merged

therealyingtong approved these changes Feb 11, 2022

View reviewed changes

CPerezz mentioned this pull request Feb 14, 2022

Re-enable conflicting equality constraint in base_conversion #331

Closed

ChihChengLiang reviewed Feb 14, 2022

View reviewed changes

keccak256/src/keccak_arith.rs Outdated Show resolved Hide resolved

keccak256/src/keccak_arith.rs Outdated Show resolved Hide resolved

keccak256/src/keccak_arith.rs Outdated Show resolved Hide resolved

keccak256/src/keccak_arith.rs Outdated Show resolved Hide resolved

Fix review nits

36928a4

CPerezz force-pushed the keccak_circuit branch from 89351d0 to 36928a4 Compare February 14, 2022 09:39

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

Merge branch 'main' into keccak_circuit

92b13eb

github-actions bot removed crate-bus-mapping Issues related to the bus-mapping workspace member T-opcode Type: opcode-related and focused PR/Issue labels Feb 14, 2022

CPerezz merged commit 898ff56 into main Feb 14, 2022

CPerezz deleted the keccak_circuit branch February 14, 2022 10:05

CPerezz mentioned this pull request Feb 23, 2022

Bench Keccak Cost #168

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keccak circuit implementation #268

Keccak circuit implementation #268

CPerezz commented Jan 10, 2022 •

edited

therealyingtong left a comment

therealyingtong Feb 7, 2022

CPerezz Feb 8, 2022

ChihChengLiang Feb 8, 2022

ChihChengLiang Feb 9, 2022

ChihChengLiang left a comment

Brechtpd commented Feb 9, 2022 •

edited

ChihChengLiang commented Feb 9, 2022

therealyingtong left a comment

therealyingtong Feb 11, 2022

ChihChengLiang left a comment

CPerezz commented Feb 14, 2022

Keccak circuit implementation #268

Keccak circuit implementation #268

Conversation

CPerezz commented Jan 10, 2022 • edited

therealyingtong left a comment

Choose a reason for hiding this comment

therealyingtong Feb 7, 2022

Choose a reason for hiding this comment

CPerezz Feb 8, 2022

Choose a reason for hiding this comment

ChihChengLiang Feb 8, 2022

Choose a reason for hiding this comment

ChihChengLiang Feb 9, 2022

Choose a reason for hiding this comment

ChihChengLiang left a comment

Choose a reason for hiding this comment

Brechtpd commented Feb 9, 2022 • edited

ChihChengLiang commented Feb 9, 2022

therealyingtong left a comment

Choose a reason for hiding this comment

therealyingtong Feb 11, 2022

Choose a reason for hiding this comment

ChihChengLiang left a comment

Choose a reason for hiding this comment

CPerezz commented Feb 14, 2022

CPerezz commented Jan 10, 2022 •

edited

Brechtpd commented Feb 9, 2022 •

edited