jonasnick
commented
1 year ago Adding to the current situation is that VERIFY_CHECK is not side-effect safe when built with --enable-coverage which lead to bugs in coverage mode in the past. In fact, there are again unused variable warnings right now when compiling master with --enable-coverage. So an additional advantage of your suggestion is that VERIFY_CHECK in coverage mode isn't special anymore which would hopefully reduce maintenance of that mode.
sipa
real-or-random
theStack
Current situation
The purpose of
VERIFY_CHECK(cond)is to assertcondin test builds, i.e., whenVERIFYis defined. Whilecondis supposed to have no side effects,VERIFY_CHECKwill evaluate it even in production builds, to make sure that no side effects are masked in production builds.In the case of simple
cond, we can rely on the ability of the compiler to detect thatcondhas no side effects and to omit its evaluation entirely. However, many recently introducedcondare neither simple nor have negligible computation costs. In this case, we usually wrapVERIFY_CHECK(cond)in an#ifdef VERIFYblock, which forces the evaluation to be omitted in production, at the cost of foregoing the aforementioned "side-effect safety" ofVERIFY_CHECK. [1]On top of this, we now have (full) functions like
secp256k1_fe_verify, which internally performVERIFY_CHECKs.I don't think anything we currently do is wrong, but sometimes the borders are a bit arbitrary, and with a mix of
#ifdef VERIFYand no#ifdef VERIFY, and a mix of uppercase and lowercase, also readability and consistency suffer.Options
When it comes to forcing evaluations to be omitted, we can either keep the current convention and force omitting on a case-by-case basis. Alternatively, we could just always force omitting, e.g., by defining
VERIFY_CHECKto be empty in production. That will keep things simple and save us the decision of whether to force omitting. The comment in https://github.com/bitcoin-core/secp256k1/pull/904#issuecomment-1540601651 appears to agree with this.When it comes to readability, I think that
#ifdefblocks tend to clutter the code. If we redefineVERIFY_CHECKto be empty in production, we could omit many#ifdeflines. Even if we keep two types ofVERIFY_CHECKs, I suggest introducing a separate macro instead to replace#ifdef.One remaining purpose of the
#ifdefs is to highlight the test code blocks, e.g., when we callsecp256k1_fe_verifyinfield_impl.h. But then we could just make these function names uppercase. And if we want blocks/grouping for better readability, we can simply use empty lines instead of#ifdeflines.So I suggest
VERIFY_CHECKto be empty in production_checkfunctions to uppercase.And while we're touching this anyway, we could consider renaming "check" to "assert", which is a more precise term. (In fact, if we redefine
VERIFY_CHECKto be empty in production, we have almost reimplementedassert.h...)Does this make sense?
[1] One idea was to improve this situation was to introduce a C hack that will tell us whether a compiler is able to determine that a
checkhas no side effects, but I agree with https://github.com/bitcoin-core/secp256k1/pull/904#issuecomment-1540601651 that this is not the way to go. In addition to the points brought up by @sipa, relying on the compiler is just very brittle, with optimizations depending on compiler versions and flags.