feature: Added ability for users to add a startup command

benthecarman commented at 8:45 pm on February 7, 2019: contributor

Thoughts for adding the feature is for users to be able to add things like electrum-personal-server or lnd to run whenever Bitcoin Core is running. Open to feedback about the feature.

benthecarman force-pushed on Feb 7, 2019

kristapsk commented at 9:19 pm on February 7, 2019: contributor

I don’t think this should be the scope of Bitcoin Core. Service dependencies should be handled by the init / service management system you are using. Or you can just create a simple shell script, to run all the stuff you need. Besides, it is usually recommended to run different services under different users, if possible, to increase security.

gmaxwell commented at 10:57 pm on February 7, 2019: contributor

@kristapsk Startup services can’t trigger when bitcoin is ready to be actually used. That is the big reason for a notify here. Yes, as an alternative software could just poll until its ready but the same could be said for blocknotifys.

Historically speaking– startup scripts people have created for bitcoin have been uniformly low quality (e.g. making the software unusable providing no way to get access to the rpc cookie, killing the process and corrupting its state on shutdown, etc). I have generally recommended against using these package scripts due to their low quality, so I think it would be asking a lot of them to expect them to start intelligently starting other services when so far not corrupting the Bitcoin install has been a challenge for them.

laanwj added the label Utils/log/libs on Feb 8, 2019

promag commented at 2:39 pm on February 8, 2019: member

I understand the need, I’m not fond of the solution. I think it’s perfectly sane to poll the interface or locally run something like netstat.

Does anyone knows an example using this strategy?

in src/init.cpp:1974 in b58073e212 outdated

1761@@ -1752,6 +1762,8 @@ bool AppInitMain(InitInterfaces& interfaces)
1762         return false;
1763     }
1764 
1765+    StartupNotify();
1766+
1767     // ********************************************************* Step 13: finished
1768 
1769     SetRPCWarmupFinished();

MarcoFalke commented at 4:14 pm on February 8, 2019:

I wonder if you could move this down a few lines so it would only fire after the rpc is up. This would also open the possibility to use it in the test framework to add coverage.

benthecarman commented at 4:22 pm on February 8, 2019:

Yeah I wasn’t entirely sure on the best place for the function call, I could move it to the end of the function.

gmaxwell commented at 10:36 pm on February 9, 2019: contributor

I understand the need, I’m not fond of the solution. I think it’s perfectly sane to poll the interface or locally run something like netstat.

Huh. Netstat can not tell you if bitcoind is done starting. If you connect as soon as the socket is listening you’ll get “bitcoin is starting” errors returned. The gain here over simply retrying is that you can start connecting only when it’s actually ready.

(And yes, true you could just sit in a retry loop, but you could also poll for all the other notifys and that didn’t stop us from adding them. :) )

kristapsk commented at 7:52 pm on February 10, 2019: contributor

Startup services can’t trigger when bitcoin is ready to be actually used. That is the big reason for a notify here.

Didn’t thought about that. Yes, then it makes sense, concept ACK from me too.

benthecarman force-pushed on Feb 11, 2019

benthecarman commented at 3:27 am on February 11, 2019: contributor

Moved function call to very end of init sequence

