ExecResult simplification

[alcuadrado]

This is a WIP that simplifies the ExecResult type.

The changes I made so far are:

• Always use VMError, and not directly ERROR.
• Unified ExecResult and PrecompileResult.
• Moved OOGResult to evm.ts
• Renamed EVMResult#vm to EVMResult#execResult

Each of them is implemented in separate commits, so we can pick/discard whatever we want.

[lgtm-com]

This pull request fixes 1 alert when merging 80d1293 into a990a3a - view on LGTM.com

fixed alerts:

• 1 for Unused variable, import, function or class

[alcuadrado]

Some questions:

1. EVMResult has a gasUsed field. It also has a field vm which has gasUsed. I tried to unify them and the tests broke. I suspect that the former has gasRefund (and maybe other things) into account. I think we should document that.
2. ExecResult can be used wherever PrecompileResult is used. This makes things a little simpler. Done
3. If (2) is implemented, EVMResult#vm could be renamed to EVMResult#execResult, which is clearer. Done
4. OOGResult is placed in a types file, which is weird. It should probably be part of evm.ts. Will think about this. Done

[lgtm-com]

This pull request fixes 1 alert when merging e78de1c into a990a3a - view on LGTM.com

fixed alerts:

• 1 for Unused variable, import, function or class

[coveralls]

Coverage increased (+0.005%) to 95.092% when pulling f3bd72a on exec-result-simplification into 4bbb6e3 on master.

[lgtm-com]

This pull request fixes 1 alert when merging d50bb32 into a990a3a - view on LGTM.com

fixed alerts:

• 1 for Unused variable, import, function or class

[s1na]

Re 1.:

https://github.com/ethereumjs/ethereumjs-vm/blob/a990a3a7251a2706869df31ca39b7c8761caf686/lib/runTx.ts#L147-L165

gasUsed is in fact updated in the runTx post-processing, but one question is whether we'd need to return the gasUsed within results.vm.

Something I was thinking about a bit in this direction was whether it makes sense to have separate objects for the results at each step (InterpreterResult, EVMResult and RunTxResult) instead of nesting them inside one-another.

[s1na]

Thanks for starting to work on this! I like the changes you've made.

Some other points that I was considering for simplification, which I'd be curious to hear your take on:

• return and returnValue seem redundant
• exception and exceptionError can be expressed with only one var which is null if no exception, and has an exception object if one occured
• Is returning runState necessary? if so, for every Result object (tx, evm and interpreter)?
• gas and gasUsed are two sides of the same coin, again not sure if both are really necessary

[alcuadrado]

gasUsed is in fact updated in the runTx post-processing, but one question is whether we'd need to return the gasUsed within results.vm.

I have a feeling that this may be useful for some use-cases, but I don't know of one.

Something I was thinking about a bit in this direction was whether it makes sense to have separate objects for the results at each step (InterpreterResult, EVMResult and RunTxResult) instead of nesting them inside one-another.

This is interesting. How do you imagine this being implemented? Returning a tuple form runTx?

[alcuadrado]

• return and returnValue seem redundant

Haven't noticed this before. I'll try and unify them.

• exception and exceptionError can be expressed with only one var which is null if no exception, and has an exception object if one occured

Good point. I'm also a little confused about 0 and 1 there. They seem to be inverted.

• Is returning runState necessary? if so, for every Result object (tx, evm and interpreter)?

TBH I have no idea. Maybe it's useful on failures.

• gas and gasUsed are two sides of the same coin, again not sure if both are really necessary

Is there a way to know the gas limit apart from adding them?

[lgtm-com]

This pull request fixes 1 alert when merging 370526e into a990a3a - view on LGTM.com

fixed alerts:

• 1 for Unused variable, import, function or class

[alcuadrado]

• exception and exceptionError can be expressed with only one var which is null if no exception, and has an exception object if one occured

I think I find a bug or documentation error related to this.

TxResult#status indicates success with a 0. See: https://github.com/ethereumjs/ethereumjs-vm/blob/master/lib/runBlock.ts#L52-L55

ExecResult#exception does it with a 1. See: https://github.com/ethereumjs/ethereumjs-vm/blob/master/lib/evm/evm.ts#L50-L53

But this internal function of runBlock sets TxResult#status as 1 iif ExecResult#exception is also 1. This contradicts their docs. If I change it, testAPI tests fail.

I guess is a documentation error. Any idea @s1na ?

[s1na]

This 1 | 0 is really confusing for me too. Every time I have to look up a few places to see which one's which. Yeah it seems the documentation for status is incorrect. exception is 0 when there's an exception and 1 when there's no exception. I did a search and status doesn't seem to be used in our source code.

[s1na]

Ok let's leave those options we're not sure of for now.

How do you imagine this being implemented? Returning a tuple form runTx?

