add state circuit to super circuit by lispc · Pull Request #670 · privacy-ethereum/zkevm-circuits

lispc · 2022-08-08T08:47:57Z

most of the changes are for extracting the "rw table" out of state circuit so it can be shared between evm circuit and state circuit. Most of circuit config and assignment codes are moved from StateCircuit to StateCircuitConfig similar to EvmCircuit, so StateCircuit now becomes a "tester" circuit similar to TestCircuit in evm_circuit.rs, or "MyCircuit" in bytecode circuit / copy circuit / tx circuit.

My concern is that moving codes from StateCircuit to StateCircuitConfig results large diffs and makes it a bit difficult to review... Any suggestions..?

I manually tested this PR using super circuit with cmd cargo test --release -p zkevm-circuits skip_test_super_circuit -- --ignored and it passed.

TODO:

the newly added "aux2" and "value_prev" cols in rw table are not constrained yet. After #621 is merged, we can revisit how to constrain the relationship between initial_value / committed_value / prev_value.

DreamWuGit · 2022-08-11T07:25:57Z

-
+impl RwMap {
+    /// Check rw_counter is continous and starting from 1
+    pub fn check_rw_counter_sanity(&self) {


i like this check style !!

DreamWuGit · 2022-08-11T07:40:25Z

+pub struct StateCircuit<F> {
    pub(crate) randomness: F,
    pub(crate) rows: Vec<Rw>,
    pub(crate) n_rows: usize,


one question: Does n_rows need to be equal to evm circui rows number in real proof ?

no. n_rows is the rows of rw table. rw table in evm circuit and state circuit should be same. Not related to the total rows (max of fixed_table, rw_table, tx_table, other parts..) of circuit.

DreamWuGit · 2022-08-11T07:47:29Z

+            &mut layouter,
+            &self.block.rws.table_assignments(),
+            self.block.randomness,
+            self.block.state_circuit_pad_to,


where is state_circuit_pad_to assigned and how to decide the value ?

it is left to do in the final circuit (aggregation circuit / super circuit / etc) in the future. Currently we can either set it to (1<<k - 64), or in tests set it to 0 to indicate no padding needed.

DreamWuGit · 2022-08-11T07:56:24Z

    keccak_table: KeccakTable,
    copy_table: CopyTable,
    evm_circuit: EvmCircuit<F>,
+    state_circuit: StateCircuitConfig<F>,


not sure if need add state_circuit into L86 like tx_circuit I see it will be used for unit tests

nonono TxCircuit is witness, while StateCircuitConfig is columns.... See for more details: #650

DreamWuGit

overall LGTM!! leave minor comments

z2trillion · 2022-08-11T16:12:04Z

Are we still planning to move to RLC based lookups in the future?

I think the value_prev column in the RwTable can just be constrained to be equal to the value_prev in the state circuit constraint builder.

roynalnaruto

Just a couple of questions, looks good overall. Also the copy_circuit.rs needs resolving conflict from the last merge :)

roynalnaruto · 2022-08-11T16:15:12Z

-
+impl RwMap {
+    /// Check rw_counter is continous and starting from 1
+    pub fn check_rw_counter_sanity(&self) {


In case the RWs do not pass the sanity check, do we want to panic (the debug_assert_eq will panic)? Or handle the error before loading the RW table?

there are two scenarios that cannot satisfy check_rw_counter_sanity. (1)in evm circuit witness is wrong, it is bug so we debug_assert (2) we manually construct test cases for state circuit where rw_counter may not be continuous.

So i only check_rw_counter_sanity in evm circuit assignment, and let it fail if error happens

z2trillion

looks good

lispc · 2022-08-12T02:17:51Z

Are we still planning to move to RLC based lookups in the future?

i am not sure. Even in scroll-dev branch i no longer use RLC to reduce the long term fork maintenance burden at the cost of 1-2% more rows in aggregation circuit.

I think the value_prev column in the RwTable can just be constrained to be equal to the value_prev in the state circuit constraint builder.

there are still something missing to constrain the relationship between value.prev() <=> value_prev.cur().

At least Rw::Account prev value cannot satisfy the equality. In another branch i constrain value_prev like this: https://github.com/scroll-tech/zkevm-circuits/blob/bdf8fd3361d49fc6d8110489d744ce202cf022ee/zkevm-circuits/src/state_circuit/constraint_builder.rs#L137-L186

* implement RLC chip * keccak table interfaces * minor * mock chunk tests * aggregation tests * fixes * supporing empty hash preimage * minor * remove tmp data * clean up and address comments * update docs * partial fix clippy * fix chunk construction bug * fix compiling bug * fix padded chunk's data hash * fix bug in number of valid chunks * handle the case of all valid ones * add fixed columns to rlc config * enforce the chunks are orderred * fix bugs in challenges * add missing selectors for aggregation circuit * fix the fixed cells (privacy-ethereum#677) * try to reenable skip first pass * tr to fix skip first pass again * try to fix again * clean up * fixed skip first pass * aggregation parameter with k = 19 * removing unused features; partial address comments * fix clippy * fix clippy * address comment on fixed cells * fix dynamic hash test * try to fix new fixed cells * reverting last 3 commits * Add conversion from witness `Block` to `ChunkHash`. (privacy-ethereum#683) --------- Co-authored-by: Steven <asongala@163.com>

…s for the dummy chunk proofs to avoid dummy chunk proofgen time (#712) * partiall address review comments of #670 * disable disable_proof_agg by default * fix soundness issues with data hash * add fixed cells for chunk_is_valid * fix typo * well... need morning coffee * typo in readme * fix soundness issue in is_smaller_than * [feat] unify u8 u16 lookup table (#694) * add range table * replace Range256Table of rlp_circuit_fsm * replace u16_table of tx_circuit * use TableColumn * add type alias * use type alias * annotate lookup column * opt min_num_rows_block in poseidon_circuit (#700) prune debug prints and lint * speedup ci using verify_at_rows (#703) * speedup ci using verify_at_rows * use verify_at_rows to speedup super circuit ci tests * update super circuit row estimation API (#695) * remove num_of_chunks in aggregation circuit's instance column (#704) Co-authored-by: kunxian xia <xiakunxian130@gmail.com> * fix: lost tx_type when doing type conversion (#710) * fix condition (#708) * gates for zero checks * statement 1 is correct * reenable statements 3,6,7 * reenable statement 4 * everything seems to work again * update aggregation test accordingly * update spec * minor clean up * Fix fmt. * Make `ChunkHash` fields public. * fix decompose function * update figures * clean up * address comments * add is_final checks * update readme * constraint hash input length * fix clippy --------- Co-authored-by: Akase Cho <lightsing@users.noreply.github.com> Co-authored-by: Ho <noel.wei@gmail.com> Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com> Co-authored-by: kunxian xia <xiakunxian130@gmail.com> Co-authored-by: Steven Gu <asongala@163.com>

add state circuit to super circuit

9827796

lispc requested a review from miha-stopar as a code owner August 8, 2022 08:47

github-actions Bot added crate-gadgets Issues related to the gadgets workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Aug 8, 2022

lispc marked this pull request as draft August 8, 2022 09:00

fix

ceb1442

lispc marked this pull request as ready for review August 8, 2022 09:06

lispc changed the title ~~add state circuit to super circuit~~ WIP add state circuit to super circuit Aug 8, 2022

lispc removed the request for review from miha-stopar August 8, 2022 09:07

lispc linked an issue Aug 8, 2022 that may be closed by this pull request

SuperCircuit: Add StateCircuit #662

Closed

lispc mentioned this pull request Aug 8, 2022

state circuit: add prev value column and constrain it privacy-ethereum/zkevm-specs#202

Closed

lispc added 2 commits August 9, 2022 14:49

remove QUICK_CHECK

1ccd39b

clean codes

171abb7

lispc changed the title ~~WIP add state circuit to super circuit~~ add state circuit to super circuit Aug 9, 2022

lispc requested review from ed255, roynalnaruto and z2trillion and removed request for ed255 and z2trillion August 9, 2022 07:31

lispc and others added 2 commits August 10, 2022 20:14

Merge branch 'main' into feat/add_state_to_super

1322e68

Merge branch 'main' into feat/add_state_to_super

6330e39

DreamWuGit reviewed Aug 11, 2022

View reviewed changes

DreamWuGit approved these changes Aug 11, 2022

View reviewed changes

z2trillion suggested changes Aug 11, 2022

View reviewed changes

Comment thread zkevm-circuits/src/state_circuit.rs Outdated

Comment thread zkevm-circuits/src/state_circuit/lexicographic_ordering.rs Outdated

Comment thread zkevm-circuits/src/state_circuit/multiple_precision_integer.rs

Comment thread zkevm-circuits/src/state_circuit/random_linear_combination.rs

roynalnaruto approved these changes Aug 11, 2022

View reviewed changes

lispc added 2 commits August 12, 2022 10:08

address review comments

e7a1269

Merge remote-tracking branch 'origin/main' into feat/add_state_to_super

51e2c03

lispc requested a review from z2trillion August 12, 2022 02:12

z2trillion approved these changes Aug 12, 2022

View reviewed changes

lispc merged commit 1c60be2 into privacy-ethereum:main Aug 12, 2022

lispc deleted the feat/add_state_to_super branch August 12, 2022 02:30

Conversation

lispc commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DreamWuGit Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DreamWuGit left a comment

Choose a reason for hiding this comment

Uh oh!

z2trillion commented Aug 11, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roynalnaruto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

z2trillion left a comment

Choose a reason for hiding this comment

Uh oh!

lispc commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lispc commented Aug 8, 2022 •

edited

Loading

DreamWuGit Aug 11, 2022 •

edited

Loading

lispc commented Aug 12, 2022 •

edited

Loading