#35461 util: Clarify the assertion message in assertion failures (Assert, Assume, etc.)
https://github.com/bitcoin/bitcoin/pull/35461 · · +52/-14 in 4 files, 3 commits · labels: Utils/log/libs
Goal
- Make assertion failure messages unambiguous for developers debugging failures
- Ensure nonfatal unreachable checks honor the test abort-on-assume setting
This PR updates assertion failure messages across Assert, Assume, and CHECK_NONFATAL to explicitly state that the condition check failed (for example, wrapping the expression in backticks and appending 'check failed'). It also unifies NONFATAL_UNREACHABLE with CHECK_NONFATAL so that it honors the G_ABORT_ON_FAILED_ASSUME setting in test environments.
Problem: When an assertion fails, the generated error message prints the expression as-is (e.g., 'Internal bug detected: pindex != nullptr'), causing ambiguity for developers regarding whether the printed condition was the expected state or what actually evaluated to true. Additionally, NONFATAL_UNREACHABLE previously bypassed the abort-on-assume configuration used in fuzzing.
Category: Utilities (logging, arguments, libraries) (#61 of 66)
P4 · cleanup
- P4 because the benefit is a minor diagnostic clarity fix for developers
- Affects only internal failure reporting and test configuration consistency
P4 because it is a cosmetic diagnostic improvement to assertion output alongside a minor consistency cleanup for unreachable aborts in test configurations. As l0rinc noted, 'it\'s a tiny difference that we don\'t often expect to encounter, but when we do, it better tell us a realistic story.'
Membership: Modifies internal assertion and check helpers in src/util/check.h and src/util/check.cpp.
Factors: security/stability 0, bug 1, performance 0, user value 0, leverage 0
Reviewability: Ready
- Ready to review
- Clean merge and all reviewer suggestions have been incorporated
The branch merges cleanly, CI is passing, and all review comments from both reviewers have been addressed in recent force-pushes.
Author status: active
Resolved concerns:
- Formatting details: l0rinc requested backtick delimiters and avoided duplicating 'Assertion failed' prefixes in the abort path.
- Macro consistency: maflcko suggested routing NONFATAL_UNREACHABLE through the shared non-fatal failure helper so it respects G_ABORT_ON_FAILED_ASSUME.
- PR description updates to match the final wording and scope.
Agreement: Strong
- Strong consensus with all suggested changes resolved
- Tested and approved following wording and formatting adjustments (l0rinc)
- Supported approach and helped refine unreachable abort helper reuse (maflcko)
Strong: approved by l0rinc and maflcko with all suggested changes incorporated.
Both active reviewers supported the concept and implementation details, with maflcko providing the architectural suggestion for NONFATAL_UNREACHABLE and l0rinc reACKing the final commit structure.
- l0rinc gave tested ACK and code review reACK on 6494a7e1c8 after wording updates
- maflcko approved the approach and helped refine check_non_fatal_fail helper reuse
Review verdicts (DrahtBot): 1
- ACK: l0rinc
Files
25 lines under test/bench/ci.
- src/test/util_check_tests.cpp +19/-3
- src/util/check.h +15/-7
- src/util/check.cpp +16/-3
- test/functional/rpc_misc.py +2/-1
Card
Clarifies assertion and assume failure diagnostics by formatting messages as '`<condition>` check failed' instead of printing bare expressions that confuse expected and actual states. It also unifies NONFATAL_UNREACHABLE with CHECK_NONFATAL to respect G_ABORT_ON_FAILED_ASSUME in test and fuzzing builds. Importance is low as it addresses minor developer diagnostic ambiguity rather than production bugs or performance. The PR is fully reviewed and ready with approvals from l0rinc and maflcko.