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

Copy Circuit#584

Merged
roynalnaruto merged 55 commits into
privacy-ethereum:mainfrom
scroll-tech:feat/copy-circuit
Jul 18, 2022
Merged

Copy Circuit#584
roynalnaruto merged 55 commits into
privacy-ethereum:mainfrom
scroll-tech:feat/copy-circuit

Conversation

@roynalnaruto
Copy link
Copy Markdown
Collaborator

@roynalnaruto roynalnaruto commented Jun 20, 2022 •
edited
Loading

Implement Copy Table:

@roynalnaruto roynalnaruto changed the title Feat/copy circuit Copy Circuit Jun 20, 2022
@github-actions github-actions Bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Jun 20, 2022
@CPerezz CPerezz mentioned this pull request Jun 21, 2022
Merged
@github-actions github-actions Bot added the crate-eth-types Issues related to the eth-types workspace member label Jun 22, 2022
@github-actions github-actions Bot added the crate-gadgets Issues related to the gadgets workspace member label Jun 22, 2022
@icemelon icemelon force-pushed the feat/copy-circuit branch from 0faf177 to 4e1296e Compare July 2, 2022 06:26
@roynalnaruto roynalnaruto mentioned this pull request Jul 8, 2022
Closed
lispc
lispc reviewed Jul 8, 2022
View reviewed changes
Comment thread gadgets/src/is_zero.rs
lispc
lispc reviewed Jul 8, 2022
View reviewed changes
Comment thread zkevm-circuits/src/evm_circuit/step.rs Outdated
@roynalnaruto roynalnaruto mentioned this pull request Jul 10, 2022
Merged
5 tasks
@roynalnaruto roynalnaruto marked this pull request as ready for review July 12, 2022 02:46
@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Jul 12, 2022

@roynalnaruto Should I wait for #611 to be merged before reviewing this?

@roynalnaruto
Copy link
Copy Markdown
Collaborator Author

roynalnaruto commented Jul 12, 2022

@ed255 yes after the utils are refactored into gadgets, this PR still needs to be rebased. So may be you can wait.

Although apart from the refactoring into the gadgets crate, the rest of the additios/changes are ready. The relevant opcode execution tests are passing using the Copy Table.

@roynalnaruto
Copy link
Copy Markdown
Collaborator Author

roynalnaruto commented Jul 12, 2022

@ed255 now that #611 is merged and I've rebased this PR, its ready to be reviewed :)

ed255
ed255 reviewed Jul 13, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@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 review round. I've reviewed all files except for zkevm-circuits/src/copy_table.rs.

Overall looks really good and clean! I've left many comments but most of them are small things or questions :)

Comment thread bus-mapping/src/evm/opcodes/calldatacopy.rs
Comment thread bus-mapping/src/evm/opcodes/calldatacopy.rs
Comment thread bus-mapping/src/evm/opcodes/logs.rs Outdated
Comment thread eth-types/src/bytecode.rs Outdated
Comment thread zkevm-circuits/src/evm_circuit/table.rs Outdated
Comment thread zkevm-circuits/src/copy_table.rs Outdated
Comment thread zkevm-circuits/src/copy_table.rs Outdated
Comment thread zkevm-circuits/src/evm_circuit.rs Outdated
Comment thread zkevm-circuits/src/copy_table.rs
Comment thread zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs Outdated
@github-actions github-actions Bot added crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member T-bench Type: benchmark improvements labels Jul 14, 2022
ed255
ed255 reviewed Jul 15, 2022
View reviewed changes
Comment thread zkevm-circuits/src/evm_circuit/witness.rs Outdated
ed255
ed255 reviewed Jul 15, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

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

I've just finished reviewing zkevm-circuits/src/copy_circuit.rs. Everything looks good! I've added a comment but it's a small thing.
The circuit implementation looks really clean and it was very easy to follow along the spec! Great work!

Comment thread zkevm-circuits/src/copy_circuit.rs Outdated
@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Jul 15, 2022

A part from my last two minor comments, I think it would be helpful to have a stand alone unit test for the Copy Circuit as mentioned in another comment; I'll mark the PR as approved from my side after that :)

