client: some skeleton improvements from observations on devnet syncs #3014

g11tech · 2023-09-05T17:37:48Z

some improvements for more robust backfill + beacon sync sim

codecov · 2023-09-05T17:41:02Z

Codecov Report

Merging #3014 (e6457eb) into master (d2261ea) will increase coverage by 0.03%.
The diff coverage is 51.28%.

Additional details and impacted files

Flag	Coverage Δ
block	`88.73% <ø> (ø)`
blockchain	`92.58% <ø> (ø)`
client	`87.36% <51.28%> (+0.08%)`	⬆️
common	`98.18% <ø> (ø)`
ethash	`∅ <ø> (∅)`
evm	`71.70% <ø> (ø)`
rlp	`?`
statemanager	`89.91% <ø> (ø)`
trie	`90.18% <ø> (ø)`
tx	`96.38% <ø> (ø)`
util	`86.78% <ø> (ø)`
vm	`80.42% <ø> (ø)`

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

acolytec3

Left a couple of small spelling nits on comments (for clarity's sake) but looks good so far. A few of the skeleton tests are now failing though I'm assuming this is due to the logic changes around backstepping and adjusting subchains.

acolytec3 · 2023-09-05T21:02:16Z

packages/client/src/sync/skeleton.ts

        edited = true
      }
      // If the old subchain is an extension of the new one, merge the two
      // and let the skeleton syncer restart (to clean internal state)

-      const subChain1Head = await this.getBlock(this.status.progress.subchains[1].head)
+      // subchains are useful is subChain1Head is in skeleton only and its tail correct


Suggested change

// subchains are useful is subChain1Head is in skeleton only and its tail correct

// subchains are useful if subChain1Head is in skeleton only and its tail correct

acolytec3 · 2023-09-05T21:02:47Z

packages/client/src/sync/skeleton.ts

+        subChain1Tail === undefined ||
+        !equalsBytes(subChain1Tail.header.parentHash, this.status.progress.subchains[1].next)
+      ) {
+        // if subChain1Head is not in the skeleton that all previous subchains are not useful


Suggested change

// if subChain1Head is not in the skeleton that all previous subchains are not useful

// if subChain1Head is not in the skeleton then all previous subchains are not useful

holgerd77 · 2023-09-11T11:34:04Z

This is still WIP, right?

g11tech · 2023-09-11T19:03:28Z

This is still WIP, right?

yes was buried into building a beacon sync sim test that i plan to extend to do beacon engine api updates along side snapsync (in a speparate PR)

sim is mostly done, need to fix resolution of the synced promise to close out the test

g11tech · 2023-09-11T19:05:29Z

packages/client/test/sim/mainnet.spec.ts

@@ -48,26 +51,30 @@ describe('simple mainnet test run', async () => {

  const blockHashes: string[] = []
  // ------------Sanity checks--------------------------------


rest of the changes in this file are mostly because of formatting triggered by adding sim run timelimit

g11tech · 2023-09-11T19:06:37Z

packages/client/test/sim/mainnet.spec.ts

@@ -25,6 +25,9 @@ export async function runTx(data: string, to?: string, value?: bigint) {
 }

 describe('simple mainnet test run', async () => {
+  if (process.env.EXTRA_CL_PARAMS === undefined) {


shifted the genesis to run from shanghai, this does the same for CL

g11tech · 2023-09-13T12:07:46Z

should be merge ready cc @holgerd77 @acolytec3
hopefully all checks pass 🙂

acolytec3

Left a few suggestions/questions. The code looks good and working to get the new beacon sync test running now.

acolytec3 · 2023-09-13T14:08:53Z

packages/client/test/sim/beaconsync.spec.ts

+          assert.fail('could not complete beacon sync in 8 minutes')
+        }
+      } else {
+        assert.fail('ethereumjs client not setup properly for snap sync')


What is this error referencing? Is snap sync somehow being tested here?

sorry copied over from snapsyncspec , will cleanup

acolytec3 · 2023-09-13T14:10:03Z

packages/client/test/sim/simutils.ts


 import type { Common } from '@ethereumjs/common'
 import type { TransactionType, TxData, TxOptions } from '@ethereumjs/tx'
 import type { ChildProcessWithoutNullStreams } from 'child_process'
 import type { Client } from 'jayson/promise'

 export const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms))
+// This function switches between the native web implementation and a nodejs implemnetation


Suggested change

// This function switches between the native web implementation and a nodejs implemnetation

// This function switches between the native web implementation and a nodejs implementation

acolytec3 · 2023-09-13T14:10:42Z

packages/client/test/sim/simutils.ts

+  // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions
+  if (globalThis.EventSource) {


Why disable this and not just add gloablThis.EventSource !== undefined? I know it's a nit but is there a good reason to fight the linter here?

acolytec3 · 2023-09-13T14:13:43Z

packages/client/test/sim/simutils.ts

+  eventSource.addEventListener(topics[0], (async (_event: MessageEvent) => {
+    if (syncState === 'PAUSED') return
+    try {
+      // just fetch finalized updated, it has all relevant hashesh to fcU


Suggested change

// just fetch finalized updated, it has all relevant hashesh to fcU

// just fetch finalized updated, it has all relevant hashes to fcU

acolytec3 · 2023-09-13T14:25:40Z

packages/client/test/sim/beaconsync.md

+
+## Prerequisites
+
+1. Bash terminal


Suggested change

1. Bash terminal

1. ZSH terminal

acolytec3 · 2023-09-13T14:30:53Z

packages/client/test/sim/beaconsync.md

+3. `jq` & `curl` installed
+4. `ethereumjs-monorepo` codebase build via `npm i` (for e.g. at `/usr/app/ethereumjs`)
+
+You may pre-download docker images for lodestar (`docker pull chainsafe/lodestar:latest`) and geth (`docker pull ethereum/client-go:v1.11.6`) to avoid any test timeout issues.


Should we be using get 1.11.6 or 1.12.2 like the test looks for?

acolytec3

Passing. Nice work!

g11tech added PR state: WIP package: client labels Sep 5, 2023

acolytec3 requested changes Sep 5, 2023

View reviewed changes

g11tech force-pushed the skeleon-improvs-sep5 branch from 0bbcefb to e7fedfd Compare September 11, 2023 19:01

g11tech commented Sep 11, 2023

View reviewed changes

g11tech force-pushed the skeleon-improvs-sep5 branch from 25dc17f to ad3d866 Compare September 13, 2023 12:06

g11tech added PR state: merge ready and removed PR state: WIP labels Sep 13, 2023

acolytec3 reviewed Sep 13, 2023

View reviewed changes

acolytec3 approved these changes Sep 14, 2023

View reviewed changes

g11tech added 8 commits September 14, 2023 21:19

client: some skeleton improvements from observations on devnet syncs

dda6d41

skip filling chain on duplicate sethead

6047ba0

apply nits

94b54aa

add beacon sync sim for incline client

1f5b246

get the beacon sync to resolve

a2abdf7

fix breaking skeleton spec

046110c

fix reverse block fetcher

315d3b0

apply feeback

e6457eb

g11tech force-pushed the skeleon-improvs-sep5 branch from c755763 to e6457eb Compare September 14, 2023 15:49

g11tech merged commit 04aa4da into master Sep 14, 2023

holgerd77 deleted the skeleon-improvs-sep5 branch September 14, 2023 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: some skeleton improvements from observations on devnet syncs #3014

client: some skeleton improvements from observations on devnet syncs #3014

g11tech commented Sep 5, 2023 •

edited

Loading

codecov bot commented Sep 5, 2023 •

edited

Loading

acolytec3 left a comment

acolytec3 Sep 5, 2023

acolytec3 Sep 5, 2023

holgerd77 commented Sep 11, 2023

g11tech commented Sep 11, 2023

g11tech Sep 11, 2023

g11tech Sep 11, 2023

g11tech commented Sep 13, 2023

acolytec3 left a comment

acolytec3 Sep 13, 2023

g11tech Sep 13, 2023

acolytec3 Sep 13, 2023

acolytec3 Sep 13, 2023

acolytec3 Sep 13, 2023

acolytec3 Sep 13, 2023

acolytec3 Sep 13, 2023

acolytec3 left a comment

	// subchains are useful is subChain1Head is in skeleton only and its tail correct
	// subchains are useful if subChain1Head is in skeleton only and its tail correct

	// if subChain1Head is not in the skeleton that all previous subchains are not useful
	// if subChain1Head is not in the skeleton then all previous subchains are not useful

		@@ -48,26 +51,30 @@ describe('simple mainnet test run', async () => {

		const blockHashes: string[] = []
		// ------------Sanity checks--------------------------------

	// This function switches between the native web implementation and a nodejs implemnetation
	// This function switches between the native web implementation and a nodejs implementation

		// eslint-disable-next-line @typescript-eslint/strict-boolean-expressions
		if (globalThis.EventSource) {

	// just fetch finalized updated, it has all relevant hashesh to fcU
	// just fetch finalized updated, it has all relevant hashes to fcU

client: some skeleton improvements from observations on devnet syncs #3014

client: some skeleton improvements from observations on devnet syncs #3014

Conversation

g11tech commented Sep 5, 2023 • edited Loading

codecov bot commented Sep 5, 2023 • edited Loading

Codecov Report

acolytec3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Sep 11, 2023

g11tech commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g11tech commented Sep 13, 2023

acolytec3 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

acolytec3 left a comment

Choose a reason for hiding this comment

g11tech commented Sep 5, 2023 •

edited

Loading

codecov bot commented Sep 5, 2023 •

edited

Loading