common/block/blockchain: genesis refactor #1916

ryanio · 2022-05-27T00:56:18Z

This PR works on the ideas presented in #1833, mainly #1833 (comment) and #1833 (comment).

This PR:

Removes stateRoot and hash from Common
Moves genesis functionality from Block to Blockchain
Moves genesisStates from Common to Blockchain
RemovesVM.runBlockchain()
- unused, except for in BlockchainTestsRunner where we now directly putBlocks and run the blockchain iterator

I haven't run the test suite yet to fix failing tests as I wanted some confirmation that we want to proceed on this path before working on the final touches.

The only downside so far (that I see) is that when setting up a custom chain you used to only need a very portable common but now you have to additionally pass the genesis states into blockchain, but this was easy enough to adjust to in the client by passing an optional blockchain when custom genesis states (or geth genesis) is provided.

Removing stateRoot and hash was also tricky in a few places but I think I overcame the challenges.

codecov · 2022-05-27T01:14:03Z

Codecov Report

Merging #1916 (5d90fff) into develop (7765b7e) will increase coverage by 1.80%.
The diff coverage is 67.44%.

❗ Current head 5d90fff differs from pull request most recent head 803300a. Consider uploading reports for the commit 803300a to get more accurate results

Flag	Coverage Δ
block	`86.15% <100.00%> (+0.86%)`	⬆️
blockchain	`81.32% <54.54%> (-1.82%)`	⬇️
client	`?`
common	`95.01% <86.95%> (-0.56%)`	⬇️
devp2p	`?`
ethash	`90.81% <ø> (ø)`
statemanager	`84.87% <ø> (+0.62%)`	⬆️
trie	`80.19% <ø> (ø)`
tx	`92.09% <ø> (-0.05%)`	⬇️
util	`87.29% <ø> (ø)`
vm	`81.09% <100.00%> (+0.01%)`	⬆️

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

holgerd77 · 2022-05-27T10:13:04Z

Thanks so much. 🙂

I think I am a fan of continuing on this.

ryanio · 2022-05-28T16:57:38Z

Have to look into two ci issues, vm-api browser run timing out (taking too long?) and client cli connection. The PR can still be reviewed for everything else meanwhile.
edit: now resolved

ryanio · 2022-05-31T01:46:32Z

This is ready for review :)

holgerd77 · 2022-05-31T09:50:29Z

Oh, 1 commit 😬 (thanks a thousand times, otherwise totally great that you got through this!!!).

This will be a bit harsh for review, would have preferred to have this step-by-step on such a large change set. But will try my best. 😋

holgerd77

Ok, this is such a convicing refactor (and it actually looks good on all fronts! 🙂 👍), I will directly take this in.

So so great, now seeing this I totally love the outlook (even more) of finally having these light-weight lower level libraries. Especially for tx this is a great win.

Will merge.

holgerd77 · 2022-05-31T09:58:20Z

packages/block/src/header.ts

-      ) {
-        baseFeePerGas = BigInt(this._common.genesis().baseFeePerGas!)
-      }
-    }


Ok, I wondered a bit along seeing this how much people are using this functionality and if they will or will not miss this, but: I guess in case this is requested, we can very well add some less intrusive helpers (e.g. - similar to what we thought about for Common, allow passing in a genesis json + stateRoot + hash to create a block), which would be a non-breaking addition. So I think we can go with this.

holgerd77 · 2022-05-31T10:00:13Z

packages/block/test/block.spec.ts

+    const rlp = Buffer.from(testDataGenesis.test.genesis_rlp_hex, 'hex')
+    const hash = Buffer.from(testDataGenesis.test.genesis_hash, 'hex')
+    const block = Block.fromRLPSerializedBlock(rlp, { common })
+    st.ok(block.hash().equals(hash), 'genesis hash match')


Ah, nice replacement. 🙂

holgerd77 · 2022-05-31T10:08:01Z

packages/blockchain/src/index.ts

