util: Add and use AssertUnreachable #36087

maflcko commented at 5:24 AM on August 26, 2026: member

The current code uses assertions with falsy literal values in a few places. This is perfectly fine, but this comes with a bit of confusion sometimes:

assert(false) may cause confusion when it is unclear if NDEBUG can disable it. (The intention is to disallow NDEBUG compilation)
Assert(false) hides the noreturn attribute behind a function call, which may cause GCC return-type warnings. (Clang seems to understand it) E.g. https://godbolt.org/z/e7bWsPn51
When C++23 will be allowed, devs may prefer std::unreachable. However, this is unsafe and will invoke UB.
There is confusion why a CHECK_NONFATAL macro exists, but no fatal equivalent.

Fix all those issues by:

Adding the util/check.h include for all assertions to import the NDEBUG compile error. (scripted-diff + clang-format)
Adding a new AssertUnreachable, which directly calls the noreturn assertion_fail helper.
Adding an assert_falsy linter to enforce all code to use this macro.
Adding a linter to forbid std::unreachable.

DrahtBot added the label Utils/log/libs on Aug 26, 2026

DrahtBot commented at 5:24 AM on August 26, 2026: contributor

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/36087.

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/954 (Add dialog to select change output when bumping fee by pablomartin4btc)
#bitcoin-core/gui/945 (qt: Fix sign message address book filtering by Bushstar)
#bitcoin-core/gui/762 (Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog by pablomartin4btc)
#36159 (http: Improve HTTPRemoteClient::MaybeDisconnect() by hodlinator)
#36135 (fuzz: test HTTPRequest state machine in http_request by frankomosh)
#36130 (test: add tests in transaction_tests.cpp covering live mutants by ViniciusCestarii)
#36122 (BIP460: CISA for Taproot key path spends by fjahr)
#36116 (iwyu: Fix warnings in src/rpc and treat them as errors by hebasto)
#36091 (test: Add debug output to common tested types by rustaceanrob)
#36070 (wallet: Add deriveHDKey interface by PraneethGunas)
#36068 (fuzz: reuse one fuzzed wallet across inputs by brunoerg)
#36015 (txorphanage: bound orphan memory by storing transactions serialized by brunoerg)
#35998 (wallet: Handle or explicitly ignore WalletBatch write failures by achow101)
#35916 (fuzz: improve ipc fuzz coverage by enirox001)
#35911 (Warn on and add missing [[noreturn]] by fanquake)
#35887 (ipc: use std::optional for checkSpawned(), add tests and rename arg -ipcfd to -ipcchild by ViniciusCestarii)
#35752 (wallet: make encryption state updates atomic by l0rinc)
#35731 (Indexes: Harden the flush-error notification invariant by arejula27)
#35714 (validation: stop writes after flush failure by l0rinc)
#35713 (Remove boost as a unit test runner by rustaceanrob)
#35646 (RFC: Separate out runtime errors from BlockValidationState using util::Expected by yuvicc)
#35511 (RFC: consensus: Make CAmount a class by hodlinator)
#35461 (util: Clarify the assertion message in assertion failures (Assert, Assume, etc.) by optout21)
#35429 (wallet: avoid global access in external signer SPKM by w0xlt)
#35377 (wallet: Allow importing of descriptors without private keys when the wallet has the private keys by achow101)
#35301 (Silent Payments: Implement bip352 (take 2) by Eunovo)
#35003 (validation: improve block data I/O error handling in P2P paths by furszy)
#34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
#34861 (wallet: Add importdescriptors interface by polespinasa)
#34778 (logging: rewrite macros to enforce restrictions at compile-time, improve efficiency and usability by ryanofsky)
#34681 (wallet: move rescan logic into ChainScanner and wallet/scan by Eunovo)
#34603 (wallet: Fix detection of symlinks on Windows by achow101)
#34566 (feature: Use different datadirs for different signets by ekzyis)
#34520 (refactor: Add [[nodiscard]] to functions returning bool+mutable ref by maflcko)
#34193 (wallet: make migration more robust against failures by furszy)
#34083 (Add initial vectorized chacha20 implementation for 2-3x speedup by theuni)
#32387 (ipc: add windows support by ryanofsky)
#31252 (rpc: print P2WSH and P2SH redem Script in getrawtransaction and getblock by polespinasa)
#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)
#29256 (log, refactor: Allow log macros to accept context arguments by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko force-pushed on Aug 26, 2026

maflcko marked this as a draft on Aug 26, 2026

DrahtBot added the label CI failed on Aug 26, 2026

DrahtBot commented at 5:45 AM on August 26, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/32933915544/job/98071384264</sub> <sub>LLM reason (✨ experimental): CI failed because the assert_falsy lint check detected an assert(0 && ...) (in src/tinyformat.h) that must be replaced with AssertUnreachable().</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>

DrahtBot added the label Needs rebase on Aug 26, 2026

maflcko force-pushed on Aug 26, 2026

DrahtBot removed the label Needs rebase on Aug 26, 2026

DrahtBot added the label Needs rebase on Aug 29, 2026

maflcko force-pushed on Aug 29, 2026

DrahtBot removed the label Needs rebase on Aug 29, 2026

jeanpablojp commented at 5:37 PM on August 29, 2026: contributor

Left 3 comments.

in src/script/interpreter.cpp:414 in e8d53b989d outdated

 410 | @@ -411,7 +411,7 @@ static bool EvalChecksig(const valtype& sig, const valtype& pubkey, CScript::con
 411 |          // Key path spending in Taproot has no script, so this is unreachable.
 412 |          break;
 413 |      }
 414 | -    assert(false);
 415 | +    AssertUnreachable();

jeanpablojp commented at 5:37 PM on August 29, 2026:

