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

[word lo/hi] pi circuit replace rand/rlc by keccak hash #1345

Merged
merged 3 commits into from
Jun 30, 2023
Merged

[word lo/hi] pi circuit replace rand/rlc by keccak hash #1345

merged 3 commits into from
Jun 30, 2023

Conversation

hero78119
Copy link
Member

@hero78119 hero78119 commented Mar 30, 2023 •
edited
Loading

Description

replace rand/rlc by pure keccak hashing

Issue Link

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Contents

  • replace rpi rand/rlc logic by pure hashing
  • simplify public input into just 2 fields: digest[0:16] as hi, and digest[16:32] as lo, while digest = Keccak(<public data>)
  • adopt word-lo-hi

nitpick

  • prefix table column annotation with table name for better debugging

Rationale

[design decisions and extended information]

How Has This Been Tested?

[explanation]

@hero78119 hero78119 marked this pull request as draft March 30, 2023 15:02
@github-actions github-actions bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label Mar 30, 2023
@hero78119 hero78119 changed the title WIP pi circuit revamp WIP improve pi circuit: replace rand/rlc by pures hash Apr 4, 2023
@hero78119 hero78119 changed the title WIP improve pi circuit: replace rand/rlc by pures hash WIP improve pi circuit: replace rand/rlc by pure keccak hashing Apr 4, 2023
@hero78119 hero78119 marked this pull request as ready for review April 6, 2023 04:24
@hero78119 hero78119 changed the title WIP improve pi circuit: replace rand/rlc by pure keccak hashing Improve pi circuit: replace rand/rlc by pure keccak hashing Apr 6, 2023
@github-actions github-actions bot added crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member T-bench Type: benchmark improvements labels Apr 6, 2023
@hero78119 hero78119 force-pushed the pi_revamp branch 3 times, most recently from 0f15455 to 4d9acf0 Compare April 6, 2023 05:02
@hero78119 hero78119 changed the title Improve pi circuit: replace rand/rlc by pure keccak hashing Improve pi circuit: replace rand/rlc by keccak hashing Apr 6, 2023
@hero78119 hero78119 added the T-tech-debt Type: tech-debt generated or cleaned up label Apr 6, 2023
@ed255 ed255 self-requested a review April 13, 2023 11:22
ed255
ed255 reviewed Apr 19, 2023
View reviewed changes
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

First pass review. I think that there are many subtleties in this design, I would appreciate having a python implementation of the design to analyze it better before continuing the Rust implementation.

zkevm-circuits/src/evm_circuit/param.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/param.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/super_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit.rs Outdated Show resolved Hide resolved
@han0110 han0110 self-requested a review April 20, 2023 06:44
han0110
han0110 reviewed Apr 20, 2023
View reviewed changes
Copy link
Collaborator

@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.

First round reviwe, I also agree with Edu that we should use little endian for all values, which might make life easier (considering we'd need to write solidity to handle the decomposition).

zkevm-circuits/src/pi_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit.rs Outdated Show resolved Hide resolved
@hero78119 hero78119 added this to the tech-debt-1 milestone Apr 20, 2023
@hero78119 hero78119 force-pushed the pi_revamp branch 3 times, most recently from b9efdb6 to 71d0e02 Compare April 21, 2023 05:18
@github-actions github-actions bot added the crate-integration-tests Issues related to the integration-tests workspace member label Apr 21, 2023
@hero78119 hero78119 closed this Apr 23, 2023
@hero78119 hero78119 deleted the pi_revamp branch April 23, 2023 04:28
@hero78119 hero78119 restored the pi_revamp branch April 23, 2023 04:31
@hero78119 hero78119 changed the title Improve pi circuit: replace rand/rlc by keccak hashing [word lo/hi] pi circuit replace rand/rlc by keccak hash Jun 14, 2023
@hero78119
Copy link
Member Author

Updated: combined together with word-lo-hi to address #1383 so we can review together. Unitttest under pi circuit is all pass

@hero78119 hero78119 force-pushed the pi_revamp branch 2 times, most recently from 1f41365 to 4c53744 Compare June 14, 2023 09:19
@davidnevadoc davidnevadoc self-requested a review June 15, 2023 16:09
@ed255 ed255 linked an issue Jun 16, 2023 that may be closed by this pull request
Pi Circuit: Refactor word_rlc into word lo/hi #1383
Closed
1 task
@hero78119
define column and constraints
73aac5a 
lookup constraints

pi circuit block table assignment

pi circuit tx table assignment

new way to get public input

circuit assignment ready

pi circuit pass compile

fix cell un-assignment error

format code and bug fix

fixed copy constraints error

fix existing error during unittest

code cosmetics

fix error due to challenge usage

fix lints and code cosmetics

pi circuit more test with real block data

super circuit keccak dev_load

rewrite gate to clearly the branch

rewrite circuit in bottom up style and fix all constriants

refactor timing for pi circuit keccak table load

nonce/gas_limit to u64

code cosmetics

increase keccak row to cover new pi input circuit

skip signdata generation on non pi circuit test

set max keccak row 35000 to cover new pi input circuit

fix pi bytes endian in witness block

fix fmt

migrate to word lo/hi
@hero78119
Copy link
Member Author

Updated: rebase onto the latest word-lo-hi and resolved conflict

@adria0 adria0 self-requested a review June 22, 2023 13:59
@ed255 ed255 modified the milestones: tech-debt-1, word-hi-lo Jun 23, 2023
adria0
adria0 reviewed Jun 28, 2023
View reviewed changes
zkevm-circuits/src/instance.rs
pub chain_id: Word,
/// History hashes contains the most recent 256 block hashes in history,
/// where the latest one is at history_hashes[history_hashes.len() - 1].
pub history_hashes: Vec<Word>,
Copy link
Member

Choose a reason for hiding this comment

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

Since this will be constant, is not better to express it as [Word;256] ?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are other places use Vec on history_hashes therefore I think we can keep it the same. Change all of them is a bit out-of-scope and can be separate task later :)

Copy link
Member Author

@hero78119 hero78119 Jun 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I forget to attach the link to show many other place still use history_hashes as Vec :)
https://github.com/search?q=repo%3Aprivacy-scaling-explorations%2Fzkevm-circuits%20history_hashes%3A%20Vec%3CWord%3E%2C&type=code

zkevm-circuits/src/instance.rs
let result = iter::empty()
.chain(0u8.to_be_bytes()) // zero byte
.chain(block_values.coinbase.to_fixed_bytes()) // coinbase
.chain(block_values.gas_limit.to_be_bytes()) // gas_limit
Copy link
Member

Choose a reason for hiding this comment

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

Spec says big endian?

Copy link
Member Author

Choose a reason for hiding this comment

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

(Should be little endian in specs)

Its a good catch! in specs byte is attaching in little endian, but there is an reverse during assignment to make coding easier. But in Rust, efficient is more important, so the endian in bytes is keep in big endian in first place.

I submit a pr to align specs pi circuit endian privacy-scaling-explorations/zkevm-specs#438

zkevm-circuits/src/instance.rs

// Assign extra fields
let extra_vals = self.get_extra_values();
let result = result
Copy link
Member

Choose a reason for hiding this comment

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

I see in the specs that block hash is also added

Copy link
Member Author

@hero78119 hero78119 Jun 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed in commit 3eaf4b7

I found it's just introduced in mock_block and merge to main branch recently 3575aab

davidnevadoc
davidnevadoc approved these changes Jun 28, 2023
View reviewed changes
Copy link
Contributor

@davidnevadoc davidnevadoc left a comment

Choose a reason for hiding this comment

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

This was quite a big change, thanks a lot for the work!!
Everything is in line with the spec so once @adria0 comments are addressed LGTM 👍

zkevm-circuits/src/pi_circuit.rs Show resolved Hide resolved
adria0
adria0 reviewed Jun 28, 2023
View reviewed changes
zkevm-circuits/src/pi_circuit.rs
.map(|x| x.to_vec())
.collect();

*current_offset = value_bytes_chunk.iter().try_fold(
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if possible to rewrite it using for loops, I think that will help the code reader a lot

Copy link
Member Author

Choose a reason for hiding this comment

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

My option is use functional programming style is more self-explanation than forloop imperative style. With forloop, more intermediate variable need to be allocated. And reader need to be more attention for what each variable meaning. I think try_fold and folding/refreshing on rpi_value_lc make thing clear. So I tend to be keep current style.

(I tried few variants, still think try_fold is better)

Copy link
Member

Choose a reason for hiding this comment

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

In general, I'm more in the imperative side, mainly because I never wrote a big project in functional, so I do not have the "functional architecture mindset".

@adria0
Copy link
Member

adria0 commented Jun 28, 2023

Overall, LGTM 👍
Thanks for the hard work doing this PR!

@hero78119
Copy link
Member Author

All review comment are addressed! thanks for all review and will merge it now

@hero78119 hero78119 merged commit dd8e246 into privacy-scaling-explorations:word-lo-hi Jun 30, 2023
8 checks passed
This was referenced Jun 30, 2023
Closed
Closed
@hero78119 hero78119 mentioned this pull request Jul 13, 2023
Merged
cameron1024 pushed a commit to cameron1024/zkevm-circuits that referenced this pull request Jun 30, 2024
@roynalnaruto
fix: table kind should not change over the fse code trailing bits (pr…
92fcf47 
…ivacy-scaling-explorations#1345)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@adria0 adria0 adria0 approved these changes

@han0110 han0110 han0110 left review comments

@ed255 ed255 ed255 left review comments

@davidnevadoc davidnevadoc davidnevadoc approved these changes

Assignees
No one assigned
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member crate-integration-tests Issues related to the integration-tests workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member T-bench Type: benchmark improvements T-tech-debt Type: tech-debt generated or cleaned up
Projects
None yet
Milestone
word-hi-lo
Development

Successfully merging this pull request may close these issues.

Pi Circuit: Refactor word_rlc into word lo/hi revamp raw public input rlc by Keccak hash.
6 participants
@hero78119 @ChihChengLiang @adria0 @davidnevadoc @han0110 @ed255