RunTxResult extends EVMResult which nests ExecResult, so you could say txRes.vm.exception. What I had in mind is not to nest ExecResult, but define RunTxResult fully and to include only what makes sense for the result of running a tx. Similarly for runCall which returns EVMResult (which nests ExecResult). runCode itself returns ExecResult.

I don't have a strong opinion on this. I just think nesting data structures like we're doing now results in returning extra unnecessary data.

On runState: it contains program counter, memory, e.g. of an EVM bytecode being interpreted. When running a transaction, multiple bytecodes (from multiple contracts) could be executed. I don't see how it's useful to return the program counter of the last executed bytecode in a transaction (note that multiple bytecodes or contracts could be executed during a transaction). At most I think that could be relevant information for runCode, not even runCall. Besides, this information is being emitted as an event.

Is there a way to know the gas limit apart from adding them?

Gas limit is usually provided by the calling code. But let's leave this gas and gasUsed as is for now.

[s1na]

To help our thought process it might be good to have every result interface and their relationship in one glance (from the lowest level of bytecode execution, to highest level of block execution):

Running a bytecode via Interpreter returns:

export interface InterpreterResult {
  runState?: RunState
  exception: IsException
  exceptionError?: VmError | ERROR
}

It further produces these side-effects which are stored in EEI (this is also not ideal):

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
   */
  selfdestruct: { [k: string]: Buffer }
}

After executing a single call, EVM.runInterpreter combines the InterpreterResult and RunResult and processes them, turning it into ExecResult:

export interface ExecResult {
  runState?: RunState
  /**
   * `0` if the contract encountered an exception, `1` otherwise
   */
  exception: IsException
  /**
   * Description of the exception, if any occured
   */
  exceptionError?: VmError | ERROR
  /**
   * Amount of gas left
   */
  gas?: BN
  /**
   * Amount of gas the code used to run
   */
  gasUsed: BN
  /**
   * Return value from the contract
   */
  return: Buffer
  /**
   * Array of logs that the contract emitted
   */
  logs?: any[]
  /**
   * Value returned by the contract
   */
  returnValue?: Buffer
  /**
   * 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
   */
  selfdestruct?: { [k: string]: Buffer }
}

EVM.executeMessage (or more specifically _executeCall and _executeCreate) which could potentially invoke multiple nested calls takes ExecResult and processes it into EVMResult:

export interface EVMResult {
  /**
   * Amount of gas used by the transaction
   */
  gasUsed: BN
  /**
   * Address of created account durint transaction, if any
   */
  createdAddress?: Buffer
  /**
   * Contains the results from running the code, if any, as described in [[runCode]]
   */
  vm: ExecResult
}

After EVM.executeMessage in runTx we know that the main call (and all its nested calls) are finished, so it does the final processing, calculating bloom, final gas usage and amount spent, resulting in (it also modifies EVMresult fields):

/**
 * Execution result of a transaction
 */
export interface RunTxResult extends EVMResult {
  /**
   * Bloom filter resulted from transaction
   */
  bloom: Bloom
  /**
   * The amount of ether used by this transaction
   */
  amountSpent: BN
  /**
   * The amount of gas as that was refunded during the transaction (i.e. `gasUsed = totalGasConsumed - gasRefund`)
   */
  gasRefund?: BN
}

runBlock then executes multiple transactions and aggregates their receipts and results:

/**
 * Result of [[runBlock]]
 */
export interface RunBlockResult {
  /**
   * Receipts generated for transactions in the block
   */
  receipts: TxReceipt[]
  /**
   * Results of executing the transactions in the block
   */
  results: RunTxResult[]
}

[alcuadrado]

Yeah it seems the documentation for status is incorrect. exception is 0 when there's an exception and 1 when there's no exception. I did a search and status doesn't seem to be used in our source code.

Ah yes, it's incorrect. I think it makes sense to keep the status in the receipt despite it not being used internally. It is part of the standard receipt in the clients, the RPC, and also included in the white paper.

RunTxResult extends EVMResult which nests ExecResult, so you could say txRes.vm.exception. What I had in mind is not to nest ExecResult, but define RunTxResult fully and to include only what makes sense for the result of running a tx. Similarly for runCall which returns EVMResult (which nests ExecResult). runCode itself returns ExecResult.

Sounds reasonable. I'll pay more attention to these types and think about it.

I don't see how it's useful to return the program counter of the last executed bytecode in a transaction (note that multiple bytecodes or contracts could be executed during a transaction). At most I think that could be relevant information for runCode, not even runCall. Besides, this information is being emitted as an event.

This is a very good point.

To help our thought process it might be good to have every result interface and their relationship in one glance (from the lowest level of bytecode execution, to highest level of block execution):

Thanks! This is super useful! I'll take a deeper look at this later today/tomorrow.

[s1na]

I think it makes sense to keep the status in the receipt despite it not being used internally. It is part of the standard receipt in the clients, the RPC, and also included in the white paper.

