status-im / nim-secp256k1

A wrapper for libsecp256k1
Apache License 2.0
7 stars 5 forks source link

Enable and expose Schnorrsig #44

Closed Gruruya closed 1 year ago

Gruruya commented 1 year ago

This patchset enables and adds an interface for multikey and schnorrsig in abi.nim.

I'm not sure how you would want the interface in secp256k1.nim to look, so I haven't touched that.

arnetheduck commented 1 year ago

Thanks for the PR!

I'm not sure how you would want the interface

Generally, the interface strives to provide a "safe" experience for Nim users, as far as the language allows one to do so - it's fine that we start with the ABI - my suggestion would be that you start using the raw ABI and think about how to safely wrap it in your application - once you have something you're satisfied with, we can revisit.

Gruruya commented 1 year ago

Currently, I'm rewriting SkKeyPair to use secp256k1_keypair so that I can use that for Schnorr signatures.

I've been able to mostly keep the behavior of keypair.pubkey, keypair.seckey, and keypair.seckey = the same. However, the field accesses are copies, seckey = changes the pubkey as well, and pubkey = is a no-op.

Most code would be unaffected by these changes, however, clear keypair.seckey now errors, instead requiring clear keypair. Would this breaking change be acceptable?

If not, I could instead make a new type such as SkMultiKeyPair, or I could make an implicit secp256k1_keypair out of a SkSecretKey every time I need a secp256k1_keypair. What do you think?

Edit: Nevermind, I decided to not bother with the breaking changes.

Gruruya commented 1 year ago

Good to go

One thing I'm wondering is how I should expose the aux_rand32 param for signSchnorr, should it (optionally) accept an Rng/FullproofRng or a 32-bit array of (presumably) random bytes?

Edit: I've implemented something 3c80efb. Its using secp256k1_ec_seckey_verify to check the RNG, but I'm not sure if this is excessive. Is there a more suitable way to check the RNG?

arnetheduck commented 1 year ago

32-bit array of (presumably) random bytes?

hm, with the ability to pass in an RNG, this is strictly not needed (the rng can simply return a given array), but I do wonder about the convenience.

Also, compared to the C version, we're making the "random-less" version easier to call (there, you need to explicitly specify null) - an explicit "no-randomness" version might be preferable, whether that is through Opt[Rng] or some other convention.

arnetheduck commented 1 year ago

Its using secp256k1_ec_seckey_verify to check the RNG, but I'm not sure if this is excessive. Is there a more suitable way to check the RNG?

Per https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#default-signing, the randomness is just that: random bytes - there is no need for further validation (unlike private keys which have a "valid range" and the randomness is actually treated as a number)

Gruruya commented 1 year ago

Also, compared to the C version, we're making the "random-less" version easier to call (there, you need to explicitly specify null) - an explicit "no-randomness" version might be preferable, whether that is through Opt[Rng] or some other convention.

Could keep it as is but add a warning or change its name to something like signSchnorrNoRng.

Gruruya commented 1 year ago

Also, compared to the C version, we're making the "random-less" version easier to call

Good point. I've renamed it to signSchnorrUnsafe. Could also do a different name or a warning, or even remove the proc.

Edit: I'll replace it with a signSchnorr that accepts an Opt array of bytes.

Gruruya commented 1 year ago

I get the error cannot open file: stew/byteutils on Nim >1.6. Removing the nimble.lock file fixes this, but updating it doesn't.

Also, a version bump once this pr is merged would be appreciated.

arnetheduck commented 1 year ago

Removing the nimble.lock file fixes this, but updating it doesn't.

Remove it - it's not yet ready for prime time

Gruruya commented 1 year ago

I'm using this patchset now without anything on top, it should be ready.