Skip to content
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

CIP-0049? | ECDSA and Schnorr signatures in Plutus Core #250

Merged
merged 9 commits into from
Oct 25, 2022

Conversation

kozross
Copy link
Contributor

@kozross kozross commented Apr 26, 2022

This CIP describes the (recently merged) support for ECDSA and Schnorr signatures over the SECP256k1 curve. I've tried to include all the detail I can - any fields I am not sure about, I have left blank.

@michaelpj - does this seem like what you were looking for?

CIP-0042/README.md Outdated Show resolved Hide resolved
@KtorZ KtorZ changed the title SECP proposal CIP-0049? | SECP proposal May 11, 2022
@KtorZ KtorZ changed the title CIP-0049? | SECP proposal CIP-0049? | ECDSA and Schnorr signatures in Plutus Core May 11, 2022
Copy link
Contributor

@iquerejeta iquerejeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I would, however, give a little more information on how the builtins work. I suggested that we add some further details on the serialisation format. I think it would also be useful to provide some detail on how the verification works. For Schnorr, it is sufficient to say that we are compatible with BIP340, and for ECDSA I would put somewhere more explicit that the function takes as input the hash of the message. It is important to insist that the verifier must hash the 'claimed' message before verification, and not accept the hash directly from the signer (maybe point to this explanation of why?)

