util: Abort on failing CHECK_NONFATAL in debug builds #32588

maflcko · 2025-05-22T13:29:51Z

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.

DrahtBot · 2025-05-22T13:29:55Z

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32588.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot · 2025-05-22T14:34:58Z

🚧 At least one of the CI tasks failed.
_{Task multiprocess, i686, DEBUG: https://github.com/bitcoin/bitcoin/runs/42714583822}
_{LLM reason (✨ experimental): The CI failure is due to the "rpc_tests" subprocess aborting during the test execution.}

Hints

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.

ryanofsky · 2025-05-22T19:16:49Z

Concept ACK. Nice idea, and it does seem useful to have a macro checking for unexpected but not very serious conditions by throwing an exception that gets reported in release builds but is a fatal error in debug builds. And current uses of the macro seem like good candidates for that behavior.

The only possible issues I see are:

(1) The name CHECK_NONFATAL doesn't make a lot of sense anymore, now triggering fatal errors when it literally says "nonfatal" in the name.
(2) It is now more cumbersome to write unit tests checking for these conditions since they require a release build to run.

Both could be addressed in followups. Issue (2) could be addressed by having a g_abort_hook or similar hook allowing specific unit tests to write custom code to check for these errors if they want. (This could also be used to replace the g_debug_lockorder_abort variable which does something similar.)

IMO, issue (1) would be nice to address by coming up with a better designed set of checking macros and starting to use them. I think it could be good to have a:

CHECK to check conditions and abort if false in all builds
DCHECK to do the same but be compiled out of release builds,
CHECK_LOG to log an "internal bug detected please report" type log message in release builds, and abort in debug builds
CHECK_THROW to throw an exception in release builds, and abort in log builds.

Then, current assert uses could become CHECK, current Assume uses in hotspots could become DCHECK, majority of other Assume uses could become CHECK_LOG, and current CHECK_NONFATAL uses could become CHECK_THROW.

Just a thought though. Maybe current names are not a real problem, and naming shouldn't block this PR in any case.

maflcko · 2025-05-23T06:34:56Z

CHECK_THROW

I don't think this solves issue (1). Instead of NONFATAL being inaccurately named in debug builds, it will now be THROW, because the check is neither nonfatal nor throwing in debug builds.

Then, current assert uses could become CHECK, current Assume uses in hotspots could become DCHECK, majority of other Assume uses could become CHECK_LOG, and current CHECK_NONFATAL uses could become CHECK_THROW.

No objection, just mentioning that this would be a larger diff (including link-time changes, which are for some reason more involved in this area (#26688 (comment), #32543 (comment))), so a separate discussion/issue/pull seems better.

cumbersome to write unit tests

Thx, pushed a commit to fix this.

achow101 · 2025-06-02T20:28:49Z

Concept ACK

ryanofsky

Code review ACK faae9a2. I think this is a good change. It makes sense conceptually to have check macros that always abort in debug builds, but do different things depending on cost of the check & severity of the error in release builds.

re: #32588 (comment)

I don't think this solves issue (1). Instead of NONFATAL being inaccurately named in debug builds, it will now be THROW, because the check is neither nonfatal nor throwing in debug builds.

IMO it does solve it, because the current issue is that the macro is literally doing the thing its name says it will not do (trigger a fatal error). By contrast, I don't think it is a problem for function name to just describe its primary purpose and not everything else it may do. No need to solve everything here though. Current PR seems like a step forward.

src/rpc/node.cpp

ryanofsky

Code review ACK fa68bda. Looks good! Since last review added some more test coverage and reverted some unneeded changes.

src/test/fuzz/rpc.cpp

src/rpc/node.cpp

This does not change behavior, but documents that G_ABORT_ON_FAILED_ASSUME is set when G_FUZZING_BUILD.

DrahtBot · 2025-07-24T11:34:10Z

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/runs/46638610833}
_{LLM reason (✨ experimental): The CI failure is caused by a lint error due to Python code issues identified by ruff.}

Hints

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.

ryanofsky

Code review ACK fac50ee, just rebased, added back functional test, and tweaked fuzz test since last review.

Overall this looks good and conceptually I like this change because it makes all the checking macros do the exact same thing in debug builds and abort, only having varying behavior in release builds.

src/test/fuzz/integer.cpp

This allows specific tests to mock the check behavior to consistently use exceptions instead of aborts for intentionally failing checks in all build configurations.

This requires adjusting some tests to force exceptions over aborts, or accept either exceptions or aborts. Also, remove a fuzz test in integer.cpp that is mostly redundant with the unit test added in the prior commit.

ryanofsky

Code review ACK fa37153, just catching subprocess.CalledProcessError in test fixing up a comment since last review

achow101 · 2025-07-29T19:41:59Z

ACK fa37153

DrahtBot changed the title ~~util: Abort on failing CHECK_NONFATAL in debug builds~~ util: Abort on failing CHECK_NONFATAL in debug builds May 22, 2025

DrahtBot added the Utils/log/libs label May 22, 2025

maflcko force-pushed the 2505-abort-debug-check-nonfatal branch 2 times, most recently from fadb1c8 to fa033fb Compare May 22, 2025 14:34

DrahtBot added the CI failed label May 22, 2025

DrahtBot removed the CI failed label May 22, 2025

maflcko force-pushed the 2505-abort-debug-check-nonfatal branch from fa033fb to faae9a2 Compare May 23, 2025 06:32

maflcko mentioned this pull request Jun 2, 2025

wallet: addhdkey RPC to add just keys to wallets via new unused(KEY) descriptor #29136

Open

ryanofsky approved these changes Jun 2, 2025

View reviewed changes

src/rpc/node.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from achow101 June 2, 2025 20:44

maflcko force-pushed the 2505-abort-debug-check-nonfatal branch from faae9a2 to fa68bda Compare July 22, 2025 14:22

DrahtBot mentioned this pull request Jul 23, 2025

improve MallocUsage() accuracy #28531

Draft

ryanofsky approved these changes Jul 23, 2025

View reviewed changes

src/test/fuzz/rpc.cpp Outdated Show resolved Hide resolved

src/rpc/node.cpp Outdated Show resolved Hide resolved

refactor: Set G_ABORT_ON_FAILED_ASSUME when G_FUZZING_BUILD

faeb58f

This does not change behavior, but documents that G_ABORT_ON_FAILED_ASSUME is set when G_FUZZING_BUILD.

maflcko force-pushed the 2505-abort-debug-check-nonfatal branch 2 times, most recently from fae1423 to faabcc0 Compare July 24, 2025 11:34

DrahtBot added the CI failed label Jul 24, 2025

maflcko force-pushed the 2505-abort-debug-check-nonfatal branch 2 times, most recently from fa54573 to fac50ee Compare July 24, 2025 12:17

ryanofsky approved these changes Jul 24, 2025

View reviewed changes

src/test/fuzz/integer.cpp Show resolved Hide resolved

test: Allow testing of check failures

fa0dc4b

This allows specific tests to mock the check behavior to consistently use exceptions instead of aborts for intentionally failing checks in all build configurations.

maflcko force-pushed the 2505-abort-debug-check-nonfatal branch 2 times, most recently from fa3f413 to fa27b23 Compare July 25, 2025 06:39

util: Abort on failing CHECK_NONFATAL in debug builds

fa37153

This requires adjusting some tests to force exceptions over aborts, or accept either exceptions or aborts. Also, remove a fuzz test in integer.cpp that is mostly redundant with the unit test added in the prior commit.

maflcko force-pushed the 2505-abort-debug-check-nonfatal branch from fa27b23 to fa37153 Compare July 25, 2025 06:44

DrahtBot removed the CI failed label Jul 25, 2025

ryanofsky approved these changes Jul 28, 2025

View reviewed changes

maflcko mentioned this pull request Aug 8, 2025

log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError #32604

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

util: Abort on failing CHECK_NONFATAL in debug builds #32588

util: Abort on failing CHECK_NONFATAL in debug builds #32588

maflcko commented May 22, 2025 •

edited

Loading

Uh oh!

DrahtBot commented May 22, 2025 •

edited

Loading

Uh oh!

DrahtBot commented May 22, 2025

Uh oh!

ryanofsky commented May 22, 2025

Uh oh!

maflcko commented May 23, 2025

Uh oh!

achow101 commented Jun 2, 2025

Uh oh!

ryanofsky left a comment

Uh oh!

Uh oh!

ryanofsky left a comment

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Jul 24, 2025

Uh oh!

ryanofsky left a comment

Uh oh!

Uh oh!

ryanofsky left a comment

Uh oh!

achow101 commented Jul 29, 2025

Uh oh!

Uh oh!

util: Abort on failing CHECK_NONFATAL in debug builds #32588

Are you sure you want to change the base?

util: Abort on failing CHECK_NONFATAL in debug builds #32588

Conversation

maflcko commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented May 22, 2025

Uh oh!

ryanofsky commented May 22, 2025

Uh oh!

maflcko commented May 23, 2025

Uh oh!

achow101 commented Jun 2, 2025

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Jul 24, 2025

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 29, 2025

Uh oh!

Uh oh!

maflcko commented May 22, 2025 •

edited

Loading

DrahtBot commented May 22, 2025 •

edited

Loading