nimbus initial commit #3036
nimbus initial commit #3036
Conversation
- chronicle support - Added config files with support for both nimbus-eth clients configuration. - thread model suggestion. - Minimal nimbus config for given thread model. - layers handlers.
pedromiguelmiranda
force-pushed
the
dev/pedro/kikof
branch
from
020d5f9 to
e5aac41
Compare
pedromiguelmiranda
force-pushed
the
dev/pedro/kikof
branch
from
e5aac41 to
709bf4b
Compare
| @@ -104,6 +104,7 @@ VERIF_PROXY_OUT_PATH ?= build/libverifproxy/ | |||
| update \ | |||
| nimbus \ | |||
| nimbus_execution_client \ | |||
| nimbus_unified \ | |||
There was a problem hiding this comment.
the target nimbus is on rampdown:
https://github.com/status-im/nimbus-eth1/blob/dev/pedro/kikof/Makefile#L217
and we need to solve issue #2738 and update some README make commands refs
Given that this is a project in development yet to be released (https://nimbus.guide/execution-client.html), how much time should we wait for the rampdown period?
Agree that it would be nice to get the renaming before starting landing this.
| notice "Exited all services" | ||
|
|
||
| ## Service monitoring | ||
| proc monitor*(nimbus: Nimbus) = |
There was a problem hiding this comment.
what's this for, ie what will it be doing?
There was a problem hiding this comment.
Monitor if one of the services ended (gracefully) and an to ask remaining services to end gracefully.
(check one the service prototype wrappers for an example).
This is a small naive mechanism using polling, to be replaced by some wake up mechanism (maybe a conditional variable)
| setControlCHook(controlCHandler) | ||
|
|
||
| var | ||
| execution = LayerConfig(kind: Execution, executionConfig: NimbusConf()) |
There was a problem hiding this comment.
both of the configs contain garbage-collected types - these cannot be passed as thread arguments - instead, one needs to allocate a memory areas with createShared (and release it manually as well).
There's a small example here: https://status-im.github.io/nim-style-guide/interop.html#passing-data-between-threads
There was a problem hiding this comment.
This is a nice hint. thank you
Wasn't aware of the configs..
| # Short debugging identifier to be placed in the ENR | ||
| enrClientInfoShort* = toBytes("f") | ||
|
|
||
| func getNimGitHash*(): string = |
There was a problem hiding this comment.
we have a "nimbus_binary_common" file for hosting little helpers like this - the idea with that file is no matter what the binary contains, certain things work the same (logging options, version numbers etc) - it would be good to refactor the existing binaries to make use of such utilities, just like the logging.
There was a problem hiding this comment.
Great. all projects are implementing similar code for the same purpous.
Let me try to land that before this.
| # Helper Functions | ||
| # ---------------------------------------------------------------------------- | ||
|
|
||
| template fileExists(filename: string): bool = |
There was a problem hiding this comment.
what's wrong with https://nim-lang.org/docs/files.html#fileExists%2CPath?
There was a problem hiding this comment.
as an alternative, if this function does not exist in io2, it's better to add a correctly implemented function there (ie an implementation that uses the right os utilities, similar to the above linked documentation but with different error handling
There was a problem hiding this comment.
absolutely nothing wrong with those components. This was my half way implementing unit tests for the createPidFile which is not being used.
Which will be moved to binary common as well
| template removeFile(filename: string) = | ||
| try: | ||
| discard io2.removeFile(filename) | ||
| except IOError: |
There was a problem hiding this comment.
io2 does not raise exceptions, or at least it shouldn't - we use either Result or exceptions but not both in a single function (with a few exceptions that don't apply here).
There was a problem hiding this comment.
same as preivous comment
| # Short debugging identifier to be placed in the ENR | ||
| enrClientInfoShort* = toBytes("f") | ||
|
|
There was a problem hiding this comment.
This is a fluffy specific identifier currently used in the ENR. I don't think you need it here.
project structure with: