Subdivide runState, refactor loop #498
Conversation
|
|
| @@ -4,7 +4,6 @@ const BN = utils.BN | |||
| const exceptions = require('../exceptions.js') | |||
| const ERROR = exceptions.ERROR | |||
| const VmError = exceptions.VmError | |||
| const PStateManager = require('../state/promisified').default | |||
| const MASK_160 = new BN(1).shln(160).subn(1) | |||
There was a problem hiding this comment.
Great to have StateManager refactored out of here. 👍 😄
| if (empty) { | ||
| runState.eei.useGas(new BN(runState._common.param('gasPrices', 'callNewAccount'))) | ||
| } | ||
| } |
There was a problem hiding this comment.
What is happening here respectively why has this been added?
There was a problem hiding this comment.
This is to calculate the gas cost of selfdestruct. It used to be here, but I had moved it to EEI to avoid using stateManager here. Now I've added the necessary method isAccountEmpty to EEI, and brought the gas calculation again back to the opcode handler.
| @@ -892,7 +897,7 @@ function updateSstoreGas (runState, found, value) { | |||
| runState.eei.useGas(new BN(runState._common.param('gasPrices', 'sstoreReset'))) | |||
| } else if (value.length === 0 && found.length) { | |||
| runState.eei.useGas(new BN(runState._common.param('gasPrices', 'sstoreReset'))) | |||
| runState.gasRefund.iaddn(runState._common.param('gasPrices', 'sstoreRefund')) | |||
| runState.eei.refundGas(runState._common.param('gasPrices', 'sstoreRefund')) | |||
| } else if (value.length !== 0 && !found.length) { | |||
| runState.eei.useGas(new BN(runState._common.param('gasPrices', 'sstoreSet'))) | |||
| } else if (value.length !== 0 && found.length) { | |||
There was a problem hiding this comment.
Transition of environment data access from runState to EEI, nice, looks good.
| } | ||
|
|
||
| return { name: opcode, opcode: op, fee: fee, dynamic: code[2], isAsync: code[3] } | ||
| return { name: opcode, opcode: op, fee: code[1], dynamic: code[2], isAsync: code[3] } | ||
| } |
There was a problem hiding this comment.
freeLogs removal, cleanup, ok.
|
|
||
| constructor (env: any) { | ||
| constructor (env: any, runState: any, result: any, state: PStateManager, interpreter: any) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Interpreter is needed for calls and creates. Not sure how to work around it...
There was a problem hiding this comment.
Might these kind of logic rather belong to the Interpreter itself? We can keep for now and keep this in discussion though.
There was a problem hiding this comment.
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.
lib/evm/loop.js
Outdated
| isStatic: opts.message.isStatic || false, | ||
| depth: opts.message.depth || 0, | ||
| gasPrice: opts.txContext.gasPrice, | ||
| origin: opts.txContext.origin || opts.message.caller || utils.zeros(32), | ||
| block: opts.block || new Block() | ||
| } |
There was a problem hiding this comment.
Cleaner to have this encapsulated in message and txContext, good. 👍
|
|
||
| constructor (env: any, runState: any, result: any, state: PStateManager, interpreter: any) { | ||
| this._env = env | ||
| this._runState = runState | ||
| this._result = result | ||
| this._state = state | ||
| this._interpreter = interpreter | ||
| this._lastReturned = Buffer.alloc(0) |
There was a problem hiding this comment.
Also just for understanding: what is the main reasoning for this move?
There was a problem hiding this comment.
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.
| _common: this._vm._common, | ||
| stateManager: this._state._wrapped, | ||
| storageReader: new StorageReader(this._state._wrapped) | ||
| } |
There was a problem hiding this comment.
Nice to have this locally here, logically fits.
| storageReader: opts.storageReader || new StorageReader(this._state._wrapped) | ||
| } | ||
| storageReader: opts.storageReader || this._runState.storageReader | ||
| }) |
There was a problem hiding this comment.
Nice to have this base value initialization and runtime value assignment separation.
| gas: this._runState.gasLeft, | ||
| gasUsed, | ||
| 'return': this._result.returnValue ? this._result.returnValue : Buffer.alloc(0) | ||
| }) |
This is a mixed bag PR, that partly aims to prepare future migration of
Loopto typescript:emitFreeLogsflag. This is a very custom logic that can be after v4 implemented by inheritingLooprunJitas deprecated, and adds the necessary logic for running precompiles directly toInterpreterrunStatean attribute ofLoopinstead of passing it to every method