-
Notifications
You must be signed in to change notification settings - Fork 476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SECP256k1 support #4368
SECP256k1 support #4368
Conversation
It's confusing to refer to all of this as "SECP256k1 support" since SECP256k1 is just a curve. Since you are using secp256k1-haskell, it seems you do not support the Schnorr (BIP-340) signatures recently added to Bitcoin. So I suspect that you have in fact added ECDSA signature support using the SECP256k1 curve. Is that correct? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Haskell part looks great to me, apart from PlutusTx.Builtins.Internal.verifySECP256k1Signature
, which I think could be better.
Can't comment on the crypto part.
verifySECP256k1Signature | ||
:: BS.ByteString -- ^ Public key | ||
-> BS.ByteString -- ^ Signature | ||
-> BS.ByteString -- ^ Message hash | ||
-> Emitter (EvaluationResult Bool) | ||
verifySECP256k1Signature pk sig msg = | ||
case DSIGN.rawDeserialiseVerKeyDSIGN @SECP256k1DSIGN pk of | ||
Nothing -> do | ||
emitM "SECP256k1: Given invalid signing key." | ||
pure EvaluationFailure | ||
Just pk' -> case DSIGN.rawDeserialiseSigDSIGN @SECP256k1DSIGN sig of | ||
Nothing -> do | ||
emitM "SECP256k1: Given invalid signature." | ||
pure EvaluationFailure | ||
Just sig' -> case SECP.msg msg of | ||
Nothing -> do | ||
emitM "SECP256k1: Given invalid message hash." | ||
pure EvaluationFailure | ||
Just msg' -> case DSIGN.verifyDSIGN () pk' msg' sig' of | ||
Left _ -> pure . pure $ False | ||
Right () -> pure . pure $ True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kwxm bad news for ya, we have another emitting bulitin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a problem in some sense I'm not aware of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kozross, not a concern for you, just a pain in the ass to describe in the specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we were really expecting this to be used only in the in the implementation of trace
and not in arbitrary builtins, but I suppose it's harmless and it's commendable to give programmers extra feedback with something that it would be easy to get wrong.
I think from the point of specification, I'd inclined to just not mention this in the specification on the basis that the error messages are a detail specific to this particular implementation. Trying to include the details of the logging mechanism in the specification would complicate things quite a bit, so maybe we could say that an implementation of Plutus Core may choose to provide facilities for logging errors as long as they don't have any effect on the semantics of built-in functions; that's not really got anything to do with this PR though.
Off-topic, this makes me wonder if the logging mechanism could be used to do unexpected things in contract code. Could the off-chain part of a contract run some validation code (also off the chain), look at the log files and then branch on the contents to determine the future behaviour of the contract (which you could do even using trace
)? That might provide a way to leak more information from validation than the true
/false
answer that we expect, but would that actually be a problem?
Ah, yeah, it must conflict horribly with my recent refactoring, I'll fix it for you. |
…to mlabs-haskell-koz/secp256k1
Done (not horribly at all). |
Merge `master` into `mlabs-haskell-koz/secp256k1` and fix what got broken
For some reason it won't let me respond to the existing comments... I agree that it's harmless to include tracing here. It's interesting that this is the first builtin other than I'm not worried about specifying the tracing. We deliberately don't specify anything to do with log emitting since it's not observable in the main behaviour of the program.
Yes, but you can do whatever you want off-chain. You can run your PLC through a symbolic executor and buy a sandwich iff it returns true on Tuesday 22nd 2024. So I don't think we need to care about that. |
I suppose there's not much that can go wrong with most of the existing builtins (mostly out of range errors), so there's no need give details when something goes wrong. Maybe we could be more informative with some of the other ones though. I 'll have to benchmark this for costing purposes at some point and while I'm doing that I'll check that the logging isn't adding any overhead. It's highly unlikely that it will be since nothing at all should be happening on chain, but there's no harm checking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@@ -786,6 +791,7 @@ instance Flat DefaultFun where | |||
Sha3_256 -> 19 | |||
Blake2b_256 -> 20 | |||
VerifySignature -> 21 | |||
VerifySECP256k1Signature -> 51 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bid odd seeing the results in non-sequential order, but I see the point. We probably don't want to do this too much though, or it'll be hard so spot gaps and duplicates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, maybe that should go at the end. If someone else wants to add a new builtin they might look at the end of the list and think that 51's the next free tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So do you suggest I use the largest number available for VerifySECP256k1Signature
instead of 51
? There's no specified system of numbering these in the documentation of the module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I meant that if you look at the end of that list of cases it says
UnListData -> 44
UnIData -> 45
UnBData -> 46
EqualsData -> 47
MkPairData -> 48
MkNilData -> 49
MkNilPairData -> 50
so it would be easy to think that the next free number is 51. Of course if we arrange them numerically then it moves VerifySECP256k1Signature
away from the rest of the cryptographic builtins, so you get a different kind of confusion.
_ -> False) | ||
|
||
-- All data needed for an erroring case | ||
data SECP256k1ErrorCase = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that these are all in the order key/message/signature, which is different from the order of the input arguments. (Not a complaint, just a remark!)
plutus-tx/src/PlutusTx/Builtins.hs
Outdated
-- appropriate form and length for SECP256k1. This function will error if any of | ||
-- these are not the case. | ||
verifySECP256k1Signature | ||
:: BuiltinByteString -- ^ Public key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the comments here please say what lengths the input bytestrings should be, just so that it's documented here without having to consult the library? I was sure we had a comment about the valid inputs for verifySignature
, but embarrassingly I can't find it; I'll fix that at the next opportunity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
SECP.Msg -> | ||
Gen SECP256k1NoErrorCase | ||
mkWrongKey sk pk msg = do | ||
pkBad <- Gen.filter (/= pk) genPubKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very conscientious! Let's hope this filter isn't coming into play very often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very unlikely. Assuming the generator is 'fair' (that is, any given key is equally likely to be generated), the probability that this filter would be tripped is 1 over (2^8)^32, due to pubkey lengths.
plutus-tx/src/PlutusTx/Builtins.hs
Outdated
{-# INLINEABLE verifySECP256k1Signature #-} | ||
-- | Given a SECP256k1 public key, a SECP256k1 signature, and a SECP256k1 | ||
-- message hash (all as 'BuiltinByteString's), verify the hash with that key and | ||
-- signature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, here the "message" is always a 32-byte hash of the real message (unlike verifySignature
, which can take a message of any length). Does that hash have to be produced by any particular method, or will anything do? I'm wondering it we're ever going to have to produce the hashes on chain (which might require more new bultins), or if for real world applications it'll always be possible to generate the hashes off-chain and just perform the verification on the chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, this is a concern outside of this builtin, cardano-base
and even secp256k1-haskell
! Specifically, secp256k1-haskell
assumes you can somehow obtain this hash, and that's as far as its concerns go.
-> Bool | ||
verifySECP256k1Signature pk sig msg = | ||
fromBuiltin (BI.verifySECP256k1Signature pk sig msg) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to suggest that this should take its arguments in the order key/message/signature for consistency with verifySignature
, but I see that the argument order in both functions matches that of the underlying library function, and their arguments are in different orders. It looks as if we're doomed to inconsistency whatever we do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some digging through the repositories (with @michaelpj 's help) reveals that there is a DSIGN interface for Ed255911 as well. We should probably switch to using that for verifySignature
instead, but unfortunately we won't be able to rearrange the arguments for reasons of backward compatibility. Anyway, the order above seems the right one for things using DSIGN
and we should probably use the same order for any further signature checking functions we implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that being consistent in some sense is better here, but this is a concern far outside this PR. I picked the argument order the way I did for precisely the reason you said: by matching the underlying library function, I minimize the amount of mistakes I would make by forgetting which argument goes where.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
precisely the reason you said: by matching the underlying library function
I think I was wrong about that: see the comment in Crypto.hs. I'm thoroughly confused now though.
emitM "SECP256k1: Given invalid message hash." | ||
pure EvaluationFailure | ||
Just msg' -> case DSIGN.verifyDSIGN () pk' msg' sig' of | ||
Left _ -> pure . pure $ False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand all the stuff in DSIGN
, but I see that the _
is a string. Is that something that would be worth logging in the Left
case, or is it stuff that should be caught by the earlier checks anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's nothing to log; the implementation in secp256k1-haskell
produces a Bool
only.
@ysangkok Thank you for bringing this to our attention. We do indeed need Schnorr signature support; therefore, I'm marking this PR as WIP for now. I'll keep it current with @michaelpj and @kwxm: I have attempted, based on these instructions here and your feedback here, to generate a cost model for |
@kozross Please undo this!!!! As mentioned in the second link above, these benchmarks MUST be run on a specific machine which at the moment only we have access to (unfortunately, but that's hard to change and there may even be legal difficulties because the machine is provided to us by an external party and we may be limited in who's allowed to access it). The changes to We can generate a suitable cost model once this PR is merged. In the meantime I did send an email about this to the address on your GitHub account a few days ago, but I'm not sure if you got it. |
@kwxm OK, I'll revert the change. I did receive your email, but the 'reference machine' part must have eluded me. |
baa97fb
to
4693f32
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Sorry, I was trying to leave some comments and I seem to have started another review.)
Nothing -> do | ||
emitM "SECP256k1: Given invalid message hash." | ||
pure EvaluationFailure | ||
Just msg' -> case DSIGN.verifyDSIGN () pk' msg' sig' of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait! verifySECP256k1Signature
takes its arguments in the order pk/sig/msg, but DSIGN.verifyDSIGN
takes them in the order pk'/msg'/sig': see also the source of the instance here. Earlier I thought both of these took their arguments in the same order. Would it be reasonable to change the order of the arguments in the main function? That would make it consistent with the order expected by DSIGN
(and also with the existing verifySignature
function), which would be helpful if we ever implement other signature schemes covered by DSIGN
.
-> Bool | ||
verifySECP256k1Signature pk sig msg = | ||
fromBuiltin (BI.verifySECP256k1Signature pk sig msg) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
precisely the reason you said: by matching the underlying library function
I think I was wrong about that: see the comment in Crypto.hs. I'm thoroughly confused now though.
Thanks! I'm afraid that this situation's not very satisfactory, but it's not clear what else we can do at the moment. Eventually there'll have to be some community process for this kind of thing, but that's probably quite a long way off at the moment. |
I'm glad we discovered the confusion about the signature algorithm before we merged this! @kozross I'll let you work out what it is that you actually need, I guess the only thing to bear in mind for coming back to this PR is that we should indeed include the algorithm name as well as the curve in the name of the builtin, so we don't get confused in future. |
Now the build issues are sorted out I think we can return to this. I'm not totally clear where we are on it: I think we're basically good to go? The failing builds are darwin (we have build machine issues, so it's disabled on master); merging master should fix that. And there are a few conflicts. |
@michaelpj I've resolved the conflicts. Other than this, I believe the work is done: I have addressed reviewer concerns, I'm not aiming at any branches (such as for Edit: Spoke too soon, it would seem. The parser error is really strange here, for two reasons:
Given that it first generates a |
@@ -282,6 +284,8 @@ defineBuiltinTerms = do | |||
defineBuiltinTerm 'Builtins.lessThanEqualsByteString $ mkBuiltin PLC.LessThanEqualsByteString | |||
defineBuiltinTerm 'Builtins.emptyByteString $ PIR.mkConstant () BS.empty | |||
defineBuiltinTerm 'Builtins.decodeUtf8 $ mkBuiltin PLC.DecodeUtf8 | |||
defineBuiltinTerm 'Builtins.verifyEcdsaSecp256k1Signature $ mkBuiltin PLC.VerifyEcdsaSecp256k1Signature | |||
defineBuiltinTerm 'Builtins.verifySchnorrSecp256k1Signature $ mkBuiltin PLC.VerifyEcdsaSecp256k1Signature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defineBuiltinTerm 'Builtins.verifySchnorrSecp256k1Signature $ mkBuiltin PLC.VerifyEcdsaSecp256k1Signature | |
defineBuiltinTerm 'Builtins.verifySchnorrSecp256k1Signature $ mkBuiltin PLC.VerifySchnorrSecp256k1Signature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed that (as well as the attendant golden test), but the issue with the parser roundtripping still happens.
The error message is hard to read because megaparsec errors don't (@thealmarty can we make this list auto-generated so this doesn't happen in future? I think we should just be able to enumerate |
(It's how it used to be. Not only doing this automatically is way more convenient, but also allows us to handle sets of built-in functions other than |
Ah yes, sorry about that. @effectfully mentioned that before the merge too. I will push a fix. |
@michaelpj Yeah, I believe it used to work this way, and must have changed without my noticing. |
I can't seem to reproduce this failure locally:
I had to repair a similar issue before, but on my local copy of my branch (which I just pushed), this test passes:
|
I agree it's weird, but I suspect you can just revert the change contained in your PR, I suspect it will accept the old one. |
That's a static test I don't see why it would do that either. Anyway lmk if you think the parser is suspicious. It seems my fix works. |
@thealmarty - your fix looks like it worked. I also have no idea why this test fails. |
What a saga. Thanks! |
* Initial core support for SECP256k1 verification * Add SECP256k1 builtins for plutus-tx * Ensure tests for SECP256k1 pass * Use upstream cardano-base again * Fix a semantic conflict, fix 'stack' * Use EvaluationResult instead of Identity for Emit tracing * Attempt to bump haskell.nix * Try bumping nixpkgs * Ensure everything builds with newer cardano-base * Schnorr verification builtin and tests * Rename old SECP builtin for clarity, add Schnorr builtin * Add note about sequencing, Emitter and EvaluationFailure * Aim at cardano-base master for Schnorr support * Ensure Windows can find libsecp256k1 * Fix parser * Remove unused import * Fix plugin * Fix extra * Fix Windows build issues, update to use new Emitter * Renumber builtins to avoid clashes * Ensure the SECP builtins are shown as available * Ensure plutus-tx-plugin names SECP builtins properly * Roll back deconstructorData2 golden test Co-authored-by: effectfully <effectfully@gmail.com> Co-authored-by: Michael Peyton Jones <michael.peyton-jones@iohk.io>
* Initial core support for SECP256k1 verification * Add SECP256k1 builtins for plutus-tx * Ensure tests for SECP256k1 pass * Use upstream cardano-base again * Fix a semantic conflict, fix 'stack' * Use EvaluationResult instead of Identity for Emit tracing * Attempt to bump haskell.nix * Try bumping nixpkgs * Ensure everything builds with newer cardano-base * Schnorr verification builtin and tests * Rename old SECP builtin for clarity, add Schnorr builtin * Add note about sequencing, Emitter and EvaluationFailure * Aim at cardano-base master for Schnorr support * Ensure Windows can find libsecp256k1 * Fix parser * Remove unused import * Fix plugin * Fix extra * Fix Windows build issues, update to use new Emitter * Renumber builtins to avoid clashes * Ensure the SECP builtins are shown as available * Ensure plutus-tx-plugin names SECP builtins properly * Roll back deconstructorData2 golden test Co-authored-by: effectfully <effectfully@gmail.com> Co-authored-by: Michael Peyton Jones <michael.peyton-jones@iohk.io>
This closes #4233, together with the relevant
cardano-base
PR.Currently, this PR depends on the MLabs fork ofcardano-base
; once the linked PR merges, I will amend to work againstmaster
again. This shouldn't affect review, as while this depends on functionality fromcardano-base
, nothing in here is affected by precisely what branch ofcardano-base
we pull from.Edit: This is now done.
I've added some property-based tests using Hedgehog to ensure all possible paths are equally covered. Specifically, we check each of the following:
As per @kwxm's feedback on my prior work on
plutus-tx
, I've raised the test count high enough to ensure that fewer than 1 in a 1000 executions fail due to probabilistic coverage failures (a flaw in Hedgehog, and one we can't avoid).A few concerns remain:
BuiltinByteString
s. I didn't know if we wanted helpers (inplutus-tx
or elsewhere) to make this process less nasty withnewtype
wrappers and helper functionality, so I haven't added it.untyped-plutus-core
are far too few in number: almost everything in there should be a property test, but even those which are run 200 tests, which is too few to indicate anything. For a demonstration of why: I found multiple bugs in how I implemented the SECP256k1 builtin only after over a thousand tests, and even then it took several attempts! Furthermore, coverage is not checked at all, which is alarming. The tests I wrote for the SECP256k1 primitive are vulnerable to neither.mempty
. I don't know how much of an issue this is.