981e5be38c492f0c0230fbe61be555d157380331 ci: Fix typo in comment (Tim Ruffing)
e9e96482196da641733a8a6763341a84f8b9806a ci: Reduce number of macOS tasks from 28 to 8 (Tim Ruffing)
609093b3877b2fb21bd4bb2301a3eafb444a2fdb ci: Add x86_64 Linux tasks for gcc and clang snapshots (Tim Ruffing)
1deecaaf3b94dbf08896e015e7f1e5ec328a40f2 ci: Install development snapshots of gcc and clang (Tim Ruffing)
Pull request description:
ACKs for top commit:
hebasto:
re-ACK 981e5be38c492f0c0230fbe61be555d157380331
jonasnick:
ACK 981e5be38c492f0c0230fbe61be555d157380331
Tree-SHA512: a36ef6f3c30a7f6e09e186e67b8eeb6e16e05de3bd97f21342866e75e33275103d463b6a12603ce235da7e26e4acdef4d811f62f369f18db9ac4e7ff06749136
b79ba8aa4c074b2cd09188f6f85ba68d6b80fe50 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 be82bd8e0347e090037ff1d30a22a9d614db8c9f.
ACKs for top commit:
sipa:
utACK b79ba8aa4c074b2cd09188f6f85ba68d6b80fe50
Tree-SHA512: eecec7674d8cef7833d50f4041b87241ca8de4839aa8027df1c422b89f5a1bcef3916ac785057a596c459ce1aa9d41e5a21ecb6fed9c5d15a1d9f588c7ee208e
600c5adcd59240305e22918943f45dceeabb7e93 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 13c88efed0005eb6745a222963ee74564054eafb).
ACKs for top commit:
sipa:
ACK 600c5adcd59240305e22918943f45dceeabb7e93
real-or-random:
ACK 600c5adcd59240305e22918943f45dceeabb7e93
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
13c88efed0005eb6745a222963ee74564054eafb).
That is, use it also in the definition and not only the declaration.
I believe this was the intention of commit
be82bd8e0347e090037ff1d30a22a9d614db8c9f, but it was omitted there.
2792119278bcb2a0befce3fbc64c83578df54953 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 2792119278bcb2a0befce3fbc64c83578df54953
real-or-random:
utACK 2792119278bcb2a0befce3fbc64c83578df54953
Tree-SHA512: c51d3d99e9839793b3c15d75b9a29f01080db160ab8819973abd877288f9f0af972ea4264290220ab1cd035fdebcfac7767436aa39154d924ef0bf6a5733a55d
07c0e8b82e2cea87f85263512945fed7adffea18 group: remove unneeded normalize_weak in `secp256k1_gej_eq_x_var` (Sebastian Falbesoner)
efa76c4bf7cab1c22aa476cd2730e891450ad4a0 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 7d7d43c6dd2741853de4631881d77ae38a14cd23.
* `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 07c0e8b82e2cea87f85263512945fed7adffea18
jonasnick:
ACK 07c0e8b82e2cea87f85263512945fed7adffea18
Tree-SHA512: 9037e4af881ce7bf3347414d6da06b99e3d318733ba4f70e8b24d2320c2f26d022144e17bd6b95c1a4ef1be3825a4464e56ce2d2b3ae7bbced04257048832b7f
c6cd2b15a007ad0a2d5c4656ae641ba442d8b2fe ci: Add task for static library on Windows + CMake (Hennadii Stepanov)
020bf69a44ba700624d09de0c18ceb867369d24e build: Add extensive docs on visibility issues (Tim Ruffing)
0196e8ade16e2b2d8efadac01d8520205553ee39 build: Introduce `SECP256k1_DLL_EXPORT` macro (Hennadii Stepanov)
9f1b1904a358e4ce7248c6542e8c7ac143ba0e3f refactor: Replace `SECP256K1_API_VAR` with `SECP256K1_API` (Hennadii Stepanov)
ae9db95ceaa2605138fac9c237c640acea3f3bd6 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 c6cd2b15a007ad0a2d5c4656ae641ba442d8b2fe
Tree-SHA512: d58694452d630aefbd047916033249891bc726b7475433aaaa7c3ea2a07ded8f185a598385b67c2ee3440ec5904ff9d9452c97b0961d84dcb2eb2cf46caa171e
b6b9834e8da7f3fd91b95f750a4ee7a10bf67435 small fixes (Alejandro)
Pull request description:
Corrected some typos
ACKs for top commit:
real-or-random:
ACK b6b9834e8da7f3fd91b95f750a4ee7a10bf67435
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.
5b9f37f136620b9c61cd66439904b2db266fba70 ci: Add `CFLAGS: -O1` to task matrix (Hennadii Stepanov)
a6ca76cdf2a3d0aef091e3d26d7c6c8ee9c88e72 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 5b9f37f136620b9c61cd66439904b2db266fba70
jonasnick:
tACK 5b9f37f136620b9c61cd66439904b2db266fba70
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.
5a95a268b944ffe64b7857e58f5b3b44aba514da tests: introduce helper for non-zero `random_fe_test` results (Sebastian Falbesoner)
304421d57b66670428de656ae6b3272c1ab6fde5 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 5a95a268b944ffe64b7857e58f5b3b44aba514da
Tree-SHA512: 920404f38ebe8b84bfd52f3354dc17ae6a0fd6355f99b78c9aeb53bf21f7eca5fd4518edc8a422d84f430ae95864661b497de42a3ab7fa9c49515a1df2f1d466
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.
This commit 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_fe_test` is already normalized (so
strictly speaking, this is not a pure refactor).
There is a function `random_fe_test` which does exactly the
same, so use that instead. Note that it's also moved up before the
`random_group_element_test` function, in order to avoid needing a forward
declaration.
be8ff3a02aeff87c60d49883a1b2afa8b2999bbe field: Static-assert that int args affecting magnitude are constant (Tim Ruffing)
Pull request description:
See #1001.
Try to revert the lines in `tests.c` to see the error message in action.
ACKs for top commit:
sipa:
ACK be8ff3a02aeff87c60d49883a1b2afa8b2999bbe. Verified by introducing some non-constant expressions and seeing compilation fail.
theStack:
ACK be8ff3a02aeff87c60d49883a1b2afa8b2999bbe
Tree-SHA512: 8befec6ee64959cdc7f3e29b4b622410794cfaf69e9df8df17600390a93bc787dba5cf86239de6eb2e99c038b9aca5461e4b3c82f0e0c4cf066ad7c689941b19
7d8d5c86df8b27b45e80ed50341dd0ce64546c0f tests: refactor: take use of `secp256k1_ge_x_on_curve_var` (Sebastian Falbesoner)
Pull request description:
The recently merged ellswift PR (#1129) introduced a helper `secp256k1_ge_x_on_curve_var` to check if a given X coordinate is on the curve (i.e. the expression x^3 + 7 is square, see commit 79e5b2a8b80f507e2c9936ff1c4e2fb39bc66a4e). This can be used for code deduplication in the `ecmult_const_mult_xonly` test.
(Found this instance via `$ git grep add_int.*SECP256K1_B`, I think it's the only one where the helper can be used.)
ACKs for top commit:
sipa:
utACK 7d8d5c86df8b27b45e80ed50341dd0ce64546c0f
real-or-random:
utACK 7d8d5c86df8b27b45e80ed50341dd0ce64546c0f
Tree-SHA512: aebff9b5ef2f6f6664ce89e4e1272cb55b6aac81cfb379652c4b7ab30dd1d7fd82a2c3b47c7b7429755ba28f011a3a9e2e6d3aa5c77d3b105d159104c24b89f3
c862a9fb49e885dcafb42d4e21e05a244248aab0 ci: Adjust Docker image to Debian 12 "bookworm" (Hennadii Stepanov)
a1782098a9f0174aa7b7da431bf77c009dfeef51 ci: Force DWARF v4 for Clang when Valgrind tests are expected (Hennadii Stepanov)
8a7273465b3b17d6dedc67c7aac32a89a0a4dacf Help the compiler prove that a loop is entered (Tim Ruffing)
Pull request description:
Since the [release](https://www.debian.org/News/2023/20230610.html) of Debian 12 "bookworm", it has become the "stable" one that our `ci/linux-debian.Dockerfile` relies on.
Last time the Docker image was built basing on Debian Bullseye.
Changes in packages are significant, for instance:
- `gcc` 10.2. --> 12.2
- `clang` 11.0 --> 14.0
- `wine` 5.0 --> 8.0
which requires certain adjustments provided in this PR.
The first commit has been cherry-picked from https://github.com/bitcoin-core/secp256k1/pull/1313.
ACKs for top commit:
sipa:
utACK c862a9fb49e885dcafb42d4e21e05a244248aab0
real-or-random:
ACK c862a9fb49e885dcafb42d4e21e05a244248aab0
Tree-SHA512: 2a62a8865f904a460274f1f3ec02d2b0b72c84b25722a383c6455cfe672c1d93382941a5027e8dceb2c0f5fe0f0efd49a0ed6b72303982f9e32991f1535538eb
The recently merged ellswift PR (#1129) introduced a helper
`secp256k1_ge_x_on_curve_var` to check if a given X coordinate is
valid (i.e. the expression x^3 + 7 is square, see commit
79e5b2a8b80f507e2c9936ff1c4e2fb39bc66a4e). This can be used for code
deduplication in the `ecmult_const_mult_xonly` test.
67887ae65cf11d02c7055709082acd0e5d86db9b Fix a typo in the error message (Hennadii Stepanov)
Pull request description:
The code has been copy-pasted from the `precompute_ecmult_gen.c` source file.
ACKs for top commit:
real-or-random:
ACK 67887ae65cf11d02c7055709082acd0e5d86db9b
Tree-SHA512: d6874949310197e5d2d6c43f5a7c2165b4ee0f6cbe3cc1491d0f97163fa5329ebeab2b2adf10246c87382016fbe738c69dfd3f2253e93c906bf404cbf439b12a
7c7467ab7f935f6b982064c8c48772a433da1f8f Refer to ellswift.md in API docs (Pieter Wuille)
c32ffd8d8c833a964ee7fbb294640764ad25de5d Add ellswift to CHANGELOG (Pieter Wuille)
Pull request description:
A follow-up with a CHANGELOG entry for #1129.
ACKs for top commit:
real-or-random:
ACK 7c7467ab7f935f6b982064c8c48772a433da1f8f
theStack:
ACK 7c7467ab7f935f6b982064c8c48772a433da1f8f
Tree-SHA512: 4f066e4b8d5e130f2b5bea0ed4c634e9426bc576342aad6c306e0805a8354e27a5e679b15ec869d4e7d36eb5d53174e46b3bf5e15d19a7e165afc82e46ddfcf5
bc7c8db179a56cf7273f3c4c0decd10543a10521 abi: Use dllexport for mingw builds (Cory Fields)
Pull request description:
Addresses the first part of #1181. See the discussion there for more context and history.
After this, all that remains is a (platform-independent) exports checker for c-i. Or perhaps a linker script or .def file could be tricked into testing as a side-effect.
This should fix mingw exports, specifically hiding the following:
`secp256k1_pre_g_128`
`secp256k1_pre_g`
`secp256k1_ecmult_gen_prec_table`
This changes our visibility macros to look more like [gcc's recommendation](https://gcc.gnu.org/wiki/Visibility#How_to_use_the_new_C.2B-.2B-_visibility_support).
Edit:
Note that we could further complicate this by supporting `__attribute__ ((dllexport))` as well, though I didn't bother as I'm not sure what compiler combo would accept that but not the bare dllexport syntax.
Edit2:
As the title implies, this affects this ABI and could affect downstream libs/apps in unintended ways (though it's hard to imagine any real downside). Though because it's win32 only, I'm imagining very little real-world impact at all.
ACKs for top commit:
hebasto:
re-ACK bc7c8db179a56cf7273f3c4c0decd10543a10521, only a comment has been adjusted since my recent [review](https://github.com/bitcoin-core/secp256k1/pull/1295#pullrequestreview-1414928537),
real-or-random:
utACK bc7c8db179a56cf7273f3c4c0decd10543a10521
Tree-SHA512: 378e15556da49494f551bdf4f7b41304db9d03a435f21fcc947c9520aa43e3c655cfe216fba57a5179a871c975c806460eef7c33b105f2726e1de0937ff2444e
5b7bf2e9d4ee02cbec1105ad6e890c34a4da1beb Use `__shiftright128` intrinsic in `secp256k1_u128_rshift` on MSVC (Hennadii Stepanov)
Pull request description:
Closes https://github.com/bitcoin-core/secp256k1/issues/1324.
As the `__shiftright128` [docs](https://learn.microsoft.com/en-us/cpp/intrinsics/shiftright128) state:
> The `Shift` value is always modulo 64...
it is not applicable for the `n >= 64` branch.
ACKs for top commit:
sipa:
utACK 5b7bf2e9d4ee02cbec1105ad6e890c34a4da1beb
real-or-random:
ACK 5b7bf2e9d4ee02cbec1105ad6e890c34a4da1beb tested with MSVC x64
Tree-SHA512: bc4c245a9da83c783a0479e751a4bc2ec77a34b99189fcc4431033a5420c93b610f3b960d3f23c15bce2eb010beba665b3e84d468b3fdab3d5846d4f27016898
