Copy Circuit by icemelon · Pull Request #194 · privacy-ethereum/zkevm-specs

icemelon · 2022-05-07T00:28:58Z

This PR brings the copy circuit that eliminates the multi-slot opcodes such as CALLDATACOPY, CODECOPY, and LOG.

roynalnaruto · 2022-06-14T20:08:36Z

@icemelon You will need to edit this PR from "draft" to "open"

ed255

Overall looks very good!
I've added a few notes here and there but they are mainly topics for discussion and some small suggestions.

ed255 · 2022-06-17T15:01:51Z

+
+    def __init__(self) -> None:
+        self.rows = []
+        self.pad_rows = [CopyCircuitRow(*[FQ(0)] * 18)] * 2


This is an example of how padding rows work right? Not padding in the sense that a 0 is read after the src end; but in the sense that the circuit has a fixed number of rows and maybe not all of them are used.

If that's the case, could you add a line in the markdown doc saying that padding rows are set to all 0s, and that these values always pass the constraints?

roynalnaruto · 2022-06-17T15:47:12Z

Overall looks very good!

I've added a few notes here and there but they are mainly topics for discussion and some small suggestions.

Thanks for the suggestions. They all make sense and I'll update the PR soon :)

DreamWuGit · 2022-06-23T02:44:27Z

+        # bytes_left == 1 for last step
+        cs.constrain_zero(rows[1].is_last * (1 - rows[0].bytes_left))
+        # bytes_left == bytes_left_next + 1 for non-last step
+        cs.constrain_zero((1 - rows[1].is_last) * (rows[0].bytes_left - rows[2].bytes_left - 1))


not sure why use rows[2].bytes_left , refer to last step second row?

If rows[0] is the read step (given that condition rows[0].q_step) then we want to constrain:
rows[0].bytes_left == rows[2].bytes_left + 1 as rows[2] is the next read step if rows[1] was not the last step.

DreamWuGit · 2022-06-23T03:31:44Z

just have a quick look ,one point to discuss: seems to me maybe there is possible duplicated constraint between copy circuit and state circuit. for example, read value constrain to be equal to writing value at the same address. in the state circuit should constrain this (even though not implemented yet sort of) by design, but it is not big problem from my side.

roynalnaruto

Small question WRT some removed checks

ed255 · 2022-06-23T12:58:00Z

just have a quick look ,one point to discuss: seems to me maybe there is possible duplicated constraint between copy circuit and state circuit. for example, read value constrain to be equal to writing value at the same address. in the state circuit should constrain this (even though not implemented yet sort of) by design, but it is not big problem from my side.

Are you referring to the constraint that the value in the write row is the same as the value in the read row (of the same copy step)? If that's the case, I don't see how it's redundant as the CopyCircuit is copying between two different places. Could you elaborate on this duplicate constraint between Copy Circuit and State Circuit; and point to the exact constraint in the python spec?

Thanks!

ed255 · 2022-06-27T14:09:26Z

+    src_id: FQ
+    src_type: FQ
+    dst_id: FQ
+    dst_type: FQ
+    src_addr: FQ
+    src_addr_end: FQ
+    dst_addr: FQ
+    length: FQ
+    rw_counter: FQ
+    rwc_inc: FQ
+    log_id: FQ = field(default=FQ(0))


Suggested change

src_id: FQ

src_type: FQ

dst_id: FQ

dst_type: FQ

src_addr: FQ

src_addr_end: FQ

dst_addr: FQ

length: FQ

rw_counter: FQ

rwc_inc: FQ

log_id: FQ = field(default=FQ(0))

src_id: FQ # Copy source type-specific id. The id for each type is: {Bytecode: code_hash, Memory: call_id, TxCallData: tx_id, TxLog: tx_id}

src_type: FQ # Copy source type from `CopyDataTypeTag`

dst_id: FQ # Copy destination type-specific id

dst_type: FQ # Copy destination type from `CopyDataTypeTag`

src_addr: FQ # Copy source start address / index

src_addr_end: FQ # Size of the Copy source. When source address equals this value or higher, the read value is `0`.

