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

nixos/geth: initial service #106983

Merged
merged 1 commit into from Feb 23, 2021
Merged

nixos/geth: initial service #106983

merged 1 commit into from Feb 23, 2021

Conversation

bachp
Copy link
Member

@bachp bachp commented Dec 15, 2020

Motivation for this change

Add a service to run a Ethereum node with go-ethereum.
The service uses the same pattern as the bitcoind and allows multiple instances to be run.

Contributes to #103890

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@bachp
Copy link
Member Author

bachp commented Dec 15, 2020

/cc @RaghavSood

@excogitation
Copy link

Did a quick test synching goerli. Looks to be working.
Screenshot from 2021-01-03 01-45-46

Wouldn't it make sense to have "ipcdisable" as an option to be able to enable attaching to geth?

@bachp
Copy link
Member Author

bachp commented Jan 3, 2021

@excogitation I ussually attacht to the node over http via geth attach http://localhost:8545. Adding an option would sure be possible. Do you see an advantage in having IPC too? The option could also be added later should the need arise.

As the module supports multiple instances, it might also be worth to create a wrapper script geht-<instance>, that has the correct parameters already set.

@bachp bachp mentioned this pull request Jan 3, 2021
23 tasks
@excogitation
Copy link

excogitation commented Jan 5, 2021

@excogitation I ussually attacht to the node over http via geth attach http://localhost:8545. Adding an option would sure be possible. Do you see an advantage in having IPC too? The option could also be added later should the need arise.

@bachp I do not see an advantage to that. That makes sense.

@bachp
Copy link
Member Author

bachp commented Jan 5, 2021

@excogitation I ussually attacht to the node over http via geth attach http://localhost:8545. Adding an option would sure be possible. Do you see an advantage in having IPC too? The option could also be added later should the need arise.

@bachp I do not see an advantage to that. That makes sense.

@excogitation To be honest I don't even know if ipc would work as we use the DynamicUser feature from systemd. So the state directory where the ipc file is create might not be accessible by any user, excep the dynamically created one.

@bachp
Copy link
Member Author

bachp commented Jan 18, 2021

@RaghavSood @Mic92 Could you take a look at this MR?

@adisbladis
Copy link
Member

I do not want to rename the package to to geth.
Geth is just one of the binaries in this package and the upstream repository name is go-ethereum.

I would be fine with an alias mapping geth to go-ethereum.

@adisbladis
Copy link
Member

adisbladis commented Feb 21, 2021

@bachp I decided to make go-ethereum multi output in #113960 so this service doesn't need to pull in the entirety of go-ethereum in the runtime closure.
This reduces the service closure by roughly 200M.

@adisbladis adisbladis mentioned this pull request Feb 21, 2021
10 tasks
@bachp
Copy link
Member Author

bachp commented Feb 23, 2021

I did the following changes:

  • package now defaults to go-ethereum.geth
  • package is now added to the systemPackages to be able to interact with the daemon
  • added test that checks if the daemon starts up with the correct network

@adisbladis
Copy link
Member

adisbladis commented Feb 23, 2021

All good. Thank you!

@adisbladis adisbladis merged commit 779ed9e into NixOS:master Feb 23, 2021
@bachp bachp deleted the geth-service branch February 23, 2021 19:55
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

4 participants