Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client: integrate snapsync on experimental basis #3031

Merged
merged 39 commits into from
Oct 31, 2023
Merged

Conversation

g11tech
Copy link
Contributor

@g11tech g11tech commented Sep 18, 2023

this PR aims to integrate snap sync on experimental basis (and has quite a good number fixes/refacs all over client beyond snapsync)

How to test it:

from packages/client

  1. run a single node local network with static state
    NETWORK=mainnet NETWORKID=1337903 ELCLIENT=geth EXTRA_CL_PARAMS="--params.CAPELLA_FORK_EPOCH 0" DATADIR=/usr/app/ethereumjs/packages/client/data test/sim/single-run.sh
    let it run for 32+ slots so that finalization happens, else you might end up waiting on step 2 anyway
  2. run snapsync sim (you can run this as many times as you want while 1. is still running)
    rm -rf ./datadir; EXTERNAL_RUN=true NETWORK=mainnet NETWORKID=1337903 ELCLIENT=geth SNAP_SYNC=true DATADIR=/usr/app/ethereumjs/packages/client/data npx vitest run test/sim/snapsync.spec.ts

OR if you are feeling lucky then just try it on holesky (modify your data and jwtsecret args accordingly):
npm run client:start:ts -- --dataDir /data/lodestar-quickstart/holedata/ethereumjs --rpcEngineAddr 0.0.0.0 --jwtSecret /data/lodestar-quickstart/holesky-data/jwtsecret --network holesky --rpcEngine --snap
i.e. just add --snap additonal arg to whatever is your usual run command

@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Merging #3031 (ea4076d) into master (ac9830f) will decrease coverage by 1.69%.
The diff coverage is 72.63%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 88.80% <100.00%> (ø)
blockchain 92.27% <ø> (ø)
client 86.20% <72.50%> (-1.37%) ⬇️
common 98.19% <ø> (ø)
ethash ?
evm 71.93% <ø> (ø)
rlp ∅ <ø> (∅)
statemanager 90.13% <70.58%> (-0.17%) ⬇️
trie 89.42% <ø> (-0.30%) ⬇️
tx ?
util 87.53% <83.33%> (?)
vm 76.13% <ø> (?)

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

@g11tech g11tech force-pushed the integrate-snapsync branch 2 times, most recently from fab73aa to 7e531e0 Compare October 26, 2023 09:38
@g11tech g11tech marked this pull request as ready for review October 27, 2023 09:27
const addressHex = bytesToUnprefixedHex(address.bytes)
const storageTrie = this._storageTries[addressHex]
// TODO PR: have a better interface for hashed address pull?
protected _getStorageTrie(addressOrHash: Address | Uint8Array, account?: Account): Trie {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@holgerd77 for your attention and feedback 🙏

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, since this is an internal method I would assume this should be fine for now.

Just FYI: we experimented though lately to get rid of this method (and generally: mechanism) completely here #3117 to see if we can get rid of creating all these trie copies.

That's likely (?) another story though.

@jochem-brouwer
Copy link
Member

Have merged master and pushed a fix for the conflicts (am branching off this one as suggested to do reorg logic)

@g11tech
Copy link
Contributor Author

g11tech commented Oct 28, 2023

Have merged master and pushed a fix for the conflicts (am branching off this one as suggested to do reorg logic)

added the handling for keeping unfinalized non canonical blocks around in skeleton to handle some reorgs without backfill
reccomend to add that commit into your hive reorg fixes branch as well

@holgerd77
Copy link
Member

Have updated this via UI

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, have had another high level look here, mainly from a how-is-the-main codebase affected. Also did two sanity run checs, one mainnet sync up to block 80.000 or so and one Holesky sync.

Seems things are working fine. 🙂 Can't oversee all the changes in the Skeleton class, e.g., but I do think we should take this code base in. Then we are getting more familiar with this over time, so this is very much a thing where one needs to grow some understanding/feeling of the behavior over time anyhow.

Will merge. 🎉 (thanks @scorbajio for the base stack and @g11tech for the integration! 😍)

@@ -680,7 +681,7 @@ export class Config {
*/
getDnsDiscovery(option: boolean | undefined): boolean {
if (option !== undefined) return option
const dnsNets = ['holesky', 'sepolia']
const dnsNets = ['goerli', 'sepolia', 'holesky']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, HERE this was hidden!

(I wondered why this never made it to master weeks after we had discussed 😂)

@holgerd77 holgerd77 merged commit 9f91d22 into master Oct 31, 2023
43 of 44 checks passed
@holgerd77 holgerd77 deleted the integrate-snapsync branch October 31, 2023 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants