Add VERIFY_CHECKs and documentation that flags must be 0 or 1

[john-moffett]

Flags for constant-time masking rely on the values being exactly 0 or 1 rather than 0 or true (any nonzero). One function, secp256k1_fe_cmov documents and VERIFY_CHECKs this, but most don't.

This updates the documentation and adds VERIFY_CHECKs enforcing flag == 0 || flag == 1 for:

secp256k1_fe_storage_cmov
secp256k1_gej_cmov
secp256k1_ge_storage_cmov
secp256k1_scalar_cadd_bit
secp256k1_scalar_cond_negate
secp256k1_scalar_cmov
secp256k1_int_cmov

[furszy]

Wondering if it worth adding a macro for this; something like VERIFY_BOOL_ARG.

[real-or-random]

utACK 0a6ebe0

If you want to touch this again: I like "flag must be 0 or 1." because it's so clear. It would be good to add it even to the comments in "If flag is 1..." style.

[real-or-random]

Wondering if it worth adding a macro for this; something like VERIFY_BOOL_ARG.

Weak Concept NACK for the reasons laid out in #1779. There, it was a Concept ~0, but here I deem the value below 0 because we don't even save lines. VERIFY_BOOL_ARG(flag) is not simpler to read than VERIFY_CHECK(flag == 0 || flag == 1). If anything, it's harder to read because it's less explicit (unless you look up the macro).

If we really want to improve this, then let's switch to C99 and use bool. But I'm not sure if I really want to have this discussion. :D

edit: Sorry, Concept NACK. I had typed "Concept ACK" first.

[real-or-random]

utACK 9fa11a7

[furszy]

Wondering if it worth adding a macro for this; something like VERIFY_BOOL_ARG.

Weak Concept NACK for the reasons laid out in #1779. There, it was a Concept ~0, but here I deem the value below 0 because we don't even save lines. VERIFY_BOOL_ARG(flag) is not simpler to read than VERIFY_CHECK(flag == 0 || flag == 1). If anything, it's harder to read because it's less explicit (unless you look up the macro).

That's probably because VERIFY_BOOL_ARG is not explicit/readable enough?
My broader thought is that argument checks should come from well-named, established helpers at this point, so we don’t have to keep worrying about the raw checks in every function. We should understand what the function does just by reading its name.

Furthermore, if you push me a bit, I’d even say we should aim for a more generic mechanism for argument validation altogether (some sort of decorator), so validating args becomes a no-brainer entirely.

If we really want to improve this, then let's switch to C99 and use bool. But I'm not sure if I really want to have this discussion. :D

hehe, not planning to go that far. I just think we should focus on what actually matters and minimize the time we spend reviewing malformed input-arg checks.

[real-or-random]

That's probably because VERIFY_BOOL_ARG is not explicit/readable enough?

It's just that spelling out the condition, as in VERIFY_CHECK(flag == 0 || flag == 1) is, by the meaning of the word, more explicit, and it's still short enough to be read with a single glimpse. This gives VERIFY_BOOL_ARG a tiny disadvantage because readers may need to look it up. But this is not about the name of the macro. I like the name. The name is rather explicit and probably as good as it can get.

My broader thought is that argument checks should come from well-named, established helpers at this point, so we don’t have to keep worrying about the raw checks in every function. We should understand what the function does just by reading its name.

Furthermore, if you push me a bit, I’d even say we should aim for a more generic mechanism for argument validation altogether (some sort of decorator), so validating args becomes a no-brainer entirely.

This is a different direction, of course. If we had macros for almost every type (or even some more clever thing, but that seems difficult in C89), then the tiny disadvantage of VERIFY_BOOL_ARG would most likely be outweighed by the advantage of having a consistent framework. Curious to see proposals if you have something in mind!

Still, the raw checks are mostly easy to read, and they keep the advantage of being very explicit. So I think in the current state of the code base, they make sense.