kernel: Run sanity checks on context construction

TheCharlatan commented at 7:54 am on August 7, 2023: contributor

Moving the kernel sanity check code to kernel context instantiation time ensures that it is impossible to create an invalid kernel context. This is based on a suggestion made in a comment by @theuni in the original pull request first introducing the kernel::Context.

DrahtBot commented at 7:54 am on August 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
Stale ACK	stickies-v

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:

#29252 (kernel: Remove key module from kernel library by TheCharlatan)
#28960 (kernel: Remove dependency on CScheduler by TheCharlatan)

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.

DrahtBot added the label Validation on Aug 7, 2023

in src/kernel/context.cpp:14 in fb248a88d4 outdated

 9 #include <key.h>
10 #include <logging.h>
11 #include <pubkey.h>
12 #include <random.h>
13+#include <util/result.h>
14+#include <util/time.h>

furszy commented at 11:54 pm on August 10, 2023:

is this time.h include needed?

in src/bitcoind.cpp:190 in fb248a88d4 outdated

188-        if (!AppInitSanityChecks(*node.kernel))
189-        {
190+        auto result{kernel::Context::MakeContext()};
191+        if (!result) {
192+            InitError(util::ErrorString(result));
193+            return InitError(strprintf(_("Initialization kernel sanity check failed. %s is shutting down."), PACKAGE_NAME));

furszy commented at 2:53 am on August 11, 2023:

This will trigger two error dialogs in the GUI, one after the other. Maybe better to concatenate the errors and call InitError() only once?

in src/kernel/context.cpp:32 in fb248a88d4 outdated

30     LogPrintf("Using the '%s' SHA256 implementation\n", sha256_algo);
31     RandomInit();
32     ECC_Start();
33+
34+    if (auto res{SanityChecks(*context)}; !res) {
35+        LogPrintf("Kernel sanity check failed: %s\n", ErrorString(res).original);

furszy commented at 2:54 am on August 11, 2023:

Isn’t this logged in the caller as well?

TheCharlatan commented at 7:43 am on August 11, 2023:

Yeah, I’ll remove this line.

TheCharlatan force-pushed on Aug 11, 2023

TheCharlatan commented at 9:32 am on August 11, 2023: contributor

Thank you for the review @furszy

Updated cfe93642b27cbd4770200b9a6e571e1388ba35dc -> 3c1c434ae0f1e9649949a1bb27ea14c1dbad06cc (contextSanityChecks_0 -> contextSanityChecks_1, compare)

Addressed @furszy’s comment, cleaned up includes of context.cpp/.h.
Addressed @furszy’s comment, squashed the two log messages into one message. Also made sure that both the daemon and gui are still logging the same message.
Addressed @furszy’s comment, removed failure log statement in MakeContext.

theuni commented at 1:48 pm on August 11, 2023: member

Assuming I’m understanding the init order bug, please correct me if I’m not… If AppInitBasicSetup() and AppInitBasicSetup() and AppInitParameterInteraction() are relying on globals created by the kernel, perhaps it makes sense for them to require a kernel to be passed in, even if it’s unused inside those functions for now?

It would document/force the correct creation order
Presumably when de-globalizing, these functions will need the kernel anyway.

TheCharlatan commented at 3:54 pm on August 11, 2023: contributor

Re #28228 (comment)

Presumably when de-globalizing, these functions will need the kernel anyway.

I am not sure how we could go about registering our signal handlers without the shutdown global.

It would document/force the correct creation order

I originally wanted to to introduce this in a separate PR (fixing the order for gui initialization), but I think it makes more sense here, yes.

TheCharlatan marked this as a draft on Aug 11, 2023

TheCharlatan force-pushed on Aug 12, 2023

TheCharlatan commented at 8:53 am on August 12, 2023: contributor

Thank you for the comments @theuni

Updated 3c1c434ae0f1e9649949a1bb27ea14c1dbad06cc -> 8275733e192baf81916bf8e218f5ddb24c5ddb74 (contextSanityChecks_1 -> contextSanityChecks_2, compare)

Added check and precondition docs for the global kernel context.
Fixed initialization order for bitcoin-qt in baseInitialize

TheCharlatan marked this as ready for review on Aug 12, 2023

TheCharlatan force-pushed on Aug 12, 2023

DrahtBot added the label CI failed on Sep 4, 2023

in src/kernel/context.cpp:20 in 8275733e19 outdated

19 
20 namespace kernel {
21 Context* g_context;
22 
23-Context::Context()
24+util::Result<std::unique_ptr<Context>> Context::MakeContext()

stickies-v commented at 5:43 pm on September 4, 2023:

nit: missing #include <memory>

TheCharlatan commented at 6:32 am on September 5, 2023:

It’s already in the header.

stickies-v commented at 8:01 am on September 5, 2023:

“Every .cpp and .h file should #include every header file it directly uses”. It’s a nit, so only if you have to force push?

in src/kernel/context.cpp:24 in 8275733e19 outdated

21 Context* g_context;
22 
23-Context::Context()
24+util::Result<std::unique_ptr<Context>> Context::MakeContext()
25 {
26     assert(!g_context);

stickies-v commented at 5:45 pm on September 4, 2023:

Since we’re now returning a Result, how about replacing this assertion with returning an Error?

TheCharlatan commented at 6:38 am on September 5, 2023:

The global context is removed in #28051, so not sure this is worth it.

stickies-v commented at 8:01 am on September 5, 2023:

Ah, wasn’t aware. Makes sense, resolved.

in src/node/interfaces.cpp:102 in 8275733e19 outdated

 97+        if (!res) {
 98+            return InitError(strprintf(_("Initialization kernel sanity check failed: %s. %s is shutting down."),
 99+                                       util::ErrorString(res), PACKAGE_NAME));
100+        }
101+        m_context->kernel = std::move(res.value());
102+

stickies-v commented at 6:01 pm on September 4, 2023:

Could implement a NodeContext::make_kernel() method to avoid this duplication between here and bitcoind.cpp?

TheCharlatan commented at 8:14 am on September 5, 2023:

I remember some prior comments that adding helper methods directly to the NodeContext struct is undesirable. Could add a init.h helper function instead that takes a NodeContext as an argument?

stickies-v commented at 6:43 pm on September 5, 2023:

Ah, it’s in the docstring.

I think a non-member function in init makes sense, yeah. It aligns well with the existing design of baseInitialize and AppInit.

stickies-v commented at 6:30 pm on September 4, 2023: contributor

Approach ACK

DrahtBot removed the label CI failed on Sep 5, 2023

TheCharlatan force-pushed on Sep 6, 2023

TheCharlatan commented at 7:04 am on September 6, 2023: contributor

Thank you for the comments @stickies-v

Rebased 8275733e192baf81916bf8e218f5ddb24c5ddb74 -> 14448df7241e449af454754ec412d2f6aadc874f (contextSanityChecks_2 -> contextSanityChecks_3, compare)

Dropped the second commit in favor of #28051

Updated 14448df7241e449af454754ec412d2f6aadc874f -> d0daf3edf400f0400f827ac4a8e7ed7398826c1a (contextSanityChecks_3 -> contextSanityChecks_4, compare)

Addressed @stickies-v’s comment, adding a helper function in init.* to create the kernel context.
Addressed @stickies-v’s comment, including missing header.

in src/node/interfaces.cpp:101 in d0daf3edf4 outdated

 96@@ -97,8 +97,9 @@ class NodeImpl : public Node
 97         if (!AppInitBasicSetup(args(), Assert(context())->exit_status)) return false;
 98         if (!AppInitParameterInteraction(args())) return false;
 99 
100-        m_context->kernel = std::make_unique<kernel::Context>();
101-        if (!AppInitSanityChecks(*m_context->kernel)) return false;
102+        if (!InitKernel(*m_context)) return false;
103+

stickies-v commented at 9:53 am on September 6, 2023:

supernit: would be okay removing all these whitelines? not sure there’s a meaningful meaning behind breaking these up in blocks?

in src/kernel/context.h:24 in d0daf3edf4 outdated

24@@ -24,8 +25,11 @@ struct Context {
25     //! Declare default constructor and destructor that are not inline, so code
26     //! instantiating the kernel::Context struct doesn't need to #include class
27     //! definitions for all the unique_ptr members.
28-    Context();
29+    static util::Result<std::unique_ptr<Context>> MakeContext();

stickies-v commented at 10:07 am on September 6, 2023:

nit: as the above comment applies to all struct methods, how about generalizing the docstring?

 0diff --git a/src/kernel/context.h b/src/kernel/context.h
 1index 84fdcae800..9712ed6018 100644
 2--- a/src/kernel/context.h
 3+++ b/src/kernel/context.h
 4@@ -18,13 +18,13 @@ namespace kernel {
 5 //!
 6 //! State stored directly in this struct should be simple. More complex state
 7 //! should be stored to std::unique_ptr members pointing to opaque types.
 8+//!
 9+//! Methods should not be inline, so code instantiating the kernel::Context struct
10+//! doesn't need to #include class definitions for all the unique_ptr members.
11 struct Context {
12     //! Interrupt object that can be used to stop long-running kernel operations.
13     util::SignalInterrupt interrupt;
14 
15-    //! Declare default constructor and destructor that are not inline, so code
16-    //! instantiating the kernel::Context struct doesn't need to #include class
17-    //! definitions for all the unique_ptr members.
18     static util::Result<std::unique_ptr<Context>> MakeContext();
19     ~Context();
20

stickies-v approved

stickies-v commented at 10:36 am on September 6, 2023: contributor

ACK d0daf3edf400f0400f827ac4a8e7ed7398826c1a

PR description needs updating to reflect dropping the bugfix
~nit: commit message mentions deduplicating the init error messages, but that’s not entirely true since bitcoind.cpp didn’t have an init error message to begin with, this change is relative to a previous version of this PR but not to master, I think?~ edit: resolved as clarified here

TheCharlatan commented at 10:44 am on September 6, 2023: contributor

Re #28228#pullrequestreview-1612950908

nit: commit message mentions deduplicating the init error messages, but that’s not entirely true since bitcoind.cpp didn’t have an init error message to begin with, this change is relative to a previous version of this PR but not to master, I think?

This refers to these two messages here: https://github.com/bitcoin/bitcoin/pull/28228/files#diff-b1e19192258d83199d8adaa5ac31f067af98f63554bfdd679bd8e8073815e69dL1034-L1035 that have been merged into a single one.

TheCharlatan force-pushed on Sep 6, 2023

TheCharlatan commented at 10:54 am on September 6, 2023: contributor

Updated d0daf3edf400f0400f827ac4a8e7ed7398826c1a -> 53f0be73cc816f39e0131f5809bf14b43fee094a (contextSanityChecks_4 -> contextSanityChecks_5, compare)

Addressed @stickies-v’s comment, improving/correcting docstring.
Addressed @stickies-v’s comment, improved whitespace spacing.
Addressed @stickies-v’s comment, improved commit message a bit.

Also fixed-up PR description.

stickies-v commented at 10:59 am on September 6, 2023: contributor

re-ACK 53f0be73cc816f39e0131f5809bf14b43fee094a

ryanofsky commented at 6:43 pm on September 11, 2023: contributor

Looking at 53f0be73cc816f39e0131f5809bf14b43fee094a, I don’t understand the benefits of this PR. It seems unfortunate for the kernel struct now to have to live in a unique_ptr instead of kernel library users being able to declare and allocate it however they want. It seems unfortunate for the context struct now to have a static method, when it’s intended to be passive container holding state, not a place where functionality is implemented. It seems unfortunate for it now to depend on checks.cpp and do extra work that’s not strictly necessary. I know there’s history and discussion here that I’m not aware of but the current change by itself does seem like a regression.

DrahtBot added the label CI failed on Oct 23, 2023

maflcko commented at 9:15 am on October 25, 2023: member

Are you still working on this?

TheCharlatan commented at 9:22 am on October 25, 2023: contributor

Re #28228 (comment)

Are you still working on this?

I would like to hear @theuni’s opinion here, since he originally suggested the concept.

DrahtBot removed the label CI failed on Oct 28, 2023

DrahtBot added the label Needs rebase on Dec 14, 2023

kernel: Move sanity check to context constructor

This ensures that it is impossible to construct a context that does not
pass the sanity checks.

Also de-duplicate the init error messages in case of a kernel context
sanity check error.

673bcf4d25

TheCharlatan force-pushed on Jan 10, 2024

TheCharlatan commented at 10:10 am on January 10, 2024: contributor

Rebased 53f0be73cc816f39e0131f5809bf14b43fee094a -> 673bcf4d25572bc49ff25e06515afe6c2e776537 (contextSanityChecks_5 -> contextSanityChecks_6, compare)

Fixed conflict with #28051

DrahtBot removed the label Needs rebase on Jan 10, 2024

DrahtBot added the label CI failed on Jan 12, 2024

DrahtBot added the label Needs rebase on Mar 9, 2024

DrahtBot commented at 2:14 am on March 9, 2024: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

achow101 requested review from theuni on Apr 9, 2024

in src/bitcoin-chainstate.cpp:30 in 673bcf4d25

26@@ -27,6 +27,7 @@
27 #include <script/sigcache.h>
28 #include <util/chaintype.h>
29 #include <util/fs.h>
30+#include <util/signalinterrupt.h>

Manuela123q commented at 7:31 pm on June 11, 2024:

Hhfft

Manuela123q changes_requested

TheCharlatan commented at 11:38 am on September 2, 2024: contributor

Closing this again, having worked with the code some more over the past year, I don’t think this is particularly pressing change anymore. As ryanofsky elaborated, our context objects should have functionality and instead just hold state - which is what it does already.

TheCharlatan closed this on Sep 2, 2024

kernel: Run sanity checks on context construction #28228

Code Coverage

Reviews

Conflicts