The globals setup by the function calls when creating a new kernel context only need to be setup once. Calling them multiple times may be wasteful and has no apparent benefit.
Besides kernel users potentially creating multiple contexts, this change may also be useful for tests creating multiple setups.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, tdb3, maflcko |
| Stale ACK | danielabrozzoni |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
18- LogPrintf("Using the '%s' SHA256 implementation\n", sha256_algo);
19- RandomInit();
20+ static bool initialized{false};
21+ if (!initialized) {
22+ std::string sha256_algo = SHA256AutoDetect();
23+ LogPrintf("Using the '%s' SHA256 implementation\n", sha256_algo);
While this code is being touched, do we want to use more specific logging instead of LogPrintf? (e.g. LogDebug, LogInfo, etc.)
https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#logging
Updated de7853277625a4a483d157a9a85df9b51d07c9d5 -> 50c14e5a170950f30aa6b474bb231bbbfdd82427 (staticInitKernel_0 -> staticInitKernel_1, compare)
15 Context::Context()
16 {
17- std::string sha256_algo = SHA256AutoDetect();
18- LogPrintf("Using the '%s' SHA256 implementation\n", sha256_algo);
19- RandomInit();
20+ static bool initialized{false};
nit: std::call_once would be appropriate here I think, but the current approach is clear too so not important
0diff --git a/src/kernel/context.cpp b/src/kernel/context.cpp
1index a7c3457c8c..c34dc84704 100644
2--- a/src/kernel/context.cpp
3+++ b/src/kernel/context.cpp
4@@ -8,19 +8,18 @@
5 #include <logging.h>
6 #include <random.h>
7
8+#include <mutex>
9 #include <string>
10
11 namespace kernel {
12 Context::Context()
13 {
14- static bool initialized{false};
15- if (!initialized) {
16+ static std::once_flag is_initialized;
17+ std::call_once(is_initialized, [](){
18 std::string sha256_algo = SHA256AutoDetect();
19 LogInfo("Using the '%s' SHA256 implementation\n", sha256_algo);
20 RandomInit();
21- }
22- initialized = true;
23+ });
24 }
25
26-
27 } // namespace kernel
nit: since the flag is about the globals, and not the context, being initialized - would globals_initialized be a better name? It doesn’t make a difference now, but it will when the constructor does more than just setting up globals.
0 static bool globals_initialized{false};
bitcoind and bitcoin-qt, only affects the tests (and downstream).
The globals setup by the function calls when creating a new kernel
context only need to be setup once. Calling them multiple times may be
wasteful and has no apparent benefit.
Besides kernel users potentially creating multiple contexts, this change
may also be useful for tests creating multiple setups.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Thanks for the suggestions @stickies-v,
50c14e5a170950f30aa6b474bb231bbbfdd82427 -> 93fb0e7897000072ea790a91816aea876718ad27 (staticInitKernel_1 -> staticInitKernel_2, compare)
std::call_once instead of static boolean flag. This makes the behavior consistent on the off chance that a context is created concurrently.initialized to globals_initialized.
Concept ACK. The context constructor isn’t thread-safe. This can be tested with tsan and:
0diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
1index abe3d6a505..05d00eaf97 100644
2--- a/src/test/util/setup_common.cpp
3+++ b/src/test/util/setup_common.cpp
4@@ -62,6 +62,7 @@
5
6 #include <algorithm>
7 #include <functional>
8+#include <future>
9 #include <stdexcept>
10
11 using kernel::BlockTreeDB;
12@@ -195,6 +196,13 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts)
13 AppInitParameterInteraction(*m_node.args);
14 LogInstance().StartLogging();
15 m_node.warnings = std::make_unique<node::Warnings>();
16+ {
17+ auto fun{[] { kernel::Context{}; }};
18+ auto futs{std::array{
19+ std::async(std::launch::async, fun),
20+ std::async(std::launch::async, fun),
21+ }};
22+ }
23 m_node.kernel = std::make_unique<kernel::Context>();
24 m_node.ecc_context = std::make_unique<ECC_Context>();
25 SetupEnvironment();
Which should print a data-race. So it seems fine to guard the code and only execute it once.
ACK 93fb0e7897000072ea790a91816aea876718ad27 👝
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 93fb0e7897000072ea790a91816aea876718ad27 👝
3qDwtQZZCJDX/9k96k2fh3KtTVl9a3ZmElpWNyBB9S0OOk47oYfmKnOTgbM0NiJD/TKvFbXiAMTKqGYgiLK/LAQ==