AssertUnreachable here and in SignatureHashSchnorr calls assertion_fail from util/check.cpp. This file is in libbitcoin_consensus, which should only depend on crypto. Normal builds hide it, bitcoin_common pulls util in anyway, but the riscv job links the lib standalone and breaks. Exclude consensus from assert_falsy under the same "not linking the util lib" reason, or move assertion_fail?

maflcko commented at 7:09 AM on September 1, 2026:

Thx, will fix this some time before taking out of draft.

in test/lint/test_runner/src/lint_cpp.rs:189 in e8d53b989d

 184 | +    let found = git()
 185 | +        .args([
 186 | +            "grep",
 187 | +            "--line-number",
 188 | +            "--extended-regexp",
 189 | +            r"\<(a|A)ssert\((0|false)( &&|\))",

jeanpablojp commented at 5:37 PM on August 29, 2026:

The message says any falsy literal, but the regex hardcodes the spacing, so assert (false), assert(false ) and assert(0&&"x") all slip through. \<(a|A)ssert\s*\(\s*(0|false)\s*(&&|\)) catches them. And std_unreachable lets __builtin_unreachable() through. libstdc++ implements std::unreachable() as exactly that, and it builds under C++20 where std::unreachable() doesn't. Worth catching the builtin too?

maflcko commented at 7:08 AM on September 1, 2026:

\<(a|A)ssert\s*\(\s*(0|false)\s*(&&|\))

Thx, used that.

__builtin_unreachable

I think the risk of someone using it should be low, because it is a compiler-specific built-in.

in test/lint/test_runner/src/main.rs:78 in e8d53b989d outdated

  72 | @@ -73,6 +73,16 @@ fn get_linter_list() -> Vec<&'static Linter> {
  73 |              name: "rpc_assert",
  74 |              lint_fn: lint_rpc_assert
  75 |          },
  76 | +        &Linter {
  77 | +            description: "Check that assertions with falsy literal values are not used",
  78 | +            name: "assert_falsy",

jeanpablojp commented at 5:37 PM on August 29, 2026:

The scripted-diff that satisfies this linter is the next commit, so lint is red here, 135 lines. Swapping the two applies cleanly and gives the same tree. The linters already in master came in with their fix in one commit.

maflcko commented at 7:09 AM on September 1, 2026:

Thx, swapped commits

DrahtBot added the label Needs rebase on Aug 31, 2026

scripted-diff: Add util/check.h includes for assertions

The project has a compile error when compiled without assertions in
util/check.h. Thus, util/check.h should be included for all assertions.

So do that with a scripted-diff for Assert and assert, and remove the
cassert include, which is exported from util/check.h.

-BEGIN VERIFY SCRIPT-

 paths=(
   'src' \
   ':(exclude)src/bench/nanobench.h' \
   ':(exclude)src/tinyformat.h' \
   ':(exclude)src/univalue' \
   ':(exclude)src/crc32c' \
   ':(exclude)src/crypto/ctaes' \
   ':(exclude)src/ipc/libmultiprocess' \
   ':(exclude)src/leveldb' \
   ':(exclude)src/minisketch' \
   ':(exclude)src/secp256k1' \
 )

 # Add the util/check.h includes
 for f in $(git grep -l --extended-regexp "\<(a|A)ssert\(" -- "${paths[@]}"); do
   if ! grep --quiet "util/check.h" "$f"; then
     line=$(grep --line-number --max-count=1 '^#include' "$f" | cut --delimiter=: --fields=1)
     sed --in-place "${line}i#include <util/check.h>" "$f"
   fi
 done

 # Remove cassert includes
 for f in $(git grep -l '<cassert>' -- "${paths[@]}"); do
   sed --in-place '/^#include <cassert>$/d' "$f"
 done

-END VERIFY SCRIPT-

6f48b96b53

refactor: Add missing #include external_signer.h to external_signer_scriptpubkeyman.h

This include is required and this change is also required for the next commit.

33e78c167a

scripted-diff: Sort includes

-BEGIN VERIFY SCRIPT-
git show -U0 HEAD~1 | ./contrib/devtools/clang-format-diff.py -p1 -i -v
-END VERIFY SCRIPT-

1c636d994a

util: Add AssertUnreachable

Also, in the tests convert a falsy literal to a "runtime" zero value.

2c658f176f

scripted-diff: Replace falsy assertions with AssertUnreachable

Replace assertions with falsy literal values by the explicit unreachable-code assertion macro.

-BEGIN VERIFY SCRIPT-

 paths=(
   'src'
   ':(exclude)src/tinyformat.h'
   ':(exclude)src/univalue/'
   ':(exclude)src/crc32c'
   ':(exclude)src/crypto/ctaes'
   ':(exclude)src/ipc/libmultiprocess'
   ':(exclude)src/leveldb'
   ':(exclude)src/minisketch'
   ':(exclude)src/secp256k1'
 )

 sed --in-place --regexp-extended 's/\<(a|A)ssert\((0|false)\)/AssertUnreachable()/g' $( \
   git grep -l --extended-regexp '\<(a|A)ssert\((0|false)\)' -- "${paths[@]}" \
 )

 sed --in-place --regexp-extended 's/assert\((0|false) && "([^"]*)"\);/AssertUnreachable(); \/\/ \2/g' $( \
   git grep -l --extended-regexp '\<assert\((0|false) &&' -- "${paths[@]}" \
 )

-END VERIFY SCRIPT-

281b80c766

lint: Add assert_falsy linter 2d8025c569

lint: Add std_unreachable linter d9e7bf5d98

maflcko force-pushed on Sep 1, 2026

DrahtBot removed the label Needs rebase on Sep 1, 2026

DrahtBot added the label Needs rebase on Sep 5, 2026

DrahtBot commented at 1:21 PM on September 5, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.