Return EXIT_FAILURE on post-init fatal errors #27708

It seems odd to return EXIT_SUCCESS when the node aborted execution due a fatal internal error or any post-init problem that triggers an unrequested shutdown.

e.g. blocks or coins db I/O errors, disconnect block failure, failure during thread import (external blocks loading process error), among others.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #27607 (index: improve initialization and pruning violation check by furszy)
• #26966 (index: blockfilter initial sync speedup, parallelize process by furszy)
• #26697 (logging: use bitset for categories by LarryRuane)

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.

Concept ACK

Was wondering if this was attempted before and skimmed through the pull request history, but I did not find prior attempts.

Feedback tackled. Thanks TheCharlatan!. Changes:

• Moved ThreadImport ABC error to use AbortNode. Per comment.
• And moved AbortNode code below StartShutdown so it can use the introduced static function instead of having to add a public StartErrorShutdown function that is only used by the .cpp file and not externally.

Concept ACK

Regarding the conclusion in the first commit ("'StartShutdown' should only be used for user requested shutdowns. Internal errors that cause a shutdown should use 'AbortNode'."), the following is probably another candidate where AbortNode is preferred over StartShutdown?

https://github.com/bitcoin/bitcoin/blob/8cc65f093c0cf7a27b3bfc6da90fd7a6ac8f5e48/src/index/base.cpp#L33-L41

(not saying it necessarily has to happen in this PR though, if it makes sense).

Regarding the conclusion in the first commit ("'StartShutdown' should only be used for user requested shutdowns. Internal errors that cause a shutdown should use 'AbortNode'."), the following is probably another candidate where AbortNode is preferred over StartShutdown?

Yeah absolutely. Good eye. If you compare it in-detail, that function is a plain copy of AbortNode: it (1) calls SetMiscWarning, (2) logs the error, (3) notifies the UI by calling InitError (noui listeners as well) and (4) calls to the shutdown function.. So, we can just get rid of it.

Regarding the conclusion in the first commit ("'StartShutdown' should only be used for user requested shutdowns. Internal errors that cause a shutdown should use 'AbortNode'."), the following is probably another candidate where AbortNode is preferred over StartShutdown?

Yeah absolutely. Good eye. If you compare it in-detail, that function is a plain copy of AbortNode: it (1) calls SetMiscWarning, (2) logs the error, (3) notifies the UI by calling InitError (noui listeners as well) and (4) calls to the shutdown function.. So, we can just get rid of it.

Oh you're right, didn't look at AbortNode before, it's indeed almost the same. Feel free to pull in the dedup commit then: https://github.com/theStack/bitcoin/commit/3eca21000eb33b96063fdcdf30e8ad8514e41126 Note that I still kept the FatalError helper for ease-of-use for the callers, as it still is useful for not having to call tfm::format(...) each time (thanks to simple template magic).

done, cherry-picked 🚜.

Approach ACK d44dd73bcabc694880dfc966fad21980cd3b4c64. Need to review more closely, but these changes look very good and should help with #27711

Code review ACK d44dd73bcabc694880dfc966fad21980cd3b4c64. But it would be nice to extend this change to work with bitcoin-qt as well. I also think it would be good to apply the following changes, which replace the bool shutdown_due_error global with an int exit_status{EXIT_SUCCESS} NodeContext member.

<details><summary>diff</summary> <p>

diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index c305d5e6cdc1..0a2a6ed4ebc2 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -169,6 +169,9 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
         // Set this early so that parameter interactions go to console
         InitLogging(args);
         InitParameterInteraction(args);
+        if (!InitShutdownState(node.exit_status)) {
+            return InitError(Untranslated("Initializing wait-for-shutdown state failed."));
+        }
         if (!AppInitBasicSetup(args)) {
             // InitError will have been called with detailed error, which ends up on console
             return false;
@@ -236,11 +239,6 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
     }
 #endif
     SetSyscallSandboxPolicy(SyscallSandboxPolicy::SHUTOFF);
-    if (fRet) {
-        fRet = WaitForShutdown();
-    }
-    Interrupt(node);
-    Shutdown(node);
 
     return fRet;
 }
@@ -264,5 +262,12 @@ MAIN_FUNCTION
     // Connect bitcoind signal handlers
     noui_connect();
 
-    return (AppInit(node, argc, argv) ? EXIT_SUCCESS : EXIT_FAILURE);
+    if (AppInit(node, argc, argv)) {
+        WaitForShutdown();
+    } else {
+        node.exit_status = EXIT_FAILURE;
+    }
+    Interrupt(node);
+    Shutdown(node);
+    return node.exit_status;
 }
diff --git a/src/init.cpp b/src/init.cpp
index 1ae21ec8d2dd..c4b09eb2373c 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -812,9 +812,6 @@ bool AppInitBasicSetup(const ArgsManager& args)
     // Enable heap terminate-on-corruption
     HeapSetInformation(nullptr, HeapEnableTerminationOnCorruption, nullptr, 0);
 #endif
-    if (!InitShutdownState()) {
-        return InitError(Untranslated("Initializing wait-for-shutdown state failed."));
-    }
 
     if (!SetupNetworking()) {
         return InitError(Untranslated("Initializing networking failed."));
diff --git a/src/node/context.h b/src/node/context.h
index 84f4053c8407..b24f778f4f64 100644
--- a/src/node/context.h
+++ b/src/node/context.h
@@ -7,7 +7,9 @@
 
 #include <kernel/context.h>
 
+#include <atomic>
 #include <cassert>
+#include <cstdlib>
 #include <functional>
 #include <memory>
 #include <vector>
@@ -62,6 +64,7 @@ struct NodeContext {
     interfaces::WalletLoader* wallet_loader{nullptr};
     std::unique_ptr<CScheduler> scheduler;
     std::function<void()> rpc_interruption_point = [] {};
+    std::atomic<int> exit_status{EXIT_SUCCESS};
 
     //! Declare default constructor and destructor that are not inline, so code
     //! instantiating the NodeContext struct doesn't need to #include class
diff --git a/src/shutdown.cpp b/src/shutdown.cpp
index 72f23be21ca0..7641ee3b066a 100644
--- a/src/shutdown.cpp
+++ b/src/shutdown.cpp
@@ -11,6 +11,7 @@
 
 #include <logging.h>
 #include <node/interface_ui.h>
+#include <util/check.h>
 #include <util/tokenpipe.h>
 #include <warnings.h>
 
@@ -21,8 +22,7 @@
 #endif
 
 static std::atomic<bool> fRequestShutdown(false);
-// Whether shutdown was requested by the user or because of an internal error
-static std::atomic<bool> shutdown_due_error{false};
+static std::atomic<int>* g_exit_status{nullptr};
 
 #ifdef WIN32
 /** On windows it is possible to simply use a condition variable. */
@@ -35,8 +35,9 @@ static TokenPipeEnd g_shutdown_r;
 static TokenPipeEnd g_shutdown_w;
 #endif
 
-bool InitShutdownState()
+bool InitShutdownState(std::atomic<int>& exit_status)
 {
+    g_exit_status = &exit_status;
 #ifndef WIN32
     std::optional<TokenPipe> pipe = TokenPipe::Make();
     if (!pipe) return false;
@@ -46,9 +47,8 @@ bool InitShutdownState()
     return true;
 }
 
-static void StartShutdown(bool error)
+void StartShutdown()
 {
-    if (error) shutdown_due_error.exchange(error);
 #ifdef WIN32
     std::unique_lock<std::mutex> lk(g_shutdown_mutex);
     fRequestShutdown = true;
@@ -68,8 +68,6 @@ static void StartShutdown(bool error)
 #endif
 }
 
-void StartShutdown() { return StartShutdown(/*error=*/false); }
-
 bool AbortNode(const std::string& strMessage, bilingual_str user_message)
 {
     SetMiscWarning(Untranslated(strMessage));
@@ -78,7 +76,8 @@ bool AbortNode(const std::string& strMessage, bilingual_str user_message)
         user_message = _("A fatal internal error occurred, see debug.log for details");
     }
     InitError(user_message);
-    StartShutdown(/*error=*/true);
+    Assert(g_exit_status)->store(EXIT_FAILURE);
+    StartShutdown();
     return false;
 }
 
@@ -97,7 +96,7 @@ bool ShutdownRequested()
     return fRequestShutdown;
 }
 
-bool WaitForShutdown()
+void WaitForShutdown()
 {
 #ifdef WIN32
     std::unique_lock<std::mutex> lk(g_shutdown_mutex);
@@ -109,5 +108,4 @@ bool WaitForShutdown()
         assert(0);
     }
 #endif
-    return !shutdown_due_error;
 }
diff --git a/src/shutdown.h b/src/shutdown.h
index 34d5d30bc074..2b8fdb779ac6 100644
--- a/src/shutdown.h
+++ b/src/shutdown.h
@@ -8,10 +8,12 @@
 
 #include <util/translation.h> // For bilingual_str
 
+#include <atomic>
+
 /** Initialize shutdown state. This must be called before using either StartShutdown(),
  * AbortShutdown() or WaitForShutdown(). Calling ShutdownRequested() is always safe.
  */
