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
VERIFY_BOOL_ARG.
utACK 0a6ebe0b24c247aebfa63ccb0017d4de93e2e5a8
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.
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.
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 thanVERIFY_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.
That’s probably because
VERIFY_BOOL_ARGis 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.
ACK 9fa11a76eacbe4774ade813f9c25c2c348cb9086
It seems you could also do it for secp256k1_musig_secnonce_invalidate (at least the comment update).
ACK 9fa11a76eacbe4774ade813f9c25c2c348cb9086, I have reviewed the code and it looks OK.
We should understand what the function does just by reading its name.
Perhaps this would be a good time to rename secp256k1_scalar_cond_negate to reflect its constant-time behaviour?
Flags for constant-time masking rely
on the values being exactly 0 or 1 rather
than 0 or true. Add VERIFY_CHECKs to enforce
in VERIFY builds as a preventative
measure and add documentation where relevant.
secp256k1_scalar_add, secp256k1_scalar_mul, secp256k1_scalar_cond_negate. It seems like only variable-time ones are marked as such with a _var suffix. Let me know if you had something else in mind, though.