- `secp256k1_scalar_set_int` in scalar_low uses input mod EXHAUSTIVE_TEST_ORDER
- directly store s in sig64 without reducing it mod the group order for testing
b097a466c1 util: remove unused checked_realloc (Cory Fields)
Pull request description:
Usage was removed in 6fe50439 . This should be a NOOP.
Noticed when analyzing for zenbleed exposure: stdlib calls that aren't optimized away.
In this case realloc isn't making it into the final binary, but as far as I can tell this is completely dead code and should be dropped.
ACKs for top commit:
jonasnick:
ACK b097a466c1
real-or-random:
ACK b097a466c1
Tree-SHA512: d4249215eddd4035be2b50a8bb48b8a681abdab4ab41ca53f6c2a2507edfbc9ffa39ba22eb48e7da52f978e224198294495ce64f9d571d98c19283b20b82a63a
Remove also the explicit magnitude restriction `a->x.magnitude <= 31`
in `secp256k1_gej_eq_x_var` (introduced in commit
07c0e8b82e), as this is implied by the
new limits.
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
The group element checks `secp256k1_{ge,gej}_verify` have first been
implemented and added in commit f20266722a
(PR #1299). This commit adds additional verification calls in group
functions, to match the ones that were originally proposed in commit
09dbba561f of WIP-PR #1032 (which is
obviously not rebased on #1299 yet).
Also, for easier review, all functions handling group elements are
structured in the following wasy for easier review (idea suggested by
Tim Ruffing):
- on entry, verify all input ge, gej (and fe)
- empty line
- actual function body
- empty line
- on exit, verify all output ge, gej
Co-authored-by: Peter Dettman <peter.dettman@gmail.com>
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
c424e2fb43 ellswift: fix probabilistic test failure when swapping sides (Jonas Nick)
Pull request description:
Reported by jonatack in https://github.com/bitcoin/bitcoin/issues/28079.
When configured with `--disable-module-ecdh --enable-module-recovery`, then `./tests 64 81af32fd7ab8c9cbc2e62a689f642106` fails with
```
src/modules/ellswift/tests_impl.h:396: test condition failed: secp256k1_memcmp_var(share32_bad, share32a, 32) != 0
```
This tests verifies that changing the `party` bit of the `secp256k1_ellswift_xdh` function results in a different share. However, that's not the case when the secret keys of both parties are the same and this is actually what happens in the observed test failure. The keys can be equal in this test case because they are created by the `random_scalar_order_test` function whose output is not uniformly random (it's biased towards 0).
This commit restores the assumption that the secret keys differ.
ACKs for top commit:
sipa:
utACK c424e2fb43
real-or-random:
utACK c424e2fb43
Tree-SHA512: d1ab61473a77478f9aeffb21ad73e0bba478c90d8573c72ec89d2e0140434cc65c9d5f4d56e5f259931dc68fc1800695c6cd5d63d9cfce4c1c4d6744eeaa2028
When configured with `--disable-module-ecdh --enable-module-recovery`, then
`./tests 64 81af32fd7ab8c9cbc2e62a689f642106` fails with
```
src/modules/ellswift/tests_impl.h:396: test condition failed: secp256k1_memcmp_var(share32_bad, share32a, 32) != 0
```
This tests verifies that changing the `party` bit of the
`secp256k1_ellswift_xdh` function results in a different share. However, that's
not the case when the secret keys of both parties are the same and this is
actually what happens in the observed test failure. The keys can be equal in
this test case because they are created by the `random_scalar_order_test`
function whose output is not uniformly random (it's biased towards 0).
This commit restores the assummption that the secret keys differ.
This change simplifies running examples on Windows, because the DLL
must reside either in the same folder where the executable is or
somewhere in PATH.
981e5be38c ci: Fix typo in comment (Tim Ruffing)
e9e9648219 ci: Reduce number of macOS tasks from 28 to 8 (Tim Ruffing)
609093b387 ci: Add x86_64 Linux tasks for gcc and clang snapshots (Tim Ruffing)
1deecaaf3b ci: Install development snapshots of gcc and clang (Tim Ruffing)
Pull request description:
ACKs for top commit:
hebasto:
re-ACK 981e5be38c
jonasnick:
ACK 981e5be38c
Tree-SHA512: a36ef6f3c30a7f6e09e186e67b8eeb6e16e05de3bd97f21342866e75e33275103d463b6a12603ce235da7e26e4acdef4d811f62f369f18db9ac4e7ff06749136
b79ba8aa4c field: Use `restrict` consistently in fe_sqrt (Tim Ruffing)
Pull request description:
That is, use it also in the definition and not only the declaration.
I believe this was the intention of commit
be82bd8e03, but it was omitted there.
edit: Changed the description. I'm not entirely sure but after looking at the standard, I tend to think this is more than a cosmetic change, and only this change actually makes the parameters `restrict`. Anyway, I believe making them `restrict` was simply forgotten in be82bd8e03.
ACKs for top commit:
sipa:
utACK b79ba8aa4c
Tree-SHA512: eecec7674d8cef7833d50f4041b87241ca8de4839aa8027df1c422b89f5a1bcef3916ac785057a596c459ce1aa9d41e5a21ecb6fed9c5d15a1d9f588c7ee208e
600c5adcd5 clean up in-comment Sage code (refer to secp256k1_params.sage, update to Python3) (Sebastian Falbesoner)
Pull request description:
Some of the C source files contain contain in-comment Sage code calculating secp256k1 parameters that are already defined in the file secp256k1_params.sage. Replace that by a corresponding load instruction and access the necessary variables. In ecdsa_impl.h, update the comment to use a one-line shell command calling sage to get the values.
The remaining code (test `test_add_neg_y_diff_x` in tests.c) is updated to work with a current version based on Python3 (Sage 9.0+, see https://wiki.sagemath.org/Python3-Switch).
The latter can be seen as a small follow-up to PR #849 (commit 13c88efed0).
ACKs for top commit:
sipa:
ACK 600c5adcd5
real-or-random:
ACK 600c5adcd5
Tree-SHA512: a9e52f6afbce65edd9ab14203612c3d423639f450fe8f0d269a3dda04bebefa95b607f7aa0faec864cb78b46d49f281632bb1277118749b7d8613e9f5dcc8f3d
Some of the C source files contain contain in-comment Sage code
calculating secp256k1 parameters that are already defined in the file
secp256k1_params.sage. Replace that by a corresponding load instruction
and access the necessary variables. In ecdsa_impl.h, update the comment
to use a one-line shell command calling sage to get the values.
The remaining code (test `test_add_neg_y_diff_x` in tests.c) is updated
to work with a current version based on Python3 (Sage 9.0+, see
https://wiki.sagemath.org/Python3-Switch).
The latter can be seen as a small follow-up to PR #849 (commit
13c88efed0).
2792119278 Add exhaustive test for ellswift (create+decode roundtrip) (Sebastian Falbesoner)
Pull request description:
This PR adds the basic structure for ellswift exhaustive tests. Right now only a `secp256k1_ellswift_create` + `secp256k1_ellswift_decode` indirect roundtrip (exhaustive loop scalar -> ellswift pubkey -> decoded pubkey -> decoded group element, compared with exhaustive precomputed group element) is included.
The exhaustive tests passes locally with all currently supported orders (n=13 [default] and n=199). Note that for n=7, the test is skipped, as the used curve in this case is even-ordered and ellswift only supports odd-ordered curves.
ACKs for top commit:
sipa:
utACK 2792119278
real-or-random:
utACK 2792119278
Tree-SHA512: c51d3d99e9839793b3c15d75b9a29f01080db160ab8819973abd877288f9f0af972ea4264290220ab1cd035fdebcfac7767436aa39154d924ef0bf6a5733a55d
07c0e8b82e group: remove unneeded normalize_weak in `secp256k1_gej_eq_x_var` (Sebastian Falbesoner)
efa76c4bf7 group: remove unneeded normalize_weak in `secp256k1_ge_is_valid_var` (Sebastian Falbesoner)
Pull request description:
This PR removes unneeded normalize_weak calls in two group element functions:
* `secp256k1_ge_is_valid_var`: After calculating the right-hand side of the elliptic curve equation (x^3 + 7), the field element `x3` has a magnitude of 2 (1 as result of `secp256k1_fe_mul`, then increased by 1 due to `secp256k1_fe_add_int`). This is fine for `secp256k1_fe_equal_var`, as the second parameter only requires the magnitude to not exceed 31, and the normalize_weak call is hence not needed and can be dropped. Note that the interface description for `secp256k1_fe_equal` (which also applies to `secp256k1_fe_equal_var`) once stated that _both_ parameters need to have magnitude 1, but that was corrected in commit 7d7d43c6dd.
* `secp256k1_gej_eq_x_var`: By requiring that the input group element's X coordinate (`a->x`) has a magnitude of <= 31, the normalize_weak call and also the field element variable `r2` are not needed anymore and hence can be dropped.
ACKs for top commit:
sipa:
utACK 07c0e8b82e
jonasnick:
ACK 07c0e8b82e
Tree-SHA512: 9037e4af881ce7bf3347414d6da06b99e3d318733ba4f70e8b24d2320c2f26d022144e17bd6b95c1a4ef1be3825a4464e56ce2d2b3ae7bbced04257048832b7f
c6cd2b15a0 ci: Add task for static library on Windows + CMake (Hennadii Stepanov)
020bf69a44 build: Add extensive docs on visibility issues (Tim Ruffing)
0196e8ade1 build: Introduce `SECP256k1_DLL_EXPORT` macro (Hennadii Stepanov)
9f1b1904a3 refactor: Replace `SECP256K1_API_VAR` with `SECP256K1_API` (Hennadii Stepanov)
ae9db95cea build: Introduce `SECP256K1_STATIC` macro for Windows users (Hennadii Stepanov)
Pull request description:
Previous attempts:
- https://github.com/bitcoin-core/secp256k1/pull/1346
- https://github.com/bitcoin-core/secp256k1/pull/1362
The result is as follows:
1. Simple, concise and extensively documented code.
2. Explicitly documented use cases with no ambiguities.
3. No workarounds for linker warnings.
4. Solves one item in https://github.com/bitcoin-core/secp256k1/issues/1235.
ACKs for top commit:
real-or-random:
utACK c6cd2b15a0
Tree-SHA512: d58694452d630aefbd047916033249891bc726b7475433aaaa7c3ea2a07ded8f185a598385b67c2ee3440ec5904ff9d9452c97b0961d84dcb2eb2cf46caa171e
b6b9834e8d small fixes (Alejandro)
Pull request description:
Corrected some typos
ACKs for top commit:
real-or-random:
ACK b6b9834e8d
Tree-SHA512: c40c22c66f1067ecca351f08cca07a78b00bb98af2f6cfb08c25d0b1db6845e0e32ace1954c386db7020cf9fc7ae973ff15bd6d9c0144f3d21ea28c15741050f
By requiring that the input group element's X coordinate (`a->x`) has a
magnitude of <= 31, the normalize_weak call and also the field element
variable `r2` are not needed anymore and hence can be dropped.
5b9f37f136 ci: Add `CFLAGS: -O1` to task matrix (Hennadii Stepanov)
a6ca76cdf2 Avoid `-Wmaybe-uninitialized` when compiling with `gcc -O1` (Hennadii Stepanov)
Pull request description:
Fixes https://github.com/bitcoin-core/secp256k1/issues/1361.
CI tasks have been adjusted to catch similar issues in the future.
ACKs for top commit:
real-or-random:
utACK 5b9f37f136
jonasnick:
tACK 5b9f37f136
Tree-SHA512: 8aa5ec22ed88579ecd37681df68d64f8bab93cd14bdbf432a3af41cadc7ab3eba86c33c179db15bf3a3c798c33064bd845ebdedb02ee617ef634e98c596838c2
It is a non-Libtool-specific way to explicitly specify the user's
intention to consume a static `libseck256k1`.
This change allows to get rid of MSVC linker warnings LNK4217 and
LNK4286. Also, it makes possible to merge the `SECP256K1_API` and
`SECP256K1_API_VAR` into one.
5a95a268b9 tests: introduce helper for non-zero `random_fe_test` results (Sebastian Falbesoner)
304421d57b tests: refactor: remove duplicate function `random_field_element_test` (Sebastian Falbesoner)
Pull request description:
There are several instances in the tests where random non-zero field elements are generated by calling `random_fe_test` in a do/while-loop with is-zero condition. This PR deduplicates all these by introducing a `random_fe_non_zero_test` helper. Note that some instances checked the is-zero condition via `secp256k1_fe_normalizes_to_zero_var`, which is unnecessary, as the result of `random_field_element_test` is already normalized (so strictly speaking, this is not a pure refactor, and there could be tiny run-time improvements, though I doubt that's measurable).
Additionally, the first commit removes the function `random_field_element_test` as it is logically a duplicate of `random_fe_test`.
ACKs for top commit:
real-or-random:
ACK 5a95a268b9
Tree-SHA512: 920404f38ebe8b84bfd52f3354dc17ae6a0fd6355f99b78c9aeb53bf21f7eca5fd4518edc8a422d84f430ae95864661b497de42a3ab7fa9c49515a1df2f1d466
