Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

secp256k1proto: split APrimeFE.from_bytes into checked and wrapping variants #79

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

theStack
Copy link
Contributor

@theStack theStack commented Feb 2, 2025

Note that the issue title refers to a from_int method, but since the check-or-wraparound discussion mostly concerns the initialization of scalars/fes from byte-strings (e.g. pseudo-random data from tagged hash results), the split-up is applied on the .from_bytes method. As suggested in #77, the BIP-340 implementation is not adapted and still applies the wrapping manually (% GE.ORDER), in order to keep it close to the reference implementation.

Resolves #77.

This is the first step of splitting up the `APrimeFE` deserialization
method into a checked and wrapping variant.
Note that this function is intentionally not used in the BIP-340
implementation to keep it close to the reference implementation.
@real-or-random
Copy link
Collaborator

Concept ACK

Given my comment in #80, perhaps we should do this:

  • add from_int_checked and from_int_wrapping
  • from_byte_checked and from_byte_wrapping can delegate to the from_int_* methods
  • The constructor can be an alias for from_int_checked

@theStack
Copy link
Contributor Author

theStack commented Mar 8, 2025

The constructor can be an alias for from_int_checked

Hm, the APrimeFE constructor currently takes two parameters a,b for initializing numerator/denominator and we pass overflow values internally on a few places (see e.g. __add__, __mul__). Should we rename this constructor body to something like from_num_denom and have the actual constructor with one parameter instead? Or keep it as-is but throw if either of the values a,b are integers and overflow (with the drawback that the reduction has to be done on the call-sites internally)?

(Btw I plan to address this issue directly in secp256k1lab, but thought it makes sense to continue the discussion still here to not lose context).

@real-or-random
Copy link
Collaborator

real-or-random commented Mar 10, 2025

Good point. So we shouldn't touch the constructor. And yes, we use it often within the class, so everything will break if it checks for overflow.

Perhaps we can document in the docstring of the class, or in the one of __init__, that it's is low-level and callers should usually prefer one of the factory methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

secp256k1proto: Split APrime.from_int into a wrapping and a checked variant
2 participants