kernel: Run sanity checks on context construction #28228

sedited · 2023-08-07T07:54:41Z

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 · 2023-08-07T07:54:44Z

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.

src/kernel/context.cpp

src/bitcoind.cpp

src/kernel/context.cpp

sedited · 2023-08-11T09:32:12Z

Thank you for the review @furszy

Updated cfe9364 -> 3c1c434 (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 · 2023-08-11T13:48:58Z

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.

sedited · 2023-08-11T15:54:41Z

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.

sedited · 2023-08-12T08:53:37Z

Thank you for the comments @theuni

Updated 3c1c434 -> 8275733 (contextSanityChecks_1 -> contextSanityChecks_2, compare)

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

stickies-v

Approach ACK

src/kernel/context.cpp

stickies-v · 2023-09-04T18:01:31Z

src/node/interfaces.cpp

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


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

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?

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.

sedited · 2023-09-06T07:04:31Z

Thank you for the comments @stickies-v

Rebased 8275733 -> 14448df (contextSanityChecks_2 -> contextSanityChecks_3, compare)

Dropped the second commit in favor of Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly #28051

Updated 14448df -> d0daf3e (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.

stickies-v

ACK d0daf3e

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

src/node/interfaces.cpp

src/kernel/context.h

sedited · 2023-09-06T10:44:13Z

Re #28228 (review)

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.

sedited · 2023-09-06T10:54:56Z

Updated d0daf3e -> 53f0be7 (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 · 2023-09-06T10:59:42Z

re-ACK 53f0be7

ryanofsky · 2023-09-11T18:43:59Z

Looking at 53f0be7, 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.

maflcko · 2023-10-25T09:15:03Z

Are you still working on this?

sedited · 2023-10-25T09:22:12Z

Re #28228 (comment)

Are you still working on this?

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

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.

sedited · 2024-01-10T10:10:09Z

Rebased 53f0be7 -> 673bcf4 (contextSanityChecks_5 -> contextSanityChecks_6, compare)

Fixed conflict with Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly #28051

DrahtBot · 2024-03-09T02:14:16Z

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

Manuela123q · 2024-06-11T19:31:51Z

src/bitcoin-chainstate.cpp

 #include <script/sigcache.h>
 #include <util/chaintype.h>
 #include <util/fs.h>
+#include <util/signalinterrupt.h>


sedited · 2024-09-02T11:38:25Z

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.

DrahtBot added the Validation label Aug 7, 2023

furszy reviewed Aug 11, 2023

View reviewed changes

src/kernel/context.cpp Outdated Show resolved Hide resolved

src/bitcoind.cpp Outdated Show resolved Hide resolved

src/kernel/context.cpp Outdated Show resolved Hide resolved

sedited force-pushed the contextSanityChecks branch from cfe9364 to 3c1c434 Compare August 11, 2023 09:31

sedited marked this pull request as draft August 11, 2023 15:54

sedited force-pushed the contextSanityChecks branch from 3c1c434 to b333faa Compare August 12, 2023 08:46

sedited marked this pull request as ready for review August 12, 2023 08:53

sedited force-pushed the contextSanityChecks branch from b333faa to 8275733 Compare August 12, 2023 09:01

DrahtBot mentioned this pull request Aug 22, 2023

Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly #28051

Merged

DrahtBot added the CI failed label Sep 4, 2023

stickies-v reviewed Sep 4, 2023

View reviewed changes

DrahtBot removed the CI failed label Sep 5, 2023

sedited force-pushed the contextSanityChecks branch from 8275733 to d0daf3e Compare September 6, 2023 07:04

stickies-v approved these changes Sep 6, 2023

View reviewed changes

src/node/interfaces.cpp Outdated Show resolved Hide resolved

src/kernel/context.h Show resolved Hide resolved

sedited force-pushed the contextSanityChecks branch from d0daf3e to 53f0be7 Compare September 6, 2023 10:52

DrahtBot added the CI failed label Oct 23, 2023

DrahtBot removed the CI failed label Oct 28, 2023

DrahtBot mentioned this pull request Nov 28, 2023

kernel: Remove dependency on CScheduler #28960

Merged

DrahtBot added the Needs rebase label Dec 14, 2023

kernel: Move sanity check to context constructor

673bcf4

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.

sedited force-pushed the contextSanityChecks branch from 53f0be7 to 673bcf4 Compare January 10, 2024 10:09

DrahtBot added CI failed and removed Needs rebase labels Jan 10, 2024

DrahtBot mentioned this pull request Jan 15, 2024

kernel: Remove key module from kernel library #29252

Merged

DrahtBot added the Needs rebase label Mar 9, 2024

achow101 requested a review from theuni April 9, 2024 14:22

Manuela123q suggested changes Jun 11, 2024

View reviewed changes

sedited closed this Sep 2, 2024

bitcoin locked and limited conversation to collaborators Sep 2, 2025

kernel: Run sanity checks on context construction #28228

kernel: Run sanity checks on context construction #28228

Uh oh!

Conversation

sedited commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sedited commented Aug 11, 2023

Uh oh!

theuni commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sedited commented Aug 11, 2023

Uh oh!

sedited commented Aug 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stickies-v Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

sedited Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

stickies-v Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

sedited commented Sep 6, 2023

Uh oh!

stickies-v left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sedited commented Sep 6, 2023

Uh oh!

sedited commented Sep 6, 2023

Uh oh!

stickies-v commented Sep 6, 2023

Uh oh!

ryanofsky commented Sep 11, 2023

Uh oh!

maflcko commented Oct 25, 2023

Uh oh!

sedited commented Oct 25, 2023

Uh oh!

sedited commented Jan 10, 2024

Uh oh!

DrahtBot commented Mar 9, 2024

Uh oh!

Manuela123q Jun 11, 2024

Choose a reason for hiding this comment

Uh oh!

sedited commented Sep 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sedited commented Aug 7, 2023 •

edited

Loading

DrahtBot commented Aug 7, 2023 •

edited

Loading

theuni commented Aug 11, 2023 •

edited

Loading

sedited commented Aug 12, 2023 •

edited

Loading

stickies-v left a comment •

edited

Loading