schnorrsig API overhaul

[jonasnick]

This is a work in progress because I wanted to put this up for discussion before writing tests. It addresses the TODOs that didn't make it in the schnorrsig PR and changes the APIs of schnorrsig_sign, schnorrsig_verify and hardened_nonce_function.

• Ideally, the new aux_rand32 argument for sign would be const, but didn't find a solution I was happy with.
• Support for variable length message signing and verification supports the suggested BIP amendment for such messages.
• sign_custom with its opaque config object allows adding more arguments later without having to change the API again. Perhaps there are other sensible customization options, but I'm thinking of sign-to-contract/covert-channel in particular. It would require adding the fields unsigned char *s2c_data32 and secp256k1_s2c_opening *s2c_opening to the config struct. The former is the data to commit to and the latter is written to by sign_custom. (EDIT: see below)

[jonasnick]

ping :)

[sipa]

Concept ACK

[elichai]

Can we use an opaque "array" struct for the config instead of using malloc? this will still allow us to extend the struct while also supporting platforms without heap allocation

[jonasnick]

@elichai Yeah good point, I agree that we should avoid dynamic memory allocation... and was hoping for suggestions :)

The opaque array struct wouldn't be fully backwards compatible because it only works if the caller is linked to the correct version of libsecp. In my limited understanding, I can count three reasonable options if we want to avoid malloc and be binary backwards compatible:

1. Use a versioned struct as in the Linux kernel. I implemented this variant in a branch but I don't know enough of the implications, i.e. how well this works with ffi.
2. Make space for future additions in the object by making the opaque byte array extra large or adding dummy members if we'd use a public struct instead.
3. Allocate it on the scratch space and work on optionally providing stack space to the scratch space (instead of having it malloc). That's not exactly backwards compatible either because the provided scratch space may become to small.

In case 1 and 2 we should document that the struct size may change between versions such that no one relies on this (otherwise it seems like this would become super complex)

[real-or-random]

Am I right that the user can still create config objects simply on the stack? This should be the case, we shouldn't force the user to have malloc.

But if the answer to this question is yes, then I think we could get rid of the create/destroy functions entirely. Config objects will be small, it's fine and natural to use the stack. And if the user really wants dynamic memory, she can call still call malloc.

Or do we think the size of the config object isn't statically given? This is true for example for the context object. Here, the size depends on the flags. (Although you could argue that there are only four types of contexts with known static sizes...) But here I don't think this will be case for config objects.

[real-or-random]

Ah sorry, I haven't seen your previous comment @jonasnick.

I think what I have in mind is similar to 1. but even simpler.

As long as we keep this an opaque object with manipulation functions provided by us, and we give the caller a way to figure out it's size (e.g. for FFI/dynamic alloc), versioning should not be necessary. Am I right?

[apoelstra]

@real-or-random the problem is that the "manipulation functions provided by us" need to use malloc.

[jonasnick]

I implemented & pushed option 1, the versioned struct, after playing with it here. The only downside is that if we add fields, we have to make sure that the new struct is in fact larger than the old one - on all supported compilers and platforms. We already make an assumption on the biggest alignment, so that should help figuring out when to add dummy fields.

I also mentioned above that there would be issues with FFI, but there are actually none. The concern in the issue I linked to was essentially that a struct could change out from under the bindings, but that's exactly what versioned structs prevent.

I'll add the missing tests and documentation.

As long as we keep this an opaque object

@real-or-random callers can't put an opaque object on the stack because they can't statically determine its size.

[jonasnick]

I pushed a new version of this PR with two major changes:

1. The versioned struct for extra sign_custom parameters is removed. Versioned structs are useful for forward compatibility (i.e. application expecting newer library actually calls older library), but it's not clear what to do in case we add sign to contract members to the struct, but the library doesn't understand S2C yet. We can abort, but that's not really forward compatibility. Instead it's a simple struct now with a magic value which can be used to achieve backward compatibility. If we decide to change the struct, we also change the magic which then acts as a version number and allows libsecp to distinguish between different structs.
2. The schnorrsig_sign function only accepts 32-byte messages again. Also, I added the public secp256k1_tagged_sha256 function to create tagged hashes according to BIP-340. The schnorrsig_sign doc explains that for messages that are not 32 bytes, one should use secp256k1_tagged_sha256 with a domain seperator. schnorrsig_sign_custom still allows truly signing variable length messages. As such, with libsecp both methods for variable length messages can be used, but one is clearly recommended to not confuse users and that's the one that currently has the most adoption (Taproot, DLCs, Lightning offers). And the additional tag argument to secp256k1_tagged_sha256 makes it harder to miss domain separation.

I also added tests and docs. Since this PR doesn't contradict the discussed BIP-340 changes, I removed the WIP status.

[jonasnick]

Rebased on top of master because cirrus wasn't included in this branch.

[apoelstra]

ack 6450368

[sipa]

API looks good, code review ACK. Feel like squashing in the fixup?

[jonasnick]

Done.

[jonasnick]

rebased

[jonasnick]

The new leak sanitizer in CI detected a missing context_destroy in the tagged_sha256_tests. Fixed & squashed.

[apoelstra]

ack 439ed2f

Glad we came to a "resolution" on the forward-compat issue.

[ariard]

Mostly nits, SGTM

[ariard]

utACK 66ff2fa

Thanks for taking the nits.

[apoelstra]

utACK the fixup commit

[jonasnick]

squashed the comment fixup commit

[apoelstra]

ack 5f6ceaf

[ariard]

utACK 5f6ceaf

[LLFourn]

utACK 5f6ceaf