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
Merged

Subdivide runState, refactor loop #498

Show file tree
Hide file tree
Changes from 1 commit
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
14 changes: 8 additions & 6 deletions lib/evm/eei.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,15 @@ export default class EEI {
_result: any
_state: PStateManager
_interpreter: any
_lastReturned: Buffer

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._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 Down Expand Up @@ -151,7 +153,7 @@ export default class EEI {
* @returns {BN}
*/
getReturnDataSize (): BN {
return new BN(this._runState.lastReturned.length)
return new BN(this._lastReturned.length)
}

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

/**
Expand Down Expand Up @@ -427,7 +429,7 @@ export default class EEI {
msg.selfdestruct = selfdestruct

// empty the return data buffer
this._runState.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._runState._common.param('vm', 'stackLimit') || (msg.delegatecall !== true && new BN(this._env.contract.balance).lt(msg.value))) {
Expand All @@ -450,7 +452,7 @@ export default class EEI {

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

if (!results.vm.exceptionError) {
Expand Down Expand Up @@ -482,7 +484,7 @@ export default class EEI {
})

// empty the return data buffer
this._runState.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._runState._common.param('vm', 'stackLimit') || (msg.delegatecall !== true && new BN(this._env.contract.balance).lt(msg.value))) {
Expand All @@ -508,7 +510,7 @@ export default class EEI {

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

if (!results.vm.exceptionError) {
Expand Down
114 changes: 55 additions & 59 deletions lib/evm/loop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,19 @@ module.exports = class Loop {
this._vm = vm // TODO: remove when not needed
this._state = new PStateManager(vm.stateManager)
this._interpreter = interpreter
this._runState = {
stopped: false,
programCounter: 0,
opCode: undefined,
memory: new Memory(),
memoryWordCount: new BN(0),
highestMemCost: new BN(0),
stack: new Stack(),
// TODO: Replace with EEI methods
_common: this._vm._common,
stateManager: this._state._wrapped,
storageReader: new StorageReader(this._state._wrapped)
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice to have this locally here, logically fits.

this._result = {
logs: [],
returnValue: false,
Expand All @@ -30,56 +43,45 @@ module.exports = class Loop {
}

// Initialize internal vm state
const runState = await this.initRunState(opts)
await this.initRunState(opts)

// Check that the programCounter is in range
const pc = runState.programCounter
if (pc !== 0 && (pc < 0 || pc >= runState.code.length)) {
const pc = this._runState.programCounter
if (pc !== 0 && (pc < 0 || pc >= this._runState.code.length)) {
throw new Error('Internal error: program counter not in range')
}

let err
// iterate through the given ops until something breaks or we hit STOP
while (this.canContinueExecution(runState)) {
const opCode = runState.code[runState.programCounter]
runState.opCode = opCode
await this.runStepHook(runState)
while (this.canContinueExecution()) {
const opCode = this._runState.code[this._runState.programCounter]
this._runState.opCode = opCode
await this.runStepHook()

try {
await this.runStep(runState)
await this.runStep()
} catch (e) {
err = e
break
}
}

return this.parseVmResults(runState, err, opts.message.gasLimit)
return this.parseVmResults(err, opts.message.gasLimit)
}

canContinueExecution (runState) {
const notAtEnd = runState.programCounter < runState.code.length
return !runState.stopped && notAtEnd && !this._result.vmError && !this._result.returnValue
canContinueExecution () {
const notAtEnd = this._runState.programCounter < this._runState.code.length
return !this._runState.stopped && notAtEnd && !this._result.vmError && !this._result.returnValue
}

async initRunState (opts) {
const runState = {
Object.assign(this._runState, {
code: opts.message.code,
stopped: false,
programCounter: opts.pc | 0,
opCode: undefined,
opName: undefined,
programCounter: opts.pc | this._runState.programCounter,
gasLeft: new BN(opts.message.gasLimit),
memory: new Memory(),
memoryWordCount: new BN(0),
stack: new Stack(),
validJumps: this._getValidJumpDests(opts.message.code),
highestMemCost: new BN(0),
lastReturned: [],
// TODO: Replace with EEI methods
_common: this._vm._common,
stateManager: this._state._wrapped,
storageReader: opts.storageReader || new StorageReader(this._state._wrapped)
}
storageReader: opts.storageReader || this._runState.storageReader
})
Copy link
Member

Choose a reason for hiding this comment

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

Nice to have this base value initialization and runtime value assignment separation.


const env = {
blockchain: this._vm.blockchain, // Only used in BLOCKHASH
Expand All @@ -100,38 +102,32 @@ module.exports = class Loop {
env.contract = await this._state.getAccount(env.address)
}

runState.eei = new EEI(env, runState, this._result, this._state, this._interpreter)

return runState
this._runState.eei = new EEI(env, this._runState, this._result, this._state, this._interpreter)
}

async runStep (runState) {
const opInfo = lookupOpInfo(runState.opCode, false)
const opName = opInfo.name

runState.opName = opName

async runStep () {
const opInfo = lookupOpInfo(this._runState.opCode, false)
// check for invalid opcode
if (runState.opName === 'INVALID') {
if (opInfo.name === 'INVALID') {
throw new VmError(ERROR.INVALID_OPCODE)
}

// calculate gas
runState.gasLeft = runState.gasLeft.sub(new BN(opInfo.fee))
if (runState.gasLeft.ltn(0)) {
runState.gasLeft = new BN(0)
this._runState.gasLeft = this._runState.gasLeft.sub(new BN(opInfo.fee))
if (this._runState.gasLeft.ltn(0)) {
this._runState.gasLeft = new BN(0)
throw new VmError(ERROR.OUT_OF_GAS)
}

// advance program counter
runState.programCounter++
this._runState.programCounter++

await this.handleOp(runState, opInfo)
await this.handleOp(opInfo)
}

async handleOp (runState, opInfo) {
async handleOp (opInfo) {
const opFn = this.getOpHandler(opInfo)
let args = [runState]
let args = [this._runState]

// run the opcode
if (opInfo.isAsync) {
Expand All @@ -145,21 +141,21 @@ module.exports = class Loop {
return opFns[opInfo.name]
}

async parseVmResults (runState, err, gasLimit) {
async parseVmResults (err, gasLimit) {
// remove any logs on error
if (err) {
this._result.logs = []
this._result.vmError = true
}

const results = {
runState: Object.assign({}, runState, this._result, runState.eei._env),
runState: Object.assign({}, this._runState, this._result, this._runState.eei._env),
selfdestruct: this._result.selfdestruct,
gasRefund: this._result.gasRefund,
exception: err ? 0 : 1,
exceptionError: err,
logs: this._result.logs,
gas: runState.gasLeft,
gas: this._runState.gasLeft,
'return': this._result.returnValue ? this._result.returnValue : Buffer.alloc(0)
}

Expand All @@ -171,24 +167,24 @@ module.exports = class Loop {
if (err && err.error !== ERROR.REVERT) {
results.gasUsed = gasLimit
} else {
results.gasUsed = gasLimit.sub(runState.gasLeft)
results.gasUsed = gasLimit.sub(this._runState.gasLeft)
}

return results
}

async runStepHook (runState) {
async runStepHook () {
const eventObj = {
pc: runState.programCounter,
gasLeft: runState.gasLeft,
opcode: lookupOpInfo(runState.opCode, true),
stack: runState.stack._store,
depth: runState.eei._env.depth,
address: runState.eei._env.address,
account: runState.eei._env.contract,
stateManager: runState.stateManager,
memory: runState.memory._store, // Return underlying array for backwards-compatibility
memoryWordCount: runState.memoryWordCount
pc: this._runState.programCounter,
gasLeft: this._runState.gasLeft,
opcode: lookupOpInfo(this._runState.opCode, true),
stack: this._runState.stack._store,
depth: this._runState.eei._env.depth,
address: this._runState.eei._env.address,
account: this._runState.eei._env.contract,
stateManager: this._runState.stateManager,
memory: this._runState.memory._store, // Return underlying array for backwards-compatibility
memoryWordCount: this._runState.memoryWordCount
}
/**
* The `step` event for trace output
Expand Down