ecdh/ellswift: simplify seckey loading with `_scalar_set_b32_seckey` #1916

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.

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;                                                                                                         
      |            ^~~~~                                                

utACK c00dc8181054fb31a0149ce03ef8413300a3a8bf

Note that introducing parantheses around !!ret was necessary to avoid warnings in the following form:

Hm, indeed, but not sure why the compiler warns here. ! still binds stronger than & according to https://en.cppreference.com/c/language/operator_precedence. Perhaps is_sec_valid & !!ret would work without parantheses (but I'm not suggesting a change).

It's probably good to keep it for consistency, but this entire business of using & instead of && may be a little cargo cult. At least in this case, it shouldn't make a difference when it comes to constant-time compilation. We anyway don't know what the compiler will do.

Pull request overview

Simplifies secret-key validation in ECDH and EllSwift XDH using the existing scalar helper.

Changes:

• Replaces manual overflow/zero checks with secp256k1_scalar_set_b32_seckey.
• Preserves invalid-key substitution and return behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

💡 <a href="/bitcoin-core/secp256k1/new/master?filename=.github/skills/code-review/SKILL.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add a code-review agent skill</a> or configure MCP servers for context-aware, tailored reviews. <a href="https://docs.github.com/en/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review#mcp-servers-and-agent-skills" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn more in the docs.</a>

Note that introducing parantheses around !!ret was necessary to avoid warnings in the following form:

Hm, indeed, but not sure why the compiler warns here. ! still binds stronger than & according to https://en.cppreference.com/c/language/operator_precedence. Perhaps is_sec_valid & !!ret would work without parantheses (but I'm not suggesting a change).

Was also a bit surprised about the warning. is_sec_valid & !!ret would indeed work without parantheses as I just verified :shrug:

It's probably good to keep it for consistency, but this entire business of using & instead of && may be a little cargo cult. At least in this case, it shouldn't make a difference when it comes to constant-time compilation. We anyway don't know what the compiler will do.

Ah interesting, I honestly wasn't even aware that the idea behind using & instead of && was constant-time reasons. Agree that it shouldn't make a difference. If we apply #1621 and follow the suggestion "function is constant-time only for valid inputs", these constructs will be gone anyways.

There are by the way two other instances where _scalar_set_b32_seckey could be used that I missed in this PR (due to only focusing on public API functions), in the internal silentpayment functions for calculating the input_hash and t_k scalars: https://github.com/bitcoin-core/secp256k1/blob/a37d7cc064af8ac5be994b026f100dc51ab98765/src/modules/silentpayments/main_impl.h#L84-L85 https://github.com/bitcoin-core/secp256k1/blob/a37d7cc064af8ac5be994b026f100dc51ab98765/src/modules/silentpayments/main_impl.h#L144-L148

Would be nice to simplify there as well, on the other hand these values are not exactly "secret keys" (they just need to be in the same range, i.e. 0 < scalar < N), so not sure if that would be an improvement w.r.t. readability.

ACK c00dc8181054fb31a0149ce03ef8413300a3a8bf; successfully ran local tests