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

Subdivide runState, refactor loop #498

Merged
merged 11 commits into from
Apr 25, 2019
+254 −310
Merged

Subdivide runState, refactor loop #498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d037219
Break runState into runState, result, env
s1na Apr 23, 2019
b7139e0
Drop emitFreeLogs flag, mv static from runState to env
s1na Apr 23, 2019
3a94f2d
Mv depth to env, throw on pc out of range
s1na Apr 23, 2019
becd29b
Add interpreter runPrecompile to replace runJit
s1na Apr 23, 2019
15a1499
Pass message & txContext directly to loop.run
s1na Apr 23, 2019
e205a44
evm: make runState an attribute of loop, mv lastReturned to eei
s1na Apr 23, 2019
9339bb0
evm: fix isAccountEmpty
s1na Apr 23, 2019
446fbdf
evm: mv parseVmResults logic to loop.run
s1na Apr 23, 2019
e27ac52
Minor improvements
s1na Apr 24, 2019
5c721b5
Fix linting errors
s1na Apr 24, 2019
51b8d6b
Fix typo
s1na Apr 24, 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
108 changes: 62 additions & 46 deletions lib/evm/eei.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,19 @@ const { VmError, ERROR } = require('../exceptions')

export default class EEI {
_env: any
_runState: any
_result: any
_state: PStateManager
_interpreter: any
_lastReturned: Buffer

constructor (env: any) {
constructor (env: any, runState: any, result: any, state: PStateManager, interpreter: any) {
Copy link
Member

Choose a reason for hiding this comment

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

Much cleaner separation like this with reduced runState + env and explicit results.

Not sure if interpreter should be passed to the EEI, but it's at least better to have this passed explicit/separate then being hidden in the environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interpreter is needed for calls and creates. Not sure how to work around it...

Copy link
Member

Choose a reason for hiding this comment

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

Might these kind of logic rather belong to the Interpreter itself? We can keep for now and keep this in discussion though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm thinking along the same lines. I think it makes sense to move most of the call/create logic to Interpreter. One downside would be however that the logic would be further away from the opcode handlers.

this._env = env
this._state = new PStateManager(this._env.stateManager)
this._runState = runState
this._result = result
this._state = state
this._interpreter = interpreter
this._lastReturned = Buffer.alloc(0)
Copy link
Member

Choose a reason for hiding this comment

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

Also just for understanding: what is the main reasoning for this move?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reasoning was to localize variables as much as possible. lastReturned is there to track the last value returned from a call/create. It was only used in EEI, so I moved it from runState. But if the call/create logic moves elsewhere (I'm thinking Interpreter) this might need to be moved again. So, not quite sure yet where to put it.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, makes sense.

}

/**
Expand All @@ -20,13 +28,17 @@ export default class EEI {
* @throws if out of gas
*/
useGas (amount: BN): void {
this._env.gasLeft.isub(amount)
if (this._env.gasLeft.ltn(0)) {
this._env.gasLeft = new BN(0)
this._runState.gasLeft.isub(amount)
if (this._runState.gasLeft.ltn(0)) {
this._runState.gasLeft = new BN(0)
trap(ERROR.OUT_OF_GAS)
}
}

refundGas (amount: BN): void {
this._result.gasRefund.iaddn(amount)
}

/**
* Returns address of currently executing account.
* @returns {Buffer}
Expand Down Expand Up @@ -109,6 +121,10 @@ export default class EEI {
return this._env.code
}

isStatic (): boolean {
return this._env.isStatic
}

/**
* Get size of an account’s code.
* @param {BN} address - Address of account
Expand Down Expand Up @@ -137,7 +153,7 @@ export default class EEI {
* @returns {BN}
*/
getReturnDataSize (): BN {
return new BN(this._env.lastReturned.length)
return new BN(this._lastReturned.length)
}

/**
Expand All @@ -147,7 +163,7 @@ export default class EEI {
* @returns {Buffer}
*/
getReturnData (): Buffer {
return this._env.lastReturned
return this._lastReturned
}

/**
Expand Down Expand Up @@ -242,15 +258,15 @@ export default class EEI {
* @returns {BN}
*/
getGasLeft (): BN {
return new BN(this._env.gasLeft)
return new BN(this._runState.gasLeft)
}

/**
* Set the returning output data for the execution.
* @param {Buffer} returnData - Output data to return
*/
finish (returnData: Buffer): void {
this._env.returnValue = returnData
this._result.returnValue = returnData
}

/**
Expand All @@ -259,7 +275,7 @@ export default class EEI {
* @param {Buffer} returnData - Output data to return
*/
revert (returnData: Buffer): void {
this._env.returnValue = returnData
this._result.returnValue = returnData
trap(ERROR.REVERT)
}

Expand All @@ -274,21 +290,13 @@ export default class EEI {
}

async _selfDestruct (toAddress: Buffer): Promise<void> {
// TODO: Determine if gas consumption & refund should happen in EEI or opFn
if ((new BN(this._env.contract.balance)).gtn(0)) {
const empty = await this._state.accountIsEmpty(toAddress)
if (empty) {
this.useGas(new BN(this._env._common.param('gasPrices', 'callNewAccount')))
}
}

// only add to refund if this is the first selfdestruct for the address
if (!this._env.selfdestruct[this._env.address.toString('hex')]) {
this._env.gasRefund = this._env.gasRefund.addn(this._env._common.param('gasPrices', 'selfdestructRefund'))
if (!this._result.selfdestruct[this._env.address.toString('hex')]) {
this._result.gasRefund = this._result.gasRefund.addn(this._runState._common.param('gasPrices', 'selfdestructRefund'))
}

this._env.selfdestruct[this._env.address.toString('hex')] = toAddress
this._env.stopped = true
this._result.selfdestruct[this._env.address.toString('hex')] = toAddress
this._runState.stopped = true

// Add to beneficiary balance
const toAccount = await this._state.getAccount(toAddress)
Expand Down Expand Up @@ -323,7 +331,7 @@ export default class EEI {

// add data
log.push(data)
this._env.logs.push(log)
this._result.logs.push(log)
}

/**
Expand All @@ -340,7 +348,7 @@ export default class EEI {
to: address,
value: value,
data: data,
isStatic: this._env.static,
isStatic: this._env.isStatic,
depth: this._env.depth + 1
})

Expand All @@ -362,7 +370,7 @@ export default class EEI {
codeAddress: address,
value: value,
data: data,
isStatic: this._env.static,
isStatic: this._env.isStatic,
depth: this._env.depth + 1
Copy link
Member

Choose a reason for hiding this comment

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

Not getting this depth to env change - at least not directly intuitively - since I would place this on first thought to the run state, could you give a short explanation?

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 it to env to stress the fact that it should not be modified in a single call context. It is incremented each time a new call (or create) is invoked.

})

Expand Down Expand Up @@ -408,7 +416,7 @@ export default class EEI {
codeAddress: address,
value: value,
data: data,
isStatic: this._env.static,
isStatic: this._env.isStatic,
delegatecall: true,
depth: this._env.depth + 1
})
Expand All @@ -417,39 +425,38 @@ export default class EEI {
}

async _baseCall (msg: Message): Promise<BN> {
const selfdestruct = Object.assign({}, this._env.selfdestruct)
const selfdestruct = Object.assign({}, this._result.selfdestruct)
msg.selfdestruct = selfdestruct

// empty the return data buffer
this._env.lastReturned = Buffer.alloc(0)
this._lastReturned = Buffer.alloc(0)

// Check if account has enough ether and max depth not exceeded
if (this._env.depth >= this._env._common.param('vm', 'stackLimit') || (msg.delegatecall !== true && new BN(this._env.contract.balance).lt(msg.value))) {
if (this._env.depth >= this._runState._common.param('vm', 'stackLimit') || (msg.delegatecall !== true && new BN(this._env.contract.balance).lt(msg.value))) {
return new BN(0)
}

const results = await this._env.interpreter.executeMessage(msg)
const results = await this._interpreter.executeMessage(msg)

// concat the runState.logs
if (results.vm.logs) {
this._env.logs = this._env.logs.concat(results.vm.logs)
this._result.logs = this._result.logs.concat(results.vm.logs)
}

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

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

// Set return value
if (results.vm.return && (!results.vm.exceptionError || results.vm.exceptionError.error === ERROR.REVERT)) {
this._env.lastReturned = results.vm.return
this._lastReturned = results.vm.return
}

if (!results.vm.exceptionError) {
Object.assign(this._env.selfdestruct, selfdestruct)
Object.assign(this._result.selfdestruct, selfdestruct)
// update stateRoot on current contract
const account = await this._state.getAccount(this._env.address)
this._env.contract = account
Expand All @@ -465,7 +472,7 @@ export default class EEI {
* @param {Buffer} data
*/
async create (gasLimit: BN, value: BN, data: Buffer, salt: Buffer | null = null): Promise<BN> {
const selfdestruct = Object.assign({}, this._env.selfdestruct)
const selfdestruct = Object.assign({}, this._result.selfdestruct)
const msg = new Message({
caller: this._env.address,
gasLimit: gasLimit,
Expand All @@ -477,38 +484,37 @@ export default class EEI {
})

// empty the return data buffer
this._env.lastReturned = Buffer.alloc(0)
this._lastReturned = Buffer.alloc(0)

// Check if account has enough ether and max depth not exceeded
if (this._env.depth >= this._env._common.param('vm', 'stackLimit') || (msg.delegatecall !== true && new BN(this._env.contract.balance).lt(msg.value))) {
if (this._env.depth >= this._runState._common.param('vm', 'stackLimit') || (msg.delegatecall !== true && new BN(this._env.contract.balance).lt(msg.value))) {
return new BN(0)
}

this._env.contract.nonce = new BN(this._env.contract.nonce).addn(1)
await this._state.putAccount(this._env.address, this._env.contract)

const results = await this._env.interpreter.executeMessage(msg)
const results = await this._interpreter.executeMessage(msg)

// concat the runState.logs
if (results.vm.logs) {
this._env.logs = this._env.logs.concat(results.vm.logs)
this._result.logs = this._result.logs.concat(results.vm.logs)
}

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

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

// Set return buffer in case revert happened
if (results.vm.exceptionError && results.vm.exceptionError.error === ERROR.REVERT) {
this._env.lastReturned = results.vm.return
this._lastReturned = results.vm.return
}

if (!results.vm.exceptionError) {
Object.assign(this._env.selfdestruct, selfdestruct)
Object.assign(this._result.selfdestruct, selfdestruct)
// update stateRoot on current contract
const account = await this._state.getAccount(this._env.address)
this._env.contract = account
Expand Down Expand Up @@ -537,6 +543,15 @@ export default class EEI {
async create2 (gasLimit: BN, value: BN, data: Buffer, salt: Buffer): Promise<BN> {
return this.create(gasLimit, value, data, salt)
}

/**
* Returns true if account is empty (according to EIP-161).
* @param address - Address of account
*/
async isAccountEmpty (address: BN): Promise<boolean> {
const addressBuf = addressToBuffer(address)
return this._state.accountIsEmpty(addressBuf)
}
}

function trap (err: string) {
Expand All @@ -545,5 +560,6 @@ function trap (err: string) {

const MASK_160 = new BN(1).shln(160).subn(1)
function addressToBuffer (address: BN) {
if (Buffer.isBuffer(address)) return address
return address.and(MASK_160).toArrayLike(Buffer, 'be', 20)
}
51 changes: 25 additions & 26 deletions lib/evm/interpreter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
const promisify = require('util.promisify')
const BN = require('bn.js')
const ethUtil = require('ethereumjs-util')
const { ERROR } = require('../exceptions')
Expand Down Expand Up @@ -48,8 +47,8 @@ module.exports = class Interpreter {
}
}

let { err, results } = await this.runLoop(message)
results = await this._parseRunResult(err, results, message, createdAddress)
let results = await this.runLoop(message)
results = await this._parseRunResult(results, message, createdAddress)

// Save code if a new contract was created
if (createdAddress && !results.runState.vmError && results.return && results.return.toString() !== '') {
Expand All @@ -67,35 +66,20 @@ module.exports = class Interpreter {
const opts = {
storageReader: this._storageReader,
block: this._block,
gasPrice: this._tx.gasPrice,
origin: this._tx.origin,
code: message.code,
data: message.data,
gasLimit: message.gasLimit,
address: message.to,
caller: message.caller,
value: message.value,
depth: message.depth,
selfdestruct: message.selfdestruct,
static: message.isStatic
txContext: this._tx,
message
}

// Run code
let err, results
let results
const loop = new Loop(this._vm, this)
if (message.isCompiled) {
({ err, results } = await new Promise((resolve, reject) => {
this._vm.runJIT(opts, (err, results) => {
// TODO: Standardize errors so they're all VmError
// if (err && err.errorType !== 'VmError' && err !== ERROR.OUT_OF_GAS) return reject(err)
return resolve({ err, results })
})
}))
results = this.runPrecompile(message.code, message.data, message.gasLimit)
} else {
({ err, results } = await loop.run(opts))
results = await loop.run(opts)
}

return { err, results }
return results
}

/**
Expand All @@ -107,6 +91,20 @@ module.exports = class Interpreter {
return getPrecompile(address.toString('hex'))
}

runPrecompile (code, data, gasLimit) {
if (typeof code !== 'function') {
throw new Error('Invalid precompile')
}

const opts = {
data,
gasLimit,
_common: this._vm._common
}

return code(opts)
}

Copy link
Member

Choose a reason for hiding this comment

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

Much more fitting to have this here, always felt this being far too high out within the execution stack.

async _reduceSenderBalance (account, message) {
if (!message.delegatecall) {
const newBalance = new BN(account.balance).sub(message.value)
Expand Down Expand Up @@ -165,7 +163,7 @@ module.exports = class Interpreter {
// Setup new contract
await this._state.clearContractStorage(address)

await promisify(this._vm.emit.bind(this._vm))('newContract', {
await this._vm._emit('newContract', {
address: address,
code: message.code
})
Expand All @@ -175,7 +173,8 @@ module.exports = class Interpreter {
return { toAccount: account, createdAddress }
}

async _parseRunResult (err, results, message, createdAddress) {
async _parseRunResult (results, message, createdAddress) {
let err = results.exceptionError
if (createdAddress) {
// fee for size of the return value
var totalGas = results.gasUsed
Expand Down
Loading