Yeah sorry for the confusion, I didn't suggest we drop status, I know it's an important field. I was just looking to see if it's used internally, how are the 0 and 1 values interpreted, just to make sure the documentation is wrong.

By the way, we don't have to do all the simplification in this PR. This can be a longer process. Let me know whenever you're happy with the PR yourself and I'll review.

[alcuadrado]

After working for a few days with the beta of v4, I came to appreciate this simplification a lot. I will update this PR today so it can be reviewed.

Should we release a new beta with this change @holgerd77?

[holgerd77]

@alcuadrado Yes, maybe another beta can't hurt to get some in-between milestones and help things to settle. Will prepare some release notes once this has been merged, or is there something else you guys would like to get in (the block library TS transition is likely too much?)? //cc @s1na

[lgtm-com]

This pull request fixes 1 alert when merging d5d7484 into 4bbb6e3 - view on LGTM.com

fixed alerts:

• 1 for Unused variable, import, function or class

[alcuadrado]

I unified the ExecResult#exception and ExecResult#exceptionError into just using ExecResult#exceptionError and check that it's not undefined instead of using ExecResult#exception.

I think I encountered a bug in the EEI implementation. Can you take a look at this @s1na? d5d7484#diff-73c9d0f1a5deec3528a056f5d9e78b3aR575

Apart from that, I think this is ready to review.

Things discussed but not included in this PR:

1. Simplify the naming around gas variables in the different result objects.
2. Removing runState from some types where it's not useful to have it.
3. Inlining return values instead of nesting them.

[alcuadrado]

@alcuadrado Yes, maybe another beta can't hurt to get some in-between milestones and help things to settle. Will prepare some release notes once this has been merged, or is there something else you guys would like to get in (the block library TS transition is likely too much?)?

I missed this @holgerd77. What about a beta when this gets merged, and another one if we migrate block to ts?

[s1na]

This is very good thanks @alcuadrado. Just for documentation purposes, it introduces 3 breaking changes:

1. The result vm field being renamed to execResult (wondering if this is worth a breaking change. Thoughts @holgerd77?)
2. exception is dropped
3. returnValue in one of the result interfaces is dropped in favor of return (I do favor returnValue slightly more though, because it's not a reserved keyword)

I think I encountered a bug in the EEI implementation. Can you take a look at this @s1na? d5d7484#diff-73c9d0f1a5deec3528a056f5d9e78b3aR575

This is consensus logic, I assume the current implementation is correct, otherwise tests would have caught it. (We're not fully compatible with the Ewasm EEI and have already other differences.)

[alcuadrado]

1. The result vm field being renamed to execResult (wondering if this is worth a breaking change. Thoughts @holgerd77?)
   I'm not sure either.

3. returnValue in one of the result interfaces is dropped in favor of return (I do favor returnValue slightly more though, because it's not a reserved keyword)

This is a good point, I'll rename it.

This is consensus logic, I assume the current implementation is correct, otherwise tests would have caught it. (We're not fully compatible with the Ewasm EEI and have already other differences.)

Yes, I initially implemented the logic described in the spec and lots of tests failed. Should I leave the comment there?

[lgtm-com]

This pull request fixes 1 alert when merging 5263ea6 into 4bbb6e3 - view on LGTM.com

fixed alerts:

• 1 for Unused variable, import, function or class

[holgerd77]

@s1na I think I like the vm -> execResult renaming, so since you also have no strong opinion on that I would leave it to Patricio to eventually revert or keep if still confident on this. Think generally some breaking change is not so overly problematic on this release especially.

Just had a rough look, there is this return to returnValue revert not yet done in places like here, otherwise it would be ready for review, am I correct on this?

@s1na Would be good if you then just finish the review and eventually merge, not yet so deep into this PR respectively the code changes and would like to avoid digging deeper if possible.

@alcuadrado @s1na Yes, we can also do another beta after the block TS transition (if someone actually has the capacity to do it, technically not totally some pre-condition for the release, would be nice though).

[alcuadrado]

@s1na I think I like the vm -> execResult renaming, so since you also have no strong opinion on that I would leave it to Patricio to eventually revert or keep if still confident on this. Think generally some breaking change is not so overly problematic on this release especially.

I think this kind of change only makes sense in the context of a bigger release, like this one.

Just had a rough look, there is this return to returnValue revert not yet done in places like here, otherwise it would be ready for review, am I correct on this?

I forgot about that. It's already renamed.

@s1na Would be good if you then just finish the review and eventually merge, not yet so deep into this PR respectively the code changes and would like to avoid digging deeper if possible.

@alcuadrado @s1na Yes, we can also do another beta after the block TS transition (if someone actually has the capacity to do it, technically not totally some pre-condition for the release, would be nice though).

Sounds good.

[lgtm-com]

This pull request fixes 1 alert when merging f3bd72a into 4bbb6e3 - view on LGTM.com

fixed alerts:

• 1 for Unused variable, import, function or class

[s1na]

Looks good to me!