roynalnaruto and others added 20 commits July 18, 2022 11:57
@roynalnaruto
fix: codecopy tests passing (code hash is LE bytes)
5c47c2d
@roynalnaruto
fix(copy-table): addr assignment for tx log
238c2a7
@roynalnaruto
fix: copy events by pc handles multiple copy events in a block
cdabd99
@icemelon @roynalnaruto
fix opcode for log
4ef7327
@roynalnaruto
feat: use binary number chip instead of iszerochip for tag constraining
ee7ec93
@roynalnaruto
fix: compilation after rebasing
dcb41ca
@roynalnaruto
chore: comments on the iszero gadget
955f01d
@roynalnaruto
chore: refactor (1 - x) to not(x)
a9339c3
@roynalnaruto
chore: rebase and fix compilation
59857ad
@roynalnaruto
chore: docs and renaming -> copy circuit
5f2ab75
@roynalnaruto @ed255
Update zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs
7d43f7a 
Co-authored-by: Eduard S. <eduardsanou@posteo.net>
@roynalnaruto
chore: remove duplication of code() and to_vec()
36fab91
@roynalnaruto
fix: resolve collision possibility in hashmap for copy events
5936392
@roynalnaruto
fix: separate out constraints of copy circuit from evm circuit
5312567
@roynalnaruto
chore: describe separate fields in the copy lookup instead of one vec
51d01bd
@roynalnaruto
chore(bus-mapping): refactor copy steps generation in logs
4f56ab5
@roynalnaruto
tests(copy_circuit): valid and invalid values for calldatacopy/codecopy
a6ace56
@roynalnaruto
refactor: use tuple instead of u256 for simplicity as key
d8a897b
@roynalnaruto
fix: move constraint to correct condition
d00ca2e
@roynalnaruto
refactor(tests): remove code duplication, perturb different possible …
e50d8ed 
…fields
ed255
ed255 approved these changes Jul 18, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

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

LGTM! Thanks for addressing my comments. Great work!

@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Jul 18, 2022

@ed255 resolved all comments and added a few tests for the copy circuit. We can even improve the tests by perturbing fields other than value (but I can add that in a separate PR? Because there are already a lot of changes here, and since copy circuit is a blocker for the RETURN opcode impl)?

I agree that improving the tests can be left for a future PR. I've just approved :)

@lispc considering that you've already taken a look at this PR, could you check it again and approve it if you find it ok? (so that we reach 2 approvals)

@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Jul 18, 2022

@roynalnaruto feel free to merge this PR whenever you want :)

