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

Add more data into the execution trace #20

Merged
merged 86 commits into from
Feb 21, 2022
Merged

Add more data into the execution trace #20

merged 86 commits into from
Feb 21, 2022

Conversation

mask-pp
Copy link

@mask-pp mask-pp commented Jan 28, 2022

No description provided.

mask-pp and others added 30 commits January 12, 2022 10:38
Co-authored-by: Haichen Shen <[email protected]>
Co-authored-by: Haichen Shen <[email protected]>
Co-authored-by: Haichen Shen <[email protected]>
Co-authored-by: Haichen Shen <[email protected]>
core/types/l2trace.go Outdated Show resolved Hide resolved
Comment on lines 18 to 19
CREATE: {traceSenderAddress, traceNonce},
CREATE2: {traceSenderAddress},
Copy link

@0xmountaintop 0xmountaintop Feb 18, 2022

Choose a reason for hiding this comment

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

missing "created contract address’s accountProof"?

Copy link
Author

@mask-pp mask-pp Feb 20, 2022

Choose a reason for hiding this comment

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

Do both CREATE and CREATE2 need to be added?

Choose a reason for hiding this comment

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

according to our https://www.notion.so/EVM-Trace-2e476641b6484b71a029c868516ad7b2, both CREATE and CREATE2

Comment on lines 14 to 15
CALL: {traceToAddressCodeHash, traceLastNAddressCodeHash(1), traceOriginProof, traceLastNAddressProof(1)},
CALLCODE: {traceToAddressCodeHash, traceLastNAddressCodeHash(1), traceOriginProof, traceLastNAddressProof(1)},

Choose a reason for hiding this comment

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

using traceOriginProof here can be wrong. caller may not be the tx sender.

we can retrieve caller_address using "ScopeContext.Contract.CallerAddress"

Copy link
Author

@mask-pp mask-pp Feb 20, 2022

Choose a reason for hiding this comment

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

This line can show origin is tx sender address.

But this's a mistake we only need contract caller proof, Don't tx's sender proof, that right?

Choose a reason for hiding this comment

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

Yes, that's what I mean. We need caller instead of sender. sometime the caller is the sender, but sometimes not

@0xmountaintop
Copy link

0xmountaintop commented Feb 18, 2022

If @ChuhanJin thinks it's urgent we can merge it now, but I think there are some mistakes in thi PR:

  1. we want "caller's proof" instead of "origin (tx_sender)'s proof" in CALL & CALLCODE: Add more data into the execution trace #20 (comment)
  2. we miss "created contract address’s accountProof" in CREATE and CREATE2: Add more data into the execution trace #20 (comment)

@ChuhanJin
Copy link

Can one of the admins verify this patch?

@0xmountaintop 0xmountaintop merged commit 69c291c into scroll-tech:zkrollup Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants