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

common: add support for new Sepolia chain #1581

Merged
merged 10 commits into from
Jan 3, 2022
Merged

Conversation

emersonmacro
Copy link
Contributor

@codecov
Copy link

codecov bot commented Nov 22, 2021

Codecov Report

Merging #1581 (5d2dc60) into master (3751bbd) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
block 85.06% <100.00%> (+0.09%) ⬆️
blockchain 85.08% <ø> (ø)
client 71.27% <ø> (ø)
common 100.00% <100.00%> (ø)
devp2p 83.31% <ø> (+0.27%) ⬆️
ethash ∅ <ø> (∅)
trie 86.09% <ø> (ø)
tx 88.62% <ø> (ø)
util 91.84% <ø> (ø)
vm 79.89% <ø> (ø)

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

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.

Review is not yet complete (to be continued by me or someone else).

Is this Sepolia network already running? If so: have you (@emersonmacro) tested this setup in the client?

packages/common/src/chains/sepolia.json Outdated Show resolved Hide resolved
packages/common/src/chains/sepolia.json Outdated Show resolved Hide resolved
packages/common/src/chains/sepolia.json Show resolved Hide resolved
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.

Some change requests, I will directly push these after the review.

The initial connection to the bootnode is now working with this.

We get the following follow-up error though:

grafik

Guess that has something to do with the genesis block which might not be written properly to the chain. This will need some debugging. A good starting point here is likely the blockchain library and there the related genesis block initialization code parts.

packages/common/src/chains/sepolia.json Outdated Show resolved Hide resolved
packages/common/src/chains/sepolia.json Show resolved Hide resolved
packages/common/src/chains/sepolia.json Outdated Show resolved Hide resolved
@holgerd77
Copy link
Member

What's the status of this?

@emersonmacro
Copy link
Contributor Author

What's the status of this?

I was able to get the same initial connection to the bootnode and subsequent error that you posted. I'm doing some debugging trying to figure out what's causing the error.

@ryanio
Copy link
Contributor

ryanio commented Dec 30, 2021

sync working! smooth so far

Screen Shot 2021-12-30 at 10 14 55 AM

@ryanio
Copy link
Contributor

ryanio commented Dec 30, 2021

i successfully sync'd to head but not sure why it didn't continue following the tip of the chain

Screen Shot 2021-12-30 at 11 17 02 AM

This error:

(node:24407) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 close listeners added to [BlockFetcher]. Use emitter.setMaxListeners() to increase limit

is concerning, might be related to lib/sync/fetcher/fetcher.ts:L348 -> this.on('close', () => {

@jochem-brouwer
Copy link
Member

Maybe no one is mining Sepolia so there are no new blocks? Check the timestamp of the latest block?

@holgerd77
Copy link
Member

But this Sepolia network is a great chance to really really test our client in production, and all the related tip of the chain behavior with it! 😀

@emersonmacro
Copy link
Contributor Author

I got it fully synced yesterday and did not see the same error:
image

Just restarted the client and it synced back up very quickly, at a slightly higher block height:
image

ryanio
ryanio previously approved these changes Jan 3, 2022
Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

this LGTM!

we can address following the tip of chain behavior in a separate PR, and this should be a nice testnet to do that with :)

@ryanio ryanio merged commit 23a7f35 into master Jan 3, 2022
@ryanio ryanio deleted the common-add-sepolia-support branch January 3, 2022 20:35
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.

None yet

5 participants