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

Update bitcoin.service to conform to init.md #12255

Merged
merged 2 commits into from Feb 4, 2019

Conversation

Projects
None yet
10 participants
@dongcarl
Copy link
Contributor

commented Jan 24, 2018

  • -datadir option specified.
  • Ask systemd to create and set the right mode for PID directory, configuration directory, and data directory.
  • Tell systemd our group so it will set the right owner for aforementioned directories.

More information: https://www.freedesktop.org/software/systemd/man/systemd.exec.html

@dongcarl dongcarl force-pushed the dongcarl:patch-2 branch Jan 24, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 24, 2018

Last update was in PR #10529 - please try to coordinate the changes with the people replying there.

@dongcarl

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2018

@laanwj Sorry I'm not sure I entirely know what you mean. Am I to bring up this PR on that thread or shall I tag them in a comment here?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 24, 2018

I think the link was supposed to be #12102

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

Sorry I'm not sure I entirely know what you mean. Am I to bring up this PR on that thread or shall I tag them in a comment here?

Either, or both, is fine!

@dongcarl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2018

@laanwj @MarcoFalke should I be waiting for the other PR to get merged? Seems like the OP has been unresponsive.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 6, 2018

I think the only thing left is give people time to review and then merge both whenever they are reviewed.

@dongcarl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2018

Cool beans.

@Flowdalic

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2018

Thanks for your ping in #12102 (comment), here are my thoughts on this PR:

This PR contains the following changes to the systemd bitcoind.service file

  1. Provide -datadir argument to bitcoind via ExecStart
  2. Create various runtime directories and set their access mode
  3. Set the Group to bitcoin

I deliberately did not put (1) in the systemd file as it can also be set via bitcoind's conf. I believe that it is good practice to put as much as possible in generic service config file (here: bitcoin.conf) as opposed to the system's service management facilities (here: systemd).

(2) is typically done by the distribution's package manager and would hence override settings of distributions that don't use those standard directories (therefore those distributions would need to patch the upstream systemd file or use their own, which is not both not desirable). Nit: I don't think it is necessary to protect the runtime directory with 700 as there a no secrets in it. Also the used settings are only available with systemd >= 235.

(3) is changes the behavior as it explicitly mentions the existence of a group named 'bitcoin', which may or may not exist. I believe that the behavior if 'Group' is not set, using the default group of the user, is more portable and should be used.

Further nits of this PR include that the order of -pid and -conf is changed for no reason, and the (correct) comment telling us that the /run/bitcoind is owned by 'bitcoin' is removed.

In conclusion I am not provided with and don't see any compelling reasons to apply this.

@dongcarl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2018

@Flowdalic The spirit of my PR is to follow in the footsteps of doc/init.md. Which states that these changes are to be made.

(1) doc/init.md states that the data directory should be at /var/lib/bitcoind, but if it's not indicated in the service file, the data directory is not at /var/lib/bitcoind, but rather at ~/.bitcoin
(2, 3) doc/init.md states that "The configuration file, PID directory (if applicable) and data directory should all be owned by the bitcoin user and group. It is advised for security reasons to make the configuration file and data directory only readable by the bitcoin user and group. Access to bitcoin-cli and other bitcoind rpc clients can then be controlled by group membership.". Perhaps according to the last sentence it'd be better to set the permissions to 750?

If someone was installing from source, and reading from doc/init.md, they would experience all kinds of confusion, as the data directory won't be where doc/init.md indicated it is. With 2 and 3 I think it's reasonable to think that this is optional, but my personal opinion is that what's in the repository should reflect the latest best practices and be consistent.

I'm actually torn for what we should do for (2), while I believe it's better to have consistency on our end and have the distributions figure it out, I don't wanna break things for distributions with alternative hiers.

Happy to discuss more 😄

@randolf

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2018

utACK.

@sugarjig

This comment has been minimized.

Copy link

commented Jul 10, 2018

I'd like to throw in a vote for (1). I recently stood up an Ubuntu 18.04 server, using the instructions in doc/init.md to install the service. Like @dongcarl is saying, I was a bit confused when it came to configuring the data directory. It worked when I added the -datadir option to the service file.

