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

Merge structs between CIB and evm circuit witness #1391

Closed
3 of 5 tasks
ChihChengLiang opened this issue May 4, 2023 · 1 comment · Fixed by #1457
Closed
3 of 5 tasks

Merge structs between CIB and evm circuit witness #1391

ChihChengLiang opened this issue May 4, 2023 · 1 comment · Fixed by #1457
Labels
T-feature Type: new features T-tech-debt Type: tech-debt generated or cleaned up

Comments

@ChihChengLiang
Copy link
Collaborator

ChihChengLiang commented May 4, 2023 •
edited
Loading

Describe the feature you would like

This is a continuation of unfinished tasks in #528

We explore more merging opportunities between the structs in the circuit input builder(CIB) and the evm circuit witness.

  • Bytecode. The witness has one more field, the bytecode hash. If we add the bytecode hash in CIB's Bytecode struct, then we can safely remove the struct in witness.rs. WIP
  • Block. witness::Block and witness::BlockContext have many overlapping fields with CIB::Block. We can create better structs in CIB and remove some of the witness's block structs.
  • ExecStep. Many duplicated fields between witness::ExecStep and CIB::ExecStep, which suggests possible struct merging.
  • Tx. Further deduplication seems still possible. In review Remove witness tx struct #1457
  • Call

Additional context

No response
@ChihChengLiang ChihChengLiang added the T-feature Type: new features label May 4, 2023
@ChihChengLiang ChihChengLiang added this to the tech-debt-1 milestone May 4, 2023
This was referenced May 4, 2023
Closed
Merged
ChihChengLiang added a commit that referenced this issue May 10, 2023
@ChihChengLiang
Simplify bytecode circuit assignment (#1397)
fbbe1bd 
### Description

We simplify the assignment logic in the bytecode circuit by reducing the
number of function arguments. The reduction is achieved by

1. Construct IsZero Chips early and make them a part of the circuit
struct so that they don't stay with the function arguments.
2. Create a new row struct that carries all the relevant data.

### Issue Link

This would be part of #1391. 

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

### Rationale

- I avoid using `BytecodeRow` in the `bytecode_unroller` since they are
part of the future simplification plan.


### How Has This Been Tested?

Sent to the CI and pray.
@ChihChengLiang ChihChengLiang mentioned this issue May 11, 2023
Merged
4 tasks
ChihChengLiang added a commit that referenced this issue May 12, 2023
@ChihChengLiang @ed255
Replace Witness.rs's ExecStep with CIP's (#1403)
0e1d230 
### Description

- We replace ExecStep struct in witness.rs with the one in the Circuit
Input Builder.
- We add some convenient methods in CIP's ExecStep so that we don't
create extra struct fields.
- Add `get_rws` method to `witness::Block` to streamline the logic of
getting Read-Write records.

### Issue Link

Part of #1391

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

### Rationale

- Notes for reviewers, one place that might be error-prone would be the
rw_indices. After the cleanup in this PR, we can replace the manual
counting of rw indices with something like
`evm_circuit::utils::StepRws`.

---------

Co-authored-by: Eduard S. <eduardsanou@posteo.net>
@ChihChengLiang ChihChengLiang mentioned this issue May 12, 2023
Merged
4 tasks
ChihChengLiang added a commit that referenced this issue May 17, 2023
@ChihChengLiang
Remove RwTableTag (#1406)
69cb8a9 
### Description

`RwTableTag` can be fully replaced by `Target`.

### Issue Link

Part of #1391 

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

We add EnumIter, Hash, Expr to Target to let it serve different needs.

Note to reviewers: the `impl From<Target> for usize` is required by
BinaryNumberConfig.
This was referenced May 19, 2023
Closed
Closed
@ChihChengLiang ChihChengLiang mentioned this issue Jun 5, 2023
Merged
@ed255 ed255 linked a pull request Jun 15, 2023 that will close this issue
Remove witness tx struct #1457
Merged
@ed255
Copy link
Member

ed255 commented Jun 23, 2023

A bit related to this issue: #1451

@ed255 ed255 added the T-tech-debt Type: tech-debt generated or cleaned up label Jul 3, 2023
@ed255 ed255 removed this from the tech-debt-1 milestone Jul 3, 2023
github-merge-queue bot pushed a commit that referenced this issue Jul 5, 2023
@ChihChengLiang
Remove witness tx struct (#1457)
dd1e1c4 
### Description

We remove the witness tx struct.

We can wait for the lo-hi refactor ready then for this PR.

### Issue Link

Part of #1391

### Type of change

New feature (non-breaking change which adds functionality)

### Contents

- Remove zkevm-circuits/src/witness/tx.rs entirely
- Remove eth_block field from block
- Acquire txID ASAP so we don't need a Optional<tx_id>
@ChihChengLiang ChihChengLiang mentioned this issue Jul 13, 2023
Merged
github-merge-queue bot pushed a commit that referenced this issue Jul 24, 2023
@ChihChengLiang
Bytecode, and a collection of bytecodes. (#1529)
8a633f7 
### Description

We revisit the handling of the bytecodes in the codebase. We reduce
manual manipulation of bytes and use a more streamlined API with
eth_types::Bytecode and bus_mapping::CodeDB.

### Issue Link

Missed out work in #1391 

### Type of change

Refactor

### Contents

- Removed witness::bytecode module.
- Extend eth_types::Bytecode and bus_mapping::CodeDB.
  - eth_types::Bytecode handles a single bytecode instance, and
  - CodeDB handles a group of bytecode instances
- rewrite the bytecode circuit witness assignment.
- rewrite the bytecode circuit tests, so that overwriting will not be
included in the production code.
- deduplicated the Mock account testing code.

### Rationale

- I kept the scope of the change small to reduce the review burden.
- I will add more rationale with review comments.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
T-feature Type: new features T-tech-debt Type: tech-debt generated or cleaned up
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove witness tx struct privacy-scaling-explorations/zkevm-circuits
2 participants
@ChihChengLiang @ed255