change secp256k1-haskell to libsecp256k1 #44

ProofOfKeags · 2024-04-22T18:28:42Z

Here we update the secp256k1 dependency to the new one. Turns out it revealed some bugs in that implementation!

I tried to take good care to make sure nothing broke, I would appreciate specific review around how I tweaked the taproot code using the new PubKeyXO construct as opposed to the newtype wrapper we used to use in this library.

ProofOfKeags · 2024-04-22T18:30:16Z

bitcoin/src/Bitcoin/Crypto/Signature.hs

+signHash :: SecKey -> Hash256 -> Maybe Signature
+signHash k = ecdsaSign k . fromShort . getHash256


Also I'd like some feedback here around whether the very low probability of signature failure should be modeled as a Maybe or if we should be naughty and just make it a runtime error.

Why not both? I think we definitely want the "main" version to be pure and handle the error case properly. Because of laziness, impure errors are especially dangerous, IMO. However, we can also export signHashUnsafe (or something) which calls throw.

My goto for this used to be decodeUtf8 and decodeUtf8' from text, but after a recent change, you have to explicitly say what error treatment you want.

Cool. I ask only because I updated the core libsecp256k1 function to reflect the failure possibility. However, pulling from the C Lib docs:

/** Create an ECDSA signature. * * Returns: 1: signature created * 0: the nonce generation function failed, or the private key was invalid. * Args: ctx: pointer to a context object, initialized for signing (cannot be NULL) * Out: sig: pointer to an array where the signature will be placed (cannot be NULL) * In: msg32: the 32-byte message hash being signed (cannot be NULL) * seckey: pointer to a 32-byte secret key (cannot be NULL) * noncefp:pointer to a nonce generation function. If NULL, secp256k1_nonce_function_default is used * ndata: pointer to arbitrary data used by the nonce generation function (can be NULL) * * The created signature is always in lower-S form. See * secp256k1_ecdsa_signature_normalize for more details. */

The two failure conditions are either:

that the private key is invalid which we should be able to exclude using the importSecKey function

nonce generation failed and we currently leave that up to the C-libsecp256k1 default (which itself currently uses RFC6979), so I'm fairly certain that cannot fail.

So it makes me wonder if I can actually get away with ditching the Maybe here.

I lean towards ditching the Maybe if there isn't a real case of failure. It sounds like this library, if used correctly, would never result in a Maybe.

I think that's right. I'll go ahead and update this PR to reflect that.

ProofOfKeags · 2024-04-22T18:33:47Z

bitcoin/src/Bitcoin/Transaction/Taproot.hs

@@ -145,34 +121,34 @@ leafHash leafVersion leafScript =

 -- | Representation of a full taproot output.
 data TaprootOutput = TaprootOutput
-    { taprootInternalKey :: PubKey
+    { taprootInternalKey :: PubKeyXO


I combed over BIP341 several times to get this to pass the tests. I think this is right but this would benefit from careful review.

Yeah, this seems right. The spec says that the internal key is the 32 byte x-only keys.

ProofOfKeags · 2024-04-22T18:34:58Z

bitcoin/src/Bitcoin/Transaction/Taproot.hs

+    onComputedKey computedKey computedParity =
+        fst (xyToXO outputKey) == computedKey
+            && expectedParity == computedParity


This is probably the trickiest part and could make the difference between neutering the test and updating it correctly.

wraithm · 2024-05-02T18:14:53Z

@ProofOfKeags looks like there are some merge conflicts now, also some minor fourmolu things. This is looking great though! Very exciting.

ProofOfKeags · 2024-05-02T19:20:34Z

Will rebase this evening and see if we can make the red green.

wraithm · 2024-05-22T16:03:27Z

bitcoin-test/lib/Bitcoin/Crypto/SignatureSpec.hs

@@ -201,7 +204,7 @@ validImplMap =


 getImpl :: Maybe ValidImpl
-getImpl = implSig `Map.lookup` validImplMap
+getImpl = pure ImplCore


Should we just completely get rid of the ImplABC stuff? Maybe that needs to be in a separate/follow-up PR.

Yeah that's my plan for my next PR actually

wraithm · 2024-05-22T16:09:42Z

bitcoin-test/lib/Bitcoin/Orphans.hs

@@ -14,15 +14,16 @@ import Bitcoin (
    OutPoint (OutPoint),
    ParsedPath (..),
    PubKeyI,


Random comment:

Upstream secp256k1 actually did a bunch of renames for the various types and stuff. I liked a lot of those changes/renames. We should consider adopting some of those name changes in our fork.

Do you have a list of the ones you like?

wraithm · 2024-05-22T16:12:37Z

bitcoin/src/Bitcoin/Keys/Common.hs

-    tweakAddPubKey,
-    tweakAddSecKey,
- )
+import Crypto.Secp256k1 (PubKeyXY, SecKey, derivePubKey, exportPubKeyXY, exportSecKey, importPubKeyXY, importSecKey, importTweak, pubKeyTweakAdd, secKeyTweakAdd)


nit: I prefer the vertical layout. Is there a reason to have these long lines like this?

I think that was a choice that fourmolu made. I'll see if I can nudge it to a vertical layout. I agree that vertical is better but the thing I care most about is that this is handled automatically by the formatter.

wraithm · 2024-05-22T16:14:32Z

bitcoin/src/Bitcoin/Transaction/Partial.hs

@@ -360,7 +360,7 @@ onPrevTxOut net signer tx ix input prevTxData =
  where
    newSigs = HM.mapWithKey sigForInput sigKeys
    sigForInput thePubKey theSecKey =
-        encodeTxSig . makeSignature net tx ix theSigInput $
+        maybe (error "Signature Gen Failed") encodeTxSig . makeSignature net tx ix theSigInput $


Hm. Calling error in the library code isn't great. Is this known to never happen?

Yeah this is one of those consequences of changing the signing function to return a maybe. It looks like you're already aware of this. I'll respond in that thread.

wraithm · 2024-05-22T16:15:13Z

bitcoin/src/Bitcoin/Transaction/Taproot.hs

 taprootCommitment internalKey merkleRoot =
    BA.convert
        . hashFinalize
        . maybe id (flip hashUpdate) merkleRoot
        . (`hashUpdates` BSL.toChunks keyBytes)
        $ initTaggedHash "TapTweak"
  where
-    keyBytes = Bin.encode $ XOnlyPubKey internalKey
+    keyBytes = BSL.fromStrict $ exportPubKeyXO $ internalKey


got an hlint here

wraithm · 2024-05-22T16:16:52Z

stack.yaml

 nix:
  packages:
-    - secp256k1


Don't we still need secp256k1 here?

Ah, yeah this is a good question. I removed this because the default nix build doesn't actually ship all of the headers. Maybe we can try and fix the nix build to include all of the modules. I can try adding it back in and reporting back with what errors end up firing. I have forgotten what they were, but IIRC they aren't fatal to the build process but they look very noisy and sus.

wraithm · 2024-05-22T16:21:26Z

bitcoin/bitcoin.cabal

@@ -5,7 +5,7 @@ cabal-version: 1.12
 -- see: https://github.com/sol/hpack

 name:           bitcoin
-version:        0.1.0
+version:        0.1.1


nit: Should this not be a major version bump? I'd also be okay with not bumping the version because we haven't really made an official release yet, have we?

Definitely. This is a mistake. Originally I was going to avoid changing the bitcoin api altogether but as this evolved there are tiny changes to it. I'll bump to 0.2.0. I don't mind aggressively bumping version numbers to ensure that we don't forget to do so.

ProofOfKeags requested review from wraithm and GambolingPangolin April 22, 2024 18:28

ProofOfKeags commented Apr 22, 2024

View reviewed changes

ProofOfKeags mentioned this pull request Apr 22, 2024

Feature/new secp #31

Closed

ProofOfKeags force-pushed the dependency/secp256k1 branch from 6c048dd to c23d0a3 Compare May 3, 2024 01:40

change secp256k1-haskell to libsecp256k1

20e26cc

ProofOfKeags force-pushed the dependency/secp256k1 branch from c23d0a3 to 20e26cc Compare May 3, 2024 02:07

ProofOfKeags added 5 commits May 2, 2024 19:23

update fourmolu

1e5e5c9

update fourmolu version

43d968e

commit fourmolu diff

64a0760

update secp256k1 library to deal with deprecations

4a3cbe4

remove secp256k1 from nix dependencies

4b96587

ProofOfKeags force-pushed the dependency/secp256k1 branch 4 times, most recently from 486dfd1 to 40a3087 Compare May 3, 2024 18:46

Enable secp256k1 recovery module in Windows build

a4b527a

ProofOfKeags force-pushed the dependency/secp256k1 branch from 40a3087 to a4b527a Compare May 3, 2024 20:38

wraithm reviewed May 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change secp256k1-haskell to libsecp256k1 #44

change secp256k1-haskell to libsecp256k1 #44

ProofOfKeags commented Apr 22, 2024

ProofOfKeags Apr 22, 2024

GambolingPangolin Apr 28, 2024

ProofOfKeags Apr 30, 2024

wraithm May 22, 2024

ProofOfKeags May 22, 2024

ProofOfKeags Apr 22, 2024

wraithm May 22, 2024

ProofOfKeags Apr 22, 2024

wraithm commented May 2, 2024

ProofOfKeags commented May 2, 2024

wraithm May 22, 2024

ProofOfKeags May 22, 2024

wraithm May 22, 2024

ProofOfKeags May 22, 2024

wraithm May 22, 2024

ProofOfKeags May 22, 2024

wraithm May 22, 2024

ProofOfKeags May 22, 2024

wraithm May 22, 2024

wraithm May 22, 2024

ProofOfKeags May 22, 2024

wraithm May 22, 2024

ProofOfKeags May 22, 2024

		signHash :: SecKey -> Hash256 -> Maybe Signature
		signHash k = ecdsaSign k . fromShort . getHash256

change secp256k1-haskell to libsecp256k1 #44

Are you sure you want to change the base?

change secp256k1-haskell to libsecp256k1 #44

Conversation

ProofOfKeags commented Apr 22, 2024

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

wraithm commented May 2, 2024

ProofOfKeags commented May 2, 2024

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