-
Notifications
You must be signed in to change notification settings - Fork 856
Conversation
### Description [_PR description_] ### Issue Link #1388 ### Type of change - [x] Breaking change (fix or feature that would cause existing functionality to not work as expected) ### Contents - [x] define general type `Word` and `WordLimbs` along with util function to replace RandomLinearCombination in `word.rs` - [x] evm util function/common gadgets switch to new Word type - [ ] (Ongoing, partially done) EVM circuit switch to new Word type Pending List ### Rationale Defined `WordN`, where N represent number of limbs, where the word size is EVM word 256bits. For instance, `Word4` means 4 limbs, each limb 64 bits. Giving 2 word word_n1, word_n2, and n1 % n2 = 0, There are util function to convert n1 to n2. Words related utility are defined in new file `word.rc` constraint builder also introduce few util apis - util functions to support query `WordN` cells. For N=32, limb cell integrate with byte lookup. For N < 32, need caller to have range check carefully. - word equality check - separate read/write api to hint which need careful range check #### Range check strategy State circuit will assure read value match with last write. Therefore, for lookup table except `stack.pop`, such as txtable, blocktable, callcontext, we need to assure value `write` got proper range check carefully. For read part, range check can be skip by purpose. Special note for `stack.pop`, since some arithmetcis operation might need special range for operand from stack, and stack can be any value. Therefore, some case read part also need range check carefully, unless it's just some equality check, like `CmpGadget`, where the value range is not matter, such kind of operation can skip range check. > TODO Any other case need range check during read? ### Pending tasks - [ ] evm circuit switch to word type. It will be finished in another evm circuit pr. In this pr have incomplete evm circuit work, just to verify the usage of utility functions. - [ ] optimise cell comsumption, e.g. U64 type, such as `gas` type which just need 8 bytes cells range check. For example, refer `CommonCallGadget` gas type. - [ ] support byte16 range check - [ ] review common_gadget all range check. - [ ] review reversible info range check - [ ] review word::from_lo_uncheck range check - [ ] optimise range check for lookup read, see comment #1394 (comment) - [ ] replace `Word<Expression<F>>` with generic `WordExpr<F>` in lookup so can save `to_word()` conversion in most of gadget - [ ] static assertion on const generic check - [ ] review memory to assure if length is 0 then offset can be larger than u64 ### How Has This Been Tested? - [ ] compile pass first - [ ] all unittest pass <hr> ---------
### Description The current plan to implement word-lo-hi is to divide the work for different circuits among different implementors. The problem is that the word-lo-hi branch does not build, so the implementors of different circuits do not know if they are implementing the circuit correctly. The word-lo-hi branch does not build because we introduced many incompatible changes that propagate in the codebase beyond the reasonable effort to fix the build. We can resolve this issue by bringing back some old functions and gadgets for compatibility. Meanwhile, we mark them as deprecated. ### Issue Link
### Description fix build error ### Issue Link Closed #1442 ### Type of change - [x] Bug fix (non-breaking change which fixes an issue)
### Description Follow #1435 design duplicate field design. ### Type of change - [x] New feature (non-breaking change which adds functionality) ### Contents - more utilities on `Word` type, and further refactor few to be more generics
### Description Support word lo-hi for copy circuit ### Issue Link Address #1384 ### Type of change - [x] Breaking change (fix or feature that would cause existing functionality to not work as expected) ### Contents - Change the column id to word lo hi columns
### Description Support word lo-hi in keccak circuit ### Issue Link #1385 ### Type of change - [x] Breaking change (fix or feature that would cause existing functionality to not work as expected) ### Contents - Change the keccak circuit hash output format from RLC to word lo-hi. - Add convenient methods `map` for Word. This can easily map two limbs to other types. - Add a constructor for the zero known value word. ### Rationale ### Testss The following tests can pass if we remove math_gadget tests below ``` cargo test -p zkevm-circuits packed_multi_keccak_simple --features test ``` ``` zkevm-circuits/src/evm_circuit/util/math_gadget/add_words.rs zkevm-circuits/src/evm_circuit/util/math_gadget/cmp_words.rs zkevm-circuits/src/evm_circuit/util/math_gadget/lt_word.rs zkevm-circuits/src/evm_circuit/util/math_gadget/min_max_word.rs zkevm-circuits/src/evm_circuit/util/math_gadget/modulo.rs zkevm-circuits/src/evm_circuit/util/math_gadget/mul_add_words.rs ```
### Description State Circuit: Refactor word_rlc into word lo/hi ### Issue Link - #1382 ### Contents - As expected, changing rlc encoded values into `word::Word`, as defined in [`rw_table` specs](https://github.com/privacy-scaling-explorations/zkevm-specs/blob/master/specs/tables.md#rw_table) - Add helper methods to `ConstraintBuilder` (like `require_word_zero` ) - Make MPI able to manage multi-field values (to be able to use U256's) - Remove `random_linear_combination.rs` because Word uses MPI instead rlc - Removed test focused on test rlc - Removed unused field `aux1` and renamed `aux2` to `init_val` - Added helper methods to `word::Word` - Also updated `mpt` witness to pass the state tests ### Rationale The lexicographic ordering, used word_rlc randomness for [another use](https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/e3e6034a11349cb07dd08e88026e3ca40d771a66/zkevm-circuits/src/state_circuit/lexicographic_ordering.rs#L312). Since this randomess is going to be removed, we have three options here: - Use another method to build the constraints that does not uses rlc at all - Create another challenge value - Use another existing randomness In order to change only the code concerning hi/lo refactor, the choice is to use the [keccak randomness](https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/e3e6034a11349cb07dd08e88026e3ca40d771a66/zkevm-circuits/src/state_circuit.rs#L97) instead
### Description merge main to word-lo-hi branch ### Issue Link N/A ##### File conflicts list, other file merge from main successfully ``` Conflict file path: deleted: keccak256/src/gate_helpers.rs both modified: zkevm-circuits/src/copy_circuit.rs both modified: zkevm-circuits/src/evm_circuit/execution.rs both modified: zkevm-circuits/src/evm_circuit/execution/balance.rs both modified: zkevm-circuits/src/evm_circuit/execution/begin_tx.rs both modified: zkevm-circuits/src/evm_circuit/execution/end_tx.rs both modified: zkevm-circuits/src/evm_circuit/execution/logs.rs both modified: zkevm-circuits/src/evm_circuit/step.rs both modified: zkevm-circuits/src/evm_circuit/util.rs both modified: zkevm-circuits/src/evm_circuit/util/common_gadget.rs both modified: zkevm-circuits/src/state_circuit.rs both modified: zkevm-circuits/src/state_circuit/constraint_builder.rs both modified: zkevm-circuits/src/table.rs both modified: zkevm-circuits/src/witness/rw.rs ``` After fixing conflict, file change list ``` modified: zkevm-circuits/src/evm_circuit/execution/create.rs modified: zkevm-circuits/src/evm_circuit/util/constraint_builder.rs modified: zkevm-circuits/src/table/block_table.rs modified: zkevm-circuits/src/table/bytecode_table.rs modified: zkevm-circuits/src/table/copy_table.rs modified: zkevm-circuits/src/table/keccak_table.rs modified: zkevm-circuits/src/table/mpt_table.rs modified: zkevm-circuits/src/table/rw_table.rs modified: zkevm-circuits/src/table/tx_table.rs modified: zkevm-circuits/src/util/word.rs ``` --------- Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com> Co-authored-by: adria0.eth <5526331+adria0@users.noreply.github.com> Co-authored-by: adria0 <adria0@nowhere> Co-authored-by: Eduard S <eduardsanou@posteo.net> Co-authored-by: Han <tinghan0110@gmail.com> Co-authored-by: Paul <108982045+ChengYueJia@users.noreply.github.com> Co-authored-by: Steven <asongala@163.com> Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com> Co-authored-by: KimiWu <leonhartwu@gmail.com> Co-authored-by: Raphael <raphael.r.toledo@gmail.com> Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com> Co-authored-by: naure <naure@users.noreply.github.com> Co-authored-by: Aurélien Nicolas <info@nau.re> Co-authored-by: Enrico Bottazzi <85900164+enricobottazzi@users.noreply.github.com> Co-authored-by: Raphael <contact@raphael-toledo.com>
### Description This PR is based on #1394 Need to merge #1394 first before review this. ### Issue Link #1379 ### Type of change - [x] Breaking change (fix or feature that would cause existing functionality to not work as expected) ### Contents - [x] fixed most of op compiling errors other than `callop.rs` and `begin_tx.rs`. - [x] fixed `callop.rs` and `begin_tx.rs` - [x] remove all compatible workaround `construct_new` under evm circuit - [x] unittest under evm circuit all pass `cargo test --features warn-unimplemented --features test --package zkevm-circuits --lib -- evm_circuit::execution::` - [x] fix few `word` gadgets generics to take `Word<T>` instead of `T` to restrict it flexibility, since it's non sense to put type not related to word - [x] remove most of `deprecated` api under evm circuits - [x] add IntDecomposition type as an alternative to RandomLinearComposition, with base 256 ### Cell utilization on main branch vs on word-lo-hi branch #### Storage_1 ``` Main: +-----------------------------------+------------------------------+-------------------------+ | "storage_1" total_available_cells | "storage_1" total_used_cells | "storage_1" Utilization (%) | +-----------------------------------+------------------------------+-------------------------+ | 25480 | 6482 | 25.4 | +-----------------------------------+------------------------------+-------------------------+ Word-lo-hi +-----------------------------------+------------------------------+-----------------------------+ | "storage_1" total_available_cells | "storage_1" total_used_cells | "storage_1" Utilization (%) | +-----------------------------------+------------------------------+-----------------------------+ | 24080 | 7078 | 29.4 | +-----------------------------------+------------------------------+-----------------------------+ ``` #### Storage_2 ``` Main +-----------------------------------+------------------------------+-------------------------+ | "storage_2" total_available_cells | "storage_2" total_used_cells | "storage_2" Utilization | +-----------------------------------+------------------------------+-------------------------+ | 1456 | 467 | 32.1 | +-----------------------------------+------------------------------+-------------------------+ Word-lo-hi +-----------------------------------+------------------------------+-----------------------------+ | "storage_2" total_available_cells | "storage_2" total_used_cells | "storage_2" Utilization (%) | +-----------------------------------+------------------------------+-----------------------------+ | 1376 | 14 | 1.0 | +-----------------------------------+------------------------------+-----------------------------+ ``` #### Byte_lookup ``` Main +-------------------------------------+--------------------------------+---------------------------+ | "byte_lookup" total_available_cells | "byte_lookup" total_used_cells | "byte_lookup" Utilization | +-------------------------------------+--------------------------------+---------------------------+ | 8736 | 6786 | 77.7 | +-------------------------------------+--------------------------------+---------------------------+ Word-lo-hi +-------------------------------------+--------------------------------+-------------------------------+ | "byte_lookup" total_available_cells | "byte_lookup" total_used_cells | "byte_lookup" Utilization (%) | +-------------------------------------+--------------------------------+-------------------------------+ | 8256 | 6566 | 79.5 | +-------------------------------------+--------------------------------+-------------------------------+ ``` --------- Co-authored-by: Wu Sung-Ming <wusm@sea.com>
…1488) ### Description Add back - evm_circuit - keccak_circuit - exp_circuit - pi_circuit - state_circuit unittest to ci Other subcircuit `tx_circuit/copy_circuit/bytecode_circuit/root_circuit` still failed for now, will be fixed in another pr ### Issue Link [N/A](#1487) ### Type of change - [x] New feature (non-breaking change which adds functionality) ### Contents - Fix unittest assertion [failed](https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/word-lo-hi/zkevm-circuits/src/util/int_decomposition.rs#L39-L43) due to sanity check on address H160 retrieved from U256. We need to clean up MSB 12 bytes to 0 - add subcircuit light tests on CI in whitelist based
### Description Bytecode Circuit: Refactor word_rlc into word lo/hi ### Issue Link Closes #1380 ### Type of change - [X] Refactor - [X] Lo/Hi ### Contents Bytecode Circuit: Refactor word_rlc into word lo/hi ### Rationale In Issue ### How Has This Been Tested? Running unit tests
### Description [_PR description_] ### Issue Link Close #1426 Depends by #1414 ### Type of change - [x] Breaking change (fix or feature that would cause existing functionality to not work as expected) ### Contents - [x] constraint builder in evm circuit support u16 lookup - [x] rename `byte_table` to `u8_table` and `query_byte` to `query_u8` so it align with `query_u16` and so on in the future - [x] refactor state circuit and evm circuit to reuse range table.
### Description part of word lo/hi ### Issue Link #1387 ### Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [x] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Contents - applied word lo/hi to tx_circuit --------- Co-authored-by: sm.wu <hero78119@gmail.com> Co-authored-by: Eduard S <eduardsanou@posteo.net>
### Description Clean up workaround introduced in #1435 ### Issue Link #1487 ### Type of change - [x] Bug fix (non-breaking change which fixes an issue) ### Contents - it touch many files just under evm_circuit. Will not cause conflict with other word-lo-hi pr - all just cleanup/renaming without any logic change
### Description replace rand/rlc by pure keccak hashing ### Issue Link - #1344 - #1383 ### Type of change - [x] 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_]
5ad03df
to
808e74a
Compare
808e74a
to
360ad81
Compare
Updated: its ready for review for all ci checked passed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we already reviewed and approved each part individually, I approve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I added a nitpick.
I also highlighted some changes introduced only in this PR but not in previous PRs.
assert!(cs.degree() <= 9); | ||
assert!(cs.degree() <= 12); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: In this PR, the super_circuit degree is 12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick fix to reduce degree from 12 -> 10 in this pr #1508. After pr is appoved and merged will update this pr as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in latest commit.
Final degree 9 -> 10
pub const MAX_STEP_HEIGHT: usize = 21; | ||
pub const MAX_STEP_HEIGHT: usize = 19; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: MAX_STEP_HEIGHT is reduced from 21 to 19.
max_rws: max_rws + 1, | ||
max_rws: max_rws + 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: A workaround to fix padding overflow
See #1507
### Description [_PR description_] ### Issue Link #1499 ### Type of change - [x] Bug fix (non-breaking change which fixes an issue) ### Contents This pr is to rewrite/split copy circuit constrains to trade more gates with less degree. copy table `tag` field is binary number decomposition with degree N (N represent number of bits, in copy table case N = 3). Once put in `or::expr([#num_or_condition])`, gate degrees will be exploded quickly by `N*#num_or_condition`. Before PR, copy circuit max is degree 12, After PR, copy circuit max 6 degree with 2 more gates. However, circuit degree upper bound still increase by 1 (9 -> 10) for word-lo-hi refactor, due to state-circuit `batch_is_zero-"is_zero is 1 if values are all zero">` gate degrees from 6 -> 10 Due to we fold 2 extra column, each contribute 2 degrees https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/word-lo-hi/zkevm-circuits/src/state_circuit.rs#L129-L141. State circuit need some discussion, as I thought rewrite it might be a bit over-engineering.
Description
This pr is to complete milestone
word-lo-hi
back to main branch.Since
word-lo-hi
branch is out-of-sync with main branch due to few features merged to main recently, in order to save review effort to review 1 pr instead of 2 pr, a new branchword-lo-hi-merge-main
is created which covers_word
suffix and remove deprecated table fieldmax_rwc + 1 + 2
magic number mention in another issue Investigate error log on ci light-test padding_len overflow #1507investigate super circuit degree 9 -> 12optimise to reduce super circuit degree #1499 After fixed, degree 9 -> 10With this pr, reviewer just need to review once.
Issue Link
#1509
Type of change
Contents
to complete milestone
word-lo-hi