dst_addr: FQ # Copy destination start address / index

length: FQ # Copy length: number of bytes written into destination

rw_counter: FQ # Rw counter value at the beginning of the copy

rwc_inc: FQ # How much will the rw_counter increase after the copy is complete

log_id: FQ = field(default=FQ(0))

Field description suggestions.

NOTE: I later realized that this fields are properly described in tables.md, so maybe these comments are redundant.

ed255

LGTM! Great work!

DreamWuGit · 2022-06-30T13:18:31Z

Are you referring to the constraint that the value in the write row is the same as the value in the read row (of the same copy step)? If that's the case, I don't see how it's redundant as the CopyCircuit is copying between two different places. Could you elaborate on this duplicate constraint between Copy Circuit and State Circuit; and point to the exact constraint in the python spec?

I check current state circuit don't constrain value read/write consistency for now, so it not problem !

DreamWuGit · 2022-06-30T13:22:41Z

+- **is_last**: a boolean value to indicate the last row in a copy event.
+- **ID**: could be `$txID`, `$callID`, `$codeHash` (RLC encoded).
+- **Type**: indicates the type of data source, including `Memory`, `Bytecode`, `TxCalldata`, `TxLog`.
+- **Address**: indicates the address in the source data, could be memory address, byte index in the bytecode, tx call data, and tx log data. When the data type is `TxLog`, the address is the combination of byte index, `TxLogFieldTag.Data` tag, and `LogID`.


after #214 merged, Address for TxLog change to log_id + field_tag + index now. maybe need update accordingly . thanks !

Yes, I already updated the txlog lookup in the latest commit. could you help take a look?

ed255 · 2022-06-30T13:55:44Z

I check current state circuit don't constrain value read/write consistency for now, so it not problem !

But that's just because it's a TODO rigth? The spec has de read consistency constraint: https://github.com/privacy-scaling-explorations/zkevm-specs/blob/34eafad5f5c1031834d48adc2499a0f94bc3ae3e/src/zkevm_specs/state.py#L490-L496
And the circuit should have it too.

Once the read consistency constraint is added to the state circuit, do you think there will be redundancy of constraints with the copy circuit?

ed255 · 2022-07-01T17:53:25Z

@lispc in case you haven't reviewed this yet; could you take a look please? So that we reach 2 approvals :)

lispc · 2022-07-05T03:49:26Z

@ed255 @roynalnaruto OK! I missed it! i will have a review now

lispc · 2022-07-07T10:07:09Z

great job! current codes seems much more elegant!

ed255 · 2022-07-07T22:59:49Z

@icemelon feel free to merge this PR whenever you like :) (as you submitted it originally)

DreamWuGit · 2022-07-08T08:07:05Z

I check current state circuit don't constrain value read/write consistency for now, so it not problem !

But that's just because it's a TODO rigth? The spec has de read consistency constraint:

https://github.com/privacy-scaling-explorations/zkevm-specs/blob/34eafad5f5c1031834d48adc2499a0f94bc3ae3e/src/zkevm_specs/state.py#L490-L496

And the circuit should have it too.
Once the read consistency constraint is added to the state circuit, do you think there will be redundancy of constraints with the copy circuit?

@ed255 sorry for the delay, yes, I think so , current state circuit don't implement yet. we can keep an eye when it starts implementation .
Filed issue #232 to track this in case we miss it in the future !

icemelon changed the title ~~Add Copy Circuit~~ Copy Circuit May 7, 2022

han0110 mentioned this pull request May 13, 2022

Executed bytecode source for opcode lookup #73

Closed

roynalnaruto mentioned this pull request May 26, 2022

opcode CODESIZE #191

Merged

roynalnaruto force-pushed the feat/copy-circuit branch from 357e49e to 6415046 Compare May 30, 2022 09:24

roynalnaruto force-pushed the feat/copy-circuit branch from 888d073 to e22a654 Compare June 13, 2022 10:37

icemelon marked this pull request as ready for review June 15, 2022 16:48

ed255 self-requested a review June 16, 2022 09:21

ed255 reviewed Jun 17, 2022

