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

Implementation ExecutionState::CALL#278

Merged
han0110 merged 5 commits into
privacy-ethereum:mainfrom
han0110:feature/call
Apr 4, 2022
Merged

Implementation ExecutionState::CALL#278
han0110 merged 5 commits into
privacy-ethereum:mainfrom
han0110:feature/call

Conversation

@han0110
Copy link
Copy Markdown
Contributor

@han0110 han0110 commented Jan 13, 2022 •
edited
Loading

This PR aims to implement ExecutionState::CALL.

For spec, please refer to privacy-ethereum/zkevm-specs#82.

TODO

@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 13, 2022
@ed255 ed255 self-requested a review January 13, 2022 16:22
@han0110 han0110 mentioned this pull request Jan 14, 2022
Merged
ed255
ed255 reviewed Jan 14, 2022
View reviewed changes
Comment thread bus-mapping/src/operation.rs
Comment thread bus-mapping/src/operation.rs
Comment thread bus-mapping/src/operation.rs
Comment thread bus-mapping/src/operation.rs
Comment thread geth-utils/lib/lib.go Outdated
Comment thread bus-mapping/src/circuit_input_builder.rs Outdated
Comment thread bus-mapping/src/circuit_input_builder.rs Outdated
Comment thread bus-mapping/src/evm/opcodes/call.rs Outdated
Comment thread bus-mapping/src/evm/opcodes/call.rs Outdated
Comment thread bus-mapping/src/evm/opcodes/call.rs
CPerezz
CPerezz reviewed Jan 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

Some comments. A lot of code is indeed already present in #279 and therefore I haven't left any review on it here. Instead, I left it in the other PR.

Comment thread bus-mapping/src/circuit_input_builder.rs Outdated
Comment thread bus-mapping/src/circuit_input_builder.rs Outdated
Comment thread bus-mapping/src/circuit_input_builder.rs Outdated
Comment thread bus-mapping/src/circuit_input_builder.rs Outdated
Comment thread bus-mapping/src/evm/opcodes/call.rs
Comment thread bus-mapping/src/evm/opcodes/call.rs Outdated
@ed255 ed255 mentioned this pull request Jan 17, 2022
Closed
@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Jan 17, 2022

I've opened this issue #284 to track the tasks related to improving the CircuitInputBuilder following the discussions that appeared here, so that we can move on with this PR and work on the improvements in a future PR.

@ed255 ed255 mentioned this pull request Jan 18, 2022
Merged
2 tasks
@han0110 han0110 mentioned this pull request Jan 20, 2022
Merged
@han0110 han0110 force-pushed the feature/call branch 2 times, most recently from 17b6bb1 to b91584e Compare January 20, 2022 06:47
@barryWhiteHat barryWhiteHat added benchmarks: ALL Triggers a long running prover benchmark and removed benchmarks: ALL Triggers a long running prover benchmark labels Jan 20, 2022
@AronisAt79 AronisAt79 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 and removed crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member T-opcode Type: opcode-related and focused PR/Issue labels Jan 20, 2022
@han0110 han0110 mentioned this pull request Jan 21, 2022
Merged
@barryWhiteHat barryWhiteHat added the benchmarks: ALL Triggers a long running prover benchmark label Jan 21, 2022
@han0110 han0110 force-pushed the feature/call branch 2 times, most recently from 006d6e6 to 4309eaa Compare January 21, 2022 14:15
@barryWhiteHat barryWhiteHat removed crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Jan 21, 2022
@adria0 adria0 mentioned this pull request Mar 11, 2022
Closed
11 tasks
@silathdiir silathdiir mentioned this pull request Mar 18, 2022
Merged
@han0110 han0110 changed the title [WIP] Implementation ExecutionState::CALL Implementation ExecutionState::CALL Mar 23, 2022
@han0110 han0110 marked this pull request as ready for review March 23, 2022 04:09
@han0110 han0110 requested review from CPerezz and ed255 March 28, 2022 17:32
icemelon
icemelon reviewed Mar 30, 2022
View reviewed changes
Comment thread eth-types/src/evm_types/memory.rs Outdated
Comment thread zkevm-circuits/src/evm_circuit/util/constraint_builder.rs
ed255
ed255 approved these changes Mar 30, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@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.

Great work! Overall looks good to me, so I'm approving the PR.
The CallGadget implementation was easy to follow from the python spec as it follows the exact same structure :)

I've left some suggestions to improve some details, please take a look!

Comment thread bus-mapping/src/evm/opcodes.rs Outdated
Comment thread bus-mapping/src/evm/opcodes/call.rs Outdated
Comment thread bus-mapping/src/evm/opcodes/call.rs
Comment thread eth-types/src/bytecode.rs Outdated
Comment thread zkevm-circuits/src/evm_circuit/util/common_gadget.rs
Comment thread zkevm-circuits/src/evm_circuit/execution/call.rs
Comment thread zkevm-circuits/src/evm_circuit/execution/call.rs Outdated
Comment thread zkevm-circuits/src/evm_circuit/execution/call.rs Outdated
CPerezz
CPerezz reviewed Mar 30, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

  • We should probably consider moving begin_tx ops impl to its own file instead of being in opcodes.rs in bus-mapping crate.

Other concerns/questions/nits are in the comments directly.

Awesome work!!!! Looks close to be merged!!! 🥇

Comment thread bus-mapping/src/evm/opcodes.rs
Comment thread bus-mapping/src/evm/opcodes.rs Outdated
Comment thread bus-mapping/src/evm/opcodes.rs Outdated
Comment thread bus-mapping/src/evm/opcodes.rs Outdated
Comment thread bus-mapping/src/evm/opcodes/call.rs Outdated
Comment thread eth-types/src/lib.rs Outdated
Comment thread zkevm-circuits/src/evm_circuit/execution/call.rs
Comment thread zkevm-circuits/src/evm_circuit/execution/call.rs
Comment thread zkevm-circuits/src/evm_circuit/execution/call.rs Outdated
Comment thread zkevm-circuits/src/evm_circuit/execution/extcodehash.rs
@github-actions github-actions Bot added the crate-keccak Issues related to the keccak workspace member label Mar 31, 2022
@CPerezz CPerezz self-requested a review April 1, 2022 11:14
CPerezz
CPerezz approved these changes Apr 1, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

LGTM!

Awesome work @han0110 🥳 🎉

@han0110 han0110 mentioned this pull request Apr 2, 2022
Merged
6 tasks
@han0110 han0110 merged commit 706aa59 into privacy-ethereum:main Apr 4, 2022
@han0110 han0110 deleted the feature/call branch April 4, 2022 04:42
@CPerezz CPerezz mentioned this pull request Apr 4, 2022
Closed
19 tasks
@roynalnaruto roynalnaruto mentioned this pull request Apr 7, 2022
Merged
1 task
@ed255 ed255 mentioned this pull request Apr 27, 2022
Closed
lispc pushed a commit to zhenfeizhang/zkevm-circuits that referenced this pull request Feb 1, 2023
@silathdiir
Add timeout for action cache in CI. (privacy-ethereum#278)
393cfac
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@icemelon icemelon icemelon left review comments

@ed255 ed255 ed255 approved these changes

@CPerezz CPerezz CPerezz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

benchmarks: ALL Triggers a long running prover benchmark crate-bus-mapping Issues related to the bus-mapping workspace member crate-keccak Issues related to the keccak workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member T-opcode Type: opcode-related and focused PR/Issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@han0110 @ed255 @icemelon @CPerezz @barryWhiteHat @AronisAt79