-bool InitShutdownState();
+bool InitShutdownState(std::atomic<int>& exit_status);
 
 /** Request shutdown of the application. */
 void StartShutdown();
@@ -29,9 +31,7 @@ bool ShutdownRequested();
 
 /** Wait for StartShutdown to be called in any thread. This can only be used
  * from a single thread.
- * Returns true if shutdown was requested by the user. Otherwise, it's an
- * internally requested shutdown which only happens when an error arises.
  */
-bool WaitForShutdown();
+void WaitForShutdown();
 
 #endif // BITCOIN_SHUTDOWN_H

</p> </details>

I think this approach has a few advantages:

• Avoids adding a new shutdown global right before #27711 removes all the shutdown globals, so there is less cleanup #27711 needs to do later.
• I think semantics of an exit_status field are more obvious than a shutdown_due_error field. Easier to think concretely about what the exit code will be than abstractly about "whether shutdown was requested by the user".
• Having a NodeContext::exit_status field should make it easier to port this change to the GUI, since the GUI doesn't use the WaitForShutdown function.

(EDIT: Updated code suggestion above to fix early return bug in initial post)

Thanks @ryanofsky! I see most of the changes great, except one.

If we change AppInit return value to be an int, we need to change all the return lines to return an int, not only the ones at the end of the function.

Otherwise, the return false (or return error(msg)) statements will be treated as return 0 which means EXIT_SUCCESS, which is the opposite that we want.

Other than that nuance, pulling them :).

Oh, you are right. I thought I searched for early returns, but somehow I missed seeing them. I updated the suggestion above to fix this. I think AppInit should keep returning bool not int to keep the changes minimal, and because returning bool is better for following existing coding conventions.

Thanks ryanofsky, updated per feedback with a tiny diff:

Didn't move InitShutdownState from AppInitBasicSetup to AppInit and instead provided the NodeContext ref to AppInitBasicSetup. Not sure if you have an strong opinion about this (shoot if you have it) but the rationale was to keep the same workflow as we have now so the bitcoin-qt related changes b0267f2 doesn't introduce another InitShutdownState call.

I think AppInit should keep returning bool not int to keep the changes minimal, and because returning bool is better for following existing coding conventions.

Funny enough, GuiMain() function returns an int. I think that we just don't have a convention here. Still, agree to follow the simpler path.

Another topic would be whether to drop 99d3212 or not. It is not needed anymore. I was going to drop it but.. it feels good to place the global booleans first in the .cpp file and not in the middle of it. Thoughts?

Reworked the diff a bit more check.

It's because AppInit can return true without initializing the shutdown pipe. e.g. when the user starts bitcoind with a -h or -version. Which leads to an assertion error at the WaitForShutdown call.

Probably we should refactor AppInit in the future, I liked where you were going.

Code review ACK f13880e83200ec824d7528061f96708d96bf9bd4

This looks good as is, but would suggest two changes if you feel like making them:

1. Maybe drop the "move 'AbortNode' after 'StartShutdown'" commit if it is not needed anymore. Dropping it would help git blame work better and make history more legible. Also #27711 is going to remove AbortNode so moving it now will cause an uglier conflict.

2. I think it would be better if the main() function always returned node.exit_status instead conditionally returning it. Better to make it clear that setting node.exit_status always determines the exit code and not create doubt about whether the value will be ignored. Would suggest the following (despite slightly longer diff)

diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index 71b2eef814c6..4133269b3433 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -236,13 +236,6 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
     }
 #endif
     SetSyscallSandboxPolicy(SyscallSandboxPolicy::SHUTOFF);
-    if (fRet) {
-        WaitForShutdown();
-    } else {
-        node.exit_status = EXIT_FAILURE;
-    }
-    Interrupt(node);
-    Shutdown(node);
 
     return fRet;
 }
@@ -266,5 +259,13 @@ MAIN_FUNCTION
     // Connect bitcoind signal handlers
     noui_connect();
 
-    return AppInit(node, argc, argv) ? node.exit_status.load() : EXIT_FAILURE;
+    if (AppInit(node, argc, argv)) {
+        WaitForShutdown();
+    } else {
+        node.exit_status = EXIT_FAILURE;
+    }
+    Interrupt(node);
+    Shutdown(node);
+
+    return node.exit_status;
 }

I think this would be a general improvement anyway because it reduces complexity of AppInit and make it a pure initialization function, not a function that initializes, interrupts, and waits.

2. I think it would be better if the main() function always returned node.exit_status instead conditionally returning it. Better to make it clear that setting node.exit_status always determines the exit code and not create doubt about whether the value will be ignored. I think this would be a general improvement anyway because it reduces complexity of AppInit and make it a pure initialization function, not a function that initializes, interrupts, and waits.

