Add SchnorrSignature type with parsing and serialization

[prolic]

• Introduced SchnorrSignature type with parsing and serialization functions.

resolves #8

[prolic]

@ProofOfKeags Let me know if I should change something. I also added the randomness, that was only put as "todo".

[prolic]

I can also remove the randomness, it's not required for my use case, I just saw it as Todo and thought I might as well do it quickly.

…

On Mon, Sep 30, 2024, 16:52 ProofOfKeags ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Crypto/Secp256k1.hs <#9 (comment)> : > schnorrSign KeyPair{..} bs | BS.length bs /= 32 = Nothing | otherwise = unsafePerformIO . evalContT $ do (msgHashPtr, _) <- ContT (unsafeUseByteString bs) keyPairPtr <- ContT (withForeignPtr keyPairFPtr) lift $ do sigBuf <- mallocBytes 64 - -- TODO: provide randomness here instead of supplying a null pointer - ret <- Prim.schnorrsigSign ctx sigBuf msgHashPtr keyPairPtr nullPtr - if isSuccess ret - then Just . Signature <$> newForeignPtr finalizerFree sigBuf - else free sigBuf $> Nothing + gen <- newStdGen Also please put the randomness change in as a separate commit. — Reply to this email directly, view it on GitHub <#9 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADAJPBB3KNNQ7ES2NYFXVLZZG2X3AVCNFSM6AAAAABPEEXN3CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMZYGU2DCNZVGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[prolic]

@ProofOfKeags done

[prolic]

@ProofOfKeags I added a second commit with the randomness changes required (also schnorrSign lives now in IO). It's up to you if you want to accept the randomness or not, but the schnorr signature changes with import / export capabilities are something that I absolutely need.

Thanks for your great work, without you, I would have to maintain yet another lib.

[ProofOfKeags]

One change I'd make and otherwise I think this is ready to go.

[prolic]

Alright, not sure if I get to it today, but latest tomorrow you'll have it.

…

On Mon, Sep 30, 2024, 18:10 ProofOfKeags ***@***.***> wrote: ***@***.**** requested changes on this pull request. One change I'd make and otherwise I think this is ready to go. ------------------------------ In src/Crypto/Secp256k1.hs <#9 (comment)> : > @@ -702,28 +745,30 @@ keyPairPubKeyXOTweakAdd KeyPair{..} Tweak{..} = unsafePerformIO . evalContT $ do -- | Compute a schnorr signature using a 'KeyPair'. The @ByteString@ must be 32 bytes long to get a @just@ out of this -- function -schnorrSign :: KeyPair -> ByteString -> Maybe Signature +schnorrSign :: KeyPair -> ByteString -> IO (Maybe SchnorrSignature) Sorry for running you in circles, I think you should break this up into three functions: schnorrSign :: Maybe StdGen -> KeyPair -> ByteString -> Maybe SchnorrSignature schnorrSign = ... schnorrSignDeterministic :: KeyPair -> ByteString -> Maybe SchnorrSignature schnorrSignDeterministic = schnorrSign Nothing schnorrSignNondeterministic :: KeyPair -> ByteString -> IO (Maybe SchnorrSignature) schnorrSignNondeterministic kp bs = newStdGen >>= \gen -> schnorrSign (Just gen) kp bs — Reply to this email directly, view it on GitHub <#9 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADAJPHGAJZ3LE2IV4DIOIDZZHD4ZAVCNFSM6AAAAABPEEXN3CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMZYG4YDEOBVHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[prolic]

@ProofOfKeags done. Please double check, I hope I didn't miss anything.

[ProofOfKeags]

This is likely to cause memory safety issues as is. I'm surprised the tests pass. If you get burned out on this PR I can take it over. Also happy to keep giving feedback and getting it to where it needs to be.

[prolic]

Sorry, I actually can't do any serious C 🤣. But I'll give it another shot tomorrow if you don't get to it before me.

…

On Tue, Oct 1, 2024, 22:13 ProofOfKeags ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Crypto/Secp256k1.hs <#9 (comment)> : > @@ -756,7 +756,9 @@ schnorrSign mGen KeyPair{..} bs lift $ do sigBuf <- mallocBytes 64 randomPtr <- case mGen of - Just gen -> fmap castPtr $ BS.useAsCString (BS.pack $ Prelude.take 32 $ randoms gen) return + Just gen -> do + let randomBytes = BS.pack $ Prelude.take 32 $ randoms gen + BS.useAsCStringLen randomBytes $ \(ptr, _) -> pure $ castPtr ptr you still can't do this. castPtr still leaks the underlying CString pointer. What you need to do is use the pointer for the computation you ultimately need. In this case that answer is the schnorrSign function. -- here we define a helper that lets us just supply the final random pointerlet sign = Prim.schnorrSign ctx sigBuf msgHashPtr keyPairPtr ret <- case mGen of Just gen -> do -- Note here that you don't need to go to a list you can randomgen the bytestring directly let randomBytes = BS.fromShort $ genShortByteString 32 gen -- this doesn't leak the pointer because the result is the success code, meaning it is no longer use-after-free BS.useAsCString randomBytes sign Nothing -> sign nullPtrif isSucess ret ... Does what I'm saying make sense? — Reply to this email directly, view it on GitHub <#9 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADAJPHQS32LWA6ZS26VNCTZZNJF5AVCNFSM6AAAAABPEEXN3CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNBRG44TKMZXHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[ProofOfKeags]

Sorry, I actually can't do any serious C 🤣. But I'll give it another shot
tomorrow if you don't get to it before me.

No worries. The main thing to pay attention to is that you don't want to let the pointer escape the scope of the closure. Basically, don't return the pointer out of the closure passed to useAsCString.

[prolic]

@ProofOfKeags Alright, I squashed the last 2 commits. I think I now understand your point and I hope the solution I provided is sufficient.

[ProofOfKeags]

Looks like I can't push to this branch right now despite being a maintainer. This is good but I need two more tiny things to be done and we can merge:

1. please reauthor 52473c8 to include a change to the package.yaml file with the random dependency added. We do not use cabal files directly here. They are autogenerated by the package.yaml
2. please squash (preferably with a fixup) 73758ee and ed56452

[prolic]

@ProofOfKeags pushed

[ProofOfKeags]

I think this is good to go @wraithm do you wanna take a second look?

[prolic]

ping @ProofOfKeags @wraithm

[prolic]

@ProofOfKeags Thank you so much! Any change I can get this on hackage, too?

[ProofOfKeags]

Yes, can do. Give me a day or so.

[prolic]

@ProofOfKeags please relase on hackage ❤️

[ProofOfKeags]

Uploaded at 0.3.0

[prolic]

thank you!