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

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

Merged
merged 14 commits into from
Mar 9, 2022
Merged

Conversation

jochem-brouwer
Copy link
Member

@jochem-brouwer jochem-brouwer commented Dec 29, 2021

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
Copy link

codecov bot commented Dec 29, 2021

Codecov Report

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

Impacted file tree graph

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
Copy link
Member Author

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
Copy link
Member

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

@jochem-brouwer
Copy link
Member Author

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

Base automatically changed from tests3855 to master February 1, 2022 16:13
@jochem-brouwer
Copy link
Member Author

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
Copy link
Member Author

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

@holgerd77
Copy link
Member

So would you regard this as ready for review?

@jochem-brouwer
Copy link
Member Author

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
Copy link
Member

@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
Copy link
Member

axic commented Feb 15, 2022

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
Copy link
Member Author

@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
Copy link
Member

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

@holgerd77
Copy link
Member

(or: review and merge)

@jochem-brouwer
Copy link
Member Author

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
Copy link
Member

axic commented Feb 15, 2022

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
Copy link
Member Author

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
Copy link
Member

axic commented Feb 16, 2022

Pinging @gumb0 about this 😬

@gumb0
Copy link

gumb0 commented Feb 16, 2022

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
Copy link
Member Author

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
Copy link
Member Author

jochem-brouwer commented Mar 2, 2022

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)

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

if (this.common.isActivatedEIP(3860)) {
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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed this

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

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

Thanks Jochem! 🙂 Cool! 👍

@holgerd77
Copy link
Member

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

@holgerd77 holgerd77 changed the title Implement EIP3860 "limit and meter initcode" VM, Tx, Common: Implement EIP3860 "limit and meter initcode" Mar 9, 2022
@holgerd77 holgerd77 merged commit 8d1fb2e into master Mar 9, 2022
@holgerd77 holgerd77 deleted the eip3860 branch March 9, 2022 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants