The same file says that the illegal callback will only triger for violations
explicitly mentioned, which is not true without this commit because we often
don't mention that an argument is not allowed to be NULL.
This line is extracted from #783 in the hope that it gets merged faster because other PRs depend on it.
Tim Ruffing [Thu, 6 May 2021 07:38:18 +0000 (09:38 +0200)]
Merge #937: Have ge_set_gej_var, gej_double_var and ge_set_all_gej_var initialize all fields of their outputs.
14c9739a1fb485bb56dbe3447132a37bcbef4e22 tests: Improve secp256k1_ge_set_all_gej_var for some infinity inputs (Tim Ruffing) 4a19668c37bc77d0165f4a1c0e626e321e9c4a09 tests: Test secp256k1_ge_set_all_gej_var for all infinity inputs (Tim Ruffing) 45b6468d7e3ed9849ed474c71e9a9479de1a77db Have secp256k1_ge_set_all_gej_var initialize all fields. Previous behaviour would not initialize r->y values in the case where infinity is passed in. Furthermore, the previous behaviour wouldn't initialize anything in the case where all inputs were infinity. (Russell O'Connor) 31c0f6de413e521731ad0e63424431b3dd49cec8 Have secp256k1_gej_double_var initialize all fields. Previous behaviour would not initialize r->x and r->y values in the case where infinity is passed in. (Russell O'Connor) dd6c3de322740a3054cf6a1994a38dc8f201b473 Have secp256k1_ge_set_gej_var initialize all fields. Previous behaviour would not initialize r->x and r->y values in the case where infinity is passed in. (Russell O'Connor)
Pull request description:
Previous behaviour would not initialize `r->x` and `r->y` values in the case where infinity is passed in.
Referencing #924 , this PR splits the two issues brought on to a smaller to digest change. What this does is removes the prefix "include/" when referencing the local library header files.
Rationale besides styling and consistency across other files in the repo, it makes it easier for outside builds to properly locate the headers.
A live example seen here when attempting to build this library within bitcoin repo:
```sh
[ 14%] Building CXX object leveldb/CMakeFiles/leveldb.dir/util/bloom.cc.o
/tmp/bitcoin/src/secp256k1/src/secp256k1.c:7:10: fatal error: include/secp256k1.h: No such file or directory
7 | #include "include/secp256k1.h"
| ^~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [secp256k1/CMakeFiles/Secp256k1.dir/build.make:76: secp256k1/CMakeFiles/Secp256k1.dir/src/secp256k1.c.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:537: secp256k1/CMakeFiles/Secp256k1.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
Have secp256k1_ge_set_all_gej_var initialize all fields.
Previous behaviour would not initialize r->y values in the case where infinity is passed in.
Furthermore, the previous behaviour wouldn't initialize anything in the case where all inputs were infinity.
Have secp256k1_gej_double_var initialize all fields.
Previous behaviour would not initialize r->x and r->y values in the case where infinity is passed in.
Have secp256k1_ge_set_gej_var initialize all fields.
Previous behaviour would not initialize r->x and r->y values in the case where infinity is passed in.
ACKs for top commit:
gmaxwell:
ACK c8483520c9077905a1dc8b9adb88b6ea2a3bd9ef looks good and works here. Undefign is kinda yuck, but it is already doing it and it's cleaner than the obvious alternatives.
sipa:
utACK c8483520c9077905a1dc8b9adb88b6ea2a3bd9ef. I verified that building still works on ARM64, but without asm of course.
Jonas Nick [Wed, 28 Apr 2021 16:57:49 +0000 (16:57 +0000)]
secp256k1.h: clarify that by default arguments must be != NULL
The same file says that the illegal callback will only triger for violations
explicitly mentioned, which is not true without this commit because we often
don't mention that an argument is not allowed to be NULL.
This updates the divsteps-based modular inverse code to use the modified version which starts with delta=1/2. For variable time, the delta=1 variant is still used as it appears to be faster.
See https://github.com/sipa/safegcd-bounds/tree/master/coq and https://medium.com/blockstream/a-formal-proof-of-safegcd-bounds-695e1735a348 for a proof of correctness of this variant.
TODO:
* [x] Update unit tests to include edge cases specific to this variant
I'm still running the Coq proof verification for the 590 bound in non-native mode. It's unclear how long this will take.
Tim Ruffing [Thu, 15 Apr 2021 14:17:53 +0000 (16:17 +0200)]
gen_context: Don't include basic-config.h
Before this commit, gen_context.c both included libsecp256k1-config.h
and basic-config.h: The former only to obtain ECMULT_GEN_PREC_BITS
and the latter to obtain a basic working configuration to be able to
use the library.
This was inelegant and confusing: It meant that basic-config.h needs
to #undef all the macros defined in libsecp256k1-config.h. Moreover,
it meant that basic-config.h cannot define ECMULT_GEN_PREC_BITS,
essentially making this file specific for use in gen_context.c.
After this commit, gen_context.c include only libsecp256k1-config.h.
basic-config.h is not necessary anymore for the modules used in
gen_context.c because 79f1f7a made the preprocessor detect all the
relevant config options.
On the way, we remove an unused #define in basic-config.h.
Pieter Wuille [Fri, 1 Jan 2021 19:15:10 +0000 (11:15 -0800)]
Use modified divsteps with initial delta=1/2 for constant-time
Instead of using eta=-delta, use zeta=-(delta+1/2) to represent
delta. This variant only needs at most 590 iterations for 256-bit
inputs rather than 724 (by convex hull bounds analysis).
This is a rebased and squashed version of #767, adding safegcd-based implementations of constant-time and variable-time modular inverses for scalars and field elements, by Peter Dettman. The PR is organized as follows:
* **Add secp256k1_ctz{32,64}_var functions** Introduction of ctz functions to util.h (which use `__builtin_ctz` on recent GCC and Clang, but fall back to using a software emulation using de Bruijn on other platforms). This isn't used anywhere in this commit, but does include tests.
* **Add safegcd based modular inverse modules** Add Peter Dettman's safegcd code from #767 (without some of his optimizations, which are moved to later commits), turned into separate modules by me.
* **Add extensive comments on the safegcd algorithm and implementation** Add a long description of the algorithm and optimizations to `doc/safegcd_implementation.md`, as well as additional comments to the code itself. It is probably best to review this together with the previous commit (they're separated to keep authorship).
* **Add tests for modinv modules** Adds tests on the modinv interface directly, for arbitrary moduli.
* **Improve bounds checks in modinv modules** Adds a lot of sanity checking to the modinv modules.
* **Move secp256k1_scalar_{inverse{_var},is_even} to per-impl files** A pure refactor to prepare for switching the field and scalar code to modinv.
* **Make field/scalar code use the new modinv modules for inverses** Actually switch over.
* **Add extra modular inverse tests** This adds modular inverse tests through the field/scalar interface, now that those use modinv.
* **Remove unused Jacobi symbol support** No longer needed.
* **Remove num/gmp support** Bye-bye.
* 3 commits with further optimizations.
Peter Dettman [Wed, 16 Dec 2020 02:17:19 +0000 (18:17 -0800)]
Optimization: track f,g limb count and pass to new variable-time update_fg_var
The magnitude of the f and g variables generally goes down as the algorithm
progresses. Make use of this by keeping tracking how many limbs are used, and
when the number becomes small enough, make use of this to reduce the complexity
of arithmetic on them.
Pieter Wuille [Sat, 28 Nov 2020 23:58:22 +0000 (15:58 -0800)]
Optimization: special-case zero modulus limbs in modinv64
Both the field and scalar modulus can be written in signed{30,62} notation
with one or more zero limbs. Make use of this in the update_de function to
avoid a few wide multiplications when that is the case.
This doesn't appear to be a win in the 32-bit implementation, so only
do it for the 64-bit one.
Pieter Wuille [Mon, 12 Oct 2020 06:20:32 +0000 (23:20 -0700)]
Improve field/scalar inverse tests
Add a new run_inverse_tests that replaces all existing field/scalar inverse tests,
and tests a few identities for fixed inputs, small numbers (-999...999), random
inputs (structured and unstructured), as well as comparing with the output of
secp256k1_fe_inv_all_var.
Pieter Wuille [Wed, 23 Dec 2020 19:13:57 +0000 (11:13 -0800)]
Improve bounds checks in modinv modules
This commit adds functions to verify and compare numbers in signed{30,62} notation,
and uses that to do more extensive bounds checking on various variables in the modinv
code.
Pieter Wuille [Wed, 23 Dec 2020 02:24:36 +0000 (18:24 -0800)]
Add tests for modinv modules
This adds tests for the modinv{32,64}_impl.h directly (before the functions are used
inside the field/scalar code). It uses a naive implementation of modular multiplication
and gcds in order to verify the modular inverses themselves.
Tim Ruffing [Fri, 26 Feb 2021 14:52:40 +0000 (15:52 +0100)]
ci: Switch all Linux builds to Debian
The experiment of using Nix Shell was not really successful. Most
notably, Nix uses a bunch of wrapper scripts around compilers, which
make the build much less "pure". This may be useful but it's exactly
not what we want for CI. In particular, this resulted in gcc being used
for the "clang" builds because a wrapper script redefined the CC env
variable.
This now builds a single docker image (Debian) for all architectures
that we test in CI on Linux.
PiRK [Sun, 31 Jan 2021 17:41:35 +0000 (18:41 +0100)]
print warnings for conditional-uninitialized
This compiler flag is available for clang but not gcc.
Test plan:
```
autogen.sh
./configure
make check
CC=clang ./configure
make check
```
If a variable is used uninitialized, the warning should look something
like:
```
CC src/tests-tests.o
src/tests.c:4336:15: warning: variable 'recid' may be uninitialized when used here [-Wconditional-uninitialized]
CHECK(recid >= 0 && recid < 4);
^~~~~
./src/util.h:54:18: note: expanded from macro 'CHECK'
if (EXPECT(!(cond), 0)) { \
^~~~
./src/util.h:41:39: note: expanded from macro 'EXPECT'
^
src/tests.c:4327:14: note: initialize the variable 'recid' to silence this warning
int recid;
^
= 0
1 warning generated.
```
Tim Ruffing [Tue, 26 Jan 2021 08:36:14 +0000 (09:36 +0100)]
Merge #880: Add parens around ROUND_TO_ALIGN's parameter.
b6f649889ae78573f1959f04172a8e1fe15beab7 Add parens around ROUND_TO_ALIGN's parameter. This makes the macro robust against a hypothetical ROUND_TO_ALIGN(foo ? sizeA : size B) invocation. (Russell O'Connor)
Pull request description:
This makes the macro robust against a hypothetical `ROUND_TO_ALIGN(foo ? sizeA : size B)` invocation.
See also <https://wiki.sei.cmu.edu/confluence/display/c/PRE01-C.+Use+parentheses+within+macros+around+parameter+names>.
Jonas Nick [Mon, 25 Jan 2021 13:57:35 +0000 (13:57 +0000)]
Merge #874: Remove underscores from header defs.
fb390c5299e999e06b7dff9e77e373600fae9fdf Remove underscores from header defs. This makes them consistent with other files and avoids reserved identifiers. (Russell O'Connor)
See individual commit messages. These are improvements in preparation of the switch to Cirrus CI. (Maybe I'll just open a PR on top of this one.)
The first commit made the difference between successful build https://cirrus-ci.com/task/6740575057608704 and unsuccessful build https://cirrus-ci.com/task/4909571074424832.
I've tested the second commit without cross-compilation and with cross-compilation for android (https://github.com/bitcoin-core/secp256k1/issues/621#issuecomment-495703399)
When working on the autoconf stuff, I noticed two things that I just want to write down here:
- At some point we should update [build-aux/m4/ax_prog_cc_for_build.m4](https://www.gnu.org/software/autoconf-archive/ax_prog_cc_for_build.html). This is outdated, and [there have been a lot of fixes](https://github.com/autoconf-archive/autoconf-archive/pull/207) But the latest version is [broken](https://lists.gnu.org/archive/html/autoconf-archive-maintainers/2020-06/msg00002.html), so now is probably not the time.
- The latest autoconf 2.70 deprecates `AC_PROG_CC_C89`. It's not needed anymore because `AC_PROG_CC` cares about testing for version support. This makes autoconf 2.70 output a warning that we should probably just ignore. We don't want to force users onto 2.70...
With schnorrsig if you need to tweak the secret key (for BIP32) you must use the keypair API to get compatible secret/public keys which you do by calling `secp256k1_keypair_xonly_tweak_add()`, but after that there's no currently a way to extract the secret key back for storage.
so I added a `secp256k1_keypair_seckey` function to extract the key
Tim Ruffing [Sat, 2 Jan 2021 14:15:21 +0000 (15:15 +0100)]
Improve CC_FOR_BUILD detection
This commits simply uses CC as CC_FOR_BUILD and the same for
corresponding flags if we're not cross-compiling. This has a number of
benefits in this common case:
- It avoids strange cases where very old compilers are used (#768).
- Flags are consistently set for CC and CC_FOR_BUILD.
- ./configure is faster.
- You get compiler x consistently if you set CC=x; we got this wrong
in CI in the past.
./configure warns if a _FOR_BUILD variable is set but ignored because
we're not cross-compiling.
The change exposed that //-style comments are used in gen_context.c,
which is also fixed by this commit.
This commit also reorganizes code in configure.ac to have a cleaner
separation of sections.
Tim Ruffing [Wed, 23 Dec 2020 21:08:03 +0000 (22:08 +0100)]
Ask brew for valgrind include path
Valgrind is typically installed using brew on macOS. This commit
makes ./configure detect this case set the appropriate include
directory (in the same way as we already do for openssl and gmp).
projects / secp256k1.git / log