VM, Tx, Common: Implement EIP3860 "limit and meter initcode" #1619

jochem-brouwer · 2021-12-29T21:37:30Z

Implements https://eips.ethereum.org/EIPS/eip-3860

Requires #1617

Note: EIP is in review but it is likely that some details get changed.

To test:

npm run build --workspaces
cd ./packages/ethereum-tests
git remote add ewasm git@github.com:ewasm/tests.git
git checkout ewasm/eip-3860
cd ../vm
npm run test:blockchain -- --fork=London+3860

Another note: the current tests do not match the EIP spec so at this point we cannot test it because the tests are invalid.

codecov · 2021-12-29T21:41:34Z

Codecov Report

Merging #1619 (ddf627b) into master (da0c698) will decrease coverage by 0.08%.
The diff coverage is 31.25%.

Flag	Coverage Δ
block	`85.57% <ø> (ø)`
blockchain	`83.28% <ø> (ø)`
client	`75.23% <ø> (+0.09%)`	⬆️
common	`93.90% <ø> (ø)`
devp2p	`82.63% <ø> (+0.26%)`	⬆️
ethash	`90.76% <ø> (ø)`
trie	`86.18% <ø> (ø)`
tx	`88.20% <36.84%> (-1.75%)`	⬇️
util	`92.62% <ø> (ø)`
vm	`80.95% <23.07%> (-0.25%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

jochem-brouwer · 2021-12-30T14:14:35Z

Errors thrown in file: GeneralStateTests/stCodeSizeLimit/create2InitCodeSizeLimit.json test: create2InitCodeSizeLimit_d0g0v0_London+3860:
	Error: invalid receiptTrie (vm hf=london -> block number=1 hash=0xbfbd043495ac98f0a3a03edcce1cafab3a3d2c752cdff090465f772f549fd102 hf=london baseFeePerGas=10 txs=1 uncles=0)
	correct last header block
Errors thrown in file: GeneralStateTests/stCodeSizeLimit/create2InitCodeSizeLimit.json test: create2InitCodeSizeLimit_d1g0v0_London+3860:
	Error: invalid receiptTrie (vm hf=london -> block number=1 hash=0x5bb4b3bf60d0dc6ea703640941bdbcc231028c05256edfe836655007821165b9 hf=london baseFeePerGas=10 txs=1 uncles=0)
	correct last header block
Errors thrown in file: GeneralStateTests/stCodeSizeLimit/createInitCodeSizeLimit.json test: createInitCodeSizeLimit_d0g0v0_London+3860:
	Error: invalid receiptTrie (vm hf=london -> block number=1 hash=0xe5437b836f3d7ace37d5716bac8f811e7ab55d4e6851e2e28f12fa4dd1460f9a hf=london baseFeePerGas=10 txs=1 uncles=0)
	correct last header block
Errors thrown in file: GeneralStateTests/stCodeSizeLimit/createInitCodeSizeLimit.json test: createInitCodeSizeLimit_d1g0v0_London+3860:
	Error: invalid receiptTrie (vm hf=london -> block number=1 hash=0x94ab4d95df530d9fad183d554aab9628a4314ba0511761ec3d7071127c229fba hf=london baseFeePerGas=10 txs=1 uncles=0)
	correct last header block

1..22
# tests 22
# pass  14
# fail  8

holgerd77 · 2022-01-28T11:27:02Z

Some reference: Transaction tests for EIP-3860 PR ethereum/tests#1012

jochem-brouwer · 2022-01-31T22:42:36Z

Ah those are new, thanks @holgerd77 😄 Seems like we can test this now!

jochem-brouwer · 2022-02-08T17:15:26Z

We should also require #1701 here due to the added TransactionTests.

Will continue here to see if I can figure out what's causing the tests to fail.

jochem-brouwer · 2022-02-09T16:26:00Z

Updated to latest version, tests pass on London+3860, on this tests commit: ethereum/tests@9a19e31

holgerd77 · 2022-02-11T17:15:25Z

So would you regard this as ready for review?

jochem-brouwer · 2022-02-14T18:22:10Z

We could do a preliminary review, the problem is that the EIP will most likely get changed (in particular gas costs), so I don't think we should merge it already.

holgerd77 · 2022-02-15T09:19:17Z

@jochem-brouwer ah ok, sorry, wasn't aware of that, then let's wait.

Just had a look: Go Ethereum PR is also still in a DRAFT state: ethereum/go-ethereum#23847

axic · 2022-02-15T11:09:27Z

the problem is that the EIP will most likely get changed (in particular gas costs)

@jochem-brouwer which part do you mean? Though in your implementation it seems everything relevant is in the config file (limits and gas costs), so it shouldn't be a big deal.

jochem-brouwer · 2022-02-15T13:56:57Z

@axic This comment: ethereum/tests#990 (comment)

The EIP spec (text) would thus get updated, but it seems that the tests already match what "the probable direction EIP spec will go", so in that case nothing would change in this implementation.

holgerd77 · 2022-02-15T14:07:00Z

@jochem-brouwer I guess since @axic is one of the EIP authors we can then relatively safely merge here?

holgerd77 · 2022-02-15T14:07:37Z

(or: review and merge)

jochem-brouwer · 2022-02-15T16:12:55Z

I just re-read the EIP and noticed that there is a check which I have not yet added (check that a contract creation tx has a calldata payload <= maxInitCodeSize. These are invalid in the same way as underpricing txs (sending eth to someone and having gasLimit less than 21000). I should also add TransactionTests (this one is also not yet merged in ethereum/tests) in the transaction tests and ensure they pass as well.

axic · 2022-02-15T20:11:41Z

I think the EIP and tests should be in sync now, and the test suite will only be merged once geth merges the code?

I do appreciate your work here quite a bit, as any second implementation can unearth questions, just like it did for you @jochem-brouwer.

jochem-brouwer · 2022-02-15T23:10:06Z

Hi @axic, the ethereum/tests PR is currently not in sync with the EIP spec.

This line from the EIP:

In these three cases the entire execution is exceptionally aborted.

In those cases, all gas in the frame is consumed, but in ethereum/tests only the gas cost of CREATE/CREATE2 will be consumed and 0 will be put on the stack.

I also agree with you on the second implementation thing - in fact, I think this should be considered for all EIPs which change execution logic. Second implementations will be a "check" to see if the EIP spec is not ambigous and if you can thus use the self-contained EIP to implement it. I will also start implementing other EIPs which are considered for Shanghai. It is both very fun to implement those, and it is also healthy for the ecosystem to ensure that the EIPs are correct 😄

axic · 2022-02-16T13:09:55Z

Pinging @gumb0 about this 😬

gumb0 · 2022-02-16T13:22:53Z

This line from the EIP:

In these three cases the entire execution is exceptionally aborted.

In those cases, all gas in the frame is consumed, but in ethereum/tests only the gas cost of CREATE/CREATE2 will be consumed and 0 will be put on the stack.

Wait, that is the line from Rationale, it refers to already existing (non-creation-specific) failure cases, and says new failures from this EIP don't do this.

(So I believe it should be in sync with tests now)

jochem-brouwer · 2022-02-16T13:34:56Z

Ah right, upon re-reading I agree that it indeed matches the current EIP spec. I do think that the addition of these "3 checks" is a bit confusing though (this lead me to believe I should spend all gas). For clarity it might be better to add a line like "in case the initcode limit is violated when trying to execute CREATE/CREATE2, then only the upfront gas is consumed: this is thus the base cost + memory expansion cost + initcode cost (+ hashing cost (CREATE2)), and 0 is put on the stack". I think the addition of mentioning the cases where all gas is consumed is not necessary and would only lead to confusion.

But it might also be my poor reading ability to lead to implement the "wrong" version 😅

@axic @gumb0

jochem-brouwer · 2022-03-02T21:09:38Z

To test TransactionTests:

cd ./packages/ethereum-tests
git remote add ipsilon git@github.com:ipsilon/tests.git
git fetch ipsilon
git checkout ipsilon/eip-3860-transactions
cd ../tx
npm run test -- -t

(They pass)

jochem-brouwer · 2022-03-02T21:10:37Z

packages/tx/src/legacyTransaction.ts

@@ -135,6 +135,16 @@ export default class Transaction extends BaseTransaction<Transaction> {
      }
    }

+    if (this.common.isActivatedEIP(3860)) {


This block is in all three transaction types, the reason is that we do not have access to common in baseTransaction. Adding it in baseTransaction is somewhat hard as especially legacyTransaction has some extra logic to extract chainId.

hm. i don't like how we have to keep duplicating code across the txs classes like this. but thanks for the reasoning why.

would be much better if we always write the code just once. what if we move it to a method inutil.ts called e.g. checkMaxInitCodeSize(), then we can just keep the if isActivatedEIP(3860) and call the method inside.

other than that, PR looks great! I'm not sure why the errors don't have any coverage on them though (I'm seeing codecov warnings), i assume the added TransactionTests should cover that. if not maybe we should add a few explicit tests for coverage.

Yep, great suggestion moving it to util! I also did not like it.

Addressed this

acolytec3 · 2022-03-08T01:13:20Z

packages/vm/src/evm/evm.ts

+          execResult: {
+            returnValue: Buffer.alloc(0),
+            exceptionError: new VmError(ERROR.INITCODE_SIZE_VIOLATION),
+            gasUsed: message.gasLimit,


When I read the EIP, it says that if a "create transaction exceeds MAX_INITCODE_SIZE, transaction is invalid." Does that mean that we do an exceptional abort and consume all gas or that the transaction is just rejected and not included in the block?

I went ahead and added a new EIP-3860 test file to the api/EIPs section and added a real basic test to trigger this error so we can bump test coverage.

It is the latter, I remember having the same question and it is answered somewhere, but I cannot find it again 😅 So, the block is invalid if you include such transaction and nodes will reject that broadcasted block.

holgerd77

Latest comments from @ryanio have been addressed, will approve and merge.

Thanks Jochem! 🙂 Cool! 👍

holgerd77 · 2022-03-09T13:04:44Z

Hmm, codecov is not reporting. 🤔 Will nevertheless merge here.

jochem-brouwer added 3 commits December 28, 2021 16:21

vm/tests: add support for test networks with activated EIPs

a149785

vm/tests: remove console.log

a21c434

tx/common: start of EIP3860 implementation

5487b01

jochem-brouwer added type: feature PR state: WIP PR/Issue state: blocked package: tx package: vm package: common labels Dec 29, 2021

jochem-brouwer added 2 commits December 30, 2021 15:12

tx: add eip3860

1dd5687

vm: add eip3860

ab73256

Base automatically changed from tests3855 to master February 1, 2022 16:13

Merge branch 'master' into eip3860

91f07a6

jochem-brouwer added 2 commits February 8, 2022 20:57

vm: fix eip3860 tests

b0d8b5b

vm: update eip3860 to latest spec

3a67bd4

acolytec3 mentioned this pull request Feb 25, 2022

VM, Common: Add EIP-3670 (EOF - Code Validation) #1743

Merged

jochem-brouwer added 2 commits March 2, 2022 21:32

Merge branch 'master' into eip3860

455d41e

tx: add 3860 tests + fix

e566dc2

jochem-brouwer commented Mar 2, 2022

View reviewed changes

jochem-brouwer added PR state: needs review and removed PR state: WIP labels Mar 2, 2022

jochem-brouwer requested review from holgerd77 and ryanio March 2, 2022 21:11

jochem-brouwer marked this pull request as ready for review March 2, 2022 21:11

tx: move checks to util

26a9ae3

acolytec3 reviewed Mar 8, 2022

View reviewed changes

acolytec3 and others added 3 commits March 7, 2022 20:37

Add test for exceeds max initcode

f3cf8f6

Merge remote-tracking branch 'origin/master' into eip3860

ddf627b

Merge branch 'master' into eip3860

ba157f2

holgerd77 approved these changes Mar 9, 2022

View reviewed changes

holgerd77 changed the title ~~Implement EIP3860 "limit and meter initcode"~~ VM, Tx, Common: Implement EIP3860 "limit and meter initcode" Mar 9, 2022

holgerd77 merged commit 8d1fb2e into master Mar 9, 2022

holgerd77 deleted the eip3860 branch March 9, 2022 13:05

yperbasis mentioned this pull request Sep 2, 2022

Update Erigon Shanghai status ethereum/execution-specs#596

Merged

holgerd77 mentioned this pull request Sep 20, 2022

Early Pre-Shanghai EthereumJS Ephemeral Testnet #2298

Closed

11 tasks

gakonst mentioned this pull request Feb 11, 2023

Tracking: Shanghai Support paradigmxyz/reth#1295

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VM, Tx, Common: Implement EIP3860 "limit and meter initcode" #1619

VM, Tx, Common: Implement EIP3860 "limit and meter initcode" #1619

jochem-brouwer commented Dec 29, 2021 •

edited

codecov bot commented Dec 29, 2021 •

edited

jochem-brouwer commented Dec 30, 2021

holgerd77 commented Jan 28, 2022

jochem-brouwer commented Jan 31, 2022

jochem-brouwer commented Feb 8, 2022

jochem-brouwer commented Feb 9, 2022

holgerd77 commented Feb 11, 2022

jochem-brouwer commented Feb 14, 2022

holgerd77 commented Feb 15, 2022

axic commented Feb 15, 2022

jochem-brouwer commented Feb 15, 2022

holgerd77 commented Feb 15, 2022

holgerd77 commented Feb 15, 2022

jochem-brouwer commented Feb 15, 2022

axic commented Feb 15, 2022

jochem-brouwer commented Feb 15, 2022

axic commented Feb 16, 2022

gumb0 commented Feb 16, 2022 •

edited

jochem-brouwer commented Feb 16, 2022

jochem-brouwer commented Mar 2, 2022 •

edited

jochem-brouwer Mar 2, 2022

ryanio Mar 3, 2022

ryanio Mar 3, 2022

jochem-brouwer Mar 6, 2022

jochem-brouwer Mar 7, 2022

acolytec3 Mar 8, 2022

acolytec3 Mar 8, 2022

jochem-brouwer Mar 9, 2022

holgerd77 left a comment

holgerd77 commented Mar 9, 2022

VM, Tx, Common: Implement EIP3860 "limit and meter initcode" #1619

VM, Tx, Common: Implement EIP3860 "limit and meter initcode" #1619

Conversation

jochem-brouwer commented Dec 29, 2021 • edited

codecov bot commented Dec 29, 2021 • edited

Codecov Report

jochem-brouwer commented Dec 30, 2021

holgerd77 commented Jan 28, 2022

jochem-brouwer commented Jan 31, 2022

jochem-brouwer commented Feb 8, 2022

jochem-brouwer commented Feb 9, 2022

holgerd77 commented Feb 11, 2022

jochem-brouwer commented Feb 14, 2022

holgerd77 commented Feb 15, 2022

axic commented Feb 15, 2022

jochem-brouwer commented Feb 15, 2022

holgerd77 commented Feb 15, 2022

holgerd77 commented Feb 15, 2022

jochem-brouwer commented Feb 15, 2022

axic commented Feb 15, 2022

jochem-brouwer commented Feb 15, 2022

axic commented Feb 16, 2022

gumb0 commented Feb 16, 2022 • edited

jochem-brouwer commented Feb 16, 2022

jochem-brouwer commented Mar 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 left a comment

Choose a reason for hiding this comment

holgerd77 commented Mar 9, 2022

jochem-brouwer commented Dec 29, 2021 •

edited

codecov bot commented Dec 29, 2021 •

edited

gumb0 commented Feb 16, 2022 •

edited

jochem-brouwer commented Mar 2, 2022 •

edited