[word-lo-hi] evm circuit by hero78119 · Pull Request #1411 · privacy-ethereum/zkevm-circuits

hero78119 · 2023-05-16T07:49:35Z

Description

This PR is based on #1394
Need to merge #1394 first before review this.

Issue Link

#1379

Type of change

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

fixed most of op compiling errors other than callop.rs and begin_tx.rs.
fixed callop.rs and begin_tx.rs
remove all compatible workaround construct_new under evm circuit
unittest under evm circuit all pass cargo test --features warn-unimplemented --features test --package zkevm-circuits --lib -- evm_circuit::execution::
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
remove most of deprecated api under evm circuits
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                          |
+-------------------------------------+--------------------------------+-------------------------------+

hero78119 · 2023-05-17T16:28:12Z

related to #1415

hero78119 · 2023-05-23T08:23:01Z

Update: fix caller/callee/coinbase cell to word type

hero78119 · 2023-05-23T13:30:07Z

Update: fix log opcode topic type to word 43c4d9f

ed255 · 2023-06-15T15:28:22Z

This is a big PR and while reviewing I think I can categorize my comments into:

a. Something that I think is wrong
b. Something that I think should be improved at API level / code style
c. Something that I think should be improved for optimization (cell usage or constraints)
d. Improvements that I see that are unrelated to this refactor. I try not to focus on these to avoid noise!

I think the most important one to resolve is a. If it's an over-constraint, we should be able to detect them also via the unit tests. If it's an under constraint, that's more difficult to detect other than via review.

For b and c I think we could defer to resolve some of the discussions to a future PR if @hero78119 considers it (on a case by case basis). This way we can get this PR merged sooner, and work on b and c after the unit tests are passing, so that we can apply changes more confidently.

ChihChengLiang · 2023-06-16T09:32:49Z

@ed255 Good point. I'll focus on finding issues in a in the review.

hero78119 · 2023-06-16T10:04:41Z

This is a big PR and while reviewing I think I can categorize my comments into:

a. Something that I think is wrong

b. Something that I think should be improved at API level / code style

c. Something that I think should be improved for optimization (cell usage or constraints)

d. Improvements that I see that are unrelated to this refactor. I try not to focus on these to avoid noise!

I think the most important one to resolve is a. If it's an over-constraint, we should be able to detect them also via the unit tests. If it's an under constraint, that's more difficult to detect other than via review.

For b and c I think we could defer to resolve some of the discussions to a future PR if @hero78119 considers it (on a case by case basis). This way we can get this PR merged sooner, and work on b and c after the unit tests are passing, so that we can apply changes more confidently.

It make senses! since evm circuit touch many change, I think first huge pr we can focus a more. Besides I will try to fix few related to b, c, d as well. My principle for this pr is all evm circuit unittest test should pass (Right now it's already in this state ✅ and so as later change should keep all uniitest pass as well!)

ChihChengLiang

LGTM after addressing all issues.

hero78119 · 2023-06-19T15:05:23Z

Updated pr description to compare main/word-lo-hi cell utilization via command cargo run --bin stats --features stats -- exec

hero78119 · 2023-06-19T15:16:23Z

    }

-    pub(crate) fn reversion_info_write(
+    pub(crate) fn reversion_info_write_unchecked(


Discussion: I noticed in begin_tx.rs and ReversionInfo main branch there is no range check, does it safe?

ed255 · 2023-06-20T08:08:57Z

        let address = block.get_rws(step, 0).stack_value();
-        self.address_word
-            .assign(region, offset, Some(address.to_le_bytes()))?;
+        self.address.assign_u256(region, offset, address)?;


Shouldn't this be assign_h160(..., address.to_address())?

assign_u256 is correct because here address already in u256 little endian 20 bytes.
of course assign_h160(..., address.to_address()) is also equivalent, just we will convert to big endian then convert back to little endian immediately. I think this is just coding style, and assign_u256 looks still ok and efficiently save some conversion, so i prefer to keep assign_u256

ed255 · 2023-06-20T08:51:27Z

+            quotient
+                .to_word()
+                .mul_selector(is_shl.expr())
+                .add_unchecked(dividend.to_word().mul_selector(is_shr.expr())),


I think we could find a better way to write these kind of expressions (I also saw them in another opcode).

So originally we had:

v = is_shl.expr() * dividend.expr() + is_shr.expr() * quotient.expr()) * (1.expr() - divisor_is_zero.expr()

The logic for this is the following:

v = if (not (divisor_is_zero) { if (is_shl) { dividend } else if (is_shr) { quotient } else { 0 } } else { 0 };

So maybe we could have some helpers to write these if/else where the conditions are booleans and the values are words. But let's leave this for a future PR! I just wanted to share my thoughts here :)

Yes, agree current unchecked style looks very ugly and hard to read 🥲
It will be good to left it as a future PR and check whether can leverage macro! features to make it looks confortable!

hero78119 · 2023-06-20T10:18:46Z

This is a big PR and while reviewing I think I can categorize my comments into:

a. Something that I think is wrong

b. Something that I think should be improved at API level / code style

c. Something that I think should be improved for optimization (cell usage or constraints)

d. Improvements that I see that are unrelated to this refactor. I try not to focus on these to avoid noise!

I think the most important one to resolve is a. If it's an over-constraint, we should be able to detect them also via the unit tests. If it's an under constraint, that's more difficult to detect other than via review.

For b and c I think we could defer to resolve some of the discussions to a future PR if @hero78119 considers it (on a case by case basis). This way we can get this PR merged sooner, and work on b and c after the unit tests are passing, so that we can apply changes more confidently.

@ed255 I create another issue #1487 for recording future optimisation and housekeeping related to word-lo-hi, if you thought any related to b, c, d and can be addressed in the future :)

ed255 · 2023-06-20T11:52:08Z

-    pub fn expr_unchecked(&self) -> Expression<F> {
-        self.lo() + self.hi() * (1 << (N_BYTES_HALF_WORD * 8)).expr()
+    /// No overflow check on lo/hi limbs
+    pub fn mul_unchecked(self, rhs: Self) -> Self {


For a future PR I think we should try to remove add_unchecked, sub_unchecked and mul_unchecked and rewrite the gadgets that use these functionalities in a more clear way.

ed255

I've finished reviewing all the files, phew!

There are still comments from me marked as unresolved but I think we can discuss them / figure them out in a future PR. The only comment that I would like to really discuss before merging the PR is this one #1411 (comment)

Aside from that everything looks good to me! This was a massive work! Awesome job @hero78119 !!! 🎊

I'm approving the PR already so that you can merge when you see fit!

hero78119 · 2023-06-20T12:48:07Z

Appreciate @ChihChengLiang and @ed255 great effort on reviewing 🔥 👏 🙏 Will merge it sooner :)

github-actions Bot added crate-keccak Issues related to the keccak workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels May 16, 2023

hero78119 marked this pull request as draft May 16, 2023 07:49

hero78119 changed the title ~~evm circuit word-lo-hi~~ WIP evm circuit word-lo-hi May 16, 2023

hero78119 force-pushed the feat/evm_circuit_word_lo_hi branch 4 times, most recently from 974c04e to 7be1ccd Compare May 17, 2023 06:26

github-actions Bot removed the crate-keccak Issues related to the keccak workspace member label May 17, 2023

hero78119 force-pushed the feat/evm_circuit_word_lo_hi branch 3 times, most recently from a8f84d8 to a601c89 Compare May 17, 2023 16:22

hero78119 changed the title ~~WIP evm circuit word-lo-hi~~ WIP [word-lo-hi] evm circuit May 17, 2023

hero78119 force-pushed the feat/evm_circuit_word_lo_hi branch 3 times, most recently from 56c13ed to a18b8d6 Compare May 19, 2023 14:01

hero78119 mentioned this pull request May 21, 2023

EVM Circuit: Refactor word_rlc into word lo/hi #1379

Closed

5 tasks

hero78119 changed the title ~~WIP [word-lo-hi] evm circuit~~ [word-lo-hi] evm circuit May 21, 2023

hero78119 marked this pull request as ready for review May 21, 2023 03:00

hero78119 force-pushed the feat/evm_circuit_word_lo_hi branch from a18b8d6 to 9083bf5 Compare May 22, 2023 16:30

hero78119 mentioned this pull request May 23, 2023

Write Word utilities, types and abstractions for word lo/hi #1388

Closed

hero78119 force-pushed the feat/evm_circuit_word_lo_hi branch from 9083bf5 to ec8f365 Compare May 23, 2023 08:19

hero78119 closed this May 26, 2023

hero78119 deleted the feat/evm_circuit_word_lo_hi branch May 26, 2023 03:38

hero78119 restored the feat/evm_circuit_word_lo_hi branch May 26, 2023 08:49

hero78119 reopened this May 26, 2023

ed255 linked an issue Jun 1, 2023 that may be closed by this pull request

EVM Circuit: Refactor word_rlc into word lo/hi #1379

Closed

5 tasks

ChihChengLiang self-requested a review June 19, 2023 12:45

hero78119 added 3 commits June 19, 2023 20:52

chore: fix account_access_list to less verbose

03a9218

formalized api uncheck suffix

498c502

clean up blockctx variants

7b1dc94

ChihChengLiang approved these changes Jun 19, 2023

View reviewed changes

Comment thread zkevm-circuits/src/evm_circuit/execution/exp.rs

Comment thread zkevm-circuits/src/evm_circuit/execution/error_invalid_jump.rs Outdated

Comment thread zkevm-circuits/src/evm_circuit/execution/callop.rs Outdated

hero78119 force-pushed the feat/evm_circuit_word_lo_hi branch from 3574d57 to 1a6f6a9 Compare June 19, 2023 14:38

optimise gadget cell usage

6bc88fe

hero78119 force-pushed the feat/evm_circuit_word_lo_hi branch from 1a6f6a9 to 6bc88fe Compare June 19, 2023 14:46

hero78119 commented Jun 19, 2023

View reviewed changes

hero78119 force-pushed the feat/evm_circuit_word_lo_hi branch from 9ad99e7 to 6d82c49 Compare June 20, 2023 04:53

add int decomposition type

87370f8

hero78119 force-pushed the feat/evm_circuit_word_lo_hi branch from 6d82c49 to 87370f8 Compare June 20, 2023 04:57

hero78119 added 3 commits June 20, 2023 13:07

optimise txid

43b03c8

add new file

9d4d088

chores: add int decomposition api

5095468

ed255 reviewed Jun 20, 2023

View reviewed changes

chores: housekeeping and address review feedback

b81a860

hero78119 mentioned this pull request Jun 20, 2023

[word lo/hi] optimisation and improvements. #1487

Closed

10 tasks

ed255 reviewed Jun 20, 2023

View reviewed changes

ed255 approved these changes Jun 20, 2023

View reviewed changes

chores: code cosmetics and assertion

ad221d8

hero78119 force-pushed the feat/evm_circuit_word_lo_hi branch from f245af8 to ad221d8 Compare June 20, 2023 12:58

hero78119 merged commit 0a0d712 into privacy-ethereum:word-lo-hi Jun 20, 2023

ChihChengLiang mentioned this pull request Jun 20, 2023

Bytecode Circuit: Refactor word_rlc into word lo/hi #1468

Merged

2 tasks

Conversation

hero78119 commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue Link

Type of change

Contents

Cell utilization on main branch vs on word-lo-hi branch

Storage_1

Storage_2

Byte_lookup

Uh oh!

hero78119 commented May 17, 2023

Uh oh!

hero78119 commented May 23, 2023

Uh oh!

hero78119 commented May 23, 2023

Uh oh!

ed255 commented Jun 15, 2023

Uh oh!

ChihChengLiang commented Jun 16, 2023

Uh oh!

hero78119 commented Jun 16, 2023

Uh oh!

ChihChengLiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hero78119 commented Jun 19, 2023

Uh oh!

hero78119 Jun 19, 2023

Choose a reason for hiding this comment

Uh oh!

ed255 Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

hero78119 Jun 20, 2023 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ed255 Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

hero78119 Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

hero78119 commented Jun 20, 2023

Uh oh!

Uh oh!

Uh oh!

ed255 Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

ed255 left a comment

Choose a reason for hiding this comment

Uh oh!

hero78119 commented Jun 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hero78119 commented May 16, 2023 •

edited

Loading

hero78119 Jun 20, 2023 •

edited

Loading