Policy for VERIFY_CHECK and #ifdef VERIFY

[real-or-random]

Current situation

The purpose of VERIFY_CHECK(cond) is to assert cond in test builds, i.e., when VERIFY is defined. While cond is supposed to have no side effects, VERIFY_CHECK will 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 that cond has no side effects and to omit its evaluation entirely. However, many recently introduced cond are neither simple nor have negligible computation costs. In this case, we usually wrap VERIFY_CHECK(cond) in an #ifdef VERIFY block, which forces the evaluation to be omitted in production, at the cost of foregoing the aforementioned "side-effect safety" of VERIFY_CHECK. [1]

On top of this, we now have (full) functions like secp256k1_fe_verify, which internally perform VERIFY_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 VERIFY and 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_CHECK to be empty in production. That will keep things simple and save us the decision of whether to force omitting. The comment in #904 (comment) appears to agree with this.

When it comes to readability, I think that #ifdef blocks tend to clutter the code. If we redefine VERIFY_CHECK to be empty in production, we could omit many #ifdef lines. Even if we keep two types of VERIFY_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 call secp256k1_fe_verify in field_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 #ifdef lines.

So I suggest

• redefining VERIFY_CHECK to be empty in production
• renaming _check functions 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_CHECK to be empty in production, we have almost reimplemented assert.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 check has no side effects, but I agree with #904 (comment) 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.

[jonasnick]

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]

Agree, I think we should make VERIFY_CHECK compile to nothing in non-VERIFY mode. It was worthwhile to aim for side-effect freeness, but the way to codebase has evolved I believe makes the benefit not worth the cost. As pointed out in #904, the benefit is effectively restricted already to the simplest cases, where arguably it doesn't matter much anyway.

I wouldn't actually rename the verify_ functions to be upper case, but instead leave them as lowercase functions, but with an uppercase macro around them (maybe one that also compiles to nothing in non-VERIFY mode, so that not even a dummy verify_ function is needed in that case). This is just for style reasons - I think it's good that a reader can guess what is a function and what is a macro just on the name.

[real-or-random]

I wouldn't actually rename the verify_ functions to be upper case, but instead leave them as lowercase functions, but with an uppercase macro around them [...]

Sounds good to me.

@theStack Are you interested to look into this by any chance?

[theStack]

I wouldn't actually rename the verify_ functions to be upper case, but instead leave them as lowercase functions, but with an uppercase macro around them [...]

Sounds good to me.

@theStack Are you interested to look into this by any chance?

Yes, planning to tackle this within the next days.

[real-or-random]

This is done, except for the potential renaming to "assert", which is now tracked in #1449.