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

Move defaultDataDir procedure to nimbus_binary_common. #6939

Draft
wants to merge 2 commits into
base: unstable
Choose a base branch
from

Conversation

pedromiguelmiranda
Copy link
Contributor

This intends to fix all Nimbus clients declaring the same function, and once its merged, unify this use on all of them.

Copy link

github-actions bot commented Feb 20, 2025

Unit Test Results

       15 files  ±  0    2 619 suites  +5   1h 13m 17s ⏱️ - 7m 53s
  6 414 tests +  2    5 893 ✔️ +  2  521 💤 ±0  0 ±0 
44 626 runs  +10  43 908 ✔️ +10  718 💤 ±0  0 ±0 

Results for commit 23fa283. ± Comparison against base commit 7b2a674.

♻️ This comment has been updated with latest results.

@pedromiguelmiranda pedromiguelmiranda marked this pull request as draft February 20, 2025 14:43
@pedromiguelmiranda pedromiguelmiranda force-pushed the dev/pedro/default_dirs branch 4 times, most recently from cbcb9ba to 9b24074 Compare February 20, 2025 23:11
@@ -161,7 +161,7 @@ type

dataDir* {.
desc: "The directory where nimbus will store all blockchain data"
defaultValue: config.defaultDataDir()
defaultValue: config.defaultDataDir("BeaconNode")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely should not be string literal, because its easy to make mistake which you have made.

Copy link
Contributor Author

@pedromiguelmiranda pedromiguelmiranda Feb 21, 2025

Choose a reason for hiding this comment

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

thanks. Which mistake was made?
Ok, I see your point, force type checking with an enum or a table. That would be interesting, however check the comments below.
Deserves a const though

@@ -897,7 +897,7 @@ type

dataDir* {.
desc: "The directory where nimbus will store all blockchain data"
defaultValue: config.defaultDataDir()
defaultValue: config.defaultDataDir("BeaconNode")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not BeaconNode configuration it is Validator Client configuration.

Copy link
Contributor Author

@pedromiguelmiranda pedromiguelmiranda Feb 21, 2025

Choose a reason for hiding this comment

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

thanks.
all of these declarations were using the same defaultDataDir procedure with "BeaconNode" hardcoded on the procedure it self.
I just made it generic, and move it from beacon_chain/conf to nimbus_binary_common module.

Is it safe to start using a diferent data dir for each client and/or component?

@@ -1078,7 +1078,7 @@ type

dataDir* {.
desc: "The directory where nimbus will store validator's keys"
defaultValue: config.defaultDataDir()
defaultValue: config.defaultDataDir("BeaconNode")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not BeaconNode configuration it is Signing Node configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same comment as before.

@arnetheduck
Copy link
Member

The general direction we want to be going in is that we have a directory structure that allows the applications to co-exist in it - ie imagine beacon node, vc and execution client as separate binaries - if I pass the same --data-dir to each of them separately, the outcome should be the same if I run them in a single binary - if I run the single binary, stop it and then run just the beacon node with the same --data-dir, that should "just work".

@arnetheduck
Copy link
Member

arnetheduck commented Feb 21, 2025

the consequence of the above is that we want to tie directory structure to "functionality" and not to any particular end-user application (ie it doesn't matter if the user starts a "validator client" as a separate binary or use the integrated beacon validator - it's the "validator functionality" is what guides what the layout looks like - in this sense, it doesn't make sense to have names like "beacon node" or "validator client" as part of the directory structure.

@pedromiguelmiranda
Copy link
Contributor Author

the consequence of the above is that we want to tie directory structure to "functionality" and not to any particular end-user application (ie it doesn't matter if the user starts a "validator client" as a separate binary or use the integrated beacon validator - it's the "validator functionality" is what guides what the layout looks like - in this sense, it doesn't make sense to have names like "beacon node" or "validator client" as part of the directory structure.

I agree and personally like the direction, and this is the first step towards it:

  • Make the procedure generic for all clients (they will be addressed later on a different PR) and removing the hardcoded path "BeaconNode" keeping the same behaviour as before.

Other steps, besides the obvious one of applying the same strategy to other clients still in development phase, is that Nimbus beacon node client is public and users are not aware of the changes proposed (removing the "BeaconNode" path)

A strategy and specially a delivery method need to be defined and tested among all clients. The delivery part is more concerning given that we we need to either warn users using default data directory to move the data manually, or doing it during installation or doing it via code.

Strategies, in my opinion, to be addressed and reasoned about on upcoming PR's

pmmiranda added 2 commits February 24, 2025 23:20
This intends to fix all Nimbus clients are declaring the same function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants