Apparently clang 15 is able to compile our cmov code into a branch,
at least for fe_cmov and fe_storage_cmov. This commit makes the
condition volatile in all cmov implementations (except ge but that
one only calls into the fe impls).
This is just a quick fix. We should still look into other methods,
e.g., asm and #457. We should also consider not caring about
constant-time in scalar_low_impl.h
We should also consider testing on very new compilers in nightly CI,
see https://github.com/bitcoin-core/secp256k1/pull/864#issuecomment-769211867
The implementation calls the secp256k1_modinvNN_jacobi_var code, falling back
to computing a square root in the (extremely rare) case it failed converge.
1287786c7a97eff520ffbd6b0d8b2f99dbfc6371 doc: Add comment to top of field_10x26_impl.h (Elliott Jin)
58da5bd589f61b0e0e9b58388ee3e0da8a2c3c3a doc: Fix upper bounds + cleanup in field_5x52_impl.h comment (Elliott Jin)
Pull request description:
When reviewing #816 I noticed the upper bounds in the comment at the top of `field_5x52_impl.h` were off by 1 (see `fe_verify`). This PR fixes the upper bounds and also cleans up the comment along the way.
ACKs for top commit:
real-or-random:
ACK 1287786c7a97eff520ffbd6b0d8b2f99dbfc6371
Tree-SHA512: 4b7dadc92451ab1ceb5a547a3101ff37f3ffd0645490563f1f3442ea8d6219f100ed914289d22435c4172d190fa1ff52e37e4464132bb3f9bbcc338488227f7b
1. secp256k1_fe_verify is removed from tests since, it throws an error if VERIFY is not defined during compilation.
(Ex: ./configure --enable-coverage)
2. `secp256k1_fe_from_storage` calls `secp256k1_fe_verify` in the VERIFY build to check for invalid field element.
There were several places where the code was non-constant time
for invalid secret inputs. These are harmless under sane use
but get in the way of automatic const-time validation.
(Nonce overflow in signing is not addressed, nor is s==0 in
signing)
Identifiers starting with an underscore and followed immediately by a capital letter are reserved by the C++ standard.
The only header guards not fixed are those in the headers auto-generated from java.
field_get_b32: min 0.890us / avg 0.905us / max 0.956us
field_set_b32: min 1.12us / avg 1.15us / max 1.19us
becomes
field_get_b32: min 0us / avg 0.000000119us / max 0.000000238us
field_set_b32: min 0.0532us / avg 0.0584us / max 0.0782us
Use a conditional move of the same kind we use for the affine points
in the storage type instead of multiplying with the infinity flag
and adding. This results in fewer constructions to worry about for
sidechannel behavior.
It also might be faster: It doesn't appear to benchmark as slower for
me at least; but I think the CMOV is faster than the mul_int + add,
but slower than the set+add; making it a wash.
Unbraced statements spanning multiple lines has been shown in many
projects to contribute to the introduction of bugs and a failure
to catch them in review, especially for maintenance on infrequently
modified code.
Most, but not all, of the existing practice in the codebase were not
cases that I would have expected to eventually result in bugs but
applying it as a rule makes it easier for other people to safely
contribute.
I'm not aware of any such evidence for the case with the statement
on a single line, but some people strongly prefer to never do that
and the opposite rule of "_always_ use a single line for single
statement blocks" isn't a reasonable rule for formatting reasons.
Might as well brace all these too, since that's more universally
acceptable.
[In any case, I seem to have introduced the vast majority of the
single-line form (as they're my preference where they fit).]
This also removes a broken test which is no longer needed.
C doesn't include the null in an array initilized from a
string literal if it doesn't fit, in C++ this is invalid.
The vararray style prototypes and init+calc also changed in
this commit are not C89 enough for some tools.
This makes the software more portable to embedded systems
and static analysis tools.
Sadly, it can't result in identical binaries because C99 mixed
declarations seem to make GCC emit superfluous stack-pointer
updates. The compiler is also somewhat dependent on the
declaration order.
