[VM/Common] move gas base fees to Common #806

jochem-brouwer · 2020-07-03T21:05:34Z

Closes #803

This PR moves all gas base fees to Common. It also disables checks within the opcodes to see if the opcode is available. These opcodes are not available if VM has a Common defined before the hardfork which introduced this opcode. See getOpcodesForHF here.

Also started adding the right fee values for ChainStart and TangerineWhistle so this also introduces some work to support ChainStart/TangerineWhistle again! 😄 Related issue: #652

This only makes opcodes.ts slightly less typesafe by calling createOpcodes in getOpcodesForHF.

codecov · 2020-07-03T21:06:17Z

Codecov Report

Merging #806 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #806   +/-   ##
=======================================
  Coverage   84.56%   84.56%           
=======================================
  Files          18       18           
  Lines        1250     1250           
  Branches      247      247           
=======================================
  Hits         1057     1057           
  Misses        125      125           
  Partials       68       68

Flag	Coverage Δ
#account	`92.85% <ø> (ø)`
#block	`80.35% <ø> (ø)`
#blockchain	`84.71% <ø> (ø)`
#common	`93.98% <ø> (ø)`
#ethash	`85.40% <ø> (ø)`
#tx	`94.16% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5a140b...2423cf3. Read the comment docs.

jochem-brouwer · 2020-07-03T21:19:56Z

I assumed base fees in opcodes.ts were chainstart fees: actually, they are not. So let's fix that.

[VM] remove unneeded opcode entries which only introduced gas changes [VM/Common] Add right fork base fees, remove STATICCALL from base opcodes [Common] remove STATICCALL from chainstart

holgerd77

Looks good, nice work @jochem-brouwer and - very cool - this even directly make it still into it's way to the new Common release @evertonfraga is preparing, Common is getting a lot more complete with that. 👍

holgerd77 · 2020-07-06T07:49:20Z

packages/vm/lib/evm/opcodes.ts

+      throw new Error('base fee not defined for: ' + opcodeBuilder[key].name)
+    }
+    opcodeBuilder[key].fee = common.param('gasPrices', opcodeBuilder[key].name.toLowerCase())
+  }


Ah, didn't get that at first and thought that the dictionary changes from above would be a breaking change we would eventually life with. Just seeing here that the structure is kept "as is". Nice implementation! 😄

holgerd77 · 2020-07-06T07:55:08Z

packages/vm/lib/evm/opcodes.ts

-      0x3f: { name: 'EXTCODEHASH', fee: 700, isAsync: true }, // gas price change, EIP 1884
-      0x46: { name: 'CHAINID', fee: 2, isAsync: false }, // EIP 1344
-      0x47: { name: 'SELFBALANCE', fee: 5, isAsync: false }, // EIP 1884
-      0x54: { name: 'SLOAD', fee: 800, isAsync: true }, // gas price change, EIP 1884


Whew, really strange and a bit spooky that we have these gas price changes integrated here within this hardfork list creation structure, nice that this is factored out now.

holgerd77 · 2020-07-06T07:57:25Z

packages/common/src/hardforks/byzantium.json

+    },
+    "returndatacopy": {
+      "v": 3,
+      "d": "Base fee of the RETURNDATACOPY opcode"


Byzantium looks good, all opcodes included.

holgerd77 · 2020-07-06T08:05:47Z

packages/common/src/hardforks/constantinople.json

+    },
+    "create2": {
+      "v": 32000,
+      "d": "Base fee of the CREATE2 opcode"


Yes, Constantinople looks ok, was a bit confused (again, this will likely never end 😋 ) on the Constantinople/Petersburg relation and how this translates to here but this look ok from this angle as well (since it's Constantinople introducing all those opcodes), Petersburg is just reverting changes to SSTORE gas calculation.

holgerd77 · 2020-07-06T08:14:34Z

packages/common/src/hardforks/homestead.json

+      "v": 40,
+      "d": "Base fee of the DELEGATECALL opcode"
+    }
+  },


Yes, Homestead looks good as well.

holgerd77 · 2020-07-06T08:16:49Z

packages/common/src/hardforks/tangerineWhistle.json

+    },
+    "selfdestruct": {
+      "v": 5000,
+      "d": "Base fee of the SELFDESTRUCT opcode"


TangerineWhistle looks ok.

Taken from: https://eips.ethereum.org/EIPS/eip-150 (the only EIP included)

holgerd77 · 2020-07-06T08:18:16Z

packages/vm/lib/evm/opFns.ts

@@ -806,9 +773,6 @@ export const handlers: { [k: string]: OpHandler } = {
    runState.eei.finish(returnData)
  },
  REVERT: function (runState: RunState) {
-    if (!runState._common.gteHardfork('byzantium')) {
-      trap(ERROR.INVALID_OPCODE)
-    }
    const [offset, length] = runState.stack.popN(2)
    subMemUsage(runState, offset, length)
    let returnData = Buffer.alloc(0)


Had a short look here to give this an additional thought if there might be side effects (conclusion: no).

s1na

Good PR Jochem!

Just documenting a few tiny nitpicks I noticed for later on.

s1na · 2020-07-06T10:53:55Z

packages/vm/lib/evm/opcodes.ts

+    if (baseFee === undefined) {
+      throw new Error('base fee not defined for: ' + opcodeBuilder[key].name)
+    }
+    opcodeBuilder[key].fee = common.param('gasPrices', opcodeBuilder[key].name.toLowerCase())


opcodeBuilder[key].fee = baseFee

also it'd be great if someone could have a look at optimizing common.param (see #775)

s1na · 2020-07-06T10:54:09Z

packages/vm/lib/evm/opcodes.ts

-})
+  0xfe: { name: 'INVALID', isAsync: false },
+  0xff: { name: 'SELFDESTRUCT', isAsync: true },
+}

 // Array of hard forks in order. These changes are repeatedly applied to `opcodes` until the hard fork is in the future based upon the common
 // TODO: All gas price changes should be moved to common


this TODO can be removed

s1na · 2020-07-06T10:54:48Z

packages/vm/lib/evm/opcodes.ts

@@ -36,198 +36,194 @@ export interface OpcodeList {
 }

 // Base opcode list. The opcode list is extended in future hardforks
-const opcodes: OpcodeList = createOpcodes({
+const opcodes = {


can later on be renamed to chainstartOpcodes for clarification (after support for other old HFs is added)

holgerd77 · 2020-07-06T11:26:50Z

packages/vm/lib/evm/opcodes.ts


  for (let fork = 0; fork < hardforkOpcodes.length; fork++) {
    if (common.gteHardfork(hardforkOpcodes[fork].hardforkName)) {
      opcodeBuilder = { ...opcodeBuilder, ...hardforkOpcodes[fork].opcodes }
    }
  }

-  return opcodeBuilder
+  for (let key in opcodeBuilder) {
+    let baseFee = common.param('gasPrices', opcodeBuilder[key].name.toLowerCase())


Post-review note: this toLowerCase() is a pretty dangerous assumption - since Common is using camelCase for multi-word parameter names - I actually wonder that this is working at all on second thought since Common throws on using the wrong case. Probably some lucky coincidence on what values are read in this context. 😄

We should definitely improve here otherwise this is bound to go wrong at some PIT. Not sure how to tackle:

Ignore case on the the common side respectively only compare lower case versions? Side effects?

Use the upcoming release on Common and move everything to lowercase and enforce? Hmm.

Other suggestions?

Not completely convinced myself on any of the solutions proposed yet.

I created a quick script to extract opcode fees from the opcodes object and then lower cased them since that seemed to be the standard in Common (had sload gasprice instead of sLoad or something different). But I agree we should standardize this or something, good point.

The thing is, lower case is NOT standard in Common but camelCase and camelCase names are all over the place - see e.g. the chainstart.json file. I would guess that this didn't trigger an exception here is out of pure randomness, maybe all the camelCase names are for non-base fee gas prices or precompiles (like ecAdd e.g.), but that was likely pure luck here. 😄

Therefore the somewhat urgency of the comment. Or are we talking past each other somehow?

I understand your concern. I think we can change it to camelCase just to be consistent, but to implement this in a clean way in opcodes.ts we also need to change all uppercase opcodes there to the right camelCased opcodes.

jochem-brouwer added PR state: needs review package: vm package: common labels Jul 3, 2020

jochem-brouwer requested review from holgerd77 and s1na July 3, 2020 21:05

jochem-brouwer marked this pull request as draft July 3, 2020 21:19

jochem-brouwer removed request for holgerd77 and s1na July 3, 2020 21:19

jochem-brouwer added PR state: WIP and removed PR state: needs review labels Jul 3, 2020

jochem-brouwer requested review from holgerd77 and s1na July 3, 2020 22:18

jochem-brouwer added PR state: needs review and removed PR state: WIP labels Jul 3, 2020

jochem-brouwer marked this pull request as ready for review July 3, 2020 22:18

[VM/Common] move gas base fees to Common

2bef38c

[VM] remove unneeded opcode entries which only introduced gas changes [VM/Common] Add right fork base fees, remove STATICCALL from base opcodes [Common] remove STATICCALL from chainstart

jochem-brouwer force-pushed the move-base-gascost-common branch from b76ad58 to 2bef38c Compare July 3, 2020 22:36

Merge branch 'master' into move-base-gascost-common

2423cf3

holgerd77 approved these changes Jul 6, 2020

View reviewed changes

holgerd77 merged commit 195be3b into master Jul 6, 2020

holgerd77 deleted the move-base-gascost-common branch July 6, 2020 08:20

s1na reviewed Jul 6, 2020

View reviewed changes

holgerd77 reviewed Jul 6, 2020

View reviewed changes

holgerd77 mentioned this pull request Aug 12, 2020

Implement EIP-2315: subroutines #754

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VM/Common] move gas base fees to Common #806

[VM/Common] move gas base fees to Common #806

jochem-brouwer commented Jul 3, 2020 •

edited

Loading

codecov bot commented Jul 3, 2020 •

edited

Loading

jochem-brouwer commented Jul 3, 2020

holgerd77 left a comment

holgerd77 Jul 6, 2020

holgerd77 Jul 6, 2020

holgerd77 Jul 6, 2020

holgerd77 Jul 6, 2020

holgerd77 Jul 6, 2020

holgerd77 Jul 6, 2020

holgerd77 Jul 6, 2020

holgerd77 Jul 6, 2020

s1na left a comment

s1na Jul 6, 2020

s1na Jul 6, 2020

s1na Jul 6, 2020

holgerd77 Jul 6, 2020

jochem-brouwer Jul 6, 2020

holgerd77 Jul 6, 2020

jochem-brouwer Jul 6, 2020

[VM/Common] move gas base fees to Common #806

[VM/Common] move gas base fees to Common #806

Conversation

jochem-brouwer commented Jul 3, 2020 • edited Loading

codecov bot commented Jul 3, 2020 • edited Loading

Codecov Report

jochem-brouwer commented Jul 3, 2020

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1na left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochem-brouwer commented Jul 3, 2020 •

edited

Loading

codecov bot commented Jul 3, 2020 •

edited

Loading