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

Conversation

silathdiir
Copy link
Contributor

@silathdiir silathdiir commented May 22, 2023

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

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

@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-external-tracer Issues related to the external-tracer workspace member crate-geth-utils Issues related to the geth-utils workspace member crate-mock Issues related to the mock workspace member labels May 22, 2023
@silathdiir silathdiir marked this pull request as draft May 22, 2023 06:30
@silathdiir silathdiir changed the title feat: add shanghai feature and support it in geth tracer [WIP] feat: add shanghai feature and support it in geth tracer May 22, 2023
@github-actions github-actions bot added crate-eth-types Issues related to the eth-types workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels May 22, 2023
@silathdiir silathdiir marked this pull request as ready for review May 22, 2023 09:51
@silathdiir silathdiir changed the title [WIP] feat: add shanghai feature and support it in geth tracer feat: add shanghai feature and make CI pass with it May 22, 2023
@silathdiir silathdiir changed the title feat: add shanghai feature and make CI pass with it feat: add shanghai feature and make CI pass with Shanghai geth traces May 22, 2023
Copy link
Member

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

The implementation looks good to me but I would like to discuss the feature approach.
So far we haven't introduced any rust feature to support new hard forks, we just implement the latest EIPs included. This PR introduces a feature so that the zkevm can be built with/without shanghai support. In which cases do we want to built without shanghai?
The code would look simpler without the rust feature, having shanghai implemented unconditionally. Could you give some thoughts on why implement this under a rust feature?

@silathdiir
Copy link
Contributor Author

The implementation looks good to me but I would like to discuss the feature approach. So far we haven't introduced any rust feature to support new hard forks, we just implement the latest EIPs included. This PR introduces a feature so that the zkevm can be built with/without shanghai support. In which cases do we want to built without shanghai? The code would look simpler without the rust feature, having shanghai implemented unconditionally. Could you give some thoughts on why implement this under a rust feature?

Yes. I see. I will delete related feature guard of shanghai (and update some code) in this PR. Thanks.

When started to implement shanghai EIPs, we also supposed to upgrade testool, and could not confirm the difference between shanghai and not-shanghai.

After finishing shanghai EIPs, we upgraded testool and run for both shanghai and not-shanghai via this feature guard. And it seems to be not much different.

with-shanghai (24 Panic):
https://circuit-release.s3.us-west-2.amazonaws.com/testool/default.1684984242.79c0291.html
without-shanghai (25 Panic):
https://circuit-release.s3.us-west-2.amazonaws.com/testool/default.1684984542.79c0291.html

@github-actions github-actions bot removed the crate-mock Issues related to the mock workspace member label Jun 1, 2023
@silathdiir silathdiir requested a review from ed255 June 1, 2023 03:11
@silathdiir
Copy link
Contributor Author

Hi @ed255, I deleted the related code of shanghai feature in commit-3d02ed3, please help review again when you have time. Thanks.

Copy link
Member

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

LGTM! Thanks for working on this!

@ed255
Copy link
Member

ed255 commented Jun 2, 2023

This is ready to be merged once the branch is rebased against main.

@ed255 ed255 added this pull request to the merge queue Jun 2, 2023
Merged via the queue into privacy-scaling-explorations:main with commit de9f845 Jun 2, 2023
@silathdiir silathdiir deleted the feat/add-shanghai-feature-and-support-in-geth-tracer branch June 2, 2023 11:36
johntaiko pushed a commit to johntaiko/zkevm-circuits that referenced this pull request Aug 20, 2023
…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 <[email protected]>
johntaiko pushed a commit to johntaiko/zkevm-circuits that referenced this pull request Aug 20, 2023
…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 <[email protected]>
johntaiko pushed a commit to johntaiko/zkevm-circuits that referenced this pull request Aug 20, 2023
…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 <[email protected]>
johntaiko pushed a commit to johntaiko/zkevm-circuits that referenced this pull request Aug 20, 2023
…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 <[email protected]>
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-eth-types Issues related to the eth-types workspace member crate-external-tracer Issues related to the external-tracer 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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants