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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a149785
vm/tests: add support for test networks with activated EIPs
jochem-brouwer Dec 28, 2021
a21c434
vm/tests: remove console.log
jochem-brouwer Dec 28, 2021
5487b01
tx/common: start of EIP3860 implementation
jochem-brouwer Dec 29, 2021
1dd5687
tx: add eip3860
jochem-brouwer Dec 30, 2021
ab73256
vm: add eip3860
jochem-brouwer Dec 30, 2021
91f07a6
Merge branch 'master' into eip3860
jochem-brouwer Feb 8, 2022
b0d8b5b
vm: fix eip3860 tests
jochem-brouwer Feb 8, 2022
3a67bd4
vm: update eip3860 to latest spec
jochem-brouwer Feb 9, 2022
455d41e
Merge branch 'master' into eip3860
jochem-brouwer Mar 2, 2022
e566dc2
tx: add 3860 tests + fix
jochem-brouwer Mar 2, 2022
26a9ae3
tx: move checks to util
jochem-brouwer Mar 7, 2022
f3cf8f6
Add test for exceeds max initcode
acolytec3 Mar 8, 2022
ddf627b
Merge remote-tracking branch 'origin/master' into eip3860
acolytec3 Mar 8, 2022
ba157f2
Merge branch 'master' into eip3860
holgerd77 Mar 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions packages/tx/src/eip1559Transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,16 @@ export default class FeeMarketEIP1559Transaction extends BaseTransaction<FeeMark
throw new Error(msg)
}

if (this.common.isActivatedEIP(3860)) {
if (this.data.length > this.common.param('vm', 'maxInitCodeSize')) {
throw new Error(
`the initcode size of this transaction is too large: it is ${
this.data.length
} while the max is ${this.common.param('vm', 'maxInitCodeSize')}`
)
}
}

const freeze = opts?.freeze ?? true
if (freeze) {
Object.freeze(this)
Expand Down
10 changes: 10 additions & 0 deletions packages/tx/src/eip2930Transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,16 @@ export default class AccessListEIP2930Transaction extends BaseTransaction<Access
throw new Error(msg)
}

if (this.common.isActivatedEIP(3860)) {
if (this.data.length > this.common.param('vm', 'maxInitCodeSize')) {
throw new Error(
`the initcode size of this transaction is too large: it is ${
this.data.length
} while the max is ${this.common.param('vm', 'maxInitCodeSize')}`
)
}
}

const freeze = opts?.freeze ?? true
if (freeze) {
Object.freeze(this)
Expand Down
10 changes: 10 additions & 0 deletions packages/tx/src/legacyTransaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

if (this.data.length > this.common.param('vm', 'maxInitCodeSize')) {
throw new Error(
`the initcode size of this transaction is too large: it is ${
this.data.length
} while the max is ${this.common.param('vm', 'maxInitCodeSize')}`
)
}
}

const freeze = opts?.freeze ?? true
if (freeze) {
Object.freeze(this)
Expand Down
13 changes: 13 additions & 0 deletions packages/tx/test/transactionRunner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const argv = minimist(process.argv.slice(2))
const file: string | undefined = argv.file

const forkNames: ForkName[] = [
'London+3860',
'London',
'Berlin',
'Istanbul',
Expand All @@ -23,6 +24,7 @@ const forkNames: ForkName[] = [
]

const forkNameMap: ForkNamesMap = {
'London+3860': 'london',
London: 'london',
Berlin: 'berlin',
Istanbul: 'istanbul',
Expand All @@ -35,6 +37,10 @@ const forkNameMap: ForkNamesMap = {
Homestead: 'homestead',
}

const EIPs: any = {
'London+3860': [3860],
}

tape('TransactionTests', async (t) => {
const fileFilterRegex = file ? new RegExp(file + '[^\\w]') : undefined
await getTests(
Expand All @@ -46,13 +52,20 @@ tape('TransactionTests', async (t) => {
) => {
t.test(testName, (st) => {
for (const forkName of forkNames) {
if (testData.result[forkName] === undefined) {
continue
}
const forkTestData = testData.result[forkName]
const shouldBeInvalid = !!forkTestData.exception

try {
const rawTx = toBuffer(testData.txbytes)
const hardfork = forkNameMap[forkName]
const common = new Common({ chain: 1, hardfork })
const activateEIPs = EIPs[forkName]
if (activateEIPs) {
common.setEIPs(activateEIPs)
}
const tx = Transaction.fromSerializedTx(rawTx, { common })
const sender = tx.getSenderAddress().toString()
const hash = tx.hash().toString('hex')
Expand Down
1 change: 1 addition & 0 deletions packages/tx/test/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
export type ForkName =
| 'London+3860'
| 'London'
| 'Berlin'
| 'Istanbul'
Expand Down