Add forkhash support to Common library #765

holgerd77 · 2020-06-05T10:09:02Z

This PR adds forkhash support to the Common library in preparation for eth/64 support for the devp2p library as outlined in ethereumjs/ethereumjs-devp2p#55.

It does so by adding a new Common.forkHash() method which either returns the forkHash for a HF from a chain file or calculates via the newly added internal _calcForkHash() function.

I didn't add a Common.forkHashes() method yet as proposed here. I am not completely sure about the output format.

Such a method can be added later if needed along the integration work on the devp2p library.

Fork hashes hardcoded in the chain files are validated with the _calcForkHash() function in the tests.

codecov · 2020-06-05T10:11:36Z

Codecov Report

Merging #765 into master will decrease coverage by 0.02%.
The diff coverage is 92.00%.

@@            Coverage Diff             @@
##           master     #765      +/-   ##
==========================================
- Coverage   92.02%   91.99%   -0.03%     
==========================================
  Files          47       44       -3     
  Lines        2995     3011      +16     
  Branches      469      472       +3     
==========================================
+ Hits         2756     2770      +14     
- Misses        144      145       +1     
- Partials       95       96       +1

Flag	Coverage Δ
#account	`94.11% <ø> (ø)`
#block	`88.09% <ø> (ø)`
#blockchain	`88.74% <ø> (ø)`
#common	`93.52% <92.00%> (-0.63%)`	⬇️
#tx	`94.16% <ø> (ø)`
#vm	`93.09% <ø> (ø)`

Impacted Files	Coverage Δ
packages/common/src/index.ts	`93.52% <92.00%> (-0.35%)`	⬇️
packages/common/src/hardforks/index.ts
packages/common/src/chains/index.ts
packages/common/src/genesisStates/index.ts

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 6ac21f6...b08d076. Read the comment docs.

evertonfraga · 2020-06-05T12:02:57Z

@holgerd77 do you have the forkHash references to speed up review?

Edit: got most of forkIds from geth implementaion: https://github.com/ethereum/go-ethereum/blob/15540ae9928441a38f8af47d8227347e6bada5ae/core/forkid/forkid_test.go#L41-L116

evertonfraga · 2020-06-05T11:59:28Z

packages/common/package.json

@@ -63,5 +63,7 @@
      "email": "Holger.Drewes@gmail.com"
    }
  ],