I tried adding datadir=/var/lib/bitcoind to bitcoin.conf like @Flowdalic suggested, but Bitcoin failed to start. Running journalctl -xe, I see the the error boost::filesystem::create_directories: Permission denied: "/home/bitcoin". It seems this option in the config file has no effect.

@Flowdalic

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2018

I had a look at the code and it appears that bitcoind init first checks the datadir and then reads the config file, which would explain why datadir in the config file has no effect if the default datadir (or the datadir specified by command line argument) is for some reason inaccessible. I have created #13621 in an attempt to fix this.

@DrahtBot DrahtBot closed this Jul 20, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

The last travis run for this pull request was 177 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@ryanofsky
Copy link
Contributor

left a comment

utACK 8b04bf40f7396518d638a7feacf2d8e8e9931780. The inconsistencies between doc/init.md and contrib/init/bitcoind.service and contrib/init/bitcoind.service and the other init files are confusing and this PR gets rid of them and is an improvement over the status quo.

But I think @Flowdalic makes very good points, especially about this change breaking the ability to specify datadir in the config file in #12255 (comment). So to follow up, I would suggest the following changes (either here or in separate PRs):

  • Add release-notes-pr12255.md file mentioning that contrib/init/bitcoind.service has changed to use /var/lib/bitcoind as the datadir instead of $HOME/.bitcoin (which is typically /var/lib/bitcoin/.bitcoin or /home/bitcoin/.bitcoin). The change makes bitcoin more consistent with other services, makes the systemd config more consistent with existing upstart and openrc configs. The release notes should also mention that datadir is now managed entirely in the bitcoind.service file and that trying to override it in /etc/bitcoin/bitcoin.conf will have no effect.

  • Mention somewhere in doc/init.md that it is not possible to override datadir in /etc/bitcoin/bitcoin.conf with the current systemd, openrc, and upstart init files.

  • (Option for a future PR.) I think it's probably better to just document inability to override datadir in /etc/bitcoin/bitcoin.conf, rather than try to do anything about it. But if there are use-cases for wanting to use our service files but also needing the ability to change datadir from the config file, there are changes we could make to bitcoind to support removing the -datadir=/var/lib/bitcoind startup argument. For example we could change GetDefaultDataDir to return "$BITCOIN_HOME" if it is set, instead of "$HOME/.bitcoin", with no loss of compatibility. (GnuPG has a similar GNUPGHOME variable.) The init files could then set $BITCOIN_HOME instead of overriding -datadir.

  • I think Flowdalic is right that dropping the Group=bitcoin line would be less fragile. This should be safe because we aren't giving the group any permissions, and systemd.exec specifies "If no group is set, the default group of the user is used". But I would want the group to be specified explicitly if we were granting the group any permissions (even read only).

@dongcarl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2018

I think Flowdalic is right that dropping the Group=bitcoin line would be less fragile. This should be safe because we aren't giving the group any permissions, and systemd.exec specifies "If no group is set, the default group of the user is used". But I would want the group to be specified explicitly if we were granting the group any permissions (even read only).

I believe that there are valid uses for the bitcoin group. Some examples:

  • elements uses the bitcoin auth cookie file thru its -mainchainrpccookiefile flag for its RPC calls to bitcoind
  • c-lightning allows specifying the bitcoin data directory thru its --bitcoin-datadir flag

Perhaps what should be done is to make the permissions for the datadir 770 or 750, so that these applications can run under the bitcoin group and automatically get rw/ro access to it.

@dongcarl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2018

Add release-notes-pr12255.md file mentioning that contrib/init/bitcoind.service has changed to use /var/lib/bitcoind as the datadir instead of $HOME/.bitcoin (which is typically /var/lib/bitcoin/.bitcoin or /home/bitcoin/.bitcoin). The change makes bitcoin more consistent with other services, makes the systemd config more consistent with existing upstart and openrc configs. The release notes should also mention that datadir is now managed entirely in the bitcoind.service file and that trying to override it in /etc/bitcoin/bitcoin.conf will have no effect.

Mention somewhere in doc/init.md that it is not possible to override datadir in /etc/bitcoin/bitcoin.conf with the current systemd, openrc, and upstart init files.

Sounds good to me. Will do.

(Option for a future PR.) I think it's probably better to just document inability to override datadir in /etc/bitcoin/bitcoin.conf, rather than try to do anything about it. But if there are use-cases for wanting to use our service files but also needing the ability to change datadir from the config file, there are changes we could make to bitcoind to support removing the -datadir=/var/lib/bitcoind startup argument. For example we could change GetDefaultDataDir to return "$BITCOIN_HOME" if it is set, instead of "$HOME/.bitcoin", with no loss of compatibility. (GnuPG has a similar GNUPGHOME variable.) The init files could then set $BITCOIN_HOME instead of overriding -datadir.

Just to verify I understand what you're saying:

  • In the case that both $BITCOIN_HOME and -datadir are set, -datadir would override $BITCOIN_HOME
  • An unset $BITCOIN_HOME effectively is env BITCOIN_HOME=$HOME/.bitcoin as it is currently
@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

I believe that there are valid uses for the bitcoin group

Sure, I'm just agreeing with previous commenters that bitcoin group should either be required to exist and given permissions or it should be dropped. It's unnecessarily fragile to reference a group that doesn't need to exist and doesn't have any permissions for no reason.

Just to verify I understand what you're saying

I'm not really saying the second thing. The GetDefaultDataDir function is more complex than that. I'm just saying we could add a one-line override at the top of that function:

if (char* ret = getenv("BITCOIN_HOME")) return ret;

that would give init systems a way to set up a default data location without causing datadir= options in /etc/bitcoin/bitcoin.conf to get silently ignored. I'm not saying we should do this. It's fine IMO to just document the limitations that exist right now. But if they are a problem that needs to be solved, this would be a simple, backwards compatible way to solve them.

@dongcarl dongcarl force-pushed the dongcarl:patch-2 branch 3 times, most recently Jan 3, 2019

@dongcarl

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

  • Cleaned up commits
  • Given bitcoin group exec permission on directories
  • Modified doc/init.md
  • Added release note
@ryanofsky
Copy link
Contributor

left a comment

utACK 40d16e0664091e69fa921ffbf6254f4951c47ed6. I think all the issues that were brought up earlier are now addressed, and this looks like a nice improvement. I left a new comment, but it can be ignored if it's not important.

contrib/init/bitcoind.service Outdated
# Creates /run/bitcoind owned by bitcoin
RuntimeDirectory=bitcoind
User=bitcoin
Type=forking
PIDFile=/run/bitcoind/bitcoind.pid
Type=simple

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 4, 2019

Contributor

In commit "init: Don't daemonize when runnning under systemd" (558dddfa8a493a3fa521d7bd098ec6d87e148ced)

I don't think changing from a forking service to a simple service is actually a good idea, and I wonder if there any downsides at all to just dropping this commit, while keeping the other two commits, because:

  • Simple services are more limited that forking services. You can't tell the difference between a running service and service that's starting up, and you can't do things like specifying Requires= and After= in a dependent service to avoid starting it if starting bitcoind fails.

  • Using a simple service requires passing -daemon=0 which currently has the side effect of enabling logging to stdout. If any debug options are specified this can mean a lot of information will go into system logs that isn't going there now. Also, since bitcoind prints timestamps on the console by default, the log entries will look strange in journalctl.

It might be a good idea to add -printtoconsole=0 if we need to keep -daemon=0, or at least mention in release notes that new logs may now show up in journald.

This comment has been minimized.

Copy link
@dongcarl

dongcarl Jan 5, 2019

Author Contributor

You are right! After doing some digging I think forking is superior right now. Perhaps the following changes can be made in future PRs:

  1. Use sd_notify to give systemd more information on the state of the process if a compile flag --enable-systemd-notify is supplied to ./configure
  2. Make logs available in journald in a sane way somehow
init: Use systemd automatic directory creation
Tell systemd to create, set, and ensure the right mode for the PID,
configuration, and data directories.

Only the exec bit is set for groups for the aforementioned directories.
This is the least privilege perm that allows for the
reading/writing/execing of files under the directory _if_ the files
themselves give permission to its group to do so (e.g. when -sysperms is
specified). Note that this does not allow for the listing of files under
the directory.

@dongcarl dongcarl force-pushed the dongcarl:patch-2 branch Jan 5, 2019

@fanquake
Copy link
Member

left a comment

utACK 3b6b72a

nits can be fixed up when the release notes are merged.

Show resolved Hide resolved doc/release-notes/release-notes-pr12255.md Outdated

@dongcarl dongcarl force-pushed the dongcarl:patch-2 branch to ca07fa8 Jan 5, 2019

@fanquake
Copy link
Member

left a comment

re-utACK ca07fa8

@ryanofsky
Copy link
Contributor

left a comment

utACK ca07fa8. Only changes since previous review are dropping the first commit (in response to #12255 (comment)) and tweaking the release notes.

doc/init.md Outdated
NOTE: It is not currently possible to override `datadir` in
`/etc/bitcoin/bitcoin.conf` with the current systemd, OpenRC, and Upstart init
files. The command line arguments specified in the init files take precedence
over the configurations in `/etc/bitcoin/bitcoin.conf`.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jan 15, 2019

Member

At least OpenRC's init allows for setting BITCOIND_DATADIR to override it.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 18, 2019

Contributor

re: #12255 (comment)

At least OpenRC's init allows for setting BITCOIND_DATADIR to override it.

re: http://www.erisian.com.au/bitcoin-core-dev/log-2019-01-17.html#l-365

I see... I think I'll make it easy to override for systemd as well then. Thanks!

It would be good to mention /etc/conf.d/bitcoind for OpenRC, but I wouldn't put a huge amount effort into implementing a configuration mechanism for the systemd service (unless your really want to). RC scripts tend to support external configuration because can they can be pretty long and complicated. Systemd service files are (in theory) supposed to stay simple and rely on more coarse override mechanisms like systemctl edit.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

I think this could be merged. It looks like the only outstanding suggestion is to mention the openrc config file in the documentation (#12255 (comment)), which could be done in a followup later.

@dongcarl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

@ryanofsky Perhaps I should just omit the other init systems in the release notes, as it is unrelated to this PR and I don't have enough confidence that they don't have a way of overriding. If that seems acceptable I will make the wording changes.

@dongcarl dongcarl force-pushed the dongcarl:patch-2 branch from ca07fa8 to 9c8d535 Feb 1, 2019

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

The sentence:

It is not currently possible to override datadir in /etc/bitcoin/bitcoin.conf with the current systemd, OpenRC, and Upstart init files

is correct as written. If you want to mention that OpenRC and Upstart have other config mechanisms that would be wonderful but not necessary. I do think it is important to mention that out of the box in all three init systems trying to set datadir in /etc/bitcoin/bitcoin.conf is not supported and will have no effect.

@dongcarl dongcarl force-pushed the dongcarl:patch-2 branch from 9c8d535 to f24e99b Feb 1, 2019

@dongcarl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

Pushed clearer wording

@@ -0,0 +1,17 @@
PR #12255

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 2, 2019

Member

Thanks for adding a release notes item.
I'd suggest systemd init file as title, the PR number won't tell most users much

@dongcarl dongcarl force-pushed the dongcarl:patch-2 branch from f24e99b to bad1716 Feb 4, 2019

@dongcarl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

Fixed @laanwj's suggestion.

@laanwj laanwj merged commit bad1716 into bitcoin:master Feb 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

pull bot pushed a commit to jaschadub/bitcoin that referenced this pull request Feb 4, 2019

Merge bitcoin#12255: Update bitcoin.service to conform to init.md
bad1716 init: Modify docs and add release note for 12255 (Carl Dong)
b0c7b54 init: Use systemd automatic directory creation (Carl Dong)

Pull request description:

  - `-datadir` option specified.
  - Ask systemd to create and set the right mode for PID directory, configuration directory, and data directory.
  - Tell systemd our group so it will set the right owner for aforementioned directories.

  More information: https://www.freedesktop.org/software/systemd/man/systemd.exec.html

Tree-SHA512: a6fad1efa2be433c1fdd863df3ff232736ed709a9e281f51a003b40987d8c213dc64a52bc13a19c85bf85680e78f0be112ecaf32ac274b1ff93bac84a1208845
@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

Thanks! utACK bad1716

@ryanofsky
Copy link
Contributor

left a comment

utACK bad1716

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.