Introduce opaque secret key and stronger types for fixed size byte arrays

rickmark commented at 6:51 am on March 26, 2021: none

This changeset introduces a level of indirection between the EC secret key compact representation (a big endian 32 byte value) and the secp256k1_seckey type. This allows for the type to have an internal representation that differs in the future. Because the types are compatible today, this breaks the API but not the ABI. Callers should parse their compact keys with secp256k1_ec_seckey_parse_compact before using that value where a secret key is expected.

Introduce opaque secret key and stronger types

This changeset introduces a level of indirection between the EC secret key compact representation (a big endian 32 byte value) and the secp256k1_seckey type.  This allows for the type to have an internal representation that differs in the future.  Because the types are compatible today, this breaks the API but not the ABI.  Callers should parse their compact keys with secp256k1_ec_seckey_parse_compact before using that value where a secret key is expected.

49b4ef46ed

rickmark cross-referenced this on Mar 26, 2021 from issue EC private/secret key needs a level of indirection / opaque type like public keys by rickmark

sipa commented at 7:01 am on March 26, 2021: contributor

What is the motivation for wanting a different representation for secret keys?

The types for which we have this do so because they either have multiple common serializations, or because there are is preprocessing involved that makes it faster. For secret keys I can’t really imagine something like that - apart from the byteswapping, but that seems unlikely to weigh up against the cost of introducing an extra indirection.

Note also that for newer code we’re using the extrakeys module’s keypair type, which carries a combined private key/pubkey.

rickmark commented at 7:19 am on March 26, 2021: none

What is the motivation for wanting a different representation for secret keys?

Well there are at least two common representations of private keys, raw 32 byte integers, and DER encoded. This was meant to also help people not feed an ASN private key into functions expecting a private.

I figure byte swapping might be a thing with some hardware. Being able to parse a 32 byte big endian integer into the machine native format for bigint may provide a lot of value.

Note also that for newer code we’re using the extrakeys module’s keypair type, which carries a combined private key/pubkey.

Yep exactly, I was figuring a private key in the future may also bring along its public to prevent multiple computation of the public key.

Since API stabilization is a blocker to a v1 release this change allows one to modify private keys in the future without breaking API compatibility then

jonasnick commented at 1:35 pm on March 26, 2021: contributor

Hi @rickmark,

I like that this way seckey_verify is called always instead of leaving that up to the user. Not sure if that’s worth the indirection. Also, there’s some existing effort to remove secret key DER parsing (https://github.com/bitcoin-core/secp256k1/pull/781).

a private key in the future may also bring along its public to prevent multiple computation of the public key.

Why not reuse the keypair type for this?

You may want to have a look at the v1.0 milestone which should include the high priority items.

rickmark commented at 4:54 pm on March 26, 2021: none

a private key in the future may also bring along its public to prevent multiple computation of the public key.

Why not reuse the keypair type for this?

The main reasons are for the tweak methods which take a private key

I like that this way seckey_verify is called always instead of leaving that up to the user. Not sure if that’s worth the indirection. Also, there’s some existing effort to remove secret key DER parsing (#781).

The goal is that we should prefer the indirection now because it would be difficult to add to the API / ABI later on.

sipa commented at 5:59 pm on March 26, 2021: contributor

because it would be difficult to add to the API / ABI later on.

I agree with the fact that if we expect to want to change the representation at some point, it is preferable to introduce an abstraction like this - and if we’re going to do so, it should be done before stabilizing the API.

I’m not convinced there is a reason why we’d want to change the representation to something else that fits in 32 bytes. Some possibilities for other representations include:

secp256k1_scalar (if that’s 32 bytes), allowing saving the cost of byteswapping if you’re going to do more than one operation with the same private key. I expect this is a tiny benefit at best.
carrying the public key with it: that won’t fit in 32 bytes anyway, and for that secp256k1_keypair should be used.

So I’m not sure this is worth the API complexity.

elichai commented at 7:51 pm on March 26, 2021: contributor

I would just say that one of the things I like about schnorr is that the signature is just a 64 byte array. Note that a lot of time when abstracting to other languages people write wrappers that take byte arrays and call multiple functions (even bitcoin core does something similar: https://github.com/bitcoin/bitcoin/blob/master/src/pubkey.cpp#L191)

rickmark commented at 6:10 pm on March 29, 2021: none

carrying the public key with it: that won’t fit in 32 bytes anyway, and for that secp256k1_keypair should be used.

I’m looking at the types secp256k1_pubkey and secp256k1_seckey as ways to do a memcpy with sizeof(secp256k1_seckey) so that it will remain source compatible if they change sizes.

The function takes a 32 byte compact representation now, and emits that opaque type. Goal again is source compatibility so 32 bytes today, but easily may become 96 if it becomes a private public pairing. Really its expressing the type to prevent someone from doing something silly like taking an arbitrary byte stream (like a DER private key for example, or accidentally pointing to a public key byte array). I know this sounds a little silly, people using this library ought understand cyrpto enough to not do that 😆

sipa commented at 6:14 pm on March 29, 2021: contributor

Again, if you want a type that is a combination of private and public key, use secp256k1_keypair; it already exists.

The only point that matters here is whether or not we expect to ever want a way to represent private keys in another way that’s guaranteed to fit in 32 bytes. I’m not convinced.

rickmark commented at 6:17 pm on March 29, 2021: none

The only point that matters here is whether or not we expect to ever want a way to represent private keys in another way that’s guaranteed to fit in 32 bytes. I’m not convinced.

Maybe a typedef secp256k1_scalar secp256k1_seckey would be enough here and using an inline function for secp256k1_seckey_parse_compact returning the same value is the correct thing to do then…

sipa commented at 6:17 pm on March 29, 2021: contributor

secp256k1_scalar is an internal type that’s not exposed. It depends on compile-time parameters that shouldn’t influence the public API.

real-or-random commented at 6:20 pm on March 29, 2021: contributor

No matter if we have given a guarantee on API stability or not, a lot of projects rely on this library, so I think it will be good not to break the API now unless we have a good reason (edit: at least in such basic things such as secret keys.)

sipa commented at 2:49 pm on May 9, 2023: contributor

Closing, I don’t think we want to make such an invasive change at this point in the lifetime of the library without tangible advantages.

I do think it’s worth thinking about what the criterion going forward should be for new types, whether they get a new abstract type, or as byte array pointers.

sipa closed this on May 9, 2023

Introduce opaque secret key and stronger types for fixed size byte arrays #911