Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for the shanghai hardfork #3767

Merged
merged 17 commits into from
Apr 10, 2023
Merged

Add support for the shanghai hardfork #3767

merged 17 commits into from
Apr 10, 2023

Conversation

fvictorio
Copy link
Member

@fvictorio fvictorio commented Mar 16, 2023

Adds support for the shanghai hardfork. The two main changes are:

  • Syncing our fork of ethereumjs with upstream, and making some changes to make allowUnlimitedContractSize to work with the EIP-3860 initcode limit.
  • Add withdrawals and withdrawalsRoot fields to RPC blocks if Shanghai is enabled

@changeset-bot
Copy link

changeset-bot bot commented Mar 16, 2023

🦋 Changeset detected

Latest commit: fcb182d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
hardhat Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Mar 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
hardhat ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 22, 2023 at 0:22AM (UTC)
hardhat-storybook ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 22, 2023 at 0:22AM (UTC)

- london
- arrowGlacier
- grayGlacier
Copy link
Member Author

Choose a reason for hiding this comment

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

These two hardforks were missing in the list, so I just added them.

"@nomicfoundation/ethereumjs-trie": "5.0.4",
"@nomicfoundation/ethereumjs-tx": "4.1.1",
"@nomicfoundation/ethereumjs-util": "8.0.5",
"@nomicfoundation/ethereumjs-vm": "6.4.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

I used pinned versions this time (I should've done that the previous time). The fork also has pinned versions in their intra-dependencies.

@@ -63,7 +63,7 @@ export abstract class BlockchainBase {

public async getBlock(
blockHashOrNumber: Buffer | bigint | number
): Promise<Block | null> {
): Promise<Block> {
Copy link
Member Author

Choose a reason for hiding this comment

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

BlockchainInterface has this breaking change.

if (blockByNumber === undefined) {
throw new Error("Block not found");
}
return blockByNumber;
Copy link
Member Author

Choose a reason for hiding this comment

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

We now throw instead of returning null when the block doesn't exist.

@@ -48,7 +48,8 @@ export function serializeTransaction(

export function deserializeTransaction(
tx: SerializedTransaction,
common: Common
common: Common,
{ allowUnlimitedContractSize }: { allowUnlimitedContractSize: boolean }
Copy link
Member Author

Choose a reason for hiding this comment

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

A lot of the changes in this PR come from the fact that we need to pass the value of allowUnlimitedContractSize to be able to create transactions with the correct disableMaxInitCodeSizeCheck value.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a validation that is meant to be run only when receiving a new transaction, isn't it?

If that's the case we can run it in the RPC layer / Node, and use true everywhere else. WDYT?

Copy link
Member Author

@fvictorio fvictorio Mar 22, 2023

Choose a reason for hiding this comment

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

You mean always using disableMaxInitCodeSizeCheck: true, and making the max initcode check ourselves in an upper layer? Makes sense to me, I'll do that.

@@ -68,7 +72,7 @@ export class ReadOnlyValidEIP1559Transaction extends FeeMarketEIP1559Transaction
}
);

super(data, { freeze: false, common: fakeCommon });
super(data, { ...opts, freeze: false, common: fakeCommon });
Copy link
Member Author

Choose a reason for hiding this comment

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

This kind of change is needed to pass the disableMaxInitCodeSizeCheck flag around.

Copy link
Member

Choose a reason for hiding this comment

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

@@ -712,3 +712,38 @@ contract CallSelfDestruct {
},
topics: {},
};

export const EXAMPLE_TOUCH_ADDRESS_CONTRACT = {
Copy link
Member Author

Choose a reason for hiding this comment

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

This new example contract is used to test EIP-3651 (warm COINBASE)

assert.equal(await blockchain.getBlock(0), undefined);
assert.equal(await blockchain.getBlock(randomHashBuffer()), undefined);
it("throws error for non-existent block", async () => {
await assert.isRejected(blockchain.getBlock(0), "Block not found");
Copy link
Member Author

Choose a reason for hiding this comment

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

Some tests changed because getBlock now throws instead of returning null.

@@ -1532,4 +1537,289 @@ describe("Eth module - hardfork dependant tests", function () {
});
});
});

describe("shanghai hardfork", function () {
Copy link
Member Author

Choose a reason for hiding this comment

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

These are the most interesting tests, please review them carefully.

];

for (const { url, blockToRun, networkName, chainId } of forkedBlocks) {
const remoteCommon = new Common({ chain: chainId });
const hardfork = remoteCommon.getHardforkByBlockNumber(blockToRun);
Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't work for shanghai because getHardforkByBlockNumber needs the block timestamp to correctly detect that a block is in that hardfork. Now the hardfork is detected inside runFullBlock because we have fetched the block there and can pass the timestamp.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if I follow this, but the rest LGTM

@fvictorio fvictorio merged commit 7645946 into main Apr 10, 2023
@fvictorio fvictorio deleted the shanghai branch April 10, 2023 08:25
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants