Client: add VM execution #1028

holgerd77 · 2021-01-02T11:13:09Z

Continuation of #1017

This has been rebased on top of #1023 and is working apart from one integration test failing (FullSync, this is going into an endless loop in the while loop in sync.start()).

Will give some more comments on the changes later the day.

We should merge relatively soon otherwise this is getting a bit out of hands. 😋

codecov · 2021-01-02T11:14:53Z

Codecov Report

Merging #1028 (dc2c99e) into master (01031f2) will increase coverage by 0.45%.
The diff coverage is 90.78%.

Flag	Coverage Δ
block	`77.92% <ø> (+0.27%)`	⬆️
blockchain	`77.92% <ø> (ø)`
client	`88.38% <90.56%> (+1.06%)`	⬆️
common	`91.87% <ø> (ø)`
devp2p	`82.34% <88.88%> (-0.01%)`	⬇️
ethash	`82.08% <ø> (ø)`
tx	`86.25% <ø> (ø)`
vm	`83.05% <92.85%> (ø)`

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

holgerd77 · 2021-01-02T11:47:48Z

Some more debugging: integration test failure is due to FullSynchronizer.best() not delivering a peer on the sync() call.

holgerd77 · 2021-01-02T11:56:03Z

This difficulty check (!best && td.gte(this.chain.blocks.td)) in best() is failing now (in contrast to before).

holgerd77 · 2021-01-02T16:07:55Z

Ok, tests are passing and client run is working both initially and resumed upon an existing chain state, so this is now ready for review. 😄

The devp2p changes from 8bfec9d are technically independent from this PR, I did this yesterday because sync start did take so long and often was unreliable so it got very much a hazzle to further debug the PR. The change is subdividing the peer findNeighboursrefresh calls, so now calls for all the peers are not done any more at the same time (given by the interval option) but calls are distributed (still keeping the interval time for a dedicated peer) throughout the time period with a segregation by a peer.id derived selector, I had this change in mind already for quite some time. This leads to a more distributed networking traffic (avoiding networking spikes switching with no-activity periods) and leads to a faster sync pick-up.

holgerd77 · 2021-01-05T10:19:19Z

Rebased this.

holgerd77 · 2021-01-05T13:12:36Z

This is now open for review (and merge), #1030 should subsequently automatically update its base.

holgerd77 · 2021-01-05T14:26:25Z

Rebased this on top of #1026

holgerd77 · 2021-01-06T08:29:51Z

Hi @jochem-brouwer, good morning 😀 , then only thing I would insist upon: can we please get this merged before you start rewriting the cache mechanism for trie? This is otherwise getting a bit hairy.

This PR shouldn't be too hard to review, especially since the core logic was rewritten by yourself, some explanations:

After rebasing upon your fetcher type improvements in 1fe5e03 I did the following:

The state DB is properly closed d984684 (line 248 in fullsync.ts, actually needs to be expanded to the the close() line, addition seems to have been done in a commit before that) in FullSynchronizer.stop(), follow-up correction in c7e32c8
State and chain DBs are better differentiated by renaming the original db -> chainDB and aligning state DB to also be an option in node.ts analog to db, 67aaf12 and baf5c33
There is now a more consistent and chain/state separating directory layout with c7079d9 so that we have the network data in folders like:
- /[HOME_DIR]/Ethereum/ethereumjs/mainnet/chain
- /[HOME_DIR]/Ethereum/ethereumjs/mainnet/state
- /[HOME_DIR]/Ethereum/ethereumjs/rinkeby/chain
- /[HOME_DIR]/Ethereum/ethereumjs/rinkeby/state
- ...
Devp2p DPT changes from d3b5856 are already explained in Client: add VM execution #1028 (comment). This is in the peer discovery mechanism, findNeighbours() looks for others peers to connect to based on the existing (initially: bootstrap) peers. The call to refresh() is now done 10 times more often with a subdivided refresh interval by const refreshIntervalSubdivided = Math.floor((options.refreshInterval || ms('60s')) / 10). In refresh() there is then a selector extracted from the peer ID with const selector = buffer2int(peer.id.slice(0, 1)) % 10, so this just gives a number 0..9 for each peer. On a refresh call there are then only peers called where this selector matches a looping 0..9 counter, selector === this._refreshIntervalSelectionCounter. All this is just to distribute the network traffic a bit better, before there were all calls together in one moment in time (every refreshInterval seconds). Hope this is a sufficient explanation for review. Peer discovery in the client is still working as before with this (actually: substantially better, therefore the change)
The remaining commits are just some added tests and some minor fixes

jochem-brouwer

I got some questions, but nothing alarming. Looks good! 😄

