kernel: Use typed errors for fatal and flush error notifications #36326

arejula27 commented at 12:46 PM on September 24, 2026: contributor

Part of the libbitcoinkernel project (tracking issue #27587 ), specifically the "Remove translation.h usage from the kernel" item. kernel::Notifications::fatalError() and flushError() take a bilingual_str, which pulls util/translation.h and the application-level G_TRANSLATION_FUN into the kernel library, and translation is an application concern rather than a consensus one. The same interface already shows the way out: warningSet() takes a kernel::Warning enum, and the node turns it into a translated string in node/kernel_notifications.cpp.

Along with removing the dependency, another goal here is separating responsibilities, because the goals of error handling and error reporting are different: presenting an error to a user is an application concern, not something libbitcoinkernel itself should own. The error is structured on both sides: in C++ as the full kernel::FatalError variant, and in the C API as its numeric value. With the current changes the C side only carries half of that structure today though, the dynamic fields (a block hash, a path, an exception's what()) do not cross the C API yet, even though ideally they should.

This PR does the same for the two error notifications. A new src/kernel/error.h defines kernel::FlushError as a plain enum and kernel::FatalError as a std::variant with one struct per error, whose fields carry the context the message needs: a block hash, a path, the what() of a caught exception, block heights. It holds types and nothing else, so every message is now built on the node side. The exception is FatalErrorString() in validation.cpp, because a fatal error raised during validation also fails the block through BlockValidationState::Error(), which takes the reason as a plain string right there, and fatalError() returns void, so the node has no way to put its message into that state. A follow-up could store the typed error in the state instead and let the node create the string message.

This does not remove the kernel's dependency on util/translation.h, it is just a step. The other two notifications, progress() and warningSet(), still take a bilingual_str, and bitcoinkernel.cpp, checks.cpp, node/chainstate.cpp, txmempool.cpp and validation.cpp still include the header.

There is one behavior change to mention here. The two C API callbacks lose their message parameter, so they now carry only the error code, the same pattern that btck_BlockValidationResult already uses.

kernel: Add FatalError and FlushError types 949bda0e85

kernel: Use FlushError enum in Notifications::flushError

Translation is an application concern, so the node builds the message.
The C API callback takes the error code alone.

8a16909208

kernel: Use FatalError variant in Notifications::fatalError

Same as the previous commit, with each error's dynamic context carried as
typed fields. std::visit keeps the node's message table exhaustive.

node/blockstorage.cpp no longer needs util/translation.h.

1ff7871846

node: Add KernelNotifications::abort()

Moves the m_shutdown_on_fatal_error check into a single place, so that node
code aborting without a kernel error can reuse it.

e4a9152fd1

kernel: Drop FailedToStartIndexes from the fatal error set

init.cpp is not part of libbitcoinkernel, so no consumer of the library
could ever be notified of this error; it now aborts through AbortNode()
directly.

c96a4016fd

kernel: Type the snapshot coins db rename failure

It was forwarded as a plain string, so the node could only re-emit it
untranslated; InvalidateCoinsDBOnDisk() now returns the two paths and the
node builds the message, which also takes the last _() out of the kernel's
fatal error paths.

1d66b88dcd

DrahtBot added the label Validation on Sep 24, 2026

DrahtBot commented at 12:46 PM on September 24, 2026: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36326.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35731 (Indexes: Harden the flush-error notification invariant by arejula27)
#35646 (RFC: Separate out runtime errors from BlockValidationState using util::Expected by yuvicc)
#35641 (kernel: Add script evaluation tracer by sedited)
#35570 (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)
#35187 (kernel: Block validation without a complete UTXO set by sedited)
#34374 (kernel: use struct-based logging and simplify logging interface by stickies-v)
#33847 (kernel: Attach logging connections to contexts, per-connection log levels by ryanofsky)

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.

sedited commented at 12:59 PM on September 24, 2026: contributor

Concept ACK :)

DrahtBot added the label CI failed on Sep 24, 2026

DrahtBot commented at 1:50 PM on September 24, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/36001208610/job/107638021690</sub> <sub>LLM reason (✨ experimental): CI failed because IWYU detected missing/incorrect includes and generated changes (“Failure generated from IWYU”).</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

ryanofsky commented at 2:50 PM on September 24, 2026: contributor

Remove translation.h usage from the kernel

Interesting. I didn't know this was a goal. Has this been discussed somewhere? Do we want to have no ability for the kernel to show detailed translated messages when something fails in the kernel, like "Bad undo data in block {hash} height {height} file {filename}" (to pick a random example)? Is this because we want (1) all kernel messages to be untranslated? Or (2) all messages that are translated to be represented as structs and enums in the kernel and made into strings by applications? Or some combination of these approaches?

I do think these approaches are reasonable, but they do come with costs. Approach (1) makes harder to write polished applications that are localized and do not require english. Approach (2) is costly to developers and users because it either results in vague messages that do not include important details necessary to understand, or it requires complicated structural representations of exceptional conditions and statuses in code.

IMO, util/translation.h is a low overhead way to provide helpful error and status messages that can be easily translated as needed. I don't see significant benefits to removing it and don't think it should be a goal to remove.

Which is not to say that providing structural error information is not useful at all. Right now we do not provide enough structural error information and should provide more. But it is also possible to take things too far and add too much structure. We should not be creating a situation where calling any kernel API means needing to handle a half-dozen error codes, when a simple error status and a translated string would be easier to handle in code and provide more detailed and helpful information to users.

arejula27 commented at 3:31 PM on September 25, 2026: contributor

@ryanofsky thanks for sharing your opinion!

I don't know if this has been discussed in more depth than the checklist item in #27587 from i took inspiration. @sedited might be able to answer that better. If more discussion is needed, we can keep it here on the PR, or I (or someone else, not sure about the process) can start a thread on the mailing list, whatever works best.

Given the two directions you describe, I was thinking on the 2 when writing the PR.

I think this is less about the dependency itself (despite removing it is a nice win for me), util/translation.h is small, as you say, and more about scope: should the kernel be responsible for producing user-facing output messages at all? The way I see the libbitcoinkernel project (which can be wrong or misunderstood), it is not a module of Core, but a library that uses Core's consensus code and can be used anywhere else, to build any application that needs consensus, not only a node that writes to debug.log. Bark, for example, uses the rust-bitcoinkernel bindings in their test suite (see this tweet), and that kind of consumer is exactly the case where a fixed string is not the right fit. Handing back a string (and I mean any fixed string, not just a bilingual_str) is already an application-level decision (how to log, how to present in a GUI), and the kernel should not be the one making it, it forces the clients to parser the message in case they want to handle it. It also limits the library's own clients: if the kernel ships an already-written message, it pushes the consumer toward the kernel's wording instead of whatever the application wants to say. A structured type does not have that problem, the consumer decides all of it.

On the complexity you are worried about: this set is small and closed. There are 18 cases in total right now, 16 FatalError variants plus 2 FlushError ones, and I do not expect it to grow, these are internal validation/IO failure modes, not an open API surface. It already shrank by one in this branch (FailedToStartIndexes, dropped because it was unreachable from any real kernel consumer).

On your example, "Bad undo data in block {hash} height {height} file {filename}", option (2) does not have to produce a vaguer message. AssumeutxoDataNotFound/SnapshotValidationFailed and similar carry the typed fields needed (a block hash, heights, a path), and the node still builds that exact detailed, translated message (see FatalErrorMessage in kernel_notifications.cpp), nothing is lost. It is true that right now the C API loses that message: it only hands back the numeric code. I was thinking about sending the dynamic part of the error the same way I do for the enum itself, maybe through a union (instead of a variant), but that might push manual maintenance work onto the bindings (that is the reason I did not do it in the first approach, worth a discussion).

On "calling any kernel API means needing to handle a half-dozen error codes": I see that as a positive. Handling the error would be the application's responsibility, and I think that is what lets the kernel stay smaller and closer to consensus, further away from logging or presenting errors. The C API just hands back the numeric code (btck_FatalError); no message is exposed, only the code, the same precedent btck_BlockValidationResult already set. Building anything out of it is entirely up to the consumer. The node (in this repo) is the application that wants the full detailed message, so it is the one place doing an exhaustive std::visit over all the variants (other consumers are free not to be that exhaustive).

All of the above is just my own opinion. As a fairly recent contributor, I am not trying to impose any of this, just making proposals I am happy to change and discuss.

ryanofsky commented at 11:15 PM on September 25, 2026: contributor

Thanks, and I want apologize because my last comment was just based on an initial reaction and not very constructive.

I do agree with the direction of the PR and approach of adding structured error types in a src/kernel/error.h file. Right now kernel functions don't return enough structured error information and they should return more.

I just want to caution against trying to use types intended for error handling for error reporting as well, because the goals of error handling and error reporting are different. For error handling you generally want fewer branching cases to avoid code complexity and bugs, while for error reporting you usually want more detail. I was also just surprised to see the goal of removing the translation header because it is so small and it was unclear what benefits that removal would bring.

Overall direction of the PR seems good, but would maybe just want to clarify the goals a little more. I think there are a number of problems worth addressing in the error handling area, the main ones being that errors are returned by callback instead of return value, which makes them difficult to handle, and that we don't have good error type, and to extend we do there is no type safety so a function that can actually only return 3 errors looks like it returns 15 errors and caller have to deal with unnecessary ambiguity and mess.

But looking at actual changes in this PR they seem reasonable, and like something that could be built on to fix other problems. The one thing I would change is that it seems like btck_NotifyFatalError and btck_NotifyFlushError are losing all details about errors that occur because messages are being replaced by codes. This seems like it could cause real harm to kernel applications because there could be no clean way to extract and present error information to users. But could be fixed by keeping the error messages and adding the codes instead of replacing the messages with codes

arejula27 commented at 12:18 PM on September 26, 2026: contributor

Appreciate that, @ryanofsky. I have updated the PR description, hope it is what you wanted.

I would also like to recommend reviewers take a look at #35646 (I just left a review there): although DrahtBot does not flag it as a conflict, both PRs define a kernel::FatalError type with the same name but different goal and the main purpose of the PR is other.