-
Notifications
You must be signed in to change notification settings - Fork 114
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
cl+el proof of concept [WiP] #2804
base: master
Are you sure you want to change the base?
Conversation
Makefile: Disabled libbacktrace: for some reason was given a strange error DW_FORM_addrx. Requires further investigation, given that it can be a macos issue, or related to the fact tathatth nimbus-eth2 is a submodule. moved getPid to thread worker it self. might required further investigation regarding thread pattern. small typos
There are currently 2 issues: - fetch genesis state on empty data folder: error msg "The downloaded genesis state cannot be verified (checksum mismatch)\" The error arises from here: fetchGenesisState->network_metadata_downloads.fetchGenesisBytes.L58 something to do with the readssz or withState, investigations point to the data downloaded or some config missing WA: comment lines 58-62, compile and run until the fetch genesis state is completed, then you can uncomment, it works from here - spam of error messages: \"metrics error:New label values must be added from same thread as the metric was created from\": This happens due to the fact that libp2p declares some gauges, and given that they are created inside a thread, metrics library starts to complain. (no WA/correction so far)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to refactor block_chain_dag
in nimbus-eth2
so it can be useful for both than copy/paste 2900 lines of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect a stream of small improvements in all projects before we land this, so as long as this is just a wip/pr, it's not really an issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are both right.
For now, the code has a lot of Todo's
, Note's
and Adapted from eth2
, which can be translated to: Can and will need refactor. Blockchain_dag
is one of them, but nimbus_beacon_node
will require a lot of removals and improvements (first reason: declares gauges that metrics library does not like when it is declared on a thread).
Personally, I would tackle these improvements when we have a more "advanced" running software.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear these wrapper shells scripts are useful. nimbus-eth2
retains them for the sake of compatibility with existing installations because https://nimbus.guide/ refers to them, but potentially not worth introducing them in a context which has never had/needed/required them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, and due to your comment I now know why they are kept on nimbus-eth2.
For now it is useful for development purposes (anyone can quickly spin the application) but don't plan on merging them
This is acting as hot fix for now, given that some metrics are requirements. However we need to collect the ones we need / want and find a way to extract them from nimbus-eth2 (or make them reusable by both )
This is a work in progress for a cl+el single application.
Some notes:
comments with:
# NOTE, #TODO,
are related to improvements or refactorings that need to be donecomments with:
# adapted from nimbus-eth2
, things that were required to be overridden or not used (or we think that is not used)Probably the best course of action will be to create issues regarding it.
current state: able to synch (or it appears so) with two minor issues (check latest commit message)