#35461 util: Clarify the assertion message in assertion failures (Assert, Assume, etc.)

full analysis

https://github.com/bitcoin/bitcoin/pull/35461 · optout21 · +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

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.

Data

dossier JSON · extract JSON · model openrouter/google/gemini-3.8-flash, generated 2026-09-17T15:53, confidence high, input hash e4d0708c4eb038f4