-  "dependencies": {}
+  "dependencies": {
+    "crc-32": "^1.2.0"


evertonfraga · 2020-06-05T12:30:42Z

packages/common/src/chains/goerli.json

+      "block": 0,
+      "forkHash": "0xa3f5ab08"
    },
    {
      "name": "byzantium",
-      "block": 0
+      "block": 0,
+      "forkHash": "0xa3f5ab08"
    },
    {
      "name": "constantinople",
-      "block": 0
+      "block": 0,
+      "forkHash": "0xa3f5ab08"
    },
    {
      "name": "petersburg",
-      "block": 0
+      "block": 0,
+      "forkHash": "0xa3f5ab08"
    },
    {
      "name": "istanbul",
-      "block": 1561651
+      "block": 1561651,
+      "forkHash": "0xc25efa5c"
    },
    {
      "name": "berlin",
-      "block": null
+      "block": null,
+      "forkHash": null


👍 It matches Geth's goerli: https://github.com/ethereum/go-ethereum/blob/15540ae9928441a38f8af47d8227347e6bada5ae/core/forkid/forkid_test.go#L112-L115

evertonfraga · 2020-06-05T13:01:00Z

packages/common/src/chains/rinkeby.json

+      "block": 0,
+      "forkHash": "0x3b8e0691"
    },
    {
      "name": "homestead",
-      "block": 1
+      "block": 1,
+      "forkHash": "0x60949295"
    },
    {
      "name": "dao",
-      "block": null
+      "block": null,
+      "forkHash": null
    },
    {
      "name": "tangerineWhistle",
-      "block": 2
+      "block": 2,
+      "forkHash": "0x8bde40dd"
    },
    {
      "name": "spuriousDragon",
-      "block": 3
+      "block": 3,
+      "forkHash": "0xcb3a64bb"
    },
    {
      "name": "byzantium",
-      "block": 1035301
+      "block": 1035301,
+      "forkHash": "0x8d748b57"
    },
    {
      "name": "constantinople",
-      "block": 3660663
+      "block": 3660663,
+      "forkHash": "0xe49cab14"
    },
    {
      "name": "petersburg",
-      "block": 4321234
+      "block": 4321234,
+      "forkHash": "0xafec6b27"
    },
    {
      "name": "istanbul",
-      "block": 5435345
+      "block": 5435345,
+      "forkHash": "0xcbdb8838"
    },
    {
      "name": "berlin",
-      "block": null
+      "block": null,
+      "forkHash": null


OK. double checked against geth implementation.
https://github.com/ethereum/go-ethereum/blob/15540ae9928441a38f8af47d8227347e6bada5ae/core/forkid/forkid_test.go#L92-L104

Rinkeby does not have DAO fork.

evertonfraga · 2020-06-05T13:11:42Z

packages/common/src/chains/mainnet.json

+      "block": 7280000,
+      "forkHash": "0x668db0af"
    },
    {
      "name": "petersburg",
-      "block": 7280000
+      "block": 7280000,
+      "forkHash": "0x668db0af"


OK, they share the same forkHash.

evertonfraga · 2020-06-05T13:21:41Z

packages/common/src/chains/mainnet.json

    },
    {
      "name": "berlin",
-      "block": null
+      "block": null,
+      "forkHash": null


no forkHash yet. OK

evertonfraga · 2020-06-05T13:25:35Z

packages/common/src/chains/ropsten.json

+      "block": 0,
+      "forkHash": "0x30c7ddbc"
    },
    {
      "name": "homestead",
-      "block": 0
+      "block": 0,
+      "forkHash": "0x30c7ddbc"
    },
    {
      "name": "dao",
-      "block": null
+      "block": null,
+      "forkHash": null
    },
    {
      "name": "tangerineWhistle",
-      "block": 0
+      "block": 0,
+      "forkHash": "0x30c7ddbc"
    },
    {
      "name": "spuriousDragon",
-      "block": 10
+      "block": 10,
+      "forkHash": "0x63760190"
    },
    {
      "name": "byzantium",
-      "block": 1700000
+      "block": 1700000,
+      "forkHash": "0x3ea159c7"
    },
    {
      "name": "constantinople",
-      "block": 4230000
+      "block": 4230000,
+      "forkHash": "0x97b544f3"
    },
    {
      "name": "petersburg",
-      "block": 4939394
+      "block": 4939394,
+      "forkHash": "0xd6e2149b"
    },
    {
      "name": "istanbul",
-      "block": 6485846
+      "block": 6485846,
+      "forkHash": "0x4bc66396"
    },
    {
      "name": "muirGlacier",
-      "block": 7117117
+      "block": 7117117,
+      "forkHash": "0x6727ef90"
    },
    {
      "name": "berlin",
-      "block": null
+      "block": null,
+      "forkHash": null


OK. double checked against geth implementation: https://github.com/ethereum/go-ethereum/blob/15540ae9928441a38f8af47d8227347e6bada5ae/core/forkid/forkid_test.go#L71-L84

packages/common/src/index.ts

…h method

holgerd77 · 2020-06-05T15:36:42Z

@evertonfraga ok, hopefully have addressed everything 😄 , ready for re-review.

evertonfraga · 2020-06-05T16:11:14Z

packages/common/src/index.ts

@@ -129,8 +129,7 @@ export default class Common {
   * @param hardfork Hardfork given to function as a parameter
   * @returns Hardfork chosen to be used
   */
-  _chooseHardfork(hardfork?: string | null, onlySupported?: boolean): string {
-    onlySupported = onlySupported === undefined ? true : onlySupported
+  _chooseHardfork(hardfork?: string | null, onlySupported: boolean = true): string {


This change does not alter the final signature. The compiler implies that setting a default value means that the parameter is optional.

Thanks, yeah, makes sense! 👍

evertonfraga

@holgerd77 this is good to go! I liked how you did the calc function, so it can be used as a future-proof mechanism while also fulfill custom chain requirements.

I don't know how expensive CRC32 execution is, but down the road, if it's expressive enough, memoizing forkHash() would save resources for cases other than those represented in this repo.

holgerd77 added 3 commits June 5, 2020 12:01

Added forkHash() method to Common class

8ed8d88

Updated chain files with forkHash property

fabbf95

Added tests for forkHash() and internal _calcForkHash() functions

9fec99d

holgerd77 added PR state: needs review package: common labels Jun 5, 2020

holgerd77 requested review from ryanio and evertonfraga June 5, 2020 10:09

Fixed linting errors

b4cb409

evertonfraga suggested changes Jun 5, 2020

View reviewed changes

Used native String.prototype.padStart() method in Common._calcForkHas…

cc25754

…h method

refactor: sets default value on fn signature, removing manual check.

b08d076

evertonfraga reviewed Jun 5, 2020

View reviewed changes

evertonfraga self-requested a review June 5, 2020 16:47

evertonfraga approved these changes Jun 5, 2020

View reviewed changes

holgerd77 merged commit 28b8fa4 into master Jun 5, 2020

holgerd77 deleted the add-forkhash-support branch June 5, 2020 17:57

evertonfraga added this to the VM v5 milestone Jun 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add forkhash support to Common library #765

Add forkhash support to Common library #765

holgerd77 commented Jun 5, 2020

codecov bot commented Jun 5, 2020 •

edited

Loading

evertonfraga commented Jun 5, 2020 •

edited

Loading

evertonfraga Jun 5, 2020

evertonfraga Jun 5, 2020

evertonfraga Jun 5, 2020

evertonfraga Jun 5, 2020

evertonfraga Jun 5, 2020

evertonfraga Jun 5, 2020

holgerd77 commented Jun 5, 2020

evertonfraga Jun 5, 2020

holgerd77 Jun 5, 2020

evertonfraga left a comment

Add forkhash support to Common library #765

Add forkhash support to Common library #765

Conversation

holgerd77 commented Jun 5, 2020

codecov bot commented Jun 5, 2020 • edited Loading

Codecov Report

evertonfraga commented Jun 5, 2020 • edited Loading

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

holgerd77 commented Jun 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evertonfraga left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 5, 2020 •

edited

Loading

evertonfraga commented Jun 5, 2020 •

edited

Loading