Schnorr signature support #258

kozross · 2022-02-21T22:17:17Z

This forms the first step in addressing this requirement, as well as addressing this naming concern. The test suite has been extended to cover this case as well.

This allows a more recent version of libsecp256k1 that has schnorr signatures.

…/cardano-base into koz/schnorr

kozross · 2022-02-21T22:18:53Z

@michaelpj and @iquerejeta - does this seem good?

iquerejeta

Naming looks good. I've left some comments motivated by consistency. Note that I'm not a haskell expert. But if we decide to have some generic names (such as secpKeyPairCreate or SECP256k1SecKey), would it make sense to do qualified imports to make these types or functions explicitly linked to each algorithm?

iquerejeta · 2022-02-22T08:47:31Z

cardano-crypto-class/src/Cardano/Crypto/DSIGN/SchnorrSecp256k1.hs

+  type SizeVerKeyDSIGN SchnorrSecp256k1DSIGN = 64
+  type Signable SchnorrSecp256k1DSIGN = SignableRepresentation
+  newtype VerKeyDSIGN SchnorrSecp256k1DSIGN = 
+    VerKeySchnorr256k1 (ForeignPtr SECP256k1XOnlyPubKey)


To maintain consistency, we may want to add

Suggested change

VerKeySchnorr256k1 (ForeignPtr SECP256k1XOnlyPubKey)

VerKeySchnorrSecp256k1 (ForeignPtr SECP256k1XOnlyPubKey)

Fair enough, I can do that.

Edit: On second thought, given that the constructors aren't publically-visible, what's the advantage here?

Oh, the advantage is simply the consistency of the code. It is a nit, but after all we are making this PR to use correct naming. IMO we should be consistent, and maintain correctness of naming throughout the code.

iquerejeta · 2022-02-22T08:59:08Z

cardano-crypto-class/src/Cardano/Crypto/DSIGN/EcdsaSecp256k1.hs

+  type SizeVerKeyDSIGN EcdsaSecp256k1DSIGN = 64
+  type Signable EcdsaSecp256k1DSIGN = ((~) SECP.Msg)
+  newtype VerKeyDSIGN EcdsaSecp256k1DSIGN = 
+    VerKeyEcdsaSecp256k1 SECP.PubKey


SECP is still Crypto.Secp256k1. Are these SECP associated types and functions (e.g. SECP.PubKey, SECP.derivePubKey, SECP.signMsg) here and below still exclusively linked to ECDSA?

Yes, they are. I could rename the import if that would be clearer, but I figured it wouldn't matter much.

The problem I see is that in the import

import qualified Crypto.Secp256k1 as SECP

there is no mention whatsoever to the actual signing algorithm. I would change the import to

import qualified Crypto.EcdsaSecp256k1 as SECP

or, if it is not possible, to

import qualified Crypto.Secp256k1 as ECDSA

iquerejeta · 2022-02-22T09:04:15Z

cardano-crypto-class/src/Cardano/Crypto/DSIGN/SchnorrSecp256k1.hs

+  SECP256k1XOnlyPubKey, 
+  secpKeyPairCreate, 
+  SECP256k1Context, 
+  secpKeyPairXOnlyPub,
+  SECP256k1SecKey, 
+  secpSchnorrSigVerify, 
+  secpContextSignVerify,
+  SECP256k1SchnorrSig, 
+  secpSchnorrSigSignCustom, 
+  secpContextCreate


Some reason why some of these functions contain Schnorr, and other don't? or why some contain secp and others SECP256k1? And a nit, have we changed the order of the prefixes in purpose? In here we go secp256k1schnorr while above we go with schnorrsecp256k1.

This is because they mirror the naming of the C functions they wrap (without 256k1 everywhere, admittedly).

iquerejeta · 2022-02-22T09:07:38Z

cardano-crypto-class/src/Cardano/Crypto/Schnorr.hs

+data SECP256k1KeyPair
+
+foreign import capi "secp256k1_extrakeys.h secp256k1_keypair_create"
+  secpKeyPairCreate :: 


are we excluding the prefix because the function is the same for both algorithms?

This function is not algorithmically-tied. The secp256k1_extrakeys.h header provides general keypair-related manipulation: it is not specific to Schnorr or ECDSA signatures.

michaelpj

I am extremely unqualified to review the FFI stuff, we should find someone to do that.

michaelpj · 2022-02-22T11:36:52Z

cardano-crypto-class/src/Cardano/Crypto/Schnorr.hs

+{-# LANGUAGE DerivingVia #-}
+{-# LANGUAGE ScopedTypeVariables #-}
+
+module Cardano.Crypto.Schnorr (


Hmmm, do you think we could contribute this upstream to secp256k1-haskell?

We could, but I chose not to, in the interests of getting this out the door faster. Possibly useful for the future though.

michaelpj · 2022-02-22T11:39:49Z

cardano-crypto-class/src/Cardano/Crypto/DSIGN/SchnorrSecp256k1.hs

+  algorithmNameDSIGN _ = "schnorr-secp256k1"
+  {-# NOINLINE deriveVerKeyDSIGN #-}
+  deriveVerKeyDSIGN (SignKeySchnorr256k1 fp) = 
+    unsafeDupablePerformIO . withForeignPtr fp $ \skp -> do


Do we want to isolate this stuff in a module that provides a nicer Haskell interface, more like secp256k1-haskell provides?

I don't feel this would help much. When it comes to FFI stuff, my policy is to bind as closely as possible to the C API, then wrap it directly. Adding another level of indirection in between doesn't strike me as useful, and has actually backfired quite badly in the past.

michaelpj · 2022-02-23T11:18:36Z

cardano-crypto-class/src/Cardano/Crypto/DSIGN/SchnorrSecp256k1.hs

+  deriveVerKeyDSIGN (SignKeySchnorr256k1 fp) = 
+    unsafeDupablePerformIO . withForeignPtr fp $ \skp -> do
+      let skp' :: Ptr CUChar = castPtr skp
+      allocaBytes 96 $ \kpp -> do


I note that this package has some utilities for slightly safer allocation, in here. I think you can use this in your foreign declarations (like here) to get some more safety and fewer magic numbers.

I saw that, but I actually don't think it helps all that much. I prefer keeping close to C, magic numbers and all, and avoiding the cost of 'calling down' type-level numbers to the value level.

Nonetheless, if it's going to be in this repository, I think it should follow the practices of this repository 🤷

I respectfully disagree. You're injecting quite expensive code into (arguably) a hot code path for no real benefit, since nobody outside of the maintainers of this repo will see it. If magic numbers are a concern, I am happy to replace those with named constants.

You're injecting quite expensive code into (arguably) a hot code path for no real benefit

I think both of these are wrong:

The code is not expensive. GHC happily reifies the statically known information at compile time: I looked at the Core for the Ed25519 module and it has numeric literals as the size arguments.

This is not a hot path: we are calling expensive primitives, we shouldn't need to worry too much about the FFI overhead.

In any case, please remove the magic numbers. You already had to define named constants in the class: use them. I also don't think there's a good reason not to use the library helpers here, and it makes things more consistent.

michaelpj · 2022-02-23T11:24:13Z

cardano-crypto-class/src/Cardano/Crypto/DSIGN/SchnorrSecp256k1.hs

+        res <- secpKeyPairCreate ctxPtr kpp skp'
+        when (res /= 1) (error "signDSIGN: Failed to create keypair")
+        sigFP <- mallocForeignPtrBytes 64
+        let (msgFP, msgOff, msgLen) = toForeignPtr bs


I think you could replace many (all?) the uses of toForeignPointer with useAsCString(Len), which doesn't require the bytestring Internal module.

michaelpj · 2022-02-23T11:28:03Z

cardano-crypto-class/src/Cardano/Crypto/DSIGN/SchnorrSecp256k1.hs

+      let skp' :: Ptr CUChar = castPtr skp
+      allocaBytes 96 $ \kpp -> do
+        res <- secpKeyPairCreate ctxPtr kpp skp'
+        when (res /= 1) (error "deriveVerKeyDSIGN: Failed to create keypair")


The error handling around here seems a bit strange, but I note that some of the other modules at least throw IO exceptions: https://github.com/input-output-hk/cardano-base/blob/39f9cf0eaf15a6888a47ac9d8de0b37dd1131d0b/cardano-crypto-class/src/Cardano/Crypto/DSIGN/Ed25519.hs#L53

Perhaps that function could be moved to Cardano.Crypto.Util and used here too.

I'm not sure whether error or an IO exception is more appropriate here. We're using unsafe*PerformIO everywhere anyway, so I don't think it makes that much difference.

error throws an IOException anyway, doesn't it?

michaelpj · 2022-02-23T11:30:36Z

cardano-crypto-class/src/Cardano/Crypto/DSIGN/SchnorrSecp256k1.hs

+        OnlyCheckWhnfNamed "VerKeySchnorr256k1" (ForeignPtr SECP256k1XOnlyPubKey)
+        )
+  newtype SignKeyDSIGN SchnorrSecp256k1DSIGN = 
+    SignKeySchnorr256k1 (ForeignPtr SECP256k1SecKey)


Why not use PinnedSizedBytes for these?

For the same reason as mentioned previously. My experience with FFI tells me that it's much better to be closer to C, and 'calling down' type level numbers to the value level is not a cheap operation.

Again, this is false: look at the Core if you don't believe me. These are statically known, so there is no overhead.

michaelpj · 2022-02-23T11:31:31Z

All of my recent comments were just caused by taking a look at some of the other code in this package, and suggesting ways of making it more consistent and using the utilities that the package provides. It would be great if you could do such a pass yourself, you'll probably find more stuff that I haven't spotted!

kozross · 2022-02-28T00:34:09Z

@michaelpj I have now rewritten the Schnorr signature work to use the wrappers in cardano-base, as well as useAsCStringLen. I believe this should resolve all outstanding concerns.

michaelpj · 2022-02-28T10:54:26Z

cardano-crypto-class/src/Cardano/Crypto/DSIGN/SchnorrSecp256k1.hs

-      unsafePackCStringLen (castPtr p, 64)
+      psb <- withForeignPtr (plusForeignPtr bsFP bsOff) $ \bsp -> do
+        psbCreate $ \skp -> do
+          copyPtr skp bsp 32 


why did this change? Please replace the magic numbers with constants.

michaelpj · 2022-02-28T10:56:44Z

I think the only outstanding comment is the one about magic numbers. I would still prefer you used SizedPtr, but I can do that afterwards so long as it's actually clear what the numbers mean.

@iquerejeta are you happy with the naming now?

iquerejeta · 2022-02-28T11:05:30Z

There are two unresolved comments above regarding naming. I agree with @kozross that the functions will not be exposed so we can be more flexible with the naming, but I still believe we need to be consistent and correct, for ease of maintenance of the code. The two particular cases are with the qualified import of ECDSA

import qualified Crypto.Secp256k1 as SECP

where we cannot see that it actually relates to ECDSA. I would prefer something along the lines of import qualified Crypto.EcdsaSecp256k1, but I would be ok with the second suggestion.

And the second point is that VerKeySchnorr256k1 should be changed to VerKeySchnorrSecp256k1.

kozross · 2022-02-28T18:16:36Z

@michaelpj How do you get a SizedPtr to the contents of an arbitrary ByteString?

@iquerejeta I'm not sure what you're asking for here: Crypto.Secp256k1 refers specifically to this module here. I can import it as ECDSA, but to be honest, if the naming confusion is anywhere, it's upstream. I can rename VerkeySchnorr256k1 as you suggest, but then, for consistency, I would need to rename all the other constructors too.

michaelpj · 2022-03-01T14:05:12Z

Okay, LGTM. Please merge master and I'll merge this.

michaelpj · 2022-03-01T14:05:44Z

Normally I feel like you get the option to merge master into other people's branches, but not here 🤔

iquerejeta · 2022-03-01T14:09:46Z

I can import it as ECDSA, but to be honest, if the naming confusion is anywhere, it's upstream.

Sounds good. I think that even if the naming confusion is upstream, we should make it as clear as we can in our code base. So that when future developers come to change the code, they know that they are handling the ECDSA algorithm, and not something different. My impression is that the upstream library was initially build only for ECDSA, and afterwards schnorr was added. That might justify some of the confusing naming. However, we are in a better position. We are including both ECDSA and Schnorr at the same time, so I don't see the need of making the same mistakes as upstream.

I can rename VerkeySchnorr256k1 as you suggest, but then, for consistency, I would need to rename all the other constructors too.

Sounds reasonable to me. I think it is important to be consistent.

Edit: And sorry if I made some comments that did not make much sense with respect to renaming the modules. As I said, I've never programmed in Haskell, so it was hard for me to distinguish between the names being introduced here, and the names of upstream.

kozross · 2022-03-01T18:11:41Z

@iquerejeta secp256k1-haskell doesn't support Schnorr signatures at all, which is why I had to FFI into secp256k1 directly.

michaelpj · 2022-03-03T15:05:30Z

Follow ups in #262

tdammers

Looks good to me.

tdammers · 2022-03-11T11:59:28Z

cardano-crypto-class/cardano-crypto-class.cabal

@@ -1,129 +1,127 @@
-cabal-version: 2.2
+cabal-version:      2.2


It would have been better to leave the formatting style intact; reformatting file like this when you're only making minor changes produces "diff noise", makes code harder to merge / rebase, and makes it more difficult to spot the actual changes. Improving formatting is great, but I'd recommend (for the future) to do it in a separate commit.

tdammers · 2022-03-11T12:13:29Z

cardano-crypto-class/src/Cardano/Crypto/DSIGN/SchnorrSecp256k1.hs

+      let skp' :: Ptr CUChar = castPtr skp
+      allocaBytes 96 $ \kpp -> do
+        res <- secpKeyPairCreate ctxPtr kpp skp'
+        when (res /= 1) (error "deriveVerKeyDSIGN: Failed to create keypair")


error throws an IOException anyway, doesn't it?

kozross and others added 12 commits February 15, 2022 07:08

Initial bindings

a27a66c

Try updating haskell.nix

87b4065

Add nixpkgs pin, move to recent revision

d085afd

This allows a more recent version of libsecp256k1 that has schnorr signatures.

Bump haskell.nix

7a8c98b

Fix nixpkgs

e227b99

Merge branch 'master' into mpj/bump-nixpkgs

fcefd4f

Merge branch 'mpj/bump-nixpkgs' of https://github.com/input-output-hk…

465f757

…/cardano-base into koz/schnorr

Finish FFI bits

68acbf7

Rename the prior SECP work to be more accurate

332b95a

Implement DSIGN interface for Schnorr signatures

72f4266

Add to test suite, ensure everything passes

2bfa4d8

Merge branch 'master' into koz/schnorr

3caf9f8

iquerejeta reviewed Feb 22, 2022

View reviewed changes

michaelpj reviewed Feb 22, 2022

View reviewed changes

michaelpj reviewed Feb 23, 2022

View reviewed changes

kozross mentioned this pull request Feb 24, 2022

SECP256k1 support IntersectMBO/plutus#4368

Merged

kozross added 2 commits February 28, 2022 13:14

Rewrite Schnorr implementation with PinnedByteArray

7436983

Rewrite Schnorr with useAsCStringLen

f6c2453

michaelpj reviewed Feb 28, 2022

View reviewed changes

kozross added 2 commits March 1, 2022 07:25

Remove magic number on copyPtr

a7a77ab

Rename SECP import to ECDSA, clearer constructor names for Schnorr

0557a3a

Merge branch 'master' into koz/schnorr

ec9e615

michaelpj approved these changes Mar 2, 2022

View reviewed changes

michaelpj merged commit 1587462 into IntersectMBO:master Mar 2, 2022

tdammers reviewed Mar 11, 2022

View reviewed changes

This was referenced Sep 28, 2022

Secp256k1 new built ins #304

Closed

CIP-0049? | ECDSA and Schnorr signatures in Plutus Core cardano-foundation/CIPs#250

Merged

	VerKeySchnorr256k1 (ForeignPtr SECP256k1XOnlyPubKey)
	VerKeySchnorrSecp256k1 (ForeignPtr SECP256k1XOnlyPubKey)

Schnorr signature support #258

Schnorr signature support #258

Conversation

kozross commented Feb 21, 2022 • edited Loading

kozross commented Feb 21, 2022

iquerejeta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozross Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj commented Feb 23, 2022

kozross commented Feb 28, 2022

Choose a reason for hiding this comment

michaelpj commented Feb 28, 2022

iquerejeta commented Feb 28, 2022

kozross commented Feb 28, 2022 • edited Loading

michaelpj commented Mar 1, 2022

michaelpj commented Mar 1, 2022

iquerejeta commented Mar 1, 2022 • edited Loading

kozross commented Mar 1, 2022

michaelpj commented Mar 3, 2022

tdammers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozross commented Feb 21, 2022 •

edited

Loading

kozross Feb 22, 2022 •

edited

Loading

kozross commented Feb 28, 2022 •

edited

Loading

iquerejeta commented Mar 1, 2022 •

edited

Loading