common: Relocate geth genesis, state parsers to common, blockchain respectively #2300

g11tech · 2022-09-21T08:55:15Z

Relocate geth genesis, state parsers to common, blockchain respectively

Partially addresses #1708 as parseGethGenesisState can set these objects

code
storage

Nonces still need to be checked

Note (from @holgerd77): this also fixes a severe bug in the Blockchain.create() method.

…spectively

codecov · 2022-09-21T08:58:51Z

Codecov Report

Merging #2300 (9b80d91) into master (5ca079f) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Additional details and impacted files

Flag	Coverage Δ
block	`92.82% <ø> (ø)`
blockchain	`89.40% <100.00%> (+0.93%)`	⬆️
client	`86.96% <ø> (-0.03%)`	⬇️
common	`98.45% <100.00%> (+0.36%)`	⬆️
devp2p	`92.37% <ø> (-0.09%)`	⬇️
ethash	`∅ <ø> (∅)`
evm	`79.23% <ø> (ø)`
rlp	`?`
statemanager	`88.43% <ø> (ø)`
trie	`90.36% <ø> (ø)`
tx	`97.98% <ø> (ø)`
util	`91.67% <ø> (ø)`
vm	`85.31% <ø> (ø)`

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

holgerd77 · 2022-09-21T12:23:12Z

packages/common/README.md

+})
+// You either have genesis block hash or calculate it from using the geth state via `blockchain` module
+// const genesisBlockHash = ...
+common.setForkHashes(genesisBlockHash)


Hi Gajinder,
this looks already pretty great all over the place! 😄

Just for context/understanding: are fork hashes generally not included in genesis.json files? 🤔 Or is this just an "emergency mode" addition in case they are not included but normally they are?

hey @holgerd77 , thanks ❤️ the geth genesis doesn't has any forkHashes, so yes this would need to be set explicitly for custom chains based out of geth configs, and also can work as the fallback as you mentioned 👍

holgerd77 · 2022-09-21T12:51:09Z

Can you add at least one test in Common directly using the new functionality?

Also just to mention: when I suggested this I thought about adding a new static constructor like Common.fromGethConfig() or the like. If this has no logic hickups I would still find this a bit more elegant, but I can also life with the current solution. Also to mention that I guess we can still always additionally add such a constructor afterwards in addition to the existing helper function (that might generally be good to have both ways/options for flexibility).

Just some thoughts, if you also think this is a good idea to streamline things feel free to additionally add such a constructor if this doesn't collide with option types or the like. If you don't then just leave for now.

g11tech · 2022-09-21T12:58:35Z

Can you add at least one test in Common directly using the new functionality?

Also just to mention: when I suggested this I thought about adding a new static constructor like Common.fromGethConfig() or the like. If this has no logic hickups I would still find this a bit more elegant, but I can also life with the current solution. Also to mention that I guess we can still always additionally add such a constructor afterwards in addition to the existing helper function (that might generally be good to have both ways/options for flexibility).

Just some thoughts, if you also think this is a good idea to streamline things feel free to additionally add such a constructor if this doesn't collide with option types or the like. If you don't then just leave for now.

sounds good @holgerd77 🙂 will update 👍 and add some tests as well

g11tech · 2022-09-21T21:07:29Z

added a test for kiln genesis's block root match (sourced from an old geth build), seems to be not matching 🤔 , currently debugging for fix

holgerd77 · 2022-09-22T09:13:27Z

packages/blockchain/src/genesisStates/index.ts

@@ -36,7 +36,7 @@ export async function genesisStateRoot(genesisState: GenesisState) {
        account.codeHash = Buffer.from(keccak256(toBuffer(code)))
      }
      if (storage !== undefined) {
-        const storageTrie = new Trie()
+        const storageTrie = new Trie({ useKeyHashing: true })


Oh. The v6 final release clean-up bugs keep trickling in (still on a very modest level though).

Great find! 🙂

yea it took quite a digging to find this one 😅

Just following up the call-path here,so this is actually not such a "mild" bug being triggered directly in Blockchain.create() in certain circumstances, right? 🤔 "Only" in genesis storage scenarios, but anyhow. Guess some bugfix releases are really getting closer to be realized/done at some point.

yes in genesis storage scenarios only! thats why i digged around and used kiln genesis (since i wanted to incorporate a little complex genesis and luckily found this bug) and corresponding genesis block ( had to find an old geth build to get it 😅 )

g11tech · 2022-09-22T09:40:07Z

@holgerd77 PR should be good to merge 🙏

holgerd77

Nice, looks really really great, we have been waiting for such a functionality/constructor for quite some time, @ryanio would/will love this as well 🙂, quite some pre-work being done to enable this and this will really ease (expand on) use cases for the other libraries (easily instantiate a Block from a custom chain where you only have a genesis.json file and the block RPC e.g., these kind of things).

So, great! Will merge.

holgerd77 · 2022-09-22T10:53:10Z

packages/blockchain/test/utils.spec.ts

+      'kiln genesis hash matches'
+    )
+  })
+})


