bitcoind: Add -daemonwait option to wait for initialization #21007

laanwj · 2021-01-25T22:06:34Z

This adds a -daemonwait flag that does the same as -daemon except that it, from a user perspective, backgrounds the process only after initialization is complete. This is similar to the behaviour of some other software such as c-lightning.

This can be useful when the process launching bitcoind wants to guarantee that either the RPC server is running, or that initialization failed, before continuing. The exit code indicates the initialization result.

The use of the libc function daemon() is replaced by a custom implementation which is inspired by the glibc implementation, but which also creates a pipe from the child to the parent process for communication.

An additional advantage of having our own daemon() implementation is that no MACOS-specific pragmas are needed anymore to silence a deprecation warning.

TODO:

Factor out token_read and token_write to an utility, and use them in shutdown.cpp as well—this is exactly the same kind of communication mechanism.
- RAII-ify pipe endpoints.
Improve granularity of the configure.ac checks. This currently still checks for the function daemon() which makes no sense as it's not used. It should check for individual functions such as
fork() and setsid() etc—the former being required, the second optional.
[-] Signal propagation during initialization: if say, pressing Ctrl-C during -daemonwait it would be good to pass this SIGINT on to the child process instead of detaching the parent process and letting the child run free. This is not necessary, see bitcoind: Add -daemonwait option to wait for initialization #21007 (comment).

Future:

Consider if it makes sense to use this in the RPC tests (there would be no more need for "is RPC ready" polling loops). I think this is out of scope for this PR.

hebasto · 2021-01-25T22:29:47Z

This can be useful when the process launching bitcoind wants to guarantee that either the RPC server is running, or that initialization failed, before continuing. The exit code indicates the initialization result.

Why such behavior couldn't be the default?

src/init.cpp

laanwj · 2021-01-25T22:33:10Z

Why such behavior couldn't be the default?

I didn't do that to avoid that discussion for now 🙂 . It could always be made default at some point in the future if that's what people want.

hebasto · 2021-01-25T22:34:36Z

Concept ACK.

DrahtBot · 2021-01-26T02:08:44Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: replace util::Ref with std::any (C++17) #21366 (refactor: replace util::Ref with std::any (C++17) by theStack)
util: Make default arg values more specific #19471 (util: Make default arg values more specific by hebasto)
multiprocess: Add bitcoin-gui -ipcconnect option #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
multiprocess: Add bitcoin-wallet -ipcconnect option #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
multiprocess: Add basic spawn and IPC support #19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
Multiprocess bitcoin #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

practicalswift · 2021-01-26T09:37:16Z

Concept ACK

theStack · 2021-01-26T18:57:30Z

Concept ACK

promag

Tested ACK 7482634 on macos.

With the following diff

