The libbitcoinkernel project is an effort to decouple Bitcoin Core’s consensus engine from other non-consensus modules (such as the various indices) in the codebase. We have previously covered libbitcoinkernel-related PRs #25527, #24410 and #20158.
One such type of event is the consensus engine requiring a shutdown, expectedly or unexpectedly.
This PR #27711 adds two new notification methods kernel::Notifications::startShutdown and kernel::Notifications::fatalError to allow the node to implement the necessary behaviour.
Moreover, this PR moves the shutdown files as well as the remaining usages of uiInterface out of the kernel code, as started in #27636.
<stickies-v> welcome everyone! Today we're looking at #27711, authored by TheCharlatan. The notes and questions are available on https://bitcoincore.reviews/27711
<stickies-v> LarryRuane: GitHub allows you to browse a repo at a certain commit (like checkout), and if you do that and then click the commit history button, you can see the history indeed
<stickies-v> the PR turned out to be a bit more complex than I imagined when I first selected it hah, as also evidenced by the recent discussions and today's force-push
<LarryRuane> mutex: yes i of course use `git` constantly, but to interact with github, there's a `gh` command you can install, which i did, but i haven't used it yet
<stickies-v> I see there's not been too much review, so perhaps we can start if anyone has any questions about the purpose of this PR? does the approach make sense?
<stickies-v> LarryRuane: oh, good question. I don't think that's quite in the scope of the libbitcoinkernel project, since `fRequestShutdown` is owned by the node
<stickies-v> I think the version of the PR that we're reviewing doesn't really make any progress towards that goal either, since we're still very much relying on a global boolean value
<stickies-v> mutex: yeah, there was quite a big code overhaul this morning. we do include the HEAD that we look at on the bitcoincore.reviews website, but I now see that it only is mentioned for reviews in the past, not for current/upcoming ones
<stickies-v> LarryRuane: I think `KernelNotifications notifications` and `KernelNotifications notifications{}` are equivalent, since `KernelNotifications` is a class with a default constructor
<evansmj> `As a standalone library the libbitcoinkernel should not have to rely on code that accesses shutdown state through a global.` was `static std::atomic<bool> fRequestShutdown(false);` the original global? the state being just true/false?
<stickies-v> "...as more and more modules are decoupled from the consensus engine, this list will shrink to only those which are absolutely necessary."
<mutex> So the PR changes the signaling method, does the behavior change at all? I see a few exceptions being thrown, does that change the behavior other than to stop hashing?
<LarryRuane> so the first two commits (in the review club version, not the latest PR tip) are removing calls to the global `ShutdownRequested()` function from those files (that make up libbitcoinkernel), IIUC
<evansmj> so now, classes interested in the shutdown state need to look at the kernel Context notifications for the signal interrupt flag. what kinds of things/classes are interested in this? i see bitcoind checks it immediately in AppInit(), is that somehow the main looping check?
<LarryRuane> I assume one of the goals of a PR like this is making consensus code more testable? So that last commit, "kernel: Remove shutdown from kernel library" changes all the calls (in libbitcoinkernel) to the global `StartShutdown()`, to instead call a notification callback, for example `chainman.GetNotifications().startShutdown(ShutdownReason::StopAfterBlockImport)`
<LarryRuane> test code wouldn't really get shut down, it might check that this callback did happen (like presumably it's supposed to), and fail the test if not .. something like that
<stickies-v> LarryRuane: yeah I think that's definitely true, and probably holds for pretty much all decoupling? the less intertwined everything is, the more straightforward your tests can be