f can never equal -m #1603
Merged
f can never equal -m #1603
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
roconnor-blockstream
force-pushed
the
f-is-not-neg-modulus_2024-09
branch
from
cb6abf7
to
e53ed2e
Compare
real-or-random
approved these changes
Sep 25, 2024
There was a problem hiding this comment.
Okay yes. All VERIFY_CHECKs are redundant in a sense, but this one is really covered by another VERIFY_CHECK already.
utACK e53ed2e
src/modinv32_impl.h
Outdated
| @@ -643,13 +643,12 @@ static void secp256k1_modinv32_var(secp256k1_modinv32_signed30 *x, const secp256 | |||
|
|
|||
| /* g == 0 */ | |||
| VERIFY_CHECK(secp256k1_modinv32_mul_cmp_30(&g, len, &SECP256K1_SIGNED30_ONE, 0) == 0); | |||
| /* |f| == 1, or (x == 0 and d == 0 and |f|=modulus) */ | |||
| /* |f| == 1, or (x == 0 and d == 0 and f=modulus) */ | |||
There was a problem hiding this comment.
micro-nit:
Suggested change
| /* |f| == 1, or (x == 0 and d == 0 and f=modulus) */ | |
| /* |f| == 1, or (x == 0 and d == 0 and f == modulus) */ |
|
Just to be clear, this PR isn't removing a redundant check, it is strengthening the existing check by removing a disjunctive clause. |
Oh, right, I got this wrong. But I still think the change makes sense. |
In fact, before reaching this particular VERIFY_CHECK, we had already successfully passed through
VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(&f, len, &modinfo->modulus, -1) > 0); /* f > -modulus */
ensuring that f is not -m.
roconnor-blockstream
force-pushed
the
f-is-not-neg-modulus_2024-09
branch
from
e53ed2e
to
ef7ff03
Compare
|
Done. I've also added the same change to the constant-time versions of these functions. |
real-or-random
approved these changes
Sep 25, 2024
There was a problem hiding this comment.
utACK ef7ff03
(There seems to be some CI hiccup unrelated to this PR.)
|
ACK ef7ff03 |
achow101
added a commit
to achow101/bitcoin
that referenced
this pull request
Oct 12, 2024
a88aa935063 Merge bitcoin-core/secp256k1#1603: f can never equal -m 3660fe5e2a9 Merge bitcoin-core/secp256k1#1479: Add module "musig" that implements MuSig2 multi-signatures (BIP 327) 168c92011f5 build: allow enabling the musig module in cmake f411841a46b Add module "musig" that implements MuSig2 multi-signatures (BIP 327) 0be79660f38 util: add constant-time is_zero_array function c8fbdb1b972 group: add ge_to_bytes_ext and ge_from_bytes_ext ef7ff03407f f can never equal -m 4c57c7a5a95 Merge bitcoin-core/secp256k1#1554: cmake: Clean up testing code 472faaa8ee6 Merge bitcoin-core/secp256k1#1604: doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description 292310fbb24 doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description 85e224dd97f group: add ge_to_bytes and ge_from_bytes 7c987ec89e6 cmake: Call `enable_testing()` unconditionally 6aa576515ef cmake: Delete `CTest` module git-subtree-dir: src/secp256k1 git-subtree-split: a88aa9350633c2d2472bace5c290aa291c7f12c9
hebasto
added a commit
to hebasto/bitcoin
that referenced
this pull request
Oct 16, 2024
4f64b9f4cd build: Drop no longer needed `-fvisibility=hidden` compiler option 5d978f1899 ci: Run `tools/symbol-check.py` 258dba9e11 test: Add `tools/symbol-check.py` 3144ba99f3 Introduce `SECP256K1_LOCAL_VAR` macro e2571385e0 Do not export `secp256k1_musig_nonce_gen_internal` e59158b6eb Merge bitcoin-core/secp256k1#1553: cmake: Set top-level target output locations 18f9b967c2 Merge bitcoin-core/secp256k1#1616: examples: do not retry generating seckey randomness in musig 5bab8f6d3c examples: make key generation doc consistent e8908221a4 examples: do not retry generating seckey randomness in musig 70b6be1834 extrakeys: improve doc of keypair_create (don't suggest retry) 01b5893389 Merge bitcoin-core/secp256k1#1599: bitcoin#1570 improve examples: remove key generation loop cd4f84f3ba Improve examples/documentation: remove key generation loops a88aa93506 Merge bitcoin-core/secp256k1#1603: f can never equal -m 3660fe5e2a Merge bitcoin-core/secp256k1#1479: Add module "musig" that implements MuSig2 multi-signatures (BIP 327) 168c92011f build: allow enabling the musig module in cmake f411841a46 Add module "musig" that implements MuSig2 multi-signatures (BIP 327) 0be79660f3 util: add constant-time is_zero_array function c8fbdb1b97 group: add ge_to_bytes_ext and ge_from_bytes_ext ef7ff03407 f can never equal -m c232486d84 Revert "cmake: Set `ENVIRONMENT` property for examples on Windows" 26e4a7c214 cmake: Set top-level target output locations 4c57c7a5a9 Merge bitcoin-core/secp256k1#1554: cmake: Clean up testing code 472faaa8ee Merge bitcoin-core/secp256k1#1604: doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description 292310fbb2 doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description 85e224dd97 group: add ge_to_bytes and ge_from_bytes 7c987ec89e cmake: Call `enable_testing()` unconditionally 6aa576515e cmake: Delete `CTest` module git-subtree-dir: src/secp256k1 git-subtree-split: 4f64b9f4cdf130d2c6d6b7bd855a04faf6f88e08
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.
In fact, before reaching this particular VERIFY_CHECK, we had already successfully passed through
ensuring that f is not -m.