-
Notifications
You must be signed in to change notification settings - Fork 773
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
Changes from all commits
a149785
a21c434
5487b01
1dd5687
ab73256
91f07a6
b0d8b5b
3a67bd4
455d41e
e566dc2
26a9ae3
f3cf8f6
ddf627b
ba157f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
{ | ||
"name": "EIP-3860", | ||
"number": 3860, | ||
"comment": "Limit and meter initcode", | ||
"url": "https://eips.ethereum.org/EIPS/eip-3860", | ||
"status": "Review", | ||
"minimumHardfork": "spuriousDragon", | ||
"requiredEIPs": [], | ||
"gasConfig": {}, | ||
"gasPrices": { | ||
"initCodeWordCost": { | ||
"v": 2, | ||
"d": "Gas to pay for each word (32 bytes) of initcode when creating a contract" | ||
} | ||
}, | ||
"vm": { | ||
"maxInitCodeSize": { | ||
"v": 49152, | ||
"d": "Maximum length of initialization code when creating a contract" | ||
} | ||
}, | ||
"pow": {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
export type ForkName = | ||
| 'London+3860' | ||
| 'London' | ||
| 'Berlin' | ||
| 'Istanbul' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -298,6 +298,20 @@ export default class EVM { | |
// Reduce tx value from sender | ||
await this._reduceSenderBalance(account, message) | ||
|
||
if (this._vm._common.isActivatedEIP(3860)) { | ||
if (message.data.length > this._vm._common.param('vm', 'maxInitCodeSize')) { | ||
return { | ||
gasUsed: message.gasLimit, | ||
createdAddress: message.to, | ||
execResult: { | ||
returnValue: Buffer.alloc(0), | ||
exceptionError: new VmError(ERROR.INITCODE_SIZE_VIOLATION), | ||
gasUsed: message.gasLimit, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
}, | ||
} | ||
} | ||
} | ||
|
||
message.code = message.data | ||
message.data = Buffer.alloc(0) | ||
message.to = await this._generateAddress(message) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
import tape from 'tape' | ||
import VM from '../../../src' | ||
import Common, { Chain, Hardfork } from '@ethereumjs/common' | ||
import { FeeMarketEIP1559Transaction } from '@ethereumjs/tx' | ||
import { Address, BN, privateToAddress } from 'ethereumjs-util' | ||
const pkey = Buffer.from('20'.repeat(32), 'hex') | ||
const GWEI = new BN('1000000000') | ||
const sender = new Address(privateToAddress(pkey)) | ||
|
||
tape('EIP 3860 tests', (t) => { | ||
const common = new Common({ | ||
chain: Chain.Mainnet, | ||
hardfork: Hardfork.London, | ||
eips: [3860], | ||
}) | ||
|
||
t.test('EIP-3860 tests', async (st) => { | ||
const vm = new VM({ common }) | ||
const account = await vm.stateManager.getAccount(sender) | ||
const balance = GWEI.muln(21000).muln(10000000) | ||
account.balance = balance | ||
await vm.stateManager.putAccount(sender, account) | ||
|
||
const buffer = Buffer.allocUnsafe(1000000).fill(0x60) | ||
console.log(common.isActivatedEIP(3860)) | ||
const tx = FeeMarketEIP1559Transaction.fromTxData({ | ||
data: | ||
'0x7F6000020000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000060005260206000F3' + | ||
buffer.toString('hex'), | ||
gasLimit: 100000000000, | ||
maxFeePerGas: 7, | ||
nonce: 0, | ||
}).sign(pkey) | ||
const result = await vm.runTx({ tx }) | ||
st.ok( | ||
(result.execResult.exceptionError?.error as string) === 'initcode exceeds max initcode size', | ||
'initcode exceeds max size' | ||
) | ||
}) | ||
}) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 in
util.ts
called e.g.checkMaxInitCodeSize()
, then we can just keep the if isActivatedEIP(3860) and call the method inside.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed this