Migrate opFns to typescript, modify loop/interpreter structure #506
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import BN = require('bn.js') | ||
| import { toBuffer, generateAddress, generateAddress2, zeros, KECCAK256_NULL, MAX_INTEGER } from 'ethereumjs-util' | ||
| import Account from 'ethereumjs-account' | ||
| import { VmError, ERROR } from '../exceptions' | ||
| import { StorageReader } from '../state' | ||
|
|
@@ -8,12 +8,28 @@ import { getPrecompile, PrecompileFunc, PrecompileResult } from './precompiles' | |
| import { OOGResult } from './precompiles/types' | ||
| import TxContext from './txContext' | ||
| import Message from './message' | ||
| import EEI from './eei' | ||
| import { default as Loop, LoopResult, RunState, IsException, RunOpts } from './loop' | ||
| const Block = require('ethereumjs-block') | ||
|
|
||
| export interface InterpreterResult { | ||
| gasUsed: BN | ||
| createdAddress?: Buffer | ||
| vm: ExecResult | ||
| } | ||
|
|
||
| export interface ExecResult { | ||
| runState?: RunState | ||
| exception: IsException | ||
| exceptionError?: VmError | ERROR | ||
| gas?: BN | ||
| gasUsed: BN | ||
| return: Buffer | ||
| // From RunResult | ||
| logs?: Buffer[] | ||
| returnValue?: Buffer | ||
| gasRefund?: BN | ||
| selfdestruct?: {[k: string]: Buffer} | ||
| } | ||
|
|
||
| export default class Interpreter { | ||
|
|
@@ -87,7 +103,13 @@ export default class Interpreter { | |
| } | ||
| } | ||
|
|
||
| let result | ||
| if (message.isCompiled) { | ||
| result = this.runPrecompile(message.code as PrecompileFunc, message.data, message.gasLimit) | ||
| } else { | ||
| result = await this.runLoop(message) | ||
| } | ||
|
|
||
| return { | ||
| gasUsed: result.gasUsed, | ||
| vm: result | ||
|
|
@@ -168,24 +190,53 @@ export default class Interpreter { | |
| } | ||
| } | ||
|
|
||
| async runLoop (message: Message, loopOpts: RunOpts = {}): Promise<ExecResult> { | ||
| const env = { | ||
| blockchain: this._vm.blockchain, // Only used in BLOCKHASH | ||
| address: message.to || zeros(32), | ||
| caller: message.caller || zeros(32), | ||
| callData: message.data || Buffer.from([0]), | ||
| callValue: message.value || new BN(0), | ||
| code: message.code as Buffer, | ||
| isStatic: message.isStatic || false, | ||
| depth: message.depth || 0, | ||
| gasPrice: this._tx.gasPrice, | ||
| origin: this._tx.origin || message.caller || zeros(32), | ||
| block: this._block || new Block(), | ||
| contract: await this._state.getAccount(message.to || zeros(32)) | ||
| } | ||
| const eei = new EEI(env, this._state, this, this._vm._common, message.gasLimit.clone()) | ||
| if (message.selfdestruct) { | ||
| eei._result.selfdestruct = message.selfdestruct | ||
| } | ||
|
|
||
| const loop = new Loop(this._vm, eei) | ||
| const opts = Object.assign({ storageReader: this._storageReader }, loopOpts) | ||
| const loopRes = await loop.run(message.code as Buffer, opts) | ||
|
|
||
| let result = eei._result | ||
| let gasUsed = message.gasLimit.sub(eei._gasLeft) | ||
| if (loopRes.exceptionError) { | ||
| if ((loopRes.exceptionError as VmError).error !== ERROR.REVERT) { | ||
| gasUsed = message.gasLimit | ||
| } | ||
|
|
||
| // remove any logs on error | ||
| result = Object.assign({}, result, { | ||
| logs: [], | ||
| gasRefund: null, | ||
| selfdestruct: null | ||
| }) | ||
| } | ||
|
|
||
| return Object.assign({}, result, { | ||
| runState: Object.assign({}, loopRes.runState, result, eei._env), | ||
| exception: loopRes.exception, | ||
| exceptionError: loopRes.exceptionError, | ||
| gas: eei._gasLeft, | ||
| gasUsed, | ||
| 'return': result.returnValue ? result.returnValue : Buffer.alloc(0) | ||
| }) | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Can't completely judge these changes in There was a problem hiding this comment. Yeah this is mostly temporary. But I'd appreciate comments on how to change it in future (I'm not yet sure myself). There was a problem hiding this comment. Will give this some thought, maybe @alcuadrado can also join for discussion? |
||
| /** | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't need to be addressed in this PR but still have the impression, that gas handling should rather have it's own
gas.tsmodule since this doesn't really belong to the environment but is rather some internal state component likeStackorMemory. This would be likely also similarly useful for VM users if they could hook into that class and get step-by-step gas usage info or can manipulate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also one indicator:
gasLeftdoesn't really look right in the constructor here, this is also used only in very isolated places and not throughout the whole class.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On-top note: but please resist the temptation of just doing it because I repeat the idea 2-3 times if you don't think it's a good idea. 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I'm not sure what you have in mind to put in the
gas.tsmodule :) TheuseGasandrefundGasmethods (maybe like this)?It's mostly used in
opFns(and also used for instruction base fee inLoop). On purpose I avoided doing gas metering inEEI.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for digging this out from Py-EVM, yes I was actually thinking about something like this. We can do this later though if agreed upon, don't let this distract you here. I would assume this would be useful if e.g. extended with events which could be used to real-time display gas usage in IDEs e.g..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I think this is doable. One possibility (for next versions) is to take inspiration from go-ethereum's gas table bring most gas calculation logic there (possibly even hardfork-specific gas costs).