Nice tests. 👍

holgerd77 · 2022-09-22T10:57:08Z

packages/common/README.md

+// post calculating it via `blockchain`)
+common.setForkHashes(genesisHash)
+```
+


holgerd77 · 2022-09-22T11:02:02Z

packages/common/tests/utils.spec.ts

+
+tape('[Utils/Parse]', (t) => {
+  t.test('should parse geth params file', async (t) => {
+    const json = require(`../../client/test/testdata/geth-genesis/testnet.json`)


Hmm. Won't make it a blocker here but this is a bit dangerous.

So if someone reorganizes test data in client, this will (at least locally) remain unnoticed when someone is working "client only" and running the client tests and the like. Will break on CI then so somewhat ok, but nevertheless we should generally avoid and in doubt rather take over.

At some point we might also want to consolidate testdata a bit more (or at least discuss), and do a dedicated directory testdata on packages folder level or the like and in turn keep the test data a bit more curated and high quality level. Just some thoughts for now though.

common: Relocate geth genesis, state parsers to common, blockchain re…

9598510

…spectively

lint utils

7b99029

g11tech marked this pull request as draft September 21, 2022 10:54

relocate setForkHashes

8f4484b

g11tech marked this pull request as ready for review September 21, 2022 12:13

g11tech added PR state: merge ready package: blockchain package: common package: client target: master labels Sep 21, 2022

holgerd77 reviewed Sep 21, 2022

View reviewed changes

update readme

bad08aa

g11tech mentioned this pull request Sep 21, 2022

Early Pre-Shanghai EthereumJS Ephemeral Testnet #2298

Closed

11 tasks

g11tech added 7 commits September 21, 2022 21:10

use Common.fromGethGenesis wherever possible

fccb8b8

relocate parseGethGenesis tests

90f05df

remove unnecessary await

f583c3f

fix tests access across common and client

11ea0e6

provide abs path

6164173

add test for parseGethGenesisState

262af6b

add test cases for setcommon hashes and correct kiln genesis root

f2ecc55

add stateroot match test

b69b335

g11tech added PR state: WIP and removed PR state: merge ready labels Sep 22, 2022

fix the storage trie to also use key hashing

9b80d91

g11tech added PR state: merge ready and removed PR state: WIP labels Sep 22, 2022

holgerd77 reviewed Sep 22, 2022

View reviewed changes

holgerd77 approved these changes Sep 22, 2022

View reviewed changes

holgerd77 merged commit 3712a1b into master Sep 22, 2022

holgerd77 deleted the g11tech/relocate-geth-config-parsers branch September 22, 2022 11:05

holgerd77 mentioned this pull request May 23, 2023

Common: Support Geth Config #1708

Closed

This was referenced Aug 20, 2023

[Snyk] Upgrade typedoc from 0.23.10 to 0.24.8 Woodpile37/ethereumjs-monorepo#5

Merged

[Snyk] Upgrade typedoc from 0.23.10 to 0.24.8 Woodpile37/ethereumjs-monorepo#40

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

common: Relocate geth genesis, state parsers to common, blockchain respectively #2300

common: Relocate geth genesis, state parsers to common, blockchain respectively #2300

g11tech commented Sep 21, 2022 •

edited by holgerd77

Loading

codecov bot commented Sep 21, 2022 •

edited

Loading

holgerd77 Sep 21, 2022

g11tech Sep 21, 2022

holgerd77 commented Sep 21, 2022

g11tech commented Sep 21, 2022

g11tech commented Sep 21, 2022

holgerd77 Sep 22, 2022

g11tech Sep 22, 2022

holgerd77 Sep 22, 2022

g11tech Sep 22, 2022 •

edited

Loading

g11tech commented Sep 22, 2022

holgerd77 left a comment

holgerd77 Sep 22, 2022

holgerd77 Sep 22, 2022

holgerd77 Sep 22, 2022

common: Relocate geth genesis, state parsers to common, blockchain respectively #2300

common: Relocate geth genesis, state parsers to common, blockchain respectively #2300

Conversation

g11tech commented Sep 21, 2022 • edited by holgerd77 Loading

codecov bot commented Sep 21, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Sep 21, 2022

g11tech commented Sep 21, 2022

g11tech commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g11tech Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

g11tech commented Sep 22, 2022

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

g11tech commented Sep 21, 2022 •

edited by holgerd77

Loading

codecov bot commented Sep 21, 2022 •

edited

Loading

g11tech Sep 22, 2022 •

edited

Loading