Skip to content

scripted-diff: [test] Add util/check.h includes for assertions - #36074

Draft
maflcko wants to merge 3 commits into
bitcoin:masterfrom
maflcko:2608-test-assert
Draft

scripted-diff: [test] Add util/check.h includes for assertions#36074
maflcko wants to merge 3 commits into
bitcoin:masterfrom
maflcko:2608-test-assert

Conversation

@maflcko

@maflcko maflcko commented Aug 25, 2026

Copy link
Copy Markdown
Member

In test code, assert is used. This is perfectly fine, but sometimes confusion arises, when it is unclear whether NDEBUG can disable the assertions, or whether to use assert or Assert.

Avoid that confusion in test code with a scripted replacement to add the util/check.h include. The changes here will also make it easier to run IWYU.

Scope: This change is only about test code (bench, fuzz, unit), other code can be done later, if there is need to.

@DrahtBot DrahtBot changed the title scripted-diff: [test] replace assert with Assert scripted-diff: [test] replace assert with Assert Aug 25, 2026
@DrahtBot

DrahtBot commented Aug 25, 2026

Copy link
Copy Markdown
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/36074.

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)
  • #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)
  • #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)
  • #35998 (wallet: Handle or explicitly ignore WalletBatch write failures by achow101)
  • #35916 (fuzz: improve ipc fuzz coverage by enirox001)
  • #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)
  • #35377 (wallet: Allow importing of descriptors without private keys when the wallet has the private keys by achow101)
  • #35003 (validation: improve block data I/O error handling in P2P paths by furszy)
  • #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)
  • #32387 (ipc: add windows support by ryanofsky)
  • #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.

@DrahtBot

Copy link
Copy Markdown
Contributor

🚧 At least one of the CI tasks failed.
Task i686, no IPC: https://github.com/bitcoin/bitcoin/actions/runs/32832295932/job/97753466551
LLM reason (✨ experimental): CI failed due to a C++ build error treated as fatal (-Werror=return-type) in bench/sign_transaction.cpp where an Assert(false) lambda triggers “control reaches end of non-void function.”

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.

@maflcko
maflcko marked this pull request as draft August 25, 2026 12:08
@maflcko

maflcko commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

Hmm, I guess Assert(0/false) being a function trips GCC into thinking it can return? Though, the optimized codegen is unaffected on GCC. Funnily clang doesn't warn, but pessimises codegen: https://godbolt.org/z/h79K1scWK

I guess that means we could add an explicit #define AssertUnreachable() assertion_fail(std::source_location::current(), "Unreachable code")

Comment thread src/test/fuzz/mini_miner.cpp
Comment thread test/lint/test_runner/src/lint_cpp.rs Outdated
MarcoFalke added 3 commits September 1, 2026 08:52
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-

 # Select all test .cpp and .h files
 paths=(
   'src/bench/'
   'src/ipc/test/'
   'src/qt/test/'
   'src/test/'
   'src/wallet/test/'
   ':(exclude)src/bench/nanobench.h'
 )

 # 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-
Also, remove the unused pushd/popd. No command in this file requires a
special PWD.
-BEGIN VERIFY SCRIPT-
git show -U0 HEAD~1 | ./contrib/devtools/clang-format-diff.py -p1 -i -v
-END VERIFY SCRIPT-
@DrahtBot

DrahtBot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants