Skip to content
This repository was archived by the owner on Jul 5, 2024. It is now read-only.

[Keccak] Thinest and longest#596

Merged
ChihChengLiang merged 12 commits into
mainfrom
revamp-base-conv
Aug 12, 2022
Merged

[Keccak] Thinest and longest#596
ChihChengLiang merged 12 commits into
mainfrom
revamp-base-conv

Conversation

@ChihChengLiang
Copy link
Copy Markdown
Collaborator

@ChihChengLiang ChihChengLiang commented Jul 1, 2022

highlight:

  • Moved all greeks to the component.rs as functions.
  • 3 advice columns from the generic gate
  • 18 fixed columns from (lookup 1 selector + 3 table columns) * 4 + 1 constant fixed column + 1 selector from generic gate.
  • added the convert back to base 2 part, which was missing in the current implementation.
Num advice columns: 3
Num fixed columns: 18
Num instance columns: 0
Num lookups: 4
Num gates: 1
Max advice rows 64794 - add advice
num polys: 1
max_gate_degree_poly: 3 - 
Circuit layout: Click to expand!

keccak-f

@github-actions github-actions Bot added the crate-keccak Issues related to the keccak workspace member label Jul 1, 2022
@github-actions github-actions Bot added crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member T-bench Type: benchmark improvements labels Aug 9, 2022
@ChihChengLiang ChihChengLiang changed the base branch from refactor-rho-table to main August 10, 2022 13:42
@ChihChengLiang ChihChengLiang marked this pull request as ready for review August 10, 2022 14:08
@ChihChengLiang ChihChengLiang requested a review from CPerezz August 10, 2022 14:08
Copy link
Copy Markdown
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

So far looks nice.

I's always a nice and grateful job if you can chop off 1000+ lines of code!!
I've added a comment and some remarks about performance and layout structure.

Just need you to fix the comment, the other stuff is just informative :)

Comment thread keccak256/src/permutation/tables.rs Outdated
Comment thread keccak256/src/permutation/tables.rs Outdated
let mut input_cells = vec![];
let mut output_b9_cells = vec![];
let mut output_b13_cells = vec![];
for (offset, input_coef) in input_coefs.iter().enumerate() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is where I think we can optimize this a way more by not increasing the offset each time we add and input and instead increase the amount of columns used and not repeat it.


pub fn assign_rho<F: Field>(
layouter: &mut impl Layouter<F>,
base13to9_config: &Base13toBase9TableConfig<F>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we pass the config with the columns already assigned instead of creating new ones, we are directly increasing the height of the entire thing.

Not saying to address it now, but these are the things that are causing us to have really tall layouts.

@CPerezz CPerezz added the benchmarks: Keccak Triggers a Keccak round benchmark label Aug 11, 2022
@CPerezz
Copy link
Copy Markdown
Contributor

CPerezz commented Aug 11, 2022

Results of the benchmarks:

running 1 test
Start:   Setup generation with degree = 19
End:     Setup generation with degree = 19 .........................................4.534s
Start:   Keccak Proof generation with 19 degree
End:     Keccak Proof generation with 19 degree ....................................2.298s
Start:   Keccak Proof verification
End:     Keccak Proof verification .................................................3.668ms
test keccak_permutation::tests::bench_keccak_round ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 3 filtered out; finished in 23.94s

Maximum CPU Usage at 17.8%
Maximum Mem Usage at 4.4Gb

The results are really really nice!
We would just need to bring down the height of the layout of the circuit and make it a bit more wide so that the permutations don't blow up the degree required by the SRS.

Copy link
Copy Markdown
Contributor

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

LGTM!

To compare with #634, this implementation can process 219/64794 = 8.09 keccak_f per 2.298 seconds (≈ 3.52 keccak_f/s).

One low hanging fruit to improve is to stack all tables into one as 4-columns table, with an extra fixed tag column to select the table, it should be helpful to reduce both prove and verify.

@ChihChengLiang ChihChengLiang force-pushed the revamp-base-conv branch 2 times, most recently from 68b30d8 to 9aabf7b Compare August 12, 2022 05:54
@CPerezz
Copy link
Copy Markdown
Contributor

CPerezz commented Aug 12, 2022

LGTM!

To compare with #634, this implementation can process 219/64794 = 8.09 keccak_f per 2.298 seconds (≈ 3.52 keccak_f/s).

One low hanging fruit to improve is to stack all tables into one as 4-columns table, with an extra fixed tag column to select the table, it should be helpful to reduce both prove and verify.

I think the most significant thing to address if we want to bring this impl to the competition is to fix the height of the base conversion.
On that way, we would be able to stack a way more permutations/DEGREE

Copy link
Copy Markdown
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

LGTM!

Nice job @ChihChengLiang 🎉

@ChihChengLiang ChihChengLiang merged commit 89ad089 into main Aug 12, 2022
@ChihChengLiang ChihChengLiang deleted the revamp-base-conv branch May 17, 2023 08:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

benchmarks: Keccak Triggers a Keccak round benchmark crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member crate-keccak Issues related to the keccak workspace member T-bench Type: benchmark improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants