-datadir option specified.More information: https://www.freedesktop.org/software/systemd/man/systemd.exec.html
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!
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
-datadir argument to bitcoind via ExecStartI 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.
@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 :smile:
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.
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
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).
I think Flowdalic is right that dropping the
Group=bitcoinline 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 bitcoindc-lightning allows specifying the bitcoin data directory thru its --bitcoin-datadir flagPerhaps 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.
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:
$BITCOIN_HOME and -datadir are set, -datadir would override $BITCOIN_HOME$BITCOIN_HOME effectively is env BITCOIN_HOME=$HOME/.bitcoin as it is currently
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:
0if (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.
bitcoin group exec permission on directoriesdoc/init.md17 # Creates /run/bitcoind owned by bitcoin
18 RuntimeDirectory=bitcoind
19 User=bitcoin
20-Type=forking
21-PIDFile=/run/bitcoind/bitcoind.pid
22+Type=simple
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.
You are right! After doing some digging I think forking is superior right now. Perhaps the following changes can be made in future PRs:
sd_notify to give systemd more information on the state of the process if a compile flag --enable-systemd-notify is supplied to ./configurejournald in a sane way somehow
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.
0@@ -0,0 +1,17 @@
1+PR #12255
2+=========
3+
4+The systemd init file (`contrib/init/bitcoind.service`) has changed to use
5+`/var/lib/bitcoind` as the data directory instead of `~bitcoin/.bitcoin`. This
6+change makes bitcoin more consistent with other services, and makes the systemd
utACK 3b6b72a
nits can be fixed up when the release notes are merged.
73+for the listing of files under the directory.
74+
75+NOTE: It is not currently possible to override `datadir` in
76+`/etc/bitcoin/bitcoin.conf` with the current systemd, OpenRC, and Upstart init
77+files. The command line arguments specified in the init files take precedence
78+over the configurations in `/etc/bitcoin/bitcoin.conf`.
BITCOIND_DATADIR to override it.
re: #12255 (review)
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.
The sentence:
It is not currently possible to override
datadirin/etc/bitcoin/bitcoin.confwith 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.
0@@ -0,0 +1,17 @@
1+PR #12255
systemd init file as title, the PR number won’t tell most users much