kernel: Rm ShutdownRequested and AbortNode from validation code. #27861

TheCharlatan · 2023-06-12T13:16:33Z

Get rid of all ShutdownRequested calls in validation code by introducing an interrupt object that applications can use to cancel long-running kernel operations.

Replace all AbortNode calls in validation code with new fatal error and flush error notifications so kernel applications can be notified about failures and choose how to handle them.

This pull request is part of the libbitcoinkernel project #27587 The libbitcoinkernel Project and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".

The pull request mostly allows dropping the kernel dependency on shutdown.cpp. The only dependency left after this is a StartShutdown call which will be removed in followup PR #27711. This PR also drops the last reference to the uiInterface global in kernel code. The process of moving the uiInterface out of the kernel was started in #27636.

This pull request contains a subset of patches originally proposed in #27711. It will be part of a series of changes required to make handling of interrupts (or in other words the current shutdown procedure) in the kernel library more transparent and less reliable on global mutable state. The set of patches contained here was originally proposed by @ryanofsky here.

DrahtBot · 2023-06-12T13:16:37Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, ryanofsky, achow101

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:

#27866 (blockstorage: Return on fatal flush errors by TheCharlatan)
#27850 (test: Add unit & functional test coverage for blockstore by pinheadmz)
#27823 (init: return error when block index is non-contiguous, fix feature_init.py file perturbation by mzumsande)
#27746 (Rework validation logic for assumeutxo by sdaftuar)
#27460 (rpc: Add importmempool RPC by MarcoFalke)
#26966 (index: blockfilter initial sync speedup, parallelize process by furszy)
#26762 (Make CCheckQueue RAII-styled by hebasto)
#26045 (rpc: Optimize serialization disk space of dumptxoutset by aureleoules)

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.

ryanofsky

Code review ACK d60ab3b. Overall looks great! Feel free to ignore most of comments I left. The only thing I think really should be addressed is the two BlockManager::Flush calls aborting without returning errors. I think this is confusing and potentially could lead to bugs, but since the changes here don't actually affect the way the code run in practice, it doesn't need to block the PR.

src/shutdown.cpp

src/util/signalinterrupt.h

src/util/signalinterrupt.cpp

src/kernel/blockmanager_opts.h

ryanofsky · 2023-06-12T15:24:52Z

src/node/blockstorage.cpp

@@ -529,7 +529,7 @@ void BlockManager::FlushUndoFile(int block_file, bool finalize)
 {
    FlatFilePos undo_pos_old(block_file, m_blockfile_info[block_file].nUndoSize);
    if (!UndoFileSeq().Flush(undo_pos_old, finalize)) {
-        AbortNode("Flushing undo file to disk failed. This is likely the result of an I/O error.");
+        m_opts.notifications.fatalError("Flushing undo file to disk failed. This is likely the result of an I/O error.");


In commit "kernel: Add fatalError method to notifications" (d60ab3b)

This is calling fatalError and then continuing without returning an error or raising an exception, so I think the new code here is more confusing than the current code (even if strictly speaking it is a refactoring).

We could potentially change this code to treat failing flush as a normal error that gets returned to the caller. But I think if we are going to do that, it would be better to do in a separate PR which could receive targeted review without needing reviewers to look at all the other changes here.

For this PR, I would suggest doing one of two things:

Adding a kernel::Notifications::flushError method and calling flushError here instead of afatalError. The current code is making this AbortNode call in a different way than other AbortNode calls, and it would be confusing for the kernel notification interface to hide differences between errors that are not handled the same. A flushError method could have a comment like "The flush error notification is sent when there is an I/O error saving state in memory to disk. It is recommended that applications treat this a as a fatal error, even though it is technically less severe, and the kernel may do some work after it happens."

Reverting the change to this line and keeping the AbortNode function for now. This would get most of the work in the PR done, just leaving behind leaving 2 AbortNode calls that can be cleaned up with more care later.

ryanofsky · 2023-06-12T15:25:37Z

src/node/blockstorage.cpp

-        AbortNode("Flushing block file to disk failed. This is likely the result of an I/O error.");
+        m_opts.notifications.fatalError("Flushing block file to disk failed. This is likely the result of an I/O error.");


In commit "kernel: Add fatalError method to notifications" (d60ab3b)

This is the other misleading fatalError call (see comment above)

src/node/chainstate.cpp

src/validation.cpp

TheCharlatan · 2023-06-12T17:24:09Z

The only thing I think really should be addressed is the two BlockManager::Flush calls aborting without returning errors.

I was about to open a separate PR from this branch here: https://github.com/TheCharlatan/bitcoin/tree/kernelReturnOnAbort . I think this should go in a separate PR for ease of review. I'll also provide a list of all the other non-immediate returns that should be fine to do.

ryanofsky · 2023-06-12T18:35:09Z

The only thing I think really should be addressed is the two BlockManager::Flush calls aborting without returning errors.

I was about to open a separate PR from this branch here: https://github.com/TheCharlatan/bitcoin/tree/kernelReturnOnAbort

Great! I also opened #27862 earlier which overlaps with the branch a little. I think between the branch and PR all the cases where errors are not currently returned are covered.

TheCharlatan · 2023-06-13T11:03:54Z

Rebased d60ab3b -> 3e60c02 (kernelInterrupt_0 -> kernelInterrupt_1, compare).

Added src/node/abort.* to allow both the notification code and index/base.cpp to use a common abort definition as introduced in 3c06926
De-globalized the exit_status in shutdown.*. My current approach touches quite a few lines though and seems a bit unclean, suggestions for alternative approaches are appreciated.

Updated 3e60c02 -> 39d9b51 (kernelInterrupt_1 -> kernelInterrupt_2, compare).

Addressed @ryanofsky's comment, cleaning up includes.
Addressed @ryanofsky's comment, renaming sleep to wait.
Addressed @ryanofsky's comments (1, 2, 3), avoiding behavior changes around atomic memory acquisition and release.
Addressed @ryanofsky's comment, removing a field that was accidentally carried over from the parent PR.

ryanofsky

Code review ACK 39d9b51. I left a suggestion to reduce size of changes to index code, and I still think it is inappropriate to treat flush errors as fatal errors, and would prefer to add a separate flush error method (#27861 (comment)), but I won't get hung up on this.

src/index/base.cpp

ryanofsky · 2023-06-13T17:27:15Z

re: #27861 (comment)

De-globalized the exit_status in shutdown.*. My current approach touches quite a few lines though and seems a bit unclean, suggestions for alternative approaches are appreciated.

I'm actually not sure what this is referring to. exit_status is not referenced in shutdown.cpp at all after this PR: https://github.com/TheCharlatan/bitcoin/blob/39d9b5144410c15385ee48bc886e0dea36f34b6a/src/shutdown.cpp.

Also, after this PR shutdown.cpp is just a thin wrapper around the kernel::Context::interrupt object, so we could pretty easily delete shutdown.cpp entirely and update callers to use the object directly.

TheCharlatan · 2023-06-13T18:34:28Z

Re #27861 (review)

and I still think it is inappropriate to treat flush errors as fatal errors, and would prefer to add a separate flush error method (#27861 (comment)), but I won't get hung up on this.

ACK, I'll revert to using AbortNode then for now, that should keep any potential churn minimal. Once #27866 has received sufficient review, we can always decide to add special cases (e.g. flush error notifications) afterwards.

TheCharlatan · 2023-06-13T19:01:56Z

Re #27861 (comment)

ACK, I'll revert to using AbortNode then for now, that should keep any potential churn minimal. Once #27866 has received sufficient review, we can always decide to add special cases (e.g. flush error notifications) afterwards.

Ugh, just noticed this gets annoying with the exit status - we probably don't want to introduce that in the blockstorage scope. Leaving as is then :/.

TheCharlatan · 2023-06-13T20:30:32Z

Updated 39d9b51 -> 315915b (kernelInterrupt_2 -> kernelInterrupt_3, compare).

Adopted @ryanofsky's suggestion, making FatalErrorf a member function.

src/Makefile.am

src/util/threadinterrupt.h

This change is mostly a refectoring that removes some code and gets rid of an unnecessary layer of indirection after bitcoin#27861 But it is not a pure refactoring since StartShutdown, AbortShutdown, and WaitForShutdown functions used to abort on failure, and the replacement code logs or returns errors instead.

Replace exceptions thrown by signal and wait methods with [[nodiscard]] return values. This is mostly a refactoring, but there is a slight change of behavior if AbortShutdown function fails. The original behavior which was unintentionally changed in bitcoin#27861 is restored, so it now triggers an assert failure again instead of throwing an exception. (The AbortShutdown function is only ever called in the the GUI version of Bitcoin Core when corruption is detected on loading and the user tries to reindex.) Problems with using exceptions were pointed out by MarcoFalke in bitcoin#27861 (comment).