Abstract out and merge all the magnitude/normalized logic#1066
Merged
Conversation
jonasnick
reviewed
Jan 30, 2022
jonasnick
left a comment
Contributor
There was a problem hiding this comment.
PR looks good to me otherwise. Net negative diff 🎉
robot-dreams
left a comment
Contributor
There was a problem hiding this comment.
Concept ACK
The process you used (rename fe_ to fe_impl, move verification into new fe_ functions in field_impl.h) makes sense.
I don't see any other relevant #ifdef VERIFY in field_10x26_impl.h or field_5x52_impl.h so it seems like you covered everything.
I just have a few clarifying questions, and then I want to do one final pass (maybe there's (1) some easy semi-automated way to check/reproduce the changes or (2) some more macros that can make field_impl.h shorter).
cda9763 to
8df6f75
Compare
Contributor
Author
|
Made some significant changes here, and addressed some of the comments. |
robot-dreams
reviewed
Feb 1, 2022
robot-dreams
left a comment
Contributor
There was a problem hiding this comment.
New approach makes a lot of sense and commits are very well structured. Looks good overall, I just have a few cosmetic comments (feel free not to apply all of them).
| VERIFY_CHECK(a*r->magnitude <= 32); | ||
| secp256k1_fe_impl_mul_int(r, a); | ||
| r->magnitude *= a; | ||
| r->normalized = 0; |
Contributor
There was a problem hiding this comment.
(Not this PR) Should normalized only be cleared if a > 1?
Contributor
Author
There was a problem hiding this comment.
I'd instead rather add an VERIFY_CHECK(a > 1), because using this function with a=0 or a=1 is dumb.
Contributor
There was a problem hiding this comment.
I think it's fine. Making normalization depend on the value may be a step into the wrong direction (it makes magnitude less static, though the int argument probably doesn't matter.)
And I don't see a good reason to VERIFY_CHECK the input. a may be determined at run time. For example, we currently call this with 0 (determined at run time) in the tests. If you want to add the VERIFY_CHECK, you'd need to adjust random_field_element_magnitude.
Contributor
|
Concept ACK But is there you reason why you prefer to do this before #1001? (@peterdettman and I argued to in #1060 that it may make sense to solve #1001.) I haven't looked at the details here but I think this will non-trivially conflict with #1062. @sipa can you have a look and suggest how we should move forward? |
0af77b5 to
08a9875
Compare
Contributor
Author
The impetus for this is really that I want to bring #967 up-to-date and more production-ready, and doing it properly I think would be very unclear without a change like this (having details from the "old" fields leak into the verify checks of the new one etc). I'll comment in #1060, but I think this PR makes it at least obvious in what places norm/mag are not compile-time constants.
It will. That's one of the reasons why I kept the changes for each function in separate commits. So I'm happy to rebase this after #1062, or otherwise incorporate the commits here. |
This was referenced Feb 1, 2022
08a9875 to
029e781
Compare
Contributor
Author
|
Rebased. |
029e781 to
03cfd45
Compare
03cfd45 to
9cae2d8
Compare
Contributor
Author
|
Rebased, adding an abstraction for |
9cae2d8 to
d86adb1
Compare
Contributor
Author
|
Rebased after merge of #1178. |
d86adb1 to
2e26c31
Compare
Contributor
Author
627957a to
dd5ca02
Compare
Also split secp256k1_fe_verify into a generic and an implementation specific part.
jonasnick
reviewed
May 11, 2023
jonasnick
reviewed
May 11, 2023
Fabcien
pushed a commit
to Bitcoin-ABC/secp256k1
that referenced
this pull request
Apr 20, 2026
Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@7d7d43c Depends on D19866 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19867
Fabcien
pushed a commit
to Bitcoin-ABC/secp256k1
that referenced
this pull request
Apr 20, 2026
…get_b32, fe_negate Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@ce4d209 bitcoin-core/secp256k1@f7a7666 bitcoin-core/secp256k1@1446708 bitcoin-core/secp256k1@65d82a3 Depends on D19867 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19868
Fabcien
pushed a commit
to Bitcoin-ABC/secp256k1
that referenced
this pull request
Apr 20, 2026
… fe_sqr Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@7e7ad7f bitcoin-core/secp256k1@e179e65 bitcoin-core/secp256k1@4c25f6e bitcoin-core/secp256k1@6ab3508 Depends on D19868 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19869
Fabcien
pushed a commit
to Bitcoin-ABC/secp256k1
that referenced
this pull request
Apr 20, 2026
Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@be82bd8 Depends on D19869 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19870
Fabcien
pushed a commit
to Bitcoin-ABC/secp256k1
that referenced
this pull request
Apr 20, 2026
…from_storage, fe_inv{,_var}
Summary:
This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]]
bitcoin-core/secp256k1@1e6894b
bitcoin-core/secp256k1@76d31e5
bitcoin-core/secp256k1@3167646
bitcoin-core/secp256k1@d5aa2f0
Depends on D19870
Test Plan: `ninja check-secp256k1`
Reviewers: #bitcoin_abc, Fabien
Reviewed By: #bitcoin_abc, Fabien
Differential Revision: https://reviews.bitcoinabc.org/D19871
Fabcien
pushed a commit
to Bitcoin-ABC/secp256k1
that referenced
this pull request
Apr 20, 2026
…add_int, fe_is_square_var Summary: This is a backport of [[bitcoin-core/secp256k1#1334 | secp256k1#1334]] and a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@283cd80 bitcoin-core/secp256k1@89e324c bitcoin-core/secp256k1@4371f98 & [[bitcoin-core/secp256k1#1334 | secp256k1#1334]] (fix a comment) bitcoin-core/secp256k1@4e176ad Depends on D19871 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19872
Fabcien
pushed a commit
to Bitcoin-ABC/secp256k1
that referenced
this pull request
Apr 20, 2026
Summary: This concludes backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@7fc642f Depends on D19872 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19873
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 20, 2026
Summary: Also split secp256k1_fe_verify into a generic and an implementation specific part. This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@b29566c Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D19856
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 20, 2026
Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@7fa5195 Depends on D19856 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19857
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 20, 2026
…weak, fe_normalize_var, fe_normalizes_to_zero{,_var}
Summary:
> Right now, all the logic for propagating/computing the magnitude/normalized fields in secp256k1_fe (when VERIFY is defined) and the code for checking it, is duplicated across the two field implementations. I believe that is undesirable, as these properties should purely be a function of the performed fe_ functions, and not of the choice of field implementation. This becomes even uglier with #967, which would copy all that, and even needs an additional dimension that would then need to be added to the two other fields.
This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]]
bitcoin-core/secp256k1@b6b6f9c
bitcoin-core/secp256k1@6c31371
bitcoin-core/secp256k1@6c31371
bitcoin-core/secp256k1@864f9db
Depends on D19857
Test Plan: `ninja check-secp256k1`
Reviewers: #bitcoin_abc, Fabien
Reviewed By: #bitcoin_abc, Fabien
Differential Revision: https://reviews.bitcoinabc.org/D19865
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 20, 2026
…_zero, fe_is_odd Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@19a2bfe bitcoin-core/secp256k1@c701d9a bitcoin-core/secp256k1@d3f3fe8 bitcoin-core/secp256k1@c5e788d Depends on D19865 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19866
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 20, 2026
Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@7d7d43c Depends on D19866 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19867
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 20, 2026
…get_b32, fe_negate Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@ce4d209 bitcoin-core/secp256k1@f7a7666 bitcoin-core/secp256k1@1446708 bitcoin-core/secp256k1@65d82a3 Depends on D19867 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19868
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 20, 2026
… fe_sqr Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@7e7ad7f bitcoin-core/secp256k1@e179e65 bitcoin-core/secp256k1@4c25f6e bitcoin-core/secp256k1@6ab3508 Depends on D19868 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19869
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 20, 2026
Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@be82bd8 Depends on D19869 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19870
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 20, 2026
…from_storage, fe_inv{,_var}
Summary:
This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]]
bitcoin-core/secp256k1@1e6894b
bitcoin-core/secp256k1@76d31e5
bitcoin-core/secp256k1@3167646
bitcoin-core/secp256k1@d5aa2f0
Depends on D19870
Test Plan: `ninja check-secp256k1`
Reviewers: #bitcoin_abc, Fabien
Reviewed By: #bitcoin_abc, Fabien
Differential Revision: https://reviews.bitcoinabc.org/D19871
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 20, 2026
…add_int, fe_is_square_var Summary: This is a backport of [[bitcoin-core/secp256k1#1334 | secp256k1#1334]] and a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@283cd80 bitcoin-core/secp256k1@89e324c bitcoin-core/secp256k1@4371f98 & [[bitcoin-core/secp256k1#1334 | secp256k1#1334]] (fix a comment) bitcoin-core/secp256k1@4e176ad Depends on D19871 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19872
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 20, 2026
Summary: This concludes backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@7fc642f Depends on D19872 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19873
TyphooN-
added a commit
to TyphooN-/junocash
that referenced
this pull request
May 24, 2026
705ce7ed8c Merge bitcoin-core/secp256k1#1129: ElligatorSwift + integrated x-only DH 0702ecb061 Merge bitcoin-core/secp256k1#1338: Drop no longer needed `#include "../include/secp256k1.h"` 90e360acc2 Add doc/ellswift.md with ElligatorSwift explanation 4f091847c2 Add ellswift testing to CI 1bcea8c57f Add benchmarks for ellswift module 2d1d41acf8 Add ctime tests for ellswift module df633cdeba Add _prefix and _bip324 ellswift_xdh hash functions 9695deb351 Add tests for ellswift module c47917bbd6 Add ellswift module implementing ElligatorSwift 79e5b2a8b8 Add functions to test if X coordinate is valid a597a5a9ce Add benchmark for key generation 30574f22ea Merge bitcoin-core/secp256k1#1349: Normalize ge produced from secp256k1_pubkey_load 45c5ca7675 Merge bitcoin-core/secp256k1#1350: scalar: introduce and use `secp256k1_{read,write}_be64` helpers f1652528be Normalize ge produced from secp256k1_pubkey_load 7067ee54b4 tests: add tests for `secp256k1_{read,write}_be64` 740528caad scalar: use newly introduced `secp256k1_{read,write}_be64` helpers (4x64 impl.) 67214f5f7d Merge bitcoin-core/secp256k1#1339: scalar: refactor: use `secp256k1_{read,write}_be32` helpers cb1a59275c Merge bitcoin-core/secp256k1#1341: docs: correct `pubkey` param descriptions for `secp256k1_keypair_{xonly_,}pub` f3644287b1 docs: correct `pubkey` param descriptions for `secp256k1_keypair_{xonly_,}pub` 887183e7de scalar: use `secp256k1_{read,write}_be32` helpers (4x64 impl.) 52b84238de scalar: use `secp256k1_{read,write}_be32` helpers (8x32 impl.) e449af6872 Drop no longer needed `#include "../include/secp256k1.h"` 60556c9f49 Merge bitcoin-core/secp256k1#1337: ci: Fix error D8037 in `cl.exe` (attempt 2) db29bf220c ci: Remove quirk that runs dummy command after wineserver c7db4942b3 ci: Fix error D8037 in `cl.exe` 7dae115861 Revert "ci: Move wine prefix to /tmp to avoid error D8037 in cl.exe" bf29f8d0a6 Merge bitcoin-core/secp256k1#1334: fix input range comment for `secp256k1_fe_add_int` 605e07e365 fix input range comment for `secp256k1_fe_add_int` debf3e5c08 Merge bitcoin-core/secp256k1#1330: refactor: take use of `secp256k1_scalar_{zero,one}` constants d75dc59b58 Merge bitcoin-core/secp256k1#1333: test: Warn if both `VERIFY` and `COVERAGE` are defined ade5b36701 tests: add checks for scalar constants `secp256k1_scalar_{zero,one}` e83801f5db test: Warn if both `VERIFY` and `COVERAGE` are defined 654246c635 refactor: take use of `secp256k1_scalar_{zero,one}` constants 908e02d596 Merge bitcoin-core/secp256k1#1328: build: Bump MSVC warning level up to W3 1549db0ca5 build: Level up MSVC warnings 20a5da5fb1 Merge bitcoin-core/secp256k1#1310: Refine release process ad84603297 release process: clarify change log updates 6348bc7eee release process: fix process for maintenance release 79fa50b082 release process: mention targeted release schedule 165206789b release process: add sanity checks 09df0bfb23 Merge bitcoin-core/secp256k1#1327: ci: Move wine prefix to /tmp to avoid error D8037 in cl.exe 27504d5c94 ci: Move wine prefix to /tmp to avoid error D8037 in cl.exe d373a7215b Merge bitcoin-core/secp256k1#1316: Do not invoke fe_is_zero on failed set_b32_limit 6433175ffe Do not invoke fe_is_zero on failed set_b32_limit 5f7903c73c Merge bitcoin-core/secp256k1#1318: build: Enable -DVERIFY for precomputation binaries e9e4526a4e Merge bitcoin-core/secp256k1#1317: Make fe_cmov take max of magnitudes 5768b50229 build: Enable -DVERIFY for precomputation binaries 31b4bbee1e Make fe_cmov take max of magnitudes 83186db34a Merge bitcoin-core/secp256k1#1314: release cleanup: bump version after 0.3.2 95448ef2f8 release cleanup: bump version after 0.3.2 acf5c55ae6 Merge bitcoin-core/secp256k1#1312: release: Prepare for 0.3.2 d490ca2046 release: Prepare for 0.3.2 3e3d125b83 Merge bitcoin-core/secp256k1#1309: changelog: Catch up e8295d07ab Merge bitcoin-core/secp256k1#1311: Revert "Remove unused scratch space from API" 697e1ccf4a changelog: Catch up 3ad1027a40 Revert "Remove unused scratch space from API" 76b43f3443 changelog: Add entry for #1303 7d4f86d242 Merge bitcoin-core/secp256k1#1307: Mark more assembly outputs as early clobber b54a0672ef Merge bitcoin-core/secp256k1#1304: build: Rename arm to arm32 and check if it's really supported c6bb29b303 build: Rename `64bit` to `x86_64` 8c9ae37a5a Add release note 03246457a8 autotools: Add `SECP_ARM32_ASM_CHECK` macro ed4ba238e2 cmake: Add `check_arm32_assembly` function 350b4bd6e6 Mark stack variables as early clobber for technical correctness 0c729ba70d Bugfix: mark outputs as early clobber in scalar x86_64 asm 3353d3c753 Merge bitcoin-core/secp256k1#1207: Split fe_set_b32 into reducing and normalizing variants 5b32602295 Split fe_set_b32 into reducing and normalizing variants 006ddc1f42 Merge bitcoin-core/secp256k1#1306: build: Make tests work with external default callbacks 1907f0f166 build: Make tests work with external default callbacks fb3a806365 Merge bitcoin-core/secp256k1#1133: schnorrsig: Add test vectors for variable-length messages cd54ac7c1c schnorrsig: Improve docs of schnorrsig_sign_custom 28687b0312 schnorrsig: Add BIP340 varlen test vectors 97a98bed1e schnorrsig: Refactor test vector code to allow varlen messages ab5a917128 Merge bitcoin-core/secp256k1#1303: ct: Use more volatile 9eb6934f69 Merge bitcoin-core/secp256k1#1305: Remove unused scratch space from API 073d98a076 Merge bitcoin-core/secp256k1#1292: refactor: Make 64-bit shift explicit 17fa21733a ct: Be cautious and use volatile trick in more "conditional" paths 5fb336f9ce ct: Use volatile trick in scalar_cond_negate 712e7f8722 Remove unused scratch space from API 54d34b6c24 Merge bitcoin-core/secp256k1#1300: Avoid normalize conditional on VERIFY c63ec88ebf Merge bitcoin-core/secp256k1#1066: Abstract out and merge all the magnitude/normalized logic 7fc642fa25 Simplify secp256k1_fe_{impl_,}verify 4e176ad5b9 Abstract out verify logic for fe_is_square_var 4371f98346 Abstract out verify logic for fe_add_int 89e324c6b9 Abstract out verify logic for fe_half 283cd80ab4 Abstract out verify logic for fe_get_bounds d5aa2f0358 Abstract out verify logic for fe_inv{,_var} 3167646072 Abstract out verify logic for fe_from_storage 76d31e5047 Abstract out verify logic for fe_to_storage 1e6894bdd7 Abstract out verify logic for fe_cmov be82bd8e03 Improve comments/checks for fe_sqrt 6ab35082ef Abstract out verify logic for fe_sqr 4c25f6efbd Abstract out verify logic for fe_mul e179e651cb Abstract out verify logic for fe_add 7e7ad7ff57 Abstract out verify logic for fe_mul_int 65d82a3445 Abstract out verify logic for fe_negate 144670893e Abstract out verify logic for fe_get_b32 f7a7666aeb Abstract out verify logic for fe_set_b32 ce4d2093e8 Abstract out verify logic for fe_cmp_var 7d7d43c6dd Improve comments/check for fe_equal{,_var} c5e788d672 Abstract out verify logic for fe_is_odd d3f3fe8616 Abstract out verify logic for fe_is_zero c701d9a471 Abstract out verify logic for fe_clear 19a2bfeeea Abstract out verify logic for fe_set_int 864f9db491 Abstract out verify logic for fe_normalizes_to_zero{,_var} 6c31371120 Abstract out verify logic for fe_normalize_var e28b51f522 Abstract out verify logic for fe_normalize_weak b6b6f9cb97 Abstract out verify logic for fe_normalize 7fa5195559 Bugfix: correct SECP256K1_FE_CONST mag/norm fields e5cf4bf3ff build: Rename `arm` to `arm32` b29566c51b Merge magnitude/normalized fields, move/improve comments 97c63b9039 Avoid normalize conditional on VERIFY 341cc19726 Merge bitcoin-core/secp256k1#1299: Infinity handling: ecmult_const(infinity) works, and group verification bbc834467c Avoid secp256k1_ge_set_gej_zinv with uninitialized z 0a2e0b2ae4 Make secp256k1_{fe,ge,gej}_verify work as no-op if non-VERIFY f20266722a Add invariant checking to group elements a18821d5b1 Always initialize output coordinates in secp256k1_ge_set_gej 3086cb90ac Expose secp256k1_fe_verify to other modules a0e696fd4d Make secp256k1_ecmult_const handle infinity 24c768ae09 Merge bitcoin-core/secp256k1#1301: Avoid using bench_verify_data as bench_sign_data; merge them 2e65f1fdbc Avoid using bench_verify_data as bench_sign_data; merge them 1cf15ebd94 Merge bitcoin-core/secp256k1#1296: docs: complete interface description for `secp256k1_schnorrsig_sign_custom` 149c41cee1 docs: complete interface description for `secp256k1_schnorrsig_sign_custom` f30c74866b Merge bitcoin-core/secp256k1#1270: cmake: Fix library ABI versioning d1e48e5474 refactor: Make 64-bit shift explicit b2e29e43d0 ci: Treat all compiler warnings as errors in "Windows (VS 2022)" task 3c81838856 Merge bitcoin-core/secp256k1#1289: cmake: Use full signature of `add_test()` command 755629bc03 cmake: Use full signature of `add_test()` command bef448f9af cmake: Fix library ABI versioning 4b0f711d46 Merge bitcoin-core/secp256k1#1277: autotools: Clean up after adding Wycheproof 222ecaf661 Merge bitcoin-core/secp256k1#1284: cmake: Some improvements using `PROJECT_IS_TOP_LEVEL` variable 71f746c057 cmake: Include `include` directory for subtree builds 024a409484 Merge bitcoin-core/secp256k1#1240: cmake: Improve and document compiler flag checks a8d059f76c cmake, doc: Document compiler flags 6ece1507cb cmake, refactor: Rename `try_add_compile_option` to `try_append_cflags` 19516ed3e9 cmake: Use `add_compile_options()` in `try_add_compile_option()` 4b84f4bf0f Merge bitcoin-core/secp256k1#1239: cmake: Bugfix and other improvements after bumping CMake up to 3.13 596b336ff6 Merge bitcoin-core/secp256k1#1234: cmake: Add dev-mode 6b7e5b717d Merge bitcoin-core/secp256k1#1275: build: Fix C4005 "macro redefinition" MSVC warnings in examples 1c89536718 Merge bitcoin-core/secp256k1#1286: tests: remove extra semicolon in macro c4062d6b5d debug: move helper for printing buffers into util.h 7e977b3c50 autotools: Take VPATH builds into account when generating testvectors 2418d3260a autotools: Create src/wycheproof dir before creating file in it 8764034ed5 autotools: Make all "pregenerated" targets .PHONY e1b9ce8811 autotools: Use same conventions for all pregenerated files 3858bad2c6 tests: remove extra semicolon in macro 1f33bb2b1c Merge bitcoin-core/secp256k1#1205: field: Improve docs +tests of secp256k1_fe_set_b32 162da73e9a tests: Add debug helper for printing buffers e9fd3dff76 field: Improve docs and tests of secp256k1_fe_set_b32 f6bef03c0a Merge bitcoin-core/secp256k1#1283: Get rid of secp256k1_fe_const_b 5431b9decd cmake: Make `SECP256K1_INSTALL` default depend on `PROJECT_IS_TOP_LEVEL` 5ec1333d4f Merge bitcoin-core/secp256k1#1285: bench: Make sys/time.h a system include 68b16a1662 bench: Make sys/time.h a system include 162608cc98 cmake: Emulate `PROJECT_IS_TOP_LEVEL` for CMake<3.21 69e1ec0331 Get rid of secp256k1_fe_const_b ce5ba9e24d gitignore: Add CMakeUserPresets.json 0a446a312f cmake: Add dev-mode CMake preset a6f4bcf6e1 Merge bitcoin-core/secp256k1#1231: Move `SECP256K1_INLINE` macro definition out from `include/secp256k1.h` a273d74b2e cmake: Improve version comparison 6a58b483ef cmake: Use `if(... IN_LIST ...)` command 2445808c02 cmake: Use dedicated `GENERATOR_IS_MULTI_CONFIG` property 9f8703ef17 cmake: Use dedicated `CMAKE_HOST_APPLE` variable 8c2017035a cmake: Use recommended `add_compile_definitions` command 04d4cc071a cmake: Add `DESCRIPTION` and `HOMEPAGE_URL` options to `project` command 8a8b6536ef cmake: Use `SameMinorVersion` compatibility mode 5b0444a3b5 Merge bitcoin-core/secp256k1#1263: cmake: Make installation optional 47ac3d63cd cmake: Make installation optional 2e035af251 Merge bitcoin-core/secp256k1#1273: build: Make `SECP_VALGRIND_CHECK` preserve `CPPFLAGS` 5be353d658 Merge bitcoin-core/secp256k1#1279: tests: lint wycheproof's python script 08f4b1632d autotools: Move code around to tidy Makefile 04bf3f6778 Merge bitcoin-core/secp256k1#1230: Build: allow static or shared but not both 9ce9984f32 Merge bitcoin-core/secp256k1#1265: Remove bits argument from secp256k1_wnaf_const{_xonly} 566faa17d3 Merge bitcoin-core/secp256k1#1267: doc: clarify process for patch releases ef49a11d29 build: allow static or shared but not both 35ada3b954 tests: lint wycheproof's python script 529b54d922 autotools: Move Wycheproof header from EXTRA_DIST to noinst_HEADERS dc0657c762 build: Fix C4005 "macro redefinition" MSVC warnings in examples 1ecb94ebe9 build: Make `SECP_VALGRIND_CHECK` preserve `CPPFLAGS` 1b6fb5593c doc: clarify process for patch releases a575339c02 Remove bits argument from secp256k1_wnaf_const (always 256) 36b0adf1b9 build: remove warning until it's reproducible 8e142ca410 Move `SECP256K1_INLINE` macro definition out from `include/secp256k1.h` 77445898a5 Remove `SECP256K1_INLINE` usage from examples ca92a35d01 field: Simplify code in secp256k1_fe_set_b32 d93f62e369 field: Verify field element even after secp256k1_fe_set_b32 fails git-subtree-dir: src/secp256k1 git-subtree-split: 705ce7ed8c1557a31e1bfc99be06082c5098d9f5
roqqit
pushed a commit
to doged-io/doged
that referenced
this pull request
Jun 11, 2026
Summary: Also split secp256k1_fe_verify into a generic and an implementation specific part. This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@b29566c Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D19856
roqqit
pushed a commit
to doged-io/doged
that referenced
this pull request
Jun 11, 2026
Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@7fa5195 Depends on D19856 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19857
roqqit
pushed a commit
to doged-io/doged
that referenced
this pull request
Jun 11, 2026
…weak, fe_normalize_var, fe_normalizes_to_zero{,_var}
Summary:
> Right now, all the logic for propagating/computing the magnitude/normalized fields in secp256k1_fe (when VERIFY is defined) and the code for checking it, is duplicated across the two field implementations. I believe that is undesirable, as these properties should purely be a function of the performed fe_ functions, and not of the choice of field implementation. This becomes even uglier with #967, which would copy all that, and even needs an additional dimension that would then need to be added to the two other fields.
This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]]
bitcoin-core/secp256k1@b6b6f9c
bitcoin-core/secp256k1@6c31371
bitcoin-core/secp256k1@6c31371
bitcoin-core/secp256k1@864f9db
Depends on D19857
Test Plan: `ninja check-secp256k1`
Reviewers: #bitcoin_abc, Fabien
Reviewed By: #bitcoin_abc, Fabien
Differential Revision: https://reviews.bitcoinabc.org/D19865
roqqit
pushed a commit
to doged-io/doged
that referenced
this pull request
Jun 11, 2026
…_zero, fe_is_odd Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@19a2bfe bitcoin-core/secp256k1@c701d9a bitcoin-core/secp256k1@d3f3fe8 bitcoin-core/secp256k1@c5e788d Depends on D19865 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19866
roqqit
pushed a commit
to doged-io/doged
that referenced
this pull request
Jun 11, 2026
Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@7d7d43c Depends on D19866 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19867
roqqit
pushed a commit
to doged-io/doged
that referenced
this pull request
Jun 11, 2026
…get_b32, fe_negate Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@ce4d209 bitcoin-core/secp256k1@f7a7666 bitcoin-core/secp256k1@1446708 bitcoin-core/secp256k1@65d82a3 Depends on D19867 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19868
roqqit
pushed a commit
to doged-io/doged
that referenced
this pull request
Jun 11, 2026
… fe_sqr Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@7e7ad7f bitcoin-core/secp256k1@e179e65 bitcoin-core/secp256k1@4c25f6e bitcoin-core/secp256k1@6ab3508 Depends on D19868 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19869
roqqit
pushed a commit
to doged-io/doged
that referenced
this pull request
Jun 11, 2026
Summary: This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@be82bd8 Depends on D19869 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19870
roqqit
pushed a commit
to doged-io/doged
that referenced
this pull request
Jun 11, 2026
…from_storage, fe_inv{,_var}
Summary:
This is a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]]
bitcoin-core/secp256k1@1e6894b
bitcoin-core/secp256k1@76d31e5
bitcoin-core/secp256k1@3167646
bitcoin-core/secp256k1@d5aa2f0
Depends on D19870
Test Plan: `ninja check-secp256k1`
Reviewers: #bitcoin_abc, Fabien
Reviewed By: #bitcoin_abc, Fabien
Differential Revision: https://reviews.bitcoinabc.org/D19871
roqqit
pushed a commit
to doged-io/doged
that referenced
this pull request
Jun 11, 2026
…add_int, fe_is_square_var Summary: This is a backport of [[bitcoin-core/secp256k1#1334 | secp256k1#1334]] and a partial backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@283cd80 bitcoin-core/secp256k1@89e324c bitcoin-core/secp256k1@4371f98 & [[bitcoin-core/secp256k1#1334 | secp256k1#1334]] (fix a comment) bitcoin-core/secp256k1@4e176ad Depends on D19871 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19872
roqqit
pushed a commit
to doged-io/doged
that referenced
this pull request
Jun 11, 2026
Summary: This concludes backport of [[bitcoin-core/secp256k1#1066 | secp256k1#1066]] bitcoin-core/secp256k1@7fc642f Depends on D19872 Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D19873
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Right now, all the logic for propagating/computing the magnitude/normalized fields in
secp256k1_fe(whenVERIFYis defined) and the code for checking it, is duplicated across the two field implementations. I believe that is undesirable, as these properties should purely be a function of the performed fe_ functions, and not of the choice of field implementation. This becomes even uglier with #967, which would copy all that, and even needs an additional dimension that would then need to be added to the two other fields. It's also related to #1001, which I think will become easier if it doesn't need to be done/reasoned about separately for every field.This PR moves all logic around these fields (collectively called field verification) to implementations in field_impl.h, which dispatch to renamed functions in field_*_impl.h for the actual implementation.
Fixes #1060.