Design flaws in recent API changes #439

I merged secp256k1 into our open source code base back in July 2015. Since then, I notice that there has been considerable work and activity, most of it for the better - great job! However, there are some choices that I think might have overlooked some things. In particular a parsed public key must be stored in this type:

typedef struct {
    unsigned char data[64];
} secp256k1_pubkey;

I think this was a mistake. It forces callers into one of 3 unpalatable choices:

1. Limit the containers used to hold public keys
2. Perform a buffer copy from an arbitrary container to a temporary secp256k1_pubkey
3. Write code which could cause undefined behavior (e.g. reinterpret_cast or a C-style cast)

Consider a simple case where a std::vector is used to store a single application defined byte, followed by a public key:

std::vector<unsigned char> v;
v.resize(65);
v[0] = 42; // application defined value
unsigned char const* pubkey = &v[1]; // 64 byte public key
...

How do we pass the public key to secp256k1_ec_pubkey_serialize? There are two ways:

secp256k1_pubkey temp;
std::memcpy(&temp.data[0], pubkey, sizeof(temp.data));
secp256k1_ec_pubkey_serialize(..., &temp, ...);

or

secp256k1_ec_pubkey_serialize(...,
    reinterpret_cast<secp256k1_pubkey const*>(pubkey), ...);

In the first case we have an unnecessary buffer copy. In the second case, we have the potential for undefined behavior due to misalignment. Furthermore, use of reinterpret_cast is a code smell (this could just have easily been a C-style cast).

The third option is not to use std::vector in that fashion, but this means the API choices of secp256k1 functions constrain callers' choice of containers used to hold cryptographic primitives.

I think it would have been better simply to use void const* pubkey as the parameter type in functions that currently accept secp256k1_pubkey*.

If you have your own representation of keys, I would expect that your application makes assumptions about its contents. In that case, my suggestion would be to use a vector (or any container you like), with a serialized key in it. Whenever you need the key, deserialize it into a secp256k1_pubkey. That has well-defined stable properties that don't change over time, and has no risk in breaking assumptions with future versions.

If for performance reasons you want to keep deserialized keys around, then I believe there is no problem in being restricted to using secp256k1_pubkey as data type. It makes it clear to contributors of the application code base that the contents is defined by the secp256k1 library, and can change.

I think it would have been better simply to use void const* pubkey as the parameter type in functions that currently accept secp256k1_pubkey*.

Yes, that was the alternative we considered. However, for good abstraction, allocation of such objects would also need to be left to the library (i.e. have a secp256k1_pubkey_create and secp256k1_pubkey_free function), which would unnecessarily require heap allocation for objects that fit on the stack (a performance issue for some use cases). So the choice was made to expose the size of type, but not its representation.

Sounds good to me then - I'll close this.