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

pull optout21 wants to merge 3 commits into bitcoin:master from optout21:2606-assert-msg changing 5 files +51 −15
  1. optout21 commented at 11:19 AM on June 4, 2026: contributor

    Clarify the message in case of assertion failures. Additionally unify the behavior of NONFATAL_UNREACHABLE w.r.t. abort handling.

    Motivation: This was discussed in #34844 (review)

    The error message generated by Assert, Assume, and CHECK_NONFATAL contains the failing assertion condition text, but it's unclear whether that's the failed expectation or the actual case.

    For example, "Internal bug detected: pindex != nullptr" in fact means that what happened was pindex == nullptr.

    Clarify the situation, by inserting "`%s` check failed".

    The above example is now: "Internal bug detected: `pindex != nullptr` check failed" which is more unambiguous.

    Additionally, change NONFATAL_UNREACHABLE to behave like CHECK_NONFATAL, and honor the special abort setting (G_ABORT_ON_FAILED_ASSUME).

  2. DrahtBot commented at 11:19 AM on June 4, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    External sites

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process.

    Type Reviewers
    ACK l0rinc, maflcko, sedited

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  3. optout21 force-pushed on Jun 4, 2026
  4. DrahtBot added the label CI failed on Jun 4, 2026
  5. optout21 force-pushed on Jun 4, 2026
  6. optout21 commented at 12:33 PM on June 4, 2026: contributor

    Added a test update (following CI failures), condensed util_check test.

  7. DrahtBot removed the label CI failed on Jun 4, 2026
  8. optout21 marked this as ready for review on Jun 4, 2026
  9. in src/test/util_check_tests.cpp:25 in d46f1fae63
      21 | @@ -22,12 +22,25 @@ BOOST_AUTO_TEST_CASE(check_fail)
      22 |      test_only_CheckFailuresAreExceptionsNotAborts mock_checks{};
      23 |  
      24 |      if constexpr (G_ABORT_ON_FAILED_ASSUME) {
      25 | -        BOOST_CHECK_EXCEPTION(Assume(false), NonFatalCheckError, HasReason{"Internal bug detected: false"});
      26 | +        BOOST_CHECK_EXCEPTION(Assume(false), NonFatalCheckError, HasReason{"Internal bug detected: Failed: 'false'"});
    


    l0rinc commented at 7:36 PM on June 4, 2026:

    I don't particularly like the double colon and starting with capital letters in the middle, please consider avoiding the two stacked labels by adding the text after the failure, e.g:

    diff --git a/src/test/util_check_tests.cpp b/src/test/util_check_tests.cpp
    index 49954dff9c..0d944427f8 100644
    --- a/src/test/util_check_tests.cpp
    +++ b/src/test/util_check_tests.cpp
    @@ -22,16 +22,16 @@ BOOST_AUTO_TEST_CASE(check_fail)
         test_only_CheckFailuresAreExceptionsNotAborts mock_checks{};
     
         if constexpr (G_ABORT_ON_FAILED_ASSUME) {
    -        BOOST_CHECK_EXCEPTION(Assume(false), NonFatalCheckError, HasReason{"Internal bug detected: Failed: 'false'"});
    +        BOOST_CHECK_EXCEPTION(Assume(false), NonFatalCheckError, HasReason{"Internal bug detected: `false` check failed"});
         } else {
             BOOST_CHECK_NO_THROW(Assume(false));
         }
    -    BOOST_CHECK_EXCEPTION(Assert(false), NonFatalCheckError, HasReason{"Internal bug detected: Failed: 'false'"});
    -    BOOST_CHECK_EXCEPTION(CHECK_NONFATAL(false), NonFatalCheckError, HasReason{"Internal bug detected: Failed: 'false'"});
    +    BOOST_CHECK_EXCEPTION(Assert(false), NonFatalCheckError, HasReason{"Internal bug detected: `false` check failed"});
    +    BOOST_CHECK_EXCEPTION(CHECK_NONFATAL(false), NonFatalCheckError, HasReason{"Internal bug detected: `false` check failed"});
     
         // Repeat with a more realistic assert condition
         void* this_should_be_nonnull{nullptr};
    -    BOOST_CHECK_EXCEPTION(Assert(this_should_be_nonnull != nullptr), NonFatalCheckError, HasReason{"Internal bug detected: Failed: 'this_should_be_nonnull != nullptr'"});
    +    BOOST_CHECK_EXCEPTION(Assert(this_should_be_nonnull != nullptr), NonFatalCheckError, HasReason{"Internal bug detected: `this_should_be_nonnull != nullptr` check failed"});
     }
     
     BOOST_AUTO_TEST_SUITE_END()
    diff --git a/src/util/check.cpp b/src/util/check.cpp
    index e26f3d7b41..b62520b1b4 100644
    --- a/src/util/check.cpp
    +++ b/src/util/check.cpp
    @@ -17,7 +17,7 @@
     
     std::string StrFormatFailedCheck(std::string_view assertion)
     {
    -    return strprintf("Failed: '%s'", assertion);
    +    return strprintf("`%s` check failed", assertion);
     }
     
     std::string StrFormatInternalBug(std::string_view msg, const std::source_location& loc)
    diff --git a/test/functional/rpc_misc.py b/test/functional/rpc_misc.py
    index 412c9455ce..7fd104a815 100755
    --- a/test/functional/rpc_misc.py
    +++ b/test/functional/rpc_misc.py
    @@ -27,6 +27,7 @@ class RpcMiscTest(BitcoinTestFramework):
     
             self.log.info("test CHECK_NONFATAL")
             msg_internal_bug = 'request.params[9].get_str() != "trigger_internal_bug"'
    +        msg_failed_check = f"`{msg_internal_bug}` check failed"
             self.restart_node(0)  # Required to flush the chainstate
             try:
                 node.echo(arg9="trigger_internal_bug")
    @@ -41,7 +42,7 @@ class RpcMiscTest(BitcoinTestFramework):
                 self.start_node(0)
             except JSONRPCException as e:
                 assert_equal(e.error["code"], -1)
    -            assert f"Internal bug detected: Failed: '{msg_internal_bug}'" in e.error["message"]
    +            assert f"Internal bug detected: {msg_failed_check}" in e.error["message"]
     
             self.log.info("test max arg size")
             ARG_SZ_COMMON = 131071  # Common limit, used previously in the test framework, serves as a regression test
    
  10. in src/util/check.cpp:32 in d46f1fae63
      28 | @@ -29,12 +29,17 @@ NonFatalCheckError::NonFatalCheckError(std::string_view msg, const std::source_l
      29 |  {
      30 |  }
      31 |  
      32 | +NonFatalCheckError NonFatalCheckError::CreateFailedAssert(std::string_view assertion, const std::source_location& loc)
    


    l0rinc commented at 7:38 PM on June 4, 2026:

    Could we avoid adding a public NonFatalCheckError factory? The new formatting is only used by Assert, Assume, and CHECK_NONFATAL, so a helper for the formatted string keeps the generic exception constructor as the only public error API:

    diff --git a/src/test/util_check_tests.cpp b/src/test/util_check_tests.cpp
    index 3a47ecb3c2..49954dff9c 100644
    --- a/src/test/util_check_tests.cpp
    +++ b/src/test/util_check_tests.cpp
    @@ -34,13 +34,4 @@ BOOST_AUTO_TEST_CASE(check_fail)
         BOOST_CHECK_EXCEPTION(Assert(this_should_be_nonnull != nullptr), NonFatalCheckError, HasReason{"Internal bug detected: Failed: 'this_should_be_nonnull != nullptr'"});
     }
     
    -BOOST_AUTO_TEST_CASE(check_exception_constructor)
    -{
    -    const auto assert_text{"pointer != nullptr"};
    -    const auto location{std::source_location::current()};
    -
    -    BOOST_CHECK(std::string_view{NonFatalCheckError("some_message", location).what()}.find("Internal bug detected: some_message\n") != std::string_view::npos);
    -    BOOST_CHECK(std::string_view{NonFatalCheckError::CreateFailedAssert(assert_text, location).what()}.find("Internal bug detected: Failed: 'pointer != nullptr'\n") != std::string_view::npos);
    -}
    -
     BOOST_AUTO_TEST_SUITE_END()
    diff --git a/src/util/check.cpp b/src/util/check.cpp
    index 36885d6db1..e26f3d7b41 100644
    --- a/src/util/check.cpp
    +++ b/src/util/check.cpp
    @@ -15,6 +15,11 @@
     #include <string>
     #include <string_view>
     
    +std::string StrFormatFailedCheck(std::string_view assertion)
    +{
    +    return strprintf("Failed: '%s'", assertion);
    +}
    +
     std::string StrFormatInternalBug(std::string_view msg, const std::source_location& loc)
     {
         return strprintf("Internal bug detected: %s\n%s:%d (%s)\n"
    @@ -29,17 +34,12 @@ NonFatalCheckError::NonFatalCheckError(std::string_view msg, const std::source_l
     {
     }
     
    -NonFatalCheckError NonFatalCheckError::CreateFailedAssert(std::string_view assertion, const std::source_location& loc)
    -{
    -    return NonFatalCheckError(strprintf("Failed: '%s'", assertion), loc);
    -}
    -
     bool g_detail_test_only_CheckFailuresAreExceptionsNotAborts{false};
     
     void assertion_fail(const std::source_location& loc, std::string_view assertion)
     {
         if (g_detail_test_only_CheckFailuresAreExceptionsNotAborts) {
    -        throw NonFatalCheckError::CreateFailedAssert(assertion, loc);
    +        throw NonFatalCheckError{StrFormatFailedCheck(assertion), loc};
         }
         auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", loc.file_name(), loc.line(), loc.function_name(), assertion);
         fwrite(str.data(), 1, str.size(), stderr);
    diff --git a/src/util/check.h b/src/util/check.h
    index 4a7a52b6a8..3ffe8af282 100644
    --- a/src/util/check.h
    +++ b/src/util/check.h
    @@ -57,13 +57,12 @@ struct test_only_CheckFailuresAreExceptionsNotAborts {
     };
     
     std::string StrFormatInternalBug(std::string_view msg, const std::source_location& loc);
    +std::string StrFormatFailedCheck(std::string_view assertion);
     
     class NonFatalCheckError : public std::runtime_error
     {
     public:
         NonFatalCheckError(std::string_view msg, const std::source_location& loc);
    -    /// Create an instance in case of a failed assert condition, message is constructed accordingly
    -    static NonFatalCheckError CreateFailedAssert(std::string_view assertion, const std::source_location& loc);
     };
     
     /** Internal helper */
    @@ -77,7 +76,7 @@ T&& inline_check_non_fatal(LIFETIMEBOUND T&& val, const std::source_location& lo
             if constexpr (G_ABORT_ON_FAILED_ASSUME) {
                 assertion_fail(loc, assertion);
             }
    -        throw NonFatalCheckError::CreateFailedAssert(assertion, loc);
    +        throw NonFatalCheckError{StrFormatFailedCheck(assertion), loc};
         }
         return std::forward<T>(val);
     }
    
  11. in src/util/check.cpp:44 in d46f1fae63
      40 |  {
      41 |      if (g_detail_test_only_CheckFailuresAreExceptionsNotAborts) {
      42 | -        throw NonFatalCheckError{assertion, loc};
      43 | +        throw NonFatalCheckError::CreateFailedAssert(assertion, loc);
      44 |      }
      45 |      auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", loc.file_name(), loc.line(), loc.function_name(), assertion);
    


    l0rinc commented at 7:39 PM on June 4, 2026:

    Should the aborting assertion path use the same clarified wording? Without this, a normal fatal Assert still prints the old Assertion 'condition' failed shape while the exception path says the check failed.

    diff --git a/src/util/check.cpp b/src/util/check.cpp
    --- a/src/util/check.cpp	(revision e8c1542b132f855eecb2cf621eb280cd35d63cf0)
    +++ b/src/util/check.cpp	(revision 4ffb91e3bbf0a06166b2a7bbd7f543d2f5c6b36d)
    @@ -41,7 +41,7 @@
         if (g_detail_test_only_CheckFailuresAreExceptionsNotAborts) {
             throw NonFatalCheckError{StrFormatFailedCheck(assertion), loc};
         }
    -    auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", loc.file_name(), loc.line(), loc.function_name(), assertion);
    +    auto str = strprintf("%s:%s %s: Assertion failed: %s.\n", loc.file_name(), loc.line(), loc.function_name(), StrFormatFailedCheck(assertion));
         fwrite(str.data(), 1, str.size(), stderr);
         std::abort();
     }
    

    optout21 commented at 12:44 PM on June 17, 2026:

    Good point. Unfortunately, this is not possible/easy to test in a unit test (due to the abort).

  12. in src/test/util_check_tests.cpp:35 in d46f1fae63 outdated
      33 | +    BOOST_CHECK_EXCEPTION(CHECK_NONFATAL(false), NonFatalCheckError, HasReason{"Internal bug detected: Failed: 'false'"});
      34 | +
      35 | +    // Repeat with a more realistic assert condition
      36 | +    void* this_should_be_nonnull{nullptr};
      37 | +    BOOST_CHECK_EXCEPTION(Assert(this_should_be_nonnull != nullptr), NonFatalCheckError, HasReason{"Internal bug detected: Failed: 'this_should_be_nonnull != nullptr'"});
      38 | +}
    


    l0rinc commented at 7:41 PM on June 4, 2026:

    Could this test the note about non-assert errors directly? Checking NONFATAL_UNREACHABLE keeps the coverage tied to the behavior reviewers need to preserve, instead of only exercising the constructor plumbing.

    diff --git a/src/test/util_check_tests.cpp b/src/test/util_check_tests.cpp
    --- a/src/test/util_check_tests.cpp	(revision 4ffb91e3bbf0a06166b2a7bbd7f543d2f5c6b36d)
    +++ b/src/test/util_check_tests.cpp	(revision 014fae1a7202498a6e9022a1f4215b0acf23cc9c)
    @@ -9,6 +9,13 @@
     
     BOOST_AUTO_TEST_SUITE(util_check_tests)
     
    +namespace {
    +void TriggerNonFatalUnreachable()
    +{
    +    NONFATAL_UNREACHABLE();
    +}
    +} // namespace
    +
     BOOST_AUTO_TEST_CASE(check_pass)
     {
         Assume(true);
    @@ -34,4 +41,9 @@
         BOOST_CHECK_EXCEPTION(Assert(this_should_be_nonnull != nullptr), NonFatalCheckError, HasReason{"Internal bug detected: `this_should_be_nonnull != nullptr` check failed"});
     }
     
    +BOOST_AUTO_TEST_CASE(unreachable_diagnostic_unchanged)
    +{
    +    BOOST_CHECK_EXCEPTION(TriggerNonFatalUnreachable(), NonFatalCheckError, HasReason{"Internal bug detected: Unreachable code reached (non-fatal)"});
    +}
    +
     BOOST_AUTO_TEST_SUITE_END()
    
  13. l0rinc approved
  14. l0rinc commented at 7:45 PM on June 4, 2026: contributor

    Concept ACK, it's a tiny difference that we don't often expect to encounter, but when we do, it better tell us a realistic story.

    Left a few nits about slightly clearer phrasing and leaving the new formatter private.

    <details><summary>Local patch</summary>

    diff --git a/src/test/util_check_tests.cpp b/src/test/util_check_tests.cpp
    index 3a47ecb3c2..8a92376608 100644
    --- a/src/test/util_check_tests.cpp
    +++ b/src/test/util_check_tests.cpp
    @@ -9,6 +9,13 @@
     
     BOOST_AUTO_TEST_SUITE(util_check_tests)
     
    +namespace {
    +void TriggerNonFatalUnreachable()
    +{
    +    NONFATAL_UNREACHABLE();
    +}
    +} // namespace
    +
     BOOST_AUTO_TEST_CASE(check_pass)
     {
         Assume(true);
    @@ -22,25 +29,21 @@ BOOST_AUTO_TEST_CASE(check_fail)
         test_only_CheckFailuresAreExceptionsNotAborts mock_checks{};
     
         if constexpr (G_ABORT_ON_FAILED_ASSUME) {
    -        BOOST_CHECK_EXCEPTION(Assume(false), NonFatalCheckError, HasReason{"Internal bug detected: Failed: 'false'"});
    +        BOOST_CHECK_EXCEPTION(Assume(false), NonFatalCheckError, HasReason{"Internal bug detected: `false` check failed"});
         } else {
             BOOST_CHECK_NO_THROW(Assume(false));
         }
    -    BOOST_CHECK_EXCEPTION(Assert(false), NonFatalCheckError, HasReason{"Internal bug detected: Failed: 'false'"});
    -    BOOST_CHECK_EXCEPTION(CHECK_NONFATAL(false), NonFatalCheckError, HasReason{"Internal bug detected: Failed: 'false'"});
    +    BOOST_CHECK_EXCEPTION(Assert(false), NonFatalCheckError, HasReason{"Internal bug detected: `false` check failed"});
    +    BOOST_CHECK_EXCEPTION(CHECK_NONFATAL(false), NonFatalCheckError, HasReason{"Internal bug detected: `false` check failed"});
     
         // Repeat with a more realistic assert condition
         void* this_should_be_nonnull{nullptr};
    -    BOOST_CHECK_EXCEPTION(Assert(this_should_be_nonnull != nullptr), NonFatalCheckError, HasReason{"Internal bug detected: Failed: 'this_should_be_nonnull != nullptr'"});
    +    BOOST_CHECK_EXCEPTION(Assert(this_should_be_nonnull != nullptr), NonFatalCheckError, HasReason{"Internal bug detected: `this_should_be_nonnull != nullptr` check failed"});
     }
     
    -BOOST_AUTO_TEST_CASE(check_exception_constructor)
    +BOOST_AUTO_TEST_CASE(unreachable_diagnostic_unchanged)
     {
    -    const auto assert_text{"pointer != nullptr"};
    -    const auto location{std::source_location::current()};
    -
    -    BOOST_CHECK(std::string_view{NonFatalCheckError("some_message", location).what()}.find("Internal bug detected: some_message\n") != std::string_view::npos);
    -    BOOST_CHECK(std::string_view{NonFatalCheckError::CreateFailedAssert(assert_text, location).what()}.find("Internal bug detected: Failed: 'pointer != nullptr'\n") != std::string_view::npos);
    +    BOOST_CHECK_EXCEPTION(TriggerNonFatalUnreachable(), NonFatalCheckError, HasReason{"Internal bug detected: Unreachable code reached (non-fatal)"});
     }
     
     BOOST_AUTO_TEST_SUITE_END()
    diff --git a/src/util/check.cpp b/src/util/check.cpp
    index 36885d6db1..f67fb5374f 100644
    --- a/src/util/check.cpp
    +++ b/src/util/check.cpp
    @@ -15,6 +15,11 @@
     #include <string>
     #include <string_view>
     
    +std::string StrFormatFailedCheck(std::string_view assertion)
    +{
    +    return strprintf("`%s` check failed", assertion);
    +}
    +
     std::string StrFormatInternalBug(std::string_view msg, const std::source_location& loc)
     {
         return strprintf("Internal bug detected: %s\n%s:%d (%s)\n"
    @@ -29,19 +34,14 @@ NonFatalCheckError::NonFatalCheckError(std::string_view msg, const std::source_l
     {
     }
     
    -NonFatalCheckError NonFatalCheckError::CreateFailedAssert(std::string_view assertion, const std::source_location& loc)
    -{
    -    return NonFatalCheckError(strprintf("Failed: '%s'", assertion), loc);
    -}
    -
     bool g_detail_test_only_CheckFailuresAreExceptionsNotAborts{false};
     
     void assertion_fail(const std::source_location& loc, std::string_view assertion)
     {
         if (g_detail_test_only_CheckFailuresAreExceptionsNotAborts) {
    -        throw NonFatalCheckError::CreateFailedAssert(assertion, loc);
    +        throw NonFatalCheckError{StrFormatFailedCheck(assertion), loc};
         }
    -    auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", loc.file_name(), loc.line(), loc.function_name(), assertion);
    +    auto str = strprintf("%s:%s %s: Assertion failed: %s.\n", loc.file_name(), loc.line(), loc.function_name(), StrFormatFailedCheck(assertion));
         fwrite(str.data(), 1, str.size(), stderr);
         std::abort();
     }
    diff --git a/src/util/check.h b/src/util/check.h
    index 4a7a52b6a8..3ffe8af282 100644
    --- a/src/util/check.h
    +++ b/src/util/check.h
    @@ -57,13 +57,12 @@ struct test_only_CheckFailuresAreExceptionsNotAborts {
     };
     
     std::string StrFormatInternalBug(std::string_view msg, const std::source_location& loc);
    +std::string StrFormatFailedCheck(std::string_view assertion);
     
     class NonFatalCheckError : public std::runtime_error
     {
     public:
         NonFatalCheckError(std::string_view msg, const std::source_location& loc);
    -    /// Create an instance in case of a failed assert condition, message is constructed accordingly
    -    static NonFatalCheckError CreateFailedAssert(std::string_view assertion, const std::source_location& loc);
     };
     
     /** Internal helper */
    @@ -77,7 +76,7 @@ T&& inline_check_non_fatal(LIFETIMEBOUND T&& val, const std::source_location& lo
             if constexpr (G_ABORT_ON_FAILED_ASSUME) {
                 assertion_fail(loc, assertion);
             }
    -        throw NonFatalCheckError::CreateFailedAssert(assertion, loc);
    +        throw NonFatalCheckError{StrFormatFailedCheck(assertion), loc};
         }
         return std::forward<T>(val);
     }
    diff --git a/test/functional/rpc_misc.py b/test/functional/rpc_misc.py
    index 412c9455ce..7fd104a815 100755
    --- a/test/functional/rpc_misc.py
    +++ b/test/functional/rpc_misc.py
    @@ -27,6 +27,7 @@ class RpcMiscTest(BitcoinTestFramework):
     
             self.log.info("test CHECK_NONFATAL")
             msg_internal_bug = 'request.params[9].get_str() != "trigger_internal_bug"'
    +        msg_failed_check = f"`{msg_internal_bug}` check failed"
             self.restart_node(0)  # Required to flush the chainstate
             try:
                 node.echo(arg9="trigger_internal_bug")
    @@ -41,7 +42,7 @@ class RpcMiscTest(BitcoinTestFramework):
                 self.start_node(0)
             except JSONRPCException as e:
                 assert_equal(e.error["code"], -1)
    -            assert f"Internal bug detected: Failed: '{msg_internal_bug}'" in e.error["message"]
    +            assert f"Internal bug detected: {msg_failed_check}" in e.error["message"]
     
             self.log.info("test max arg size")
             ARG_SZ_COMMON = 131071  # Common limit, used previously in the test framework, serves as a regression test
    

    </details>

  15. optout21 renamed this:
    Clarify the assertion message in Assert, Assume and CHECK_NONFATAL
    util: Clarify the assertion message in Assert, Assume and CHECK_NONFATAL
    on Jun 4, 2026
  16. DrahtBot added the label Utils/log/libs on Jun 4, 2026
  17. optout21 commented at 12:46 PM on June 17, 2026: contributor

    Thanks for reviewing, @l0rinc; suggestions applied. Summary of changes:

    • Wording: Get rid of double prefix, "Internal bug detected: 'pindex != nullptr' check failed" instead of "Internal bug detected: Failed: 'pindex != nullptr'"
    • For formatting, use a standalone helper instead of a special factory for the exception
    • In assertion_fail, for the non-test-only abort path, also use the same wording.
    • Test: add test for NONFATAL_UNREACHABLE (no assert).

    (git diff d46f1fae63ab8d81731ccf0a563119d9edeeeef8 00b0b0f68a825864481103a16caf2a64559f6951)

  18. optout21 force-pushed on Jun 17, 2026
  19. optout21 force-pushed on Jun 17, 2026
  20. optout21 force-pushed on Jun 17, 2026
  21. optout21 force-pushed on Jun 17, 2026
  22. DrahtBot added the label CI failed on Jun 17, 2026
  23. optout21 force-pushed on Jun 17, 2026
  24. DrahtBot removed the label CI failed on Jun 17, 2026
  25. sedited requested review from l0rinc on Aug 27, 2026
  26. in src/test/util_check_tests.cpp:32 in 952c64ca50
      28 | @@ -22,12 +29,21 @@ BOOST_AUTO_TEST_CASE(check_fail)
      29 |      test_only_CheckFailuresAreExceptionsNotAborts mock_checks{};
      30 |  
      31 |      if constexpr (G_ABORT_ON_FAILED_ASSUME) {
      32 | -        BOOST_CHECK_EXCEPTION(Assume(false), NonFatalCheckError, HasReason{"Internal bug detected: false"});
      33 | +        BOOST_CHECK_EXCEPTION(Assume(false), NonFatalCheckError, HasReason{"Internal bug detected: 'false' check failed"});
    


    l0rinc commented at 5:51 PM on August 27, 2026:

    Single quotes around a stringized check collide with character literals in real conditions such as Assert(cmd.at(0) != '-'), yielding ... 'cmd.at(0) != '-'' check failed.

    Could we rather use backticks as the outer delimiters so the condition remains visually distinct?


    optout21 commented at 3:36 PM on September 1, 2026:

    Good point, applied.


    l0rinc commented at 4:57 PM on September 7, 2026:

    Please update the PR description after this change

  27. in src/util/check.cpp:44 in 952c64ca50
      38 | @@ -34,9 +39,9 @@ bool g_detail_test_only_CheckFailuresAreExceptionsNotAborts{false};
      39 |  void assertion_fail(const std::source_location& loc, std::string_view assertion)
      40 |  {
      41 |      if (g_detail_test_only_CheckFailuresAreExceptionsNotAborts) {
      42 | -        throw NonFatalCheckError{assertion, loc};
      43 | +        throw NonFatalCheckError{StrFormatFailedCheck(assertion), loc};
      44 |      }
      45 | -    auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", loc.file_name(), loc.line(), loc.function_name(), assertion);
      46 | +    auto str = strprintf("%s:%s %s: Assertion failed: %s.\n", loc.file_name(), loc.line(), loc.function_name(), StrFormatFailedCheck(assertion));
    


    l0rinc commented at 5:52 PM on August 27, 2026:

    The fatal path currently renders "Assertion failed: <condition> check failed", repeating the same outcome around the condition.

    Could we render "Assertion <condition> check failed" once?

        auto str = strprintf("%s:%s %s: Assertion %s.\n", loc.file_name(), loc.line(), loc.function_name(), StrFormatFailedCheck(assertion));
    

    optout21 commented at 3:36 PM on September 1, 2026:

    Applied.

  28. l0rinc commented at 5:53 PM on August 27, 2026: contributor

    Forgot about this one, please ping me next time if I don't reply in time (thanks @sedited)

  29. optout21 commented at 3:37 PM on September 1, 2026: contributor

    Thanks for the review, @l0rinc; minor message string changes applied.

    git diff 952c64ca50be450ed6288ca85a5a092a85001b42 4b63136bea857e8133c0ff3885fa9ec8d83c03ad

  30. optout21 force-pushed on Sep 1, 2026
  31. DrahtBot added the label CI failed on Sep 1, 2026
  32. DrahtBot commented at 4:49 PM on September 1, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task Alpine (musl): https://github.com/bitcoin/bitcoin/actions/runs/33526907826/job/99919961765</sub> <sub>LLM reason (✨ experimental): CI failed due to a compiler crash (internal compiler error: Segmentation fault) while building the fuzz target (cmpctblock.cpp).</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  33. l0rinc commented at 5:43 PM on September 1, 2026: contributor

    tested ACK 4b63136bea857e8133c0ff3885fa9ec8d83c03ad

    The Alpine failure seems unrelated. The PR description still shows the old Failed: '<condition>' wording and should be updated to match the current output.

  34. in src/test/util_check_tests.cpp:46 in 4b63136bea outdated
      44 | +    BOOST_CHECK_EXCEPTION(Assert(this_should_be_nonnull != nullptr), NonFatalCheckError, HasReason{"Internal bug detected: `this_should_be_nonnull != nullptr` check failed"});
      45 | +}
      46 | +
      47 | +BOOST_AUTO_TEST_CASE(unreachable_diagnostics)
      48 | +{
      49 | +    BOOST_CHECK_EXCEPTION(TriggerNonFatalUnreachable(), NonFatalCheckError, HasReason{"Internal bug detected: Unreachable code reached (non-fatal)"});
    


    maflcko commented at 6:46 PM on September 1, 2026:

    Since you are adding this test, I wonder if the impl should be #define NONFATAL_UNREACHABLE inline_check_non_fatal(false, current(), "unreachable"), so that G_ABORT_ON_FAILED_ASSUME is recognized, just like for CHECK_NONFATAL.


    optout21 commented at 10:11 AM on September 2, 2026:

    This suggestion makes sense, however I'm a bit reluctant to include it in this PR, which is only about message rewording. I did the suggestion here: https://github.com/optout21/bitcoin/commit/5ac866eb21736deb7f6a616aebd066e6d8ec1a80

    Please advise (change it / make new PR with it / include in this PR).


    maflcko commented at 10:23 AM on September 2, 2026:

    I really think this can just be a call to inline_check_non_fatal(false, current(), "unreachable") with the benefits:

    • If the G_ABORT_ON_FAILED_ASSUME logic ever changes again in the future, it is more likely to apply to all places. (Less code, logic and functions to keep track of)
    • It is less code and the compiler can inline and flatten the false literal by itself.
    • All of the macros here are for internal bugs, which should never happen, so the "unreachable" string is fine and sufficient for a dev-only debug-only string.

    Personally I think it is fine to cycle this into this pull, but no strong opinion.


    optout21 commented at 11:34 AM on September 2, 2026:

    A technicality: the inline_check_non_fatal was duplicated into a [[noreturn]] check_non_fatal_unreachable to avoid the compiler giving "control reaches end of non-void function [-Wreturn-type]" warnings in a few places where NONFATAL_UNREACHABLE is used. While the macro was a simple throw, it could be used as a last statement in a non-void method; with the new if this is not clear for the compiler, hence the need for a [[noreturn]] marker (which cannot be applied on inline_check_non_fatal).


    maflcko commented at 11:53 AM on September 2, 2026:

    Right. In that case, it could make sense to have a single [[noreturn]] void check_non_fatal_fai(const std::source_location& loc, std::string_view err_msg); helper. And then call it in both places:

    • In inline_check_non_fatal: check_non_fatal_fai(loc, assertion);
    • And in: #define NONFATAL_UNREACHABLE check_non_fatal_fai( current(), "unreachable")

    optout21 commented at 5:08 PM on September 3, 2026:

    Suggestion applied.

  35. in src/test/util_check_tests.cpp:39 in 4b63136bea
      37 | -    BOOST_CHECK_EXCEPTION(Assert(false), NonFatalCheckError, HasReason{"Internal bug detected: false"});
      38 | -    BOOST_CHECK_EXCEPTION(CHECK_NONFATAL(false), NonFatalCheckError, HasReason{"Internal bug detected: false"});
      39 | +    BOOST_CHECK_EXCEPTION(Assert(false), NonFatalCheckError, HasReason{"Internal bug detected: `false` check failed"});
      40 | +    BOOST_CHECK_EXCEPTION(CHECK_NONFATAL(false), NonFatalCheckError, HasReason{"Internal bug detected: `false` check failed"});
      41 | +
      42 | +    // Repeat with a more realistic assert condition
    


    maflcko commented at 6:47 PM on September 1, 2026:

    nit: Probably no need to repeat this. Could just adjust the above?


    optout21 commented at 9:55 AM on September 2, 2026:

    Applied.

  36. maflcko approved
  37. maflcko commented at 6:49 PM on September 1, 2026: member

    lgtm, left two nits

  38. optout21 commented at 8:45 AM on September 2, 2026: contributor

    The PR description still shows the old [...] wording ...

    Done. (Thanks for the hint; commit message was updated but not the PR desc.)

  39. optout21 commented at 10:12 AM on September 2, 2026: contributor

    Thanks for the comments, @maflcko . One nit applied, the other I've tried, but haven't included currently.

  40. optout21 force-pushed on Sep 2, 2026
  41. DrahtBot removed the label CI failed on Sep 2, 2026
  42. optout21 marked this as a draft on Sep 3, 2026
  43. optout21 force-pushed on Sep 3, 2026
  44. optout21 renamed this:
    util: Clarify the assertion message in Assert, Assume and CHECK_NONFATAL
    util: Clarify the assertion message in assertion failures (Assert, Assume, etc.)
    on Sep 3, 2026
  45. optout21 commented at 11:57 AM on September 3, 2026: contributor

    Reworked based on @maflcko's suggestion, and unified the non-fatal unreachable and check behavior, so that unreachable also honors special abort setting (G_ABORT_ON_FAILED_ASSUME). Added two more commits (but also adjusted the first to be in sync with the latter ones).

    • Error cases with an assert include the "'condition' check failed" message (but not the unreachable error).
    • Non-fatal unreachable and check cases throw, but abort if G_ABORT_ON_FAILED_ASSUME is set (and thwow if test g_detail_test_only_CheckFailuresAreExceptionsNotAborts is set).
    • The condition checking G_ABORT_ON_FAILED_ASSUME has been extracted into a new helper check_non_fatal_fail(), so it can be reused.
    • The internal helper assertion_fail() has been renamed to the more descriptive internal_abort_helper().
  46. DrahtBot added the label CI failed on Sep 3, 2026
  47. optout21 marked this as ready for review on Sep 3, 2026
  48. optout21 commented at 5:08 PM on September 3, 2026: contributor

    Ready for review. CI failure seems unrelated (MacOS, device out of space).

  49. optout21 requested review from l0rinc on Sep 3, 2026
  50. optout21 requested review from maflcko on Sep 3, 2026
  51. optout21 closed this on Sep 7, 2026

  52. optout21 reopened this on Sep 7, 2026

  53. l0rinc commented at 4:58 PM on September 7, 2026: contributor

    code review reACK 6494a7e1c863321a5d787ff0e50a51a2e34e9970
    The PR description still needs update to use backticks and about NONFATAL_UNREACHABLE not being affected

  54. optout21 commented at 5:36 PM on September 7, 2026: contributor

    The PR description still needs update

    Description updated, remaining inconsistencies fixed, thanks.

  55. DrahtBot removed the label CI failed on Sep 7, 2026
  56. optout21 commented at 11:31 AM on October 19, 2026: contributor

    Still awaiting review.

  57. in src/util/check.cpp:44 in 0d67983acd
      42 |      if (g_detail_test_only_CheckFailuresAreExceptionsNotAborts) {
      43 | -        throw NonFatalCheckError{assertion, loc};
      44 | +        throw NonFatalCheckError{error_msg, loc};
      45 |      }
      46 | -    auto str = strprintf("%s:%s %s: Assertion: %s.\n", loc.file_name(), loc.line(), loc.function_name(), assertion);
      47 | +    auto str = strprintf("%s:%s %s: Assertion: %s.\n", loc.file_name(), loc.line(), loc.function_name(), error_msg);
    


    maflcko commented at 1:23 PM on October 19, 2026:

    nit in 0d67983acdfe3ccdb9147994ca05383f5c0d1d41: I think users will see that the program aborted after writing this, so the Assertion: could also be removed. This means the msgs will be:

    • [location] bla check failed.
    • and [location] unreachable.

    which seems fine.


    optout21 commented at 6:36 PM on October 20, 2026:

    Good point, taken

  58. in src/util/check.h:72 in 6494a7e1c8
      67 | @@ -68,15 +68,15 @@ class NonFatalCheckError : public std::runtime_error
      68 |  /// Internal helper. The noreturn enables optimizers to discard invalid paths.
      69 |  [[noreturn]] void internal_abort_helper(const std::source_location& loc, std::string_view error_msg);
      70 |  
      71 | +/** Helper for CHECK_NONFATAL() and NONFATAL_UNREACHABLE() */
      72 | +[[noreturn]] void check_non_fatal_fail(const std::source_location& loc, std::string_view error_msg);
    


    maflcko commented at 1:26 PM on October 19, 2026:

    nit in 6494a7e1c863321a5d787ff0e50a51a2e34e9970: This is my fault for suggesting the name, but I think a better name would be internal_non_fatal_helper. with the docstring:

    /// Internal helper. The noreturn enables optimizers to discard invalid paths.
    

    (like above)

    There is no need to list all call sites. If someone wanted to know them, they could grep.


    optout21 commented at 6:37 PM on October 20, 2026:

    Done

  59. in src/util/check.h:139 in 6494a7e1c8
     132 | @@ -130,9 +133,13 @@ constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] con
     133 |  
     134 |  /**
     135 |   * NONFATAL_UNREACHABLE() is a macro that is used to mark unreachable code. It throws a NonFatalCheckError.
     136 | + * Control flow will not continue past this macro.
     137 | + *
     138 | + * Note: in certain test environments an abort is triggered instead
     139 | + * (in fuzz tests, when ABORT_ON_FAILED_ASSUME is set, etc.; see G_ABORT_ON_FAILED_ASSUME).
    


    maflcko commented at 1:32 PM on October 19, 2026:

    I think it would be good to keep the docs symmetric with Assume which uses the wording "In debug builds it behaves ...`. So the wording here could simply be: "In debug builds an abort is triggered"


    optout21 commented at 6:37 PM on October 20, 2026:

    Done

  60. in src/util/check.h:114 in 6494a7e1c8
     108 | @@ -109,6 +109,9 @@ constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] con
     109 |   * For example in RPC code, where it is undesirable to crash the whole program, this can be generally used to replace
     110 |   * asserts or recoverable logic errors. A NonFatalCheckError in RPC code is caught and passed as a string to the RPC
     111 |   * caller, which can then report the issue to the developers.
     112 | + *
     113 | + * Note: in certain test environments an abort is triggered instead
     114 | + * (in fuzz tests, when ABORT_ON_FAILED_ASSUME is set, etc.; see G_ABORT_ON_FAILED_ASSUME).
    


    maflcko commented at 1:32 PM on October 19, 2026:

    6494a7e1c863321a5d787ff0e50a51a2e34e9970: Same


    optout21 commented at 6:37 PM on October 20, 2026:

    Done

  61. maflcko approved
  62. maflcko commented at 1:33 PM on October 19, 2026: member

    review ACK 6494a7e1c863321a5d787ff0e50a51a2e34e9970 🍅

    <details><summary>Show signature</summary>

    Signature:

    untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    trusted comment: review ACK 6494a7e1c863321a5d787ff0e50a51a2e34e9970 🍅
    l6phmxcotFO8icaJX33SEkPwdw6zXxX8HiXEDwNgGOJhlTrcCiamd4Rl8qiEe5pheXzoWxWaxIC3j2SZ/LZkDg==
    

    </details>

  63. l0rinc commented at 4:09 PM on October 19, 2026: contributor

    Happy to re-review if you take the suggestions

  64. optout21 force-pushed on Oct 20, 2026
  65. optout21 commented at 6:45 PM on October 20, 2026: contributor

    @maflcko, thanks for review suggestions, reviewed and applied all.

    git range-diff 6494a7e1c863321a5d787ff0e50a51a2e34e9970...704ce3a7adb7d2b67082c5fe8fc328a2d2d43891

  66. l0rinc commented at 7:05 PM on October 20, 2026: contributor

    Could you rebase and update util_not_null_tests to expect Internal bug detected: `m_ptr != nullptr` check failed? The stale expectation is causing the CI failures.

    Please also drop the description's Note: … are not affected sentence, since the NONFATAL_UNREACHABLE message changes to unreachable.

  67. util: In Assert clarify the failing condition in the message
    The error message generated by Assert, Assume, and CHECK_NONFATAL
    contained the failing assertion condition text, but it was
    unclear whether that's the failed expectation or the
    actual case. E.g. "Internal bug detected: pindex != nullptr"
    meant that pindex == nullptr.
    
    Clarify the situation, by including "`%s` check failed".
    The above example is now:
    "Internal bug detected: `pindex != nullptr` check failed".
    2e3a53464d
  68. util: Rename assertion_fail() -> internal_abort_helper()
    Rename only. Use a more descriptive naming, the helper is for abort.
    aa64d77ce8
  69. util: Unify assertion and unreachable handling
    Unreachable error also honors `G_ABORT_ON_FAILED_ASSUME`, and if set,
    it aborts instead of throwing.
    The condition checking `G_ABORT_ON_FAILED_ASSUME` has been extracted
    into a new helper, `internal_non_fatal_helper()`, and it is reused.
    81fde00cc7
  70. optout21 force-pushed on Oct 20, 2026
  71. optout21 commented at 7:21 PM on October 20, 2026: contributor

    A recently introduced unit test failed in CI (util_not_null_tests). Rebased and adjusted the test. Also adjusted the description, according to @l0rinc's finding.

  72. DrahtBot added the label CI failed on Oct 20, 2026
  73. DrahtBot commented at 7:21 PM on October 20, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task macOS native: https://github.com/bitcoin/bitcoin/actions/runs/39477470807/job/118817050199</sub> <sub>LLM reason (✨ experimental): CI failed because util_not_null_tests had a failing exception check in check_null_smart_pointer (the HasReason validation raised NonFatalCheckError).</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  74. optout21 requested review from maflcko on Oct 20, 2026
  75. DrahtBot removed the label CI failed on Oct 20, 2026
  76. l0rinc commented at 8:22 PM on October 20, 2026: contributor

    ACK 81fde00cc7ff2c5ae85e8601d8b780ddde7dc2a6

    Thanks for the quick updates!

  77. maflcko commented at 7:50 AM on October 21, 2026: member

    review ACK 81fde00cc7ff2c5ae85e8601d8b780ddde7dc2a6 🎷

    <details><summary>Show signature</summary>

    Signature:

    untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    trusted comment: review ACK 81fde00cc7ff2c5ae85e8601d8b780ddde7dc2a6 🎷
    0TxKh5I3IF3ukLe+anKfRPCtJElRV6DwBpCPbZXz5fiU6BBn0vnJjPF8xW7/xt4Sc0ox0p0EVEs+9iHmvPIQAw==
    

    </details>

  78. sedited approved
  79. sedited commented at 8:35 AM on October 21, 2026: contributor

    ACK 81fde00cc7ff2c5ae85e8601d8b780ddde7dc2a6

  80. sedited merged this on Oct 21, 2026
  81. sedited closed this on Oct 21, 2026


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-10-30 09:40 UTC