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:

#35569 (Encapsulation for CTransaction by purpleKarrot)
#34864 (coins: tighten cache entry state invariants by l0rinc)
#34803 (mempool: asynchronous mempool fee rate diagram updates via validation interface by ismaelsadeeq)

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.

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