packages/client/lib/client.ts

packages/client/lib/config.ts

packages/client/lib/sync/fullsync.ts

jochem-brouwer · 2021-01-06T16:36:13Z

packages/client/lib/sync/sync.ts

@@ -47,6 +52,7 @@ export abstract class Synchronizer extends EventEmitter {

    this.pool = options.pool
    this.chain = options.chain
+    this.stateDB = options.stateDB


I don't think we need stateDB here, since it does not seem we use it anywhere?

I put it there and not directly in FullSynchronizer since I thought this might be more flexible if we want to use stateDB in other synchronizers. Do you want me to move over to FullSynchronizer directly? Don't have a strong opinion on this myself, think these are all things which can be easily evolved and adopted further down the road.

I have no strong opinion either, it is OK to keep it here for now, if we focus on the light client then we can remove it if it turns out it is unnecessary.

packages/vm/lib/runBlock.ts

…ance for a clear synchronizer object association

…ded state persistence by introducing StateManager

vm: add maxBlocks option to runBlockchain

client: fullsync: ensure VM executes final block upon shutdown

…uest) along import/execution decoupling, added tx number in block to execution message

…cks with 0 txs (performance)

…ctivity problems

…ferentiation towards stateDB

… directory structures

…ed missed out test base directory tests (e.g. test/client.spec.ts) to test:unit package.json command

…ls for improved discovery performance and to avoid network traffic spikes

… linting fixes

…r flow internet connections, fixed rebase bug in DPT.refresh()

…sh coalescent operator for options.refreshInterval selection in DPT

…, removed checkpointing exception for zero tx blocks in VM.runBlocks()

holgerd77 · 2021-01-06T18:05:09Z

@jochem-brouwer ok, just pushed the changes, ready for re-review. 😀

ryanio · 2021-01-06T23:32:10Z

packages/client/lib/sync/fullsync.ts

+
+  // Tracking vars for log msg condensation on zero tx blocks
+  private NUM_ZERO_TXS_PER_LOG_MSG = 50
+  public zeroTxsBlockLogMsgCounter: number = 0


nit: I don't think need the number type annotation since it's being initialized with 0

ryanio · 2021-01-06T23:34:54Z

packages/devp2p/src/dpt/dpt.ts

