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

Istanbul: EIP-2200 net gas metering for SSTORE #590

Merged
merged 11 commits into from
Sep 10, 2019
12 changes: 12 additions & 0 deletions lib/evm/eei.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,18 @@ export default class EEI {
this._result.gasRefund.iadd(amount)
}

/**
* Reduces amount of gas to be refunded by a positive value.
* @param amount - Amount to subtract from gas refunds
*/
subRefund(amount: BN): void {
this._result.gasRefund.isub(amount)
if (this._result.gasRefund.ltn(0)) {
this._result.gasRefund = new BN(0)
trap(ERROR.REFUND_EXHAUSTED)
}
}

/**
* Returns address of currently executing account.
*/
Expand Down
73 changes: 69 additions & 4 deletions lib/evm/opFns.ts
Original file line number Diff line number Diff line change
Expand Up @@ -912,7 +912,10 @@ function getContractStorage(runState: RunState, address: Buffer, key: Buffer) {
}
runState.stateManager.getContractStorage(address, key, (err: Error, current: Buffer) => {
if (err) return cb(err, null)
if (runState._common.hardfork() === 'constantinople') {
if (
runState._common.hardfork() === 'constantinople' ||
runState._common.gteHardfork('istanbul')
) {
runState.stateManager.getOriginalContractStorage(
address,
key,
Expand Down Expand Up @@ -956,9 +959,8 @@ function updateSstoreGas(runState: RunState, found: any, value: Buffer) {
// If original value is not 0
if (current.length === 0) {
// If current value is 0 (also means that new value is not 0), remove 15000 gas from refund counter. We can prove that refund counter will never go below 0.
// TODO: Remove usage of private attr
runState.eei._result.gasRefund = runState.eei._result.gasRefund.subn(
runState._common.param('gasPrices', 'netSstoreClearRefund'),
runState.eei._result.gasRefund.isub(
new BN(runState._common.param('gasPrices', 'netSstoreClearRefund')),
)
} else if (value.length === 0) {
// If new value is 0 (also means that current value is not 0), add 15000 gas to refund counter.
Expand All @@ -978,6 +980,69 @@ function updateSstoreGas(runState: RunState, found: any, value: Buffer) {
}
}
return runState.eei.useGas(new BN(runState._common.param('gasPrices', 'netSstoreDirtyGas')))
} else if (runState._common.gteHardfork('istanbul')) {
// EIP-2200
const original = found.original
const current = found.current
// Fail if not enough gas is left
if (
runState.eei.getGasLeft().lten(runState._common.param('gasPrices', 'sstoreSentryGasEIP2200'))
) {
trap(ERROR.OUT_OF_GAS)
}

// Noop
if (current.equals(value)) {
return runState.eei.useGas(
new BN(runState._common.param('gasPrices', 'sstoreNoopGasEIP2200')),
)
}
if (original.equals(current)) {
// Create slot
if (original.length === 0) {
return runState.eei.useGas(
new BN(runState._common.param('gasPrices', 'sstoreInitGasEIP2200')),
)
}
// Delete slot
if (value.length === 0) {
runState.eei.refundGas(
new BN(runState._common.param('gasPrices', 'sstoreClearRefundEIP2200')),
)
}
// Write existing slot
return runState.eei.useGas(
new BN(runState._common.param('gasPrices', 'sstoreCleanGasEIP2200')),
)
}
if (original.length > 0) {
if (current.length === 0) {
// Recreate slot
runState.eei.subRefund(
new BN(runState._common.param('gasPrices', 'sstoreClearRefundEIP2200')),
)
} else if (value.length === 0) {
// Delete slot
runState.eei.refundGas(
new BN(runState._common.param('gasPrices', 'sstoreClearRefundEIP2200')),
)
}
}
if (original.equals(value)) {
if (original.length === 0) {
// Reset to original non-existent slot
runState.eei.refundGas(
new BN(runState._common.param('gasPrices', 'sstoreInitRefundEIP2200')),
)
} else {
// Reset to original existing slot
runState.eei.refundGas(
new BN(runState._common.param('gasPrices', 'sstoreCleanRefundEIP2200')),
)
}
}
// Dirty update
return runState.eei.useGas(new BN(runState._common.param('gasPrices', 'sstoreDirtyGasEIP2200')))
} else {
if (value.length === 0 && !found.length) {
runState.eei.useGas(new BN(runState._common.param('gasPrices', 'sstoreReset')))
Expand Down
2 changes: 1 addition & 1 deletion lib/evm/precompiles/09-blake2f.ts
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ export default function(opts: PrecompileInput): ExecResult {
// final
const f = lastByte === 1

const gasUsed = new BN(opts._common.param('gasPrices', 'blake2bRound'))
const gasUsed = new BN(opts._common.param('gasPrices', 'blake2Round'))
gasUsed.imuln(rounds)
if (opts.gasLimit.lt(gasUsed)) {
return OOGResult(opts.gasLimit)
Expand Down
1 change: 1 addition & 0 deletions lib/exceptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export enum ERROR {
INTERNAL_ERROR = 'internal error',
CREATE_COLLISION = 'create collision',
STOP = 'stop',
REFUND_EXHAUSTED = 'refund exhausted',
}

export class VmError {
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
"ethereumjs-account": "^3.0.0",
"ethereumjs-block": "~2.2.0",
"ethereumjs-blockchain": "^4.0.1",
"ethereumjs-common": "^1.3.1",
"ethereumjs-common": "^1.3.2",
"ethereumjs-tx": "^2.1.1",
"ethereumjs-util": "^6.1.0",
"fake-merkle-patricia-tree": "^1.0.1",
Expand Down
68 changes: 68 additions & 0 deletions tests/api/istanbul/eip-2200.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
const tape = require('tape')
const BN = require('bn.js')
const Common = require('ethereumjs-common').default
const VM = require('../../../dist/index').default
const PStateManager = require('../../../dist/state/promisified').default
const { ERROR } = require('../../../dist/exceptions')
const { createAccount } = require('../utils')

const testCases = [
{ original: new BN(0), code: '60006000556000600055', used: 1612, refund: 0 }, // 0 -> 0 -> 0
{ original: new BN(0), code: '60006000556001600055', used: 20812, refund: 0 }, // 0 -> 0 -> 1
{ original: new BN(0), code: '60016000556000600055', used: 20812, refund: 19200 }, // 0 -> 1 -> 0
{ original: new BN(0), code: '60016000556002600055', used: 20812, refund: 0 }, // 0 -> 1 -> 2
{ original: new BN(0), code: '60016000556001600055', used: 20812, refund: 0 }, // 0 -> 1 -> 1
{ original: new BN(1), code: '60006000556000600055', used: 5812, refund: 15000 }, // 1 -> 0 -> 0
{ original: new BN(1), code: '60006000556001600055', used: 5812, refund: 4200 }, // 1 -> 0 -> 1
{ original: new BN(1), code: '60006000556002600055', used: 5812, refund: 0 }, // 1 -> 0 -> 2
{ original: new BN(1), code: '60026000556000600055', used: 5812, refund: 15000 }, // 1 -> 2 -> 0
{ original: new BN(1), code: '60026000556003600055', used: 5812, refund: 0 }, // 1 -> 2 -> 3
{ original: new BN(1), code: '60026000556001600055', used: 5812, refund: 4200 }, // 1 -> 2 -> 1
{ original: new BN(1), code: '60026000556002600055', used: 5812, refund: 0 }, // 1 -> 2 -> 2
{ original: new BN(1), code: '60016000556000600055', used: 5812, refund: 15000 }, // 1 -> 1 -> 0
{ original: new BN(1), code: '60016000556002600055', used: 5812, refund: 0 }, // 1 -> 1 -> 2
{ original: new BN(1), code: '60016000556001600055', used: 1612, refund: 0 }, // 1 -> 1 -> 1
{ original: new BN(0), code: '600160005560006000556001600055', used: 40818, refund: 19200 }, // 0 -> 1 -> 0 -> 1
{ original: new BN(1), code: '600060005560016000556000600055', used: 10818, refund: 19200 }, // 1 -> 0 -> 1 -> 0
{ original: new BN(1), gas: new BN(2306), code: '6001600055', used: 2306, refund: 0, err: ERROR.OUT_OF_GAS }, // 1 -> 1 (2300 sentry + 2xPUSH)
{ original: new BN(1), gas: new BN(2307), code: '6001600055', used: 806, refund: 0 } // 1 -> 1 (2301 sentry + 2xPUSH)
]
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, the numbers in the test cases differ to what is listed in the EIP? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, these test cases have been added after I created the PR: ethereum/EIPs@01b66af

It seems the EIP document has also been modified. Will have to look into it

Copy link
Member

Choose a reason for hiding this comment

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

The parameters in the EIP are also named very much differently than here (and in Go), did this also change? Or is this an intentional deviation to be in line with the old Constantinople naming?

If this changed we should likely do the extra round sigh on the Common library and change the parameter names (and eventually delete some?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the test vectors that were recently added to the EIP are for an older version of the EIP which assumed cost of SLOAD to be 200 instead of 800. See comment: ethereum/EIPs#2200 (comment)

About the param names, as long as we're conforming to test cases it should be fine. I don't think every client uses the same param names. The EIP is also somewhat under-specified. I'm more confident in geth's code as that's what eventually makes it into mainnet. But we can wait until the EIP's PR is merged, hopefully by then things will be more clear.


tape('Istanbul: EIP-2200: net-metering SSTORE', async (t) => {
const caller = Buffer.from('0000000000000000000000000000000000000000', 'hex')
const addr = Buffer.from('00000000000000000000000000000000000000ff', 'hex')
const key = new BN(0).toArrayLike(Buffer, 'be', 32)
for (const testCase of testCases) {
const common = new Common('mainnet', 'istanbul')
const vm = new VM({ common })
const state = new PStateManager(vm.stateManager)

const account = createAccount('00', '00')
await state.putAccount(addr, account)
await state.putContractCode(addr, Buffer.from(testCase.code, 'hex'))
if (!testCase.original.isZero()) {
await state.putContractStorage(addr, key, testCase.original)
}

const runCallArgs = {
caller,
gasLimit: testCase.gas ? testCase.gas : new BN(0xffffffffff),
to: addr
}

try {
const res = await vm.runCall(runCallArgs)
if (testCase.err) {
t.equal(res.execResult.exceptionError.error, testCase.err)
} else {
t.assert(res.execResult.exceptionError === undefined)
}
t.assert(new BN(testCase.used).eq(res.gasUsed))
t.assert(new BN(testCase.refund).eq(res.execResult.gasRefund))
} catch (e) {
t.fail(e.message)
}
}

t.end()
})