merge word-lo-hi to main #1504

hero78119 · 2023-06-30T07:13:24Z

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 branch word-lo-hi-merge-main is created which covers

based on latest word-lo-hi branch
remove _word suffix and remove deprecated table field
also sync main and resolve conflict
Fix problem on main branch: applied a workaround max_rwc + 1 + 2 magic number mention in another issue Investigate error log on ci light-test padding_len overflow #1507
~~investigate super circuit degree 9 -> 12~~ optimise to reduce super circuit degree #1499 After fixed, degree 9 -> 10

With this pr, reviewer just need to review once.

Issue Link

#1509

Type of change

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

### 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

…1496) ### Description retain all test ### Issue Link Close #1478 ### Type of change - [x] Bug fix (non-breaking change which fixes an issue) ### Content - retain/rollback ci.yml workaround changes - fix copy circuit unittest

### 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_]

hero78119 · 2023-07-03T03:31:10Z

Updated: its ready for review for all ci checked passed

adria0

Since we already reviewed and approved each part individually, I approve it.

ChihChengLiang

LGTM.
I added a nitpick.
I also highlighted some changes introduced only in this PR but not in previous PRs.

ChihChengLiang · 2023-07-03T08:32:24Z

zkevm-circuits/src/super_circuit/test.rs

-    assert!(cs.degree() <= 9);
+    assert!(cs.degree() <= 12);


Note: In this PR, ~~the super_circuit degree is 12.~~

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

Updated in latest commit.

Final degree 9 -> 10

ChihChengLiang · 2023-07-03T08:34:06Z

zkevm-circuits/src/evm_circuit/param.rs

-pub const MAX_STEP_HEIGHT: usize = 21;
+pub const MAX_STEP_HEIGHT: usize = 19;


Note: MAX_STEP_HEIGHT is reduced from 21 to 19.

ChihChengLiang · 2023-07-03T08:35:39Z

bus-mapping/src/circuit_input_builder.rs

-                max_rws: max_rws + 1,
+                max_rws: max_rws + 3,


Note: A workaround to fix padding overflow
See #1507

integration-tests/src/integration_test_circuits.rs

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

hero78119 and others added 19 commits May 17, 2023 14:23

[word lo-hi] fix evm circuit build failed (#1446)

31f32e1

### Description fix build error ### Issue Link Closed #1442 ### Type of change - [x] Bug fix (non-breaking change which fixes an issue)

[word lo/hi] docs and more word utilities (#1441)

4e611b8

### 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

remove _word suffix

abf069e

Merge branch 'main' into feat/remove_word_suffix

d78d7f4

merge with main

4f4b32a

hero78119 marked this pull request as draft June 30, 2023 07:13

retain all ci

bde3eb2

github-actions bot removed the CI Issues related to the Continuous Integration mechanisms of the repository. label Jun 30, 2023

hero78119 force-pushed the word-lo-hi-merge-main branch from 5ad03df to 808e74a Compare June 30, 2023 08:10

github-actions bot removed the crate-mock Issues related to the mock workspace member label Jun 30, 2023

hero78119 changed the title ~~merge word-lo-hi to main~~ [Draft] test ci: merge word-lo-hi to main Jun 30, 2023

hero78119 marked this pull request as ready for review June 30, 2023 08:13

hero78119 changed the title ~~[Draft] test ci: merge word-lo-hi to main~~ [Draft] [light test failed, investigating] merge word-lo-hi to main Jun 30, 2023

hero78119 added 2 commits June 30, 2023 23:48

fix conflicts

abcd22b

wordaround to fix padding overflow

360ad81

hero78119 force-pushed the word-lo-hi-merge-main branch from 808e74a to 360ad81 Compare June 30, 2023 15:49

reduce step height

4bca50e

hero78119 changed the title ~~[Draft] [light test failed, investigating] merge word-lo-hi to main~~ merge word-lo-hi to main Jul 3, 2023

lispc mentioned this pull request Jul 3, 2023

Memory opt with word look up scroll-tech/zkevm-circuits#466

Merged

23 tasks

adria0 self-requested a review July 3, 2023 08:23

adria0 approved these changes Jul 3, 2023

View reviewed changes

adria0 mentioned this pull request Jul 3, 2023

BeginTx missing constraints #1475

Closed

ChihChengLiang linked an issue Jul 3, 2023 that may be closed by this pull request

Merging Word Lo-hi to main #1509

Closed

ChihChengLiang self-requested a review July 3, 2023 08:30

hero78119 mentioned this pull request Jul 3, 2023

Investigate error log on ci light-test padding_len overflow #1507

Closed

ChihChengLiang approved these changes Jul 3, 2023

View reviewed changes

hero78119 added 3 commits July 3, 2023 16:55

fixed breaking format in macro

9063b65

Merge branch 'word-lo-hi' into word-lo-hi-merge-main

ea8aad4

hero78119 added this pull request to the merge queue Jul 3, 2023

Merged via the queue into main with commit ee93a18 Jul 3, 2023
19 checks passed

hero78119 deleted the word-lo-hi-merge-main branch July 3, 2023 14:04

ed255 mentioned this pull request Jul 3, 2023

Meta: Refactor word_rlc into word hi/lo #1221

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

merge word-lo-hi to main #1504

merge word-lo-hi to main #1504

hero78119 commented Jun 30, 2023 •

edited

Loading

hero78119 commented Jul 3, 2023

adria0 left a comment

ChihChengLiang left a comment

ChihChengLiang Jul 3, 2023 •

edited

Loading

hero78119 Jul 3, 2023

hero78119 Jul 3, 2023

ChihChengLiang Jul 3, 2023

ChihChengLiang Jul 3, 2023

		pub const MAX_STEP_HEIGHT: usize = 21;
		pub const MAX_STEP_HEIGHT: usize = 19;

merge word-lo-hi to main #1504

merge word-lo-hi to main #1504

Conversation

hero78119 commented Jun 30, 2023 • edited Loading

Description

Issue Link

Type of change

Contents

hero78119 commented Jul 3, 2023

adria0 left a comment

Choose a reason for hiding this comment

ChihChengLiang left a comment

Choose a reason for hiding this comment

ChihChengLiang Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

hero78119 Jul 3, 2023

Choose a reason for hiding this comment

hero78119 Jul 3, 2023

Choose a reason for hiding this comment

ChihChengLiang Jul 3, 2023

Choose a reason for hiding this comment

ChihChengLiang Jul 3, 2023

Choose a reason for hiding this comment

hero78119 commented Jun 30, 2023 •

edited

Loading

ChihChengLiang Jul 3, 2023 •

edited

Loading