Skip to content
This repository was archived by the owner on Apr 18, 2025. It is now read-only.

GasGadget for GAS opcode#72

Closed
roynalnaruto wants to merge 6 commits into
mainfrom
feat/gas-gadget
Closed

GasGadget for GAS opcode#72
roynalnaruto wants to merge 6 commits into
mainfrom
feat/gas-gadget

Conversation

@roynalnaruto
Copy link
Copy Markdown

@roynalnaruto roynalnaruto commented Jan 17, 2022 •
edited
Loading

Implement the GasGadget for opcode GAS (5A).

Specs here.

Other changes:

  1. Fix constant gas cost for OpcodeId::EXP (comments below)

icemelon
icemelon reviewed Jan 17, 2022
View reviewed changes
Copy link
Copy Markdown
Member

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

I think you need to add GAS implementation in the bus-mapping (bus-mapping/src/evm/opcodes/) as well to pass the CI

Comment thread zkevm-circuits/src/evm_circuit/execution/gas.rs Outdated
roynalnaruto
roynalnaruto commented Jan 17, 2022
View reviewed changes
Comment thread bus-mapping/src/evm/opcodes/ids.rs
roynalnaruto
roynalnaruto commented Jan 17, 2022
View reviewed changes
Copy link
Copy Markdown
Author

@roynalnaruto roynalnaruto left a comment

Choose a reason for hiding this comment

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

@icemelon added a comment for the gas cost bug fix for OpcodeId::EXP

@roynalnaruto roynalnaruto requested a review from icemelon January 17, 2022 08:45
@lispc
Copy link
Copy Markdown

lispc commented Jan 20, 2022

Is it possible to use StackOnlyOpcode::<0>::gen_associated_ops like #73 ?

@roynalnaruto
Copy link
Copy Markdown
Author

roynalnaruto commented Jan 20, 2022

Is it possible to use StackOnlyOpcode::<0>::gen_associated_ops like #73 ?

@lispc Yes its possible to use after the change: step.stack.last_filled().map(|a| a - 1 + N)

I had earlier seen step.stack.nth_last_filled(N - 1) hence decided to create a separate Opcode implementation. But I can re-use the most updated one (although the TIMESTAMP PR will need to be merged before then).

@lispc
Copy link
Copy Markdown

lispc commented Jan 20, 2022

ok i suggest StackOnlyOpcode::<0> after merging of timestamp

icemelon
icemelon reviewed Jan 21, 2022
View reviewed changes
Comment thread zkevm-circuits/src/evm_circuit/param.rs Outdated
Comment thread zkevm-circuits/src/evm_circuit/execution/gas.rs Outdated
Comment thread zkevm-circuits/src/evm_circuit/execution/gas.rs Outdated
Comment thread zkevm-circuits/src/evm_circuit/execution/gas.rs Outdated
Comment thread zkevm-circuits/src/evm_circuit/execution/gas.rs Outdated
Comment thread zkevm-circuits/src/evm_circuit/execution/gas.rs Outdated
icemelon
icemelon approved these changes Jan 21, 2022
View reviewed changes
Copy link
Copy Markdown
Member

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

LGTM. need to rebase

@icemelon
Copy link
Copy Markdown
Member

icemelon commented Jan 22, 2022

Close and move to privacy-ethereum#301

@icemelon icemelon closed this Jan 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lispc lispc Awaiting requested review from lispc

@DreamWuGit DreamWuGit Awaiting requested review from DreamWuGit

@0xmountaintop 0xmountaintop Awaiting requested review from 0xmountaintop

1 more reviewer

@icemelon icemelon icemelon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bus-mapping opcode

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@roynalnaruto @lispc @icemelon