VM: report dynamic gas values in `fee` field of `step` event #1364

jochem-brouwer · 2021-07-18T16:30:09Z

This PR closes #1169, closes #593

Related: #1215

This PR internally refactors the EVM. Opcodes are now split into their behavior (stack edits, getting block hashes, etc.) and their gas cost. However, due to the complex behavior of gas cost they still edit some of the state of the VM, for instance, for memory expansions this is done into the gas calculation part.

The result is that the fee result of the step event now represents the actual cost of an operation, not just the base fee.

This PR is a breaking change; it changes the fee field of the step event from number to BN.

The _runStepHook() function signature is also edited, but since this is an underscore, I assume this was meant to be private, so that should be non-breaking.

codecov · 2021-07-18T16:31:47Z

Codecov Report

Merging #1364 (3264d5b) into develop (866f865) will decrease coverage by 0.52%.
The diff coverage is 94.96%.

Flag	Coverage Δ
block	`86.63% <ø> (ø)`
blockchain	`83.56% <ø> (ø)`
client	`82.62% <ø> (+0.31%)`	⬆️
common	`94.44% <ø> (+0.20%)`	⬆️
devp2p	`82.81% <ø> (+0.14%)`	⬆️
ethash	`86.56% <ø> (ø)`
tx	`88.24% <ø> (ø)`
vm	`81.18% <94.96%> (-1.60%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

jochem-brouwer · 2021-07-19T21:05:25Z

This should pass at least Istanbul, MuirGlacier, Berlin and London. Running all tests to see if I missed something on earlier forks.

There are some TODOs left, but this is ready for early-review @ryanio @acolytec3

jochem-brouwer · 2021-07-19T21:05:45Z

Just a note, this is not yet EIP3155-ready, this will happen in another PR.

jochem-brouwer · 2021-07-19T21:42:55Z

@alcuadrado could you maybe check if you see some inconsistencies in the fee output? There are some cases not yet covered; one of them is reporting the call gas limit for any CALL opcode, and the invalid opcode now should report 0 fee, this is not consistent with Geth's output.

ryanio · 2021-07-20T18:01:54Z

just getting started reviewing, looks really great jochem, feels very organized and logical to follow.

added a commit with some small improvements as I dig in further.

ryanio · 2021-07-20T18:20:01Z

~~this comment is not relevant for this particular PR, but do you know the history of this todo? TODO: Replace getContractStorage with EEI method~~

~~I wouldn't mind fixing it along with typing the returned found object (which seems to be value: Buffer | { current: Buffer, original: Buffer })~~ edit: now fixed via vm todos PR

ryanio · 2021-07-20T18:25:46Z

Rebased on master and force pushed

ryanio · 2021-07-20T18:32:46Z

If we want to merge this in without worrying about the breaking change on the fee field of the step event, can we at the last moment in the InterpreterStep do fee: fee.toNumber()? Then we can change the type to BN as a breaking change item in #1024

ryanio · 2021-07-20T23:23:56Z

packages/vm/src/evm/interpreter.ts

+      await dynamicGasHandler(this._runState, gas)
+    }
+
+    // TODO: figure out if we should try/catch this (in case step event throws)


do you have an example of when or how the step event would throw? it seems innocent enough with some debug and emit.

if it throws, in some extreme case, we would probably want the error to bubble up to the user, although we could wrap it more nicely.

If we use a number instead of BN (so we convert the BN to a number) then it might throw if the BN does not fit in a number. What I actually meant with this TODO is to figure out what happens if any of the listeners throws. The previous behavior is that the tx then stops executing, so I guess we should keep that.

What I actually meant with this TODO is to figure out what happens if any of the listeners throws. The previous behavior is that the tx then stops executing, so I guess we should keep that.

ooh I see what you mean, if the consumer of the step event has a listener that throws? i thought you were maybe referring to it throwing within our own code, but it externally throwing also makes sense. in that case, could we consider wrapping the emit in a try/catch? then we could return some kind of sensible error to the user with the original error context attached. (i'd have to double check with my own code that this is actually how it would work)

If we use a number instead of BN (so we convert the BN to a number) then it might throw if the BN does not fit in a number.

yeah, in what kind of situation would the fee be > 2^53 - 1 though? seems like a pretty extreme case. i only mention it because then we can merge this in without a breaking change and then update the number to BN in a future planned breaking changes release (which we can get started on as soon as london goes live on mainnet)

I have ran into the number being too large, I am assuming this was some ethereum/tests test. It could theoretically also happen if we implement the trace specification; then if we normally would forward "all" gas, then Geth would report this amount of gas which would get forwarded, so if we have blocks with extremely large gas limits (i.e. in ethereum/tests) then it would report a number which is too large.

circling back to this TODO, do you have any fresh thoughts on this?

// TODO: figure out if we should try/catch this (in case step event throws)

i figure it would be harmless to add a try/catch as an extra precaution, can even add a test for it where a listener on the step event throws and expect that it doesn't stop vm execution.

Not sure if this has been addressed. Will nevertheless merge this in here, in doubt we can add separtely along a follow-up PR.

ryanio · 2021-07-21T19:14:32Z

do you think we should add coverage for these missed lines? they seem like pretty delicate parts of the code. are these probably otherwise tested in ethereum/tests that our coverage suite isn't running?

Click to expand!

jochem-brouwer · 2021-07-22T14:57:37Z

These are probably not ran by coverage, since these are hardfork specific rules, and IIRC we only run a specific fork in the coverage?

jochem-brouwer · 2021-07-28T10:51:01Z

I will write some tests for the uncovered lines as reported by codecov.

jochem-brouwer · 2021-07-28T16:12:12Z

That's weird, we don't have API tests for EIP 1283. And for 2929 this is probably not triggered by codecov since that runs in Berlin, and codecov runs Istanbul (should we update the default fork to either Berlin or London?).

jochem-brouwer · 2021-07-29T13:45:54Z

I will add a few more tests to increase coverage of currently uncovered lines.

jochem-brouwer · 2021-07-29T17:14:35Z

I found some redundant code after some long research, here a quick explanation.

Code in question is: (context is CALLCODE)

      if (gasLimit.gt(runState.eei.getGasLeft().isub(gas))) {
        trap(ERROR.OUT_OF_GAS)
      }
      if (gas.gt(runState.eei.getGasLeft())) {
        trap(ERROR.OUT_OF_GAS)
      }

The second check is redundant. Note that gasLimit is a stack value, so this is 0 or larger. If we have the case that gas > eei.getGasLeft(), then this means that eei.getGasLeft().isub(gas) is thus a negative value, so the first check will always return true, and we can thus remove the second check. See 36dc08c

packages/vm/src/evm/opcodes/util.ts

jochem-brouwer · 2021-07-30T20:24:32Z

Rebasing this one.

jochem-brouwer · 2021-08-01T13:27:39Z

This was an extremely lengthy rebase process - lets hope tests pass.

vm: clarify EIP-150 comment

…anbul run

vm: explicitly clone gasLimit for maxCallGas

… inferred

jochem-brouwer · 2021-08-29T18:55:06Z

Rebased this on master, develop branch now points to current master chain tip (866f865). Please merge this PR (if CI passes) into develop so we can use this branch for breaking changes.

holgerd77

Looks really great, again, really remarkable work! Totally love this now having all the gas logic together in one file and so transparently together, I would assume this will open up more use cases beyond the concrete (important) one addressed here.

Will merge to develop.

* vm: codes: add dynamicGas property vm: codes: make codes more typesafe * vm: create no-op dynamic gas handlers * vm: first batch of dynamic gas up to 0x3f * vm: add other opcodes to gas map vm: change step event fee type from number to BN vm: deduct dynamic gas vm: fix stack peek vm: do not add gasLimit to gas cost for CREATE * vm: move errors to gas map * vm: fix memory dynamic gas bug * vm: fix gas bugs caused by not considering base fee * vm: fix message call gas related bugs, clone current gas left * add typedoc for peek use underscore for unused peek params (and fix eslint config) format some comment newlines for readability switch from require to import for exceptions * simplify the 2929 state manager castings in runTx * add changelog entry * vm: add EIP1283 tests * vm: split non-eip2929 and eip2929 gas costs * vm: fix gas costs * vm: add early-hardfork coverage * vm: clarify pre-Constantinople SSTORE gas vm: clarify EIP-150 comment * run coverage for all state and blockchain tests, remove redundant istanbul run * vm: fix CALLCODE gas vm: explicitly clone gasLimit for maxCallGas * vm: remove TODO in interpreter * update defaultCost to BN, cast 2929 statemanager to simplify use syntax * use underscore for unused variables, simplify types since they can be inferred * vm: fix browser tests + fix rebase Co-authored-by: Ryan Ghods <ryan@ryanio.com>

* VM: report dynamic gas values in `fee` field of `step` event (#1364) * vm: codes: add dynamicGas property vm: codes: make codes more typesafe * vm: create no-op dynamic gas handlers * vm: first batch of dynamic gas up to 0x3f * vm: add other opcodes to gas map vm: change step event fee type from number to BN vm: deduct dynamic gas vm: fix stack peek vm: do not add gasLimit to gas cost for CREATE * vm: move errors to gas map * vm: fix memory dynamic gas bug * vm: fix gas bugs caused by not considering base fee * vm: fix message call gas related bugs, clone current gas left * add typedoc for peek use underscore for unused peek params (and fix eslint config) format some comment newlines for readability switch from require to import for exceptions * simplify the 2929 state manager castings in runTx * add changelog entry * vm: add EIP1283 tests * vm: split non-eip2929 and eip2929 gas costs * vm: fix gas costs * vm: add early-hardfork coverage * vm: clarify pre-Constantinople SSTORE gas vm: clarify EIP-150 comment * run coverage for all state and blockchain tests, remove redundant istanbul run * vm: fix CALLCODE gas vm: explicitly clone gasLimit for maxCallGas * vm: remove TODO in interpreter * update defaultCost to BN, cast 2929 statemanager to simplify use syntax * use underscore for unused variables, simplify types since they can be inferred * vm: fix browser tests + fix rebase * VM: moved dynamic fee to dedicated dynamicFee field in step event and retained fee behavior and type for backwards compatibility * VM: aligned InterpreterStep and step event object property documentation, completed missing step event properties * VM: test fix * vm: fix hardhat e2e tests * vm: fix MSTORE opcodes * vm: added dynamicGas property to push0 (EIP-3855) opcode * hardhat e2e: add temporary workaround for skipping tests with inconsistent memory field * nit style: use underscore instead of comment out unused variable Co-authored-by: Jochem Brouwer <jochembrouwer96@gmail.com> Co-authored-by: Ryan Ghods <ryan@ryanio.com>

* VM: Adopted Backwards-compatible Dynamic Gas Cost Refactoring (#1553) * VM: report dynamic gas values in `fee` field of `step` event (#1364) * vm: codes: add dynamicGas property vm: codes: make codes more typesafe * vm: create no-op dynamic gas handlers * vm: first batch of dynamic gas up to 0x3f * vm: add other opcodes to gas map vm: change step event fee type from number to BN vm: deduct dynamic gas vm: fix stack peek vm: do not add gasLimit to gas cost for CREATE * vm: move errors to gas map * vm: fix memory dynamic gas bug * vm: fix gas bugs caused by not considering base fee * vm: fix message call gas related bugs, clone current gas left * add typedoc for peek use underscore for unused peek params (and fix eslint config) format some comment newlines for readability switch from require to import for exceptions * simplify the 2929 state manager castings in runTx * add changelog entry * vm: add EIP1283 tests * vm: split non-eip2929 and eip2929 gas costs * vm: fix gas costs * vm: add early-hardfork coverage * vm: clarify pre-Constantinople SSTORE gas vm: clarify EIP-150 comment * run coverage for all state and blockchain tests, remove redundant istanbul run * vm: fix CALLCODE gas vm: explicitly clone gasLimit for maxCallGas * vm: remove TODO in interpreter * update defaultCost to BN, cast 2929 statemanager to simplify use syntax * use underscore for unused variables, simplify types since they can be inferred * vm: fix browser tests + fix rebase * VM: moved dynamic fee to dedicated dynamicFee field in step event and retained fee behavior and type for backwards compatibility * VM: aligned InterpreterStep and step event object property documentation, completed missing step event properties * VM: test fix * vm: fix hardhat e2e tests * vm: fix MSTORE opcodes * vm: added dynamicGas property to push0 (EIP-3855) opcode * hardhat e2e: add temporary workaround for skipping tests with inconsistent memory field * nit style: use underscore instead of comment out unused variable Co-authored-by: Jochem Brouwer <jochembrouwer96@gmail.com> Co-authored-by: Ryan Ghods <ryan@ryanio.com> * ci: bump karma-typescript version (#1631) * bump karma-typescript to new release * bump libp2p-bootstrap * update package-lock * VM: Jump dest analysis enhancements (#1629) * Change valid jumps to single array * Switch array to Uint8Array * use correct opt code * Check for jumpsub * Address feedback * Address fixes * More efficiency * Adjust if/else logic for efficiency * Move comments Co-authored-by: Holger Drewes <Holger.Drewes@gmail.com> Co-authored-by: Ryan Ghods <ryan@ryanio.com> * rlp: add to root readme (#1642) * add rlp to root readme * add carryforward for rlp flag since it is not always tested on every build unless its paths are modified * Client: Add tests to verify shutdown (#1641) * Add tests to verify client shutdown * Add libp2p test * Address feedback * most libp2p into separate file * VM: Add comparison testing between branchs for State Test Runner (#1634) * Add diff tester and performance * update script * Simplify script * Readme updates * move start * Update stashing logic in script * tx: input value testing (#1620) * tx: helper methods to generate tx values * tx: helper methods to generate tx values * chore: scaffold test and add extra to values * chore: add missing spread operator * chore: single value for each tx key * tx: add generateCombinations method and legacy + eip 1559 test cases * chore: remove console log * chore: use london hardfork for eip1559 * tx: refactor generateCombinations interface and properly compare hash * tx: deterministically randomly sample 1000 elements from array for testing * chore: remove v * chore: remove todo * chore: clearer variable declarations and types * tx: fix and simplify random sampling * tx: display tx hash in testing output * tx: convert buffer to hex for output log * ci: fix node-versions run for node <16 (#1653) * re-add updating to npm v7 for node versions <16 * only upgrade npm for node v <16 * fix bin/rlp js: node 12 doesn't support ES11 which added support for nullish coalescing operator (??) so we'll use ternary here alternatively we could write this file in TS and compile to e.g. dist/bin/rlp (like we do in the client bin), but maybe if the file gets more complicated, in its current state i don't think it's so neccessary * use same errorMsg format for JSON.parse, remove unneeded extra Uint8Array.from (already is uint8array) * rlp: v3 updates from integration (#1648) * rlp updates * util: add arrToBufArr and bufArrToArr and tests * util updates, add deprecation notice for re-exports * rlp tsconfig: include rootDir for composite option * remove util exports deprecation notices * rlp: add readme note for buffer compatibility * undo capitalization * Devp2p, Client: Fix duplicated debug messages (#1643) * devp2p: update debug package * blockchain: update debug package * client: update debug package * vm: update debug package * devp2p/dpt fix for #1485 * devp2p/eth: Fix for 485 * devp2p: Fix for #1485: add base debug * devp2p/dpt #1485: change to debug.extend() * devp2p:dpt:server change to degub.extend() * devp2p:eth change to debug.extend() * devp2p/les: change to debug.extend() * devp2p:rlpx: change to debug.extend() * rlps: change to debug.extend() * Update debug to use IP as first tag * vm, client: removed ProofStateManager interface, added optional getProof, verifyProof methods to StateManager interface (#1660) * Monorepo (& Tx, VM): Examples scripts in CI (#1658) * chore(examples): examples added to ci * chore(examples-ci): remove script from VM (for now) & rename examples workflow file * chore(ci): new script formwatted with prettier & example workflow changes to run with non-test branches Co-authored-by: Holger Drewes <Holger.Drewes@gmail.com> Co-authored-by: Holger Drewes <Holger.Drewes@gmail.com> Co-authored-by: acolytec3 <17355484+acolytec3@users.noreply.github.com> Co-authored-by: Gabriel Rocheleau <contact@rockwaterweb.com> Co-authored-by: ScottyPoi <66335769+ScottyPoi@users.noreply.github.com> Co-authored-by: Cesar Brazon <cesarbrazon10@gmail.com>

jochem-brouwer added PR state: WIP package: vm labels Jul 18, 2021

jochem-brouwer force-pushed the dynamic-gas branch 2 times, most recently from 13a04a4 to cb9169e Compare July 18, 2021 19:17

jochem-brouwer added the breaking label Jul 19, 2021

jochem-brouwer force-pushed the dynamic-gas branch 2 times, most recently from ad113b2 to ce50052 Compare July 19, 2021 16:02

jochem-brouwer added the type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR. label Jul 19, 2021

ryanio force-pushed the dynamic-gas branch from f79fdbb to b6cd21c Compare July 20, 2021 18:26

ryanio reviewed Jul 20, 2021

View reviewed changes

ryanio changed the title ~~VM: report dynamic gas values in fee field of step event~~ VM: report dynamic gas values in fee field of step event Jul 27, 2021

ryanio force-pushed the dynamic-gas branch from 264be22 to 8660f0d Compare July 27, 2021 19:59

jochem-brouwer commented Jul 30, 2021

View reviewed changes

packages/vm/src/evm/opcodes/util.ts Outdated Show resolved Hide resolved

jochem-brouwer force-pushed the dynamic-gas branch from 6f13708 to fb2d5c9 Compare August 1, 2021 13:27

ryanio and others added 12 commits August 29, 2021 20:53

add changelog entry

a4a4add

vm: add EIP1283 tests

9edf9a0

vm: split non-eip2929 and eip2929 gas costs

4a6404d

vm: fix gas costs

f3fb920

vm: add early-hardfork coverage

2d58847

vm: clarify pre-Constantinople SSTORE gas

ba821ef

vm: clarify EIP-150 comment

run coverage for all state and blockchain tests, remove redundant ist…

804a0ad

…anbul run

vm: fix CALLCODE gas

6276969

vm: explicitly clone gasLimit for maxCallGas

vm: remove TODO in interpreter

f01f69f

update defaultCost to BN, cast 2929 statemanager to simplify use syntax

cb3a1f2

use underscore for unused variables, simplify types since they can be…

44b4ac3

… inferred

vm: fix browser tests + fix rebase

3264d5b

jochem-brouwer force-pushed the dynamic-gas branch from 07a0c8d to 3264d5b Compare August 29, 2021 18:53

jochem-brouwer changed the base branch from master to develop August 29, 2021 18:54

holgerd77 removed PR state: do-not-merge PR/Issue state: blocked labels Aug 30, 2021

holgerd77 approved these changes Aug 30, 2021

View reviewed changes

holgerd77 merged commit b481409 into develop Aug 30, 2021

holgerd77 deleted the dynamic-gas branch August 30, 2021 09:35

holgerd77 restored the dynamic-gas branch August 30, 2021 11:05

ryanio deleted the dynamic-gas branch October 28, 2021 15:41

holgerd77 mentioned this pull request Jan 13, 2022

Dynamic gas costs are not exposed by the VM, but rather consumed during execution #1169

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VM: report dynamic gas values in `fee` field of `step` event #1364

VM: report dynamic gas values in `fee` field of `step` event #1364

jochem-brouwer commented Jul 18, 2021 •

edited

Loading

codecov bot commented Jul 18, 2021 •

edited

Loading

jochem-brouwer commented Jul 19, 2021

jochem-brouwer commented Jul 19, 2021

jochem-brouwer commented Jul 19, 2021

ryanio commented Jul 20, 2021 •

edited

Loading

ryanio commented Jul 20, 2021 •

edited

Loading

ryanio commented Jul 20, 2021

ryanio commented Jul 20, 2021

ryanio Jul 20, 2021

jochem-brouwer Jul 21, 2021

ryanio Jul 21, 2021 •

edited

Loading

jochem-brouwer Jul 22, 2021

ryanio Aug 5, 2021

holgerd77 Aug 30, 2021

ryanio commented Jul 21, 2021 •

edited

Loading

jochem-brouwer commented Jul 22, 2021

jochem-brouwer commented Jul 28, 2021

jochem-brouwer commented Jul 28, 2021

jochem-brouwer commented Jul 29, 2021

jochem-brouwer commented Jul 29, 2021 •

edited

Loading

jochem-brouwer commented Jul 30, 2021

jochem-brouwer commented Aug 1, 2021

jochem-brouwer commented Aug 29, 2021

holgerd77 left a comment

VM: report dynamic gas values in fee field of step event #1364

VM: report dynamic gas values in fee field of step event #1364

Conversation

jochem-brouwer commented Jul 18, 2021 • edited Loading

codecov bot commented Jul 18, 2021 • edited Loading

Codecov Report

jochem-brouwer commented Jul 19, 2021

jochem-brouwer commented Jul 19, 2021

jochem-brouwer commented Jul 19, 2021

ryanio commented Jul 20, 2021 • edited Loading

ryanio commented Jul 20, 2021 • edited Loading

ryanio commented Jul 20, 2021

ryanio commented Jul 20, 2021

ryanio Jul 20, 2021

Choose a reason for hiding this comment

jochem-brouwer Jul 21, 2021

Choose a reason for hiding this comment

ryanio Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

jochem-brouwer Jul 22, 2021

Choose a reason for hiding this comment

ryanio Aug 5, 2021

Choose a reason for hiding this comment

holgerd77 Aug 30, 2021

Choose a reason for hiding this comment

ryanio commented Jul 21, 2021 • edited Loading

jochem-brouwer commented Jul 22, 2021

jochem-brouwer commented Jul 28, 2021

jochem-brouwer commented Jul 28, 2021

jochem-brouwer commented Jul 29, 2021

jochem-brouwer commented Jul 29, 2021 • edited Loading

jochem-brouwer commented Jul 30, 2021

jochem-brouwer commented Aug 1, 2021

jochem-brouwer commented Aug 29, 2021

holgerd77 left a comment

Choose a reason for hiding this comment

VM: report dynamic gas values in `fee` field of `step` event #1364

VM: report dynamic gas values in `fee` field of `step` event #1364

jochem-brouwer commented Jul 18, 2021 •

edited

Loading

codecov bot commented Jul 18, 2021 •

edited

Loading

ryanio commented Jul 20, 2021 •

edited

Loading

ryanio commented Jul 20, 2021 •

edited

Loading

ryanio Jul 21, 2021 •

edited

Loading

ryanio commented Jul 21, 2021 •

edited

Loading

jochem-brouwer commented Jul 29, 2021 •

edited

Loading