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-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| Concept NACK | ajtowns |
| Concept ACK | sedited, josibake, willcl-ark |
| Approach NACK | l0rinc, stickies-v |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></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?
19 | +## Strategy 20 | + 21 | +The fundamental rule is: 22 | + 23 | +> **Return values represent successful outcomes. Exceptions represent failures 24 | +> to perform the operation. Defects terminate the affected execution.**
Return values represent successful outcomes
This was discussed in #34931 (comment), this isn't how we've been doing things, this isn't the functional approach, I'm very much against these unpredictable side-effects that exceptions are bringing. It doesn't seem to me that we have consensus over this, I don't like that you're forcing these instead of discussing them (see my ignored comment above).
I'm very much against ...
Well, that is your personal opinion. And it is fighting the language, as @sedited correctly concluded.
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?
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.horutil/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::Expectedandutil::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
ErrororExpected<T>return values, while this doc proposes exceptions and explicitly says new code must not usestd::expectedor 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 newutil::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:
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:
std::expected trades runtime efficiency for runtime determinism.Additionally, exceptions have more general advantages:
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::Expectedwas 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.
???
@mzumsande, I understand that different subsystems have different requirements for when it is appropriate to bring down the whole process. However, I am also confident that a common strategy for error handling can account for those differences and further that a single strategy would be beneficial for the project.
The answer may already be found in this sentence, but some clarifying wording may be missing:
The distinction is therefore based on why the operation did not produce its result
That essentially says that the classification of result, error, and defect depends on the established contract of the interface.
It is perfectly reasonable that p2p code will have wider contracts than, say, validation. It is also perfectly fine for code to replace Assert() with Assume(). This does not imply that bugs are fine in non-critical parts. It just means that, in this particular place, certain inputs that were considered contract violations before, are now part of an allowed value range with well defined behavior, even though the behavior may be just log and continue.
Before I incorporate this (and other) changes, I'd appreciate your feedback whether you think it could resolve your concerns.
Imo there are different aspects: First, the technical/language-level (exception vs return values etc.). I don't have strong opinions here - I personally like variety more than a single policy enforced everywhere, but I do think that more consistency within modules such as validation would be good.
The second one is the functionality (what should the program do if a defect is encountered), and I do have a strong opinion there that it is completely unacceptable to sacrifice any locally desired functional outcome in favor of nicer code or project-wide consistency:
Bitcoin Core is quite unique in that 1) it's a peer-to-peer application that is useless without a functioning network, 2) The overwhelming majority of network participants run some version of Core code 3) we don't want auto-updates, so bug fixes can only rolled out over a time frame of months/years.
Given the fact that due to the complexity of the program, defects (as in: state that we think is impossible but actually can happen) will likely always be encountered, it is important to program defensively and triage them correctly at a point where we don't know if the condition will ever be breached: We must make a decision on a case by case basis whether a potential defect warrants shutting down the node, stopping the execution of the particular task but continuing at a higher level, or just carry on as if nothing happened, even we are convinced the defect can never actually occur. This is not just theory, there is a long history of remote crashes via bad asserts (e.g. Bitcoin Unlimited during the block size war).
And this is not just by subsystem: Also in validation, there are many bugs that we want to know about and fix, but that don't warrant shutting down the node and, as a consequence (because they can often be triggered by miners at a cost), the whole network. That is why CheckBlockIndex() exists, which is not active by default and documents invariants that we think should be guaranteed even in the most artificial fringe scenarios, so that these can be tested by fuzzing, Antithesis, LLMs etc.. The philosophy there should be to make the asserts as strict as possible, and as a result, defects are expected and do happen regularly, see for example #36021, #36117.
So I think that any error policy must reflect that decisions how to deal with state we thought was impossible to happen, should it happen anyway, must be made on a case-by-case basis - a policy can give guidelines for these decisions, but it cannot dictate just one outcome.
No activity for almost a month? Could turn into draft for now, while this is not ready for review?
@maflcko, why do you consider this not ready for review? It is not ready to merge in the current state, but I also have not received enough change recommendations to submit an updated version. If consistent guidelines for error handling are not desired, we can also close.
@maflcko, why do you consider this not ready for review? It is not ready to merge in the current state, but I also have not received enough change recommendations to submit an updated version.
Heh, I won't hold anyone back from reviewing this, but I think there has been plenty of feedback that needs to be addressed, so the value of reviewing a stale state that won't be merged anyway seems low (There are more than 400 other pulls waiting for review). But again, anyone is still free to review this, even while it is in Draft.
Concept ACK on documenting error handling guidelines, so they can be used to avoid repeating the same discussions in separate PRs.
I agree with reviewers before me that many of the statements in this document are too sweeping, not (at least currently) a good fit for our codebase, and lack sufficient contributor consensus, so Approach NACK. For example, I agree with @mzumsande that continuing after a defect can be a sensible decision, especially where peers can trigger it, as we want to minimize the chances of nodes crashing. And while more consistency would be nice, I haven't converged on a single error propagation mechanism that we must always use. Both exceptions (e.g. invisible in signatures, and easy to turn into std::terminate from destructors or threads) and return types like util::Expected (e.g. boilerplate at every call site, no way to fail from a constructor) have drawbacks significant enough that I wouldn't feel comfortable mandating either.
Instead, I think it would be useful and productive to update developer-notes.md with the shared vocabulary (result/error/defect) outlined here, the subset of guidelines we can so far agree on (e.g. "never report an error as a negative result" seems straightforward and actionable), and specific examples of when/how it is good to throw or catch exceptions or return failure types, and when it is an anti-pattern.
As toolchains improve, our codebase evolves, and perhaps our agreement on error handling best practices increases, we can become more prescriptive over time when it makes sense.
There are three separate points I would like to address.
The meaning of “defect” @stickies-v @mzumsande, I think we should be able to find common ground here, because I suspect our disagreement is primarily about terminology.
I define a defect as an unexpected state in the program. If a function is designed to continue correctly from a particular state (even an arbitrary state) then that state is, by definition, accounted for by its design. It is therefore not unexpected, and hence not a defect in the sense I am using here.
Under that definition, your examples of safe continuation seem compatible with my position. Can we agree on that distinction? I think making it explicit would help us avoid arguing past each other.
The purpose of coding guidelines
I do not think coding guidelines need to describe the status quo. They can define a goal toward which the codebase should evolve. Consequently, the fact that existing code follows a different approach is not an argument against the proposed guidelines.
The current codebase is relevant to how we implement a transition, but it does not determine whether the proposed goal is the right one. I would like us to assess that goal on its technical merits.
Error propagation through consensus code
In my view, a stateless, side-effect-free validation library, as proposed in #35904, written in idiomatic C++ should be free of explicit error handling code. It should neither throw nor catch, nor wrap or unwrap runtime errors in a result type. Errors should not be stored in global variables, and functions should not be able to intercept error propagation.
This last requirement goes beyond prohibiting catch: intermediate functions should not receive errors as values or through shared state that they can inspect, suppress, or turn into validation results. The absence of explicit error handling is a deliberate design constraint. Each such handling point creates an opportunity for misuse.
The 2013 chain split, where a database resource limit effectively became a consensus rule, is a concrete reason to take this seriously. A failure to complete validation must remain distinct from a completed validation that finds a block invalid.
Consensus code should therefore be written so that runtime errors reported by coin lookup can safely propagate without causing damage along the way. In other words, it should conform to exception-correct programming practices. I consider this a stronger design because it removes opportunities to mishandle runtime failures from the consensus logic itself.
I hope we can resolve the first point by agreeing on the terminology. On the other two, I recognize that we may still disagree about the design we should pursue. My position remains that guidelines should establish that goal, and that keeping explicit error handling out of consensus logic is the stronger design. I would welcome concrete technical arguments against that goal, independently of how much work it would take to reach it.
- I define a defect as an unexpected state in the program. If a function is designed to continue correctly from a particular state (even an arbitrary state) then that state is, by definition, accounted for by its design. It is therefore not unexpected, and hence not a defect in the sense I am using here.
So what does that mean practically? If a function's input parameter is documented to be non-null, and the function recognizes it has received a null pointer, but it can handle the situation (e.g. by returning early, by shutting down gracefully, ...), then the "Proper Defect handling" rule does not force us to terminate? Practically speaking, in cases where we don't want to tighten the crash behaviour, that means we'd be loosening contracts (e.g. document nullptr as allowed) rather than changing termination behaviour? I.e. only assert where we really have no other option but to crash, or where crashing is fine?
Perhaps to approach it from a different angle: what is the problem that you're addressing with the "Proper Defect handling" rule? In the developer notes, for example, we often add a rationale, and I think that would be helpful here too.
I do not think coding guidelines need to describe the status quo.
I definitely wasn't arguing for that. My argument was that documented rules/guidelines need to represent developer consensus on where we want to go, which is lacking especially for the error propagation mechanism. I agree that this kind of documentation can help us make more progress in a good direction.
3. In my view, a stateless, side-effect-free validation library, as proposed in [Factoring out a stateless, side-effect free validation library #35904](https://github.com/bitcoin/bitcoin/issues/35904), written in idiomatic C++ should be free of explicit error handling code. It should neither
thrownorcatch, nor wrap or unwrap runtime errors in a result type. Errors should not be stored in global variables, and functions should not be able to intercept error propagation.
At first glance, that seems like a sensible approach for a narrowly scoped (and newly carved out/refactored) part of the codebase, though I haven't thought through all the trade-offs yet. Could we document that for this new library, and discuss it further in the relevant PRs or in a dedicated issue? I think the global scope of the proposal is the hard sell here.
If a function's input parameter is documented to be non-null, and the function recognizes it has received a null pointer, but it can handle the situation (e.g. by returning early, by shutting down gracefully, ...), then the "Proper Defect handling" rule does not force us to terminate?
Yes, that's what I'm getting at.
I have strong opinions about nullable arguments, which I see as related to the infamous "Boolean Trap", and about passing pointers as function arguments in general. But those are separate from the question of error handling.
The question here is really: "Should functions be required to terminate on precondition violations?" I feel less strongly about whether they actually terminate, as long as the violation isn't presented as a runtime failure or used to produce an otherwise invalid result. Doing nothing may be perfectly valid behavior for a function. But if that's how it handles this input, then that input is already part of the domain it supports.
Practically speaking, in cases where we don't want to tighten the crash behaviour, that means we'd be loosening contracts (e.g. document nullptr as allowed) rather than changing termination behaviour? I.e. only assert where we really have no other option but to crash, or where crashing is fine?
Yes. According to Hyrum's Law, the function already has a wider contract in practice. Updating the documentation would bring it into line with that behavior. Documenting a restriction just to reserve the right to enforce it later is unlikely to work well: callers may already depend on the behavior, regardless of what the documentation says.
And yes to the assertion question as well. That's what an assertion expresses: a condition that must hold for the code to proceed correctly. The same reasoning applies to preconditions: they state what must hold for the function to fulfill its contract. If the function can correctly handle the condition being false, then that condition isn't actually required and shouldn't be presented as such.
I do not think coding guidelines need to describe the status quo.
I definitely wasn't arguing for that.
I didn't mean to imply that you were. Sorry, I should have been clearer about whom I was responding to. That was a separate point addressing “this isn't how we've been doing things” and “doesn't reflect existing practice” elsewhere in the discussion.
I think we're getting closer on the first point. Your example describes a function that can safely handle an input its documentation excludes. The documentation should reflect that behavior. My concern is with continuing when the assumptions needed for correctness no longer hold. Precondition violations should also not be treated as runtime failures or affect the function's return value. I'm well aware that the standard library doesn't always follow this guideline, but I still think it's the right goal for the code we control.