kernel: Move background load thread to node context

TheCharlatan commented at 11:28 am on September 13, 2024: contributor

The thread handle is never used by the ChainstateManager, so move it out and into the node context. Users of the kernel library now no longer have to manually join the thread when destructing the ChainstateManager.

DrahtBot commented at 11:28 am on September 13, 2024: 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	maflcko, ryanofsky, jonatack, stickies-v, achow101

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

DrahtBot added the label Validation on Sep 13, 2024

in src/init.cpp:301 in 3a26c839c6 outdated

297@@ -298,7 +298,7 @@ void Shutdown(NodeContext& node)
298 
299     StopTorControl();
300 
301-    if (node.chainman && node.chainman->m_thread_load.joinable()) node.chainman->m_thread_load.join();
302+    if (node.chainman && node.background_init_thread.joinable()) node.background_init_thread.join();

maflcko commented at 11:51 am on September 13, 2024:

nit: While the thread shouldn’t be joinable when chainman is null, it may be cleaner to remove the check, or add a comment why it is needed?

TheCharlatan commented at 2:10 pm on September 13, 2024:

Yeah, on second thought this is a bit confusing. The default constructed thread should already be not joinable.

maflcko approved

maflcko commented at 11:52 am on September 13, 2024: member

Make sense to rename and move the thread, now that it is doing more general init things and not only importing blocks.

ACK 3a26c839c63ecda2d3b2491d6d84a03c1d8f508f 🤠

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 3a26c839c63ecda2d3b2491d6d84a03c1d8f508f 🤠
3VezQmUV7RyyLGIgClPZqgPQoosT41EKqVfcJYqH+6NUCHRYqjRYJNbLKeu0cKp2d3QaBfSFdOpDv9n0yJAf/BA==

kernel: Move background load thread to node context

The thread handle is never used by the ChainstateManager, so move it out
and into the node context. Users of the kernel library now no longer
have to manually join the thread when destructing the ChainstateManager.

bc7900f33d

TheCharlatan force-pushed on Sep 13, 2024

TheCharlatan commented at 2:13 pm on September 13, 2024: contributor

Updated 3a26c839c63ecda2d3b2491d6d84a03c1d8f508f -> bc7900f33db3d01fb93dfee7981c01ea495cd42e (ChainmanRmThreadLoad_0 -> ChainmanRmThreadLoad_1, compare)

Addressed @maflcko’s comment, removed no longer needed guard against the chainman not being initialized yet.

maflcko commented at 2:16 pm on September 13, 2024: member

ACK bc7900f33db3d01fb93dfee7981c01ea495cd42e 🔄

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 bc7900f33db3d01fb93dfee7981c01ea495cd42e 🔄
3C4mJm32/ZTaaHIiVMR+ho6U5vWum6CXVK+YwlKI7GRuNemWa4ZZ8soB2Fj7esOhhM7IJ+OOVHE0L3Xc3NCIxBw==

ryanofsky approved

ryanofsky commented at 2:22 pm on September 13, 2024: contributor

Code review ACK bc7900f33db3d01fb93dfee7981c01ea495cd42e. Nice cleanup

in src/node/context.h:90 in bc7900f33d

86@@ -86,6 +87,7 @@ struct NodeContext {
87     std::atomic<int> exit_status{EXIT_SUCCESS};
88     //! Manages all the node warnings
89     std::unique_ptr<node::Warnings> warnings;
90+    std::thread background_init_thread;

stickies-v commented at 4:35 pm on September 13, 2024:

nit: needs Doxygen cross-reference update in developer-notes.md to reflect the new anchor structnode_1_1_node_context.html#aef02af873199206a8f032a3917277bef (as per cmake --build build -t docs on 3a26c839c63ecda2d3b2491d6d84a03c1d8f508f)

 0diff --git a/doc/developer-notes.md b/doc/developer-notes.md
 1index a630957f41..febd158c68 100644
 2--- a/doc/developer-notes.md
 3+++ b/doc/developer-notes.md
 4@@ -634,7 +634,7 @@ Threads
 5   : Started from `main()` in `bitcoind.cpp`. Responsible for starting up and
 6   shutting down the application.
 7 
 8-- [Init load (`b-initload`)](https://doxygen.bitcoincore.org/namespacenode.html#ab4305679079866f0f420f7dbf278381d)
 9+- [Init load (`b-initload`)](https://doxygen.bitcoincore.org/structnode_1_1_node_context.html#aef02af873199206a8f032a3917277bef)
10   : Performs various loading tasks that are part of init but shouldn't block the node from being started: external block import,
11    reindex, reindex-chainstate, main chain activation, spawn indexes background sync threads and mempool load.
12

maflcko commented at 8:39 am on September 16, 2024:

I think the doxygen links in this doc are dead anyway, or at least stale. (At least I can’t remember when they last worked and were up-to-date).

jonatack commented at 4:49 pm on September 13, 2024: member

Light ACK bc7900f33db3d01fb93dfee7981c01ea495cd42e

Good further separation of concerns.

stickies-v approved

stickies-v commented at 4:59 pm on September 13, 2024: contributor

ACK bc7900f33db3d01fb93dfee7981c01ea495cd42e

I looked into the history of why m_thread_load is a ChainstateManager member, but it appears there’s no particular reason (edit: besides the thread at that point only being used to load blocks, as pointed out by maflcko earlier). faf843c07f99f91603e08ea858f972516f1d669a deglobalized g_load_block into ChainstateManager::m_load_block (later renamed (04575106b2529f495ce8110ddf7ed2247d4bc339) to m_thread_load) and it appears the clean-up of this PR could have been applied to that commit too (but building that >3yo commit seems non-trivial so I can’t verify).

DrahtBot requested review from stickies-v on Sep 13, 2024

achow101 commented at 9:48 pm on September 13, 2024: member

ACK bc7900f33db3d01fb93dfee7981c01ea495cd42e

achow101 merged this on Sep 13, 2024

achow101 closed this on Sep 13, 2024

kernel: Move background load thread to node context #30896

Code Coverage

Reviews