Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialize LastCallee* of CallContext #298

Conversation

han0110
Copy link
Collaborator

@han0110 han0110 commented Jan 21, 2022

This PR is once #278, and aims to initialize last callee's information of CallContext in BeginTx.

For spec, see privacy-scaling-explorations/zkevm-specs#99.

It depends on #292.

@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member T-opcode Type: opcode-related and focused PR/Issue crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Jan 21, 2022
@han0110 han0110 mentioned this pull request Jan 21, 2022
6 tasks
@han0110 han0110 force-pushed the feature/untrack-last-callee-info branch 2 times, most recently from c84f32f to e9d94e0 Compare January 21, 2022 14:15
@@ -166,12 +163,20 @@ impl<F: FieldExt> ExecutionGadget<F> for BeginTxGadget<F> {
),
(CallContextFieldTag::Value, tx_value.expr()),
(CallContextFieldTag::IsStatic, 0.expr()),
(CallContextFieldTag::LastCalleeId, 0.expr()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

want to know why call last callee in begin_tx ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid a malicious prover set these fields to read arbitrary call's memory, we need to make sure it's initilized to 0 properly

@han0110 han0110 force-pushed the feature/untrack-last-callee-info branch 2 times, most recently from 1ec98b0 to 7d398d7 Compare January 30, 2022 09:50
@han0110 han0110 force-pushed the feature/untrack-last-callee-info branch 2 times, most recently from df8d1ae to 189b340 Compare February 18, 2022 03:37
@github-actions github-actions bot removed the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label Feb 18, 2022
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I have a question though:
LastCallee* rows are always READ right?

But in the same call, we can have different values of LastCallee.
Imagine the following situation

Begin CALL 1
  Begin CALL 2
  End CALL 2
  Begin CALL 3
  End CALL 3
End Call 1

I imagine that the LastCalleeId rows for callId = 1 (in rw_table) will be:

0 rwc 1 isWrite 2 Key0 (Tag) 3 callId 5 Key3 7 Value0
1 false CallContext 1 LastCalleeId 0
2 false CallContext 1 LastCalleeId 2
3 false CallContext 1 LastCalleeId 3

Is this right? If that's the case, then these rows don't follow the READ constraint, because for the same set of keys, the value is changing. Am I missing something?

@han0110
Copy link
Collaborator Author

han0110 commented Feb 18, 2022

Fields LastCallee* will be written when callee switch context back to caller (e.g. STOP, RETURN, REVERT, all the other ExecutionState that halt). So in your case it would be like:

0 rwc 1 isWrite 2 Key0 (Tag) 3 callId 5 Key3 7 Value0
1 false CallContext 1 LastCalleeId 0
2 true CallContext 1 LastCalleeId 2
3 false CallContext 1 LastCalleeId 2
4 true CallContext 1 LastCalleeId 3
5 false CallContext 1 LastCalleeId 3

There is also implemented in privacy-scaling-explorations/zkevm-specs#100 and the writes are here

@ed255
Copy link
Member

ed255 commented Feb 18, 2022

Fields LastCallee* will be written when callee switch context back to caller (e.g. STOP, RETURN, REVERT, all the other ExecutionState that halt). So in your case it would be like:
...

Thanks! This resolves my doubt. I had the misunderstanding that LastCalleeId* rows where never WRITE.

Copy link
Collaborator

@DreamWuGit DreamWuGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks !

@@ -652,6 +652,12 @@ pub enum CallContextField {
IsPersistent,
/// IsStatic
IsStatic,
/// LastCalleeId
LastCalleeId,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just thinking how about if we only record LastCalleeId without LastCalleeReturnDataOffset LastCalleeReturnDataOffset ? then we can look up those two items by LastCalleeId as already have ReturnDataOffset ReturnDataOffset , something missing ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fields LastCalleeReturnData* are decided by the callee's step which halts.

Fields ReturnData* are decided by the caller's step which triggers the call, to ask callee to copy the ReturnData to the specified range.

So they are for different purposes.

@CPerezz
Copy link
Member

CPerezz commented Feb 23, 2022

Should we update and merge this @han0110 ?

@han0110 han0110 force-pushed the feature/untrack-last-callee-info branch 2 times, most recently from a99b432 to ace926d Compare February 25, 2022 05:03
@han0110 han0110 force-pushed the feature/untrack-last-callee-info branch from ace926d to e0cbfd9 Compare February 26, 2022 07:43
@han0110 han0110 merged commit a07200f into privacy-scaling-explorations:main Feb 26, 2022
@han0110 han0110 deleted the feature/untrack-last-callee-info branch February 26, 2022 08:34
zemse pushed a commit to zemse/zkevm-circuits that referenced this pull request Mar 22, 2023
Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member T-opcode Type: opcode-related and focused PR/Issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants