#36101 doc: Add an error handling strategy
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36101 doc: Add an error handling strategy | Waiting on author | 0 -3 | Blocked | 124 | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
This pull request adds `doc/error-handling.md`, proposing an architectural error-handling strategy for Bitcoin Core. It formalizes a three-tier model: return values represent successful domain outcomes, exceptions represent external runtime errors, and defects (bugs and invariant violations) trigger fail-fast termination. It explicitly forbids new code from introducing `std::expected` or similar return-value mechanisms for propagating errors.
Problem: Bitcoin Core lacks a unified error-handling strategy, leading to repeated ad-hoc debates across validation and subsystem PRs over when to use exceptions, return types like `util::Expected`, or process aborts.
Reviewability
Paused · Waiting on author
Open objections from ajtowns, maflcko, sedited with no author reply for 20 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: addressing standing objections and suggestions regarding scope, ungraceful defect crashes, and contradiction with existing codebase utilities 2026-10-08
Agreement
Blocked
objection that needs a change, nobody has replied (ajtowns, maflcko)
Thread notes: Added alexanderwiederin as a neutral participant and recorded their comment under other_replies for l0rinc's exception objection (r:5043038193).
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| sedited (member) | question, approach | Whether a failure to serialize should be categorized as a Result under the proposed strategy. | answered · author replied explaining that it depends on the function's contract and may return std::optional | answered | yes | 2026-08-27: 'Should a failure to serialize be what you call here a Result?' Status from: 2026-08-27: purpleKarrot: 'Yes. Well, it depends on the contract of the function... Such a function would return std::optional<Block>, for example.' |
| sedited (member) | question, approach | What category unchecked exceptions like bad_alloc or standard library length errors and invalid arguments fit into under the proposal. | open | answered | no | 2026-08-27: '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?' |
| l0rinc (member) | objection, approach | Exceptions introduce unpredictable side-effects and invisible non-local control flow contrary to recent project direction. | contested · l0rinc pushed back arguing exceptions introduce non-local invisible control flow | changed | yes | 2026-08-27: 'Approach NACK, instead of presenting your personal opinion as the only way forward, try gathering info first on the project's needs' Status from: 2026-08-28: l0rinc: 'An exception is an unannounced return value, it's a surprise, a side-effect: a lie.' |
| furszy (member) | objection, safety | Ungraceful process aborts on defects prevent graceful flushing and can leave the wallet database in an inconsistent or unrecoverable state, risking funds. | contested · furszy pushed back arguing defect in one subsystem does not justify aborting without allowing wallet to flush | changed | yes | 2026-08-27: 'There are multiple examples where abrupt terminations can cause the wallet to enter an inconsistent/unrecoverable state, which puts users' funds at risk.' Status from: 2026-08-28: furszy: 'Only the affected subsystem should avoid flushing to disk. All others should be allowed to shut down gracefully.' |
| ajtowns (member) | objection, approach | Contradicts established codebase patterns like util::Expected and util::Result, introducing technical debt and worse error handling semantics. | open | changed | no | 2026-08-28: 'NACK, this isn't a problem that needs a 750 word essay to address... and the essay provided doesn't reflect existing practice (eg util/expected.h or util/result.h) so if merged would introduce signifi' |
| maflcko (member) | objection, maintenance | Broad exception propagation forces reviewers to audit all calling threads and destructors for deadlock hazards during shutdown. | open | answered | no | 2026-08-28: 'If exceptions were used more broadly, reviewers must always implicitly consider all calling threads for all functions and waste review cycles on that' |
| willcl-ark (member) | question, approach | Whether the author considered the LLVM-style explicit-error-handling model and why exceptions are considered more appropriate for Bitcoin Core. | answered · author replied detailing reasons why exceptions are preferred for Bitcoin Core | answered | yes | 2026-08-28: '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?' Status from: 2026-08-28: purpleKarrot: 'The two reasons why exceptions are preferred for Bitcoin Core specifically are: A clear separation between Result and Er' |
| mzumsande (member) | objection, approach | A global abort-on-defect directive breaks existing resilience patterns where non-consensus modules like P2P or RPC can handle bugs without node crashes. | contested · mzumsande reiterated that sacrificing defensive resilience for project-wide consistency is unacceptable | changed | yes | 2026-08-28: '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' Status from: 2026-09-03: mzumsande: 'it is completely unacceptable to sacrifice any locally desired functional outcome in favor of nicer code or project-wide' |
| maflcko (member) | objection, scope | The document creates internally contradictory documentation with existing utilities and belongs in doc/developer-notes.md rather than as a top-level document. | open · author acknowledged refinement is needed in c:5461417690 but deferred actionable updates | changed | yes | 2026-08-29: '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 ' |
| maflcko (member) | suggestion, scope | Move the error handling guide into doc/developer-notes.md rather than having it as a separate doc/error-handling.md file. | open · author replied in c:5461417690 | answered | yes | 2026-08-29: 'This should simply be a section (or update the existing section(s)) in doc/developer-notes.md.' |
| stickies-v (member) | objection, approach | The proposed guidelines are too sweeping, lack contributor consensus, and mandating a single error propagation mechanism or crash-on-defect policy risks node stability. | contested · stickies-v pushed back, reiterating that developer consensus on error propagation is lacking and the global scope is a hard sell | changed | yes | 2026-10-07: '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' Status from: 2026-10-08: stickies-v: 'My argument was that documented rules/guidelines need to represent developer consensus on where we want to go, which is ' |
| stickies-v (member) | suggestion, scope | Update developer-notes.md with shared vocabulary and agreed-upon guidelines instead of introducing a prescriptive document. | answered · author replied arguing guidelines should define an evolutionary goal rather than describing the status quo | answered | yes | 2026-10-07: 'Instead, I think it would be useful and productive to update `developer-notes.md` with the shared vocabulary (result/error/defect) outlined here' Status from: 2026-10-07: purpleKarrot: 'I do not think coding guidelines need to describe the status quo.' |
| stickies-v (member) | question, approach | What problem is addressed by the 'Proper Defect handling' rule, whether handling unexpected states gracefully requires loosening contracts, and what rationale supports it. | answered · author explained the defect handling rationale regarding precondition violations and contract definitions | answered | yes | 2026-10-08: '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.' Status from: 2026-10-09: purpleKarrot: 'The question here is really: "Should functions be required to terminate on precondition violations?"' |
| stickies-v (member) | suggestion, scope | Document the error handling strategy narrowly for the new library and discuss it in relevant PRs or a dedicated issue rather than proposing a global scope. | open | answered | no | 2026-10-08: '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.' |
Support:
- sedited (member) (Concept ACK, 2026-08-27) [read]: I think this is a good place to start off and matches close to my own ideal.
- josibake (member) (Concept ACK, 2026-08-28): I strongly agree that an established error handling strategy is essential for a project of this size.
- willcl-ark (member) (Concept ACK, 2026-08-28) [read]: ~ Concept ACK for having a more-well-defined approach to this for the project in general.
Participants:
- furszy (member, objection, 2 statements 2026-08-27 to 2026-08-28): objects to abrupt defect terminations that could corrupt wallet databases and risk user funds
- l0rinc (member, objection, 4 statements 2026-08-27 to 2026-08-29): Approach NACK; objects to exceptions due to unpredictable side effects and non-local control flow
- sedited (member, support, 2 statements 2026-08-27 to 2026-08-27): Concept ACK; supports establishing an error handling strategy, asked about serialization and standard library exceptions
- ajtowns (member, objection, 1 statements 2026-08-28 to 2026-08-28): NACK; argues exceptions introduce technical debt and contradict existing util::Expected/util::Result practice
- josibake (member, support, 3 statements 2026-08-28 to 2026-08-28): Concept ACK; strongly agrees establishing a central error handling strategy is essential
- maflcko (member, objection, 4 statements 2026-08-28 to 2026-10-01): objects to merging as-is; raises review overhead and deadlock concerns, requests moving to developer-notes.md, and notes ample feedback remains unaddressed
- mzumsande (member, objection, 3 statements 2026-08-28 to 2026-09-03): objects to a global abort-on-defect policy, preferring domain-specific resilience patterns in P2P and RPC
- willcl-ark (member, support, 1 statements 2026-08-28 to 2026-08-28): Concept ACK; supports having a well-defined error handling approach while inquiring about LLVM-style vs exceptions
- stickies-v (member, objection, 2 statements 2026-10-07 to 2026-10-08): Concept ACK on documenting guidelines, but Approach NACK due to sweeping statements, lack of consensus, and node crash risks
- alexanderwiederin (member, neutral, 1 statements 2026-10-12 to 2026-10-12): questioned l0rinc on the practical severity of exception-related costs
Dependencies
Files
0 lines under test/bench/ci.
- doc/error-handling.md +124/-0
Card
The short text the ranking pass compares across PRs.
This pull request introduces doc/error-handling.md, proposing a formal project-wide error handling architecture that categorizes program behavior into results, errors (propagated via exceptions), and defects (handled via fail-fast termination). The problem it addresses is the lack of a consistent error-handling convention, which currently fuels recurring disagreements in validation and I/O pull requests. Multiple reviewers support establishing clear guidelines, but the specific proposal faces strong pushback and NACKs from maintainers and contributors over mandating exceptions over std::expected and requiring process aborts for defects. The PR is Ready for review as a design document but is heavily Disputed on its core approach.
Data
dossier JSON · extract JSON · model ledger, generated 2026-10-13T01:21, confidence high, input hash 0c1b1164d622752e
thread read through 2026-10-12 (35 statements), head 076938eda0
labels: Brainstorming, Docs · draft
thread read through 2026-10-12 (35 statements), head 076938eda0
labels: Brainstorming, Docs · draft
Categories
Documentation
P2 · unblocks #34931
A unified error handling strategy addresses repeated architectural debates across subsystem and validation PRs. The PR description notes that Bitcoin Core lacks a unified error-handling strategy, leading to ad-hoc debates, while enabling guidance across multiple PRs (#34931, #35003, #34132).
Membership: Matches the Docs label and exclusively adds a design document at doc/error-handling.md.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 2