--- a/src/init.cpp
+++ b/src/init.cpp
@@ -780,6 +780,9 @@ static bool InitSanityCheck()

 static bool AppInitServers(const util::Ref& context, NodeContext& node)
 {
+    sleep(5);
+    return false;
+
     const ArgsManager& args = *Assert(node.args);
     RPCServer::OnStarted(&OnRPCStarted);
     RPCServer::OnStopped(&OnRPCStopped);

On master

./src/bitcoind -regtest -daemon  && echo running
Bitcoin Core starting
running

But it's not running

And with this change:

./src/bitcoind -regtest -daemonwait && echo running
Bitcoin Core starting
Error during initializaton - check debug.log for details

Good stuff, still have to review code change.

laanwj · 2021-01-27T13:57:32Z

Thanks for testing! FWIW what I use to force a failure in AppInitMain is

src/bitcoind -regtest -daemonwait -pid=/invalid/path

(pid is, by definition, only written in the daemon process)
But that works too 😄

hebasto · 2021-01-27T14:01:26Z

(pid is, by definition, only written in the daemon process)

Not sure if this is true in the master branch.

laanwj · 2021-01-27T14:11:04Z

Not sure if this is true in the master branch.

I am sure of this. I did not change this at all, and besides, it would be a bug otherwise. After all, what use is the temporary parent process' PID.

hebasto · 2021-01-27T14:17:39Z

... it would be a bug otherwise.

src/bitcoind -daemon=0

creates bitcoind.pid

laanwj · 2021-01-27T14:24:04Z

creates bitcoind.pid

That's not my point. My point is that it creates the PID file after daemonizing (when daemon is enabled). Sure, it also creates the PID file otherwise, that's okay. The only thing I was trying to say is that the option can be used for testing -daemonwait because it fails after daemonizing, nothing more.

jonatack

Tested at 7482634, debug-builds cleanly for me and seems to behave as expected.

invalid path

$ ./src/bitcoind -signet -daemonwait -pid=/invalid/path
Bitcoin Core starting
Error during initializaton - check debug.log for details

(this particular case does not log anything to the debug log)

happy path

$ ./src/bitcoind -signet -daemonwait ; ./src/bitcoin-cli -signet stop
Bitcoin Core starting
Bitcoin Core stopping

debug log is as expected

2021-01-27T17:49:33Z Command-line arg: daemonwait=""
2021-01-27T17:49:33Z Command-line arg: signet=""
2021-01-27T17:49:33Z Using at most 125 automatic connections (1024 file descriptors available)
...
2021-01-27T17:49:35Z init message: Done loading
2021-01-27T17:49:35Z dnsseed thread start
2021-01-27T17:49:35Z Waiting 11 seconds before querying DNS seeds.
2021-01-27T17:49:35Z tor: Thread interrupt
2021-01-27T17:49:35Z addcon thread exit
...

2021-01-27T17:49:36Z Shutdown: done

passing -daemonwait=1 behaves (and logs) as expected as well

Ctrl-C works with a second or two of delay; it seems to not take effect until the wait is released but I'm not sure.

laanwj · 2021-01-27T18:13:08Z

Thanks for testing @jonatack ,

(this particular case does not log anything to the debug log)

That it doesn't log anything is peculiar ! (but unrelated to this PR, I guess, the same problem would happen with -daemon, it would just mysteriously exit in the background).

Ctrl-C works with a second or two of delay; it seems to not take effect until the wait is released but I'm not sure.

It does respond to it? That's interesting. I thought the child process wouldn't get the SIGINT signal at all (hence my remaining TODO). That it doesn't react immediately is normal, It can't interrupt during verification of a block, for example. it's the same when interrupting a non-daemon startup.

jonatack · 2021-01-27T20:23:43Z

Tested with testnet, which is much slower than signet for me (95 sec vs 3)

$ time ./src/bitcoind -testnet -daemonwait=1 && ./src/bitcoin-cli -testnet stop
Bitcoin Core starting

real	1m35.265s
user	0m0.137s
sys	0m0.015s
Bitcoin Core stopping
$

Ctrl-C right after launching interrupts successfully after the same time period elapses

$ time ./src/bitcoind -testnet -daemonwait=1
Bitcoin Core starting
^C
real	1m39.239s
user	0m0.130s
sys	0m0.022s
$

laanwj · 2021-01-28T12:01:54Z

I've looked into this a bit.

It looks like that until the parent process exits, the child process is still attached to the terminal (or at least, shell session), so it still gets the SIGINT from Ctrl-C directly. Even though it did dup2 stdin/stdout/stderr to /dev/null already.

So there appears to be no need to do explicit signal propagation. That's neat. I'll remove that TODO and open this for review.

ajtowns

utACK 8e6d339

src/bitcoind.cpp

src/util/tokenpipe.cpp

jonatack

tACK 8e6d339

Some nit suggestions below, feel free to ignore.

src/Makefile.am

src/util/tokenpipe.cpp

src/init.h

jonatack · 2021-03-01T11:54:46Z

src/bitcoind.cpp

+        // from terminal.
+        int fd = open("/dev/null", O_RDWR);
+        if (fd >= 0) {
+            bool err = dup2(fd, STDIN_FILENO) < 0 || dup2(fd, STDOUT_FILENO) < 0 || dup2(fd, STDERR_FILENO) < 0;


fd and err can be const

I'm not really sure what the common thing is to do for internal variables, do we have a const-unless-required-otherwise recommendation?

Yes, I don't think it's explicitly stated, other than maybe via the C++ Code Guidelines linked to in the developer notes, but it's what I've been doing/reviewing, and seeing others say and do.

jonatack · 2021-03-01T11:55:41Z

src/shutdown.cpp

-            assert(result == 1);
-            break;
-        }
+    int res = g_shutdown_r.TokenRead();


res here and line 54 can be const

hebasto

Approach ACK 8e6d339

src/util/tokenpipe.cpp

src/util/tokenpipe.h

src/bitcoind.cpp

src/util/tokenpipe.h

laanwj · 2021-03-03T12:58:37Z

@hebasto @jonatack @ajtowns
Thanks for the reviews. I have addressed your comments.

jonatack

ACK b83b386 reviewed diff/code, built/tested

Some nits if you retouch.

src/bitcoind.cpp

src/util/tokenpipe.cpp

src/bitcoind.cpp

laanwj · 2021-03-04T14:14:04Z

Thanks @jonatack, have addressed the comments and re-pushed
b83b386c6e03a57f93110dfefdbbf515e62b5753..9a09a494e59b61d7a185da44f9c6736cb536c0c1

jonatack · 2021-03-04T16:09:36Z

re-ACK 9a09a49

This adds a `-daemonwait` flag that does the same as `-daemon` except it, from a user perspective, backgrounds the process only after initialization is complete. This can be useful when the process launching bitcoind wants to guarantee that either the RPC server is running, or that initialization failed, before continuing. The exit code indicates the initialization result. This replaces the use of the libc function `daemon()` by a custom implementation which is inspired by the glibc implementation, but also creates a pipe from the child to the parent process for communication. An additional advantage of having our own `daemon()` implementation is that no MACOS-specific pragmas are needed anymore to silence a deprecation warning.

laanwj · 2021-03-04T17:27:31Z

Repushed for an ordering issue in tokenpipe.cpp reported out-of-band, the implementation file now has same class order as the header
9a09a494e59b61d7a185da44f9c6736cb536c0c1..e017a913d0d78ef0766cf73586fe7a38488e1a26

jonatack · 2021-03-04T18:23:29Z

Tested ACK e017a91 checked change since previous review is move-only

$ time ./src/bitcoind -testnet -daemonwait=1 && ./src/bitcoin-cli -testnet stop
Bitcoin Core starting

real	1m4.095s
user	0m0.132s
sys	0m0.027s
Bitcoin Core stopping

$ time ./src/bitcoind -testnet -daemonwait=1
Bitcoin Core starting
^C 
real	1m5.311s
user	0m0.047s
sys	0m0.023s

…tialization

4d008f9 Always add -daemonwait to known command line arguments (Hennadii Stepanov) Pull request description: This is a follow up of #21007. When `AC_CHECK_DECLS([fork])` fails: - on master (8e65320): ``` $ src/bitcoind -daemonwait Error: Error parsing command line arguments: Invalid parameter -daemonwait ``` - with this PR: ``` $ src/bitcoind -daemonwait Error: -daemon is not supported on this operating system ``` ACKs for top commit: laanwj: Code review ACK 4d008f9 Tree-SHA512: 7fcb5e9d76958adcf57e04fa74bd2a98d62459d81a3c57a97bd74c346cbf47c53e560a15455fb024e912c3b44e8487a83499e993b282871ba069953e665d88a9

…uments 4d008f9 Always add -daemonwait to known command line arguments (Hennadii Stepanov) Pull request description: This is a follow up of bitcoin#21007. When `AC_CHECK_DECLS([fork])` fails: - on master (8e65320): ``` $ src/bitcoind -daemonwait Error: Error parsing command line arguments: Invalid parameter -daemonwait ``` - with this PR: ``` $ src/bitcoind -daemonwait Error: -daemon is not supported on this operating system ``` ACKs for top commit: laanwj: Code review ACK 4d008f9 Tree-SHA512: 7fcb5e9d76958adcf57e04fa74bd2a98d62459d81a3c57a97bd74c346cbf47c53e560a15455fb024e912c3b44e8487a83499e993b282871ba069953e665d88a9

When other initscripts depend on bitcoind, it's because their daemons want to be able to invoke bitcoin-cli or to communicate with bitcoind via RPC. That can't happen until some time after bitcoind has forked into the background when it is started with the -daemon option. The -daemonwait option was added in 92cf3a2 (bitcoin#21007) to address exactly this issue, so let's use it. To avoid blocking startup for arbitrarily long, as users would be annoyed if agetty/display-manager doesn't start quickly, we now mark the bitcoind service initially as inactive at startup and then call back to OpenRC to mark the service as started (and proceed to start any dependent services) after bitcoind forks into the background.

fanquake · 2022-09-15T15:14:22Z

This change was part of 22.0, removing "Needs release note".

laanwj added the Utils/log/libs label Jan 25, 2021

laanwj marked this pull request as draft January 25, 2021 22:07

laanwj force-pushed the 2021-01-daemonwait branch from c886e29 to f88aa35 Compare January 25, 2021 22:16

hebasto reviewed Jan 25, 2021

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

This was referenced Jan 26, 2021

util: Make default arg values more specific #19471

Closed

multiprocess: Add basic spawn and IPC support #19160

Merged

laanwj mentioned this pull request Jan 26, 2021

[Feature request] Wait a while for Bitcoin Core to start instead of crashing immediately ElementsProject/lightning#4355

Open

laanwj force-pushed the 2021-01-daemonwait branch 2 times, most recently from 1b8856c to fc5cc74 Compare January 26, 2021 18:53

laanwj force-pushed the 2021-01-daemonwait branch from fc5cc74 to 7482634 Compare January 26, 2021 19:12

DrahtBot mentioned this pull request Jan 27, 2021

Avoid signed integer overflow and invalid integer negation when loading malformed mempool.dat files #20383

Closed

promag reviewed Jan 27, 2021

View reviewed changes

jonatack reviewed Jan 27, 2021

View reviewed changes

laanwj marked this pull request as ready for review January 28, 2021 12:03

This was referenced Jan 30, 2021

Multiprocess bitcoin #10102

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

laanwj mentioned this pull request Mar 1, 2021

contrib: Current systemd usage doesn't allow for dependencies #21322

Closed

ajtowns reviewed Mar 1, 2021

View reviewed changes

src/bitcoind.cpp Outdated Show resolved Hide resolved

src/bitcoind.cpp Outdated Show resolved Hide resolved

src/util/tokenpipe.cpp Outdated Show resolved Hide resolved

src/util/tokenpipe.cpp Outdated Show resolved Hide resolved

jonatack reviewed Mar 1, 2021

View reviewed changes

hebasto reviewed Mar 1, 2021

View reviewed changes

laanwj force-pushed the 2021-01-daemonwait branch from 8e6d339 to b83b386 Compare March 3, 2021 12:58

jonatack reviewed Mar 3, 2021

View reviewed changes

src/bitcoind.cpp Outdated Show resolved Hide resolved

src/util/tokenpipe.cpp Show resolved Hide resolved

src/bitcoind.cpp Outdated Show resolved Hide resolved

laanwj force-pushed the 2021-01-daemonwait branch from b83b386 to 9a09a49 Compare March 4, 2021 14:06

laanwj added 3 commits March 4, 2021 18:24

util: Add RAII TokenPipe

612f746

shutdown: Use RAII TokenPipe in shutdown

c3e6fde

laanwj force-pushed the 2021-01-daemonwait branch from 9a09a49 to e017a91 Compare March 4, 2021 17:24

DrahtBot mentioned this pull request Mar 5, 2021

refactor: replace util::Ref with std::any (C++17) #21366

Merged

laanwj merged commit 92cf3a2 into bitcoin:master Mar 11, 2021

laanwj added the Needs release note label Mar 11, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 11, 2021

Merge bitcoin#21007: bitcoind: Add -daemonwait option to wait for ini…

51927ee

…tialization

jonatack mentioned this pull request Mar 14, 2021

test: add -daemon and -daemonwait config arg tests #21439

Closed

hebasto mentioned this pull request Mar 16, 2021

Always add -daemonwait to known command line arguments #21447

Merged

whitslack mentioned this pull request Jan 14, 2022

contrib/init: (OpenRC) use -daemonwait to wait for startup completion #24066

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

fanquake removed the Needs release note label Sep 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bitcoind: Add -daemonwait option to wait for initialization #21007

bitcoind: Add -daemonwait option to wait for initialization #21007

laanwj commented Jan 25, 2021 •

edited

Loading

hebasto commented Jan 25, 2021

laanwj commented Jan 25, 2021

hebasto commented Jan 25, 2021

DrahtBot commented Jan 26, 2021 •

edited

Loading

practicalswift commented Jan 26, 2021

theStack commented Jan 26, 2021

promag left a comment

laanwj commented Jan 27, 2021

hebasto commented Jan 27, 2021

laanwj commented Jan 27, 2021

hebasto commented Jan 27, 2021

laanwj commented Jan 27, 2021 •

edited

Loading

jonatack left a comment

laanwj commented Jan 27, 2021

jonatack commented Jan 27, 2021

laanwj commented Jan 28, 2021 •

edited

Loading

ajtowns left a comment

jonatack left a comment

jonatack Mar 1, 2021

laanwj Mar 3, 2021

jonatack Mar 3, 2021

jonatack Mar 1, 2021

hebasto left a comment

laanwj commented Mar 3, 2021

jonatack left a comment •

edited

Loading

laanwj commented Mar 4, 2021

jonatack commented Mar 4, 2021

laanwj commented Mar 4, 2021

jonatack commented Mar 4, 2021

fanquake commented Sep 15, 2022

bitcoind: Add -daemonwait option to wait for initialization #21007

bitcoind: Add -daemonwait option to wait for initialization #21007

Conversation

laanwj commented Jan 25, 2021 • edited Loading

hebasto commented Jan 25, 2021

laanwj commented Jan 25, 2021

hebasto commented Jan 25, 2021

DrahtBot commented Jan 26, 2021 • edited Loading

Conflicts

practicalswift commented Jan 26, 2021

theStack commented Jan 26, 2021

promag left a comment

Choose a reason for hiding this comment

laanwj commented Jan 27, 2021

hebasto commented Jan 27, 2021

laanwj commented Jan 27, 2021

hebasto commented Jan 27, 2021

laanwj commented Jan 27, 2021 • edited Loading

jonatack left a comment

Choose a reason for hiding this comment

laanwj commented Jan 27, 2021

jonatack commented Jan 27, 2021

laanwj commented Jan 28, 2021 • edited Loading

ajtowns left a comment

Choose a reason for hiding this comment

jonatack left a comment

Choose a reason for hiding this comment

jonatack Mar 1, 2021

Choose a reason for hiding this comment

laanwj Mar 3, 2021

Choose a reason for hiding this comment

jonatack Mar 3, 2021

Choose a reason for hiding this comment

jonatack Mar 1, 2021

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

laanwj commented Mar 3, 2021

jonatack left a comment • edited Loading

Choose a reason for hiding this comment

laanwj commented Mar 4, 2021

jonatack commented Mar 4, 2021

laanwj commented Mar 4, 2021

jonatack commented Mar 4, 2021

fanquake commented Sep 15, 2022

laanwj commented Jan 25, 2021 •

edited

Loading

DrahtBot commented Jan 26, 2021 •

edited

Loading

laanwj commented Jan 27, 2021 •

edited

Loading

laanwj commented Jan 28, 2021 •

edited

Loading

jonatack left a comment •

edited

Loading