provides us with improved interoperability with systems based on Bitcoin, but
also compatibility with other interoperability systems, such as Wanchain and
Renbridge, which use SECP256k1 signatures for verification.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Moreover, introducing Schnorr signature verification would enable verification of Schnorr compatible multi/threshold signatures such as [MuSig2](https://eprint.iacr.org/2020/1261.pdf) or [Frost](https://eprint.iacr.org/2020/852) among others.

* A verification function for ECDSA signatures using the SECP256k1 curve; and
* A verification function for Schnorr signatures using the SECP256k1 curve.

These would be based on `libsecp256k1`, a reference implementation of both kinds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
These would be based on `libsecp256k1`, a reference implementation of both kinds
These would be based on `[bitcoin-core/libsecp256k1](https://github.com/bitcoin-core/secp256k1)`, a reference implementation of both kinds


More specifically, Plutus would gain the following primitive operations:

* ```verifyEcdsaSecp256k1Signature :: BuiltinByteString -> BuiltinByteString ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't parse nicely. I believe we only need one `

Suggested change
* ```verifyEcdsaSecp256k1Signature :: BuiltinByteString -> BuiltinByteString ->
* `verifyEcdsaSecp256k1Signature :: BuiltinByteString -> BuiltinByteString ->

More specifically, Plutus would gain the following primitive operations:

* ```verifyEcdsaSecp256k1Signature :: BuiltinByteString -> BuiltinByteString ->
BuiltinByteString -> BuiltinBool```, for verifying 32-byte message hashes signed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
BuiltinByteString -> BuiltinBool```, for verifying 32-byte message hashes signed
BuiltinByteString -> BuiltinBool`, for verifying 32-byte message hashes signed

BuiltinByteString -> BuiltinBool```, for verifying 32-byte message hashes signed
using the ECDSA signature scheme on the SECP256k1 curve; and
* ```verifySchnorrSecp256k1Signature :: BuiltinByteString -> BuiltinByteString
-> BuiltinByteString -> BuiltinBool```, for verifying arbitrary binary messages
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

correspond to the result produced by
[``secp256k1_xonly_pubkey_serialize``](https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1_extrakeys.h#L61).
* The message can be of any length, and can contain any bytes in any position.
* The signature must be 64 bytes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* The signature must be 64 bytes.
* The signature must be 64 bytes. Again this follows the bip340 standard, and the 64 bytes correspond to:
* 32 bytes for the big-endian representation of the `x` coordinate of `R`, and
* 32 bytes for the big-endian representation of `s`


We consider the implementation trustworthy: `libsecp256k1` is the reference
implementation for both signature schemes, and is already being used in
production by Bitcoin.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it the reference implementation for both signature schemes over secp256k1? And regarding being used in production by bitcoin, I think it is of interest to say that both primitives have been used in bitcoin. ECDSA before taproot and Schnorr BIP340 after that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fairly sure that it is the reference implementation for both. I agree about the history of use notes, will add those.


* It requires 'rolling your own crypto', rather than re-using existing
implementations. This has been shown historically to be a bad idea;
furthermore, if existing implementations have undergone review and audit, and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
furthermore, if existing implementations have undergone review and audit, and
furthermore, if existing implementations have undergone review and audit,

@@ -1,4 +1,3 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this intentional?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this seems like an error.

@@ -0,0 +1,153 @@
CIP: ?0043
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are missing the --- before the first line for this to parse correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed I am! This is because of a dirty merge.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, rename the supporting folder and change the CIP number to 49 (without leading-zeros, otherwise Github markdown renderer has the brilliant idea to interpret those numbers in octal base 😬 ).

Copy link
Contributor

@iquerejeta iquerejeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just some minor comments.


* The verification key must correspond to the _(x, y)_ coordinates of a point
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bullet point and the next one might confuse the reader, as we say that a verification key "must correspond" to two different format. I would differentiate this bullet point with the next one using phrasing as follows:

* A verification key corresponds to the _(x, y)_ coordinates of a point in the SECP256k1 curve, 
where _x, y_ are unsigned integers in big-endian form.
* The serialised (compressed) verification key format accepted by the builtins must correspond [...]

* The input to verify must be a 32-byte hash of the message to be checked. We
assume that the caller of `verifyEcdsaSecp256k1Signature` receives the
message and hashes it, rather than accepting a hash directly: doing so
[can be dangerous](https://bitcoin.stackexchange.com/a/81116/35586).
Typically, the hashing function used would be SHA256; however, this is not
required, as only the length is checked.
* The signature must correspond to two unsigned integers in big-endian form;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. In this case it is a bit less confusing as both formats match, but maybe I would again rephrase in that the first bullet point is what the signature is, and the second is the compressed format accepted by the builtins.

* The verification key must follow the
[BIP-340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki)
standard for encoding, as emboded by the [``secp256k1_xonly_pubkey_serialize``](https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1_extrakeys.h#L61).
* The verification key must correspond to the _(x, y)_ coordinates of a point
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as above. Probably similar phrasing to that of the ECDSA verification keys would be good.

For the [ECDSA signature
scheme](https://en.bitcoin.it/wiki/Elliptic_Curve_Digital_Signature_Algorithm),
the requirements are as follows. Note that these differ from the serialization
used by Bitcoin.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that these differ from the serialization used by Bitcoin

Why is that? May we justify this choice under the Rationale section?

This implies all of the following:
* The signature is 64 bytes long.
* The first 32 bytes are the bytes of _r_.
* The last 32 bytes are the bytes of _s_.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: some visual support to summarize the few points above.

┏━━━━━━━━━━━━━━┯━━━━━━━━━━━━━━━┓
┃ r <32 bytes> │ s <32 bytes>  ┃
┗━━━━━━━━━━━━━━┷━━━━━━━━━━━━━━━┛
<--------- signature ---------->

workflow involving interoperability services like Wanchain and Renbridge. An
example of such a workflow would be: given some Cardano assets, their Bitcoin
equivalent could be minted, but only if the correct Schnorr signature can be
provided.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This paragraph seems redundant with the Motivation section.

Plutus](https://github.com/input-output-hk/plutus/pull/4368). Tests of the
functionality have also been included, although costing is currently
outstanding, as it cannot be done by MLabs due to limitations in how costing is
calculated. Costing will instead be done by the Plutus Core team.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 cc @michaelpj, I assume you are aware of this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep

@@ -1,4 +1,3 @@
---
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this seems like an error.

@@ -0,0 +1,153 @@
CIP: ?0043
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, rename the supporting folder and change the CIP number to 49 (without leading-zeros, otherwise Github markdown renderer has the brilliant idea to interpret those numbers in octal base 😬 ).

@rphair
Copy link
Collaborator

rphair commented Sep 13, 2022

@kozross this was on the CIP Editors' Meeting agenda today where it was suggested this proposal is being discussed, audited, or somehow moving forward in other channels. If so, could you please update the status here so editors know how to move forward with this (or what we are waiting for)?

Also can you please renumber the CIP and its directory name as requested here (#250 (comment)), so that we can merge this proposal when it becomes ready?

@iquerejeta
Copy link
Contributor

The bindings for Schnorr and ECDSA signatures are currently being audited by bcryptic. We will soon have a report for it (ETA next week). The bindings have also been merged to cardano-base master (PRs 252, 258, 262 and 289). I'll try to talk with Koz to modify the suggested changes.

@iquerejeta
Copy link
Contributor

@rphair , @KtorZ , is there anything we can do to move this forward?

@rphair
Copy link
Collaborator

rphair commented Oct 18, 2022

@iquerejeta apologies for the distracting references above (from someone else's CIP using your earlier assigned # 49).

We've put this on the agenda as "Last Check" for CIP meeting # 56 (theme: "Plutus core changes") (invite) (2022-10-25 at 08:30 UTC). It would be great if you and/or @kozross could be there: in any case it seems to me (@KtorZ?) like this will be merged unless there are any objections at the meeting.

@iquerejeta
Copy link
Contributor

Great, I'll try to be there! 👍

@rphair rphair merged commit 4c3c4c3 into cardano-foundation:master Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants