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

Parse proper SEC1 EC point encoding in Secp256k1 decompress precompile #22

Open
huitseeker opened this issue Jun 5, 2024 · 0 comments

Comments

@huitseeker
Copy link
Contributor

Paraphrasing from https://github.com/wormhole-foundation/wp1/pull/138#discussion_r1575949868:

This call to k256::AffinePoint::decompress is actually making assumptions on representation which I'm not sure are represented elsewhere.

Indeed, just like BLS12-381, Secp256k1 has a point serialization standard in compressed form, and I'm not sure if the API of the Secp256k1 precompile should be this "compressed serialization" format, or a generic coordinate format.

The compressed serialization for Secp256k1 is defined through SEC1 paragraphs 2.3.3 and 2.3.4. Basically, it's a 33-byte serialization format, with the MSB being 02 if the intended y coordinate is even, 03 if it's odd.

The call we're making here to k256::AffinePoint::decompress in secp256k1_decompress is not following SEC1:
https://github.com/RustCrypto/elliptic-curves/blob/6ff3bb7d8632ea9970aa583c89e944356b8bc8d1/k256/src/arithmetic/affine.rs#L185

But k256 contains conversions to/from EncodedPoint that do follow that standard:
https://github.com/RustCrypto/elliptic-curves/blob/6ff3bb7d8632ea9970aa583c89e944356b8bc8d1/k256/src/arithmetic/affine.rs#L302-L312

And the EncodedPoint embeds those conventions:
https://github.com/RustCrypto/elliptic-curves/blob/6ff3bb7d8632ea9970aa583c89e944356b8bc8d1/primeorder/src/affine.rs#L164-L195

Leaving this issue open until we decide whether we want to improve on this (by using the proper EncodedPoint functions with full SEC1 support, and handling that both in and out of circuit), or decide that the current state is good enough and we just make sure to properly document the precompile mentioning this caveat.

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

No branches or pull requests

1 participant