CODECOPY opcode

[roynalnaruto]

Circuits PR: privacy-ethereum/zkevm-circuits#391

[CPerezz]

Hey @roynalnaruto! I saw the PR for the circuits already up and ready for review.
Could we set it in Draft and focus on merging this one first? As until we don't have a merged spec we can't approve a circuit PR directly.

[roynalnaruto]

Hey @roynalnaruto! I saw the PR for the circuits already up and ready for review. Could we set it in Draft and focus on merging this one first? As until we don't have a merged spec we can't approve a circuit PR directly.

@CPerezz I just had to push an unchanged change and rebase with upstream. I've now opened this PR.

[han0110]

Hi @roynalnaruto, thanks for working on this!

Have you considered about retrieving the CodeSize information from bytecode_table? Bytecode circuit already has it, but just currently not designed to be lookup-able (see #102 for more discussion).

Also state trie doesn't contain the CodeSize of account, so we can't rely on MPT circuit to open its value, then we still need to check it with the one in Bytecode circuit every time when we access an account, so I think it doesn't makes too much sense to have such information maintained in State circuit.

[CPerezz]

Some nits/questions.
Overall is looking good! :)

[CPerezz]

@han0110 made an excellent point there!
Bytecode table should be a place that allows us to leverage info holded now by te State circuit.

Worth considering to refactor this in that regard.

[roynalnaruto]

@han0110 @CPerezz I will check more on #102 and refactor the CodeSize part. Thanks for the review :)

[roynalnaruto]

@han0110 @CPerezz I've added a commit to extend the bytecode circuit (and related code/tests) that:

1. Adds tags Length and Byte as BytecodeFieldTag
2. Adds a row to the bytecode circuit (for every bytecode) for its length
3. CODECOPY makes use of instruction.bytecode_length(code_hash) to fetch the bytecode size

[han0110]

Overall looks good! Although I thought you could just change the interface in table.py without needing to worry about updating bytecode.py itself.

Currently Bytecode circuit is modified to have access to previous row and next row, which seems hard to track all the branches (also brings some duplication I think), I have submitted another proposal #151 that I always want to try, if you are interesting perhaps you can have a look and implement it in another PR.

[roynalnaruto]

I have submitted another proposal #151 that I always want to try, if you are interesting perhaps you can have a look and implement it in another PR.

@han0110 Yes, I think the proposal makes a lot of sense. I would like to take that up and refactor the bytecode circuit. But I believe that should be done as a separate PR, once this one is approved and merged (just trying to avoid too many changes and potential conflicts for others working on this repo). Wdyt?

[han0110]

Yes, I think the proposal makes a lot of sense. I would like to take that up and refactor the bytecode circuit. But I believe that should be done as a separate PR, once this one is approved and merged (just trying to avoid too many changes and potential conflicts for others working on this repo). Wdyt?

Sure, let's do it in another PR.

[han0110]

All LGTM now. Great work!

[roynalnaruto]

@CPerezz and @icemelon would be good to have your reviews on the PR :)

[CPerezz]

LGTM!! Nice job!!!

Only a small nit in the markdown file 🙂

[ChihChengLiang]

@roynalnaruto Can we rebase this and merge?