Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merge matching structs between circuit input builder and evm circuit witness (where possible) #528

Closed
ed255 opened this issue May 24, 2022 · 4 comments · Fixed by #1378
Closed

Merge matching structs between circuit input builder and evm circuit witness (where possible) #528

ed255 opened this issue May 24, 2022 · 4 comments · Fixed by #1378
Assignees
@ChihChengLiang
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member T-refactor Type: cleanup/refactor T-tech-debt Type: tech-debt generated or cleaned up
Milestone
tech-debt-1

Comments

@ed255
Copy link
Member

ed255 commented May 24, 2022 •
edited by CPerezz

The idea of having duplicate structs between circuit input builder and evm witness generation was to allow the development of the circuit input builder in parallel with the circuits while minimizing the changes that one implies to the other. But once things stabilize, I think we can merge those.

Related to #484 (comment)

See the latest discussions upon that:

Unify the types Block, Call, Step, Transaction between these two:

  • zkevm-circuits/src/witness
  • bus-mapping/src/circuit_input_builder

Also see:

Remove duplicated fields in eth_block -> https://github.com/appliedzkp/zkevm-circuits/blob/feat%2Fannotate_evm_cols/bus-mapping/src/circuit_input_builder/block.rs#L54-L57
(Work on refactoring and have just 2 blocks, 1 for RPC and the other for general purpose)
@ed255 ed255 mentioned this issue May 24, 2022
Merged
2 tasks
@DreamWuGit
Copy link
Collaborator

Really good idea to reduce redundant structs !!!

@ed255 ed255 added crate-bus-mapping Issues related to the bus-mapping workspace member T-refactor Type: cleanup/refactor crate-zkevm-circuits Issues related to the zkevm-circuits workspace member T-tech-debt Type: tech-debt generated or cleaned up labels May 25, 2022
@ed255 ed255 mentioned this issue Oct 4, 2022
Merged
1 task
@ed255 ed255 added this to the tech-debt-1 milestone Feb 16, 2023
@KimiWu123 KimiWu123 self-assigned this Apr 6, 2023
@KimiWu123 KimiWu123 removed their assignment Apr 20, 2023
@ChihChengLiang
Copy link
Collaborator

I can take this one.

@ChihChengLiang ChihChengLiang self-assigned this May 1, 2023
@ChihChengLiang ChihChengLiang mentioned this issue May 1, 2023
Merged
4 tasks
@ChihChengLiang ChihChengLiang linked a pull request May 1, 2023 that will close this issue
Prune tx struct fields and rm witness Call #1378
Merged
4 tasks
ChihChengLiang added a commit that referenced this issue May 4, 2023
@ChihChengLiang
Prune tx struct fields and rm witness Call (#1378)
68e3d98 
### Description

We tried to remove some unused tx structs.

### Issue Link

#528 

### Type of change

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] This change requires a documentation update

### Contents

- Remove zkevm-circuits/src/witness/call.rs and the `Call` struct
- Remove fields in circuit_input_builder Transaction struct. Reuse Geth
type Transaction struct whenever possible
- Remove duplicated call data gas cost computation
- Add APIs to get "To" address so that we know when we want contract
address or zero address.

### Rationale

We try to start the de-duplication from the low-hanging fruits. So I
didn't remove the witness.rs Transaction struct in this PR.

### How Has This Been Tested?

Local build tests and quick tests
@ed255
Copy link
Member Author

ed255 commented May 4, 2023

There are still some structs that can be de-duplicated after the PR #1378 @ChihChengLiang will open a new issue to track those.

@ChihChengLiang ChihChengLiang mentioned this issue May 4, 2023
Closed
5 tasks
@ChihChengLiang
Copy link
Collaborator

Created #1391 to continue the work.

jonathanpwang pushed a commit to axiom-crypto/zkevm-circuits that referenced this issue Aug 1, 2023
@kunxian-xia @roynalnaruto
feat: refactor rlp circuit and tx circuit to support more tx types (p…
79caaf4 
…rivacy-scaling-explorations#528)

* wip

* wip 22

* wip 3

* wip 4

* wip 5

* wip 6

* wip 7

* wip 8

* wip 9

* wip 10

* wip 11

* wip 12

* wip 13

* wip 14

* wip 15

* wip 16

* wip 17

* wip 18

* wip 19

* add macros

* addressing some comments

* rewrite constraint (part 1)

* reduce condition degree

* rewrite constraint (part 2)

* rewrite constraint (part 3)

* rewrite constraint (part 4)

* rewrite constraint (part 5)

* add docs

* docs

* data table assignments

* add witness gen part 1

* add more fields for supporting pre-eip155 tx

* add witness gen part 2

* add witness gen part 3

* finish gen_sm_witness

* pass eip1559 rlp witness test

* refactor

* add pre-eip155 test case

* add unit test in rlp_circuit_fsm

* pass pre-eip155 tx unit test in rlp circuit

* fix constraint errors part 1

* fix witness gen bug

* update unit test

* fix constraint errors (part 2)

* add eip155 tx test

* make witness gen more precise

* update data table checks

* fix clippy errors

* fix clippy

* fix clippy

* fmt

* update sm checks

* reduce degree to 9

* add eip1559 test and add constraints on DecodeTagStart -> End

* finish padding

* move away RLP internal tables from table.rs

* remove old rlp circuit

* re-organize tests in rlp_circuit_fsm into its own dir

* clean

* clean

* disable tx circuit

* fmt

* clippy

* fix typo

* fix a bug in keccak circuit's min_num_rows_block

* move q_enable to RlpTable

* finish rom of l1_msg_hash

* add sm init checks

* remove old RlpTable

* fix typos and refine comments

* bug-fix: use max_length to select b to accumulate bytes' value

* update tx_data_gas_cost calc rule

* clean

* ignore tx_l1_fee unit tests in evm_circuit

* refactor tx_circuit

* add empty row

* turn on the tx_circuit

* fix clippy errors

* fmt

* fix

* assign padding_tx in rlp circuit

* clean

* pass pre-eip155 tx test

* clippy and add constraints on sig.v

* ignore null signature

* add ChainID

* dep: use scroll tech's fork of ethers-rs

* refactor

* add l1 msg tx test in rlp circuit

* pass l1 msg unit test in tx circuit

* finish

* update Cargo.lock

* fix

* refactor: move is_zero to util/

* add doc

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ChihChengLiang ChihChengLiang

Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member T-refactor Type: cleanup/refactor T-tech-debt Type: tech-debt generated or cleaned up
Projects
zkEVM Community Edition
Status: ✅ Done
Milestone
tech-debt-1
Development

Successfully merging a pull request may close this issue.

Prune tx struct fields and rm witness Call privacy-scaling-explorations/zkevm-circuits
4 participants
@ChihChengLiang @DreamWuGit @KimiWu123 @ed255