+   * }
+   * ```
+   */
+  genesisState?: GenesisState


Ok, just followed this along just to get some confirmation: so if one now would want to run a VM with/upon some custom genesis state, one would create a Blockchain object first with this genesis state and passes this into the VM along initialization. Seems a reasonable replacement to me for this eventual use case.

holgerd77 · 2022-05-31T10:08:58Z

packages/blockchain/src/index.ts

@@ -115,7 +143,8 @@ export default class Blockchain implements BlockchainInterface {
  db: LevelUp
  dbManager: DBManager

-  private _genesis?: Buffer // the genesis hash of this blockchain
+  private _genesisBlock?: Block /** The genesis block of this blockchain */


This is actually also a nice replacement to have the Block here and not level down to just the hash (why?).

we have the whole block now so I figure it's nicer to cache it, as the hash can still be accessed, and it makes life easier e.g. in the client chain that returns genesis() as the genesis block. there might be more uses for the other fields in the future.

holgerd77 · 2022-05-31T10:09:35Z

packages/blockchain/src/index.ts

+        stateRoot = Buffer.from(
+          'd7f8974fb5ac78d9ac099b9ad5018bedc2ce0a72dad1827a1709da30580f0544',
+          'hex'
+        )


👍

Great optimization!

holgerd77 · 2022-05-31T10:16:35Z

packages/blockchain/test/index.spec.ts

    const blocks: Block[] = []
-    const genesisBlock = Block.genesis({ header: { gasLimit: 8000000 } })


Oh, impressive, that this can be so easily replaced in the tests by the generic functions, seems specifically using genesis() wasn't necessary at all here in most cases?

heh yeah since the block number still defaults to zero, we weren't losing much, as there were not very many cases where we needed the actual genesis params.

holgerd77 · 2022-05-31T10:26:30Z

packages/client/lib/blockchain/chain.ts

-    })
-    return genesisParams
+  get genesis() {
+    return this.blockchain.genesisBlock()


holgerd77 · 2022-05-31T10:27:28Z

packages/client/lib/net/protocol/ethprotocol.ts

+          max === 0 ? Buffer.from([]) : intToBuffer(max),
+          skip === 0 ? Buffer.from([]) : intToBuffer(skip),
+          !reverse ? Buffer.from([]) : Buffer.from([1]),
+        ],


Just out of interest and because I can't spot right away: why did this code needed to change in this context?

this is related to the RLP v3 changes, not sure why it just popped up in my PR, but when doing bufArrToArr we need all fields to be Buffer, otherwise if it is is a plain number it didn't convert correctly to Uint8Array.

holgerd77 · 2022-05-31T10:28:53Z

packages/client/lib/util/parse.ts

-  common.setHardforkByBlockNumber(0)
-  return BlockHeader.fromHeaderData(headerData, { common })
-}
-


Ugh. This is just falling off the cliff? How great is this? 😀

yeah! some great simplifications through the process here

holgerd77 · 2022-05-31T10:31:24Z

packages/common/src/common.ts

    }
-    return this._calcForkHash(hardfork)
+    if (!genesisHash) throw new Error('genesisHash required for forkHash calculation')
+    return this._calcForkHash(hardfork, genesisHash)
  }


That's a great modified forkHash version.

holgerd77 · 2022-05-31T10:41:15Z

Rebased this via UI after local test rebase.

ryanio added PR state: WIP package: block package: blockchain package: vm package: common package: client labels May 27, 2022

ryanio mentioned this pull request May 27, 2022

Common: Support custom hash & state root on genesis #1833

Closed

ryanio added the target: develop label May 27, 2022

ryanio force-pushed the common-blockchain-genesis-refactor branch from 1c65eb5 to 7ca78aa Compare May 27, 2022 01:41

ryanio force-pushed the common-blockchain-genesis-refactor branch from 7ca78aa to 30068c6 Compare May 28, 2022 00:30

ryanio added PR state: needs review and removed PR state: WIP labels May 28, 2022

ryanio marked this pull request as ready for review May 28, 2022 01:15

ryanio force-pushed the common-blockchain-genesis-refactor branch 3 times, most recently from 6e7785d to a84cc40 Compare May 31, 2022 00:48

ryanio requested a review from holgerd77 May 31, 2022 01:47

holgerd77 approved these changes May 31, 2022

View reviewed changes

common/blockchain genesis refactor

803300a

holgerd77 force-pushed the common-blockchain-genesis-refactor branch from a84cc40 to 803300a Compare May 31, 2022 10:40

holgerd77 merged commit e0a659d into develop May 31, 2022

holgerd77 deleted the common-blockchain-genesis-refactor branch May 31, 2022 11:02

ryanio mentioned this pull request May 31, 2022

develop: small fixes #1924

Merged

g11tech pushed a commit that referenced this pull request Jun 2, 2022

common/blockchain genesis refactor (#1916)

129ff5b

g11tech pushed a commit that referenced this pull request Jun 2, 2022

common/blockchain genesis refactor (#1916)

be7d0d1

g11tech pushed a commit that referenced this pull request Jun 2, 2022

common/blockchain genesis refactor (#1916)

2a7ed58

g11tech pushed a commit that referenced this pull request Jun 3, 2022

common/blockchain genesis refactor (#1916)

99a44bd

holgerd77 pushed a commit that referenced this pull request Jun 8, 2022

common/blockchain genesis refactor (#1916)

2165d23

This was referenced Jun 15, 2022

Common: Support Geth Config #1708

Closed

Common: Remove Genesis State & Functionality #1696

Closed

Monorepo: Analyze Bundle Sizes #1673

Open

holgerd77 mentioned this pull request Jul 6, 2022

Blockchain: add genesisState Option Test Case #2020

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

common/block/blockchain: genesis refactor #1916

common/block/blockchain: genesis refactor #1916

ryanio commented May 27, 2022

codecov bot commented May 27, 2022 •

edited

Loading

holgerd77 commented May 27, 2022

ryanio commented May 28, 2022 •

edited

Loading

ryanio commented May 31, 2022

holgerd77 commented May 31, 2022

holgerd77 left a comment

holgerd77 May 31, 2022

holgerd77 May 31, 2022

holgerd77 May 31, 2022

holgerd77 May 31, 2022

ryanio May 31, 2022

holgerd77 May 31, 2022

holgerd77 May 31, 2022

ryanio May 31, 2022 •

edited

Loading

holgerd77 May 31, 2022

holgerd77 May 31, 2022

ryanio May 31, 2022

holgerd77 May 31, 2022

ryanio May 31, 2022

holgerd77 May 31, 2022

holgerd77 commented May 31, 2022

		const blocks: Block[] = []
		const genesisBlock = Block.genesis({ header: { gasLimit: 8000000 } })

common/block/blockchain: genesis refactor #1916

common/block/blockchain: genesis refactor #1916

Conversation

ryanio commented May 27, 2022

codecov bot commented May 27, 2022 • edited Loading

Codecov Report

holgerd77 commented May 27, 2022

ryanio commented May 28, 2022 • edited Loading

ryanio commented May 31, 2022

holgerd77 commented May 31, 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

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

ryanio May 31, 2022 • 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

Choose a reason for hiding this comment

holgerd77 commented May 31, 2022

codecov bot commented May 27, 2022 •

edited

Loading

ryanio commented May 28, 2022 •

edited

Loading

ryanio May 31, 2022 •

edited

Loading