Merge bitcoin-core/secp256k1#1916: ecdh/ellswift: simplify seckey loading with `_scalar_set_b32_seckey`
What the developer wrote
Authored by merge-script
Merge bitcoin-core/secp256k1#1916: ecdh/ellswift: simplify seckey loading with `_scalar_set_b32_seckey`
c00dc8181054fb31a0149ce03ef8413300a3a8bf ecdh/ellswift: simplify seckey loading with `_scalar_set_b32_seckey` (Sebastian Falbesoner)
Pull request description:
Instead of checking for overflow and zero manually, use the existing `_scalar_set_b32_seckey` helper which does both and returns zero if either of these two conditions apply. This can be seen as a very late follow-up to PR #701, commit 3fec9826086aa45ebbac1ff6fc3bb7b25ca78b1d, where the helper has been introduced and applied to the functions `_ecdsa_sign`, `_pubkey_create` and `_seckey_verify`. The variable name `is_sec_valid` has been chosen as it is currently also used in [`secp256k1_ecdsa_sign_inner`](https://github.com/bitcoin-core/secp256k1/blob/439278a649d3099d62dde966a76dc04aaca7ccb3/src/secp256k1.c#L555).
Note that introducing parantheses around `!!ret` was necessary to avoid warnings in the following form:
```
/home/thestack/secp256k1_master/src/modules/ecdh/main_impl.h: In function ‘secp256k1_ecdh’:
/home/thestack/secp256k1_master/src/modules/ecdh/main_impl.h:74:12: warning: suggest parentheses around operand of ‘!’ or change ‘&’ to ‘&&’ or ‘
!’ to ‘~’ [-Wparentheses]
74 | return !!ret & is_sec_valid;
| ^~~~~
```
ACKs for top commit:
real-or-random:
utACK c00dc8181054fb31a0149ce03ef8413300a3a8bf
Tree-SHA512: fc28140b09b0d8143b05a21f83b9d24f424384823ca26b8b3fbe103156b2bd2d8d7df76468002d794c43a73d4d7c0532420e2396f2b604a7e65d467baba51600
What changed, and why it matters
This is a small internal cleanup in Bitcoin Core's secp256k1 cryptography library. It replaces a manual secret-key validity check (overflow plus zero) with an existing helper function that does the same thing. The behavior is intended to be identical; no security vulnerability is described or visible in the code change.
No security action required. Treat as normal code-quality refactor. If reviewing, verify that secp256k1_scalar_set_b32_seckey semantics match the replaced overflow|is_zero logic, which the diff and PR description confirm.
Security signals we found
No security-relevant behavioral change is described or evident
Refactoring only: equivalent overflow-and-zero check via existing helper
Return value logic preserved with added parentheses for warning avoidance
No bounds changes, no new branches, no new inputs, no secret leakage
Evidence from the diff
The commit refactors two functions, secp256k1_ecdh and secp256k1_ellswift_xdh, to use secp256k1_scalar_set_b32_seckey instead of separately calling secp256k1_scalar_set_b32, secp256k1_scalar_is_zero, and OR-ing the results. The helper returns zero if the scalar is zero or overflows, and the callers then substitute the scalar with one in those cases. The return expression is rewritten from !!ret & !overflow to (!!ret) & is_sec_valid, which is logically equivalent. The change reduces code duplication and fixes a compiler warning about operator precedence.
Changed components
src/modules/ecdh/main_impl.hsrc/modules/ellswift/main_impl.hInspect captured patch +8 / −11
### src/modules/ecdh/main_impl.h
@@ -33,7 +33,7 @@ const secp256k1_ecdh_hash_function secp256k1_ecdh_hash_function_default = ecdh_h
int secp256k1_ecdh(const secp256k1_context* ctx, unsigned char *output, const secp256k1_pubkey *point, const unsigned char *scalar, secp256k1_ecdh_hash_function hashfp, void *data) {
int ret = 0;
- int overflow = 0;
+ int is_sec_valid;
secp256k1_gej res;
secp256k1_ge pt;
secp256k1_scalar s;
@@ -46,10 +46,8 @@ int secp256k1_ecdh(const secp256k1_context* ctx, unsigned char *output, const se
ARG_CHECK(scalar != NULL);
secp256k1_pubkey_load(ctx, &pt, point);
- secp256k1_scalar_set_b32(&s, scalar, &overflow);
-
- overflow |= secp256k1_scalar_is_zero(&s);
- secp256k1_scalar_cmov(&s, &secp256k1_scalar_one, overflow);
+ is_sec_valid = secp256k1_scalar_set_b32_seckey(&s, scalar);
+ secp256k1_scalar_cmov(&s, &secp256k1_scalar_one, !is_sec_valid);
secp256k1_ecmult_const(&res, &pt, &s);
secp256k1_ge_set_gej(&pt, &res);
@@ -73,7 +71,7 @@ int secp256k1_ecdh(const secp256k1_context* ctx, unsigned char *output, const se
secp256k1_ge_clear(&pt);
secp256k1_gej_clear(&res);
- return !!ret & !overflow;
+ return (!!ret) & is_sec_valid;
}
#endif /* SECP256K1_MODULE_ECDH_MAIN_H */
### src/modules/ellswift/main_impl.h
@@ -535,7 +535,7 @@ const secp256k1_ellswift_xdh_hash_function secp256k1_ellswift_xdh_hash_function_
int secp256k1_ellswift_xdh(const secp256k1_context *ctx, unsigned char *output, const unsigned char *ell_a64, const unsigned char *ell_b64, const unsigned char *seckey32, int party, secp256k1_ellswift_xdh_hash_function hashfp, void *data) {
int ret = 0;
- int overflow;
+ int is_sec_valid;
secp256k1_scalar s;
secp256k1_fe xn, xd, px, u, t;
unsigned char sx[32];
@@ -555,9 +555,8 @@ int secp256k1_ellswift_xdh(const secp256k1_context *ctx, unsigned char *output,
secp256k1_ellswift_xswiftec_frac_var(&xn, &xd, &u, &t);
/* Load private key (using one if invalid). */
- secp256k1_scalar_set_b32(&s, seckey32, &overflow);
- overflow |= secp256k1_scalar_is_zero(&s);
- secp256k1_scalar_cmov(&s, &secp256k1_scalar_one, overflow);
+ is_sec_valid = secp256k1_scalar_set_b32_seckey(&s, seckey32);
+ secp256k1_scalar_cmov(&s, &secp256k1_scalar_one, !is_sec_valid);
/* Compute shared X coordinate. */
secp256k1_ecmult_const_xonly(&px, &xn, &xd, &s, 1);
@@ -577,7 +576,7 @@ int secp256k1_ellswift_xdh(const secp256k1_context *ctx, unsigned char *output,
secp256k1_fe_clear(&px);
secp256k1_scalar_clear(&s);
- return !!ret & !overflow;
+ return (!!ret) & is_sec_valid;
}
#endifWhy this scored 18/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.