Add circuit for RETURNDATASIZE opcode by xiaodino · Pull Request #703 · privacy-ethereum/zkevm-circuits

xiaodino · 2022-08-22T22:58:20Z

No description provided.

CPerezz

Great work! And thanks for submitting a PR for it.

I have some things that should probably be addressed. Specifically with the testing cases applied here.

Aside from that the rest looks good! 😄

Signed-off-by: smtmfft <smtm@taiko.xyz> Signed-off-by: smtmfft <smtm@taiko.xyz>

…ircuits into xiaodino/returndatasize

xiaodino

@CPerezz Thanks for the feedbacks. They have been addressed.

CPerezz

LGTM!! Thanks for your work on the comments @xiaodino

Requesting one more review before merging.
Maybe @roynalnaruto can check this?

roynalnaruto · 2022-08-29T07:50:44Z

I'll review this PR before end of today.

CPerezz · 2022-08-29T08:03:19Z

@xiaodino please can you fix the issues with rustfmt?

Signed-off-by: smtmfft <smtm@taiko.xyz> Signed-off-by: smtmfft <smtm@taiko.xyz>

) i.e., only if it is not root call. RETURN spec: The `RETURN` opcode terminates the call successfully with return data for the caller. It behaves differently in different scenarios: - `is_create` and `is_root` - A. Returns the specified memory chunk as deployment code. - B. End the execution - `is_create` and `not is_root` - A. Returns the specified memory chunk as deployment code. - C. Restores caller's context and switch to it. - `not is_create` and `is_root` - B. End the execution - `not is_create` and `not is_root` - D. Returns the specified memory chunk to the caller. - C. Restores caller's context and switch to it. Signed-off-by: smtmfft <smtmfft@taikocha.in> Signed-off-by: smtmfft <smtmfft@taikocha.in> Co-authored-by: smtmfft <smtmfft@taikocha.in>

…ircuits into xiaodino/returndatasize

xiaodino · 2022-08-29T16:38:50Z

The issues with rustfmt have been fixed.

…d update tests

…ircuits into xiaodino/returndatasize

CPerezz · 2022-09-12T18:00:24Z

@roynalnaruto I actually don't really agree with the U256 instead of U64 used for the value RLC.

See: #703 (comment)

roynalnaruto · 2022-09-12T22:48:29Z

@roynalnaruto I actually don't really agree with the U256 instead of U64 used for the value RLC.

See: #703 (comment)

I mentioned that since geth uses uint64, so I thought we should stick to 8 bytes (N_BYTES_U64). N_BYTES_MEMORY_ADDRESS was 5 bytes.

Brechtpd · 2022-09-13T20:11:24Z

I think in most opcodes we do use N_BYTES_MEMORY_WORD_SIZE for storing a similar value (memory length/memory offsets in MemoryAddress types). Though a small difference here is that this opcode on its own does not not need any memory expansion (which is where the gas cost would overflow a 64bit value for these 5-byte sizes/offsets already) because it's a simple getter. So in that aspect N_BYTES_U64 could make sense instead, but of course the RETURN will already have done the memory expansion so it is as impossible to have bigger values than N_BYTES_MEMORY_WORD_SIZE as in all other uses cases where that is used I believe.

So good points to make for both I feel, but practically N_BYTES_MEMORY_WORD_SIZE should be as okay as its use in other opcodes. On the other hand, using N_BYTES_U64 won't make that much difference.

han0110

LGTM! After rebase we could merge this.

…ircuits into xiaodino/returndatasize

han0110

LGTM! Once the CI passes we can merge this.

…ircuits into xiaodino/returndatasize

xiaodino · 2022-10-29T04:58:01Z

LGTM! Once the CI passes we can merge this.

CI passed!

* speedup ci using verify_at_rows * use verify_at_rows to speedup super circuit ci tests

