Merge bitcoin/bitcoin#32588: util: Abort on failing CHECK_NONFATAL in debug builds

fa37153288 util: Abort on failing CHECK_NONFATAL in debug builds (MarcoFalke)
fa0dc4bdff test: Allow testing of check failures (MarcoFalke)
faeb58fe66 refactor: Set G_ABORT_ON_FAILED_ASSUME when G_FUZZING_BUILD (MarcoFalke)

Pull request description:

  A failing `CHECK_NONFATAL` will throw an exception. This is fine and even desired in production builds, because the program may catch the exception and give the user a way to easily report the bug upstream.

  However, in debug development builds, exceptions for internal bugs are problematic:

  * The exception could accidentally be caught and silently ignored
  * The exception does not include a full stacktrace, possibly making debugging harder

  Fix all issues by turning the exception into an abort in debug builds.

  This can be tested by reverting the hunks to `src/rpc/node.cpp` and `test/functional/rpc_misc.py` and then running the functional or fuzz tests.

ACKs for top commit:
  achow101:
    ACK fa37153288
  ryanofsky:
    Code review ACK fa37153288, just catching subprocess.CalledProcessError in test fixing up a comment since last review
  stickies-v:
    ACK fa37153288

Tree-SHA512: 2d892b838ccef6f9b25a066e7c2f6cd6f5acc94aad1d91fce62308983bd3f5c5d724897a76de4e3cc5c3678ddadc87e2ee8c87362965373526038e598dfb0101

src/test/CMakeLists.txt

@@ -119,6 +119,7 @@ add_executable(test_bitcoin
   txvalidation_tests.cpp
   txvalidationcache_tests.cpp
   uint256_tests.cpp
+  util_check_tests.cpp
   util_string_tests.cpp
   util_tests.cpp
   util_threadnames_tests.cpp

src/test/fuzz/integer.cpp

@@ -251,9 +251,4 @@ FUZZ_TARGET(integer, .init = initialize_integer)
         } catch (const std::ios_base::failure&) {
         }
     }
-
-    try {
-        CHECK_NONFATAL(b);
-    } catch (const NonFatalCheckError&) {
-    }
 }

src/test/fuzz/rpc.cpp

@@ -381,6 +381,12 @@ FUZZ_TARGET(rpc, .init = initialize_rpc)
         arguments.push_back(ConsumeRPCArgument(fuzzed_data_provider, good_data));
     }
     try {
+        std::optional<test_only_CheckFailuresAreExceptionsNotAborts> maybe_mock{};
+        if (rpc_command == "echo") {
+            // Avoid aborting fuzzing for this specific test-only RPC with an
+            // intentional trigger_internal_bug
+            maybe_mock.emplace();
+        }
         rpc_testing_setup->CallRPC(rpc_command, arguments);
     } catch (const UniValue& json_rpc_error) {
         const std::string error_msg{json_rpc_error.find_value("message").get_str()};

src/test/rpc_tests.cpp

@@ -534,20 +534,23 @@ BOOST_AUTO_TEST_CASE(check_dup_param_names)
     make_rpc({{"p1", NAMED_ONLY}, {"p2", NAMED}});
     make_rpc({{"p1", NAMED_ONLY}, {"p2", NAMED_ONLY}});
 
-    // Error if parameters names are duplicates, unless one parameter is
+    {
-    // positional and the other is named and .also_positional is true.
+        test_only_CheckFailuresAreExceptionsNotAborts mock_checks{};
-    BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", POSITIONAL}}), NonFatalCheckError);
+        // Error if parameter names are duplicates, unless one parameter is
-    make_rpc({{"p1", POSITIONAL}, {"p1", NAMED}});
+        // positional and the other is named and .also_positional is true.
-    BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", NAMED_ONLY}}), NonFatalCheckError);
+        BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", POSITIONAL}}), NonFatalCheckError);
-    make_rpc({{"p1", NAMED}, {"p1", POSITIONAL}});
+        make_rpc({{"p1", POSITIONAL}, {"p1", NAMED}});
-    BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED}}), NonFatalCheckError);
+        BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", NAMED_ONLY}}), NonFatalCheckError);
-    BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED_ONLY}}), NonFatalCheckError);
+        make_rpc({{"p1", NAMED}, {"p1", POSITIONAL}});
-    BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", POSITIONAL}}), NonFatalCheckError);
+        BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED}}), NonFatalCheckError);
-    BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED}}), NonFatalCheckError);
+        BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED_ONLY}}), NonFatalCheckError);
-    BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED_ONLY}}), NonFatalCheckError);
+        BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", POSITIONAL}}), NonFatalCheckError);
+        BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED}}), NonFatalCheckError);
+        BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED_ONLY}}), NonFatalCheckError);
 
-    // Make sure duplicate aliases are detected too.
+        // Make sure duplicate aliases are detected too.
-    BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p2|p1", NAMED_ONLY}}), NonFatalCheckError);
+        BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p2|p1", NAMED_ONLY}}), NonFatalCheckError);
+    }
 }
 
 BOOST_AUTO_TEST_CASE(help_example)

src/test/util_check_tests.cpp

