kernel::Context, encapsulate global init/teardown
#25065
init/common.cpp is dependent on things like ArgsManager (which we wish to remove from libbitcoinkernel in the future) and sanity checks. These aren’t necessary for libbitcoinkernel so we only extract the portion that is necessary (namely init::{Set,Unset}Globals().
871@@ -870,7 +872,7 @@ libbitcoinkernel_la_SOURCES = \
872 index/base.cpp \
873 index/blockfilterindex.cpp \
874 index/coinstatsindex.cpp \
875- init/common.cpp \
876+ kernel/init/common.cpp \
Not sure if we want a kernel folder for code that is shared between node and kernel. An alternative would be to name it init/kernel.cpp or init/globals.cpp?
Otherwise it will be difficult to decide whether to put kernel code that is used elsewhere as well in kernel or in the “normal” hierarchy? cc @ryanofsky
I think the main thing here is to have a name that clearly indicates that this contains “only the common initialization code that the kernel needs” so that people don’t accidentally add non-kernel code to it (it would result in a linker error when building bitcoin-chainstate).
Other PRs in the project (e.g. #24410) does this as well, since “extracting only what the kernel needs” is the main goal of this phase of the project. I like using the kernel/ prefix since it makes the indication mentioned in the previous paragraph clear, and doesn’t require inventing a new name.
In this particular case though, perhaps kernel/init.cpp would work.
One question is: do we keep the two functions moved in the init:: namespace or the kernel:: namespace.
In the longer run, I think eventually (very far off when libbitcoinkernel is very minimal) we could put everything that links into libbitcoinkernel into the src/kernel/ folder.
So is the goal to eventually split validation into kernel/validation.cpp and node/validation.cpp ?
Yes, far off into the future we will likely do that (although I think in reality node/validation.cpp will likely be either: 1. a very very short file, or 2. deleted because what would be left belongs better in a differently named file).
What about util functions that are included in the kernel?
I’ve discussed this with Ryan in #24352, and we came to the conclusion that we will move all the utils that libbitcoinkernel depends on to libbitcoin_util, now that’s just w/re the libraries, we could do either one of two things for the hierarchy:
src/kernel/ and src/util/, orsrc/kernel/ and src/kernel/util/Either is fine with me. See here for more discussion: #24352 (comment)
Hmm, so to recall the goals of kernel are:
(Correct me if I am wrong)
Putting all utils into the kernel util library helps neither goal. Though, at the same time anything used by kernel isn’t strictly consensus code. For example logging is used, but it is not consensus critical. Still, anything not used by kernel can’t be consensus code, so I am wondering if there should be two util libs? If yes, then it would make sense to have both src/util and src/kernel/util. If not, then I don’t care what is done.
so I am wondering if there should be two util libs? If yes, then it would make sense to have both
src/utilandsrc/kernel/util. If not, then I don’t care what is done.
I think that the point of #24352 (comment) is to establish exactly what you’re saying. In effect:
common will be the non-kernel “util lib” (i.e. src/util), andutil will be the kernel “util lib” (i.e. src/kernel/util).
I think that the point of #24352 (comment) is to establish exactly what you’re saying. In effect:
SGTM
For example logging is used
Yes. Though, for the eventual libbitcoin kernel we need to find some way to do logging that doesn’t make the client application depend on bitcoind’s logging subsystem (say, a hook / notification registration). Out of scope here, of course.
7@@ -8,11 +8,11 @@
8 #ifndef BITCOIN_INIT_COMMON_H
9 #define BITCOIN_INIT_COMMON_H
10
11+#include <kernel/include/init/common.h>
Ah right. Yeah the thought here (in general for the libbitcoinkernel project) is for the existing headers to export things from the “kernel-extracted version of headers” so that we don’t have too much code churn adding #include <kernel/*.h> all over the place. (See my other comment here for more details: #25065 (review))
I think a good solution would be to do:
0#include <kernel/include/init/common.h> // IWYU pragma: export
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
Pushed d5876d1efbf01f45fff2b42e0bcf2bc12eb25240 -> 13e65511dba702d115b7bea147b12eefaea9d53f
Pushed 13e65511dba702d115b7bea147b12eefaea9d53f -> 708e2711e591ce4958900715348cb0313763f2cb
Pushed 708e2711e591ce4958900715348cb0313763f2cb -> e4c35528e58a74138aa7e0560dd7f59f1086c765
#ifdef: https://github.com/bitcoin/bitcoin/runs/6313105188
7@@ -8,11 +8,11 @@
8 #ifndef BITCOIN_INIT_COMMON_H
9 #define BITCOIN_INIT_COMMON_H
10
11+#include <kernel/init/common.h> // IWYU pragma: export
This is the first of these pragmas I’ve seen. I suspect I’m out of the loop on something.
I’m assuming this is some hint to IWYU, but I’m not sure what it means?
12+
13 class ArgsManager;
14
15 namespace init {
16-void SetGlobals();
17-void UnsetGlobals();
SanityChecks out as well, as it’s effectively a runtime test for SetGlobals (and it depends on it too).
bool kernel::Context::SanityCheck()? Doing that does mean we can’t remove compat/glibcxx_sanity.cpp from libbitcoinkernel, but I’m fine with it either way so just lmk what you prefer.
re: #25065 (review)
Do you mean something like
bool kernel::Context::SanityCheck()? Doing that does mean we can’t removecompat/glibcxx_sanity.cppfrom libbitcoinkernel, but I’m fine with it either way so just lmk what you prefer.
I’d agree it would be good to move sanity checks to the kernel assuming using glibcxx_sanity.cpp doesn’t add costs or complications. (glibcxx_sanity.cpp is already part of libbitcoin_util so it seems fine at first glance). But it doesn’t seem essential to add the checks if it is a problem.
I do think if you add the checks, you should move them to a standalone function bool SanityChecks(Context&) in src/kernel/checks.cpp or similar. A bool Context::SanityChecks() method in src/kernel/context.cpp would make kernel code less modular, and risk sending Context object down a road where it becomes a disorganized CWallet-like object with methods that do many unrelated things.
re: #25065 (review)
Doing that does mean we can’t remove
compat/glibcxx_sanity.cppfrom libbitcoinkernel
Looking at current SanityChecks() function, maybe a bigger problem moving it into the kernel is that it is using
InitError from ui_interface.cpp. So might be worth postponing this or doing it separately if it would complicate this PR.
Nice, thanks.
Now that we have the notion of a context, it would be possible to either:
But those can be done much later when we start actually defining an api.
8+#include <pubkey.h>
9+#include <random.h>
10+
11+#include <memory>
12+
13+static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle;
I don’t love the idea of chopping up and rearranging and moving the static cruft to the new clean home.
Do you have a next step in mind for this stuff?
Concept ACK and looks good from a light review.
No comment on the naming/structure, what was discussed above SGTM. I suspect this will change around a few times before finally settling anyway.
The split here seems pretty arbitrary… I could imagine a few different approaches to chopping up common.cpp and (without digging too much) this isn’t the most obvious one to me. But if this keeps things moving, sure 👍.
Few suggestions (feel free to ignore):
src/kernel/init/common.h,cpp to src/kernel/init.h,cppnamespace init to namespace kernel (namespace name not matching directory name is like a sign pointing in wrong direction, probably worse than not having the sign)kernel::SetGlobals kernel::UnsetGlobals call these kernel::Init kernel::Destroy or Load/Unload or similar. If you wanted to be really fancy you could call these kernel::Context::Context() and kernel::Context::~Context() but no need to go crazy with another context struct yet.
To explain suggestions in #25065 (comment) a little, I think the libbitcoin_kernel external interface should not use global variables. libbitcoin_kernel is supposed to be a library, and having used many libraries, I’d claim that libraries which use global variables or other implicit singletons are a pain to develop and test with.
I understand bitcoin code uses global variables, and you don’t want to make big changes to existing code to get rid of them. This is fine. But we shouldn’t add kernel functions which require the implementation use to global variables, and make them difficult to get rid of in the future.
You can easily create a clean API that does not require other code to use global variables by defining a kernel::Context struct that owns whatever state is needed to run kernel functions, and passing Context* to functions in src/kernel/ meant to be called externally. Internally (especially at first), Context* won’t be accessible to a lot of existing code, so global variables should still be used. They just need to point to the context or to specific members of the context instead of owning the state directly. Not much existing code should have to change. The only code that might have to change is the lowest-level code directly referencing global variables that were moved. Only some of this code would have to change, and no code above it would have to change.
At a higher level, the kernel::Context struct could just be understood as a thing which lets libbitcoin_kernel code remember state between calls, and interact with the outside world without relying on globals. So it can ask where is the data dir? How do I log this message? How do I generate a random number? What time is it? And so on
I understand bitcoin code uses global variables, and you don’t want to make big changes to existing code to get rid of them. This is fine. But we shouldn’t add kernel functions which require the implementation use to global variables, and make them difficult to get rid of in the future.
Totally agree with this. I think I was pretty much complaining about the same thing here: #25065 (review)
I expect lots of weird in-between-ness at this stage. But I was generally hoping that for the most part, the stuff that gets moved into the library would be relatively clean and not in need of much more untangling. In that regard, this PR seems a bit regressive.
At a higher level, the
kernel::Contextstruct could just be understood as a thing which lets libbitcoin_kernel code remember state between calls, and interact with the outside world without relying on globals. So it can ask where is the data dir? How do I log this message? How do I generate a random number? What time is it? And so on
100% agree with this as well. I’ve been expecting a new Context struct to come into play and at this point it’s a bit weird that we don’t have one. This PR may be a good opportunity to introduce it.
Pushed 6d93c443a44f06296e8014c65bc70f44cc494fca -> 9bc9309bd61f36ac68b41fe2328d2e3f505397e1
kernel::Context and move the global initialization logic to its ctor/dtor. (Thanks Cory+Ryan)compat/glibcxx_sanity.cpp in libbitcoinkernel for now until we figure out #25065 (review)
Thanks for all the feedback @ryanofsky and @theuni, I really appreciate them. I totally agree that in this case it was a great idea and easy to introduce the kernel::Context along with setting the globals, I actually discovered I had an old branch that did exactly this 😅
Although in this case it was quite easy to make this change so we get the N I C E C O D E, it might be the case that in future PRs there will be instances where getting the N I C E C O D E will require very intrusive changes that doesn’t serve the goal of Phase 1, which is just about decoupling modules. Should these cases arise, I hope we can decide to table them for later (although most of these cases are likely solved by introducing a kernel::Context g_kernel, but we’ll do that when we get to it).
Code review ACK 9bc9309bd61f36ac68b41fe2328d2e3f505397e1. Looks great, I’d say.
there will be instances where getting the N I C E C O D E will require very intrusive changes that doesn’t serve the goal
This seems fine as long as we have a reasonable idea of what a good cleaned up state would look like. Would just want to avoid making a mess without a plan to clean it up.
Push 9bc9309bd61f36ac68b41fe2328d2e3f505397e1 -> 9ac87854af62d271df2f383d8f2f798c70453448
@ryanofsky I know you don’t like overly-split commits, happy to merge the last two if you wish.
11
12 struct Context;
13
14+enum class SanityCheckError {
15+ ERROR_INITITALIZING_ECC,
16+ ERROR_GLIBCXX_INSANITY,
Pushed 9ac87854af62d271df2f383d8f2f798c70453448 -> 239501f5d0e1df8d36a62f10c4de4e9cdc913a9c
15+//! kernel libary API is a work in progress, so state organization and member
16+//! list will evolve over time.
17+//!
18+//! State stored directly in this struct should be simple. More complex state
19+//! should be stored to std::unique_ptr members pointing to opaque types.
20+struct Context {
The commit message of the first commit test: Use Set/UnsetGlobals in BasicTestingSetup reads:
In a future PR, we will introduce a kernel::Context …
However, kernel::Context is introduced in this PR. Maybe s/a future PR/the next commit/.
8+#include <memory>
9+
10+class ECCVerifyHandle;
11+
12+namespace kernel {
13+//! Context struct holding global the kernel library's global state, and passed
0//! Context struct holding the kernel library's state, and passed
50@@ -49,7 +51,7 @@ int main(int argc, char* argv[])
51 SelectParams(CBaseChainParams::MAIN);
52 const CChainParams& chainparams = Params();
53
54- init::SetGlobals(); // ECC_Start, etc.
55+ kernel::Context context{};
kernel::Context is called context here and kernel inside struct NodeContext. kernel_context, kernel_state or kernel_ctx may be better.
kernel_context here and kernel elsewhere. Not a big deal, but I think it is better to be consistent if there are no other reasons for the naming.
9+#include <random.h>
10+#include <util/time.h>
11+
12+namespace kernel {
13+
14+std::optional<SanityCheckError> SanityChecks(Context&)
Why bother introducing an argument if it is going to be unused?
It’s because src/kernel/ is supposed to provide a library API, and the API should not have to change as we remove globals internally.
10+namespace kernel {
11+
12+struct Context;
13+
14+enum class SanityCheckError {
15+ ERROR_ECC,
The word “error” is redundant in the enum name and in each enum value:
0SanityCheckError::ERROR_ECC
1 ^^^^^ ^^^^^
Maybe strip the ERROR_ prefix: SanityCheckError::ECC?
ACK 239501f5d0e1df8d36a62f10c4de4e9cdc913a9c
Some non-blockers below.
1095+bool AppInitSanityChecks(NodeContext& node)
1096 {
1097 // ********************************************************* Step 4: sanity checks
1098
1099- init::SetGlobals();
1100+ node.kernel = std::make_unique<kernel::Context>();
Mmm, this whole function is a mess and doesn’t really make much sense anymore.
I really don’t like the creation of the kernel::Context happening in here. That’s not at all intuitive. I would much prefer to see the caller create and set it. Mind addressing that here?
A few potential follow-ups for a later PR:
kernel::Context creation as a next step.NodeContext to take a kernel::Context in its constructor so that it always exists in the NodeContext. That might be much easier said than done though, I haven’t looked deeply into it.LockDataDirectory here, meaning that AppInitSanityChecks would just go away.
I would much prefer to see the caller create and set it. Mind addressing that here?
Let me know if you think 8d63a934cf4053851ba79d4a44326cd079bc8288 suffices. If so, I will squash it in.
A few potential follow-ups for a later PR: …
I will open an issue after this PR is merged so we don’t lose track of these!
Pushed cf4c2043f592afe1d175f4c54e9728da38c0b184 -> 8d63a934cf4053851ba79d4a44326cd079bc8288
Pushed 8d63a934cf4053851ba79d4a44326cd079bc8288 -> ded2ef302cd29d3dc88a847f0fe8c947b3a6044a
50@@ -49,7 +51,7 @@ int main(int argc, char* argv[])
51 SelectParams(CBaseChainParams::MAIN);
52 const CChainParams& chainparams = Params();
53
54- init::SetGlobals(); // ECC_Start, etc.
55+ kernel::Context kernel_context{};
This also needs a kernel::SanityChecks(kernel_context), no? Another argument for making it part of context creation.
(It was also missing before, but presumably because it was still tangled up with the ui).
Hmm, it is strange to sanity-check a default constructed object:
0Context foo;
1assert(sanity_check(foo));
Shouldn’t a default constructor create a sane object? If sometimes it can produce insane object (based on what? it takes no arguments) then it is better to throw from the constructor instead of assert()ing immediately after object creation.
Completely agree it’s wonky, I had a similar complaint here: #25065 (review)
I think it’s fine as-is for now though, as context creation will definitely be changing in subsequent PRs.
ACK ded2ef302cd29d3dc88a847f0fe8c947b3a6044a
I would expect the kernel API to be defined in src/kernel/*.h inside namespace kernel and those files to not define anything else. src/kernel/chainstatemanager_opts.h does not fit that pattern - it defines ChainstateManagerOpts outside of the kernel namespace. So, is ChainstateManagerOpts part of the kernel API? If yes, then it should be inside namespace kernel, if not then it should not be defined in src/kernel/*.h. ChainstateManagerOpts is only used in validation.h.
...instead of calling initialization functions directly and having to
keep around a ECCVerifyHandle member variable.
This makes the initialization codepath of our tests more closely
resemble those of AppInitMain and potentially eases the review of
subsequent commit removing init::{Set,Unset}Globals.
[META] In a future commit, we will introduce a kernel::Context which
calls init::{Set,Unset}Globals in its ctor and dtor. It will be
owned by node::NodeContext, so in the end, this patchset won't
have made the previously local ECCVerifyHandle global.
[META] In the next commit, we will move the init::{Set,Unset}Globals
logic into this struct.
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>Pushed ded2ef302cd29d3dc88a847f0fe8c947b3a6044a -> fa2ee7b12c0c15be8e9807444443b5bd2c91c3e5
#includes in src/bitcoin-chainstate.cpp
I would expect the kernel API to be defined in
src/kernel/*.hinsidenamespace kerneland those files to not define anything else.src/kernel/chainstatemanager_opts.hdoes not fit that pattern - it definesChainstateManagerOptsoutside of thekernelnamespace. So, isChainstateManagerOptspart of the kernel API? If yes, then it should be insidenamespace kernel, if not then it should not be defined insrc/kernel/*.h.ChainstateManagerOptsis only used invalidation.h.
Sorry about my cheeky post above 😄 To respond to your comment: yes it should eventually be in namespace kernel, so should most of validation.cpp. Haven’t done it yet because it’s not required by any of the “PRs in the main thrust of libbitcoinkernel”.
1090@@ -1089,13 +1091,23 @@ static bool LockDataDirectory(bool probeOnly)
1091 return true;
1092 }
1093
1094-bool AppInitSanityChecks()
1095+bool AppInitSanityChecks(kernel::Context& kernel)
18+};
19+
20+/**
21+ * Ensure a usable environment with all necessary library support.
22+ */
23+std::optional<SanityCheckError> SanityChecks(Context&);
89@@ -90,8 +90,14 @@ class NodeImpl : public Node
90 uint32_t getLogCategories() override { return LogInstance().GetCategoryMask(); }
91 bool baseInitialize() override
92 {
93- return AppInitBasicSetup(gArgs) && AppInitParameterInteraction(gArgs, /*use_syscall_sandbox=*/false) && AppInitSanityChecks() &&
94- AppInitLockDataDirectory() && AppInitInterfaces(*m_context);
95+ return AppInitBasicSetup(gArgs) &&
Heh, this has gotten unwieldy and hard to read imo.
How about (untested)
0if (!AppInitBasicSetup(gArgs)) return false;
1if (!AppInitParameterInteraction(gArgs, /*use_syscall_sandbox=*/false)) return false;
2
3m_context->kernel = std::make_unique<kernel::Context>();
4if (!AppInitSanityChecks(*m_context->kernel)) return false;
5if (!AppInitLockDataDirectory()) return false;
6if (!AppInitInterfaces(*m_context)) return false;
7return true;
I took one last pass through, looking good.
Left a comment about a messy function, not the end of the world if it doesn’t get addressed here.
I do think the const ref issues (there may be others) should be addressed though so that it’s clear moving forward what’s intended to be read-only.
ACK fa2ee7b12c0c15be8e9807444443b5bd2c91c3e5
yes it should eventually be in
namespace kernel
Ok, why not get it right from the first time, since it is trivial to do so:
0--- i/src/kernel/chainstatemanager_opts.h
1+++ w/src/kernel/chainstatemanager_opts.h
2@@ -7,17 +7,20 @@
3
4 #include <cstdint>
5 #include <functional>
6
7 class CChainParams;
8
9+namespace kernel {
10+
11 /**
12 * An options struct for `ChainstateManager`, more ergonomically referred to as
13 * `ChainstateManager::Options` due to the using-declaration in
14 * `ChainstateManager`.
15 */
16 struct ChainstateManagerOpts {
17 const CChainParams& chainparams;
18 const std::function<int64_t()> adjusted_time_callback{nullptr};
19 };
20+} // namespace kernel
21
22 #endif // BITCOIN_KERNEL_CHAINSTATEMANAGER_OPTS_H
23--- i/src/validation.h
24+++ w/src/validation.h
25@@ -854,13 +854,13 @@ private:
26 const CBlockHeader& block,
27 BlockValidationState& state,
28 CBlockIndex** ppindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
29 friend CChainState;
30
31 public:
32- using Options = ChainstateManagerOpts;
33+ using Options = kernel::ChainstateManagerOpts;
34
35 explicit ChainstateManager(const Options& opts)
36 : m_chainparams{opts.chainparams},
37 m_adjusted_time_callback{Assert(opts.adjusted_time_callback)} {};
38
39 const CChainParams& GetParams() const { return m_chainparams; }
...instead of explicitly calling init::{Set,Unset}Globals.
Cool thing about this is that in both the testing and bitcoin-chainstate
codepaths, we no longer need to explicitly unset globals. The
kernel::Context goes out of scope and the globals are unset
"automatically".
Also construct kernel::Context outside of AppInitSanityChecks()Pushed fa2ee7b12c0c15be8e9807444443b5bd2c91c3e5 -> a8cdeca079a362d2a021aaf7387fbb75a15ebc18
Ok, why not get it right from the first time, since it is trivial to do so:
Yeah sorry I should have done that in the first place. But it’s unrelated to the current PR right now so we’ll just do it later. I’ve added it as a TODO to the umbrella issue!
This reduces libbitcoinkernel's coupling with ui_interface and
translation.
1109+ case kernel::SanityCheckError::ERROR_RANDOM:
1110+ InitError(Untranslated("OS cryptographic RNG sanity check failure. Aborting."));
1111+ break;
1112+ case kernel::SanityCheckError::ERROR_CHRONO:
1113+ InitError(Untranslated("Clock epoch mismatch. Aborting."));
1114+ break;
0default:
1 assert(0)
?
FWIW, gcc/clang have built-ins (__builtin_unreachable) for expressing this explicitly.
LLVM even has a macro for it, and specifies the use of llvm_unreachable in its coding standard.
Maybe we should borrow the idea and add a similar macro. Not in this PR though, of course :)
Edit: explanation of what llvm_unreachable does:
“When assertions are enabled, this will print the message if it’s ever reached and then exit the program. When assertions are disabled (i.e. in release builds), llvm_unreachable becomes a hint to compilers to skip generating code for this branch. If the compiler does not support this, it will fall back to the “abort” implementation.”
Interesting, though I think we’d always want to abort, even in release builds. For example, it would be UB to skip return in a non-void function, no? This is also what the docs say:
llvm_unreachable() becomes an optimizer hint that the current location is not supposed to be reachable: the hint turns such code path into undefined behavior.
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/ErrorHandling.h#L131
For reference, in #24812 (review) I argued that assert(0) is good enough for us. Also, I don’t think there is a compiler that doesn’t understand assert(0)?
The only reason I suggested is because imo it’s nice to be able to state “shouldn’t be able to get here” differently than “crash if ever here”, as not stating the former clearly leads to (good) questions in review like this one from @JeremyRubin.
Even if bitcoin_unreachable() is just a macro defined as assert(0), I think it’d be an improvement over nothing.
assert(0), if ppl think it is useful.
… why there is no default case …
My favorite reason is that, because the switch value is an enum, we get a compile-time warning (or error with -Werror which is used by CI) if not all enum values are listed.
Pushed a8cdeca079a362d2a021aaf7387fbb75a15ebc18 -> d87784ac87364fc977bbf9769c8bdb72dea8cbf9
80@@ -81,7 +81,7 @@ static constexpr CAmount CENT{1000000};
81 * This just configures logging, data dir and chain parameters.
82 */
83 struct BasicTestingSetup {
84- node::NodeContext m_node;
85+ node::NodeContext m_node; // keep as first member to be destructed last
Something like this?
0diff --git a/src/node/context.h b/src/node/context.h
1index 31be308787..7ed1292180 100644
2--- a/src/node/context.h
3+++ b/src/node/context.h
4@@ -29,6 +29,16 @@ class Init;
5 class WalletLoader;
6 } // namespace interfaces
7
8+namespace {
9+/**
10+ * Dummy NodeContext to ensure that kernel::Context is always destructed last.
11+ */
12+struct BaseNodeContext {
13+ //! libbitcoin_kernel context
14+ std::unique_ptr<kernel::Context> kernel;
15+};
16+}
17+
18 namespace node {
19 //! NodeContext struct containing references to chain state and connection
20 //! state.
21@@ -40,9 +50,7 @@ namespace node {
22 //! and make code more modular and testable. The struct isn't intended to have
23 //! any member functions. It should just be a collection of references that can
24 //! be used without pulling in unwanted dependencies or functionality.
25-struct NodeContext {
26- //! libbitcoin_kernel context
27- std::unique_ptr<kernel::Context> kernel;
28+struct NodeContext : BaseNodeContext {
29 //! Init interface for initializing current process and connecting to other processes.
30 interfaces::Init* init{nullptr};
31 std::unique_ptr<AddrMan> addrman;
32diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
33index 5c31cfc22b..87d5d4a24b 100644
34--- a/src/test/util/setup_common.h
35+++ b/src/test/util/setup_common.h
36@@ -77,12 +77,20 @@ static inline bool InsecureRandBool() { return g_insecure_rand_ctx.randbool(); }
37
38 static constexpr CAmount CENT{1000000};
39
40+namespace {
41+/**
42+ * Dummy TestingSetup to ensure that node::NodeContext (and by extension
43+ * kernel::Context) is always destructed last.
44+ */
45+struct BaseTestingSetup {
46+ node::NodeContext m_node;
47+};
48+}
49+
50 /** Basic testing setup.
51 * This just configures logging, data dir and chain parameters.
52 */
53-struct BasicTestingSetup {
54- node::NodeContext m_node; // keep as first member to be destructed last
55-
56+struct BasicTestingSetup : BaseTestingSetup {
57 explicit BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::vector<const char*>& extra_args = {});
58 ~BasicTestingSetup();
59
Yep, pretty much!
Sorry, I didn’t mean to imply that needed to be done as part of this PR though. It might be worth taking a look at doing it generically with a template, which could help communicate the intent more clearly:
0BasicTestingSetup : MustDestructLast<node::NodeContext>
But maybe that’s over-engineering, idk.
A foot-gun-proof way to do this would be to inherit from a class that holds a node::NodeContext.
Would really encourage not doing this. There is no ambiguity about initialization order in the c++ language: earlier members are constructed first and destructed last, later members are constructed later and destructed earlier. Moving members into a base class to control what they are initialized here would seems mostly like a form of obfuscation.
It is true that if a future developer willingly wants to ignore a “this member should be first” comment it will be harder for them to do that if inheritance is involved, but it’s true in general that if you obfuscate code it is harder to modify, and I don’t think this logic leads to good places
@ryanofsky Points taken. I think the intents/relationships need to be made clear somehow. Programmatically is one way, but maybe a comment works just as well without all the obfuscation.
In any case, this definitely shouldn’t slow down this PR.
51@@ -49,7 +52,11 @@ int main(int argc, char* argv[])
52 SelectParams(CBaseChainParams::MAIN);
53 const CChainParams& chainparams = Params();
54
55- init::SetGlobals(); // ECC_Start, etc.
56+ kernel::Context kernel_context{};
57+ // We can't use a goto here, but we can use an assert since none of the
goto instead of adding comments on why goto can’t be used here. :sweat_smile:
19+{
20+ std::string sha256_algo = SHA256AutoDetect();
21+ LogPrintf("Using the '%s' SHA256 implementation\n", sha256_algo);
22+ RandomInit();
23+ ECC_Start();
24+ ecc_verify_handle.reset(new ECCVerifyHandle());
145@@ -146,7 +146,6 @@ BasicTestingSetup::~BasicTestingSetup()
146 LogInstance().DisconnectTestLogger();
147 fs::remove_all(m_path_root);
148 gArgs.ClearArgs();
149- init::UnsetGlobals();
m_node.kernel isn’t reset and left “dangling” until the end of ~BasicTestingSetup, which only happens to be here? Seems both brittle and inconsistent with the shutdown sequence in bitcoind, no?
0@@ -0,0 +1,33 @@
1+// Copyright (c) 2022 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
nit: Could have added new files to be checked by iwyu?
0diff --git a/ci/test/06_script_b.sh b/ci/test/06_script_b.sh
1index e64af2ad5..f17d823ee 100755
2--- a/ci/test/06_script_b.sh
3+++ b/ci/test/06_script_b.sh
4@@ -43,2 +41,4 @@ if [ "${RUN_TIDY}" = "true" ]; then
5 " src/init"\
6+ " src/kernel/checks.cpp"\
7+ " src/kernel/context.cpp"\
8 " src/rpc/fees.cpp"\
src/kernel is now under IWYU.
10+class ECCVerifyHandle;
11+
12+namespace kernel {
13+//! Context struct holding the kernel library's logically global state, and
14+//! passed to external libbitcoin_kernel functions which need access to this
15+//! state. The kernel libary API is a work in progress, so state organization
0src/kernel/context.h:15: libary ==> library