Identity (dataCopy) precompile #1396

roynalnaruto · 2023-05-08T15:14:22Z

Description

[PR description]

Issue Link

[link issue here]

Type of change

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

[design decisions and extended information]

How Has This Been Tested?

[explanation]

How to fill a PR description

Please give a concise description of your PR.

The target readers could be future developers, reviewers, and auditors. By reading your description, they should easily understand the changes proposed in this pull request.

MUST: Reference the issue to resolve

Single responsability

Is RECOMMENDED to create single responsibility commits, but not mandatory.

Anyway, you MUST enumerate the changes in a unitary way, e.g.

This PR contains:
- Cleanup of xxxx, yyyy
- Changed xxxx to yyyy in order to bla bla
- Added xxxx function to ...
- Refactored ....

Design choices

RECOMMENDED to:

What types of design choices did you face?
What decisions you have made?
Any valuable information that could help reviewers to think critically

lispc · 2023-05-19T06:31:07Z

Finished the 1st round review. the framework seems good for me.

(small code style suggestion:

move the CallContext xxx rws inside bus-mapping identity.rs into a common file like util.rs or precompile.rs since later precompiles needs these too)

lispc · 2023-05-22T05:59:51Z

zkevm-circuits/src/evm_circuit/execution/callop.rs

+                    (
+                        CallContextFieldTag::LastCalleeReturnDataLength,
+                        // return_data_len.expr(),
+                        1.expr(), // TODO(rohit): fix this.


I have updated this

han0110

Overall LGTM, only have some questions about step state transition.

han0110 · 2023-05-22T07:07:12Z

zkevm-circuits/src/evm_circuit/util/precompile.rs

+        let address = BinaryNumberGadget::construct(cb, callee_address.expr());
+
+        cb.condition(address.value_equals(PrecompileCalls::Identity), |cb| {
+            cb.constrain_next_step(ExecutionState::PrecompileIdentity, None, |cb| {


Are these require_equal necessary if we already read the call context from rw table in precompile step? I feel we only need to make sure the next step is the precompile execution step read from the precompile info by callee address.

han0110 · 2023-05-22T07:24:01Z

zkevm-circuits/src/evm_circuit/execution/callop.rs

+                // Save caller's call state
+                for (field_tag, value) in [
+                    (CallContextFieldTag::LastCalleeId, callee_call_id.expr()),
+                    (CallContextFieldTag::LastCalleeReturnDataOffset, 0.expr()),
+                    (
+                        CallContextFieldTag::LastCalleeReturnDataLength,
+                        // return_data_len.expr(),
+                        1.expr(), // TODO(rohit): fix this.
+                    ),
+                ] {
+                    cb.call_context_lookup(true.expr(), None, field_tag, value);
+                }


For each precompile, their return data length should be different, should we set them in the precompile step? Or we can also handle them one by one here.

I have updated this part. It was a TODO while I was debugging/fixing something, but later forgot to update the code.

han0110 · 2023-05-22T07:27:13Z

zkevm-circuits/src/evm_circuit/execution/callop.rs

+                    call_gadget.rd_address.offset(),
+                    call_gadget.rd_address.length(),
+                )
+            },


We'd need to do step state transition here to callee's state here, and restore that in callee's step, right? Otherwise I don't see any constraint to enforce that.

This commit introduces the support for precompiled contracts. For more information, check the appendix E of the Ethereum Yellow Paper. It is a simplified version of the identity precompile PR of PSE. It strips the implementation of the identity precompile variant, leaving only the skeleton for the lookup table calls. [Ethereum Yellow Paper](https://ethereum.github.io/yellowpaper/paper.pdf) [Identity (dataCopy) precompile privacy-scaling-explorations#1396](privacy-scaling-explorations#1396)

ed255 · 2023-09-21T09:25:45Z

Status: This PR needs to be updated to address Han's review and also to rebase against main. @roynalnaruto do you have some estimation on when you plan to continue working on this? No pressure!

lispc · 2023-09-22T03:36:11Z

@darth-cy already working on this

lispc · 2023-10-16T10:32:18Z

closed for the new one #1628

@roynalnaruto

### Description This is a follow-up PR for [PR#1396](#1396) from @roynalnaruto. After merging in 1396's changes to a fresh fork, the branch is compared with Scroll's develop head to incorporate latest patterns on precompile-related development. ### Issue Link [Aggregating precompile issue 924](#924) ### Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [x] 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 ### Design choices See [documentation](https://www.notion.so/scrollzkp/PR-1396-Follow-up-Precompile-Identity-for-Upstream-3624af055d9b4820a0354e9af4fa7918) --------- Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com> Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>

prerequisite for handling precompile gadgets

9f661a3

github-actions bot added crate-eth-types Issues related to the eth-types workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels May 8, 2023

roynalnaruto added 2 commits May 9, 2023 10:59

identity gadget config

3381421

copy table lookups for identity precompile

70259ca

github-actions bot added the crate-bus-mapping Issues related to the bus-mapping workspace member label May 9, 2023

han0110 self-requested a review May 11, 2023 05:44

roynalnaruto added 2 commits May 18, 2023 13:18

Merge branch 'main' into feat/precompile-identity

eac7209

structure for verifying precompile call

6911fdc

roynalnaruto requested review from lispc and icemelon May 18, 2023 18:33

roynalnaruto added 2 commits May 18, 2023 21:57

connect bus-mapping of precompile to callop

335c14b

copy events for identity precompile

76cf3d2

roynalnaruto added 9 commits May 19, 2023 09:07

minor refactor

7a1ef55

witness assignment

24de2d2

Merge branch 'main' into feat/precompile-identity

5aefc1d

err: debugging precomp identity

63572f2

Merge branch 'main' into feat/precompile-identity

778ff12

some fixes

07775fd

wip debugging

6c6c8e1

more fixes

d389a9f

more fixes

4cc0780

roynalnaruto marked this pull request as ready for review May 21, 2023 19:50

lispc reviewed May 22, 2023

View reviewed changes

han0110 reviewed May 22, 2023

View reviewed changes

roynalnaruto added 4 commits May 22, 2023 11:25

refactoring

86f0a41

busmapping precompile call test failure fix

7d713f2

fix order of operation (rw counter issue)

a2818b5

more tests

3df95c2

Brechtpd mentioned this pull request Jun 15, 2023

Add challenge API to sha256 taikoxyz/zkevm-circuits#52

Open

ChihChengLiang mentioned this pull request Jul 5, 2023

Meta: Precompiles #924

Open

30 tasks

vlopes11 mentioned this pull request Jul 25, 2023

feat: add precompile skeleton taikoxyz/zkevm-circuits#128

Merged

4 tasks

darth-cy mentioned this pull request Sep 26, 2023

Identity (dataCopy) precompile #1628

Merged

4 tasks

lispc closed this Oct 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Identity (dataCopy) precompile #1396

Identity (dataCopy) precompile #1396

roynalnaruto commented May 8, 2023

lispc commented May 19, 2023

lispc May 22, 2023

roynalnaruto May 22, 2023

han0110 left a comment

han0110 May 22, 2023

han0110 May 22, 2023

roynalnaruto May 22, 2023

han0110 May 22, 2023

ed255 commented Sep 21, 2023

lispc commented Sep 22, 2023

lispc commented Oct 16, 2023

Identity (dataCopy) precompile #1396

Identity (dataCopy) precompile #1396

Conversation

roynalnaruto commented May 8, 2023

Description

Issue Link

Type of change

Contents

Rationale

How Has This Been Tested?

How to fill a PR description

Single responsability

Design choices

lispc commented May 19, 2023

lispc May 22, 2023

Choose a reason for hiding this comment

roynalnaruto May 22, 2023

Choose a reason for hiding this comment

han0110 left a comment

Choose a reason for hiding this comment

han0110 May 22, 2023

Choose a reason for hiding this comment

han0110 May 22, 2023

Choose a reason for hiding this comment

roynalnaruto May 22, 2023

Choose a reason for hiding this comment

han0110 May 22, 2023

Choose a reason for hiding this comment

ed255 commented Sep 21, 2023

lispc commented Sep 22, 2023

lispc commented Oct 16, 2023