#36087 util: Add and use AssertUnreachable

ReviewabilityReviewsAgreementSize
#36087 maflcko util: Add and use AssertUnreachableAuthor silent 61d0Mild368 + 422 tests
  • Eliminate compiler return-type warnings caused by unreachable code assertions
  • Prevent assertion ambiguity and undefined behavior in unreachable code paths
  • Worth reviewing now because the rebase conflict is mechanical
  • No review verdicts recorded
  • Objection pending code update: standalone consensus library build breaks (jeanpablojp)
  • Resolved prior concerns regarding linter regex matching and commit ordering (jeanpablojp)
  • 368 lines added or modified outside tests
  • 422 lines added or modified in tests
  • 546 lines removed in total
  • 312 files, 7 commits
Test infrastructure#43 of 54
P4 · cleanup
  • P4 because new linters enforce assertion style without altering test coverage
  • Enforcement of assertion conventions provides minor maintenance value across the codebase
Ranking notes
Utilities (logging, arguments, libraries)#48 of 59
P3 · cleanup
  • P3 because it clears GCC return-type warnings and resolves ambiguous debug assertion behavior
  • It also protects against undefined behavior hazards in unreachable code paths
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Adds an `AssertUnreachable()` macro to `util/check.h` that calls the noreturn `assertion_fail` helper, replacing existing `assert(false)` and `Assert(false)` calls throughout the codebase. Adds linters in the test suite to forbid falsy assertions and disallow `std::unreachable`.

Problem: Assertions on falsy literals like `assert(false)` cause ambiguity about `NDEBUG` semantics, while `Assert(false)` hides `[[noreturn]]` behind a call, triggering GCC return-type warnings. Additionally, future adoption of C++23 `std::unreachable` would invite undefined behavior if reached.

Reviewability

Stale · Author silent 61d

No author activity for 61 days; a review may wait a long time for a response.

Agreement

Mild

objection that needs a change was answered, no reply from the objector (jeanpablojp: Standalone libbitcoin_consensus build breaks because AssertUnreachable introduces an unwanted dependency from interprete)

Thread notes: Recorded three claims raised by jeanpablojp. The consensus library linkage issue remains open as the author only promised to fix it before undrafting, while the linter regex improvements and commit reordering were resolved.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
jeanpablojp (contributor)objection, correctnessStandalone libbitcoin_consensus build breaks because AssertUnreachable introduces an unwanted dependency from interpreter.cpp into util/check.cppanswered · reply on record; standing not assessed yet · v1, not yet re-checkedchangedyes2026-08-29: '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.'
jeanpablojp (contributor)objection, correctnessThe assert_falsy linter regex misses falsy assertions with variable whitespace or 0&&"x"fixed · fix pushed · v1, not yet re-checkedansweredyes2026-08-29: 'The message says any falsy literal, but the regex hardcodes the spacing, so assert (false), assert(false ) and assert(0&&"x") all slip through.'
Status from: 2026-09-01: maflcko: 'Thx, used that.'
jeanpablojp (contributor)objection, maintenanceThe commit introducing the linter fails CI lint mid-history because the scripted-diff fix is placed in a subsequent commitfixed · fix pushed · v1, not yet re-checkedansweredyes2026-08-29: '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.'
Status from: 2026-09-01: maflcko: 'Thx, swapped commits'

Participants:

  • jeanpablojp (contributor, objection, 4 statements 2026-08-29 to 2026-08-29): pointed out consensus linkage breakage in standalone builds, linter regex gaps, and commit ordering breaking CI lint

Files

657 lines under test/bench/ci.

  • test/lint/test_runner/src/lint_cpp.rs +58/-2
  • src/qt/sendcoinsdialog.cpp +13/-13
  • src/qt/walletmodel.cpp +13/-13
  • src/qt/transactiontablemodel.cpp +10/-10
  • src/qt/guiutil.cpp +9/-10
  • src/qt/addresstablemodel.cpp +9/-9
  • src/wallet/test/wallet_tests.cpp +9/-9
  • src/netaddress.cpp +9/-8
  • src/qt/test/wallettests.cpp +9/-8
  • src/qt/bitcoinunits.cpp +8/-8
  • src/qt/test/addressbooktests.cpp +8/-8
  • src/cluster_linearize.h +8/-7
  • src/test/miniscript_tests.cpp +8/-7
  • src/test/transaction_tests.cpp +7/-8
  • src/kernel/bitcoinkernel.cpp +7/-7
  • src/qt/walletcontroller.cpp +7/-7
  • src/script/miniscript.h +7/-7
  • test/lint/test_runner/src/main.rs +12/-2
  • src/ipc/capnp/common-types.h +7/-6
  • src/ipc/test/ipc_tests.cpp +7/-6
  • src/qt/peertablemodel.cpp +6/-6
  • src/test/fuzz/feeratediagram.cpp +5/-7
  • src/qt/utilitydialog.cpp +5/-6
  • src/test/fuzz/versionbits.cpp +6/-5
  • src/blockfilter.cpp +6/-4
  • src/test/util_check_tests.cpp +6/-4
  • src/wallet/scriptpubkeyman.cpp +5/-5
  • src/ipc/test/fuzz/ipc.cpp +5/-4
  • src/qt/bantablemodel.cpp +5/-4
  • src/test/blockmanager_tests.cpp +5/-4
  • src/wallet/wallet.cpp +4/-5
  • src/policy/fees/block_policy_estimator.cpp +4/-4
  • src/qt/optionsmodel.cpp +4/-4
  • src/qt/receivecoinsdialog.cpp +4/-4
  • src/qt/walletframe.cpp +4/-4
  • src/test/fuzz/miniscript.cpp +5/-3
  • src/test/txrequest_tests.cpp +4/-4
  • src/test/util_tests.cpp +4/-4
  • src/test/validation_tests.cpp +4/-4
  • src/txgraph.cpp +4/-4
  • src/wallet/external_signer_scriptpubkeyman.cpp +5/-3
  • src/ipc/capnp/protocol.cpp +4/-3
  • src/node/connection_types.cpp +4/-3
  • src/test/crypto_tests.cpp +4/-3
  • src/test/fuzz/base_encode_decode.cpp +3/-4
  • src/test/fuzz/merkle.cpp +4/-3
  • src/test/logging_tests.cpp +4/-3
  • src/test/miniminer_tests.cpp +4/-3
  • src/test/util/net.cpp +4/-3
  • src/common/messages.cpp +3/-3
  • src/memusage.h +3/-3
  • src/net.h +3/-3
  • src/outputtype.cpp +3/-3
  • src/policy/feerate.cpp +4/-2
  • src/qt/bitcoin.h +3/-3
  • src/qt/walletview.cpp +3/-3
  • src/script/interpreter.cpp +3/-3
  • src/test/blockencodings_tests.cpp +3/-3
  • src/test/blockfilter_tests.cpp +3/-3
  • src/test/fuzz/feefrac.cpp +4/-2
  • src/test/fuzz/muhash.cpp +4/-2
  • src/test/fuzz/txgraph.cpp +4/-2
  • src/test/fuzz/txrequest.cpp +4/-2
  • src/test/key_io_tests.cpp +3/-3
  • src/test/private_broadcast_tests.cpp +4/-2
  • src/test/validation_chainstatemanager_tests.cpp +3/-3
  • src/crypto/chacha20.cpp +3/-2
  • src/kernel/mempool_removal_reason.cpp +3/-2
  • src/netbase.cpp +3/-2
  • src/psbt.h +3/-2
  • src/qt/rpcconsole.cpp +3/-2
  • src/qt/splashscreen.cpp +3/-2
  • src/qt/transactionrecord.cpp +3/-2
  • src/test/compress_tests.cpp +3/-2
  • src/test/fuzz/addrman.cpp +3/-2
  • src/test/fuzz/bech32.cpp +3/-2
  • src/test/fuzz/block.cpp +3/-2
  • src/test/fuzz/key.cpp +3/-2
  • src/test/fuzz/key_io.cpp +3/-2
  • src/test/fuzz/locale.cpp +3/-2
  • src/test/fuzz/script.cpp +3/-2
  • src/test/fuzz/string.cpp +3/-2
  • src/test/fuzz/threadpool.cpp +3/-2
  • src/test/fuzz/transaction.cpp +3/-2
  • src/test/fuzz/util/mempool.cpp +3/-2
  • src/test/headers_sync_chainwork_tests.cpp +3/-2
  • src/test/mempool_tests.cpp +3/-2
  • src/test/node_init_tests.cpp +3/-2
  • src/test/orphanage_tests.cpp +3/-2
  • src/test/pcp_tests.cpp +3/-2
  • src/test/rbf_tests.cpp +3/-2
  • src/test/sigopcount_tests.cpp +3/-2
  • src/test/txdownload_tests.cpp +3/-2
  • src/wallet/export.cpp +3/-2
  • src/wallet/feebumper.h +3/-2
  • src/wallet/test/ismine_tests.cpp +3/-2
  • doc/developer-notes.md +2/-2
  • src/addresstype.cpp +2/-2
  • src/bench/verify_script.cpp +2/-2
  • src/chainparams.cpp +2/-2
  • src/chainparamsbase.cpp +2/-2
  • src/node/minisketchwrapper.cpp +2/-2
  • src/node/psbt.cpp +3/-1
  • src/node/transaction.cpp +3/-1
  • src/qt/askpassphrasedialog.cpp +2/-2
  • src/qt/bitcoinamountfield.cpp +2/-2
  • src/script/miniscript.cpp +2/-2
  • src/script/solver.cpp +2/-2
  • src/test/fuzz/asmap.cpp +3/-1
  • src/test/fuzz/asmap_direct.cpp +2/-2
  • src/test/fuzz/bitdeque.cpp +3/-1
  • src/test/fuzz/bitset.cpp +3/-1
  • src/test/fuzz/cluster_linearize.cpp +3/-1
  • src/test/fuzz/fees.cpp +3/-1
  • src/test/fuzz/headerssync.cpp +3/-1
  • src/test/fuzz/net.cpp +3/-1
  • src/test/fuzz/private_broadcast.cpp +3/-1
  • src/test/fuzz/tx_pool.cpp +2/-2
  • src/test/fuzz/txdownloadman.cpp +3/-1
  • src/test/fuzz/util/net.cpp +2/-2
  • src/test/fuzz/vecdeque.cpp +3/-1
  • src/test/util/chainstate.h +2/-2
  • src/test/util/net.h +2/-2
  • src/test/util/transaction_utils.cpp +3/-1
  • src/util/fees.cpp +2/-2
  • src/wallet/db.cpp +3/-1
  • src/wallet/test/coinselector_tests.cpp +3/-1
  • src/wallet/test/fuzz/coincontrol.cpp +3/-1
  • src/wallet/test/fuzz/coinselection.cpp +3/-1
  • src/wallet/test/fuzz/spend.cpp +3/-1
  • src/wallet/test/util.cpp +2/-2
  • src/wallet/wallet.h +2/-2
  • src/base58.cpp +1/-2
  • src/bech32.cpp +2/-1
  • src/bitcoin-cli.cpp +2/-1
  • src/bitcoin-util.cpp +2/-1
  • src/crypto/hex_base.cpp +2/-1
  • src/crypto/sha256.cpp +2/-1
  • src/crypto/sha3.cpp +2/-1
  • src/netaddress.h +2/-1
  • src/node/txdownloadman_impl.cpp +2/-1
  • src/policy/fees/estimator_man.cpp +2/-1
  • src/prevector.h +2/-1
  • src/qt/bitcoin.cpp +2/-1
  • src/qt/test/apptests.cpp +2/-1
  • src/test/fuzz/banman.cpp +2/-1
  • src/test/fuzz/difference_formatter.cpp +2/-1
  • src/test/fuzz/flatfile.cpp +2/-1
  • src/test/fuzz/net_permissions.cpp +2/-1
  • src/test/fuzz/netaddress.cpp +2/-1
  • src/test/fuzz/util/wallet.h +2/-1
  • src/test/httpserver_tests.cpp +2/-1
  • src/test/interfaces_tests.cpp +2/-1
  • src/test/scriptnum10.h +2/-1
  • src/test/txvalidation_tests.cpp +2/-1
  • src/util/check.h +3/-0
  • src/wallet/external_signer_scriptpubkeyman.h +2/-1
  • src/wallet/test/wallet_test_fixture.cpp +2/-1
  • src/wallet/walletutil.cpp +2/-1
  • src/zmq/zmqabstractnotifier.cpp +2/-1
  • src/addrman.cpp +1/-1
  • src/arith_uint256.cpp +1/-1
  • src/bench/sign_transaction.cpp +1/-1
  • src/bip324.cpp +1/-1
  • src/blockencodings.cpp +2/-0
  • src/chain.h +1/-1
  • src/common/signmessage.cpp +1/-1
  • src/crypto/chacha20poly1305.cpp +1/-1
  • src/crypto/hkdf_sha256_32.cpp +1/-1
  • src/crypto/poly1305.h +1/-1
  • src/crypto/siphash.cpp +1/-1
  • src/dbwrapper.cpp +1/-1
  • src/deploymentinfo.h +1/-1
  • src/httpserver.cpp +1/-1
  • src/index/blockfilterindex.cpp +1/-1
  • src/index/txindex.cpp +1/-1
  • src/init.cpp +1/-1
  • src/kernel/chainparams.cpp +1/-1
  • src/kernel/coinstats.cpp +1/-1
  • src/kernel/disconnected_transactions.cpp +1/-1
  • src/key.cpp +1/-1
  • src/key_io.cpp +1/-1
  • src/logging.cpp +1/-1
  • src/mapport.cpp +1/-1
  • src/net_processing.cpp +1/-1
  • src/node/chainstate.cpp +1/-1
  • src/node/eviction.cpp +2/-0
  • src/node/txorphanage.cpp +1/-1
  • src/node/utxo_snapshot.cpp +1/-1
  • src/primitives/transaction.cpp +1/-1
  • src/psbt.cpp +1/-1
  • src/pubkey.cpp +1/-1
  • src/qt/peertablesortproxy.cpp +1/-1
  • src/rest.cpp +1/-1
  • src/rpc/server.cpp +1/-1
  • src/scheduler.cpp +1/-1
  • src/script/descriptor.cpp +1/-1
  • src/script/script.h +1/-1
  • src/script/sign.cpp +1/-1
  • src/sync.h +1/-1
  • src/test/fuzz/bip324.cpp +2/-0
  • src/test/fuzz/block_header.cpp +1/-1
  • src/test/fuzz/bloom_filter.cpp +1/-1
  • src/test/fuzz/coins_view.cpp +1/-1
  • src/test/fuzz/coinscache_sim.cpp +1/-1
  • src/test/fuzz/crypto_aes256.cpp +1/-1
  • src/test/fuzz/crypto_aes256cbc.cpp +1/-1
  • src/test/fuzz/crypto_common.cpp +1/-1
  • src/test/fuzz/cuckoocache.cpp +2/-0
  • src/test/fuzz/decode_tx.cpp +1/-1
  • src/test/fuzz/float.cpp +1/-1
  • src/test/fuzz/golomb_rice.cpp +1/-1
  • src/test/fuzz/hex.cpp +1/-1
  • src/test/fuzz/http_request.cpp +1/-1
  • src/test/fuzz/message.cpp +1/-1
  • src/test/fuzz/node_eviction.cpp +1/-1
  • src/test/fuzz/p2p_transport_serialization.cpp +1/-1
  • src/test/fuzz/parse_hd_keypath.cpp +1/-1
  • src/test/fuzz/parse_iso8601.cpp +1/-1
  • src/test/fuzz/parse_numbers.cpp +1/-1
  • src/test/fuzz/prevector.cpp +2/-0
  • src/test/fuzz/rolling_bloom_filter.cpp +1/-1
  • src/test/fuzz/rpc.cpp +1/-1
  • src/test/fuzz/script_flags.cpp +1/-1
  • src/test/fuzz/script_sign.cpp +1/-1
  • src/test/fuzz/scriptnum_ops.cpp +1/-1
  • src/test/random_tests.cpp +1/-1
  • src/test/sock_tests.cpp +1/-1
  • src/txdb.cpp +1/-1
  • src/txrequest.cpp +1/-1
  • src/uint256.h +1/-1
  • src/util/asmap.cpp +1/-1
  • src/util/chaintype.cpp +1/-1
  • src/wallet/coinselection.cpp +1/-1
  • src/wallet/spend.cpp +1/-1
  • src/wallet/test/group_outputs_tests.cpp +1/-1
  • src/bitcoin-chainstate.cpp +0/-1
  • src/coins.cpp +1/-0
  • src/coins.h +0/-1
  • src/compressor.cpp +1/-0
  • src/deploymentinfo.cpp +1/-0
  • src/deploymentstatus.h +1/-0
  • src/hash.h +1/-0
  • src/merkleblock.cpp +1/-0
  • src/net.cpp +1/-0
  • src/net_permissions.h +1/-0
  • src/netgroup.cpp +1/-0
  • src/netgroup.h +1/-0
  • src/node/coin.cpp +1/-0
  • src/policy/packages.cpp +0/-1
  • src/policy/policy.cpp +1/-0
  • src/protocol.cpp +1/-0
  • src/protocol.h +1/-0
  • src/qt/bitcoingui.cpp +1/-0
  • src/qt/guiutil.h +0/-1
  • src/qt/optionsmodel.h +0/-1
  • src/random.cpp +1/-0
  • src/random.h +0/-1
  • src/script/script.cpp +1/-0
  • src/span.h +0/-1
  • src/streams.h +0/-1
  • src/support/allocators/pool.h +0/-1
  • src/test/blockchain_tests.cpp +1/-0
  • src/test/btcsignals_tests.cpp +1/-0
  • src/test/chainstate_write_tests.cpp +1/-0
  • src/test/checkqueue_tests.cpp +1/-0
  • src/test/cuckoocache_tests.cpp +1/-0
  • src/test/flatfile_tests.cpp +1/-0
  • src/test/fuzz/addition_overflow.cpp +1/-0
  • src/test/fuzz/block_index.cpp +1/-0
  • src/test/fuzz/block_index_tree.cpp +1/-0
  • src/test/fuzz/connect_block.cpp +1/-0
  • src/test/fuzz/connman.cpp +1/-0
  • src/test/fuzz/crypto_chacha20.cpp +1/-0
  • src/test/fuzz/crypto_chacha20poly1305.cpp +1/-0
  • src/test/fuzz/crypto_diff_fuzz_chacha20.cpp +1/-0
  • src/test/fuzz/crypto_poly1305.cpp +1/-0
  • src/test/fuzz/dbwrapper.cpp +0/-1
  • src/test/fuzz/descriptor_parse.cpp +1/-0
  • src/test/fuzz/deserialize.cpp +1/-0
  • src/test/fuzz/fee_rate.cpp +1/-0
  • src/test/fuzz/integer.cpp +0/-1
  • src/test/fuzz/kitchen_sink.cpp +1/-0
  • src/test/fuzz/multiplication_overflow.cpp +1/-0
  • src/test/fuzz/netbase_dns_lookup.cpp +1/-0
  • src/test/fuzz/p2p_headers_presync.cpp +1/-0
  • src/test/fuzz/p2p_private_broadcast.cpp +1/-0
  • src/test/fuzz/poolresource.cpp +1/-0
  • src/test/fuzz/primitives_transaction.cpp +1/-0
  • src/test/fuzz/script_descriptor_cache.cpp +1/-0
  • src/test/fuzz/secp256k1_ec_seckey_import_export_der.cpp +1/-0
  • src/test/fuzz/span.cpp +0/-1
  • src/test/fuzz/tx_in.cpp +0/-1
  • src/test/fuzz/tx_out.cpp +1/-0
  • src/test/fuzz/util.h +1/-0
  • src/test/mempool_fee_estimator_tests.cpp +1/-0
  • src/test/net_tests.cpp +1/-0
  • src/test/scheduler_tests.cpp +1/-0
  • src/test/streams_tests.cpp +1/-0
  • src/test/threadpool_tests.cpp +1/-0
  • src/test/util/cluster_linearize.h +1/-0
  • src/test/util/poolresourcetester.h +0/-1
  • src/torcontrol.cpp +0/-1
  • src/txmempool.h +1/-0
  • src/util/obfuscation.h +1/-0
  • src/util/result.h +1/-0
  • src/validation.cpp +0/-1
  • src/wallet/coincontrol.cpp +1/-0
  • src/wallet/test/fuzz/wallet_bdb_parser.cpp +1/-0
  • src/wallet/test/psbt_wallet_tests.cpp +1/-0
  • src/wallet/test/spend_tests.cpp +1/-0
  • src/wallet/transaction.cpp +1/-0