@@ -0,0 +1,33 @@
+// Copyright (c) The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <util/check.h>
+
+#include <boost/test/unit_test.hpp>
+#include <test/util/setup_common.h>
+
+BOOST_AUTO_TEST_SUITE(util_check_tests)
+
+BOOST_AUTO_TEST_CASE(check_pass)
+{
+    Assume(true);
+    Assert(true);
+    CHECK_NONFATAL(true);
+}
+
+BOOST_AUTO_TEST_CASE(check_fail)
+{
+    // Disable aborts for easier testing here
+    test_only_CheckFailuresAreExceptionsNotAborts mock_checks{};
+
+    if constexpr (G_ABORT_ON_FAILED_ASSUME) {
+        BOOST_CHECK_EXCEPTION(Assume(false), NonFatalCheckError, HasReason{"Internal bug detected: false"});
+    } else {
+        BOOST_CHECK_NO_THROW(Assume(false));
+    }
+    BOOST_CHECK_EXCEPTION(Assert(false), NonFatalCheckError, HasReason{"Internal bug detected: false"});
+    BOOST_CHECK_EXCEPTION(CHECK_NONFATAL(false), NonFatalCheckError, HasReason{"Internal bug detected: false"});
+}
+
+BOOST_AUTO_TEST_SUITE_END()

src/util/check.cpp

@@ -27,8 +27,13 @@ NonFatalCheckError::NonFatalCheckError(std::string_view msg, std::string_view fi
 {
 }
 
+bool g_detail_test_only_CheckFailuresAreExceptionsNotAborts{false};
+
 void assertion_fail(std::string_view file, int line, std::string_view func, std::string_view assertion)
 {
+    if (g_detail_test_only_CheckFailuresAreExceptionsNotAborts) {
+        throw NonFatalCheckError{assertion, file, line, func};
+    }
     auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", file, line, func, assertion);
     fwrite(str.data(), 1, str.size(), stderr);
     std::abort();

src/util/check.h

@@ -21,7 +21,7 @@ constexpr bool G_FUZZING_BUILD{
     false
 #endif
 };
-constexpr bool G_ABORT_ON_FAILED_ASSUME{
+constexpr bool G_ABORT_ON_FAILED_ASSUME{G_FUZZING_BUILD ||
 #ifdef ABORT_ON_FAILED_ASSUME
     true
 #else
@@ -46,6 +46,12 @@ inline bool EnableFuzzDeterminism()
     }
 }
 
+extern bool g_detail_test_only_CheckFailuresAreExceptionsNotAborts;
+struct test_only_CheckFailuresAreExceptionsNotAborts {
+    test_only_CheckFailuresAreExceptionsNotAborts() { g_detail_test_only_CheckFailuresAreExceptionsNotAborts = true; };
+    ~test_only_CheckFailuresAreExceptionsNotAborts() { g_detail_test_only_CheckFailuresAreExceptionsNotAborts = false; };
+};
+
 std::string StrFormatInternalBug(std::string_view msg, std::string_view file, int line, std::string_view func);
 
 class NonFatalCheckError : public std::runtime_error
@@ -54,11 +60,17 @@ public:
     NonFatalCheckError(std::string_view msg, std::string_view file, int line, std::string_view func);
 };
 
+/** Internal helper */
+void assertion_fail(std::string_view file, int line, std::string_view func, std::string_view assertion);
+
 /** Helper for CHECK_NONFATAL() */
 template <typename T>
 T&& inline_check_non_fatal(LIFETIMEBOUND T&& val, const char* file, int line, const char* func, const char* assertion)
 {
     if (!val) {
+        if constexpr (G_ABORT_ON_FAILED_ASSUME) {
+            assertion_fail(file, line, func, assertion);
+        }
         throw NonFatalCheckError{assertion, file, line, func};
     }
     return std::forward<T>(val);
@@ -68,14 +80,11 @@ T&& inline_check_non_fatal(LIFETIMEBOUND T&& val, const char* file, int line, co
 #error "Cannot compile without assertions!"
 #endif
 
-/** Helper for Assert() */
-void assertion_fail(std::string_view file, int line, std::string_view func, std::string_view assertion);
-
 /** Helper for Assert()/Assume() */
 template <bool IS_ASSERT, typename T>
 constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* file, [[maybe_unused]] int line, [[maybe_unused]] const char* func, [[maybe_unused]] const char* assertion)
 {
-    if (IS_ASSERT || std::is_constant_evaluated() || G_FUZZING_BUILD || G_ABORT_ON_FAILED_ASSUME) {
+    if (IS_ASSERT || std::is_constant_evaluated() || G_ABORT_ON_FAILED_ASSUME) {
         if (!val) {
             assertion_fail(file, line, func, assertion);
         }

test/functional/rpc_misc.py

@@ -15,6 +15,9 @@ from test_framework.util import (
 
 from test_framework.authproxy import JSONRPCException
 
+import http
+import subprocess
+
 
 class RpcMiscTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -24,11 +27,22 @@ class RpcMiscTest(BitcoinTestFramework):
         node = self.nodes[0]
 
         self.log.info("test CHECK_NONFATAL")
-        assert_raises_rpc_error(
+        msg_internal_bug = 'request.params[9].get_str() != "trigger_internal_bug"'
-            -1,
+        self.restart_node(0)  # Required to flush the chainstate
-            'Internal bug detected: request.params[9].get_str() != "trigger_internal_bug"',
+        try:
-            lambda: node.echo(arg9='trigger_internal_bug'),
+            node.echo(arg9="trigger_internal_bug")
-        )
+            assert False  # Must hit one of the exceptions below
+        except (
+                subprocess.CalledProcessError,
+                http.client.CannotSendRequest,
+                http.client.RemoteDisconnected,
+        ):
+            self.log.info("Restart node after crash")
+            assert_equal(-6, node.process.wait(timeout=10))
+            self.start_node(0)
+        except JSONRPCException as e:
+            assert_equal(e.error["code"], -1)
+            assert f"Internal bug detected: {msg_internal_bug}" 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