View reviewed changes

roynalnaruto mentioned this pull request Jun 20, 2022

Copy Circuit privacy-ethereum/zkevm-circuits#584

Merged

8 tasks

DreamWuGit reviewed Jun 23, 2022

View reviewed changes

Comment thread specs/copy-proof.md Outdated

DreamWuGit reviewed Jun 23, 2022

View reviewed changes

Comment thread src/zkevm_specs/copy_circuit.py

DreamWuGit reviewed Jun 23, 2022

View reviewed changes

roynalnaruto reviewed Jun 23, 2022

View reviewed changes

Comment thread src/zkevm_specs/copy_circuit.py Outdated

roynalnaruto reviewed Jun 23, 2022

View reviewed changes

ed255 mentioned this pull request Jun 23, 2022

Add keccak support in the copy circuit #228

Closed

ed255 reviewed Jun 27, 2022

View reviewed changes

ed255 reviewed Jun 28, 2022

View reviewed changes

Comment thread src/zkevm_specs/evm/typing.py Outdated

ed255 mentioned this pull request Jun 28, 2022

Add spec for RETURN opcode #229

Merged

2 tasks

icemelon force-pushed the feat/copy-circuit branch from 3eb2dc3 to 50b77d2 Compare June 30, 2022 05:38

ed255 approved these changes Jun 30, 2022

View reviewed changes

DreamWuGit reviewed Jun 30, 2022

View reviewed changes

icemelon added 2 commits July 5, 2022 11:41

add copy circuit

0f58135

update copy opcode

ca8c074

roynalnaruto and others added 14 commits July 5, 2022 11:46

fix: compilation, type-check and lint

c29bb43

fix(table): compilation

a0ed520

add doc for copy table

f0e33f5

fix: compilation and bytecode init

4a72e52

fix: rebase pull/214

5d401c7

add pair for root contract creation

87689fd

address comments

93156b1

fix: review revision

9816c1d

minor fix

007ec42

address comments

a51d97a

update the txlog lookup and remove logid column

3159867

lint

03b59ed

lint

73ef53e

chore: remove redundant copy-pairs from fixed table

bed19e2

roynalnaruto force-pushed the feat/copy-circuit branch from b200a05 to bed19e2 Compare July 5, 2022 03:46

lispc approved these changes Jul 7, 2022

View reviewed changes

icemelon merged commit 1ad4004 into privacy-ethereum:master Jul 7, 2022

icemelon deleted the feat/copy-circuit branch July 7, 2022 23:57

DreamWuGit mentioned this pull request Jul 8, 2022

check possible redundant constrain between copy circuit and state circuit #232

Open

Conversation

icemelon commented May 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roynalnaruto commented Jun 14, 2022

Uh oh!

ed255 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ed255 Jun 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

roynalnaruto commented Jun 17, 2022

Uh oh!

Uh oh!

Uh oh!

DreamWuGit Jun 23, 2022

Choose a reason for hiding this comment

Uh oh!

roynalnaruto Jun 23, 2022

Choose a reason for hiding this comment

Uh oh!

DreamWuGit commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

roynalnaruto left a comment

Choose a reason for hiding this comment

Uh oh!

ed255 commented Jun 23, 2022

Uh oh!

Uh oh!

ed255 Jun 27, 2022 • 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!

ed255 left a comment

Choose a reason for hiding this comment

Uh oh!

DreamWuGit commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DreamWuGit Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

icemelon Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

ed255 commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ed255 commented Jul 1, 2022

Uh oh!

lispc commented Jul 5, 2022

Uh oh!

lispc commented Jul 7, 2022

Uh oh!

ed255 commented Jul 7, 2022

Uh oh!

DreamWuGit commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

icemelon commented May 7, 2022 •

edited

Loading

DreamWuGit commented Jun 23, 2022 •

edited

Loading

ed255 Jun 27, 2022 •

edited

Loading

DreamWuGit commented Jun 30, 2022 •

edited

Loading

ed255 commented Jun 30, 2022 •

edited

Loading

DreamWuGit commented Jul 8, 2022 •

edited

Loading