Card

The short text the ranking pass compares across PRs.

This PR introduces `AssertUnreachable` in `util/check.h` and replaces falsy assertion patterns like `assert(false)` across hundreds of files in the project, accompanied by two new linters. It aims to eliminate GCC return-type warnings caused by hidden noreturn attributes and prevent future use of `std::unreachable` which introduces undefined behavior. Review is currently stale as the PR has merge conflicts, failing CI, and an unresolved linkage issue where consensus code inadvertently pulled in `util/check`. It has no dependencies and is waiting for author updates.

Data

dossier JSON · extract JSON · model ledger, generated 2026-09-18T03:44, confidence high, input hash 093073f29763ee59
thread read through 2026-09-01 (7 statements), head d9e7bf5d98
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: Utils/log/libs, Needs rebase, CI failed · draft

Categories

Test infrastructure

P4 · cleanup

Adds linters in the test suite to forbid falsy assertions and disallow std::unreachable. Enforces coding style and assertion patterns across the repository but does not affect test coverage or framework reliability.

Membership: Adds custom linter logic in test/lint/test_runner/src/lint_cpp.rs.

Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1

Utilities (logging, arguments, libraries)

P3 · cleanup

Adds an AssertUnreachable macro to util/check.h that calls the noreturn assertion_fail helper, replacing existing assert(false) and Assert(false) calls throughout the codebase. This resolves GCC return-type warnings caused by hiding [[noreturn]] behind Assert(false), avoids NDEBUG ambiguities, and prevents potential undefined behavior hazards from future std::unreachable adoption.

Membership: Carries the Utils/log/libs label and adds AssertUnreachable to util/check.h.

Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1