[kernel 2c/n] Introduce `kernel::Context`, encapsulate global init/teardown #25065

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 ? What about util functions that are included in the kernel?

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:

1. src/kernel/ and src/util/, or
2. src/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:

• Provide a self-contained thing for other projects to use.
• Make it clear what code is consensus related and what code isn't.

(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/util and src/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), and
• util 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.

nit: Pretty sure this is illegal according to iwyu?

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:

#include <kernel/include/init/common.h> // IWYU pragma: export

Fixed as of: 31c21f3644f6f42f6e4c3c231e95bf0b87ab86f5

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?

No longer relevant as of 9bc9309bd61f36ac68b41fe2328d2e3f505397e1

I think it makes sense to move SanityChecks out as well, as it's effectively a runtime test for SetGlobals (and it depends on it too).

Do you mean something like 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 remove compat/glibcxx_sanity.cpp from 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.cpp from 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.

Fixed as of 9ac87854af62d271df2f383d8f2f798c70453448, feel free to reopen if still an issue.

Nice, thanks.

Now that we have the notion of a context, it would be possible to either:

• Make it impossible to create an invalid context (sanity checks called at construction time)
• Allow for "invalid" contexts, which api calls check for.

But those can be done much later when we start actually defining an api.

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?

Fixed as of 9bc9309bd61f36ac68b41fe2328d2e3f505397e1

Though it's quite literal here, keeping with precedent, let's not reintroduce this language: #5727

The errors are already namespaced as part of the enum class anyway, how about just SanityCheckError::ERROR_GLIBCXX ?

Fixed as of 239501f5d0e1df8d36a62f10c4de4e9cdc913a9c

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/.

Fixed in 4fa8d0b11aefa06f548013a35bbcfa9fdbed81ff

//! Context struct holding the kernel library's state, and passed

Fixed in cae459cd9aa0da220da92e11b3a645b9980cdaf5

The variable of type kernel::Context is called context here and kernel inside struct NodeContext. kernel_context, kernel_state or kernel_ctx may be better.

Fixed in 319031b9798f1ed8f0a06e679284be26e0b42c92

Now it is called 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.

Why bother introducing an argument if it is going to be unused?

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.

Going to mark this as resolved, feel free to reopen if not!

The word "error" is redundant in the enum name and in each enum value:

SanityCheckError::ERROR_ECC
           ^^^^^  ^^^^^

Maybe strip the ERROR_ prefix: SanityCheckError::ECC?

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:

• I'm hoping that the SanityChecks move into the kernel::Context creation as a next step.
• I think it may make sense for 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.
• Those changes would leave only 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!

Ah yes, this is much more straightforward imo. +1 for squashing that in.

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).

Btw, I'm loving how this just untangled itself. Great suggestion from @ryanofsky here!

Fixed as of: fa2ee7b12c0c15be8e9807444443b5bd2c91c3e5

Hmm, it is strange to sanity-check a default constructed object:

Context foo;
assert(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.

This should be const ref I think.

Also const ref?

I agree, it would be weird for a sanity-check function to modify the object it is checking.

Heh, this has gotten unwieldy and hard to read imo.

How about (untested)

if (!AppInitBasicSetup(gArgs)) return false;
if (!AppInitParameterInteraction(gArgs, /*use_syscall_sandbox=*/false)) return false;

m_context->kernel = std::make_unique<kernel::Context>();
if (!AppInitSanityChecks(*m_context->kernel)) return false;
if (!AppInitLockDataDirectory()) return false;
if (!AppInitInterfaces(*m_context)) return false;
return true;

default:
   assert(0)

?

Addressed in d87784ac87364fc977bbf9769c8bdb72dea8cbf9, feel free to reopen this convo if not resolved!

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.

I think the primary question here was why there is no default case, to which the answer is that we happily accept a fallthrough to an InitError. No objection to a macro that just calls 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.

A foot-gun-proof way to do this would be to inherit from a class that holds a node::NodeContext.

Something like this?

diff --git a/src/node/context.h b/src/node/context.h
index 31be308787..7ed1292180 100644
--- a/src/node/context.h
+++ b/src/node/context.h
@@ -29,6 +29,16 @@ class Init;
 class WalletLoader;
 } // namespace interfaces
 
+namespace {
+/**
+ * Dummy NodeContext to ensure that kernel::Context is always destructed last.
+ */
+struct BaseNodeContext {
+    //! libbitcoin_kernel context
+    std::unique_ptr<kernel::Context> kernel;
+};
+}
+
 namespace node {
 //! NodeContext struct containing references to chain state and connection
 //! state.
@@ -40,9 +50,7 @@ namespace node {
 //! and make code more modular and testable. The struct isn't intended to have
 //! any member functions. It should just be a collection of references that can
 //! be used without pulling in unwanted dependencies or functionality.
-struct NodeContext {
-    //! libbitcoin_kernel context
-    std::unique_ptr<kernel::Context> kernel;
+struct NodeContext : BaseNodeContext {
     //! Init interface for initializing current process and connecting to other processes.
     interfaces::Init* init{nullptr};
     std::unique_ptr<AddrMan> addrman;
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 5c31cfc22b..87d5d4a24b 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -77,12 +77,20 @@ static inline bool InsecureRandBool() { return g_insecure_rand_ctx.randbool(); }
 
 static constexpr CAmount CENT{1000000};
 
+namespace {
+/**
+ * Dummy TestingSetup to ensure that node::NodeContext (and by extension
+ * kernel::Context) is always destructed last.
+ */
+struct BaseTestingSetup {
+    node::NodeContext m_node;
+};
+}
+
 /** Basic testing setup.
  * This just configures logging, data dir and chain parameters.
  */
-struct BasicTestingSetup {
-    node::NodeContext m_node; // keep as first member to be destructed last
-
+struct BasicTestingSetup : BaseTestingSetup {
     explicit BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::vector<const char*>& extra_args = {});
     ~BasicTestingSetup();
 

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:

BasicTestingSetup : 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.

I do agree relationship should be explicit if one thing depends on the other. Another safeguard that may be useful is if thing A needs to be destroyed after thing B, thing B destructor can assert that thing A is not destroyed yet (assuming relevant code will already have test coverage since it is part of the test setup)

Okay, sounds like we're okay with leaving it as just a comment like in the latest push d87784ac87364fc977bbf9769c8bdb72dea8cbf9. Going to close this convo but feel free to re-open if further discussion is worth it.

unrelated: Probably a good time to get rid of goto instead of adding comments on why goto can't be used here. :sweat_smile:

fed085a1a4cd2787202752b6a0d98e42dce97f09 nit: Why not change to make_unique while changing the line?

This code no-longer exists. Was removed in the most recent secp256k1 update (#26691).

fed085a1a4cd2787202752b6a0d98e42dce97f09: Looks like 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?

nit: Could have added new files to be checked by iwyu?

diff --git a/ci/test/06_script_b.sh b/ci/test/06_script_b.sh
index e64af2ad5..f17d823ee 100755
--- a/ci/test/06_script_b.sh
+++ b/ci/test/06_script_b.sh
@@ -43,2 +41,4 @@ if [ "${RUN_TIDY}" = "true" ]; then
           " src/init"\
+          " src/kernel/checks.cpp"\
+          " src/kernel/context.cpp"\
           " src/rpc/fees.cpp"\

All of src/kernel is now under IWYU.

src/kernel/context.h:15: libary ==> library

being done in #25307