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

Fix EIP2929 #1124

Merged
merged 21 commits into from
Feb 23, 2021
Merged

Fix EIP2929 #1124

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
bdf73e7
vm: stateManager: add EIP2929 access tracker
jochem-brouwer Feb 21, 2021
acd608c
vm: add origin/to/precompiles to warm addresses
jochem-brouwer Feb 21, 2021
212f4cd
vm: update EIP2929 to use StateManager
jochem-brouwer Feb 21, 2021
2eeddab
vm: fix build errors
jochem-brouwer Feb 21, 2021
3d9dcbc
vm: fix EIP2929 StateManager bug
jochem-brouwer Feb 22, 2021
dfadddf
vm: clean EIP2929 code
jochem-brouwer Feb 22, 2021
b40432b
vm: make EIP2929 use runTx
jochem-brouwer Feb 22, 2021
488d392
vm: add EIP2929 tests
jochem-brouwer Feb 22, 2021
99c0253
Merge branch 'master' into fix-eip2929
holgerd77 Feb 22, 2021
f4bdf9b
vm -> EIPs: replaced eips().includes() checks with HF-including commo…
holgerd77 Feb 22, 2021
37d6c8c
common: added EIP-2565 and EIP-2929 to berlin HF definition file
holgerd77 Feb 22, 2021
f8fed0c
vm -> testing: added Berlin to the HFs being tested by default on CI
holgerd77 Feb 22, 2021
dc3d90d
Merge pull request #1126 from ethereumjs/common-update-berlin
jochem-brouwer Feb 22, 2021
e34982a
vm: fix api tests
jochem-brouwer Feb 22, 2021
0826286
vm: fix CREATE/CREATE2 on EIP2929
jochem-brouwer Feb 22, 2021
715fcb1
vm: fix EIP2929 delegatecall bug
jochem-brouwer Feb 22, 2021
5772bf5
common: fix EIP2200 gas costs for EIP2929
jochem-brouwer Feb 22, 2021
21faebc
Merge branch 'master' into fix-eip2929
holgerd77 Feb 23, 2021
2e7c765
vm -> debug: keep opcode loggers uppercase, added new vm:eei:gas logger
holgerd77 Feb 23, 2021
231b071
common: set EIP2929-related op gas costs to 0
jochem-brouwer Feb 23, 2021
d67e8bd
CI: add MuirGlacier
jochem-brouwer Feb 23, 2021
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
44 changes: 40 additions & 4 deletions packages/common/src/eips/2929.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@
"v": 2900,
"d": "Once per SSTORE operation from clean non-zero to something else"
},
"sload": {
"v": 100,
"d": "Base fee of the SLOAD opcode"
},
"sstoreNoopGasEIP2200": {
"v": 100,
"d": "Once per SSTORE operation if the value doesn't change"
Expand All @@ -41,6 +37,46 @@
"sstoreCleanRefundEIP2200": {
"v": 4900,
"d": "Once per SSTORE operation for resetting to the original non-zero value"
},
"call": {
"v": 0,
"d": "Base fee of the CALL opcode"
},
"callcode": {
"v": 0,
"d": "Base fee of the CALLCODE opcode"
},
"delegatecall": {
"v": 0,
"d": "Base fee of the DELEGATECALL opcode"
},
"staticcall": {
"v": 0,
"d": "Base fee of the STATICCALL opcode"
},
"balance": {
"v": 0,
"d": "Base fee of the BALANCE opcode"
},
"extcodesize": {
"v": 0,
"d": "Base fee of the EXTCODESIZE opcode"
},
"extcodecopy": {
"v": 0,
"d": "Base fee of the EXTCODECOPY opcode"
},
"extcodehash": {
"v": 0,
"d": "Base fee of the EXTCODEHASH opcode"
},
"sload": {
"v": 0,
"d": "Base fee of the SLOAD opcode"
},
"sstore": {
"v": 0,
"d": "Base fee of the SSTORE opcode"
}
},
"vm": {},
Expand Down
20 changes: 12 additions & 8 deletions packages/vm/lib/evm/opcodes/EIP2929.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@ import { RunState } from './../interpreter'
* @param {RunState} runState
* @param {BN} address
*/
export function accessAddressEIP2929(runState: RunState, address: Address, baseFee?: number) {
export function accessAddressEIP2929(
runState: RunState,
address: Address,
chargeGas = true,
isSelfdestruct = false
) {
if (!runState._common.isActivatedEIP(2929)) return

const addressStr = address.buf
Expand All @@ -22,16 +27,16 @@ export function accessAddressEIP2929(runState: RunState, address: Address, baseF

// CREATE, CREATE2 opcodes have the address warmed for free.
// selfdestruct beneficiary address reads are charged an *additional* cold access
if (baseFee !== undefined) {
if (chargeGas) {
runState.eei.useGas(
new BN(runState._common.param('gasPrices', 'coldaccountaccess') - baseFee),
new BN(runState._common.param('gasPrices', 'coldaccountaccess')),
'EIP-2929 -> coldaccountaccess'
)
}
// Warm: (selfdestruct beneficiary address reads are not charged when warm)
} else if (baseFee !== undefined && baseFee > 0) {
} else if (chargeGas && !isSelfdestruct) {
Copy link
Member

Choose a reason for hiding this comment

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

That's a lot better than these implicit assumptions from before! 👍

runState.eei.useGas(
new BN(runState._common.param('gasPrices', 'warmstorageread') - baseFee),
new BN(runState._common.param('gasPrices', 'warmstorageread')),
'EIP-2929 -> warmstorageread'
)
}
Expand All @@ -47,7 +52,6 @@ export function accessAddressEIP2929(runState: RunState, address: Address, baseF
export function accessStorageEIP2929(runState: RunState, key: Buffer, isSstore: boolean) {
if (!runState._common.isActivatedEIP(2929)) return

const baseFee = !isSstore ? runState._common.param('gasPrices', 'sload') : 0
const address = runState.eei.getAddress().buf

const slotIsCold = !(<EIP2929StateManager>runState.stateManager).isWarmedStorage(address, key)
Copy link
Member

Choose a reason for hiding this comment

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

Nice and thought-through solution, cool! 👍

Expand All @@ -57,12 +61,12 @@ export function accessStorageEIP2929(runState: RunState, key: Buffer, isSstore:
// eslint-disable-next-line prettier/prettier
(<EIP2929StateManager>runState.stateManager).addWarmedStorage(address, key)
runState.eei.useGas(
new BN(runState._common.param('gasPrices', 'coldsload') - baseFee),
new BN(runState._common.param('gasPrices', 'coldsload')),
'EIP-2929 -> coldsload'
)
} else if (!isSstore) {
runState.eei.useGas(
new BN(runState._common.param('gasPrices', 'warmstorageread') - baseFee),
new BN(runState._common.param('gasPrices', 'warmstorageread')),
'EIP-2929 -> warmstorageread'
)
}
Expand Down
23 changes: 12 additions & 11 deletions packages/vm/lib/evm/opcodes/functions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ export const handlers: Map<number, OpHandler> = new Map([
async function (runState: RunState) {
const addressBN = runState.stack.pop()
const address = new Address(addressToBuffer(addressBN))
accessAddressEIP2929(runState, address, runState._common.param('gasPrices', 'balance'))
accessAddressEIP2929(runState, address)
const balance = await runState.eei.getExternalBalance(address)
runState.stack.push(balance)
},
Expand Down Expand Up @@ -514,7 +514,7 @@ export const handlers: Map<number, OpHandler> = new Map([
async function (runState: RunState) {
const addressBN = runState.stack.pop()
const address = new Address(addressToBuffer(addressBN))
accessAddressEIP2929(runState, address, runState._common.param('gasPrices', 'extcodesize'))
accessAddressEIP2929(runState, address)
const size = await runState.eei.getExternalCodeSize(addressBN)
runState.stack.push(size)
},
Expand All @@ -528,7 +528,7 @@ export const handlers: Map<number, OpHandler> = new Map([
// FIXME: for some reason this must come before subGas
subMemUsage(runState, memOffset, length)
const address = new Address(addressToBuffer(addressBN))
accessAddressEIP2929(runState, address, runState._common.param('gasPrices', 'extcodecopy'))
accessAddressEIP2929(runState, address)
// copy fee
runState.eei.useGas(
new BN(runState._common.param('gasPrices', 'copy')).imul(divCeil(length, new BN(32))),
Expand All @@ -550,7 +550,7 @@ export const handlers: Map<number, OpHandler> = new Map([
async function (runState: RunState) {
const addressBN = runState.stack.pop()
const address = new Address(addressToBuffer(addressBN))
accessAddressEIP2929(runState, address, runState._common.param('gasPrices', 'extcodehash'))
accessAddressEIP2929(runState, address)
const empty = await runState.eei.isAccountEmpty(address)
if (empty) {
runState.stack.push(new BN(0))
Expand Down Expand Up @@ -940,6 +940,8 @@ export const handlers: Map<number, OpHandler> = new Map([

const [value, offset, length] = runState.stack.popN(3)

accessAddressEIP2929(runState, runState.eei.getAddress(), false)

Copy link
Member

Choose a reason for hiding this comment

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

Just as some post-review question: is the order with subMemUsage() relevant here? Since in all the other cases this is the other way around?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, and also a bit sloppy of me. But I just explicitly checked; the memory gas cost is deterministic and does not depend on the current gas available, so the order does not matter. I don't really want to trigger a new CI run, so I will merge here.

subMemUsage(runState, offset, length)
let gasLimit = new BN(runState.eei.getGasLeft())
gasLimit = maxCallGas(gasLimit, runState.eei.getGasLeft(), runState)
Expand All @@ -949,7 +951,6 @@ export const handlers: Map<number, OpHandler> = new Map([
data = runState.memory.read(offset.toNumber(), length.toNumber())
}

accessAddressEIP2929(runState, runState.eei.getAddress())
const ret = await runState.eei.create(gasLimit, value, data)
runState.stack.push(ret)
},
Expand All @@ -965,7 +966,7 @@ export const handlers: Map<number, OpHandler> = new Map([
const [value, offset, length, salt] = runState.stack.popN(4)

subMemUsage(runState, offset, length)
accessAddressEIP2929(runState, runState.eei.getAddress())
accessAddressEIP2929(runState, runState.eei.getAddress(), false)

// Deduct gas costs for hashing
runState.eei.useGas(
Expand Down Expand Up @@ -1009,7 +1010,7 @@ export const handlers: Map<number, OpHandler> = new Map([
}
subMemUsage(runState, inOffset, inLength)
subMemUsage(runState, outOffset, outLength)
accessAddressEIP2929(runState, toAddress, runState._common.param('gasPrices', 'call'))
accessAddressEIP2929(runState, toAddress)

if (!value.isZero()) {
runState.eei.useGas(
Expand Down Expand Up @@ -1076,7 +1077,7 @@ export const handlers: Map<number, OpHandler> = new Map([

subMemUsage(runState, inOffset, inLength)
subMemUsage(runState, outOffset, outLength)
accessAddressEIP2929(runState, toAddress, runState._common.param('gasPrices', 'callcode'))
accessAddressEIP2929(runState, toAddress)

if (!value.isZero()) {
runState.eei.useGas(
Expand Down Expand Up @@ -1123,7 +1124,7 @@ export const handlers: Map<number, OpHandler> = new Map([

subMemUsage(runState, inOffset, inLength)
subMemUsage(runState, outOffset, outLength)
accessAddressEIP2929(runState, toAddress, runState._common.param('gasPrices', 'delegatecall'))
accessAddressEIP2929(runState, toAddress)
const gasLimit = maxCallGas(currentGasLimit, runState.eei.getGasLeft(), runState)
// note that TangerineWhistle or later this cannot happen (it could have ran out of gas prior to getting here though)
if (gasLimit.gt(runState.eei.getGasLeft())) {
Expand Down Expand Up @@ -1158,7 +1159,7 @@ export const handlers: Map<number, OpHandler> = new Map([

subMemUsage(runState, inOffset, inLength)
subMemUsage(runState, outOffset, outLength)
accessAddressEIP2929(runState, toAddress, runState._common.param('gasPrices', 'staticcall'))
accessAddressEIP2929(runState, toAddress)
const gasLimit = maxCallGas(currentGasLimit, runState.eei.getGasLeft(), runState) // we set TangerineWhistle or later to true here, as STATICCALL was available from Byzantium (which is after TangerineWhistle)

let data = Buffer.alloc(0)
Expand Down Expand Up @@ -1235,7 +1236,7 @@ export const handlers: Map<number, OpHandler> = new Map([
)
}

accessAddressEIP2929(runState, selfdestructToAddress, 0)
accessAddressEIP2929(runState, selfdestructToAddress, true, true)
return runState.eei.selfDestruct(selfdestructToAddress)
},
],
Expand Down