util: move function calls outside standard assertions #36030

l0rinc commented at 10:04 PM on August 19, 2026: contributor

Problem: KeyPair::KeyPair() performed two required libsecp256k1 calls inside assert(). Disabling assertions skips those calls, so the Taproot tweak is computed from uninitialized data. Supported builds reject NDEBUG, so this affects only unsupported configurations, but it motivated a broader audit of calls inside standard assertions.

Fix: Evaluate the required key operations explicitly. Audit the remaining call sites and use the always-evaluated Assert() helper only when a call initializes output used afterward, mutates state, performs I/O, or releases resources. Leave pure predicates and calculations as standard assertions.

Reproducer: Compiler Explorer demonstrates assert(keypair_xonly_pub(&pubkey)) skipping the required key derivation under -DNDEBUG.

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

DrahtBot commented at 10:04 PM on August 19, 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/36030.

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:

#36042 (build: Bump g++ minimum supported version to 13 by maflcko)
#35975 (wallet: Fix CWalletTx malleated transaction metadata sync by achow101)
#35916 (fuzz: improve ipc fuzz coverage by enirox001)
#35570 (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)
#35170 (test: Better test coverage for legacy ParseHDKeypath() by optout21)
#25722 (refactor: Use util::Result class for wallet loading 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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

CheckBlock(block, cvstate, chainparams.GetConsensus(), false, false) in src/bench/duplicate_inputs.cpp

key: evaluate secp calls outside assertions

`KeyPair` derives and serializes its x-only public key before computing a Taproot tweak.
Both libsecp calls ran inside `assert()`, so disabling assertions left `pubkey_bytes` uninitialized.
Reading those bytes when computing the tweak is undefined behavior.

Supported builds reject `NDEBUG`, so released binaries are unaffected.
Both calls now run before the tweak computation, allowing the existing cleanup path to handle either failure.

67eb32f422

l0rinc force-pushed on Aug 19, 2026

DrahtBot added the label CI failed on Aug 19, 2026

l0rinc renamed this:
~~util: keep function calls outside standard assertions~~
util: move function calls outside standard assertions
on Aug 19, 2026

l0rinc marked this as a draft on Aug 20, 2026

util: evaluate side effects outside assertions

`assert()` does not evaluate its expression when `NDEBUG` is defined. Keep only calls that write output used afterward, mutate state, perform I/O, or release resources; these operations are part of setup or the target under test rather than the invariant alone.

Use `Assert()` so their evaluation does not depend on the standard assertion macro.

246ca6cf4d

in src/.clang-tidy:44 in 76ca7546a5

  39 |  WarningsAsErrors: '*'
  40 |  CheckOptions:
  41 | + - key: bugprone-assert-side-effect.CheckFunctionCalls
  42 | +   value: true
  43 | + - key: bugprone-assert-side-effect.IgnoredFunctions
  44 | +   value: '^MoneyRange$;^base_blob<.*>::size$;^std::basic_string<.*>::data$;^std::map<.*>::end$;^std::numeric_limits<.*>::(min|max)$;^std::this_thread::get_id$;^std::unordered_map<.*>::end$;^std::unordered_set<.*>::end$;^std::vector<.*>::(back|data)$'

maflcko commented at 4:48 AM on August 20, 2026:

Not sure about excluding stuff.

Either assert is overall fine to use, and this pull can just be closed.

Or, it is not, and then it should just be disallowed completely. Maybe with a simple git grep assert linter (like lint_rpc_assert)?

Anything in-between just seems inconsistent and pointless.

l0rinc commented at 5:16 AM on August 20, 2026:

Either assert is overall fine to use [...] Or, it is not

I'd argue it's more complicated because of possible side effects of the calls. Pure calls should be safe to eliminate, so it doesn't matter if it's assert or Assert. We can migrate the remaining assert calls all in a follow-up to get rid of this confusion.

But here I rather wanted to focus on the non-const ones (but it seems that's still too broad or not broad enough), so I've refocused on side-effectful calls here that we can never eliminate: remove the assert in KeyPair::KeyPair, and migrate assert to Assert for calls like ConnectBlock.

maflcko commented at 6:57 AM on August 20, 2026:

Pure calls should be safe to eliminate, so it doesn't matter if it's assert or Assert.

I don't think this is true. What about "pure" calls or calls without side-effects to assert that are there to prevent UB in the next line?

l0rinc commented at 4:44 PM on August 20, 2026:

I agree, I don't think that contradict what I said. I'm fixing the worst offenders here, I'm all for fixing the rest in a follow-up PR (though we likely have to extract the assertion helpers to avoid circular dependencies)

maflcko commented at 5:00 PM on August 20, 2026:

I think the build dep issue is already fixed in #28690, no?

l0rinc commented at 6:04 PM on August 20, 2026:

I haven't dug deeply into #28690 yet, but I see that it moves util/check.cpp into bitcoin_util_kernel, so kernel code should be fine to migrate to Assert() afterward.

A complete migration still seems to require either exempting the lowest-level libraries or splitting the assertion failure helper further. Regardless, this PR isn't focused on that migration, it only makes sure required function calls are always evaluated.

maflcko commented at 6:46 AM on August 24, 2026:

They already are evaluated, and compilation fails if they are not. This is not a fix, but a style cleanup.

fixing the worst offenders

None of them are real issues, so it seems odd to ask reviewers to classify them into severity. I remain, that this style cleanup should either fix all or none (https://github.com/bitcoin/bitcoin/pull/36030#discussion_r3818698180)

l0rinc commented at 6:21 PM on August 24, 2026:

assert has many pitfalls (some are documented in https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2829.htm#remedy), I assume it's why we introduced Assert. But currently we can't just switch to Assert everywhere because of the mentioned circular dependencies - but looking forward to when we will be able to do that. As mentioned above and in the PR description, this isn't a style change, it's the smallest change that I found to make sure the assertions cannot remove critical code (e.g. if NDEBUG is enabled or the code forked and changed, etc).

maflcko commented at 9:30 AM on August 25, 2026:

The paper you link to is about a small style cleanup of the assert macro to not require extra () around the condition in some case. I don't think there is any pitfall in that paper relevant to the code here.

(e.g. if NDEBUG is enabled

This won't compile

or the code forked and changed, etc).

If someone removes check, it is on them, and they could equally remove any other check and there is nothing that can be done about that.

Also, if such a use-case was supported and the user cared about checks being retained, then UB checks should equally be preserved, because UB can also lead to critical code being removed (by the compiler or at runtime).

Also, since you are modifying benchmarks: If the assert was compiled out, the benchmark would be wrong, etc.

I am not against this change, and it is probably fine to do (even if the only goal is to avoid confusion), but either all places are fixed (and enforced), or none of them. (I've done it for all test code in #36074)

Cherry-picking single cases one-by-one and asking for review for every single one in a separate pull doesn't seem scalable.

l0rinc force-pushed on Aug 20, 2026

l0rinc marked this as ready for review on Aug 20, 2026

DrahtBot removed the label CI failed on Aug 20, 2026

l0rinc closed this on Aug 26, 2026