DrahtBot commented at 3:34 am on February 11, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17127 (util: Correct permissions for datadir and wallets subdir by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag commented at 7:10 pm on February 11, 2019: member

Still not convinced about this. I think the general approach is to poll the interface of interest.

Docker, for instance, supports a directive to check if a container is healthy https://docs.docker.com/v17.09/engine/reference/builder/#healthcheck

Anyway, what if you want to trigger some command when both bitcoind and other service are up and available? You would have to poll the second. Take your example, what if you want to run something else after bitcoind and lnd are up?

Also, from the client point of view, I think it’s a good practice to have retry mechanisms, no because of the startup, but because in runtime a service can be unavailable for some reason.

abitfan commented at 6:18 pm on February 25, 2019: contributor

Electrum (and most likely lnd as well) already have a retry mechanism for rpc and the rpc can be unavailable for other reasons as well so this doesn’t add much functionality.
You can watch debug.log for “init message: Done loading” or setup a rawtx zmq publisher and consider bitcoind started when the first notification comes in.

Not specific to this pr but running random commands is not a good security practice and can lead to disasters like privilege escalation. Consider the scenario where someone out of hurry and not wanting to deal with file permissions puts his notifywhatever script in a world writable place and forgets about it only to have his wallet flushed some months later.

benthecarman commented at 8:49 pm on February 25, 2019: contributor

Consider the scenario where someone out of hurry and not wanting to deal with file permissions puts his notifywhatever script in a world writable place and forgets about it only to have his wallet flushed some months later.

While that is true the same could be done with the other notify options, which are ran much more often.

DrahtBot added the label Needs rebase on Aug 2, 2019

benthecarman force-pushed on Oct 13, 2019

DrahtBot removed the label Needs rebase on Oct 13, 2019

in src/init.cpp:402 in bfe463b0af outdated

398@@ -399,6 +399,7 @@ void SetupServerArgs()
399                  " If <type> is not supplied or if <type> = 1, indexes for all known types are enabled.",
400                  ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
401 
402+	gArgs.AddArg("-startupnotify=<cmd>", "Execute command on startup.", false, OptionsCategory::OPTIONS);

luke-jr commented at 11:35 pm on October 22, 2019:

Why is this indented weird?

luke-jr commented at 11:40 pm on October 22, 2019:

false is wrong here too

benthecarman commented at 1:59 am on October 23, 2019:

Fixed.

luke-jr approved

luke-jr commented at 11:38 pm on October 22, 2019: member

utACK

benthecarman force-pushed on Oct 23, 2019

DrahtBot added the label Needs rebase on May 23, 2020

benthecarman force-pushed on May 23, 2020

DrahtBot removed the label Needs rebase on May 23, 2020

DrahtBot added the label Needs rebase on Jul 30, 2020

benthecarman force-pushed on Jul 30, 2020

benthecarman commented at 4:59 pm on July 30, 2020: contributor

Rebased

DrahtBot removed the label Needs rebase on Jul 30, 2020

dongcarl commented at 6:49 pm on July 30, 2020: member

Concept ACK!

This is also a nice, init-system agnostic way of signaling to the init system that bitcoind is ready and in steady state.

On systemd systems, it could look like:

0-startupnotify='systemd-notify --ready'

Couple this with Type=notify in bitcoind.service and set dependent units to After=bitcoind.service and Lennart will visit you in your dreams! :smile:

dongcarl commented at 7:25 pm on July 30, 2020: member

tACK 128d9fe1e2e1264753f821c972212fd24d9a2290

Note the -p Type=notify which tells systemd that it’ll be notified when the unit is ready, and the final log line Started..., which indicates that systemd received the ready notification after sockets are bound to and threads started.

 0$ systemd-run -p Type=notify --unit=bitcoind-test ./src/bitcoind -startupnotify='systemd-notify --ready'
 1$ journalctl -fu bitcoind-test
 2Jul 30 15:14:22 dai systemd[1]: Starting /home/dongcarl/src/bitcoin/benthecarman/startup_commands/./src/bitcoind -startupnotify=systemd-notify --ready...
 3Jul 30 15:14:22 dai bitcoind[2462525]: 2020-07-30T19:14:22Z Bitcoin Core version v0.20.99.0-128d9fe1e2 (release build)
 4...
 5Jul 30 15:14:22 dai bitcoind[2462525]: 2020-07-30T19:14:22Z Command-line arg: startupnotify="systemd-notify --ready"
 6...
 7Jul 30 15:14:23 dai bitcoind[2462525]: 2020-07-30T19:14:23Z init message: Starting network threads...
 8Jul 30 15:14:23 dai bitcoind[2462525]: 2020-07-30T19:14:23Z net thread start
 9Jul 30 15:14:23 dai bitcoind[2462525]: 2020-07-30T19:14:23Z opencon thread start
10Jul 30 15:14:23 dai bitcoind[2462525]: 2020-07-30T19:14:23Z init message: Done loading
11Jul 30 15:14:23 dai bitcoind[2462525]: 2020-07-30T19:14:23Z dnsseed thread start
12Jul 30 15:14:23 dai bitcoind[2462525]: 2020-07-30T19:14:23Z msghand thread start
13Jul 30 15:14:23 dai bitcoind[2462525]: 2020-07-30T19:14:23Z addcon thread start
14Jul 30 15:14:23 dai bitcoind[2462525]: 2020-07-30T19:14:23Z Waiting 300 seconds before querying DNS seeds.
15Jul 30 15:14:23 dai systemd[1]: Started /home/dongcarl/src/bitcoin/benthecarman/startup_commands/./src/bitcoind -startupnotify=systemd-notify --ready.

Whereas without -startupnotify='systemd-notify --ready' the unit is stuck as activating (start)

Note that Type=notify can’t be used with -daemon, as it assumes that the command doesn’t fork. I even tried additionally supplying PIDFile=<path> to system and -pidfile=<path> to bitcoind and that doesn’t work either. It shouldn’t hurt to not daemonize tho.

jonatack commented at 6:14 pm on August 17, 2020: member

Concept ACK

DrahtBot added the label Needs rebase on Aug 26, 2020

benthecarman force-pushed on Aug 26, 2020

DrahtBot removed the label Needs rebase on Aug 26, 2020

MarcoFalke commented at 6:08 pm on August 26, 2020: member

Concept ACK

benthecarman force-pushed on Aug 27, 2020

benthecarman commented at 5:53 am on August 27, 2020: contributor

Rebased and added #if HAVE_SYSTEM around the config option and function definition, similar to the other notify options

benthecarman force-pushed on Aug 27, 2020

kristapsk approved

kristapsk commented at 9:12 pm on September 25, 2020: contributor

ACK 962d730efcc2877bf8f9affe46720388952e179b, I have tested the code (on Linux).

in doc/release-notes-15367.md:6 in 962d730efc outdated

0@@ -0,0 +1,6 @@
1+Configuration option changes
2+----------------------------
3+
4+- The `startupnotify` option is used to specify a command to
5+  execute when Bitcoin Core has finished with its startup
6+  sequence.

MarcoFalke commented at 7:23 am on September 28, 2020:

style-nit:

0  sequence. (#15367)

benthecarman commented at 3:51 pm on September 28, 2020:

Done

in src/init.cpp:679 in 962d730efc outdated

672@@ -670,6 +673,17 @@ static void CleanupBlockRevFiles()
673     }
674 }
675 
676+#if HAVE_SYSTEM
677+static void StartupNotify()
678+{
679+    std::string strCmd = gArgs.GetArg("-startupnotify", "");

MarcoFalke commented at 7:25 am on September 28, 2020:

style-nit: Could pass in the argsmanager to avoid referencing the global?

0    std::string cmd = args.GetArg("-startupnotify", "");

benthecarman commented at 3:51 pm on September 28, 2020:

Done

MarcoFalke approved

MarcoFalke commented at 7:26 am on September 28, 2020: member

review ACK 962d730efcc2877bf8f9affe46720388952e179b

feature: Added ability for users to add a startup command 090530cc24

benthecarman force-pushed on Sep 28, 2020

benthecarman commented at 3:39 pm on September 28, 2020: contributor

Incorporated @MarcoFalke’s suggestions

MarcoFalke added this to the milestone 0.21.0 on Sep 28, 2020

MarcoFalke commented at 6:10 pm on September 28, 2020: member

re-ACK 090530cc24 @dongcarl or @kristapsk would be good to get another re-ACK or so, so that this can be merged into the upcoming major release.

dongcarl commented at 6:41 pm on September 28, 2020: member

tACK 090530c

MarcoFalke merged this on Sep 28, 2020

MarcoFalke closed this on Sep 28, 2020

benthecarman deleted the branch on Sep 28, 2020

laanwj commented at 7:34 pm on September 28, 2020: member

Posthumous ACK

Kixunil commented at 8:23 am on January 19, 2021: none

Thank you all for working on this! Very helpful to me and came in pretty much exactly when needed. And thanks @dongcarl for pointers - didn’t know about systemd-notify command.

One more tip for other readers: systemd-notify doesn’t seem to work if launched from within a script that is executed by startupnotify=/path/to/somescript.sh. That’s no problem startupnotify=systemd-notify --ready; /path/to/somescript.sh works fine.

I have some trouble with this though: it occasionally times out even when timeout is 10 minutes. Is it possible for bitcoind to start more than 10 minutes (seems quite long to me) or perhaps this doesn’t trigger in some cases? I tried several tests using regtest in a VM with decent host HW.

~~I will try with even longer timeouts but perhaps this is an indication there’s something wrong with bitcoind?~~

~~It seems to get stuck on message: Adding fixed seed nodes as DNS doesn't seem to be available.~~

The problem was solved, I actually had overloaded system.

Kixunil commented at 2:07 pm on January 19, 2021: none

Hmm, seems to be affecting mainnet too. :(

whitslack referenced this in commit a38fbb7227 on Jun 27, 2021

whitslack referenced this in commit 698e793a80 on Jun 27, 2021

deadalnix referenced this in commit f681ab5da6 on Oct 14, 2021

PiRK referenced this in commit befab88838 on Aug 16, 2022

DrahtBot locked this on Oct 30, 2022

feature: Added ability for users to add a startup command #15367

Conflicts