…s for the dummy chunk proofs to avoid dummy chunk proofgen time (#712) * partiall address review comments of #670 * disable disable_proof_agg by default * fix soundness issues with data hash * add fixed cells for chunk_is_valid * fix typo * well... need morning coffee * typo in readme * fix soundness issue in is_smaller_than * [feat] unify u8 u16 lookup table (#694) * add range table * replace Range256Table of rlp_circuit_fsm * replace u16_table of tx_circuit * use TableColumn * add type alias * use type alias * annotate lookup column * opt min_num_rows_block in poseidon_circuit (#700) prune debug prints and lint * speedup ci using verify_at_rows (#703) * speedup ci using verify_at_rows * use verify_at_rows to speedup super circuit ci tests * update super circuit row estimation API (#695) * remove num_of_chunks in aggregation circuit's instance column (#704) Co-authored-by: kunxian xia <xiakunxian130@gmail.com> * fix: lost tx_type when doing type conversion (#710) * fix condition (#708) * gates for zero checks * statement 1 is correct * reenable statements 3,6,7 * reenable statement 4 * everything seems to work again * update aggregation test accordingly * update spec * minor clean up * Fix fmt. * Make `ChunkHash` fields public. * fix decompose function * update figures * clean up * address comments * add is_final checks * update readme * constraint hash input length * fix clippy --------- Co-authored-by: Akase Cho <lightsing@users.noreply.github.com> Co-authored-by: Ho <noel.wei@gmail.com> Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com> Co-authored-by: kunxian xia <xiakunxian130@gmail.com> Co-authored-by: Steven Gu <asongala@163.com>

Add circuit for RETURNDATASIZE opcode

219b303

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 Aug 22, 2022

CPerezz self-requested a review August 23, 2022 07:25

CPerezz suggested changes Aug 23, 2022

View reviewed changes

xiaodino and others added 7 commits August 25, 2022 00:44

Update tests

dce1366

update last call info in return (#2)

3a1d151

Signed-off-by: smtmfft <smtm@taiko.xyz> Signed-off-by: smtmfft <smtm@taiko.xyz>

Update tests

d162713

Merge branch 'xiaodino/returndatasize' of github.com:xiaodino/zkevm-c…

f3d78c4

…ircuits into xiaodino/returndatasize

Update tests

d8891b7

Update format in zkevm-circuits/src/evm_circuit/execution.rs

69080e1

Change unwrap to expect in returndatasize.rs

fde42c3

xiaodino commented Aug 27, 2022

View reviewed changes

xiaodino requested a review from CPerezz August 27, 2022 07:46

CPerezz approved these changes Aug 29, 2022

View reviewed changes

CPerezz requested a review from roynalnaruto August 29, 2022 07:43

xiaodino and others added 11 commits August 29, 2022 10:15

Add circuit for RETURNDATASIZE opcode

aed1d7d

Update tests

012cea4

update last call info in return (#2)

136fad9

Signed-off-by: smtmfft <smtm@taiko.xyz> Signed-off-by: smtmfft <smtm@taiko.xyz>

Update tests

c0b8872

Update tests

2955c3d

Update format in zkevm-circuits/src/evm_circuit/execution.rs

7a03cb4

Change unwrap to expect in returndatasize.rs

fbdaa0e

Merge branch 'xiaodino/returndatasize' of github.com:xiaodino/zkevm-c…

3dda80d

…ircuits into xiaodino/returndatasize

Fix the issues with rustfmt

6f7766c

Fix merge conflicts

0ee04d2

xiaodino added 3 commits September 12, 2022 08:04

Merge branch 'main' into xiaodino/returndatasize

700b07a

Update bus-mapping return.rs, use N_BYTES_U64 for return_data_size an…

2d057a0

…d update tests

Merge branch 'xiaodino/returndatasize' of github.com:xiaodino/zkevm-c…

3e8f1f0

…ircuits into xiaodino/returndatasize

xiaodino requested a review from roynalnaruto September 12, 2022 15:36

smtmfft mentioned this pull request Sep 19, 2022

Add circuit for RETURNDATACOPY opcode #779

Merged

Merge branch 'main' into xiaodino/returndatasize

c0d6421

z2trillion requested a review from a team September 23, 2022 05:36

roynalnaruto added 2 commits September 24, 2022 17:33

Merge branch 'main' into xiaodino/returndatasize

3b5841f

Merge branch 'main' into xiaodino/returndatasize

6b9becc

smtmfft mentioned this pull request Oct 9, 2022

[zkevm] RETURNDATASIZE impl taikoxyz/taiko-mono#126

Closed

han0110 reviewed Oct 27, 2022

View reviewed changes

Comment thread bus-mapping/src/evm/opcodes/return.rs

xiaodino added 3 commits October 27, 2022 22:23

Revert changes in bus-mapping return.rs

18cdb29

Merge branch 'xiaodino/returndatasize' of github.com:xiaodino/zkevm-c…

595847b

…ircuits into xiaodino/returndatasize

Revert change in bus-mapping return

6696a1b

xiaodino requested review from han0110 and removed request for roynalnaruto October 28, 2022 05:54

han0110 approved these changes Oct 28, 2022

View reviewed changes

adria0 and others added 4 commits October 28, 2022 08:14

Merge branch 'main' into xiaodino/returndatasize

0f035a1

Merge branch 'main' into xiaodino/returndatasize

f5ac803

Update bus-mapping returndatasize_tests

db9e3d4

Merge branch 'xiaodino/returndatasize' of github.com:xiaodino/zkevm-c…

06f7b69

…ircuits into xiaodino/returndatasize

han0110 merged commit 103ddd7 into privacy-ethereum:main Oct 29, 2022

xiaodino mentioned this pull request Oct 30, 2022

Implement RETURNDATASIZE #698

Closed

pinkiebell mentioned this pull request Nov 5, 2022

Required opcodes for basic L2 Bridge #369

Closed

19 tasks

lispc added a commit that referenced this pull request Aug 3, 2023

speedup ci using verify_at_rows (#703)

610f4f1

* speedup ci using verify_at_rows * use verify_at_rows to speedup super circuit ci tests

Conversation

xiaodino commented Aug 22, 2022

Uh oh!

CPerezz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xiaodino left a comment • 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!

CPerezz left a comment

Choose a reason for hiding this comment

Uh oh!

roynalnaruto commented Aug 29, 2022

Uh oh!

CPerezz commented Aug 29, 2022

Uh oh!

xiaodino commented Aug 29, 2022

Uh oh!

CPerezz commented Sep 12, 2022

Uh oh!

roynalnaruto commented Sep 12, 2022

Uh oh!

Brechtpd commented Sep 13, 2022

Uh oh!

han0110 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

han0110 left a comment

Choose a reason for hiding this comment

Uh oh!

xiaodino commented Oct 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

xiaodino left a comment •

edited

Loading