Upstream PRs 1357, 1345, 1358, 1368, 1369, 1323, 1364, 1370, 1367, 1344, 1371, 1363, 1340, 1366, 1313, 1378, 1298, 1382, 1383, 1384, 1386 #259
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Our RNG has been replaced with Xoshiro256++, a well-analyzed RNG. Our unit tests should not be resposible for verifying its statistical qualities.
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 can hence be dropped.
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 79e5b2a). This can be used for code deduplication in the `ecmult_const_mult_xonly` test.
…256k1_ge_x_on_curve_var` 7d8d5c8 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 79e5b2a). 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 7d8d5c8 real-or-random: utACK 7d8d5c8 Tree-SHA512: aebff9b5ef2f6f6664ce89e4e1272cb55b6aac81cfb379652c4b7ab30dd1d7fd82a2c3b47c7b7429755ba28f011a3a9e2e6d3aa5c77d3b105d159104c24b89f3
… affecting magnitude are constant be8ff3a 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 be8ff3a. Verified by introducing some non-constant expressions and seeing compilation fail. theStack: ACK be8ff3a Tree-SHA512: 8befec6ee64959cdc7f3e29b4b622410794cfaf69e9df8df17600390a93bc787dba5cf86239de6eb2e99c038b9aca5461e4b3c82f0e0c4cf066ad7c689941b19
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.
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).
…ro `random_fe_test()` results 5a95a26 tests: introduce helper for non-zero `random_fe_test` results (Sebastian Falbesoner) 304421d 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 5a95a26 Tree-SHA512: 920404f38ebe8b84bfd52f3354dc17ae6a0fd6355f99b78c9aeb53bf21f7eca5fd4518edc8a422d84f430ae95864661b497de42a3ab7fa9c49515a1df2f1d466
This is no longer necessary as of cirruslabs/cirrus-ci-docs#791 (comment) .
This change adds the same functionality to Windows containers that is already available in Linux containers.
a7bec34 ci: Print commit in Windows container (Hennadii Stepanov) Pull request description: This PR is a follow-up to bitcoin-core/secp256k1#1368 and adds the same functionality to Windows containers that is already available in Linux containers. See: bitcoin-core/secp256k1#1368 (comment). ACKs for top commit: real-or-random: ACK a7bec34 seems to work: https://cirrus-ci.com/task/4919320090771456?logs=git_show#L2 Tree-SHA512: 0998e0f7231e3057a7e358a27b34071c73ca556973da20494db84fc67f2a72ad2fe582e59647a425ee41e7d9103a0a22fb3cdf0ace6fe0aed1d21f2f75c8ec53
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.
This change provides a way to build a shared library that is not tired to the Libtool-specific `DLL_EXPORT` macro.
… compiling with `gcc -O1` 5b9f37f ci: Add `CFLAGS: -O1` to task matrix (Hennadii Stepanov) a6ca76c Avoid `-Wmaybe-uninitialized` when compiling with `gcc -O1` (Hennadii Stepanov) Pull request description: Fixes bitcoin-core/secp256k1#1361. CI tasks have been adjusted to catch similar issues in the future. ACKs for top commit: real-or-random: utACK 5b9f37f jonasnick: tACK 5b9f37f Tree-SHA512: 8aa5ec22ed88579ecd37681df68d64f8bab93cd14bdbf432a3af41cadc7ab3eba86c33c179db15bf3a3c798c33064bd845ebdedb02ee617ef634e98c596838c2
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.
restoring wycheproof files restoring wycheproof files2
…bility logic on Windows (attempt 3) c6cd2b1 ci: Add task for static library on Windows + CMake (Hennadii Stepanov) 020bf69 build: Add extensive docs on visibility issues (Tim Ruffing) 0196e8a build: Introduce `SECP256k1_DLL_EXPORT` macro (Hennadii Stepanov) 9f1b190 refactor: Replace `SECP256K1_API_VAR` with `SECP256K1_API` (Hennadii Stepanov) ae9db95 build: Introduce `SECP256K1_STATIC` macro for Windows users (Hennadii Stepanov) Pull request description: Previous attempts: - bitcoin-core/secp256k1#1346 - bitcoin-core/secp256k1#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 bitcoin-core/secp256k1#1235. ACKs for top commit: real-or-random: utACK c6cd2b1 Tree-SHA512: d58694452d630aefbd047916033249891bc726b7475433aaaa7c3ea2a07ded8f185a598385b67c2ee3440ec5904ff9d9452c97b0961d84dcb2eb2cf46caa171e
…n `secp256k1_ge_is_valid_var`/`secp256k1_gej_eq_x_var` 07c0e8b group: remove unneeded normalize_weak in `secp256k1_gej_eq_x_var` (Sebastian Falbesoner) efa76c4 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 7d7d43c. * `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 07c0e8b jonasnick: ACK 07c0e8b Tree-SHA512: 9037e4af881ce7bf3347414d6da06b99e3d318733ba4f70e8b24d2320c2f26d022144e17bd6b95c1a4ef1be3825a4464e56ce2d2b3ae7bbced04257048832b7f
… to Python3) 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 13c88ef).
…er to secp256k1_params.sage, update to Python3) 600c5ad 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 13c88ef). ACKs for top commit: sipa: ACK 600c5ad real-or-random: ACK 600c5ad Tree-SHA512: a9e52f6afbce65edd9ab14203612c3d423639f450fe8f0d269a3dda04bebefa95b607f7aa0faec864cb78b46d49f281632bb1277118749b7d8613e9f5dcc8f3d
… in fe_sqrt b79ba8a 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 bitcoin-core/secp256k1@be82bd8, 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 be82bd8. ACKs for top commit: sipa: utACK b79ba8a Tree-SHA512: eecec7674d8cef7833d50f4041b87241ca8de4839aa8027df1c422b89f5a1bcef3916ac785057a596c459ce1aa9d41e5a21ecb6fed9c5d15a1d9f588c7ee208e
…of GCC and Clang 981e5be ci: Fix typo in comment (Tim Ruffing) e9e9648 ci: Reduce number of macOS tasks from 28 to 8 (Tim Ruffing) 609093b ci: Add x86_64 Linux tasks for gcc and clang snapshots (Tim Ruffing) 1deecaa ci: Install development snapshots of gcc and clang (Tim Ruffing) Pull request description: ACKs for top commit: hebasto: re-ACK 981e5be jonasnick: ACK 981e5be Tree-SHA512: a36ef6f3c30a7f6e09e186e67b8eeb6e16e05de3bd97f21342866e75e33275103d463b6a12603ce235da7e26e4acdef4d811f62f369f18db9ac4e7ff06749136
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.
…ailure when swapping sides c424e2f ellswift: fix probabilistic test failure when swapping sides (Jonas Nick) Pull request description: Reported by jonatack in bitcoin/bitcoin#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 c424e2f real-or-random: utACK c424e2f Tree-SHA512: d1ab61473a77478f9aeffb21ad73e0bba478c90d8573c72ec89d2e0140434cc65c9d5f4d56e5f259931dc68fc1800695c6cd5d63d9cfce4c1c4d6744eeaa2028
6ec3731 Simplify test PRNG implementation (Pieter Wuille) fb5bfa4 Add static test vector for Xoshiro256++ (Tim Ruffing) 723e8ca Remove randomness tests (Pieter Wuille) Pull request description: ACKs for top commit: real-or-random: utACK 6ec3731 jonasnick: ACK 6ec3731 Tree-SHA512: 4cbbb9c42e31f067b17dd9169ae5d5e68bce77d1253452db9df523d3be2b5d61002d5a4203e5a153f257ec63c5ff2113555743eeb402d4b6c573069ea494d407
Usage was removed in 6fe5043 .
b097a46 util: remove unused checked_realloc (Cory Fields) Pull request description: Usage was removed in 6fe5043 . 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 b097a46 real-or-random: ACK b097a46 Tree-SHA512: d4249215eddd4035be2b50a8bb48b8a681abdab4ab41ca53f6c2a2507edfbc9ffa39ba22eb48e7da52f978e224198294495ce64f9d571d98c19283b20b82a63a
...like the other modules.
…SECP_CONFIG_DEFINES 78ca880 build: enable ellswift module via SECP_CONFIG_DEFINES (Jonas Nick) Pull request description: ...like the other modules. ACKs for top commit: sipa: utACK 78ca880 real-or-random: utACK 78ca880 Tree-SHA512: c157a1ed912b9aa1a318aa0a70859a3ac67cb22303993f08ff00ed601e6ac197380dd503d3b361cbc4e698fc6489b5283b782f570f2703809d23668f3ebe5ba6
|
Pushed a commit to fix the test failures. Let's see if this is all. |
The test is supposed to create an invalid sign byte. Before this PR, the generated sign byte could in fact be valid due to an overflow. Co-authored-by: Jonas Nick <[email protected]>
real-or-random
force-pushed
the
temp-merge-1386
branch
from
00a5ccd
to
82777bb
Compare
|
force-pushed to trigger a rebuild... |
delta1
added a commit
to delta1/elements
that referenced
this pull request
Aug 23, 2023
delta1
added a commit
to delta1/elements
that referenced
this pull request
Aug 23, 2023
psgreco
pushed a commit
to psgreco/elements
that referenced
this pull request
Aug 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[bitcoin-core/secp256k1#1357]: tests: refactor: take use of
secp256k1_ge_x_on_curve_var[bitcoin-core/secp256k1#1345]: field: Static-assert that int args affecting magnitude are constant
[bitcoin-core/secp256k1#1358]: tests: introduce helper for non-zero
random_fe_test()results[bitcoin-core/secp256k1#1368]: ci: Drop manual checkout of merge commit
[bitcoin-core/secp256k1#1369]: ci: Print commit in Windows container
[bitcoin-core/secp256k1#1323]: tweak_add: fix API doc for tweak=0
[bitcoin-core/secp256k1#1364]: Avoid
-Wmaybe-uninitializedwhen compiling withgcc -O1[bitcoin-core/secp256k1#1370]: Corrected some typos
[bitcoin-core/secp256k1#1367]: build: Improvements to symbol visibility logic on Windows (attempt 3)
[bitcoin-core/secp256k1#1344]: group: save normalize_weak calls in
secp256k1_ge_is_valid_var/secp256k1_gej_eq_x_var[bitcoin-core/secp256k1#1371]: Add exhaustive tests for ellswift (with create+decode roundtrip)
[bitcoin-core/secp256k1#1363]: doc: minor ellswift.md updates
[bitcoin-core/secp256k1#1340]: clean up in-comment Sage code (refer to secp256k1_params.sage, update to Python3)
[bitcoin-core/secp256k1#1366]: field: Use
restrictconsistently in fe_sqrt[bitcoin-core/secp256k1#1313]: ci: Test on development snapshots of GCC and Clang
[bitcoin-core/secp256k1#1378]: ellswift: fix probabilistic test failure when swapping sides
[bitcoin-core/secp256k1#1298]: Remove randomness tests
[bitcoin-core/secp256k1#1382]: refactor: Drop unused cast
[bitcoin-core/secp256k1#1383]: util: remove unused checked_realloc
[bitcoin-core/secp256k1#1384]: build: enable ellswift module via SECP_CONFIG_DEFINES
[bitcoin-core/secp256k1#1386]: ci: print $ELLSWIFT in cirrus.sh
This PR can be recreated with
./contrib/sync-upstream.sh -b sync-upstream range 26392da2fb7153addae0bd604495eb96f995d76c.Tip: Use
git show --remerge-diffto show the changes manually added to the merge commit.