Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly #28051

pull ryanofsky wants to merge 11 commits into bitcoin:master from ryanofsky:pr/noshut changing 43 files +164 −192
  1. ryanofsky commented at 10:26 pm on July 7, 2023: contributor

    This change drops shutdown.h and shutdown.cpp files, replacing them with a NodeContext::shutdown member which is used to trigger shutdowns directly. This gets rid of an unnecessary layer of indirection, and allows getting rid of the kernel::g_context global.

    Additionally, this PR tries to improve error handling of SignalInterrupt code by marking relevant methods [[nodiscard]] to avoid the possibility of uncaught exceptions mentioned #27861 (review).

    Behavior is changing In a few cases which are noted in individual commit messages. Particularly: GUI code more consistently interrupts RPCs when it is shutting down, shutdown state no longer persists between unit tests, the stop RPC now returns an RPC error if requesting shutdown fails instead of aborting, and other failed shutdown calls now log errors instead of aborting.

    This PR is a net reduction in lines of code, but in some cases the explicit error handling and lack of global shutdown functions do make it more verbose. The verbosity can be seen as good thing if it discourages more code from directly triggering shutdowns, and instead encourages code to return errors or send notifications that could be translated into shutdowns. Probably a number of existing shutdown calls could just be replaced by better error handling.

  2. DrahtBot commented at 10:26 pm on July 7, 2023: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK TheCharlatan, maflcko, stickies-v, achow101
    Concept ACK fanquake

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28960 (kernel: Remove dependency on CScheduler by TheCharlatan)
    • #28956 (Nuke adjusted time (attempt 2) by dergoegge)
    • #28863 (wallet, mempool: propagete checkChainLimits error message to wallet by ismaelsadeeq)
    • #28843 ([refactor] Remove BlockAssembler m_mempool member by TheCharlatan)
    • #28830 ([refactor] Check CTxMemPool options in ctor by TheCharlatan)
    • #28792 (Embedding ASMap files as binary dump header file by fjahr)
    • #28228 (kernel: Run sanity checks on context construction by TheCharlatan)
    • #28052 (blockstorage: XOR blocksdir *.dat files by maflcko)
    • #27253 (httpserver, rest: improving URI validation by pablomartin4btc)
    • #26966 (index: blockfilter initial sync speedup, parallelize process by furszy)

    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.

  3. maflcko commented at 10:44 am on July 8, 2023: member
    I have the same question here: #27861 (review)
  4. in src/rpc/server.cpp:184 in d9453bddd6 outdated 
    180@@ -179,7 +181,8 @@ static RPCHelpMan stop()
