Policy for VERIFY_CHECK and #ifdef VERIFY about secp256k1 HOT 5 CLOSED

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.

from secp256k1.

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.

from secp256k1.

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?

from secp256k1.

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.

from secp256k1.

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

from secp256k1.