doc: Add an error handling strategy #36101

The C++ Core Guidelines recommend establishing an error handling strategy early in a project's design (E.1). Under the presumption that "we are still early" and given the fact that there is lots of development (eg #34931, #35003, #34132, ...) without such a strategy, it might be the right time to come up with a design document.

I post this as a PR rather than an Issue, because it will simplify giving feedback on individual parts of the text.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

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

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Concept ACK

I think this is a good place to start off and matches close to my own ideal.

The conversations in the pull requests you linked, and which I very much hope we can resolve here, seem to mostly revolve around not having a good answer on who should handle errors, and not necessarily the strategy for communicating them. The document touches on this, but "Exceptions should propagate until reaching a layer that can meaningfully respond to the error." seems a bit light on details. If we terminate ungracefully, an exception in a non-vital part of the node brings down the entire node, which is often undesirable. Similarly, some developers do not seem confident that components like the wallet are robust against termination. Not terminating means higher level loops now need to think about all the different ways exceptions may occur so we never get stuck in doom loops (see #35003).

Some interesting advice towards handling exceptions is in this document: https://www.autosar.org/fileadmin/standards/R17-10_R1.2.0/AP/AUTOSAR_RS_CPP14Guidelines.pdf , and specifically its advisory A15-0 and A15-3, which broadly categorizes exceptions into a checked and unchecked category. The basic advice is to not recover from unchecked exceptions, which would make things like catching with catch (const std::exception& e) an anti-pattern. I think this is sensible, and we might even consider listing the valid checked exception derivation classes.

A common fear I've heard is that exceptions might be remotely triggerable in an unexpected way and that exhaustively enumerating our errors and propagating them would protect us against crashes. I think this is fighting the language: Errors are communicated with exceptions in the standard and I don't think translating them all exhaustively is possible. Similarly, many of our dependencies and sub-components throw on errors. Still, I couldn't say for certain that e.g. not swallowing all filesystem errors in ProcessMessages is a safe practice (and conversely that swallowing them is).

A common culprit for introducing exception handling is the serialization code. Should a failure to serialize be what you call here a Result?

Approach NACK, instead of presenting your personal opinion as the only way forward, try gathering info first on the project's needs and try to represent the consensus instead of trying to force the only-correct-way™...

If we terminate ungracefully, an exception in a non-vital part of the node brings down the entire node

Note that I recommend ungraceful termination for defects only. If a defect is discovered, all bets are off. Even if it occurs in a non-critical part of the program, you cannot make any claim of the validity of the critical parts (for example, a buffer overflow in a non-critical part might have corrupted critical data).

An uncought exception on the other hand should just propagate up until main and execute all destructors along the way, which is essentially a graceful shutdown.

The basic advice is to not recover from unchecked exceptions

The listed examples (length error, invalid argument) sound like precondition violations (defects, not errors) to me.

Should a failure to serialize be what you call here a Result?

Yes. Well, it depends on the contract of the function. If the function promises to successfully parse a random sequence of bytes into a block, then it would need to report an error, since there is no way to keep that promise. But in the more reasonable approach, the function would have a contract that caters for the case where the input is corrupt or truncated. Such a function would return std::optional<Block>, for example. And the function would further not be marked noexcept. It would return std::nullopt if the input data does not contain a block or it would throw an exception in case of a runtime error (like allocation failure).

instead of presenting your personal opinion as the only way forward

I am deeply sorry if I have hurt your feelings, but I opened this PR as a base for discussion.

try gathering info first on the project's needs

I did that. And I mention it in the document. The project's requirement is runtime performance over runtime determinism. A bitcoin node needs to validate a block roughly every ten minutes on average, but do it as fast as possible. That is very much different from the embedded domain where data often needs to be processed on a fixed hard time schedule.

The listed examples (length error, invalid argument) sound like precondition violations (defects, not errors) to me.

A commonly used example in the document I linked for an unchecked exception is bad_alloc, which doesn't seem like a defect to me, but an actual error. That might also be a problem in the extremes of the verbage in the introduced doc here: Some of the code we call in the standard library does throw on length errors and invalid arguments. In what category do those fit in?

Proper Defect handling

A defect means that the program can no longer rely on its state satisfying its requirements. A defect must therefore not trigger a graceful shutdown that persists or flushes that state to durable storage.

Doing so could turn an in-memory defect into persistent corruption: state that was already known to be invalid could overwrite previously valid state in the database, making recovery from the defect substantially harder or impossible.

The response to a defect must terminate execution without performing further state transitions that depend on the affected state

This lacks perspective of other systems running concurrently. Please see discussion #35676 (review). There are multiple examples where abrupt terminations can cause the wallet to enter an inconsistent/unrecoverable state, which puts users' funds at risk.

At least while we keep multiple subsystems coupled into the same process, I don't think it's safe to assume abrupt terminations are harmless. And we should try to avoid them whenever possible.

Proper Defect handling

This lacks perspective of other systems running concurrently. Please see discussion #35676 (comment).

Does it? The linked discussion is about a situation that can occur when a user unplugs a drive. That is clearly an error and not a defect (It is outside the correctness of the program: There is no way a program can guarantee that the user does not unplug the drive). It should be propagated as an exception (the simplest approach would be to just call the single-argument form of create_directories). The exception should propagate up, resulting in a graceful shutdown if it is not caught before.

I don't think it's safe to assume abrupt terminations are harmless.

Correct! Hence, ungraceful termination should be reserved for cases where a graceful shutdown is more harmful because the in-memory state cannot be trusted.

The linked discussion is about a situation that can occur when a user unplugs a drive. That is clearly an error and not a defect

Not sure how you can assert that this is clearly just an error and not a reason to abort, when marko and josie argued strongly in favor of aborting there. What seems clear to me is that people have different views and perspectives, and introducing the possibility of abruptly aborting in more places could do more harm than good.

I don't think it's safe to assume abrupt terminations are harmless.

Correct! Hence, ungraceful termination should be reserved for cases where a graceful shutdown is more harmful because the in-memory state cannot be trusted.

That conclusion doesn't really follow. The fact that one subsystem's in-memory state cannot be trusted does not imply the same for the rest of the process. Aborting may harm other subsystems whose state remains valid. I wrote an example using the wallet above. Only the affected subsystem should avoid flushing to disk. All others should be allowed to shut down gracefully.

That is clearly an error and not a defect

Not sure how you can assert that this is clearly just an error and not a reason to abort

The answer is found in the document: "The distinction is therefore based on why the operation did not produce its result, not on whether the result is desirable or how frequently it occurs".

The fact that one subsystem's in-memory state cannot be trusted does not imply the same for the rest of the process.

It does. I gave an example above: "If a defect is discovered, all bets are off. Even if it occurs in a non-critical part of the program, you cannot make any claim of the validity of the critical parts (for example, a buffer overflow in a non-critical part might have corrupted critical data)".

NACK, this isn't a problem that needs a 750 word essay to address (or a set of issues/PRs bikeshedding the topic), and the essay provided doesn't reflect existing practice (eg util/expected.h or util/result.h) so if merged would introduce significant technical debt to change those structures. In my opinion exceptions are also worse than the existing approaches.

I am not against this, but I think the main driving factor here should be code review: Whatever error handling strategy is picked, should lead to the least overhead during code review and require the least amount of manual fixups. Of course, future cost is hard to quantify, so I guess this is just my opinion.

Specifically, I worry about runtime errors turning into programming errors (defects). Sure, there are high-level catch-alls for exceptions already in the code today. So one could assume it is fine to just use exceptions for runtime errors. However, Bitcoin Core is multi-threaded and the shutdown requires threads to be alive (to be able to flush their events and stuff). Otherwise there could be a defect (like a deadlock or memory error from a stale pointer or whatever). If exceptions were used more broadly, reviewers must always implicitly consider all calling threads for all functions and waste review cycles on that instead of focussing on the logic itself.

Also, as already mentioned above, code such as P2P may have to deal with different runtime errors differently (i.e. enumerate them) to be able to kindly disconnect or gracefully continue, but it doesn't do so today. I am not sure if manual code review is the ideal way forward here to ensure such handling happens (and will happen with all possible future code changes).

Moreover, I am sure everyone is aware of the "exceptions in C++ dtor meme", but I don't think it is on reviewer's normal radar. If it was, we wouldn't see runtime errors turning into defects, like #36049 (comment).

C++ is still trying to port the Java-style checked exceptions, so if we go down this path here, we should be aware of the downsides and possible review overhead as well. Recall that the AUTOSAR rules quoted above are mostly marked with "non-automated".

I understand you want this pull to be a doc-only change, but if the requirement is that new code "must follow it", there needs to be an assurance or at least plan that the codebase is ready to properly deal with exceptions in all contexts and threads.

Concept ACK

I strongly agree that an established error handling strategy is essential for a project of this size. I haven't yet read the document in full, but having one place to start the conversation is already much better than having whackamole conversations on different error handling PRs. Ideally, we can agree on some general guidelines here and then use that to revisit some of the open PRs, before moving on to existing code.

I am not against this, but I think the main driving factor here should be code review: Whatever error handling strategy is picked, should lead to the least overhead during code review and require the least amount of manual fixups. Of course, future cost is hard to quantify, so I guess this is just my opinion.

When you say code review, do you mean code review for moving to the new strategy? Or ongoing code review when someone wants to determine if all the errors have been handled when introducing new code or touch existing code?

@l0rinc

Approach NACK, instead of presenting your personal opinion as the only way forward, try gathering info first on the project's needs and try to represent the consensus instead of trying to force the only-correct-way™... @ajtowns

NACK, this isn't a problem that needs a 750 word essay to address (or a set of issues/PRs bikeshedding the topic), and the essay provided doesn't reflect existing practice (eg util/expected.h or util/result.h) so if merged would introduce significant technical debt to change those structures. In my opinion exceptions are also worse than the existing approaches.

Please take a look at https://github.com/bitcoin-core/meta. In particular:

• Comments will be about ideas, not people.
• Comments may offer pointed criticism, if it is criticism about specific technical ideas or decisions, not general criticism, or criticism of individuals or groups. Even the smartest people can have ideas that don't work out, and people with good intentions can make decisions that backfire. It does not add a lot of value generally to speculate about peoples motives or capabilities when discussing the merits of their ideas, and doing so will be considered off-topic in technical discussions.

Obviously, expected and encouraged that people will have technical disagreements on these subjects. But my hope is that we can work through those constructively to end up with a document that is more generally useful than not having one at all. The NACKs you both provided don't seem instructive, constructive, or on topic at all.

Yes, please keep the discussion here focused on the (de)merits of the proposal and leave personal attacks for elsewhere (or better still nowhere at all).

~ Concept ACK for having a more-well-defined approach to this for the project in general. @purpleKarrot I have felt that more recently Bitcoin Core has been moving towards a more LLVM-style explicit-error-handling model, as seen by our introduction/use of util::Expected and util::Result...

On first comparison the two documents ~ agree on programmer defects: invariant or contract violations should be detected and fail fast. They differ on environmental and recoverable failures. LLVM models those as explicit Error or Expected<T> return values, while this doc proposes exceptions and explicitly says new code must not use std::expected or another return-value mechanism.

There are other more stubtle diferences, for example in how malformed input is classified. LLVM treats it as a recoverable error. Under this PR's contract-based approach, malformed input could instead be a normal negative result if a function's job is to determine whether arbitray input is valid.

Adopting LLVM's direction would mean keeping (and extending) util::Expected, making failure paths visible, and strengthening enforcement that callers handle errors. Adopting the approach here would prohibit new util::Expected-style error APIs and steer future code toward exceptions, which would likely require us auditing our exception safety and thread boundaries a lot more closely.

I'd be interested to know whether you considered the LLVM-style approach in formulating this, and why you consider your approach as more appropriate for our project?

My read is that this proposal would likely help us remove error branches from local callsites, but we'd have to beef up our exception safety a fair bit too. LLVM-style woudl require more boilerplate in local functions but potentially? be easier to audit that we are handling failures correctly. I don't see how we could wholesale move to LLVM-style though, as we certainly have code which throws exceptions, so the choice is probably down to either "a hybrid" (similar to what we have currently) or something more as prescribed here.

I have felt that more recently Bitcoin Core has been moving towards a more LLVM-style explicit-error-handling model

Is there a reason for this? At first glance, I don't see how these two projects have anything in common considering one is a large set of libraries and tools, and the other a running service with certain performance guarantees. I also skimmed through the doc, and it reads to me like they reinvent a lot of machinery to get back the same functionality of exceptions. See specifically their handleErrors stuff.

Furthermore, it sees their only reason for not using exceptions that I can find is mentioned here: https://llvm.org/docs/CodingStandards.html#do-not-use-rtti-or-exceptions , where they have one sentence saying "we don't use exceptions or RTTI to reduce code and executable size." That seems counterintuitive to me since using exceptions should reduce code, as it removes explicit error code handling branches that are rarely/never used. Somewhere else on StackOverflow runtime performance is mentioned, but that's also largely dependent on the code you write. In fact, I'd expect runtime performance to improve if we switched to an exceptions error handling strategy.

While there may be something I'm missing, it feels like they rolled their own exception handling by writing a lot of extra machinery. Considering we don't have even remotely the same use case, it feels kinda dangerous to adopt their strategy.

Seems very dubious to have a single strategy for the entire project, especially concerning defects, given how different its modules are:

While a defect in validation will often justify a shutdown, in p2p code that is triggerable remotely it is often best practice to add a debug-only Assume() and just abort the processing of the message, but not the entire program, after various bugs such as https://bitcoincore.org/en/2024/10/08/disclose-blocktxn-crash/ where the assert was far worse than the actual issue at hand. In rpc code, CHECK_NONFATAL is often used for defects, so the user gets an "Internal bug detected:" message but the node doesn't stop running.

"If a defect is discovered, all bets are off." That is incorrect. Not all defects lead to memory corruption. And even for those that would, often the checks can be placed in a spot before any UB would occur that could affect other areas of the code (see for example here or here ) so that memory of other modules would not be affected.

I see no reason why we would regress on these pragmatic, domain-specific solutions in favor of a global policy from the drawing board.

In my opinion, we should not have a project-wide directive. A doc could point out different solutions and situations/domain in which they are suitable - but we have this already, see "Assertions and Checks" in the developer notes.

Thanks for the review, @willcl-ark!

I have felt that more recently Bitcoin Core has been moving towards a more LLVM-style explicit-error-handling model, as seen by our introduction/use of util::Expected and util::Result...

Yes, I observe the same trend. I fear that blindly following the LLVM-crowd puts the code base at risk, which motivated this design document. There definitely are domains where errors are better propagated via return values. I don't want to comment on whether this is appropriate for LLVM, but for Bitcoin Core in particular, it is the wrong direction.

This is not a question about personal preference. Bitcoin Core has specific requirements on performance and correctness that make exceptions the better choice.

On first comparison the two documents ~ agree on programmer defects: invariant or contract violations should be detected and fail fast. They differ on environmental and recoverable failures. LLVM models those as explicit Error or Expected<T> return values, while this doc proposes exceptions and explicitly says new code must not use std::expected or another return-value mechanism.

Correct. While LLVM describes two categories, this document knows three and makes a clear distinction between Results and Errors. For LLVM, this distinction may not be relevant. If Clang fails to compile a file, the developer will just retry. If Bitcoin Core fails to validate a block, we need to be very careful whether a validation result should be persisted or not. This is not a made up example. This is exactly what happened before.

There are other more stubtle diferences, for example in how malformed input is classified. LLVM treats it as a recoverable error. Under this PR's contract-based approach, malformed input could instead be a normal negative result if a function's job is to determine whether arbitray input is valid.

Right. A bitcoin node receives data across the network that it then attempts to parse into its own in-memory structure, like CBlock. Parsing failures are normal, because data received from the network is never trustworthy. Bitcoin Core currently reports parsing failure by throwing std::ios_base::failure. That is problematic. Throwing exceptions for normal control flow has huge performance downsides. Code using exceptions may be 100-1000 times slower in the failure case than in the success case.

In the LLVM approach, the performance problem does not exist. Recoverable errors use the return value; there is no difference in performance between the failure case and the success case.

Adopting LLVM's direction would mean keeping (and extending) util::Expected, making failure paths visible, and strengthening enforcement that callers handle errors. Adopting the approach here would prohibit new util::Expected-style error APIs and steer future code toward exceptions, which would likely require us auditing our exception safety and thread boundaries a lot more closely.

There are a few misconceptions here that I need to address:

• All code needs to have an error handling strategy. There needs to be a mechanism for propagating error information up the call stack in a way that invariants are preserved and no resources are leaked, regardless of what mechanism is chosen. It is not the case that one approach requires more auditing than another.
• Idiomatic C++ is inherently thread-save. There is no need for a visible failure path to know where resources should be freed. Resources are simply freed when their owner goes out of scope, due to an error or not.
• Likewise, code that looks wrong should be fixed to look correct, even in the absence of a failure path.
• What does it even mean that callers handle errors?

I'd be interested to know whether you considered the LLVM-style approach in formulating this, and why you consider your approach as more appropriate for our project?

I did. The two reasons why exceptions are preferred for Bitcoin Core specifically are:

• A clear separation between Result and Error. This is particularly important for consensus code. We need to be able to reason about the case that a block can be successfully found to be invalid.
• The optimization for the success path. The failure path can be 100-1000 times slower than the success path. For Bitcoin Core, that trade-off is appropriate. std::expected trades runtime efficiency for runtime determinism.

Additionally, exceptions have more general advantages:

• It is the default error handling mechanism in C++, built into the compiler.
• It is the only way to propagate errors from a constructor.
• It is the mechanism that composes with standard algorithms and view pipelines.
• It is the mechanism that composes with structured concurrency code (executors and senders).

While a defect in validation will often justify a shutdown, in p2p code that is triggerable remotely

There seems to be a misunderstanding what a defect is. A defect is a programming error. There is no way to cause a programming error remotely over p2p. If an assert() can be triggered via a p2p message, there is a programming error somewhere else.

Not all defects lead to memory corruption.

Correct. The corruption is not guaranteed. But the correctness is neither. So you have to assume the worst case.

There seems to be a misunderstanding what a defect is. A defect is a programming error. There is no way to cause a programming error remotely over p2p. If an assert() can be triggered via a p2p message, there is a programming error somewhere else.

A programming error has occurred first - the programmer thought some state is impossible, and added an assert for that and code that relied on it, but they were either wrong all the way, or maybe later changes by others made it wrong, so at some point the state is no longer impossible.

Now, if a remote attacker can trigger the conditions for this state via p2p messages, they can take down each node that runs the code by triggering the assert. That is why we try to move away from asserts and use the Assume/return pattern, so that even if there is a programming error, the consequences are less catastrophic - especially in modules that are not consensus-critical.

I have felt that more recently Bitcoin Core has been moving towards a more LLVM-style explicit-error-handling model

It's not LLVM specific, it's simply a functional programming paradigm to avoid surprises and hidden assumptions:

It is a declarative programming paradigm in which function definitions are trees of expressions that map values to other values, rather than a sequence of imperative statements which update the running state of the program.

An exception is an unannounced return value, it's a surprise, a side-effect: a lie. It seems we're in agreement that using exceptions for non-exceptional control flows should be avoided - I argue that it's best to avoid exceptions in most other cases as well. Once we migrate to C++23 we will have less awkward ways to deal with these states - but even this awkwardness is preferable to the exception surprises.

blindly following the LLVM-crowd puts the code base at risk

Calling everyone who disagrees with you blind isn't a winning strategy...

This is not a question about personal preference. Bitcoin Core has specific requirements on performance and correctness that make exceptions the better choice.

These absolutes also don't help. Core is a complicated project, you can't just "blindly" bring your textbook best-practices over without understanding the reasoning behind previous decisions.

only reason for not using exceptions that I can find is mentioned here

Google's general guide is against all exceptions (see https://google.github.io/styleguide/cppguide.html#Exceptions), it states that they introduce non-local, invisible control flow: when a deep function starts throwing, every transitive caller has to satisfy exception-safety requirements even when it does not handle the exception itself.

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p0323r12.html#yay focuses on exceptions being invisible in a function signature, whereas expected<T,E> makes failure visible at the interface. It also explicitly calls out the "monadic interface" as a way to separate error handling while keeping normal control flow clean.

Our fuzzers also require a lot of hacking around to deal with these exception, see https://github.com/bitcoin/bitcoin/pull/32313/changes#diff-f0ed73d62dae6ca28ebd3045e5fc0d5d02eaaacadb4c2a292985a3fbd7e1c77cR81

Throwing exceptions for normal control flow has huge performance downsides. Code using exceptions may be 100-1000 times slower in the failure case than in the success case.

We're in agreement here, exceptions should preferably be avoided.

It is the only way to propagate errors from a constructor.

Exactly, exceptions force you into these awkward structures where a constructor has these side-effects. If the construction of an object is non-trivial, we can use a builder instead of relying on these side-effects. This is a code smell, we should step back and rethink it instead. Throwing constructors are a recipe for disaster.

It is the mechanism that composes with standard algorithms and view pipelines

This was the case for older C++ versions, newer ones embrace the functional composition paradigm, see https://peschinskiy.github.io/functional-programming-features-in-cpp20-and-cpp23

@l0rinc, I respect your personal preference against exceptions. But please keep the discussion technical and focused on the topic. If you have a concern agaings exceptions beyond the fact that you think they are "awkward" or that some other project or company does not use them, then please provide a rationale. If there was a previous decision, then just link to it. But also accept that previos decisions are not sacrosanct.

Multiple pieces of information are not present in function signatures and are usually mentioned in documentation instead: The function's contract (preconditions, postconditions, invariants), its complexity, the aliasing of its arguments (may they overlap?), its exception safety guarantees (will the object roll back to its original state in case of an error?), the types of the exceptions it can throw, and under which conditions it throws them. The fact that those pieces are not part of the function signature does not classify them as "surprise", "side-effect", or "lie".

If a constructor throws an exception because it fails to establish the object's invariants, it is not a "smell" or a "recipe for disaster", but a guarantee that the object can never exist in an invalid state, which eliminates a complete category of bugs.

The guidelines that you linked are well aware of the advantages of exceptions. They clearly state "the benefits of using exceptions outweigh the costs". They only disallow exceptions for hysterical raisins, because there is a lot of pre-exceptions code. Bitcoin Core is in a different situation. It has been using exceptions from the beginning and util::Expected was introduced just 8 months ago.

It has been using exceptions from the beginning and util::Expected was introduced just 8 months ago.

The statement isn't wrong, but it seems to imply that there was some recent and sudden switch in the error handling strategy. Just for completeness: Bitcoin Core has been using boost/std::optional (or an ugly plain bool) for many years and util::Result was introduced more than 4 years ago.

Again, as mentioned above, I am not against this change, but it can absolutely not be merged in its current form, while ignoring the review feedback above. Merging it would create internally contradictory documentation. Also, doc/error-handling.md is the wrong place to put the docs. This isn't an error handling recommendation for end-users, but a purely internal developer style guide. This should simply be a section (or update the existing section(s)) in doc/developer-notes.md. Also, ./src/util/{check.h,result.h,expected.h} etc will need their docs to be updated.

Thanks for giving actionable feedback and adding historical context, @maflcko!

It is much easier to respond to feedback if it states what needs to be done rather than whether it is acceptable in the current form. I have no doubt that the document needs refinement. But it is hard to extract the actionable items from all the noise.

all the noise.

???