SECP256k1 support

[kozross]

This is the first stage of solving this issue, as per this request.

A few questions remain outstanding:

• The size of the verification key (that is, public key) is not an exact multiple of 8 bits, as it's compressed and DER-encoded, for a total of 257 bits. I've thus set it to be 33 bytes in the implementation, but I am unsure if this is correct.
• Secret key generation has to be implemented partially, as the original implementation returns in a Maybe. According to the implementation, we ensure that the only invariant is met, but this seems suspicious.

Tagging @michaelpj for review.

[tdammers]

Could you also add this new DSIGN algorithm to the test suite? It may not be obvious, but if you look in the cardano-crypto-tests subproject, there's a file src/Test/Crypto/DSIGN.hs, and adding the new algorithm is as simple as adding a line to the tests data structure. You can pretty much just copy one of the existing lines and change the algorithm name to SECP256k1.

[kozross]

@tdammers I actually can't add the tests, due to this error:

src/Test/Crypto/DSIGN.hs:37:7: error:
    • Couldn't match type ‘(~)
                             secp256k1-haskell-0.5.0:Crypto.Secp256k1.Msg’
                     with ‘SignableRepresentation’
        arising from a use of ‘testDSIGNAlgorithm’
    • In the expression:
        testDSIGNAlgorithm (Proxy :: Proxy SECP256k1DSIGN) "SECP-256k1"
      In the second argument of ‘testGroup’, namely
        ‘[testDSIGNAlgorithm (Proxy :: Proxy MockDSIGN) "MockDSIGN",
          testDSIGNAlgorithm (Proxy :: Proxy Ed25519DSIGN) "Ed25519DSIGN",
          testDSIGNAlgorithm (Proxy :: Proxy Ed448DSIGN) "Ed448DSIGN",
          testDSIGNAlgorithm (Proxy :: Proxy SECP256k1DSIGN) "SECP-256k1"]’
      In the expression:
        testGroup
          "Crypto.DSIGN"
          [testDSIGNAlgorithm (Proxy :: Proxy MockDSIGN) "MockDSIGN",
           testDSIGNAlgorithm (Proxy :: Proxy Ed25519DSIGN) "Ed25519DSIGN",
           testDSIGNAlgorithm (Proxy :: Proxy Ed448DSIGN) "Ed448DSIGN",
           testDSIGNAlgorithm (Proxy :: Proxy SECP256k1DSIGN) "SECP-256k1"]
   |
37 |     , testDSIGNAlgorithm (Proxy :: Proxy SECP256k1DSIGN) "SECP-256k1"
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This is caused by the fact that SignableRepresentation is 'too big' a type (class) for what the implementation in secp256k1-haskell can sign: we can only sign an opaque Msg type, which is what that constraint is designed to enforce. I'm happy to repair the tests to deal with this, but it's possible I'm misunderstanding something.

[michaelpj]

It does seem odd to have the Signable type family in the class and then force it to all be the same constraint in the tests. If we want to do that we should maybe just get rid of Signable...

Presumably @kozross there's a way to make a Msg from a ByteString, and SignableRepresentation lets you get a ByteString, so you could use that as your Signable constraint...

[kozross]

@michaelpj Yes, you can make a Msg from a ByteString, but it's subject to constraints on size. Making SECP256k1's Signable be SignableRepresentation is inappropriate: you can't just jam any old ByteString in there, but the very first instance of SignableRepresentation is ByteString!

The primary issue here isn't what the Signable constraint is: the issue is that it must be SignableRepresentation, or the tests can't handle it. Basically, you've got two things at cross purposes here; only one of these assumptions can work presently without something being changed to accommodate the other:

• Signable is meant to be choosable per-algorithm, to work within any specific restrictions of the algorithm or its implementation. This is indeed the case here: we can only sign Msg. If so, the tests are written with inappropriate assumptions and have to be modified not to assume SignableRepresentation everywhere.
• Signable is meant to always be SignableRepresentation, in which case you must either be able to reject inputs as not signable by the algorithm, or forbid anything that restricts what can be signed. In this case, why make the Signable constraint definable in the type class?

I'm happy to fix either issue, but I'd rather know which would be acceptable.

[michaelpj]

I agree with your analysis, and personally I think it would be preferable for the tests to not assume that Signable ~ SignableRepresentation. What do you think @tdammers ?

[kozross]

@michaelpj I've already worked out a fix to the tests which makes this assumption unnecessary. I still have some minor fixes to make so that all the SECP256k1 tests pass, but once that's done, it should be ready.

At the same time, I've noticed some concerning issues with the tests themselves:

1. The tests are not thread safe. This is strange, considering that they're built with -threaded, but if you actually try making them run in parallel (by adding -rtsopts -with-rtsopts=-N), you start to get nondeterministic failures.
2. Only 100 tests per property are run, which is far too few to indicate absolutely anything.
3. No shrinking is being done anywhere, which makes counter-examples huge and rather difficult to work with.
4. The tests run very slowly, which suggests performance issues. 100 tests per property should not take as long as this does.

I suspect there may be more issues: these are just the ones I've spotted in the DSIGN tests.

[michaelpj]

I noticed you made some changes to the sizes, was that because they were wrong?

[michaelpj]

LGTM, so long as @tdammers is happy with the test changes.

[kozross]

I noticed you made some changes to the sizes, was that because they were wrong?

Yes: the tests also caught me, so it's good that @tdammers told me about them.

[michaelpj]

Conflicts with the other PR.

[kozross]

@michaelpj I've addressed the conflicts - is this OK to merge?

[michaelpj]

Tobias said he wanted to have a look at this today, so I'm waiting for that.

[tdammers]

This isn't going to break anything, but I'm not 100% convinced that this is the best possible design here.

[tdammers]

Why is it necessary to also provide a generalization of genVK and genSK, when we never use it? Can we not simply pass a Gen (SigDSIGN v)?

[tdammers]

Oh, and this design also prevents shrinking, doesn't it?

[kozross]

We do use them. Consider line ranges 167-173, 180-187, 195-201, 208-209, 213-214, 218-223 at least. We also have several derived generators using one or both.

As far as preventing shrinking goes - we can't shrink anyway for almost anything, and indeed, the original design also prevented shrinking, since all shrinkers were defined as const []. My changes have not removed any functionality or capability that existed before, but added plenty.

[tdammers]

Ah, sorry about the confusion - yes, we are using the (quasi-)methods, but we are not using the ability to generalize - all the methodologies we define here use the same implementations for genVerKey and genSignKey, so we might as well just use default{Ver,Sign}KeyGen.

[michaelpj]

If we did what I suggest below then we would need genSignKey, but perhaps still not genVerKey. Not sure it costs us much, though.

[kozross]

@tdammers I'm happy to specialize in the way you describe. I don't think it's a particularly bad idea to have the ability to generalize in the way the verification and sign keys are generated, even if we don't use it currently, but I'm not strongly wedded to this.

Edit: This is now done.

[tdammers]

I totally understand the thinking here, it just tickles my YAGNI nerve. I would personally start with only generalizing what needs to be generalized, which, at this point, is only genSig bit.

And, upon further thought, what we're really generalizing here isn't even generating a signature, but generating a message to sign. If you compare the defSigGen function with the go function inside the secp256k1Methodology, the only difference is how the message is generated.

So what I would propose is that we ditch the TypesMethodology type entirely, and instead just pass a generator for the message type (genSECPMsg for secp256k1, arbitrary for everything else), and then implement genSig in terms of that.

[michaelpj]

If we're going to do this, arguably we could get rid of the key generation method from the DSIGN class, since it's presumably only used for testing, and this lets us give the generators per-type as we need.

[kozross]

This is potentially quite a breaking change, so I'm reluctant to do this unless absolutely necessary. I've fixed Tobias' original over-generality concern without this, but if you feel a change to DSIGN is needed to get this merged, I can do it.

[michaelpj]

Nope, not necessary, it just seems ugly to me to have the generator in there.

[tdammers]

Remaining complaints from my side are largely cosmetic; as far as I'm concerned, we can merge now, and maybe bikeshed later.

[michaelpj]

👍

[iquerejeta]

Naming in this PR is again incorrect. We should not use secp256k1SigGen, or more generally, we should not use secp256k1 as the name of the signature algorithm, as it is not a signature algorithm. This has been referred to incorrectly since the first issue, and should be resolved. In my opinion, the naming should be corrected not only in the code but also in the issues and PR descriptions.

[kozross]

@iquerejeta What do you suggest instead?

[iquerejeta]

I would suggest using the signature algorithm's name. I'm not sure which one you have implemented here, but if it is ECDSA, I would go with something like EcdsaSecp256k1SigGen, and if its Schnorr, I would use SchnorrSecp256k1SigGen. I think keeping the curve in the name is good, so that the distinction between the same algorithm over different curves is explicit in the naming.

[kozross]

@iquerejeta Thank you - that's a good solution. @michaelpj - can I combine the name-change with the Schnorr signature PR I'm working on, or do you want that done separately?

[michaelpj]

Happy for them both to go together.