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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | TheCharlatan, ryanofsky, pinheadmz, theStack |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
18@@ -19,6 +19,9 @@ bool InitShutdownState();
19 /** Request shutdown of the application. */
20 void StartShutdown();
21
22+/** Request application shutdown due an internal error. */
23+void StartErrorShutdown();
AbortNode() instead. Same as we do with the external block import failures.
shutdown.cpp, so AbortNode is placed after the static StartErrorShutdown function. Which I’m not so sure that worth it due the conflicts that could cause with your PRs. Thoughts?
StartErrorShutdown?
Which.. isn’t bad as it will remove another ui_interface dependency.
Can you elaborate on this point? AbortNode still relies on ui_interface from what I can tell.
From my current understanding both the StartShutdown in ThreadImport and in the check are call shutdown and node AbortNode, because BlockImport might run before the ui is available (this is me guessing out of historical reasons). I am not sold on this point though, since this runs in another thread and we also provide the noui consumer of the ui interface. So I think these should both become calls to AbortNode.
Which.. isn’t bad as it will remove another ui_interface dependency.
Can you elaborate on this point?
AbortNodestill relies onui_interfacefrom what I can tell.
Ok, sorry for the confusion.
The ui_interface dependency that can be removed is the base.cpp one. Thus why said to “bubble up” the error string up to init.cpp instead of calling InitError from the index base class Start method.
From my current understanding both the
StartShutdowninThreadImportand in the check are call shutdown and nodeAbortNode, becauseBlockImportmight run before the ui is available (this is me guessing out of historical reasons).
AbortNode isn’t doing anything special. We already use InitError in init.cpp prior spawning the import thread.
What the node will do when there is no gui is print the error to the command line, and the debug file, prior shutting down (using the noui consumer as you said).
I am not sold on this point though, since this runs in another thread and we also provide the noui consumer of the ui interface. So I think these should both become calls to
AbortNode.
Yeah, that is my idea as well. The ThreadImport “fatal” failures should use AbortNode.
31@@ -29,7 +32,9 @@ bool ShutdownRequested();
32
33 /** Wait for StartShutdown to be called in any thread. This can only be used
34 * from a single thread.
35+ * Returns true if shutdown was requested by the user. Otherwise, it's an
36+ * internally requested shutdown which only happens when an error arises.
37 */
38-void WaitForShutdown();
39+bool WaitForShutdown();
[[nodiscard]] here?
AbortShutdown().
31@@ -29,7 +32,9 @@ bool ShutdownRequested();
32
33 /** Wait for StartShutdown to be called in any thread. This can only be used
34 * from a single thread.
35+ * Returns true if shutdown was requested by the user. Otherwise, it's an
36+ * internally requested shutdown which only happens when an error arises.
stop_at_height set, but shutdown is still triggered internally. There is also one error case in https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L931 that triggers a normal shutdown.
This comment seems a bit inaccurate, e.g. we may have
stop_at_heightset, but shutdown is still triggered internally. There is also one error case in https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L931 that triggers a normal shutdown.
I would label stop_at_height as an action requested by the user. The user had to provide -stopatheight at startup so the shutdown is expected.
On the other hand, https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L931 seems to be a good candidate for AbortNode. It’s a error in the ThreadImport, so either the db data got modified in some wrong way or the user provided invalid external blocks.
Feedback tackled. Thanks TheCharlatan!. Changes:
AbortNode. Per comment.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?
(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
AbortNodeis preferred overStartShutdown?
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
AbortNodeis preferred overStartShutdown?Yeah absolutely. Good eye. If you compare it in-detail, that function is a plain copy of
AbortNode: it (1) callsSetMiscWarning, (2) logs the error, (3) notifies the UI by callingInitError(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).
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.
0diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
1index c305d5e6cdc1..0a2a6ed4ebc2 100644
2--- a/src/bitcoind.cpp
3+++ b/src/bitcoind.cpp
4@@ -169,6 +169,9 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
5 // Set this early so that parameter interactions go to console
6 InitLogging(args);
7 InitParameterInteraction(args);
8+ if (!InitShutdownState(node.exit_status)) {
9+ return InitError(Untranslated("Initializing wait-for-shutdown state failed."));
10+ }
11 if (!AppInitBasicSetup(args)) {
12 // InitError will have been called with detailed error, which ends up on console
13 return false;
14@@ -236,11 +239,6 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
15 }
16 #endif
17 SetSyscallSandboxPolicy(SyscallSandboxPolicy::SHUTOFF);
18- if (fRet) {
19- fRet = WaitForShutdown();
20- }
21- Interrupt(node);
22- Shutdown(node);
23
24 return fRet;
25 }
26@@ -264,5 +262,12 @@ MAIN_FUNCTION
27 // Connect bitcoind signal handlers
28 noui_connect();
29
30- return (AppInit(node, argc, argv) ? EXIT_SUCCESS : EXIT_FAILURE);
31+ if (AppInit(node, argc, argv)) {
32+ WaitForShutdown();
33+ } else {
34+ node.exit_status = EXIT_FAILURE;
35+ }
36+ Interrupt(node);
37+ Shutdown(node);
38+ return node.exit_status;
39 }
40diff --git a/src/init.cpp b/src/init.cpp
41index 1ae21ec8d2dd..c4b09eb2373c 100644
42--- a/src/init.cpp
43+++ b/src/init.cpp
44@@ -812,9 +812,6 @@ bool AppInitBasicSetup(const ArgsManager& args)
45 // Enable heap terminate-on-corruption
46 HeapSetInformation(nullptr, HeapEnableTerminationOnCorruption, nullptr, 0);
47 #endif
48- if (!InitShutdownState()) {
49- return InitError(Untranslated("Initializing wait-for-shutdown state failed."));
50- }
51
52 if (!SetupNetworking()) {
53 return InitError(Untranslated("Initializing networking failed."));
54diff --git a/src/node/context.h b/src/node/context.h
55index 84f4053c8407..b24f778f4f64 100644
56--- a/src/node/context.h
57+++ b/src/node/context.h
58@@ -7,7 +7,9 @@
59
60 #include <kernel/context.h>
61
62+#include <atomic>
63 #include <cassert>
64+#include <cstdlib>
65 #include <functional>
66 #include <memory>
67 #include <vector>
68@@ -62,6 +64,7 @@ struct NodeContext {
69 interfaces::WalletLoader* wallet_loader{nullptr};
70 std::unique_ptr<CScheduler> scheduler;
71 std::function<void()> rpc_interruption_point = [] {};
72+ std::atomic<int> exit_status{EXIT_SUCCESS};
73
74 //! Declare default constructor and destructor that are not inline, so code
75 //! instantiating the NodeContext struct doesn't need to #include class
76diff --git a/src/shutdown.cpp b/src/shutdown.cpp
77index 72f23be21ca0..7641ee3b066a 100644
78--- a/src/shutdown.cpp
79+++ b/src/shutdown.cpp
80@@ -11,6 +11,7 @@
81
82 #include <logging.h>
83 #include <node/interface_ui.h>
84+#include <util/check.h>
85 #include <util/tokenpipe.h>
86 #include <warnings.h>
87
88@@ -21,8 +22,7 @@
89 #endif
90
91 static std::atomic<bool> fRequestShutdown(false);
92-// Whether shutdown was requested by the user or because of an internal error
93-static std::atomic<bool> shutdown_due_error{false};
94+static std::atomic<int>* g_exit_status{nullptr};
95
96 #ifdef WIN32
97 /** On windows it is possible to simply use a condition variable. */
98@@ -35,8 +35,9 @@ static TokenPipeEnd g_shutdown_r;
99 static TokenPipeEnd g_shutdown_w;
100 #endif
101
102-bool InitShutdownState()
103+bool InitShutdownState(std::atomic<int>& exit_status)
104 {
105+ g_exit_status = &exit_status;
106 #ifndef WIN32
107 std::optional<TokenPipe> pipe = TokenPipe::Make();
108 if (!pipe) return false;
109@@ -46,9 +47,8 @@ bool InitShutdownState()
110 return true;
111 }
112
113-static void StartShutdown(bool error)
114+void StartShutdown()
115 {
116- if (error) shutdown_due_error.exchange(error);
117 #ifdef WIN32
118 std::unique_lock<std::mutex> lk(g_shutdown_mutex);
119 fRequestShutdown = true;
120@@ -68,8 +68,6 @@ static void StartShutdown(bool error)
121 #endif
122 }
123
124-void StartShutdown() { return StartShutdown(/*error=*/false); }
125-
126 bool AbortNode(const std::string& strMessage, bilingual_str user_message)
127 {
128 SetMiscWarning(Untranslated(strMessage));
129@@ -78,7 +76,8 @@ bool AbortNode(const std::string& strMessage, bilingual_str user_message)
130 user_message = _("A fatal internal error occurred, see debug.log for details");
131 }
132 InitError(user_message);
133- StartShutdown(/*error=*/true);
134+ Assert(g_exit_status)->store(EXIT_FAILURE);
135+ StartShutdown();
136 return false;
137 }
138
139@@ -97,7 +96,7 @@ bool ShutdownRequested()
140 return fRequestShutdown;
141 }
142
143-bool WaitForShutdown()
144+void WaitForShutdown()
145 {
146 #ifdef WIN32
147 std::unique_lock<std::mutex> lk(g_shutdown_mutex);
148@@ -109,5 +108,4 @@ bool WaitForShutdown()
149 assert(0);
150 }
151 #endif
152- return !shutdown_due_error;
153 }
154diff --git a/src/shutdown.h b/src/shutdown.h
155index 34d5d30bc074..2b8fdb779ac6 100644
156--- a/src/shutdown.h
157+++ b/src/shutdown.h
158@@ -8,10 +8,12 @@
159
160 #include <util/translation.h> // For bilingual_str
161
162+#include <atomic>
163+
164 /** Initialize shutdown state. This must be called before using either StartShutdown(),
165 * AbortShutdown() or WaitForShutdown(). Calling ShutdownRequested() is always safe.
166 */
167-bool InitShutdownState();
168+bool InitShutdownState(std::atomic<int>& exit_status);
169
170 /** Request shutdown of the application. */
171 void StartShutdown();
172@@ -29,9 +31,7 @@ bool ShutdownRequested();
173
174 /** Wait for StartShutdown to be called in any thread. This can only be used
175 * from a single thread.
176- * Returns true if shutdown was requested by the user. Otherwise, it's an
177- * internally requested shutdown which only happens when an error arises.
178 */
179-bool WaitForShutdown();
180+void WaitForShutdown();
181
182 #endif // BITCOIN_SHUTDOWN_H
I think this approach has a few advantages:
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”.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 :).
'StartShutdown' should only be used for user requested
shutdowns. Internal errors that cause a shutdown should
use 'AbortNode'.
Deduplicates code in the `FatalError` template function by using
`AbortNode` which does the exact same thing if called without any user
message (i.e. without second parameter specified). The template is still
kept for ease-of-use w.r.t. not having to call `tfm::format(...)` at the
call-side each time, and also to keep the diff minimal.
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:
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.
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)
0diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
1index 71b2eef814c6..4133269b3433 100644
2--- a/src/bitcoind.cpp
3+++ b/src/bitcoind.cpp
4@@ -236,13 +236,6 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
5 }
6 #endif
7 SetSyscallSandboxPolicy(SyscallSandboxPolicy::SHUTOFF);
8- if (fRet) {
9- WaitForShutdown();
10- } else {
11- node.exit_status = EXIT_FAILURE;
12- }
13- Interrupt(node);
14- Shutdown(node);
15
16 return fRet;
17 }
18@@ -266,5 +259,13 @@ MAIN_FUNCTION
19 // Connect bitcoind signal handlers
20 noui_connect();
21
22- return AppInit(node, argc, argv) ? node.exit_status.load() : EXIT_FAILURE;
23+ if (AppInit(node, argc, argv)) {
24+ WaitForShutdown();
25+ } else {
26+ node.exit_status = EXIT_FAILURE;
27+ }
28+ Interrupt(node);
29+ Shutdown(node);
30+
31+ return node.exit_status;
32 }
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.
- I think it would be better if the main() function always returned
node.exit_statusinstead conditionally returning it. Better to make it clear that settingnode.exit_statusalways 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 ofAppInitand 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.
111@@ -112,12 +112,8 @@ int fork_daemon(bool nochdir, bool noclose, TokenPipeEnd& endpoint)
112
113 #endif
114
115-static bool AppInit(NodeContext& node, int argc, char* argv[])
116+static bool ParseArgs(NodeContext& node, int argc, char* argv[])
In commit “refactor: decouple early return commands from AppInit” (61fe1af01418d8b93d38aba8aeb65aecfa6a95cd)
Would be nice to just pass ArgsManager& to this function instead of NodeContext& node, since accessing other parts of the context is not needed.
799@@ -800,7 +800,7 @@ std::set<BlockFilterType> g_enabled_filter_types;
800 std::terminate();
801 };
802
803-bool AppInitBasicSetup(const ArgsManager& args)
804+bool AppInitBasicSetup(const ArgsManager& args, NodeContext& node)
In commit “refactor: index: use AbortNode in fatal error helper” (3c06926cf21dcca3074ef51506f556b2286c299b)
It would be slightly better to just pass a reference to node.exit_status instead of passing the whole NodeContext, because passing NodeContext exposes a lot of other state this function doesn’t need to have access to. But it’s not important because #27711 gets rid of the InitShutdownState function and will drop the parameter anyway.
exit_status ref.
398@@ -398,8 +399,7 @@ void BitcoinApplication::initializeResult(bool success, interfaces::BlockAndHead
399 qDebug() << __func__ << ": Initialization result: " << success;
400
401 // Set exit result.
In commit “gui: return EXIT_FAILURE on post-init fatal errors” (c8806e972f2be9f8a05bf11b1f6a8a2e70e01962)
Should delete the “Set exit result” comment here or move it down below. Probably fine to delete because exit_status.store(EXIT_FAILURE) should pretty clear by itself.
464@@ -464,6 +465,10 @@ void BitcoinApplication::handleNonFatalException(const QString& message)
465 QLatin1String("<br><br>") + GUIUtil::MakeHtmlLink(message, PACKAGE_BUGREPORT));
466 }
467
468+int BitcoinApplication::getExitStatus() const {
469+ return node().context()->exit_status.load();
In commit “gui: return EXIT_FAILURE on post-init fatal errors” (c8806e972f2be9f8a05bf11b1f6a8a2e70e01962)
This is ok for now, but if you want to future-proof the code and make it compatible with #10102, it would be better to add an interfaces::Node::getExitStatus() method and have GUI call node().getExitStatus(). The problem is that with #10102, node().context() will return null when the node is running in a different process than the GUI.
I also think it might be clearer just to delete the BitcoinApplication::getExitStatus method entirely and just return node().getExitStatus() at the end of GuiMain()
EDIT: I guess you would also need to add a interfaces::Node::setExitStatus() method as well to replace ’m_node->context()->exit_status.store` above. Would be nice, but this is also fine for now.
I also think it might be clearer just to delete the BitcoinApplication::getExitStatus method entirely and just return node().getExitStatus() at the end of GuiMain()
That was my first idea too but GuiMain() is a static function with no direct access to the node interface. The interface is created inside the BitcoinApplication::createNode class method.
This is ok for now, but if you want to future-proof the code and make it compatible with #10102, it would be better to add an interfaces::Node::getExitStatus() method and have GUI call node().getExitStatus().
Yeah, good. Added getExitStatus() to the node interface.
re: #27708 (review)
That was my first idea too but
GuiMain()is a static function with no direct access to the node interface.
I think it can just do return app.node().getExitStatus();. The app.node() method is public and already used in GuiMain()
171@@ -153,17 +172,6 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
172 std::any context{&node};
173 try
174 {
175- if (auto error = common::InitConfig(args)) {
In commit “refactor: decouple early return commands from AppInit” (61fe1af01418d8b93d38aba8aeb65aecfa6a95cd)
Note for reviewers: Moving the common::InitConfig call out of the try block means if InitConfig throws an exception it won’t be caught. But this is good and should not change behavior because InitConfig is already catching std::exception internally and should just be doing basic parsing and not triggering other types of exceptions.
IsSwitchChar().
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.
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.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
439@@ -441,6 +440,7 @@ void BitcoinApplication::initializeResult(bool success, interfaces::BlockAndHead
440 #endif
441 pollShutdownTimer->start(SHUTDOWN_POLLING_DELAY);
442 } else {
443+ m_node->context()->exit_status.store(EXIT_FAILURE);
In commit “gui: return EXIT_FAILURE on post-init fatal errors” (624340f9acbdd3c2d54c3824248e2c0cff4cf487)
Not necessary to address here, but with #10102 when the node and GUI are running in separate processes, m_node->context() will be null, so this will crash.
You could fix and future-proof this by adding a interfaces::Node::setExitStatus() function, or just update the existing appInitMain() function to set the error code:
0 bool appInitMain(interfaces::BlockAndHeaderTipInfo* tip_info) override
1 {
2 if (!AppInitMain(*m_context, tip_info)) {
3 m_context->exit_status.store(EXIT_FAILURE);
4 return false;
5 }
6 return true;
7 }
If you let appInitMain set the error code, you could also simplify GUI code even more, skipping Q_EMIT initializeResult(rv, tip_info) when appInitMain fails and getting rid of the rv parameter.
You could fix and future-proof this by adding a interfaces::Node::setExitStatus() function, or just update the existing appInitMain() function to set the error code.
Great, added it. Thanks!
If you let appInitMain set the error code, you could also simplify GUI code even more, skipping Q_EMIT initializeResult(rv, tip_info) when appInitMain fails and getting rid of the rv parameter.
Hmm, that would be nice but, in a quick glance, I don’t think that we can remove the rv parameter without introducing another signal to the InitExecutor class (which wouldn’t be bad). Because we still need to call to the shutdown function at the GUI level: the BitcoinApplication::requestShutdown that is called inside BitcoinApplication::initializeResult when the rv parameter is false.
Still, even when I would love to keep cleaning stuff, maybe better to leave it for a follow-up so the gui related changes are minimal here.
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.
Cleaned up the init flow to make it more obvious when
the 'exit_status' value will and won't be returned.
This is because it was confusing that `AppInit` was
returning true under two different circumstances:
1) When bitcoind was launched only to retrieve the "-help"
or "-version" information. In this case, the app was
not initialized.
2) When the user triggers a shutdown. In this case,
the app was fully initialized.
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.
30@@ -28,6 +31,7 @@ bool AbortNode(const std::string& strMessage, bilingual_str user_message)
31 user_message = _("A fatal internal error occurred, see debug.log for details");
32 }
33 InitError(user_message);
34+ Assert(g_exit_status)->store(EXIT_FAILURE);
Does this make InitShutdownState() required before calling AbortNode() ?
I suppose that could be added to the comment here. I’m just making sure I understand, I don’t think you need to retouch:
20@@ -20,6 +21,8 @@
21 #include <condition_variable>
22 #endif
23
24+static std::atomic<int>* g_exit_status{nullptr};
NodeContext::exit_status?
FWIW, this could be de-globalized again by the interrupt class introduced in #27861.
Yes #27861 replaces the AbortNode function with a KernelNotifications::fatalError method that can access whatever state it needs through the KernelNotifications class without needing any globals. AbortNode is the only function using g_exit_status so it should just disappear then.
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.
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA256
2
3ACK 61c569ab6069d04079a0831468eb713983919636
4-----BEGIN PGP SIGNATURE-----
5
6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmSHODIACgkQ5+KYS2KJ
7yTpd4g/+I56MeCxiLqHXVtFe2yUw9JY1wYNeo6kuLU2A5Yx+kWeTBKZl9dA/lTIu
85/TYbFaTai5px1HZzynnfD+XrFLtjaxynGqHhbfRmX8cGUyo5WkRRY6IcYUuqiXs
9FBUsTqlXrEq00j012RL63xpiZhswUZTEpw3BzHD5c1IlK7FkdCWCVT0xl3o68ivs
10W0pfXRXnAcAtseWgMlrZtPHGaEsQS1CMHAaz2KXvRPbOCu0p0vjAuGkzqpBZuu3x
112kzVcPnuUunb6F+t62Q2Mrk4bCuKYlT9I5CozGVV8W9rWJODvzKyTCmTLCXbIxC0
12MrGN6DHn0AzBbCBm6E65o5B80czNzM0Wci/7ZElmjBkDJxWP0KPDtaNp9QsazYbO
13AVWhKfMMBDEqmwRyXvMK1iv/zJrZ2Y2QQf/LPt1vbLjVwHT2wF7HyEOUJHKKmrI2
14fLWf62JKvBvrrNdeb12s5ZaMwMgcM5UA/8H2YuDw/zOvgJYZZiXAiRI3ETDvsm5B
15dFIgn0M5njMq3UFgRl2hHW/qBwKiufs8QvrKw8zxuXxF5H+4sQcwCVtiwe//dsDY
16179WwJQcQ3t7iwg1TNgscv9nVY5nRJvPzHs4iopLbPhks60akDic1g6ruMCdbAEj
17/Md+oVk7d59HJLvhDlR6xpS8iKycyadFcsiMlOuK/Xn56ZPLKt0=
18=N/HN
19-----END PGP SIGNATURE-----
pinheadmz’s public key is on keybase
376@@ -377,17 +377,23 @@ def is_node_stopped(self):
377 return False
378
379 # process has stopped. Assert that it didn't return an error code.
380- assert return_code == 0, self._node_msg(
381- "Node returned non-zero exit code (%d) when stopping" % return_code)
382+ # unless 'expected_ret_code' is provided.
383+ if expected_ret_code is not None:
384+ assert return_code == expected_ret_code, self._node_msg(
385+ "Node returned unexpected exit code (%d) vs (%d) when stopping" % (return_code, expected_ret_code))
nit, could take the opportunity to change to f-strings here (and below), as suggested in the functional test style guidelines:
0 f"Node returned unexpected exit code ({return_code}) vs ({expected_ret_code}) when stopping")
381- "Node returned non-zero exit code (%d) when stopping" % return_code)
382+ # unless 'expected_ret_code' is provided.
383+ if expected_ret_code is not None:
384+ assert return_code == expected_ret_code, self._node_msg(
385+ "Node returned unexpected exit code (%d) vs (%d) when stopping" % (return_code, expected_ret_code))
386+ else:
In 3b2c61e8198bcefb1c2343caff1d705951026cc4:
Any reason to make expected_ret_code optional? Seems easier to just make the default value 0 and drop this whole else block.