@roynalnaruto roynalnaruto merged commit b375530 into privacy-ethereum:main Jul 18, 2022
@roynalnaruto roynalnaruto deleted the feat/copy-circuit branch July 18, 2022 14:37
lispc added a commit that referenced this pull request Aug 28, 2023
@kunxian-xia @roynalnaruto @lispc
Fix the bugs in RLP/Tx/PI circuit which are reported by Zellic & KALO…
2e42287 
…S auditors (#572)

* fix finding 3 (#575)

* Fix zellic finding 4 (#576)

* fix finding 3 (#575)

* fix finding 4

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* add range check on diffs (#586)

* Fix finding 10 (#578)

* fix finding 3 (#575)

* fix finding 10

* Fix finding 13 (#579)

* fix finding 3 (#575)

* fix finding 13

* Fix zellic finding 14 (#580)

* fix finding 3 (#575)

* fix finding 14

* Fix zellic finding 5 (#584)

* fix finding 3 (#575)

* fix finding 5

* refine comments

* fmt

* Fix finding 17 (#602)

* add q_last

* fix

* add more diff range check

* fix finding 7 (#625)

* tx_id = 1 when sm starts

* Fix finding 11 : use length for rlc in rlp table (#719)

* fix: use tag_bytes_rlc and tag_length to copy tag's bytes around

* fix lookup input for Len & RLC & GasCost fields in tx circuit

* refactor

* fix

* refactor

* fix col phase issue

* refactor bytes_rlc type

* Fix the bugs in Tx & PI circuits reported by Zellic & KALOS auditors (#612)

* lookup chain_id to RLP table

* fix finding 22 (#614)

* fix finding 21 (#613)

* fix finding 23 (#618)

* fix finding 26 (#622)

* fix finding 28 (#624)

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* fix finding 29 (#623)

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* enforce is_final is true at the last row and fix RLC related vul (#735)

* Fix finding 30  (#733)

* enforce all txs in a block are included in the tx table

* clippy

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* Fix Zellic / Kalos finding25 (#619)

* fix finding 25

* add comment

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* fix conflicts

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>

* use q_first instead

* fmt

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
lispc added a commit that referenced this pull request Aug 28, 2023
@darth-cy @lispc
@kunxian-xia @roynalnaruto
refactor: remove keccak constants from aggregator (#752)
1d190ba 
* add row counting interface for keccak

* add class level capacity calculator for keccak

* remove f capacity from core

* remove capacity calculator in aggregator util

* remove unnecessary imports

* replace max keccak round in core

* replace reference for max keccak

* remove unnecessary keccak imports and constants

* remove max keccak constant

* remove constants in hash cell parsing

* remove constant column sanity check

* add state column usage log

* adjust input bytes column

* add long column padding

* correct fmt

* fix fmt

* minor fixes

* fix

* Fix: allow skipping of L1Msg tx part 2 (calculate num_all_txs in tx circuit) (#778)

* calculate num_l1_msgs and num_l2_txs in tx circuit

* fix

* fmt and clippy

* fix: non-last tx requires next is calldata

* add NumAllTxs in block table and copy it from pi to block table

* add lookup for NumAllTxs in tx circuit

* clippy

* add block num diff check to avoid two real block have same num

* clippy

* address comments

* Fix the bugs in RLP/Tx/PI circuit which are reported by Zellic & KALOS auditors (#572)

* fix finding 3 (#575)

* Fix zellic finding 4 (#576)

* fix finding 3 (#575)

* fix finding 4

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* add range check on diffs (#586)

* Fix finding 10 (#578)

* fix finding 3 (#575)

* fix finding 10

* Fix finding 13 (#579)

* fix finding 3 (#575)

* fix finding 13

* Fix zellic finding 14 (#580)

* fix finding 3 (#575)

* fix finding 14

* Fix zellic finding 5 (#584)

* fix finding 3 (#575)

* fix finding 5

* refine comments

* fmt

* Fix finding 17 (#602)

* add q_last

* fix

* add more diff range check

* fix finding 7 (#625)

* tx_id = 1 when sm starts

* Fix finding 11 : use length for rlc in rlp table (#719)

* fix: use tag_bytes_rlc and tag_length to copy tag's bytes around

* fix lookup input for Len & RLC & GasCost fields in tx circuit

* refactor

* fix

* refactor

* fix col phase issue

* refactor bytes_rlc type

* Fix the bugs in Tx & PI circuits reported by Zellic & KALOS auditors (#612)

* lookup chain_id to RLP table

* fix finding 22 (#614)

* fix finding 21 (#613)

* fix finding 23 (#618)

* fix finding 26 (#622)

* fix finding 28 (#624)

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* fix finding 29 (#623)

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* enforce is_final is true at the last row and fix RLC related vul (#735)

* Fix finding 30  (#733)

* enforce all txs in a block are included in the tx table

* clippy

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* Fix Zellic / Kalos finding25 (#619)

* fix finding 25

* add comment

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* fix conflicts

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>

* use q_first instead

* fmt

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>

* add pi comments

* rename preimage col idx

* add keccak rows check

* rename input bytes col finder fn

* modify keccak row env constaint

* modify keccak row env constaint

* add named constant setup vars

* modify keccak row check

* clippy advised

* add comments on chunk hash

* fmt

* avoid constant lookup table

* avoid repetitive computation of input_bytes_col_idx

---------

Co-authored-by: Zhuo Zhang <mycinbrin@gmail.com>
Co-authored-by: xkx <xiakunxian130@gmail.com>
Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@miha-stopar miha-stopar Awaiting requested review from miha-stopar

@CPerezz CPerezz Awaiting requested review from CPerezz

@icemelon icemelon Awaiting requested review from icemelon

2 more reviewers

@lispc lispc lispc approved these changes

@ed255 ed255 ed255 approved these changes

Reviewers whose approvals may not affect merge requirements

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-eth-types Issues related to the eth-types workspace member crate-gadgets Issues related to the gadgets workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member T-bench Type: benchmark improvements

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@roynalnaruto @ed255 @lispc @icemelon