Possibly wrong context type for the incremental EdDSA init functions

[LoupVaillant]

The following functions:

crypto_sign_init_first_pass()
crypto_check_init()
crypto_ed25519_sign_init_first_pass()
crypto_ed25519_check_init()

All take a crypto_sign_ctx_abstract* as first argument. Thing is, under the hood, they are using a specific vtable, which expect a very specific context type: crypto_sign_ctx and crypto_sign_ed25519_ctx respectively. Otherwise it would be undefined behaviour.

If we compare this to C++, those functions would be constructors. Constructors don't just take pointers to the base class, they produce values of the derived classes directly. By that logic, those init functions should return an instance of the concrete context type. Or, to be consistent with the rest of Monocypher's init functions, take a pointer to the concrete type.

So, I believe those function prototypes should be changed to the following:

void crypto_sign_init_first_pass(crypto_sign_ctx, /* ... */);
void crypto_check_init(crypto_check_ctx, /* ... */)
void crypto_ed25519_sign_init_first_pass(crypto_sign_ed25519_ctx, /* ... */)
void crypto_ed25519_check_init(crypto_check_ed25519_ctx, /* ... */)

Hopefully this does not count as "breaking compatibility".

@fscoto, thoughts?

[fscoto]

Your proposal changes the function signatures. When compiling Monocypher as C++, the types necessarily become part of the function name (C++ name mangling).

While the Monocypher makefile compiles Monocypher as C, compilation as C++ is also explicitly supported in the README. It therefore follows that it is possible that for an embedded system, Monocypher was compiled as C++ and shipped as part of an SDK. The downstream consumers of the SDK would then have to lock-step upgrade firmware and SDK.

Since we can't rule out that people are in this situation, this is a breaking ABI change but only in C++ compilation.

That's reason enough for a “no” from me.

[LoupVaillant]

Okay, I can accept that. I would have incremented the soname, but it's only at version 3 right now, so there might be an expectation that the ABI is as stable as the API. And the problem isn't big enough to justify breaking the ABI.

One more note: the manual is lying to a very small extent: the incremental functions without custom hash advertise a pointer to the specialised context type already. So, users who don't just look at the header without looking at the manual are already strongly encouraged to use the right context type from the get go. On the other hand, if we take the manual at face value, there's a small contradiction between it and the custom hash manual page: we use the regular update and final functions, which can take any outer context, but the manual said it took only a Blake2b outer context.

Oh well. This is not perfect, but I'd rather lie a little than hurt the clarity of the "less" advanced manual page on the incremental signature API.