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

feat: support shanghai hardfork #4272

Merged
merged 104 commits into from
Apr 12, 2023
Merged

feat: support shanghai hardfork #4272

merged 104 commits into from
Apr 12, 2023

Conversation

davidmurdoch
Copy link
Member

@davidmurdoch davidmurdoch commented Mar 2, 2023

  • EIP-3651: Warm COINBASE
    • Needed to handle warming ourselves in simulateTransaction (eth_call). A new test was added for this.
  • EIP-3855: PUSH0 instruction
    • No changes required
  • EIP-3860: Limit and meter initcode
    • We've added a new option allowUnlimitedInitCodeSize to disable this EIP to continue to allow for uploading very large contracts. In most cases it will need to be combined with the allowUnlimitedContractSize option. Two new tests were added for this.
  • EIP-4895: Beacon chain push withdrawals as operations
    • Needed to add new fields to Block: withdrawals and withdrawalsRoot.
  • EIP-6049: Deprecate SELFDESTRUCT
    • No changes required. We could technically hook into the EVM steps and console.log a deprecation notice if the SELFDESTRUCT opcode is hit, but I feel like this sort of warning should happen at compilation, and not by a dev net client like Ganache. Also, maintaining the deprecation notice forever would suck. 🤔

This also fixes two bugs:

  • block size was not computed correctly (persisted databases will be updated and migrated automatically)
    • the database migration may add a one-time delay to startup when using a persistent database (--database.dbPath). This should be insignificant for the majority of cases. The migration was benchmarked with 180000 blocks, and completed in 6.4 seconds.
  • internal "intrinsic gas" checks now correctly include accessList fees, if there are any.

@davidmurdoch davidmurdoch marked this pull request as ready for review March 8, 2023 16:17
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Mar 14, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7ffc5c3
Status: ✅  Deploy successful!
Preview URL: https://cb5ba1b5.ganache.pages.dev
Branch Preview URL: https://update-ethereumjs.ganache.pages.dev

View logs

@davidmurdoch davidmurdoch changed the title feat: support shanghai hardfork feat: support shanghai hardfork Mar 15, 2023
@davidmurdoch
Copy link
Member Author

Note: CI failed due to time outs several times. I'm concerned that this PR introduces a performance regression and need to test Test timings in a more controlled manner (locally) before merging.

davidmurdoch and others added 2 commits April 11, 2023 10:43
uint256 balance = address(block.coinbase).balance;
require(balance != 0, "coinbase balance is not zero");
return gasleft();
Copy link
Contributor

Choose a reason for hiding this comment

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

accessCoinBase might be better, but it doesn't hint at what the return value is 🤷

Comment on lines +1387 to +1390
// this test uses the `network: "goerli"` option, which requires an
// infura key; when run our tests it must be provided as an environment
// variable.
if (!process.env.INFURA_KEY) this.skip();
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we able to check if we are in CI and fail if the INFURA_KEY is not set?

Copy link
Member Author

@davidmurdoch davidmurdoch Apr 12, 2023

Choose a reason for hiding this comment

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

Yes, github sets an env var we could check. This is a pattern we copied from elsewhere to skip tests that a run without providing an Infura key.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a good future improvement :)

@@ -337,6 +338,128 @@ describe("provider", () => {
);
await provider.disconnect();
}).timeout(10000);

it("allows unlimited init code in transaction when the allowUnlimitedInitCodeSize option is set", async () => {
const largeInitCode = "0x" + "00".repeat(49153); // larger than init code allowance
Copy link
Contributor

Choose a reason for hiding this comment

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

Data.toString(0, 49153) would do the same 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like a reasonable change to me! I think this test was a copy+paste of a similar test that predates Data.toString , so I just didn't bother to change the format.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants