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

Implement PUSH0 instruction #1463

Merged
merged 7 commits into from
Aug 23, 2023
Merged

Implement PUSH0 instruction #1463

merged 7 commits into from
Aug 23, 2023

Conversation

silathdiir
Copy link
Contributor

@silathdiir silathdiir commented Jun 8, 2023 •
edited
Loading

Description

  1. Add PUSH0 opcode to eth-types. It could be parsed from 0x5f or a string of PUSH0.

  2. In eth-types, update is_push function to return true for PUSH0 .. PUSH32, and add a new function is_push_with_data, it returns true for PUSH1 .. PUSH32.

  3. Small fixes to replace PUSH1 - 1 with PUSH0 value.

  4. Add PUSH0 implementation to bus-mapping, and PushGadget to support PUSH0 in zkevm-circuits.

Issue Link

Close #1473

Reference previous PR #1361

zkevm-specs PR privacy-scaling-explorations/zkevm-specs#471

Type of change

  • New feature (non-breaking change which adds functionality)

@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-eth-types Issues related to the eth-types workspace member crate-geth-utils Issues related to the geth-utils workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Jun 8, 2023
@ed255 ed255 linked an issue Jun 15, 2023 that may be closed by this pull request
Implement PUSH0 instruction #1473
Closed
@ed255
Copy link
Member

ed255 commented Jun 15, 2023

Hi @silathdiir thanks for this PR! I noticed that we don't have the spec/python for PUSH0 in https://github.com/privacy-scaling-explorations/zkevm-specs could you also make a spec PR so that we can review that first?

@silathdiir
Copy link
Contributor Author

Hi @silathdiir thanks for this PR! I noticed that we don't have the spec/python for PUSH0 in https://github.com/privacy-scaling-explorations/zkevm-specs could you also make a spec PR so that we can review that first?

Got it. Will add a spec PR first. Thanks.

@silathdiir silathdiir marked this pull request as draft June 26, 2023 05:50
@ed255 ed255 mentioned this pull request Jul 13, 2023
Closed
4 tasks
@ed255
Copy link
Member

ed255 commented Jul 13, 2023

Hi @silathdiir do you have any update on the spec for the PUSH0? Do you need any help from our side?

@silathdiir
Copy link
Contributor Author

silathdiir commented Jul 13, 2023 •
edited
Loading

Hi @silathdiir do you have any update on the spec for the PUSH0? Do you need any help from our side?

I am sorry that I haven't start to work on the spec for PUSH0. Will try to start on it later.

@silathdiir silathdiir force-pushed the shanghai/eip-3855-push0 branch 2 times, most recently from 4be4555 to 032a4af Compare July 23, 2023 07:39
@silathdiir silathdiir mentioned this pull request Jul 23, 2023
Merged
@silathdiir silathdiir marked this pull request as ready for review July 23, 2023 07:53
@silathdiir
Copy link
Contributor Author

Hi @ed255, sorry for the delay. I add the PUSH0 spec PR privacy-scaling-explorations/zkevm-specs#471, and update the circuit. Please help review when you have time. Thanks.

@lispc
Copy link
Collaborator

lispc commented Jul 28, 2023

CI: "Waiting for a runner to pick up this job..."

@ed255
Copy link
Member

ed255 commented Jul 28, 2023 •
edited
Loading

CI: "Waiting for a runner to pick up this job..."

It seems the self hosted github workers are taking a long time to pick up the jobs. Maybe the AWS region we're using has low availability for the required server instances right now?

Update: On the last run, the time for the workers to pick up the job was within an acceptable time range, so probably the previous issue was temporary.

rrtoledo
rrtoledo reviewed Aug 1, 2023
View reviewed changes
Copy link
Contributor

@rrtoledo rrtoledo left a comment

Choose a reason for hiding this comment

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

The branch needs to be rebased (since pushn has been merged). I added a question for checking that 0 is indeed pushed on the stack.

zkevm-circuits/src/evm_circuit/execution/push.rs Outdated
if idx == 0 {
- (OpcodeId::PUSH0.as_u8() + idx as u8).expr();

cb.condition(sel.expr(), |cb| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we also lookup-check the byte if is_push0 is true at idx=0 (we check later on that byte = 0 when is_push0 is true)?
In which case, for idx=0, we could remove the condition iiuc.

@lispc
Copy link
Collaborator

lispc commented Aug 1, 2023

on scroll fork, we support both push0 and the new PUSHN full constraints. We can pick the changes later this week. ref scroll-tech@2b4e90d

@rrtoledo
Copy link
Contributor

rrtoledo commented Aug 2, 2023

on scroll fork, we support both push0 and the new PUSHN full constraints. We can pick the changes later this week. ref scroll-tech@2b4e90d

Thanks!
I have found an error in the code however, I commented on the commit directly ref scroll-tech@2b4e90d

@silathdiir
Copy link
Contributor Author

Hi @rrtoledo, I pick the latest changes of PUSHn from scroll. Please help review again when you have time. Thanks.

@ChihChengLiang ChihChengLiang self-requested a review August 9, 2023 22:10
@github-actions github-actions bot removed the crate-geth-utils Issues related to the geth-utils workspace member label Aug 11, 2023
ChihChengLiang
ChihChengLiang approved these changes Aug 11, 2023
View reviewed changes
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

LGTM with some nitpicks.
Thanks for adding the PUSH0 and clean up the circuit code!

bus-mapping/src/evm/opcodes.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/push.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/push.rs
Comment on lines +80 to +81
let mut is_pushed_cell_prev = true.expr();
let mut is_padding_cell_prev = true.expr();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great adding clear variable names :)

rrtoledo
rrtoledo approved these changes Aug 23, 2023
View reviewed changes
Copy link
Contributor

@rrtoledo rrtoledo left a comment

Choose a reason for hiding this comment

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

LGTM

@ChihChengLiang ChihChengLiang added this pull request to the merge queue Aug 23, 2023
Merged via the queue into privacy-scaling-explorations:main with commit 96b3c4e Aug 23, 2023
11 checks passed
@silathdiir silathdiir deleted the shanghai/eip-3855-push0 branch August 23, 2023 14:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rrtoledo rrtoledo rrtoledo approved these changes

@ChihChengLiang ChihChengLiang ChihChengLiang approved these changes

Assignees
No one assigned
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-eth-types Issues related to the eth-types workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement PUSH0 instruction
5 participants
@silathdiir @ed255 @lispc @rrtoledo @ChihChengLiang