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

feat: implement Nimbus beacon service #434

Merged
merged 1 commit into from
Feb 2, 2024
Merged

Conversation

iofq
Copy link
Contributor

@iofq iofq commented Jan 25, 2024

Implements Nimbus Beacon service (validator service to follow).

Two things I think could be contentious:

  • Using ExecPreStart for the checkpoint sync. Nimbus has this implemented somewhat awkwardly IMO, in that it's a separate command. ExecPreStart has a timeout of 15 mins; in all my testing the checkpoint finished downloading in 2 or 3 mins, but had it not completed before the timeout, the systemd service will fail to start. We can as well adjust that 15 minute timeout to be longer or infinite.

  • Opening REST and Metrics ports by default. This seems to be done by default in Lighthouse at least, which i used for reference for a lot of this, however I would argue that the majority of users would not want these exposed and as such maybe it should be a more explicit enable, either within the module or done outside of it. Not sure what the best UX is here.

@iofq iofq changed the title Implement Nimbus Beacon service feat: implement Nimbus beacon service Jan 25, 2024
@brianmcgee brianmcgee self-requested a review January 25, 2024 16:55
@brianmcgee
Copy link
Collaborator

@iofq thanks for the PR!

I'll have a look start of next week, but a cursory glance looks good.

Using ExecPreStart for the checkpoint sync. Nimbus has this implemented somewhat awkwardly IMO, in that it's a separate command. ExecPreStart has a timeout of 15 mins; in all my testing the checkpoint finished downloading in 2 or 3 mins, but had it not completed before the timeout, the systemd service will fail to start. We can as well adjust that 15 minute timeout to be longer or infinite.

I think leaving the 15 min timeout for now is a good start.

Opening REST and Metrics ports by default. This seems to be done by default in Lighthouse at least, which i used for reference for a lot of this, however I would argue that the majority of users would not want these exposed and as such maybe it should be a more explicit enable, either within the module or done outside of it. Not sure what the best UX is here.

I think the pattern has been to try and mimic what the underlying process does, but I agree that by default exposing the REST interface and metrics could be dangerous. Maybe this approach needs changed throughout all the services.

@iofq
Copy link
Contributor Author

iofq commented Feb 2, 2024

rebased and signed commit

Copy link
Collaborator

@brianmcgee brianmcgee left a comment

Choose a reason for hiding this comment

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

lgtm

@brianmcgee brianmcgee merged commit 0ccb71b into nix-community:main Feb 2, 2024
69 of 71 checks passed
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.

2 participants