181 {
182     // Event loop will exit after current HTTP requests have been handled, so
183     // this reply will get back to the client.
184-    StartShutdown();
185+    auto node_context = CHECK_NONFATAL(util::AnyPtr<node::NodeContext>(jsonRequest.context));
    TheCharlatan commented at 12:45 pm on July 8, 2023:
    Why not use EnsureAnyNodeContext here as done in many other places?
    ryanofsky commented at 8:07 pm on July 8, 2023:

    re: #28051 (review)

    Why not use EnsureAnyNodeContext here as done in many other places?

    Thanks switched to that. I was trying to find the function and couldn’t find it. I just assumed it had been lost in some refactoring.

  5. in src/qt/winshutdownmonitor.cpp:30 in d9453bddd6 outdated 
    26@@ -25,7 +27,7 @@ bool WinShutdownMonitor::nativeEventFilter(const QByteArray &eventType, void *pM
27            {
28                // Initiate a client shutdown after receiving a WM_QUERYENDSESSION and block
29                // Windows session end until we have finished client shutdown.
30-               StartShutdown();
31+               Assert(kernel::g_context)->interrupt();
    TheCharlatan commented at 2:47 pm on July 8, 2023:
    I think we might have introduced a bug here before. This may be possible to trigger when running the GUI before the kernel context is initialized with app.baseInitialize(). We could just move it to after baseInitialize (see my patch here), but I am not sure if that would be correct either. Maybe app.baseInitialize() can be moved up a bit instead?
    ryanofsky commented at 1:28 pm on September 20, 2023:

    re: #28051 (review)

    I think we might have introduced a bug here before. This may be possible to trigger when running the GUI before the kernel context is initialized with app.baseInitialize(). We could just move it to after baseInitialize (see my patch here), but I am not sure if that would be correct either. Maybe app.baseInitialize() can be moved up a bit instead?

    This bug was resolved by moving the signalinterrupt object from the kernel context to the node context and initializing it earlier. Also by abstracting this code and having it call interfaces::Node::startShutdown()

  6. in src/init.cpp:376 in d9453bddd6 outdated 
    373@@ -376,7 +374,7 @@ static void HandleSIGHUP(int)
374 #else
375 static BOOL WINAPI consoleCtrlHandler(DWORD dwCtrlType)
376 {
377-    StartShutdown();
378+    Assert(kernel::g_context)->interrupt();
    TheCharlatan commented at 3:03 pm on July 8, 2023:
    I was looking at the initialization order here and noticed that I might have introduced more unsafe behavior here in e2d680a32d757de0ef8eb836047a0daa1d82e3c4. These handlers are registered in AppInitBasicSetup. But if you look in bitcoind.cpp, the kernel context is created after AppInitBasicSetup is called.
    ryanofsky commented at 1:28 pm on September 20, 2023:

    re: #28051 (review)

    I was looking at the initialization order here and noticed that I might have introduced more unsafe behavior here in e2d680a. These handlers are registered in AppInitBasicSetup. But if you look in bitcoind.cpp, the kernel context is created after AppInitBasicSetup is called.

    Yes this is now fixed by having ctrl-c and signal handler use a g_shutdown global instead of using the kernel context.

  7. TheCharlatan commented at 7:53 pm on July 8, 2023: contributor
    Concept ACK
  8. DrahtBot added the label Needs rebase on Jul 10, 2023
  9. ryanofsky marked this as a draft on Jul 10, 2023
  10. ryanofsky force-pushed on Jul 10, 2023
  11. ryanofsky commented at 11:57 pm on July 10, 2023: contributor

    Making this into a draft because I want to do more work to avoid the uncaught exceptions that Marco mentioned #27861 (review), and address initialization order issues TheCharlatan mentioned #28051 (review) and #28051 (review)

    Updated d9453bddd6ce5446552e844f4d5b65368d831656 -> 24169b10d8df29684eaf6fd261370ccc0a940f30 (pr/noshut.2 -> pr/noshut.3, compare) with EnsureAnyNodeContext suggestion

  12. ryanofsky force-pushed on Jul 13, 2023
  13. DrahtBot removed the label Needs rebase on Jul 13, 2023
  14. DrahtBot added the label Needs rebase on Jul 14, 2023
  15. fanquake commented at 9:41 am on August 7, 2023: member
    Concept ACK
  16. ryanofsky force-pushed on Aug 21, 2023
  17. ryanofsky marked this as ready for review on Aug 21, 2023
  18. ryanofsky commented at 8:29 pm on August 21, 2023: contributor

    Making this into a draft because I want to do more work to avoid the uncaught exceptions that Marco mentioned #27861 (comment), and address initialization order issues TheCharlatan mentioned #28051 (comment) and #28051 (comment)

    Removed draft state and rebased 0dce0042687d2f7151ff9596295ed0b7f84aadd5 -> eccea15a88d0b1ea29847ff6a1f7683459516139 (pr/noshut.4 -> pr/noshut.5, compare) removing all the SignalInterrupt exceptions, fixing initialization order issues that were mentioned, getting rid of the kernel::g_context variable, and rebasing to fix conflicts with #28048 and #28244

  19. DrahtBot added the label CI failed on Aug 21, 2023
  20. DrahtBot removed the label Needs rebase on Aug 21, 2023
  21. fanquake commented at 9:17 am on August 22, 2023: member

    Windows builds are currently unhappy:

     0  CXX      kernel/libbitcoin_node_a-mempool_persist.o
 1init.cpp: In function ‘BOOL consoleCtrlHandler(DWORD)’:
 2init.cpp:386:15: error: ignoring return value of ‘bool util::SignalInterrupt::operator()()’, declared with attribute ‘nodiscard’ [-Werror=unused-result]
 3  386 |     g_shutdown();
 4      |     ~~~~~~~~~~^~
 5In file included from ./kernel/context.h:8,
 6                 from ./node/context.h:8,
 7                 from init.cpp:48:
 8./util/signalinterrupt.h:32:24: note: declared here
 9   32 |     [[nodiscard]] bool operator()();
10      |                        ^~~~~~~~
11cc1plus: all warnings being treated as errors
12make[2]: *** [Makefile:10167: libbitcoin_node_a-init.o] Error 1
  22. ryanofsky force-pushed on Aug 22, 2023
  23. maflcko commented at 9:47 am on August 23, 2023: member
    Does tsan pass locally, if you run the fun tests with it?
  24. ryanofsky commented at 4:20 pm on August 23, 2023: contributor

    Does tsan pass locally, if you run the fun tests with it?

    Tsan seems to output warnings so the test runner fails. I was able to reproduce the CI error from https://cirrus-ci.com/task/5608405029617664 locally by running

    0cd /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu
1test/functional/test_runner.py

    I am a little confused though because even though testrunner fails on the create_cache step, running it manually with

    0test/functional/create_cache.py --cachedir=test/cache --tmpdir=/tmp/cache

    seems to succeed.

    I also found that if I ran src/bitcoind -regtest and src/bitcoin-cli -regtest stop, shutdown would work, but with the tsan warnings, which I’ll try to debug:

      0WARNING: ThreadSanitizer: data race (pid=102057)
  1  Read of size 4 at 0x558be9794bd8 by thread T10:
  2    [#0](/bitcoin-bitcoin/0/) TokenPipeEnd::IsOpen() src/./util/tokenpipe.h:53:28 (bitcoind+0xab59e2) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
  3    [#1](/bitcoin-bitcoin/1/) util::SignalInterrupt::open() src/util/signalinterrupt.cpp:21:19 (bitcoind+0xab59e2)
  4    [#2](/bitcoin-bitcoin/2/) util::SignalInterrupt::operator()() src/util/signalinterrupt.cpp:47:10 (bitcoind+0xab5cfb) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
  5    [#3](/bitcoin-bitcoin/3/) stop()::$_0::operator()(RPCHelpMan const&, JSONRPCRequest const&) const src/rpc/server.cpp:184:10 (bitcoind+0x4ddc70) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
  6    [#4](/bitcoin-bitcoin/4/) decltype(std::declval<stop()::$_0&>()(std::declval<RPCHelpMan const&>(), std::declval<JSONRPCRequest const&>())) std::__1::__invoke[abi:v160000]<stop()::$_0&, RPCHelpMan const&, JSONRPCRequest const&>(stop()::$_0&, RPCHelpMan const&, JSONRPCRequest const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/invoke.h:394:23 (bitcoind+0x4ddc70)
  7    [#5](/bitcoin-bitcoin/5/) UniValue std::__1::__invoke_void_return_wrapper<UniValue, false>::__call<stop()::$_0&, RPCHelpMan const&, JSONRPCRequest const&>(stop()::$_0&, RPCHelpMan const&, JSONRPCRequest const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/invoke.h:478:16 (bitcoind+0x4ddc70)
  8    [#6](/bitcoin-bitcoin/6/) std::__1::__function::__alloc_func<stop()::$_0, std::__1::allocator<stop()::$_0>, UniValue (RPCHelpMan const&, JSONRPCRequest const&)>::operator()[abi:v160000](RPCHelpMan const&, JSONRPCRequest const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:185:16 (bitcoind+0x4ddc70)
  9    [#7](/bitcoin-bitcoin/7/) std::__1::__function::__func<stop()::$_0, std::__1::allocator<stop()::$_0>, UniValue (RPCHelpMan const&, JSONRPCRequest const&)>::operator()(RPCHelpMan const&, JSONRPCRequest const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:356:12 (bitcoind+0x4ddc70)
 10    [#8](/bitcoin-bitcoin/8/) std::__1::__function::__value_func<UniValue (RPCHelpMan const&, JSONRPCRequest const&)>::operator()[abi:v160000](RPCHelpMan const&, JSONRPCRequest const&) const /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:510:16 (bitcoind+0x9ce48f) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 11    [#9](/bitcoin-bitcoin/9/) std::__1::function<UniValue (RPCHelpMan const&, JSONRPCRequest const&)>::operator()(RPCHelpMan const&, JSONRPCRequest const&) const /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:1156:12 (bitcoind+0x9ce48f)
 12    [#10](/bitcoin-bitcoin/10/) RPCHelpMan::HandleRequest(JSONRPCRequest const&) const src/rpc/util.cpp:612:20 (bitcoind+0x9ce48f)
 13    [#11](/bitcoin-bitcoin/11/) CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)::operator()(JSONRPCRequest const&, UniValue&, bool) const src/./rpc/server.h:109:91 (bitcoind+0x3aad0d) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 14    [#12](/bitcoin-bitcoin/12/) decltype(std::declval<CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)&>()(std::declval<JSONRPCRequest const&>(), std::declval<UniValue&>(), std::declval<bool>())) std::__1::__invoke[abi:v160000]<CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)&, JSONRPCRequest const&, UniValue&, bool>(CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)&, JSONRPCRequest const&, UniValue&, bool&&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/invoke.h:394:23 (bitcoind+0x3aabbb) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 15    [#13](/bitcoin-bitcoin/13/) bool std::__1::__invoke_void_return_wrapper<bool, false>::__call<CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)&, JSONRPCRequest const&, UniValue&, bool>(CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)&, JSONRPCRequest const&, UniValue&, bool&&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/invoke.h:478:16 (bitcoind+0x3aabbb)
 16    [#14](/bitcoin-bitcoin/14/) std::__1::__function::__alloc_func<CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool), std::__1::allocator<CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)>, bool (JSONRPCRequest const&, UniValue&, bool)>::operator()[abi:v160000](JSONRPCRequest const&, UniValue&, bool&&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:185:16 (bitcoind+0x3aabbb)
 17    [#15](/bitcoin-bitcoin/15/) std::__1::__function::__func<CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool), std::__1::allocator<CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)>, bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool&&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:356:12 (bitcoind+0x3aabbb)
 18    [#16](/bitcoin-bitcoin/16/) std::__1::__function::__value_func<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()[abi:v160000](JSONRPCRequest const&, UniValue&, bool&&) const /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:510:16 (bitcoind+0x4d91e1) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 19    [#17](/bitcoin-bitcoin/17/) std::__1::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:1156:12 (bitcoind+0x4d91e1)
 20    [#18](/bitcoin-bitcoin/18/) ExecuteCommand(CRPCCommand const&, JSONRPCRequest const&, UniValue&, bool) src/rpc/server.cpp:541:20 (bitcoind+0x4d91e1)
 21    [#19](/bitcoin-bitcoin/19/) ExecuteCommands(std::__1::vector<CRPCCommand const*, std::__1::allocator<CRPCCommand const*>> const&, JSONRPCRequest const&, UniValue&) src/rpc/server.cpp:506:13 (bitcoind+0x4d91e1)
 22    [#20](/bitcoin-bitcoin/20/) CRPCTable::execute(JSONRPCRequest const&) const src/rpc/server.cpp:526:13 (bitcoind+0x4d8dd2) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 23    [#21](/bitcoin-bitcoin/21/) HTTPReq_JSONRPC(std::__1::any const&, HTTPRequest*) src/httprpc.cpp:202:40 (bitcoind+0x6210f4) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 24    [#22](/bitcoin-bitcoin/22/) StartHTTPRPC(std::__1::any const&)::$_0::operator()(HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) const src/httprpc.cpp:301:80 (bitcoind+0x6210f4)
 25    [#23](/bitcoin-bitcoin/23/) decltype(std::declval<StartHTTPRPC(std::__1::any const&)::$_0&>()(std::declval<HTTPRequest*>(), std::declval<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&>())) std::__1::__invoke[abi:v160000]<StartHTTPRPC(std::__1::any const&)::$_0&, HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&>(StartHTTPRPC(std::__1::any const&)::$_0&, HTTPRequest*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/invoke.h:394:23 (bitcoind+0x6210f4)
 26    [#24](/bitcoin-bitcoin/24/) bool std::__1::__invoke_void_return_wrapper<bool, false>::__call<StartHTTPRPC(std::__1::any const&)::$_0&, HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&>(StartHTTPRPC(std::__1::any const&)::$_0&, HTTPRequest*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/invoke.h:478:16 (bitcoind+0x6210f4)
 27    [#25](/bitcoin-bitcoin/25/) std::__1::__function::__alloc_func<StartHTTPRPC(std::__1::any const&)::$_0, std::__1::allocator<StartHTTPRPC(std::__1::any const&)::$_0>, bool (HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)>::operator()[abi:v160000](HTTPRequest*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:185:16 (bitcoind+0x6210f4)
 28    [#26](/bitcoin-bitcoin/26/) std::__1::__function::__func<StartHTTPRPC(std::__1::any const&)::$_0, std::__1::allocator<StartHTTPRPC(std::__1::any const&)::$_0>, bool (HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)>::operator()(HTTPRequest*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:356:12 (bitcoind+0x6210f4)
 29    [#27](/bitcoin-bitcoin/27/) std::__1::__function::__value_func<bool (HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)>::operator()[abi:v160000](HTTPRequest*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) const /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:510:16 (bitcoind+0x632d80) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 30    [#28](/bitcoin-bitcoin/28/) std::__1::function<bool (HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)>::operator()(HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) const /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:1156:12 (bitcoind+0x632d80)
 31    [#29](/bitcoin-bitcoin/29/) HTTPWorkItem::operator()() src/httpserver.cpp:59:9 (bitcoind+0x632d80)
 32    [#30](/bitcoin-bitcoin/30/) WorkQueue<HTTPClosure>::Run() src/httpserver.cpp:114:13 (bitcoind+0x634b37) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 33    [#31](/bitcoin-bitcoin/31/) HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) src/httpserver.cpp:353:12 (bitcoind+0x62b7ea) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 34    [#32](/bitcoin-bitcoin/32/) decltype(std::declval<void (*)(WorkQueue<HTTPClosure>*, int)>()(std::declval<WorkQueue<HTTPClosure>*>(), std::declval<int>())) std::__1::__invoke[abi:v160000]<void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int>(void (*&&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/invoke.h:394:23 (bitcoind+0x6354f7) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 35    [#33](/bitcoin-bitcoin/33/) void std::__1::__thread_execute[abi:v160000]<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct>>, void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct>>, void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int>&, std::__1::__tuple_indices<2ul, 3ul>) /usr/lib/llvm-16/bin/../include/c++/v1/thread:282:5 (bitcoind+0x6354f7)
 36    [#34](/bitcoin-bitcoin/34/) void* std::__1::__thread_proxy[abi:v160000]<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct>>, void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int>>(void*) /usr/lib/llvm-16/bin/../include/c++/v1/thread:293:5 (bitcoind+0x6354f7)
 37
 38  Previous write of size 4 at 0x558be9794bd8 by main thread:
 39    [#0](/bitcoin-bitcoin/0/) TokenPipeEnd::Close() src/util/tokenpipe.cpp:81:10 (bitcoind+0xad379f) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 40    [#1](/bitcoin-bitcoin/1/) TokenPipeEnd::operator=(TokenPipeEnd&&) src/./util/tokenpipe.h:63:9 (bitcoind+0xab5a28) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 41    [#2](/bitcoin-bitcoin/2/) util::SignalInterrupt::open() src/util/signalinterrupt.cpp:24:18 (bitcoind+0xab5a28)
 42    [#3](/bitcoin-bitcoin/3/) util::SignalInterrupt::wait() src/util/signalinterrupt.cpp:69:10 (bitcoind+0xab5c6b) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 43    [#4](/bitcoin-bitcoin/4/) main src/bitcoind.cpp:274:51 (bitcoind+0x16b99d) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 44
 45  Location is global 'g_shutdown' of size 12 at 0x558be9794bd4 (bitcoind+0x25b1bd8)
 46
 47  Thread T10 'b-httpworker.0' (tid=102068, running) created by main thread at:
 48    [#0](/bitcoin-bitcoin/0/) pthread_create <null> (bitcoind+0xe1beb) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 49    [#1](/bitcoin-bitcoin/1/) std::__1::__libcpp_thread_create[abi:v160000](unsigned long*, void* (*)(void*), void*) /usr/lib/llvm-16/bin/../include/c++/v1/__threading_support:378:10 (bitcoind+0x6353ab) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 50    [#2](/bitcoin-bitcoin/2/) std::__1::thread::thread<void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&, void>(void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-16/bin/../include/c++/v1/thread:309:16 (bitcoind+0x6353ab)
 51    [#3](/bitcoin-bitcoin/3/) void std::__1::allocator<std::__1::thread>::construct[abi:v160000]<std::__1::thread, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&>(std::__1::thread*, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-16/bin/../include/c++/v1/__memory/allocator.h:168:28 (bitcoind+0x635126) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 52    [#4](/bitcoin-bitcoin/4/) void std::__1::allocator_traits<std::__1::allocator<std::__1::thread>>::construct[abi:v160000]<std::__1::thread, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&, void>(std::__1::allocator<std::__1::thread>&, std::__1::thread*, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-16/bin/../include/c++/v1/__memory/allocator_traits.h:296:13 (bitcoind+0x635126)
 53    [#5](/bitcoin-bitcoin/5/) void std::__1::vector<std::__1::thread, std::__1::allocator<std::__1::thread>>::__emplace_back_slow_path<void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&>(void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-16/bin/../include/c++/v1/vector:1582:5 (bitcoind+0x635126)
 54    [#6](/bitcoin-bitcoin/6/) std::__1::thread& std::__1::vector<std::__1::thread, std::__1::allocator<std::__1::thread>>::emplace_back<void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&>(void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-16/bin/../include/c++/v1/vector:1603:9 (bitcoind+0x62b2f4) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 55    [#7](/bitcoin-bitcoin/7/) StartHTTPServer() src/httpserver.cpp:443:31 (bitcoind+0x62b2f4)
 56    [#8](/bitcoin-bitcoin/8/) AppInitServers(node::NodeContext&) src/init.cpp:693:5 (bitcoind+0x19138b) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 57    [#9](/bitcoin-bitcoin/9/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1173:14 (bitcoind+0x18697a) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 58    [#10](/bitcoin-bitcoin/10/) AppInit(node::NodeContext&) src/bitcoind.cpp:228:43 (bitcoind+0x16b0ce) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 59    [#11](/bitcoin-bitcoin/11/) main src/bitcoind.cpp:274:10 (bitcoind+0x16b0ce)
 60
 61SUMMARY: ThreadSanitizer: data race src/./util/tokenpipe.h:53:28 in TokenPipeEnd::IsOpen()
 62==================
 63==================
 64WARNING: ThreadSanitizer: data race (pid=102057)
 65  Read of size 8 at 0x7bb000000320 by thread T10:
 66    [#0](/bitcoin-bitcoin/0/) write <null> (bitcoind+0xef7cf) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 67    [#1](/bitcoin-bitcoin/1/) TokenPipeEnd::TokenWrite(unsigned char) src/util/tokenpipe.cpp:43:26 (bitcoind+0xad3846) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 68    [#2](/bitcoin-bitcoin/2/) util::SignalInterrupt::operator()() src/util/signalinterrupt.cpp:58:28 (bitcoind+0xab5d26) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 69    [#3](/bitcoin-bitcoin/3/) stop()::$_0::operator()(RPCHelpMan const&, JSONRPCRequest const&) const src/rpc/server.cpp:184:10 (bitcoind+0x4ddc70) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 70    [#4](/bitcoin-bitcoin/4/) decltype(std::declval<stop()::$_0&>()(std::declval<RPCHelpMan const&>(), std::declval<JSONRPCRequest const&>())) std::__1::__invoke[abi:v160000]<stop()::$_0&, RPCHelpMan const&, JSONRPCRequest const&>(stop()::$_0&, RPCHelpMan const&, JSONRPCRequest const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/invoke.h:394:23 (bitcoind+0x4ddc70)
 71    [#5](/bitcoin-bitcoin/5/) UniValue std::__1::__invoke_void_return_wrapper<UniValue, false>::__call<stop()::$_0&, RPCHelpMan const&, JSONRPCRequest const&>(stop()::$_0&, RPCHelpMan const&, JSONRPCRequest const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/invoke.h:478:16 (bitcoind+0x4ddc70)
 72    [#6](/bitcoin-bitcoin/6/) std::__1::__function::__alloc_func<stop()::$_0, std::__1::allocator<stop()::$_0>, UniValue (RPCHelpMan const&, JSONRPCRequest const&)>::operator()[abi:v160000](RPCHelpMan const&, JSONRPCRequest const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:185:16 (bitcoind+0x4ddc70)
 73    [#7](/bitcoin-bitcoin/7/) std::__1::__function::__func<stop()::$_0, std::__1::allocator<stop()::$_0>, UniValue (RPCHelpMan const&, JSONRPCRequest const&)>::operator()(RPCHelpMan const&, JSONRPCRequest const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:356:12 (bitcoind+0x4ddc70)
 74    [#8](/bitcoin-bitcoin/8/) std::__1::__function::__value_func<UniValue (RPCHelpMan const&, JSONRPCRequest const&)>::operator()[abi:v160000](RPCHelpMan const&, JSONRPCRequest const&) const /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:510:16 (bitcoind+0x9ce48f) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 75    [#9](/bitcoin-bitcoin/9/) std::__1::function<UniValue (RPCHelpMan const&, JSONRPCRequest const&)>::operator()(RPCHelpMan const&, JSONRPCRequest const&) const /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:1156:12 (bitcoind+0x9ce48f)
 76    [#10](/bitcoin-bitcoin/10/) RPCHelpMan::HandleRequest(JSONRPCRequest const&) const src/rpc/util.cpp:612:20 (bitcoind+0x9ce48f)
 77    [#11](/bitcoin-bitcoin/11/) CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)::operator()(JSONRPCRequest const&, UniValue&, bool) const src/./rpc/server.h:109:91 (bitcoind+0x3aad0d) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 78    [#12](/bitcoin-bitcoin/12/) decltype(std::declval<CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)&>()(std::declval<JSONRPCRequest const&>(), std::declval<UniValue&>(), std::declval<bool>())) std::__1::__invoke[abi:v160000]<CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)&, JSONRPCRequest const&, UniValue&, bool>(CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)&, JSONRPCRequest const&, UniValue&, bool&&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/invoke.h:394:23 (bitcoind+0x3aabbb) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 79    [#13](/bitcoin-bitcoin/13/) bool std::__1::__invoke_void_return_wrapper<bool, false>::__call<CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)&, JSONRPCRequest const&, UniValue&, bool>(CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)&, JSONRPCRequest const&, UniValue&, bool&&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/invoke.h:478:16 (bitcoind+0x3aabbb)
 80    [#14](/bitcoin-bitcoin/14/) std::__1::__function::__alloc_func<CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool), std::__1::allocator<CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)>, bool (JSONRPCRequest const&, UniValue&, bool)>::operator()[abi:v160000](JSONRPCRequest const&, UniValue&, bool&&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:185:16 (bitcoind+0x3aabbb)
 81    [#15](/bitcoin-bitcoin/15/) std::__1::__function::__func<CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool), std::__1::allocator<CRPCCommand::CRPCCommand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)>, bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool&&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:356:12 (bitcoind+0x3aabbb)
 82    [#16](/bitcoin-bitcoin/16/) std::__1::__function::__value_func<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()[abi:v160000](JSONRPCRequest const&, UniValue&, bool&&) const /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:510:16 (bitcoind+0x4d91e1) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 83    [#17](/bitcoin-bitcoin/17/) std::__1::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:1156:12 (bitcoind+0x4d91e1)
 84    [#18](/bitcoin-bitcoin/18/) ExecuteCommand(CRPCCommand const&, JSONRPCRequest const&, UniValue&, bool) src/rpc/server.cpp:541:20 (bitcoind+0x4d91e1)
 85    [#19](/bitcoin-bitcoin/19/) ExecuteCommands(std::__1::vector<CRPCCommand const*, std::__1::allocator<CRPCCommand const*>> const&, JSONRPCRequest const&, UniValue&) src/rpc/server.cpp:506:13 (bitcoind+0x4d91e1)
 86    [#20](/bitcoin-bitcoin/20/) CRPCTable::execute(JSONRPCRequest const&) const src/rpc/server.cpp:526:13 (bitcoind+0x4d8dd2) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 87    [#21](/bitcoin-bitcoin/21/) HTTPReq_JSONRPC(std::__1::any const&, HTTPRequest*) src/httprpc.cpp:202:40 (bitcoind+0x6210f4) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 88    [#22](/bitcoin-bitcoin/22/) StartHTTPRPC(std::__1::any const&)::$_0::operator()(HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) const src/httprpc.cpp:301:80 (bitcoind+0x6210f4)
 89    [#23](/bitcoin-bitcoin/23/) decltype(std::declval<StartHTTPRPC(std::__1::any const&)::$_0&>()(std::declval<HTTPRequest*>(), std::declval<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&>())) std::__1::__invoke[abi:v160000]<StartHTTPRPC(std::__1::any const&)::$_0&, HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&>(StartHTTPRPC(std::__1::any const&)::$_0&, HTTPRequest*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/invoke.h:394:23 (bitcoind+0x6210f4)
 90    [#24](/bitcoin-bitcoin/24/) bool std::__1::__invoke_void_return_wrapper<bool, false>::__call<StartHTTPRPC(std::__1::any const&)::$_0&, HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&>(StartHTTPRPC(std::__1::any const&)::$_0&, HTTPRequest*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/invoke.h:478:16 (bitcoind+0x6210f4)
 91    [#25](/bitcoin-bitcoin/25/) std::__1::__function::__alloc_func<StartHTTPRPC(std::__1::any const&)::$_0, std::__1::allocator<StartHTTPRPC(std::__1::any const&)::$_0>, bool (HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)>::operator()[abi:v160000](HTTPRequest*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:185:16 (bitcoind+0x6210f4)
 92    [#26](/bitcoin-bitcoin/26/) std::__1::__function::__func<StartHTTPRPC(std::__1::any const&)::$_0, std::__1::allocator<StartHTTPRPC(std::__1::any const&)::$_0>, bool (HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)>::operator()(HTTPRequest*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:356:12 (bitcoind+0x6210f4)
 93    [#27](/bitcoin-bitcoin/27/) std::__1::__function::__value_func<bool (HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)>::operator()[abi:v160000](HTTPRequest*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) const /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:510:16 (bitcoind+0x632d80) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 94    [#28](/bitcoin-bitcoin/28/) std::__1::function<bool (HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)>::operator()(HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) const /usr/lib/llvm-16/bin/../include/c++/v1/__functional/function.h:1156:12 (bitcoind+0x632d80)
 95    [#29](/bitcoin-bitcoin/29/) HTTPWorkItem::operator()() src/httpserver.cpp:59:9 (bitcoind+0x632d80)
 96    [#30](/bitcoin-bitcoin/30/) WorkQueue<HTTPClosure>::Run() src/httpserver.cpp:114:13 (bitcoind+0x634b37) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 97    [#31](/bitcoin-bitcoin/31/) HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) src/httpserver.cpp:353:12 (bitcoind+0x62b7ea) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 98    [#32](/bitcoin-bitcoin/32/) decltype(std::declval<void (*)(WorkQueue<HTTPClosure>*, int)>()(std::declval<WorkQueue<HTTPClosure>*>(), std::declval<int>())) std::__1::__invoke[abi:v160000]<void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int>(void (*&&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&&) /usr/lib/llvm-16/bin/../include/c++/v1/__functional/invoke.h:394:23 (bitcoind+0x6354f7) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
 99    [#33](/bitcoin-bitcoin/33/) void std::__1::__thread_execute[abi:v160000]<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct>>, void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct>>, void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int>&, std::__1::__tuple_indices<2ul, 3ul>) /usr/lib/llvm-16/bin/../include/c++/v1/thread:282:5 (bitcoind+0x6354f7)
100    [#34](/bitcoin-bitcoin/34/) void* std::__1::__thread_proxy[abi:v160000]<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct>>, void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int>>(void*) /usr/lib/llvm-16/bin/../include/c++/v1/thread:293:5 (bitcoind+0x6354f7)
101
102  Previous write of size 8 at 0x7bb000000320 by main thread:
103    [#0](/bitcoin-bitcoin/0/) pipe2 <null> (bitcoind+0xe7fcd) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
104    [#1](/bitcoin-bitcoin/1/) TokenPipe::Make() src/util/tokenpipe.cpp:88:9 (bitcoind+0xad39d6) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
105    [#2](/bitcoin-bitcoin/2/) util::SignalInterrupt::open() src/util/signalinterrupt.cpp:22:41 (bitcoind+0xab5a08) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
106    [#3](/bitcoin-bitcoin/3/) util::SignalInterrupt::wait() src/util/signalinterrupt.cpp:69:10 (bitcoind+0xab5c6b) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
107    [#4](/bitcoin-bitcoin/4/) main src/bitcoind.cpp:274:51 (bitcoind+0x16b99d) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
108
109  Location is file descriptor 25 created by main thread at:
110    [#0](/bitcoin-bitcoin/0/) pipe2 <null> (bitcoind+0xe7fcd) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
111    [#1](/bitcoin-bitcoin/1/) TokenPipe::Make() src/util/tokenpipe.cpp:88:9 (bitcoind+0xad39d6) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
112    [#2](/bitcoin-bitcoin/2/) util::SignalInterrupt::open() src/util/signalinterrupt.cpp:22:41 (bitcoind+0xab5a08) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
113    [#3](/bitcoin-bitcoin/3/) util::SignalInterrupt::wait() src/util/signalinterrupt.cpp:69:10 (bitcoind+0xab5c6b) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
114    [#4](/bitcoin-bitcoin/4/) main src/bitcoind.cpp:274:51 (bitcoind+0x16b99d) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
115
116  Thread T10 'b-httpworker.0' (tid=102068, running) created by main thread at:
117    [#0](/bitcoin-bitcoin/0/) pthread_create <null> (bitcoind+0xe1beb) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
118    [#1](/bitcoin-bitcoin/1/) std::__1::__libcpp_thread_create[abi:v160000](unsigned long*, void* (*)(void*), void*) /usr/lib/llvm-16/bin/../include/c++/v1/__threading_support:378:10 (bitcoind+0x6353ab) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
119    [#2](/bitcoin-bitcoin/2/) std::__1::thread::thread<void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&, void>(void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-16/bin/../include/c++/v1/thread:309:16 (bitcoind+0x6353ab)
120    [#3](/bitcoin-bitcoin/3/) void std::__1::allocator<std::__1::thread>::construct[abi:v160000]<std::__1::thread, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&>(std::__1::thread*, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-16/bin/../include/c++/v1/__memory/allocator.h:168:28 (bitcoind+0x635126) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
121    [#4](/bitcoin-bitcoin/4/) void std::__1::allocator_traits<std::__1::allocator<std::__1::thread>>::construct[abi:v160000]<std::__1::thread, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&, void>(std::__1::allocator<std::__1::thread>&, std::__1::thread*, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-16/bin/../include/c++/v1/__memory/allocator_traits.h:296:13 (bitcoind+0x635126)
122    [#5](/bitcoin-bitcoin/5/) void std::__1::vector<std::__1::thread, std::__1::allocator<std::__1::thread>>::__emplace_back_slow_path<void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&>(void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-16/bin/../include/c++/v1/vector:1582:5 (bitcoind+0x635126)
123    [#6](/bitcoin-bitcoin/6/) std::__1::thread& std::__1::vector<std::__1::thread, std::__1::allocator<std::__1::thread>>::emplace_back<void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&>(void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-16/bin/../include/c++/v1/vector:1603:9 (bitcoind+0x62b2f4) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
124    [#7](/bitcoin-bitcoin/7/) StartHTTPServer() src/httpserver.cpp:443:31 (bitcoind+0x62b2f4)
125    [#8](/bitcoin-bitcoin/8/) AppInitServers(node::NodeContext&) src/init.cpp:693:5 (bitcoind+0x19138b) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
126    [#9](/bitcoin-bitcoin/9/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1173:14 (bitcoind+0x18697a) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
127    [#10](/bitcoin-bitcoin/10/) AppInit(node::NodeContext&) src/bitcoind.cpp:228:43 (bitcoind+0x16b0ce) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa)
128    [#11](/bitcoin-bitcoin/11/) main src/bitcoind.cpp:274:10 (bitcoind+0x16b0ce)
129
130SUMMARY: ThreadSanitizer: data race (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bitcoind+0xef7cf) (BuildId: 33b1022354bfa28e1e3d045e0355f280a1c0eeaa) in write
131==================
1322023-08-23T16:08:28Z tor: Thread interrupt
1332023-08-23T16:08:28Z torcontrol thread exit
1342023-08-23T16:08:28Z addcon thread exit
1352023-08-23T16:08:28Z opencon thread exit
1362023-08-23T16:08:28Z Shutdown: In progress...
1372023-08-23T16:08:28Z net thread exit
1382023-08-23T16:08:28Z msghand thread exit
1392023-08-23T16:08:28Z DumpAnchors: Flush 0 outbound block-relay-only peer addresses to anchors.dat started
1402023-08-23T16:08:28Z DumpAnchors: Flush 0 outbound block-relay-only peer addresses to anchors.dat completed (0.00s)
1412023-08-23T16:08:28Z scheduler thread exit
1422023-08-23T16:08:28Z Writing 0 unbroadcast transactions to disk.
1432023-08-23T16:08:28Z Dumped mempool: 3.8938e-05s to copy, 0.00434593s to dump
1442023-08-23T16:08:28Z Flushed fee estimates to fee_estimates.dat.
1452023-08-23T16:08:28Z Shutdown: done
146ThreadSanitizer: reported 2 warnings
147root@7b5296fd879e:/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu# src/bitcoind -regtest
  25. ryanofsky force-pushed on Aug 23, 2023
  26. ryanofsky commented at 5:09 pm on August 23, 2023: contributor
    Updated c1ff72ae3c72bba1ed0e5c70364b7832ae6059ef -> 7bfe4a42de7822695da2fb0838a31d0d0d1ffa6d (pr/noshut.6 -> pr/noshut.7, compare) to fix tsan warnings
  27. ryanofsky force-pushed on Aug 23, 2023
  28. DrahtBot removed the label CI failed on Aug 24, 2023
  29. in src/qt/bitcoin.cpp:664 in c7bbca13da outdated 
    629@@ -630,7 +630,7 @@ int GuiMain(int argc, char* argv[])
630     app.installEventFilter(new GUIUtil::LabelOutOfFocusEventFilter(&app));
631 #if defined(Q_OS_WIN)
632     // Install global event filter for processing Windows session related Windows messages (WM_QUERYENDSESSION and WM_ENDSESSION)
633-    qApp->installNativeEventFilter(new WinShutdownMonitor());
    TheCharlatan commented at 8:45 am on August 24, 2023:
    I’m still not sure if this is safe. The passed in node’s shutdown is still a nullptr at this point, because app.baseInitialize() has not been called yet, which would in turn call the required initialization functions. I’ll have to requisition a windows machine to test this out though.
    ryanofsky commented at 2:31 pm on August 24, 2023:

    re: #28051 (review)

    I’m still not sure if this is safe.

    Good catch, It’s not safe. The initialization order was fixed by the rewrite #28051 (comment) but then got broken again by the tsan update: #28051 (comment). Should be fixed again now.

  30. ryanofsky force-pushed on Aug 24, 2023
  31. ryanofsky commented at 2:36 pm on August 24, 2023: contributor
    Updated c7bbca13da3c569358632fa7cc2d2040aa86a06b -> d8db41dfd4fc8e14094900c4bcd019ec46f66312 (pr/noshut.8 -> pr/noshut.9, compare) to fix qt winshutdown initialization order issue that could cause an assert failure if a windows shutdown signal was received before baseInitialize was called. Updated d8db41dfd4fc8e14094900c4bcd019ec46f66312 -> 1fb571973dc5da8d79fafa23e3484541fb12bbf9 (pr/noshut.9 -> pr/noshut.10, compare) with minor cleanups.
  32. ryanofsky force-pushed on Aug 24, 2023
  33. in src/init.cpp:199 in 1fb571973d outdated 
    173@@ -175,6 +174,17 @@ static fs::path GetPidFile(const ArgsManager& args)
174     }
175 }
176 
177+static std::optional<util::SignalInterrupt> g_shutdown;
178+
179+void InitContext(NodeContext& node)
180+{
181+    assert(!g_shutdown);
182+    g_shutdown.emplace();
    TheCharlatan commented at 3:19 pm on August 24, 2023:
    If an exception is thrown during construction here, the program will core dump. How about adding a try-catch block in the various MakeNodeInit that populates the correct exit_status and returns a nullptr?
    ryanofsky commented at 3:56 pm on August 24, 2023:

    re: #28051 (review)

    If an exception is thrown during construction here, the program will core dump. How about adding a try-catch block in the various MakeNodeInit that populates the correct exit_status and returns a nullptr?

    This is not a good place to handle exceptions, because this function is intended to be called by interfaces::Init constructors, and in constructors exceptions are a more natural error handling mechanism than return codes. The best thing to do here is let any exception bubble up, and the best place to catch exceptions would be around the interfaces::MakeNodeInit and interfaces::MakeGuiInit functions (which can already throw bad_alloc) which call this.

    Exceptions are not currently caught around the interfaces::MakeNodeInit and interfaces::MakeGuiInit calls and I do not think it would make sense to change that now. These calls are the very first calls that get made in the bitcoind and bitcoin-qt main() functions before anything else happens: before any threads are created, before any libraries are initialized, even before any command line arguments are parsed. So it should be impossible for these functions to fail unless the operating environment is unusable, and if they do fail there is no cleanup or logging that could be done, and it would be doubtful if even printing would work. So there would only by drawbacks trying to intercept the failure there as well.

    Basically this is a long way of saying if core dumps are enabled by the OS, it would be safe and appropriate to core dump here.

  34. in src/node/blockstorage.cpp:13 in 1fb571973d outdated 
     9@@ -10,6 +10,7 @@
10 #include <flatfile.h>
11 #include <hash.h>
12 #include <kernel/chainparams.h>
13+#include <kernel/context.h>
    TheCharlatan commented at 8:17 pm on August 24, 2023:
    This must have snuck in during one of the revisions.
    ryanofsky commented at 10:20 pm on August 24, 2023:

    re: #28051 (review)

    This must have snuck in during one of the revisions.

    Thanks, reverted

  35. in src/node/chainstate.cpp:59 in 1fb571973d outdated 
    55@@ -55,14 +56,14 @@ static ChainstateLoadResult CompleteChainstateInitialization(
56         }
57     }
58 
59-    if (options.check_interrupt && options.check_interrupt()) return {ChainstateLoadStatus::INTERRUPTED, {}};
60+    if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}};
    TheCharlatan commented at 8:19 pm on August 24, 2023:
    MIght this be a regression in case of the chainman reference being eventually removed from the chainstate?
    ryanofsky commented at 10:23 pm on August 24, 2023:

    re: #28051 (review)

    MIght this be a regression in case of the chainman reference being eventually removed from the chainstate?

    Is this referencing #28218 (review)? If so, I don’t think I understand the connection because there isn’t a Chainstate object here.

    TheCharlatan commented at 5:29 am on August 25, 2023:
    Yeah, right. I don’t think this applies here either now. Please resolve.
  36. in src/validation.cpp:24 in 1fb571973d outdated 
    20@@ -21,6 +21,7 @@
21 #include <flatfile.h>
22 #include <hash.h>
23 #include <kernel/chainparams.h>
24+#include <kernel/context.h>
    TheCharlatan commented at 8:32 pm on August 24, 2023:
    This also seems to come from a previous revision.
    ryanofsky commented at 10:24 pm on August 24, 2023:

    re: #28051 (review)

    This also seems to come from a previous revision.

    Thanks, reverted

  37. TheCharlatan commented at 8:51 pm on August 24, 2023: contributor

    ACK 1fb571973dc5da8d79fafa23e3484541fb12bbf9

    Thank you for fixing all of this. I think moving the interrupt out of the kernel context again is the right move. If it becomes annoying somewhere down the road we can always think about how to move it back again.

  38. ryanofsky force-pushed on Aug 24, 2023
  39. ryanofsky commented at 10:32 pm on August 24, 2023: contributor
    Updated 1fb571973dc5da8d79fafa23e3484541fb12bbf9 -> dcadcfcc81078465aedcaf480668efca81c5bbcc (pr/noshut.10 -> pr/noshut.11, compare) making suggested changes
  40. TheCharlatan approved
  41. TheCharlatan commented at 6:25 am on August 25, 2023: contributor
    re-ACK dcadcfcc81078465aedcaf480668efca81c5bbcc
  42. DrahtBot added the label Needs rebase on Sep 5, 2023
  43. ryanofsky force-pushed on Sep 7, 2023
  44. ryanofsky commented at 6:21 pm on September 7, 2023: contributor

    Split this into commits so it is easier to review, also rebased due to conflict

    Rebased dcadcfcc81078465aedcaf480668efca81c5bbcc -> 860911a72a771bce72cdee88726b37bd37495c49 (pr/noshut.11 -> pr/noshut.12, compare) due to conflict with #28195

  45. DrahtBot removed the label Needs rebase on Sep 7, 2023
  46. TheCharlatan commented at 7:11 pm on September 7, 2023: contributor

    re-ACK 860911a72a771bce72cdee88726b37bd37495c49

    Only net change is the split into separate commits.

  47. ryanofsky force-pushed on Sep 11, 2023
  48. ryanofsky commented at 2:45 pm on September 11, 2023: contributor
    Updated 860911a72a771bce72cdee88726b37bd37495c49 -> b34e064c2c4ec343dac63ca3b985155ca00ed535 (pr/noshut.12 -> pr/noshut.13, compare) fixing bug in intermediate commit (no change overall)
  49. DrahtBot added the label CI failed on Sep 15, 2023
  50. ryanofsky force-pushed on Sep 20, 2023
  51. ryanofsky commented at 11:15 pm on September 20, 2023: contributor
    Rebased b34e064c2c4ec343dac63ca3b985155ca00ed535 -> 19e9781364891c45941007c086211d5659334b39 (pr/noshut.13 -> pr/noshut.14, compare) to fix silent conflict with #27850: https://cirrus-ci.com/task/5572474910277632?logs=ci#L2460 Rebased 19e9781364891c45941007c086211d5659334b39 -> dde5736c10d663958f4b6fba3ed7214a2b806ffa (pr/noshut.14 -> pr/noshut.15, compare) due to minor conflict with #28551
  52. DrahtBot removed the label CI failed on Sep 21, 2023
  53. DrahtBot added the label Needs rebase on Oct 4, 2023
  54. ryanofsky force-pushed on Oct 4, 2023
  55. DrahtBot removed the label Needs rebase on Oct 4, 2023
  56. DrahtBot added the label CI failed on Oct 16, 2023
  57. maflcko commented at 4:11 pm on October 16, 2023: member
    Needs rebase if still relevant
  58. ryanofsky force-pushed on Oct 18, 2023
  59. ryanofsky commented at 6:27 pm on October 18, 2023: contributor
    Rebased dde5736c10d663958f4b6fba3ed7214a2b806ffa -> d7477b1c9cde1e795e006d1788c79b8985970eee (pr/noshut.15 -> pr/noshut.16, compare) due to silent conflict with #26331
  60. in src/qt/winshutdownmonitor.h:18 in d7477b1c9c outdated 
    12@@ -13,14 +13,22 @@
13 
14 #include <QAbstractNativeEventFilter>
15 
16+namespace interfaces {
17+class Node;
18+} // namespaces interfaces
    TheCharlatan commented at 6:58 pm on October 18, 2023:
    NIt: s/namespaces/namespace/
    ryanofsky commented at 1:12 pm on November 9, 2023:

    re: #28051 (review)

    NIt: s/namespaces/namespace/

    Thanks, fixed

  61. DrahtBot removed the label CI failed on Oct 18, 2023
  62. TheCharlatan approved
  63. TheCharlatan commented at 8:34 pm on October 18, 2023: contributor
    Re-ACK d7477b1c9cde1e795e006d1788c79b8985970eee
  64. DrahtBot requested review from fanquake on Oct 18, 2023
  65. in src/qt/winshutdownmonitor.cpp:28 in dc7be790fa outdated 
    24@@ -25,7 +25,7 @@ bool WinShutdownMonitor::nativeEventFilter(const QByteArray &eventType, void *pM
25            {
26                // Initiate a client shutdown after receiving a WM_QUERYENDSESSION and block
27                // Windows session end until we have finished client shutdown.
28-               StartShutdown();
29+               m_node.startShutdown();
    stickies-v commented at 2:26 pm on October 31, 2023:
    note for other reviewers (and future self): NodeImpl::startShutDown() does have this additional code block, but the InterruptRPC() and StopRPC() calls are executed only once regardless and I think the ordering remains the same too, so I don’t see (but am not at all familiar with the GUI code) how this could cause issues
    ryanofsky commented at 1:12 pm on November 9, 2023:

    re: #28051 (review)

    note for other reviewers (and future self): NodeImpl::startShutDown() does have this additional code block

    There is a change in behavior here but the new behavior should be more correct. RPC calls should be interrupted whenever the GUI is shut down, regardless of how it is shut down. Calls to StartShutdown in GUI code were replaced with calls to interfaces::Node::startShutdown a long time ago, and I think this call was just missed because the replacement was done by searching for gui->node link symbol dependencies in a non-windows build, and this file is windows-only.

  66. in src/node/chainstate.cpp:66 in d7477b1c9c outdated 
    63     // block file from disk.
64     // Note that it also sets fReindex global based on the disk flag!
65     // From here on, fReindex and options.reindex values may be different!
66     if (!chainman.LoadBlockIndex()) {
67-        if (options.check_interrupt && options.check_interrupt()) return {ChainstateLoadStatus::INTERRUPTED, {}};
68+        if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}};
    stickies-v commented at 2:31 pm on October 31, 2023:
    This seems a bit brittle and probably better handled by having LoadBlockIndex() return e.g. a util::Result (especially after #25665), but orthogonal to this refactor so I think it’s best to leave as is here.
  67. in src/test/util/setup_common.cpp:182 in d7477b1c9c outdated 
    167@@ -168,7 +168,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto
168 
169     m_cache_sizes = CalculateCacheSizes(m_args);
170 
171-    m_node.notifications = std::make_unique<KernelNotifications>(m_node.exit_status);
172+    m_node.notifications = std::make_unique<KernelNotifications>(*Assert(m_node.shutdown), m_node.exit_status);
    stickies-v commented at 3:08 pm on October 31, 2023:
    nit: missing #include <util/check.h>
    ryanofsky commented at 1:13 pm on November 9, 2023:

    re: #28051 (review)

    nit: missing #include <util/check.h>

    Thanks, fixed

  68. in src/httpserver.cpp:583 in d7477b1c9c outdated 
    579@@ -580,7 +580,7 @@ void HTTPEvent::trigger(struct timeval* tv)
580     else
581         evtimer_add(ev, tv); // trigger after timeval passed
582 }
583-HTTPRequest::HTTPRequest(struct evhttp_request* _req, bool _replySent) : req(_req), replySent(_replySent)
584+HTTPRequest::HTTPRequest(struct evhttp_request* _req, const util::SignalInterrupt& interrupt, bool _replySent) : req(_req), m_interrupt(interrupt), replySent(_replySent)
    stickies-v commented at 3:28 pm on October 31, 2023:
    nit: long line
    ryanofsky commented at 1:12 pm on November 9, 2023:

    re: #28051 (review)

    nit: long line

    Thanks, added line break

  69. in src/rpc/server.cpp:186 in d7477b1c9c outdated 
    180@@ -179,7 +181,9 @@ static RPCHelpMan stop()
181 {
182     // Event loop will exit after current HTTP requests have been handled, so
183     // this reply will get back to the client.
184-    StartShutdown();
185+    if (!(*Assert(EnsureAnyNodeContext(jsonRequest.context).shutdown))()) {
186+        throw JSONRPCError(RPC_MISC_ERROR, "Failed to send shutdown signal");
187+    }
    stickies-v commented at 10:05 pm on November 1, 2023:

    I don’t think we need to expose those internal error details. Also, I know we exclude server.cpp from the CHECK_NONFATAL linter, but given that this is an RPC method I think it would be preferable here?

    0    CHECK_NONFATAL((*CHECK_NONFATAL(EnsureAnyNodeContext(jsonRequest.context).shutdown))());
    ryanofsky commented at 1:13 pm on November 9, 2023:

    re: #28051 (review)

    I don’t think we need to expose those internal error details. Also, I know we exclude server.cpp from the CHECK_NONFATAL linter, but given that this is an RPC method I think it would be preferable here?

    Thanks, replaced JSONRPCError exception with nonfatal exception

  70. stickies-v commented at 10:10 pm on November 1, 2023: contributor

    Approach ACK. It’s a fairly extensive PR to review, and the changes don’t always help with verbosity (e.g. (!(*Assert(Assert(m_context)->shutdown))()) instead of StartShutdown), but it seems like the right direction to go regardless.

    I’ve gone through the code multiple times now and it looks good, so I don’t think I’ll come up with (m)any more comments, but I’m not fully comfortable yet with all the implications of the changes so I’ll have to spend some more time on it before signing off. Will try to do that in the next few days.

  71. ryanofsky force-pushed on Nov 9, 2023
  72. ryanofsky commented at 1:50 pm on November 9, 2023: contributor

    Thanks for the reviews.

    Updated d7477b1c9cde1e795e006d1788c79b8985970eee -> 52ecb6547dd2b59643483e43531d1201941c0df0 (pr/noshut.16 -> pr/noshut.17, compare) with suggested changes Rebased 52ecb6547dd2b59643483e43531d1201941c0df0 -> 437a0c2aa1f0c6b55bdc4a2d91d2b7f0b1cb8e69 (pr/noshut.17 -> pr/noshut.18, compare) due to conflict with #28758 caused by new change

  73. ryanofsky force-pushed on Nov 9, 2023
  74. in src/node/chainstate.cpp:20 in 437a0c2aa1 outdated 
    16@@ -17,6 +17,7 @@
17 #include <txdb.h>
18 #include <uint256.h>
19 #include <util/fs.h>
20+#include <util/signalinterrupt.h>
    stickies-v commented at 3:37 pm on November 9, 2023:
    nit: I think this needs to be included in validation.h instead of here since ChainstateManager::m_interrupt is a const util::SignalInterrupt&
    ryanofsky commented at 11:40 pm on November 27, 2023:

    re: #28051 (review)

    nit: I think this needs to be included in validation.h instead of here since ChainstateManager::m_interrupt is a const util::SignalInterrupt&

    In commit “refactor: Remove call to ShutdownRequested from chainstate init” (970fba3988036e0eb8dbccdabd8578a3759eb013)

    I think this is doing the right thing currently. validation.h has a forward declaration, which works because the class member is never used in validation.h and is a reference, so the layout of the ChainstateManager class does not depend on the SignalInterrupt definition. And the #include is needed in this file (according to IWYU rules) because code in this file is calling the operator bool method defined in the signalinterrupt.h header.

    stickies-v commented at 8:47 am on December 5, 2023:
    You’re right, I didn’t think about the operator bool. Thanks.
  75. in src/test/util/index.cpp:12 in 437a0c2aa1 outdated 
     9 #include <util/check.h>
10+#include <util/signalinterrupt.h>
11 #include <util/time.h>
12 
13-void IndexWaitSynced(const BaseIndex& index)
14+void IndexWaitSynced(const BaseIndex& index, const util::SignalInterrupt& interrupt)
    stickies-v commented at 4:27 pm on November 9, 2023:

    nit: would ~interrupt~ -> shutdown be more appropriate and consistent name?

    (same for node::AbortNode())

    ryanofsky commented at 11:53 pm on November 27, 2023:

    re: #28051 (review)

    In commit “refactor: Remove call to ShutdownRequested from IndexWaitSynced” (ca6c67229f851303c204616a64de4886d2a754a8)

    nit: would ~interrupt~ -> shutdown be more appropriate and consistent name?

    (same for node::AbortNode())

    I don’t think it would be more appropriate. shutdown is the name being used in high-level code managing the node state and sending the interrupt signal (for example in the NodeContext::shutdown variable). The name shutdown describes the purpose of the signal. But in lower level code receiving the signal the signal is just called interrupt because the only thing this code needs to do is stop processing and return when the signal is received. It should not know why the signal is being sent.

    For example in indexing code right now the interrupt signal is only sent when the node is shut down, but in the future indexes could be enabled, interrupted, and disabled at runtime by RPC or GUI commands without requiring a shutdown. In validation code, the interrupt signal is currently sent when the node is shuttting down, but in the future, the signal could be sent when there is no node at all and the signal is just being sent by a user of the libbitcoinkernel library to stop an operation.

    So the variable used to request shutdowns is called shutdown and the variables used to receive interruptions are called interrupt because the variables are being used for different things. The variables could actually have different types, but since SignalInterrupt is modeled off of CThreadInterrupt just one type is used.

    stickies-v commented at 8:46 am on December 5, 2023:
    That makes sense, and I agree with your rationale, so I think my suggestion here in test/util/index.cpp indeed should be disregarded. In node::AbortNode() though, we are sending an interrupt signal to shutdown the node, so I still think it would make sense there? (non-blocking nit, ofc)
    ryanofsky commented at 8:18 pm on December 11, 2023:

    re: #28051 (review)

    In node::AbortNode() though, we are sending an interrupt signal to shutdown the node, so I still think it would make sense there? (non-blocking nit, ofc)

    Sorry I missed this comment before, and you are right about AbortNode. I renamed interrupt to shutdown there now and in the KernelNotifications class as well.

  76. in src/init.cpp:412 in 437a0c2aa1 outdated 
    392@@ -378,7 +393,10 @@ static void HandleSIGHUP(int)
393 #else
394 static BOOL WINAPI consoleCtrlHandler(DWORD dwCtrlType)
395 {
396-    StartShutdown();
397+    if (!(*Assert(g_shutdown))()) {
398+        LogPrintf("Error: failed to send shutdown signal on Ctrl-C\n");
399+        return false;
400+    }
    stickies-v commented at 5:24 pm on November 9, 2023:
    nit: on Windows, I don’t think the failure code path is ever reached, since SignalInterrupt::operator()() always returns true for #ifdef WIN32. But perhaps that would be overly relying on the implementation of SignalInterrupt.
    ryanofsky commented at 11:42 pm on November 9, 2023:

    re: #28051 (review)

    nit: on Windows, I don’t think the failure code path is ever reached, since SignalInterrupt::operator()() always returns true for #ifdef WIN32. But perhaps that would be overly relying on the implementation of SignalInterrupt.

    Yes, that is the reasoning. There is an interface for sending the signal that returns success or failure, and I want this code to just call the interface without making assumptions about how the signal is sent. The way the signal is sent could change in the future if #10102 progresses and code running in separate processes. If this log print is a problem I can replace it with an Assert. But I think just making the call and logging an error when the call fails is the most straightforward approach.

    stickies-v commented at 8:48 am on December 5, 2023:

    But I think just making the call and logging an error when the call fails is the most straightforward approach.

    Agreed, can be marked as resolved.

  77. in src/init.cpp:1167 in 437a0c2aa1 outdated 
    1164         if (!CheckDiskSpace(args.GetBlocksDirPath(), min_disk_space)) {
1165             LogPrintf("Shutting down due to lack of disk space!\n");
1166-            StartShutdown();
1167+            if (!(*Assert(node.shutdown))()) {
1168+                LogPrintf("Error: failed to send shutdown signal after disk space check\n");
1169+            }
    stickies-v commented at 6:14 pm on November 9, 2023:

    This interface of dealing with shutdown failures feels a bit awkward to me. It’s verbose and quite difficult to parse. These shutdown failures aren’t really related to the callsite, and it looks like mostly we just log and then ignore them anyway.

    What do you think about adding failure callbacks to SignalInterrupt? It still allows the caller to individually handle shutdown failures when it makes sense (still returning bools), but in most cases the generic callback will probably suffice?

    https://github.com/bitcoin/bitcoin/compare/437a0c2aa1f0c6b55bdc4a2d91d2b7f0b1cb8e69...stickies-v:bitcoin:pr/28051-use-cbs

    I think the first commit can be used ~as is, the second would probably have to be split across multiple existing commits. I think this PR could be an appropriate place to implement it, but otherwise (if you think the approach makes sense) I’m also happy to open a follow-up pull.

    ryanofsky commented at 0:04 am on November 10, 2023:

    re: #28051 (review)

    This interface of dealing with shutdown failures feels a bit awkward to me. It’s verbose and quite difficult to parse. These shutdown failures aren’t really related to the callsite, and it looks like mostly we just log and then ignore them anyway.

    This is intentional. The problem is that we have low-level code scattered in the codebase that is able to send shutdown signals, and this is not a good design. Lower level code should just be returning errors and notifications, and high level code should be using that information to decide whether to shut down, not reacting to shutdowns that could be triggered anywhere. The fact that bad code sending shutdown signals is more verbose now does not seem a problem, since code like this shoudl be discouraged and cleaned up anyway. This PR takes a step in that direction by removing the global StartShutdown function and discouraging more code like this from being written in the future.

    Another thing this PR is trying to do is add appropriate error handling when sending shutdown signals fails, instead of crashing or ignoring the failures. The right way to handle these errors depends on where the errors happen. If an error happens in the stop RPC, an error should be returned to the RPC caller. If the error happens in a windows ctrl-c handler, the handler should log the failure and return false. If the error happens in a unix signal handler, the only thing that can be done without triggering undefined behavior is to ignore it. The problem with the suggested use-cbs branch is that it doesn’t treat different errors differently. I think also it complicates control flow without a good justification. It is true that [[nodiscard]] bool error handling is a little verbose, but it’s also more straightforward than std::function callbacks. And the verbosity can go away if more code changed to send status information instead of shutdown signals, which is something that should happen anyway.

    stickies-v commented at 9:09 am on December 5, 2023:

    Lower level code should just be returning errors and notifications, and high level code should be using that information to decide whether to shut down

    That does seem like a better design, indeed. I agree then that having the bad approach be more verbose is not something to worry about, and my suggestion should be discarded. Thank you for providing the rationale, I think this might be useful to add to the PR description?

    ryanofsky commented at 8:17 pm on December 11, 2023:

    re: #28051 (review)

    Thank you for providing the rationale, I think this might be useful to add to the PR description?

    Definitely, and the PR description was pretty bad so I rewrote it. This is mentioned now at the end.

  78. ryanofsky commented at 9:51 pm on November 28, 2023: contributor
    Thanks for reviewing and sorry my answers to your questions are so long. I hope they make sense!
  79. refactor: Remove call to ShutdownRequested from chainstate init 
    Use chainman.m_interrupt object instead

There is no change in behavior in this commit
    263b23f008
  80. refactor: Remove call to ShutdownRequested from rpc/mining 
    Use chainman.m_interrupt object instead

There is no change in behavior in this commit
    f0c73c1336
  81. refactor: Remove call to StartShutdown from qt 
    Use interfaces::Node object instead.

There is a minor change in behavior in this commit, because the new code calls
InterruptRPC() and StopRPC() when previous code did not do this.  But this
should be a good thing since it makes sense to interrupt RPC when the system is
shutting down, and it is better for the GUI shut down in a consistent way
regardless of how the shutdown is triggered.
    f4a8bd6e2f
  82. refactor: Add NodeContext::shutdown member 
    Add NodeContext::shutdown variable and start using it to replace the
kernel::Context::interrupt variable. The latter can't easily be removed right
away but will be removed later in this PR.

Moving the interrupt object from the kernel context to the node context
increases flexibility of the kernel API so it is possible to use multiple
interrupt objects, or avoid creating one if one is not needed. It will also
allow getting rid of the kernel::g_context global later in this PR, replacing
it with a private SignalInterrupt instance in init.cpp

There is no change in behavior in this commit outside of unit tests. In unit
tests there should be no visible change either, but internally now each test
has its own interrupt variable so the variable will be automatically reset
between tests.
    73133c36aa
  83. refactor: Remove call to ShutdownRequested from HTTPRequest 
    Pass HTTP server an interrupt object instead of having it depend on shutdown.h
and global shutdown state.

There is no change in behavior in this commit.
    42e5829d97
  84. refactor: Remove call to ShutdownRequested from IndexWaitSynced 
    Use the node interrupt object instead.

There is no change in behavior in this commit.
    ba93966368
  85. util: Get rid of uncaught exceptions thrown by SignalInterrupt class 
    Replace exceptions thrown by signal and wait methods with [[nodiscard]] return
values.

This is mostly a refactoring, but there is a slight change of behavior if
AbortShutdown function fails. The original behavior which was unintentionally
changed in #27861 is restored, so it now triggers an assert failure again
instead of throwing an exception. (The AbortShutdown function is only ever
called in the the GUI version of Bitcoin Core when corruption is detected on
loading and the user tries to reindex.)

Problems with using exceptions were pointed out by MarcoFalke in
https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1255496707.
    1d92d89edb
  86. refactor: Remove call to StartShutdown from stop RPC 
    Use SignalInterrupt object instead. There is a slight change in behavior here
because the previous StartShutdown code used to abort on failure and the
new code returns an RPC error instead.
    6824eecaf1
  87. refactor: Remove calls to StartShutdown from KernelNotifications 
    Use SignalInterrupt object instead. There is a slight change in behavior here
because the previous StartShutdown code used to abort on failure and the
new code logs errors instead.
    feeb7b816a
  88. refactor: Add InitContext function to initialize NodeContext with global pointers 
    Having InitContext() avoids the need to add duplicate code to src/init/*.cpp
files in the next commit. It also lets these files avoid referencing global
variables like gArgs.

There is no change in behavior in this commit.
    213542b625
  89. Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly 
    This change is mostly a refectoring that removes some code and gets rid of an
unnecessary layer of indirection after #27861

But it is not a pure refactoring since StartShutdown, AbortShutdown, and
WaitForShutdown functions used to abort on failure, and the replacement code
logs or returns errors instead.
    6db04be102
  90. DrahtBot added the label Needs rebase on Dec 4, 2023
  91. ryanofsky force-pushed on Dec 4, 2023
  92. ryanofsky commented at 8:34 pm on December 4, 2023: contributor
    Rebased 437a0c2aa1f0c6b55bdc4a2d91d2b7f0b1cb8e69 -> 8a02ce59ffa16c73611aeda6caf8b54d85c6208f (pr/noshut.18 -> pr/noshut.19, compare) due to conflict with #28946
  93. DrahtBot removed the label Needs rebase on Dec 4, 2023
  94. stickies-v approved
  95. stickies-v commented at 9:12 am on December 5, 2023: contributor

    ACK 8a02ce59ffa16c73611aeda6caf8b54d85c6208f

    sorry my answers to your questions are so long

    I really appreciate the time you’ve taken to thoroughly address my comments. Thank you, it’s extremely helpful.

  96. DrahtBot requested review from TheCharlatan on Dec 5, 2023
  97. in src/qt/bitcoin.cpp:664 in 8a02ce59ff outdated 
    660@@ -661,7 +661,7 @@ int GuiMain(int argc, char* argv[])
661     app.installEventFilter(new GUIUtil::LabelOutOfFocusEventFilter(&app));
662 #if defined(Q_OS_WIN)
663     // Install global event filter for processing Windows session related Windows messages (WM_QUERYENDSESSION and WM_ENDSESSION)
664-    qApp->installNativeEventFilter(new WinShutdownMonitor());
665+    qApp->installNativeEventFilter(new WinShutdownMonitor(app.node()));
    TheCharlatan commented at 11:49 am on December 6, 2023:

    Calling app.node() here before app.createNode results in a crash on windows when running bitcoin-qt.exe. I think the most straight forward way to resolve this would be to move this block of code to after createNode is called.

    Alternatively, to keep the current control flow, the way I read it, make_node does not actually manipulate the node context, besides eventually setting it in the NodeImpl further down the call stack. So I think the call to it could be moved further up:

     0diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
 1index 8d4ca4ea92..a195db550f 100644
 2--- a/src/qt/bitcoin.cpp
 3+++ b/src/qt/bitcoin.cpp
 4@@ -301,0 +302,2 @@ void BitcoinApplication::createSplashScreen(const NetworkStyle *networkStyle)
 5+    assert(m_node);
 6+    m_splash->setNode(*m_node);
 7@@ -658,0 +661 @@ int GuiMain(int argc, char* argv[])
 8+    app.createNode(*init);
 9@@ -675,2 +677,0 @@ int GuiMain(int argc, char* argv[])
10-    app.createNode(*init);

    Would be nice if we had tests that would exercise this code. Maybe a quick start and kill routine can be added to the windows native CI job?

    ryanofsky commented at 4:00 pm on December 6, 2023:

    re: #28051 (review)

    Calling app.node() here before app.createNode results in a crash on windows when running bitcoin-qt.exe.

    Great catch! This should be fixed now by passing a shutdown callback and delaying the call to app.node(). I didn’t want to move the createNode call, because the interfaces::Node object is intentionally created after the splash screen, so the splash screen can show up without a delay. The delay wouldn’t be significant now, but with #10102 creating the node requires an IPC round trip, so might be noticeable.

    Also agree with you it’s a shame this part of the Qt codebase doesn’t have CI test coverage. Your suggestion to add a CI step starting and stopping bitcoin-qt on windows (and probably all platforms bitcoin-qt is built for) would be a nice thing to implement. I think it would be most easily implemented as a python test.

    Other ways to get more test coverage for this code might be to change one of the existing CI jobs to run existing python tests with bitcoin-qt instead of bitcoind. Or to refactor the GuiMain function and write unit tests for it. But unlike your suggestion, these approaches would be unlikely to trigger this particular bug since it is windows-specific.

    TheCharlatan commented at 10:11 pm on December 6, 2023:
    Thanks for the rationale, I indeed did not think of the future IPC delay. If we only start processing events after app.exec(), why not move this to just before registerShutdownBlockReason? Looking at the history it looks to me like this was put where it is to be in the general vicinity of the installEvent* calls.
    ryanofsky commented at 11:00 pm on December 6, 2023:

    Thanks for the rationale, I indeed did not think of the future IPC delay. If we only start processing events after app.exec(), why not move this to just before registerShutdownBlockReason? Looking at the history it looks to me like this was put where it is to be in the general vicinity of the installEvent* calls.

    I can make this change if you still think it’s a good idea after this, but I think it’s reasonable to want to keep the installEvent calls together. I also didn’t want to just move the install event call when it’s not necessary.

    I think the current approach of passing a lambda argument to shutdownmonitor instead of passing a interfaces::Node pointer like the previous approach is better in any case, because it makes it obvious that the only thing shutdownmonitor can do is call the startShutdown function, and that it is not going to interact with the node in some other way. It also makes the shutdown monitor class more future-proof so it the WM_QUERYENDSESSION / WM_ENDSESSION code shouldn’t need to change when there are changes to the node interface.

  98. in src/qt/winshutdownmonitor.h:31 in 8a02ce59ff outdated 
    26     bool nativeEventFilter(const QByteArray &eventType, void *pMessage, long *pnResult) override;
27 
28     /** Register the reason for blocking shutdown on Windows to allow clean client exit */
29     static void registerShutdownBlockReason(const QString& strReason, const HWND& mainWinId);
30+
31+    interfaces::Node& m_node;
    TheCharlatan commented at 11:49 am on December 6, 2023:
    Nit: Could this be private?
    ryanofsky commented at 6:42 pm on December 6, 2023:

    re: #28051 (review)

    Nit: Could this be private?

    Sure, switched to a private member in the latest push

  99. DrahtBot requested review from TheCharlatan on Dec 6, 2023
  100. ryanofsky force-pushed on Dec 6, 2023
  101. ryanofsky commented at 6:45 pm on December 6, 2023: contributor
    Updated 8a02ce59ffa16c73611aeda6caf8b54d85c6208f -> 4a3a2651420ca1808cb25aed8a33d57dfcd627f5 (pr/noshut.19 -> pr/noshut.20, compare) with Qt windows bugfix
  102. TheCharlatan approved
  103. TheCharlatan commented at 10:36 am on December 7, 2023: contributor
    Re-ACK 4a3a2651420ca1808cb25aed8a33d57dfcd627f5
  104. DrahtBot requested review from stickies-v on Dec 7, 2023
  105. ryanofsky commented at 9:38 pm on December 11, 2023: contributor
    Updated 4a3a2651420ca1808cb25aed8a33d57dfcd627f5 -> eaf915d61d470372e63f41f11d6a873c1936f73f (pr/noshut.20 -> pr/noshut.22, compare) just implementing a suggested change to rename
  106. ryanofsky force-pushed on Dec 11, 2023
  107. TheCharlatan approved
  108. TheCharlatan commented at 10:13 pm on December 11, 2023: contributor
    Re-ACK eaf915d61d470372e63f41f11d6a873c1936f73f
  109. in src/node/abort.cpp:26 in ae6642973d outdated 
    23     LogPrintf("*** %s\n", debug_message);
24     InitError(user_message.empty() ? _("A fatal internal error occurred, see debug.log for details") : user_message);
25     exit_status.store(EXIT_FAILURE);
26-    if (shutdown) StartShutdown();
27+    if (shutdown && !(*shutdown)()) {
28+        LogPrintf("Error: failed to send shutdown signal\n");
    maflcko commented at 9:48 am on December 12, 2023:
    ae6642973dead188514aceec30ae5d73404ebcdf: Is it required to change the behavior here? Why not keep the abort?
    ryanofsky commented at 1:40 pm on December 12, 2023:

    re: #28051 (review)

    ae66429: Is it required to change the behavior here? Why not keep the abort?

    In general, I don’t think dumping core is a good way to handle errors. I think it can be a reasonable thing to do if inconsistent internal state is detected or if a precondition is violated, and halting right away will provide better debug information. Or it could make sense if it’s likely that data corruption or other bad outcomes will happen if execution doesn’t stop as quickly as possible.

    But in this case when an ordinary system call that is not saving data returns an error code, I think it it is better to log the failure and move on than to crash the entire process abruptly when we don’t know what other threads are running, what wallets might be loaded, what data might be lost, etc, as a result of an abort().

    maflcko commented at 8:44 pm on December 12, 2023:

    I guess this code will never be hit, so it doesn’t matter much. If it was hit, I don’t see how the user would shutdown the process without an unclean termination, so might as well do an abort. Generally, it should be fine to abort Bitcoin Core at any time, because any critical data should always be flushed atomically to storage.

    But as this code should never be hit, it also seems fine to give the user the option to call RPCs to unload wallets, flush the chainstate, and disconnect peers, etc, before doing the unclean termination.

  110. in src/node/kernel_notifications.cpp:65 in ae6642973d outdated 
    60@@ -62,7 +61,9 @@ kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state
61 {
62     uiInterface.NotifyBlockTip(state, &index);
63     if (m_stop_at_height && index.nHeight >= m_stop_at_height) {
64-        StartShutdown();
65+        if (!m_shutdown()) {
66+            LogPrintf("Error: failed to send shutdown signal after reaching stop height\n");
    maflcko commented at 9:49 am on December 12, 2023:
    same
    ryanofsky commented at 1:40 pm on December 12, 2023:

    re: #28051 (review)

    same

    I’d give the same answer (https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423738157) here too

  111. in src/init.cpp:217 in eaf915d61d outdated 
    217-// signal handler sets ShutdownRequested(), which makes main thread's
218-// WaitForShutdown() interrupts the thread group.
219-// And then, WaitForShutdown() makes all other on-going threads
220-// in the thread group join the main thread.
221+// A clean exit happens when the SignalInterrupt object is triggered, which
222+// makes main thread's SignalInterrupt::wait() call return, and join all other
    maflcko commented at 9:59 am on December 12, 2023:
    nit in eaf915d61d470372e63f41f11d6a873c1936f73f: “the main thread”
    ryanofsky commented at 1:39 pm on December 12, 2023:

    re: #28051 (review)

    nit in eaf915d: “the main thread”

    Fixed, thank you!

  112. in src/init.cpp:230 in eaf915d61d outdated 
    224@@ -222,6 +225,11 @@ void InitContext(NodeContext& node)
225 // shutdown thing.
226 //
227 
228+bool ShutdownRequested(node::NodeContext& node)
229+{
230+    return bool{*(Assert(node.shutdown))};
    maflcko commented at 10:00 am on December 12, 2023:
    nit in eaf915d61d470372e63f41f11d6a873c1936f73f: Can drop the () around Assert()?
    ryanofsky commented at 1:40 pm on December 12, 2023:

    re: #28051 (review)

    nit in eaf915d: Can drop the () around Assert()?

    Good catch, fixed

  113. maflcko approved
  114. maflcko commented at 10:05 am on December 12, 2023: member

    It would be good to explain why the behavior changes are needed. Otherwise,

    concept ACK eaf915d61d470372e63f41f11d6a873c1936f73f 👇

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: concept ACK eaf915d61d470372e63f41f11d6a873c1936f73f 👇
3Zj5llDu5414hpNoftK5OmsCqlMoeZXi2akcICzuiYesSYUrLhMj2dGrGP1Ibp7xziOqBbwqKkTbUNzoKgL5eBQ==
  115. ryanofsky force-pushed on Dec 12, 2023
  116. ryanofsky commented at 7:41 pm on December 12, 2023: contributor

    Updated eaf915d61d470372e63f41f11d6a873c1936f73f -> 6db04be102807ee0120981a9b8de62a55439dabb (pr/noshut.22 -> pr/noshut.23, compare) just making suggested code tweaks and updating all commit messages to say what behavior is changing.

    re: #28051#pullrequestreview-1777064592

    It would be good to explain why the behavior changes are needed.

    Thank you, good catch. Some of the commits were explaining behavior changes, but not not all, and the PR description did not mention them. This should all be fixed now.

    concept ACK eaf915d61d470372e63f41f11d6a873c1936f73f 👇

    FYI, I noticed DrahtBot is interpreting this concept ack as a normal ACK in #28051 (comment), probably because “concept” is not capitalized

  117. TheCharlatan approved
  118. TheCharlatan commented at 8:16 pm on December 12, 2023: contributor
    Re-ACK 6db04be102807ee0120981a9b8de62a55439dabb
  119. DrahtBot requested review from maflcko on Dec 12, 2023
  120. in src/init.h:30 in 6db04be102 
    25@@ -26,6 +26,11 @@ namespace node {
26 struct NodeContext;
27 } // namespace node
28 
29+/** Initialize node context shutdown and args variables. */
30+void InitContext(node::NodeContext& node);
    maflcko commented at 1:15 pm on December 13, 2023:
    Nit in the last commit: Shouldn’t new node code be in the node namespace? Otherwise it will have to be touched again when it is moved into the node namespace later?
    ryanofsky commented at 8:38 pm on December 13, 2023:

    re: #28051 (review)

    Nit in the last commit: Shouldn’t new node code be in the node namespace? Otherwise it will have to be touched again when it is moved into the node namespace later?

    This is true, but it is not different from other init.h methods. Logically init.h and init.cpp belong in node subdirectory and should use the node namespace, so I didn’t want to make this one function stand out. The function also does need to be located in this file because it is accesses a static variable. So just keeping the Init functions in the same namespace seemed like the most straightforward approach for now.

  121. maflcko commented at 1:29 pm on December 13, 2023: member

    ACK 6db04be102807ee0120981a9b8de62a55439dabb 👗

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 6db04be102807ee0120981a9b8de62a55439dabb 👗
3svhRtNkGG1s2bMv7s6ElM2CG7nlbJ6ersNj58rdTM6YsNu2LTGwUMhVYMVduUxOzUnE++tiWYBOB91ptoUk5Ag==
  122. stickies-v approved
  123. stickies-v commented at 5:01 pm on December 13, 2023: contributor
    re-ACK 6db04be102807ee0120981a9b8de62a55439dabb
  124. achow101 commented at 8:03 pm on December 14, 2023: member

    ACK 6db04be102807ee0120981a9b8de62a55439dabb

    Code seems fine, but I’m not familiar enough with this area to say whether this is the right approach or if there are any side effects that haven’t been thought of.

  125. achow101 merged this on Dec 14, 2023
  126. achow101 closed this on Dec 14, 2023


ryanofsky DrahtBot maflcko TheCharlatan fanquake stickies-v achow101 theuni


fanquake

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-23 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me