Skip to content
This repository was archived by the owner on Jul 5, 2024. It is now read-only.

CALLDATACOPY: Internal call and refactoring#450

Merged
han0110 merged 8 commits into
privacy-ethereum:mainfrom
scroll-tech:fix/calldatacopy-internal
Apr 12, 2022
Merged

CALLDATACOPY: Internal call and refactoring#450
han0110 merged 8 commits into
privacy-ethereum:mainfrom
scroll-tech:fix/calldatacopy-internal

Conversation

@roynalnaruto
Copy link
Copy Markdown
Collaborator

@roynalnaruto roynalnaruto commented Apr 9, 2022

Addresses and closes: privacy-ethereum/zkevm-specs#134

Specs PR:

This PR:

@roynalnaruto roynalnaruto requested review from a team and han0110 and removed request for a team April 9, 2022 03:34
@github-actions github-actions Bot added the T-opcode Type: opcode-related and focused PR/Issue label Apr 9, 2022
@roynalnaruto
Copy link
Copy Markdown
Collaborator Author

@han0110 #434 (comment)

@roynalnaruto roynalnaruto force-pushed the fix/calldatacopy-internal branch from bc7afd9 to 4ce1dfa Compare April 11, 2022 08:04
@github-actions github-actions Bot added the crate-bus-mapping Issues related to the bus-mapping workspace member label Apr 11, 2022
@CPerezz CPerezz removed the opcode label Apr 11, 2022
@github-actions github-actions Bot added crate-zkevm-circuits Issues related to the zkevm-circuits workspace member and removed T-opcode Type: opcode-related and focused PR/Issue labels Apr 12, 2022
@roynalnaruto
Copy link
Copy Markdown
Collaborator Author

@han0110 I've updated the bus-mapping tests for CALLDATACOPY to test an internal call case:
https://github.com/scroll-tech/zkevm-circuits/blob/fix/calldatacopy-internal/bus-mapping/src/evm/opcodes/calldatacopy.rs#L201

which passes as expected.

Although the same test fails in the evm_circuit/execution module:
https://github.com/scroll-tech/zkevm-circuits/blob/fix/calldatacopy-internal/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs#L500

with this error:

`Err([Lookup { name: "LOOKUP: \"Fixed table\"", lookup_index: 33, location: InRegion { region: Region { index: 5, name: "Execution step" }, offset: 176 } }, Lookup { name: "LOOKUP: \"Fixed table\"", lookup_index: 38, location: InRegion { region: Region { index: 5, name: "Execution step" }, offset: 176 } }, Lookup { name: "LOOKUP: \"Bytecode table\"", lookup_index: 162, location: InRegion { region: Region { index: 5, name: "Execution step" }, offset: 288 } }])`

Do you know what might be the issue here?

@han0110
Copy link
Copy Markdown
Contributor

han0110 commented Apr 12, 2022

  1. I forgot to update the get_fixed_table to have Range64 and Range1024 (my apology), so 2 fixed_table lookup in CALL failed. Adding them should remove first 2 failures.
  2. Currently STOP can't handle index bytecode out of range, so the bytecode_table lookup failed. Adding a STOP after https://github.com/scroll-tech/zkevm-circuits/blob/fix/calldatacopy-internal/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs#L533 should remove the last failure.

@roynalnaruto
Copy link
Copy Markdown
Collaborator Author

Should I update get_fixed_table and commit in this PR?

@han0110
Copy link
Copy Markdown
Contributor

han0110 commented Apr 12, 2022

Yes, let's fix it in this PR. Thanks!

Copy link
Copy Markdown
Collaborator

@icemelon icemelon left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this. Overall LGTM. Just one minor comment

Comment thread zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs Outdated
@roynalnaruto
Copy link
Copy Markdown
Collaborator Author

@han0110 This is ready to be reviewed again. All changes included from my end.

@roynalnaruto roynalnaruto changed the title CALLDATACOPY: Read from caller memory in case of internal call CALLDATACOPY: Internal call and refactoring Apr 12, 2022
Copy link
Copy Markdown
Contributor

@han0110 han0110 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!

@han0110 han0110 merged commit 3eae97d into privacy-ethereum:main Apr 12, 2022
@roynalnaruto roynalnaruto deleted the fix/calldatacopy-internal branch April 12, 2022 11:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CALLDATACOPY should read caller's memory

4 participants