#30361 doc: Drop description of LogError messages as fatal
https://github.com/bitcoin/bitcoin/pull/30361 · · +5/-6 in 1 files, 1 commits · labels: Docs
Goal
- Clarify developer notes by redefining LogError as severe admin errors rather than strictly fatal shutdowns
- Resolves mismatch where over half of existing LogError calls in the codebase are non-fatal
This pull request updates developer-notes.md to drop the requirement that LogError be reserved for fatal errors causing a node or subsystem shutdown. It redefines LogError as severe errors requiring administrator action and LogWarning as unexpected conditions indicating potentially severe problems.
Problem: The developer documentation states LogError is only for fatal errors, but less than half of current LogError calls in the codebase actually trigger node shutdown.
Category: Documentation (#9 of 9)
P4 · cleanup
- P4 because adjusting developer guidelines for LogError is an internal cleanup with no direct user impact
- Maintainers prefer fixing call sites in code rather than relaxing documentation standards
P4 because this is an internal developer documentation change about logging conventions that provides no user-visible benefit. Reviewers have pointed out that adjusting guidelines to match past incorrect usages is counterproductive, with ongoing work (#34729, #34730) pursuing better solutions.
Membership: Changes developer logging advice in doc/developer-notes.md
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 0
Reviewability: Stale: Author silent
- Author has been inactive for over 300 days following concept objections
The author has been silent for over eleven months, leaving multiple standing Concept NACKs and a request for status unaddressed.
Author status: silent since 2025-10-15
Open concerns:
- ajtowns and maflcko Concept NACKed weakening documentation to match legacy mistakes rather than fixing the code or consolidating log levels as in #34730.
- sedited retracted their approval, noting the project is closer to the original guidelines and recommending closure.
Resolved concerns:
- Clarification of examples and re-adding the word severe to LogWarning descriptions addressed early review feedback.
Agreement: Blocked
- Unaddressed objection: notes should set standards to fix bad call sites rather than match them (ajtowns)
- Unaddressed objection: guidelines should not be weakened to legitimize imperfect code (maflcko)
- Retracted approval due to standing objections and suggested closing (sedited)
Blocked: ajtowns and maflcko Concept NACKed the approach; author has been silent since October 2025.
Multiple reviewers Concept NACKed weakening developer documentation standards to conform to existing incorrect call sites, and the author has not responded to those blocking objections.
- ajtowns: 'Concept NACK. Most of the issues here... were introduced via scripted-diff in #29236 without putting much thought into whether the old error() and the then-new LogError were a good match.'
- maflcko: 'I am NACK -ish on increasing confusion in the docs, motivated by code that will be removed or changed long-term anyway.'
- sedited: 'Retracting my A-C-K here... Given the two standing N-A-C-Ks, I think this should be closed.'
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| ajtowns | approach | weakens documentation standards to match poor call-site practice and distracts from fixing log levels or consolidating them | open | yes | no | 2026-03-04: 'Concept NACK... logging not-very-important errors as LogError (or even LogWarning) is not a desirable thing for our codebase' |
| maflcko | approach | increases developer confusion and weakens long-term code standards based on temporary improper log calls | open | yes | no | 2024-09-18: 'I am NACK -ish on increasing confusion in the docs, motivated by code that will be removed or changed long-term anyway.' |
| sedited | approach | unnecessary change given progress toward realizing original log level guidelines | open | yes | no | 2026-05-10: 'Retracting my A-C-K here... Given the two standing N-A-C-Ks, I think this should be closed.' |
Support:
- l0rinc: Considers it a slight improvement over the status quo [not substantive]
Participants: ajtowns (objection), maflcko (objection), l0rinc (support), sedited (objection)
State derived from the lists: blocking objection open with no author reply (ajtowns, maflcko, sedited)
Review verdicts (DrahtBot): 0 (+1) -2
Files
0 lines under test/bench/ci.
- doc/developer-notes.md +5/-6
Card
This pull request modifies doc/developer-notes.md to drop the guideline that LogError must be fatal, adapting the documentation to existing codebase call sites. Reviewers including ajtowns and maflcko Concept NACKed the change, arguing developer documentation should set aspirational standards rather than weakening rules to match legacy misuse. The PR is blocked and stale, with the author silent for over eleven months.