Upstream PRs 1391, 1290, 1389, 1397, 1399, 1400, 1348, 1402, 1274, 1394, 1404, 1062, 1401, 1373, 1403, 1398, 1405, 1396, 1406, 1410, 1409, 1411, 1412, 1414, 1413, 1415, 1417, 1390, 1416, 1422, 1424, 1395 #270
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
This change aims to simplify the following commit.
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.
The group element checks `secp256k1_{ge,gej}_verify` have first been
implemented and added in commit f202667
(PR #1299). This commit adds additional verification calls in group
functions, to match the ones that were originally proposed in commit
09dbba561fdb9d57a2cc9842ce041d9ba29a6189 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 <[email protected]>
Co-authored-by: Tim Ruffing <[email protected]>
Co-authored-by: Tim Ruffing <[email protected]>
Remove also the explicit magnitude restriction `a->x.magnitude <= 31` in `secp256k1_gej_eq_x_var` (introduced in commit 07c0e8b), as this is implied by the new limits. Co-authored-by: Sebastian Falbesoner <[email protected]>
…stive test - `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
secp256k1_scalar_verify checks that scalars are reduced mod the group order
- adjust test methods that randomize magnitudes Co-authored-by: Sebastian Falbesoner <[email protected]> Co-authored-by: Jonas Nick <[email protected]>
Also update the operations count comments in each of the affected functions accordingly and remove a redundant VERIFY_CHECK in secp256k1_gej_add_ge (the infinity value range check [0,1] is already covered by secp256k1_gej_verify above). Co-authored-by: Sebastian Falbesoner <[email protected]> Co-authored-by: Tim Ruffing <[email protected]> Co-authored-by: Jonas Nick <[email protected]>
…calar_{zero,one}` constants (part 2)
a1bd497 refactor: take use of `secp256k1_scalar_{zero,one}` constants (part 2) (Sebastian Falbesoner)
Pull request description:
ACKs for top commit:
real-or-random:
utACK bitcoin-core/secp256k1@a1bd497
jonasnick:
ACK a1bd497
Tree-SHA512: 09ef6d9be1d3f9c19f8fe4614fe629de5c45197027e0e3f9dd8d4679a510a7b57f8aa499707a6daf652041f255c87316c9883bf7cf9a08bd41a3651bff54299e
…for examples on Windows 175db31 ci: Drop no longer needed `PATH` variable update on Windows (Hennadii Stepanov) 116d2ab cmake: Set `ENVIRONMENT` property for examples on Windows (Hennadii Stepanov) cef3739 cmake, refactor: Use helper function instead of interface library (Hennadii Stepanov) Pull request description: This PR simplifies running examples on Windows, because the DLL must reside either in the same folder where the executable is or somewhere in PATH. It is an alternative to #1233. ACKs for top commit: real-or-random: utACK 175db31 Tree-SHA512: 8188018589a5bcf0179647a039cdafcce661dc103a70a5bb9e6b6f680b899332ba30b1e9ef5dad2a8c22c315d7794747e49d8cf2e391eebea21e3d8505ee334b
… GitHub Actions a2f7ccd ci: Run "Windows (VS 2022)" job on GitHub Actions (Hennadii Stepanov) Pull request description: This PR solves one item in bitcoin-core/secp256k1#1392. In response to upcoming [limiting free usage of Cirrus CI](https://cirrus-ci.org/blog/2023/07/17/limiting-free-usage-of-cirrus-ci/), suggesting to move (partially?) CI tasks/jobs from Cirrus CI to [GitHub Actions](https://docs.github.com/actions) (GHA). Here is example from my personal repo: https://github.com/hebasto/secp256k1/actions/runs/5806269046. For security concerns, see: - bitcoin/bitcoin#28098 (comment) - bitcoin/bitcoin#28098 (comment) I'm suggesting the repository "Actions permissions" as follows:   --- See build logs in my personal repo: https://github.com/hebasto/secp256k1/actions/runs/5692587475. ACKs for top commit: real-or-random: utACK a2f7ccd Tree-SHA512: b6329a29391146e3cdee9a56f6151b6672aa45837dfaacb708ba4209719801ed029a6928d638d314b71c7533d927d771b3eca4b9e740cfcf580a40ba07970ae4
…k from Cirrus CI f1774e5 ci, gha: Make MSVC job presentation more explicit (Hennadii Stepanov) 5ee039b ci: Remove "Windows (VS 2022)" task from Cirrus CI (Hennadii Stepanov) Pull request description: A follow-up for bitcoin-core/secp256k1#1389. bitcoin-core/secp256k1#1389 (comment): > Or actually... hebasto Can you remove the second commit for now, if we're unsure whether this works at all. --- Second commit effect: - [before (master branch)](https://github.com/bitcoin-core/secp256k1/actions/runs/5809860925):  - [after (this PR)](https://github.com/bitcoin-core/secp256k1/actions/runs/5810140851):  ACKs for top commit: real-or-random: utACK f1774e5 Tree-SHA512: ed36c5cef3ba4cf6769d480358f753ecc4a8a150103201f586b05d8d364c580ff637fe5b915918c695c8f7067c1bd7de6384eea1a12d1b8575ba5b629779ebf4
This reverts commit d8d5485.
… on GitHub Actions 8408dfd Revert "ci: Run sage prover on CI" (Hennadii Stepanov) c8d9914 ci, gha: Run "SageMath prover" job on GitHub Actions (Hennadii Stepanov) Pull request description: ACKs for top commit: real-or-random: utACK 8408dfd jonasnick: ACK 8408dfd Tree-SHA512: 4de628b6d5535023c5351faebfd98d2bd9effe6592f14ffe0d0f7c6eeedd7426b9891da70aa3ea7fa830f0abc054f6b015af01fb6e26f50d45eb26177a7a6310
…KMEM macros also for ellswift 9c91ea4 ci: Enable ellswift module where it's missing (Tim Ruffing) db32a24 ctimetests: Use new SECP256K1_CHECKMEM macros also for ellswift (Tim Ruffing) Pull request description: ACKs for top commit: hebasto: ACK 9c91ea4. jonasnick: ACK 9c91ea4 Tree-SHA512: e918236cb38b2bb6e69f84fcfa5f550c54f0df018103627082646a8fd731c238ce68b1b85badf042f08300208015012677143a96f9b97d94065b9a00c1da7876
…ve normalize_weak calls in group add methods (revival of #1032) b7c685e Save _normalize_weak calls in group add methods (Peter Dettman) c83afa6 Tighten group magnitude limits (Peter Dettman) 173e8d0 Implement current magnitude assumptions (Peter Dettman) 49afd2f Take use of _fe_verify_magnitude in field_impl.h (Sebastian Falbesoner) 4e9661f Add _fe_verify_magnitude (no-op unless VERIFY is enabled) (Peter Dettman) 690b0fc add missing group element invariant checks (Sebastian Falbesoner) Pull request description: This PR picks up #1032 by peterdettman. It's essentially a rebase on master; the original first commit (09dbba561fdb9d57a2cc9842ce041d9ba29a6189) which introduced group verification methods has mostly been replaced by PR #1299 (commit f202667) and what remains now is only adding a few missing checks at some places. The remaining commits are unchanged, though some (easy-to-solve) conflicts appeared through cherry-picking. The last commit which actually removes the `normalize_weak` calls is obviously the critical one and needs the most attention for review. ACKs for top commit: sipa: utACK b7c685e real-or-random: ACK b7c685e jonasnick: ACK b7c685e Tree-SHA512: f15167eff7ef6ed971c726a4d738de9a15be95b0c947d7e38329e7b16656202b7113497d36625304e784866349f2293f6f1d8cb97df35393af9ea465a4156da3
It is not neccessary for the second argument in `secp256k1_fe_equal_var` (or `secp256k1_fe_equal`) to have magnitude = 1. Hence, removed the `secp256k1_fe_normalize_weak` call for those argument.
`fe_equal_var` hits a fast path only when the inputs are unequal, which is uncommon among its callers (public key parsing, ECDSA verify).
This commit also explicitly initializes shortpubkey. For some reason, removing surrounding, unrelated lines results in gcc warnings when configured with --enable-ctime-tests=no --with-valgrind=no.
1633980 release: Prepare for 0.4.0 (Tim Ruffing) d9a8506 changelog: Catch up in preparation of release (Tim Ruffing) Pull request description: ACKs for top commit: hebasto: re-ACK 1633980. sipa: ACK 1633980 jonasnick: ACK 1633980 Tree-SHA512: 9b29edc8beece44cb8456de9844bf22e13f41b43bb5567b3f37dcbdcb7cd5ca6a976a0f805973ddfa7666509aa452247a4d8297e3cfb362acaf4f0fa942daa21
…r 0.4.0 9b118bc release cleanup: bump version after 0.4.0 (Jonas Nick) Pull request description: based on #1415 ACKs for top commit: sipa: ACK 9b118bc hebasto: ACK 9b118bc real-or-random: ACK 9b118bc Tree-SHA512: 76df87c41bdc3379df4e88619645f5110010d7713ebe20bad3e7c99472bd62b90f4bd3c6b558ad5a23119acc4734e39383d96a9800e4a43dfadc086ef66fd0ab
…llbacks with CHECK_ILLEGAL_VOID 7030364 tests: add CHECK_ERROR_VOID and use it in scratch tests (Jonas Nick) f8d7ea6 tests: Replace counting_illegal_callbacks with CHECK_ILLEGAL_VOID (Jonas Nick) a1d52e3 tests: remove unnecessary test in run_ec_pubkey_parse_test (Jonas Nick) 875b0ad tests: remove unnecessary set_illegal_callback (Jonas Nick) Pull request description: Fixes #1167 ACKs for top commit: siv2r: reACK 7030364 (tests pass locally) real-or-random: reACK 7030364 Tree-SHA512: 0ca1f1c92a1c3a93b412433e53e882be56f3c7c55d4cbf12683ab7d9b8a916231b6508270099bfed0bfaa9d0af19cb8fdf0fe3274112ab48d33a0bd2356f2fa7
… CI ones b0f7bfe doc: Do not mention soname in CHANGELOG.md "ABI Compatibility" section (Hennadii Stepanov) bd9d98d doc: Align documented scripts with CI ones (Hennadii Stepanov) Pull request description: ACKs for top commit: sipa: ACK b0f7bfe real-or-random: ACK b0f7bfe Tree-SHA512: 99cbc065cf9610923a863bac34e607ce4f2b1fe71fc32cb96fed33203e42c914ef29924cd9eade89859f63fdd95ffb214c5a2a1066bfca9c202e85aec5f7c16e
This change allows downstream projects to use pkg-config to search for the libsecp256k1 library that is built with CMake.
421d848 ci: Align Autotools/CMake `CI_INSTALL` directory names (Hennadii Stepanov) 9f005c6 cmake: Install `libsecp256k1.pc` file (Hennadii Stepanov) Pull request description: This PR allows downstream projects to use pkg-config to search for the libsecp256k1 library that is built with CMake. Addressed bitcoin-core/secp256k1#1419 (comment): > We could just ship the pkg-config file also in CMake builds. ACKs for top commit: real-or-random: ACK bitcoin-core/secp256k1@421d848 I compared the generated pc files and they match in autotools and CMake Tree-SHA512: 8e54eb7c76bc727ab18715258c06cc2a419c6c04892a2bd7bfe34392f9a3223f673ff84d2d21b00b3c222b357f02296ec49c872532d98ea0a2f17ef1ed6b6ac1
… actions d9d80fd ci: Bump major versions for docker actions (Hennadii Stepanov) Pull request description: See: - https://github.com/docker/build-push-action/releases/tag/v5.0.0 - https://github.com/docker/setup-buildx-action/releases/tag/v3.0.0 ACKs for top commit: real-or-random: ACK d9d80fd Tree-SHA512: b1266e46cd02f8e893b4ce3b4bf51f7fb2ea7c6ae54a5c24a4bc5df4f6e97e99afaf90cf598d4321e8b83a250ba5fd7d43c34d53a8cc71f70f6c6e05cc973d6f
…o` (remove loop limit and unneeded normalize) c45b7c4 refactor: introduce testutil.h (deduplicate `random_fe_`, `ge_equals_` helpers) (Sebastian Falbesoner) dc55141 tests: simplify `random_fe_non_zero` (remove loop limit and unneeded normalize) (Sebastian Falbesoner) Pull request description: `random_fe_non_zero` contains a loop iteration limit that ensures that we abort if `random_fe` ever yielded zero more than ten times in a row. This construct was first introduced in PR BlockstreamResearch#19 (commit 09ca4f3) for random non-square field elements and was later refactored into the non-zero helper in PR BlockstreamResearch#25 (commit 6d6102f). The copy-over to the exhaustive tests happened recently in PR #1118 (commit 0f86420). This case seems to be practically irrelevant and I'd argue for keeping things simple and removing it (which was already suggested in bitcoin-core/secp256k1#1118 (comment)); if there's really a worry that the test's random generator is heavily biased towards certain values or value ranges then there should consequently be checks at other places too (e.g. directly in `random_fe` for 256-bit values that repeatedly overflow, i.e. >= p). Also, the _fe_normalize call is not needed and can be removed, as the result of `random_fe` is already normalized. ACKs for top commit: real-or-random: utACK c45b7c4 siv2r: ACK `c45b7c4` (reviewed the changes and tests for both the commits passed locally). Tree-SHA512: 4ffa66dd0b8392d7d0083a71e7b0682ad18f9261fd4ce8548c3059b497d3462db97e16114fded9787661ca447a877a27f5b996bd7d47e6f91c4454079d28a8ac
jonasnick
force-pushed
the
temp-merge-1395
branch
2 times, most recently
from
4cf28de
to
c5861f3
Compare
b327abf 5d8fa82 3d05c86 bcffeb1 de657c2 060e32c 0ba2b94 48b1d93 6b9507a 5373693 2e6cf9b 6ee1455 26a9899 4d7fe60 ea26b71 65c79fe 727bec5 0b4640a 199d27c cbf3053 49be5be b10ddd2 4fd00f4 ba9cb6f ee7aaf2 ' into temp-merge-1395 - Replace fe_equal_var with fe_equal - Use CHECK_ILLEGAL instead of CHECK/ecount - Turn on secp256k1-zkp specific modules in CI
jonasnick
force-pushed
the
temp-merge-1395
branch
from
c5861f3
to
775f5e2
Compare
|
My fork shows that the branch passes GH Actions CI: https://github.com/jonasnick/secp256k1-zkp/actions |
|
|
The remaining occurances of |
|
Oh sure |
jonasnick
force-pushed
the
temp-merge-1395
branch
from
af475bb
to
e9d522f
Compare
This was referenced Oct 12, 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#1391]: refactor: take use of
secp256k1_scalar_{zero,one}constants (part 2)[bitcoin-core/secp256k1#1290]: cmake: Set
ENVIRONMENTproperty for examples on Windows[bitcoin-core/secp256k1#1389]: ci: Run "Windows (VS 2022)" job on GitHub Actions
[bitcoin-core/secp256k1#1397]: ci: Remove "Windows (VS 2022)" task from Cirrus CI
[bitcoin-core/secp256k1#1399]: ci, gha: Run "SageMath prover" job on GitHub Actions
[bitcoin-core/secp256k1#1400]: ctimetests: Use new SECP256K1_CHECKMEM macros also for ellswift
[bitcoin-core/secp256k1#1348]: tighten group magnitude limits, save normalize_weak calls in group add methods (revival of #1032)
[bitcoin-core/secp256k1#1402]: ci: Use Homebrew'''s gcc in native macOS task
[bitcoin-core/secp256k1#1274]: test: Silent noisy clang warnings about Valgrind code on macOS x86_64
[bitcoin-core/secp256k1#1394]: ci, gha: Run "x86_64: macOS Ventura" job on GitHub Actions
[bitcoin-core/secp256k1#1404]: ci: Remove "arm64: macOS Ventura" task from Cirrus CI
[bitcoin-core/secp256k1#1062]: Removes
_fe_equal_var, and unwanted_fe_normalize_weakcalls (in tests)[bitcoin-core/secp256k1#1401]: ci, gha: Run all MSVC tests on Windows natively
[bitcoin-core/secp256k1#1373]: Add invariant checking for scalars
[bitcoin-core/secp256k1#1403]: ci, gha: Ensure only a single workflow processes
github.refat a time[bitcoin-core/secp256k1#1398]: ci, gha: Add Windows jobs based on Linux image
[bitcoin-core/secp256k1#1405]: ci: Drop no longer needed workaround
[bitcoin-core/secp256k1#1396]: ci, gha: Add "x86_64: Linux (Debian stable)" GitHub Actions job
[bitcoin-core/secp256k1#1406]: ci, gha: Move more non-x86_64 tasks from Cirrus CI to GitHub Actions
[bitcoin-core/secp256k1#1410]: ci: Use concurrency for pull requests only
[bitcoin-core/secp256k1#1409]: ci: Move remained task from Cirrus to GitHub Actions
[bitcoin-core/secp256k1#1411]: ci: Make repetitive command the default one
[bitcoin-core/secp256k1#1412]: ci: Switch macOS from Ventura to Monterey and add Valgrind
[bitcoin-core/secp256k1#1414]: ci/gha: Add ARM64 QEMU jobs for clang and clang-snapshot
[bitcoin-core/secp256k1#1413]: ci: Add
releasejob[bitcoin-core/secp256k1#1415]: release: Prepare for 0.4.0
[bitcoin-core/secp256k1#1417]: release cleanup: bump version after 0.4.0
[bitcoin-core/secp256k1#1390]: tests: Replace counting_illegal_callbacks with CHECK_ILLEGAL_VOID
[bitcoin-core/secp256k1#1416]: doc: Align documented scripts with CI ones
[bitcoin-core/secp256k1#1422]: cmake: Install
libsecp256k1.pcfile[bitcoin-core/secp256k1#1424]: ci: Bump major versions for docker actions
[bitcoin-core/secp256k1#1395]: tests: simplify
random_fe_non_zero(remove loop limit and unneeded normalize)This PR can be recreated with
./contrib/sync-upstream.sh -b master range ee7aaf213ea3eb42fc8960c7d178b5ffb286440f.Tip: Use
git show --remerge-diffto show the changes manually added to the merge commit.' --web