@@ -55,6 +55,7 @@ export class DPT extends EventEmitter {
  private _kbucket: KBucket
  private _server: DPTServer
  private _refreshIntervalId: NodeJS.Timeout
+  private _refreshIntervalSelectionCounter: number = 0


same nit here for number type annotation, I believe not needed when already initialized with a number

jochem-brouwer

Yup, great!! 😄

jochem-brouwer · 2021-01-07T15:42:31Z

I assume that first this one gets moved into master and then the checkpoint one immediately after?

holgerd77 · 2021-01-07T15:58:36Z

@jochem-brouwer thanks for the review 😄 , yes, I will now first merge here.

holgerd77 added PR state: WIP package: devp2p package: client labels Jan 2, 2021

holgerd77 mentioned this pull request Jan 2, 2021

Client: add VM execution (old) #1017

Closed

holgerd77 added the package: vm label Jan 2, 2021

holgerd77 added PR state: needs review and removed PR state: WIP labels Jan 2, 2021

holgerd77 requested review from ryanio, jochem-brouwer and cgewecke January 2, 2021 15:56

This was referenced Jan 4, 2021

Trie: Rework Checkpointing Mechanism #1030

Merged

Devp2p DPT Type Improvements #1029

Merged

holgerd77 force-pushed the client-add-vm-execution-rebase branch from 8f506ae to fb2826d Compare January 5, 2021 10:17

holgerd77 force-pushed the client-add-vm-execution-rebase branch from a8d7cca to 1d4ca4f Compare January 5, 2021 14:25

jochem-brouwer reviewed Jan 6, 2021

View reviewed changes

holgerd77 and others added 7 commits January 6, 2021 18:18

client -> vm execution: removed headerfetcher -> blockfetcher inherit…

04e9de1

…ance for a clear synchronizer object association

client -> vm execution: moved VM to BlockFetcher, added VM execution

8116cda

client -> vm execution: use vm.runBlockchain(), fix execution run, ad…

416597f

…ded state persistence by introducing StateManager

client: move VM and block execution logic to FullSync

3f373e2

vm: add maxBlocks option to runBlockchain

client: fullsync: handle runBlocks concurrency

8ad02f8

client: fullsync: ensure VM executes final block upon shutdown

client -> vm execution: went back to 250 max import blocks (maxPerReq…

451bba3

…uest) along import/execution decoupling, added tx number in block to execution message

client, vm -> vm execution: removed checkpointing in runBlock for blo…

22af4af

…cks with 0 txs (performance)

holgerd77 added 19 commits January 6, 2021 18:18

client -> vm execution: added condensed zero txs block execution log msg

75daf4b

client -> vm execution: fixed linting in vm

709aa59

client -> vm execution: lowered maxPerRequest back to 50 due to conne…

08e15ba

…ctivity problems

client -> vm execution: fixed oldHead <-> newHead semantic switch

9cc4ae9

client -> VM execution: fixed VM test

79e1095

client -> VM execution: rebase (fetcher typesafe improvements) fixes

7c68d3f

client -> VM execution: close stateDB, fixed fullsync tests

727304a

client -> VM execution: renamed chain db instances to chainDB for dif…

2bd2ff1

…ferentiation towards stateDB

client -> VM execution: made stateDB a client option analogue to chainDB

475ccfc

client -> VM execution: unified and consolidated chain and state data…

c0ec528

… directory structures

client -> VM execution: added some Config directory method tests, add…

33fd42a

…ed missed out test base directory tests (e.g. test/client.spec.ts) to test:unit package.json command

client -> VM execution: added simple runBlocks test

a29ddc5

client -> VM execution: added more runBlocks() test cases

4d2680a

client, devp2p -> VM execution: subdivided DPT findneighbour peer cal…

e2817fe

…ls for improved discovery performance and to avoid network traffic spikes

client -> VM execution: made state DB closing more resilient

54e1a78

client -> VM execution: fixed full-/lightsync integration tests, some…

28733ac

… linting fixes

client, devp2p -> VM execution: increased protocol timeout to 4000 fo…

4b2d1cc

…r flow internet connections, fixed rebase bug in DPT.refresh()

client, devp2p -> VM execution: fixed devp2p linting error, use nulli…

bd32047

…sh coalescent operator for options.refreshInterval selection in DPT

client, vm -> vm execution: removed stateDB from LightEthereumService…

dc2c99e

…, removed checkpointing exception for zero tx blocks in VM.runBlocks()

holgerd77 force-pushed the client-add-vm-execution-rebase branch from 1d4ca4f to dc2c99e Compare January 6, 2021 17:56

ryanio reviewed Jan 6, 2021

View reviewed changes

jochem-brouwer approved these changes Jan 7, 2021

View reviewed changes

holgerd77 merged commit ec0f059 into master Jan 7, 2021

holgerd77 deleted the client-add-vm-execution-rebase branch January 7, 2021 16:02

holgerd77 mentioned this pull request Feb 15, 2021

Client run early on VM tests #983

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client: add VM execution #1028

Client: add VM execution #1028

holgerd77 commented Jan 2, 2021

codecov bot commented Jan 2, 2021 •

edited

Loading

holgerd77 commented Jan 2, 2021

holgerd77 commented Jan 2, 2021

holgerd77 commented Jan 2, 2021

holgerd77 commented Jan 5, 2021

holgerd77 commented Jan 5, 2021

holgerd77 commented Jan 5, 2021

holgerd77 commented Jan 6, 2021

jochem-brouwer left a comment

jochem-brouwer Jan 6, 2021

holgerd77 Jan 6, 2021

jochem-brouwer Jan 6, 2021

holgerd77 commented Jan 6, 2021

ryanio Jan 6, 2021

ryanio Jan 6, 2021

jochem-brouwer left a comment

jochem-brouwer commented Jan 7, 2021

holgerd77 commented Jan 7, 2021

Client: add VM execution #1028

Client: add VM execution #1028

Conversation

holgerd77 commented Jan 2, 2021

codecov bot commented Jan 2, 2021 • edited Loading

Codecov Report

holgerd77 commented Jan 2, 2021

holgerd77 commented Jan 2, 2021

holgerd77 commented Jan 2, 2021

holgerd77 commented Jan 5, 2021

holgerd77 commented Jan 5, 2021

holgerd77 commented Jan 5, 2021

holgerd77 commented Jan 6, 2021

jochem-brouwer left a comment

Choose a reason for hiding this comment

jochem-brouwer Jan 6, 2021

Choose a reason for hiding this comment

holgerd77 Jan 6, 2021

Choose a reason for hiding this comment

jochem-brouwer Jan 6, 2021

Choose a reason for hiding this comment

holgerd77 commented Jan 6, 2021

ryanio Jan 6, 2021

Choose a reason for hiding this comment

ryanio Jan 6, 2021

Choose a reason for hiding this comment

jochem-brouwer left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Jan 7, 2021

holgerd77 commented Jan 7, 2021

codecov bot commented Jan 2, 2021 •

edited

Loading