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

Add state tests for Istanbul #607

Merged
merged 15 commits into from
Nov 18, 2019
+105 −66
Merged

Add state tests for Istanbul #607

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
83e42d8
Add state tests for Istanbul
s1na Nov 5, 2019
63c9032
Fix circleci config for istanbul tests
s1na Nov 5, 2019
d91f246
Fix transaction's hardfork option for istanbul tests
s1na Nov 5, 2019
3f15631
Store refund counter in EVM (#612)
s1na Nov 11, 2019
b0d71f0
Skip zero storage slots read from state tests
s1na Nov 11, 2019
29f14fb
Clone refund counter in EVMResult, fix eip-2200 api tests
s1na Nov 11, 2019
1b5538e
Fix blake2f precompile for rounds > 0x4000000
s1na Nov 11, 2019
7776c06
Mark CALLBlake2f_MaxRounds as a slow test
s1na Nov 11, 2019
c3e1ccf
Fix issues causing RevertPrecompiled* test failures
s1na Nov 13, 2019
b26f15c
Update ethereumjs-block to v2.2.1
s1na Nov 15, 2019
b9ebe69
Mark randomStatetest94_Istanbul as slow
s1na Nov 15, 2019
a29de25
Point ethereumjs-testing dependency to v1.3.0 tag
s1na Nov 15, 2019
18b34f9
Limit ethereumjs-util to ~6.1.0
s1na Nov 15, 2019
57d7483
Fix common mismatch in VM api tests
s1na Nov 18, 2019
970ddbf
Re-add gasRefund to ExecResult for backwards-compatibility
s1na Nov 18, 2019
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
24 changes: 24 additions & 0 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,15 @@ jobs:
- run:
name: testStatePetersburg
command: npm run testStatePetersburg
test_state_istanbul:
<<: *defaults
steps:
- attach_workspace:
at: ~/project
- *restore_node_modules
- run:
name: testStateIstanbul
command: npm run testStateIstanbul
test_blockchain:
<<: *defaults
steps:
Expand All @@ -91,6 +100,15 @@ jobs:
- run:
name: testBlockchain
command: npm run testBlockchain
test_blockchain_petersburg:
<<: *defaults
steps:
- attach_workspace:
at: ~/project
- *restore_node_modules
- run:
name: testBlockchainPetersburg
command: npm run testBlockchainPetersburg
coveralls:
<<: *defaults
steps:
Expand Down Expand Up @@ -123,9 +141,15 @@ workflows:
- test_state_petersburg:
requires:
- install
- test_state_istanbul:
requires:
- install
- test_blockchain:
requires:
- install
- test_blockchain_petersburg:
requires:
- install
- coveralls:
requires:
- install
28 changes: 5 additions & 23 deletions lib/evm/eei.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ export interface Env {
export interface RunResult {
logs: any // TODO: define type for Log (each log: [Buffer(address), [Buffer(topic0), ...]])
returnValue?: Buffer
gasRefund: BN
/**
* A map from the accounts that have self-destructed to the addresses to send their funds to
*/
Expand Down Expand Up @@ -67,7 +66,6 @@ export default class EEI {
this._result = {
logs: [],
returnValue: undefined,
gasRefund: new BN(0),
selfdestruct: {},
}
}
Expand All @@ -90,17 +88,17 @@ export default class EEI {
* @param amount - Amount of gas refunded
*/
refundGas(amount: BN): void {
this._result.gasRefund.iadd(amount)
this._evm._refund.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)
this._evm._refund.isub(amount)
if (this._evm._refund.ltn(0)) {
this._evm._refund = new BN(0)
trap(ERROR.REFUND_EXHAUSTED)
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can do this._result.gasRefund = this._evm._refund after modifying it. It clutters the code a little, but it avoids a breaking change.

As David, said Ganache only depends on gasRefund at the end of every tx, so this would work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I re-added gasRefund to ExecResult, by copying EVM._refund at the end of message execution in EVM. This should work for the Ganache use-case. I think we should be backwards-compatible now, at least for the most part.

}
Expand Down Expand Up @@ -163,10 +161,6 @@ export default class EEI {
* input data passed with the message call instruction or transaction.
*/
getCallDataSize(): BN {
if (this._env.callData.length === 1 && this._env.callData[0] === 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some tests were using CALLDATASIZE as a way of determining the address to call. They were failing for the case were calldata was Buffer<00> (CALLDATASIZE returned 0 instead of 1).

I'm not sure why this condition was here? any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

This is really weird. I checked in pyevm, and it doesn't have this logic either.

return new BN(0)
}

return new BN(this._env.callData.length)
}

Expand Down Expand Up @@ -354,9 +348,7 @@ export default class EEI {
async _selfDestruct(toAddress: Buffer): Promise<void> {
// only add to refund if this is the first selfdestruct for the address
if (!this._result.selfdestruct[this._env.address.toString('hex')]) {
this._result.gasRefund = this._result.gasRefund.addn(
this._common.param('gasPrices', 'selfdestructRefund'),
)
this.refundGas(new BN(this._common.param('gasPrices', 'selfdestructRefund')))
}

this._result.selfdestruct[this._env.address.toString('hex')] = toAddress
Expand Down Expand Up @@ -491,11 +483,6 @@ export default class EEI {
this._result.logs = this._result.logs.concat(results.execResult.logs)
}

// add gasRefund
if (results.execResult.gasRefund) {
this._result.gasRefund = this._result.gasRefund.add(results.execResult.gasRefund)
}

// this should always be safe
this.useGas(results.gasUsed)

Expand Down Expand Up @@ -553,11 +540,6 @@ export default class EEI {
this._result.logs = this._result.logs.concat(results.execResult.logs)
}

// add gasRefund
if (results.execResult.gasRefund) {
this._result.gasRefund = this._result.gasRefund.add(results.execResult.gasRefund)
}

// this should always be safe
this.useGas(results.gasUsed)

Expand Down
29 changes: 14 additions & 15 deletions lib/evm/evm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
import Account from 'ethereumjs-account'
import { ERROR, VmError } from '../exceptions'
import PStateManager from '../state/promisified'
import { getPrecompile, PrecompileFunc } from './precompiles'
import { getPrecompile, PrecompileFunc, ripemdPrecompileAddress } from './precompiles'
import TxContext from './txContext'
import Message from './message'
import EEI from './eei'
Expand All @@ -34,6 +34,10 @@ export interface EVMResult {
* Contains the results from running the code, if any, as described in [[runCode]]
*/
execResult: ExecResult
/**
* Amount of gas to refund from deleting storage values
*/
gasRefund?: BN
}

/**
Expand Down Expand Up @@ -61,10 +65,6 @@ export interface ExecResult {
* Array of logs that the contract emitted
*/
logs?: any[]
/**
* Amount of gas to refund from deleting storage values
*/
gasRefund?: BN
/**
* A map from the accounts that have self-destructed to the addresses to send their funds to
*/
Expand Down Expand Up @@ -96,12 +96,17 @@ export default class EVM {
_state: PStateManager
_tx: TxContext
_block: any
/**
* Amount of gas to refund from deleting storage values
*/
_refund: BN

constructor(vm: any, txContext: TxContext, block: any) {
this._vm = vm
this._state = this._vm.pStateManager
this._tx = txContext
this._block = block
this._refund = new BN(0)
}

/**
Expand All @@ -120,20 +125,12 @@ export default class EVM {
} else {
result = await this._executeCreate(message)
}
result.gasRefund = this._refund.clone()

const err = result.execResult.exceptionError
if (err) {
result.execResult.logs = []
await this._state.revert()
if (message.isCompiled) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we were re-touching any precompile in case of OOG.

// Empty precompiled contracts need to be deleted even in case of OOG
// because the bug in both Geth and Parity led to deleting RIPEMD precompiled in this case
// see https://github.com/ethereum/go-ethereum/pull/3341/files#diff-2433aa143ee4772026454b8abd76b9dd
// We mark the account as touched here, so that is can be removed among other touched empty accounts (after tx finalization)
if (err.error === ERROR.OUT_OF_GAS) {
await this._touchAccount(message.to)
}
}
} else {
await this._state.commit()
}
Expand Down Expand Up @@ -289,6 +286,7 @@ export default class EVM {
eei._result.selfdestruct = message.selfdestruct
}

const oldRefund = this._refund.clone()
const interpreter = new Interpreter(this._vm, eei)
const interpreterRes = await interpreter.run(message.code as Buffer, opts)

Expand All @@ -303,9 +301,10 @@ export default class EVM {
result = {
...result,
logs: [],
gasRefund: new BN(0),
selfdestruct: {},
}
// Revert gas refund if message failed
this._refund = oldRefund
}

return {
Expand Down
4 changes: 1 addition & 3 deletions lib/evm/opFns.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -959,9 +959,7 @@ 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.
runState.eei._result.gasRefund.isub(
new BN(runState._common.param('gasPrices', 'netSstoreClearRefund')),
)
runState.eei.subRefund(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.
runState.eei.refundGas(new BN(runState._common.param('gasPrices', 'netSstoreClearRefund')))
Expand Down
2 changes: 1 addition & 1 deletion lib/evm/precompiles/09-blake2f.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ export default function(opts: PrecompileInput): ExecResult {
const f = lastByte === 1

const gasUsed = new BN(opts._common.param('gasPrices', 'blake2Round'))
gasUsed.imuln(rounds)
gasUsed.imul(new BN(rounds))
if (opts.gasLimit.lt(gasUsed)) {
return OOGResult(opts.gasLimit)
}
Expand Down
5 changes: 3 additions & 2 deletions lib/evm/precompiles/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@ interface Precompiles {
[key: string]: PrecompileFunc
}

const ripemdPrecompileAddress = '0000000000000000000000000000000000000003'
const precompiles: Precompiles = {
'0000000000000000000000000000000000000001': p1,
'0000000000000000000000000000000000000002': p2,
'0000000000000000000000000000000000000003': p3,
[ripemdPrecompileAddress]: p3,
'0000000000000000000000000000000000000004': p4,
'0000000000000000000000000000000000000005': p5,
'0000000000000000000000000000000000000006': p6,
Expand All @@ -29,4 +30,4 @@ function getPrecompile(address: string): PrecompileFunc {
return precompiles[address]
}

export { precompiles, getPrecompile, PrecompileFunc, PrecompileInput }
export { precompiles, getPrecompile, PrecompileFunc, PrecompileInput, ripemdPrecompileAddress }
8 changes: 4 additions & 4 deletions lib/runTx.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,10 @@ async function _runTx(this: VM, opts: RunTxOpts): Promise<RunTxResult> {
// Caculate the total gas used
results.gasUsed = results.gasUsed.add(basefee)
// Process any gas refund
results.gasRefund = results.execResult.gasRefund
if (results.gasRefund) {
if (results.gasRefund.lt(results.gasUsed.divn(2))) {
results.gasUsed.isub(results.gasRefund)
const gasRefund = evm._refund
if (gasRefund) {
if (gasRefund.lt(results.gasUsed.divn(2))) {
results.gasUsed.isub(gasRefund)
} else {
results.gasUsed.isub(results.gasUsed.divn(2))
}
Expand Down
23 changes: 21 additions & 2 deletions lib/state/stateManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import Common from 'ethereumjs-common'
import { genesisStateByName } from 'ethereumjs-common/dist/genesisStates'
import Account from 'ethereumjs-account'
import Cache from './cache'
import { ripemdPrecompileAddress } from '../evm/precompiles'

/**
* Storage values of an account
Expand Down Expand Up @@ -102,11 +103,22 @@ export default class StateManager {
// if (toAccount.balance.toString('hex') === '00') {
// if they have money or a non-zero nonce or code, then write to tree
this._cache.put(address, account)
this._touched.add(address.toString('hex'))
this.touchAccount(address)
// self._trie.put(addressHex, account.serialize(), cb)
cb()
}

/**
* Marks an account as touched, according to the definition
* in [EIP-158](https://github.com/ethereum/EIPs/issues/158).
* This happens when the account is triggered for a state-changing
* event. Touched accounts that are empty will be cleared
* at the end of the tx.
*/
touchAccount(address: Buffer): void {
this._touched.add(address.toString('hex'))
}

/**
* Adds `value` to the state trie as code, and sets `codeHash` on the account
* corresponding to `address` to reference this.
Expand Down Expand Up @@ -275,7 +287,7 @@ export default class StateManager {
const contract = this._cache.get(address)
contract.stateRoot = storageTrie.root
this.putAccount(address, contract, cb)
this._touched.add(address.toString('hex'))
this.touchAccount(address)
})
})
}
Expand Down Expand Up @@ -372,6 +384,13 @@ export default class StateManager {
if (!touched) {
throw new Error('Reverting to invalid state checkpoint failed')
}
// Exceptional case due to consensus issue in Geth and Parity.
// See [EIP-716](https://github.com/ethereum/EIPs/issues/716) for context.
// The RIPEMD precompile has to remain *touched* even when the call reverts,
// and be considered for deletion.
if (this._touched.has(ripemdPrecompileAddress)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the re-touching of precompiles in case of OOG here (not a strong preference, I could re-move it to evm.ts). It is also limited to the precompile at 0x03.

touched.add(ripemdPrecompileAddress)
}
this._touched = touched
this._checkpointCount--

Expand Down
6 changes: 4 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@
"testStateByzantium": "npm run build:dist && node ./tests/tester -s --fork='Byzantium' --dist",
"testStateConstantinople": "npm run build:dist && node ./tests/tester -s --fork='Constantinople' --dist",
"testStatePetersburg": "npm run build:dist && node ./tests/tester -s --fork='Petersburg' --dist",
"testStateIstanbul": "npm run build:dist && node ./tests/tester -s --fork='Istanbul' --dist",
"testBuildIntegrity": "npm run build:dist && node ./tests/tester -s --dist --test='stackOverflow'",
"testBlockchain": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --fork='Petersburg' --dist --excludeDir='GeneralStateTests'",
"testBlockchain": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --fork='Istanbul' --dist --excludeDir='GeneralStateTests'",
"testBlockchainPetersburg": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --fork='Petersburg' --dist --excludeDir='GeneralStateTests'",
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious about this line. Weren't we testing some of the HF in the CI?

Copy link
Member

Choose a reason for hiding this comment

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

We are always only testing the latest or eventually the latest two HFs on CI, otherwise this would blow CI time (this nevertheless improved recently and e.g. allowed to add the blockchain tests beneath the state tests at all, before only state tests were running).

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thanks for the reply.

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, maybe we can cron a daily full test run on master using https://circleci.com/docs/2.0/configuration-reference/#schedule

I use the travis equivalent for Buidler, and lets me catch errors introduced by dependencies asap.

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 we're running the state tests for all HFs from Byzantium, but the blockchain tests were being run only for the latest HF because of their runtime, which was improved in #536. So I guess now we could run the blockchain tests for maybe 2-3 HFs.

Scheduling cron jobs is a good idea, I like it. We could also run run slow tests (#472) as a cron job.

The VM's testing infrastructure could generally use some improvements.

"testBlockchainGeneralStateTests": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --dist --dir='GeneralStateTests'",
"testAPI": "npm run build:dist && tape './tests/api/**/*.js'",
"testAPI:browser": "npm run build:dist && karma start karma.conf.js",
Expand Down Expand Up @@ -74,7 +76,7 @@
"@types/node": "^11.13.4",
"browserify": "^16.2.3",
"coveralls": "^3.0.0",
"ethereumjs-testing": "git+https://github.com/ethereumjs/ethereumjs-testing.git#v1.2.7",
"ethereumjs-testing": "git+https://github.com/ethereumjs/ethereumjs-testing.git#istanbul-tests",
"husky": "^2.1.0",
"karma": "^4.0.1",
"karma-browserify": "^6.0.0",
Expand Down
2 changes: 1 addition & 1 deletion tests/GeneralStateTestsRunner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ function runTestCase (options, testData, t, cb) {
testUtil.setupPreConditions(state, testData, done)
},
function (done) {
var tx = testUtil.makeTx(testData.transaction)
var tx = testUtil.makeTx(testData.transaction, options.forkConfig.toLowerCase())
block = testUtil.makeBlockFromEnv(testData.env)
tx._homestead = true
tx.enableHomestead = true
Expand Down
2 changes: 1 addition & 1 deletion tests/api/istanbul/eip-2200.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ tape('Istanbul: EIP-2200: net-metering SSTORE', async (t) => {
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))
t.assert(new BN(testCase.refund).eq(res.gasRefund))
} catch (e) {
t.fail(e.message)
}
Expand Down
Loading