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 APrime.from_int into a wrapping and a checked variant #77

Open
real-or-random opened this issue Jan 15, 2025 · 1 comment · May be fixed by #79
Open

secp256k1proto: Split APrime.from_int into a wrapping and a checked variant #77

real-or-random opened this issue Jan 15, 2025 · 1 comment · May be fixed by #79

Comments

@real-or-random
Copy link
Collaborator

real-or-random commented Jan 15, 2025

I think the proper way is to change secp256k1proto to split APrimeFE.from_int into a APrimeFE.from_int_wrapping, which wraps around, and a APrimeFE.from_int_checked, which raises OverflowError. (APrimeFE is the abstract superclass of Scalar).

Then we can use the appropriate function everywhere. (Not sure if we want to touch bip340.py though, I tried to keep this as close as possible to the reference code in the BIP, so it's okay to work with integers there and keep the explicit %.)

Scalar(int_from_bytes(...)) is also used for generating pads (self_pad and ecdh). I think this is acceptable since we’re generating random numbers,

Acceptable, but probably better to do this in a consistent way unless you really want to wrap around.

The wrapping there was intentional: Wrapping is not only acceptable when the input is uniformly random (or a hash), it yields simpler code because it removes an error path. This was the rationale for why we wrap in BIP340, after a long discussion.

But the ECDH implementation is supposed to replicate whatever libsecp256k1 does, and it errors out in case of overflow...

And then, if we wrap in ECDH, we may as well wrap in self_pad for consistency. Sigh.

Originally posted by @real-or-random in #76 (comment)

@theStack
Copy link
Contributor

theStack commented Feb 2, 2025

The wrapping there was intentional: Wrapping is not only acceptable when the input is uniformly random (or a hash), it yields simpler code because it removes an error path. This was the rationale for why we wrap in BIP340, after a long discussion.

But the ECDH implementation is supposed to replicate whatever libsecp256k1 does, and it errors out in case of overflow...

Looked into this in the course of #79. The error in case of overflow is only related to the input scalar, not the result (which is just a 32-byte-string and not further converted), so it seems fine to keep the higher-level ecdh with wrapping? ecdh_libsecp256k1 already replicates the behaviour of secp256k1_ecdh, if I'm not missing anything.

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 a pull request may close this issue.

2 participants