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

Introduce support for PUSH0 #1361

Closed
wants to merge 4 commits into from
Closed

Introduce support for PUSH0 #1361

wants to merge 4 commits into from

Conversation

axic
Copy link
Contributor

@axic axic commented Apr 16, 2023 •
edited
Loading

Description

Introduction of support of the PUSH0 instruction, together with bumping geth integration to a newer version supporting it.

This is a PR for discussion and not meant to be merged, given the amount of changes and potential regressions.

Issue Link

#1362

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Contents

The PR consists of 3 parts:

  • Introduction of PUSH0 (both in the circuits and testing). This is simple enough given the instruction is very basic.
  • Updating geth from v1.10.18 to v1.11.5. The previous version is rather old (and it of course does not support Shanghai). This I think could be pulled out and merged on its own.
  • Changing geth to run in Shanghai mode. This change is risky and likely causes failures.

Rationale

N/A

How Has This Been Tested?

Unit test in bus-mapping.

@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 Apr 16, 2023
axic
axic commented Apr 16, 2023
View reviewed changes
eth-types/src/bytecode.rs
@@ -96,6 +96,11 @@ impl Bytecode {

/// Push
pub fn push<T: ToWord>(&mut self, n: u8, value: T) -> &mut Self {
if n == 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually not needed.

axic
axic commented Apr 16, 2023
View reviewed changes
zkevm-circuits/src/evm_circuit/execution/push0.rs

fn configure(cb: &mut ConstraintBuilder<F>) -> Self {
// Push the value to the stack
cb.stack_push(Constant(F::from(0u64)));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I guess this would suffice too:

Suggested change
cb.stack_push(Constant(F::from(0u64)));
cb.stack_push(0.expr());

Seen both patterns in the codebase.

@lispc lispc mentioned this pull request Apr 16, 2023
Open
@axic axic mentioned this pull request Apr 16, 2023
Merged
4 tasks
ed255 pushed a commit that referenced this pull request Apr 18, 2023
@axic
Update geth dependency from v1.10.18 to v1.11.5 (#1363)
3429f83 
### Description

Updates the geth dependency in geth-utils to latest release.

Pulled this out from #1361.

### Issue Link

Related to #1362.

### Type of change

- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] This change requires a documentation update

### Contents

- Update geth dependency (and ran `go mod tidy` to update
sub-dependencies)
- Update code bindings to struct changes

### Rationale

N/A

### How Has This Been Tested?

Using the testing suite in the repo.
@lispc
Copy link
Collaborator

lispc commented Apr 27, 2023

hi @axic is this considered finished? if yes then i will mark it as ready for review and try ci.

@ChihChengLiang
Copy link
Collaborator

@lispc I think we can work on our own PR. Feel free to create one.
As @axic wrote in the description

This is a PR for discussion and not meant to be merged

This was referenced May 22, 2023
Merged
Merged
ed255 pushed a commit that referenced this pull request Jun 2, 2023
@silathdiir @lispc
feat: add shanghai feature and make CI pass with Shanghai geth trac…
de9f845 
…es (#1424)

### Description

1. Add Shanghai related fields to chain config in geth-utils.

2. EIP-3651 (Warm COINBASE): add a new access-list write for coinbase to
Begin TX.

3. Part of EIP-3860 (Limit and meter initcode): only add gas cost of
init code to Begin TX (missing gas cost changes in Create and OOG
Create).

### Issue Link

Issue
#1362

Local related PRs:
scroll-tech#497
scroll-tech#500
scroll-tech#507

Reference previous PR:
#1361

### Type of change

- [x] Breaking change (fix or feature that would cause existing
functionality to not work as expected)

### How Has This Been Tested?

Unit test cases of CI could pass with Shanghai geth traces.

---------

Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
@silathdiir silathdiir mentioned this pull request Jun 8, 2023
Merged
1 task
@KimiWu123 KimiWu123 mentioned this pull request Jul 7, 2023
Closed
@KimiWu123 KimiWu123 added this to the Feature Completeness milestone Jul 7, 2023
@KimiWu123 KimiWu123 mentioned this pull request Jul 7, 2023
Closed
@ed255
Copy link
Member

ed255 commented Jul 13, 2023

Closing this PR in favor of #1463

@ed255 ed255 closed this Jul 13, 2023
@ChihChengLiang
Copy link
Collaborator

Thank you @axic for the contribution and demonstrating the idea!

johntaiko pushed a commit to johntaiko/zkevm-circuits that referenced this pull request Aug 20, 2023
@silathdiir @lispc @johntaiko
feat: add shanghai feature and make CI pass with Shanghai geth trac…
b3edabc 
…es (privacy-scaling-explorations#1424)

1. Add Shanghai related fields to chain config in geth-utils.

2. EIP-3651 (Warm COINBASE): add a new access-list write for coinbase to
Begin TX.

3. Part of EIP-3860 (Limit and meter initcode): only add gas cost of
init code to Begin TX (missing gas cost changes in Create and OOG
Create).

Issue
privacy-scaling-explorations#1362

Local related PRs:
scroll-tech#497
scroll-tech#500
scroll-tech#507

Reference previous PR:
privacy-scaling-explorations#1361

- [x] Breaking change (fix or feature that would cause existing
functionality to not work as expected)

Unit test cases of CI could pass with Shanghai geth traces.

---------

Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
johntaiko pushed a commit to johntaiko/zkevm-circuits that referenced this pull request Aug 20, 2023
@silathdiir @lispc @johntaiko
feat: add shanghai feature and make CI pass with Shanghai geth trac…
e3451a5 
…es (privacy-scaling-explorations#1424)

1. Add Shanghai related fields to chain config in geth-utils.

2. EIP-3651 (Warm COINBASE): add a new access-list write for coinbase to
Begin TX.

3. Part of EIP-3860 (Limit and meter initcode): only add gas cost of
init code to Begin TX (missing gas cost changes in Create and OOG
Create).

Issue
privacy-scaling-explorations#1362

Local related PRs:
scroll-tech#497
scroll-tech#500
scroll-tech#507

Reference previous PR:
privacy-scaling-explorations#1361

- [x] Breaking change (fix or feature that would cause existing
functionality to not work as expected)

Unit test cases of CI could pass with Shanghai geth traces.

---------

Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
johntaiko pushed a commit to johntaiko/zkevm-circuits that referenced this pull request Aug 20, 2023
@silathdiir @lispc @johntaiko
feat: add shanghai feature and make CI pass with Shanghai geth trac…
2196cf8 
…es (privacy-scaling-explorations#1424)

1. Add Shanghai related fields to chain config in geth-utils.

2. EIP-3651 (Warm COINBASE): add a new access-list write for coinbase to
Begin TX.

3. Part of EIP-3860 (Limit and meter initcode): only add gas cost of
init code to Begin TX (missing gas cost changes in Create and OOG
Create).

Issue
privacy-scaling-explorations#1362

Local related PRs:
scroll-tech#497
scroll-tech#500
scroll-tech#507

Reference previous PR:
privacy-scaling-explorations#1361

- [x] Breaking change (fix or feature that would cause existing
functionality to not work as expected)

Unit test cases of CI could pass with Shanghai geth traces.

---------

Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
johntaiko pushed a commit to johntaiko/zkevm-circuits that referenced this pull request Aug 20, 2023
@silathdiir @lispc @johntaiko
feat: add shanghai feature and make CI pass with Shanghai geth trac…
46c5125 
…es (privacy-scaling-explorations#1424)

1. Add Shanghai related fields to chain config in geth-utils.

2. EIP-3651 (Warm COINBASE): add a new access-list write for coinbase to
Begin TX.

3. Part of EIP-3860 (Limit and meter initcode): only add gas cost of
init code to Begin TX (missing gas cost changes in Create and OOG
Create).

Issue
privacy-scaling-explorations#1362

Local related PRs:
scroll-tech#497
scroll-tech#500
scroll-tech#507

Reference previous PR:
privacy-scaling-explorations#1361

- [x] Breaking change (fix or feature that would cause existing
functionality to not work as expected)

Unit test cases of CI could pass with Shanghai geth traces.

---------

Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
github-merge-queue bot pushed a commit that referenced this pull request Aug 23, 2023
@silathdiir @ChihChengLiang
Implement PUSH0 instruction (#1463)
96b3c4e 
### 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

- [x] New feature (non-breaking change which adds functionality)

---------

Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
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-geth-utils Issues related to the geth-utils workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
zkEVM Community Edition
Status: ✅ Done
Milestone
Feature Completeness
Development

Successfully merging this pull request may close these issues.
5 participants
@axic @lispc @ChihChengLiang @ed255 @KimiWu123