Hmm, isn't the diff exactly what I changed in my last push? check #27708 (comment).

The bug there is that AppInit can return true without initializing the shutdown pipe, which makes WaitForShutdown assertion fail (cannot call WaitForShutdown without initializing the pipe). The user could have provided the -help or -version arguments, see, which prints information and return early without initializing the node.

Overall, I like where those changes are going, but they require to refactor AppInit further so the args parsing step, and the execution of commands that return early without initializing the node, are performed in a different function. If you feel strong on adding it here, I could add it. Not really a thing to introduce more changes for me.

Maybe drop the "move 'AbortNode' after 'StartShutdown'" commit if it is not needed anymore. Dropping it would help git blame work better and make history more legible. Also #27711 is going to remove AbortNode so moving it now will cause an uglier conflict.

k good, didn't know about 27711 removing it. Will drop it.

Have quickly drafted the needed changes to make your diff work so we are in the same page, check 61fe1af.

Code review ACK 61fe1af01418d8b93d38aba8aeb65aecfa6a95cd. This looks very good.

Appreciate you cleaning up the code to make it more obvious when the new exit_status value will and won't be returned. (And sorry I posted another buggy diff that forgot about the early returns!) Left a few more minor suggestions that you could take or leave.

Thanks @ryanofsky! Feedback tackled. Small diff.

Code review ACK da60d86bfbe7fe49ce7181b5b394c0e3d8c53854

Thanks for following up on earlier suggestions. Left some new comments below, but this also looks good in it's current form.

Updated per feedback, thanks ryanofsky. Tiny diff:

• Removed BitcoinApplication::getExitStatus.
• Moved exit_status.store(EXIT_FAILURE) to the interface appInitMain() method to be compatible with #10102.

concept ACK

I was looking for this writing #27850 was surprised that a fatal internal error didn't satisfy the python test for assert init error on startup

ACK 61c569ab6069d04079a0831468eb713983919636

Code review ACK 61c569ab6069d04079a0831468eb713983919636

This should be a really nice change because it actually makes bitcoind return an error code when there is an error. But it also clears out StartShutdown clutter and makes the main() function comprehensible, so is a nice code improvment.

I think this would be ready for merge if another reviewer added an ACK.

ACK 61c569ab6069d04079a0831468eb713983919636

A few non-breaking questions below. Reviewed each commit and ran all tests locally. Confirmed that #2039 behavior now exits with code 1 instead of 0 like it did on master. Also confirmed that remaining StartShutdown() calls are all user-triggered.

<details><summary>Show Signature</summary>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 61c569ab6069d04079a0831468eb713983919636
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmSHODIACgkQ5+KYS2KJ
yTpd4g/+I56MeCxiLqHXVtFe2yUw9JY1wYNeo6kuLU2A5Yx+kWeTBKZl9dA/lTIu
5/TYbFaTai5px1HZzynnfD+XrFLtjaxynGqHhbfRmX8cGUyo5WkRRY6IcYUuqiXs
FBUsTqlXrEq00j012RL63xpiZhswUZTEpw3BzHD5c1IlK7FkdCWCVT0xl3o68ivs
W0pfXRXnAcAtseWgMlrZtPHGaEsQS1CMHAaz2KXvRPbOCu0p0vjAuGkzqpBZuu3x
2kzVcPnuUunb6F+t62Q2Mrk4bCuKYlT9I5CozGVV8W9rWJODvzKyTCmTLCXbIxC0
MrGN6DHn0AzBbCBm6E65o5B80czNzM0Wci/7ZElmjBkDJxWP0KPDtaNp9QsazYbO
AVWhKfMMBDEqmwRyXvMK1iv/zJrZ2Y2QQf/LPt1vbLjVwHT2wF7HyEOUJHKKmrI2
fLWf62JKvBvrrNdeb12s5ZaMwMgcM5UA/8H2YuDw/zOvgJYZZiXAiRI3ETDvsm5B
dFIgn0M5njMq3UFgRl2hHW/qBwKiufs8QvrKw8zxuXxF5H+4sQcwCVtiwe//dsDY
179WwJQcQ3t7iwg1TNgscv9nVY5nRJvPzHs4iopLbPhks60akDic1g6ruMCdbAEj
/Md+oVk7d59HJLvhDlR6xpS8iKycyadFcsiMlOuK/Xn56ZPLKt0=
=N/HN
-----END PGP SIGNATURE-----

pinheadmz's public key is on keybase

</details>

Code-review ACK 61c569ab6069d04079a0831468eb713983919636

left a question / test nit for follow-up